Re: [RFC] net: dsa: qca8k: implement rgmii-id mode

["Michal Vokáč" <michal.vokac@ysoft.com>]

On 18. 02. 19 11:45, Vinod Koul wrote:
> On 15-02-19, 16:23, Andrew Lunn wrote:
>> On Fri, Feb 15, 2019 at 04:01:08PM +0100, Michal Vokáč wrote:
>>> Hi,
>>>
>>> networking on my boards [1], which are currently in linux-next, suddently
>>> stopped working. I tracked it down to this commit 5ecdd77c61c8 ("net: dsa:
>>> qca8k: disable delay for RGMII mode") [2].
>>>
>>> So I think the rgmii-id mode is obviously needed in my case.
>>> I was able to find a couple drivers that read tx/rx-delay or
>>> tx/rx-internal-delay from device tree. Namely:
>>>
>>>    drivers/net/ethernet/apm/xgene/xgene_enet_main.c
>>>    drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
>>>    drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
>>>    drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
>>>    drivers/net/phy/dp83867.c
>>>
>>> I would appreciate any hints how to add similar function to qca8k driver
>>> if that is the correct way to go. Can I take some of the above mentioned
>>> drivers as a good example for that? How should the binding look like?
>>>
>>> I would expect something like this:
>>>
>>> 	switch@0 {
>>> 		compatible = "qca,qca8334";
>>> 		reg = <0>;
>>>
>>> 		switch_ports: ports {
>>> 			#address-cells = <1>;
>>> 			#size-cells = <0>;
>>>
>>> 			ethphy0: port@0 {
>>> 				reg = <0>;
>>> 				label = "cpu";
>>> 				phy-mode = "rgmii-id";
>>> 				qca,tx-delay = <3>;
>>> 				qca,rx-delay = <3>;
>>> 				ethernet = <&fec>;
>>> 		};
>>
>> Hi Michal
>>
>> Your submission used:
>>
>> +				ethphy0: port@0 {
>> +					reg = <0>;
>> +					label = "cpu";
>> +					phy-mode = "rgmii";
>> +					ethernet = <&fec>;
>> +
>> +					fixed-link {
>> +						speed = <1000>;
>> +						full-duplex;
>> +					};
>> +				};
>>
>> This is good. If you have a fixed-link you can pass a phy-mode.

Yes, I am using fixed-link and the plan was to implement the rgmii-id
mode.

>>
>> The comment that was removed was:
>>
>> -               /* According to the datasheet, RGMII delay is enabled through
>> -                * PORT5_PAD_CTRL for all ports, rather than individual port
>> -                * registers
>> -                */
>>
>> Is it possible to enable delays per port? Ideally, you want to enable
>> delays for just selected ports. Add another case for
>> PHY_INTERFACE_MODE_RGMII_ID to enable the delays.

I am still trying to collect all the relevant notes and bits from the
horrible docs to understand how this is done. The problem is different
parts and different versions of the documentation provide diffrerent
details.

For example the QCA8334 model does not have PORT5 and so that registers
are not documented. Though some application note document says:

   """
   The MAC0 timing control is in the Port0 PAD Mode Control Register (offset 0x0004):

   1. RGMII timing delay for the output path of QCA8337(N) is enabled by 0x8[24].
      Set 1 to add 2ns delay in 1000 mode for all RGMII interfaces.

   2. Bit [21:20]: select the delay time for the output path in 10/100 mode.

   3. Bit 25: enable the timing delay for the input path of QCA8337(N) in 1000 mode.

   4. Bit [23:22]: select the delay time for the input path in 1000 mode.
     00: 0.2ns
     01: 1.2ns
     10: 2.1ns
     11: 3.1ns
   """

That is in line with the removed comment. The bit 24 at address 0x8 is
used to enable/disable delays globally. And obviously it is needed on the
QCA8334 model as well even though it should not have the PORT5.

> In the hindsight I should not have removed the comment, let me ressurect
> that as well as add handling of the RGMII modes...

OK, thanks.

> Please do test

Sure, will do. Just let me know when you have something ready.
Thank you!

Michal