Re: [PATCH v2 14/15] ARM: dts: bcm2711: Add the BSC interrupt controller
On Fri, Feb 12, 2021 at 12:20:14PM +0100, Nicolas Saenz Julienne wrote: > On Wed, 2021-02-10 at 10:49 -0800, Florian Fainelli wrote: > > On 2/10/21 7:49 AM, Dave Stevenson wrote: > > > Hi Marc. > > > > > > On Wed, 10 Feb 2021 at 15:30, Marc Zyngier wrote: > > > > > > > > Hi Maxime, > > > > > > > > On 2021-02-10 14:40, Maxime Ripard wrote: > > > > > Hi Dave, > > > > > > > > > > On Tue, Feb 09, 2021 at 09:49:05AM +, Dave Stevenson wrote: > > > > > > On Mon, 11 Jan 2021 at 14:23, Maxime Ripard > > > > > > wrote: > > > > > > > > > > > > > > The BSC controllers used for the HDMI DDC have an interrupt > > > > > > > controller > > > > > > > shared between both instances. Let's add it to avoid polling. > > > > > > > > > > > > This seems to have unintended side effects. > > > > > > GIC interrupt 117 is shared between the standard I2C controllers > > > > > > (i2c-bcm2835) and the l2-intc block handling the HDMI I2C > > > > > > interrupts. > > > > > > > > > > > > Whilst i2c-bcm2835 requests the interrupt with IRQF_SHARED, that > > > > > > doesn't appear to be an option for l2-intc registering as an > > > > > > interrupt > > > > > > controller. i2c-bcm2835 therefore loses out and fails to register > > > > > > for > > > > > > the interrupt. > > > > > > > > > > > > Is there an equivalent flag that an interrupt controller can add to > > > > > > say that the parent interrupt is shared? Is that even supported? > > > > > > > > > > Indeed, it looks like setting an equivalent to IRQF_SHARED would be > > > > > the > > > > > solution, but I couldn't find anything that would allow us to in the > > > > > irqchip code. > > > > > > > > > > Marc, Thomas, is it something that is allowed? > > > > > > > > No, not really. That's because the chained handler is actually an > > > > interrupt flow, and not a normal handler. IRQF_SHARED acts at the wrong > > > > level for that. > > > > > > > > I can see two possibilities: > > > > > > > > - the l2-intc gets turned into a normal handler, and does the demux > > > > from there. Horrible stuff. > > > > > > > > - the i2c controller gets parented to the l2c-int as a fake interrupt, > > > > and gets called from there. Horrible stuff. > > > > > > > > Pick your poison... :-/ > > > > > > Thanks for the info. > > > > > > Option 3 - remove l2-intc and drop back to polling the i2c-brcmstb > > > blocks (which the driver supports anyway). > > > HDMI I2C generally isn't heavily used once displays are connected, so > > > I'd be OK with that. > > > > > > (We can keep the l2-intc that handles CEC and HPD as that is on a > > > unique GIC interrupt). > > > > Agreed, Maxime or Nicolas do you want me to send a revert of this patch? > > Reverting seems the safe move, but I'll defer to whatever Maxime says. Yes, reverting it seems like the easiest way forward. If you can send it Florian that would be great :) Maxime signature.asc Description: PGP signature
Re: [PATCH v2 14/15] ARM: dts: bcm2711: Add the BSC interrupt controller
On Wed, 2021-02-10 at 10:49 -0800, Florian Fainelli wrote: > On 2/10/21 7:49 AM, Dave Stevenson wrote: > > Hi Marc. > > > > On Wed, 10 Feb 2021 at 15:30, Marc Zyngier wrote: > > > > > > Hi Maxime, > > > > > > On 2021-02-10 14:40, Maxime Ripard wrote: > > > > Hi Dave, > > > > > > > > On Tue, Feb 09, 2021 at 09:49:05AM +, Dave Stevenson wrote: > > > > > On Mon, 11 Jan 2021 at 14:23, Maxime Ripard wrote: > > > > > > > > > > > > The BSC controllers used for the HDMI DDC have an interrupt > > > > > > controller > > > > > > shared between both instances. Let's add it to avoid polling. > > > > > > > > > > This seems to have unintended side effects. > > > > > GIC interrupt 117 is shared between the standard I2C controllers > > > > > (i2c-bcm2835) and the l2-intc block handling the HDMI I2C interrupts. > > > > > > > > > > Whilst i2c-bcm2835 requests the interrupt with IRQF_SHARED, that > > > > > doesn't appear to be an option for l2-intc registering as an interrupt > > > > > controller. i2c-bcm2835 therefore loses out and fails to register for > > > > > the interrupt. > > > > > > > > > > Is there an equivalent flag that an interrupt controller can add to > > > > > say that the parent interrupt is shared? Is that even supported? > > > > > > > > Indeed, it looks like setting an equivalent to IRQF_SHARED would be the > > > > solution, but I couldn't find anything that would allow us to in the > > > > irqchip code. > > > > > > > > Marc, Thomas, is it something that is allowed? > > > > > > No, not really. That's because the chained handler is actually an > > > interrupt flow, and not a normal handler. IRQF_SHARED acts at the wrong > > > level for that. > > > > > > I can see two possibilities: > > > > > > - the l2-intc gets turned into a normal handler, and does the demux > > > from there. Horrible stuff. > > > > > > - the i2c controller gets parented to the l2c-int as a fake interrupt, > > > and gets called from there. Horrible stuff. > > > > > > Pick your poison... :-/ > > > > Thanks for the info. > > > > Option 3 - remove l2-intc and drop back to polling the i2c-brcmstb > > blocks (which the driver supports anyway). > > HDMI I2C generally isn't heavily used once displays are connected, so > > I'd be OK with that. > > > > (We can keep the l2-intc that handles CEC and HPD as that is on a > > unique GIC interrupt). > > Agreed, Maxime or Nicolas do you want me to send a revert of this patch? Reverting seems the safe move, but I'll defer to whatever Maxime says. Regards, Nicolas signature.asc Description: This is a digitally signed message part
Re: [PATCH v2 14/15] ARM: dts: bcm2711: Add the BSC interrupt controller
On 2/10/21 7:49 AM, Dave Stevenson wrote: > Hi Marc. > > On Wed, 10 Feb 2021 at 15:30, Marc Zyngier wrote: >> >> Hi Maxime, >> >> On 2021-02-10 14:40, Maxime Ripard wrote: >>> Hi Dave, >>> >>> On Tue, Feb 09, 2021 at 09:49:05AM +, Dave Stevenson wrote: On Mon, 11 Jan 2021 at 14:23, Maxime Ripard wrote: > > The BSC controllers used for the HDMI DDC have an interrupt controller > shared between both instances. Let's add it to avoid polling. This seems to have unintended side effects. GIC interrupt 117 is shared between the standard I2C controllers (i2c-bcm2835) and the l2-intc block handling the HDMI I2C interrupts. Whilst i2c-bcm2835 requests the interrupt with IRQF_SHARED, that doesn't appear to be an option for l2-intc registering as an interrupt controller. i2c-bcm2835 therefore loses out and fails to register for the interrupt. Is there an equivalent flag that an interrupt controller can add to say that the parent interrupt is shared? Is that even supported? >>> >>> Indeed, it looks like setting an equivalent to IRQF_SHARED would be the >>> solution, but I couldn't find anything that would allow us to in the >>> irqchip code. >>> >>> Marc, Thomas, is it something that is allowed? >> >> No, not really. That's because the chained handler is actually an >> interrupt flow, and not a normal handler. IRQF_SHARED acts at the wrong >> level for that. >> >> I can see two possibilities: >> >> - the l2-intc gets turned into a normal handler, and does the demux >>from there. Horrible stuff. >> >> - the i2c controller gets parented to the l2c-int as a fake interrupt, >>and gets called from there. Horrible stuff. >> >> Pick your poison... :-/ > > Thanks for the info. > > Option 3 - remove l2-intc and drop back to polling the i2c-brcmstb > blocks (which the driver supports anyway). > HDMI I2C generally isn't heavily used once displays are connected, so > I'd be OK with that. > > (We can keep the l2-intc that handles CEC and HPD as that is on a > unique GIC interrupt). Agreed, Maxime or Nicolas do you want me to send a revert of this patch? -- Florian
Re: [PATCH v2 14/15] ARM: dts: bcm2711: Add the BSC interrupt controller
Hi Marc. On Wed, 10 Feb 2021 at 15:30, Marc Zyngier wrote: > > Hi Maxime, > > On 2021-02-10 14:40, Maxime Ripard wrote: > > Hi Dave, > > > > On Tue, Feb 09, 2021 at 09:49:05AM +, Dave Stevenson wrote: > >> On Mon, 11 Jan 2021 at 14:23, Maxime Ripard wrote: > >> > > >> > The BSC controllers used for the HDMI DDC have an interrupt controller > >> > shared between both instances. Let's add it to avoid polling. > >> > >> This seems to have unintended side effects. > >> GIC interrupt 117 is shared between the standard I2C controllers > >> (i2c-bcm2835) and the l2-intc block handling the HDMI I2C interrupts. > >> > >> Whilst i2c-bcm2835 requests the interrupt with IRQF_SHARED, that > >> doesn't appear to be an option for l2-intc registering as an interrupt > >> controller. i2c-bcm2835 therefore loses out and fails to register for > >> the interrupt. > >> > >> Is there an equivalent flag that an interrupt controller can add to > >> say that the parent interrupt is shared? Is that even supported? > > > > Indeed, it looks like setting an equivalent to IRQF_SHARED would be the > > solution, but I couldn't find anything that would allow us to in the > > irqchip code. > > > > Marc, Thomas, is it something that is allowed? > > No, not really. That's because the chained handler is actually an > interrupt flow, and not a normal handler. IRQF_SHARED acts at the wrong > level for that. > > I can see two possibilities: > > - the l2-intc gets turned into a normal handler, and does the demux >from there. Horrible stuff. > > - the i2c controller gets parented to the l2c-int as a fake interrupt, >and gets called from there. Horrible stuff. > > Pick your poison... :-/ Thanks for the info. Option 3 - remove l2-intc and drop back to polling the i2c-brcmstb blocks (which the driver supports anyway). HDMI I2C generally isn't heavily used once displays are connected, so I'd be OK with that. (We can keep the l2-intc that handles CEC and HPD as that is on a unique GIC interrupt). Dave
Re: [PATCH v2 14/15] ARM: dts: bcm2711: Add the BSC interrupt controller
Hi Maxime, On 2021-02-10 14:40, Maxime Ripard wrote: Hi Dave, On Tue, Feb 09, 2021 at 09:49:05AM +, Dave Stevenson wrote: On Mon, 11 Jan 2021 at 14:23, Maxime Ripard wrote: > > The BSC controllers used for the HDMI DDC have an interrupt controller > shared between both instances. Let's add it to avoid polling. This seems to have unintended side effects. GIC interrupt 117 is shared between the standard I2C controllers (i2c-bcm2835) and the l2-intc block handling the HDMI I2C interrupts. Whilst i2c-bcm2835 requests the interrupt with IRQF_SHARED, that doesn't appear to be an option for l2-intc registering as an interrupt controller. i2c-bcm2835 therefore loses out and fails to register for the interrupt. Is there an equivalent flag that an interrupt controller can add to say that the parent interrupt is shared? Is that even supported? Indeed, it looks like setting an equivalent to IRQF_SHARED would be the solution, but I couldn't find anything that would allow us to in the irqchip code. Marc, Thomas, is it something that is allowed? No, not really. That's because the chained handler is actually an interrupt flow, and not a normal handler. IRQF_SHARED acts at the wrong level for that. I can see two possibilities: - the l2-intc gets turned into a normal handler, and does the demux from there. Horrible stuff. - the i2c controller gets parented to the l2c-int as a fake interrupt, and gets called from there. Horrible stuff. Pick your poison... :-/ M. -- Jazz is not dead. It just smells funny...
Re: [PATCH v2 14/15] ARM: dts: bcm2711: Add the BSC interrupt controller
Hi Dave, On Tue, Feb 09, 2021 at 09:49:05AM +, Dave Stevenson wrote: > On Mon, 11 Jan 2021 at 14:23, Maxime Ripard wrote: > > > > The BSC controllers used for the HDMI DDC have an interrupt controller > > shared between both instances. Let's add it to avoid polling. > > This seems to have unintended side effects. > GIC interrupt 117 is shared between the standard I2C controllers > (i2c-bcm2835) and the l2-intc block handling the HDMI I2C interrupts. > > Whilst i2c-bcm2835 requests the interrupt with IRQF_SHARED, that > doesn't appear to be an option for l2-intc registering as an interrupt > controller. i2c-bcm2835 therefore loses out and fails to register for > the interrupt. > > Is there an equivalent flag that an interrupt controller can add to > say that the parent interrupt is shared? Is that even supported? Indeed, it looks like setting an equivalent to IRQF_SHARED would be the solution, but I couldn't find anything that would allow us to in the irqchip code. Marc, Thomas, is it something that is allowed? Thanks! Maxime signature.asc Description: PGP signature
Re: [PATCH v2 14/15] ARM: dts: bcm2711: Add the BSC interrupt controller
Hi Maxime
On Mon, 11 Jan 2021 at 14:23, Maxime Ripard wrote:
>
> The BSC controllers used for the HDMI DDC have an interrupt controller
> shared between both instances. Let's add it to avoid polling.
This seems to have unintended side effects.
GIC interrupt 117 is shared between the standard I2C controllers
(i2c-bcm2835) and the l2-intc block handling the HDMI I2C interrupts.
Whilst i2c-bcm2835 requests the interrupt with IRQF_SHARED, that
doesn't appear to be an option for l2-intc registering as an interrupt
controller. i2c-bcm2835 therefore loses out and fails to register for
the interrupt.
Is there an equivalent flag that an interrupt controller can add to
say that the parent interrupt is shared? Is that even supported?
Thanks
Dave
> Reviewed-by: Florian Fainelli
> Signed-off-by: Maxime Ripard
> ---
> arch/arm/boot/dts/bcm2711.dtsi | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/arch/arm/boot/dts/bcm2711.dtsi b/arch/arm/boot/dts/bcm2711.dtsi
> index 4847dd305317..8bb46ae76a92 100644
> --- a/arch/arm/boot/dts/bcm2711.dtsi
> +++ b/arch/arm/boot/dts/bcm2711.dtsi
> @@ -308,6 +308,14 @@ dvp: clock@7ef0 {
> #reset-cells = <1>;
> };
>
> + bsc_intr: interrupt-controller@7ef00040 {
> + compatible = "brcm,bcm2711-l2-intc", "brcm,l2-intc";
> + reg = <0x7ef00040 0x30>;
> + interrupts = ;
> + interrupt-controller;
> + #interrupt-cells = <1>;
> + };
> +
> hdmi0: hdmi@7ef00700 {
> compatible = "brcm,bcm2711-hdmi0";
> reg = <0x7ef00700 0x300>,
> @@ -341,6 +349,8 @@ ddc0: i2c@7ef04500 {
> reg = <0x7ef04500 0x100>, <0x7ef00b00 0x300>;
> reg-names = "bsc", "auto-i2c";
> clock-frequency = <97500>;
> + interrupt-parent = <_intr>;
> + interrupts = <0>;
> status = "disabled";
> };
>
> @@ -377,6 +387,8 @@ ddc1: i2c@7ef09500 {
> reg = <0x7ef09500 0x100>, <0x7ef05b00 0x300>;
> reg-names = "bsc", "auto-i2c";
> clock-frequency = <97500>;
> + interrupt-parent = <_intr>;
> + interrupts = <1>;
> status = "disabled";
> };
> };
> --
> 2.29.2
>
[PATCH v2 14/15] ARM: dts: bcm2711: Add the BSC interrupt controller
The BSC controllers used for the HDMI DDC have an interrupt controller
shared between both instances. Let's add it to avoid polling.
Reviewed-by: Florian Fainelli
Signed-off-by: Maxime Ripard
---
arch/arm/boot/dts/bcm2711.dtsi | 12
1 file changed, 12 insertions(+)
diff --git a/arch/arm/boot/dts/bcm2711.dtsi b/arch/arm/boot/dts/bcm2711.dtsi
index 4847dd305317..8bb46ae76a92 100644
--- a/arch/arm/boot/dts/bcm2711.dtsi
+++ b/arch/arm/boot/dts/bcm2711.dtsi
@@ -308,6 +308,14 @@ dvp: clock@7ef0 {
#reset-cells = <1>;
};
+ bsc_intr: interrupt-controller@7ef00040 {
+ compatible = "brcm,bcm2711-l2-intc", "brcm,l2-intc";
+ reg = <0x7ef00040 0x30>;
+ interrupts = ;
+ interrupt-controller;
+ #interrupt-cells = <1>;
+ };
+
hdmi0: hdmi@7ef00700 {
compatible = "brcm,bcm2711-hdmi0";
reg = <0x7ef00700 0x300>,
@@ -341,6 +349,8 @@ ddc0: i2c@7ef04500 {
reg = <0x7ef04500 0x100>, <0x7ef00b00 0x300>;
reg-names = "bsc", "auto-i2c";
clock-frequency = <97500>;
+ interrupt-parent = <_intr>;
+ interrupts = <0>;
status = "disabled";
};
@@ -377,6 +387,8 @@ ddc1: i2c@7ef09500 {
reg = <0x7ef09500 0x100>, <0x7ef05b00 0x300>;
reg-names = "bsc", "auto-i2c";
clock-frequency = <97500>;
+ interrupt-parent = <_intr>;
+ interrupts = <1>;
status = "disabled";
};
};
--
2.29.2
