diff mbox

[3/3] arm64: dts: mt8173: Add nor flash node

Message ID 1441705796-11365-4-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 Mediatek nor flash node

Signed-off-by: Bayi Cheng <bayi.cheng@mediatek.com>
---
 arch/arm64/boot/dts/mediatek/mt8173.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Jagan Teki Sept. 8, 2015, 11:53 a.m. UTC | #1
On 8 September 2015 at 15:19, Bayi Cheng <bayi.cheng@mediatek.com> wrote:
> Add Mediatek nor flash node
>
> Signed-off-by: Bayi Cheng <bayi.cheng@mediatek.com>
> ---
>  arch/arm64/boot/dts/mediatek/mt8173.dtsi | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> index d18ee42..a14f005 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> @@ -365,6 +365,16 @@
>                         status = "disabled";
>                 };
>
> +               nor_flash: nor@1100d000 {

Based on the comments from 1/3 - this notation needs to be change something like
qspi0: quadspi@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";
> +               };
> +
>                 i2c3: i2c3@11010000 {
>                         compatible = "mediatek,mt8173-i2c";
>                         reg = <0 0x11010000 0 0x70>,
> --
> 1.8.1.1.dirty
>

thanks!
Ezequiel Garcia Sept. 8, 2015, 3:18 p.m. UTC | #2
On 8 September 2015 at 08:53, Jagan Teki <jteki@openedev.com> wrote:
> On 8 September 2015 at 15:19, Bayi Cheng <bayi.cheng@mediatek.com> wrote:
>> Add Mediatek nor flash node
>>
>> Signed-off-by: Bayi Cheng <bayi.cheng@mediatek.com>
>> ---
>>  arch/arm64/boot/dts/mediatek/mt8173.dtsi | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>> index d18ee42..a14f005 100644
>> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>> @@ -365,6 +365,16 @@
>>                         status = "disabled";
>>                 };
>>
>> +               nor_flash: nor@1100d000 {
>
> Based on the comments from 1/3 - this notation needs to be change something like
> qspi0: quadspi@1100d000
>

Actually, to follow ePAPR recomendations it should be named as flash@1100d000.
(See ePAPR, 2.2.2 Generic Names Recommendation).

I believe the phandle name choice can be more descriptive.
Rob Herring (Arm) Sept. 8, 2015, 11:46 p.m. UTC | #3
On 09/08/2015 10:18 AM, Ezequiel Garcia wrote:
> On 8 September 2015 at 08:53, Jagan Teki <jteki@openedev.com> wrote:
>> On 8 September 2015 at 15:19, Bayi Cheng <bayi.cheng@mediatek.com> wrote:
>>> Add Mediatek nor flash node
>>>
>>> Signed-off-by: Bayi Cheng <bayi.cheng@mediatek.com>
>>> ---
>>>  arch/arm64/boot/dts/mediatek/mt8173.dtsi | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>>> index d18ee42..a14f005 100644
>>> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>>> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
>>> @@ -365,6 +365,16 @@
>>>                         status = "disabled";
>>>                 };
>>>
>>> +               nor_flash: nor@1100d000 {
>>
>> Based on the comments from 1/3 - this notation needs to be change something like
>> qspi0: quadspi@1100d000
>>
> 
> Actually, to follow ePAPR recomendations it should be named as flash@1100d000.
> (See ePAPR, 2.2.2 Generic Names Recommendation).

The flash device node should, but this is the controller which should be
"spi" IMO even if this is not a general purpose controller.

Rob
Bayi Cheng Sept. 11, 2015, 9:47 a.m. UTC | #4
On Tue, 2015-09-08 at 17:23 +0530, Jagan Teki wrote:
> On 8 September 2015 at 15:19, Bayi Cheng <bayi.cheng@mediatek.com> wrote:
> > Add Mediatek nor flash node
> >
> > Signed-off-by: Bayi Cheng <bayi.cheng@mediatek.com>
> > ---
> >  arch/arm64/boot/dts/mediatek/mt8173.dtsi | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > index d18ee42..a14f005 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > @@ -365,6 +365,16 @@
> >                         status = "disabled";
> >                 };
> >
> > +               nor_flash: nor@1100d000 {
> 
> Based on the comments from 1/3 - this notation needs to be change something like
> qspi0: quadspi@1100d000

OK, I will fixed it!
> 
> > +                       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";
> > +               };
> > +
> >                 i2c3: i2c3@11010000 {
> >                         compatible = "mediatek,mt8173-i2c";
> >                         reg = <0 0x11010000 0 0x70>,
> > --
> > 1.8.1.1.dirty
> >
> 
> thanks!
Bayi Cheng Sept. 11, 2015, 9:49 a.m. UTC | #5
On Tue, 2015-09-08 at 12:18 -0300, Ezequiel Garcia wrote:
> On 8 September 2015 at 08:53, Jagan Teki <jteki@openedev.com> wrote:
> > On 8 September 2015 at 15:19, Bayi Cheng <bayi.cheng@mediatek.com> wrote:
> >> Add Mediatek nor flash node
> >>
> >> Signed-off-by: Bayi Cheng <bayi.cheng@mediatek.com>
> >> ---
> >>  arch/arm64/boot/dts/mediatek/mt8173.dtsi | 10 ++++++++++
> >>  1 file changed, 10 insertions(+)
> >>
> >> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> >> index d18ee42..a14f005 100644
> >> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> >> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> >> @@ -365,6 +365,16 @@
> >>                         status = "disabled";
> >>                 };
> >>
> >> +               nor_flash: nor@1100d000 {
> >
> > Based on the comments from 1/3 - this notation needs to be change something like
> > qspi0: quadspi@1100d000
> >
> 
> Actually, to follow ePAPR recomendations it should be named as flash@1100d000.
> (See ePAPR, 2.2.2 Generic Names Recommendation).
> 
> I believe the phandle name choice can be more descriptive.

Thanks for your instruct, I will fix it in the next patch !
Bayi Cheng Sept. 11, 2015, 9:51 a.m. UTC | #6
On Tue, 2015-09-08 at 18:46 -0500, Rob Herring wrote:
> On 09/08/2015 10:18 AM, Ezequiel Garcia wrote:
> > On 8 September 2015 at 08:53, Jagan Teki <jteki@openedev.com> wrote:
> >> On 8 September 2015 at 15:19, Bayi Cheng <bayi.cheng@mediatek.com> wrote:
> >>> Add Mediatek nor flash node
> >>>
> >>> Signed-off-by: Bayi Cheng <bayi.cheng@mediatek.com>
> >>> ---
> >>>  arch/arm64/boot/dts/mediatek/mt8173.dtsi | 10 ++++++++++
> >>>  1 file changed, 10 insertions(+)
> >>>
> >>> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> >>> index d18ee42..a14f005 100644
> >>> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> >>> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> >>> @@ -365,6 +365,16 @@
> >>>                         status = "disabled";
> >>>                 };
> >>>
> >>> +               nor_flash: nor@1100d000 {
> >>
> >> Based on the comments from 1/3 - this notation needs to be change something like
> >> qspi0: quadspi@1100d000
> >>
> > 
> > Actually, to follow ePAPR recomendations it should be named as flash@1100d000.
> > (See ePAPR, 2.2.2 Generic Names Recommendation).
> 
> The flash device node should, but this is the controller which should be
> "spi" IMO even if this is not a general purpose controller.
> 
> Rob
> 
HI Rob, Thanks for your instruct! I will fix it !
Brian Norris Sept. 11, 2015, 9:56 p.m. UTC | #7
On Fri, Sep 11, 2015 at 05:51:16PM +0800, bayi.cheng wrote:
> On Tue, 2015-09-08 at 18:46 -0500, Rob Herring wrote:
> > On 09/08/2015 10:18 AM, Ezequiel Garcia wrote:
> > > On 8 September 2015 at 08:53, Jagan Teki <jteki@openedev.com> wrote:
> > >> On 8 September 2015 at 15:19, Bayi Cheng <bayi.cheng@mediatek.com> wrote:
> > >>> Add Mediatek nor flash node
> > >>>
> > >>> Signed-off-by: Bayi Cheng <bayi.cheng@mediatek.com>
> > >>> ---
> > >>>  arch/arm64/boot/dts/mediatek/mt8173.dtsi | 10 ++++++++++
> > >>>  1 file changed, 10 insertions(+)
> > >>>
> > >>> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > >>> index d18ee42..a14f005 100644
> > >>> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > >>> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > >>> @@ -365,6 +365,16 @@
> > >>>                         status = "disabled";
> > >>>                 };
> > >>>
> > >>> +               nor_flash: nor@1100d000 {
> > >>
> > >> Based on the comments from 1/3 - this notation needs to be change something like
> > >> qspi0: quadspi@1100d000
> > >>
> > > 
> > > Actually, to follow ePAPR recomendations it should be named as flash@1100d000.
> > > (See ePAPR, 2.2.2 Generic Names Recommendation).
> > 
> > The flash device node should, but this is the controller which should be
> > "spi" IMO even if this is not a general purpose controller.
> > 
> > Rob
> > 
> HI Rob, Thanks for your instruct! I will fix it !

OK, so given there are several cooks in the kitchen, and I just read
these replies after replying independently to the binding doc, I'll try
to summarize the consensus, flavored with my own opinion:

 * the node name should not be 'nor'; it should be something more along
   the lines of ePAPR. So 'flash' or 'spi'
 * the phandle name is less important, and can be more descriptive
 * there should be a subnode to represent the flash separately from the
   controller

So we might put this together to:

	whatever_name_you_like: spi@1100d000 {
		...
		flash@0 {
			...
		};
	};

Brian
diff mbox

Patch

diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
index d18ee42..a14f005 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
@@ -365,6 +365,16 @@ 
 			status = "disabled";
 		};
 
+		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";
+		};
+
 		i2c3: i2c3@11010000 {
 			compatible = "mediatek,mt8173-i2c";
 			reg = <0 0x11010000 0 0x70>,