Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1440043 > unrolled thread

[PATCH v2 00/10] ARM: NUC900: Add NUC970 SoC support

Started byWan Zongshun <vw@iommu.org>
First post2016-07-10 09:30 +0200
Last post2016-07-12 00:20 +0200
Articles 11 on this page of 31 — 7 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 00/10] ARM: NUC900: Add NUC970 SoC support Wan Zongshun <vw@iommu.org> - 2016-07-10 09:30 +0200
    [PATCH v2 01/10] ARM: NUC900: Add nuc970 machine support Wan Zongshun <vw@iommu.org> - 2016-07-10 09:30 +0200
      Re: [PATCH v2 01/10] ARM: NUC900: Add nuc970 machine support Arnd Bergmann <arnd@arndb.de> - 2016-07-11 00:10 +0200
      Re: [PATCH v2 01/10] ARM: NUC900: Add nuc970 machine support Arnd Bergmann <arnd@arndb.de> - 2016-07-11 18:10 +0200
        Re: [PATCH v2 01/10] ARM: NUC900: Add nuc970 machine support Wan Zongshun <vw@iommu.org> - 2016-07-12 06:40 +0200
          Re: [PATCH v2 01/10] ARM: NUC900: Add nuc970 machine support Wan Zongshun <vw@iommu.org> - 2016-07-12 09:20 +0200
            Re: [PATCH v2 01/10] ARM: NUC900: Add nuc970 machine support Arnd Bergmann <arnd@arndb.de> - 2016-07-12 10:30 +0200
    [PATCH v2 03/10] Clocksource: add nuc970 clocksource driver Wan Zongshun <vw@iommu.org> - 2016-07-10 09:30 +0200
      Re: [PATCH v2 03/10] Clocksource: add nuc970 clocksource driver Arnd Bergmann <arnd@arndb.de> - 2016-07-11 17:40 +0200
        Re: [PATCH v2 03/10] Clocksource: add nuc970 clocksource driver Wan Zongshun <vw@iommu.org> - 2016-07-12 09:40 +0200
          Re: [PATCH v2 03/10] Clocksource: add nuc970 clocksource driver Arnd Bergmann <arnd@arndb.de> - 2016-07-12 10:30 +0200
            Re: [PATCH v2 03/10] Clocksource: add nuc970 clocksource driver Arnd Bergmann <arnd@arndb.de> - 2016-07-21 15:00 +0200
            Re: [PATCH v2 03/10] Clocksource: add nuc970 clocksource driver Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-07-21 15:00 +0200
    [PATCH v2 02/10] irqchip: add irqchip driver for nuc900 Wan Zongshun <vw@iommu.org> - 2016-07-10 09:30 +0200
      Re: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900 Paul Gortmaker <paul.gortmaker@windriver.com> - 2016-07-11 00:00 +0200
        Re: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900 Wan Zongshun <vw@iommu.org> - 2016-07-11 04:20 +0200
      Re: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900 Arnd Bergmann <arnd@arndb.de> - 2016-07-11 17:50 +0200
        Re: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900 Wan Zongshun <vw@iommu.org> - 2016-07-12 09:10 +0200
          Re: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900 Arnd Bergmann <arnd@arndb.de> - 2016-07-12 10:30 +0200
            Re: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900 Wan Zongshun <vw@iommu.org> - 2016-07-14 11:00 +0200
              Re: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900 Arnd Bergmann <arnd@arndb.de> - 2016-07-14 13:20 +0200
      Re: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900 Jason Cooper <jason@lakedaemon.net> - 2016-07-13 22:10 +0200
        Re: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900 Wan Zongshun <vw@iommu.org> - 2016-07-14 05:40 +0200
          Re: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900 Jason Cooper <jason@lakedaemon.net> - 2016-07-14 16:00 +0200
            Re: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900 Wan Zongshun <vw@iommu.org> - 2016-07-15 07:20 +0200
              Re: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900 Arnd Bergmann <arnd@arndb.de> - 2016-07-15 09:10 +0200
                Re: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900 Wan ZongShun <mcuos.com@gmail.com> - 2016-07-15 11:50 +0200
                  Re: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900 Jason Cooper <jason@lakedaemon.net> - 2016-07-15 17:50 +0200
                  Re: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900 Arnd Bergmann <arnd@arndb.de> - 2016-07-21 13:00 +0200
                    Re: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900 Jason Cooper <jason@lakedaemon.net> - 2016-07-21 20:50 +0200
    Re: [PATCH v2 04/10] clk: add Clock driver for nuc970 Michael Turquette <mturquette@baylibre.com> - 2016-07-12 00:20 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1443360 — Re: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900

FromArnd Bergmann <arnd@arndb.de>
Date2016-07-14 13:20 +0200
SubjectRe: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900
Message-ID<rUMQp-xN-3@gated-at.bofh.it>
In reply to#1443296
On Thursday, July 14, 2016 4:52:29 PM CEST Wan Zongshun wrote:
> 
> On 2016年07月12日 16:26, Arnd Bergmann wrote:
> > On Tuesday, July 12, 2016 3:04:42 PM CEST Wan Zongshun wrote:
> >>>
> >>> Ideally, this should just go away once we use SPARSE_IRQ.
> >>
> >> This platform also can use SPARSE_IRQ? this just a simple irq map and no
> >> more irq number in this Soc.
> >>
> >
> > SPARSE_IRQ is implied by ARCH_MULTIPLATFORM, so we will have to
> > use it once that gets enabled.
> >
> > Your new irqchip driver already handles IRQ domains, so it will
> > work out of the box with SPARSE_IRQ, but you have to change the
> > reference to "NR_IRQS" into something else.
> >
> > I've prototyped a patch series to enable ARCH_MULTIPLATFORM,
> > I hope you can start working from what I have and get it to run.
> 
> I go through the ARCH_MULTIPLATFORM and SPARSE_IRQ related codes, but I 
> find I also have to define the NUC900_NR_IRQS firstly like below, so 
> that I can init the .nr_irq.
> 
> +#if !defined(CONFIG_SOC_NUC970)
>   #define NUC900_NR_IRQS		(IRQ_ADC+1)
> +#else
> +#define NUC900_NR_IRQS		62
> +#endif
> 
>   DT_MACHINE_START(nuc900_dt, "Nuvoton NUC900 (Device Tree Support)")
>          .dt_compat      = nuc900_dt_compat,
> +       .nr_irqs        = NUC900_NR_IRQS,
>   MACHINE_END

You don't need to set this for the DT based machines, this number
is just for the set of IRQ that have a hardcoded mapping. With DT,
they get dynamically allocated as required.

For the board files, you can hardcode the original definition of 32
IRQs, but I think you don't need that if you register a legacy IRQ
domain in mach-w90x900/irq.c.

> and then in my irqchip driver, I will use the NUC900_NR_IRQS:
> 
> +aic_domain = irq_domain_add_linear(node, NUC900_NR_IRQS,
> +				    &aic_irq_domain_ops, NULL);
> 
> 
> Is that a right usage?

This does not look right when NUC900_NR_IRQS can have configuration
dependent values. I can see two ways of handling it:

a) register the maximum number of IRQs that the irqchip can handle.
   There is no real cost for having a large number here, as SPARSE_IRQ
   ensures we only need to allocate the descriptors that are actually
   used.

b) make the number of interrupts dependent on the compatible string
   for the irqchip, and handle NUC970 differently from the others
   in the driver.

In the meantime, I also have a series to enable multiplatform support
for all of mach-w90x900 based on your patches, but lacking a proper
clk driver. I'll send that to you so you can include it in your
series (after verifying that it works, or fixing it where necessary).

	Arnd

[toc] | [prev] | [next] | [standalone]

#1442801 — Re: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900

FromJason Cooper <jason@lakedaemon.net>
Date2016-07-13 22:10 +0200
SubjectRe: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900
Message-ID<rUyDL-7RI-11@gated-at.bofh.it>
In reply to#1440046
Hi Wan Zongshun,

On Sun, Jul 10, 2016 at 03:27:22PM +0800, Wan Zongshun wrote:
> This patch is to add irqchip driver support for nuc900 plat,
> current this driver only supports nuc970 SoC.
> 
> Signed-off-by: Wan Zongshun <mcuos.com@gmail.com>
> ---
>  arch/arm/mach-w90x900/include/mach/irqs.h |   5 +
>  drivers/irqchip/Makefile                  |   1 +
>  drivers/irqchip/irq-nuc900.c              | 150 ++++++++++++++++++++++++++++++
>  3 files changed, 156 insertions(+)
>  create mode 100644 drivers/irqchip/irq-nuc900.c
> 
> diff --git a/arch/arm/mach-w90x900/include/mach/irqs.h b/arch/arm/mach-w90x900/include/mach/irqs.h
> index 9d5cba3..3b035c6 100644
> --- a/arch/arm/mach-w90x900/include/mach/irqs.h
> +++ b/arch/arm/mach-w90x900/include/mach/irqs.h
> @@ -59,7 +59,12 @@
>  #define IRQ_KPI		W90X900_IRQ(29)
>  #define IRQ_P2SGROUP	W90X900_IRQ(30)
>  #define IRQ_ADC		W90X900_IRQ(31)
> +
> +#if !defined(CONFIG_SOC_NUC900)
>  #define NR_IRQS		(IRQ_ADC+1)
> +#else
> +#define NR_IRQS		62
> +#endif

Arnd already covered this...

