Re: [PATCH v2 2/2] usb: musb-new: sunxi: clarify the purpose of SRAM initialization

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

On Fri,  9 Jun 2023 15:37:16 -0600
Sam Edwards <cfsworks@gmail.com> wrote:

Hi,

> This is largely a cosmetic change, with one functional distinction:
> We are now only setting BIT(0), and no longer clearing BIT(1).
> 
> The A20 manual confirms the purpose and bitwidth of this field, and we
> have also been doing it this way for a while in Linux-land: The prior
> narrative about this initialization being about configuring a FIFO has
> pretty much been debunked for years now.
> 
> This cleanup also adds a TODO comment about runtime discovery
> of the SYSCON base, per discussion with Andre.
> 
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> Cc: Andre Przywara <andre.przywara@arm.com>
> ---
>  drivers/usb/musb-new/sunxi.c | 38 +++++++++++++++++++++++++++---------
>  1 file changed, 29 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c
> index 1111a67eaf..be7faaa11e 100644
> --- a/drivers/usb/musb-new/sunxi.c
> +++ b/drivers/usb/musb-new/sunxi.c
> @@ -171,15 +171,29 @@ static void USBC_ForceVbusValidToHigh(__iomem void *base)
>  	musb_writel(base, USBC_REG_o_ISCR, reg_val);
>  }
>  
> -static void USBC_ConfigFIFO_Base(void)
> -{
> -	u32 reg_value;
> +/******************************************************************************
> + * Non-USBC register access needed for initialization
> + ******************************************************************************/
>  
> -	/* config usb fifo, 8kb mode */
> -	reg_value = readl(SUNXI_SRAMC_BASE + 0x04);
> -	reg_value &= ~(0x03 << 0);
> -	reg_value |= BIT(0);
> -	writel(reg_value, SUNXI_SRAMC_BASE + 0x04);
> +/*
> + * A10(s), A13, GR8, A20:
> + * switch ownership of SRAM block 'D' to the USB-OTG controller
> + */
> +static void sunxi_musb_claim_sram(void)
> +{
> +	/*
> +	 * TODO: Do not use hardcoded SUNXI_SRAMC_BASE; find the syscon base by
> +	 * traversing this OTG device's `allwinner,sram` FDT property and
> +	 * working upward to the system controller.
> +	 */
> +	void *syscon_base = (void *)SUNXI_SRAMC_BASE;

Can you pass this in as a parameter, then call the function with
SUNXI_SRAMC_BASE, for now? Because syscon_base will become a
member of struct sunxi_glue, which we have readily accessible in
sunxi_musb_init().
And no need for a comment, I think I will come up with something soon enough.

Many thanks!
Andre

> +
> +	/*
> +	 * BIT(0) of SRAM_CTRL_REG1 (syscon+0x04) controls SRAM-D ownership:
> +	 * '0' -> exclusive access by CPU
> +	 * '1' -> exclusive access by USB0
> +	 */
> +	setbits_le32(syscon_base + 0x04, BIT(0));
>  }
>  
>  /******************************************************************************
> @@ -313,7 +327,13 @@ static int sunxi_musb_init(struct musb *musb)
>  	musb->isr = sunxi_musb_interrupt;
>  
>  	if (glue->cfg->has_sram) {
> -		USBC_ConfigFIFO_Base();
> +		/*
> +		 * This is an older USB-OTG controller that Allwinner did not
> +		 * endow with a dedicated SRAM block; it instead uses SRAM
> +		 * block 'D', ownership of which needs to be handed over by
> +		 * the CPU
> +		 */
> +		sunxi_musb_claim_sram();
>  	}
>  
>  	USBC_EnableDpDmPullUp(musb->mregs);