Adding a .platform_init callback to sdhci_arasan_ops
Sebastian Frias
sf84 at laposte.net
Mon Dec 5 04:28:04 PST 2016
Hi Doug,
On 28/11/16 18:46, Doug Anderson wrote:
> Hi,
>
> On Mon, Nov 28, 2016 at 6:39 AM, Sebastian Frias <sf84 at 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.
More information about the linux-arm-kernel
mailing list