>  
>  /*for irq group*/
>  
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 38853a1..9ccd5af8a 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -69,3 +69,4 @@ obj-$(CONFIG_PIC32_EVIC)		+= irq-pic32-evic.o
>  obj-$(CONFIG_MVEBU_ODMI)		+= irq-mvebu-odmi.o
>  obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
>  obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
> +obj-$(CONFIG_SOC_NUC970)		+= irq-nuc900.o
> diff --git a/drivers/irqchip/irq-nuc900.c b/drivers/irqchip/irq-nuc900.c
> new file mode 100644
> index 0000000..c4b2e39
> --- /dev/null
> +++ b/drivers/irqchip/irq-nuc900.c
> @@ -0,0 +1,150 @@
> +/*
> + * Copyright 2016 Wan Zongshun <mcuos.com@gmail.com>
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqdomain.h>
> +#include <linux/io.h>
> +#include <linux/ioport.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +
> +#include <asm/exception.h>
> +#include <asm/hardirq.h>
> +
> +#define	REG_AIC_SCR1	0x00
> +#define	REG_AIC_SCR2	0x04
> +#define	REG_AIC_SCR3	0x08
> +#define	REG_AIC_SCR4	0x0C
> +#define	REG_AIC_SCR5	0x10
> +#define	REG_AIC_SCR6	0x14
> +#define	REG_AIC_SCR7	0x18
> +#define	REG_AIC_SCR8	0x1C
> +#define	REG_AIC_SCR9	0x20
> +#define	REG_AIC_SCR10	0x24
> +#define	REG_AIC_SCR11	0x28
> +#define	REG_AIC_SCR12	0x2C
> +#define	REG_AIC_SCR13	0x30
> +#define	REG_AIC_SCR14	0x34
> +#define	REG_AIC_SCR15	0x38
> +#define	REG_AIC_IRSR	0x100
> +#define	REG_AIC_IRSRH	0x104
> +#define	REG_AIC_IASR	0x108
> +#define	REG_AIC_IASRH	0x10C
> +#define	REG_AIC_ISR	0x110
> +#define	REG_AIC_ISRH	0x114
> +#define	REG_AIC_IPER	0x118
> +#define	REG_AIC_ISNR	0x120
> +#define	REG_AIC_OISR	0x124
> +#define	REG_AIC_IMR	0x128
> +#define	REG_AIC_IMRH	0x12C
> +#define	REG_AIC_MECR	0x130
> +#define	REG_AIC_MECRH	0x134
> +#define	REG_AIC_MDCR	0x138
> +#define	REG_AIC_MDCRH	0x13C
> +#define	REG_AIC_SSCR	0x140
> +#define	REG_AIC_SSCRH	0x144
> +#define	REG_AIC_SCCR	0x148
> +#define	REG_AIC_SCCRH	0x14C
> +#define	REG_AIC_EOSCR	0x150

Please remove unused defines.

> +
> +static void __iomem *aic_base;
> +static struct irq_domain *aic_domain;
> +
> +static void nuc900_irq_mask(struct irq_data *d)
> +{
> +	if (d->irq < 32)
> +		writel(1 << (d->irq), aic_base + REG_AIC_MDCR);
> +	else
> +		writel(1 << (d->irq - 32), aic_base + REG_AIC_MDCRH);
> +}
> +
> +static void nuc900_irq_ack(struct irq_data *d)
> +{
> +	writel(0x01, aic_base + REG_AIC_EOSCR);
> +}
> +
> +static void nuc900_irq_unmask(struct irq_data *d)
> +{
> +	if (d->irq < 32)
> +		writel(1 << (d->irq), aic_base + REG_AIC_MECR);
> +	else
> +		writel(1 << (d->irq - 32), aic_base + REG_AIC_MECRH);
> +}
> +
> +static struct irq_chip nuc900_irq_chip = {
> +	.irq_ack	= nuc900_irq_ack,
> +	.irq_mask	= nuc900_irq_mask,
> +	.irq_unmask	= nuc900_irq_unmask,
> +};
> +
> +void __exception_irq_entry aic_handle_irq(struct pt_regs *regs)
> +{
> +	u32 hwirq;
> +
> +	hwirq = readl(aic_base + REG_AIC_IPER);
> +	hwirq = readl(aic_base + REG_AIC_ISNR);
> +	if (!hwirq)
> +		writel(0x01, aic_base + REG_AIC_EOSCR);

Could you explain what's going on here?

> +
> +	handle_IRQ((irq_find_mapping(aic_domain, hwirq)), regs);
> +}
> +
> +static int aic_irq_domain_map(struct irq_domain *d, unsigned int virq,
> +			      irq_hw_number_t hw)
> +{
> +	irq_set_chip_and_handler(virq, &nuc900_irq_chip, handle_level_irq);
> +	irq_clear_status_flags(virq, IRQ_NOREQUEST);
> +
> +	return 0;
> +}
> +
> +static struct irq_domain_ops aic_irq_domain_ops = {
> +	.map = aic_irq_domain_map,
> +	.xlate = irq_domain_xlate_onecell,
> +};
> +
> +static int __init aic_of_init(struct device_node *node,
> +			      struct device_node *parent)
> +{
> +	int ret;
> +
> +	aic_base = of_iomap(node, 0);
> +	if (!aic_base) {
> +		ret = -ENOMEM;
> +		pr_err("%s: unable to map registers\n", node->full_name);
> +		goto err_iomap;
> +	}
> +
> +	writel(0xFFFFFFFC, aic_base + REG_AIC_MDCR);
> +	writel(0xFFFFFFFF, aic_base + REG_AIC_MDCRH);
> +
> +	aic_domain = irq_domain_add_linear(node, NR_IRQS,
> +					   &aic_irq_domain_ops, NULL);
> +
> +	if (!aic_domain) {
> +		ret = -ENOMEM;
> +		pr_err("%s: unable to create IRQ domain\n", node->full_name);
> +		goto err_aic_domain;
> +	}
> +
> +	set_handle_irq(aic_handle_irq);
> +	return 0;
> +
> +err_aic_domain:
> +	iounmap(aic_base);
> +err_iomap:
> +	return ret;
> +}
> +
> +IRQCHIP_DECLARE(nuc900, "nuvoton,nuc900-aic", aic_of_init);

afaict, there is no 'nuc900', that's a family.  If so, this compatible
needs to be 'nuvoton,nuc970-aic'.

thx,

Jason.

[toc] | [prev] | [next] | [standalone]

#1443055 — Re: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900

FromWan Zongshun <vw@iommu.org>
Date2016-07-14 05:40 +0200
SubjectRe: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900
Message-ID<rUFFg-43M-19@gated-at.bofh.it>
In reply to#1442801

On 2016年07月14日 04:09, Jason Cooper wrote:
> Hi Wan Zongshun,
>
> On Sun, Jul 10, 2016 at 03:27:22PM +0800, Wan Zongshun wrote:
>> This patch is to add irqchip driver support for nuc900 plat,
>> current this driver only supports nuc970 SoC.
>>
>> Signed-off-by: Wan Zongshun <mcuos.com@gmail.com>
>> ---
>>   arch/arm/mach-w90x900/include/mach/irqs.h |   5 +
>>   drivers/irqchip/Makefile                  |   1 +
>>   drivers/irqchip/irq-nuc900.c              | 150 ++++++++++++++++++++++++++++++
>>   3 files changed, 156 insertions(+)
>>   create mode 100644 drivers/irqchip/irq-nuc900.c
>>
>> diff --git a/arch/arm/mach-w90x900/include/mach/irqs.h b/arch/arm/mach-w90x900/include/mach/irqs.h
>> index 9d5cba3..3b035c6 100644
>> --- a/arch/arm/mach-w90x900/include/mach/irqs.h
>> +++ b/arch/arm/mach-w90x900/include/mach/irqs.h
>> @@ -59,7 +59,12 @@
>>   #define IRQ_KPI		W90X900_IRQ(29)
>>   #define IRQ_P2SGROUP	W90X900_IRQ(30)
>>   #define IRQ_ADC		W90X900_IRQ(31)
>> +
>> +#if !defined(CONFIG_SOC_NUC900)
>>   #define NR_IRQS		(IRQ_ADC+1)
>> +#else
>> +#define NR_IRQS		62
>> +#endif
>
> Arnd already covered this...
>
>>
>>   /*for irq group*/
>>
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index 38853a1..9ccd5af8a 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -69,3 +69,4 @@ obj-$(CONFIG_PIC32_EVIC)		+= irq-pic32-evic.o
>>   obj-$(CONFIG_MVEBU_ODMI)		+= irq-mvebu-odmi.o
>>   obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
>>   obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
>> +obj-$(CONFIG_SOC_NUC970)		+= irq-nuc900.o
>> diff --git a/drivers/irqchip/irq-nuc900.c b/drivers/irqchip/irq-nuc900.c
>> new file mode 100644
>> index 0000000..c4b2e39
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-nuc900.c
>> @@ -0,0 +1,150 @@
>> +/*
>> + * Copyright 2016 Wan Zongshun <mcuos.com@gmail.com>
>> + *
>> + * The code contained herein is licensed under the GNU General Public
>> + * License. You may obtain a copy of the GNU General Public License
>> + * Version 2 or later at the following locations:
>> + *
>> + * http://www.opensource.org/licenses/gpl-license.html
>> + * http://www.gnu.org/copyleft/gpl.html
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/irq.h>
>> +#include <linux/irqchip.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/io.h>
>> +#include <linux/ioport.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +
>> +#include <asm/exception.h>
>> +#include <asm/hardirq.h>
>> +
>> +#define	REG_AIC_SCR1	0x00
>> +#define	REG_AIC_SCR2	0x04
>> +#define	REG_AIC_SCR3	0x08
>> +#define	REG_AIC_SCR4	0x0C
>> +#define	REG_AIC_SCR5	0x10
>> +#define	REG_AIC_SCR6	0x14
>> +#define	REG_AIC_SCR7	0x18
>> +#define	REG_AIC_SCR8	0x1C
>> +#define	REG_AIC_SCR9	0x20
>> +#define	REG_AIC_SCR10	0x24
>> +#define	REG_AIC_SCR11	0x28
>> +#define	REG_AIC_SCR12	0x2C
>> +#define	REG_AIC_SCR13	0x30
>> +#define	REG_AIC_SCR14	0x34
>> +#define	REG_AIC_SCR15	0x38
>> +#define	REG_AIC_IRSR	0x100
>> +#define	REG_AIC_IRSRH	0x104
>> +#define	REG_AIC_IASR	0x108
>> +#define	REG_AIC_IASRH	0x10C
>> +#define	REG_AIC_ISR	0x110
>> +#define	REG_AIC_ISRH	0x114
>> +#define	REG_AIC_IPER	0x118
>> +#define	REG_AIC_ISNR	0x120
>> +#define	REG_AIC_OISR	0x124
>> +#define	REG_AIC_IMR	0x128
>> +#define	REG_AIC_IMRH	0x12C
>> +#define	REG_AIC_MECR	0x130
>> +#define	REG_AIC_MECRH	0x134
>> +#define	REG_AIC_MDCR	0x138
>> +#define	REG_AIC_MDCRH	0x13C
>> +#define	REG_AIC_SSCR	0x140
>> +#define	REG_AIC_SSCRH	0x144
>> +#define	REG_AIC_SCCR	0x148
>> +#define	REG_AIC_SCCRH	0x14C
>> +#define	REG_AIC_EOSCR	0x150
>
> Please remove unused defines.

Okay.

>
>> +
>> +static void __iomem *aic_base;
>> +static struct irq_domain *aic_domain;
>> +
>> +static void nuc900_irq_mask(struct irq_data *d)
>> +{
>> +	if (d->irq < 32)
>> +		writel(1 << (d->irq), aic_base + REG_AIC_MDCR);
>> +	else
>> +		writel(1 << (d->irq - 32), aic_base + REG_AIC_MDCRH);
>> +}
>> +
>> +static void nuc900_irq_ack(struct irq_data *d)
>> +{
>> +	writel(0x01, aic_base + REG_AIC_EOSCR);
>> +}
>> +
>> +static void nuc900_irq_unmask(struct irq_data *d)
>> +{
>> +	if (d->irq < 32)
>> +		writel(1 << (d->irq), aic_base + REG_AIC_MECR);
>> +	else
>> +		writel(1 << (d->irq - 32), aic_base + REG_AIC_MECRH);
>> +}
>> +
>> +static struct irq_chip nuc900_irq_chip = {
>> +	.irq_ack	= nuc900_irq_ack,
>> +	.irq_mask	= nuc900_irq_mask,
>> +	.irq_unmask	= nuc900_irq_unmask,
>> +};
>> +
>> +void __exception_irq_entry aic_handle_irq(struct pt_regs *regs)
>> +{
>> +	u32 hwirq;
>> +
>> +	hwirq = readl(aic_base + REG_AIC_IPER);
>> +	hwirq = readl(aic_base + REG_AIC_ISNR);
>> +	if (!hwirq)
>> +		writel(0x01, aic_base + REG_AIC_EOSCR);
>
> Could you explain what's going on here?

AIC_IPER, When the AIC generates the interrupt, VECTOR represents the 
interrupt channel number that is active, enabled, and has the highest 
priority.

The value of VECTOR is copied to the register AIC_ISNR thereafter by the 
AIC. This register was restored a value 0 after it was read by the
interrupt handler.

So I must read two registers for one irq number, after read IPER, the 
ISNR(bit0:5) will be updated. ISNR value has no need do offset, it is 
better than read IPER(bit2:7).

“writel(0x01, aic_base + REG_AIC_EOSCR);”　seems not necessary here, 
since irqchip has the ack interface. I will remove it.

This AIC_EOSCR register is used by the interrupt service routine to 
indicate that it is completely served. Thus, the
interrupt handler can write any value to this register to indicate the 
end of its interrupt service.

>
>> +
>> +	handle_IRQ((irq_find_mapping(aic_domain, hwirq)), regs);
>> +}
>> +
>> +static int aic_irq_domain_map(struct irq_domain *d, unsigned int virq,
>> +			      irq_hw_number_t hw)
>> +{
>> +	irq_set_chip_and_handler(virq, &nuc900_irq_chip, handle_level_irq);
>> +	irq_clear_status_flags(virq, IRQ_NOREQUEST);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct irq_domain_ops aic_irq_domain_ops = {
>> +	.map = aic_irq_domain_map,
>> +	.xlate = irq_domain_xlate_onecell,
>> +};
>> +
>> +static int __init aic_of_init(struct device_node *node,
>> +			      struct device_node *parent)
>> +{
>> +	int ret;
>> +
>> +	aic_base = of_iomap(node, 0);
>> +	if (!aic_base) {
>> +		ret = -ENOMEM;
>> +		pr_err("%s: unable to map registers\n", node->full_name);
>> +		goto err_iomap;
>> +	}
>> +
>> +	writel(0xFFFFFFFC, aic_base + REG_AIC_MDCR);
>> +	writel(0xFFFFFFFF, aic_base + REG_AIC_MDCRH);
>> +
>> +	aic_domain = irq_domain_add_linear(node, NR_IRQS,
>> +					   &aic_irq_domain_ops, NULL);
>> +
>> +	if (!aic_domain) {
>> +		ret = -ENOMEM;
>> +		pr_err("%s: unable to create IRQ domain\n", node->full_name);
>> +		goto err_aic_domain;
>> +	}
>> +
>> +	set_handle_irq(aic_handle_irq);
>> +	return 0;
>> +
>> +err_aic_domain:
>> +	iounmap(aic_base);
>> +err_iomap:
>> +	return ret;
>> +}
>> +
>> +IRQCHIP_DECLARE(nuc900, "nuvoton,nuc900-aic", aic_of_init);
>
> afaict, there is no 'nuc900', that's a family.  If so, this compatible
> needs to be 'nuvoton,nuc970-aic'.
>

IRQCHIP_DECLARE(nuvoton, "nuvoton,nuc900-aic", aic_of_init);, change 
nuc900 to nuvoton, ok?

> thx,
>
> Jason.
>
>

[toc] | [prev] | [next] | [standalone]

#1443471 — Re: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900

FromJason Cooper <jason@lakedaemon.net>
Date2016-07-14 16:00 +0200
SubjectRe: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900
Message-ID<rUPlf-1Xp-7@gated-at.bofh.it>
In reply to#1443055
Hi Wan Zongshun,

On Thu, Jul 14, 2016 at 11:36:53AM +0800, Wan Zongshun wrote:
> On 2016年07月14日 04:09, Jason Cooper wrote:
> >On Sun, Jul 10, 2016 at 03:27:22PM +0800, Wan Zongshun wrote:
> >>This patch is to add irqchip driver support for nuc900 plat,
> >>current this driver only supports nuc970 SoC.
> >>
> >>Signed-off-by: Wan Zongshun <mcuos.com@gmail.com>
> >>---
> >>  arch/arm/mach-w90x900/include/mach/irqs.h |   5 +
> >>  drivers/irqchip/Makefile                  |   1 +
> >>  drivers/irqchip/irq-nuc900.c              | 150 ++++++++++++++++++++++++++++++
> >>  3 files changed, 156 insertions(+)
> >>  create mode 100644 drivers/irqchip/irq-nuc900.c
> >>
> >>diff --git a/arch/arm/mach-w90x900/include/mach/irqs.h b/arch/arm/mach-w90x900/include/mach/irqs.h
> >>index 9d5cba3..3b035c6 100644
> >>--- a/arch/arm/mach-w90x900/include/mach/irqs.h
> >>+++ b/arch/arm/mach-w90x900/include/mach/irqs.h
> >>@@ -59,7 +59,12 @@
> >>  #define IRQ_KPI		W90X900_IRQ(29)
> >>  #define IRQ_P2SGROUP	W90X900_IRQ(30)
> >>  #define IRQ_ADC		W90X900_IRQ(31)
> >>+
> >>+#if !defined(CONFIG_SOC_NUC900)
> >>  #define NR_IRQS		(IRQ_ADC+1)
> >>+#else
> >>+#define NR_IRQS		62
> >>+#endif
> >
> >Arnd already covered this...
> >
> >>
> >>  /*for irq group*/
> >>
> >>diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> >>index 38853a1..9ccd5af8a 100644
> >>--- a/drivers/irqchip/Makefile
> >>+++ b/drivers/irqchip/Makefile
> >>@@ -69,3 +69,4 @@ obj-$(CONFIG_PIC32_EVIC)		+= irq-pic32-evic.o
> >>  obj-$(CONFIG_MVEBU_ODMI)		+= irq-mvebu-odmi.o
> >>  obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
> >>  obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
> >>+obj-$(CONFIG_SOC_NUC970)		+= irq-nuc900.o
> >>diff --git a/drivers/irqchip/irq-nuc900.c b/drivers/irqchip/irq-nuc900.c
> >>new file mode 100644
> >>index 0000000..c4b2e39
> >>--- /dev/null
> >>+++ b/drivers/irqchip/irq-nuc900.c
> >>@@ -0,0 +1,150 @@
> >>+/*
> >>+ * Copyright 2016 Wan Zongshun <mcuos.com@gmail.com>
> >>+ *
> >>+ * The code contained herein is licensed under the GNU General Public
> >>+ * License. You may obtain a copy of the GNU General Public License
> >>+ * Version 2 or later at the following locations:
> >>+ *
> >>+ * http://www.opensource.org/licenses/gpl-license.html
> >>+ * http://www.gnu.org/copyleft/gpl.html
> >>+ */
> >>+
> >>+#include <linux/module.h>
> >>+#include <linux/init.h>
> >>+#include <linux/irq.h>
> >>+#include <linux/irqchip.h>
> >>+#include <linux/irqdomain.h>
> >>+#include <linux/io.h>
> >>+#include <linux/ioport.h>
> >>+#include <linux/of_address.h>
> >>+#include <linux/of_irq.h>
> >>+
> >>+#include <asm/exception.h>
> >>+#include <asm/hardirq.h>
> >>+
> >>+#define	REG_AIC_SCR1	0x00
> >>+#define	REG_AIC_SCR2	0x04
> >>+#define	REG_AIC_SCR3	0x08
> >>+#define	REG_AIC_SCR4	0x0C
> >>+#define	REG_AIC_SCR5	0x10
> >>+#define	REG_AIC_SCR6	0x14
> >>+#define	REG_AIC_SCR7	0x18
> >>+#define	REG_AIC_SCR8	0x1C
> >>+#define	REG_AIC_SCR9	0x20
> >>+#define	REG_AIC_SCR10	0x24
> >>+#define	REG_AIC_SCR11	0x28
> >>+#define	REG_AIC_SCR12	0x2C
> >>+#define	REG_AIC_SCR13	0x30
> >>+#define	REG_AIC_SCR14	0x34
> >>+#define	REG_AIC_SCR15	0x38
> >>+#define	REG_AIC_IRSR	0x100
> >>+#define	REG_AIC_IRSRH	0x104
> >>+#define	REG_AIC_IASR	0x108
> >>+#define	REG_AIC_IASRH	0x10C
> >>+#define	REG_AIC_ISR	0x110
> >>+#define	REG_AIC_ISRH	0x114
> >>+#define	REG_AIC_IPER	0x118
> >>+#define	REG_AIC_ISNR	0x120
> >>+#define	REG_AIC_OISR	0x124
> >>+#define	REG_AIC_IMR	0x128
> >>+#define	REG_AIC_IMRH	0x12C
> >>+#define	REG_AIC_MECR	0x130
> >>+#define	REG_AIC_MECRH	0x134
> >>+#define	REG_AIC_MDCR	0x138
> >>+#define	REG_AIC_MDCRH	0x13C
> >>+#define	REG_AIC_SSCR	0x140
> >>+#define	REG_AIC_SSCRH	0x144
> >>+#define	REG_AIC_SCCR	0x148
> >>+#define	REG_AIC_SCCRH	0x14C
> >>+#define	REG_AIC_EOSCR	0x150
> >
> >Please remove unused defines.
> 
> Okay.
> 
> >
> >>+
> >>+static void __iomem *aic_base;
> >>+static struct irq_domain *aic_domain;
> >>+
> >>+static void nuc900_irq_mask(struct irq_data *d)
> >>+{
> >>+	if (d->irq < 32)
> >>+		writel(1 << (d->irq), aic_base + REG_AIC_MDCR);
> >>+	else
> >>+		writel(1 << (d->irq - 32), aic_base + REG_AIC_MDCRH);
> >>+}
> >>+
> >>+static void nuc900_irq_ack(struct irq_data *d)
> >>+{
> >>+	writel(0x01, aic_base + REG_AIC_EOSCR);
> >>+}
> >>+
> >>+static void nuc900_irq_unmask(struct irq_data *d)
> >>+{
> >>+	if (d->irq < 32)
> >>+		writel(1 << (d->irq), aic_base + REG_AIC_MECR);
> >>+	else
> >>+		writel(1 << (d->irq - 32), aic_base + REG_AIC_MECRH);
> >>+}
> >>+
> >>+static struct irq_chip nuc900_irq_chip = {
> >>+	.irq_ack	= nuc900_irq_ack,
> >>+	.irq_mask	= nuc900_irq_mask,
> >>+	.irq_unmask	= nuc900_irq_unmask,
> >>+};
> >>+
> >>+void __exception_irq_entry aic_handle_irq(struct pt_regs *regs)
> >>+{
> >>+	u32 hwirq;
> >>+
> >>+	hwirq = readl(aic_base + REG_AIC_IPER);
> >>+	hwirq = readl(aic_base + REG_AIC_ISNR);
> >>+	if (!hwirq)
> >>+		writel(0x01, aic_base + REG_AIC_EOSCR);
> >
> >Could you explain what's going on here?
> 
> AIC_IPER, When the AIC generates the interrupt, VECTOR represents
> the interrupt channel number that is active, enabled, and has the
> highest priority.
> 
> The value of VECTOR is copied to the register AIC_ISNR thereafter by
> the AIC. This register was restored a value 0 after it was read by
> the
> interrupt handler.

So, iiuc, the first readl() of AIC_IPER kicks the AIC to copy the value
of VECTOR into AIC_ISNR?  Then we can read it?

If so, please add a comment above the code explaining that.

> So I must read two registers for one irq number, after read IPER,
> the ISNR(bit0:5) will be updated. ISNR value has no need do offset,
> it is better than read IPER(bit2:7).

But you aren't using the value from the first readl(...AIC_IPER)?  You
overwrite hwirq with the value from the second readl(...AIC_ISNR)
...

The first part of your explaination makes sense, but this part isn't
clear to me.


> “writel(0x01, aic_base + REG_AIC_EOSCR);”　seems not necessary here,
> since irqchip has the ack interface. I will remove it.

Ok, thanks.

> This AIC_EOSCR register is used by the interrupt service routine to
> indicate that it is completely served. Thus, the
> interrupt handler can write any value to this register to indicate
> the end of its interrupt service.

A short comment in the ack routine would be helpful.


> >>+IRQCHIP_DECLARE(nuc900, "nuvoton,nuc900-aic", aic_of_init);
> >
> >afaict, there is no 'nuc900', that's a family.  If so, this compatible
> >needs to be 'nuvoton,nuc970-aic'.
> >
> 
> IRQCHIP_DECLARE(nuvoton, "nuvoton,nuc900-aic", aic_of_init);, change
> nuc900 to nuvoton, ok?

No, I was only talking about the second argument.  The compatible
string.  For devicetree, it needs to refer to a specific model number.
I suspect 'nuc900' is equivalent to saying 'nuc9xx'.  Meaning it refers
to a family of devices.  The compatible string needs to refer
specifically to the first model that the binding is compatible with.  In
this case, I presume that would be 'nuvoton,nuc970-aic'.

thx,

Jason.

[toc] | [prev] | [next] | [standalone]

#1443947 — Re: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900

FromWan Zongshun <vw@iommu.org>
Date2016-07-15 07:20 +0200
SubjectRe: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900
Message-ID<rV3Hz-2Ms-3@gated-at.bofh.it>
In reply to#1443471

On 2016年07月14日 21:54, Jason Cooper wrote:
> Hi Wan Zongshun,
>
> On Thu, Jul 14, 2016 at 11:36:53AM +0800, Wan Zongshun wrote:
>> On 2016年07月14日 04:09, Jason Cooper wrote:
>>> On Sun, Jul 10, 2016 at 03:27:22PM +0800, Wan Zongshun wrote:
>>>> This patch is to add irqchip driver support for nuc900 plat,
>>>> current this driver only supports nuc970 SoC.
>>>>
>>>> Signed-off-by: Wan Zongshun <mcuos.com@gmail.com>
>>>> ---
>>>>   arch/arm/mach-w90x900/include/mach/irqs.h |   5 +
>>>>   drivers/irqchip/Makefile                  |   1 +
>>>>   drivers/irqchip/irq-nuc900.c              | 150 ++++++++++++++++++++++++++++++
>>>>   3 files changed, 156 insertions(+)
>>>>   create mode 100644 drivers/irqchip/irq-nuc900.c
>>>>
>>>> diff --git a/arch/arm/mach-w90x900/include/mach/irqs.h b/arch/arm/mach-w90x900/include/mach/irqs.h
>>>> index 9d5cba3..3b035c6 100644
>>>> --- a/arch/arm/mach-w90x900/include/mach/irqs.h
>>>> +++ b/arch/arm/mach-w90x900/include/mach/irqs.h
>>>> @@ -59,7 +59,12 @@
>>>>   #define IRQ_KPI		W90X900_IRQ(29)
>>>>   #define IRQ_P2SGROUP	W90X900_IRQ(30)
>>>>   #define IRQ_ADC		W90X900_IRQ(31)
>>>> +
>>>> +#if !defined(CONFIG_SOC_NUC900)
>>>>   #define NR_IRQS		(IRQ_ADC+1)
>>>> +#else
>>>> +#define NR_IRQS		62
>>>> +#endif
>>>
>>> Arnd already covered this...
>>>
>>>>
>>>>   /*for irq group*/
>>>>
>>>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>>>> index 38853a1..9ccd5af8a 100644
>>>> --- a/drivers/irqchip/Makefile
>>>> +++ b/drivers/irqchip/Makefile
>>>> @@ -69,3 +69,4 @@ obj-$(CONFIG_PIC32_EVIC)		+= irq-pic32-evic.o
>>>>   obj-$(CONFIG_MVEBU_ODMI)		+= irq-mvebu-odmi.o
>>>>   obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
>>>>   obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
>>>> +obj-$(CONFIG_SOC_NUC970)		+= irq-nuc900.o
>>>> diff --git a/drivers/irqchip/irq-nuc900.c b/drivers/irqchip/irq-nuc900.c
>>>> new file mode 100644
>>>> index 0000000..c4b2e39
>>>> --- /dev/null
>>>> +++ b/drivers/irqchip/irq-nuc900.c
>>>> @@ -0,0 +1,150 @@
>>>> +/*
>>>> + * Copyright 2016 Wan Zongshun <mcuos.com@gmail.com>
>>>> + *
>>>> + * The code contained herein is licensed under the GNU General Public
>>>> + * License. You may obtain a copy of the GNU General Public License
>>>> + * Version 2 or later at the following locations:
>>>> + *
>>>> + * http://www.opensource.org/licenses/gpl-license.html
>>>> + * http://www.gnu.org/copyleft/gpl.html
>>>> + */
>>>> +
>>>> +#include <linux/module.h>
>>>> +#include <linux/init.h>
>>>> +#include <linux/irq.h>
>>>> +#include <linux/irqchip.h>
>>>> +#include <linux/irqdomain.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/ioport.h>
>>>> +#include <linux/of_address.h>
>>>> +#include <linux/of_irq.h>
>>>> +
>>>> +#include <asm/exception.h>
>>>> +#include <asm/hardirq.h>
>>>> +
>>>> +#define	REG_AIC_SCR1	0x00
>>>> +#define	REG_AIC_SCR2	0x04
>>>> +#define	REG_AIC_SCR3	0x08
>>>> +#define	REG_AIC_SCR4	0x0C
>>>> +#define	REG_AIC_SCR5	0x10
>>>> +#define	REG_AIC_SCR6	0x14
>>>> +#define	REG_AIC_SCR7	0x18
>>>> +#define	REG_AIC_SCR8	0x1C
>>>> +#define	REG_AIC_SCR9	0x20
>>>> +#define	REG_AIC_SCR10	0x24
>>>> +#define	REG_AIC_SCR11	0x28
>>>> +#define	REG_AIC_SCR12	0x2C
>>>> +#define	REG_AIC_SCR13	0x30
>>>> +#define	REG_AIC_SCR14	0x34
>>>> +#define	REG_AIC_SCR15	0x38
>>>> +#define	REG_AIC_IRSR	0x100
>>>> +#define	REG_AIC_IRSRH	0x104
>>>> +#define	REG_AIC_IASR	0x108
>>>> +#define	REG_AIC_IASRH	0x10C
>>>> +#define	REG_AIC_ISR	0x110
>>>> +#define	REG_AIC_ISRH	0x114
>>>> +#define	REG_AIC_IPER	0x118
>>>> +#define	REG_AIC_ISNR	0x120
>>>> +#define	REG_AIC_OISR	0x124
>>>> +#define	REG_AIC_IMR	0x128
>>>> +#define	REG_AIC_IMRH	0x12C
>>>> +#define	REG_AIC_MECR	0x130
>>>> +#define	REG_AIC_MECRH	0x134
>>>> +#define	REG_AIC_MDCR	0x138
>>>> +#define	REG_AIC_MDCRH	0x13C
>>>> +#define	REG_AIC_SSCR	0x140
>>>> +#define	REG_AIC_SSCRH	0x144
>>>> +#define	REG_AIC_SCCR	0x148
>>>> +#define	REG_AIC_SCCRH	0x14C
>>>> +#define	REG_AIC_EOSCR	0x150
>>>
>>> Please remove unused defines.
>>
>> Okay.
>>
>>>
>>>> +
>>>> +static void __iomem *aic_base;
>>>> +static struct irq_domain *aic_domain;
>>>> +
>>>> +static void nuc900_irq_mask(struct irq_data *d)
>>>> +{
>>>> +	if (d->irq < 32)
>>>> +		writel(1 << (d->irq), aic_base + REG_AIC_MDCR);
>>>> +	else
>>>> +		writel(1 << (d->irq - 32), aic_base + REG_AIC_MDCRH);
>>>> +}
>>>> +
>>>> +static void nuc900_irq_ack(struct irq_data *d)
>>>> +{
>>>> +	writel(0x01, aic_base + REG_AIC_EOSCR);
>>>> +}
>>>> +
>>>> +static void nuc900_irq_unmask(struct irq_data *d)
>>>> +{
>>>> +	if (d->irq < 32)
>>>> +		writel(1 << (d->irq), aic_base + REG_AIC_MECR);
>>>> +	else
>>>> +		writel(1 << (d->irq - 32), aic_base + REG_AIC_MECRH);
>>>> +}
>>>> +
>>>> +static struct irq_chip nuc900_irq_chip = {
>>>> +	.irq_ack	= nuc900_irq_ack,
>>>> +	.irq_mask	= nuc900_irq_mask,
>>>> +	.irq_unmask	= nuc900_irq_unmask,
>>>> +};
>>>> +
>>>> +void __exception_irq_entry aic_handle_irq(struct pt_regs *regs)
>>>> +{
>>>> +	u32 hwirq;
>>>> +
>>>> +	hwirq = readl(aic_base + REG_AIC_IPER);
>>>> +	hwirq = readl(aic_base + REG_AIC_ISNR);
>>>> +	if (!hwirq)
>>>> +		writel(0x01, aic_base + REG_AIC_EOSCR);
>>>
>>> Could you explain what's going on here?
>>
>> AIC_IPER, When the AIC generates the interrupt, VECTOR represents
>> the interrupt channel number that is active, enabled, and has the
>> highest priority.
>>
>> The value of VECTOR is copied to the register AIC_ISNR thereafter by
>> the AIC. This register was restored a value 0 after it was read by
>> the
>> interrupt handler.
>
> So, iiuc, the first readl() of AIC_IPER kicks the AIC to copy the value
> of VECTOR into AIC_ISNR?  Then we can read it?
>
> If so, please add a comment above the code explaining that.
>
>> So I must read two registers for one irq number, after read IPER,
>> the ISNR(bit0:5) will be updated. ISNR value has no need do offset,
>> it is better than read IPER(bit2:7).
>
> But you aren't using the value from the first readl(...AIC_IPER)?  You
> overwrite hwirq with the value from the second readl(...AIC_ISNR)
> ...
>
> The first part of your explaination makes sense, but this part isn't
> clear to me.

Actually, I have two choice to implement this function:

option1:

void __exception_irq_entry aic_handle_irq(struct pt_regs *regs)
{
	u32 hwirq;

	(void)readl(aic_base + REG_AIC_IPER);
	hwirq = readl(aic_base + REG_AIC_ISNR);

	handle_IRQ((irq_find_mapping(aic_domain, hwirq)), regs);
}

option2:

void __exception_irq_entry aic_handle_irq(struct pt_regs *regs)
{
	u32 hwirq;

	hwirq = readl(aic_base + REG_AIC_IPER);
	hwirq <<= 2;

	handle_IRQ((irq_find_mapping(aic_domain, hwirq)), regs);
}

Though the option2 do shift for hwirq, but it seems better than do io 
operation by readl,so I prefer to option2, agree?

>
>
>> “writel(0x01, aic_base + REG_AIC_EOSCR);”　seems not necessary here,
>> since irqchip has the ack interface. I will remove it.
>
> Ok, thanks.
>
>> This AIC_EOSCR register is used by the interrupt service routine to
>> indicate that it is completely served. Thus, the
>> interrupt handler can write any value to this register to indicate
>> the end of its interrupt service.
>
> A short comment in the ack routine would be helpful.
>
>
>>>> +IRQCHIP_DECLARE(nuc900, "nuvoton,nuc900-aic", aic_of_init);
>>>
>>> afaict, there is no 'nuc900', that's a family.  If so, this compatible
>>> needs to be 'nuvoton,nuc970-aic'.
>>>
>>
>> IRQCHIP_DECLARE(nuvoton, "nuvoton,nuc900-aic", aic_of_init);, change
>> nuc900 to nuvoton, ok?
>
> No, I was only talking about the second argument.  The compatible
> string.  For devicetree, it needs to refer to a specific model number.
> I suspect 'nuc900' is equivalent to saying 'nuc9xx'.  Meaning it refers
> to a family of devices.  The compatible string needs to refer
> specifically to the first model that the binding is compatible with.  In
> this case, I presume that would be 'nuvoton,nuc970-aic'.
>
> thx,
>
> Jason.
>
>

[toc] | [prev] | [next] | [standalone]

#1443977 — Re: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900

FromArnd Bergmann <arnd@arndb.de>
Date2016-07-15 09:10 +0200
SubjectRe: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900
Message-ID<rV5q1-3U6-1@gated-at.bofh.it>
In reply to#1443947
On Friday, July 15, 2016 1:15:58 PM CEST Wan Zongshun wrote:
> 
> Actually, I have two choice to implement this function:
> 
> option1:
> 
> void __exception_irq_entry aic_handle_irq(struct pt_regs *regs)
> {
>         u32 hwirq;
> 
>         (void)readl(aic_base + REG_AIC_IPER);
>         hwirq = readl(aic_base + REG_AIC_ISNR);
> 
>         handle_IRQ((irq_find_mapping(aic_domain, hwirq)), regs);
> }

(side note: I think you want handle_domain_irq())

> option2:
> 
> void __exception_irq_entry aic_handle_irq(struct pt_regs *regs)
> {
>         u32 hwirq;
> 
>         hwirq = readl(aic_base + REG_AIC_IPER);
>         hwirq <<= 2;
> 
>         handle_IRQ((irq_find_mapping(aic_domain, hwirq)), regs);
> }
> 
> Though the option2 do shift for hwirq, but it seems better than do io 
> operation by readl,so I prefer to option2, agree?

That will only return an irq number that is a multiple of four, which
seems wrong since the numbers are not that. Did you mean to write

	hwirq = ilog2(hwirq);   ?

That assumes that REG_AIC_IPER contains a 32-bit value with one single
bit set to indicate which IRQ was triggered.

If the difference is only in performance, you could try measuring which
of the two ends up being faster.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1444124 — Re: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900

FromWan ZongShun <mcuos.com@gmail.com>
Date2016-07-15 11:50 +0200
SubjectRe: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900
Message-ID<rV7UR-5ff-5@gated-at.bofh.it>
In reply to#1443977
2016-07-15 15:00 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
> On Friday, July 15, 2016 1:15:58 PM CEST Wan Zongshun wrote:
>>
>> Actually, I have two choice to implement this function:
>>
>> option1:
>>
>> void __exception_irq_entry aic_handle_irq(struct pt_regs *regs)
>> {
>>         u32 hwirq;
>>
>>         (void)readl(aic_base + REG_AIC_IPER);
>>         hwirq = readl(aic_base + REG_AIC_ISNR);
>>
>>         handle_IRQ((irq_find_mapping(aic_domain, hwirq)), regs);
>> }
>
> (side note: I think you want handle_domain_irq())
>
>> option2:
>>
>> void __exception_irq_entry aic_handle_irq(struct pt_regs *regs)
>> {
>>         u32 hwirq;
>>
>>         hwirq = readl(aic_base + REG_AIC_IPER);
>>         hwirq <<= 2;
>>
>>         handle_IRQ((irq_find_mapping(aic_domain, hwirq)), regs);
>> }
>>
>> Though the option2 do shift for hwirq, but it seems better than do io
>> operation by readl,so I prefer to option2, agree?
>
> That will only return an irq number that is a multiple of four, which
> seems wrong since the numbers are not that. Did you mean to write
>
>         hwirq = ilog2(hwirq);   ?

Sorry, my fault, I mean hwirq >>= 2, bit[7:2] indicates which irq is triggering.
so I have to do right shift 2 for IPER value.

>
> That assumes that REG_AIC_IPER contains a 32-bit value with one single
> bit set to indicate which IRQ was triggered.
>
> If the difference is only in performance, you could try measuring which
> of the two ends up being faster.

It seems hard to measure. I think Do IO operation should be slower
than shift 2. :)

>
>         Arnd



-- 
---
Vincent Wan(Zongshun)
www.mcuos.com

[toc] | [prev] | [next] | [standalone]

#1444393 — Re: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900

FromJason Cooper <jason@lakedaemon.net>
Date2016-07-15 17:50 +0200
SubjectRe: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900
Message-ID<rVdxf-dW-7@gated-at.bofh.it>
In reply to#1444124
On Fri, Jul 15, 2016 at 05:44:50PM +0800, Wan ZongShun wrote:
> 2016-07-15 15:00 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
> > On Friday, July 15, 2016 1:15:58 PM CEST Wan Zongshun wrote:
> >>
> >> Actually, I have two choice to implement this function:
> >>
> >> option1:
> >>
> >> void __exception_irq_entry aic_handle_irq(struct pt_regs *regs)
> >> {
> >>         u32 hwirq;
> >>
> >>         (void)readl(aic_base + REG_AIC_IPER);
> >>         hwirq = readl(aic_base + REG_AIC_ISNR);
> >>
> >>         handle_IRQ((irq_find_mapping(aic_domain, hwirq)), regs);
> >> }
> >
> > (side note: I think you want handle_domain_irq())
> >
> >> option2:
> >>
> >> void __exception_irq_entry aic_handle_irq(struct pt_regs *regs)
> >> {
> >>         u32 hwirq;
> >>
> >>         hwirq = readl(aic_base + REG_AIC_IPER);
> >>         hwirq <<= 2;
> >>
> >>         handle_IRQ((irq_find_mapping(aic_domain, hwirq)), regs);
> >> }
> >>
> >> Though the option2 do shift for hwirq, but it seems better than do io
> >> operation by readl,so I prefer to option2, agree?
> >
> > That will only return an irq number that is a multiple of four, which
> > seems wrong since the numbers are not that. Did you mean to write
> >
> >         hwirq = ilog2(hwirq);   ?
> 
> Sorry, my fault, I mean hwirq >>= 2, bit[7:2] indicates which irq is triggering.
> so I have to do right shift 2 for IPER value.

Ok, this makes a lot more sense now. :)

> > That assumes that REG_AIC_IPER contains a 32-bit value with one single
> > bit set to indicate which IRQ was triggered.
> >
> > If the difference is only in performance, you could try measuring which
> > of the two ends up being faster.
> 
> It seems hard to measure. I think Do IO operation should be slower
> than shift 2. :)

Agreed.

thx,

Jason.

[toc] | [prev] | [next] | [standalone]

#1447794 — Re: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900

FromArnd Bergmann <arnd@arndb.de>
Date2016-07-21 13:00 +0200
SubjectRe: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900
Message-ID<rXjRU-6ms-19@gated-at.bofh.it>
In reply to#1444124
On Friday, July 15, 2016 5:44:50 PM CEST Wan ZongShun wrote:
> 2016-07-15 15:00 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
> > On Friday, July 15, 2016 1:15:58 PM CEST Wan Zongshun wrote:
> >>
> >> Actually, I have two choice to implement this function:
> >>
> >> option1:
> >>
> >> void __exception_irq_entry aic_handle_irq(struct pt_regs *regs)
> >> {
> >>         u32 hwirq;
> >>
> >>         (void)readl(aic_base + REG_AIC_IPER);
> >>         hwirq = readl(aic_base + REG_AIC_ISNR);
> >>
> >>         handle_IRQ((irq_find_mapping(aic_domain, hwirq)), regs);
> >> }
> >
> > (side note: I think you want handle_domain_irq())
> >
> >> option2:
> >>
> >> void __exception_irq_entry aic_handle_irq(struct pt_regs *regs)
> >> {
> >>         u32 hwirq;
> >>
> >>         hwirq = readl(aic_base + REG_AIC_IPER);
> >>         hwirq <<= 2;
> >>
> >>         handle_IRQ((irq_find_mapping(aic_domain, hwirq)), regs);
> >> }
> >>
> >> Though the option2 do shift for hwirq, but it seems better than do io
> >> operation by readl,so I prefer to option2, agree?
> >
> > That will only return an irq number that is a multiple of four, which
> > seems wrong since the numbers are not that. Did you mean to write
> >
> >         hwirq = ilog2(hwirq);   ?
> 
> Sorry, my fault, I mean hwirq >>= 2, bit[7:2] indicates which irq is triggering.
> so I have to do right shift 2 for IPER value.

