[1/2] reset: Add brcm,bcm63xx-reset device tree binding

Add device tree binding for the BCM63xx soft reset controller.

The BCM63xx contains a soft-reset controller activated by setting
a bit (that must previously have cleared).

Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
---
 .../bindings/reset/brcm,bcm63xx-reset.txt          | 37 ++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/brcm,bcm63xx-reset.txt

On Mon, Nov 30, 2015 at 08:57:31PM +0000, Simon Arlott wrote:
> Add device tree binding for the BCM63xx soft reset controller.
> 
> The BCM63xx contains a soft-reset controller activated by setting
> a bit (that must previously have cleared).
> 
> Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
> ---
>  .../bindings/reset/brcm,bcm63xx-reset.txt          | 37 ++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/brcm,bcm63xx-reset.txt
> 
> diff --git a/Documentation/devicetree/bindings/reset/brcm,bcm63xx-reset.txt b/Documentation/devicetree/bindings/reset/brcm,bcm63xx-reset.txt
> new file mode 100644
> index 0000000..48e9daf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/brcm,bcm63xx-reset.txt
> @@ -0,0 +1,37 @@
> +BCM63xx reset controller
> +
> +The BCM63xx contains a basic soft reset controller in the perf register
> +set which resets components using a bit in a register.
> +
> +Please also refer to reset.txt in this directory for common reset
> +controller binding usage.
> +
> +Required properties:
> +- compatible:	Should be "brcm,bcm<soc>-reset", "brcm,bcm63xx-reset"
> +- regmap:	The register map phandle
> +- offset:	Offset in the register map for the reset register (in bytes)
> +- #reset-cells:	Must be set to 1
> +
> +Optional properties:
> +- mask:		Mask of valid reset bits in the reset register (32 bit access)
> +		(Defaults to all bits)

The bits correspond to the cell values in resets properties? If so then, 
is this really needed? If you are only validating cell values against 
this mask, then drop it (assume the DT does not have crap).

Rob

> +
> +Example:
> +
> +periph_soft_rst: reset-controller {
> +	compatible = "brcm,bcm63168-reset", "brcm,bcm63xx-reset";
> +	regmap = <&periph_cntl>;
> +	offset = <0x10>;
> +
> +	#reset-cells = <1>;
> +};
> +
> +usbh: usbphy@10002700 {
> +	compatible = "brcm,bcm63168-usbh";
> +	reg = <0x10002700 0x38>;
> +	clocks = <&periph_clk 13>, <&timer_clk 18>;
> +	resets = <&periph_soft_rst 6>;
> +	power-supply = <&power_usbh>;
> +	#phy-cells = <0>;
> +};
> +
> -- 
> 2.1.4
> 
> -- 
> Simon Arlott
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hello.

On 11/30/2015 11:57 PM, Simon Arlott wrote:

> Add device tree binding for the BCM63xx soft reset controller.
>
> The BCM63xx contains a soft-reset controller activated by setting
> a bit (that must previously have cleared).
>
> Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
> ---
>   .../bindings/reset/brcm,bcm63xx-reset.txt          | 37 ++++++++++++++++++++++
>   1 file changed, 37 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/reset/brcm,bcm63xx-reset.txt
>
> diff --git a/Documentation/devicetree/bindings/reset/brcm,bcm63xx-reset.txt b/Documentation/devicetree/bindings/reset/brcm,bcm63xx-reset.txt
> new file mode 100644
> index 0000000..48e9daf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/brcm,bcm63xx-reset.txt
> @@ -0,0 +1,37 @@
> +BCM63xx reset controller
> +
> +The BCM63xx contains a basic soft reset controller in the perf register
> +set which resets components using a bit in a register.
> +
> +Please also refer to reset.txt in this directory for common reset
> +controller binding usage.
> +
> +Required properties:
> +- compatible:	Should be "brcm,bcm<soc>-reset", "brcm,bcm63xx-reset"

     Wildcards (xx) are not allowed here. Please choose a "least common 
denominator" SoC and name the string after it.

[...]

MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

