diff mbox series

[1/4] dt-bindings: reset: Add PDC reset binding for SDM845 SoCs

Message ID 20180727152811.15258-1-sibis@codeaurora.org (mailing list archive)
State New, archived
Headers show
Series [1/4] dt-bindings: reset: Add PDC reset binding for SDM845 SoCs | expand

Commit Message

Sibi Sankar July 27, 2018, 3:28 p.m. UTC
Add SDM845 PDC (Power Domain Controller) reset controller binding

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---
 .../bindings/reset/qcom,pdc-reset.txt         | 52 +++++++++++++++++++
 include/dt-bindings/reset/qcom,sdm845-pdc.h   | 20 +++++++
 2 files changed, 72 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
 create mode 100644 include/dt-bindings/reset/qcom,sdm845-pdc.h

Comments

Philipp Zabel July 31, 2018, 8:42 a.m. UTC | #1
Hi Sibi,

On Fri, 2018-07-27 at 20:58 +0530, Sibi Sankar wrote:
> Add SDM845 PDC (Power Domain Controller) reset controller binding
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  .../bindings/reset/qcom,pdc-reset.txt         | 52 +++++++++++++++++++
>  include/dt-bindings/reset/qcom,sdm845-pdc.h   | 20 +++++++
>  2 files changed, 72 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
>  create mode 100644 include/dt-bindings/reset/qcom,sdm845-pdc.h
> 
> diff --git a/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
> new file mode 100644
> index 000000000000..85e159962e08
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
> @@ -0,0 +1,52 @@
> +PDC Reset Controller
> +======================================
> +
> +This binding describes a reset-controller found on PDC-Global(Power Domain
> +Controller) block for Qualcomm Technologies Inc SDM845 SoCs.
> +
> +Required properties:
> +- compatible:
> +	Usage: required
> +	Value type: <string>
> +	Definition: must be:
> +		    "qcom,sdm845-pdc-global"
> +
> +- reg:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: must specify the base address and size of the register
> +	            space.
> +
> +- #reset-cells:
> +	Usage: required
> +	Value type: <uint>
> +	Definition: must be 1; cell entry represents the reset index.
> +
> +Example:
> +
> +pdc_reset: reset-controller@b2e0000 {

Is this really just a reset controller?

The name makes it sound like a driver binding to this should also
provide pm_genpd and the binding should probably call this a power-
controller: Documentation/devicetree/bindings/power/power_domain.txt.

> +	compatible = "qcom,sdm845-pdc-global";
> +	reg = <0xb2e0000 0x20000>;

This looks like this is the register space of the complete PDC, not just
the reset register?

> +	#reset-cells = <1>;
> +};
> +
> +PDC reset clients
> +======================================
> +
> +Device nodes that need access to reset lines should
> +specify them as a reset phandle in their corresponding node as
> +specified in reset.txt.
> +
> +For list of all valid reset indicies see
> +<dt-bindings/reset/qcom,sdm845-pdc.h>
> +
> +Example:
> +
> +modem-pil@4080000 {
> +	...
> +
> +	resets = <&pdc_reset PDC_MODEM_SYNC_RESET>;
> +	reset-names = "pdc_restart";
> +
> +	...
> +};
> diff --git a/include/dt-bindings/reset/qcom,sdm845-pdc.h b/include/dt-bindings/reset/qcom,sdm845-pdc.h
> new file mode 100644
> index 000000000000..53c37f9c319a
> --- /dev/null
> +++ b/include/dt-bindings/reset/qcom,sdm845-pdc.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2018 The Linux Foundation. All rights reserved.
> + */
> +
> +#ifndef _DT_BINDINGS_RESET_PDC_SDM_845_H
> +#define _DT_BINDINGS_RESET_PDC_SDM_845_H
> +
> +#define PDC_APPS_SYNC_RESET	0
> +#define PDC_SP_SYNC_RESET	1
> +#define PDC_AUDIO_SYNC_RESET	2
> +#define PDC_SENSORS_SYNC_RESET	3
> +#define PDC_AOP_SYNC_RESET	4
> +#define PDC_DEBUG_SYNC_RESET	5
> +#define PDC_GPU_SYNC_RESET	6
> +#define PDC_DISPLAY_SYNC_RESET	7
> +#define PDC_COMPUTE_SYNC_RESET	8
> +#define PDC_MODEM_SYNC_RESET	9
> +
> +#endif

