[PATCH V2 0/2] sata_rcar: Add Gen2 PHY support and document existing DT bindings

[PATCH V2 0/2] sata_rcar: Add Gen2 PHY support and document existing DT bindings

[Valentine Barshak]

This adds SATA R-Car Gen2 PHY support and documents existing DT bindings.
Changes from the previous version:
* Minor whitespace clean-up to make both array terminators "{ },".

Valentine Barshak (2):
  sata_rcar: Add R-Car Gen2 SATA PHY support
  dt: Document sata_rcar bindings

 .../devicetree/bindings/ata/sata_rcar.txt          |  18 ++++
 drivers/ata/sata_rcar.c                            | 118 ++++++++++++++++++---
 2 files changed, 120 insertions(+), 16 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/ata/sata_rcar.txt

-- 
1.8.3.1

[PATCH V2 1/2] sata_rcar: Add R-Car Gen2 SATA PHY support

[Valentine Barshak]

R-Car Gen2 SoCs have a different PHY which is not compatible
with the older R-Car H1 (R8A7779) version.
This adds OF/platform device id tables and PHY initialization
callbacks for the following Gen2 SoCs:
  * R-Car H2: R8A7790;
  * R-Car M2: R8A7791.

PHY initialization method is chosen based on the device id.
Default PHY settings are applied for Gen2 SoCs, which should
suit the Gen2 boards available.

While at it, this also adds "sata-r8a7779" compatibility string
for R8A7779 SATA, while keeping the old one for compatibility.

Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
---
 drivers/ata/sata_rcar.c | 118 +++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 102 insertions(+), 16 deletions(-)

diff --git a/drivers/ata/sata_rcar.c b/drivers/ata/sata_rcar.c
index c2d95e9..231559f 100644
--- a/drivers/ata/sata_rcar.c
+++ b/drivers/ata/sata_rcar.c
@@ -15,6 +15,7 @@
 #include <linux/module.h>
 #include <linux/ata.h>
 #include <linux/libata.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/clk.h>
 #include <linux/err.h>
@@ -123,12 +124,37 @@
 
 #define SATA_RCAR_DMA_BOUNDARY		0x1FFFFFFEUL
 
+/* Gen2 Physical Layer Control Registers */
+#define RCAR_GEN2_PHY_CTL1_REG		0x1704
+#define RCAR_GEN2_PHY_CTL1		0x34180002
+#define RCAR_GEN2_PHY_CTL1_SS		0xC180	/* Spread Spectrum */
+
+#define RCAR_GEN2_PHY_CTL2_REG		0x170C
+#define RCAR_GEN2_PHY_CTL2		0x00002303
+
+#define RCAR_GEN2_PHY_CTL3_REG		0x171C
+#define RCAR_GEN2_PHY_CTL3		0x000B0194
+
+#define RCAR_GEN2_PHY_CTL4_REG		0x1724
+#define RCAR_GEN2_PHY_CTL4		0x00030994
+
+#define RCAR_GEN2_PHY_CTL5_REG		0x1740
+#define RCAR_GEN2_PHY_CTL5		0x03004001
+#define RCAR_GEN2_PHY_CTL5_DC		BIT(1)	/* DC connection */
+#define RCAR_GEN2_PHY_CTL5_TR		BIT(2)	/* Termination Resistor */
+
+enum sata_rcar_type {
+	RCAR_GEN1_SATA,
+	RCAR_GEN2_SATA,
+};
+
 struct sata_rcar_priv {
 	void __iomem *base;
 	struct clk *clk;
+	enum sata_rcar_type type;
 };
 
-static void sata_rcar_phy_initialize(struct sata_rcar_priv *priv)
+static void sata_rcar_gen1_phy_preinit(struct sata_rcar_priv *priv)
 {
 	void __iomem *base = priv->base;
 
@@ -141,8 +167,8 @@ static void sata_rcar_phy_initialize(struct sata_rcar_priv *priv)
 	iowrite32(0, base + SATAPHYRESET_REG);
 }
 
