Received-SPF: pass (google.com: domain of linux-kernel+bounces-195626-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 30 May 2024 11:53:33 -0400
From: Frank Li <Frank.li@nxp.com>
To: Marco Felsch <m.felsch@pengutronix.de>
Cc: Shengjiu Wang <shengjiu.wang@nxp.com>, p.zabel@pengutronix.de,
	abelvesa@kernel.org, peng.fan@nxp.com, mturquette@baylibre.com,
	sboyd@kernel.org, robh@kernel.org, krzk+dt@kernel.org,
	conor+dt@kernel.org, shawnguo@kernel.org, s.hauer@pengutronix.de,
	kernel@pengutronix.de, festevam@gmail.com, marex@denx.de,
	linux-clk@vger.kernel.org, imx@lists.linux.dev,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, shengjiu.wang@gmail.com
Subject: Re: [PATCH v6 3/5] reset: imx-aux: Add i.MX auxiliary reset driver
Message-ID: <Zlig/Z7u4nxvKLoQ@lizhi-Precision-Tower-5810>
References: <1717036278-3515-1-git-send-email-shengjiu.wang@nxp.com>
 <1717036278-3515-4-git-send-email-shengjiu.wang@nxp.com>
 <20240530090558.53reobf2zea22oi2@pengutronix.de>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240530090558.53reobf2zea22oi2@pengutronix.de>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?CVWi2Nl5OgsmaPPBEnHJj8mbSDd8o7UZjHZiZlm4mVQBrYtplV7rggwYSkuc?=
 =?us-ascii?Q?4KuuRuWxC8JnEVD8P+wFOQYrSgfw3E0/Gf0rN5leetKELcAzQjS1080lj/U2?=
 =?us-ascii?Q?OmEI3ZKEMW36TmmBmmOkGuhbiSDgjUeK/CpFjdVPj3dWm8q6tcCS7CX4/5AQ?=
 =?us-ascii?Q?xdJwyW0oREO/iB2PLaO3TvsYAKF4a0nPb0My7FWNg1o8LY7sGMquw7EonyH0?=
 =?us-ascii?Q?4QgqunRcOm3n/PFM286u1WiKx72ne23LtrAjUSvqWY0bgB664KAzxZG2uSCS?=
 =?us-ascii?Q?E+W4oGnPtPQbWHBSQka7kV+/0sfT9H0rUJdUnWVPEBA9HolHOKrMzP3eOjQ6?=
 =?us-ascii?Q?xNq1MOqa26H/ZW5o6dBM1BxNDOrDSRtC/2UTkR9PdVXcjbdTpbJHc0pD/GgN?=
 =?us-ascii?Q?JGKcsWy4KsNCLl/uGQ2CGVhVSla8MITvDZTHiM+fTObRBKe+/mZuMKzgh6lT?=
 =?us-ascii?Q?xAQTPwgzc6PCjcWVXpfJPq7w+qmqJJnDwt259ROmWq3OKvL7LGQlhtK7Twy/?=
 =?us-ascii?Q?Xnd6uuvLUe/ITPAx1bSxc9XsoueHnfbIjfhNjXZ30Be/AXK6l/dWxyxKmmjQ?=
 =?us-ascii?Q?A3mbcybt0MCe6YqFPv268yuXw44ShT6XQBbgwE3TfKXkNmUpS886f+Y6TEoo?=
 =?us-ascii?Q?5lj5v8y95CFW3Qw4UjNLTkpNR9/YvdlTUIoHe9+Abiq6F7t2jWFqMSJOiH3q?=
 =?us-ascii?Q?48SetFK3qCv949zaHp5UTbC2YjOC9YD3A0PCAMli/uhVy7QKf+pj177GN8yP?=
 =?us-ascii?Q?JDuPhX3Kka9IY4zLOBhQTJxEtrTOup1LfubNXECL9YyIxPeKcF/gJ3qZQwTL?=
 =?us-ascii?Q?ruPggq6TeH6ErYaMamdCMnDweK/hX14JXK08zwaUELhWDvpTXOmiqFVpRP16?=
 =?us-ascii?Q?1RmmxTzxeDcnsTbLIu0D1tHeMTo2JMjdILKE9Dt887PB6h04lIAf1v/Piytp?=
 =?us-ascii?Q?BSWLwvAUU/4SBBAOhbKOH/g98+9uZQyIgHaiO6sF2YeGJV4iIAXB1qM8VhLn?=
 =?us-ascii?Q?SkPWqiz5ofcBefUNcmyYwOtI/alurgx7Xmv+CMatSJxMz7cCJliOKAsOMZqx?=
 =?us-ascii?Q?dXxtdNYfU+ldxb8v/rDhLajB29+zHXltrjNGqPqgUrvjc5XvsOLCHSEHVW/n?=
 =?us-ascii?Q?A+Ks/+AjYdF9W2X6DffeoFDP44Jn1WC9cHd02Ck+vvpP0gcAbXs78/qtdLy5?=
 =?us-ascii?Q?5G0n3KF/3aBedTV00QW3y/nDtUPQmEVP1O7aOJ5q8kM71kyTSYCPay5VNNNj?=
 =?us-ascii?Q?0zSHthzYdH+3cZRtLVVeudbfaZK4SA0OQqYAS9othVtGzB2L+h/iFKPNaOCW?=
 =?us-ascii?Q?KlaDlj4OFpKqxDg+QRCK1ZtH+n+TBxSN4AuMXWUuwtjimFipYt8CSkaOGvXZ?=
 =?us-ascii?Q?XRjdpacRbkY8RobF+VPXVOT7oK72/4dTn0NLP88AELx2N2c8G4BvZHy9bM6I?=
 =?us-ascii?Q?ZTZqMb8cw5Y0U84bDmYBXFakLjUbTTVg8D4A/iPkg4XXDdQtoEpN+LEgUsjv?=
 =?us-ascii?Q?7uX8oqubhw5/xrFO+FrrPnPzg8ktceR+yoFW4K6ggNRveQXvruODiL92UicQ?=
 =?us-ascii?Q?agef2zh28Ito9JAF95w=3D?=
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 8ea0168b-1ebf-4907-8570-08dc80c0af83
X-MS-Exchange-CrossTenant-AuthSource: PAXPR04MB9642.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 May 2024 15:53:44.7557
 (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: H5+Zx2wEqiYn0PrJ2PbFDTGJMQRAZ5lzputQWaAsh8E60bS5qb/CDIHDFBp5DV+8/JAH803Y87Dl5J3rPGEUZA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS8PR04MB9079

On Thu, May 30, 2024 at 11:05:58AM +0200, Marco Felsch wrote:
> Hi,
> 
> On 24-05-30, Shengjiu Wang wrote:
> > Add support for the resets on i.MX8MP Audio Block Control module,
> > which includes the EARC PHY software reset and EARC controller
> > software reset. The reset controller is created using the auxiliary
> > device framework and set up in the clock driver.
> > 
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > ---
> >  drivers/reset/Kconfig         |   8 ++
> >  drivers/reset/Makefile        |   1 +
> >  drivers/reset/reset-imx-aux.c | 217 ++++++++++++++++++++++++++++++++++
> 			    ^
> You make use of the auxiliary bus but this isn't a aux driver, it's the
> i.MX8MP EARC reset driver. According the TRM only the EARC reset bits
> are covered by the AUDIOMIX blk-ctrl.
> 
> >  3 files changed, 226 insertions(+)
> >  create mode 100644 drivers/reset/reset-imx-aux.c
> > 
> > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> > index 7112f5932609..38fdf05b326b 100644
> > --- a/drivers/reset/Kconfig
> > +++ b/drivers/reset/Kconfig
> > @@ -91,6 +91,14 @@ config RESET_IMX7
> >  	help
> >  	  This enables the reset controller driver for i.MX7 SoCs.
> >  
> > +config RESET_IMX_AUX
> > +	tristate "i.MX Auxiliary Reset Driver"
> 			^
>               Same applies here
> 
> > +	depends on CLK_IMX8MP
> > +	select AUXILIARY_BUS
> > +	default CLK_IMX8MP
> > +	help
> > +	  This enables the auxiliary reset controller driver for i.MX.
> > +
> >  config RESET_INTEL_GW
> >  	bool "Intel Reset Controller Driver"
> >  	depends on X86 || COMPILE_TEST
> > diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> > index fd8b49fa46fc..f078da14c327 100644
> > --- a/drivers/reset/Makefile
> > +++ b/drivers/reset/Makefile
> > @@ -14,6 +14,7 @@ obj-$(CONFIG_RESET_BRCMSTB_RESCAL) += reset-brcmstb-rescal.o
> >  obj-$(CONFIG_RESET_GPIO) += reset-gpio.o
> >  obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o
> >  obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
> > +obj-$(CONFIG_RESET_IMX_AUX) += reset-imx-aux.o
> >  obj-$(CONFIG_RESET_INTEL_GW) += reset-intel-gw.o
> >  obj-$(CONFIG_RESET_K210) += reset-k210.o
> >  obj-$(CONFIG_RESET_LANTIQ) += reset-lantiq.o
> > diff --git a/drivers/reset/reset-imx-aux.c b/drivers/reset/reset-imx-aux.c
> > new file mode 100644
> > index 000000000000..61c353abc84e
> > --- /dev/null
> > +++ b/drivers/reset/reset-imx-aux.c
> > @@ -0,0 +1,217 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright 2024 NXP
> > + */
> > +
> > +#include <linux/auxiliary_bus.h>
> > +#include <linux/device.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/reset-controller.h>
> > +
> > +/*
> > + * The reset does not support the feature and corresponding
> > + * values are not valid
> > + */
> > +#define ASSERT_NONE     BIT(0)
> > +#define DEASSERT_NONE   BIT(1)
> > +#define STATUS_NONE     BIT(2)
> > +
> > +/* When set this function is activated by setting(vs clearing) this bit */
> > +#define ASSERT_SET      BIT(3)
> > +#define DEASSERT_SET    BIT(4)
> > +#define STATUS_SET      BIT(5)
> > +
> > +/* The following are the inverse of the above and are added for consistency */
> > +#define ASSERT_CLEAR    (0 << 3)
> > +#define DEASSERT_CLEAR  (0 << 4)
> > +#define STATUS_CLEAR    (0 << 5)
> > +
> > +/**
> > + * struct imx_reset_ctrl - reset control structure
> > + * @assert_offset: reset assert control register offset
> > + * @assert_bit: reset assert bit in the reset assert control register
> > + * @deassert_offset: reset deassert control register offset
> > + * @deassert_bit: reset deassert bit in the reset deassert control register
> > + * @status_offset: reset status register offset
> > + * @status_bit: reset status bit in the reset status register
> > + * @flags: reset flag indicating how the (de)assert and status are handled
> > + */
> > +struct imx_reset_ctrl {
> > +	u32 assert_offset;
> > +	u32 assert_bit;
> > +	u32 deassert_offset;
> > +	u32 deassert_bit;
> > +	u32 status_offset;
> > +	u32 status_bit;
> > +	u32 flags;
> > +};
> 
> Why do we make it this compicated for an simple EARC module reset? I
> understand that you want to provide a generic driver which can be
> re-used but there is actual no other user and may will get no other user
> in the future too. Therefore I would like to keep it simple at the
> begin and adapt the code on-demand.

There are many similar cases. such as
https://elixir.bootlin.com/linux/v6.10-rc1/source/drivers/pci/controller/dwc/pci-layerscape.c#L251

Previously it use syscon and regmap to a global register space region and
direct operate the register. Now this way will not preferred. It needs
export as reset driver. but actually, it just write some bits. 

We face the similar problem at difference driver when do upstream.

One on going a discussion about sim module reset
https://lore.kernel.org/imx/131e46b1-61d9-41de-a225-853b09c765d1@gmail.com/

We hope an unified and simple method to handle these cases.

Frank

> 
> Regards,
>   Marco
> 
> > +struct imx_reset_data {
> > +	const struct imx_reset_ctrl *rst_ctrl;
> > +	size_t rst_ctrl_num;
> > +};
> > +
> > +struct imx_aux_reset_priv {
> > +	struct reset_controller_dev rcdev;
> > +	void __iomem *base;
> > +	const struct imx_reset_data *data;
> > +};
> > +
> > +static int imx_aux_reset_assert(struct reset_controller_dev *rcdev,
> > +				unsigned long id)
> > +{
> > +	struct imx_aux_reset_priv *priv = container_of(rcdev,
> > +					struct imx_aux_reset_priv, rcdev);
> > +	const struct imx_reset_data *data = priv->data;
> > +	void __iomem *reg_addr = priv->base;
> > +	const struct imx_reset_ctrl *ctrl;
> > +	unsigned int mask, value, reg;
> > +
> > +	if (id >= data->rst_ctrl_num)
> > +		return -EINVAL;
> > +
> > +	ctrl = &data->rst_ctrl[id];
> > +
> > +	/* assert not supported for this reset */
> > +	if (ctrl->flags & ASSERT_NONE)
> > +		return -EOPNOTSUPP;
> > +
> > +	mask = BIT(ctrl->assert_bit);
> > +	value = (ctrl->flags & ASSERT_SET) ? mask : 0x0;
> > +
> > +	reg = readl(reg_addr + ctrl->assert_offset);
> > +	writel(reg | value, reg_addr + ctrl->assert_offset);
> > +
> > +	return 0;
> > +}
> > +
> > +static int imx_aux_reset_deassert(struct reset_controller_dev *rcdev,
> > +				  unsigned long id)
> > +{
> > +	struct imx_aux_reset_priv *priv = container_of(rcdev,
> > +					struct imx_aux_reset_priv, rcdev);
> > +	const struct imx_reset_data *data = priv->data;
> > +	void __iomem *reg_addr = priv->base;
> > +	const struct imx_reset_ctrl *ctrl;
> > +	unsigned int mask, value, reg;
> > +
> > +	if (id >= data->rst_ctrl_num)
> > +		return -EINVAL;
> > +
> > +	ctrl = &data->rst_ctrl[id];
> > +
> > +	/* deassert not supported for this reset */
> > +	if (ctrl->flags & DEASSERT_NONE)
> > +		return -EOPNOTSUPP;
> > +
> > +	mask = BIT(ctrl->deassert_bit);
> > +	value = (ctrl->flags & DEASSERT_SET) ? mask : 0x0;
> > +
> > +	reg = readl(reg_addr + ctrl->deassert_offset);
> > +	writel(reg | value, reg_addr + ctrl->deassert_offset);
> > +
> > +	return 0;
> > +}
> > +
> > +static int imx_aux_reset_status(struct reset_controller_dev *rcdev,
> > +				unsigned long id)
> > +{
> > +	struct imx_aux_reset_priv *priv = container_of(rcdev,
> > +					struct imx_aux_reset_priv, rcdev);
> > +	const struct imx_reset_data *data = priv->data;
> > +	void __iomem *reg_addr = priv->base;
> > +	const struct imx_reset_ctrl *ctrl;
> > +	unsigned int reset_state;
> > +
> > +	if (id >= data->rst_ctrl_num)
> > +		return -EINVAL;
> > +
> > +	ctrl = &data->rst_ctrl[id];
> > +
> > +	/* status not supported for this reset */
> > +	if (ctrl->flags & STATUS_NONE)
> > +		return -EOPNOTSUPP;
> > +
> > +	reset_state = readl(reg_addr + ctrl->status_offset);
> > +
> > +	return !(reset_state & BIT(ctrl->status_bit)) ==
> > +		!(ctrl->flags & STATUS_SET);
> > +}
> > +
> > +static const struct reset_control_ops imx_aux_reset_ops = {
> > +	.assert   = imx_aux_reset_assert,
> > +	.deassert = imx_aux_reset_deassert,
> > +	.status	  = imx_aux_reset_status,
> > +};
> > +
> > +static int imx_aux_reset_probe(struct auxiliary_device *adev,
> > +			       const struct auxiliary_device_id *id)
> > +{
> > +	struct imx_reset_data *data = (struct imx_reset_data *)(id->driver_data);
> > +	struct imx_aux_reset_priv *priv;
> > +	struct device *dev = &adev->dev;
> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->rcdev.owner     = THIS_MODULE;
> > +	priv->rcdev.nr_resets = data->rst_ctrl_num;
> > +	priv->rcdev.ops       = &imx_aux_reset_ops;
> > +	priv->rcdev.of_node   = dev->parent->of_node;
> > +	priv->rcdev.dev	      = dev;
> > +	priv->rcdev.of_reset_n_cells = 1;
> > +	priv->base            = of_iomap(dev->parent->of_node, 0);
> > +	priv->data            = data;
> > +
> > +	return devm_reset_controller_register(dev, &priv->rcdev);
> > +}
> > +
> > +#define EARC  0x200
> > +
> > +static const struct imx_reset_ctrl imx8mp_audiomix_rst_ctrl[] = {
> > +	{
> > +		.assert_offset = EARC,
> > +		.assert_bit = 0,
> > +		.deassert_offset = EARC,
> > +		.deassert_bit = 0,
> > +		.flags  = ASSERT_CLEAR | DEASSERT_SET | STATUS_NONE,
> > +	},
> > +	{
> > +		.assert_offset = EARC,
> > +		.assert_bit = 1,
> > +		.deassert_offset = EARC,
> > +		.deassert_bit = 1,
> > +		.flags  = ASSERT_CLEAR | DEASSERT_SET | STATUS_NONE,
> > +	},
> > +};
> > +
> > +static const struct imx_reset_data imx8mp_audiomix_rst_data = {
> > +	.rst_ctrl = imx8mp_audiomix_rst_ctrl,
> > +	.rst_ctrl_num = ARRAY_SIZE(imx8mp_audiomix_rst_ctrl),
> > +};
> > +
> > +static const struct auxiliary_device_id imx_aux_reset_ids[] = {
> > +	{
> > +		.name = "clk_imx8mp_audiomix.reset",
> > +		.driver_data = (kernel_ulong_t)&imx8mp_audiomix_rst_data,
> > +	},
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(auxiliary, imx_aux_reset_ids);
> > +
> > +static struct auxiliary_driver imx_aux_reset_driver = {
> > +	.probe		= imx_aux_reset_probe,
> > +	.id_table	= imx_aux_reset_ids,
> > +};
> > +
> > +module_auxiliary_driver(imx_aux_reset_driver);
> > +
> > +MODULE_AUTHOR("Shengjiu Wang <shengjiu.wang@nxp.com>");
> > +MODULE_DESCRIPTION("Freescale i.MX auxiliary reset driver");
> > +MODULE_LICENSE("GPL");
> > -- 
> > 2.34.1
> > 
> > 
> >