[1/4] dt-binding: irq: imx-irqsteer: use irq number per channel instead of group number
diff mbox series

Message ID 1547797668-30342-2-git-send-email-aisheng.dong@nxp.com
State New
Headers show
Series
  • irq: imx-irqsteer: add 32 interrupts chan and multi outputs support
Related show

Commit Message

Aisheng Dong Jan. 18, 2019, 7:53 a.m. UTC
Not all 64 interrupts may be used in one group. e.g. most irqsteer in
imx8qxp and imx8qm subsystems supports only 32 interrupts.

As the IP integration parameters are Channel number and interrupts number,
let's use fsl,irqs-per-chan to represents how many interrupts supported
by this irqsteer channel.

Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 .../devicetree/bindings/interrupt-controller/fsl,irqsteer.txt       | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Lucas Stach Jan. 18, 2019, 8:48 a.m. UTC | #1
Am Freitag, den 18.01.2019, 07:53 +0000 schrieb Aisheng Dong:
> Not all 64 interrupts may be used in one group. e.g. most irqsteer in
> imx8qxp and imx8qm subsystems supports only 32 interrupts.
> 
> As the IP integration parameters are Channel number and interrupts number,
> let's use fsl,irqs-per-chan to represents how many interrupts supported
> by this irqsteer channel.

Sorry, but total NACK. I've got to great lengths with dumping the
actually implemented register layout on i.MX8M and AFAICS the IRQs are
always managed in groups of 64 IRQs, even if less than that are
connected as input IRQs. This is what the actually present register set
on i.MX8M tells us.

Regards,
Lucas

> Cc: Marc Zyngier <marc.zyngier@arm.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Lucas Stach <l.stach@pengutronix.de>
> > Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: devicetree@vger.kernel.org
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>  .../devicetree/bindings/interrupt-controller/fsl,irqsteer.txt       | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> index 45790ce..eaabcda 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
> @@ -16,8 +16,8 @@ Required properties:
>  - #interrupt-cells: Specifies the number of cells needed to encode an
>    interrupt source. The value must be 1.
>  - fsl,channel: The output channel that all input IRQs should be steered into.
> -- fsl,irq-groups: Number of IRQ groups managed by this controller instance.
> -  Each group manages 64 input interrupts.
> +- fsl,irqs-per-chan: Number of input interrupts per channel. Should be multiple of 32
> +  input interrupts and up to 512 interrupts.
>  
>  Example:
>  
> @@ -28,7 +28,7 @@ Example:
> >  		clocks = <&clk IMX8MQ_CLK_DISP_APB_ROOT>;
> >  		clock-names = "ipg";
> >  		fsl,channel = <0>;
> > -		fsl,irq-groups = <1>;
> > +		fsl,irqs-per-chan= <64>;
> >  		interrupt-controller;
> >  		#interrupt-cells = <1>;
> >  	};
Marc Zyngier Jan. 18, 2019, 9:39 a.m. UTC | #2
On 18/01/2019 08:48, Lucas Stach wrote:
> Am Freitag, den 18.01.2019, 07:53 +0000 schrieb Aisheng Dong:
>> Not all 64 interrupts may be used in one group. e.g. most irqsteer in
>> imx8qxp and imx8qm subsystems supports only 32 interrupts.
>>
>> As the IP integration parameters are Channel number and interrupts number,
>> let's use fsl,irqs-per-chan to represents how many interrupts supported
>> by this irqsteer channel.
> 
> Sorry, but total NACK. I've got to great lengths with dumping the
> actually implemented register layout on i.MX8M and AFAICS the IRQs are
> always managed in groups of 64 IRQs, even if less than that are
> connected as input IRQs. This is what the actually present register set
> on i.MX8M tells us.

Also, I'd really like the DT bindings not to change at every release. So
whatever change (if any) has to be done for this driver to support
existing HW, please make sure that the DT bindings are kept as stable as
possible.

Thanks,

	M.
Aisheng Dong Jan. 18, 2019, 9:45 a.m. UTC | #3
> -----Original Message-----
> From: Lucas Stach [mailto:l.stach@pengutronix.de]
> Sent: Friday, January 18, 2019 4:48 PM
> 
> Am Freitag, den 18.01.2019, 07:53 +0000 schrieb Aisheng Dong:
> > Not all 64 interrupts may be used in one group. e.g. most irqsteer in
> > imx8qxp and imx8qm subsystems supports only 32 interrupts.
> >
> > As the IP integration parameters are Channel number and interrupts
> > number, let's use fsl,irqs-per-chan to represents how many interrupts
> > supported by this irqsteer channel.
> 
> Sorry, but total NACK. I've got to great lengths with dumping the actually
> implemented register layout on i.MX8M and AFAICS the IRQs are always
> managed in groups of 64 IRQs, even if less than that are connected as input
> IRQs. This is what the actually present register set on i.MX8M tells us.
> 

The register layout varies depends on the irq per channel supports.
It's true 64 IRQs on MX8M, but not true for QXP and QM.
That's why we shouldn't use fsl,irq-groups.

Regards
Dong Aisheng

> Regards,
> Lucas
> 
> > Cc: Marc Zyngier <marc.zyngier@arm.com>
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Cc: Lucas Stach <l.stach@pengutronix.de>
> > > Cc: Shawn Guo <shawnguo@kernel.org>
> > Cc: devicetree@vger.kernel.org
> > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> >  .../devicetree/bindings/interrupt-controller/fsl,irqsteer.txt       |
> > 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.
> > txt
> > b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.
> > txt
> > index 45790ce..eaabcda 100644
> > ---
> > a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.
> > txt
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqst
> > +++ eer.txt
> > @@ -16,8 +16,8 @@ Required properties:
> >  - #interrupt-cells: Specifies the number of cells needed to encode an
> >    interrupt source. The value must be 1.
> >  - fsl,channel: The output channel that all input IRQs should be steered into.
> > -- fsl,irq-groups: Number of IRQ groups managed by this controller instance.
> > -  Each group manages 64 input interrupts.
> > +- fsl,irqs-per-chan: Number of input interrupts per channel. Should
> > +be multiple of 32
> > +  input interrupts and up to 512 interrupts.
> >
> >  Example:
> >
> > @@ -28,7 +28,7 @@ Example:
> > >  		clocks = <&clk IMX8MQ_CLK_DISP_APB_ROOT>;
> > >  		clock-names = "ipg";
> > >  		fsl,channel = <0>;
> > > -		fsl,irq-groups = <1>;
> > > +		fsl,irqs-per-chan= <64>;
> > >  		interrupt-controller;
> > >  		#interrupt-cells = <1>;
> > >  	};
Aisheng Dong Jan. 18, 2019, 9:46 a.m. UTC | #4
> From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> Sent: Friday, January 18, 2019 5:39 PM
> On 18/01/2019 08:48, Lucas Stach wrote:
> > Am Freitag, den 18.01.2019, 07:53 +0000 schrieb Aisheng Dong:
> >> Not all 64 interrupts may be used in one group. e.g. most irqsteer in
> >> imx8qxp and imx8qm subsystems supports only 32 interrupts.
> >>
> >> As the IP integration parameters are Channel number and interrupts
> >> number, let's use fsl,irqs-per-chan to represents how many interrupts
> >> supported by this irqsteer channel.
> >
> > Sorry, but total NACK. I've got to great lengths with dumping the
> > actually implemented register layout on i.MX8M and AFAICS the IRQs are
> > always managed in groups of 64 IRQs, even if less than that are
> > connected as input IRQs. This is what the actually present register
> > set on i.MX8M tells us.
> 
> Also, I'd really like the DT bindings not to change at every release. So whatever
> change (if any) has to be done for this driver to support existing HW, please
> make sure that the DT bindings are kept as stable as possible.
> 

Sorry I should clarify it a bit.
There's still no users in Devicetree.
So I guess we can update it, right? Or not?

Regards
Dong Aisheng

> Thanks,
> 
> 	M.
> --
> Jazz is not dead. It just smells funny...
Marc Zyngier Jan. 18, 2019, 10:12 a.m. UTC | #5
On 18/01/2019 09:46, Aisheng Dong wrote:
>> From: Marc Zyngier [mailto:marc.zyngier@arm.com]
>> Sent: Friday, January 18, 2019 5:39 PM
>> On 18/01/2019 08:48, Lucas Stach wrote:
>>> Am Freitag, den 18.01.2019, 07:53 +0000 schrieb Aisheng Dong:
>>>> Not all 64 interrupts may be used in one group. e.g. most irqsteer in
>>>> imx8qxp and imx8qm subsystems supports only 32 interrupts.
>>>>
>>>> As the IP integration parameters are Channel number and interrupts
>>>> number, let's use fsl,irqs-per-chan to represents how many interrupts
>>>> supported by this irqsteer channel.
>>>
>>> Sorry, but total NACK. I've got to great lengths with dumping the
>>> actually implemented register layout on i.MX8M and AFAICS the IRQs are
>>> always managed in groups of 64 IRQs, even if less than that are
>>> connected as input IRQs. This is what the actually present register
>>> set on i.MX8M tells us.
>>
>> Also, I'd really like the DT bindings not to change at every release. So whatever
>> change (if any) has to be done for this driver to support existing HW, please
>> make sure that the DT bindings are kept as stable as possible.
>>
> 
> Sorry I should clarify it a bit.
> There's still no users in Devicetree.
> So I guess we can update it, right? Or not?

