net: macb: fail when there's no PHY

net: macb: fail when there's no PHY

[Grant Edwards]

Several years back (circa 2.6.33) I had to hack up macb.c to work on
an at91 board that didn't have a PHY connected to the macb controller.
Now I might need to get a recent kernel version running on that board.

It looks like the macb driver still can't handle boards that don't
have a PHY.  Is that correct?

What's the right way to deal with this?

With the older macb driver, I ended up adding code to macb.c that
presented a "fake" PHY that discarded MDIO writes and returned some
hard-wired values for MDIO reads.  That seemed like a pretty ugly way
to deal with the situation, so I never bothered to submit a patch.

-- 
Grant Edwards
grant.b.edwards@gmail.com

Re: net: macb: fail when there's no PHY

[Florian Fainelli]

On 09/21/2017 12:59 PM, Grant Edwards wrote:
> Several years back (circa 2.6.33) I had to hack up macb.c to work on
> an at91 board that didn't have a PHY connected to the macb controller.
> Now I might need to get a recent kernel version running on that board.
> 
> It looks like the macb driver still can't handle boards that don't
> have a PHY.  Is that correct?

Not since:

dacdbb4dfc1a1a1378df8ebc914d4fe82259ed46 ("net: macb: add fixed-link
node support")

> 
> What's the right way to deal with this?

Declaring a fixed PHY that will present an emulated link UP, with a
fixed speed/duplex etc. is the way to go.

> 
> With the older macb driver, I ended up adding code to macb.c that
> presented a "fake" PHY that discarded MDIO writes and returned some
> hard-wired values for MDIO reads.  That seemed like a pretty ugly way
> to deal with the situation, so I never bothered to submit a patch.
> 

Yeah, no :)
-- 
Florian

Re: net: macb: fail when there's no PHY

[Grant Edwards]

On Thu, Sep 21, 2017 at 01:05:57PM -0700, Florian Fainelli wrote:

