Hello, as I already wrote in earlier revisions I find this driver complicated and wonder if this is because the hardware is complicated or because the driver adds unneeded complexity. So here come a few suggestions that might seem to be trivial but IMHO simplify understanding the driver. On Fri, Oct 18, 2024 at 02:00:44PM +0100, Biju Das wrote: > [...] > diff --git a/drivers/pwm/pwm-rzg2l-gpt.c b/drivers/pwm/pwm-rzg2l-gpt.c > new file mode 100644 > index 000000000000..28ed39eecb93 > --- /dev/null > +++ b/drivers/pwm/pwm-rzg2l-gpt.c > @@ -0,0 +1,473 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Renesas RZ/G2L General PWM Timer (GPT) driver > + * > + * Copyright (C) 2024 Renesas Electronics Corporation > + * > + * Hardware manual for this IP can be found here > + * https://www.renesas.com/eu/en/document/mah/rzg2l-group-rzg2lc-group-users-manual-hardware-0?language=en > + * > + * Limitations: > + * - Counter must be stopped before modifying Mode and Prescaler. > + * - When PWM is disabled, the output is driven to inactive. > + * - While the hardware supports both polarities, the driver (for now) > + * only handles normal polarity. > + * - General PWM Timer (GPT) has 8 HW channels for PWM operations and > + * each HW channel have 2 IOs. > + * - Each IO is modelled as an independent PWM channel. > + * - When both channels are used, disabling the channel on one stops the > + * other. > + */ > + > +#include <linux/bitfield.h> > +#include <linux/clk.h> > +#include <linux/io.h> > +#include <linux/limits.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/pwm.h> > +#include <linux/reset.h> > +#include <linux/time.h> > +#include <linux/units.h> > + > +#define RZG2L_GET_CH(a) ((a) / 2) The parameter is a hwpwm value. If you use "hwpwm" instead of "a" this is directly obvious. > +#define RZG2L_GET_CH_OFFS(i) (0x100 * (i)) The parameter is a channel number, rename it to ch. > +#define RZG2L_GTCR(ch) (0x2c + RZG2L_GET_CH_OFFS(ch)) > +#define RZG2L_GTUDDTYC(ch) (0x30 + RZG2L_GET_CH_OFFS(ch)) > +#define RZG2L_GTIOR(ch) (0x34 + RZG2L_GET_CH_OFFS(ch)) > +#define RZG2L_GTBER(ch) (0x40 + RZG2L_GET_CH_OFFS(ch)) > +#define RZG2L_GTCNT(ch) (0x48 + RZG2L_GET_CH_OFFS(ch)) > +#define RZG2L_GTCCR(ch, sub_ch) (0x4c + RZG2L_GET_CH_OFFS(ch) + 4 * (sub_ch)) > +#define RZG2L_GTPR(ch) (0x64 + RZG2L_GET_CH_OFFS(ch)) > + > +#define RZG2L_GTCR_CST BIT(0) > +#define RZG2L_GTCR_MD GENMASK(18, 16) > +#define RZG2L_GTCR_TPCS GENMASK(26, 24) > + > +#define RZG2L_GTCR_MD_SAW_WAVE_PWM_MODE FIELD_PREP(RZG2L_GTCR_MD, 0) > + > +#define RZG2L_GTUDDTYC_UP BIT(0) > +#define RZG2L_GTUDDTYC_UDF BIT(1) > +#define RZG2L_GTUDDTYC_UP_COUNTING (RZG2L_GTUDDTYC_UP | RZG2L_GTUDDTYC_UDF) > + > +#define RZG2L_GTIOR_GTIOA GENMASK(4, 0) > +#define RZG2L_GTIOR_GTIOB GENMASK(20, 16) > +#define RZG2L_GTIOR_GTIOx(a) ((a) ? RZG2L_GTIOR_GTIOB : RZG2L_GTIOR_GTIOA) sub_ch instead of a. > +#define RZG2L_GTIOR_OAE BIT(8) > +#define RZG2L_GTIOR_OBE BIT(24) > +#define RZG2L_GTIOR_OxE(a) ((a) ? RZG2L_GTIOR_OBE : RZG2L_GTIOR_OAE) > + > +#define RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE 0x1b > +#define RZG2L_GTIOR_GTIOA_OUT_HI_END_TOGGLE_CMP_MATCH \ > + (RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE | RZG2L_GTIOR_OAE) > +#define RZG2L_GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH \ > + (FIELD_PREP(RZG2L_GTIOR_GTIOB, RZG2L_INIT_OUT_HI_OUT_HI_END_TOGGLE) | RZG2L_GTIOR_OBE) > + > +#define RZG2L_GTIOR_GTIOx_OUT_HI_END_TOGGLE_CMP_MATCH(a) \ > + ((a) ? RZG2L_GTIOR_GTIOB_OUT_HI_END_TOGGLE_CMP_MATCH : \ > + RZG2L_GTIOR_GTIOA_OUT_HI_END_TOGGLE_CMP_MATCH) > + > +#define RZG2L_MAX_HW_CHANNELS 8 > +#define RZG2L_CHANNELS_PER_IO 2 > +#define RZG2L_MAX_PWM_CHANNELS (RZG2L_MAX_HW_CHANNELS * RZG2L_CHANNELS_PER_IO) > +#define RZG2L_MAX_SCALE_FACTOR 1024 > +#define RZG2L_MAX_TICKS ((u64)U32_MAX * RZG2L_MAX_SCALE_FACTOR) > + > +struct rzg2l_gpt_chip { > + void __iomem *mmio; > + struct mutex lock; /* lock to protect shared channel resources */ Hmm, I nearly claimed you'd not need that lock since 1cc2e1faafb3 ("pwm: Add more locking") but that doesn't cover ->request(). Probably that should change. (i.e. no action item for you.) > + unsigned long rate_khz; > + u32 period_ticks[RZG2L_MAX_HW_CHANNELS]; > + u32 user_count[RZG2L_MAX_HW_CHANNELS]; This tracks the count of requests per channel. So maybe call it channel_request_count? > + u32 enable_count[RZG2L_MAX_HW_CHANNELS]; channel_enable_count? > +}; > [...] > +/* Caller holds the lock while calling rzg2l_gpt_disable() */ > +static void rzg2l_gpt_disable(struct rzg2l_gpt_chip *rzg2l_gpt, > + struct pwm_device *pwm) > +{ > + u8 sub_ch = rzg2l_gpt_subchannel(pwm->hwpwm); > + u8 ch = RZG2L_GET_CH(pwm->hwpwm); > + > + /* Stop count, Output low on GTIOCx pin when counting stops */ > + rzg2l_gpt->enable_count[ch]--; > + > + if (!rzg2l_gpt->enable_count[ch]) > + rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR(ch), RZG2L_GTCR_CST, 0); > + > + /* Disable pin output */ > + rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTIOR(ch), RZG2L_GTIOR_OxE(sub_ch), 0); > +} > + > +static u64 calculate_period_or_duty(struct rzg2l_gpt_chip *rzg2l_gpt, u32 val, u8 prescale) Missing name prefix > +{ > [...] > +/* 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) > +{ > + struct rzg2l_gpt_chip *rzg2l_gpt = to_rzg2l_gpt_chip(chip); > + u8 sub_ch = rzg2l_gpt_subchannel(pwm->hwpwm); > + u8 ch = RZG2L_GET_CH(pwm->hwpwm); > + u64 period_ticks, duty_ticks; > + unsigned long pv, dc; > + u8 prescale; > + > + /* 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); > + if (period_ticks > RZG2L_MAX_TICKS) > + period_ticks = RZG2L_MAX_TICKS; > + /* > + * GPT counter is shared by multiple channels, so prescale and period shared by the two IOs of a single channel? > + * can NOT be modified when there are multiple channels in use with multiple IOs? > + * different settings. > + */ > + if (rzg2l_gpt->user_count[ch] > 1 && period_ticks < rzg2l_gpt->period_ticks[ch]) > + return -EBUSY; > + > + 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); > + if (duty_ticks > RZG2L_MAX_TICKS) > + duty_ticks = RZG2L_MAX_TICKS; > + dc = rzg2l_gpt_calculate_pv_or_dc(duty_ticks, prescale); > + > + /* > + * GPT counter is shared by multiple channels, we cache the period ticks > + * from the first enabled channel and use the same value for both > + * channels. > + */ > + rzg2l_gpt->period_ticks[ch] = period_ticks; Unless I'm missing something you might overwrite the value of the other IO in the same channel here. > + /* > + * Counter must be stopped before modifying mode, prescaler, timer > + * counter and buffer enable registers. These registers are shared > + * between both channels. So allow updating these registers only for the both IOs? > + * first enabled channel. > + */ > + if (rzg2l_gpt->enable_count[ch] <= 1) { > + rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR(ch), RZG2L_GTCR_CST, 0); > + > + /* GPT set operating mode (saw-wave up-counting) */ > + rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR(ch), RZG2L_GTCR_MD, > + RZG2L_GTCR_MD_SAW_WAVE_PWM_MODE); > + > + /* Set count direction */ > + rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTUDDTYC(ch), RZG2L_GTUDDTYC_UP_COUNTING); > + > + /* Select count clock */ > + rzg2l_gpt_modify(rzg2l_gpt, RZG2L_GTCR(ch), RZG2L_GTCR_TPCS, > + FIELD_PREP(RZG2L_GTCR_TPCS, prescale)); > + > + /* Set period */ > + rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTPR(ch), pv); > + } > + > + /* Set duty cycle */ > + rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTCCR(ch, sub_ch), dc); > + > + if (rzg2l_gpt->enable_count[ch] <= 1) { > + /* Set initial value for counter */ > + rzg2l_gpt_write(rzg2l_gpt, RZG2L_GTCNT(ch), 0); > + > + /* 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); > + } So you're not writing duty_cycle to hardware. Then you should check that the actual value in use is <= the intended value as you did above with period. > +static int rzg2l_gpt_probe(struct platform_device *pdev) > +{ > [...] > + rstc = devm_reset_control_get_exclusive(dev, NULL); > + if (IS_ERR(rstc)) > + return dev_err_probe(dev, PTR_ERR(rstc), "get reset failed\n"); > + > + clk = devm_clk_get_enabled(dev, NULL); > + if (IS_ERR(clk)) > + return dev_err_probe(dev, PTR_ERR(clk), "cannot get clock\n"); > + > + ret = devm_clk_rate_exclusive_get(dev, clk); > + if (ret) > + return ret; > + > + rate = clk_get_rate(clk); > + if (!rate) > + return dev_err_probe(dev, -EINVAL, "gpt clk rate is 0"); > + > + /* > + * Refuse clk rates > 1 GHz to prevent overflow later for computing > + * period and duty cycle. > + */ > + if (rate > NSEC_PER_SEC) > + return dev_err_probe(dev, -EINVAL, "gpt clk rate is > 1GHz"); > + > + /* > + * Rate is in MHz and is always integer for peripheral clk > + * 2^32 * 2^10 (prescalar) * 10^6 (rate_khz) < 2^64 > + * So make sure rate is multiple of 1000. > + */ > + rzg2l_gpt->rate_khz = rate / KILO; > + if (rzg2l_gpt->rate_khz * KILO != rate) > + return dev_err_probe(dev, -EINVAL, "rate is not multiple of 1000"); > + > + ret = reset_control_deassert(rstc); Please move reset deassertion directly after devm_reset_control_get_exclusive() that it can later be trivially converted to devm_reset_control_get_exclusive_deasserted(). If you base the next revision on top of v6.13-rc1 you can also make use of it already. > + chip->ops = &rzg2l_gpt_ops; > + ret = devm_pwmchip_add(dev, chip); > + if (ret) > + return dev_err_probe(dev, ret, "failed to add PWM chip\n"); nitpick: Can you make the error messages all start with a capital letter please? Best regards Uwe
Attachment:
signature.asc
Description: PGP signature
