On Tue, Oct 29, 2019 at 05:40:55PM -0500, Bjorn Helgaas wrote: > On Tue, Oct 29, 2019 at 09:23:42PM +0530, m.karthikeyan@xxxxxxxxxxxxxx wrote: > > From: Karthikeyan Mitran <m.karthikeyan@xxxxxxxxxxxxxx> > > *All* patches modify something, so the subject line isn't very > informative. I think you're actually fixing a bug: > > > - interrupt-map = <0 0 0 0 &pci_express 0>, > > + interrupt-map = <0 0 0 1 &pci_express 0>, > > and *that* should be clear in the subject. Maybe something like: > > dt-bindings: PCI: mobiveil: Correct INTx mapping > > I don't know the implications of this for backwards compatibility. > > > Legacy IRQs Interrupt pins map 01h, 02h, 03h, and 04h while value of 00h > > indicates Function uses no legacy interrupt Message > > > > Signed-off-by: Karthikeyan Mitran <m.karthikeyan@xxxxxxxxxxxxxx> > > --- > > .../devicetree/bindings/pci/mobiveil-pcie.txt | 19 ++++++++++++------- > > 1 file changed, 12 insertions(+), 7 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/pci/mobiveil-pcie.txt b/Documentation/devicetree/bindings/pci/mobiveil-pcie.txt > > index 64156993e05..b9dcb0ddc19 100644 > > --- a/Documentation/devicetree/bindings/pci/mobiveil-pcie.txt > > +++ b/Documentation/devicetree/bindings/pci/mobiveil-pcie.txt > > @@ -31,9 +31,14 @@ Required properties: > > - interrupts: The interrupt line of the PCIe controller > > last cell of this field is set to 4 to > > denote it as IRQ_TYPE_LEVEL_HIGH type interrupt. > > -- interrupt-map-mask, > > - interrupt-map: standard PCI properties to define the mapping of the > > - PCI interface to interrupt numbers. > > +- interrupt-map-mask: > > + Its a 4-tuple like structure denoting phys.hi, phys.mid, > > + phys.low and interrupt-cell > > +- interrupt-map: standard PCI properties to define the mapping of the > > + PCI interface to interrupt numbers. Here the first 4-tuple > > + are represented similar to interrupt-map-mask representation > > + while the next fields represents Interrupt controller phandle > > + and its #interrupt-cells fields > > The original text was basically the same as all the other bindings, so > I don't really see the point of changing this to be different from all > the rest. > > A few (mediatek, nvidia) refer to the "standard PCI bus binding > document" for more details. > > Maybe there should be a common place in the Linux source for > describing these "standard properties" so it's not repeated > everywhere? > > > - ranges: ranges for the PCI memory regions (I/O space region is not > > supported by hardware) > > Please refer to the standard PCI bus binding document for a more > > @@ -63,10 +68,10 @@ Example: > > #interrupt-cells = <1>; > > interrupts = < 0 89 4 >; > > interrupt-map-mask = <0 0 0 7>; > > - interrupt-map = <0 0 0 0 &pci_express 0>, > > - <0 0 0 1 &pci_express 1>, > > - <0 0 0 2 &pci_express 2>, > > - <0 0 0 3 &pci_express 3>; > > + interrupt-map = <0 0 0 1 &pci_express 0>, > > + <0 0 0 2 &pci_express 1>, > > + <0 0 0 3 &pci_express 2>, > > + <0 0 0 4 &pci_express 3>; > > Above you say the first 4-tuple in interrupt-map is similar to > interrupt-map-mask, but these all look the same and they don't look > like interrupt-map-mask. > > Oh, I guess you mean the "0 0 0 1" is a 4-tuple and the > "&pci_express 0" part is the "next fields". I would have called that > a 6-tuple. But I'm not a DT person, so maybe I just don't know the > terminology. I guess what happened here is as assumption that INTA starts at 0 instead of 1. I think we can prevent this type of error from happening and improve the clarity of the DT by adding a #define, e.g.: --- a/include/dt-bindings/interrupt-controller/irq.h +++ b/include/dt-bindings/interrupt-controller/irq.h @@ -17,4 +17,9 @@ #define IRQ_TYPE_LEVEL_HIGH 4 #define IRQ_TYPE_LEVEL_LOW 8 +#define IRQ_INTA 1 +#define IRQ_INTB 2 +#define IRQ_INTC 3 +#define IRQ_INTD 4 + #endif Which would refactor the DT files as: > > - interrupt-map = <0 0 0 0 &pci_express 0>, > > - <0 0 0 1 &pci_express 1>, > > - <0 0 0 2 &pci_express 2>, > > - <0 0 0 3 &pci_express 3>; > > + interrupt-map = <0 0 0 IRQ_INTA &pci_express 0>, > > + <0 0 0 IRQ_INTB &pci_express 1>, > > + <0 0 0 IRQ_INTC &pci_express 2>, > > + <0 0 0 IRQ_INTD &pci_express 3>; Would this make sense? I'll put together a patchset and put it on the list. Thanks, Andrew Murray > > > ranges = < 0x83000000 0 0x00000000 0xa8000000 0 0x8000000>; > > > > }; > > -- > > 2.17.1 > > > > > > -- > > Mobiveil INC., CONFIDENTIALITY NOTICE: This e-mail message, including any > > attachments, is for the sole use of the intended recipient(s) and may > > contain proprietary confidential or privileged information or otherwise be > > protected by law. Any unauthorized review, use, disclosure or distribution > > is prohibited. If you are not the intended recipient, please notify the > > sender and destroy all copies and the original message. > > You should try to avoid confidentiality notices like this in email to > the public mailing lists. I don't know whether we could apply a patch > with this notice on it or not.
