[U-Boot] [PATCH] dm: allow setting driver_data before/during bind
Simon Glass
sjg at chromium.org
Wed May 11 20:59:48 CEST 2016
Hi Stephen,
On 11 May 2016 at 10:52, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 05/10/2016 08:25 PM, Simon Glass wrote:
>>
>> Hi Stephen,
>>
>> On 4 May 2016 at 12:42, Stephen Warren <swarren at wwwdotorg.org> wrote:
>>>
>>> On 05/01/2016 01:27 PM, Simon Glass wrote:
>>>>
>>>>
>>>> Hi Stephen,
>>>>
>>>> On 28 April 2016 at 17:08, Stephen Warren <swarren at wwwdotorg.org> wrote:
>>>>>
>>>>>
>>>>> From: Stephen Warren <swarren at nvidia.com>
>>>>>
>>>>> This will allow a driver's bind function to use the driver data. One
>>>>> example is the Tegra186 GPIO driver, which instantiates child devices
>>>>> for each of its GPIO ports, yet supports two different HW instances
>>>>> each
>>>>> with a different set of ports, and identified by the udevice_id .data
>>>>> field.
>>>>>
>>>>> Signed-off-by: Stephen Warren <swarren at nvidia.com>
>>>>> ---
>>>>> drivers/core/device.c | 7 ++++---
>>>>> drivers/core/lists.c | 6 +++---
>>>>> drivers/gpio/dwapb_gpio.c | 2 +-
>>>>> drivers/gpio/s5p_gpio.c | 2 +-
>>>>> drivers/gpio/sunxi_gpio.c | 2 +-
>>>>> drivers/gpio/tegra_gpio.c | 2 +-
>>>>> drivers/mtd/spi/sandbox.c | 2 +-
>>>>> drivers/net/mvpp2.c | 3 ++-
>>>>> drivers/pci/pci-uclass.c | 5 ++---
>>>>> drivers/power/pmic/pmic-uclass.c | 2 +-
>>>>> drivers/usb/host/usb-uclass.c | 5 ++---
>>>>> include/dm/device-internal.h | 5 +++--
>>>>> 12 files changed, 22 insertions(+), 21 deletions(-)
>>>>
>>>>
>>>>
>>>> I'm not sure this extra parameter carries its weight:
>>>>
>>>> - most callers just pass 0
>>>
>>>
>>> The same is true of the existing platdata field in many cases.
>>
>>
>> Yes, but platdata is defined to be needed by bind(), whereas
>> driver_data is supposed to be used in probe() to find out which
>> device tree compatible string matched.
>
>
> This seems to conflict with the documentation in include/dm/device.h; it
> claims that platdata is created by calling ofdata_to_platdata() just before
> calling probe(), at least for the DT case (for the case where U_BOOT_DEVICE
> is used, the platdata is available right from the start).
Exactly. That isn't a conflict. It's just that with DT the platform
data is provided by ofdata_to_platdata() whereas without it (the
degenerate case) it must be provided before bind().
>
> I couldn't find anywhere in the documentation that states when driver_data
> is supposed to be used; could you point me at it so I can read it?
No, but we should add something. See device.h:
* @of_offset: Device tree node offset for this device (- for none)
* @driver_data: Driver data word for the entry that matched this device with
* its driver
It is related to DT, nothing else.
>
>> Remember that the device tree
>>
>> properties are not looked at during bind(), only later. So it makes
>> sense to include platdata in the device_bind() call, but not
>> driver_data.
>
>
> Hmm, drivers/gpio/tegra_gpio.c:gpio_tegra_bind() uses DT (which you wrote or
> at least converted it to DM and chose where to put the DT accesses), and you
> very recently reviewed and applied "video: tegra: refuse to bind to disabled
> dcs" which modified drivers/video/tegra.c:tegra_lcd_bind() to use DT.
Re GPIO, this is because on tegra this information is not present in
the DT - it uses the 'interrupts' property to figure out the number of
GPIO banks. It does not involve using driverdata.
The video thing is checking for a disabled device, something that is
already done in dm_scan_fdt_node().
>
> Admittedly I do now see the following in doc/driver-model/README.txt:
>
>> The device's bind() method is permitted to perform simple actions, but
>> should not scan the device tree node, not initialise hardware, nor set up
>> structures or allocate memory. All of these tasks should be left for
>> the probe() method.
>
>
> ... but then I wonder why that rule was enforced for the patch in this
> thread, but not in the other cases?
>
> This inconsistency in review is extremely frustrating to me.
See above. There are always grey areas, but the two you cite don't
involve core DM changes.
>
>>>> - the field is supposed to be set up by device tree and probing tables,
>>>> not code
>>>
>>>
>>> While the existence of this new parameter does allow arbitrary code to
>>> set
>>> the parameter, this patch only actually sets the parameter in the case
>>> where
>>> DT and probing tables have determined that value.
>>
>>
>> I don't think so. That value is set in lists_bind_fdt().
>
>
> Sure, but that function is only used from 3 places, and explicitly accepts a
> parameter to indicate which DT node to instantiate a device for. It won't
> work unless a valid DT node is passed to it, and therefore can only work for
> DT-based probing.
>
>> I wonder if you could set it yourself after calling device_bind()?
>
>
> The Tegra186 GPIO driver explicitly needs to use the data inside the
> driver's bind() function in order to know how many child devices to
> instantiate. Setting the value after calling device_bind() (which the core
> DM code already does) is too late.
>
> For reference, you can see the exact code at:
> http://lists.denx.de/pipermail/u-boot/2016-April/252238.html
> "gpio: add Tegra186 GPIO driver"
>
> Search for "tegra186_gpio_bind" and look at the assignment to, and use of,
> ctlr_data.
>
> I had also quoted that part of the code in my previous email.
>
>>>> - bind() methods should not care about the driver data (they are not
>>>> allowed to touch hardware), so setting it later is fine
>>>
>>>
>>> Not touching HW is fine, but the driver data can still feed into purely
>>> SW
>>> decisions that bind makes. More details below.
>>>
>>>> - you can already pass platform data to the driver which is the
>>>> preferred communication method from a parent to its children
>>>
>>>
>>> I don't believe this is possible for devices instantiated from DT is it?
>>> In
>>> that case, platform data is always NULL:
>>
>>
>> That's right. For DT the paltform data is set up in the
>> ofdata_to_platdata() method. Since you are using DT, you should follow
>> that convention.
>
>
> This is the opposite of what you said above, which was that platdata is for
> bind().
I'm really not sure what you are saying here. It seems quite
straightforward to me:
- For the DT case (which is normal), the device tree data is used by
ofdata_to_platdata() to create the platdata. Before that there is no
platdata. The driver_data indicates which compatible string was
matched
- For the non-DT case (abnormal) it is provided by U_BOOT_DEVICE() and
available before bind()
>
>
>>> int lists_bind_fdt(struct udevice *parent, const void *blob, int offset,
()>>> struct udevice **devp)
>>> ...
>>> ret = device_bind(parent, entry, name, NULL, id->data,
>>> offset, &dev);
>>>
>>> (That quoted code is with this patch applied, and the NULL value is the
>>> platform data parameter.)
>>>
>>>> Also it's not clear from your Tegra 186 GPIO patch where you are using
>>>> this.
>>>
>>>
>>> Here's the relevant part from the Tegra186 GPIO driver patch I posted:
>>>
>>>> +static int tegra186_gpio_bind(struct udevice *parent)
>>>> +{
>>>> + struct tegra186_gpio_platdata *parent_plat = parent->platdata;
>>>> + struct tegra186_gpio_ctlr_data *ctlr_data =
>>>> + (struct tegra186_gpio_ctlr_data *)parent->driver_data;
>>>
>>>
>>> ...
>>>>
>>>>
>>>> + /* If this is a child device, there is nothing to do here */
>>>> + if (parent_plat)
>>>> + return 0;
>>>
>>>
>>> ...
>>>>
>>>>
>>>> + for (port = 0; port < ctlr_data->port_count; port++) {
>>>
>>>
>>> ...
>>>>
>>>>
>>>> + plat->name = ctlr_data->ports[port].name;
>>>> + plat->regs = &(regs[ctlr_data->ports[port].offset / 4]);
>>>
>>>
>>>
>>> The data is used to determine how many child devices (one per port) to
>>> create, and the name and register offset of each one. This is modelled
>>> after
>>> the logic in the previous Tegra GPIO driver that you wrote, with the
>>> unfortunate modification that the register layout is more "interesting"
>>> on
>>> Tegra186, and so we can't determine the number of and parameters for the
>>> child devices purely algorithmically, since the register layout is
>>> decidedly
>>> non-linear.
>>
>>
>> OK I see. This feels like something that your device tree should
>> describe.
>
>
> DT generally describes the presence of HW blocks, not the internal details
> of those HW blocks, since that is a fixed facet of the hardware design and
> can be statically derived from the value of the compatible property.
>
> Equally, I'm not sure how describing the details of the HW differences in DT
> would help. The details need to be known by the driver's bind() function,
> which you and the U-Boot documentation both state isn't allowed to access
> DT.
It would help because each bank would have a separate node, so you
wouldn't need any of this logic.
>
>> Failing that, how about a hard-coded table of information in
>> the source code? You can look through the table and create the
>> appropriate child devices.
>
>
> That is EXACTLY what the code is doing. The only issue is that the driver
> supports two different compatible properties and needs to know which one was
> found in DT in order to use the right table. That's what driver_data is; the
> .data value from the udevice_id/of_match table.
>
>>> I suppose an alternative would be to create separate U_BOOT_DRIVER()s for
>>> each compatible value with different register layout, and then have the
>>> bind() for each of those call into some common implementation with a
>>> hard-coded parameter. Still, it seems like the usage in the current code
>>> is
>>> exactly what udevice_id.data is for; to avoid having to implement
>>> separate
>>> functions that do that.
>>
>>
>> Yes, but you should use different compatible strings for the nodes.
>
>
> That's EXACTLY what the code does. However, there is currently no way for
> bind() to find out which entry in the udevice_id table matched, since the DM
> core currently sets driver_data after calling bind() rather than before.
OK I see.
>
>> As
>> I understand it, you only have a single node, so re-purposing this
>> does not seem right to me.
>
>
> I must not understand what you're conveying by "single node" here. To quote
> the example from the Tegra186 GPIO DT binding documentation, here is what
> the DT looks like:
>
> gpio at 2200000 {
> compatible = "nvidia,tegra186-gpio";
> reg-names = "security", "gpio";
> reg =
> <0x0 0x2200000 0x0 0x10000>,
> <0x0 0x2210000 0x0 0x10000>;
> interrupts =
> <0 47 IRQ_TYPE_LEVEL_HIGH>,
> <0 50 IRQ_TYPE_LEVEL_HIGH>,
> <0 53 IRQ_TYPE_LEVEL_HIGH>,
> <0 56 IRQ_TYPE_LEVEL_HIGH>,
> <0 59 IRQ_TYPE_LEVEL_HIGH>,
> <0 180 IRQ_TYPE_LEVEL_HIGH>;
> gpio-controller;
> #gpio-cells = <2>;
> interrupt-controller;
> #interrupt-cells = <2>;
> };
>
> gpio at c2f0000 {
> compatible = "nvidia,tegra186-gpio-aon";
> reg-names = "security", "gpio";
> reg =
> <0x0 0xc2f0000 0x0 0x1000>,
> <0x0 0xc2f1000 0x0 0x1000>;
> interrupts =
> <0 60 IRQ_TYPE_LEVEL_HIGH>;
> gpio-controller;
> #gpio-cells = <2>;
> interrupt-controller;
> #interrupt-cells = <2>;
> };
>
> Those two HW blocks are roughly but not exactly the same HW. Hence, there
> are two compatible values. The two HW blocks are similar enough to be
> handled by a single driver though, and hence the udevice_id table has two
> entries:
>
> static const struct udevice_id tegra186_gpio_ids[] = {
> {
> .compatible = "nvidia,tegra186-gpio",
> .data = (ulong)&tegra186_gpio_main_data,
> },
> {
> .compatible = "nvidia,tegra186-gpio-aon",
> .data = (ulong)&tegra186_gpio_aon_data,
> },
> { }
> };
>
> The .data value there provides all necessary information for the driver to
> handle both HW block designs.
>
> All I need is to be able to access the ".data" from that table in bind(),
> whereas the DM core currently doesn't allow that.
OK
>
>>> Perhaps the creation of the child devices could happen in probe() rather
>>> than bind()? I imagine there's some reason this wouldn't work (such as
>>> this
>>> causing the devices to be created too late to be referenced by other
>>> drivers?) or you would have done this in the existing Tegra GPIO driver.
>>
>>
>> Best not - it is good to have the devices known on start-up. Let me
>> know if the above solution doesn't work.
>
>
> I must admit, I didn't see any solution offered in your email.
>
> If the child devices must be created in bind(), which I do agree makes
> sense, then bind() must be able to determine which udevice_id table entry is
> related to the device being bound. Is there another way to do that besides
> using the driver_data from the udevice_id table? I suppose I could make
> bind() go read the compatible property from the DT node for the device, and
> manually search through the udevice_id table and find the matching entry.
> However, that would be duplicating work the DM core has already done, and
> exposes by setting the device's driver_data right after calling the driver's
> bind(), so doesn't make sense to me.
I was suggesting passing the platform data pointer to device_bind(),
but actually it doesn't help here as it is the parent device that
needs it.
This is a hard case. I think the least worst alternative is to add a
new device_bind_with_driver_data() function which can be called only
within driver model (from lists_bind_fdt() and I suppose
device_bind()). The existing device_bind() function signature can then
be left alone. Would that suit?
If so, can you please do a core DM patch and update the README to
explain this usage?
Regards,
Simon
More information about the U-Boot
mailing list