-static void sata_rcar_phy_write(struct sata_rcar_priv *priv, u16 reg, u32 val,
-				int group)
+static void sata_rcar_gen1_phy_write(struct sata_rcar_priv *priv, u16 reg,
+				     u32 val, int group)
 {
 	void __iomem *base = priv->base;
 	int timeout;
@@ -170,6 +196,29 @@ static void sata_rcar_phy_write(struct sata_rcar_priv *priv, u16 reg, u32 val,
 	iowrite32(0, base + SATAPHYADDR_REG);
 }
 
+static void sata_rcar_gen1_phy_init(struct sata_rcar_priv *priv)
+{
+	sata_rcar_gen1_phy_preinit(priv);
+	sata_rcar_gen1_phy_write(priv, SATAPCTLR1_REG, 0x00200188, 0);
+	sata_rcar_gen1_phy_write(priv, SATAPCTLR1_REG, 0x00200188, 1);
+	sata_rcar_gen1_phy_write(priv, SATAPCTLR3_REG, 0x0000A061, 0);
+	sata_rcar_gen1_phy_write(priv, SATAPCTLR2_REG, 0x20000000, 0);
+	sata_rcar_gen1_phy_write(priv, SATAPCTLR2_REG, 0x20000000, 1);
+	sata_rcar_gen1_phy_write(priv, SATAPCTLR4_REG, 0x28E80000, 0);
+}
+
+static void sata_rcar_gen2_phy_init(struct sata_rcar_priv *priv)
+{
+	void __iomem *base = priv->base;
+
+	iowrite32(RCAR_GEN2_PHY_CTL1, base + RCAR_GEN2_PHY_CTL1_REG);
+	iowrite32(RCAR_GEN2_PHY_CTL2, base + RCAR_GEN2_PHY_CTL2_REG);
+	iowrite32(RCAR_GEN2_PHY_CTL3, base + RCAR_GEN2_PHY_CTL3_REG);
+	iowrite32(RCAR_GEN2_PHY_CTL4, base + RCAR_GEN2_PHY_CTL4_REG);
+	iowrite32(RCAR_GEN2_PHY_CTL5 | RCAR_GEN2_PHY_CTL5_DC |
+		  RCAR_GEN2_PHY_CTL5_TR, base + RCAR_GEN2_PHY_CTL5_REG);
+}
+
 static void sata_rcar_freeze(struct ata_port *ap)
 {
 	struct sata_rcar_priv *priv = ap->host->private_data;
@@ -738,13 +787,17 @@ static void sata_rcar_init_controller(struct ata_host *host)
 	u32 val;
 
 	/* reset and setup phy */
-	sata_rcar_phy_initialize(priv);
-	sata_rcar_phy_write(priv, SATAPCTLR1_REG, 0x00200188, 0);
-	sata_rcar_phy_write(priv, SATAPCTLR1_REG, 0x00200188, 1);
-	sata_rcar_phy_write(priv, SATAPCTLR3_REG, 0x0000A061, 0);
-	sata_rcar_phy_write(priv, SATAPCTLR2_REG, 0x20000000, 0);
-	sata_rcar_phy_write(priv, SATAPCTLR2_REG, 0x20000000, 1);
-	sata_rcar_phy_write(priv, SATAPCTLR4_REG, 0x28E80000, 0);
+	switch (priv->type) {
+	case RCAR_GEN1_SATA:
+		sata_rcar_gen1_phy_init(priv);
+		break;
+	case RCAR_GEN2_SATA:
+		sata_rcar_gen2_phy_init(priv);
+		break;
+	default:
+		dev_warn(host->dev, "SATA phy is not initialized\n");
+		break;
+	}
 
 	/* SATA-IP reset state */
 	val = ioread32(base + ATAPI_CONTROL1_REG);
@@ -770,8 +823,40 @@ static void sata_rcar_init_controller(struct ata_host *host)
 	iowrite32(ATAPI_INT_ENABLE_SATAINT, base + ATAPI_INT_ENABLE_REG);
 }
 
+static struct of_device_id sata_rcar_match[] = {
+	{
+		/* Deprecated by "renesas,sata-r8a7779" */
+		.compatible = "renesas,rcar-sata",
+		.data = (void *)RCAR_GEN1_SATA,
+	},
+	{
+		.compatible = "renesas,sata-r8a7779",
+		.data = (void *)RCAR_GEN1_SATA,
+	},
+	{
+		.compatible = "renesas,sata-r8a7790",
+		.data = (void *)RCAR_GEN2_SATA
+	},
+	{
+		.compatible = "renesas,sata-r8a7791",
+		.data = (void *)RCAR_GEN2_SATA
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, sata_rcar_match);
+
+static const struct platform_device_id sata_rcar_id_table[] = {
+	{ "sata_rcar", RCAR_GEN1_SATA }, /* Deprecated by "sata-r8a7779" */
+	{ "sata-r8a7779", RCAR_GEN1_SATA },
+	{ "sata-r8a7790", RCAR_GEN2_SATA },
+	{ "sata-r8a7791", RCAR_GEN2_SATA },
+	{ },
+};
+MODULE_DEVICE_TABLE(platform, sata_rcar_id_table);
+
 static int sata_rcar_probe(struct platform_device *pdev)
 {
+	const struct of_device_id *of_id;
 	struct ata_host *host;
 	struct sata_rcar_priv *priv;
 	struct resource *mem;
@@ -787,6 +872,12 @@ static int sata_rcar_probe(struct platform_device *pdev)
 	if (!priv)
 		return -ENOMEM;
 
+	of_id = of_match_device(sata_rcar_match, &pdev->dev);
+	if (of_id)
+		priv->type = (enum sata_rcar_type)of_id->data;
+	else
+		priv->type = platform_get_device_id(pdev)->driver_data;
+
 	priv->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(priv->clk)) {
 		dev_err(&pdev->dev, "failed to get access to sata clock\n");
@@ -892,15 +983,10 @@ static const struct dev_pm_ops sata_rcar_pm_ops = {
 };
 #endif
 
-static struct of_device_id sata_rcar_match[] = {
-	{ .compatible = "renesas,rcar-sata", },
-	{},
-};
-MODULE_DEVICE_TABLE(of, sata_rcar_match);
-
 static struct platform_driver sata_rcar_driver = {
 	.probe		= sata_rcar_probe,
 	.remove		= sata_rcar_remove,
+	.id_table	= sata_rcar_id_table,
 	.driver = {
 		.name		= DRV_NAME,
 		.owner		= THIS_MODULE,
-- 
1.8.3.1

Re: [PATCH V2 1/2] sata_rcar: Add R-Car Gen2 SATA PHY support

[Valentine]

On 11/08/2013 12:04 AM, Valentine Barshak wrote:
> R-Car Gen2 SoCs have a different PHY which is not compatible
> with the older R-Car H1 (R8A7779) version.
> This adds OF/platform device id tables and PHY initialization
> callbacks for the following Gen2 SoCs:
>    * R-Car H2: R8A7790;
>    * R-Car M2: R8A7791.
>
> PHY initialization method is chosen based on the device id.
> Default PHY settings are applied for Gen2 SoCs, which should
> suit the Gen2 boards available.
>
> While at it, this also adds "sata-r8a7779" compatibility string
> for R8A7779 SATA, while keeping the old one for compatibility.
>
> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>

Tejun,
are you OK with applying this?

All the comments have been addressed in the previous version.
I've just decided to give it a minor white-space clean-up here
to make both array terminators look identical: "{ },".

I wonder if it's not to late for the 3.13.

Thanks,
Val.

[PATCH V2 2/2] dt: Document sata_rcar bindings

[Valentine Barshak]

These bindings can be used to register SATA devices found on R-Car SoC.

Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
Acked-by: Kumar Gala <galak@codeaurora.org>
---
 Documentation/devicetree/bindings/ata/sata_rcar.txt | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ata/sata_rcar.txt

diff --git a/Documentation/devicetree/bindings/ata/sata_rcar.txt b/Documentation/devicetree/bindings/ata/sata_rcar.txt
new file mode 100644
index 0000000..d6b20a6
--- /dev/null
+++ b/Documentation/devicetree/bindings/ata/sata_rcar.txt
@@ -0,0 +1,18 @@
+* Renesas R-Car SATA
+
+Required properties:
+- compatible		: should contain one of the following:
+			  - "renesas,sata-r8a7779" for R-Car H1
+			  - "renesas,sata-r8a7790" for R-Car H2
+			  - "renesas,sata-r8a7791" for R-Car M2
+- reg			: offset and length of the SATA registers;
+- interrupts		: must consist of one interrupt specifier.
+
+Example:
+
+sata: sata@fc600000 {
+	compatible = "renesas,sata-r8a7779";
+	reg = <0xfc600000 0x2000>;
+	interrupt-parent = <&gic>;
+	interrupts = <0 100 0x4>;
+};
-- 
1.8.3.1

Re: [PATCH V2 2/2] dt: Document sata_rcar bindings

[Laurent Pinchart]

Hi Valentine,

Thank you for the patch. Two small comments below.

On Friday 08 November 2013 00:04:02 Valentine Barshak wrote:
> These bindings can be used to register SATA devices found on R-Car SoC.
> 
> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
> Acked-by: Kumar Gala <galak@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/ata/sata_rcar.txt | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/ata/sata_rcar.txt
> 
> diff --git a/Documentation/devicetree/bindings/ata/sata_rcar.txt
> b/Documentation/devicetree/bindings/ata/sata_rcar.txt new file mode 100644
> index 0000000..d6b20a6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ata/sata_rcar.txt
> @@ -0,0 +1,18 @@
> +* Renesas R-Car SATA
> +
> +Required properties:
> +- compatible		: should contain one of the following:
> +			  - "renesas,sata-r8a7779" for R-Car H1
> +			  - "renesas,sata-r8a7790" for R-Car H2
> +			  - "renesas,sata-r8a7791" for R-Car M2
> +- reg			: offset and length of the SATA registers;

Maybe address instead of offset ?

> +- interrupts		: must consist of one interrupt specifier.
> +
> +Example:
> +
> +sata: sata@fc600000 {
> +	compatible = "renesas,sata-r8a7779";
> +	reg = <0xfc600000 0x2000>;
> +	interrupt-parent = <&gic>;
> +	interrupts = <0 100 0x4>;

Please use IRQ_TYPE_LEVEL_HIGH instead of 0x4.

> +};
-- 
Regards,

Laurent Pinchart

Re: [PATCH V2 2/2] dt: Document sata_rcar bindings

[Kuninori Morimoto]

Hi Laurent, Simon

One question

> > +- interrupts		: must consist of one interrupt specifier.
> > +
> > +Example:
> > +
> > +sata: sata@fc600000 {
> > +	compatible = "renesas,sata-r8a7779";
> > +	reg = <0xfc600000 0x2000>;
> > +	interrupt-parent = <&gic>;
> > +	interrupts = <0 100 0x4>;
> 
> Please use IRQ_TYPE_LEVEL_HIGH instead of 0x4.

Now, our many SoC codes are using 0x4 in "interrupts".
Do you think we should use IRQ_TYPE_LEVEL_HIGH ?

Best regards
---
Kuninori Morimoto

Re: [PATCH V2 2/2] dt: Document sata_rcar bindings

[Simon Horman]

On Thu, Nov 07, 2013 at 10:27:08PM -0800, Kuninori Morimoto wrote:
> 
> Hi Laurent, Simon
> 
> One question
> 
> > > +- interrupts		: must consist of one interrupt specifier.
> > > +
> > > +Example:
> > > +
> > > +sata: sata@fc600000 {
> > > +	compatible = "renesas,sata-r8a7779";
> > > +	reg = <0xfc600000 0x2000>;
> > > +	interrupt-parent = <&gic>;
> > > +	interrupts = <0 100 0x4>;
> > 
> > Please use IRQ_TYPE_LEVEL_HIGH instead of 0x4.
> 
> Now, our many SoC codes are using 0x4 in "interrupts".
> Do you think we should use IRQ_TYPE_LEVEL_HIGH ?

Personally I think that would be nice.

Re: [PATCH V2 2/2] dt: Document sata_rcar bindings

[Valentine]

On 11/08/2013 05:02 AM, Laurent Pinchart wrote:
> Hi Valentine,
>

Hi Laurent,

> Thank you for the patch. Two small comments below.
>
> On Friday 08 November 2013 00:04:02 Valentine Barshak wrote:
>> These bindings can be used to register SATA devices found on R-Car SoC.
>>
>> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
>> Acked-by: Kumar Gala <galak@codeaurora.org>
>> ---
>>   Documentation/devicetree/bindings/ata/sata_rcar.txt | 18 ++++++++++++++++++
>> 1 file changed, 18 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/ata/sata_rcar.txt
>>
>> diff --git a/Documentation/devicetree/bindings/ata/sata_rcar.txt
>> b/Documentation/devicetree/bindings/ata/sata_rcar.txt new file mode 100644
>> index 0000000..d6b20a6
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/ata/sata_rcar.txt
>> @@ -0,0 +1,18 @@
>> +* Renesas R-Car SATA
>> +
>> +Required properties:
>> +- compatible		: should contain one of the following:
>> +			  - "renesas,sata-r8a7779" for R-Car H1
>> +			  - "renesas,sata-r8a7790" for R-Car H2
>> +			  - "renesas,sata-r8a7791" for R-Car M2
>> +- reg			: offset and length of the SATA registers;
>
> Maybe address instead of offset ?

OK.

>
>> +- interrupts		: must consist of one interrupt specifier.
>> +
>> +Example:
>> +
>> +sata: sata@fc600000 {
>> +	compatible = "renesas,sata-r8a7779";
>> +	reg = <0xfc600000 0x2000>;
>> +	interrupt-parent = <&gic>;
>> +	interrupts = <0 100 0x4>;
>
> Please use IRQ_TYPE_LEVEL_HIGH instead of 0x4.

OK, I'll do that.

Please, note that all the Renesas dts[i] files do not include <dt-bindings/interrupt-controller/irq.h>
Thus, blindly following this bindings documentation when adding new SATA nodes to the existing
r8a7790.dtsi, for example, will cause DT compilation error.

>
>> +};

Thanks,
Val.

Re: [PATCH V2 2/2] dt: Document sata_rcar bindings

[Valentine]

On 11/08/2013 03:26 PM, Valentine wrote:
> On 11/08/2013 05:02 AM, Laurent Pinchart wrote:
>> Hi Valentine,
>>
>
> Hi Laurent,
>
>> Thank you for the patch. Two small comments below.
>>
>> On Friday 08 November 2013 00:04:02 Valentine Barshak wrote:
>>> These bindings can be used to register SATA devices found on R-Car SoC.
>>>
>>> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
>>> Acked-by: Kumar Gala <galak@codeaurora.org>
>>> ---
>>>   Documentation/devicetree/bindings/ata/sata_rcar.txt | 18 ++++++++++++++++++
>>> 1 file changed, 18 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/ata/sata_rcar.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/ata/sata_rcar.txt
>>> b/Documentation/devicetree/bindings/ata/sata_rcar.txt new file mode 100644
>>> index 0000000..d6b20a6
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/ata/sata_rcar.txt
>>> @@ -0,0 +1,18 @@
>>> +* Renesas R-Car SATA
>>> +
>>> +Required properties:
>>> +- compatible        : should contain one of the following:
>>> +              - "renesas,sata-r8a7779" for R-Car H1
>>> +              - "renesas,sata-r8a7790" for R-Car H2
>>> +              - "renesas,sata-r8a7791" for R-Car M2
>>> +- reg            : offset and length of the SATA registers;
>>
>> Maybe address instead of offset ?
>
> OK.
>
>>
>>> +- interrupts        : must consist of one interrupt specifier.
>>> +
>>> +Example:
>>> +
>>> +sata: sata@fc600000 {
>>> +    compatible = "renesas,sata-r8a7779";
>>> +    reg = <0xfc600000 0x2000>;
>>> +    interrupt-parent = <&gic>;
>>> +    interrupts = <0 100 0x4>;
>>
>> Please use IRQ_TYPE_LEVEL_HIGH instead of 0x4.
>
> OK, I'll do that.
>
> Please, note that all the Renesas dts[i] files do not include <dt-bindings/interrupt-controller/irq.h>
> Thus, blindly following this bindings documentation when adding new SATA nodes to the existing
> r8a7790.dtsi, for example, will cause DT compilation error.
>

BTW, we'll also have to switch to #include instead of /include/ to make it work.

Thanks,
Val.

Re: [PATCH V2 2/2] dt: Document sata_rcar bindings

[Laurent Pinchart]

Hi Valentine,

On Friday 08 November 2013 15:52:45 Valentine wrote:
> On 11/08/2013 03:26 PM, Valentine wrote:
> > On 11/08/2013 05:02 AM, Laurent Pinchart wrote:
> >> On Friday 08 November 2013 00:04:02 Valentine Barshak wrote:
> >>> These bindings can be used to register SATA devices found on R-Car SoC.
> >>> 
> >>> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
> >>> Acked-by: Kumar Gala <galak@codeaurora.org>
> >>> ---
> >>> 
> >>>   Documentation/devicetree/bindings/ata/sata_rcar.txt | 18 +++++++++++++
> >>> 
> >>> 1 file changed, 18 insertions(+)
> >>> 
> >>>   create mode 100644 Documentation/devicetree/bindings/ata/sata_rcar.txt
> >>> 
> >>> diff --git a/Documentation/devicetree/bindings/ata/sata_rcar.txt
> >>> b/Documentation/devicetree/bindings/ata/sata_rcar.txt new file mode
> >>> 100644
> >>> index 0000000..d6b20a6
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/ata/sata_rcar.txt
> >>> @@ -0,0 +1,18 @@
> >>> +* Renesas R-Car SATA
> >>> +
> >>> +Required properties:
> >>> +- compatible        : should contain one of the following:
> >>> +              - "renesas,sata-r8a7779" for R-Car H1
> >>> +              - "renesas,sata-r8a7790" for R-Car H2
> >>> +              - "renesas,sata-r8a7791" for R-Car M2
> >>> +- reg            : offset and length of the SATA registers;
> >> 
> >> Maybe address instead of offset ?
> > 
> > OK.
> > 
> >>> +- interrupts        : must consist of one interrupt specifier.
> >>> +
> >>> +Example:
> >>> +
> >>> +sata: sata@fc600000 {
> >>> +    compatible = "renesas,sata-r8a7779";
> >>> +    reg = <0xfc600000 0x2000>;
> >>> +    interrupt-parent = <&gic>;
> >>> +    interrupts = <0 100 0x4>;
> >> 
> >> Please use IRQ_TYPE_LEVEL_HIGH instead of 0x4.
> > 
> > OK, I'll do that.
> > 
> > Please, note that all the Renesas dts[i] files do not include
> > <dt-bindings/interrupt-controller/irq.h> Thus, blindly following this
> > bindings documentation when adding new SATA nodes to the existing
> > r8a7790.dtsi, for example, will cause DT compilation error.
> 
> BTW, we'll also have to switch to #include instead of /include/ to make it
> work.

Sure, of course.

I was actually thinking about sending a patch to convert our DT sources to use 
the IRQ_* macros. Morimoto-san, are you working on that already ? If not I'll 
do it.

Simon, would you like one patch per SoC or would a single patch do ? Only 
.dts(i) files will be touched.

-- 
Regards,

Laurent Pinchart

Re: [PATCH V2 2/2] dt: Document sata_rcar bindings

[Simon Horman]

On Fri, Nov 08, 2013 at 04:31:03PM +0100, Laurent Pinchart wrote:
> Hi Valentine,
> 
> On Friday 08 November 2013 15:52:45 Valentine wrote:
> > On 11/08/2013 03:26 PM, Valentine wrote:
> > > On 11/08/2013 05:02 AM, Laurent Pinchart wrote:
> > >> On Friday 08 November 2013 00:04:02 Valentine Barshak wrote:
> > >>> These bindings can be used to register SATA devices found on R-Car SoC.
> > >>> 
> > >>> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
> > >>> Acked-by: Kumar Gala <galak@codeaurora.org>
> > >>> ---
> > >>> 
> > >>>   Documentation/devicetree/bindings/ata/sata_rcar.txt | 18 +++++++++++++
> > >>> 
> > >>> 1 file changed, 18 insertions(+)
> > >>> 
> > >>>   create mode 100644 Documentation/devicetree/bindings/ata/sata_rcar.txt
> > >>> 
> > >>> diff --git a/Documentation/devicetree/bindings/ata/sata_rcar.txt
> > >>> b/Documentation/devicetree/bindings/ata/sata_rcar.txt new file mode
> > >>> 100644
> > >>> index 0000000..d6b20a6
> > >>> --- /dev/null
> > >>> +++ b/Documentation/devicetree/bindings/ata/sata_rcar.txt
> > >>> @@ -0,0 +1,18 @@
> > >>> +* Renesas R-Car SATA
> > >>> +
> > >>> +Required properties:
> > >>> +- compatible        : should contain one of the following:
> > >>> +              - "renesas,sata-r8a7779" for R-Car H1
> > >>> +              - "renesas,sata-r8a7790" for R-Car H2
> > >>> +              - "renesas,sata-r8a7791" for R-Car M2
> > >>> +- reg            : offset and length of the SATA registers;
> > >> 
> > >> Maybe address instead of offset ?
> > > 
> > > OK.
> > > 
> > >>> +- interrupts        : must consist of one interrupt specifier.
> > >>> +
> > >>> +Example:
> > >>> +
> > >>> +sata: sata@fc600000 {
> > >>> +    compatible = "renesas,sata-r8a7779";
> > >>> +    reg = <0xfc600000 0x2000>;
> > >>> +    interrupt-parent = <&gic>;
> > >>> +    interrupts = <0 100 0x4>;
> > >> 
> > >> Please use IRQ_TYPE_LEVEL_HIGH instead of 0x4.
> > > 
> > > OK, I'll do that.
> > > 
> > > Please, note that all the Renesas dts[i] files do not include
> > > <dt-bindings/interrupt-controller/irq.h> Thus, blindly following this
> > > bindings documentation when adding new SATA nodes to the existing
> > > r8a7790.dtsi, for example, will cause DT compilation error.
> > 
> > BTW, we'll also have to switch to #include instead of /include/ to make it
> > work.
> 
> Sure, of course.
> 
> I was actually thinking about sending a patch to convert our DT sources to use 
> the IRQ_* macros. Morimoto-san, are you working on that already ? If not I'll 
> do it.
> 
> Simon, would you like one patch per SoC or would a single patch do ? Only 
> .dts(i) files will be touched.

I would prefer one patch per SoC.
The reason is that in general that approach makes backporting easier.

Re: [PATCH V2 2/2] dt: Document sata_rcar bindings

[Laurent Pinchart]

Hi Simon,

On Tuesday 12 November 2013 12:42:58 Simon Horman wrote:
> On Fri, Nov 08, 2013 at 04:31:03PM +0100, Laurent Pinchart wrote:
> > On Friday 08 November 2013 15:52:45 Valentine wrote:
> > > On 11/08/2013 03:26 PM, Valentine wrote:
> > > > On 11/08/2013 05:02 AM, Laurent Pinchart wrote:
> > > >> On Friday 08 November 2013 00:04:02 Valentine Barshak wrote:
> > > >>> These bindings can be used to register SATA devices found on R-Car
> > > >>> SoC.
> > > >>> 
> > > >>> Signed-off-by: Valentine Barshak
> > > >>> <valentine.barshak@cogentembedded.com>
> > > >>> Acked-by: Kumar Gala <galak@codeaurora.org>
> > > >>> ---
> > > >>> 
> > > >>> Documentation/devicetree/bindings/ata/sata_rcar.txt | 18 +++++++++++
> > > >>> 1 file changed, 18 insertions(+)
> > > >>> 
> > > >>>   create mode 100644
> > > >>>   Documentation/devicetree/bindings/ata/sata_rcar.txt
> > > >>> 
> > > >>> diff --git a/Documentation/devicetree/bindings/ata/sata_rcar.txt
> > > >>> b/Documentation/devicetree/bindings/ata/sata_rcar.txt new file mode
> > > >>> 100644
> > > >>> index 0000000..d6b20a6
> > > >>> --- /dev/null
> > > >>> +++ b/Documentation/devicetree/bindings/ata/sata_rcar.txt
> > > >>> @@ -0,0 +1,18 @@
> > > >>> +* Renesas R-Car SATA
> > > >>> +
> > > >>> +Required properties:
> > > >>> +- compatible        : should contain one of the following:
> > > >>> +              - "renesas,sata-r8a7779" for R-Car H1
> > > >>> +              - "renesas,sata-r8a7790" for R-Car H2
> > > >>> +              - "renesas,sata-r8a7791" for R-Car M2
> > > >>> +- reg            : offset and length of the SATA registers;
> > > >> 
> > > >> Maybe address instead of offset ?
> > > > 
> > > > OK.
> > > > 
> > > >>> +- interrupts        : must consist of one interrupt specifier.
> > > >>> +
> > > >>> +Example:
> > > >>> +
> > > >>> +sata: sata@fc600000 {
> > > >>> +    compatible = "renesas,sata-r8a7779";
> > > >>> +    reg = <0xfc600000 0x2000>;
> > > >>> +    interrupt-parent = <&gic>;
> > > >>> +    interrupts = <0 100 0x4>;
> > > >> 
> > > >> Please use IRQ_TYPE_LEVEL_HIGH instead of 0x4.
> > > > 
> > > > OK, I'll do that.
> > > > 
> > > > Please, note that all the Renesas dts[i] files do not include
> > > > <dt-bindings/interrupt-controller/irq.h> Thus, blindly following this
> > > > bindings documentation when adding new SATA nodes to the existing
> > > > r8a7790.dtsi, for example, will cause DT compilation error.
> > > 
> > > BTW, we'll also have to switch to #include instead of /include/ to make
> > > it work.
> > 
> > Sure, of course.
> > 
> > I was actually thinking about sending a patch to convert our DT sources to
> > use the IRQ_* macros. Morimoto-san, are you working on that already ? If
> > not I'll do it.
> > 
> > Simon, would you like one patch per SoC or would a single patch do ? Only
> > .dts(i) files will be touched.
> 
> I would prefer one patch per SoC.
> The reason is that in general that approach makes backporting easier.

I've already posted the series on Saturday, it's called "[PATCH 0/7] ARM: 
shmobile: Use interrupt macros in DT files". Could you please have a look ? I 
can split patches 1/7 to 3/7 in 28 patches and repost if you would like me to, 
but that sounds like a really high number of patches for something so simple, 
especially for patch 1/7.

-- 
Regards,

Laurent Pinchart