diff mbox series

[RFC] clk: renesas: rcar-gen3: remove stp_ck handling for SDHI

Message ID 20200914090426.16022-1-wsa+renesas@sang-engineering.com (mailing list archive)
State Changes Requested, archived
Headers show
Series [RFC] clk: renesas: rcar-gen3: remove stp_ck handling for SDHI | expand

Commit Message

Wolfram Sang Sept. 14, 2020, 9:04 a.m. UTC 
There is no case (and none forseen) where we would need to disable the
SDn clock. So, for simplicity, remove its handling.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

One paradigm is to stay minimal and remove unneeded things. Another one
is to not change working code unnecessarily. I favor the first one a bit
more, but would understand arguing with the second one.

Despite that, tested on a Renesas Salvator-XS (M3-N) with eMMC and two
kinds of SD cards. Checksumming large files works at expected speeds.

 drivers/clk/renesas/rcar-gen3-cpg.c | 51 ++++++++++++++---------------
 1 file changed, 25 insertions(+), 26 deletions(-)

Comments

Geert Uytterhoeven Sept. 14, 2020, 9:35 a.m. UTC | #1 
Hi Wolfram,

On Mon, Sep 14, 2020 at 11:04 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> There is no case (and none forseen) where we would need to disable the

foreseen

> SDn clock. So, for simplicity, remove its handling.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>
> One paradigm is to stay minimal and remove unneeded things. Another one
> is to not change working code unnecessarily. I favor the first one a bit
> more, but would understand arguing with the second one.

Indeed.

Does this make the code rely on bootloader setup or reset state?

Gr{oetje,eeting}s,

                        Geert
Wolfram Sang Sept. 14, 2020, 9:53 a.m. UTC | #2 
On Mon, Sep 14, 2020 at 11:35:07AM +0200, Geert Uytterhoeven wrote:
> Hi Wolfram,
> 
> On Mon, Sep 14, 2020 at 11:04 AM Wolfram Sang
> <wsa+renesas@sang-engineering.com> wrote:
> > There is no case (and none forseen) where we would need to disable the
> 
> foreseen

Oops, yes.

> 
> > SDn clock. So, for simplicity, remove its handling.
> >
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > ---
> >
> > One paradigm is to stay minimal and remove unneeded things. Another one
> > is to not change working code unnecessarily. I favor the first one a bit
> > more, but would understand arguing with the second one.
> 
> Indeed.
> 
> Does this make the code rely on bootloader setup or reset state?

Before, we wrote '0' or '1' to that bit depending on 'stp_ck'. After
removing 'stp_ck', we unconditionally write 0. So, I think we are safe.
diff mbox series

Patch

