| Message ID | 20230602214958.167909-2-CFSworks@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Andre Przywara |
| Headers | show |
| Series | Allwinner sunxi USB gadget improvements | expand |
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)
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); > + } > } > > /******************************************************************************
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
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
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
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); + } } /******************************************************************************
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(-)