[PATCH v5 03/16] phy: qcom: Add SNPS femto v2 USB HS phy

[Sumit Garg]

On Tue, 2 Apr 2024 at 15:37, Caleb Connolly <caleb.connolly at linaro.org> wrote:
>
> Hi Sumit,
>
> On 01/04/2024 06:46, Sumit Garg wrote:
> > On Thu, 28 Mar 2024 at 23:29, Caleb Connolly <caleb.connolly at linaro.org> wrote:
> >>
> >> From: Bhupesh Sharma <bhupesh.linux at gmail.com>
> >>
> >> Some Qualcomm SoCs newer than SDM845 feature a so-called "7nm phy"
> >> driver, notable the SM8250 SoC which will gain U-Boot support in
> >> upcoming patches.
> >>
> >> Introduce a driver based on the Linux driver.
> >>
> >> Signed-off-by: Bhupesh Sharma <bhupesh.sharma at linaro.org>
> >> [code cleanup, align symbol names with Linux, switch to clk/reset_bulk APIs]
> >> Signed-off-by: Caleb Connolly <caleb.connolly at linaro.org>
> >> ---
> >>   drivers/phy/qcom/Kconfig                  |   8 ++
> >>   drivers/phy/qcom/Makefile                 |   1 +
> >>   drivers/phy/qcom/phy-qcom-snps-femto-v2.c | 207 ++++++++++++++++++++++++++++++
> >>   3 files changed, 216 insertions(+)
> >>
> ...
> >> diff --git a/drivers/phy/qcom/phy-qcom-snps-femto-v2.c b/drivers/phy/qcom/phy-qcom-snps-femto-v2.c
> >> new file mode 100644
> >> index 000000000000..58eb01972402
> >> --- /dev/null
> >> +++ b/drivers/phy/qcom/phy-qcom-snps-femto-v2.c
> ...
> >> +static inline void qcom_snps_hsphy_write_mask(void __iomem *base, u32 offset,
> >> +                                             u32 mask, u32 val)
> >> +{
> >> +       u32 reg;
> >> +
> >> +       reg = readl_relaxed(base + offset);
> >> +
> >> +       reg &= ~mask;
> >> +       reg |= val & mask;
> >> +       writel_relaxed(reg, base + offset);
> >> +
> >> +       /* Ensure above write is completed */
> >> +       readl_relaxed(base + offset);
> >
> > It looks like you have missed addressing comments related to this API.
> > Again, why do we need this special handling in U-Boot? Why not just
> > clrsetbits_le32()?
>
> I have tried to find more info about this, but from what I can tell it's
> important (my guess is that it's a weird bus arbitration thing with the
> QMP phy's).
>
> If I replace this with clrsetbits_le32() then my SM8250 board crashdumps
> while probing USB. So it is needed (and I'm wondering now if I ought to
> switch back to this for the dwc3 qcom glue as well).

Strange, probably it's due to some strict device memory ordering
rules. If you can add some reasoning in comments for this API then
feel free to add:

Acked-by: Sumit Garg <sumit.garg at linaro.org>

-Sumit