Hi Caleb,

Thank you for the patch.

On mer., janv. 31, 2024 at 14:57, Caleb Connolly caleb.connolly@linaro.org wrote:

The Qualcomm specific dwc3 wrapper isn't hugely complicated, implemented the missing initialisation for host and gadget mode.

Signed-off-by: Caleb Connolly caleb.connolly@linaro.org

drivers/usb/dwc3/dwc3-generic.c | 99 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 98 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c index 48da621ba966..1119cdecd26d 100644 --- a/drivers/usb/dwc3/dwc3-generic.c +++ b/drivers/usb/dwc3/dwc3-generic.c @@ -419,6 +419,99 @@ struct dwc3_glue_ops ti_ops = { .glue_configure = dwc3_ti_glue_configure, };

+/* USB QSCRATCH Hardware registers */ +#define QSCRATCH_HS_PHY_CTRL 0x10 +#define UTMI_OTG_VBUS_VALID BIT(20) +#define SW_SESSVLD_SEL BIT(28)

+#define QSCRATCH_SS_PHY_CTRL 0x30 +#define LANE0_PWR_PRESENT BIT(24)

+#define QSCRATCH_GENERAL_CFG 0x08 +#define PIPE_UTMI_CLK_SEL BIT(0) +#define PIPE3_PHYSTATUS_SW BIT(3) +#define PIPE_UTMI_CLK_DIS BIT(8)

+#define PWR_EVNT_IRQ_STAT_REG 0x58 +#define PWR_EVNT_LPM_IN_L2_MASK BIT(4) +#define PWR_EVNT_LPM_OUT_L2_MASK BIT(5)

+#define SDM845_QSCRATCH_BASE_OFFSET 0xf8800 +#define SDM845_QSCRATCH_SIZE 0x400 +#define SDM845_DWC3_CORE_SIZE 0xcd00 +static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val) +{

• u32 reg;
• reg = readl(base + offset);
• reg |= val;
• writel(reg, base + offset);

Why can't we use the setbits() macro here? see: arch/arm/include/asm/io.h

• /* ensure that above write is through */
• readl(base + offset);

+}

+static inline void dwc3_qcom_clrbits(void __iomem *base, u32 offset, u32 val) +{

• u32 reg;
• reg = readl(base + offset);
• reg &= ~val;
• writel(reg, base + offset);

Same question for clrbits()

• /* ensure that above write is through */
• readl(base + offset);

+}

+static void dwc3_qcom_vbus_override_enable(void __iomem *qscratch_base, bool enable) +{

• if (enable) {

• dwc3_qcom_setbits(qscratch_base, QSCRATCH_SS_PHY_CTRL,
  

• 		  LANE0_PWR_PRESENT);
  

• dwc3_qcom_setbits(qscratch_base, QSCRATCH_HS_PHY_CTRL,
  

• 		  UTMI_OTG_VBUS_VALID | SW_SESSVLD_SEL);
  

• } else {

• dwc3_qcom_clrbits(qscratch_base, QSCRATCH_SS_PHY_CTRL,
  

• 		  LANE0_PWR_PRESENT);
  

• dwc3_qcom_clrbits(qscratch_base, QSCRATCH_HS_PHY_CTRL,
  

• 		  UTMI_OTG_VBUS_VALID | SW_SESSVLD_SEL);
  

• }

+}

+/* For controllers running without superspeed PHYs */ +static void dwc3_qcom_select_utmi_clk(void __iomem *qscratch_base) +{

• /* Configure dwc3 to use UTMI clock as PIPE clock not present */
• dwc3_qcom_setbits(qscratch_base, QSCRATCH_GENERAL_CFG,

• 	  PIPE_UTMI_CLK_DIS);
  

• udelay(500);
• dwc3_qcom_setbits(qscratch_base, QSCRATCH_GENERAL_CFG,

• 	  PIPE_UTMI_CLK_SEL | PIPE3_PHYSTATUS_SW);
  

• udelay(500);
• dwc3_qcom_clrbits(qscratch_base, QSCRATCH_GENERAL_CFG,

• 	  PIPE_UTMI_CLK_DIS);
  

+}

+static void dwc3_qcom_glue_configure(struct udevice *dev, int index,

• 		     enum usb_dr_mode mode)
  

+{

• void __iomem *qscratch_base = (void __iomem *)dev_read_addr(dev);
• if (dev_read_bool(dev, "qcom,select-utmi-as-pipe-clk"))

• dwc3_qcom_select_utmi_clk(qscratch_base);
  

• if (mode != USB_DR_MODE_HOST)

• dwc3_qcom_vbus_override_enable(qscratch_base, true);
  

+}

+struct dwc3_glue_ops qcom_ops = {

• .glue_configure = dwc3_qcom_glue_configure,

+};

