* [PATCH v6 0/2] CLK: microchip: Add clkcfg driver for Microchip PolarFire SoC
@ 2021-11-30 14:07 conor.dooley
2021-11-30 14:07 ` [PATCH v6 1/2] dt-bindings: clk: microchip: Add Microchip PolarFire host binding conor.dooley
2021-11-30 14:07 ` [PATCH v6 2/2] clk: microchip: Add driver for Microchip PolarFire SoC conor.dooley
0 siblings, 2 replies; 7+ messages in thread
From: conor.dooley @ 2021-11-30 14:07 UTC (permalink / raw)
To: mturquette, sboyd, linux-clk, robh+dt, devicetree
Cc: krzysztof.kozlowski, geert, david.abdurachmanov, palmer,
daire.mcnamara, cyril.jean, conor.dooley
From: Conor Dooley <conor.dooley@microchip.com>
This patchset adds support for the Microchip PolarFire clkcfg
hardware block.
Changes since v5:
* Dropped clock-output-names property
Major changes since v4:
* Adjusted license for microchip,mpfs-clock.h to match microchip,mpfs.yaml
* Corrected the number of clocks to 33 from 32
Major changes since v3:
* Patch reformatted so microchip,mpfs-clock.h is part of device-tree patch
Major changes since v2:
* In mpfs_cfg_clk_set_rate, return immediately if divider_get_val
returns <0
* rebased to v5.12-rc1
Major changes since v1:
* Dependency on SOC_MICROCHIP_POLARFIRE
* All references to PFSOC/pfsoc changed to MPFS/mpfs
* Cleaned error handling in _probe
* Re-ordered code to place structs et al at top
Daire McNamara (2):
dt-bindings: clk: microchip: Add Microchip PolarFire host binding
clk: microchip: Add driver for Microchip PolarFire SoC
.../bindings/clock/microchip,mpfs.yaml | 58 +++
drivers/clk/Kconfig | 1 +
drivers/clk/Makefile | 2 +-
drivers/clk/microchip/Kconfig | 7 +
drivers/clk/microchip/Makefile | 6 +-
drivers/clk/microchip/clk-mpfs.c | 444 ++++++++++++++++++
.../dt-bindings/clock/microchip,mpfs-clock.h | 45 ++
7 files changed, 560 insertions(+), 3 deletions(-)
create mode 100644 Documentation/devicetree/bindings/clock/microchip,mpfs.yaml
create mode 100644 drivers/clk/microchip/Kconfig
create mode 100644 drivers/clk/microchip/clk-mpfs.c
create mode 100644 include/dt-bindings/clock/microchip,mpfs-clock.h
--
2.33.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v6 1/2] dt-bindings: clk: microchip: Add Microchip PolarFire host binding
2021-11-30 14:07 [PATCH v6 0/2] CLK: microchip: Add clkcfg driver for Microchip PolarFire SoC conor.dooley
@ 2021-11-30 14:07 ` conor.dooley
2021-12-06 15:12 ` Geert Uytterhoeven
2021-11-30 14:07 ` [PATCH v6 2/2] clk: microchip: Add driver for Microchip PolarFire SoC conor.dooley
1 sibling, 1 reply; 7+ messages in thread
From: conor.dooley @ 2021-11-30 14:07 UTC (permalink / raw)
To: mturquette, sboyd, linux-clk, robh+dt, devicetree
Cc: krzysztof.kozlowski, geert, david.abdurachmanov, palmer,
daire.mcnamara, cyril.jean, conor.dooley, Rob Herring
From: Daire McNamara <daire.mcnamara@microchip.com>
Add device tree bindings for the Microchip PolarFire system
clock controller
Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
.../bindings/clock/microchip,mpfs.yaml | 58 +++++++++++++++++++
.../dt-bindings/clock/microchip,mpfs-clock.h | 45 ++++++++++++++
2 files changed, 103 insertions(+)
create mode 100644 Documentation/devicetree/bindings/clock/microchip,mpfs.yaml
create mode 100644 include/dt-bindings/clock/microchip,mpfs-clock.h
diff --git a/Documentation/devicetree/bindings/clock/microchip,mpfs.yaml b/Documentation/devicetree/bindings/clock/microchip,mpfs.yaml
new file mode 100644
index 000000000000..0c15afa2214c
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/microchip,mpfs.yaml
@@ -0,0 +1,58 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/microchip,mpfs.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip PolarFire Clock Control Module Binding
+
+maintainers:
+ - Daire McNamara <daire.mcnamara@microchip.com>
+
+description: |
+ Microchip PolarFire clock control (CLKCFG) is an integrated clock controller,
+ which gates and enables all peripheral clocks.
+
+ This device tree binding describes 33 gate clocks. Clocks are referenced by
+ user nodes by the CLKCFG node phandle and the clock index in the group, from
+ 0 to 32.
+
+properties:
+ compatible:
+ const: microchip,mpfs-clkcfg
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ maxItems: 1
+
+ '#clock-cells':
+ const: 1
+ description: |
+ The clock consumer should specify the desired clock by having the clock
+ ID in its "clocks" phandle cell. See include/dt-bindings/clock/microchip,mpfs-clock.h
+ for the full list of PolarFire clock IDs.
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - '#clock-cells'
+
+additionalProperties: false
+
+examples:
+ # Clock Config node:
+ - |
+ #include <dt-bindings/clock/microchip,mpfs-clock.h>
+ soc {
+ #address-cells = <2>;
+ #size-cells = <2>;
+ clkcfg: clock-controller@20002000 {
+ compatible = "microchip,mpfs-clkcfg";
+ reg = <0x0 0x20002000 0x0 0x1000>;
+ clocks = <&ref>;
+ #clock-cells = <1>;
+ };
+ };
diff --git a/include/dt-bindings/clock/microchip,mpfs-clock.h b/include/dt-bindings/clock/microchip,mpfs-clock.h
new file mode 100644
index 000000000000..73f2a9324857
--- /dev/null
+++ b/include/dt-bindings/clock/microchip,mpfs-clock.h
@@ -0,0 +1,45 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * Daire McNamara,<daire.mcnamara@microchip.com>
+ * Copyright (C) 2020 Microchip Technology Inc. All rights reserved.
+ */
+
+#ifndef _DT_BINDINGS_CLK_MICROCHIP_MPFS_H_
+#define _DT_BINDINGS_CLK_MICROCHIP_MPFS_H_
+
+#define CLK_CPU 0
+#define CLK_AXI 1
+#define CLK_AHB 2
+
+#define CLK_ENVM 3
+#define CLK_MAC0 4
+#define CLK_MAC1 5
+#define CLK_MMC 6
+#define CLK_TIMER 7
+#define CLK_MMUART0 8
+#define CLK_MMUART1 9
+#define CLK_MMUART2 10
+#define CLK_MMUART3 11
+#define CLK_MMUART4 12
+#define CLK_SPI0 13
+#define CLK_SPI1 14
+#define CLK_I2C0 15
+#define CLK_I2C1 16
+#define CLK_CAN0 17
+#define CLK_CAN1 18
+#define CLK_USB 19
+#define CLK_RESERVED 20
+#define CLK_RTC 21
+#define CLK_QSPI 22
+#define CLK_GPIO0 23
+#define CLK_GPIO1 24
+#define CLK_GPIO2 25
+#define CLK_DDRC 26
+#define CLK_FIC0 27
+#define CLK_FIC1 28
+#define CLK_FIC2 29
+#define CLK_FIC3 30
+#define CLK_ATHENA 31
+#define CLK_CFM 32
+
+#endif /* _DT_BINDINGS_CLK_MICROCHIP_MPFS_H_ */
--
2.33.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v6 2/2] clk: microchip: Add driver for Microchip PolarFire SoC
2021-11-30 14:07 [PATCH v6 0/2] CLK: microchip: Add clkcfg driver for Microchip PolarFire SoC conor.dooley
2021-11-30 14:07 ` [PATCH v6 1/2] dt-bindings: clk: microchip: Add Microchip PolarFire host binding conor.dooley
@ 2021-11-30 14:07 ` conor.dooley
2021-12-06 15:53 ` Geert Uytterhoeven
1 sibling, 1 reply; 7+ messages in thread
From: conor.dooley @ 2021-11-30 14:07 UTC (permalink / raw)
To: mturquette, sboyd, linux-clk, robh+dt, devicetree
Cc: krzysztof.kozlowski, geert, david.abdurachmanov, palmer,
daire.mcnamara, cyril.jean, conor.dooley
From: Daire McNamara <daire.mcnamara@microchip.com>
Add support for clock configuration on Microchip PolarFire SoC
Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
drivers/clk/Kconfig | 1 +
drivers/clk/Makefile | 2 +-
drivers/clk/microchip/Kconfig | 7 +
drivers/clk/microchip/Makefile | 6 +-
drivers/clk/microchip/clk-mpfs.c | 444 +++++++++++++++++++++++++++++++
5 files changed, 457 insertions(+), 3 deletions(-)
create mode 100644 drivers/clk/microchip/Kconfig
create mode 100644 drivers/clk/microchip/clk-mpfs.c
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index c5b3dc97396a..92d112a40269 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -401,6 +401,7 @@ source "drivers/clk/ingenic/Kconfig"
source "drivers/clk/keystone/Kconfig"
source "drivers/clk/mediatek/Kconfig"
source "drivers/clk/meson/Kconfig"
+source "drivers/clk/microchip/Kconfig"
source "drivers/clk/mstar/Kconfig"
source "drivers/clk/mvebu/Kconfig"
source "drivers/clk/pistachio/Kconfig"
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index e42312121e51..b60471a6061f 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -89,7 +89,7 @@ obj-$(CONFIG_ARCH_KEYSTONE) += keystone/
obj-$(CONFIG_MACH_LOONGSON32) += loongson1/
obj-y += mediatek/
obj-$(CONFIG_ARCH_MESON) += meson/
-obj-$(CONFIG_MACH_PIC32) += microchip/
+obj-y += microchip/
ifeq ($(CONFIG_COMMON_CLK), y)
obj-$(CONFIG_ARCH_MMP) += mmp/
endif
diff --git a/drivers/clk/microchip/Kconfig b/drivers/clk/microchip/Kconfig
new file mode 100644
index 000000000000..f5edc7b3c07c
--- /dev/null
+++ b/drivers/clk/microchip/Kconfig
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+
+config MCHP_CLK_MPFS
+ bool "Clk driver for PolarFire SoC"
+ depends on (RISCV && SOC_MICROCHIP_POLARFIRE) || COMPILE_TEST
+ help
+ Supports Clock Configuration for PolarFire SoC
diff --git a/drivers/clk/microchip/Makefile b/drivers/clk/microchip/Makefile
index f34b247e870f..0dce0b12eac4 100644
--- a/drivers/clk/microchip/Makefile
+++ b/drivers/clk/microchip/Makefile
@@ -1,3 +1,5 @@
# SPDX-License-Identifier: GPL-2.0-only
-obj-$(CONFIG_COMMON_CLK_PIC32) += clk-core.o
-obj-$(CONFIG_PIC32MZDA) += clk-pic32mzda.o
+
+obj-$(CONFIG_COMMON_CLK_PIC32) += clk-core.o
+obj-$(CONFIG_PIC32MZDA) += clk-pic32mzda.o
+obj-$(CONFIG_MCHP_CLK_MPFS) += clk-mpfs.o
diff --git a/drivers/clk/microchip/clk-mpfs.c b/drivers/clk/microchip/clk-mpfs.c
new file mode 100644
index 000000000000..541f88181877
--- /dev/null
+++ b/drivers/clk/microchip/clk-mpfs.c
@@ -0,0 +1,444 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Daire McNamara,<daire.mcnamara@microchip.com>
+ * Copyright (C) 2020 Microchip Technology Inc. All rights reserved.
+ */
+#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>
+
+/* address offset of control registers */
+#define REG_CLOCK_CONFIG_CR 0x08u
+#define REG_SUBBLK_CLOCK_CR 0x84u
+#define REG_SUBBLK_RESET_CR 0x88u
+
+struct mpfs_clock_data {
+ void __iomem *base;
+ struct clk_hw_onecell_data hw_data;
+};
+
+struct mpfs_cfg_clock {
+ unsigned int id;
+ const char *name;
+ u8 shift;
+ u8 width;
+ const struct clk_div_table *table;
+ unsigned long flags;
+};
+
+struct mpfs_cfg_hw_clock {
+ struct mpfs_cfg_clock cfg;
+ void __iomem *sys_base;
+ /* lock is used to prevent multiple writes */
+ spinlock_t *lock;
+ struct clk_hw hw;
+ struct clk_init_data init;
+};
+
+#define to_mpfs_cfg_clk(_hw) container_of(_hw, struct mpfs_cfg_hw_clock, hw)
+
+struct mpfs_periph_clock {
+ unsigned int id;
+ const char *name;
+ u8 shift;
+ unsigned long flags;
+};
+
+struct mpfs_periph_hw_clock {
+ struct mpfs_periph_clock periph;
+ void __iomem *sys_base;
+ /* lock is used to prevent multiple writes */
+ spinlock_t *lock;
+ struct clk_hw hw;
+};
+
+#define to_mpfs_periph_clk(_hw) container_of(_hw, struct mpfs_periph_hw_clock, hw)
+
+/*
+ * mpfs_clk_lock prevents anything else from writing to the
+ * mpfs clk block while a software locked register is being written.
+ */
+static DEFINE_SPINLOCK(mpfs_clk_lock);
+
+static struct clk_parent_data mpfs_cfg_parent[] = {
+ { .fw_name = "msspllclk", .name = "msspllclk" },
+};
+
+static const struct clk_div_table mpfs_div_cpu_axi_table[] = {
+ { 0, 1 }, { 1, 2 }, { 2, 4 }, { 3, 8 },
+ { 0, 0 }
+};
+
+static const struct clk_div_table mpfs_div_ahb_table[] = {
+ { 1, 2 }, { 2, 4}, { 3, 8 },
+ { 0, 0 }
+};
+
+static unsigned long mpfs_cfg_clk_recalc_rate(struct clk_hw *hw, unsigned long prate)
+{
+ struct mpfs_cfg_hw_clock *cfg_hw = to_mpfs_cfg_clk(hw);
+ struct mpfs_cfg_clock *cfg = &cfg_hw->cfg;
+ void __iomem *base_addr = cfg_hw->sys_base;
+ unsigned long rate;
+ u32 val;
+
+ val = readl_relaxed(base_addr + REG_CLOCK_CONFIG_CR) >> cfg->shift;
+ val &= clk_div_mask(cfg->width);
+ rate = prate / (1u << val);
+
+ return rate;
+}
+
+static long mpfs_cfg_clk_round_rate(struct clk_hw *hw, unsigned long rate, unsigned long *prate)
+{
+ struct mpfs_cfg_hw_clock *cfg_hw = to_mpfs_cfg_clk(hw);
+ struct mpfs_cfg_clock *cfg = &cfg_hw->cfg;
+
+ return divider_round_rate(hw, rate, prate, cfg->table, cfg->width, cfg->flags);
+}
+
+static int mpfs_cfg_clk_set_rate(struct clk_hw *hw, unsigned long rate, unsigned long prate)
+{
+ struct mpfs_cfg_hw_clock *cfg_hw = to_mpfs_cfg_clk(hw);
+ struct mpfs_cfg_clock *cfg = &cfg_hw->cfg;
+ void __iomem *base_addr = cfg_hw->sys_base;
+ unsigned long flags = 0;
+ u32 val;
+ int divider_setting;
+
+ divider_setting = divider_get_val(rate, prate, cfg->table, cfg->width, cfg_hw->cfg.flags);
+
+ if (divider_setting < 0)
+ return divider_setting;
+
+ if (cfg_hw->lock)
+ spin_lock_irqsave(cfg_hw->lock, flags);
+ else
+ __acquire(cfg_hw->lock);
+
+ val = readl_relaxed(base_addr + REG_CLOCK_CONFIG_CR);
+ val &= ~(clk_div_mask(cfg->width) << cfg_hw->cfg.shift);
+ val |= divider_setting << cfg->shift;
+ writel_relaxed(val, base_addr + REG_CLOCK_CONFIG_CR);
+
+ if (cfg_hw->lock)
+ spin_unlock_irqrestore(cfg_hw->lock, flags);
+ else
+ __release(cfg_hw->lock);
+
+ return 0;
+}
+
+static const struct clk_ops mpfs_clk_cfg_ops = {
+ .recalc_rate = mpfs_cfg_clk_recalc_rate,
+ .round_rate = mpfs_cfg_clk_round_rate,
+ .set_rate = mpfs_cfg_clk_set_rate,
+};
+
+#define CLK_CFG(_id, _name, _parent, _shift, _width, _table, _flags) { \
+ .cfg.id = _id, \
+ .cfg.name = _name, \
+ .cfg.shift = _shift, \
+ .cfg.width = _width, \
+ .cfg.table = _table, \
+ .hw.init = CLK_HW_INIT_PARENTS_DATA(_name, _parent, &mpfs_clk_cfg_ops, \
+ _flags), \
+ }
+
+static struct mpfs_cfg_hw_clock mpfs_cfg_clks[] = {
+ CLK_CFG(CLK_CPU, "clk_cpu", mpfs_cfg_parent, 0, 2, mpfs_div_cpu_axi_table, 0),
+ CLK_CFG(CLK_AXI, "clk_axi", mpfs_cfg_parent, 2, 2, mpfs_div_cpu_axi_table, 0),
+ CLK_CFG(CLK_AHB, "clk_ahb", mpfs_cfg_parent, 4, 2, mpfs_div_ahb_table, 0),
+};
+
+static void mpfs_clk_unregister_cfg(struct device *dev, struct clk_hw *hw)
+{
+ struct mpfs_cfg_hw_clock *cfg_hw = to_mpfs_cfg_clk(hw);
+
+ devm_clk_hw_unregister(dev, hw);
+ kfree(cfg_hw);
+}
+
+static struct clk_hw *mpfs_clk_register_cfg(struct device *dev,
+ struct mpfs_cfg_hw_clock *cfg_hw,
+ void __iomem *sys_base)
+{
+ struct clk_hw *hw;
+ int err;
+
+ cfg_hw->sys_base = sys_base;
+ cfg_hw->lock = &mpfs_clk_lock;
+
+ hw = &cfg_hw->hw;
+ err = devm_clk_hw_register(dev, hw);
+ if (err)
+ return ERR_PTR(err);
+
+ return hw;
+}
+
+static int mpfs_clk_register_cfgs(struct device *dev, struct mpfs_cfg_hw_clock *cfg_hws,
+ int num_clks, struct mpfs_clock_data *data)
+{
+ struct clk_hw *hw;
+ void __iomem *sys_base = data->base;
+ unsigned int i, id;
+
+ for (i = 0; i < num_clks; i++) {
+ struct mpfs_cfg_hw_clock *cfg_hw = &cfg_hws[i];
+
+ hw = mpfs_clk_register_cfg(dev, cfg_hw, sys_base);
+ if (IS_ERR(hw)) {
+ dev_err(dev, "%s: failed to register clock %s\n", __func__,
+ cfg_hw->cfg.name);
+ goto err_clk;
+ }
+
+ id = cfg_hws[i].cfg.id;
+ data->hw_data.hws[id] = hw;
+ }
+
+ return 0;
+
+err_clk:
+ while (i--)
+ mpfs_clk_unregister_cfg(dev, data->hw_data.hws[cfg_hws[i].cfg.id]);
+
+ return PTR_ERR(hw);
+}
+
+static int mpfs_periph_clk_enable(struct clk_hw *hw)
+{
+ struct mpfs_periph_hw_clock *periph_hw = to_mpfs_periph_clk(hw);
+ struct mpfs_periph_clock *periph = &periph_hw->periph;
+ void __iomem *base_addr = periph_hw->sys_base;
+ u32 reg, val;
+
+ 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);
+
+ return 0;
+}
+
+static void mpfs_periph_clk_disable(struct clk_hw *hw)
+{
+ struct mpfs_periph_hw_clock *periph_hw = to_mpfs_periph_clk(hw);
+ struct mpfs_periph_clock *periph = &periph_hw->periph;
+ void __iomem *base_addr = periph_hw->sys_base;
+ u32 reg, val;
+
+ 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);
+}
+
+static int mpfs_periph_clk_is_enabled(struct clk_hw *hw)
+{
+ struct mpfs_periph_hw_clock *periph_hw = to_mpfs_periph_clk(hw);
+ struct mpfs_periph_clock *periph = &periph_hw->periph;
+ void __iomem *base_addr = periph_hw->sys_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;
+ }
+
+ return 0;
+}
+
+static unsigned long mpfs_periph_clk_recalc_rate(struct clk_hw *hw, unsigned long prate)
+{
+ return prate;
+}
+
+static const struct clk_ops mpfs_periph_clk_ops = {
+ .enable = mpfs_periph_clk_enable,
+ .disable = mpfs_periph_clk_disable,
+ .is_enabled = mpfs_periph_clk_is_enabled,
+ .recalc_rate = mpfs_periph_clk_recalc_rate,
+};
+
+#define CLK_PERIPH(_id, _name, _parent, _shift, _flags) { \
+ .periph.id = _id, \
+ .periph.name = _name, \
+ .periph.shift = _shift, \
+ .hw.init = CLK_HW_INIT_HW(_name, _parent, &mpfs_periph_clk_ops, \
+ _flags), \
+ }
+
+#define PARENT_CLK(PARENT) (&mpfs_cfg_clks[CLK_##PARENT].hw)
+
+static struct mpfs_periph_hw_clock mpfs_periph_clks[] = {
+ CLK_PERIPH(CLK_ENVM, "clk_periph_envm", PARENT_CLK(AHB), 0, CLK_IS_CRITICAL),
+ CLK_PERIPH(CLK_MAC0, "clk_periph_mac0", PARENT_CLK(AHB), 1, 0),
+ CLK_PERIPH(CLK_MAC1, "clk_periph_mac1", PARENT_CLK(AHB), 2, 0),
+ CLK_PERIPH(CLK_MMC, "clk_periph_mmc", PARENT_CLK(AHB), 3, 0),
+ CLK_PERIPH(CLK_TIMER, "clk_periph_timer", PARENT_CLK(AHB), 4, 0),
+ CLK_PERIPH(CLK_MMUART0, "clk_periph_mmuart0", PARENT_CLK(AHB), 5, CLK_IS_CRITICAL),
+ CLK_PERIPH(CLK_MMUART1, "clk_periph_mmuart1", PARENT_CLK(AHB), 6, 0),
+ CLK_PERIPH(CLK_MMUART2, "clk_periph_mmuart2", PARENT_CLK(AHB), 7, 0),
+ CLK_PERIPH(CLK_MMUART3, "clk_periph_mmuart3", PARENT_CLK(AHB), 8, 0),
+ CLK_PERIPH(CLK_MMUART4, "clk_periph_mmuart4", PARENT_CLK(AHB), 9, 0),
+ CLK_PERIPH(CLK_SPI0, "clk_periph_spi0", PARENT_CLK(AHB), 10, 0),
+ CLK_PERIPH(CLK_SPI1, "clk_periph_spi1", PARENT_CLK(AHB), 11, 0),
+ CLK_PERIPH(CLK_I2C0, "clk_periph_i2c0", PARENT_CLK(AHB), 12, 0),
+ CLK_PERIPH(CLK_I2C1, "clk_periph_i2c1", PARENT_CLK(AHB), 13, 0),
+ CLK_PERIPH(CLK_CAN0, "clk_periph_can0", PARENT_CLK(AHB), 14, 0),
+ CLK_PERIPH(CLK_CAN1, "clk_periph_can1", PARENT_CLK(AHB), 15, 0),
+ CLK_PERIPH(CLK_USB, "clk_periph_usb", PARENT_CLK(AHB), 16, 0),
+ CLK_PERIPH(CLK_RTC, "clk_periph_rtc", PARENT_CLK(AHB), 18, 0),
+ CLK_PERIPH(CLK_QSPI, "clk_periph_qspi", PARENT_CLK(AHB), 19, 0),
+ CLK_PERIPH(CLK_GPIO0, "clk_periph_gpio0", PARENT_CLK(AHB), 20, 0),
+ CLK_PERIPH(CLK_GPIO1, "clk_periph_gpio1", PARENT_CLK(AHB), 21, 0),
+ CLK_PERIPH(CLK_GPIO2, "clk_periph_gpio2", PARENT_CLK(AHB), 22, 0),
+ CLK_PERIPH(CLK_DDRC, "clk_periph_ddrc", PARENT_CLK(AHB), 23, CLK_IS_CRITICAL),
+ CLK_PERIPH(CLK_FIC0, "clk_periph_fic0", PARENT_CLK(AHB), 24, 0),
+ CLK_PERIPH(CLK_FIC1, "clk_periph_fic1", PARENT_CLK(AHB), 25, 0),
+ CLK_PERIPH(CLK_FIC2, "clk_periph_fic2", PARENT_CLK(AHB), 26, 0),
+ CLK_PERIPH(CLK_FIC3, "clk_periph_fic3", PARENT_CLK(AHB), 27, 0),
+ CLK_PERIPH(CLK_ATHENA, "clk_periph_athena", PARENT_CLK(AHB), 28, 0),
+ CLK_PERIPH(CLK_CFM, "clk_periph_cfm", PARENT_CLK(AHB), 29, 0),
+};
+
+static void mpfs_clk_unregister_periph(struct device *dev, struct clk_hw *hw)
+{
+ struct mpfs_periph_hw_clock *periph_hw = to_mpfs_periph_clk(hw);
+
+ devm_clk_hw_unregister(dev, hw);
+ kfree(periph_hw);
+}
+
+static struct clk_hw *mpfs_clk_register_periph(struct device *dev,
+ struct mpfs_periph_hw_clock *periph_hw,
+ void __iomem *sys_base)
+{
+ struct clk_hw *hw;
+ int err;
+
+ periph_hw->sys_base = sys_base;
+ periph_hw->lock = &mpfs_clk_lock;
+
+ hw = &periph_hw->hw;
+ err = devm_clk_hw_register(dev, hw);
+ if (err)
+ return ERR_PTR(err);
+
+ return hw;
+}
+
+static int mpfs_clk_register_periphs(struct device *dev, struct mpfs_periph_hw_clock *periph_hws,
+ int num_clks, struct mpfs_clock_data *data)
+{
+ struct clk_hw *hw;
+ void __iomem *sys_base = data->base;
+ unsigned int i, id;
+
+ for (i = 0; i < num_clks; i++) {
+ struct mpfs_periph_hw_clock *periph_hw = &periph_hws[i];
+
+ hw = mpfs_clk_register_periph(dev, periph_hw, sys_base);
+ if (IS_ERR(hw)) {
+ dev_err(dev, "%s: failed to register clock %s\n", __func__,
+ periph_hw->periph.name);
+ goto err_clk;
+ }
+
+ id = periph_hws[i].periph.id;
+ data->hw_data.hws[id] = hw;
+ }
+
+ return 0;
+
+err_clk:
+ while (i--)
+ mpfs_clk_unregister_periph(dev, data->hw_data.hws[periph_hws[i].periph.id]);
+
+ return PTR_ERR(hw);
+}
+
+static int mpfs_clk_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct mpfs_clock_data *clk_data;
+ struct resource *res;
+ int num_clks;
+ int ret;
+
+ num_clks = ARRAY_SIZE(mpfs_cfg_clks) + ARRAY_SIZE(mpfs_periph_clks);
+
+ clk_data = devm_kzalloc(dev, struct_size(clk_data, hw_data.hws, num_clks), GFP_KERNEL);
+ if (!clk_data)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ clk_data->base = devm_ioremap_resource(dev, res);
+ if (IS_ERR(clk_data->base))
+ return PTR_ERR(clk_data->base);
+
+ clk_data->hw_data.num = num_clks;
+
+ ret = mpfs_clk_register_cfgs(dev, mpfs_cfg_clks, ARRAY_SIZE(mpfs_cfg_clks), clk_data);
+ if (ret)
+ goto err_clk;
+
+ ret = mpfs_clk_register_periphs(dev, mpfs_periph_clks, ARRAY_SIZE(mpfs_periph_clks),
+ clk_data);
+ if (ret)
+ goto err_clk;
+
+ ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, &clk_data->hw_data);
+ if (ret)
+ goto err_clk;
+
+ dev_info(dev, "registered MPFS core clocks\n");
+ return ret;
+
+err_clk:
+ dev_err(dev, "failed to register MPFS core clocks\n");
+ return ret;
+}
+
+static const struct of_device_id mpfs_clk_of_match_table[] = {
+ { .compatible = "microchip,mpfs-clkcfg", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, mpfs_clk_match_table);
+
+static struct platform_driver mpfs_clk_driver = {
+ .probe = mpfs_clk_probe,
+ .driver = {
+ .name = "microchip-mpfs-clkcfg",
+ .of_match_table = mpfs_clk_of_match_table,
+ },
+};
+
+static int __init clk_mpfs_init(void)
+{
+ return platform_driver_register(&mpfs_clk_driver);
+}
+core_initcall(clk_mpfs_init);
+
+static void __exit clk_mpfs_exit(void)
+{
+ platform_driver_unregister(&mpfs_clk_driver);
+}
+module_exit(clk_mpfs_exit);
+
+MODULE_DESCRIPTION("Microchip PolarFire SoC Clock Driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:clk-mpfs");
--
2.33.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v6 1/2] dt-bindings: clk: microchip: Add Microchip PolarFire host binding
2021-11-30 14:07 ` [PATCH v6 1/2] dt-bindings: clk: microchip: Add Microchip PolarFire host binding conor.dooley
@ 2021-12-06 15:12 ` Geert Uytterhoeven
0 siblings, 0 replies; 7+ messages in thread
From: Geert Uytterhoeven @ 2021-12-06 15:12 UTC (permalink / raw)
To: Conor Dooley
Cc: Michael Turquette, Stephen Boyd, linux-clk, Rob Herring,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Krzysztof Kozlowski, david.abdurachmanov, Palmer Dabbelt,
daire.mcnamara, cyril.jean, Rob Herring
On Tue, Nov 30, 2021 at 3:06 PM <conor.dooley@microchip.com> wrote:
> From: Daire McNamara <daire.mcnamara@microchip.com>
>
> Add device tree bindings for the Microchip PolarFire system
> clock controller
>
> Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v6 2/2] clk: microchip: Add driver for Microchip PolarFire SoC
2021-11-30 14:07 ` [PATCH v6 2/2] clk: microchip: Add driver for Microchip PolarFire SoC conor.dooley
@ 2021-12-06 15:53 ` Geert Uytterhoeven
2021-12-08 15:29 ` Conor.Dooley
0 siblings, 1 reply; 7+ messages in thread
From: Geert Uytterhoeven @ 2021-12-06 15:53 UTC (permalink / raw)
To: Conor Dooley, daire.mcnamara
Cc: Michael Turquette, Stephen Boyd, linux-clk, Rob Herring,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Krzysztof Kozlowski, david.abdurachmanov, Palmer Dabbelt,
cyril.jean
Hi Conor, Daire,
On Tue, Nov 30, 2021 at 3:06 PM <conor.dooley@microchip.com> wrote:
> From: Daire McNamara <daire.mcnamara@microchip.com>
>
> Add support for clock configuration on Microchip PolarFire SoC
>
> Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
Thanks for your patch!
> --- /dev/null
> +++ b/drivers/clk/microchip/clk-mpfs.c
> @@ -0,0 +1,444 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Daire McNamara,<daire.mcnamara@microchip.com>
> + * Copyright (C) 2020 Microchip Technology Inc. All rights reserved.
> + */
> +#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>
> +
> +/* address offset of control registers */
> +#define REG_CLOCK_CONFIG_CR 0x08u
> +#define REG_SUBBLK_CLOCK_CR 0x84u
> +#define REG_SUBBLK_RESET_CR 0x88u
> +
> +struct mpfs_clock_data {
> + void __iomem *base;
> + struct clk_hw_onecell_data hw_data;
> +};
> +
> +struct mpfs_cfg_clock {
> + unsigned int id;
implicit gap of 4 bytes
> + const char *name;
> + u8 shift;
> + u8 width;
implicit gap of 6 bytes
> + const struct clk_div_table *table;
> + unsigned long flags;
Hence you better move id, shift, and width to the end.
> +};
> +
> +struct mpfs_cfg_hw_clock {
> + struct mpfs_cfg_clock cfg;
> + void __iomem *sys_base;
> + /* lock is used to prevent multiple writes */
> + spinlock_t *lock;
> + struct clk_hw hw;
> + struct clk_init_data init;
> +};
> +
> +#define to_mpfs_cfg_clk(_hw) container_of(_hw, struct mpfs_cfg_hw_clock, hw)
> +
> +struct mpfs_periph_clock {
> + unsigned int id;
implicit gap of 4 bytes
> + const char *name;
> + u8 shift;
implicit gap of 7 bytes
> + unsigned long flags;
> +};
> +
> +struct mpfs_periph_hw_clock {
> + struct mpfs_periph_clock periph;
> + void __iomem *sys_base;
> + /* lock is used to prevent multiple writes */
> + spinlock_t *lock;
This is not used?
> + struct clk_hw hw;
> +};
> +
> +#define to_mpfs_periph_clk(_hw) container_of(_hw, struct mpfs_periph_hw_clock, hw)
> +
> +/*
> + * mpfs_clk_lock prevents anything else from writing to the
> + * mpfs clk block while a software locked register is being written.
> + */
> +static DEFINE_SPINLOCK(mpfs_clk_lock);
> +
> +static struct clk_parent_data mpfs_cfg_parent[] = {
> + { .fw_name = "msspllclk", .name = "msspllclk" },
So you rely on the name of the fixed-clock reference clock? That is
very fragile. Please instead use devm_clk_get() to get the reference
clock from the "clocks" property of the clock-controller node.
> +static unsigned long mpfs_cfg_clk_recalc_rate(struct clk_hw *hw, unsigned long prate)
> +{
> + struct mpfs_cfg_hw_clock *cfg_hw = to_mpfs_cfg_clk(hw);
> + struct mpfs_cfg_clock *cfg = &cfg_hw->cfg;
> + void __iomem *base_addr = cfg_hw->sys_base;
> + unsigned long rate;
> + u32 val;
> +
> + val = readl_relaxed(base_addr + REG_CLOCK_CONFIG_CR) >> cfg->shift;
> + val &= clk_div_mask(cfg->width);
> + rate = prate / (1u << val);
> +
> + return rate;
return prate / (1u << val);
> +}
> +static int mpfs_cfg_clk_set_rate(struct clk_hw *hw, unsigned long rate, unsigned long prate)
> +{
> + struct mpfs_cfg_hw_clock *cfg_hw = to_mpfs_cfg_clk(hw);
> + struct mpfs_cfg_clock *cfg = &cfg_hw->cfg;
> + void __iomem *base_addr = cfg_hw->sys_base;
> + unsigned long flags = 0;
> + u32 val;
> + int divider_setting;
> +
> + divider_setting = divider_get_val(rate, prate, cfg->table, cfg->width, cfg_hw->cfg.flags);
> +
> + if (divider_setting < 0)
> + return divider_setting;
> +
> + if (cfg_hw->lock)
Isn't this branch always taken?
> + spin_lock_irqsave(cfg_hw->lock, flags);
> + else
> + __acquire(cfg_hw->lock);
> +
> + val = readl_relaxed(base_addr + REG_CLOCK_CONFIG_CR);
> + val &= ~(clk_div_mask(cfg->width) << cfg_hw->cfg.shift);
> + val |= divider_setting << cfg->shift;
> + writel_relaxed(val, base_addr + REG_CLOCK_CONFIG_CR);
> +
> + if (cfg_hw->lock)
> + spin_unlock_irqrestore(cfg_hw->lock, flags);
> + else
> + __release(cfg_hw->lock);
> +
> + return 0;
> +}
> +static void mpfs_clk_unregister_cfg(struct device *dev, struct clk_hw *hw)
> +{
> + struct mpfs_cfg_hw_clock *cfg_hw = to_mpfs_cfg_clk(hw);
> +
> + devm_clk_hw_unregister(dev, hw);
> + kfree(cfg_hw);
This is freeing a part of the big array allocated with devm_kzalloc()?
> +}
> +
> +static struct clk_hw *mpfs_clk_register_cfg(struct device *dev,
> + struct mpfs_cfg_hw_clock *cfg_hw,
> + void __iomem *sys_base)
> +{
> + struct clk_hw *hw;
> + int err;
> +
> + cfg_hw->sys_base = sys_base;
> + cfg_hw->lock = &mpfs_clk_lock;
> +
> + hw = &cfg_hw->hw;
> + err = devm_clk_hw_register(dev, hw);
> + if (err)
> + return ERR_PTR(err);
> +
> + return hw;
> +}
> +
> +static int mpfs_clk_register_cfgs(struct device *dev, struct mpfs_cfg_hw_clock *cfg_hws,
> + int num_clks, struct mpfs_clock_data *data)
unsigned int num_clks
> +{
> + struct clk_hw *hw;
> + void __iomem *sys_base = data->base;
> + unsigned int i, id;
> +
> + for (i = 0; i < num_clks; i++) {
> + struct mpfs_cfg_hw_clock *cfg_hw = &cfg_hws[i];
> +
> + hw = mpfs_clk_register_cfg(dev, cfg_hw, sys_base);
> + if (IS_ERR(hw)) {
> + dev_err(dev, "%s: failed to register clock %s\n", __func__,
I guess the __func__ can be dropped, as the clock name is unique?
> + cfg_hw->cfg.name);
> + goto err_clk;
> + }
> +
> + id = cfg_hws[i].cfg.id;
> + data->hw_data.hws[id] = hw;
> + }
> +
> + return 0;
> +
> +err_clk:
> + while (i--)
> + mpfs_clk_unregister_cfg(dev, data->hw_data.hws[cfg_hws[i].cfg.id]);
> +
> + return PTR_ERR(hw);
> +}
> +static void mpfs_clk_unregister_periph(struct device *dev, struct clk_hw *hw)
> +{
> + struct mpfs_periph_hw_clock *periph_hw = to_mpfs_periph_clk(hw);
> +
> + devm_clk_hw_unregister(dev, hw);
> + kfree(periph_hw);'
This is freeing a part of the big array allocated with devm_kzalloc()?
> +}
> +static int mpfs_clk_register_periphs(struct device *dev, struct mpfs_periph_hw_clock *periph_hws,
> + int num_clks, struct mpfs_clock_data *data)
> +{
> + struct clk_hw *hw;
> + void __iomem *sys_base = data->base;
> + unsigned int i, id;
> +
> + for (i = 0; i < num_clks; i++) {
> + struct mpfs_periph_hw_clock *periph_hw = &periph_hws[i];
> +
> + hw = mpfs_clk_register_periph(dev, periph_hw, sys_base);
> + if (IS_ERR(hw)) {
> + dev_err(dev, "%s: failed to register clock %s\n", __func__,
I guess the __func__ can be dropped, as the clock name is unique?
> + periph_hw->periph.name);
> + goto err_clk;
> + }
> +
> + id = periph_hws[i].periph.id;
> + data->hw_data.hws[id] = hw;
> + }
> +
> + return 0;
> +
> +err_clk:
> + while (i--)
> + mpfs_clk_unregister_periph(dev, data->hw_data.hws[periph_hws[i].periph.id]);
> +
> + return PTR_ERR(hw);
> +}
> +
> +static int mpfs_clk_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct mpfs_clock_data *clk_data;
> + struct resource *res;
> + int num_clks;
unsigned int
> + int ret;
> +
> + num_clks = ARRAY_SIZE(mpfs_cfg_clks) + ARRAY_SIZE(mpfs_periph_clks);
As mpfs_periph_clks[] lacks an entry for CLK_RESERVED, num_clks is
one too small...
> +
> + clk_data = devm_kzalloc(dev, struct_size(clk_data, hw_data.hws, num_clks), GFP_KERNEL);
> + if (!clk_data)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + clk_data->base = devm_ioremap_resource(dev, res);
> + if (IS_ERR(clk_data->base))
> + return PTR_ERR(clk_data->base);
> +
> + clk_data->hw_data.num = num_clks;
> +
> + ret = mpfs_clk_register_cfgs(dev, mpfs_cfg_clks, ARRAY_SIZE(mpfs_cfg_clks), clk_data);
> + if (ret)
> + goto err_clk;
> +
> + ret = mpfs_clk_register_periphs(dev, mpfs_periph_clks, ARRAY_SIZE(mpfs_periph_clks),
> + clk_data);
... and initializing CLK_CFM will write beyond the end of clk_data[].
> + if (ret)
> + goto err_clk;
> +
> + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, &clk_data->hw_data);
> + if (ret)
> + goto err_clk;
> +
> + dev_info(dev, "registered MPFS core clocks\n");
> + return ret;
> +
> +err_clk:
> + dev_err(dev, "failed to register MPFS core clocks\n");
> + return ret;
> +}
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v6 2/2] clk: microchip: Add driver for Microchip PolarFire SoC
2021-12-06 15:53 ` Geert Uytterhoeven
@ 2021-12-08 15:29 ` Conor.Dooley
2021-12-08 18:28 ` Geert Uytterhoeven
0 siblings, 1 reply; 7+ messages in thread
From: Conor.Dooley @ 2021-12-08 15:29 UTC (permalink / raw)
To: geert, Daire.McNamara
Cc: mturquette, sboyd, linux-clk, robh+dt, devicetree,
krzysztof.kozlowski, david.abdurachmanov, palmer, Cyril.Jean
Hi Geert,
Just going to ack the other items you raised for now, but curious about
this one:
On 06/12/2021 15:53, Geert Uytterhoeven wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>> +static void mpfs_clk_unregister_cfg(struct device *dev, struct clk_hw *hw)
>> +{
>> + struct mpfs_cfg_hw_clock *cfg_hw = to_mpfs_cfg_clk(hw);
>> +
>> + devm_clk_hw_unregister(dev, hw);
>> + kfree(cfg_hw);
>
> This is freeing a part of the big array allocated with devm_kzalloc()?
I took a look at this, and I don't think it is freeing the devm
allocated array.
To me, what is actually being freed is an element in the array of
structs passed to mpfs_clk_register_cfgs in the probe function.
However, this struct is statically defined - so its elements shouldn't
be freed at all?
drivers/clk/clk-bm1880.c has the same behaviour in the unregister
function, where it calls kfree on the elements of a static array of clk
structs. So if my understanding is correct it would need fixing there too.
Cheers,
Conor.
>> +}
>> +
>> +static struct clk_hw *mpfs_clk_register_cfg(struct device *dev,
>> + struct mpfs_cfg_hw_clock *cfg_hw,
>> + void __iomem *sys_base)
>> +{
>> + struct clk_hw *hw;
>> + int err;
>> +
>> + cfg_hw->sys_base = sys_base;
>> + cfg_hw->lock = &mpfs_clk_lock;
>> +
>> + hw = &cfg_hw->hw;
>> + err = devm_clk_hw_register(dev, hw);
>> + if (err)
>> + return ERR_PTR(err);
>> +
>> + return hw;
>> +}
>> +
>> +static int mpfs_clk_register_cfgs(struct device *dev, struct mpfs_cfg_hw_clock *cfg_hws,
>> + int num_clks, struct mpfs_clock_data *data)
>
> unsigned int num_clks
>
>> +{
>> + struct clk_hw *hw;
>> + void __iomem *sys_base = data->base;
>> + unsigned int i, id;
>> +
>> + for (i = 0; i < num_clks; i++) {
>> + struct mpfs_cfg_hw_clock *cfg_hw = &cfg_hws[i];
>> +
>> + hw = mpfs_clk_register_cfg(dev, cfg_hw, sys_base);
>> + if (IS_ERR(hw)) {
>> + dev_err(dev, "%s: failed to register clock %s\n", __func__,
>
> I guess the __func__ can be dropped, as the clock name is unique?
>
>> + cfg_hw->cfg.name);
>> + goto err_clk;
>> + }
>> +
>> + id = cfg_hws[i].cfg.id;
>> + data->hw_data.hws[id] = hw;
>> + }
>> +
>> + return 0;
>> +
>> +err_clk:
>> + while (i--)
>> + mpfs_clk_unregister_cfg(dev, data->hw_data.hws[cfg_hws[i].cfg.id]);
>> +
>> + return PTR_ERR(hw);
>> +}
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v6 2/2] clk: microchip: Add driver for Microchip PolarFire SoC
2021-12-08 15:29 ` Conor.Dooley
@ 2021-12-08 18:28 ` Geert Uytterhoeven
0 siblings, 0 replies; 7+ messages in thread
From: Geert Uytterhoeven @ 2021-12-08 18:28 UTC (permalink / raw)
To: Conor Dooley
Cc: Daire.McNamara, Michael Turquette, Stephen Boyd, linux-clk,
Rob Herring,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Krzysztof Kozlowski, david.abdurachmanov, Palmer Dabbelt,
Cyril.Jean
Hi Conor,
On Wed, Dec 8, 2021 at 4:30 PM <Conor.Dooley@microchip.com> wrote:
> Just going to ack the other items you raised for now, but curious about
> this one:
> On 06/12/2021 15:53, Geert Uytterhoeven wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >> +static void mpfs_clk_unregister_cfg(struct device *dev, struct clk_hw *hw)
> >> +{
> >> + struct mpfs_cfg_hw_clock *cfg_hw = to_mpfs_cfg_clk(hw);
> >> +
> >> + devm_clk_hw_unregister(dev, hw);
> >> + kfree(cfg_hw);
> >
> > This is freeing a part of the big array allocated with devm_kzalloc()?
> I took a look at this, and I don't think it is freeing the devm
> allocated array.
>
> To me, what is actually being freed is an element in the array of
> structs passed to mpfs_clk_register_cfgs in the probe function.
> However, this struct is statically defined - so its elements shouldn't
> be freed at all?
You're right, it's part of the static array. I was misled by the
back-and-forth conversions between the different types.
Regardless, the kfree() calls should be removed.
> drivers/clk/clk-bm1880.c has the same behaviour in the unregister
> function, where it calls kfree on the elements of a static array of clk
> structs. So if my understanding is correct it would need fixing there too.
If that is true (didn't check), it needs to be fixed, too.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-12-08 18:28 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-30 14:07 [PATCH v6 0/2] CLK: microchip: Add clkcfg driver for Microchip PolarFire SoC conor.dooley
2021-11-30 14:07 ` [PATCH v6 1/2] dt-bindings: clk: microchip: Add Microchip PolarFire host binding conor.dooley
2021-12-06 15:12 ` Geert Uytterhoeven
2021-11-30 14:07 ` [PATCH v6 2/2] clk: microchip: Add driver for Microchip PolarFire SoC conor.dooley
2021-12-06 15:53 ` Geert Uytterhoeven
2021-12-08 15:29 ` Conor.Dooley
2021-12-08 18:28 ` Geert Uytterhoeven
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).