diff --git a/drivers/clk/renesas/rcar-gen3-cpg.c b/drivers/clk/renesas/rcar-gen3-cpg.c
index 488f8b3980c5..a7debddf7d09 100644
--- a/drivers/clk/renesas/rcar-gen3-cpg.c
+++ b/drivers/clk/renesas/rcar-gen3-cpg.c
@@ -224,10 +224,9 @@  static struct clk * __init cpg_z_clk_register(const char *name,
 #define CPG_SD_STP_MASK		(CPG_SD_STP_HCK | CPG_SD_STP_CK)
 #define CPG_SD_FC_MASK		(0x7 << 2 | 0x3 << 0)
 
-#define CPG_SD_DIV_TABLE_DATA(stp_hck, stp_ck, sd_srcfc, sd_fc, sd_div) \
+#define CPG_SD_DIV_TABLE_DATA(stp_hck, sd_srcfc, sd_fc, sd_div) \
 { \
 	.val = ((stp_hck) ? CPG_SD_STP_HCK : 0) | \
-	       ((stp_ck) ? CPG_SD_STP_CK : 0) | \
 	       ((sd_srcfc) << 2) | \
 	       ((sd_fc) << 0), \
 	.div = (sd_div), \
@@ -247,36 +246,36 @@  struct sd_clock {
 };
 
 /* SDn divider
- *                     sd_srcfc   sd_fc   div
- * stp_hck   stp_ck    (div)      (div)     = sd_srcfc x sd_fc
- *-------------------------------------------------------------------
- *  0         0         0 (1)      1 (4)      4 : SDR104 / HS200 / HS400 (8 TAP)
- *  0         0         1 (2)      1 (4)      8 : SDR50
- *  1         0         2 (4)      1 (4)     16 : HS / SDR25
- *  1         0         3 (8)      1 (4)     32 : NS / SDR12
- *  1         0         4 (16)     1 (4)     64
- *  0         0         0 (1)      0 (2)      2
- *  0         0         1 (2)      0 (2)      4 : SDR104 / HS200 / HS400 (4 TAP)
- *  1         0         2 (4)      0 (2)      8
- *  1         0         3 (8)      0 (2)     16
- *  1         0         4 (16)     0 (2)     32
+ *           sd_srcfc   sd_fc   div
+ * stp_hck   (div)      (div)     = sd_srcfc x sd_fc
+ *---------------------------------------------------------
+ *  0         0 (1)      1 (4)      4 : SDR104 / HS200 / HS400 (8 TAP)
+ *  0         1 (2)      1 (4)      8 : SDR50
+ *  1         2 (4)      1 (4)     16 : HS / SDR25
+ *  1         3 (8)      1 (4)     32 : NS / SDR12
+ *  1         4 (16)     1 (4)     64
+ *  0         0 (1)      0 (2)      2
+ *  0         1 (2)      0 (2)      4 : SDR104 / HS200 / HS400 (4 TAP)
+ *  1         2 (4)      0 (2)      8
+ *  1         3 (8)      0 (2)     16
+ *  1         4 (16)     0 (2)     32
  *
  *  NOTE: There is a quirk option to ignore the first row of the dividers
  *  table when searching for suitable settings. This is because HS400 on
  *  early ES versions of H3 and M3-W requires a specific setting to work.
  */
 static const struct sd_div_table cpg_sd_div_table[] = {
-/*	CPG_SD_DIV_TABLE_DATA(stp_hck,  stp_ck,   sd_srcfc,   sd_fc,  sd_div) */
-	CPG_SD_DIV_TABLE_DATA(0,        0,        0,          1,        4),
-	CPG_SD_DIV_TABLE_DATA(0,        0,        1,          1,        8),
-	CPG_SD_DIV_TABLE_DATA(1,        0,        2,          1,       16),
-	CPG_SD_DIV_TABLE_DATA(1,        0,        3,          1,       32),
-	CPG_SD_DIV_TABLE_DATA(1,        0,        4,          1,       64),
-	CPG_SD_DIV_TABLE_DATA(0,        0,        0,          0,        2),
-	CPG_SD_DIV_TABLE_DATA(0,        0,        1,          0,        4),
-	CPG_SD_DIV_TABLE_DATA(1,        0,        2,          0,        8),
-	CPG_SD_DIV_TABLE_DATA(1,        0,        3,          0,       16),
-	CPG_SD_DIV_TABLE_DATA(1,        0,        4,          0,       32),
+/*	CPG_SD_DIV_TABLE_DATA(stp_hck,  sd_srcfc,   sd_fc,  sd_div) */
+	CPG_SD_DIV_TABLE_DATA(0,        0,          1,        4),
+	CPG_SD_DIV_TABLE_DATA(0,        1,          1,        8),
+	CPG_SD_DIV_TABLE_DATA(1,        2,          1,       16),
+	CPG_SD_DIV_TABLE_DATA(1,        3,          1,       32),
+	CPG_SD_DIV_TABLE_DATA(1,        4,          1,       64),
+	CPG_SD_DIV_TABLE_DATA(0,        0,          0,        2),
+	CPG_SD_DIV_TABLE_DATA(0,        1,          0,        4),
+	CPG_SD_DIV_TABLE_DATA(1,        2,          0,        8),
+	CPG_SD_DIV_TABLE_DATA(1,        3,          0,       16),
+	CPG_SD_DIV_TABLE_DATA(1,        4,          0,       32),
 };
 
 #define to_sd_clock(_hw) container_of(_hw, struct sd_clock, hw)