static int dwc3_rk_glue_get_ctrl_dev(struct udevice *dev, ofnode *node) { *node = dev_ofnode(dev); @@ -506,6 +599,10 @@ static int dwc3_glue_reset_init(struct udevice *dev, else if (ret) return ret;

• if (device_is_compatible(dev, "qcom,dwc3")) {

• reset_assert_bulk(&glue->resets);
  

Any reason for not doing error handling on reset_assert_bulk() like it's done below ?

• udelay(500);
  

• } ret = reset_deassert_bulk(&glue->resets); if (ret) { reset_release_bulk(&glue->resets);

@@ -623,7 +720,7 @@ static const struct udevice_id dwc3_glue_ids[] = { { .compatible = "rockchip,rk3399-dwc3" }, { .compatible = "rockchip,rk3568-dwc3", .data = (ulong)&rk_ops }, { .compatible = "rockchip,rk3588-dwc3", .data = (ulong)&rk_ops },

• { .compatible = "qcom,dwc3" },

• { .compatible = "qcom,dwc3", .data = (ulong)&qcom_ops }, { .compatible = "fsl,imx8mp-dwc3", .data = (ulong)&imx8mp_ops }, { .compatible = "fsl,imx8mq-dwc3" }, { .compatible = "intel,tangier-dwc3" },

-- 2.43.0

Hi Caleb,

On Thu, Feb 01, 2024 at 14:24, Caleb Connolly caleb.connolly@linaro.org wrote:

On 01/02/2024 09:34, Mattijs Korpershoek wrote:

...

Hi Caleb,

Thank you for the patch.

On mer., janv. 31, 2024 at 14:57, Caleb Connolly caleb.connolly@linaro.org wrote:

...

The Qualcomm specific dwc3 wrapper isn't hugely complicated, implemented the missing initialisation for host and gadget mode.

Signed-off-by: Caleb Connolly caleb.connolly@linaro.org

drivers/usb/dwc3/dwc3-generic.c | 99 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 98 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c index 48da621ba966..1119cdecd26d 100644 --- a/drivers/usb/dwc3/dwc3-generic.c +++ b/drivers/usb/dwc3/dwc3-generic.c @@ -419,6 +419,99 @@ struct dwc3_glue_ops ti_ops = { .glue_configure = dwc3_ti_glue_configure, };

+/* USB QSCRATCH Hardware registers */ +#define QSCRATCH_HS_PHY_CTRL 0x10 +#define UTMI_OTG_VBUS_VALID BIT(20) +#define SW_SESSVLD_SEL BIT(28)

+#define QSCRATCH_SS_PHY_CTRL 0x30 +#define LANE0_PWR_PRESENT BIT(24)

+#define QSCRATCH_GENERAL_CFG 0x08 +#define PIPE_UTMI_CLK_SEL BIT(0) +#define PIPE3_PHYSTATUS_SW BIT(3) +#define PIPE_UTMI_CLK_DIS BIT(8)

+#define PWR_EVNT_IRQ_STAT_REG 0x58 +#define PWR_EVNT_LPM_IN_L2_MASK BIT(4) +#define PWR_EVNT_LPM_OUT_L2_MASK BIT(5)

+#define SDM845_QSCRATCH_BASE_OFFSET 0xf8800 +#define SDM845_QSCRATCH_SIZE 0x400 +#define SDM845_DWC3_CORE_SIZE 0xcd00 +static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val) +{

• u32 reg;
• reg = readl(base + offset);
• reg |= val;
• writel(reg, base + offset);

Why can't we use the setbits() macro here? see: arch/arm/include/asm/io.h

setbits doesn't give us the same cache coherency guarantees I think(?) the readl/writel macros include wmb() calls.

Indeed, I did not pay attention to that difference.

That said, I won't pretend to really know what I'm talking about here, possible setbits() would be suitable, do you have any idea?

I am sorry, I don't know.

It's fine to keep it as is.

...

...

• /* ensure that above write is through */
• readl(base + offset);

+}

+static inline void dwc3_qcom_clrbits(void __iomem *base, u32 offset, u32 val) +{

• u32 reg;
• reg = readl(base + offset);
• reg &= ~val;
• writel(reg, base + offset);

Same question for clrbits()

...

• /* ensure that above write is through */
• readl(base + offset);

+}

[snip]

...

...

• if (device_is_compatible(dev, "qcom,dwc3")) {

• reset_assert_bulk(&glue->resets);
  

Any reason for not doing error handling on reset_assert_bulk() like it's done below ?

Well, the Qualcomm reset driver will never return an error, and if it did, the assert failing (presumably because it's already asserted) is not necessarily an error we'd need to care about - it's only an error if we can't deassert the reset.

So I think this is fine.

Ok, agreed.

Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.com

...

...

• udelay(500);
  

• } ret = reset_deassert_bulk(&glue->resets); if (ret) { reset_release_bulk(&glue->resets);

@@ -623,7 +720,7 @@ static const struct udevice_id dwc3_glue_ids[] = { { .compatible = "rockchip,rk3399-dwc3" }, { .compatible = "rockchip,rk3568-dwc3", .data = (ulong)&rk_ops }, { .compatible = "rockchip,rk3588-dwc3", .data = (ulong)&rk_ops },

• { .compatible = "qcom,dwc3" },

• { .compatible = "qcom,dwc3", .data = (ulong)&qcom_ops }, { .compatible = "fsl,imx8mp-dwc3", .data = (ulong)&imx8mp_ops }, { .compatible = "fsl,imx8mq-dwc3" }, { .compatible = "intel,tangier-dwc3" },

-- 2.43.0

-- // Caleb (they/them)

Hi Marek,

On 06/02/2024 20:36, Marek Vasut wrote:

On 1/31/24 15:57, Caleb Connolly wrote:

...

The Qualcomm specific dwc3 wrapper isn't hugely complicated, implemented the missing initialisation for host and gadget mode.

Signed-off-by: Caleb Connolly caleb.connolly@linaro.org

drivers/usb/dwc3/dwc3-generic.c | 99 ++++++++++++++++++++++++++++++++++++++++-   1 file changed, 98 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c index 48da621ba966..1119cdecd26d 100644 --- a/drivers/usb/dwc3/dwc3-generic.c +++ b/drivers/usb/dwc3/dwc3-generic.c @@ -419,6 +419,99 @@ struct dwc3_glue_ops ti_ops = {       .glue_configure = dwc3_ti_glue_configure,   };   +/* USB QSCRATCH Hardware registers */ +#define QSCRATCH_HS_PHY_CTRL 0x10 +#define UTMI_OTG_VBUS_VALID BIT(20) +#define SW_SESSVLD_SEL BIT(28)

+#define QSCRATCH_SS_PHY_CTRL 0x30 +#define LANE0_PWR_PRESENT BIT(24)

+#define QSCRATCH_GENERAL_CFG 0x08 +#define PIPE_UTMI_CLK_SEL BIT(0) +#define PIPE3_PHYSTATUS_SW BIT(3) +#define PIPE_UTMI_CLK_DIS BIT(8)

+#define PWR_EVNT_IRQ_STAT_REG 0x58 +#define PWR_EVNT_LPM_IN_L2_MASK BIT(4) +#define PWR_EVNT_LPM_OUT_L2_MASK BIT(5)

+#define SDM845_QSCRATCH_BASE_OFFSET 0xf8800 +#define SDM845_QSCRATCH_SIZE 0x400 +#define SDM845_DWC3_CORE_SIZE 0xcd00

Newline here please

...

+static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val) +{ +    u32 reg;

+    reg = readl(base + offset); +    reg |= val; +    writel(reg, base + offset);

Use setbits_le32() .

...

+    /* ensure that above write is through */ +    readl(base + offset);

Is this needed ?

I honestly don't know, this is copied from the Linux driver and it seems to be very defensively written. I doubt it's strictly necessary.

...

+}

+static inline void dwc3_qcom_clrbits(void __iomem *base, u32 offset, u32 val) +{ +    u32 reg;

+    reg = readl(base + offset); +    reg &= ~val; +    writel(reg, base + offset);

clrbits_le32()

...

+    /* ensure that above write is through */ +    readl(base + offset); +}

+static void dwc3_qcom_vbus_override_enable(void __iomem *qscratch_base, bool enable) +{ +    if (enable) { +        dwc3_qcom_setbits(qscratch_base, QSCRATCH_SS_PHY_CTRL, +                  LANE0_PWR_PRESENT); +        dwc3_qcom_setbits(qscratch_base, QSCRATCH_HS_PHY_CTRL, +                  UTMI_OTG_VBUS_VALID | SW_SESSVLD_SEL); +    } else { +        dwc3_qcom_clrbits(qscratch_base, QSCRATCH_SS_PHY_CTRL, +                  LANE0_PWR_PRESENT); +        dwc3_qcom_clrbits(qscratch_base, QSCRATCH_HS_PHY_CTRL, +                  UTMI_OTG_VBUS_VALID | SW_SESSVLD_SEL); +    } +}

+/* For controllers running without superspeed PHYs */ +static void dwc3_qcom_select_utmi_clk(void __iomem *qscratch_base) +{ +    /* Configure dwc3 to use UTMI clock as PIPE clock not present */ +    dwc3_qcom_setbits(qscratch_base, QSCRATCH_GENERAL_CFG, +              PIPE_UTMI_CLK_DIS);

+    udelay(500);

Isn't there some possibility to poll for completion instead of fixed delay ? If so, use wait_for_bit or some such .

Not that I'm aware of, no. I think this hardware just has a blanket "writes take X bus cycles to complete" rule or something. It's totally possible that this code was originally written this way to work around some issues on an FPGA prototype or something. Everything seems to still work if I remove the delays so I'll drop them...

...

+    dwc3_qcom_setbits(qscratch_base, QSCRATCH_GENERAL_CFG, +              PIPE_UTMI_CLK_SEL | PIPE3_PHYSTATUS_SW);

+    udelay(500);

+    dwc3_qcom_clrbits(qscratch_base, QSCRATCH_GENERAL_CFG, +              PIPE_UTMI_CLK_DIS); +}

+static void dwc3_qcom_glue_configure(struct udevice *dev, int index, +                     enum usb_dr_mode mode) +{ +    void __iomem *qscratch_base = (void __iomem *)dev_read_addr(dev);

Can this be NULL ? If yes, add check for != NULL .

Will do.

...

+    if (dev_read_bool(dev, "qcom,select-utmi-as-pipe-clk")) +        dwc3_qcom_select_utmi_clk(qscratch_base);

+    if (mode != USB_DR_MODE_HOST) +        dwc3_qcom_vbus_override_enable(qscratch_base, true); +}

+struct dwc3_glue_ops qcom_ops = { +    .glue_configure = dwc3_qcom_glue_configure, +};

static int dwc3_rk_glue_get_ctrl_dev(struct udevice *dev, ofnode *node)   {       *node = dev_ofnode(dev); @@ -506,6 +599,10 @@ static int dwc3_glue_reset_init(struct udevice *dev,       else if (ret)           return ret;   +    if (device_is_compatible(dev, "qcom,dwc3")) { +        reset_assert_bulk(&glue->resets); +        udelay(500);

Why this delay here ?

According to the docs, the reset should be asserted for at least 6 sleep clock cycles, that's ~200us on sdm845, but it can vary by platform.

[...]

Hi Caleb, Marek,

On jeu., mars 21, 2024 at 06:34, Marek Vasut marex@denx.de wrote:

On 3/13/24 7:22 PM, Caleb Connolly wrote:

[...]

...

...

...

+static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val) +{ +    u32 reg;

+    reg = readl(base + offset); +    reg |= val; +    writel(reg, base + offset);

Use setbits_le32() .

...

+    /* ensure that above write is through */ +    readl(base + offset);

Is this needed ?

I honestly don't know, this is copied from the Linux driver and it seems to be very defensively written. I doubt it's strictly necessary.

Does git log indicate anything ?

I suspect this is some sort of barrier .

[...]

...

...

...

+/* For controllers running without superspeed PHYs */ +static void dwc3_qcom_select_utmi_clk(void __iomem *qscratch_base) +{ +    /* Configure dwc3 to use UTMI clock as PIPE clock not present */ +    dwc3_qcom_setbits(qscratch_base, QSCRATCH_GENERAL_CFG, +              PIPE_UTMI_CLK_DIS);

+    udelay(500);

Isn't there some possibility to poll for completion instead of fixed delay ? If so, use wait_for_bit or some such .

Not that I'm aware of, no. I think this hardware just has a blanket "writes take X bus cycles to complete" rule or something. It's totally possible that this code was originally written this way to work around some issues on an FPGA prototype or something. Everything seems to still work if I remove the delays so I'll drop them...

Could you possibly ask someone ?

[...]

...

...

...

static int dwc3_rk_glue_get_ctrl_dev(struct udevice *dev, ofnode *node)   {       *node = dev_ofnode(dev); @@ -506,6 +599,10 @@ static int dwc3_glue_reset_init(struct udevice *dev,       else if (ret)           return ret;   +    if (device_is_compatible(dev, "qcom,dwc3")) { +        reset_assert_bulk(&glue->resets); +        udelay(500);

Why this delay here ?

According to the docs, the reset should be asserted for at least 6 sleep clock cycles, that's ~200us on sdm845, but it can vary by platform.

A comment in the code would be nice.

Sorry for the abysmal delay in replies.

btw. the new version of this series is still OK to go in, unless you want to fill in the comments. They can also go in in separate follow up patch.

I'm interested by the answers above as well. As I took in the series [1] (to avoid delaying it too much), please consider a follow up patch to add a comment.

[1] https://lore.kernel.org/r/all/171101299073.1017001.16411913317437946645.b4-t...

On jeu., mars 21, 2024 at 11:34, Caleb Connolly caleb.connolly@linaro.org wrote:

Hi,

On 21/03/2024 09:25, Mattijs Korpershoek wrote:

...

Hi Caleb, Marek,

On jeu., mars 21, 2024 at 06:34, Marek Vasut marex@denx.de wrote:

...

On 3/13/24 7:22 PM, Caleb Connolly wrote:

[...]

...

...

...

+static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val) +{ +    u32 reg;

+    reg = readl(base + offset); +    reg |= val; +    writel(reg, base + offset);

Use setbits_le32() .

...

+    /* ensure that above write is through */ +    readl(base + offset);

Is this needed ?

I honestly don't know, this is copied from the Linux driver and it seems to be very defensively written. I doubt it's strictly necessary.

Does git log indicate anything ?

Nope :/ it's there from when the driver was first added.

...

...

I suspect this is some sort of barrier .

[...]

...

...

...

+/* For controllers running without superspeed PHYs */ +static void dwc3_qcom_select_utmi_clk(void __iomem *qscratch_base) +{ +    /* Configure dwc3 to use UTMI clock as PIPE clock not present */ +    dwc3_qcom_setbits(qscratch_base, QSCRATCH_GENERAL_CFG, +              PIPE_UTMI_CLK_DIS);

+    udelay(500);

Isn't there some possibility to poll for completion instead of fixed delay ? If so, use wait_for_bit or some such .

Not that I'm aware of, no. I think this hardware just has a blanket "writes take X bus cycles to complete" rule or something. It's totally possible that this code was originally written this way to work around some issues on an FPGA prototype or something. Everything seems to still work if I remove the delays so I'll drop them...

Could you possibly ask someone ?

Yeah I'll ask around, I'm not confident I'll find an answer though.

...

...

[...]

...

...

...

static int dwc3_rk_glue_get_ctrl_dev(struct udevice *dev, ofnode *node)   {       *node = dev_ofnode(dev); @@ -506,6 +599,10 @@ static int dwc3_glue_reset_init(struct udevice *dev,       else if (ret)           return ret;   +    if (device_is_compatible(dev, "qcom,dwc3")) { +        reset_assert_bulk(&glue->resets); +        udelay(500);

Why this delay here ?

According to the docs, the reset should be asserted for at least 6 sleep clock cycles, that's ~200us on sdm845, but it can vary by platform.

A comment in the code would be nice.

Sorry for the abysmal delay in replies.

btw. the new version of this series is still OK to go in, unless you want to fill in the comments. They can also go in in separate follow up patch.

I'm interested by the answers above as well. As I took in the series [1] (to avoid delaying it too much), please consider a follow up patch to add a comment.

The v4 you picked up has a comment explaining this.

Right, sorry I missed that. Thanks for pointing it out to me!

...

[1] https://lore.kernel.org/r/all/171101299073.1017001.16411913317437946645.b4-t...

-- // Caleb (they/them)

Hi Caleb,

Thank you for the patch.

On mer., janv. 31, 2024 at 14:57, Caleb Connolly caleb.connolly@linaro.org wrote:

DWC3 platforms depend on DM_USB_GADGET for gadget drivers to work, otherwise compilation fails due to no implementation of dm_usb_gadget_handle_interrupts().

Signed-off-by: Caleb Connolly caleb.connolly@linaro.org

Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.com

---

drivers/usb/dwc3/Kconfig | 1 + 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig index c0c8c16fd9c2..8a70bc682322 100644 --- a/drivers/usb/dwc3/Kconfig +++ b/drivers/usb/dwc3/Kconfig @@ -11,6 +11,7 @@ config USB_DWC3_GADGET bool "USB Gadget support for DWC3" default y depends on USB_GADGET

• select DM_USB_GADGET select USB_GADGET_DUALSPEED

comment "Platform Glue Driver Support"

-- 2.43.0

Hi Caleb,

Thank you for the patch.

On mer., janv. 31, 2024 at 14:57, Caleb Connolly caleb.connolly@linaro.org wrote:

To actually use the gadget the peripheral driver must be probed and we must call g_dnl_clear_detach(). Otherwise acm_stdio_start() will always fail to find a UDC on DT platforms.

Signed-off-by: Caleb Connolly caleb.connolly@linaro.org

Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.com

---

drivers/usb/gadget/f_acm.c | 9 +++++++++ 1 file changed, 9 insertions(+)

diff --git a/drivers/usb/gadget/f_acm.c b/drivers/usb/gadget/f_acm.c index de42e0189e8d..ba216128ab27 100644 --- a/drivers/usb/gadget/f_acm.c +++ b/drivers/usb/gadget/f_acm.c @@ -623,12 +623,21 @@ static void acm_stdio_puts(struct stdio_dev *dev, const char *str)

static int acm_stdio_start(struct stdio_dev *dev) {

• struct udevice *udc; int ret;

  if (dev->priv) { /* function already exist */ return 0; }

• ret = udc_device_get_by_index(0, &udc);

• if (ret) {

• pr_err("USB init failed: %d\n", ret);
  

• return ret;
  

• }

• g_dnl_clear_detach();

• ret = g_dnl_register("usb_serial_acm"); if (ret) return ret;

-- 2.43.0

Hi Caleb,

Thank you for the patch.

On mer., janv. 31, 2024 at 14:57, Caleb Connolly caleb.connolly@linaro.org wrote:

UFS storage often uses a 4096-byte sector size, add support for dynamic sector sizes based loosely on the Linux implementation.

Signed-off-by: Caleb Connolly caleb.connolly@linaro.org

Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.com

---

cmd/usb_mass_storage.c | 4 -- drivers/usb/gadget/f_mass_storage.c | 101 ++++++++++++++++++++---------------- drivers/usb/gadget/storage_common.c | 12 +++-- include/usb_mass_storage.h | 1 - 4 files changed, 65 insertions(+), 53 deletions(-)

diff --git a/cmd/usb_mass_storage.c b/cmd/usb_mass_storage.c index a8ddeb494628..751701fe73af 100644 --- a/cmd/usb_mass_storage.c +++ b/cmd/usb_mass_storage.c @@ -88,10 +88,6 @@ static int ums_init(const char *devtype, const char *devnums_part_str) if (!strchr(devnum_part_str, ':')) partnum = 0;

• /* f_mass_storage.c assumes SECTOR_SIZE sectors */
  

• if (block_dev->blksz != SECTOR_SIZE)
  

• 	goto cleanup;
  

• ums_new = realloc(ums, (ums_count + 1) * sizeof(*ums)); if (!ums_new) goto cleanup;

diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c index c725aed3f626..d880928044f4 100644 --- a/drivers/usb/gadget/f_mass_storage.c +++ b/drivers/usb/gadget/f_mass_storage.c @@ -724,12 +724,13 @@ static int do_read(struct fsg_common *common) curlun->sense_data = SS_LOGICAL_BLOCK_ADDRESS_OUT_OF_RANGE; return -EINVAL; }

• file_offset = ((loff_t) lba) << 9;

• file_offset = ((loff_t)lba) << curlun->blkbits;

  /* Carry out the file reads */ amount_left = common->data_size_from_cmnd;

• if (unlikely(amount_left == 0))

• if (unlikely(amount_left == 0)) { return -EIO; /* No default reply */

• }

  for (;;) {

@@ -768,13 +769,13 @@ static int do_read(struct fsg_common *common)

/* Perform the read */
rc = ums[common->lun].read_sector(&ums[common->lun],

• 		      file_offset / SECTOR_SIZE,
  

• 		      amount / SECTOR_SIZE,
  

• 		      file_offset / curlun->blksize,
  

• 		      amount / curlun->blksize,
  	      (char __user *)bh->buf);
  

  if (!rc) return -EIO;

• nread = rc * SECTOR_SIZE;
  

• nread = rc * curlun->blksize;
  
  

  VLDBG(curlun, "file read %u @ %llu -> %d\n", amount, (unsigned long long) file_offset,

@@ -787,7 +788,7 @@ static int do_read(struct fsg_common *common) } else if (nread < amount) { LDBG(curlun, "partial file read: %d/%u\n", (int) nread, amount);

• 	nread -= (nread & 511);	/* Round down to a block */
  

• 	nread -= (nread & (curlun->blksize - 1));	/* Round down to a block */
  

  } file_offset += nread; amount_left -= nread;

@@ -861,7 +862,7 @@ static int do_write(struct fsg_common *common)

/* Carry out the file writes */ get_some_more = 1;

• file_offset = usb_offset = ((loff_t) lba) << 9;

• file_offset = usb_offset = ((loff_t)lba) << curlun->blkbits; amount_left_to_req = common->data_size_from_cmnd; amount_left_to_write = common->data_size_from_cmnd;

@@ -893,7 +894,7 @@ static int do_write(struct fsg_common *common) curlun->info_valid = 1; continue; }

• 	amount -= (amount & 511);
  

• 	amount -= (amount & (curlun->blksize - 1));
  if (amount == 0) {
  
  	/* Why were we were asked to transfer a
  

@@ -942,12 +943,12 @@ static int do_write(struct fsg_common *common)

	/* Perform the write */
	rc = ums[common->lun].write_sector(&ums[common->lun],

• 			       file_offset / SECTOR_SIZE,
  

• 			       amount / SECTOR_SIZE,
  

• 			       file_offset / curlun->blksize,
  

• 			       amount / curlun->blksize,
  		       (char __user *)bh->buf);
  if (!rc)
  	return -EIO;
  

• 	nwritten = rc * SECTOR_SIZE;
  

• 	nwritten = rc * curlun->blksize;
  
  VLDBG(curlun, "file write %u @ %llu -> %d\n", amount,
  		(unsigned long long) file_offset,
  

@@ -960,7 +961,7 @@ static int do_write(struct fsg_common *common) } else if (nwritten < amount) { LDBG(curlun, "partial file write: %d/%u\n", (int) nwritten, amount);

• 		nwritten -= (nwritten & 511);
  

• 		nwritten -= (nwritten & (curlun->blksize - 1));
  	/* Round down to a block */
  }
  file_offset += nwritten;
  

@@ -1034,8 +1035,8 @@ static int do_verify(struct fsg_common *common) return -EIO; /* No default reply */

/* Prepare to carry out the file verify */

• amount_left = verification_length << 9;
• file_offset = ((loff_t) lba) << 9;

• amount_left = verification_length << curlun->blkbits;

• file_offset = ((loff_t) lba) << curlun->blkbits;

  /* Write out all the dirty buffers before invalidating them */

@@ -1058,12 +1059,12 @@ static int do_verify(struct fsg_common *common)

/* Perform the read */
rc = ums[common->lun].read_sector(&ums[common->lun],

• 		      file_offset / SECTOR_SIZE,
  

• 		      amount / SECTOR_SIZE,
  

• 		      file_offset / curlun->blksize,
  

• 		      amount / curlun->blksize,
  	      (char __user *)bh->buf);
  

  if (!rc) return -EIO;

• nread = rc * SECTOR_SIZE;
  

• nread = rc * curlun->blksize;
  
  

  VLDBG(curlun, "file read %u @ %llu -> %d\n", amount, (unsigned long long) file_offset,

@@ -1075,7 +1076,7 @@ static int do_verify(struct fsg_common *common) } else if (nread < amount) { LDBG(curlun, "partial file verify: %d/%u\n", (int) nread, amount);

• 	nread -= (nread & 511);	/* Round down to a sector */
  

• 	nread -= (nread & (curlun->blksize - 1));	/* Round down to a sector */
  

  } if (nread == 0) { curlun->sense_data = SS_UNRECOVERED_READ_ERROR;

@@ -1183,7 +1184,7 @@ static int do_read_capacity(struct fsg_common *common, struct fsg_buffhd *bh)

put_unaligned_be32(curlun->num_sectors - 1, &buf[0]); /* Max logical block */

• put_unaligned_be32(512, &buf[4]); /* Block length */

• put_unaligned_be32(curlun->blksize, &buf[4]); /* Block length */ return 8;

}

@@ -1370,7 +1371,7 @@ static int do_read_format_capacities(struct fsg_common *common,

put_unaligned_be32(curlun->num_sectors, &buf[0]); /* Number of blocks */

• put_unaligned_be32(512, &buf[4]); /* Block length */

• put_unaligned_be32(curlun->blksize, &buf[4]); /* Block length */ buf[4] = 0x02; /* Current capacity */ return 12;

} @@ -1781,6 +1782,16 @@ static int check_command(struct fsg_common *common, int cmnd_size, return 0; }

+/* wrapper of check_command for data size in blocks handling */ +static int check_command_size_in_blocks(struct fsg_common *common,

• int cmnd_size, enum data_direction data_dir,
  

• unsigned int mask, int needs_medium, const char *name)
  

+{

• common->data_size_from_cmnd <<= common->luns[common->lun].blkbits;
• return check_command(common, cmnd_size, data_dir,

• 	mask, needs_medium, name);
  

+}

static int do_scsi_command(struct fsg_common *common) { @@ -1865,30 +1876,30 @@ static int do_scsi_command(struct fsg_common *common)

case SC_READ_6: i = common->cmnd[4];

• common->data_size_from_cmnd = (i == 0 ? 256 : i) << 9;
  

• reply = check_command(common, 6, DATA_DIR_TO_HOST,
  

• 		      (7<<1) | (1<<4), 1,
  

• 		      "READ(6)");
  

• common->data_size_from_cmnd = (i == 0 ? 256 : i);
  

• reply = check_command_size_in_blocks(common, 6, DATA_DIR_TO_HOST,
  

• 				     (7<<1) | (1<<4), 1,
  

• 				     "READ(6)");
  

  if (reply == 0) reply = do_read(common); break;

  case SC_READ_10: common->data_size_from_cmnd =

• 		get_unaligned_be16(&common->cmnd[7]) << 9;
  

• reply = check_command(common, 10, DATA_DIR_TO_HOST,
  

• 		      (1<<1) | (0xf<<2) | (3<<7), 1,
  

• 		      "READ(10)");
  

• 		get_unaligned_be16(&common->cmnd[7]);
  

• reply = check_command_size_in_blocks(common, 10, DATA_DIR_TO_HOST,
  

• 				     (1<<1) | (0xf<<2) | (3<<7), 1,
  

• 				     "READ(10)");
  

  if (reply == 0) reply = do_read(common); break;

  case SC_READ_12: common->data_size_from_cmnd =

• 		get_unaligned_be32(&common->cmnd[6]) << 9;
  

• reply = check_command(common, 12, DATA_DIR_TO_HOST,
  

• 		      (1<<1) | (0xf<<2) | (0xf<<6), 1,
  

• 		      "READ(12)");
  

• 		get_unaligned_be32(&common->cmnd[6]);
  

• reply = check_command_size_in_blocks(common, 12, DATA_DIR_TO_HOST,
  

• 				     (1<<1) | (0xf<<2) | (0xf<<6), 1,
  

• 				     "READ(12)");
  

  if (reply == 0) reply = do_read(common); break;

@@ -1983,30 +1994,30 @@ static int do_scsi_command(struct fsg_common *common)

case SC_WRITE_6: i = common->cmnd[4];

• common->data_size_from_cmnd = (i == 0 ? 256 : i) << 9;
  

• reply = check_command(common, 6, DATA_DIR_FROM_HOST,
  

• 		      (7<<1) | (1<<4), 1,
  

• 		      "WRITE(6)");
  

• common->data_size_from_cmnd = (i == 0 ? 256 : i);
  

• reply = check_command_size_in_blocks(common, 6, DATA_DIR_FROM_HOST,
  

• 				     (7<<1) | (1<<4), 1,
  

• 				     "WRITE(6)");
  

  if (reply == 0) reply = do_write(common); break;

  case SC_WRITE_10: common->data_size_from_cmnd =

• 		get_unaligned_be16(&common->cmnd[7]) << 9;
  

• reply = check_command(common, 10, DATA_DIR_FROM_HOST,
  

• 		      (1<<1) | (0xf<<2) | (3<<7), 1,
  

• 		      "WRITE(10)");
  

• 		get_unaligned_be16(&common->cmnd[7]);
  

• reply = check_command_size_in_blocks(common, 10, DATA_DIR_FROM_HOST,
  

• 				     (1<<1) | (0xf<<2) | (3<<7), 1,
  

• 				     "WRITE(10)");
  

  if (reply == 0) reply = do_write(common); break;

  case SC_WRITE_12: common->data_size_from_cmnd =

• 		get_unaligned_be32(&common->cmnd[6]) << 9;
  

• reply = check_command(common, 12, DATA_DIR_FROM_HOST,
  

• 		      (1<<1) | (0xf<<2) | (0xf<<6), 1,
  

• 		      "WRITE(12)");
  

• 		get_unaligned_be32(&common->cmnd[6]);
  

• reply = check_command_size_in_blocks(common, 12, DATA_DIR_FROM_HOST,
  

• 				     (1<<1) | (0xf<<2) | (0xf<<6), 1,
  

• 				     "WRITE(12)");
  

  if (reply == 0) reply = do_write(common); break;

@@ -2497,7 +2508,7 @@ static struct fsg_common *fsg_common_init(struct fsg_common *common, for (i = 0; i < nluns; i++) { common->luns[i].removable = 1;

• rc = fsg_lun_open(&common->luns[i], ums[i].num_sectors, "");
  

• rc = fsg_lun_open(&common->luns[i], ums[i].num_sectors, ums->block_dev.blksz, "");
  

  if (rc) goto error_luns; }

diff --git a/drivers/usb/gadget/storage_common.c b/drivers/usb/gadget/storage_common.c index 5674e8fe4940..97dc6b6f729c 100644 --- a/drivers/usb/gadget/storage_common.c +++ b/drivers/usb/gadget/storage_common.c @@ -269,6 +269,7 @@ struct device_attribute { int i; }; #define ETOOSMALL 525

#include <log.h> +#include <linux/log2.h> #include <usb_mass_storage.h> #include <dm/device_compat.h>

@@ -290,6 +291,8 @@ struct fsg_lun { u32 sense_data; u32 sense_data_info; u32 unit_attention_data;

• unsigned int blkbits;

• unsigned int blksize; /* logical block size of bound block device */

  struct device dev;

}; @@ -566,7 +569,7 @@ static struct usb_gadget_strings fsg_stringtab = { */

static int fsg_lun_open(struct fsg_lun *curlun, unsigned int num_sectors,

• 	const char *filename)
  

• 	unsigned int sector_size, const char *filename)
  

{ int ro;

@@ -574,9 +577,12 @@ static int fsg_lun_open(struct fsg_lun *curlun, unsigned int num_sectors, ro = curlun->initially_ro;

curlun->ro = ro;

• curlun->file_length = num_sectors << 9;

• curlun->file_length = num_sectors * sector_size; curlun->num_sectors = num_sectors;

• debug("open backing file: %s\n", filename);

• curlun->blksize = sector_size;

• curlun->blkbits = order_base_2(sector_size >> 9) + 9;

• debug("blksize: %u\n", sector_size);

• debug("open backing file: '%s'\n", filename);

  return 0;

} diff --git a/include/usb_mass_storage.h b/include/usb_mass_storage.h index 83ab93b530d7..6d83d93cad7f 100644 --- a/include/usb_mass_storage.h +++ b/include/usb_mass_storage.h @@ -7,7 +7,6 @@ #ifndef __USB_MASS_STORAGE_H__ #define __USB_MASS_STORAGE_H__

-#define SECTOR_SIZE 0x200 #include <part.h> #include <linux/usb/composite.h>

-- 2.43.0

Hi Caleb,

Thank you for the patch.

On mer., janv. 31, 2024 at 14:57, Caleb Connolly caleb.connolly@linaro.org wrote:

The priv struct was wrong in dump_boot_mappings(). Causing errors when compiling with -DDEBUG. Fix this.

Signed-off-by: Caleb Connolly caleb.connolly@linaro.org

Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.com

---

drivers/iommu/qcom-hyp-smmu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/qcom-hyp-smmu.c b/drivers/iommu/qcom-hyp-smmu.c index 8e5cdb581550..f2b39de56f4a 100644 --- a/drivers/iommu/qcom-hyp-smmu.c +++ b/drivers/iommu/qcom-hyp-smmu.c @@ -319,7 +319,7 @@ static int qcom_smmu_connect(struct udevice *dev) }

#ifdef DEBUG -static inline void dump_boot_mappings(struct arm_smmu_priv *priv) +static inline void dump_boot_mappings(struct qcom_smmu_priv *priv) { u32 val; int i;

-- 2.43.0