Hi Andrew,

On 15/04/24 6:45 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Fri, Apr 12, 2024 at 10:43:15AM +0000, Parthiban.Veerasooran@microchip.com wrote:
>> Hi Andrew,
>>
>> After implementing the new APIs oa_tc6_mdiobus_read_c45() and
>> oa_tc6_mdiobus_write_c45(), I tried testing. But, for some reason these
>> APIs are never get called by phy_read_mmd() or phy_write_mmd() as those
>> APIs are checking for phydev->is_c45 flag for calling this new c45 APIs
>> which is not set in this case.
>>
>> If the phydev is found via c22, phylib does not set phydev->is_c45, and
>> everything ends up going indirect.
> 
> We don't have a clean separation between C22/C45 register space and
> C22/C45 MDIO bus protocols. As you said, if the PHY is discovered via
> C22 bus protocol it assumes it uses C22 protocol.
> 
Thanks for confirming my understanding.
> Those PHYs which do support C45, and in particular, features which
> need C45, all call genphy_c45_pma_read_abilities() in there
> .get_features function to add in C45 features. However, it will
> continue using C45 over C22 because genphy_c45_pma_read_abilities()
> only really says the device has C45 registers, not C45 protocol.
> 
OK.
> I can see two different things you can try.
> 
> 1) set .read_mmd and .write_mmd in the PHY driver to phy_read_mmd()
>     and phy_write_mmd(). That is not great however, since each vendor
>     is likely to have their own PHY driver.
> 
I tried this approach and it works as expected. Means whenever there is 
a c45 register access, it directly uses the 
oa_tc6_read_c45()/oa_tc6_write_c45() functions. Herewith I have attached 
the patch 
(v4-0006-net-ethernet-oa_tc6-implement-internal-PHY-initia.patch) which 
has this new implementation for your reference. Is this you expected? 
Can you comment on this?
> 2) Add a helper to set is_c45. This however has side effects you might
>     not want. e.g. auto-neg will be performed via C45, not C22. This
>     might not matter for a T1 PHY where is think auto-neg is not
>     actually used. But i don't see anything in TC6 which limits it to
>     T1, i could well imagine a T2 or T4 PHY being used with full
>     auto-neg.
> 
I tried this approach by setting up is_c45 flag when I use 
phy_read_mmd() function but ended up with the kernel call trace 
(c45_kernel_call_trace.png) attached here for your reference.
> We really do need to separate C45 registers from C45 protocol in the
> phylib core to cleanly solve this.
OK. In my opinion, until having the above implementation in the phylib 
core, shall we go with the first approach [1] I tried out as it is 
fulfilling our requirement? What do you think?

Best regards,
Parthiban V
> 
>         Andrew
>