>> It looks like the macb driver still can't handle boards that don't
>> have a PHY.  Is that correct?
> 
> Not since:
> 
> dacdbb4dfc1a1a1378df8ebc914d4fe82259ed46 ("net: macb: add fixed-link
> node support")

Yep, it's obvious now that I've got the diff in front of me.

Thanks!

[I just started working with device tree for the first time yesterday,
and I must say it's way better than the "old days" which required all
sorts of ugly to produce a kernel that could work on two slightly
different boards.]

-- 
Grant

Re: net: macb: fail when there's no PHY

[Brandon Streiff]

> On Thu, Sep 21, 2017 at 01:05:57PM -0700, Florian Fainelli wrote:
>
>>> It looks like the macb driver still can't handle boards that don't
>>> have a PHY.  Is that correct?
>> 
>> Not since:
>> 
>> dacdbb4dfc1a1a1378df8ebc914d4fe82259ed46 ("net: macb: add fixed-link
>> node support")
>
> Yep, it's obvious now that I've got the diff in front of me.
>
> Thanks!
>
> [I just started working with device tree for the first time yesterday,
> and I must say it's way better than the "old days" which required all
> sorts of ugly to produce a kernel that could work on two slightly
> different boards.]
>
> -- 
> Grant

I have a board that's in a similar boat. My workaround was to undo
portions of dacdbb4dfc1a with the following patch; this lets me still
use fixed-link and have MDIO (to configure a switch), but not require
a PHY.

There was a patch set last year by Harini Katakam ("net: macb: Add MDIO
driver for accessing multiple PHY devices") that might ultimately be a
better approach to tackling this problem, although I haven't seen any
further chatter on it.

---
 drivers/net/ethernet/cadence/macb_main.c | 38 +++++++++++++++-----------------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 1741cda..a45848e 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -564,30 +564,28 @@ static int macb_mii_init(struct macb *bp)
 				goto err_out_unregister_bus;
 			}
 			bp->phy_node = of_node_get(np);
+		}
 
-			err = mdiobus_register(bp->mii_bus);
-		} else {
-			/* try dt phy registration */
-			err = of_mdiobus_register(bp->mii_bus, np);
+		/* try dt phy registration */
+		err = of_mdiobus_register(bp->mii_bus, np);
 
-			/* fallback to standard phy registration if no phy were
-			 * found during dt phy registration
-			 */
-			if (!err && !phy_find_first(bp->mii_bus)) {
-				for (i = 0; i < PHY_MAX_ADDR; i++) {
-					struct phy_device *phydev;
-
-					phydev = mdiobus_scan(bp->mii_bus, i);
-					if (IS_ERR(phydev) &&
-					    PTR_ERR(phydev) != -ENODEV) {
-						err = PTR_ERR(phydev);
-						break;
-					}
+		/* fallback to standard phy registration if no phy were
+		 * found during dt phy registration
+		 */
+		if (!err && !phy_find_first(bp->mii_bus)) {
+			for (i = 0; i < PHY_MAX_ADDR; i++) {
+				struct phy_device *phydev;
+
+				phydev = mdiobus_scan(bp->mii_bus, i);
+				if (IS_ERR(phydev) &&
+				    PTR_ERR(phydev) != -ENODEV) {
+					err = PTR_ERR(phydev);
+					break;
 				}
-
-				if (err)
-					goto err_out_unregister_bus;
 			}
+
+			if (err)
+				goto err_out_unregister_bus;
 		}
 	} else {
 		for (i = 0; i < PHY_MAX_ADDR; i++)
-- 
2.1.4

Re: net: macb: fail when there's no PHY

[Harini Katakam]

Hi Brandon,

On Fri, Sep 22, 2017 at 3:05 AM, Brandon Streiff <brandon.streiff@ni.com> wrote:
>> On Thu, Sep 21, 2017 at 01:05:57PM -0700, Florian Fainelli wrote:
<snip>
>
> I have a board that's in a similar boat. My workaround was to undo
> portions of dacdbb4dfc1a with the following patch; this lets me still
> use fixed-link and have MDIO (to configure a switch), but not require
> a PHY.
>
> There was a patch set last year by Harini Katakam ("net: macb: Add MDIO
> driver for accessing multiple PHY devices") that might ultimately be a
> better approach to tackling this problem, although I haven't seen any
> further chatter on it.

That patch was not backward compatible and I was trying to find a better
solution for a common MDIO bus.
I plan to send a new series next month and work on the review comments.

Regards,
Harini

Re: net: macb: fail when there's no PHY

[Florian Fainelli]

+Andrew,

On 12/2/2020 9:50 AM, Grant Edwards wrote:
> On Thu, Sep 21, 2017 at 3:06 PM Florian Fainelli <f.fainelli@gmail.com
> <mailto:f.fainelli@gmail.com>> wrote:
> 
>     On 09/21/2017 12:59 PM, Grant Edwards wrote:
>     > Several years back (circa 2.6.33) I had to hack up macb.c to work on
>     > an at91 board that didn't have a PHY connected to the macb controller.
>     > [...] 
>     > It looks like the macb driver still can't handle boards that don't
>     > have a PHY.  Is that correct?
> 
>     Not since:
>     dacdbb4dfc1a1a1378df8ebc914d4fe82259ed46 ("net: macb: add fixed-link
>     node support")
> 
>     > What's the right way to deal with this?
> 
>     Declaring a fixed PHY that will present an emulated link UP, with a
>     fixed speed/duplex etc. is the way to go.
> 
> 
> I know this thread is a couple years old, but I finally got around to
> working with a newer kernel (5.4) that has the "fixed phy" support.
> Unfortunately, the existing "fixed phy" support is unusable for us. It
> doesn't just present a fake, fixed, PHY. It replaces the entire mii
> (mdio/mdc) bus with a fake bus. That means our code loses the ability to
> talk to the devices that /are/ attached to the macb's mdio management bus.

You did not indicate this was a requirement.

> 
> So, I ended up porting my hack from the 2.6.33 macb.c driver to the 5.4
> macb.c driver. It presents a fake PHY at one address on the mdio bus,
> but still allows normal communication with devices at other addresses on
> the bus. We use SIOC[SG]MIIREG ioctl() calls from userspace to talk to
> those "real" devices. Adding a fake PHY to the macb's mdio bus takes a
> total of about two dozen lines of code.

That should be unnecessary see below.

> 
> Was there some other way I should have done this with a 5.4 kernel that
> I was unable to discover?

You should be able to continue having the macb MDIO bus controller be
registered with no PHY/MDIO devices represented in Device Tree such that
user-space can continue to use it for ioctl() *and* you can point the
macb node to a fixed PHY for the purpose of having fixed link parameters.

There are various drivers that support exactly this mode of operation
where they use a fixed PHY for the Ethernet MAC link parameters, yet
their MDIO bus controller is used to interface with other devices such
as Ethernet switches over MDIO. Example of drivers supporting that are
stmmac, fec, mtk_star_emac and ag71xx. The way it ends up looking like
in Device Tree is the following:

&eth0 {
	fixed-link {
		speed = <1000>;
		full-duplex;
	};

	mdio {
		phy0: phy@0 {
			reg = <0>;
		};
	};
};

The key thing here is to support a "mdio" bus container node which is
optional and is used as a hint that you need to register the MACB MDIO
bus controller regardless of MII probing having found devices or not.
-- 
Florian

Re: net: macb: fail when there's no PHY

[Grant Edwards]

[Sorry for sending my previous message twice, it was rejected by the
list server the first time because it contained both plaintext and
HTML alternatives].

On Wed, Dec 02, 2020 at 10:10:37AM -0800, Florian Fainelli wrote:
> On 12/2/2020 9:50 AM, Grant Edwards wrote:
>
> > I know this thread is a couple years old, but I finally got around
> > to working with a newer kernel (5.4) that has the "fixed phy"
> > support.  Unfortunately, the existing "fixed phy" support is
> > unusable for us. It doesn't just present a fake, fixed, PHY. It
> > replaces the entire mii (mdio/mdc) bus with a fake bus. That means
> > our code loses the ability to talk to the devices that /are/
> > attached to the macb's mdio management bus.
> 
> You did not indicate this was a requirement.

Indeed, I should have done so. It didn't occur to me since I was
discussing adding a fake PHY, not a fake bus.

>> So, I ended up porting my hack from the 2.6.33 macb.c driver to the
>> 5.4 macb.c driver. [...]
> 
> That should be unnecessary see below.

> &eth0 {
> 	fixed-link {
> 		speed = <1000>;
> 		full-duplex;
> 	};
> 	mdio {
> 		phy0: phy@0 {
> 			reg = <0>;
> 		};
> 	};
> };

Thanks! I may try that if we can resolve the performance issues with
the newer kernels.

When using the SIOC[SG]MIIREG ioctl() call, how does one control
whether the fake fixed-link bus is accessed or the real macb-mdio bus
is accessed? Do different phy_ids automatically get mapped to the two
busses? That requires that a particular id can only exist on one bus
(which isn't a problem).

> The key thing here is to support a "mdio" bus container node which is
> optional and is used as a hint that you need to register the MACB MDIO
> bus controller regardless of MII probing having found devices or not.

How does the macb driver decide which bus/id combination to use as
"the phy" that controls the link duplex/speed settting the the MAC?

[Feel free to point me at appropriat documentation for this.]

--
Grant

Re: net: macb: fail when there's no PHY

[Andrew Lunn]

> When using the SIOC[SG]MIIREG ioctl() call, how does one control
> whether the fake fixed-link bus is accessed or the real macb-mdio bus
> is accessed?

As far as i remember, that ioctl is based on the interface name. So it
will access the MDIO bus of the PHY that is attached to the MAC.

I guess you have user space drivers using the IOCTL to access devices
on the real bus? A switch? Can you swap to a DSA driver?

> How does the macb driver decide which bus/id combination to use as
> "the phy" that controls the link duplex/speed settting the the MAC?

phy-handle points to a PHY.

	   Andrew

Re: net: macb: fail when there's no PHY

[Grant Edwards]

On 2020-12-02, Andrew Lunn <andrew@lunn.ch> wrote:
>> When using the SIOC[SG]MIIREG ioctl() call, how does one control
>> whether the fake fixed-link bus is accessed or the real macb-mdio bus
>> is accessed?
>
> As far as i remember, that ioctl is based on the interface name.

Right.

> So it will access the MDIO bus of the PHY that is attached to the
> MAC.

If that's the case, wouldn't the ioctl() calls "just work" even when
only the fixed-phy mdio bus and fake PHY are declared in the device
tree?  There must have been something else going on that caused our
user-space code to be unable to talk to the devices on the mdio
bus. We assumed it was because there was only one mdio bus (the fake
one) in the device tree. I'm beginning to suspect that's not the
reason.

> I guess you have user space drivers using the IOCTL to access
> devices on the real bus?

Yes.

> A switch?

There is a switch, but it's not on the mdio bus (on some models, the
switch is access via memory-mapped registers, on others the switch is
accessed via SPI). The PHYs that are attached to the "other" ports of
the switch are on the macb mdio bus.

> Can you swap to a DSA driver?

Possibly. It looks like DSA uses frame tagging. We have two slightly
different platforms. One doesn't have any tagging capabilities. The
other does, but the tags are reserved for use by another chunk of
custom code we've added to the macb.c driver to provide MAC-level
access for a userspace protocol stack which operates in parallel with
the kernel's network stack. It's almost, but not quite, as ugly as it
sounds.

>> How does the macb driver decide which bus/id combination to use as
>> "the phy" that controls the link duplex/speed settting the the MAC?
>
> phy-handle points to a PHY.

OK, I think I've got a vague idea of how that would be done.

[When it comes to device-tree stuff, I've learned that "a vague idea"
is pretty much the best I can manage. Nothing ever works the way I
think it's going to the first time, but with enough guesses I usually
get there.]

--
Grant

Re: net: macb: fail when there's no PHY

[Andrew Lunn]

> > So it will access the MDIO bus of the PHY that is attached to the
> > MAC.
> 
> If that's the case, wouldn't the ioctl() calls "just work" even when
> only the fixed-phy mdio bus and fake PHY are declared in the device
> tree?

The fixed-link PHY is connected to the MAC. So the IOCTL calls will be
made to the fixed-link fake MDIO bus.

> OK, I think I've got a vague idea of how that would be done.
> 
> [When it comes to device-tree stuff, I've learned that "a vague idea"
> is pretty much the best I can manage. Nothing ever works the way I
> think it's going to the first time, but with enough guesses I usually
> get there.]

There are plenty of examples to follow.

e.g. arch/arm/boot/dts/vf610-twr.dts

&fec0 {
        phy-mode = "rmii";
        phy-handle = <&ethphy0>;
        pinctrl-names = "default";
        pinctrl-0 = <&pinctrl_fec0>;
        status = "okay";

        mdio {
                #address-cells = <1>;
                #size-cells = <0>;

                ethphy0: ethernet-phy@0 {
                        reg = <0>;
                };

                ethphy1: ethernet-phy@1 {
                        reg = <1>;
                };
        };
};

So one Ethernet controller with an MDIO bus with two PHYs on it. It
has a phy-handle pointing it is own PHY.

&fec1 {
        phy-mode = "rmii";
        phy-handle = <&ethphy1>;
        pinctrl-names = "default";
        pinctrl-0 = <&pinctrl_fec1>;
        status = "okay";
};

A second Ethernet, with phy-handle pointing to the second PHY on the
other controllers MDIO bus.

	    Andrew

Re: net: macb: fail when there's no PHY

[Grant Edwards]

On 2020-12-02, Andrew Lunn <andrew@lunn.ch> wrote:
>> > So it will access the MDIO bus of the PHY that is attached to the
>> > MAC.
>> 
>> If that's the case, wouldn't the ioctl() calls "just work" even when
>> only the fixed-phy mdio bus and fake PHY are declared in the device
>> tree?
>
> The fixed-link PHY is connected to the MAC. So the IOCTL calls will be
> made to the fixed-link fake MDIO bus.

So how do you control which of the two mdio buses is connected to
the MAC?

> There are plenty of examples to follow.

That's true, but knowing which ones do what you're trying to do is the
hard part. If you already know how to do it, it's easy to find
examples showing it.  :)

--
Grant

Re: net: macb: fail when there's no PHY

[Andrew Lunn]

On Wed, Dec 02, 2020 at 09:23:40PM -0000, Grant Edwards wrote:
> On 2020-12-02, Andrew Lunn <andrew@lunn.ch> wrote:
> >> > So it will access the MDIO bus of the PHY that is attached to the
> >> > MAC.
> >> 
> >> If that's the case, wouldn't the ioctl() calls "just work" even when
> >> only the fixed-phy mdio bus and fake PHY are declared in the device
> >> tree?
> >
> > The fixed-link PHY is connected to the MAC. So the IOCTL calls will be
> > made to the fixed-link fake MDIO bus.
> 
> So how do you control which of the two mdio buses is connected to
> the MAC?

The bus is not connected to the MAC. The PHY is.

https://elixir.bootlin.com/linux/HEAD/source/drivers/net/ethernet/cadence/macb_main.c#L699

	if (dn)
		ret = phylink_of_phy_connect(bp->phylink, dn, 0);

	if (!dn || (ret && !macb_phy_handle_exists(dn))) {
		phydev = phy_find_first(bp->mii_bus);
		if (!phydev) {
			netdev_err(dev, "no PHY found\n");
			return -ENXIO;
		}

		/* attach the mac to the phy */
		ret = phylink_connect_phy(bp->phylink, phydev);
	}

The call to phylink_of_phy_connect() will go looking in device tree to
find the phy-handle. If it exists, it follows it to the PHY, and the
PHY is connected to the MAC. This code also handles fixed link. But in
this case, because it is using phylink, not phylib, the emulation is
different. The phylib fixed-link has the limitation of only emulating
C22 PHYs upto 1Gbps. 2.5G, 10G etc is becoming more popular, so
Russell King implemented fixed-link in phylink differently. phylib has
emulated MDIO registers which the generic PHY driver uses. phylink
however incorporates fixed-link into the core code, there is no
emulation. And IOCTL is a stub.

Back to the code. If there is no PHY via device tree, it searches its
own MDIO bus for the first PHY, and connects that to the MAC. This is
obviously not ideal when you have multiple devices on the bus, so
using a phy-handle is best practice. This code is here for backwards
compatibility. macb has some funky backwards compatibility code with
respect to its MDIO bus. So you might be hitting a corner case which
is not handled correct.

> > There are plenty of examples to follow.
> 
> That's true, but knowing which ones do what you're trying to do is the
> hard part. If you already know how to do it, it's easy to find
> examples showing it.  :)

Feel free to ask.

     Andrew

Re: net: macb: fail when there's no PHY

[Grant Edwards]

On 2020-12-02, Andrew Lunn <andrew@lunn.ch> wrote:

>>> So it will access the MDIO bus of the PHY that is attached to the
>>> MAC.
>>
>> If that's the case, wouldn't the ioctl() calls "just work" even when
>> only the fixed-phy mdio bus and fake PHY are declared in the device
>> tree?
>
> The fixed-link PHY is connected to the MAC. So the IOCTL calls will be
> made to the fixed-link fake MDIO bus.

Ah! When you said "the PHY that is attached to the MAC" above, I
thought you meant electrically attached to the MAC via the mdio bus.

Then how does Forian Fainelli's solution below work? Won't the first
phy found be the fixed one, and then the ioctl() calls will use the
fixed-link bus?

Florian Fainelli wrote:

>>> You should be able to continue having the macb MDIO bus controller be
>>> registered with no PHY/MDIO devices represented in Device Tree such that
>>> user-space can continue to use it for ioctl() *and* you can point the
>>> macb node to a fixed PHY for the purpose of having fixed link parameters.
>>>
>>> There are various drivers that support exactly this mode of operation
>>> where they use a fixed PHY for the Ethernet MAC link parameters, yet
>>> their MDIO bus controller is used to interface with other devices such
>>> as Ethernet switches over MDIO. Example of drivers supporting that are
>>> stmmac, fec, mtk_star_emac and ag71xx. The way it ends up looking like
>>> in Device Tree is the following:
>>>
>>> &eth0 {
>>>         fixed-link {
>>>                 speed = <1000>;
>>>                 full-duplex;
>>>         };
>>>
>>>         mdio {
>>>                 phy0: phy@0 {
>>>                         reg = <0>;
>>>                 };
>>>         };
>>> };

Re: net: macb: fail when there's no PHY

[Florian Fainelli]

On 12/2/2020 7:03 PM, Grant Edwards wrote:
> On 2020-12-02, Andrew Lunn <andrew@lunn.ch> wrote:
> 
>>>> So it will access the MDIO bus of the PHY that is attached to the
>>>> MAC.
>>>
>>> If that's the case, wouldn't the ioctl() calls "just work" even when
>>> only the fixed-phy mdio bus and fake PHY are declared in the device
>>> tree?
>>
>> The fixed-link PHY is connected to the MAC. So the IOCTL calls will be
>> made to the fixed-link fake MDIO bus.
> 
> Ah! When you said "the PHY that is attached to the MAC" above, I
> thought you meant electrically attached to the MAC via the mdio bus.
> 
> Then how does Forian Fainelli's solution below work? Won't the first
> phy found be the fixed one, and then the ioctl() calls will use the
> fixed-link bus?

You would have to have a local hack that intercepts the macb_ioctl() and
instead of calling phylink_mii_ioctl() it would have to implement a
custom ioctl() that does what drivers/net/phy/phy.c::phy_mii_ioctl does
except the mdiobus should be pointed to the MACB MDIO bus instance and
not be derived from the phy_device instance (because that one points to
the fixed PHY).
-- 
Florian

Re: net: macb: fail when there's no PHY

[Grant Edwards]

On 2020-12-03, Florian Fainelli <f.fainelli@gmail.com> wrote:

> You would have to have a local hack that intercepts the macb_ioctl()
> and instead of calling phylink_mii_ioctl() it would have to
> implement a custom ioctl() that does what
> drivers/net/phy/phy.c::phy_mii_ioctl does except the mdiobus should
> be pointed to the MACB MDIO bus instance and not be derived from the
> phy_device instance (because that one points to the fixed PHY).

So I can avoid my local hack to macb_main.c by doing a doing a local
hack to macb_main.c?

--
Grant

Re: net: macb: fail when there's no PHY

[Andrew Lunn]

> So I can avoid my local hack to macb_main.c by doing a doing a local
> hack to macb_main.c?

User space drivers were never supported in any meaningful way. The
IOCTL call is basically there for mii-tool, and nothing much more.

The way to avoid your local hack is to move your drivers into the
kernel, along side all the other drivers for devices on MDIO busses.

	Andrew

Re: net: macb: fail when there's no PHY

[Grant Edwards]

On 2020-12-03, Andrew Lunn <andrew@lunn.ch> wrote:
>> So I can avoid my local hack to macb_main.c by doing a doing a local
>> hack to macb_main.c?
>
> User space drivers were never supported in any meaningful way. The
> IOCTL call is basically there for mii-tool, and nothing much more.

I probably wouldn't call a single ioctl() to check the link status a
user-space-driver, but I guess that's what it is. If it's good enough
for the mii-tool, it's good enough for me.

> The way to avoid your local hack is to move your drivers into the
> kernel, along side all the other drivers for devices on MDIO busses.

I don't think I can justify the additional effort to devlope and
maintain a custom kern-space driver. We've already got a custom macb
driver, and writing a spearate driver in order to remove a handful of
lines from the macb driver just isn't worth it.

What has confused me all along was Florian Fainelli's post:

>> [I modified the macb driver to support fixed PHY plus mdio access]
>
> That should be unnecessary see below.
>
>> Was there some other way I should have done this with a 5.4 kernel
>> that I was unable to discover?
>
> You should be able to continue having the macb MDIO bus controller
> be registered with no PHY/MDIO devices represented in Device Tree
> such that user-space can continue to use it for ioctl() *and* you
> can point the macb node to a fixed PHY for the purpose of having
> fixed link parameters.

But to do that, I have to modify the macb driver to support a fixed
PHY plus mdio access. What would be the advantage of that modification
over the modification I've already made and tested?

Alternatively, I could write a seperate kernel driver that would "own"
the macb's mdio bus and provide some something equivalent to the
existing SIOC[SG]MIIREG ioctl() calls to access the devices on that
mdio bus.

Thanks for clearing this up for me.

BTW Andrew, we're still shipping plenty of product that running
eCos. :)

--
Grant

Re: net: macb: fail when there's no PHY

[Andrew Lunn]

On Thu, Dec 03, 2020 at 03:07:58PM -0000, Grant Edwards wrote:
> On 2020-12-03, Andrew Lunn <andrew@lunn.ch> wrote:
> >> So I can avoid my local hack to macb_main.c by doing a doing a local
> >> hack to macb_main.c?
> >
> > User space drivers were never supported in any meaningful way. The
> > IOCTL call is basically there for mii-tool, and nothing much more.
> 
> I probably wouldn't call a single ioctl() to check the link status a
> user-space-driver, but I guess that's what it is. If it's good enough
> for the mii-tool, it's good enough for me.
> 
> > The way to avoid your local hack is to move your drivers into the
> > kernel, along side all the other drivers for devices on MDIO busses.
> 
> I don't think I can justify the additional effort to devlope and
> maintain a custom kern-space driver.

Why custom? You should contribute it.

> >> Was there some other way I should have done this with a 5.4 kernel
> >> that I was unable to discover?

Ah, i missed you are using 5.4. You should probably jump to 5.10. There
have been quite a few changes in this area in the macb driver.
> 
> BTW Andrew, we're still shipping plenty of product that running
> eCos. :)

Cool. There is still a space for such an RTOS, linux has not yet taken
over the world everywhere.

     Andrew

Re: net: macb: fail when there's no PHY

[Grant Edwards]

On 2020-12-03, Andrew Lunn <andrew@lunn.ch> wrote:
> On Thu, Dec 03, 2020 at 03:07:58PM -0000, Grant Edwards wrote:
>
>> I don't think I can justify the additional effort to devlope and
>> maintain a custom kern-space driver.
>
> Why custom? You should contribute it.

I can't imagine that it would be useful to anybody else. My attempts
to donate drivers in the past were unproductive wastes of time, so I'm
not doing that again.

>>>> Was there some other way I should have done this with a 5.4 kernel
>>>> that I was unable to discover?
>
> Ah, i missed you are using 5.4. You should probably jump to
> 5.10. There have been quite a few changes in this area in the macb
> driver.

I don't think there's any way I could justify using a kernel that
doesn't have long-term support.

[It looks like we're going to have to abandon the effort to use
5.4. The performance is so bad compared to 2.6.33.7 that our product
just plain won't work. We've already had remove features to the get
5.4 kernel down to a usable size, but we were prepared to live with
that.]

>> BTW Andrew, we're still shipping plenty of product that running
>> eCos. :)
>
> Cool. There is still a space for such an RTOS, linux has not yet taken
> over the world everywhere.

