Re: [PATCH v2 11/13] ARM: boards: sunxi: Add initial support for the pinephone

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

On 01.06.23 08:19, Jules Maselbas wrote:
> On Thu, Jun 01, 2023 at 08:00:47AM +0200, Ahmad Fatoum wrote:
>> On 01.06.23 07:50, Jules Maselbas wrote:
>>> Hi Sascha,
>>>
>>> Thanks for your review
>>>
>>> On Tue, May 30, 2023 at 10:42:36AM +0200, Sascha Hauer wrote:
>>>> On Thu, May 25, 2023 at 01:43:26AM +0200, Jules Maselbas wrote:
>>>>> Signed-off-by: Jules Maselbas <jmaselbas@zdiv.net>
>>>>> ---
>>>>>  arch/arm/boards/Makefile                    |   1 +
>>>>>  arch/arm/boards/pine64-pinephone/Makefile   |   2 +
>>>>>  arch/arm/boards/pine64-pinephone/board.c    |   0
>>>>>  arch/arm/boards/pine64-pinephone/lowlevel.c | 104 ++++++++++++++++++++
>>>>>  arch/arm/configs/pinephone_defconfig        |  12 +++
>>>>>  arch/arm/dts/Makefile                       |   1 +
>>>>>  arch/arm/dts/sun50i-a64-pinephone-1_2.dts   |   3 +
>>>>>  arch/arm/mach-sunxi/Kconfig                 |  17 ++++
>>>>>  images/Makefile.sunxi                       |   9 ++
>>>>>  include/mach/sunxi/init.h                   |   4 +
>>>>>  10 files changed, 153 insertions(+)
>>>>>  create mode 100644 arch/arm/boards/pine64-pinephone/Makefile
>>>>>  create mode 100644 arch/arm/boards/pine64-pinephone/board.c
>>>>>  create mode 100644 arch/arm/boards/pine64-pinephone/lowlevel.c
>>>>>  create mode 100644 arch/arm/configs/pinephone_defconfig
>>>>>  create mode 100644 arch/arm/dts/sun50i-a64-pinephone-1_2.dts
>>>>>
>>>>> diff --git a/arch/arm/boards/Makefile b/arch/arm/boards/Makefile
>>>>> index b204c257f6..f4796f5374 100644
>>>>> --- a/arch/arm/boards/Makefile
>>>>> +++ b/arch/arm/boards/Makefile
>>>>> @@ -96,6 +96,7 @@ obj-$(CONFIG_MACH_PHYTEC_SOM_IMX6)		+= phytec-som-imx6/
>>>>>  obj-$(CONFIG_MACH_PHYTEC_PHYCORE_IMX7)		+= phytec-phycore-imx7/
>>>>>  obj-$(CONFIG_MACH_PHYTEC_PHYCORE_STM32MP1)	+= phytec-phycore-stm32mp1/
>>>>>  obj-$(CONFIG_MACH_PHYTEC_SOM_IMX8MQ)		+= phytec-som-imx8mq/
>>>>> +obj-$(CONFIG_MACH_PINE64_PINEPHONE)		+= pine64-pinephone/
>>>>>  obj-$(CONFIG_MACH_PLATHOME_OPENBLOCKS_AX3)	+= plathome-openblocks-ax3/
>>>>>  obj-$(CONFIG_MACH_PLATHOME_OPENBLOCKS_A6)	+= plathome-openblocks-a6/
>>>>>  obj-$(CONFIG_MACH_PM9261)			+= pm9261/
>>>>> diff --git a/arch/arm/boards/pine64-pinephone/Makefile b/arch/arm/boards/pine64-pinephone/Makefile
>>>>> new file mode 100644
>>>>> index 0000000000..092c31d6b2
>>>>> --- /dev/null
>>>>> +++ b/arch/arm/boards/pine64-pinephone/Makefile
>>>>> @@ -0,0 +1,2 @@
>>>>> +lwl-y += lowlevel.o
>>>>> +obj-y += board.o
>>>>> diff --git a/arch/arm/boards/pine64-pinephone/board.c b/arch/arm/boards/pine64-pinephone/board.c
>>>>> new file mode 100644
>>>>> index 0000000000..e69de29bb2
>>>>> diff --git a/arch/arm/boards/pine64-pinephone/lowlevel.c b/arch/arm/boards/pine64-pinephone/lowlevel.c
>>>>> new file mode 100644
>>>>> index 0000000000..262d194864
>>>>> --- /dev/null
>>>>> +++ b/arch/arm/boards/pine64-pinephone/lowlevel.c
>>>>> @@ -0,0 +1,104 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>>> +#include <common.h>
>>>>> +#include <debug_ll.h>
>>>>> +#include <linux/sizes.h>
>>>>> +#include <linux/bitops.h>
>>>>> +#include <mach/sunxi/barebox-arm.h>
>>>>
>>>> This file is missing in this series. Forgot to git add it?
>>> ooops, yes forgot to add it.
>>> There is almost nothing in it:
>>> ```
>>> #include <asm/barebox-arm.h>
>>>
>>> #define SUN50I_A64_ENTRY_FUNCTION(name, arg0, arg1, arg2) \
>>> 	ENTRY_FUNCTION_WITHSTACK(name, SUN50I_PBL_STACK_TOP, arg0, arg1, arg2)
>>> ```
>>> that's all
>>
>> Can this pull in the eGON header as well? That way sun50i entry points
>> can just look like normal C functions and board porters need not be aware
>> of the magic.
> well, I already spent huge amount of time trying to do so and failed so right
> now I do not want to try to fit the headers into the entry macro again.
> The issue was that I got multiple instances of the each headers because there
> where two instance of the entry macro in the same lowlevel.c file...

