diff mbox

[V2,03/19] irqchip: crossbar: Skip some irqs from getting mapped to crossbar

Message ID 1402574007-13987-4-git-send-email-r.sricharan@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

R Sricharan June 12, 2014, 11:53 a.m. UTC
From: Nishanth Menon <nm@ti.com>

When, in the system due to varied reasons, interrupts might be unusable
due to hardware behavior, but register maps do exist, then those interrupts
should be skipped while mapping irq to crossbars.

Signed-off-by: Nishanth Menon <nm@ti.com>
Signed-off-by: Sricharan R <r.sricharan@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/irqchip/irq-crossbar.c |   47 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 43 insertions(+), 4 deletions(-)

Comments

Jason Cooper June 12, 2014, 12:51 p.m. UTC | #1
On Thu, Jun 12, 2014 at 05:23:11PM +0530, Sricharan R wrote:
> From: Nishanth Menon <nm@ti.com>
> 
> When, in the system due to varied reasons, interrupts might be unusable
> due to hardware behavior, but register maps do exist, then those interrupts
> should be skipped while mapping irq to crossbars.
> 
> Signed-off-by: Nishanth Menon <nm@ti.com>
> Signed-off-by: Sricharan R <r.sricharan@ti.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Tony, have you applied these somewhere already?

> ---
>  drivers/irqchip/irq-crossbar.c |   47 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 43 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
> index 51d4b87..847f6e3 100644
> --- a/drivers/irqchip/irq-crossbar.c
> +++ b/drivers/irqchip/irq-crossbar.c
> @@ -13,11 +13,13 @@
>  #include <linux/io.h>
>  #include <linux/of_address.h>
>  #include <linux/of_irq.h>
> +#include <linux/of_device.h>
>  #include <linux/slab.h>
>  #include <linux/irqchip/arm-gic.h>
>  
>  #define IRQ_FREE	-1
>  #define IRQ_RESERVED	-2
> +#define IRQ_SKIP	-3
>  #define GIC_IRQ_START	32
>  
>  /*
> @@ -34,6 +36,16 @@ struct crossbar_device {
>  	void (*write) (int, int);
>  };
>  
> +/**
> + * struct crossbar_data: Platform specific data
> + * @irqs_unused: array of irqs that cannot be used because of hw erratas
> + * @size: size of the irqs_unused array
> + */
> +struct crossbar_data {
> +	const uint *irqs_unused;
> +	const uint size;
> +};
> +
>  static struct crossbar_device *cb;
>  
>  static inline void crossbar_writel(int irq_no, int cb_no)
> @@ -119,10 +131,12 @@ const struct irq_domain_ops routable_irq_domain_ops = {
>  	.xlate = crossbar_domain_xlate
>  };
>  
> -static int __init crossbar_of_init(struct device_node *node)
> +static int __init crossbar_of_init(struct device_node *node,
> +				   const struct crossbar_data *data)
>  {
>  	int i, size, max, reserved = 0, entry;
>  	const __be32 *irqsr;
> +	const int *irqsk = NULL;
>  
>  	cb = kzalloc(sizeof(*cb), GFP_KERNEL);
>  
> @@ -194,6 +208,22 @@ static int __init crossbar_of_init(struct device_node *node)
>  		reserved += size;
>  	}
>  
> +	/* Skip the ones marked as unused */
> +	if (data) {
> +		irqsk = data->irqs_unused;
> +		size = data->size;
> +
> +		for (i = 0; i < size; i++) {
> +			entry = irqsk[i];
> +
> +			if (entry > max) {
> +				pr_err("Invalid skip entry\n");
> +				goto err3;
> +			}
> +			cb->irq_map[entry] = IRQ_SKIP;
> +		}
> +	}
> +
>  	register_routable_domain_ops(&routable_irq_domain_ops);
>  	return 0;
>  
> @@ -208,18 +238,27 @@ err1:
>  	return -ENOMEM;
>  }
>  
> +/* irq number 10 cannot be used because of hw bug */
> +int dra_irqs_unused[] = { 10 };
> +struct crossbar_data cb_dra_data = { dra_irqs_unused,
> +				     ARRAY_SIZE(dra_irqs_unused) };
> +
>  static const struct of_device_id crossbar_match[] __initconst = {
> -	{ .compatible = "ti,irq-crossbar" },
> +	{ .compatible = "ti,irq-crossbar", .data = &cb_dra_data },
>  	{}
>  };

This is a bug in all implementations of this IP?  Or, a specific
SoC's implementation?  Would this be better expressed in the dts via a
property?  Can we expect future implementations to be fixed?

thx,

Jason.
R Sricharan June 12, 2014, 1:19 p.m. UTC | #2
Hi Jason,

On Thursday 12 June 2014 06:21 PM, Jason Cooper wrote:
> On Thu, Jun 12, 2014 at 05:23:11PM +0530, Sricharan R wrote:
>> From: Nishanth Menon <nm@ti.com>
>>
>> When, in the system due to varied reasons, interrupts might be unusable
>> due to hardware behavior, but register maps do exist, then those interrupts
>> should be skipped while mapping irq to crossbars.
>>
>> Signed-off-by: Nishanth Menon <nm@ti.com>
>> Signed-off-by: Sricharan R <r.sricharan@ti.com>
>> Signed-off-by: Tony Lindgren <tony@atomide.com>
> 
> Tony, have you applied these somewhere already?
> 
>> ---
>>  drivers/irqchip/irq-crossbar.c |   47 ++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 43 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
>> index 51d4b87..847f6e3 100644
>> --- a/drivers/irqchip/irq-crossbar.c
>> +++ b/drivers/irqchip/irq-crossbar.c
>> @@ -13,11 +13,13 @@
>>  #include <linux/io.h>
>>  #include <linux/of_address.h>
>>  #include <linux/of_irq.h>
>> +#include <linux/of_device.h>
>>  #include <linux/slab.h>
>>  #include <linux/irqchip/arm-gic.h>
>>  
>>  #define IRQ_FREE	-1
>>  #define IRQ_RESERVED	-2
>> +#define IRQ_SKIP	-3
>>  #define GIC_IRQ_START	32
>>  
>>  /*
>> @@ -34,6 +36,16 @@ struct crossbar_device {
>>  	void (*write) (int, int);
>>  };
>>  
>> +/**
>> + * struct crossbar_data: Platform specific data
>> + * @irqs_unused: array of irqs that cannot be used because of hw erratas
>> + * @size: size of the irqs_unused array
>> + */
>> +struct crossbar_data {
>> +	const uint *irqs_unused;
>> +	const uint size;
>> +};
>> +
>>  static struct crossbar_device *cb;
>>  
>>  static inline void crossbar_writel(int irq_no, int cb_no)
>> @@ -119,10 +131,12 @@ const struct irq_domain_ops routable_irq_domain_ops = {
>>  	.xlate = crossbar_domain_xlate
>>  };
>>  
>> -static int __init crossbar_of_init(struct device_node *node)
>> +static int __init crossbar_of_init(struct device_node *node,
>> +				   const struct crossbar_data *data)
>>  {
>>  	int i, size, max, reserved = 0, entry;
>>  	const __be32 *irqsr;
>> +	const int *irqsk = NULL;
>>  
>>  	cb = kzalloc(sizeof(*cb), GFP_KERNEL);
>>  
>> @@ -194,6 +208,22 @@ static int __init crossbar_of_init(struct device_node *node)
>>  		reserved += size;
>>  	}
>>  
>> +	/* Skip the ones marked as unused */
>> +	if (data) {
>> +		irqsk = data->irqs_unused;
>> +		size = data->size;
>> +
>> +		for (i = 0; i < size; i++) {
>> +			entry = irqsk[i];
>> +
>> +			if (entry > max) {
>> +				pr_err("Invalid skip entry\n");
>> +				goto err3;
>> +			}
>> +			cb->irq_map[entry] = IRQ_SKIP;
>> +		}
>> +	}
>> +
>>  	register_routable_domain_ops(&routable_irq_domain_ops);
>>  	return 0;
>>  
>> @@ -208,18 +238,27 @@ err1:
>>  	return -ENOMEM;
>>  }
>>  
>> +/* irq number 10 cannot be used because of hw bug */
>> +int dra_irqs_unused[] = { 10 };
>> +struct crossbar_data cb_dra_data = { dra_irqs_unused,
>> +				     ARRAY_SIZE(dra_irqs_unused) };
>> +
>>  static const struct of_device_id crossbar_match[] __initconst = {
>> -	{ .compatible = "ti,irq-crossbar" },
>> +	{ .compatible = "ti,irq-crossbar", .data = &cb_dra_data },
>>  	{}
>>  };
> 
> This is a bug in all implementations of this IP?  Or, a specific
> SoC's implementation?  Would this be better expressed in the dts via a
> property?  Can we expect future implementations to be fixed?
> 
> thx,
> 
> Jason.
 Infact this and PATCH#10 should be merged. I will change that.

 So in Socs's (2 so far) that do have a crossbar, some irqs are mapped
 through a crossbar and some are directly wired to the irqchip.
 These 'unused irqs' are those which are directly wired but they still
 have a crossbar register. Their routing cannot be changed. So this
 is not really expected usage of the crossbar hw ip. We initially thought
 having a dts property separately for this, but took this path to avoid
 loading the dts with additional bindings which may not be generic.

Regards,
 Sricharan
Tony Lindgren June 12, 2014, 1:57 p.m. UTC | #3
* Jason Cooper <jason@lakedaemon.net> [140612 05:52]:
> On Thu, Jun 12, 2014 at 05:23:11PM +0530, Sricharan R wrote:
> > From: Nishanth Menon <nm@ti.com>
> > 
> > When, in the system due to varied reasons, interrupts might be unusable
> > due to hardware behavior, but register maps do exist, then those interrupts
> > should be skipped while mapping irq to crossbars.
> > 
> > Signed-off-by: Nishanth Menon <nm@ti.com>
> > Signed-off-by: Sricharan R <r.sricharan@ti.com>
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> 
> Tony, have you applied these somewhere already?

I think some of these I had applied into a branch ready for
merging but then it was discovered that further changes
were needed and the branch was dropped.

Sricharan, please remove my Signed-off-by from this series.
If I end up applying it for merging my scripts will add it
automatically.

Regards,

