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

[Caleb Connolly]

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 at 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 at 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.

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?
> 
>> +
>> +	/* 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.
> 
>> +		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)