Hi Uwe,
On 30. 01. 19 16:39, Uwe Kleine-König wrote:
On Wed, Jan 30, 2019 at 03:42:29PM +0100, Michal Vokáč wrote:
[...]> There are quite some drivers known (to me) being buggy here. My feeling
is that Thierry doesn't share that impression and I think the only
reasonable path forward is to first fix the requirements for drivers and
when this is done, look at the implementations and fix them or conclude
that the requirements are not practical and shift them accordingly.
I can't really comment on this as my experience is limited to the
i.MX only, but what you say sounds quite reasonable. What I think might
be a problem is it seems there is not many active members in the "PWM
community". Namely I think it is you and Thierry. Others are just
occasional contributors to PWM hw drivers. So it is not going to be an
easy task to conclude on a new/better/fixed set of requirements.
From my POV the situation is as follows: I suggested a few changes that
seemed reasonable to me (e.g.: don't rely on the pin state to be
inactive after disable; don't rely on a driver to block in .apply until
the new configuration is active; thought about dropping .disable
altogether) and Thierry shot them down indicating that he is not willing
to change the API and all affected users for only one or a few
implementations. My consequence is to be strict about the requirements
because that's the only way to show the resulting pain (or see there
isn't so much pain).
I remember those discussions. That is why I am worried if this is ever
going to move forward as the whole issue dates at least back to 2015.
Fingers crossed!
I did not delve into that too much but I believe there are some HW
requirements on those platforms to stop the PWM that way. Others
simply stop the PWM straight away. So I am confused even more
and wonder where your requirements came from?
I'm aware that many drivers don't adhere to these requirements. IMHO
this is related to the poor situation regarding documentation for
pwm-driver authors and lack of time for in-deep reviews by Thierry.
Never the less: The requirement to complete the currently running period
comes from Thierry. There is a patch by me waiting for review that
targets improving the documentation and you already have to suffer from
my plan to spend some time on pwm-reviews :-)
Yeah, that is unpleasant if there are some requirements coming from
the maintainer and are not documented. The response times are not
really good as well. Except from you. That is why I actually welcome
the fact that you opted to spend your time on pwm reviews!
I looked at the documentation patch you submitted and at least it
clearly describes some of the questionable situations. Whether
I like them or not is irrelevant.
Anyway, as I could not find any real example I tried to solve it
according to your earlier suggestion. That is configure duty_cycle=0
and some time later disable PWM.
It generates additional problems. The PWM block has a FIFO with four
slots. Before adding the sample with duty cycle=0 into the FIFO it
is wise to wait for a free slot in the FIFO. Then when the sample is
added it may actually happen that the sample is the fourth in the
FIFO. So it may take up to four periods until we can disable the PWM.
These four periods can take up to 16s. Hmmm :(
No it cannot. Because if you put a new configuration into the FIFO you
have to block until the requested configuration is active, so it must
not happen that you hit the FIFO with 4 busy entries. (Unless a user
calls pwm_apply_state() in four contexts in parallel, which should not
happen. And if it does, we should implement serialization in the
pwm-framework such that pwm-drivers doesn't need to care.)
I was describing what the current code does/allows. There is a function
to check there is a free slot in the FIFO. It returns without blocking
if there is at least one free slot out of the four. So currently it
is possible to run this script:
#!/bin/sh
echo 0 > enable
echo 1000000000 > period # 1 second
echo 100000000 > duty_cycle # 1. sample, 100ms
echo 1 > enable # clears FIFO and writes 1. sample
echo 200000000 > duty_cycle # 2. sample, 200ms
echo 400000000 > duty_cycle # 3. sample, 400ms
echo 800000000 > duty_cycle # 4. sample, 800ms
and the driver does not block on writing the samples 2-4 and I can see
a pulse train of all the configured samples on a scope. This definitely
does not call pwm_apply_state() in parallel contexts.
If I attempt to write another sample unless the first period passes
(when the FIFO is still full) the driver does block for msleep(period_ms)
and then the new sample is written and I see all 5 samples on the scope.
The last (fith one) is repeated as expected.
From what you say here and what I read in your proposed change to the
documentation you require the driver to always block until the first
sample is utilized. So effectively shrinking the FIFO from 4 to 1 sample,
right?
This makes me to think: How we can define a fixed set of requirements
for (not just) PWM HW drivers and at the same time allow to utilize
the maximum capabilities of each HW?
I agree there are bugs in the driver and it is far from providing
complete support of the i.MX PWM HW. Still, I believe those are totally
independent problems from the pinctrl stuff and so should not block
the discussion/inclusion of this series.
I think while there are people who care about a driver, the known
problems should be addressed before a change is "sneaked" in that makes
the contributor happy and care about other stuff.
You can argue it is subjective but isn't the fact that the i.MX PWM
HW has some issues (and people are trying to solve them), a known
problem as well? I agree that there may be reasons to change priority
and first change/fix the requirements and then fix the drivers
accordingly. Unfortunately, even though I would love, I can not really
help much with that.
Best regards,
Michal
