diff mbox

[04/10] arm: zynq: Load scu baseaddress at run time

Message ID 1364219596-4954-4-git-send-email-michal.simek@xilinx.com (mailing list archive)
State New, archived
Headers show

Commit Message

Michal Simek March 25, 2013, 1:53 p.m. UTC
Use Cortex a9 cp15 to read scu baseaddress.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
 arch/arm/boot/dts/zynq-7000.dtsi |    5 +++++
 arch/arm/mach-zynq/common.c      |   33 +++++++++++++++++++++------------
 arch/arm/mach-zynq/common.h      |    2 ++
 3 files changed, 28 insertions(+), 12 deletions(-)

Comments

Rob Herring March 25, 2013, 2:06 p.m. UTC | #1
On 03/25/2013 08:53 AM, Michal Simek wrote:
> Use Cortex a9 cp15 to read scu baseaddress.
> 
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>  arch/arm/boot/dts/zynq-7000.dtsi |    5 +++++
>  arch/arm/mach-zynq/common.c      |   33 +++++++++++++++++++++------------
>  arch/arm/mach-zynq/common.h      |    2 ++
>  3 files changed, 28 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
> index 2a72339..4943568 100644
> --- a/arch/arm/boot/dts/zynq-7000.dtsi
> +++ b/arch/arm/boot/dts/zynq-7000.dtsi
> @@ -22,6 +22,11 @@
>  		interrupt-parent = <&intc>;
>  		ranges;
>  
> +		scu: scu@f8f000000 {
> +			compatible = "arm,cortex-a9-scu";
> +			reg = <0xf8f00000 0x58>;
> +		};
> +

It's fine to add this, but you don't really need it for this patch.

