[lkml]   [2018]   [Mar]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH v5 2/2] PCI: mediatek: Set up class type and vendor ID for MT7622
On Wed, Dec 27, 2017 at 12:45:42PM -0600, Bjorn Helgaas wrote:


> > + /* Set up class code for MT7622 */
> > + val = PCI_CLASS_BRIDGE_PCI << 16;
> > + writel(val, port->base + PCIE_CONF_CLASS);
> 1) Your comments mention MT7622 specifically, but this code is run for
> both mt2712-pcie and mt7622-pcie. If this code is safe and necessary
> for both mt2712-pcie and mt7622-pcie, please remove the mention of
> MT7622.
> 2) The first comment mentions both "vendor ID and device ID" but you
> don't write the device ID. Since this code applies to both
> mt2712-pcie and mt7622-pcie, my guess is that you don't *want* to
> write the device ID. If that's the case, please fix the comment.
> 3) If you only need to set the vendor ID, you're performing a 32-bit
> write (writel()) to update a 16-bit value. Please use writew()
> instead.
> 4) If you only need to set the vendor ID, please use a definition like
> "PCIE_CONF_VENDOR_ID" instead of the ambiguous "PCIE_CONF_ID".
> 5) If you only need to set the vendor ID, please update the changelog
> to mention "vendor ID" specifically instead of the ambiguous "IDs".
> 6) Please add a space before the closing "*/" of the first comment.
> 7) PCI_CLASS_BRIDGE_PCI is for a PCI-to-PCI bridge, i.e., one that has
> PCI on both the primary (upstream) side and the secondary (downstream)
> side. That kind of bridge has a type 1 config header (see
> registers tell us the bus number of the primary and secondary sides.
> I don't believe this device is a PCI-to-PCI bridge. I think it's a
> *host* bridge that has some non-PCI interface on the upstream side and
> should have a type 0 config header. If that's the case you should use

I think these registers actually programme the root port register space
in the RC (whether real or fake - that depends on the RC design) so
the class is PCI_CLASS_BRIDGE_PCI, that's what most of host bridge
drivers in drivers/pci/host do.

I would like to get to the bottom of this since it is indeed misleading
(and I do not have HW to test it myself to check my understanding).


 \ /
  Last update: 2018-03-16 13:14    [W:0.096 / U:1.516 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site