On Thu, 2017-02-09 at 09:43 +0000, Marc Zyngier wrote:
> On 09/02/17 09:31, Mars Cheng wrote:
> > 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.
>
> Hum. OK. We'll see how long this holds true, I suppose.
>
> >
> >> - 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.
>
> That's not what I mean. writel has a DSB *before* performing the write
> to the device. Do you have any write (to memory) that needs to be made
> visible to the irqchip before the write to the register occurs?
>
> Looking at the code, I'd say no. This is a standard device
> read-modify-write sequence, no in-memory data that needs to make it
> before the write occurs.
>
> So please turn this into a writel_relaxed.
Got it, you are right. will fix this in v3.
>
> >
> >>>
> >>> 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?
>
> rm scripts/checkpatch.pl
>
> Look, no warning! More seriously, if you're really worried about this,
> you can reformat it:
>
> chip_data->intpol_words = kcalloc(nr_intpol_bases,
> sizeof(u32), GFP_KERNEL);
>
> like this.
>
Got it, will fix it in v3. Thanks. :-)
> Thanks,
>
> M.
--
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
