LinuxLists.cc - [PATCH 0/9] Add exynos_pmu_update/read/write() APIs to exynos-pmu

[permalink] [raw]

Subject: [PATCH 5/9] arm64: dts: exynosautov9: remove deprecated samsung,syscon-phandle

samsung,syscon-phandle is no longer used by the Samsung watchdog driver
to access PMU registers.

Signed-off-by: Peter Griffin <[email protected]>
---
arch/arm64/boot/dts/exynos/exynosautov9.dtsi | 2 --
1 file changed, 2 deletions(-)

diff --git a/arch/arm64/boot/dts/exynos/exynosautov9.dtsi b/arch/arm64/boot/dts/exynos/exynosautov9.dtsi
index c871a2f49fda..94c8d68fe92c 100644
--- a/arch/arm64/boot/dts/exynos/exynosautov9.dtsi
+++ b/arch/arm64/boot/dts/exynos/exynosautov9.dtsi
@@ -1551,7 +1551,6 @@ watchdog_cl0: watchdog@10050000 {
interrupts = <GIC_SPI 476 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&cmu_peris CLK_GOUT_WDT_CLUSTER0>, <&xtcxo>;
clock-names = "watchdog", "watchdog_src";
- samsung,syscon-phandle = <&pmu_system_controller>;
samsung,cluster-index = <0>;
};

@@ -1561,7 +1560,6 @@ watchdog_cl1: watchdog@10060000 {
interrupts = <GIC_SPI 475 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&cmu_peris CLK_GOUT_WDT_CLUSTER1>, <&xtcxo>;
clock-names = "watchdog", "watchdog_src";
- samsung,syscon-phandle = <&pmu_system_controller>;
samsung,cluster-index = <1>;
};

--
2.43.0.429.g432eaa2c6b-goog

2024-01-22 22:59:28

[permalink] [raw]

Subject: [PATCH 4/9] arm64: dts: fsd: remove deprecated samsung,syscon-phandle

samsung,syscon-phandle is no longer used by the Samsung watchdog driver
to access PMU registers.

Signed-off-by: Peter Griffin <[email protected]>
---
arch/arm64/boot/dts/tesla/fsd.dtsi | 3 ---
1 file changed, 3 deletions(-)

diff --git a/arch/arm64/boot/dts/tesla/fsd.dtsi b/arch/arm64/boot/dts/tesla/fsd.dtsi
index aaffb50b8b60..9b55e44c1db0 100644
--- a/arch/arm64/boot/dts/tesla/fsd.dtsi
+++ b/arch/arm64/boot/dts/tesla/fsd.dtsi
@@ -625,7 +625,6 @@ watchdog_0: watchdog@100a0000 {
compatible = "tesla,fsd-wdt", "samsung,exynos7-wdt";
reg = <0x0 0x100a0000 0x0 0x100>;
interrupts = <GIC_SPI 471 IRQ_TYPE_LEVEL_HIGH>;
- samsung,syscon-phandle = <&pmu_system_controller>;
clocks = <&fin_pll>;
clock-names = "watchdog";
};
@@ -634,7 +633,6 @@ watchdog_1: watchdog@100b0000 {
compatible = "tesla,fsd-wdt", "samsung,exynos7-wdt";
reg = <0x0 0x100b0000 0x0 0x100>;
interrupts = <GIC_SPI 472 IRQ_TYPE_LEVEL_HIGH>;
- samsung,syscon-phandle = <&pmu_system_controller>;
clocks = <&fin_pll>;
clock-names = "watchdog";
};
@@ -643,7 +641,6 @@ watchdog_2: watchdog@100c0000 {
compatible = "tesla,fsd-wdt", "samsung,exynos7-wdt";
reg = <0x0 0x100c0000 0x0 0x100>;
interrupts = <GIC_SPI 473 IRQ_TYPE_LEVEL_HIGH>;
- samsung,syscon-phandle = <&pmu_system_controller>;
clocks = <&fin_pll>;
clock-names = "watchdog";
};
--
2.43.0.429.g432eaa2c6b-goog

2024-01-22 22:59:35

[permalink] [raw]

Subject: [PATCH 7/9] arm64: dts: exynos7: remove deprecated samsung,syscon-phandle

2024-01-22 22:59:51

[permalink] [raw]

Subject: [PATCH 1/9] dt-bindings: watchdog: samsung-wdt: deprecate samsung,syscon-phandle

The watchdog driver no longer requires a phandle to obtain a regmap
to the PMU registers. So mark this as deprecated.

Signed-off-by: Peter Griffin <[email protected]>
---
.../devicetree/bindings/watchdog/samsung-wdt.yaml | 15 +--------------
1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml b/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml
index 77a5ddd0426e..3970d6bf8576 100644
--- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml
+++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml
@@ -56,6 +56,7 @@ properties:
description:
Phandle to the PMU system controller node (in case of Exynos5250,
Exynos5420, Exynos7, Exynos850 and gs101).
+ deprecated: true

required:
- compatible
@@ -66,20 +67,6 @@ required:

allOf:
- $ref: watchdog.yaml#
- - if:
- properties:
- compatible:
- contains:
- enum:
- - google,gs101-wdt
- - samsung,exynos5250-wdt
- - samsung,exynos5420-wdt
- - samsung,exynos7-wdt
- - samsung,exynos850-wdt
- - samsung,exynosautov9-wdt
- then:
- required:
- - samsung,syscon-phandle
- if:
properties:
compatible:
--
2.43.0.429.g432eaa2c6b-goog

2024-01-22 22:59:57

[permalink] [raw]

Subject: [PATCH 8/9] ARM: dts: samsung: exynos4: remove deprecated samsung,syscon-phandle

samsung,syscon-phandle is no longer used by the Samsung watchdog driver
to access PMU registers.

Signed-off-by: Peter Griffin <[email protected]>
---
arch/arm/boot/dts/samsung/exynos4x12.dtsi | 1 -
1 file changed, 1 deletion(-)

diff --git a/arch/arm/boot/dts/samsung/exynos4x12.dtsi b/arch/arm/boot/dts/samsung/exynos4x12.dtsi
index b4b5e769145b..0fea32616c89 100644
--- a/arch/arm/boot/dts/samsung/exynos4x12.dtsi
+++ b/arch/arm/boot/dts/samsung/exynos4x12.dtsi
@@ -311,7 +311,6 @@ watchdog: watchdog@10060000 {
interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&clock CLK_WDT>;
clock-names = "watchdog";
- samsung,syscon-phandle = <&pmu_system_controller>;
};

adc: adc@126c0000 {
--
2.43.0.429.g432eaa2c6b-goog

2024-01-22 23:00:04

[permalink] [raw]

Subject: [PATCH 6/9] arm64: dts: exynos850: remove deprecated samsung,syscon-phandle

samsung,syscon-phandle is no longer used by the Samsung watchdog driver
to access PMU registers and is deprecated.

Signed-off-by: Peter Griffin <[email protected]>
---
arch/arm64/boot/dts/exynos/exynos850.dtsi | 2 --
1 file changed, 2 deletions(-)

diff --git a/arch/arm64/boot/dts/exynos/exynos850.dtsi b/arch/arm64/boot/dts/exynos/exynos850.dtsi
index da3f4a791e68..6d4789c77a1c 100644
--- a/arch/arm64/boot/dts/exynos/exynos850.dtsi
+++ b/arch/arm64/boot/dts/exynos/exynos850.dtsi
@@ -216,7 +216,6 @@ watchdog_cl0: watchdog@10050000 {
interrupts = <GIC_SPI 228 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&cmu_peri CLK_GOUT_WDT0_PCLK>, <&oscclk>;
clock-names = "watchdog", "watchdog_src";
- samsung,syscon-phandle = <&pmu_system_controller>;
samsung,cluster-index = <0>;
status = "disabled";
};
@@ -227,7 +226,6 @@ watchdog_cl1: watchdog@10060000 {
interrupts = <GIC_SPI 229 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&cmu_peri CLK_GOUT_WDT1_PCLK>, <&oscclk>;
clock-names = "watchdog", "watchdog_src";
- samsung,syscon-phandle = <&pmu_system_controller>;
samsung,cluster-index = <1>;
status = "disabled";
};
--
2.43.0.429.g432eaa2c6b-goog

2024-01-22 23:00:08

[permalink] [raw]

Subject: [PATCH 9/9] ARM: dts: exynos5250: remove deprecated samsung,syscon-phandle

samsung,syscon-phandle is no longer used by the Samsung watchdog driver
to access PMU registers.

Signed-off-by: Peter Griffin <[email protected]>
---
arch/arm/boot/dts/samsung/exynos5250.dtsi | 1 -
1 file changed, 1 deletion(-)

diff --git a/arch/arm/boot/dts/samsung/exynos5250.dtsi b/arch/arm/boot/dts/samsung/exynos5250.dtsi
index 99c84bebf25a..2bbeb0f0d898 100644
--- a/arch/arm/boot/dts/samsung/exynos5250.dtsi
+++ b/arch/arm/boot/dts/samsung/exynos5250.dtsi
@@ -312,7 +312,6 @@ watchdog@101d0000 {
interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&clock CLK_WDT>;
clock-names = "watchdog";
- samsung,syscon-phandle = <&pmu_system_controller>;
};

mfc: codec@11000000 {
--
2.43.0.429.g432eaa2c6b-goog

2024-01-22 23:00:26

[permalink] [raw]

Subject: [PATCH 3/9] watchdog: s3c2410_wdt: update to use new exynos_pmu_*() apis

Instead of obtaining the PMU regmap directly use the new exynos_pmu_*()
APIs. The exynos_pmu_ APIs allow support of newer Exynos SoCs that have
atomic set/clear bit hardware and platforms where the PMU registers can
only be accessed via SMC call.

As all platforms that have PMU registers use these new APIs, remove the
syscon regmap lookup code, as it is now redundant.

Signed-off-by: Peter Griffin <[email protected]>
---
drivers/watchdog/Kconfig | 1 +
drivers/watchdog/s3c2410_wdt.c | 25 +++++++++----------------
2 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 7d22051b15a2..b3e90e1ddf14 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -513,6 +513,7 @@ config S3C2410_WATCHDOG
depends on ARCH_S3C64XX || ARCH_S5PV210 || ARCH_EXYNOS || COMPILE_TEST
select WATCHDOG_CORE
select MFD_SYSCON if ARCH_EXYNOS
+ select EXYNOS_PMU
help
Watchdog timer block in the Samsung S3C64xx, S5Pv210 and Exynos
SoCs. This will reboot the system when the timer expires with
diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 349d30462c8c..fd3a9ce870a0 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -28,6 +28,8 @@
#include <linux/regmap.h>
#include <linux/delay.h>

+#include <linux/soc/samsung/exynos-pmu.h>
+
#define S3C2410_WTCON 0x00
#define S3C2410_WTDAT 0x04
#define S3C2410_WTCNT 0x08
@@ -187,7 +189,6 @@ struct s3c2410_wdt {
struct watchdog_device wdt_device;
struct notifier_block freq_transition;
const struct s3c2410_wdt_variant *drv_data;
- struct regmap *pmureg;
};

static const struct s3c2410_wdt_variant drv_data_s3c2410 = {
@@ -355,8 +356,8 @@ static int s3c2410wdt_disable_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
const u32 val = mask ? mask_val : 0;
int ret;

- ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->disable_reg,
- mask_val, val);
+ ret = exynos_pmu_update(wdt->drv_data->disable_reg,
+ mask_val, val);
if (ret < 0)
dev_err(wdt->dev, "failed to update reg(%d)\n", ret);

@@ -370,8 +371,8 @@ static int s3c2410wdt_mask_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
const u32 val = (mask ^ val_inv) ? mask_val : 0;
int ret;

- ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->mask_reset_reg,
- mask_val, val);
+ ret = exynos_pmu_update(wdt->drv_data->mask_reset_reg,
+ mask_val, val);
if (ret < 0)
dev_err(wdt->dev, "failed to update reg(%d)\n", ret);

@@ -384,8 +385,8 @@ static int s3c2410wdt_enable_counter(struct s3c2410_wdt *wdt, bool en)
const u32 val = en ? mask_val : 0;
int ret;

- ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->cnt_en_reg,
- mask_val, val);
+ ret = exynos_pmu_update(wdt->drv_data->cnt_en_reg,
+ mask_val, val);
if (ret < 0)
dev_err(wdt->dev, "failed to update reg(%d)\n", ret);

@@ -617,7 +618,7 @@ static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt)
if (!(wdt->drv_data->quirks & QUIRK_HAS_PMU_RST_STAT))
return 0;

- ret = regmap_read(wdt->pmureg, wdt->drv_data->rst_stat_reg, &rst_stat);
+ ret = exynos_pmu_read(wdt->drv_data->rst_stat_reg, &rst_stat);
if (ret)
dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
@@ -698,14 +699,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
if (ret)
return ret;

- if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
- wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
- "samsung,syscon-phandle");
- if (IS_ERR(wdt->pmureg))
- return dev_err_probe(dev, PTR_ERR(wdt->pmureg),
- "syscon regmap lookup failed.\n");
- }
-
wdt_irq = platform_get_irq(pdev, 0);
if (wdt_irq < 0)
return wdt_irq;
--
2.43.0.429.g432eaa2c6b-goog

2024-01-22 23:00:44

[permalink] [raw]

Subject: [PATCH 2/9] soc: samsung: exynos-pmu: Add exynos_pmu_update/read/write APIs and SoC quirks

Newer Exynos SoCs have atomic set/clear bit hardware for PMU registers as
these registers can be accessed by multiple masters. Some platforms also
protect the PMU registers for security hardening reasons so they can't be
written by normal world and are only write acessible in el3 via a SMC call.

Add support for both of these usecases using SoC specific quirks that are
determined from the DT compatible string.

Drivers which need to read and write PMU registers should now use these
new exynos_pmu_*() APIs instead of obtaining a regmap using
syscon_regmap_lookup_by_phandle()

Depending on the SoC specific quirks, the exynos_pmu_*() APIs will access
the PMU register in the appropriate way.

