Hi Uwe,
> Subject: RE: [PATCH v5 2/2] pwm: Add support for RZ/G2L GPT
>
> 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.
>
I have created v6 incorporating the comments above
and also updated handling of bootloader enabling/disabling pwm.
Please provide feedback, if any.
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");
> > |
