| Message ID | 20221214223733.648167-1-marijn.suijten@somainline.org |
|---|---|
| State | Accepted |
| Commit | d60c471a8670635b65b3d67b6e7aaa263d191cc3 |
| Headers | show |
| Series | Revert "phy: qualcomm: usb28nm: Add MDM9607 init sequence" | expand |
On 14/12/2022 22:37, Marijn Suijten wrote: > This reverts commit 557a28811c7e0286d3816842032db5eb7bb5f156. > > This commit introduced an init sequence from downstream DT [1] in the > driver. As mentioned by the comment above the HSPHY_INIT_CFG macro for > this sequence: > > /* > * The macro is used to define an initialization sequence. Each tuple > * is meant to program 'value' into phy register at 'offset' with 'delay' > * in us followed. > */ > > Instead of corresponding to offsets into the phy register, the sequence > read by the downstream driver [2] is passed into ulpi_write [3] which > crafts the address-value pair into a new value and writes it into the > same register at USB_ULPI_VIEWPORT [4]. In other words, this init > sequence is programmed into the hardware in a totally different way than > downstream and is unlikely to achieve the desired result, if the hsphy > is working at all. > > An alternative method needs to be found to write these init values at > the desired location. Fortunately mdm9607 did not land upstream yet [5] > and should have its compatible revised to use the generic one, instead > of a compatible that writes wrong data to the wrong registers. > > [1]: https://android.googlesource.com/kernel/msm/+/android-7.1.0_r0.2/arch/arm/boot/dts/qcom/mdm9607.dtsi#585 > [2]: https://android.googlesource.com/kernel/msm/+/android-7.1.0_r0.2/drivers/usb/phy/phy-msm-usb.c#4183 > [3]: https://android.googlesource.com/kernel/msm/+/android-7.1.0_r0.2/drivers/usb/phy/phy-msm-usb.c#468 > [4]: https://android.googlesource.com/kernel/msm/+/android-7.1.0_r0.2/drivers/usb/phy/phy-msm-usb.c#418 > [5]: https://lore.kernel.org/linux-arm-msm/20210805222812.40731-1-konrad.dybcio@somainline.org/ > > Reported-by: Michael Srba <Michael.Srba@seznam.cz> > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> Hmm. I got a bit concerned qcs405 was broken too, which would be worrying since I remember testing this code, though not specifically turning off the PHY init, then again, there's a near zero chance the USB PHY would work after reset without the specified init seq. The original upstreamed code for qcs404/405 https://android.googlesource.com/kernel/msm/+/refs/heads/android-msm-coral-4.14-android10/arch/arm64/boot/dts/qcom/qcs405-usb.dtsi https://android.googlesource.com/kernel/msm/+/refs/heads/android-msm-coral-4.14-android10/drivers/usb/phy/phy-qcom-snps-28nm-hs.c Does a relative write to an offset of the PHY CSR CSR base is 0x7a000 https://android.googlesource.com/kernel/msm/+/refs/heads/android-msm-coral-4.14-android10/arch/arm64/boot/dts/qcom/qcs405-usb.dtsi#74 https://android.googlesource.com/kernel/msm/+/refs/heads/android-msm-coral-4.14-android10/drivers/usb/phy/phy-qcom-snps-28nm-hs.c#285 which will result in 0x7a000 + 0xc0 = 0x01, etc which for us upstream then is https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/qcom/qcs404.dtsi#L336 and https://github.com/torvalds/linux/blob/master/drivers/phy/qualcomm/phy-qcom-usb-hs-28nm.c#L388 Writing 0x7a000 + 0xc0 = 0x01, etc - a writel() in this case so that bit is fine. mdm9607 https://android.googlesource.com/kernel/msm/+/android-7.1.0_r0.2/arch/arm/boot/dts/qcom/mdm9607.dtsi#569 - compat string is CSR is @ 0x6c000 https://android.googlesource.com/kernel/msm/+/android-7.1.0_r0.2/drivers/usb/phy/phy-msm-usb.c#664 downstream PHY init seq https://android.googlesource.com/kernel/msm/+/android-7.1.0_r0.2/arch/arm/boot/dts/qcom/mdm9607.dtsi#585 and downstream driver PHY init write https://android.googlesource.com/kernel/msm/+/android-7.1.0_r0.2/drivers/usb/phy/phy-msm-usb.c#664 Which does writel_relaxed(ULPI_RUN | ULPI_WRITE | ULPI_ADDR(reg) | ULPI_DATA(val), USB_ULPI_VIEWPORT); Yep, you're right, we can't do a simple writeb(csr + reg, val); for mdm9607 but, also the qcs404 => compat = "qcom,usb-hs-28nm-femtophy" is doing the right thing. Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
On Wed, Dec 14, 2022 at 11:37:32PM +0100, Marijn Suijten wrote: > This reverts commit 557a28811c7e0286d3816842032db5eb7bb5f156. > > This commit introduced an init sequence from downstream DT [1] in the > driver. As mentioned by the comment above the HSPHY_INIT_CFG macro for > this sequence: > > /* > * The macro is used to define an initialization sequence. Each tuple > * is meant to program 'value' into phy register at 'offset' with 'delay' > * in us followed. > */ > > Instead of corresponding to offsets into the phy register, the sequence > read by the downstream driver [2] is passed into ulpi_write [3] which > crafts the address-value pair into a new value and writes it into the > same register at USB_ULPI_VIEWPORT [4]. In other words, this init > sequence is programmed into the hardware in a totally different way than > downstream and is unlikely to achieve the desired result, if the hsphy > is working at all. > Thanks for sending this, I've also been meaning to do this for quite some time :) Reviewed-by: Stephan Gerhold <stephan@gerhold.net> > An alternative method needs to be found to write these init values at > the desired location. Fortunately mdm9607 did not land upstream yet [5] > and should have its compatible revised to use the generic one, instead > of a compatible that writes wrong data to the wrong registers. > There is a simple solution to write the init values correctly: You can just use the ULPI PHY driver for MSM8916 (phy-qcom-usb-hs.c), which writes those init values correctly. If you look closely at downstream you can see that all targets with the "SNPS Femto PHY" (at least MSM8909, MDM9607, MSM8976) actually use a mixture of the code currently implemented in the mainline ULPI PHY driver (phy-qcom-usb-hs.c) and the actual Femto PHY driver (phy-qcom-usb-hs-28nm.c): - The device trees contains "qcom,dp-manual-pullup", which enables the ULPI_MISC_A_VBUSVLDEXT magic that is currently implemented partly in phy-qcom-usb-hs.c and partly in ci_hdrc_msm.c: - Downstream: https://git.codelinaro.org/clo/la/kernel/msm-4.9/-/blob/29f81c613f4c853f4125ef189ede1f6d610653c0/drivers/usb/gadget/ci13xxx_msm.c#L113 - Mainline: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/qualcomm/phy-qcom-usb-hs.c?h=v6.1#n92 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/chipidea/ci_hdrc_msm.c?h=v6.1#n117 - The ULPI init sequence is also implemented by phy-qcom-usb-hs.c in mainline: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/qualcomm/phy-qcom-usb-hs.c?h=v6.1#n144 - The PHY CSR registers implemented by phy-qcom-usb-hs-28nm.c are only used to enter retention mode downstream (I guess some low-power mode where an USB device/host could still wake up the other host/device). We don't have support for proper support for retention on mainline (phy-qcom-usb-hs-28nm.c never enables the retention mode without fully powering off the PHY immediately after using the regulators). So I suggest that we simply use the ULPI PHY driver for MSM8916 at least for MSM8909 and MDM9607 (haven't checked MSM8976 in detail). I have tried it and it works without any problems on both MSM8909 and MDM9607. No driver changes needed! Thanks, Stephan
On 14-12-22, 23:37, Marijn Suijten wrote: > This reverts commit 557a28811c7e0286d3816842032db5eb7bb5f156. > > This commit introduced an init sequence from downstream DT [1] in the > driver. As mentioned by the comment above the HSPHY_INIT_CFG macro for > this sequence: > > /* > * The macro is used to define an initialization sequence. Each tuple > * is meant to program 'value' into phy register at 'offset' with 'delay' > * in us followed. > */ > > Instead of corresponding to offsets into the phy register, the sequence > read by the downstream driver [2] is passed into ulpi_write [3] which > crafts the address-value pair into a new value and writes it into the > same register at USB_ULPI_VIEWPORT [4]. In other words, this init > sequence is programmed into the hardware in a totally different way than > downstream and is unlikely to achieve the desired result, if the hsphy > is working at all. > > An alternative method needs to be found to write these init values at > the desired location. Fortunately mdm9607 did not land upstream yet [5] > and should have its compatible revised to use the generic one, instead > of a compatible that writes wrong data to the wrong registers. Applied after adding missing subsystem tag, thanks
On 2023-01-13 23:16:15, Vinod Koul wrote: > On 14-12-22, 23:37, Marijn Suijten wrote: > > This reverts commit 557a28811c7e0286d3816842032db5eb7bb5f156. > > > > This commit introduced an init sequence from downstream DT [1] in the > > driver. As mentioned by the comment above the HSPHY_INIT_CFG macro for > > this sequence: > > > > /* > > * The macro is used to define an initialization sequence. Each tuple > > * is meant to program 'value' into phy register at 'offset' with 'delay' > > * in us followed. > > */ > > > > Instead of corresponding to offsets into the phy register, the sequence > > read by the downstream driver [2] is passed into ulpi_write [3] which > > crafts the address-value pair into a new value and writes it into the > > same register at USB_ULPI_VIEWPORT [4]. In other words, this init > > sequence is programmed into the hardware in a totally different way than > > downstream and is unlikely to achieve the desired result, if the hsphy > > is working at all. > > > > An alternative method needs to be found to write these init values at > > the desired location. Fortunately mdm9607 did not land upstream yet [5] > > and should have its compatible revised to use the generic one, instead > > of a compatible that writes wrong data to the wrong registers. > > Applied after adding missing subsystem tag, thanks Thanks, it wasn't clear to me whether to suffix the title when already included in the Revert: "phy: qualcomm: ..." title :) - Marijn
On 16-01-23, 21:35, Marijn Suijten wrote: > On 2023-01-13 23:16:15, Vinod Koul wrote: > > On 14-12-22, 23:37, Marijn Suijten wrote: > > > This reverts commit 557a28811c7e0286d3816842032db5eb7bb5f156. > > > > > > This commit introduced an init sequence from downstream DT [1] in the > > > driver. As mentioned by the comment above the HSPHY_INIT_CFG macro for > > > this sequence: > > > > > > /* > > > * The macro is used to define an initialization sequence. Each tuple > > > * is meant to program 'value' into phy register at 'offset' with 'delay' > > > * in us followed. > > > */ > > > > > > Instead of corresponding to offsets into the phy register, the sequence > > > read by the downstream driver [2] is passed into ulpi_write [3] which > > > crafts the address-value pair into a new value and writes it into the > > > same register at USB_ULPI_VIEWPORT [4]. In other words, this init > > > sequence is programmed into the hardware in a totally different way than > > > downstream and is unlikely to achieve the desired result, if the hsphy > > > is working at all. > > > > > > An alternative method needs to be found to write these init values at > > > the desired location. Fortunately mdm9607 did not land upstream yet [5] > > > and should have its compatible revised to use the generic one, instead > > > of a compatible that writes wrong data to the wrong registers. > > > > Applied after adding missing subsystem tag, thanks > > Thanks, it wasn't clear to me whether to suffix the title when already > included in the Revert: "phy: qualcomm: ..." title :) A revert patch is a patch as well so the patch rules apply there as well, so should say "subsystem tag: other tags: Revert foo..." Thanks
On 2023-01-17 11:06:12, Vinod Koul wrote: <snip> > > Thanks, it wasn't clear to me whether to suffix the title when already > > included in the Revert: "phy: qualcomm: ..." title :) > > A revert patch is a patch as well so the patch rules apply there as well, > so should say "subsystem tag: other tags: Revert foo..." Ack, but then /keep/ "subsystem tag: other tags:" /within/ the Revert string, so "phy: qualcomm: Revert "phy: qualcomm: ...""? - Marijn
On 17-01-23, 10:19, Marijn Suijten wrote: > On 2023-01-17 11:06:12, Vinod Koul wrote: > <snip> > > > Thanks, it wasn't clear to me whether to suffix the title when already > > > included in the Revert: "phy: qualcomm: ..." title :) > > > > A revert patch is a patch as well so the patch rules apply there as well, > > so should say "subsystem tag: other tags: Revert foo..." > > Ack, but then /keep/ "subsystem tag: other tags:" /within/ the Revert > string, so "phy: qualcomm: Revert "phy: qualcomm: ...""? Ideally yes!
diff --git a/Documentation/devicetree/bindings/phy/qcom,usb-hs-28nm.yaml b/Documentation/devicetree/bindings/phy/qcom,usb-hs-28nm.yaml index abcc4373f39e..ca6a0836b53c 100644 --- a/Documentation/devicetree/bindings/phy/qcom,usb-hs-28nm.yaml +++ b/Documentation/devicetree/bindings/phy/qcom,usb-hs-28nm.yaml @@ -16,7 +16,6 @@ properties: compatible: enum: - qcom,usb-hs-28nm-femtophy - - qcom,usb-hs-28nm-mdm9607 reg: maxItems: 1 diff --git a/drivers/phy/qualcomm/phy-qcom-usb-hs-28nm.c b/drivers/phy/qualcomm/phy-qcom-usb-hs-28nm.c index 8807e59a1162..a52a9bf13b75 100644 --- a/drivers/phy/qualcomm/phy-qcom-usb-hs-28nm.c +++ b/drivers/phy/qualcomm/phy-qcom-usb-hs-28nm.c @@ -401,26 +401,13 @@ static const struct hsphy_init_seq init_seq_femtophy[] = { HSPHY_INIT_CFG(0x90, 0x60, 0), }; -static const struct hsphy_init_seq init_seq_mdm9607[] = { - HSPHY_INIT_CFG(0x80, 0x44, 0), - HSPHY_INIT_CFG(0x81, 0x38, 0), - HSPHY_INIT_CFG(0x82, 0x24, 0), - HSPHY_INIT_CFG(0x83, 0x13, 0), -}; - static const struct hsphy_data hsphy_data_femtophy = { .init_seq = init_seq_femtophy, .init_seq_num = ARRAY_SIZE(init_seq_femtophy), }; -static const struct hsphy_data hsphy_data_mdm9607 = { - .init_seq = init_seq_mdm9607, - .init_seq_num = ARRAY_SIZE(init_seq_mdm9607), -}; - static const struct of_device_id qcom_snps_hsphy_match[] = { { .compatible = "qcom,usb-hs-28nm-femtophy", .data = &hsphy_data_femtophy, }, - { .compatible = "qcom,usb-hs-28nm-mdm9607", .data = &hsphy_data_mdm9607, }, { }, }; MODULE_DEVICE_TABLE(of, qcom_snps_hsphy_match);
This reverts commit 557a28811c7e0286d3816842032db5eb7bb5f156. This commit introduced an init sequence from downstream DT [1] in the driver. As mentioned by the comment above the HSPHY_INIT_CFG macro for this sequence: /* * The macro is used to define an initialization sequence. Each tuple * is meant to program 'value' into phy register at 'offset' with 'delay' * in us followed. */ Instead of corresponding to offsets into the phy register, the sequence read by the downstream driver [2] is passed into ulpi_write [3] which crafts the address-value pair into a new value and writes it into the same register at USB_ULPI_VIEWPORT [4]. In other words, this init sequence is programmed into the hardware in a totally different way than downstream and is unlikely to achieve the desired result, if the hsphy is working at all. An alternative method needs to be found to write these init values at the desired location. Fortunately mdm9607 did not land upstream yet [5] and should have its compatible revised to use the generic one, instead of a compatible that writes wrong data to the wrong registers. [1]: https://android.googlesource.com/kernel/msm/+/android-7.1.0_r0.2/arch/arm/boot/dts/qcom/mdm9607.dtsi#585 [2]: https://android.googlesource.com/kernel/msm/+/android-7.1.0_r0.2/drivers/usb/phy/phy-msm-usb.c#4183 [3]: https://android.googlesource.com/kernel/msm/+/android-7.1.0_r0.2/drivers/usb/phy/phy-msm-usb.c#468 [4]: https://android.googlesource.com/kernel/msm/+/android-7.1.0_r0.2/drivers/usb/phy/phy-msm-usb.c#418 [5]: https://lore.kernel.org/linux-arm-msm/20210805222812.40731-1-konrad.dybcio@somainline.org/ Reported-by: Michael Srba <Michael.Srba@seznam.cz> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> --- .../devicetree/bindings/phy/qcom,usb-hs-28nm.yaml | 1 - drivers/phy/qualcomm/phy-qcom-usb-hs-28nm.c | 13 ------------- 2 files changed, 14 deletions(-)