[PATCH] mmc: sdhci-of-arasan: Properly set corecfg_clockmultipler on rk3399

[Shawn Lin]

在 2016/8/26 16:31, Heiko Stübner 写道:
> Am Freitag, 26. August 2016, 15:50:57 schrieb Shawn Lin:
>> + Heiko
>>
>> 在 2016/8/26 14:38, Ziyuan Xu 写道:
>>> Hi Shawn,
>>>
>>> On 2016年08月26日 11:22, Shawn Lin wrote:
>>>> corecfg_clockmultipler indicates clock multiplier value of
>>>> programmable clock generator which should be the same value
>>>> of SDHCI_CAPABILITIES_1. The default value of the register,
>>>> corecfg_clockmultipler, is 0x10. But actually it is a mistake
>>>
>>> corecfg_clockmultipler==>corecfg_clockmultiplier
>>>
>>>> by designer as our intention was to set it to be zero which
>>>> means we don't support programmable clock generator. So we have
>>>> to made it to be zero on bootloader which seems work fine until
>>>
>>> made==>make
>>>
>>>> now. But now we find a issue that when deploying genpd support
>>>
>>> a issue==>an issue
>>>
>>>> for it, the remove callback will trigger the genpd to poweroff the
>>>> power domain for sdhci-of-arasan which magage the controller, phy
>>>
>>> magage==>manage
>>>
>>>> and corecfg_* stuff.
>>>>
>>>> So when we do bind/unbind the driver, we have already reinit
>>>
>>> do==>doing
>>>
>>>> the controller and phy, but without doing that for corecfg_*.
>>>> Regarding to only the corecfg_clockmultipler is wrong, let's
>>>> fix it by explicitly marking it to be zero when probing. With
>>>> this change, we could do bind/unbind successfully.
>>>>
>>>> Reported-by: Ziyuan Xu <xzy.xu at rock-chips.com>
>>>> Cc: Douglas Anderson <dianders at chromium.org>
>>>> Signed-off-by: Shawn Lin <shawn.lin at rock-chips.com>
>>>> ---
>>>>
>>>>   drivers/mmc/host/sdhci-of-arasan.c | 9 +++++++++
>>>>   1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c
>>>> b/drivers/mmc/host/sdhci-of-arasan.c
>>>> index 0b3a9cf..c1eb263 100644
>>>> --- a/drivers/mmc/host/sdhci-of-arasan.c
>>>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
>>>> @@ -67,10 +67,12 @@ struct sdhci_arasan_soc_ctl_field {
>>>>
>>>>    * accessible via the syscon API.
>>>>    *
>>>>    * @baseclkfreq:    Where to find corecfg_baseclkfreq
>>>>
>>>> + * @clockmultiplier:    Where to find corecfg_clockmultiplier
>>>>
>>>>    * @hiword_update:    If true, use HIWORD_UPDATE to access the syscon
>>>>    */
>>>>
>>>>   struct sdhci_arasan_soc_ctl_map {
>>>>
>>>>       struct sdhci_arasan_soc_ctl_field    baseclkfreq;
>>>>
>>>> +    struct sdhci_arasan_soc_ctl_field    clockmultiplier;
>>>>
>>>>       bool                    hiword_update;
>>>>
>>>>   };
>>>>   @@ -100,6 +102,7 @@ struct sdhci_arasan_data {
>>>>
>>>>     static const struct sdhci_arasan_soc_ctl_map rk3399_soc_ctl_map = {
>>>>
>>>>       .baseclkfreq = { .reg = 0xf000, .width = 8, .shift = 8 },
>>>>
>>>> +    .clockmultiplier = { .reg = 0xf02c, .width = 8, .shit = 0},
>>>>
>>>>       .hiword_update = true,
>>>>
>>>>   };
>>>>   @@ -528,6 +531,12 @@ static int sdhci_arasan_probe(struct
>>>>
>>>> platform_device *pdev)
>>>>
>>>>                       ret);
>>>>
>>>>               goto err_pltfm_free;
>>>>
>>>>           }
>>>>
>>>> +
>>>> +        if (of_device_is_compatible(pdev->dev.of_node,
>>>> +                        "rockchip,rk3399-sdhci-5.1"))
>>>> +            sdhci_arasan_syscon_write(host,
>>>> +                          &soc_ctl_map->clockmultiplier,
>>>> +                          0);
>>>
>>> Variable soc_ctl_map is undefined in sdhci_arasan_probe(), please fix it.
>>
>> Woops, I change the code a bit after generating patch, I will fix it.
>>
>>> Moreover, we can not access grf_emmccore/grf_emmcphy prior to enable
>>> clk_ahb, so do it after clock enable.
>>
>> Hrmm, we configure the grf vai CPU and the clock for accessing
>> grf is aclk_emmc_grf( I saw aclk_emmc_noc is critical and it seems we
>> should treat aclk_emmc_* the same way).
>
> No :-)
>
> The aclk_emmc_grf is (similar to aclk_vio_grf etc) part of the emmc block.
>
> The noc clocks (as far as I understand) are the interconnect clocks that
> supply the link between interconnect and peripheral, so are part of the
> interconnect block.
>
> We don't model the interconnect on any rockchip soc right now, so the noc
> clocks are rightfully marked critical, but we of course do model the emmc
> block, so it should handle its grf clock, similar to how the drm driver is
> (planning to) doing it right now.

Thanks for these details. Seems we should plan to handle this inside the
sdhci-of-arasan for rockchip soc.

>
>
> Heiko
>
>
>


-- 
Best Regards
Shawn Lin