2015-11-30 12:58 GMT-08:00 Simon Arlott <simon@fire.lp0.eu>:
> The BCM63xx contains a soft-reset controller activated by setting
> a bit (that must previously have cleared).
>
> Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
> ---
>  MAINTAINERS                   |   1 +
>  drivers/reset/Kconfig         |   9 +++
>  drivers/reset/Makefile        |   1 +
>  drivers/reset/reset-bcm63xx.c | 134 ++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 145 insertions(+)
>  create mode 100644 drivers/reset/reset-bcm63xx.c


Could you create a bcm directory and then add your reset-bcm63xx.c
file there? I have a pending submission for the BCM63138 reset
controller which is not at all using the same structure and will share
nothing with your driver.

[snip]

> +static int bcm63xx_reset_xlate(struct reset_controller_dev *rcdev,
> +       const struct of_phandle_args *reset_spec)
> +{
> +       struct bcm63xx_reset_priv *priv = to_bcm63xx_reset_priv(rcdev);
> +
> +       if (WARN_ON(reset_spec->args_count != rcdev->of_reset_n_cells))
> +               return -EINVAL;
> +
> +       if (reset_spec->args[0] >= rcdev->nr_resets)
> +               return -EINVAL;

Should not these two things be moved to the core reset controller code
based on the #reset-cells value?

[snip]

> +       if (of_property_read_u32(np, "offset", &priv->offset))
> +               return -EINVAL;
> +
> +       /* valid reset bits */
> +       if (of_property_read_u32(np, "mask", &priv->mask))
> +               priv->mask = 0xffffffff;
> +
> +       priv->rcdev.owner = THIS_MODULE;
> +       priv->rcdev.ops = &bcm63xx_reset_ops;
> +       priv->rcdev.nr_resets = 32;

Should not that come from Device Tree, or be computed based on the
mask property, like hweight_long() or something along these lines?

> +       priv->rcdev.of_node = pdev->dev.of_node;
> +       priv->rcdev.of_reset_n_cells = 1;
> +       priv->rcdev.of_xlate = bcm63xx_reset_xlate;

On 02/12/15 18:03, Florian Fainelli wrote:
> 2015-11-30 12:58 GMT-08:00 Simon Arlott <simon@fire.lp0.eu>:
>> The BCM63xx contains a soft-reset controller activated by setting
>> a bit (that must previously have cleared).
>>
>> Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
>> ---
>>  MAINTAINERS                   |   1 +
>>  drivers/reset/Kconfig         |   9 +++
>>  drivers/reset/Makefile        |   1 +
>>  drivers/reset/reset-bcm63xx.c | 134 ++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 145 insertions(+)
>>  create mode 100644 drivers/reset/reset-bcm63xx.c
> 
> 
> Could you create a bcm directory and then add your reset-bcm63xx.c
> file there? I have a pending submission for the BCM63138 reset
> controller which is not at all using the same structure and will share
> nothing with your driver.
> 

Ok, I'll call it reset-bcm6345.c to avoid confusion.

> 
>> +static int bcm63xx_reset_xlate(struct reset_controller_dev *rcdev,
>> +       const struct of_phandle_args *reset_spec)
>> +{
>> +       struct bcm63xx_reset_priv *priv = to_bcm63xx_reset_priv(rcdev);
>> +
>> +       if (WARN_ON(reset_spec->args_count != rcdev->of_reset_n_cells))
>> +               return -EINVAL;
>> +
>> +       if (reset_spec->args[0] >= rcdev->nr_resets)
>> +               return -EINVAL;
> 
> Should not these two things be moved to the core reset controller code
> based on the #reset-cells value?
> 

This has already been removed from the next version of the patch.

> 
>> +       if (of_property_read_u32(np, "offset", &priv->offset))
>> +               return -EINVAL;
>> +
>> +       /* valid reset bits */
>> +       if (of_property_read_u32(np, "mask", &priv->mask))
>> +               priv->mask = 0xffffffff;
>> +
>> +       priv->rcdev.owner = THIS_MODULE;
>> +       priv->rcdev.ops = &bcm63xx_reset_ops;
>> +       priv->rcdev.nr_resets = 32;
> 
> Should not that come from Device Tree, or be computed based on the
> mask property, like hweight_long() or something along these lines?

The "mask" property has been removed. It will assume 32 resets and rely
on the rest of the DT to only refer to valid bits.