Re: [U-Boot] [PATCH 1/3] arm: socfpga: Convert reset manager from struct to defines

[Ley Foon Tan]

On Wed, Aug 28, 2019 at 9:39 PM Marek Vasut  wrote:
>
> On 8/28/19 3:28 PM, Ley Foon Tan wrote:
>
> [...]
>
> >>>Tested this on Agilex, look okay. But, pending test on other device
> >>> families, like Gen5 or A10. Not sure any side effect with this change.
> >>> 2. Secure (for PSCI) and non-secure functions share same functions,
> >>> these functions can't use global data (for base address). Secure
> >>> functions have its own secure data region.
> >>
> >> There was a discussion about this before, instead of hacking up this
> >> part constantly, maybe we should create a separate build of U-Boot (like
> >> SPL/TPL) and use that for the "secure" mode. But that's for another
> >> patchset and another discussion.
> > Yes, we have discussion here internally regarding this secure firmware.
> > We plan change the boot flow to include ATF. But, our downstream git
> > still need to maintain this "secure" region for some period (so
> > customer can migrate to new boot flow).
> > If change to DT address, we need patch our downstream git for these
> > secure functions. :)
> > Anyway, I will try change for upstream. I'm trying to find where is
> > the proper place to get DT address in Uboot. SPL is more straight
> > forward, get the DT address in board_init_f.
>
> I have to admit, I have many things to say about ATF and none of them
> are positive. So it's real sad this is starting to move onto S10 too.
>
> Anyway, one of the fdt_ functions should allow you to fish out the
> address from DT.
Yes, we can use fdt_ functions. I got this part working.

Thanks.

Regards
Ley Foon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 1/3] arm: socfpga: Convert reset manager from struct to defines

[Marek Vasut]

On 8/28/19 3:28 PM, Ley Foon Tan wrote:

[...]

>>>Tested this on Agilex, look okay. But, pending test on other device
>>> families, like Gen5 or A10. Not sure any side effect with this change.
>>> 2. Secure (for PSCI) and non-secure functions share same functions,
>>> these functions can't use global data (for base address). Secure
>>> functions have its own secure data region.
>>
>> There was a discussion about this before, instead of hacking up this
>> part constantly, maybe we should create a separate build of U-Boot (like
>> SPL/TPL) and use that for the "secure" mode. But that's for another
>> patchset and another discussion.
> Yes, we have discussion here internally regarding this secure firmware.
> We plan change the boot flow to include ATF. But, our downstream git
> still need to maintain this "secure" region for some period (so
> customer can migrate to new boot flow).
> If change to DT address, we need patch our downstream git for these
> secure functions. :)
> Anyway, I will try change for upstream. I'm trying to find where is
> the proper place to get DT address in Uboot. SPL is more straight
> forward, get the DT address in board_init_f.

I have to admit, I have many things to say about ATF and none of them
are positive. So it's real sad this is starting to move onto S10 too.

Anyway, one of the fdt_ functions should allow you to fish out the
address from DT.
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 1/3] arm: socfpga: Convert reset manager from struct to defines

[Ley Foon Tan]

On Wed, Aug 28, 2019 at 8:44 PM Marek Vasut  wrote:
>
> On 8/28/19 10:13 AM, Ley Foon Tan wrote:
> [...]
>
> >> If want to use address from DT, we need get address every time need to
> >> use the address.
> >> For non-DM, it is easier to use constant address. Let me know if you
> >> still prefer to use address from DT.
> 
>  Surely you can cache the address or even better, convert to DM/DT ?
> >>> Will try to cache the address for now.  But, we need get address from
> >>> DT twice, one in SPL, one in Uboot. They not sharing global data.
> >>
> >> I don't think there's much we can do about that, unless we can somehow
> >> share parsed live DT from SPL to U-Boot, but that's for another
> >> discussion/patchset/etc.
> > I am working on this now and found there are 2 challenges if we get
> > base address from DT.
> > 1. We can only get the address from DT after gd->fdt_blob is
> > initialized in spl_early_init(). So, spl_early_init() need move to
> > beginning of board_init_f().
>
> This should be reasonably easy to solve I guess ?
Yes, this shouldn't be tough, just need testing. Make sure it doesn't
break anything.
>
> >Tested this on Agilex, look okay. But, pending test on other device
> > families, like Gen5 or A10. Not sure any side effect with this change.
> > 2. Secure (for PSCI) and non-secure functions share same functions,
> > these functions can't use global data (for base address). Secure
> > functions have its own secure data region.
>
> There was a discussion about this before, instead of hacking up this
> part constantly, maybe we should create a separate build of U-Boot (like
> SPL/TPL) and use that for the "secure" mode. But that's for another
> patchset and another discussion.
Yes, we have discussion here internally regarding this secure firmware.
We plan change the boot flow to include ATF. But, our downstream git
still need to maintain this "secure" region for some period (so
customer can migrate to new boot flow).
If change to DT address, we need patch our downstream git for these
secure functions. :)
Anyway, I will try change for upstream. I'm trying to find where is
the proper place to get DT address in Uboot. SPL is more straight
forward, get the DT address in board_init_f.

Thanks.

Regards
Ley Foon

>
> > #2 is more difficult to overcome. Do you have strong opinion to get
> > base address from DT? Otherwise, can we use constant define for these
> > base addresses?
>
> See above, the secure part might need further work.
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 1/3] arm: socfpga: Convert reset manager from struct to defines

[Simon Goldschmidt]

Marek Vasut  schrieb am Mi., 28. Aug. 2019, 14:44:

> On 8/28/19 10:13 AM, Ley Foon Tan wrote:
> [...]
>
> >> If want to use address from DT, we need get address every time need
> to
> >> use the address.
> >> For non-DM, it is easier to use constant address. Let me know if you
> >> still prefer to use address from DT.
> 
>  Surely you can cache the address or even better, convert to DM/DT ?
> >>> Will try to cache the address for now.  But, we need get address from
> >>> DT twice, one in SPL, one in Uboot. They not sharing global data.
> >>
> >> I don't think there's much we can do about that, unless we can somehow
> >> share parsed live DT from SPL to U-Boot, but that's for another
> >> discussion/patchset/etc.
> > I am working on this now and found there are 2 challenges if we get
> > base address from DT.
> > 1. We can only get the address from DT after gd->fdt_blob is
> > initialized in spl_early_init(). So, spl_early_init() need move to
> > beginning of board_init_f().
>
> This should be reasonably easy to solve I guess ?
>

I did that for gen5 already in my pending patchset. It was kind of easy,
although problems like coding bugs or lack of heap are hard to debug as the
debug Hart doesn't work that early (no ponmux setup etc.)


> >Tested this on Agilex, look okay. But, pending test on other device
> > families, like Gen5 or A10. Not sure any side effect with this change.
> > 2. Secure (for PSCI) and non-secure functions share same functions,
> > these functions can't use global data (for base address). Secure
> > functions have its own secure data region.
>
> There was a discussion about this before, instead of hacking up this
> part constantly, maybe we should create a separate build of U-Boot (like
> SPL/TPL) and use that for the "secure" mode. But that's for another
> patchset and another discussion.
>
> > #2 is more difficult to overcome. Do you have strong opinion to get
> > base address from DT? Otherwise, can we use constant define for these
> > base addresses?
>
> See above, the secure part might need further work.
>
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 1/3] arm: socfpga: Convert reset manager from struct to defines

[Marek Vasut]

On 8/28/19 10:13 AM, Ley Foon Tan wrote:
[...]

>> If want to use address from DT, we need get address every time need to
>> use the address.
>> For non-DM, it is easier to use constant address. Let me know if you
>> still prefer to use address from DT.

 Surely you can cache the address or even better, convert to DM/DT ?
>>> Will try to cache the address for now.  But, we need get address from
>>> DT twice, one in SPL, one in Uboot. They not sharing global data.
>>
>> I don't think there's much we can do about that, unless we can somehow
>> share parsed live DT from SPL to U-Boot, but that's for another
>> discussion/patchset/etc.
> I am working on this now and found there are 2 challenges if we get
> base address from DT.
> 1. We can only get the address from DT after gd->fdt_blob is
> initialized in spl_early_init(). So, spl_early_init() need move to
> beginning of board_init_f().

This should be reasonably easy to solve I guess ?

>Tested this on Agilex, look okay. But, pending test on other device
> families, like Gen5 or A10. Not sure any side effect with this change.
> 2. Secure (for PSCI) and non-secure functions share same functions,
> these functions can't use global data (for base address). Secure
> functions have its own secure data region.

There was a discussion about this before, instead of hacking up this
part constantly, maybe we should create a separate build of U-Boot (like
SPL/TPL) and use that for the "secure" mode. But that's for another
patchset and another discussion.

> #2 is more difficult to overcome. Do you have strong opinion to get
> base address from DT? Otherwise, can we use constant define for these
> base addresses?

See above, the secure part might need further work.
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 1/3] arm: socfpga: Convert reset manager from struct to defines

[Ley Foon Tan]

On Fri, Aug 23, 2019 at 5:56 PM Marek Vasut  wrote:
>
> On 8/23/19 11:52 AM, Ley Foon Tan wrote:
> > On Fri, Aug 23, 2019 at 5:23 PM Marek Vasut  wrote:
> >>
> >> On 8/23/19 10:57 AM, Ley Foon Tan wrote:
> >>> On Wed, Aug 21, 2019 at 9:50 AM Ley Foon Tan  
> >>> wrote:
> 
>  On Tue, Aug 20, 2019 at 5:50 PM Marek Vasut  wrote:
> >
> > On 8/20/19 4:35 AM, Ley Foon Tan wrote:
> >> Convert reset manager for Gen5, Arria 10 and Stratix 10 from struct
> >> to defines.
> >> No functional change.
> >>
> >> Signed-off-by: Ley Foon Tan 
> >> ---
> >>  .../mach-socfpga/include/mach/reset_manager.h | 12 +
> >>  .../include/mach/reset_manager_arria10.h  | 41 +++-
> >>  .../include/mach/reset_manager_gen5.h | 20 +++-
> >>  .../include/mach/reset_manager_s10.h  | 33 ++---
> >>  arch/arm/mach-socfpga/misc_gen5.c |  6 +--
> >>  arch/arm/mach-socfpga/reset_manager_arria10.c | 49 +--
> >>  arch/arm/mach-socfpga/reset_manager_gen5.c| 26 +-
> >>  arch/arm/mach-socfpga/reset_manager_s10.c | 35 ++---
> >>  drivers/sysreset/sysreset_socfpga.c   |  6 +--
> >>  9 files changed, 86 insertions(+), 142 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-socfpga/include/mach/reset_manager.h 
> >> b/arch/arm/mach-socfpga/include/mach/reset_manager.h
> >> index 6ad037e325..c460e89d22 100644
> >> --- a/arch/arm/mach-socfpga/include/mach/reset_manager.h
> >> +++ b/arch/arm/mach-socfpga/include/mach/reset_manager.h
> >> @@ -6,6 +6,18 @@
> >>  #ifndef _RESET_MANAGER_H_
> >>  #define _RESET_MANAGER_H_
> >>
> >> +#define RSTMGR_READL(reg)\
> >> + readl(SOCFPGA_RSTMGR_ADDRESS + (reg))
> >> +
> >> +#define RSTMGR_WRITEL(data, reg) \
> >> + writel(data, SOCFPGA_RSTMGR_ADDRESS + (reg))
> >> +
> >> +#define RSTMGR_CLRBITS(reg, mask)\
> >> + clrbits_le32(SOCFPGA_RSTMGR_ADDRESS + (reg), mask)
> >> +
> >> +#define RSTMGR_SETBITS(reg, mask)\
> >> + setbits_le32(SOCFPGA_RSTMGR_ADDRESS + (reg), mask)
> >>
> >> btw. mask is missing parenthesis, but see below.
> >>
> >> +
> >
> > No, don't introduce such macros. Use readl()/writel()/... in the driver.
> > The address should come from DT. Besides, there is no type checking in
> > such macros.
>  These macros call to writel() and readl() underlying, it just add base
>  address to it.
> >>
> >> Right, it just makes the code hardware to read and understand.
> > Okay, will change it.
>
> It should be easy to do:
> $ sed "s@RSTMGR_SETBITS(\([^,]\+\),
> \([^)]\+\))@setbits_le32(SOCFPGA_RSTMGR_ADDRESS + \1, \1)@"
> $ git add -u ; git commit -sm part1
> $ checkpatch -f --fix --fix-inplace 
> $ git diff # review the fixes
> $ git add -u ; git commit --amend #
>
>  So, it should have type checking as call writel()/readl() directly?
> >>
> >> The functions in the macro get typechecked. But unlike function
> >> arguments, macro arguments are not checked.
> > Noted.
> >>
>  If want to use address from DT, we need get address every time need to
>  use the address.
>  For non-DM, it is easier to use constant address. Let me know if you
>  still prefer to use address from DT.
> >>
> >> Surely you can cache the address or even better, convert to DM/DT ?
> > Will try to cache the address for now.  But, we need get address from
> > DT twice, one in SPL, one in Uboot. They not sharing global data.
>
> I don't think there's much we can do about that, unless we can somehow
> share parsed live DT from SPL to U-Boot, but that's for another
> discussion/patchset/etc.
I am working on this now and found there are 2 challenges if we get
base address from DT.
1. We can only get the address from DT after gd->fdt_blob is
initialized in spl_early_init(). So, spl_early_init() need move to
beginning of board_init_f().
   Tested this on Agilex, look okay. But, pending test on other device
families, like Gen5 or A10. Not sure any side effect with this change.
2. Secure (for PSCI) and non-secure functions share same functions,
these functions can't use global data (for base address). Secure
functions have its own secure data region.
#2 is more difficult to overcome. Do you have strong opinion to get
base address from DT? Otherwise, can we use constant define for these
base addresses?

Thanks.

Regards
Ley Foon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 1/3] arm: socfpga: Convert reset manager from struct to defines

[Simon Goldschmidt]

Marek Vasut  schrieb am Mo., 26. Aug. 2019, 10:50:

> On 8/26/19 9:28 AM, Ley Foon Tan wrote:
> [...]
> >> If want to use address from DT, we need get address every time need
> to
> >> use the address.
> >> For non-DM, it is easier to use constant address. Let me know if you
> >> still prefer to use address from DT.
> 
>  Surely you can cache the address or even better, convert to DM/DT ?
> >>> Will try to cache the address for now.  But, we need get address from
> >>> DT twice, one in SPL, one in Uboot. They not sharing global data.
> >>
> >> I don't think there's much we can do about that, unless we can somehow
> >> share parsed live DT from SPL to U-Boot, but that's for another
> >> discussion/patchset/etc.
> >>
> >>> There is plan to convert clock drivers to DM. But, not in this
> patchset.
> >>
> >> Isn't Simon working on that already ?
> > Our plan is convert S10 clock driver, Simon is working on this?
>
> I think he was working on Gen5 clock driver.
>

Right. I have been working on a gen5 click driver only. It's more or less
ready to submit, but I'm still facing memory problems because now more
uclasses and drivers are active, also the SPL deb got quite big...

Regards,
Simon

>
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 1/3] arm: socfpga: Convert reset manager from struct to defines

[Marek Vasut]

On 8/26/19 9:28 AM, Ley Foon Tan wrote:
[...]
>> If want to use address from DT, we need get address every time need to
>> use the address.
>> For non-DM, it is easier to use constant address. Let me know if you
>> still prefer to use address from DT.

 Surely you can cache the address or even better, convert to DM/DT ?
>>> Will try to cache the address for now.  But, we need get address from
>>> DT twice, one in SPL, one in Uboot. They not sharing global data.
>>
>> I don't think there's much we can do about that, unless we can somehow
>> share parsed live DT from SPL to U-Boot, but that's for another
>> discussion/patchset/etc.
>>
>>> There is plan to convert clock drivers to DM. But, not in this patchset.
>>
>> Isn't Simon working on that already ?
> Our plan is convert S10 clock driver, Simon is working on this?

I think he was working on Gen5 clock driver.
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 1/3] arm: socfpga: Convert reset manager from struct to defines

[Ley Foon Tan]

