Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a5f3e0f9-6574-1e41-fd4d-7faa3a3fbd3d@intel.com>
Date:   Tue, 11 Apr 2023 08:44:37 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.9.1
Subject: Re: [PATCH net-next] net: dsa: mt7530: fix support for MT7531BE
Content-Language: en-US
To:     Daniel Golle <daniel@makrotopia.org>, <netdev@vger.kernel.org>,
        <linux-mediatek@lists.infradead.org>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-kernel@vger.kernel.org>, Sean Wang <sean.wang@mediatek.com>,
        "Landen Chao" <Landen.Chao@mediatek.com>,
        DENG Qingfang <dqfext@gmail.com>,
        =?UTF-8?B?QXLEsW7DpyDDnE5BTA==?= <arinc.unal@arinc9.com>,
        Russell King <linux@armlinux.org.uk>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        Paolo Abeni <pabeni@redhat.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Eric Dumazet <edumazet@google.com>,
        "David S. Miller" <davem@davemloft.net>,
        Vladimir Oltean <olteanv@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Andrew Lunn <andrew@lunn.ch>
References: <ZDSlm-0gyyDZXy_k@makrotopia.org>
From:   Jesse Brandeburg <jesse.brandeburg@intel.com>
In-Reply-To: <ZDSlm-0gyyDZXy_k@makrotopia.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SEtYUG9XM0FVZmJZcmtSZTg5em92akRjN2RXcTNCbjlueTd3dkRyaU9ZQTIw?=
 =?utf-8?B?WG4zSmJzVjVWQkx6VzB6MlNSWXVpQ0piOFZrYXZUSlVRZ0dNb3QzVFRISjRS?=
 =?utf-8?B?Mm1QeVAwQnRvWDJLZ09PandMTllXWkJSQUl3enA1N3ppYlBpRnRsMkxXakVk?=
 =?utf-8?B?bVp2bm8wM0hlMnk3RjVVTTJuNVNoQjZTaWlrYTB6ZmQxalZyMm8vc2JscWMr?=
 =?utf-8?B?NjBSQUZRWi9iZE1HZkxvb1FreHJXZmJqSXpUcy9iVnlVWnlIZjFtTEVtOVpj?=
 =?utf-8?B?Qi9DVkJVOFk2RFBDTzdZQ2Npc29ib1RIdHQ4bXdrNjJzYTBSaTFPVVIrdm1X?=
 =?utf-8?B?ZGptVnAreFJRQzZwcEZRd1VaZ2FXS0JXcGRMVVpUZFNDZ2p0dmx5M1BSR1lm?=
 =?utf-8?B?ZGhyWVVpMzFDTmNXbWZ5eFRiY2p1Q21xVHdMeVFML3VHTU14SXlzWFFDbCs2?=
 =?utf-8?B?ZzU0dUk4YXhjT3RET2pTUk1TMW93cWRMVVBLbHA2T2pEMGxBNktMUXdoSzNu?=
 =?utf-8?B?alA3cnhkNmo3OUV3cjI0eUx0US81d2tSaDJ2WVJGeWsrZzdSUGpibi9RdGRL?=
 =?utf-8?B?Ky9DalNLQTVHK3NMVEJKenhSRlgzZVk4c2pUSGhaaGNuNWt4QXg2OTNqZWVs?=
 =?utf-8?B?WHdXZUFDWWx6bjYyTG01WkRjNDN6T2cwQ1NzQVp3T2tLMFdtR1ZjOHRxMTcv?=
 =?utf-8?B?c0VtMmE1SHRZZXRwY1ZwUmJ6cmhzbVM0b1dCL2dXb0pGZ25idXJwbTVUU3pl?=
 =?utf-8?B?RG5uRjBSU0lQZnBFUno5STlDNTNLQ2tFT2FtT0FoVnVRVW1oa3FvR3MrOUFP?=
 =?utf-8?B?bGN6TnNCQ2xHQzdMaDhxbEowak5rYXFsMitraWg0R2QyODE5RHhJT2RYYjJU?=
 =?utf-8?B?OWM4aGpiMDJxUS9YRFpFREg1b0tPWS9IMUc0cmxzMVJJT2UrQTVwZjl5ZEFH?=
 =?utf-8?B?U2s4aElMcTMrMXZ6cCtoMWNNM3lZRlpTSnA0clJuMytrdk9NVUxvYTQzWi92?=
 =?utf-8?B?eEhzSFNnVG5uQ0lMZ0RJZFp6SG9UR0JDeFJ4bDIvRHBESGJxblE2WXlFaURl?=
 =?utf-8?B?alNNK0Q3UjdISnNEYVV6ZXBtWTcva0w3SjU1MVB3SXVac2FOcExXZ1c2RHVo?=
 =?utf-8?B?SjRoa3EvUFJSUUZzL0tDdVdDU2llN2tDVEJ5TWZsS1VjRjJLZjZxNVdKZUZC?=
 =?utf-8?B?cDFQc01FdW41a0FpS2QyaHpXT3h3dU5MdHNrOEc0OXlUcEI3am1SdUtUaHBL?=
 =?utf-8?B?WWdHWGZmMHdLOG9XbEFkelY0ZDVuMDZxaUtieCswM2ZEWXUzU3hIckV1YWdX?=
 =?utf-8?B?NWVRMWl5ZW51cFZReDBxeUNlSG9hMk1KN2Z2RmlqRlY2TkgraTk5cXF3UWF0?=
 =?utf-8?B?Y3FsZi9RUGhGYkZKNjBoaHhOY2dEY0JRaFNJTFduRXJ1Q1FCalRPSmRBbDBF?=
 =?utf-8?B?Q1E1dDBFcUNmSlZmclI5Y0lBTEprYWVSY3kreUdmY2ZjQTFVbk1UNHJVdnB2?=
 =?utf-8?B?eGtVcVdpaCtudEVFL0U1Rm9rSmluejh2aEplamRBamtOTHZLYmcyOFFJbGdY?=
 =?utf-8?B?c2o5UUdiL3NTNnNxY28vcXB3Ziszc0w1NTdnd082YWovUE1Qa0J5dFJkR2Nh?=
 =?utf-8?B?Ny93Um5mVm8wbVY3K2p0TGo0UmFGS1hQUzNnbHBZTEx6UWdnK0x2akcyaVFo?=
 =?utf-8?B?QitaWjdkVDhEa2VUTUJNNlgrUzBnZEhidERVMG8va3EvaEtyWFN2SGtzaVYz?=
 =?utf-8?B?Qzl2UDVmelJFUUFaZjBJWjhIUThtZ0IxaUN3RWE2Z0xPUFVZVWh4eFhmK2Fw?=
 =?utf-8?B?T1pVY29EODl2aGxwQlVOVlMrVWZKZUxHd3RLRzdjK2dqanAvWUFjTU53WmYv?=
 =?utf-8?B?cFRDQjlDK1RFdVhIeStQQ0RobENMdE13QTg3M2lkVWljblVRVjBaQ0tLZnB5?=
 =?utf-8?B?eUdIWGhqVmNmaXZRNFZEa1UwZzRlakZMMndCd0phQ1BDMU9pa1FxTG84bzdy?=
 =?utf-8?B?M2x0VjlYSGRqZXBRZ2Y4UWRjTm9EWEhjMDc0UzZQdUFrVWJsdGRHVVJnUDR0?=
 =?utf-8?B?YXJ5TEJadThDaXRMNi9EdVdmN0RobzdkWkhmSjRXTkdqZU4ycU9NYnpDNENo?=
 =?utf-8?B?SkNaMC9XRjZCbzh4OWtVaGs2ZGRhUFI2UnR1SzFRbXg0SGwxU0loRmhGUmFO?=
 =?utf-8?B?UUE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: c7c0a3a9-43ff-4ce4-2e6c-08db3aa3a971
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB4914.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Apr 2023 15:44:40.2621
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: HsRu0bhrERVTuPUn62muvsCjfZvl358LYFulA+eoZVutRBtn2SXUM6hdrn+xfQTTfXMQa/e42NkvGO9UwfUf4fWDeSJ2qgVgjEhoCfhAXiY=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5152
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.7 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_MED,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE
        autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 4/10/2023 5:11 PM, Daniel Golle wrote:
