[PATCH 08/15] pwm: Add new pwm-samsung driver

[Thierry Reding]

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:
> > On Wednesday 19 of June 2013 11:43:56 Thierry Reding wrote:
> > > On Wed, Jun 19, 2013 at 12:45:02AM +0200, Tomasz Figa wrote:
> > > > On Wednesday 19 of June 2013 00:17:06 Thierry Reding wrote:
> > > > > On Mon, Jun 17, 2013 at 10:50:40PM +0200, Tomasz Figa wrote:
> > > > > > On Monday 17 of June 2013 22:29:11 Thierry Reding wrote:
> > > > > > > On Wed, Jun 05, 2013 at 11:18:13PM +0200, Tomasz Figa wrote:
> > > > > [...]
> > > > > 
> > > > > > > > +#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?
> > > > > 
> > > > > > > > +		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");
> > > > > > 
> > > > > > > > +			return -EINVAL;
> > > > > > > > +		}
> > > > > > > > +
> > > > > > > > +		memcpy(&chip->variant, pdev->dev.platform_data,
> > > > > > > > +
> > > > 
> > > > sizeof(chip-
> > > > 
> > > > > > >variant));
> > > > > > >
> > > > > > > > +	}
> > > > > > > 
> > > > > > > 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.
> > > > > 
> > > > > > 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
> > > > 
> > > > region
> > > > 
> > > > > 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.
> > > 
> > > > 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.
> > 
> > > > - 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.
> > 
> > > > - 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.
> > 
> > > > 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>