On Fri, Aug 23, 2019 at 5:56 PM Marek Vasut  wrote:
>
> On 8/23/19 11:52 AM, Ley Foon Tan wrote:
> > On Fri, Aug 23, 2019 at 5:23 PM Marek Vasut  wrote:
> >>
> >> On 8/23/19 10:57 AM, Ley Foon Tan wrote:
> >>> On Wed, Aug 21, 2019 at 9:50 AM Ley Foon Tan  
> >>> wrote:
> 
>  On Tue, Aug 20, 2019 at 5:50 PM Marek Vasut  wrote:
> >
> > On 8/20/19 4:35 AM, Ley Foon Tan wrote:
> >> Convert reset manager for Gen5, Arria 10 and Stratix 10 from struct
> >> to defines.
> >> No functional change.
> >>
> >> Signed-off-by: Ley Foon Tan 
> >> ---
> >>  .../mach-socfpga/include/mach/reset_manager.h | 12 +
> >>  .../include/mach/reset_manager_arria10.h  | 41 +++-
> >>  .../include/mach/reset_manager_gen5.h | 20 +++-
> >>  .../include/mach/reset_manager_s10.h  | 33 ++---
> >>  arch/arm/mach-socfpga/misc_gen5.c |  6 +--
> >>  arch/arm/mach-socfpga/reset_manager_arria10.c | 49 +--
> >>  arch/arm/mach-socfpga/reset_manager_gen5.c| 26 +-
> >>  arch/arm/mach-socfpga/reset_manager_s10.c | 35 ++---
> >>  drivers/sysreset/sysreset_socfpga.c   |  6 +--
> >>  9 files changed, 86 insertions(+), 142 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-socfpga/include/mach/reset_manager.h 
> >> b/arch/arm/mach-socfpga/include/mach/reset_manager.h
> >> index 6ad037e325..c460e89d22 100644
> >> --- a/arch/arm/mach-socfpga/include/mach/reset_manager.h
> >> +++ b/arch/arm/mach-socfpga/include/mach/reset_manager.h
> >> @@ -6,6 +6,18 @@
> >>  #ifndef _RESET_MANAGER_H_
> >>  #define _RESET_MANAGER_H_
> >>
> >> +#define RSTMGR_READL(reg)\
> >> + readl(SOCFPGA_RSTMGR_ADDRESS + (reg))
> >> +
> >> +#define RSTMGR_WRITEL(data, reg) \
> >> + writel(data, SOCFPGA_RSTMGR_ADDRESS + (reg))
> >> +
> >> +#define RSTMGR_CLRBITS(reg, mask)\
> >> + clrbits_le32(SOCFPGA_RSTMGR_ADDRESS + (reg), mask)
> >> +
> >> +#define RSTMGR_SETBITS(reg, mask)\
> >> + setbits_le32(SOCFPGA_RSTMGR_ADDRESS + (reg), mask)
> >>
> >> btw. mask is missing parenthesis, but see below.
> >>
> >> +
> >
> > No, don't introduce such macros. Use readl()/writel()/... in the driver.
> > The address should come from DT. Besides, there is no type checking in
> > such macros.
>  These macros call to writel() and readl() underlying, it just add base
>  address to it.
> >>
> >> Right, it just makes the code hardware to read and understand.
> > Okay, will change it.
>
> It should be easy to do:
> $ sed "s@RSTMGR_SETBITS(\([^,]\+\),
> \([^)]\+\))@setbits_le32(SOCFPGA_RSTMGR_ADDRESS + \1, \1)@"
> $ git add -u ; git commit -sm part1
> $ checkpatch -f --fix --fix-inplace 
> $ git diff # review the fixes
> $ git add -u ; git commit --amend #
>
>  So, it should have type checking as call writel()/readl() directly?
> >>
> >> The functions in the macro get typechecked. But unlike function
> >> arguments, macro arguments are not checked.
> > Noted.
> >>
>  If want to use address from DT, we need get address every time need to
>  use the address.
>  For non-DM, it is easier to use constant address. Let me know if you
>  still prefer to use address from DT.
> >>
> >> Surely you can cache the address or even better, convert to DM/DT ?
> > Will try to cache the address for now.  But, we need get address from
> > DT twice, one in SPL, one in Uboot. They not sharing global data.
>
> I don't think there's much we can do about that, unless we can somehow
> share parsed live DT from SPL to U-Boot, but that's for another
> discussion/patchset/etc.
>
> > There is plan to convert clock drivers to DM. But, not in this patchset.
>
> Isn't Simon working on that already ?
Our plan is convert S10 clock driver, Simon is working on this?

Regards
Ley Foon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 1/3] arm: socfpga: Convert reset manager from struct to defines

[Marek Vasut]

On 8/23/19 11:52 AM, Ley Foon Tan wrote:
> On Fri, Aug 23, 2019 at 5:23 PM Marek Vasut  wrote:
>>
>> On 8/23/19 10:57 AM, Ley Foon Tan wrote:
>>> On Wed, Aug 21, 2019 at 9:50 AM Ley Foon Tan  wrote:

 On Tue, Aug 20, 2019 at 5:50 PM Marek Vasut  wrote:
>
> On 8/20/19 4:35 AM, Ley Foon Tan wrote:
>> Convert reset manager for Gen5, Arria 10 and Stratix 10 from struct
>> to defines.
>> No functional change.
>>
>> Signed-off-by: Ley Foon Tan 
>> ---
>>  .../mach-socfpga/include/mach/reset_manager.h | 12 +
>>  .../include/mach/reset_manager_arria10.h  | 41 +++-
>>  .../include/mach/reset_manager_gen5.h | 20 +++-
>>  .../include/mach/reset_manager_s10.h  | 33 ++---
>>  arch/arm/mach-socfpga/misc_gen5.c |  6 +--
>>  arch/arm/mach-socfpga/reset_manager_arria10.c | 49 +--
>>  arch/arm/mach-socfpga/reset_manager_gen5.c| 26 +-
>>  arch/arm/mach-socfpga/reset_manager_s10.c | 35 ++---
>>  drivers/sysreset/sysreset_socfpga.c   |  6 +--
>>  9 files changed, 86 insertions(+), 142 deletions(-)
>>
>> diff --git a/arch/arm/mach-socfpga/include/mach/reset_manager.h 
>> b/arch/arm/mach-socfpga/include/mach/reset_manager.h
>> index 6ad037e325..c460e89d22 100644
>> --- a/arch/arm/mach-socfpga/include/mach/reset_manager.h
>> +++ b/arch/arm/mach-socfpga/include/mach/reset_manager.h
>> @@ -6,6 +6,18 @@
>>  #ifndef _RESET_MANAGER_H_
>>  #define _RESET_MANAGER_H_
>>
>> +#define RSTMGR_READL(reg)\
>> + readl(SOCFPGA_RSTMGR_ADDRESS + (reg))
>> +
>> +#define RSTMGR_WRITEL(data, reg) \
>> + writel(data, SOCFPGA_RSTMGR_ADDRESS + (reg))
>> +
>> +#define RSTMGR_CLRBITS(reg, mask)\
>> + clrbits_le32(SOCFPGA_RSTMGR_ADDRESS + (reg), mask)
>> +
>> +#define RSTMGR_SETBITS(reg, mask)\
>> + setbits_le32(SOCFPGA_RSTMGR_ADDRESS + (reg), mask)
>>
>> btw. mask is missing parenthesis, but see below.
>>
>> +
>
> No, don't introduce such macros. Use readl()/writel()/... in the driver.
> The address should come from DT. Besides, there is no type checking in
> such macros.
 These macros call to writel() and readl() underlying, it just add base
 address to it.
>>
>> Right, it just makes the code hardware to read and understand.
> Okay, will change it.

It should be easy to do:
$ sed "s@RSTMGR_SETBITS(\([^,]\+\),
\([^)]\+\))@setbits_le32(SOCFPGA_RSTMGR_ADDRESS + \1, \1)@"
$ git add -u ; git commit -sm part1
$ checkpatch -f --fix --fix-inplace 
$ git diff # review the fixes
$ git add -u ; git commit --amend #

 So, it should have type checking as call writel()/readl() directly?
>>
>> The functions in the macro get typechecked. But unlike function
>> arguments, macro arguments are not checked.
> Noted.
>>
 If want to use address from DT, we need get address every time need to
 use the address.
 For non-DM, it is easier to use constant address. Let me know if you
 still prefer to use address from DT.
>>
>> Surely you can cache the address or even better, convert to DM/DT ?
> Will try to cache the address for now.  But, we need get address from
> DT twice, one in SPL, one in Uboot. They not sharing global data.

I don't think there's much we can do about that, unless we can somehow
share parsed live DT from SPL to U-Boot, but that's for another
discussion/patchset/etc.

> There is plan to convert clock drivers to DM. But, not in this patchset.

Isn't Simon working on that already ?
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 1/3] arm: socfpga: Convert reset manager from struct to defines

[Ley Foon Tan]

On Fri, Aug 23, 2019 at 5:23 PM Marek Vasut  wrote:
>
> On 8/23/19 10:57 AM, Ley Foon Tan wrote:
> > On Wed, Aug 21, 2019 at 9:50 AM Ley Foon Tan  wrote:
> >>
> >> On Tue, Aug 20, 2019 at 5:50 PM Marek Vasut  wrote:
> >>>
> >>> On 8/20/19 4:35 AM, Ley Foon Tan wrote:
>  Convert reset manager for Gen5, Arria 10 and Stratix 10 from struct
>  to defines.
>  No functional change.
> 
>  Signed-off-by: Ley Foon Tan 
>  ---
>   .../mach-socfpga/include/mach/reset_manager.h | 12 +
>   .../include/mach/reset_manager_arria10.h  | 41 +++-
>   .../include/mach/reset_manager_gen5.h | 20 +++-
>   .../include/mach/reset_manager_s10.h  | 33 ++---
>   arch/arm/mach-socfpga/misc_gen5.c |  6 +--
>   arch/arm/mach-socfpga/reset_manager_arria10.c | 49 +--
>   arch/arm/mach-socfpga/reset_manager_gen5.c| 26 +-
>   arch/arm/mach-socfpga/reset_manager_s10.c | 35 ++---
>   drivers/sysreset/sysreset_socfpga.c   |  6 +--
>   9 files changed, 86 insertions(+), 142 deletions(-)
> 
>  diff --git a/arch/arm/mach-socfpga/include/mach/reset_manager.h 
>  b/arch/arm/mach-socfpga/include/mach/reset_manager.h
>  index 6ad037e325..c460e89d22 100644
>  --- a/arch/arm/mach-socfpga/include/mach/reset_manager.h
>  +++ b/arch/arm/mach-socfpga/include/mach/reset_manager.h
>  @@ -6,6 +6,18 @@
>   #ifndef _RESET_MANAGER_H_
>   #define _RESET_MANAGER_H_
> 
>  +#define RSTMGR_READL(reg)\
>  + readl(SOCFPGA_RSTMGR_ADDRESS + (reg))
>  +
>  +#define RSTMGR_WRITEL(data, reg) \
>  + writel(data, SOCFPGA_RSTMGR_ADDRESS + (reg))
>  +
>  +#define RSTMGR_CLRBITS(reg, mask)\
>  + clrbits_le32(SOCFPGA_RSTMGR_ADDRESS + (reg), mask)
>  +
>  +#define RSTMGR_SETBITS(reg, mask)\
>  + setbits_le32(SOCFPGA_RSTMGR_ADDRESS + (reg), mask)
>
> btw. mask is missing parenthesis, but see below.
>
>  +
> >>>
> >>> No, don't introduce such macros. Use readl()/writel()/... in the driver.
> >>> The address should come from DT. Besides, there is no type checking in
> >>> such macros.
> >> These macros call to writel() and readl() underlying, it just add base
> >> address to it.
>
> Right, it just makes the code hardware to read and understand.
Okay, will change it.
>
> >> So, it should have type checking as call writel()/readl() directly?
>
> The functions in the macro get typechecked. But unlike function
> arguments, macro arguments are not checked.
Noted.
>
> >> If want to use address from DT, we need get address every time need to
> >> use the address.
> >> For non-DM, it is easier to use constant address. Let me know if you
> >> still prefer to use address from DT.
>
> Surely you can cache the address or even better, convert to DM/DT ?
Will try to cache the address for now.  But, we need get address from
DT twice, one in SPL, one in Uboot. They not sharing global data.

There is plan to convert clock drivers to DM. But, not in this patchset.

Regards
Ley Foon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 1/3] arm: socfpga: Convert reset manager from struct to defines

[Marek Vasut]

On 8/23/19 10:57 AM, Ley Foon Tan wrote:
> On Wed, Aug 21, 2019 at 9:50 AM Ley Foon Tan  wrote:
>>
>> On Tue, Aug 20, 2019 at 5:50 PM Marek Vasut  wrote:
>>>
>>> On 8/20/19 4:35 AM, Ley Foon Tan wrote:
 Convert reset manager for Gen5, Arria 10 and Stratix 10 from struct
 to defines.
 No functional change.

 Signed-off-by: Ley Foon Tan 
 ---
  .../mach-socfpga/include/mach/reset_manager.h | 12 +
  .../include/mach/reset_manager_arria10.h  | 41 +++-
  .../include/mach/reset_manager_gen5.h | 20 +++-
  .../include/mach/reset_manager_s10.h  | 33 ++---
  arch/arm/mach-socfpga/misc_gen5.c |  6 +--
  arch/arm/mach-socfpga/reset_manager_arria10.c | 49 +--
  arch/arm/mach-socfpga/reset_manager_gen5.c| 26 +-
  arch/arm/mach-socfpga/reset_manager_s10.c | 35 ++---
  drivers/sysreset/sysreset_socfpga.c   |  6 +--
  9 files changed, 86 insertions(+), 142 deletions(-)

 diff --git a/arch/arm/mach-socfpga/include/mach/reset_manager.h 
 b/arch/arm/mach-socfpga/include/mach/reset_manager.h
 index 6ad037e325..c460e89d22 100644
 --- a/arch/arm/mach-socfpga/include/mach/reset_manager.h
 +++ b/arch/arm/mach-socfpga/include/mach/reset_manager.h
 @@ -6,6 +6,18 @@
  #ifndef _RESET_MANAGER_H_
  #define _RESET_MANAGER_H_

 +#define RSTMGR_READL(reg)\
 + readl(SOCFPGA_RSTMGR_ADDRESS + (reg))
 +
 +#define RSTMGR_WRITEL(data, reg) \
 + writel(data, SOCFPGA_RSTMGR_ADDRESS + (reg))
 +
 +#define RSTMGR_CLRBITS(reg, mask)\
 + clrbits_le32(SOCFPGA_RSTMGR_ADDRESS + (reg), mask)
 +
 +#define RSTMGR_SETBITS(reg, mask)\
 + setbits_le32(SOCFPGA_RSTMGR_ADDRESS + (reg), mask)

btw. mask is missing parenthesis, but see below.

 +
>>>
>>> No, don't introduce such macros. Use readl()/writel()/... in the driver.
>>> The address should come from DT. Besides, there is no type checking in
>>> such macros.
>> These macros call to writel() and readl() underlying, it just add base
>> address to it.

Right, it just makes the code hardware to read and understand.

>> So, it should have type checking as call writel()/readl() directly?

The functions in the macro get typechecked. But unlike function
arguments, macro arguments are not checked.

>> If want to use address from DT, we need get address every time need to
>> use the address.
>> For non-DM, it is easier to use constant address. Let me know if you
>> still prefer to use address from DT.

Surely you can cache the address or even better, convert to DM/DT ?
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 1/3] arm: socfpga: Convert reset manager from struct to defines

[Ley Foon Tan]

On Wed, Aug 21, 2019 at 9:50 AM Ley Foon Tan  wrote:
>
> On Tue, Aug 20, 2019 at 5:50 PM Marek Vasut  wrote:
> >
> > On 8/20/19 4:35 AM, Ley Foon Tan wrote:
> > > Convert reset manager for Gen5, Arria 10 and Stratix 10 from struct
> > > to defines.
> > > No functional change.
> > >
> > > Signed-off-by: Ley Foon Tan 
> > > ---
> > >  .../mach-socfpga/include/mach/reset_manager.h | 12 +
> > >  .../include/mach/reset_manager_arria10.h  | 41 +++-
> > >  .../include/mach/reset_manager_gen5.h | 20 +++-
> > >  .../include/mach/reset_manager_s10.h  | 33 ++---
> > >  arch/arm/mach-socfpga/misc_gen5.c |  6 +--
> > >  arch/arm/mach-socfpga/reset_manager_arria10.c | 49 +--
> > >  arch/arm/mach-socfpga/reset_manager_gen5.c| 26 +-
> > >  arch/arm/mach-socfpga/reset_manager_s10.c | 35 ++---
> > >  drivers/sysreset/sysreset_socfpga.c   |  6 +--
> > >  9 files changed, 86 insertions(+), 142 deletions(-)
> > >
> > > diff --git a/arch/arm/mach-socfpga/include/mach/reset_manager.h 
> > > b/arch/arm/mach-socfpga/include/mach/reset_manager.h
> > > index 6ad037e325..c460e89d22 100644
> > > --- a/arch/arm/mach-socfpga/include/mach/reset_manager.h
> > > +++ b/arch/arm/mach-socfpga/include/mach/reset_manager.h
> > > @@ -6,6 +6,18 @@
> > >  #ifndef _RESET_MANAGER_H_
> > >  #define _RESET_MANAGER_H_
> > >
> > > +#define RSTMGR_READL(reg)\
> > > + readl(SOCFPGA_RSTMGR_ADDRESS + (reg))
> > > +
> > > +#define RSTMGR_WRITEL(data, reg) \
> > > + writel(data, SOCFPGA_RSTMGR_ADDRESS + (reg))
> > > +
> > > +#define RSTMGR_CLRBITS(reg, mask)\
> > > + clrbits_le32(SOCFPGA_RSTMGR_ADDRESS + (reg), mask)
> > > +
> > > +#define RSTMGR_SETBITS(reg, mask)\
> > > + setbits_le32(SOCFPGA_RSTMGR_ADDRESS + (reg), mask)
> > > +
> >
> > No, don't introduce such macros. Use readl()/writel()/... in the driver.
> > The address should come from DT. Besides, there is no type checking in
> > such macros.
> These macros call to writel() and readl() underlying, it just add base
> address to it.
> So, it should have type checking as call writel()/readl() directly?
> If want to use address from DT, we need get address every time need to
> use the address.
> For non-DM, it is easier to use constant address. Let me know if you
> still prefer to use address from DT.

Hi Marek

Do you have further comment for these?

Regards
Ley Foon

> >
> > >  void reset_cpu(ulong addr);
> > >
> > >  void socfpga_per_reset(u32 reset, int set);
> > > diff --git a/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h 
> > > b/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> > > index 6623ebee65..8b72f41498 100644
> > > --- a/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> > > +++ b/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> > > @@ -14,40 +14,13 @@ int socfpga_reset_deassert_bridges_handoff(void);
> > >  void socfpga_reset_deassert_osc1wd0(void);
> > >  int socfpga_bridges_reset(void);
> > >
> > > -struct socfpga_reset_manager {
> > > - u32 stat;
> > > - u32 ramstat;
> > > - u32 miscstat;
> > > - u32 ctrl;
> > > - u32 hdsken;
> > > - u32 hdskreq;
> > > - u32 hdskack;
> > > - u32 counts;
> > > - u32 mpumodrst;
> > > - u32 per0modrst;
> > > - u32 per1modrst;
> > > - u32 brgmodrst;
> > > - u32 sysmodrst;
> > > - u32 coldmodrst;
> > > - u32 nrstmodrst;
> > > - u32 dbgmodrst;
> > > - u32 mpuwarmmask;
> > > - u32 per0warmmask;
> > > - u32 per1warmmask;
> > > - u32 brgwarmmask;
> > > - u32 syswarmmask;
> > > - u32 nrstwarmmask;
> > > - u32 l3warmmask;
> > > - u32 tststa;
> > > - u32 tstscratch;
> > > - u32 hdsktimeout;
> > > - u32 hmcintr;
> > > - u32 hmcintren;
> > > - u32 hmcintrens;
> > > - u32 hmcintrenr;
> > > - u32 hmcgpout;
> > > - u32 hmcgpin;
> > > -};
> > > +#define RSTMGR_STATUS0
> > > +#define RSTMGR_CTRL  0xc
> > > +#define RSTMGR_MPUMODRST 0x20
> > > +#define RSTMGR_PER0MODRST0x24
> > > +#define RSTMGR_PER1MODRST0x28
> > > +#define RSTMGR_BRGMODRST 0x2c
> > > +#define RSTMGR_SYSMODRST 0x30
> >
> > It would be much better to have some SOCFPGA_ prefix here, to clearly
> > identify those macros. Also, you are missing quite a few registers.
> There are few registers same across all devices and use in their
> common functions. These common registers need to have same macro
> names.
>
> For those different registers, they will be something like these
> (macro names will be longer):
> SOCFPGA_A10_RSTMGR_MPUMODRST
> SOCFPGA_G5_RSTMGR_MPUMODRST
> SOCFPGA_S10_RSTMGR_MPUMODRST
> SOCFPGA_AGILEX_RSTMGR_MPUMODRST
>
> Regards
> Ley Foon

