[v3] The Yosemite V3.5 is a facebook multi-node server platform that host four OCP server. The BMC in the Yosemite V3.5 platform based on AST2600 SoC.

This patch adds linux device tree entry related to
Yosemite V3.5 specific devices connected to BMC SoC.

Signed-off-by: Logananth Sundararaj <logananth_s@hcl.com>

---
--- v3 - addressed v2 patch comments.
--- v2 - Enabled i2c drivers.
--- v1 - Initial draft.
---
---
 arch/arm/boot/dts/Makefile                    |   1 +
 .../boot/dts/aspeed-bmc-facebook-fby35.dts    | 266 ++++++++++++++++++
 2 files changed, 267 insertions(+)
 create mode 100644 arch/arm/boot/dts/aspeed-bmc-facebook-fby35.dts

On Thu, Jul 7, 2022 at 3:20 PM Logananth Sundararaj
<logananth13.hcl@gmail.com> wrote:
>
> This patch adds linux device tree entry related to
> Yosemite V3.5 specific devices connected to BMC SoC.
>
> Signed-off-by: Logananth Sundararaj <logananth_s@hcl.com>


Something went wrong with the patch description, it looks like you dropped
the subject and sent the first paragraph as the subject instead.

> +/ {
> +       model = "Facebook fby35";
> +       compatible = "facebook,fby35", "aspeed,ast2600";
> +
> +       aliases {
> +               serial4 = &uart5;
> +       };

Why not start at serial0 here?

> +       chosen {
> +               stdout-path = &uart5;
> +               bootargs = "console=ttyS4,57600n8 root=/dev/ram rw vmalloc=384M";
> +       };

The bootargs should really come from the boot loader.

Why do you need the vmalloc=384M? That seems excessive.

> +
> +       memory@80000000 {
> +               device_type = "memory";
> +               reg = <0x80000000 0x80000000>;
> +       };

The memory location and size is usually also set by the boot loader

        Arnd

On Thu, Jul 07, 2022 at 03:33:48PM +0200, Arnd Bergmann wrote:
> On Thu, Jul 7, 2022 at 3:20 PM Logananth Sundararaj
> <logananth13.hcl@gmail.com> wrote:
> >
> > This patch adds linux device tree entry related to
> > Yosemite V3.5 specific devices connected to BMC SoC.
> >
> > Signed-off-by: Logananth Sundararaj <logananth_s@hcl.com>
> 
> 
> Something went wrong with the patch description, it looks like you dropped
> the subject and sent the first paragraph as the subject instead.
> 
> > +/ {
> > +       model = "Facebook fby35";
> > +       compatible = "facebook,fby35", "aspeed,ast2600";
> > +
> > +       aliases {
> > +               serial4 = &uart5;
> > +       };
> 
> Why not start at serial0 here?

Hey, Facebook person jumping in here (using a personal email):

I think you're right, it should be like this:

	aliases {
		serial0 = &uart5;
		serial1 = &uart1;
		serial2 = &uart2;
		serial3 = &uart3;
		serial4 = &uart4;
	};

> 
> > +       chosen {
> > +               stdout-path = &uart5;
> > +               bootargs = "console=ttyS4,57600n8 root=/dev/ram rw vmalloc=384M";
> > +       };

Also: if we do serial0 = &uart5, it should be console=ttyS0, not ttyS4.

> 
> The bootargs should really come from the boot loader.

What if we want to boot the kernel by itself with QEMU? It's kinda annoying to
have to specify '-append "console=ttyS0,57600n8...' everytime, or to have to use
a wrapper script. But, it's also a source of bugs: I realized yesterday the
dts we were using here:

https://github.com/facebook/openbmc-linux/blob/e26c76992e0761d9e440ff514538009384c094b4/arch/arm/boot/dts/aspeed-bmc-facebook-fby35.dts

Has the wrong console setting.

Booting the kernel directly is actualy really useful for us, because U-Boot
literally takes 4+ minutes in QEMU because we execute in-place from flash for
most of it.

> 
> Why do you need the vmalloc=384M? That seems excessive.

Yeah I'm not sure what that is about, would need to find
the person who wrote this originally. Speaking of which:

The dts I linked above, from our repo, how come this patch is not just a copy of
that?

> 
> > +
> > +       memory@80000000 {
> > +               device_type = "memory";
> > +               reg = <0x80000000 0x80000000>;
> > +       };
> 
> The memory location and size is usually also set by the boot loader

Yeah not sure what happens if I remove this and boot the kernel directly in
QEMU, would we need to specify the RAM size explicitly in console args? Hmmm,
I'll check it out.

Thanks for your comments!
Peter

> 
>         Arnd

On Thu, Jul 14, 2022 at 8:05 PM Peter Delevoryas <peter@pjd.dev> wrote:
> On Thu, Jul 07, 2022 at 03:33:48PM +0200, Arnd Bergmann wrote:
> > > +       model = "Facebook fby35";
> > > +       compatible = "facebook,fby35", "aspeed,ast2600";
> > > +
> > > +       aliases {
> > > +               serial4 = &uart5;
> > > +       };
> >
> > Why not start at serial0 here?
>
> Hey, Facebook person jumping in here (using a personal email):
>
> I think you're right, it should be like this:
>
>         aliases {
>                 serial0 = &uart5;
>                 serial1 = &uart1;
>                 serial2 = &uart2;
>                 serial3 = &uart3;
>                 serial4 = &uart4;
>         };

Are you actually using all five uarts though?

> > > +       chosen {
> > > +               stdout-path = &uart5;
> > > +               bootargs = "console=ttyS4,57600n8 root=/dev/ram rw vmalloc=384M";
> > > +       };
>
> Also: if we do serial0 = &uart5, it should be console=ttyS0, not ttyS4.
>
> >
> > The bootargs should really come from the boot loader.
>
> What if we want to boot the kernel by itself with QEMU? It's kinda annoying to
> have to specify '-append "console=ttyS0,57600n8...' everytime, or to have to use
> a wrapper script. But, it's also a source of bugs: I realized yesterday the
> dts we were using here:
>
> https://github.com/facebook/openbmc-linux/blob/e26c76992e0761d9e440ff514538009384c094b4/arch/arm/boot/dts/aspeed-bmc-facebook-fby35.dts
>
> Has the wrong console setting.

You can encode the uart settings like

           stdout-path = "serial0:115200n8"

the rest really should be passed on the command line, not in
the DT shipped with the kernel.

        Arnd

On Thu, Jul 14, 2022 at 10:15:15PM +0200, Arnd Bergmann wrote:
> On Thu, Jul 14, 2022 at 8:05 PM Peter Delevoryas <peter@pjd.dev> wrote:
> > On Thu, Jul 07, 2022 at 03:33:48PM +0200, Arnd Bergmann wrote:
> > > > +       model = "Facebook fby35";
> > > > +       compatible = "facebook,fby35", "aspeed,ast2600";
> > > > +
> > > > +       aliases {
> > > > +               serial4 = &uart5;
> > > > +       };
> > >
> > > Why not start at serial0 here?
> >
> > Hey, Facebook person jumping in here (using a personal email):
> >
> > I think you're right, it should be like this:
> >
> >         aliases {
> >                 serial0 = &uart5;
> >                 serial1 = &uart1;
> >                 serial2 = &uart2;
> >                 serial3 = &uart3;
> >                 serial4 = &uart4;
> >         };
> 
> Are you actually using all five uarts though?

Actually yes, I should have mentioned this in my previous message.

YosemiteV3.5 is similar to YosemiteV3, which you can see here:

https://www.opencompute.org/products/423/wiwynn-yosemite-v3-server

This dts is for the BMC on the sled baseboard, and it manages the 4 slots in the
sled. Each slot has a "Bridge Interconnect" (BIC) (an AST1030) that manages the
slot CPU/etc. uart1 is connected to a uart on slot1's BIC, uart2 to slot2, etc.

We also have a work-in-progress QEMU model for this:

https://lore.kernel.org/qemu-devel/20220714154456.2565189-1-clg@kaod.org/

> 
> > > > +       chosen {
> > > > +               stdout-path = &uart5;
> > > > +               bootargs = "console=ttyS4,57600n8 root=/dev/ram rw vmalloc=384M";
> > > > +       };
> >
> > Also: if we do serial0 = &uart5, it should be console=ttyS0, not ttyS4.
> >
> > >
> > > The bootargs should really come from the boot loader.
> >
> > What if we want to boot the kernel by itself with QEMU? It's kinda annoying to
> > have to specify '-append "console=ttyS0,57600n8...' everytime, or to have to use
> > a wrapper script. But, it's also a source of bugs: I realized yesterday the
> > dts we were using here:
> >
> > https://github.com/facebook/openbmc-linux/blob/e26c76992e0761d9e440ff514538009384c094b4/arch/arm/boot/dts/aspeed-bmc-facebook-fby35.dts
> >
> > Has the wrong console setting.
> 
> You can encode the uart settings like
> 
>            stdout-path = "serial0:115200n8"
> 
> the rest really should be passed on the command line, not in
> the DT shipped with the kernel.
> 
>         Arnd

On Thu, Jul 14, 2022 at 01:24:59PM -0700, Peter Delevoryas wrote:
> On Thu, Jul 14, 2022 at 10:15:15PM +0200, Arnd Bergmann wrote:
> > On Thu, Jul 14, 2022 at 8:05 PM Peter Delevoryas <peter@pjd.dev> wrote:
> > > On Thu, Jul 07, 2022 at 03:33:48PM +0200, Arnd Bergmann wrote:
> > > > > +       model = "Facebook fby35";
> > > > > +       compatible = "facebook,fby35", "aspeed,ast2600";
> > > > > +
> > > > > +       aliases {
> > > > > +               serial4 = &uart5;
> > > > > +       };
> > > >
> > > > Why not start at serial0 here?
> > >
> > > Hey, Facebook person jumping in here (using a personal email):
> > >
> > > I think you're right, it should be like this:
> > >
> > >         aliases {
> > >                 serial0 = &uart5;
> > >                 serial1 = &uart1;
> > >                 serial2 = &uart2;
> > >                 serial3 = &uart3;
> > >                 serial4 = &uart4;
> > >         };
> > 
> > Are you actually using all five uarts though?
> 
> Actually yes, I should have mentioned this in my previous message.
> 
> YosemiteV3.5 is similar to YosemiteV3, which you can see here:
> 
> https://www.opencompute.org/products/423/wiwynn-yosemite-v3-server
> 
> This dts is for the BMC on the sled baseboard, and it manages the 4 slots in the
> sled. Each slot has a "Bridge Interconnect" (BIC) (an AST1030) that manages the
> slot CPU/etc. uart1 is connected to a uart on slot1's BIC, uart2 to slot2, etc.
> 
> We also have a work-in-progress QEMU model for this:
> 
> https://lore.kernel.org/qemu-devel/20220714154456.2565189-1-clg@kaod.org/
> 
> > 
> > > > > +       chosen {
> > > > > +               stdout-path = &uart5;
> > > > > +               bootargs = "console=ttyS4,57600n8 root=/dev/ram rw vmalloc=384M";
> > > > > +       };
> > >
> > > Also: if we do serial0 = &uart5, it should be console=ttyS0, not ttyS4.
> > >
> > > >
> > > > The bootargs should really come from the boot loader.
> > >
> > > What if we want to boot the kernel by itself with QEMU? It's kinda annoying to
> > > have to specify '-append "console=ttyS0,57600n8...' everytime, or to have to use
> > > a wrapper script. But, it's also a source of bugs: I realized yesterday the
> > > dts we were using here:
> > >
> > > https://github.com/facebook/openbmc-linux/blob/e26c76992e0761d9e440ff514538009384c094b4/arch/arm/boot/dts/aspeed-bmc-facebook-fby35.dts
> > >
> > > Has the wrong console setting.
> > 
> > You can encode the uart settings like
> > 
> >            stdout-path = "serial0:115200n8"
> > 
> > the rest really should be passed on the command line, not in
> > the DT shipped with the kernel.

Oh sorry, I missed this comment:

That sounds good, I'm fine with that. We should remove the bootargs then.

Thanks,
Peter

> > 
> >         Arnd