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

[Doug Anderson]

Hi,

On Fri, Aug 26, 2016 at 1:51 AM, Shawn Lin <shawn.lin at rock-chips.com> wrote:
> corecfg_clockmultiplier 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_clockmultiplier, is 0x10. But actually it is a mistake
> 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 make it to be zero on bootloader which seems work fine until
> now. But now we find an issue that when deploying genpd support
> for it, the remove callback will trigger the genpd to poweroff the
> power domain for sdhci-of-arasan which manage the controller, phy
> and corecfg_* stuff.
>
> So when we do bind/unbind the driver, we have already reinit
> 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>
>
> ---
>
> Changes in v2:
> - fix some typos and build
> - move the configuration of corecfg_clockmultiplier after
>   enabling aclk_emmc to avoid the off state of aclk_emmc_grf.
>   I add a new function to make it more common for other coming
>   users who have the same requirment for setting diff clkmul.
>
>  drivers/mmc/host/sdhci-of-arasan.c | 46 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index 0b3a9cf..33601a8 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, .shift = 0},
>         .hiword_update = true,
>  };
>
> @@ -379,6 +382,45 @@ static const struct clk_ops arasan_sdcardclk_ops = {
>  };
>
>  /**
> + * sdhci_arasan_update_clockmultiplier - Set corecfg_clockmultiplier
> + *
> + * The corecfg_clockmultiplier is supposed to contain clock multiplier
> + * value of programmable clock generator.
> + *
> + * NOTES:
> + * - Many existing devices don't seem to do this and work fine.  To keep
> + *   compatibility for old hardware where the device tree doesn't provide a
> + *   register map, this function is a noop if a soc_ctl_map hasn't been provided
> + *   for this platform.
> + * - The value of corecfg_clockmultiplier should sync with that of corresponding
> + *   value reading from sdhci_capability_register. So this function is called
> + *   once at probe time and never called again.
> + *
> + * @host:              The sdhci_host
> + */
> +static void sdhci_arasan_update_clockmultiplier(struct sdhci_host *host,
> +                                               u32 value)
> +{
> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +       struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
> +       const struct sdhci_arasan_soc_ctl_map *soc_ctl_map =
> +               sdhci_arasan->soc_ctl_map;
> +
> +       /* Having a map is optional */
> +       if (!soc_ctl_map)
> +               return;
> +
> +       /* If we have a map, we expect to have a syscon */
> +       if (!sdhci_arasan->soc_ctl_base) {
> +               pr_warn("%s: Have regmap, but no soc-ctl-syscon\n",
> +                       mmc_hostname(host->mmc));
> +               return;
> +       }
> +
> +       sdhci_arasan_syscon_write(host, &soc_ctl_map->clockmultiplier, value);
> +}

It would be nice if all the "Having a map is optional" and "If we have
a map, we expect to have a syscon" were shared between
"sdhci_arasan_update_clockmultiplier" and
"sdhci_arasan_update_baseclkfreq".  You could add a
"sdhci_arasan_syscon_write_opt" helper or something?

...but IMHO that could be done once the third copy is added, so:

Reviewed-by: Douglas Anderson <dianders at chromium.org>