regards
Philipp
--
To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sibi Sankar July 31, 2018, 12:57 p.m. UTC | #2
Hi Philipp,
Thanks for the review!

On 07/31/2018 02:12 PM, Philipp Zabel wrote:
> Hi Sibi,
> 
> On Fri, 2018-07-27 at 20:58 +0530, Sibi Sankar wrote:
>> Add SDM845 PDC (Power Domain Controller) reset controller binding
>>
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>>   .../bindings/reset/qcom,pdc-reset.txt         | 52 +++++++++++++++++++
>>   include/dt-bindings/reset/qcom,sdm845-pdc.h   | 20 +++++++
>>   2 files changed, 72 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
>>   create mode 100644 include/dt-bindings/reset/qcom,sdm845-pdc.h
>>
>> diff --git a/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
>> new file mode 100644
>> index 000000000000..85e159962e08
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
>> @@ -0,0 +1,52 @@
>> +PDC Reset Controller
>> +======================================
>> +
>> +This binding describes a reset-controller found on PDC-Global(Power Domain
>> +Controller) block for Qualcomm Technologies Inc SDM845 SoCs.
>> +
>> +Required properties:
>> +- compatible:
>> +	Usage: required
>> +	Value type: <string>
>> +	Definition: must be:
>> +		    "qcom,sdm845-pdc-global"
>> +
>> +- reg:
>> +	Usage: required
>> +	Value type: <prop-encoded-array>
>> +	Definition: must specify the base address and size of the register
>> +	            space.
>> +
>> +- #reset-cells:
>> +	Usage: required
>> +	Value type: <uint>
>> +	Definition: must be 1; cell entry represents the reset index.
>> +
>> +Example:
>> +
>> +pdc_reset: reset-controller@b2e0000 {
> 
> Is this really just a reset controller?
> 
> The name makes it sound like a driver binding to this should also
> provide pm_genpd and the binding should probably call this a power-
> controller: Documentation/devicetree/bindings/power/power_domain.txt.
> 

The PDC-global reg space which is a part of PDC-wrapper reg space seems
to be only used for the reset lines.

Couple of other drivers use other parts of the PDC-wrapper reg space:
https://patchwork.kernel.org/patch/10223701/ (PDC-Interrupt controller)
https://patchwork.kernel.org/patch/10255767/ (GMU-PDC incorrectly tries
to occupy the entire pdc-wrapper reg space)

since it couldn't be logically mapped into pdc-interrupt driver, it had
to be included as a separate reset driver.

>> +	compatible = "qcom,sdm845-pdc-global";
>> +	reg = <0xb2e0000 0x20000>;
> 
> This looks like this is the register space of the complete PDC, not just
> the reset register?
> 

The entire register space was chosen because it is only used for its
reset lines (had a good look at the downstream kernel and had a 
conversation with Lina) and to ensure break backward compatibility for
the for the dt entry if the reg-space was used for other purposes in
the future.

>> +	#reset-cells = <1>;
>> +};
>> +
>> +PDC reset clients
>> +======================================
>> +
>> +Device nodes that need access to reset lines should
>> +specify them as a reset phandle in their corresponding node as
>> +specified in reset.txt.
>> +
>> +For list of all valid reset indicies see
>> +<dt-bindings/reset/qcom,sdm845-pdc.h>
>> +
>> +Example:
>> +
>> +modem-pil@4080000 {
>> +	...
>> +
>> +	resets = <&pdc_reset PDC_MODEM_SYNC_RESET>;
>> +	reset-names = "pdc_restart";
>> +
>> +	...
>> +};
>> diff --git a/include/dt-bindings/reset/qcom,sdm845-pdc.h b/include/dt-bindings/reset/qcom,sdm845-pdc.h
>> new file mode 100644
>> index 000000000000..53c37f9c319a
>> --- /dev/null
>> +++ b/include/dt-bindings/reset/qcom,sdm845-pdc.h
>> @@ -0,0 +1,20 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (C) 2018 The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#ifndef _DT_BINDINGS_RESET_PDC_SDM_845_H
>> +#define _DT_BINDINGS_RESET_PDC_SDM_845_H
>> +
>> +#define PDC_APPS_SYNC_RESET	0
>> +#define PDC_SP_SYNC_RESET	1
>> +#define PDC_AUDIO_SYNC_RESET	2
>> +#define PDC_SENSORS_SYNC_RESET	3
>> +#define PDC_AOP_SYNC_RESET	4
>> +#define PDC_DEBUG_SYNC_RESET	5
>> +#define PDC_GPU_SYNC_RESET	6
>> +#define PDC_DISPLAY_SYNC_RESET	7
>> +#define PDC_COMPUTE_SYNC_RESET	8
>> +#define PDC_MODEM_SYNC_RESET	9
>> +
>> +#endif
> 
> regards
> Philipp
>
Rob Herring (Arm) Aug. 7, 2018, 6:16 p.m. UTC | #3
On Tue, Jul 31, 2018 at 06:27:24PM +0530, Sibi S wrote:
> Hi Philipp,
> Thanks for the review!
> 
> On 07/31/2018 02:12 PM, Philipp Zabel wrote:
> > Hi Sibi,
> > 
> > On Fri, 2018-07-27 at 20:58 +0530, Sibi Sankar wrote:
> > > Add SDM845 PDC (Power Domain Controller) reset controller binding
> > > 
> > > Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> > > ---
> > >   .../bindings/reset/qcom,pdc-reset.txt         | 52 +++++++++++++++++++
> > >   include/dt-bindings/reset/qcom,sdm845-pdc.h   | 20 +++++++
> > >   2 files changed, 72 insertions(+)
> > >   create mode 100644 Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
> > >   create mode 100644 include/dt-bindings/reset/qcom,sdm845-pdc.h
> > > 
> > > diff --git a/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
> > > new file mode 100644
> > > index 000000000000..85e159962e08
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
> > > @@ -0,0 +1,52 @@
> > > +PDC Reset Controller
> > > +======================================
> > > +
> > > +This binding describes a reset-controller found on PDC-Global(Power Domain
> > > +Controller) block for Qualcomm Technologies Inc SDM845 SoCs.
> > > +
> > > +Required properties:
> > > +- compatible:
> > > +	Usage: required
> > > +	Value type: <string>
> > > +	Definition: must be:
> > > +		    "qcom,sdm845-pdc-global"
> > > +
> > > +- reg:
> > > +	Usage: required
> > > +	Value type: <prop-encoded-array>
> > > +	Definition: must specify the base address and size of the register
> > > +	            space.
> > > +
> > > +- #reset-cells:
> > > +	Usage: required
> > > +	Value type: <uint>
> > > +	Definition: must be 1; cell entry represents the reset index.
> > > +
> > > +Example:
> > > +
> > > +pdc_reset: reset-controller@b2e0000 {
> > 
> > Is this really just a reset controller?
> > 
> > The name makes it sound like a driver binding to this should also
> > provide pm_genpd and the binding should probably call this a power-
> > controller: Documentation/devicetree/bindings/power/power_domain.txt.
> > 
> 
> The PDC-global reg space which is a part of PDC-wrapper reg space seems
> to be only used for the reset lines.
> 
> Couple of other drivers use other parts of the PDC-wrapper reg space:
> https://patchwork.kernel.org/patch/10223701/ (PDC-Interrupt controller)
> https://patchwork.kernel.org/patch/10255767/ (GMU-PDC incorrectly tries
> to occupy the entire pdc-wrapper reg space)
> 
> since it couldn't be logically mapped into pdc-interrupt driver, it had
> to be included as a separate reset driver.

You can't have overlapping regions in DT (well, you can because we have 
to work-around existing DTs that do, but you shouldn't).

A single node can be multiple providers such as interrupt controller and 
reset controller. It's an OS problem to split that into multiple 
drivers.
 
> > > +	compatible = "qcom,sdm845-pdc-global";
> > > +	reg = <0xb2e0000 0x20000>;
> > 
> > This looks like this is the register space of the complete PDC, not just
> > the reset register?
> > 
> 
> The entire register space was chosen because it is only used for its
> reset lines (had a good look at the downstream kernel and had a conversation
> with Lina) and to ensure break backward compatibility for
> the for the dt entry if the reg-space was used for other purposes in
> the future.

Why do you want to ensure breaking backwards compatibility?

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sibi Sankar Aug. 8, 2018, 3:44 p.m. UTC | #4
Hi Rob,
Thanks for the review

