Messages in this thread | | | Date | Thu, 21 Apr 2022 10:42:15 +0200 | Subject | Re: Fw: [PATCH v2] pwm: meson: external clock configuration for s4 | From | Neil Armstrong <> |
| |
Hi Junyi,
On 19/04/2022 09:39, junyi.zhao@amlogic.com wrote: > > Hi Neil Armstrong,have u check this? > Because of jiayi's personnel changes, this pwm upstream flow under my charge now.
What's the state of the patchset ?
On my side, I did a review of the v2 patchset, if you have some questions on my comments, please ask them in-line.
Overall, the patchset could be much simpler since basically the PWM mux is now in the clock controller.
Thanks, Neil
> ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ > junyi.zhao@amlogic.com > > *From:* jiayi.zhou@amlogic.com <mailto:jiayi.zhou@amlogic.com> > *Date:* 2022-04-08 17:00 > *To:* junyi.zhao <mailto:junyi.zhao@amlogic.com> > *Subject:* Fw: Re: [PATCH v2] pwm: meson: external clock configuration for s4 > > > ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ > jiayi.zhou@amlogic.com > > *From:* Neil Armstrong <mailto:narmstrong@baylibre.com> > *Date:* 2022-01-14 18:16 > *To:* Jiayi.Zhou <mailto:Jiayi.Zhou@amlogic.com>; jbrunet <mailto:jbrunet@baylibre.com>; thierry.reding <mailto:thierry.reding@gmail.com>; u.kleine-koenig <mailto:u.kleine-koenig@pengutronix.de>; lee.jones <mailto:lee.jones@linaro.org>; khilman <mailto:khilman@baylibre.com>; martin.blumenstingl <mailto:martin.blumenstingl@googlemail.com> > *CC:* linux-arm-kernel <mailto:linux-arm-kernel@lists.infradead.org>; linux-amlogic <mailto:linux-amlogic@lists.infradead.org>; linux-kernel <mailto:linux-kernel@vger.kernel.org> > *Subject:* Re: [PATCH v2] pwm: meson: external clock configuration for s4 > [ EXTERNAL EMAIL ] > Hi, > On 14/01/2022 10:07, Jiayi.Zhou@amlogic.com wrote: > > From: "Jiayi.zhou" <jiayi.zhou@amlogic.com> > > > > For PWM controller in the Meson-S4 SoC, > > PWM needs to obtain an external clock source. > > This patch tries to describe them in the DT compatible data. > Can you be more explicit here ? On which SoC ? Why ? > As I understand while reading the S805X2 datasheet, now the PWM clock input is > done in independent CLKCTRL registers and no more in the PWM registers, right ? > So with S4, instead of passing the mux input clock of clkin0 & clkin1, now you pass > the PWM mux output clock. > > > > Signed-off-by: Jiayi.zhou <jiayi.zhou@amlogic.com> > > --- > > drivers/pwm/pwm-meson.c | 145 ++++++++++++++++++++++++++++++---------- > > 1 file changed, 110 insertions(+), 35 deletions(-) > > > > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c > > index 3cf3bcf5ddfc..54db56c95be2 100644 > > --- a/drivers/pwm/pwm-meson.c > > +++ b/drivers/pwm/pwm-meson.c > > @@ -51,6 +51,7 @@ > > #define REG_MISC_AB 0x8 > > #define MISC_B_CLK_EN BIT(23) > > #define MISC_A_CLK_EN BIT(15) > > + > > #define MISC_CLK_DIV_MASK 0x7f > > #define MISC_B_CLK_DIV_SHIFT 16 > > #define MISC_A_CLK_DIV_SHIFT 8 > > @@ -93,11 +94,13 @@ struct meson_pwm_channel { > > struct clk *clk_parent; > > struct clk_mux mux; > > struct clk *clk; > > + struct clk *clk_mux; > Why do you add a mux ? > > }; > > > > struct meson_pwm_data { > > const char * const *parent_names; > > unsigned int num_parents; > > + unsigned int extern_clk; > > }; > > > > struct meson_pwm { > > @@ -105,6 +108,7 @@ struct meson_pwm { > > const struct meson_pwm_data *data; > > struct meson_pwm_channel channels[MESON_NUM_PWMS]; > > void __iomem *base; > > + > > /* > > * Protects register (write) access to the REG_MISC_AB register > > * that is shared between the two PWMs. > > @@ -130,13 +134,15 @@ static int meson_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) > > > > channel = &meson->channels[pwm->hwpwm]; > > > > - if (channel->clk_parent) { > > - err = clk_set_parent(channel->clk, channel->clk_parent); > > - if (err < 0) { > > - dev_err(dev, "failed to set parent %s for %s: %d\n", > > - __clk_get_name(channel->clk_parent), > > - __clk_get_name(channel->clk), err); > > - return err; > > + if (!meson->data->extern_clk) { > > + if (channel->clk_parent) { > > + err = clk_set_parent(channel->clk, channel->clk_parent); > > + if (err < 0) { > > + dev_err(dev, "failed to set parent %s for %s: %d\n", > > + __clk_get_name(channel->clk_parent), > > + __clk_get_name(channel->clk), err); > > + return err; > > + } > > } > > } > > > > @@ -171,7 +177,11 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, > > if (state->polarity == PWM_POLARITY_INVERSED) > > duty = period - duty; > > > > - fin_freq = clk_get_rate(channel->clk); > > + if (meson->data->extern_clk) { > > + fin_freq = clk_get_rate(channel->clk_mux);> + } else { > > + fin_freq = clk_get_rate(channel->clk); > > + } > > if (fin_freq == 0) { > > dev_err(meson->chip.dev, "invalid source clock frequency\n"); > > return -EINVAL; > > @@ -228,17 +238,32 @@ static void meson_pwm_enable(struct meson_pwm *meson, struct pwm_device *pwm) > > struct meson_pwm_channel_data *channel_data; > > unsigned long flags; > > u32 value; > > + unsigned long set_clk; > > + u32 err; > > > > channel_data = &meson_pwm_per_channel_data[pwm->hwpwm]; > > > > - spin_lock_irqsave(&meson->lock, flags); > > - > > value = readl(meson->base + REG_MISC_AB); > > - value &= ~(MISC_CLK_DIV_MASK << channel_data->clk_div_shift); > > - value |= channel->pre_div << channel_data->clk_div_shift; > > - value |= channel_data->clk_en_mask; > > - writel(value, meson->base + REG_MISC_AB); > > + if (meson->data->extern_clk) { > > + set_clk = clk_get_rate(channel->clk_mux); > > + > > + if (set_clk == 0) > > + dev_err(meson->chip.dev, "invalid source clock frequency\n"); > > + set_clk /= channel->pre_div + 1; > > + err = clk_set_rate(channel->clk, set_clk); > > + if (err) > > + dev_err(meson->chip.dev, "%s: error in setting pwm rate!\n", __func__); > > + } else { > > + spin_lock_irqsave(&meson->lock, flags); > > + > > + value &= ~(MISC_CLK_DIV_MASK << channel_data->clk_div_shift); > > + value |= channel->pre_div << channel_data->clk_div_shift; > > + value |= channel_data->clk_en_mask; > > + writel(value, meson->base + REG_MISC_AB); > > > > + spin_unlock_irqrestore(&meson->lock, flags); > > + } > > + spin_lock_irqsave(&meson->lock, flags); > > value = FIELD_PREP(PWM_HIGH_MASK, channel->hi) | > > FIELD_PREP(PWM_LOW_MASK, channel->lo); > > writel(value, meson->base + channel_data->reg_offset); > > @@ -246,7 +271,6 @@ static void meson_pwm_enable(struct meson_pwm *meson, struct pwm_device *pwm) > > value = readl(meson->base + REG_MISC_AB); > > value |= channel_data->pwm_en_mask; > > writel(value, meson->base + REG_MISC_AB); > > - > > spin_unlock_irqrestore(&meson->lock, flags); > > } > > > > @@ -333,7 +357,8 @@ static void meson_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > > struct meson_pwm *meson = to_meson_pwm(chip); > > struct meson_pwm_channel_data *channel_data; > > struct meson_pwm_channel *channel; > > - u32 value, tmp; > > + u32 value_pwm, value, tmp; > > + unsigned long gate_clk, fre; > > > > if (!state) > > return; > > @@ -341,30 +366,42 @@ static void meson_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > > channel = &meson->channels[pwm->hwpwm]; > > channel_data = &meson_pwm_per_channel_data[pwm->hwpwm]; > > > > - value = readl(meson->base + REG_MISC_AB); > > - > > - tmp = channel_data->pwm_en_mask | channel_data->clk_en_mask; > > - state->enabled = (value & tmp) == tmp; > > - > > - tmp = value >> channel_data->clk_div_shift; > > - channel->pre_div = FIELD_GET(MISC_CLK_DIV_MASK, tmp); > > - > > + value_pwm = readl(meson->base + REG_MISC_AB); > > value = readl(meson->base + channel_data->reg_offset); > > > > channel->lo = FIELD_GET(PWM_LOW_MASK, value); > > channel->hi = FIELD_GET(PWM_HIGH_MASK, value); > > > > - if (channel->lo == 0) { > > - state->period = meson_pwm_cnt_to_ns(chip, pwm, channel->hi); > > - state->duty_cycle = state->period; > > - } else if (channel->lo >= channel->hi) { > > - state->period = meson_pwm_cnt_to_ns(chip, pwm, > > - channel->lo + channel->hi); > > - state->duty_cycle = meson_pwm_cnt_to_ns(chip, pwm, > > - channel->hi); > > + if (meson->data->extern_clk) { > > + gate_clk = clk_get_rate(channel->clk); > > + fre = gate_clk / (channel->lo + channel->hi); > > + state->period = div_u64(NSEC_PER_SEC, fre); > > + state->duty_cycle = div_u64(state->period * channel->hi, channel->lo + channel->hi); > > + tmp = channel_data->pwm_en_mask; > > + > > + if (((value_pwm & tmp) == tmp) && __clk_is_enabled(channel->clk)) > > + state->enabled = true; > > + else > > + state->enabled = false; > > } else { > > - state->period = 0; > > - state->duty_cycle = 0; > > + tmp = channel_data->pwm_en_mask | channel_data->clk_en_mask; > > + > > + state->enabled = (value_pwm & tmp) == tmp; > > + > > + tmp = value_pwm >> channel_data->clk_div_shift; > > + channel->pre_div = FIELD_GET(MISC_CLK_DIV_MASK, tmp); > > + > > + if (channel->lo == 0) { > > + state->period = meson_pwm_cnt_to_ns(chip, pwm, channel->hi); > > + state->duty_cycle = state->period; > > + } else if (channel->lo >= channel->hi) { > > + state->period = meson_pwm_cnt_to_ns(chip, pwm, channel->lo > > + + channel->hi); > > + state->duty_cycle = meson_pwm_cnt_to_ns(chip, pwm, channel->hi); > > + } else { > > + state->period = 0; > > + state->duty_cycle = 0; > > + } > > } > > } > You need to rewrite the clock calculation entirely, instead of relying on a fin_freq, you need > to set_rate() the needed rate to the input clock, then calculate the periods on the rate > set by CCF. > One way is to forget trying to set a rate of the input clock, simply get_rate() of the input clock > and try to calculate periods on it. > Instead rely on assignes-clock-parent & assigned-clock-rate in DT to set the right parent of the mux > and rate of the mux post divider to feed to correct rate to the PWM controller. > > > > @@ -452,6 +489,10 @@ static const struct meson_pwm_data pwm_g12a_ee_data = { > > .num_parents = ARRAY_SIZE(pwm_g12a_ee_parent_names), > > }; > > > > +static const struct meson_pwm_data pwm_meson_data = { > > + .extern_clk = true, > > +}; > > + > > static const struct of_device_id meson_pwm_matches[] = { > > { > > .compatible = "amlogic,meson8b-pwm", > > @@ -485,6 +526,10 @@ static const struct of_device_id meson_pwm_matches[] = { > > .compatible = "amlogic,meson-g12a-ao-pwm-cd", > > .data = &pwm_g12a_ao_cd_data > > }, > > + { > > + .compatible = "amlogic,meson-s4-pwm", > > + .data = &pwm_meson_data > > + }, > > {}, > > }; > > MODULE_DEVICE_TABLE(of, meson_pwm_matches); > > @@ -534,6 +579,33 @@ static int meson_pwm_init_channels(struct meson_pwm *meson) > > return 0; > > } > > > > +static int meson_pwm_extern_init_channels(struct meson_pwm *meson) > > +{ > > + struct meson_pwm_channel *channels = meson->channels; > > + struct device *dev = meson->chip.dev; > > + unsigned int i; > > + char name[255]; > > + > > + for (i = 0; i < meson->chip.npwm; i++) { > > + snprintf(name, sizeof(name), "clkin%u", i); > > + (channels + i)->clk = devm_clk_get(dev, name); > > + if (IS_ERR((channels + i)->clk)) { > > + dev_err_probe(meson->chip.dev, PTR_ERR((channels + i)->clk), > > + "can't get device clock\n"); > > + return PTR_ERR((channels + i)->clk); > > + } > > + snprintf(name, sizeof(name), "mux%u", i); > > + (channels + i)->clk_mux = devm_clk_get(dev, name); > > + if (IS_ERR((channels + i)->clk_mux)) { > > + dev_err_probe(meson->chip.dev, PTR_ERR((channels + i)->clk_mux), > > + "can't get device clock\n"); > > + return PTR_ERR((channels + i)->clk_mux); > If you add mux you'll need to changes the bindings. > > + } > > + } > > + > > + return 0; > > +} > > + > > static int meson_pwm_probe(struct platform_device *pdev) > > { > > struct meson_pwm *meson; > > @@ -553,8 +625,11 @@ static int meson_pwm_probe(struct platform_device *pdev) > > meson->chip.npwm = MESON_NUM_PWMS; > > > > meson->data = of_device_get_match_data(&pdev->dev); > > + if (meson->data->extern_clk) > > + err = meson_pwm_extern_init_channels(meson); > > + else > > + err = meson_pwm_init_channels(meson); > > > > - err = meson_pwm_init_channels(meson); > > if (err < 0) > > return err; > > > > >
| |