mbox series

[0/5] clk: sunxi-ng: Allwinner R528/T113 clock support

Message ID 20221126191319.6404-1-samuel@sholland.org
Headers show
Series clk: sunxi-ng: Allwinner R528/T113 clock support | expand

Message

Samuel Holland Nov. 26, 2022, 7:13 p.m. UTC 
R528 and T113 are SoCs based on the same design as D1/D1s, but with ARM
CPUs instead of RISC-V. They use the same CCU implementation, meaning
the CCU has gates/resets for all peripherals present on any SoC in this
family. I verified the CAN bus bits are also present on D1/D1s.

Patches 1-2 clean up the Kconfig in preparation for patch 3, which
allows building the driver. Patches 4-5 add the missing driver bits.


András Szemző (1):
  clk: sunxi-ng: d1: Mark cpux clock as critical

Fabien Poussin (1):
  clk: sunxi-ng: d1: Add CAN bus gates and resets

Samuel Holland (3):
  clk: sunxi-ng: Remove duplicate ARCH_SUNXI dependencies
  clk: sunxi-ng: Move SoC driver conditions to dependencies
  clk: sunxi-ng: d1: Allow building for R528/T113

 drivers/clk/sunxi-ng/Kconfig              | 71 ++++++++++++-----------
 drivers/clk/sunxi-ng/ccu-sun20i-d1.c      | 13 ++++-
 drivers/clk/sunxi-ng/ccu-sun20i-d1.h      |  2 +-
 include/dt-bindings/clock/sun20i-d1-ccu.h |  2 +
 include/dt-bindings/reset/sun20i-d1-ccu.h |  2 +
 5 files changed, 53 insertions(+), 37 deletions(-)

Comments

Andre Przywara Dec. 3, 2022, 12:14 a.m. UTC | #1 
On Sat, 26 Nov 2022 13:13:15 -0600
Samuel Holland <samuel@sholland.org> wrote:

Hi,

thanks for addressing this!

> SUNXI_CCU already depends on ARCH_SUNXI, so adding the dependency to
> individual SoC drivers is redundant.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
>  drivers/clk/sunxi-ng/Kconfig | 43 ++++++++++++++++++------------------
>  1 file changed, 21 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/clk/sunxi-ng/Kconfig b/drivers/clk/sunxi-ng/Kconfig
> index 461537679c04..64cfa022e320 100644
> --- a/drivers/clk/sunxi-ng/Kconfig
> +++ b/drivers/clk/sunxi-ng/Kconfig
> @@ -14,43 +14,43 @@ config SUNIV_F1C100S_CCU
>  
>  config SUN20I_D1_CCU
>  	tristate "Support for the Allwinner D1 CCU"
> -	default RISCV && ARCH_SUNXI
> -	depends on (RISCV && ARCH_SUNXI) || COMPILE_TEST
> +	default RISCV
> +	depends on RISCV || COMPILE_TEST

I agree on the "depends" part: Indeed the guard symbol already covers
that, so it's redundant.
However I am not so sure about the "default" part: When ARCH_SUNXI is
deselected, but COMPILE_TEST in enabled, we default to every CCU driver
being built-in. I am not sure this is the intention, or at least
expected when doing compile testing?

>  
>  config SUN20I_D1_R_CCU
>  	tristate "Support for the Allwinner D1 PRCM CCU"
> -	default RISCV && ARCH_SUNXI
> -	depends on (RISCV && ARCH_SUNXI) || COMPILE_TEST
> +	default RISCV
> +	depends on RISCV || COMPILE_TEST
>  
>  config SUN50I_A64_CCU
>  	tristate "Support for the Allwinner A64 CCU"
> -	default ARM64 && ARCH_SUNXI
> -	depends on (ARM64 && ARCH_SUNXI) || COMPILE_TEST
> +	default ARM64
> +	depends on ARM64 || COMPILE_TEST

I wonder if this "depends" line was always wrong and should be fixed:
We can compile a 32-bit ARM kernel and run it on an A64. Granted this
requires a special bootloader or a hacked U-Boot (tried that), and
reveals some other issues with the decompressor, but technically there
is no 64-bit dependency in here.
The same goes for all the other ARM64 CCUs: Cortex-A53s can run AArch32
in all exception levels.
So shall we just completely remove the "depends" line for those, and
let SUNXI_CCU do that job? Or use use !RISCV || COMPILE_TEST?

Cheers,
Andre

>  
>  config SUN50I_A100_CCU
>  	tristate "Support for the Allwinner A100 CCU"
> -	default ARM64 && ARCH_SUNXI
> -	depends on (ARM64 && ARCH_SUNXI) || COMPILE_TEST
> +	default ARM64
> +	depends on ARM64 || COMPILE_TEST
>  
>  config SUN50I_A100_R_CCU
>  	tristate "Support for the Allwinner A100 PRCM CCU"
> -	default ARM64 && ARCH_SUNXI
> -	depends on (ARM64 && ARCH_SUNXI) || COMPILE_TEST
> +	default ARM64
> +	depends on ARM64 || COMPILE_TEST
>  
>  config SUN50I_H6_CCU
>  	tristate "Support for the Allwinner H6 CCU"
> -	default ARM64 && ARCH_SUNXI
> -	depends on (ARM64 && ARCH_SUNXI) || COMPILE_TEST
> +	default ARM64
> +	depends on ARM64 || COMPILE_TEST
>  
>  config SUN50I_H616_CCU
>  	tristate "Support for the Allwinner H616 CCU"
> -	default ARM64 && ARCH_SUNXI
> -	depends on (ARM64 && ARCH_SUNXI) || COMPILE_TEST
> +	default ARM64
> +	depends on ARM64 || COMPILE_TEST
>  
>  config SUN50I_H6_R_CCU
>  	tristate "Support for the Allwinner H6 and H616 PRCM CCU"
> -	default ARM64 && ARCH_SUNXI
> -	depends on (ARM64 && ARCH_SUNXI) || COMPILE_TEST
> +	default ARM64
> +	depends on ARM64 || COMPILE_TEST
>  
>  config SUN4I_A10_CCU
>  	tristate "Support for the Allwinner A10/A20 CCU"
> @@ -71,8 +71,7 @@ config SUN6I_A31_CCU
>  
>  config SUN6I_RTC_CCU
>  	tristate "Support for the Allwinner H616/R329 RTC CCU"
> -	default ARCH_SUNXI
> -	depends on ARCH_SUNXI || COMPILE_TEST
> +	default y
>  
>  config SUN8I_A23_CCU
>  	tristate "Support for the Allwinner A23 CCU"
> @@ -91,8 +90,8 @@ config SUN8I_A83T_CCU
>  
>  config SUN8I_H3_CCU
>  	tristate "Support for the Allwinner H3 CCU"
> -	default MACH_SUN8I || (ARM64 && ARCH_SUNXI)
> -	depends on MACH_SUN8I || (ARM64 && ARCH_SUNXI) || COMPILE_TEST
> +	default MACH_SUN8I || ARM64
> +	depends on MACH_SUN8I || ARM64 || COMPILE_TEST
>  
>  config SUN8I_V3S_CCU
>  	tristate "Support for the Allwinner V3s CCU"
> @@ -101,7 +100,7 @@ config SUN8I_V3S_CCU
>  
>  config SUN8I_DE2_CCU
>  	tristate "Support for the Allwinner SoCs DE2 CCU"
> -	default MACH_SUN8I || (ARM64 && ARCH_SUNXI)
> +	default MACH_SUN8I || ARM64
>  
>  config SUN8I_R40_CCU
>  	tristate "Support for the Allwinner R40 CCU"
> @@ -115,6 +114,6 @@ config SUN9I_A80_CCU
>  
>  config SUN8I_R_CCU
>  	tristate "Support for Allwinner SoCs' PRCM CCUs"
> -	default MACH_SUN8I || (ARCH_SUNXI && ARM64)
> +	default MACH_SUN8I || ARM64
>  
>  endif
Andre Przywara Dec. 3, 2022, 12:22 a.m. UTC | #2 
On Sat, 26 Nov 2022 13:13:18 -0600
Samuel Holland <samuel@sholland.org> wrote:

> From: András Szemző <szemzo.andras@gmail.com>
> 
> Some SoCs in the D1 family feature ARM CPUs instead of a RISC-V CPU.
> In that case, the CPUs are driven from the 'cpux' clock, so it needs
> to be marked as critical.

Yes, my board hangs without that patch somewhere into the boot, and
this patch fixes it.

Can you also explain in the commit message why this is needed? IIRC
the CPU node itself does not "consume" the clock, this would only be
done by DVFS code?
And it might be worth noting that we do this for every other
Allwinner SoC as well.

> Signed-off-by: András Szemző <szemzo.andras@gmail.com>
> Signed-off-by: Samuel Holland <samuel@sholland.org>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
> 
>  drivers/clk/sunxi-ng/ccu-sun20i-d1.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/sunxi-ng/ccu-sun20i-d1.c b/drivers/clk/sunxi-ng/ccu-sun20i-d1.c
> index 8ef3cdeb7962..c5a7df93602c 100644
> --- a/drivers/clk/sunxi-ng/ccu-sun20i-d1.c
> +++ b/drivers/clk/sunxi-ng/ccu-sun20i-d1.c
> @@ -240,7 +240,7 @@ static const struct clk_parent_data cpux_parents[] = {
>  	{ .hw = &pll_periph0_800M_clk.common.hw },
>  };
>  static SUNXI_CCU_MUX_DATA(cpux_clk, "cpux", cpux_parents,
> -			  0x500, 24, 3, CLK_SET_RATE_PARENT);
> +			  0x500, 24, 3, CLK_SET_RATE_PARENT | CLK_IS_CRITICAL);
>  
>  static const struct clk_hw *cpux_hws[] = { &cpux_clk.common.hw };
>  static SUNXI_CCU_M_HWS(cpux_axi_clk, "cpux-axi",
Samuel Holland Dec. 3, 2022, 1:52 a.m. UTC | #3 
On 12/2/22 18:14, Andre Przywara wrote:
> On Sat, 26 Nov 2022 13:13:15 -0600
> Samuel Holland <samuel@sholland.org> wrote:
> 
> Hi,
> 
> thanks for addressing this!
> 
>> SUNXI_CCU already depends on ARCH_SUNXI, so adding the dependency to
>> individual SoC drivers is redundant.
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>
>>  drivers/clk/sunxi-ng/Kconfig | 43 ++++++++++++++++++------------------
>>  1 file changed, 21 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/clk/sunxi-ng/Kconfig b/drivers/clk/sunxi-ng/Kconfig
>> index 461537679c04..64cfa022e320 100644
>> --- a/drivers/clk/sunxi-ng/Kconfig
>> +++ b/drivers/clk/sunxi-ng/Kconfig
>> @@ -14,43 +14,43 @@ config SUNIV_F1C100S_CCU
>>  
>>  config SUN20I_D1_CCU
>>  	tristate "Support for the Allwinner D1 CCU"
>> -	default RISCV && ARCH_SUNXI
>> -	depends on (RISCV && ARCH_SUNXI) || COMPILE_TEST
>> +	default RISCV
>> +	depends on RISCV || COMPILE_TEST
> 
> I agree on the "depends" part: Indeed the guard symbol already covers
> that, so it's redundant.
> However I am not so sure about the "default" part: When ARCH_SUNXI is
> deselected, but COMPILE_TEST in enabled, we default to every CCU driver
> being built-in. I am not sure this is the intention, or at least
> expected when doing compile testing?

SUNXI_CCU, which these depend on, is still "default ARCH_SUNXI", so if
you have ARCH_SUNXI disabled, you only get any drivers if you manually
enable SUNXI_CCU. I mentioned this in the patch 2 description, but maybe
I should move that comment here.

>>  
>>  config SUN20I_D1_R_CCU
>>  	tristate "Support for the Allwinner D1 PRCM CCU"
>> -	default RISCV && ARCH_SUNXI
>> -	depends on (RISCV && ARCH_SUNXI) || COMPILE_TEST
>> +	default RISCV
>> +	depends on RISCV || COMPILE_TEST
>>  
>>  config SUN50I_A64_CCU
>>  	tristate "Support for the Allwinner A64 CCU"
>> -	default ARM64 && ARCH_SUNXI
>> -	depends on (ARM64 && ARCH_SUNXI) || COMPILE_TEST
>> +	default ARM64
>> +	depends on ARM64 || COMPILE_TEST
> 
> I wonder if this "depends" line was always wrong and should be fixed:
> We can compile a 32-bit ARM kernel and run it on an A64. Granted this
> requires a special bootloader or a hacked U-Boot (tried that), and
> reveals some other issues with the decompressor, but technically there
> is no 64-bit dependency in here.
> The same goes for all the other ARM64 CCUs: Cortex-A53s can run AArch32
> in all exception levels.

I was trying to simplify things by hiding irrelevant options, and you
bring up an edge case of an edge case. :) I am okay with relaxing the
dependency, though I would want to leave them disabled by default for
32-bit kernels (excluding them from the change in patch 2).

> So shall we just completely remove the "depends" line for those, and
> let SUNXI_CCU do that job? Or use use !RISCV || COMPILE_TEST?

That, or we could add MACH_SUN8I to the condition. I don't have a strong
opinion.

Regards,
Samuel
Andre Przywara Dec. 3, 2022, 1:57 p.m. UTC | #4 
On Fri, 2 Dec 2022 19:52:41 -0600
Samuel Holland <samuel@sholland.org> wrote:

Hi Samuel,

> On 12/2/22 18:14, Andre Przywara wrote:
> > On Sat, 26 Nov 2022 13:13:15 -0600
> > Samuel Holland <samuel@sholland.org> wrote:
> > 
> > Hi,
> > 
> > thanks for addressing this!
> >   
> >> SUNXI_CCU already depends on ARCH_SUNXI, so adding the dependency to
> >> individual SoC drivers is redundant.
> >>
> >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> >> ---
> >>
> >>  drivers/clk/sunxi-ng/Kconfig | 43 ++++++++++++++++++------------------
> >>  1 file changed, 21 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/drivers/clk/sunxi-ng/Kconfig b/drivers/clk/sunxi-ng/Kconfig
> >> index 461537679c04..64cfa022e320 100644
> >> --- a/drivers/clk/sunxi-ng/Kconfig
> >> +++ b/drivers/clk/sunxi-ng/Kconfig
> >> @@ -14,43 +14,43 @@ config SUNIV_F1C100S_CCU
> >>  
> >>  config SUN20I_D1_CCU
> >>  	tristate "Support for the Allwinner D1 CCU"
> >> -	default RISCV && ARCH_SUNXI
> >> -	depends on (RISCV && ARCH_SUNXI) || COMPILE_TEST
> >> +	default RISCV
> >> +	depends on RISCV || COMPILE_TEST  
> > 
> > I agree on the "depends" part: Indeed the guard symbol already covers
> > that, so it's redundant.
> > However I am not so sure about the "default" part: When ARCH_SUNXI is
> > deselected, but COMPILE_TEST in enabled, we default to every CCU driver
> > being built-in. I am not sure this is the intention, or at least
> > expected when doing compile testing?  
> 
> SUNXI_CCU, which these depend on, is still "default ARCH_SUNXI", so if
> you have ARCH_SUNXI disabled, you only get any drivers if you manually
> enable SUNXI_CCU. I mentioned this in the patch 2 description, but maybe
> I should move that comment here.

Yeah, I read this later on, I guess it's fine then.

> 
> >>  
> >>  config SUN20I_D1_R_CCU
> >>  	tristate "Support for the Allwinner D1 PRCM CCU"
> >> -	default RISCV && ARCH_SUNXI
> >> -	depends on (RISCV && ARCH_SUNXI) || COMPILE_TEST
> >> +	default RISCV
> >> +	depends on RISCV || COMPILE_TEST
> >>  
> >>  config SUN50I_A64_CCU
> >>  	tristate "Support for the Allwinner A64 CCU"
> >> -	default ARM64 && ARCH_SUNXI
> >> -	depends on (ARM64 && ARCH_SUNXI) || COMPILE_TEST
> >> +	default ARM64
> >> +	depends on ARM64 || COMPILE_TEST  
> > 
> > I wonder if this "depends" line was always wrong and should be fixed:
> > We can compile a 32-bit ARM kernel and run it on an A64. Granted this
> > requires a special bootloader or a hacked U-Boot (tried that), and
> > reveals some other issues with the decompressor, but technically there
> > is no 64-bit dependency in here.
> > The same goes for all the other ARM64 CCUs: Cortex-A53s can run AArch32
> > in all exception levels.  
> 
> I was trying to simplify things by hiding irrelevant options, and you
> bring up an edge case of an edge case. :) I am okay with relaxing the
> dependency, though I would want to leave them disabled by default for
> 32-bit kernels (excluding them from the change in patch 2).

Yes, definitely, that was the idea.

And sorry for being a nuisance, but I think this "depends on ARCH_SUNXI"
here is and was always misplaced. In contrast to things like "depends
on PCI" or "depends on GPIOLIB", there is no real dependency on
ARCH_SUNXI or even ARM/RISCV here, it's more a "only useful on
ARCH_SUNXI".
And this ARM vs ARM64 was just another rationale for not being
overzealous with the dependency.

But I see that this is an orthogonal discussion to this patch, so this
should not block it. I will meditate over both patches again, since I
have the gut feeling that the end result is fine.

Cheers,
Andre

> 
> > So shall we just completely remove the "depends" line for those, and
> > let SUNXI_CCU do that job? Or use use !RISCV || COMPILE_TEST?  
> 
> That, or we could add MACH_SUN8I to the condition. I don't have a strong
> opinion.
> 
> Regards,
> Samuel
>