Add definitions for AST2600 i3c clocks and resets
Subsystems (2)
v4 cover Add definitions for AST2600 i3c clocks and resets
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(-)
Signature · 1 line
2.39.1
PATCH v4 1/5 clk: ast2600: allow empty entries in aspeed_g6_gates
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.
---
v3:
- reword commit message
--- drivers/clk/clk-ast2600.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/clk/clk-ast2600.c +3 -0
@@ -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.
Signature · 1 line
2.39.1
PATCH v4 1/5 clk: ast2600: allow empty entries in aspeed_g6_gates
Quoted 3 lines · 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.
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 :(
Cheers,
Joel
Quoted 27 lines
Signed-off-by Jeremy Kerr <[email protected]>---
v3:
- reword commit message--- drivers/clk/clk-ast2600.c | 3 +++ 1 file changed, 3 insertions(+)drivers/clk/clk-ast2600.c +3
-1@@ -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
In reply to Jeremy KerrThe 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 <[email protected]>.
---
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(+)
include/dt-bindings/clock/ast2600-clock.h +1 -0
@@ -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
Signature · 1 line
2.39.1
PATCH v4 2/5 dt-bindings: clock: ast2600: Add top-level I3C clock
Quoted 11 lines · 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
ed44b8cdfdband1a35eb926d7from Aspeed's own tree, originally by Dylan Hung <[email protected]>.Signed-off-by Jeremy Kerr <[email protected]>---
Best regards,
Krzysztof
PATCH v4 2/5 dt-bindings: clock: ast2600: Add top-level I3C clock
In reply to Jeremy KerrQuoted 10 lines · 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
ed44b8cdfdband1a35eb926d7from Aspeed's own tree, originally by Dylan Hung <[email protected]>.Signed-off-by Jeremy Kerr <[email protected]>
Quoted 27 lines
---
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(+)include/dt-bindings/clock/ast2600-clock.h +1
-1@@ -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
In reply to Jeremy KerrThe 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 <[email protected]>.
---
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(-)
drivers/clk/clk-ast2600.c +31 -9
@@ -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)
Signature · 1 line
2.39.1
PATCH v4 3/5 clk: ast2600: Add full configs for I3C clocks
Quoted 9 lines · 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_SELECTION5was last set to.
Quoted 8 lines
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
ed44b8cdfdband1a35eb926d7from Aspeed's own tree, originally by Dylan Hung <[email protected]>.Signed-off-by Jeremy Kerr <[email protected]>
Looks good. I have some suggestions below.
Quoted 25 lines
---
v4:
- expandNUM_CLKSfor 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(-)drivers/clk/clk-ast2600.c +1
-0@@ -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>
drivers/clk/clk-ast2600.c line 19
Quoted 5 lines
drivers/clk/clk-ast2600.c +1
-1@@ -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))?
Quoted 45 lines
drivers/clk/clk-ast2600.c +19
-8#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 */
drivers/clk/clk-ast2600.c static void __init aspeed_g6_cc(struct regmap *map) · line 793
Quoted 7 lines
drivers/clk/clk-ast2600.c +4
-0@@ -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.
drivers/clk/clk-ast2600.c static void __init aspeed_g6_cc(struct regmap *map) · line 796
Quoted 3 lines
drivers/clk/clk-ast2600.c +3
-0+ 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.
Quoted 9 lines
drivers/clk/clk-ast2600.c +3
-1+ + 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 3/5 clk: ast2600: Add full configs for I3C clocks
Hi Joel,
Thanks for the review. Some replies inline:
Quoted 9 lines
Review
Quoted 6 lines
(quoted diff) +0
-0@@ -15,7 +16,7 @@?#include "
clk-aspeed.h"(quoted diff) +1
-1-#define ASPEED_G6_NUM_CLKS???????????? 71 +#define ASPEED_G6_NUM_CLKS???????????? 72
NUM_CLKSseems dangerous. Should we instead useARRAY_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?
Quoted 11 lines
Quoted 8 lines
??????? /* 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.
Quoted 11 lines
Quoted 4 lines
+?????? 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_PREPmacros.
Yep, that's much nicer, I'll change. The FIELD_PREP parts are just from the initial ASPEED implementation.
Cheers,
Jeremy
PATCH v4 3/5 clk: ast2600: Add full configs for I3C clocks
Quoted 9 lines · Jeremy Kerr
Hi Joel,
Thanks for the review. Some replies inline:
Quoted 4 lines
Quoted 4 lines
drivers/clk/clk-ast2600.c +0
-0@@ -15,7 +16,7 @@#include "clk-aspeed.h"
Quoted 8 lines
Quoted 5 lines
drivers/clk/clk-ast2600.c line 19
Quoted 2 lines
drivers/clk/clk-ast2600.c +1
-1-#define ASPEED_G6_NUM_CLKS 71 +#define ASPEED_G6_NUM_CLKS 72
NUM_CLKSseems dangerous. Should we instead useARRAY_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.
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
(quoted diff) +0
-0/* 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;drivers/clk/clk-ast2600.c +4
-0+ +/* i3c clock: source from apll, divide by 8 */ +regmap_read (map,ASPEED_G6_CLK_SELECTION5 , &val); + val &= ~(I3C_CLK_SELECTION |APLL_DIV_SELECTION );
Quoted 5 lines
Quoted 2 lines
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
Quoted 9 lines
Quoted 9 lines
drivers/clk/clk-ast2600.c static void __init aspeed_g6_cc(struct regmap *map) · line 796
Quoted 5 lines
+ val |= FIELD_PREP(I3C_CLK_SELECTION,
I3C_CLK_SELECT_APLL_DIV);drivers/clk/clk-ast2600.c +2
-0+ 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:
Quoted 6 lines
Quoted 3 lines
regmap_update_bits(map, ASPEED_G6_CLK_SELECTION1, GENMASK(10, 8), BIT(10));Which uses the regmap API instead of
FIELD_PREPmacros.Yep, that's much nicer, I'll change. The
FIELD_PREPparts are just from the initial ASPEED implementation.
Cool.
PATCH v4 3/5 clk: ast2600: Add full configs for I3C clocks
In reply to Jeremy KerrHi Joel,
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
(quoted diff) +0
-0@@ -15,7 +16,7 @@?#include "
clk-aspeed.h"
Quoted 8 lines
Quoted 5 lines
Review
Quoted 2 lines
(quoted diff) +1
-1-#define ASPEED_G6_NUM_CLKS???????????? 71 +#define ASPEED_G6_NUM_CLKS???????????? 72
NUM_CLKSseems dangerous. Should we instead useARRAY_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
PATCH v4 4/5 dt-bindings: clock: ast2600: remove IC36 & I3C7 clock definitions
In reply to Jeremy KerrThe 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 <[email protected]>.
---
v3:
- split dt-bindings and clk changes
v2:
- reword commit message
--- include/dt-bindings/clock/ast2600-clock.h | 2 -- 1 file changed, 2 deletions(-)
include/dt-bindings/clock/ast2600-clock.h +0 -2
@@ -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
Signature · 1 line
2.39.1
PATCH v4 4/5 dt-bindings: clock: ast2600: remove IC36 & I3C7 clock definitions
Quoted 9 lines · 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.
Quoted 6 lines
This is a partial cherry-pick and rework of
ed44b8cdfdband1a35eb926d7from Aspeed's own tree, originally by Dylan Hung <[email protected]>.Signed-off-by Jeremy Kerr <[email protected]>
Quoted 26 lines
---
v3:
- split dt-bindings and clk changes
v2:
- reword commit message--- include/dt-bindings/clock/ast2600-clock.h | 2 -- 1 file changed, 2 deletions(-)include/dt-bindings/clock/ast2600-clock.h +0
-3@@ -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
In reply to Jeremy KerrAdd reset line definitions for the AST2600 I3C block's reset inputs.
---
v2:
- reword commit message
--- include/dt-bindings/clock/ast2600-clock.h | 6 ++++++ 1 file changed, 6 insertions(+)
include/dt-bindings/clock/ast2600-clock.h +6 -0
@@ -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
Signature · 1 line
2.39.1
PATCH v4 5/5 dt-bindings: clock: ast2600: Add reset config for I3C
Quoted 18 lines · Jeremy Kerr
Add reset line definitions for the AST2600 I3C block's reset inputs.
Signed-off-by Jeremy Kerr <[email protected]>---
v2:
- reword commit message--- include/dt-bindings/clock/ast2600-clock.h | 6 ++++++ 1 file changed, 6 insertions(+)include/dt-bindings/clock/ast2600-clock.h +0
-0
include/dt-bindings/clock/ast2600-clock.h line 90
Quoted 1 line
include/dt-bindings/clock/ast2600-clock.h +0
-0@@ -90,6 +90,12 @@/* Only list resets here that are not part of a gate */
These definitions are part of a gate, yeah?
Quoted 14 lines
include/dt-bindings/clock/ast2600-clock.h +6
-1#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
PATCH v4 5/5 dt-bindings: clock: ast2600: Add reset config for I3C
Hi Joel,
Quoted 9 lines
Quoted 7 lines
include/dt-bindings/clock/ast2600-clock.h +0
-0@@ -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
PATCH v4 5/5 dt-bindings: clock: ast2600: Add reset config for I3C
Quoted 13 lines · Jeremy Kerr
Hi Joel,
Quoted 9 lines
include/dt-bindings/clock/ast2600-clock.h line 90
Quoted 7 lines
include/dt-bindings/clock/ast2600-clock.h +0
-0@@ -90,6 +90,12 @@/* Only list resets here that are not part of a gate */ These definitions are part of a gate, yeah?
Quoted 8 lines
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
PATCH v4 5/5 dt-bindings: clock: ast2600: Add reset config for I3C
Hi Joel,
Quoted 4 lines
Quoted 2 lines
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!
Quoted 1 line
which is me.
oh.
Quoted 6 lines
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:
Quoted 8 lines
?1. Place IP in reset
?2. Enable clock
?3. Delay
?4. Release resetSo we do this with the
aspeed_g6_gatesarray. 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.
Quoted 6 lines
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