Date:   Tue, 7 Nov 2017 11:00:41 -0600
From:   Grygorii Strashko <grygorii.strashko@...com>
To:     Thierry Reding <thierry.reding@...il.com>
CC:     Linus Walleij <linus.walleij@...aro.org>,
        Jonathan Hunter <jonathanh@...dia.com>,
        <linux-gpio@...r.kernel.org>, <linux-tegra@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6 00/12] gpio: Tight IRQ chip integration



On 11/07/2017 05:13 AM, Thierry Reding wrote:
> On Mon, Nov 06, 2017 at 05:13:33PM -0600, Grygorii Strashko wrote:
>>
>>
>> On 11/06/2017 05:18 AM, Thierry Reding wrote:
>>> On Fri, Nov 03, 2017 at 05:30:30PM -0500, Grygorii Strashko wrote:
>>>> Hi
>>>>
>>>> On 11/02/2017 12:49 PM, Thierry Reding wrote:
>>>>> From: Thierry Reding <treding@...dia.com>
>>>>>
>>>>> Hi Linus,
>>>>>
>>>>> here's the latest series of patches that implement the tighter IRQ chip
>>>>> integration. I've dropped the banked infrastructure for now as per the
>>>>> discussion with Grygorii.
>>>>>
>>>>> The first couple of patches are mostly preparatory work in order to
>>>>> consolidate all IRQ chip related fields in a new structure and create
>>>>> the base functionality for adding IRQ chips.
>>>>>
>>>>> After that, I've added the Tegra186 GPIO support patch that makes use of
>>>>> the new tight integration.
>>>>>
>>>>> Changes in v6:
>>>>> - rebased on latest linux-gpio devel branch
>>>>> - one patch dropped due to rebase
>>>>>
>>>>> Changes in v5:
>>>>> - dropped the banked infrastructure patches for now (Grygorii)
>>>>> - allocate interrupts on demand, rather than upfront (Grygorii)
>>>>> - split up the first patch further as requested by Grygorii
>>>>>
>>>>> Not sure what happened in between here. Notes in commit logs indicate
>>>>> that this is actually version 5, but I can't find the cover letter for
>>>>> v3 and v4.
>>>>>
>>>>> Changes in v2:
>>>>> - rename pins to lines for consistent terminology
>>>>> - rename gpio_irq_chip_banked_handler() to
>>>>>      gpio_irq_chip_banked_chained_handler()
>>>>>
>>>>
>>>> Sry, for delayed reply, very sorry.
>>>>
>>>> Patches 1 - 9, 11 : looks good, so
>>>> Acked-by: Grygorii Strashko <grygorii.strashko@...com>
>>>>
>>>> Patch 10 - unfortunately not all my comment were incorporated [1],
>>>> so below diff on top of patch 10
>>>> which illustrates what i want and also converts gpio-omap to use new infra as
>>>> test for this new infra.
>>>>
>>>> Pls, take a look
>>>>
>>>> [1] https://www.spinics.net/lists/linux-tegra/msg31145.html
>>>
>>> Most of the changes I had deemed to be material for follow-up patches
>>> since they aren't immediately relevant to the gpio_irq_chip conversion
>>> nor needed by the Tegra186 patch.
>>>
>>> However, a couple of the hunks below seem like they should be part of
>>> the initial series.
>>>
>>>> -----------><-------------
>>>>   From 210fe3ad7a642691a1b7603e441f6980b10ff2b4 Mon Sep 17 00:00:00 2001
>>>> From: Grygorii Strashko <grygorii.strashko@...com>
>>>> Date: Fri, 3 Nov 2017 17:24:51 -0500
>>>> Subject: [PATCH] [not for merge] gpiolib: update and test new gpioirq chip
>>>>    infra
>>>>
>>>> changes in gpiolib:
>>>>    - move set_parent to gpiochip_irq_map() and use parents instead of map for only
>>>>      one parent
>>>>    - add gpio_irq_chip->first_irq for static IRQ allocation
>>>>    - fix nested = true if parent_handler not set
>>>>    - fix gpiochip_irqchip_remove() if parent_handler not set
>>>>    - get of_node from gpiodev
>>>>    - fix lock_key: drivers do not need to set it, but looks a bit overcomplicated
>>>>      as lock_class_key will be created for each gpiochip_add_data() call.
>>>>      Honestly, do not seem better way :(
>>>>
>>>> GPIO OMAP driver updated to use new gpioirq chip infra and tested on am335x-evm.
>>>> seems it's working - can see irqs from keys.
>>>>
>>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@...com>
>>>> ---
>>>>    drivers/gpio/gpio-omap.c    | 36 ++++++++++++++--------------
>>>>    drivers/gpio/gpiolib.c      | 58 +++++++++++++++++++--------------------------
>>>>    include/linux/gpio/driver.h | 32 ++++++++++++++++++++++++-
>>>>    3 files changed, 73 insertions(+), 53 deletions(-)
>>> [...]
>>>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>>> [...]
>>>> @@ -1642,6 +1644,7 @@ static int gpiochip_irq_map(struct irq_domain *d, unsigned int irq,
>>>>    			    irq_hw_number_t hwirq)
>>>>    {
>>>>    	struct gpio_chip *chip = d->host_data;
>>>> +	int err = 0;
>>>>    
>>>>    	if (!gpiochip_irqchip_irq_valid(chip, hwirq))
>>>>    		return -ENXIO;
>>>> @@ -1658,6 +1661,12 @@ static int gpiochip_irq_map(struct irq_domain *d, unsigned int irq,
>>>>    		irq_set_nested_thread(irq, 1);
>>>>    	irq_set_noprobe(irq);
>>>>    
>>>> +	if (chip->irq.num_parents == 1)
>>>> +		err = irq_set_parent(irq, chip->irq.parents[0]);
>>>> +	else if (chip->irq.map)
>>>> +		err = irq_set_parent(irq, chip->irq.map[hwirq]);
>>>> +	if (err < 0)
>>>> +		return err;
>>>
>>> Yeah, this looks sensible. Both in that this is a slightly better place
>>> to call it and that the handling for num_parents == 1 is necessary, too.
>>>
>>>>    	/*
>>>>    	 * No set-up of the hardware will happen if IRQ_TYPE_NONE
>>>>    	 * is passed as default type.
>>>> @@ -1712,30 +1721,18 @@ static void gpiochip_irq_relres(struct irq_data *d)
>>>>    
>>>>    static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
>>>>    {
>>>> -	unsigned int irq;
>>>> -	int err;
>>>> -
>>>>    	if (!gpiochip_irqchip_irq_valid(chip, offset))
>>>>    		return -ENXIO;
>>>>    
>>>> -	irq = irq_create_mapping(chip->irq.domain, offset);
>>>> -	if (!irq)
>>>> -		return 0;
>>>> -
>>>> -	if (chip->irq.map) {
>>>> -		err = irq_set_parent(irq, chip->irq.map[offset]);
>>>> -		if (err < 0)
>>>> -			return err;
>>>> -	}
>>>> -
>>>> -	return irq;
>>>> +	return irq_create_mapping(chip->irq.domain, offset);
>>>>    }
>>>>    
>>>>    /**
>>>>     * gpiochip_add_irqchip() - adds an IRQ chip to a GPIO chip
>>>>     * @gpiochip: the GPIO chip to add the IRQ chip to
>>>>     */
>>>> -static int gpiochip_add_irqchip(struct gpio_chip *gpiochip)
>>>> +static int gpiochip_add_irqchip_key(struct gpio_chip *gpiochip,
>>>> +				    struct lock_class_key *lock_key)
>>>>    {
>>>>    	struct irq_chip *irqchip = gpiochip->irq.chip;
>>>>    	const struct irq_domain_ops *ops;
>>>> @@ -1753,17 +1750,8 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip)
>>>>    	}
>>>>    
>>>>    	type = gpiochip->irq.default_type;
>>>> -	np = gpiochip->parent->of_node;
>>>> -
>>>> -#ifdef CONFIG_OF_GPIO
>>>> -	/*
>>>> -	 * If the gpiochip has an assigned OF node this takes precedence
>>>> -	 * FIXME: get rid of this and use gpiochip->parent->of_node
>>>> -	 * everywhere
>>>> -	 */
>>>> -	if (gpiochip->of_node)
>>>> -		np = gpiochip->of_node;
>>>> -#endif
>>>> +	/* called from gpiochip_add_data() and is set properly */
>>>> +	np = gpiochip->gpiodev->dev.of_node;
>>>
>>> Indeed, looks like we have this twice now.
>>>
>>>>    
>>>>    	/*
>>>>    	 * Specifying a default trigger is a terrible idea if DT or ACPI is
>>>> @@ -1789,7 +1777,8 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip)
>>>>    		ops = &gpiochip_domain_ops;
>>>>    
>>>>    	gpiochip->irq.domain = irq_domain_add_simple(np, gpiochip->ngpio,
>>>> -						     0, ops, gpiochip);
>>>> +						     gpiochip->irq.first_irq,
>>>> +						     ops, gpiochip);
>>>>    	if (!gpiochip->irq.domain)
>>>>    		return -EINVAL;
>>>
>>> This seems backward. You just spent a lot of time getting rid of all
>>> users of first_irq (it's actually the reason why I dropped one of the
>>> patches from the series, since first_irq is no longer used), so why
>>> reintroduce it?
>>
>> Yes. It required for HW/drivers not supported (or partially supported)
>> SPARSE_IRQ. And it's not exactly the same as dropped base_irq.
>> If SPARSE_IRQ not supported - driver should ensure that proper
>>   Linux IRQ range allocated (or reserved) and pass first_irq number to the gpiolib
>> For example, in case of OMAP GPIO this is required for OMAP1 support and
>> driver has code
>>    irq_base = devm_irq_alloc_descs(bank->chip.parent, -1, 0, bank->width, 0);
>>
>> irq_base makes no sense in case of SPARSE_IRQ compatible driver and
>> therefore was removed from gpiolib to avoid mis-usage - drivers should
>> maintain it by itself if requred.
> 
> But this is not something that is currently being used. I understand
> that you'll want to add this in order to support fixed IRQ numbers on
> OMAP, but I don't see why it would need to be part of this series. It
> will simply be dead code until the OMAP patches get merged.

My opinion is that if this modification will be merged in gpiolib - 
drivers should be able to use it out of the box.

Any way finial decision is up to Linus.

> 
>>>> @@ -1818,9 +1807,9 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip)
>>>>    		}
>>>>    
>>>>    		gpiochip->irq.nested = false;
>>>> -	} else {
>>>> -		gpiochip->irq.nested = true;
>>>>    	}
>>>> +	/* GPIO driver might not specify parent_handler, but it doesn't mean
>>>> +	 * it's irq_nested, as driver may use request_irq() */
>>>
>>> That's certainly how irq_nested is used in gpiolib, though. Interrupts
>>> are considered either cascaded or nested. Maybe this needs clarification
>>> in general?
>>
>> No. Please, take closer look at
>> gpiochip_set_nested_irqchip()
>> gpiochip_set_cascaded_irqchip()
>> gpiochip_set_chained_irqchip()
>> gpiochip_set_nested_irqchip()

Sry, above is copy-paste err:
gpiochip_set_chained_irqchip
gpiochip_set_nested_irqchip
gpiochip_irqchip_add
gpiochip_irqchip_add_nested

>>   and how they are used.
>> Also, take a look on OMAP driver - it doesn't install chained handler.
>>
>> gpiolib now can discover automatically only when child irqs are not nested
>> (nested here means - threaded nested), therefore different APIs were introduced
>> gpiochip_set_chained_irqchip()
>> gpiochip_set_nested_irqchip()
>>
>> So, with your change:
>> - nested = false; can be set auto if parent_handler provided
>> - nested = <set by driver> if no parent_handler provided
>> because with this patch full GPIO IRQ configuration expected to be done
>> from inside  gpiochip_add_data().
> 
> Sorry if I'm being dense. The only user-callable functions currently are
> gpiochip_set_chained_irqchip() and gpiochip_set_nested_irqchip(). Both
> internally call gpiochip_set_cascaded_irqchip() and the only difference
> is that the former passes a parent handler while the latter doesn't. So
> this seems to me like exactly the same thing that gpiochip_add_irqchip()
> does in my series.
> 
> I think there's some ambiguity in how irq_nested is currently being used
> because it really means two things: a) not-chained and b) threaded. So I
> think what we want is to separate these. For a) I think it's pretty much
> solved because we have the parent handler that disambiguates. So instead
> of just setting irq.nested = false when a parent handler is available we
> should probably also WARN if the driver has set irq.nested = true and
> force it to false.

good point.

> 
> Then, as you suggested, leave the else branch open to allow the driver
> to specify whether or not it uses threaded handlers so that
> gpiochip_irq_map() and gpiochip_irq_unmap() will know to call
> irq_set_nested_thread().

correct

> 
> In this case, perhaps it would be better to rename irq.nested to
> irq.threaded. Then we no longer need to check/force irq.nested for
> chained IRQ chips and instead use only irq.threaded to have interrupts
> marked as nested/threaded.

agree

> 
>>>>    
>>>>    	acpi_gpiochip_request_interrupts(gpiochip);
>>>>    
>>>> @@ -1839,7 +1828,7 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip)
>>>>    
>>>>    	acpi_gpiochip_free_interrupts(gpiochip);
>>>>    
>>>> -	if (gpiochip->irq.chip) {
>>>> +	if (gpiochip->irq.chip && gpiochip->irq.parent_handler) {
>>>>    		struct gpio_irq_chip *irq = &gpiochip->irq;
>>>>    		unsigned int i;
>>>>    
>>>
>>> Yeah, this seems like a good idea, though I think this is safe
>>> regardless of irq.parent_handler.
>>>
>>>> @@ -1972,7 +1961,8 @@ EXPORT_SYMBOL_GPL(gpiochip_irqchip_add_key);
>>>>    
>>>>    #else /* CONFIG_GPIOLIB_IRQCHIP */
>>>>    
>>>> -static inline int gpiochip_add_irqchip(struct gpio_chip *gpiochip)
>>>> +static inline int gpiochip_add_irqchip_key(struct gpio_chip *gpiochip,
>>>> +					   struct lock_class_key *lock_key)
>>>>    {
>>>>    	return 0;
>>>>    }
>>>> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
>>>> index 51fc7b0..3254996 100644
>>>> --- a/include/linux/gpio/driver.h
>>>> +++ b/include/linux/gpio/driver.h
>>>> @@ -128,6 +128,15 @@ struct gpio_irq_chip {
>>>>    	 * in IRQ domain of the chip.
>>>>    	 */
>>>>    	unsigned long *valid_mask;
>>>> +
>>>> +	/**
>>>> +	 * @first_irq:
>>>> +	 *
>>>> +	 * Required for static irq allocation.
>>>> +	 * if set irq_domain_add_simple() will allocate and map all IRQs
>>>> +	 * during initialization.
>>>> +	 */
>>>> +	unsigned int first_irq;
>>>
>>> Again, what was the point of removing all users of this if we need to
>>> add it again?
>>>
>>> It seems to me like dynamic allocation should be a prerequisite for
>>> drivers to use this new code, otherwise we'll just end up adding special
>>> cases to this otherwise generic code.
>>
>> The idea, as i see it, is to be able to re-use you change for as much drivers
>> as possible (as I illustrated for OMAP) and in this case we need support backward
>> compatibility with non SPARSE_IRQ compatible drivers.
> 
> Okay, understood. How about we focus on getting the current series
> merged and then we can add this hunk as a preparator patch for the OMAP
> conversion series?

as above -  finial decision is up to Linus.

> 
>>>
>>>>    };
>>>>    
>>>>    static inline struct gpio_irq_chip *to_gpio_irq_chip(struct irq_chip *chip)
>>>> @@ -312,8 +321,29 @@ struct gpio_chip {
>>>>    extern const char *gpiochip_is_requested(struct gpio_chip *chip,
>>>>    			unsigned offset);
>>>>    
>>>> +extern int gpiochip_add_data_key(struct gpio_chip *chip, void *data,
>>>> +				 struct  *irq_lock_key);
>>>> +#ifdef CONFIG_LOCKDEP
>>>> +/*
>>>> + * Lockdep requires that each irqchip instance be created with a
>>>> + * unique key so as to avoid unnecessary warnings. This upfront
>>>> + * boilerplate static inlines provides such a key for each
>>>> + * unique instance which is created now from inside gpiochip_add_data_key().
>>>> + */
>>>> +static inline int gpiochip_add_data(struct gpio_chip *chip, void *data)
>>>> +{
>>>> +	static struct lock_class_key key;
>>>> +
>>>> +	return gpiochip_add_data_key(chip, data, key);
>>>> +}
>>>
>>> This looks like a neat improvement, but I think it can be done in a
>>> follow-up to remove the boilerplate in drivers.
>>
>> Can't agree here - it better to be considered now.
>> Now only two GPIO drivers define lock_class_key:
>> ./drivers/gpio/gpio-bcm-kona.c:static struct lock_class_key gpio_lock_class;
>> ./drivers/gpio/gpio-brcmstb.c:static struct lock_class_key brcmstb_gpio_irq_lock_class;
>>
>> and these drivers do not use gpioirq framework (your tegra driver will be the third).
>>
>> So, if proposed changes will be applied all drivers switched to use it will need to define
>> its own lock_class_key again and it will be step back.
> 
> I think this would be a minor, mostly mechanical refactoring to do as
> follow-up. But since you feel very strongly about it, I'll add that into
> the series.


-- 
regards,
-grygorii

Powered by blists - more mailing lists