I keep thinking that one of these days something is going to happen
that will force us to switch to a different RTOS, but it hasn't
happened yet...

--
Grant

Re: net: macb: fail when there's no PHY

[Andrew Lunn]

> I don't think there's any way I could justify using a kernel that
> doesn't have long-term support.

5.10 is LTS. Well, it will be, once it is actually released!

     Andrew

> [It looks like we're going to have to abandon the effort to use
> 5.4. The performance is so bad compared to 2.6.33.7 that our product
> just plain won't work. We've already had remove features to the get
> 5.4 kernel down to a usable size, but we were prepared to live with
> that.]

Ah. Small caches? The OpenWRT guys make valid complaints that the code
hot path of the network stack is getting too big to fit in the cache
of small systems. So there is a lot of cache misses and performance is
not good. If i remember correctly, just having netfilter in the build
is bad, even if it is not used.

   Andrew

Re: net: macb: fail when there's no PHY

[Grant Edwards]

On 2020-12-03, Andrew Lunn <andrew@lunn.ch> wrote:
>> I don't think there's any way I could justify using a kernel that
>> doesn't have long-term support.
>
> 5.10 is LTS. Well, it will be, once it is actually released!

Convincing people to ship an unreleased kernel would be a whole
'nother bucket of worms.

It's all moot now. The decision was just made to shelve the 5.4 kernel
"upgrade" and stick with 2.6.33 for now.

