| Message ID | 20180806191227.28401-2-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: renesas_sdhi: extend quirk selection to handle ES revisions | expand |
Hi Niklas, On Mon, Aug 6, 2018 at 9:43 PM Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> 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(&pdev->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
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 > <niklas.soderlund+renesas@ragnatech.se> 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(&pdev->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 -- geert@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
> 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"
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 <linux/pinctrl/consumer.h> #include <linux/pinctrl/pinctrl-state.h> #include <linux/regulator/consumer.h> +#include <linux/sys_soc.h> #include "renesas_sdhi.h" #include "tmio_mmc.h" @@ -48,6 +49,10 @@ #define SDHI_VER_GEN3_SD 0xcc10 #define SDHI_VER_GEN3_SDMMC 0xcd10 +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 = &sdhi_quirks_h3_m3w }, + { .soc_id = "r8a7795", .revision = "ES2.0", .data = &sdhi_quirks_h3_m3w }, + { .soc_id = "r8a7796", .revision = "ES1.0", .data = &sdhi_quirks_h3_m3w }, + { .soc_id = "r8a7796", .revision = "ES1.1", .data = &sdhi_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(&pdev->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;
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 <niklas.soderlund+renesas@ragnatech.se> --- drivers/mmc/host/renesas_sdhi_core.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)