diff mbox series

dm: core: Read parent ofdata before children

Message ID 20200405153813.1.I23fe0a6aceda96decde70b46477268190ea77d83@changeid
State Accepted
Commit b0dcc87106464c3fc019e3771378a092fd32ebdb
Headers show
Series dm: core: Read parent ofdata before children | expand

Commit Message

Simon Glass April 5, 2020, 9:38 p.m. UTC 
At present a device can read its ofdata before its parent has done the
same. This can cause problems in the case where the parent has a 'ranges'
property, thus affecting the operation of dev_read_addr(), for example.

We already probe parent devices before children so it does not seem to be
a large step to do the same with ofdata.

Make the change and update the documentation in this area.

Signed-off-by: Simon Glass <sjg at chromium.org>
---

 doc/driver-model/design.rst | 94 +++++++++++++++++++++++++++----------
 drivers/core/device.c       | 16 +++++++
 test/dm/test-fdt.c          | 25 ++++++++++
 3 files changed, 111 insertions(+), 24 deletions(-)

Comments

Tan, Ley Foon April 6, 2020, 8:53 a.m. UTC | #1 
> -----Original Message-----
> From: Simon Glass <sjg at chromium.org>
> Sent: Monday, April 6, 2020 5:38 AM
> To: U-Boot Mailing List <u-boot at lists.denx.de>
> Cc: Ang, Chee Hong <chee.hong.ang at intel.com>; Tom Rini
> <trini at konsulko.com>; Tan, Ley Foon <ley.foon.tan at intel.com>;
> Westergreen, Dalon <dalon.westergreen at intel.com>; See, Chin Liang
> <chin.liang.see at intel.com>; Simon Goldschmidt
> <simon.k.r.goldschmidt at gmail.com>; Marek Vasut <marex at denx.de>;
> Simon Glass <sjg at chromium.org>; Bin Meng <bmeng.cn at gmail.com>; Dario
> Binacchi <dariobin at libero.it>; Heinrich Schuchardt <xypron.glpk at gmx.de>;
> Patrick Delaunay <patrick.delaunay at st.com>; Pavel Herrmann
> <morpheus.ibis at gmail.com>
> Subject: [PATCH] dm: core: Read parent ofdata before children
> 
> At present a device can read its ofdata before its parent has done the same.
> This can cause problems in the case where the parent has a 'ranges'
> property, thus affecting the operation of dev_read_addr(), for example.
> 
> We already probe parent devices before children so it does not seem to be a
> large step to do the same with ofdata.
> 
> Make the change and update the documentation in this area.
> 
> Signed-off-by: Simon Glass <sjg at chromium.org>
Hi Simon

This patch fixed Arria 10 boot up issue in SPL previously due to commit 82de42fa1468.
It able boot uboot now.

Tested-by: Ley Foon Tan <ley.foon.tan at intel.com>

Thanks.

Regards
Ley Foon
Tom Rini April 6, 2020, 12:17 p.m. UTC | #2 
On Sun, Apr 05, 2020 at 03:38:19PM -0600, Simon Glass wrote:

> At present a device can read its ofdata before its parent has done the
> same. This can cause problems in the case where the parent has a 'ranges'
> property, thus affecting the operation of dev_read_addr(), for example.
> 
> We already probe parent devices before children so it does not seem to be
> a large step to do the same with ofdata.
> 
> Make the change and update the documentation in this area.
> 
> Signed-off-by: Simon Glass <sjg at chromium.org>

Is this intended for the current release, or part of the changes you had
been thinking about for the next release?  Also, with this change should
we undo other changes that had adapted to 82de42fa1468 ?  Thanks!
Simon Glass April 6, 2020, 1:41 p.m. UTC | #3 
Hi Tom,

On Mon, 6 Apr 2020 at 06:17, Tom Rini <trini at konsulko.com> wrote:
>
> On Sun, Apr 05, 2020 at 03:38:19PM -0600, Simon Glass wrote:
>
> > At present a device can read its ofdata before its parent has done the
> > same. This can cause problems in the case where the parent has a 'ranges'
> > property, thus affecting the operation of dev_read_addr(), for example.
> >
> > We already probe parent devices before children so it does not seem to be
> > a large step to do the same with ofdata.
> >
> > Make the change and update the documentation in this area.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
>
> Is this intended for the current release, or part of the changes you had
> been thinking about for the next release?  Also, with this change should
> we undo other changes that had adapted to 82de42fa1468 ?  Thanks!

I am not keen on adding this into the current release as it might
break other things. I have been trying to think of what problems it
could cause, but it is hard to be sure.

We can bring it in as soon as the merge window opens and then undo
those changes that are not needed.

Regards,
Simon
Simon Glass April 10, 2020, 7:31 p.m. UTC | #4 
Hi Tom,

