| Message ID | fad131e9-265f-6c4d-3223-932f69c9a927@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pwm: meson: add support for S4 chip family | expand |
Hello, On Fri, Mar 24, 2023 at 11:23:09PM +0100, Heiner Kallweit wrote: > This adds pwm support for (at least) the s4 chip family. The extension > is based on the vendor driver that can be found at [0]. There the > version with the new clock handling is called meson-v2-pwm. > Central change is that the clock is now fully provided by the SoC clock > core. The multiplexer isn't any longer part of the pwm block. > > This was tested on a sc2-based system that uses the same pwm block. > > [0] https://github.com/khadas/linux/blob/khadas-vims-5.4.y/drivers/pwm/pwm-meson.c > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > Adding the amlogic,meson-s4-pwm compatible to the documentation was part > of the yaml conversion already. This refers to https://lore.kernel.org/linux-pwm/3edc5ba6-bf3d-e45b-377a-9e7ece7642a7@gmail.com Does the external mux clk behave in the same way as the internal ones before? (I.e. it can select one of a few parents and has a single divider?) > --- > drivers/pwm/pwm-meson.c | 38 ++++++++++++++++++++++++++++++++++---- > 1 file changed, 34 insertions(+), 4 deletions(-) > > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c > index 16d79ca5d..7a93fdada 100644 > --- a/drivers/pwm/pwm-meson.c > +++ b/drivers/pwm/pwm-meson.c > @@ -98,6 +98,7 @@ struct meson_pwm_channel { > struct meson_pwm_data { > const char * const *parent_names; > unsigned int num_parents; > + unsigned int ext_clk:1; > }; > > struct meson_pwm { > @@ -158,6 +159,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, > struct meson_pwm_channel *channel = &meson->channels[pwm->hwpwm]; > unsigned int duty, period, pre_div, cnt, duty_cnt; > unsigned long fin_freq; > + int err; > > duty = state->duty_cycle; > period = state->period; > @@ -165,6 +167,14 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, > if (state->polarity == PWM_POLARITY_INVERSED) > duty = period - duty; > > + if (meson->data->ext_clk) { err could be local to this block. > + err = clk_set_rate(channel->clk, 0xffffUL * NSEC_PER_SEC / period); > + if (err) { > + dev_err(meson->chip.dev, "failed to set pwm clock rate\n"); > + return err; > + } > + } > + > fin_freq = clk_get_rate(channel->clk); > if (fin_freq == 0) { > dev_err(meson->chip.dev, "invalid source clock frequency\n"); > @@ -173,10 +183,14 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, > > dev_dbg(meson->chip.dev, "fin_freq: %lu Hz\n", fin_freq); > > - pre_div = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * 0xffffLL); > - if (pre_div > MISC_CLK_DIV_MASK) { > - dev_err(meson->chip.dev, "unable to get period pre_div\n"); > - return -EINVAL; > + if (meson->data->ext_clk) { > + pre_div = 0; > + } else { > + pre_div = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * 0xffffLL); > + if (pre_div > MISC_CLK_DIV_MASK) { > + dev_err(meson->chip.dev, "unable to get period pre_div\n"); > + return -EINVAL; > + } > } > > cnt = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * (pre_div + 1)); > @@ -445,6 +459,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_s4_data = { > + .ext_clk = 1, > +}; > + > static const struct of_device_id meson_pwm_matches[] = { > { > .compatible = "amlogic,meson8b-pwm", > @@ -478,6 +496,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_s4_data > + }, > {}, > }; > MODULE_DEVICE_TABLE(of, meson_pwm_matches); > @@ -493,6 +515,14 @@ static int meson_pwm_init_channels(struct meson_pwm *meson) > for (i = 0; i < meson->chip.npwm; i++) { > struct meson_pwm_channel *channel = &meson->channels[i]; > > + if (meson->data->ext_clk) { > + snprintf(name, sizeof(name), "clkin%u", i); > + channel->clk = devm_clk_get(dev, name); > + if (IS_ERR(channel->clk)) > + return PTR_ERR(channel->clk); > + continue; > + } > + This requires a binding change, right? Would it make sense to drop the ext_clk flag and determine that by trying to get the clk and if it's there, assume ext_clk would have been set? Also I wonder if it would make sense to use the same name as used when the mux clk is internal, i.e. reuse name from the line below. > snprintf(name, sizeof(name), "%s#mux%u", dev_name(dev), i); Best regards Uwe
On 25.03.2023 09:20, Uwe Kleine-König wrote: > Hello, > > On Fri, Mar 24, 2023 at 11:23:09PM +0100, Heiner Kallweit wrote: >> This adds pwm support for (at least) the s4 chip family. The extension >> is based on the vendor driver that can be found at [0]. There the >> version with the new clock handling is called meson-v2-pwm. >> Central change is that the clock is now fully provided by the SoC clock >> core. The multiplexer isn't any longer part of the pwm block. >> >> This was tested on a sc2-based system that uses the same pwm block. >> >> [0] https://github.com/khadas/linux/blob/khadas-vims-5.4.y/drivers/pwm/pwm-meson.c >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> Adding the amlogic,meson-s4-pwm compatible to the documentation was part >> of the yaml conversion already. > > This refers to > https://lore.kernel.org/linux-pwm/3edc5ba6-bf3d-e45b-377a-9e7ece7642a7@gmail.com > Right > Does the external mux clk behave in the same way as the internal ones > before? (I.e. it can select one of a few parents and has a single > divider?) > Yes, it's a standard clock with few parents, a mux, and a divider. >> --- >> drivers/pwm/pwm-meson.c | 38 ++++++++++++++++++++++++++++++++++---- >> 1 file changed, 34 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c >> index 16d79ca5d..7a93fdada 100644 >> --- a/drivers/pwm/pwm-meson.c >> +++ b/drivers/pwm/pwm-meson.c >> @@ -98,6 +98,7 @@ struct meson_pwm_channel { >> struct meson_pwm_data { >> const char * const *parent_names; >> unsigned int num_parents; >> + unsigned int ext_clk:1; >> }; >> >> struct meson_pwm { >> @@ -158,6 +159,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, >> struct meson_pwm_channel *channel = &meson->channels[pwm->hwpwm]; >> unsigned int duty, period, pre_div, cnt, duty_cnt; >> unsigned long fin_freq; >> + int err; >> >> duty = state->duty_cycle; >> period = state->period; >> @@ -165,6 +167,14 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, >> if (state->polarity == PWM_POLARITY_INVERSED) >> duty = period - duty; >> >> + if (meson->data->ext_clk) { > > err could be local to this block. > OK >> + err = clk_set_rate(channel->clk, 0xffffUL * NSEC_PER_SEC / period); >> + if (err) { >> + dev_err(meson->chip.dev, "failed to set pwm clock rate\n"); >> + return err; >> + } >> + } >> + >> fin_freq = clk_get_rate(channel->clk); >> if (fin_freq == 0) { >> dev_err(meson->chip.dev, "invalid source clock frequency\n"); >> @@ -173,10 +183,14 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, >> >> dev_dbg(meson->chip.dev, "fin_freq: %lu Hz\n", fin_freq); >> >> - pre_div = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * 0xffffLL); >> - if (pre_div > MISC_CLK_DIV_MASK) { >> - dev_err(meson->chip.dev, "unable to get period pre_div\n"); >> - return -EINVAL; >> + if (meson->data->ext_clk) { >> + pre_div = 0; >> + } else { >> + pre_div = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * 0xffffLL); >> + if (pre_div > MISC_CLK_DIV_MASK) { >> + dev_err(meson->chip.dev, "unable to get period pre_div\n"); >> + return -EINVAL; >> + } >> } >> >> cnt = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * (pre_div + 1)); >> @@ -445,6 +459,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_s4_data = { >> + .ext_clk = 1, >> +}; >> + >> static const struct of_device_id meson_pwm_matches[] = { >> { >> .compatible = "amlogic,meson8b-pwm", >> @@ -478,6 +496,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_s4_data >> + }, >> {}, >> }; >> MODULE_DEVICE_TABLE(of, meson_pwm_matches); >> @@ -493,6 +515,14 @@ static int meson_pwm_init_channels(struct meson_pwm *meson) >> for (i = 0; i < meson->chip.npwm; i++) { >> struct meson_pwm_channel *channel = &meson->channels[i]; >> >> + if (meson->data->ext_clk) { >> + snprintf(name, sizeof(name), "clkin%u", i); >> + channel->clk = devm_clk_get(dev, name); >> + if (IS_ERR(channel->clk)) >> + return PTR_ERR(channel->clk); >> + continue; >> + } >> + > > This requires a binding change, right? Would it make sense to drop the > ext_clk flag and determine that by trying to get the clk and if it's > there, assume ext_clk would have been set? > Clocks clkin0 and clkin1 are used already, therefore the binding doesn't change. Just the type of clock usage is different. So far the clocks are the parents for the internal mux/div, now it's the "final" clocks. > Also I wonder if it would make sense to use the same name as used when > the mux clk is internal, i.e. reuse name from the line below. > IMO this name is ok when used internally, but wouldn't be nice if used in the binding, e.g. because part of it is dev_name(dev). Therefore I'd prefer to stick with the clkin0/clkin1 names. >> snprintf(name, sizeof(name), "%s#mux%u", dev_name(dev), i); > > Best regards > Uwe > Heiner
On Fri 24 Mar 2023 at 23:23, Heiner Kallweit <hkallweit1@gmail.com> wrote: > This adds pwm support for (at least) the s4 chip family. The extension > is based on the vendor driver that can be found at [0]. There the > version with the new clock handling is called meson-v2-pwm. > Central change is that the clock is now fully provided by the SoC clock > core. The multiplexer isn't any longer part of the pwm block. As far as the documentation is concerned this is not true. There is a input multiplexer with the xtal, vid_pll, fdiv3 and fdiv4 I'm not sure the differences mentionned here actually exists. > > This was tested on a sc2-based system that uses the same pwm block. > > [0] https://github.com/khadas/linux/blob/khadas-vims-5.4.y/drivers/pwm/pwm-meson.c AFAICT, this looks more like a choice in vendor SDK to not use the input mux. IOW, just SW decision. I don't think such change makes sense in mainline if the HW has not actually changed. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > Adding the amlogic,meson-s4-pwm compatible to the documentation was part > of the yaml conversion already. > --- > drivers/pwm/pwm-meson.c | 38 ++++++++++++++++++++++++++++++++++---- > 1 file changed, 34 insertions(+), 4 deletions(-) > > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c > index 16d79ca5d..7a93fdada 100644 > --- a/drivers/pwm/pwm-meson.c > +++ b/drivers/pwm/pwm-meson.c > @@ -98,6 +98,7 @@ struct meson_pwm_channel { > struct meson_pwm_data { > const char * const *parent_names; > unsigned int num_parents; > + unsigned int ext_clk:1; > }; > > struct meson_pwm { > @@ -158,6 +159,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, > struct meson_pwm_channel *channel = &meson->channels[pwm->hwpwm]; > unsigned int duty, period, pre_div, cnt, duty_cnt; > unsigned long fin_freq; > + int err; > > duty = state->duty_cycle; > period = state->period; > @@ -165,6 +167,14 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, > if (state->polarity == PWM_POLARITY_INVERSED) > duty = period - duty; > > + if (meson->data->ext_clk) { > + err = clk_set_rate(channel->clk, 0xffffUL * NSEC_PER_SEC / period); > + if (err) { > + dev_err(meson->chip.dev, "failed to set pwm clock rate\n"); > + return err; > + } > + } > + > fin_freq = clk_get_rate(channel->clk); > if (fin_freq == 0) { > dev_err(meson->chip.dev, "invalid source clock frequency\n"); > @@ -173,10 +183,14 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, > > dev_dbg(meson->chip.dev, "fin_freq: %lu Hz\n", fin_freq); > > - pre_div = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * 0xffffLL); > - if (pre_div > MISC_CLK_DIV_MASK) { > - dev_err(meson->chip.dev, "unable to get period pre_div\n"); > - return -EINVAL; > + if (meson->data->ext_clk) { > + pre_div = 0; > + } else { > + pre_div = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * 0xffffLL); > + if (pre_div > MISC_CLK_DIV_MASK) { > + dev_err(meson->chip.dev, "unable to get period pre_div\n"); > + return -EINVAL; > + } > } > > cnt = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * (pre_div + 1)); > @@ -445,6 +459,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_s4_data = { > + .ext_clk = 1, > +}; > + > static const struct of_device_id meson_pwm_matches[] = { > { > .compatible = "amlogic,meson8b-pwm", > @@ -478,6 +496,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_s4_data > + }, > {}, > }; > MODULE_DEVICE_TABLE(of, meson_pwm_matches); > @@ -493,6 +515,14 @@ static int meson_pwm_init_channels(struct meson_pwm *meson) > for (i = 0; i < meson->chip.npwm; i++) { > struct meson_pwm_channel *channel = &meson->channels[i]; > > + if (meson->data->ext_clk) { > + snprintf(name, sizeof(name), "clkin%u", i); > + channel->clk = devm_clk_get(dev, name); > + if (IS_ERR(channel->clk)) > + return PTR_ERR(channel->clk); > + continue; > + } > + > snprintf(name, sizeof(name), "%s#mux%u", dev_name(dev), i); > > init.name = name;
On 25.03.2023 14:24, Jerome Brunet wrote: > > On Fri 24 Mar 2023 at 23:23, Heiner Kallweit <hkallweit1@gmail.com> wrote: > >> This adds pwm support for (at least) the s4 chip family. The extension >> is based on the vendor driver that can be found at [0]. There the >> version with the new clock handling is called meson-v2-pwm. >> Central change is that the clock is now fully provided by the SoC clock >> core. The multiplexer isn't any longer part of the pwm block. > > As far as the documentation is concerned this is not true. > There is a input multiplexer with the xtal, vid_pll, fdiv3 and fdiv4 > > I'm not sure the differences mentionned here actually exists. > I don't have access to a S905X4 datasheet, just to the one for S905X3. What makes me think that the hw is different: - In the clock drivers for families before s4 I see no hint that dedicated pwm clocks exist. From s4 there are CLKID_PWM_... clocks. - In the S905X3 datasheet I see no hint that the pwm block can be fed from an external clock (except the standard 4 mux parents). >> >> This was tested on a sc2-based system that uses the same pwm block. >> >> [0] https://github.com/khadas/linux/blob/khadas-vims-5.4.y/drivers/pwm/pwm-meson.c > > AFAICT, this looks more like a choice in vendor SDK to not use the input > mux. IOW, just SW decision. > - If it would be a sw decision to use internal mux/div or an external clock, then there should be a way to configure whether to use option a or b. I see no such switch in the vendor driver code. Maybe you can check in the datasheet whether there's such a switch. > I don't think such change makes sense in mainline if the HW has not > actually changed. > >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> Adding the amlogic,meson-s4-pwm compatible to the documentation was part >> of the yaml conversion already. >> --- >> drivers/pwm/pwm-meson.c | 38 ++++++++++++++++++++++++++++++++++---- >> 1 file changed, 34 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c >> index 16d79ca5d..7a93fdada 100644 >> --- a/drivers/pwm/pwm-meson.c >> +++ b/drivers/pwm/pwm-meson.c >> @@ -98,6 +98,7 @@ struct meson_pwm_channel { >> struct meson_pwm_data { >> const char * const *parent_names; >> unsigned int num_parents; >> + unsigned int ext_clk:1; >> }; >> >> struct meson_pwm { >> @@ -158,6 +159,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, >> struct meson_pwm_channel *channel = &meson->channels[pwm->hwpwm]; >> unsigned int duty, period, pre_div, cnt, duty_cnt; >> unsigned long fin_freq; >> + int err; >> >> duty = state->duty_cycle; >> period = state->period; >> @@ -165,6 +167,14 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, >> if (state->polarity == PWM_POLARITY_INVERSED) >> duty = period - duty; >> >> + if (meson->data->ext_clk) { >> + err = clk_set_rate(channel->clk, 0xffffUL * NSEC_PER_SEC / period); >> + if (err) { >> + dev_err(meson->chip.dev, "failed to set pwm clock rate\n"); >> + return err; >> + } >> + } >> + >> fin_freq = clk_get_rate(channel->clk); >> if (fin_freq == 0) { >> dev_err(meson->chip.dev, "invalid source clock frequency\n"); >> @@ -173,10 +183,14 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, >> >> dev_dbg(meson->chip.dev, "fin_freq: %lu Hz\n", fin_freq); >> >> - pre_div = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * 0xffffLL); >> - if (pre_div > MISC_CLK_DIV_MASK) { >> - dev_err(meson->chip.dev, "unable to get period pre_div\n"); >> - return -EINVAL; >> + if (meson->data->ext_clk) { >> + pre_div = 0; >> + } else { >> + pre_div = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * 0xffffLL); >> + if (pre_div > MISC_CLK_DIV_MASK) { >> + dev_err(meson->chip.dev, "unable to get period pre_div\n"); >> + return -EINVAL; >> + } >> } >> >> cnt = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * (pre_div + 1)); >> @@ -445,6 +459,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_s4_data = { >> + .ext_clk = 1, >> +}; >> + >> static const struct of_device_id meson_pwm_matches[] = { >> { >> .compatible = "amlogic,meson8b-pwm", >> @@ -478,6 +496,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_s4_data >> + }, >> {}, >> }; >> MODULE_DEVICE_TABLE(of, meson_pwm_matches); >> @@ -493,6 +515,14 @@ static int meson_pwm_init_channels(struct meson_pwm *meson) >> for (i = 0; i < meson->chip.npwm; i++) { >> struct meson_pwm_channel *channel = &meson->channels[i]; >> >> + if (meson->data->ext_clk) { >> + snprintf(name, sizeof(name), "clkin%u", i); >> + channel->clk = devm_clk_get(dev, name); >> + if (IS_ERR(channel->clk)) >> + return PTR_ERR(channel->clk); >> + continue; >> + } >> + >> snprintf(name, sizeof(name), "%s#mux%u", dev_name(dev), i); >> >> init.name = name; >
On 25.03.2023 23:58, Heiner Kallweit wrote: > On 25.03.2023 14:24, Jerome Brunet wrote: >> >> On Fri 24 Mar 2023 at 23:23, Heiner Kallweit <hkallweit1@gmail.com> wrote: >> >>> This adds pwm support for (at least) the s4 chip family. The extension >>> is based on the vendor driver that can be found at [0]. There the >>> version with the new clock handling is called meson-v2-pwm. >>> Central change is that the clock is now fully provided by the SoC clock >>> core. The multiplexer isn't any longer part of the pwm block. >> >> As far as the documentation is concerned this is not true. >> There is a input multiplexer with the xtal, vid_pll, fdiv3 and fdiv4 >> >> I'm not sure the differences mentionned here actually exists. >> > > I don't have access to a S905X4 datasheet, just to the one for S905X3. > What makes me think that the hw is different: > > - In the clock drivers for families before s4 I see no hint that > dedicated pwm clocks exist. From s4 there are CLKID_PWM_... clocks. > In addition, that's what the relevant commit in the vendor driver says: https://github.com/khadas/linux/commit/4f35972692b52a7f7b96f86b06c01feb7f3b3862 -> sc2 pwm controller moves clk to clktree > - In the S905X3 datasheet I see no hint that the pwm block can be fed > from an external clock (except the standard 4 mux parents). > >>> >>> This was tested on a sc2-based system that uses the same pwm block. >>> >>> [0] https://github.com/khadas/linux/blob/khadas-vims-5.4.y/drivers/pwm/pwm-meson.c >> >> AFAICT, this looks more like a choice in vendor SDK to not use the input >> mux. IOW, just SW decision. >> > > - If it would be a sw decision to use internal mux/div or an external > clock, then there should be a way to configure whether to use option a or b. > I see no such switch in the vendor driver code. Maybe you can check in the > datasheet whether there's such a switch. > >> I don't think such change makes sense in mainline if the HW has not >> actually changed. >> >>> >>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >>> --- >>> Adding the amlogic,meson-s4-pwm compatible to the documentation was part >>> of the yaml conversion already. >>> --- >>> drivers/pwm/pwm-meson.c | 38 ++++++++++++++++++++++++++++++++++---- >>> 1 file changed, 34 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c >>> index 16d79ca5d..7a93fdada 100644 >>> --- a/drivers/pwm/pwm-meson.c >>> +++ b/drivers/pwm/pwm-meson.c >>> @@ -98,6 +98,7 @@ struct meson_pwm_channel { >>> struct meson_pwm_data { >>> const char * const *parent_names; >>> unsigned int num_parents; >>> + unsigned int ext_clk:1; >>> }; >>> >>> struct meson_pwm { >>> @@ -158,6 +159,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, >>> struct meson_pwm_channel *channel = &meson->channels[pwm->hwpwm]; >>> unsigned int duty, period, pre_div, cnt, duty_cnt; >>> unsigned long fin_freq; >>> + int err; >>> >>> duty = state->duty_cycle; >>> period = state->period; >>> @@ -165,6 +167,14 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, >>> if (state->polarity == PWM_POLARITY_INVERSED) >>> duty = period - duty; >>> >>> + if (meson->data->ext_clk) { >>> + err = clk_set_rate(channel->clk, 0xffffUL * NSEC_PER_SEC / period); >>> + if (err) { >>> + dev_err(meson->chip.dev, "failed to set pwm clock rate\n"); >>> + return err; >>> + } >>> + } >>> + >>> fin_freq = clk_get_rate(channel->clk); >>> if (fin_freq == 0) { >>> dev_err(meson->chip.dev, "invalid source clock frequency\n"); >>> @@ -173,10 +183,14 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, >>> >>> dev_dbg(meson->chip.dev, "fin_freq: %lu Hz\n", fin_freq); >>> >>> - pre_div = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * 0xffffLL); >>> - if (pre_div > MISC_CLK_DIV_MASK) { >>> - dev_err(meson->chip.dev, "unable to get period pre_div\n"); >>> - return -EINVAL; >>> + if (meson->data->ext_clk) { >>> + pre_div = 0; >>> + } else { >>> + pre_div = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * 0xffffLL); >>> + if (pre_div > MISC_CLK_DIV_MASK) { >>> + dev_err(meson->chip.dev, "unable to get period pre_div\n"); >>> + return -EINVAL; >>> + } >>> } >>> >>> cnt = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * (pre_div + 1)); >>> @@ -445,6 +459,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_s4_data = { >>> + .ext_clk = 1, >>> +}; >>> + >>> static const struct of_device_id meson_pwm_matches[] = { >>> { >>> .compatible = "amlogic,meson8b-pwm", >>> @@ -478,6 +496,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_s4_data >>> + }, >>> {}, >>> }; >>> MODULE_DEVICE_TABLE(of, meson_pwm_matches); >>> @@ -493,6 +515,14 @@ static int meson_pwm_init_channels(struct meson_pwm *meson) >>> for (i = 0; i < meson->chip.npwm; i++) { >>> struct meson_pwm_channel *channel = &meson->channels[i]; >>> >>> + if (meson->data->ext_clk) { >>> + snprintf(name, sizeof(name), "clkin%u", i); >>> + channel->clk = devm_clk_get(dev, name); >>> + if (IS_ERR(channel->clk)) >>> + return PTR_ERR(channel->clk); >>> + continue; >>> + } >>> + >>> snprintf(name, sizeof(name), "%s#mux%u", dev_name(dev), i); >>> >>> init.name = name; >> >
On 24/03/2023 23:23, Heiner Kallweit wrote: > This adds pwm support for (at least) the s4 chip family. The extension > is based on the vendor driver that can be found at [0]. There the > version with the new clock handling is called meson-v2-pwm. > Central change is that the clock is now fully provided by the SoC clock > core. The multiplexer isn't any longer part of the pwm block. > > This was tested on a sc2-based system that uses the same pwm block. > > [0] https://github.com/khadas/linux/blob/khadas-vims-5.4.y/drivers/pwm/pwm-meson.c > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > Adding the amlogic,meson-s4-pwm compatible to the documentation was part > of the yaml conversion already. > --- > drivers/pwm/pwm-meson.c | 38 ++++++++++++++++++++++++++++++++++---- > 1 file changed, 34 insertions(+), 4 deletions(-) > > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c > index 16d79ca5d..7a93fdada 100644 > --- a/drivers/pwm/pwm-meson.c > +++ b/drivers/pwm/pwm-meson.c > @@ -98,6 +98,7 @@ struct meson_pwm_channel { > struct meson_pwm_data { > const char * const *parent_names; > unsigned int num_parents; > + unsigned int ext_clk:1; Drop the :1, the compiler will align the struct size to the register size, so simply use unsigned int or bool. I'd use a better name for that, like: no_mux_blk, pwm_clk_input.... > }; > > struct meson_pwm { > @@ -158,6 +159,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, > struct meson_pwm_channel *channel = &meson->channels[pwm->hwpwm]; > unsigned int duty, period, pre_div, cnt, duty_cnt; > unsigned long fin_freq; > + int err; > > duty = state->duty_cycle; > period = state->period; > @@ -165,6 +167,14 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, > if (state->polarity == PWM_POLARITY_INVERSED) > duty = period - duty; > > + if (meson->data->ext_clk) { > + err = clk_set_rate(channel->clk, 0xffffUL * NSEC_PER_SEC / period); > + if (err) { > + dev_err(meson->chip.dev, "failed to set pwm clock rate\n"); > + return err; > + } If the same MUX architecture has been moved out of the PWM registers, then why would you call set_rate ? we don't call set_rate today, so why should we ? Today the MUX selection is static depending on the clk names specified in the bindings (yes it's really not the good way to do that,....) but now the MUX is outside the PWM registers block we should stick to the same architecture and mark the MUX as NO_REPARENT and instead of using a bad bindings specify the MUX parent & rate in dt using assigned-clk-parent/rate. So please drop this set_rate(). > + } > + > fin_freq = clk_get_rate(channel->clk); > if (fin_freq == 0) { > dev_err(meson->chip.dev, "invalid source clock frequency\n"); > @@ -173,10 +183,14 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, > > dev_dbg(meson->chip.dev, "fin_freq: %lu Hz\n", fin_freq); > > - pre_div = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * 0xffffLL); > - if (pre_div > MISC_CLK_DIV_MASK) { > - dev_err(meson->chip.dev, "unable to get period pre_div\n"); > - return -EINVAL; > + if (meson->data->ext_clk) { > + pre_div = 0; > + } else { > + pre_div = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * 0xffffLL); > + if (pre_div > MISC_CLK_DIV_MASK) { > + dev_err(meson->chip.dev, "unable to get period pre_div\n"); > + return -EINVAL; > + } > } > > cnt = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * (pre_div + 1)); > @@ -445,6 +459,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_s4_data = { > + .ext_clk = 1, > +}; > + > static const struct of_device_id meson_pwm_matches[] = { > { > .compatible = "amlogic,meson8b-pwm", > @@ -478,6 +496,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", This deserved a bindings update, and we should perhaps start a new bindings by using new clock-names (!= clkin0/1) to differentiate from the old buggy bindings. Neil > + .data = &pwm_s4_data > + }, > {}, > }; > MODULE_DEVICE_TABLE(of, meson_pwm_matches); > @@ -493,6 +515,14 @@ static int meson_pwm_init_channels(struct meson_pwm *meson) > for (i = 0; i < meson->chip.npwm; i++) { > struct meson_pwm_channel *channel = &meson->channels[i]; > > + if (meson->data->ext_clk) { > + snprintf(name, sizeof(name), "clkin%u", i); > + channel->clk = devm_clk_get(dev, name); > + if (IS_ERR(channel->clk)) > + return PTR_ERR(channel->clk); > + continue; > + } > + > snprintf(name, sizeof(name), "%s#mux%u", dev_name(dev), i); > > init.name = name;
On Sat 25 Mar 2023 at 23:58, Heiner Kallweit <hkallweit1@gmail.com> wrote: > On 25.03.2023 14:24, Jerome Brunet wrote: >> >> On Fri 24 Mar 2023 at 23:23, Heiner Kallweit <hkallweit1@gmail.com> wrote: >> >>> This adds pwm support for (at least) the s4 chip family. The extension >>> is based on the vendor driver that can be found at [0]. There the >>> version with the new clock handling is called meson-v2-pwm. >>> Central change is that the clock is now fully provided by the SoC clock >>> core. The multiplexer isn't any longer part of the pwm block. >> >> As far as the documentation is concerned this is not true. >> There is a input multiplexer with the xtal, vid_pll, fdiv3 and fdiv4 >> >> I'm not sure the differences mentionned here actually exists. >> > > I don't have access to a S905X4 datasheet, just to the one for S905X3. > What makes me think that the hw is different: > > - In the clock drivers for families before s4 I see no hint that > dedicated pwm clocks exist. From s4 there are CLKID_PWM_... clocks. That the clock driver concern, not the PWM one. The fact is the whole PWM clock block is still there. It still has 4 inputs, which may be improperly documented AFAICT. As previously stated, the doc says: xtal, vid_pll, fdiv3 and fdiv4 Since Amlogic does not bother to intialize the "SEL" value anymore, you may assume the PWM clock is connected to the input number maching the register field value. (most likely 0 - so replacing the XTAL) Since you have been able to test this - change this SEL field, you'll see what it does and should be able to determine which clock is behind each of the mux inputs. > > - In the S905X3 datasheet I see no hint that the pwm block can be fed > from an external clock (except the standard 4 mux parents). > >>> >>> This was tested on a sc2-based system that uses the same pwm block. >>> >>> [0] https://github.com/khadas/linux/blob/khadas-vims-5.4.y/drivers/pwm/pwm-meson.c >> >> AFAICT, this looks more like a choice in vendor SDK to not use the input >> mux. IOW, just SW decision. >> > > - If it would be a sw decision to use internal mux/div or an external > clock, then there should be a way to configure whether to use option a or b. > I see no such switch in the vendor driver code. Maybe you can check in the > datasheet whether there's such a switch. > >> I don't think such change makes sense in mainline if the HW has not >> actually changed. >> >>> >>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >>> --- >>> Adding the amlogic,meson-s4-pwm compatible to the documentation was part >>> of the yaml conversion already. >>> --- >>> drivers/pwm/pwm-meson.c | 38 ++++++++++++++++++++++++++++++++++---- >>> 1 file changed, 34 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c >>> index 16d79ca5d..7a93fdada 100644 >>> --- a/drivers/pwm/pwm-meson.c >>> +++ b/drivers/pwm/pwm-meson.c >>> @@ -98,6 +98,7 @@ struct meson_pwm_channel { >>> struct meson_pwm_data { >>> const char * const *parent_names; >>> unsigned int num_parents; >>> + unsigned int ext_clk:1; >>> }; >>> >>> struct meson_pwm { >>> @@ -158,6 +159,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, >>> struct meson_pwm_channel *channel = &meson->channels[pwm->hwpwm]; >>> unsigned int duty, period, pre_div, cnt, duty_cnt; >>> unsigned long fin_freq; >>> + int err; >>> >>> duty = state->duty_cycle; >>> period = state->period; >>> @@ -165,6 +167,14 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, >>> if (state->polarity == PWM_POLARITY_INVERSED) >>> duty = period - duty; >>> >>> + if (meson->data->ext_clk) { >>> + err = clk_set_rate(channel->clk, 0xffffUL * NSEC_PER_SEC / period); >>> + if (err) { >>> + dev_err(meson->chip.dev, "failed to set pwm clock rate\n"); >>> + return err; >>> + } >>> + } >>> + >>> fin_freq = clk_get_rate(channel->clk); >>> if (fin_freq == 0) { >>> dev_err(meson->chip.dev, "invalid source clock frequency\n"); >>> @@ -173,10 +183,14 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, >>> >>> dev_dbg(meson->chip.dev, "fin_freq: %lu Hz\n", fin_freq); >>> >>> - pre_div = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * 0xffffLL); >>> - if (pre_div > MISC_CLK_DIV_MASK) { >>> - dev_err(meson->chip.dev, "unable to get period pre_div\n"); >>> - return -EINVAL; >>> + if (meson->data->ext_clk) { >>> + pre_div = 0; >>> + } else { >>> + pre_div = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * 0xffffLL); >>> + if (pre_div > MISC_CLK_DIV_MASK) { >>> + dev_err(meson->chip.dev, "unable to get period pre_div\n"); >>> + return -EINVAL; >>> + } >>> } >>> >>> cnt = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * (pre_div + 1)); >>> @@ -445,6 +459,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_s4_data = { >>> + .ext_clk = 1, >>> +}; >>> + >>> static const struct of_device_id meson_pwm_matches[] = { >>> { >>> .compatible = "amlogic,meson8b-pwm", >>> @@ -478,6 +496,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_s4_data >>> + }, >>> {}, >>> }; >>> MODULE_DEVICE_TABLE(of, meson_pwm_matches); >>> @@ -493,6 +515,14 @@ static int meson_pwm_init_channels(struct meson_pwm *meson) >>> for (i = 0; i < meson->chip.npwm; i++) { >>> struct meson_pwm_channel *channel = &meson->channels[i]; >>> >>> + if (meson->data->ext_clk) { >>> + snprintf(name, sizeof(name), "clkin%u", i); >>> + channel->clk = devm_clk_get(dev, name); >>> + if (IS_ERR(channel->clk)) >>> + return PTR_ERR(channel->clk); >>> + continue; >>> + } >>> + >>> snprintf(name, sizeof(name), "%s#mux%u", dev_name(dev), i); >>> >>> init.name = name; >>
On 27.03.2023 09:33, Neil Armstrong wrote: > On 24/03/2023 23:23, Heiner Kallweit wrote: >> This adds pwm support for (at least) the s4 chip family. The extension >> is based on the vendor driver that can be found at [0]. There the >> version with the new clock handling is called meson-v2-pwm. >> Central change is that the clock is now fully provided by the SoC clock >> core. The multiplexer isn't any longer part of the pwm block. >> >> This was tested on a sc2-based system that uses the same pwm block. >> >> [0] https://github.com/khadas/linux/blob/khadas-vims-5.4.y/drivers/pwm/pwm-meson.c >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> Adding the amlogic,meson-s4-pwm compatible to the documentation was part >> of the yaml conversion already. >> --- >> drivers/pwm/pwm-meson.c | 38 ++++++++++++++++++++++++++++++++++---- >> 1 file changed, 34 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c >> index 16d79ca5d..7a93fdada 100644 >> --- a/drivers/pwm/pwm-meson.c >> +++ b/drivers/pwm/pwm-meson.c >> @@ -98,6 +98,7 @@ struct meson_pwm_channel { >> struct meson_pwm_data { >> const char * const *parent_names; >> unsigned int num_parents; >> + unsigned int ext_clk:1; > > Drop the :1, the compiler will align the struct size to the register size, so simply use unsigned int or bool. > Having one flag only it doesn't really make a difference, right. I chose this single bit because in future there may come another flag, then having them in one bitfield would be advantageous. Also https://www.kernel.org/doc/Documentation/process/coding-style.rst Part "17) Using bool" prefers bits for bool values in structs, even though the mentioned use cases like multiple flags don't apply here. > I'd use a better name for that, like: no_mux_blk, pwm_clk_input.... > OK >> }; >> struct meson_pwm { >> @@ -158,6 +159,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, >> struct meson_pwm_channel *channel = &meson->channels[pwm->hwpwm]; >> unsigned int duty, period, pre_div, cnt, duty_cnt; >> unsigned long fin_freq; >> + int err; >> duty = state->duty_cycle; >> period = state->period; >> @@ -165,6 +167,14 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, >> if (state->polarity == PWM_POLARITY_INVERSED) >> duty = period - duty; >> + if (meson->data->ext_clk) { >> + err = clk_set_rate(channel->clk, 0xffffUL * NSEC_PER_SEC / period); >> + if (err) { >> + dev_err(meson->chip.dev, "failed to set pwm clock rate\n"); >> + return err; >> + } > > If the same MUX architecture has been moved out of the PWM registers, then why would you call > set_rate ? we don't call set_rate today, so why should we ? > > Today the MUX selection is static depending on the clk names specified in the bindings (yes it's > really not the good way to do that,....) but now the MUX is outside the PWM registers block > we should stick to the same architecture and mark the MUX as NO_REPARENT and instead of using > a bad bindings specify the MUX parent & rate in dt using assigned-clk-parent/rate. > > So please drop this set_rate(). > Implicitly there's a set_rate() also today, by setting the pre_div divider value. By the way: Not sure why the divider isn't handled via CCF, like e.g. in meson-gx-mmc. The best (for achieving best precision) input frequency is 0xffff / period. So we should do our best to come as close as possible to this frequency. By using set_rate() CCF will choose the optimal mux input and divider value. Not sure why we should restrict ourselves to one mux parent only. E.g. for a very low duty cycle a higher input frequency than the xtal 24MHz may be preferable. Not only the mux is outside the PWM block now, also the divider (8 bit wide instead of 7 bit as of today). So we need a set_rate() anyway to set the divider value. What I can't say is whether the internal divider still exists (then external and internal divider would be cascaded) or is removed or bypassed. It seems like when adding the external PWM clock feature Amlogic forgot to update the PWM block documentation, as there's no reference at all to the now external input clock (except in the clocks section). >> + } >> + >> fin_freq = clk_get_rate(channel->clk); >> if (fin_freq == 0) { >> dev_err(meson->chip.dev, "invalid source clock frequency\n"); >> @@ -173,10 +183,14 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, >> dev_dbg(meson->chip.dev, "fin_freq: %lu Hz\n", fin_freq); >> - pre_div = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * 0xffffLL); >> - if (pre_div > MISC_CLK_DIV_MASK) { >> - dev_err(meson->chip.dev, "unable to get period pre_div\n"); >> - return -EINVAL; >> + if (meson->data->ext_clk) { >> + pre_div = 0; >> + } else { >> + pre_div = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * 0xffffLL); >> + if (pre_div > MISC_CLK_DIV_MASK) { >> + dev_err(meson->chip.dev, "unable to get period pre_div\n"); >> + return -EINVAL; >> + } >> } >> cnt = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * (pre_div + 1)); >> @@ -445,6 +459,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_s4_data = { >> + .ext_clk = 1, >> +}; >> + >> static const struct of_device_id meson_pwm_matches[] = { >> { >> .compatible = "amlogic,meson8b-pwm", >> @@ -478,6 +496,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", > > This deserved a bindings update, and we should perhaps start a new bindings by using > new clock-names (!= clkin0/1) to differentiate from the old buggy bindings. > We could simply name the clocks pwm_a, pwm_b. If you think this could be confusing for pwm PWM_C/PWM_D, then we may use pwm0 and pwm1. The changed binding would have to reflect that now both input clocks for a pwm are required. > Neil > >> + .data = &pwm_s4_data >> + }, >> {}, >> }; >> MODULE_DEVICE_TABLE(of, meson_pwm_matches); >> @@ -493,6 +515,14 @@ static int meson_pwm_init_channels(struct meson_pwm *meson) >> for (i = 0; i < meson->chip.npwm; i++) { >> struct meson_pwm_channel *channel = &meson->channels[i]; >> + if (meson->data->ext_clk) { >> + snprintf(name, sizeof(name), "clkin%u", i); >> + channel->clk = devm_clk_get(dev, name); >> + if (IS_ERR(channel->clk)) >> + return PTR_ERR(channel->clk); >> + continue; >> + } >> + >> snprintf(name, sizeof(name), "%s#mux%u", dev_name(dev), i); >> init.name = name; >
On Mon, Mar 27, 2023 at 07:00:35PM +0200, Heiner Kallweit wrote: > The best (for achieving best precision) input frequency is 0xffff / period. > So we should do our best to come as close as possible to this frequency. > By using set_rate() CCF will choose the optimal mux input and divider value. Just for the record: Here might be a misconception. There is (AFAIK) no reason to believe that set_rate would pick an optimal configuration. And if it's only because there is no objective definition of "optimal". For example if a driver for an SD card requests 400MHz, it might not want anything faster and so probably prefers 202 MHz over 404 MHz. However a driver for a UART would prefer the 404 MHz in this case. IMHO we'd need something like clk_rounddown_rate(); clk_roundnear_rate(); and maybe clk_roundup_rate() to please all drivers. But that sounds like a bigger quest. If your clk can provide (say) 1000000 Hz and 2000000 Hz (and nothing in between) there is no rule in the CCF that tells you which of the two to pick if you request 1000001 Hz or 1999999 Hz. (The only thing I would not expect is that you get 1000000 Hz when requesting 1999999 Hz *and* 2000000 Hz when requesting 1000001 Hz.) > Not sure why we should restrict ourselves to one mux parent only. > E.g. for a very low duty cycle a higher input frequency than the xtal 24MHz may be preferable. > > Not only the mux is outside the PWM block now, also the divider (8 bit wide instead of 7 bit > as of today). So we need a set_rate() anyway to set the divider value. > What I can't say is whether the internal divider still exists (then external and internal > divider would be cascaded) or is removed or bypassed. > It seems like when adding the external PWM clock feature Amlogic forgot to update > the PWM block documentation, as there's no reference at all to the now external input clock > (except in the clocks section). It would be great to test (or ask?) if the internal divider still exists and do the right thing then. Having said that, I wonder if it would make sense to rework the driver for the variants with the internal mux to also abstract the divider as a proper clk. Best regards Uwe
On 27/03/2023 19:00, Heiner Kallweit wrote: > On 27.03.2023 09:33, Neil Armstrong wrote: >> On 24/03/2023 23:23, Heiner Kallweit wrote: >>> This adds pwm support for (at least) the s4 chip family. The extension >>> is based on the vendor driver that can be found at [0]. There the >>> version with the new clock handling is called meson-v2-pwm. >>> Central change is that the clock is now fully provided by the SoC clock >>> core. The multiplexer isn't any longer part of the pwm block. >>> >>> This was tested on a sc2-based system that uses the same pwm block. >>> >>> [0] https://github.com/khadas/linux/blob/khadas-vims-5.4.y/drivers/pwm/pwm-meson.c >>> >>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >>> --- >>> Adding the amlogic,meson-s4-pwm compatible to the documentation was part >>> of the yaml conversion already. >>> --- >>> drivers/pwm/pwm-meson.c | 38 ++++++++++++++++++++++++++++++++++---- >>> 1 file changed, 34 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c >>> index 16d79ca5d..7a93fdada 100644 >>> --- a/drivers/pwm/pwm-meson.c >>> +++ b/drivers/pwm/pwm-meson.c >>> @@ -98,6 +98,7 @@ struct meson_pwm_channel { >>> struct meson_pwm_data { >>> const char * const *parent_names; >>> unsigned int num_parents; >>> + unsigned int ext_clk:1; >> >> Drop the :1, the compiler will align the struct size to the register size, so simply use unsigned int or bool. >> > Having one flag only it doesn't really make a difference, right. > I chose this single bit because in future there may come another flag, > then having them in one bitfield would be advantageous. > > Also https://www.kernel.org/doc/Documentation/process/coding-style.rst > Part "17) Using bool" prefers bits for bool values in structs, even though > the mentioned use cases like multiple flags don't apply here. > >> I'd use a better name for that, like: no_mux_blk, pwm_clk_input.... >> > OK > >>> }; >>> struct meson_pwm { >>> @@ -158,6 +159,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, >>> struct meson_pwm_channel *channel = &meson->channels[pwm->hwpwm]; >>> unsigned int duty, period, pre_div, cnt, duty_cnt; >>> unsigned long fin_freq; >>> + int err; >>> duty = state->duty_cycle; >>> period = state->period; >>> @@ -165,6 +167,14 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, >>> if (state->polarity == PWM_POLARITY_INVERSED) >>> duty = period - duty; >>> + if (meson->data->ext_clk) { >>> + err = clk_set_rate(channel->clk, 0xffffUL * NSEC_PER_SEC / period); >>> + if (err) { >>> + dev_err(meson->chip.dev, "failed to set pwm clock rate\n"); >>> + return err; >>> + } >> >> If the same MUX architecture has been moved out of the PWM registers, then why would you call >> set_rate ? we don't call set_rate today, so why should we ? >> >> Today the MUX selection is static depending on the clk names specified in the bindings (yes it's >> really not the good way to do that,....) but now the MUX is outside the PWM registers block >> we should stick to the same architecture and mark the MUX as NO_REPARENT and instead of using >> a bad bindings specify the MUX parent & rate in dt using assigned-clk-parent/rate. >> >> So please drop this set_rate(). >> > Implicitly there's a set_rate() also today, by setting the pre_div divider value. > By the way: Not sure why the divider isn't handled via CCF, like e.g. in meson-gx-mmc. Ok now I understand your point, it wasn't clear enough the pre-div was out aswell. This should be clear documented in the code to outline the differences. > > The best (for achieving best precision) input frequency is 0xffff / period. > So we should do our best to come as close as possible to this frequency. > By using set_rate() CCF will choose the optimal mux input and divider value. > Not sure why we should restrict ourselves to one mux parent only. > E.g. for a very low duty cycle a higher input frequency than the xtal 24MHz may be preferable. > > Not only the mux is outside the PWM block now, also the divider (8 bit wide instead of 7 bit > as of today). So we need a set_rate() anyway to set the divider value. > What I can't say is whether the internal divider still exists (then external and internal > divider would be cascaded) or is removed or bypassed. > It seems like when adding the external PWM clock feature Amlogic forgot to update > the PWM block documentation, as there's no reference at all to the now external input clock > (except in the clocks section). > >>> + } >>> + >>> fin_freq = clk_get_rate(channel->clk); >>> if (fin_freq == 0) { >>> dev_err(meson->chip.dev, "invalid source clock frequency\n"); >>> @@ -173,10 +183,14 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, >>> dev_dbg(meson->chip.dev, "fin_freq: %lu Hz\n", fin_freq); >>> - pre_div = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * 0xffffLL); >>> - if (pre_div > MISC_CLK_DIV_MASK) { >>> - dev_err(meson->chip.dev, "unable to get period pre_div\n"); >>> - return -EINVAL; >>> + if (meson->data->ext_clk) { >>> + pre_div = 0; >>> + } else { >>> + pre_div = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * 0xffffLL); >>> + if (pre_div > MISC_CLK_DIV_MASK) { >>> + dev_err(meson->chip.dev, "unable to get period pre_div\n"); >>> + return -EINVAL; >>> + } >>> } >>> cnt = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * (pre_div + 1)); >>> @@ -445,6 +459,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_s4_data = { >>> + .ext_clk = 1, >>> +}; >>> + >>> static const struct of_device_id meson_pwm_matches[] = { >>> { >>> .compatible = "amlogic,meson8b-pwm", >>> @@ -478,6 +496,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", >> >> This deserved a bindings update, and we should perhaps start a new bindings by using >> new clock-names (!= clkin0/1) to differentiate from the old buggy bindings. >> > We could simply name the clocks pwm_a, pwm_b. If you think this could be confusing > for pwm PWM_C/PWM_D, then we may use pwm0 and pwm1. > > The changed binding would have to reflect that now both input clocks for a pwm > are required. > >> Neil >> >>> + .data = &pwm_s4_data >>> + }, >>> {}, >>> }; >>> MODULE_DEVICE_TABLE(of, meson_pwm_matches); >>> @@ -493,6 +515,14 @@ static int meson_pwm_init_channels(struct meson_pwm *meson) >>> for (i = 0; i < meson->chip.npwm; i++) { >>> struct meson_pwm_channel *channel = &meson->channels[i]; >>> + if (meson->data->ext_clk) { >>> + snprintf(name, sizeof(name), "clkin%u", i); >>> + channel->clk = devm_clk_get(dev, name); >>> + if (IS_ERR(channel->clk)) >>> + return PTR_ERR(channel->clk); >>> + continue; >>> + } >>> + >>> snprintf(name, sizeof(name), "%s#mux%u", dev_name(dev), i); >>> init.name = name; >> >
On 27.03.2023 19:50, Uwe Kleine-König wrote: > On Mon, Mar 27, 2023 at 07:00:35PM +0200, Heiner Kallweit wrote: >> The best (for achieving best precision) input frequency is 0xffff / period. >> So we should do our best to come as close as possible to this frequency. >> By using set_rate() CCF will choose the optimal mux input and divider value. > > Just for the record: Here might be a misconception. There is (AFAIK) no > reason to believe that set_rate would pick an optimal configuration. And > if it's only because there is no objective definition of "optimal". For > example if a driver for an SD card requests 400MHz, it might not want > anything faster and so probably prefers 202 MHz over 404 MHz. However a > driver for a UART would prefer the 404 MHz in this case. IMHO we'd need > something like > Right, I wasn't precise enough. In my case optimal is "fastest rate <= requested rate". This seems to be the behavior of clk core default implementations like clk_mux_determine_rate_flags() an divider_determine_rate(), used by the meson clock drivers. However in general clock providers can provide their own determine_rate() implementations. And, as a disclaimer, I have to admit that I'm no expert in CCF. > clk_rounddown_rate(); > clk_roundnear_rate(); and maybe > clk_roundup_rate() > > to please all drivers. But that sounds like a bigger quest. > > If your clk can provide (say) 1000000 Hz and 2000000 Hz (and nothing in > between) there is no rule in the CCF that tells you which of the two to > pick if you request 1000001 Hz or 1999999 Hz. (The only thing I would > not expect is that you get 1000000 Hz when requesting 1999999 Hz *and* > 2000000 Hz when requesting 1000001 Hz.) > >> Not sure why we should restrict ourselves to one mux parent only. >> E.g. for a very low duty cycle a higher input frequency than the xtal 24MHz may be preferable. >> >> Not only the mux is outside the PWM block now, also the divider (8 bit wide instead of 7 bit >> as of today). So we need a set_rate() anyway to set the divider value. >> What I can't say is whether the internal divider still exists (then external and internal >> divider would be cascaded) or is removed or bypassed. >> It seems like when adding the external PWM clock feature Amlogic forgot to update >> the PWM block documentation, as there's no reference at all to the now external input clock >> (except in the clocks section). > > It would be great to test (or ask?) if the internal divider still exists > and do the right thing then. > > Having said that, I wonder if it would make sense to rework the driver > for the variants with the internal mux to also abstract the divider as a > proper clk. > I think this would make sense. Then we could ignore the mux parent set in DT and let CCF select the mux parent and divider value. And we could use one pwm calc logic and get rid of most if(ext_clk). > Best regards > Uwe >
diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c index 16d79ca5d..7a93fdada 100644 --- a/drivers/pwm/pwm-meson.c +++ b/drivers/pwm/pwm-meson.c @@ -98,6 +98,7 @@ struct meson_pwm_channel { struct meson_pwm_data { const char * const *parent_names; unsigned int num_parents; + unsigned int ext_clk:1; }; struct meson_pwm { @@ -158,6 +159,7 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, struct meson_pwm_channel *channel = &meson->channels[pwm->hwpwm]; unsigned int duty, period, pre_div, cnt, duty_cnt; unsigned long fin_freq; + int err; duty = state->duty_cycle; period = state->period; @@ -165,6 +167,14 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, if (state->polarity == PWM_POLARITY_INVERSED) duty = period - duty; + if (meson->data->ext_clk) { + err = clk_set_rate(channel->clk, 0xffffUL * NSEC_PER_SEC / period); + if (err) { + dev_err(meson->chip.dev, "failed to set pwm clock rate\n"); + return err; + } + } + fin_freq = clk_get_rate(channel->clk); if (fin_freq == 0) { dev_err(meson->chip.dev, "invalid source clock frequency\n"); @@ -173,10 +183,14 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, dev_dbg(meson->chip.dev, "fin_freq: %lu Hz\n", fin_freq); - pre_div = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * 0xffffLL); - if (pre_div > MISC_CLK_DIV_MASK) { - dev_err(meson->chip.dev, "unable to get period pre_div\n"); - return -EINVAL; + if (meson->data->ext_clk) { + pre_div = 0; + } else { + pre_div = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * 0xffffLL); + if (pre_div > MISC_CLK_DIV_MASK) { + dev_err(meson->chip.dev, "unable to get period pre_div\n"); + return -EINVAL; + } } cnt = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * (pre_div + 1)); @@ -445,6 +459,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_s4_data = { + .ext_clk = 1, +}; + static const struct of_device_id meson_pwm_matches[] = { { .compatible = "amlogic,meson8b-pwm", @@ -478,6 +496,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_s4_data + }, {}, }; MODULE_DEVICE_TABLE(of, meson_pwm_matches); @@ -493,6 +515,14 @@ static int meson_pwm_init_channels(struct meson_pwm *meson) for (i = 0; i < meson->chip.npwm; i++) { struct meson_pwm_channel *channel = &meson->channels[i]; + if (meson->data->ext_clk) { + snprintf(name, sizeof(name), "clkin%u", i); + channel->clk = devm_clk_get(dev, name); + if (IS_ERR(channel->clk)) + return PTR_ERR(channel->clk); + continue; + } + snprintf(name, sizeof(name), "%s#mux%u", dev_name(dev), i); init.name = name;
This adds pwm support for (at least) the s4 chip family. The extension is based on the vendor driver that can be found at [0]. There the version with the new clock handling is called meson-v2-pwm. Central change is that the clock is now fully provided by the SoC clock core. The multiplexer isn't any longer part of the pwm block. This was tested on a sc2-based system that uses the same pwm block. [0] https://github.com/khadas/linux/blob/khadas-vims-5.4.y/drivers/pwm/pwm-meson.c Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- Adding the amlogic,meson-s4-pwm compatible to the documentation was part of the yaml conversion already. --- drivers/pwm/pwm-meson.c | 38 ++++++++++++++++++++++++++++++++++---- 1 file changed, 34 insertions(+), 4 deletions(-)