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