Re: [PATCH 05/11] ARM: sunxi: Add support for F1C100s

[Jesse Taube <mr.bossman075@gmail.com>]

On 1/25/22 21:05, Andre Przywara wrote:
> On Tue,  4 Jan 2022 19:35:02 -0500
> Jesse Taube <mr.bossman075@gmail.com> wrote:
> 
> Hi,
> 
> can you please change the subject, to maybe mention clocks? Reads
> rather generic as is.
> 
>> From: Icenowy Zheng <icenowy@aosc.io>
>>
>> This patch aims to add header files for the F1C100s.
>> The header files included add support for gpio, dram and clocks.
> 
> This looks to be about clocks and UARTs only?
> 
>>
>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> Signed-off-by: Jesse Taube <Mr.Bossman075@gmail.com>
>> ---
>>   arch/arm/include/asm/arch-sunxi/clock.h       |  2 +-
>>   arch/arm/include/asm/arch-sunxi/clock_sun6i.h | 25 +++++++++++++++++++
>>   arch/arm/include/asm/arch-sunxi/cpu_sun4i.h   |  8 ++++++
>>   arch/arm/include/asm/arch-sunxi/gpio.h        |  1 +
>>   4 files changed, 35 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/include/asm/arch-sunxi/clock.h b/arch/arm/include/asm/arch-sunxi/clock.h
>> index cbbe5c7a1e..2cfd540742 100644
>> --- a/arch/arm/include/asm/arch-sunxi/clock.h
>> +++ b/arch/arm/include/asm/arch-sunxi/clock.h
>> @@ -19,7 +19,7 @@
>>   #elif defined(CONFIG_SUN50I_GEN_H6)
>>   #include <asm/arch/clock_sun50i_h6.h>
>>   #elif defined(CONFIG_MACH_SUN6I) || defined(CONFIG_MACH_SUN8I) || \
>> -      defined(CONFIG_MACH_SUN50I)
>> +      defined(CONFIG_MACH_SUN50I) || defined(CONFIG_MACH_SUNIV)
>>   #include <asm/arch/clock_sun6i.h>
>>   #elif defined(CONFIG_MACH_SUN9I)
>>   #include <asm/arch/clock_sun9i.h>
>> diff --git a/arch/arm/include/asm/arch-sunxi/clock_sun6i.h b/arch/arm/include/asm/arch-sunxi/clock_sun6i.h
>> index ee387127f3..5ecdf58bd5 100644
>> --- a/arch/arm/include/asm/arch-sunxi/clock_sun6i.h
>> +++ b/arch/arm/include/asm/arch-sunxi/clock_sun6i.h
>> @@ -168,6 +168,14 @@ struct sunxi_ccm_reg {
>>   	u32 pll_lock_ctrl;	/* 0x320 PLL lock control, R40 only */
>>   };
>>   
>> +/* apb1 bit field */
>> +#ifdef CONFIG_MACH_SUNIV
>> +#define APB1_GATE_UART_SHIFT	(20)
>> +#define APB1_GATE_UART_MASK		(0x7 << APB1_GATE_UART_SHIFT)
>> +#define APB1_GATE_TWI_SHIFT	(16)
>> +#define APB1_GATE_TWI_MASK		(0x7 << APB1_GATE_TWI_SHIFT)
> 
> The values match the user manual, but I don't see the *_MASK values used
> anywhere. Please just drop them.
> 
>> +#endif
>> +
>>   /* apb2 bit field */
>>   #define APB2_CLK_SRC_LOSC		(0x0 << 24)
>>   #define APB2_CLK_SRC_OSC24M		(0x1 << 24)
>> @@ -226,7 +234,12 @@ struct sunxi_ccm_reg {
>>   #define CCM_PLL5_CTRL_SIGMA_DELTA_EN	(0x1 << 24)
>>   #define CCM_PLL5_CTRL_EN		(0x1 << 31)
>>   
>> +#if !defined(CONFIG_MACH_SUNIV)
>>   #define PLL6_CFG_DEFAULT		0x90041811 /* 600 MHz */
>> +#else
>> +/* suniv pll6 doesn't have postdiv 2, so k is set to 0 */
>> +#define PLL6_CFG_DEFAULT		0x90041800
> 
> The manual says that bit 0 resets to 1, as in the other SoCs. It just
> seems to affect the "back door clock output", so we should leave it as
> 1.

Uh I don't understand.

>> +#endif
>>   
>>   #define CCM_PLL6_CTRL_N_SHIFT		8
>>   #define CCM_PLL6_CTRL_N_MASK		(0x1f << CCM_PLL6_CTRL_N_SHIFT)
>> @@ -310,6 +323,8 @@ struct sunxi_ccm_reg {
>>   #define AHB_GATE_OFFSET_USB0		25
>>   #define AHB_GATE_OFFSET_SATA		24
>>   #endif
>> +#define AHB_GATE_OFFSET_SPI1		21
>> +#define AHB_GATE_OFFSET_SPI0		20
> 
> I think this is legacy, we don't need this anymore with the DM clock
> driver we use in the SPI driver. The SPL SPI is separate anyway.
> Please just drop.
> 
>>   #define AHB_GATE_OFFSET_MCTL		14
>>   #define AHB_GATE_OFFSET_GMAC		17
>>   #define AHB_GATE_OFFSET_NAND0		13
>> @@ -458,6 +473,8 @@ struct sunxi_ccm_reg {
>>   #ifdef CONFIG_MACH_SUN8I_R40
>>   #define AHB_RESET_OFFSET_SATA		24
>>   #endif
>> +#define AHB_RESET_OFFSET_SPI1		21
>> +#define AHB_RESET_OFFSET_SPI0		20
> 
> Same here, I don't see that used anywhere.
> 
>>   #define AHB_RESET_OFFSET_GMAC		17
>>   #define AHB_RESET_OFFSET_MCTL		14
>>   #define AHB_RESET_OFFSET_MMC3		11
>> @@ -488,6 +505,14 @@ struct sunxi_ccm_reg {
>>   #define AHB_RESET_OFFSET_EPHY		2
>>   #define AHB_RESET_OFFSET_LVDS		0
>>   
>> +/* apb1 reset */
>> +#ifdef CONFIG_MACH_SUNIV
>> +#define APB1_RESET_UART_SHIFT	(20)
>> +#define APB1_RESET_UART_MASK		(0x7 << APB1_RESET_UART_SHIFT)
>> +#define APB1_RESET_TWI_SHIFT	(16)
>> +#define APB1_RESET_TWI_MASK		(0x7 << APB1_RESET_TWI_SHIFT)
>> +#endif
>> +
>>   /* apb2 reset */
>>   #define APB2_RESET_UART_SHIFT		(16)
>>   #define APB2_RESET_UART_MASK		(0xff << APB2_RESET_UART_SHIFT)
>> diff --git a/arch/arm/include/asm/arch-sunxi/cpu_sun4i.h b/arch/arm/include/asm/arch-sunxi/cpu_sun4i.h
>> index d4c795d89c..83178dd5c8 100644
>> --- a/arch/arm/include/asm/arch-sunxi/cpu_sun4i.h
>> +++ b/arch/arm/include/asm/arch-sunxi/cpu_sun4i.h
>> @@ -122,6 +122,12 @@ defined(CONFIG_MACH_SUN50I)
>>   
>>   #define SUNXI_SJTAG_BASE		0x01c23c00
>>   
>> +#ifdef CONFIG_MACH_SUNIV
>> +#define SUNXI_UART0_BASE		0x01c25000
>> +#define SUNXI_UART1_BASE		0x01c25400
>> +#define SUNXI_UART2_BASE		0x01c25800
>> +#endif
> 
> That should be moved down and merged into the other chunk below.
> 
> Rest looks alright.
> 
> Cheers,
> Andre
> 
> 
>> +
>>   #define SUNXI_TP_BASE			0x01c25000
>>   #define SUNXI_PMU_BASE			0x01c25400
>>   
>> @@ -129,9 +135,11 @@ defined(CONFIG_MACH_SUN50I)
>>   #define SUNXI_CPUCFG_BASE		0x01c25c00
>>   #endif
>>   
>> +#ifndef CONFIG_MACH_SUNIV
>>   #define SUNXI_UART0_BASE		0x01c28000
>>   #define SUNXI_UART1_BASE		0x01c28400
>>   #define SUNXI_UART2_BASE		0x01c28800
>> +#endif
>>   #define SUNXI_UART3_BASE		0x01c28c00
>>   #define SUNXI_UART4_BASE		0x01c29000
>>   #define SUNXI_UART5_BASE		0x01c29400
>> diff --git a/arch/arm/include/asm/arch-sunxi/gpio.h b/arch/arm/include/asm/arch-sunxi/gpio.h
>> index f3ab1aea0e..ced69f7dd4 100644
>> --- a/arch/arm/include/asm/arch-sunxi/gpio.h
>> +++ b/arch/arm/include/asm/arch-sunxi/gpio.h
>> @@ -165,6 +165,7 @@ enum sunxi_gpio_number {
>>   #define SUNXI_GPD_LVDS0		3
>>   #define SUNXI_GPD_PWM		2
>>   
>> +#define SUNIV_GPE_UART0		5
>>   #define SUN8I_GPE_TWI2		3
>>   #define SUN50I_GPE_TWI2		3
>>   
>