On 08/07/2018 11:46 PM, Rob Herring wrote:
> On Tue, Jul 31, 2018 at 06:27:24PM +0530, Sibi S wrote:
>> Hi Philipp,
>> Thanks for the review!
>>
>> On 07/31/2018 02:12 PM, Philipp Zabel wrote:
>>> Hi Sibi,
>>>
>>> On Fri, 2018-07-27 at 20:58 +0530, Sibi Sankar wrote:
>>>> Add SDM845 PDC (Power Domain Controller) reset controller binding
>>>>
>>>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>>>> ---
>>>>    .../bindings/reset/qcom,pdc-reset.txt         | 52 +++++++++++++++++++
>>>>    include/dt-bindings/reset/qcom,sdm845-pdc.h   | 20 +++++++
>>>>    2 files changed, 72 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
>>>>    create mode 100644 include/dt-bindings/reset/qcom,sdm845-pdc.h
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
>>>> new file mode 100644
>>>> index 000000000000..85e159962e08
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
>>>> @@ -0,0 +1,52 @@
>>>> +PDC Reset Controller
>>>> +======================================
>>>> +
>>>> +This binding describes a reset-controller found on PDC-Global(Power Domain
>>>> +Controller) block for Qualcomm Technologies Inc SDM845 SoCs.
>>>> +
>>>> +Required properties:
>>>> +- compatible:
>>>> +	Usage: required
>>>> +	Value type: <string>
>>>> +	Definition: must be:
>>>> +		    "qcom,sdm845-pdc-global"
>>>> +
>>>> +- reg:
>>>> +	Usage: required
>>>> +	Value type: <prop-encoded-array>
>>>> +	Definition: must specify the base address and size of the register
>>>> +	            space.
>>>> +
>>>> +- #reset-cells:
>>>> +	Usage: required
>>>> +	Value type: <uint>
>>>> +	Definition: must be 1; cell entry represents the reset index.
>>>> +
>>>> +Example:
>>>> +
>>>> +pdc_reset: reset-controller@b2e0000 {
>>>
>>> Is this really just a reset controller?
>>>
>>> The name makes it sound like a driver binding to this should also
>>> provide pm_genpd and the binding should probably call this a power-
>>> controller: Documentation/devicetree/bindings/power/power_domain.txt.
>>>
>>
>> The PDC-global reg space which is a part of PDC-wrapper reg space seems
>> to be only used for the reset lines.
>>
>> Couple of other drivers use other parts of the PDC-wrapper reg space:
>> https://patchwork.kernel.org/patch/10223701/ (PDC-Interrupt controller)
>> https://patchwork.kernel.org/patch/10255767/ (GMU-PDC incorrectly tries
>> to occupy the entire pdc-wrapper reg space)
>>
>> since it couldn't be logically mapped into pdc-interrupt driver, it had
>> to be included as a separate reset driver.
> 
> You can't have overlapping regions in DT (well, you can because we have
> to work-around existing DTs that do, but you shouldn't).
> 
> A single node can be multiple providers such as interrupt controller and
> reset controller. It's an OS problem to split that into multiple
> drivers.
>   

There will be no overlaps. Jordan will be changing the dt binding of
gmu_pdc so that there is no overlap I guess. What I meant to say is that
pdc-global is a separate reg-space and currently has no other
functionality other than exposing the reset lines.

>>>> +	compatible = "qcom,sdm845-pdc-global";
>>>> +	reg = <0xb2e0000 0x20000>;
>>>
>>> This looks like this is the register space of the complete PDC, not just
>>> the reset register?
>>>
>>
>> The entire register space was chosen because it is only used for its
>> reset lines (had a good look at the downstream kernel and had a conversation
>> with Lina) and to ensure break backward compatibility for
>> the for the dt entry if the reg-space was used for other purposes in
>> the future.
> 
> Why do you want to ensure breaking backwards compatibility?
>

Similar to the AOSS reset driver which had a unused clock part, this
driver also exposes a reg space of which only reset lines are used.

> Rob
>
Jordan Crouse Aug. 8, 2018, 9:37 p.m. UTC | #5
On Wed, Aug 08, 2018 at 09:14:05PM +0530, Sibi Sankar wrote:
> Hi Rob,
> Thanks for the review
> 
> On 08/07/2018 11:46 PM, Rob Herring wrote:
> >On Tue, Jul 31, 2018 at 06:27:24PM +0530, Sibi S wrote:
> >>Hi Philipp,
> >>Thanks for the review!
> >>
> >>On 07/31/2018 02:12 PM, Philipp Zabel wrote:
> >>>Hi Sibi,
> >>>
> >>>On Fri, 2018-07-27 at 20:58 +0530, Sibi Sankar wrote:
> >>>>Add SDM845 PDC (Power Domain Controller) reset controller binding
> >>>>
> >>>>Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> >>>>---
> >>>>   .../bindings/reset/qcom,pdc-reset.txt         | 52 +++++++++++++++++++
> >>>>   include/dt-bindings/reset/qcom,sdm845-pdc.h   | 20 +++++++
> >>>>   2 files changed, 72 insertions(+)
> >>>>   create mode 100644 Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
> >>>>   create mode 100644 include/dt-bindings/reset/qcom,sdm845-pdc.h
> >>>>
> >>>>diff --git a/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
> >>>>new file mode 100644
> >>>>index 000000000000..85e159962e08
> >>>>--- /dev/null
> >>>>+++ b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
> >>>>@@ -0,0 +1,52 @@
> >>>>+PDC Reset Controller
> >>>>+======================================
> >>>>+
> >>>>+This binding describes a reset-controller found on PDC-Global(Power Domain
> >>>>+Controller) block for Qualcomm Technologies Inc SDM845 SoCs.
> >>>>+
> >>>>+Required properties:
> >>>>+- compatible:
> >>>>+	Usage: required
> >>>>+	Value type: <string>
> >>>>+	Definition: must be:
> >>>>+		    "qcom,sdm845-pdc-global"
> >>>>+
> >>>>+- reg:
> >>>>+	Usage: required
> >>>>+	Value type: <prop-encoded-array>
> >>>>+	Definition: must specify the base address and size of the register
> >>>>+	            space.
> >>>>+
> >>>>+- #reset-cells:
> >>>>+	Usage: required
> >>>>+	Value type: <uint>
> >>>>+	Definition: must be 1; cell entry represents the reset index.
> >>>>+
> >>>>+Example:
> >>>>+
> >>>>+pdc_reset: reset-controller@b2e0000 {
> >>>
> >>>Is this really just a reset controller?
> >>>
> >>>The name makes it sound like a driver binding to this should also
> >>>provide pm_genpd and the binding should probably call this a power-
> >>>controller: Documentation/devicetree/bindings/power/power_domain.txt.
> >>>
> >>
> >>The PDC-global reg space which is a part of PDC-wrapper reg space seems
> >>to be only used for the reset lines.
> >>
> >>Couple of other drivers use other parts of the PDC-wrapper reg space:
> >>https://patchwork.kernel.org/patch/10223701/ (PDC-Interrupt controller)
> >>https://patchwork.kernel.org/patch/10255767/ (GMU-PDC incorrectly tries
> >>to occupy the entire pdc-wrapper reg space)
> >>
> >>since it couldn't be logically mapped into pdc-interrupt driver, it had
> >>to be included as a separate reset driver.
> >
> >You can't have overlapping regions in DT (well, you can because we have
> >to work-around existing DTs that do, but you shouldn't).
> >
> >A single node can be multiple providers such as interrupt controller and
> >reset controller. It's an OS problem to split that into multiple
> >drivers.
> 
> There will be no overlaps. Jordan will be changing the dt binding of
> gmu_pdc so that there is no overlap I guess. What I meant to say is that
> pdc-global is a separate reg-space and currently has no other
> functionality other than exposing the reset lines.

Correct - the updated GPU DT will be:

  reg = <0x506a000 0x30000>,
     <0xb280000 0x10000>,
     <0xb480000, 0x10000>;
  reg-names = "gmu", "gmu_pdc", "gmu_pdc_seq";

