Re: [PATCH 1/5] usb: dwc3-generic: implement Qualcomm wrapper

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

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)