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

Wed Jun 10 12:16:52 CEST 2020

Hi Ashok,

On 10/06/20 2:48 pm, Ashok Reddy Soma wrote:
> 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. 
> 

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