Re: [PATCH v4 1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT

[Balaji T K <balajitk@ti.com>]

On Monday 29 July 2013 01:47 PM, Javier Martinez Canillas wrote:
> Hi Alexander,
>
> On Mon, Jul 29, 2013 at 8:41 AM, Alexander Holler <holler@ahsoftware.de> wrote:
>> Am 28.07.2013 21:06, schrieb Javier Martinez Canillas:
>>
>>> On Sun, Jul 28, 2013 at 8:22 PM, Linus Walleij <linus.walleij@linaro.org>
>>> wrote:
>>>>
>>>> On Sun, Jul 28, 2013 at 7:33 PM, Javier Martinez Canillas
>>>> <martinez.javier@gmail.com> wrote:
>>>>
>>>>> According to Documentation/devicetree/bindings/mmc/mmc.txt:
>>>>>
>>>>> cd-gpios: Specify GPIOs for card detection, see gpio binding
>>>>>
>>>>> So it just says that it is a GPIO for card detection and not an IRQ so
>>>>> this assumption comes from either the omap_hsmmc driver or Alexander'
>>>>> DTS is missing something like:
>>>>>
>>>>>                   interrupt-parent = <&gpio6>;
>>>>>                   interrupts = <16 8>;
>>
>>
>> What do the values 16 and 8 mean here? GPIO numbers?
>> And where do I have to place that?
>>
>
> If you look at Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> for the two cells interrupt controllers definition:
>
>    b) two cells
>    ------------
>    The #interrupt-cells property is set to 2 and the first cell defines the
>    index of the interrupt within the controller, while the second cell is used
>    to specify any of the following flags:
>      - bits[3:0] trigger type and level flags
>          1 = low-to-high edge triggered
>          2 = high-to-low edge triggered
>          4 = active high level-sensitive
>          8 = active low level-sensitive
>
> So, the first cell in this example means the 16th GPIO in the omap
> gpio6 controller using the edge/level flag IRQ_TYPE_LEVEL_LOW.
>
>> I've now tried the following:
>>
>> --
>> &mmc1 {
>>          vmmc-supply = <&vmmcsd_fixed>;
>>          status = "okay";
>>          pinctrl-names = "default";
>>          pinctrl-0 = <&mmc1_pins>; /* pinmux for GPIO0__6 */
>>          interrupt-parent = <&gpio0>;
>>          interrupts = <6>;
>>
>>          cd-gpios = <&gpio0 6 GPIO_ACTIVE_HIGH>;
>>          cd-inverted;
>> };
>> --
>> (the gpio which should be used for the IRQ is GPIO0_6)
>>
>> The result was that the gpio_request() failed with -EBUSY.
>> Then I've commented out that, because it isn't necessary anymore as the
>> gpio should be set as input automatically (as I've understood the commit
>> msg).
>>
>
> Indeed, drivers shouldn't explicitly call gpio_request() when booting
> with DT. This only made sense with legacy boot. So maybe the
> omap_hsmmc driver has not been completely converted to be used with DT
> booting when using the "cd-gpios" property?
>
>> The result was
>>
>> [    1.397100] genirq: Flags mismatch irq 144. 00002003 (mmc0) vs.
>> 00000000 (mmc0)
>>
>> and request_threaded_irq() returned with -EBUSY.
>
> By looking at kernel/irq/manage.c, this error comes from __setup_irq()
> because the same interrupt line is tried to be shared by two users
> with different trigger (edge/level) type flags. Don't know what is
> triggering that with your DTS though.
>
>>
>>
>> To stop that discussion about some "non-standard" dts I'm using (I wonder
>> where the standard is), I try to formulate a clear question:
>>
>> If a driver uses
>> --
>> irq = gpio_to_irq(some_gpio_number);
>> /*
>> gpio_request();
>> gpio_direction_input();
>> */
>> request_threaded_irq(irq);
>> --
>>
>> How should the dts or the driver be changed that this works with 3.11-rc2?
>>
>
> Drivers shouldn't do that IMHO, they should just use IRQ (virtual)
> numbers and request them. Drivers shouldn't care whether it is a GPIO
> line acting as an IRQ number or a real IRQ from an interrupt
> controller.
>
> My understanding is that defining:
>
> interrupt-parent = <&phandle>;
> interrupts = <index type_flags>;
>
> *should* be enough to make the sequence you are mentioning to work.
> Now, you said that this breaks your DTS when using the "cd-gpios"
> property with the omap hsmmc driver. Unfortunately I'm not familiar
> with neither the omap hsmmc driver nor the mmc "cd-gpios" property so
> I can't be of too much help here. I guess Balaji can take a step
> forward here and shed some light on this.
>
> My guess is that it was working on your setup because the omap hsmmc
> driver is handling GPIO-IRQ in the same way that board files do and it
> was not even described in your DTS because support for GPIO-IRQ was
> not really working for DT in OMAP (an explicit call to gpio_request
> had to be made anyways).
>
> So, I would like to first check if this really is a regression (bug)
> introduced by these changes or is an issue with the omap hsmmc driver,
> how it handles the "cd-gpios" binding or your DTS definition and
> reverting the patch-set will just hide the real problem.
>
Hi,

omap_hsmmc received the gpio number from cd-gpios and configured
it for card detect gpio interrupt. omap_hsmmc driver did gpio_request(),
gpio_direction_input() as it was required to do so, as in legacy board
case.

> As I said, I don't mind if these patches are reverted although many
> people (Jon, Santosh, Grant and myself) think that this is the right
> approach and reverting them will break DTS for OMAP boards that are
> already in mainline.

Since cd-gpios dts binding for omap boards is not in mainline,
omap_hsmmc driver can be patched with new binding added.

>
> Yes, it is a PITA that things get broken but is the price of progress
> I think, I'll prefer fixing them than just blindly reverting patches
> because they show hidden issues. If I'm proven to be wrong I'll be
> happy if these patches get reverted though.
>
>> Regards,
>>
>> Alexander Holler
>
> Thanks a lot and best regards,
> Javier
>