Tony
Jason Cooper June 12, 2014, 2:05 p.m. UTC | #4
On Thu, Jun 12, 2014 at 06:57:15AM -0700, Tony Lindgren wrote:
> * Jason Cooper <jason@lakedaemon.net> [140612 05:52]:
> > On Thu, Jun 12, 2014 at 05:23:11PM +0530, Sricharan R wrote:
> > > From: Nishanth Menon <nm@ti.com>
> > > 
> > > When, in the system due to varied reasons, interrupts might be unusable
> > > due to hardware behavior, but register maps do exist, then those interrupts
> > > should be skipped while mapping irq to crossbars.
> > > 
> > > Signed-off-by: Nishanth Menon <nm@ti.com>
> > > Signed-off-by: Sricharan R <r.sricharan@ti.com>
> > > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > 
> > Tony, have you applied these somewhere already?
> 
> I think some of these I had applied into a branch ready for
> merging but then it was discovered that further changes
> were needed and the branch was dropped.

Ok.

> Sricharan, please remove my Signed-off-by from this series.
> If I end up applying it for merging my scripts will add it
> automatically.

Do you have other changes outside of irqchip depending on this series?
If so, I can set up a topic branch for you guys to base off of.
Otherwise, I'll just apply them to irqchip/core when they're ready.

Also, Sricharan, when you respin this, please clearly identify (in the
comment section) those patches that need to be flagged for stable.  It
would be helpful if they were the first patches in the series as well.

thx,

Jason.
Jason Cooper June 12, 2014, 2:07 p.m. UTC | #5
On Thu, Jun 12, 2014 at 06:49:17PM +0530, Sricharan R wrote:
> Hi Jason,
> 
> On Thursday 12 June 2014 06:21 PM, Jason Cooper wrote:
> > On Thu, Jun 12, 2014 at 05:23:11PM +0530, Sricharan R wrote:
> >> From: Nishanth Menon <nm@ti.com>
> >>
> >> When, in the system due to varied reasons, interrupts might be unusable
> >> due to hardware behavior, but register maps do exist, then those interrupts
> >> should be skipped while mapping irq to crossbars.
> >>
> >> Signed-off-by: Nishanth Menon <nm@ti.com>
> >> Signed-off-by: Sricharan R <r.sricharan@ti.com>
> >> Signed-off-by: Tony Lindgren <tony@atomide.com>
> > 
> > Tony, have you applied these somewhere already?
> > 
> >> ---
> >>  drivers/irqchip/irq-crossbar.c |   47 ++++++++++++++++++++++++++++++++++++----
> >>  1 file changed, 43 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
> >> index 51d4b87..847f6e3 100644
> >> --- a/drivers/irqchip/irq-crossbar.c
> >> +++ b/drivers/irqchip/irq-crossbar.c
> >> @@ -13,11 +13,13 @@
> >>  #include <linux/io.h>
> >>  #include <linux/of_address.h>
> >>  #include <linux/of_irq.h>
> >> +#include <linux/of_device.h>
> >>  #include <linux/slab.h>
> >>  #include <linux/irqchip/arm-gic.h>
> >>  
> >>  #define IRQ_FREE	-1
> >>  #define IRQ_RESERVED	-2
> >> +#define IRQ_SKIP	-3
> >>  #define GIC_IRQ_START	32
> >>  
> >>  /*
> >> @@ -34,6 +36,16 @@ struct crossbar_device {
> >>  	void (*write) (int, int);
> >>  };
> >>  
> >> +/**
> >> + * struct crossbar_data: Platform specific data
> >> + * @irqs_unused: array of irqs that cannot be used because of hw erratas
> >> + * @size: size of the irqs_unused array
> >> + */
> >> +struct crossbar_data {
> >> +	const uint *irqs_unused;
> >> +	const uint size;
> >> +};
> >> +
> >>  static struct crossbar_device *cb;
> >>  
> >>  static inline void crossbar_writel(int irq_no, int cb_no)
> >> @@ -119,10 +131,12 @@ const struct irq_domain_ops routable_irq_domain_ops = {
> >>  	.xlate = crossbar_domain_xlate
> >>  };
> >>  
> >> -static int __init crossbar_of_init(struct device_node *node)
> >> +static int __init crossbar_of_init(struct device_node *node,
> >> +				   const struct crossbar_data *data)
> >>  {
> >>  	int i, size, max, reserved = 0, entry;
> >>  	const __be32 *irqsr;
> >> +	const int *irqsk = NULL;
> >>  
> >>  	cb = kzalloc(sizeof(*cb), GFP_KERNEL);
> >>  
> >> @@ -194,6 +208,22 @@ static int __init crossbar_of_init(struct device_node *node)
> >>  		reserved += size;
> >>  	}
> >>  
> >> +	/* Skip the ones marked as unused */
> >> +	if (data) {
> >> +		irqsk = data->irqs_unused;
> >> +		size = data->size;
> >> +
> >> +		for (i = 0; i < size; i++) {
> >> +			entry = irqsk[i];
> >> +
> >> +			if (entry > max) {
> >> +				pr_err("Invalid skip entry\n");
> >> +				goto err3;
> >> +			}
> >> +			cb->irq_map[entry] = IRQ_SKIP;
> >> +		}
> >> +	}
> >> +
> >>  	register_routable_domain_ops(&routable_irq_domain_ops);
> >>  	return 0;
> >>  
> >> @@ -208,18 +238,27 @@ err1:
> >>  	return -ENOMEM;
> >>  }
> >>  
> >> +/* irq number 10 cannot be used because of hw bug */
> >> +int dra_irqs_unused[] = { 10 };
> >> +struct crossbar_data cb_dra_data = { dra_irqs_unused,
> >> +				     ARRAY_SIZE(dra_irqs_unused) };
> >> +
> >>  static const struct of_device_id crossbar_match[] __initconst = {
> >> -	{ .compatible = "ti,irq-crossbar" },
> >> +	{ .compatible = "ti,irq-crossbar", .data = &cb_dra_data },
> >>  	{}
> >>  };
> > 
> > This is a bug in all implementations of this IP?  Or, a specific
> > SoC's implementation?  Would this be better expressed in the dts via a
> > property?  Can we expect future implementations to be fixed?
> > 
> > thx,
> > 
> > Jason.
>  Infact this and PATCH#10 should be merged. I will change that.
> 
>  So in Socs's (2 so far) that do have a crossbar, some irqs are mapped
>  through a crossbar and some are directly wired to the irqchip.
>  These 'unused irqs' are those which are directly wired but they still
>  have a crossbar register. Their routing cannot be changed. So this
>  is not really expected usage of the crossbar hw ip. We initially thought
>  having a dts property separately for this, but took this path to avoid
>  loading the dts with additional bindings which may not be generic.

