[U-Boot] [PATCH 1/2] Add a reset driver framework/uclass

[Stephen Warren]

On 05/17/2016 03:56 PM, Simon Glass wrote:
> Hi Stephen,
>
> On 17 May 2016 at 10:46, Stephen Warren <swarren at wwwdotorg.org> wrote:
>> From: Stephen Warren <swarren at 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.