diff mbox

[1/2] documentation: add rockchip spi documentation

Message ID 1403582324-9485-2-git-send-email-addy.ke@rock-chips.com (mailing list archive)
State New, archived
Headers show

Commit Message

addy ke June 24, 2014, 3:58 a.m. UTC
Signed-off-by: addy ke <addy.ke@rock-chips.com>
---
 .../devicetree/bindings/spi/spi-rockchip.txt       | 51 ++++++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-rockchip.txt

Comments

Mark Rutland June 24, 2014, 10:18 a.m. UTC | #1
On Tue, Jun 24, 2014 at 04:58:43AM +0100, addy ke wrote:
> Signed-off-by: addy ke <addy.ke@rock-chips.com>
> ---
>  .../devicetree/bindings/spi/spi-rockchip.txt       | 51 ++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/spi-rockchip.txt
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-rockchip.txt b/Documentation/devicetree/bindings/spi/spi-rockchip.txt
> new file mode 100644
> index 0000000..ce9c881
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/spi-rockchip.txt
> @@ -0,0 +1,51 @@
> +* Rockchip SPI Controller
> +
> +The Rockchip SPI controller is used to interface with various devices such as flash
> +and display controllers using the SPI communication interface.
> +
> +Required SoC Specific Properties:
> +
> +- compatible: should be one of the following.
> +    - rockchip,rk3066-spi: for rk3066, rk3188 and rk3288 platforms.

Are you sure you don't want specifc strings for rk3188 and rk3288 (in
addtion to the common "rockchip,rk3066-spi")?

