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

[Piotr Sroka]

> -----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].
> 
> 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.
I will add this description. 
I think the suffix in this case will not be necessary because the unit is a 2.5ns.
What is your opinion?

> 
> Similar comment applies to all new bindings below.
> 
> > +  Delay configuration stay unchanged if this property is not provided.
> 
> I would remove this information from the DT doc, it's just confusion when
> you refer to something that should remain "unchanged".
> 

Ok I will remove it in next version of patch.

> Similar comment applies to all new bindings below.
> 
> > +- phy-input-delay-sd-default:
> > +  Value of the delay in the input path for Default Speed work mode.
> > +  Valid range = [0:0x1F].
> > +  Delay configuration stay unchanged if this property is not provided.
> > +- phy-input-delay-sd-sdr12:
> > +  Value of the delay in the input path for SDR12 work mode.
> > +  Valid range = [0:0x1F].
> > +  Delay configuration stay unchanged if this property is not provided.
> > +- phy-input-delay-sd-sdr25:
> > +  Value of the delay in the input path for SDR25 work mode.
> > +  Valid range = [0:0x1F].
> > +  Delay configuration stay unchanged if this property is not provided.
> > +- phy-input-delay-sd-sdr50:
> > +  Value of the delay in the input path for SDR50 work mode.
> > +  Valid range = [0:0x1F].
> > +  Delay configuration stay unchanged if this property is not provided.
> > +- phy-input-delay-sd-ddr50:
> > +  Value of the delay in the input path for DDR50 work mode.
> > +  Valid range = [0:0x1F].
> > +  Delay configuration stay unchanged if this property is not provided.
> > +- phy-input-delay-emmc-legacy:
> 
> Legacy? As in legacy speed mode?

Yes as legacy speed mode. But there are two delays each for one specific mode.
One for SD legacy mode and one for MMC legacy mode.

> 
> > +  Value of the delay in the input path for eMMC legacy work mode.
> > +  Valid range = [0:0x1F].
> > +  Delay configuration stay unchanged if this property is not provided.
> > +- phy-input-delay-emmc-sdr:
> > +  Value of the delay in the input path for eMMC SDR work mode.
> > +  Valid range = [0:0x1F].
> > +  Delay configuration stay unchanged if this property is not provided.
> > +- phy-input-delay-emmc-ddr:
> > +  Value of the delay in the input path for eMMC DDR work mode.
> > +  Valid range = [0:0x1F].
> > +  Delay configuration stay unchanged if this property is not provided.
> > +- phy-dll-delay-sdclk:
> > +  Value of the delay introduced on the sdclk output
> > +  for all modes except HS200, HS400 and HS400_ES.
> > +  Valid range = [0:0x7F].
> > +  Delay configuration stay unchanged if this property is not provided.
> > +- phy-dll-delay-sdclk-hsmmc:
> > +  Value of the delay introduced on the sdclk output
> > +  for HS200, HS400 and HS400_ES speed modes.
> > +  Valid range = [0:0x7F].
> > +  Delay configuration stay unchanged if this property is not provided.
> > +- phy-dll-delay-strobe:
> > +  Value of the delay introduced on the dat_strobe input
> > +  used in HS400 / HS400_ES speed modes.
> > +  Valid range = [0:0x7F].
> > +  Delay configuration stay unchanged if this property is not provided.
> 
> A general comment, is that I suggest you look at the generic mmc DT bindings
> for the different speedmodes, and align these new names of DT bindings to
> those.

Ok thanks for suggestion. Does below property names looks more clearly?
phy-input-delay-sd-highspeed, 
phy-input-delay-sd-legacy, 
phy-input-delay-sd-uhs-sdr12
phy-input-delay-sd-uhs-sdr25
phy-input-delay-sd-uhs-sdr50
phy-input-delay-sd-uhs-ddr50
phy-input-delay-mmc-legacy, 
phy-input-delay-mmc-ddr
phy-input-delay-mmc-h200
phy-input-delay-mmc-h400

The last three delays are used for few modes so I will add the names of these modes 
in description of each property. I propose do not change the names. 
phy-dll-delay-sdclk 
phy-dll-delay-sdclk-hsmmc
phy-dll-delay-strobe

> > +               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;
> 
> This all looks very weird. Should you write all this to the phy register, even
> before you know anything about what kind of eMMC/MMC/SD/SDIO card
> that is attached? Does that even work?

Yes it is initial configuration of PHY. Each mode has own input delay. 
Delay for legacy timing mode is not used in high speed mode.
The do not interfere with each other.
It works.

> >  }
> >
> >  static inline void *sdhci_cdns_priv(struct sdhci_host *host) @@
> > -224,10 +288,11 @@ static int sdhci_cdns_probe(struct platform_device
> *pdev)
> >         struct sdhci_host *host;
> >         struct sdhci_pltfm_host *pltfm_host;
> >         struct sdhci_cdns_priv *priv;
> > +       struct device *dev = &pdev->dev;
> >         struct clk *clk;
> >         int ret;
> >
> > -       clk = devm_clk_get(&pdev->dev, NULL);
> > +       clk = devm_clk_get(dev, NULL);
> 
> This seems like and unrelated change. Please remove this change from the
> patch.

Ok it will be removed.

Regards
Piotr