Hi Eugen,
On Thu, Sep 1, 2022 at 7:57 AM <eugen.hris...@microchip.com> wrote:
>
> On 8/31/22 5:19 PM, Michael Nazzareno Trimarchi wrote:
> > Hi
> >
> > On Wed, Aug 31, 2022 at 3:31 PM <eugen.hris...@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.gunasun...@microchip.com> wrote:
> >>>>
> >>>> Enable the EBI and NAND flash controller. Define the pinctrl and
> >>>> partition table
> >>>>
> >>>> Signed-off-by: Balamanikandan Gunasundar
> >>>> <balamanikandan.gunasun...@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.
The "pinctrl" node in linux mainline is not a subnode of "apb" but is
referenced.
This improves the indentation. Therefore, it is better to add the NAND
subnodes to a
referenced pinctrl node. In the meantime, I have submitted a patch
that fixes the
indentation for the current subnodes of pinctrl.
Thanks and regards,
Dario
>
> 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
> >>> mich...@amarulasolutions.com
> >>> __________________________________
> >>>
> >>> Amarula Solutions BV
> >>> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> >>> T. +31 (0)85 111 9172
> >>> i...@amarulasolutions.com
> >>> www.amarulasolutions.com
> >>>
> >>
> >
> >
> > --
> > Michael Nazzareno Trimarchi
> > Co-Founder & Chief Executive Officer
> > M. +39 347 913 2170
> > mich...@amarulasolutions.com
> > __________________________________
> >
> > Amarula Solutions BV
> > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > T. +31 (0)85 111 9172
> > i...@amarulasolutions.com
> > www.amarulasolutions.com
> >
>
--
Dario Binacchi
Embedded Linux Developer
dario.binac...@amarulasolutions.com
__________________________________
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
i...@amarulasolutions.com
www.amarulasolutions.com