> +- reg: physical base address of the controller and length of memory mapped
> +       region.
> +- interrupts: The interrupt number to the cpu. The interrupt specifier format
> +              depends on the interrupt controller.
> +- clocks: Must contain an entry for each entry in clock-names.
> +- clock-names: Shall be "spiclk" for the transfer-clock, and "apb_pclk" for
> +			   the peripheral clock.
> +
> +Optional properties:
> +- dmas: DMA specifiers for tx and rx dma. See the DMA client binding,
> +		Documentation/devicetree/bindings/dma/dma.txt
> +- dma-names: DMA request names should include "tx" and "rx" if present.
> +
> +Example:
> +
> +- SoC Specific Portion:
> +
> +	spi0: spi@ff110000 {
> +		compatible = "rockchip,rockchip-spi";

This does not match the description of the compatible property.

> +		reg = <0xff110000 0x1000>;
> +		dmas = <&pdma1 11>, <&pdma1 12>;
> +		dma-names = "tx", "rx";
> +		#address-cells = <1>;
> +		#size-cells = <0>;

These weren't mentioned.

> +		interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&spi0_clk &spi0_tx &spi0_rx &spi0_cs0 &spi0_cs1>;

pinctrl was not mentioned.

> +		clocks = <&cru SCLK_SPI0>, <&cru PCLK_SPI0>;
> +		clock-names = "spiclk", "apb_pclk";
> +		status = "disabled";

Any reason for the status?

> +	};
> +
> +- Board Specific Portion:
> +
> +	&spi0 {
> +		status = "okay";
> +		spi_test@00 {
> +			compatible = "rockchip,spi_test";

Huh?

Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Heiko Stübner June 24, 2014, 10:32 a.m. UTC | #2
Am Dienstag, 24. Juni 2014, 11:18:02 schrieb Mark Rutland:
> On Tue, Jun 24, 2014 at 04:58:43AM +0100, addy ke wrote:
> > Signed-off-by: addy ke <addy.ke@rock-chips.com>
> > ---
> > 
> >  .../devicetree/bindings/spi/spi-rockchip.txt       | 51
> >  ++++++++++++++++++++++ 1 file changed, 51 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/spi/spi-rockchip.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/spi/spi-rockchip.txt
> > b/Documentation/devicetree/bindings/spi/spi-rockchip.txt new file mode
> > 100644
> > index 0000000..ce9c881
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/spi/spi-rockchip.txt
> > @@ -0,0 +1,51 @@
> > +* Rockchip SPI Controller
> > +
> > +The Rockchip SPI controller is used to interface with various devices
> > such as flash +and display controllers using the SPI communication
> > interface.
> > +
> > +Required SoC Specific Properties:
> > +
> > +- compatible: should be one of the following.
> > +    - rockchip,rk3066-spi: for rk3066, rk3188 and rk3288 platforms.
> 
> Are you sure you don't want specifc strings for rk3188 and rk3288 (in
> addtion to the common "rockchip,rk3066-spi")?

Wasn't the convention that "later" platforms that are compatible to an earlier 
one, reuse this compatible string instead of introducing a new one?

From what I've heard so far, the specific spi controller got introduced with 
the rk3066 [earlier SoCs used a different implementation] and didn't change for 
rk3188 and rk3288. Addy may be able to verify this.


> > +- reg: physical base address of the controller and length of memory
> > mapped
> > +       region.
> > +- interrupts: The interrupt number to the cpu. The interrupt specifier
> > format +              depends on the interrupt controller.
> > +- clocks: Must contain an entry for each entry in clock-names.
> > +- clock-names: Shall be "spiclk" for the transfer-clock, and "apb_pclk"
> > for +			   the peripheral clock.
> > +
> > +Optional properties:
> > +- dmas: DMA specifiers for tx and rx dma. See the DMA client binding,
> > +		Documentation/devicetree/bindings/dma/dma.txt
> > +- dma-names: DMA request names should include "tx" and "rx" if present.
> > +
> > +Example:
> > +
> > +- SoC Specific Portion:
> > +
> > +	spi0: spi@ff110000 {
> > +		compatible = "rockchip,rockchip-spi";
> 
> This does not match the description of the compatible property.
> 
> > +		reg = <0xff110000 0x1000>;
> > +		dmas = <&pdma1 11>, <&pdma1 12>;
> > +		dma-names = "tx", "rx";
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> 
> These weren't mentioned.
> 
> > +		interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>;
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&spi0_clk &spi0_tx &spi0_rx &spi0_cs0 &spi0_cs1>;
> 
> pinctrl was not mentioned.
> 
> > +		clocks = <&cru SCLK_SPI0>, <&cru PCLK_SPI0>;
> > +		clock-names = "spiclk", "apb_pclk";
> > +		status = "disabled";
> 
> Any reason for the status?

I guess to have the spi controller only be enabled when a board is using it as 
below. But it may be an implementation detail which could be omitted from the 
binding doc.


> 
> > +	};
> > +
> > +- Board Specific Portion:
> > +
> > +	&spi0 {
> > +		status = "okay";
> > +		spi_test@00 {
> > +			compatible = "rockchip,spi_test";
> 
> Huh?

SPI declares it's devices similar to i2c, so while the example might profit 
from a more casual device, I'm not exactly sure what is the problem here.


Thanks
Heiko

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown June 24, 2014, 10:33 a.m. UTC | #3
On Tue, Jun 24, 2014 at 11:18:02AM +0100, Mark Rutland wrote:
> On Tue, Jun 24, 2014 at 04:58:43AM +0100, addy ke wrote:

> > +- Board Specific Portion:
> > +
> > +	&spi0 {
> > +		status = "okay";
> > +		spi_test@00 {
> > +			compatible = "rockchip,spi_test";

> Huh?

I expect that's just intended as an example of how to set up a device
and they're doing something like use a driver that generates known
patterns.  We'd normally omit such examples from the binding document
since it's generic and covered well enough elsehwere.
Mark Rutland June 24, 2014, 10:47 a.m. UTC | #4
On Tue, Jun 24, 2014 at 11:32:21AM +0100, Heiko Stübner wrote:
> Am Dienstag, 24. Juni 2014, 11:18:02 schrieb Mark Rutland:
> > On Tue, Jun 24, 2014 at 04:58:43AM +0100, addy ke wrote:
> > > Signed-off-by: addy ke <addy.ke@rock-chips.com>
> > > ---
> > > 
> > >  .../devicetree/bindings/spi/spi-rockchip.txt       | 51
> > >  ++++++++++++++++++++++ 1 file changed, 51 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/spi/spi-rockchip.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/spi/spi-rockchip.txt
> > > b/Documentation/devicetree/bindings/spi/spi-rockchip.txt new file mode
> > > 100644
> > > index 0000000..ce9c881
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/spi/spi-rockchip.txt
> > > @@ -0,0 +1,51 @@
> > > +* Rockchip SPI Controller
> > > +
> > > +The Rockchip SPI controller is used to interface with various devices
> > > such as flash +and display controllers using the SPI communication
> > > interface.
> > > +
> > > +Required SoC Specific Properties:
> > > +
> > > +- compatible: should be one of the following.
> > > +    - rockchip,rk3066-spi: for rk3066, rk3188 and rk3288 platforms.
> > 
> > Are you sure you don't want specifc strings for rk3188 and rk3288 (in
> > addtion to the common "rockchip,rk3066-spi")?
> 
> Wasn't the convention that "later" platforms that are compatible to an earlier 
> one, reuse this compatible string instead of introducing a new one?

That's why I said in addition to the common one. I'd only expect the
driver to look for "rockchip,rk3066-spi", but a DTB could have:

	compatible = "rockchip,rk3188-spi", "rockchip,rk3066-spi";

Seeding the DTBs with the extra strings early makes it more likely that
we can rely on them later. If we don't happen to need them they only
clutter some DTBs.

> From what I've heard so far, the specific spi controller got introduced with 
> the rk3066 [earlier SoCs used a different implementation] and didn't change for 
> rk3188 and rk3288. Addy may be able to verify this.
> 
> 
> > > +- reg: physical base address of the controller and length of memory
> > > mapped
> > > +       region.
> > > +- interrupts: The interrupt number to the cpu. The interrupt specifier
> > > format +              depends on the interrupt controller.
> > > +- clocks: Must contain an entry for each entry in clock-names.
> > > +- clock-names: Shall be "spiclk" for the transfer-clock, and "apb_pclk"
> > > for +			   the peripheral clock.
> > > +
> > > +Optional properties:
> > > +- dmas: DMA specifiers for tx and rx dma. See the DMA client binding,
> > > +		Documentation/devicetree/bindings/dma/dma.txt
> > > +- dma-names: DMA request names should include "tx" and "rx" if present.
> > > +
> > > +Example:
> > > +
> > > +- SoC Specific Portion:
> > > +
> > > +	spi0: spi@ff110000 {
> > > +		compatible = "rockchip,rockchip-spi";
> > 
> > This does not match the description of the compatible property.
> > 
> > > +		reg = <0xff110000 0x1000>;
> > > +		dmas = <&pdma1 11>, <&pdma1 12>;
> > > +		dma-names = "tx", "rx";
> > > +		#address-cells = <1>;
> > > +		#size-cells = <0>;
> > 
> > These weren't mentioned.
> > 
> > > +		interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>;
> > > +		pinctrl-names = "default";
> > > +		pinctrl-0 = <&spi0_clk &spi0_tx &spi0_rx &spi0_cs0 &spi0_cs1>;
> > 
> > pinctrl was not mentioned.
> > 
> > > +		clocks = <&cru SCLK_SPI0>, <&cru PCLK_SPI0>;
> > > +		clock-names = "spiclk", "apb_pclk";
> > > +		status = "disabled";
> > 
> > Any reason for the status?
> 
> I guess to have the spi controller only be enabled when a board is using it as 
> below. But it may be an implementation detail which could be omitted from the 
> binding doc.

Yes please :)

> 
> > 
> > > +	};
> > > +
> > > +- Board Specific Portion:
> > > +
> > > +	&spi0 {
> > > +		status = "okay";
> > > +		spi_test@00 {
> > > +			compatible = "rockchip,spi_test";
> > 
> > Huh?
> 
> SPI declares it's devices similar to i2c, so while the example might profit 
> from a more casual device, I'm not exactly sure what is the problem here.

Sure. Given that this is a known pattern for SPI controllers, do we need
this in each binding doc?

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

Patch

diff --git a/Documentation/devicetree/bindings/spi/spi-rockchip.txt b/Documentation/devicetree/bindings/spi/spi-rockchip.txt
new file mode 100644
index 0000000..ce9c881
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spi-rockchip.txt
@@ -0,0 +1,51 @@ 
+* Rockchip SPI Controller
+
+The Rockchip SPI controller is used to interface with various devices such as flash
+and display controllers using the SPI communication interface.
+
+Required SoC Specific Properties:
+
+- compatible: should be one of the following.
+    - rockchip,rk3066-spi: for rk3066, rk3188 and rk3288 platforms.
+- reg: physical base address of the controller and length of memory mapped
+       region.
+- interrupts: The interrupt number to the cpu. The interrupt specifier format
+              depends on the interrupt controller.
+- clocks: Must contain an entry for each entry in clock-names.
+- clock-names: Shall be "spiclk" for the transfer-clock, and "apb_pclk" for
+			   the peripheral clock.
+
+Optional properties:
+- dmas: DMA specifiers for tx and rx dma. See the DMA client binding,
+		Documentation/devicetree/bindings/dma/dma.txt
+- dma-names: DMA request names should include "tx" and "rx" if present.
+
+Example:
+
+- SoC Specific Portion:
+
+	spi0: spi@ff110000 {
+		compatible = "rockchip,rockchip-spi";
+		reg = <0xff110000 0x1000>;
+		dmas = <&pdma1 11>, <&pdma1 12>;
+		dma-names = "tx", "rx";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&spi0_clk &spi0_tx &spi0_rx &spi0_cs0 &spi0_cs1>;
+		clocks = <&cru SCLK_SPI0>, <&cru PCLK_SPI0>;
+		clock-names = "spiclk", "apb_pclk";
+		status = "disabled";
+	};
+
+- Board Specific Portion:
+
+	&spi0 {
+		status = "okay";
+		spi_test@00 {
+			compatible = "rockchip,spi_test";
+			reg = <0>;
+			spi-max-frequency = <24000000>;
+		};
+	};