| Message ID | 20230411064743.273388-5-changhuang.liang@starfivetech.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add JH7110 DPHY PMU support | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/tree_selection | fail | Failed to apply to next/pending-fixes or riscv/for-next |
Hey Changhuang Liang, On Mon, Apr 10, 2023 at 11:47:40PM -0700, Changhuang Liang wrote: > Add pmu type, make a distinction between different PMU. Please write more detailed commit messages, thanks. > > Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com> > --- > drivers/soc/starfive/jh71xx_pmu.c | 55 ++++++++++++++++++++++--------- > 1 file changed, 39 insertions(+), 16 deletions(-) > > diff --git a/drivers/soc/starfive/jh71xx_pmu.c b/drivers/soc/starfive/jh71xx_pmu.c > index 306218c83691..98f6849d61de 100644 > --- a/drivers/soc/starfive/jh71xx_pmu.c > +++ b/drivers/soc/starfive/jh71xx_pmu.c > @@ -45,6 +45,12 @@ > */ > #define JH71XX_PMU_TIMEOUT_US 100 > > +/* pmu type */ Delete this comment, it's obvious. > +enum pmu_type { > + JH71XX_PMU_GENERAL, I'm really not sold on GENERAL as a name. Why not name these after the compatibles? > + JH71XX_PMU_DPHY, > +}; > + > struct jh71xx_domain_info { > const char * const name; > unsigned int flags; > @@ -54,6 +60,7 @@ struct jh71xx_domain_info { > struct jh71xx_pmu_match_data { > const struct jh71xx_domain_info *domain_info; > int num_domains; > + u8 pmu_type; This is an enum, not a u8? Thanks, Conor.
On 2023/4/12 4:52, Conor Dooley wrote: > Hey Changhuang Liang, > > On Mon, Apr 10, 2023 at 11:47:40PM -0700, Changhuang Liang wrote: >> Add pmu type, make a distinction between different PMU. > > Please write more detailed commit messages, thanks. > OK, will write more detail for it. >> >> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com> >> --- >> drivers/soc/starfive/jh71xx_pmu.c | 55 ++++++++++++++++++++++--------- >> 1 file changed, 39 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/soc/starfive/jh71xx_pmu.c b/drivers/soc/starfive/jh71xx_pmu.c >> index 306218c83691..98f6849d61de 100644 >> --- a/drivers/soc/starfive/jh71xx_pmu.c >> +++ b/drivers/soc/starfive/jh71xx_pmu.c >> @@ -45,6 +45,12 @@ >> */ >> #define JH71XX_PMU_TIMEOUT_US 100 >> >> +/* pmu type */ > > Delete this comment, it's obvious. > OK, will delete this line. >> +enum pmu_type { >> + JH71XX_PMU_GENERAL, > > I'm really not sold on GENERAL as a name. > Why not name these after the compatibles? > OK, will change to "JH71XX_PMU". >> + JH71XX_PMU_DPHY, >> +}; >> + >> struct jh71xx_domain_info { >> const char * const name; >> unsigned int flags; >> @@ -54,6 +60,7 @@ struct jh71xx_domain_info { >> struct jh71xx_pmu_match_data { >> const struct jh71xx_domain_info *domain_info; >> int num_domains; >> + u8 pmu_type; > > This is an enum, not a u8? > OK, will fix it. > Thanks, > Conor. >
diff --git a/drivers/soc/starfive/jh71xx_pmu.c b/drivers/soc/starfive/jh71xx_pmu.c index 306218c83691..98f6849d61de 100644 --- a/drivers/soc/starfive/jh71xx_pmu.c +++ b/drivers/soc/starfive/jh71xx_pmu.c @@ -45,6 +45,12 @@ */ #define JH71XX_PMU_TIMEOUT_US 100 +/* pmu type */ +enum pmu_type { + JH71XX_PMU_GENERAL, + JH71XX_PMU_DPHY, +}; + struct jh71xx_domain_info { const char * const name; unsigned int flags; @@ -54,6 +60,7 @@ struct jh71xx_domain_info { struct jh71xx_pmu_match_data { const struct jh71xx_domain_info *domain_info; int num_domains; + u8 pmu_type; }; struct jh71xx_pmu { @@ -75,19 +82,23 @@ struct jh71xx_pmu_dev { static int jh71xx_pmu_get_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool *is_on) { struct jh71xx_pmu *pmu = pmd->pmu; + unsigned int offset = 0; unsigned int val; if (!mask) return -EINVAL; - regmap_read(pmu->base, JH71XX_PMU_CURR_POWER_MODE, &val); + if (pmu->match_data->pmu_type == JH71XX_PMU_GENERAL) + offset = JH71XX_PMU_CURR_POWER_MODE; + + regmap_read(pmu->base, offset, &val); *is_on = val & mask; return 0; } -static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on) +static int jh71xx_pmu_general_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on) { struct jh71xx_pmu *pmu = pmd->pmu; unsigned long flags; @@ -95,22 +106,8 @@ static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on) 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); /* @@ -169,6 +166,31 @@ static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on) return 0; } +static int jh71xx_pmu_set_state(struct jh71xx_pmu_dev *pmd, u32 mask, bool on) +{ + struct jh71xx_pmu *pmu = pmd->pmu; + bool is_on; + int ret = 0; + + 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; + } + + if (pmu->match_data->pmu_type == JH71XX_PMU_GENERAL) + ret = jh71xx_pmu_general_set_state(pmd, mask, on); + + return ret; +} + static int jh71xx_pmu_on(struct generic_pm_domain *genpd) { struct jh71xx_pmu_dev *pmd = container_of(genpd, @@ -360,6 +382,7 @@ static const struct jh71xx_domain_info jh7110_power_domains[] = { static const struct jh71xx_pmu_match_data jh7110_pmu = { .num_domains = ARRAY_SIZE(jh7110_power_domains), .domain_info = jh7110_power_domains, + .pmu_type = JH71XX_PMU_GENERAL, }; static const struct of_device_id jh71xx_pmu_of_match[] = {
Add pmu type, make a distinction between different PMU. Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com> --- drivers/soc/starfive/jh71xx_pmu.c | 55 ++++++++++++++++++++++--------- 1 file changed, 39 insertions(+), 16 deletions(-)