Re: [PATCH 1/5] arm: apple: Add initial support for Apple's M1 SoC

21 Sep 2021

Hi Mark,
On Mon, 20 Sept 2021 at 02:49, Mark Kettenis mark.kettenis@xs4all.nl wrote:
...
...
From: Simon Glass sjg@chromium.org
Date: Sun, 19 Sep 2021 21:15:57 -0600
Hi Mark,
On Sat, 18 Sept 2021 at 07:55, Mark Kettenis kettenis@openbsd.org wrote:
...
Add support for Apple's M1 SoC that is used in "Apple Silicon"
Macs.  This builds a basic U-Boot that can be used as a payload
for the m1n1 boot loader being developed by the Asahi Linux
project.
Signed-off-by: Mark Kettenis kettenis@openbsd.org
arch/arm/Kconfig                    |  22 ++++
 arch/arm/Makefile                   |   1 +
 arch/arm/mach-apple/Kconfig         |  18 ++++
 arch/arm/mach-apple/Makefile        |   4 +
 arch/arm/mach-apple/board.c         | 158 ++++++++++++++++++++++++++++
 arch/arm/mach-apple/lowlevel_init.S |  16 +++
 configs/apple_m1_defconfig          |  14 +++
 include/configs/apple.h             |  38 +++++++
 8 files changed, 271 insertions(+)
 create mode 100644 arch/arm/mach-apple/Kconfig
 create mode 100644 arch/arm/mach-apple/Makefile
 create mode 100644 arch/arm/mach-apple/board.c
 create mode 100644 arch/arm/mach-apple/lowlevel_init.S
 create mode 100644 configs/apple_m1_defconfig
 create mode 100644 include/configs/apple.h

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index b5bd3284cd..7cdea1f615 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -895,6 +895,26 @@ config ARCH_NEXELL
        select DM
        select GPIO_EXTRA_HEADER
+config ARCH_APPLE

  bool "Apple SoCs"


  select ARM64


  select LINUX_KERNEL_IMAGE_HEADER


  select POSITION_INDEPENDENT


  select BLK


  select DM


  select DM_KEYBOARD


  select DM_SERIAL


  select DM_USB


  select DM_VIDEO


  select CMD_USB


  select MISC


  select OF_CONTROL


  select OF_BOARD


  select USB


  imply CMD_DM


  imply CMD_GPT


  imply DISTRO_DEFAULTS



Suggest sorting these
As in sort all the selects alphabetically and sort all the implies
alphabetically seperately?
I suppose so.
...
Does my use of impy here even make sense?
It allows people to disable it, whereas select does not. It looks OK to me.
...
This whole Kconfig stuff is a bit alien to me and I must say that it
isn't obvious what "best-practice" is in this area...
We try to avoid select so only use it if it really cannot build/run
without that feature. From what I can tell you have done that.
I know this is a different project, but there are some ideas here:
https://docs.zephyrproject.org/1.14.0/guides/kconfig/index.html
[..]
...
...
...
+int dram_init(void)
+{

  int index, node, ret;


  fdt_addr_t base;


  fdt_size_t size;



  ret = fdtdec_setup_mem_size_base();


  if (ret)


          return ret;



  /* Update RAM mapping. */



Do you want the period at the end of that?
Given that Bin brought up the same thing, I gather that not having a
full stop at the end of single-line comments is preferred?
Yes, the periods just clutter things and people start adding them only
when it is a valid sentence, etc. Best to just leave them out.
...
...
...

  index = ARRAY_SIZE(apple_mem_map) - 3;


  apple_mem_map[index].virt = gd->ram_base;


  apple_mem_map[index].phys = gd->ram_base;


  apple_mem_map[index].size = gd->ram_size;



  node = fdt_path_offset(gd->fdt_blob, "/chosen/framebuffer");



Can you use the ofnode interface here and below?
I can try...
Something like:
ofnode node;
node = ofnode_path("/chosen/framebuffer");
then use ofnode_get_addr_size()
...
...
...

  if (node < 0)


          return 0;



  base = fdtdec_get_addr_size(gd->fdt_blob, node, "reg", &size);


  if (base == FDT_ADDR_T_NONE)


          return 0;



  /* Add framebuffer mapping. */


  index = ARRAY_SIZE(apple_mem_map) - 2;


  apple_mem_map[index].virt = base;


  apple_mem_map[index].phys = base;


  apple_mem_map[index].size = size;


  apple_mem_map[index].attrs = PTE_BLOCK_MEMTYPE(MT_NORMAL_NC) |


          PTE_BLOCK_INNER_SHARE | PTE_BLOCK_PXN | PTE_BLOCK_UXN;



  return 0;



+}
[..]
...
...
...
diff --git a/arch/arm/mach-apple/lowlevel_init.S b/arch/arm/mach-apple/lowlevel_init.S
new file mode 100644
index 0000000000..0f5313163e
--- /dev/null
+++ b/arch/arm/mach-apple/lowlevel_init.S
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*


(C) Copyright 2021 Mark Kettenis kettenis@openbsd.org


*/


+.align 8
+.global fw_dtb_pointer
+fw_dtb_pointer:

  .quad   0




+.global save_boot_params
+save_boot_params:

  adr     x1, fw_dtb_pointer



could use a comment as to where this is set up. Previous-stage
firmware, I suppose?
Yes.  I'm basically making U-Boot look like a Linux kernel, and m1n1
passes the DT in x1 just like the Linux kernel expects.
I suspect using CONFIG_OF_PRIOR_STAGE would make it more obvious what
is happening here.  But (a) I didn't know that existed and (b) we
discussed removing that option in the near future.
That sounds right to me, but a comment would help.
Regards,
Simon

    