[Skiboot] [PATCH v2 05/10] npu2: Use read-modify-write in npu2_assign_gmb()

[Alistair Popple]

On Fri, 21 Jul 2017 10:47:42 AM Reza Arbab wrote:
> We currently set the GPU memory BAR value by OR'ing our values with what
> is already set. This makes for a conflict if Hostboot or other system
> boot software has already assigned different values than us.
> 
> Do a read-modify-write here instead of OR'ing our values in.
> 
> Signed-off-by: Reza Arbab <arbab at linux.vnet.ibm.com>
> Cc: Alistair Popple <alistair at popple.id.au>
> Cc: Andrew Donnellan <andrew.donnellan at au1.ibm.com>
> Cc: Frederic Barrat <fbarrat at linux.vnet.ibm.com>
> ---
>  hw/npu2.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/npu2.c b/hw/npu2.c
> index c7e636f..a752ab5 100644
> --- a/hw/npu2.c
> +++ b/hw/npu2.c
> @@ -597,7 +597,17 @@ static int npu2_assign_gmb(struct npu2_dev *ndev)
>  
>  	/* Base address is in GB */
>  	base >>= 30;
> -	val = SETFIELD(NPU2_MEM_BAR_SEL_MEM, 0ULL, 4);
> +
> +	reg = NPU2_REG_OFFSET(NPU2_STACK_STCK_0 + NPU2DEV_STACK(ndev),
> +			      NPU2_BLOCK_SM_0,
> +			      NPU2_GPU0_MEM_BAR);
> +
> +	val = npu2_read(p, reg);
> +	old_val = val;
> +	if (NPU2DEV_BRICK(ndev))
> +		val <<= 32;

I think I figured out how this works, but it was a little hard to follow ... or
maybe it's just I need more coffee.

> +
> +	val = SETFIELD(NPU2_MEM_BAR_SEL_MEM, val, 4);
>  	val = SETFIELD(NPU2_MEM_BAR_NODE_ADDR, val, base);
>  	val = SETFIELD(NPU2_MEM_BAR_GROUP | NPU2_MEM_BAR_CHIP, val, p->chip_id);
>  	val = SETFIELD(NPU2_MEM_BAR_POISON, val, 1);
> @@ -631,13 +641,7 @@ static int npu2_assign_gmb(struct npu2_dev *ndev)
>  	mode += ndev->bdfn & 0x7;
>  	val = SETFIELD(NPU2_MEM_BAR_MODE, val, mode);
>  	if (NPU2DEV_BRICK(ndev))
> -		val >>= 32;
> -	reg = NPU2_REG_OFFSET(NPU2_STACK_STCK_0 + NPU2DEV_STACK(ndev),
> -			      NPU2_BLOCK_SM_0,
> -			      NPU2_GPU0_MEM_BAR);
> -
> -	old_val = npu2_read(p, reg);
> -	val |= old_val;
> +		val = (old_val & 0xffffffff00000000) | (val >> 32);

Personally I think it would be clearer if we just calculated the BAR setup in
val with no shift and then use SETFIELD to assign it to the upper or lower half
of the SCOM register. Eg:

val = npu2_read(p, reg)
gmb = SETFIELD(NPU2_MEM_BAR_SEL_MEM, *0*, 4)
...
if (NPU2DEV_BIRCK(ndev))
   val = SETFIELD(PPC_BITMASK(0, 31), val, gmb);
else
   val = SETFIELD(PPC_BITMASK(32, 63), val, gmb);
npu2_write(p, reg, val);

Of course this would be a little different on DD2 where you only have to change
the register.

- Alistair

>  	npu2_write(p, reg, val);
>  	reg = NPU2_REG_OFFSET(NPU2_STACK_STCK_0 + NPU2DEV_STACK(ndev),
>