Signed-off-by: Peter Griffin <[email protected]>
---
drivers/soc/samsung/exynos-pmu.c | 209 ++++++++++++++++++++++++-
drivers/soc/samsung/exynos-pmu.h | 4 +
include/linux/soc/samsung/exynos-pmu.h | 28 ++++
3 files changed, 234 insertions(+), 7 deletions(-)

diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
index 250537d7cfd6..e9e933ede568 100644
--- a/drivers/soc/samsung/exynos-pmu.c
+++ b/drivers/soc/samsung/exynos-pmu.c
@@ -5,6 +5,7 @@
//
// Exynos - CPU PMU(Power Management Unit) support

+#include <linux/arm-smccc.h>
#include <linux/of.h>
#include <linux/of_address.h>
#include <linux/mfd/core.h>
@@ -12,29 +13,204 @@
#include <linux/of_platform.h>
#include <linux/platform_device.h>
#include <linux/delay.h>
+#include <linux/regmap.h>

#include <linux/soc/samsung/exynos-regs-pmu.h>
#include <linux/soc/samsung/exynos-pmu.h>

#include "exynos-pmu.h"

+/**
+ * DOC: Quirk flags for different Exynos PMU IP-cores
+ *
+ * This driver supports multiple Exynos based SoCs, each of which might have a
+ * different set of registers and features supported.
+ *
+ * Quirk flags described below serve the purpose of telling the driver about
+ * mentioned SoC traits, and can be specified in driver data for each particular
+ * supported device.
+ *
+ * %QUIRK_HAS_ATOMIC_BITSETHW: PMU IP has special atomic bit set/clear HW
+ * to protect against PMU registers being accessed from multiple bus masters.
+ *
+ * %QUIRK_PMU_ALIVE_WRITE_SEC: PMU registers are *not* write accesible from
+ * normal world. This is found on some SoCs as a security hardening measure. PMU
+ * registers on these SoCs can only be written via a SMC call and registers are
+ * checked by EL3 firmware against an allowlist before the write can procede.
+ * Note: This quirk should only be set for platforms whose el3 firmware
+ * implements the TENSOR_SMC_PMU_SEC_REG interface below.
+ */
+
+#define QUIRK_HAS_ATOMIC_BITSETHW BIT(0)
+#define QUIRK_PMU_ALIVE_WRITE_SEC BIT(1)
+
+#define PMUALIVE_MASK GENMASK(14, 0)
+
struct exynos_pmu_context {
struct device *dev;
const struct exynos_pmu_data *pmu_data;
+ struct regmap *pmureg;
+ void __iomem *pmu_base_addr;
+ phys_addr_t pmu_base_pa;
+ /* protect PMU reg atomic update operations */
+ spinlock_t update_lock;
};

-void __iomem *pmu_base_addr;
static struct exynos_pmu_context *pmu_context;

+/*
+ * Some SoCs are configured so that PMU_ALIVE registers can only be written
+ * from el3. As Linux needs to write some of these registers, the following
+ * SMC register read/write/read,write,modify interface is used.
+ *
+ * Note: This SMC interface is known to be implemented on gs101 and derivative
+ * SoCs.
+ */
+#define TENSOR_SMC_PMU_SEC_REG (0x82000504)
+#define TENSOR_PMUREG_READ 0
+#define TENSOR_PMUREG_WRITE 1
+#define TENSOR_PMUREG_RMW 2
+
+int set_priv_reg(phys_addr_t reg, u32 val)
+{
+ struct arm_smccc_res res;
+
+ arm_smccc_smc(TENSOR_SMC_PMU_SEC_REG,
+ reg,
+ TENSOR_PMUREG_WRITE,
+ val, 0, 0, 0, 0, &res);
+
+ if (res.a0)
+ pr_warn("%s(): SMC failed: %lu\n", __func__, res.a0);
+
+ return (int)res.a0;
+}
+
+int rmw_priv_reg(phys_addr_t reg, u32 mask, u32 val)
+{
+ struct arm_smccc_res res;
+
+ arm_smccc_smc(TENSOR_SMC_PMU_SEC_REG,
+ reg,
+ TENSOR_PMUREG_RMW,
+ mask, val, 0, 0, 0, &res);
+
+ if (res.a0)
+ pr_warn("%s(): SMC failed: %lu\n", __func__, res.a0);
+
+ return (int)res.a0;
+}
+
+/*
+ * For SoCs that have set/clear bit hardware (as indicated by
+ * QUIRK_HAS_ATOMIC_BITSETHW) this function can be used when
+ * the PMU register will be accessed by multiple masters.
+ *
+ * For example, to set bits 13:8 in PMU reg offset 0x3e80
+ * exynos_pmu_set_bit_atomic(0x3e80, 0x3f00, 0x3f00);
+ *
+ * To clear bits 13:8 in PMU offset 0x3e80
+ * exynos_pmu_set_bit_atomic(0x3e80, 0x0, 0x3f00);
+ */
+static inline void exynos_pmu_set_bit_atomic(unsigned int offset,
+ u32 val, u32 mask)
+{
+ unsigned long flags;
+ unsigned int i;
+
+ spin_lock_irqsave(&pmu_context->update_lock, flags);
+ for (i = 0; i < 32; i++) {
+ if (mask & BIT(i)) {
+ if (val & BIT(i)) {
+ offset |= 0xc000;
+ pmu_raw_writel(i, offset);
+ } else {
+ offset |= 0x8000;
+ pmu_raw_writel(i, offset);
+ }
+ }
+ }
+ spin_unlock_irqrestore(&pmu_context->update_lock, flags);
+}
+
+int exynos_pmu_update_bits(unsigned int offset, unsigned int mask,
+ unsigned int val)
+{
+ if (pmu_context->pmu_data &&
+ pmu_context->pmu_data->quirks & QUIRK_PMU_ALIVE_WRITE_SEC)
+ return rmw_priv_reg(pmu_context->pmu_base_pa + offset,
+ mask, val);
+
+ return regmap_update_bits(pmu_context->pmureg, offset, mask, val);
+}
+EXPORT_SYMBOL(exynos_pmu_update_bits);
+
void pmu_raw_writel(u32 val, u32 offset)
{
- writel_relaxed(val, pmu_base_addr + offset);
+ if (pmu_context->pmu_data &&
+ pmu_context->pmu_data->quirks & QUIRK_PMU_ALIVE_WRITE_SEC)
+ return (void)set_priv_reg(pmu_context->pmu_base_pa + offset,
+ val);
+
+ return writel_relaxed(val, pmu_context->pmu_base_addr + offset);
}

u32 pmu_raw_readl(u32 offset)
{
- return readl_relaxed(pmu_base_addr + offset);
+ return readl_relaxed(pmu_context->pmu_base_addr + offset);
+}
+
+int exynos_pmu_read(unsigned int offset, unsigned int *val)
+{
+ if (!pmu_context)
+ return -ENODEV;
+
+ /*
+ * For platforms that protect PMU registers they
+ * are still accessible to read from normal world
+ */
+ return regmap_read(pmu_context->pmureg, offset, val);
+}
+EXPORT_SYMBOL(exynos_pmu_read);
+
+int exynos_pmu_write(unsigned int offset, unsigned int val)
+{
+ if (!pmu_context)
+ return -ENODEV;
+
+ if (pmu_context->pmu_data &&
+ pmu_context->pmu_data->quirks & QUIRK_PMU_ALIVE_WRITE_SEC)
+ return set_priv_reg(pmu_context->pmu_base_pa + offset, val);
+
+ return regmap_write(pmu_context->pmureg, offset, val);
+}
+EXPORT_SYMBOL(exynos_pmu_write);
+
+int exynos_pmu_update(unsigned int offset, unsigned int mask, unsigned int val)
+{
+ int ret = 0;
+
+ if (!pmu_context)
+ return -ENODEV;
+
+ if (pmu_context->pmu_data &&
+ pmu_context->pmu_data->quirks & QUIRK_HAS_ATOMIC_BITSETHW) {
+ /*
+ * Use atomic operations for PMU_ALIVE registers (offset 0~0x3FFF)
+ * as the target registers can be accessed by multiple masters.
+ */
+ if (offset > PMUALIVE_MASK)
+ return exynos_pmu_update_bits(offset, mask, val);
+
+ exynos_pmu_set_bit_atomic(offset, val, mask);
+
+ } else {
+ return exynos_pmu_update_bits(offset, mask, val);
+ }
+
+ return ret;
}
+EXPORT_SYMBOL(exynos_pmu_update);

