| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<TY3PR01MB1134600B9DF5AA79AE121CD148609A@TY3PR01MB11346.jpnprd01.prod.outlook.com>
Date: Thu, 11 Sep 2025 09:46:38 +0000
From: Biju Das <biju.das.jz@...renesas.com>
To: Uwe Kleine-König <ukleinek@...nel.org>, biju.das.au
<biju.das.au@...il.com>
CC: "linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Geert
Uytterhoeven <geert+renesas@...der.be>, Prabhakar Mahadev Lad
<prabhakar.mahadev-lad.rj@...renesas.com>,
"linux-renesas-soc@...r.kernel.org" <linux-renesas-soc@...r.kernel.org>
Subject: RE: [PATCH v2] pwm: rzg2l-gpt: Implementation of the waveform
callbacks
Hi Uwe Kleine-König,
Thanks for the feedback.
> -----Original Message-----
> From: Uwe Kleine-König <ukleinek@...nel.org>
> Sent: 09 September 2025 16:19
> Subject: Re: [PATCH v2] pwm: rzg2l-gpt: Implementation of the waveform callbacks
>
> Hello Biju,
>
> On Thu, Aug 14, 2025 at 12:50:20PM +0100, Biju wrote:
> > From: Biju Das <biju.das.jz@...renesas.com>
> >
> > Convert the rzg2l-gpt driver to use the new callbacks for hardware
> > programming.
> >
> > Signed-off-by: Biju Das <biju.das.jz@...renesas.com>
> > ---
> > v1->v2:
> > * Dropped modifing hardware from .round_waveform_tohw() callback.
> > ---
> > drivers/pwm/pwm-rzg2l-gpt.c | 175
> > +++++++++++++++++++++---------------
> > 1 file changed, 102 insertions(+), 73 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-rzg2l-gpt.c b/drivers/pwm/pwm-rzg2l-gpt.c
> > index 360c8bf3b190..f0a8531457ca 100644
> > --- a/drivers/pwm/pwm-rzg2l-gpt.c
> > +++ b/drivers/pwm/pwm-rzg2l-gpt.c
> > @@ -86,6 +86,13 @@ struct rzg2l_gpt_chip {
> > u32 channel_enable_count[RZG2L_MAX_HW_CHANNELS];
> > };
> >
> > +/* This represents a hardware configuration for one channel */ struct
> > +rzg2l_gpt_waveform {
> > + u32 gtpr;
> > + u32 gtccr;
> > + u8 prescale;
> > +};
> > +
> > static inline struct rzg2l_gpt_chip *to_rzg2l_gpt_chip(struct
> > pwm_chip *chip) {
> > return pwmchip_get_drvdata(chip);
> > @@ -190,8 +197,10 @@ static void rzg2l_gpt_disable(struct rzg2l_gpt_chip *rzg2l_gpt,
> > /* Stop count, Output low on GTIOCx pin when counting stops */
> > rzg2l_gpt->channel_enable_count[ch]--;
> >
> > - if (!rzg2l_gpt->channel_enable_count[ch])
> > + if (!rzg2l_gpt->channel_enable_count[ch]) {
> > rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR(ch), RZG2L_GTCR_CST, 0);
> > + rzg2l_gpt->period_ticks[ch] = 0;
> > + }
>
> I wonder if this is a fix that is orthogonal to the waveform conversion.
> Shouldn't that be a separate commit?
OK, I will send this fix as a separate commit.
>
> > /* Disable pin output */
> > rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTIOR(ch),
> > RZG2L_GTIOR_OxE(sub_ch), 0); @@ -215,54 +224,37 @@ static u64
> rzg2l_gpt_calculate_period_or_duty(struct rzg2l_gpt_chip *rzg2l_gpt,
> > return DIV64_U64_ROUND_UP(tmp, rzg2l_gpt->rate_khz); }
> >
> > -static int rzg2l_gpt_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > - struct pwm_state *state)
> > -{
> > - struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> > -
> > - state->enabled = rzg2l_gpt_is_ch_enabled(rzg2l_gpt, pwm->hwpwm);
> > - if (state->enabled) {
> > - u32 sub_ch = rzg2l_gpt_subchannel(pwm->hwpwm);
> > - u32 ch = RZG2L_GET_CH(pwm->hwpwm);
> > - u8 prescale;
> > - u32 val;
> > -
> > - val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR(ch));
> > - prescale = FIELD_GET(RZG2L_GTCR_TPCS, val);
> > -
> > - val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTPR(ch));
> > - state->period = rzg2l_gpt_calculate_period_or_duty(rzg2l_gpt, val, prescale);
> > -
> > - val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCCR(ch, sub_ch));
> > - state->duty_cycle = rzg2l_gpt_calculate_period_or_duty(rzg2l_gpt, val, prescale);
> > - if (state->duty_cycle > state->period)
> > - state->duty_cycle = state->period;
> > - }
> > -
> > - state->polarity = PWM_POLARITY_NORMAL;
> > -
> > - return 0;
> > -}
> > -
> > static u32 rzg2l_gpt_calculate_pv_or_dc(u64 period_or_duty_cycle, u8
> > prescale) {
> > return min_t(u64, DIV_ROUND_DOWN_ULL(period_or_duty_cycle, 1 << (2 * prescale)),
> > U32_MAX);
> > }
> >
> > -/* Caller holds the lock while calling rzg2l_gpt_config() */ -static
> > int rzg2l_gpt_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > - const struct pwm_state *state)
> > +static int rzg2l_gpt_round_waveform_tohw(struct pwm_chip *chip,
> > + struct pwm_device *pwm,
> > + const struct pwm_waveform *wf,
> > + void *_wfhw)
> > +
> > {
> > struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> > - u8 sub_ch = rzg2l_gpt_subchannel(pwm->hwpwm);
> > + struct rzg2l_gpt_waveform *wfhw = _wfhw;
> > u8 ch = RZG2L_GET_CH(pwm->hwpwm);
> > u64 period_ticks, duty_ticks;
> > unsigned long pv, dc;
> > - u8 prescale;
> > +
> > + guard(mutex)(&rzg2l_gpt->lock);
> > + if (wf->period_length_ns == 0) {
> > + *wfhw = (struct rzg2l_gpt_waveform){
> > + .gtpr = 0,
> > + .gtccr = 0,
> > + .prescale = 0,
> > + };
> > +
> > + return 0;
> > + }
> >
> > /* Limit period/duty cycle to max value supported by the HW */
> > - period_ticks = mul_u64_u64_div_u64(state->period, rzg2l_gpt->rate_khz, USEC_PER_SEC);
> > + period_ticks = mul_u64_u64_div_u64(wf->period_length_ns,
> > +rzg2l_gpt->rate_khz, USEC_PER_SEC);
> > if (period_ticks > RZG2L_MAX_TICKS)
> > period_ticks = RZG2L_MAX_TICKS;
> > /*
>
> The code that follows here needs adaption. Other than .apply(),
> .round_waveform_tohw() is supposed to not fail if the requested period is too small but use the
> smallest possible value then (and return 1).
You mean something like below
if (rzg2l_gpt->channel_request_count[ch] > 1) {
if (period_ticks < rzg2l_gpt->period_ticks[ch])
- return -EBUSY;
+ is_small_second_period = true;
else
period_ticks = rzg2l_gpt->period_ticks[ch];
}
@@ -272,6 +276,9 @@ static int rzg2l_gpt_round_waveform_tohw(struct pwm_chip *chip,
wfhw->prescale = rzg2l_gpt_calculate_prescale(rzg2l_gpt, period_ticks);
pv = rzg2l_gpt_calculate_pv_or_dc(period_ticks, wfhw->prescale);
wfhw->gtpr = pv;
+ if (is_small_second_period)
+ return 1;
>
> > @@ -277,13 +269,14 @@ static int rzg2l_gpt_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > period_ticks = rzg2l_gpt->period_ticks[ch];
> > }
> >
> > - prescale = rzg2l_gpt_calculate_prescale(rzg2l_gpt, period_ticks);
> > - pv = rzg2l_gpt_calculate_pv_or_dc(period_ticks, prescale);
> > -
> > - duty_ticks = mul_u64_u64_div_u64(state->duty_cycle, rzg2l_gpt->rate_khz, USEC_PER_SEC);
> > + wfhw->prescale = rzg2l_gpt_calculate_prescale(rzg2l_gpt, period_ticks);
> > + pv = rzg2l_gpt_calculate_pv_or_dc(period_ticks, wfhw->prescale);
> > + wfhw->gtpr = pv;
> > + duty_ticks = mul_u64_u64_div_u64(wf->duty_length_ns,
> > +rzg2l_gpt->rate_khz, USEC_PER_SEC);
> > if (duty_ticks > period_ticks)
> > duty_ticks = period_ticks;
> > - dc = rzg2l_gpt_calculate_pv_or_dc(duty_ticks, prescale);
> > + dc = rzg2l_gpt_calculate_pv_or_dc(duty_ticks, wfhw->prescale);
> > + wfhw->gtccr = dc;
> >
> > /*
> > * GPT counter is shared by multiple channels, we cache the period
> > ticks @@ -292,6 +285,58 @@ static int rzg2l_gpt_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > */
> > rzg2l_gpt->period_ticks[ch] = period_ticks;
> >
> > + return 0;
> > +}
> > +
> > +static int rzg2l_gpt_round_waveform_fromhw(struct pwm_chip *chip,
> > + struct pwm_device *pwm,
> > + const void *_wfhw,
> > + struct pwm_waveform *wf)
> > +{
> > + struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> > + const struct rzg2l_gpt_waveform *wfhw = _wfhw;
> > +
> > + wf->period_length_ns = rzg2l_gpt_calculate_period_or_duty(rzg2l_gpt, wfhw->gtpr,
> > + wfhw->prescale);
> > + wf->duty_length_ns = rzg2l_gpt_calculate_period_or_duty(rzg2l_gpt, wfhw->gtccr,
> > + wfhw->prescale);
> > + wf->duty_offset_ns = 0;
> > +
> > + return 0;
> > +}
> > +
> > +static int rzg2l_gpt_read_waveform(struct pwm_chip *chip,
> > + struct pwm_device *pwm,
> > + void *_wfhw)
> > +{
> > + struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> > + struct rzg2l_gpt_waveform *wfhw = _wfhw;
> > + u32 sub_ch = rzg2l_gpt_subchannel(pwm->hwpwm);
> > + u32 ch = RZG2L_GET_CH(pwm->hwpwm);
> > + u32 gtcr;
> > +
>
> Why does rzg2l_gpt_write_waveform() grab the lock but
> rzg2l_gpt_read_waveform() doesn't?
OK will add lock here as well.
>
> > + if (rzg2l_gpt_is_ch_enabled(rzg2l_gpt, pwm->hwpwm)) {
> > + gtcr = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR(ch));
> > + wfhw->prescale = FIELD_GET(RZG2L_GTCR_TPCS, gtcr);
> > + wfhw->gtpr = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTPR(ch));
> > + wfhw->gtccr = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCCR(ch, sub_ch));
> > + if (wfhw->gtccr > wfhw->gtpr)
> > + wfhw->gtccr = wfhw->gtpr;
> > + }
>
> If rzg2l_gpt_is_ch_enabled() returns false wfhw must still be initialized, I guess using:
>
> *wfhw = (struct rzg2l_gpt_waveform) { };
OK.
>
> Minor optimization: rzg2l_gpt_is_ch_enabled() reads RZG2L_GTCR(ch) already, would be nice to only do it
> once.
OK will update function something like below
-static bool rzg2l_gpt_is_ch_enabled(struct rzg2l_gpt_chip *rzg2l_gpt, u8 hwpwm)
+static bool rzg2l_gpt_is_ch_enabled(struct rzg2l_gpt_chip *rzg2l_gpt, u8 hwpwm, u32 *gtcr)
{
u8 ch = RZG2L_GET_CH(hwpwm);
u32 val;
@@ -164,6 +164,9 @@ static bool rzg2l_gpt_is_ch_enabled(struct rzg2l_gpt_chip *rzg2l_gpt, u8 hwpwm)
if (!(val & RZG2L_GTCR_CST))
return false;
+ if (gtcr)
+ *gtcr = val;
>
> > + return 0;
> > +}
> > +
> > +static int rzg2l_gpt_write_waveform(struct pwm_chip *chip,
> > + struct pwm_device *pwm,
> > + const void *_wfhw)
> > +{
> > + struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> > + const struct rzg2l_gpt_waveform *wfhw = _wfhw;
> > + u8 sub_ch = rzg2l_gpt_subchannel(pwm->hwpwm);
> > + u8 ch = RZG2L_GET_CH(pwm->hwpwm);
> > +
> > + guard(mutex)(&rzg2l_gpt->lock);
> > /*
> > * Counter must be stopped before modifying mode, prescaler, timer
> > * counter and buffer enable registers. These registers are shared
> > @@ -310,14 +355,14 @@ static int rzg2l_gpt_config(struct pwm_chip
> > *chip, struct pwm_device *pwm,
> >
> > /* Select count clock */
> > rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR(ch), RZG2L_GTCR_TPCS,
> > - FIELD_PREP(RZG2L_GTCR_TPCS, prescale));
> > + FIELD_PREP(RZG2L_GTCR_TPCS, wfhw->prescale));
> >
> > /* Set period */
> > - rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTPR(ch), pv);
> > + rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTPR(ch), wfhw->gtpr);
> > }
> >
> > /* Set duty cycle */
> > - rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTCCR(ch, sub_ch), dc);
> > + rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTCCR(ch, sub_ch), wfhw->gtccr);
> >
> > if (rzg2l_gpt->channel_enable_count[ch] <= 1) {
> > /* Set initial value for counter */ @@ -326,44 +371,28 @@ static
> > int rzg2l_gpt_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > /* Set no buffer operation */
> > rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTBER(ch), 0);
> >
> > - /* Restart the counter after updating the registers */
> > - rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR(ch),
> > - RZG2L_GTCR_CST, RZG2L_GTCR_CST);
> > - }
> > -
> > - return 0;
> > -}
>
> .write_waveform() must make sure to not disturb the other channel. So this is the place where you need
> to check if the other channel relies on the current value of global period (and fail with an error if
> yes and the period value to write is different).
OK, will return error like below, for the second channel
+ } else {
+ if (wfhw->gtpr) {
+ gptr = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTPR(ch));
+ if (wfhw->gtpr < gptr)
+ return -EBUSY;
+ }
}
Cheers,
Biju
Powered by blists - more mailing lists