| Message ID | 6359949.bhCrxaQvmL@wuerfel (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On 11/23, Arnd Bergmann wrote: > On Monday 23 November 2015 13:32:06 Stephen Boyd wrote: > > On 11/23, Arnd Bergmann wrote: > > > On Monday 23 November 2015 12:38:47 Stephen Boyd wrote: > > > > It would be nice to drop the ARCH_MSM* configs entirely. If we > > could select the right timers from kconfig without using selects > > then we could drop them. Or we could just select both types of > > timers when building qcom platforms. > > Ok, dropping the specific Kconfig entries is actually an awesome > idea, as it completely solves the other problem as well, more on > that below. > > In that case, don't worry about listing all the models, once > we stop listing a subset of them, the confusion is already > reduced by the fact that one has to look at the .dts files > so see which models we support, and I assume there will be > additional ones coming in for at least a few more years (before > you stop caring about 32-bit MSM and compatibles). > > Regarding the timers: > HAVE_ARM_ARCH_TIMER is already user-selectable, so maybe something > like > > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig > index b251013eef0a..bad6343c34d5 100644 > --- a/drivers/clocksource/Kconfig > +++ b/drivers/clocksource/Kconfig > @@ -324,8 +324,9 @@ config EM_TIMER_STI > such as EMEV2 from former NEC Electronics. > > config CLKSRC_QCOM > - bool "Qualcomm MSM timer" if COMPILE_TEST > + bool "Qualcomm MSM timer" if ARCH_QCOM || COMPILE_TEST > depends on ARM > + default ARCH_QCOM > select CLKSRC_OF > help > This enables the clocksource and the per CPU clockevent driver for the > > would make both of them equally configurable and not clutter up > the Kconfig file when ARCH_QCOM is not selected. I've added > Daniel Lezcano to Cc, he probably has an opinion on this too. Yeah I think that architected timers are an outlier. I recall some words from John Stultz that platforms should select the clocksources they use, but maybe things have changed. For this kind of thing I wouldn't mind putting it in the defconfig though. I'll put the patches on the list to get the discussion started. > > > > > > The ones we do support are MSM8x60 (Scorpion), MSM8960 > > > > > (Krait-without-number),and MSM7874 (Krait 400). Do those all > > > > > support IDIV but not LPAE? > > > > > > > > > > > > > Krait supports IDIV for all versions. Scorpion doesn't support > > > > IDIV or lpae. Here's the output of /proc/cpuinfo on that device. > > > > > > > > # cat /proc/cpuinfo > > > > processor : 0 > > > > model name : ARMv7 Processor rev 2 (v7l) > > > > BogoMIPS : 13.50 > > > > Features : half thumb fastmult vfp edsp neon vfpv3 tls vfpd32 > > > > CPU implementer : 0x51 > > > > CPU architecture: 7 > > > > CPU variant : 0x0 > > > > CPU part : 0x02d > > > > CPU revision : 2 > > > > > > Ok, that leaves just one missing puzzle piece: can you confirm that > > > no supported Krait variant other than Krait 450 / apq8084 has LPAE? > > > > > > > Right, apq8084 is the only SoC with a Krait CPU that supports > > LPAE. > > Ok, thanks for the confirmation. > > Summarizing what we've found, I think we can get away with just > introducing two Kconfig symbols ARCH_MULTI_V7VE and CPU_V7VE. > Most CPUs fall clearly into one category or the other, and then > we can allow LPAE to be selected for V7VE-only build but not > for plain V7, and we can unconditionally build the kernel with > > arch-$(CONFIG_CPU_32v7VE) = -D__LINUX_ARM_ARCH__=7 $(call cc-option,-march=armv7ve,-march=armv7-a -mcpu=cortex-a15) > > This works perfectly for Cortex-A5, -A8, -A9, -A12, -A15, -A17, Brahma-B15, > PJ4B-MP, Scorpion and Krait-450, which all clearly fall into one of > the two other categories. > > The two exceptions that don't quite fit are still "good enough": > > - PJ4/PJ4B (not PJ4B-MP) has a different custom opcode for udiv and sdiv > in ARM mode. We don't support that with true multiplatform kernels > because those opcodes work nowhere else, though with your proposed > series we could easily do that for dynamic patching. Do you have the information on these custom opcodes? I can work that into the patches assuming the MIDR is different. > > - Krait (pre-450) won't run kernels with LPAE disabled, but if we only > have one global ARCH_QCOM option that can be enabled for both > ARCH_MULTI_V7VE and ARCH_MULTI_V7, we still win: a mach-qcom > kernel with only ARCH_MULTI_V7VE will use IDIV by default, and > give you the option to enable LPAE. If you pick LPAE, it will > still work fine on Krait-450 but not the older ones, and that is > a user error. If you enable ARCH_MULTI_V7 / CPU_V7, you get neither > LPAE nor IDIV, and the kernel will be able to run on both Scorpion > and Krait, as long as you have the right drivers too. > So if I have built mach-qcom with ARCH_MULTI_V7VE won't I get a kernel that uses idiv instructions that could be run on Scorpion, where the instruction doesn't exist? Or is that a user error again like picking LPAE? It seems fine to me to go ahead with this approach. Should I take care of cooking up the patches? I can package this all up into a series that adds the new CPU type, updates the affected platforms, and layers the runtime patching on top when plain V7 is a selected CPU type.
On 11/23, Arnd Bergmann wrote: > > Ok, thanks for the confirmation. > > Summarizing what we've found, I think we can get away with just > introducing two Kconfig symbols ARCH_MULTI_V7VE and CPU_V7VE. > Most CPUs fall clearly into one category or the other, and then > we can allow LPAE to be selected for V7VE-only build but not > for plain V7, and we can unconditionally build the kernel with > > arch-$(CONFIG_CPU_32v7VE) = -D__LINUX_ARM_ARCH__=7 $(call cc-option,-march=armv7ve,-march=armv7-a -mcpu=cortex-a15) > This causes compiler spew for me: warning: switch -mcpu=cortex-a15 conflicts with -march=armv7-a switch Removing -march=armv7-a from there makes it quiet. Also, it's sort of feels wrong to have -mcpu in a place where we're exclusively doing -march. Perhaps the fallback should be bog standard -march=armv7-a? (or the fallback for that one "-march=armv5t -Wa$(comma)-march=armv7-a")?
On Monday 23 November 2015 15:13:52 Stephen Boyd wrote: > On 11/23, Arnd Bergmann wrote: > > On Monday 23 November 2015 13:32:06 Stephen Boyd wrote: > > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig > > index b251013eef0a..bad6343c34d5 100644 > > --- a/drivers/clocksource/Kconfig > > +++ b/drivers/clocksource/Kconfig > > @@ -324,8 +324,9 @@ config EM_TIMER_STI > > such as EMEV2 from former NEC Electronics. > > > > config CLKSRC_QCOM > > - bool "Qualcomm MSM timer" if COMPILE_TEST > > + bool "Qualcomm MSM timer" if ARCH_QCOM || COMPILE_TEST > > depends on ARM > > + default ARCH_QCOM > > select CLKSRC_OF > > help > > This enables the clocksource and the per CPU clockevent driver for the > > > > would make both of them equally configurable and not clutter up > > the Kconfig file when ARCH_QCOM is not selected. I've added > > Daniel Lezcano to Cc, he probably has an opinion on this too. > > Yeah I think that architected timers are an outlier. I recall > some words from John Stultz that platforms should select the > clocksources they use, but maybe things have changed. For this > kind of thing I wouldn't mind putting it in the defconfig though. > I'll put the patches on the list to get the discussion started. Ok, thanks! > > This works perfectly for Cortex-A5, -A8, -A9, -A12, -A15, -A17, Brahma-B15, > > PJ4B-MP, Scorpion and Krait-450, which all clearly fall into one of > > the two other categories. > > > > The two exceptions that don't quite fit are still "good enough": > > > > - PJ4/PJ4B (not PJ4B-MP) has a different custom opcode for udiv and sdiv > > in ARM mode. We don't support that with true multiplatform kernels > > because those opcodes work nowhere else, though with your proposed > > series we could easily do that for dynamic patching. > > Do you have the information on these custom opcodes? I can work > that into the patches assuming the MIDR is different. Thomas Petazzoni said this in a private mail: | According to the datasheet, the PJ4B has integer signed and unsigned | divide, similar to the sdiv and udiv ARM instructions. But the way to | access it is by doing a MRC instruction. | | MRC<cond> p6, 1, Rd , CRn , CRm, 4 | |for PJ4B is the same as: | | SDIV Rd , Rn, Rm | | on ARM cores. | |And: | | MRC<cond> p6, 1, Rd , CRn , CRm, 0 | |for PJ4B is the same as: | | UDIV Rd , Rn, Rm | |on ARM cores. | |This is documented in the "Extended instructions" section of the |PJ4B datasheet. I assume what he meant was that this is true for both PJ4 and PJ4B but not for PJ4B-MP, which has the normal udiv/sdiv instructions. IOW, anything with CPU implementer 0x56 part 0x581 should use those, while part 0x584 can use the sdiv/udiv that it reports correctly. > > - Krait (pre-450) won't run kernels with LPAE disabled, but if we only > > have one global ARCH_QCOM option that can be enabled for both > > ARCH_MULTI_V7VE and ARCH_MULTI_V7, we still win: a mach-qcom > > kernel with only ARCH_MULTI_V7VE will use IDIV by default, and > > give you the option to enable LPAE. If you pick LPAE, it will > > still work fine on Krait-450 but not the older ones, and that is > > a user error. If you enable ARCH_MULTI_V7 / CPU_V7, you get neither > > LPAE nor IDIV, and the kernel will be able to run on both Scorpion > > and Krait, as long as you have the right drivers too. > > > > So if I have built mach-qcom with ARCH_MULTI_V7VE won't I get a > kernel that uses idiv instructions that could be run on Scorpion, > where the instruction doesn't exist? Or is that a user error > again like picking LPAE? Right. If you want to run on Scorpion, you have to select ARCH_MULTI_V7. If both are set, we should build with -march=armv7-a and not use the idiv instructions. > It seems fine to me to go ahead with this approach. Should I take > care of cooking up the patches? I can package this all up into a > series that adds the new CPU type, updates the affected > platforms, and layers the runtime patching on top when plain V7 > is a selected CPU type. That would be nice, yes. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 23, 2015 at 04:13:06PM -0800, Stephen Boyd wrote: > On 11/23, Arnd Bergmann wrote: > > > > Ok, thanks for the confirmation. > > > > Summarizing what we've found, I think we can get away with just > > introducing two Kconfig symbols ARCH_MULTI_V7VE and CPU_V7VE. > > Most CPUs fall clearly into one category or the other, and then > > we can allow LPAE to be selected for V7VE-only build but not > > for plain V7, and we can unconditionally build the kernel with > > > > arch-$(CONFIG_CPU_32v7VE) = -D__LINUX_ARM_ARCH__=7 $(call cc-option,-march=armv7ve,-march=armv7-a -mcpu=cortex-a15) > > > > This causes compiler spew for me: > > warning: switch -mcpu=cortex-a15 conflicts with -march=armv7-a switch > > Removing -march=armv7-a from there makes it quiet. > > Also, it's sort of feels wrong to have -mcpu in a place where > we're exclusively doing -march. Perhaps the fallback should be > bog standard -march=armv7-a? (or the fallback for that one > "-march=armv5t -Wa$(comma)-march=armv7-a")? How it was explained to me years ago is that -march selects the instruction set, -mtune selects the instruction scheduling, and -mcpu selects both.
Arnd Bergmann <arnd@arndb.de> writes: > On Monday 23 November 2015 15:13:52 Stephen Boyd wrote: >> On 11/23, Arnd Bergmann wrote: >> > On Monday 23 November 2015 13:32:06 Stephen Boyd wrote: >> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig >> > index b251013eef0a..bad6343c34d5 100644 >> > --- a/drivers/clocksource/Kconfig >> > +++ b/drivers/clocksource/Kconfig >> > @@ -324,8 +324,9 @@ config EM_TIMER_STI >> > such as EMEV2 from former NEC Electronics. >> > >> > config CLKSRC_QCOM >> > - bool "Qualcomm MSM timer" if COMPILE_TEST >> > + bool "Qualcomm MSM timer" if ARCH_QCOM || COMPILE_TEST >> > depends on ARM >> > + default ARCH_QCOM >> > select CLKSRC_OF >> > help >> > This enables the clocksource and the per CPU clockevent driver for the >> > >> > would make both of them equally configurable and not clutter up >> > the Kconfig file when ARCH_QCOM is not selected. I've added >> > Daniel Lezcano to Cc, he probably has an opinion on this too. >> >> Yeah I think that architected timers are an outlier. I recall >> some words from John Stultz that platforms should select the >> clocksources they use, but maybe things have changed. For this >> kind of thing I wouldn't mind putting it in the defconfig though. >> I'll put the patches on the list to get the discussion started. > > Ok, thanks! > >> > This works perfectly for Cortex-A5, -A8, -A9, -A12, -A15, -A17, Brahma-B15, >> > PJ4B-MP, Scorpion and Krait-450, which all clearly fall into one of >> > the two other categories. >> > >> > The two exceptions that don't quite fit are still "good enough": >> > >> > - PJ4/PJ4B (not PJ4B-MP) has a different custom opcode for udiv and sdiv >> > in ARM mode. We don't support that with true multiplatform kernels >> > because those opcodes work nowhere else, though with your proposed >> > series we could easily do that for dynamic patching. >> >> Do you have the information on these custom opcodes? I can work >> that into the patches assuming the MIDR is different. > > Thomas Petazzoni said this in a private mail: > > | According to the datasheet, the PJ4B has integer signed and unsigned > | divide, similar to the sdiv and udiv ARM instructions. But the way to > | access it is by doing a MRC instruction. > | > | MRC<cond> p6, 1, Rd , CRn , CRm, 4 > | > |for PJ4B is the same as: > | > | SDIV Rd , Rn, Rm > | > | on ARM cores. > | > |And: > | > | MRC<cond> p6, 1, Rd , CRn , CRm, 0 > | > |for PJ4B is the same as: > | > | UDIV Rd , Rn, Rm > | > |on ARM cores. > | > |This is documented in the "Extended instructions" section of the > |PJ4B datasheet. > > I assume what he meant was that this is true for both PJ4 and PJ4B > but not for PJ4B-MP, which has the normal udiv/sdiv instructions. > > IOW, anything with CPU implementer 0x56 part 0x581 should use those, > while part 0x584 can use the sdiv/udiv that it reports correctly. Or we could simply ignore those and they'd be no worse off than they are now.
On Tuesday 24 November 2015 12:15:13 Måns Rullgård wrote: > Arnd Bergmann <arnd@arndb.de> writes: > > On Monday 23 November 2015 15:13:52 Stephen Boyd wrote: > >> On 11/23, Arnd Bergmann wrote: > >> > On Monday 23 November 2015 13:32:06 Stephen Boyd wrote: > >> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig > >> > index b251013eef0a..bad6343c34d5 100644 > >> Do you have the information on these custom opcodes? I can work > >> that into the patches assuming the MIDR is different. > > > > Thomas Petazzoni said this in a private mail: > > > > | According to the datasheet, the PJ4B has integer signed and unsigned > > | divide, similar to the sdiv and udiv ARM instructions. But the way to > > | access it is by doing a MRC instruction. > > | > > | MRC<cond> p6, 1, Rd , CRn , CRm, 4 > > | > > |for PJ4B is the same as: > > | > > | SDIV Rd , Rn, Rm > > | > > | on ARM cores. > > | > > |And: > > | > > | MRC<cond> p6, 1, Rd , CRn , CRm, 0 > > | > > |for PJ4B is the same as: > > | > > | UDIV Rd , Rn, Rm > > | > > |on ARM cores. > > | > > |This is documented in the "Extended instructions" section of the > > |PJ4B datasheet. > > > > I assume what he meant was that this is true for both PJ4 and PJ4B > > but not for PJ4B-MP, which has the normal udiv/sdiv instructions. > > > > IOW, anything with CPU implementer 0x56 part 0x581 should use those, > > while part 0x584 can use the sdiv/udiv that it reports correctly. > > Or we could simply ignore those and they'd be no worse off than they are > now. Well, if we add all the infrastructure to do dynamic patching, we might as well use it here, that is a very little extra effort. I'm not convinced that the dynamic patching for idiv is actually needed but I'm not objecting either, and Stephen has done the work already. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/24, Arnd Bergmann wrote: > On Monday 23 November 2015 15:13:52 Stephen Boyd wrote: > > On 11/23, Arnd Bergmann wrote: > > > > > > - PJ4/PJ4B (not PJ4B-MP) has a different custom opcode for udiv and sdiv > > > in ARM mode. We don't support that with true multiplatform kernels > > > because those opcodes work nowhere else, though with your proposed > > > series we could easily do that for dynamic patching. > > > > Do you have the information on these custom opcodes? I can work > > that into the patches assuming the MIDR is different. > > Thomas Petazzoni said this in a private mail: > > | According to the datasheet, the PJ4B has integer signed and unsigned > | divide, similar to the sdiv and udiv ARM instructions. But the way to > | access it is by doing a MRC instruction. > | > | MRC<cond> p6, 1, Rd , CRn , CRm, 4 > | > |for PJ4B is the same as: > | > | SDIV Rd , Rn, Rm > | > | on ARM cores. > | > |And: > | > | MRC<cond> p6, 1, Rd , CRn , CRm, 0 > | > |for PJ4B is the same as: > | > | UDIV Rd , Rn, Rm > | > |on ARM cores. > | > |This is documented in the "Extended instructions" section of the > |PJ4B datasheet. > > I assume what he meant was that this is true for both PJ4 and PJ4B > but not for PJ4B-MP, which has the normal udiv/sdiv instructions. > > IOW, anything with CPU implementer 0x56 part 0x581 should use those, > while part 0x584 can use the sdiv/udiv that it reports correctly. > It looks like we have some sort of function that mostly does this, except it doesn't differentiate on that lower bit for 1 vs 4. I guess I'll write another one for that. static inline int cpu_is_pj4(void) { unsigned int id; id = read_cpuid_id(); if ((id & 0xff0fff00) == 0x560f5800) return 1; return 0; }
On Tuesday 24 November 2015 17:51:37 Stephen Boyd wrote: > On 11/24, Arnd Bergmann wrote: > > On Monday 23 November 2015 15:13:52 Stephen Boyd wrote: > > IOW, anything with CPU implementer 0x56 part 0x581 should use those, > > while part 0x584 can use the sdiv/udiv that it reports correctly. > > > > It looks like we have some sort of function that mostly does > this, except it doesn't differentiate on that lower bit for 1 vs > 4. I guess I'll write another one for that. > > static inline int cpu_is_pj4(void) > { > unsigned int id; > > id = read_cpuid_id(); > if ((id & 0xff0fff00) == 0x560f5800) > return 1; > > return 0; > } Correct, thanks. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig index b251013eef0a..bad6343c34d5 100644 --- a/drivers/clocksource/Kconfig +++ b/drivers/clocksource/Kconfig @@ -324,8 +324,9 @@ config EM_TIMER_STI such as EMEV2 from former NEC Electronics. config CLKSRC_QCOM - bool "Qualcomm MSM timer" if COMPILE_TEST + bool "Qualcomm MSM timer" if ARCH_QCOM || COMPILE_TEST depends on ARM + default ARCH_QCOM select CLKSRC_OF help This enables the clocksource and the per CPU clockevent driver for the