Hi Stephen, On 11 May 2016 at 10:52, Stephen Warren <swar...@wwwdotorg.org> wrote: > On 05/10/2016 08:25 PM, Simon Glass wrote: >> >> Hi Stephen, >> >> On 4 May 2016 at 12:42, Stephen Warren <swar...@wwwdotorg.org> wrote: >>> >>> On 05/01/2016 01:27 PM, Simon Glass wrote: >>>> >>>> >>>> Hi Stephen, >>>> >>>> On 28 April 2016 at 17:08, Stephen Warren <swar...@wwwdotorg.org> wrote: >>>>> >>>>> >>>>> From: Stephen Warren <swar...@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 <swar...@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@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@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot