| Message ID | 20230123120807.3101313-1-dmitry.baryshkov@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | phy: qcom-qmp-combo: correct DP register offsets | expand |
On Mon, Jan 23, 2023 at 08:59:48PM +0200, Dmitry Baryshkov wrote: > On Mon, 23 Jan 2023 at 16:00, Johan Hovold <johan@kernel.org> wrote: > > > > On Mon, Jan 23, 2023 at 02:08:07PM +0200, Dmitry Baryshkov wrote: > > > Correct DP register offsets used with new DT bindings scheme. First, DP > > > TX registers reside in separate regions, not in the same regions as USB > > > TX registers do. Second, correct DP_PHY region offset to follow the > > > offset used for earlier generations/bindings. > > > > No, this patch is broken. SC8280XP is different, doesn't seem to have > > separate DP TX regions and the DP_PHY registers lies at a different > > offset than on previous generations. > > > > You can't just pull these numbers out of your ... ;) > > > > This is the only platform that I can test the DP part on and it is > > working. If that happens to be by chance, then you need to blame the > > commit adding support for sc8280xp (i.e. not the one that fixed the > > binding). And note that this was added by Bjorn who do have access to > > the documentation for this SoC (as well as actual hardware). > > Ack, let's wait for Bjorn to check the offsets. I find it extremely > suspicious that dp_txa/txb use the same region as usb txa/txb do. Yeah, I agree, it seems odd, but then again the sc8280xp PHY is a different beast which also supports USB4, etc. The DP_PHY offset comes from the header file in the vendor tree and there are no separate DP TX registers there. By the way, I forgot that Bjorn actually copy-pasted the devicetree nodes from an earlier platform when first adding the PHY and only later realised that the DP part did not work. The fix for that (e.g. to use the same TX registers) was never merged to mainline (instead we temporarily disabled the DP part) so the Fixes tag you used would have been correct if this turns out to be wrong. > > > Cc: Johan Hovold <johan+linaro@kernel.org> > > > Fixes: 83a0bbe39b17 ("phy: qcom-qmp-combo: add support for updated sc8280xp binding") > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Johan
On 24/01/2023 09:35, Johan Hovold wrote: > On Mon, Jan 23, 2023 at 08:59:48PM +0200, Dmitry Baryshkov wrote: >> On Mon, 23 Jan 2023 at 16:00, Johan Hovold <johan@kernel.org> wrote: >>> >>> On Mon, Jan 23, 2023 at 02:08:07PM +0200, Dmitry Baryshkov wrote: >>>> Correct DP register offsets used with new DT bindings scheme. First, DP >>>> TX registers reside in separate regions, not in the same regions as USB >>>> TX registers do. Second, correct DP_PHY region offset to follow the >>>> offset used for earlier generations/bindings. >>> >>> No, this patch is broken. SC8280XP is different, doesn't seem to have >>> separate DP TX regions and the DP_PHY registers lies at a different >>> offset than on previous generations. >>> >>> You can't just pull these numbers out of your ... ;) >>> >>> This is the only platform that I can test the DP part on and it is >>> working. If that happens to be by chance, then you need to blame the >>> commit adding support for sc8280xp (i.e. not the one that fixed the >>> binding). And note that this was added by Bjorn who do have access to >>> the documentation for this SoC (as well as actual hardware). >> >> Ack, let's wait for Bjorn to check the offsets. I find it extremely >> suspicious that dp_txa/txb use the same region as usb txa/txb do. > > Yeah, I agree, it seems odd, but then again the sc8280xp PHY is a > different beast which also supports USB4, etc. The DP_PHY offset comes > from the header file in the vendor tree and there are no separate DP TX > registers there. > > By the way, I forgot that Bjorn actually copy-pasted the devicetree > nodes from an earlier platform when first adding the PHY and only later > realised that the DP part did not work. The fix for that (e.g. to use > the same TX registers) was never merged to mainline (instead we > temporarily disabled the DP part) so the Fixes tag you used would have > been correct if this turns out to be wrong. Let's check with Bjorn. However even if the original offsets are correct, I think we should add dp_txa/dp_txb offsets (and make them equal to txa/txb in sc8280xp case). WDYT? > >>>> Cc: Johan Hovold <johan+linaro@kernel.org> >>>> Fixes: 83a0bbe39b17 ("phy: qcom-qmp-combo: add support for updated sc8280xp binding") >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > Johan
On Tue, Jan 24, 2023 at 11:19:32AM +0200, Dmitry Baryshkov wrote: > Let's check with Bjorn. However even if the original offsets are > correct, I think we should add dp_txa/dp_txb offsets (and make them > equal to txa/txb in sc8280xp case). WDYT? That is already being done as part of Luca's series: https://lore.kernel.org/lkml/20230120-sm6350-usbphy-v4-2-4d700a90ba16@fairphone.com/ Johan
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c index 1f022e580407..c6634f92de19 100644 --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c @@ -807,6 +807,8 @@ struct qmp_combo_offsets { u16 usb3_pcs; u16 usb3_pcs_usb; u16 dp_serdes; + u16 dp_txa; + u16 dp_txb; u16 dp_dp_phy; }; @@ -984,7 +986,9 @@ static const struct qmp_combo_offsets qmp_combo_offsets_v5 = { .usb3_pcs = 0x1400, .usb3_pcs_usb = 0x1700, .dp_serdes = 0x2000, - .dp_dp_phy = 0x2200, + .dp_txa = 0x2200, + .dp_txa = 0x2600, + .dp_dp_phy = 0x2a00, }; static const struct qmp_phy_cfg sc7180_usb3dpphy_cfg = { @@ -2639,8 +2643,8 @@ static int qmp_combo_parse_dt(struct qmp_combo *qmp) qmp->pcs_usb = base + offs->usb3_pcs_usb; qmp->dp_serdes = base + offs->dp_serdes; - qmp->dp_tx = base + offs->txa; - qmp->dp_tx2 = base + offs->txb; + qmp->dp_tx = base + offs->dp_txa; + qmp->dp_tx2 = base + offs->dp_txb; qmp->dp_dp_phy = base + offs->dp_dp_phy; qmp->pipe_clk = devm_clk_get(dev, "usb3_pipe");
Correct DP register offsets used with new DT bindings scheme. First, DP TX registers reside in separate regions, not in the same regions as USB TX registers do. Second, correct DP_PHY region offset to follow the offset used for earlier generations/bindings. Cc: Johan Hovold <johan+linaro@kernel.org> Fixes: 83a0bbe39b17 ("phy: qcom-qmp-combo: add support for updated sc8280xp binding") Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)