[U-Boot] [PATCH v3 2/3] net: Add a command to access the EEPROM from ethernet devices

[Simon Glass]

Hi Alban,

On 15 October 2014 03:42, Alban Bedel <alban.bedel at avionic-design.de> wrote:
> On Tue, 14 Oct 2014 21:18:37 +0200
> Simon Glass <sjg at chromium.org> wrote:
>
>> Hi Joe,
>>
>> On 14 October 2014 21:14, Joe Hershberger <joe.hershberger at gmail.com> wrote:
>> >
>> > On Tue, Oct 14, 2014 at 12:21 PM, Simon Glass <sjg at chromium.org> wrote:
>> > >
>> > > Hi,
>> > >
>> > > On 14 October 2014 18:26, Alban Bedel <alban.bedel at avionic-design.de> wrote:
>> > > > Many ethernet devices use an EEPROM to store various settings, most
>> > > > commonly the device MAC address. But on some devices it can contains
>> > > > a lot more, for example USB device might also have many USB related
>> > > > parameters.
>> > > >
>> > > > This commit add a set of commands to read/write this EEPROM, write a
>> > > > default configuration and read/write the device MAC address. The
>> > > > defaults command allow priming the EEPROM for devices that need more
>> > > > than just a MAC address in the EEPROM.
>> > > >
>> > > > Signed-off-by: Alban Bedel <alban.bedel at avionic-design.de>
>> > > > ---
>> > > > v2: * No changes since v1
>> > > > v3: * Replace the dedicated 'eth_eeprom' command with a subcommand
>> > > >       to the newly introduced 'eth' command
>> > >
>> > > I see a few EEPROM implementations in the code base. It feels to me
>> > > like we need an EEPROM interface. In driver model terms this could be
>> > > a uclass. I started something here:
>> > >
>> > > http://patchwork.ozlabs.org/patch/399039/
>> > >
>> > > Of course we don't have DM for Ethernet yet - when we do I think a
>> > > child EEPROM device below the Ethernet would make sense. It could be
>> > > created by the Ethernet driver when it knows that this information
>> > > exists. But even without that I feel that the EEPROM should be
>> > > logically separated from Ethernet.
>> > >
>> > > So I suggest that instead of an #ifdef to adjust the Ethernet API, add
>> > > a pointer to another struct containing the EEPROM API and put it in
>> > > its own header. I also feel that there should ultimately be an eeprom
>> > > command which operates on these. Then the only Ethernet API call would
>> > > be to find the EEPROM pointer, if there is one.
>> > >
>> > > If someone feels like taking it on, driver model support for Ethernet
>> > > should be pretty easy. Or even EEPROM could be done now and this might
>> > > avoid churn. But I would be happy for this series to be applied as is
>> > > while working on a driver model version. I just don't feel we should
>> > > be adding new subsystems that don't use driver model.
>> >
>> > I agree that we shouldn't add new subsystems that don't use DM.  I think
>> > this (adding eeprom access to Ethernet) is a decent compromise until
>> > we have driver model for Ethernet.  I do like your idea of having an
>> > eeprom class / subsystem / command that can operate on this type of
>> > thing the same way no matter where it's connected, but it probably
>> > isn't a good idea to add an "eeprom" command for that without using DM
>> > first.
>> >
>> > So your idea sounds good, but it leaves me with one question... if we want
>> > to accept this now as it is, then do we want to introduce a new
>> > command (eth eeprom) when we already know we want to change its
>> > behavior or delete it once we add a uclass for eeproms?
>>
>> Good question, in general I suppose we want to avoid changing the
>> commands, although if you get to Ethernet in this merge window then it
>> might not matter much. One option is to indicate in the help that it
>> is a temporary command and the syntax may change.
>>
>
> A generic EEPROM driver / command sounds good, however we would still
> need a few ethernet driver specific commands like the ones to
> manipulate the MAC address.
>
> If possible I would really like to settle the command syntax, so I
> would suggest using the following scheme for the future eeprom command:
>
>   eeprom <dev> <command> [<args>...]
>
> If this command is also available internally with an interface such as:
>
>   int eeprom_cmd(struct eeprom_device *dev,
>                  int argc, char * const argv[]);
>
> the 'eth eeprom' implementation could easily provides its own commands
> on the eeprom while falling back on eeprom_cmd() for the generic ones.
> That would avoid any code duplication while keeping the 'eth eeprom'
> functionality.

There is already an eeprom command actually - I think Marex was
looking at tidying it up. But what you propose sounds good.

With driver model, the Ethernet would just provide another eeprom
device when enabled. Yes agreed you can have 'eth eeprom' for anything
specific to Ethernet.

>
> I'm unsure about the 'defaults' commands, I would tend to see it at
> the eeprom level as it might be needed in various situations. However
> it must be provided by the device, not the eeprom driver itself.
> It might also make sense to drop the callback in favour of a simple
> data array as it would allow setting it from device tree.

I'm not sure what this refers to sorry.

>
> Alban

Regards,
SImon