Hi

On Fri, Jul 03, 2020 at 05:47:08PM +0200, Andrzej Hajda wrote:

On 30.06.2020 15:27, Maxime Ripard wrote:

...

I've tried to bring-up the DSI controller on the RaspberryPi4, and I've just encountered something that could make it troublesome to support.

For context, the RaspberryPi has an official panel that uses a DSI->DPI bridge, a DPI panel, a touchscreen and a small micro-controller. That microcontroller controls the power management on the screen, so communicating with it is very much needed, and it's done through an i2c bus.

To reflect that, the entire panel has been described in the Device Tree as an I2C device (since that's how you would control it), together with an OF-Graph endpoint linking that i2c device to the DSI controller[1].

That deviates a bit from the generic DSI binding though[2], since it requires that the panel should be a subnode of the DSI controller (which also makes sense since DCS commands is usually how you would control that device).

This is where the trouble begins. Since the two devices are on entirely different buses, there's basically no guarantee on the probe order. The driver has tried to address this by using the OF-Graph and the component framework. Indeed, the DSI driver (component-based) will register a MIPI-DSI host in its probe, and call component_add[3]. If component_add fails, it will remove the DSI host and return the error code. It makes sense.

The panel on the other hand will probe, and look for a DSI host through the OF-Graph [4]. If it isn't there, it will return EPROBE_DEFER, hoping that it will be available at some point. It also makes complete sense.

Where the issue lies is that component_add has two very different behaviours here that will create the bug that we see on the RPi4:

• If there's still components to probe, component_add will simply return 0 [5][6]

• And if we're the last component to probe, component_add will then run all the bind callbacks and return the result on error of the master bind callback[7]. That master bind will usually have component_bind_all that will return the result of the bind callback of each component.

Now, on the RPi4, the last component to probe is the DSI controller since it relies on a power-domain exposed by the firmware driver, so the driver core will defer its probe function until the power-domain is there [8]. We're thus pretty much guaranteed to fall in the second case above and run through all the bind callbacks. The DSI bind callback however will try to find and attach its panel, and return EPROBE_DEFER if it doesn't find it[9]. That error will then be propagated to the return code of component_bind_all, then to the master bind callback, and finally will be the return code of component_add.

And since component_add is failing, we remove the DSI host. Since the DSI host isn't there, on the next occasion the i2c panel driver will not probe either, and we enter a loop that cannot really be broken, since one depends on the other.

This was working on the RPi3 because the DSI is not the last driver to probe: indeed the v3d is depending on the same power domain[10][11] and is further down the list of components to add in the driver [12], so we're always in the first component_add case for DSI above, the DSI host sticks around, and the i2c driver can probe.

I'm not entirely sure how we can fix that though. I guess the real flaw here is the assumption that component_add will not fail if one of the bind fails, which isn't true, but we can't really ignore those errors either since it might be something else than DSI that returns that error.

One way to work around it is to make the mailbox, firmware and power domain drivers probe earlier by tweaking the initcalls at which they register, but it's not really fixing anything and compiling them as module would make it broken again.

Forgive me - I have not read whole post, but I hope you have a problem already solved.

As I understand you have:

1. Componentized DSI-host.

2. Some sink laying on DSI bus.

General rule I promote: "do not expose functionality, until you have all dependencies", in this case it would be "do not call component_add until you know sink(your dependency) is ready".

Already tested solution (to be checked in drivers):

1. In DSI-host you register dsi bus in probe, but do not call component_add.

2. In DSI-host callback informing about DSI device registration you get the

sink and since you have all resources then you call component_add.

That's a great idea :)

I just tested and it works, so it ended up to much easier to fix than I anticipated :)

Thanks! Maxime