[PATCH] PCI: mobiveil: Modified the Device tree bindings interrupt-map example

[PATCH] PCI: mobiveil: Modified the Device tree bindings interrupt-map example

[m.karthikeyan]

From: Karthikeyan Mitran <m.karthikeyan@mobiveil.co.in>

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
 - 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>;
 		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.

Re: [PATCH] PCI: mobiveil: Modified the Device tree bindings interrupt-map example

[Bjorn Helgaas]

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.

>  		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.

Re: [PATCH] PCI: mobiveil: Modified the Device tree bindings interrupt-map example

[Andrew Murray]

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.

Re: [PATCH] PCI: mobiveil: Modified the Device tree bindings interrupt-map example

[Lorenzo Pieralisi]

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.