Message ID | 20250304-sm8750_usb_master-v2-0-a698a2e68e06@quicinc.com |
---|---|
Headers | show |
Series | phy: qcom: Introduce USB support for SM8750 | expand |
On 3/4/25 10:56 PM, Melody Olvera wrote: > From: Wesley Cheng <quic_wcheng@quicinc.com> > > SM8750 utilizes an eUSB2 PHY from M31. Add the initialization > sequences to bring it out of reset and into an operational state. This > differs to the M31 USB driver, in that the M31 eUSB2 driver will > require a connection to an eUSB2 repeater. This PHY driver will handle > the initialization of the associated eUSB2 repeater when required. > > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> > Signed-off-by: Melody Olvera <quic_molvera@quicinc.com> > --- [...] > +static int msm_m31_eusb2_write_readback(void __iomem *base, u32 offset, > + const u32 mask, u32 val) > +{ > + u32 write_val; > + u32 tmp; > + > + tmp = readl_relaxed(base + offset); > + tmp &= ~mask; > + write_val = tmp | val; > + > + writel_relaxed(write_val, base + offset); > + > + tmp = readl_relaxed(base + offset); > + tmp &= mask; > + > + if (tmp != val) { > + pr_err("write: %x to offset: %x FAILED\n", val, offset); > + return -EINVAL; > + } > + > + return 0; Is there a reason we need to read back every write? Does this have to do with some funny write buffering? Konrad
Hi Konrad, On 3/11/2025 4:19 AM, Konrad Dybcio wrote: > On 3/4/25 10:56 PM, Melody Olvera wrote: >> From: Wesley Cheng <quic_wcheng@quicinc.com> >> >> SM8750 utilizes an eUSB2 PHY from M31. Add the initialization >> sequences to bring it out of reset and into an operational state. This >> differs to the M31 USB driver, in that the M31 eUSB2 driver will >> require a connection to an eUSB2 repeater. This PHY driver will handle >> the initialization of the associated eUSB2 repeater when required. >> >> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> >> Signed-off-by: Melody Olvera <quic_molvera@quicinc.com> >> --- > > [...] > >> +static int msm_m31_eusb2_write_readback(void __iomem *base, u32 offset, >> + const u32 mask, u32 val) >> +{ >> + u32 write_val; >> + u32 tmp; >> + >> + tmp = readl_relaxed(base + offset); >> + tmp &= ~mask; >> + write_val = tmp | val; >> + >> + writel_relaxed(write_val, base + offset); >> + >> + tmp = readl_relaxed(base + offset); >> + tmp &= mask; >> + >> + if (tmp != val) { >> + pr_err("write: %x to offset: %x FAILED\n", val, offset); >> + return -EINVAL; >> + } >> + >> + return 0; > > Is there a reason we need to read back every write? > > Does this have to do with some funny write buffering? > Probably because its just a form of write synchronization, since we're using the relaxed variants. If desired I can switch to just using writel and remove the readback. Thanks Wesley Cheng
On 3/19/25 8:03 PM, Wesley Cheng wrote: > Hi Konrad, > > On 3/11/2025 4:19 AM, Konrad Dybcio wrote: >> On 3/4/25 10:56 PM, Melody Olvera wrote: >>> From: Wesley Cheng <quic_wcheng@quicinc.com> >>> >>> SM8750 utilizes an eUSB2 PHY from M31. Add the initialization >>> sequences to bring it out of reset and into an operational state. This >>> differs to the M31 USB driver, in that the M31 eUSB2 driver will >>> require a connection to an eUSB2 repeater. This PHY driver will handle >>> the initialization of the associated eUSB2 repeater when required. >>> >>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com> >>> Signed-off-by: Melody Olvera <quic_molvera@quicinc.com> >>> --- >> >> [...] >> >>> +static int msm_m31_eusb2_write_readback(void __iomem *base, u32 offset, >>> + const u32 mask, u32 val) >>> +{ >>> + u32 write_val; >>> + u32 tmp; >>> + >>> + tmp = readl_relaxed(base + offset); >>> + tmp &= ~mask; >>> + write_val = tmp | val; >>> + >>> + writel_relaxed(write_val, base + offset); >>> + >>> + tmp = readl_relaxed(base + offset); >>> + tmp &= mask; >>> + >>> + if (tmp != val) { >>> + pr_err("write: %x to offset: %x FAILED\n", val, offset); >>> + return -EINVAL; >>> + } >>> + >>> + return 0; >> >> Is there a reason we need to read back every write? >> >> Does this have to do with some funny write buffering? >> > > Probably because its just a form of write synchronization, since we're > using the relaxed variants. If desired I can switch to just using writel > and remove the readback. non-relaxed variants are defined something like: writel(foo) { writel_relaxed(foo); wmb(); } with readbacks enforcing much stronger ordering (via a data/address dependency) than a barrier, i.e. if you write to an address and read back the register, the write must have arrived at the destination hardware (which is not a given otherwise, see: 2f8cf2c3f3e3 ("clk: qcom: reset: Ensure write completion on reset de/assertion") Konrad