Re: [PATCH net-next 17/18] net: dsa: mv88e6xxx: support the VTU Page bit

[Andrew Lunn]

> That's not a big deal though, one can make it clearer later if needed.

Agreed. Lets keep it as is.

Andrew

Re: [PATCH net-next 17/18] net: dsa: mv88e6xxx: support the VTU Page bit

[Andrew Lunn]

> That's not a big deal though, one can make it clearer later if needed.

Agreed. Lets keep it as is.

Andrew

Re: [PATCH net-next 17/18] net: dsa: mv88e6xxx: support the VTU Page bit

[Vivien Didelot]

Hi Andrew,

Andrew Lunn  writes:

>> +if (val & GLOBAL_VTU_VID_PAGE)
>> +entry->vid |= 0x1000;
>
> I'm undecided myself, so i will just bring it up for discussion.
>
> Maybe it would be more readable to say:
>
>   entry->vid += 4096;
>
> ???

VID values are usually 12-bit. Marvell uses 13-bit VID values for the
88E6390 family. Because Marvell is Marvell, the 13th bit of the VID is
the 14th bit of the VID register.

It feels natural to me to stay bitwise here when handling VID values.

That's not a big deal though, one can make it clearer later if needed.

Thanks,

Vivien

Re: [PATCH net-next 17/18] net: dsa: mv88e6xxx: support the VTU Page bit

[Vivien Didelot]

Hi Andrew,

Andrew Lunn  writes:

>> +if (val & GLOBAL_VTU_VID_PAGE)
>> +entry->vid |= 0x1000;
>
> I'm undecided myself, so i will just bring it up for discussion.
>
> Maybe it would be more readable to say:
>
>   entry->vid += 4096;
>
> ???

VID values are usually 12-bit. Marvell uses 13-bit VID values for the
88E6390 family. Because Marvell is Marvell, the 13th bit of the VID is
the 14th bit of the VID register.

It feels natural to me to stay bitwise here when handling VID values.

That's not a big deal though, one can make it clearer later if needed.

Thanks,

Vivien

Re: [PATCH net-next 17/18] net: dsa: mv88e6xxx: support the VTU Page bit

[Andrew Lunn]

On Wed, Apr 26, 2017 at 11:53:35AM -0400, Vivien Didelot wrote:
> Newer chips such as the 88E6390 have a VTU Page bit in the VTU VID
> register to specify a 13th bit for the VID. This can be used to support
> 8K VLANs.

Hi Vivien

At the moment, this code appears to be generic to all chips. Do we
need checks to ensure we don't look at this bit on older devices where
it is not valid? Particularly on read. On write, we already verify the
vid is less than info->max_vids, so i don't think that is a problem.

> When dumping the whole VTU, all VID bits must be set to one, including
> this VTU Page bit. Add support for VID greater than 4095.
> 
> Signed-off-by: Vivien Didelot 
> ---
>  drivers/net/dsa/mv88e6xxx/global1_vtu.c | 7 +++
>  drivers/net/dsa/mv88e6xxx/mv88e6xxx.h   | 1 +
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/global1_vtu.c 
> b/drivers/net/dsa/mv88e6xxx/global1_vtu.c
> index cb0f3359d60b..2ad080291208 100644
> --- a/drivers/net/dsa/mv88e6xxx/global1_vtu.c
> +++ b/drivers/net/dsa/mv88e6xxx/global1_vtu.c
> @@ -95,6 +95,10 @@ static int mv88e6xxx_g1_vtu_vid_read(struct mv88e6xxx_chip 
> *chip,
>   return err;
>  
>   entry->vid = val & 0xfff;
> +
> + if (val & GLOBAL_VTU_VID_PAGE)
> + entry->vid |= 0x1000;
> +

I'm undecided myself, so i will just bring it up for discussion.

Maybe it would be more readable to say:

entry->vid += 4096;

???

Andrew

Re: [PATCH net-next 17/18] net: dsa: mv88e6xxx: support the VTU Page bit

[Andrew Lunn]

On Wed, Apr 26, 2017 at 11:53:35AM -0400, Vivien Didelot wrote:
> Newer chips such as the 88E6390 have a VTU Page bit in the VTU VID
> register to specify a 13th bit for the VID. This can be used to support
> 8K VLANs.

Hi Vivien

At the moment, this code appears to be generic to all chips. Do we
need checks to ensure we don't look at this bit on older devices where
it is not valid? Particularly on read. On write, we already verify the
vid is less than info->max_vids, so i don't think that is a problem.

> When dumping the whole VTU, all VID bits must be set to one, including
> this VTU Page bit. Add support for VID greater than 4095.
> 
> Signed-off-by: Vivien Didelot 
> ---
>  drivers/net/dsa/mv88e6xxx/global1_vtu.c | 7 +++
>  drivers/net/dsa/mv88e6xxx/mv88e6xxx.h   | 1 +
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/global1_vtu.c 
> b/drivers/net/dsa/mv88e6xxx/global1_vtu.c
> index cb0f3359d60b..2ad080291208 100644
> --- a/drivers/net/dsa/mv88e6xxx/global1_vtu.c
> +++ b/drivers/net/dsa/mv88e6xxx/global1_vtu.c
> @@ -95,6 +95,10 @@ static int mv88e6xxx_g1_vtu_vid_read(struct mv88e6xxx_chip 
> *chip,
>   return err;
>  
>   entry->vid = val & 0xfff;
> +
> + if (val & GLOBAL_VTU_VID_PAGE)
> + entry->vid |= 0x1000;
> +

I'm undecided myself, so i will just bring it up for discussion.

Maybe it would be more readable to say:

entry->vid += 4096;

???

Andrew