Re: [PATCH net 0/1] phy: microchip_t1s: lan865x rev.b1 support

[Parthiban.Veerasooran]

Hi,

On 24/05/24 9:00 pm, Russell King (Oracle) wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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@xxxxxxxxxxxxx/)
>>
>> 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
Sorry for the misunderstanding here. Probably I should have used "avoid" 
in the place of "save" might clear the things properly. The intention of 
this comment is, PHY C45 direct access will be faster than PHY C45 
indirect access as PHY C45 indirect access needs 4 SPI bus access.

If you agree, I will correct it in the next version.

Best regards,
Parthiban V
> 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!