Re: [PATCH v3 net-next 05/10] net: dsa: microchip: add DSA support for microchip lan937x

On Wed, Aug 04, 2021 at 07:58:15PM +0530, Prasanna Vengateshan wrote:

On Wed, 2021-08-04 at 13:46 +0300, Vladimir Oltean wrote:

quoted

The problem is that I have no clear migration path for the drivers I
maintain, like sja1105, and I suspect that others might be in the exact
same situation.

Currently, if the sja1105 needs to add internal delays in a MAC-to-MAC
(fixed-link) setup, it does that based on the phy-mode string. So
"rgmii-id" + "fixed-link" means for sja1105 "add RX and TX RGMII
internal delays", even though the documentation now says "the MAC should
not add the RX or TX delays in this case".

There are 2 cases to think about, old driver with new DT blob and new
driver with old DT blob. If breakage is involved, I am not actually very
interested in doing the migration, because even though the interpretation
of the phy-mode string is inconsistent between the phy-handle and fixed-link
case (which was deliberate), at least it currently does all that I need it to.

I am not even clear what is the expected canonical behavior for a MAC
driver. It parses rx-internal-delay-ps and tx-internal-delay-ps, and
then what? It treats all "rgmii*" phy-mode strings identically? Or is it
an error to have "rgmii-rxid" for phy-mode and non-zero rx-internal-delay-ps?
If it is an error, should all MAC drivers check for it? And if it is an
error, does it not make migration even more difficult (adding an
rx-internal-delay-ps property to a MAC OF node which already uses
"rgmii-id" would be preferable to also having to change the "rgmii-id"
to "rgmii", because an old kernel might also need to work with that DT
blob, and that will ignore the new rx-internal-delay-ps property).

Considering the PHY is responsible to add internal delays w.r.to phy-mode, "*-
tx-internal-delay-ps" approach that i was applying to different connections as
shown below by bringing up different examples.

1) Fixed-link MAC-MAC: 
       port@4 {
            .....
            phy-mode = "rgmii";
            rx-internal-delay-ps = <xxx>;
            tx-internal-delay-ps = <xxx>;
            ethernet = <&ethernet>;
            fixed-link {
           	......
            };
          };

2) Fixed-link MAC-Unknown:
        port@5 {
            ......
            phy-mode = "rgmii-id";
            rx-internal-delay-ps = <xxx>;
            tx-internal-delay-ps = <xxx>;
            fixed-link {
           .	....
            };
          };

3) Fixed-link :
        port@5 {
            ......
            phy-mode = "rgmii-id";
            fixed-link {
              .....
            };
          };

From above examples,
	a) MAC node is responsible to add RGMII delay by parsing "*-internal-
delay-ps" for (1) & (2). Its a known item in this discussion.
	b) Is rgmii-* to be ignored by the MAC in (2) and just apply the delays
from MAC side? Because if its forced to have "rgmii", would it become just -

quoted

interface=*_MODE_RGMII and affects legacy?

Yes, I think the MAC would have to accept any "rgmii*" phy-mode in
fixed-link. The legacy behavior would be do to whatever it did before,
and the new behavior would be to NOT apply any MAC-level delays based on
the phy-mode value, but only based on the {rx,tx}-internal-delay-ps
properties if these are present, or fall back to the legacy behavior if
they aren't.

This way:
- New kernel with old DT blob falls back to legacy behavior
- New kernel with new DT blob finds the explicit {rx,tx}-internal-delay-ps
  properties and applies MAC-level delays only according to those, while
  accepting any phy-mode string
- Old kernel with new DT blob behaves the same as before, because it
  does not parse {rx,tx}-internal-delay-ps and we will not change its
  phy-mode.

	c) if MAC follows standard delay, then it needs to be validated against
"*-internal-delay-ps", may be validating against single value and throw an
error. Might be okay.

Drivers with no legacy might throw an error if:
- phy-mode == "rgmii-id" or "rgmii-rxid" and there is a non-zero rx-internal-delay-ps
- phy-mode == "rgmii-id" or "rgmii-txid" and there is a non-zero tx-internal-delay-ps

but considering that most drivers already have a legacy to support, I'm
not sure how useful that error will be.

	d) For 3), Neither MAC nor other side will apply delays. Expected.

In the "new" behavior, correct. In "legacy" behavior, they might have to.

3) MAC-PHY

	i) &test3 {
		phy-handle = <&phy0>;
		phy-mode = "rgmii-id";
		phy0: ethernet-phy@xx {
			.....
			rx-internal-delay = <xxx>;
			tx-internal-delay = <xxx>;
		};
	  };

	ii) &test4 {
		phy-handle = <&phy0>;
		phy-mode = "rgmii";
        	rx-internal-delay-ps = <xxx>;
        	tx-internal-delay-ps = <xxx>;
		phy0: ethernet-phy@xx {
			reg = <x>;
	        };
	     };


For 3(i), I assume phy would apply internal delay values by checking its phydev-

quoted

interface.

PHY drivers have a phy_get_internal_delay() helper that takes into
consideration both the phy-mode value and the {rx,tx}-internal-delay
properties. In example 3(i), the {rx,tx}-internal-delay properties would
prevail as long as the PHY driver uses that helper.

For 3(ii), MAC would apply the delays.

Overall, only (b) need a right decision? or any other items are missed?


Prasanna V