On the new i.MX8x SoC family, the following changes were made on the FTM
block:
1. Need to enable the IPG clock before accessing any FTM registers. Because
the IPG clock is not an option for FTM counter clock source, it can't be
used as the ftm_sys clock.
2. An additional PWM enable bit was added for each PWM channel in register
FTM_SC[16:23]. It supports 8 channels. Bit16 is for channel 0, and bit23
for channel 7.
As the IP version information can not be obtained in any of the FTM
registers, a property of "fsl,has-pwmen-bits" is added in the ftm pwm
device node. If it has the property, the driver set the PWM enable bit
when a PWM channel is requested.
Signed-off-by: Shenwei Wang <[email protected]>
---
drivers/pwm/pwm-fsl-ftm.c | 35 +++++++++++++++++++++++++++++------
1 file changed, 29 insertions(+), 6 deletions(-)
diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
index 557b4ea..0426458f 100644
--- a/drivers/pwm/pwm-fsl-ftm.c
+++ b/drivers/pwm/pwm-fsl-ftm.c
@@ -86,7 +86,9 @@ struct fsl_pwm_chip {
struct regmap *regmap;
int period_ns;
+ bool has_pwmen;
+ struct clk *ipg_clk;
struct clk *clk[FSL_PWM_CLK_MAX];
};
@@ -97,16 +99,31 @@ static inline struct fsl_pwm_chip *to_fsl_chip(struct pwm_chip *chip)
static int fsl_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
{
+ int ret;
struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
- return clk_prepare_enable(fpc->clk[FSL_PWM_CLK_SYS]);
+ ret = clk_prepare_enable(fpc->ipg_clk);
+
+ if ((!ret) && (fpc->has_pwmen)) {
+ mutex_lock(&fpc->lock);
+ regmap_update_bits(fpc->regmap, FTM_SC,
+ BIT(pwm->hwpwm + 16), BIT(pwm->hwpwm + 16));
+ mutex_unlock(&fpc->lock);
+ }
+
+ return ret;
}
static void fsl_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
{
struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
- clk_disable_unprepare(fpc->clk[FSL_PWM_CLK_SYS]);
+ if (fpc->has_pwmen) {
+ mutex_lock(&fpc->lock);
+ regmap_update_bits(fpc->regmap, FTM_SC, BIT(pwm->hwpwm + 16), 0);
+ mutex_unlock(&fpc->lock);
+ }
+ clk_disable_unprepare(fpc->ipg_clk);
}
static int fsl_pwm_calculate_default_ps(struct fsl_pwm_chip *fpc,
@@ -363,7 +380,7 @@ static int fsl_pwm_init(struct fsl_pwm_chip *fpc)
{
int ret;
- ret = clk_prepare_enable(fpc->clk[FSL_PWM_CLK_SYS]);
+ ret = clk_prepare_enable(fpc->ipg_clk);
if (ret)
return ret;
@@ -371,7 +388,7 @@ static int fsl_pwm_init(struct fsl_pwm_chip *fpc)
regmap_write(fpc->regmap, FTM_OUTINIT, 0x00);
regmap_write(fpc->regmap, FTM_OUTMASK, 0xFF);
- clk_disable_unprepare(fpc->clk[FSL_PWM_CLK_SYS]);
+ clk_disable_unprepare(fpc->ipg_clk);
return 0;
}
@@ -428,6 +445,10 @@ static int fsl_pwm_probe(struct platform_device *pdev)
return PTR_ERR(fpc->clk[FSL_PWM_CLK_SYS]);
}
+ fpc->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
+ if (IS_ERR(fpc->ipg_clk))
+ fpc->ipg_clk = fpc->clk[FSL_PWM_CLK_SYS];
+
fpc->clk[FSL_PWM_CLK_FIX] = devm_clk_get(fpc->chip.dev, "ftm_fix");
if (IS_ERR(fpc->clk[FSL_PWM_CLK_FIX]))
return PTR_ERR(fpc->clk[FSL_PWM_CLK_FIX]);
@@ -446,6 +467,8 @@ static int fsl_pwm_probe(struct platform_device *pdev)
fpc->chip.of_pwm_n_cells = 3;
fpc->chip.base = -1;
fpc->chip.npwm = 8;
+ fpc->has_pwmen = of_property_read_bool(pdev->dev.of_node,
+ "fsl,ftm-has-pwmen-bits");
ret = pwmchip_add(&fpc->chip);
if (ret < 0) {
@@ -480,7 +503,7 @@ static int fsl_pwm_suspend(struct device *dev)
if (!test_bit(PWMF_REQUESTED, &pwm->flags))
continue;
- clk_disable_unprepare(fpc->clk[FSL_PWM_CLK_SYS]);
+ clk_disable_unprepare(fpc->ipg_clk);
if (!pwm_is_enabled(pwm))
continue;
@@ -503,7 +526,7 @@ static int fsl_pwm_resume(struct device *dev)
if (!test_bit(PWMF_REQUESTED, &pwm->flags))
continue;
- clk_prepare_enable(fpc->clk[FSL_PWM_CLK_SYS]);
+ clk_prepare_enable(fpc->ipg_clk);
if (!pwm_is_enabled(pwm))
continue;
--
2.9.2
Ping.
Shenwei
-----Original Message-----
From: Shenwei Wang
Sent: Thursday, May 24, 2018 1:09 PM
To: [email protected]
Cc: [email protected]; dl-linux-imx <[email protected]>; [email protected]; Shenwei Wang <[email protected]>
Subject: [PATCH 1/1] pwm: fsl-ftm: Support the new version of FTM block on i.MX8x
On the new i.MX8x SoC family, the following changes were made on the FTM
block:
1. Need to enable the IPG clock before accessing any FTM registers. Because the IPG clock is not an option for FTM counter clock source, it can't be used as the ftm_sys clock.
2. An additional PWM enable bit was added for each PWM channel in register FTM_SC[16:23]. It supports 8 channels. Bit16 is for channel 0, and bit23 for channel 7.
As the IP version information can not be obtained in any of the FTM registers, a property of "fsl,has-pwmen-bits" is added in the ftm pwm device node. If it has the property, the driver set the PWM enable bit when a PWM channel is requested.
Signed-off-by: Shenwei Wang <[email protected]>
---
drivers/pwm/pwm-fsl-ftm.c | 35 +++++++++++++++++++++++++++++------
1 file changed, 29 insertions(+), 6 deletions(-)
diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c index 557b4ea..0426458f 100644
--- a/drivers/pwm/pwm-fsl-ftm.c
+++ b/drivers/pwm/pwm-fsl-ftm.c
@@ -86,7 +86,9 @@ struct fsl_pwm_chip {
struct regmap *regmap;
int period_ns;
+ bool has_pwmen;
+ struct clk *ipg_clk;
struct clk *clk[FSL_PWM_CLK_MAX];
};
@@ -97,16 +99,31 @@ static inline struct fsl_pwm_chip *to_fsl_chip(struct pwm_chip *chip)
static int fsl_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) {
+ int ret;
struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
- return clk_prepare_enable(fpc->clk[FSL_PWM_CLK_SYS]);
+ ret = clk_prepare_enable(fpc->ipg_clk);
+
+ if ((!ret) && (fpc->has_pwmen)) {
+ mutex_lock(&fpc->lock);
+ regmap_update_bits(fpc->regmap, FTM_SC,
+ BIT(pwm->hwpwm + 16), BIT(pwm->hwpwm + 16));
+ mutex_unlock(&fpc->lock);
+ }
+
+ return ret;
}
static void fsl_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) {
struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
- clk_disable_unprepare(fpc->clk[FSL_PWM_CLK_SYS]);
+ if (fpc->has_pwmen) {
+ mutex_lock(&fpc->lock);
+ regmap_update_bits(fpc->regmap, FTM_SC, BIT(pwm->hwpwm + 16), 0);
+ mutex_unlock(&fpc->lock);
+ }
+ clk_disable_unprepare(fpc->ipg_clk);
}
static int fsl_pwm_calculate_default_ps(struct fsl_pwm_chip *fpc, @@ -363,7 +380,7 @@ static int fsl_pwm_init(struct fsl_pwm_chip *fpc) {
int ret;
- ret = clk_prepare_enable(fpc->clk[FSL_PWM_CLK_SYS]);
+ ret = clk_prepare_enable(fpc->ipg_clk);
if (ret)
return ret;
@@ -371,7 +388,7 @@ static int fsl_pwm_init(struct fsl_pwm_chip *fpc)
regmap_write(fpc->regmap, FTM_OUTINIT, 0x00);
regmap_write(fpc->regmap, FTM_OUTMASK, 0xFF);
- clk_disable_unprepare(fpc->clk[FSL_PWM_CLK_SYS]);
+ clk_disable_unprepare(fpc->ipg_clk);
return 0;
}
@@ -428,6 +445,10 @@ static int fsl_pwm_probe(struct platform_device *pdev)
return PTR_ERR(fpc->clk[FSL_PWM_CLK_SYS]);
}
+ fpc->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
+ if (IS_ERR(fpc->ipg_clk))
+ fpc->ipg_clk = fpc->clk[FSL_PWM_CLK_SYS];
+
fpc->clk[FSL_PWM_CLK_FIX] = devm_clk_get(fpc->chip.dev, "ftm_fix");
if (IS_ERR(fpc->clk[FSL_PWM_CLK_FIX]))
return PTR_ERR(fpc->clk[FSL_PWM_CLK_FIX]);
@@ -446,6 +467,8 @@ static int fsl_pwm_probe(struct platform_device *pdev)
fpc->chip.of_pwm_n_cells = 3;
fpc->chip.base = -1;
fpc->chip.npwm = 8;
+ fpc->has_pwmen = of_property_read_bool(pdev->dev.of_node,
+ "fsl,ftm-has-pwmen-bits");
ret = pwmchip_add(&fpc->chip);
if (ret < 0) {
@@ -480,7 +503,7 @@ static int fsl_pwm_suspend(struct device *dev)
if (!test_bit(PWMF_REQUESTED, &pwm->flags))
continue;
- clk_disable_unprepare(fpc->clk[FSL_PWM_CLK_SYS]);
+ clk_disable_unprepare(fpc->ipg_clk);
if (!pwm_is_enabled(pwm))
continue;
@@ -503,7 +526,7 @@ static int fsl_pwm_resume(struct device *dev)
if (!test_bit(PWMF_REQUESTED, &pwm->flags))
continue;
- clk_prepare_enable(fpc->clk[FSL_PWM_CLK_SYS]);
+ clk_prepare_enable(fpc->ipg_clk);
if (!pwm_is_enabled(pwm))
continue;
--
2.9.2
Hello Thierry,
Can you please share your comments on the patch?
Thanks,
Shenwei
-----Original Message-----
From: Shenwei Wang
Sent: Wednesday, May 30, 2018 12:00 PM
To: [email protected]
Cc: [email protected]; dl-linux-imx <[email protected]>; [email protected]
Subject: RE: [PATCH 1/1] pwm: fsl-ftm: Support the new version of FTM block on i.MX8x
Ping.
Shenwei
-----Original Message-----
From: Shenwei Wang
Sent: Thursday, May 24, 2018 1:09 PM
To: [email protected]
Cc: [email protected]; dl-linux-imx <[email protected]>; [email protected]; Shenwei Wang <[email protected]>
Subject: [PATCH 1/1] pwm: fsl-ftm: Support the new version of FTM block on i.MX8x
On the new i.MX8x SoC family, the following changes were made on the FTM
block:
1. Need to enable the IPG clock before accessing any FTM registers. Because the IPG clock is not an option for FTM counter clock source, it can't be used as the ftm_sys clock.
2. An additional PWM enable bit was added for each PWM channel in register FTM_SC[16:23]. It supports 8 channels. Bit16 is for channel 0, and bit23 for channel 7.
As the IP version information can not be obtained in any of the FTM registers, a property of "fsl,has-pwmen-bits" is added in the ftm pwm device node. If it has the property, the driver set the PWM enable bit when a PWM channel is requested.
Signed-off-by: Shenwei Wang <[email protected]>
---
drivers/pwm/pwm-fsl-ftm.c | 35 +++++++++++++++++++++++++++++------
1 file changed, 29 insertions(+), 6 deletions(-)
diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c index 557b4ea..0426458f 100644
--- a/drivers/pwm/pwm-fsl-ftm.c
+++ b/drivers/pwm/pwm-fsl-ftm.c
@@ -86,7 +86,9 @@ struct fsl_pwm_chip {
struct regmap *regmap;
int period_ns;
+ bool has_pwmen;
+ struct clk *ipg_clk;
struct clk *clk[FSL_PWM_CLK_MAX];
};
@@ -97,16 +99,31 @@ static inline struct fsl_pwm_chip *to_fsl_chip(struct pwm_chip *chip)
static int fsl_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) {
+ int ret;
struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
- return clk_prepare_enable(fpc->clk[FSL_PWM_CLK_SYS]);
+ ret = clk_prepare_enable(fpc->ipg_clk);
+
+ if ((!ret) && (fpc->has_pwmen)) {
+ mutex_lock(&fpc->lock);
+ regmap_update_bits(fpc->regmap, FTM_SC,
+ BIT(pwm->hwpwm + 16), BIT(pwm->hwpwm + 16));
+ mutex_unlock(&fpc->lock);
+ }
+
+ return ret;
}
static void fsl_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) {
struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
- clk_disable_unprepare(fpc->clk[FSL_PWM_CLK_SYS]);
+ if (fpc->has_pwmen) {
+ mutex_lock(&fpc->lock);
+ regmap_update_bits(fpc->regmap, FTM_SC, BIT(pwm->hwpwm + 16), 0);
+ mutex_unlock(&fpc->lock);
+ }
+ clk_disable_unprepare(fpc->ipg_clk);
}
static int fsl_pwm_calculate_default_ps(struct fsl_pwm_chip *fpc, @@ -363,7 +380,7 @@ static int fsl_pwm_init(struct fsl_pwm_chip *fpc) {
int ret;
- ret = clk_prepare_enable(fpc->clk[FSL_PWM_CLK_SYS]);
+ ret = clk_prepare_enable(fpc->ipg_clk);
if (ret)
return ret;
@@ -371,7 +388,7 @@ static int fsl_pwm_init(struct fsl_pwm_chip *fpc)
regmap_write(fpc->regmap, FTM_OUTINIT, 0x00);
regmap_write(fpc->regmap, FTM_OUTMASK, 0xFF);
- clk_disable_unprepare(fpc->clk[FSL_PWM_CLK_SYS]);
+ clk_disable_unprepare(fpc->ipg_clk);
return 0;
}
@@ -428,6 +445,10 @@ static int fsl_pwm_probe(struct platform_device *pdev)
return PTR_ERR(fpc->clk[FSL_PWM_CLK_SYS]);
}
+ fpc->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
+ if (IS_ERR(fpc->ipg_clk))
+ fpc->ipg_clk = fpc->clk[FSL_PWM_CLK_SYS];
+
fpc->clk[FSL_PWM_CLK_FIX] = devm_clk_get(fpc->chip.dev, "ftm_fix");
if (IS_ERR(fpc->clk[FSL_PWM_CLK_FIX]))
return PTR_ERR(fpc->clk[FSL_PWM_CLK_FIX]);
@@ -446,6 +467,8 @@ static int fsl_pwm_probe(struct platform_device *pdev)
fpc->chip.of_pwm_n_cells = 3;
fpc->chip.base = -1;
fpc->chip.npwm = 8;
+ fpc->has_pwmen = of_property_read_bool(pdev->dev.of_node,
+ "fsl,ftm-has-pwmen-bits");
ret = pwmchip_add(&fpc->chip);
if (ret < 0) {
@@ -480,7 +503,7 @@ static int fsl_pwm_suspend(struct device *dev)
if (!test_bit(PWMF_REQUESTED, &pwm->flags))
continue;
- clk_disable_unprepare(fpc->clk[FSL_PWM_CLK_SYS]);
+ clk_disable_unprepare(fpc->ipg_clk);
if (!pwm_is_enabled(pwm))
continue;
@@ -503,7 +526,7 @@ static int fsl_pwm_resume(struct device *dev)
if (!test_bit(PWMF_REQUESTED, &pwm->flags))
continue;
- clk_prepare_enable(fpc->clk[FSL_PWM_CLK_SYS]);
+ clk_prepare_enable(fpc->ipg_clk);
if (!pwm_is_enabled(pwm))
continue;
--
2.9.2
On Thu, May 24, 2018 at 01:08:48PM -0500, [email protected] wrote:
> On the new i.MX8x SoC family, the following changes were made on the FTM
> block:
>
> 1. Need to enable the IPG clock before accessing any FTM registers. Because
> the IPG clock is not an option for FTM counter clock source, it can't be
> used as the ftm_sys clock.
>
> 2. An additional PWM enable bit was added for each PWM channel in register
> FTM_SC[16:23]. It supports 8 channels. Bit16 is for channel 0, and bit23
> for channel 7.
Generally if you need to itemize changes in your commit message it is a
good indication that you should be splitting up the patch into multiple
logical changes. In this case, one possibility would be to turn this
into three patches:
1. Introduce the concept on an "interface" clock which is by
default the same as the ftm_sys clock.
2. Add support for enable bits, based on some per-compatible
data structure (see for example pwm-tegra.c for an example of
how to do this).
3. Enable support for the new SoC family by adding an instance
of the per-compatible structure for that compatible string.
> As the IP version information can not be obtained in any of the FTM
> registers, a property of "fsl,has-pwmen-bits" is added in the ftm pwm
> device node. If it has the property, the driver set the PWM enable bit
> when a PWM channel is requested.
I don't see a corresponding device tree bindings update for this change.
Also, I don't think doing this via a separate property is the right way,
you can just derive this from the new compatible string which you need
to add anyway.
So to the above patches, add:
0. Add DT bindings for new SoC family with a mention that they
need to provide the new IPG clock.
>
> Signed-off-by: Shenwei Wang <[email protected]>
> ---
> drivers/pwm/pwm-fsl-ftm.c | 35 +++++++++++++++++++++++++++++------
> 1 file changed, 29 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
> index 557b4ea..0426458f 100644
> --- a/drivers/pwm/pwm-fsl-ftm.c
> +++ b/drivers/pwm/pwm-fsl-ftm.c
> @@ -86,7 +86,9 @@ struct fsl_pwm_chip {
> struct regmap *regmap;
>
> int period_ns;
> + bool has_pwmen;
>
> + struct clk *ipg_clk;
> struct clk *clk[FSL_PWM_CLK_MAX];
> };
>
> @@ -97,16 +99,31 @@ static inline struct fsl_pwm_chip *to_fsl_chip(struct pwm_chip *chip)
>
> static int fsl_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> {
> + int ret;
> struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
>
> - return clk_prepare_enable(fpc->clk[FSL_PWM_CLK_SYS]);
> + ret = clk_prepare_enable(fpc->ipg_clk);
This is confusing because it looks as if you're breaking existing
drivers, until you realize that...
> +
> + if ((!ret) && (fpc->has_pwmen)) {
> + mutex_lock(&fpc->lock);
> + regmap_update_bits(fpc->regmap, FTM_SC,
> + BIT(pwm->hwpwm + 16), BIT(pwm->hwpwm + 16));
> + mutex_unlock(&fpc->lock);
> + }
> +
> + return ret;
> }
>
> static void fsl_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> {
> struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
>
> - clk_disable_unprepare(fpc->clk[FSL_PWM_CLK_SYS]);
> + if (fpc->has_pwmen) {
> + mutex_lock(&fpc->lock);
> + regmap_update_bits(fpc->regmap, FTM_SC, BIT(pwm->hwpwm + 16), 0);
> + mutex_unlock(&fpc->lock);
> + }
> + clk_disable_unprepare(fpc->ipg_clk);
> }
>
> static int fsl_pwm_calculate_default_ps(struct fsl_pwm_chip *fpc,
> @@ -363,7 +380,7 @@ static int fsl_pwm_init(struct fsl_pwm_chip *fpc)
> {
> int ret;
>
> - ret = clk_prepare_enable(fpc->clk[FSL_PWM_CLK_SYS]);
> + ret = clk_prepare_enable(fpc->ipg_clk);
> if (ret)
> return ret;
>
> @@ -371,7 +388,7 @@ static int fsl_pwm_init(struct fsl_pwm_chip *fpc)
> regmap_write(fpc->regmap, FTM_OUTINIT, 0x00);
> regmap_write(fpc->regmap, FTM_OUTMASK, 0xFF);
>
> - clk_disable_unprepare(fpc->clk[FSL_PWM_CLK_SYS]);
> + clk_disable_unprepare(fpc->ipg_clk);
>
> return 0;
> }
> @@ -428,6 +445,10 @@ static int fsl_pwm_probe(struct platform_device *pdev)
> return PTR_ERR(fpc->clk[FSL_PWM_CLK_SYS]);
> }
>
> + fpc->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
> + if (IS_ERR(fpc->ipg_clk))
> + fpc->ipg_clk = fpc->clk[FSL_PWM_CLK_SYS];
... fpc->ipg_clk is actually the same as fpc->clk[FSL_PWM_CLK_SYS] for
older chips. I think this could be made more obvious by splitting this
patch into several smaller ones as suggested above.
> +
> fpc->clk[FSL_PWM_CLK_FIX] = devm_clk_get(fpc->chip.dev, "ftm_fix");
> if (IS_ERR(fpc->clk[FSL_PWM_CLK_FIX]))
> return PTR_ERR(fpc->clk[FSL_PWM_CLK_FIX]);
> @@ -446,6 +467,8 @@ static int fsl_pwm_probe(struct platform_device *pdev)
> fpc->chip.of_pwm_n_cells = 3;
> fpc->chip.base = -1;
> fpc->chip.npwm = 8;
> + fpc->has_pwmen = of_property_read_bool(pdev->dev.of_node,
> + "fsl,ftm-has-pwmen-bits");
As I said earlier, I think this should be derived from the compatible
string. That is, you could have a data structure such as:
struct fsl_pwm_soc {
bool has_enable_bits;
};
static const struct fsl_pwm_soc vf610_ftm_pwm = {
.has_enable_bits = false,
};
/* up to here would be part of patch 2 */
/* and this is part of patch 3, along with an entry in the OF
* match table */
static const struct fsl_pwm_soc imx8x_ftm_pwm = {
.has_enable_bits = true,
};
Thierry
Hi Thierry,
Thank you for the detailed review. I was intending to avoid adding a new compatible string, and to limit the changes. But your comments make sense, and I will split the commit and use the compatible string to identify the IP differences.
Thanks,
Shenwei
-----Original Message-----
From: Thierry Reding [mailto:[email protected]]
Sent: Wednesday, June 6, 2018 3:34 AM
To: Shenwei Wang <[email protected]>
Cc: [email protected]; dl-linux-imx <[email protected]>; [email protected]
Subject: Re: [PATCH 1/1] pwm: fsl-ftm: Support the new version of FTM block on i.MX8x
On Thu, May 24, 2018 at 01:08:48PM -0500, [email protected] wrote:
> On the new i.MX8x SoC family, the following changes were made on the
> FTM
> block:
>
> 1. Need to enable the IPG clock before accessing any FTM registers.
> Because the IPG clock is not an option for FTM counter clock source,
> it can't be used as the ftm_sys clock.
>
> 2. An additional PWM enable bit was added for each PWM channel in
> register FTM_SC[16:23]. It supports 8 channels. Bit16 is for channel
> 0, and bit23 for channel 7.
Generally if you need to itemize changes in your commit message it is a good indication that you should be splitting up the patch into multiple logical changes. In this case, one possibility would be to turn this into three patches:
1. Introduce the concept on an "interface" clock which is by
default the same as the ftm_sys clock.
2. Add support for enable bits, based on some per-compatible
data structure (see for example pwm-tegra.c for an example of
how to do this).
3. Enable support for the new SoC family by adding an instance
of the per-compatible structure for that compatible string.
> As the IP version information can not be obtained in any of the FTM
> registers, a property of "fsl,has-pwmen-bits" is added in the ftm pwm
> device node. If it has the property, the driver set the PWM enable bit
> when a PWM channel is requested.
I don't see a corresponding device tree bindings update for this change.
Also, I don't think doing this via a separate property is the right way, you can just derive this from the new compatible string which you need to add anyway.
So to the above patches, add:
0. Add DT bindings for new SoC family with a mention that they
need to provide the new IPG clock.
>
> Signed-off-by: Shenwei Wang <[email protected]>
> ---
> drivers/pwm/pwm-fsl-ftm.c | 35 +++++++++++++++++++++++++++++------
> 1 file changed, 29 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
> index 557b4ea..0426458f 100644
> --- a/drivers/pwm/pwm-fsl-ftm.c
> +++ b/drivers/pwm/pwm-fsl-ftm.c
> @@ -86,7 +86,9 @@ struct fsl_pwm_chip {
> struct regmap *regmap;
>
> int period_ns;
> + bool has_pwmen;
>
> + struct clk *ipg_clk;
> struct clk *clk[FSL_PWM_CLK_MAX];
> };
>
> @@ -97,16 +99,31 @@ static inline struct fsl_pwm_chip
> *to_fsl_chip(struct pwm_chip *chip)
>
> static int fsl_pwm_request(struct pwm_chip *chip, struct pwm_device
> *pwm) {
> + int ret;
> struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
>
> - return clk_prepare_enable(fpc->clk[FSL_PWM_CLK_SYS]);
> + ret = clk_prepare_enable(fpc->ipg_clk);
This is confusing because it looks as if you're breaking existing drivers, until you realize that...
> +
> + if ((!ret) && (fpc->has_pwmen)) {
> + mutex_lock(&fpc->lock);
> + regmap_update_bits(fpc->regmap, FTM_SC,
> + BIT(pwm->hwpwm + 16), BIT(pwm->hwpwm + 16));
> + mutex_unlock(&fpc->lock);
> + }
> +
> + return ret;
> }
>
> static void fsl_pwm_free(struct pwm_chip *chip, struct pwm_device
> *pwm) {
> struct fsl_pwm_chip *fpc = to_fsl_chip(chip);
>
> - clk_disable_unprepare(fpc->clk[FSL_PWM_CLK_SYS]);
> + if (fpc->has_pwmen) {
> + mutex_lock(&fpc->lock);
> + regmap_update_bits(fpc->regmap, FTM_SC, BIT(pwm->hwpwm + 16), 0);
> + mutex_unlock(&fpc->lock);
> + }
> + clk_disable_unprepare(fpc->ipg_clk);
> }
>
> static int fsl_pwm_calculate_default_ps(struct fsl_pwm_chip *fpc, @@
> -363,7 +380,7 @@ static int fsl_pwm_init(struct fsl_pwm_chip *fpc) {
> int ret;
>
> - ret = clk_prepare_enable(fpc->clk[FSL_PWM_CLK_SYS]);
> + ret = clk_prepare_enable(fpc->ipg_clk);
> if (ret)
> return ret;
>
> @@ -371,7 +388,7 @@ static int fsl_pwm_init(struct fsl_pwm_chip *fpc)
> regmap_write(fpc->regmap, FTM_OUTINIT, 0x00);
> regmap_write(fpc->regmap, FTM_OUTMASK, 0xFF);
>
> - clk_disable_unprepare(fpc->clk[FSL_PWM_CLK_SYS]);
> + clk_disable_unprepare(fpc->ipg_clk);
>
> return 0;
> }
> @@ -428,6 +445,10 @@ static int fsl_pwm_probe(struct platform_device *pdev)
> return PTR_ERR(fpc->clk[FSL_PWM_CLK_SYS]);
> }
>
> + fpc->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
> + if (IS_ERR(fpc->ipg_clk))
> + fpc->ipg_clk = fpc->clk[FSL_PWM_CLK_SYS];
... fpc->ipg_clk is actually the same as fpc->clk[FSL_PWM_CLK_SYS] for older chips. I think this could be made more obvious by splitting this patch into several smaller ones as suggested above.
> +
> fpc->clk[FSL_PWM_CLK_FIX] = devm_clk_get(fpc->chip.dev, "ftm_fix");
> if (IS_ERR(fpc->clk[FSL_PWM_CLK_FIX]))
> return PTR_ERR(fpc->clk[FSL_PWM_CLK_FIX]);
> @@ -446,6 +467,8 @@ static int fsl_pwm_probe(struct platform_device *pdev)
> fpc->chip.of_pwm_n_cells = 3;
> fpc->chip.base = -1;
> fpc->chip.npwm = 8;
> + fpc->has_pwmen = of_property_read_bool(pdev->dev.of_node,
> + "fsl,ftm-has-pwmen-bits");
As I said earlier, I think this should be derived from the compatible string. That is, you could have a data structure such as:
struct fsl_pwm_soc {
bool has_enable_bits;
};
static const struct fsl_pwm_soc vf610_ftm_pwm = {
.has_enable_bits = false,
};
/* up to here would be part of patch 2 */
/* and this is part of patch 3, along with an entry in the OF
* match table */
static const struct fsl_pwm_soc imx8x_ftm_pwm = {
.has_enable_bits = true,
};
Thierry