Re: [PATCH v3 1/9] dt-bindings: reset: Add a binding for the RPi Firmware reset controller

[Nicolas Saenz Julienne <nsaenzjulienne@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 3035 bytes --]

On Mon, 2020-07-13 at 12:23 -0600, Rob Herring wrote:
> On Fri, Jun 12, 2020 at 07:13:25PM +0200, Nicolas Saenz Julienne wrote:
> > The firmware running on the RPi VideoCore can be used to reset and
> > initialize HW controlled by the firmware.
> > 
> > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> > 
> > ---
> > Changes since v2:
> >  - Add include file for reset IDs
> > 
> > Changes since v1:
> >  - Correct cells binding as per Florian's comment
> >  - Change compatible string to be more generic
> > 
> >  .../arm/bcm/raspberrypi,bcm2835-firmware.yaml | 21 +++++++++++++++++++
> >  .../reset/raspberrypi,firmware-reset.h        | 13 ++++++++++++
> >  2 files changed, 34 insertions(+)
> >  create mode 100644 include/dt-bindings/reset/raspberrypi,firmware-reset.h
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-
> > firmware.yaml
> > b/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-
> > firmware.yaml
> > index b48ed875eb8e..23a885af3a28 100644
> > --- a/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-
> > firmware.yaml
> > +++ b/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-
> > firmware.yaml
> > @@ -39,6 +39,22 @@ properties:
> >        - compatible
> >        - "#clock-cells"
> >  
> > +  reset:
> 
> I'm not really thrilled how this is evolving with a node per provider. 
> There's no reason you can't just add #clock-cells and #reset-cells to 
> the parent firmware node.

What are the downsides? The way I see it there is not much difference. And this
way of handling things is feels more intuitive and flexible (overlays can
control what to enable easily, we can take advantage of the platform device
core).

> I probably should have complained with the clocks node, but that's only 
> pending for 5.9.

Note that there are more users for this pattern: "raspberrypi,firmware-ts" and
"raspberrypi,firmware-gpio". Actually you were the one to originally propose
this it[1]. :P

There already is a fair amount of churn in these drivers because of all the DT
changes we did in the past, and if we need to change how we integrate these
again, I'd really like it to be for good.

> The bigger issue is this stuff is just trickling in one bit at a time 
> which gives no context for review. What's next? Is it really a mystery 
> as to what functions the firmware provides?

We have no control over it, RPi engineers integrate new designs and new
firmware interfaces show up. This is a good example of it.

I proposed them to use SCMI as it covers most of what they are already
providing here. But no luck so far.

> You don't have to have a driver in place for every function.

I see your point, it could be more monolithic, that said, having a driver is
essential. See the reverts I managed to pull off at the end of the series.

[1] https://patchwork.kernel.org/patch/10166783/#21421571


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]