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