How do you plan to handle future SoCs with this IP and possibly
different hard-wired irqs?

thx,

Jason.
R Sricharan June 13, 2014, 6:31 a.m. UTC | #6
On Thursday 12 June 2014 07:27 PM, Tony Lindgren wrote:
> * Jason Cooper <jason@lakedaemon.net> [140612 05:52]:
>> On Thu, Jun 12, 2014 at 05:23:11PM +0530, Sricharan R wrote:
>>> From: Nishanth Menon <nm@ti.com>
>>>
>>> When, in the system due to varied reasons, interrupts might be unusable
>>> due to hardware behavior, but register maps do exist, then those interrupts
>>> should be skipped while mapping irq to crossbars.
>>>
>>> Signed-off-by: Nishanth Menon <nm@ti.com>
>>> Signed-off-by: Sricharan R <r.sricharan@ti.com>
>>> Signed-off-by: Tony Lindgren <tony@atomide.com>
>>
>> Tony, have you applied these somewhere already?
> 
> I think some of these I had applied into a branch ready for
> merging but then it was discovered that further changes
> were needed and the branch was dropped.
> 
> Sricharan, please remove my Signed-off-by from this series.
> If I end up applying it for merging my scripts will add it
> automatically.
> 
 Ok, will remove it.

Regards,
 Sricharan
R Sricharan June 13, 2014, 6:37 a.m. UTC | #7
Hi Jason,

On Thursday 12 June 2014 07:37 PM, Jason Cooper wrote:
> On Thu, Jun 12, 2014 at 06:49:17PM +0530, Sricharan R wrote:
>> Hi Jason,
>>
>> On Thursday 12 June 2014 06:21 PM, Jason Cooper wrote:
>>> On Thu, Jun 12, 2014 at 05:23:11PM +0530, Sricharan R wrote:
>>>> From: Nishanth Menon <nm@ti.com>
>>>>
>>>> When, in the system due to varied reasons, interrupts might be unusable
>>>> due to hardware behavior, but register maps do exist, then those interrupts
>>>> should be skipped while mapping irq to crossbars.
>>>>
>>>> Signed-off-by: Nishanth Menon <nm@ti.com>
>>>> Signed-off-by: Sricharan R <r.sricharan@ti.com>
>>>> Signed-off-by: Tony Lindgren <tony@atomide.com>
>>>
>>> Tony, have you applied these somewhere already?
>>>
>>>> ---
>>>>  drivers/irqchip/irq-crossbar.c |   47 ++++++++++++++++++++++++++++++++++++----
>>>>  1 file changed, 43 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
>>>> index 51d4b87..847f6e3 100644
>>>> --- a/drivers/irqchip/irq-crossbar.c
>>>> +++ b/drivers/irqchip/irq-crossbar.c
>>>> @@ -13,11 +13,13 @@
>>>>  #include <linux/io.h>
>>>>  #include <linux/of_address.h>
>>>>  #include <linux/of_irq.h>
>>>> +#include <linux/of_device.h>
>>>>  #include <linux/slab.h>
>>>>  #include <linux/irqchip/arm-gic.h>
>>>>  
>>>>  #define IRQ_FREE	-1
>>>>  #define IRQ_RESERVED	-2
>>>> +#define IRQ_SKIP	-3
>>>>  #define GIC_IRQ_START	32
>>>>  
>>>>  /*
>>>> @@ -34,6 +36,16 @@ struct crossbar_device {
>>>>  	void (*write) (int, int);
>>>>  };
>>>>  
>>>> +/**
>>>> + * struct crossbar_data: Platform specific data
>>>> + * @irqs_unused: array of irqs that cannot be used because of hw erratas
>>>> + * @size: size of the irqs_unused array
>>>> + */
>>>> +struct crossbar_data {
>>>> +	const uint *irqs_unused;
>>>> +	const uint size;
>>>> +};
>>>> +
>>>>  static struct crossbar_device *cb;
>>>>  
>>>>  static inline void crossbar_writel(int irq_no, int cb_no)
>>>> @@ -119,10 +131,12 @@ const struct irq_domain_ops routable_irq_domain_ops = {
>>>>  	.xlate = crossbar_domain_xlate
>>>>  };
>>>>  
>>>> -static int __init crossbar_of_init(struct device_node *node)
>>>> +static int __init crossbar_of_init(struct device_node *node,
>>>> +				   const struct crossbar_data *data)
>>>>  {
>>>>  	int i, size, max, reserved = 0, entry;
>>>>  	const __be32 *irqsr;
>>>> +	const int *irqsk = NULL;
>>>>  
>>>>  	cb = kzalloc(sizeof(*cb), GFP_KERNEL);
>>>>  
>>>> @@ -194,6 +208,22 @@ static int __init crossbar_of_init(struct device_node *node)
>>>>  		reserved += size;
>>>>  	}
>>>>  
>>>> +	/* Skip the ones marked as unused */
>>>> +	if (data) {
>>>> +		irqsk = data->irqs_unused;
>>>> +		size = data->size;
>>>> +
>>>> +		for (i = 0; i < size; i++) {
>>>> +			entry = irqsk[i];
>>>> +
>>>> +			if (entry > max) {
>>>> +				pr_err("Invalid skip entry\n");
>>>> +				goto err3;
>>>> +			}
>>>> +			cb->irq_map[entry] = IRQ_SKIP;
>>>> +		}
>>>> +	}
>>>> +
>>>>  	register_routable_domain_ops(&routable_irq_domain_ops);
>>>>  	return 0;
>>>>  
>>>> @@ -208,18 +238,27 @@ err1:
>>>>  	return -ENOMEM;
>>>>  }
>>>>  
>>>> +/* irq number 10 cannot be used because of hw bug */
>>>> +int dra_irqs_unused[] = { 10 };
>>>> +struct crossbar_data cb_dra_data = { dra_irqs_unused,
>>>> +				     ARRAY_SIZE(dra_irqs_unused) };
>>>> +
>>>>  static const struct of_device_id crossbar_match[] __initconst = {
>>>> -	{ .compatible = "ti,irq-crossbar" },
>>>> +	{ .compatible = "ti,irq-crossbar", .data = &cb_dra_data },
>>>>  	{}
>>>>  };
>>>
>>> This is a bug in all implementations of this IP?  Or, a specific
>>> SoC's implementation?  Would this be better expressed in the dts via a
>>> property?  Can we expect future implementations to be fixed?
>>>
>>> thx,
>>>
>>> Jason.
>>  Infact this and PATCH#10 should be merged. I will change that.
>>
>>  So in Socs's (2 so far) that do have a crossbar, some irqs are mapped
>>  through a crossbar and some are directly wired to the irqchip.
>>  These 'unused irqs' are those which are directly wired but they still
>>  have a crossbar register. Their routing cannot be changed. So this
>>  is not really expected usage of the crossbar hw ip. We initially thought
>>  having a dts property separately for this, but took this path to avoid
>>  loading the dts with additional bindings which may not be generic.
> 
> How do you plan to handle future SoCs with this IP and possibly
> different hard-wired irqs?
  Yes, that would require adding a new compatible in the above list and dts.
  So if adding a new binding in the dts would be cleaner, then i will change
  it that way.

