Message-ID: <20131206085931.GA11990@sonymobile.com>
Date:	Fri, 6 Dec 2013 00:59:33 -0800
From:	Bjorn Andersson <bjorn.andersson@...ymobile.com>
To:	Linus Walleij <linus.walleij@...aro.org>
CC:	David Brown <davidb@...eaurora.org>,
	Daniel Walker <dwalker@...o99.com>,
	Bryan Huntsman <bryanh@...eaurora.org>,
	"linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
	Rob Herring <rob.herring@...xeda.com>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Stephen Warren <swarren@...dotorg.org>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Rob Landley <rob@...dley.net>,
	Grant Likely <grant.likely@...aro.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Stephen Boyd <sboyd@...eaurora.org>
Subject: Re: [PATCH 1/3] pinctrl: Add Qualcomm TLMM driver

On Tue 03 Dec 00:50 PST 2013, Linus Walleij wrote:

> On Sun, Nov 24, 2013 at 12:38 AM, Bjorn Andersson
> <bjorn.andersson@...ymobile.com> wrote:
> 
> > This adds a pinctrl, pinmux, pinconf and gpiolib driver for the
> > Qualcomm TLMM block.
> >
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@...ymobile.com>
> 
> Overall this is looking *very* good, using established infrastructure
> and design patterns like per-SoC plugs, utils, generic pinconf etc.
> 
> A few things remain to be fixed for v2 and after that I expect
> to apply the patch for inclusion in kernel v3.14, just putting the
> MSM maintainers into the To: line so they are aware that this is
> what they will need to use.
> 
> Of course that requires the DT bindings to be ACKed by the
> DT people though.
> 

Thanks, glad you liked it :)

