| Message ID | 22b8d8014e6539a876c9761f70d9cd80fb8c1964.1590144247.git.michal.simek@xilinx.com |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | mmc: zynqmp_sdhci: Add support for Tap delay | expand |
On 5/22/20 7:44 PM, Michal Simek wrote: > From: Ashok Reddy Soma <ashok.reddy.soma@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@xilinx.com> > Signed-off-by: Michal Simek <michal.simek@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 */ > > /** >
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@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@xilinx.com> >> Signed-off-by: Michal Simek <michal.simek@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. Thanks, Faiz
Hi Faiz, > -----Original Message----- > From: Faiz Abbas <faiz_abbas@ti.com> > Sent: Wednesday, May 27, 2020 12:28 PM > To: Jaehoon Chung <jh80.chung@samsung.com>; Michal Simek > <michals@xilinx.com>; u-boot@lists.denx.de; git <git@xilinx.com> > Cc: Ashok Reddy Soma <ashokred@xilinx.com>; Heinrich Schuchardt > <xypron.glpk@gmx.de>; Lokesh Vutla <lokeshvutla@ti.com>; Marek Vasut > <marek.vasut@gmail.com>; Masahiro Yamada <masahiroy@kernel.org>; > Peng Fan <peng.fan@nxp.com>; Sam Protsenko > <semen.protsenko@linaro.org>; Simon Glass <sjg@chromium.org>; Yann > Gautier <yann.gautier@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@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@xilinx.com> > >> Signed-off-by: Michal Simek <michal.simek@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
Hi Ashok, On 10/06/20 2:48 pm, Ashok Reddy Soma wrote: > Hi Faiz, > >> -----Original Message----- >> From: Faiz Abbas <faiz_abbas@ti.com> >> Sent: Wednesday, May 27, 2020 12:28 PM >> To: Jaehoon Chung <jh80.chung@samsung.com>; Michal Simek >> <michals@xilinx.com>; u-boot@lists.denx.de; git <git@xilinx.com> >> Cc: Ashok Reddy Soma <ashokred@xilinx.com>; Heinrich Schuchardt >> <xypron.glpk@gmx.de>; Lokesh Vutla <lokeshvutla@ti.com>; Marek Vasut >> <marek.vasut@gmail.com>; Masahiro Yamada <masahiroy@kernel.org>; >> Peng Fan <peng.fan@nxp.com>; Sam Protsenko >> <semen.protsenko@linaro.org>; Simon Glass <sjg@chromium.org>; Yann >> Gautier <yann.gautier@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@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@xilinx.com> >>>> Signed-off-by: Michal Simek <michal.simek@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. > A better approach would be to try to bring the U-boot macro in line with kernel. That way more drivers can take advantage of the similarities. Adding extra symbols just confuses people about which ones are being used in core and which ones in your driver. Thanks, Faiz
On 6/10/20 6:18 PM, Ashok Reddy Soma wrote: > Hi Faiz, > >> -----Original Message----- >> From: Faiz Abbas <faiz_abbas@ti.com> >> Sent: Wednesday, May 27, 2020 12:28 PM >> To: Jaehoon Chung <jh80.chung@samsung.com>; Michal Simek >> <michals@xilinx.com>; u-boot@lists.denx.de; git <git@xilinx.com> >> Cc: Ashok Reddy Soma <ashokred@xilinx.com>; Heinrich Schuchardt >> <xypron.glpk@gmx.de>; Lokesh Vutla <lokeshvutla@ti.com>; Marek Vasut >> <marek.vasut@gmail.com>; Masahiro Yamada <masahiroy@kernel.org>; >> Peng Fan <peng.fan@nxp.com>; Sam Protsenko >> <semen.protsenko@linaro.org>; Simon Glass <sjg@chromium.org>; Yann >> Gautier <yann.gautier@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@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@xilinx.com> >>>> Signed-off-by: Michal Simek <michal.simek@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. enum bus_mode is a specification which mode is. MMc_TIMING_xxx is a specification which timing is. As i know, its meaning is a little different. But it's not reason that can't use it. *specification* is using to make more readable. I don't have any objection to add or not. But i don't agree about copy and paste everything from linux driver. Anyway, I don't know why you have to fix array value. const u32 zynqmap_iclk_phases[] = { [UHS_SDR12] = 0, ... } Is it impossible to assign to proper value into array? > > /* 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://protect2.fireeye.com/url?k=34df76b5-69117766-34defdfa-000babff317b-0937b081dc4534f6&q=1&u=https%3A%2F%2Fgitlab.denx.de%2Fu-boot%2Fu-boot%2F-%2Fblob%2Fmaster%2Fdrivers%2Fmmc%2Fxenon_sdhci.c%23L98 > > So I have added these macro's in include/mmc.h for everyone's use. If you want to use this for everyone, i think that it needs to change subject. Your subject looks like adding it only for zynq_sdhci. I think Peng also has his opinion. let's wait for his opinion. Best Regards, Jaehoon Chung > >> >> Thanks, >> Faiz > > Thanks, > Ashok >
Hi Jaehoon, > -----Original Message----- > From: Jaehoon Chung <jh80.chung@samsung.com> > Sent: Wednesday, June 10, 2020 4:07 PM > To: Ashok Reddy Soma <ashokred@xilinx.com>; Faiz Abbas > <faiz_abbas@ti.com>; Michal Simek <michals@xilinx.com>; u- > boot@lists.denx.de; git <git@xilinx.com> > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>; Lokesh Vutla > <lokeshvutla@ti.com>; Marek Vasut <marek.vasut@gmail.com>; Masahiro > Yamada <masahiroy@kernel.org>; Peng Fan <peng.fan@nxp.com>; Sam > Protsenko <semen.protsenko@linaro.org>; Simon Glass > <sjg@chromium.org>; Yann Gautier <yann.gautier@st.com> > Subject: Re: [PATCH 2/7] mmc: zynq_sdhci: Define timing macro's > > On 6/10/20 6:18 PM, Ashok Reddy Soma wrote: > > Hi Faiz, > > > >> -----Original Message----- > >> From: Faiz Abbas <faiz_abbas@ti.com> > >> Sent: Wednesday, May 27, 2020 12:28 PM > >> To: Jaehoon Chung <jh80.chung@samsung.com>; Michal Simek > >> <michals@xilinx.com>; u-boot@lists.denx.de; git <git@xilinx.com> > >> Cc: Ashok Reddy Soma <ashokred@xilinx.com>; Heinrich Schuchardt > >> <xypron.glpk@gmx.de>; Lokesh Vutla <lokeshvutla@ti.com>; Marek Vasut > >> <marek.vasut@gmail.com>; Masahiro Yamada <masahiroy@kernel.org>; > Peng > >> Fan <peng.fan@nxp.com>; Sam Protsenko > <semen.protsenko@linaro.org>; > >> Simon Glass <sjg@chromium.org>; Yann Gautier <yann.gautier@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@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@xilinx.com> > >>>> Signed-off-by: Michal Simek <michal.simek@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. > > enum bus_mode is a specification which mode is. MMc_TIMING_xxx is a > specification which timing is. Correct, these timing macro's are being used for setting phase delays. That was the background. > As i know, its meaning is a little different. But it's not reason that can't use it. > *specification* is using to make more readable. I don't have any objection to > add or not. > But i don't agree about copy and paste everything from linux driver. > > Anyway, I don't know why you have to fix array value. > > const u32 zynqmap_iclk_phases[] = { > > [UHS_SDR12] = 0, > ... > } > > Is it impossible to assign to proper value into array? It can be done, I just quoted a example. > > > > > > /* 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://protect2.fireeye.com/url?k=34df76b5-69117766-34defdfa-000babff > > 317b-0937b081dc4534f6&q=1&u=https%3A%2F%2Fgitlab.denx.de%2Fu- > boot%2Fu- > > boot%2F-%2Fblob%2Fmaster%2Fdrivers%2Fmmc%2Fxenon_sdhci.c%23L98 > > > > So I have added these macro's in include/mmc.h for everyone's use. > > If you want to use this for everyone, i think that it needs to change subject. > Your subject looks like adding it only for zynq_sdhci. Sure, I will change the subject and send v2. > > I think Peng also has his opinion. let's wait for his opinion. Sure. > > Best Regards, > Jaehoon Chung > > > > >> > >> Thanks, > >> Faiz > > > > Thanks, > > Ashok > >
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 */ /**