[00/14] Add basic support for Socionext Milbeaut M10V SoCs

Message ID 1542589274-13878-1-git-send-email-sugaya.taichi@socionext.com
Headers show
Series
  • Add basic support for Socionext Milbeaut M10V SoCs
Related show

Message

Sugaya, Taichi Nov. 19, 2018, 1:01 a.m.
Hi,

Here is the series of patches the initial support for SC2000(M10V) of
Milbeaut SoCs. "M10V" is the internal name of SC2000, so commonly used in
source code.

SC2000 is a SoC of the Milbeaut series. equipped with a DSP optimized for
computer vision. It also features advanced functionalities such as 360-degree,
real-time spherical stitching with multi cameras, image stabilization for
without mechanical gimbals, and rolling shutter correction. More detail is
below:
https://www.socionext.com/en/products/assp/milbeaut/SC2000.html

Specifications for developers are below:
 - Quad-core 32bit Cortex-A7 on ARMv7-A architecture
 - NEON support
 - DSP
 - GPU
 - MAX 3GB DDR3
 - Cortex-M0 for power control
 - NAND Flash Interface
 - SD UHS-I
 - SD UHS-II
 - SDIO
 - USB2.0 HOST / Device
 - USB3.0 HOST / Device
 - PCI express Gen2
 - Ethernet Engine
 - I2C
 - UART
 - SPI
 - PWM

Support is quite minimal for now, since it only includes timer, clock,
pictrl and serial controller drivers, so we can only boot to userspace
through initramfs. Support for the other peripherals  will come eventually.


Sugaya Taichi (14):
  ARM: milbeaut: Add basic support for Milbeaut m10v SoC
  dt-bindings: soc: milbeaut: Add Milbeaut trampoline description
  ARM: milbeaut: Add Milbeaut M10V early printk
  dt-bindings: timer: Add Milbeaut M10V timer description
  clocksource/drivers/timer-milbeaut: Add Milbeaut M10V timer
  dt-bindings: clock: milbeaut: add Milbeaut clock description
  clock: milbeaut: Add Milbeaut M10V clock control
  dt-bindings: serial: Add Milbeaut M10V serial description
  serial: Add Milbeaut M10V serial control
  dt-bindings: pinctrl: milbeaut: Add Milbeaut M10V pinctrl description
  pinctrl: milbeaut: Add Milbeaut M10V pinctrl
  ARM: dts: milbeaut: Add device tree set for the Milbeaut M10V board
  ARM: configs: Add Milbeaut M10V defconfig
  MAINTAINERS: Add entry to MAINTAINERS for Milbeaut

 .../devicetree/bindings/clock/milbeaut-clock.txt   |  93 +++
 .../pinctrl/socionext,milbeaut-pinctrl.txt         |  33 +
 .../devicetree/bindings/serial/milbeaut-uart.txt   |  23 +
 .../bindings/soc/socionext/socionext,m10v.txt      |  12 +
 .../bindings/timer/socionext,milbeaut-timer.txt    |  17 +
 MAINTAINERS                                        |   9 +
 arch/arm/Kconfig                                   |   2 +
 arch/arm/Kconfig.debug                             |  12 +-
 arch/arm/Makefile                                  |   1 +
 arch/arm/boot/dts/Makefile                         |   1 +
 arch/arm/boot/dts/milbeaut-m10v-evb.dts            |  35 +
 arch/arm/boot/dts/milbeaut-m10v-evb.dtsi           |  17 +
 arch/arm/boot/dts/milbeaut-m10v.dtsi               | 510 ++++++++++++++
 arch/arm/configs/milbeaut_m10v_defconfig           | 364 ++++++++++
 arch/arm/include/debug/milbeaut.S                  |  25 +
 arch/arm/mach-milbeaut/Kconfig                     |  28 +
 arch/arm/mach-milbeaut/Makefile                    |   3 +
 arch/arm/mach-milbeaut/m10v_evb.c                  |  31 +
 arch/arm/mach-milbeaut/platsmp.c                   | 157 +++++
 drivers/clk/Makefile                               |   1 +
 drivers/clk/clk-m10v.c                             | 671 ++++++++++++++++++
 drivers/clocksource/Kconfig                        |   8 +
 drivers/clocksource/Makefile                       |   1 +
 drivers/clocksource/timer-m10v.c                   | 146 ++++
 drivers/pinctrl/Kconfig                            |   9 +
 drivers/pinctrl/Makefile                           |   1 +
 drivers/pinctrl/pinctrl-m10v.c                     | 765 +++++++++++++++++++++
 drivers/tty/serial/Kconfig                         |  24 +
 drivers/tty/serial/Makefile                        |   1 +
 drivers/tty/serial/m10v_usio.c                     | 605 ++++++++++++++++
 include/uapi/linux/serial_core.h                   |   3 +
 31 files changed, 3606 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/milbeaut-clock.txt
 create mode 100644 Documentation/devicetree/bindings/pinctrl/socionext,milbeaut-pinctrl.txt
 create mode 100644 Documentation/devicetree/bindings/serial/milbeaut-uart.txt
 create mode 100644 Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt
 create mode 100644 Documentation/devicetree/bindings/timer/socionext,milbeaut-timer.txt
 create mode 100644 arch/arm/boot/dts/milbeaut-m10v-evb.dts
 create mode 100644 arch/arm/boot/dts/milbeaut-m10v-evb.dtsi
 create mode 100644 arch/arm/boot/dts/milbeaut-m10v.dtsi
 create mode 100644 arch/arm/configs/milbeaut_m10v_defconfig
 create mode 100644 arch/arm/include/debug/milbeaut.S
 create mode 100644 arch/arm/mach-milbeaut/Kconfig
 create mode 100644 arch/arm/mach-milbeaut/Makefile
 create mode 100644 arch/arm/mach-milbeaut/m10v_evb.c
 create mode 100644 arch/arm/mach-milbeaut/platsmp.c
 create mode 100644 drivers/clk/clk-m10v.c
 create mode 100644 drivers/clocksource/timer-m10v.c
 create mode 100644 drivers/pinctrl/pinctrl-m10v.c
 create mode 100644 drivers/tty/serial/m10v_usio.c

-- 
1.9.1

Comments

Sugaya, Taichi Nov. 22, 2018, 2:23 a.m. | #1
Hi Daniel

Thank you for your comments.

On 2018/11/21 19:08, Daniel Lezcano wrote:
> Hi Sugaya,

>

>

> On 19/11/2018 02:01, Sugaya Taichi wrote:

>> Add Milbeaut M10V timer using 32bit timer in peripheral.

> Give a better description of the timer as it is new timer introduced.


I got it. Add more description.


>> Signed-off-by: Sugaya Taichi <sugaya.taichi@socionext.com>

>> ---

>>   drivers/clocksource/Kconfig      |   8 +++

>>   drivers/clocksource/Makefile     |   1 +

>>   drivers/clocksource/timer-m10v.c | 146 +++++++++++++++++++++++++++++++++++++++

>>   3 files changed, 155 insertions(+)

>>   create mode 100644 drivers/clocksource/timer-m10v.c

>>

>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig

>> index 55c77e4..a278d72 100644

>> --- a/drivers/clocksource/Kconfig

>> +++ b/drivers/clocksource/Kconfig

>> @@ -638,4 +638,12 @@ config GX6605S_TIMER

>>   	help

>>   	  This option enables support for gx6605s SOC's timer.

>>   

>> +config M10V_TIMER

>> +	bool "Milbeaut M10V timer driver" if COMPILE_TEST

>> +	depends on OF

>> +	depends on ARM

>> +	select TIMER_OF

>> +	help

>> +	  Enables the support for Milbeaut M10V timer driver.

>> +

>>   endmenu

>> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile

>> index dd91381..8e908b4 100644

>> --- a/drivers/clocksource/Makefile

>> +++ b/drivers/clocksource/Makefile

>> @@ -55,6 +55,7 @@ obj-$(CONFIG_CLKSRC_TI_32K)	+= timer-ti-32k.o

>>   obj-$(CONFIG_CLKSRC_NPS)	+= timer-nps.o

>>   obj-$(CONFIG_OXNAS_RPS_TIMER)	+= timer-oxnas-rps.o

>>   obj-$(CONFIG_OWL_TIMER)		+= timer-owl.o

>> +obj-$(CONFIG_M10V_TIMER)	+= timer-m10v.o

>>   obj-$(CONFIG_SPRD_TIMER)	+= timer-sprd.o

>>   obj-$(CONFIG_NPCM7XX_TIMER)	+= timer-npcm7xx.o

>>   

>> diff --git a/drivers/clocksource/timer-m10v.c b/drivers/clocksource/timer-m10v.c

>> new file mode 100644

>> index 0000000..ff97c23

>> --- /dev/null

>> +++ b/drivers/clocksource/timer-m10v.c

>> @@ -0,0 +1,146 @@

>> +// SPDX-License-Identifier: GPL-2.0

>> +/*

>> + * Copyright (C) 2018 Socionext Inc.

>> + */

>> +

>> +#include <linux/clk.h>

> ------->

>

>> +#include <linux/clockchips.h>

> It is included from timer-of.h

>

> <-------


OK. Remove it.


>

>> +#include <linux/interrupt.h>

>> +#include <linux/irq.h>

>> +#include <linux/irqreturn.h>

> ------->

>

>> +#include <linux/of.h>

>> +#include <linux/of_address.h>

>> +#include <linux/of_irq.h>

>> +#include <linux/slab.h>

> Those are not needed IMO.

>

> <---------


OK, remove these.


>

>

>> +#include "timer-of.h"

>> +

>> +#define FSL_TMR_TMCSR_OFS	0x0

>> +#define FSL_TMR_TMR_OFS		0x4

>> +#define FSL_TMR_TMRLR1_OFS	0x8

>> +#define FSL_TMR_TMRLR2_OFS	0xc

>> +#define FSL_RMT_REGSZPCH	0x10

>> +