Regards,
 Sricharan
R Sricharan June 13, 2014, 6:56 a.m. UTC | #8
Hi Jason,

On Thursday 12 June 2014 07:35 PM, Jason Cooper wrote:
> On Thu, Jun 12, 2014 at 06:57:15AM -0700, Tony Lindgren wrote:
>> * Jason Cooper <jason@lakedaemon.net> [140612 05:52]:
>>> On Thu, Jun 12, 2014 at 05:23:11PM +0530, Sricharan R wrote:
>>>> From: Nishanth Menon <nm@ti.com>
>>>>
>>>> When, in the system due to varied reasons, interrupts might be unusable
>>>> due to hardware behavior, but register maps do exist, then those interrupts
>>>> should be skipped while mapping irq to crossbars.
>>>>
>>>> Signed-off-by: Nishanth Menon <nm@ti.com>
>>>> Signed-off-by: Sricharan R <r.sricharan@ti.com>
>>>> Signed-off-by: Tony Lindgren <tony@atomide.com>
>>>
>>> Tony, have you applied these somewhere already?
>>
>> I think some of these I had applied into a branch ready for
>> merging but then it was discovered that further changes
>> were needed and the branch was dropped.
> 
> Ok.
> 
>> Sricharan, please remove my Signed-off-by from this series.
>> If I end up applying it for merging my scripts will add it
>> automatically.
> 
> Do you have other changes outside of irqchip depending on this series?
> If so, I can set up a topic branch for you guys to base off of.
> Otherwise, I'll just apply them to irqchip/core when they're ready.
> 
 There are dts changes which are dependent upon this series.

  http://www.spinics.net/lists/linux-omap/msg108116.html

> Also, Sricharan, when you respin this, please clearly identify (in the
> comment section) those patches that need to be flagged for stable.  It
> would be helpful if they were the first patches in the series as well.

Ok, i will point this out clearly.

Regards,
 Sricharan
