Re: [U-Boot] [PATCH 0/6] Add ARMv8 PSCI framework

[Hongbo Zhang]

Thanks Tom.
I am sending out an updated v2 soon, with the related configs updated.

On Fri, Oct 28, 2016 at 9:30 PM, Tom Rini  wrote:
> On Wed, Sep 28, 2016 at 03:16:38PM +0800, Hongbo Zhang wrote:
>> On Wed, Sep 28, 2016 at 1:23 AM, Tom Rini  wrote:
>> > On Tue, Sep 27, 2016 at 05:29:00PM +0800, macro.wav...@gmail.com wrote:
>> >> From: Hongbo Zhang 
>> >>
>> >> This patch set introduces ARMv8 PSCI framework, all the PSCI functions are
>> >> implemented a default dummy one, it is up to each platform to implement 
>> >> their
>> >> own specific ones.
>> >>
>> >> The first 1/6 patch is a prepare clean up for adding ARMv8 PSCI.
>> >> Patches 2/6 to 5/6 introduce new ARMv8 framework and set it up.
>> >> The last 6/6 adds a most simple implementation on NXP LS1043 platform, to
>> >> verify this framework.
>> >>
>> >> This patch set mainly introduces ARMv8 PSCI framework, for easier review 
>> >> and
>> >> merge, further PSCI implementation on LS1043 is coming later.
>> >>
>> >> Hongbo Zhang (6):
>> >>   ARMv8: LS1043A: change macro CONFIG_ARMV8_PSCI definition
>> >>   ARMv8: Add secure sections for PSCI text and data
>> >>   ARMv8: Add basic PSCI framework
>> >>   ARMv8: Setup PSCI memory and dt
>> >>   ARMv8: Enable SMC instruction
>> >>   ARMv8: LS1043A: Enable LS1043A default PSCI support
>> >
>> > Conceptually this is good.  I have some issues around order of the
>> > patches, and where the Kconfig entries end up.  Looking over the series
>> > we introduce usage of some CONFIG symbols prior to declaring them in
>> > Kconfig.  This is more of a hard no now as it will break bisecting when
>> > the test for no new CONFIG symbols is tripped.  The other problem is
>> > that I think the symbols you're adding in
>> > board/freescale/ls1043ardb/Kconfig need to be in
>> > arch/arm/cpu/armv8/Kconfig and then use default ... if ... to give the
>> > right address for the layerscape boards.
>>
>> Thanks Tom for quick response.
>>
>> For config options introduced:
>> CONFIG_ARMV8_PSCI
>> CONFIG_ARMV8_PSCI_NR_CPUS
>> CONFIG_CPU_PER_CLUSTER
>> CONFIG_ARMV8_SECURE_BASE
>>
>> I've tested adding patch one by one, there is no problem with the
>> check-config script.
>
> OK.
>
>> And my idea was like this: let the CONFIG_ARMV8_PSCI to be an overall
>> switch, and if it is enabled even without the other three ones, the
>> default PSCI still works, as I've tested, this really works because
>> any of the other three macros, when used, there is a #ifdef to check
>> if it exists, if no, a default value is used or it isn't used at all.
>> The later three macros, because they are platform specific so I
>> intended to let every platform to define them.
>>
>> This is slightly different from ARMv7, plan was if this get accepted,
>> I would like to send patch to update ARMv7's.
>
> I think that at the end of the day we need to have less options be
> defined and asked under board/ and make more and in some cases better
> use of the common Kconfig files.  Looking at how things are done in the
> Linux Kernel, in general, can be instructive here.  Maybe the right
> answer here is to have CONFIG_ARCH_WANT_GENERIC_PSCI_... with default y
> (if most cases would be the generic one) and in the negative use the
> other option which is board specific values.
>
> But re-reading patch 6/6, I'm still not convinced that we shouldn't
> start out with these being all in arch/arm/cpu/armv8/Kconfig, under the
> PSCI option, for everyone, and default ... if layerscape.  And that
> reminds that I wonder if we shouldn't have some higher level option to
> say "I am ARMv8 Layerscape" to cover the cases where today we test vs a
> number of TARGET_LS choices.  Thanks!
>
> --
> Tom
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 0/6] Add ARMv8 PSCI framework

[Tom Rini]

On Wed, Sep 28, 2016 at 03:16:38PM +0800, Hongbo Zhang wrote:
> On Wed, Sep 28, 2016 at 1:23 AM, Tom Rini  wrote:
> > On Tue, Sep 27, 2016 at 05:29:00PM +0800, macro.wav...@gmail.com wrote:
> >> From: Hongbo Zhang 
> >>
> >> This patch set introduces ARMv8 PSCI framework, all the PSCI functions are
> >> implemented a default dummy one, it is up to each platform to implement 
> >> their
> >> own specific ones.
> >>
> >> The first 1/6 patch is a prepare clean up for adding ARMv8 PSCI.
> >> Patches 2/6 to 5/6 introduce new ARMv8 framework and set it up.
> >> The last 6/6 adds a most simple implementation on NXP LS1043 platform, to
> >> verify this framework.
> >>
> >> This patch set mainly introduces ARMv8 PSCI framework, for easier review 
> >> and
> >> merge, further PSCI implementation on LS1043 is coming later.
> >>
> >> Hongbo Zhang (6):
> >>   ARMv8: LS1043A: change macro CONFIG_ARMV8_PSCI definition
> >>   ARMv8: Add secure sections for PSCI text and data
> >>   ARMv8: Add basic PSCI framework
> >>   ARMv8: Setup PSCI memory and dt
> >>   ARMv8: Enable SMC instruction
> >>   ARMv8: LS1043A: Enable LS1043A default PSCI support
> >
> > Conceptually this is good.  I have some issues around order of the
> > patches, and where the Kconfig entries end up.  Looking over the series
> > we introduce usage of some CONFIG symbols prior to declaring them in
> > Kconfig.  This is more of a hard no now as it will break bisecting when
> > the test for no new CONFIG symbols is tripped.  The other problem is
> > that I think the symbols you're adding in
> > board/freescale/ls1043ardb/Kconfig need to be in
> > arch/arm/cpu/armv8/Kconfig and then use default ... if ... to give the
> > right address for the layerscape boards.
> 
> Thanks Tom for quick response.
> 
> For config options introduced:
> CONFIG_ARMV8_PSCI
> CONFIG_ARMV8_PSCI_NR_CPUS
> CONFIG_CPU_PER_CLUSTER
> CONFIG_ARMV8_SECURE_BASE
> 
> I've tested adding patch one by one, there is no problem with the
> check-config script.

OK.

> And my idea was like this: let the CONFIG_ARMV8_PSCI to be an overall
> switch, and if it is enabled even without the other three ones, the
> default PSCI still works, as I've tested, this really works because
> any of the other three macros, when used, there is a #ifdef to check
> if it exists, if no, a default value is used or it isn't used at all.
> The later three macros, because they are platform specific so I
> intended to let every platform to define them.
> 
> This is slightly different from ARMv7, plan was if this get accepted,
> I would like to send patch to update ARMv7's.

I think that at the end of the day we need to have less options be
defined and asked under board/ and make more and in some cases better
use of the common Kconfig files.  Looking at how things are done in the
Linux Kernel, in general, can be instructive here.  Maybe the right
answer here is to have CONFIG_ARCH_WANT_GENERIC_PSCI_... with default y
(if most cases would be the generic one) and in the negative use the
other option which is board specific values.

But re-reading patch 6/6, I'm still not convinced that we shouldn't
start out with these being all in arch/arm/cpu/armv8/Kconfig, under the
PSCI option, for everyone, and default ... if layerscape.  And that
reminds that I wonder if we shouldn't have some higher level option to
say "I am ARMv8 Layerscape" to cover the cases where today we test vs a
number of TARGET_LS choices.  Thanks!

