On Thu, 2017-02-09 at 09:03 +0000, Marc Zyngier wrote:
> On 06/02/17 12:15, Mars Cheng wrote:
> > Originally driver only supports one base. However, MT6797 has
> > more than one bases to configure interrupt polarity. To support
> > possible design change, here comes a solution to use arbitrary
> > number of bases.
> >
> > Signed-off-by: Mars Cheng <mars.cheng@xxxxxxxxxxxx>
> > ---
> > drivers/irqchip/irq-mtk-sysirq.c | 71 +++++++++++++++++++++++++++-----------
> > 1 file changed, 50 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-mtk-sysirq.c b/drivers/irqchip/irq-mtk-sysirq.c
> > index 63ac73b..2645706 100644
> > --- a/drivers/irqchip/irq-mtk-sysirq.c
> > +++ b/drivers/irqchip/irq-mtk-sysirq.c
> > @@ -24,7 +24,9 @@
> >
> > struct mtk_sysirq_chip_data {
> > spinlock_t lock;
> > - void __iomem *intpol_base;
> > + u32 nr_intpol_bases;
> > + void __iomem **intpol_bases;
> > + u32 *intpol_words;
> > };
> >
> > static int mtk_sysirq_set_type(struct irq_data *data, unsigned int type)
> > @@ -33,13 +35,15 @@ static int mtk_sysirq_set_type(struct irq_data *data, unsigned int type)
> > struct mtk_sysirq_chip_data *chip_data = data->chip_data;
> > u32 offset, reg_index, value;
> > unsigned long flags;
> > - int ret;
> > + int ret, i;
> >
> > offset = hwirq & 0x1f;
> > reg_index = hwirq >> 5;
> > + for (i = 0; reg_index >= chip_data->intpol_words[i]; i++)
> > + reg_index -= chip_data->intpol_words[i];
>
> Two questions:
> - What guarantees that two successive regions deal with consecutive
> interrupts? For example, if I have region A that deals with interrupts
> 0-31, what guarantees that region B covers 32-63?
It is guaranteed by mediatek's chip design team. For those chips with
multiple bases, they all have the consecutive interrupts in the next
region.
> - Given that there is a static relation between a region and a hwirq,
> can't you compute this relation at init time, and let set_type be a fast
> path?
>
sure, I can do this to optimize set_type. will do it in v3
> >
> > spin_lock_irqsave(&chip_data->lock, flags);
> > - value = readl_relaxed(chip_data->intpol_base + reg_index * 4);
> > + value = readl_relaxed(chip_data->intpol_bases[i] + reg_index * 4);
> > if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_EDGE_FALLING) {
> > if (type == IRQ_TYPE_LEVEL_LOW)
> > type = IRQ_TYPE_LEVEL_HIGH;
> > @@ -49,7 +53,8 @@ static int mtk_sysirq_set_type(struct irq_data *data, unsigned int type)
> > } else {
> > value &= ~(1 << offset);
> > }
> > - writel(value, chip_data->intpol_base + reg_index * 4);
> > +
> > + writel(value, chip_data->intpol_bases[i] + reg_index * 4);
>
> Why do you have a writel here, while you're using relaxed accessors
> otherwise? Is there anything else that needs to be made visible to the
> irqchip?
>
before we call spin_unlock_irqrestore() in the end of set_type, polarity
should take effect so we use writel() here. This patch did not change
the usage.
> >
> > data = data->parent_data;
> > ret = data->chip->irq_set_type(data, type);
> > @@ -124,8 +129,7 @@ static int __init mtk_sysirq_of_init(struct device_node *node,
> > {
> > struct irq_domain *domain, *domain_parent;
> > struct mtk_sysirq_chip_data *chip_data;
> > - int ret, size, intpol_num;
> > - struct resource res;
> > + int ret, size, intpol_num = 0, nr_intpol_bases, i;
> >
> > domain_parent = irq_find_host(parent);
> > if (!domain_parent) {
> > @@ -133,36 +137,61 @@ static int __init mtk_sysirq_of_init(struct device_node *node,
> > return -EINVAL;
> > }
> >
> > - ret = of_address_to_resource(node, 0, &res);
> > - if (ret)
> > - return ret;
> > -
> > chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL);
> > if (!chip_data)
> > return -ENOMEM;
> >
> > - size = resource_size(&res);
> > - intpol_num = size * 8;
> > - chip_data->intpol_base = ioremap(res.start, size);
> > - if (!chip_data->intpol_base) {
> > - pr_err("mtk_sysirq: unable to map sysirq register\n");
> > - ret = -ENXIO;
> > - goto out_free;
> > + if (of_property_read_u32(node, "#intpol-bases", &nr_intpol_bases))
> > + nr_intpol_bases = 1;
> > +
> > + chip_data->intpol_words =
> > + kcalloc(nr_intpol_bases, sizeof(u32), GFP_KERNEL);
>
> Please keep the assignment on a single line.
>
Got it, but another warning (prefer 75 char in single line) would pop
up. Would you still like me to keep it on a single line?
> > + if (!chip_data->intpol_words) {
> > + ret = -ENOMEM;
> > + goto out_free_chip;
> > + }
> > +
> > + chip_data->intpol_bases =
> > + kcalloc(nr_intpol_bases, sizeof(void __iomem *), GFP_KERNEL);
>
> Same here.
>
> > + if (!chip_data->intpol_bases) {
> > + ret = -ENOMEM;
> > + goto out_free_intpol_words;
> > + }
> > +
> > + for (i = 0; i < nr_intpol_bases; i++) {
> > + struct resource res;
> > +
> > + ret = of_address_to_resource(node, i, &res);
> > + size = resource_size(&res);
> > + intpol_num += size * 8;
> > + chip_data->intpol_words[i] = size / 4;
> > + chip_data->intpol_bases[i] = of_iomap(node, i);
> > + if (ret || !chip_data->intpol_bases[i]) {
> > + pr_err("%s: couldn't map region %d\n",
> > + node->full_name, i);
> > + ret = -ENODEV;
> > + goto out_free_intpol;
> > + }
> > }
> >
> > domain = irq_domain_add_hierarchy(domain_parent, 0, intpol_num, node,
> > &sysirq_domain_ops, chip_data);
> > if (!domain) {
> > ret = -ENOMEM;
> > - goto out_unmap;
> > + goto out_free_intpol;
> > }
> > spin_lock_init(&chip_data->lock);
> >
> > return 0;
> >
> > -out_unmap:
> > - iounmap(chip_data->intpol_base);
> > -out_free:
> > +out_free_intpol:
> > + for (i = 0; i < nr_intpol_bases; i++)
> > + if (chip_data->intpol_bases[i])
> > + iounmap(chip_data->intpol_bases[i]);
> > + kfree(chip_data->intpol_bases);
> > +out_free_intpol_words:
> > + kfree(chip_data->intpol_words);
> > +out_free_chip:
> > kfree(chip_data);
> > return ret;
> > }
> >
>
> Thanks,
>
> M.
Thanks,
Mars Cheng
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