> > +/**
> > + * struct msm_pinctrl - state for a pinctrl-msm device
> > + * @dev:            device handle.
> > + * @pctrl:          pinctrl handle.
> > + * @domain:         irqdomain handle.
> > + * @chip:           gpiochip handle.
> > + * @irq:            Summary irq for the TLMM chip.
> 
> I wonder what a summary IRQ is :-)
> We could call it the parent IRQ for the TLMM irq_chip.
> 

OK

> > + * @lock:           Spinlock to protect register resources as well
> > + *                  as msm_pinctrl data structures.
> > + * @enabled_irqs:   Bitmap of currently enabled irqs.
> > + * @dual_edge_irqs: Bitmap of irqs that need sw emulated dual edge
> > + *                  detection.
> > + * @wake_irqs:      Bitmap of irqs with requested as wakeup source.
> 
> So these are bitmaps... see below.
> 
...
> 
> Further in probe() these are allocated like this:
> 
> > +       pctrl->enabled_irqs = devm_kzalloc(pctrl->dev,
> > +                                          sizeof(unsigned long) * BITS_TO_LONGS(chip->ngpio),
> > +                                          GFP_KERNEL);
> > +       if (!pctrl->enabled_irqs) {
> > +               dev_err(pctrl->dev, "Failed to allocate enabled_irqs bitmap\n");
> > +               return -ENOMEM;
> > +       }
> 
> I take it that this can be standardized and a bit simpler using
> bitmap accessors from <linux/bitmap.h>?
> 
> (...)

A bitmap allocation function or at least a BITS_TO_BYTES macro would be
useful. But this seems to be the way that bitmaps are to be dynamically
allocated in the kernel.

> > +static int msm_dt_subnode_to_map(struct pinctrl_dev *pctldev,
> > +                                struct device_node *np,
> > +                                struct pinctrl_map **map,
> > +                                unsigned *reserved_maps,
> > +                                unsigned *num_maps)
> > +{
> > +       ret = of_property_read_string(np, "qcom,function", &function);
> (...)
> > +       ret = of_property_count_strings(np, "qcom,pins");
> (...)
> 
> I guess I need for these to be approved through the binding
> patch (maybe that should be patch 1?) but we have recently
> discussed the possibilty of creating a generic binding for this
> and move to the utils. This driver is a potential candidate to
> take the step and do that, bringing it into the
> drivers/pinctrl/devicetree.c file.
> 
> However I will not require that right now.
> 

Thanks for Stephen, I replaced this entire function with its generic
counterpart.

> > +static int msm_pinmux_enable(struct pinctrl_dev *pctldev,
> > +                            unsigned function,
> > +                            unsigned group)
> > +{
> (...)
> > +       spin_lock_irqsave(&pctrl->lock, flags);
> > +
> > +       val = readl(pctrl->regs + g->ctl_reg);
> > +       val &= ~(0x7 << g->mux_bit);
> > +       val |= i << g->mux_bit;
> > +       writel(val, pctrl->regs + g->ctl_reg);
> > +
> > +       spin_unlock_irqrestore(&pctrl->lock, flags);
> 
> Why do you need a lock around this read-modify-write?
> The pin control core will serialize/marshal the calls to
> this and other functions AFICT (I could be wrong, so check
> me!). Of course it is nice to be on the safe side, but...
> 
> (Same comment on the disable() call)
> 

Both the pinctrl and gpio functions touch the same registers. Hence the
locking.

> > +static int msm_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
> > +{
> (...)
> > +       spin_lock_irqsave(&pctrl->lock, flags);
> > +
> > +       val = readl(pctrl->regs + g->ctl_reg);
> > +       val &= ~BIT(g->oe_bit);
> > +       writel(val, pctrl->regs + g->ctl_reg);
> > +
> > +       spin_unlock_irqrestore(&pctrl->lock, flags);
> > +
> > +       return 0;
> > +}
> 
> For the GPIO API it seems to be necessary to have the lock
> though, as it is asynchronous.
> 

Note that 'ctl_reg' is also used in the pinmux/pinconf functions above.

> > +static int msm_gpio_get(struct gpio_chip *chip, unsigned offset)
> > +{
> > +       const struct msm_pingroup *g;
> > +       struct msm_pinctrl *pctrl = container_of(chip, struct msm_pinctrl, chip);
> > +       u32 val;
> > +
> > +       if (WARN_ON(offset >= pctrl->soc->ngroups))
> > +               return -EINVAL;
> > +
> > +       g = &pctrl->soc->groups[offset];
> > +
> > +       val = readl(pctrl->regs + g->io_reg);
> > +       return val & BIT(g->in_bit);
> 
> Nit: please clamp the returned value to {0,1} using this
> funny pattern:
> 
> return !!(val & BIT(g->in_bit));
> 

OK

> > +static int msm_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
> > +{
> > +       struct msm_pinctrl *pctrl = container_of(chip, struct msm_pinctrl, chip);
> > +
> > +       return irq_create_mapping(pctrl->domain, offset);
> 
> Recently there has been discussions about not using gpio_to_irq()
> to create this mapping, as it is legal to request an IRQ out of a
> node without first requesting the GPIO and translationg it to an
> IRQ, and on that path this function never gets called.
> 
> Intead: call irq_create_mapping() for all valid IRQs in probe() so they
> are all mapped already, and just use irq_find_mapping() here.
> 

For v2 I've adapted this model intead.

> > +static struct irq_chip msm_gpio_irq_chip = {
> > +       .name           = "msmgpio",
> > +       .irq_mask       = msm_gpio_irq_mask,
> > +       .irq_unmask     = msm_gpio_irq_unmask,
> > +       .irq_ack        = msm_gpio_irq_ack,
> > +       .irq_set_type   = msm_gpio_irq_set_type,
> > +       .irq_set_wake   = msm_gpio_irq_set_wake,
> > +};
> 
> This kernel cycle I'm working on flagging GPIO lines used for
> IRQs properly.
> 
> Please implement .irq_startup() and .irq_shutdown() in accordance
> with one of the patches I sent for this, e.g. this:
> http://marc.info/?l=linux-gpio&m=138546071323849&w=2
> Notice that mask()/unmask() has to be called from these startup/shutdown
> functions so as to satisfy the semantics of the interrupt core.
> 

Included in v2.

> > +static irqreturn_t msm_gpio_irq_handler(int irq, void *data)
> > +{
> (...)
> > +       for_each_set_bit(i, pctrl->enabled_irqs, ngpio) {
> > +               g = &pctrl->soc->groups[i];
> > +               val = readl(pctrl->regs + g->intr_status_reg);
> > +               if (val & BIT(g->intr_status_bit))
> > +                       generic_handle_irq(msm_gpio_to_irq(&pctrl->chip, i));
> > +       }
> 
> This may miss IRQs  arriving while the handler is running I think?
> 
> If you look at the idiomatic implementations in drivers/irqchip/*
> we usually do something like this:
> 
> static int handle_one(struct foo *foo, struct pt_regs *regs)
> {
>         u32 stat, irq;
> 
>         while ((stat = readl_relaxed(foo->base + STATUS))) {
>                 irq = ffs(stat) - 1;
>                 handle_IRQ(irq_find_mapping(foo->domain, irq), regs);
>         }
> 
>         return IRQ_HANDLED;
> }
> 
> By re-reading the status register on each iteration of the while()
> clause, we make sure the status is really zero when we exit the loop.
> 

I haven't looked into the details of this before, but this is what I found.

chained_irq_enter acks the parent irq, so any additional edges triggered
in the block should result in a new interrupt. If such interrupt comes
during our execution of the loop IRQS_PENDING would be set for the parent
irq and upon returning from the handler it will be called immediately
again.

If this was not the case, there would always be a race condition between the
last read of the status register and the next time the code "serves"
interrupts; so it does make sense that it would work this way.


Each gpio has its own irq status register, so the idiomatic implementation
would be way more complicated. Looking at other drivers in the tree I found
both solutions, so I hope you're okay with me leaving this as is.


For v2, I did add an comment to highlight what the bitmap is used for here,
i.e. limiting the number of status registers to check.

> If we should be zuper-picky you should actually do something
> like this first:
> 
> if (status == 0) {
>                 do_bad_IRQ(irq, desc);
>                 return IRQ_INVALID;
> }
> 
> It's not super-important maybe, but could avoid some nasty bug hunts
> later in the history of this driver...
> 

Added a call to handle_bad_irq() in v2.

> > +static int msm_gpio_irq_map(struct irq_domain *d, unsigned int irq,
> > +                           irq_hw_number_t hwirq)
> > +{
> > +       struct msm_pinctrl *pctrl = d->host_data;
> > +
> > +       if (!pctrl)
> > +               return -EINVAL;
> > +
> > +       irq_set_chip_and_handler(irq, &msm_gpio_irq_chip, handle_edge_irq);
> > +       set_irq_flags(irq, IRQF_VALID);
> > +       irq_set_chip_data(irq, pctrl);
> > +       irq_set_irq_type(irq, IRQ_TYPE_EDGE_FALLING);
> 
> Is that really part of the semantics for the mapping function?
> 
> If you do as suggested and call create_map() for each valid IRQ
> in probe() you can set the default type there instead.
> 

Gone in v2.

> > +static int msm_gpio_init(struct msm_pinctrl *pctrl)
> > +{
> (...)
> > +       ret = gpiochip_add(&pctrl->chip);
> > +       if (ret) {
> > +               dev_err(pctrl->dev, "Failed register gpiochip\n");
> > +               return ret;
> > +       }
> > +
> > +       pctrl->domain = irq_domain_add_simple(chip->of_node,
> > +                                             chip->ngpio,
> > +                                             0,
> > +                                             &msm_gpio_irq_simple_ops,
> > +                                             pctrl);
> > +       if (!pctrl->domain) {
> > +               dev_err(pctrl->dev, "Failed to register irq domain\n");
> > +               r = gpiochip_remove(&pctrl->chip);
> > +               return -ENOSYS;
> > +       }
> 
> Please use gpiochip_add_pin_range() from <linux/gpio.h>
> here instead of using pinctrl_add_gpio_range() in the pinctrl
> init call. Doing pinctrl_add_gpio_range() is the old way of
> doing things that we want to get rid of.
> 
> The upside with gpiochip_add_pin_range() is that it
> uses relative numbers (offsets) for the GPIOs instead
> of having to know or calculate the global
> GPIO numbers for the range.
> 

Done in v2.

> > +int msm_pinctrl_probe(struct platform_device *pdev,
> > +                     const struct msm_pinctrl_soc_data *soc_data)
> > +{
> (...)
> > +       msm_pinctrl_desc.name = dev_name(&pdev->dev);
> > +       msm_pinctrl_desc.pins = pctrl->soc->pins;
> > +       msm_pinctrl_desc.npins = pctrl->soc->npins;
> > +       pctrl->pctrl = pinctrl_register(&msm_pinctrl_desc, &pdev->dev, pctrl);
> > +       if (!pctrl->pctrl) {
> > +               dev_err(&pdev->dev, "Couldn't register pinctrl driver\n");
> > +               irq_domain_remove(pctrl->domain);
> > +               r = gpiochip_remove(&pctrl->chip);
> > +               return -ENODEV;
> > +       }
> > +
> > +       pinctrl_add_gpio_range(pctrl->pctrl, soc_data->gpio_range);
> 
> So get rid of this if you can, this may require you to twist the
> init order around, so that you register the pin controller first,
> then the gpio chip.
> 
> C.f. drivers/pinctrl/pinctrl-abx500.c
> 
> Those are the only few things standing out. Maybe this
> last thing will be a bit hairy, sorry for that...

Thanks for your review, v2 is on it's way.

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists