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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
