linux-mips
[Top] [All Lists]

Re: [PATCH v7 2/4] MIPS: Octeon: Setup irq_domains for interrupts.

To: Rob Herring <robherring2@gmail.com>
Subject: Re: [PATCH v7 2/4] MIPS: Octeon: Setup irq_domains for interrupts.
From: David Daney <david.daney@cavium.com>
Date: Tue, 27 Mar 2012 11:24:51 -0700
Cc: David Daney <ddaney.cavm@gmail.com>, linux-mips@linux-mips.org, ralf@linux-mips.org, devicetree-discuss@lists.ozlabs.org, Grant Likely <grant.likely@secretlab.ca>, Rob Herring <rob.herring@calxeda.com>, linux-kernel@vger.kernel.org
In-reply-to: <4F711E69.1080302@gmail.com>
References: <1332790281-9648-1-git-send-email-ddaney.cavm@gmail.com> <1332790281-9648-3-git-send-email-ddaney.cavm@gmail.com> <4F711E69.1080302@gmail.com>
Sender: linux-mips-bounce@linux-mips.org
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.15) Gecko/20101027 Fedora/3.0.10-1.fc12 Thunderbird/3.0.10
On 03/26/2012 06:56 PM, Rob Herring wrote:
On 03/26/2012 02:31 PM, David Daney wrote:
From: David Daney<david.daney@cavium.com>
[...]
+static bool octeon_irq_ciu_is_edge(unsigned int line, unsigned int bit)
+{
+       bool edge = false;
+
+       if (line == 0)
+               switch (bit) {
+               case 48 ... 49: /* GMX DRP */
+               case 50: /* IPD_DRP */
+               case 52 ... 55: /* Timers */
+               case 58: /* MPI */
+                       edge = true;
+                       break;
+               default:
+                       break;
+               }
+       else /* line == 1 */
+               switch (bit) {
+               case 47: /* PTP */
+                       edge = true;
+                       break;
+               default:
+                       break;
+               }
+       return edge;

Moving in the right direction, but I still don't get why this is not in
the CIU binding as a 3rd cell?

There are a several reasons, in no particular order they are:

o There is no 3rd cell.  The bindings were discussed with Grant here:
  http://www.linux-mips.org/archives/linux-mips/2011-05/msg00355.html

o The edge/level thing cannot be changed, and the irq lines don't leave the SOC, so hard coding it is possible.


+}
+
+struct octeon_irq_gpio_domain_data {
+       unsigned int base_hwirq;
+};
+
+static int octeon_irq_gpio_xlat(struct irq_domain *d,
+                               struct device_node *node,
+                               const u32 *intspec,
+                               unsigned int intsize,
+                               unsigned long *out_hwirq,
+                               unsigned int *out_type)
+{
+       unsigned int type;
+       unsigned int pin;
+       unsigned int trigger;
+       bool set_edge_handler = false;
+       struct octeon_irq_gpio_domain_data *gpiod;
+
+       if (d->of_node != node)
+               return -EINVAL;
+
+       if (intsize<  2)
+               return -EINVAL;
+
+       pin = intspec[0];
+       if (pin>= 16)
+               return -EINVAL;
+
+       trigger = intspec[1];
+
+       switch (trigger) {
+       case 1:
+               type = IRQ_TYPE_EDGE_RISING;
+               set_edge_handler = true;

This is never used.

Right, it was leftover from the previous version.


+               break;
+       case 2:
+               type = IRQ_TYPE_EDGE_FALLING;
+               set_edge_handler = true;
+               break;
+       case 4:
+               type = IRQ_TYPE_LEVEL_HIGH;
+               break;
+       case 8:
+               type = IRQ_TYPE_LEVEL_LOW;
+               break;
+       default:
+               pr_err("Error: (%s) Invalid irq trigger specification: %x\n",
+                      node->name,
+                      trigger);
+               type = IRQ_TYPE_LEVEL_LOW;
+               break;
+       }
+       *out_type = type;

Can't you get rid of the whole switch statement and just do:

*out_type = intspec[1];

That wouldn't catch erroneous values like 6.


[...]
+static int octeon_irq_ciu_map(struct irq_domain *d,
+                             unsigned int virq, irq_hw_number_t hw)
+{
+       unsigned int line = hw>>  6;
+       unsigned int bit = hw&  63;
+
+       if (virq>= 256)
+               return -EINVAL;

Drop this. You should not care what the virq numbers are.


I care that they don't overflow the width of octeon_irq_ciu_to_irq (a u8).

So really I want to say:

   if (virq >= (1 << sizeof (octeon_irq_ciu_to_irq[0][0]))) {
       WARN(...);
       return -EINVAL;
   }


I need a map external to any one irq_domain. The irq handling code handles sources that come from two separate irq_domains, as well as irqs that are not part of any domain.


Thanks for looking at this again,  I will re-spin the patch,
David Daney

<Prev in Thread] Current Thread [Next in Thread>