Hello, this is v2 of my series to update the atmel PWM driver. (Implicit) v1 was sent on Aug 15, starting with Message-Id: 20190815214133.11134-1-uwe@kleine-koenig.org. I updated the patches from the feedback I got in v1, see the individual patches for the details. Best regards Uwe Uwe Kleine-König (6): pwm: atmel: Add a hint where to find hardware documentation pwm: atmel: use a constant for maximum prescale value pwm: atmel: replace loop in prescale calculation by ad-hoc calculation pwm: atmel: document known weaknesses of both hardware and software pwm: atmel: use atmel_pwm_writel in atmel_pwm_ch_writel; ditto for readl pwm: atmel: implement .get_state() drivers/pwm/pwm-atmel.c | 86 ++++++++++++++++++++++++++++++++++------- 1 file changed, 73 insertions(+), 13 deletions(-) -- 2.20.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Most Microchip (formerly Atmel) chips have publicly available manuals. A comprehensive list is already contained in the documentation folder. Reference this list in the header of the driver to allow reviewers to find the relevant manuals. Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> --- Changes since (implicit) v1 sent with Message-Id: 20190815214133.11134-1-uwe@kleine-koenig.org: - Only reference Documentation/arm/microchip.rst instead of starting another list of links drivers/pwm/pwm-atmel.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c index e5e1eaf372fa..a61a30fa8b7e 100644 --- a/drivers/pwm/pwm-atmel.c +++ b/drivers/pwm/pwm-atmel.c @@ -4,6 +4,9 @@ * * Copyright (C) 2013 Atmel Corporation * Bo Shen <voice.shen@atmel.com> + * + * Links to reference manuals for the supported PWM chips can be found in + * Documentation/arm/microchip.rst. */ #include <linux/clk.h> -- 2.20.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
The maximal prescale value is 10 for all supported variants. So drop the member in the variant description and introduce a global constant instead. This reduces the size of the variant descriptions and the .apply() callback can be compiled a bit more effectively. Acked-by: Claudiu Beznea <claudiu.beznea@microchip.com> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> --- Changes since (implicit) v1, sent with Message-Id: 20190815214133.11134-2-uwe@kleine-koenig.org: - Added Claudiu's Ack drivers/pwm/pwm-atmel.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c index a61a30fa8b7e..f497f84771f0 100644 --- a/drivers/pwm/pwm-atmel.c +++ b/drivers/pwm/pwm-atmel.c @@ -50,6 +50,8 @@ #define PWMV2_CPRD 0x0C #define PWMV2_CPRDUPD 0x10 +#define PWM_MAX_PRES 10 + struct atmel_pwm_registers { u8 period; u8 period_upd; @@ -59,7 +61,6 @@ struct atmel_pwm_registers { struct atmel_pwm_config { u32 max_period; - u32 max_pres; }; struct atmel_pwm_data { @@ -126,7 +127,7 @@ static int atmel_pwm_calculate_cprd_and_pres(struct pwm_chip *chip, for (*pres = 0; cycles > atmel_pwm->data->cfg.max_period; cycles >>= 1) (*pres)++; - if (*pres > atmel_pwm->data->cfg.max_pres) { + if (*pres > PWM_MAX_PRES) { dev_err(chip->dev, "pres exceeds the maximum value\n"); return -EINVAL; } @@ -289,7 +290,6 @@ static const struct atmel_pwm_data atmel_sam9rl_pwm_data = { .cfg = { /* 16 bits to keep period and duty. */ .max_period = 0xffff, - .max_pres = 10, }, }; @@ -303,7 +303,6 @@ static const struct atmel_pwm_data atmel_sama5_pwm_data = { .cfg = { /* 16 bits to keep period and duty. */ .max_period = 0xffff, - .max_pres = 10, }, }; @@ -317,7 +316,6 @@ static const struct atmel_pwm_data mchp_sam9x60_pwm_data = { .cfg = { /* 32 bits to keep period and duty. */ .max_period = 0xffffffff, - .max_pres = 10, }, }; -- 2.20.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
The calculated values are the same with the modified algorithm. The only difference is that the calculation is a bit more efficient. Acked-by: Claudiu Beznea <claudiu.beznea@microchip.com> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> --- Changes since (implicit) v1 sent with Message-Id: 20190815214133.11134-3-uwe@kleine-koenig.org - Added Claudiu's Ack drivers/pwm/pwm-atmel.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c index f497f84771f0..3786ab9db5cf 100644 --- a/drivers/pwm/pwm-atmel.c +++ b/drivers/pwm/pwm-atmel.c @@ -60,7 +60,7 @@ struct atmel_pwm_registers { }; struct atmel_pwm_config { - u32 max_period; + u32 period_bits; }; struct atmel_pwm_data { @@ -119,17 +119,27 @@ static int atmel_pwm_calculate_cprd_and_pres(struct pwm_chip *chip, { struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip); unsigned long long cycles = state->period; + int shift; /* Calculate the period cycles and prescale value */ cycles *= clk_get_rate(atmel_pwm->clk); do_div(cycles, NSEC_PER_SEC); - for (*pres = 0; cycles > atmel_pwm->data->cfg.max_period; cycles >>= 1) - (*pres)++; + /* + * The register for the period length is cfg.period_bits bits wide. + * So for each bit the number of clock cycles is wider divide the input + * clock frequency by two using pres and shift cprd accordingly. + */ + shift = fls(cycles) - atmel_pwm->data->cfg.period_bits; - if (*pres > PWM_MAX_PRES) { + if (shift > PWM_MAX_PRES) { dev_err(chip->dev, "pres exceeds the maximum value\n"); return -EINVAL; + } else if (shift > 0) { + *pres = shift; + cycles >>= *pres; + } else { + *pres = 0; } *cprd = cycles; @@ -289,7 +299,7 @@ static const struct atmel_pwm_data atmel_sam9rl_pwm_data = { }, .cfg = { /* 16 bits to keep period and duty. */ - .max_period = 0xffff, + .period_bits = 16, }, }; @@ -302,7 +312,7 @@ static const struct atmel_pwm_data atmel_sama5_pwm_data = { }, .cfg = { /* 16 bits to keep period and duty. */ - .max_period = 0xffff, + .period_bits = 16, }, }; @@ -315,7 +325,7 @@ static const struct atmel_pwm_data mchp_sam9x60_pwm_data = { }, .cfg = { /* 32 bits to keep period and duty. */ - .max_period = 0xffffffff, + .period_bits = 32, }, }; -- 2.20.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
This documents the my findings while reading through the driver and the reference manual. Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> --- Changes since (implicit) v1 sent with Message-Id: 20190816093748.11769-1-uwe@kleine-koenig.org: - Add some prosa to commit log drivers/pwm/pwm-atmel.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c index 3786ab9db5cf..89f3a62f7541 100644 --- a/drivers/pwm/pwm-atmel.c +++ b/drivers/pwm/pwm-atmel.c @@ -7,6 +7,16 @@ * * Links to reference manuals for the supported PWM chips can be found in * Documentation/arm/microchip.rst. + * + * Limitations: + * - Periods start with the inactive level. + * - Hardware has to be stopped in general to update settings. + * + * Software bugs/possible improvements: + * - When atmel_pwm_apply() is called with state->enabled=false a change in + * state->polarity isn't honored. + * - Instead of sleeping to wait for a completed period, the interrupt + * functionality could be used. */ #include <linux/clk.h> -- 2.20.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
This makes it a bit easier when instrumenting register access to only have to add code in one place. Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> --- New in v2 drivers/pwm/pwm-atmel.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c index 89f3a62f7541..152c2b7dd6df 100644 --- a/drivers/pwm/pwm-atmel.c +++ b/drivers/pwm/pwm-atmel.c @@ -111,7 +111,7 @@ static inline u32 atmel_pwm_ch_readl(struct atmel_pwm_chip *chip, { unsigned long base = PWM_CH_REG_OFFSET + ch * PWM_CH_REG_SIZE; - return readl_relaxed(chip->base + base + offset); + return atmel_pwm_readl(chip, base + offset); } static inline void atmel_pwm_ch_writel(struct atmel_pwm_chip *chip, @@ -120,7 +120,7 @@ static inline void atmel_pwm_ch_writel(struct atmel_pwm_chip *chip, { unsigned long base = PWM_CH_REG_OFFSET + ch * PWM_CH_REG_SIZE; - writel_relaxed(val, chip->base + base + offset); + atmel_pwm_writel(chip, base + offset, val); } static int atmel_pwm_calculate_cprd_and_pres(struct pwm_chip *chip, -- 2.20.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
This function reads back the configured parameters from the hardware. As .apply rounds down (mostly) I'm rounding up in .get_state() to achieve that applying a state just read from hardware is a no-op. Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> --- New in v2 drivers/pwm/pwm-atmel.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c index 152c2b7dd6df..f788501ab6ca 100644 --- a/drivers/pwm/pwm-atmel.c +++ b/drivers/pwm/pwm-atmel.c @@ -295,8 +295,47 @@ static int atmel_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, return 0; } +static void atmel_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, + struct pwm_state *state) +{ + struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip); + u32 sr, cmr; + + sr = atmel_pwm_readl(atmel_pwm, PWM_SR); + cmr = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR); + + if (sr & (1 << pwm->hwpwm)) { + unsigned long rate = clk_get_rate(atmel_pwm->clk); + u32 cdty, cprd, pres; + u64 tmp; + + pres = cmr & PWM_CMR_CPRE_MSK; + + cprd = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, atmel_pwm->data->regs.period); + tmp = (u64)cprd * NSEC_PER_SEC; + tmp <<= pres; + state->period = DIV64_U64_ROUND_UP(tmp, rate); + + cdty = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, atmel_pwm->data->regs.duty); + tmp = (u64)cdty * NSEC_PER_SEC; + tmp <<= pres; + state->duty_cycle = DIV64_U64_ROUND_UP(tmp, rate); + + state->enabled = true; + } else { + state->enabled = false; + } + + if (cmr & PWM_CMR_CPOL) + state->polarity = PWM_POLARITY_INVERSED; + else + state->polarity = PWM_POLARITY_NORMAL; + +} + static const struct pwm_ops atmel_pwm_ops = { .apply = atmel_pwm_apply, + .get_state = atmel_pwm_get_state, .owner = THIS_MODULE, }; -- 2.20.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 24/08/2019 at 02:10, Uwe Kleine-König wrote: > External E-Mail > > > Most Microchip (formerly Atmel) chips have publicly available manuals. > A comprehensive list is already contained in the documentation folder. > Reference this list in the header of the driver to allow reviewers to > find the relevant manuals. > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com> Thanks Uwe! Best regards, Nicolas > --- > Changes since (implicit) v1 sent with Message-Id: > 20190815214133.11134-1-uwe@kleine-koenig.org: > > - Only reference Documentation/arm/microchip.rst instead of starting > another list of links > > drivers/pwm/pwm-atmel.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c > index e5e1eaf372fa..a61a30fa8b7e 100644 > --- a/drivers/pwm/pwm-atmel.c > +++ b/drivers/pwm/pwm-atmel.c > @@ -4,6 +4,9 @@ > * > * Copyright (C) 2013 Atmel Corporation > * Bo Shen <voice.shen@atmel.com> > + * > + * Links to reference manuals for the supported PWM chips can be found in > + * Documentation/arm/microchip.rst. > */ > > #include <linux/clk.h> > -- Nicolas Ferre _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 24.08.2019 03:10, Uwe Kleine-König wrote: > External E-Mail > > > This function reads back the configured parameters from the hardware. As > .apply rounds down (mostly) I'm rounding up in .get_state() to achieve > that applying a state just read from hardware is a no-op. Since this read is only at probing, at least for the moment, and, as far as I remember, the idea w/ .get_state was to reflect in Linux the states of PWMs that were setup before Linux takes control (e.g. PWMs setup in bootloaders) I think it would no problem if it would be no-ops in this scenario. In case of run-time state retrieval, pwm_get_state() should be enough. If one would get the state previously saved w/ this .get_state API he/she would change it, then it would apply the changes to the hardware. No changes of PWM state would be anyway skipped from the beginning, in pwm_apply_state() by this code: if (state->period == pwm->state.period && state->duty_cycle == pwm->state.duty_cycle && state->polarity == pwm->state.polarity && state->enabled == pwm->state.enabled) return 0; But maybe I'm missing something. > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> > --- > New in v2 > > drivers/pwm/pwm-atmel.c | 39 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c > index 152c2b7dd6df..f788501ab6ca 100644 > --- a/drivers/pwm/pwm-atmel.c > +++ b/drivers/pwm/pwm-atmel.c > @@ -295,8 +295,47 @@ static int atmel_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > return 0; > } > > +static void atmel_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > + struct pwm_state *state) > +{ > + struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip); > + u32 sr, cmr; > + > + sr = atmel_pwm_readl(atmel_pwm, PWM_SR); > + cmr = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR); > + > + if (sr & (1 << pwm->hwpwm)) { > + unsigned long rate = clk_get_rate(atmel_pwm->clk); > + u32 cdty, cprd, pres; There is a whitespace at the end of this line. > + u64 tmp; > + > + pres = cmr & PWM_CMR_CPRE_MSK; > + > + cprd = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, atmel_pwm->data->regs.period); If this is possible, please try to keep it at 80 chars per line. In my opinion this still looks good: cprd = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, atmel_pwm->data->regs.period); > + tmp = (u64)cprd * NSEC_PER_SEC; > + tmp <<= pres; > + state->period = DIV64_U64_ROUND_UP(tmp, rate); > + > + cdty = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, atmel_pwm->data->regs.duty); Ditto. > + tmp = (u64)cdty * NSEC_PER_SEC; > + tmp <<= pres; > + state->duty_cycle = DIV64_U64_ROUND_UP(tmp, rate); > + > + state->enabled = true; > + } else { > + state->enabled = false; > + } > + > + if (cmr & PWM_CMR_CPOL) > + state->polarity = PWM_POLARITY_INVERSED; > + else > + state->polarity = PWM_POLARITY_NORMAL; > + > +} > + > static const struct pwm_ops atmel_pwm_ops = { > .apply = atmel_pwm_apply, > + .get_state = atmel_pwm_get_state, > .owner = THIS_MODULE, > }; Other than the minor things above, Acked-by: Claudiu Beznea <claudiu.beznea@microchip.com> > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hello Claudiu, On Wed, Aug 28, 2019 at 10:26:18AM +0000, Claudiu.Beznea@microchip.com wrote: > On 24.08.2019 03:10, Uwe Kleine-König wrote: > > External E-Mail > > This function reads back the configured parameters from the hardware. As > > .apply rounds down (mostly) I'm rounding up in .get_state() to achieve > > that applying a state just read from hardware is a no-op. > > Since this read is only at probing, at least for the moment, and, as far as Yes, up to now .get_state() is only called at probing time. There is a patch series (by me) on the list that changes that. (Though I'm not entirely sure this is a good idea. Will comment my doubts in that thread later.) > I remember, the idea w/ .get_state was to reflect in Linux the states of > PWMs that were setup before Linux takes control (e.g. PWMs setup in > bootloaders) I think it would no problem if it would be no-ops in this > scenario. IMHO it should be a no-op. > In case of run-time state retrieval, pwm_get_state() should be > enough. If one would get the state previously saved w/ this .get_state API > he/she would change it, then it would apply the changes to the hardware. No > changes of PWM state would be anyway skipped from the beginning, in > pwm_apply_state() by this code: > > if (state->period == pwm->state.period && > state->duty_cycle == pwm->state.duty_cycle && > state->polarity == pwm->state.polarity && > state->enabled == pwm->state.enabled) > return 0; > > But maybe I'm missing something. There is a problem I want to solve generally, not only for the atmel driver. For example I consider it "expected" that s1 = pwm_get_state(pwm) pwm_apply_state(pwm, s2) pwm_apply_state(pwm, s1) ends in the same configuration as it started. For that it is necessary (even for the atmel driver with the guard you pointed out above) to round up in .get_state if .apply rounds down. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hello Thierry, On Sat, Aug 24, 2019 at 02:10:35AM +0200, Uwe Kleine-König wrote: > this is v2 of my series to update the atmel PWM driver. (Implicit) v1 > was sent on Aug 15, starting with Message-Id: > 20190815214133.11134-1-uwe@kleine-koenig.org. > > I updated the patches from the feedback I got in v1, see the individual > patches for the details. This patch series is marked "Superseeded" in patchwork[1] but AFAIC this is the most recent version of this series. Can you please reconsider applying this series? Best regards Uwe [1] https://patchwork.ozlabs.org/project/linux-pwm/list/?series=127096&state=* -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
[-- Attachment #1.1: Type: text/plain, Size: 1105 bytes --] On Sat, Aug 24, 2019 at 02:10:35AM +0200, Uwe Kleine-König wrote: > Hello, > > this is v2 of my series to update the atmel PWM driver. (Implicit) v1 > was sent on Aug 15, starting with Message-Id: > 20190815214133.11134-1-uwe@kleine-koenig.org. > > I updated the patches from the feedback I got in v1, see the individual > patches for the details. > > Best regards > Uwe > > Uwe Kleine-König (6): > pwm: atmel: Add a hint where to find hardware documentation > pwm: atmel: use a constant for maximum prescale value > pwm: atmel: replace loop in prescale calculation by ad-hoc calculation > pwm: atmel: document known weaknesses of both hardware and software > pwm: atmel: use atmel_pwm_writel in atmel_pwm_ch_writel; ditto for > readl > pwm: atmel: implement .get_state() > > drivers/pwm/pwm-atmel.c | 86 ++++++++++++++++++++++++++++++++++------- > 1 file changed, 73 insertions(+), 13 deletions(-) There were two patches in this that were not reviewed or acked, but they seem trivial enough, so I've just applied the whole series. Thanks, Thierry [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel