[PATCH v3 0/2] sunxi, usb: Clean up SRAM initialization code

[PATCH v3 0/2] sunxi, usb: Clean up SRAM initialization code

[Sam Edwards]

Hello again,

The only change from v2 is that `syscon_base` is now a param of
`sunxi_musb_claim_sram`, and the TODO comment has been removed.

Cheers,
Sam

Sam Edwards (2):
  usb: musb-new: sunxi: only perform SRAM initialization when necessary
  usb: musb-new: sunxi: clarify the purpose of SRAM initialization

 drivers/usb/musb-new/sunxi.c | 36 +++++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)

-- 
2.39.2

[PATCH v3 1/2] usb: musb-new: sunxi: only perform SRAM initialization when necessary

[Sam Edwards]

Only the older (ca. A10, A20) sunxis need this poke for the MUSB to
function. Mimic the Linux kernel and add a `has_sram` flag to the config
structure that is only set for the specific compatibles that require
this initialization.

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Tested-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/usb/musb-new/sunxi.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c
index c5c63249aa..1111a67eaf 100644
--- a/drivers/usb/musb-new/sunxi.c
+++ b/drivers/usb/musb-new/sunxi.c
@@ -83,6 +83,7 @@
 
 struct sunxi_musb_config {
 	struct musb_hdrc_config *config;
+	bool has_sram;
 };
 
 struct sunxi_glue {
@@ -311,7 +312,10 @@ static int sunxi_musb_init(struct musb *musb)
 
 	musb->isr = sunxi_musb_interrupt;
 
-	USBC_ConfigFIFO_Base();
+	if (glue->cfg->has_sram) {
+		USBC_ConfigFIFO_Base();
+	}
+
 	USBC_EnableDpDmPullUp(musb->mregs);
 	USBC_EnableIdPullUp(musb->mregs);
 
@@ -517,6 +521,7 @@ static int musb_usb_remove(struct udevice *dev)
 
 static const struct sunxi_musb_config sun4i_a10_cfg = {
 	.config = &musb_config,
+	.has_sram = true,
 };
 
 static const struct sunxi_musb_config sun6i_a31_cfg = {
-- 
2.39.2

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

[Sam Edwards]

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 | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c
index 1111a67eaf..a8b1a8f870 100644
--- a/drivers/usb/musb-new/sunxi.c
+++ b/drivers/usb/musb-new/sunxi.c
@@ -171,15 +171,22 @@ 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(uintptr_t syscon_base)
+{
+	/*
+	 * 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 +320,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(SUNXI_SRAMC_BASE);
 	}
 
 	USBC_EnableDpDmPullUp(musb->mregs);
-- 
2.39.2

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

[Andre Przywara]

On Mon, 12 Jun 2023 14:03:02 -0600
Sam Edwards <cfsworks@gmail.com> wrote:

> 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>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Thanks for the changes!

Cheers,
Andre

> Cc: Andre Przywara <andre.przywara@arm.com>
> ---
>  drivers/usb/musb-new/sunxi.c | 31 ++++++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c
> index 1111a67eaf..a8b1a8f870 100644
> --- a/drivers/usb/musb-new/sunxi.c
> +++ b/drivers/usb/musb-new/sunxi.c
> @@ -171,15 +171,22 @@ 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(uintptr_t syscon_base)
> +{
> +	/*
> +	 * 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 +320,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(SUNXI_SRAMC_BASE);
>  	}
>  
>  	USBC_EnableDpDmPullUp(musb->mregs);