| Message ID | 1718350923-21392-4-git-send-email-shengjiu.wang@nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | clk: imx: clk-audiomix: Improvement for audiomix | expand |
Hi, On Fr, 2024-06-14 at 15:42 +0800, 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> > Reviewed-by: Marco Felsch <m.felsch@pengutronix.de> > --- > drivers/reset/Kconfig | 8 ++ > drivers/reset/Makefile | 1 + > drivers/reset/reset-imx8mp-audiomix.c | 106 ++++++++++++++++++++++++++ > 3 files changed, 115 insertions(+) > create mode 100644 drivers/reset/reset-imx8mp-audiomix.c > > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig > index 7112f5932609..b3c0e528d08c 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_IMX8MP_AUDIOMIX > + tristate "i.MX8MP AudioMix Reset Driver" > + depends on CLK_IMX8MP I'd like this to be made compile-testable without CLK_IMX8MP being enabled. > + select AUXILIARY_BUS > + default CLK_IMX8MP > + help > + This enables the reset controller driver for i.MX8MP AudioMix > + > 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..a6796e83900b 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_IMX8MP_AUDIOMIX) += reset-imx8mp-audiomix.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-imx8mp-audiomix.c b/drivers/reset/reset-imx8mp-audiomix.c > new file mode 100644 > index 000000000000..1fc984bc08c0 > --- /dev/null > +++ b/drivers/reset/reset-imx8mp-audiomix.c > @@ -0,0 +1,106 @@ > +// 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> Still needed? > +#include <linux/reset-controller.h> > + > +#define EARC 0x200 > +#define EARC_RESET_MASK 0x3 > + > +struct imx8mp_audiomix_reset { > + struct reset_controller_dev rcdev; > + void __iomem *base; > +}; > + > +static struct imx8mp_audiomix_reset *to_imx8mp_audiomix_reset(struct reset_controller_dev *rcdev) > +{ > + return container_of(rcdev, struct imx8mp_audiomix_reset, rcdev); > +} > + > +static int imx8mp_audiomix_reset_assert(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + struct imx8mp_audiomix_reset *priv = to_imx8mp_audiomix_reset(rcdev); > + void __iomem *reg_addr = priv->base; > + unsigned int mask, reg; > + > + if (id >= fls(EARC_RESET_MASK)) > + return -EINVAL; This check is not required. Since you have nr_resets set to fls(EARC_RESET_MASK), the same is already checked in of_reset_simple_xlate, before a reset control is even returned. > + mask = BIT(id); > + reg = readl(reg_addr + EARC); > + writel(reg & ~mask, reg_addr + EARC); There are multiple (well, two) resets in this register, so it would be good style to protect the read-modify-write cycle with a spinlock. > + > + return 0; > +} > + > +static int imx8mp_audiomix_reset_deassert(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + struct imx8mp_audiomix_reset *priv = to_imx8mp_audiomix_reset(rcdev); > + void __iomem *reg_addr = priv->base; > + unsigned int mask, reg; > + > + if (id >= fls(EARC_RESET_MASK)) > + return -EINVAL; > + > + mask = BIT(id); > + reg = readl(reg_addr + EARC); > + writel(reg | mask, reg_addr + EARC); > + > + return 0; > +} > + > +static const struct reset_control_ops imx8mp_audiomix_reset_ops = { > + .assert = imx8mp_audiomix_reset_assert, > + .deassert = imx8mp_audiomix_reset_deassert, > +}; > + > +static int imx8mp_audiomix_reset_probe(struct auxiliary_device *adev, > + const struct auxiliary_device_id *id) > +{ > + struct imx8mp_audiomix_reset *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 = fls(EARC_RESET_MASK); > + priv->rcdev.ops = &imx8mp_audiomix_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); This is missing the corresponding iounmap(). I wonder why we map at all. If the parent driver already has iomem mapped, can't it just pass that in, like JH7110 does? regards Philipp
On Fri, Jun 21, 2024 at 6:59 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > Hi, > > On Fr, 2024-06-14 at 15:42 +0800, 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> > > Reviewed-by: Marco Felsch <m.felsch@pengutronix.de> > > --- > > drivers/reset/Kconfig | 8 ++ > > drivers/reset/Makefile | 1 + > > drivers/reset/reset-imx8mp-audiomix.c | 106 ++++++++++++++++++++++++++ > > 3 files changed, 115 insertions(+) > > create mode 100644 drivers/reset/reset-imx8mp-audiomix.c > > > > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig > > index 7112f5932609..b3c0e528d08c 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_IMX8MP_AUDIOMIX > > + tristate "i.MX8MP AudioMix Reset Driver" > > + depends on CLK_IMX8MP > > I'd like this to be made compile-testable without CLK_IMX8MP being > enabled. Ok, will remove the depends. > > > + select AUXILIARY_BUS > > + default CLK_IMX8MP > > + help > > + This enables the reset controller driver for i.MX8MP AudioMix > > + > > 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..a6796e83900b 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_IMX8MP_AUDIOMIX) += reset-imx8mp-audiomix.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-imx8mp-audiomix.c b/drivers/reset/reset-imx8mp-audiomix.c > > new file mode 100644 > > index 000000000000..1fc984bc08c0 > > --- /dev/null > > +++ b/drivers/reset/reset-imx8mp-audiomix.c > > @@ -0,0 +1,106 @@ > > +// 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> > > Still needed? Ok, can be removed. > > > +#include <linux/reset-controller.h> > > + > > +#define EARC 0x200 > > +#define EARC_RESET_MASK 0x3 > > + > > +struct imx8mp_audiomix_reset { > > + struct reset_controller_dev rcdev; > > + void __iomem *base; > > +}; > > + > > +static struct imx8mp_audiomix_reset *to_imx8mp_audiomix_reset(struct reset_controller_dev *rcdev) > > +{ > > + return container_of(rcdev, struct imx8mp_audiomix_reset, rcdev); > > +} > > + > > +static int imx8mp_audiomix_reset_assert(struct reset_controller_dev *rcdev, > > + unsigned long id) > > +{ > > + struct imx8mp_audiomix_reset *priv = to_imx8mp_audiomix_reset(rcdev); > > + void __iomem *reg_addr = priv->base; > > + unsigned int mask, reg; > > + > > + if (id >= fls(EARC_RESET_MASK)) > > + return -EINVAL; > > This check is not required. > > Since you have nr_resets set to fls(EARC_RESET_MASK), the same is > already checked in of_reset_simple_xlate, before a reset control is > even returned. Ok, will remove it. > > > + mask = BIT(id); > > + reg = readl(reg_addr + EARC); > > + writel(reg & ~mask, reg_addr + EARC); > > There are multiple (well, two) resets in this register, so it would be > good style to protect the read-modify-write cycle with a spinlock. Ok, will add spinlock > > > + > > + return 0; > > +} > > + > > +static int imx8mp_audiomix_reset_deassert(struct reset_controller_dev *rcdev, > > + unsigned long id) > > +{ > > + struct imx8mp_audiomix_reset *priv = to_imx8mp_audiomix_reset(rcdev); > > + void __iomem *reg_addr = priv->base; > > + unsigned int mask, reg; > > + > > + if (id >= fls(EARC_RESET_MASK)) > > + return -EINVAL; > > + > > + mask = BIT(id); > > + reg = readl(reg_addr + EARC); > > + writel(reg | mask, reg_addr + EARC); > > + > > + return 0; > > +} > > + > > +static const struct reset_control_ops imx8mp_audiomix_reset_ops = { > > + .assert = imx8mp_audiomix_reset_assert, > > + .deassert = imx8mp_audiomix_reset_deassert, > > +}; > > + > > +static int imx8mp_audiomix_reset_probe(struct auxiliary_device *adev, > > + const struct auxiliary_device_id *id) > > +{ > > + struct imx8mp_audiomix_reset *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 = fls(EARC_RESET_MASK); > > + priv->rcdev.ops = &imx8mp_audiomix_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); > > This is missing the corresponding iounmap(). Ok, will add iounmap. > > I wonder why we map at all. If the parent driver already has iomem > mapped, can't it just pass that in, like JH7110 does? Because this is a simple reset driver, just two reset bits, I want to avoid adding a header file for the parent driver and this driver. Thanks for reviewing. By the way: shall I just send a new version for this patch only next time? Best regards Shengjiu Wang > > > regards > Philipp
On Fr, 2024-06-21 at 20:34 +0800, Shengjiu Wang wrote: > > I wonder why we map at all. If the parent driver already has iomem > > mapped, can't it just pass that in, like JH7110 does? > > Because this is a simple reset driver, just two reset bits, > I want to avoid adding a header file for the parent driver and this driver. Ok. > Thanks for reviewing. > > By the way: shall I just send a new version for this patch only next time? Yes, this is the only remaining patch for me to pick up, no need to resend the patches that were already applied. regards Philipp
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig index 7112f5932609..b3c0e528d08c 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_IMX8MP_AUDIOMIX + tristate "i.MX8MP AudioMix Reset Driver" + depends on CLK_IMX8MP + select AUXILIARY_BUS + default CLK_IMX8MP + help + This enables the reset controller driver for i.MX8MP AudioMix + 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..a6796e83900b 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_IMX8MP_AUDIOMIX) += reset-imx8mp-audiomix.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-imx8mp-audiomix.c b/drivers/reset/reset-imx8mp-audiomix.c new file mode 100644 index 000000000000..1fc984bc08c0 --- /dev/null +++ b/drivers/reset/reset-imx8mp-audiomix.c @@ -0,0 +1,106 @@ +// 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> + +#define EARC 0x200 +#define EARC_RESET_MASK 0x3 + +struct imx8mp_audiomix_reset { + struct reset_controller_dev rcdev; + void __iomem *base; +}; + +static struct imx8mp_audiomix_reset *to_imx8mp_audiomix_reset(struct reset_controller_dev *rcdev) +{ + return container_of(rcdev, struct imx8mp_audiomix_reset, rcdev); +} + +static int imx8mp_audiomix_reset_assert(struct reset_controller_dev *rcdev, + unsigned long id) +{ + struct imx8mp_audiomix_reset *priv = to_imx8mp_audiomix_reset(rcdev); + void __iomem *reg_addr = priv->base; + unsigned int mask, reg; + + if (id >= fls(EARC_RESET_MASK)) + return -EINVAL; + + mask = BIT(id); + reg = readl(reg_addr + EARC); + writel(reg & ~mask, reg_addr + EARC); + + return 0; +} + +static int imx8mp_audiomix_reset_deassert(struct reset_controller_dev *rcdev, + unsigned long id) +{ + struct imx8mp_audiomix_reset *priv = to_imx8mp_audiomix_reset(rcdev); + void __iomem *reg_addr = priv->base; + unsigned int mask, reg; + + if (id >= fls(EARC_RESET_MASK)) + return -EINVAL; + + mask = BIT(id); + reg = readl(reg_addr + EARC); + writel(reg | mask, reg_addr + EARC); + + return 0; +} + +static const struct reset_control_ops imx8mp_audiomix_reset_ops = { + .assert = imx8mp_audiomix_reset_assert, + .deassert = imx8mp_audiomix_reset_deassert, +}; + +static int imx8mp_audiomix_reset_probe(struct auxiliary_device *adev, + const struct auxiliary_device_id *id) +{ + struct imx8mp_audiomix_reset *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 = fls(EARC_RESET_MASK); + priv->rcdev.ops = &imx8mp_audiomix_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); + + return devm_reset_controller_register(dev, &priv->rcdev); +} + +static const struct auxiliary_device_id imx8mp_audiomix_reset_ids[] = { + { + .name = "clk_imx8mp_audiomix.reset", + }, + { } +}; +MODULE_DEVICE_TABLE(auxiliary, imx8mp_audiomix_reset_ids); + +static struct auxiliary_driver imx8mp_audiomix_reset_driver = { + .probe = imx8mp_audiomix_reset_probe, + .id_table = imx8mp_audiomix_reset_ids, +}; + +module_auxiliary_driver(imx8mp_audiomix_reset_driver); + +MODULE_AUTHOR("Shengjiu Wang <shengjiu.wang@nxp.com>"); +MODULE_DESCRIPTION("Freescale i.MX8MP Audio Block Controller reset driver"); +MODULE_LICENSE("GPL");