Re: [PATCH] [RFC] net: smc911x: Drop the standalone EEPROM example
On Tue, Mar 17, 2020 at 07:54:51PM -0500, Joe Hershberger wrote:
On Tue, Mar 17, 2020 at 1:55 PM Tom Rini trini@konsulko.com wrote:
On Tue, Mar 17, 2020 at 07:53:58PM +0100, Marek Vasut wrote:
On 3/17/20 7:44 PM, Tom Rini wrote:
On Tue, Mar 17, 2020 at 07:43:11PM +0100, Marek Vasut wrote:
On 3/17/20 7:42 PM, Tom Rini wrote:
On Tue, Mar 17, 2020 at 07:39:49PM +0100, Marek Vasut wrote: > On 3/17/20 7:30 PM, Tom Rini wrote: >> On Tue, Mar 17, 2020 at 07:23:07PM +0100, Marek Vasut wrote: >>> On 3/17/20 7:10 PM, Masahiro Yamada wrote: >>>> On Sun, Mar 15, 2020 at 8:19 AM Marek Vasut marek.vasut@gmail.com wrote: >>>>> >>>>> Drop the example, for two reasons. First, it is tapping directly into >>>>> the IO accessors of the SMC911x, while it should instead go through >>>>> the net device API. Second, this makes conversion of the SMC911x driver >>>>> to DM real hard. >>>>> >>>>> Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com >>>>> Cc: Joe Hershberger joe.hershberger@ni.com >>>>> Cc: Tom Rini trini@konsulko.com >>>>> --- >>>>> examples/standalone/Makefile | 1 - >>>>> examples/standalone/smc911x_eeprom.c | 379 --------------------------- >>>>> 2 files changed, 380 deletions(-) >>>>> delete mode 100644 examples/standalone/smc911x_eeprom.c >>>> >>>> >>>> Yeah, I was disturbed by this example code. >>>> >>>> I agree we should drop it. >>>> >>>> Reviewed-by: Masahiro Yamada yamada.masahiro@socionext.com >>> >>> Well I dunno. Can this be rewritten on top of DM somehow ? Do we even >>> have U-Boot application API to access DM EEPROM ? >> >> We should just drop it I think. The biggest surface we have today for >> external application is EFI application now, not U-Boot specific API. >> We can't drop the API but we don't expand it without very good reason. > > But this drops the ability to access the SMC911x EEPROM too. > So maybe we need some DM EEPROM implementation in the SMC911x driver ? > Does anyone have SMC911x with an external EEPROM ?
All this does is drop an example. I don't see anything removing API code itself.
Where did I say anything about API code ?
Nowhere, which is my point. You're just dropping an example, not the ability to do $X.
If $X is ability to access the EEPROM, then I am dropping $X here.
No, you're dropping an example of doing $X.
Correct. But the move to DM in the driver will drop the functions this example was using, no?
If it was using something that's not in <_exports.h> I don't see that as a problem. A standalone app could do whatever it likes with the hardware and needs to restore the hardware before passing control back to U-Boot (if it's doing that).
Hi Tom,
On Tue, Mar 17, 2020 at 7:59 PM Tom Rini trini@konsulko.com wrote:
On Tue, Mar 17, 2020 at 07:54:51PM -0500, Joe Hershberger wrote:
On Tue, Mar 17, 2020 at 1:55 PM Tom Rini trini@konsulko.com wrote:
On Tue, Mar 17, 2020 at 07:53:58PM +0100, Marek Vasut wrote:
On 3/17/20 7:44 PM, Tom Rini wrote:
On Tue, Mar 17, 2020 at 07:43:11PM +0100, Marek Vasut wrote:
On 3/17/20 7:42 PM, Tom Rini wrote: > On Tue, Mar 17, 2020 at 07:39:49PM +0100, Marek Vasut wrote: >> On 3/17/20 7:30 PM, Tom Rini wrote: >>> On Tue, Mar 17, 2020 at 07:23:07PM +0100, Marek Vasut wrote: >>>> On 3/17/20 7:10 PM, Masahiro Yamada wrote: >>>>> On Sun, Mar 15, 2020 at 8:19 AM Marek Vasut marek.vasut@gmail.com wrote: >>>>>> >>>>>> Drop the example, for two reasons. First, it is tapping directly into >>>>>> the IO accessors of the SMC911x, while it should instead go through >>>>>> the net device API. Second, this makes conversion of the SMC911x driver >>>>>> to DM real hard. >>>>>> >>>>>> Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com >>>>>> Cc: Joe Hershberger joe.hershberger@ni.com >>>>>> Cc: Tom Rini trini@konsulko.com >>>>>> --- >>>>>> examples/standalone/Makefile | 1 - >>>>>> examples/standalone/smc911x_eeprom.c | 379 --------------------------- >>>>>> 2 files changed, 380 deletions(-) >>>>>> delete mode 100644 examples/standalone/smc911x_eeprom.c >>>>> >>>>> >>>>> Yeah, I was disturbed by this example code. >>>>> >>>>> I agree we should drop it. >>>>> >>>>> Reviewed-by: Masahiro Yamada yamada.masahiro@socionext.com >>>> >>>> Well I dunno. Can this be rewritten on top of DM somehow ? Do we even >>>> have U-Boot application API to access DM EEPROM ? >>> >>> We should just drop it I think. The biggest surface we have today for >>> external application is EFI application now, not U-Boot specific API. >>> We can't drop the API but we don't expand it without very good reason. >> >> But this drops the ability to access the SMC911x EEPROM too. >> So maybe we need some DM EEPROM implementation in the SMC911x driver ? >> Does anyone have SMC911x with an external EEPROM ? > > All this does is drop an example. I don't see anything removing API > code itself.
Where did I say anything about API code ?
Nowhere, which is my point. You're just dropping an example, not the ability to do $X.
If $X is ability to access the EEPROM, then I am dropping $X here.
No, you're dropping an example of doing $X.
Correct. But the move to DM in the driver will drop the functions this example was using, no?
If it was using something that's not in <_exports.h> I don't see that as a problem. A standalone app could do whatever it likes with the hardware and needs to restore the hardware before passing control back to U-Boot (if it's doing that).
My understanding (correct me if I'm wrong) is that the SCM911x driver in question used to expose its eeprom through the external API, but not as though there was a special API directly implemented in that driver.
I think the behavior change that Marek was concerned about was that this particular device would not be accessible via the external eeprom api any longer.
-Joe
On 3/18/20 2:14 AM, Joe Hershberger wrote:
Hi Tom,
On Tue, Mar 17, 2020 at 7:59 PM Tom Rini trini@konsulko.com wrote:
On Tue, Mar 17, 2020 at 07:54:51PM -0500, Joe Hershberger wrote:
On Tue, Mar 17, 2020 at 1:55 PM Tom Rini trini@konsulko.com wrote:
On Tue, Mar 17, 2020 at 07:53:58PM +0100, Marek Vasut wrote:
On 3/17/20 7:44 PM, Tom Rini wrote:
On Tue, Mar 17, 2020 at 07:43:11PM +0100, Marek Vasut wrote: > On 3/17/20 7:42 PM, Tom Rini wrote: >> On Tue, Mar 17, 2020 at 07:39:49PM +0100, Marek Vasut wrote: >>> On 3/17/20 7:30 PM, Tom Rini wrote: >>>> On Tue, Mar 17, 2020 at 07:23:07PM +0100, Marek Vasut wrote: >>>>> On 3/17/20 7:10 PM, Masahiro Yamada wrote: >>>>>> On Sun, Mar 15, 2020 at 8:19 AM Marek Vasut marek.vasut@gmail.com wrote: >>>>>>> >>>>>>> Drop the example, for two reasons. First, it is tapping directly into >>>>>>> the IO accessors of the SMC911x, while it should instead go through >>>>>>> the net device API. Second, this makes conversion of the SMC911x driver >>>>>>> to DM real hard. >>>>>>> >>>>>>> Signed-off-by: Marek Vasut marek.vasut+renesas@gmail.com >>>>>>> Cc: Joe Hershberger joe.hershberger@ni.com >>>>>>> Cc: Tom Rini trini@konsulko.com >>>>>>> --- >>>>>>> examples/standalone/Makefile | 1 - >>>>>>> examples/standalone/smc911x_eeprom.c | 379 --------------------------- >>>>>>> 2 files changed, 380 deletions(-) >>>>>>> delete mode 100644 examples/standalone/smc911x_eeprom.c >>>>>> >>>>>> >>>>>> Yeah, I was disturbed by this example code. >>>>>> >>>>>> I agree we should drop it. >>>>>> >>>>>> Reviewed-by: Masahiro Yamada yamada.masahiro@socionext.com >>>>> >>>>> Well I dunno. Can this be rewritten on top of DM somehow ? Do we even >>>>> have U-Boot application API to access DM EEPROM ? >>>> >>>> We should just drop it I think. The biggest surface we have today for >>>> external application is EFI application now, not U-Boot specific API. >>>> We can't drop the API but we don't expand it without very good reason. >>> >>> But this drops the ability to access the SMC911x EEPROM too. >>> So maybe we need some DM EEPROM implementation in the SMC911x driver ? >>> Does anyone have SMC911x with an external EEPROM ? >> >> All this does is drop an example. I don't see anything removing API >> code itself. > > Where did I say anything about API code ?
Nowhere, which is my point. You're just dropping an example, not the ability to do $X.
If $X is ability to access the EEPROM, then I am dropping $X here.
No, you're dropping an example of doing $X.
Correct. But the move to DM in the driver will drop the functions this example was using, no?
If it was using something that's not in <_exports.h> I don't see that as a problem. A standalone app could do whatever it likes with the hardware and needs to restore the hardware before passing control back to U-Boot (if it's doing that).
My understanding (correct me if I'm wrong) is that the SCM911x driver in question used to expose its eeprom through the external API, but not as though there was a special API directly implemented in that driver.
I think the behavior change that Marek was concerned about was that this particular device would not be accessible via the external eeprom api any longer.
One can have the driver expose some EEPROM interface, but not via the U-Boot application example. But then, the U-Boot application was poking registers directly anyway, which is awful.
participants (3)
-
Joe Hershberger -
Marek Vasut -
Tom Rini