Reading twd_base at run-time

Reading twd_base at run-time

[Mason]

Hello everyone,

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).

Here's an incomplete patch to express my intent:

diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index 6591e26..5177db8 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -369,14 +369,14 @@ out_free:
         return err;
  }
  
-int __init twd_local_timer_register(struct twd_local_timer *tlt)
+int __init twd_local_timer_register(void)
  {
         if (twd_base || twd_evt)
                 return -EBUSY;
  
-       twd_ppi = tlt->res[1].start;
+       twd_ppi = 29;
  
-       twd_base = ioremap(tlt->res[0].start, resource_size(&tlt->res[0]));
+       twd_base = ioremap(scu_a9_get_base() + 0x600, 0x10);
         if (!twd_base)
                 return -ENOMEM;
  

As far as I can tell, all platforms use 29 for twd_ppi, but I can make
sure if people agree this patch is indeed an improvement.

Regards.

Reading twd_base at run-time

[Marc Zyngier]

Hi Mason,

On 27/03/15 16:16, Mason wrote:
> Hello everyone,
> 
> 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.

> Here's an incomplete patch to express my intent:
> 
> diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
> index 6591e26..5177db8 100644
> --- a/arch/arm/kernel/smp_twd.c
> +++ b/arch/arm/kernel/smp_twd.c
> @@ -369,14 +369,14 @@ out_free:
>          return err;
>   }
>   
> -int __init twd_local_timer_register(struct twd_local_timer *tlt)
> +int __init twd_local_timer_register(void)
>   {
>          if (twd_base || twd_evt)
>                  return -EBUSY;
>   
> -       twd_ppi = tlt->res[1].start;
> +       twd_ppi = 29;
>   
> -       twd_base = ioremap(tlt->res[0].start, resource_size(&tlt->res[0]));
> +       twd_base = ioremap(scu_a9_get_base() + 0x600, 0x10);
>          if (!twd_base)
>                  return -ENOMEM;
>   
> 
> As far as I can tell, all platforms use 29 for twd_ppi, but I can make
> sure if people agree this patch is indeed an improvement.

I don't think we'll see that many new platforms not using DT, so this
would effectively remove one line from the helper, and generate a bit of
churn on platforms that are usually left in a dusty corner...

	M.
-- 
Jazz is not dead. It just smells funny...

Reading twd_base at run-time

[Mason]

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?

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?

>> Here's an incomplete patch to express my intent:
>>
>> diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
>> index 6591e26..5177db8 100644
>> --- a/arch/arm/kernel/smp_twd.c
>> +++ b/arch/arm/kernel/smp_twd.c
>> @@ -369,14 +369,14 @@ out_free:
>>          return err;
>>   }
>>   
>> -int __init twd_local_timer_register(struct twd_local_timer *tlt)
>> +int __init twd_local_timer_register(void)
>>   {
>>          if (twd_base || twd_evt)
>>                  return -EBUSY;
>>   
>> -       twd_ppi = tlt->res[1].start;
>> +       twd_ppi = 29;
>>   
>> -       twd_base = ioremap(tlt->res[0].start, resource_size(&tlt->res[0]));
>> +       twd_base = ioremap(scu_a9_get_base() + 0x600, 0x10);
>>          if (!twd_base)
>>                  return -ENOMEM;
>>   
>>
>> As far as I can tell, all platforms use 29 for twd_ppi, but I can make
>> sure if people agree this patch is indeed an improvement.
> 
> I don't think we'll see that many new platforms not using DT, so this
> would effectively remove one line from the helper, and generate a bit of
> churn on platforms that are usually left in a dusty corner...

OK, I understand your point about what is now considered
"legacy" code. I suppose it might even be removed in the
not-too-distant future?

That being said, the principle stands for the of function!

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;


By the way, have you seen my other thread?
("Dropping "depends on SMP" for HAVE_ARM_TWD")
I CCed you as the author of twd_local_timer_of_register.

Regards.

Reading twd_base at run-time

[Russell King - ARM Linux]

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.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Reading twd_base at run-time

[Mason]

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.

Reading twd_base at run-time

[Russell King - ARM Linux]

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...

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Reading twd_base at run-time

[Mason]

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.

Reading twd_base at run-time

[Marc Zyngier]

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.
-- 
Jazz is not dead. It just smells funny...

Reading twd_base at run-time

[Mason]

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.

Reading twd_base at run-time

[Mason]

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.

Reading twd_base at run-time

[Marc Zyngier]

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.
-- 
Jazz is not dead. It just smells funny...

Reading twd_base at run-time

[Mason]

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.

Reading twd_base at run-time

[Marc Zyngier]

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.
-- 
Jazz is not dead. It just smells funny...