Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sun, 10 Oct 2021 20:18:53 +0200
From:   Ansuel Smith <ansuelsmth@gmail.com>
To:     Vladimir Oltean <olteanv@gmail.com>
Cc:     Andrew Lunn <andrew@lunn.ch>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Russell King <linux@armlinux.org.uk>, netdev@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [net-next PATCH v4 06/13] net: dsa: qca8k: move rgmii delay
 detection to phylink mac_config
Message-ID: <YWMujVh232s+Q7dU@Ansuel-xps.localdomain>
References: <20211010111556.30447-1-ansuelsmth@gmail.com>
 <20211010111556.30447-7-ansuelsmth@gmail.com>
 <20211010124732.fageoraoweqqfoew@skbuf>
 <YWLqh2X0lVwiDMCn@Ansuel-xps.localdomain>
 <20211010181107.4as42prroyitew3m@skbuf>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211010181107.4as42prroyitew3m@skbuf>
Precedence: bulk

On Sun, Oct 10, 2021 at 09:11:07PM +0300, Vladimir Oltean wrote:
> On Sun, Oct 10, 2021 at 03:28:39PM +0200, Ansuel Smith wrote:
> > > I was actually going to say that since RGMII delays are runtime
> > > invariants, you should move their entire programming to probe time, now
> > > you move device tree parsing to runtime :-/
> > >
> >
> > The main idea here was to move everything to mac config and scan the DT
> > node of the current port that is being configured.
> 
> If you insist on doing static configuration in a phylink callback, sure,
> the comment was mostly about not accessing directly this struct dsa_port
> member. It might change in the future, and the less refactoring required,
> the better.
>

I think I will put values in qca8k_priv and configure them in mac_config.

> > > > -{
> > > > -	struct device_node *port_dn;
> > > > -	phy_interface_t mode;
> > > > -	struct dsa_port *dp;
> > > > -	u32 val;
> > > > -
> > > > -	/* CPU port is already checked */
> > > > -	dp = dsa_to_port(priv->ds, 0);
> > > > -
> > > > -	port_dn = dp->dn;
> > > > -
> > > > -	/* Check if port 0 is set to the correct type */
> > > > -	of_get_phy_mode(port_dn, &mode);
> > > > -	if (mode != PHY_INTERFACE_MODE_RGMII_ID &&
> > > > -	    mode != PHY_INTERFACE_MODE_RGMII_RXID &&
> > > > -	    mode != PHY_INTERFACE_MODE_RGMII_TXID) {
> > > > -		return 0;
> > > > -	}
> > > > -
> > > > -	switch (mode) {
> > > > -	case PHY_INTERFACE_MODE_RGMII_ID:
> > > > -	case PHY_INTERFACE_MODE_RGMII_RXID:
> > >
> > > Also, since you touch this area.
> > > There have been tons of discussions on this topic, but I believe that
> > > your interpretation of the RGMII delays is wrong.
> > > Basically a MAC should not apply delays based on the phy-mode string (so
> > > it should treat "rgmii" same as "rgmii-id"), but based on the value of
> > > "rx-internal-delay-ps" and "tx-internal-delay-ps".
> > > The phy-mode is for a PHY to use.
> > >
> >
> > Ok so we can just drop the case and directly check for the
> > internal-delay-ps presence?
> 
> Yes, but please consider existing device trees for this driver. I see
> qcom-ipq8064-rb3011.dts and imx6dl-yapp4-common.dtsi, and neither use
> explicit rx-internal-delay-ps or tx-internal-delay-ps properties. So
> changing the driver to look at just those and ignore "rgmii-id" will
> break those device trees, which is not pleasant. What would work is to
> search first for *-internal-delay-ps, and then revert to determining the
> delays based on the phy-mode, for compatibility.

Ok. Will try to implement something that is not entireley a big complex
condition ahahah.

-- 
	Ansuel