[PATCH 0/3] Allwinner sunxi USB gadget improvements

[PATCH 0/3] Allwinner sunxi USB gadget improvements

[Sam Edwards]

Hi list,

This patchset improves the musb-new variant used in sunxi for DM support and
contains a couple of small fixes. Nothing too exciting here.

A note for Andre (as I suspect you'll be the one reviewing this):
Could you update your R528/T113s series with:
cpu_sunxi_ncat2.h:
+#define SUNXI_SRAMC_BASE		0
Kconfig:
+	select PHY_SUN4I_USB
...as appropriate? Those are the only changes necessary to get this USB gadget
driver working over there. :)

Many thanks,
Sam

Sam Edwards (3):
  usb: musb-new: sunxi: do not attempt to access NULL SRAMC
  usb: musb-new: sunxi: fix error check
  usb: musb-new: sunxi: make compatible with UDC/DM gadget model

 drivers/usb/musb-new/sunxi.c | 76 ++++++++++++++++++++++--------------
 1 file changed, 47 insertions(+), 29 deletions(-)

-- 
2.39.2

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

[Sam Edwards]

I believe that some sunxis (ncat2?) lack a SRAMC block,
as accessing this region results in a data abort. 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);
+	}
 }
 
 /******************************************************************************
-- 
2.39.2

[PATCH 2/3] usb: musb-new: sunxi: fix error check

[Sam Edwards]

The `musb_register` function returns some ERR_PTR(...) on failure,
not NULL, so update the check here appropriately.

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
---
 drivers/usb/musb-new/sunxi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c
index 6e60dd47e0..65a528e229 100644
--- a/drivers/usb/musb-new/sunxi.c
+++ b/drivers/usb/musb-new/sunxi.c
@@ -488,7 +488,7 @@ static int musb_usb_probe(struct udevice *dev)
 #else
 	pdata.mode = MUSB_PERIPHERAL;
 	host->host = musb_register(&pdata, &glue->dev, base);
-	if (!host->host)
+	if (IS_ERR_OR_NULL(host->host))
 		return -EIO;
 
 	printf("Allwinner mUSB OTG (Peripheral)\n");
-- 
2.39.2

[PATCH 3/3] usb: musb-new: sunxi: make compatible with UDC/DM gadget model

[Sam Edwards]

Since many sunxi boards do not implement a `board_usb_init`, it's
better if we just make the sunxi USB driver compatible with the
DM gadget model, as many other musb-new variants already are.

This change has been verified working on a T113s.

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
---
 drivers/usb/musb-new/sunxi.c | 60 +++++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 22 deletions(-)

diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c
index 65a528e229..ac6d1a41bb 100644
--- a/drivers/usb/musb-new/sunxi.c
+++ b/drivers/usb/musb-new/sunxi.c
@@ -432,6 +432,16 @@ static struct musb_hdrc_config musb_config_h3 = {
 	.ram_bits	= SUNXI_MUSB_RAM_BITS,
 };
 
+#if CONFIG_IS_ENABLED(DM_USB_GADGET)
+int dm_usb_gadget_handle_interrupts(struct udevice *dev) {
+	struct sunxi_glue *glue = dev_get_priv(dev);
+	struct musb_host_data *host = &glue->mdata;
+
+	host->host->isr(0, host->host);
+	return 0;
+}
+#endif
+
 static int musb_usb_probe(struct udevice *dev)
 {
 	struct sunxi_glue *glue = dev_get_priv(dev);
@@ -440,10 +450,6 @@ static int musb_usb_probe(struct udevice *dev)
 	void *base = dev_read_addr_ptr(dev);
 	int ret;
 
-#ifdef CONFIG_USB_MUSB_HOST
-	struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
-#endif
-
 	if (!base)
 		return -EINVAL;
 
@@ -474,25 +480,35 @@ static int musb_usb_probe(struct udevice *dev)
 	pdata.platform_ops = &sunxi_musb_ops;
 	pdata.config = glue->cfg->config;
 
-#ifdef CONFIG_USB_MUSB_HOST
-	priv->desc_before_addr = true;
-
-	pdata.mode = MUSB_HOST;
-	host->host = musb_init_controller(&pdata, &glue->dev, base);
-	if (!host->host)
-		return -EIO;
-
-	ret = musb_lowlevel_init(host);
-	if (!ret)
-		printf("Allwinner mUSB OTG (Host)\n");
-#else
-	pdata.mode = MUSB_PERIPHERAL;
-	host->host = musb_register(&pdata, &glue->dev, base);
-	if (IS_ERR_OR_NULL(host->host))
-		return -EIO;
+	if (IS_ENABLED(CONFIG_USB_MUSB_HOST)) {
+		struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
+		priv->desc_before_addr = true;
+
+		pdata.mode = MUSB_HOST;
+		host->host = musb_init_controller(&pdata, &glue->dev, base);
+		if (!host->host)
+			return -EIO;
+
+		ret = musb_lowlevel_init(host);
+		if (!ret)
+			printf("Allwinner mUSB OTG (Host)\n");
+	} else if (CONFIG_IS_ENABLED(DM_USB_GADGET)) {
+		pdata.mode = MUSB_PERIPHERAL;
+		host->host = musb_init_controller(&pdata, &glue->dev, base);
+		if (!host->host)
+			return -EIO;
+
+		ret = usb_add_gadget_udc(&glue->dev, &host->host->g);
+		if (!ret)
+			printf("Allwinner mUSB OTG (Peripheral)\n");
+	} else {
+		pdata.mode = MUSB_PERIPHERAL;
+		host->host = musb_register(&pdata, &glue->dev, base);
+		if (IS_ERR_OR_NULL(host->host))
+			return -EIO;
 
-	printf("Allwinner mUSB OTG (Peripheral)\n");
-#endif
+		printf("Allwinner mUSB OTG (Peripheral)\n");
+	}
 
 	return ret;
 }
-- 
2.39.2

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

[Marek Vasut]

On 6/2/23 23:49, Sam Edwards wrote:
> I believe that some sunxis (ncat2?) lack a SRAMC block,
> as accessing this region results in a data abort. 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>

Could it be that the SRAMC clock need to be enabled instead ?
(note that I don't know anything about sunxi stuff)

Re: [PATCH 2/3] usb: musb-new: sunxi: fix error check

[Marek Vasut]

On 6/2/23 23:49, Sam Edwards wrote:
> The `musb_register` function returns some ERR_PTR(...) on failure,
> not NULL, so update the check here appropriately.
> 
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> ---
>   drivers/usb/musb-new/sunxi.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c
> index 6e60dd47e0..65a528e229 100644
> --- a/drivers/usb/musb-new/sunxi.c
> +++ b/drivers/usb/musb-new/sunxi.c
> @@ -488,7 +488,7 @@ static int musb_usb_probe(struct udevice *dev)
>   #else
>   	pdata.mode = MUSB_PERIPHERAL;
>   	host->host = musb_register(&pdata, &glue->dev, base);
> -	if (!host->host)
> +	if (IS_ERR_OR_NULL(host->host))
>   		return -EIO;
>   
>   	printf("Allwinner mUSB OTG (Peripheral)\n");

Reviewed-by: Marek Vasut <marex@denx.de>

Can you please send this one separately, so I can pick it for current 
release ?

Re: [PATCH 3/3] usb: musb-new: sunxi: make compatible with UDC/DM gadget model

[Marek Vasut]

On 6/2/23 23:49, Sam Edwards wrote:
> Since many sunxi boards do not implement a `board_usb_init`, it's
> better if we just make the sunxi USB driver compatible with the
> DM gadget model, as many other musb-new variants already are.
> 
> This change has been verified working on a T113s.
> 
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> ---
>   drivers/usb/musb-new/sunxi.c | 60 +++++++++++++++++++++++-------------
>   1 file changed, 38 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c
> index 65a528e229..ac6d1a41bb 100644
> --- a/drivers/usb/musb-new/sunxi.c
> +++ b/drivers/usb/musb-new/sunxi.c
> @@ -432,6 +432,16 @@ static struct musb_hdrc_config musb_config_h3 = {
>   	.ram_bits	= SUNXI_MUSB_RAM_BITS,
>   };
>   
> +#if CONFIG_IS_ENABLED(DM_USB_GADGET)
> +int dm_usb_gadget_handle_interrupts(struct udevice *dev) {
> +	struct sunxi_glue *glue = dev_get_priv(dev);
> +	struct musb_host_data *host = &glue->mdata;
> +
> +	host->host->isr(0, host->host);
> +	return 0;
> +}
> +#endif
> +
>   static int musb_usb_probe(struct udevice *dev)
>   {
>   	struct sunxi_glue *glue = dev_get_priv(dev);
> @@ -440,10 +450,6 @@ static int musb_usb_probe(struct udevice *dev)
>   	void *base = dev_read_addr_ptr(dev);
>   	int ret;
>   
> -#ifdef CONFIG_USB_MUSB_HOST
> -	struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
> -#endif
> -
>   	if (!base)
>   		return -EINVAL;
>   
> @@ -474,25 +480,35 @@ static int musb_usb_probe(struct udevice *dev)
>   	pdata.platform_ops = &sunxi_musb_ops;
>   	pdata.config = glue->cfg->config;
>   
> -#ifdef CONFIG_USB_MUSB_HOST
> -	priv->desc_before_addr = true;
> -
> -	pdata.mode = MUSB_HOST;
> -	host->host = musb_init_controller(&pdata, &glue->dev, base);
> -	if (!host->host)
> -		return -EIO;
> -
> -	ret = musb_lowlevel_init(host);
> -	if (!ret)
> -		printf("Allwinner mUSB OTG (Host)\n");
> -#else
> -	pdata.mode = MUSB_PERIPHERAL;
> -	host->host = musb_register(&pdata, &glue->dev, base);
> -	if (IS_ERR_OR_NULL(host->host))
> -		return -EIO;
> +	if (IS_ENABLED(CONFIG_USB_MUSB_HOST)) {
> +		struct usb_bus_priv *priv = dev_get_uclass_priv(dev);
> +		priv->desc_before_addr = true;
> +
> +		pdata.mode = MUSB_HOST;
> +		host->host = musb_init_controller(&pdata, &glue->dev, base);
> +		if (!host->host)
> +			return -EIO;
> +
> +		ret = musb_lowlevel_init(host);
> +		if (!ret)
> +			printf("Allwinner mUSB OTG (Host)\n");

Please just drop this print for DM (or in general).

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

[Andre Przywara]

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

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

[Sam Edwards]

Howdy Andre,

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

I did understand SRAMC to mean "SRAM controller," but what a funny 
coincidence that NCAT2 does away with SRAM 'C' at around the same time I 
sent in this patch! I did not know it's now the "syscon," I just deduced 
that it wasn't being used for anything important when I couldn't find 
any relevant code relying on it.

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

I spent longer than I thought I would looking into this! :)

Adding a `has_sram` field to the match table data was easy enough, but 
dynamic discovery of the syscon is, for sure, more complicated. The 
biggest problem is that the data model for representing these bits seems 
overengineered for what it is, and most of the logic is just for 
identifying *which bit* needs to be set. Linux's logic for the "syscon 
base" part of it is just: the first allwinner,*-system-control device to 
probe, registers itself globally -- that's it. Surely we could keep the 
"set the 1 bit" part of it hardcoded and just do the same thing (global 
registration) for the syscon, no need to chase the allwinner,sram 
phandle? That should suffice if the goal is to remove the 
SUNXI_SRAMC_BASE define, no?

(By the way, apparently this facility in the SYSCON+0x4 register is only 
1 bit wide -- not 2 as U-Boot believes. It also seems to be for 
switching ownership of SRAM block 'D' between the USB controller and 
CPU, and if so the "config usb fifo, 8kb mode" comment and 
USBC_ConfigFIFO_Base function name are both wrong. I am only judging by 
the Linux implementation of this logic, though.)

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

I'll do that latter part locally (can you take care of it in your 
series?) and send in a patch for the `has_sram` change that also 
clarifies the purpose of the syscon poke. The SUNXI_SRAMC_BASE removal I 
just now mentioned could be interesting, but not something I want to 
hold up NCAT2 support on.

Best regards,
Sam

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

[Andre Przywara]

On Tue, 6 Jun 2023 23:39:24 -0600
Sam Edwards <cfsworks@gmail.com> wrote:

Hi Sam,

> On 6/5/23 05:04, Andre Przywara wrote:
> > 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.  
> 
> I did understand SRAMC to mean "SRAM controller," but what a funny 

Yeah, I figured you would know, but just wanted to make this clear, also
for the benefit of others, because it confused me in the past.

> coincidence that NCAT2 does away with SRAM 'C' at around the same time I 
> sent in this patch! I did not know it's now the "syscon," I just deduced 
> that it wasn't being used for anything important when I couldn't find 
> any relevant code relying on it.

"syscon" really just means "a bunch of gates where AW didn't know where
else to put them". In modern SoCs it prominently contains the version
register and some EMAC control bits, partly, but not entirely, related
to internal PHYs. And it's already the A10 manual that uses the term
"system controller", maybe "SRAMC" comes from the original BSP source code.
Definitely it somewhat evolved, because originally it was really just the
SRAM control bits in there, but now has other functionality, and might not
even control the SRAM muxing anymore.

> > 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.  
> 
> I spent longer than I thought I would looking into this! :)
> 
> Adding a `has_sram` field to the match table data was easy enough, but 
> dynamic discovery of the syscon is, for sure, more complicated. The 
> biggest problem is that the data model for representing these bits seems 
> overengineered for what it is, and most of the logic is just for 
> identifying *which bit* needs to be set.

I understand that it seems like boiling the ocean for just flipping a
single bit somewhere, but at least for Linux there are good reasons to do
it that way, see below. And to be honest, those problems stem more from
AW's somewhat problematic design to use a generic "syscon" block, for
*multiple* different things, in the first place.

> Linux's logic for the "syscon 
> base" part of it is just: the first allwinner,*-system-control device to 
> probe, registers itself globally -- that's it.

Well, we indeed only seem to support one instance of syscon, but this is
somewhat backed by its idea of some "catch-all" register collection.
What's important for Linux is that only one device can claim a certain
MMIO region. When other devices want to access even a single bit in there,
they need to somehow talk to that other device. In Linux this is modelled
via phandles and regmaps, and works somewhat nicely, if at the cost of
having the boilerplate for a whole extra device.
Now porting this over in its entirety to U-Boot is possible, but somewhat
over-the-top for a bootloader, I believe. We have shortcuts, though, see
sun8i_emac.c:sun8i_emac_eth_of_to_plat().

> Surely we could keep the
> "set the 1 bit" part of it hardcoded and just do the same thing (global 
> registration) for the syscon, no need to chase the allwinner,sram 
> phandle? That should suffice if the goal is to remove the 
> SUNXI_SRAMC_BASE define, no?

I think for that we could follow the sun8i-emac route: follow the phandle,
and pick the "reg" property from the DTB. No need for an extra driver.

> (By the way, apparently this facility in the SYSCON+0x4 register is only 
> 1 bit wide -- not 2 as U-Boot believes. It also seems to be for 
> switching ownership of SRAM block 'D' between the USB controller and 
> CPU, and if so the "config usb fifo, 8kb mode" comment and 
> USBC_ConfigFIFO_Base function name are both wrong. I am only judging by 
> the Linux implementation of this logic, though.)

Yeah, it seems this way, though nobody knows for sure ;-) I believe this
is code that came from the original Allwinner BSP, which tends to do,
well, weird stuff at times ;-)

> > 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.  
> 
> I'll do that latter part locally (can you take care of it in your 
> series?) and send in a patch for the `has_sram` change that also 
> clarifies the purpose of the syscon poke. The SUNXI_SRAMC_BASE removal I 
> just now mentioned could be interesting, but not something I want to 
> hold up NCAT2 support on.

Yeah, so there are three steps, in increasing order of complexity:
1) Change SUNXI_SRAMC_BASE to 0x3000000. That's done already in my tree.
2) Introduce some has_sram property in U-Boot's MUSB driver and only poke
the SRAM D bit for those SoCs that need it.
3) Follow the allwinner,sram phandle in the MUSB driver and fish out the
syscon base address from the DTB, to avoid hardcoding it, at least for the
MUSB driver. We need it elsewhere, to access the version register and
print the SoC name.

Can you confirm that just changing the SUNXI_SRAMC_BASE to 0x3000000 avoids
the crash you saw, and removes the immediate need for this very patch here?

And if I get this right, you already have 2) implemented? I would love to
see it on the list, then.

3) doesn't really have priority, as we need SUNXI_SRAMC_BASE in cpu_info.c
anyway. But looking at the sun8i_emac.c shortcut, it might not be too hard
to do either.

Thanks,
Andre

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

[Sam Edwards]

Hey Andre,

On 6/7/23 04:45, Andre Przywara wrote:
> "syscon" really just means "a bunch of gates where AW didn't know where
> else to put them".

The good ol' "kitchen sink" register block, eh? Its lack of clear, 
definite purpose is even reflected in the name, because when you think 
about it, isn't *every* MMIO register for "system control"? :)

> Yeah, it seems this way, though nobody knows for sure ;-) I believe this
> is code that came from the original Allwinner BSP, which tends to do,
> well, weird stuff at times ;-)

I've gone ahead and submitted a patch for rewording this in the driver 
source anyway. In the commit message, I've included a way we could 
verify this experimentally, though I don't have access to an early 
Allwinner SoC with which to try it.

> Yeah, so there are three steps, in increasing order of complexity:
> 1) Change SUNXI_SRAMC_BASE to 0x3000000. That's done already in my tree.

I've done it over here as well.

> 2) Introduce some has_sram property in U-Boot's MUSB driver and only poke
> the SRAM D bit for those SoCs that need it.

Done, a patch for that has been sent in.

> 3) Follow the allwinner,sram phandle in the MUSB driver and fish out the
> syscon base address from the DTB, to avoid hardcoding it, at least for the
> MUSB driver. We need it elsewhere, to access the version register and
> print the SoC name.

One of the patches I just sent in also adds a "TODO" comment to do this. 
I also kept the function separate specifically so someone coming along 
later wanting to add this has plenty of breathing room to do so.

> Can you confirm that just changing the SUNXI_SRAMC_BASE to 0x3000000 avoids
> the crash you saw, and removes the immediate need for this very patch here?

Yes: I reverted enough changes to make the crash reappear, and then 
changed the base register. Updating SUNXI_SRAM_BASE to 0x03000000 
resolves the crash with no side effects: it seems (BIT(0) of) 0x03000004 
is RAZ/WI.

> And if I get this right, you already have 2) implemented? I would love to
> see it on the list, then.

See Patchwork series #358672.

Thanks for your continued efforts,
Sam