Message ID | 20221118133216.17037-4-walker.chen@starfivetech.com |
---|---|
State | New |
Headers | show |
Series | JH7110 Power Domain Support | expand |
On Fri, 18 Nov 2022 at 14:35, Walker Chen <walker.chen@starfivetech.com> wrote: > > Add generic power domain driver for the StarFive JH71XX SoC. SoCs > Signed-off-by: Walker Chen <walker.chen@starfivetech.com> > --- > 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@starfivetech.com> > +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@infradead.org> > M: Josh Poimboeuf <jpoimboe@kernel.org> > 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@starfivetech.com> > + */ > + > +#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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
Hey Walker, On Fri, Nov 18, 2022 at 09:32:15PM +0800, Walker Chen wrote: > Add generic power domain driver for the StarFive JH71XX SoC. > > Signed-off-by: Walker Chen <walker.chen@starfivetech.com> > --- > 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) `default SOC_STARFIVE` perhaps? > + select PM_GENERIC_DOMAINS > + help > + Say y here to enable power domain support. > + > 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 Some JH71XX_PMU_ prefixes for the above, as I think Emil pointed out, would be great. > +struct starfive_power_dev { > + struct generic_pm_domain genpd; > + struct starfive_pmu *power; > + uint32_t mask; > +}; > + > +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; > + unsigned int flags; Generally, ordering pointers first in these structs would be nice. > +}; > + > +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); Where are the users for these exports? Also, should they be exported as GPL? Either way, what is the point of the extra layer of abstraction here around the writel()? > +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; Is there a specific reason that you are using the __raw variants here (and elsewhere) in the driver? > + > + return 0; > +} > + > +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; > + 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"); Am I missing something here: you've just declared is_on, so it must be false & therefore this check is all a little pointless? The check just becomes if (false == on) which I don't think is what you're going for here? Or did I miss something obvious? > + 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); Is register "sticky"? IOW, could you set it in probe and leave this mode always on? Or does it need to be set every time you want to use this feature? > + __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); Could you not just decide the 3rd arg outside of the readl_poll..() and save on duplicating the wait logic here? > + if (ret) { > + dev_err(pmu->pdev, "%s: failed to power off\n", pmd->genpd.name); > + return -ETIMEDOUT; > + } > + } > + > + 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); > + > + 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); > +} > + > +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++; > + } This looks like something that could be replaced by the functions in linux/list.h, no? Same below. > + > + 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); This two allocations both look like checkpatch would whinge about their alignment. > + 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); In what scenario would it not be possible to get the state? Again, I hope I'm not missing something obvious. From what I can see, this is the only caller of starfive_pmu_get_state(), which looks like: > > + struct starfive_pmu *pmu = pmd->power; > > + > > + if (!pmd->mask) { How can !pmd->mask evaluate to true, given it's just been set to BIT(n)? > > + *is_on = false; > > + return -EINVAL; > > + } > > + > > + *is_on = __raw_readl(pmu->base + CURR_POWER_MODE) & pmd->mask; > > + > > + return 0; If I've not missed something, looks like you could likely remove the function and just do the readl() here? > + > + 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[] = { Is this driver jh7110 only or actually jh71XX? Have you just started out by implementing one SoC both intend to support both? > + { > + .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 */ > + }, Can drop this , after the sentinel ;) I don't know /jack/ about power domain stuff, so I can barely review this at all sadly. Thanks, Conor. > +}; > + > +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);
Hi Walker, Walker Chen <walker.chen@starfivetech.com> writes: > Add generic power domain driver for the StarFive JH71XX SoC. > > Signed-off-by: Walker Chen <walker.chen@starfivetech.com> A bit more detail about the features power domain hardware would be helpful for reviewers. I'm assuming there's no public documenation for this, but if there is, a link to that would be great also. [...] > 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 Where do these delay/timeout numbers come from? Is this just based one experimentation, or is this something described by the HW docs. Please add some comments. > + > +struct starfive_power_dev { > + struct generic_pm_domain genpd; > + struct starfive_pmu *power; > + uint32_t mask; > +}; > + > +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; > + unsigned int flags; > +}; > + > +static void __iomem *pmu_base; > + > +static inline void pmu_writel(u32 val, u32 offset) > +{ > + writel(val, pmu_base + offset); > +} You use writel() in this helper, but __raw_writel() throughout the rest of the driver. If that's intentional, you should explain a bit more about why. If not, please make it consistent. If you're going to use a wrapper/helper, you should use it throughout. But in this case, I'm not sure it's adding anything to readability. In fact, it's only adding confusion since you don't use it for most of the register accesses. > +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); This looks wrong. Why are you allowing external users to power on/off the domains? The sate of the domain should be managed by the PM core (and runtime PM use-counts etc.) allowing external users to change the domain state is going to lead to the PM core being confused about the state of the domain. > +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; > +} > + > +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; > + bool is_on; > + int ret; > + > + if (!pmd->mask) > + return -EINVAL; > + > + if (is_on == on) { Comparing an uninitialzed stack variable to 'on'? > + dev_info(pmu->pdev, "pm domain [%s] is already %sable status.\n", > + pmd->genpd.name, on ? "en" : "dis"); Using dev_info() will probably make this a bit verbose. I'd suggest dev_dbg() for this kind of message. > + 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); This "encourage" feature is peculiar. What happens if you do not do this? Does it take a lot longer for the domain to change state? or does it not change at all? In the absence of HW specs, a bit of description here would be helpful. > + 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; > + } > + } > + > + 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); > + > + 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); > +} > + > +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); I don't think you need to spinlock in the interrupt itself, as interrupts will already be disabled, and this handler is not IRQF_SHARED. > + 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; > +} > + [...] Kevin
On 2022/11/25 19:17, Conor Dooley wrote: > On Fri, Nov 25, 2022 at 06:04:59PM +0800, Walker Chen wrote: >> On 2022/11/19 8:24, Conor Dooley wrote: >> > On Fri, Nov 18, 2022 at 09:32:15PM +0800, Walker Chen wrote: > >> >> +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); >> > >> > Where are the users for these exports? Also, should they be exported as >> > GPL? >> > >> > Either way, what is the point of the extra layer of abstraction here >> > around the writel()? >> >> The two export functions are only prepared for GPU module. But accordint to >> the latest information, it seems that there is no open source plan for GPU. >> So the two functions will be drop in next version of patch. > > That's a shame! Need to comply with certain commercial terms. > >> >> +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; >> > >> > Is there a specific reason that you are using the __raw variants here >> > (and elsewhere) in the driver? >> >> Will use unified function '__raw_readl' and '__raw_writel' > > No no, I want to know *why* you are using the __raw accessors here. My > understanding was that __raw variants are unbarriered & unordered with > respect to other io accesses. > > I do notice that the bcm driver you mentioned uses the __raw variants, > but only __raw variants - whereas you use readl() which is ordered and > barriered & __raw_readl(). > > Is there a reason why you would not use readl() or readl_relaxed()? Your question led me to deeply understand the usage of these io accessors. __raw_readl / __raw_writel denotes native byte order, no memory barrier. readl / writel do guarantee the byte order with barrier, ensure that previous writes are done. Seem that non-raw accessors are more safe. > >> >> + >> >> + return 0; >> >> +} >> >> + >> >> +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; >> >> + 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"); >> > >> > Am I missing something here: you've just declared is_on, so it must be >> > false & therefore this check is all a little pointless? The check just >> > becomes if (false == on) which I don't think is what you're going for >> > here? Or did I miss something obvious? >> >> Sorry, indeed I missed several lines of code. It should be witten like this: >> ret = jh71xx_pmu_get_state(pmd, &is_on); >> if (ret) { >> dev_dbg(pmu->pdev, "unable to get current state for %s\n", >> pmd->genpd.name); >> return ret; >> } > > Heh, this looks more sane :) > >> >> if (is_on == on) { >> dev_dbg(pmu->pdev, "pm domain [%s] is already %sable status.\n", >> pmd->genpd.name, on ? "en" : "dis"); >> return 0; >> } >> >> > >> >> + 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); >> > >> > Is register "sticky"? IOW, could you set it in probe and leave this mode >> > always on? Or does it need to be set every time you want to use this >> > feature? >> >> These power domain registers need to be set by other module according to the >> specific situation. >> For example some power domains should be turned off via System PM mechanism >> when system goes to sleep, >> and then they are turned on when system resume. > > I was just wondering if SW_MODE_ENCOURAGE_ON would retain it's value > during operation or if it had to be written every time. It's fine if > that's not the case. Writing SW_MODE_ENCOURAGE_ON (0xFF) to SW_ENCOURAGE register, the purpose is to reset the state machine which is going to parse instruction sequence. It has to be written every time. > >> >> + __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); >> > >> > Could you not just decide the 3rd arg outside of the readl_poll..() and >> > save on duplicating the wait logic here? >> >> Seems that the readl_poll..() can only be called in two cases >> because the CURR_POWER_MODE register is read to val and then operation with mask every time. >> > > I'm sorry, I completely forgot that read*_poll..() actually not actually > a function. Please ignore this comment! > >> >> +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++; >> >> + } >> > >> > This looks like something that could be replaced by the functions in >> > linux/list.h, no? Same below. >> >> Nowadays other platforms on linux mainline mostly write in this way or write like this: >> for (i = 0; i < num; i++) { >> ... >> pm_genpd_init(&pmd[i]->genpd, NULL, !is_on); >> } > > That's not what this specific bit of code is doing though, right? You're > walking jh7110_power_domains to find the highest bit. I was looking at what > some other drivers do, and took a look at drivers/soc/qcom/rpmhpd.c > where they know the size of this struct at compile time & so can do > store the number of power domains in the match data. If you did that, > you could use a loop like the one other platforms use. > >> >> + if (!pmu->genpd) >> >> + return -ENOMEM; >> >> + >> >> + pmu->genpd_data.domains = pmu->genpd; >> >> + >> >> + i = 0; >> >> + for (entry = table; entry->name; entry++) { > > And it's the same here. By now, you should know how many power domains > you have, no? > > Anyways, as I said before I don't know much about this power domain > stuff, it's just that these two loops seem odd. About the usage of loop, I took a look at other platforms like drivers/soc/qcom/rpmhpd.c, maybe that way is simpler and easier to understand. I will try to change to that looping in next version. > >> >> + 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); > >> >> +static const struct starfive_pmu_data jh7110_power_domains[] = { >> > >> > Is this driver jh7110 only or actually jh71XX? Have you just started out >> > by implementing one SoC both intend to support both? >> >> JH7110 is our first SoC, probably the next generation of SoC jh7120 still >> use this controller driver. >> Maybe now the naming for JH71XX is not very suitable. > > Right. My question was more about if this supported the JH7100 too, but > I saw from your answer to Emil that it won't. I don't have a preference > as to whether you call it jh71xx or jh7110, I'll leave that up to > yourselves and Emil. This particular struct should still be called > `jh7110_power_domains` though since it is particular to this SoC, no > matter what you end up calling the file etc. Emil has gave me a good suggestion. > >> > I don't know /jack/ about power domain stuff, so I can barely review >> > this at all sadly. > >> Thank you for taking the time to review the code, you helped me a lot. >> Thank you so much. > > No worries, looking forward to getting my board :) > Have you purchased a VisionFive 2 board online? Do you need the shopping link ? https://forum.rvspace.org/t/how-to-purchase-visionfive-2/665 Best Regards, Walker Chen
Hey Walker, Apologies for my formatting here. On 1 December 2022 03:56:27 GMT, Walker Chen <walker.chen@starfivetech.com> wrote: >On 2022/11/25 19:17, Conor Dooley wrote: >> On Fri, Nov 25, 2022 at 06:04:59PM +0800, Walker Chen wrote: >>> On 2022/11/19 8:24, Conor Dooley wrote: >>> > On Fri, Nov 18, 2022 at 09:32:15PM +0800, Walker Chen wrote: >> >>> >> +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); >>> > >>> > Where are the users for these exports? Also, should they be exported as >>> > GPL? >>> > >>> > Either way, what is the point of the extra layer of abstraction here >>> > around the writel()? >>> >>> The two export functions are only prepared for GPU module. But accordint to >>> the latest information, it seems that there is no open source plan for GPU. >>> So the two functions will be drop in next version of patch. >> >> That's a shame! > >Need to comply with certain commercial terms. > >> >>> >> +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; >>> > >>> > Is there a specific reason that you are using the __raw variants here >>> > (and elsewhere) in the driver? >>> >>> Will use unified function '__raw_readl' and '__raw_writel' >> >> No no, I want to know *why* you are using the __raw accessors here. My >> understanding was that __raw variants are unbarriered & unordered with >> respect to other io accesses. >> >> I do notice that the bcm driver you mentioned uses the __raw variants, >> but only __raw variants - whereas you use readl() which is ordered and >> barriered & __raw_readl(). >> >> Is there a reason why you would not use readl() or readl_relaxed()? > >Your question led me to deeply understand the usage of these io accessors. >__raw_readl / __raw_writel denotes native byte order, no memory barrier. >readl / writel do guarantee the byte order with barrier, ensure that previous writes are done. >Seem that non-raw accessors are more safe. Yeah, if there's no good reason to use these "raw" versions then please use readl/readl_relaxed. >> No worries, looking forward to getting my board :) >> >Have you purchased a VisionFive 2 board online? I have :)
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* +STARFIVE JH71XX PMU CONTROLLER DRIVER +M: Walker Chen <walker.chen@starfivetech.com> +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@infradead.org> M: Josh Poimboeuf <jpoimboe@kernel.org> 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. + 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 + +struct starfive_power_dev { + struct generic_pm_domain genpd; + struct starfive_pmu *power; + uint32_t mask; +}; + +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; + 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; +} + +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; + 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; + } + } + + 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); + + 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); +} + +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); + +MODULE_AUTHOR("Walker Chen <walker.chen@starfivetech.com>"); +MODULE_DESCRIPTION("StarFive JH71XX Power Domain 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@starfivetech.com> + */ + +#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__ */
Add generic power domain driver for the StarFive JH71XX SoC. Signed-off-by: Walker Chen <walker.chen@starfivetech.com> --- 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