Hi Masahiro
2017-03-21 08:3 AM Masahiro Yamada <yamada.masah...@socionext.com>:
> Hi Piotr,
>
> 2017-03-21 16:01 GMT+09:00 Piotr Sroka <pio...@cadence.com>:
> >
> > Hi Masahiro
> >
> > 2017-03-21 02:46 AM Masahiro Yamada <yamada.masah...@socionext.com>:
> >> Hi Piotr,
> >>
> >>
> >> 2017-03-20 20:20 GMT+09:00 Piotr Sroka <pio...@cadence.com>:
> >> > DTS properties are used instead of fixed data
> >> > because PHY settings can be different for different chips/boards.
> >> >
> >> > Signed-off-by: Piotr Sroka <pio...@cadence.com>
> >>
> >>
> >> I found this version is a problem for me.
> >>
> >>
> >> > +
> >> > +static const struct sdhci_cdns_phy_cfg sdhci_cdns_phy_cfgs[] = {
> >> > + { "cdns,phy-input-delay-sd-highspeed", SDHCI_CDNS_PHY_DLY_SD_HS,
> >> > },
> >> > + { "cdns,phy-input-delay-sd-legacy",
> >> > SDHCI_CDNS_PHY_DLY_SD_DEFAULT, },
> >> > + { "cdns,phy-input-delay-sd-uhs-sdr12",
> >> > SDHCI_CDNS_PHY_DLY_UHS_SDR12, },
> >> > + { "cdns,phy-input-delay-sd-uhs-sdr25",
> >> > SDHCI_CDNS_PHY_DLY_UHS_SDR25, },
> >> > + { "cdns,phy-input-delay-sd-uhs-sdr50",
> >> > SDHCI_CDNS_PHY_DLY_UHS_SDR50, },
> >> > + { "cdns,phy-input-delay-sd-uhs-ddr50",
> >> > SDHCI_CDNS_PHY_DLY_UHS_DDR50, },
> >> > + { "cdns,phy-input-delay-mmc-highspeed",
> >> > SDHCI_CDNS_PHY_DLY_EMMC_SDR, },
> >> > + { "cdns,phy-input-delay-mmc-ddr", SDHCI_CDNS_PHY_DLY_EMMC_DDR, },
> >> > + { "cdns,phy-dll-delay-sdclk", SDHCI_CDNS_PHY_DLY_SDCLK, },
> >> > + { "cdns,phy-dll-delay-sdclk-hsmmc", SDHCI_CDNS_PHY_DLY_HSMMC, },
> >> > + { "cdns,phy-dll-delay-strobe", SDHCI_CDNS_PHY_DLY_STROBE, },
> >> > +};
> >> > +
> >>
> >>
> >> I see mmc-legacy property in v1,
> >> but it is missing now.
> >>
> >>
> >> > static int sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
> >> > u8 addr, u8 data)
> >> > {
> >> > @@ -90,13 +113,26 @@ static int sdhci_cdns_write_phy_reg(struct
> >> > sdhci_cdns_priv *priv,
> >> > return 0;
> >> > }
> >> >
> >> > -static void sdhci_cdns_phy_init(struct sdhci_cdns_priv *priv)
> >> > +static int sdhci_cdns_phy_init(struct device_node *np,
> >> > + struct sdhci_cdns_priv *priv)
> >> > {
> >> > - sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_SD_HS, 4);
> >> > - sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_SD_DEFAULT, 4);
> >> > - sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_EMMC_LEGACY,
> >> > 9);
> >> > - sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_EMMC_SDR, 2);
> >> > - sdhci_cdns_write_phy_reg(priv, SDHCI_CDNS_PHY_DLY_EMMC_DDR, 3);
> >>
> >>
> >> I need to set SDHCI_CDNS_PHY_DLY_EMMC_LEGACY to 9 for my SoC.
> >>
> >> Maybe, do we need a DT property for this, too?
> >>
> > I can add it but could you check if you realy need it? There is no
> > selection MMC legacy mode
> > in this driver.
> >
>
> Ah, you are right.
>
> For Linux, SD-Legacy and MMC-Legacy share the same timing flag.
> The legacy mode is covered by SDHCI_CDNS_PHY_DLY_SD_DEFAULT,
> so we need not to set SDHCI_CDNS_PHY_DLY_EMMC_LEGACY.
>
> So, I have no problem with patch.
>
> Reviewed-by: Masahiro Yamada <yamada.masah...@socionext.com>
>
>
>
>
> From here, just my minor comments.
> The binding should not be too Linux-oriented.
> Device Tree is hardware description language, and project-neutral from
> its concept.
> I wonder if there is a project that handles SD-Legacy and MMC-Legacy
> separately.
> I am not sure about this, and I do not have a strong opinion, either.
>
> I leave this to you (and other developers).
>
Thanks for your comments.
Because patch adding HS400 enhanced strobe support was applied
for next branch I needed to create v5. BTW I added one extra patch modifing
probe function as you suggested.
I also modify binding to be consistent with linux MMC timing names.
The driver does not use MMC legacy at all so I think there is no need
to distinguish between SD Legacy and MMC Legacy. So there are no
contraindications
for being consistent with Linux.
Best Regards
Piotr Sroka
