[PATCH 0/3] DM_ETH v mpc83xx fixups

[PATCH 0/3] DM_ETH v mpc83xx fixups

[Rasmus Villemoes]

Hi Heiko

I finally managed to figure out what was wrong; the fixed-link phy was
simply ignored. This is fixed by the first patch, though I don't know
if that's the proper way to make this work.

While poking around the code I found two minor things that might as
well be fixed.

I'd also like to do a somewhat more extensive change to dm_qe_uec.c: I
find it very confusing with the qe_uec_priv versus uec_priv, so I'd
like to just add a phydev member to struct uec_priv, remove struct
qe_uec_priv, make .priv_auto_alloc_size = sizeof(struct uec_priv), and
eliminate the malloc/free pair in .prove/.remove. It's mostly
mechanical using coccinelle, but WDYT?

Rasmus Villemoes (3):
  mdio-uclass.c: support fixed-link subnodes
  dm_qe_uec.c: fix indentation in uec_set_mac_if_mode()
  uec.h: fix COFIG_DM typo

 drivers/net/qe/dm_qe_uec.c | 4 ++--
 drivers/net/qe/uec.h       | 2 +-
 net/mdio-uclass.c          | 7 +++++++
 3 files changed, 10 insertions(+), 3 deletions(-)

-- 
2.23.0

[PATCH 1/3] mdio-uclass.c: support fixed-link subnodes

[Rasmus Villemoes]

When trying to port our mpc8309-based board to DM_ETH, on top of
Heiko's patches, I found that nothing in mdio-uclass.c seems to
support the use of a fixed-link subnode of the ethernet DT node. That
is, the ethernet node looks like

		enet0: ethernet at 2000 {
			device_type = "network";
			compatible = "ucc_geth";
			...
			fixed-link {
				reg = <0xffffffff>;
				speed = <100>;
				full-duplex;
			};

but the current code expects there to be phy-handle property. Adding
that, i.e.

			phy-handle = <&enet0phy>;
			enet0phy: fixed-link {

just makes the code break a few lines later since a fixed-link node
doesn't have a reg property. Ignoring the dtc complaint and adding a
dummy reg property, we of course hit "can't find MDIO bus for node
ethernet at 2000" since indeed, the parent node of the phy node does not
represent an MDIO bus. So that's obviously the wrong path.

Now, in linux, it seems that the fixed link case is treated specially;
in the of_phy_get_and_connect() which roughly corresponds to
dm_eth_connect_phy_handle() we have

    if (of_phy_is_fixed_link(np)) {
        ret = of_phy_register_fixed_link(np);
        ...
    } else {
        phy_np = of_parse_phandle(np, "phy-handle", 0);
	...
    }

    phy = of_phy_connect(dev, phy_np, hndlr, 0, iface);

And U-Boot's phy_connect() does have support for fixed-link
subnodes. Calling phy_connect() directly with NULL bus and a dummy
address does seem to make the ethernet work.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 net/mdio-uclass.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/mdio-uclass.c b/net/mdio-uclass.c
index 66ee2e1976..2f932b77df 100644
--- a/net/mdio-uclass.c
+++ b/net/mdio-uclass.c
@@ -139,6 +139,12 @@ static struct phy_device *dm_eth_connect_phy_handle(struct udevice *ethdev,
 	struct ofnode_phandle_args phandle = {.node = ofnode_null()};
 	int i;
 
+	if (CONFIG_IS_ENABLED(PHY_FIXED) &&
+	    ofnode_valid(dev_read_subnode(ethdev, "fixed-link"))) {
+		phy = phy_connect(NULL, -1, ethdev, interface);
+		goto out;
+	}
+
 	for (i = 0; i < PHY_HANDLE_STR_CNT; i++)
 		if (!dev_read_phandle_with_args(ethdev, phy_handle_str[i], NULL,
 						0, 0, &phandle))
@@ -168,6 +174,7 @@ static struct phy_device *dm_eth_connect_phy_handle(struct udevice *ethdev,
 
 	phy = dm_mdio_phy_connect(mdiodev, phy_addr, ethdev, interface);
 
+out:
 	if (phy)
 		phy->node = phandle.node;
 
-- 
2.23.0

[PATCH 1/3] mdio-uclass.c: support fixed-link subnodes

[Heiko Schocher]

Hello Rasmus,

Am 05.10.2020 um 15:15 schrieb Rasmus Villemoes:
> When trying to port our mpc8309-based board to DM_ETH, on top of
> Heiko's patches, I found that nothing in mdio-uclass.c seems to
> support the use of a fixed-link subnode of the ethernet DT node. That
> is, the ethernet node looks like
> 
> 		enet0: ethernet at 2000 {
> 			device_type = "network";
> 			compatible = "ucc_geth";
> 			...
> 			fixed-link {
> 				reg = <0xffffffff>;
> 				speed = <100>;
> 				full-duplex;
> 			};
> 
> but the current code expects there to be phy-handle property. Adding
> that, i.e.
> 
> 			phy-handle = <&enet0phy>;
> 			enet0phy: fixed-link {
> 
> just makes the code break a few lines later since a fixed-link node
> doesn't have a reg property. Ignoring the dtc complaint and adding a
> dummy reg property, we of course hit "can't find MDIO bus for node
> ethernet at 2000" since indeed, the parent node of the phy node does not
> represent an MDIO bus. So that's obviously the wrong path.
> 
> Now, in linux, it seems that the fixed link case is treated specially;
> in the of_phy_get_and_connect() which roughly corresponds to
> dm_eth_connect_phy_handle() we have
> 
>      if (of_phy_is_fixed_link(np)) {
>          ret = of_phy_register_fixed_link(np);
>          ...
>      } else {
>          phy_np = of_parse_phandle(np, "phy-handle", 0);
> 	...
>      }
> 
>      phy = of_phy_connect(dev, phy_np, hndlr, 0, iface);
> 
> And U-Boot's phy_connect() does have support for fixed-link
> subnodes. Calling phy_connect() directly with NULL bus and a dummy
> address does seem to make the ethernet work.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>   net/mdio-uclass.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/net/mdio-uclass.c b/net/mdio-uclass.c
> index 66ee2e1976..2f932b77df 100644
> --- a/net/mdio-uclass.c
> +++ b/net/mdio-uclass.c
> @@ -139,6 +139,12 @@ static struct phy_device *dm_eth_connect_phy_handle(struct udevice *ethdev,
>   	struct ofnode_phandle_args phandle = {.node = ofnode_null()};
>   	int i;
>   
> +	if (CONFIG_IS_ENABLED(PHY_FIXED) &&
> +	    ofnode_valid(dev_read_subnode(ethdev, "fixed-link"))) {
> +		phy = phy_connect(NULL, -1, ethdev, interface);
> +		goto out;
> +	}
> +
>   	for (i = 0; i < PHY_HANDLE_STR_CNT; i++)
>   		if (!dev_read_phandle_with_args(ethdev, phy_handle_str[i], NULL,
>   						0, 0, &phandle))
> @@ -168,6 +174,7 @@ static struct phy_device *dm_eth_connect_phy_handle(struct udevice *ethdev,
>   
>   	phy = dm_mdio_phy_connect(mdiodev, phy_addr, ethdev, interface);
>   
> +out:
>   	if (phy)
>   		phy->node = phandle.node;

Looks good to me... but I am not an expert in net subsystem...
nevertheless:

Reviewed-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[PATCH 1/3] mdio-uclass.c: support fixed-link subnodes

[Rasmus Villemoes]

On 06/10/2020 08.02, Heiko Schocher wrote:
> Hello Rasmus,
> 
> Am 05.10.2020 um 15:15 schrieb Rasmus Villemoes:
>> When trying to port our mpc8309-based board to DM_ETH, on top of
>> Heiko's patches, I found that nothing in mdio-uclass.c seems to
>> support the use of a fixed-link subnode of the ethernet DT node. That
>> is, the ethernet node looks like
>>
>> ??????? enet0: ethernet at 2000 {
>> ??????????? device_type = "network";
>> ??????????? compatible = "ucc_geth";
>> ??????????? ...
>> ??????????? fixed-link {
>> ??????????????? reg = <0xffffffff>;

Well, it looks like that when a dummy reg property has been added, not
IRL. Sorry.

>> ---
>> ? net/mdio-uclass.c | 7 +++++++
>> ? 1 file changed, 7 insertions(+)
>>
>> diff --git a/net/mdio-uclass.c b/net/mdio-uclass.c
>> index 66ee2e1976..2f932b77df 100644
>> --- a/net/mdio-uclass.c
>> +++ b/net/mdio-uclass.c
>> @@ -139,6 +139,12 @@ static struct phy_device
>> *dm_eth_connect_phy_handle(struct udevice *ethdev,
>> ????? struct ofnode_phandle_args phandle = {.node = ofnode_null()};
>> ????? int i;
>> ? +??? if (CONFIG_IS_ENABLED(PHY_FIXED) &&
>> +??????? ofnode_valid(dev_read_subnode(ethdev, "fixed-link"))) {
>> +??????? phy = phy_connect(NULL, -1, ethdev, interface);
>> +??????? goto out;
>> +??? }
>> +
>> ????? for (i = 0; i < PHY_HANDLE_STR_CNT; i++)
>> ????????? if (!dev_read_phandle_with_args(ethdev, phy_handle_str[i],
>> NULL,
>> ????????????????????????? 0, 0, &phandle))
>> @@ -168,6 +174,7 @@ static struct phy_device
>> *dm_eth_connect_phy_handle(struct udevice *ethdev,
>> ? ????? phy = dm_mdio_phy_connect(mdiodev, phy_addr, ethdev, interface);
>> ? +out:
>> ????? if (phy)
>> ????????? phy->node = phandle.node;
> 
> Looks good to me... but I am not an expert in net subsystem...
> nevertheless:
> 
> Reviewed-by: Heiko Schocher <hs@denx.de>

Thanks.

Yeah, as I said, I have no idea if this is the right way to do things.
Some ethernet drivers seem to call phy_connect directly instead of going
through dm_eth_phy_connect(), some handle fixed-link themselves, etc.
But from a very selfish POV, this is the minimal patch to make our board
(and others with a ucc_geth ethernet dev with a fixed-link) work with
DM_ETH.

Rasmus

[PATCH 1/3] mdio-uclass.c: support fixed-link subnodes

[Tom Rini]

On Mon, Oct 05, 2020 at 03:15:16PM +0200, Rasmus Villemoes wrote:

> When trying to port our mpc8309-based board to DM_ETH, on top of
> Heiko's patches, I found that nothing in mdio-uclass.c seems to
> support the use of a fixed-link subnode of the ethernet DT node. That
> is, the ethernet node looks like
> 
> 		enet0: ethernet at 2000 {
> 			device_type = "network";
> 			compatible = "ucc_geth";
> 			...
> 			fixed-link {
> 				reg = <0xffffffff>;
> 				speed = <100>;
> 				full-duplex;
> 			};
> 
> but the current code expects there to be phy-handle property. Adding
> that, i.e.
> 
> 			phy-handle = <&enet0phy>;
> 			enet0phy: fixed-link {
> 
> just makes the code break a few lines later since a fixed-link node
> doesn't have a reg property. Ignoring the dtc complaint and adding a
> dummy reg property, we of course hit "can't find MDIO bus for node
> ethernet at 2000" since indeed, the parent node of the phy node does not
> represent an MDIO bus. So that's obviously the wrong path.
> 
> Now, in linux, it seems that the fixed link case is treated specially;
> in the of_phy_get_and_connect() which roughly corresponds to
> dm_eth_connect_phy_handle() we have
> 
>     if (of_phy_is_fixed_link(np)) {
>         ret = of_phy_register_fixed_link(np);
>         ...
>     } else {
>         phy_np = of_parse_phandle(np, "phy-handle", 0);
> 	...
>     }
> 
>     phy = of_phy_connect(dev, phy_np, hndlr, 0, iface);
> 
> And U-Boot's phy_connect() does have support for fixed-link
> subnodes. Calling phy_connect() directly with NULL bus and a dummy
> address does seem to make the ethernet work.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Reviewed-by: Heiko Schocher <hs@denx.de>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210222/df0a8a17/attachment.sig>

[PATCH 2/3] dm_qe_uec.c: fix indentation in uec_set_mac_if_mode()

[Rasmus Villemoes]

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/net/qe/dm_qe_uec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/qe/dm_qe_uec.c b/drivers/net/qe/dm_qe_uec.c
index 3482b3ff17..e2b8bf02f9 100644
--- a/drivers/net/qe/dm_qe_uec.c
+++ b/drivers/net/qe/dm_qe_uec.c
@@ -171,8 +171,8 @@ static int uec_set_mac_if_mode(struct uec_priv *uec)
 			break;
 		default:
 			return -EINVAL;
-	}
-	break;
+		}
+		break;
 	case SPEED_1000:
 		maccfg2 |= MACCFG2_INTERFACE_MODE_BYTE;
 		switch (enet_if_mode) {
-- 
2.23.0

[PATCH 2/3] dm_qe_uec.c: fix indentation in uec_set_mac_if_mode()

[Heiko Schocher]

Hello Rasmus,

Am 05.10.2020 um 15:15 schrieb Rasmus Villemoes:
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>   drivers/net/qe/dm_qe_uec.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/qe/dm_qe_uec.c b/drivers/net/qe/dm_qe_uec.c
> index 3482b3ff17..e2b8bf02f9 100644
> --- a/drivers/net/qe/dm_qe_uec.c
> +++ b/drivers/net/qe/dm_qe_uec.c
> @@ -171,8 +171,8 @@ static int uec_set_mac_if_mode(struct uec_priv *uec)
>   			break;
>   		default:
>   			return -EINVAL;
> -	}
> -	break;
> +		}
> +		break;
>   	case SPEED_1000:
>   		maccfg2 |= MACCFG2_INTERFACE_MODE_BYTE;
>   		switch (enet_if_mode) {
> 

Thanks!

Reviewed-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[PATCH 2/3] dm_qe_uec.c: fix indentation in uec_set_mac_if_mode()

[Tom Rini]

On Mon, Oct 05, 2020 at 03:15:17PM +0200, Rasmus Villemoes wrote:

> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Reviewed-by: Heiko Schocher <hs@denx.de>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210222/c8961063/attachment.sig>

[PATCH 3/3] uec.h: fix COFIG_DM typo

[Rasmus Villemoes]

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/net/qe/uec.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/qe/uec.h b/drivers/net/qe/uec.h
index 7cd4b8737a..32b7d3e561 100644
--- a/drivers/net/qe/uec.h
+++ b/drivers/net/qe/uec.h
@@ -678,7 +678,7 @@ struct uec_priv {
 	int				grace_stopped_tx;
 	int				grace_stopped_rx;
 	int				the_first_run;
-#if !defined(COFIG_DM)
+#if !defined(CONFIG_DM)
 	/* PHY specific */
 	struct uec_mii_info		*mii_info;
 	int				oldspeed;
-- 
2.23.0

[PATCH 3/3] uec.h: fix COFIG_DM typo

[Heiko Schocher]

Hello Rasmus,

Am 05.10.2020 um 15:15 schrieb Rasmus Villemoes:
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>   drivers/net/qe/uec.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/qe/uec.h b/drivers/net/qe/uec.h
> index 7cd4b8737a..32b7d3e561 100644
> --- a/drivers/net/qe/uec.h
> +++ b/drivers/net/qe/uec.h
> @@ -678,7 +678,7 @@ struct uec_priv {
>   	int				grace_stopped_tx;
>   	int				grace_stopped_rx;
>   	int				the_first_run;
> -#if !defined(COFIG_DM)
> +#if !defined(CONFIG_DM)
>   	/* PHY specific */
>   	struct uec_mii_info		*mii_info;
>   	int				oldspeed;
> 

Ouch... Thanks for detecting this!

Reviewed-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[PATCH 3/3] uec.h: fix COFIG_DM typo

[Tom Rini]

On Mon, Oct 05, 2020 at 03:15:18PM +0200, Rasmus Villemoes wrote:

> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Reviewed-by: Heiko Schocher <hs@denx.de>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210222/dca33b3b/attachment.sig>

[PATCH 0/3] DM_ETH v mpc83xx fixups

[Heiko Schocher]

Hello Rasmus,

Am 05.10.2020 um 15:15 schrieb Rasmus Villemoes:
> Hi Heiko
> 
> I finally managed to figure out what was wrong; the fixed-link phy was
> simply ignored. This is fixed by the first patch, though I don't know
> if that's the proper way to make this work.
> 
> While poking around the code I found two minor things that might as
> well be fixed.

Thanks for the fixes (and testing)!

> I'd also like to do a somewhat more extensive change to dm_qe_uec.c: I
> find it very confusing with the qe_uec_priv versus uec_priv, so I'd
> like to just add a phydev member to struct uec_priv, remove struct
> qe_uec_priv, make .priv_auto_alloc_size = sizeof(struct uec_priv), and
> eliminate the malloc/free pair in .prove/.remove. It's mostly
> mechanical using coccinelle, but WDYT?

Sounds good, so please send a patch... thanks!

bye,
Heiko
> 
> Rasmus Villemoes (3):
>    mdio-uclass.c: support fixed-link subnodes
>    dm_qe_uec.c: fix indentation in uec_set_mac_if_mode()
>    uec.h: fix COFIG_DM typo
> 
>   drivers/net/qe/dm_qe_uec.c | 4 ++--
>   drivers/net/qe/uec.h       | 2 +-
>   net/mdio-uclass.c          | 7 +++++++
>   3 files changed, 10 insertions(+), 3 deletions(-)
> 

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de