-- 
Tom


signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 0/6] Add ARMv8 PSCI framework

[Hongbo Zhang]

York,
Yes I am going to send an update, one patch needs to be reordered as
I've mentioned.
I didn't get other feedback.
But one thing I'm wondering is how to introduce the CONFIG_ options
well, Tom's concerns make sense, and I had my explain too, we need a
final solution before I send out a v2.

On Thu, Oct 27, 2016 at 2:17 AM, york sun  wrote:
> On 10/18/2016 12:18 AM, Hongbo Zhang wrote:
>> Ping all,
>> Some time ago I saw several people mentioned ARMv8 PSCI, are you
>> interested in leaving your review comments?
>> I know it is not so easy for reviewing assembly language patches, but
>> code structures and assembly instructions in this patch set are not
>> complicated.
>> When this common framework gets merged, it will be possible for each
>> platform to implement their platform PSCI functions in C too.
>>
>> Hi Tom,
>> Any further concerns? comments?
>>
>> Thanks all.
>
> Hongbo,
>
> This set has been marked as "changes requested" in patchwork. Did you
> get feedback other than those captured by patchwork? Are you going to
> send an update?
>
> York
>
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 0/6] Add ARMv8 PSCI framework

[york sun]

On 10/18/2016 12:18 AM, Hongbo Zhang wrote:
> Ping all,
> Some time ago I saw several people mentioned ARMv8 PSCI, are you
> interested in leaving your review comments?
> I know it is not so easy for reviewing assembly language patches, but
> code structures and assembly instructions in this patch set are not
> complicated.
> When this common framework gets merged, it will be possible for each
> platform to implement their platform PSCI functions in C too.
>
> Hi Tom,
> Any further concerns? comments?
>
> Thanks all.

Hongbo,

This set has been marked as "changes requested" in patchwork. Did you 
get feedback other than those captured by patchwork? Are you going to 
send an update?

York

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

Re: [U-Boot] [PATCH 0/6] Add ARMv8 PSCI framework

[Hongbo Zhang]

Ping all,
Some time ago I saw several people mentioned ARMv8 PSCI, are you
interested in leaving your review comments?
I know it is not so easy for reviewing assembly language patches, but
code structures and assembly instructions in this patch set are not
complicated.
When this common framework gets merged, it will be possible for each
platform to implement their platform PSCI functions in C too.

Hi Tom,
Any further concerns? comments?

Thanks all.

On Wed, Sep 28, 2016 at 4:27 PM, Hongbo Zhang  wrote:
> I just explained why and how I tried to place newly introduced macros
> in arch/arm/cpu/armv8/Kconfig and  board/freescale/ls1043ardb/Kconfig.
>
> As to the order, is it compulsory to define such a CONFIG_* before
> using it, even there is #ifdef test when using it?
>
> If yes, I have to define all the four new CONFIG_* into
> arch/arm/cpu/armv8/Kconfig along with the adding-secure-sections
> patch, but default values have to be set which is bit hard to handle,
> for the CONFIG_ARMV8_PSCI_NR_CPUS, we can set a 4 although it isn't
> correct in most cases, but for the CONFIG_ARMV8_SECURE_BASE, we don't
> know its default value, it is really platform specific.
>
> In my implementation, I just wanted to treat CONFIG_ARMV8_PSCI_NR_CPUS
> and CONFIG_CPU_PER_CLUSTER as same as CONFIG_ARMV8_SECURE_BASE, since
> they are all platform specific.
>
> And for the CONFIG_ARMV8_PSCI_NR_CPUS, it is slightly different form
> ARMv7's, if not defined, the lds file still works:
> #ifdef CONFIG_ARMV8_PSCI_NR_CPUS
> . = . + CONFIG_ARMV8_PSCI_NR_CPUS * ARM_PSCI_STACK_SIZE;
> #else
> . = . + 4 * ARM_PSCI_STACK_SIZE;
> #endif
>
>
>
> On Wed, Sep 28, 2016 at 3:16 PM, Hongbo Zhang  wrote:
>> On Wed, Sep 28, 2016 at 1:23 AM, Tom Rini  wrote:
>>> On Tue, Sep 27, 2016 at 05:29:00PM +0800, macro.wav...@gmail.com wrote:
 From: Hongbo Zhang 

 This patch set introduces ARMv8 PSCI framework, all the PSCI functions are
 implemented a default dummy one, it is up to each platform to implement 
 their
 own specific ones.

 The first 1/6 patch is a prepare clean up for adding ARMv8 PSCI.
 Patches 2/6 to 5/6 introduce new ARMv8 framework and set it up.
 The last 6/6 adds a most simple implementation on NXP LS1043 platform, to
 verify this framework.

 This patch set mainly introduces ARMv8 PSCI framework, for easier review 
 and
 merge, further PSCI implementation on LS1043 is coming later.

 Hongbo Zhang (6):
   ARMv8: LS1043A: change macro CONFIG_ARMV8_PSCI definition
   ARMv8: Add secure sections for PSCI text and data
   ARMv8: Add basic PSCI framework
   ARMv8: Setup PSCI memory and dt
   ARMv8: Enable SMC instruction
   ARMv8: LS1043A: Enable LS1043A default PSCI support
>>>
>>> Conceptually this is good.  I have some issues around order of the
>>> patches, and where the Kconfig entries end up.  Looking over the series
>>> we introduce usage of some CONFIG symbols prior to declaring them in
>>> Kconfig.  This is more of a hard no now as it will break bisecting when
>>> the test for no new CONFIG symbols is tripped.  The other problem is
>>> that I think the symbols you're adding in
>>> board/freescale/ls1043ardb/Kconfig need to be in
>>> arch/arm/cpu/armv8/Kconfig and then use default ... if ... to give the
>>> right address for the layerscape boards.
>>
>> Thanks Tom for quick response.
>>
>> For config options introduced:
>> CONFIG_ARMV8_PSCI
>> CONFIG_ARMV8_PSCI_NR_CPUS
>> CONFIG_CPU_PER_CLUSTER
>> CONFIG_ARMV8_SECURE_BASE
>>
>> I've tested adding patch one by one, there is no problem with the
>> check-config script.
>>
>> And my idea was like this: let the CONFIG_ARMV8_PSCI to be an overall
>> switch, and if it is enabled even without the other three ones, the
>> default PSCI still works, as I've tested, this really works because
>> any of the other three macros, when used, there is a #ifdef to check
>> if it exists, if no, a default value is used or it isn't used at all.
>> The later three macros, because they are platform specific so I
>> intended to let every platform to define them.
>>
>> This is slightly different from ARMv7, plan was if this get accepted,
>> I would like to send patch to update ARMv7's.
>>
>>>
>>> --
>>> Tom
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 0/6] Add ARMv8 PSCI framework

[Hongbo Zhang]

I just explained why and how I tried to place newly introduced macros
in arch/arm/cpu/armv8/Kconfig and  board/freescale/ls1043ardb/Kconfig.

As to the order, is it compulsory to define such a CONFIG_* before
using it, even there is #ifdef test when using it?

If yes, I have to define all the four new CONFIG_* into
arch/arm/cpu/armv8/Kconfig along with the adding-secure-sections
patch, but default values have to be set which is bit hard to handle,
for the CONFIG_ARMV8_PSCI_NR_CPUS, we can set a 4 although it isn't
correct in most cases, but for the CONFIG_ARMV8_SECURE_BASE, we don't
know its default value, it is really platform specific.

In my implementation, I just wanted to treat CONFIG_ARMV8_PSCI_NR_CPUS
and CONFIG_CPU_PER_CLUSTER as same as CONFIG_ARMV8_SECURE_BASE, since
they are all platform specific.

And for the CONFIG_ARMV8_PSCI_NR_CPUS, it is slightly different form
ARMv7's, if not defined, the lds file still works:
#ifdef CONFIG_ARMV8_PSCI_NR_CPUS
. = . + CONFIG_ARMV8_PSCI_NR_CPUS * ARM_PSCI_STACK_SIZE;
#else
. = . + 4 * ARM_PSCI_STACK_SIZE;
#endif



On Wed, Sep 28, 2016 at 3:16 PM, Hongbo Zhang  wrote:
> On Wed, Sep 28, 2016 at 1:23 AM, Tom Rini  wrote:
>> On Tue, Sep 27, 2016 at 05:29:00PM +0800, macro.wav...@gmail.com wrote:
>>> From: Hongbo Zhang 
>>>
>>> This patch set introduces ARMv8 PSCI framework, all the PSCI functions are
>>> implemented a default dummy one, it is up to each platform to implement 
>>> their
>>> own specific ones.
>>>
>>> The first 1/6 patch is a prepare clean up for adding ARMv8 PSCI.
>>> Patches 2/6 to 5/6 introduce new ARMv8 framework and set it up.
>>> The last 6/6 adds a most simple implementation on NXP LS1043 platform, to
>>> verify this framework.
>>>
>>> This patch set mainly introduces ARMv8 PSCI framework, for easier review and
>>> merge, further PSCI implementation on LS1043 is coming later.
>>>
>>> Hongbo Zhang (6):
>>>   ARMv8: LS1043A: change macro CONFIG_ARMV8_PSCI definition
>>>   ARMv8: Add secure sections for PSCI text and data
>>>   ARMv8: Add basic PSCI framework
>>>   ARMv8: Setup PSCI memory and dt
>>>   ARMv8: Enable SMC instruction
>>>   ARMv8: LS1043A: Enable LS1043A default PSCI support
>>
>> Conceptually this is good.  I have some issues around order of the
>> patches, and where the Kconfig entries end up.  Looking over the series
>> we introduce usage of some CONFIG symbols prior to declaring them in
>> Kconfig.  This is more of a hard no now as it will break bisecting when
>> the test for no new CONFIG symbols is tripped.  The other problem is
>> that I think the symbols you're adding in
>> board/freescale/ls1043ardb/Kconfig need to be in
>> arch/arm/cpu/armv8/Kconfig and then use default ... if ... to give the
>> right address for the layerscape boards.
>
> Thanks Tom for quick response.
>
> For config options introduced:
> CONFIG_ARMV8_PSCI
> CONFIG_ARMV8_PSCI_NR_CPUS
> CONFIG_CPU_PER_CLUSTER
> CONFIG_ARMV8_SECURE_BASE
>
> I've tested adding patch one by one, there is no problem with the
> check-config script.
>
> And my idea was like this: let the CONFIG_ARMV8_PSCI to be an overall
> switch, and if it is enabled even without the other three ones, the
> default PSCI still works, as I've tested, this really works because
> any of the other three macros, when used, there is a #ifdef to check
> if it exists, if no, a default value is used or it isn't used at all.
> The later three macros, because they are platform specific so I
> intended to let every platform to define them.
>
> This is slightly different from ARMv7, plan was if this get accepted,
> I would like to send patch to update ARMv7's.
>
>>
>> --
>> Tom
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 0/6] Add ARMv8 PSCI framework

[Hongbo Zhang]

On Wed, Sep 28, 2016 at 1:23 AM, Tom Rini  wrote:
> On Tue, Sep 27, 2016 at 05:29:00PM +0800, macro.wav...@gmail.com wrote:
>> From: Hongbo Zhang 
>>
>> This patch set introduces ARMv8 PSCI framework, all the PSCI functions are
>> implemented a default dummy one, it is up to each platform to implement their
>> own specific ones.
>>
>> The first 1/6 patch is a prepare clean up for adding ARMv8 PSCI.
>> Patches 2/6 to 5/6 introduce new ARMv8 framework and set it up.
>> The last 6/6 adds a most simple implementation on NXP LS1043 platform, to
>> verify this framework.
>>
>> This patch set mainly introduces ARMv8 PSCI framework, for easier review and
>> merge, further PSCI implementation on LS1043 is coming later.
>>
>> Hongbo Zhang (6):
>>   ARMv8: LS1043A: change macro CONFIG_ARMV8_PSCI definition
>>   ARMv8: Add secure sections for PSCI text and data
>>   ARMv8: Add basic PSCI framework
>>   ARMv8: Setup PSCI memory and dt
>>   ARMv8: Enable SMC instruction
>>   ARMv8: LS1043A: Enable LS1043A default PSCI support
>
> Conceptually this is good.  I have some issues around order of the
> patches, and where the Kconfig entries end up.  Looking over the series
> we introduce usage of some CONFIG symbols prior to declaring them in
> Kconfig.  This is more of a hard no now as it will break bisecting when
> the test for no new CONFIG symbols is tripped.  The other problem is
> that I think the symbols you're adding in
> board/freescale/ls1043ardb/Kconfig need to be in
> arch/arm/cpu/armv8/Kconfig and then use default ... if ... to give the
> right address for the layerscape boards.

Thanks Tom for quick response.

For config options introduced:
CONFIG_ARMV8_PSCI
CONFIG_ARMV8_PSCI_NR_CPUS
CONFIG_CPU_PER_CLUSTER
CONFIG_ARMV8_SECURE_BASE

I've tested adding patch one by one, there is no problem with the
check-config script.

And my idea was like this: let the CONFIG_ARMV8_PSCI to be an overall
switch, and if it is enabled even without the other three ones, the
default PSCI still works, as I've tested, this really works because
any of the other three macros, when used, there is a #ifdef to check
if it exists, if no, a default value is used or it isn't used at all.
The later three macros, because they are platform specific so I
intended to let every platform to define them.

This is slightly different from ARMv7, plan was if this get accepted,
I would like to send patch to update ARMv7's.

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

Re: [U-Boot] [PATCH 0/6] Add ARMv8 PSCI framework

[Tom Rini]

On Tue, Sep 27, 2016 at 05:29:00PM +0800, macro.wav...@gmail.com wrote:
> From: Hongbo Zhang 
> 
> This patch set introduces ARMv8 PSCI framework, all the PSCI functions are
> implemented a default dummy one, it is up to each platform to implement their
> own specific ones.
> 
> The first 1/6 patch is a prepare clean up for adding ARMv8 PSCI.
> Patches 2/6 to 5/6 introduce new ARMv8 framework and set it up.
> The last 6/6 adds a most simple implementation on NXP LS1043 platform, to
> verify this framework.
> 
> This patch set mainly introduces ARMv8 PSCI framework, for easier review and
> merge, further PSCI implementation on LS1043 is coming later.
> 
> Hongbo Zhang (6):
>   ARMv8: LS1043A: change macro CONFIG_ARMV8_PSCI definition
>   ARMv8: Add secure sections for PSCI text and data
>   ARMv8: Add basic PSCI framework
>   ARMv8: Setup PSCI memory and dt
>   ARMv8: Enable SMC instruction
>   ARMv8: LS1043A: Enable LS1043A default PSCI support

Conceptually this is good.  I have some issues around order of the
patches, and where the Kconfig entries end up.  Looking over the series
we introduce usage of some CONFIG symbols prior to declaring them in
Kconfig.  This is more of a hard no now as it will break bisecting when
the test for no new CONFIG symbols is tripped.  The other problem is
that I think the symbols you're adding in
board/freescale/ls1043ardb/Kconfig need to be in
arch/arm/cpu/armv8/Kconfig and then use default ... if ... to give the
right address for the layerscape boards.

-- 
Tom


signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot