Re: [PATCH 2/2] [2/2] mmc: sdhci-cadence: Update PHY delay configuration

[Masahiro Yamada]

Hi.

2017-02-17 23:12 GMT+09:00 Piotr Sroka <piotrs@xxxxxxxxxxx>:
>> -----Original Message-----
>> From: Ulf Hansson [mailto:ulf.hansson@xxxxxxxxxx]
>> Sent: 16 February, 2017 4:10 PM
>> Subject: Re: [PATCH 2/2] [2/2] mmc: sdhci-cadence: Update PHY delay
>> configuration
>>
>> On 16 February 2017 at 14:06, Piotr Sroka <piotrs@xxxxxxxxxxx> wrote:
>> > DTS properties are used instead of fixed data because PHY settings can
>> > be different for different platforms.
>> > Configuration of new three PHY delays were added
>> >
>> > Signed-off-by: Piotr Sroka <piotrs@xxxxxxxxxxx>
>> > ---
>> >  .../devicetree/bindings/mmc/sdhci-cadence.txt      | 54 ++++++++++++++
>>
>> Please split this patch.
>>
>> DT docs should be a separate patch and make sure it precedes the changes
>> where the new bindings are being parsed in the driver code.
>>
>
> Ok I will do it in next version of patch.
>
>> >  drivers/mmc/host/sdhci-cadence.c                   | 83 +++++++++++++++++++-
>> --
>> >  2 files changed, 129 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
>> > b/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
>> > index c0f37cb..221d3fe 100644
>> > --- a/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
>> > +++ b/Documentation/devicetree/bindings/mmc/sdhci-cadence.txt
>> > @@ -19,6 +19,59 @@ if supported.  See mmc.txt for details.
>> >  - mmc-hs400-1_8v
>> >  - mmc-hs400-1_2v
>> >
>> > +- phy-input-delay-sd-hs:
>> > +  Value of the delay in the input path for High Speed work mode.
>> > +  Valid range = [0:0x1F].

Instead of bunch of new bindings,
a data associated with an SoC specific compatible will do in most cases.


static const struct of_device_id sdhci_cdns_match[] = {
        {
                .compatible = "socionext,uniphier-sd4hc",
                .data = sdhci_cdns_uniphier_phy_data,
        },
        { .compatible = "cdns,sd4hc" },
        { /* sentinel */ }
};


Strictly speaking, the DLL delays will depend on board design as well as SoC.
So, DT bindings would be more flexible, though.





>> Please specify what unit this in. And then also a suffix, like "-ns"
>> to the name of the binding.
>
> The delay starts from 5ns (for delay parameter equal to 0), and it is increased by 2.5ns.
> 0 - means 5ns, 1 means 7.5 ns etc.


In short, all the DT values here are
kind of mysterious register values for the PHY.



>
>> > +               if (ret)
>> > +                       return ret;
>> > +       }
>> > +       if (!of_property_read_u32(np, "phy-dll-delay-sdclk-hsmmc", &tmp)) {
>> > +               ret = sdhci_cdns_write_phy_reg(priv,
>> > +                                              SDHCI_CDNS_PHY_DLY_HSMMC, tmp);
>> > +               if (ret)
>> > +                       return ret;
>> > +       }
>> > +       if (!of_property_read_u32(np, "phy-dll-delay-strobe", &tmp)) {
>> > +               ret = sdhci_cdns_write_phy_reg(priv,
>> > +                                              SDHCI_CDNS_PHY_DLY_STROBE, tmp);
>> > +               if (ret)
>> > +                       return ret;
>> > +       }
>> > +       return 0;


The repeat of the same pattern,
"look up a DT property, then if it exists, set it to a register."


Maybe, is it better to describe it with data array + loop, like this?


struct sdhci_cdns_phy_cfg {
        const char *property;
        u8 addr;
};

static const struct sdhci_cdns_phy_cfg sdhci_cdns_phy_cfgs[] =  {
        { "phy-input-delay-sd-hs", SDHCI_CDNS_PHY_DLY_SD_HS, },
        { "phy-input-delay-sd-default", SDHCI_CDNS_PHY_DLY_SD_DEFAULT, },
        { "phy-input-delay-sd-sdr12", SDHCI_CDNS_PHY_DLY_UHS_SDR12, },
        { "phy-input-delay-sd-sdr25", SDHCI_CDNS_PHY_DLY_UHS_SDR25, },
        { "phy-input-delay-sd-sdr50", SDHCI_CDNS_PHY_DLY_UHS_SDR50, },
        ...
};

static int sdhci_cdns_phy_init(struct device_node *np,
                               struct sdhci_cdns_priv *priv)
{
        u32 val;
        int i;

        for (i = 0; i < ARRAY_SIZE(sdhci_cdns_phy_cfgs), i++) {
                ret = of_property_read_u32(np, sdhci_cdns_phy_cfgs[i].property,
                                           &val);
                if (ret)
                        continue;

                ret = sdhci_cdns_write_phy_reg(priv,
                                               sdhci_cdns_phy_cfgs[i].addr,
                                               val);
                if (ret)
                        return ret;
        }
}





-- 
Best Regards
Masahiro Yamada