lkml.org 
[lkml]   [2016]   [Dec]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: Adding a .platform_init callback to sdhci_arasan_ops
From
Date
Hi Doug,

On 28/11/16 18:46, Doug Anderson wrote:
> Hi,
>
> On Mon, Nov 28, 2016 at 6:39 AM, Sebastian Frias <sf84@laposte.net> 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.

\
 
 \ /
  Last update: 2016-12-05 13:29    [W:0.113 / U:0.364 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site