On Thu Jul 10, 2025 at 2:29 PM CEST, Konrad Dybcio wrote: > On 7/10/25 2:25 PM, Luca Weiss wrote: >> On Thu Jul 10, 2025 at 2:10 PM CEST, Konrad Dybcio wrote: >>> On 7/9/25 11:18 AM, Luca Weiss wrote: >>>> As per the downstream devicetree for Milos, add a register write for >>>> QCOM_USB_PHY_CFG_CTRL_1 as per the "eUSB2 HPG version 1.0.2 update". >>>> >>>> Reviewed-by: Neil Armstrong <[email protected]> >>>> Signed-off-by: Luca Weiss <[email protected]> >>>> --- >>>> The downstream driver supports an arbitrary extra init sequence via >>>> qcom,param-override-seq. >>>> >>>> volcano-usb.dtsi has the following which is implemented in this patch: >>>> >>>> /* eUSB2 HPG version 1.0.2 update */ >>>> qcom,param-override-seq = >>>> <0x00 0x58>; >>>> --- >>>> drivers/phy/phy-snps-eusb2.c | 6 ++++++ >>>> 1 file changed, 6 insertions(+) >>>> >>>> diff --git a/drivers/phy/phy-snps-eusb2.c b/drivers/phy/phy-snps-eusb2.c >>>> index >>>> e232b8b4d29100b8fee9e913e2124788af09f2aa..87fc086424ba4d9fb3ce870aa7f7971a51d4a567 >>>> 100644 >>>> --- a/drivers/phy/phy-snps-eusb2.c >>>> +++ b/drivers/phy/phy-snps-eusb2.c >>>> @@ -420,6 +420,12 @@ static int qcom_snps_eusb2_hsphy_init(struct phy *p) >>>> /* set default parameters */ >>>> qcom_eusb2_default_parameters(phy); >>>> >>>> + if (of_device_is_compatible(p->dev.of_node, >>>> "qcom,milos-snps-eusb2-phy")) { >>>> + /* eUSB2 HPG version 1.0.2 update */ >>>> + writel_relaxed(0x0, phy->base + QCOM_USB_PHY_CFG_CTRL_1); >>>> + readl_relaxed(phy->base + QCOM_USB_PHY_CFG_CTRL_1); >>> >>> Said HPG asks to clear bits [7:1] on all targets >> >> Okay, so make this unconditional and only update those bits instead of >> writing the full register? > > Yes > >> >> Keep the write at this location, or move the code somewhere else in the >> function? > Let's simply do this instead: > > diff --git a/drivers/phy/phy-snps-eusb2.c b/drivers/phy/phy-snps-eusb2.c > index 87f323e758d6..6c44d0366f34 100644 > --- a/drivers/phy/phy-snps-eusb2.c > +++ b/drivers/phy/phy-snps-eusb2.c > @@ -392,7 +392,7 @@ static int qcom_snps_eusb2_hsphy_init(struct phy *p) > > snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_CFG_CTRL_1, > PHY_CFG_PLL_CPBIAS_CNTRL_MASK, > - FIELD_PREP(PHY_CFG_PLL_CPBIAS_CNTRL_MASK, > 0x1)); > + FIELD_PREP(PHY_CFG_PLL_CPBIAS_CNTRL_MASK, > 0x0)); > > snps_eusb2_hsphy_write_mask(phy->base, QCOM_USB_PHY_CFG_CTRL_4, > PHY_CFG_PLL_INT_CNTRL_MASK
Sounds reasonable, if the HPG suggests that... Just asking myself why this wasn't updated in the driver but only added via the qcom,param-override-seq for some SoCs. But downstream is still downstream I guess. Regards Luca > > > Konrad
