| Message ID | 1486383336-16892-3-git-send-email-mars.cheng@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@mediatek.com> > --- > 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? - 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? > > 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? > > 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. > + 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.
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@mediatek.com> > > --- > > 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
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@mediatek.com> >>> --- >>> 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. > >>> >>> 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. Thanks, M.
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@mediatek.com> > >>> --- > >>> 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.
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]; 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); 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); + if (!chip_data->intpol_words) { + ret = -ENOMEM; + goto out_free_chip; + } + + chip_data->intpol_bases = + kcalloc(nr_intpol_bases, sizeof(void __iomem *), GFP_KERNEL); + 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; }
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@mediatek.com> --- drivers/irqchip/irq-mtk-sysirq.c | 71 +++++++++++++++++++++++++++----------- 1 file changed, 50 insertions(+), 21 deletions(-)