Hi Uwe,
Thanks for the feedback.
> Subject: Re: [PATCH v5 2/2] pwm: Add support for RZ/G2L GPT
>
> Hello,
>
> On Fri, Aug 05, 2022 at 03:57:04PM +0100, Biju Das wrote:
> > +#define RZG2L_GTIOR_GTIOA_OUT_HI_END_TOGGLE_CMP_MATCH \
> > + (RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE | RZG2L_GTIOR_OAE) #define
> > +RZG2L_GTIOR_GTIOA_OUT_LO_END_TOGGLE_CMP_MATCH \
> > + (RZG2L_INIT_OUT_LO_OUT_LO_END_TOGGLE | RZG2L_GTIOR_OAE) #define
> > +RZG2L_GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH \
> > + ((RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE << 16) | RZG2L_GTIOR_OBE)
>
> FIELD_PREP(RZG2L_GTIOR_GTIOB, RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE)
We cannot use this macro for structure initialization,
I get below compilation errors for variable .value in struct rzg2l_gpt_phase.
Not sure, Maybe we should use if statement with the "FIELD_PREP" macro and
drop the variable "value" from struct rzg2l_gpt_phase??
In file included from drivers/pwm/pwm-rzg2l-gpt.c:17:
./include/linux/bitfield.h:113:2: error: braced-group within expression allowed only inside a function
113 | ({ \
| ^
drivers/pwm/pwm-rzg2l-gpt.c:60:2: note: in expansion of macro 'FIELD_PREP'
60 | FIELD_PREP(RZG2L_GTIOR_GTIOB, RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE)
| ^~~~~~~~~~
drivers/pwm/pwm-rzg2l-gpt.c:81:12: note: in expansion of macro 'RZG2L_GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH'
81 | .value = RZG2L_GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> > +#define RZG2L_GTIOR_GTIOB_OUT_LO_END_TOGGLE_CMP_MATCH \
> > + ((RZG2L_INIT_OUT_LO_OUT_LO_END_TOGGLE << 16) | RZG2L_GTIOR_OBE)
> > +
> > [...]
> > +static u8 rzg2l_calculate_prescale(struct rzg2l_gpt_chip *rzg2l_gpt,
> > + u64 period_cycles)
> > +{
> > + u32 prescaled_period_cycles;
> > + u8 prescale;
> > +
> > + prescaled_period_cycles = period_cycles >> 32;
> > +
> > + if (prescaled_period_cycles >= 256)
> > + prescale = 5;
> > + else
> > + prescale = (roundup_pow_of_two(prescaled_period_cycles + 1)
> + 1) /
> > +2;
>
> I double checked, this looks correct to me.
>
> > +
> > + return prescale;
> > +}
> > +
> > [...]
> > +static int rzg2l_gpt_config(struct pwm_chip *chip, struct pwm_device
> *pwm,
> > + const struct pwm_state *state) {
> > + struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> > + struct rzg2l_gpt_pwm_device *gpt = &rzg2l_gpt->gpt[pwm->hwpwm];
> > + unsigned long pv, dc;
> > + u64 period_cycles;
> > + u64 duty_cycles;
> > + u8 prescale;
> > +
> > + /*
> > + * Refuse clk rates > 1 GHz to prevent overflowing the following
> > + * calculation.
> > + */
> > + if (rzg2l_gpt->rate > NSEC_PER_SEC)
> > + return -EINVAL;
> > +
> > + /*
> > + * GPT counter is shared by multiple channels, so prescale and
> period
> > + * can NOT be modified when there are multiple channels in use
> with
> > + * different settings.
> > + */
> > + if (state->period != rzg2l_gpt->real_period && rzg2l_gpt-
> >user_count > 1)
> > + return -EBUSY;
>
> Optional improvement here: If a period of (say) 100000 ns is requested
> the hardware might likely actually implement 99875 ns. As rzg2l_gpt-
> >real_period corresponds to the requested period (is that a
> misnomer?)
Yes, it is misnomer here. Actually, it is rzg2l_gpt->state_period,
Where we cache the first period value for both the channels with usage count
incremented.
Here 100000 ns means, we are caching 100000 ns.
>you could accept state->period = 99900.
>
> Accepting state->period >= rzg2l_gpt->real_period is fine.
>
> > +
> > + period_cycles = mul_u64_u32_div(state->period, rzg2l_gpt->rate,
> NSEC_PER_SEC);
> > + prescale = rzg2l_calculate_prescale(rzg2l_gpt, period_cycles);
> > +
> > + pv = period_cycles >> (2 * prescale);
>
> If period_cycles is >= (1024 << 32), we get prescale = 5 and so
> period_cycles >> (2 * prescale) doesn't fit into 32 bits. This needs
> handling.
OK, will do the below change.
+ if (period_cycles >= (1024ULL << 32))
+ period_cycles = 1024ULL << 32;
>
> > + duty_cycles = mul_u64_u32_div(state->duty_cycle, rzg2l_gpt->rate,
> NSEC_PER_SEC);
> > + dc = duty_cycles >> (2 * prescale);
> > +
> > + /* Counter must be stopped before modifying Mode and Prescaler */
> > + if (rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR) & RZG2L_GTCR_CST)
> > + rzg2l_gpt_disable(rzg2l_gpt);
>
> Does this affect the other channel? If yes, that's a bad thing and it
> might be worth to improve here.
Yes, it affects the other channels, please share any suggestions
for improvement??
>
> > + /* GPT set operating mode (saw-wave up-counting) */
> > + rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR,
> > + RZG2L_GTCR_MD, RZG2L_GTCR_MD_SAW_WAVE_PWM_MODE);
> > +
> > + /* Set count direction */
> > + rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTUDDTYC, RZG2L_UP_COUNTING);
> > +
> > + rzg2l_gpt->real_period = state->period;
> > + /* Select count clock */
> > + rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR, RZG2L_GTCR_TPCS,
> > + FIELD_PREP(RZG2L_GTCR_TPCS, prescale));
> > +
> > + /* Set period */
> > + rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTPR, pv);
> > +
> > + /* Set duty cycle */
> > + rzg2l_gpt_write(rzg2l_gpt, gpt->ph->duty_reg_offset, dc);
> > +
> > + /* Set initial value for counter */
> > + rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTCNT, 0);
> > +
> > + /* Set no buffer operation */
> > + rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTBER, 0);
> > +
> > + /* Enable pin output */
> > + rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTIOR, gpt->ph->mask,
> > +gpt->ph->value);
> > +
> > + return 0;
> > +}
> > +
> > +static void 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);
> > + struct rzg2l_gpt_pwm_device *gpt = &rzg2l_gpt->gpt[pwm->hwpwm];
> > + u8 prescale;
> > + u64 tmp;
> > + u32 val;
> > +
> > + /* get period */
> > + state->period = rzg2l_gpt->real_period;
> > +
> > + pm_runtime_get_sync(chip->dev);
> > + val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR);
> > + state->enabled = val & RZG2L_GTCR_CST;
> > + if (state->enabled) {
> > + prescale = FIELD_GET(RZG2L_GTCR_TPCS, val);
> > +
> > + val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTPR);
> > + tmp = NSEC_PER_SEC * val << (2 * prescale);
> > + state->period = DIV_ROUND_UP_ULL(tmp, rzg2l_gpt->rate);
> > +
> > + val = rzg2l_gpt_read(rzg2l_gpt, gpt->ph->duty_reg_offset);
>
> I still wonder if this is really better/more effective/easier to
> understand than just:
>
> /* These are actually called GTCCRA and GTCCRB */ #define RZG2L_GTCCR(i)
> (0x4c + 4 * (i))
>
> plus
>
> val = rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCCR(pwm->hwpwm));
Agreed.
>
>
> > + tmp = NSEC_PER_SEC * val << (2 * prescale);
> > + state->duty_cycle = DIV_ROUND_UP_ULL(tmp, rzg2l_gpt->rate);
> > + /*
> > + * Ordering is important, when we set a period for the
> second
> > + * channel, as pwm_request_from_chip() calling get_state()
> will
> > + * have an invalid duty cycle value as the register is not
> > + * initialized yet. So set duty_cycle to zero.
>
> I don't understand that issue. Can you just drop the check "rzg2l_gpt-
> >user_count > 1"?
No, I get a high duty cycle register value (Reset value) for the second channel
as the Register is not initialized yet. I have enabled some debug for better understanding.
Please let me know still it s clear.
pr_info("## %s ch=%d enabled=%d, duty_cycle_reg=%x",__func__,pwm->hwpwm,state->enabled,rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCCR(pwm->hwpwm)));
Please see the test code[1] and corresponding log[2].
The problematic condition is here.
[ 36.751009] ## rzg2l_gpt_get_state ch=1 enabled=1, duty_cycle_reg=ffffffff
As you can see with pwm_debug enabled, the state->enable is 1, even though
duty cycle register is not initialized.
[1] Please see the test code with PWM_DEBUG enabled:
echo 0 > /sys/class/pwm/pwmchip0/export
echo 500000000 > /sys/class/pwm/pwmchip0/pwm0/period
echo 250000000 > /sys/class/pwm/pwmchip0/pwm0/duty_cycle
echo 1 > /sys/class/pwm/pwmchip0/pwm0/enable
echo 1 > /sys/class/pwm/pwmchip0/export
echo 150000000 > /sys/class/pwm/pwmchip0/pwm1/duty_cycle
echo 500000000 > /sys/class/pwm/pwmchip0/pwm1/period
echo 1 > /sys/class/pwm/pwmchip0/pwm1/enable
echo 0 > /sys/class/pwm/pwmchip0/pwm0/enable
echo 0 > /sys/class/pwm/pwmchip0/pwm1/enable
echo 1 > /sys/class/pwm/pwmchip0/unexport
echo 0 > /sys/class/pwm/pwmchip0/unexport
[2]logs:-
root@smarc-rzg2l:~# /poeg_dual.sh
[ 36.688776] ## rzg2l_gpt_get_state ch=0 enabled=0, duty_cycle_reg=ffffffff
[ 36.698051] ## rzg2l_gpt_get_state ch=0 enabled=0, duty_cycle_reg=ffffffff
[ 36.705625] ## rzg2l_gpt_get_state ch=0 enabled=0, duty_cycle_reg=ffffffff
[ 36.718096] ## rzg2l_gpt_get_state ch=0 enabled=0, duty_cycle_reg=ffffffff
[ 36.725717] ## rzg2l_gpt_get_state ch=0 enabled=0, duty_cycle_reg=ffffffff
[ 36.735656] ## rzg2l_gpt_get_state ch=0 enabled=1, duty_cycle_reg=5f5e10
[ 36.743156] ## rzg2l_gpt_get_state ch=0 enabled=1, duty_cycle_reg=5f5e10
[ 36.751009] ## rzg2l_gpt_get_state ch=1 enabled=1, duty_cycle_reg=ffffffff
[ 36.760968] ## rzg2l_gpt_get_state ch=1 enabled=1, duty_cycle_reg=393870
[ 36.768136] ## rzg2l_gpt_get_state ch=1 enabled=1, duty_cycle_reg=393870
[ 39.834765] ## rzg2l_gpt_get_state ch=0 enabled=0, duty_cycle_reg=5f5e10
[ 39.841729] ## rzg2l_gpt_get_state ch=0 enabled=0, duty_cycle_reg=5f5e10
[ 42.886686] ## rzg2l_gpt_get_state ch=1 enabled=0, duty_cycle_reg=393870
>
> If you configure channel #0 while channel #1 is still untouched (in
> software), does this modify the output of channel #1?
>
> > + */
> > + if (state->duty_cycle > state->period &&
> > + rzg2l_gpt->user_count > 1)
> > + state->duty_cycle = 0;
>
> Does this setting (i.e. GTCCR{A,B} > GTPR) correspond to a 100% relative
> duty cycle?
No. I am setting to a value of 0, Since register is not initialized as mentioned above.
>
> > + }
> > +
> > + state->polarity = PWM_POLARITY_NORMAL;
> > + pm_runtime_put(chip->dev);
> > +}
> > +
> > +static int rzg2l_gpt_apply(struct pwm_chip *chip, struct pwm_device
> *pwm,
> > + const struct pwm_state *state)
> > +{
> > + struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip);
> > + int ret;
> > +
> > + if (state->polarity != PWM_POLARITY_NORMAL)
> > + return -EINVAL;
> > +
> > + pm_runtime_get_sync(chip->dev);
> > + if (!state->enabled) {
> > + rzg2l_gpt_disable(rzg2l_gpt);
> > + ret = 0;
> > + goto done;
> > + }
> > +
> > + mutex_lock(&rzg2l_gpt->lock);
> > + ret = rzg2l_gpt_config(chip, pwm, state);
> > + mutex_unlock(&rzg2l_gpt->lock);
> > + if (ret)
> > + goto done;
> > +
> > + return rzg2l_gpt_enable(rzg2l_gpt);
> > +
> > +done:
> > + pm_runtime_put(chip->dev);
> > +
> > + return ret;
> > +}
> > +
> > +static const struct pwm_ops rzg2l_gpt_ops = {
> > + .request = rzg2l_gpt_request,
> > + .free = rzg2l_gpt_free,
> > + .get_state = rzg2l_gpt_get_state,
> > + .apply = rzg2l_gpt_apply,
> > + .owner = THIS_MODULE,
> > +};
> > +
> > +static const struct of_device_id rzg2l_gpt_of_table[] = {
> > + { .compatible = "renesas,rzg2l-gpt", },
> > + { /* Sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, rzg2l_gpt_of_table);
> > +
> > +static void rzg2l_gpt_reset_assert_pm_disable(void *data) {
> > + struct rzg2l_gpt_chip *rzg2l_gpt = data;
> > +
> > + pm_runtime_disable(rzg2l_gpt->chip.dev);
> > + reset_control_assert(rzg2l_gpt->rstc);
> > +}
> > +
> > +static int rzg2l_gpt_probe(struct platform_device *pdev) {
> > + struct rzg2l_gpt_chip *rzg2l_gpt;
> > + struct clk *clk;
> > + int ret, i;
> > +
> > + rzg2l_gpt = devm_kzalloc(&pdev->dev, sizeof(*rzg2l_gpt),
> GFP_KERNEL);
> > + if (!rzg2l_gpt)
> > + return -ENOMEM;
> > +
> > + rzg2l_gpt->mmio = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(rzg2l_gpt->mmio))
> > + return PTR_ERR(rzg2l_gpt->mmio);
> > +
> > + rzg2l_gpt->rstc = devm_reset_control_get_shared(&pdev->dev, NULL);
> > + if (IS_ERR(rzg2l_gpt->rstc))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(rzg2l_gpt->rstc),
> > + "get reset failed\n");
> > +
> > + ret = reset_control_deassert(rzg2l_gpt->rstc);
> > + if (ret)
> > + return dev_err_probe(&pdev->dev, ret,
> > + "cannot deassert reset control\n");
> > +
> > + pm_runtime_enable(&pdev->dev);
> > + ret = devm_add_action_or_reset(&pdev->dev,
> > + rzg2l_gpt_reset_assert_pm_disable,
> > + rzg2l_gpt);
> > + if (ret < 0)
> > + return ret;
> > +
> > + clk = devm_clk_get_enabled(&pdev->dev, NULL);
> > + if (IS_ERR(clk))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(clk),
> > + "cannot get clock\n");
> > +
> > + rzg2l_gpt->rate = clk_get_rate(clk);
> > + /*
> > + * We need to keep the clock on, in case the bootloader enabled
> PWM and
> > + * is running during probe().
> > + */
> > + if (!(rzg2l_gpt_read(rzg2l_gpt, RZG2L_GTCR) & RZG2L_GTCR_CST))
> > + devm_clk_put(&pdev->dev, clk);
>
> I still think this looks wrong. Please at least comment about the idea
> here. ie. devm_clk_put disables the clk and holding a reference on the
> clk isn't needed because runtime-pm handles the needed enabling.
OK, will add the comment. Yes, the code should be improved
1) This code is OK for the case bootloader is not turning on PWM.
2) But if Bootloader turns on the PWM, then we need extra handling to call
"devm_clk_put" when usage count becomes "0" after activation of PWM channels.
ie, use a private variable for tracking bootloader on case and call "devm_clk_put"
during "rzg2l_gpt_free" and reset the private variable.
After that there is no clock framework resources and PM handles the clocks
exclusively.
>
> Is this really true? Does runtime-pm disable the clk if after the clk
> wasn't put here both PWMs are disabled?
No it need special handling, See point 2 above.
Cheers,
Biju
>
> > + mutex_init(&rzg2l_gpt->lock);
> > +
> > + rzg2l_gpt->chip.dev = &pdev->dev;
> > + rzg2l_gpt->chip.ops = &rzg2l_gpt_ops;
> > + rzg2l_gpt->chip.npwm = 2;
> > + for (i = 0; i < rzg2l_gpt->chip.npwm; i++)
> > + rzg2l_gpt->gpt[i].ph = &rzg2l_gpt_phase_params[i];
> > +
> > + ret = devm_pwmchip_add(&pdev->dev, &rzg2l_gpt->chip);
> > + if (ret)
> > + dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
> > +
> > + return ret;
> > +}
> > +
> > +static struct platform_driver rzg2l_gpt_driver = {
> > + .driver = {
> > + .name = "pwm-rzg2l-gpt",
> > + .of_match_table = of_match_ptr(rzg2l_gpt_of_table),
> > + },
> > + .probe = rzg2l_gpt_probe,
> > +};
> > +module_platform_driver(rzg2l_gpt_driver);
> > +
> > +MODULE_AUTHOR("Biju Das <biju.das.jz@xxxxxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("Renesas RZ/G2L General PWM Timer (GPT) Driver");
> > +MODULE_LICENSE("GPL"); MODULE_ALIAS("platform:pwm-rzg2l-gpt");
> |