void exynos_sys_powerdown_conf(enum sys_powerdown mode)
{
@@ -75,11 +251,18 @@ void exynos_sys_powerdown_conf(enum sys_powerdown mode)
#define exynos_pmu_data_arm_ptr(data) NULL
#endif

+static const struct exynos_pmu_data gs101_pmu_data = {
+ .quirks = QUIRK_HAS_ATOMIC_BITSETHW | QUIRK_PMU_ALIVE_WRITE_SEC,
+};
+
/*
* PMU platform driver and devicetree bindings.
*/
static const struct of_device_id exynos_pmu_of_device_ids[] = {
{
+ .compatible = "google,gs101-pmu",
+ .data = &gs101_pmu_data,
+ }, {
.compatible = "samsung,exynos3250-pmu",
.data = exynos_pmu_data_arm_ptr(exynos3250_pmu_data),
}, {
@@ -125,18 +308,30 @@ EXPORT_SYMBOL_GPL(exynos_get_pmu_regmap);

static int exynos_pmu_probe(struct platform_device *pdev)
{
+ struct resource *res;
struct device *dev = &pdev->dev;
int ret;

- pmu_base_addr = devm_platform_ioremap_resource(pdev, 0);
- if (IS_ERR(pmu_base_addr))
- return PTR_ERR(pmu_base_addr);
-
pmu_context = devm_kzalloc(&pdev->dev,
sizeof(struct exynos_pmu_context),
GFP_KERNEL);
if (!pmu_context)
return -ENOMEM;
+
+ pmu_context->pmu_base_addr = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(pmu_context->pmu_base_addr))
+ return PTR_ERR(pmu_context->pmu_base_addr);
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res)
+ return -ENODEV;
+
+ pmu_context->pmu_base_pa = res->start;
+ pmu_context->pmureg = exynos_get_pmu_regmap();
+ if (IS_ERR(pmu_context->pmureg))
+ return PTR_ERR(pmu_context->pmureg);
+
+ spin_lock_init(&pmu_context->update_lock);
pmu_context->dev = dev;
pmu_context->pmu_data = of_device_get_match_data(dev);

diff --git a/drivers/soc/samsung/exynos-pmu.h b/drivers/soc/samsung/exynos-pmu.h
index 1c652ffd79b4..570c6e4dc8c3 100644
--- a/drivers/soc/samsung/exynos-pmu.h
+++ b/drivers/soc/samsung/exynos-pmu.h
@@ -25,8 +25,12 @@ struct exynos_pmu_data {
void (*pmu_init)(void);
void (*powerdown_conf)(enum sys_powerdown);
void (*powerdown_conf_extra)(enum sys_powerdown);
+ u32 quirks;
};

+int set_priv_reg(phys_addr_t reg, u32 val);
+int rmw_priv_reg(phys_addr_t reg, u32 mask, u32 val);
+
extern void __iomem *pmu_base_addr;

#ifdef CONFIG_EXYNOS_PMU_ARM_DRIVERS
diff --git a/include/linux/soc/samsung/exynos-pmu.h b/include/linux/soc/samsung/exynos-pmu.h
index a4f5516cc956..2c5ce21fb00b 100644
--- a/include/linux/soc/samsung/exynos-pmu.h
+++ b/include/linux/soc/samsung/exynos-pmu.h
@@ -21,11 +21,39 @@ enum sys_powerdown {
extern void exynos_sys_powerdown_conf(enum sys_powerdown mode);
#ifdef CONFIG_EXYNOS_PMU
extern struct regmap *exynos_get_pmu_regmap(void);
+extern int exynos_pmu_update_bits(unsigned int offset, unsigned int mask,
+ unsigned int val);
+extern int exynos_pmu_update(unsigned int offset, unsigned int mask,
+ unsigned int val);
+extern int exynos_pmu_write(unsigned int offset, unsigned int val);
+extern int exynos_pmu_read(unsigned int offset, unsigned int *val);
#else
static inline struct regmap *exynos_get_pmu_regmap(void)
{
return ERR_PTR(-ENODEV);
}
+
+static inline int exynos_pmu_update_bits(unsigned int offset, unsigned int mask,
+ unsigned int val);
+{
+ return ERR_PTR(-ENODEV);
+}
+
+static inline int exynos_pmu_update(unsigned int offset, unsigned int mask,
+ unsigned int val);
+{
+ return ERR_PTR(-ENODEV);
+}
+
+static inline int exynos_pmu_write(unsigned int offset, unsigned int val)
+{
+ return ERR_PTR(-ENODEV);
+}
+
+static inline int exynos_pmu_read(unsigned int offset, unsigned int *val)
+{
+ return ERR_PTR(-ENODEV);
+}
#endif

#endif /* __LINUX_SOC_EXYNOS_PMU_H */
--
2.43.0.429.g432eaa2c6b-goog

2024-01-23 07:42:29

by Arnd Bergmann

[permalink] [raw]

Subject: Re: [PATCH 9/9] ARM: dts: exynos5250: remove deprecated samsung,syscon-phandle

On Mon, Jan 22, 2024, at 23:57, Peter Griffin wrote:
> samsung,syscon-phandle is no longer used by the Samsung watchdog driver
> to access PMU registers.
>
> Signed-off-by: Peter Griffin <[email protected]>

Can you mention the driver commit that led to the property no
longer being required? Since this change would break compatibility
with older kernels, I would want to make sure that at least a
couple of years have passed since then to give users an upgrade
path.

Arnd

2024-01-23 08:11:37

by Arnd Bergmann

[permalink] [raw]

Subject: Re: [PATCH 2/9] soc: samsung: exynos-pmu: Add exynos_pmu_update/read/write APIs and SoC quirks

On Mon, Jan 22, 2024, at 23:57, Peter Griffin wrote:

> --- a/include/linux/soc/samsung/exynos-pmu.h
> +++ b/include/linux/soc/samsung/exynos-pmu.h
> @@ -21,11 +21,39 @@ enum sys_powerdown {
> extern void exynos_sys_powerdown_conf(enum sys_powerdown mode);
> #ifdef CONFIG_EXYNOS_PMU
> extern struct regmap *exynos_get_pmu_regmap(void);
> +extern int exynos_pmu_update_bits(unsigned int offset, unsigned int
> mask,
> + unsigned int val);
> +extern int exynos_pmu_update(unsigned int offset, unsigned int mask,
> + unsigned int val);
> +extern int exynos_pmu_write(unsigned int offset, unsigned int val);
> +extern int exynos_pmu_read(unsigned int offset, unsigned int *val);
> #else
> static inline struct regmap *exynos_get_pmu_regmap(void)
> {
> return ERR_PTR(-ENODEV);
> }
> +
> +static inline int exynos_pmu_update_bits(unsigned int offset, unsigned
> int mask,
> + unsigned int val);
> +{
> + return ERR_PTR(-ENODEV);
> +}
> +
> +static inline int exynos_pmu_update(unsigned int offset, unsigned int
> mask,
> + unsigned int val);
> +{
> + return ERR_PTR(-ENODEV);
> +}

This won't build since you have the wrong return type.
I would suggest you just remove the #ifdef check entirely
and instead require drivers using this to have correct
dependencies.

Arnd

2024-01-23 10:33:53

by Guenter Roeck

[permalink] [raw]

Subject: Re: [PATCH 3/9] watchdog: s3c2410_wdt: update to use new exynos_pmu_*() apis

On 1/22/24 14:57, Peter Griffin wrote:
> Instead of obtaining the PMU regmap directly use the new exynos_pmu_*()
> APIs. The exynos_pmu_ APIs allow support of newer Exynos SoCs that have
> atomic set/clear bit hardware and platforms where the PMU registers can
> only be accessed via SMC call.
>

Not really sure about using a direect API instead of regmap. I personally
think that regmap is more generic and like the idea of abstracting hardware
accesses this way. Since that is POV, I won't argue about it. However,

> As all platforms that have PMU registers use these new APIs, remove the
> syscon regmap lookup code, as it is now redundant.
>

if syscon is now no longer needed, why keep selecting MFD_SYSCON below,
and why are linux/mfd/syscon.h and linux/regmap.h still included ?

Also, the driver did not previously only support ARCH_EXYNOS but also
ARCH_S3C64XX and ARCH_S5PV210. It is not entirely (actually, not at all)
clear to me if and how those platforms are now supported. EXYNOS_PMU
still seems to depend on ARCH_EXYNOS. How can the driver select
EXYNOS_PMU if ARCH_EXYNOS=n ?

Also, ARCH_EXYNOS already selects EXYNOS_PMU, so a conditional
"select EXYNOS_PMU if ARCH_EXYNOS" would not make sense (or be required)
either.

Please explain all the above.

Thanks,
Guenter

> Signed-off-by: Peter Griffin <[email protected]>
> ---
> drivers/watchdog/Kconfig | 1 +
> drivers/watchdog/s3c2410_wdt.c | 25 +++++++++----------------
> 2 files changed, 10 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 7d22051b15a2..b3e90e1ddf14 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -513,6 +513,7 @@ config S3C2410_WATCHDOG
> depends on ARCH_S3C64XX || ARCH_S5PV210 || ARCH_EXYNOS || COMPILE_TEST
> select WATCHDOG_CORE
> select MFD_SYSCON if ARCH_EXYNOS
> + select EXYNOS_PMU
> help
> Watchdog timer block in the Samsung S3C64xx, S5Pv210 and Exynos
> SoCs. This will reboot the system when the timer expires with
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index 349d30462c8c..fd3a9ce870a0 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -28,6 +28,8 @@
> #include <linux/regmap.h>
> #include <linux/delay.h>
>
> +#include <linux/soc/samsung/exynos-pmu.h>
> +
> #define S3C2410_WTCON 0x00
> #define S3C2410_WTDAT 0x04
> #define S3C2410_WTCNT 0x08
> @@ -187,7 +189,6 @@ struct s3c2410_wdt {
> struct watchdog_device wdt_device;
> struct notifier_block freq_transition;
> const struct s3c2410_wdt_variant *drv_data;
> - struct regmap *pmureg;
> };
>
> static const struct s3c2410_wdt_variant drv_data_s3c2410 = {
> @@ -355,8 +356,8 @@ static int s3c2410wdt_disable_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
> const u32 val = mask ? mask_val : 0;
> int ret;
>
> - ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->disable_reg,
> - mask_val, val);
> + ret = exynos_pmu_update(wdt->drv_data->disable_reg,
> + mask_val, val);
> if (ret < 0)
> dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
>
> @@ -370,8 +371,8 @@ static int s3c2410wdt_mask_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
> const u32 val = (mask ^ val_inv) ? mask_val : 0;
> int ret;
>
> - ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->mask_reset_reg,
> - mask_val, val);
> + ret = exynos_pmu_update(wdt->drv_data->mask_reset_reg,
> + mask_val, val);
> if (ret < 0)
> dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
>
> @@ -384,8 +385,8 @@ static int s3c2410wdt_enable_counter(struct s3c2410_wdt *wdt, bool en)
> const u32 val = en ? mask_val : 0;
> int ret;
>
> - ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->cnt_en_reg,
> - mask_val, val);
> + ret = exynos_pmu_update(wdt->drv_data->cnt_en_reg,
> + mask_val, val);
> if (ret < 0)
> dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
>
> @@ -617,7 +618,7 @@ static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt)
> if (!(wdt->drv_data->quirks & QUIRK_HAS_PMU_RST_STAT))
> return 0;
>
> - ret = regmap_read(wdt->pmureg, wdt->drv_data->rst_stat_reg, &rst_stat);
> + ret = exynos_pmu_read(wdt->drv_data->rst_stat_reg, &rst_stat);
> if (ret)
> dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
> else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
> @@ -698,14 +699,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> - if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
> - wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
> - "samsung,syscon-phandle");
> - if (IS_ERR(wdt->pmureg))
> - return dev_err_probe(dev, PTR_ERR(wdt->pmureg),
> - "syscon regmap lookup failed.\n");
> - }
> -
> wdt_irq = platform_get_irq(pdev, 0);
> if (wdt_irq < 0)
> return wdt_irq;

2024-01-23 11:24:51

[permalink] [raw]

Subject: Re: [PATCH 1/9] dt-bindings: watchdog: samsung-wdt: deprecate samsung,syscon-phandle

On 22/01/2024 23:57, Peter Griffin wrote:
> The watchdog driver no longer requires a phandle to obtain a regmap
> to the PMU registers. So mark this as deprecated.
>
> Signed-off-by: Peter Griffin <[email protected]>
> ---
> .../devicetree/bindings/watchdog/samsung-wdt.yaml | 15 +--------------
> 1 file changed, 1 insertion(+), 14 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml b/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml
> index 77a5ddd0426e..3970d6bf8576 100644
> --- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml
> +++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml
> @@ -56,6 +56,7 @@ properties:
> description:
> Phandle to the PMU system controller node (in case of Exynos5250,
> Exynos5420, Exynos7, Exynos850 and gs101).
> + deprecated: true

I don't see how your driver handles probe or suspend ordering, so I
don't think this is correct approach.

Handling of the watchdog requires poking PMU, thus the watchdog device
node must have reference to the PMU node. Removing it from DTS makes the
hardware representation incomplete, beside mentioned driver issue.

Best regards,
Krzysztof

2024-01-23 11:27:28

[permalink] [raw]

Subject: Re: [PATCH 2/9] soc: samsung: exynos-pmu: Add exynos_pmu_update/read/write APIs and SoC quirks

On 22/01/2024 23:57, Peter Griffin wrote:
> Newer Exynos SoCs have atomic set/clear bit hardware for PMU registers as
> these registers can be accessed by multiple masters. Some platforms also
> protect the PMU registers for security hardening reasons so they can't be
> written by normal world and are only write acessible in el3 via a SMC call.

Typo? accessible?

>
> Add support for both of these usecases using SoC specific quirks that are
> determined from the DT compatible string.>
> Drivers which need to read and write PMU registers should now use these
> new exynos_pmu_*() APIs instead of obtaining a regmap using
> syscon_regmap_lookup_by_phandle()
>
> Depending on the SoC specific quirks, the exynos_pmu_*() APIs will access
> the PMU register in the appropriate way.
>
> Signed-off-by: Peter Griffin <[email protected]>
> ---
> drivers/soc/samsung/exynos-pmu.c | 209 ++++++++++++++++++++++++-
> drivers/soc/samsung/exynos-pmu.h | 4 +
> include/linux/soc/samsung/exynos-pmu.h | 28 ++++
> 3 files changed, 234 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
> index 250537d7cfd6..e9e933ede568 100644
> --- a/drivers/soc/samsung/exynos-pmu.c
> +++ b/drivers/soc/samsung/exynos-pmu.c
> @@ -5,6 +5,7 @@
> //
> // Exynos - CPU PMU(Power Management Unit) support
>
> +#include <linux/arm-smccc.h>
> #include <linux/of.h>
> #include <linux/of_address.h>
> #include <linux/mfd/core.h>
> @@ -12,29 +13,204 @@
> #include <linux/of_platform.h>
> #include <linux/platform_device.h>
> #include <linux/delay.h>
> +#include <linux/regmap.h>
>
> #include <linux/soc/samsung/exynos-regs-pmu.h>
> #include <linux/soc/samsung/exynos-pmu.h>
>
> #include "exynos-pmu.h"
>
> +/**
> + * DOC: Quirk flags for different Exynos PMU IP-cores
> + *
> + * This driver supports multiple Exynos based SoCs, each of which might have a
> + * different set of registers and features supported.
> + *
> + * Quirk flags described below serve the purpose of telling the driver about
> + * mentioned SoC traits, and can be specified in driver data for each particular
> + * supported device.
> + *
> + * %QUIRK_HAS_ATOMIC_BITSETHW: PMU IP has special atomic bit set/clear HW
> + * to protect against PMU registers being accessed from multiple bus masters.
> + *
> + * %QUIRK_PMU_ALIVE_WRITE_SEC: PMU registers are *not* write accesible from
> + * normal world. This is found on some SoCs as a security hardening measure. PMU
> + * registers on these SoCs can only be written via a SMC call and registers are
> + * checked by EL3 firmware against an allowlist before the write can procede.
> + * Note: This quirk should only be set for platforms whose el3 firmware
> + * implements the TENSOR_SMC_PMU_SEC_REG interface below.
> + */
> +
> +#define QUIRK_HAS_ATOMIC_BITSETHW BIT(0)
> +#define QUIRK_PMU_ALIVE_WRITE_SEC BIT(1)
> +
> +#define PMUALIVE_MASK GENMASK(14, 0)
> +
> struct exynos_pmu_context {
> struct device *dev;
> const struct exynos_pmu_data *pmu_data;
> + struct regmap *pmureg;
> + void __iomem *pmu_base_addr;
> + phys_addr_t pmu_base_pa;
> + /* protect PMU reg atomic update operations */
> + spinlock_t update_lock;
> };
>
> -void __iomem *pmu_base_addr;
> static struct exynos_pmu_context *pmu_context;
>
> +/*
> + * Some SoCs are configured so that PMU_ALIVE registers can only be written
> + * from el3. As Linux needs to write some of these registers, the following
> + * SMC register read/write/read,write,modify interface is used.
> + *
> + * Note: This SMC interface is known to be implemented on gs101 and derivative
> + * SoCs.
> + */
> +#define TENSOR_SMC_PMU_SEC_REG (0x82000504)
> +#define TENSOR_PMUREG_READ 0
> +#define TENSOR_PMUREG_WRITE 1
> +#define TENSOR_PMUREG_RMW 2

These are tensor specific...

> +
> +int set_priv_reg(phys_addr_t reg, u32 val)

..but this not...

> +{
> + struct arm_smccc_res res;
> +
> + arm_smccc_smc(TENSOR_SMC_PMU_SEC_REG,

.. and this is again.

Some naming should be clarified, e.g. tensor specific functions should
have some prefix as well, e.g. tensor_writel(), tensor_cmpxchg() or
something similar.

> + reg,
> + TENSOR_PMUREG_WRITE,
> + val, 0, 0, 0, 0, &res);
> +
> + if (res.a0)
> + pr_warn("%s(): SMC failed: %lu\n", __func__, res.a0);
> +
> + return (int)res.a0;
> +}
> +
> +int rmw_priv_reg(phys_addr_t reg, u32 mask, u32 val)
> +{
> + struct arm_smccc_res res;
> +
> + arm_smccc_smc(TENSOR_SMC_PMU_SEC_REG,
> + reg,
> + TENSOR_PMUREG_RMW,
> + mask, val, 0, 0, 0, &res);
> +
> + if (res.a0)
> + pr_warn("%s(): SMC failed: %lu\n", __func__, res.a0);
> +
> + return (int)res.a0;
> +}
> +
> +/*
> + * For SoCs that have set/clear bit hardware (as indicated by
> + * QUIRK_HAS_ATOMIC_BITSETHW) this function can be used when
> + * the PMU register will be accessed by multiple masters.
> + *
> + * For example, to set bits 13:8 in PMU reg offset 0x3e80
> + * exynos_pmu_set_bit_atomic(0x3e80, 0x3f00, 0x3f00);
> + *
> + * To clear bits 13:8 in PMU offset 0x3e80
> + * exynos_pmu_set_bit_atomic(0x3e80, 0x0, 0x3f00);
> + */
> +static inline void exynos_pmu_set_bit_atomic(unsigned int offset,
> + u32 val, u32 mask)
> +{
> + unsigned long flags;
> + unsigned int i;
> +
> + spin_lock_irqsave(&pmu_context->update_lock, flags);
> + for (i = 0; i < 32; i++) {
> + if (mask & BIT(i)) {
> + if (val & BIT(i)) {
> + offset |= 0xc000;
> + pmu_raw_writel(i, offset);
> + } else {
> + offset |= 0x8000;
> + pmu_raw_writel(i, offset);
> + }
> + }
> + }
> + spin_unlock_irqrestore(&pmu_context->update_lock, flags);
> +}
> +
> +int exynos_pmu_update_bits(unsigned int offset, unsigned int mask,
> + unsigned int val)
> +{
> + if (pmu_context->pmu_data &&
> + pmu_context->pmu_data->quirks & QUIRK_PMU_ALIVE_WRITE_SEC)
> + return rmw_priv_reg(pmu_context->pmu_base_pa + offset,
> + mask, val);
> +
> + return regmap_update_bits(pmu_context->pmureg, offset, mask, val);
> +}
> +EXPORT_SYMBOL(exynos_pmu_update_bits);

