[PATCH V3 1/3] socinfo: Add missing SoC ID for SM6125

[PATCH V3 1/3] socinfo: Add missing SoC ID for SM6125

[Martin Botka]

Add SM6125 SoC ID to the soc_id struct

Signed-off-by: Martin Botka <martin.botka@somainline.org>
---
Changes in V2:
Add commit description
Changes in V3:
None
 drivers/soc/qcom/socinfo.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
index f6cfb79338f0..c52145e92f03 100644
--- a/drivers/soc/qcom/socinfo.c
+++ b/drivers/soc/qcom/socinfo.c
@@ -254,6 +254,7 @@ static const struct soc_id soc_id[] = {
 	{ 350, "SDA632" },
 	{ 351, "SDA450" },
 	{ 356, "SM8250" },
+	{ 394, "SM6125" },
 	{ 402, "IPQ6018" },
 	{ 425, "SC7180" },
 	{ 455, "QRB5165" },
-- 
2.31.1

[PATCH V3 2/3] dt-bindings: mailbox: Add binding for sm6125

[Martin Botka]

This patch adds the binding for sm6125

Signed-off-by: Martin Botka <martin.botka@somainline.org>
---
Changes in V3:
Add this file
 .../devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml      | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
index 5dc1173d03fd..9f4fbc6a141c 100644
--- a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
+++ b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml
@@ -27,6 +27,7 @@ properties:
       - qcom,sc8180x-apss-shared
       - qcom,sdm660-apcs-hmss-global
       - qcom,sdm845-apss-shared
+      - qcom,sm6125-apcs-hmss-global
       - qcom,sm8150-apss-shared
 
   reg:
@@ -75,6 +76,7 @@ allOf:
             - qcom,sc7180-apss-shared
             - qcom,sdm660-apcs-hmss-global
             - qcom,sdm845-apss-shared
+            - qcom,sm6125-apcs-hmss-global
             - qcom,sm8150-apss-shared
     then:
       properties:
-- 
2.31.1

Re: [PATCH V3 2/3] dt-bindings: mailbox: Add binding for sm6125

[Rob Herring]

On Sat, 12 Jun 2021 11:46:29 +0200, Martin Botka wrote:
> This patch adds the binding for sm6125
> 
> Signed-off-by: Martin Botka <martin.botka@somainline.org>
> ---
> Changes in V3:
> Add this file
>  .../devicetree/bindings/mailbox/qcom,apcs-kpss-global.yaml      | 2 ++
>  1 file changed, 2 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

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

[Martin Botka]

This commit adds compatible for the SM6125 SoC

Signed-off-by: Martin Botka <martin.botka@somainline.org>
---
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 },
 	{}
-- 
2.31.1

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

[Jassi Brar]

On Sat, Jun 12, 2021 at 4:46 AM Martin Botka
<martin.botka@somainline.org> wrote:
>
> This commit adds compatible for the SM6125 SoC
>
> Signed-off-by: Martin Botka <martin.botka@somainline.org>
> ---
> 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.

-Jassi

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

[Martin Botka]

On Sun, Jun 20 2021 at 11:03:04 PM -0500, Jassi Brar 
<jassisinghbrar@gmail.com> wrote:
> On Sat, Jun 12, 2021 at 4:46 AM Martin Botka
> <martin.botka@somainline.org> wrote:
>> 
>>  This commit adds compatible for the SM6125 SoC
>> 
>>  Signed-off-by: Martin Botka <martin.botka@somainline.org>
>>  ---
>>  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.
> 
> -Jassi

I'm aware that its not great to duplicate but i just followed
how it was done previously.

I will probably send a patch to lump the common conf together.

Best Regards,
Martin

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

[Rob Herring]

On Sun, Jun 20, 2021 at 10:03 PM Jassi Brar <jassisinghbrar@gmail.com> wrote:
>
> On Sat, Jun 12, 2021 at 4:46 AM Martin Botka
> <martin.botka@somainline.org> wrote:
> >
> > This commit adds compatible for the SM6125 SoC
> >
> > Signed-off-by: Martin Botka <martin.botka@somainline.org>
> > ---
> > 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.

3rd party IP is generally not any different. SoC vendors manage to
make their implementations unique...

Rob

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

[Jassi Brar]

On Mon, Jun 21, 2021 at 2:46 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Sun, Jun 20, 2021 at 10:03 PM Jassi Brar <jassisinghbrar@gmail.com> wrote:
> >
> > On Sat, Jun 12, 2021 at 4:46 AM Martin Botka
> > <martin.botka@somainline.org> wrote:
> > >
> > > This commit adds compatible for the SM6125 SoC
> > >
> > > Signed-off-by: Martin Botka <martin.botka@somainline.org>
> > > ---
> > > 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.

