On Mon, Jun 24, 2013 at 07:52:11PM +0200, Tomasz Figa wrote:

Hi Thierry,

On Wednesday 19 of June 2013 12:23:52 Tomasz Figa wrote:

quoted

On Wednesday 19 of June 2013 11:43:56 Thierry Reding wrote:

quoted

On Wed, Jun 19, 2013 at 12:45:02AM +0200, Tomasz Figa wrote:

quoted

On Wednesday 19 of June 2013 00:17:06 Thierry Reding wrote:

quoted

On Mon, Jun 17, 2013 at 10:50:40PM +0200, Tomasz Figa wrote:

quoted

On Monday 17 of June 2013 22:29:11 Thierry Reding wrote:

quoted

On Wed, Jun 05, 2013 at 11:18:13PM +0200, Tomasz Figa wrote:

[...]

quoted

quoted

quoted

+#ifndef CONFIG_CLKSRC_SAMSUNG_PWM
+static DEFINE_SPINLOCK(samsung_pwm_lock);
+#endif

Why is this lock global? Shouldn't it more correctly be part
of
samsung_pwm_chip?

There are few registers shared with samsung_pwm_timer
clocksource
driver and so normally the spinlock is exported from it. However
on
on some platforms (namely Exynos >=4x12) kernel can be compiled
without that driver, so the lock must be defined locally, just
to
synchronize multiple PWM channels, as they share registers as
well.

Okay, I think this needs further explanation. The clocksource
driver
is
used for what exactly? From a quick look it seems to be very much
PWM-
specific. According to the device tree binding for the PWM driver,
the
timer blocks can also be used as clock sources and clock event
timers.
So if I understand correctly you have setups where you use one or
more
channels as clock source or clock event timer and one or more
channels
as PWM outputs.

In that case it's a very bad idea to use a global lock to
synchronize
accesses. You need to do much more than that. To properly split
this
across several drivers there needs to be a mechanism to allocate
channels for use either as clock source/event timer or PWM.
Otherwise,
how do you know that drivers aren't stepping on each other's toes?

quoted

quoted

quoted

+		ret = pwm_samsung_parse_dt(chip);
+		if (ret)
+			return ret;
+
+		chip->chip.of_xlate = of_pwm_xlate_with_flags;
+		chip->chip.of_pwm_n_cells = 3;
+	} else {
+		if (!pdev->dev.platform_data) {
+			dev_err(&pdev->dev, "no platform data

specified\n");

quoted

quoted

+			return -EINVAL;
+		}
+
+		memcpy(&chip->variant, pdev->dev.platform_data,
+

sizeof(chip-

quoted

quoted

quoted

variant));

quoted

+	}

Obviously this needs some modification in order for the
variant
to
become constant. But I think you can easily do so by making
the
driver
match using the platform_driver's id_table field, similar to
how
the
matching is done for OF.

Generally output_mask is board-dependent and is passed inside a
variant
struct using platform_data pointer.

That's okay. But output_mask is the only thing that's
board-dependent.
Everything else in the variant is SoC dependent judging by the OF
device
table. So really only the output_mask should be part of the
platform
data.

quoted

Same platform data is used in samsung_pwm_timer clocksource
driver,
so
I just reused it here without adding the need to rename platform
device at runtime (see arch/arm/plat-samsung/devs.c).

Looking a bit at git log for the clocksource driver, there's this

commit:
	a3ce54f clocksource: samsung_pwm_timer: Do not request PWM 

mem

quoted

quoted

quoted

region

quoted

That's an ugly workaround for sharing registers between two
drivers.
There's a reason why drivers do request_mem_region(), and it is
precisely to prevent them from accessing the same registers. As I
already said above, I think you need to come up with some sort of
API
to
share resources between the drivers.

There was a similar issue a few months back with the pwm-tiehrpwm
and
pwm-tiecap drivers, which use a shared block of registers.
Initially
something similar was done as you do here, but eventually we came
up
with a much better solution that involved introducing a new driver
for
the shared functionality and an exported API.

The situation seems to be somewhat different here since you
actually
share the same resources for different functionality instead of
sharing
one subset of register across multiple drivers, but I think a
similar
solution can be applied here.

Well, current design, or rather lack of thereof, has been
established
after a significant amount of discussion, mostly with ARM SoC and
MFD
maintainers.

It started from a simple reorganization of the clocksource driver to
be
multiplatform friendly, without any means of synchronizing both
drivers
other than local_irq_save() and _restore() that used to be there
originally, before I started my work on this. This was enough to
work
correctly, because both drivers at the same time are used only on
uniprocessor systems.

Here's the thread with patches:
http://thread.gmane.org/gmane.linux.kernel.samsung-soc/16664/focus=1
726
7

Then I opened a Pandora's Box by asking for opinion in this thread:
http://thread.gmane.org/gmane.linux.kernel.samsung-soc/16480/focus=1
650
0

After that I started working on a complex framework for sharing this
IP
block that would solve all the problems with synchronization,
channel
allocation, device tree parsing, variant management, etc.

Here's an initial version, which started as an MFD master driver,
which
would let 2 client drivers use the hardware (only the clocksource
part
was implemented in that series):
http://thread.gmane.org/gmane.linux.kernel.samsung-soc/17464/focus=2
292
50

Still, that version didn't receive too good feedback, with comments
pointing me to change the architecture to master-slave, with the
clocksource driver being the master and PWM driver being the slave.
And
so v5 was made:
http://thread.gmane.org/gmane.linux.kernel.samsung-soc/17864

Yeah, this seemed to be going in the right direction.

quoted

This version was generally accepted, but then we discussed on IRC
(mostly me and Arnd) whether such complex infrastructure is really
needed and concluded that for this platform a simple shared spinlock
would be enough, based on following reasons:
- on all SoCs on which the clocksource driver is going to be used
there
is only one instance of the PWM block, with 5 channels

You say that now. But then at some point somebody decides that it
might
be useful to have a second instance.

No, he won't.

Simply because no new SoC is going to be made which will use the
samsung_pwm_timer driver and all existing ones have only a single
instance of this IP.

quoted

quoted

- on all existing boards there are always at least two channels that
don't have outputs

I don't see how that's relevant here.

This is the information passed through output_mask. Clocksource driver
can use _only_ channels without output and PWM driver can use _only_
channels with output.

Both drivers receive the same output_mask value, either from the same
variant structure or from device tree.

quoted

quoted

- operations on particular PWM channels must be synchronized anyway,
because there are registers shared by all PWM channels (TCON being
most
important where enable, autoreload and invert bits are located for
all
channels).

And that's precisely why there should be a more explicit way of
synchronizing than just acquiring a lock. Otherwise you have no
possibility whatsoever to detect when a board tries to use both the
clocksource and PWM drivers in incompatible ways. The lock may prevent
concurrent accesses to the shared registers, but that won't help you
if
the PWM driver suddenly decides to use an output that was originally
meant to be used as clock source.

See above.

quoted

quoted

And so we got to current design which is basically a shared spinlock
and
per-board output_mask, which specifies which channels have outputs
on
particular boards. If you look to the clocksource driver, you can
see
that it tries to allocate two output-less channels for timekeeping
purposes and if it fails to do so, it simply panics.

IMHO this solution is fine, because it's simple, has little overhead
and
it just works in case of platforms for which it is needed.

Okay, I like simple. But I see too much potential for this to break
in the future. There are just too many assumptions for my taste. It
isn't anything that I'm looking forward to have to maintain.

Those are completely safe assumptions for existing platforms. Any new
platform with the same PWM IP will use this driver exclusively, without
the clocksource driver and so without the problems you mentioned.

Would you mind taking into account things I pointed in reply above?

I took those into account. But quite frankly they're no excuse for not
having at least a half-decent design. Even if you could guarantee that
the same IP wasn't used in a different combination than you expect,
keeping what you have now sets a very bad example and people will copy
because they don't know better and I'll have to explain this all over
again.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130624/44e97f05/attachment.sig>