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

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

On 18. 02. 19 14:03, Vinod Koul wrote:
> On 18-02-19, 12:54, Michal Vokáč wrote:
>> 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.
> 
> Since it used to work for you before, you need older code in RGMII_ID
> mode, I have added that along with the comment

Sure, that is OK for me. I just thought about implementing something more
generic with the option to set the delay value, even though I do not really
need it.

Thank you,
Michal