R Sricharan June 13, 2014, 11:04 a.m. UTC | #9
On Friday 13 June 2014 12:26 PM, Sricharan R wrote:
> Hi Jason,
> 
> On Thursday 12 June 2014 07:35 PM, Jason Cooper wrote:
>> On Thu, Jun 12, 2014 at 06:57:15AM -0700, Tony Lindgren wrote:
>>> * Jason Cooper <jason@lakedaemon.net> [140612 05:52]:
>>>> On Thu, Jun 12, 2014 at 05:23:11PM +0530, Sricharan R wrote:
>>>>> From: Nishanth Menon <nm@ti.com>
>>>>>
>>>>> When, in the system due to varied reasons, interrupts might be unusable
>>>>> due to hardware behavior, but register maps do exist, then those interrupts
>>>>> should be skipped while mapping irq to crossbars.
>>>>>
>>>>> Signed-off-by: Nishanth Menon <nm@ti.com>
>>>>> Signed-off-by: Sricharan R <r.sricharan@ti.com>
>>>>> Signed-off-by: Tony Lindgren <tony@atomide.com>
>>>>
>>>> Tony, have you applied these somewhere already?
>>>
>>> I think some of these I had applied into a branch ready for
>>> merging but then it was discovered that further changes
>>> were needed and the branch was dropped.
>>
>> Ok.
>>
>>> Sricharan, please remove my Signed-off-by from this series.
>>> If I end up applying it for merging my scripts will add it
>>> automatically.
>>
>> Do you have other changes outside of irqchip depending on this series?
>> If so, I can set up a topic branch for you guys to base off of.
>> Otherwise, I'll just apply them to irqchip/core when they're ready.
>>
>  There are dts changes which are dependent upon this series.
> 
>   http://www.spinics.net/lists/linux-omap/msg108116.html
> 
>> Also, Sricharan, when you respin this, please clearly identify (in the
>> comment section) those patches that need to be flagged for stable.  It
>> would be helpful if they were the first patches in the series as well.
> 
> Ok, i will point this out clearly.
 Infact since the dts node is not present in the older kernel (even now),
 the driver itself is not used. So i feel there is nothing to be flagged
  for stable as such.

Regards,
 Sricharan
Jason Cooper June 13, 2014, 1:10 p.m. UTC | #10
On Fri, Jun 13, 2014 at 12:26:10PM +0530, Sricharan R wrote:
> On Thursday 12 June 2014 07:35 PM, Jason Cooper wrote:
...
> > Do you have other changes outside of irqchip depending on this series?
> > If so, I can set up a topic branch for you guys to base off of.
> > Otherwise, I'll just apply them to irqchip/core when they're ready.
> > 
>  There are dts changes which are dependent upon this series.
> 
>   http://www.spinics.net/lists/linux-omap/msg108116.html

In general, dts changes shouldn't depend on code changes or vice-versa.
If they do, that's an indicator that we're breaking compatibility with
older dtbs.

Looking at the dra7.dtsi changes, we're redefining the interrupt
property, which can't be good. :(

Perhaps a better solution would be to add a property, say 'ti,cross-irq'
that is the exact same format as 'interrupts', but is used by the
crossbar driver?

I'm not convinced of this yet, I suspect we may not actually have a
dependency between the dtsi changes and the code changes.  We would have
the ugly "if you have the crossbar node, 'interrupts' means X, if not it
means Y" in the binding docs.  But the absence of the node prevents the
crossbar driver from re-interpreting the interrupts property.

Have you tried booting all the different scenarios?  eg:

  old dtb, new driver
  new dtb, old driver
  old dtb, old driver
  new dtb, new driver

thx,

