| Message ID | 5515BEA5.6040307@free.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 27, 2015 at 09:33:41PM +0100, Mason wrote: > Heya, > > On 27/03/2015 17:35, Marc Zyngier wrote: > > > Hi Mason, > > > > On 27/03/15 16:16, Mason wrote: > > > >> In arch/arm/kernel/smp_twd.c, twd_local_timer_register() receives a > >> struct twd_local_timer argument, which specifies > >> > >> 1) the physical address of twd_base > >> 2) the twd interrupt number > >> > >> There's a helper to fill out the static struct: DEFINE_TWD_LOCAL_TIMER() > >> > >> But it seems to me (please correct me if I'm wrong) that the address > >> of twd_base can be read at run-time, making it one less parameter to > >> specify by hand at compile-time (with the risk that a HW engineer > >> change the address in the next chip with no warning). > > > > The main problem with that is that said HW engineer has ^%$%^%-up > > PERIPHBASE, and that you can't trust it. I've seen a number of these > > kicking around. > > I don't understand your remark, perhaps mine was unclear? Mark's reply seems clear to me. > The value of PERIPHBASE is left to the implementer, right? > So the HW engineer picks addrA for revA, then changes his > mind for revB, and picks addrB. No screw-up there, right? > > The problem comes when said engineer forgets to notify the > grunts in software that they're supposed to change > #define PERIPH_BASE ADDR_A > to > #define PERIPH_BASE ADDR_B > in the header files for revB. > > Looking up PERIPH_BASE at run-time through cp15 solves that > particular issue. Do you disagree? That's one scenario. Here's the scenario Mark is describing - one which has real-world examples: Hardware engineer picks address A for rev A and sets CP15 to address A. Everything works. Hardware engineer then picks address B for rev B, but forgets to update CP15. It breaks. If it's in DT, it can be fixed. It should be there anyway as part of the hardware description. DT is a description of the hardware.
On 27/03/2015 21:53, Russell King - ARM Linux wrote: > That's one scenario. Here's the scenario Mark is describing - one which > has real-world examples: > > Hardware engineer picks address A for rev A and sets CP15 to address A. > Everything works. Hardware engineer then picks address B for rev B, but > forgets to update CP15. It breaks. The hardware engineer told me that whatever arbitrary value is chosen for PERIPH_BASE is automatically exported through CP15 (which is how I thought this worked). So there is no "forgetting to update CP15" (for this platform, at least). > If it's in DT, it can be fixed. It should be there anyway as part of > the hardware description. DT is a description of the hardware. I thought DT was supposed to describe hardware that /cannot/ be probed or discovered at run-time? If everything could be probed at run-time, what would be the point of DT? (For my own reference, DT on x86) http://thread.gmane.org/gmane.linux.kernel/1104014 http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/log/arch/x86/kernel/devicetree.c Regards.
On Wed, Apr 01, 2015 at 02:07:05PM +0200, Mason wrote: > On 27/03/2015 21:53, Russell King - ARM Linux wrote: > > >That's one scenario. Here's the scenario Mark is describing - one which > >has real-world examples: > > > >Hardware engineer picks address A for rev A and sets CP15 to address A. > >Everything works. Hardware engineer then picks address B for rev B, but > >forgets to update CP15. It breaks. > > The hardware engineer told me that whatever arbitrary value is chosen > for PERIPH_BASE is automatically exported through CP15 (which is how > I thought this worked). So there is no "forgetting to update CP15" > (for this platform, at least). I'm sorry, it's not that I don't believe you, it's that ARM Ltd employees already have evidence to the contary, and they should know what's possible, they (as a company) designed the hardware and they're the ones who have to deal with queries from _all_ the silicon vendors. They get to know what the entire ARM ecosystem is doing, what vendors get wrong, etc. They're in a far better position than just one silicon vendor to know what's possible and what isn't. So when Mark says something has been seen, I believe him, and that trumps what hardware engineers at individual silicon vendors claim. > >If it's in DT, it can be fixed. It should be there anyway as part of > >the hardware description. DT is a description of the hardware. > > I thought DT was supposed to describe hardware that /cannot/ be probed > or discovered at run-time? And what about the cases where it is possible to probe the hardware on some platforms but doing so crashes the kernel on others? I guess you don't care about anything but your own platform - that's the kind of message you're putting out...
On 01/04/15 13:07, Mason wrote: > On 27/03/2015 21:53, Russell King - ARM Linux wrote: > >> That's one scenario. Here's the scenario Mark is describing - one which >> has real-world examples: >> >> Hardware engineer picks address A for rev A and sets CP15 to address A. >> Everything works. Hardware engineer then picks address B for rev B, but >> forgets to update CP15. It breaks. > > The hardware engineer told me that whatever arbitrary value is chosen > for PERIPH_BASE is automatically exported through CP15 (which is how > I thought this worked). So there is no "forgetting to update CP15" > (for this platform, at least). Go tell that to the TI guys who created this awesome derivative of the OMAP4, which reports PERIPH_BASE as 0, despite the peripherals being mapped somewhere else. What you describe is what happens when someone properly reconfigures their RTL by running the whole integration thing, which HW guys are eager to skip. What they often do is to cut and paste an existing design, and see how many screws are left after doing so. The cupboard in front of me is full of system presenting similar issues. As much as I love HW engineers, it is a well known fact that they will lie to you most of the time! ;-) It is worth mentioning that PERIPH_BASE is *not* an architected register, so an implementation is perfectly allowed not to implement it. Even on Cortex A9, a UP implementation will report PERIPH_BASE as zero. It is still likely to have a TWD though. >> If it's in DT, it can be fixed. It should be there anyway as part of >> the hardware description. DT is a description of the hardware. > > I thought DT was supposed to describe hardware that /cannot/ be probed > or discovered at run-time? Indeed. And when probing is so unreliable, it is not worth using it. Thanks, M.
On 01/04/2015 14:12, Russell King - ARM Linux wrote: > On Wed, Apr 01, 2015 at 02:07:05PM +0200, Mason wrote: > > I'm sorry, it's not that I don't believe you, it's that ARM Ltd > employees already have evidence to the contrary, and they should know > what's possible, they (as a company) designed the hardware and they're > the ones who have to deal with queries from _all_ the silicon vendors. > They get to know what the entire ARM ecosystem is doing, what vendors > get wrong, etc. They're in a far better position than just one silicon > vendor to know what's possible and what isn't. (Disclaimer: I am not a HW engineer.) It seems the above reasoning is based on a false dichotomy. I assume that the "wiring" of PERIPH_BASE to CP15 is left up to the implementor. Maybe some implementors just copy a value, while others wire the actual value? > So when Mark says something has been seen, I believe him, and that > trumps what hardware engineers at individual silicon vendors claim. o_O ?? I have never disputed Marc's evidence. > And what about the cases where it is possible to probe the hardware on > some platforms but doing so crashes the kernel on others? I guess you > don't care about anything but your own platform - that's the kind of > message you're putting out... Why would I post patches here if all I cared about was my own platform? Anyway, I do feel a lot of animosity, and I apologize if some of my messages have offended you or others here. Perhaps if you could point out the offending comments, I could adjust in the future? Regards.
On 01/04/2015 14:28, Marc Zyngier wrote: > It is worth mentioning that PERIPH_BASE is *not* an architected > register, so an implementation is perfectly allowed not to implement it. > Even on Cortex A9, a UP implementation will report PERIPH_BASE as zero. > It is still likely to have a TWD though. I understand now that the kernel cannot reliably discover PERIPH_BASE at run-time; thus this piece of info must be communicated through DT. > Indeed. And when probing is so unreliable, it is not worth using it. Regards.
On 01/04/2015 14:28, Marc Zyngier wrote: > It is worth mentioning that PERIPH_BASE is *not* an architected > register, so an implementation is perfectly allowed not to implement it. > Even on Cortex A9, a UP implementation will report PERIPH_BASE as zero. > It is still likely to have a TWD though. It is interesting that you would mention TWD and UP implementations, because "config HAVE_ARM_TWD" depends on SMP, as I mentioned in a separate thread ("Dropping "depends on SMP" for HAVE_ARM_TWD"). Would it make sense to drop the dependency? I have a single-core Cortex A9 MPcore system where I want to use the local timers. If it's too much trouble changing the build options, I suppose I can just run an SMP kernel? Regards.
On 01/04/15 14:01, Mason wrote: > On 01/04/2015 14:28, Marc Zyngier wrote: > >> It is worth mentioning that PERIPH_BASE is *not* an architected >> register, so an implementation is perfectly allowed not to implement it. >> Even on Cortex A9, a UP implementation will report PERIPH_BASE as zero. >> It is still likely to have a TWD though. > > It is interesting that you would mention TWD and UP implementations, > because "config HAVE_ARM_TWD" depends on SMP, as I mentioned in a > separate thread ("Dropping "depends on SMP" for HAVE_ARM_TWD"). > > Would it make sense to drop the dependency? > > I have a single-core Cortex A9 MPcore system where I want to use > the local timers. If it's too much trouble changing the build > options, I suppose I can just run an SMP kernel? There used to be a time where the TWD was completely tied to the SMP code, but I think we now deal with per-cpu timers in a way similar to the global timers (more or less...). Worth trying, and see what breaks. On the other hand, SMP on UP should give you the same result. M.
On 01/04/2015 15:14, Marc Zyngier wrote: > On 01/04/15 14:01, Mason wrote: >> On 01/04/2015 14:28, Marc Zyngier wrote: >> >>> It is worth mentioning that PERIPH_BASE is *not* an architected >>> register, so an implementation is perfectly allowed not to implement it. >>> Even on Cortex A9, a UP implementation will report PERIPH_BASE as zero. >>> It is still likely to have a TWD though. >> >> It is interesting that you would mention TWD and UP implementations, >> because "config HAVE_ARM_TWD" depends on SMP, as I mentioned in a >> separate thread ("Dropping "depends on SMP" for HAVE_ARM_TWD"). >> >> Would it make sense to drop the dependency? >> >> I have a single-core Cortex A9 MPcore system where I want to use >> the local timers. If it's too much trouble changing the build >> options, I suppose I can just run an SMP kernel? > > There used to be a time where the TWD was completely tied to the SMP > code, but I think we now deal with per-cpu timers in a way similar to > the global timers (more or less...). > > Worth trying, and see what breaks. On the other hand, SMP on UP should > give you the same result. And now that you mention SMP_ON_UP and TI, I remembered just where I got the crazy idea to get PERIPH_BASE from CP15 ;-) commit bc41b8724 ("Update SMP_ON_UP code to detect A9MPCore with 1 CPU devices") arch/arm/kernel/head.S | 21 ++++++++++++++++++++- @ If a future SoC *does* use 0x0 as the PERIPH_BASE, then the @ below address check will need to be #ifdef'd or equivalent @ for the Aegis platform. mrc p15, 4, r0, c15, c0 @ get SCU base address teq r0, #0x0 @ '0' on actual UP A9 hardware beq __fixup_smp_on_up @ So its an A9 UP Isn't this as problematic here as doing it later in the boot. Is the DT data already available in head.S? Regards.
On 01/04/15 15:39, Mason wrote: > On 01/04/2015 15:14, Marc Zyngier wrote: >> On 01/04/15 14:01, Mason wrote: >>> On 01/04/2015 14:28, Marc Zyngier wrote: >>> >>>> It is worth mentioning that PERIPH_BASE is *not* an architected >>>> register, so an implementation is perfectly allowed not to implement it. >>>> Even on Cortex A9, a UP implementation will report PERIPH_BASE as zero. >>>> It is still likely to have a TWD though. >>> >>> It is interesting that you would mention TWD and UP implementations, >>> because "config HAVE_ARM_TWD" depends on SMP, as I mentioned in a >>> separate thread ("Dropping "depends on SMP" for HAVE_ARM_TWD"). >>> >>> Would it make sense to drop the dependency? >>> >>> I have a single-core Cortex A9 MPcore system where I want to use >>> the local timers. If it's too much trouble changing the build >>> options, I suppose I can just run an SMP kernel? >> >> There used to be a time where the TWD was completely tied to the SMP >> code, but I think we now deal with per-cpu timers in a way similar to >> the global timers (more or less...). >> >> Worth trying, and see what breaks. On the other hand, SMP on UP should >> give you the same result. > > And now that you mention SMP_ON_UP and TI, I remembered just where > I got the crazy idea to get PERIPH_BASE from CP15 ;-) > > commit bc41b8724 ("Update SMP_ON_UP code to detect A9MPCore with > 1 CPU devices") > > arch/arm/kernel/head.S | 21 ++++++++++++++++++++- > > @ If a future SoC *does* use 0x0 as the PERIPH_BASE, then the > @ below address check will need to be #ifdef'd or equivalent > @ for the Aegis platform. > mrc p15, 4, r0, c15, c0 @ get SCU base address > teq r0, #0x0 @ '0' on actual UP A9 hardware > beq __fixup_smp_on_up @ So its an A9 UP > > > Isn't this as problematic here as doing it later in the boot. The problem is that this is the only known way to detect this horrible piece of junk. Otherwise, we're going to treat it as a proper SMP platform, and things will break (the HW advertises itself as MPx1, but has no SCU). > Is the DT data already available in head.S? It is available in the sense that it is in memory, but at that stage, we're not in a position to use it. We're at a point where we're barely finding out what sort of CPU we have. DT will only be really available much later in the boot process. M.
diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c index 172c6a05..94fb0cd 100644 --- a/arch/arm/kernel/smp_twd.c +++ b/arch/arm/kernel/smp_twd.c @@ -391,13 +391,13 @@ static void __init twd_local_timer_of_register(struct device_node *np) if (!is_smp() || !setup_max_cpus) return; - twd_ppi = irq_of_parse_and_map(np, 0); + twd_ppi = 29; if (!twd_ppi) { err = -EINVAL; goto out; } - twd_base = of_iomap(np, 0); + twd_base = ioremap(scu_a9_get_base() + 0x600, 0x10); if (!twd_base) { err = -ENOMEM; goto out;