Hi Faiz, > -----Original Message----- > From: Faiz Abbas <faiz_ab...@ti.com> > Sent: Wednesday, May 27, 2020 12:28 PM > To: Jaehoon Chung <jh80.ch...@samsung.com>; Michal Simek > <mich...@xilinx.com>; u-boot@lists.denx.de; git <g...@xilinx.com> > Cc: Ashok Reddy Soma <ashok...@xilinx.com>; Heinrich Schuchardt > <xypron.g...@gmx.de>; Lokesh Vutla <lokeshvu...@ti.com>; Marek Vasut > <marek.va...@gmail.com>; Masahiro Yamada <masahi...@kernel.org>; > Peng Fan <peng....@nxp.com>; Sam Protsenko > <semen.protse...@linaro.org>; Simon Glass <s...@chromium.org>; Yann > Gautier <yann.gaut...@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.s...@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.s...@xilinx.com> > >> Signed-off-by: Michal Simek <michal.si...@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