[PATCH v5 0/6] Add definitions for AST2600 i3c clocks

[PATCH v5 0/6] Add definitions for AST2600 i3c clocks

[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.

v5:
 - use regmap_update_bits rather than FIELD_PREP
 - drop reset defines; resets should be handled by clock control
 - add comments for the above
 - add comment for NUM_CLKS semantics
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 (6):
  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
  clk: ast2600: Add comment about combined clock + reset handling
  dt-bindings: clock: ast2600: Expand comment on reset definitions

 drivers/clk/clk-ast2600.c                 | 67 ++++++++++++++++++++---
 include/dt-bindings/clock/ast2600-clock.h |  5 +-
 2 files changed, 60 insertions(+), 12 deletions(-)

-- 
2.39.1

[PATCH v5 1/6] 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.

Reviewed-by: Joel Stanley <joel@jms.id.au>
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 v5 2/6] 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>.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Joel Stanley <joel@jms.id.au>
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 v5 3/6] 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>

---
v5:
 - use regmap_update_bits()
 - add comment on NUM_CLKS semantics
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 | 43 +++++++++++++++++++++++++++++++--------
 1 file changed, 34 insertions(+), 9 deletions(-)

diff --git a/drivers/clk/clk-ast2600.c b/drivers/clk/clk-ast2600.c
index 1f08ff3c60fa..09f26ab5f9af 100644
--- a/drivers/clk/clk-ast2600.c
+++ b/drivers/clk/clk-ast2600.c
@@ -15,7 +15,11 @@
 
 #include "clk-aspeed.h"
 
-#define ASPEED_G6_NUM_CLKS		71
+/*
+ * This includes the gates (configured from aspeed_g6_gates), plus the
+ * explicitly-configured clocks (ASPEED_CLK_HPLL and up).
+ */
+#define ASPEED_G6_NUM_CLKS		72
 
 #define ASPEED_G6_SILICON_REV		0x014
 #define CHIP_REVISION_ID			GENMASK(23, 16)
@@ -32,6 +36,20 @@
 #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_SHIFT	31
+#define   I3C_CLK_SELECTION		BIT(31)
+#define     I3C_CLK_SELECT_HCLK		(0 << I3C_CLK_SELECTION_SHIFT)
+#define     I3C_CLK_SELECT_APLL_DIV	(1 << I3C_CLK_SELECTION_SHIFT)
+#define   APLL_DIV_SELECTION_SHIFT	28
+#define   APLL_DIV_SELECTION		GENMASK(30, 28)
+#define     APLL_DIV_2			(0b001 << APLL_DIV_SELECTION_SHIFT)
+#define     APLL_DIV_3			(0b010 << APLL_DIV_SELECTION_SHIFT)
+#define     APLL_DIV_4			(0b011 << APLL_DIV_SELECTION_SHIFT)
+#define     APLL_DIV_5			(0b100 << APLL_DIV_SELECTION_SHIFT)
+#define     APLL_DIV_6			(0b101 << APLL_DIV_SELECTION_SHIFT)
+#define     APLL_DIV_7			(0b110 << APLL_DIV_SELECTION_SHIFT)
+#define     APLL_DIV_8			(0b111 << APLL_DIV_SELECTION_SHIFT)
 
 #define ASPEED_HPLL_PARAM		0x200
 #define ASPEED_APLL_PARAM		0x210
@@ -97,14 +115,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 +792,14 @@ 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_update_bits(map, ASPEED_G6_CLK_SELECTION5,
+			   I3C_CLK_SELECTION | APLL_DIV_SELECTION,
+			   I3C_CLK_SELECT_APLL_DIV | APLL_DIV_8);
+
+	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 v5 4/6] 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>.

Reviewed-by: Joel Stanley <joel@jms.id.au>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.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

[PATCH v5 5/6] clk: ast2600: Add comment about combined clock + reset handling

[Jeremy Kerr]

Add a little description about how reset lines can be implicit with
clock enable/disable. This is mostly based on the commit message
from the original submission in 15ed8ce5f8.

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
---
 drivers/clk/clk-ast2600.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/clk/clk-ast2600.c b/drivers/clk/clk-ast2600.c
index 09f26ab5f9af..a094a2601a37 100644
--- a/drivers/clk/clk-ast2600.c
+++ b/drivers/clk/clk-ast2600.c
@@ -73,6 +73,27 @@ static void __iomem *scu_g6_base;
 static u8 soc_rev;
 
 /*
+ * The majority of the clocks in the system are gates paired with a reset
+ * controller that holds the IP in reset; this is represented by the @reset_idx
+ * member of entries here.
+ *
+ * This borrows from clk_hw_register_gate, but registers two 'gates', one
+ * to control the clock enable register and the other to control the reset
+ * IP. This allows us to enforce the ordering:
+ *
+ * 1. Place IP in reset
+ * 2. Enable clock
+ * 3. Delay
+ * 4. Release reset
+ *
+ * Consequently, if reset_idx is set, reset control is implicit: the clock
+ * consumer does not need its own reset handling, as enabling the clock will
+ * also deassert reset.
+ *
+ * There are some gates that do not have an associated reset; these are
+ * handled by using -1 as the index for the reset, and the consumer must
+ * explictly assert/deassert reset lines as required.
+ *
  * Clocks marked with CLK_IS_CRITICAL:
  *
  *  ref0 and ref1 are essential for the SoC to operate
-- 
2.39.1

[PATCH v5 6/6] dt-bindings: clock: ast2600: Expand comment on reset definitions

[Jeremy Kerr]

The current "not part of a gate" is a little ambiguous. Expand this a
little to clarify the reference to the paired clock + reset control.

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
---
 include/dt-bindings/clock/ast2600-clock.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/dt-bindings/clock/ast2600-clock.h b/include/dt-bindings/clock/ast2600-clock.h
index b4d69103d722..e149eee61588 100644
--- a/include/dt-bindings/clock/ast2600-clock.h
+++ b/include/dt-bindings/clock/ast2600-clock.h
@@ -87,7 +87,7 @@
 #define ASPEED_CLK_MAC4RCLK		70
 #define ASPEED_CLK_I3C			71
 
-/* Only list resets here that are not part of a gate */
+/* Only list resets here that are not part of a clock gate + reset pair */
 #define ASPEED_RESET_ADC		55
 #define ASPEED_RESET_JTAG_MASTER2	54
 #define ASPEED_RESET_I3C_DMA		39
-- 
2.39.1

Re: [PATCH v5 3/6] clk: ast2600: Add full configs for I3C clocks

[Joel Stanley]

On Thu, 2 Mar 2023 at 00:58, 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>

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

>
> ---
> v5:
>  - use regmap_update_bits()
>  - add comment on NUM_CLKS semantics
> 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 | 43 +++++++++++++++++++++++++++++++--------
>  1 file changed, 34 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/clk/clk-ast2600.c b/drivers/clk/clk-ast2600.c
> index 1f08ff3c60fa..09f26ab5f9af 100644
> --- a/drivers/clk/clk-ast2600.c
> +++ b/drivers/clk/clk-ast2600.c
> @@ -15,7 +15,11 @@
>
>  #include "clk-aspeed.h"
>
> -#define ASPEED_G6_NUM_CLKS             71
> +/*
> + * This includes the gates (configured from aspeed_g6_gates), plus the
> + * explicitly-configured clocks (ASPEED_CLK_HPLL and up).
> + */
> +#define ASPEED_G6_NUM_CLKS             72
>
>  #define ASPEED_G6_SILICON_REV          0x014
>  #define CHIP_REVISION_ID                       GENMASK(23, 16)
> @@ -32,6 +36,20 @@
>  #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_SHIFT      31
> +#define   I3C_CLK_SELECTION            BIT(31)
> +#define     I3C_CLK_SELECT_HCLK                (0 << I3C_CLK_SELECTION_SHIFT)
> +#define     I3C_CLK_SELECT_APLL_DIV    (1 << I3C_CLK_SELECTION_SHIFT)
> +#define   APLL_DIV_SELECTION_SHIFT     28
> +#define   APLL_DIV_SELECTION           GENMASK(30, 28)
> +#define     APLL_DIV_2                 (0b001 << APLL_DIV_SELECTION_SHIFT)
> +#define     APLL_DIV_3                 (0b010 << APLL_DIV_SELECTION_SHIFT)
> +#define     APLL_DIV_4                 (0b011 << APLL_DIV_SELECTION_SHIFT)
> +#define     APLL_DIV_5                 (0b100 << APLL_DIV_SELECTION_SHIFT)
> +#define     APLL_DIV_6                 (0b101 << APLL_DIV_SELECTION_SHIFT)
> +#define     APLL_DIV_7                 (0b110 << APLL_DIV_SELECTION_SHIFT)
> +#define     APLL_DIV_8                 (0b111 << APLL_DIV_SELECTION_SHIFT)
>
>  #define ASPEED_HPLL_PARAM              0x200
>  #define ASPEED_APLL_PARAM              0x210
> @@ -97,14 +115,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 +792,14 @@ 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_update_bits(map, ASPEED_G6_CLK_SELECTION5,
> +                          I3C_CLK_SELECTION | APLL_DIV_SELECTION,
> +                          I3C_CLK_SELECT_APLL_DIV | APLL_DIV_8);
> +
> +       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 v5 6/6] dt-bindings: clock: ast2600: Expand comment on reset definitions

