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