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

[<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().

> 
>> 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?

> 
> 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.

Thank you,
Claudiu Beznea

> 
> -michael
> 
>>> Ususually that
>>> clock
>>> is defined in a device tree include. So you'd have to redefine that
>>> node
>>> in
>>> an actual board file which means duplicating the other clocks.
>>>
>>> -michael