Re: [PATCH v1 2/3] mach-sunxi: Add spi boot for SUNIV

[Andre Przywara <andre.przywara@arm.com>]

On Wed,  9 Feb 2022 23:34:37 -0500
Jesse Taube <mr.bossman075@gmail.com> wrote:

Hi Jesse,

many thanks for sending this, I guess this makes those little boards much
more useful.

> Add support for the spi boot in spl on suniv architecture.

A more elaborate commit message would be welcomed.
Please mention the F1C100s, to give some more context. Also briefly
mention the differences, I think Icenowy summarised this quite well in
her version of the patch (06/27):

The suniv SoC come with a sun6i-style SPI controller at the base address
of sun4i SPI controller. The module clock of the SPI controller is also
missing.

You could add: "... is also missing, which leaves us running directly from
the AHB clock, set to 200 MHz."

> 
> Signed-off-by: Jesse Taube <Mr.Bossman075@gmail.com>
> ---
>  arch/arm/include/asm/arch-sunxi/gpio.h |  1 +
>  arch/arm/mach-sunxi/spl_spi_sunxi.c    | 26 +++++++++++++++++++-------
>  2 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm/include/asm/arch-sunxi/gpio.h b/arch/arm/include/asm/arch-sunxi/gpio.h
> index 7f7eb0517c..edd0fbf49f 100644
> --- a/arch/arm/include/asm/arch-sunxi/gpio.h
> +++ b/arch/arm/include/asm/arch-sunxi/gpio.h
> @@ -160,6 +160,7 @@ enum sunxi_gpio_number {
>  #define SUNXI_GPC_SDC2		3
>  #define SUN6I_GPC_SDC3		4
>  #define SUN50I_GPC_SPI0		4
> +#define SUNIV_GPC_SPI0		2
>  
>  #define SUNXI_GPD_LCD0		2
>  #define SUNXI_GPD_LVDS0		3
> diff --git a/arch/arm/mach-sunxi/spl_spi_sunxi.c b/arch/arm/mach-sunxi/spl_spi_sunxi.c
> index 910e805016..9a3666a2d7 100644
> --- a/arch/arm/mach-sunxi/spl_spi_sunxi.c
> +++ b/arch/arm/mach-sunxi/spl_spi_sunxi.c
> @@ -90,6 +90,7 @@
>  
>  #define SPI0_CLK_DIV_BY_2           0x1000
>  #define SPI0_CLK_DIV_BY_4           0x1001
> +#define SPI0_CLK_DIV_BY_32          0x100f
>  
>  /*****************************************************************************/
>  
> @@ -132,7 +133,8 @@ static uintptr_t spi0_base_address(void)
>  	if (IS_ENABLED(CONFIG_MACH_SUN50I_H6))
>  		return 0x05010000;
>  
> -	if (!is_sun6i_gen_spi())
> +	if (!is_sun6i_gen_spi() ||
> +	    IS_ENABLED(CONFIG_MACH_SUNIV))
>  		return 0x01C05000;
>  
>  	return 0x01C68000;
> @@ -156,11 +158,17 @@ static void spi0_enable_clock(void)
>  	if (!IS_ENABLED(CONFIG_MACH_SUN50I_H6))
>  		setbits_le32(CCM_AHB_GATING0, (1 << AHB_GATE_OFFSET_SPI0));
>  
> -	/* Divide by 4 */
> -	writel(SPI0_CLK_DIV_BY_4, base + (is_sun6i_gen_spi() ?
> -				  SUN6I_SPI0_CCTL : SUN4I_SPI0_CCTL));
> -	/* 24MHz from OSC24M */
> -	writel((1 << 31), CCM_SPI0_CLK);
> +	if (IS_ENABLED(CONFIG_MACH_SUNIV)) {
> +		/* Divide by 32, clock source is AHB clock 200MHz */
> +		writel(SPI0_CLK_DIV_BY_32, base + (IS_ENABLED(CONFIG_SUNXI_GEN_SUN6I) ?

This seems pointless and redundant, since we exactly know the register when
MACH_SUNIV is selected.

> +					   SUN6I_SPI0_CCTL : SUN4I_SPI0_CCTL));
> +	} else {
> +		/* Divide by 4 */
> +		writel(SPI0_CLK_DIV_BY_4, base + (is_sun6i_gen_spi() ?
> +					  SUN6I_SPI0_CCTL : SUN4I_SPI0_CCTL));
> +		/* 24MHz from OSC24M */
> +		writel((1 << 31), CCM_SPI0_CLK);
> +	}
>  
>  	if (is_sun6i_gen_spi()) {
>  		/* Enable SPI in the master mode and do a soft reset */
> @@ -191,7 +199,8 @@ static void spi0_disable_clock(void)
>  					     SUN4I_CTL_ENABLE);
>  
>  	/* Disable the SPI0 clock */
> -	writel(0, CCM_SPI0_CLK);
> +	if (!IS_ENABLED(CONFIG_MACH_SUNIV))
> +		writel(0, CCM_SPI0_CLK);
>  
>  	/* Close the SPI0 gate */
>  	if (!IS_ENABLED(CONFIG_MACH_SUN50I_H6))
> @@ -213,6 +222,9 @@ static void spi0_init(void)
>  	    IS_ENABLED(CONFIG_MACH_SUN50I_H6))
>  		pin_function = SUN50I_GPC_SPI0;
>  
> +	if (IS_ENABLED(CONFIG_MACH_SUNIV))
> +		pin_function = SUNIV_GPC_SPI0;
> +

It looks a bit better to tie connect this with an "else if" to the
previous comparison, since there is only one choice.

Cheers,
Andre

>  	spi0_pinmux_setup(pin_function);
>  	spi0_enable_clock();
>  }