[PATCH 2/3] ARM: bcm2835: Add the Raspberry Pi firmware driver

[Eric Anholt]

Stephen Warren <swarren at wwwdotorg.org> writes:

> On 05/12/2015 11:46 AM, Eric Anholt wrote:
>> Stephen Warren <swarren at wwwdotorg.org> writes:
>>
>>> On 04/29/15 11:51, Eric Anholt wrote:
>>>> Stephen Warren <swarren at wwwdotorg.org> writes:
>>>>
>>>>> On 04/27/2015 05:14 PM, Eric Anholt wrote:
>>>>>> This gives us a function for making mailbox property channel requests
>>>>>> of the firmware, and uses it to control the 3 power domains provided
>>>>>> by the firmware.
>>>>>
>>>>>> diff --git a/arch/arm/mach-bcm/raspberrypi-firmware.c b/arch/arm/mach-bcm/raspberrypi-firmware.c
>>>>>
>>>>>> +/*
>>>>>> + * Submits a set of concatenated tags to the VPU firmware through the
>>>>>> + * mailbox property interface.
>>>>>> + *
>>>>>> + * The buffer header and the ending tag are added by this function and
>>>>>> + * don't need to be supplied, just the actual tags for your operation.
>>>>>> + * See struct raspberrypi_firmware_property_tag_header for the per-tag structure.
>>>>>> + */
>>>>>> +int raspberrypi_firmware_property(void *data, size_t tag_size)
>>>>>> +{
>>>>>> +	size_t size = tag_size + 12;
>>>>>> +	u32 *buf;
>>>>>> +	dma_addr_t bus_addr;
>>>>>> +	int ret = 0;
>>>>>> +
>>>>>> +	if (!firmware)
>>>>>> +		return -EPROBE_DEFER;
>>>>>
>>>>> I think it'd make more sense if the clients looked up the firmware
>>>>> driver via phandle at their probe time. This would mean:
>>>>>
>>>>> * No need for global "firmware", since clients could pass the firmware
>>>>> driver handle into this function.
>>>>>
>>>>> * Clients resolve deferred probe at their probe time. That way, they
>>>>> won't register themselves with subsystems asserting they can provide
>>>>> services, but find out they can't yet provide the service at that time.
>>>>
>>>> The one client so far (vc4) was resolving deferred probe at its probe
>>>> time, but not taking a reference on the firmware driver.  I figure I'll
>>>> have it do the phandle lookup and refcount -- do you still want the
>>>> struct platform_device passed in here?  If we de-global firmware, it's
>>>> going to mean some faffing in the power domain side of things to find
>>>> the device again, it seems.
>>>
>>> I think I'd expect the API in the firmware driver to require the client
>>> to pass the client DT node pointer plus a property name, and do all the
>>> lookup itself. That's what most DT resource lookup APIs in the kernel do
>>> now.
>>
>> I've made this change, but it's not great -- the client has to know some
>> details of how this driver is structured (that it sets the drvdata) in
>> order to figure out whether the driver is loaded or not and return
>> -EPROBE_DEFERRED.  I couldn't find any other existing solutions than
>> that in the tree.
>
> The client shouldn't need to know that.
>
> I'd expect the client to pass a DT node pointer to the provider's 
> driver, and that provider driver would know and isolate all the details 
> about its own internals. The provider could return some kind of 
> handler/... to the provider device if required.
>
> I would expect any subsystem that supports client->provider references 
> in DT would have an example of this (e.g. IRQs, GPIOs, regulators, ...). 
> However, the code for those can be rather complex to dive into for the 
> first time. For a fairly simple standalone example, check out:
>
> Provider:
> drivers/amba/tegra-ahb.c
> tegra_ahb_enable_smmu()
>
> Consumer:
> drivers/iommu/tegra-smmu.c
> tegra_smmu_ahb_enable()
> (called from tegra_smmu_probe() in the same file)

It's somewhat suspicious to me as an example of how -EPROBE_DEFER ought
to work, that tegra_ahb_enable_smmu()'s return value isn't being checked
by tegra_smmu_ahb_enable().

That said, this looks like this is "Yeah, just call into the producer
driver to do setup, and if it returns -EPROBE_DEFER, then bail out and
return -EPROBE_DEFER, yourself."  That's what I was doing in this
function and its consumers, that you commented on, only I wasn't passing
in the dt node.  Is that all you wanted changed?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-rpi-kernel/attachments/20150512/1e75a572/attachment.sig>