Message-ID: <67f40dec.170a0220.5cda5.994c@mx.google.com>
Date: Mon, 7 Apr 2025 19:39:54 +0200
From: Christian Marangi <ansuelsmth@...il.com>
To: Uwe Kleine-König <ukleinek@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org,
	Benjamin Larsson <benjamin.larsson@...exis.eu>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	Lorenzo Bianconi <lorenzo@...nel.org>
Subject: Re: [RESEND PATCH v11] pwm: airoha: Add support for EN7581 SoC

On Thu, Apr 03, 2025 at 05:01:46PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Thu, Apr 03, 2025 at 12:50:27PM +0200, Christian Marangi wrote:
> > On Thu, Apr 03, 2025 at 11:58:48AM +0200, Uwe Kleine-König wrote:
> > > > +	if (hwpwm >= AIROHA_PWM_NUM_GPIO)
> > > > +		offset -= AIROHA_PWM_NUM_GPIO;
> > > > +
> > > > +	/* One FLASH_MAP register handles 8 pins */
> > > > +	*shift = do_div(offset, AIROHA_PWM_PINS_PER_FLASH_MAP);
> > > > +	*shift = AIROHA_PWM_REG_GPIO_FLASH_MAP_SHIFT(*shift);
> > > > +
> > > > +	if (offset < AIROHA_PWM_NUM_GPIO)
> > > > +		*addr = AIROHA_PWM_REG_GPIO_FLASH_MAP(offset);
> > > > +	else
> > > > +		*addr = AIROHA_PWM_REG_SIPO_FLASH_MAP(offset);
> > > 
> > > A single if would be a bit more straight forward. Something like:
> > > 
> > > 	static void airoha_pwm_get_flash_map_addr_and_shift(unsigned int hwpwm,
> > > 							    unsigned int *addr, unsigned int *shift)
> > > 	{
> > > 		u64 offset = hwpwm;
> > > 	
> > > 		if (hwpwm >= AIROHA_PWM_NUM_GPIO) {
> > > 			unsigned sipohwpwm = hwpwm - AIROHA_PWM_NUM_GPIO;
> > > 
> > > 			*shift = AIROHA_PWM_REG_GPIO_FLASH_MAP_SHIFT(sipohwpwm % AIROHA_PWM_PINS_PER_FLASH_MAP)
> > > 			*addr = AIROHA_PWM_REG_SIPO_FLASH_MAP(sipohwpwm);
> > > 		} else {
> > > 			*shift = AIROHA_PWM_REG_GPIO_FLASH_MAP_SHIFT(hwpwm % AIROHA_PWM_PINS_PER_FLASH_MAP)
> > > 			*addr = AIROHA_PWM_REG_GPIO_FLASH_MAP(hwpwm);
> > > 		}
> > > 	}
> > > 
> > 
> > I think you missed the do_div that do side effect on offset. Also that
> > needs to be / AIROHA_PWM_PINS_PER_FLASH_MAP.
> 
> Ack. Luckily you got the idea anyhow. This double effect of do_div() is
> easy to miss, so getting rid of them sounds sensible.
> 
> > > > +		if (duty_ns == bucket->duty_ns &&
> > > > +		    period_ns == bucket->period_ns)
> > > > +			return i;
> > > 
> > > If period_ns == 4010 and bucket->period_ns == 4000 you're not
> > > considering *bucket even though it has the right period setting.
> > > (period_ns is the requested period and not the expected period actually
> > > implemented by HW, right?)
> > > 
> > 
> > Doesn't that requires a different generator? The value we store in the
> > bucket is the requested period yes.
> 
> No it doesn't. If I understood right the possible periods are: 4ms, 8ms,
> ..., so a request to do 4.01ms will be mapped to 4ms which allows to
> share the generator.
> 
> > > > +				  AIROHA_PWM_GPIO_FLASH_EN << shift);
> > > > +		return;
> > > > +	}
> > > > +
> > > > +	regmap_update_bits(pc->regmap, addr,
> > > > +			   AIROHA_PWM_GPIO_FLASH_SET_ID << shift,
> > > > +			   FIELD_PREP(AIROHA_PWM_GPIO_FLASH_SET_ID, index) << shift);
> > > 
> > > Huh, I'd prefer:
> > > 
> > > 	regmap_update_bits(pc->regmap, addr,
> > > 			   AIROHA_PWM_GPIO_FLASH_SET_ID(hwpwm % 8)
> > > 			   FIELD_PREP(AIROHA_PWM_GPIO_FLASH_SET_ID(hwpwm % 8), index));
> > > 
> > > (That probably doesn't work out of the box because of the
> > > __builtin_constant_p check on mask, so you might need a local
> > > alternative to FIELD_PREP without that check.)
> > 
> > Honestly it's not worth to introduce custom FIELD_PREP for this. Yes the
> > problem is that FIELD_PREP requires constant mask so hwpwm % 8 is
> > problematic. An old implementation had stuff in define but resulted in
> > very ugly and confusing define and macro. The shift and FIELD_PREP
> > permits a much cleaner description in the define part at the cost of
> > that additional << shift needed.
> > 
> > Hope you can understand why I think it's better to keep it this way.
> 
> OK, I can live with that.
> 
> > > > +	bucket = airoha_pwm_consume_generator(pc, duty_ns, period_ns,
> > > > +					      hwpwm);
> > > > +	if (bucket < 0)
> > > > +		return -EBUSY;
> > > > +
> > > > +	/*
> > > > +	 * For SIPO reinit is always needed to trigger the shift register chip
> > > > +	 * and apply the new flash configuration.
> > > 
> > > I don't understand that sentence. What is the shift register chip? What
> > > is a flash configuration?
> > > 
> > 
> > The SoC can have attached a shift register chip to supports multiple LEDs.
> > The handling of this chip and comunication is done internally to the SoC
> > and it's exposed to register with these additional register.
> > 
> > When such channel are used with an assumed shift register, to apply the
> > new configuration in airoha_pwm_config_flash_map(), the shift register
> > chip needs to be reinit to actually refresh the chip internal register
> > with the new "flash configuration" (aka the values for
> > AIROHA_PWM_GPIO_FLASH_SET_ID)
> > 
> > Will add more comments to this to make it more clear.
> 
> Sounds good.
> 
> > > > +	state->enabled = FIELD_GET(AIROHA_PWM_GPIO_FLASH_EN, val >> shift);
> > > > +	if (!state->enabled)
> > > > +		return 0;
> > > > +
> > > > +	state->polarity = PWM_POLARITY_NORMAL;
> > > > +
> > > > +	bucket = FIELD_GET(AIROHA_PWM_GPIO_FLASH_SET_ID, val >> shift);
> > > > +	state->period = pc->buckets[bucket].period_ns;
> > > 
> > > Does .period_ns hold the requested or the actual period? You should
> > > return the latter.
> > 
> > Problem is that putting .period_ns here cause error in the PWM_DEBUG
> > validations. This is caused by the conversion error of the various / and
> > * done to convert tick to ns. Also on applying the configuration we
> > already do all the validation to make sure the request value is the
> > expected one.
> 
> Then there is a bug somewhere. I wouldn't completely rule out it's in
> the PWM_DEBUG logic, but my bet is on your driver then.
>  
> > For the initial values, there is airoha_pwm_fill_bucket that read the
> > current PWM values at boot and fill the buckets with the current values.
> > 
> > This is the compromise I found to handle both pre-configured bucket and
> > also handle the division errors in the ns - tick conversion.
> > 
> > Hope this is acceptable, do you have hint on better handling this?
> 
> I'd wait for the next iteration of your patch and then take a deeper
> look in the maths involved. It should be possible to make PWM_DEBUG
> happy and still report the actual configuration.
> 

Hi Uwe,

I just sent v12. While making more test I notice something interesting
where the duty was off-by-1 in get_state in the PWM_DEBUG scenario.

That caused the idemponent error, I found the cause so now get_state
actually return the real values instead of the local one in the bucket!

I address everything else, hope v12 is the one!

-- 
	Ansuel

Powered by blists - more mailing lists