Re: net: dsa: mv88e6xxx: error parsing ethernet node from dts

["Jürgen Lambrecht" <j.lambrecht@televic.com>]

On 12/24/19 2:57 PM, Jürgen Lambrecht wrote:
> CAUTION: This Email originated from outside Televic. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
>
> On 12/24/19 12:19 PM, Andrew Lunn wrote:
>> CAUTION: This Email originated from outside Televic. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>>
>>
>> On Tue, Dec 24, 2019 at 11:28:27AM +0100, Jürgen Lambrecht wrote:
>>> On 12/4/19 6:13 PM, Andrew Lunn wrote:
>>>> But returning 0x0000 is odd. Normally, if an MDIO device does not
>>>> respond, you read 0xffff, because of the pull up resistor on the bus.
>>>>
>>>> The fact you find it ones means it is something like this, some minor
>>>> configuration problem, power management, etc.
>>> Hi Adrew,
>>>
>>> to close this issue: you were right: the Marvell clock, that comes from the iMX clocking block ENET1_REF_CLK_25M suddenly stops running:
>>>
>>> an oscilloscope showed that the Marvell main clock stops shortly after the first probe, and only comes back 5s later at the end of booting when the fixed-phy is configured.
>>> It is not the fec that stops the clock, because if fec1 is "disabled" also the clock stops, but then does not come back.
>>>
>>> We did not found yet how to keep the clock enabled (independent of the fec), so if you have any hints - more than welcome.
>> Let me make sure i understand your design correct.
>>
>> I think you are saying your switch does not have a crystal connected
>> to XTAL_OUT/XTAL_IN. Instead you want the iMX to generate a 25MHz
>> clock, which you are feeding to the switch?
> indeed.
>> All the designs i've used have the crystal connected to the
>> switch. The FEC clock line is used as an input, either driven from a
>> PHY connected to the other FEC port, or the clock output from the
>> switch.
>>
>> So for your design, you need to ensure the 25MHz clock output is
>> always ticking. Looking at the FEC driver, you see the optional clock
>> fep->clk_enet_out. This clock is enabled when the FEC interface is
>> opened, i.e. configured up. It is disabled when the FEC is closed. It
>> is enabled during probe, but turned off at the end of the probe to
>> save power. The FEC also has runtime suspend/resume support. This
>> actually does not touch the clk_enet_out, but it does enable/disable
>> clocks needed for MDIO to work. I had to fix this many years ago.

You mean you added then the code that touches clk_enet_out: 'ret = clk_prepare_enable(fep->clk_enet_out);' and 'clk_disable_unprepare(fep->clk_enet_out);' ?

For the PHY, this can be the PHY chip clock, so it must be running for MDIO to work.
(and for us it is the switch clock)

>>
>> It appears this clock is just a plain SOC clock.
>>
>> In imx7d.dtsi we see:
>>
>>                 clocks = <&clks IMX7D_ENET2_IPG_ROOT_CLK>,
>>                         <&clks IMX7D_ENET_AXI_ROOT_CLK>,
>>                         <&clks IMX7D_ENET2_TIME_ROOT_CLK>,
>>                         <&clks IMX7D_PLL_ENET_MAIN_125M_CLK>,
>>                         <&clks IMX7D_ENET_PHY_REF_ROOT_CLK>;
>>                 clock-names = "ipg", "ahb", "ptp",
>>                         "enet_clk_ref", "enet_out";
>>
>> The mapping between clock-names and clocks seem a bit odd. But there
>> is some room for error here, since the FEC driver mostly just enables
>> them all or disables them all. But you need one specific clock.
>>
>> What i suggest you do is add clock support to DSA. Allow DSA to look
>> up a clock in DT, and call clk_prepare_enable() and
>> clk_disable_unprepare(). The clock framework uses reference
>> counting. So if DSA turns a clock on, it does not matter what the FEC
>> does, it will stay on. It will only go off when all users of the clock
>> turn it off.
>>
>> I'm not sure if this can be done in a generic way for all DSA drivers,
>> or if you need to add it to the mv88e6xxx driver. The DSA core only
>> gets involved once the probe of the switch is over. And you probably
>> need the clock reliably ticking during probe. So maybe it needs to be
>> in the mv88e6xxx driver.

We could do that, but actually, we prefer that the master clock of the switch chip is running always, independently of any kernel code.

So we would like to add support to be able to specify in DTS a clock output that should be always running. Because on our board that Ethernet clock output of the iMX6 is used to clock the switch, but it could also be used to clock any other chip that needs 25MHz.
We already tested such a driver and it works fine now.
What do you think?

I would then send a patch proposal to the arm mailing list.

(FYI: it has no use to remove that clock from the fec1 entry in imx6ul.dtsi, because then (after a patch) the clock is not touched anymore, but the power-management puts the clocking block in power-down when MDIO finishes and its clock is turned off, but the same block generates several clocks, so it is not possible to ignore the kernel because the bootloader already enables the clock.)

Kind regards,

Jürgen