Received-SPF: pass (google.com: domain of linux-kernel+bounces-188871-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 24 May 2024 16:30:35 +0100
From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: =?iso-8859-1?Q?Ram=F3n?= Nordin Rodriguez <ramon.nordin.rodriguez@ferroamp.se>
Cc: andrew@lunn.ch, hkallweit1@gmail.com, davem@davemloft.net,
	edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	parthiban.veerasooran@microchip.com
Subject: Re: [PATCH net 0/1] phy: microchip_t1s: lan865x rev.b1 support
Message-ID: <ZlCym64L+T8SIjgt@shell.armlinux.org.uk>
References: <20240524140706.359537-1-ramon.nordin.rodriguez@ferroamp.se>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240524140706.359537-1-ramon.nordin.rodriguez@ferroamp.se>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>

On Fri, May 24, 2024 at 04:07:05PM +0200, Ram?n Nordin Rodriguez wrote:
> Hi,
> Let me first prepend this submission with 4 points:
> 
> * this is not in a merge-ready state
> * some code has been copied from the ongoing oa_tc6 work by Parthiban
> * this has to interop with code not yet merged (oa_tc6)
> * Microchip is looking into if rev.b0 can use the rev.b1 init procedure
> 
> The ongoing work by Parthiban Veerasooran is probably gonna get at least
> one more revision
> (https://lore.kernel.org/netdev/20240418125648.372526-1-Parthiban.Veerasooran@microchip.com/)
> 
> I'm publishing this early as it could benefit some of the discussions in
> the oa_tc6 threads, as well as giving other devs the possibility
> massaging things to a state where they can use the rev.b1 chip (rev.b0
> is eol).
> And I need feedback on how to wrap this up.
> 
> Far as I can tell the phy-driver cannot access some of the regs necessary
> for probing the hardware and performing the init/fixup without going
> over the spi interface.
> The MMDCTRL register (used with indirect access) can address
> 
> * PMA - mms 3
> * PCS - mms 2
> * Vendor specific / PLCA - mms 4
> 
> This driver needs to access mms (memory map seleector)
> * mac registers - mms 1,
> * vendor specific / PLCA - mms 4
> * vencor specific - mms 10
> 
> Far as I can tell, mms 1 and 10 are only accessible via spi. In the
> oa_tc6 patches this is enabled by the oa_tc6 framework by populating the
> mdiobus->read/write_c45 funcs.
> 
> In order to access any mms I needed I added the following change in the
> oa_tc6.c module
> 
> static int oa_tc6_get_phy_c45_mms(int devnum)
>  {
> +       if(devnum & BIT(31))
> +               return devnum & GENMASK(30, 0);
> 
> Which corresponds to the 'mms | BIT(31)' snippets in this commit, this
> is really not how things should be handled, and I need input on how to
> proceed here.

So if bit 31 of the devnum is set, then the other bits specify the
MMS instead of the MMD.

I'm not sure we want to overload the PHY interface in this way. We
have been down this path before with the MDIO bus read/write methods
being used for both C22 and C45 accesses, and it created problems,
so I don't think we want to repeat that mistake by doing the same
thing here.

There's a comment in the original patches etc about the PHY being
discovered via C22, and then not using the direct accesses to the
C45 register space. I'm wondering whether we should split
phydev->is_c45 to be phydev->probed_c45 / phydev->use_c45.

The former gets used during bus scanning and probe time to determine
how we match the device driver to the phydev. The latter gets used
_only_ to determine whether the read/write_mmd ops use direct mode
or indirect mode.

Before the driver probe is called, we should do:

	phydev->use_mmd = phydev->probed_c45;

to ensure that todays behaviour is preserved. Then, provide a
function, maybe, phy_use_direct_c45(phydev) which will set this
phydev->use_mmd flag, and phy_use_indirect_c45(phydev) which will
clear it.

phy_use_direct_c45() should also check whether the MDIO bus that
is attached supports C45 access, and return an error if not.

That will give you the ability to use the direct access method
where necessary.

There's a comment in the referred to code:

+	/* OPEN Alliance 10BASE-T1x compliance MAC-PHYs will have both C22 and
+	 * C45 registers space. If the PHY is discovered via C22 bus protocol it
+	 * assumes it uses C22 protocol and always uses C22 registers indirect
+	 * access to access C45 registers. This is because, we don't have a
+	 * clean separation between C22/C45 register space and C22/C45 MDIO bus
+	 * protocols. Resulting, PHY C45 registers direct access can't be used
+	 * which can save multiple SPI bus access. To support this feature, PHY
+	 * drivers can set .read_mmd/.write_mmd in the PHY driver to call
+	 * .read_c45/.write_c45. Ex: drivers/net/phy/microchip_t1s.c
+	 */

which I don't really understand. It claims that C45 direct access
"saves" multiple SPI bus accesses. However, C45 indirect access
requires:

1. A C22 write to the MMD control register
2. A C22 write to the MMD data register
3. Another C22 write to the MMD control register
4. A c22 read or write to access the actual data.

Do four C22 bus transactions over SPI require more or less SPI bus
accesses than a single C45 bus transaction over SPI? I suspect not,
which makes the comment above factually incorrect.

If we have direct C45 access working, does that remove the need to
have this special bit-31 to signal MMS access requirement?

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!