Ok, got it.

> > That assumes that REG_AIC_IPER contains a 32-bit value with one single
> > bit set to indicate which IRQ was triggered.
> >
> > If the difference is only in performance, you could try measuring which
> > of the two ends up being faster.
> 
> It seems hard to measure. I think Do IO operation should be slower
> than shift 2. 

It depends on how fast that particular I/O path is. A lot of readl()
operations are awfully slow, but the hardware design for the interrupt
controller may in fact have optimized this to be reasonably fast.

Another option would be to avoid the shift and just use the raw value
of the REG_AIC_IPER register as the hwirq, with a custom map()
callback that turns shifts the number read from the DT two bits
so it matches the register value.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1448064 — Re: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900

FromJason Cooper <jason@lakedaemon.net>
Date2016-07-21 20:50 +0200
SubjectRe: [PATCH v2 02/10] irqchip: add irqchip driver for nuc900
Message-ID<rXrcK-2T6-31@gated-at.bofh.it>
In reply to#1447794
Wan ZongShun,

On Fri, Jul 15, 2016 at 12:02:55PM +0200, Arnd Bergmann wrote:
> On Friday, July 15, 2016 5:44:50 PM CEST Wan ZongShun wrote:
> > 2016-07-15 15:00 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:
> > > On Friday, July 15, 2016 1:15:58 PM CEST Wan Zongshun wrote:
...
> > > That assumes that REG_AIC_IPER contains a 32-bit value with one single
> > > bit set to indicate which IRQ was triggered.
> > >
> > > If the difference is only in performance, you could try measuring which
> > > of the two ends up being faster.
> > 
> > It seems hard to measure. I think Do IO operation should be slower
> > than shift 2. 
> 
> It depends on how fast that particular I/O path is. A lot of readl()
> operations are awfully slow, but the hardware design for the interrupt
> controller may in fact have optimized this to be reasonably fast.
> 
> Another option would be to avoid the shift and just use the raw value
> of the REG_AIC_IPER register as the hwirq, with a custom map()
> callback that turns shifts the number read from the DT two bits
> so it matches the register value.