You need kerneldoc for all exported functions.

Also, EXPORT_SYMBOL_GPL

> +
> void pmu_raw_writel(u32 val, u32 offset)
> {
> - writel_relaxed(val, pmu_base_addr + offset);
> + if (pmu_context->pmu_data &&
> + pmu_context->pmu_data->quirks & QUIRK_PMU_ALIVE_WRITE_SEC)
> + return (void)set_priv_reg(pmu_context->pmu_base_pa + offset,
> + val);
> +
> + return writel_relaxed(val, pmu_context->pmu_base_addr + offset);
> }
>

..

> diff --git a/drivers/soc/samsung/exynos-pmu.h b/drivers/soc/samsung/exynos-pmu.h
> index 1c652ffd79b4..570c6e4dc8c3 100644
> --- a/drivers/soc/samsung/exynos-pmu.h
> +++ b/drivers/soc/samsung/exynos-pmu.h
> @@ -25,8 +25,12 @@ struct exynos_pmu_data {
> void (*pmu_init)(void);
> void (*powerdown_conf)(enum sys_powerdown);
> void (*powerdown_conf_extra)(enum sys_powerdown);
> + u32 quirks;
> };
>
> +int set_priv_reg(phys_addr_t reg, u32 val);
> +int rmw_priv_reg(phys_addr_t reg, u32 mask, u32 val);

Why these are in the header?

Best regards,
Krzysztof

2024-01-23 11:32:19

[permalink] [raw]

Subject: Re: [PATCH 3/9] watchdog: s3c2410_wdt: update to use new exynos_pmu_*() apis

On 22/01/2024 23:57, Peter Griffin wrote:
> Instead of obtaining the PMU regmap directly use the new exynos_pmu_*()
> APIs. The exynos_pmu_ APIs allow support of newer Exynos SoCs that have
> atomic set/clear bit hardware and platforms where the PMU registers can
> only be accessed via SMC call.
>
> As all platforms that have PMU registers use these new APIs, remove the
> syscon regmap lookup code, as it is now redundant.
>
> Signed-off-by: Peter Griffin <[email protected]>
> ---
> drivers/watchdog/Kconfig | 1 +
> drivers/watchdog/s3c2410_wdt.c | 25 +++++++++----------------
> 2 files changed, 10 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 7d22051b15a2..b3e90e1ddf14 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -513,6 +513,7 @@ config S3C2410_WATCHDOG
> depends on ARCH_S3C64XX || ARCH_S5PV210 || ARCH_EXYNOS || COMPILE_TEST
> select WATCHDOG_CORE
> select MFD_SYSCON if ARCH_EXYNOS
> + select EXYNOS_PMU

This does not look compatible with S3C64xx and S5Pv210.

> help
> Watchdog timer block in the Samsung S3C64xx, S5Pv210 and Exynos
> SoCs. This will reboot the system when the timer expires with
> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> index 349d30462c8c..fd3a9ce870a0 100644
> --- a/drivers/watchdog/s3c2410_wdt.c
> +++ b/drivers/watchdog/s3c2410_wdt.c
> @@ -28,6 +28,8 @@
> #include <linux/regmap.h>
> #include <linux/delay.h>
>
> +#include <linux/soc/samsung/exynos-pmu.h>
> +
> #define S3C2410_WTCON 0x00
> #define S3C2410_WTDAT 0x04
> #define S3C2410_WTCNT 0x08
> @@ -187,7 +189,6 @@ struct s3c2410_wdt {
> struct watchdog_device wdt_device;
> struct notifier_block freq_transition;
> const struct s3c2410_wdt_variant *drv_data;
> - struct regmap *pmureg;
> };
>
> static const struct s3c2410_wdt_variant drv_data_s3c2410 = {
> @@ -355,8 +356,8 @@ static int s3c2410wdt_disable_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
> const u32 val = mask ? mask_val : 0;
> int ret;
>
> - ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->disable_reg,
> - mask_val, val);
> + ret = exynos_pmu_update(wdt->drv_data->disable_reg,
> + mask_val, val);
> if (ret < 0)
> dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
>
> @@ -370,8 +371,8 @@ static int s3c2410wdt_mask_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
> const u32 val = (mask ^ val_inv) ? mask_val : 0;
> int ret;
>
> - ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->mask_reset_reg,
> - mask_val, val);
> + ret = exynos_pmu_update(wdt->drv_data->mask_reset_reg,
> + mask_val, val);
> if (ret < 0)
> dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
>
> @@ -384,8 +385,8 @@ static int s3c2410wdt_enable_counter(struct s3c2410_wdt *wdt, bool en)
> const u32 val = en ? mask_val : 0;
> int ret;
>
> - ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->cnt_en_reg,
> - mask_val, val);
> + ret = exynos_pmu_update(wdt->drv_data->cnt_en_reg,
> + mask_val, val);
> if (ret < 0)
> dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
>
> @@ -617,7 +618,7 @@ static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt)
> if (!(wdt->drv_data->quirks & QUIRK_HAS_PMU_RST_STAT))
> return 0;
>
> - ret = regmap_read(wdt->pmureg, wdt->drv_data->rst_stat_reg, &rst_stat);
> + ret = exynos_pmu_read(wdt->drv_data->rst_stat_reg, &rst_stat);
> if (ret)
> dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
> else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
> @@ -698,14 +699,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> - if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
> - wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
> - "samsung,syscon-phandle");
> - if (IS_ERR(wdt->pmureg))
> - return dev_err_probe(dev, PTR_ERR(wdt->pmureg),
> - "syscon regmap lookup failed.\n");

Continuing topic from the binding: I don't see how you handle probe
deferral, suspend ordering.

Best regards,
Krzysztof

2024-01-23 15:54:13

[permalink] [raw]

Subject: Re: [PATCH 3/9] watchdog: s3c2410_wdt: update to use new exynos_pmu_*() apis

Hi Guenter,

Thanks for the review feedback.

On Tue, 23 Jan 2024 at 10:33, Guenter Roeck <[email protected]> wrote:
>
> On 1/22/24 14:57, Peter Griffin wrote:
> > Instead of obtaining the PMU regmap directly use the new exynos_pmu_*()
> > APIs. The exynos_pmu_ APIs allow support of newer Exynos SoCs that have
> > atomic set/clear bit hardware and platforms where the PMU registers can
> > only be accessed via SMC call.
> >
>
> Not really sure about using a direect API instead of regmap. I personally
> think that regmap is more generic and like the idea of abstracting hardware
> accesses this way. Since that is POV, I won't argue about it. However,

I did also look into the possibility of a SMC backend to regmap but that was
already tried and nacked upstream previously.

>
> > As all platforms that have PMU registers use these new APIs, remove the
> > syscon regmap lookup code, as it is now redundant.
> >
>
> if syscon is now no longer needed, why keep selecting MFD_SYSCON below,
> and why are linux/mfd/syscon.h and linux/regmap.h still included ?

Good point, those headers and the select of MFD_SYSCON are now superfluous.
Will fix it in v2.

> Also, the driver did not previously only support ARCH_EXYNOS but also
> ARCH_S3C64XX and ARCH_S5PV210. It is not entirely (actually, not at all)
> clear to me if and how those platforms are now supported. EXYNOS_PMU
> still seems to depend on ARCH_EXYNOS. How can the driver select
> EXYNOS_PMU if ARCH_EXYNOS=n ?
>
> Also, ARCH_EXYNOS already selects EXYNOS_PMU, so a conditional
> "select EXYNOS_PMU if ARCH_EXYNOS" would not make sense (or be required)
> either.
>
> Please explain all the above.

Fixing this for ARCH_S3C64XX and ARCH_S5PV210 looks to be a case of

+++ b/drivers/watchdog/Kconfig
@@ -512,8 +512,6 @@ config S3C2410_WATCHDOG
tristate "S3C6410/S5Pv210/Exynos Watchdog"
depends on ARCH_S3C64XX || ARCH_S5PV210 || ARCH_EXYNOS || COMPILE_TEST
select WATCHDOG_CORE
- select MFD_SYSCON if ARCH_EXYNOS
- select EXYNOS_PMU

and fixing the return type in the stubs that Arnd pointed out.

static inline int exynos_pmu_write(unsigned int offset, unsigned int val)
{
- return ERR_PTR(-ENODEV);
+ return -ENODEV;
}

That then compiles OK with s5pv210_defconfig and s3c6400_defconfig.

Neither ARCH_S3C64XX or ARCH_S5PV210 have PMU registers or set the PMU
register quirk flags so none of the code for setting PMU registers
would get called at runtime on those platforms.
I can make the changes described above in v2 which should fix the
ARCH_S3C64XX and ARCH_S5PV210 compatibility.

Thanks,

Peter

2024-01-23 17:31:19

[permalink] [raw]

Subject: Re: [PATCH 3/9] watchdog: s3c2410_wdt: update to use new exynos_pmu_*() apis

Hi Krzysztof,

Thanks for your review feedback.

On Tue, 23 Jan 2024 at 11:19, Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 22/01/2024 23:57, Peter Griffin wrote:
> > Instead of obtaining the PMU regmap directly use the new exynos_pmu_*()
> > APIs. The exynos_pmu_ APIs allow support of newer Exynos SoCs that have
> > atomic set/clear bit hardware and platforms where the PMU registers can
> > only be accessed via SMC call.
> >
> > As all platforms that have PMU registers use these new APIs, remove the
> > syscon regmap lookup code, as it is now redundant.
> >
> > Signed-off-by: Peter Griffin <[email protected]>
> > ---
> > drivers/watchdog/Kconfig | 1 +
> > drivers/watchdog/s3c2410_wdt.c | 25 +++++++++----------------
> > 2 files changed, 10 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index 7d22051b15a2..b3e90e1ddf14 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -513,6 +513,7 @@ config S3C2410_WATCHDOG
> > depends on ARCH_S3C64XX || ARCH_S5PV210 || ARCH_EXYNOS || COMPILE_TEST
> > select WATCHDOG_CORE
> > select MFD_SYSCON if ARCH_EXYNOS
> > + select EXYNOS_PMU
>
> This does not look compatible with S3C64xx and S5Pv210.

Please refer to my reply to Guenter on how I propose fixing that in v2.

>
> > help
> > Watchdog timer block in the Samsung S3C64xx, S5Pv210 and Exynos
> > SoCs. This will reboot the system when the timer expires with
> > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> > index 349d30462c8c..fd3a9ce870a0 100644
> > --- a/drivers/watchdog/s3c2410_wdt.c
> > +++ b/drivers/watchdog/s3c2410_wdt.c
> > @@ -28,6 +28,8 @@
> > #include <linux/regmap.h>
> > #include <linux/delay.h>
> >
> > +#include <linux/soc/samsung/exynos-pmu.h>
> > +
> > #define S3C2410_WTCON 0x00
> > #define S3C2410_WTDAT 0x04
> > #define S3C2410_WTCNT 0x08
> > @@ -187,7 +189,6 @@ struct s3c2410_wdt {
> > struct watchdog_device wdt_device;
> > struct notifier_block freq_transition;
> > const struct s3c2410_wdt_variant *drv_data;
> > - struct regmap *pmureg;
> > };
> >
> > static const struct s3c2410_wdt_variant drv_data_s3c2410 = {
> > @@ -355,8 +356,8 @@ static int s3c2410wdt_disable_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
> > const u32 val = mask ? mask_val : 0;
> > int ret;
> >
> > - ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->disable_reg,
> > - mask_val, val);
> > + ret = exynos_pmu_update(wdt->drv_data->disable_reg,
> > + mask_val, val);
> > if (ret < 0)
> > dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
> >
> > @@ -370,8 +371,8 @@ static int s3c2410wdt_mask_wdt_reset(struct s3c2410_wdt *wdt, bool mask)
> > const u32 val = (mask ^ val_inv) ? mask_val : 0;
> > int ret;
> >
> > - ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->mask_reset_reg,
> > - mask_val, val);
> > + ret = exynos_pmu_update(wdt->drv_data->mask_reset_reg,
> > + mask_val, val);
> > if (ret < 0)
> > dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
> >
> > @@ -384,8 +385,8 @@ static int s3c2410wdt_enable_counter(struct s3c2410_wdt *wdt, bool en)
> > const u32 val = en ? mask_val : 0;
> > int ret;
> >
> > - ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->cnt_en_reg,
> > - mask_val, val);
> > + ret = exynos_pmu_update(wdt->drv_data->cnt_en_reg,
> > + mask_val, val);
> > if (ret < 0)
> > dev_err(wdt->dev, "failed to update reg(%d)\n", ret);
> >
> > @@ -617,7 +618,7 @@ static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt)
> > if (!(wdt->drv_data->quirks & QUIRK_HAS_PMU_RST_STAT))
> > return 0;
> >
> > - ret = regmap_read(wdt->pmureg, wdt->drv_data->rst_stat_reg, &rst_stat);
> > + ret = exynos_pmu_read(wdt->drv_data->rst_stat_reg, &rst_stat);
> > if (ret)
> > dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
> > else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
> > @@ -698,14 +699,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> > if (ret)
> > return ret;
> >
> > - if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
> > - wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
> > - "samsung,syscon-phandle");
> > - if (IS_ERR(wdt->pmureg))
> > - return dev_err_probe(dev, PTR_ERR(wdt->pmureg),
> > - "syscon regmap lookup failed.\n");
>
>
> Continuing topic from the binding: I don't see how you handle probe
> deferral, suspend ordering.

The current implementation is simply relying on exynos-pmu being
postcore_initcall level.

I was just looking around for any existing Linux APIs that could be a
more robust solution. It looks like

of_parse_phandle()
and
of_find_device_by_node();

Are often used to solve this type of probe deferral issue between
devices. Is that what you would recommend using? Or is there something
even better?

Thanks,

Peter

2024-01-23 18:13:25

[permalink] [raw]

Subject: Re: [PATCH 3/9] watchdog: s3c2410_wdt: update to use new exynos_pmu_*() apis

On 23/01/2024 18:30, Peter Griffin wrote:
>>> dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
>>> else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
>>> @@ -698,14 +699,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>>> if (ret)
>>> return ret;
>>>
>>> - if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
>>> - wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
>>> - "samsung,syscon-phandle");
>>> - if (IS_ERR(wdt->pmureg))
>>> - return dev_err_probe(dev, PTR_ERR(wdt->pmureg),
>>> - "syscon regmap lookup failed.\n");
>>
>>
>> Continuing topic from the binding: I don't see how you handle probe
>> deferral, suspend ordering.
>
> The current implementation is simply relying on exynos-pmu being
> postcore_initcall level.
>
> I was just looking around for any existing Linux APIs that could be a
> more robust solution. It looks like
>
> of_parse_phandle()
> and
> of_find_device_by_node();
>
> Are often used to solve this type of probe deferral issue between
> devices. Is that what you would recommend using? Or is there something
> even better?

I think you should keep the phandle and then set device link based on
of_find_device_by_node(). This would actually improve the code, because
syscon_regmap_lookup_by_phandle() does not create device links.

Best regards,
Krzysztof

2024-01-23 19:03:24

by Sam Protsenko

[permalink] [raw]

Subject: Re: [PATCH 2/9] soc: samsung: exynos-pmu: Add exynos_pmu_update/read/write APIs and SoC quirks

On Mon, Jan 22, 2024 at 4:57 PM Peter Griffin <peter.griffin@linaroorg> wrote:
>
> Newer Exynos SoCs have atomic set/clear bit hardware for PMU registers as
> these registers can be accessed by multiple masters. Some platforms also
> protect the PMU registers for security hardening reasons so they can't be
> written by normal world and are only write acessible in el3 via a SMC call.
>
> Add support for both of these usecases using SoC specific quirks that are
> determined from the DT compatible string.
>
> Drivers which need to read and write PMU registers should now use these
> new exynos_pmu_*() APIs instead of obtaining a regmap using
> syscon_regmap_lookup_by_phandle()
>
> Depending on the SoC specific quirks, the exynos_pmu_*() APIs will access
> the PMU register in the appropriate way.
>
> Signed-off-by: Peter Griffin <[email protected]>
> ---
> drivers/soc/samsung/exynos-pmu.c | 209 ++++++++++++++++++++++++-
> drivers/soc/samsung/exynos-pmu.h | 4 +
> include/linux/soc/samsung/exynos-pmu.h | 28 ++++
> 3 files changed, 234 insertions(+), 7 deletions(-)
>

[snip]

> +
> +int exynos_pmu_update_bits(unsigned int offset, unsigned int mask,
> + unsigned int val)
> +{
> + if (pmu_context->pmu_data &&
> + pmu_context->pmu_data->quirks & QUIRK_PMU_ALIVE_WRITE_SEC)
> + return rmw_priv_reg(pmu_context->pmu_base_pa + offset,
> + mask, val);
> +
> + return regmap_update_bits(pmu_context->pmureg, offset, mask, val);
> +}
> +EXPORT_SYMBOL(exynos_pmu_update_bits);
> +

This seems a bit hacky, from the design perspective. This way the user
will have to worry about things like driver dependencies, making sure
everything is instantiated in a correct order, etc. It also hides the
details otherwise visible through "syscon-phandle" property in the
device tree. Can we instead rework it by overriding regmap
implementation for Exynos specifics, and then continue to use it in
the leaf drivers via "syscon-phandle" property?

[snip]

2024-01-24 03:38:26

[permalink] [raw]

Subject: Re: [PATCH 3/9] watchdog: s3c2410_wdt: update to use new exynos_pmu_*() apis

On Tue, Jan 23, 2024 at 10:12 AM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 23/01/2024 18:30, Peter Griffin wrote:
> >>> dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
> >>> else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
> >>> @@ -698,14 +699,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> >>> if (ret)
> >>> return ret;
> >>>
> >>> - if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
> >>> - wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
> >>> - "samsung,syscon-phandle");
> >>> - if (IS_ERR(wdt->pmureg))
> >>> - return dev_err_probe(dev, PTR_ERR(wdt->pmureg),
> >>> - "syscon regmap lookup failed.\n");
> >>
> >>
> >> Continuing topic from the binding: I don't see how you handle probe
> >> deferral, suspend ordering.
> >
> > The current implementation is simply relying on exynos-pmu being
> > postcore_initcall level.
> >
> > I was just looking around for any existing Linux APIs that could be a
> > more robust solution. It looks like
> >
> > of_parse_phandle()
> > and
> > of_find_device_by_node();
> >
> > Are often used to solve this type of probe deferral issue between
> > devices. Is that what you would recommend using? Or is there something
> > even better?
>
> I think you should keep the phandle and then set device link based on
> of_find_device_by_node(). This would actually improve the code, because
> syscon_regmap_lookup_by_phandle() does not create device links.

I kinda agree with this. Just because we no longer use a syscon API to
find the PMU register address doesn't mean the WDT doesn't depend on
the PMU.

However, I think we should move to a generic "syscon" property. Then I
can add support for "syscon" property to fw_devlink and then things
will just work in terms of probe ordering, suspend/resume and also
showing the dependency in DT even if you don't use the syscon APIs.

Side note 1:

I think we really should officially document a generic syscon DT
property similar to how we have a generic "clocks" or "dmas" property.
Then we can have a syscon_get_regmap() that's like so:

struct regmap *syscon_get_regmap(struct device *dev)
{
return syscon_regmap_lookup_by_phandle(dev->of_node, "syscon");
}

Instead of every device defining its own bespoke DT property to do the
exact same thing. I did a quick "back of the envelope" grep on this
and I get about 143 unique properties just to get the syscon regmap.
$ git grep -A1 syscon_regmap_lookup_by_phandle | grep '"' | sed -e
's/^[^"]*//' -e 's/"[^"]*$/"/' | sort | uniq | wc -l
143

Side note 2:

How are we making sure that it's the exynos-pmu driver that ends up
probing the PMU and not the generic syscon driver? Both of these are
platform drivers. And the exynos PMU device lists both the exynos
compatible string and the syscon property. Is it purely a link order
coincidence?

-Saravana

2024-01-24 06:27:54

[permalink] [raw]

Subject: Re: [PATCH 3/9] watchdog: s3c2410_wdt: update to use new exynos_pmu_*() apis

On 24/01/2024 04:37, Saravana Kannan wrote:
> On Tue, Jan 23, 2024 at 10:12 AM Krzysztof Kozlowski
> <[email protected]> wrote:
>>
>> On 23/01/2024 18:30, Peter Griffin wrote:
>>>>> dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
>>>>> else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
>>>>> @@ -698,14 +699,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>>>>> if (ret)
>>>>> return ret;
>>>>>
>>>>> - if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
>>>>> - wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
>>>>> - "samsung,syscon-phandle");
>>>>> - if (IS_ERR(wdt->pmureg))
>>>>> - return dev_err_probe(dev, PTR_ERR(wdt->pmureg),
>>>>> - "syscon regmap lookup failed.\n");
>>>>
>>>>
>>>> Continuing topic from the binding: I don't see how you handle probe
>>>> deferral, suspend ordering.
>>>
>>> The current implementation is simply relying on exynos-pmu being
>>> postcore_initcall level.
>>>
>>> I was just looking around for any existing Linux APIs that could be a
>>> more robust solution. It looks like
>>>
>>> of_parse_phandle()
>>> and
>>> of_find_device_by_node();
>>>
>>> Are often used to solve this type of probe deferral issue between
>>> devices. Is that what you would recommend using? Or is there something
>>> even better?
>>
>> I think you should keep the phandle and then set device link based on
>> of_find_device_by_node(). This would actually improve the code, because
>> syscon_regmap_lookup_by_phandle() does not create device links.
>
> I kinda agree with this. Just because we no longer use a syscon API to
> find the PMU register address doesn't mean the WDT doesn't depend on
> the PMU.
>
> However, I think we should move to a generic "syscon" property. Then I
> can add support for "syscon" property to fw_devlink and then things
> will just work in terms of probe ordering, suspend/resume and also
> showing the dependency in DT even if you don't use the syscon APIs.
>
> Side note 1:
>
> I think we really should officially document a generic syscon DT
> property similar to how we have a generic "clocks" or "dmas" property.
> Then we can have a syscon_get_regmap() that's like so:
>
> struct regmap *syscon_get_regmap(struct device *dev)
> {
> return syscon_regmap_lookup_by_phandle(dev->of_node, "syscon");
> }
>
> Instead of every device defining its own bespoke DT property to do the
> exact same thing. I did a quick "back of the envelope" grep on this
> and I get about 143 unique properties just to get the syscon regmap.
> $ git grep -A1 syscon_regmap_lookup_by_phandle | grep '"' | sed -e
> 's/^[^"]*//' -e 's/"[^"]*$/"/' | sort | uniq | wc -l
> 143

Sorry, generic "syscon" property won't fly with DT maintainers, because
there is no such thing as syscon in any of hardware.

>
> Side note 2:
>
> How are we making sure that it's the exynos-pmu driver that ends up
> probing the PMU and not the generic syscon driver? Both of these are
> platform drivers. And the exynos PMU device lists both the exynos
> compatible string and the syscon property. Is it purely a link order
> coincidence?

initcall ordering

Best regards,
Krzysztof

2024-01-24 10:03:14

[permalink] [raw]

Subject: Re: [PATCH 2/9] soc: samsung: exynos-pmu: Add exynos_pmu_update/read/write APIs and SoC quirks

Hi Sam,

Thanks for the review feedback.

On Tue, 23 Jan 2024 at 18:56, Sam Protsenko <[email protected]> wrote:
>
> On Mon, Jan 22, 2024 at 4:57 PM Peter Griffin <[email protected]> wrote:
> >
> > Newer Exynos SoCs have atomic set/clear bit hardware for PMU registers as
> > these registers can be accessed by multiple masters. Some platforms also
> > protect the PMU registers for security hardening reasons so they can't be
> > written by normal world and are only write acessible in el3 via a SMC call.
> >
> > Add support for both of these usecases using SoC specific quirks that are
> > determined from the DT compatible string.
> >
> > Drivers which need to read and write PMU registers should now use these
> > new exynos_pmu_*() APIs instead of obtaining a regmap using
> > syscon_regmap_lookup_by_phandle()
> >
> > Depending on the SoC specific quirks, the exynos_pmu_*() APIs will access
> > the PMU register in the appropriate way.
> >
> > Signed-off-by: Peter Griffin <[email protected]>
> > ---
> > drivers/soc/samsung/exynos-pmu.c | 209 ++++++++++++++++++++++++-
> > drivers/soc/samsung/exynos-pmu.h | 4 +
> > include/linux/soc/samsung/exynos-pmu.h | 28 ++++
> > 3 files changed, 234 insertions(+), 7 deletions(-)
> >
>
> [snip]
>
> > +
> > +int exynos_pmu_update_bits(unsigned int offset, unsigned int mask,
> > + unsigned int val)
> > +{
> > + if (pmu_context->pmu_data &&
> > + pmu_context->pmu_data->quirks & QUIRK_PMU_ALIVE_WRITE_SEC)
> > + return rmw_priv_reg(pmu_context->pmu_base_pa + offset,
> > + mask, val);
> > +
> > + return regmap_update_bits(pmu_context->pmureg, offset, mask, val);
> > +}
> > +EXPORT_SYMBOL(exynos_pmu_update_bits);
> > +
>
> This seems a bit hacky, from the design perspective. This way the user
> will have to worry about things like driver dependencies, making sure
> everything is instantiated in a correct order, etc. It also hides the
> details otherwise visible through "syscon-phandle" property in the
> device tree.

In v2 I will keep the phandle to pmu_system_controller in DT, and add
some -EPROBE_DEFER logic (See my email with Krzysztof).

> Can we instead rework it by overriding regmap
> implementation for Exynos specifics, and then continue to use it in
> the leaf drivers via "syscon-phandle" property?

I did look at that possibility first, as like you say it would avoid
updating the leaf drivers to use the new API. Unfortunately a SMC
backend to regmap was already tried and nacked upstream pretty hard.
See here https://lore.kernel.org/lkml/[email protected]/T/

regards,

Peter.

2024-01-24 10:21:29

[permalink] [raw]

Subject: Re: [PATCH 2/9] soc: samsung: exynos-pmu: Add exynos_pmu_update/read/write APIs and SoC quirks

Hi Arnd,

On Tue, 23 Jan 2024 at 08:11, Arnd Bergmann <[email protected]> wrote:
>
> On Mon, Jan 22, 2024, at 23:57, Peter Griffin wrote:
>
> > --- a/include/linux/soc/samsung/exynos-pmu.h
> > +++ b/include/linux/soc/samsung/exynos-pmu.h
> > @@ -21,11 +21,39 @@ enum sys_powerdown {
> > extern void exynos_sys_powerdown_conf(enum sys_powerdown mode);
> > #ifdef CONFIG_EXYNOS_PMU
> > extern struct regmap *exynos_get_pmu_regmap(void);
> > +extern int exynos_pmu_update_bits(unsigned int offset, unsigned int
> > mask,
> > + unsigned int val);
> > +extern int exynos_pmu_update(unsigned int offset, unsigned int mask,
> > + unsigned int val);
> > +extern int exynos_pmu_write(unsigned int offset, unsigned int val);
> > +extern int exynos_pmu_read(unsigned int offset, unsigned int *val);
> > #else
> > static inline struct regmap *exynos_get_pmu_regmap(void)
> > {
> > return ERR_PTR(-ENODEV);
> > }
> > +
> > +static inline int exynos_pmu_update_bits(unsigned int offset, unsigned
> > int mask,
> > + unsigned int val);
> > +{
> > + return ERR_PTR(-ENODEV);
> > +}
> > +
> > +static inline int exynos_pmu_update(unsigned int offset, unsigned int
> > mask,
> > + unsigned int val);
> > +{
> > + return ERR_PTR(-ENODEV);
> > +}
>
> This won't build since you have the wrong return type.
> I would suggest you just remove the #ifdef check entirely
> and instead require drivers using this to have correct
> dependencies.

Whoops, will fix it in v2. We need those stubs for platforms like
ARCH_S3C64XX that don't have a PMU but use some of the same drivers.

Thanks,

Peter.

2024-01-24 10:51:21

[permalink] [raw]

Subject: Re: [PATCH 2/9] soc: samsung: exynos-pmu: Add exynos_pmu_update/read/write APIs and SoC quirks

Hi Krzysztof,

Thanks for the review feedback.

On Tue, 23 Jan 2024 at 11:17, Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 22/01/2024 23:57, Peter Griffin wrote:
> > Newer Exynos SoCs have atomic set/clear bit hardware for PMU registers as
> > these registers can be accessed by multiple masters. Some platforms also
> > protect the PMU registers for security hardening reasons so they can't be
> > written by normal world and are only write acessible in el3 via a SMC call.
>
>
> Typo? accessible?

Will fix in v2.
>
> >
> > Add support for both of these usecases using SoC specific quirks that are
> > determined from the DT compatible string.>
> > Drivers which need to read and write PMU registers should now use these
> > new exynos_pmu_*() APIs instead of obtaining a regmap using
> > syscon_regmap_lookup_by_phandle()
> >
> > Depending on the SoC specific quirks, the exynos_pmu_*() APIs will access
> > the PMU register in the appropriate way.
> >
> > Signed-off-by: Peter Griffin <[email protected]>
> > ---
> > drivers/soc/samsung/exynos-pmu.c | 209 ++++++++++++++++++++++++-
> > drivers/soc/samsung/exynos-pmu.h | 4 +
> > include/linux/soc/samsung/exynos-pmu.h | 28 ++++
> > 3 files changed, 234 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
> > index 250537d7cfd6..e9e933ede568 100644
> > --- a/drivers/soc/samsung/exynos-pmu.c
> > +++ b/drivers/soc/samsung/exynos-pmu.c
> > @@ -5,6 +5,7 @@
> > //
> > // Exynos - CPU PMU(Power Management Unit) support
> >
> > +#include <linux/arm-smccc.h>
> > #include <linux/of.h>
> > #include <linux/of_address.h>
> > #include <linux/mfd/core.h>
> > @@ -12,29 +13,204 @@
> > #include <linux/of_platform.h>
> > #include <linux/platform_device.h>
> > #include <linux/delay.h>
> > +#include <linux/regmap.h>
> >
> > #include <linux/soc/samsung/exynos-regs-pmu.h>
> > #include <linux/soc/samsung/exynos-pmu.h>
> >
> > #include "exynos-pmu.h"
> >
> > +/**
> > + * DOC: Quirk flags for different Exynos PMU IP-cores
> > + *
> > + * This driver supports multiple Exynos based SoCs, each of which might have a
> > + * different set of registers and features supported.
> > + *
> > + * Quirk flags described below serve the purpose of telling the driver about
> > + * mentioned SoC traits, and can be specified in driver data for each particular
> > + * supported device.
> > + *
> > + * %QUIRK_HAS_ATOMIC_BITSETHW: PMU IP has special atomic bit set/clear HW
> > + * to protect against PMU registers being accessed from multiple bus masters.
> > + *
> > + * %QUIRK_PMU_ALIVE_WRITE_SEC: PMU registers are *not* write accesible from
> > + * normal world. This is found on some SoCs as a security hardening measure. PMU
> > + * registers on these SoCs can only be written via a SMC call and registers are
> > + * checked by EL3 firmware against an allowlist before the write can procede.
> > + * Note: This quirk should only be set for platforms whose el3 firmware
> > + * implements the TENSOR_SMC_PMU_SEC_REG interface below.
> > + */
> > +
> > +#define QUIRK_HAS_ATOMIC_BITSETHW BIT(0)
> > +#define QUIRK_PMU_ALIVE_WRITE_SEC BIT(1)
> > +
> > +#define PMUALIVE_MASK GENMASK(14, 0)
> > +
> > struct exynos_pmu_context {
> > struct device *dev;
> > const struct exynos_pmu_data *pmu_data;
> > + struct regmap *pmureg;
> > + void __iomem *pmu_base_addr;
> > + phys_addr_t pmu_base_pa;
> > + /* protect PMU reg atomic update operations */
> > + spinlock_t update_lock;
> > };
> >
> > -void __iomem *pmu_base_addr;
> > static struct exynos_pmu_context *pmu_context;
> >
> > +/*
> > + * Some SoCs are configured so that PMU_ALIVE registers can only be written
> > + * from el3. As Linux needs to write some of these registers, the following
> > + * SMC register read/write/read,write,modify interface is used.
> > + *
> > + * Note: This SMC interface is known to be implemented on gs101 and derivative
> > + * SoCs.
> > + */
> > +#define TENSOR_SMC_PMU_SEC_REG (0x82000504)
> > +#define TENSOR_PMUREG_READ 0
> > +#define TENSOR_PMUREG_WRITE 1
> > +#define TENSOR_PMUREG_RMW 2
>
> These are tensor specific...
>
> > +
> > +int set_priv_reg(phys_addr_t reg, u32 val)
>
> ...but this not...
>
> > +{
> > + struct arm_smccc_res res;
> > +
> > + arm_smccc_smc(TENSOR_SMC_PMU_SEC_REG,
>
> ... and this is again.
>
> Some naming should be clarified, e.g. tensor specific functions should
> have some prefix as well, e.g. tensor_writel(), tensor_cmpxchg() or
> something similar.

Noted. I will add a tensor prefix on these two functions as well in v2.

>
>
> > + reg,
> > + TENSOR_PMUREG_WRITE,
> > + val, 0, 0, 0, 0, &res);
> > +
> > + if (res.a0)
> > + pr_warn("%s(): SMC failed: %lu\n", __func__, res.a0);
> > +
> > + return (int)res.a0;
> > +}
> > +
> > +int rmw_priv_reg(phys_addr_t reg, u32 mask, u32 val)
> > +{
> > + struct arm_smccc_res res;
> > +
> > + arm_smccc_smc(TENSOR_SMC_PMU_SEC_REG,
> > + reg,
> > + TENSOR_PMUREG_RMW,
> > + mask, val, 0, 0, 0, &res);
> > +
> > + if (res.a0)
> > + pr_warn("%s(): SMC failed: %lu\n", __func__, res.a0);
> > +
> > + return (int)res.a0;
> > +}
> > +
> > +/*
> > + * For SoCs that have set/clear bit hardware (as indicated by
> > + * QUIRK_HAS_ATOMIC_BITSETHW) this function can be used when
> > + * the PMU register will be accessed by multiple masters.
> > + *
> > + * For example, to set bits 13:8 in PMU reg offset 0x3e80
> > + * exynos_pmu_set_bit_atomic(0x3e80, 0x3f00, 0x3f00);
> > + *
> > + * To clear bits 13:8 in PMU offset 0x3e80
> > + * exynos_pmu_set_bit_atomic(0x3e80, 0x0, 0x3f00);
> > + */
> > +static inline void exynos_pmu_set_bit_atomic(unsigned int offset,
> > + u32 val, u32 mask)
> > +{
> > + unsigned long flags;
> > + unsigned int i;
> > +
> > + spin_lock_irqsave(&pmu_context->update_lock, flags);
> > + for (i = 0; i < 32; i++) {
> > + if (mask & BIT(i)) {
> > + if (val & BIT(i)) {
> > + offset |= 0xc000;
> > + pmu_raw_writel(i, offset);
> > + } else {
> > + offset |= 0x8000;
> > + pmu_raw_writel(i, offset);
> > + }
> > + }
> > + }
> > + spin_unlock_irqrestore(&pmu_context->update_lock, flags);
> > +}
> > +
> > +int exynos_pmu_update_bits(unsigned int offset, unsigned int mask,
> > + unsigned int val)
> > +{
> > + if (pmu_context->pmu_data &&
> > + pmu_context->pmu_data->quirks & QUIRK_PMU_ALIVE_WRITE_SEC)
> > + return rmw_priv_reg(pmu_context->pmu_base_pa + offset,
> > + mask, val);
> > +
> > + return regmap_update_bits(pmu_context->pmureg, offset, mask, val);
> > +}
> > +EXPORT_SYMBOL(exynos_pmu_update_bits);
>
> You need kerneldoc for all exported functions.
>
> Also, EXPORT_SYMBOL_GPL

Will fix both in v2.

>
> > +
> > void pmu_raw_writel(u32 val, u32 offset)
> > {
> > - writel_relaxed(val, pmu_base_addr + offset);
> > + if (pmu_context->pmu_data &&
> > + pmu_context->pmu_data->quirks & QUIRK_PMU_ALIVE_WRITE_SEC)
> > + return (void)set_priv_reg(pmu_context->pmu_base_pa + offset,
> > + val);
> > +
> > + return writel_relaxed(val, pmu_context->pmu_base_addr + offset);
> > }
> >
>
> ...
>
> > diff --git a/drivers/soc/samsung/exynos-pmu.h b/drivers/soc/samsung/exynos-pmu.h
> > index 1c652ffd79b4..570c6e4dc8c3 100644
> > --- a/drivers/soc/samsung/exynos-pmu.h
> > +++ b/drivers/soc/samsung/exynos-pmu.h
> > @@ -25,8 +25,12 @@ struct exynos_pmu_data {
> > void (*pmu_init)(void);
> > void (*powerdown_conf)(enum sys_powerdown);
> > void (*powerdown_conf_extra)(enum sys_powerdown);
> > + u32 quirks;
> > };
> >
> > +int set_priv_reg(phys_addr_t reg, u32 val);
> > +int rmw_priv_reg(phys_addr_t reg, u32 mask, u32 val);
>
> Why these are in the header?

Will fix in v2.

regards,

Peter.

2024-01-24 20:31:35

by Sam Protsenko

[permalink] [raw]

Subject: Re: [PATCH 2/9] soc: samsung: exynos-pmu: Add exynos_pmu_update/read/write APIs and SoC quirks

On Wed, Jan 24, 2024 at 4:02 AM Peter Griffin <peter.griffin@linaroorg> wrote:
>
> Hi Sam,
>
> Thanks for the review feedback.
>
> On Tue, 23 Jan 2024 at 18:56, Sam Protsenko <[email protected]> wrote:
> >
> > On Mon, Jan 22, 2024 at 4:57 PM Peter Griffin <[email protected]> wrote:
> > >
> > > Newer Exynos SoCs have atomic set/clear bit hardware for PMU registers as
> > > these registers can be accessed by multiple masters. Some platforms also
> > > protect the PMU registers for security hardening reasons so they can't be
> > > written by normal world and are only write acessible in el3 via a SMC call.
> > >
> > > Add support for both of these usecases using SoC specific quirks that are
> > > determined from the DT compatible string.
> > >
> > > Drivers which need to read and write PMU registers should now use these
> > > new exynos_pmu_*() APIs instead of obtaining a regmap using
> > > syscon_regmap_lookup_by_phandle()
> > >
> > > Depending on the SoC specific quirks, the exynos_pmu_*() APIs will access
> > > the PMU register in the appropriate way.
> > >
> > > Signed-off-by: Peter Griffin <[email protected]>
> > > ---
> > > drivers/soc/samsung/exynos-pmu.c | 209 ++++++++++++++++++++++++-
> > > drivers/soc/samsung/exynos-pmu.h | 4 +
> > > include/linux/soc/samsung/exynos-pmu.h | 28 ++++
> > > 3 files changed, 234 insertions(+), 7 deletions(-)
> > >
> >
> > [snip]
> >
> > > +
> > > +int exynos_pmu_update_bits(unsigned int offset, unsigned int mask,
> > > + unsigned int val)
> > > +{
> > > + if (pmu_context->pmu_data &&
> > > + pmu_context->pmu_data->quirks & QUIRK_PMU_ALIVE_WRITE_SEC)
> > > + return rmw_priv_reg(pmu_context->pmu_base_pa + offset,
> > > + mask, val);
> > > +
> > > + return regmap_update_bits(pmu_context->pmureg, offset, mask, val);
> > > +}
> > > +EXPORT_SYMBOL(exynos_pmu_update_bits);
> > > +
> >
> > This seems a bit hacky, from the design perspective. This way the user
> > will have to worry about things like driver dependencies, making sure
> > everything is instantiated in a correct order, etc. It also hides the
> > details otherwise visible through "syscon-phandle" property in the
> > device tree.
>
> In v2 I will keep the phandle to pmu_system_controller in DT, and add
> some -EPROBE_DEFER logic (See my email with Krzysztof).
>
> > Can we instead rework it by overriding regmap
> > implementation for Exynos specifics, and then continue to use it in
> > the leaf drivers via "syscon-phandle" property?
>
> I did look at that possibility first, as like you say it would avoid
> updating the leaf drivers to use the new API. Unfortunately a SMC
> backend to regmap was already tried and nacked upstream pretty hard.
> See here https://lore.kernel.org/lkml/[email protected]/T/
>

Oh, I didn't mean creating a new regmap implementation :) To
illustrate what I meant, please look at these:

- drivers/mfd/altera-sysmgr.c
- altr_sysmgr_regmap_lookup_by_phandle()
- arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
- drivers/mmc/host/dw_mmc-pltfm.c

They basically implement their own regmap operations (with smcc too)
in their syscon implementation. So they can actually reference that
syscon as phandle in device tree and avoid exporting and calling
read/write operations (which I think looks hacky). Instead they use
altr_sysmgr_regmap_lookup_by_phandle() to get their regmap (which
performs smcc), and then they just use regular regmap_read() /
regmap_write or whatever functions to operate on their regmap object.
That's what I meant by "overriding" the regmap.

Do you think this approach would be clearer and more "productizable"
so to speak? Just a thought.

> regards,
>
> Peter.

2024-01-24 21:28:13

[permalink] [raw]

Subject: Re: [PATCH 3/9] watchdog: s3c2410_wdt: update to use new exynos_pmu_*() apis

On Tue, Jan 23, 2024 at 10:27 PM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 24/01/2024 04:37, Saravana Kannan wrote:
> > On Tue, Jan 23, 2024 at 10:12 AM Krzysztof Kozlowski
> > <[email protected]> wrote:
> >>
> >> On 23/01/2024 18:30, Peter Griffin wrote:
> >>>>> dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
> >>>>> else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
> >>>>> @@ -698,14 +699,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> >>>>> if (ret)
> >>>>> return ret;
> >>>>>
> >>>>> - if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
> >>>>> - wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
> >>>>> - "samsung,syscon-phandle");
> >>>>> - if (IS_ERR(wdt->pmureg))
> >>>>> - return dev_err_probe(dev, PTR_ERR(wdt->pmureg),
> >>>>> - "syscon regmap lookup failed.\n");
> >>>>
> >>>>
> >>>> Continuing topic from the binding: I don't see how you handle probe
> >>>> deferral, suspend ordering.
> >>>
> >>> The current implementation is simply relying on exynos-pmu being
> >>> postcore_initcall level.
> >>>
> >>> I was just looking around for any existing Linux APIs that could be a
> >>> more robust solution. It looks like
> >>>
> >>> of_parse_phandle()
> >>> and
> >>> of_find_device_by_node();
> >>>
> >>> Are often used to solve this type of probe deferral issue between
> >>> devices. Is that what you would recommend using? Or is there something
> >>> even better?
> >>
> >> I think you should keep the phandle and then set device link based on
> >> of_find_device_by_node(). This would actually improve the code, because
> >> syscon_regmap_lookup_by_phandle() does not create device links.
> >
> > I kinda agree with this. Just because we no longer use a syscon API to
> > find the PMU register address doesn't mean the WDT doesn't depend on
> > the PMU.
> >
> > However, I think we should move to a generic "syscon" property. Then I
> > can add support for "syscon" property to fw_devlink and then things
> > will just work in terms of probe ordering, suspend/resume and also
> > showing the dependency in DT even if you don't use the syscon APIs.
> >
> > Side note 1:
> >
> > I think we really should officially document a generic syscon DT
> > property similar to how we have a generic "clocks" or "dmas" property.
> > Then we can have a syscon_get_regmap() that's like so:
> >
> > struct regmap *syscon_get_regmap(struct device *dev)
> > {
> > return syscon_regmap_lookup_by_phandle(dev->of_node, "syscon");
> > }
> >
> > Instead of every device defining its own bespoke DT property to do the
> > exact same thing. I did a quick "back of the envelope" grep on this
> > and I get about 143 unique properties just to get the syscon regmap.
> > $ git grep -A1 syscon_regmap_lookup_by_phandle | grep '"' | sed -e
> > 's/^[^"]*//' -e 's/"[^"]*$/"/' | sort | uniq | wc -l
> > 143
>
> Sorry, generic "syscon" property won't fly with DT maintainers, because
> there is no such thing as syscon in any of hardware.

Then why do we allow a "syscon" compatible string and nodes? If the
"syscon" property isn't clear enough, we can make it something like
gpios and have it be <whatever>-syscon or have syscon-names property
if you want to give it a name.
143 bespoke properties all to say "here are some registers I need to
twiddle that's outside my regmap" doesn't seem great.

> >
> > Side note 2:
> >
> > How are we making sure that it's the exynos-pmu driver that ends up
> > probing the PMU and not the generic syscon driver? Both of these are
> > platform drivers. And the exynos PMU device lists both the exynos
> > compatible string and the syscon property. Is it purely a link order
> > coincidence?
>
> initcall ordering

Both these drivers usr postcore_initcall(). So it's purely because
soc/ is listed earlier in drivers/Makefile than mfd/. And as soon as
drivers are made into modules this is going to break. This is
terrible. If you want to have a modular system, this is going to throw
in a wrench.

-Saravana

2024-01-25 07:38:19

[permalink] [raw]

Subject: Re: [PATCH 3/9] watchdog: s3c2410_wdt: update to use new exynos_pmu_*() apis

On 24/01/2024 22:27, Saravana Kannan wrote:
> On Tue, Jan 23, 2024 at 10:27 PM Krzysztof Kozlowski
> <[email protected]> wrote:
>>
>> On 24/01/2024 04:37, Saravana Kannan wrote:
>>> On Tue, Jan 23, 2024 at 10:12 AM Krzysztof Kozlowski
>>> <[email protected]> wrote:
>>>>
>>>> On 23/01/2024 18:30, Peter Griffin wrote:
>>>>>>> dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
>>>>>>> else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
>>>>>>> @@ -698,14 +699,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
>>>>>>> if (ret)
>>>>>>> return ret;
>>>>>>>
>>>>>>> - if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
>>>>>>> - wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
>>>>>>> - "samsung,syscon-phandle");
>>>>>>> - if (IS_ERR(wdt->pmureg))
>>>>>>> - return dev_err_probe(dev, PTR_ERR(wdt->pmureg),
>>>>>>> - "syscon regmap lookup failed.\n");
>>>>>>
>>>>>>
>>>>>> Continuing topic from the binding: I don't see how you handle probe
>>>>>> deferral, suspend ordering.
>>>>>
>>>>> The current implementation is simply relying on exynos-pmu being
>>>>> postcore_initcall level.
>>>>>
>>>>> I was just looking around for any existing Linux APIs that could be a
>>>>> more robust solution. It looks like
>>>>>
>>>>> of_parse_phandle()
>>>>> and
>>>>> of_find_device_by_node();
>>>>>
>>>>> Are often used to solve this type of probe deferral issue between
>>>>> devices. Is that what you would recommend using? Or is there something
>>>>> even better?
>>>>
>>>> I think you should keep the phandle and then set device link based on
>>>> of_find_device_by_node(). This would actually improve the code, because
>>>> syscon_regmap_lookup_by_phandle() does not create device links.
>>>
>>> I kinda agree with this. Just because we no longer use a syscon API to
>>> find the PMU register address doesn't mean the WDT doesn't depend on
>>> the PMU.
>>>
>>> However, I think we should move to a generic "syscon" property. Then I
>>> can add support for "syscon" property to fw_devlink and then things
>>> will just work in terms of probe ordering, suspend/resume and also
>>> showing the dependency in DT even if you don't use the syscon APIs.
>>>
>>> Side note 1:
>>>
>>> I think we really should officially document a generic syscon DT
>>> property similar to how we have a generic "clocks" or "dmas" property.
>>> Then we can have a syscon_get_regmap() that's like so:
>>>
>>> struct regmap *syscon_get_regmap(struct device *dev)
>>> {
>>> return syscon_regmap_lookup_by_phandle(dev->of_node, "syscon");
>>> }
>>>
>>> Instead of every device defining its own bespoke DT property to do the
>>> exact same thing. I did a quick "back of the envelope" grep on this
>>> and I get about 143 unique properties just to get the syscon regmap.
>>> $ git grep -A1 syscon_regmap_lookup_by_phandle | grep '"' | sed -e
>>> 's/^[^"]*//' -e 's/"[^"]*$/"/' | sort | uniq | wc -l
>>> 143
>>
>> Sorry, generic "syscon" property won't fly with DT maintainers, because
>> there is no such thing as syscon in any of hardware.
>
> Then why do we allow a "syscon" compatible string and nodes? If the

To bind Linux drivers.

> "syscon" property isn't clear enough, we can make it something like
> gpios and have it be <whatever>-syscon or have syscon-names property
> if you want to give it a name.

This could work.

> 143 bespoke properties all to say "here are some registers I need to
> twiddle that's outside my regmap" doesn't seem great.

Why? 143 of these registers are not the same.

>
>>>
>>> Side note 2:
>>>
>>> How are we making sure that it's the exynos-pmu driver that ends up
>>> probing the PMU and not the generic syscon driver? Both of these are
>>> platform drivers. And the exynos PMU device lists both the exynos
>>> compatible string and the syscon property. Is it purely a link order
>>> coincidence?
>>
>> initcall ordering
>
> Both these drivers usr postcore_initcall(). So it's purely because
> soc/ is listed earlier in drivers/Makefile than mfd/. And as soon as

Oh... great :/.

> drivers are made into modules this is going to break. This is
> terrible. If you want to have a modular system, this is going to throw
> in a wrench.

Best regards,
Krzysztof

2024-01-25 11:06:54

[permalink] [raw]

Subject: Re: [PATCH 2/9] soc: samsung: exynos-pmu: Add exynos_pmu_update/read/write APIs and SoC quirks

Hi Sam,

Thanks for your review feedback!

On Wed, 24 Jan 2024 at 20:23, Sam Protsenko <[email protected]> wrote:
>
> On Wed, Jan 24, 2024 at 4:02 AM Peter Griffin <[email protected]> wrote:
> >
> > Hi Sam,
> >
> > Thanks for the review feedback.
> >
> > On Tue, 23 Jan 2024 at 18:56, Sam Protsenko <[email protected]> wrote:
> > >
> > > On Mon, Jan 22, 2024 at 4:57 PM Peter Griffin <[email protected]> wrote:
> > > >
> > > > Newer Exynos SoCs have atomic set/clear bit hardware for PMU registers as
> > > > these registers can be accessed by multiple masters. Some platforms also
> > > > protect the PMU registers for security hardening reasons so they can't be
> > > > written by normal world and are only write acessible in el3 via a SMC call.
> > > >
> > > > Add support for both of these usecases using SoC specific quirks that are
> > > > determined from the DT compatible string.
> > > >
> > > > Drivers which need to read and write PMU registers should now use these
> > > > new exynos_pmu_*() APIs instead of obtaining a regmap using
> > > > syscon_regmap_lookup_by_phandle()
> > > >
> > > > Depending on the SoC specific quirks, the exynos_pmu_*() APIs will access
> > > > the PMU register in the appropriate way.
> > > >
> > > > Signed-off-by: Peter Griffin <[email protected]>
> > > > ---
> > > > drivers/soc/samsung/exynos-pmu.c | 209 ++++++++++++++++++++++++-
> > > > drivers/soc/samsung/exynos-pmu.h | 4 +
> > > > include/linux/soc/samsung/exynos-pmu.h | 28 ++++
> > > > 3 files changed, 234 insertions(+), 7 deletions(-)
> > > >
> > >
> > > [snip]
> > >
> > > > +
> > > > +int exynos_pmu_update_bits(unsigned int offset, unsigned int mask,
> > > > + unsigned int val)
> > > > +{
> > > > + if (pmu_context->pmu_data &&
> > > > + pmu_context->pmu_data->quirks & QUIRK_PMU_ALIVE_WRITE_SEC)
> > > > + return rmw_priv_reg(pmu_context->pmu_base_pa + offset,
> > > > + mask, val);
> > > > +
> > > > + return regmap_update_bits(pmu_context->pmureg, offset, mask, val);
> > > > +}
> > > > +EXPORT_SYMBOL(exynos_pmu_update_bits);
> > > > +
> > >
> > > This seems a bit hacky, from the design perspective. This way the user
> > > will have to worry about things like driver dependencies, making sure
> > > everything is instantiated in a correct order, etc. It also hides the
> > > details otherwise visible through "syscon-phandle" property in the
> > > device tree.
> >
> > In v2 I will keep the phandle to pmu_system_controller in DT, and add
> > some -EPROBE_DEFER logic (See my email with Krzysztof).
> >
> > > Can we instead rework it by overriding regmap
> > > implementation for Exynos specifics, and then continue to use it in
> > > the leaf drivers via "syscon-phandle" property?
> >
> > I did look at that possibility first, as like you say it would avoid
> > updating the leaf drivers to use the new API. Unfortunately a SMC
> > backend to regmap was already tried and nacked upstream pretty hard.
> > See here https://lore.kernel.org/lkml/[email protected]/T/
> >
>
> Oh, I didn't mean creating a new regmap implementation :) To
> illustrate what I meant, please look at these:
>
> - drivers/mfd/altera-sysmgr.c
> - altr_sysmgr_regmap_lookup_by_phandle()
> - arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
> - drivers/mmc/host/dw_mmc-pltfm.c

Thanks for the pointers :) I hadn't spotted this when looking
previously. I did find the previous threads I linked to and (it
appears wrongly concluded) that such a regmap SMC would not be
acceptable.
>
> They basically implement their own regmap operations (with smcc too)
> in their syscon implementation. So they can actually reference that
> syscon as phandle in device tree and avoid exporting and calling
> read/write operations (which I think looks hacky). Instead they use
> altr_sysmgr_regmap_lookup_by_phandle() to get their regmap (which
> performs smcc), and then they just use regular regmap_read() /
> regmap_write or whatever functions to operate on their regmap object.
> That's what I meant by "overriding" the regmap.
>
> Do you think this approach would be clearer and more "productizable"
> so to speak? Just a thought.

Keeping it as a regmap was certainly always my preference. I'll try
and re-work it in a similar way and see if I hit any blocking issues.

Thanks,

Peter.

2024-01-25 11:46:24

by Lee Jones

[permalink] [raw]

Subject: Re: [PATCH 3/9] watchdog: s3c2410_wdt: update to use new exynos_pmu_*() apis

On Thu, 25 Jan 2024, Krzysztof Kozlowski wrote:

> On 24/01/2024 22:27, Saravana Kannan wrote:
> > On Tue, Jan 23, 2024 at 10:27 PM Krzysztof Kozlowski
> > <[email protected]> wrote:
> >>
> >> On 24/01/2024 04:37, Saravana Kannan wrote:
> >>> On Tue, Jan 23, 2024 at 10:12 AM Krzysztof Kozlowski
> >>> <[email protected]> wrote:
> >>>>
> >>>> On 23/01/2024 18:30, Peter Griffin wrote:
> >>>>>>> dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
> >>>>>>> else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
> >>>>>>> @@ -698,14 +699,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> >>>>>>> if (ret)
> >>>>>>> return ret;
> >>>>>>>
> >>>>>>> - if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
> >>>>>>> - wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
> >>>>>>> - "samsung,syscon-phandle");
> >>>>>>> - if (IS_ERR(wdt->pmureg))
> >>>>>>> - return dev_err_probe(dev, PTR_ERR(wdt->pmureg),
> >>>>>>> - "syscon regmap lookup failed.\n");
> >>>>>>
> >>>>>>
> >>>>>> Continuing topic from the binding: I don't see how you handle probe
> >>>>>> deferral, suspend ordering.
> >>>>>
> >>>>> The current implementation is simply relying on exynos-pmu being
> >>>>> postcore_initcall level.
> >>>>>
> >>>>> I was just looking around for any existing Linux APIs that could be a
> >>>>> more robust solution. It looks like
> >>>>>
> >>>>> of_parse_phandle()
> >>>>> and
> >>>>> of_find_device_by_node();
> >>>>>
> >>>>> Are often used to solve this type of probe deferral issue between
> >>>>> devices. Is that what you would recommend using? Or is there something
> >>>>> even better?
> >>>>
> >>>> I think you should keep the phandle and then set device link based on
> >>>> of_find_device_by_node(). This would actually improve the code, because
> >>>> syscon_regmap_lookup_by_phandle() does not create device links.
> >>>
> >>> I kinda agree with this. Just because we no longer use a syscon API to
> >>> find the PMU register address doesn't mean the WDT doesn't depend on
> >>> the PMU.
> >>>
> >>> However, I think we should move to a generic "syscon" property. Then I
> >>> can add support for "syscon" property to fw_devlink and then things
> >>> will just work in terms of probe ordering, suspend/resume and also
> >>> showing the dependency in DT even if you don't use the syscon APIs.
> >>>
> >>> Side note 1:
> >>>
> >>> I think we really should officially document a generic syscon DT
> >>> property similar to how we have a generic "clocks" or "dmas" property.
> >>> Then we can have a syscon_get_regmap() that's like so:
> >>>
> >>> struct regmap *syscon_get_regmap(struct device *dev)
> >>> {
> >>> return syscon_regmap_lookup_by_phandle(dev->of_node, "syscon");
> >>> }
> >>>
> >>> Instead of every device defining its own bespoke DT property to do the
> >>> exact same thing. I did a quick "back of the envelope" grep on this
> >>> and I get about 143 unique properties just to get the syscon regmap.
> >>> $ git grep -A1 syscon_regmap_lookup_by_phandle | grep '"' | sed -e
> >>> 's/^[^"]*//' -e 's/"[^"]*$/"/' | sort | uniq | wc -l
> >>> 143
> >>
> >> Sorry, generic "syscon" property won't fly with DT maintainers, because
> >> there is no such thing as syscon in any of hardware.
> >
> > Then why do we allow a "syscon" compatible string and nodes? If the
>
> To bind Linux drivers.
>
> > "syscon" property isn't clear enough, we can make it something like
> > gpios and have it be <whatever>-syscon or have syscon-names property
> > if you want to give it a name.
>
> This could work.

I'm not opposed to this idea. The issue you'll have is keeping the
kernel backwards compatible with older DTBs, thus this solution may only
be possible for newly created bindings. More than happy to be proven
wrong here though.

> >>> How are we making sure that it's the exynos-pmu driver that ends up
> >>> probing the PMU and not the generic syscon driver? Both of these are
> >>> platform drivers. And the exynos PMU device lists both the exynos
> >>> compatible string and the syscon property. Is it purely a link order
> >>> coincidence?
> >>
> >> initcall ordering
> >
> > Both these drivers usr postcore_initcall(). So it's purely because
> > soc/ is listed earlier in drivers/Makefile than mfd/. And as soon as
>
> Oh... great :/.

Agree.

Even using initcalls for ordering is fragile. Relying on the
lexicographical order of a directory / filename structure is akin to
rolling a dice. It would be far nicer if you are able to find a more
robust method of ensuring load order e.g. dynamically poking at
hardware and / or utilising -EPROBE_DEFER.

--
Lee Jones [李琼斯]

2024-01-25 13:20:13

[permalink] [raw]

Subject: Re: [PATCH 3/9] watchdog: s3c2410_wdt: update to use new exynos_pmu_*() apis

Hi Saravana,

Thanks for the feedback!

On Wed, 24 Jan 2024 at 21:27, Saravana Kannan <[email protected]> wrote:
>
> On Tue, Jan 23, 2024 at 10:27 PM Krzysztof Kozlowski
> <[email protected]> wrote:
> >
> > On 24/01/2024 04:37, Saravana Kannan wrote:
> > > On Tue, Jan 23, 2024 at 10:12 AM Krzysztof Kozlowski
> > > <[email protected]> wrote:
> > >>
> > >> On 23/01/2024 18:30, Peter Griffin wrote:
> > >>>>> dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
> > >>>>> else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
> > >>>>> @@ -698,14 +699,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> > >>>>> if (ret)
> > >>>>> return ret;
> > >>>>>
> > >>>>> - if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
> > >>>>> - wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
> > >>>>> - "samsung,syscon-phandle");
> > >>>>> - if (IS_ERR(wdt->pmureg))
> > >>>>> - return dev_err_probe(dev, PTR_ERR(wdt->pmureg),
> > >>>>> - "syscon regmap lookup failed.\n");
> > >>>>
> > >>>>
> > >>>> Continuing topic from the binding: I don't see how you handle probe
> > >>>> deferral, suspend ordering.
> > >>>
> > >>> The current implementation is simply relying on exynos-pmu being
> > >>> postcore_initcall level.
> > >>>
> > >>> I was just looking around for any existing Linux APIs that could be a
> > >>> more robust solution. It looks like
> > >>>
> > >>> of_parse_phandle()
> > >>> and
> > >>> of_find_device_by_node();
> > >>>
> > >>> Are often used to solve this type of probe deferral issue between
> > >>> devices. Is that what you would recommend using? Or is there something
> > >>> even better?
> > >>
> > >> I think you should keep the phandle and then set device link based on
> > >> of_find_device_by_node(). This would actually improve the code, because
> > >> syscon_regmap_lookup_by_phandle() does not create device links.
> > >
> > > I kinda agree with this. Just because we no longer use a syscon API to
> > > find the PMU register address doesn't mean the WDT doesn't depend on
> > > the PMU.
> > >
> > > However, I think we should move to a generic "syscon" property. Then I
> > > can add support for "syscon" property to fw_devlink and then things
> > > will just work in terms of probe ordering, suspend/resume and also
> > > showing the dependency in DT even if you don't use the syscon APIs.
> > >
> > > Side note 1:
> > >
> > > I think we really should officially document a generic syscon DT
> > > property similar to how we have a generic "clocks" or "dmas" property.
> > > Then we can have a syscon_get_regmap() that's like so:
> > >
> > > struct regmap *syscon_get_regmap(struct device *dev)
> > > {
> > > return syscon_regmap_lookup_by_phandle(dev->of_node, "syscon");
> > > }
> > >
> > > Instead of every device defining its own bespoke DT property to do the
> > > exact same thing. I did a quick "back of the envelope" grep on this
> > > and I get about 143 unique properties just to get the syscon regmap.
> > > $ git grep -A1 syscon_regmap_lookup_by_phandle | grep '"' | sed -e
> > > 's/^[^"]*//' -e 's/"[^"]*$/"/' | sort | uniq | wc -l
> > > 143
> >
> > Sorry, generic "syscon" property won't fly with DT maintainers, because
> > there is no such thing as syscon in any of hardware.
>
> Then why do we allow a "syscon" compatible string and nodes? If the
> "syscon" property isn't clear enough, we can make it something like
> gpios and have it be <whatever>-syscon or have syscon-names property
> if you want to give it a name.
> 143 bespoke properties all to say "here are some registers I need to
> twiddle that's outside my regmap" doesn't seem great.

Some sort of standardization on the naming seems like a good idea to
me. Especially if it then means fw_devlink support can be added.

>
> > >
> > > Side note 2:
> > >
> > > How are we making sure that it's the exynos-pmu driver that ends up
> > > probing the PMU and not the generic syscon driver? Both of these are
> > > platform drivers. And the exynos PMU device lists both the exynos
> > > compatible string and the syscon property. Is it purely a link order
> > > coincidence?
> >
> > initcall ordering
>
> Both these drivers usr postcore_initcall(). So it's purely because
> soc/ is listed earlier in drivers/Makefile than mfd/. And as soon as
> drivers are made into modules this is going to break. This is
> terrible. If you want to have a modular system, this is going to throw
> in a wrench.
>

That does look to be a bug, or fragility at least with the current
upstream exynos-pmu driver. I think upstream Exynos most likely hasn't
encountered these types of issues because ARCH_EXYNOS has these
drivers as built-in, and as you say the alphabetical ordering in the
Makefile.

regards,

Peter.

2024-01-26 02:18:21

[permalink] [raw]

Subject: Re: [PATCH 3/9] watchdog: s3c2410_wdt: update to use new exynos_pmu_*() apis

On Wed, Jan 24, 2024 at 11:37 PM Krzysztof Kozlowski
<[email protected]> wrote:
>
> On 24/01/2024 22:27, Saravana Kannan wrote:
> > On Tue, Jan 23, 2024 at 10:27 PM Krzysztof Kozlowski
> > <[email protected]> wrote:
> >>
> >> On 24/01/2024 04:37, Saravana Kannan wrote:
> >>> On Tue, Jan 23, 2024 at 10:12 AM Krzysztof Kozlowski
> >>> <[email protected]> wrote:
> >>>>
> >>>> On 23/01/2024 18:30, Peter Griffin wrote:
> >>>>>>> dev_warn(wdt->dev, "Couldn't get RST_STAT register\n");
> >>>>>>> else if (rst_stat & BIT(wdt->drv_data->rst_stat_bit))
> >>>>>>> @@ -698,14 +699,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> >>>>>>> if (ret)
> >>>>>>> return ret;
> >>>>>>>
> >>>>>>> - if (wdt->drv_data->quirks & QUIRKS_HAVE_PMUREG) {
> >>>>>>> - wdt->pmureg = syscon_regmap_lookup_by_phandle(dev->of_node,
> >>>>>>> - "samsung,syscon-phandle");
> >>>>>>> - if (IS_ERR(wdt->pmureg))
> >>>>>>> - return dev_err_probe(dev, PTR_ERR(wdt->pmureg),
> >>>>>>> - "syscon regmap lookup failed.\n");
> >>>>>>
> >>>>>>
> >>>>>> Continuing topic from the binding: I don't see how you handle probe
> >>>>>> deferral, suspend ordering.
> >>>>>
> >>>>> The current implementation is simply relying on exynos-pmu being
> >>>>> postcore_initcall level.
> >>>>>
> >>>>> I was just looking around for any existing Linux APIs that could be a
> >>>>> more robust solution. It looks like
> >>>>>
> >>>>> of_parse_phandle()
> >>>>> and
> >>>>> of_find_device_by_node();
> >>>>>
> >>>>> Are often used to solve this type of probe deferral issue between
> >>>>> devices. Is that what you would recommend using? Or is there something
> >>>>> even better?
> >>>>
> >>>> I think you should keep the phandle and then set device link based on
> >>>> of_find_device_by_node(). This would actually improve the code, because
> >>>> syscon_regmap_lookup_by_phandle() does not create device links.
> >>>
> >>> I kinda agree with this. Just because we no longer use a syscon API to
> >>> find the PMU register address doesn't mean the WDT doesn't depend on
> >>> the PMU.
> >>>
> >>> However, I think we should move to a generic "syscon" property. Then I
> >>> can add support for "syscon" property to fw_devlink and then things
> >>> will just work in terms of probe ordering, suspend/resume and also
> >>> showing the dependency in DT even if you don't use the syscon APIs.
> >>>
> >>> Side note 1:
> >>>
> >>> I think we really should officially document a generic syscon DT
> >>> property similar to how we have a generic "clocks" or "dmas" property.
> >>> Then we can have a syscon_get_regmap() that's like so:
> >>>
> >>> struct regmap *syscon_get_regmap(struct device *dev)
> >>> {
> >>> return syscon_regmap_lookup_by_phandle(dev->of_node, "syscon");
> >>> }
> >>>
> >>> Instead of every device defining its own bespoke DT property to do the
> >>> exact same thing. I did a quick "back of the envelope" grep on this
> >>> and I get about 143 unique properties just to get the syscon regmap.
> >>> $ git grep -A1 syscon_regmap_lookup_by_phandle | grep '"' | sed -e
> >>> 's/^[^"]*//' -e 's/"[^"]*$/"/' | sort | uniq | wc -l
> >>> 143
> >>
> >> Sorry, generic "syscon" property won't fly with DT maintainers, because
> >> there is no such thing as syscon in any of hardware.
> >
> > Then why do we allow a "syscon" compatible string and nodes? If the
>
> To bind Linux drivers.

My point was that if your statement "there's no such thing as syscon
in any of hardware" is true, then we shouldn't have a syscon node
either because I don't think there's any "syscon" IP block. It's just
a random set of registers grouped together for system control.

>
> > "syscon" property isn't clear enough, we can make it something like
> > gpios and have it be <whatever>-syscon or have syscon-names property
> > if you want to give it a name.
>
> This could work.
>
> > 143 bespoke properties all to say "here are some registers I need to
> > twiddle that's outside my regmap" doesn't seem great.
>
> Why? 143 of these registers are not the same.

You could make the same argument about clocks. None of the "clocks"
listed in DT is the same clock across all devices. But the idea behind
the clocks property is that in DT we are representing that the device
needs clocks and in DT we don't really care how/what it is used for
(the driver deals with that). Similarly "syscon" is a system control
register needed by a device (the why/how is left to the driver).

-Saravana

> >
> >>>
> >>> Side note 2:
> >>>
> >>> How are we making sure that it's the exynos-pmu driver that ends up
> >>> probing the PMU and not the generic syscon driver? Both of these are
> >>> platform drivers. And the exynos PMU device lists both the exynos
> >>> compatible string and the syscon property. Is it purely a link order
> >>> coincidence?
> >>
> >> initcall ordering
> >
> > Both these drivers usr postcore_initcall(). So it's purely because
> > soc/ is listed earlier in drivers/Makefile than mfd/. And as soon as
>
> Oh... great :/.
>
> > drivers are made into modules this is going to break. This is
> > terrible. If you want to have a modular system, this is going to throw
> > in a wrench.
>
> Best regards,
> Krzysztof
>

2024-01-26 02:24:40