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.cas...@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.cas...@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.
>
>
>> + 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;
>>
