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