[Joel Stanley]

On Thu, 2 Mar 2023 at 00:58, Jeremy Kerr <jk@codeconstruct.com.au> wrote:
>
> The current "not part of a gate" is a little ambiguous. Expand this a
> little to clarify the reference to the paired clock + reset control.

This confuses me, but if you think it helps then that's good.

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

>
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
> ---
>  include/dt-bindings/clock/ast2600-clock.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/dt-bindings/clock/ast2600-clock.h b/include/dt-bindings/clock/ast2600-clock.h
> index b4d69103d722..e149eee61588 100644
> --- a/include/dt-bindings/clock/ast2600-clock.h
> +++ b/include/dt-bindings/clock/ast2600-clock.h
> @@ -87,7 +87,7 @@
>  #define ASPEED_CLK_MAC4RCLK            70
>  #define ASPEED_CLK_I3C                 71
>
> -/* Only list resets here that are not part of a gate */
> +/* Only list resets here that are not part of a clock gate + reset pair */
>  #define ASPEED_RESET_ADC               55
>  #define ASPEED_RESET_JTAG_MASTER2      54
>  #define ASPEED_RESET_I3C_DMA           39
> --
> 2.39.1
>

Re: [PATCH v5 5/6] clk: ast2600: Add comment about combined clock + reset handling

[Joel Stanley]

On Thu, 2 Mar 2023 at 00:58, Jeremy Kerr <jk@codeconstruct.com.au> wrote:
>
> Add a little description about how reset lines can be implicit with
> clock enable/disable. This is mostly based on the commit message
> from the original submission in 15ed8ce5f8.

Excellent, thank you.

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

