Re: [PATCH v6 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver

From: Bjorn Helgaas <helgaas@kernel.org>
To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Rob Herring <robh@kernel.org>,
	Hanjie Lin <hanjie.lin@amlogic.com>,
	Jianxin Pan <jianxin.pan@amlogic.com>,
	linux-pci@vger.kernel.org, Kevin Hilman <khilman@baylibre.com>,
	Shawn Lin <shawn.lin@rock-chips.com>,
	Yixun Lan <yixun.lan@amlogic.com>,
	linux-kernel@vger.kernel.org, Yue Wang <yue.wang@amlogic.com>,
	Qiufang Dai <qiufang.dai@amlogic.com>,
	Jian Hu <jian.hu@amlogic.com>,
	Philippe Ombredanne <pombredanne@nexb.com>,
	Carlo Caione <carlo@caione.org>,
	Cyrille Pitchen <cyrille.pitchen@free-electrons.com>,
	linux-amlogic@lists.infradead.org,
	Gustavo Pimentel <gustavo.pimentel@synopsys.com>,
	Liang Yang <liang.yang@amlogic.com>,
	linux-arm-kernel@lists.infradead.org,
	Jerome Brunet <jbrunet@baylibre.com>
Subject: Re: [PATCH v6 2/2] PCI: amlogic: Add the Amlogic Meson PCIe controller driver
Date: Mon, 3 Dec 2018 16:57:31 -0600	[thread overview]
Message-ID: <20181203225731.GE207198@google.com> (raw)
In-Reply-To: <20181203164150.GA11855@e107981-ln.cambridge.arm.com>

On Mon, Dec 03, 2018 at 04:41:50PM +0000, Lorenzo Pieralisi wrote:
> On Thu, Nov 22, 2018 at 04:53:54PM +0800, Hanjie Lin wrote:
> 
> [...]
> 
> > +static int meson_pcie_rd_own_conf(struct pcie_port *pp, int where, int size,
> > +				  u32 *val)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +
> > +	/*
> > +	 * there is a bug of MESON AXG pcie controller that software can not
> > +	 * programe PCI_CLASS_DEVICE register, so we must return a fake right
> > +	 * value to ensure driver could probe successfully.
> > +	 */
> > +	if (where == PCI_CLASS_REVISION) {
> > +		*val = readl(pci->dbi_base + PCI_CLASS_REVISION);
> > +		/* keep revision id */
> > +		*val &= PCI_CLASS_REVISION_MASK;
> > +		*val |= PCI_CLASS_BRIDGE_PCI << 16;
> > +		return PCIBIOS_SUCCESSFUL;
> > +	}
> 
> As I said before, this looks broken. If this code (or other drivers with
> the same broken assumptions, eg dwc/pcie-qcom.c) carries out a, say,
> byte sized config access of eg PCI_CLASS_DEVICE you will get junk out of
> it according to your comment above.
> 
> I would like to pick Bjorn's brain on this to see what we can really do
> to fix this (and other) drivers.

  - Check to see whether you're reading anything in the 32-bit dword at
    offset 0x08.

  - Do the 32-bit readl().

  - Insert the correct Sub-Class and Base Class code (you also throw
    away the Programming Interface; not sure why that is)

  - If you're reading something smaller than 32 bits, mask & shift as
    needed.  pci_bridge_emul_conf_read() does something similar that
    you might be able to copy.

Out of curiosity, what code depends on PCI_CLASS_BRIDGE_PCI?  There
are several places in the kernel that currently depend on it, but I
think several of them *should* be checking dev->hdr_type to identify a
type 1 header instead.

Bjorn

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic