device_tree binding for "amba bus"

device_tree binding for "amba bus"

[Stephen Neuendorffer]

Grant,

I've taken a quick look at the arm trace buffer driver
(arch/arm/kernel/etm.c).
This driver uses the amba bus driver (drivers/amba/bus.c).
It appears that this driver does three things:

1) ensures that the clock named "apb_pclk" is running.
2) ensures that the regulator named "vcore" is on.
3) queries the core to verify that a hardcoded register has the correct
value.

None of this seems terribly interesting enough to justify a bus that has
a
small number of users:

drivers/tty/serial/amba-pl010.c, line 788
drivers/tty/serial/amba-pl011.c, line 1477
drivers/input/serio/ambakmi.c, line 197
drivers/watchdog/sp805_wdt.c, line 359
drivers/rtc/rtc-pl031.c, line 478
drivers/rtc/rtc-pl030.c, line 183
drivers/char/hw_random/nomadik-rng.c, line 97
drivers/mmc/host/mmci.c, line 1049
drivers/video/amba-clcd.c, line 538
drivers/gpio/pl061.c, line 344
drivers/spi/amba-pl022.c, line 2284
drivers/dma/amba-pl08x.c, line 2062
drivers/dma/pl330.c, line 841
sound/arm/aaci.c, line 1125
arch/arm/kernel/etm.c:
line 421
line 625

And at the very least the hardcoded nature of the names seems, well...
hardcoded.

Any thoughts about how to abstract this in a device tree?
(My thoughts: devices need to reference a clock, devices need to
reference a regulator,
and it seems to me that the register check could be pulled out into a
single call that the
devices that care about it can do themselves during probe() )

Steve


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

device_tree binding for "amba bus"

[Russell King - ARM Linux]

On Tue, May 17, 2011 at 01:28:33PM -0700, Stephen Neuendorffer wrote:
> None of this seems terribly interesting enough to justify a bus that has
> a small number of users:

However, without this infrastructure, drivers have to start doing those
three points you bring up themselves, manually, with additional code,
which makes for additional bugs.  No thanks.  Let's keep the bus
abstraction there as it serves to ensure that the right things happen
at the right time.

> And at the very least the hardcoded nature of the names seems, well...
> hardcoded.

What hardcoded names?  Driver names no matter what bus are part of the
_userspace_ API - remember that almost everything in the device model
is exported to userspace and is therefore part of the kernel's userspace
API.  So logically the driver names do want to be stable.

> Any thoughts about how to abstract this in a device tree?
> (My thoughts: devices need to reference a clock, devices need to
> reference a regulator,
> and it seems to me that the register check could be pulled out into a
> single call that the
> devices that care about it can do themselves during probe() )

And another during their remove callback - and where do they store the
data associated with that?  Will the drivers need to have a certain
base driver_data structure?

This all sounds like getting rather icky and prone to bugs.

device_tree binding for "amba bus"

[Stephen Neuendorffer]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Tuesday, May 17, 2011 3:42 PM
> To: Stephen Neuendorffer
> Cc: grant.likely at secretlab.ca; devicetree-discuss at lists.ozlabs.org;
linux-arm-
> kernel at lists.infradead.org
> Subject: Re: device_tree binding for "amba bus"
> 
> On Tue, May 17, 2011 at 01:28:33PM -0700, Stephen Neuendorffer wrote:
> > None of this seems terribly interesting enough to justify a bus that
has
> > a small number of users:
> 
> However, without this infrastructure, drivers have to start doing
those
> three points you bring up themselves, manually, with additional code,
> which makes for additional bugs.  No thanks.  Let's keep the bus
> abstraction there as it serves to ensure that the right things happen
> at the right time.
>
> > And at the very least the hardcoded nature of the names seems,
well...
> > hardcoded.
> 
> What hardcoded names?  Driver names no matter what bus are part of the
> _userspace_ API - remember that almost everything in the device model
> is exported to userspace and is therefore part of the kernel's
userspace
> API.  So logically the driver names do want to be stable.

In drivers/amba/bus.c:

static int amba_get_enable_pclk(struct amba_device *pcdev)
{
	struct clk *pclk = clk_get(&pcdev->dev, "apb_pclk");

static int amba_get_enable_vcore(struct amba_device *pcdev)
{
	struct regulator *vcore = regulator_get(&pcdev->dev, "vcore");

So users of this bus have to have a clock and a regulator with those
exact names.
Is it your expectation that the clock/regulator names are standardized
across arch/arm?

And a little grepping suggests that almost all of the machines that
possibly use
this don't do anything useful with it (just implementing duplicated
dummy code
so that "apb_pclk" has something to refer to).

[stephenn at xsjfox4 arm]$ grep -r apb_pclk mach-*
mach-bcmring/core.c:static struct clk dummy_apb_pclk = {
mach-bcmring/core.c:            .con_id = "apb_pclk",
mach-bcmring/core.c:            .clk = &dummy_apb_pclk,
mach-ep93xx/clock.c:    INIT_CK(NULL,                   "apb_pclk",
&clk_p),
mach-integrator/core.c:static struct clk dummy_apb_pclk;
mach-integrator/core.c:         .con_id         = "apb_pclk",
mach-integrator/core.c:         .clk            = &dummy_apb_pclk,
mach-mxs/clock-mx23.c:  _REGISTER_CLOCK("duart", "apb_pclk", xbus_clk)
mach-mxs/clock-mx28.c:  _REGISTER_CLOCK("duart", "apb_pclk", xbus_clk)
mach-nomadik/clock.c:           .con_id         = "apb_pclk",
mach-omap2/clock3xxx_data.c:static struct clk dummy_apb_pclk = {
mach-omap2/clock3xxx_data.c:    .name           = "apb_pclk",
mach-omap2/clock3xxx_data.c:    CLK(NULL,       "apb_pclk",
&dummy_apb_pclk,        CK_3XXX),
mach-realview/core.c:static struct clk dummy_apb_pclk;
mach-realview/core.c:           .con_id         = "apb_pclk",
mach-realview/core.c:           .clk            = &dummy_apb_pclk,
mach-spear3xx/clock.c:static struct clk dummy_apb_pclk;
mach-spear3xx/clock.c:  { .con_id = "apb_pclk", .clk = &dummy_apb_pclk},
mach-spear6xx/clock.c:static struct clk dummy_apb_pclk;
mach-spear6xx/clock.c:  { .con_id = "apb_pclk", .clk = &dummy_apb_pclk},
mach-u300/clock.c: * The MMCI apb_pclk is hardwired to the same terminal
as the
mach-u300/clock.c: * The SPI apb_pclk is hardwired to the same terminal
as the
mach-u300/clock.c:      DEF_LOOKUP_CON("intcon", "apb_pclk",
&intcon_clk),
mach-u300/clock.c:      DEF_LOOKUP_CON("pl172", "apb_pclk", &emif_clk),
mach-u300/clock.c:      DEF_LOOKUP_CON("mmci", "apb_pclk", &mmcsd_clk),
mach-u300/clock.c:      DEF_LOOKUP_CON("pl022", "apb_pclk", &spi_clk),
mach-u300/clock.c:      DEF_LOOKUP_CON("uart1", "apb_pclk",
&uart1_pclk),
mach-u300/clock.c:      DEF_LOOKUP_CON("uart0", "apb_pclk",
&uart0_pclk),
mach-ux500/clock.c:static struct clk clk_dummy_apb_pclk = {
mach-ux500/clock.c:     .name = "apb_pclk",
mach-ux500/clock.c:     CLK(dummy_apb_pclk, NULL,       "apb_pclk"),
mach-versatile/core.c:static struct clk dummy_apb_pclk;
mach-versatile/core.c:          .con_id         = "apb_pclk",
mach-versatile/core.c:          .clk            = &dummy_apb_pclk,
mach-vexpress/v2m.c:static struct clk dummy_apb_pclk;
mach-vexpress/v2m.c:            .con_id         = "apb_pclk",
mach-vexpress/v2m.c:            .clk            = &dummy_apb_pclk,


My thinking (at least with respect to these) was that clock sources and
regulators could
be represented by nodes in the device tree and if a device required a
particular clock
source to be enabled that it would declare that in the device tree.
This would enable
reusing the existing etb driver (more or less) in a device tree platform
(mach-zynq).
Basically, it seems to me that if amba_bus does something useful, then
its utility should
be generalized into the generic platform_bus, which would enable it to
be used with
device trees, rather than building device tree specific code to enable
amba_bus.

Note that for arch/arm/kernel/etm.c, there is apparently another clock
that *isn't* handled by amba_bus
that it manages internally:

 	t->emu_clk = clk_get(&dev->dev, "emu_src_ck");
 	if (IS_ERR(t->emu_clk)) {
 		dev_dbg(&dev->dev, "Failed to obtain emu_src_ck.\n");
 		ret = -EFAULT;
		goto out;
 	}

This could be handled automatically by a more general abstraction of the
clock dependencies
of an arbitrary driver.

> > Any thoughts about how to abstract this in a device tree?
> > (My thoughts: devices need to reference a clock, devices need to
> > reference a regulator,
> > and it seems to me that the register check could be pulled out into
a
> > single call that the
> > devices that care about it can do themselves during probe() )
> 
> And another during their remove callback - and where do they store the
> data associated with that?  Will the drivers need to have a certain
> base driver_data structure?
>
> This all sounds like getting rather icky and prone to bugs.

I guess I was assuming that it would happen automatically in whatever
generic code would understand
the clocks and regulators in the first place?  (Note that I'm *NOT*
suggesting that the
driver be responsible for the things above, which seem to be
'fundamental infrastructure' 
things.)  I'm suggesting a way of reducing the amount of extra
machine-specific code,
while doing more or less what amba_bus does today.

The *other* thing that amba_bus does is validate the peripheral_id,
which seems to me to
be something that a driver could perhaps do for itself by calling a
validation function
in probe() rather than bothering to export it top amba_bus?

Forgive me if these are stupid questions, but I'm trying hard to figure
out to use the
driver code that is there without adding a bunch of new (duplicated, do
nothing)
machine-specific code to meet the assumptions of amba_bus.

Steve


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

device_tree binding for "amba bus"

[Russell King - ARM Linux]

On Tue, May 17, 2011 at 05:05:12PM -0700, Stephen Neuendorffer wrote:
> In drivers/amba/bus.c:
> 
> static int amba_get_enable_pclk(struct amba_device *pcdev)
> {
> 	struct clk *pclk = clk_get(&pcdev->dev, "apb_pclk");
> 
> static int amba_get_enable_vcore(struct amba_device *pcdev)
> {
> 	struct regulator *vcore = regulator_get(&pcdev->dev, "vcore");
> 
> So users of this bus have to have a clock and a regulator with those
> exact names.
> Is it your expectation that the clock/regulator names are standardized
> across arch/arm?

Let's fix one thing here.

The second argument to clk_get is *not* a unique clock name.  It is a
_connection_ name for the device, to distinguish between different
clocks on the same device.  It does not identify _any_ particular
clock on its own.

> And a little grepping suggests that almost all of the machines that
> possibly use this don't do anything useful with it (just implementing
> duplicated dummy code so that "apb_pclk" has something to refer to).

apb_pclk is the _bus_ clock for the device, which must be enabled for
any register access to the device, including the Primecell ID registers.
Most platforms do not have any facilities to control this clock, but
there are some which do.

One of those which does is the ST stuff:

> mach-u300/clock.c: * The MMCI apb_pclk is hardwired to the same terminal
> as the
> mach-u300/clock.c: * The SPI apb_pclk is hardwired to the same terminal
> as the
> mach-u300/clock.c:      DEF_LOOKUP_CON("intcon", "apb_pclk",
> &intcon_clk),
> mach-u300/clock.c:      DEF_LOOKUP_CON("pl172", "apb_pclk", &emif_clk),
> mach-u300/clock.c:      DEF_LOOKUP_CON("mmci", "apb_pclk", &mmcsd_clk),
> mach-u300/clock.c:      DEF_LOOKUP_CON("pl022", "apb_pclk", &spi_clk),
> mach-u300/clock.c:      DEF_LOOKUP_CON("uart1", "apb_pclk",
> &uart1_pclk),
> mach-u300/clock.c:      DEF_LOOKUP_CON("uart0", "apb_pclk",
> &uart0_pclk),

For these, they gate the bus clock along with the functional clock for the
device.  The side effect of that is using the clock enable points in the
driver-based code which are designed for the functional clock results in
the bus clock being turned off, and then the driver tries to access the
device registers - resulting in the device not being accessible.

So, we decided to separate out this bus clock, call it 'apb_pclk' (it's the
ARM Primecell Bus, PCLK input on the device) and allow platforms which have
this problem to deal with it _without_ having to add ifdefs or other shite
to all the drivers.

Note that you need to turn on this very clock to even read the IDs out of
the device, in order to match the driver.  So, it really doesn't make
sense for the driver to do it when the bus level code which understands
the extraction of the IDs needs to fiddle with it anyway.

I didn't want to go around _all_ the primecell drivers adding yet another
probe step, failure path in the probe function, and additional call in
their remove functions - every additional thing which needs to be done
in order is another thing that can get out of order or be forgotten in
the failure cleanup path.

The ST devices can alias the apb_pclk to the functional clock, thereby
ensuring that while the device needs to be accessible, both the device PCLK
and functional clock remain enabled.  Meanwhile others which don't allow
the PCLK to be turned on and off can control their functional clock as the
driver desires.

I hope this helps to understand what's going on here.

I think what we have today is an elegant solution to the problem
presented by some SoCs.

device_tree binding for "amba bus"

[Grant Likely]

On Tue, May 17, 2011 at 6:23 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, May 17, 2011 at 05:05:12PM -0700, Stephen Neuendorffer wrote:
>> In drivers/amba/bus.c:
>>
>> static int amba_get_enable_pclk(struct amba_device *pcdev)
>> {
>> ? ? ? struct clk *pclk = clk_get(&pcdev->dev, "apb_pclk");
>>
>> static int amba_get_enable_vcore(struct amba_device *pcdev)
>> {
>> ? ? ? struct regulator *vcore = regulator_get(&pcdev->dev, "vcore");
>>
>> So users of this bus have to have a clock and a regulator with those
>> exact names.
>> Is it your expectation that the clock/regulator names are standardized
>> across arch/arm?
>
> Let's fix one thing here.
>
> The second argument to clk_get is *not* a unique clock name. ?It is a
> _connection_ name for the device, to distinguish between different
> clocks on the same device. ?It does not identify _any_ particular
> clock on its own.

The draft device tree clock matching code reflects this too.  If an
"apb_pclk-clock" property (which is a phandle to a clock provider) is
provided by the device tree, then the dt clock decode logic will use
it to determine which clock source to use.  The current code is a bit
of an ugly hack, and the exact mechanics may change, but in principle
the device tree support code will continue to follow this model.
apb_pclk is the generic name, but it could be wired to any clock in
the system.

g.

device_tree binding for "amba bus"

[Mark Brown]

On Tue, May 17, 2011 at 05:05:12PM -0700, Stephen Neuendorffer wrote:

Please fix your mail client to word wrap properly.  You've got lines
alternating 80 and ~20 columns which is really hard to read.

> So users of this bus have to have a clock and a regulator with those
> exact names.
> Is it your expectation that the clock/regulator names are standardized
> across arch/arm?

For both clock and regulator APIs the names are defined in terms of the
consumer of the driver.  Even if the consumer drivers wouild open code
this each driver would the same for all platforms using the driver.

> My thinking (at least with respect to these) was that clock sources and
> regulators could
> be represented by nodes in the device tree and if a device required a
> particular clock
> source to be enabled that it would declare that in the device tree.

This is pretty much what happens already, it's just that the tables
doing the mappings are defined in code.  Many systems are already doing
the clock management as part of their runtime PM infrastructure for the
basic clocks required to keep things operational and there's a
generalisation of this just going into the core runtime PM code at the
minute.

> This could be handled automatically by a more general abstraction of the
> clock dependencies
> of an arbitrary driver.

OTOH some clocks will need to be actively managed by drivers at runtime
- they're not just basic "turn it on when I'm running" clocks.

This is all pretty much orthogonal to device tree, I don't understand
why you feel that it's related.