u-boot.lists.denx.de archive mirror
 help / color / mirror / Atom feed
From: <Eugen.Hristev@microchip.com>
To: <michael@amarulasolutions.com>
Cc: <Balamanikandan.Gunasundar@microchip.com>, <u-boot@lists.denx.de>,
	<Sandeep.Sheriker@microchip.com>,
	<dario.binacchi@amarulasolutions.com>, <Mihai.Sain@microchip.com>,
	<Sergiu.Moga@microchip.com>, <Claudiu.Beznea@microchip.com>,
	<hs@denx.de>, <Tudor.Ambarus@microchip.com>, <sjg@chromium.org>,
	<ascull@google.com>, <kettenis@openbsd.org>,
	<jh80.chung@samsung.com>, <peng.fan@nxp.com>, <ye.li@nxp.com>,
	<Hari.PrasathGE@microchip.com>
Subject: Re: [PATCH v2 8/9] ARM: dts: at91: sam9x60ek: Enable NAND support
Date: Thu, 1 Sep 2022 05:57:51 +0000	[thread overview]
Message-ID: <1d672764-5af1-14f5-64fb-dbbc1f2b03a0@microchip.com> (raw)
In-Reply-To: <CAOf5uwnFD=L4GmaJDsxqbNxxsYzU2auTKoAiTwABjAhRf30iOQ@mail.gmail.com>

