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