Re: [U-Boot] [PATCH 1/3] arm: socfpga: Convert reset manager from struct to defines

[Ley Foon Tan]

On Tue, Aug 20, 2019 at 7:24 PM Marek Vasut  wrote:
>
> On 8/20/19 12:29 PM, Simon Goldschmidt wrote:
> >
> >
> > Marek Vasut mailto:ma...@denx.de>> schrieb am Di., 20.
> > Aug. 2019, 12:15:
> >
> > On 8/20/19 11:55 AM, Simon Goldschmidt wrote:
> > >
> > >
> > > Marek Vasut mailto:ma...@denx.de>
> > >> schrieb am Di., 20.
> > > Aug. 2019, 11:50:
> > >
> > > On 8/20/19 4:35 AM, Ley Foon Tan wrote:
> > > > Convert reset manager for Gen5, Arria 10 and Stratix 10 from
> > struct
> > > > to defines.
> > > > No functional change.
> > > >
> > > > Signed-off-by: Ley Foon Tan  > 
> > > >>
> > > > ---
> > > >  .../mach-socfpga/include/mach/reset_manager.h | 12 +
> > > >  .../include/mach/reset_manager_arria10.h  | 41
> > +++-
> > > >  .../include/mach/reset_manager_gen5.h | 20 +++-
> > > >  .../include/mach/reset_manager_s10.h  | 33
> > ++---
> > > >  arch/arm/mach-socfpga/misc_gen5.c |  6 +--
> > > >  arch/arm/mach-socfpga/reset_manager_arria10.c | 49
> > > +--
> > > >  arch/arm/mach-socfpga/reset_manager_gen5.c| 26 +-
> > > >  arch/arm/mach-socfpga/reset_manager_s10.c | 35
> > ++---
> > > >  drivers/sysreset/sysreset_socfpga.c   |  6 +--
> > > >  9 files changed, 86 insertions(+), 142 deletions(-)
> > > >
> > > > diff --git a/arch/arm/mach-socfpga/include/mach/reset_manager.h
> > > b/arch/arm/mach-socfpga/include/mach/reset_manager.h
> > > > index 6ad037e325..c460e89d22 100644
> > > > --- a/arch/arm/mach-socfpga/include/mach/reset_manager.h
> > > > +++ b/arch/arm/mach-socfpga/include/mach/reset_manager.h
> > > > @@ -6,6 +6,18 @@
> > > >  #ifndef _RESET_MANAGER_H_
> > > >  #define _RESET_MANAGER_H_
> > > >
> > > > +#define RSTMGR_READL(reg)\
> > > > + readl(SOCFPGA_RSTMGR_ADDRESS + (reg))
> > > > +
> > > > +#define RSTMGR_WRITEL(data, reg) \
> > > > + writel(data, SOCFPGA_RSTMGR_ADDRESS + (reg))
> > > > +
> > > > +#define RSTMGR_CLRBITS(reg, mask)\
> > > > + clrbits_le32(SOCFPGA_RSTMGR_ADDRESS + (reg), mask)
> > > > +
> > > > +#define RSTMGR_SETBITS(reg, mask)\
> > > > + setbits_le32(SOCFPGA_RSTMGR_ADDRESS + (reg), mask)
> > > > +
> > >
> > > No, don't introduce such macros. Use readl()/writel()/... in
> > the driver.
> > > The address should come from DT. Besides, there is no type
> > checking in
> > > such macros.
> > >
> > > >  void reset_cpu(ulong addr);
> > > >
> > > >  void socfpga_per_reset(u32 reset, int set);
> > > > diff --git
> > > a/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> > > b/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> > > > index 6623ebee65..8b72f41498 100644
> > > > --- a/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> > > > +++ b/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> > > > @@ -14,40 +14,13 @@ int
> > socfpga_reset_deassert_bridges_handoff(void);
> > > >  void socfpga_reset_deassert_osc1wd0(void);
> > > >  int socfpga_bridges_reset(void);
> > > >
> > > > -struct socfpga_reset_manager {
> > > > - u32 stat;
> > > > - u32 ramstat;
> > > > - u32 miscstat;
> > > > - u32 ctrl;
> > > > - u32 hdsken;
> > > > - u32 hdskreq;
> > > > - u32 hdskack;
> > > > - u32 counts;
> > > > - u32 mpumodrst;
> > > > - u32 per0modrst;
> > > > - u32 per1modrst;
> > > > - u32 brgmodrst;
> > > > - u32 sysmodrst;
> > > > - u32 coldmodrst;
> > > > - u32 nrstmodrst;
> > > > - u32 dbgmodrst;
> > > > - u32 mpuwarmmask;
> > > > - u32 per0warmmask;
> > > > - u32 per1warmmask;
> > > > - u32 brgwarmmask;
> > > > - u32 syswarmmask;
> > > > - u32 nrstwarmmask;
> > > > - u32 l3warmmask;
> > > > - u32 tststa;
> > > > - u32 tstscratch;
> > > > - u32 hdsktimeout;
> > > > - u32 hmcintr;
> > > > - u32 hmcintren;
> > > > - u32 hmcintrens;
> > > > - u32 hmcintrenr;
> >  

Re: [U-Boot] [PATCH 1/3] arm: socfpga: Convert reset manager from struct to defines

[Ley Foon Tan]

On Tue, Aug 20, 2019 at 5:50 PM Marek Vasut  wrote:
>
> On 8/20/19 4:35 AM, Ley Foon Tan wrote:
> > Convert reset manager for Gen5, Arria 10 and Stratix 10 from struct
> > to defines.
> > No functional change.
> >
> > Signed-off-by: Ley Foon Tan 
> > ---
> >  .../mach-socfpga/include/mach/reset_manager.h | 12 +
> >  .../include/mach/reset_manager_arria10.h  | 41 +++-
> >  .../include/mach/reset_manager_gen5.h | 20 +++-
> >  .../include/mach/reset_manager_s10.h  | 33 ++---
> >  arch/arm/mach-socfpga/misc_gen5.c |  6 +--
> >  arch/arm/mach-socfpga/reset_manager_arria10.c | 49 +--
> >  arch/arm/mach-socfpga/reset_manager_gen5.c| 26 +-
> >  arch/arm/mach-socfpga/reset_manager_s10.c | 35 ++---
> >  drivers/sysreset/sysreset_socfpga.c   |  6 +--
> >  9 files changed, 86 insertions(+), 142 deletions(-)
> >
> > diff --git a/arch/arm/mach-socfpga/include/mach/reset_manager.h 
> > b/arch/arm/mach-socfpga/include/mach/reset_manager.h
> > index 6ad037e325..c460e89d22 100644
> > --- a/arch/arm/mach-socfpga/include/mach/reset_manager.h
> > +++ b/arch/arm/mach-socfpga/include/mach/reset_manager.h
> > @@ -6,6 +6,18 @@
> >  #ifndef _RESET_MANAGER_H_
> >  #define _RESET_MANAGER_H_
> >
> > +#define RSTMGR_READL(reg)\
> > + readl(SOCFPGA_RSTMGR_ADDRESS + (reg))
> > +
> > +#define RSTMGR_WRITEL(data, reg) \
> > + writel(data, SOCFPGA_RSTMGR_ADDRESS + (reg))
> > +
> > +#define RSTMGR_CLRBITS(reg, mask)\
> > + clrbits_le32(SOCFPGA_RSTMGR_ADDRESS + (reg), mask)
> > +
> > +#define RSTMGR_SETBITS(reg, mask)\
> > + setbits_le32(SOCFPGA_RSTMGR_ADDRESS + (reg), mask)
> > +
>
> No, don't introduce such macros. Use readl()/writel()/... in the driver.
> The address should come from DT. Besides, there is no type checking in
> such macros.
These macros call to writel() and readl() underlying, it just add base
address to it.
So, it should have type checking as call writel()/readl() directly?
If want to use address from DT, we need get address every time need to
use the address.
For non-DM, it is easier to use constant address. Let me know if you
still prefer to use address from DT.
>
> >  void reset_cpu(ulong addr);
> >
> >  void socfpga_per_reset(u32 reset, int set);
> > diff --git a/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h 
> > b/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> > index 6623ebee65..8b72f41498 100644
> > --- a/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> > +++ b/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> > @@ -14,40 +14,13 @@ int socfpga_reset_deassert_bridges_handoff(void);
> >  void socfpga_reset_deassert_osc1wd0(void);
> >  int socfpga_bridges_reset(void);
> >
> > -struct socfpga_reset_manager {
> > - u32 stat;
> > - u32 ramstat;
> > - u32 miscstat;
> > - u32 ctrl;
> > - u32 hdsken;
> > - u32 hdskreq;
> > - u32 hdskack;
> > - u32 counts;
> > - u32 mpumodrst;
> > - u32 per0modrst;
> > - u32 per1modrst;
> > - u32 brgmodrst;
> > - u32 sysmodrst;
> > - u32 coldmodrst;
> > - u32 nrstmodrst;
> > - u32 dbgmodrst;
> > - u32 mpuwarmmask;
> > - u32 per0warmmask;
> > - u32 per1warmmask;
> > - u32 brgwarmmask;
> > - u32 syswarmmask;
> > - u32 nrstwarmmask;
> > - u32 l3warmmask;
> > - u32 tststa;
> > - u32 tstscratch;
> > - u32 hdsktimeout;
> > - u32 hmcintr;
> > - u32 hmcintren;
> > - u32 hmcintrens;
> > - u32 hmcintrenr;
> > - u32 hmcgpout;
> > - u32 hmcgpin;
> > -};
> > +#define RSTMGR_STATUS0
> > +#define RSTMGR_CTRL  0xc
> > +#define RSTMGR_MPUMODRST 0x20
> > +#define RSTMGR_PER0MODRST0x24
> > +#define RSTMGR_PER1MODRST0x28
> > +#define RSTMGR_BRGMODRST 0x2c
> > +#define RSTMGR_SYSMODRST 0x30
>
> It would be much better to have some SOCFPGA_ prefix here, to clearly
> identify those macros. Also, you are missing quite a few registers.
There are few registers same across all devices and use in their
common functions. These common registers need to have same macro
names.

For those different registers, they will be something like these
(macro names will be longer):
SOCFPGA_A10_RSTMGR_MPUMODRST
SOCFPGA_G5_RSTMGR_MPUMODRST
SOCFPGA_S10_RSTMGR_MPUMODRST
SOCFPGA_AGILEX_RSTMGR_MPUMODRST

Regards
Ley Foon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 1/3] arm: socfpga: Convert reset manager from struct to defines

[Marek Vasut]

On 8/20/19 12:29 PM, Simon Goldschmidt wrote:
> 
> 
> Marek Vasut mailto:ma...@denx.de>> schrieb am Di., 20.
> Aug. 2019, 12:15:
> 
> On 8/20/19 11:55 AM, Simon Goldschmidt wrote:
> >
> >
> > Marek Vasut mailto:ma...@denx.de>
> >> schrieb am Di., 20.
> > Aug. 2019, 11:50:
> >
> >     On 8/20/19 4:35 AM, Ley Foon Tan wrote:
> >     > Convert reset manager for Gen5, Arria 10 and Stratix 10 from
> struct
> >     > to defines.
> >     > No functional change.
> >     >
> >     > Signed-off-by: Ley Foon Tan  
> >     >>
> >     > ---
> >     >  .../mach-socfpga/include/mach/reset_manager.h | 12 +
> >     >  .../include/mach/reset_manager_arria10.h      | 41
> +++-
> >     >  .../include/mach/reset_manager_gen5.h         | 20 +++-
> >     >  .../include/mach/reset_manager_s10.h          | 33
> ++---
> >     >  arch/arm/mach-socfpga/misc_gen5.c             |  6 +--
> >     >  arch/arm/mach-socfpga/reset_manager_arria10.c | 49
> >     +--
> >     >  arch/arm/mach-socfpga/reset_manager_gen5.c    | 26 +-
> >     >  arch/arm/mach-socfpga/reset_manager_s10.c     | 35
> ++---
> >     >  drivers/sysreset/sysreset_socfpga.c           |  6 +--
> >     >  9 files changed, 86 insertions(+), 142 deletions(-)
> >     >
> >     > diff --git a/arch/arm/mach-socfpga/include/mach/reset_manager.h
> >     b/arch/arm/mach-socfpga/include/mach/reset_manager.h
> >     > index 6ad037e325..c460e89d22 100644
> >     > --- a/arch/arm/mach-socfpga/include/mach/reset_manager.h
> >     > +++ b/arch/arm/mach-socfpga/include/mach/reset_manager.h
> >     > @@ -6,6 +6,18 @@
> >     >  #ifndef _RESET_MANAGER_H_
> >     >  #define _RESET_MANAGER_H_
> >     > 
> >     > +#define RSTMGR_READL(reg)                    \
> >     > +     readl(SOCFPGA_RSTMGR_ADDRESS + (reg))
> >     > +
> >     > +#define RSTMGR_WRITEL(data, reg)             \
> >     > +     writel(data, SOCFPGA_RSTMGR_ADDRESS + (reg))
> >     > +
> >     > +#define RSTMGR_CLRBITS(reg, mask)            \
> >     > +     clrbits_le32(SOCFPGA_RSTMGR_ADDRESS + (reg), mask)
> >     > +
> >     > +#define RSTMGR_SETBITS(reg, mask)            \
> >     > +     setbits_le32(SOCFPGA_RSTMGR_ADDRESS + (reg), mask)
> >     > +
> >
> >     No, don't introduce such macros. Use readl()/writel()/... in
> the driver.
> >     The address should come from DT. Besides, there is no type
> checking in
> >     such macros.
> >
> >     >  void reset_cpu(ulong addr);
> >     > 
> >     >  void socfpga_per_reset(u32 reset, int set);
> >     > diff --git
> >     a/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> >     b/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> >     > index 6623ebee65..8b72f41498 100644
> >     > --- a/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> >     > +++ b/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> >     > @@ -14,40 +14,13 @@ int
> socfpga_reset_deassert_bridges_handoff(void);
> >     >  void socfpga_reset_deassert_osc1wd0(void);
> >     >  int socfpga_bridges_reset(void);
> >     > 
> >     > -struct socfpga_reset_manager {
> >     > -     u32     stat;
> >     > -     u32     ramstat;
> >     > -     u32     miscstat;
> >     > -     u32     ctrl;
> >     > -     u32     hdsken;
> >     > -     u32     hdskreq;
> >     > -     u32     hdskack;
> >     > -     u32     counts;
> >     > -     u32     mpumodrst;
> >     > -     u32     per0modrst;
> >     > -     u32     per1modrst;
> >     > -     u32     brgmodrst;
> >     > -     u32     sysmodrst;
> >     > -     u32     coldmodrst;
> >     > -     u32     nrstmodrst;
> >     > -     u32     dbgmodrst;
> >     > -     u32     mpuwarmmask;
> >     > -     u32     per0warmmask;
> >     > -     u32     per1warmmask;
> >     > -     u32     brgwarmmask;
> >     > -     u32     syswarmmask;
> >     > -     u32     nrstwarmmask;
> >     > -     u32     l3warmmask;
> >     > -     u32     tststa;
> >     > -     u32     tstscratch;
> >     > -     u32     hdsktimeout;
> >     > -     u32     hmcintr;
> >     > -     u32     hmcintren;
> >     > -     u32     hmcintrens;
> >     > -     u32     hmcintrenr;
> >     > -     u32     hmcgpout;
> >     > -     u32     hmcgpin;
> >     > -};
> >     > +#define RSTMGR_STATUS                0
> >     > +#define RSTMGR_CTRL          0xc
> >     > +#define RSTMGR_MPUMODRST     0x20
> >     > +#define 

Re: [U-Boot] [PATCH 1/3] arm: socfpga: Convert reset manager from struct to defines

[Simon Goldschmidt]

Marek Vasut  schrieb am Di., 20. Aug. 2019, 12:15:

> On 8/20/19 11:55 AM, Simon Goldschmidt wrote:
> >
> >
> > Marek Vasut mailto:ma...@denx.de>> schrieb am Di., 20.
> > Aug. 2019, 11:50:
> >
> > On 8/20/19 4:35 AM, Ley Foon Tan wrote:
> > > Convert reset manager for Gen5, Arria 10 and Stratix 10 from struct
> > > to defines.
> > > No functional change.
> > >
> > > Signed-off-by: Ley Foon Tan  > >
> > > ---
> > >  .../mach-socfpga/include/mach/reset_manager.h | 12 +
> > >  .../include/mach/reset_manager_arria10.h  | 41
> +++-
> > >  .../include/mach/reset_manager_gen5.h | 20 +++-
> > >  .../include/mach/reset_manager_s10.h  | 33 ++---
> > >  arch/arm/mach-socfpga/misc_gen5.c |  6 +--
> > >  arch/arm/mach-socfpga/reset_manager_arria10.c | 49
> > +--
> > >  arch/arm/mach-socfpga/reset_manager_gen5.c| 26 +-
> > >  arch/arm/mach-socfpga/reset_manager_s10.c | 35 ++---
> > >  drivers/sysreset/sysreset_socfpga.c   |  6 +--
> > >  9 files changed, 86 insertions(+), 142 deletions(-)
> > >
> > > diff --git a/arch/arm/mach-socfpga/include/mach/reset_manager.h
> > b/arch/arm/mach-socfpga/include/mach/reset_manager.h
> > > index 6ad037e325..c460e89d22 100644
> > > --- a/arch/arm/mach-socfpga/include/mach/reset_manager.h
> > > +++ b/arch/arm/mach-socfpga/include/mach/reset_manager.h
> > > @@ -6,6 +6,18 @@
> > >  #ifndef _RESET_MANAGER_H_
> > >  #define _RESET_MANAGER_H_
> > >
> > > +#define RSTMGR_READL(reg)\
> > > + readl(SOCFPGA_RSTMGR_ADDRESS + (reg))
> > > +
> > > +#define RSTMGR_WRITEL(data, reg) \
> > > + writel(data, SOCFPGA_RSTMGR_ADDRESS + (reg))
> > > +
> > > +#define RSTMGR_CLRBITS(reg, mask)\
> > > + clrbits_le32(SOCFPGA_RSTMGR_ADDRESS + (reg), mask)
> > > +
> > > +#define RSTMGR_SETBITS(reg, mask)\
> > > + setbits_le32(SOCFPGA_RSTMGR_ADDRESS + (reg), mask)
> > > +
> >
> > No, don't introduce such macros. Use readl()/writel()/... in the
> driver.
> > The address should come from DT. Besides, there is no type checking
> in
> > such macros.
> >
> > >  void reset_cpu(ulong addr);
> > >
> > >  void socfpga_per_reset(u32 reset, int set);
> > > diff --git
> > a/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> > b/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> > > index 6623ebee65..8b72f41498 100644
> > > --- a/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> > > +++ b/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> > > @@ -14,40 +14,13 @@ int
> socfpga_reset_deassert_bridges_handoff(void);
> > >  void socfpga_reset_deassert_osc1wd0(void);
> > >  int socfpga_bridges_reset(void);
> > >
> > > -struct socfpga_reset_manager {
> > > - u32 stat;
> > > - u32 ramstat;
> > > - u32 miscstat;
> > > - u32 ctrl;
> > > - u32 hdsken;
> > > - u32 hdskreq;
> > > - u32 hdskack;
> > > - u32 counts;
> > > - u32 mpumodrst;
> > > - u32 per0modrst;
> > > - u32 per1modrst;
> > > - u32 brgmodrst;
> > > - u32 sysmodrst;
> > > - u32 coldmodrst;
> > > - u32 nrstmodrst;
> > > - u32 dbgmodrst;
> > > - u32 mpuwarmmask;
> > > - u32 per0warmmask;
> > > - u32 per1warmmask;
> > > - u32 brgwarmmask;
> > > - u32 syswarmmask;
> > > - u32 nrstwarmmask;
> > > - u32 l3warmmask;
> > > - u32 tststa;
> > > - u32 tstscratch;
> > > - u32 hdsktimeout;
> > > - u32 hmcintr;
> > > - u32 hmcintren;
> > > - u32 hmcintrens;
> > > - u32 hmcintrenr;
> > > - u32 hmcgpout;
> > > - u32 hmcgpin;
> > > -};
> > > +#define RSTMGR_STATUS0
> > > +#define RSTMGR_CTRL  0xc
> > > +#define RSTMGR_MPUMODRST 0x20
> > > +#define RSTMGR_PER0MODRST0x24
> > > +#define RSTMGR_PER1MODRST0x28
> > > +#define RSTMGR_BRGMODRST 0x2c
> > > +#define RSTMGR_SYSMODRST 0x30
> >
> > It would be much better to have some SOCFPGA_ prefix here, to clearly
> > identify those macros. Also, you are missing quite a few registers.
> >
> >
> > I agree on the prefix, but I thought leaving away unused registers was
> > one of the positive sides of this change?
> >
> > That also allows using the same defines for S10 and Agilex even if some
> > registers are different. That's how we came to this patchset, if I
> > remember correctly.
>
> What happens if you want to use 

Re: [U-Boot] [PATCH 1/3] arm: socfpga: Convert reset manager from struct to defines

[Marek Vasut]

On 8/20/19 11:55 AM, Simon Goldschmidt wrote:
> 
> 
> Marek Vasut mailto:ma...@denx.de>> schrieb am Di., 20.
> Aug. 2019, 11:50:
> 
> On 8/20/19 4:35 AM, Ley Foon Tan wrote:
> > Convert reset manager for Gen5, Arria 10 and Stratix 10 from struct
> > to defines.
> > No functional change.
> >
> > Signed-off-by: Ley Foon Tan  >
> > ---
> >  .../mach-socfpga/include/mach/reset_manager.h | 12 +
> >  .../include/mach/reset_manager_arria10.h      | 41 +++-
> >  .../include/mach/reset_manager_gen5.h         | 20 +++-
> >  .../include/mach/reset_manager_s10.h          | 33 ++---
> >  arch/arm/mach-socfpga/misc_gen5.c             |  6 +--
> >  arch/arm/mach-socfpga/reset_manager_arria10.c | 49
> +--
> >  arch/arm/mach-socfpga/reset_manager_gen5.c    | 26 +-
> >  arch/arm/mach-socfpga/reset_manager_s10.c     | 35 ++---
> >  drivers/sysreset/sysreset_socfpga.c           |  6 +--
> >  9 files changed, 86 insertions(+), 142 deletions(-)
> >
> > diff --git a/arch/arm/mach-socfpga/include/mach/reset_manager.h
> b/arch/arm/mach-socfpga/include/mach/reset_manager.h
> > index 6ad037e325..c460e89d22 100644
> > --- a/arch/arm/mach-socfpga/include/mach/reset_manager.h
> > +++ b/arch/arm/mach-socfpga/include/mach/reset_manager.h
> > @@ -6,6 +6,18 @@
> >  #ifndef _RESET_MANAGER_H_
> >  #define _RESET_MANAGER_H_
> > 
> > +#define RSTMGR_READL(reg)                    \
> > +     readl(SOCFPGA_RSTMGR_ADDRESS + (reg))
> > +
> > +#define RSTMGR_WRITEL(data, reg)             \
> > +     writel(data, SOCFPGA_RSTMGR_ADDRESS + (reg))
> > +
> > +#define RSTMGR_CLRBITS(reg, mask)            \
> > +     clrbits_le32(SOCFPGA_RSTMGR_ADDRESS + (reg), mask)
> > +
> > +#define RSTMGR_SETBITS(reg, mask)            \
> > +     setbits_le32(SOCFPGA_RSTMGR_ADDRESS + (reg), mask)
> > +
> 
> No, don't introduce such macros. Use readl()/writel()/... in the driver.
> The address should come from DT. Besides, there is no type checking in
> such macros.
> 
> >  void reset_cpu(ulong addr);
> > 
> >  void socfpga_per_reset(u32 reset, int set);
> > diff --git
> a/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> b/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> > index 6623ebee65..8b72f41498 100644
> > --- a/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> > +++ b/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> > @@ -14,40 +14,13 @@ int socfpga_reset_deassert_bridges_handoff(void);
> >  void socfpga_reset_deassert_osc1wd0(void);
> >  int socfpga_bridges_reset(void);
> > 
> > -struct socfpga_reset_manager {
> > -     u32     stat;
> > -     u32     ramstat;
> > -     u32     miscstat;
> > -     u32     ctrl;
> > -     u32     hdsken;
> > -     u32     hdskreq;
> > -     u32     hdskack;
> > -     u32     counts;
> > -     u32     mpumodrst;
> > -     u32     per0modrst;
> > -     u32     per1modrst;
> > -     u32     brgmodrst;
> > -     u32     sysmodrst;
> > -     u32     coldmodrst;
> > -     u32     nrstmodrst;
> > -     u32     dbgmodrst;
> > -     u32     mpuwarmmask;
> > -     u32     per0warmmask;
> > -     u32     per1warmmask;
> > -     u32     brgwarmmask;
> > -     u32     syswarmmask;
> > -     u32     nrstwarmmask;
> > -     u32     l3warmmask;
> > -     u32     tststa;
> > -     u32     tstscratch;
> > -     u32     hdsktimeout;
> > -     u32     hmcintr;
> > -     u32     hmcintren;
> > -     u32     hmcintrens;
> > -     u32     hmcintrenr;
> > -     u32     hmcgpout;
> > -     u32     hmcgpin;
> > -};
> > +#define RSTMGR_STATUS                0
> > +#define RSTMGR_CTRL          0xc
> > +#define RSTMGR_MPUMODRST     0x20
> > +#define RSTMGR_PER0MODRST    0x24
> > +#define RSTMGR_PER1MODRST    0x28
> > +#define RSTMGR_BRGMODRST     0x2c
> > +#define RSTMGR_SYSMODRST     0x30
> 
> It would be much better to have some SOCFPGA_ prefix here, to clearly
> identify those macros. Also, you are missing quite a few registers.
> 
> 
> I agree on the prefix, but I thought leaving away unused registers was
> one of the positive sides of this change?
> 
> That also allows using the same defines for S10 and Agilex even if some
> registers are different. That's how we came to this patchset, if I
> remember correctly.

What happens if you want to use that register then ? I guess you can add
it to the list, although it feels a bit strange. But maybe that's OK.
The different registers should then probably have some SOCFPGA__
prefix or something.
___
U-Boot mailing list

Re: [U-Boot] [PATCH 1/3] arm: socfpga: Convert reset manager from struct to defines

[Simon Goldschmidt]

Marek Vasut  schrieb am Di., 20. Aug. 2019, 11:50:

> On 8/20/19 4:35 AM, Ley Foon Tan wrote:
> > Convert reset manager for Gen5, Arria 10 and Stratix 10 from struct
> > to defines.
> > No functional change.
> >
> > Signed-off-by: Ley Foon Tan 
> > ---
> >  .../mach-socfpga/include/mach/reset_manager.h | 12 +
> >  .../include/mach/reset_manager_arria10.h  | 41 +++-
> >  .../include/mach/reset_manager_gen5.h | 20 +++-
> >  .../include/mach/reset_manager_s10.h  | 33 ++---
> >  arch/arm/mach-socfpga/misc_gen5.c |  6 +--
> >  arch/arm/mach-socfpga/reset_manager_arria10.c | 49 +--
> >  arch/arm/mach-socfpga/reset_manager_gen5.c| 26 +-
> >  arch/arm/mach-socfpga/reset_manager_s10.c | 35 ++---
> >  drivers/sysreset/sysreset_socfpga.c   |  6 +--
> >  9 files changed, 86 insertions(+), 142 deletions(-)
> >
> > diff --git a/arch/arm/mach-socfpga/include/mach/reset_manager.h
> b/arch/arm/mach-socfpga/include/mach/reset_manager.h
> > index 6ad037e325..c460e89d22 100644
> > --- a/arch/arm/mach-socfpga/include/mach/reset_manager.h
> > +++ b/arch/arm/mach-socfpga/include/mach/reset_manager.h
> > @@ -6,6 +6,18 @@
> >  #ifndef _RESET_MANAGER_H_
> >  #define _RESET_MANAGER_H_
> >
> > +#define RSTMGR_READL(reg)\
> > + readl(SOCFPGA_RSTMGR_ADDRESS + (reg))
> > +
> > +#define RSTMGR_WRITEL(data, reg) \
> > + writel(data, SOCFPGA_RSTMGR_ADDRESS + (reg))
> > +
> > +#define RSTMGR_CLRBITS(reg, mask)\
> > + clrbits_le32(SOCFPGA_RSTMGR_ADDRESS + (reg), mask)
> > +
> > +#define RSTMGR_SETBITS(reg, mask)\
> > + setbits_le32(SOCFPGA_RSTMGR_ADDRESS + (reg), mask)
> > +
>
> No, don't introduce such macros. Use readl()/writel()/... in the driver.
> The address should come from DT. Besides, there is no type checking in
> such macros.
>
> >  void reset_cpu(ulong addr);
> >
> >  void socfpga_per_reset(u32 reset, int set);
> > diff --git a/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> b/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> > index 6623ebee65..8b72f41498 100644
> > --- a/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> > +++ b/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> > @@ -14,40 +14,13 @@ int socfpga_reset_deassert_bridges_handoff(void);
> >  void socfpga_reset_deassert_osc1wd0(void);
> >  int socfpga_bridges_reset(void);
> >
> > -struct socfpga_reset_manager {
> > - u32 stat;
> > - u32 ramstat;
> > - u32 miscstat;
> > - u32 ctrl;
> > - u32 hdsken;
> > - u32 hdskreq;
> > - u32 hdskack;
> > - u32 counts;
> > - u32 mpumodrst;
> > - u32 per0modrst;
> > - u32 per1modrst;
> > - u32 brgmodrst;
> > - u32 sysmodrst;
> > - u32 coldmodrst;
> > - u32 nrstmodrst;
> > - u32 dbgmodrst;
> > - u32 mpuwarmmask;
> > - u32 per0warmmask;
> > - u32 per1warmmask;
> > - u32 brgwarmmask;
> > - u32 syswarmmask;
> > - u32 nrstwarmmask;
> > - u32 l3warmmask;
> > - u32 tststa;
> > - u32 tstscratch;
> > - u32 hdsktimeout;
> > - u32 hmcintr;
> > - u32 hmcintren;
> > - u32 hmcintrens;
> > - u32 hmcintrenr;
> > - u32 hmcgpout;
> > - u32 hmcgpin;
> > -};
> > +#define RSTMGR_STATUS0
> > +#define RSTMGR_CTRL  0xc
> > +#define RSTMGR_MPUMODRST 0x20
> > +#define RSTMGR_PER0MODRST0x24
> > +#define RSTMGR_PER1MODRST0x28
> > +#define RSTMGR_BRGMODRST 0x2c
> > +#define RSTMGR_SYSMODRST 0x30
>
> It would be much better to have some SOCFPGA_ prefix here, to clearly
> identify those macros. Also, you are missing quite a few registers.
>

I agree on the prefix, but I thought leaving away unused registers was one
of the positive sides of this change?

That also allows using the same defines for S10 and Agilex even if some
registers are different. That's how we came to this patchset, if I remember
correctly.

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 1/3] arm: socfpga: Convert reset manager from struct to defines

[Marek Vasut]

On 8/20/19 4:35 AM, Ley Foon Tan wrote:
> Convert reset manager for Gen5, Arria 10 and Stratix 10 from struct
> to defines.
> No functional change.
> 
> Signed-off-by: Ley Foon Tan 
> ---
>  .../mach-socfpga/include/mach/reset_manager.h | 12 +
>  .../include/mach/reset_manager_arria10.h  | 41 +++-
>  .../include/mach/reset_manager_gen5.h | 20 +++-
>  .../include/mach/reset_manager_s10.h  | 33 ++---
>  arch/arm/mach-socfpga/misc_gen5.c |  6 +--
>  arch/arm/mach-socfpga/reset_manager_arria10.c | 49 +--
>  arch/arm/mach-socfpga/reset_manager_gen5.c| 26 +-
>  arch/arm/mach-socfpga/reset_manager_s10.c | 35 ++---
>  drivers/sysreset/sysreset_socfpga.c   |  6 +--
>  9 files changed, 86 insertions(+), 142 deletions(-)
> 
> diff --git a/arch/arm/mach-socfpga/include/mach/reset_manager.h 
> b/arch/arm/mach-socfpga/include/mach/reset_manager.h
> index 6ad037e325..c460e89d22 100644
> --- a/arch/arm/mach-socfpga/include/mach/reset_manager.h
> +++ b/arch/arm/mach-socfpga/include/mach/reset_manager.h
> @@ -6,6 +6,18 @@
>  #ifndef _RESET_MANAGER_H_
>  #define _RESET_MANAGER_H_
>  
> +#define RSTMGR_READL(reg)\
> + readl(SOCFPGA_RSTMGR_ADDRESS + (reg))
> +
> +#define RSTMGR_WRITEL(data, reg) \
> + writel(data, SOCFPGA_RSTMGR_ADDRESS + (reg))
> +
> +#define RSTMGR_CLRBITS(reg, mask)\
> + clrbits_le32(SOCFPGA_RSTMGR_ADDRESS + (reg), mask)
> +
> +#define RSTMGR_SETBITS(reg, mask)\
> + setbits_le32(SOCFPGA_RSTMGR_ADDRESS + (reg), mask)
> +

No, don't introduce such macros. Use readl()/writel()/... in the driver.
The address should come from DT. Besides, there is no type checking in
such macros.

>  void reset_cpu(ulong addr);
>  
>  void socfpga_per_reset(u32 reset, int set);
> diff --git a/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h 
> b/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> index 6623ebee65..8b72f41498 100644
> --- a/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> +++ b/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h
> @@ -14,40 +14,13 @@ int socfpga_reset_deassert_bridges_handoff(void);
>  void socfpga_reset_deassert_osc1wd0(void);
>  int socfpga_bridges_reset(void);
>  
> -struct socfpga_reset_manager {
> - u32 stat;
> - u32 ramstat;
> - u32 miscstat;
> - u32 ctrl;
> - u32 hdsken;
> - u32 hdskreq;
> - u32 hdskack;
> - u32 counts;
> - u32 mpumodrst;
> - u32 per0modrst;
> - u32 per1modrst;
> - u32 brgmodrst;
> - u32 sysmodrst;
> - u32 coldmodrst;
> - u32 nrstmodrst;
> - u32 dbgmodrst;
> - u32 mpuwarmmask;
> - u32 per0warmmask;
> - u32 per1warmmask;
> - u32 brgwarmmask;
> - u32 syswarmmask;
> - u32 nrstwarmmask;
> - u32 l3warmmask;
> - u32 tststa;
> - u32 tstscratch;
> - u32 hdsktimeout;
> - u32 hmcintr;
> - u32 hmcintren;
> - u32 hmcintrens;
> - u32 hmcintrenr;
> - u32 hmcgpout;
> - u32 hmcgpin;
> -};
> +#define RSTMGR_STATUS0
> +#define RSTMGR_CTRL  0xc
> +#define RSTMGR_MPUMODRST 0x20
> +#define RSTMGR_PER0MODRST0x24
> +#define RSTMGR_PER1MODRST0x28
> +#define RSTMGR_BRGMODRST 0x2c
> +#define RSTMGR_SYSMODRST 0x30

It would be much better to have some SOCFPGA_ prefix here, to clearly
identify those macros. Also, you are missing quite a few registers.

[...]
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot