[PATCH 0/2] reset: img: Add pistachio SoC reset support

[PATCH 0/2] reset: img: Add pistachio SoC reset support

[James Hartley]

This series provides the reset controller driver and documentation for the
pistachio SoC. 

Further platform support and a device tree will be posted via the MIPS
mailing list shortly.

These patches are based on linux-next.

Damien.Horsley (2):
  reset: img: Add pistachio reset controller binding document
  reset: img: Add Pistachio reset controller driver

 .../bindings/reset/img,pistachio-reset.txt         |   51 ++++++
 drivers/reset/Makefile                             |    1 +
 drivers/reset/reset-pistachio.c                    |  162 ++++++++++++++++++++
 .../reset-controller/pistachio-resets.h            |   36 +++++
 4 files changed, 250 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/img,pistachio-reset.txt
 create mode 100644 drivers/reset/reset-pistachio.c
 create mode 100644 include/dt-bindings/reset-controller/pistachio-resets.h

-- 
1.7.9.5

Re: [PATCH 1/2] reset: img: Add pistachio reset controller binding document

[Philipp Zabel]

Am Freitag, den 15.01.2016, 14:44 +0000 schrieb James Hartley:
> From: "Damien.Horsley" <Damien.Horsley@imgtec.com>
> 
> Add binding document for the Pistachio SoC reset controller
> 
> Signed-off-by: Damien.Horsley <Damien.Horsley@imgtec.com>
> Signed-off-by: James Hartley <james.hartley@imgtec.com>
> ---
>  .../bindings/reset/img,pistachio-reset.txt         |   51 ++++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/img,pistachio-reset.txt
> 
> diff --git a/Documentation/devicetree/bindings/reset/img,pistachio-reset.txt b/Documentation/devicetree/bindings/reset/img,pistachio-reset.txt
> new file mode 100644
> index 0000000..12558d2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/img,pistachio-reset.txt
> @@ -0,0 +1,51 @@
> +Pistachio Reset Controller
> +=============================================================================
> +
> +This binding describes a reset controller device that is used to enable and
> +disable individual IP blocks within the Pistachio SoC using "soft reset"
> +control bits found in the Pistachio SoC top level registers.
> +
> +The actual action taken when soft reset is asserted is hardware dependent.
> +However, when asserted it may not be possible to access the hardware's
> +registers, and following an assert/deassert sequence the hardware's previous
> +state may no longer be valid.
> +
> +Please refer to Documentation/devicetree/bindings/reset/reset.txt
> +for common reset controller binding usage.
> +
> +Required properties:
> +
> +- compatible: Contains "img,pistachio-reset"
> +
> +- img,cr-periph: Contains a phandle to the peripheral control syscon
> +                    node which contains the soft reset register
> +
> +- #reset-cells: Contains 1
> +
> +Example:
> +
> +	pistachio_reset: reset-controller {
> +		compatible = "img,pistachio-reset";
> +		img,cr-periph = <&cr_periph>;
> +		#reset-cells = <1>;
> +	};

If this is just a register in the cr_periph syscon, why not put the
reset-controller node inside the cr_periph node:

    cr_periph: syscon@xyz {
        compatible = "syscon", "simple-mfd";
        pistachio_reset: reset-controller {
            compatible = "img,pistachio-reset";
            #reset-cells = <1>;
        };
    };

And in the driver use 
    syscon_node_to_regmap(np->parent);
instead of
    syscon_regmap_lookup_by_phandle(np, "img,cr-periph");
?

Do you need a reg property, or is will the register offset be 0 in all
pistachio variants?