What do you mean by no users? This driver is in 5.0, and I assume people
are using it one way or another. Not having a platform in the kernel
tree is pretty much irrelevant, as the kernel tree is not a canonical
repository of existing platforms.

Thanks,

	M.
Aisheng Dong Jan. 22, 2019, 10:56 a.m. UTC | #6
> From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> Sent: Friday, January 18, 2019 6:12 PM
[...]
> >> From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> >> Sent: Friday, January 18, 2019 5:39 PM On 18/01/2019 08:48, Lucas
> >> Stach wrote:
> >>> Am Freitag, den 18.01.2019, 07:53 +0000 schrieb Aisheng Dong:
> >>>> Not all 64 interrupts may be used in one group. e.g. most irqsteer
> >>>> in imx8qxp and imx8qm subsystems supports only 32 interrupts.
> >>>>
> >>>> As the IP integration parameters are Channel number and interrupts
> >>>> number, let's use fsl,irqs-per-chan to represents how many
> >>>> interrupts supported by this irqsteer channel.
> >>>
> >>> Sorry, but total NACK. I've got to great lengths with dumping the
> >>> actually implemented register layout on i.MX8M and AFAICS the IRQs
> >>> are always managed in groups of 64 IRQs, even if less than that are
> >>> connected as input IRQs. This is what the actually present register
> >>> set on i.MX8M tells us.
> >>
> >> Also, I'd really like the DT bindings not to change at every release.
> >> So whatever change (if any) has to be done for this driver to support
> >> existing HW, please make sure that the DT bindings are kept as stable as
> possible.
> >>
> >
> > Sorry I should clarify it a bit.
> > There's still no users in Devicetree.
> > So I guess we can update it, right? Or not?
> 
> What do you mean by no users? This driver is in 5.0, and I assume people are
> using it one way or another. Not having a platform in the kernel tree is pretty
> much irrelevant, as the kernel tree is not a canonical repository of existing
> platforms.
> 

I understand the concern.
Theoretically yes, but it's very unlikely that there's already an out of tree users
wants to use it for a long term as we're still at the very initial stage.

And the most important reason is that current using actually is wrong.
We can also choose to mark it as 'depreciated' and keep the backward compatibility in driver,
but I'm not sure whether it's worthy to do it as we may add a lot ugly code in driver
benefits no users.

Ideas?

Regards
Dong Aisheng

> Thanks,
> 
> 	M.
> --
> Jazz is not dead. It just smells funny...
Lucas Stach Jan. 22, 2019, 11:05 a.m. UTC | #7
Am Dienstag, den 22.01.2019, 10:56 +0000 schrieb Aisheng Dong:
> > > > From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> > Sent: Friday, January 18, 2019 6:12 PM
> 
> [...]
> > > > > > > > From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> > > > Sent: Friday, January 18, 2019 5:39 PM On 18/01/2019 08:48, Lucas
> > > > Stach wrote:
> > > > > Am Freitag, den 18.01.2019, 07:53 +0000 schrieb Aisheng Dong:
> > > > > > Not all 64 interrupts may be used in one group. e.g. most irqsteer
> > > > > > in imx8qxp and imx8qm subsystems supports only 32 interrupts.
> > > > > > 
> > > > > > As the IP integration parameters are Channel number and interrupts
> > > > > > number, let's use fsl,irqs-per-chan to represents how many
> > > > > > interrupts supported by this irqsteer channel.
> > > > > 
> > > > > Sorry, but total NACK. I've got to great lengths with dumping the
> > > > > actually implemented register layout on i.MX8M and AFAICS the IRQs
> > > > > are always managed in groups of 64 IRQs, even if less than that are
> > > > > connected as input IRQs. This is what the actually present register
> > > > > set on i.MX8M tells us.
> > > > 
> > > > Also, I'd really like the DT bindings not to change at every release.
> > > > So whatever change (if any) has to be done for this driver to support
> > > > existing HW, please make sure that the DT bindings are kept as stable as
> > 
> > possible.
> > > > 
> > > 
> > > Sorry I should clarify it a bit.
> > > There's still no users in Devicetree.
> > > So I guess we can update it, right? Or not?
> > 
> > What do you mean by no users? This driver is in 5.0, and I assume people are
> > using it one way or another. Not having a platform in the kernel tree is pretty
> > much irrelevant, as the kernel tree is not a canonical repository of existing
> > platforms.
> > 
> 
> I understand the concern.
> Theoretically yes, but it's very unlikely that there's already an out of tree users
> wants to use it for a long term as we're still at the very initial stage.
> 
> And the most important reason is that current using actually is wrong.
> We can also choose to mark it as 'depreciated' and keep the backward compatibility in driver,
> but I'm not sure whether it's worthy to do it as we may add a lot ugly code in driver
> benefits no users.
> 
> Ideas?

I'm all for doing a breaking DT change now. The binding is
significantly different from the downstream one anyways and I'm not
aware of any upstream users that wouldn't be able to cope with a change
at this point.

I want to reach a conclusion on the discussion about how the HW
actually works and is configured in reply to Patch 4/4 first.

Regards,
Lucas
Marc Zyngier Jan. 22, 2019, 11:48 a.m. UTC | #8
On Tue, 22 Jan 2019 10:56:42 +0000,
Aisheng Dong <aisheng.dong@nxp.com> wrote:
> 
> > From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> > Sent: Friday, January 18, 2019 6:12 PM
> [...]
> > >> From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> > >> Sent: Friday, January 18, 2019 5:39 PM On 18/01/2019 08:48, Lucas
> > >> Stach wrote:
> > >>> Am Freitag, den 18.01.2019, 07:53 +0000 schrieb Aisheng Dong:
> > >>>> Not all 64 interrupts may be used in one group. e.g. most irqsteer
> > >>>> in imx8qxp and imx8qm subsystems supports only 32 interrupts.
> > >>>>
> > >>>> As the IP integration parameters are Channel number and interrupts
> > >>>> number, let's use fsl,irqs-per-chan to represents how many
> > >>>> interrupts supported by this irqsteer channel.
> > >>>
> > >>> Sorry, but total NACK. I've got to great lengths with dumping the
> > >>> actually implemented register layout on i.MX8M and AFAICS the IRQs
> > >>> are always managed in groups of 64 IRQs, even if less than that are
> > >>> connected as input IRQs. This is what the actually present register
> > >>> set on i.MX8M tells us.
> > >>
> > >> Also, I'd really like the DT bindings not to change at every release.
> > >> So whatever change (if any) has to be done for this driver to support
> > >> existing HW, please make sure that the DT bindings are kept as stable as
> > possible.
> > >>
> > >
> > > Sorry I should clarify it a bit.
> > > There's still no users in Devicetree.
> > > So I guess we can update it, right? Or not?
> > 
> > What do you mean by no users? This driver is in 5.0, and I assume people are
> > using it one way or another. Not having a platform in the kernel tree is pretty
> > much irrelevant, as the kernel tree is not a canonical repository of existing
> > platforms.
> > 
> 
> I understand the concern.
> Theoretically yes, but it's very unlikely that there's already an out of tree users
> wants to use it for a long term as we're still at the very initial stage.
> 
> And the most important reason is that current using actually is wrong.
> We can also choose to mark it as 'depreciated' and keep the backward compatibility in driver,
> but I'm not sure whether it's worthy to do it as we may add a lot ugly code in driver
> benefits no users.
> 
> Ideas?

At this stage, and given that there is no consensus on how this driver
should work, I'm tempted to just rip it out entirely by reverting
0136afa08967 and be done with it until people work out a way forward.

	M.

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
index 45790ce..eaabcda 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.txt
@@ -16,8 +16,8 @@  Required properties:
 - #interrupt-cells: Specifies the number of cells needed to encode an
   interrupt source. The value must be 1.
 - fsl,channel: The output channel that all input IRQs should be steered into.
-- fsl,irq-groups: Number of IRQ groups managed by this controller instance.
-  Each group manages 64 input interrupts.
+- fsl,irqs-per-chan: Number of input interrupts per channel. Should be multiple of 32
+  input interrupts and up to 512 interrupts.
 
 Example:
 
@@ -28,7 +28,7 @@  Example:
 		clocks = <&clk IMX8MQ_CLK_DISP_APB_ROOT>;
 		clock-names = "ipg";
 		fsl,channel = <0>;
-		fsl,irq-groups = <1>;
+		fsl,irqs-per-chan= <64>;
 		interrupt-controller;
 		#interrupt-cells = <1>;
 	};