Re: [PATCH v2 06/10] clk: sunxi: Add H3 clocks support

[Jens Kuske <jenskuske-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

Hi,

On 05/17/15 16:27, Maxime Ripard wrote:
> On Fri, May 15, 2015 at 06:38:56PM +0200, Jens Kuske wrote:
>> The H3 clock control unit is similar to the those of other sun8i family
>> members like the A23.
>>
>> It makes use of the new multiple parents option for the bus gates.
>> Some of the gates use the new AHB2 clock as parent, whose clock source
>> is muxable between AHB1 and PLL6/2. The documentation isn't totally clear
>> about which devices belong to AHB2 now, especially USB EHIC/OHIC, so it
>> is mostly based on Allwinner kernel source code.
>>
>> Signed-off-by: Jens Kuske <jenskuske-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  Documentation/devicetree/bindings/clock/sunxi.txt |  6 +++
>>  drivers/clk/sunxi/clk-sunxi.c                     | 50 ++++++++++++++++++++++-
>>  2 files changed, 55 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt
>> index 4fa11af..e367963 100644
>> --- a/Documentation/devicetree/bindings/clock/sunxi.txt
>> +++ b/Documentation/devicetree/bindings/clock/sunxi.txt
>> @@ -14,6 +14,7 @@ Required properties:
>>  	"allwinner,sun4i-a10-pll5-clk" - for the PLL5 clock
>>  	"allwinner,sun4i-a10-pll6-clk" - for the PLL6 clock
>>  	"allwinner,sun6i-a31-pll6-clk" - for the PLL6 clock on A31
>> +	"allwinner,sun8i-h3-pll6-clk" - for the PLL6 clock on H3
>>  	"allwinner,sun9i-a80-gt-clk" - for the GT bus clock on A80
>>  	"allwinner,sun4i-a10-cpu-clk" - for the CPU multiplexer clock
>>  	"allwinner,sun4i-a10-axi-clk" - for the AXI clock
>> @@ -28,6 +29,7 @@ Required properties:
>>  	"allwinner,sun7i-a20-ahb-gates-clk" - for the AHB gates on A20
>>  	"allwinner,sun6i-a31-ar100-clk" - for the AR100 on A31
>>  	"allwinner,sun6i-a31-ahb1-clk" - for the AHB1 clock on A31
>> +	"allwinner,sun8i-h3-ahb2-clk" - for the AHB2 clock on H3
>>  	"allwinner,sun6i-a31-ahb1-gates-clk" - for the AHB1 gates on A31
>>  	"allwinner,sun8i-a23-ahb1-gates-clk" - for the AHB1 gates on A23
>>  	"allwinner,sun9i-a80-ahb0-gates-clk" - for the AHB0 gates on A80
>> @@ -55,6 +57,7 @@ Required properties:
>>  	"allwinner,sun9i-a80-apb1-gates-clk" - for the APB1 gates on A80
>>  	"allwinner,sun6i-a31-apb2-gates-clk" - for the APB2 gates on A31
>>  	"allwinner,sun8i-a23-apb2-gates-clk" - for the APB2 gates on A23
>> +	"allwinner,sun8i-h3-bus-gates-clk" - for the bus gates on H3
>>  	"allwinner,sun5i-a13-mbus-clk" - for the MBUS clock on A13
>>  	"allwinner,sun4i-a10-mmc-clk" - for the MMC clock
>>  	"allwinner,sun9i-a80-mmc-clk" - for mmc module clocks on A80
>> @@ -95,6 +98,9 @@ The "allwinner,sun9i-a80-mmc-config-clk" clock also requires:
>>  For "allwinner,sun7i-a20-gmac-clk", the parent clocks shall be fixed rate
>>  dummy clocks at 25 MHz and 125 MHz, respectively. See example.
>>  
>> +For "allwinner,sun8i-h3-bus-gates-clk", the parent clocks shall be
>> +AHB1, AHB2, APB1 and APB2, in that order.
>> +
>>  Clock consumers should specify the desired clocks they use with a
>>  "clocks" phandle cell. Consumers that are using a gated clock should
>>  provide an additional ID in their clock property. This ID is the
>> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
>> index afe560c..79364be 100644
>> --- a/drivers/clk/sunxi/clk-sunxi.c
>> +++ b/drivers/clk/sunxi/clk-sunxi.c
>> @@ -771,6 +771,10 @@ static const struct mux_data sun6i_a31_ahb1_mux_data __initconst = {
>>  	.shift = 12,
>>  };
>>  
>> +static const struct mux_data sun8i_h3_ahb2_mux_data __initconst = {
>> +	.shift = 0,
>> +};
>> +
>>  static void __init sunxi_mux_clk_setup(struct device_node *node,
>>  				       struct mux_data *data)
>>  {
>> @@ -886,7 +890,7 @@ static void __init sunxi_divider_clk_setup(struct device_node *node,
>>   * sunxi_gates_clk_setup() - Setup function for leaf gates on clocks
>>   */
>>  
>> -#define SUNXI_GATES_MAX_SIZE	64
>> +#define SUNXI_GATES_MAX_SIZE	160
>>  
>>  struct gates_data {
>>  	DECLARE_BITMAP(mask, SUNXI_GATES_MAX_SIZE);
>> @@ -990,6 +994,36 @@ static const struct gates_data sun8i_a23_apb2_gates_data __initconst = {
>>  	.mask = {0x1F0007},
>>  };
>>  
>> +#define BUS_GATE_PARENT_AHB1	0
>> +#define BUS_GATE_PARENT_AHB2	1
>> +#define BUS_GATE_PARENT_APB1	2
>> +#define BUS_GATE_PARENT_APB2	3
>> +
>> +static const u8 sun8i_h3_bus_gates_parents[] __initconst = {
>> +	BUS_GATE_PARENT_AHB1, BUS_GATE_PARENT_AHB1, BUS_GATE_PARENT_AHB1,
>> +	BUS_GATE_PARENT_AHB1, BUS_GATE_PARENT_AHB1, BUS_GATE_PARENT_AHB1,
>> +	BUS_GATE_PARENT_AHB1,
> 
>> BUS_GATE_PARENT_AHB2,
>> BUS_GATE_PARENT_AHB1,
>> +	BUS_GATE_PARENT_AHB1, BUS_GATE_PARENT_AHB1, BUS_GATE_PARENT_AHB1,
>> +	BUS_GATE_PARENT_AHB1, BUS_GATE_PARENT_AHB1, BUS_GATE_PARENT_AHB1,
>> +	BUS_GATE_PARENT_AHB1, BUS_GATE_PARENT_AHB1, BUS_GATE_PARENT_AHB1,
> 
>> +	BUS_GATE_PARENT_AHB2, BUS_GATE_PARENT_AHB2, BUS_GATE_PARENT_AHB2,
> 
>> +	BUS_GATE_PARENT_AHB1, BUS_GATE_PARENT_AHB1, BUS_GATE_PARENT_AHB1,
>> +	BUS_GATE_PARENT_AHB1, BUS_GATE_PARENT_AHB1, BUS_GATE_PARENT_AHB1,
>> +	BUS_GATE_PARENT_AHB1, BUS_GATE_PARENT_AHB1, BUS_GATE_PARENT_AHB1,
>> +	BUS_GATE_PARENT_AHB1, BUS_GATE_PARENT_AHB1,
> 
>> BUS_GATE_PARENT_APB1,
>> +	BUS_GATE_PARENT_APB1, BUS_GATE_PARENT_APB1, BUS_GATE_PARENT_APB1,
>> +	BUS_GATE_PARENT_APB1, BUS_GATE_PARENT_APB1, BUS_GATE_PARENT_APB1,
> 
>> +	BUS_GATE_PARENT_APB2, BUS_GATE_PARENT_APB2, BUS_GATE_PARENT_APB2,
>> +	BUS_GATE_PARENT_APB2, BUS_GATE_PARENT_APB2, BUS_GATE_PARENT_APB2,
>> +	BUS_GATE_PARENT_APB2, BUS_GATE_PARENT_APB2,
> 
>> BUS_GATE_PARENT_AHB1,
>> +	BUS_GATE_PARENT_AHB1
>> +}
> 
> I think something like this:
> 
> if (index < .. || index > ..)
>     clk_parent = "ahb1"
> if (index == .. || index < ...)
>     clk_parent = "ahb2"
> etc...
> 
> Would be both easier to maintain and to read.

At least as long as there aren't too many disjunct groups.
But it would mean duplicate setup functions for each SoC (or something
like sun8i_h3_bus_gate_get_parent(int index))

But since you want to rework the gate setup anyway I think I'll wait
until then. Currently I can't see how this should look like.

> 
>> +
>> +static const struct gates_data sun8i_h3_bus_gates_data __initconst = {
>> +	.mask = {0xffbe6760, 0x00701b39, 0x00007123, 0x001f0007, 0x00000081},
>> +	.parents = sun8i_h3_bus_gates_parents,
>> +};
>> +
>>  static void __init sunxi_gates_clk_setup(struct device_node *node,
>>  					 struct gates_data *data)
>>  {
>> @@ -1110,6 +1144,16 @@ static const struct divs_data sun6i_a31_pll6_divs_data __initconst = {
>>  	}
>>  };
>>  
>> +static const struct divs_data sun8i_h3_pll6_divs_data __initconst = {
>> +	.factors = &sun6i_a31_pll6_data,
>> +	.ndivs = 3,
>> +	.div = {
>> +		{ .fixed = 2 }, /* normal output, pll6 */
>> +		{ .self = 1 }, /* base factor clock, pll6 x2 */
>> +		{ .fixed = 4 }, /* divided output, pll6 /2 */
>> +	}
>> +};
> 
> This is exactly the same clock as A31's PLL6, it shouldn't be declared
> as a different one.

Um, sorry, but I can't see the /2 output at A31's pll6.

Or shall we add the /2 output to A31's PLL6 and simply ignore it on A31?
I don't think this is a good idea, what happens if we need to add an
additional output on A31 later.

Regards,
Jens