Good idea.  Are the two lsb bits constant or do they need to be masked?

thx,

Jason.

[toc] | [prev] | [next] | [standalone]

#1440941 — Re: [PATCH v2 04/10] clk: add Clock driver for nuc970

FromMichael Turquette <mturquette@baylibre.com>
Date2016-07-12 00:20 +0200
SubjectRe: [PATCH v2 04/10] clk: add Clock driver for nuc970
Message-ID<rTRIt-4Dg-5@gated-at.bofh.it>
In reply to#1440043
Quoting Wan Zongshun (2016-07-10 00:27:24)
> diff --git a/drivers/clk/nuc900/clk-apll.c b/drivers/clk/nuc900/clk-apll.c
> new file mode 100644
> index 0000000..a05aec7
> --- /dev/null
> +++ b/drivers/clk/nuc900/clk-apll.c
> @@ -0,0 +1,168 @@
> +/*
> + * Copyright 2016 Wan Zongshun <mcuos.com@gmail.com>
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <linux/kernel.h>
> +#include <linux/err.h>
> +
> +#include "clk-ccf.h"

Maybe call it clk-nuc.h?

> +static struct clk_ops clk_apll_ops = {
> +       .recalc_rate = clk_apll_recalc_rate,
> +       .enable = clk_apll_enable,
> +       .disable = clk_apll_disable,

Can you provide a .is_enabled?

> +static void __init nuc970_clocks_init(struct device_node *np)
> +{
> +       int i, ret;
> +
> +       clkctrl = of_iomap(np, 0);
> +       if (!clkctrl)
> +               pr_err("%s: unable to map registers\n", np->full_name);
> +
> +
> +       /* source */
> +       clk[XIN]        = nuc970_clk_fixed("xin", 12000000);
> +       clk[XIN32K]     = nuc970_clk_fixed("xin32k", 32768);
> +       clk[APLL]       = nuc970_clk_apll("apll", "xin", REG_CLK_APLLCON);
> +       clk[UPLL]       = nuc970_clk_upll("upll", "xin", REG_CLK_UPLLCON);
> +       clk[XIN128_DIV] = nuc970_clk_fixed_factor("xin128_div", "xin", 1, 128);
> +       clk[SYS_MUX]    = nuc970_clk_mux("sys_mux", REG_CLK_DIV0, 3, 2,
> +                                        sys_sel_clks,
> +                                        ARRAY_SIZE(sys_sel_clks));