On Mon, 6 Apr 2020 at 06:17, Tom Rini <trini at konsulko.com> wrote:
>
> On Sun, Apr 05, 2020 at 03:38:19PM -0600, Simon Glass wrote:
>
> > At present a device can read its ofdata before its parent has done the
> > same. This can cause problems in the case where the parent has a 'ranges'
> > property, thus affecting the operation of dev_read_addr(), for example.
> >
> > We already probe parent devices before children so it does not seem to be
> > a large step to do the same with ofdata.
> >
> > Make the change and update the documentation in this area.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
>
> Is this intended for the current release, or part of the changes you had
> been thinking about for the next release?  Also, with this change should
> we undo other changes that had adapted to 82de42fa1468 ?  Thanks!

I am not keen on adding this into the current release as it might
break other things. I have been trying to think of what problems it
could cause, but it is hard to be sure.

We can bring it in as soon as the merge window opens and then undo
those changes that are not needed.

Regards,
Simon

Applied to u-boot-dm/next, thanks!
diff mbox series

Patch

diff --git a/doc/driver-model/design.rst b/doc/driver-model/design.rst
index 5247ecc2768..f8319ba62b9 100644
--- a/doc/driver-model/design.rst
+++ b/doc/driver-model/design.rst
@@ -683,11 +683,17 @@  probe/remove which is independent of bind/unbind. This is partly because in
 U-Boot it may be expensive to probe devices and we don't want to do it until
 they are needed, or perhaps until after relocation.
 
-Activation/probe
-^^^^^^^^^^^^^^^^
+Reading ofdata
+^^^^^^^^^^^^^^
+
+Most devices have data in the device tree which they can read to find out the
+base address of hardware registers and parameters relating to driver
+operation. This is called 'ofdata' (Open-Firmware data).
 
-When a device needs to be used, U-Boot activates it, by following these
-steps (see device_probe()):
+The device's_ofdata_to_platdata() implemnents allocation and reading of
+platdata. A parent's ofdata is always read before a child.
+
+The steps are:
 
    1. If priv_auto_alloc_size is non-zero, then the device-private space
    is allocated for the device and zeroed. It will be accessible as
@@ -713,32 +719,72 @@  steps (see device_probe()):
    space. The controller can hold information about the USB state of each
    of its children.
 
-   5. All parent devices are probed. It is not possible to activate a device
+   5. If the driver provides an ofdata_to_platdata() method, then this is
+   called to convert the device tree data into platform data. This should
+   do various calls like dev_read_u32(dev, ...) to access the node and store
+   the resulting information into dev->platdata. After this point, the device
+   works the same way whether it was bound using a device tree node or
+   U_BOOT_DEVICE() structure. In either case, the platform data is now stored
+   in the platdata structure. Typically you will use the
+   platdata_auto_alloc_size feature to specify the size of the platform data
+   structure, and U-Boot will automatically allocate and zero it for you before
+   entry to ofdata_to_platdata(). But if not, you can allocate it yourself in
+   ofdata_to_platdata(). Note that it is preferable to do all the device tree
+   decoding in ofdata_to_platdata() rather than in probe(). (Apart from the
+   ugliness of mixing configuration and run-time data, one day it is possible
+   that U-Boot will cache platform data for devices which are regularly
+   de/activated).
+
+   5. The device is marked 'platdata valid'.
+
+Note that ofdata reading is always done (for a child and all its parents)
+before probing starts. Thus devices go through two distinct states when
+probing: reading platform data and actually touching the hardware to bring
+the device up.
+
+Having probing separate from ofdata-reading helps deal with of-platdata, where
+the probe() method is common to both DT/of-platdata operation, but the
+ofdata_to_platdata() method is implemented differently.
+
+Another case has come up where this separate is useful. Generation of ACPI
+tables uses the of-platdata but does not want to probe the device. Probing
+would cause U-Boot to violate one of its design principles, viz that it
+should only probe devices that are used. For ACPI we want to generate a
+table for each device, even if U-Boot does not use it. In fact it may not
+even be possible to probe the device - e.g. an SD card which is not
+present will cause an error on probe, yet we still must tell Linux about
+the SD card connector in case it is used while Linux is running.
+
+It is important that the ofdata_to_platdata() method does not actually probe
+the device itself. However there are cases where other devices must be probed
+in the ofdata_to_platdata() method. An example is where a device requires a
+GPIO for it to operate. To select a GPIO obviously requires that the GPIO
+device is probed. This is OK when used by common, core devices such as GPIO,
+clock, interrupts, reset and the like.
+
+If your device relies on its parent setting up a suitable address space, so
+that dev_read_addr() works correctly, then make sure that the parent device
+has its setup code in ofdata_to_platdata(). If it has it in the probe method,
+then you cannot call dev_read_addr() from the child device's
+ofdata_to_platdata() method. Move it to probe() instead. Buses like PCI can
+fall afoul of this rule.
+
+Activation/probe
+^^^^^^^^^^^^^^^^
+
+When a device needs to be used, U-Boot activates it, by first reading ofdata
+as above and then following these steps (see device_probe()):
+
+   1. All parent devices are probed. It is not possible to activate a device
    unless its predecessors (all the way up to the root device) are activated.
    This means (for example) that an I2C driver will require that its bus
    be activated.
 
