Re: [PATCH V3 3/3] mailbox: qcom-apcs: Add SM6125 compatible

[Bjorn Andersson]

On Mon 21 Jun 20:00 CDT 2021, Jassi Brar wrote:

> On Mon, Jun 21, 2021 at 6:35 PM Bjorn Andersson
> <bjorn.andersson@xxxxxxxxxx> wrote:
> >
> > On Mon 21 Jun 18:19 CDT 2021, Rob Herring wrote:
> >
> > > On Mon, Jun 21, 2021 at 5:10 PM Jassi Brar <jassisinghbrar@xxxxxxxxx> wrote:
> > > >
> > > > On Mon, Jun 21, 2021 at 2:46 PM Rob Herring <robh+dt@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Sun, Jun 20, 2021 at 10:03 PM Jassi Brar <jassisinghbrar@xxxxxxxxx> wrote:
> > > > > >
> > > > > > On Sat, Jun 12, 2021 at 4:46 AM Martin Botka
> > > > > > <martin.botka@xxxxxxxxxxxxxx> wrote:
> > > > > > >
> > > > > > > This commit adds compatible for the SM6125 SoC
> > > > > > >
> > > > > > > Signed-off-by: Martin Botka <martin.botka@xxxxxxxxxxxxxx>
> > > > > > > ---
> > > > > > > Changes in V2:
> > > > > > > None
> > > > > > > Changes in V3:
> > > > > > > Change compatible to apcs-hmss-global
> > > > > > >  drivers/mailbox/qcom-apcs-ipc-mailbox.c | 5 +++++
> > > > > > >  1 file changed, 5 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
> > > > > > > index f25324d03842..f24c5ad8d658 100644
> > > > > > > --- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
> > > > > > > +++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
> > > > > > > @@ -57,6 +57,10 @@ static const struct qcom_apcs_ipc_data sdm660_apcs_data = {
> > > > > > >         .offset = 8, .clk_name = NULL
> > > > > > >  };
> > > > > > >
> > > > > > > +static const struct qcom_apcs_ipc_data sm6125_apcs_data = {
> > > > > > > +       .offset = 8, .clk_name = NULL
> > > > > > > +};
> > > > > > > +
> > > > > > >  static const struct qcom_apcs_ipc_data apps_shared_apcs_data = {
> > > > > > >         .offset = 12, .clk_name = NULL
> > > > > > >  };
> > > > > > > @@ -166,6 +170,7 @@ static const struct of_device_id qcom_apcs_ipc_of_match[] = {
> > > > > > >         { .compatible = "qcom,sc8180x-apss-shared", .data = &apps_shared_apcs_data },
> > > > > > >         { .compatible = "qcom,sdm660-apcs-hmss-global", .data = &sdm660_apcs_data },
> > > > > > >         { .compatible = "qcom,sdm845-apss-shared", .data = &apps_shared_apcs_data },
> > > > > > > +       { .compatible = "qcom,sm6125-apcs-hmss-global", .data = &sm6125_apcs_data },
> > > > > > >         { .compatible = "qcom,sm8150-apss-shared", .data = &apps_shared_apcs_data },
> > > > > > >         { .compatible = "qcom,sdx55-apcs-gcc", .data = &sdx55_apcs_data },
> > > > > > >         {}
> > > > > > >
> > > > > > These all are basically different names for the same controller.
> > > > > > The 'offset' is a configuration parameter and the 'clock', when NULL,
> > > > > > is basically some "always-on" clock.
> > > > > > I am sure we wouldn't be doing it, if the controller was third-party.
> > > > >
> > > > > If newer implementations are 'the same', then they should have a
> > > > > fallback compatible to the existing one that is the same and no driver
> > > > > change is needed. If the differences are board or instance (within an
> > > > > SoC) specific, then a DT property would be appropriate.
> > > > >
> > > > The controllers (13 now) only differ by the 'offset' where the
> > > > registers are mapped. Clock-name is a pure s/w artifact.
> > > > So, maybe we could push all these in DT.
> > >
> > > Why is 'reg' not used for the offset?
> > >
> >
> > The DT node and its "reg" describes the whole IP block.
> >
> > The particular register that we care of has, as you can see, moved
> > around during the various platforms and some incarnations of this IP
> > block provides controls for CPU-related clocks as well.
> >
> > We can certainly have the multiple compatible points to the same
> > apcs_data, but I'm not able to spot a reasonable "catch-all compatible"
> > given that I don't see any natural groupings.
> >
> Any platform that comes later may reuse the already available compatible.
> For example drop this patch and reuse "qcom,sdm660-apcs-hmss-global" ?
> 

The problem is that this would change the meaning of
"qcom,sdm660-apcs-hmss-global" from meaning "The apcs hmss global block
_in_ sdm660" to "any random apcs block with the mailbox register at
offset 8".

Using a compatible that actually describes the specific IP block allows
us to add more implementation details to the existing binding after the
fact, like we had to do in c815d769b598 ("mailbox: qcom: Create APCS
child device for clock controller")


So what we typically do between Qualcomm platforms is to come up with
some common compatible that really states the equivalent of "a generic
apcs with mailbox register at offset 8", we specify both the specific
and the generic in DT and we implement only the generic - until the day
when we need the specific.

But we need to figure out what the common descriptor is for these
various groups of apcs blocks for that to work out.

> > > In any case, we can't really get rid of the first 13 instances though...
> > >
> >
> > Right, we have the problem that we have DTBs out there that relies on
> > these compatibles, but as Jassi requests we'd have to start describing
> > the internal register layout in DT - which this binding purposefully
> > avoids.
> >
> Not these strings, but 'offset' and 'clock-name' as optional
> properties that new platforms can use.
> 

Relying on completely generic compatibles to match the driver and then
distinguish each platform using additional properties is exactly what
Qualcomm does downstream.  The community has clarified countless times
that this is not the way to write DT bindings.

That said, I certainly dislike the current situation, because bringing a
new Qualcomm platform upstream means adding a couple of drivers (clocks,
pinctl etc) and then it's a large amount of oneliners throughout the
various drivers just adding compatibles in the right palces.

Regards,
Bjorn