[U-Boot] [PATCH v2 1/3] drivers: Add board uclass

[Simon Glass]

Hi Philipp,

[somehow the quoting is messed up here - so it's hard to distinguish
my response from yours.

On 1 May 2018 at 07:14, Dr. Philipp Tomsich
<philipp.tomsich at theobroma-systems.com> wrote:
> Simon,
>
> On 1 May 2018, at 01:12, Simon Glass <sjg at chromium.org> wrote:
>
> Hi,
>
> On 27 April 2018 at 07:02, Dr. Philipp Tomsich
> <philipp.tomsich at theobroma-systems.com> wrote:
>
>
> On 27 Apr 2018, at 14:51, Mario Six <mario.six at gdsys.cc> wrote:
>
> Since there is no canonical "board device" that can be used in board
> files, it is difficult to use DM function for board initialization in
> these cases.
>
> Hence, add a uclass that implements a simple "board device", which can
> hold devices not suitable anywhere else in the device tree, and is also
> able to read encoded information, e.g. hard-wired GPIOs on a GPIO
> expander, read-only memory ICs, etc. that carry information about the
> hardware.
>
>
> With comments below:
>
> Reviewed-by: Simon Glass <sjg at chromium.org>
>
>
> A board-uclass should model a board (i.e. provide implementations for
> the same key abstractions that we have in place today: e.g., board_init).
>
> This seems more like a specialised form of a misc-device.
>
> The devices of this uclass expose methods to read generic data types
> (integers, strings, booleans) to encode the information provided by the
> hardware.
>
>
> Again, this is what a misc-device is intended for… and extending the
> misc-device APIs (or having a convenience layer on top of its ioctl
> interface?) might be more appropriate.

We could certainly use a misc device, but I feel that a board device
is a better abstraction for lots of reasons. It is the board that
collects together all the different devices that U-Boot has access to.

To me a misc device is useful as a parent device to collect a few
things together, perhaps a GPIO expander which has a clock output. But
using a misc device to model a board seems like a short-term strategy.

Addressing your idea about implementing the board init in this uclass,
yes I think that would be a good idea. In fact I did a series for that
some time ago:

https://lists.denx.de/pipermail/u-boot/2017-March/284086.html

But, baby steps.

>
> After reviewing the below, the similarities to the misc-device are even
> more apparent: if you need typed access to a key-value pair, this can
> be easily implemented through a common ioctl with semantic sugar
> at the misc-uclass level.
>
>
> A misc device might be similar in action but it is not the same in
> terms of concept. For example it would be very strange to have a misc
> device which points to lots of other devices that are on the board.
> For example, consider this hypothetical case:
>
> board {
>   identity-eeprom = <&eeprom>;
>   id-gpios = <&gpio0 2>, <&gpio0 4>;
>   ec = <&cros_ec>;
> };
>
>
> Maybe I am getting hung up on the ‘board’ name, as in my mind the board
> selection should be based on the top-level .compatible:
> e.g. compatible = "tsd,rk3399-q7", "tsd,puma", "rockchip,rk3399";
> and the selected board-classes could then encapsulate the various init
> hooks and inquire into devices defined via /aliases, /config and /chosen.
>
> A board-class should restructure existing board.c-files into the .ops that
> are used there (i.e. init, late-init, usb-init, debug-uart-init, etc.) ...
> this is
> different from what’s being proposed here.

See above. I agree the compatible string should be used.

>
> The hypothetical case you give is already covered wholly by today’s device
> model. Let me explain with a counter example:
> aliases {
> identity-eeprom = <&eeprom>;
> id-gpios = <&board-id>;
> ec = <&cros_ec>;
> }
>
> board-id {
> /* a good use for a hypothetical misc-device that exports the
>    values of a variable-length list of GPIOs */
> .compatible = “identity-gpios”;
> id-gpios = <&gpio0 2>, <&gpio0 4>;
> }

Yes, agreed, that will work. But I want to push this a bit further and
I think a board uclass makes sense overall.

>
> Here we point to a few devices which may otherwise require specific
> device-tree references to locate. Already we are seeing this sort of
> code in U-Boot:
>
> /* find the grf node */
> node = fdt_node_offset_by_compatible(blob, -1,
> "rockchip,rk3288-grf”);
>
>
> This is usually caused by badly-modelled device-trees or missing device
> tree support in drivers.
> In this specific case it’s a wart caused by dwc2_udc_probe(…) and that
> this is not called from the device-tree.
>
> I would hope that this will eventually be included converted to the device
> model and that the info will be processed from the dwc2_udc driver.
>
> ret = regulator_get_by_platname("vdd_arm", &dev);
> if (ret) {
> debug("Cannot set regulator name\n");
> return ret;
> }
>
>
> I know this specific example, and it’s from a use case, where a voltage
> needs to be slowly raised.  The reason for this being there is that we
> don’t have the same DTS as in Linux and the modelling (and driver)
> for the PMU subsystem are not there.
>
> So I don’t seem to follow on how this would resolve itself with the
> proposed board-uclass.
>
> These are the sorts of things which suggest that some sort of 'board
> directory', pointing to the things that the board needs to init, would
> be useful.
>
>
> In fact, I have been raising the topic of having a ‘board’ and a ‘soc’
> uclass a few weeks back in order to structure the boot-up through
> the device-model.
>
> This is exactly where my comments are coming from: the proposed
> board uclass (as in this patch-series) does not aim to break up the
> existing board-files and put them into a framework of .ops in a new
> board-uclass.
>
> In the current code-base I see cases like
>
> int rk3288_qos_init(void)
> {
>         int val = 2 << PRIORITY_HIGH_SHIFT | 2 << PRIORITY_LOW_SHIFT;
>         /* set vop qos to higher priority */
>         writel(val, CPU_AXI_QOS_PRIORITY + VIO0_VOP_QOS);
>         writel(val, CPU_AXI_QOS_PRIORITY + VIO1_VOP_QOS);
>
>         if (!fdt_node_check_compatible(gd->fdt_blob, 0,
>                                        "rockchip,rk3288-tinker"))
>         {
>                 /* set isp qos to higher priority */
>                 writel(val, CPU_AXI_QOS_PRIORITY + VIO1_ISP_R_QOS);
>                 writel(val, CPU_AXI_QOS_PRIORITY + VIO1_ISP_W0_QOS);
>                 writel(val, CPU_AXI_QOS_PRIORITY + VIO1_ISP_W1_QOS);
>         }
>         return 0;
> }
>
> which clearly requires a multi-step init-process for a
> soc->init(...)
> board->init(...)
> with both derived from
> compatible = "rockchip,rk3288-tinker", "rockchip,rk3288";
>
> Existing entry points like the debug_uart_init, etc. should get their own
> .ops-entry …

Yes, I agree that would be a good thing to do. See my series referenced above.

[..]

> + * to read the serial number from the device.
> + */
> +
> +struct board_ops {
> +     /**
> +      * detect() - Run the hardware info detection procedure for this
> +      *            device.
> +      *
> +      * @dev:      The device containing the information
> +      * @return 0 if OK, -ve on error.
> +      */
> +     int (*detect)(struct udevice *dev);
>
>
> This doesn’t make any sense, as the probe entry-point is intended to
> probe for a device.
>
>
> I can see your point, but I feel that it does make some sense.
>
> Probing the board device could be used to simply locate all the
> dependent devices and set them up in a data structure. This might be a
> fairly fast operation.
>
> Detecting the hardware could take a lot longer:
> - reading from an EEPROM
> - calling out to an EC to ask for details
> - checking for the present of a device on an I2C device
>
> I don't like the idea of probe() taking a really long time (e.g.
> 50ms). It should be possible to get access to a device without
> incurring that delay. This way, we can access the devices without
> necessarily knowing exactly what type of board it is.
>
> The comment above definitely needs expanding to explain why this
> doesn't happen in probe()]
>
>
> Maybe I have been misreading the device-model all along, but I
> thought this was the difference between the bind and the probe
> operations:
>
>  * @bind: Called to bind a device to its driver
>  * @probe: Called to probe a device, i.e. activate it
>
> So the bind should be blindingly fast, whereas the probe should
> activate the device and thus might be slower (if activating the
> device might take longer).

That's right, but even so I want most devices to probe quickly where
possible. For example an I2C bus does not scan itself in probe(). A
display port peripheral does not start up the display in probe().
There is clearly a need in some cases to separate probe() from
enable() or whatever you call it. Otherwise we risk breaking the
'lazy-init' requirement of U-Boot. Probing all the devices in the
system should not take a long time.

Regards,
Simon