Jordan
Jordan Crouse Aug. 8, 2018, 10:48 p.m. UTC | #6
On Wed, Aug 08, 2018 at 03:37:24PM -0600, Jordan Crouse wrote:
> On Wed, Aug 08, 2018 at 09:14:05PM +0530, Sibi Sankar wrote:
> > Hi Rob,
> > Thanks for the review
> > 
> > On 08/07/2018 11:46 PM, Rob Herring wrote:
> > >On Tue, Jul 31, 2018 at 06:27:24PM +0530, Sibi S wrote:
> > >>Hi Philipp,
> > >>Thanks for the review!
> > >>
> > >>On 07/31/2018 02:12 PM, Philipp Zabel wrote:
> > >>>Hi Sibi,
> > >>>
> > >>>On Fri, 2018-07-27 at 20:58 +0530, Sibi Sankar wrote:
> > >>>>Add SDM845 PDC (Power Domain Controller) reset controller binding
> > >>>>
> > >>>>Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> > >>>>---
> > >>>>   .../bindings/reset/qcom,pdc-reset.txt         | 52 +++++++++++++++++++
> > >>>>   include/dt-bindings/reset/qcom,sdm845-pdc.h   | 20 +++++++
> > >>>>   2 files changed, 72 insertions(+)
> > >>>>   create mode 100644 Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
> > >>>>   create mode 100644 include/dt-bindings/reset/qcom,sdm845-pdc.h
> > >>>>
> > >>>>diff --git a/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
> > >>>>new file mode 100644
> > >>>>index 000000000000..85e159962e08
> > >>>>--- /dev/null
> > >>>>+++ b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
> > >>>>@@ -0,0 +1,52 @@
> > >>>>+PDC Reset Controller
> > >>>>+======================================
> > >>>>+
> > >>>>+This binding describes a reset-controller found on PDC-Global(Power Domain
> > >>>>+Controller) block for Qualcomm Technologies Inc SDM845 SoCs.
> > >>>>+
> > >>>>+Required properties:
> > >>>>+- compatible:
> > >>>>+	Usage: required
> > >>>>+	Value type: <string>
> > >>>>+	Definition: must be:
> > >>>>+		    "qcom,sdm845-pdc-global"
> > >>>>+
> > >>>>+- reg:
> > >>>>+	Usage: required
> > >>>>+	Value type: <prop-encoded-array>
> > >>>>+	Definition: must specify the base address and size of the register
> > >>>>+	            space.
> > >>>>+
> > >>>>+- #reset-cells:
> > >>>>+	Usage: required
> > >>>>+	Value type: <uint>
> > >>>>+	Definition: must be 1; cell entry represents the reset index.
> > >>>>+
> > >>>>+Example:
> > >>>>+
> > >>>>+pdc_reset: reset-controller@b2e0000 {
> > >>>
> > >>>Is this really just a reset controller?
> > >>>
> > >>>The name makes it sound like a driver binding to this should also
> > >>>provide pm_genpd and the binding should probably call this a power-
> > >>>controller: Documentation/devicetree/bindings/power/power_domain.txt.
> > >>>
> > >>
> > >>The PDC-global reg space which is a part of PDC-wrapper reg space seems
> > >>to be only used for the reset lines.
> > >>
> > >>Couple of other drivers use other parts of the PDC-wrapper reg space:
> > >>https://patchwork.kernel.org/patch/10223701/ (PDC-Interrupt controller)
> > >>https://patchwork.kernel.org/patch/10255767/ (GMU-PDC incorrectly tries
> > >>to occupy the entire pdc-wrapper reg space)
> > >>
> > >>since it couldn't be logically mapped into pdc-interrupt driver, it had
> > >>to be included as a separate reset driver.
> > >
> > >You can't have overlapping regions in DT (well, you can because we have
> > >to work-around existing DTs that do, but you shouldn't).
> > >
> > >A single node can be multiple providers such as interrupt controller and
> > >reset controller. It's an OS problem to split that into multiple
> > >drivers.
> > 
> > There will be no overlaps. Jordan will be changing the dt binding of
> > gmu_pdc so that there is no overlap I guess. What I meant to say is that
> > pdc-global is a separate reg-space and currently has no other
> > functionality other than exposing the reset lines.
> 
> Correct - the updated GPU DT will be:
> 
>   reg = <0x506a000 0x30000>,
>      <0xb280000 0x10000>,
>      <0xb480000, 0x10000>;
>   reg-names = "gmu", "gmu_pdc", "gmu_pdc_seq";
> 
> Jordan

Code change:  https://patchwork.freedesktop.org/patch/243513/
DT change: https://patchwork.freedesktop.org/patch/243539/

Jordan
Bjorn Andersson Aug. 21, 2018, 10:08 p.m. UTC | #7
On Fri 27 Jul 08:28 PDT 2018, Sibi Sankar wrote:

> Add SDM845 PDC (Power Domain Controller) reset controller binding
> 

Even though this is currently describing only a reset controller I think
this binding better be talking about the "PDC Global" hardware.

> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  .../bindings/reset/qcom,pdc-reset.txt         | 52 +++++++++++++++++++
>  include/dt-bindings/reset/qcom,sdm845-pdc.h   | 20 +++++++
>  2 files changed, 72 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
>  create mode 100644 include/dt-bindings/reset/qcom,sdm845-pdc.h
> 
> diff --git a/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt

Rename this qcom,pdc-global to match the compatible, and hardware name.

> new file mode 100644
> index 000000000000..85e159962e08
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
> @@ -0,0 +1,52 @@
> +PDC Reset Controller

PDC Global

> +======================================
> +
> +This binding describes a reset-controller found on PDC-Global(Power Domain
> +Controller) block for Qualcomm Technologies Inc SDM845 SoCs.

This looks good.

> +
> +Required properties:
> +- compatible:
> +	Usage: required
> +	Value type: <string>
> +	Definition: must be:
> +		    "qcom,sdm845-pdc-global"
> +
> +- reg:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: must specify the base address and size of the register
> +	            space.
> +
> +- #reset-cells:
> +	Usage: required
> +	Value type: <uint>
> +	Definition: must be 1; cell entry represents the reset index.
> +
> +Example:
> +
> +pdc_reset: reset-controller@b2e0000 {

This is perfectly fine, in its current form it is a reset-controller.

> +	compatible = "qcom,sdm845-pdc-global";
> +	reg = <0xb2e0000 0x20000>;
> +	#reset-cells = <1>;
> +};
> +

Apart from this, the binding looks good!

Regards,
Bjorn
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
new file mode 100644
index 000000000000..85e159962e08
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/qcom,pdc-reset.txt
@@ -0,0 +1,52 @@ 
+PDC Reset Controller
+======================================
+
+This binding describes a reset-controller found on PDC-Global(Power Domain
+Controller) block for Qualcomm Technologies Inc SDM845 SoCs.
+
+Required properties:
+- compatible:
+	Usage: required
+	Value type: <string>
+	Definition: must be:
+		    "qcom,sdm845-pdc-global"
+
+- reg:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: must specify the base address and size of the register
+	            space.
+
+- #reset-cells:
+	Usage: required
+	Value type: <uint>
+	Definition: must be 1; cell entry represents the reset index.
+
+Example:
+
+pdc_reset: reset-controller@b2e0000 {
+	compatible = "qcom,sdm845-pdc-global";
+	reg = <0xb2e0000 0x20000>;
+	#reset-cells = <1>;
+};
+
+PDC reset clients
+======================================
+
+Device nodes that need access to reset lines should
+specify them as a reset phandle in their corresponding node as
+specified in reset.txt.
+
+For list of all valid reset indicies see
+<dt-bindings/reset/qcom,sdm845-pdc.h>
+
+Example:
+
+modem-pil@4080000 {
+	...
+
+	resets = <&pdc_reset PDC_MODEM_SYNC_RESET>;
+	reset-names = "pdc_restart";
+
+	...
+};
diff --git a/include/dt-bindings/reset/qcom,sdm845-pdc.h b/include/dt-bindings/reset/qcom,sdm845-pdc.h
new file mode 100644
index 000000000000..53c37f9c319a
--- /dev/null
+++ b/include/dt-bindings/reset/qcom,sdm845-pdc.h
@@ -0,0 +1,20 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2018 The Linux Foundation. All rights reserved.
+ */
+
+#ifndef _DT_BINDINGS_RESET_PDC_SDM_845_H
+#define _DT_BINDINGS_RESET_PDC_SDM_845_H
+
+#define PDC_APPS_SYNC_RESET	0
+#define PDC_SP_SYNC_RESET	1
+#define PDC_AUDIO_SYNC_RESET	2
+#define PDC_SENSORS_SYNC_RESET	3
+#define PDC_AOP_SYNC_RESET	4
+#define PDC_DEBUG_SYNC_RESET	5
+#define PDC_GPU_SYNC_RESET	6
+#define PDC_DISPLAY_SYNC_RESET	7
+#define PDC_COMPUTE_SYNC_RESET	8
+#define PDC_MODEM_SYNC_RESET	9
+
+#endif