>  		intc: interrupt-controller@f8f01000 {
>  			compatible = "arm,cortex-a9-gic";
>  			#interrupt-cells = <3>;
> diff --git a/arch/arm/mach-zynq/common.c b/arch/arm/mach-zynq/common.c
> index 68e0907..13f9d8b 100644
> --- a/arch/arm/mach-zynq/common.c
> +++ b/arch/arm/mach-zynq/common.c
> @@ -33,10 +33,13 @@
>  #include <asm/mach-types.h>
>  #include <asm/page.h>
>  #include <asm/pgtable.h>
> +#include <asm/smp_scu.h>
>  #include <asm/hardware/cache-l2x0.h>
>  
>  #include "common.h"
>  
> +void __iomem *scu_base;
> +
>  static struct of_device_id zynq_of_bus_ids[] __initdata = {
>  	{ .compatible = "simple-bus", },
>  	{}
> @@ -56,17 +59,6 @@ static void __init xilinx_init_machine(void)
>  	of_platform_bus_probe(NULL, zynq_of_bus_ids, NULL);
>  }
>  
> -#define SCU_PERIPH_PHYS		0xF8F00000
> -#define SCU_PERIPH_SIZE		SZ_8K
> -#define SCU_PERIPH_VIRT		(VMALLOC_END - SCU_PERIPH_SIZE)
> -
> -static struct map_desc scu_desc __initdata = {
> -	.virtual	= SCU_PERIPH_VIRT,
> -	.pfn		= __phys_to_pfn(SCU_PERIPH_PHYS),
> -	.length		= SCU_PERIPH_SIZE,
> -	.type		= MT_DEVICE,
> -};
> -
>  static void __init xilinx_zynq_timer_init(void)
>  {
>  	struct device_node *np;
> @@ -81,13 +73,30 @@ static void __init xilinx_zynq_timer_init(void)
>  	clocksource_of_init();
>  }
>  
> +static struct map_desc zynq_cortex_a9_scu_map __initdata = {
> +	.length	= SZ_256,
> +	.type	= MT_DEVICE,
> +};
> +
> +static void __init scu_init(void)
> +{
> +	unsigned long base;
> +
> +	base = scu_a9_get_base();
> +	zynq_cortex_a9_scu_map.pfn = __phys_to_pfn(base);
> +	zynq_cortex_a9_scu_map.virtual = base;

You are setting the virtual address to the physical base?

> +	iotable_init(&zynq_cortex_a9_scu_map, 1);

Then creating a static mapping...

> +	scu_base = ioremap(base, zynq_cortex_a9_scu_map.length);

And also a dynamic mapping?

Rob
Michal Simek March 25, 2013, 2:51 p.m. UTC | #2
Hi Rob,

2013/3/25 Rob Herring <robherring2@gmail.com>:
> On 03/25/2013 08:53 AM, Michal Simek wrote:
>> Use Cortex a9 cp15 to read scu baseaddress.
>>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> ---
>>  arch/arm/boot/dts/zynq-7000.dtsi |    5 +++++
>>  arch/arm/mach-zynq/common.c      |   33 +++++++++++++++++++++------------
>>  arch/arm/mach-zynq/common.h      |    2 ++
>>  3 files changed, 28 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
>> index 2a72339..4943568 100644
>> --- a/arch/arm/boot/dts/zynq-7000.dtsi
>> +++ b/arch/arm/boot/dts/zynq-7000.dtsi
>> @@ -22,6 +22,11 @@
>>               interrupt-parent = <&intc>;
>>               ranges;
>>
>> +             scu: scu@f8f000000 {
>> +                     compatible = "arm,cortex-a9-scu";
>> +                     reg = <0xf8f00000 0x58>;
>> +             };
>> +
>
> It's fine to add this, but you don't really need it for this patch.

yes truth.

>
>>               intc: interrupt-controller@f8f01000 {
>>                       compatible = "arm,cortex-a9-gic";
>>                       #interrupt-cells = <3>;
>> diff --git a/arch/arm/mach-zynq/common.c b/arch/arm/mach-zynq/common.c
>> index 68e0907..13f9d8b 100644
>> --- a/arch/arm/mach-zynq/common.c
>> +++ b/arch/arm/mach-zynq/common.c
>> @@ -33,10 +33,13 @@
>>  #include <asm/mach-types.h>
>>  #include <asm/page.h>
>>  #include <asm/pgtable.h>
>> +#include <asm/smp_scu.h>
>>  #include <asm/hardware/cache-l2x0.h>
>>
>>  #include "common.h"
>>
>> +void __iomem *scu_base;
>> +
>>  static struct of_device_id zynq_of_bus_ids[] __initdata = {
>>       { .compatible = "simple-bus", },
>>       {}
>> @@ -56,17 +59,6 @@ static void __init xilinx_init_machine(void)
>>       of_platform_bus_probe(NULL, zynq_of_bus_ids, NULL);
>>  }
>>
>> -#define SCU_PERIPH_PHYS              0xF8F00000
>> -#define SCU_PERIPH_SIZE              SZ_8K
>> -#define SCU_PERIPH_VIRT              (VMALLOC_END - SCU_PERIPH_SIZE)
>> -
>> -static struct map_desc scu_desc __initdata = {
>> -     .virtual        = SCU_PERIPH_VIRT,
>> -     .pfn            = __phys_to_pfn(SCU_PERIPH_PHYS),
>> -     .length         = SCU_PERIPH_SIZE,
>> -     .type           = MT_DEVICE,
>> -};
>> -
>>  static void __init xilinx_zynq_timer_init(void)
>>  {
>>       struct device_node *np;
>> @@ -81,13 +73,30 @@ static void __init xilinx_zynq_timer_init(void)
>>       clocksource_of_init();
>>  }
>>
>> +static struct map_desc zynq_cortex_a9_scu_map __initdata = {
>> +     .length = SZ_256,
>> +     .type   = MT_DEVICE,
>> +};
>> +
>> +static void __init scu_init(void)
>> +{
>> +     unsigned long base;
>> +
>> +     base = scu_a9_get_base();
>> +     zynq_cortex_a9_scu_map.pfn = __phys_to_pfn(base);
>> +     zynq_cortex_a9_scu_map.virtual = base;
>
> You are setting the virtual address to the physical base?
>
>> +     iotable_init(&zynq_cortex_a9_scu_map, 1);
>
> Then creating a static mapping...
>
>> +     scu_base = ioremap(base, zynq_cortex_a9_scu_map.length);
>
> And also a dynamic mapping?

Yes - exactly.
I was talking to Olof about this code at ELC and he mentioned that someone
else might know better way how to do it.

It is quite a long time I played with this code.
I found this solution in vexpress platform (mach-vexpress/platsmp.c)

IRC: Static mapping is necessary to be able to access device so early.
On the other hand you can't use this static mapping when kernel runs
for that you need to use dynamic allocation.
Calling ioremap so early caused that the return address is the same
and you can just use it later. and it is also in the correct vmalloc area.

We are using scu_base for power management code.
Let me check if dynamic mapping is also required.

Thanks,
Michal
Rob Herring March 25, 2013, 3:37 p.m. UTC | #3
On 03/25/2013 09:51 AM, Michal Simek wrote:
> Hi Rob,
> 
> 2013/3/25 Rob Herring <robherring2@gmail.com>:
>> On 03/25/2013 08:53 AM, Michal Simek wrote:
>>> Use Cortex a9 cp15 to read scu baseaddress.

[...]

>>> +static void __init scu_init(void)
>>> +{
>>> +     unsigned long base;
>>> +
>>> +     base = scu_a9_get_base();
>>> +     zynq_cortex_a9_scu_map.pfn = __phys_to_pfn(base);
>>> +     zynq_cortex_a9_scu_map.virtual = base;
>>
>> You are setting the virtual address to the physical base?
>>
>>> +     iotable_init(&zynq_cortex_a9_scu_map, 1);
>>
>> Then creating a static mapping...
>>
>>> +     scu_base = ioremap(base, zynq_cortex_a9_scu_map.length);
>>
>> And also a dynamic mapping?
> 
> Yes - exactly.

You are simply getting lucky that it works. If the physical address did
not happen to be in the vmalloc address region, it would not work. You
should not do this because you have an implicit requirement and the code
will look broken to anyone that reads it.

> I was talking to Olof about this code at ELC and he mentioned that someone
> else might know better way how to do it.
> 
> It is quite a long time I played with this code.
> I found this solution in vexpress platform (mach-vexpress/platsmp.c)
> 
> IRC: Static mapping is necessary to be able to access device so early.

Only to read the number of cores. That's not really worth an early
mapping. I would move to using DT to count the number of cores. Support
for that is already in place.

> On the other hand you can't use this static mapping when kernel runs
> for that you need to use dynamic allocation.
> Calling ioremap so early caused that the return address is the same
> and you can just use it later. and it is also in the correct vmalloc area.
> 
> We are using scu_base for power management code.
> Let me check if dynamic mapping is also required.

The should be no reason you need both.

Rob
Michal Simek March 25, 2013, 4:07 p.m. UTC | #4
2013/3/25 Rob Herring <robherring2@gmail.com>:
> On 03/25/2013 09:51 AM, Michal Simek wrote:
>> Hi Rob,
>>
>> 2013/3/25 Rob Herring <robherring2@gmail.com>:
>>> On 03/25/2013 08:53 AM, Michal Simek wrote:
>>>> Use Cortex a9 cp15 to read scu baseaddress.
>
> [...]
>
>>>> +static void __init scu_init(void)
>>>> +{
>>>> +     unsigned long base;
>>>> +
>>>> +     base = scu_a9_get_base();
>>>> +     zynq_cortex_a9_scu_map.pfn = __phys_to_pfn(base);
>>>> +     zynq_cortex_a9_scu_map.virtual = base;
>>>
>>> You are setting the virtual address to the physical base?
>>>
>>>> +     iotable_init(&zynq_cortex_a9_scu_map, 1);
>>>
>>> Then creating a static mapping...
>>>
>>>> +     scu_base = ioremap(base, zynq_cortex_a9_scu_map.length);
>>>
>>> And also a dynamic mapping?
>>
>> Yes - exactly.
>
> You are simply getting lucky that it works. If the physical address did
> not happen to be in the vmalloc address region, it would not work. You
> should not do this because you have an implicit requirement and the code
> will look broken to anyone that reads it.

yeah correct. I will add there a comment to mentioned that.

>> I was talking to Olof about this code at ELC and he mentioned that someone
>> else might know better way how to do it.
>>
>> It is quite a long time I played with this code.
>> I found this solution in vexpress platform (mach-vexpress/platsmp.c)
>>
>> IRC: Static mapping is necessary to be able to access device so early.
>
> Only to read the number of cores. That's not really worth an early
> mapping. I would move to using DT to count the number of cores. Support
> for that is already in place.

What's the functions for that?
From my point of view is better to read it directly on SoC and not to read
dts. It should be also faster.

>> On the other hand you can't use this static mapping when kernel runs
>> for that you need to use dynamic allocation.
>> Calling ioremap so early caused that the return address is the same
>> and you can just use it later. and it is also in the correct vmalloc area.
>>
>> We are using scu_base for power management code.
>> Let me check if dynamic mapping is also required.
>
> The should be no reason you need both.

I have done some tests and that ioremap could be probably removed.
Pawel: You are the author of this code in vexpress. Is there any
particular reason
to have there that ioremap?

Thanks,
Michal
Rob Herring March 25, 2013, 10:34 p.m. UTC | #5
On 03/25/2013 11:07 AM, Michal Simek wrote:
> 2013/3/25 Rob Herring <robherring2@gmail.com>:
>> On 03/25/2013 09:51 AM, Michal Simek wrote:
>>> Hi Rob,
>>>
>>> 2013/3/25 Rob Herring <robherring2@gmail.com>:
>>>> On 03/25/2013 08:53 AM, Michal Simek wrote:
>>>>> Use Cortex a9 cp15 to read scu baseaddress.
>>
>> [...]
>>
>>>>> +static void __init scu_init(void)
>>>>> +{
>>>>> +     unsigned long base;
>>>>> +
>>>>> +     base = scu_a9_get_base();
>>>>> +     zynq_cortex_a9_scu_map.pfn = __phys_to_pfn(base);
>>>>> +     zynq_cortex_a9_scu_map.virtual = base;
>>>>
>>>> You are setting the virtual address to the physical base?
>>>>
>>>>> +     iotable_init(&zynq_cortex_a9_scu_map, 1);
>>>>
>>>> Then creating a static mapping...
>>>>
>>>>> +     scu_base = ioremap(base, zynq_cortex_a9_scu_map.length);
>>>>
>>>> And also a dynamic mapping?
>>>
>>> Yes - exactly.
>>
>> You are simply getting lucky that it works. If the physical address did
>> not happen to be in the vmalloc address region, it would not work. You
>> should not do this because you have an implicit requirement and the code
>> will look broken to anyone that reads it.
> 
> yeah correct. I will add there a comment to mentioned that.

I think leaving the define would be better, but either way is fine.

>>> I was talking to Olof about this code at ELC and he mentioned that someone
>>> else might know better way how to do it.
>>>
>>> It is quite a long time I played with this code.
>>> I found this solution in vexpress platform (mach-vexpress/platsmp.c)
>>>
>>> IRC: Static mapping is necessary to be able to access device so early.
>>
>> Only to read the number of cores. That's not really worth an early
>> mapping. I would move to using DT to count the number of cores. Support
>> for that is already in place.
> 
> What's the functions for that?
> From my point of view is better to read it directly on SoC and not to read
> dts. It should be also faster.

Actually, I think you just remove all the code related to
set_cpu_possible().

The relevant function is arm_dt_init_cpu_maps.

Rob
Michal Simek March 26, 2013, 10:45 a.m. UTC | #6
On 03/25/2013 11:34 PM, Rob Herring wrote:
> On 03/25/2013 11:07 AM, Michal Simek wrote:
>> 2013/3/25 Rob Herring <robherring2@gmail.com>:
>>> On 03/25/2013 09:51 AM, Michal Simek wrote:
>>>> Hi Rob,
>>>>
>>>> 2013/3/25 Rob Herring <robherring2@gmail.com>:
>>>>> On 03/25/2013 08:53 AM, Michal Simek wrote:
>>>>>> Use Cortex a9 cp15 to read scu baseaddress.
>>>
>>> [...]
>>>
>>>>>> +static void __init scu_init(void)
>>>>>> +{
>>>>>> +     unsigned long base;
>>>>>> +
>>>>>> +     base = scu_a9_get_base();
>>>>>> +     zynq_cortex_a9_scu_map.pfn = __phys_to_pfn(base);
>>>>>> +     zynq_cortex_a9_scu_map.virtual = base;
>>>>>
>>>>> You are setting the virtual address to the physical base?
>>>>>
>>>>>> +     iotable_init(&zynq_cortex_a9_scu_map, 1);
>>>>>
>>>>> Then creating a static mapping...
>>>>>
>>>>>> +     scu_base = ioremap(base, zynq_cortex_a9_scu_map.length);
>>>>>
>>>>> And also a dynamic mapping?
>>>>
>>>> Yes - exactly.
>>>
>>> You are simply getting lucky that it works. If the physical address did
>>> not happen to be in the vmalloc address region, it would not work. You
>>> should not do this because you have an implicit requirement and the code
>>> will look broken to anyone that reads it.
>>
>> yeah correct. I will add there a comment to mentioned that.
>
> I think leaving the define would be better, but either way is fine.

ok. From my point of view is better to keep scu where it is and any early vmalloc
allocator will be really helpful. Because there is also early map
for early console where virtual addresses are hardcoded.
It will be just easier to ask generic code to return base if if you request
for size.

static unsigned long iomapstart = VMALLOC_END;

unsigned long get_iomap_virtbase(unsigned long size)
{
	iomapstart -= size;
	return iomapstart;
}

Something like this will be helpful and then instead of setup virtual
addresses for every static io mapping just to call this function
and you will get mapping correct all the time.


>>>> I was talking to Olof about this code at ELC and he mentioned that someone
>>>> else might know better way how to do it.
>>>>
>>>> It is quite a long time I played with this code.
>>>> I found this solution in vexpress platform (mach-vexpress/platsmp.c)
>>>>
>>>> IRC: Static mapping is necessary to be able to access device so early.
>>>
>>> Only to read the number of cores. That's not really worth an early
>>> mapping. I would move to using DT to count the number of cores. Support
>>> for that is already in place.
>>
>> What's the functions for that?
>>  From my point of view is better to read it directly on SoC and not to read
>> dts. It should be also faster.
>
> Actually, I think you just remove all the code related to
> set_cpu_possible().
>
> The relevant function is arm_dt_init_cpu_maps.

Ok. Thanks.

I will look at it but currently I prefer to cound number of cores
via scu because we are using too many dtses.

Thanks,
Michal
Rob Herring March 26, 2013, 12:28 p.m. UTC | #7
On 03/26/2013 05:45 AM, Michal Simek wrote:
> On 03/25/2013 11:34 PM, Rob Herring wrote:
>> On 03/25/2013 11:07 AM, Michal Simek wrote:
>>> 2013/3/25 Rob Herring <robherring2@gmail.com>:
>>>> On 03/25/2013 09:51 AM, Michal Simek wrote:
>>>>> Hi Rob,
>>>>>
>>>>> 2013/3/25 Rob Herring <robherring2@gmail.com>:
>>>>>> On 03/25/2013 08:53 AM, Michal Simek wrote:
>>>>>>> Use Cortex a9 cp15 to read scu baseaddress.
>>>>
>>>> [...]
>>>>
>>>>>>> +static void __init scu_init(void)
>>>>>>> +{
>>>>>>> +     unsigned long base;
>>>>>>> +
>>>>>>> +     base = scu_a9_get_base();
>>>>>>> +     zynq_cortex_a9_scu_map.pfn = __phys_to_pfn(base);
>>>>>>> +     zynq_cortex_a9_scu_map.virtual = base;
>>>>>>
>>>>>> You are setting the virtual address to the physical base?
>>>>>>
>>>>>>> +     iotable_init(&zynq_cortex_a9_scu_map, 1);
>>>>>>
>>>>>> Then creating a static mapping...
>>>>>>
>>>>>>> +     scu_base = ioremap(base, zynq_cortex_a9_scu_map.length);
>>>>>>
>>>>>> And also a dynamic mapping?
>>>>>
>>>>> Yes - exactly.
>>>>
>>>> You are simply getting lucky that it works. If the physical address did
>>>> not happen to be in the vmalloc address region, it would not work. You
>>>> should not do this because you have an implicit requirement and the
>>>> code
>>>> will look broken to anyone that reads it.
>>>
>>> yeah correct. I will add there a comment to mentioned that.
>>
>> I think leaving the define would be better, but either way is fine.
> 
> ok. From my point of view is better to keep scu where it is and any
> early vmalloc
> allocator will be really helpful. Because there is also early map
> for early console where virtual addresses are hardcoded.
> It will be just easier to ask generic code to return base if if you request
> for size.
> 
> static unsigned long iomapstart = VMALLOC_END;
> 
> unsigned long get_iomap_virtbase(unsigned long size)
> {
>     iomapstart -= size;
>     return iomapstart;
> }

It wouldn't be so simple with page/section alignment requirements and such.

> Something like this will be helpful and then instead of setup virtual
> addresses for every static io mapping just to call this function
> and you will get mapping correct all the time.

If you use DT for core count, then you don't need the static mapping and
can use ioremap (or of_iomap).

Rob
Michal Simek March 26, 2013, 12:33 p.m. UTC | #8
2013/3/26 Rob Herring <robherring2@gmail.com>:
> On 03/26/2013 05:45 AM, Michal Simek wrote:
>> On 03/25/2013 11:34 PM, Rob Herring wrote:
>>> On 03/25/2013 11:07 AM, Michal Simek wrote:
>>>> 2013/3/25 Rob Herring <robherring2@gmail.com>:
>>>>> On 03/25/2013 09:51 AM, Michal Simek wrote:
>>>>>> Hi Rob,
>>>>>>
>>>>>> 2013/3/25 Rob Herring <robherring2@gmail.com>:
>>>>>>> On 03/25/2013 08:53 AM, Michal Simek wrote:
>>>>>>>> Use Cortex a9 cp15 to read scu baseaddress.
>>>>>
>>>>> [...]
>>>>>
>>>>>>>> +static void __init scu_init(void)
>>>>>>>> +{
>>>>>>>> +     unsigned long base;
>>>>>>>> +
>>>>>>>> +     base = scu_a9_get_base();
>>>>>>>> +     zynq_cortex_a9_scu_map.pfn = __phys_to_pfn(base);
>>>>>>>> +     zynq_cortex_a9_scu_map.virtual = base;
>>>>>>>
>>>>>>> You are setting the virtual address to the physical base?
>>>>>>>
>>>>>>>> +     iotable_init(&zynq_cortex_a9_scu_map, 1);
>>>>>>>
>>>>>>> Then creating a static mapping...
>>>>>>>
>>>>>>>> +     scu_base = ioremap(base, zynq_cortex_a9_scu_map.length);
>>>>>>>
>>>>>>> And also a dynamic mapping?
>>>>>>
>>>>>> Yes - exactly.
>>>>>
>>>>> You are simply getting lucky that it works. If the physical address did
>>>>> not happen to be in the vmalloc address region, it would not work. You
>>>>> should not do this because you have an implicit requirement and the
>>>>> code
>>>>> will look broken to anyone that reads it.
>>>>
>>>> yeah correct. I will add there a comment to mentioned that.
>>>
>>> I think leaving the define would be better, but either way is fine.
>>
>> ok. From my point of view is better to keep scu where it is and any
>> early vmalloc
>> allocator will be really helpful. Because there is also early map
>> for early console where virtual addresses are hardcoded.
>> It will be just easier to ask generic code to return base if if you request
>> for size.
>>
>> static unsigned long iomapstart = VMALLOC_END;
>>
>> unsigned long get_iomap_virtbase(unsigned long size)
>> {
>>     iomapstart -= size;
>>     return iomapstart;
>> }
>
> It wouldn't be so simple with page/section alignment requirements and such.

Adding alignment to it is not so hard.


>> Something like this will be helpful and then instead of setup virtual
>> addresses for every static io mapping just to call this function
>> and you will get mapping correct all the time.
>
> If you use DT for core count, then you don't need the static mapping and
> can use ioremap (or of_iomap).

yep but on the other hand I have to be sure that all users
are using proper DTS files. And based on my experience with users
I would rather relate on scu count instead of DT count.

Thanks,
Michal
Pawel Moll April 2, 2013, 4:40 p.m. UTC | #9
Greetings,

Apologies about "slightly" delayed response.

On Mon, 2013-03-25 at 16:07 +0000, Michal Simek wrote:
> Pawel: You are the author of this code in vexpress. Is there any
> particular reason
> to have there that ioremap?

At the time there was no standard DT->platsmp solution and the
traditional way was using the SCU to work out the CPUs.

If I was doing this today (and I'll re-do this code at some point) I'd
rely on the DT in the first place and maybe then have a SCU
"micro-driver" double-checking the DT data against the SCU information.
That way I could reduce the dependency on static mappings. And this is
what I want to get - if possible I don't want any static mapping at all.

Pawe?
Steffen Trumtrar April 3, 2013, 7:25 a.m. UTC | #10
On Wed, Apr 03, 2013 at 08:33:19AM +0200, Michal Simek wrote:
> Hi
> 
> 2013/4/2 Pawel Moll <pawel.moll@arm.com>
> 
> > Greetings,
> >
> > Apologies about "slightly" delayed response.
> >
> > On Mon, 2013-03-25 at 16:07 +0000, Michal Simek wrote:
> > > Pawel: You are the author of this code in vexpress. Is there any
> > > particular reason
> > > to have there that ioremap?
> >
> > At the time there was no standard DT->platsmp solution and the
> > traditional way was using the SCU to work out the CPUs.
> >
> > If I was doing this today (and I'll re-do this code at some point) I'd
> > rely on the DT in the first place and maybe then have a SCU
> > "micro-driver" double-checking the DT data against the SCU information.
> > That way I could reduce the dependency on static mappings. And this is
> > what I want to get - if possible I don't want any static mapping at all.
> >
> 
> Sure. Currently I want to use that static mapping because I know that user
> DTSes are just wrong and relating on HW is better for me right now.
> 

Wouldn't we risk getting more faulty DTSes instead of stopping them?
If you stop the support starting with kernel 3.xx everyone has to fix
their *.dts and be done with it.

Regards,
Steffen
Pawel Moll April 3, 2013, 4:06 p.m. UTC | #11
On Wed, 2013-04-03 at 07:33 +0100, Michal Simek wrote:
> The point is different. Was there any reason why you used this static
> mapping
> and then dynamic?
> This code:
>         iotable_init(&vexpress_dt_cortex_a9_scu_map, 1);
>         vexpress_dt_cortex_a9_scu_base = ioremap(phys_addr, SZ_256);

The SCU code is being used very early - too early for "normal" ioremap()
(one problem I'm aware of is lack of kmalloc, but there may be more). So
the static mapping is required. Now, traditionally there was some (more
or less cryptic) pointer arithmetic was used (eg. "base = map_virt_addr
+ 0x100"). I'm sure some would argue it's fine, but I personally dislike
it. Then Nico Pitre's patches made the ioremap() aware of the static
mappings so it will reuse them when possible. Of course it would be
better to be able to do normal ioremap() without any tricks, but it's
another story and I don't feel like discussing it.

So, to summarize, the static mapping is there to make the ioremap()
possible in the first place. If your ioremap() works without it (which
means that you're doing it late enough) you can ignore the
iotable_init() completely.

Does this answer the question?

Pawe?
Pawel Moll April 3, 2013, 5:11 p.m. UTC | #12
On Wed, 2013-04-03 at 18:04 +0100, Michal Simek wrote:
> Then my question is if you can just remove that ioremap, as I have
> done for zynq,
> without any related problem.

Well, the vexpress platsmp functions are going away soon, replaced by
the generic DT code. When this happen I won't even try to access the
SCU, so both its static mapping and the ioremap will go as well :-)

Pawe?
diff mbox

Patch

diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
index 2a72339..4943568 100644
--- a/arch/arm/boot/dts/zynq-7000.dtsi
+++ b/arch/arm/boot/dts/zynq-7000.dtsi
@@ -22,6 +22,11 @@ 
 		interrupt-parent = <&intc>;
 		ranges;
 
+		scu: scu@f8f000000 {
+			compatible = "arm,cortex-a9-scu";
+			reg = <0xf8f00000 0x58>;
+		};
+
 		intc: interrupt-controller@f8f01000 {
 			compatible = "arm,cortex-a9-gic";
 			#interrupt-cells = <3>;
diff --git a/arch/arm/mach-zynq/common.c b/arch/arm/mach-zynq/common.c
index 68e0907..13f9d8b 100644
--- a/arch/arm/mach-zynq/common.c
+++ b/arch/arm/mach-zynq/common.c
@@ -33,10 +33,13 @@ 
 #include <asm/mach-types.h>
 #include <asm/page.h>
 #include <asm/pgtable.h>
+#include <asm/smp_scu.h>
 #include <asm/hardware/cache-l2x0.h>
 
 #include "common.h"
 
+void __iomem *scu_base;
+
 static struct of_device_id zynq_of_bus_ids[] __initdata = {
 	{ .compatible = "simple-bus", },
 	{}
@@ -56,17 +59,6 @@  static void __init xilinx_init_machine(void)
 	of_platform_bus_probe(NULL, zynq_of_bus_ids, NULL);
 }
 
-#define SCU_PERIPH_PHYS		0xF8F00000
-#define SCU_PERIPH_SIZE		SZ_8K
-#define SCU_PERIPH_VIRT		(VMALLOC_END - SCU_PERIPH_SIZE)
-
-static struct map_desc scu_desc __initdata = {
-	.virtual	= SCU_PERIPH_VIRT,
-	.pfn		= __phys_to_pfn(SCU_PERIPH_PHYS),
-	.length		= SCU_PERIPH_SIZE,
-	.type		= MT_DEVICE,
-};
-
 static void __init xilinx_zynq_timer_init(void)
 {
 	struct device_node *np;
@@ -81,13 +73,30 @@  static void __init xilinx_zynq_timer_init(void)
 	clocksource_of_init();
 }
 
+static struct map_desc zynq_cortex_a9_scu_map __initdata = {
+	.length	= SZ_256,
+	.type	= MT_DEVICE,
+};
+
+static void __init scu_init(void)
+{
+	unsigned long base;
+
+	base = scu_a9_get_base();
+	zynq_cortex_a9_scu_map.pfn = __phys_to_pfn(base);
+	zynq_cortex_a9_scu_map.virtual = base;
+	iotable_init(&zynq_cortex_a9_scu_map, 1);
+	scu_base = ioremap(base, zynq_cortex_a9_scu_map.length);
+	BUG_ON(!scu_base);
+}
+
 /**
  * xilinx_map_io() - Create memory mappings needed for early I/O.
  */
 static void __init xilinx_map_io(void)
 {
 	debug_ll_io_init();
-	iotable_init(&scu_desc, 1);
+	scu_init();
 }
 
 static const char *xilinx_dt_match[] = {
diff --git a/arch/arm/mach-zynq/common.h b/arch/arm/mach-zynq/common.h
index 5050bb1..38727a2 100644
--- a/arch/arm/mach-zynq/common.h
+++ b/arch/arm/mach-zynq/common.h
@@ -17,4 +17,6 @@ 
 #ifndef __MACH_ZYNQ_COMMON_H__
 #define __MACH_ZYNQ_COMMON_H__
 
+extern void __iomem *scu_base;
+
 #endif