On Fri, 2014-11-14 at 11:52AM +0100, Michal Simek wrote:
> Get OCM configuration from SLCR.
>
> Signed-off-by: Michal Simek <[email protected]>
> ---
>
> Changes in v4:
> - slcr.h has moved to soc/include/ folder. Move definition there too.
>
> Changes in v3: None
> Changes in v2: None
>
> arch/arm/mach-zynq/slcr.c | 15 +++++++++++++++
> include/soc/zynq/slcr.h | 1 +
> 2 files changed, 16 insertions(+)
>
> diff --git a/arch/arm/mach-zynq/slcr.c b/arch/arm/mach-zynq/slcr.c
> index b77e42d999c8..f7c606e57525 100644
> --- a/arch/arm/mach-zynq/slcr.c
> +++ b/arch/arm/mach-zynq/slcr.c
> @@ -29,6 +29,7 @@
> #define SLCR_A9_CPU_RST_CTRL_OFFSET 0x244 /* CPU Software Reset Control */
> #define SLCR_REBOOT_STATUS_OFFSET 0x258 /* PS Reboot Status */
> #define SLCR_PSS_IDCODE 0x530 /* PS IDCODE */
> +#define SLCR_OCM_CFG_OFFSET 0x910 /* OCM Address Mapping */
>
> #define SLCR_UNLOCK_MAGIC 0xDF0D
> #define SLCR_A9_CPU_CLKSTOP 0x10
> @@ -128,6 +129,20 @@ void zynq_slcr_system_reset(void)
> }
>
> /**
> + * zynq_slcr_get_ocm_config - Get SLCR OCM config
> + *
> + * Return: OCM config bits
> + */
> +u32 zynq_slcr_get_ocm_config(void)
> +{
> + u32 val;
> +
> + zynq_slcr_read(&val, SLCR_OCM_CFG_OFFSET);
> +
> + return val;
> +}
> +
> +/**
> * zynq_slcr_cpu_start - Start cpu
> * @cpu: cpu number
> */
> diff --git a/include/soc/zynq/slcr.h b/include/soc/zynq/slcr.h
> index 7b4edab666ee..639723c4f90c 100644
> --- a/include/soc/zynq/slcr.h
> +++ b/include/soc/zynq/slcr.h
> @@ -25,5 +25,6 @@ extern void zynq_slcr_cpu_start(int cpu);
> extern bool zynq_slcr_cpu_state_read(int cpu);
> extern void zynq_slcr_cpu_state_write(int cpu, bool die);
> extern u32 zynq_slcr_get_device_id(void);
> +extern u32 zynq_slcr_get_ocm_config(void);
I still believe we should consolidate this somehow instead of adding
another function for each and every register that might have to be read
by something else.
Usage of syscon/regmap might be an option. Or probably something like:
enum zynq_slcr_reg {
zynq_slcr_ocm_cfg,
zynq_slcr_device_id,
...
};
u32 zyn_slcr_read(enum zynq_slcr_reg id)
{
switch (id) {
case ...
}
Sören
Hi Michal,
Am 14.11.2014 um 11:52 schrieb Michal Simek:
> diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
> index ce2ef5bec4f2..e217fb1c1169 100644
> --- a/arch/arm/boot/dts/zynq-7000.dtsi
> +++ b/arch/arm/boot/dts/zynq-7000.dtsi
> @@ -150,6 +150,13 @@
> reg = <0xf8006000 0x1000>;
> };
>
> + ocmc: memory-controller@f800c000 {
> + compatible = "xlnx,zynq-ocmc-1.0";
> + interrupt-parent = <&intc>;
> + interrupts = <0 3 4>;
> + reg = <0xf800c000 0x1000>;
> + };
> +
> uart0: serial@e0000000 {
> compatible = "xlnx,xuartps", "cdns,uart-r1p8";
> status = "disabled";
Not directly related to this patch: As one can see here, the node order
is quite a mess... According to Olof, nodes should be ordered by unit
address, whereas here some but not all seem ordered by node name. Would
you welcome a cleanup patch, or can you fix that yourself?
Regards,
Andreas
--
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imend?rffer; HRB 21284 AG N?rnberg
On Sun, 2014-11-16 at 11:51AM +0100, Andreas Färber wrote:
> Hi Michal,
>
> Am 14.11.2014 um 11:52 schrieb Michal Simek:
> > diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
> > index ce2ef5bec4f2..e217fb1c1169 100644
> > --- a/arch/arm/boot/dts/zynq-7000.dtsi
> > +++ b/arch/arm/boot/dts/zynq-7000.dtsi
> > @@ -150,6 +150,13 @@
> > reg = <0xf8006000 0x1000>;
> > };
> >
> > + ocmc: memory-controller@f800c000 {
> > + compatible = "xlnx,zynq-ocmc-1.0";
> > + interrupt-parent = <&intc>;
> > + interrupts = <0 3 4>;
> > + reg = <0xf800c000 0x1000>;
> > + };
> > +
> > uart0: serial@e0000000 {
> > compatible = "xlnx,xuartps", "cdns,uart-r1p8";
> > status = "disabled";
>
> Not directly related to this patch: As one can see here, the node order
> is quite a mess... According to Olof, nodes should be ordered by unit
> address, whereas here some but not all seem ordered by node name. Would
> you welcome a cleanup patch, or can you fix that yourself?
I wouldn't say it's a mess, just a different property to sort the nodes
by. For humans reading the DT, searching for nodes, alphabetical order
helps finding the right node, IMHO. What advantage would sorting by
address have?
Thanks,
Sören
On Mon, Nov 17, 2014 at 5:32 AM, Sören Brinkmann
<[email protected]> wrote:
> On Sun, 2014-11-16 at 11:51AM +0100, Andreas Färber wrote:
>> Hi Michal,
>>
>> Am 14.11.2014 um 11:52 schrieb Michal Simek:
>> > diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
>> > index ce2ef5bec4f2..e217fb1c1169 100644
>> > --- a/arch/arm/boot/dts/zynq-7000.dtsi
>> > +++ b/arch/arm/boot/dts/zynq-7000.dtsi
>> > @@ -150,6 +150,13 @@
>> > reg = <0xf8006000 0x1000>;
>> > };
>> >
>> > + ocmc: memory-controller@f800c000 {
>> > + compatible = "xlnx,zynq-ocmc-1.0";
>> > + interrupt-parent = <&intc>;
>> > + interrupts = <0 3 4>;
>> > + reg = <0xf800c000 0x1000>;
>> > + };
>> > +
>> > uart0: serial@e0000000 {
>> > compatible = "xlnx,xuartps", "cdns,uart-r1p8";
>> > status = "disabled";
>>
>> Not directly related to this patch: As one can see here, the node order
>> is quite a mess... According to Olof, nodes should be ordered by unit
>> address, whereas here some but not all seem ordered by node name. Would
>> you welcome a cleanup patch, or can you fix that yourself?
>
> I wouldn't say it's a mess, just a different property to sort the nodes
> by. For humans reading the DT, searching for nodes, alphabetical order
> helps finding the right node, IMHO.
I do generally find myself asking "whats that thing at that address"
more than I find myself asking the "wheres that piece of hardware" so
Andreas' sorting scheme makes more sense to me. Vertically scanning a
DT to give yourself an overview of the system level address map is
good too. Wheras alphabetic sorting doesn't mean to much.
Regards,
Peter
What advantage would sorting by
> address have?
>
> Thanks,
> Sören
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On 11/17/2014 12:00 AM, Peter Crosthwaite wrote:
> On Mon, Nov 17, 2014 at 5:32 AM, Sören Brinkmann
> <[email protected]> wrote:
>> On Sun, 2014-11-16 at 11:51AM +0100, Andreas Färber wrote:
>>> Hi Michal,
>>>
>>> Am 14.11.2014 um 11:52 schrieb Michal Simek:
>>>> diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
>>>> index ce2ef5bec4f2..e217fb1c1169 100644
>>>> --- a/arch/arm/boot/dts/zynq-7000.dtsi
>>>> +++ b/arch/arm/boot/dts/zynq-7000.dtsi
>>>> @@ -150,6 +150,13 @@
>>>> reg = <0xf8006000 0x1000>;
>>>> };
>>>>
>>>> + ocmc: memory-controller@f800c000 {
>>>> + compatible = "xlnx,zynq-ocmc-1.0";
>>>> + interrupt-parent = <&intc>;
>>>> + interrupts = <0 3 4>;
>>>> + reg = <0xf800c000 0x1000>;
>>>> + };
>>>> +
>>>> uart0: serial@e0000000 {
>>>> compatible = "xlnx,xuartps", "cdns,uart-r1p8";
>>>> status = "disabled";
>>>
>>> Not directly related to this patch: As one can see here, the node order
>>> is quite a mess... According to Olof, nodes should be ordered by unit
>>> address, whereas here some but not all seem ordered by node name. Would
>>> you welcome a cleanup patch, or can you fix that yourself?
>>
>> I wouldn't say it's a mess, just a different property to sort the nodes
>> by. For humans reading the DT, searching for nodes, alphabetical order
>> helps finding the right node, IMHO.
>
> I do generally find myself asking "whats that thing at that address"
> more than I find myself asking the "wheres that piece of hardware" so
> Andreas' sorting scheme makes more sense to me. Vertically scanning a
> DT to give yourself an overview of the system level address map is
> good too. Wheras alphabetic sorting doesn't mean to much.
IMHO the reason why we have names in DT is that it is easily to read/understand them
that's why name sorting seems to me more reasonable.
Something like machine code and assembler - asm is also sorted by names not by opcode.
Is this strict rule?
Thanks,
Michal
On Fri, Nov 14, 2014 at 11:52 AM, Michal Simek <[email protected]> wrote:
> The driver provide memory allocator which can
> be used by others drivers to allocate memory inside OCM.
> All location for 64kB blocks are supported
Allocation?
> and driver is trying to allocate the largest continuous
> block of memory.
Isn't all genalloc allocations continuous?
(...)
> + zynq_ocmc->pool = devm_gen_pool_create(&pdev->dev,
> + ilog2(ZYNQ_OCMC_GRANULARITY),
> + -1);
Do this:
#include <linux/sizes.h>
zynq_ocmc->pool = devm_gen_pool_create(&pdev->dev,
ilog2(SZ_64K),
-1);
And get rid of the #define for ZYNQ_OCMC_GRANULARITY
Yours,
Linus Walleij
On 11/27/2014 02:20 PM, Linus Walleij wrote:
> On Fri, Nov 14, 2014 at 11:52 AM, Michal Simek <[email protected]> wrote:
>
>> The driver provide memory allocator which can
>> be used by others drivers to allocate memory inside OCM.
>> All location for 64kB blocks are supported
>
> Allocation?
>
>> and driver is trying to allocate the largest continuous
>> block of memory.
>
> Isn't all genalloc allocations continuous?
I hope it is but blocks can be at different locations.
Two locations 0 or 0xfffc0000.
it means for every 64k block can be at low or high address location
first block 0x0 or 0xfffc0000
second block 0x10000 or 0xfffd0000
third block 0x20000 or 0xfffe0000
four block 0x30000 or 0xffff0000
it is worth to allocate to biggest continuous memory.
For example 2 blocks low and then 2 blocks high.
2 blocks with 128kB is better then 4 blocks with 64kB
>
> (...)
>> + zynq_ocmc->pool = devm_gen_pool_create(&pdev->dev,
>> + ilog2(ZYNQ_OCMC_GRANULARITY),
>> + -1);
>
> Do this:
>
> #include <linux/sizes.h>
>
> zynq_ocmc->pool = devm_gen_pool_create(&pdev->dev,
> ilog2(SZ_64K),
> -1);
>
> And get rid of the #define for ZYNQ_OCMC_GRANULARITY
ilog2 from 32 is different to ilog2 from ilog2 from 0x10000.
But I will look at gen pool internals.
Do you have any opinion regarding calling zynq_slcr_get_ocm_config()?
Is it better to expose slcr this interface to drivers?
Or use regmap and read this value directly?
Also I do read for CONFIG_SMP case jump trampoline size - maybe
you can have better idea how this can be done.
Thanks for your input,
Michal
On Thu, Nov 27, 2014 at 2:57 PM, Michal Simek <[email protected]> wrote:
> On 11/27/2014 02:20 PM, Linus Walleij wrote:
>> On Fri, Nov 14, 2014 at 11:52 AM, Michal Simek <[email protected]> wrote:
>> (...)
>>> + zynq_ocmc->pool = devm_gen_pool_create(&pdev->dev,
>>> + ilog2(ZYNQ_OCMC_GRANULARITY),
>>> + -1);
>>
>> Do this:
>>
>> #include <linux/sizes.h>
>>
>> zynq_ocmc->pool = devm_gen_pool_create(&pdev->dev,
>> ilog2(SZ_64K),
>> -1);
>>
>> And get rid of the #define for ZYNQ_OCMC_GRANULARITY
>
> ilog2 from 32 is different to ilog2 from ilog2 from 0x10000.
Bah I misread the code, forget this comment.
Maybe it's more like I wanted
+#define ZYNQ_OCMC_BLOCK_SIZE 0x10000
To be replaced with SZ_64K
But it's a petty detail anyway.
> Do you have any opinion regarding calling zynq_slcr_get_ocm_config()?
>
> Is it better to expose slcr this interface to drivers?
> Or use regmap and read this value directly?
Depends on what provides that call. The pattern I usually follow
is to expose the mixed-registers range as a syscon device
using drivers/mfd/syscon.c and then use one of the methods from
<linux/mfd/syscon.h> to look up a reference to the regmap and
use it to access misc registers.
> Also I do read for CONFIG_SMP case jump trampoline size - maybe
> you can have better idea how this can be done.
No I have no clue about that... :(
Yours,
Linus Walleij
On 11/28/2014 04:35 PM, Linus Walleij wrote:
> On Thu, Nov 27, 2014 at 2:57 PM, Michal Simek <[email protected]> wrote:
>> On 11/27/2014 02:20 PM, Linus Walleij wrote:
>>> On Fri, Nov 14, 2014 at 11:52 AM, Michal Simek <[email protected]> wrote:
>
>>> (...)
>>>> + zynq_ocmc->pool = devm_gen_pool_create(&pdev->dev,
>>>> + ilog2(ZYNQ_OCMC_GRANULARITY),
>>>> + -1);
>>>
>>> Do this:
>>>
>>> #include <linux/sizes.h>
>>>
>>> zynq_ocmc->pool = devm_gen_pool_create(&pdev->dev,
>>> ilog2(SZ_64K),
>>> -1);
>>>
>>> And get rid of the #define for ZYNQ_OCMC_GRANULARITY
>>
>> ilog2 from 32 is different to ilog2 from ilog2 from 0x10000.
>
> Bah I misread the code, forget this comment.
>
> Maybe it's more like I wanted
>
> +#define ZYNQ_OCMC_BLOCK_SIZE 0x10000
>
> To be replaced with SZ_64K
>
> But it's a petty detail anyway.
I have fixed it.
>> Do you have any opinion regarding calling zynq_slcr_get_ocm_config()?
>>
>> Is it better to expose slcr this interface to drivers?
>> Or use regmap and read this value directly?
>
> Depends on what provides that call. The pattern I usually follow
> is to expose the mixed-registers range as a syscon device
> using drivers/mfd/syscon.c and then use one of the methods from
> <linux/mfd/syscon.h> to look up a reference to the regmap and
> use it to access misc registers.
I have tried it and I can just use it without any problem.
I have sent v5 with origin version but in cover letter there
is a code for that.
>> Also I do read for CONFIG_SMP case jump trampoline size - maybe
>> you can have better idea how this can be done.
>
> No I have no clue about that... :(
ok - fair enough. One option is to keep it as is. The second
option is to allocate any hardcoded size or size passed via DT.
But run-time detection is the best IMHO.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform