| Message ID | 1438696464-59858-6-git-send-email-scott.shu@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 04, 2015 at 09:54:21PM +0800, Scott Shu wrote: > Add support for cpu enable-method "mediatek,mt6580-smp" for booting > secondary CPUs on MT6580. If you have CPU power domain support, and you power up and power down the CPUs, why do you need the boot_lock and pen_release stuff? Isn't keeping the power off on a CPU sufficient to prevent it entering the kernel?
On Tuesday, August 04, 2015 09:54:21 PM Scott Shu wrote: > Add support for cpu enable-method "mediatek,mt6580-smp" for booting > secondary CPUs on MT6580. > > Signed-off-by: Scott Shu <scott.shu@mediatek.com> > --- > arch/arm/mach-mediatek/platsmp.c | 137 > ++++++++++++++++++++++++++++++++++++++ 1 file changed, 137 insertions(+) > > diff --git a/arch/arm/mach-mediatek/platsmp.c > b/arch/arm/mach-mediatek/platsmp.c index a5bc108..94f7865 100644 > --- a/arch/arm/mach-mediatek/platsmp.c > +++ b/arch/arm/mach-mediatek/platsmp.c > @@ -21,10 +21,16 @@ > #include <linux/of_address.h> > #include <linux/string.h> > #include <linux/threads.h> > +#include <linux/delay.h> > +#include <asm/cacheflush.h> > + > +#include <linux/soc/mediatek/scpsys.h> > > #define MTK_MAX_CPU 8 > #define MTK_SMP_REG_SIZE 0x1000 > > +static DEFINE_SPINLOCK(boot_lock); > + > struct mtk_smp_boot_info { > unsigned long smp_base; > unsigned int jump_reg; > @@ -56,6 +62,126 @@ static const struct of_device_id mtk_smp_boot_infos[] > __initconst = { static void __iomem *mtk_smp_base; > static const struct mtk_smp_boot_info *mtk_smp_info; > > +#ifdef CONFIG_HOTPLUG_CPU > +static int mt6580_cpu_kill(unsigned cpu) > +{ > + int ret; > + > + ret = spm_cpu_mtcmos_off(cpu, 1); is this function ever called with wfi == 0? > + if (ret < 0) > + return 0; > + > + return 1; > +} > + > +static void mt6580_cpu_die(unsigned int cpu) > +{ > + for (;;) > + cpu_do_idle(); > +} > +#endif > + > +static void write_pen_release(int val) > +{ > + pen_release = val; > + /* Make sure this is visible to other CPUs */ > + smp_wmb(); > + sync_cache_w(&pen_release); > +} > + > +/* > + * Refer common "pen" secondary release method > + */ > +static int mt6580_boot_secondary(unsigned int cpu, struct task_struct > *idle) +{ > + unsigned long timeout; > + int ret; > + > + /* > + * Set synchronisation state between this boot processor > + * and the secondary one > + */ > + spin_lock(&boot_lock); > + > + /* > + * The secondary processor is waiting to be released from > + * the holding pen - release it, then wait for it to flag > + * that it has been released by resetting pen_release. > + * > + * Note that "pen_release" is the hardware CPU ID, whereas > + * "cpu" is Linux's internal ID. > + */ > + write_pen_release(cpu); > + > + /* > + * CPU power on control by SPM > + */ > + ret = spm_cpu_mtcmos_on(cpu); > + if (ret < 0) { > + spin_unlock(&boot_lock); > + return -ENXIO; > + } > + > + timeout = jiffies + (1 * HZ); > + while (time_before(jiffies, timeout)) { > + /* Read barrier */ > + smp_rmb(); > + > + if (pen_release == -1) > + break; > + > + udelay(10); > + } > + > + /* > + * Now the secondary core is starting up let it run its > + * calibrations, then wait for it to finish > + */ > + spin_unlock(&boot_lock); > + > + return (pen_release != -1 ? -ENXIO : 0); > +} > + > +static void mt6580_secondary_init(unsigned int cpu) > +{ > + /* > + * Let the primary processor know we're out of the > + * pen, then head off into the C entry point > + */ > + write_pen_release(-1); > + > + /* > + * Synchronise with the boot thread. > + */ > + spin_lock(&boot_lock); > + spin_unlock(&boot_lock); > +} > + > +#define MT6580_INFRACFG_AO 0x10001000 > +#define SW_ROM_PD BIT(31) Please put the defines on the beginning of the file. SW_ROM_PD bit is the same on other SoCs? I wasn't able to reveal that from the data sheets. If this is MT6580 only, the define should be renamed. > + > +static void __init mt6580_smp_prepare_cpus(unsigned int max_cpus) > +{ > + static void __iomem *infracfg_ao_base; > + > + infracfg_ao_base = ioremap(MT6580_INFRACFG_AO, 0x1000); > + if (!infracfg_ao_base) { > + pr_err("%s: Unable to map I/O memory\n", __func__); > + return; > + } > + > + /* Enable bootrom power down mode */ > + writel_relaxed(readl(infracfg_ao_base + 0x804) | SW_ROM_PD, > + infracfg_ao_base + 0x804); Please use a define for the offset. I prefer to not cascade reads and writes but to use a temporary variable. But if this is fine with the kernel coding style (I doubt, but I'm not sure) then this is fine. > + > + /* Write the address of slave startup into boot address > + register for bootrom power down mode */ > + writel_relaxed(virt_to_phys(secondary_startup_arm), > + infracfg_ao_base + 0x800); > + > + iounmap(infracfg_ao_base); > +} > + > static int mtk_boot_secondary(unsigned int cpu, struct task_struct *idle) > { > if (!mtk_smp_base) > @@ -142,3 +268,14 @@ static struct smp_operations mt6589_smp_ops __initdata > = { .smp_boot_secondary = mtk_boot_secondary, > }; > CPU_METHOD_OF_DECLARE(mt6589_smp, "mediatek,mt6589-smp", &mt6589_smp_ops); > + > +static struct smp_operations mt6580_smp_ops __initdata = { > + .smp_prepare_cpus = mt6580_smp_prepare_cpus, > + .smp_secondary_init = mt6580_secondary_init, > + .smp_boot_secondary = mt6580_boot_secondary, > +#ifdef CONFIG_HOTPLUG_CPU > + .cpu_kill = mt6580_cpu_kill, > + .cpu_die = mt6580_cpu_die, > +#endif > +}; > +CPU_METHOD_OF_DECLARE(mt6580_smp, "mediatek,mt6580-smp", &mt6580_smp_ops);
On Wed, Aug 05, 2015 at 06:47:03PM +0200, Matthias Brugger wrote: > On Tuesday, August 04, 2015 09:54:21 PM Scott Shu wrote: > > Add support for cpu enable-method "mediatek,mt6580-smp" for booting > > secondary CPUs on MT6580. > > > > Signed-off-by: Scott Shu <scott.shu@mediatek.com> > > --- > > arch/arm/mach-mediatek/platsmp.c | 137 > > ++++++++++++++++++++++++++++++++++++++ 1 file changed, 137 insertions(+) > > + > > +static void __init mt6580_smp_prepare_cpus(unsigned int max_cpus) > > +{ > > + static void __iomem *infracfg_ao_base; > > + > > + infracfg_ao_base = ioremap(MT6580_INFRACFG_AO, 0x1000); > > + if (!infracfg_ao_base) { > > + pr_err("%s: Unable to map I/O memory\n", __func__); > > + return; > > + } > > + > > + /* Enable bootrom power down mode */ > > + writel_relaxed(readl(infracfg_ao_base + 0x804) | SW_ROM_PD, > > + infracfg_ao_base + 0x804); > > Please use a define for the offset. > I prefer to not cascade reads and writes but to use a temporary variable. But > if this is fine with the kernel coding style (I doubt, but I'm not sure) then > this is fine. For what it's worth I also prefer val = readl(infracfg_ao_base + 0x804); val |= SW_ROM_PD: writel_relaxed(val, infracfg_ao_base + 0x804); I find this better readable. I don't think though that there's common agreement that this style is better. Sascha
On Wed, 2015-08-05 at 18:47 +0200, Matthias Brugger wrote: > On Tuesday, August 04, 2015 09:54:21 PM Scott Shu wrote: > > Add support for cpu enable-method "mediatek,mt6580-smp" for booting > > secondary CPUs on MT6580. > > > > Signed-off-by: Scott Shu <scott.shu@mediatek.com> > > --- > > arch/arm/mach-mediatek/platsmp.c | 137 > > ++++++++++++++++++++++++++++++++++++++ 1 file changed, 137 insertions(+) > > > > diff --git a/arch/arm/mach-mediatek/platsmp.c > > b/arch/arm/mach-mediatek/platsmp.c index a5bc108..94f7865 100644 > > --- a/arch/arm/mach-mediatek/platsmp.c > > +++ b/arch/arm/mach-mediatek/platsmp.c > > @@ -21,10 +21,16 @@ > > #include <linux/of_address.h> > > #include <linux/string.h> > > #include <linux/threads.h> > > +#include <linux/delay.h> > > +#include <asm/cacheflush.h> > > + > > +#include <linux/soc/mediatek/scpsys.h> > > > > #define MTK_MAX_CPU 8 > > #define MTK_SMP_REG_SIZE 0x1000 > > > > +static DEFINE_SPINLOCK(boot_lock); > > + > > struct mtk_smp_boot_info { > > unsigned long smp_base; > > unsigned int jump_reg; > > @@ -56,6 +62,126 @@ static const struct of_device_id mtk_smp_boot_infos[] > > __initconst = { static void __iomem *mtk_smp_base; > > static const struct mtk_smp_boot_info *mtk_smp_info; > > > > +#ifdef CONFIG_HOTPLUG_CPU > > +static int mt6580_cpu_kill(unsigned cpu) > > +{ > > + int ret; > > + > > + ret = spm_cpu_mtcmos_off(cpu, 1); > > is this function ever called with wfi == 0? Yes, wfi == 0 at MT6582 "special" dual-core platform, but for MT6580, wfi is always 1. > > + if (ret < 0) > > + return 0; > > + > > + return 1; > > +} > > + > > +static void mt6580_cpu_die(unsigned int cpu) > > +{ > > + for (;;) > > + cpu_do_idle(); > > +} > > +#endif > > + > > +static void write_pen_release(int val) > > +{ > > + pen_release = val; > > + /* Make sure this is visible to other CPUs */ > > + smp_wmb(); > > + sync_cache_w(&pen_release); > > +} > > + > > +/* > > + * Refer common "pen" secondary release method > > + */ > > +static int mt6580_boot_secondary(unsigned int cpu, struct task_struct > > *idle) +{ > > + unsigned long timeout; > > + int ret; > > + > > + /* > > + * Set synchronisation state between this boot processor > > + * and the secondary one > > + */ > > + spin_lock(&boot_lock); > > + > > + /* > > + * The secondary processor is waiting to be released from > > + * the holding pen - release it, then wait for it to flag > > + * that it has been released by resetting pen_release. > > + * > > + * Note that "pen_release" is the hardware CPU ID, whereas > > + * "cpu" is Linux's internal ID. > > + */ > > + write_pen_release(cpu); > > + > > + /* > > + * CPU power on control by SPM > > + */ > > + ret = spm_cpu_mtcmos_on(cpu); > > + if (ret < 0) { > > + spin_unlock(&boot_lock); > > + return -ENXIO; > > + } > > + > > + timeout = jiffies + (1 * HZ); > > + while (time_before(jiffies, timeout)) { > > + /* Read barrier */ > > + smp_rmb(); > > + > > + if (pen_release == -1) > > + break; > > + > > + udelay(10); > > + } > > + > > + /* > > + * Now the secondary core is starting up let it run its > > + * calibrations, then wait for it to finish > > + */ > > + spin_unlock(&boot_lock); > > + > > + return (pen_release != -1 ? -ENXIO : 0); > > +} > > + > > +static void mt6580_secondary_init(unsigned int cpu) > > +{ > > + /* > > + * Let the primary processor know we're out of the > > + * pen, then head off into the C entry point > > + */ > > + write_pen_release(-1); > > + > > + /* > > + * Synchronise with the boot thread. > > + */ > > + spin_lock(&boot_lock); > > + spin_unlock(&boot_lock); > > +} > > + > > +#define MT6580_INFRACFG_AO 0x10001000 > > +#define SW_ROM_PD BIT(31) > > Please put the defines on the beginning of the file. > SW_ROM_PD bit is the same on other SoCs? I wasn't able to reveal that from the > data sheets. If this is MT6580 only, the define should be renamed. > OK, fixed in next version. > > + > > +static void __init mt6580_smp_prepare_cpus(unsigned int max_cpus) > > +{ > > + static void __iomem *infracfg_ao_base; > > + > > + infracfg_ao_base = ioremap(MT6580_INFRACFG_AO, 0x1000); > > + if (!infracfg_ao_base) { > > + pr_err("%s: Unable to map I/O memory\n", __func__); > > + return; > > + } > > + > > + /* Enable bootrom power down mode */ > > + writel_relaxed(readl(infracfg_ao_base + 0x804) | SW_ROM_PD, > > + infracfg_ao_base + 0x804); > > Please use a define for the offset. > I prefer to not cascade reads and writes but to use a temporary variable. But > if this is fine with the kernel coding style (I doubt, but I'm not sure) then > this is fine. > OK, fixed in next version. Thanks, Scott > > + > > + /* Write the address of slave startup into boot address > > + register for bootrom power down mode */ > > + writel_relaxed(virt_to_phys(secondary_startup_arm), > > + infracfg_ao_base + 0x800); > > + > > + iounmap(infracfg_ao_base); > > +} > > + > > static int mtk_boot_secondary(unsigned int cpu, struct task_struct *idle) > > { > > if (!mtk_smp_base) > > @@ -142,3 +268,14 @@ static struct smp_operations mt6589_smp_ops __initdata > > = { .smp_boot_secondary = mtk_boot_secondary, > > }; > > CPU_METHOD_OF_DECLARE(mt6589_smp, "mediatek,mt6589-smp", &mt6589_smp_ops); > > + > > +static struct smp_operations mt6580_smp_ops __initdata = { > > + .smp_prepare_cpus = mt6580_smp_prepare_cpus, > > + .smp_secondary_init = mt6580_secondary_init, > > + .smp_boot_secondary = mt6580_boot_secondary, > > +#ifdef CONFIG_HOTPLUG_CPU > > + .cpu_kill = mt6580_cpu_kill, > > + .cpu_die = mt6580_cpu_die, > > +#endif > > +}; > > +CPU_METHOD_OF_DECLARE(mt6580_smp, "mediatek,mt6580-smp", &mt6580_smp_ops); >
On Thu, 2015-08-06 at 08:19 +0200, Sascha Hauer wrote: > On Wed, Aug 05, 2015 at 06:47:03PM +0200, Matthias Brugger wrote: > > On Tuesday, August 04, 2015 09:54:21 PM Scott Shu wrote: > > > Add support for cpu enable-method "mediatek,mt6580-smp" for booting > > > secondary CPUs on MT6580. > > > > > > Signed-off-by: Scott Shu <scott.shu@mediatek.com> > > > --- > > > arch/arm/mach-mediatek/platsmp.c | 137 > > > ++++++++++++++++++++++++++++++++++++++ 1 file changed, 137 insertions(+) > > > + > > > +static void __init mt6580_smp_prepare_cpus(unsigned int max_cpus) > > > +{ > > > + static void __iomem *infracfg_ao_base; > > > + > > > + infracfg_ao_base = ioremap(MT6580_INFRACFG_AO, 0x1000); > > > + if (!infracfg_ao_base) { > > > + pr_err("%s: Unable to map I/O memory\n", __func__); > > > + return; > > > + } > > > + > > > + /* Enable bootrom power down mode */ > > > + writel_relaxed(readl(infracfg_ao_base + 0x804) | SW_ROM_PD, > > > + infracfg_ao_base + 0x804); > > > > Please use a define for the offset. > > I prefer to not cascade reads and writes but to use a temporary variable. But > > if this is fine with the kernel coding style (I doubt, but I'm not sure) then > > this is fine. > > For what it's worth I also prefer > > val = readl(infracfg_ao_base + 0x804); > val |= SW_ROM_PD: > writel_relaxed(val, infracfg_ao_base + 0x804); > > I find this better readable. I don't think though that there's common > agreement that this style is better. > > Sascha > Fixed in next version. Thanks. Scott
On Wed, 2015-08-05 at 10:44 +0100, Russell King - ARM Linux wrote: > On Tue, Aug 04, 2015 at 09:54:21PM +0800, Scott Shu wrote: > > Add support for cpu enable-method "mediatek,mt6580-smp" for booting > > secondary CPUs on MT6580. > > If you have CPU power domain support, and you power up and power down the > CPUs, why do you need the boot_lock and pen_release stuff? Isn't keeping > the power off on a CPU sufficient to prevent it entering the kernel? > Thanks Russell. Yes, the system works well after removing boot_lock and pen_release. Will be fixed in next version. Scott
diff --git a/arch/arm/mach-mediatek/platsmp.c b/arch/arm/mach-mediatek/platsmp.c index a5bc108..94f7865 100644 --- a/arch/arm/mach-mediatek/platsmp.c +++ b/arch/arm/mach-mediatek/platsmp.c @@ -21,10 +21,16 @@ #include <linux/of_address.h> #include <linux/string.h> #include <linux/threads.h> +#include <linux/delay.h> +#include <asm/cacheflush.h> + +#include <linux/soc/mediatek/scpsys.h> #define MTK_MAX_CPU 8 #define MTK_SMP_REG_SIZE 0x1000 +static DEFINE_SPINLOCK(boot_lock); + struct mtk_smp_boot_info { unsigned long smp_base; unsigned int jump_reg; @@ -56,6 +62,126 @@ static const struct of_device_id mtk_smp_boot_infos[] __initconst = { static void __iomem *mtk_smp_base; static const struct mtk_smp_boot_info *mtk_smp_info; +#ifdef CONFIG_HOTPLUG_CPU +static int mt6580_cpu_kill(unsigned cpu) +{ + int ret; + + ret = spm_cpu_mtcmos_off(cpu, 1); + if (ret < 0) + return 0; + + return 1; +} + +static void mt6580_cpu_die(unsigned int cpu) +{ + for (;;) + cpu_do_idle(); +} +#endif + +static void write_pen_release(int val) +{ + pen_release = val; + /* Make sure this is visible to other CPUs */ + smp_wmb(); + sync_cache_w(&pen_release); +} + +/* + * Refer common "pen" secondary release method + */ +static int mt6580_boot_secondary(unsigned int cpu, struct task_struct *idle) +{ + unsigned long timeout; + int ret; + + /* + * Set synchronisation state between this boot processor + * and the secondary one + */ + spin_lock(&boot_lock); + + /* + * The secondary processor is waiting to be released from + * the holding pen - release it, then wait for it to flag + * that it has been released by resetting pen_release. + * + * Note that "pen_release" is the hardware CPU ID, whereas + * "cpu" is Linux's internal ID. + */ + write_pen_release(cpu); + + /* + * CPU power on control by SPM + */ + ret = spm_cpu_mtcmos_on(cpu); + if (ret < 0) { + spin_unlock(&boot_lock); + return -ENXIO; + } + + timeout = jiffies + (1 * HZ); + while (time_before(jiffies, timeout)) { + /* Read barrier */ + smp_rmb(); + + if (pen_release == -1) + break; + + udelay(10); + } + + /* + * Now the secondary core is starting up let it run its + * calibrations, then wait for it to finish + */ + spin_unlock(&boot_lock); + + return (pen_release != -1 ? -ENXIO : 0); +} + +static void mt6580_secondary_init(unsigned int cpu) +{ + /* + * Let the primary processor know we're out of the + * pen, then head off into the C entry point + */ + write_pen_release(-1); + + /* + * Synchronise with the boot thread. + */ + spin_lock(&boot_lock); + spin_unlock(&boot_lock); +} + +#define MT6580_INFRACFG_AO 0x10001000 +#define SW_ROM_PD BIT(31) + +static void __init mt6580_smp_prepare_cpus(unsigned int max_cpus) +{ + static void __iomem *infracfg_ao_base; + + infracfg_ao_base = ioremap(MT6580_INFRACFG_AO, 0x1000); + if (!infracfg_ao_base) { + pr_err("%s: Unable to map I/O memory\n", __func__); + return; + } + + /* Enable bootrom power down mode */ + writel_relaxed(readl(infracfg_ao_base + 0x804) | SW_ROM_PD, + infracfg_ao_base + 0x804); + + /* Write the address of slave startup into boot address + register for bootrom power down mode */ + writel_relaxed(virt_to_phys(secondary_startup_arm), + infracfg_ao_base + 0x800); + + iounmap(infracfg_ao_base); +} + static int mtk_boot_secondary(unsigned int cpu, struct task_struct *idle) { if (!mtk_smp_base) @@ -142,3 +268,14 @@ static struct smp_operations mt6589_smp_ops __initdata = { .smp_boot_secondary = mtk_boot_secondary, }; CPU_METHOD_OF_DECLARE(mt6589_smp, "mediatek,mt6589-smp", &mt6589_smp_ops); + +static struct smp_operations mt6580_smp_ops __initdata = { + .smp_prepare_cpus = mt6580_smp_prepare_cpus, + .smp_secondary_init = mt6580_secondary_init, + .smp_boot_secondary = mt6580_boot_secondary, +#ifdef CONFIG_HOTPLUG_CPU + .cpu_kill = mt6580_cpu_kill, + .cpu_die = mt6580_cpu_die, +#endif +}; +CPU_METHOD_OF_DECLARE(mt6580_smp, "mediatek,mt6580-smp", &mt6580_smp_ops);
Add support for cpu enable-method "mediatek,mt6580-smp" for booting secondary CPUs on MT6580. Signed-off-by: Scott Shu <scott.shu@mediatek.com> --- arch/arm/mach-mediatek/platsmp.c | 137 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 137 insertions(+)