On 21/11/2018 19:30, Mesih Kilinc wrote:
> The suniv (new F-series) chip has a timer with less functionality than
> the A10 timer, e.g. it has only 3 channels.
>
> Add a new compatible for it. As we didn't use the extra channels on A10
> either now, the code needn't to be changed.
>
> The suniv chip is based on ARM926EJ-S CPU, thus it has no architecture timer.
>
> Register sun4i_timer as sched_clock on it.
>
> Signed-off-by: Mesih Kilinc <[email protected]>

Acked-by: Daniel Lezcano <[email protected]>

> ---
> drivers/clocksource/sun4i_timer.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clocksource/sun4i_timer.c b/drivers/clocksource/sun4i_timer.c
> index 6e0180a..65f38f6 100644
> --- a/drivers/clocksource/sun4i_timer.c
> +++ b/drivers/clocksource/sun4i_timer.c
> @@ -186,7 +186,8 @@ static int __init sun4i_timer_init(struct device_node *node)
> */
> if (of_machine_is_compatible("allwinner,sun4i-a10") ||
> of_machine_is_compatible("allwinner,sun5i-a13") ||
> - of_machine_is_compatible("allwinner,sun5i-a10s"))
> + of_machine_is_compatible("allwinner,sun5i-a10s") ||
> + of_machine_is_compatible("allwinner,suniv-f1c100s"))
> sched_clock_register(sun4i_timer_sched_read, 32,
> timer_of_rate(&to));
>
> @@ -218,3 +219,5 @@ static int __init sun4i_timer_init(struct device_node *node)
> }
> TIMER_OF_DECLARE(sun4i, "allwinner,sun4i-a10-timer",
> sun4i_timer_init);
> +TIMER_OF_DECLARE(suniv, "allwinner,suniv-f1c100s-timer",
> + sun4i_timer_init);
>

--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

2018-11-23 23:46:35

by Mesih Kilinc

[permalink] [raw]

Subject: Re: [RFC PATCH v3 05/17] irqchip/sun4i: add support for suniv interrupt controller

