Re: [U-Boot] [PATCH 1/6] net: dw: Add read_rom_hwaddr net_op hook

[Joe Hershberger]

On Wed, Nov 30, 2016 at 2:16 AM, Olliver Schinagl  wrote:
> Hey Simon,
>
>
>
> On 29-11-16 22:41, Simon Glass wrote:
>>
>> Hi Oliver,
>>
>> On 28 November 2016 at 03:38, Olliver Schinagl  wrote:
>>>
>>> On 27-11-16 18:02, Simon Glass wrote:

 Hi,

 On 25 November 2016 at 08:38, Olliver Schinagl 
 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 
> ---
>drivers/net/designware.c | 16 
>1 file changed, 16 insertions(+)
>
> diff --git a/drivers/net/designware.c b/drivers/net/designware.c
> index 9e6d726..3f2f67c 100644
> --- a/drivers/net/designware.c
> +++ b/drivers/net/designware.c
> @@ -230,6 +230,21 @@ 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, int id)
> +{
> +   return -ENOSYS;
> +}
> +
> +static int designware_eth_read_rom_hwaddr(struct udevice *dev)
> +{
> +   struct eth_pdata *pdata = dev_get_platdata(dev);
> +
> +   if (!dev)
> +   return -ENOSYS;
> +
> +   return dw_board_read_rom_hwaddr(pdata->enetaddr, dev->seq);
> +}
> +
>static void dw_adjust_link(struct eth_mac_regs *mac_p,
>  struct phy_device *phydev)
>{
> @@ -685,6 +700,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)

 You should not call board code from a driver. But since this is a
 sunxi driver, why not move all the code that reads the serial number
 into this file?
>>>
>>> Hey Simon,
>>>
>>> unless I missunderstand, this is how Joe suggested in a while ago, and
>>> how
>>> it has been implemented in a few other drivers. Can you elaborate a bit
>>> more?
>>
>> Yes...drivers must not call into board-specific code, nor have
>> board-specific #defines. This limits their usefulness for other
>> boards.
>
> Hmm, well as I said, I just followed Joe's suggestion with his example. also
> isn't this exactly how the zynq does it as well?

Sorry for misleading you. Simon has since convinced me that making a
separate board-specific driver that leverages the core driver's code
is a cleaner approach, and now what I recommend.

>> Adding hooks like this (particularly with the word 'board' in the
>> name) should be avoided.
>>
>> We end up with bidirectional coupling between the board and the
>> driver. The board should use the driver but not the other way around.
>> I understand that you are trying to get around this by using a weak
>> function, but with driver model I'm really trying hard to avoid weak
>> functions. They normally indicate an ad-hoc API and can generally be
>> avoided with a bit more design thought.
>>
>> If you have a standard way of reading the serial number which is
>> supported by all sunxi boards, then you should be able to add your
>> changes to the sunxi Ethernet driver (which uses designware.c?). Then
>> you can leave the designware.c code alone and avoid adding a hook.
>
> Well yes and no. We use designware, but also sunxi_emac, and some
> sdio_realtek that does not have a driver yet. But in essence, this is
> somewhat what I do in this patch I guess. I have the weak driver specific
> function in the sunxi code.
>
> But I think I'm starting to understand your solution and will read up on the
> rockchip patches and rewrite this bit.
>
>>
>> In a sense you end up subclassing the designware driver.
>>
>> Also see this series which deals with a similar problem with rockchip:
>>
>> http://lists.denx.de/pipermail/u-boot/2016-November/274256.html
>>
>> Regards,
>> Simon
>
>
> ___
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/6] net: dw: Add read_rom_hwaddr net_op hook

[Olliver Schinagl]

Hey Simon,


On 29-11-16 22:41, Simon Glass wrote:

Hi Oliver,

On 28 November 2016 at 03:38, Olliver Schinagl  wrote:

On 27-11-16 18:02, Simon Glass wrote:

Hi,

On 25 November 2016 at 08:38, Olliver Schinagl  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 
---
   drivers/net/designware.c | 16 
   1 file changed, 16 insertions(+)

diff --git a/drivers/net/designware.c b/drivers/net/designware.c
index 9e6d726..3f2f67c 100644
--- a/drivers/net/designware.c
+++ b/drivers/net/designware.c
@@ -230,6 +230,21 @@ 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, int id)
+{
+   return -ENOSYS;
+}
+
+static int designware_eth_read_rom_hwaddr(struct udevice *dev)
+{
+   struct eth_pdata *pdata = dev_get_platdata(dev);
+
+   if (!dev)
+   return -ENOSYS;
+
+   return dw_board_read_rom_hwaddr(pdata->enetaddr, dev->seq);
+}
+
   static void dw_adjust_link(struct eth_mac_regs *mac_p,
 struct phy_device *phydev)
   {
@@ -685,6 +700,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)

You should not call board code from a driver. But since this is a
sunxi driver, why not move all the code that reads the serial number
into this file?

Hey Simon,

unless I missunderstand, this is how Joe suggested in a while ago, and how
it has been implemented in a few other drivers. Can you elaborate a bit
more?

Yes...drivers must not call into board-specific code, nor have
board-specific #defines. This limits their usefulness for other
boards.
Hmm, well as I said, I just followed Joe's suggestion with his example. 
also isn't this exactly how the zynq does it as well?


Adding hooks like this (particularly with the word 'board' in the
name) should be avoided.

We end up with bidirectional coupling between the board and the
driver. The board should use the driver but not the other way around.
I understand that you are trying to get around this by using a weak
function, but with driver model I'm really trying hard to avoid weak
functions. They normally indicate an ad-hoc API and can generally be
avoided with a bit more design thought.

If you have a standard way of reading the serial number which is
supported by all sunxi boards, then you should be able to add your
changes to the sunxi Ethernet driver (which uses designware.c?). Then
you can leave the designware.c code alone and avoid adding a hook.
Well yes and no. We use designware, but also sunxi_emac, and some 
sdio_realtek that does not have a driver yet. But in essence, this is 
somewhat what I do in this patch I guess. I have the weak driver 
specific function in the sunxi code.


But I think I'm starting to understand your solution and will read up on 
the rockchip patches and rewrite this bit.


In a sense you end up subclassing the designware driver.

Also see this series which deals with a similar problem with rockchip:

http://lists.denx.de/pipermail/u-boot/2016-November/274256.html

Regards,
Simon


___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/6] net: dw: Add read_rom_hwaddr net_op hook

[Simon Glass]

Hi Oliver,

On 28 November 2016 at 03:38, Olliver Schinagl  wrote:
> On 27-11-16 18:02, Simon Glass wrote:
>>
>> Hi,
>>
>> On 25 November 2016 at 08:38, Olliver Schinagl  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 
>>> ---
>>>   drivers/net/designware.c | 16 
>>>   1 file changed, 16 insertions(+)
>>>
>>> diff --git a/drivers/net/designware.c b/drivers/net/designware.c
>>> index 9e6d726..3f2f67c 100644
>>> --- a/drivers/net/designware.c
>>> +++ b/drivers/net/designware.c
>>> @@ -230,6 +230,21 @@ 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, int id)
>>> +{
>>> +   return -ENOSYS;
>>> +}
>>> +
>>> +static int designware_eth_read_rom_hwaddr(struct udevice *dev)
>>> +{
>>> +   struct eth_pdata *pdata = dev_get_platdata(dev);
>>> +
>>> +   if (!dev)
>>> +   return -ENOSYS;
>>> +
>>> +   return dw_board_read_rom_hwaddr(pdata->enetaddr, dev->seq);
>>> +}
>>> +
>>>   static void dw_adjust_link(struct eth_mac_regs *mac_p,
>>> struct phy_device *phydev)
>>>   {
>>> @@ -685,6 +700,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)
>>
>> You should not call board code from a driver. But since this is a
>> sunxi driver, why not move all the code that reads the serial number
>> into this file?
>
> Hey Simon,
>
> unless I missunderstand, this is how Joe suggested in a while ago, and how
> it has been implemented in a few other drivers. Can you elaborate a bit
> more?

Yes...drivers must not call into board-specific code, nor have
board-specific #defines. This limits their usefulness for other
boards.

Adding hooks like this (particularly with the word 'board' in the
name) should be avoided.

We end up with bidirectional coupling between the board and the
driver. The board should use the driver but not the other way around.
I understand that you are trying to get around this by using a weak
function, but with driver model I'm really trying hard to avoid weak
functions. They normally indicate an ad-hoc API and can generally be
avoided with a bit more design thought.

If you have a standard way of reading the serial number which is
supported by all sunxi boards, then you should be able to add your
changes to the sunxi Ethernet driver (which uses designware.c?). Then
you can leave the designware.c code alone and avoid adding a hook.

In a sense you end up subclassing the designware driver.

Also see this series which deals with a similar problem with rockchip:

http://lists.denx.de/pipermail/u-boot/2016-November/274256.html

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/6] net: dw: Add read_rom_hwaddr net_op hook

[Olliver Schinagl]

On 27-11-16 18:02, Simon Glass wrote:

Hi,

On 25 November 2016 at 08:38, Olliver Schinagl  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 
---
  drivers/net/designware.c | 16 
  1 file changed, 16 insertions(+)

diff --git a/drivers/net/designware.c b/drivers/net/designware.c
index 9e6d726..3f2f67c 100644
--- a/drivers/net/designware.c
+++ b/drivers/net/designware.c
@@ -230,6 +230,21 @@ 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, int id)
+{
+   return -ENOSYS;
+}
+
+static int designware_eth_read_rom_hwaddr(struct udevice *dev)
+{
+   struct eth_pdata *pdata = dev_get_platdata(dev);
+
+   if (!dev)
+   return -ENOSYS;
+
+   return dw_board_read_rom_hwaddr(pdata->enetaddr, dev->seq);
+}
+
  static void dw_adjust_link(struct eth_mac_regs *mac_p,
struct phy_device *phydev)
  {
@@ -685,6 +700,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)

You should not call board code from a driver. But since this is a
sunxi driver, why not move all the code that reads the serial number
into this file?

Hey Simon,

unless I missunderstand, this is how Joe suggested in a while ago, and 
how it has been implemented in a few other drivers. Can you elaborate a 
bit more?


Olliver


Regards,
Simon



___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 1/6] net: dw: Add read_rom_hwaddr net_op hook

[Simon Glass]

Hi,

On 25 November 2016 at 08:38, Olliver Schinagl  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 
> ---
>  drivers/net/designware.c | 16 
>  1 file changed, 16 insertions(+)
>
> diff --git a/drivers/net/designware.c b/drivers/net/designware.c
> index 9e6d726..3f2f67c 100644
> --- a/drivers/net/designware.c
> +++ b/drivers/net/designware.c
> @@ -230,6 +230,21 @@ 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, int id)
> +{
> +   return -ENOSYS;
> +}
> +
> +static int designware_eth_read_rom_hwaddr(struct udevice *dev)
> +{
> +   struct eth_pdata *pdata = dev_get_platdata(dev);
> +
> +   if (!dev)
> +   return -ENOSYS;
> +
> +   return dw_board_read_rom_hwaddr(pdata->enetaddr, dev->seq);
> +}
> +
>  static void dw_adjust_link(struct eth_mac_regs *mac_p,
>struct phy_device *phydev)
>  {
> @@ -685,6 +700,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)

You should not call board code from a driver. But since this is a
sunxi driver, why not move all the code that reads the serial number
into this file?

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 1/6] net: dw: Add read_rom_hwaddr net_op hook

[Olliver Schinagl]

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 
---
 drivers/net/designware.c | 16 
 1 file changed, 16 insertions(+)

diff --git a/drivers/net/designware.c b/drivers/net/designware.c
index 9e6d726..3f2f67c 100644
--- a/drivers/net/designware.c
+++ b/drivers/net/designware.c
@@ -230,6 +230,21 @@ 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, int id)
+{
+   return -ENOSYS;
+}
+
+static int designware_eth_read_rom_hwaddr(struct udevice *dev)
+{
+   struct eth_pdata *pdata = dev_get_platdata(dev);
+
+   if (!dev)
+   return -ENOSYS;
+
+   return dw_board_read_rom_hwaddr(pdata->enetaddr, dev->seq);
+}
+
 static void dw_adjust_link(struct eth_mac_regs *mac_p,
   struct phy_device *phydev)
 {
@@ -685,6 +700,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

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot