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

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

[Sam Edwards]

Hi list,

This pair of patches is a byproduct of discussion on my earlier patchset
(Patchwork series #357953), which had simply disabled this initialization where
SUNXI_SRAMC_BASE was set to null. It is true that this initialization is not
needed on newer devices, but not because the SRAMC (now called "SYSCON") has
been removed, so a null SRAMC base was not the appropriate fix.

Of the two patches here, only the first is an actual fix; the second is a
mostly-cosmetic cleanup, which also adds a TODO to make this function
independent of SUNXI_SRAMC_BASE altogether, per the aforementioned prior
discussion.

I do not have an A10, A10s, A13, GR8, or A20 against which to test this change;
a reviewer with access to one of these should verify that USB-OTG functionality
is still working. That ought to be a sufficient regression test. :)

A note to committers: If patch 1 is acceptable but patch 2 is not, go ahead and
commit only patch 1 (unless the U-Boot project specifically avoids doing this).

Thanks for your time,
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 | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

-- 
2.39.2

[PATCH 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>
---
 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 ab55d68620..c05c0d5561 100644
--- a/drivers/usb/musb-new/sunxi.c
+++ b/drivers/usb/musb-new/sunxi.c
@@ -85,6 +85,7 @@
 
 struct sunxi_musb_config {
 	struct musb_hdrc_config *config;
+	bool has_sram;
 };
 
 struct sunxi_glue {
@@ -313,7 +314,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);
 
@@ -525,6 +529,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

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

[Andre Przywara]

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

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

The patch looks alright in general, thanks for sending this! I will try to
test it on an older SoC in the next days. As an added bonus, that should
actually help the F1C100s USB(-OTG) support.

Cheers,
Andre

> Signed-off-by: Sam Edwards <CFSworks@gmail.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 ab55d68620..c05c0d5561 100644
> --- a/drivers/usb/musb-new/sunxi.c
> +++ b/drivers/usb/musb-new/sunxi.c
> @@ -85,6 +85,7 @@
>  
>  struct sunxi_musb_config {
>  	struct musb_hdrc_config *config;
> +	bool has_sram;
>  };
>  
>  struct sunxi_glue {
> @@ -313,7 +314,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);
>  
> @@ -525,6 +529,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 = {

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

[Andre Przywara]

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

Hi,

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

So I grabbed a BananaPi (A20) and played with it a little. Gadgets still
work with this patch, also I can confirm that this bit is necessary
(turned it off with mw.l, and the gadget stopped working), and also
that the bit flip works (set "has_sram = false;" and it didn't work anymore).

Also tested on an OrangePi Zero (H3), which doesn't need the SRAM switch.
It worked with both the bit set and cleared, also before and after the
patch, so it's all fine.

> Signed-off-by: Sam Edwards <CFSworks@gmail.com>

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

Thanks,
Andre

> ---
>  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 ab55d68620..c05c0d5561 100644
> --- a/drivers/usb/musb-new/sunxi.c
> +++ b/drivers/usb/musb-new/sunxi.c
> @@ -85,6 +85,7 @@
>  
>  struct sunxi_musb_config {
>  	struct musb_hdrc_config *config;
> +	bool has_sram;
>  };
>  
>  struct sunxi_glue {
> @@ -313,7 +314,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);
>  
> @@ -525,6 +529,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 = {

[PATCH 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 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.

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
Cc: Andre Przywara <andre.przywara@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)
+
+static void sunxi_musb_claim_sram(void)
 {
-	u32 reg_value;
+	/* 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. */
 
-	/* 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);
 }
 
 /******************************************************************************
@@ -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
+		 * 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);
-- 
2.39.2

Re: [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@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@gmail.com>
> Cc: Andre Przywara <andre.przywara@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);

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

[Sam Edwards]

Hi Andre,

I've applied most of this feedback (most of which comes as a relief; I 
dislike inventing names for mystery bits) in preparation to send a v2, 
but had two questions:

On 6/9/23 04:13, Andre Przywara wrote:
>> 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.

I take it that this (in combination with your review on 1/2) means you 
concur with my speculated purpose of the mystery bit. If so, I'd like to 
rephrase the above paragraph in the commit message to:

"""
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. This also reflects what's been observed on actual
hardware: SRAM-D is inaccessible by the CPU when the bit is set, and
the MUSB unit crashes when this bit is cleared while USB is in use,
leaving behind debris in SRAM-D from its use as a "scratch space."
"""

Does this accurately reflect what you've seen, particularly (especially) 
that last line, or should I end the commentary at "is in use."?

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

This seems to be an obscure term of art, and searching "non-net comment" 
and "non-net style" on Google isn't finding me any style guide or set of 
rules to check against. (Amusingly: if I search for the "net" style, I 
get a lot of .NET suggestions.)

The main thing I'm trying to figure out is if it also demands */ on its 
own line, which I would intuitively think makes sense (it does look 
better to me that way), but I'm unsure if your lack of critique at the 
closing side of my multiline comments means you would prefer:
/*
  * A block of text that's long enough to become a
  * multiline comment and ends up looking like this */

Thanks for your quick and thorough feedback,
Sam

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

[Andre Przywara]

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

Hi Sam,

> I've applied most of this feedback (most of which comes as a relief; I 
> dislike inventing names for mystery bits) in preparation to send a v2, 
> but had two questions:
> 
> On 6/9/23 04:13, Andre Przywara wrote:
> >> 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.  
> 
> I take it that this (in combination with your review on 1/2) means you 
> concur with my speculated purpose of the mystery bit. If so, I'd like to 

Oh, but why mystery bit? The A20 manual [1] spells that out:
Offset: 0x4 | SRAM_CTRL_REG1
...
Bit 0: R/W, default 0x0: SRAMD_MAP: SRAM D Area Config:
			 0: map to CPU/DMA
			 1: map to USB0

So I am afraid you need to rephrase the commit message again ;-)

> rephrase the above paragraph in the commit message to:
> 
> """
> 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. This also reflects what's been observed on actual
> hardware: SRAM-D is inaccessible by the CPU when the bit is set, and
> the MUSB unit crashes when this bit is cleared while USB is in use,
> leaving behind debris in SRAM-D from its use as a "scratch space."
> """
> 
> Does this accurately reflect what you've seen, particularly (especially) 
> that last line, or should I end the commentary at "is in use."?

We mostly gave up on finding 100% proof for everything, especially in
U-Boot there is more pragmatic approach (check the DRAM controller init
code): it if works and the BSP code does it like this, it's good to go.

I confirmed that setting this bit before calling "ums" makes the
difference between the gadget appearing on the other end or not. I did
not turn it off while the gadget was in literal use - which is tricky
to pull off anyway, since you don't have a prompt while a gadget is
running.

So while it's honourable to make sure that this is correct, there is no
real need to provide mathematical proof for those bits, especially if
they have been working over years now ;-) 

> > I think we should use "non-net" commenting style, with the "/*" on a line
> > on its own.  
> 
> This seems to be an obscure term of art, and searching "non-net comment" 
> and "non-net style" on Google isn't finding me any style guide or set of 
> rules to check against. (Amusingly: if I search for the "net" style, I 
> get a lot of .NET suggestions.)

Ah, sorry, I somewhat made this term up, apparently in a non-Google
compatible way ;-) The Linux kernel recommends this "/* on a line of its
own" commenting style, but inside the net/ folder the rules are slightly
different:

https://www.kernel.org/doc/html/v4.10/process/coding-style.html#commenting

Hope that helps!

Cheers,
Andre


> The main thing I'm trying to figure out is if it also demands */ on its 
> own line, which I would intuitively think makes sense (it does look 
> better to me that way), but I'm unsure if your lack of critique at the 
> closing side of my multiline comments means you would prefer:
> /*
>   * A block of text that's long enough to become a
>   * multiline comment and ends up looking like this */
> 
> Thanks for your quick and thorough feedback,
> Sam