Messages in this thread | | | From | <> | Subject | Re: [PATCH 1/2] pwm: add microchip soft ip corePWM driver | Date | Sun, 12 Jun 2022 13:00:53 +0000 |
| |
Hey Uwe, one last one for ya..
On 08/06/2022 16:13, Uwe Kleine-König wrote: > Hello Conor, > > On Wed, Jun 08, 2022 at 12:12:37PM +0000, Conor.Dooley@microchip.com wrote: >> On 07/06/2022 21:07, Uwe Kleine-König wrote: >>> On Tue, Jun 07, 2022 at 09:45:51AM +0100, Conor Dooley wrote: >>>> Add a driver that supports the Microchip FPGA "soft" PWM IP core. >>>> >>>> Signed-off-by: Conor Dooley <conor.dooley@microchip.com> >>>> --- ---8<--- >>>> +struct mchp_core_pwm_registers { >>>> + u8 posedge; >>>> + u8 negedge; >>>> + u8 period_steps; >>>> + u8 prescale; >>> >>> these are the four registers for each channel, right? Can you add a >>> short overview how these registers define the resulting output wave. >> >> Ehh, only the edges are per channel. Does that change anything about >> your feedback? >> I'll add an explanation for each, sure. > > So the channels share the same period? If so you'll have to keep track > of which PWM channels are enabled and only change the period if no other > running channel is affected.
When I am capping the period (or not allowing it to be changed in this case here) should I correct the duty cycle so that the the ratio is preserved? Thanks, Conor.
> >>>> +}; >>>> + >>>> +struct mchp_core_pwm_chip { >>>> + struct pwm_chip chip; >>>> + struct clk *clk; >>>> + void __iomem *base; >>>> + struct mchp_core_pwm_registers *regs; >>>> +}; >>>> + >>>> +static inline struct mchp_core_pwm_chip *to_mchp_core_pwm(struct pwm_chip *chip) >>>> +{ >>>> + return container_of(chip, struct mchp_core_pwm_chip, chip); >>>> +} >>>> + >>>> +static void mchp_core_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm, >>>> + bool enable) >>>> +{ >>>> + struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip); >>>> + u8 channel_enable, reg_offset, shift; >>>> + >>>> + /* >>>> + * There are two adjacent 8 bit control regs, the lower reg controls >>>> + * 0-7 and the upper reg 8-15. Check if the pwm is in the upper reg >>>> + * and if so, offset by the bus width. >>>> + */ >>>> + reg_offset = PWM_EN_LOW_REG + (pwm->hwpwm >> 3) * sizeof(u32); >>>> + shift = pwm->hwpwm > 7 ? pwm->hwpwm - 8 : pwm->hwpwm; >>>> + >>>> + channel_enable = readb_relaxed(mchp_core_pwm->base + reg_offset); >>>> + channel_enable &= ~(1 << shift); >>>> + channel_enable |= (enable << shift); >>>> + >>>> + writel_relaxed(channel_enable, mchp_core_pwm->base + reg_offset); >>>> +} >>>> + >>>> +static void mchp_core_pwm_calculate_duty(struct pwm_chip *chip, >>>> + const struct pwm_state *desired_state, >>>> + struct mchp_core_pwm_registers *regs) >>>> +{ >>>> + struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip); >>>> + u64 clk_period = NSEC_PER_SEC; >>>> + u64 duty_steps; >>>> + >>>> + /* Calculate the clk period and then the duty cycle edges */ >>>> + do_div(clk_period, clk_get_rate(mchp_core_pwm->clk)); >>>> + >>>> + duty_steps = desired_state->duty_cycle * PREG_TO_VAL(regs->period_steps); >>>> + do_div(duty_steps, (clk_period * PREG_TO_VAL(regs->period_steps))); >>> >>> Don't divide by a result of a division. >>> >>>> + if (desired_state->polarity == PWM_POLARITY_INVERSED) { >>>> + regs->negedge = 0u; >>>> + regs->posedge = duty_steps; >>>> + } else { >>>> + regs->posedge = 0u; >>>> + regs->negedge = duty_steps; >>>> + } >>>> +} >>>> + >>>> +static void mchp_core_pwm_apply_duty(const u8 channel, >>>> + struct mchp_core_pwm_chip *pwm_chip, >>>> + struct mchp_core_pwm_registers *regs) >>>> +{ >>>> + void __iomem *channel_base = pwm_chip->base + channel * CHANNEL_OFFSET; >>>> + >>>> + writel_relaxed(regs->posedge, channel_base + POSEDGE_OFFSET); >>>> + writel_relaxed(regs->negedge, channel_base + NEGEDGE_OFFSET); >>>> +} >>>> + >>>> +static void mchp_core_pwm_apply_period(struct mchp_core_pwm_chip *pwm_chip, >>>> + struct mchp_core_pwm_registers *regs) >>>> +{ >>>> + writel_relaxed(regs->prescale, pwm_chip->base + PRESCALE_REG); >>>> + writel_relaxed(regs->period_steps, pwm_chip->base + PERIOD_REG); >>>> +} >>>> + >>>> +static int mchp_core_pwm_calculate_base(struct pwm_chip *chip, >>>> + const struct pwm_state *desired_state, >>>> + u8 *period_steps_r, u8 *prescale_r) >>>> +{ >>>> + struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip); >>>> + u64 tmp = desired_state->period; >>>> + >>>> + /* Calculate the period cycles and prescale value */ >>>> + tmp *= clk_get_rate(mchp_core_pwm->clk); >>>> + do_div(tmp, NSEC_PER_SEC); >>>> + >>>> + if (tmp > 65535) { >>> >>> If a too long period is requested, please configure the biggest possible >>> period. >>> >>>> + dev_err(chip->dev, >>>> + "requested prescale exceeds the maximum possible\n"); >>> >>> No error message in .apply() please. >> >> No /error/ or no error /message/? > > No error message. Otherwise a userspace application might easily trash > the kernel log. > >> As in, can I make it a dev_warn or do you want it removed entirely >> and replaced by capping at the max value? > > Yes, just cap to the max value. So the rule is always to pick the > biggest possible period not bigger than the requested period. And for > that one pick the biggest duty_cycle not bigger than the requested > duty_cycle. > >>>> + if (desired_state->enabled) { >>>> + if (current_state.enabled && >>>> + current_state.period == desired_state->period && >>>> + current_state.polarity == desired_state->polarity) { >>> >>> If everything is as before, why are you doing something at all? >> >> This is a change in duty without any other change. >> Could just remove this & recalculate everything when apply is called >> to simply the logic? > > Ah, right. A comment (e.g. "only duty cycle changed") would be good for > such stupid readers like me :-) > > I don't feel strong here. For many cases the period (and polarity) is > kept constant and only duty_cycle changes. So optimizing for that case > looks ok. > >>>> + mchp_core_pwm_calculate_duty(chip, desired_state, mchp_core_pwm->regs); >>>> + mchp_core_pwm_apply_duty(channel, mchp_core_pwm, mchp_core_pwm->regs); >>>> + } else { >>>> + ret = mchp_core_pwm_calculate_base(chip, desired_state, &period_steps_r, >>>> + &prescale_r); >>>> + if (ret) { >>>> + dev_err(chip->dev, "failed to calculate base\n"); >>> >>> mchp_core_pwm_calculate_base might already emit an error message. Apply >>> shouldn't emit an error message at all. >>> >>>> + return ret; >>>> + } >>>> + >>>> + mchp_core_pwm->regs->period_steps = period_steps_r; >>>> + mchp_core_pwm->regs->prescale = prescale_r; >>>> + >>>> + mchp_core_pwm_calculate_duty(chip, desired_state, mchp_core_pwm->regs); >>>> + mchp_core_pwm_apply_duty(channel, mchp_core_pwm, mchp_core_pwm->regs); >>>> + mchp_core_pwm_apply_period(mchp_core_pwm, mchp_core_pwm->regs); >>> >>> Is there a race where e.g. the output is defined by the previous period >>> and the new duty_cycle? >> >> "Yes". It depends on how the IP block is configured. I'll add a write to >> the sync register (which is a NOP if not configured for sync mode). > > Several drivers have a "Limitations" section at the top of the driver. > Something like that would be good to document there. Please stick to the > format found in e.g. pwm-sl28cpld.c, that is: "Limitations:" (even if > it's more about "Hardware Details") and then a list of items without > empty line in between for easy greppability. > >>> >>>> + } >>>> + >>>> + if (mchp_core_pwm->regs->posedge == mchp_core_pwm->regs->negedge) >>>> + mchp_core_pwm_enable(chip, pwm, false); >>>> + else >>>> + mchp_core_pwm_enable(chip, pwm, true); >>> >>> Here is a race: If the PWM is running and it configured to be disabled >>> with a different duty_cycle/period the duty_cycle/period might be >>> (shortly) visible on the output with is undesirable. >> >> This is trying to work around some nasty behaviour in the IP block. >> If negedge == posedge, it gives you a 50% duty cycle at twice the >> period you asked for. >> ie since the negedge and posedge are at the same time, it does >> whichever edge is actually possible each time that period step is >> reached. > > I didn't understand the normal behaviour of these registers yet, so > cannot comment. Usually it's a good idea to document corner cases in > comments. > >> If the state requested is disabled, it should be caught by the if() >> prior to entering this & exit early & avoid this entirely. >> >> I'll put the sync reg write after this, so if the block is configured >> to support sync updates, the output waveform won't do anything odd. > > Sounds good. > > Best regards > Uwe > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
| |