Reset controller within a clock driver

Commit Message

Conor Dooley May 27, 2022, 6:40 p.m. UTC

Hi Stephen,

After I sent the fix for the broken resets in clk/microchip/clk-mpfs.c,
[0] I started looking at making a proper reset controller driver a la
clk/renesas/{renesas-cpg-mssr,rzgl2l-cpg}.c where the reset controller
is part of the clock driver file.

I did it that way b/c the reset controller is just a single reg,
surrounded by registers used by clocks. It's roughly a +130,-10 line
change to the existing driver. A /very/ rough version that will not
apply without other cleanup is appended for context.

Before I got around to testing properly and cleaning it up for
submission, I saw a mail you had sent and wondered if I'd gone for the
wrong approach [1].

Should I instead have my clock driver create a device for the reset
controller to bind to, or is that overkill for a single register &
Serge's situation is different b/c he'd created a file purely for
a reset controller?

Thanks,
Conor.

0 - https://lore.kernel.org/linux-clk/[e-mail]/
1 - https://lore.kernel.org/linux-clk/[e-mail]/

Comments

Stephen Boyd May 27, 2022, 7:26 p.m. UTC | #1

Quoting [e-mail] (2022-05-27 11:40:59)
> Hi Stephen,
> 
> After I sent the fix for the broken resets in clk/microchip/clk-mpfs.c,
> [0] I started looking at making a proper reset controller driver a la
> clk/renesas/{renesas-cpg-mssr,rzgl2l-cpg}.c where the reset controller
> is part of the clock driver file.
> 
> I did it that way b/c the reset controller is just a single reg,
> surrounded by registers used by clocks. It's roughly a +130,-10 line
> change to the existing driver. A /very/ rough version that will not
> apply without other cleanup is appended for context.
> 
> Before I got around to testing properly and cleaning it up for
> submission, I saw a mail you had sent and wondered if I'd gone for the
> wrong approach [1].
> 
> Should I instead have my clock driver create a device for the reset
> controller to bind to, or is that overkill for a single register &
> Serge's situation is different b/c he'd created a file purely for
> a reset controller?
> 

It's really up to you. It may be better to use auxiliary bus to split
the logic out to different subsystems. I can review the reset code but
I'm not the reset maintainer. Historically we've just accepted that
sometimes SoCs combine the clk and reset controls together into a "clock
and reset controller" and so we have the driver register clks and
resets. Using the bus to split up the device will help move these
registration calls to the appropriate subsystem so that the
reviewer/maintainer load is spread around.

Conor Dooley May 27, 2022, 8:52 p.m. UTC | #2

On 27/05/2022 20:26, Stephen Boyd wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Quoting [e-mail] (2022-05-27 11:40:59)
>> Hi Stephen,
>>
>> After I sent the fix for the broken resets in clk/microchip/clk-mpfs.c,
>> [0] I started looking at making a proper reset controller driver a la
>> clk/renesas/{renesas-cpg-mssr,rzgl2l-cpg}.c where the reset controller
>> is part of the clock driver file.
>>
>> I did it that way b/c the reset controller is just a single reg,
>> surrounded by registers used by clocks. It's roughly a +130,-10 line
>> change to the existing driver. A /very/ rough version that will not
>> apply without other cleanup is appended for context.
>>
>> Before I got around to testing properly and cleaning it up for
>> submission, I saw a mail you had sent and wondered if I'd gone for the
>> wrong approach [1].
>>
>> Should I instead have my clock driver create a device for the reset
>> controller to bind to, or is that overkill for a single register &
>> Serge's situation is different b/c he'd created a file purely for
>> a reset controller?
>>
> 
> It's really up to you. It may be better to use auxiliary bus to split
> the logic out to different subsystems. I can review the reset code but
> I'm not the reset maintainer.

Aye, CC'ed him in case he had an opinion too.

> Historically we've just accepted that
> sometimes SoCs combine the clk and reset controls together into a "clock
> and reset controller" and so we have the driver register clks and
> resets. Using the bus to split up the device will help move these
> registration calls to the appropriate subsystem so that the
> reviewer/maintainer load is spread around.

SGTM, I'll give it a go.
Thanks,
Conor.

Patch

diff --git a/drivers/clk/microchip/clk-mpfs.c b/drivers/clk/microchip/clk-mpfs.c
index ce3a48472fba..d9d1a4d9f131 100644
--- a/drivers/clk/microchip/clk-mpfs.c
+++ b/drivers/clk/microchip/clk-mpfs.c
@@ -9,6 +9,7 @@ 
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/reset-controller.h>
 #include <linux/slab.h>
 #include <dt-bindings/clock/microchip,mpfs-clock.h>
 
@@ -29,7 +30,13 @@ 
 #define MSSPLL_POSTDIV_WIDTH   0x07u
 #define MSSPLL_FIXED_DIV       4u
 
+#define MPFS_PERIPH_OFFSET     3u
+
 struct mpfs_clock_data {
+       struct device *dev;
+#ifdef CONFIG_RESET_CONTROLLER
+       struct reset_controller_dev rcdev;
+#endif
        void __iomem *base;
        void __iomem *msspll_base;
        struct clk_hw_onecell_data hw_data;
@@ -344,10 +351,6 @@  static int mpfs_periph_clk_enable(struct clk_hw *hw)
 
        spin_lock_irqsave(&mpfs_clk_lock, flags);
 
-       reg = readl_relaxed(base_addr + REG_SUBBLK_RESET_CR);
-       val = reg & ~(1u << periph->shift);
-       writel_relaxed(val, base_addr + REG_SUBBLK_RESET_CR);
-
        reg = readl_relaxed(base_addr + REG_SUBBLK_CLOCK_CR);
        val = reg | (1u << periph->shift);
        writel_relaxed(val, base_addr + REG_SUBBLK_CLOCK_CR);
@@ -381,12 +384,9 @@  static int mpfs_periph_clk_is_enabled(struct clk_hw *hw)
        void __iomem *base_addr = periph_hw->base;
        u32 reg;
 
-       reg = readl_relaxed(base_addr + REG_SUBBLK_RESET_CR);
-       if ((reg & (1u << periph->shift)) == 0u) {
-               reg = readl_relaxed(base_addr + REG_SUBBLK_CLOCK_CR);
-               if (reg & (1u << periph->shift))
-                       return 1;
-       }
+       reg = readl_relaxed(base_addr + REG_SUBBLK_CLOCK_CR);
+       if (reg & (1u << periph->shift))
+               return 1;
 
        return 0;
 }
@@ -472,6 +472,118 @@  static int mpfs_clk_register_periphs(struct device *dev, struct mpfs_periph_hw_c
        return 0;
 }
 
+/*
+ * Peripheral clock resets
+ *
+ * CLK_RESERVED does not map to a clock, but it does map to a reset line, so it
+ * has to be accounted for here.
+ *
+ */
+
+#ifdef CONFIG_RESET_CONTROLLER
+
+#define rcdev_to_clock_data(x) container_of((x), struct mpfs_clock_data, rcdev)
+
+// static int mpfs_reset(struct reset_controller_dev *rcdev, unsigned long id)
+// { 
+//     struct mpfs_clock_data *clk_data = rcdev_to_clock_data(rcdev);
+//
+//
+//
+//
+//
+//
+//
+//
+//
+//
+//
+//
+//
+//
+//
+//     return 0;
+// }
+
+static int mpfs_assert(struct reset_controller_dev *rcdev, unsigned long id)
+{
+       struct mpfs_clock_data *clk_data = rcdev_to_clock_data(rcdev);
+       u32 reg, val;
+
+       reg = readl_relaxed(clk_data->base + REG_SUBBLK_RESET_CR);
+       val = reg | (1u << id);
+       writel_relaxed(val, clk_data->base + REG_SUBBLK_RESET_CR);
+
+       dev_dbg(clk_data->dev, "deassert reset: %02lu\n", id);
+       return 0;
+}
+
+static int mpfs_deassert(struct reset_controller_dev *rcdev, unsigned long id)
+{
+       struct mpfs_clock_data *clk_data = rcdev_to_clock_data(rcdev);
+       u32 reg, val;
+
+       reg = readl_relaxed(clk_data->base + REG_SUBBLK_RESET_CR);
+       val = reg & ~(1u << id);
+       writel_relaxed(val, clk_data->base + REG_SUBBLK_RESET_CR);
+
+       dev_dbg(clk_data->dev, "deassert reset: %02lu\n", id);
+
+       return 0;
+}
+
+static int mpfs_status(struct reset_controller_dev *rcdev, unsigned long id)
+{
+       struct mpfs_clock_data *clk_data = rcdev_to_clock_data(rcdev);
+       u32 reg;
+
+       reg = readl_relaxed(clk_data->base + REG_SUBBLK_RESET_CR);
+       return (reg & (1u << id));
+}
+
+       // .reset = mpfs_reset,
+static const struct reset_control_ops mpfs_reset_ops = {
+       .assert = mpfs_assert,
+       .deassert = mpfs_deassert,
+       .status = mpfs_status,
+};
+
+//geert - does it make sense to reuse the clk_ indexes for the reset ctrlr?
+// -> they run from 3 to 32 but skip one
+//if yes, do i p much just subtract 3 in of_xlate & manipulate that bit?
+static int mpfs_reset_xlate(struct reset_controller_dev *rcdev,
+                           const struct of_phandle_args *reset_spec)
+{
+       struct mpfs_clock_data *clk_data = rcdev_to_clock_data(rcdev);
+       unsigned int index = reset_spec->args[0];
+       /* account for reserved fpga fabric reset */
+       unsigned int num_resets = ARRAY_SIZE(mpfs_periph_clks) + 1;
+
+       if (index < MPFS_PERIPH_OFFSET || index > (MPFS_PERIPH_OFFSET + num_resets)) {
+               dev_err(clk_data->dev, "Invalid reset index %u\n", reset_spec->args[0]);
+               return -EINVAL;
+       }
+
+       return index - MPFS_PERIPH_OFFSET;
+}
+static int mpfs_reset_controller_register(struct mpfs_clock_data *clk_data)
+{
+       clk_data->rcdev.ops = &mpfs_reset_ops;
+       clk_data->rcdev.of_node = clk_data->dev->of_node;
+       clk_data->rcdev.of_reset_n_cells = 1;
+       clk_data->rcdev.of_xlate = mpfs_reset_xlate;
+       /* CLK_RESERVED is not part of mpfs_periph_clks, so add 1 */
+       clk_data->rcdev.nr_resets = ARRAY_SIZE(mpfs_periph_clks) + 1;
+       return devm_reset_controller_register(clk_data->dev, &clk_data->rcdev);
+}
+
+#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;
@@ -496,6 +608,7 @@  static int mpfs_clk_probe(struct platform_device *pdev)
                return PTR_ERR(clk_data->msspll_base);
 
        clk_data->hw_data.num = num_clks;
+       clk_data->dev = dev;
 
        ret = mpfs_clk_register_mssplls(dev, mpfs_msspll_clks, ARRAY_SIZE(mpfs_msspll_clks),
                                        clk_data);
@@ -515,6 +628,10 @@  static int mpfs_clk_probe(struct platform_device *pdev)
        if (ret)
                return ret;
 
+       ret = mpfs_reset_controller_register(clk_data);
+       if (ret)
+               return ret;
+
        return ret;
 }