Adding a .platform_init callback to sdhci_arasan_ops
Doug Anderson
dianders at chromium.org
Mon Dec 5 08:13:12 PST 2016
Hi,
On Mon, Dec 5, 2016 at 4:28 AM, Sebastian Frias <sf84 at laposte.net> wrote:
> 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.
I have no objections to chip-specific functions if they are needed.
It's really just a cleaner way to avoid doing "if this chip then, else
if this other chip then, else if this third chip them".
The thing I worry about is that too much stuff will be jammed into
chip-specific functions and that we'll end up solving the same thing
more than one way.
> 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.
I believe the soc_ctl_map will be used by more than one chip, mostly
because I saw these same fields referenced in the generic
(non-Rockchip) Arasan docs. Obviously I have a very small view of the
SDHCI-Arasan world though.
> 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.
I believe the only chip-specific stuff was the part that is currently
guarded by the "if rk3399" check. Everything else seems like it ought
to be applicable to other platforms using Arasan's SDHCI IP.
>> 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.
Yup. I wrote that. See "git blame".
More information about the linux-arm-kernel
mailing list