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

["Heiko Stübner" <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>]

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@rock-chips.com>
> >> Cc: Douglas Anderson <dianders@chromium.org>
> >> Signed-off-by: Shawn Lin <shawn.lin@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.


Heiko

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip