Re: [PATCH 1/3] usb: musb-new: sunxi: do not attempt to access NULL SRAMC

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

On Fri,  2 Jun 2023 15:49:56 -0600
Sam Edwards <cfsworks@gmail.com> wrote:

Hi Sam,

thanks for taking care and sending patched!

> I believe that some sunxis (ncat2?) lack a SRAMC block,
> as accessing this region results in a data abort.

Ah, that's a good find, but I think it goes a bit deeper:
Just to be clear, "SRAMC" stands for "SRAM controller", not "SRAM memory
block C" (which other SoCs have, but indeed not the D1/T113s). However
we (sort of) have an "SRAM controller", although the manual and DT call
this IP block "syscon" these days. The address currently in ncat2.h is
just plain wrong, it's actually 0x3000000.

Now looking at the Linux MUSB driver, only the older SoCs (A10, A20,
F1C100s) need to switch some SRAM block to the OTG controller:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/musb/sunxi.c#n817

So the code is already wrong, we should not touch SYSCON+0x04 for any
newer SoCs, based on the compatible. We seem to be just lucky that newer
syscons don't have any register at offset 0x4.
And using SUNXI_SRAMC_BASE is somewhat dodgy to begin with, we should use
the "allwinner,sram" property from the DT, although this is surely more
complicated.

Do you have spare cycles to convert this over to look at the DT for this
SRAM part? For now you might just change the SRAM address in ncat2.h to
0x03000000, to be inline with the other SoCs.

Cheers,
Andre

> Checking
> that it's non-null before accessing it allows this to be
> set to NULL for SoCs where it's not present.
> 
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> ---
>  drivers/usb/musb-new/sunxi.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c
> index dc4cfc2194..6e60dd47e0 100644
> --- a/drivers/usb/musb-new/sunxi.c
> +++ b/drivers/usb/musb-new/sunxi.c
> @@ -174,13 +174,15 @@ static void USBC_ForceVbusValidToHigh(__iomem void *base)
>  
>  static void USBC_ConfigFIFO_Base(void)
>  {
> -	u32 reg_value;
> -
> -	/* 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);
> +	if (SUNXI_SRAMC_BASE) {
> +		u32 reg_value;
> +
> +		/* 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);
> +	}
>  }
>  
>  /******************************************************************************