| Message ID | 20161108155437.1085-2-oliver@schinagl.nl |
|---|---|
| State | Superseded |
| Delegated to: | Joe Hershberger |
| Headers | show |
On Tue, Nov 8, 2016 at 9:54 AM, Olliver Schinagl <oliver@schinagl.nl> wrote: > Add the read_rom_hwaddr net_op hook so that it can be called from boards > to read the mac from a ROM chip. > > Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
Hi Oliver, On 8 November 2016 at 08:54, Olliver Schinagl <oliver@schinagl.nl> wrote: > Add the read_rom_hwaddr net_op hook so that it can be called from boards > to read the mac from a ROM chip. > > Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> > --- > drivers/net/designware.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/drivers/net/designware.c b/drivers/net/designware.c > index 9e6d726..aa87f30 100644 > --- a/drivers/net/designware.c > +++ b/drivers/net/designware.c > @@ -230,6 +230,23 @@ static int _dw_write_hwaddr(struct dw_eth_dev *priv, u8 *mac_id) > return 0; > } > > +__weak int dw_board_read_rom_hwaddr(unsigned char *enetaddr) > +{ > + return -ENOSYS; > +} Instead of a weak function I think this should use driver model, with a driver supplied by the board to read this value. It should be possible to supply the 'hardware-address reading' device to any Ethernet driver, not just dwmmc. > + > +static int designware_eth_read_rom_hwaddr(struct udevice *dev) > +{ > + int retval; > + struct eth_pdata *pdata = dev_get_platdata(dev); > + > + retval = dw_board_read_rom_hwaddr(pdata->enetaddr); > + if (retval == -ENOSYS) > + return 0; > + > + return retval; > +} > + > static void dw_adjust_link(struct eth_mac_regs *mac_p, > struct phy_device *phydev) > { > @@ -685,6 +702,7 @@ static const struct eth_ops designware_eth_ops = { > .free_pkt = designware_eth_free_pkt, > .stop = designware_eth_stop, > .write_hwaddr = designware_eth_write_hwaddr, > + .read_rom_hwaddr = designware_eth_read_rom_hwaddr, > }; > > static int designware_eth_ofdata_to_platdata(struct udevice *dev) > -- > 2.10.2 > Regards, Simon
Hi Simon, On Thu, Nov 17, 2016 at 7:13 PM, Simon Glass <sjg@chromium.org> wrote: > Hi Oliver, > > On 8 November 2016 at 08:54, Olliver Schinagl <oliver@schinagl.nl> wrote: >> Add the read_rom_hwaddr net_op hook so that it can be called from boards >> to read the mac from a ROM chip. >> >> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> >> --- >> drivers/net/designware.c | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> diff --git a/drivers/net/designware.c b/drivers/net/designware.c >> index 9e6d726..aa87f30 100644 >> --- a/drivers/net/designware.c >> +++ b/drivers/net/designware.c >> @@ -230,6 +230,23 @@ static int _dw_write_hwaddr(struct dw_eth_dev *priv, u8 *mac_id) >> return 0; >> } >> >> +__weak int dw_board_read_rom_hwaddr(unsigned char *enetaddr) >> +{ >> + return -ENOSYS; >> +} > > Instead of a weak function I think this should use driver model, with > a driver supplied by the board to read this value. It should be > possible to supply the 'hardware-address reading' device to any > Ethernet driver, not just dwmmc. How do we reconcile something like that with the concern of using the device tree for boards using only Linux bindings, and sharing the device tree with Linux? Linux probably doesn't care about this and so won't have a binding for defining this relationship. This is a fairly generic question. Where have we landed on this? Thanks, -Joe
Hi Joe, On 29 November 2016 at 14:24, Joe Hershberger <joe.hershberger@gmail.com> wrote: > > Hi Simon, > > On Thu, Nov 17, 2016 at 7:13 PM, Simon Glass <sjg@chromium.org> wrote: > > Hi Oliver, > > > > On 8 November 2016 at 08:54, Olliver Schinagl <oliver@schinagl.nl> wrote: > >> Add the read_rom_hwaddr net_op hook so that it can be called from boards > >> to read the mac from a ROM chip. > >> > >> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> > >> --- > >> drivers/net/designware.c | 18 ++++++++++++++++++ > >> 1 file changed, 18 insertions(+) > >> > >> diff --git a/drivers/net/designware.c b/drivers/net/designware.c > >> index 9e6d726..aa87f30 100644 > >> --- a/drivers/net/designware.c > >> +++ b/drivers/net/designware.c > >> @@ -230,6 +230,23 @@ static int _dw_write_hwaddr(struct dw_eth_dev *priv, u8 *mac_id) > >> return 0; > >> } > >> > >> +__weak int dw_board_read_rom_hwaddr(unsigned char *enetaddr) > >> +{ > >> + return -ENOSYS; > >> +} > > > > Instead of a weak function I think this should use driver model, with > > a driver supplied by the board to read this value. It should be > > possible to supply the 'hardware-address reading' device to any > > Ethernet driver, not just dwmmc. > > How do we reconcile something like that with the concern of using the > device tree for boards using only Linux bindings, and sharing the > device tree with Linux? Linux probably doesn't care about this and so > won't have a binding for defining this relationship. This is a fairly > generic question. Where have we landed on this? So far I have not seen something that cannot be solved either as I suggest above or with platform data. Often you can pass platform data to the driver - e.g. see the end of board_init() in gurnard.c which tells the video driver which LCD to use. Is there another case? I certainly have ideas but don't want to create something without a solid case. Regards, Simon
On Tue, Nov 29, 2016 at 3:40 PM, Simon Glass <sjg@chromium.org> wrote: > Hi Joe, > > On 29 November 2016 at 14:24, Joe Hershberger <joe.hershberger@gmail.com> wrote: >> >> Hi Simon, >> >> On Thu, Nov 17, 2016 at 7:13 PM, Simon Glass <sjg@chromium.org> wrote: >> > Hi Oliver, >> > >> > On 8 November 2016 at 08:54, Olliver Schinagl <oliver@schinagl.nl> wrote: >> >> Add the read_rom_hwaddr net_op hook so that it can be called from boards >> >> to read the mac from a ROM chip. >> >> >> >> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> >> >> --- >> >> drivers/net/designware.c | 18 ++++++++++++++++++ >> >> 1 file changed, 18 insertions(+) >> >> >> >> diff --git a/drivers/net/designware.c b/drivers/net/designware.c >> >> index 9e6d726..aa87f30 100644 >> >> --- a/drivers/net/designware.c >> >> +++ b/drivers/net/designware.c >> >> @@ -230,6 +230,23 @@ static int _dw_write_hwaddr(struct dw_eth_dev *priv, u8 *mac_id) >> >> return 0; >> >> } >> >> >> >> +__weak int dw_board_read_rom_hwaddr(unsigned char *enetaddr) >> >> +{ >> >> + return -ENOSYS; >> >> +} >> > >> > Instead of a weak function I think this should use driver model, with >> > a driver supplied by the board to read this value. It should be >> > possible to supply the 'hardware-address reading' device to any >> > Ethernet driver, not just dwmmc. >> >> How do we reconcile something like that with the concern of using the >> device tree for boards using only Linux bindings, and sharing the >> device tree with Linux? Linux probably doesn't care about this and so >> won't have a binding for defining this relationship. This is a fairly >> generic question. Where have we landed on this? > > So far I have not seen something that cannot be solved either as I > suggest above or with platform data. > > Often you can pass platform data to the driver - e.g. see the end of > board_init() in gurnard.c which tells the video driver which LCD to > use. > > Is there another case? I certainly have ideas but don't want to create > something without a solid case. I hadn't understood what pattern you were referring to when you said "with a driver supplied by the board to read this value." I just reviewed the rockchip series that you referred to and I think that pattern works pretty cleanly. Thanks, -Joe
diff --git a/drivers/net/designware.c b/drivers/net/designware.c index 9e6d726..aa87f30 100644 --- a/drivers/net/designware.c +++ b/drivers/net/designware.c @@ -230,6 +230,23 @@ static int _dw_write_hwaddr(struct dw_eth_dev *priv, u8 *mac_id) return 0; } +__weak int dw_board_read_rom_hwaddr(unsigned char *enetaddr) +{ + return -ENOSYS; +} + +static int designware_eth_read_rom_hwaddr(struct udevice *dev) +{ + int retval; + struct eth_pdata *pdata = dev_get_platdata(dev); + + retval = dw_board_read_rom_hwaddr(pdata->enetaddr); + if (retval == -ENOSYS) + return 0; + + return retval; +} + static void dw_adjust_link(struct eth_mac_regs *mac_p, struct phy_device *phydev) { @@ -685,6 +702,7 @@ static const struct eth_ops designware_eth_ops = { .free_pkt = designware_eth_free_pkt, .stop = designware_eth_stop, .write_hwaddr = designware_eth_write_hwaddr, + .read_rom_hwaddr = designware_eth_read_rom_hwaddr, }; static int designware_eth_ofdata_to_platdata(struct udevice *dev)
Add the read_rom_hwaddr net_op hook so that it can be called from boards to read the mac from a ROM chip. Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> --- drivers/net/designware.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)