[PATCH] ARM: psci: boot_secondary: replace __pa with virt_to_idmap

[PATCH] ARM: psci: boot_secondary: replace __pa with virt_to_idmap

[Grygorii Strashko]

On some PAE systems (e.g. TI Keystone), memory is above the 32-bit
addressable limit, and the interconnect provides an aliased view of
parts of physical memory in the 32-bit addressable space. This alias
is strictly for boot time usage, and is not otherwise usable because
of coherency limitations.

On such systems, the idmap mechanism has to be used to pass correct
boot address of secondary CPU to FW.
virt_to_idmap() will fall-back to existing virt_to_phys() macro if
such conversation is not required.

Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Nicolas Pitre <nico@linaro.org>
Cc: Santosh Shilimkar <ssantosh@kernel.org>
Cc: Vitaly Andrianov <vitalya@ti.com>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 arch/arm/kernel/psci_smp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/kernel/psci_smp.c b/arch/arm/kernel/psci_smp.c
index 28a1db4..244aadd 100644
--- a/arch/arm/kernel/psci_smp.c
+++ b/arch/arm/kernel/psci_smp.c
@@ -51,7 +51,7 @@ static int psci_boot_secondary(unsigned int cpu, struct task_struct *idle)
 {
 	if (psci_ops.cpu_on)
 		return psci_ops.cpu_on(cpu_logical_map(cpu),
-				       __pa(secondary_startup));
+					virt_to_idmap(&secondary_startup));
 	return -ENODEV;
 }
 
-- 
2.4.5

[PATCH] ARM: psci: boot_secondary: replace __pa with virt_to_idmap

[santosh shilimkar]

On 6/30/2015 3:44 AM, Grygorii Strashko wrote:
> On some PAE systems (e.g. TI Keystone), memory is above the 32-bit
> addressable limit, and the interconnect provides an aliased view of
> parts of physical memory in the 32-bit addressable space. This alias
> is strictly for boot time usage, and is not otherwise usable because
> of coherency limitations.
>
> On such systems, the idmap mechanism has to be used to pass correct
> boot address of secondary CPU to FW.
> virt_to_idmap() will fall-back to existing virt_to_phys() macro if
> such conversation is not required.
>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Nicolas Pitre <nico@linaro.org>
> Cc: Santosh Shilimkar <ssantosh@kernel.org>
> Cc: Vitaly Andrianov <vitalya@ti.com>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
Acked-by: Santosh Shilimkar <ssantosh@kernel.org>

>   arch/arm/kernel/psci_smp.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/kernel/psci_smp.c b/arch/arm/kernel/psci_smp.c
> index 28a1db4..244aadd 100644
> --- a/arch/arm/kernel/psci_smp.c
> +++ b/arch/arm/kernel/psci_smp.c
> @@ -51,7 +51,7 @@ static int psci_boot_secondary(unsigned int cpu, struct task_struct *idle)
>   {
>   	if (psci_ops.cpu_on)
>   		return psci_ops.cpu_on(cpu_logical_map(cpu),
> -				       __pa(secondary_startup));
> +					virt_to_idmap(&secondary_startup));
>   	return -ENODEV;
>   }
>
>

[PATCH] ARM: psci: boot_secondary: replace __pa with virt_to_idmap

[Russell King - ARM Linux]

On Tue, Jun 30, 2015 at 01:44:22PM +0300, Grygorii Strashko wrote:
> On some PAE systems (e.g. TI Keystone), memory is above the 32-bit
> addressable limit, and the interconnect provides an aliased view of
> parts of physical memory in the 32-bit addressable space. This alias
> is strictly for boot time usage, and is not otherwise usable because
> of coherency limitations.
> 
> On such systems, the idmap mechanism has to be used to pass correct
> boot address of secondary CPU to FW.
> virt_to_idmap() will fall-back to existing virt_to_phys() macro if
> such conversation is not required.

I think this could do with improvement so that those who aren't aware of
the issue can understand better.  The idmap mechanism is always used for
secondary booting anyway.

"In this case, virt_to_phys(secondary_startup) would return the
physical address of the secondary CPU boot entry point, but on such
systems, this would be above the 4GB limit.

