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)
> +#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?) 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.
> + 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.
> + /* 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));
> + 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"?
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?
> + }
> +
> + 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.
Is this really true? Does runtime-pm disable the clk if after the clk
wasn't put here both PWMs are disabled?
> + 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");
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
