Re: [PATCH 1/2] mmc: renesas_sdhi: handle 4tap hs400 mode quirk based on SoC revision
> I think this design is less messy as the quirks gets added in > renesas_sdhi_core.c instead of needing to keep > renesas_sdhi_internal_dmac.c and renesas_sdhi_sys_dmac.c in sync with > the quirks. > > Wolfram what do you think about this design? I wonder if we can improve it if we base it on top of Yamada-san's latest patch series? For example, I never liked TMIO_MMC_HAVE_4TAP_HS400 which is totally Renesas SDHI specific to be in the TMIO_* namespace. Given Yamada-san's refactoring, that code went from the generic TMIO code to Renesas specific code. I am sure we can remove TMIO_MMC_HAVE_4TAP_HS400 then, and wonder if we can't add a simpler quirk handling on top of it. The latter is a gut feeling, I haven't researched it. Niklas, can you have a look? [1] "[PATCH v3 0/6] mmc: tmio: refactor TMIO core a bit and add UniPhier SD/eMMC controller support" signature.asc Description: PGP signature
Re: [PATCH 1/2] mmc: renesas_sdhi: handle 4tap hs400 mode quirk based on SoC revision
Hi Geert,
Thanks for your feedback.
On 2018-08-06 21:55:17 +0200, Geert Uytterhoeven wrote:
> Hi Niklas,
>
> On Mon, Aug 6, 2018 at 9:43 PM Niklas Söderlund
> wrote:
> > Latest datasheet makes it clear that not all ES revisions of the H3 and
> > M3-W have the 4-tap HS400 mode quirk, currently the quirk is set
> > unconditionally for these two SoCs. Prepare to handle the quirk based on
> > SoC revision instead of compatibility value by using soc_device_match()
> > and set the TMIO_MMC_HAVE_4TAP_HS400 flag explicitly.
>
> Thanks for your patch!
>
> > The reason for adding a new quirks struct instead of just a flag is that
> > looking ahead it seems more quirks needs to be handled in a SoC revision
> > basis.
>
> Do you expect to add a lot? If yes...
Looking at the BSP it looks like there will be just one more quirk
needed.
>
> > --- a/drivers/mmc/host/renesas_sdhi_core.c
> > +++ b/drivers/mmc/host/renesas_sdhi_core.c
>
> > @@ -569,6 +588,10 @@ int renesas_sdhi_probe(struct platform_device *pdev,
> >
> > of_data = of_device_get_match_data(>dev);
> >
> > + attr = soc_device_match(sdhi_quirks_match);
> > + if (attr)
> > + quirks = attr->data;
>
> ... you may consider just storing a plain struct renesas_sdhi_of_data in
> the sdhi_quirks_match[] table.
I think this design is less messy as the quirks gets added in
renesas_sdhi_core.c instead of needing to keep
renesas_sdhi_internal_dmac.c and renesas_sdhi_sys_dmac.c in sync with
the quirks.
Wolfram what do you think about this design?
>
> > +
> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > if (!res)
> > return -EINVAL;
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like
> that.
> -- Linus Torvalds
--
Regards,
Niklas Söderlund
Re: [PATCH 1/2] mmc: renesas_sdhi: handle 4tap hs400 mode quirk based on SoC revision
Hi Niklas,
On Mon, Aug 6, 2018 at 9:43 PM Niklas Söderlund
wrote:
> Latest datasheet makes it clear that not all ES revisions of the H3 and
> M3-W have the 4-tap HS400 mode quirk, currently the quirk is set
> unconditionally for these two SoCs. Prepare to handle the quirk based on
> SoC revision instead of compatibility value by using soc_device_match()
> and set the TMIO_MMC_HAVE_4TAP_HS400 flag explicitly.
Thanks for your patch!
> The reason for adding a new quirks struct instead of just a flag is that
> looking ahead it seems more quirks needs to be handled in a SoC revision
> basis.
Do you expect to add a lot? If yes...
> --- a/drivers/mmc/host/renesas_sdhi_core.c
> +++ b/drivers/mmc/host/renesas_sdhi_core.c
> @@ -569,6 +588,10 @@ int renesas_sdhi_probe(struct platform_device *pdev,
>
> of_data = of_device_get_match_data(>dev);
>
> + attr = soc_device_match(sdhi_quirks_match);
> + if (attr)
> + quirks = attr->data;
... you may consider just storing a plain struct renesas_sdhi_of_data in
the sdhi_quirks_match[] table.
> +
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> if (!res)
> return -EINVAL;
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
[PATCH 1/2] mmc: renesas_sdhi: handle 4tap hs400 mode quirk based on SoC revision
Latest datasheet makes it clear that not all ES revisions of the H3 and
M3-W have the 4-tap HS400 mode quirk, currently the quirk is set
unconditionally for these two SoCs. Prepare to handle the quirk based on
SoC revision instead of compatibility value by using soc_device_match()
and set the TMIO_MMC_HAVE_4TAP_HS400 flag explicitly.
The reason for adding a new quirks struct instead of just a flag is that
looking ahead it seems more quirks needs to be handled in a SoC revision
basis.
Signed-off-by: Niklas Söderlund
---
drivers/mmc/host/renesas_sdhi_core.c | 26 ++
1 file changed, 26 insertions(+)
diff --git a/drivers/mmc/host/renesas_sdhi_core.c
b/drivers/mmc/host/renesas_sdhi_core.c
index 66e90f233cefcba5..781a31592330f872 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -35,6 +35,7 @@
#include
#include
#include
+#include
#include "renesas_sdhi.h"
#include "tmio_mmc.h"
@@ -48,6 +49,10 @@
#define SDHI_VER_GEN3_SD 0xcc10
#define SDHI_VER_GEN3_SDMMC0xcd10
+struct renesas_sdhi_quirks {
+ bool hs400_4taps;
+};
+
static void renesas_sdhi_sdbuf_width(struct tmio_mmc_host *host, int width)
{
u32 val;
@@ -555,11 +560,25 @@ static void renesas_sdhi_enable_dma(struct tmio_mmc_host
*host, bool enable)
renesas_sdhi_sdbuf_width(host, enable ? width : 16);
}
+static const struct renesas_sdhi_quirks sdhi_quirks_h3_m3w = {
+ .hs400_4taps = true,
+};
+
+static const struct soc_device_attribute sdhi_quirks_match[] = {
+ { .soc_id = "r8a7795", .revision = "ES1.*", .data = _quirks_h3_m3w
},
+ { .soc_id = "r8a7795", .revision = "ES2.0", .data = _quirks_h3_m3w
},
+ { .soc_id = "r8a7796", .revision = "ES1.0", .data = _quirks_h3_m3w
},
+ { .soc_id = "r8a7796", .revision = "ES1.1", .data = _quirks_h3_m3w
},
+ { /* Sentinel. */ },
+};
+
int renesas_sdhi_probe(struct platform_device *pdev,
const struct tmio_mmc_dma_ops *dma_ops)
{
struct tmio_mmc_data *mmd = pdev->dev.platform_data;
+ const struct renesas_sdhi_quirks *quirks = NULL;
const struct renesas_sdhi_of_data *of_data;
+ const struct soc_device_attribute *attr;
struct tmio_mmc_data *mmc_data;
struct tmio_mmc_dma *dma_priv;
struct tmio_mmc_host *host;
@@ -569,6 +588,10 @@ int renesas_sdhi_probe(struct platform_device *pdev,
of_data = of_device_get_match_data(>dev);
+ attr = soc_device_match(sdhi_quirks_match);
+ if (attr)
+ quirks = attr->data;
+
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!res)
return -EINVAL;
@@ -634,6 +657,9 @@ int renesas_sdhi_probe(struct platform_device *pdev,
host->multi_io_quirk= renesas_sdhi_multi_io_quirk;
host->dma_ops = dma_ops;
+ if (quirks && quirks->hs400_4taps)
+ mmc_data->flags |= TMIO_MMC_HAVE_4TAP_HS400;
+
/* For some SoC, we disable internal WP. GPIO may override this */
if (mmc_can_gpio_ro(host->mmc))
mmc_data->capabilities2 &= ~MMC_CAP2_NO_WRITE_PROTECT;
--
2.18.0