On 8/31/22 5:19 PM, Michael Nazzareno Trimarchi wrote:
> Hi
> 
> On Wed, Aug 31, 2022 at 3:31 PM <Eugen.Hristev@microchip.com> wrote:
>>
>> On 8/31/22 4:14 PM, Michael Nazzareno Trimarchi wrote:
>>> Hi
>>>
>>> On Mon, Aug 29, 2022 at 8:20 AM Balamanikandan Gunasundar
>>> <balamanikandan.gunasundar@microchip.com> wrote:
>>>>
>>>> Enable the EBI and NAND flash controller. Define the pinctrl and
>>>> partition table
>>>>
>>>> Signed-off-by: Balamanikandan Gunasundar <balamanikandan.gunasundar@microchip.com>
>>>> ---
>>>>    arch/arm/dts/sam9x60ek.dts | 103 +++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 103 insertions(+)
>>>>
>>>> diff --git a/arch/arm/dts/sam9x60ek.dts b/arch/arm/dts/sam9x60ek.dts
>>>> index 54c694bd78..6cb81dd90f 100644
>>>> --- a/arch/arm/dts/sam9x60ek.dts
>>>> +++ b/arch/arm/dts/sam9x60ek.dts
>>>> @@ -80,6 +80,44 @@
>>>>                           };
>>>>
>>>>                           pinctrl {
>>>> +                                       nand {
>>>
>>> I can see two tabs here. You don't need it. The indentation go far on the right
>>>
>>>> +                                               pinctrl_nand_oe_we: nand-oe-we-0 {
>>>> +                                                       atmel,pins =
>>>> +                                                               <AT91_PIOD 0 AT91_PERIPH_A (AT91_PINCTRL_NONE | AT91_PINCTRL_SLEWRATE_DIS)
>>>> +                                                                AT91_PIOD 1 AT91_PERIPH_A (AT91_PINCTRL_NONE | AT91_PINCTRL_SLEWRATE_DIS)>;
>>>> +                                               };
>>>> +
>>>> +                                               pinctrl_nand_rb: nand-rb-0 {
>>>> +                                                       atmel,pins =
>>>> +                                                               <AT91_PIOD 5 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP>;
>>>> +                                               };
>>>> +
>>>> +                                               pinctrl_nand_cs: nand-cs-0 {
>>>> +                                                       atmel,pins =
>>>> +                                                               <AT91_PIOD 4 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP>;
>>>> +                                               };
>>>> +                                       };
>>>> +
>>>> +                                       ebi {
>>>> +                                               pinctrl_ebi_data_0_7: ebi-data-lsb-0 {
>>>> +                                                       atmel,pins =
>>>> +                                                               <AT91_PIOD 6 AT91_PERIPH_A (AT91_PINCTRL_NONE | AT91_PINCTRL_SLEWRATE_DIS)
>>>> +                                                                AT91_PIOD 7 AT91_PERIPH_A (AT91_PINCTRL_NONE | AT91_PINCTRL_SLEWRATE_DIS)
>>>> +                                                                AT91_PIOD 8 AT91_PERIPH_A (AT91_PINCTRL_NONE | AT91_PINCTRL_SLEWRATE_DIS)
>>>> +                                                                AT91_PIOD 9 AT91_PERIPH_A (AT91_PINCTRL_NONE | AT91_PINCTRL_SLEWRATE_DIS)
>>>> +                                                                AT91_PIOD 10 AT91_PERIPH_A (AT91_PINCTRL_NONE | AT91_PINCTRL_SLEWRATE_DIS)
>>>> +                                                                AT91_PIOD 11 AT91_PERIPH_A (AT91_PINCTRL_NONE | AT91_PINCTRL_SLEWRATE_DIS)
>>>> +                                                                AT91_PIOD 12 AT91_PERIPH_A (AT91_PINCTRL_NONE | AT91_PINCTRL_SLEWRATE_DIS)
>>>> +                                                                AT91_PIOD 13 AT91_PERIPH_A (AT91_PINCTRL_NONE | AT91_PINCTRL_SLEWRATE_DIS)>;
>>>> +                                               };
>>>> +
>>>> +                                               pinctrl_ebi_addr_nand: ebi-addr-0 {
>>>> +                                                       atmel,pins =
>>>> +                                                               <AT91_PIOD 2 AT91_PERIPH_A (AT91_PINCTRL_NONE | AT91_PINCTRL_SLEWRATE_DIS)
>>>> +                                                                AT91_PIOD 3 AT91_PERIPH_A (AT91_PINCTRL_NONE | AT91_PINCTRL_SLEWRATE_DIS)>;
>>>> +                                               };
>>>> +                                       };
>>>> +
>>>
>>> Please remove the ebi and nand block so you have one tab less. Then I
>>> suggest to align to linux dts and refer to pinctrl instead to create
>>> another level of indentation
>>
>> Hello Michael,
>>
>> In Linux I see the nand and ebi block. We have to keep the same DT as in
>> Linux. Do you agree ?
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/at91-sam9x60ek.dts#n378
>>
> 
> I have seen it already but I don't think that dtsi is aligned with it.
> So you don't have pinctrl: pinctrl@0ffff400 and you can refer
> to this node as it is. Here the question is if we need to continue to
> have a no-align dts component inside the pinctrl and add more
> elements in it. If you look at that file pinctrl_qspi: qspi { is not
> in any block and for the linux should be even before. It should
> be coherent in what we have or in what linux have now. This does not
> block driver review so I think that can be acked by microchip
> maintainers in this part.
> 

Hi Michael,

I have difficulties to follow your reasoning. Could you detail it a bit 
so I can understand what is the problem you are referring to ?

As I see it, the DT in the patch is aligned with Linux. The nodes are 
identical.

Eugen

> Michael
> 
> 
>> Eugen
>>
>>>
>>>>                                           pinctrl_qspi: qspi {
>>>>                                                   atmel,pins =
>>>>                                                           <AT91_PIOB 19 AT91_PERIPH_A AT91_PINCTRL_NONE
>>> This part can be tab back
>>>
>>> Michael
>>>
>>>> @@ -106,6 +144,71 @@
>>>>           };
>>>>    };
>>>>
>>>> +&ebi {
>>>> +       pinctrl-names = "default";
>>>> +       pinctrl-0 = <&pinctrl_ebi_addr_nand &pinctrl_ebi_data_0_7>;
>>>> +       status = "okay";
>>>> +
>>>> +       nand_controller: nand-controller {
>>>> +               pinctrl-names = "default";
>>>> +               pinctrl-0 = <&pinctrl_nand_oe_we &pinctrl_nand_cs &pinctrl_nand_rb>;
>>>> +               status = "okay";
>>>> +
>>>> +               nand@3 {
>>>> +                       reg = <0x3 0x0 0x800000>;
>>>> +                       rb-gpios = <&pioD 5 GPIO_ACTIVE_HIGH>;
>>>> +                       cs-gpios = <&pioD 4 GPIO_ACTIVE_HIGH>;
>>>> +                       nand-bus-width = <8>;
>>>> +                       nand-ecc-mode = "hw";
>>>> +                       nand-ecc-strength = <8>;
>>>> +                       nand-ecc-step-size = <512>;
>>>> +                       nand-on-flash-bbt;
>>>> +                       label = "atmel_nand";
>>>> +
>>>> +                       partitions {
>>>> +                               compatible = "fixed-partitions";
>>>> +                               #address-cells = <1>;
>>>> +                               #size-cells = <1>;
>>>> +
>>>> +                               at91bootstrap@0 {
>>>> +                                       label = "at91bootstrap";
>>>> +                                       reg = <0x0 0x40000>;
>>>> +                               };
>>>> +
>>>> +                               uboot@40000 {
>>>> +                                       label = "u-boot";
>>>> +                                       reg = <0x40000 0xc0000>;
>>>> +                               };
>>>> +
>>>> +                               ubootenvred@100000 {
>>>> +                                       label = "U-Boot Env Redundant";
>>>> +                                       reg = <0x100000 0x40000>;
>>>> +                               };
>>>> +
>>>> +                               ubootenv@140000 {
>>>> +                                       label = "U-Boot Env";
>>>> +                                       reg = <0x140000 0x40000>;
>>>> +                               };
>>>> +
>>>> +                               dtb@180000 {
>>>> +                                       label = "device tree";
>>>> +                                       reg = <0x180000 0x80000>;
>>>> +                               };
>>>> +
>>>> +                               kernel@200000 {
>>>> +                                       label = "kernel";
>>>> +                                       reg = <0x200000 0x600000>;
>>>> +                               };
>>>> +
>>>> +                               rootfs@800000 {
>>>> +                                       label = "rootfs";
>>>> +                                       reg = <0x800000 0x1f800000>;
>>>> +                               };
>>>> +                       };
>>>> +               };
>>>> +       };
>>>> +};
>>>> +
>>>>    &macb0 {
>>>>           phy-mode = "rmii";
>>>>           status = "okay";
>>>> --
>>>> 2.34.1
>>>>
>>>
>>>
>>> --
>>> Michael Nazzareno Trimarchi
>>> Co-Founder & Chief Executive Officer
>>> M. +39 347 913 2170
>>> michael@amarulasolutions.com
>>> __________________________________
>>>
>>> Amarula Solutions BV
>>> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
>>> T. +31 (0)85 111 9172
>>> info@amarulasolutions.com
>>> www.amarulasolutions.com
>>>
>>
> 
> 
> --
> Michael Nazzareno Trimarchi
> Co-Founder & Chief Executive Officer
> M. +39 347 913 2170
> michael@amarulasolutions.com
> __________________________________
> 
> Amarula Solutions BV
> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> T. +31 (0)85 111 9172
> info@amarulasolutions.com
> www.amarulasolutions.com
> 


  reply	other threads:[~2022-09-01  5:58 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-08-29  6:19 [PATCH v2 0/9] Add DM support for atmel NAND driver Balamanikandan Gunasundar
2022-08-29  6:19 ` [PATCH v2 1/9] nand: atmel: Add DM based " Balamanikandan Gunasundar
2022-09-05  8:55   ` Eugen.Hristev
2022-08-29  6:19 ` [PATCH v2 2/9] nand: atmel: Add pmecc driver Balamanikandan Gunasundar
2022-08-29  6:19 ` [PATCH v2 3/9] mfd: syscon: Add atmel-matrix registers definition Balamanikandan Gunasundar
2022-08-29  6:19 ` [PATCH v2 4/9] memory: atmel-ebi: add Atmel EBI (External Bus Interface) driver Balamanikandan Gunasundar
2022-08-29  6:19 ` [PATCH v2 5/9] mfd: syscon: atmel-smc: Add driver for atmel SMC Balamanikandan Gunasundar
2022-08-31 12:53   ` Michael Nazzareno Trimarchi
2022-09-30 11:02     ` Michael Nazzareno Trimarchi
2022-09-30 14:00       ` Tom Rini
2022-08-29  6:19 ` [PATCH v2 6/9] configs: at91: sam9x60ek: Enable DM based nand driver Balamanikandan Gunasundar
2022-08-29  6:19 ` [PATCH v2 7/9] ARM: dts: at91: sam9x60: Add nodes for EBI and NAND Balamanikandan Gunasundar
2022-08-29  6:19 ` [PATCH v2 8/9] ARM: dts: at91: sam9x60ek: Enable NAND support Balamanikandan Gunasundar
2022-08-31 13:14   ` Michael Nazzareno Trimarchi
2022-08-31 13:31     ` Eugen.Hristev
2022-08-31 14:19       ` Michael Nazzareno Trimarchi
2022-09-01  5:57         ` Eugen.Hristev [this message]
2022-09-20  8:41           ` Dario Binacchi
2022-08-29  6:19 ` [PATCH v2 9/9] board: sam9x60ek: remove nand init from board file Balamanikandan Gunasundar
2022-08-29  6:41 ` [PATCH v2 0/9] Add DM support for atmel NAND driver Michael Nazzareno Trimarchi
2022-08-30  2:30   ` Simon Glass
2022-08-31  6:34     ` Eugen.Hristev
2022-08-31  6:37       ` Michael Nazzareno Trimarchi
2022-08-31 13:46       ` Simon Glass

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1d672764-5af1-14f5-64fb-dbbc1f2b03a0@microchip.com \
    --to=eugen.hristev@microchip.com \
    --cc=Balamanikandan.Gunasundar@microchip.com \
    --cc=Claudiu.Beznea@microchip.com \
    --cc=Hari.PrasathGE@microchip.com \
    --cc=Mihai.Sain@microchip.com \
    --cc=Sandeep.Sheriker@microchip.com \
    --cc=Sergiu.Moga@microchip.com \
    --cc=Tudor.Ambarus@microchip.com \
    --cc=ascull@google.com \
    --cc=dario.binacchi@amarulasolutions.com \
    --cc=hs@denx.de \
    --cc=jh80.chung@samsung.com \
    --cc=kettenis@openbsd.org \
    --cc=michael@amarulasolutions.com \
    --cc=peng.fan@nxp.com \
    --cc=sjg@chromium.org \
    --cc=u-boot@lists.denx.de \
    --cc=ye.li@nxp.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).