* Re: [PATCH] PCI: mobiveil: Modified the Device tree bindings interrupt-map example
2019-10-29 22:40 ` Bjorn Helgaas
@ 2019-11-01 16:10 ` Andrew Murray
2020-07-02 15:29 ` Lorenzo Pieralisi
1 sibling, 0 replies; 4+ messages in thread
From: Andrew Murray @ 2019-11-01 16:10 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: m.karthikeyan, linux-pci, lorenzo.pieralisi, mingkai.hu,
mark.rutland, minghuan.lian, zhiqiang.hou, l.subrahmanya
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@mobiveil.co.in wrote:
> > From: Karthikeyan Mitran <m.karthikeyan@mobiveil.co.in>
>
> *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@mobiveil.co.in>
> > ---
> > .../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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] PCI: mobiveil: Modified the Device tree bindings interrupt-map example
2019-10-29 22:40 ` Bjorn Helgaas
2019-11-01 16:10 ` Andrew Murray
@ 2020-07-02 15:29 ` Lorenzo Pieralisi
1 sibling, 0 replies; 4+ messages in thread
From: Lorenzo Pieralisi @ 2020-07-02 15:29 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: m.karthikeyan, linux-pci, mingkai.hu, mark.rutland, minghuan.lian,
zhiqiang.hou, l.subrahmanya
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@mobiveil.co.in wrote:
> > From: Karthikeyan Mitran <m.karthikeyan@mobiveil.co.in>
>
> *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.
Yes that has to be tested but nonetheless this binding is still
broken (and probably the driver was made to work with it so I
need to check it).
It is certain that the current binding can't work with a PCI device
requiring an INTD.
Lorenzo
>
> > 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@mobiveil.co.in>
> > ---
> > .../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.
>
> > 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.
^ permalink raw reply [flat|nested] 4+ messages in thread