lkml.org 
[lkml]   [2014]   [Jul]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v4 1/8] of: Add NVIDIA Tegra SATA controller binding
    Hi,

    On 07/17/2014 01:42 PM, Hans de Goede wrote:
    > Hi,
    >
    > On 07/17/2014 12:52 PM, Thierry Reding wrote:
    >> On Thu, Jul 17, 2014 at 12:23:47PM +0200, Hans de Goede wrote:
    >
    > <snip>
    >
    >>> The libahci_platform.c code / ahci_platform.c code is also used for
    >>> devices going way back who may not yet be using the new clk framework,
    >>> so where we need to use clk_get(dev, NULL); quoting from libahci_platform.c :
    >>>
    >>> for (i = 0; i < AHCI_MAX_CLKS; i++) {
    >>> /*
    >>> * For now we must use clk_get(dev, NULL) for the first clock,
    >>> * because some platforms (da850, spear13xx) are not yet
    >>> * converted to use devicetree for clocks. For new platforms
    >>> * this is equivalent to of_clk_get(dev->of_node, 0).
    >>> */
    >>> if (i == 0)
    >>> clk = clk_get(dev, NULL);
    >>> else
    >>> clk = of_clk_get(dev->of_node, i);
    >>>
    >>> if (IS_ERR(clk)) {
    >>> rc = PTR_ERR(clk);
    >>> if (rc == -EPROBE_DEFER)
    >>> goto err_out;
    >>> break;
    >>> }
    >>> hpriv->clks[i] = clk;
    >>> }
    >>>
    >>> And there is no devm variant of that, nor is there one to get clocks by index.
    >>> Note that we also need ahci_platform_put_resources for runtime pm support, so
    >>> that one is going to stay around anyways and thus there is not that much value
    >>> in fixing this.
    >>>
    >>> So although I like Thierry's idea, if we go this way (which sounds good), we
    >>> should add support for taking a NULL ahci_platform_resources argument and in
    >>> that case behave as before, esp. because of the platforms needing the old
    >>> style clock handling. An advantage of doing this, is that we can simply patch
    >>> all existing users to pass NULL.
    >>
    >> Isn't the "legacy" case really just this:
    >>
    >> static const char *const legacy_ahci_clocks[] = {
    >> NULL
    >> };
    >>
    >> static const struct ahci_platform_resources legacy_ahci_resources = {
    >> .num_clocks = ARRAY_SIZE(legacy_ahci_clocks),
    >> .clocks = legacy_ahci_clocks,
    >> };
    >>
    >> ?
    >
    > Ah yes that would work for the really legacy ones, as well as less legacy
    > (full dts) ones with only one clk, we need to check if there are current
    > users which use more then one clk (yes there are which is why MAX_CLKS
    > was 3) and fixup those to pass in a correct ahci_platform_resources struct
    > then.
    >
    > The checking + fixing up will be a bit of extra work, but I think the end result
    > will be quite nice. so I'm all in favor of this.

    Correction, this is not going to work I'm afraid, as not all current dts files
    set clock-names. So we need a fallback to get clks by index for compatibility
    with old dts files.

    At least: arch/arm/boot/dts/sun4i-a10.dtsi and arch/arm/boot/dts/sun7i-a20.dtsi
    are affected. Note in this case the dts file is typically not burned into
    a ROM or some such, so IMHO we could get away with requiring a new dts file.

    So it might be worthwhile to still do a full check if all affected SoCs and
    see if we can move over to using clock-names for all platforms with
    more then 1 ahci/sata clk.

    FWIW I've also just checked imx6q.dts which is the one which has 3 clocks, and
    that one does define clk names.

    Regards,

    Hans


    \
     
     \ /
      Last update: 2014-07-17 14:41    [W:3.745 / U:0.780 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site