diff mbox series

Revert "phy: qualcomm: usb28nm: Add MDM9607 init sequence"

Message ID 20221214223733.648167-1-marijn.suijten@somainline.org (mailing list archive)
State Not Applicable
Headers show
Series Revert "phy: qualcomm: usb28nm: Add MDM9607 init sequence" | expand

Commit Message

Marijn Suijten Dec. 14, 2022, 10:37 p.m. UTC
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(-)

Comments

Bryan O'Donoghue Dec. 15, 2022, 1:51 a.m. UTC | #1
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>
Vinod Koul Jan. 13, 2023, 5:46 p.m. UTC | #2
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
Marijn Suijten Jan. 16, 2023, 8:35 p.m. UTC | #3
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
Vinod Koul Jan. 17, 2023, 5:36 a.m. UTC | #4
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
Marijn Suijten Jan. 17, 2023, 9:19 a.m. UTC | #5
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
Vinod Koul Jan. 18, 2023, 4:55 p.m. UTC | #6
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 mbox series

Patch

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);