Fw: [PATCH v2] pwm: meson: external clock configuration for s4

Neil Armstrong narmstrong at baylibre.com
Thu Apr 21 01:42:15 PDT 2022


Hi Junyi,

On 19/04/2022 09:39, junyi.zhao at 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 at amlogic.com
> 
>     *From:* jiayi.zhou at amlogic.com <mailto:jiayi.zhou at amlogic.com>
>     *Date:* 2022-04-08 17:00
>     *To:* junyi.zhao <mailto:junyi.zhao at amlogic.com>
>     *Subject:* Fw: Re: [PATCH v2] pwm: meson: external clock configuration for s4
> 
> 
>     ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>     jiayi.zhou at amlogic.com
> 
>         *From:* Neil Armstrong <mailto:narmstrong at baylibre.com>
>         *Date:* 2022-01-14 18:16
>         *To:* Jiayi.Zhou <mailto:Jiayi.Zhou at amlogic.com>; jbrunet <mailto:jbrunet at baylibre.com>; thierry.reding <mailto:thierry.reding at gmail.com>; u.kleine-koenig <mailto:u.kleine-koenig at pengutronix.de>; lee.jones <mailto:lee.jones at linaro.org>; khilman <mailto:khilman at baylibre.com>; martin.blumenstingl <mailto:martin.blumenstingl at googlemail.com>
>         *CC:* linux-arm-kernel <mailto:linux-arm-kernel at lists.infradead.org>; linux-amlogic <mailto:linux-amlogic at lists.infradead.org>; linux-kernel <mailto:linux-kernel at 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 at amlogic.com wrote:
>          > From: "Jiayi.zhou" <jiayi.zhou at 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 at 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;
>          >
>          >
> 



More information about the linux-amlogic mailing list