diff mbox

[1/3] doc: dt: add documentation for Mediatek spi-nor controller

Message ID 1441705796-11365-2-git-send-email-bayi.cheng@mediatek.com (mailing list archive)
State New, archived
Headers show

Commit Message

Bayi Cheng Sept. 8, 2015, 9:49 a.m. UTC 
Add device tree binding documentation for serial flash with
Mediatek serial flash controller

Signed-off-by: Bayi Cheng <bayi.cheng@mediatek.com>
---
 Documentation/devicetree/bindings/mtd/mtk_nor.txt | 25 +++++++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/mtk_nor.txt

Comments

Jagan Teki Sept. 8, 2015, 11:34 a.m. UTC | #1 
On 8 September 2015 at 15:19, Bayi Cheng <bayi.cheng@mediatek.com> wrote:
> Add device tree binding documentation for serial flash with
> Mediatek serial flash controller
>
> Signed-off-by: Bayi Cheng <bayi.cheng@mediatek.com>
> ---
>  Documentation/devicetree/bindings/mtd/mtk_nor.txt | 25 +++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/mtk_nor.txt
>
> diff --git a/Documentation/devicetree/bindings/mtd/mtk_nor.txt b/Documentation/devicetree/bindings/mtd/mtk_nor.txt
> new file mode 100644
> index 0000000..0eca0cd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/mtk_nor.txt

IMHO - looks like the name of the driver file not resembles controller
driver, usually spi-nor framework has a name include 'nor' and related
controller drivers uses simple notion with vendor/soc_name-quadspi or
something similar.

cadence-quadspi.c
fsl-quadspi.c

Simple suggestion - mtk-quadspi.c (since it supports quad)

> @@ -0,0 +1,25 @@
> +* MTD SPI nor driver for MTK MT81xx (and similar) serial flash controller
> +
> +MTK MT81xx serial flash controller is designed for serial Flash device.
> +It supports one Flash device with signal mode, dual mode and quad mode.
> +
> +Required properties:
> +- compatible: should be "mediatek,mt8173-nor";
> +- reg: physical base address and length of the controller's register
> +- clocks: spi nor source clock
> +- clock-names: "spi_clk", "axi_clk", "mux_clk", "sf_clk"
> +
> +See Documentation/devicetree/bindings/clock/clock-bindings.txt

As this related to clock-names, just add this line in-continuous with
clock-name property description.

> +and Documentation/mtd/spi-nor.txt for details.

This explicit mentioning about spi-nor documentation is may not
required as this .txt will describe how spi-nor works and not related
to bindings.

> +
> +Example:
> +nor_flash: nor@1100d000 {
> +       compatible = "mediatek,mt8173-nor";
> +       reg = <0 0x1100d000 0 0xe0>;
> +       clocks = <&pericfg CLK_PERI_SPI>,
> +                <&topckgen CLK_TOP_AXI_SEL>,
> +                <&topckgen CLK_TOP_UNIVPLL2_D8>,
> +                <&topckgen CLK_TOP_SPINFI_IFR_SEL>;
> +       clock-names = "spi_clk", "axi_clk", "mux_clk", "sf_clk";
> +};
> +
> --
> 1.8.1.1.dirty
>

thanks!
Sascha Hauer Sept. 9, 2015, 5:47 a.m. UTC | #2 
On Tue, Sep 08, 2015 at 05:49:54PM +0800, Bayi Cheng wrote:
> Add device tree binding documentation for serial flash with
> Mediatek serial flash controller
> 
> Signed-off-by: Bayi Cheng <bayi.cheng@mediatek.com>
> ---
>  Documentation/devicetree/bindings/mtd/mtk_nor.txt | 25 +++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/mtk_nor.txt
> 
> diff --git a/Documentation/devicetree/bindings/mtd/mtk_nor.txt b/Documentation/devicetree/bindings/mtd/mtk_nor.txt
> new file mode 100644
> index 0000000..0eca0cd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/mtk_nor.txt
> @@ -0,0 +1,25 @@
> +* MTD SPI nor driver for MTK MT81xx (and similar) serial flash controller
> +
> +MTK MT81xx serial flash controller is designed for serial Flash device.
> +It supports one Flash device with signal mode, dual mode and quad mode.
> +
> +Required properties:
> +- compatible: should be "mediatek,mt8173-nor";
> +- reg: physical base address and length of the controller's register
> +- clocks: spi nor source clock
> +- clock-names: "spi_clk", "axi_clk", "mux_clk", "sf_clk"

Drop the _clk suffix. It's the clock-names property which already makes
it clear that these are clock names.

Sascha
Brian Norris Sept. 11, 2015, 9:47 p.m. UTC | #3 
On Tue, Sep 08, 2015 at 05:49:54PM +0800, Bayi Cheng wrote:
> Add device tree binding documentation for serial flash with
> Mediatek serial flash controller
> 
> Signed-off-by: Bayi Cheng <bayi.cheng@mediatek.com>
> ---
>  Documentation/devicetree/bindings/mtd/mtk_nor.txt | 25 +++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/mtk_nor.txt
> 
> diff --git a/Documentation/devicetree/bindings/mtd/mtk_nor.txt b/Documentation/devicetree/bindings/mtd/mtk_nor.txt
> new file mode 100644
> index 0000000..0eca0cd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/mtk_nor.txt
> @@ -0,0 +1,25 @@
> +* MTD SPI nor driver for MTK MT81xx (and similar) serial flash controller
> +
> +MTK MT81xx serial flash controller is designed for serial Flash device.
> +It supports one Flash device with signal mode, dual mode and quad mode.
> +
> +Required properties:
> +- compatible: should be "mediatek,mt8173-nor";
> +- reg: physical base address and length of the controller's register
> +- clocks: spi nor source clock
> +- clock-names: "spi_clk", "axi_clk", "mux_clk", "sf_clk"
> +
> +See Documentation/devicetree/bindings/clock/clock-bindings.txt
> +and Documentation/mtd/spi-nor.txt for details.
> +
> +Example:
> +nor_flash: nor@1100d000 {
> +	compatible = "mediatek,mt8173-nor";
> +	reg = <0 0x1100d000 0 0xe0>;
> +	clocks = <&pericfg CLK_PERI_SPI>,
> +		 <&topckgen CLK_TOP_AXI_SEL>,
> +		 <&topckgen CLK_TOP_UNIVPLL2_D8>,
> +		 <&topckgen CLK_TOP_SPINFI_IFR_SEL>;
> +	clock-names = "spi_clk", "axi_clk", "mux_clk", "sf_clk";
> +};

I understand that for now, you only support a single flash, and you
don't need any extra flash-specific DT properties, but in the interest
of being more generic and more in-line with other drivers, can you
include:

  * #addres-cells (= <1>) and #size-cells (= <0>) properties
  * sub-node(s) representing the flash; reference [1], and there's a
    good example in a recent submission [2]

So I'd expect something like:

	nor_flash: nor@1100d000 {
		compatible = "mediatek,mt8173-nor";
		...
		#address-cells = <1>;
		#size-cells = <0>;

		flash@0 {
			compatible = "jedec,spi-nor";
			reg = <0>;
			...
		};
	};

This patch is also relevant [3] (hopefully I'll get to merge that one
soon); you'll want to use the sub-node (not the main node) when
initializing the flash device.

I think maybe we'll want to codify some of this in a "SPI NOR
controller" document, so we can make sure more developers follow this
when designing their binding.

Brian

[1] https://www.kernel.org/doc/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
[2] http://lists.infradead.org/pipermail/linux-mtd/2015-August/061439.html
[3] http://lists.infradead.org/pipermail/linux-mtd/2015-September/061637.html
Brian Norris Sept. 11, 2015, 9:49 p.m. UTC | #4 
One more thing:

On Tue, Sep 08, 2015 at 05:49:54PM +0800, Bayi Cheng wrote:
> +- clocks: spi nor source clock

^^ you only list one clock here

> +- clock-names: "spi_clk", "axi_clk", "mux_clk", "sf_clk"

But you have 4 names here.

...
> +	clocks = <&pericfg CLK_PERI_SPI>,
> +		 <&topckgen CLK_TOP_AXI_SEL>,
> +		 <&topckgen CLK_TOP_UNIVPLL2_D8>,
> +		 <&topckgen CLK_TOP_SPINFI_IFR_SEL>;

And you provide 4 clocks.

> +	clock-names = "spi_clk", "axi_clk", "mux_clk", "sf_clk";

Please list all 4 under the "clocks" property, not just under the
"clock-names" property.

Brian
Bayi Cheng Sept. 15, 2015, 6:39 a.m. UTC | #5 
On Wed, 2015-09-09 at 07:47 +0200, Sascha Hauer wrote:
> On Tue, Sep 08, 2015 at 05:49:54PM +0800, Bayi Cheng wrote:
> > Add device tree binding documentation for serial flash with
> > Mediatek serial flash controller
> > 
> > Signed-off-by: Bayi Cheng <bayi.cheng@mediatek.com>
> > ---
> >  Documentation/devicetree/bindings/mtd/mtk_nor.txt | 25 +++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mtd/mtk_nor.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/mtd/mtk_nor.txt b/Documentation/devicetree/bindings/mtd/mtk_nor.txt
> > new file mode 100644
> > index 0000000..0eca0cd
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mtd/mtk_nor.txt
> > @@ -0,0 +1,25 @@
> > +* MTD SPI nor driver for MTK MT81xx (and similar) serial flash controller
> > +
> > +MTK MT81xx serial flash controller is designed for serial Flash device.
> > +It supports one Flash device with signal mode, dual mode and quad mode.
> > +
> > +Required properties:
> > +- compatible: should be "mediatek,mt8173-nor";
> > +- reg: physical base address and length of the controller's register
> > +- clocks: spi nor source clock
> > +- clock-names: "spi_clk", "axi_clk", "mux_clk", "sf_clk"
> 
> Drop the _clk suffix. It's the clock-names property which already makes
> it clear that these are clock names.
> 
> Sascha
> 
> 
OK, I will drop the _clk suffix here!
Bayi Cheng Sept. 15, 2015, 6:49 a.m. UTC | #6 
On Tue, 2015-09-08 at 17:04 +0530, Jagan Teki wrote:
> On 8 September 2015 at 15:19, Bayi Cheng <bayi.cheng@mediatek.com> wrote:
> > Add device tree binding documentation for serial flash with
> > Mediatek serial flash controller
> >
> > Signed-off-by: Bayi Cheng <bayi.cheng@mediatek.com>
> > ---
> >  Documentation/devicetree/bindings/mtd/mtk_nor.txt | 25 +++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mtd/mtk_nor.txt
> >
> > diff --git a/Documentation/devicetree/bindings/mtd/mtk_nor.txt b/Documentation/devicetree/bindings/mtd/mtk_nor.txt
> > new file mode 100644
> > index 0000000..0eca0cd
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mtd/mtk_nor.txt
> 
> IMHO - looks like the name of the driver file not resembles controller
> driver, usually spi-nor framework has a name include 'nor' and related
> controller drivers uses simple notion with vendor/soc_name-quadspi or
> something similar.
> 
> cadence-quadspi.c
> fsl-quadspi.c
> 
> Simple suggestion - mtk-quadspi.c (since it supports quad)

Ok, Thanks for your instruct! I will change the name of the driver file
from mtk_nor.c to mtk-quadspi.c . at the same time, change the
mtk_nor.txt to mtk-quadspi.txt .
> 
> > @@ -0,0 +1,25 @@
> > +* MTD SPI nor driver for MTK MT81xx (and similar) serial flash controller
> > +
> > +MTK MT81xx serial flash controller is designed for serial Flash device.
> > +It supports one Flash device with signal mode, dual mode and quad mode.
> > +
> > +Required properties:
> > +- compatible: should be "mediatek,mt8173-nor";
> > +- reg: physical base address and length of the controller's register
> > +- clocks: spi nor source clock
> > +- clock-names: "spi_clk", "axi_clk", "mux_clk", "sf_clk"
> > +
> > +See Documentation/devicetree/bindings/clock/clock-bindings.txt
> 
> As this related to clock-names, just add this line in-continuous with
> clock-name property description.
> 
OK, I will change it.
> > +and Documentation/mtd/spi-nor.txt for details.
> 
> This explicit mentioning about spi-nor documentation is may not
> required as this .txt will describe how spi-nor works and not related
> to bindings.
> 
yes, you are right, and I will drop this line
> > +
> > +Example:
> > +nor_flash: nor@1100d000 {
> > +       compatible = "mediatek,mt8173-nor";
> > +       reg = <0 0x1100d000 0 0xe0>;
> > +       clocks = <&pericfg CLK_PERI_SPI>,
> > +                <&topckgen CLK_TOP_AXI_SEL>,
> > +                <&topckgen CLK_TOP_UNIVPLL2_D8>,
> > +                <&topckgen CLK_TOP_SPINFI_IFR_SEL>;
> > +       clock-names = "spi_clk", "axi_clk", "mux_clk", "sf_clk";
> > +};
> > +
> > --
> > 1.8.1.1.dirty
> >
> 
> thanks!
Bayi Cheng Sept. 15, 2015, 6:53 a.m. UTC | #7 
On Fri, 2015-09-11 at 14:49 -0700, Brian Norris wrote:
> One more thing:
> 
> On Tue, Sep 08, 2015 at 05:49:54PM +0800, Bayi Cheng wrote:
> > +- clocks: spi nor source clock
> 
> ^^ you only list one clock here
> 
Ok, I will add other clocks
> > +- clock-names: "spi_clk", "axi_clk", "mux_clk", "sf_clk"
> 
> But you have 4 names here.
> 
> ...
> > +	clocks = <&pericfg CLK_PERI_SPI>,
> > +		 <&topckgen CLK_TOP_AXI_SEL>,
> > +		 <&topckgen CLK_TOP_UNIVPLL2_D8>,
> > +		 <&topckgen CLK_TOP_SPINFI_IFR_SEL>;
> 
> And you provide 4 clocks.
> 
> > +	clock-names = "spi_clk", "axi_clk", "mux_clk", "sf_clk";
> 
> Please list all 4 under the "clocks" property, not just under the
> "clock-names" property.
> 
OK,  Thanks for your remind!
> Brian
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/mtd/mtk_nor.txt b/Documentation/devicetree/bindings/mtd/mtk_nor.txt
new file mode 100644
index 0000000..0eca0cd
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/mtk_nor.txt
@@ -0,0 +1,25 @@ 
+* MTD SPI nor driver for MTK MT81xx (and similar) serial flash controller
+
+MTK MT81xx serial flash controller is designed for serial Flash device.
+It supports one Flash device with signal mode, dual mode and quad mode.
+
+Required properties:
+- compatible: should be "mediatek,mt8173-nor";
+- reg: physical base address and length of the controller's register
+- clocks: spi nor source clock
+- clock-names: "spi_clk", "axi_clk", "mux_clk", "sf_clk"
+
+See Documentation/devicetree/bindings/clock/clock-bindings.txt
+and Documentation/mtd/spi-nor.txt for details.
+
+Example:
+nor_flash: nor@1100d000 {
+	compatible = "mediatek,mt8173-nor";
+	reg = <0 0x1100d000 0 0xe0>;
+	clocks = <&pericfg CLK_PERI_SPI>,
+		 <&topckgen CLK_TOP_AXI_SEL>,
+		 <&topckgen CLK_TOP_UNIVPLL2_D8>,
+		 <&topckgen CLK_TOP_SPINFI_IFR_SEL>;
+	clock-names = "spi_clk", "axi_clk", "mux_clk", "sf_clk";
+};
+