* [PATCH] irqchip/irq-pruss-intc: implement set_type() callback
@ 2021-01-04 18:36 ` David Lechner
0 siblings, 0 replies; 6+ messages in thread
From: David Lechner @ 2021-01-04 18:36 UTC (permalink / raw)
To: linux-kernel
Cc: David Lechner, Thomas Gleixner, Marc Zyngier, Suman Anna,
Grzegorz Jaszczyk, Sekhar Nori, Bartosz Golaszewski,
linux-arm-kernel
This implements the irqchip set_type() callback for the TI PRUSS
interrupt controller. This is needed for cases where an event needs
to be active low.
According to the technical reference manual, the polarity should always
be set to high, however in practice, the polarity needs to be set low
for the McASP Tx/Rx system event in conjunction with soft UART PRU
firmware for TI AM18XX SoCs, otherwise it doesn't work.
Signed-off-by: David Lechner <david@lechnology.com>
---
drivers/irqchip/irq-pruss-intc.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/drivers/irqchip/irq-pruss-intc.c b/drivers/irqchip/irq-pruss-intc.c
index 5409016e6ca0..f882af8a7ded 100644
--- a/drivers/irqchip/irq-pruss-intc.c
+++ b/drivers/irqchip/irq-pruss-intc.c
@@ -334,6 +334,32 @@ static void pruss_intc_irq_unmask(struct irq_data *data)
pruss_intc_write_reg(intc, PRU_INTC_EISR, hwirq);
}
+static int pruss_intc_irq_set_type(struct irq_data *data, unsigned int type)
+{
+ struct pruss_intc *intc = irq_data_get_irq_chip_data(data);
+ u32 reg, bit, val;
+
+ if (type & IRQ_TYPE_LEVEL_MASK) {
+ /* polarity register */
+ reg = PRU_INTC_SIPR(data->hwirq / 32);
+ bit = BIT(data->hwirq % 32);
+ val = pruss_intc_read_reg(intc, reg);
+
+ /*
+ * This check also ensures that IRQ_TYPE_DEFAULT will result
+ * in setting the level to high.
+ */
+ if (type & IRQ_TYPE_LEVEL_HIGH)
+ val |= bit;
+ else
+ val &= ~bit;
+
+ pruss_intc_write_reg(intc, reg, val);
+ }
+
+ return 0;
+}
+
static int pruss_intc_irq_reqres(struct irq_data *data)
{
if (!try_module_get(THIS_MODULE))
@@ -389,6 +415,7 @@ static struct irq_chip pruss_irqchip = {
.irq_ack = pruss_intc_irq_ack,
.irq_mask = pruss_intc_irq_mask,
.irq_unmask = pruss_intc_irq_unmask,
+ .irq_set_type = pruss_intc_irq_set_type,
.irq_request_resources = pruss_intc_irq_reqres,
.irq_release_resources = pruss_intc_irq_relres,
.irq_get_irqchip_state = pruss_intc_irq_get_irqchip_state,
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH] irqchip/irq-pruss-intc: implement set_type() callback
@ 2021-01-04 18:36 ` David Lechner
0 siblings, 0 replies; 6+ messages in thread
From: David Lechner @ 2021-01-04 18:36 UTC (permalink / raw)
To: linux-kernel
Cc: Grzegorz Jaszczyk, David Lechner, Marc Zyngier, Sekhar Nori,
Bartosz Golaszewski, Thomas Gleixner, linux-arm-kernel
This implements the irqchip set_type() callback for the TI PRUSS
interrupt controller. This is needed for cases where an event needs
to be active low.
According to the technical reference manual, the polarity should always
be set to high, however in practice, the polarity needs to be set low
for the McASP Tx/Rx system event in conjunction with soft UART PRU
firmware for TI AM18XX SoCs, otherwise it doesn't work.
Signed-off-by: David Lechner <david@lechnology.com>
---
drivers/irqchip/irq-pruss-intc.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/drivers/irqchip/irq-pruss-intc.c b/drivers/irqchip/irq-pruss-intc.c
index 5409016e6ca0..f882af8a7ded 100644
--- a/drivers/irqchip/irq-pruss-intc.c
+++ b/drivers/irqchip/irq-pruss-intc.c
@@ -334,6 +334,32 @@ static void pruss_intc_irq_unmask(struct irq_data *data)
pruss_intc_write_reg(intc, PRU_INTC_EISR, hwirq);
}
+static int pruss_intc_irq_set_type(struct irq_data *data, unsigned int type)
+{
+ struct pruss_intc *intc = irq_data_get_irq_chip_data(data);
+ u32 reg, bit, val;
+
+ if (type & IRQ_TYPE_LEVEL_MASK) {
+ /* polarity register */
+ reg = PRU_INTC_SIPR(data->hwirq / 32);
+ bit = BIT(data->hwirq % 32);
+ val = pruss_intc_read_reg(intc, reg);
+
+ /*
+ * This check also ensures that IRQ_TYPE_DEFAULT will result
+ * in setting the level to high.
+ */
+ if (type & IRQ_TYPE_LEVEL_HIGH)
+ val |= bit;
+ else
+ val &= ~bit;
+
+ pruss_intc_write_reg(intc, reg, val);
+ }
+
+ return 0;
+}
+
static int pruss_intc_irq_reqres(struct irq_data *data)
{
if (!try_module_get(THIS_MODULE))
@@ -389,6 +415,7 @@ static struct irq_chip pruss_irqchip = {
.irq_ack = pruss_intc_irq_ack,
.irq_mask = pruss_intc_irq_mask,
.irq_unmask = pruss_intc_irq_unmask,
+ .irq_set_type = pruss_intc_irq_set_type,
.irq_request_resources = pruss_intc_irq_reqres,
.irq_release_resources = pruss_intc_irq_relres,
.irq_get_irqchip_state = pruss_intc_irq_get_irqchip_state,
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] irqchip/irq-pruss-intc: implement set_type() callback
2021-01-04 18:36 ` David Lechner
@ 2021-01-05 8:47 ` Marc Zyngier
-1 siblings, 0 replies; 6+ messages in thread
From: Marc Zyngier @ 2021-01-05 8:47 UTC (permalink / raw)
To: David Lechner
Cc: linux-kernel, Thomas Gleixner, Suman Anna, Grzegorz Jaszczyk,
Sekhar Nori, Bartosz Golaszewski, linux-arm-kernel
On 2021-01-04 18:36, David Lechner wrote:
> This implements the irqchip set_type() callback for the TI PRUSS
> interrupt controller. This is needed for cases where an event needs
> to be active low.
>
> According to the technical reference manual, the polarity should always
> be set to high, however in practice, the polarity needs to be set low
> for the McASP Tx/Rx system event in conjunction with soft UART PRU
> firmware for TI AM18XX SoCs, otherwise it doesn't work.
I remember asking about this when I reviewed the patch series, and
was told that there was no need to handle anything *but* level high.
As a consequence, the DT binding doesn't have any way to express
the trigger configuration.
Now this? What is going to drive the configuration?
>
> Signed-off-by: David Lechner <david@lechnology.com>
> ---
> drivers/irqchip/irq-pruss-intc.c | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/drivers/irqchip/irq-pruss-intc.c
> b/drivers/irqchip/irq-pruss-intc.c
> index 5409016e6ca0..f882af8a7ded 100644
> --- a/drivers/irqchip/irq-pruss-intc.c
> +++ b/drivers/irqchip/irq-pruss-intc.c
> @@ -334,6 +334,32 @@ static void pruss_intc_irq_unmask(struct irq_data
> *data)
> pruss_intc_write_reg(intc, PRU_INTC_EISR, hwirq);
> }
>
> +static int pruss_intc_irq_set_type(struct irq_data *data, unsigned int
> type)
> +{
> + struct pruss_intc *intc = irq_data_get_irq_chip_data(data);
> + u32 reg, bit, val;
> +
> + if (type & IRQ_TYPE_LEVEL_MASK) {
> + /* polarity register */
> + reg = PRU_INTC_SIPR(data->hwirq / 32);
> + bit = BIT(data->hwirq % 32);
> + val = pruss_intc_read_reg(intc, reg);
> +
> + /*
> + * This check also ensures that IRQ_TYPE_DEFAULT will result
> + * in setting the level to high.
> + */
> + if (type & IRQ_TYPE_LEVEL_HIGH)
> + val |= bit;
> + else
> + val &= ~bit;
> +
> + pruss_intc_write_reg(intc, reg, val);
RMW of a shared register without locking?
> + }
> +
> + return 0;
What happens when this is passed an edge configuration? It should at
least return an error.
> +}
> +
> static int pruss_intc_irq_reqres(struct irq_data *data)
> {
> if (!try_module_get(THIS_MODULE))
> @@ -389,6 +415,7 @@ static struct irq_chip pruss_irqchip = {
> .irq_ack = pruss_intc_irq_ack,
> .irq_mask = pruss_intc_irq_mask,
> .irq_unmask = pruss_intc_irq_unmask,
> + .irq_set_type = pruss_intc_irq_set_type,
> .irq_request_resources = pruss_intc_irq_reqres,
> .irq_release_resources = pruss_intc_irq_relres,
> .irq_get_irqchip_state = pruss_intc_irq_get_irqchip_state,
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] irqchip/irq-pruss-intc: implement set_type() callback
@ 2021-01-05 8:47 ` Marc Zyngier
0 siblings, 0 replies; 6+ messages in thread
From: Marc Zyngier @ 2021-01-05 8:47 UTC (permalink / raw)
To: David Lechner
Cc: Grzegorz Jaszczyk, Sekhar Nori, linux-kernel,
Bartosz Golaszewski, Thomas Gleixner, linux-arm-kernel
On 2021-01-04 18:36, David Lechner wrote:
> This implements the irqchip set_type() callback for the TI PRUSS
> interrupt controller. This is needed for cases where an event needs
> to be active low.
>
> According to the technical reference manual, the polarity should always
> be set to high, however in practice, the polarity needs to be set low
> for the McASP Tx/Rx system event in conjunction with soft UART PRU
> firmware for TI AM18XX SoCs, otherwise it doesn't work.
I remember asking about this when I reviewed the patch series, and
was told that there was no need to handle anything *but* level high.
As a consequence, the DT binding doesn't have any way to express
the trigger configuration.
Now this? What is going to drive the configuration?
>
> Signed-off-by: David Lechner <david@lechnology.com>
> ---
> drivers/irqchip/irq-pruss-intc.c | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/drivers/irqchip/irq-pruss-intc.c
> b/drivers/irqchip/irq-pruss-intc.c
> index 5409016e6ca0..f882af8a7ded 100644
> --- a/drivers/irqchip/irq-pruss-intc.c
> +++ b/drivers/irqchip/irq-pruss-intc.c
> @@ -334,6 +334,32 @@ static void pruss_intc_irq_unmask(struct irq_data
> *data)
> pruss_intc_write_reg(intc, PRU_INTC_EISR, hwirq);
> }
>
> +static int pruss_intc_irq_set_type(struct irq_data *data, unsigned int
> type)
> +{
> + struct pruss_intc *intc = irq_data_get_irq_chip_data(data);
> + u32 reg, bit, val;
> +
> + if (type & IRQ_TYPE_LEVEL_MASK) {
> + /* polarity register */
> + reg = PRU_INTC_SIPR(data->hwirq / 32);
> + bit = BIT(data->hwirq % 32);
> + val = pruss_intc_read_reg(intc, reg);
> +
> + /*
> + * This check also ensures that IRQ_TYPE_DEFAULT will result
> + * in setting the level to high.
> + */
> + if (type & IRQ_TYPE_LEVEL_HIGH)
> + val |= bit;
> + else
> + val &= ~bit;
> +
> + pruss_intc_write_reg(intc, reg, val);
RMW of a shared register without locking?
> + }
> +
> + return 0;
What happens when this is passed an edge configuration? It should at
least return an error.
> +}
> +
> static int pruss_intc_irq_reqres(struct irq_data *data)
> {
> if (!try_module_get(THIS_MODULE))
> @@ -389,6 +415,7 @@ static struct irq_chip pruss_irqchip = {
> .irq_ack = pruss_intc_irq_ack,
> .irq_mask = pruss_intc_irq_mask,
> .irq_unmask = pruss_intc_irq_unmask,
> + .irq_set_type = pruss_intc_irq_set_type,
> .irq_request_resources = pruss_intc_irq_reqres,
> .irq_release_resources = pruss_intc_irq_relres,
> .irq_get_irqchip_state = pruss_intc_irq_get_irqchip_state,
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] irqchip/irq-pruss-intc: implement set_type() callback
2021-01-05 8:47 ` Marc Zyngier
@ 2021-01-05 17:37 ` David Lechner
-1 siblings, 0 replies; 6+ messages in thread
From: David Lechner @ 2021-01-05 17:37 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-kernel, Thomas Gleixner, Suman Anna, Grzegorz Jaszczyk,
Sekhar Nori, Bartosz Golaszewski, linux-arm-kernel
On 1/5/21 2:47 AM, Marc Zyngier wrote:
> On 2021-01-04 18:36, David Lechner wrote:
>> This implements the irqchip set_type() callback for the TI PRUSS
>> interrupt controller. This is needed for cases where an event needs
>> to be active low.
>>
>> According to the technical reference manual, the polarity should always
>> be set to high, however in practice, the polarity needs to be set low
>> for the McASP Tx/Rx system event in conjunction with soft UART PRU
>> firmware for TI AM18XX SoCs, otherwise it doesn't work.
>
> I remember asking about this when I reviewed the patch series, and
> was told that there was no need to handle anything *but* level high.
> As a consequence, the DT binding doesn't have any way to express
> the trigger configuration.
>
> Now this? What is going to drive the configuration?
I made it work by setting IRQF_TRIGGER_LOW in devm_request_irq().
I can try to see if there is a way to change the (10 year old)
firmware instead.
Hopefully someone from TI can shed more light on the subject?
>
>>
>> Signed-off-by: David Lechner <david@lechnology.com>
>> ---
>> drivers/irqchip/irq-pruss-intc.c | 27 +++++++++++++++++++++++++++
>> 1 file changed, 27 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-pruss-intc.c b/drivers/irqchip/irq-pruss-intc.c
>> index 5409016e6ca0..f882af8a7ded 100644
>> --- a/drivers/irqchip/irq-pruss-intc.c
>> +++ b/drivers/irqchip/irq-pruss-intc.c
>> @@ -334,6 +334,32 @@ static void pruss_intc_irq_unmask(struct irq_data *data)
>> pruss_intc_write_reg(intc, PRU_INTC_EISR, hwirq);
>> }
>>
>> +static int pruss_intc_irq_set_type(struct irq_data *data, unsigned int type)
>> +{
>> + struct pruss_intc *intc = irq_data_get_irq_chip_data(data);
>> + u32 reg, bit, val;
>> +
>> + if (type & IRQ_TYPE_LEVEL_MASK) {
>> + /* polarity register */
>> + reg = PRU_INTC_SIPR(data->hwirq / 32);
>> + bit = BIT(data->hwirq % 32);
>> + val = pruss_intc_read_reg(intc, reg);
>> +
>> + /*
>> + * This check also ensures that IRQ_TYPE_DEFAULT will result
>> + * in setting the level to high.
>> + */
>> + if (type & IRQ_TYPE_LEVEL_HIGH)
>> + val |= bit;
>> + else
>> + val &= ~bit;
>> +
>> + pruss_intc_write_reg(intc, reg, val);
>
> RMW of a shared register without locking?
>
>> + }
>> +
>> + return 0;
>
> What happens when this is passed an edge configuration? It should at
> least return an error.
This is probably a moot point if we are not going to allow
changing the type, but there is another register for selecting
edge or level. But the manual says similarly says it should
always be set to level.
>
>> +}
>> +
>> static int pruss_intc_irq_reqres(struct irq_data *data)
>> {
>> if (!try_module_get(THIS_MODULE))
>> @@ -389,6 +415,7 @@ static struct irq_chip pruss_irqchip = {
>> .irq_ack = pruss_intc_irq_ack,
>> .irq_mask = pruss_intc_irq_mask,
>> .irq_unmask = pruss_intc_irq_unmask,
>> + .irq_set_type = pruss_intc_irq_set_type,
>> .irq_request_resources = pruss_intc_irq_reqres,
>> .irq_release_resources = pruss_intc_irq_relres,
>> .irq_get_irqchip_state = pruss_intc_irq_get_irqchip_state,
>
> Thanks,
>
> M.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] irqchip/irq-pruss-intc: implement set_type() callback
@ 2021-01-05 17:37 ` David Lechner
0 siblings, 0 replies; 6+ messages in thread
From: David Lechner @ 2021-01-05 17:37 UTC (permalink / raw)
To: Marc Zyngier
Cc: Grzegorz Jaszczyk, Sekhar Nori, linux-kernel,
Bartosz Golaszewski, Thomas Gleixner, linux-arm-kernel
On 1/5/21 2:47 AM, Marc Zyngier wrote:
> On 2021-01-04 18:36, David Lechner wrote:
>> This implements the irqchip set_type() callback for the TI PRUSS
>> interrupt controller. This is needed for cases where an event needs
>> to be active low.
>>
>> According to the technical reference manual, the polarity should always
>> be set to high, however in practice, the polarity needs to be set low
>> for the McASP Tx/Rx system event in conjunction with soft UART PRU
>> firmware for TI AM18XX SoCs, otherwise it doesn't work.
>
> I remember asking about this when I reviewed the patch series, and
> was told that there was no need to handle anything *but* level high.
> As a consequence, the DT binding doesn't have any way to express
> the trigger configuration.
>
> Now this? What is going to drive the configuration?
I made it work by setting IRQF_TRIGGER_LOW in devm_request_irq().
I can try to see if there is a way to change the (10 year old)
firmware instead.
Hopefully someone from TI can shed more light on the subject?
>
>>
>> Signed-off-by: David Lechner <david@lechnology.com>
>> ---
>> drivers/irqchip/irq-pruss-intc.c | 27 +++++++++++++++++++++++++++
>> 1 file changed, 27 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-pruss-intc.c b/drivers/irqchip/irq-pruss-intc.c
>> index 5409016e6ca0..f882af8a7ded 100644
>> --- a/drivers/irqchip/irq-pruss-intc.c
>> +++ b/drivers/irqchip/irq-pruss-intc.c
>> @@ -334,6 +334,32 @@ static void pruss_intc_irq_unmask(struct irq_data *data)
>> pruss_intc_write_reg(intc, PRU_INTC_EISR, hwirq);
>> }
>>
>> +static int pruss_intc_irq_set_type(struct irq_data *data, unsigned int type)
>> +{
>> + struct pruss_intc *intc = irq_data_get_irq_chip_data(data);
>> + u32 reg, bit, val;
>> +
>> + if (type & IRQ_TYPE_LEVEL_MASK) {
>> + /* polarity register */
>> + reg = PRU_INTC_SIPR(data->hwirq / 32);
>> + bit = BIT(data->hwirq % 32);
>> + val = pruss_intc_read_reg(intc, reg);
>> +
>> + /*
>> + * This check also ensures that IRQ_TYPE_DEFAULT will result
>> + * in setting the level to high.
>> + */
>> + if (type & IRQ_TYPE_LEVEL_HIGH)
>> + val |= bit;
>> + else
>> + val &= ~bit;
>> +
>> + pruss_intc_write_reg(intc, reg, val);
>
> RMW of a shared register without locking?
>
>> + }
>> +
>> + return 0;
>
> What happens when this is passed an edge configuration? It should at
> least return an error.
This is probably a moot point if we are not going to allow
changing the type, but there is another register for selecting
edge or level. But the manual says similarly says it should
always be set to level.
>
>> +}
>> +
>> static int pruss_intc_irq_reqres(struct irq_data *data)
>> {
>> if (!try_module_get(THIS_MODULE))
>> @@ -389,6 +415,7 @@ static struct irq_chip pruss_irqchip = {
>> .irq_ack = pruss_intc_irq_ack,
>> .irq_mask = pruss_intc_irq_mask,
>> .irq_unmask = pruss_intc_irq_unmask,
>> + .irq_set_type = pruss_intc_irq_set_type,
>> .irq_request_resources = pruss_intc_irq_reqres,
>> .irq_release_resources = pruss_intc_irq_relres,
>> .irq_get_irqchip_state = pruss_intc_irq_get_irqchip_state,
>
> Thanks,
>
> M.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-01-05 17:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-04 18:36 [PATCH] irqchip/irq-pruss-intc: implement set_type() callback David Lechner
2021-01-04 18:36 ` David Lechner
2021-01-05 8:47 ` Marc Zyngier
2021-01-05 8:47 ` Marc Zyngier
2021-01-05 17:37 ` David Lechner
2021-01-05 17:37 ` David Lechner
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.