Hi Uwe Kleine-König, Thanks for the feedback. > On Fri, Jul 21, 2023 at 07:08:39AM +0100, Biju Das wrote: > > [...] > > +static int 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); > > + int rc; > > + > > + rc = pm_runtime_resume_and_get(chip->dev); > > + if (rc) > > + return rc; > > + > > + state->enabled = rzg2l_gpt_is_ch_enabled(rzg2l_gpt, pwm->hwpwm); > > + if (state->enabled) { > > + u32 ch = RZG2L_GET_CH(pwm->hwpwm); > > + u32 offs = RZG2L_GET_CH_OFFS(ch); > > + u8 prescale; > > + u64 tmp; > > + u32 val; > > + > > + val = rzg2l_gpt_read(rzg2l_gpt, offs + RZG2L_GTCR); > > + prescale = FIELD_GET(RZG2L_GTCR_TPCS, val); > > + > > + val = rzg2l_gpt_read(rzg2l_gpt, offs + RZG2L_GTPR); > > + tmp = NSEC_PER_SEC * (u64)val; > > + state->period = DIV_ROUND_UP_ULL(tmp, rzg2l_gpt->rate) << (2 * > > +prescale); > > You're loosing precision here. If for example we have: GTPR = 0xf, TPCS = 5 > and rzg2l_gpt->rate = 26000000, the output wave's period is: > > period = 0xf * 1000000000 / 26000000 << 10 > > The exact value is 590769.2307692308, so the right value to return is > 590770. However your calculation yields 590848. > > The problem is that the rounded value is further processed. Maybe we need a > function mul_u64_u64_div_u64_roundup(), or do you see a clever alternative? I think DIV64_U64_ROUND_UP() will do as it is giving right value 590770 where as DIV64_U64_ROUND_CLOSEST() is giving 590669. > > > + val = rzg2l_gpt_read(rzg2l_gpt, > > + offs + RZG2L_GTCCR(RZG2L_IS_IOB(pwm- > >hwpwm))); > > + > > + tmp = NSEC_PER_SEC * (u64)val; > > + state->duty_cycle = DIV_ROUND_UP_ULL(tmp, rzg2l_gpt->rate) << > (2 * > > +prescale); > > + > > + if (state->duty_cycle > state->period) > > + state->duty_cycle = state->period; > > + } > > + > > + state->polarity = PWM_POLARITY_NORMAL; > > + pm_runtime_put(chip->dev); > > + > > + return 0; > > +} > > + > > +static u32 rzg2l_gpt_calculate_pv_or_dc(u64 period_or_duty_cycle, u8 > > +prescale) { > > + return min(period_or_duty_cycle >> (2 * prescale), (u64)U32_MAX); } > > + > > Maybe mention in a comment here that rzg2l_gpt_config() is only called > holding the lock? Agreed. OK will add the comment /* 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 ch = RZG2L_GET_CH(pwm->hwpwm); > > + u32 offs = RZG2L_GET_CH_OFFS(ch); > > + unsigned long pv, dc; > > + u64 period_cycles; > > + u64 duty_cycles; > > + u8 prescale; > > + > > + /* > > + * 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->state_period[ch] && rzg2l_gpt- > >user_count[ch] > 1) > > + return -EBUSY; > > this is stricter than necessary, but if you don't want to spend additional > brain cycles (we're at v15 already), that's ok. Can be addressed later if > the need arises. Agreed. Will address it later based on the need. > > > + period_cycles = mul_u64_u32_div(state->period, rzg2l_gpt->rate, > NSEC_PER_SEC); > > + prescale = rzg2l_gpt_calculate_prescale(rzg2l_gpt, period_cycles); > > + > > + pv = rzg2l_gpt_calculate_pv_or_dc(period_cycles, prescale); > > + > > + duty_cycles = mul_u64_u32_div(state->duty_cycle, rzg2l_gpt->rate, > NSEC_PER_SEC); > > + dc = rzg2l_gpt_calculate_pv_or_dc(duty_cycles, prescale); > > + > > + /* > > + * GPT counter is shared by multiple channels, we cache the period > value > > + * from the first enabled channel and use the same value for both > > + * channels. > > + */ > > + rzg2l_gpt->state_period[ch] = state->period; > > + > > + /* > > + * If the PWM channel is disabled, make sure to turn on the clock > > + * before writing the register. > > + */ > > + if (!pwm->state.enabled) { > > + int rc; > > + > > + rc = pm_runtime_resume_and_get(chip->dev); > > + if (rc) > > + return rc; > > + } > > + > > + /* > > + * 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 > > + * first enabled channel. > > + */ > > + if (rzg2l_gpt->enable_count[ch] > 1) > > + rzg2l_gpt_modify(rzg2l_gpt, offs + RZG2L_GTCR, RZG2L_GTCR_CST, > 0); > > It's already late here, but I wonder if the condition is wrong here?! > s/>/<=/ ? Agreed. Will fix. > > > + /* GPT set operating mode (saw-wave up-counting) */ > > + rzg2l_gpt_modify(rzg2l_gpt, offs + RZG2L_GTCR, RZG2L_GTCR_MD, > > + RZG2L_GTCR_MD_SAW_WAVE_PWM_MODE); > > + > > + /* Set count direction */ > > + rzg2l_gpt_write(rzg2l_gpt, offs + RZG2L_GTUDDTYC, RZG2L_UP_COUNTING); > > + /* Select count clock */ > > + rzg2l_gpt_modify(rzg2l_gpt, offs + RZG2L_GTCR, RZG2L_GTCR_TPCS, > > + FIELD_PREP(RZG2L_GTCR_TPCS, prescale)); > > + > > + /* Set period */ > > + rzg2l_gpt_write(rzg2l_gpt, offs + RZG2L_GTPR, pv); > > + > > + /* Set duty cycle */ > > + rzg2l_gpt_write(rzg2l_gpt, offs + RZG2L_GTCCR(RZG2L_IS_IOB(pwm- > >hwpwm)), > > + dc); > > + > > + /* Set initial value for counter */ > > + rzg2l_gpt_write(rzg2l_gpt, offs + RZG2L_GTCNT, 0); > > + > > + /* Set no buffer operation */ > > + rzg2l_gpt_write(rzg2l_gpt, offs + RZG2L_GTBER, 0); > > + > > + /* Restart the counter after updating the registers */ > > + if (rzg2l_gpt->enable_count[ch] > 1) > > + rzg2l_gpt_modify(rzg2l_gpt, offs + RZG2L_GTCR, > > + RZG2L_GTCR_CST, RZG2L_GTCR_CST); > > + > > + /* If the PWM is not enabled, turn the clock off again to save power. > */ > > + if (!pwm->state.enabled) > > + pm_runtime_put(chip->dev); > > rzg2l_gpt_config() is only called if state->enabled is true, i.e. the > hardware is about to be enabled. I think it's not sensible in this case to > call pm_runtime_put(). OK will drop it. Cheers, Biju > > > + return 0; > > +} > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ |