Hi Jaehoon,
> -----Original Message-----
> From: Jaehoon Chung <jh80.ch...@samsung.com>
> Sent: Wednesday, June 10, 2020 4:07 PM
> To: Ashok Reddy Soma <ashok...@xilinx.com>; Faiz Abbas
> <faiz_ab...@ti.com>; Michal Simek <mich...@xilinx.com>; u-
> b...@lists.denx.de; git <g...@xilinx.com>
> Cc: 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
>
> On 6/10/20 6:18 PM, Ashok Reddy Soma wrote:
> > 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.
>
> 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
> >
