[PATCH 2/7] mmc: zynq_sdhci: Define timing macro's

[Ashok Reddy Soma]

Hi Faiz, 

> -----Original Message-----
> From: Faiz Abbas <faiz_abbas at ti.com>
> Sent: Wednesday, May 27, 2020 12:28 PM
> To: Jaehoon Chung <jh80.chung at samsung.com>; Michal Simek
> <michals at xilinx.com>; u-boot at lists.denx.de; git <git at xilinx.com>
> Cc: Ashok Reddy Soma <ashokred at xilinx.com>; Heinrich Schuchardt
> <xypron.glpk at gmx.de>; Lokesh Vutla <lokeshvutla at ti.com>; Marek Vasut
> <marek.vasut at gmail.com>; Masahiro Yamada <masahiroy at kernel.org>;
> Peng Fan <peng.fan at nxp.com>; Sam Protsenko
> <semen.protsenko at linaro.org>; Simon Glass <sjg at chromium.org>; Yann
> Gautier <yann.gautier at st.com>
> Subject: Re: [PATCH 2/7] mmc: zynq_sdhci: Define timing macro's
> 
> Michal,
> 
> On 27/05/20 12:17 pm, Jaehoon Chung wrote:
> > On 5/22/20 7:44 PM, Michal Simek wrote:
> >> From: Ashok Reddy Soma <ashok.reddy.soma at xilinx.com>
> >>
> >> Define timing macro's for all the available speeds of mmc. This is
> >> done similar to linux. Replace other macro's used in zynq_sdhci.c
> >> with these new macro's.
> >
> > Even though it's similar to linux, does it need to add new macro?
> >
> >>
> >> Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma at xilinx.com>
> >> Signed-off-by: Michal Simek <michal.simek at xilinx.com>
> >> ---
> >>
> >>  drivers/mmc/zynq_sdhci.c | 24 +++++++++++-------------
> >>  include/mmc.h            | 13 +++++++++++++
> >>  2 files changed, 24 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/mmc/zynq_sdhci.c b/drivers/mmc/zynq_sdhci.c
> >> index 94c69cf1c1bd..02583f76f936 100644
> >> --- a/drivers/mmc/zynq_sdhci.c
> >> +++ b/drivers/mmc/zynq_sdhci.c
> >> @@ -32,20 +32,18 @@ struct arasan_sdhci_priv {  };
> >>
> >>  #if defined(CONFIG_ARCH_ZYNQMP)
> >> -#define MMC_HS200_BUS_SPEED	5
> >> -
> >>  static const u8 mode2timing[] = {
> >> -	[MMC_LEGACY] = UHS_SDR12_BUS_SPEED,
> >> -	[MMC_HS] = HIGH_SPEED_BUS_SPEED,
> >> -	[SD_HS] = HIGH_SPEED_BUS_SPEED,
> >> -	[MMC_HS_52] = HIGH_SPEED_BUS_SPEED,
> >> -	[MMC_DDR_52] = HIGH_SPEED_BUS_SPEED,
> >> -	[UHS_SDR12] = UHS_SDR12_BUS_SPEED,
> >> -	[UHS_SDR25] = UHS_SDR25_BUS_SPEED,
> >> -	[UHS_SDR50] = UHS_SDR50_BUS_SPEED,
> >> -	[UHS_DDR50] = UHS_DDR50_BUS_SPEED,
> >> -	[UHS_SDR104] = UHS_SDR104_BUS_SPEED,
> >> -	[MMC_HS_200] = MMC_HS200_BUS_SPEED,
> >> +	[MMC_LEGACY] = MMC_TIMING_LEGACY,
> >> +	[MMC_HS] = MMC_TIMING_MMC_HS,
> >> +	[SD_HS] = MMC_TIMING_SD_HS,
> >> +	[MMC_HS_52] = MMC_TIMING_UHS_SDR50,
> >> +	[MMC_DDR_52] = MMC_TIMING_UHS_DDR50,
> >> +	[UHS_SDR12] = MMC_TIMING_UHS_SDR12,
> >> +	[UHS_SDR25] = MMC_TIMING_UHS_SDR25,
> >> +	[UHS_SDR50] = MMC_TIMING_UHS_SDR50,
> >> +	[UHS_DDR50] = MMC_TIMING_UHS_DDR50,
> >> +	[UHS_SDR104] = MMC_TIMING_UHS_SDR104,
> >> +	[MMC_HS_200] = MMC_TIMING_MMC_HS200,
> >>  };
> >>
> >>  #define SDHCI_TUNING_LOOP_COUNT	40
> >> diff --git a/include/mmc.h b/include/mmc.h index
> >> 82562193cc48..05d8ab8eeac6 100644
> >> --- a/include/mmc.h
> >> +++ b/include/mmc.h
> >> @@ -360,6 +360,19 @@ enum mmc_voltage {
> >>  #define MMC_NUM_BOOT_PARTITION	2
> >>  #define MMC_PART_RPMB           3       /* RPMB partition number */
> >>
> >> +/* timing specification used */
> >> +#define MMC_TIMING_LEGACY	0
> >> +#define MMC_TIMING_MMC_HS	1
> >> +#define MMC_TIMING_SD_HS	2
> >> +#define MMC_TIMING_UHS_SDR12	3
> >> +#define MMC_TIMING_UHS_SDR25	4
> >> +#define MMC_TIMING_UHS_SDR50	5
> >> +#define MMC_TIMING_UHS_SDR104	6
> >> +#define MMC_TIMING_UHS_DDR50	7
> >> +#define MMC_TIMING_MMC_DDR52	8
> >> +#define MMC_TIMING_MMC_HS200	9
> >> +#define MMC_TIMING_MMC_HS400	10
> >> +
> >>  /* Driver model support */
> >>
> 
> There's already an
> 
> enum bus_mode {
>         MMC_LEGACY,
>         MMC_HS,
>         SD_HS,
>         MMC_HS_52,
>         MMC_DDR_52,
>         UHS_SDR12,
>         UHS_SDR25,
>         UHS_SDR50,
>         UHS_DDR50,
>         UHS_SDR104,
>         MMC_HS_200,
>         MMC_HS_400,
>         MMC_HS_400_ES,
>         MMC_MODES_END
> };
> 
> in this file. Thats what the mmc core uses to represent timing. Please use the
> same symbols.

The enum and macro differ in values. For example UHS_SDR12 macro value is 3 whereas enum will be 5. 
This is a problem when accessing below arrays.  I take these reference values from linux driver.
If the values change in future, it will be easy for u-boot driver to just copy and paste from linux driver if we use macro's.

/* Default settings for ZynqMP Clock Phases */
 const u32 zynqmp_iclk_phases[] = {0, 63, 63, 0, 63,  0,   0, 183, 54,  0, 0};
 const u32 zynqmp_oclk_phases[] = {0, 72, 60, 0, 60, 72, 135, 48, 72, 135, 0};

I also see that xenon_sdhci.c has defined these macro's locally. 
https://gitlab.denx.de/u-boot/u-boot/-/blob/master/drivers/mmc/xenon_sdhci.c#L98

So I have added these macro's in include/mmc.h for everyone's use. 

> 
> Thanks,
> Faiz

Thanks,
Ashok