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

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

On 3/23/23 13:10, Paul Geurts wrote:
>> -----Original Message-----
>> From: Florian Fainelli <f.fainelli@gmail.com>
>> Sent: donderdag 23 maart 2023 17:43
>> To: Álvaro Fernández Rojas <noltari@gmail.com>; Paul Geurts
>> <paul.geurts@prodrive-technologies.com>; jonas.gorski@gmail.com;
>> andrew@lunn.ch; olteanv@gmail.com; davem@davemloft.net;
>> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
>> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
>> netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-
>> kernel@vger.kernel.org
>> Subject: Re: [PATCH 2/2] net: dsa: b53: mdio: add support for BCM53134
>>
>> 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:
>>
>> diff --git a/drivers/net/dsa/b53/b53_common.c
>> b/drivers/net/dsa/b53/b53_common.c
>> index 59cdfc51ce06..1c64b6ce7e78 100644
>> --- a/drivers/net/dsa/b53/b53_common.c
>> +++ b/drivers/net/dsa/b53/b53_common.c
>> @@ -1235,7 +1235,7 @@ static void b53_adjust_link(struct dsa_switch *ds,
>> int port,
>>                                 tx_pause, rx_pause);
>>           b53_force_link(dev, port, phydev->link);
>>
>> -       if (is531x5(dev) && phy_interface_is_rgmii(phydev)) {
>> +       if ((is531x5(dev) || is53134(dev)) &&
>> phy_interface_is_rgmii(phydev)) {
>>                   if (port == dev->imp_port)
>>                           off = B53_RGMII_CTRL_IMP;
>>                   else
>>
>> Other than that, LGTM!
>> --
>> Florian
> 
> I think the only reason is that the BCM53134 does not support the
> RGMII_CTRL_TIMING_SEL bit, which is set in the original block. I agree
> Putting a if statement around
> rgmii_ctrl |= RGMII_CTRL_TIMING_SEL;
> would prevent a lot of code duplication. _however_, after looking at it again,
> I don’t think the device does not support the bit. When looking at the datasheet,
> The same bit in the this register is called BYPASS_2NS_DEL. It's very uncommon
> For Broadcom to make such a change in the register interface, so maybe they
> Just renamed it. Do you think this could be the same bit?

Yes, I think this is exactly the same bit, just named differently. What 
strikes me as odd is that neither of the 53115, 53125 or 53128 which are 
guarded by the is531x5() conditional have it defined.
-- 
Florian