thanks.

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

[Rob Herring]

On Mon, Jun 21, 2021 at 5:10 PM Jassi Brar <jassisinghbrar@gmail.com> wrote:
>
> On Mon, Jun 21, 2021 at 2:46 PM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Sun, Jun 20, 2021 at 10:03 PM Jassi Brar <jassisinghbrar@gmail.com> wrote:
> > >
> > > On Sat, Jun 12, 2021 at 4:46 AM Martin Botka
> > > <martin.botka@somainline.org> wrote:
> > > >
> > > > This commit adds compatible for the SM6125 SoC
> > > >
> > > > Signed-off-by: Martin Botka <martin.botka@somainline.org>
> > > > ---
> > > > 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?

In any case, we can't really get rid of the first 13 instances though...

Rob

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

[Bjorn Andersson]

On Mon 21 Jun 18:19 CDT 2021, Rob Herring wrote:

> On Mon, Jun 21, 2021 at 5:10 PM Jassi Brar <jassisinghbrar@gmail.com> wrote:
> >
> > On Mon, Jun 21, 2021 at 2:46 PM Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > On Sun, Jun 20, 2021 at 10:03 PM Jassi Brar <jassisinghbrar@gmail.com> wrote:
> > > >
> > > > On Sat, Jun 12, 2021 at 4:46 AM Martin Botka
> > > > <martin.botka@somainline.org> wrote:
> > > > >
> > > > > This commit adds compatible for the SM6125 SoC
> > > > >
> > > > > Signed-off-by: Martin Botka <martin.botka@somainline.org>
> > > > > ---
> > > > > 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.

> 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.

Regards,
Bjorn

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

[Jassi Brar]

On Mon, Jun 21, 2021 at 6:35 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Mon 21 Jun 18:19 CDT 2021, Rob Herring wrote:
>
> > On Mon, Jun 21, 2021 at 5:10 PM Jassi Brar <jassisinghbrar@gmail.com> wrote:
> > >
> > > On Mon, Jun 21, 2021 at 2:46 PM Rob Herring <robh+dt@kernel.org> wrote:
> > > >
> > > > On Sun, Jun 20, 2021 at 10:03 PM Jassi Brar <jassisinghbrar@gmail.com> wrote:
> > > > >
> > > > > On Sat, Jun 12, 2021 at 4:46 AM Martin Botka
> > > > > <martin.botka@somainline.org> wrote:
> > > > > >
> > > > > > This commit adds compatible for the SM6125 SoC
> > > > > >
> > > > > > Signed-off-by: Martin Botka <martin.botka@somainline.org>
> > > > > > ---
> > > > > > 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" ?

> > 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.

cheers.

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@linaro.org> wrote:
> >
> > On Mon 21 Jun 18:19 CDT 2021, Rob Herring wrote:
> >
> > > On Mon, Jun 21, 2021 at 5:10 PM Jassi Brar <jassisinghbrar@gmail.com> wrote:
> > > >
> > > > On Mon, Jun 21, 2021 at 2:46 PM Rob Herring <robh+dt@kernel.org> wrote:
> > > > >
> > > > > On Sun, Jun 20, 2021 at 10:03 PM Jassi Brar <jassisinghbrar@gmail.com> wrote:
> > > > > >
> > > > > > On Sat, Jun 12, 2021 at 4:46 AM Martin Botka
> > > > > > <martin.botka@somainline.org> wrote:
> > > > > > >
> > > > > > > This commit adds compatible for the SM6125 SoC
> > > > > > >
> > > > > > > Signed-off-by: Martin Botka <martin.botka@somainline.org>
> > > > > > > ---
> > > > > > > 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

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

[Jassi Brar]

