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