>
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
> ---
>  drivers/clk/clk-ast2600.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/drivers/clk/clk-ast2600.c b/drivers/clk/clk-ast2600.c
> index 09f26ab5f9af..a094a2601a37 100644
> --- a/drivers/clk/clk-ast2600.c
> +++ b/drivers/clk/clk-ast2600.c
> @@ -73,6 +73,27 @@ static void __iomem *scu_g6_base;
>  static u8 soc_rev;
>
>  /*
> + * The majority of the clocks in the system are gates paired with a reset
> + * controller that holds the IP in reset; this is represented by the @reset_idx
> + * member of entries here.
> + *
> + * This borrows from clk_hw_register_gate, but registers two 'gates', one
> + * to control the clock enable register and the other to control the reset
> + * IP. This allows us to enforce the ordering:
> + *
> + * 1. Place IP in reset
> + * 2. Enable clock
> + * 3. Delay
> + * 4. Release reset
> + *
> + * Consequently, if reset_idx is set, reset control is implicit: the clock
> + * consumer does not need its own reset handling, as enabling the clock will
> + * also deassert reset.
> + *
> + * There are some gates that do not have an associated reset; these are
> + * handled by using -1 as the index for the reset, and the consumer must
> + * explictly assert/deassert reset lines as required.
> + *
>   * Clocks marked with CLK_IS_CRITICAL:
>   *
>   *  ref0 and ref1 are essential for the SoC to operate
> --
> 2.39.1
>

Re: [PATCH v5 0/6] Add definitions for AST2600 i3c clocks

[Joel Stanley]

On Thu, 2 Mar 2023 at 00:58, Jeremy Kerr <jk@codeconstruct.com.au> wrote:
>
> 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.

Cool, looks good:

 clkin                                6        6        0    25000000
        0     0  50000         Y
    apll                              1        1        0   800000000
        0     0  50000         Y
       i3cclk                         2        2        0   100000000
        0     0  50000         Y
          i3c5clk-gate                0        0        0   100000000
        0     0  50000         N
          i3c4clk-gate                0        0        0   100000000
        0     0  50000         N
          i3c3clk-gate                0        0        0   100000000
        0     0  50000         N
          i3c2clk-gate                0        0        0   100000000
        0     0  50000         N
          i3c1clk-gate                1        1        0   100000000
        0     0  50000         Y
          i3c0clk-gate                1        1        0   100000000
        0     0  50000         Y

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

Thanks for the cleanups and documentation.

Cheers,

Joel

>
> v5:
>  - use regmap_update_bits rather than FIELD_PREP
>  - drop reset defines; resets should be handled by clock control
>  - add comments for the above
>  - add comment for NUM_CLKS semantics
> 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 (6):
>   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
>   clk: ast2600: Add comment about combined clock + reset handling
>   dt-bindings: clock: ast2600: Expand comment on reset definitions
>
>  drivers/clk/clk-ast2600.c                 | 67 ++++++++++++++++++++---
>  include/dt-bindings/clock/ast2600-clock.h |  5 +-
>  2 files changed, 60 insertions(+), 12 deletions(-)
>
> --
> 2.39.1
>

Re: [PATCH v5 6/6] dt-bindings: clock: ast2600: Expand comment on reset definitions

[Krzysztof Kozlowski]

On 02/03/2023 01:58, Jeremy Kerr wrote:
> The current "not part of a gate" is a little ambiguous. Expand this a
> little to clarify the reference to the paired clock + reset control.
> 
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
> ---

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

Best regards,
Krzysztof

Re: [PATCH v5 1/6] clk: ast2600: allow empty entries in aspeed_g6_gates

[Stephen Boyd]

Quoting Jeremy Kerr (2023-03-01 16:58:29)
> 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.
> 
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
> 
> ---

Applied to clk-next

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

[Stephen Boyd]

Quoting Jeremy Kerr (2023-03-01 16:58:30)
> 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>.
> 
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
> 
> ---

Applied to clk-next

Re: [PATCH v5 3/6] clk: ast2600: Add full configs for I3C clocks

[Stephen Boyd]

Quoting Jeremy Kerr (2023-03-01 16:58:31)
> 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>
> 
> ---

Applied to clk-next

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

[Stephen Boyd]

Quoting Jeremy Kerr (2023-03-01 16:58:32)
> 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>.
> 
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
> 
> ---

Applied to clk-next

Re: [PATCH v5 5/6] clk: ast2600: Add comment about combined clock + reset handling

[Stephen Boyd]

Quoting Jeremy Kerr (2023-03-01 16:58:33)
> Add a little description about how reset lines can be implicit with
> clock enable/disable. This is mostly based on the commit message
> from the original submission in 15ed8ce5f8.
> 
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
> ---

Applied to clk-next

Re: [PATCH v5 6/6] dt-bindings: clock: ast2600: Expand comment on reset definitions

[Stephen Boyd]

Quoting Jeremy Kerr (2023-03-01 16:58:34)
> The current "not part of a gate" is a little ambiguous. Expand this a
> little to clarify the reference to the paired clock + reset control.
> 
> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
> ---

Applied to clk-next