Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1670468 > unrolled thread

Re: [PATCH v3 0/4] Generalize fncpy availability

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 0/4] Generalize fncpy availability Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2017-06-20 11:10 +0200
    Re: [PATCH v3 0/4] Generalize fncpy availability Florian Fainelli <f.fainelli@gmail.com> - 2017-06-20 18:30 +0200
      Re: [PATCH v3 0/4] Generalize fncpy availability Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2017-06-20 18:50 +0200
      Re: [PATCH v3 0/4] Generalize fncpy availability Sudeep Holla <sudeep.holla@arm.com> - 2017-06-20 19:00 +0200
        Re: [PATCH v3 0/4] Generalize fncpy availability Florian Fainelli <f.fainelli@gmail.com> - 2017-06-20 19:10 +0200

#1670468 — Re: [PATCH v3 0/4] Generalize fncpy availability

[+Sudeep]

On Mon, Jun 19, 2017 at 10:32:38AM -0700, Florian Fainelli wrote:
> On 06/19/2017 05:24 AM, Mark Rutland wrote:
> > On Fri, Jun 16, 2017 at 05:07:40PM -0700, Florian Fainelli wrote:
> >> Hi all,
> > 
> > Hi Florian,
> > 
> >> This patch series makes ARM's fncpy() implementation more generic (dropping the
> >> Thumb-specifics) and available in an asm-generic header file.
> >>
> >> Tested on a Broadcom ARM64 STB platform with code that is written to SRAM.
> >>
> >> Changes in v3 (thanks Doug!):
> >> - correct include guard names in asm-generic/fncpy.h to __ASM_FNCPY_H
> >> - utilize Kbuild to provide the fncpy.h header on ARM64
> >>
> >> Changes in v2:
> >> - leave the ARM implementation where it is
> >> - make the generic truly generic (no)
> >>
> >> This is helpful in making SoC-specific power management code become true drivers
> >> that can be shared between different architectures.
> > > Could you elaborate on what this is needed for?
> 
> Several uses cases come to mind:
> 
> - it could be used as a trampoline code prior to entering S2 for systems
> that do not support PSCI 1.0

I think S2 here means PM_SUSPEND_MEM. It is very wrong to manage power
states through platform specific hooks on PSCI based systems, consider
upgrading to PSCI 1.0 please (or implement PSCI CPU_SUSPEND power
states that allow to achieve same power savings as PM_SUSPEND_MEM
by just entering suspend-to-idle).

> - any code that has a specific need to relocate a performance, security
> sensitive code into SRAM and use it as another pool of memory.
> 
> > 
> > My understanding was that on 32-bit, this was to handle idle / suspend
> > cases, whereas for arm64 that should be handled by PSCI.
> 
> For systems that support PSCI 1.0, I agree, but it may not be possible
> to update those systems easily, still use case 2 is completely valid.

Just to be clear, thinking of using platform specific suspend hooks
on PSCI systems is not a viable solution, I will let other people
comment on option 2.

> > what exactly do you intend to use this for?
> 
> At the moment we use it to enter S2 on ARM64 systems (ARCH_BRCMSTB)

"At the moment", where ?

> which are PSCI 0.2 only. And yes, we do have a plan to evaluate
> upgrading to PSCI 1.0, but in general, any SoC which as an addressable
> SRAM could use it for whatever purpose it sees fit.

Not to implement suspend hooks on PSCI 0.2 systems.

Thanks,
Lorenzo

[toc] | [next] | [standalone]

#1670931

On 06/20/2017 02:10 AM, Lorenzo Pieralisi wrote:
> [+Sudeep]
> 
> On Mon, Jun 19, 2017 at 10:32:38AM -0700, Florian Fainelli wrote:
>> On 06/19/2017 05:24 AM, Mark Rutland wrote:
>>> On Fri, Jun 16, 2017 at 05:07:40PM -0700, Florian Fainelli wrote:
>>>> Hi all,
>>>
>>> Hi Florian,
>>>
>>>> This patch series makes ARM's fncpy() implementation more generic (dropping the
>>>> Thumb-specifics) and available in an asm-generic header file.
>>>>
>>>> Tested on a Broadcom ARM64 STB platform with code that is written to SRAM.
>>>>
>>>> Changes in v3 (thanks Doug!):
>>>> - correct include guard names in asm-generic/fncpy.h to __ASM_FNCPY_H
>>>> - utilize Kbuild to provide the fncpy.h header on ARM64
>>>>
>>>> Changes in v2:
>>>> - leave the ARM implementation where it is
>>>> - make the generic truly generic (no)
>>>>
>>>> This is helpful in making SoC-specific power management code become true drivers
>>>> that can be shared between different architectures.
>>>> Could you elaborate on what this is needed for?
>>
>> Several uses cases come to mind:
>>
>> - it could be used as a trampoline code prior to entering S2 for systems
>> that do not support PSCI 1.0
> 
> I think S2 here means PM_SUSPEND_MEM. It is very wrong to manage power
> states through platform specific hooks on PSCI based systems, consider
> upgrading to PSCI 1.0 please (or implement PSCI CPU_SUSPEND power
> states that allow to achieve same power savings as PM_SUSPEND_MEM
> by just entering suspend-to-idle).

