| Message ID | 1542589274-13878-1-git-send-email-sugaya.taichi@socionext.com |
|---|---|
| Headers | show |
| Series | Add basic support for Socionext Milbeaut M10V SoCs | expand |
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); >> >
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
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?
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 >
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
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
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.
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 >
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
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