Hi Caleb,
On Thu, Feb 01, 2024 at 14:24, Caleb Connolly <caleb.conno...@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.conno...@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.conno...@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 <mkorpersh...@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)