S2 is PM_SUSPEND_STANDBY and S3 is PM_SUSPEND_MEM, at least that how I
read it. I would rather we update to PSCI 1.0 (at least) to properly
support SYSTEM_SUSPEND rather than retrofitting a system-wide suspend
state into CPU_SUSPEND since that seems wrong.

> 
>> - any code that has a specific need to relocate a performance, security
>> sensitive code into SRAM and use it as another pool of memory.
>>
>>>
>>> My understanding was that on 32-bit, this was to handle idle / suspend
>>> cases, whereas for arm64 that should be handled by PSCI.
>>
>> For systems that support PSCI 1.0, I agree, but it may not be possible
>> to update those systems easily, still use case 2 is completely valid.
> 
> Just to be clear, thinking of using platform specific suspend hooks
> on PSCI systems is not a viable solution, I will let other people
> comment on option 2.
> 
>>> what exactly do you intend to use this for?
>>
>> At the moment we use it to enter S2 on ARM64 systems (ARCH_BRCMSTB)
> 
> "At the moment", where ?

Obviously not in tree, since fncpy() is not available on ARM64, here is
what it looks like for ARM & MIPS systems though:

http://lists.infradead.org/pipermail/linux-arm-kernel/2017-June/513953.html

> 
>> which are PSCI 0.2 only. And yes, we do have a plan to evaluate
>> upgrading to PSCI 1.0, but in general, any SoC which as an addressable
>> SRAM could use it for whatever purpose it sees fit.
> 
> Not to implement suspend hooks on PSCI 0.2 systems.

You have made your point and it is very valid, still that does not mean
fncpy() does not have any usefulness on ARM64 systems, this was one
although not "ARM approved" use case (system suspend), but use case
where you have code that should execute from SRAM is something that
should be possible using the standard facilities offered in
drives/misc/sram*.c
-- 
Florian

[toc] | [prev] | [next] | [standalone]

#1670946

On Tue, Jun 20, 2017 at 09:20:34AM -0700, Florian Fainelli wrote:
> On 06/20/2017 02:10 AM, Lorenzo Pieralisi wrote:
> > [+Sudeep]
> > 
> > On Mon, Jun 19, 2017 at 10:32:38AM -0700, Florian Fainelli wrote:
> >> On 06/19/2017 05:24 AM, Mark Rutland wrote:
> >>> On Fri, Jun 16, 2017 at 05:07:40PM -0700, Florian Fainelli wrote:
> >>>> Hi all,
> >>>
> >>> Hi Florian,
> >>>
> >>>> This patch series makes ARM's fncpy() implementation more generic (dropping the
> >>>> Thumb-specifics) and available in an asm-generic header file.
> >>>>
> >>>> Tested on a Broadcom ARM64 STB platform with code that is written to SRAM.
> >>>>
> >>>> Changes in v3 (thanks Doug!):
> >>>> - correct include guard names in asm-generic/fncpy.h to __ASM_FNCPY_H
> >>>> - utilize Kbuild to provide the fncpy.h header on ARM64
> >>>>
> >>>> Changes in v2:
> >>>> - leave the ARM implementation where it is
> >>>> - make the generic truly generic (no)
> >>>>
> >>>> This is helpful in making SoC-specific power management code become true drivers
> >>>> that can be shared between different architectures.
> >>>> Could you elaborate on what this is needed for?
> >>
> >> Several uses cases come to mind:
> >>
> >> - it could be used as a trampoline code prior to entering S2 for systems
> >> that do not support PSCI 1.0
> > 
> > I think S2 here means PM_SUSPEND_MEM. It is very wrong to manage power
> > states through platform specific hooks on PSCI based systems, consider
> > upgrading to PSCI 1.0 please (or implement PSCI CPU_SUSPEND power
> > states that allow to achieve same power savings as PM_SUSPEND_MEM
> > by just entering suspend-to-idle).
> 
> S2 is PM_SUSPEND_STANDBY and S3 is PM_SUSPEND_MEM, at least that how I
> read it. I would rather we update to PSCI 1.0 (at least) to properly
> support SYSTEM_SUSPEND rather than retrofitting a system-wide suspend
> state into CPU_SUSPEND since that seems wrong.

I am not asking to retrofit anything, just implementing CPU_SUSPEND
according to platform capabilities, suspend-to-idle would allow you to
enter a system suspend state where all cores are in the deepest idle
state (which, as far as the core is concerned is identical to the state
reached on S2R). Upgrading to PSCI 1.0 is what has to be done anyway.

> >> - any code that has a specific need to relocate a performance, security
> >> sensitive code into SRAM and use it as another pool of memory.
> >>
> >>>
> >>> My understanding was that on 32-bit, this was to handle idle / suspend
> >>> cases, whereas for arm64 that should be handled by PSCI.
> >>
> >> For systems that support PSCI 1.0, I agree, but it may not be possible
> >> to update those systems easily, still use case 2 is completely valid.
> > 
> > Just to be clear, thinking of using platform specific suspend hooks
> > on PSCI systems is not a viable solution, I will let other people
> > comment on option 2.
> > 
> >>> what exactly do you intend to use this for?
> >>
> >> At the moment we use it to enter S2 on ARM64 systems (ARCH_BRCMSTB)
> > 
> > "At the moment", where ?
> 
> Obviously not in tree, since fncpy() is not available on ARM64, here is
> what it looks like for ARM & MIPS systems though:
> 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2017-June/513953.html
> 
> > 
> >> which are PSCI 0.2 only. And yes, we do have a plan to evaluate
> >> upgrading to PSCI 1.0, but in general, any SoC which as an addressable
> >> SRAM could use it for whatever purpose it sees fit.
> > 
> > Not to implement suspend hooks on PSCI 0.2 systems.
> 
> You have made your point and it is very valid, still that does not mean
> fncpy() does not have any usefulness on ARM64 systems, this was one
> although not "ARM approved" use case (system suspend), but use case
> where you have code that should execute from SRAM is something that
> should be possible using the standard facilities offered in
> drives/misc/sram*.c

fncpy() won't be used to implement a suspend ops hook on ARM64, that's
the point I made so that it is clear from the beginning.

Thanks,
Lorenzo

[toc] | [prev] | [next] | [standalone]

#1670954

On 20/06/17 17:20, Florian Fainelli wrote:
> On 06/20/2017 02:10 AM, Lorenzo Pieralisi wrote:
>> [+Sudeep]
>>
>> On Mon, Jun 19, 2017 at 10:32:38AM -0700, Florian Fainelli wrote:
>>> On 06/19/2017 05:24 AM, Mark Rutland wrote:
>>>> On Fri, Jun 16, 2017 at 05:07:40PM -0700, Florian Fainelli wrote:
>>>>> Hi all,
>>>>
>>>> Hi Florian,
>>>>
>>>>> This patch series makes ARM's fncpy() implementation more generic (dropping the
>>>>> Thumb-specifics) and available in an asm-generic header file.
>>>>>
>>>>> Tested on a Broadcom ARM64 STB platform with code that is written to SRAM.
>>>>>
>>>>> Changes in v3 (thanks Doug!):
>>>>> - correct include guard names in asm-generic/fncpy.h to __ASM_FNCPY_H
>>>>> - utilize Kbuild to provide the fncpy.h header on ARM64
>>>>>
>>>>> Changes in v2:
>>>>> - leave the ARM implementation where it is
>>>>> - make the generic truly generic (no)
>>>>>
>>>>> This is helpful in making SoC-specific power management code become true drivers
>>>>> that can be shared between different architectures.
>>>>> Could you elaborate on what this is needed for?
>>>
>>> Several uses cases come to mind:
>>>
>>> - it could be used as a trampoline code prior to entering S2 for systems
>>> that do not support PSCI 1.0
>>
>> I think S2 here means PM_SUSPEND_MEM. It is very wrong to manage power
>> states through platform specific hooks on PSCI based systems, consider
>> upgrading to PSCI 1.0 please (or implement PSCI CPU_SUSPEND power
>> states that allow to achieve same power savings as PM_SUSPEND_MEM
>> by just entering suspend-to-idle).
> 
> S2 is PM_SUSPEND_STANDBY and S3 is PM_SUSPEND_MEM, at least that how I
> read it. I would rather we update to PSCI 1.0 (at least) to properly
> support SYSTEM_SUSPEND rather than retrofitting a system-wide suspend
> state into CPU_SUSPEND since that seems wrong.
> 

This has been discussed multiple times in the past. No one has come back
with strong reason to add that to the PSCI SYSTEM_SUSPEND API.

Care to explain the difference between PM_SUSPEND_STANDBY and S3 is
PM_SUSPEND_MEM on your platform. And why it can't be achieved with
suspend-to-idle ?

You can always report any issue with PSCI specification at
errata@arm.com as mentioned in the document.
-- 
Regards,
Sudeep

[toc] | [prev] | [next] | [standalone]

#1670963

On 06/20/2017 09:54 AM, Sudeep Holla wrote:
> 
> 
> On 20/06/17 17:20, Florian Fainelli wrote:
>> On 06/20/2017 02:10 AM, Lorenzo Pieralisi wrote:
>>> [+Sudeep]
>>>
>>> On Mon, Jun 19, 2017 at 10:32:38AM -0700, Florian Fainelli wrote:
>>>> On 06/19/2017 05:24 AM, Mark Rutland wrote:
>>>>> On Fri, Jun 16, 2017 at 05:07:40PM -0700, Florian Fainelli wrote:
>>>>>> Hi all,
>>>>>
>>>>> Hi Florian,
>>>>>
>>>>>> This patch series makes ARM's fncpy() implementation more generic (dropping the
>>>>>> Thumb-specifics) and available in an asm-generic header file.
>>>>>>
>>>>>> Tested on a Broadcom ARM64 STB platform with code that is written to SRAM.
>>>>>>
>>>>>> Changes in v3 (thanks Doug!):
>>>>>> - correct include guard names in asm-generic/fncpy.h to __ASM_FNCPY_H
>>>>>> - utilize Kbuild to provide the fncpy.h header on ARM64
>>>>>>
>>>>>> Changes in v2:
>>>>>> - leave the ARM implementation where it is
>>>>>> - make the generic truly generic (no)
>>>>>>
>>>>>> This is helpful in making SoC-specific power management code become true drivers
>>>>>> that can be shared between different architectures.
>>>>>> Could you elaborate on what this is needed for?
>>>>
>>>> Several uses cases come to mind:
>>>>
>>>> - it could be used as a trampoline code prior to entering S2 for systems
>>>> that do not support PSCI 1.0
>>>
>>> I think S2 here means PM_SUSPEND_MEM. It is very wrong to manage power
>>> states through platform specific hooks on PSCI based systems, consider
>>> upgrading to PSCI 1.0 please (or implement PSCI CPU_SUSPEND power
>>> states that allow to achieve same power savings as PM_SUSPEND_MEM
>>> by just entering suspend-to-idle).
>>
>> S2 is PM_SUSPEND_STANDBY and S3 is PM_SUSPEND_MEM, at least that how I
>> read it. I would rather we update to PSCI 1.0 (at least) to properly
>> support SYSTEM_SUSPEND rather than retrofitting a system-wide suspend
>> state into CPU_SUSPEND since that seems wrong.
>>
> 
> This has been discussed multiple times in the past. No one has come back
> with strong reason to add that to the PSCI SYSTEM_SUSPEND API.
> 
> Care to explain the difference between PM_SUSPEND_STANDBY and S3 is
> PM_SUSPEND_MEM on your platform. And why it can't be achieved with
> suspend-to-idle ?

S2 preserves the ON/OFF island power and allows wake logic to wake the
system (infrared, GPIOs, Wake-on-LAN/WLAN, etc.) whereas S3 allows
powering off the ON/OFF island entirely, and allows for a lower power
consumption, with a subset of the wake peripherals to actually wake the
system. S5 is also implemented although its use case is narrower (soft-off).

The higher latency involved in S3 entry/exit is totally accepted due to
the higher power savings that it yields.

> 
> You can always report any issue with PSCI specification at
> errata@arm.com as mentioned in the document.

I just did because there are a few other things missing.
-- 
Florian

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web