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

From: Michal VokÃÄ
Date: Mon Feb 18 2019 - 08:31:48 EST


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