Re: [PATCH v2 1/7] mmc: sdhci-pxav3: Fix SDR50 and DDR50 capabilities for the Armada 38x flavor

[Gregory CLEMENT <gregory.clement@free-electrons.com>]

Hi Ulf,

On 28/01/2015 21:36, Ulf Hansson wrote:
> On 23 January 2015 at 11:56, Gregory CLEMENT
> <gregory.clement@free-electrons.com> wrote:
>> According to erratum 'FE-2946959' both SDR50 and DDR50 modes require
>> specific clock adjustments in SDIO3 Configuration register. However,
>> this register was not part of the device tree binding. Even if the
>> binding can (and will) be extended we still need handling the case
>> where this register was not available. In this case we use the
>> SDHCI_QUIRK_MISSING_CAPS quirk remove them from the capabilities.
>>
>> This commit is based on the work done by Marcin Wojtas<mw@semihalf.com>
>>
>> Fixes: 5491ce3f79ee ("mmc: sdhci-pxav3: add support for the Armada 38x SDHCI controller")
>> Cc: <stable@vger.kernel.org> # v3.15+
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
>> ---
>>  drivers/mmc/host/sdhci-pxav3.c | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
>> index ca3424e7ef71..7b07325b4fba 100644
>> --- a/drivers/mmc/host/sdhci-pxav3.c
>> +++ b/drivers/mmc/host/sdhci-pxav3.c
>> @@ -118,6 +118,20 @@ static int mv_conf_mbus_windows(struct platform_device *pdev,
>>         return 0;
>>  }
>>
>> +static int armada_38x_quirks(struct sdhci_host *host)
> 
> Seems like this function can be void instead of always returning 0.

In patch 4 "mmc: sdhci-pxav3: Modify clock settings for the SDR50 and
DDR50 modes", this function can return other values than 0.

I could change the prototype in patch 4, but it would also imply
removing the test of the return value in this patch and adding in back
patch 4. By returning a value in this patch, it reduced the amount of
change over the patches.

But if you still prefer than I this function return void in this
patch, I can do it.


Thanks,

Gregory


> 
>> +{
>> +       host->quirks |= SDHCI_QUIRK_MISSING_CAPS;
>> +       /*
>> +        * According to erratum 'FE-2946959' both SDR50 and DDR50
>> +        * modes require specific clock adjustments in SDIO3
>> +        * Configuration register, if the adjustment is not done,
>> +        * remove them from the capabilities.
>> +        */
>> +       host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1);
>> +       host->caps1 &= ~(SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_DDR50);
>> +       return 0;
>> +}
>> +
>>  static void pxav3_reset(struct sdhci_host *host, u8 mask)
>>  {
>>         struct platform_device *pdev = to_platform_device(mmc_dev(host->mmc));
>> @@ -319,6 +333,9 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
>>                 clk_prepare_enable(pxa->clk_core);
>>
>>         if (of_device_is_compatible(np, "marvell,armada-380-sdhci")) {
>> +               ret = armada_38x_quirks(host);
>> +               if (ret < 0)
>> +                       goto err_quirks;
>>                 ret = mv_conf_mbus_windows(pdev, mv_mbus_dram_info());
>>                 if (ret < 0)
>>                         goto err_mbus_win;
>> @@ -400,6 +417,7 @@ err_mbus_win:
>>         if (!IS_ERR(pxa->clk_core))
>>                 clk_disable_unprepare(pxa->clk_core);
>>  err_clk_get:
>> +err_quirks:
>>         sdhci_pltfm_free(pdev);
>>         return ret;
>>  }
>> --
>> 2.1.0
>>
> 
> Kind regards
> Uffe
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com