Re: [Patch v7 3/7] irqchip: xilinx: restructure and use jump label api

2016-11-16 Thread Michal Simek 
On 15.11.2016 17:03, Zubair Lutfullah Kakakhel wrote:
> Hi,
> 
> On 11/15/2016 12:49 PM, Michal Simek wrote:
>> On 14.11.2016 13:13, Zubair Lutfullah Kakakhel wrote:
>>> Add a global structure to house various variables.
>>> And cleanup read/write handling by using jump label api.
>>>
>>> Signed-off-by: Zubair Lutfullah Kakakhel 
>>>
> 
> ...
> 
>>> @@ -138,59 +136,75 @@ static const struct irq_domain_ops
>>> xintc_irq_domain_ops = {
>>>  static int __init xilinx_intc_of_init(struct device_node *intc,
>>>   struct device_node *parent)
>>>  {
>>> -u32 nr_irq, intr_mask;
>>> +u32 nr_irq;
>>>  int ret;
>>> +struct xintc_irq_chip *irqc;
>>>
>>> -intc_baseaddr = of_iomap(intc, 0);
>>> -BUG_ON(!intc_baseaddr);
>>> +if (xintc_irqc) {
>>> +pr_err("irq-xilinx: Multiple instances aren't supported\n");
>>> +return -EINVAL;
>>> +}
>>
>> I don't agree with this.
>> Pretty long time ago we were added support for multiple instances in
>> xilinx private tree.
>> You can look here.
>>
>> https://github.com/Xilinx/linux-xlnx/blob/master/drivers/irqchip/irq-xilinx-intc.c
>>
>>
>> Not sure if this the latest way how to do it but as you can see
>> we were setting up
>> irq_set_handler_data(irq, intc);
>>
>> and then when you need that structure we were calling
>> struct intc *local_intc = irq_data_get_irq_chip_data(d);
>>
>> And that should be it to support multiple instance of this driver.
>>
>> Based on 5/7 you are describing your interrupt subsystem like this.
>>
>> Peripherals --> xilinx_intcontroller -> mips_cpu_int controller
>> If mips_cpu_int has more than one input you can connect more xilinx intc
>> controllers.
>> If not you still have an option to connect
>> xilinx_intcontroller(up to 32 peripherals) -> xilinx_intcontroller(one
>> intc + up to 31 peripherals)  -> mips_cpu_int controller
> 
> That configuration in FPGA is technically possible. Although not
> done/needed in the
> way we use the Xilinx Interrupt Controller IP block in MIPSfpga.
> 
> This series takes the drivers out of arch code and makes it accessible.
> Any further development on the driver would be common to all architectures.
> Support for multiple instances would be a 'new feature'.
> 
> I say this as this series keeps growing and mutating in terms of its scope
> and work.

fair enough - it can be added in separate patch.

> 
> Would it be possible to ack this so that the restructure out of arch code
> can move forward?

I have tested the whole series on Microblaze and I can't see any problem
in running it there.

That's why
Tested-by; Michal Simek 

If everything is right needs to be checked by irqchip experts.

Thanks,
Michal

Re: [Patch v7 3/7] irqchip: xilinx: restructure and use jump label api

2016-11-15 Thread Zubair Lutfullah Kakakhel 

Hi,

On 11/15/2016 12:49 PM, Michal Simek wrote:

On 14.11.2016 13:13, Zubair Lutfullah Kakakhel wrote:

Add a global structure to house various variables.
And cleanup read/write handling by using jump label api.

Signed-off-by: Zubair Lutfullah Kakakhel 



...


@@ -138,59 +136,75 @@ static const struct irq_domain_ops xintc_irq_domain_ops = 
{
 static int __init xilinx_intc_of_init(struct device_node *intc,
 struct device_node *parent)
 {
-   u32 nr_irq, intr_mask;
+   u32 nr_irq;
int ret;
+   struct xintc_irq_chip *irqc;

-   intc_baseaddr = of_iomap(intc, 0);
-   BUG_ON(!intc_baseaddr);
+   if (xintc_irqc) {
+   pr_err("irq-xilinx: Multiple instances aren't supported\n");
+   return -EINVAL;
+   }


I don't agree with this.
Pretty long time ago we were added support for multiple instances in
xilinx private tree.
You can look here.

https://github.com/Xilinx/linux-xlnx/blob/master/drivers/irqchip/irq-xilinx-intc.c

Not sure if this the latest way how to do it but as you can see
we were setting up
irq_set_handler_data(irq, intc);

and then when you need that structure we were calling
struct intc *local_intc = irq_data_get_irq_chip_data(d);

And that should be it to support multiple instance of this driver.

Based on 5/7 you are describing your interrupt subsystem like this.

Peripherals --> xilinx_intcontroller -> mips_cpu_int controller
If mips_cpu_int has more than one input you can connect more xilinx intc
controllers.
If not you still have an option to connect
xilinx_intcontroller(up to 32 peripherals) -> xilinx_intcontroller(one
intc + up to 31 peripherals)  -> mips_cpu_int controller


That configuration in FPGA is technically possible. Although not done/needed in 
the
way we use the Xilinx Interrupt Controller IP block in MIPSfpga.

This series takes the drivers out of arch code and makes it accessible.
Any further development on the driver would be common to all architectures.
Support for multiple instances would be a 'new feature'.

I say this as this series keeps growing and mutating in terms of its scope
and work.

Would it be possible to ack this so that the restructure out of arch code
can move forward?

Regards,
ZubairLK



Thanks,
Michal

Re: [Patch v7 3/7] irqchip: xilinx: restructure and use jump label api

2016-11-15 Thread Michal Simek 
On 14.11.2016 13:13, Zubair Lutfullah Kakakhel wrote:
> Add a global structure to house various variables.
> And cleanup read/write handling by using jump label api.
> 
> Signed-off-by: Zubair Lutfullah Kakakhel 
> 
> ---
> V6 -> V7
> Restructure and use jump label api
> Better commit log
> 
> V5 -> V6
> Removed __func__ from printk
> Rebase to v4.9-rc3
> 
> V4 -> V5
> Rebased to v4.9-rc1
> Better error handling
> 
> V3 -> V4
> Better error handling for kzalloc
> Erroring out if the axi intc is probed twice as that isn't
> supported
> 
> V2 -> V3
> New patch. Cleans up driver structure
> ---
>  drivers/irqchip/irq-xilinx-intc.c | 118 
> +-
>  1 file changed, 66 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-xilinx-intc.c 
> b/drivers/irqchip/irq-xilinx-intc.c
> index 096c1ed..7331d8c 100644
> --- a/drivers/irqchip/irq-xilinx-intc.c
> +++ b/drivers/irqchip/irq-xilinx-intc.c
> @@ -14,10 +14,9 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  
> -static void __iomem *intc_baseaddr;
> -
>  /* No one else should require these constants, so define them locally here. 
> */
>  #define ISR 0x00 /* Interrupt Status Register */
>  #define IPR 0x04 /* Interrupt Pending Register */
> @@ -31,27 +30,30 @@ static void __iomem *intc_baseaddr;
>  #define MER_ME (1<<0)
>  #define MER_HIE (1<<1)
>  
> -static unsigned int (*read_fn)(void __iomem *);
> -static void (*write_fn)(u32, void __iomem *);
> +static DEFINE_STATIC_KEY_FALSE(xintc_is_be);
>  
> -static void intc_write32(u32 val, void __iomem *addr)
> -{
> - iowrite32(val, addr);
> -}
> +struct xintc_irq_chip {
> + void__iomem *base;
> + struct  irq_domain *root_domain;
> + u32 intr_mask;
> +};
>  
> -static unsigned int intc_read32(void __iomem *addr)
> -{
> - return ioread32(addr);
> -}
> +static struct xintc_irq_chip *xintc_irqc;
>  
> -static void intc_write32_be(u32 val, void __iomem *addr)
> +static void xintc_write(int reg, u32 data)
>  {
> - iowrite32be(val, addr);
> + if (static_branch_unlikely(_is_be))
> + iowrite32be(data, xintc_irqc->base + reg);
> + else
> + iowrite32(data, xintc_irqc->base + reg);
>  }
>  
> -static unsigned int intc_read32_be(void __iomem *addr)
> +static unsigned int xintc_read(int reg)
>  {
> - return ioread32be(addr);
> + if (static_branch_unlikely(_is_be))
> + return ioread32be(xintc_irqc->base + reg);
> + else
> + return ioread32(xintc_irqc->base + reg);
>  }
>  
>  static void intc_enable_or_unmask(struct irq_data *d)
> @@ -65,21 +67,21 @@ static void intc_enable_or_unmask(struct irq_data *d)
>* acks the irq before calling the interrupt handler
>*/
>   if (irqd_is_level_type(d))
> - write_fn(mask, intc_baseaddr + IAR);
> + xintc_write(IAR, mask);
>  
> - write_fn(mask, intc_baseaddr + SIE);
> + xintc_write(SIE, mask);
>  }
>  
>  static void intc_disable_or_mask(struct irq_data *d)
>  {
>   pr_debug("irq-xilinx: disable: %ld\n", d->hwirq);
> - write_fn(1 << d->hwirq, intc_baseaddr + CIE);
> + xintc_write(CIE, 1 << d->hwirq);
>  }
>  
>  static void intc_ack(struct irq_data *d)
>  {
>   pr_debug("irq-xilinx: ack: %ld\n", d->hwirq);
> - write_fn(1 << d->hwirq, intc_baseaddr + IAR);
> + xintc_write(IAR, 1 << d->hwirq);
>  }
>  
>  static void intc_mask_ack(struct irq_data *d)
> @@ -87,8 +89,8 @@ static void intc_mask_ack(struct irq_data *d)
>   unsigned long mask = 1 << d->hwirq;
>  
>   pr_debug("irq-xilinx: disable_and_ack: %ld\n", d->hwirq);
> - write_fn(mask, intc_baseaddr + CIE);
> - write_fn(mask, intc_baseaddr + IAR);
> + xintc_write(CIE, mask);
> + xintc_write(IAR, mask);
>  }
>  
>  static struct irq_chip intc_dev = {
> @@ -99,15 +101,13 @@ static struct irq_chip intc_dev = {
>   .irq_mask_ack = intc_mask_ack,
>  };
>  
> -static struct irq_domain *root_domain;
> -
>  unsigned int get_irq(void)
>  {
>   unsigned int hwirq, irq = -1;
>  
> - hwirq = read_fn(intc_baseaddr + IVR);
> + hwirq = xintc_read(IVR);
>   if (hwirq != -1U)
> - irq = irq_find_mapping(root_domain, hwirq);
> + irq = irq_find_mapping(xintc_irqc->root_domain, hwirq);
>  
>   pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
>  
> @@ -116,9 +116,7 @@ unsigned int get_irq(void)
>  
>  static int xintc_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t 
> hw)
>  {
> - u32 intr_mask = (u32)d->host_data;
> -
> - if (intr_mask & (1 << hw)) {
> + if (xintc_irqc->intr_mask & (1 << hw)) {
>   irq_set_chip_and_handler_name(irq, _dev,
>   handle_edge_irq, "edge");
>   irq_clear_status_flags(irq, IRQ_LEVEL);
> @@ -138,59 +136,75 @@ static const struct irq_domain_ops

[Patch v7 3/7] irqchip: xilinx: restructure and use jump label api

2016-11-14 Thread Zubair Lutfullah Kakakhel 
Add a global structure to house various variables.
And cleanup read/write handling by using jump label api.

Signed-off-by: Zubair Lutfullah Kakakhel 

---
V6 -> V7
Restructure and use jump label api
Better commit log

V5 -> V6
Removed __func__ from printk
Rebase to v4.9-rc3

V4 -> V5
Rebased to v4.9-rc1
Better error handling

V3 -> V4
Better error handling for kzalloc
Erroring out if the axi intc is probed twice as that isn't
supported

V2 -> V3
New patch. Cleans up driver structure
---
 drivers/irqchip/irq-xilinx-intc.c | 118 +-
 1 file changed, 66 insertions(+), 52 deletions(-)

diff --git a/drivers/irqchip/irq-xilinx-intc.c 
b/drivers/irqchip/irq-xilinx-intc.c
index 096c1ed..7331d8c 100644
--- a/drivers/irqchip/irq-xilinx-intc.c
+++ b/drivers/irqchip/irq-xilinx-intc.c
@@ -14,10 +14,9 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
-static void __iomem *intc_baseaddr;
-
 /* No one else should require these constants, so define them locally here. */
 #define ISR 0x00   /* Interrupt Status Register */
 #define IPR 0x04   /* Interrupt Pending Register */
@@ -31,27 +30,30 @@ static void __iomem *intc_baseaddr;
 #define MER_ME (1<<0)
 #define MER_HIE (1<<1)
 
-static unsigned int (*read_fn)(void __iomem *);
-static void (*write_fn)(u32, void __iomem *);
+static DEFINE_STATIC_KEY_FALSE(xintc_is_be);
 
-static void intc_write32(u32 val, void __iomem *addr)
-{
-   iowrite32(val, addr);
-}
+struct xintc_irq_chip {
+   void__iomem *base;
+   struct  irq_domain *root_domain;
+   u32 intr_mask;
+};
 
-static unsigned int intc_read32(void __iomem *addr)
-{
-   return ioread32(addr);
-}
+static struct xintc_irq_chip *xintc_irqc;
 
-static void intc_write32_be(u32 val, void __iomem *addr)
+static void xintc_write(int reg, u32 data)
 {
-   iowrite32be(val, addr);
+   if (static_branch_unlikely(_is_be))
+   iowrite32be(data, xintc_irqc->base + reg);
+   else
+   iowrite32(data, xintc_irqc->base + reg);
 }
 
-static unsigned int intc_read32_be(void __iomem *addr)
+static unsigned int xintc_read(int reg)
 {
-   return ioread32be(addr);
+   if (static_branch_unlikely(_is_be))
+   return ioread32be(xintc_irqc->base + reg);
+   else
+   return ioread32(xintc_irqc->base + reg);
 }
 
 static void intc_enable_or_unmask(struct irq_data *d)
@@ -65,21 +67,21 @@ static void intc_enable_or_unmask(struct irq_data *d)
 * acks the irq before calling the interrupt handler
 */
if (irqd_is_level_type(d))
-   write_fn(mask, intc_baseaddr + IAR);
+   xintc_write(IAR, mask);
 
-   write_fn(mask, intc_baseaddr + SIE);
+   xintc_write(SIE, mask);
 }
 
 static void intc_disable_or_mask(struct irq_data *d)
 {
pr_debug("irq-xilinx: disable: %ld\n", d->hwirq);
-   write_fn(1 << d->hwirq, intc_baseaddr + CIE);
+   xintc_write(CIE, 1 << d->hwirq);
 }
 
 static void intc_ack(struct irq_data *d)
 {
pr_debug("irq-xilinx: ack: %ld\n", d->hwirq);
-   write_fn(1 << d->hwirq, intc_baseaddr + IAR);
+   xintc_write(IAR, 1 << d->hwirq);
 }
 
 static void intc_mask_ack(struct irq_data *d)
@@ -87,8 +89,8 @@ static void intc_mask_ack(struct irq_data *d)
unsigned long mask = 1 << d->hwirq;
 
pr_debug("irq-xilinx: disable_and_ack: %ld\n", d->hwirq);
-   write_fn(mask, intc_baseaddr + CIE);
-   write_fn(mask, intc_baseaddr + IAR);
+   xintc_write(CIE, mask);
+   xintc_write(IAR, mask);
 }
 
 static struct irq_chip intc_dev = {
@@ -99,15 +101,13 @@ static struct irq_chip intc_dev = {
.irq_mask_ack = intc_mask_ack,
 };
 
-static struct irq_domain *root_domain;
-
 unsigned int get_irq(void)
 {
unsigned int hwirq, irq = -1;
 
-   hwirq = read_fn(intc_baseaddr + IVR);
+   hwirq = xintc_read(IVR);
if (hwirq != -1U)
-   irq = irq_find_mapping(root_domain, hwirq);
+   irq = irq_find_mapping(xintc_irqc->root_domain, hwirq);
 
pr_debug("irq-xilinx: hwirq=%d, irq=%d\n", hwirq, irq);
 
@@ -116,9 +116,7 @@ unsigned int get_irq(void)
 
 static int xintc_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t 
hw)
 {
-   u32 intr_mask = (u32)d->host_data;
-
-   if (intr_mask & (1 << hw)) {
+   if (xintc_irqc->intr_mask & (1 << hw)) {
irq_set_chip_and_handler_name(irq, _dev,
handle_edge_irq, "edge");
irq_clear_status_flags(irq, IRQ_LEVEL);
@@ -138,59 +136,75 @@ static const struct irq_domain_ops xintc_irq_domain_ops = 
{
 static int __init xilinx_intc_of_init(struct device_node *intc,
 struct device_node *parent)
 {
-   u32 nr_irq, intr_mask;
+   u32 nr_irq;
int ret;
+   struct xintc_irq_chip *irqc;
 
-