[U-Boot,10/51] net:phy:marvell Add hook for m88e1510 board config

From: Dirk Eibach <dirk.eibach@gdsys.cc>

m88e1510_config() is highly board-specific. So add an optional
callback board_m88e1510_config() configurable by
CONFIG_BOARD_M88E1510_CONFIG to support different hardware.

Signed-off-by: Dirk Eibach <dirk.eibach@gdsys.cc>
Signed-off-by: Mario Six <mario.six@gdsys.cc>
---

 drivers/net/phy/marvell.c |  5 +++++
 include/marvell-phy.h     | 10 ++++++++++
 2 files changed, 15 insertions(+)
 create mode 100644 include/marvell-phy.h

Hi Mario,

On 14 July 2017 at 05:54, Mario Six <mario.six@gdsys.cc> wrote:
> From: Dirk Eibach <dirk.eibach@gdsys.cc>
>
> m88e1510_config() is highly board-specific. So add an optional
> callback board_m88e1510_config() configurable by
> CONFIG_BOARD_M88E1510_CONFIG to support different hardware.
>
> Signed-off-by: Dirk Eibach <dirk.eibach@gdsys.cc>
> Signed-off-by: Mario Six <mario.six@gdsys.cc>
> ---
>
>  drivers/net/phy/marvell.c |  5 +++++
>  include/marvell-phy.h     | 10 ++++++++++
>  2 files changed, 15 insertions(+)
>  create mode 100644 include/marvell-phy.h
>
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index 66107a8af3..b3d05d5af4 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -9,6 +9,7 @@
>  #include <config.h>
>  #include <common.h>
>  #include <errno.h>
> +#include <marvell-phy.h>
>  #include <phy.h>
>
>  #define PHY_AUTONEGOTIATE_TIMEOUT 5000
> @@ -381,6 +382,9 @@ static int m88e1518_config(struct phy_device *phydev)
>  /* Marvell 88E1510 */
>  static int m88e1510_config(struct phy_device *phydev)
>  {
> +#ifdef CONFIG_BOARD_M88E1510_CONFIG
> +       board_m88e1510_config(phydev);
> +#else

We should not be calling into board code from drivers. How could we do
this with driver model / DT?

>         /* Select page 3 */
>         phy_write(phydev, MDIO_DEVAD_NONE, MIIM_88E1118_PHY_PAGE,
>                   MIIM_88E1118_PHY_LED_PAGE);
> @@ -401,6 +405,7 @@ static int m88e1510_config(struct phy_device *phydev)
>
>         /* Reset page selection */
>         phy_write(phydev, MDIO_DEVAD_NONE, MIIM_88E1118_PHY_PAGE, 0);
> +#endif
>
>         return m88e1518_config(phydev);
>  }
> diff --git a/include/marvell-phy.h b/include/marvell-phy.h
> new file mode 100644
> index 0000000000..1cfa5ed557
> --- /dev/null
> +++ b/include/marvell-phy.h
> @@ -0,0 +1,10 @@
> +#ifndef _MARVELL_PHY_H
> +#define _MARVELL_PHY_H
> +
> +#include <phy.h>
> +
> +void m88e1518_phy_writebits(struct phy_device *phydev,
> +                  u8 reg_num, u16 offset, u16 len, u16 data);
> +int board_m88e1510_config(struct phy_device *phydev);
> +
> +#endif
> --
> 2.11.0
>

Regards,
Simon

Hi Simon,

On Tue, Jul 18, 2017 at 4:01 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi Mario,
>
> On 14 July 2017 at 05:54, Mario Six <mario.six@gdsys.cc> wrote:
>> From: Dirk Eibach <dirk.eibach@gdsys.cc>
>>
>> m88e1510_config() is highly board-specific. So add an optional
>> callback board_m88e1510_config() configurable by
>> CONFIG_BOARD_M88E1510_CONFIG to support different hardware.
>>
>> Signed-off-by: Dirk Eibach <dirk.eibach@gdsys.cc>
>> Signed-off-by: Mario Six <mario.six@gdsys.cc>
>> ---
>>
>>  drivers/net/phy/marvell.c |  5 +++++
>>  include/marvell-phy.h     | 10 ++++++++++
>>  2 files changed, 15 insertions(+)
>>  create mode 100644 include/marvell-phy.h
>>
>> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
>> index 66107a8af3..b3d05d5af4 100644
>> --- a/drivers/net/phy/marvell.c
>> +++ b/drivers/net/phy/marvell.c
>> @@ -9,6 +9,7 @@
>>  #include <config.h>
>>  #include <common.h>
>>  #include <errno.h>
>> +#include <marvell-phy.h>
>>  #include <phy.h>
>>
>>  #define PHY_AUTONEGOTIATE_TIMEOUT 5000
>> @@ -381,6 +382,9 @@ static int m88e1518_config(struct phy_device *phydev)
>>  /* Marvell 88E1510 */
>>  static int m88e1510_config(struct phy_device *phydev)
>>  {
>> +#ifdef CONFIG_BOARD_M88E1510_CONFIG
>> +       board_m88e1510_config(phydev);
>> +#else
>
> We should not be calling into board code from drivers. How could we do
> this with driver model / DT?
>

The simplest way I see would be to convert the phy to DM (i.e. make a ethernet
phy uclass if necessary), get the udevice for the phy in the board code (e.g.
in last_stage_init) to force a probe, and finally run the initialization code.
The problem is, of course, that this final part of the initialization then
happens outside the driver code; that violates the philosophy that probing the
device should initialize it completely.

Well, to collect some facts:

The board-specific initialization code cannot live in the phy driver, since
it's board-specific. Hence, it must either live in the board code or in a
different driver. Since calling into board code is not a good idea, it can only
live in a different driver. The questions is, which driver should this be, and
what other code should it contain?

The other question is that of the caller (i.e. who should call the
initialization code). Since we already saw that calling the code from the board
code is not so good, we must conclude that we have to call it from a driver.
Since we want to keep the initialization intact (i.e. not split it up), we have
to call it from the phy driver.

Hence, the other possible way would be to have a "fixup driver" with a matching
function somewhere in the DT (the code for which will probably reside in the
board directory), the phy driver gets the corresponding udevice, and runs the
initialization code by calling a suitable uclass function on that udevice. That
would make sure that the initialization is always coherent. Maybe calling the
fixup driver could also be part of the phy uclass, but that would have to be
evaluated further.

>>         /* Select page 3 */
>>         phy_write(phydev, MDIO_DEVAD_NONE, MIIM_88E1118_PHY_PAGE,
>>                   MIIM_88E1118_PHY_LED_PAGE);
>> @@ -401,6 +405,7 @@ static int m88e1510_config(struct phy_device *phydev)
>>
>>         /* Reset page selection */
>>         phy_write(phydev, MDIO_DEVAD_NONE, MIIM_88E1118_PHY_PAGE, 0);
>> +#endif
>>
>>         return m88e1518_config(phydev);
>>  }
>> diff --git a/include/marvell-phy.h b/include/marvell-phy.h
>> new file mode 100644
>> index 0000000000..1cfa5ed557
>> --- /dev/null
>> +++ b/include/marvell-phy.h
>> @@ -0,0 +1,10 @@
>> +#ifndef _MARVELL_PHY_H
>> +#define _MARVELL_PHY_H
>> +
>> +#include <phy.h>
>> +
>> +void m88e1518_phy_writebits(struct phy_device *phydev,
>> +                  u8 reg_num, u16 offset, u16 len, u16 data);
>> +int board_m88e1510_config(struct phy_device *phydev);
>> +
>> +#endif
>> --
>> 2.11.0
>>
>
> Regards,
> Simon

Best regards,

Mario