Instead of executing all of these registration functions, how about
initializing your clock data statically, and then simply calling
clk_hw_register?

For an example see the recently merged drivers/clk/meson/gxbb.c

> +       for (i = 0; i < ARRAY_SIZE(clk); i++)
> +               if (IS_ERR(clk[i]))
> +                       pr_err("nuc970 clk %d: register failed with %ld\n",
> +                               i, PTR_ERR(clk[i]));

Better to fail quickly, bail out and unwind your clk registration
instead of trying to register everything and then walk the list looking
for failures.

> +
> +       clk_data.clks = clk;
> +       clk_data.clk_num = ARRAY_SIZE(clk);
> +
> +       ret = of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
> +       if (ret)
> +               pr_err("Failed to register OF clock provider\n");
> +
> +       /* Register clock device */
> +       clk_register_clkdev(clk[TIMER0_GATE], "timer0", NULL);
> +       clk_register_clkdev(clk[TIMER1_GATE], "timer1", NULL);

Again, look at how the gxbb.c driver does this. Why do you need to call
clk_register_clkdev? You're using of_clk_add_provider above, so that
should be enough to perform lookups.

> +       /* enable some important clocks */
> +       clk_prepare_enable(clk_get(NULL, "cpu"));
> +       clk_prepare_enable(clk_get(NULL, "hclk"));
> +       clk_prepare_enable(clk_get(NULL, "sram"));
> +       clk_prepare_enable(clk_get(NULL, "dram"));
> +       clk_prepare_enable(clk_get(NULL, "ddr_hclk"));

You can use the CLK_IS_CRITICAL flag for these clocks if you want. It
would be better to have drivers that claim them and enable them of
course.

> +}
> +
> +CLK_OF_DECLARE(nuc970_clk, "nuvoton,nuc970-clk", nuc970_clocks_init);

Why do you need to use CLK_OF_DECLARE? Please convert this to a
platform_driver and load it at module_init.

Regards,
Mike

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web