>> [It looks like we're going to have to abandon the effort to use
>> 5.4. The performance is so bad compared to 2.6.33.7 that our product
>> just plain won't work. We've already had remove features to the get
>> 5.4 kernel down to a usable size, but we were prepared to live with
>> that.]
>
> Ah. Small caches?

Yep. It's An old Atmel ARM926T part (at91sam9g20) with 32KB I-cache
and 32KB D-cache.

A simple user-space multi-threaded TCP echo server benchmark showed a
30-50% (depending on number of parallel connections) drop in max
throughput. Our typical applications also show a 15-25% increase in
CPU usage for an equivalent workload.  Another problem is high
latencies with 5.4. A thread that is supposed to wake up every 1ms
works reliably on 2.6.33, but is a long ways from working on 5.4.

I asked on the arm kernel mailing list if this was typical/expected,
but the post never made it past the moderator.

> The OpenWRT guys make valid complaints that the code
> hot path of the network stack is getting too big to fit in the cache
> of small systems. So there is a lot of cache misses and performance is
> not good. If i remember correctly, just having netfilter in the build
> is bad, even if it is not used.

We've already disabled absoltely everything we can and still have a
working system. With the same features enabled, the 5.4 kernel was
about 75% larger than a 2.6.33 kernel, so we had to trim quite a bit
of meat to get it to boot on existing units.

