Re: [PATCH 0/2] PCI: amlogic: Make PCIe working reliably on AXG platforms

From: Remi Pommarel
Date: Sun Dec 15 2019 - 06:28:43 EST


On Mon, Dec 09, 2019 at 09:32:18AM +0100, Jerome Brunet wrote:
>
> On Sun 08 Dec 2019 at 22:03, Remi Pommarel <repk@xxxxxxxxxxxx> wrote:
>
> > PCIe device probing failures have been seen on some AXG platforms and were
> > due to unreliable clock signal output. Setting HHI_MIPI_CNTL0[26] bit
> > solved the problem. After being contacted about this, vendor reported that
> > this bit was linked to PCIe PLL CML output.
>
> Thanks for reporting the problem.
>
> As Martin pointed out, the CML outputs already exist in the AXG clock
> controller but are handled using HHI_PCIE_PLL_CNTL6. Although
> incomplete, it seems to be aligned with the datasheet I have (v0.9)
>
> According to the same document, HHI_MIPI_CNTL0 belong to the MIPI Phy.
> Unfortunately bit 26 is not documented
>
> AFAICT, the clock controller is not appropriate driver to deal with this
> register/bit
>

Regarding both @Martin's and your remark.

Unfortunately the documentation I have and vendor feedback are a bit
vague to me. I do agree that CLKID_PCIE_PLL_CML_ENABLE is not a proper
name for this bit because this register is MIPI related.

Here is the information I got from the vendor [1]. As you can see
HHI_MIPI_CNTL0[29] and HHI_MIPI_CNTL0[26] are related together, and
HHI_MIPI_CNTL0[29] is implemented in the clock controller as
axg_mipi_enable which is why I used this driver for HHI_MIPI_CNTL0[26].

So maybe I could rename this bit to something MIPI related ?

> >
> > This serie adds a way to set this bit through AXG clock gating logic.
> > Platforms having this kind of issue could make use of this gating by
> > applying a patch to their devicetree similar to:
> >
> > clocks = <&clkc CLKID_USB
> > &clkc CLKID_MIPI_ENABLE
> > &clkc CLKID_PCIE_A
> > - &clkc CLKID_PCIE_CML_EN0>;
> > + &clkc CLKID_PCIE_CML_EN0
> > + &clkc CLKID_PCIE_PLL_CML_ENABLE>;
> > clock-names = "pcie_general",
> > "pcie_mipi_en",
> > "pcie",
> > - "port";
> > + "port",
> > + "pll_cml_en";
> > resets = <&reset RESET_PCIE_PHY>,
> > <&reset RESET_PCIE_A>,
> > <&reset RESET_PCIE_APB>;
>
> A few remarks for your future patches:
>
> * You need to document any need binding you introduce:
> It means that there should have been a patch in
> Documentation/devicetree/... before using your newclock name in the
> pcie driver. As Martin pointed out, dt-bindings should be dealt with
> in their own patches
>
> >
> >
> > Remi Pommarel (2):
> > clk: meson: axg: add pcie pll cml gating
>
> Whenever possible, patches intended for different maintainers should be
> sent separately (different series)

Thanks, will do both of the above remarks.

>
> > PCI: amlogic: Use PCIe pll gate when available
> >
> > drivers/clk/meson/axg.c | 3 +++
> > drivers/clk/meson/axg.h | 2 +-
> > drivers/pci/controller/dwc/pci-meson.c | 5 +++++
> > include/dt-bindings/clock/axg-clkc.h | 1 +
> > 4 files changed, 10 insertions(+), 1 deletion(-)
>

Thanks for reviewing this.

[1] https://i.snipboard.io/bHMPeq.jpg
--
Remi