On 05/17/2016 03:56 PM, Simon Glass wrote:
Hi Stephen,
On 17 May 2016 at 10:46, Stephen Warren swarren@wwwdotorg.org wrote:
From: Stephen Warren swarren@nvidia.com
A reset controller is a hardware module that controls reset signals that affect other hardware modules or chips.
This patch defines a standard API that connects reset clients (i.e. the drivers for devices affected by reset signals) to drivers for reset controllers/providers. Initially, DT is the only supported method for connecting the two.
The DT binding specification (reset.txt) was taken from Linux kernel v4.5's Documentation/devicetree/bindings/reset/reset.txt.
(trimming the quoted text is useful...)
diff --git a/include/reset_client.h b/include/reset_client.h
+/**
- struct reset_ctl - A handle to (allowing control of) a single reset signal.
- Clients provide storage for reset control handles. The content of the
- structure is managed solely by the reset API and reset drivers. A reset
- control struct is initialized by "get"ing the reset control struct. The
- reset control struct is passed to all other reset APIs to identify which
- reset signal to operate upon.
- @dev: The device which implements the reset signal.
- @id: The reset signal ID within the provider.
- Currently, the reset API assumes that a single integer ID is enough to
- identify and configure any reset signal for any reset provider. If this
- assumption becomes invalid in the future, the struct could be expanded to
- either (a) add more fields to allow reset providers to store additional
- information, or (b) replace the id field with an opaque pointer, which the
- provider would dynamically allocated during its .of_xlate op, and process
- during is .request op. This may require the addition of an extra op to clean
- up the allocation.
- */
+struct reset_ctl {
struct udevice *dev;/** Written by of_xlate. We assume a single id is enough for now. In the* future, we might add more fields here.*/unsigned long id;This feels like an obfuscation to me. Why not just use an int instead of this struct? This is what clock does.
DT allows multiple cells for all resource specifiers (GPIOs, mailboxes, clocks, resets, interrupts). There are actual real-world cases (at least in DTs in the Linux kernel, even if not in U-Boot[1]) of specifiers of more than a single cell. It may not be possible to pack those multiple cells into a single integer, and semantically doesn't make much sense to do so if it didn't make sense to do so in DT. Consequently, we must plan for this case by allowing for client resource handles more capable than a single integer. The drivers I'm currently working on do use only a single integer, so that's all I've put into this struct for now. However, I fully expect other driver authors to need to add additional fields here, as explained in the comments quoted above.
Note that this scheme is identical to the mailbox driver that you already ack'd.
It's also an abstraction not an obfuscation; clients should not need to know/care about how drivers-that-provide-resources or DT or any other scheme represents/configures resources; clients should simply get a single opaque handle that allows them to manipulate the resource through standard APIs. Forcing clients to store a separate provider device handle and ID integer (and in the future, any additional fields required) puts too heavy a burden on the resource consumer, and exposes knowledge to them that they do not need.
Re: the clock API: I'm planning on changing that to this style too, for the same reasons. Additionally, clk_get_by_index() currently returns part of the resource handle (the resource's integer ID) as the return value, and part (the provider udevice pointer) as an "out" function parameter, which I found extremely confusing and non-obvious when reading the function signature. With the scheme in this patch, a single value is returned to the caller by the "get" function, which makes life much simpler.