RE: [PATCH net-next v2 2/2] net: ethtool: Add support for writing firmware blocks using EPL payload

[Danielle Ratson]

> From: Andrew Lunn <andrew@xxxxxxx>
> Sent: Wednesday, 11 September 2024 15:26
> To: Simon Horman <horms@xxxxxxxxxx>
> Cc: Danielle Ratson <danieller@xxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx;
> davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx;
> pabeni@xxxxxxxxxx; yuehaibing@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> Petr Machata <petrm@xxxxxxxxxx>
> Subject: Re: [PATCH net-next v2 2/2] net: ethtool: Add support for writing
> firmware blocks using EPL payload
> 
> On Wed, Sep 11, 2024 at 08:32:34AM +0100, Simon Horman wrote:
> > + Andrew Lunn
> >
> > On Tue, Sep 10, 2024 at 12:02:17PM +0300, Danielle Ratson wrote:
> > > In the CMIS specification for pluggable modules, LPL (Local Payload)
> > > and EPL (Extended Payload) are two types of data payloads used for
> > > managing various functions and features of the module.
> > >
> > > EPL payloads are used for more complex and extensive management
> > > functions that require a larger amount of data, so writing firmware
> > > blocks using EPL is much more efficient.
> > >
> > > Currently, only LPL payload is supported for writing firmware blocks
> > > to the module.
> > >
> > > Add support for writing firmware block using EPL payload, both to
> > > support modules that supports only EPL write mechanism, and to
> > > optimize the flashing process of modules that support LPL and EPL.
> > >
> > > Signed-off-by: Danielle Ratson <danieller@xxxxxxxxxx>
> > > Reviewed-by: Petr Machata <petrm@xxxxxxxxxx>
> > > ---
> > >
> > > Notes:
> > >     v2:
> > >     	* Initialize the variable 'bytes_written' before the first
> > >     	  iteration.
> >
> > Hi Danielle,
> >
> > Thanks for the update. From a doing-what-it-says-on-the-wrapper
> > perspective, this looks good to me.
> >
> > Reviewed-by: Simon Horman <horms@xxxxxxxxxx>
> >
> > I do note that there were some questions from Andrew Lunn (CCed) in v1
> > regarding the size of transfers over the bus. I see that you responded
> > to that. Thanks! But I do wonder if he has any further comments.
> 
> I do wounder where the speedup comes from.
> 
> > The LPL contains ap to 128 bytes, and the EPL up to 2048
> 
> Are 128 consecutive 16 byte transfers per block really faster than 8
> consecutive transfers per block? If you have an efficient I2C master, both
> should be doing 400Kbp. If the master is inefficient, you end up with the same
> amount of dead time on the wire.
> 
> And does the standard really say you can fragment the block at the I2C layer?
> 
> I suspect in the end we will have an API between the core and the driver to ask
> it what size of block it actually supports. But we can probably add that when
> we need it.
> 
> 	Andrew

Hi Andrew,

In both cases we transfer the same size of data, which corresponds to the size of the firmware image, to the module.
Moreover, in both cases the same size of data is passing on the wire, which depends on the wire obligations.

But, instead of running #n "0103h: Write FW Block LPL" commands (see section 9.7.4 in CMIS 5.2) with up to 128 bytes, we are running #n/16 "0104h: Write FW Block EPL" commands (see section 9.7.5 in CMIS 5.2) with up to 2048 bytes.
That means that instead of processing #n commands and sending back to the core the status for each one, we do it for only #n/16.

The standard does not say anything about the I2C layer, but the speedup doesn’t lie in that.

Thanks,
Danielle