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 > > > + .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) && > >