Re: [PATCH 2/2] net: dsa: b53: mdio: add support for BCM53134

[Florian Fainelli <f.fainelli@gmail.com>]

On 3/23/23 09:49, Álvaro Fernández Rojas wrote:
> El jue, 23 mar 2023 a las 17:43, Florian Fainelli
> (<f.fainelli@gmail.com>) escribió:
>>
>> On 3/23/23 05:18, Álvaro Fernández Rojas wrote:
>>> From: Paul Geurts <paul.geurts@prodrive-technologies.com>
>>>
>>> Add support for the BCM53134 Ethernet switch in the existing b53 dsa driver.
>>> BCM53134 is very similar to the BCM58XX series.
>>>
>>> Signed-off-by: Paul Geurts <paul.geurts@prodrive-technologies.com>
>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>>> ---
>>>    drivers/net/dsa/b53/b53_common.c | 53 +++++++++++++++++++++++++++++++-
>>>    drivers/net/dsa/b53/b53_mdio.c   |  5 ++-
>>>    drivers/net/dsa/b53/b53_priv.h   |  9 +++++-
>>>    3 files changed, 64 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
>>> index 1f9b251a5452..aaa0813e6f59 100644
>>> --- a/drivers/net/dsa/b53/b53_common.c
>>> +++ b/drivers/net/dsa/b53/b53_common.c
>>> @@ -1282,6 +1282,42 @@ static void b53_adjust_link(struct dsa_switch *ds, int port,
>>>        if (is63xx(dev) && port >= B53_63XX_RGMII0)
>>>                b53_adjust_63xx_rgmii(ds, port, phydev->interface);
>>>
>>> +     if (is53134(dev) && phy_interface_is_rgmii(phydev)) {
>>
>> Why is not this in the same code block as the one for the is531x5()
>> device like this:
> 
> This is what I asked on the cover letter:
>> I also added a separate RGMII handling block for is53134() since according to
>> Paul, BCM53134 doesn't support RGMII_CTRL_TIMING_SEL as opposed to is531x5().
> 
> Should I add it to the same block and avoid adding
> RGMII_CTRL_TIMING_SEL if is53134() or is it compatible with
> RGMII_CTRL_TIMING_SEL?

RGMII_CTRL_TIMING_SEL is not defined for the 53125 or 53128, and for 
53115, the register 0x60 is not even defined in the datasheet. 
RGMII_CTRL_TIMING_SEL is defined for the 53134 however and would control 
how the two other bits behave with respect to RGMII timing. My guess is 
that if we removed it entirely we would not be breaking anything, and we 
could just support all of the 531x5() and 53134 families within the same 
block.

Jonas, do you remember why setting RGMII_CTRL_TIMING_SEL might have been 
necessary?
--
Florian