Re: Adding a .platform_init callback to sdhci_arasan_ops

[Sebastian Frias]

Hi Doug,

On 28/11/16 18:46, Doug Anderson wrote:
> Hi,
> 
> On Mon, Nov 28, 2016 at 6:39 AM, Sebastian Frias <sf84@xxxxxxxxxxx> wrote:
>>> I will try to send another patch with what a different approach
>>>
>>
>> Here's a different approach (I just tested that it built, because I don't have the
>> rk3399 platform):
>>
>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
>> index 410a55b..5be6e67 100644
>> --- a/drivers/mmc/host/sdhci-of-arasan.c
>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
>> @@ -382,22 +382,6 @@ static int sdhci_arasan_resume(struct device *dev)
>>  static SIMPLE_DEV_PM_OPS(sdhci_arasan_dev_pm_ops, sdhci_arasan_suspend,
>>                          sdhci_arasan_resume);
>>
>> -static const struct of_device_id sdhci_arasan_of_match[] = {
>> -       /* SoC-specific compatible strings w/ soc_ctl_map */
>> -       {
>> -               .compatible = "rockchip,rk3399-sdhci-5.1",
>> -               .data = &rk3399_soc_ctl_map,
>> -       },
>> -
>> -       /* Generic compatible below here */
>> -       { .compatible = "arasan,sdhci-8.9a" },
>> -       { .compatible = "arasan,sdhci-5.1" },
>> -       { .compatible = "arasan,sdhci-4.9a" },
>> -
>> -       { /* sentinel */ }
>> -};
>> -MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match);
>> -
>>  /**
>>   * sdhci_arasan_sdcardclk_recalc_rate - Return the card clock rate
>>   *
>> @@ -578,28 +562,18 @@ static void sdhci_arasan_unregister_sdclk(struct device *dev)
>>         of_clk_del_provider(dev->of_node);
>>  }
>>
>> -static int sdhci_arasan_probe(struct platform_device *pdev)
>> +static int sdhci_rockchip_platform_init(struct sdhci_host *host,
>> +                                       struct platform_device *pdev)
>>  {
>>         int ret;
>> -       const struct of_device_id *match;
>>         struct device_node *node;
>> -       struct clk *clk_xin;
>> -       struct sdhci_host *host;
>>         struct sdhci_pltfm_host *pltfm_host;
>>         struct sdhci_arasan_data *sdhci_arasan;
>> -       struct device_node *np = pdev->dev.of_node;
>> -
>> -       host = sdhci_pltfm_init(pdev, &sdhci_arasan_pdata,
>> -                               sizeof(*sdhci_arasan));
>> -       if (IS_ERR(host))
>> -               return PTR_ERR(host);
>>
>>         pltfm_host = sdhci_priv(host);
>>         sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
>> -       sdhci_arasan->host = host;
>>
>> -       match = of_match_node(sdhci_arasan_of_match, pdev->dev.of_node);
>> -       sdhci_arasan->soc_ctl_map = match->data;
>> +       sdhci_arasan->soc_ctl_map = &rk3399_soc_ctl_map;
>>
>>         node = of_parse_phandle(pdev->dev.of_node, "arasan,soc-ctl-syscon", 0);
>>         if (node) {
>> @@ -611,10 +585,107 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>>                         if (ret != -EPROBE_DEFER)
>>                                 dev_err(&pdev->dev, "Can't get syscon: %d\n",
>>                                         ret);
>> -                       goto err_pltfm_free;
>> +                       return -1;
>>                 }
>>         }
>>
>> +       if (of_property_read_bool(pdev->dev.of_node, "xlnx,fails-without-test-cd"))
>> +               sdhci_arasan->quirks |= SDHCI_ARASAN_QUIRK_FORCE_CDTEST;
>> +
>> +       return 0;
>> +}
>> +
>> +static int sdhci_rockchip_clock_init(struct sdhci_host *host,
>> +                                       struct platform_device *pdev)
>> +{
>> +       struct sdhci_pltfm_host *pltfm_host;
>> +       struct sdhci_arasan_data *sdhci_arasan;
>> +
>> +       pltfm_host = sdhci_priv(host);
>> +       sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
>> +
>> +       if (of_device_is_compatible(pdev->dev.of_node,
>> +                                   "rockchip,rk3399-sdhci-5.1"))
>> +               sdhci_arasan_update_clockmultiplier(host, 0x0);
> 
> This _does_ belong in a Rockchip-specific init function, for now.

I'm not sure I understood. Are you saying that you agree to put this
into a specific function? Essentially agreeing with the concept the
patch is putting forward?

Note, I'm more interested in the concept (i.e.: init functions) and less
in knowing if my patch (which was a quick and dirty thing) properly moved
the functions into the said init functions. For example, I did not move
the code dealing with "arasan,sdhci-5.1", but it could go into another
callback.

Right now there are no "chip-specific" functions.
Just a code in sdhci_arasan_probe() that by checking various compatible
strings and the presence of other specific properties, acts as a way of
"chip-specific" initialisation, because it calls or not some functions.
(or the functions do nothing if some DT properties are not there).

The proposed patch is an attempt to clean up sdhci_arasan_probe() from
all those checks and move them into separate functions, for clarity and
maintainability reasons.

What are your thoughts in that regard?

>From what I've seen in other drivers, for example drivers/net/ethernet/aurora/nb8800.c
One matches the compatible string to get a (likely) chip-specific set of
callbacks to use in the 'probe' function.

Then, adding support for other chips is just a matter of replacing some
of those callbacks with others adapted to a given platform.

> Other platforms might want different values for
> corecfg_clockmultiplier, I think.
> 
> If it becomes common to need to set this, it wouldn't be hard to make
> it generic by putting it in the data matched by the device tree, then
> generically call sdhci_arasan_update_clockmultiplier() in cases where
> it is needed.  sdhci_arasan_update_clockmultiplier() itself should be
> generic enough to handle it.
> 
> 
>> +
>> +       sdhci_arasan_update_baseclkfreq(host);
> 
> If you make soc_ctl_map always part of "struct sdhci_arasan_cs_ops"
> then other platforms will be able to use it.

I thought that soc_ctl_map was specific and for a given platform.
For what is worth, I don't know which of these calls are or can be made
generic or not.

Indeed, I'm not an expert in this code; However, I think that given the
amount of checks for specific properties, probably part of this is chip-
specific, and as such, it would benefit from some re-factoring so that
the chip-specific parts are clearly separated from the rest, instead of
being mixed together inside the probe function.

> 
> As argued in my original patch the field "corecfg_baseclkfreq" is
> documented in the generic Arasan document
> <https://arasan.com/wp-content/media/eMMC-5-1-Total-Solution_Rev-1-3.pdf>
> and thus is unlikely to be Rockchip specific.  It is entirely possible
> that not everyone who integrates this IP block will need this register
> set, but in that case they can set an offset as "-1" and they'll be
> fine.
> 
> Said another way: the concept of whether or not to set "baseclkfreq"
> doesn't need to be tired to whether or not we're on Rockchip.
> 

I see.
For what is worth, the documentation for 'sdhci_arasan_update_baseclkfreq()'
says something like:

 *   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.

> 
>> +
>> +       return sdhci_arasan_register_sdclk(sdhci_arasan, pltfm_host->clk, &pdev->dev);
> 
> This is documented in "bindings/mmc/arasan,sdhci.txt" to be available
> to all people using this driver, not just Rockchip.  You should do it
> always.  If you don't specify "#clock-cells" in the device tree it
> will be a no-op anyway.

I see, thanks for the explanation.

> 
> 
>> +}
>> +
>> +static int sdhci_tango_platform_init(struct sdhci_host *host,
>> +                                    struct platform_device *pdev)
>> +{
>> +       return 0;
> 
> I'll comment in your other patch where you actually had an
> implementation for this.
> 

Sounds good. I will reply to you there because now I have a more
complete set of register writes.

Best regards,

Sebastian

> 
>> +}
>> +
>> +/* Chip-specific ops */
>> +struct sdhci_arasan_cs_ops {
>> +       int (*platform_init)(struct sdhci_host *host,
>> +                            struct platform_device *pdev);
>> +       int (*clock_init)(struct sdhci_host *host,
>> +                         struct platform_device *pdev);
>> +};
> 
> I really think it's a good idea to add the "soc_ctl_map" into this structure.
> 
> If nothing else when the next Rockchip SoC comes out that uses this
> then we won't have to do a second level of matching for Rockchip SoCs.
> I'm also a firm believer that others will need "soc_ctl_map" at some
> point in time, but obviously I can't predict the future.
> 
> 
>> +
>> +static const struct sdhci_arasan_cs_ops sdhci_rockchip_cs_ops = {
>> +       .platform_init = sdhci_rockchip_platform_init,
>> +       .clock_init = sdhci_rockchip_clock_init,
>> +};
>> +
>> +static const struct sdhci_arasan_cs_ops sdhci_tango_cs_ops = {
>> +       .platform_init = sdhci_tango_platform_init,
>> +};
>> +
>> +static const struct of_device_id sdhci_arasan_of_match[] = {
>> +       /* SoC-specific compatible strings */
>> +       {
>> +               .compatible = "rockchip,rk3399-sdhci-5.1",
>> +               .data = &sdhci_rockchip_cs_ops,
>> +       },
>> +       {
>> +               .compatible = "sigma,sdio-v1",
>> +               .data = &sdhci_tango_cs_ops,
>> +       },
>> +
>> +       /* Generic compatible below here */
>> +       { .compatible = "arasan,sdhci-8.9a" },
>> +       { .compatible = "arasan,sdhci-5.1" },
>> +       { .compatible = "arasan,sdhci-4.9a" },
>> +
>> +       { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, sdhci_arasan_of_match);
>> +
>> +static int sdhci_arasan_probe(struct platform_device *pdev)
>> +{
>> +       int ret;
>> +       const struct of_device_id *match;
>> +       struct clk *clk_xin;
>> +       struct sdhci_host *host;
>> +       struct sdhci_pltfm_host *pltfm_host;
>> +       struct sdhci_arasan_data *sdhci_arasan;
>> +       const struct sdhci_arasan_cs_ops *cs_ops;
>> +
>> +       host = sdhci_pltfm_init(pdev, &sdhci_arasan_pdata,
>> +                               sizeof(*sdhci_arasan));
>> +       if (IS_ERR(host))
>> +               return PTR_ERR(host);
>> +
>> +       pltfm_host = sdhci_priv(host);
>> +       sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
>> +       sdhci_arasan->host = host;
>> +
>> +       match = of_match_device(sdhci_arasan_of_match, &pdev->dev);
>> +       if (match)
>> +               cs_ops = match->data;
>> +
>> +       /* SoC-specific platform init */
>> +       if (cs_ops && cs_ops->platform_init) {
>> +               ret = cs_ops->platform_init(host, pdev);
>> +               if (ret)
>> +                       goto err_pltfm_free;
>> +       }
>> +
>>         sdhci_arasan->clk_ahb = devm_clk_get(&pdev->dev, "clk_ahb");
>>         if (IS_ERR(sdhci_arasan->clk_ahb)) {
>>                 dev_err(&pdev->dev, "clk_ahb clock not found.\n");
>> @@ -642,21 +713,14 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>>         }
>>
>>         sdhci_get_of_property(pdev);
>> -
>> -       if (of_property_read_bool(np, "xlnx,fails-without-test-cd"))
>> -               sdhci_arasan->quirks |= SDHCI_ARASAN_QUIRK_FORCE_CDTEST;
>> -
>>         pltfm_host->clk = clk_xin;
>>
>> -       if (of_device_is_compatible(pdev->dev.of_node,
>> -                                   "rockchip,rk3399-sdhci-5.1"))
>> -               sdhci_arasan_update_clockmultiplier(host, 0x0);
>> -
>> -       sdhci_arasan_update_baseclkfreq(host);
>> -
>> -       ret = sdhci_arasan_register_sdclk(sdhci_arasan, clk_xin, &pdev->dev);
>> -       if (ret)
>> -               goto clk_disable_all;
>> +       /* SoC-specific clock init */
>> +       if (cs_ops && cs_ops->clock_init) {
>> +               ret = cs_ops->clock_init(host, pdev);
>> +               if (ret)
>> +                       goto clk_disable_all;
>> +       }
>>
>>         ret = mmc_of_parse(host->mmc);
>>         if (ret) {
>>
>>
>> If you are ok with it I can clean it up to submit it properly.