Re: [PATCH] net: macb: ignore tx_clk if MII is used

[Michael Walle <michael@walle.cc>]

Am 2021-01-22 12:38, schrieb Claudiu.Beznea@microchip.com:
> On 22.01.2021 13:20, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the
>> content is safe
>> 
>> Am 2021-01-22 10:10, schrieb Claudiu.Beznea@microchip.com:
>>> On 21.01.2021 11:41, Michael Walle wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you 
>>>> know
>>>> the
>>>> content is safe
>>>> 
>>>> Hi Claudiu,
>>>> 
>>>> Am 2021-01-21 10:19, schrieb Claudiu.Beznea@microchip.com:
>>>>> On 20.01.2021 21:43, Michael Walle wrote:
>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>>>> know
>>>>>> the content is safe
>>>>>> 
>>>>>> If the MII interface is used, the PHY is the clock master, thus
>>>>>> don't
>>>>>> set the clock rate. On Zynq-7000, this will prevent the following
>>>>>> warning:
>>>>>>   macb e000b000.ethernet eth0: unable to generate target 
>>>>>> frequency:
>>>>>> 25000000 Hz
>>>>>> 
>>>>> 
>>>>> Since in this case the PHY provides the TX clock and it provides 
>>>>> the
>>>>> proper
>>>>> rate based on link speed, the MACB driver should not handle the
>>>>> bp->tx_clk
>>>>> at all (MACB driver uses this clock only for setting the proper 
>>>>> rate
>>>>> on
>>>>> it
>>>>> based on link speed). So, I believe the proper fix would be to not
>>>>> pass
>>>>> the
>>>>> tx_clk at all in device tree. This clock is optional for MACB 
>>>>> driver.
>>>> 
>>>> Thanks for looking into this.
>>>> 
>>>> I had the same thought. But shouldn't the driver handle this case
>>>> gracefully?
>>>> I mean it does know that the clock isn't needed at all.
>>> 
>>> Currently it may knows that by checking the bp->tx_clk. Moreover the
>>> clock
>>> could be provided by PHY not only for MII interface.
>> 
>> That doesn't make this patch wrong, does it? It just doesn't cover
>> all use cases (which also wasn't covered before).
> 
> I would say that it breaks setups using MII interface and with clock
> provided via DT that need to be handled by macb_set_tx_clk().

But the MII interface has by definition no tx clock? At the moment
tx_clk is set to the correct frequency for RGMII interfaces, right?
How would it break boards then?

And if you use tx_clk for like driving the PHY refclk input, that
would be an abuse of this clock (and shouldn't even work, because it
just sets the correct RGMII frequencies).

>>> Moreover the IP has the bit "refclk" of register at offset 0xc 
>>> (userio)
>>> that tells it to use the clock provided by PHY or to use one internal
>>> to
>>> the SoC. If a SoC generated clock would be used the IP logic may have
>>> the
>>> option to do the proper division based on link speed (if IP has this
>>> option
>>> enabled then this should be selected in driver with capability
>>> MACB_CAPS_CLK_HW_CHG).
>>> 
>>> If the clock provided by the PHY is the one to be used then this is
>>> selected with capability MACB_CAPS_USRIO_HAS_CLKEN. So, if the change
>>> you
>>> proposed in this patch is still imperative then checking for this
>>> capability would be the best as the clock could be provided by PHY 
>>> not
>>> only
>>> for MII interface.
>> 
>> Fair enough, but this register doesn't seem to be implemented on
>> Zynq-7000. Albeit MACB_CAPS_USRIO_DISABLED isn't defined for the
>> Zynq MACB. It isn't defined in the Zynq-7000 reference manual and
>> you cannot set any bits:
>> 
>> => mw 0xE000B00C 0xFFFFFFFF
>> => md 0xE000B00C 1
>> e000b00c: 00000000
> 
> I wasn't aware of this. In this case, maybe adding the
> MACB_CAPS_USRIO_DISABLED to the Zync-7000 capability list and checking 
> this
> one plus MACB_CAPS_USRIO_HAS_CLKEN would be better instead of checking 
> the
> MAC-PHY interface?

But then RGMII would be broken. Zynq and ZynqMP are the only users of
the tx_clk as far as I can see. There, it will set the clock generated
by the clock controller which is driving the TX_CLK for RGMII and the
corresponding input of the GEM/MACB.

I mean you have the same SoC, thus the same caps and usrio settings,
but you have two possible interfaces: MII and RGMII. (At least, because
there might also be other interfaces like GMII). Therefore, you cannot
check this bit, right? It will be the same for both MII and RGMII mode.

In any case, it should be correct to add MACB_CAPS_USRIO_DISABLED to
caps for the zynq family (I'd need to double check the ZynqMP RM).

>> Also please note, that tx_clk may be an arbitrary clock which doesn't
>> necessarily need to be the clock which is controlled by CLK_EN. Or
>> am I missing something here?
> 
> I suppose that whoever creates the device tree knows what is doing and 
> it
> passes the proper clock to macb driver.

Mh. If CLK_EN is supported this might be the case. Unfortunalty, Zynq
is the only user and doesn't have this bit.

-michael

PS. dont' get me wrong, I'm all for fixing this "the right way".