[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 <oliver at schinagl.nl> wrote:
> Hey Simon,
>
>
>
> On 29-11-16 22:41, Simon Glass wrote:
>>
>> Hi Oliver,
>>
>> On 28 November 2016 at 03:38, Olliver Schinagl <oliver at schinagl.nl> wrote:
>>>
>>> On 27-11-16 18:02, Simon Glass wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 25 November 2016 at 08:38, Olliver Schinagl <oliver at 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 at schinagl.nl>
>>>>> ---
>>>>>    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 at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot