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
