Re: [EXT] Re: [v1,ethtool] ethtool: add setting frame preemption of traffic classes

[Murali Karicheri <m-karicheri2@ti.com>]

On 12/04/2019 03:14 AM, Po Liu wrote:
> Hi Murali, Ivan,
> 
> Thank you for your feedback. Maybe it is better to use "RFC" type for these patches for the discussion.
> 
Yes please.

Murali
> 
> Br,
> Po Liu
> 
>> -----Original Message-----
>> From: Murali Karicheri <m-karicheri2@ti.com>
>> Sent: 2019年12月4日 1:42
>> To: Po Liu <po.liu@nxp.com>; rmk+kernel@armlinux.org.uk;
>> linville@tuxdriver.com; netdev-owner@vger.kernel.org; davem@davemloft.net;
>> linux-kernel@vger.kernel.org; netdev@vger.kernel.org;
>> vinicius.gomes@intel.com; simon.horman@netronome.com; Claudiu Manoil
>> <claudiu.manoil@nxp.com>; Vladimir Oltean <vladimir.oltean@nxp.com>;
>> Xiaoliang Yang <xiaoliang.yang_1@nxp.com>; Roy Zang <roy.zang@nxp.com>;
>> Mingkai Hu <mingkai.hu@nxp.com>; Jerry Huang <jerry.huang@nxp.com>; Leo
>> Li <leoyang.li@nxp.com>
>> Subject: [EXT] Re: [v1,ethtool] ethtool: add setting frame preemption of traffic
>> classes
>>
>> Caution: EXT Email
>>
>> Hi Po Liu,
>>
>> Thanks for working on this! Some suggestion below as we are working on adding
>> this support to Texas Instrument's CPSW driver on AM65x family of SoCs as well.
>> TRM for that is provided below for your reference.
>> Relevant section for IET (Frame pre-emption) is
>>
>> 12.2.1.4.6.6.1IET Configuration
>>
>> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.ti.c
>> om%2Flit%2Fug%2Fspruid7d%2Fspruid7d.pdf&amp;data=02%7C01%7Cpo.liu%
>> 40nxp.com%7C15544a9db1ff422e2fdf08d77817647f%7C686ea1d3bc2b4c6fa
>> 92cd99c5c301635%7C0%7C1%7C637109914194485742&amp;sdata=VD5kg%
>> 2FY1SDDWjwMZHjgUNlFrcvXnDvdIPGblWkx4DXs%3D&amp;reserved=0
>>
>> On 12/03/2019 11:27 AM, Ivan Khoronzhuk wrote:
>>> On Wed, Nov 27, 2019 at 09:58:52AM +0000, Po Liu wrote:
>>>
>>> Hi Po Liu,
>>>
>>>> IEEE Std 802.1Qbu standard defined the frame preemption of port
>>>> trffic classes. User can set a value to hardware. The value will be
>>>> translated to a binary, each bit represent a traffic class.
>>>> Bit "1" means preemptable traffic class. Bit "0" means express
>>>> traffic class.  MSB represent high number traffic class.
>>>>
>>>> ethtool -k devname
>>>>
>>>> This command would show if the tx-preemption feature is available.
>>>> If hareware set preemption feature. The property would be a fixed
>>>> value 'on' if hardware support the frame preemption. Feature would
>>>> show a fixed value 'off' if hardware don't support the frame preemption.
>>>>
>>>> ethtool devname
>>>>
>>>> This command would show include an item 'preemption'. A following
>>>> value '0' means all traffic classes are 'express'. A value none zero
>>>> means traffic classes preemption capabilities. The value will be
>>>> translated to a binary, each bit represent a traffic class. Bit '1'
>>>> means preemptable traffic class. Bit '0' means express traffic class.
>>>> MSB represent high number traffic class.
>>>>
>>>> ethtool -s devname preemption N
>>>
>>> What about other potential parameters like MAC fragment size, mac hold?
>>> Shouldn't be it considered along with other FP parameters to provide
>>> correct interface later?
>>>
>>> Say, preemption, lets name it fp-mask or frame-preemption-mask.
>>> Then other potential setting can be similar and added later:
>>>
>>> frame-preemption-mask
>>> frame-preemption-fragsize
>>> frame-preemption-machold
>> Need additional capabilities as described by Ivan above. Thanks Ivan!
>>
>> So it would be better to use feature/sub-parameter format so that it can be
>> extended as needed in the future.
>>
>> For express/Preemptable mask setting it becomes
>>
>> ethtool -s devname frame-preemption tc-mask  N
>>
>> For setting min fragment size
>>
>> ethtool -s devname frame-preemption min-fragsize 64
>>
> 
> I thought the fragment size set 64 is enough for Qbv.  Anyway, if you prefer more details setting. I think it is better to set a specific serial type. For example, '-p' for set  tc-mask  N  and min-fragsize 64, '-P' for status.
> 
>> Also the device may be capable of doing Verify process to detect the capability
>> of neighbor device and show the status. So we should have a way to show this
> 
> The verify process maybe disabled as default.
> 
>> status as well when user type
>>
>> ethtool devname
>>
>> We are working currently to add this feature to CPSW driver on AM65x which
>> will be upstream-ed soon. So want to have this done in an way that we can
>> extend it later.
>> Similarly for taprio, there are some parameters that user might want to tune
>> such as Max SDU size per tc class. I hope we could use ethtool to set the same
>> on a similar way.
> 
> Furthermore, this setting could be extend for a serial setting for mac and traffic class.
> 
>>
>> Thanks
>>
>> Murali
>>
>>> ....
>>>
>>> mac-hold it's rather flag, at least I've used it as priv-flag.
>>> so can or so
>>>
>>> frame-preemption-flags
>>>
>>>>
>>>> This command would set which traffic classes are frame preemptable.
>>>> The value will be translated to a binary, each bit represent a
>>>> traffic class. Bit '1' means preemptable traffic class. Bit '0'
>>>> means express traffic class. MSB represent high number traffic class.
>>>>
>>>> Signed-off-by: Po Liu <Po.Liu@nxp.com>
>>>> ---
>>>> ethtool-copy.h |  6 +++++-
>>>> ethtool.8.in   |  8 ++++++++
>>>> ethtool.c      | 18 ++++++++++++++++++
>>>> 3 files changed, 31 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/ethtool-copy.h b/ethtool-copy.h index 9afd2e6..e04bdf3
>>>> 100644
>>>> --- a/ethtool-copy.h
>>>> +++ b/ethtool-copy.h
>>>> @@ -1662,6 +1662,9 @@ static __inline__ int
>>>> ethtool_validate_duplex(__u8 duplex)
>>>> #define AUTONEG_DISABLE        0x00
>>>> #define AUTONEG_ENABLE        0x01
>>>>
>>>> +/* Disable preemtion. */
>>>> +#define PREEMPTION_DISABLE    0x0
>>>> +
>>>> /* MDI or MDI-X status/control - if MDI/MDI_X/AUTO is set then
>>>>   * the driver is required to renegotiate link  */ @@ -1878,7 +1881,8
>>>> @@ struct ethtool_link_settings {
>>>>      __s8    link_mode_masks_nwords;
>>>>      __u8    transceiver;
>>>>      __u8    reserved1[3];
>>>> -    __u32    reserved[7];
>>>> +    __u32    preemption;
>>>> +    __u32    reserved[6];
>>>>      __u32    link_mode_masks[0];
>>>>      /* layout of link_mode_masks fields:
>>>>       * __u32 map_supported[link_mode_masks_nwords];
>>>> diff --git a/ethtool.8.in b/ethtool.8.in index 062695a..7d612b2
>>>> 100644
>>>> --- a/ethtool.8.in
>>>> +++ b/ethtool.8.in
>>>> @@ -236,6 +236,7 @@ ethtool \- query or control network driver and
>>>> hardware settings
>>>> .B2 autoneg on off
>>>> .BN advertise
>>>> .BN phyad
>>>> +.BN preemption
>>>> .B2 xcvr internal external
>>>> .RB [ wol \ \*(WO]
>>>> .RB [ sopass \ \*(MA]
>>>> @@ -703,6 +704,13 @@ lB    l    lB.
>>>> .BI phyad \ N
>>>> PHY address.
>>>> .TP
>>>> +.BI preemption \ N
>>>> +Set preemptable traffic classes by bits.
>>>> +.B A
>>>> +value will be translated to a binary, each bit represent a traffic
>>>> class.
>>>> +Bit "1" means preemptable traffic class. Bit "0" means express
>>>> traffic class.
>>>> +MSB represent high number traffic class.
>>>> +.TP
>>>> .A2 xcvr internal external
>>>> Selects transceiver type. Currently only internal and external can be
>>>> specified, in the future further types might be added.
>>>> diff --git a/ethtool.c b/ethtool.c
>>>> index acf183d..d5240f8 100644
>>>> --- a/ethtool.c
>>>> +++ b/ethtool.c
>>>> @@ -928,6 +928,12 @@ dump_link_usettings(const struct
>>>> ethtool_link_usettings *link_usettings)
>>>>          }
>>>>      }
>>>>
>>>> +    if (link_usettings->base.preemption == PREEMPTION_DISABLE)
>>>> +        fprintf(stdout, "    Preemption: 0x0 (off)\n");
>>>> +    else
>>>> +        fprintf(stdout, "    Preemption: 0x%x\n",
>>>> +            link_usettings->base.preemption);
>>>> +
>>>>      return 0;
>>>> }
>>>>
>>>> @@ -2869,6 +2875,7 @@ static int do_sset(struct cmd_context *ctx)
>>>>      int port_wanted = -1;
>>>>      int mdix_wanted = -1;
>>>>      int autoneg_wanted = -1;
>>>> +    int preemption_wanted = -1;
>>>>      int phyad_wanted = -1;
>>>>      int xcvr_wanted = -1;
>>>>      u32 *full_advertising_wanted = NULL; @@ -2957,6 +2964,12 @@
>>>> static int do_sset(struct cmd_context *ctx)
>>>>              } else {
>>>>                  exit_bad_args();
>>>>              }
>>>> +        } else if (!strcmp(argp[i], "preemption")) {
>>>> +            gset_changed = 1;
>>>> +            i += 1;
>>>> +            if (i >= argc)
>>>> +                exit_bad_args();
>>>> +            preemption_wanted = get_u32(argp[i], 16);
>>>>          } else if (!strcmp(argp[i], "advertise")) {
>>>>              gset_changed = 1;
>>>>              i += 1;
>>>> @@ -3094,6 +3107,9 @@ static int do_sset(struct cmd_context *ctx)
>>>>              }
>>>>              if (autoneg_wanted != -1)
>>>>                  link_usettings->base.autoneg = autoneg_wanted;
>>>> +            if (preemption_wanted != -1)
>>>> +                link_usettings->base.preemption
>>>> +                    = preemption_wanted;
>>>>              if (phyad_wanted != -1)
>>>>                  link_usettings->base.phy_address = phyad_wanted;
>>>>              if (xcvr_wanted != -1)
>>>> @@ -3186,6 +3202,8 @@ static int do_sset(struct cmd_context *ctx)
>>>>                  fprintf(stderr, "  not setting transceiver\n");
>>>>              if (mdix_wanted != -1)
>>>>                  fprintf(stderr, "  not setting mdix\n");
>>>> +            if (preemption_wanted != -1)
>>>> +                fprintf(stderr, "  not setting preemption\n");
>>>>          }
>>>>      }
>>>>
>>>> --
>>>> 2.17.1
>>>>
>>>
>