On 2022/12/20 4:40, Conor Dooley wrote:
> Hey Walker,
>
> Hopefully just some minor bits here. Hopefully either Emil who has a
> board, or someone that knows power management stuff better can give this
> a proper review.
>
> On Thu, Dec 08, 2022 at 04:45:22PM +0800, Walker Chen wrote:
>> Add pmu driver for the StarFive JH71XX SoC.
>>
>> As the power domains provider, the Power Management Unit (PMU) is
>> designed for including multiple PM domains that can be used for power
>> gating of selected IP blocks for power saving by reduced leakage
>> current. It accepts software encourage command to switch the power mode
>> of SoC.
>>
>> Signed-off-by: Walker Chen <walker.chen@xxxxxxxxxxxxxxxx>
>> ---
>> MAINTAINERS | 14 ++
>> drivers/soc/Kconfig | 1 +
>> drivers/soc/Makefile | 1 +
>> drivers/soc/starfive/Kconfig | 11 +
>> drivers/soc/starfive/Makefile | 3 +
>> drivers/soc/starfive/jh71xx_pmu.c | 396 ++++++++++++++++++++++++++++++
>> 6 files changed, 426 insertions(+)
>> create mode 100644 drivers/soc/starfive/Kconfig
>> create mode 100644 drivers/soc/starfive/Makefile
>> create mode 100644 drivers/soc/starfive/jh71xx_pmu.c
>>
>
>> +config JH71XX_PMU
>> + bool "Support PMU for StarFive JH71XX Soc"
>> + depends on PM && (SOC_STARFIVE || COMPILE_TEST)
>
> Why not just do:
> depends on PM
> depends on SOC_STARFIVE || COMPILE_TEST
> I think that way reads a little better.
No problem, will be changed like this way.
>
>> + default SOC_STARFIVE
>> + select PM_GENERIC_DOMAINS
>> + help
>> + Say 'y' here to enable support power domain support.
>> + In order to meet low power requirements, a Power Management Unit (PMU)
>> + is designed for controlling power resources in StarFive JH71XX SoCs.
>> 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..7a0145779e07
>> --- /dev/null
>> +++ b/drivers/soc/starfive/jh71xx_pmu.c
>> @@ -0,0 +1,396 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * StarFive JH71XX PMU (Power Management Unit) 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/starfive,jh7110-pmu.h>
>> +
>> +/* register offset */
>> +#define JH71XX_PMU_HW_EVENT_ON 0x04
>> +#define JH71XX_PMU_HW_EVENT_OFF 0x08
>> +#define JH71XX_PMU_SW_TURN_ON_POWER 0x0C
>> +#define JH71XX_PMU_SW_TURN_OFF_POWER 0x10
>> +#define JH71XX_PMU_SW_ENCOURAGE 0x44
>> +#define JH71XX_PMU_INT_MASK 0x48
>> +#define JH71XX_PMU_PCH_BYPASS 0x4C
>> +#define JH71XX_PMU_PCH_PSTATE 0x50
>> +#define JH71XX_PMU_PCH_TIMEOUT 0x54
>> +#define JH71XX_PMU_LP_TIMEOUT 0x58
>> +#define JH71XX_PMU_HW_TURN_ON 0x5C
>> +#define JH71XX_PMU_CURR_POWER_MODE 0x80
>> +#define JH71XX_PMU_EVENT_STATUS 0x88
>> +#define JH71XX_PMU_INT_STATUS 0x8C
>> +
>> +/* sw encourage cfg */
>> +#define JH71XX_PMU_SW_ENCOURAGE_EN_LO 0x05
>> +#define JH71XX_PMU_SW_ENCOURAGE_EN_HI 0x50
>> +#define JH71XX_PMU_SW_ENCOURAGE_DIS_LO 0x0A
>> +#define JH71XX_PMU_SW_ENCOURAGE_DIS_HI 0xA0
>> +#define JH71XX_PMU_SW_ENCOURAGE_ON 0xFF
>> +
>> +/* pmu int status */
>> +#define JH71XX_PMU_INT_SEQ_DONE BIT(0)
>> +#define JH71XX_PMU_INT_HW_REQ BIT(1)
>> +#define JH71XX_PMU_INT_SW_FAIL GENMASK(3, 2)
>> +#define JH71XX_PMU_INT_HW_FAIL GENMASK(5, 4)
>> +#define JH71XX_PMU_INT_PCH_FAIL GENMASK(8, 6)
>> +#define JH71XX_PMU_INT_FAIL_MASK (JH71XX_PMU_INT_SW_FAIL | \
>> + JH71XX_PMU_INT_HW_FAIL | \
>> + JH71XX_PMU_INT_PCH_FAIL)
>> +#define JH71XX_PMU_INT_ALL_MASK (JH71XX_PMU_INT_SEQ_DONE | \
>> + JH71XX_PMU_INT_HW_REQ | \
>> + JH71XX_PMU_INT_FAIL_MASK)
>> +
>> +/*
>> + * The time required for switching power status is based on the time
>> + * to turn on the largest domain's power, which is at microsecond level
>> + */
>> +#define JH71XX_PMU_TIMEOUT_US 100
>> +
>> +struct jh71xx_domain_info {
>
> const char * const name;
> unsigned int flags;
> u8 bit;
>
>> +};
>> +
>> +struct jh71xx_pmu_match_data {
>
> const struct jh71xx_domain_info *domain_info;
> int num_domains;
>
> Can you switch these two around like so?
Should be no problem.
>> +};
>> +
>> +struct jh71xx_pmu {
>> + struct device *dev;
>> + const struct jh71xx_pmu_match_data *match_data;
>> + void __iomem *base;
>> + spinlock_t lock; /* protects pmu reg */
>> + int irq;
>> + struct genpd_onecell_data genpd_data;
>> + struct generic_pm_domain **genpd;
>> +};
>> +
>> +struct jh71xx_pmu_dev {
>> + struct generic_pm_domain genpd;
>> + const struct jh71xx_domain_info *domain_info;
>> + struct jh71xx_pmu *pmu;
>
> And these two too please in the same way.
Nice :)
>
>> +};
>> +
>> +static int jh71xx_pmu_get_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool *is_on)
>> +{
>> + struct jh71xx_pmu *pmu = pmd->pmu;
>> +
>> + if (!mask) {
>> + *is_on = false;
>> + return -EINVAL;
>> + }
>> +
>> + *is_on = readl(pmu->base + JH71XX_PMU_CURR_POWER_MODE) & mask;
>> +
>> + return 0;
>> +}
>> +
>> +static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on)
>> +{
>> + struct jh71xx_pmu *pmu = pmd->pmu;
>> + unsigned long flags;
>> + u32 val;
>> + u32 mode;
>> + u32 encourage_lo;
>> + u32 encourage_hi;
>> + bool is_on;
>> + int ret;
>> +
>> + ret = jh71xx_pmu_get_state(pmd, mask, &is_on);
>> + if (ret) {
>> + dev_dbg(pmu->dev, "unable to get current state for %s\n",
>> + pmd->genpd.name);
>> + return ret;
>> + }
>> +
>> + if (is_on == on) {
>> + dev_dbg(pmu->dev, "pm domain [%s] is already %sable status.\n",
>> + pmd->genpd.name, on ? "en" : "dis");
>> + return 0;
>> + }
>> +
>> + spin_lock_irqsave(&pmu->lock, flags);
>> +
>> + /*
>> + * The PMU accepts software encourage to switch power mode in the following 2 steps:
>> + *
>> + * 1. Configure the register SW_TURN_ON_POWER (offset 0x0c), write 1 to
>> + * the bit which power domain will be turn-on, write 0 to the others.
>
> Some grammatical nit picking..
>
> "Configure the register blah (offset 0x0c) by writing 1 to the bit
> corresponding to the power domain that will be turned on and writing
> zero to the others."
>
> Is that a correct correct summation of the operation?
Yes, maybe your description is better easy-to-understand.
>
>> + * Likewise, configure the register SW_TURN_OFF_POWER (offset 0x10),
>> + * write 1 to the bit which power domain will be turn-off, write 0 to the others.
>
>
>> + */
>> + if (on) {
>> + mode = JH71XX_PMU_SW_TURN_ON_POWER;
>> + encourage_lo = JH71XX_PMU_SW_ENCOURAGE_EN_LO;
>> + encourage_hi = JH71XX_PMU_SW_ENCOURAGE_EN_HI;
>> + } else {
>> + mode = JH71XX_PMU_SW_TURN_OFF_POWER;
>> + encourage_lo = JH71XX_PMU_SW_ENCOURAGE_DIS_LO;
>> + encourage_hi = JH71XX_PMU_SW_ENCOURAGE_DIS_HI;
>> + }
>> +
>> + writel(mask, pmu->base + mode);
>> +
>> + /*
>> + * 2. Write SW encourage command sequence to the Software Encourage Reg (offset 0x44)
>> + * SW turn-on command sequence: 0xff -> 0x05 -> 0x50
>> + * SW turn-off command sequence: 0xff -> 0x0a -> 0xa0
>
> I think you could replace the hard "coded" numbers here with a better
> description idk without looking at the #defines above what these
> correspond to. AFAICT, it'd be something like:
> First write the ...ENCOURAGE_ON to reset the state machine which parses
> the command sequence. It must be written every time.
> Then write the lower bits of the command sequence, followed by the upper
> bits. The sequence differs between powering on & off a domain.
Thank you for correction for these description. Because English is not my native language,
so not very good in some sentences. I'll take your advice.
>> + *
>> + * Note: writing SW_MODE_ENCOURAGE_ON (0xFF) to the 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.
>> + */
>> + writel(JH71XX_PMU_SW_ENCOURAGE_ON, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
>> + writel(encourage_lo, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
>> + writel(encourage_hi, pmu->base + JH71XX_PMU_SW_ENCOURAGE);
>> +
>> + spin_unlock_irqrestore(&pmu->lock, flags);
>> +
>> + /* Wait for the power domain bit to be enabled / disabled */
>> + if (on) {
>> + ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE,
>> + val, val & mask,
>> + 1, JH71XX_PMU_TIMEOUT_US);
>> + } else {
>> + ret = readl_poll_timeout_atomic(pmu->base + JH71XX_PMU_CURR_POWER_MODE,
>> + val, !(val & mask),
>> + 1, JH71XX_PMU_TIMEOUT_US);
>> + }
>> +
>> + if (ret) {
>> + dev_err(pmu->dev, "%s: failed to power %s\n",
>> + pmd->genpd.name, on ? "on" : "off");
>> + return -ETIMEDOUT;
>> + }
>> +
>> + return 0;
>> +}
>
>> +static int jh71xx_pmu_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct device_node *np = dev->of_node;
>> + const struct jh71xx_pmu_match_data *match_data;
>> + struct jh71xx_pmu *pmu;
>> + unsigned int i;
>> + int ret;
>> +
>> + pmu = devm_kzalloc(dev, sizeof(*pmu), GFP_KERNEL);
>> + if (!pmu)
>> + return -ENOMEM;
>> +
>> + pmu->base = devm_platform_ioremap_resource(pdev, 0);
>> + if (IS_ERR(pmu->base))
>> + return PTR_ERR(pmu->base);
>> +
>> + /* initialize pmu interrupt */
>
> nit: this comment is ~pointless.
Will be dropped.
>
>> + pmu->irq = platform_get_irq(pdev, 0);
>> + if (pmu->irq < 0)
>> + return pmu->irq;
>> +
>> + ret = devm_request_irq(dev, pmu->irq, jh71xx_pmu_interrupt,
>> + 0, pdev->name, pmu);
>> + if (ret)
>> + dev_err(dev, "request irq failed.\n");
>
> nit: s/request/requesting
Will be fixed.
>
> Unfortunately I cannot really review the rest of this, but hopefully
> I'll get a board soon and can try it out - or else send me a link to
> your TRM or w/e.
Anyway, I would like to thank you very much for your time.
Best regards,
Walker