We also can't get on-die ECC support for Micron NAND flash working
with 5.4. Even it did work, we'd still have to add the ability to
fall-back to SW ECC on read operations for the sake of backwards
compatibility on units that were initially shipped without on-die ECC
support enabled.

--
Grant

Re: net: macb: fail when there's no PHY

[Alexander Dahl]

Hello Grant,

sorry if I just hijack your conversation, but I'm curious, because we are 
using the same SoC.  Adding linux-arm-kernel to Cc for the general performance 
issues and linux-mtd for the ECC questions. O:-)

Am Donnerstag, 3. Dezember 2020, 23:20:38 CET schrieb Grant Edwards:
> On 2020-12-03, Andrew Lunn <andrew@lunn.ch> wrote:
> >> I don't think there's any way I could justify using a kernel that
> >> doesn't have long-term support.
> > 
> > 5.10 is LTS. Well, it will be, once it is actually released!
> 
> Convincing people to ship an unreleased kernel would be a whole
> 'nother bucket of worms.

+1

Judging just from the release dates of the last LTS kernels, I would have 
guessed v5.11 will get LTS.  But there has been no announcement yet and I 
suppose there will be none before release?  For ordinary users it's just like 
staring in a crystal ball, so we aim at v5.4 for our more recent hardware 
platforms. ¯\_(ツ)_/¯

> 
> It's all moot now. The decision was just made to shelve the 5.4 kernel
> "upgrade" and stick with 2.6.33 for now.
> 
> >> [It looks like we're going to have to abandon the effort to use
> >> 5.4. The performance is so bad compared to 2.6.33.7 that our product
> >> just plain won't work. We've already had remove features to the get
> >> 5.4 kernel down to a usable size, but we were prepared to live with
> >> that.]
> > 
> > Ah. Small caches?
> 
> Yep. It's An old Atmel ARM926T part (at91sam9g20) with 32KB I-cache
> and 32KB D-cache.
> 
> A simple user-space multi-threaded TCP echo server benchmark showed a
> 30-50% (depending on number of parallel connections) drop in max
> throughput. Our typical applications also show a 15-25% increase in
> CPU usage for an equivalent workload.  Another problem is high
> latencies with 5.4. A thread that is supposed to wake up every 1ms
> works reliably on 2.6.33, but is a long ways from working on 5.4.

We use the exact same SoC with kernel 4.9.220-rt143 (PREEMPT RT) currently, 
after being stuck on 2.6.36.4 for quite a while.  I did not notice significant 
performance issues, but I have to admit, we never did extensive benchmarks on 
network or CPU performance, because the workload also changed for that target.

However what gave us a lot less dropped packages was using the internal SRAM 
as DMA buffer for RX packages received by macb.  That did not make it in 
mainline however, I did not put enough effort in polishing that patch back 
when we migrated from 2.6 to 4.x.  If you're curious, it's on GitHub: 
https://github.com/LeSpocky/linux/tree/net-macb-sram-rx

> I asked on the arm kernel mailing list if this was typical/expected,
> but the post never made it past the moderator.
> 
> > The OpenWRT guys make valid complaints that the code
> > hot path of the network stack is getting too big to fit in the cache
> > of small systems. So there is a lot of cache misses and performance is
> > not good. If i remember correctly, just having netfilter in the build
> > is bad, even if it is not used.
> 
> We've already disabled absoltely everything we can and still have a
> working system. With the same features enabled, the 5.4 kernel was
> about 75% larger than a 2.6.33 kernel, so we had to trim quite a bit
> of meat to get it to boot on existing units.

Same here.  v4.9 kernel image still fits, v4.14 is already too big for some 
devices we delivered in the early days.

> We also can't get on-die ECC support for Micron NAND flash working
> with 5.4. Even it did work, we'd still have to add the ability to
> fall-back to SW ECC on read operations for the sake of backwards
> compatibility on units that were initially shipped without on-die ECC
> support enabled.

IIRC the SoC itself has issues with its ECC engine? See mainline 
at91sam9g20ek_common.dtsi for example which sets nand-ecc-mode to "soft".

The SAM9G20 Errata chapter in the complete datasheet from 2015 (Atmel-6384F) 
says two times in Section 44.1.3: "Perform the ECC computation by software."

Greets
Alex

Re: net: macb: fail when there's no PHY

[Alexander Dahl]

Hello Grant,

sorry if I just hijack your conversation, but I'm curious, because we are 
using the same SoC.  Adding linux-arm-kernel to Cc for the general performance 
issues and linux-mtd for the ECC questions. O:-)

Am Donnerstag, 3. Dezember 2020, 23:20:38 CET schrieb Grant Edwards:
> On 2020-12-03, Andrew Lunn <andrew@lunn.ch> wrote:
> >> I don't think there's any way I could justify using a kernel that
> >> doesn't have long-term support.
> > 
> > 5.10 is LTS. Well, it will be, once it is actually released!
> 
> Convincing people to ship an unreleased kernel would be a whole
> 'nother bucket of worms.

+1

Judging just from the release dates of the last LTS kernels, I would have 
guessed v5.11 will get LTS.  But there has been no announcement yet and I 
suppose there will be none before release?  For ordinary users it's just like 
staring in a crystal ball, so we aim at v5.4 for our more recent hardware 
platforms. ¯\_(ツ)_/¯

> 
> It's all moot now. The decision was just made to shelve the 5.4 kernel
> "upgrade" and stick with 2.6.33 for now.
> 
> >> [It looks like we're going to have to abandon the effort to use
> >> 5.4. The performance is so bad compared to 2.6.33.7 that our product
> >> just plain won't work. We've already had remove features to the get
> >> 5.4 kernel down to a usable size, but we were prepared to live with
> >> that.]
> > 
> > Ah. Small caches?
> 
> Yep. It's An old Atmel ARM926T part (at91sam9g20) with 32KB I-cache
> and 32KB D-cache.
> 
> A simple user-space multi-threaded TCP echo server benchmark showed a
> 30-50% (depending on number of parallel connections) drop in max
> throughput. Our typical applications also show a 15-25% increase in
> CPU usage for an equivalent workload.  Another problem is high
> latencies with 5.4. A thread that is supposed to wake up every 1ms
> works reliably on 2.6.33, but is a long ways from working on 5.4.

