Messages in this thread | | | Date | Thu, 29 Sep 2022 10:56:11 +0300 | Subject | Re: [PATCH 06/13] phy: qcom-qmp-pcie: drop bogus register update | From | Dmitry Baryshkov <> |
| |
On 29/09/2022 10:12, Johan Hovold wrote: > On Wed, Sep 28, 2022 at 10:48:40PM +0300, Dmitry Baryshkov wrote: >> On 28/09/2022 22:10, Dmitry Baryshkov wrote: >>> On 28/09/2022 18:28, Johan Hovold wrote: >>>> Since commit 0d58280cf1e6 ("phy: Update PHY power control sequence") the >>>> PHY is powered on before configuring the registers and only the MSM8996 >>>> PCIe PHY, which includes the POWER_DOWN_CONTROL register in its PCS >>>> initialisation table, may possibly require a second update afterwards. >>>> >>>> To make things worse, the POWER_DOWN_CONTROL register lies at a >>>> different offset on more recent SoCs so that the second update, which >>>> still used a hard-coded offset, would write to an unrelated register >>>> (e.g. a revision-id register on SC8280XP). >>>> >>>> As the MSM8996 PCIe PHY is now handled by a separate driver, simply drop >>>> the bogus register update. >>>> >>>> Fixes: e4d8b05ad5f9 ("phy: qcom-qmp: Use proper PWRDOWN offset for >>>> sm8150 USB") added support >>> >>> I'm not sure about the particular fixes tag. Backporting from the split >>> driver into old qmp driver would be a complete pain. >>> >>>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org> >>> >>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> >> After digging some more, I stumbled upon the commit 0d58280cf1e6 ("phy: >> Update PHY power control sequence"), which puts explicit register write >> here, telling that 'PCIe PHYs need an extra power control before >> deasserts reset state'. > > That's the commit I'm referring to above. > >> I can confirm this with the register tables from downstream dtsi. E.g. >> consider sdm845-pcie.dts, pcie0 table. The PCS_POWER_DOWN_CONTROL is the >> register 0x804. >> >> The programmings starts with <0x804 0x1 0x0>, writing 1 to >> PCS_POWER_DOWN_CONTROL (which if I'm not mistaken we do not do at this >> moment). Then after writing all the serdes/tx/rx/pcs/pcs_misc tables >> comes the write <0x804 0x3 0x0> (which you are trying to remove here). > > The PHY would already have been powered on with the mainline driver, that > write has already happened. > > Whether or not PCIe support for SDM845 has been broken since it was > first mainlined almost three years ago is a separate issue. I assume > Bjorn tested it before sending it upstream. > > 421c9a0e9731 ("phy: qcom: qmp: Add SDM845 PCIe QMP PHY support")
On SDM845 PCIe0 is used for the WiFi, e.g. on the RB3 (dragonboard845) device. The PHY definitely works with the upstream kernels.
> >> Same sequence applies to the PCIe PHY on msm8998. >> >> Most newer PHYs have the expected sequence (of writing 0x3 to >> PCS_POWER_DOWN_CONTROL) before writing all registers. >> >> As a short summary: unless we get any additional information that 8998 >> and sdm845 tables are incorrect, I'd suggest adding a conditional here >> (ugh) and using it here and in qmp_pcie_init() call. > > I see little point in doing that unless you dig out an SDM845, confirm > that it has never worked with upstream, and update the init sequence > first.
Digging out an sdm845 is not a problem per se. It works, but it also has an additional regwrite that this patch tries to remove. I will try checking whether it works with this register write being removed or not.
> >> Vinod, Bjorn, do you have any additional info? > > An explanation of the split POWER_DOWN_CONTROL updates on MSM8996 would > be good to have either way
-- With best wishes Dmitry
| |