Re: [PATCH 06/13] phy: qcom-qmp-pcie: drop bogus register update

[Johan Hovold]

On Thu, Sep 29, 2022 at 10:56:11AM +0300, Dmitry Baryshkov wrote:
> 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:

> >> 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.

Ok, good.

> >> 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.

Right, but as I already mentioned that register already holds the same
value (0x3) when the second redundant write happens. So this should be a
no-op on sdm845 currently.

(Only MSM8996 had an intermediate update of that register in its PCS
table and therefore needs this second update.)

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

Johan