diff mbox series

[PATCH/RFC,v2] mmc: host: renesas_sdhi: Refactor of_device_id.data

Message ID 20210629102033.847369-1-yoshihiro.shimoda.uh@renesas.com (mailing list archive)
State Under Review
Delegated to: Geert Uytterhoeven
Headers show
Series [PATCH/RFC,v2] mmc: host: renesas_sdhi: Refactor of_device_id.data | expand

Commit Message

Yoshihiro Shimoda June 29, 2021, 10:20 a.m. UTC 
Refactor of_device_id.data to avoid increasing numbers of
sdhi_quirks_match[] entry when we add other stable SoCs like
r8a779m*.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Reported-by: kernel test robot <lkp@intel.com>
---
 - We discussed/reviewed sdhi support for r8a779m* [1] before.
 - I still marked RFC on this patch because:
 -- should I make step-by-step patches to ease review?
 -- should I rename the current renesas_sdhi_of_data (e.g. renesas_sdhi_param)?
    (renesas_sdhi_of_data and renesas_sdhi_of_data_with_quirks seem strange
     a little?)
 - I tested this patch on r8a77951 (ES3.0), r8a77960 (ES1.0) and r8a77965.
 - Also I tested this patch on r8a7791 [2].

 [1]
 https://lore.kernel.org/linux-renesas-soc/TY2PR01MB36927B0CCE7C557A3115E481D8079@TY2PR01MB3692.jpnprd01.prod.outlook.com/

 [2]
 I tested sdhi ch0. But, with and without this patch, sdhi ch2 doesn't work
 correctly...

 Changes from RFC v1:
 - Fix build error in sys_dmac.c, reported by kernel test robot, so that
   add Reported-by tag.
 - Always set quirks, not using else statement.
 - Fix a NULL dereference if of_device_get_match_data() returns NULL.
 https://lore.kernel.org/linux-renesas-soc/20210625075508.664674-1-yoshihiro.shimoda.uh@renesas.com/

 drivers/mmc/host/renesas_sdhi.h               |  5 ++
 drivers/mmc/host/renesas_sdhi_core.c          | 45 ++---------
 drivers/mmc/host/renesas_sdhi_internal_dmac.c | 78 ++++++++++++++++++-
 drivers/mmc/host/renesas_sdhi_sys_dmac.c      | 24 +++++-
 4 files changed, 107 insertions(+), 45 deletions(-)

Comments

Wolfram Sang June 30, 2021, 4:41 a.m. UTC | #1 
Hi Shimoda-san,

thank you for taking care of this!

> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Reported-by: kernel test robot <lkp@intel.com>

I think this Rep-by can go. Test bot mentioned one build error of v1,
but it didn't report that we should refactor this code.

>  -- should I make step-by-step patches to ease review?

I'd think this is good enough.

>  -- should I rename the current renesas_sdhi_of_data (e.g. renesas_sdhi_param)?
>     (renesas_sdhi_of_data and renesas_sdhi_of_data_with_quirks seem strange
>      a little?)

Yes, this may be a little better. I'd think the current naming is good
enough, though.

>  - I tested this patch on r8a77951 (ES3.0), r8a77960 (ES1.0) and r8a77965.

I also tested this on r8a77965.

>  - Also I tested this patch on r8a7791 [2].

I'll check r8a7790 later.

>  static const struct renesas_sdhi_quirks sdhi_quirks_r8a7796_es13 = {
>  	.hs400_4taps = true,
>  	.hs400_bad_taps = BIT(2) | BIT(3) | BIT(6) | BIT(7),
>  	.hs400_calib_table = r8a7796_es13_calib_table,
>  };

You leave the quirk handling of different ES versions still in
renesas_sdhi_core. I'd think this should also be moved to
renesas_sdhi_internal_dmac? Then we have all the handling in one place.

> +static const struct renesas_sdhi_of_data_with_quirks of_r8a7795_compatible = {
> +	.of_data	= &of_data_rcar_gen3,
> +	.quirks		= &sdhi_quirks_bad_taps2367,
> +};

I'd suggest only a single space (and not a tabulator) before the "=".

Rest looks good so far!

All the best,

   Wolfram
Yoshihiro Shimoda June 30, 2021, 7:10 a.m. UTC | #2 
Hi Wolfram-san,

> From: Wolfram Sang, Sent: Wednesday, June 30, 2021 1:42 PM
> 
> Hi Shimoda-san,
> 
> thank you for taking care of this!

Thank you for your review!

> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > Reported-by: kernel test robot <lkp@intel.com>
> 
> I think this Rep-by can go. Test bot mentioned one build error of v1,
> but it didn't report that we should refactor this code.

You're correct. Perhaps, adding "# build fix on RFC" is better?
I checked the commit history, and I found such tags.

> >  -- should I make step-by-step patches to ease review?
> 
> I'd think this is good enough.

I got it.

> >  -- should I rename the current renesas_sdhi_of_data (e.g. renesas_sdhi_param)?
> >     (renesas_sdhi_of_data and renesas_sdhi_of_data_with_quirks seem strange
> >      a little?)
> 
> Yes, this may be a little better. I'd think the current naming is good
> enough, though.

I got it.

> >  - I tested this patch on r8a77951 (ES3.0), r8a77960 (ES1.0) and r8a77965.
> 
> I also tested this on r8a77965.

Thanks!

> >  - Also I tested this patch on r8a7791 [2].
> 
> I'll check r8a7790 later.

Thanks!

> >  static const struct renesas_sdhi_quirks sdhi_quirks_r8a7796_es13 = {
> >  	.hs400_4taps = true,
> >  	.hs400_bad_taps = BIT(2) | BIT(3) | BIT(6) | BIT(7),
> >  	.hs400_calib_table = r8a7796_es13_calib_table,
> >  };
> 
> You leave the quirk handling of different ES versions still in
> renesas_sdhi_core. I'd think this should also be moved to
> renesas_sdhi_internal_dmac? Then we have all the handling in one place.

I think so. So, I'll try this.

> > +static const struct renesas_sdhi_of_data_with_quirks of_r8a7795_compatible = {
> > +	.of_data	= &of_data_rcar_gen3,
> > +	.quirks		= &sdhi_quirks_bad_taps2367,
> > +};
> 
> I'd suggest only a single space (and not a tabulator) before the "=".

I got it.

> Rest looks good so far!

Thank you for your review!

Best regards,
Yoshihiro Shimoda
Wolfram Sang June 30, 2021, 7:34 a.m. UTC | #3 
Hi Shimoda-san,

> > I think this Rep-by can go. Test bot mentioned one build error of v1,
> > but it didn't report that we should refactor this code.
> 
> You're correct. Perhaps, adding "# build fix on RFC" is better?
> I checked the commit history, and I found such tags.

Oh, I didn't know that way. It sounds good!

> > You leave the quirk handling of different ES versions still in
> > renesas_sdhi_core. I'd think this should also be moved to
> > renesas_sdhi_internal_dmac? Then we have all the handling in one place.
> 
> I think so. So, I'll try this.

Glad you like it :)

Happy hacking,

   Wolfram
Wolfram Sang June 30, 2021, 7:43 a.m. UTC | #4 
>  [2]
>  I tested sdhi ch0. But, with and without this patch, sdhi ch2 doesn't work
>  correctly...

Same problem with the Lager board/r8a7790. I suppose this is the same
issue Geert was seeing during his regular tests.

But this has nothing to do with this patch. The probing of the SDHI
instances work fine.
diff mbox series

Patch

diff --git a/drivers/mmc/host/renesas_sdhi.h b/drivers/mmc/host/renesas_sdhi.h
index 53eded81a53e..7ef480d56211 100644
--- a/drivers/mmc/host/renesas_sdhi.h
+++ b/drivers/mmc/host/renesas_sdhi.h
@@ -42,6 +42,11 @@  struct renesas_sdhi_quirks {
 	const u8 (*hs400_calib_table)[SDHI_CALIB_TABLE_MAX];
 };
 
+struct renesas_sdhi_of_data_with_quirks {
+	const struct renesas_sdhi_of_data *of_data;
+	const struct renesas_sdhi_quirks *quirks;
+};
+
 struct tmio_mmc_dma {
 	enum dma_slave_buswidth dma_buswidth;
 	bool (*filter)(struct dma_chan *chan, void *arg);
diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index e49ca0f7fe9a..8c1b29545e22 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -312,20 +312,6 @@  static const u8 r8a7796_es13_calib_table[2][SDHI_CALIB_TABLE_MAX] = {
 	 12, 17, 18, 18, 18, 18, 18, 18, 18, 19, 20, 21, 22, 23, 25, 25 }
 };
 
