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

[Vinod Koul <vkoul@kernel.org>]

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