Do you have your attempt uploaded somewhere? Duplicate instances sounds like
if the section name didn't change. This should be fixable by concatenating the
function name at the end.

> 
>>>
>>>>> +#include <mach/sunxi/init.h>
>>>>> +#include <mach/sunxi/xload.h>
>>>>> +#include <mach/sunxi/egon.h>
>>>>> +#include <mach/sunxi/rmr_switch.h>
>>>>> +#include <mach/sunxi/sun50i-regs.h>
>>>>> +#include <mach/sunxi/sunxi-pinctrl.h>
>>>>> +
>>>>> +#ifdef DEBUG
>>>>> +static void debug_led_rgb(int rgb)
>>>>> +{
>>>>> +	void __iomem *piobase = SUN50I_PIO_BASE_ADDR;
>>>>> +	uint32_t clr, set = 0;
>>>>> +	int r = rgb & 0xff0000;
>>>>> +	int g = rgb & 0x00ff00;
>>>>> +	int b = rgb & 0x0000ff;
>>>>> +
>>>>> +	clr = (1 << 19) | (1 << 18) | (1 << 20);
>>>>> +	set |= r ? 1 << 19 : 0;
>>>>> +	set |= g ? 1 << 18 : 0;
>>>>> +	set |= b ? 1 << 20 : 0;
>>>>> +
>>>>> +	clrsetbits_le32(piobase + PIO_PD_DATA, clr, set);
>>>>> +}
>>>>> +
>>>>> +static void debug_led_init(void)
>>>>> +{
>>>>> +	void __iomem *ccubase = SUN50I_CCU_BASE_ADDR;
>>>>> +	void __iomem *piobase = SUN50I_PIO_BASE_ADDR;
>>>>> +
>>>>> +	/* PIO clock enable */
>>>>> +	setbits_le32(ccubase + CCU_BUS_CLK_GATE2, BIT(5));
>>>>> +	/* LED set output */
>>>>> +	clrsetbits_le32(piobase + PIO_PD_CFG2, 0x000fff00, 0x00011100);
>>>>> +}
>>>>> +#else
>>>>> +static void debug_led_rgb(int rgb) {}
>>>>> +static void debug_led_init(void) {}
>>>>> +#endif
>>>>> +
>>>>> +SUN50I_A64_ENTRY_FUNCTION(start_pine64_pinephone, r0, r1, r2)
>>>>> +{
>>>>> +	extern char __dtb_z_sun50i_a64_pinephone_1_2_start[];
>>>>> +	void *fdt;
>>>>> +	u32 size;
>>>>> +
>>>>> +	sunxi_switch_to_aarch64(.text_head_soc_header2, SUN50I_A64_RVBAR_IOMAP);
>>>>> +
>>>>> +	debug_led_init();
>>>>> +	debug_led_rgb(0xffff00);
>>>>> +
>>>>> +	sun50i_cpu_lowlevel_init();
>>>>> +	sun50i_uart_setup();
>>>>> +
>>>>> +	relocate_to_current_adr();
>>>>> +	setup_c();
>>>>> +
>>>>> +	/* Skip SDRAM initialization if we run from it */
>>>>> +	if (get_pc() < SUN50I_DRAM_ADDR) {
>>>>> +		size = sun50i_a64_lpddr3_dram_init();
>>>>> +		if (size == 0) {
>>>>> +			puts_ll("FAIL: dram init\r\n");
>>>>> +			goto reset;
>>>>> +		}
>>>>> +		puthex_ll(size);
>>>>> +		putc_ll('\r');	putc_ll('\n');
>>>>> +	}
>>>>
>>>> How can we get here with SDRAM uninitialized? Is this for USB or JTAG
>>>> boot?
>>> Yes this is when not chain loaded, when started directly from USB via fel or
>>> via JTAG.
>>>
>>>>> +
>>>>> +	puts_ll("now booting\r\n");
>>>>> +	fdt = __dtb_z_sun50i_a64_pinephone_1_2_start + get_runtime_offset();
>>>>> +	barebox_arm_entry(SUN50I_DRAM_ADDR, SZ_1G, fdt);
>>>>> +
>>>>> +reset:
>>>>> +	debug_led_rgb(0xff0000);
>>>>> +	sun50i_cpu_lowlevel_reset();
>>>>> +}
>>>>> +
>>>>> +SUN50I_A64_ENTRY_FUNCTION(start_pine64_pinephone_xload, r0, r1, r2)
>>>>> +{
>>>>> +
>>>>> +	sunxi_egon_header(.text_head_soc_header0);
>>>>> +	sunxi_switch_to_aarch64(.text_head_soc_header1, SUN50I_A64_RVBAR_IOMAP);
>>>>> +
>>>>> +	debug_led_init();
>>>>> +	debug_led_rgb(0xff0000);
>>>>> +
>>>>> +	sun50i_cpu_lowlevel_init();
>>>>> +	sun50i_uart_setup();
>>>>> +	debug_led_rgb(0xffff00);
>>>>> +
>>>>> +	relocate_to_current_adr();
>>>>> +	setup_c();
>>>>> +
>>>>> +	sun50i_a64_lpddr3_dram_init();
>>>>> +
>>>>> +	debug_led_rgb(0xff0000);
>>>>> +
>>>>
>>>> You would have to place code here to continue booting, right? In that
>>>> case you should add a comment to let the reader know that there's
>>>> something missing here.
>>> I forgot to add the code that search for barebox.bin and continue execution

I originally thought that the BootROM searches for a file in FAT, where it would
make sense to place the follow-up boot stage in FAT as well, but apparently,
it loads from a fixed offset and then first stage bootloader (here PBL) loads
second stage from FAT. This makes me wonder why use FAT at all and not just do
like we do e.g. on i.MX:

  - place full barebox at address where BootROM looks at
  - use minimum PBL size to ensure barebox truncated to SRAM behaves correctly
  - then barebox loads itself from same address into DRAM and reenters PBL
  - either have all of this in unpartitioned space at the start or create a
    partition around it if possible.

Is this possible here as well?

Also, just to be sure: barebox.bin still has a PBL right? The API between PBL
and barebox proper may change, so each barebox proper should have its own
PBL in front.

>>>
>>>>
>>>> debug_led_rgb(0xff0000) is the same color you used at the beginning of
>>>> the function. Would it make more sense to use a different color?
>>> Yes, I'll remove the first red color, I want to reserved it for error conditions.
>>>
>>>>> +	sun50i_cpu_lowlevel_reset();
>>>>
>>>> Maybe hang() here instead to give the user a chance to see the last
>>>> color?
>>> Yes, I put the reset so it will re-enter fel so the board could be reprogrammed
>>> without needing a powercycle, but that's only true if there are no valid boot
>>> entry in eMMC nor SD, IMHO this only makes sense for developpement/debugging.

How about:

if (IS_ENABLED(CONFIG_PANIC_HANG)) {
	hang();
} else {
	/* some delay to see the LED */
	sun50i_cpu_lowlevel_reset();
}
	


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |