2017-06-30 06:05:46

by Zhi Mao (毛智)

Subject: [PATCH v3 0/6] mediatek: pwm driver add MT2712/MT7622 support

change in v3:
1. add pwm clk disable in function:mtk_pwm_config()
for error parameter checking case

Zhi Mao (6):
pwm: kconfig: modify mediatek information
pwm: mediatek: fix pwm source clock selection
pwm: mediatek: fix clock control issue
pwm: bindings: add MT2712/MT7622 information
pwm: mediatek: add PWM_CLK_DIV_MAX
pwm: mediatek: add MT2712/MT7622 support

.../devicetree/bindings/pwm/pwm-mediatek.txt | 6 +-
drivers/pwm/Kconfig | 2 +-
drivers/pwm/pwm-mediatek.c | 133 ++++++++++++++------
3 files changed, 104 insertions(+), 37 deletions(-)

2017-06-30 06:05:52

by Zhi Mao (毛智)

Subject: [PATCH v3 2/6] pwm: mediatek: fix pwm source clock selection

In original code, the pwm output frequency is not correct
when set bit<3>=1 to PWMCON register.

Signed-off-by: Zhi Mao <[email protected]>
---
drivers/pwm/pwm-mediatek.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index 5c11bc7..d08b5b3 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -91,7 +91,7 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
if (clkdiv > 7)
return -EINVAL;

- mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | BIT(3) | clkdiv);
+ mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv);
mtk_pwm_writel(pc, pwm->hwpwm, PWMDWIDTH, period_ns / resolution);
mtk_pwm_writel(pc, pwm->hwpwm, PWMTHRES, duty_ns / resolution);

--
1.7.9.5

2017-06-30 06:05:58

by Zhi Mao (毛智)

Subject: [PATCH v3 5/6] pwm: mediatek: add PWM_CLK_DIV_MAX

1. Replace "7" with "PWM_CLK_DIV_MAX" in function:mtk_pwm_config()
to improve the code readablity.
2. add pwm clk disable in function:mtk_pwm_config()
for error parameter checking case.

Signed-off-by: Zhi Mao <[email protected]>
---
drivers/pwm/pwm-mediatek.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index 554a042..1d78ab1 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -30,6 +30,8 @@
#define PWMDWIDTH 0x2c
#define PWMTHRES 0x30

+#define PWM_CLK_DIV_MAX 7
+
enum {
MTK_CLK_MAIN = 0,
MTK_CLK_TOP,
@@ -130,8 +132,11 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
clkdiv++;
}

- if (clkdiv > 7)
+ if (clkdiv > PWM_CLK_DIV_MAX) {
+ mtk_pwm_clk_disable(chip, pwm);
+ dev_err(chip->dev, "period %d not supported\n", period_ns);
return -EINVAL;
+ }

mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv);
mtk_pwm_writel(pc, pwm->hwpwm, PWMDWIDTH, period_ns / resolution);
--
1.7.9.5

2017-06-30 06:06:24

by Zhi Mao (毛智)

Subject: [PATCH v3 1/6] pwm: kconfig: modify mediatek information

modify mediatek information

Signed-off-by: Zhi Mao <[email protected]>
---
drivers/pwm/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 313c107..45cdf2a 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -300,7 +300,7 @@ config PWM_MEDIATEK
Generic PWM framework driver for Mediatek ARM SoC.

To compile this driver as a module, choose M here: the module
- will be called pwm-mxs.
+ will be called pwm-mediatek.

config PWM_MXS
tristate "Freescale MXS PWM support"
--
1.7.9.5

2017-06-30 06:06:23

by Zhi Mao (毛智)

Subject: [PATCH v3 6/6] pwm: mediatek: add MT2712/MT7622 support

1. support multiple chip(MT2712, MT7622, MT7623)
2. add mtk_pwm_com_reg for match the registers of MT2712 pwm8
the register offset address of pwm8 for MT2712 is not fixed 0x40
and they are not the same as pwm0~6.

Signed-off-by: Zhi Mao <[email protected]>
---
drivers/pwm/pwm-mediatek.c | 55 +++++++++++++++++++++++++++++++++++---------
1 file changed, 44 insertions(+), 11 deletions(-)

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index 1d78ab1..2c9ce24 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -16,6 +16,7 @@
#include <linux/module.h>
#include <linux/clk.h>
#include <linux/of.h>
+#include <linux/of_device.h>
#include <linux/platform_device.h>
#include <linux/pwm.h>
#include <linux/slab.h>
@@ -40,11 +41,19 @@ enum {
MTK_CLK_PWM3,
MTK_CLK_PWM4,
MTK_CLK_PWM5,
+ MTK_CLK_PWM6,
+ MTK_CLK_PWM7,
+ MTK_CLK_PWM8,
MTK_CLK_MAX,
};

-static const char * const mtk_pwm_clk_name[] = {
- "main", "top", "pwm1", "pwm2", "pwm3", "pwm4", "pwm5"
+static const char * const mtk_pwm_clk_name[MTK_CLK_MAX] = {
+ "main", "top", "pwm1", "pwm2", "pwm3", "pwm4",
+ "pwm5", "pwm6", "pwm7", "pwm8"
+};
+
+struct mtk_com_pwm_data {
+ unsigned int pwm_nums;
};

/**
@@ -57,6 +66,11 @@ struct mtk_pwm_chip {
struct pwm_chip chip;
void __iomem *regs;
struct clk *clks[MTK_CLK_MAX];
+ const struct mtk_com_pwm_data *data;
+};
+
+static const unsigned long mtk_pwm_com_reg[] = {
+ 0x0010, 0x0050, 0x0090, 0x00d0, 0x0110, 0x0150, 0x0190, 0x0220
};

static inline struct mtk_pwm_chip *to_mtk_pwm_chip(struct pwm_chip *chip)
@@ -103,14 +117,14 @@ static void mtk_pwm_clk_disable(struct pwm_chip *chip, struct pwm_device *pwm)
static inline u32 mtk_pwm_readl(struct mtk_pwm_chip *chip, unsigned int num,
unsigned int offset)
{
- return readl(chip->regs + 0x10 + (num * 0x40) + offset);
+ return readl(chip->regs + mtk_pwm_com_reg[num] + offset);
}

static inline void mtk_pwm_writel(struct mtk_pwm_chip *chip,
unsigned int num, unsigned int offset,
u32 value)
{
- writel(value, chip->regs + 0x10 + (num * 0x40) + offset);
+ writel(value, chip->regs + mtk_pwm_com_reg[num] + offset);
}

static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -194,23 +208,28 @@ static int mtk_pwm_probe(struct platform_device *pdev)
if (!pc)
return -ENOMEM;

+ pc->data = of_device_get_match_data(&pdev->dev);
+
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
pc->regs = devm_ioremap_resource(&pdev->dev, res);
if (IS_ERR(pc->regs))
return PTR_ERR(pc->regs);

- for (i = 0; i < MTK_CLK_MAX; i++) {
+ for (i = 0; i < pc->data->pwm_nums + 2; i++) {
pc->clks[i] = devm_clk_get(&pdev->dev, mtk_pwm_clk_name[i]);
- if (IS_ERR(pc->clks[i]))
+ if (IS_ERR(pc->clks[i])) {
+ dev_err(&pdev->dev, "[PWM] clock: %s fail: %ld\n",
+ mtk_pwm_clk_name[i], PTR_ERR(pc->clks[i]));
return PTR_ERR(pc->clks[i]);
+ }
}

- platform_set_drvdata(pdev, pc);
-
pc->chip.dev = &pdev->dev;
pc->chip.ops = &mtk_pwm_ops;
pc->chip.base = -1;
- pc->chip.npwm = 5;
+ pc->chip.npwm = pc->data->pwm_nums;
+
+ platform_set_drvdata(pdev, pc);

ret = pwmchip_add(&pc->chip);
if (ret < 0) {
@@ -228,9 +247,23 @@ static int mtk_pwm_remove(struct platform_device *pdev)
return pwmchip_remove(&pc->chip);
}

+static const struct mtk_com_pwm_data mt2712_pwm_data = {
+ .pwm_nums = 8,
+};
+
+static const struct mtk_com_pwm_data mt7622_pwm_data = {
+ .pwm_nums = 6,
+};
+
+static const struct mtk_com_pwm_data mt7623_pwm_data = {
+ .pwm_nums = 5,
+};
+
static const struct of_device_id mtk_pwm_of_match[] = {
- { .compatible = "mediatek,mt7623-pwm" },
- { }
+ {.compatible = "mediatek,mt2712-pwm", .data = &mt2712_pwm_data},
+ {.compatible = "mediatek,mt7622-pwm", .data = &mt7622_pwm_data},
+ {.compatible = "mediatek,mt7623-pwm", .data = &mt7623_pwm_data},
+ {},
};
MODULE_DEVICE_TABLE(of, mtk_pwm_of_match);

--
1.7.9.5

2017-06-30 06:06:21

by Zhi Mao (毛智)

Subject: [PATCH v3 3/6] pwm: mediatek: fix clock control issue

1. prepare top/main clk in mtk_pwm_probe() function,
it will increase power consumption
and in original code these clocks is only prepeare but never enabled.
2. pwm clock should be enabled before setting pwm registers
in function: mtk_pwm_config().
3. delete "pwm_disable" in function:mtk_pwm_remove(),
as framework should disable all the pwms, before remove them.

Signed-off-by: Zhi Mao <[email protected]>
---
drivers/pwm/pwm-mediatek.c | 69 ++++++++++++++++++++++++++++++--------------
1 file changed, 47 insertions(+), 22 deletions(-)

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index d08b5b3..554a042 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -2,6 +2,7 @@
* Mediatek Pulse Width Modulator driver
*
* Copyright (C) 2015 John Crispin <[email protected]>
+ * Copyright (C) 2017 Zhi Mao <[email protected]>
*
* This file is licensed under the terms of the GNU General Public
* License version 2. This program is licensed "as is" without any
@@ -61,6 +62,42 @@ static inline struct mtk_pwm_chip *to_mtk_pwm_chip(struct pwm_chip *chip)
return container_of(chip, struct mtk_pwm_chip, chip);
}

+static int mtk_pwm_clk_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct mtk_pwm_chip *pc = to_mtk_pwm_chip(chip);
+ int ret = 0;
+
+ ret = clk_prepare_enable(pc->clks[MTK_CLK_TOP]);
+ if (ret < 0)
+ return ret;
+
+ ret = clk_prepare_enable(pc->clks[MTK_CLK_MAIN]);
+ if (ret < 0)
+ goto disable_clk_top;
+
+ ret = clk_prepare_enable(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
+ if (ret < 0)
+ goto disable_clk_main;
+
+ return 0;
+
+disable_clk_main:
+ clk_disable_unprepare(pc->clks[MTK_CLK_MAIN]);
+disable_clk_top:
+ clk_disable_unprepare(pc->clks[MTK_CLK_TOP]);
+
+ return ret;
+}
+
+static void mtk_pwm_clk_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct mtk_pwm_chip *pc = to_mtk_pwm_chip(chip);
+
+ clk_disable_unprepare(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
+ clk_disable_unprepare(pc->clks[MTK_CLK_MAIN]);
+ clk_disable_unprepare(pc->clks[MTK_CLK_TOP]);
+}
+
static inline u32 mtk_pwm_readl(struct mtk_pwm_chip *chip, unsigned int num,
unsigned int offset)
{
@@ -80,6 +117,11 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
struct mtk_pwm_chip *pc = to_mtk_pwm_chip(chip);
struct clk *clk = pc->clks[MTK_CLK_PWM1 + pwm->hwpwm];
u32 resolution, clkdiv = 0;
+ int ret;
+
+ ret = mtk_pwm_clk_enable(chip, pwm);
+ if (ret < 0)
+ return ret;

resolution = NSEC_PER_SEC / clk_get_rate(clk);

@@ -95,6 +137,8 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
mtk_pwm_writel(pc, pwm->hwpwm, PWMDWIDTH, period_ns / resolution);
mtk_pwm_writel(pc, pwm->hwpwm, PWMTHRES, duty_ns / resolution);

+ mtk_pwm_clk_disable(chip, pwm);
+
return 0;
}

@@ -104,7 +148,7 @@ static int mtk_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
u32 value;
int ret;

- ret = clk_prepare(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
+ ret = mtk_pwm_clk_enable(chip, pwm);
if (ret < 0)
return ret;

@@ -124,7 +168,7 @@ static void mtk_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
value &= ~BIT(pwm->hwpwm);
writel(value, pc->regs);

- clk_unprepare(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
+ mtk_pwm_clk_disable(chip, pwm);
}

static const struct pwm_ops mtk_pwm_ops = {
@@ -156,14 +200,6 @@ static int mtk_pwm_probe(struct platform_device *pdev)
return PTR_ERR(pc->clks[i]);
}

- ret = clk_prepare(pc->clks[MTK_CLK_TOP]);
- if (ret < 0)
- return ret;
-
- ret = clk_prepare(pc->clks[MTK_CLK_MAIN]);
- if (ret < 0)
- goto disable_clk_top;
-
platform_set_drvdata(pdev, pc);

pc->chip.dev = &pdev->dev;
@@ -174,26 +210,15 @@ static int mtk_pwm_probe(struct platform_device *pdev)
ret = pwmchip_add(&pc->chip);
if (ret < 0) {
dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
- goto disable_clk_main;
+ return ret;
}

return 0;
-
-disable_clk_main:
- clk_unprepare(pc->clks[MTK_CLK_MAIN]);
-disable_clk_top:
- clk_unprepare(pc->clks[MTK_CLK_TOP]);
-
- return ret;
}

static int mtk_pwm_remove(struct platform_device *pdev)
{
struct mtk_pwm_chip *pc = platform_get_drvdata(pdev);
- unsigned int i;
-
- for (i = 0; i < pc->chip.npwm; i++)
- pwm_disable(&pc->chip.pwms[i]);

return pwmchip_remove(&pc->chip);
}
--
1.7.9.5

2017-06-30 06:06:19

by Zhi Mao (毛智)

Subject: [PATCH v3 4/6] pwm: bindings: add MT2712/MT7622 information

add MT2712/MT7622 pwm information

Acked-by: Rob Herring <[email protected]>
Signed-off-by: Zhi Mao <[email protected]>
---
.../devicetree/bindings/pwm/pwm-mediatek.txt | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
index 54c59b0..ef8bd3c 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
+++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
@@ -2,6 +2,8 @@ MediaTek PWM controller

Required properties:
- compatible: should be "mediatek,<name>-pwm":
+ - "mediatek,mt2712-pwm": found on mt2712 SoC.
+ - "mediatek,mt7622-pwm": found on mt7622 SoC.
- "mediatek,mt7623-pwm": found on mt7623 SoC.
- reg: physical base address and length of the controller's registers.
- #pwm-cells: must be 2. See pwm.txt in this directory for a description of
@@ -10,7 +12,9 @@ Required properties:
- clock-names: must contain the following:
- "top": the top clock generator
- "main": clock used by the PWM core
- - "pwm1-5": the five per PWM clocks
+ - "pwm1-8": the eight per PWM clocks for mt2712
+ - "pwm1-6": the six per PWM clocks for mt7622
+ - "pwm1-5": the five per PWM clocks for mt7623
- pinctrl-names: Must contain a "default" entry.
- pinctrl-0: One property must exist for each entry in pinctrl-names.
See pinctrl/pinctrl-bindings.txt for details of the property values.
--
1.7.9.5

2017-07-05 11:09:57

by Matthias Brugger

Subject: Re: [PATCH v3 2/6] pwm: mediatek: fix pwm source clock selection

On 06/30/2017 08:05 AM, Zhi Mao wrote:
> In original code, the pwm output frequency is not correct
> when set bit<3>=1 to PWMCON register.
>
> Signed-off-by: Zhi Mao <[email protected]>
> ---
> drivers/pwm/pwm-mediatek.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> index 5c11bc7..d08b5b3 100644
> --- a/drivers/pwm/pwm-mediatek.c
> +++ b/drivers/pwm/pwm-mediatek.c
> @@ -91,7 +91,7 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> if (clkdiv > 7)
> return -EINVAL;
>
> - mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | BIT(3) | clkdiv);
> + mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv);

Just for clarification, BIT(15) enables old PWM mode, which ignores
CLKSEL (BIT(3)). Therefore setting BIT(3) does not have any effect and
can be discarded.

Am I correct? I took mt7623n datasheet for reference.

Regards,
Matthias

> mtk_pwm_writel(pc, pwm->hwpwm, PWMDWIDTH, period_ns / resolution);
> mtk_pwm_writel(pc, pwm->hwpwm, PWMTHRES, duty_ns / resolution);
>
>

2017-07-06 06:16:50

by Zhi Mao (毛智)

Subject: Re: [PATCH v3 2/6] pwm: mediatek: fix pwm source clock selection

On Wed, 2017-07-05 at 13:09 +0200, Matthias Brugger wrote:
>
> On 06/30/2017 08:05 AM, Zhi Mao wrote:
> > In original code, the pwm output frequency is not correct
> > when set bit<3>=1 to PWMCON register.
> >
> > Signed-off-by: Zhi Mao <[email protected]>
> > ---
> > drivers/pwm/pwm-mediatek.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> > index 5c11bc7..d08b5b3 100644
> > --- a/drivers/pwm/pwm-mediatek.c
> > +++ b/drivers/pwm/pwm-mediatek.c
> > @@ -91,7 +91,7 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > if (clkdiv > 7)
> > return -EINVAL;
> >
> > - mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | BIT(3) | clkdiv);
> > + mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv);
>
> Just for clarification, BIT(15) enables old PWM mode, which ignores
> CLKSEL (BIT(3)). Therefore setting BIT(3) does not have any effect and
> can be discarded.
>
> Am I correct? I took mt7623n datasheet for reference.
>
> Regards,
> Matthias
>
Yes, remove setting bit<3> will not take any effect.
PWMCON bit<3> is pwm source clock selecting register.
You can check the datasheet of MT7623 for details.

Regards
Zhi

> > mtk_pwm_writel(pc, pwm->hwpwm, PWMDWIDTH, period_ns / resolution);
> > mtk_pwm_writel(pc, pwm->hwpwm, PWMTHRES, duty_ns / resolution);
> >
> >

2017-07-06 06:44:07

by Zhi Mao (毛智)

Subject: Re: [PATCH v3 2/6] pwm: mediatek: fix pwm source clock selection

On Thu, 2017-07-06 at 14:16 +0800, Zhi Mao wrote:
> On Wed, 2017-07-05 at 13:09 +0200, Matthias Brugger wrote:
> >
> > On 06/30/2017 08:05 AM, Zhi Mao wrote:
> > > In original code, the pwm output frequency is not correct
> > > when set bit<3>=1 to PWMCON register.
> > >
> > > Signed-off-by: Zhi Mao <[email protected]>
> > > ---
> > > drivers/pwm/pwm-mediatek.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> > > index 5c11bc7..d08b5b3 100644
> > > --- a/drivers/pwm/pwm-mediatek.c
> > > +++ b/drivers/pwm/pwm-mediatek.c
> > > @@ -91,7 +91,7 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > > if (clkdiv > 7)
> > > return -EINVAL;
> > >
> > > - mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | BIT(3) | clkdiv);
> > > + mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv);
> >
> > Just for clarification, BIT(15) enables old PWM mode, which ignores
> > CLKSEL (BIT(3)). Therefore setting BIT(3) does not have any effect and
> > can be discarded.
> >
> > Am I correct? I took mt7623n datasheet for reference.
> >
> > Regards,
> > Matthias
> >
> Yes, remove setting bit<3> will not take any effect.
> PWMCON bit<3> is pwm source clock selecting register.
> You can check the datasheet of MT7623 for details.
>
> Regards
> Zhi
>
Hi Mattias,

Ignore the above reply, I explain this bit<3> issue for you.
In the data sheet of MT7623:
PWMCON bit<3> is PWM source clock selecting register
0: CLK=CLKSRC
1: CLK=CLKSRC/1625
for example,
bit<3>=0, PWM clk source is 26M
bit<3>=1, PWM clk source is 16K
The frequency of PWM output will based on this clock source
if set bit<3>=1, it will cause the frequency of PWM output is not
correct. I also use the oscilloscope device to measure the output,
set bit<3>=0, the output meet expectation.

Regards
Zhi
> > > mtk_pwm_writel(pc, pwm->hwpwm, PWMDWIDTH, period_ns / resolution);
> > > mtk_pwm_writel(pc, pwm->hwpwm, PWMTHRES, duty_ns / resolution);
> > >
> > >
>

2017-07-17 03:16:13

by Zhi Mao (毛智)

Subject: Re: [PATCH v3 0/6] mediatek: pwm driver add MT2712/MT7622 support

Hi John, Matthais & PWM Maintainers

Just a Gentle ping on this issue :)
Is there anything should be modified?

Regards,
Zhi


On Fri, 2017-06-30 at 14:05 +0800, Zhi Mao wrote:
> change in v3:
> 1. add pwm clk disable in function:mtk_pwm_config()
> for error parameter checking case
>
> Zhi Mao (6):
> pwm: kconfig: modify mediatek information
> pwm: mediatek: fix pwm source clock selection
> pwm: mediatek: fix clock control issue
> pwm: bindings: add MT2712/MT7622 information
> pwm: mediatek: add PWM_CLK_DIV_MAX
> pwm: mediatek: add MT2712/MT7622 support
>
> .../devicetree/bindings/pwm/pwm-mediatek.txt | 6 +-
> drivers/pwm/Kconfig | 2 +-
> drivers/pwm/pwm-mediatek.c | 133 ++++++++++++++------
> 3 files changed, 104 insertions(+), 37 deletions(-)
>

2017-07-18 16:34:23

by Matthias Brugger

Subject: Re: [PATCH v3 2/6] pwm: mediatek: fix pwm source clock selection

On 07/06/2017 08:43 AM, Zhi Mao wrote:
> On Thu, 2017-07-06 at 14:16 +0800, Zhi Mao wrote:
>> On Wed, 2017-07-05 at 13:09 +0200, Matthias Brugger wrote:
>>>
>>> On 06/30/2017 08:05 AM, Zhi Mao wrote:
>>>> In original code, the pwm output frequency is not correct
>>>> when set bit<3>=1 to PWMCON register.
>>>>
>>>> Signed-off-by: Zhi Mao <[email protected]>
>>>> ---
>>>> drivers/pwm/pwm-mediatek.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
>>>> index 5c11bc7..d08b5b3 100644
>>>> --- a/drivers/pwm/pwm-mediatek.c
>>>> +++ b/drivers/pwm/pwm-mediatek.c
>>>> @@ -91,7 +91,7 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>>>> if (clkdiv > 7)
>>>> return -EINVAL;
>>>>
>>>> - mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | BIT(3) | clkdiv);
>>>> + mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv);
>>>
>>> Just for clarification, BIT(15) enables old PWM mode, which ignores
>>> CLKSEL (BIT(3)). Therefore setting BIT(3) does not have any effect and
>>> can be discarded.
>>>
>>> Am I correct? I took mt7623n datasheet for reference.
>>>
>>> Regards,
>>> Matthias
>>>
>> Yes, remove setting bit<3> will not take any effect.
>> PWMCON bit<3> is pwm source clock selecting register.
>> You can check the datasheet of MT7623 for details.
>>
>> Regards
>> Zhi
>>
> Hi Mattias,
>
> Ignore the above reply, I explain this bit<3> issue for you.
> In the data sheet of MT7623:
> PWMCON bit<3> is PWM source clock selecting register
> 0: CLK=CLKSRC
> 1: CLK=CLKSRC/1625
> for example,
> bit<3>=0, PWM clk source is 26M
> bit<3>=1, PWM clk source is 16K
> The frequency of PWM output will based on this clock source
> if set bit<3>=1, it will cause the frequency of PWM output is not
> correct. I also use the oscilloscope device to measure the output,
> set bit<3>=0, the output meet expectation.
>

Reviewed-by: Matthias Brugger <[email protected]>

2017-08-02 07:20:05

by Zhi Mao (毛智)

Subject: Re: [PATCH v3 0/6] mediatek: pwm driver add MT2712/MT7622 support

Hi John, Matthais & Thierry,
>
> Just a gentle ping on this issue again.
> Do you have any update?
>
> Regards,
> Zhi


On Mon, 2017-07-17 at 11:16 +0800, Zhi Mao wrote:
> Hi John, Matthais & PWM Maintainers
>
> Just a Gentle ping on this issue :)
> Is there anything should be modified?
>
> Regards,
> Zhi
>
>
> On Fri, 2017-06-30 at 14:05 +0800, Zhi Mao wrote:
> > change in v3:
> > 1. add pwm clk disable in function:mtk_pwm_config()
> > for error parameter checking case
> >
> > Zhi Mao (6):
> > pwm: kconfig: modify mediatek information
> > pwm: mediatek: fix pwm source clock selection
> > pwm: mediatek: fix clock control issue
> > pwm: bindings: add MT2712/MT7622 information
> > pwm: mediatek: add PWM_CLK_DIV_MAX
> > pwm: mediatek: add MT2712/MT7622 support
> >
> > .../devicetree/bindings/pwm/pwm-mediatek.txt | 6 +-
> > drivers/pwm/Kconfig | 2 +-
> > drivers/pwm/pwm-mediatek.c | 133 ++++++++++++++------
> > 3 files changed, 104 insertions(+), 37 deletions(-)
> >
>

2017-08-02 08:42:32

by John Crispin

Subject: Re: [PATCH v3 0/6] mediatek: pwm driver add MT2712/MT7622 support

On 02/08/17 09:19, Zhi Mao wrote:
> Hi John, Matthais & Thierry,
>> Just a gentle ping on this issue again.
>> Do you have any update?
>>
>> Regards,
>> Zhi
Hi Zhi,

looks good to me

Acked-by: John Crispin <[email protected]>

John

2017-08-03 09:41:53

by Zhi Mao (毛智)

Subject: Re: [PATCH v3 0/6] mediatek: pwm driver add MT2712/MT7622 support

Thanks John's reply.

Hi Thierry & Matthais,

What's your opinion?


On Wed, 2017-08-02 at 16:42 +0800, John Crispin wrote:
>
> On 02/08/17 09:19, Zhi Mao wrote:
> > Hi John, Matthais & Thierry,
> >> Just a gentle ping on this issue again.
> >> Do you have any update?
> >>
> >> Regards,
> >> Zhi
> Hi Zhi,
>
> looks good to me
>
> Acked-by: John Crispin <[email protected]>
>
> John
>
>
>

2017-08-21 07:31:55

by Thierry Reding

Subject: Re: [PATCH v3 1/6] pwm: kconfig: modify mediatek information

On Fri, Jun 30, 2017 at 02:05:16PM +0800, Zhi Mao wrote:
> modify mediatek information
>
> Signed-off-by: Zhi Mao <[email protected]>
> ---
> drivers/pwm/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

Applied to for-4.14/drivers, with a slightly modified subject and commit
message.

Thanks,
Thierry

Attachments:

2017-08-21 07:35:22

by Thierry Reding

Subject: Re: [PATCH v3 2/6] pwm: mediatek: fix pwm source clock selection

On Fri, Jun 30, 2017 at 02:05:17PM +0800, Zhi Mao wrote:
> In original code, the pwm output frequency is not correct
> when set bit<3>=1 to PWMCON register.
>
> Signed-off-by: Zhi Mao <[email protected]>
> ---
> drivers/pwm/pwm-mediatek.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)

Applied to for-4.14/drivers, thanks.

Thierry

Attachments:

2017-08-21 07:47:56

by Thierry Reding

Subject: Re: [PATCH v3 3/6] pwm: mediatek: fix clock control issue

On Fri, Jun 30, 2017 at 02:05:18PM +0800, Zhi Mao wrote:
> 1. prepare top/main clk in mtk_pwm_probe() function,
> it will increase power consumption
> and in original code these clocks is only prepeare but never enabled.
> 2. pwm clock should be enabled before setting pwm registers
> in function: mtk_pwm_config().
> 3. delete "pwm_disable" in function:mtk_pwm_remove(),
> as framework should disable all the pwms, before remove them.
>
> Signed-off-by: Zhi Mao <[email protected]>
> ---
> drivers/pwm/pwm-mediatek.c | 69 ++++++++++++++++++++++++++++++--------------
> 1 file changed, 47 insertions(+), 22 deletions(-)

I think the commit description could be better. Try to avoid enumerating
changes like you did. Not only is it tedious to read, but this kind of
enumeration is often a sign that you can split the commit up into
multiple logical changes.

Especially the change described in 3) is not related to the clock. It's
also not a quite correct description, because there is no code in the
framework that disables PWMs on chip removal. Users should've disabled
the PWMs that they were using.

The framework could probably warn about misuse, though.

That said, I've left the change as-is, and changed the commit message to
this:

--- >8 ---
pwm: mediatek: Fix clock control issue

In order to save some power, do not prepare the top and main clocks
during mtk_pwm_probe(). Instead, prepare the clocks only when necessary
and also make sure to enable the clocks to match the semantics of the
common clock framework.

While at it, don't explicitly disable all PWM channels in ->remove()
because all users should have done that already.

Signed-off-by: Zhi Mao <[email protected]>
Acked-by: John Crispin <[email protected]>
Signed-off-by: Thierry Reding <[email protected]>
--- 8< ---

Try to keep this in mind for future patch submissions.

Applied to for-4.14/drivers, thanks.

Thierry

Attachments:

2017-08-21 07:51:29

by Thierry Reding

Subject: Re: [PATCH v3 4/6] pwm: bindings: add MT2712/MT7622 information

On Fri, Jun 30, 2017 at 02:05:19PM +0800, Zhi Mao wrote:
> add MT2712/MT7622 pwm information

Please avoid duplicating the subject in the description. You're not
adding any meaningful information. I've changed the commit to this:

--- >8 ---
dt-bindings: pwm: Add MT2712/MT7622 information

Enhance the MediaTek PWM binding with details about the IP found in the
MT2712 and MT7622 SoCs.

Acked-by: Rob Herring <[email protected]>
Signed-off-by: Zhi Mao <[email protected]>
Acked-by: John Crispin <[email protected]>
Signed-off-by: Thierry Reding <[email protected]>
--- 8< ---

Applied to for-4.14/drivers, thanks.

Thierry

Attachments:

2017-08-21 07:58:07

by Thierry Reding

Subject: Re: [PATCH v3 5/6] pwm: mediatek: add PWM_CLK_DIV_MAX

On Fri, Jun 30, 2017 at 02:05:20PM +0800, Zhi Mao wrote:
> 1. Replace "7" with "PWM_CLK_DIV_MAX" in function:mtk_pwm_config()
> to improve the code readablity.
> 2. add pwm clk disable in function:mtk_pwm_config()
> for error parameter checking case.
>
> Signed-off-by: Zhi Mao <[email protected]>
> ---
> drivers/pwm/pwm-mediatek.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)

Same comment as before. You've got two logical, unrelated changes in
this one commit. In this case you're mixing a cosmetic change with an
actual bug fix. And to make things worse, the commit subject mentions
only the cosmetic change, while the more important changes is only
described in a drive-by fashion.

You get another free pass this time, but please be more conscious about
these things in the future.

I've applied this to for-4.14/drivers with the following commit message:

--- >8 ---
pwm: mediatek: Disable clock on PWM configuration failure

Make sure to disable the PWM clock if the PWM cannot be configured due
to the clock divider exceeding the maximum value.

While at it, replace the hardcoded maximum clock divider with a defined
constant to improve code readability.

Signed-off-by: Zhi Mao <[email protected]>
Acked-by: John Crispin <[email protected]>
Signed-off-by: Thierry Reding <[email protected]>
--- 8< ---

Thierry

Attachments:

2017-08-21 08:05:17

by Thierry Reding

Subject: Re: [PATCH v3 6/6] pwm: mediatek: add MT2712/MT7622 support

On Fri, Jun 30, 2017 at 02:05:21PM +0800, Zhi Mao wrote:
> 1. support multiple chip(MT2712, MT7622, MT7623)
> 2. add mtk_pwm_com_reg for match the registers of MT2712 pwm8
> the register offset address of pwm8 for MT2712 is not fixed 0x40
> and they are not the same as pwm0~6.
>
> Signed-off-by: Zhi Mao <[email protected]>
> ---
> drivers/pwm/pwm-mediatek.c | 55 +++++++++++++++++++++++++++++++++++---------
> 1 file changed, 44 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> index 1d78ab1..2c9ce24 100644
> --- a/drivers/pwm/pwm-mediatek.c
> +++ b/drivers/pwm/pwm-mediatek.c
> @@ -16,6 +16,7 @@
> #include <linux/module.h>
> #include <linux/clk.h>
> #include <linux/of.h>
> +#include <linux/of_device.h>
> #include <linux/platform_device.h>
> #include <linux/pwm.h>
> #include <linux/slab.h>
> @@ -40,11 +41,19 @@ enum {
> MTK_CLK_PWM3,
> MTK_CLK_PWM4,
> MTK_CLK_PWM5,
> + MTK_CLK_PWM6,
> + MTK_CLK_PWM7,
> + MTK_CLK_PWM8,
> MTK_CLK_MAX,
> };
>
> -static const char * const mtk_pwm_clk_name[] = {
> - "main", "top", "pwm1", "pwm2", "pwm3", "pwm4", "pwm5"
> +static const char * const mtk_pwm_clk_name[MTK_CLK_MAX] = {
> + "main", "top", "pwm1", "pwm2", "pwm3", "pwm4",
> + "pwm5", "pwm6", "pwm7", "pwm8"

You're wrapping these lines at arbitrary boundaries. Make sure to use
all of the 80 columns at your disposal.

> +};
> +
> +struct mtk_com_pwm_data {

What does the _com stand for in the above?

> + unsigned int pwm_nums;
> };

Maybe name this num_pwms for consistency with other drivers?

>
> /**
> @@ -57,6 +66,11 @@ struct mtk_pwm_chip {
> struct pwm_chip chip;
> void __iomem *regs;
> struct clk *clks[MTK_CLK_MAX];
> + const struct mtk_com_pwm_data *data;
> +};
> +
> +static const unsigned long mtk_pwm_com_reg[] = {

There's another of these _com that I don't understand what it means.
Also since these are all fairly small offsets, these can simply be
unsigned int.

> + 0x0010, 0x0050, 0x0090, 0x00d0, 0x0110, 0x0150, 0x0190, 0x0220
> };
>
> static inline struct mtk_pwm_chip *to_mtk_pwm_chip(struct pwm_chip *chip)
> @@ -103,14 +117,14 @@ static void mtk_pwm_clk_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> static inline u32 mtk_pwm_readl(struct mtk_pwm_chip *chip, unsigned int num,
> unsigned int offset)
> {
> - return readl(chip->regs + 0x10 + (num * 0x40) + offset);
> + return readl(chip->regs + mtk_pwm_com_reg[num] + offset);
> }
>
> static inline void mtk_pwm_writel(struct mtk_pwm_chip *chip,
> unsigned int num, unsigned int offset,
> u32 value)
> {
> - writel(value, chip->regs + 0x10 + (num * 0x40) + offset);
> + writel(value, chip->regs + mtk_pwm_com_reg[num] + offset);
> }
>
> static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> @@ -194,23 +208,28 @@ static int mtk_pwm_probe(struct platform_device *pdev)
> if (!pc)
> return -ENOMEM;
>
> + pc->data = of_device_get_match_data(&pdev->dev);
> +
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> pc->regs = devm_ioremap_resource(&pdev->dev, res);
> if (IS_ERR(pc->regs))
> return PTR_ERR(pc->regs);
>
> - for (i = 0; i < MTK_CLK_MAX; i++) {
> + for (i = 0; i < pc->data->pwm_nums + 2; i++) {
> pc->clks[i] = devm_clk_get(&pdev->dev, mtk_pwm_clk_name[i]);
> - if (IS_ERR(pc->clks[i]))
> + if (IS_ERR(pc->clks[i])) {
> + dev_err(&pdev->dev, "[PWM] clock: %s fail: %ld\n",
> + mtk_pwm_clk_name[i], PTR_ERR(pc->clks[i]));

Why include the "[PWM] " prefix in the above message?

> return PTR_ERR(pc->clks[i]);
> + }
> }
>
> - platform_set_drvdata(pdev, pc);
> -
> pc->chip.dev = &pdev->dev;
> pc->chip.ops = &mtk_pwm_ops;
> pc->chip.base = -1;
> - pc->chip.npwm = 5;
> + pc->chip.npwm = pc->data->pwm_nums;
> +
> + platform_set_drvdata(pdev, pc);

No need to move the location of the platform_set_drvdata() call. It's
needless churn.

> static const struct of_device_id mtk_pwm_of_match[] = {
> - { .compatible = "mediatek,mt7623-pwm" },
> - { }
> + {.compatible = "mediatek,mt2712-pwm", .data = &mt2712_pwm_data},
> + {.compatible = "mediatek,mt7622-pwm", .data = &mt7622_pwm_data},
> + {.compatible = "mediatek,mt7623-pwm", .data = &mt7623_pwm_data},
> + {},

Spaces after { and before }, please.

Thierry

Attachments:

2017-08-21 09:05:54

by Zhi Mao (毛智)

Subject: Re: [PATCH v3 6/6] pwm: mediatek: add MT2712/MT7622 support

Hi Thierry,

Thanks for your review code.
I will modify the code as you comment in the next release.

Regards
Zhi

On Mon, 2017-08-21 at 10:05 +0200, Thierry Reding wrote:
> On Fri, Jun 30, 2017 at 02:05:21PM +0800, Zhi Mao wrote:
> > 1. support multiple chip(MT2712, MT7622, MT7623)
> > 2. add mtk_pwm_com_reg for match the registers of MT2712 pwm8
> > the register offset address of pwm8 for MT2712 is not fixed 0x40
> > and they are not the same as pwm0~6.
> >
> > Signed-off-by: Zhi Mao <[email protected]>
> > ---
> > drivers/pwm/pwm-mediatek.c | 55 +++++++++++++++++++++++++++++++++++---------
> > 1 file changed, 44 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> > index 1d78ab1..2c9ce24 100644
> > --- a/drivers/pwm/pwm-mediatek.c
> > +++ b/drivers/pwm/pwm-mediatek.c
> > @@ -16,6 +16,7 @@
> > #include <linux/module.h>
> > #include <linux/clk.h>
> > #include <linux/of.h>
> > +#include <linux/of_device.h>
> > #include <linux/platform_device.h>
> > #include <linux/pwm.h>
> > #include <linux/slab.h>
> > @@ -40,11 +41,19 @@ enum {
> > MTK_CLK_PWM3,
> > MTK_CLK_PWM4,
> > MTK_CLK_PWM5,
> > + MTK_CLK_PWM6,
> > + MTK_CLK_PWM7,
> > + MTK_CLK_PWM8,
> > MTK_CLK_MAX,
> > };
> >
> > -static const char * const mtk_pwm_clk_name[] = {
> > - "main", "top", "pwm1", "pwm2", "pwm3", "pwm4", "pwm5"
> > +static const char * const mtk_pwm_clk_name[MTK_CLK_MAX] = {
> > + "main", "top", "pwm1", "pwm2", "pwm3", "pwm4",
> > + "pwm5", "pwm6", "pwm7", "pwm8"
>
> You're wrapping these lines at arbitrary boundaries. Make sure to use
> all of the 80 columns at your disposal.
>
> > +};
> > +
> > +struct mtk_com_pwm_data {
>
> What does the _com stand for in the above?
>
> > + unsigned int pwm_nums;
> > };
>
> Maybe name this num_pwms for consistency with other drivers?
>
> >
> > /**
> > @@ -57,6 +66,11 @@ struct mtk_pwm_chip {
> > struct pwm_chip chip;
> > void __iomem *regs;
> > struct clk *clks[MTK_CLK_MAX];
> > + const struct mtk_com_pwm_data *data;
> > +};
> > +
> > +static const unsigned long mtk_pwm_com_reg[] = {
>
> There's another of these _com that I don't understand what it means.
> Also since these are all fairly small offsets, these can simply be
> unsigned int.
>
> > + 0x0010, 0x0050, 0x0090, 0x00d0, 0x0110, 0x0150, 0x0190, 0x0220
> > };
> >
> > static inline struct mtk_pwm_chip *to_mtk_pwm_chip(struct pwm_chip *chip)
> > @@ -103,14 +117,14 @@ static void mtk_pwm_clk_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> > static inline u32 mtk_pwm_readl(struct mtk_pwm_chip *chip, unsigned int num,
> > unsigned int offset)
> > {
> > - return readl(chip->regs + 0x10 + (num * 0x40) + offset);
> > + return readl(chip->regs + mtk_pwm_com_reg[num] + offset);
> > }
> >
> > static inline void mtk_pwm_writel(struct mtk_pwm_chip *chip,
> > unsigned int num, unsigned int offset,
> > u32 value)
> > {
> > - writel(value, chip->regs + 0x10 + (num * 0x40) + offset);
> > + writel(value, chip->regs + mtk_pwm_com_reg[num] + offset);
> > }
> >
> > static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > @@ -194,23 +208,28 @@ static int mtk_pwm_probe(struct platform_device *pdev)
> > if (!pc)
> > return -ENOMEM;
> >
> > + pc->data = of_device_get_match_data(&pdev->dev);
> > +
> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > pc->regs = devm_ioremap_resource(&pdev->dev, res);
> > if (IS_ERR(pc->regs))
> > return PTR_ERR(pc->regs);
> >
> > - for (i = 0; i < MTK_CLK_MAX; i++) {
> > + for (i = 0; i < pc->data->pwm_nums + 2; i++) {
> > pc->clks[i] = devm_clk_get(&pdev->dev, mtk_pwm_clk_name[i]);
> > - if (IS_ERR(pc->clks[i]))
> > + if (IS_ERR(pc->clks[i])) {
> > + dev_err(&pdev->dev, "[PWM] clock: %s fail: %ld\n",
> > + mtk_pwm_clk_name[i], PTR_ERR(pc->clks[i]));
>
> Why include the "[PWM] " prefix in the above message?
>
> > return PTR_ERR(pc->clks[i]);
> > + }
> > }
> >
> > - platform_set_drvdata(pdev, pc);
> > -
> > pc->chip.dev = &pdev->dev;
> > pc->chip.ops = &mtk_pwm_ops;
> > pc->chip.base = -1;
> > - pc->chip.npwm = 5;
> > + pc->chip.npwm = pc->data->pwm_nums;
> > +
> > + platform_set_drvdata(pdev, pc);
>
> No need to move the location of the platform_set_drvdata() call. It's
> needless churn.
>
> > static const struct of_device_id mtk_pwm_of_match[] = {
> > - { .compatible = "mediatek,mt7623-pwm" },
> > - { }
> > + {.compatible = "mediatek,mt2712-pwm", .data = &mt2712_pwm_data},
> > + {.compatible = "mediatek,mt7622-pwm", .data = &mt7622_pwm_data},
> > + {.compatible = "mediatek,mt7623-pwm", .data = &mt7623_pwm_data},
> > + {},
>
> Spaces after { and before }, please.
>
> Thierry