On 18/11/22 09:35, Maxime Ripard wrote:
> Hi Mesih,
>
Hi!
> On Wed, Nov 21, 2018 at 09:30:38PM +0300, Mesih Kilinc wrote:
> > The new F-series SoCs (suniv) from Allwinner use an stripped version of
> > the interrupt controller in A10/A13
> >
> > Add support for it in irq-sun4i driver.
> >
> > Signed-off-by: Mesih Kilinc <[email protected]>
> > ---
> > drivers/irqchip/irq-sun4i.c | 104 +++++++++++++++++++++++++++++++-------------
> > 1 file changed, 74 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-sun4i.c b/drivers/irqchip/irq-sun4i.c
> > index e3e5b91..7ca4a4d 100644
> > --- a/drivers/irqchip/irq-sun4i.c
> > +++ b/drivers/irqchip/irq-sun4i.c
> > @@ -28,11 +28,21 @@
> > #define SUN4I_IRQ_NMI_CTRL_REG 0x0c
> > #define SUN4I_IRQ_PENDING_REG(x) (0x10 + 0x4 * x)
> > #define SUN4I_IRQ_FIQ_PENDING_REG(x) (0x20 + 0x4 * x)
> > -#define SUN4I_IRQ_ENABLE_REG(x) (0x40 + 0x4 * x)
> > -#define SUN4I_IRQ_MASK_REG(x) (0x50 + 0x4 * x)
> > +#define SUN4I_IRQ_ENABLE_REG(x) (irq_ic_data->enable_req_offset + 0x4 * x)
> > +#define SUN4I_IRQ_MASK_REG(x) (irq_ic_data->mask_req_offset + 0x4 * x)
>
> You shouldn't have all the values you use passed as argument, so
> irq_ic_data should be one of them here.
>
Could you elaborate it a little bit?
> > +#define SUN4I_IRQ_ENABLE_REG_OFFSET 0x40
> > +#define SUN4I_IRQ_MASK_REG_OFFSET 0x50
> > +#define SUNIV_IRQ_ENABLE_REG_OFFSET 0x20
> > +#define SUNIV_IRQ_MASK_REG_OFFSET 0x30
> > +
> > +struct sunxi_irq_chip_data{
> ^ a space here
>
> > + void __iomem *irq_base;
> > + struct irq_domain *irq_domain;
> > + u32 enable_req_offset;
> > + u32 mask_req_offset;
>
> s/req/reg/ ?
>
Oops sorry

...
> > +
> > +static int __init suniv_ic_of_init(struct device_node *node,
> > + struct device_node *parent)
> > +{
> > + irq_ic_data = kzalloc(sizeof(struct sunxi_irq_chip_data), GFP_KERNEL);
> > + if (!irq_ic_data) {
> > + pr_err("kzalloc failed!\n");
> > + return -ENOMEM;
> > + }
> > +
> > + irq_ic_data->enable_req_offset = SUNIV_IRQ_ENABLE_REG_OFFSET;
> > + irq_ic_data->mask_req_offset = SUNIV_IRQ_MASK_REG_OFFSET;
> > +
> > + return sun4i_of_init(node, parent);
> > +}
> > +
> > +IRQCHIP_DECLARE(allwinner_sunvi_ic, "allwinner,suniv-f1c100s-ic", suniv_ic_of_init);
>
> You can even split that addition to a new patch as well.

OK. I will do 3 patches. First one will add a struct that holds only
base and domain. Second one will add register offsets to that struct.
Third one will add f1c100s support. Is that ok?

Mesih

2018-11-24 00:06:21

by Mesih Kilinc

[permalink] [raw]

Subject: Re: [RFC PATCH v3 01/17] ARM: add CONFIG_ARCH_SUNXI_V7 for differentiate ARMv5/v7 Allwinner SoCs

On 18/11/22 09:26, Maxime Ripard wrote:
> On Wed, Nov 21, 2018 at 09:30:34PM +0300, Mesih Kilinc wrote:
> > Allwinner also has some ARMv5 SoCs.
> >
> > In order to add support for them, add a CONFIG_ARCH_SUNXI_V7 bool config
> > which is selected when a ARMv7 soc is selected, and make CONFIG_ARCH_SUNXI
> > a common option which is selected by both V7 and V5 sunxi option.
> >
> > Signed-off-by: Mesih Kilinc <[email protected]>
> > ---
> > arch/arm/mach-sunxi/Kconfig | 23 ++++++++++++++++-------
> > 1 file changed, 16 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> > index d9c8ecf..7e5f173 100644
> > --- a/arch/arm/mach-sunxi/Kconfig
> > +++ b/arch/arm/mach-sunxi/Kconfig
> > @@ -6,31 +6,38 @@ menuconfig ARCH_SUNXI
> > select GENERIC_IRQ_CHIP
> > select GPIOLIB
> > select PINCTRL
> > - select PM_OPP
> > select SUN4I_TIMER
> > select RESET_CONTROLLER
> > + help
> > + Support for Allwinner ARM-based family of processors
> >
> > if ARCH_SUNXI
> >
> > +if ARCH_MULTI_V7
> > +
> > +config ARCH_SUNXI_V7
> > + bool
> > + select PM_OPP
> > +
>
> Actually, thinking more about this, I don't see why the PM_OPP config
> option should be made specific to the ARMv7 sunxi SoCs, you will have
> things cpufreq enabled at some point as well on the armv5 support.
>
OK. I was basing this code to the first patch series.
So there will be no need for ARCH_SUNXI_VX configs. Should I drop them
and differantiate IC's by only ARCH_MULTI_VX?

> > config MACH_SUN4I
> > bool "Allwinner A10 (sun4i) SoCs support"
> > - default ARCH_SUNXI
> > + select ARCH_SUNXI_V7
>
> The option is still changed from being on by default to being off by
> default. This breaks all the defconfig, and we really shouldn't change
> that.

OK. I will fix it.

Mesih Veysi Kılınç

2018-11-24 00:52:47

by Maxime Ripard

[permalink] [raw]

Subject: Re: [RFC PATCH v3 01/17] ARM: add CONFIG_ARCH_SUNXI_V7 for differentiate ARMv5/v7 Allwinner SoCs

On Thu, Nov 22, 2018 at 06:07:51PM +0300, Mesih Kilinc wrote:
> On 18/11/22 09:26, Maxime Ripard wrote:
> > On Wed, Nov 21, 2018 at 09:30:34PM +0300, Mesih Kilinc wrote:
> > > Allwinner also has some ARMv5 SoCs.
> > >
> > > In order to add support for them, add a CONFIG_ARCH_SUNXI_V7 bool config
> > > which is selected when a ARMv7 soc is selected, and make CONFIG_ARCH_SUNXI
> > > a common option which is selected by both V7 and V5 sunxi option.
> > >
> > > Signed-off-by: Mesih Kilinc <[email protected]>
> > > ---
> > > arch/arm/mach-sunxi/Kconfig | 23 ++++++++++++++++-------
> > > 1 file changed, 16 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> > > index d9c8ecf..7e5f173 100644
> > > --- a/arch/arm/mach-sunxi/Kconfig
> > > +++ b/arch/arm/mach-sunxi/Kconfig
> > > @@ -6,31 +6,38 @@ menuconfig ARCH_SUNXI
> > > select GENERIC_IRQ_CHIP
> > > select GPIOLIB
> > > select PINCTRL
> > > - select PM_OPP
> > > select SUN4I_TIMER
> > > select RESET_CONTROLLER
> > > + help
> > > + Support for Allwinner ARM-based family of processors
> > >
> > > if ARCH_SUNXI
> > >
> > > +if ARCH_MULTI_V7
> > > +
> > > +config ARCH_SUNXI_V7
> > > + bool
> > > + select PM_OPP
> > > +
> >
> > Actually, thinking more about this, I don't see why the PM_OPP config
> > option should be made specific to the ARMv7 sunxi SoCs, you will have
> > things cpufreq enabled at some point as well on the armv5 support.
> >
> OK. I was basing this code to the first patch series.
> So there will be no need for ARCH_SUNXI_VX configs. Should I drop them
> and differantiate IC's by only ARCH_MULTI_VX?

Yeah, that's probably enough for now, and we can always merge some
patch along those lines if we ever need it later.

Thanks!
Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Attachments:

(No filename) (1.90 kB)
signature.asc (235.00 B)
Download all attachments

2018-11-24 03:12:07

by Maxime Ripard

[permalink] [raw]

Subject: Re: [RFC PATCH v3 05/17] irqchip/sun4i: add support for suniv interrupt controller

On Thu, Nov 22, 2018 at 06:02:00PM +0300, Mesih Kilinc wrote:
> > > diff --git a/drivers/irqchip/irq-sun4i.c b/drivers/irqchip/irq-sun4i.c
> > > index e3e5b91..7ca4a4d 100644
> > > --- a/drivers/irqchip/irq-sun4i.c
> > > +++ b/drivers/irqchip/irq-sun4i.c
> > > @@ -28,11 +28,21 @@
> > > #define SUN4I_IRQ_NMI_CTRL_REG 0x0c
> > > #define SUN4I_IRQ_PENDING_REG(x) (0x10 + 0x4 * x)
> > > #define SUN4I_IRQ_FIQ_PENDING_REG(x) (0x20 + 0x4 * x)
> > > -#define SUN4I_IRQ_ENABLE_REG(x) (0x40 + 0x4 * x)
> > > -#define SUN4I_IRQ_MASK_REG(x) (0x50 + 0x4 * x)
> > > +#define SUN4I_IRQ_ENABLE_REG(x) (irq_ic_data->enable_req_offset + 0x4 * x)
> > > +#define SUN4I_IRQ_MASK_REG(x) (irq_ic_data->mask_req_offset + 0x4 * x)
> >
> > You shouldn't have all the values you use passed as argument, so
> > irq_ic_data should be one of them here.
> >
>
> Could you elaborate it a little bit?

You should change your macro to something like:

SUN4I_IRQ_ENABLE_REG(data, x) ((data)->enable_req_offset + 0x4 * x)

That way, you are transparent about the parameters that the macro
expects, and don't rely on some unwritten rule about the variable
name.

> > > +
> > > +static int __init suniv_ic_of_init(struct device_node *node,
> > > + struct device_node *parent)
> > > +{
> > > + irq_ic_data = kzalloc(sizeof(struct sunxi_irq_chip_data), GFP_KERNEL);
> > > + if (!irq_ic_data) {
> > > + pr_err("kzalloc failed!\n");
> > > + return -ENOMEM;
> > > + }
> > > +
> > > + irq_ic_data->enable_req_offset = SUNIV_IRQ_ENABLE_REG_OFFSET;
> > > + irq_ic_data->mask_req_offset = SUNIV_IRQ_MASK_REG_OFFSET;
> > > +
> > > + return sun4i_of_init(node, parent);
> > > +}
> > > +
> > > +IRQCHIP_DECLARE(allwinner_sunvi_ic, "allwinner,suniv-f1c100s-ic", suniv_ic_of_init);
> >
> > You can even split that addition to a new patch as well.
>
> OK. I will do 3 patches. First one will add a struct that holds only
> base and domain. Second one will add register offsets to that struct.
> Third one will add f1c100s support. Is that ok?

Sounds perfect :)

Thanks!
Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Attachments:

(No filename) (2.14 kB)
signature.asc (235.00 B)
Download all attachments