[PATCH v2 0/2] sunxi, usb: Clean up SRAM initialization code
Hi list,
This is the second version of cleaning up the SRAM initialization code in the musb-new variant for sunxis.
Patch 1 ("only perform ... when necessary") has not changed since v1. Patch 2 has had the following changes: - Remove one-off bit/reg #defines, per feedback from Andre. - Switch to the Linux multiline comment style, per feedback from Andre. - Reword the commit message, since this change isn't as "speculative" as I had originally thought. - Perhaps controversially, introduce a `void *syscon_base;` under that TODO comment I mentioned before, to further invite somebody to "DO" it. :)
Thank you once again for your continued efforts, Sam
Sam Edwards (2): usb: musb-new: sunxi: only perform SRAM initialization when necessary usb: musb-new: sunxi: clarify the purpose of SRAM initialization
drivers/usb/musb-new/sunxi.c | 43 ++++++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 9 deletions(-)
Only the older (ca. A10, A20) sunxis need this poke for the MUSB to function. Mimic the Linux kernel and add a `has_sram` flag to the config structure that is only set for the specific compatibles that require this initialization.
Signed-off-by: Sam Edwards CFSworks@gmail.com Reviewed-by: Andre Przywara andre.przywara@arm.com Tested-by: Andre Przywara andre.przywara@arm.com --- drivers/usb/musb-new/sunxi.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c index c5c63249aa..1111a67eaf 100644 --- a/drivers/usb/musb-new/sunxi.c +++ b/drivers/usb/musb-new/sunxi.c @@ -83,6 +83,7 @@
struct sunxi_musb_config { struct musb_hdrc_config *config; + bool has_sram; };
struct sunxi_glue { @@ -311,7 +312,10 @@ static int sunxi_musb_init(struct musb *musb)
musb->isr = sunxi_musb_interrupt;
- USBC_ConfigFIFO_Base(); + if (glue->cfg->has_sram) { + USBC_ConfigFIFO_Base(); + } + USBC_EnableDpDmPullUp(musb->mregs); USBC_EnableIdPullUp(musb->mregs);
@@ -517,6 +521,7 @@ static int musb_usb_remove(struct udevice *dev)
static const struct sunxi_musb_config sun4i_a10_cfg = { .config = &musb_config, + .has_sram = true, };
static const struct sunxi_musb_config sun6i_a31_cfg = {
This is largely a cosmetic change, with one functional distinction: We are now only setting BIT(0), and no longer clearing BIT(1).
The A20 manual confirms the purpose and bitwidth of this field, and we have also been doing it this way for a while in Linux-land: The prior narrative about this initialization being about configuring a FIFO has pretty much been debunked for years now.
This cleanup also adds a TODO comment about runtime discovery of the SYSCON base, per discussion with Andre.
Signed-off-by: Sam Edwards CFSworks@gmail.com Cc: Andre Przywara andre.przywara@arm.com --- drivers/usb/musb-new/sunxi.c | 38 +++++++++++++++++++++++++++--------- 1 file changed, 29 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c index 1111a67eaf..be7faaa11e 100644 --- a/drivers/usb/musb-new/sunxi.c +++ b/drivers/usb/musb-new/sunxi.c @@ -171,15 +171,29 @@ static void USBC_ForceVbusValidToHigh(__iomem void *base) musb_writel(base, USBC_REG_o_ISCR, reg_val); }
-static void USBC_ConfigFIFO_Base(void) -{ - u32 reg_value; +/****************************************************************************** + * Non-USBC register access needed for initialization + ******************************************************************************/
- /* 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); +/* + * A10(s), A13, GR8, A20: + * switch ownership of SRAM block 'D' to the USB-OTG controller + */ +static void sunxi_musb_claim_sram(void) +{ + /* + * TODO: Do not use hardcoded SUNXI_SRAMC_BASE; find the syscon base by + * traversing this OTG device's `allwinner,sram` FDT property and + * working upward to the system controller. + */ + void *syscon_base = (void *)SUNXI_SRAMC_BASE; + + /* + * BIT(0) of SRAM_CTRL_REG1 (syscon+0x04) controls SRAM-D ownership: + * '0' -> exclusive access by CPU + * '1' -> exclusive access by USB0 + */ + setbits_le32(syscon_base + 0x04, BIT(0)); }
/****************************************************************************** @@ -313,7 +327,13 @@ static int sunxi_musb_init(struct musb *musb) musb->isr = sunxi_musb_interrupt;
if (glue->cfg->has_sram) { - USBC_ConfigFIFO_Base(); + /* + * This is an older USB-OTG controller that Allwinner did not + * endow with a dedicated SRAM block; it instead uses SRAM + * block 'D', ownership of which needs to be handed over by + * the CPU + */ + sunxi_musb_claim_sram(); }
USBC_EnableDpDmPullUp(musb->mregs);
On Fri, 9 Jun 2023 15:37:16 -0600 Sam Edwards cfsworks@gmail.com wrote:
Hi,
This is largely a cosmetic change, with one functional distinction: We are now only setting BIT(0), and no longer clearing BIT(1).
The A20 manual confirms the purpose and bitwidth of this field, and we have also been doing it this way for a while in Linux-land: The prior narrative about this initialization being about configuring a FIFO has pretty much been debunked for years now.
This cleanup also adds a TODO comment about runtime discovery of the SYSCON base, per discussion with Andre.
Signed-off-by: Sam Edwards CFSworks@gmail.com Cc: Andre Przywara andre.przywara@arm.com
drivers/usb/musb-new/sunxi.c | 38 +++++++++++++++++++++++++++--------- 1 file changed, 29 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/musb-new/sunxi.c b/drivers/usb/musb-new/sunxi.c index 1111a67eaf..be7faaa11e 100644 --- a/drivers/usb/musb-new/sunxi.c +++ b/drivers/usb/musb-new/sunxi.c @@ -171,15 +171,29 @@ static void USBC_ForceVbusValidToHigh(__iomem void *base) musb_writel(base, USBC_REG_o_ISCR, reg_val); }
-static void USBC_ConfigFIFO_Base(void) -{
- u32 reg_value;
+/******************************************************************************
- Non-USBC register access needed for initialization
- ******************************************************************************/
- /* 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);
+/*
- A10(s), A13, GR8, A20:
- switch ownership of SRAM block 'D' to the USB-OTG controller
- */
+static void sunxi_musb_claim_sram(void) +{
- /*
* TODO: Do not use hardcoded SUNXI_SRAMC_BASE; find the syscon base by* traversing this OTG device's `allwinner,sram` FDT property and* working upward to the system controller.*/- void *syscon_base = (void *)SUNXI_SRAMC_BASE;
Can you pass this in as a parameter, then call the function with SUNXI_SRAMC_BASE, for now? Because syscon_base will become a member of struct sunxi_glue, which we have readily accessible in sunxi_musb_init(). And no need for a comment, I think I will come up with something soon enough.
Many thanks! Andre
- /*
* BIT(0) of SRAM_CTRL_REG1 (syscon+0x04) controls SRAM-D ownership:* '0' -> exclusive access by CPU* '1' -> exclusive access by USB0*/- setbits_le32(syscon_base + 0x04, BIT(0));
}
/****************************************************************************** @@ -313,7 +327,13 @@ static int sunxi_musb_init(struct musb *musb) musb->isr = sunxi_musb_interrupt;
if (glue->cfg->has_sram) {
USBC_ConfigFIFO_Base();
/** This is an older USB-OTG controller that Allwinner did not* endow with a dedicated SRAM block; it instead uses SRAM* block 'D', ownership of which needs to be handed over by* the CPU*/sunxi_musb_claim_sram();}
USBC_EnableDpDmPullUp(musb->mregs);
participants (2)
-
Andre Przywara -
Sam Edwards