> +Specifying reset control of devices
> +=======================================
> +
> +Device nodes should specify the reset channel required in their "resets"
> +property, containing a phandle to the pistachio reset device node and an
> +index specifying which reset to use, as described in
> +Documentation/devicetree/bindings/reset/reset.txt.
> +
> +Example:
> +
> +	spdif_out: spdif-out@18100d00 {
> +		...
> +		resets = <&pistachio_reset PISTACHIO_RESET_SPDIF_OUT>;
> +		reset-names = "rst";

reset-names is not needed for devices that only have one reset.

regards
Philipp

Re: [PATCH 1/2] reset: img: Add pistachio reset controller binding document

[James Hartley]

Hi Philipp,

Thanks for the quick review,

On 01/15/16 15:57, Philipp Zabel wrote:
> Am Freitag, den 15.01.2016, 14:44 +0000 schrieb James Hartley:
[..]
>> +Example:
>> +
>> +	pistachio_reset: reset-controller {
>> +		compatible = "img,pistachio-reset";
>> +		img,cr-periph = <&cr_periph>;
>> +		#reset-cells = <1>;
>> +	};
> If this is just a register in the cr_periph syscon, why not put the
> reset-controller node inside the cr_periph node:
>
>      cr_periph: syscon@xyz {
>          compatible = "syscon", "simple-mfd";
>          pistachio_reset: reset-controller {
>              compatible = "img,pistachio-reset";
>              #reset-cells = <1>;
>          };
>      };
>
> And in the driver use
>      syscon_node_to_regmap(np->parent);
> instead of
>      syscon_regmap_lookup_by_phandle(np, "img,cr-periph");
> ?

Yes that makes sense, I'll look at changing that for a V2 patchset.

> Do you need a reg property, or is will the register offset be 0 in all
> pistachio variants?

The offset will always be 0 for all pistachio variants.

[..]
> +Example:
> +
> +	spdif_out: spdif-out@18100d00 {
> +		...
> +		resets = <&pistachio_reset PISTACHIO_RESET_SPDIF_OUT>;
> +		reset-names = "rst";
> reset-names is not needed for devices that only have one reset.

Agreed, but there are already audio drivers that pistachio uses that 
have been merged upstream
using this name in their dt-entries - is there any harm in leaving it in?

>
> regards
> Philipp
>
Thanks,
James.

Re: [PATCH 1/2] reset: img: Add pistachio reset controller binding document

[Philipp Zabel]

Am Freitag, den 15.01.2016, 16:32 +0000 schrieb James Hartley:
> Hi Philipp,
> 
> Thanks for the quick review,
> 
> On 01/15/16 15:57, Philipp Zabel wrote:
> > Am Freitag, den 15.01.2016, 14:44 +0000 schrieb James Hartley:
> [..]
> >> +Example:
> >> +
> >> +	pistachio_reset: reset-controller {
> >> +		compatible = "img,pistachio-reset";
> >> +		img,cr-periph = <&cr_periph>;
> >> +		#reset-cells = <1>;
> >> +	};
> > If this is just a register in the cr_periph syscon, why not put the
> > reset-controller node inside the cr_periph node:
> >
> >      cr_periph: syscon@xyz {
> >          compatible = "syscon", "simple-mfd";
> >          pistachio_reset: reset-controller {
> >              compatible = "img,pistachio-reset";
> >              #reset-cells = <1>;
> >          };
> >      };
> >
> > And in the driver use
> >      syscon_node_to_regmap(np->parent);
> > instead of
> >      syscon_regmap_lookup_by_phandle(np, "img,cr-periph");
> > ?
> 
> Yes that makes sense, I'll look at changing that for a V2 patchset.
> 
> > Do you need a reg property, or is will the register offset be 0 in all
> > pistachio variants?
> 
> The offset will always be 0 for all pistachio variants.

Ok.

> [..]
> > +Example:
> > +
> > +	spdif_out: spdif-out@18100d00 {
> > +		...
> > +		resets = <&pistachio_reset PISTACHIO_RESET_SPDIF_OUT>;
> > +		reset-names = "rst";
> > reset-names is not needed for devices that only have one reset.
> 
> Agreed, but there are already audio drivers that pistachio uses that 
> have been merged upstream
> using this name in their dt-entries - is there any harm in leaving it in?

That was just a remark. As long as this requirement is there from the
beginning and it is used consistently, I don't mind.

best regards
Philipp