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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191215113634.GB7304@voidbox>
Date:   Sun, 15 Dec 2019 12:36:34 +0100
From:   Remi Pommarel <repk@...plefau.lt>
To:     Jerome Brunet <jbrunet@...libre.com>
Cc:     Neil Armstrong <narmstrong@...libre.com>,
        Kevin Hilman <khilman@...libre.com>,
        Yue Wang <yue.wang@...ogic.com>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        linux-amlogic@...ts.infradead.org, linux-clk@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-pci@...r.kernel.org
Subject: Re: [PATCH 0/2] PCI: amlogic: Make PCIe working reliably on AXG
 platforms

On Mon, Dec 09, 2019 at 09:32:18AM +0100, Jerome Brunet wrote:
> 
> On Sun 08 Dec 2019 at 22:03, Remi Pommarel <repk@...plefau.lt> 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