> There are two variants of the MT7531 switch IC which got different
> features (and pins) regarding port 5:
>  * MT7531AE: SGMII/1000Base-X/2500Base-X SerDes
>  * MT7531BE: RGMII
> 
> Moving the creation of the SerDes PCS from mt753x_setup to mt7530_probe
> with commit 6de285229773 ("net: dsa: mt7530: move SGMII PCS creation to
> mt7530_probe function") works fine for MT7531AE which got two instances
> of mtk-pcs-lynxi, however, MT7531BE requires mt7531_pll_setup to setup
> clocks before the single PCS on port 6 (usually used as CPU port)
> starts to work and hence the PCS creation failed on MT7531BE.
> 
> Fix this by introducing a pointer to mt7531_create_sgmii function in
> struct mt7530_priv and call it again at the end of mt753x_setup like it
> was before commit 6de285229773 ("net: dsa: mt7530: move SGMII PCS
> creation to mt7530_probe function").
> 
> Fixes: 6de285229773 ("net: dsa: mt7530: move SGMII PCS creation to mt7530_probe function")
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>  drivers/net/dsa/mt7530-mdio.c | 14 +++++++-------
>  drivers/net/dsa/mt7530.c      |  6 ++++++
>  drivers/net/dsa/mt7530.h      |  4 ++++
>  3 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/dsa/mt7530-mdio.c b/drivers/net/dsa/mt7530-mdio.c
> index 34a547b88e497..f17eab67d15fa 100644
> --- a/drivers/net/dsa/mt7530-mdio.c
> +++ b/drivers/net/dsa/mt7530-mdio.c
> @@ -81,14 +81,17 @@ static const struct regmap_bus mt7530_regmap_bus = {
>  };
>  
>  static int
> -mt7531_create_sgmii(struct mt7530_priv *priv)
> +mt7531_create_sgmii(struct mt7530_priv *priv, bool dual_sgmii)
>  {
>  	struct regmap_config *mt7531_pcs_config[2];

[1] use = {};

>  	struct phylink_pcs *pcs;
>  	struct regmap *regmap;
>  	int i, ret = 0;
>  
> -	for (i = 0; i < 2; i++) {
> +	/* MT7531AE has two SGMII units for port 5 and port 6
> +	 * MT7531BE has only one SGMII unit for port 6
> +	 */
> +	for (i = dual_sgmii ? 0 : 1; i < 2; i++) {
>  		mt7531_pcs_config[i] = devm_kzalloc(priv->dev,

hm, I don't like this, you're no longer initializing array[0] if
dual_sgmii is set. This seems like a recipe for disaster.

you could initialize the array to NULL above when allocating this on the
stack? [1]


>  						    sizeof(struct regmap_config),
>  						    GFP_KERNEL);
> @@ -208,11 +211,8 @@ mt7530_probe(struct mdio_device *mdiodev)
>  	if (IS_ERR(priv->regmap))
>  		return PTR_ERR(priv->regmap);
>  
> -	if (priv->id == ID_MT7531) {
> -		ret = mt7531_create_sgmii(priv);
> -		if (ret)
> -			return ret;
> -	}
> +	if (priv->id == ID_MT7531)
> +		priv->create_sgmii = mt7531_create_sgmii;
>  
>  	return dsa_register_switch(priv->ds);
>  }
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index e4bb5037d3525..c680873819b01 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -3018,6 +3018,12 @@ mt753x_setup(struct dsa_switch *ds)
>  	if (ret && priv->irq)
>  		mt7530_free_irq_common(priv);
>  
> +	if (priv->create_sgmii) {
> +		ret = priv->create_sgmii(priv, mt7531_dual_sgmii_supported(priv));
> +		if (ret && priv->irq)
> +			mt7530_free_irq(priv);
> +	}
> +
>  	return ret;
>  }
>  
> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> index 01db5c9724fa8..6e89578b4de02 100644
> --- a/drivers/net/dsa/mt7530.h
> +++ b/drivers/net/dsa/mt7530.h
> @@ -752,6 +752,8 @@ struct mt753x_info {
>   * @irq:		IRQ number of the switch
>   * @irq_domain:		IRQ domain of the switch irq_chip
>   * @irq_enable:		IRQ enable bits, synced to SYS_INT_EN
> + *

no extra blank line needed here.

> + * @create_sgmii:	Pointer to function creating SGMII PCS instance(s)
>   */
>  struct mt7530_priv {
>  	struct device		*dev;
> @@ -778,6 +780,8 @@ struct mt7530_priv {
>  	int irq;
>  	struct irq_domain *irq_domain;
>  	u32 irq_enable;
> +
> +	int (*create_sgmii)(struct mt7530_priv *priv, bool dual_sgmii);
>  };
>  
>  struct mt7530_hw_vlan_entry {