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

[Aneesh Kumar K.V]

Michael Ellerman  writes:

> Oliver O'Halloran  writes:
>
>> On Wed, Sep 28, 2016 at 12:43 PM, Aneesh Kumar K.V
>>  wrote:
>>> Balbir Singh  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
>> " 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

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

[Michael Ellerman]

Oliver O'Halloran  writes:

> On Wed, Sep 28, 2016 at 12:43 PM, Aneesh Kumar K.V
>  wrote:
>> Balbir Singh  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
> " 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"

[Oliver O'Halloran]

On Wed, Sep 28, 2016 at 12:43 PM, Aneesh Kumar K.V
 wrote:
> Balbir Singh  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
" 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 

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

[Aneesh Kumar K.V]

Balbir Singh  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 
> ---
>
>  - 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