Jason.
Jason Cooper June 13, 2014, 1:34 p.m. UTC | #11
On Fri, Jun 13, 2014 at 12:07:49PM +0530, Sricharan R wrote:
> Hi Jason,
> 
> On Thursday 12 June 2014 07:37 PM, Jason Cooper wrote:
> > On Thu, Jun 12, 2014 at 06:49:17PM +0530, Sricharan R wrote:
> >> Hi Jason,
> >>
> >> On Thursday 12 June 2014 06:21 PM, Jason Cooper wrote:
> >>> On Thu, Jun 12, 2014 at 05:23:11PM +0530, Sricharan R wrote:
> >>>> From: Nishanth Menon <nm@ti.com>
> >>>>
> >>>> When, in the system due to varied reasons, interrupts might be unusable
> >>>> due to hardware behavior, but register maps do exist, then those interrupts
> >>>> should be skipped while mapping irq to crossbars.
> >>>>
> >>>> Signed-off-by: Nishanth Menon <nm@ti.com>
> >>>> Signed-off-by: Sricharan R <r.sricharan@ti.com>
> >>>> Signed-off-by: Tony Lindgren <tony@atomide.com>
> >>>
> >>> Tony, have you applied these somewhere already?
> >>>
> >>>> ---
> >>>>  drivers/irqchip/irq-crossbar.c |   47 ++++++++++++++++++++++++++++++++++++----
> >>>>  1 file changed, 43 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
> >>>> index 51d4b87..847f6e3 100644
> >>>> --- a/drivers/irqchip/irq-crossbar.c
> >>>> +++ b/drivers/irqchip/irq-crossbar.c
> >>>> @@ -13,11 +13,13 @@
> >>>>  #include <linux/io.h>
> >>>>  #include <linux/of_address.h>
> >>>>  #include <linux/of_irq.h>
> >>>> +#include <linux/of_device.h>
> >>>>  #include <linux/slab.h>
> >>>>  #include <linux/irqchip/arm-gic.h>
> >>>>  
> >>>>  #define IRQ_FREE	-1
> >>>>  #define IRQ_RESERVED	-2
> >>>> +#define IRQ_SKIP	-3
> >>>>  #define GIC_IRQ_START	32
> >>>>  
> >>>>  /*
> >>>> @@ -34,6 +36,16 @@ struct crossbar_device {
> >>>>  	void (*write) (int, int);
> >>>>  };
> >>>>  
> >>>> +/**
> >>>> + * struct crossbar_data: Platform specific data
> >>>> + * @irqs_unused: array of irqs that cannot be used because of hw erratas
> >>>> + * @size: size of the irqs_unused array
> >>>> + */
> >>>> +struct crossbar_data {
> >>>> +	const uint *irqs_unused;
> >>>> +	const uint size;
> >>>> +};
> >>>> +
> >>>>  static struct crossbar_device *cb;
> >>>>  
> >>>>  static inline void crossbar_writel(int irq_no, int cb_no)
> >>>> @@ -119,10 +131,12 @@ const struct irq_domain_ops routable_irq_domain_ops = {
> >>>>  	.xlate = crossbar_domain_xlate
> >>>>  };
> >>>>  
> >>>> -static int __init crossbar_of_init(struct device_node *node)
> >>>> +static int __init crossbar_of_init(struct device_node *node,
> >>>> +				   const struct crossbar_data *data)
> >>>>  {
> >>>>  	int i, size, max, reserved = 0, entry;
> >>>>  	const __be32 *irqsr;
> >>>> +	const int *irqsk = NULL;
> >>>>  
> >>>>  	cb = kzalloc(sizeof(*cb), GFP_KERNEL);
> >>>>  
> >>>> @@ -194,6 +208,22 @@ static int __init crossbar_of_init(struct device_node *node)
> >>>>  		reserved += size;
> >>>>  	}
> >>>>  
> >>>> +	/* Skip the ones marked as unused */
> >>>> +	if (data) {
> >>>> +		irqsk = data->irqs_unused;
> >>>> +		size = data->size;
> >>>> +
> >>>> +		for (i = 0; i < size; i++) {
> >>>> +			entry = irqsk[i];
> >>>> +
> >>>> +			if (entry > max) {
> >>>> +				pr_err("Invalid skip entry\n");
> >>>> +				goto err3;
> >>>> +			}
> >>>> +			cb->irq_map[entry] = IRQ_SKIP;
> >>>> +		}
> >>>> +	}
> >>>> +
> >>>>  	register_routable_domain_ops(&routable_irq_domain_ops);
> >>>>  	return 0;
> >>>>  
> >>>> @@ -208,18 +238,27 @@ err1:
> >>>>  	return -ENOMEM;
> >>>>  }
> >>>>  
> >>>> +/* irq number 10 cannot be used because of hw bug */
> >>>> +int dra_irqs_unused[] = { 10 };
> >>>> +struct crossbar_data cb_dra_data = { dra_irqs_unused,
> >>>> +				     ARRAY_SIZE(dra_irqs_unused) };
> >>>> +
> >>>>  static const struct of_device_id crossbar_match[] __initconst = {
> >>>> -	{ .compatible = "ti,irq-crossbar" },
> >>>> +	{ .compatible = "ti,irq-crossbar", .data = &cb_dra_data },
> >>>>  	{}
> >>>>  };
> >>>
> >>> This is a bug in all implementations of this IP?  Or, a specific
> >>> SoC's implementation?  Would this be better expressed in the dts via a
> >>> property?  Can we expect future implementations to be fixed?
> >>>
> >>> thx,
> >>>
> >>> Jason.
> >>  Infact this and PATCH#10 should be merged. I will change that.
> >>
> >>  So in Socs's (2 so far) that do have a crossbar, some irqs are mapped
> >>  through a crossbar and some are directly wired to the irqchip.
> >>  These 'unused irqs' are those which are directly wired but they still
> >>  have a crossbar register. Their routing cannot be changed. So this
> >>  is not really expected usage of the crossbar hw ip. We initially thought
> >>  having a dts property separately for this, but took this path to avoid
> >>  loading the dts with additional bindings which may not be generic.
> > 
> > How do you plan to handle future SoCs with this IP and possibly
> > different hard-wired irqs?
>   Yes, that would require adding a new compatible in the above list and dts.
>   So if adding a new binding in the dts would be cleaner, then i will change
>   it that way.

Yes, unless the DT maintainers have shot the idea down, I'd prefer to
see a separate property.  With the method you currently have, we'll have
to change the compatible when the IP _hasn't_ changed, just because the
SoC was wired differently.

We could trigger on the SoC compatible and maintain a table, but that
seems overly hacky when the dt is supposed to describe the hardware.

thx,

Jason.
Santosh Shilimkar June 13, 2014, 1:35 p.m. UTC | #12
On Friday 13 June 2014 09:10 AM, Jason Cooper wrote:
> On Fri, Jun 13, 2014 at 12:26:10PM +0530, Sricharan R wrote:
>> On Thursday 12 June 2014 07:35 PM, Jason Cooper wrote:
> ...
>>> Do you have other changes outside of irqchip depending on this series?
>>> If so, I can set up a topic branch for you guys to base off of.
>>> Otherwise, I'll just apply them to irqchip/core when they're ready.
>>>
>>  There are dts changes which are dependent upon this series.
>>
>>   http://www.spinics.net/lists/linux-omap/msg108116.html
> 
> In general, dts changes shouldn't depend on code changes or vice-versa.
> If they do, that's an indicator that we're breaking compatibility with
> older dtbs.
>
Thats true. The case with cross-bar though is the feature wasn't
completly supported so far before this series. Perhaps the the initial
bindings should have been marked unstable. 
 
> Looking at the dra7.dtsi changes, we're redefining the interrupt
> property, which can't be good. :(
>
> Perhaps a better solution would be to add a property, say 'ti,cross-irq'
> that is the exact same format as 'interrupts', but is used by the
> crossbar driver?
>
We have gone over those earlier and it was agreed to re-use interrupt
properties and for special cases, define a cross-bar property to describe
it.
 
> I'm not convinced of this yet, I suspect we may not actually have a
> dependency between the dtsi changes and the code changes.  We would have
> the ugly "if you have the crossbar node, 'interrupts' means X, if not it
> means Y" in the binding docs.  But the absence of the node prevents the
> crossbar driver from re-interpreting the interrupts property.
> 
In ideal cross-bar hardware you don't need the assumption "if you have the
crossbar node, 'interrupts' means X, if not it means Y"

It is purely because the cross-bar irq router hardware has few nasty
bugs which needs those special handling. And thats the reason, the
property was added.

> Have you tried booting all the different scenarios?  eg:
> 
>   old dtb, new driver
>   new dtb, old driver
>   old dtb, old driver
>   new dtb, new driver
> 
Old driver wasn't complete as mentioned and hence the above
combinations becomes bit irrelevant.

Regards,
Santosh
Jason Cooper June 13, 2014, 1:41 p.m. UTC | #13
On Fri, Jun 13, 2014 at 09:35:20AM -0400, Santosh Shilimkar wrote:
> On Friday 13 June 2014 09:10 AM, Jason Cooper wrote:
...
> > Have you tried booting all the different scenarios?  eg:
> > 
> >   old dtb, new driver
> >   new dtb, old driver
> >   old dtb, old driver
> >   new dtb, new driver
> > 
> Old driver wasn't complete as mentioned and hence the above
> combinations becomes bit irrelevant.

Ahh, great!  In that case, I think we should be good without the
dependency between the code changes and the dtsi changes.

thx,

Jason.
diff mbox

Patch

diff --git a/drivers/irqchip/irq-crossbar.c b/drivers/irqchip/irq-crossbar.c
index 51d4b87..847f6e3 100644
--- a/drivers/irqchip/irq-crossbar.c
+++ b/drivers/irqchip/irq-crossbar.c
@@ -13,11 +13,13 @@ 
 #include <linux/io.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
+#include <linux/of_device.h>
 #include <linux/slab.h>
 #include <linux/irqchip/arm-gic.h>
 
 #define IRQ_FREE	-1
 #define IRQ_RESERVED	-2
+#define IRQ_SKIP	-3
 #define GIC_IRQ_START	32
 
 /*
@@ -34,6 +36,16 @@  struct crossbar_device {
 	void (*write) (int, int);
 };
 
+/**
+ * struct crossbar_data: Platform specific data
+ * @irqs_unused: array of irqs that cannot be used because of hw erratas
+ * @size: size of the irqs_unused array
+ */
+struct crossbar_data {
+	const uint *irqs_unused;
+	const uint size;
+};
+
 static struct crossbar_device *cb;
 
 static inline void crossbar_writel(int irq_no, int cb_no)
@@ -119,10 +131,12 @@  const struct irq_domain_ops routable_irq_domain_ops = {
 	.xlate = crossbar_domain_xlate
 };
 
-static int __init crossbar_of_init(struct device_node *node)
+static int __init crossbar_of_init(struct device_node *node,
+				   const struct crossbar_data *data)
 {
 	int i, size, max, reserved = 0, entry;
 	const __be32 *irqsr;
+	const int *irqsk = NULL;
 
 	cb = kzalloc(sizeof(*cb), GFP_KERNEL);
 
@@ -194,6 +208,22 @@  static int __init crossbar_of_init(struct device_node *node)
 		reserved += size;
 	}
 
+	/* Skip the ones marked as unused */
+	if (data) {
+		irqsk = data->irqs_unused;
+		size = data->size;
+
+		for (i = 0; i < size; i++) {
+			entry = irqsk[i];
+
+			if (entry > max) {
+				pr_err("Invalid skip entry\n");
+				goto err3;
+			}
+			cb->irq_map[entry] = IRQ_SKIP;
+		}
+	}
+
 	register_routable_domain_ops(&routable_irq_domain_ops);
 	return 0;
 
@@ -208,18 +238,27 @@  err1:
 	return -ENOMEM;
 }
 
+/* irq number 10 cannot be used because of hw bug */
+int dra_irqs_unused[] = { 10 };
+struct crossbar_data cb_dra_data = { dra_irqs_unused,
+				     ARRAY_SIZE(dra_irqs_unused) };
+
 static const struct of_device_id crossbar_match[] __initconst = {
-	{ .compatible = "ti,irq-crossbar" },
+	{ .compatible = "ti,irq-crossbar", .data = &cb_dra_data },
 	{}
 };
 
 int __init irqcrossbar_init(void)
 {
 	struct device_node *np;
-	np = of_find_matching_node(NULL, crossbar_match);
+	const struct of_device_id *of_id;
+	const struct crossbar_data *cdata;
+
+	np = of_find_matching_node_and_match(NULL, crossbar_match, &of_id);
 	if (!np)
 		return -ENODEV;
 
-	crossbar_of_init(np);
+	cdata = of_id->data;
+	crossbar_of_init(np, cdata);
 	return 0;
 }