On Thu, 2017-12-21 at 18:27 -0600, Bjorn Helgaas wrote:
> On Thu, Dec 21, 2017 at 03:01:12PM +0800, Honghui Zhang wrote:
> > On Thu, 2017-12-21 at 14:41 +0800, Yong Wu wrote:
> > > On Thu, 2017-12-21 at 10:11 +0800, honghui.zhang@xxxxxxxxxxxx wrote:
> > > > From: Honghui Zhang <honghui.zhang@xxxxxxxxxxxx>
> > > >
> > > > The host bridge of MT7622 has hardware code the class code to an
> > > > arbitrary, meaningless value, fix that.
> > > >
> > > > Signed-off-by: Honghui Zhang <honghui.zhang@xxxxxxxxxxxx>
> > > > ---
> > > > drivers/pci/host/pcie-mediatek.c | 12 ++++++++++++
> > > > 1 file changed, 12 insertions(+)
> > > >
> > > > diff --git a/drivers/pci/host/pcie-mediatek.c b/drivers/pci/host/pcie-mediatek.c
> > > > index 3248771..ae8d367 100644
> > > > --- a/drivers/pci/host/pcie-mediatek.c
> > > > +++ b/drivers/pci/host/pcie-mediatek.c
> > > > @@ -1174,3 +1174,15 @@ static struct platform_driver mtk_pcie_driver = {
> > > > },
> > > > };
> > > > builtin_platform_driver(mtk_pcie_driver);
> > > > +
> > > > +/* The host bridge of MT7622 advertises the wrong device class. */
> > > > +static void mtk_fixup_class(struct pci_dev *dev)
> > > > +{
> > > > + dev->class = PCI_CLASS_BRIDGE_PCI << 8;
> > > > +}
> > > > +
> > > > +/*
> > > > + * The HW default value of vendor id and device id for mt7622 are 0x0e8d,
> > > > + * 0x3258, which are arbitrary, meaningless values.
>
> They may be arbitrary but they are certainly not meaningless.
>
> > > What's the right vendor id and device id? is it possible to fix them
> > > too?
> >
> > Vendor ID is managed by PCI-SIG, you may get the assigned vendor ID
> > from:
> > https://pci-ids.ucw.cz/read/PC?restrict=
>
> It's true that Vendor IDs are managed by the PCI-SIG. The link above
> is not managed by the PCI-SIG and is not the official list of assigned
> Vendor IDs.
>
> > The vendor ID for Mediatek Corp. should be 14c3.
> > Device ID is something like vendor-defined.
> > Those values are in the configuration space and are read-only defined by
> > spec, it's been stored at the pci_dev, we may change the vendor and
> > device values in pci_dev, but I don't think it's necessary to change
> > that.
> > BTW, Does anyone really cares about the vendor ID and device ID except
> > the device's driver?
>
> Yes. This is a fundamental misunderstanding of how Vendor and Device
> IDs work. The *driver* really doesn't care about the IDs. The PCI
> *core* cares about them because it uses the IDs to select the
> appropriate driver to bind to the device.
>
Thanks, I was wrong about this, I had not seen that the Vendor IDs may
be assigned to another company in the future. I should try another way
to fix this.
> > > > +DECLARE_PCI_FIXUP_EARLY(0x0e8d, 0x3258, mtk_fixup_class);
>
> This is absolutely not OK. You cannot take over Vendor ID 0x0e8d
> unless it has been assigned to you by the PCI-SIG.
>
> To my knowledge, 0x0e8d has not been assigned to any company yet, but
> the PCI-SIG could assign it to some new company X tomorrow. Company X
> may then build a device with Device ID 0x3258. Now we cannot tell the
> difference between the Company X device that is correctly designed and
> this Mediatek device that is broken.
>
> This quirk would improperly overwrite dev->class for the Company X
> device, and we would not be able to bind the correct driver to either
> device.
>
I will try another way to fix this, thanks very much for your explain.
> I am amazed that somebody could build a device that claims to be a PCI
> device and get the Vendor ID wrong. That should be literally the
> first thing the hardware designer does. If you have hardware in the
> field that uses the wrong Vendor ID, it is definitely not
> PCI-compliant.
There's an internal control register that control the Vendor ID and
device ID values, our designer leave the default value un-touched. I
will set these values in that way for the next version of fix.
Thanks.
>
> Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
