[PATCH 1/2] pwm: add microchip soft ip corePWM driver
Conor.Dooley at microchip.com
Conor.Dooley at microchip.com
Sun Jun 12 06:00:53 PDT 2022
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 at 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 at 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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
More information about the linux-riscv
mailing list