diff mbox series

[4/5] PCI: Add quirk for multifunction devices of LS7A

Message ID 20210514080025.1828197-5-chenhuacai@loongson.cn (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series PCI: Loongson-related pci quirks | expand

Commit Message

Huacai Chen May 14, 2021, 8 a.m. UTC 
From: Jianmin Lv <lvjianmin@loongson.cn>

In LS7A, multifunction device use same pci PIN and different
irq for different function, so fix it for standard pci PIN
usage.

Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
---
 drivers/pci/quirks.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Comments

Krzysztof Wilczyński May 14, 2021, 1:22 p.m. UTC | #1 
Hi Jianmin,

[...]
> In LS7A, multifunction device use same pci PIN and different
> irq for different function, so fix it for standard pci PIN
> usage.

It would be "PCI" instead of "pci" and "IRQ" instead of "irq" in the
commit message.  This would also be true in the other patches in this
series.

[...]
> +	if (pci_match_id(devids, dev)) {
> +		u8 fun = dev->devfn & 7;

You could use the PCI_FUNC() macro here.

Krzysztof
Jiaxun Yang May 14, 2021, 2:52 p.m. UTC | #2 
在 2021/5/14 16:00, Huacai Chen 写道:
> From: Jianmin Lv <lvjianmin@loongson.cn>
>
> In LS7A, multifunction device use same pci PIN and different
> irq for different function, so fix it for standard pci PIN
> usage.

Hmm, I'm unsure about this change.
The PCIe port, or PCI-to-PCI bridge on LS7A only have a single
upstream interrupt specified in DeviceTree, how can this quirk
work?

Thanks.

- Jiaxun

>
> Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
> Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> ---
Bjorn Helgaas May 14, 2021, 3:51 p.m. UTC | #3 
On Fri, May 14, 2021 at 04:00:24PM +0800, Huacai Chen wrote:
> From: Jianmin Lv <lvjianmin@loongson.cn>
> 
> In LS7A, multifunction device use same pci PIN and different
> irq for different function, so fix it for standard pci PIN
> usage.

Apparently the defect here is that the Interrupt Pin register reports
the wrong INTx values?

Will this be fixed in future hardware so we don't have to add more
devices to the quirk?

> Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
> Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> ---
>  drivers/pci/quirks.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 10b3b2057940..6ab4b3bba36b 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -242,6 +242,23 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
>  			DEV_LS7A_LPC, loongson_system_bus_quirk);
>  
> +static void loongson_pci_pin_quirk(struct pci_dev *dev)
> +{
> +	static const struct pci_device_id devids[] = {
> +		{ PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_0) },
> +		{ PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_1) },
> +		{ PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_2) },
> +		{ 0, },
> +	};
> +
> +	if (pci_match_id(devids, dev)) {
> +		u8 fun = dev->devfn & 7;

Use PCI_FUNC().

> +		dev->pin = 1 + (fun & 3);
> +	}
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, PCI_ANY_ID, loongson_pci_pin_quirk);

The usual pattern is to list each device here instead of using
pci_match_id() in the quirk, e.g.,

  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, 0x7a09, loongson_pci_pin_quirk);
  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, 0x7a19, loongson_pci_pin_quirk);
  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, 0x7a29, loongson_pci_pin_quirk);

>  static void loongson_mrrs_quirk(struct pci_dev *dev)
>  {
>  	struct pci_bus *bus = dev->bus;
> -- 
> 2.27.0
>
Huacai Chen May 15, 2021, 3:52 a.m. UTC | #4 
Hi, Jiaxun,

On Fri, May 14, 2021 at 10:52 PM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
>
>
>
> 在 2021/5/14 16:00, Huacai Chen 写道:
> > From: Jianmin Lv <lvjianmin@loongson.cn>
> >
> > In LS7A, multifunction device use same pci PIN and different
> > irq for different function, so fix it for standard pci PIN
> > usage.
>
> Hmm, I'm unsure about this change.
> The PCIe port, or PCI-to-PCI bridge on LS7A only have a single
> upstream interrupt specified in DeviceTree, how can this quirk
> work?
LS7A will be shared by MIPS-based Loongson and LoongArch-based
Loongson, LoongArch use ACPI rather than FDT, so this quirk is needed.

Huacai
>
> Thanks.
>
> - Jiaxun
>
> >
> > Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
> > Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> > ---
>
Huacai Chen May 15, 2021, 3:56 a.m. UTC | #5 
Hi, Krzysztof and Bjorn

I will improve my spelling, others see below please.

On Fri, May 14, 2021 at 11:51 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, May 14, 2021 at 04:00:24PM +0800, Huacai Chen wrote:
> > From: Jianmin Lv <lvjianmin@loongson.cn>
> >
> > In LS7A, multifunction device use same pci PIN and different
> > irq for different function, so fix it for standard pci PIN
> > usage.
>
> Apparently the defect here is that the Interrupt Pin register reports
> the wrong INTx values?
In LS7A, the Pin register is hardcoded to the same value for all
functions in multi-function devices, so we need this.
>
> Will this be fixed in future hardware so we don't have to add more
> devices to the quirk?
This will be fixed in future hardware.

Huacai
>
> > Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
> > Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
> > ---
> >  drivers/pci/quirks.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index 10b3b2057940..6ab4b3bba36b 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -242,6 +242,23 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> >  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
> >                       DEV_LS7A_LPC, loongson_system_bus_quirk);
> >
> > +static void loongson_pci_pin_quirk(struct pci_dev *dev)
> > +{
> > +     static const struct pci_device_id devids[] = {
> > +             { PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_0) },
> > +             { PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_1) },
> > +             { PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_2) },
> > +             { 0, },
> > +     };
> > +
> > +     if (pci_match_id(devids, dev)) {
> > +             u8 fun = dev->devfn & 7;
>
> Use PCI_FUNC().
>
> > +             dev->pin = 1 + (fun & 3);
> > +     }
> > +}
> > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, PCI_ANY_ID, loongson_pci_pin_quirk);
>
> The usual pattern is to list each device here instead of using
> pci_match_id() in the quirk, e.g.,
>
>   DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, 0x7a09, loongson_pci_pin_quirk);
>   DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, 0x7a19, loongson_pci_pin_quirk);
>   DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, 0x7a29, loongson_pci_pin_quirk);
>
> >  static void loongson_mrrs_quirk(struct pci_dev *dev)
> >  {
> >       struct pci_bus *bus = dev->bus;
> > --
> > 2.27.0
> >
Bjorn Helgaas May 18, 2021, 1:59 p.m. UTC | #6 
[+cc Rob, beginning of thread
https://lore.kernel.org/r/20210514080025.1828197-5-chenhuacai@loongson.cn]

On Sat, May 15, 2021 at 11:52:53AM +0800, Huacai Chen wrote:
> On Fri, May 14, 2021 at 10:52 PM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
> > 在 2021/5/14 16:00, Huacai Chen 写道:
> > > From: Jianmin Lv <lvjianmin@loongson.cn>
> > >
> > > In LS7A, multifunction device use same pci PIN and different
> > > irq for different function, so fix it for standard pci PIN
> > > usage.
> >
> > Hmm, I'm unsure about this change.
> > The PCIe port, or PCI-to-PCI bridge on LS7A only have a single
> > upstream interrupt specified in DeviceTree, how can this quirk
> > work?
>
> LS7A will be shared by MIPS-based Loongson and LoongArch-based
> Loongson, LoongArch use ACPI rather than FDT, so this quirk is needed.

Can you expand on this a little bit?

Which DT binding are you referring to?  Is it in the Linux source
tree?

I think Linux reads Interrupt Pin for both FDT and ACPI systems, and
apparently that register contains the same value for all functions of
this multi-function device.

The quirk will be applied for both FDT and ACPI systems, but it sounds
like you're saying this is needed *because* LoongArch uses ACPI.

Bjorn
Huacai Chen May 19, 2021, 2:26 a.m. UTC | #7 
Hi, Bjorn,

On Tue, May 18, 2021 at 9:59 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Rob, beginning of thread
> https://lore.kernel.org/r/20210514080025.1828197-5-chenhuacai@loongson.cn]
>
> On Sat, May 15, 2021 at 11:52:53AM +0800, Huacai Chen wrote:
> > On Fri, May 14, 2021 at 10:52 PM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
> > > 在 2021/5/14 16:00, Huacai Chen 写道:
> > > > From: Jianmin Lv <lvjianmin@loongson.cn>
> > > >
> > > > In LS7A, multifunction device use same pci PIN and different
> > > > irq for different function, so fix it for standard pci PIN
> > > > usage.
> > >
> > > Hmm, I'm unsure about this change.
> > > The PCIe port, or PCI-to-PCI bridge on LS7A only have a single
> > > upstream interrupt specified in DeviceTree, how can this quirk
> > > work?
> >
> > LS7A will be shared by MIPS-based Loongson and LoongArch-based
> > Loongson, LoongArch use ACPI rather than FDT, so this quirk is needed.
>
> Can you expand on this a little bit?
>
> Which DT binding are you referring to?  Is it in the Linux source
> tree?
I referring to arch/mips/boot/dts/loongson/ls7a-pch.dtsi, it is
already in Linux source tree.

>
> I think Linux reads Interrupt Pin for both FDT and ACPI systems, and
> apparently that register contains the same value for all functions of
> this multi-function device.
>
> The quirk will be applied for both FDT and ACPI systems, but it sounds
> like you're saying this is needed *because* LoongArch uses ACPI.
Jiaxun said that FDT system doesn't need this quirk, maybe he know more.

Huacai
>
> Bjorn
Jiaxun Yang May 19, 2021, 3:05 a.m. UTC | #8 
在 2021/5/19 10:26, Huacai Chen 写道:
> Hi, Bjorn,
>
> On Tue, May 18, 2021 at 9:59 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>> [+cc Rob, beginning of thread
>> https://lore.kernel.org/r/20210514080025.1828197-5-chenhuacai@loongson.cn]
>>
>> On Sat, May 15, 2021 at 11:52:53AM +0800, Huacai Chen wrote:
>>> On Fri, May 14, 2021 at 10:52 PM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
>>>> 在 2021/5/14 16:00, Huacai Chen 写道:
>>>>> From: Jianmin Lv <lvjianmin@loongson.cn>
>>>>>
>>>>> In LS7A, multifunction device use same pci PIN and different
>>>>> irq for different function, so fix it for standard pci PIN
>>>>> usage.
>>>> Hmm, I'm unsure about this change.
>>>> The PCIe port, or PCI-to-PCI bridge on LS7A only have a single
>>>> upstream interrupt specified in DeviceTree, how can this quirk
>>>> work?
>>> LS7A will be shared by MIPS-based Loongson and LoongArch-based
>>> Loongson, LoongArch use ACPI rather than FDT, so this quirk is needed.
>> Can you expand on this a little bit?
>>
>> Which DT binding are you referring to?  Is it in the Linux source
>> tree?
> I referring to arch/mips/boot/dts/loongson/ls7a-pch.dtsi, it is
> already in Linux source tree.
>
>> I think Linux reads Interrupt Pin for both FDT and ACPI systems, and
>> apparently that register contains the same value for all functions of
>> this multi-function device.
>>
>> The quirk will be applied for both FDT and ACPI systems, but it sounds
>> like you're saying this is needed *because* LoongArch uses ACPI.
> Jiaxun said that FDT system doesn't need this quirk, maybe he know more.

For FDT system, the IRQ routine of PCI ports looks like:

                 interrupt-map-mask = <0 0 0 0>;
                 interrupt-map = <0 0 0 0 &pic 41 IRQ_TYPE_LEVEL_HIGH>;

It means they are all going to the same parent IRQ no matter what we read
from pin register. And it's the actual hardware behavior AFAIK.

Thanks.

- Jiaxun

>
> Huacai
>> Bjorn
diff mbox series

Patch

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 10b3b2057940..6ab4b3bba36b 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -242,6 +242,23 @@  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_LOONGSON,
 			DEV_LS7A_LPC, loongson_system_bus_quirk);
 
+static void loongson_pci_pin_quirk(struct pci_dev *dev)
+{
+	static const struct pci_device_id devids[] = {
+		{ PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_0) },
+		{ PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_1) },
+		{ PCI_VDEVICE(LOONGSON, DEV_PCIE_PORT_2) },
+		{ 0, },
+	};
+
+	if (pci_match_id(devids, dev)) {
+		u8 fun = dev->devfn & 7;
+
+		dev->pin = 1 + (fun & 3);
+	}
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON, PCI_ANY_ID, loongson_pci_pin_quirk);
+
 static void loongson_mrrs_quirk(struct pci_dev *dev)
 {
 	struct pci_bus *bus = dev->bus;