On Mon, Jun 21, 2021 at 9:27 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Mon 21 Jun 20:00 CDT 2021, Jassi Brar wrote:
>
> > On Mon, Jun 21, 2021 at 6:35 PM Bjorn Andersson
> > <bjorn.andersson@linaro.org> wrote:
> > >
> > > On Mon 21 Jun 18:19 CDT 2021, Rob Herring wrote:
> > >
> > > > On Mon, Jun 21, 2021 at 5:10 PM Jassi Brar <jassisinghbrar@gmail.com> wrote:
> > > > >
> > > > > On Mon, Jun 21, 2021 at 2:46 PM Rob Herring <robh+dt@kernel.org> wrote:
> > > > > >
> > > > > > On Sun, Jun 20, 2021 at 10:03 PM Jassi Brar <jassisinghbrar@gmail.com> wrote:
> > > > > > >
> > > > > > > On Sat, Jun 12, 2021 at 4:46 AM Martin Botka
> > > > > > > <martin.botka@somainline.org> wrote:
> > > > > > > >
> > > > > > > > This commit adds compatible for the SM6125 SoC
> > > > > > > >
> > > > > > > > Signed-off-by: Martin Botka <martin.botka@somainline.org>
> > > > > > > > ---
> > > > > > > > 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".
>
To me, the deeper problem seems to be naming a controller "The apcs
hmss global block _in_ sdm660" just because the h/w manual hasn't
given a name to it.  But that is okay too, if we name the subsequent
controllers as "the same as one in sdm660" and provide the h/w
configuration 'offset' via a DT property.

> > > > 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.
>
Yes, and I don't suggest it otherwise. For h/w quirks and
extra/missing features, it does make sense to have different
compatibles.

However, for _trivial_ variations let us get that value from DT.
'offset' is anyway a h/w property.
That way we won't be distinguishing platforms using dt properties, but
only support different platforms seamlessly.

On second thought, we have grown from 2 to 13 aliases in 4 yrs. I only
have to ignore 3 times/annum to lead a peaceful life ;)

thnx.

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

[Bjorn Andersson]

On Mon 21 Jun 22:34 CDT 2021, Jassi Brar wrote:

> On Mon, Jun 21, 2021 at 9:27 PM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > On Mon 21 Jun 20:00 CDT 2021, Jassi Brar wrote:
> >
> > > On Mon, Jun 21, 2021 at 6:35 PM Bjorn Andersson
> > > <bjorn.andersson@linaro.org> wrote:
> > > >
> > > > On Mon 21 Jun 18:19 CDT 2021, Rob Herring wrote:
> > > >
> > > > > On Mon, Jun 21, 2021 at 5:10 PM Jassi Brar <jassisinghbrar@gmail.com> wrote:
> > > > > >
> > > > > > On Mon, Jun 21, 2021 at 2:46 PM Rob Herring <robh+dt@kernel.org> wrote:
> > > > > > >
> > > > > > > On Sun, Jun 20, 2021 at 10:03 PM Jassi Brar <jassisinghbrar@gmail.com> wrote:
> > > > > > > >
> > > > > > > > On Sat, Jun 12, 2021 at 4:46 AM Martin Botka
> > > > > > > > <martin.botka@somainline.org> wrote:
> > > > > > > > >
> > > > > > > > > This commit adds compatible for the SM6125 SoC
> > > > > > > > >
> > > > > > > > > Signed-off-by: Martin Botka <martin.botka@somainline.org>
> > > > > > > > > ---
> > > > > > > > > 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".
> >
> To me, the deeper problem seems to be naming a controller "The apcs
> hmss global block _in_ sdm660" just because the h/w manual hasn't
> given a name to it.  But that is okay too, if we name the subsequent
> controllers as "the same as one in sdm660" and provide the h/w
> configuration 'offset' via a DT property.
> 

As I said, I'd need to dig through the hardware documentation for the
various platforms to see if I can find what the common denominators are.
We've always seen this as "the apcs hmss global block _in_ <platform>".

> > > > > 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.
> >
> Yes, and I don't suggest it otherwise. For h/w quirks and
> extra/missing features, it does make sense to have different
> compatibles.
> 

But what you're suggesting assumes that they are the same and that we're
done implementing all the software for this block. The platform specific
compatible allows us to postpone that question.

> However, for _trivial_ variations let us get that value from DT.
> 'offset' is anyway a h/w property.
> That way we won't be distinguishing platforms using dt properties, but
> only support different platforms seamlessly.
> 

As I said previously, this goes against the direction provided by the DT
maintainers. If a property is platform specific this should be expressed
by the compatible.

> On second thought, we have grown from 2 to 13 aliases in 4 yrs. I only
> have to ignore 3 times/annum to lead a peaceful life ;)
> 

True, but I'll try to find some time to see if we have some reuse of the
IP block to allow us to use some generic compatible.

We'd still need a patch in the DT binding for every single platform, but
we should be able to avoid the compatible additions in the driver.

Regards,
Bjorn

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

[AngeloGioacchino Del Regno]

Il 22/06/21 05:52, Bjorn Andersson ha scritto:
> On Mon 21 Jun 22:34 CDT 2021, Jassi Brar wrote:
> 
>> On Mon, Jun 21, 2021 at 9:27 PM Bjorn Andersson
>> <bjorn.andersson@linaro.org> wrote:
>>>
>>> On Mon 21 Jun 20:00 CDT 2021, Jassi Brar wrote:
>>>
>>>> On Mon, Jun 21, 2021 at 6:35 PM Bjorn Andersson
>>>> <bjorn.andersson@linaro.org> wrote:
>>>>>
>>>>> On Mon 21 Jun 18:19 CDT 2021, Rob Herring wrote:
>>>>>
>>>>>> On Mon, Jun 21, 2021 at 5:10 PM Jassi Brar <jassisinghbrar@gmail.com> wrote:
>>>>>>>
>>>>>>> On Mon, Jun 21, 2021 at 2:46 PM Rob Herring <robh+dt@kernel.org> wrote:
>>>>>>>>
>>>>>>>> On Sun, Jun 20, 2021 at 10:03 PM Jassi Brar <jassisinghbrar@gmail.com> wrote:
>>>>>>>>>
>>>>>>>>> On Sat, Jun 12, 2021 at 4:46 AM Martin Botka
>>>>>>>>> <martin.botka@somainline.org> wrote:
>>>>>>>>>>
>>>>>>>>>> This commit adds compatible for the SM6125 SoC
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Martin Botka <martin.botka@somainline.org>
>>>>>>>>>> ---
>>>>>>>>>> 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".
>>>
>> To me, the deeper problem seems to be naming a controller "The apcs
>> hmss global block _in_ sdm660" just because the h/w manual hasn't
>> given a name to it.  But that is okay too, if we name the subsequent
>> controllers as "the same as one in sdm660" and provide the h/w
>> configuration 'offset' via a DT property.
>>
> 
> As I said, I'd need to dig through the hardware documentation for the
> various platforms to see if I can find what the common denominators are.
> We've always seen this as "the apcs hmss global block _in_ <platform>".
> 
>>>>>> 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.
>>>
>> Yes, and I don't suggest it otherwise. For h/w quirks and
>> extra/missing features, it does make sense to have different
>> compatibles.
>>
> 
> But what you're suggesting assumes that they are the same and that we're
> done implementing all the software for this block. The platform specific
> compatible allows us to postpone that question.
> 
>> However, for _trivial_ variations let us get that value from DT.
>> 'offset' is anyway a h/w property.
>> That way we won't be distinguishing platforms using dt properties, but
>> only support different platforms seamlessly.
>>
> 
> As I said previously, this goes against the direction provided by the DT
> maintainers. If a property is platform specific this should be expressed
> by the compatible.
> 
>> On second thought, we have grown from 2 to 13 aliases in 4 yrs. I only
>> have to ignore 3 times/annum to lead a peaceful life ;)
>>
> 
> True, but I'll try to find some time to see if we have some reuse of the
> IP block to allow us to use some generic compatible.
> 
> We'd still need a patch in the DT binding for every single platform, but
> we should be able to avoid the compatible additions in the driver.
> 

Hello Jassi, Bjorn

I've read the entire thread and I can't say that Jassi is entirely wrong
but I also agree with Bjorn on this matter.

This driver is here to "simply" manage the register offset in the APCS
IP, which is a pretty straightforward operation.
If you check in this driver, you will see that there's not much
duplication between the various qcom_apcs_ipc_data that we have for
all the different SoCs.

Checking further, we can effectively reduce the amount of compatibles
in this driver by simply removing some "duplicated" instances and in
particular:
ipq6018, ipq8074, msm8916, msm8994, msm8998, sdm660

and eventually replacing them with either of:
- 8bits_apcs_data    qcom,apcs-apps-global-8bit
                      qcom,apcs-kpss-global-8bit
- more_appropriate_name_apcs_data qcom,(...blah)

This would mean that we would have to use a generic "qcom,apcs-clk" as
the clk_name, but no other modifications would be done, apart checking
the return value to choose whether to print or not the dev_err when the
clock name is specified but not present in dt, since the driver is
already actually covering this case.

That would make us able to reduce the compatibles from 6 to 2, relative
to the aforementioned SoC specific bindings.
I'm positive that, through time, when new SoCs arrive, we would avoid
getting this compatible list to be megabytes long...

Right now it's not an issue, but since Qualcomm SoCs are now being very
actively upstreamed, I can see this coming in the future, somehow.


Of course this means that we're getting some fair amount of patch-noise
in the mailing lists, since all qcom dtsi files will have to be changed,
but that shouldn't really be a problem, I guess.

I'm sure that I'm not the only one with such a "wow-idea" in mind :)

Yours,
- Angelo

> Regards,
> Bjorn
> 

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

[Jassi Brar]

On Tue, Jun 22, 2021 at 9:36 AM AngeloGioacchino Del Regno
<angelogioacchino.delregno@somainline.org> wrote:
....
>
> Checking further, we can effectively reduce the amount of compatibles
> in this driver by simply removing some "duplicated" instances
>
.. is basically what I think.
However, since the existing compatibles can't be removed, we could at
least stop adding new, but reuse the existing ones.

cheers.

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

[Bjorn Andersson]

On Tue 22 Jun 09:36 CDT 2021, AngeloGioacchino Del Regno wrote:

> Il 22/06/21 05:52, Bjorn Andersson ha scritto:
> > On Mon 21 Jun 22:34 CDT 2021, Jassi Brar wrote:
> > 
> > > On Mon, Jun 21, 2021 at 9:27 PM Bjorn Andersson
> > > <bjorn.andersson@linaro.org> wrote:
> > > > 
> > > > On Mon 21 Jun 20:00 CDT 2021, Jassi Brar wrote:
> > > > 
> > > > > On Mon, Jun 21, 2021 at 6:35 PM Bjorn Andersson
> > > > > <bjorn.andersson@linaro.org> wrote:
> > > > > > 
> > > > > > On Mon 21 Jun 18:19 CDT 2021, Rob Herring wrote:
> > > > > > 
> > > > > > > On Mon, Jun 21, 2021 at 5:10 PM Jassi Brar <jassisinghbrar@gmail.com> wrote:
> > > > > > > > 
> > > > > > > > On Mon, Jun 21, 2021 at 2:46 PM Rob Herring <robh+dt@kernel.org> wrote:
> > > > > > > > > 
> > > > > > > > > On Sun, Jun 20, 2021 at 10:03 PM Jassi Brar <jassisinghbrar@gmail.com> wrote:
> > > > > > > > > > 
> > > > > > > > > > On Sat, Jun 12, 2021 at 4:46 AM Martin Botka
> > > > > > > > > > <martin.botka@somainline.org> wrote:
> > > > > > > > > > > 
> > > > > > > > > > > This commit adds compatible for the SM6125 SoC
> > > > > > > > > > > 
> > > > > > > > > > > Signed-off-by: Martin Botka <martin.botka@somainline.org>
> > > > > > > > > > > ---
> > > > > > > > > > > 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".
> > > > 
> > > To me, the deeper problem seems to be naming a controller "The apcs
> > > hmss global block _in_ sdm660" just because the h/w manual hasn't
> > > given a name to it.  But that is okay too, if we name the subsequent
> > > controllers as "the same as one in sdm660" and provide the h/w
> > > configuration 'offset' via a DT property.
> > > 
> > 
> > As I said, I'd need to dig through the hardware documentation for the
> > various platforms to see if I can find what the common denominators are.
> > We've always seen this as "the apcs hmss global block _in_ <platform>".
> > 
> > > > > > > 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.
> > > > 
> > > Yes, and I don't suggest it otherwise. For h/w quirks and
> > > extra/missing features, it does make sense to have different
> > > compatibles.
> > > 
> > 
> > But what you're suggesting assumes that they are the same and that we're
> > done implementing all the software for this block. The platform specific
> > compatible allows us to postpone that question.
> > 
> > > However, for _trivial_ variations let us get that value from DT.
> > > 'offset' is anyway a h/w property.
> > > That way we won't be distinguishing platforms using dt properties, but
> > > only support different platforms seamlessly.
> > > 
> > 
> > As I said previously, this goes against the direction provided by the DT
> > maintainers. If a property is platform specific this should be expressed
> > by the compatible.
> > 
> > > On second thought, we have grown from 2 to 13 aliases in 4 yrs. I only
> > > have to ignore 3 times/annum to lead a peaceful life ;)
> > > 
> > 
> > True, but I'll try to find some time to see if we have some reuse of the
> > IP block to allow us to use some generic compatible.
> > 
> > We'd still need a patch in the DT binding for every single platform, but
> > we should be able to avoid the compatible additions in the driver.
> > 
> 
> Hello Jassi, Bjorn
> 
> I've read the entire thread and I can't say that Jassi is entirely wrong
> but I also agree with Bjorn on this matter.
> 
> This driver is here to "simply" manage the register offset in the APCS
> IP, which is a pretty straightforward operation.
> If you check in this driver, you will see that there's not much
> duplication between the various qcom_apcs_ipc_data that we have for
> all the different SoCs.
> 
> Checking further, we can effectively reduce the amount of compatibles
> in this driver by simply removing some "duplicated" instances and in
> particular:
> ipq6018, ipq8074, msm8916, msm8994, msm8998, sdm660
> 
> and eventually replacing them with either of:
> - 8bits_apcs_data    qcom,apcs-apps-global-8bit
>                      qcom,apcs-kpss-global-8bit

I don't like those compatibles, simply because the binding is supposed
to describe the hardware block, not the fact that Linux _currently_ only
pokes this one register.

We could probably "qcom,apss-global" as a catch-all for at least sc7180,
sc7280, sdm845, sm8150, sm8250 and sm8350.

But look at 8996 and 8998, both named "something-hmss-something", with
different register layout. And a quick glance seems to indicate that
sdm660 isn't a hmss after all :/

But introducing qcom,apss-global should catch a bunch of the newer
platforms.


On the DT binding side we still need the platform-specific ones and we
need each one to be added to the binding regardless of the catch-all in
the driver.

Regards,
Bjorn

> - more_appropriate_name_apcs_data qcom,(...blah)
> 
> This would mean that we would have to use a generic "qcom,apcs-clk" as
> the clk_name, but no other modifications would be done, apart checking
> the return value to choose whether to print or not the dev_err when the
> clock name is specified but not present in dt, since the driver is
> already actually covering this case.
> 
> That would make us able to reduce the compatibles from 6 to 2, relative
> to the aforementioned SoC specific bindings.
> I'm positive that, through time, when new SoCs arrive, we would avoid
> getting this compatible list to be megabytes long...
> 
> Right now it's not an issue, but since Qualcomm SoCs are now being very
> actively upstreamed, I can see this coming in the future, somehow.
> 
> 
> Of course this means that we're getting some fair amount of patch-noise
> in the mailing lists, since all qcom dtsi files will have to be changed,
> but that shouldn't really be a problem, I guess.
> 
> I'm sure that I'm not the only one with such a "wow-idea" in mind :)
> 
> Yours,
> - Angelo
> 
> > Regards,
> > Bjorn
> > 
> 

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

[AngeloGioacchino Del Regno]

Il 24/06/21 23:07, Bjorn Andersson ha scritto:
> On Tue 22 Jun 09:36 CDT 2021, AngeloGioacchino Del Regno wrote:
> 
>> Il 22/06/21 05:52, Bjorn Andersson ha scritto:
>>> On Mon 21 Jun 22:34 CDT 2021, Jassi Brar wrote:
>>>
>>>> On Mon, Jun 21, 2021 at 9:27 PM Bjorn Andersson
>>>> <bjorn.andersson@linaro.org> wrote:
>>>>>
>>>>> On Mon 21 Jun 20:00 CDT 2021, Jassi Brar wrote:
>>>>>
>>>>>> On Mon, Jun 21, 2021 at 6:35 PM Bjorn Andersson
>>>>>> <bjorn.andersson@linaro.org> wrote:
>>>>>>>
>>>>>>> On Mon 21 Jun 18:19 CDT 2021, Rob Herring wrote:
>>>>>>>
>>>>>>>> On Mon, Jun 21, 2021 at 5:10 PM Jassi Brar <jassisinghbrar@gmail.com> wrote:
>>>>>>>>>
>>>>>>>>> On Mon, Jun 21, 2021 at 2:46 PM Rob Herring <robh+dt@kernel.org> wrote:
>>>>>>>>>>
>>>>>>>>>> On Sun, Jun 20, 2021 at 10:03 PM Jassi Brar <jassisinghbrar@gmail.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On Sat, Jun 12, 2021 at 4:46 AM Martin Botka
>>>>>>>>>>> <martin.botka@somainline.org> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> This commit adds compatible for the SM6125 SoC
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Martin Botka <martin.botka@somainline.org>
>>>>>>>>>>>> ---
>>>>>>>>>>>> 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".
>>>>>
>>>> To me, the deeper problem seems to be naming a controller "The apcs
>>>> hmss global block _in_ sdm660" just because the h/w manual hasn't
>>>> given a name to it.  But that is okay too, if we name the subsequent
>>>> controllers as "the same as one in sdm660" and provide the h/w
>>>> configuration 'offset' via a DT property.
>>>>
>>>
>>> As I said, I'd need to dig through the hardware documentation for the
>>> various platforms to see if I can find what the common denominators are.
>>> We've always seen this as "the apcs hmss global block _in_ <platform>".
>>>
>>>>>>>> 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.
>>>>>
>>>> Yes, and I don't suggest it otherwise. For h/w quirks and
>>>> extra/missing features, it does make sense to have different
>>>> compatibles.
>>>>
>>>
>>> But what you're suggesting assumes that they are the same and that we're
>>> done implementing all the software for this block. The platform specific
>>> compatible allows us to postpone that question.
>>>
>>>> However, for _trivial_ variations let us get that value from DT.
>>>> 'offset' is anyway a h/w property.
>>>> That way we won't be distinguishing platforms using dt properties, but
>>>> only support different platforms seamlessly.
>>>>
>>>
>>> As I said previously, this goes against the direction provided by the DT
>>> maintainers. If a property is platform specific this should be expressed
>>> by the compatible.
>>>
>>>> On second thought, we have grown from 2 to 13 aliases in 4 yrs. I only
>>>> have to ignore 3 times/annum to lead a peaceful life ;)
>>>>
>>>
>>> True, but I'll try to find some time to see if we have some reuse of the
>>> IP block to allow us to use some generic compatible.
>>>
>>> We'd still need a patch in the DT binding for every single platform, but
>>> we should be able to avoid the compatible additions in the driver.
>>>
>>
>> Hello Jassi, Bjorn
>>
>> I've read the entire thread and I can't say that Jassi is entirely wrong
>> but I also agree with Bjorn on this matter.
>>
>> This driver is here to "simply" manage the register offset in the APCS
>> IP, which is a pretty straightforward operation.
>> If you check in this driver, you will see that there's not much
>> duplication between the various qcom_apcs_ipc_data that we have for
>> all the different SoCs.
>>
>> Checking further, we can effectively reduce the amount of compatibles
>> in this driver by simply removing some "duplicated" instances and in
>> particular:
>> ipq6018, ipq8074, msm8916, msm8994, msm8998, sdm660
>>
>> and eventually replacing them with either of:
>> - 8bits_apcs_data    qcom,apcs-apps-global-8bit
>>                       qcom,apcs-kpss-global-8bit
> 
> I don't like those compatibles, simply because the binding is supposed
> to describe the hardware block, not the fact that Linux _currently_ only
> pokes this one register.
> 

Since you've immediately misunderstood my naming, yeah, that wouldn't be
the best thing to use as a compatible.

> We could probably "qcom,apss-global" as a catch-all for at least sc7180,
> sc7280, sdm845, sm8150, sm8250 and sm8350.
> 

Doesn't look like a bad idea, but if we want to *enforce* writing also
the platform-specific compatible, I can see patch series going back
and forth and getting refused because this will not be really understood
by everyone, I think.
In this case, if writing the platform compatible is something mandatory,
the only way to really make sure to avoid losing time with reviews like
"[...] here you have to write also the platform compatible", is to just
keep the thing as it is.

> But look at 8996 and 8998, both named "something-hmss-something", with
> different register layout. And a quick glance seems to indicate that
> sdm660 isn't a hmss after all :/
> 

Starting from the fact that I don't clearly remember what-when-why of
my research done more than one year ago, I do remember that conclusion
was that, in this regard, SDM630/660 were "mostly the same" as MSM8998.
In any case, this is something that, at this point, is better get
verified, maybe.

> But introducing qcom,apss-global should catch a bunch of the newer
> platforms.
> 
> 
> On the DT binding side we still need the platform-specific ones and we
> need each one to be added to the binding regardless of the catch-all in
> the driver.
> 
> Regards,
> Bjorn
> 
>> - more_appropriate_name_apcs_data qcom,(...blah)
>>
>> This would mean that we would have to use a generic "qcom,apcs-clk" as
>> the clk_name, but no other modifications would be done, apart checking
>> the return value to choose whether to print or not the dev_err when the
>> clock name is specified but not present in dt, since the driver is
>> already actually covering this case.
>>
>> That would make us able to reduce the compatibles from 6 to 2, relative
>> to the aforementioned SoC specific bindings.
>> I'm positive that, through time, when new SoCs arrive, we would avoid
>> getting this compatible list to be megabytes long...
>>
>> Right now it's not an issue, but since Qualcomm SoCs are now being very
>> actively upstreamed, I can see this coming in the future, somehow.
>>
>>
>> Of course this means that we're getting some fair amount of patch-noise
>> in the mailing lists, since all qcom dtsi files will have to be changed,
>> but that shouldn't really be a problem, I guess.
>>
>> I'm sure that I'm not the only one with such a "wow-idea" in mind :)
>>
>> Yours,
>> - Angelo
>>
>>> Regards,
>>> Bjorn
>>>
>>

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

