From: Conor Dooley <[email protected]>
Stephen and Philipp, while reviewing patches, said that all of the aux
device creation and the register read/write code could be moved to the
reset subsystem, leaving the clock driver with no implementations of
reset_* functions at all. Move them.
Suggested-by: Philipp Zabel <[email protected]>
Suggested-by: Stephen Boyd <[email protected]>
Signed-off-by: Conor Dooley <[email protected]>
---
v2: fixup a build warning and some minor checkpatch complaints
CC: Conor Dooley <[email protected]>
CC: Daire McNamara <[email protected]>
CC: Michael Turquette <[email protected]>
CC: Stephen Boyd <[email protected]>
CC: Philipp Zabel <[email protected]>
CC: [email protected]
CC: [email protected]
CC: [email protected]
---
drivers/clk/microchip/clk-mpfs.c | 92 +------------------------------
drivers/reset/reset-mpfs.c | 95 +++++++++++++++++++++++++++++---
include/soc/microchip/mpfs.h | 10 ++--
3 files changed, 93 insertions(+), 104 deletions(-)
diff --git a/drivers/clk/microchip/clk-mpfs.c b/drivers/clk/microchip/clk-mpfs.c
index 22eab91a6712..28ec0da88cb3 100644
--- a/drivers/clk/microchip/clk-mpfs.c
+++ b/drivers/clk/microchip/clk-mpfs.c
@@ -4,12 +4,10 @@
*
* Copyright (C) 2020-2022 Microchip Technology Inc. All rights reserved.
*/
-#include <linux/auxiliary_bus.h>
#include <linux/clk-provider.h>
#include <linux/io.h>
#include <linux/module.h>
#include <linux/platform_device.h>
-#include <linux/slab.h>
#include <dt-bindings/clock/microchip,mpfs-clock.h>
#include <soc/microchip/mpfs.h>
@@ -361,93 +359,6 @@ static int mpfs_clk_register_periphs(struct device *dev, struct mpfs_periph_hw_c
return 0;
}
-/*
- * Peripheral clock resets
- */
-
-#if IS_ENABLED(CONFIG_RESET_CONTROLLER)
-
-u32 mpfs_reset_read(struct device *dev)
-{
- struct mpfs_clock_data *clock_data = dev_get_drvdata(dev->parent);
-
- return readl_relaxed(clock_data->base + REG_SUBBLK_RESET_CR);
-}
-EXPORT_SYMBOL_NS_GPL(mpfs_reset_read, MCHP_CLK_MPFS);
-
-void mpfs_reset_write(struct device *dev, u32 val)
-{
- struct mpfs_clock_data *clock_data = dev_get_drvdata(dev->parent);
-
- writel_relaxed(val, clock_data->base + REG_SUBBLK_RESET_CR);
-}
-EXPORT_SYMBOL_NS_GPL(mpfs_reset_write, MCHP_CLK_MPFS);
-
-static void mpfs_reset_unregister_adev(void *_adev)
-{
- struct auxiliary_device *adev = _adev;
-
- auxiliary_device_delete(adev);
- auxiliary_device_uninit(adev);
-}
-
-static void mpfs_reset_adev_release(struct device *dev)
-{
- struct auxiliary_device *adev = to_auxiliary_dev(dev);
-
- kfree(adev);
-}
-
-static struct auxiliary_device *mpfs_reset_adev_alloc(struct mpfs_clock_data *clk_data)
-{
- struct auxiliary_device *adev;
- int ret;
-
- adev = kzalloc(sizeof(*adev), GFP_KERNEL);
- if (!adev)
- return ERR_PTR(-ENOMEM);
-
- adev->name = "reset-mpfs";
- adev->dev.parent = clk_data->dev;
- adev->dev.release = mpfs_reset_adev_release;
- adev->id = 666u;
-
- ret = auxiliary_device_init(adev);
- if (ret) {
- kfree(adev);
- return ERR_PTR(ret);
- }
-
- return adev;
-}
-
-static int mpfs_reset_controller_register(struct mpfs_clock_data *clk_data)
-{
- struct auxiliary_device *adev;
- int ret;
-
- adev = mpfs_reset_adev_alloc(clk_data);
- if (IS_ERR(adev))
- return PTR_ERR(adev);
-
- ret = auxiliary_device_add(adev);
- if (ret) {
- auxiliary_device_uninit(adev);
- return ret;
- }
-
- return devm_add_action_or_reset(clk_data->dev, mpfs_reset_unregister_adev, adev);
-}
-
-#else /* !CONFIG_RESET_CONTROLLER */
-
-static int mpfs_reset_controller_register(struct mpfs_clock_data *clk_data)
-{
- return 0;
-}
-
-#endif /* !CONFIG_RESET_CONTROLLER */
-
static int mpfs_clk_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -499,7 +410,7 @@ static int mpfs_clk_probe(struct platform_device *pdev)
if (ret)
return ret;
- return mpfs_reset_controller_register(clk_data);
+ return mpfs_reset_controller_register(dev, clk_data->base + REG_SUBBLK_RESET_CR);
}
static const struct of_device_id mpfs_clk_of_match_table[] = {
@@ -532,3 +443,4 @@ MODULE_DESCRIPTION("Microchip PolarFire SoC Clock Driver");
MODULE_AUTHOR("Padmarao Begari <[email protected]>");
MODULE_AUTHOR("Daire McNamara <[email protected]>");
MODULE_AUTHOR("Conor Dooley <[email protected]>");
+MODULE_IMPORT_NS(MCHP_CLK_MPFS);
diff --git a/drivers/reset/reset-mpfs.c b/drivers/reset/reset-mpfs.c
index 7f3fb2d472f4..710f9c1676f9 100644
--- a/drivers/reset/reset-mpfs.c
+++ b/drivers/reset/reset-mpfs.c
@@ -8,9 +8,11 @@
*/
#include <linux/auxiliary_bus.h>
#include <linux/delay.h>
+#include <linux/io.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/platform_device.h>
+#include <linux/slab.h>
#include <linux/reset-controller.h>
#include <dt-bindings/clock/microchip,mpfs-clock.h>
#include <soc/microchip/mpfs.h>
@@ -28,20 +30,30 @@
/* block concurrent access to the soft reset register */
static DEFINE_SPINLOCK(mpfs_reset_lock);
+struct mpfs_reset {
+ void __iomem *base;
+ struct reset_controller_dev rcdev;
+};
+
+static inline struct mpfs_reset *to_mpfs_reset(struct reset_controller_dev *rcdev)
+{
+ return container_of(rcdev, struct mpfs_reset, rcdev);
+}
+
/*
* Peripheral clock resets
*/
-
static int mpfs_assert(struct reset_controller_dev *rcdev, unsigned long id)
{
+ struct mpfs_reset *rst = to_mpfs_reset(rcdev);
unsigned long flags;
u32 reg;
spin_lock_irqsave(&mpfs_reset_lock, flags);
- reg = mpfs_reset_read(rcdev->dev);
+ reg = readl(rst->base);
reg |= BIT(id);
- mpfs_reset_write(rcdev->dev, reg);
+ writel(reg, rst->base);
spin_unlock_irqrestore(&mpfs_reset_lock, flags);
@@ -50,14 +62,15 @@ static int mpfs_assert(struct reset_controller_dev *rcdev, unsigned long id)
static int mpfs_deassert(struct reset_controller_dev *rcdev, unsigned long id)
{
+ struct mpfs_reset *rst = to_mpfs_reset(rcdev);
unsigned long flags;
u32 reg;
spin_lock_irqsave(&mpfs_reset_lock, flags);
- reg = mpfs_reset_read(rcdev->dev);
+ reg = readl(rst->base);
reg &= ~BIT(id);
- mpfs_reset_write(rcdev->dev, reg);
+ writel(reg, rst->base);
spin_unlock_irqrestore(&mpfs_reset_lock, flags);
@@ -66,7 +79,8 @@ static int mpfs_deassert(struct reset_controller_dev *rcdev, unsigned long id)
static int mpfs_status(struct reset_controller_dev *rcdev, unsigned long id)
{
- u32 reg = mpfs_reset_read(rcdev->dev);
+ struct mpfs_reset *rst = to_mpfs_reset(rcdev);
+ u32 reg = readl(rst->base);
/*
* It is safe to return here as MPFS_NUM_RESETS makes sure the sign bit
@@ -121,11 +135,15 @@ static int mpfs_reset_probe(struct auxiliary_device *adev,
{
struct device *dev = &adev->dev;
struct reset_controller_dev *rcdev;
+ struct mpfs_reset *rst;
- rcdev = devm_kzalloc(dev, sizeof(*rcdev), GFP_KERNEL);
- if (!rcdev)
+ rst = devm_kzalloc(dev, sizeof(*rst), GFP_KERNEL);
+ if (!rst)
return -ENOMEM;
+ rst->base = (void __iomem *)adev->dev.platform_data;
+
+ rcdev = &rst->rcdev;
rcdev->dev = dev;
rcdev->dev->parent = dev->parent;
rcdev->ops = &mpfs_reset_ops;
@@ -137,9 +155,68 @@ static int mpfs_reset_probe(struct auxiliary_device *adev,
return devm_reset_controller_register(dev, rcdev);
}
+static void mpfs_reset_unregister_adev(void *_adev)
+{
+ struct auxiliary_device *adev = _adev;
+
+ auxiliary_device_delete(adev);
+ auxiliary_device_uninit(adev);
+}
+
+static void mpfs_reset_adev_release(struct device *dev)
+{
+ struct auxiliary_device *adev = to_auxiliary_dev(dev);
+
+ kfree(adev);
+}
+
+static struct auxiliary_device *mpfs_reset_adev_alloc(struct device *clk_dev)
+{
+ struct auxiliary_device *adev;
+ int ret;
+
+ adev = kzalloc(sizeof(*adev), GFP_KERNEL);
+ if (!adev)
+ return ERR_PTR(-ENOMEM);
+
+ adev->name = "reset-mpfs";
+ adev->dev.parent = clk_dev;
+ adev->dev.release = mpfs_reset_adev_release;
+ adev->id = 666u;
+
+ ret = auxiliary_device_init(adev);
+ if (ret) {
+ kfree(adev);
+ return ERR_PTR(ret);
+ }
+
+ return adev;
+}
+
+int mpfs_reset_controller_register(struct device *clk_dev, void __iomem *base)
+{
+ struct auxiliary_device *adev;
+ int ret;
+
+ adev = mpfs_reset_adev_alloc(clk_dev);
+ if (IS_ERR(adev))
+ return PTR_ERR(adev);
+
+ ret = auxiliary_device_add(adev);
+ if (ret) {
+ auxiliary_device_uninit(adev);
+ return ret;
+ }
+
+ adev->dev.platform_data = (__force void *)base;
+
+ return devm_add_action_or_reset(clk_dev, mpfs_reset_unregister_adev, adev);
+}
+EXPORT_SYMBOL_NS_GPL(mpfs_reset_controller_register, MCHP_CLK_MPFS);
+
static const struct auxiliary_device_id mpfs_reset_ids[] = {
{
- .name = "clk_mpfs.reset-mpfs",
+ .name = "reset_mpfs.reset-mpfs",
},
{ }
};
diff --git a/include/soc/microchip/mpfs.h b/include/soc/microchip/mpfs.h
index 09722f83b0ca..d7e612b5e22e 100644
--- a/include/soc/microchip/mpfs.h
+++ b/include/soc/microchip/mpfs.h
@@ -43,11 +43,11 @@ struct mtd_info *mpfs_sys_controller_get_flash(struct mpfs_sys_controller *mpfs_
#endif /* if IS_ENABLED(CONFIG_POLARFIRE_SOC_SYS_CTRL) */
#if IS_ENABLED(CONFIG_MCHP_CLK_MPFS)
-
-u32 mpfs_reset_read(struct device *dev);
-
-void mpfs_reset_write(struct device *dev, u32 val);
-
+#if IS_ENABLED(CONFIG_RESET_CONTROLLER)
+int mpfs_reset_controller_register(struct device *clk_dev, void __iomem *base);
+#else
+static inline int mpfs_reset_controller_register(struct device *clk_dev, void __iomem *base) { return 0; }
+#endif /* if IS_ENABLED(CONFIG_RESET_CONTROLLER) */
#endif /* if IS_ENABLED(CONFIG_MCHP_CLK_MPFS) */
#endif /* __SOC_MPFS_H__ */
--
2.43.0
Quoting Conor Dooley (2024-04-24 01:42:08)
> diff --git a/drivers/reset/reset-mpfs.c b/drivers/reset/reset-mpfs.c
> index 7f3fb2d472f4..710f9c1676f9 100644
> --- a/drivers/reset/reset-mpfs.c
> +++ b/drivers/reset/reset-mpfs.c
> @@ -121,11 +135,15 @@ static int mpfs_reset_probe(struct auxiliary_device *adev,
> {
> struct device *dev = &adev->dev;
> struct reset_controller_dev *rcdev;
> + struct mpfs_reset *rst;
>
> - rcdev = devm_kzalloc(dev, sizeof(*rcdev), GFP_KERNEL);
> - if (!rcdev)
> + rst = devm_kzalloc(dev, sizeof(*rst), GFP_KERNEL);
> + if (!rst)
> return -ENOMEM;
>
> + rst->base = (void __iomem *)adev->dev.platform_data;
Can use dev_get_platdata() here?
rst->base = (void __iomem *)dev_get_platdata(dev);
That's sad that a cast is necessary. Does it need __force as well? An
alternative would be to make a container struct for auxiliary_device and
put the pointer there.
On Wed, Apr 24, 2024 at 11:33:32AM -0700, Stephen Boyd wrote:
> Quoting Conor Dooley (2024-04-24 01:42:08)
> > diff --git a/drivers/reset/reset-mpfs.c b/drivers/reset/reset-mpfs.c
> > index 7f3fb2d472f4..710f9c1676f9 100644
> > --- a/drivers/reset/reset-mpfs.c
> > +++ b/drivers/reset/reset-mpfs.c
> > @@ -121,11 +135,15 @@ static int mpfs_reset_probe(struct auxiliary_device *adev,
> > {
> > struct device *dev = &adev->dev;
> > struct reset_controller_dev *rcdev;
> > + struct mpfs_reset *rst;
> >
> > - rcdev = devm_kzalloc(dev, sizeof(*rcdev), GFP_KERNEL);
> > - if (!rcdev)
> > + rst = devm_kzalloc(dev, sizeof(*rst), GFP_KERNEL);
> > + if (!rst)
> > return -ENOMEM;
> >
> > + rst->base = (void __iomem *)adev->dev.platform_data;
>
> Can use dev_get_platdata() here?
>
> rst->base = (void __iomem *)dev_get_platdata(dev);
>
> That's sad that a cast is necessary. Does it need __force as well? An
> alternative would be to make a container struct for auxiliary_device and
> put the pointer there.
Ye, I dunno if it was sparse that yelled at me, but either it or the
compiler didn't approve. I don't really like the casting in and out, but
the alternative I don't find elegant either, so I picked the one I deemed
simpler. I'm happy to go with whichever you prefer.
And re: __force, AFAIU that's only required while discarding the
__iomem, so the cast into the platform_data has one:
adev->dev.platform_data = (__force void *)base;
Quoting Conor Dooley (2024-04-24 13:19:04)
> On Wed, Apr 24, 2024 at 11:33:32AM -0700, Stephen Boyd wrote:
> > Quoting Conor Dooley (2024-04-24 01:42:08)
> > > diff --git a/drivers/reset/reset-mpfs.c b/drivers/reset/reset-mpfs.c
> > > index 7f3fb2d472f4..710f9c1676f9 100644
> > > --- a/drivers/reset/reset-mpfs.c
> > > +++ b/drivers/reset/reset-mpfs.c
> > > @@ -121,11 +135,15 @@ static int mpfs_reset_probe(struct auxiliary_device *adev,
> > > {
> > > struct device *dev = &adev->dev;
> > > struct reset_controller_dev *rcdev;
> > > + struct mpfs_reset *rst;
> > >
> > > - rcdev = devm_kzalloc(dev, sizeof(*rcdev), GFP_KERNEL);
> > > - if (!rcdev)
> > > + rst = devm_kzalloc(dev, sizeof(*rst), GFP_KERNEL);
> > > + if (!rst)
> > > return -ENOMEM;
> > >
> > > + rst->base = (void __iomem *)adev->dev.platform_data;
> >
> > Can use dev_get_platdata() here?
> >
> > rst->base = (void __iomem *)dev_get_platdata(dev);
> >
> > That's sad that a cast is necessary. Does it need __force as well? An
> > alternative would be to make a container struct for auxiliary_device and
> > put the pointer there.
>
>
> Ye, I dunno if it was sparse that yelled at me, but either it or the
> compiler didn't approve. I don't really like the casting in and out, but
> the alternative I don't find elegant either, so I picked the one I deemed
> simpler. I'm happy to go with whichever you prefer.
I don't really care. Am I supposed to pick this patch up?
>
> And re: __force, AFAIU that's only required while discarding the
> __iomem, so the cast into the platform_data has one:
> adev->dev.platform_data = (__force void *)base;
>
Ah, ok. Thanks for the remind.
On Wed, Apr 24, 2024 at 02:09:44PM -0700, Stephen Boyd wrote:
> Am I supposed to pick this patch up?
Eh, you'll probably save Claudiu a single patch PR if you do, there's
nothing in the clk-microchip queue at the moment & I wasn't going to ask
him to pick it up until you or Philipp were happy with it, given you two
were the ones that suggested the change.
I say go for it, unless you want me to respin for dev_get_platdata().
Thanks,
Conor.
On 25.04.2024 00:16, Conor Dooley wrote:
> On Wed, Apr 24, 2024 at 02:09:44PM -0700, Stephen Boyd wrote:
>
>> Am I supposed to pick this patch up?
>
> Eh, you'll probably save Claudiu a single patch PR if you do, there's
> nothing in the clk-microchip queue at the moment & I wasn't going to ask
> him to pick it up until you or Philipp were happy with it, given you two
> were the ones that suggested the change.
> I say go for it,
Sounds good for me.
Thank you,
Claudiu Beznea
> unless you want me to respin for dev_get_platdata().
>
> Thanks,
> Conor.
Quoting Conor Dooley (2024-04-24 01:42:08)
> From: Conor Dooley <[email protected]>
>
> Stephen and Philipp, while reviewing patches, said that all of the aux
> device creation and the register read/write code could be moved to the
> reset subsystem, leaving the clock driver with no implementations of
> reset_* functions at all. Move them.
>
> Suggested-by: Philipp Zabel <[email protected]>
> Suggested-by: Stephen Boyd <[email protected]>
> Signed-off-by: Conor Dooley <[email protected]>
> ---
Applied to clk-next