>> +#define FSL_TMR_TMCSR_OUTL	BIT(5)

>> +#define FSL_TMR_TMCSR_RELD	BIT(4)

>> +#define FSL_TMR_TMCSR_INTE	BIT(3)

>> +#define FSL_TMR_TMCSR_UF	BIT(2)

>> +#define FSL_TMR_TMCSR_CNTE	BIT(1)

>> +#define FSL_TMR_TMCSR_TRG	BIT(0)

>> +

>> +#define FSL_TMR_TMCSR_CSL_DIV2	0

>> +#define FSL_TMR_TMCSR_CSL	BIT(10)

>> +

>> +#define M10V_TIMER_RATING	500

>> +

>> +static irqreturn_t m10v_timer_interrupt(int irq, void *dev_id)

>> +{

>> +	struct clock_event_device *clk = dev_id;

>> +	struct timer_of *to = to_timer_of(clk);

>> +	u32 val;

>> +

>> +	val = readl_relaxed(timer_of_base(to) + FSL_TMR_TMCSR_OFS);

>> +	val &= ~FSL_TMR_TMCSR_UF;

>> +	writel_relaxed(val, timer_of_base(to) + FSL_TMR_TMCSR_OFS);

>> +

>> +	clk->event_handler(clk);

>> +

>> +	return IRQ_HANDLED;

>> +}

>> +

>> +static int m10v_set_state_periodic(struct clock_event_device *clk)

>> +{

>> +	struct timer_of *to = to_timer_of(clk);

>> +	u32 val = (FSL_TMR_TMCSR_CSL_DIV2 * FSL_TMR_TMCSR_CSL);

> 	FSL_TMR_TMCSR_CSL_DIV2 is zero, so val is always zero.

Ah, yes.
The FSL_TMR_TMCSR is the 10th bit field of the register, so it would be 
better
to bit-shift.
I will use it as simply 0.

> 	

>> +	writel_relaxed(val, timer_of_base(to) + FSL_TMR_TMCSR_OFS);

>> +

>> +	writel_relaxed(to->of_clk.period, timer_of_base(to) +

>> +				FSL_TMR_TMRLR1_OFS);

>> +	val |= FSL_TMR_TMCSR_RELD | FSL_TMR_TMCSR_CNTE |

>> +		FSL_TMR_TMCSR_TRG | FSL_TMR_TMCSR_INTE;

>> +	writel_relaxed(val, timer_of_base(to) + FSL_TMR_TMCSR_OFS);

>> +	return 0;

>> +}

>> +

>> +static int m10v_set_state_oneshot(struct clock_event_device *clk)

>> +{

>> +	struct timer_of *to = to_timer_of(clk);

>> +	u32 val = (FSL_TMR_TMCSR_CSL_DIV2 * FSL_TMR_TMCSR_CSL);

>> +

>> +	writel_relaxed(val, timer_of_base(to) + FSL_TMR_TMCSR_OFS);

>> +	return 0;

>> +}

>> +

>> +static int m10v_clkevt_next_event(unsigned long event,

>> +				   struct clock_event_device *clk)

>> +{

>> +	struct timer_of *to = to_timer_of(clk);

>> +

>> +	writel_relaxed(event, timer_of_base(to) + FSL_TMR_TMRLR1_OFS);

>> +	writel_relaxed((FSL_TMR_TMCSR_CSL_DIV2 * FSL_TMR_TMCSR_CSL) |

> Same comment here.


I got it.


>> +			FSL_TMR_TMCSR_CNTE | FSL_TMR_TMCSR_INTE |

>> +			FSL_TMR_TMCSR_TRG, timer_of_base(to) +

>> +			FSL_TMR_TMCSR_OFS);

>> +	return 0;

>> +}

>> +

>> +static int m10v_config_clock_source(struct timer_of *to)

>> +{

>> +	writel_relaxed(0, timer_of_base(to) + FSL_TMR_TMCSR_OFS);

>> +	writel_relaxed(~0, timer_of_base(to) + FSL_TMR_TMR_OFS);

>> +	writel_relaxed(~0, timer_of_base(to) + FSL_TMR_TMRLR1_OFS);

>> +	writel_relaxed(~0, timer_of_base(to) + FSL_TMR_TMRLR2_OFS);

>> +	writel_relaxed(BIT(4) | BIT(1) | BIT(0), timer_of_base(to) +

>> +		FSL_TMR_TMCSR_OFS);

>> +	return 0;

>> +}

>> +

>> +static int m10v_config_clock_event(struct timer_of *to)

>> +{

>> +	writel_relaxed(0, timer_of_base(to) + FSL_TMR_TMCSR_OFS);

>> +	return 0;

>> +}

>> +

>> +static struct timer_of to = {

>> +	.flags = TIMER_OF_IRQ | TIMER_OF_BASE | TIMER_OF_CLOCK,

>> +

>> +	.clkevt = {

>> +		.name = "m10v-clkevt",

>> +		.rating = M10V_TIMER_RATING,

>> +		.cpumask = cpu_possible_mask,

>> +	},

>> +

>> +	.of_irq = {

>> +		.flags = IRQF_TIMER | IRQF_IRQPOLL,

>> +	},

>> +};

>> +

>> +static int __init m10v_timer_init(struct device_node *node)

>> +{

>> +	int ret;

>> +

>> +	to.clkevt.features = CLOCK_EVT_FEAT_DYNIRQ | CLOCK_EVT_FEAT_ONESHOT;

>> +	to.clkevt.set_state_oneshot = m10v_set_state_oneshot;

>> +	to.clkevt.set_state_periodic = m10v_set_state_periodic;

>> +	to.clkevt.set_next_event = m10v_clkevt_next_event;

>> +	to.of_irq.handler = m10v_timer_interrupt;

> Move the initialization in the timer_of structure above.


Okay.


>

>> +	ret = timer_of_init(node, &to);

>> +	if (ret)

>> +		goto err;

> You should return directly, rollback is done in the timer_of_init().


OK.


>

>> +

>> +	m10v_config_clock_source(&to);

>> +	clocksource_mmio_init(timer_of_base(&to) + FSL_TMR_TMR_OFS,

>> +		node->name, timer_of_rate(&to), M10V_TIMER_RATING, 32,

>> +		clocksource_mmio_readl_down);

> May be you can add the sched_clock also ?


OK. Add it and confirm.


Thanks
Sugaya Taichi


>

>> +	m10v_config_clock_event(&to);

>> +	clockevents_config_and_register(&to.clkevt, timer_of_rate(&to),

>> +				15, 0xffffffff);

>> +

>> +	return 0;

>> +err:

>> +	timer_of_cleanup(&to);

>> +	return ret;

>> +}

>> +TIMER_OF_DECLARE(m10v_peritimer, "socionext,milbeaut-m10v-timer",

>> +		m10v_timer_init);

>>

>
Sugaya, Taichi Nov. 29, 2018, 12:24 p.m. | #2
Hi,

Thank you for your comments.

On 2018/11/28 11:01, Stephen Boyd wrote:
> Quoting Sugaya Taichi (2018-11-18 17:01:07)

>> Add DT bindings document for Milbeaut trampoline.

>>

>> Signed-off-by: Sugaya Taichi<sugaya.taichi@socionext.com>

>> ---

>>   .../devicetree/bindings/soc/socionext/socionext,m10v.txt     | 12 ++++++++++++

>>   1 file changed, 12 insertions(+)

>>   create mode 100644 Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt

>>

>> diff --git a/Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt b/Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt

>> new file mode 100644

>> index 0000000..f5d906c

>> --- /dev/null

>> +++ b/Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt

>> @@ -0,0 +1,12 @@

>> +Socionext M10V SMP trampoline driver binding

>> +

>> +This is a driver to wait for sub-cores while boot process.

>> +

>> +- compatible: should be "socionext,smp-trampoline"

>> +- reg: should be <0x4C000100 0x100>

>> +

>> +EXAMPLE

