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

[Andre Przywara]

On Wed,  7 Jun 2023 17:16:44 -0600
Sam Edwards <cfsworks at gmail.com> wrote:

Hi Sam,

> This is largely a cosmetic change, with one functional distinction:
> We are now only setting BIT(0), and no longer clearing BIT(1).
> 
> The new comments and function name are not from any official source,
> but are updated to mirror how the Linux kernel sources treat this
> mystery magic bit. If we wanted to confirm that this speculation is
> correct, we could verify that SRAM-D is inaccessible whenever the
> bit is set, and then try clearing it again while the MUSB is in use
> to see what debris gets left behind in SRAM-D.
> 
> This cleanup also adds a TODO comment about runtime discovery
> of the SYSCON base, per discussion with Andre.

thanks for sending this, looks good. Some stylistic comments below.

> 
> Signed-off-by: Sam Edwards <CFSworks at gmail.com>
> Cc: Andre Przywara <andre.przywara at arm.com>
> ---
>  drivers/usb/musb-new/sunxi.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c
> index c05c0d5561..2b954601a0 100644
> --- a/drivers/usb/musb-new/sunxi.c
> +++ b/drivers/usb/musb-new/sunxi.c
> @@ -173,15 +173,23 @@ static void USBC_ForceVbusValidToHigh(__iomem void *base)
>  	musb_writel(base, USBC_REG_o_ISCR, reg_val);
>  }
>  
> -static void USBC_ConfigFIFO_Base(void)
> +/******************************************************************************
> + * Non-USBC register access needed for initialization
> + ******************************************************************************/
> +
> +/* A10(s), A13, GR8, A20:
> + * switch ownership of SRAM block 'D' to the USB-OTG controller */
> +#define OFF_SUNXI_SRAMCTRL_D		(0x04)
> +#define SUNXI_SRAMCTRL_D_FLAG_USBOTG	BIT(0)

I am regularly not convinced that adding longish single-use macro names
for simple bit flips is really easier to read, because you always have to
lookup the definition first. If you decide to stick with it anyway, please
lose the parentheses around the 0x04, and use OFS_ instead of OFF_. Or
better use the name from the manual: SRAM_CTRL_REG1.

> +
> +static void sunxi_musb_claim_sram(void)
>  {
> -	u32 reg_value;
> +	/* TODO: Do not use hardcoded SUNXI_SRAMC_BASE; find the syscon base by

I think we should use "non-net" commenting style, with the "/*" on a line
on its own.

> +	 * traversing this OTG device's `allwinner,sram` FDT property and
> +	 * working upward to the system controller. */
>  
> -	/* 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);
> +	setbits_le32(SUNXI_SRAMC_BASE + OFF_SUNXI_SRAMCTRL_D,
> +		     SUNXI_SRAMCTRL_D_FLAG_USBOTG);

So this is the harder to understand part, IMO. If you stick something like
"Bit 0 in SRAM_CTRL_REG (offset 0x4) controls the SRAM D ownership." in the
comment above, then a simple:
	setbits_le32(SUNXI_SRAMC_BASE + 0x04, BIT(0));
becomes much easier to parse and relate to the manual, at least to my eyes.

>  }
>  
>  /******************************************************************************
> @@ -315,7 +323,11 @@ 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

Same commenting style issue like above.

Otherwise thanks for clearing this up, also for the future readers!

Cheers,
Andre

> +		 * 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);