-   6. The device's sequence number is assigned, either the requested one
+   2. The device's sequence number is assigned, either the requested one
    (assuming no conflicts) or the next available one if there is a conflict
    or nothing particular is requested.
 
-   7. If the driver provides an ofdata_to_platdata() method, then this is
-   called to convert the device tree data into platform data. This should
-   do various calls like fdtdec_get_int(gd->fdt_blob, dev_of_offset(dev), ...)
-   to access the node and store the resulting information into dev->platdata.
-   After this point, the device works the same way whether it was bound
-   using a device tree node or U_BOOT_DEVICE() structure. In either case,
-   the platform data is now stored in the platdata structure. Typically you
-   will use the platdata_auto_alloc_size feature to specify the size of the
-   platform data structure, and U-Boot will automatically allocate and zero
-   it for you before entry to ofdata_to_platdata(). But if not, you can
-   allocate it yourself in ofdata_to_platdata(). Note that it is preferable
-   to do all the device tree decoding in ofdata_to_platdata() rather than
-   in probe(). (Apart from the ugliness of mixing configuration and run-time
-   data, one day it is possible that U-Boot will cache platform data for
-   devices which are regularly de/activated).
-
-   8. The device's probe() method is called. This should do anything that
+   4. The device's probe() method is called. This should do anything that
    is required by the device to get it going. This could include checking
    that the hardware is actually present, setting up clocks for the
    hardware and setting up hardware registers to initial values. The code
@@ -753,7 +799,7 @@  steps (see device_probe()):
    allocate the priv space here yourself. The same applies also to
    platdata_auto_alloc_size. Remember to free them in the remove() method.
 
-   9. The device is marked 'activated'
+   5. The device is marked 'activated'
 
    10. The uclass's post_probe() method is called, if one exists. This may
    cause the uclass to do some housekeeping to record the device as
diff --git a/drivers/core/device.c b/drivers/core/device.c
index 89ea820d487..32d518da373 100644
--- a/drivers/core/device.c
+++ b/drivers/core/device.c
@@ -323,6 +323,22 @@  int device_ofdata_to_platdata(struct udevice *dev)
 	if (dev->flags & DM_FLAG_PLATDATA_VALID)
 		return 0;
 
+	/* Ensure all parents have ofdata */
+	if (dev->parent) {
+		ret = device_ofdata_to_platdata(dev->parent);
+		if (ret)
+			goto fail;
+
+		/*
+		 * The device might have already been probed during
+		 * the call to device_probe() on its parent device
+		 * (e.g. PCI bridge devices). Test the flags again
+		 * so that we don't mess up the device.
+		 */
+		if (dev->flags & DM_FLAG_PLATDATA_VALID)
+			return 0;
+	}
+
 	drv = dev->driver;
 	assert(drv);
 
diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c
index 75ae08081cd..73b318d604c 100644
--- a/test/dm/test-fdt.c
+++ b/test/dm/test-fdt.c
@@ -953,3 +953,28 @@  static int dm_test_first_child_probe(struct unit_test_state *uts)
 	return 0;
 }
 DM_TEST(dm_test_first_child_probe, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
+
+/* Test that ofdata is read for parents before children */
+static int dm_test_ofdata_order(struct unit_test_state *uts)
+{
+	struct udevice *bus, *dev;
+
+	ut_assertok(uclass_find_first_device(UCLASS_I2C, &bus));
+	ut_assertnonnull(bus);
+	ut_assert(!(bus->flags & DM_FLAG_PLATDATA_VALID));
+
+	ut_assertok(device_find_first_child(bus, &dev));
+	ut_assertnonnull(dev);
+	ut_assert(!(dev->flags & DM_FLAG_PLATDATA_VALID));
+
+	/* read the child's ofdata which should cause the parent's to be read */
+	ut_assertok(device_ofdata_to_platdata(dev));
+	ut_assert(dev->flags & DM_FLAG_PLATDATA_VALID);
+	ut_assert(bus->flags & DM_FLAG_PLATDATA_VALID);
+
+	ut_assert(!(dev->flags & DM_FLAG_ACTIVATED));
+	ut_assert(!(bus->flags & DM_FLAG_ACTIVATED));
+
+	return 0;
+}
+DM_TEST(dm_test_ofdata_order, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);