Hi Vladimir,

Hi Lukasz,

On Tue, Aug 29, 2023 at 10:35:33AM +0200, Lukasz Majewski wrote:

quoted

Hi Vladimir,
  

quoted

On Fri, Aug 25, 2023 at 06:48:41PM +0000,
Tristram.Ha@microchip.com wrote:  

quoted

quoted

quoted

IMHO adding functions to MMD modification would facilitate
further development (for example LED setup).    

We already have some KSZ9477 specific initialization done in
the Micrel PHY driver under drivers/net/phy/micrel.c, can we
converge on the PHY driver which has a reasonable amount of
infrastructure for dealing with workarounds, indirect or
direct MMD accesses etc.?    

Actually the internal PHY used in the KSZ9897/KSZ9477/KSZ9893
switches are special and only used inside those switches.
Putting all the switch related code in Micrel PHY driver does
not really help.  When the switch is reset all those PHY
registers need to be set again, but the PHY driver only
executes those code during PHY initialization.  I do not know
if there is a good way to tell the PHY to re-initialize again.
  

Suppose there was a method to tell the PHY driver to re-initialize
itself. What would be the key points in which the DSA switch
driver would need to trigger that method? Where is the switch
reset at runtime?  

Tristam has explained why adding the internal switch PHY errata to
generic PHY code is not optimal.  

Yes, and I didn't understand that explanation, so I asked a
clarification question.

Ok. Let's wait for Tristram's answer.

quoted

If adding MMD generic code is a problem - then I'm fine with just
clearing proper bits with just two indirect writes in the
drivers/net/dsa/microchip/ksz9477.c

I would also prefer to keep the separate ksz9477_errata() function,
so we could add other errata code there.

Just informative - without this patch the KSZ9477-EVB board's
network is useless when the other peer has EEE enabled by default
(like almost all non managed ETH switches).  

No, adding direct PHY MMD access code to the ksz9477 switch driver is
not even the biggest problem - even though, IIUC, the "workaround" to
disable EEE advertisement could be moved to ksz9477_get_features() in
drivers/net/phy/micrel.c, where phydev->supported_eee could be
cleared.

To be even more interesting (after looking into the PHY micrel.c code):
https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/micrel.c#L1804

The errata from this patch is already present.

The issue is that ksz9477_config_init() (drivers/net/phy/micrel.c) is
executed AFTER generic phy_probe():
https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy_device.c#L3256
in which the EEE advertisement registers are read.

Hence, those registers needs to be cleared earlier - as I do in
ksz9477_setup() in drivers/net/dsa/microchip/ksz9477.

Here the precedence matters ...

The biggest problem that I see is that Oleksij Rempel has "just" added
EEE support to the KSZ9477 earlier this year, with an ack from Arun
Ramadoss: 69d3b36ca045 ("net: dsa: microchip: enable EEE support").
I'm not understanding why the erratum wasn't a discussion topic then.

+1

I am currently on vacation and won't be able to look very deeply into
the problem, but IIUC, your patch undoes that work, and so, it needs
an ACK from Oleksij.

Ok.


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de