[U-Boot] [PATCH 5/9] clock: add Tegra186 clock driver

[Stephen Warren]

On 08/04/2016 07:36 PM, Simon Glass wrote:
> Hi Stephen,
>
> On 1 August 2016 at 09:46, Stephen Warren <swarren at wwwdotorg.org> wrote:
>> On 07/31/2016 07:02 PM, Simon Glass wrote:
>>>
>>> Hi Stephen,
>>>
>>> On 27 July 2016 at 15:24, Stephen Warren <swarren at wwwdotorg.org> wrote:
>>>>
>>>> From: Stephen Warren <swarren at nvidia.com>
>>>>
>>>> In Tegra186, on-SoC clocks are manipulated using IPC requests to the BPMP
>>>> (Boot and Power Management Processor). This change implements a driver
>>>> that does that. A tegra/ sub-directory is created to follow the existing
>>>> pattern. It is unconditionally selected by CONFIG_TEGRA186 since
>>>> virtually
>>>> any Tegra186 build of U-Boot will need the feature.
>>
>>
>>>> diff --git a/drivers/clk/tegra/tegra186-clk.c
>>>> b/drivers/clk/tegra/tegra186-clk.c
>>
>>
>>>> +static ulong tegra186_clk_get_rate(struct clk *clk)
>>>> +{
>>>> +       struct mrq_clk_request req;
>>>> +       struct mrq_clk_response resp;
>>>> +       int ret;
>>>> +
>>>> +       debug("%s(clk=%p) (dev=%p, id=%lu)\n", __func__, clk, clk->dev,
>>>> +             clk->id);
>>>> +
>>>> +       req.cmd_and_id = (CMD_CLK_GET_RATE << 24) | clk->id;
>>>> +
>>>> +       ret = tegra186_bpmp_call(clk->dev->parent, MRQ_CLK,
>>>> +                                &req, sizeof(req), &resp, sizeof(resp));
>>>
>>>
>>> Isn't his a MISC driver? Perhaps you can add a new method to
>>> UCLASS_MISC matching your requirements here?
>>
>>
>> The core BPMP driver is a MISC driver, yes.
>>
>> I don't see any advantage of making this call something that the MISC uclass
>> supports directly. This function is an internal implementation detail of the
>> BPMP, and certainly not something that every single MISC driver (for any SoC
>> for any HW module) would implement. If we did add direct support to the MISC
>> uclass, then the MISC "ops" structure would basically grow forever since
>> every single SoC's/HW's internal function calls would be added to it. This
>> would just bloat it up unnecessarily, and I don't see any advantage to
>> offset the disadvantage of that bloat.
>
> I'm really just suggesting that you could add one more method to the
> misc uclass, which does a simultaneous write and read.

OK, I guess I will do that if I must. However, I doubt we will see 
much/any benefit from this.

Taking a look at the cros_ec example you mentioned in your other mail, 
you'll note that the EC and BPMP ABIs are different. Sure, both accept:

device(udev) to "communicate on"
TX buffer, length
RX buffer, length

However, both of these functions take the message body from the client, 
locally create the TX header before sending, and strip off the RX header 
before giving it back to the caller. This means the function also takes 
all the parameters to construct the message header. These are different 
for EC (message number and version) vs. BPMP (just message number). 
While we could make the prototype of the misc uclass function accept 
both and just ignore the extra information, that seems quite hokey. 
Eventually we'll probably end up with some other similar situation that 
needs more parameters and we'll just have to edit every existing caller 
to provide dummy data. Equally, routing everything through the uclass 
function prevents any kind of type-safety for these generic parameters 
(say using an enum, or appropriate data size) since by definition the 
common function is common and therefore must use very generic types.

It's very obvious to me how routing truly common operations through 
standard APIs makes sense; it brings order, allows drivers to use the 
same APIs on all platforms, etc. However, once you get down to low-level 
internal details of tightly couple drivers, the benefits disappear.

Ideally, we'd actually have a single BPMP driver that provided all of 
clock, reset, power domain, I2C, ... from the one device (instantiated 
from the one BPMP device node). It's just an artificial limitation of 
U-Boot's DM that a given driver/device can only provide one service, so 
we must have multiple communicating devices. If DM allowed separation 
between devices and the services they provide (as Linux does for most 
subsystems at least), there wouldn't even be any 
driver-to-driver/device-to-device within the context of BPMP, so all 
this would be irrelevant.

>> FWIW, the Linux MFD (Multi-Function Devices) stack typically has "child"
>> (sub-)devices call custom APIs like this on the "parent"/container.
>
> Yes, I'm just trying to encourage use of the driver-model features -
> one of the design goals is to expose otherwise private driver
> interfaces.

I'm not sure that's a good goal as an absolute general thing. Sure it 
makes sense to standardize interfaces between unrelated drivers to 
remove coupling. However, when the devices/drivers really are tightly 
coupled rather than independent IP blocks, and especially if they only 
artificially exist due to DM structure, I don't see any benefit of 
forcing everything through DM.

You'd mentioned enhancing debug and visibility in your other email. I 
don't believe either would be benefited here. The debugger will step 
through a call to a function or a call to a function pointer just as 
easily. A human on the other hand will be able to understand what's 
going on much more easily if the call is directly to a named function 
rather than having to look up where a function pointer points, since 
there's no extra work to find out where the function pointer points.