On Fri, 18 Nov 2022 at 14:35, Walker Chen <walker.chen@xxxxxxxxxxxxxxxx> wrote:
>
> Add generic power domain driver for the StarFive JH71XX SoC.
SoCs
> Signed-off-by: Walker Chen <walker.chen@xxxxxxxxxxxxxxxx>
> ---
> MAINTAINERS | 8 +
> drivers/soc/Kconfig | 1 +
> drivers/soc/Makefile | 1 +
> drivers/soc/starfive/Kconfig | 9 +
> drivers/soc/starfive/Makefile | 3 +
> drivers/soc/starfive/jh71xx_pmu.c | 380 ++++++++++++++++++++++++++++++
> include/soc/starfive/pm_domains.h | 15 ++
> 7 files changed, 417 insertions(+)
> create mode 100644 drivers/soc/starfive/Kconfig
> create mode 100644 drivers/soc/starfive/Makefile
> create mode 100644 drivers/soc/starfive/jh71xx_pmu.c
> create mode 100644 include/soc/starfive/pm_domains.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a70c1d0f303e..112f1e698723 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19623,6 +19623,14 @@ F: Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml
> F: drivers/reset/starfive/
> F: include/dt-bindings/reset/starfive*
checkpatch.pl complains about not sorting these lines alphabetically.
> +STARFIVE JH71XX PMU CONTROLLER DRIVER
> +M: Walker Chen <walker.chen@xxxxxxxxxxxxxxxx>
> +S: Maintained
> +F: Documentation/devicetree/bindings/power/starfive*
> +F: drivers/soc/starfive/jh71xx_pmu.c
> +F: include/soc/starfive/pm_domains.h
> +F: include/dt-bindings/power/jh7110-power.h
> +
> STATIC BRANCH/CALL
> M: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> M: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> index e461c071189b..628fda4d5ed9 100644
> --- a/drivers/soc/Kconfig
> +++ b/drivers/soc/Kconfig
> @@ -21,6 +21,7 @@ source "drivers/soc/renesas/Kconfig"
> source "drivers/soc/rockchip/Kconfig"
> source "drivers/soc/samsung/Kconfig"
> source "drivers/soc/sifive/Kconfig"
> +source "drivers/soc/starfive/Kconfig"
> source "drivers/soc/sunxi/Kconfig"
> source "drivers/soc/tegra/Kconfig"
> source "drivers/soc/ti/Kconfig"
> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index 534669840858..cbe076f42068 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -27,6 +27,7 @@ obj-y += renesas/
> obj-y += rockchip/
> obj-$(CONFIG_SOC_SAMSUNG) += samsung/
> obj-y += sifive/
> +obj-y += starfive/
> obj-y += sunxi/
> obj-$(CONFIG_ARCH_TEGRA) += tegra/
> obj-y += ti/
> diff --git a/drivers/soc/starfive/Kconfig b/drivers/soc/starfive/Kconfig
> new file mode 100644
> index 000000000000..2bbcc1397b15
> --- /dev/null
> +++ b/drivers/soc/starfive/Kconfig
> @@ -0,0 +1,9 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +config JH71XX_PMU
> + bool "Support PMU for StarFive JH71XX Soc"
> + depends on PM && (SOC_STARFIVE || COMPILE_TEST)
> + select PM_GENERIC_DOMAINS
> + help
> + Say y here to enable power domain support.
Please expand this help text. Even checkpatch.pl complains about it.
> +
> diff --git a/drivers/soc/starfive/Makefile b/drivers/soc/starfive/Makefile
> new file mode 100644
> index 000000000000..13b589d6b5f3
> --- /dev/null
> +++ b/drivers/soc/starfive/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-$(CONFIG_JH71XX_PMU) += jh71xx_pmu.o
> diff --git a/drivers/soc/starfive/jh71xx_pmu.c b/drivers/soc/starfive/jh71xx_pmu.c
> new file mode 100644
> index 000000000000..e6c0083d166e
> --- /dev/null
> +++ b/drivers/soc/starfive/jh71xx_pmu.c
> @@ -0,0 +1,380 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * StarFive JH71XX Power Domain Controller Driver
> + *
> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <dt-bindings/power/jh7110-power.h>
> +
> +/* register offset */
> +#define HW_EVENT_TURN_ON_MASK 0x04
> +#define HW_EVENT_TURN_OFF_MASK 0x08
> +#define SW_TURN_ON_POWER_MODE 0x0C
> +#define SW_TURN_OFF_POWER_MODE 0x10
> +#define SW_ENCOURAGE 0x44
> +#define PMU_INT_MASK 0x48
> +#define PCH_BYPASS 0x4C
> +#define PCH_PSTATE 0x50
> +#define PCH_TIMEOUT 0x54
> +#define LP_TIMEOUT 0x58
> +#define HW_TURN_ON_MODE 0x5C
> +#define CURR_POWER_MODE 0x80
> +#define PMU_EVENT_STATUS 0x88
> +#define PMU_INT_STATUS 0x8C
> +
> +/* sw encourage cfg */
> +#define SW_MODE_ENCOURAGE_EN_LO 0x05
> +#define SW_MODE_ENCOURAGE_EN_HI 0x50
> +#define SW_MODE_ENCOURAGE_DIS_LO 0x0A
> +#define SW_MODE_ENCOURAGE_DIS_HI 0xA0
> +#define SW_MODE_ENCOURAGE_ON 0xFF
> +
> +/* pmu int status */
> +#define PMU_INT_SEQ_DONE BIT(0)
> +#define PMU_INT_HW_REQ BIT(1)
> +#define PMU_INT_SW_FAIL GENMASK(3, 2)
> +#define PMU_INT_HW_FAIL GENMASK(5, 4)
> +#define PMU_INT_PCH_FAIL GENMASK(8, 6)
> +#define PMU_INT_FAIL_MASK (PMU_INT_SW_FAIL | \
> + PMU_INT_HW_FAIL | \
> + PMU_INT_PCH_FAIL)
> +#define PMU_INT_ALL_MASK (PMU_INT_SEQ_DONE | \
> + PMU_INT_HW_REQ | \
> + PMU_INT_FAIL_MASK)
> +
> +#define DELAY_US 10
> +#define TIMEOUT_US 100000
Please add a JH71XX_PMU_ prefix to these definitions to avoid
accidentally clashing with a definition added to a generic header.
> +
> +struct starfive_power_dev {
> + struct generic_pm_domain genpd;
> + struct starfive_pmu *power;
> + uint32_t mask;
u32
> +};
> +
> +struct starfive_pmu {
> + void __iomem *base;
> + spinlock_t lock;
> + int irq;
> + struct device *pdev;
> + struct starfive_power_dev *dev;
> + struct genpd_onecell_data genpd_data;
> + struct generic_pm_domain **genpd;
> +};
> +
> +struct starfive_pmu_data {
> + const char * const name;
> + uint8_t bit;
u8
> + unsigned int flags;
> +};
> +
> +static void __iomem *pmu_base;
> +
> +static inline void pmu_writel(u32 val, u32 offset)
> +{
> + writel(val, pmu_base + offset);
> +}
> +
> +void starfive_pmu_hw_event_turn_on(u32 mask)
> +{
> + pmu_writel(mask, HW_EVENT_TURN_ON_MASK);
> +}
> +EXPORT_SYMBOL(starfive_pmu_hw_event_turn_on);
> +
> +void starfive_pmu_hw_event_turn_off(u32 mask)
> +{
> + pmu_writel(mask, HW_EVENT_TURN_OFF_MASK);
> +}
> +EXPORT_SYMBOL(starfive_pmu_hw_event_turn_off);
> +
> +static int starfive_pmu_get_state(struct starfive_power_dev *pmd, bool *is_on)
> +{
> + struct starfive_pmu *pmu = pmd->power;
> +
> + if (!pmd->mask) {
> + *is_on = false;
> + return -EINVAL;
> + }
> +
> + *is_on = __raw_readl(pmu->base + CURR_POWER_MODE) & pmd->mask;
> +
> + return 0;
> +}
Maybe just return an int where 0 = off, 1 = on and negative is an error.
> +
> +static int starfive_pmu_set_state(struct starfive_power_dev *pmd, bool on)
> +{
> + struct starfive_pmu *pmu = pmd->power;
> + unsigned long flags;
> + uint32_t val;
> + uint32_t mode;
> + uint32_t encourage_lo;
> + uint32_t encourage_hi;
u32
> + bool is_on;
> + int ret;
> +
> + if (!pmd->mask)
> + return -EINVAL;
> +
> + if (is_on == on) {
> + dev_info(pmu->pdev, "pm domain [%s] is already %sable status.\n",
> + pmd->genpd.name, on ? "en" : "dis");
> + return 0;
> + }
> +
> + spin_lock_irqsave(&pmu->lock, flags);
> +
> + if (on) {
> + mode = SW_TURN_ON_POWER_MODE;
> + encourage_lo = SW_MODE_ENCOURAGE_EN_LO;
> + encourage_hi = SW_MODE_ENCOURAGE_EN_HI;
> + } else {
> + mode = SW_TURN_OFF_POWER_MODE;
> + encourage_lo = SW_MODE_ENCOURAGE_DIS_LO;
> + encourage_hi = SW_MODE_ENCOURAGE_DIS_HI;
> + }
> +
> + __raw_writel(pmd->mask, pmu->base + mode);
> +
> + /* write SW_ENCOURAGE to make the configuration take effect */
> + __raw_writel(SW_MODE_ENCOURAGE_ON, pmu->base + SW_ENCOURAGE);
> + __raw_writel(encourage_lo, pmu->base + SW_ENCOURAGE);
> + __raw_writel(encourage_hi, pmu->base + SW_ENCOURAGE);
> +
> + spin_unlock_irqrestore(&pmu->lock, flags);
> +
> + if (on) {
> + ret = readl_poll_timeout_atomic(pmu->base + CURR_POWER_MODE, val,
> + val & pmd->mask, DELAY_US,
> + TIMEOUT_US);
> + if (ret) {
> + dev_err(pmu->pdev, "%s: failed to power on\n", pmd->genpd.name);
> + return -ETIMEDOUT;
> + }
> + } else {
> + ret = readl_poll_timeout_atomic(pmu->base + CURR_POWER_MODE, val,
> + !(val & pmd->mask), DELAY_US,
> + TIMEOUT_US);
> + if (ret) {
> + dev_err(pmu->pdev, "%s: failed to power off\n", pmd->genpd.name);
> + return -ETIMEDOUT;
> + }
> + }
You can move the dev_err out. Eg. dev_err(pmu->pdev, "%s: failed to
power %s\n", pmd->genpd.name, on ? "on" : "off")
> + return 0;
> +}
> +
> +static int starfive_pmu_on(struct generic_pm_domain *genpd)
> +{
> + struct starfive_power_dev *pmd = container_of(genpd,
> + struct starfive_power_dev, genpd);
checkpatch.pl --strict complains that arguments doesn't match the open
parenthesis
> +
> + return starfive_pmu_set_state(pmd, true);
> +}
> +
> +static int starfive_pmu_off(struct generic_pm_domain *genpd)
> +{
> + struct starfive_power_dev *pmd = container_of(genpd,
> + struct starfive_power_dev, genpd);
> +
> + return starfive_pmu_set_state(pmd, false);
> +}
> +
> +static void starfive_pmu_int_enable(struct starfive_pmu *pmu, u32 mask, bool enable)
> +{
> + u32 val = __raw_readl(pmu->base + PMU_INT_MASK);
> +
> + if (enable)
> + val &= ~mask;
> + else
> + val |= mask;
> +
> + __raw_writel(val, pmu->base + PMU_INT_MASK);
> +}
Consider adding comments to functions that need a lock to be held to
be used safely.
> +
> +static irqreturn_t starfive_pmu_interrupt(int irq, void *data)
> +{
> + struct starfive_pmu *pmu = data;
> + unsigned long flags;
> + u32 val;
> +
> + spin_lock_irqsave(&pmu->lock, flags);
> + val = __raw_readl(pmu->base + PMU_INT_STATUS);
> +
> + if (val & PMU_INT_SEQ_DONE)
> + dev_dbg(pmu->pdev, "sequence done.\n");
> + if (val & PMU_INT_HW_REQ)
> + dev_dbg(pmu->pdev, "hardware encourage requestion.\n");
> + if (val & PMU_INT_SW_FAIL)
> + dev_err(pmu->pdev, "software encourage fail.\n");
> + if (val & PMU_INT_HW_FAIL)
> + dev_err(pmu->pdev, "hardware encourage fail.\n");
> + if (val & PMU_INT_PCH_FAIL)
> + dev_err(pmu->pdev, "p-channel fail event.\n");
> +
> + /* clear interrupts */
> + __raw_writel(val, pmu->base + PMU_INT_STATUS);
> + __raw_writel(val, pmu->base + PMU_EVENT_STATUS);
> +
> + spin_unlock_irqrestore(&pmu->lock, flags);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int starfive_pmu_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
> + const struct starfive_pmu_data *entry, *table;
> + struct starfive_pmu *pmu;
> + unsigned int i;
> + uint8_t max_bit = 0;
> + int ret;
> +
> + pmu = devm_kzalloc(dev, sizeof(*pmu), GFP_KERNEL);
> + if (!pmu)
> + return -ENOMEM;
> +
> + pmu_base = pmu->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(pmu->base))
> + return PTR_ERR(pmu->base);
> +
> + /* initialize pmu interrupt */
> + pmu->irq = platform_get_irq(pdev, 0);
> + if (pmu->irq < 0)
> + return pmu->irq;
> +
> + ret = devm_request_irq(dev, pmu->irq, starfive_pmu_interrupt,
> + 0, pdev->name, pmu);
> + if (ret)
> + dev_err(dev, "request irq failed.\n");
> +
> + table = of_device_get_match_data(dev);
> + if (!table)
> + return -EINVAL;
> +
> + pmu->pdev = dev;
> + pmu->genpd_data.num_domains = 0;
> + i = 0;
> + for (entry = table; entry->name; entry++) {
> + max_bit = max(max_bit, entry->bit);
> + i++;
> + }
> +
> + if (!i)
> + return -ENODEV;
> +
> + pmu->genpd_data.num_domains = max_bit + 1;
> +
> + pmu->dev = devm_kcalloc(dev, pmu->genpd_data.num_domains,
> + sizeof(struct starfive_power_dev),
> + GFP_KERNEL);
> + if (!pmu->dev)
> + return -ENOMEM;
> +
> + pmu->genpd = devm_kcalloc(dev, pmu->genpd_data.num_domains,
> + sizeof(struct generic_pm_domain *),
> + GFP_KERNEL);
> + if (!pmu->genpd)
> + return -ENOMEM;
> +
> + pmu->genpd_data.domains = pmu->genpd;
> +
> + i = 0;
> + for (entry = table; entry->name; entry++) {
> + struct starfive_power_dev *pmd = &pmu->dev[i];
> + bool is_on;
> +
> + pmd->power = pmu;
> + pmd->mask = BIT(entry->bit);
> + pmd->genpd.name = entry->name;
> + pmd->genpd.flags = entry->flags;
> +
> + ret = starfive_pmu_get_state(pmd, &is_on);
> + if (ret)
> + dev_warn(dev, "unable to get current state for %s\n",
> + pmd->genpd.name);
> +
> + pmd->genpd.power_on = starfive_pmu_on;
> + pmd->genpd.power_off = starfive_pmu_off;
> +
> + pm_genpd_init(&pmd->genpd, NULL, !is_on);
> + pmu->genpd[entry->bit] = &pmd->genpd;
> +
> + i++;
> + }
> +
> + spin_lock_init(&pmu->lock);
> + starfive_pmu_int_enable(pmu, PMU_INT_ALL_MASK & ~PMU_INT_PCH_FAIL, true);
> +
> + ret = of_genpd_add_provider_onecell(np, &pmu->genpd_data);
> + if (ret) {
> + dev_err(dev, "failed to register genpd driver: %d\n", ret);
> + return ret;
> + }
> +
> + dev_info(dev, "registered %u power domains\n", i);
> +
> + return 0;
> +}
> +
> +static const struct starfive_pmu_data jh7110_power_domains[] = {
> + {
> + .name = "SYSTOP",
> + .bit = JH7110_PD_SYSTOP,
> + .flags = GENPD_FLAG_ALWAYS_ON,
> + }, {
> + .name = "CPU",
> + .bit = JH7110_PD_CPU,
> + .flags = GENPD_FLAG_ALWAYS_ON,
> + }, {
> + .name = "GPUA",
> + .bit = JH7110_PD_GPUA,
> + }, {
> + .name = "VDEC",
> + .bit = JH7110_PD_VDEC,
> + }, {
> + .name = "VOUT",
> + .bit = JH7110_PD_VOUT,
> + }, {
> + .name = "ISP",
> + .bit = JH7110_PD_ISP,
> + }, {
> + .name = "VENC",
> + .bit = JH7110_PD_VENC,
> + }, {
> + .name = "GPUB",
> + .bit = JH7110_PD_GPUB,
> + }, {
> + /* sentinel */
> + },
> +};
> +
> +static const struct of_device_id starfive_pmu_of_match[] = {
> + {
> + .compatible = "starfive,jh7110-pmu",
> + .data = &jh7110_power_domains,
> + }, {
> + /* sentinel */
> + }
> +};
> +
> +static struct platform_driver jh71xx_pmu_driver = {
> + .driver = {
> + .name = "jh71xx-pmu",
> + .of_match_table = starfive_pmu_of_match,
> + },
> + .probe = starfive_pmu_probe,
> +};
> +builtin_platform_driver(jh71xx_pmu_driver);
You're using a mix of starfive_, starfive_pmu_ and jh71xx_pmu_
prefixes. Please choose one, preferably jh71xx_pmu_ since that's what
you call the driver.
> +MODULE_LICENSE("GPL");
> diff --git a/include/soc/starfive/pm_domains.h b/include/soc/starfive/pm_domains.h
> new file mode 100644
> index 000000000000..a20e28e9baf3
> --- /dev/null
> +++ b/include/soc/starfive/pm_domains.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
> + * Author: Walker Chen <walker.chen@xxxxxxxxxxxxxxxx>
> + */
> +
> +#ifndef __SOC_STARFIVE_PMDOMAINS_H__
> +#define __SOC_STARFIVE_PMDOMAINS_H__
> +
> +#include <linux/types.h>
> +
> +void starfive_pmu_hw_event_turn_on(u32 mask);
> +void starfive_pmu_hw_event_turn_off(u32 mask);
> +
> +#endif /* __SOC_STARFIVE_PMDOMAINS_H__ */
The header and functions within are named very generic, but
implemented by the jh71xx-specific driver.
Also who should use this header? These functions are never called by
anything in this series.
> --
> 2.17.1
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-riscv
