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

Jaehoon Chung jh80.chung at samsung.com
Wed Jun 10 12:37:00 CEST 2020


On 6/10/20 6:18 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.

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
> 



More information about the U-Boot mailing list