[PATCH] Fix "ibm,processor-radix-AP-encodings"

[PATCH] Fix "ibm,processor-radix-AP-encodings"

[Balbir Singh]

The top 3 bits of the lower order byte should contain the
AP encoding, we assume the top 3 bits of the MSB.

Signed-off-by: Balbir Singh <bsingharora@gmail.com>
---

 - Detected while reviewing Chris Smart's patch to add radix-AP-encoding
   to skiboot
 - Also fixed typo (sift/shift)

 arch/powerpc/mm/pgtable-radix.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
index af897d9..d525b0b 100644
--- a/arch/powerpc/mm/pgtable-radix.c
+++ b/arch/powerpc/mm/pgtable-radix.c
@@ -245,10 +245,10 @@ static int __init radix_dt_scan_page_sizes(unsigned long node,
 
 		struct mmu_psize_def *def;
 
-		/* top 3 bit is AP encoding */
-		shift = be32_to_cpu(prop[0]) & ~(0xe << 28);
-		ap = be32_to_cpu(prop[0]) >> 29;
-		pr_info("Page size sift = %d AP=0x%x\n", shift, ap);
+		/* top 3 bits of the lower order byte is AP encoding */
+		shift = be32_to_cpu(prop[0]) & 0x1f;
+		ap = (be32_to_cpu(prop[0]) >> 5) & 0x7;
+		pr_info("Page size shift = %d AP=0x%x\n", shift, ap);
 
 		idx = get_idx_from_shift(shift);
 		if (idx < 0)
-- 
2.5.5

Re: [PATCH] Fix "ibm,processor-radix-AP-encodings"

[Aneesh Kumar K.V]

Balbir Singh <bsingharora@gmail.com> writes:

> The top 3 bits of the lower order byte should contain the
> AP encoding, we assume the top 3 bits of the MSB.


Are you sure, Power architecture documents always confuse about MSB vs
lowe order bytes. ?

>
> Signed-off-by: Balbir Singh <bsingharora@gmail.com>
> ---
>
>  - Detected while reviewing Chris Smart's patch to add radix-AP-encoding
>    to skiboot
>  - Also fixed typo (sift/shift)
>
>  arch/powerpc/mm/pgtable-radix.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
> index af897d9..d525b0b 100644
> --- a/arch/powerpc/mm/pgtable-radix.c
> +++ b/arch/powerpc/mm/pgtable-radix.c
> @@ -245,10 +245,10 @@ static int __init radix_dt_scan_page_sizes(unsigned long node,
>
>  		struct mmu_psize_def *def;
>
> -		/* top 3 bit is AP encoding */
> -		shift = be32_to_cpu(prop[0]) & ~(0xe << 28);
> -		ap = be32_to_cpu(prop[0]) >> 29;
> -		pr_info("Page size sift = %d AP=0x%x\n", shift, ap);
> +		/* top 3 bits of the lower order byte is AP encoding */
> +		shift = be32_to_cpu(prop[0]) & 0x1f;
> +		ap = (be32_to_cpu(prop[0]) >> 5) & 0x7;
> +		pr_info("Page size shift = %d AP=0x%x\n", shift, ap);
>
>  		idx = get_idx_from_shift(shift);
>  		if (idx < 0)

-aneesh

Re: [PATCH] Fix "ibm,processor-radix-AP-encodings"

[Oliver O'Halloran]

On Wed, Sep 28, 2016 at 12:43 PM, Aneesh Kumar K.V
<aneesh.kumar@linux.vnet.ibm.com> wrote:
> Balbir Singh <bsingharora@gmail.com> writes:
>
>> The top 3 bits of the lower order byte should contain the
>> AP encoding, we assume the top 3 bits of the MSB.

Balbir, could you reword this so it says "Currently we wrongly assume
<blah>" or similar. The current commit message made me think you were
changing it to look at the top 3 bits of the MSB rather than changing
it look at the LSB.

> Are you sure, Power architecture documents always confuse about MSB vs
> lowe order bytes. ?

PAPR seems to be pretty consistent about "low order" meaning "least
significant." Additionally the PAPR that describes
ibm,processor-radix-AP-encodings says that it is formatted this way so
it can be used when constructing the register argument to tlbie. The
modes of tlbie that use the AP field place it in bits 56:59 so I think
Balbir's fix is correct.

Reviewed-By: Oliver O'Halloran <oohall@gmail.com>

Re: [PATCH] Fix "ibm,processor-radix-AP-encodings"

[Michael Ellerman]

Oliver O'Halloran <oohall@gmail.com> writes:

> On Wed, Sep 28, 2016 at 12:43 PM, Aneesh Kumar K.V
> <aneesh.kumar@linux.vnet.ibm.com> wrote:
>> Balbir Singh <bsingharora@gmail.com> writes:
>>
>>> The top 3 bits of the lower order byte should contain the
>>> AP encoding, we assume the top 3 bits of the MSB.
>
> Balbir, could you reword this so it says "Currently we wrongly assume
> <blah>" or similar. The current commit message made me think you were
> changing it to look at the top 3 bits of the MSB rather than changing
> it look at the LSB.

Yeah please reword it to describe what the current code does, why it's
incorrect, and how we are fixing it.

cheers

Re: [PATCH] Fix "ibm,processor-radix-AP-encodings"

[Aneesh Kumar K.V]

Michael Ellerman <mpe@ellerman.id.au> writes:

> Oliver O'Halloran <oohall@gmail.com> writes:
>
>> On Wed, Sep 28, 2016 at 12:43 PM, Aneesh Kumar K.V
>> <aneesh.kumar@linux.vnet.ibm.com> wrote:
>>> Balbir Singh <bsingharora@gmail.com> writes:
>>>
>>>> The top 3 bits of the lower order byte should contain the
>>>> AP encoding, we assume the top 3 bits of the MSB.
>>
>> Balbir, could you reword this so it says "Currently we wrongly assume
>> <blah>" or similar. The current commit message made me think you were
>> changing it to look at the top 3 bits of the MSB rather than changing
>> it look at the LSB.
>
> Yeah please reword it to describe what the current code does, why it's
> incorrect, and how we are fixing it.
>

I have an updated patch for this, which redo the AP-encoding parsing
based on spec update. Wil send it to the list once we finalize the
details

-aneesh