[PATCH v4 0/5] Add definitions for AST2600 i3c clocks and resets

[PATCH v4 0/5] Add definitions for AST2600 i3c clocks and resets

[Jeremy Kerr]

This series adds some base configuration for the i3c controllers on
ast2600 hardware. We'll use the reset and clock definitions in the
proposed dt binding example, hence sending these first.

v4:
 - ensure we have enough space in ASPEED_G6_NUM_CLKS
v3:
 - split dt-bindings from clk changes
 - unify subject prefixes
v2:
 - based on feedback from Joel Stanley: avoid adding RESERVED clock
   definitions, allowing empty entries in aspeed_g6_gates instead.

Jeremy Kerr (5):
  clk: ast2600: allow empty entries in aspeed_g6_gates
  dt-bindings: clock: ast2600: Add top-level I3C clock
  clk: ast2600: Add full configs for I3C clocks
  dt-bindings: clock: ast2600: remove IC36 & I3C7 clock definitions
  dt-bindings: clock: ast2600: Add reset config for I3C

 drivers/clk/clk-ast2600.c                 | 43 ++++++++++++++++++-----
 include/dt-bindings/clock/ast2600-clock.h |  9 +++--
 2 files changed, 41 insertions(+), 11 deletions(-)

-- 
2.39.1

[PATCH v4 1/5] clk: ast2600: allow empty entries in aspeed_g6_gates

[Jeremy Kerr]

We're about to remove an entry from aspeed_g6_gates, but we won't want
to alter/reorder existing entries. Allow empty entries in this array.

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>

---
v3:
 - reword commit message
---
 drivers/clk/clk-ast2600.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/clk/clk-ast2600.c b/drivers/clk/clk-ast2600.c
index 9c3305bcb27a..1f08ff3c60fa 100644
--- a/drivers/clk/clk-ast2600.c
+++ b/drivers/clk/clk-ast2600.c
@@ -652,6 +652,9 @@ static int aspeed_g6_clk_probe(struct platform_device *pdev)
 		const struct aspeed_gate_data *gd = &aspeed_g6_gates[i];
 		u32 gate_flags;
 
+		if (!gd->name)
+			continue;
+
 		/*
 		 * Special case: the USB port 1 clock (bit 14) is always
 		 * working the opposite way from the other ones.
-- 
2.39.1

[PATCH v4 2/5] dt-bindings: clock: ast2600: Add top-level I3C clock

[Jeremy Kerr]

The ast2600 hardware has a top-level clock for all i3c controller
peripherals (then gated to each individual controller), so add a
top-level i3c clock line to control this.

This is a partial cherry-pick and rework of ed44b8cdfdb and 1a35eb926d7
from Aspeed's own tree, originally by Dylan Hung
<dylan_hung@aspeedtech.com>.

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>

---
v4:
 - use contiguous clock index
v3:
 - split into separate bindings & clk changes
v2:
 - reword commit message
---
 include/dt-bindings/clock/ast2600-clock.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/dt-bindings/clock/ast2600-clock.h b/include/dt-bindings/clock/ast2600-clock.h
index d8b0db2f7a7d..dd1581bfdf58 100644
--- a/include/dt-bindings/clock/ast2600-clock.h
+++ b/include/dt-bindings/clock/ast2600-clock.h
@@ -87,6 +87,7 @@
 #define ASPEED_CLK_MAC2RCLK		68
 #define ASPEED_CLK_MAC3RCLK		69
 #define ASPEED_CLK_MAC4RCLK		70
+#define ASPEED_CLK_I3C			71
 
 /* Only list resets here that are not part of a gate */
 #define ASPEED_RESET_ADC		55
-- 
2.39.1

[PATCH v4 3/5] clk: ast2600: Add full configs for I3C clocks

[Jeremy Kerr]

The current ast2600 I3C clock definitions are top-level (rather than
based on their actual hw sources: either HCLK or APLL), and include a
couple of definitions for (non-existent) i3c6 and i3c7.

Re-parent the individual I3C controller clocks to the main i3c clock,
explicitly sourced from the APLL rather than whatever G6_CLK_SELECTION5
was last set to.

While we're at it, remove the definitions for the i3c6 and i3c7 clock
lines; this hardware isn't present.

This is a partial cherry-pick and rework of ed44b8cdfdb and 1a35eb926d7
from Aspeed's own tree, originally by Dylan Hung
<dylan_hung@aspeedtech.com>.

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>

---
v4:
 - expand NUM_CLKS for the new I3C clock
v3:
 - split dt-bindings and clk changes
v2:
 - reword commit message
---
 drivers/clk/clk-ast2600.c | 40 ++++++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/drivers/clk/clk-ast2600.c b/drivers/clk/clk-ast2600.c
index 1f08ff3c60fa..d465d097e6f2 100644
--- a/drivers/clk/clk-ast2600.c
+++ b/drivers/clk/clk-ast2600.c
@@ -4,6 +4,7 @@
 
 #define pr_fmt(fmt) "clk-ast2600: " fmt
 
+#include <linux/bitfield.h>
 #include <linux/mfd/syscon.h>
 #include <linux/of_address.h>
 #include <linux/of_device.h>
@@ -15,7 +16,7 @@
 
 #include "clk-aspeed.h"
 
-#define ASPEED_G6_NUM_CLKS		71
+#define ASPEED_G6_NUM_CLKS		72
 
 #define ASPEED_G6_SILICON_REV		0x014
 #define CHIP_REVISION_ID			GENMASK(23, 16)
@@ -32,6 +33,18 @@
 #define ASPEED_G6_CLK_SELECTION1	0x300
 #define ASPEED_G6_CLK_SELECTION2	0x304
 #define ASPEED_G6_CLK_SELECTION4	0x310
+#define ASPEED_G6_CLK_SELECTION5	0x314
+#define   I3C_CLK_SELECTION		BIT(31)
+#define     I3C_CLK_SELECT_HCLK		0
+#define     I3C_CLK_SELECT_APLL_DIV	1
+#define   APLL_DIV_SELECTION		GENMASK(30, 28)
+#define     APLL_DIV_2			0b001
+#define     APLL_DIV_3			0b010
+#define     APLL_DIV_4			0b011
+#define     APLL_DIV_5			0b100
+#define     APLL_DIV_6			0b101
+#define     APLL_DIV_7			0b110
+#define     APLL_DIV_8			0b111
 
 #define ASPEED_HPLL_PARAM		0x200
 #define ASPEED_APLL_PARAM		0x210
@@ -97,14 +110,13 @@ static const struct aspeed_gate_data aspeed_g6_gates[] = {
 	[ASPEED_CLK_GATE_LHCCLK]	= { 37, -1, "lhclk-gate",	"lhclk", 0 },	/* LPC master/LPC+ */
 	/* Reserved 38 RSA: no longer used */
 	/* Reserved 39 */
-	[ASPEED_CLK_GATE_I3C0CLK]	= { 40,  40, "i3c0clk-gate",	NULL,	 0 },	/* I3C0 */
-	[ASPEED_CLK_GATE_I3C1CLK]	= { 41,  41, "i3c1clk-gate",	NULL,	 0 },	/* I3C1 */
-	[ASPEED_CLK_GATE_I3C2CLK]	= { 42,  42, "i3c2clk-gate",	NULL,	 0 },	/* I3C2 */
-	[ASPEED_CLK_GATE_I3C3CLK]	= { 43,  43, "i3c3clk-gate",	NULL,	 0 },	/* I3C3 */
-	[ASPEED_CLK_GATE_I3C4CLK]	= { 44,  44, "i3c4clk-gate",	NULL,	 0 },	/* I3C4 */
-	[ASPEED_CLK_GATE_I3C5CLK]	= { 45,  45, "i3c5clk-gate",	NULL,	 0 },	/* I3C5 */
-	[ASPEED_CLK_GATE_I3C6CLK]	= { 46,  46, "i3c6clk-gate",	NULL,	 0 },	/* I3C6 */
-	[ASPEED_CLK_GATE_I3C7CLK]	= { 47,  47, "i3c7clk-gate",	NULL,	 0 },	/* I3C7 */
+	[ASPEED_CLK_GATE_I3C0CLK]	= { 40,  40, "i3c0clk-gate",	"i3cclk", 0 }, /* I3C0 */
+	[ASPEED_CLK_GATE_I3C1CLK]	= { 41,  41, "i3c1clk-gate",	"i3cclk", 0 }, /* I3C1 */
+	[ASPEED_CLK_GATE_I3C2CLK]	= { 42,  42, "i3c2clk-gate",	"i3cclk", 0 }, /* I3C2 */
+	[ASPEED_CLK_GATE_I3C3CLK]	= { 43,  43, "i3c3clk-gate",	"i3cclk", 0 }, /* I3C3 */
+	[ASPEED_CLK_GATE_I3C4CLK]	= { 44,  44, "i3c4clk-gate",	"i3cclk", 0 }, /* I3C4 */
+	[ASPEED_CLK_GATE_I3C5CLK]	= { 45,  45, "i3c5clk-gate",	"i3cclk", 0 }, /* I3C5 */
+	/* Reserved: 46 & 47 */
 	[ASPEED_CLK_GATE_UART1CLK]	= { 48,  -1, "uart1clk-gate",	"uart",	 0 },	/* UART1 */
 	[ASPEED_CLK_GATE_UART2CLK]	= { 49,  -1, "uart2clk-gate",	"uart",	 0 },	/* UART2 */
 	[ASPEED_CLK_GATE_UART3CLK]	= { 50,  -1, "uart3clk-gate",	"uart",  0 },	/* UART3 */
@@ -775,6 +787,16 @@ static void __init aspeed_g6_cc(struct regmap *map)
 	/* USB 2.0 port1 phy 40MHz clock */
 	hw = clk_hw_register_fixed_rate(NULL, "usb-phy-40m", NULL, 0, 40000000);
 	aspeed_g6_clk_data->hws[ASPEED_CLK_USBPHY_40M] = hw;
+
+	/* i3c clock: source from apll, divide by 8 */
+	regmap_read(map, ASPEED_G6_CLK_SELECTION5, &val);
+	val &= ~(I3C_CLK_SELECTION | APLL_DIV_SELECTION);
+	val |= FIELD_PREP(I3C_CLK_SELECTION, I3C_CLK_SELECT_APLL_DIV);
+	val |= FIELD_PREP(APLL_DIV_SELECTION, APLL_DIV_8);
+	regmap_write(map, ASPEED_G6_CLK_SELECTION5, val);
+
+	hw = clk_hw_register_fixed_factor(NULL, "i3cclk", "apll", 0, 1, 8);
+	aspeed_g6_clk_data->hws[ASPEED_CLK_I3C] = hw;
 };
 
 static void __init aspeed_g6_cc_init(struct device_node *np)
-- 
2.39.1

[PATCH v4 4/5] dt-bindings: clock: ast2600: remove IC36 & I3C7 clock definitions

[Jeremy Kerr]

The current ast2600 clock definitions include entries for i3c6 and i3c7
devices, which don't exist: there are no clock control lines documented
for these, and only i3c devices 0 through 5 are present.

So, remove the definitions for I3C6 and I3C7. Although this is a
potential ABI-breaking change, there are no in-tree users of these, and
any references would be broken anyway, as the hardware doesn't exist.

This is a partial cherry-pick and rework of ed44b8cdfdb and 1a35eb926d7
from Aspeed's own tree, originally by Dylan Hung
<dylan_hung@aspeedtech.com>.

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---
v3:
 - split dt-bindings and clk changes
v2:
 - reword commit message
---
 include/dt-bindings/clock/ast2600-clock.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/dt-bindings/clock/ast2600-clock.h b/include/dt-bindings/clock/ast2600-clock.h
index dd1581bfdf58..b4d69103d722 100644
--- a/include/dt-bindings/clock/ast2600-clock.h
+++ b/include/dt-bindings/clock/ast2600-clock.h
@@ -57,8 +57,6 @@
 #define ASPEED_CLK_GATE_I3C3CLK		40
 #define ASPEED_CLK_GATE_I3C4CLK		41
 #define ASPEED_CLK_GATE_I3C5CLK		42
-#define ASPEED_CLK_GATE_I3C6CLK		43
-#define ASPEED_CLK_GATE_I3C7CLK		44
 
 #define ASPEED_CLK_GATE_FSICLK		45
 
-- 
2.39.1

[PATCH v4 5/5] dt-bindings: clock: ast2600: Add reset config for I3C

[Jeremy Kerr]

Add reset line definitions for the AST2600 I3C block's reset inputs.

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---
v2:
 - reword commit message
---
 include/dt-bindings/clock/ast2600-clock.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/dt-bindings/clock/ast2600-clock.h b/include/dt-bindings/clock/ast2600-clock.h
index b4d69103d722..b1c129977910 100644
--- a/include/dt-bindings/clock/ast2600-clock.h
+++ b/include/dt-bindings/clock/ast2600-clock.h
@@ -90,6 +90,12 @@
 /* Only list resets here that are not part of a gate */
 #define ASPEED_RESET_ADC		55
 #define ASPEED_RESET_JTAG_MASTER2	54
+#define ASPEED_RESET_I3C5		45
+#define ASPEED_RESET_I3C4		44
+#define ASPEED_RESET_I3C3		43
+#define ASPEED_RESET_I3C2		42
+#define ASPEED_RESET_I3C1		41
+#define ASPEED_RESET_I3C0		40
 #define ASPEED_RESET_I3C_DMA		39
 #define ASPEED_RESET_PWM		37
 #define ASPEED_RESET_PECI		36
-- 
2.39.1

Re: [PATCH v4 2/5] dt-bindings: clock: ast2600: Add top-level I3C clock

[Krzysztof Kozlowski]

On 28/02/2023 10:16, Jeremy Kerr wrote:
> The ast2600 hardware has a top-level clock for all i3c controller
> peripherals (then gated to each individual controller), so add a
> top-level i3c clock line to control this.
> 
> This is a partial cherry-pick and rework of ed44b8cdfdb and 1a35eb926d7
> from Aspeed's own tree, originally by Dylan Hung
> <dylan_hung@aspeedtech.com>.
> 
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
> 
> ---


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v4 3/5] clk: ast2600: Add full configs for I3C clocks

[Joel Stanley]

On Tue, 28 Feb 2023 at 09:16, Jeremy Kerr <jk@codeconstruct.com.au> wrote:
>
> The current ast2600 I3C clock definitions are top-level (rather than
> based on their actual hw sources: either HCLK or APLL), and include a
> couple of definitions for (non-existent) i3c6 and i3c7.
>
> Re-parent the individual I3C controller clocks to the main i3c clock,
> explicitly sourced from the APLL rather than whatever G6_CLK_SELECTION5
> was last set to.
>
> While we're at it, remove the definitions for the i3c6 and i3c7 clock
> lines; this hardware isn't present.
>
> This is a partial cherry-pick and rework of ed44b8cdfdb and 1a35eb926d7
> from Aspeed's own tree, originally by Dylan Hung
> <dylan_hung@aspeedtech.com>.
>
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>

Looks good. I have some suggestions below.

>
> ---
> v4:
>  - expand NUM_CLKS for the new I3C clock
> v3:
>  - split dt-bindings and clk changes
> v2:
>  - reword commit message
> ---
>  drivers/clk/clk-ast2600.c | 40 ++++++++++++++++++++++++++++++---------
>  1 file changed, 31 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/clk/clk-ast2600.c b/drivers/clk/clk-ast2600.c
> index 1f08ff3c60fa..d465d097e6f2 100644
> --- a/drivers/clk/clk-ast2600.c
> +++ b/drivers/clk/clk-ast2600.c
> @@ -4,6 +4,7 @@
>
>  #define pr_fmt(fmt) "clk-ast2600: " fmt
>
> +#include <linux/bitfield.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/of_address.h>
>  #include <linux/of_device.h>
> @@ -15,7 +16,7 @@
>
>  #include "clk-aspeed.h"
>
> -#define ASPEED_G6_NUM_CLKS             71
> +#define ASPEED_G6_NUM_CLKS             72

NUM_CLKS seems dangerous. Should we instead use ARRAY_SIZE(aspeed_g6_gates)?

Or at least a BUILD_BUG_ON( ARRAY_SIZE(aspeed_g6_gates) != ASPEED_G6_NUM_CLKS))?

>
>  #define ASPEED_G6_SILICON_REV          0x014
>  #define CHIP_REVISION_ID                       GENMASK(23, 16)
> @@ -32,6 +33,18 @@
>  #define ASPEED_G6_CLK_SELECTION1       0x300
>  #define ASPEED_G6_CLK_SELECTION2       0x304
>  #define ASPEED_G6_CLK_SELECTION4       0x310
> +#define ASPEED_G6_CLK_SELECTION5       0x314
> +#define   I3C_CLK_SELECTION            BIT(31)
> +#define     I3C_CLK_SELECT_HCLK                0
> +#define     I3C_CLK_SELECT_APLL_DIV    1
> +#define   APLL_DIV_SELECTION           GENMASK(30, 28)
> +#define     APLL_DIV_2                 0b001
> +#define     APLL_DIV_3                 0b010
> +#define     APLL_DIV_4                 0b011
> +#define     APLL_DIV_5                 0b100
> +#define     APLL_DIV_6                 0b101
> +#define     APLL_DIV_7                 0b110
> +#define     APLL_DIV_8                 0b111
>
>  #define ASPEED_HPLL_PARAM              0x200
>  #define ASPEED_APLL_PARAM              0x210
> @@ -97,14 +110,13 @@ static const struct aspeed_gate_data aspeed_g6_gates[] = {
>         [ASPEED_CLK_GATE_LHCCLK]        = { 37, -1, "lhclk-gate",       "lhclk", 0 },   /* LPC master/LPC+ */
>         /* Reserved 38 RSA: no longer used */
>         /* Reserved 39 */
> -       [ASPEED_CLK_GATE_I3C0CLK]       = { 40,  40, "i3c0clk-gate",    NULL,    0 },   /* I3C0 */
> -       [ASPEED_CLK_GATE_I3C1CLK]       = { 41,  41, "i3c1clk-gate",    NULL,    0 },   /* I3C1 */
> -       [ASPEED_CLK_GATE_I3C2CLK]       = { 42,  42, "i3c2clk-gate",    NULL,    0 },   /* I3C2 */
> -       [ASPEED_CLK_GATE_I3C3CLK]       = { 43,  43, "i3c3clk-gate",    NULL,    0 },   /* I3C3 */
> -       [ASPEED_CLK_GATE_I3C4CLK]       = { 44,  44, "i3c4clk-gate",    NULL,    0 },   /* I3C4 */
> -       [ASPEED_CLK_GATE_I3C5CLK]       = { 45,  45, "i3c5clk-gate",    NULL,    0 },   /* I3C5 */
> -       [ASPEED_CLK_GATE_I3C6CLK]       = { 46,  46, "i3c6clk-gate",    NULL,    0 },   /* I3C6 */
> -       [ASPEED_CLK_GATE_I3C7CLK]       = { 47,  47, "i3c7clk-gate",    NULL,    0 },   /* I3C7 */
> +       [ASPEED_CLK_GATE_I3C0CLK]       = { 40,  40, "i3c0clk-gate",    "i3cclk", 0 }, /* I3C0 */
> +       [ASPEED_CLK_GATE_I3C1CLK]       = { 41,  41, "i3c1clk-gate",    "i3cclk", 0 }, /* I3C1 */
> +       [ASPEED_CLK_GATE_I3C2CLK]       = { 42,  42, "i3c2clk-gate",    "i3cclk", 0 }, /* I3C2 */
> +       [ASPEED_CLK_GATE_I3C3CLK]       = { 43,  43, "i3c3clk-gate",    "i3cclk", 0 }, /* I3C3 */
> +       [ASPEED_CLK_GATE_I3C4CLK]       = { 44,  44, "i3c4clk-gate",    "i3cclk", 0 }, /* I3C4 */
> +       [ASPEED_CLK_GATE_I3C5CLK]       = { 45,  45, "i3c5clk-gate",    "i3cclk", 0 }, /* I3C5 */
> +       /* Reserved: 46 & 47 */
>         [ASPEED_CLK_GATE_UART1CLK]      = { 48,  -1, "uart1clk-gate",   "uart",  0 },   /* UART1 */
>         [ASPEED_CLK_GATE_UART2CLK]      = { 49,  -1, "uart2clk-gate",   "uart",  0 },   /* UART2 */
>         [ASPEED_CLK_GATE_UART3CLK]      = { 50,  -1, "uart3clk-gate",   "uart",  0 },   /* UART3 */
> @@ -775,6 +787,16 @@ static void __init aspeed_g6_cc(struct regmap *map)
>         /* USB 2.0 port1 phy 40MHz clock */
>         hw = clk_hw_register_fixed_rate(NULL, "usb-phy-40m", NULL, 0, 40000000);
>         aspeed_g6_clk_data->hws[ASPEED_CLK_USBPHY_40M] = hw;
> +
> +       /* i3c clock: source from apll, divide by 8 */
> +       regmap_read(map, ASPEED_G6_CLK_SELECTION5, &val);
> +       val &= ~(I3C_CLK_SELECTION | APLL_DIV_SELECTION);

Is there any value in registering a mux device here? See the emmc extclk device.

> +       val |= FIELD_PREP(I3C_CLK_SELECTION, I3C_CLK_SELECT_APLL_DIV);
> +       val |= FIELD_PREP(APLL_DIV_SELECTION, APLL_DIV_8);
> +       regmap_write(map, ASPEED_G6_CLK_SELECTION5, val);

This is a departure in style from the existing code. The existing code
did things like this:

        regmap_update_bits(map, ASPEED_G6_CLK_SELECTION1, GENMASK(10,
8), BIT(10));

Which uses the regmap API instead of FIELD_PREP macros. If you think
FIELD_PREP is better then that's fine.

> +
> +       hw = clk_hw_register_fixed_factor(NULL, "i3cclk", "apll", 0, 1, 8);
> +       aspeed_g6_clk_data->hws[ASPEED_CLK_I3C] = hw;
>  };
>
>  static void __init aspeed_g6_cc_init(struct device_node *np)
> --
> 2.39.1
>

Re: [PATCH v4 4/5] dt-bindings: clock: ast2600: remove IC36 & I3C7 clock definitions

[Joel Stanley]

On Tue, 28 Feb 2023 at 09:16, Jeremy Kerr <jk@codeconstruct.com.au> wrote:
>
> The current ast2600 clock definitions include entries for i3c6 and i3c7
> devices, which don't exist: there are no clock control lines documented
> for these, and only i3c devices 0 through 5 are present.
>
> So, remove the definitions for I3C6 and I3C7. Although this is a
> potential ABI-breaking change, there are no in-tree users of these, and
> any references would be broken anyway, as the hardware doesn't exist.
>
> This is a partial cherry-pick and rework of ed44b8cdfdb and 1a35eb926d7
> from Aspeed's own tree, originally by Dylan Hung
> <dylan_hung@aspeedtech.com>.
>
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Reviewed-by: Joel Stanley <joel@jms.id.au>

>
> ---
> v3:
>  - split dt-bindings and clk changes
> v2:
>  - reword commit message
> ---
>  include/dt-bindings/clock/ast2600-clock.h | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/include/dt-bindings/clock/ast2600-clock.h b/include/dt-bindings/clock/ast2600-clock.h
> index dd1581bfdf58..b4d69103d722 100644
> --- a/include/dt-bindings/clock/ast2600-clock.h
> +++ b/include/dt-bindings/clock/ast2600-clock.h
> @@ -57,8 +57,6 @@
>  #define ASPEED_CLK_GATE_I3C3CLK                40
>  #define ASPEED_CLK_GATE_I3C4CLK                41
>  #define ASPEED_CLK_GATE_I3C5CLK                42
> -#define ASPEED_CLK_GATE_I3C6CLK                43
> -#define ASPEED_CLK_GATE_I3C7CLK                44
>
>  #define ASPEED_CLK_GATE_FSICLK         45
>
> --
> 2.39.1
>

Re: [PATCH v4 5/5] dt-bindings: clock: ast2600: Add reset config for I3C

[Joel Stanley]

On Tue, 28 Feb 2023 at 09:16, Jeremy Kerr <jk@codeconstruct.com.au> wrote:
>
> Add reset line definitions for the AST2600 I3C block's reset inputs.
>
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>
> ---
> v2:
>  - reword commit message
> ---
>  include/dt-bindings/clock/ast2600-clock.h | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/include/dt-bindings/clock/ast2600-clock.h b/include/dt-bindings/clock/ast2600-clock.h
> index b4d69103d722..b1c129977910 100644
> --- a/include/dt-bindings/clock/ast2600-clock.h
> +++ b/include/dt-bindings/clock/ast2600-clock.h
> @@ -90,6 +90,12 @@
>  /* Only list resets here that are not part of a gate */

These definitions are part of a gate, yeah?

>  #define ASPEED_RESET_ADC               55
>  #define ASPEED_RESET_JTAG_MASTER2      54
> +#define ASPEED_RESET_I3C5              45
> +#define ASPEED_RESET_I3C4              44
> +#define ASPEED_RESET_I3C3              43
> +#define ASPEED_RESET_I3C2              42
> +#define ASPEED_RESET_I3C1              41
> +#define ASPEED_RESET_I3C0              40
>  #define ASPEED_RESET_I3C_DMA           39
>  #define ASPEED_RESET_PWM               37
>  #define ASPEED_RESET_PECI              36
> --
> 2.39.1
>

Re: [PATCH v4 1/5] clk: ast2600: allow empty entries in aspeed_g6_gates

[Joel Stanley]

On Tue, 28 Feb 2023 at 09:16, Jeremy Kerr <jk@codeconstruct.com.au> wrote:
>
> We're about to remove an entry from aspeed_g6_gates, but we won't want
> to alter/reorder existing entries. Allow empty entries in this array.

Nice. So it's recorded somewhere: the gates array should be
sequential, with the include/dt-bindings/clock/ast2600-clock.h defines
starting at 0 and counting up. If a clock gets mistakenly added and
needs to be removed, we can't have a "hole" in the array so instead we
leave it NULL and skip over adding it.

We could simply remove the bad entry but this would break the
theoretical case of a device tree with an old header, so we leave the
gaps in place :(

Reviewed-by: Joel Stanley <joel@jms.id.au>

Cheers,

Joel




>
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
>
> ---
> v3:
>  - reword commit message
> ---
>  drivers/clk/clk-ast2600.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/clk/clk-ast2600.c b/drivers/clk/clk-ast2600.c
> index 9c3305bcb27a..1f08ff3c60fa 100644
> --- a/drivers/clk/clk-ast2600.c
> +++ b/drivers/clk/clk-ast2600.c
> @@ -652,6 +652,9 @@ static int aspeed_g6_clk_probe(struct platform_device *pdev)
>                 const struct aspeed_gate_data *gd = &aspeed_g6_gates[i];
>                 u32 gate_flags;
>
> +               if (!gd->name)
> +                       continue;
> +
>                 /*
>                  * Special case: the USB port 1 clock (bit 14) is always
>                  * working the opposite way from the other ones.
> --
> 2.39.1
>

Re: [PATCH v4 2/5] dt-bindings: clock: ast2600: Add top-level I3C clock

[Joel Stanley]

On Tue, 28 Feb 2023 at 09:16, Jeremy Kerr <jk@codeconstruct.com.au> wrote:
>
> The ast2600 hardware has a top-level clock for all i3c controller
> peripherals (then gated to each individual controller), so add a
> top-level i3c clock line to control this.
>
> This is a partial cherry-pick and rework of ed44b8cdfdb and 1a35eb926d7
> from Aspeed's own tree, originally by Dylan Hung
> <dylan_hung@aspeedtech.com>.
>
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>

Reviewed-by: Joel Stanley <joel@jms.id.au>

>
> ---
> v4:
>  - use contiguous clock index
> v3:
>  - split into separate bindings & clk changes
> v2:
>  - reword commit message
> ---
>  include/dt-bindings/clock/ast2600-clock.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/dt-bindings/clock/ast2600-clock.h b/include/dt-bindings/clock/ast2600-clock.h
> index d8b0db2f7a7d..dd1581bfdf58 100644
> --- a/include/dt-bindings/clock/ast2600-clock.h
> +++ b/include/dt-bindings/clock/ast2600-clock.h
> @@ -87,6 +87,7 @@
>  #define ASPEED_CLK_MAC2RCLK            68
>  #define ASPEED_CLK_MAC3RCLK            69
>  #define ASPEED_CLK_MAC4RCLK            70
> +#define ASPEED_CLK_I3C                 71
>
>  /* Only list resets here that are not part of a gate */
>  #define ASPEED_RESET_ADC               55
> --
> 2.39.1
>

Re: [PATCH v4 3/5] clk: ast2600: Add full configs for I3C clocks

[Jeremy Kerr]

Hi Joel,

Thanks for the review. Some replies inline:

> > @@ -15,7 +16,7 @@
> > 
> >  #include "clk-aspeed.h"
> > 
> > -#define ASPEED_G6_NUM_CLKS             71
> > +#define ASPEED_G6_NUM_CLKS             72
> 
> NUM_CLKS seems dangerous. Should we instead use
> ARRAY_SIZE(aspeed_g6_gates)?

Yep, that would have saved me some time debugging. That would suit as a
separate change though, would you like it in the same series?

> >         /* USB 2.0 port1 phy 40MHz clock */
> >         hw = clk_hw_register_fixed_rate(NULL, "usb-phy-40m", NULL,
> > 0, 40000000);
> >         aspeed_g6_clk_data->hws[ASPEED_CLK_USBPHY_40M] = hw;
> > +
> > +       /* i3c clock: source from apll, divide by 8 */
> > +       regmap_read(map, ASPEED_G6_CLK_SELECTION5, &val);
> > +       val &= ~(I3C_CLK_SELECTION | APLL_DIV_SELECTION);
> 
> Is there any value in registering a mux device here? See the emmc
> extclk device.

We won't be doing any mux configuration here, so I figure the static
setup is fine.

> > +       val |= FIELD_PREP(I3C_CLK_SELECTION,
> > I3C_CLK_SELECT_APLL_DIV);
> > +       val |= FIELD_PREP(APLL_DIV_SELECTION, APLL_DIV_8);
> > +       regmap_write(map, ASPEED_G6_CLK_SELECTION5, val);
> 
> This is a departure in style from the existing code. The existing
> code did things like this:
> 
>         regmap_update_bits(map, ASPEED_G6_CLK_SELECTION1, GENMASK(10, 8), BIT(10));
> 
> Which uses the regmap API instead of FIELD_PREP macros.

Yep, that's much nicer, I'll change. The FIELD_PREP parts are just from
the initial ASPEED implementation.

Cheers,


Jeremy

Re: [PATCH v4 3/5] clk: ast2600: Add full configs for I3C clocks

[Joel Stanley]

On Wed, 1 Mar 2023 at 00:58, Jeremy Kerr <jk@codeconstruct.com.au> wrote:
>
> Hi Joel,
>
> Thanks for the review. Some replies inline:
>
> > > @@ -15,7 +16,7 @@
> > >
> > >  #include "clk-aspeed.h"
> > >
> > > -#define ASPEED_G6_NUM_CLKS             71
> > > +#define ASPEED_G6_NUM_CLKS             72
> >
> > NUM_CLKS seems dangerous. Should we instead use
> > ARRAY_SIZE(aspeed_g6_gates)?
>
> Yep, that would have saved me some time debugging. That would suit as a
> separate change though, would you like it in the same series?

Doesn't matter much. Perhaps include it at the end, for both the
aspeed drivers? But separately is fine too.

>
> > >         /* USB 2.0 port1 phy 40MHz clock */
> > >         hw = clk_hw_register_fixed_rate(NULL, "usb-phy-40m", NULL,
> > > 0, 40000000);
> > >         aspeed_g6_clk_data->hws[ASPEED_CLK_USBPHY_40M] = hw;
> > > +
> > > +       /* i3c clock: source from apll, divide by 8 */
> > > +       regmap_read(map, ASPEED_G6_CLK_SELECTION5, &val);
> > > +       val &= ~(I3C_CLK_SELECTION | APLL_DIV_SELECTION);
> >
> > Is there any value in registering a mux device here? See the emmc
> > extclk device.
>
> We won't be doing any mux configuration here, so I figure the static
> setup is fine.

ack

>
> > > +       val |= FIELD_PREP(I3C_CLK_SELECTION,
> > > I3C_CLK_SELECT_APLL_DIV);
> > > +       val |= FIELD_PREP(APLL_DIV_SELECTION, APLL_DIV_8);
> > > +       regmap_write(map, ASPEED_G6_CLK_SELECTION5, val);
> >
> > This is a departure in style from the existing code. The existing
> > code did things like this:
> >
> >         regmap_update_bits(map, ASPEED_G6_CLK_SELECTION1, GENMASK(10, 8), BIT(10));
> >
> > Which uses the regmap API instead of FIELD_PREP macros.
>
> Yep, that's much nicer, I'll change. The FIELD_PREP parts are just from
> the initial ASPEED implementation.

Cool.

Re: [PATCH v4 5/5] dt-bindings: clock: ast2600: Add reset config for I3C

[Jeremy Kerr]

Hi Joel,

> > diff --git a/include/dt-bindings/clock/ast2600-clock.h
> > b/include/dt-bindings/clock/ast2600-clock.h
> > index b4d69103d722..b1c129977910 100644
> > --- a/include/dt-bindings/clock/ast2600-clock.h
> > +++ b/include/dt-bindings/clock/ast2600-clock.h
> > @@ -90,6 +90,12 @@
> >  /* Only list resets here that are not part of a gate */
> 
> These definitions are part of a gate, yeah?

Well, no more "part of a gate" than all of the other definitions :)

All the defines in this section are references to individual bits in
the reset register banks in SCU040 & SCU050; the i3c set are the same
as the others there.

So I'm not sure what that comment is supposed to signify as to what
qualifies as a "gate" in the context of a reset...

Cheers,


Jeremy

Re: [PATCH v4 5/5] dt-bindings: clock: ast2600: Add reset config for I3C

[Joel Stanley]

On Wed, 1 Mar 2023 at 06:29, Jeremy Kerr <jk@codeconstruct.com.au> wrote:
>
> Hi Joel,
>
> > > diff --git a/include/dt-bindings/clock/ast2600-clock.h
> > > b/include/dt-bindings/clock/ast2600-clock.h
> > > index b4d69103d722..b1c129977910 100644
> > > --- a/include/dt-bindings/clock/ast2600-clock.h
> > > +++ b/include/dt-bindings/clock/ast2600-clock.h
> > > @@ -90,6 +90,12 @@
> > >  /* Only list resets here that are not part of a gate */
> >
> > These definitions are part of a gate, yeah?
>
> Well, no more "part of a gate" than all of the other definitions :)
>
> All the defines in this section are references to individual bits in
> the reset register banks in SCU040 & SCU050; the i3c set are the same
> as the others there.
>
> So I'm not sure what that comment is supposed to signify as to what
> qualifies as a "gate" in the context of a reset...

This is poor documentation from the author of the clock driver, which is me.

We only expose the reset lines in the device tree for resets that are
not associated with a clock line.

This is done because the aspeed docs specify we do a dance when enabling an IP:

 1. Place IP in reset
 2. Enable clock
 3. Delay
 4. Release reset

So we do this with the aspeed_g6_gates array. The rule is: any gate
with a number in the rst column doesn't have that reset line exposed.
That's what this cryptic comment in the header is warning about.

This was documented to some extent in the original commit message for
the 2400/2500 driver:

 https://git.kernel.org/torvalds/c/15ed8ce5f84e2b

We could hoist that out and put it in the source file(s).

Cheers,

Joel

Re: [PATCH v4 5/5] dt-bindings: clock: ast2600: Add reset config for I3C

[Jeremy Kerr]

Hi Joel,

> > So I'm not sure what that comment is supposed to signify as to what
> > qualifies as a "gate" in the context of a reset...
> 
> This is poor documentation from the author of the clock driver,

Hah, not that guy again!

> which is me.

oh.

> We only expose the reset lines in the device tree for resets that are
> not associated with a clock line.
> 
> This is done because the aspeed docs specify we do a dance when
> enabling an IP:
> 
>  1. Place IP in reset
>  2. Enable clock
>  3. Delay
>  4. Release reset
> 
> So we do this with the aspeed_g6_gates array. The rule is: any gate
> with a number in the rst column doesn't have that reset line exposed.
> That's what this cryptic comment in the header is warning about.

That makes sense, and means I can drop the explicit reset control from
the DTS, and then we don't need these definitions.

> This was documented to some extent in the original commit message for
> the 2400/2500 driver:
> 
>  https://git.kernel.org/torvalds/c/15ed8ce5f84e2b
> 
> We could hoist that out and put it in the source file(s).

Awesome, thanks for the explanation - I'll add a patch to do so.

Cheers,


Jeremy

Re: [PATCH v4 3/5] clk: ast2600: Add full configs for I3C clocks

[Jeremy Kerr]

Hi Joel,

> > > @@ -15,7 +16,7 @@
> > > 
> > >  #include "clk-aspeed.h"
> > > 
> > > -#define ASPEED_G6_NUM_CLKS             71
> > > +#define ASPEED_G6_NUM_CLKS             72
> > 
> > NUM_CLKS seems dangerous. Should we instead use
> > ARRAY_SIZE(aspeed_g6_gates)?
> 
> Yep, that would have saved me some time debugging. That would suit as
> a separate change though, would you like it in the same series?

No wait, it's not just ARRAY_SIZE(aspeed_g6_gates), there's a bunch of
manually-configured clocks in the aspeed_g6_clk_data->hws[] array too.

This might require a bit more of a restructure if we want to get rid of
the NUM_CLKS definitions...

Cheers,


Jeremy