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

[Faiz Abbas <faiz_abbas@ti.com>]

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 at 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