Re: [PATCH net-next v6 1/9] net: dsa: microchip: enable EEE support

[Arun.Ramadoss]

Hi Oleksij,
On Thu, 2023-02-09 at 06:48 +0100, Oleksij Rempel wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> Hi Arun,
> 
> On Thu, Feb 09, 2023 at 04:07:11AM +0000, Arun.Ramadoss@xxxxxxxxxxxxx
>  wrote:
> > Hi Oleksij,
> > On Wed, 2023-02-08 at 11:32 +0100, Oleksij Rempel wrote:
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > know the content is safe
> > > 
> > > Some of KSZ9477 family switches provides EEE support.
> > 
> > nit: If you can elaborate what are the chip supports will be good.
> 
> Do you mean list of supported chips or link speeds with EEE support?

Yes, since you mentioned some of KSZ9477, I thought it will be better
to mention the supported chips in commit description.

> 
> > > To enable it, we
> > > just need to register set_mac_eee/set_mac_eee handlers and
> > > validate
> > > supported chip version and port.
> > > 
> > > Signed-off-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
> > > Reviewed-by: Andrew Lunn <andrew@xxxxxxx>
> > > ---
> > >  drivers/net/dsa/microchip/ksz_common.c | 65
> > > ++++++++++++++++++++++++++
> > >  1 file changed, 65 insertions(+)
> > > 
> > > diff --git a/drivers/net/dsa/microchip/ksz_common.c
> > > b/drivers/net/dsa/microchip/ksz_common.c
> > > index 46becc0382d6..0a2d78253d17 100644
> > > --- a/drivers/net/dsa/microchip/ksz_common.c
> > > +++ b/drivers/net/dsa/microchip/ksz_common.c
> > > @@ -2673,6 +2673,69 @@ static int ksz_max_mtu(struct dsa_switch
> > > *ds,
> > > int port)
> > >         return -EOPNOTSUPP;
> > >  }
> > > 
> > > +static int ksz_get_mac_eee(struct dsa_switch *ds, int port,
> > > +                          struct ethtool_eee *e)
> > > +{
> > > +       int ret;
> > > +
> > > +       ret = ksz_validate_eee(ds, port);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       /* There is no documented control of Tx LPI
> > > configuration. */
> > > +       e->tx_lpi_enabled = true;
> > 
> > Blank line before comment will increase readability.
> > 
> > > +       /* There is no documented control of Tx LPI timer.
> > > According
> > > to tests
> > > +        * Tx LPI timer seems to be set by default to minimal
> > > value.
> > > +        */
> > > +       e->tx_lpi_timer = 0;
> > 
> > for lpi_enabled, you have used true and for lpi_timer you have used
> > 0.
> > It can be consistent either true/false or 1/0.
> 
> tx_lpi_enabled has only on/off states. This is why i use bool values.
> 
> tx_lpi_timer is a range in microseconds to re-enter LPI mode.

Got it. I overlooked the variable data type.

> 
> Beside, tx_lpi_timer can be used to optimize EEE for some
> applications.
> For example do not start Low Power Idle for some usecs so latency
> will
> be reduced. Are there some secret register to configure this value?

I am not aware of it. 

> 
> Regards,
> Oleksij
> --
> Pengutronix
> e.K.                           |                             |
> Steuerwalder Str. 21                       | 
> http://www.pengutronix.de/e/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-
> 0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-
> 5555 |