lkml.org 
[lkml]   [2016]   [Dec]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v3 3/6] net: stmmac: stmmac_platform: fix parsing of DT binding
    From
    Date
    Hi

    On 12/08/2016 10:46 AM, Niklas Cassel wrote:
    > On 12/08/2016 10:02 AM, Alexandre Torgue wrote:
    >> Hi Niklas
    >>
    >> On 12/07/2016 03:20 PM, Niklas Cassel wrote:
    >>> From: Niklas Cassel <niklas.cassel@axis.com>
    >>>
    >>> commit 64c3b252e9fc ("net: stmmac: fixed the pbl setting with DT")
    >>> changed the parsing of the DT binding.
    >>>
    >>> Before 64c3b252e9fc, snps,fixed-burst and snps,mixed-burst were parsed
    >>> regardless if the property snps,pbl existed or not.
    >>> After the commit, fixed burst and mixed burst are only parsed if
    >>> snps,pbl exists. Now when snps,aal has been added, it too is only
    >>> parsed if snps,pbl exists.
    >>>
    >>> Since the DT binding does not specify that fixed burst, mixed burst
    >>> or aal depend on snps,pbl being specified, undo changes introduced
    >>> by 64c3b252e9fc.
    >>>
    >>> The issue commit 64c3b252e9fc ("net: stmmac: fixed the pbl setting with
    >>> DT") tries to address is solved in another way:
    >>> The databook specifies that all values other than
    >>> 1, 2, 4, 8, 16, or 32 results in undefined behavior,
    >>> so snps,pbl = <0> is invalid.
    >>>
    >>> If pbl is 0 after parsing, set pbl to DEFAULT_DMA_PBL.
    >>> This handles the case where the property is omitted, and also handles
    >>> the case where the property is specified without any data.
    >>>
    >>> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
    >>> ---
    >>> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 +--
    >>> .../net/ethernet/stmicro/stmmac/stmmac_platform.c | 29 +++++++++++-----------
    >>> 2 files changed, 17 insertions(+), 16 deletions(-)
    >>>
    >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
    >>> index b1e42ddf0370..b5188122bc15 100644
    >>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
    >>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
    >>> @@ -1581,8 +1581,8 @@ static int stmmac_init_dma_engine(struct stmmac_priv *priv)
    >>> int atds = 0;
    >>> int ret = 0;
    >>>
    >>> - if (!priv->plat->dma_cfg) {
    >>> - dev_err(priv->device, "DMA configuration not found\n");
    >>> + if (!priv->plat->dma_cfg || !priv->plat->dma_cfg->pbl) {
    >>
    >> How "priv->plat->dma_cfg->pbl" could be equal to 0 if you force it to DEFAULT_DMA_PBL in "stmmac_probe_config_dt" in case of DT doesn't set pbl value?
    >
    > The PCI glue code does not call stmmac_probe_config_dt.
    >
    > Also any glue driver could override the value set by stmmac_probe_config_dt
    > before calling stmmac_dvr_probe. So I guess if we want any trustworthy
    > sanity-checking, it actually has to be done in stmmac_main.c.

    Ok I see, it is more safe. You can add my Acked-by.

    Thanks
    Alex

    >
    >
    >>
    >>
    >>> + dev_err(priv->device, "Invalid DMA configuration\n");
    >>> return -EINVAL;
    >>> }
    >>>
    >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
    >>> index d3b6f92f350a..81800f23a9c4 100644
    >>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
    >>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
    >>> @@ -304,21 +304,22 @@ stmmac_probe_config_dt(struct platform_device *pdev, const char **mac)
    >>> plat->force_sf_dma_mode = 1;
    >>> }
    >>>
    >>> - if (of_find_property(np, "snps,pbl", NULL)) {
    >>> - dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*dma_cfg),
    >>> - GFP_KERNEL);
    >>> - if (!dma_cfg) {
    >>> - stmmac_remove_config_dt(pdev, plat);
    >>> - return ERR_PTR(-ENOMEM);
    >>> - }
    >>> - plat->dma_cfg = dma_cfg;
    >>> - of_property_read_u32(np, "snps,pbl", &dma_cfg->pbl);
    >>> - dma_cfg->aal = of_property_read_bool(np, "snps,aal");
    >>> - dma_cfg->fixed_burst =
    >>> - of_property_read_bool(np, "snps,fixed-burst");
    >>> - dma_cfg->mixed_burst =
    >>> - of_property_read_bool(np, "snps,mixed-burst");
    >>> + dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*dma_cfg),
    >>> + GFP_KERNEL);
    >>> + if (!dma_cfg) {
    >>> + stmmac_remove_config_dt(pdev, plat);
    >>> + return ERR_PTR(-ENOMEM);
    >>> }
    >>> + plat->dma_cfg = dma_cfg;
    >>> +
    >>> + of_property_read_u32(np, "snps,pbl", &dma_cfg->pbl);
    >>> + if (!dma_cfg->pbl)
    >>> + dma_cfg->pbl = DEFAULT_DMA_PBL;
    >>> +
    >>> + dma_cfg->aal = of_property_read_bool(np, "snps,aal");
    >>> + dma_cfg->fixed_burst = of_property_read_bool(np, "snps,fixed-burst");
    >>> + dma_cfg->mixed_burst = of_property_read_bool(np, "snps,mixed-burst");
    >>> +
    >>> plat->force_thresh_dma_mode = of_property_read_bool(np, "snps,force_thresh_dma_mode");
    >>> if (plat->force_thresh_dma_mode) {
    >>> plat->force_sf_dma_mode = 0;
    >>>
    >

    \
     
     \ /
      Last update: 2016-12-08 10:55    [W:2.749 / U:0.356 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site