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

[Sam Edwards <cfsworks@gmail.com>]

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