Date:   Fri, 17 Mar 2023 11:46:29 +0200
From:   Vladimir Oltean <vladimir.oltean@nxp.com>
To:     netdev@vger.kernel.org
Cc:     Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Woojung Huh <woojung.huh@microchip.com>,
        UNGLinuxDriver@microchip.com,
        Arun Ramadoss <arun.ramadoss@microchip.com>,
        Russell King <linux@armlinux.org.uk>,
        Marek Vasut <marex@denx.de>,
        Oleksij Rempel <linux@rempel-privat.de>,
        linux-kernel@vger.kernel.org
Subject: Re: [RFC/RFT PATCH net-next 2/4] net: dsa: microchip: partial
 conversion to regfields API for KSZ8795 (WIP)
Message-ID: <20230317094629.nryf6qkuxp4nisul@skbuf>
References: <20230316161250.3286055-1-vladimir.oltean@nxp.com>
 <20230316161250.3286055-3-vladimir.oltean@nxp.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230316161250.3286055-3-vladimir.oltean@nxp.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?j5HnXOD+VA6ksI4mWcQy1Ap0fpERM/KmlxhTogk0lFaN4IOZ5a8ffm89aHDg?=
 =?us-ascii?Q?DKUsvVvSBNbeVZkbytbbHRBscw+BFdmCNSsrqNcHwH5zXy9wlNnkMF4+7bs7?=
 =?us-ascii?Q?euyPs5sJl6QOKQ7+M2F9Ou3jNCK4kse6zKPuYVlKIj+LXq/3IQUJHNnPxOIj?=
 =?us-ascii?Q?mYrNuHiMOkAXWShXo6tszS8o8idEoFbtowCiueB5YtfyDPP4LQnKlsRNyHbA?=
 =?us-ascii?Q?noOWi+8zmZAxEwX6gZjue0SxE3XOA755aVhjxNlXX5idTK2NDvcoJ1wXGZRG?=
 =?us-ascii?Q?PsMzKuWpvxw5uwRrfRi9xlkRbBIr6rbep9CGUIMT41ecD/VB5ECM3WcjwSjM?=
 =?us-ascii?Q?Vxo+rlUjLkIcWIxm1ml5jyAZSPDx7CjYEHmHsOOXsNHds4a4m2BHtRpmNY2m?=
 =?us-ascii?Q?K5qXiqHxWazXby8KhV2LoD3EnU9gxhwqNqopn0NCVYjga1cKdXUFzL8x7C4R?=
 =?us-ascii?Q?mSWCSNGTjEtC9PRyji8UzMunyjaUfNf/lhUMJjakU0hdX70l1jtMbypNc52F?=
 =?us-ascii?Q?jsrOJCLp9ebSaCyrHt2QaxA0Vqj8xayk4TfdPfrV1z1gYlOdQ77zJj0+4Esn?=
 =?us-ascii?Q?Vwxh0itOn/9hFJ231Bj3in0SqzJeXjDu9y02WeoFx8HOwJcHwzrATuHmOB7t?=
 =?us-ascii?Q?6S28jsDSpkG19k01Sj68s7tWJlTSKVXs4Y0GYhJxqOgRuTZUCTAxRwIWZb6F?=
 =?us-ascii?Q?uxXsDUQFRofDev/gXGKr4Gf12gsM83X02QfhAIGbK4pR1+Z6CJVA/a2fgDqH?=
 =?us-ascii?Q?1mYnzZsYVMgRYkZDyLYYVL6aIIundH69mw6g5T0IFn1b2U5IxV9kpgg3XDoB?=
 =?us-ascii?Q?oBxABCxMzOnQjcFPWyTW5MdhymcHXpPqidpTHm2kCk6aJ8UVVZLJjm862Gjv?=
 =?us-ascii?Q?HHlvmIrsElWEiVM/gMhSv8/2UcbFNXpln17gkWakdfxIBhFfm2M8t1ZM3AO6?=
 =?us-ascii?Q?vdiBWa4Q31f3rt74UprBUgt67Qn9YH8Z5Kwo4nf90BsezANiko8KraPyJVMx?=
 =?us-ascii?Q?CroNJdGyG8cOywswmQPK3912jbE1q4aTjBfp6wIIL1cSeyQoCCypHfyB6b3I?=
 =?us-ascii?Q?zRwd0vWkt6bICBJ+dfPp+txyttqRTQhBd4fFRk4GgEQ+mchgbQTT0z7LtZ3q?=
 =?us-ascii?Q?hLku+JI+iAeyK0yj+L/dZ7dcFDMFDn4sygNtCBVCcVS9iDALFDBGBdB5iNYw?=
 =?us-ascii?Q?5oJiS39zmqytMHOTIULLDiqbdvNx2KR09GFA4cpve/oJ1lA1to4ana08RZxV?=
 =?us-ascii?Q?p05CV16JwK3aXPonHlegzfGZtcKr+8sxpJ2Fn65ynIEj5XKfdTOwMuedRxij?=
 =?us-ascii?Q?A83GoHevhXaR5QrchToTSyT/xm2yIoHRP71iJNA2DeiaM6Gf+2WoKOvMz3gw?=
 =?us-ascii?Q?+SIQrpSOD/kD3byWrAnIob2ldmvAaN+VkRe5FZXF9T+oK7uT2xtpHoHioeJo?=
 =?us-ascii?Q?jw30SuYHRpFqDxWyZxNSHdBPNcEB33CPHqr/tbxJYlvI13LqRqpzh/PCjPOV?=
 =?us-ascii?Q?KQjEO1sSvbZ3Ho5V0hH9p97F40AugvDsL9jaRLm8Sj1TVnVVqBOvArkIB+et?=
 =?us-ascii?Q?WmTIkAaTKvriU+eepcObZZYVJzORsNmXxb+8tJj4DdoQGLgNWpj1Upvl+Uzw?=
 =?us-ascii?Q?zg=3D=3D?=
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-Network-Message-Id: dbbbe443-d792-406a-b1a5-08db26cc7df4
X-MS-Exchange-CrossTenant-AuthSource: DB8PR04MB6459.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Mar 2023 09:46:33.3772
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: eWQvw8GcXL1yCHNLt3y4e1PNLWgQ5ed/hMfLSa7WHk+Tm1HTzb8RqCM7xgDFE2ocHO6y10zsY4M/v0eJ5jihTQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PAWPR04MB9816
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Mar 16, 2023 at 06:12:48PM +0200, Vladimir Oltean wrote:
> During review, Russell King has pointed out that the current way in
> which the KSZ common driver performs register access is error-prone.
> 
> Based on the KSZ9477 software model where we have the XMII Port Control
> 0 Register (at offset 0xN300) and XMII Port Control 1 Register
> (at offset 0xN301), this driver also holds P_XMII_CTRL_0 and P_XMII_CTRL_1.
> 
> However, on some switches, fields accessed through P_XMII_CTRL_0 (for
> example P_MII_100MBIT_M or P_MII_DUPLEX_M) and fields accessed through
> P_XMII_CTRL_1 (for example P_MII_SEL_M) may end up accessing the same
> hardware register. Or at least, that was certainly the impression after
> commit
> https://patchwork.kernel.org/project/netdevbpf/patch/20230315231916.2998480-1-vladimir.oltean@nxp.com/
> (sorry, can't reference the sha1sum of an unmerged commit), because for
> ksz8795_regs[], P_XMII_CTRL_0 and P_XMII_CTRL_1 now have the same value.
> 
> But the reality is far more interesting. Opening a KSZ8795 datasheet, I
> see that out of the register fields accessed via P_XMII_CTRL_0:
> - what the driver names P_MII_SEL_M *is* actually "GMII/MII Mode Select"
>   (bit 2) of the Port 5 Interface Control 6, address 0x56 (all good here)
> - what the driver names P_MII_100MBIT_M is actually "Switch SW5-MII/RMII
>   Speed" (bit 4) of the Global Control 4 register, address 0x06.
> 
> That is a huge problem, because the driver cannot access this register
> for KSZ8795 in its current form, even if that register exists. This
> creates an even stronger motivation to try to do something to normalize
> the way in which this driver abstracts away register field movement from
> one switch family to another.
> 
> As I had proposed in that thread, reg_fields from regmap propose to
> solve exactly this problem. This patch contains a COMPLETELY UNTESTED
> rework of the driver, so that accesses done through the following
> registers (for demonstration purposes):
> - REG_IND_BYTE - a global register
> - REG_IND_CTRL_0 - another global register
> - P_LOCAL_CTRL - a port register
> - P_FORCE_CTRL - another port register
> - P_XMII_CTRL_0 and P_XMII_CTRL_1 - either port register, or global
>   registers, depending on which manual you read!
> 
> are converted to the regfields API.
> 
> !! WARNING !! I only attempted to add a ksz_reg_fields structure for
> KSZ8795. The other switch families will currently crash!
> 
> For easier partial migration, I have renamed the "REG_" or "P_" prefixes
> of the enum ksz_regs values into a common "RF_" (for reg field) prefix
> for a new enum type: ksz_rf. Eventually, enum ksz_regs, as well as the
> masks, should disappear completely, being replaced by reg fields.
> 
> Link: https://lore.kernel.org/netdev/Y%2FYPfxg8Ackb8zmW@shell.armlinux.org.uk/
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> diff --git a/drivers/net/dsa/microchip/ksz8863_smi.c b/drivers/net/dsa/microchip/ksz8863_smi.c
> index 5871f27451cb..f9d22a444146 100644
> --- a/drivers/net/dsa/microchip/ksz8863_smi.c
> +++ b/drivers/net/dsa/microchip/ksz8863_smi.c
> @@ -136,11 +136,16 @@ static const struct regmap_config ksz8863_regmap_config[] = {
>  
>  static int ksz8863_smi_probe(struct mdio_device *mdiodev)
>  {
> +	const struct ksz_chip_data *chip;
>  	struct regmap_config rc;
>  	struct ksz_device *dev;
>  	int ret;
>  	int i;
>  
> +	chip = device_get_match_data(ddev);

s/ddev/&mdiodev->dev/

> +	if (!chip)
> +		return -EINVAL;
> +
>  	dev = ksz_switch_alloc(&mdiodev->dev, mdiodev);
>  	if (!dev)
>  		return -ENOMEM;
> @@ -159,6 +164,10 @@ static int ksz8863_smi_probe(struct mdio_device *mdiodev)
>  		}
>  	}
>  
> +	ret = ksz_regfields_init(dev, chip);
> +	if (ret)
> +		return ret;
> +
>  	if (mdiodev->dev.platform_data)
>  		dev->pdata = mdiodev->dev.platform_data;
>  
> diff --git a/drivers/net/dsa/microchip/ksz9477_i2c.c b/drivers/net/dsa/microchip/ksz9477_i2c.c
> index 497be833f707..2cbd76aed974 100644
> --- a/drivers/net/dsa/microchip/ksz9477_i2c.c
> +++ b/drivers/net/dsa/microchip/ksz9477_i2c.c
> @@ -16,10 +16,15 @@ KSZ_REGMAP_TABLE(ksz9477, not_used, 16, 0, 0);
>  
>  static int ksz9477_i2c_probe(struct i2c_client *i2c)
>  {
> +	const struct ksz_chip_data *chip;
>  	struct regmap_config rc;
>  	struct ksz_device *dev;
>  	int i, ret;
>  
> +	chip = device_get_match_data(ddev);

s/ddev/&i2c->dev/

> +	if (!chip)
> +		return -EINVAL;
> +
>  	dev = ksz_switch_alloc(&i2c->dev, i2c);
>  	if (!dev)
>  		return -ENOMEM;
> @@ -35,6 +40,10 @@ static int ksz9477_i2c_probe(struct i2c_client *i2c)
>  		}
>  	}
>  
> +	ret = ksz_regfields_init(dev, chip);
> +	if (ret)
> +		return ret;
> +
>  	if (i2c->dev.platform_data)
>  		dev->pdata = i2c->dev.platform_data;
>