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

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

[Michal Vokáč]

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>;
		};
	};


Thanks in advance,
Michal

[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=87489ec3a77f3e01bcf0d46e353ae7112ec8c4f0
[2] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/net/dsa/qca8k.c?id=5ecdd77c61c8fe1d75ded538701e5e854963c890

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

[Andrew Lunn]

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.

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.

    Andrew

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

[Vinod Koul]

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

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

Please do test

Thanks
-- 
~Vinod

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

[Michal Vokáč]

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

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

[Vinod Koul]

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

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

Sending it shortly and ccing you as well

Thanks
-- 
~Vinod

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

[Michal Vokáč]

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