-static const u8 r8a77965_calib_table[2][SDHI_CALIB_TABLE_MAX] = {
-	{ 1,  2,  6,  6,  7,  8,  9, 10, 11, 12, 13, 14, 15, 15, 15, 16,
-	 17, 18, 19, 20, 21, 22, 23, 24, 25, 25, 26, 27, 28, 29, 30, 31 },
-	{ 2,  3,  4,  4,  5,  6,  7,  9, 10, 11, 12, 13, 14, 15, 16, 17,
-	 17, 17, 20, 21, 22, 23, 24, 25, 27, 28, 29, 30, 31, 31, 31, 31 }
-};
-
-static const u8 r8a77990_calib_table[2][SDHI_CALIB_TABLE_MAX] = {
-	{ 0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,
-	  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0 },
-	{ 0,  0,  0,  1,  2,  3,  3,  4,  4,  4,  5,  5,  6,  8,  9, 10,
-	 11, 12, 13, 15, 16, 17, 17, 18, 18, 19, 20, 22, 24, 25, 26, 26 }
-};
-
 static inline u32 sd_scc_read32(struct tmio_mmc_host *host,
 				struct renesas_sdhi *priv, int addr)
 {
@@ -909,29 +895,12 @@  static const struct renesas_sdhi_quirks sdhi_quirks_nohs400 = {
 	.hs400_disabled = true,
 };
 
-static const struct renesas_sdhi_quirks sdhi_quirks_bad_taps1357 = {
-	.hs400_bad_taps = BIT(1) | BIT(3) | BIT(5) | BIT(7),
-};
-
-static const struct renesas_sdhi_quirks sdhi_quirks_bad_taps2367 = {
-	.hs400_bad_taps = BIT(2) | BIT(3) | BIT(6) | BIT(7),
-};
-
 static const struct renesas_sdhi_quirks sdhi_quirks_r8a7796_es13 = {
 	.hs400_4taps = true,
 	.hs400_bad_taps = BIT(2) | BIT(3) | BIT(6) | BIT(7),
 	.hs400_calib_table = r8a7796_es13_calib_table,
 };
 
-static const struct renesas_sdhi_quirks sdhi_quirks_r8a77965 = {
-	.hs400_bad_taps = BIT(2) | BIT(3) | BIT(6) | BIT(7),
-	.hs400_calib_table = r8a77965_calib_table,
-};
-
-static const struct renesas_sdhi_quirks sdhi_quirks_r8a77990 = {
-	.hs400_calib_table = r8a77990_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
@@ -941,13 +910,8 @@  static const struct soc_device_attribute sdhi_quirks_match[]  = {
 	{ .soc_id = "r8a774a1", .revision = "ES1.[012]", .data = &sdhi_quirks_4tap_nohs400 },
 	{ .soc_id = "r8a7795", .revision = "ES1.*", .data = &sdhi_quirks_4tap_nohs400 },
 	{ .soc_id = "r8a7795", .revision = "ES2.0", .data = &sdhi_quirks_4tap },
-	{ .soc_id = "r8a7795", .revision = "ES3.*", .data = &sdhi_quirks_bad_taps2367 },
 	{ .soc_id = "r8a7796", .revision = "ES1.[012]", .data = &sdhi_quirks_4tap_nohs400 },
 	{ .soc_id = "r8a7796", .revision = "ES1.*", .data = &sdhi_quirks_r8a7796_es13 },
-	{ .soc_id = "r8a77961", .data = &sdhi_quirks_bad_taps1357 },
-	{ .soc_id = "r8a77965", .data = &sdhi_quirks_r8a77965 },
-	{ .soc_id = "r8a77980", .data = &sdhi_quirks_nohs400 },
-	{ .soc_id = "r8a77990", .data = &sdhi_quirks_r8a77990 },
 	{ /* Sentinel. */ },
 };
 
@@ -956,7 +920,8 @@  int renesas_sdhi_probe(struct platform_device *pdev,
 {
 	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 renesas_sdhi_of_data *of_data = NULL;
+	const struct renesas_sdhi_of_data_with_quirks *of_data_quirks;
 	const struct soc_device_attribute *attr;
 	struct tmio_mmc_data *mmc_data;
 	struct tmio_mmc_dma *dma_priv;
@@ -966,7 +931,11 @@  int renesas_sdhi_probe(struct platform_device *pdev,
 	struct resource *res;
 	u16 ver;
 
-	of_data = of_device_get_match_data(&pdev->dev);
+	of_data_quirks = of_device_get_match_data(&pdev->dev);
+	if (of_data_quirks) {
+		of_data = of_data_quirks->of_data;
+		quirks = of_data_quirks->quirks;
+	}
 
 	attr = soc_device_match(sdhi_quirks_match);
 	if (attr)
diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
index e8f4863d8f1a..c4bd602dd8cf 100644
--- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
+++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
@@ -92,7 +92,7 @@  static struct renesas_sdhi_scc rcar_gen3_scc_taps[] = {
 	},
 };
 
-static const struct renesas_sdhi_of_data of_rza2_compatible = {
+static const struct renesas_sdhi_of_data of_data_rza2 = {
 	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_CLK_ACTUAL |
 			  TMIO_MMC_HAVE_CBSY,
 	.tmio_ocr_mask	= MMC_VDD_32_33,
@@ -107,7 +107,11 @@  static const struct renesas_sdhi_of_data of_rza2_compatible = {
 	.max_segs	= 1,
 };
 
-static const struct renesas_sdhi_of_data of_rcar_gen3_compatible = {
+static const struct renesas_sdhi_of_data_with_quirks of_rza2_compatible = {
+	.of_data	= &of_data_rza2,
+};
+
+static const struct renesas_sdhi_of_data of_data_rcar_gen3 = {
 	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_CLK_ACTUAL |
 			  TMIO_MMC_HAVE_CBSY | TMIO_MMC_MIN_RCAR2,
 	.capabilities	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
@@ -122,11 +126,79 @@  static const struct renesas_sdhi_of_data of_rcar_gen3_compatible = {
 	.max_segs	= 1,
 };
 
+static const struct renesas_sdhi_quirks sdhi_quirks_nohs400 = {
+	.hs400_disabled = true,
+};
+
+static const struct renesas_sdhi_quirks sdhi_quirks_bad_taps1357 = {
+	.hs400_bad_taps = BIT(1) | BIT(3) | BIT(5) | BIT(7),
+};
+
+static const struct renesas_sdhi_quirks sdhi_quirks_bad_taps2367 = {
+	.hs400_bad_taps = BIT(2) | BIT(3) | BIT(6) | BIT(7),
+};
+
+static const u8 r8a77965_calib_table[2][SDHI_CALIB_TABLE_MAX] = {
+	{ 1,  2,  6,  6,  7,  8,  9, 10, 11, 12, 13, 14, 15, 15, 15, 16,
+	 17, 18, 19, 20, 21, 22, 23, 24, 25, 25, 26, 27, 28, 29, 30, 31 },
+	{ 2,  3,  4,  4,  5,  6,  7,  9, 10, 11, 12, 13, 14, 15, 16, 17,
+	 17, 17, 20, 21, 22, 23, 24, 25, 27, 28, 29, 30, 31, 31, 31, 31 }
+};
+
+static const u8 r8a77990_calib_table[2][SDHI_CALIB_TABLE_MAX] = {
+	{ 0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,
+	  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0,  0 },
+	{ 0,  0,  0,  1,  2,  3,  3,  4,  4,  4,  5,  5,  6,  8,  9, 10,
+	 11, 12, 13, 15, 16, 17, 17, 18, 18, 19, 20, 22, 24, 25, 26, 26 }
+};
+
+static const struct renesas_sdhi_quirks sdhi_quirks_r8a77965 = {
+	.hs400_bad_taps = BIT(2) | BIT(3) | BIT(6) | BIT(7),
+	.hs400_calib_table = r8a77965_calib_table,
+};
+
+static const struct renesas_sdhi_quirks sdhi_quirks_r8a77990 = {
+	.hs400_calib_table = r8a77990_calib_table,
+};
+
+static const struct renesas_sdhi_of_data_with_quirks of_r8a7795_compatible = {
+	.of_data	= &of_data_rcar_gen3,
+	.quirks		= &sdhi_quirks_bad_taps2367,
+};
+
+static const struct renesas_sdhi_of_data_with_quirks of_r8a77961_compatible = {
+	.of_data	= &of_data_rcar_gen3,
+	.quirks		= &sdhi_quirks_bad_taps1357,
+};
+
+static const struct renesas_sdhi_of_data_with_quirks of_r8a77965_compatible = {
+	.of_data	= &of_data_rcar_gen3,
+	.quirks		= &sdhi_quirks_r8a77965,
+};
+
+static const struct renesas_sdhi_of_data_with_quirks of_r8a77980_compatible = {
+	.of_data	= &of_data_rcar_gen3,
+	.quirks		= &sdhi_quirks_nohs400,
+};
+
+static const struct renesas_sdhi_of_data_with_quirks of_r8a77990_compatible = {
+	.of_data	= &of_data_rcar_gen3,
+	.quirks		= &sdhi_quirks_r8a77990,
+};
+
+static const struct renesas_sdhi_of_data_with_quirks of_rcar_gen3_compatible = {
+	.of_data	= &of_data_rcar_gen3,
+};
+
 static const struct of_device_id renesas_sdhi_internal_dmac_of_match[] = {
 	{ .compatible = "renesas,sdhi-r7s9210", .data = &of_rza2_compatible, },
 	{ .compatible = "renesas,sdhi-mmc-r8a77470", .data = &of_rcar_gen3_compatible, },
-	{ .compatible = "renesas,sdhi-r8a7795", .data = &of_rcar_gen3_compatible, },
+	{ .compatible = "renesas,sdhi-r8a7795", .data = &of_r8a7795_compatible, },
 	{ .compatible = "renesas,sdhi-r8a7796", .data = &of_rcar_gen3_compatible, },
+	{ .compatible = "renesas,sdhi-r8a77961", .data = &of_r8a77961_compatible, },
+	{ .compatible = "renesas,sdhi-r8a77965", .data = &of_r8a77965_compatible, },
+	{ .compatible = "renesas,sdhi-r8a77980", .data = &of_r8a77980_compatible, },
+	{ .compatible = "renesas,sdhi-r8a77990", .data = &of_r8a77990_compatible, },
 	{ .compatible = "renesas,rcar-gen3-sdhi", .data = &of_rcar_gen3_compatible, },
 	{},
 };
diff --git a/drivers/mmc/host/renesas_sdhi_sys_dmac.c b/drivers/mmc/host/renesas_sdhi_sys_dmac.c
index ffa64211f4de..3368723ae015 100644
--- a/drivers/mmc/host/renesas_sdhi_sys_dmac.c
+++ b/drivers/mmc/host/renesas_sdhi_sys_dmac.c
@@ -25,11 +25,15 @@ 
 
 #define TMIO_MMC_MIN_DMA_LEN 8
 
-static const struct renesas_sdhi_of_data of_default_cfg = {
+static const struct renesas_sdhi_of_data of_data_default_cfg = {
 	.tmio_flags = TMIO_MMC_HAS_IDLE_WAIT,
 };
 
-static const struct renesas_sdhi_of_data of_rz_compatible = {
+static const struct renesas_sdhi_of_data_with_quirks of_default_cfg = {
+	.of_data	= &of_data_default_cfg,
+};
+
+static const struct renesas_sdhi_of_data of_data_rz = {
 	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_32BIT_DATA_PORT |
 			  TMIO_MMC_HAVE_CBSY,
 	.tmio_ocr_mask	= MMC_VDD_32_33,
@@ -37,13 +41,21 @@  static const struct renesas_sdhi_of_data of_rz_compatible = {
 			  MMC_CAP_WAIT_WHILE_BUSY,
 };
 
-static const struct renesas_sdhi_of_data of_rcar_gen1_compatible = {
+static const struct renesas_sdhi_of_data_with_quirks of_rz_compatible = {
+	.of_data	= &of_data_rz,
+};
+
+static const struct renesas_sdhi_of_data of_data_rcar_gen1 = {
 	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_CLK_ACTUAL,
 	.capabilities	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
 			  MMC_CAP_WAIT_WHILE_BUSY,
 	.capabilities2	= MMC_CAP2_NO_WRITE_PROTECT,
 };
 
+static const struct renesas_sdhi_of_data_with_quirks of_rcar_gen1_compatible = {
+	.of_data	= &of_data_rcar_gen1,
+};
+
 /* Definitions for sampling clocks */
 static struct renesas_sdhi_scc rcar_gen2_scc_taps[] = {
 	{
@@ -56,7 +68,7 @@  static struct renesas_sdhi_scc rcar_gen2_scc_taps[] = {
 	},
 };
 
-static const struct renesas_sdhi_of_data of_rcar_gen2_compatible = {
+static const struct renesas_sdhi_of_data of_data_rcar_gen2 = {
 	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_CLK_ACTUAL |
 			  TMIO_MMC_HAVE_CBSY | TMIO_MMC_MIN_RCAR2,
 	.capabilities	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
@@ -70,6 +82,10 @@  static const struct renesas_sdhi_of_data of_rcar_gen2_compatible = {
 	.max_blk_count	= UINT_MAX / TMIO_MAX_BLK_SIZE,
 };
 
+static const struct renesas_sdhi_of_data_with_quirks of_rcar_gen2_compatible = {
+	.of_data	= &of_data_rcar_gen2,
+};
+
 static const struct of_device_id renesas_sdhi_sys_dmac_of_match[] = {
 	{ .compatible = "renesas,sdhi-sh73a0", .data = &of_default_cfg, },
 	{ .compatible = "renesas,sdhi-r8a73a4", .data = &of_default_cfg, },