[Bjorn Andersson]

On Thu 24 Jun 17:59 CDT 2021, AngeloGioacchino Del Regno wrote:

> Il 24/06/21 23:07, Bjorn Andersson ha scritto:
> > On Tue 22 Jun 09:36 CDT 2021, AngeloGioacchino Del Regno wrote:
[..]
> > > Hello Jassi, Bjorn
> > > 
> > > I've read the entire thread and I can't say that Jassi is entirely wrong
> > > but I also agree with Bjorn on this matter.
> > > 
> > > This driver is here to "simply" manage the register offset in the APCS
> > > IP, which is a pretty straightforward operation.
> > > If you check in this driver, you will see that there's not much
> > > duplication between the various qcom_apcs_ipc_data that we have for
> > > all the different SoCs.
> > > 
> > > Checking further, we can effectively reduce the amount of compatibles
> > > in this driver by simply removing some "duplicated" instances and in
> > > particular:
> > > ipq6018, ipq8074, msm8916, msm8994, msm8998, sdm660
> > > 
> > > and eventually replacing them with either of:
> > > - 8bits_apcs_data    qcom,apcs-apps-global-8bit
> > >                       qcom,apcs-kpss-global-8bit
> > 
> > I don't like those compatibles, simply because the binding is supposed
> > to describe the hardware block, not the fact that Linux _currently_ only
> > pokes this one register.
> > 
> 
> Since you've immediately misunderstood my naming, yeah, that wouldn't be
> the best thing to use as a compatible.
> 

Sorry, that was certainly not my intention.

> > We could probably "qcom,apss-global" as a catch-all for at least sc7180,
> > sc7280, sdm845, sm8150, sm8250 and sm8350.
> > 
> 
> Doesn't look like a bad idea, but if we want to *enforce* writing also
> the platform-specific compatible, I can see patch series going back
> and forth and getting refused because this will not be really understood
> by everyone, I think.
> In this case, if writing the platform compatible is something mandatory,
> the only way to really make sure to avoid losing time with reviews like
> "[...] here you have to write also the platform compatible", is to just
> keep the thing as it is.
> 

My understanding from the DT maintainers is that the dts would be
checked by the schema, but I suspect that you're right in that we might
have some back and forth on the DT binding, but I don't think that's a
big deal.

> > But look at 8996 and 8998, both named "something-hmss-something", with
> > different register layout. And a quick glance seems to indicate that
> > sdm660 isn't a hmss after all :/
> > 
> 
> Starting from the fact that I don't clearly remember what-when-why of
> my research done more than one year ago, I do remember that conclusion
> was that, in this regard, SDM630/660 were "mostly the same" as MSM8998.
> In any case, this is something that, at this point, is better get
> verified, maybe.
> 

Yeah, I presume that someone with the documentation would need to go
through each one of these and see what kind of grouping there might be.

And I also presume that there might be cases where the CPU clocks has
moved into the secure world, so that even though the hardware block is
the same the presence of a in-kernel clock driver in the implementation
might differ.


But just to clarify, I find it annoying having to sprinkle compatibles
all over the place every time I try to get a new board up. So I am not
against us trying to figure this out.

Regards,
Bjorn

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

[Jassi Brar]

On Thu, Jun 24, 2021 at 7:02 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
.....
>
> But just to clarify, I find it annoying having to sprinkle compatibles
> all over the place every time I try to get a new board up. So I am not
> against us trying to figure this out.
>
Rob has acked the dt-bindings. So I guess I have to pick this patch
(and eventually the more recent patch adding another 2 compatibles).
Bring it on.

Cheers!

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

[Jassi Brar]

On Mon, Jun 21, 2021 at 10:52 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:


> > > > > > 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.
> > >
> > Yes, and I don't suggest it otherwise. For h/w quirks and
> > extra/missing features, it does make sense to have different
> > compatibles.
> >
>
> But what you're suggesting assumes that they are the same and that we're
> done implementing all the software for this block. The platform specific
> compatible allows us to postpone that question.
>
It has been 4yrs and 13 platforms. The compatible strings are used
only to match the hardcoded 'offset' values. Maybe we cross the bridge
when we get to it.
I think, when the drivers are enhanced and the kernel binary needs to
be updated, we could update the dtb as well? Or is it too hard on
these platforms?

cheers.