>> +       trampoline: trampoline@0x4C000100 {

> Drop the 0x part of unit addresses.


Okay.


>> +               compatible = "socionext,smp-trampoline";

>> +               reg = <0x4C000100 0x100>;

> Looks like a software construct, which we wouldn't want to put into DT

> this way. DT doesn't describe drivers.

We would like to use this node only getting the address of the 
trampoline area
in which sub-cores wait.  (They have finished to go to this area in previous
bootloader process.)

So should we embed the constant value in source codes instead of getting 
from
DT because the address is constant at the moment? Or is there other 
approach?

Thanks
Sugaya Taichi
Stephen Boyd Nov. 30, 2018, 8:16 a.m. | #3
Quoting Sugaya, Taichi (2018-11-29 04:24:51)
> On 2018/11/28 11:01, Stephen Boyd wrote:

> > Quoting Sugaya Taichi (2018-11-18 17:01:07)

> >>   create mode 100644 Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt

> >>

> >> diff --git a/Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt b/Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt

> >> new file mode 100644

> >> index 0000000..f5d906c

> >> --- /dev/null

> >> +++ b/Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt

> >> @@ -0,0 +1,12 @@

> >> +Socionext M10V SMP trampoline driver binding

> >> +

> >> +This is a driver to wait for sub-cores while boot process.

> >> +

> >> +- compatible: should be "socionext,smp-trampoline"

> >> +- reg: should be <0x4C000100 0x100>

> >> +

> >> +EXAMPLE

> >> +       trampoline: trampoline@0x4C000100 {

> > Drop the 0x part of unit addresses.

> 

> Okay.

> 

> 

> >> +               compatible = "socionext,smp-trampoline";

> >> +               reg = <0x4C000100 0x100>;

> > Looks like a software construct, which we wouldn't want to put into DT

> > this way. DT doesn't describe drivers.

> We would like to use this node only getting the address of the 

> trampoline area

> in which sub-cores wait.  (They have finished to go to this area in previous

> bootloader process.)


Is this area part of memory, or a special SRAM? If it's part of memory,
I would expect this node to be under the reserved-memory node and
pointed to by some other node that uses this region. Could even be the
CPU nodes.

> 

> So should we embed the constant value in source codes instead of getting 

> from

> DT because the address is constant at the moment? Or is there other 

> approach?

> 


If it's constant then that also works. Why does it need to come from DT
at all then?
Sugaya, Taichi Dec. 3, 2018, 7:42 a.m. | #4
Hi,

On 2018/11/30 17:16, Stephen Boyd wrote:
> Quoting Sugaya, Taichi (2018-11-29 04:24:51)

>> On 2018/11/28 11:01, Stephen Boyd wrote:

>>> Quoting Sugaya Taichi (2018-11-18 17:01:07)

>>>>    create mode 100644 Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt

>>>>

>>>> diff --git a/Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt b/Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt

>>>> new file mode 100644

>>>> index 0000000..f5d906c

>>>> --- /dev/null

>>>> +++ b/Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt

>>>> @@ -0,0 +1,12 @@

>>>> +Socionext M10V SMP trampoline driver binding

>>>> +

>>>> +This is a driver to wait for sub-cores while boot process.

>>>> +

>>>> +- compatible: should be "socionext,smp-trampoline"

>>>> +- reg: should be <0x4C000100 0x100>

>>>> +

>>>> +EXAMPLE

>>>> +       trampoline: trampoline@0x4C000100 {

>>> Drop the 0x part of unit addresses.

>>

>> Okay.

>>

>>

>>>> +               compatible = "socionext,smp-trampoline";

>>>> +               reg = <0x4C000100 0x100>;

>>> Looks like a software construct, which we wouldn't want to put into DT

>>> this way. DT doesn't describe drivers.

>> We would like to use this node only getting the address of the

>> trampoline area

>> in which sub-cores wait.  (They have finished to go to this area in previous

>> bootloader process.)

> 

> Is this area part of memory, or a special SRAM? If it's part of memory,

> I would expect this node to be under the reserved-memory node and

> pointed to by some other node that uses this region. Could even be the

> CPU nodes.


Yes, 0x4C000100 is a part of memory under the reserved-memory node. So 
we would like to use the SRAM ( allocated 0x00000000 ) area instead.
BTW, sorry, the trampoline address of this example is simply wrong.  We 
were going to use a part of the SRAM from the beginning.

> 

>>

>> So should we embed the constant value in source codes instead of getting

>> from

>> DT because the address is constant at the moment? Or is there other

>> approach?

>>

> 

> If it's constant then that also works. Why does it need to come from DT

> at all then?


We think it is not good to embed constant value in driver codes and do 
not have another way...
Are there better ways?

Thanks
Sugaya Taichi

>
Masahiro Yamada Dec. 4, 2018, 11:03 a.m. | #5
Hi Stephen,


On Fri, Nov 30, 2018 at 5:31 PM Stephen Boyd <sboyd@kernel.org> wrote:
>

> Quoting Sugaya Taichi (2018-11-18 17:01:12)

> > Add Milbeaut M10V clock ( including PLL ) control.

>

> Please give some more details here.

>

> >

> > Signed-off-by: Sugaya Taichi <sugaya.taichi@socionext.com>

> > ---

> >  drivers/clk/Makefile   |   1 +

> >  drivers/clk/clk-m10v.c | 671 +++++++++++++++++++++++++++++++++++++++++++++++++

>

> And this is different from Uniphier? Maybe we need a socionext

> directory under drivers/clk/.




This is always a difficult question,
and I do not have a strong opinion.


I am fine with moving the files to drivers/clk/socionext
although no file would be shared.


FYI

UniPhier and Milbeaut are completely different platforms
developed/maintained by different teams.

They happen to live in the same company now
just because Socionext merged the LSI business from Panasonic and Fujitsu.

UniPhier originates in Panasonic, while Milbeaut in Fujitsu.


Thanks.




--
Best Regards
Masahiro Yamada
Rob Herring Dec. 4, 2018, 1:32 p.m. | #6
On Tue, Dec 4, 2018 at 5:30 AM Sugaya, Taichi
<sugaya.taichi@socionext.com> wrote:
>

> Hi

>

> On 2018/12/04 0:49, Rob Herring wrote:

> > On Mon, Dec 3, 2018 at 1:42 AM Sugaya, Taichi

> > <sugaya.taichi@socionext.com> wrote:

> >>

> >> Hi,

> >>

> >> On 2018/11/30 17:16, Stephen Boyd wrote:

> >>> Quoting Sugaya, Taichi (2018-11-29 04:24:51)

> >>>> On 2018/11/28 11:01, Stephen Boyd wrote:

> >>>>> Quoting Sugaya Taichi (2018-11-18 17:01:07)

> >>>>>>     create mode 100644 Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt

> >>>>>>

> >>>>>> diff --git a/Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt b/Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt

> >>>>>> new file mode 100644

> >>>>>> index 0000000..f5d906c

> >>>>>> --- /dev/null

> >>>>>> +++ b/Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt

> >>>>>> @@ -0,0 +1,12 @@

> >>>>>> +Socionext M10V SMP trampoline driver binding

> >>>>>> +

> >>>>>> +This is a driver to wait for sub-cores while boot process.

> >>>>>> +

> >>>>>> +- compatible: should be "socionext,smp-trampoline"

> >>>>>> +- reg: should be <0x4C000100 0x100>

> >>>>>> +

> >>>>>> +EXAMPLE

> >>>>>> +       trampoline: trampoline@0x4C000100 {

> >>>>> Drop the 0x part of unit addresses.

> >>>>

> >>>> Okay.

> >>>>

> >>>>

> >>>>>> +               compatible = "socionext,smp-trampoline";

> >>>>>> +               reg = <0x4C000100 0x100>;

> >>>>> Looks like a software construct, which we wouldn't want to put into DT

> >>>>> this way. DT doesn't describe drivers.

> >>>> We would like to use this node only getting the address of the

> >>>> trampoline area

> >>>> in which sub-cores wait.  (They have finished to go to this area in previous

> >>>> bootloader process.)

> >>>

> >>> Is this area part of memory, or a special SRAM? If it's part of memory,

> >>> I would expect this node to be under the reserved-memory node and

> >>> pointed to by some other node that uses this region. Could even be the

> >>> CPU nodes.

> >>

> >> Yes, 0x4C000100 is a part of memory under the reserved-memory node. So

> >> we would like to use the SRAM ( allocated 0x00000000 ) area instead.

> >> BTW, sorry, the trampoline address of this example is simply wrong.  We

> >> were going to use a part of the SRAM from the beginning.

> >>

> >>>

> >>>>

> >>>> So should we embed the constant value in source codes instead of getting

> >>>> from

> >>>> DT because the address is constant at the moment? Or is there other

> >>>> approach?

> >>>>

> >>>

> >>> If it's constant then that also works. Why does it need to come from DT

> >>> at all then?

> >>

> >> We think it is not good to embed constant value in driver codes and do

> >> not have another way...

> >> Are there better ways?

> >

> > If this is just memory, can you use the standard spin-table binding in

> > the DT spec? There are some requirements like 64-bit values even on

> > 32-bit machines (though this gets violated).

>

> The spin-table seems to be used on only 64-bit arch. Have it ever worked

> on 32-bit machine?


Yes.

> And I would like not to use it because avoid violation.


The issue now that I remember is cpu-release-addr is defined to always
be a 64-bit value while some platforms made it a 32-bit value.
'cpu-release-addr' is also used for some other enable-methods.

Rob
Stephen Boyd Dec. 4, 2018, 6:15 p.m. | #7
Quoting Sugaya, Taichi (2018-12-04 00:26:16)
> On 2018/11/30 17:31, Stephen Boyd wrote:

> > Quoting Sugaya Taichi (2018-11-18 17:01:12)

> >> +void __init m10v_clk_mux_setup(struct device_node *node)

> >> +{

> >> +       const char *clk_name = node->name;

> >> +       struct clk_init_data init;

> >> +       const char **parent_names;

> >> +       struct m10v_mux *mcm;

> >> +       struct clk *clk;

> >> +       int i, parents;

> >> +

> >> +       if (!m10v_clk_iomap())

> >> +               return;

> >> +

> >> +       of_property_read_string(node, "clock-output-names", &clk_name);

> >> +

> >> +       parents = of_clk_get_parent_count(node);

> >> +       if (parents < 2) {

> >> +               pr_err("%s: not a mux\n", clk_name);

> > 

> > How is this possible?

> 

> When the node has more than 1 clks...

> Or I am misunderstanding your question?


This looks like code that's checking DT for correctness. We don't
typically do that in the kernel because the kernel isn't a DT validator.
That's all I'm saying. I think this comment is not useful if the driver
design is done to specify parent linkages in C code instead of DT, so
don't worry about this too much.
Sugaya, Taichi Dec. 5, 2018, 10:30 a.m. | #8
Hi,

On 2018/12/04 22:32, Rob Herring wrote:
> On Tue, Dec 4, 2018 at 5:30 AM Sugaya, Taichi

> <sugaya.taichi@socionext.com> wrote:

>>

>> Hi

>>

>> On 2018/12/04 0:49, Rob Herring wrote:

>>> On Mon, Dec 3, 2018 at 1:42 AM Sugaya, Taichi

>>> <sugaya.taichi@socionext.com> wrote:

>>>>

>>>> Hi,

>>>>

>>>> On 2018/11/30 17:16, Stephen Boyd wrote:

>>>>> Quoting Sugaya, Taichi (2018-11-29 04:24:51)

>>>>>> On 2018/11/28 11:01, Stephen Boyd wrote:

>>>>>>> Quoting Sugaya Taichi (2018-11-18 17:01:07)

>>>>>>>>      create mode 100644 Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt

>>>>>>>>

>>>>>>>> diff --git a/Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt b/Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt

>>>>>>>> new file mode 100644

>>>>>>>> index 0000000..f5d906c

>>>>>>>> --- /dev/null

>>>>>>>> +++ b/Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt

>>>>>>>> @@ -0,0 +1,12 @@

>>>>>>>> +Socionext M10V SMP trampoline driver binding

>>>>>>>> +

>>>>>>>> +This is a driver to wait for sub-cores while boot process.

>>>>>>>> +

>>>>>>>> +- compatible: should be "socionext,smp-trampoline"

>>>>>>>> +- reg: should be <0x4C000100 0x100>

>>>>>>>> +

>>>>>>>> +EXAMPLE

>>>>>>>> +       trampoline: trampoline@0x4C000100 {

>>>>>>> Drop the 0x part of unit addresses.

>>>>>>

>>>>>> Okay.

>>>>>>

>>>>>>

>>>>>>>> +               compatible = "socionext,smp-trampoline";

>>>>>>>> +               reg = <0x4C000100 0x100>;

>>>>>>> Looks like a software construct, which we wouldn't want to put into DT

>>>>>>> this way. DT doesn't describe drivers.

>>>>>> We would like to use this node only getting the address of the

>>>>>> trampoline area

>>>>>> in which sub-cores wait.  (They have finished to go to this area in previous

>>>>>> bootloader process.)

>>>>>

>>>>> Is this area part of memory, or a special SRAM? If it's part of memory,

>>>>> I would expect this node to be under the reserved-memory node and

>>>>> pointed to by some other node that uses this region. Could even be the

>>>>> CPU nodes.

>>>>

>>>> Yes, 0x4C000100 is a part of memory under the reserved-memory node. So

>>>> we would like to use the SRAM ( allocated 0x00000000 ) area instead.

>>>> BTW, sorry, the trampoline address of this example is simply wrong.  We

>>>> were going to use a part of the SRAM from the beginning.

>>>>

>>>>>

>>>>>>

>>>>>> So should we embed the constant value in source codes instead of getting

>>>>>> from

>>>>>> DT because the address is constant at the moment? Or is there other

>>>>>> approach?

>>>>>>

>>>>>

>>>>> If it's constant then that also works. Why does it need to come from DT

>>>>> at all then?

>>>>

>>>> We think it is not good to embed constant value in driver codes and do

>>>> not have another way...

>>>> Are there better ways?

>>>

>>> If this is just memory, can you use the standard spin-table binding in

>>> the DT spec? There are some requirements like 64-bit values even on

>>> 32-bit machines (though this gets violated).

>>

>> The spin-table seems to be used on only 64-bit arch. Have it ever worked

>> on 32-bit machine?

> 

> Yes.

> 

>> And I would like not to use it because avoid violation.

> 

> The issue now that I remember is cpu-release-addr is defined to always

> be a 64-bit value while some platforms made it a 32-bit value.

> 'cpu-release-addr' is also used for some other enable-methods.


Thanks.
OK, try to use the spin-table.

Best Regards,
Sugaya Taichi

> 

> Rob

>
Sugaya, Taichi Dec. 26, 2018, 1:35 a.m. | #9
Hi

On 2018/11/30 17:31, Stephen Boyd wrote:
>> +       init.num_parents = parents;

>> +       init.parent_names = parent_names;

>> +

>> +       mcm->cname = clk_name;

>> +       mcm->parent = 0;

>> +       mcm->hw.init = &init;

>> +

>> +       clk = clk_register(NULL, &mcm->hw);

>> +       if (IS_ERR(clk))

>> +               goto err_clk;

>> +

>> +       of_clk_add_provider(node, of_clk_src_simple_get, clk);

>> +       return;

>> +

>> +err_clk:

>> +       kfree(mcm);

>> +err_mcm:

>> +       kfree(parent_names);

>> +}

>> +CLK_OF_DECLARE(m10v_clk_mux, "socionext,milbeaut-m10v-clk-mux",

>> +                       m10v_clk_mux_setup);

> 

> Any chance you can use a platform driver?

> 


Excuse me to re-ask you.
Why do you recommend to use a platform driver? Is that current fad?

Thanks
Sugaya Taichi
Russell King - ARM Linux admin Jan. 30, 2019, 8:40 a.m. | #10
On Tue, Jan 29, 2019 at 05:28:48PM +0900, Sugaya, Taichi wrote:
> Hi,

> 

> On 2019/01/22 20:50, Russell King - ARM Linux admin wrote:

> > On Tue, Jan 22, 2019 at 08:36:03PM +0900, Sugaya, Taichi wrote:

> > > Hi

> > > 

> > > On 2018/12/04 22:32, Rob Herring wrote:

> > > > On Tue, Dec 4, 2018 at 5:30 AM Sugaya, Taichi

> > > > <sugaya.taichi@socionext.com> wrote:

> > > > > 

> > > > > Hi

> > > > > 

> > > > > On 2018/12/04 0:49, Rob Herring wrote:

> > > > > > On Mon, Dec 3, 2018 at 1:42 AM Sugaya, Taichi

> > > > > > <sugaya.taichi@socionext.com> wrote:

> > > > > > > 

> > > > > > > Hi,

> > > > > > > 

> > > > > > > On 2018/11/30 17:16, Stephen Boyd wrote:

> > > > > > > > Quoting Sugaya, Taichi (2018-11-29 04:24:51)

> > > > > > > > > On 2018/11/28 11:01, Stephen Boyd wrote:

> > > > > > > > > > Quoting Sugaya Taichi (2018-11-18 17:01:07)

> > > > > > > > > > >       create mode 100644 Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt

> > > > > > > > > > > 

> > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt b/Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt

> > > > > > > > > > > new file mode 100644

> > > > > > > > > > > index 0000000..f5d906c

> > > > > > > > > > > --- /dev/null

> > > > > > > > > > > +++ b/Documentation/devicetree/bindings/soc/socionext/socionext,m10v.txt

> > > > > > > > > > > @@ -0,0 +1,12 @@

> > > > > > > > > > > +Socionext M10V SMP trampoline driver binding

> > > > > > > > > > > +

> > > > > > > > > > > +This is a driver to wait for sub-cores while boot process.

> > > > > > > > > > > +

> > > > > > > > > > > +- compatible: should be "socionext,smp-trampoline"

> > > > > > > > > > > +- reg: should be <0x4C000100 0x100>

> > > > > > > > > > > +

> > > > > > > > > > > +EXAMPLE

> > > > > > > > > > > +       trampoline: trampoline@0x4C000100 {

> > > > > > > > > > Drop the 0x part of unit addresses.

> > > > > > > > > 

> > > > > > > > > Okay.

> > > > > > > > > 

> > > > > > > > > 

> > > > > > > > > > > +               compatible = "socionext,smp-trampoline";

> > > > > > > > > > > +               reg = <0x4C000100 0x100>;

> > > > > > > > > > Looks like a software construct, which we wouldn't want to put into DT

> > > > > > > > > > this way. DT doesn't describe drivers.

> > > > > > > > > We would like to use this node only getting the address of the

> > > > > > > > > trampoline area

> > > > > > > > > in which sub-cores wait.  (They have finished to go to this area in previous

> > > > > > > > > bootloader process.)

> > > > > > > > 

> > > > > > > > Is this area part of memory, or a special SRAM? If it's part of memory,

> > > > > > > > I would expect this node to be under the reserved-memory node and

> > > > > > > > pointed to by some other node that uses this region. Could even be the

> > > > > > > > CPU nodes.

> > > > > > > 

> > > > > > > Yes, 0x4C000100 is a part of memory under the reserved-memory node. So

> > > > > > > we would like to use the SRAM ( allocated 0x00000000 ) area instead.

> > > > > > > BTW, sorry, the trampoline address of this example is simply wrong.  We

> > > > > > > were going to use a part of the SRAM from the beginning.

> > > > > > > 

> > > > > > > > 

> > > > > > > > > 

> > > > > > > > > So should we embed the constant value in source codes instead of getting

> > > > > > > > > from

> > > > > > > > > DT because the address is constant at the moment? Or is there other

> > > > > > > > > approach?

> > > > > > > > > 

> > > > > > > > 

> > > > > > > > If it's constant then that also works. Why does it need to come from DT

> > > > > > > > at all then?

> > > > > > > 

> > > > > > > We think it is not good to embed constant value in driver codes and do

> > > > > > > not have another way...

> > > > > > > Are there better ways?

> > > > > > 

> > > > > > If this is just memory, can you use the standard spin-table binding in

> > > > > > the DT spec? There are some requirements like 64-bit values even on

> > > > > > 32-bit machines (though this gets violated).

> > > > > 

> > > > > The spin-table seems to be used on only 64-bit arch. Have it ever worked

> > > > > on 32-bit machine?

> > > > 

> > > > Yes.

> > > > 

> > > > > And I would like not to use it because avoid violation.

> > > > 

> > > > The issue now that I remember is cpu-release-addr is defined to always

> > > > be a 64-bit value while some platforms made it a 32-bit value.

> > > > 'cpu-release-addr' is also used for some other enable-methods.

> > > 

> > > I have a question about the spin-table.

> > > The code "smp_spin_table.c" is only in "arch/arm64/kernel" directory so can

> > > not be compiled in arm-v7 arch. That means the spin-table can not be used

> > > arm-v7 arch..? , or is there the way to compile the code in arm-v7 arch?

> > 

> > Why do you think you need it?  Do you have no way to control individual

> > CPUs?

> > 

> > We are going through a process in 32-bit eliminating the "spin table"

> > stuff from platforms.

> > 

> 

> Not always necessary to us and considering the history I think it is not

> suitable to use the spin-table.

> I try to use another way.


Please do - the "spin" approach was a hack to allow ARM Ltd's platforms
to work.  These platforms have no power control or reset of secondary
CPUs and no low power states (so can't suspend/resume).  Early firmware
only had the capabilities to release _all_ secondary CPUs to the kernel
together.  The "spin" approach is incompatible with suspend/resume,
hibernate, and kexec features of the kernel.

Real platforms do not have these problems, and thus should not use the
spin-table approach.

Modern platforms use PSCI to make the control of low power modes and
secondary CPUs independent of the kernel.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up