Re: [PATCH 1/2] clk: aspeed: Add full configs for i3c clocks

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 13/02/2023 04:48, Jeremy Kerr wrote:
> The current ast2600 i3c clock definitions are top-level, and include a
> couple of definitions for (non-existent) i3c6 and i3c7.
> 
> This change re-parents these to the main i3c clock, sourced from the
> APLL. We also remove the i3c6 and i3c7 definitions and mark those entries
> in the gates array as reserved - all entries in the array must be
> contiguous.
> 
> This change 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>
> ---
>  drivers/clk/clk-ast2600.c                 | 39 ++++++++++++++++++-----
>  include/dt-bindings/clock/ast2600-clock.h |  6 ++--

Bindings are separate patches. Always.

>  2 files changed, 34 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/clk/clk-ast2600.c b/drivers/clk/clk-ast2600.c
> index 9c3305bcb27a..24ad34440e1e 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>
> @@ -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,14 @@ 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 */
> +	[ASPEED_CLK_GATE_RESERVED43]	= { 46,  46, "reserved-43",	NULL,	0 },
> +	[ASPEED_CLK_GATE_RESERVED44]	= { 47,  47, "reserved-44",	NULL,	0 },
>  	[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 */
> @@ -772,6 +785,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)
> diff --git a/include/dt-bindings/clock/ast2600-clock.h b/include/dt-bindings/clock/ast2600-clock.h
> index d8b0db2f7a7d..98dc82702755 100644
> --- a/include/dt-bindings/clock/ast2600-clock.h
> +++ b/include/dt-bindings/clock/ast2600-clock.h
> @@ -57,9 +57,8 @@
>  #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

This breaks the ABI and commit msg does not provide justfication for it.



Best regards,
Krzysztof