Re: [PATCH v3] pwm: i.MX: Avoid sample FIFO overflow for i.MX PWM version2

[Liu Ying]

Hi Sascha,

Thanks for your comments.

On 05/19/2014 03:11 PM, Sascha Hauer wrote:
> On Fri, May 16, 2014 at 01:11:08PM +0800, Liu Ying wrote:
>> The i.MX PWM version2 is embedded in several i.MX SoCs,
>> such as i.MX27, i.MX51 and i.MX6SL.  There are four 16bit
>> sample FIFOs in this IP, each of which determines the duty
>> period of a PWM waveform in one full cycle.  The IP spec
>> mentions that we should not write a fourth sample because
>> the FIFOs will become full and trigger a FIFO write error
>> (FWE) which will prevent the PWM from starting once it is
>> enabled.  In order to avoid any sample FIFO overflow issue,
>> this patch clears all sample FIFOs or waits for a rollover
>> event to consume a FIFO slot when necessary.  In this way,
>> only the first FIFO slot will be loaded at most.
>>
>> Note that the PWM controller will not generate any rollover
>> event if the duty period is zero.  This makes the logic a
>> bit complicated to determine if we clear the sample FIFOs
>> or wait for a rollover event.
>>
>> The FIFO overflow issue can be reproduced by the following
>> commands on the i.MX6SL EVK platform, assuming we use PWM2
>> for the debug LED which is driven by the pin HSIC_STROBE
>> and the maximal brightness is 255.
>> echo 0   > /sys/class/leds/user/brightness
>> echo 0   > /sys/class/leds/user/brightness
>> echo 0   > /sys/class/leds/user/brightness
>> echo 0   > /sys/class/leds/user/brightness
>> echo 255 > /sys/class/leds/user/brightness
>> Here, FWE happens(PWMSR register reads 0x58) and the LED
>> can not be lighten.
> 
> I find this overly complicated. It adds 89 lines to a 300 lines driver
> for a single workaround. Wouldn't it be sufficient to add a delay
> of period_ns in imx_pwm_config_v2 when the FIFO is full?

The delay approach looks ok if the engine is enabled.
But, if the engine is disabled, adding delay does not make sense, since
the engine will not consume any FIFO slot.
So, two cases should be handled separately at least:
1) The engine is enabled and the FIFO is full, we delay for period_ns.
2) The engine is disabled and the FIFO is full, we reset the engine?

I like the rollover interrupt approach better, because it may avoid
the potential unnecessary lag the delay approach introduces(when the
engine is enabled, the FIFO is full and one FIFO slot is consumed at
the time we delay).
Moreover, IMHO, a delay approach is somewhat the last choice for a driver.

> 
> Sascha
> 

-- 
Liu Ying
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/