On 11/3/20 7:15 PM, Biju Das wrote: > Hi Jaehoon Chung, > > Thanks for the feedback. > >> Subject: Re: [PATCH v6 2/7] mmc: renesas-sdhi: Add SDHI quirks for R-Car >> M3-W and RZ/G2M >> >> On 11/3/20 1:16 AM, Biju Das wrote: >>> Add SDHI quirks for R-Car M3-W and RZ/G2M SoC. >>> >>> Signed-off-by: Biju Das <biju.das...@bp.renesas.com> >>> Reviewed-by: Lad Prabhakar <prabhakar.mahadev- >> lad...@bp.renesas.com> >>> --- >>> drivers/mmc/renesas-sdhi.c | 110 >>> +++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 110 insertions(+) >>> >>> diff --git a/drivers/mmc/renesas-sdhi.c b/drivers/mmc/renesas-sdhi.c >>> index d80b3fc28f..39deeb94d8 100644 >>> --- a/drivers/mmc/renesas-sdhi.c >>> +++ b/drivers/mmc/renesas-sdhi.c >>> @@ -19,6 +19,7 @@ >>> #include <linux/io.h> >>> #include <linux/sizes.h> >>> #include <power/regulator.h> >>> +#include <soc.h> >>> #include <asm/unaligned.h> >>> #include "tmio-common.h" >>> >>> @@ -105,6 +106,15 @@ static const u8 >> r8a77990_calib_table[2][CALIB_TABLE_MAX] = { >>> 12, 13, 14, 16, 17, 18, 18, 18, 19, 19, 20, 24, 26, 26, 26, 26 } >>> }; >>> >>> +#define SDHI_CALIB_TABLE_MAX 32 >>> + >>> +struct renesas_sdhi_quirks { >>> + bool hs400_disabled; >>> + bool hs400_4taps; >>> + u32 hs400_bad_taps; >>> + const u8 (*hs400_calib_table)[SDHI_CALIB_TABLE_MAX]; >>> +}; >>> + >>> static int rmobile_is_gen3_mmc0(struct tmio_sd_priv *priv) { >>> /* On R-Car Gen3, MMC0 is at 0xee140000 */ @@ -855,6 +865,99 @@ >>> static ulong renesas_sdhi_clk_get_rate(struct tmio_sd_priv *priv) >>> return clk_get_rate(&priv->clk); >>> } >>> >>> +static const struct renesas_sdhi_quirks >> sdhi_quirks_4tap_nohs400_b17_dtrend = { >>> + .hs400_disabled = true, >>> + .hs400_4taps = true, >>> +}; >>> + >>> +static const struct renesas_sdhi_quirks sdhi_quirks_4tap_nohs400 = { >>> + .hs400_disabled = true, >>> + .hs400_4taps = true, >>> +}; >>> + >>> +static const struct renesas_sdhi_quirks sdhi_quirks_r8a7796_es12 = { >>> + .hs400_4taps = true, >>> + .hs400_bad_taps = BIT(2) | BIT(3) | BIT(6) | BIT(7), >> >> Use Macro, not magic code. We don't know what mean BIT(2), BIT(3), BIT(6).. > > This work is based on linux[1]. For maintainability we want to make u-boot > code similar to linux, so that in future if there > is any improvement in linux we can port here. > > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mmc/host/renesas_sdhi_core.c?h=v5.10-rc2#n886
I think that it needs to change to meaningful value in linux. :) But I agreed that you want to maintain the similar code with linux. Thanks for kindly explanation. Best Regards, Jaehoon Chung > >> >>> + .hs400_calib_table = r8a7796_rev1_calib_table, }; >>> + >>> +static const struct renesas_sdhi_quirks sdhi_quirks_r8a7796_es13 = { >>> + .hs400_bad_taps = BIT(1) | BIT(3) | BIT(5) | BIT(7), >> >> Ditto. >> >>> + .hs400_calib_table = r8a7796_rev3_calib_table, }; >>> + >>> +/* >>> + * Note for r8a7796 / r8a774a1: we can't distinguish ES1.1 and 1.2 as of >> now. >>> + * So, we want to treat them equally and only have a match for ES1.2 >>> +to enforce >>> + * this if there ever will be a way to distinguish ES1.2. >>> + */ >>> +static const struct soc_attr sdhi_quirks_match[] = { >>> + { .soc_id = "r8a774a1", >>> + .revision = "ES1.0", >>> + .data = &sdhi_quirks_4tap_nohs400_b17_dtrend >>> + }, >>> + { .soc_id = "r8a774a1", >>> + .revision = "ES1.1", >>> + .data = &sdhi_quirks_4tap_nohs400 >>> + }, >>> + { .soc_id = "r8a774a1", >>> + .revision = "ES1.2", >>> + .data = &sdhi_quirks_r8a7796_es12 >>> + }, >>> + { .soc_id = "r8a774a1", >>> + .revision = "ES1.3", >>> + .data = &sdhi_quirks_r8a7796_es13 >>> + }, >>> + { .soc_id = "r8a7796", >>> + .revision = "ES1.0", >>> + .data = &sdhi_quirks_4tap_nohs400_b17_dtrend >>> + }, >>> + { .soc_id = "r8a7796", >>> + .revision = "ES1.1", >>> + .data = &sdhi_quirks_4tap_nohs400 >>> + }, >>> + { .soc_id = "r8a7796", >>> + .revision = "ES1.2", >>> + .data = &sdhi_quirks_r8a7796_es12 >>> + }, >>> + { .soc_id = "r8a7796", >>> + .revision = "ES1.3", >>> + .data = &sdhi_quirks_r8a7796_es13 >>> + }, >>> + { /* Sentinel. */ }, >>> +}; >>> + >>> +static void renesas_sdhi_add_quirks(struct tmio_sd_plat *plat, >>> + struct tmio_sd_priv *priv, >>> + const struct renesas_sdhi_quirks *quirks) { >>> + priv->read_poll_flag = TMIO_SD_DMA_INFO1_END_RD2; >>> + >>> + if (quirks && quirks->hs400_disabled) { >>> + plat->cfg.host_caps &= ~MMC_MODE_HS400; >>> + if (quirks == &sdhi_quirks_4tap_nohs400_b17_dtrend) >>> + priv->read_poll_flag = >> TMIO_SD_DMA_INFO1_END_RD; >>> + } >>> + >>> + if (quirks && quirks->hs400_4taps) >>> + priv->nrtaps = 4; >>> + else >>> + priv->nrtaps = 8; >> >> priv->nrtraps = 8 should be default value. >> And it needs to check one time about quirks's present at first time. >> Then it can be changed to below.. > > Agreed. Will do this changes in next version. > > Regards, > Biju > >> priv->read_poll_flag = TMIO...; >> priv->nrtaps = 8; >> >> if (!quriks) >> return; >> if (quirks-.hs400_disabld) { >> ... >> } >> >> if (quirks->hs400_4taps) >> priv->nrtaps = 4; >> >> ... >> >> Then it's more readable.. >> >> Best Regards, >> Jaehoon Chung >> >>> + >>> + if (quirks && quirks->hs400_bad_taps) >>> + priv->hs400_bad_tap = quirks->hs400_bad_taps;> + >>> + if (quirks && quirks->hs400_calib_table) { >>> + priv->adjust_hs400_enable = true; >>> + priv->adjust_hs400_calib_table = >>> + quirks- >>> hs400_calib_table[!rmobile_is_gen3_mmc0(priv)]; >>> + if (quirks == &sdhi_quirks_r8a7796_es12) >>> + priv->adjust_hs400_offset = 3; >>> + else if (quirks == &sdhi_quirks_r8a7796_es13) >>> + priv->adjust_hs400_offset = 0; >>> + } >>> +} >>> + >>> static void renesas_sdhi_filter_caps(struct udevice *dev) { >>> struct tmio_sd_priv *priv = dev_get_priv(dev); @@ -866,6 +969,13 >> @@ >>> static void renesas_sdhi_filter_caps(struct udevice *dev) >>> CONFIG_IS_ENABLED(MMC_HS200_SUPPORT) || \ >>> CONFIG_IS_ENABLED(MMC_HS400_SUPPORT) >>> struct tmio_sd_plat *plat = dev_get_platdata(dev); >>> + const struct soc_attr *attr; >>> + >>> + attr = soc_device_match(sdhi_quirks_match); >>> + if (attr) { >>> + renesas_sdhi_add_quirks(plat, priv, attr->data); >>> + return; >>> + } >>> >>> /* HS400 is not supported on H3 ES1.x and M3W ES1.0, ES1.1 */ >>> if (((rmobile_get_cpu_type() == RMOBILE_CPU_TYPE_R8A7795) && >>> >