A separate function, virt_to_idmap(), has been provided to return a
usable physical address for functions in the identity mapping, and
this must be used in preference to virt_to_phys() or __pa() to find
the physical entry point for functions in the identity mapping range.

For other systems, virt_to_idmap() and virt_to_phys() return identical
physical addresses."

> 
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Nicolas Pitre <nico@linaro.org>
> Cc: Santosh Shilimkar <ssantosh@kernel.org>
> Cc: Vitaly Andrianov <vitalya@ti.com>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

Code-wise, this looks correct to me - but as it's PSCI stuff, I'd like
to see Nico ack it.

Thanks.

> ---
>  arch/arm/kernel/psci_smp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/kernel/psci_smp.c b/arch/arm/kernel/psci_smp.c
> index 28a1db4..244aadd 100644
> --- a/arch/arm/kernel/psci_smp.c
> +++ b/arch/arm/kernel/psci_smp.c
> @@ -51,7 +51,7 @@ static int psci_boot_secondary(unsigned int cpu, struct task_struct *idle)
>  {
>  	if (psci_ops.cpu_on)
>  		return psci_ops.cpu_on(cpu_logical_map(cpu),
> -				       __pa(secondary_startup));
> +					virt_to_idmap(&secondary_startup));
>  	return -ENODEV;
>  }
>  
> -- 
> 2.4.5
> 

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH] ARM: psci: boot_secondary: replace __pa with virt_to_idmap

[Nicolas Pitre]

On Tue, 30 Jun 2015, Russell King - ARM Linux wrote:

> On Tue, Jun 30, 2015 at 01:44:22PM +0300, Grygorii Strashko wrote:
> > On some PAE systems (e.g. TI Keystone), memory is above the 32-bit
> > addressable limit, and the interconnect provides an aliased view of
> > parts of physical memory in the 32-bit addressable space. This alias
> > is strictly for boot time usage, and is not otherwise usable because
> > of coherency limitations.
> > 
> > On such systems, the idmap mechanism has to be used to pass correct
> > boot address of secondary CPU to FW.
> > virt_to_idmap() will fall-back to existing virt_to_phys() macro if
> > such conversation is not required.
> 
> I think this could do with improvement so that those who aren't aware of
> the issue can understand better.  The idmap mechanism is always used for
> secondary booting anyway.
> 
> "In this case, virt_to_phys(secondary_startup) would return the
> physical address of the secondary CPU boot entry point, but on such
> systems, this would be above the 4GB limit.
> 
> A separate function, virt_to_idmap(), has been provided to return a
> usable physical address for functions in the identity mapping, and
> this must be used in preference to virt_to_phys() or __pa() to find
> the physical entry point for functions in the identity mapping range.
> 
> For other systems, virt_to_idmap() and virt_to_phys() return identical
> physical addresses."
> 
> > 
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Nicolas Pitre <nico@linaro.org>
> > Cc: Santosh Shilimkar <ssantosh@kernel.org>
> > Cc: Vitaly Andrianov <vitalya@ti.com>
> > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> 
> Code-wise, this looks correct to me - but as it's PSCI stuff, I'd like
> to see Nico ack it.

I cannot pretend to be the authority on PSCI stuff. 

But this looks correct to me as well, assuming the amended commit 
message.

Acked-by: Nicolas Pitre <nico@linaro.org>

> Thanks.
> 
> > ---
> >  arch/arm/kernel/psci_smp.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/kernel/psci_smp.c b/arch/arm/kernel/psci_smp.c
> > index 28a1db4..244aadd 100644
> > --- a/arch/arm/kernel/psci_smp.c
> > +++ b/arch/arm/kernel/psci_smp.c
> > @@ -51,7 +51,7 @@ static int psci_boot_secondary(unsigned int cpu, struct task_struct *idle)
> >  {
> >  	if (psci_ops.cpu_on)
> >  		return psci_ops.cpu_on(cpu_logical_map(cpu),
> > -				       __pa(secondary_startup));
> > +					virt_to_idmap(&secondary_startup));
> >  	return -ENODEV;
> >  }
> >  
> > -- 
> > 2.4.5
> > 
> 
> -- 
> FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
> according to speedtest.net.
> 
> 

[PATCH] ARM: psci: boot_secondary: replace __pa with virt_to_idmap

[Mark Rutland]

Hi,

On Tue, Jun 30, 2015 at 11:44:22AM +0100, Grygorii Strashko wrote:
> On some PAE systems (e.g. TI Keystone), memory is above the 32-bit
> addressable limit, and the interconnect provides an aliased view of
> parts of physical memory in the 32-bit addressable space. This alias
> is strictly for boot time usage, and is not otherwise usable because
> of coherency limitations.
> 
> On such systems, the idmap mechanism has to be used to pass correct
> boot address of secondary CPU to FW.
> virt_to_idmap() will fall-back to existing virt_to_phys() macro if
> such conversation is not required.
> 
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Nicolas Pitre <nico@linaro.org>
> Cc: Santosh Shilimkar <ssantosh@kernel.org>
> Cc: Vitaly Andrianov <vitalya@ti.com>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
>  arch/arm/kernel/psci_smp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

The code itself looks sane to me, though the commit message would be
better with Russell's suggested rewording.

This will also conflict with my migration to a common PSCI client
implementation [1,2] (especially given the absence of virt_to_idmap on
arm64), but I'm happy to fold it in or rebase atop of it.

Russell, are you able to take a look at the migration patch [2]? How
would you prefer for the two patches to be taken?

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-May/346492.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-May/346503.html

> 
> diff --git a/arch/arm/kernel/psci_smp.c b/arch/arm/kernel/psci_smp.c
> index 28a1db4..244aadd 100644
> --- a/arch/arm/kernel/psci_smp.c
> +++ b/arch/arm/kernel/psci_smp.c
> @@ -51,7 +51,7 @@ static int psci_boot_secondary(unsigned int cpu, struct task_struct *idle)
>  {
>  	if (psci_ops.cpu_on)
>  		return psci_ops.cpu_on(cpu_logical_map(cpu),
> -				       __pa(secondary_startup));
> +					virt_to_idmap(&secondary_startup));
>  	return -ENODEV;
>  }
>  
> -- 
> 2.4.5
> 

[PATCH] ARM: psci: boot_secondary: replace __pa with virt_to_idmap

[Vitaly Andrianov]

On 06/30/2015 11:59 AM, Nicolas Pitre wrote:
> On Tue, 30 Jun 2015, Russell King - ARM Linux wrote:
>
>> On Tue, Jun 30, 2015 at 01:44:22PM +0300, Grygorii Strashko wrote:
>>> On some PAE systems (e.g. TI Keystone), memory is above the 32-bit
>>> addressable limit, and the interconnect provides an aliased view of
>>> parts of physical memory in the 32-bit addressable space. This alias
>>> is strictly for boot time usage, and is not otherwise usable because
>>> of coherency limitations.
>>>
>>> On such systems, the idmap mechanism has to be used to pass correct
>>> boot address of secondary CPU to FW.
>>> virt_to_idmap() will fall-back to existing virt_to_phys() macro if
>>> such conversation is not required.
>>
>> I think this could do with improvement so that those who aren't aware of
>> the issue can understand better.  The idmap mechanism is always used for
>> secondary booting anyway.
>>
>> "In this case, virt_to_phys(secondary_startup) would return the
>> physical address of the secondary CPU boot entry point, but on such
>> systems, this would be above the 4GB limit.
>>
>> A separate function, virt_to_idmap(), has been provided to return a
>> usable physical address for functions in the identity mapping, and
>> this must be used in preference to virt_to_phys() or __pa() to find
>> the physical entry point for functions in the identity mapping range.
>>
>> For other systems, virt_to_idmap() and virt_to_phys() return identical
>> physical addresses."
>>
>>>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: Nicolas Pitre <nico@linaro.org>
>>> Cc: Santosh Shilimkar <ssantosh@kernel.org>
>>> Cc: Vitaly Andrianov <vitalya@ti.com>
>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>>
>> Code-wise, this looks correct to me - but as it's PSCI stuff, I'd like
>> to see Nico ack it.
>
> I cannot pretend to be the authority on PSCI stuff.
>
> But this looks correct to me as well, assuming the amended commit
> message.
>
> Acked-by: Nicolas Pitre <nico@linaro.org>
>
>> Thanks.
>>
>>> ---
>>>   arch/arm/kernel/psci_smp.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/kernel/psci_smp.c b/arch/arm/kernel/psci_smp.c
>>> index 28a1db4..244aadd 100644
>>> --- a/arch/arm/kernel/psci_smp.c
>>> +++ b/arch/arm/kernel/psci_smp.c
>>> @@ -51,7 +51,7 @@ static int psci_boot_secondary(unsigned int cpu, struct task_struct *idle)
>>>   {
>>>   	if (psci_ops.cpu_on)
>>>   		return psci_ops.cpu_on(cpu_logical_map(cpu),
>>> -				       __pa(secondary_startup));
>>> +					virt_to_idmap(&secondary_startup));
>>>   	return -ENODEV;
>>>   }
>>>
>>> --
>>> 2.4.5
>>>
>>
>> --
>> FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
>> according to speedtest.net.
>>
>>
Tested-by Vitaly Andrianov <vitalya@ti.com>

[PATCH] ARM: psci: boot_secondary: replace __pa with virt_to_idmap

[Russell King - ARM Linux]

On Tue, Jun 30, 2015 at 05:18:37PM +0100, Mark Rutland wrote:
> Hi,
> 
> On Tue, Jun 30, 2015 at 11:44:22AM +0100, Grygorii Strashko wrote:
> > On some PAE systems (e.g. TI Keystone), memory is above the 32-bit
> > addressable limit, and the interconnect provides an aliased view of
> > parts of physical memory in the 32-bit addressable space. This alias
> > is strictly for boot time usage, and is not otherwise usable because
> > of coherency limitations.
> > 
> > On such systems, the idmap mechanism has to be used to pass correct
> > boot address of secondary CPU to FW.
> > virt_to_idmap() will fall-back to existing virt_to_phys() macro if
> > such conversation is not required.
> > 
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Nicolas Pitre <nico@linaro.org>
> > Cc: Santosh Shilimkar <ssantosh@kernel.org>
> > Cc: Vitaly Andrianov <vitalya@ti.com>
> > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> > ---
> >  arch/arm/kernel/psci_smp.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> The code itself looks sane to me, though the commit message would be
> better with Russell's suggested rewording.
> 
> This will also conflict with my migration to a common PSCI client
> implementation [1,2] (especially given the absence of virt_to_idmap on
> arm64), but I'm happy to fold it in or rebase atop of it.
> 
> Russell, are you able to take a look at the migration patch [2]? How
> would you prefer for the two patches to be taken?

Well, the first thing that stands out about patch [2] is that it's
introducing a load of magic hex numbers, where before we had a struct
with nice definitions.  (I'm thinking about highbank_suspend_finish()
and calxeda_idle_finish().)  This seems to be a backwards step.

There's two options on how to handle this.  Either accept the
virt_to_idmap() change first, and then sort out the resulting difference,
maybe by having ARM64 grow a virt_to_idmap() of its own (which is just
a virt_to_phys()) or we do it the other way around.  Either way, we
need a new macro, and the established name for it is virt_to_idmap().
The order doesn't matter as the end result would be the same.

> Thanks,
> Mark.
> 
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-May/346492.html
> [2] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-May/346503.html

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH] ARM: psci: boot_secondary: replace __pa with virt_to_idmap

[Grygorii Strashko]

Hi,

On 06/30/2015 11:25 PM, Russell King - ARM Linux wrote:
> On Tue, Jun 30, 2015 at 05:18:37PM +0100, Mark Rutland wrote:
>> On Tue, Jun 30, 2015 at 11:44:22AM +0100, Grygorii Strashko wrote:
>>> On some PAE systems (e.g. TI Keystone), memory is above the 32-bit
>>> addressable limit, and the interconnect provides an aliased view of
>>> parts of physical memory in the 32-bit addressable space. This alias
>>> is strictly for boot time usage, and is not otherwise usable because
>>> of coherency limitations.
>>>
>>> On such systems, the idmap mechanism has to be used to pass correct
>>> boot address of secondary CPU to FW.
>>> virt_to_idmap() will fall-back to existing virt_to_phys() macro if
>>> such conversation is not required.
>>>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: Nicolas Pitre <nico@linaro.org>
>>> Cc: Santosh Shilimkar <ssantosh@kernel.org>
>>> Cc: Vitaly Andrianov <vitalya@ti.com>
>>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>>> ---
>>>   arch/arm/kernel/psci_smp.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> The code itself looks sane to me, though the commit message would be
>> better with Russell's suggested rewording.
>>
>> This will also conflict with my migration to a common PSCI client
>> implementation [1,2] (especially given the absence of virt_to_idmap on
>> arm64), but I'm happy to fold it in or rebase atop of it.

I saw your series in lists, but, seems, It is not in next (4.2).
Therefor I've not tried to base this patch on top of your series, but
you can reuse/squash it as you think is better.



>>
>> Russell, are you able to take a look at the migration patch [2]? How
>> would you prefer for the two patches to be taken?
> 
> Well, the first thing that stands out about patch [2] is that it's
> introducing a load of magic hex numbers, where before we had a struct
> with nice definitions.  (I'm thinking about highbank_suspend_finish()
> and calxeda_idle_finish().)  This seems to be a backwards step.
> 
> There's two options on how to handle this.  Either accept the
> virt_to_idmap() change first, and then sort out the resulting difference,
> maybe by having ARM64 grow a virt_to_idmap() of its own (which is just
> a virt_to_phys()) or we do it the other way around.  Either way, we
> need a new macro, and the established name for it is virt_to_idmap().
> The order doesn't matter as the end result would be the same.

There is another advantage to keep this patch as is -
as a fix it can be back-ported to stable trees (at least in 4.1, but
probably even in v3.13.

psci_smp.c added in v3.11
virt_to_idmap added in v3.13



-- 
regards,
-grygorii

[PATCH] ARM: psci: boot_secondary: replace __pa with virt_to_idmap

[Mark Rutland]

On Tue, Jun 30, 2015 at 09:25:37PM +0100, Russell King - ARM Linux wrote:
> On Tue, Jun 30, 2015 at 05:18:37PM +0100, Mark Rutland wrote:
> > Hi,
> > 
> > On Tue, Jun 30, 2015 at 11:44:22AM +0100, Grygorii Strashko wrote:
> > > On some PAE systems (e.g. TI Keystone), memory is above the 32-bit
> > > addressable limit, and the interconnect provides an aliased view of
> > > parts of physical memory in the 32-bit addressable space. This alias
> > > is strictly for boot time usage, and is not otherwise usable because
> > > of coherency limitations.
> > > 
> > > On such systems, the idmap mechanism has to be used to pass correct
> > > boot address of secondary CPU to FW.
> > > virt_to_idmap() will fall-back to existing virt_to_phys() macro if
> > > such conversation is not required.
> > > 
> > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > Cc: Nicolas Pitre <nico@linaro.org>
> > > Cc: Santosh Shilimkar <ssantosh@kernel.org>
> > > Cc: Vitaly Andrianov <vitalya@ti.com>
> > > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> > > ---
> > >  arch/arm/kernel/psci_smp.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > The code itself looks sane to me, though the commit message would be
> > better with Russell's suggested rewording.
> > 
> > This will also conflict with my migration to a common PSCI client
> > implementation [1,2] (especially given the absence of virt_to_idmap on
> > arm64), but I'm happy to fold it in or rebase atop of it.
> > 
> > Russell, are you able to take a look at the migration patch [2]? How
> > would you prefer for the two patches to be taken?
> 
> Well, the first thing that stands out about patch [2] is that it's
> introducing a load of magic hex numbers, where before we had a struct
> with nice definitions.  (I'm thinking about highbank_suspend_finish()
> and calxeda_idle_finish().)  This seems to be a backwards step.

The existing suspend parameter packing/unpacking didn't work now that
there are multiple possible formats for the suspend parameter from
PSCIv1.0 onwards, and the core PSCI code won't do any more
packing/unpacking, treating the suspend parameter as a mostly opaque
token.

I could retain the struct and packing functions for the two cases you
mention, but it seemed somewhat misleading. I'll see if I can figure out
something better than a raw hex value.

Other than that, were you happy with the arch/arm changes?

> There's two options on how to handle this.  Either accept the
> virt_to_idmap() change first, and then sort out the resulting difference,
> maybe by having ARM64 grow a virt_to_idmap() of its own (which is just
> a virt_to_phys()) or we do it the other way around.  Either way, we
> need a new macro, and the established name for it is virt_to_idmap().
> The order doesn't matter as the end result would be the same.

Ok. I guess it's easier to take Grygorii's patch first (with updated
commit message), and I'll fix the rest of my patches to cater for that.
I'll add that to the front of my series for the moment.

Thanks,
Mark.

[PATCH] ARM: psci: boot_secondary: replace __pa with virt_to_idmap

[Russell King - ARM Linux]

On Thu, Jul 02, 2015 at 11:29:37AM +0100, Mark Rutland wrote:
> On Tue, Jun 30, 2015 at 09:25:37PM +0100, Russell King - ARM Linux wrote:
> > On Tue, Jun 30, 2015 at 05:18:37PM +0100, Mark Rutland wrote:
> > > Hi,
> > > 
> > > On Tue, Jun 30, 2015 at 11:44:22AM +0100, Grygorii Strashko wrote:
> > > > On some PAE systems (e.g. TI Keystone), memory is above the 32-bit
> > > > addressable limit, and the interconnect provides an aliased view of
> > > > parts of physical memory in the 32-bit addressable space. This alias
> > > > is strictly for boot time usage, and is not otherwise usable because
> > > > of coherency limitations.
> > > > 
> > > > On such systems, the idmap mechanism has to be used to pass correct
> > > > boot address of secondary CPU to FW.
> > > > virt_to_idmap() will fall-back to existing virt_to_phys() macro if
> > > > such conversation is not required.
> > > > 
> > > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > > Cc: Nicolas Pitre <nico@linaro.org>
> > > > Cc: Santosh Shilimkar <ssantosh@kernel.org>
> > > > Cc: Vitaly Andrianov <vitalya@ti.com>
> > > > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> > > > ---
> > > >  arch/arm/kernel/psci_smp.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > The code itself looks sane to me, though the commit message would be
> > > better with Russell's suggested rewording.
> > > 
> > > This will also conflict with my migration to a common PSCI client
> > > implementation [1,2] (especially given the absence of virt_to_idmap on
> > > arm64), but I'm happy to fold it in or rebase atop of it.
> > > 
> > > Russell, are you able to take a look at the migration patch [2]? How
> > > would you prefer for the two patches to be taken?
> > 
> > Well, the first thing that stands out about patch [2] is that it's
> > introducing a load of magic hex numbers, where before we had a struct
> > with nice definitions.  (I'm thinking about highbank_suspend_finish()
> > and calxeda_idle_finish().)  This seems to be a backwards step.
> 
> The existing suspend parameter packing/unpacking didn't work now that
> there are multiple possible formats for the suspend parameter from
> PSCIv1.0 onwards, and the core PSCI code won't do any more
> packing/unpacking, treating the suspend parameter as a mostly opaque
> token.

... which is fine.

> I could retain the struct and packing functions for the two cases you
> mention, but it seemed somewhat misleading. I'll see if I can figure out
> something better than a raw hex value.

What's wrong with a few #defines for this?  It obviously is well enough
defined in existing pre-v1.0 implementations as there is an existing
structure to the parameter.

Can we not do something like:

#define PSCI_POWER_STATE_ID(x)			(x)
#define PSCI_POWER_STATE_TYPE_POWER_DOWN	0x000100000
#define PSCI_POWER_STATE_AFFINITY_LEVEL(x)	((x) << 24)

?

-- 
FTTC broadband for 0.8mile line: currently@10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH] ARM: psci: boot_secondary: replace __pa with virt_to_idmap

[Mark Rutland]

> > I could retain the struct and packing functions for the two cases you
> > mention, but it seemed somewhat misleading. I'll see if I can figure out
> > something better than a raw hex value.
> 
> What's wrong with a few #defines for this?

Nothing.

> It obviously is well enough defined in existing pre-v1.0
> implementations as there is an existing structure to the parameter.
> 
> Can we not do something like:
> 
> #define PSCI_POWER_STATE_ID(x)			(x)
> #define PSCI_POWER_STATE_TYPE_POWER_DOWN	0x000100000
> #define PSCI_POWER_STATE_AFFINITY_LEVEL(x)	((x) << 24)

Looks good.

We already have some constants defined in a UAPI header, so I'll see
about reusing those.

Thanks,
Mark.