We use the exact same SoC with kernel 4.9.220-rt143 (PREEMPT RT) currently, 
after being stuck on 2.6.36.4 for quite a while.  I did not notice significant 
performance issues, but I have to admit, we never did extensive benchmarks on 
network or CPU performance, because the workload also changed for that target.

However what gave us a lot less dropped packages was using the internal SRAM 
as DMA buffer for RX packages received by macb.  That did not make it in 
mainline however, I did not put enough effort in polishing that patch back 
when we migrated from 2.6 to 4.x.  If you're curious, it's on GitHub: 
https://github.com/LeSpocky/linux/tree/net-macb-sram-rx

> I asked on the arm kernel mailing list if this was typical/expected,
> but the post never made it past the moderator.
> 
> > The OpenWRT guys make valid complaints that the code
> > hot path of the network stack is getting too big to fit in the cache
> > of small systems. So there is a lot of cache misses and performance is
> > not good. If i remember correctly, just having netfilter in the build
> > is bad, even if it is not used.
> 
> We've already disabled absoltely everything we can and still have a
> working system. With the same features enabled, the 5.4 kernel was
> about 75% larger than a 2.6.33 kernel, so we had to trim quite a bit
> of meat to get it to boot on existing units.

Same here.  v4.9 kernel image still fits, v4.14 is already too big for some 
devices we delivered in the early days.

> We also can't get on-die ECC support for Micron NAND flash working
> with 5.4. Even it did work, we'd still have to add the ability to
> fall-back to SW ECC on read operations for the sake of backwards
> compatibility on units that were initially shipped without on-die ECC
> support enabled.

IIRC the SoC itself has issues with its ECC engine? See mainline 
at91sam9g20ek_common.dtsi for example which sets nand-ecc-mode to "soft".

The SAM9G20 Errata chapter in the complete datasheet from 2015 (Atmel-6384F) 
says two times in Section 44.1.3: "Perform the ECC computation by software."

Greets
Alex




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: net: macb: fail when there's no PHY

[Alexander Dahl]

Hello Grant,

sorry if I just hijack your conversation, but I'm curious, because we are 
using the same SoC.  Adding linux-arm-kernel to Cc for the general performance 
issues and linux-mtd for the ECC questions. O:-)

Am Donnerstag, 3. Dezember 2020, 23:20:38 CET schrieb Grant Edwards:
> On 2020-12-03, Andrew Lunn <andrew@lunn.ch> wrote:
> >> I don't think there's any way I could justify using a kernel that
> >> doesn't have long-term support.
> > 
> > 5.10 is LTS. Well, it will be, once it is actually released!
> 
> Convincing people to ship an unreleased kernel would be a whole
> 'nother bucket of worms.

+1

Judging just from the release dates of the last LTS kernels, I would have 
guessed v5.11 will get LTS.  But there has been no announcement yet and I 
suppose there will be none before release?  For ordinary users it's just like 
staring in a crystal ball, so we aim at v5.4 for our more recent hardware 
platforms. ¯\_(ツ)_/¯

> 
> It's all moot now. The decision was just made to shelve the 5.4 kernel
> "upgrade" and stick with 2.6.33 for now.
> 
> >> [It looks like we're going to have to abandon the effort to use
> >> 5.4. The performance is so bad compared to 2.6.33.7 that our product
> >> just plain won't work. We've already had remove features to the get
> >> 5.4 kernel down to a usable size, but we were prepared to live with
> >> that.]
> > 
> > Ah. Small caches?
> 
> Yep. It's An old Atmel ARM926T part (at91sam9g20) with 32KB I-cache
> and 32KB D-cache.
> 
> A simple user-space multi-threaded TCP echo server benchmark showed a
> 30-50% (depending on number of parallel connections) drop in max
> throughput. Our typical applications also show a 15-25% increase in
> CPU usage for an equivalent workload.  Another problem is high
> latencies with 5.4. A thread that is supposed to wake up every 1ms
> works reliably on 2.6.33, but is a long ways from working on 5.4.

We use the exact same SoC with kernel 4.9.220-rt143 (PREEMPT RT) currently, 
after being stuck on 2.6.36.4 for quite a while.  I did not notice significant 
performance issues, but I have to admit, we never did extensive benchmarks on 
network or CPU performance, because the workload also changed for that target.

However what gave us a lot less dropped packages was using the internal SRAM 
as DMA buffer for RX packages received by macb.  That did not make it in 
mainline however, I did not put enough effort in polishing that patch back 
when we migrated from 2.6 to 4.x.  If you're curious, it's on GitHub: 
https://github.com/LeSpocky/linux/tree/net-macb-sram-rx

> I asked on the arm kernel mailing list if this was typical/expected,
> but the post never made it past the moderator.
> 
> > The OpenWRT guys make valid complaints that the code
> > hot path of the network stack is getting too big to fit in the cache
> > of small systems. So there is a lot of cache misses and performance is
> > not good. If i remember correctly, just having netfilter in the build
> > is bad, even if it is not used.
> 
> We've already disabled absoltely everything we can and still have a
> working system. With the same features enabled, the 5.4 kernel was
> about 75% larger than a 2.6.33 kernel, so we had to trim quite a bit
> of meat to get it to boot on existing units.

Same here.  v4.9 kernel image still fits, v4.14 is already too big for some 
devices we delivered in the early days.

> We also can't get on-die ECC support for Micron NAND flash working
> with 5.4. Even it did work, we'd still have to add the ability to
> fall-back to SW ECC on read operations for the sake of backwards
> compatibility on units that were initially shipped without on-die ECC
> support enabled.

IIRC the SoC itself has issues with its ECC engine? See mainline 
at91sam9g20ek_common.dtsi for example which sets nand-ecc-mode to "soft".

The SAM9G20 Errata chapter in the complete datasheet from 2015 (Atmel-6384F) 
says two times in Section 44.1.3: "Perform the ECC computation by software."

Greets
Alex




______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: net: macb: fail when there's no PHY

[Grant Edwards]

On 2020-12-04, Alexander Dahl <ada@thorsis.com> wrote:

> sorry if I just hijack your conversation, but I'm curious, because
> we are using the same SoC.  Adding linux-arm-kernel to Cc for the
> general performance issues and linux-mtd for the ECC questions. O:-)

No worries. I tried to ask about performance issues on
linux-arm-kernel, but AFAICT, my post wasn't allowed by the moderator.

> Am Donnerstag, 3. Dezember 2020, 23:20:38 CET schrieb Grant Edwards:
>> On 2020-12-03, Andrew Lunn <andrew@lunn.ch> wrote:

>>>> I don't think there's any way I could justify using a kernel that
>>>> doesn't have long-term support.
>>> 
>>> 5.10 is LTS. Well, it will be, once it is actually released!
>> 
>> Convincing people to ship an unreleased kernel would be a whole
>> 'nother bucket of worms.
>
> +1
>
> Judging just from the release dates of the last LTS kernels, I would
> have guessed v5.11 will get LTS.  But there has been no announcement
> yet and I suppose there will be none before release?  For ordinary
> users it's just like staring in a crystal ball, so we aim at v5.4
> for our more recent hardware platforms. ¯\_(ツ)_/¯

Exactly. We're already behind schedule, and just assuming that
5.<whatever> is going to be LTS and will be released in time for
shipment just won't fly.

>> A simple user-space multi-threaded TCP echo server benchmark showed
>> a 30-50% (depending on number of parallel connections) drop in max
>> throughput. Our typical applications also show a 15-25% increase in
>> CPU usage for an equivalent workload.  Another problem is high
>> latencies with 5.4. A thread that is supposed to wake up every 1ms
>> works reliably on 2.6.33, but is a long ways from working on 5.4.
>
> We use the exact same SoC with kernel 4.9.220-rt143 (PREEMPT RT)
> currently, after being stuck on 2.6.36.4 for quite a while.  I did
> not notice significant performance issues, but I have to admit, we
> never did extensive benchmarks on network or CPU performance,
> because the workload also changed for that target.

The performance hit varied quite a bit depening on the application,
but seemed to be a minimum of about a 15% increase in CPU usage.

We discussed trying various older LTS kernels to see if we could find
one that performed acceptably, but it would take a lot of engineering
time to port and benchmark each version.

> However what gave us a lot less dropped packages was using the
> internal SRAM as DMA buffer for RX packages received by macb.  That
> did not make it in mainline however, I did not put enough effort in
> polishing that patch back when we migrated from 2.6 to 4.x.  If
> you're curious, it's on GitHub:
> https://github.com/LeSpocky/linux/tree/net-macb-sram-rx

We haven't identified the source of the drop in network throughput,
but the increased CPU usage and problems with latency affect
applications that don't use the network at all (and there is no
network traffic).

>> We've already disabled absoltely everything we can and still have a
>> working system. With the same features enabled, the 5.4 kernel was
>> about 75% larger than a 2.6.33 kernel, so we had to trim quite a bit
>> of meat to get it to boot on existing units.
>
> Same here.  v4.9 kernel image still fits, v4.14 is already too big for some 
> devices we delivered in the early days.

Yea, I was shocked at the massive amount of bloat.

>> We also can't get on-die ECC support for Micron NAND flash working
>> with 5.4. Even it did work, we'd still have to add the ability to
>> fall-back to SW ECC on read operations for the sake of backwards
>> compatibility on units that were initially shipped without on-die
>> ECC support enabled.
>
> IIRC the SoC itself has issues with its ECC engine? [...]

Sorry, then terminology in the kernel's nand subsystem is a bit
vague. The "on-die" ECC support refers to using the ECC hardware built
in to the NAND flash device itself. In the Linux nand subsystem
"hardware" or "HW" ECC refers to the ECC hardware built into the
SoC. You're right, that's broken in the '9G20 and we don't use it.

We added "on-die" support to the 2.6.33 kernel with fallback to SW ECC
on read operations for backwards compatibility. It was pretty
straight-forward and works well. The 5.4 kernel's on-die support is
several orders of magnitude more complex (I don't yet know why), and
doesn't offer SW fallback.

One of the odd things about the micron on-die support in the 5.4
kernel is that it's constantly being switched on and off. It's turned
on before every page read/write, then off afterwards. This adds a lot
of overhead to page read/write operations. After about the 6th on/off
cycle, that stops working and the "set-feature" function starts
returning an error every time. Read operations produce the correct
data, but always report uncorrectable flipped bits when there aren't
any (haven't figured out why). I haven't tried writes.

In our 2.6.33 on-die support we identify the chip at boot up. If it
has on-die support, we turn it on, leave it on, and just use it that
way. If a read operation returns an error, we check to see if the page
was written with SW ECC and if it was, use that. Writes are always
done with on-die ECC.

--
Grant


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: net: macb: fail when there's no PHY

[Andrew Lunn]

> > > 5.10 is LTS. Well, it will be, once it is actually released!
> > 
> > Convincing people to ship an unreleased kernel would be a whole
> > 'nother bucket of worms.
> 
> +1
> 
> Judging just from the release dates of the last LTS kernels, I would have 
> guessed v5.11 will get LTS.  But there has been no announcement yet and I 
> suppose there will be none before release?

It was announced a while ago. e.g:

https://itsfoss.com/kernel-5-10/

	Andrew

Re: net: macb: fail when there's no PHY

[Andrew Lunn]

> > > 5.10 is LTS. Well, it will be, once it is actually released!
> > 
> > Convincing people to ship an unreleased kernel would be a whole
> > 'nother bucket of worms.
> 
> +1
> 
> Judging just from the release dates of the last LTS kernels, I would have 
> guessed v5.11 will get LTS.  But there has been no announcement yet and I 
> suppose there will be none before release?

It was announced a while ago. e.g:

https://itsfoss.com/kernel-5-10/

	Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: net: macb: fail when there's no PHY

[Andrew Lunn]

> > > 5.10 is LTS. Well, it will be, once it is actually released!
> > 
> > Convincing people to ship an unreleased kernel would be a whole
> > 'nother bucket of worms.
> 
> +1
> 
> Judging just from the release dates of the last LTS kernels, I would have 
> guessed v5.11 will get LTS.  But there has been no announcement yet and I 
> suppose there will be none before release?

It was announced a while ago. e.g:

https://itsfoss.com/kernel-5-10/

	Andrew

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: net: macb: fail when there's no PHY

[Florian Fainelli]

On 12/2/2020 7:54 PM, Grant Edwards wrote:
> On 2020-12-03, Florian Fainelli <f.fainelli@gmail.com> wrote:
> 
>> You would have to have a local hack that intercepts the macb_ioctl()
>> and instead of calling phylink_mii_ioctl() it would have to
>> implement a custom ioctl() that does what
>> drivers/net/phy/phy.c::phy_mii_ioctl does except the mdiobus should
>> be pointed to the MACB MDIO bus instance and not be derived from the
>> phy_device instance (because that one points to the fixed PHY).
> 
> So I can avoid my local hack to macb_main.c by doing a doing a local
> hack to macb_main.c?

There is value in having the macb driver support the scheme that was
just described which is to support a fixed PHY as far as the MAC link
parameters go, and also support registering the MACB internal MDIO bus
to interface with other devices. People using DSA would typically fall
under that category.

The fact that you need to access the MACB internal MDIO bus to interface
with your PHYs would be a hack that is easier to carry forward, and
maybe more justifiable.

I don't have a dog in the fight, but dealing myself with cute little
hacks in our downstream Linux kernel, the better it fits with useful
functionality to other people, the better.
-- 
Florian

Re: net: macb: fail when there's no PHY

[Grant Edwards]

On 2020-12-04, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 12/2/2020 7:54 PM, Grant Edwards wrote:
>> On 2020-12-03, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> 
>>> You would have to have a local hack that intercepts the macb_ioctl()
>>> and instead of calling phylink_mii_ioctl() it would have to
>>> implement a custom ioctl() that does what
>>> drivers/net/phy/phy.c::phy_mii_ioctl does except the mdiobus should
>>> be pointed to the MACB MDIO bus instance and not be derived from the
>>> phy_device instance (because that one points to the fixed PHY).
>> 
>> So I can avoid my local hack to macb_main.c by doing a doing a local
>> hack to macb_main.c?
>
> There is value in having the macb driver support the scheme that was
> just described which is to support a fixed PHY as far as the MAC link
> parameters go, and also support registering the MACB internal MDIO bus
> to interface with other devices. People using DSA would typically fall
> under that category.

My hack does support both a fixed PHY as far as the MAC link
parameters go and allows interfacing with other devices via the mdio
bus, so I assume you're saying that there's value in doing that in the
"standard" way instead of the way I invented 10 years ago.

That would certainly be true if we were talking about something to be
incorporated "upstream", but like you said: it would be a local
hack. I see no intrinsic value in changing to the "standard" DSA
scheme. Switching to a different API would actually create additional
cost above and beyond the cost of writing and testing the new local
hack, since all of the applications which need to access the mdio bus
would also have to change.

If I could avoid the local hack completely by using a standard,
existing feature and API, then I could make an arguement for modifying
the applications. But proposing the writing of a new, more comlex
local hack and modifying the applications just so that we can feel
good about using a standard API would be laughed at.

> [...]
>
> I don't have a dog in the fight, but dealing myself with cute little
> hacks in our downstream Linux kernel, the better it fits with useful
> functionality to other people, the better.

I don't see why it makes any difference how well suited a local hack
is to people who will never see it or use it. It would seem to me that
the the most important attribute of a local hack would be simplicity
and ease of understanding.  My hack is 7 lines of code and one line of
a static structure declaration and initializer, all
enabled/disabled/controlled by a single preprocessor symbol.

--
Grant

Re: net: macb: fail when there's no PHY

[Florian Fainelli]

On 12/4/2020 6:52 PM, Grant Edwards wrote:
> On 2020-12-04, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 12/2/2020 7:54 PM, Grant Edwards wrote:
>>> On 2020-12-03, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>
>>>> You would have to have a local hack that intercepts the macb_ioctl()
>>>> and instead of calling phylink_mii_ioctl() it would have to
>>>> implement a custom ioctl() that does what
>>>> drivers/net/phy/phy.c::phy_mii_ioctl does except the mdiobus should
>>>> be pointed to the MACB MDIO bus instance and not be derived from the
>>>> phy_device instance (because that one points to the fixed PHY).
>>>
>>> So I can avoid my local hack to macb_main.c by doing a doing a local
>>> hack to macb_main.c?
>>
>> There is value in having the macb driver support the scheme that was
>> just described which is to support a fixed PHY as far as the MAC link
>> parameters go, and also support registering the MACB internal MDIO bus
>> to interface with other devices. People using DSA would typically fall
>> under that category.
> 
> My hack does support both a fixed PHY as far as the MAC link
> parameters go and allows interfacing with other devices via the mdio
> bus, so I assume you're saying that there's value in doing that in the
> "standard" way instead of the way I invented 10 years ago.

All I was doing was explaining how this can be done today, in a way that
is useful to you and other people. You want to keep doing things your
own way, go ahead.

> 
> That would certainly be true if we were talking about something to be
> incorporated "upstream", but like you said: it would be a local
> hack. I see no intrinsic value in changing to the "standard" DSA
> scheme. Switching to a different API would actually create additional
> cost above and beyond the cost of writing and testing the new local
> hack, since all of the applications which need to access the mdio bus
> would also have to change.

I was not trying to convince you to switch to DSA, and if this is an
area of technical debt that has a low cost for your platform compared to
others, so be it. What could have been useful was to support the
standard fixed PHY plus the internal MDIO bus.

> 
> If I could avoid the local hack completely by using a standard,
> existing feature and API, then I could make an arguement for modifying
> the applications. But proposing the writing of a new, more comlex
> local hack and modifying the applications just so that we can feel
> good about using a standard API would be laughed at.

Only you can be the judge of that, I have no visibility into what
constitutes an acceptable change and what does not.

> 
>> [...]
>>
>> I don't have a dog in the fight, but dealing myself with cute little
>> hacks in our downstream Linux kernel, the better it fits with useful
>> functionality to other people, the better.
> 
> I don't see why it makes any difference how well suited a local hack
> is to people who will never see it or use it. It would seem to me that
> the the most important attribute of a local hack would be simplicity
> and ease of understanding.  My hack is 7 lines of code and one line of
> a static structure declaration and initializer, all
> enabled/disabled/controlled by a single preprocessor symbol.

A change can be 7 lines of code and be broken any time you update to a
newer version of the kernel, and that is probably even truer in your
case since you have such a big difference between 2.6.x and 5.4 that
anything in between could have been rewritten a dozen time.

Over the course of the past 6 years, we have managed to get our
downstream kernel from ~1300 patches downstream to only about 65 as of
5.10, but it took 6 years and counting and we only targeted LTS kernels.
Maybe I am overly sensitive to how maintainable a change is, who knows.
-- 
Florian

Re: net: macb: fail when there's no PHY

[Andrew Lunn]

On Thu, Dec 03, 2020 at 03:03:30AM -0000, Grant Edwards wrote:
> On 2020-12-02, Andrew Lunn <andrew@lunn.ch> wrote:
> 
> >>> So it will access the MDIO bus of the PHY that is attached to the
> >>> MAC.
> >>
> >> If that's the case, wouldn't the ioctl() calls "just work" even when
> >> only the fixed-phy mdio bus and fake PHY are declared in the device
> >> tree?
> >
> > The fixed-link PHY is connected to the MAC. So the IOCTL calls will be
> > made to the fixed-link fake MDIO bus.
> 
> Ah! When you said "the PHY that is attached to the MAC" above, I
> thought you meant electrically attached to the MAC via the mdio bus.

Ah, sorry. phylib and phylink have API calls to connect a MAC and a
PHY. phylink_connect_phy(), phy_connect(). I was thinking in those
terms. But if you don't know you way around this subsystem, the
misunderstanding is understanding.

	 Andrew

Re: net: macb: fail when there's no PHY

[Grant Edwards]

On Thu, Sep 21, 2017 at 01:05:57PM -0700, Florian Fainelli wrote:
> On 09/21/2017 12:59 PM, Grant Edwards wrote:
> > Several years back (circa 2.6.33) I had to hack up macb.c to work on
> > an at91 board that didn't have a PHY connected to the macb controller.
> > [...]
> > It looks like the macb driver still can't handle boards that don't
> > have a PHY.  Is that correct?
> 
> Not since:
> dacdbb4dfc1a1a1378df8ebc914d4fe82259ed46 ("net: macb: add fixed-link
> node support")
> 
> > What's the right way to deal with this?
> 
> Declaring a fixed PHY that will present an emulated link UP, with a
> fixed speed/duplex etc. is the way to go.

Apologies, I know this thread is a few years old, but I finally got
around to working with a newer kernel (5.4) that has the "fixed phy"
support. Unfortunately, the existing "fixed phy" support is unusable
for us. It doesn't just present a fake fixed, PHY. It replaces the
entire mii (mdio/mdc) bus with a fake _bus_. That means our code loses
the ability to talk to the devices that are attached to the macb's
mdio management bus.

So, I ended up porting my hack from the 2.6.33 macb.c driver to the
5.4 macb.c driver. It presents a fake PHY at one address on the mdio
bus, but still allows normal communication with devices at other
addresses on the bus. We use SIOC[SG]MIIREG ioctl() calls from
userspace to talk to those real devices. Adding a fake PHY to the
macb's mdio bus takes a total of about two dozen lines of code.

Was there some other way I should have done this with a 5.4 kernel
that I was unable to discover?

[Unfortunately, the performance of the 5.4 kernel on an ARM926 is so
bad I don't think we're going to be able to use it.]

--
Grant