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
