Re: [PATCH 1/4] PCI: qcom: Reshuffle reset logic in 2_7_0 .init

[Johan Hovold]

On Sat, Dec 30, 2023 at 02:16:18AM +0100, Konrad Dybcio wrote:
> On 29.12.2023 16:29, Johan Hovold wrote:
> > [ Again, please remember to add a newline before you inline comments to
> > make you replies readable. ]
> > 
> > On Fri, Dec 29, 2023 at 04:01:27PM +0100, Konrad Dybcio wrote:
> >> On 29.12.2023 15:04, Johan Hovold wrote:
> >>> On Wed, Dec 27, 2023 at 11:17:19PM +0100, Konrad Dybcio wrote:
> >>>> At least on SC8280XP, if the PCIe reset is asserted, the corresponding
> >>>> AUX_CLK will be stuck at 'off'.
> >>>
> >>> No, this path is exercised on every boot without the aux clock ever
> >>> being stuck at off. So something is clearly missing in this description.
> > 
> >> That's likely because the hardware has been initialized and not cleanly
> >> shut down by your bootloader. When you reset it, or your bootloader
> >> wasn't so kind, you need to start initialization from scratch.
> > 
> > What does that even mean? I'm telling you that this reset is asserted on
> > each boot, on all sc8280xp platforms I have access to, and never have I
> > seen the aux clk stuck at off.
> > 
> > So clearly your claim above is too broad and the commit message is
> > incorrect or incomplete.
> 
> diff --git a/drivers/clk/qcom/gcc-sc8280xp.c b/drivers/clk/qcom/gcc-sc8280xp.c
> index 0b7801971dc1..6650bd6af5e3 100644
> --- a/drivers/clk/qcom/gcc-sc8280xp.c
> +++ b/drivers/clk/qcom/gcc-sc8280xp.c
> @@ -7566,6 +7566,18 @@ static int gcc_sc8280xp_probe(struct platform_device *pdev)
>         if (ret)
>                 goto err_put_rpm;
>  
> +       int val;
> +       regmap_read(regmap, 0xa0000, &val);
> +       pr_err("GCC_PCIE_3A_BCR = 0x%x\n", val);
> +       regmap_read(regmap, 0xa00f0, &val);
> +       pr_err("GCC_PCIE_3A_LINK_DOWN_BCR = 0x%x\n", val);
> +       regmap_read(regmap, 0xa00fc, &val);
> +       pr_err("GCC_PCIE_3A_NOCSR_COM_PHY_BCR = 0x%x\n", val);
> +       regmap_read(regmap, 0xa00e0, &val);
> +       pr_err("GCC_PCIE_3A_PHY_BCR = 0x%x\n", val);
> +       regmap_read(regmap, 0xa00e4, &val);
> +       pr_err("GCC_PCIE_3A_PHY_NOCSR_COM_PHY_BCR = 0x%x\n", val);
> +
>         pm_runtime_put(&pdev->dev);
>  
>         return 0;
> 
> 
> [root@sc8280xp-crd ~]# dmesg | grep BCR
> [    2.500245] GCC_PCIE_3A_BCR = 0x0
> [    2.500250] GCC_PCIE_3A_LINK_DOWN_BCR = 0x0
> [    2.500253] GCC_PCIE_3A_NOCSR_COM_PHY_BCR = 0x0
> [    2.500255] GCC_PCIE_3A_PHY_BCR = 0x0
> [    2.500257] GCC_PCIE_3A_PHY_NOCSR_COM_PHY_BCR = 0x0
> 
> 
> 0 meaning "not asserted".

We're clearly talking past each other. When I'm saying reset is asserted
on each boot, I'm referring to reset being asserted in
qcom_pcie_init_2_7_0(), whereas you appear to be referring to whether
the reset has been left asserted by the bootloader when the driver
probes.

I understand what you meant to say now, but I think you should rephrase:

	At least on SC8280XP, if the PCIe reset is asserted, the
	corresponding AUX_CLK will be stuck at 'off'.

because as it stands, it sounds like the problem happens when the driver
asserts reset.

> PCIE3A is used for WLAN on the CRD, btw.

You meant to say WWAN (modem).

> >>>> Assert the reset (which may end up being a NOP if it was previously
> >>>> asserted) and de-assert it back *before* turning on the clocks to avoid
> >>>> such cases.
> >>>>
> >>>> In addition to that, in case the clock bulk enable fails, assert the
> >>>> RC reset back, as the hardware is in an unknown state at best.
> >>>
> >>> This is arguably a separate change, and not necessarily one that is
> >>> correct either
> > 
> >> If the clock enable fails, the PCIe hw is not in reset state, ergo it
> >> may be doing "something", and that "something" would eat non-zero power.
> >> It's just cleaning up after yourself.
> > 
> > How can it do something without power and clocks?
> 
> Fair point.
> 
> As far as power goes, the RC hangs off CX, which is on whenever the
> system is not in power collapse. As for clocks, at least parts of it
> use the crystal oscillator, not sure if directly.
> 
> > And leaving reset
> > asserted for non-powered devices is generally not a good idea.
> 
> Depends on the hw.

That's why I said "generally".

> > The commit message does not even mention suspend, it just makes a
> > clearly false general claim about a clock being stuck unless you reorder
> > things.
> 
> No, I insist that this general statement, while indeed lacking a full
> description of the problem, is provably true. The AUX clock will not
> turn on if the PCIe reset is asserted, at least on SC8280XP.

I agree, I was misinterpreting the commit message.

Johan