[LEDE-DEV] [RFC 1/1] x86: Add support for the PC Engines APU2 Board

Sat Oct 15 01:16:34 PDT 2016

Hello Jon,

My comments can be found below:

On Sat, Oct 15, 2016 at 2:27 AM, John Crispin <john at phrozen.org> wrote:
> Hi Chris,
>
> i had  quick read of the patch and it looks good at first glance. i
> would suggest you split he patch up as follows.
>
> 1) add the 2 new drivers
> 2) add the support for the new board
> 3) the patches that add board.d, led, diag ... and all the other parts
> adding the rootfs board support to x86

Will add this to the next RFC, Thanks.

> few more comments inline
>
> On 14/10/2016 19:19, Chris Blake wrote:
>> The following patch adds support for the PC Engines APU2 Embedded Board
>> as a profile under the X86_64 target. More information on this board can
>> be found at www.pcengines.ch/apu2c4.htm
>>
>> Note that this patch is a part of an RFC, and should not be merged yet.
>>
>> Signed-off-by: Chris Blake <chrisrblake93 at gmail.com>
>
> [...]
>
>> diff --git a/target/linux/x86/64/config-default b/target/linux/x86/64/config-default
>> index 646e773..bbd1b44 100644
>> --- a/target/linux/x86/64/config-default
>> +++ b/target/linux/x86/64/config-default
>> @@ -74,6 +74,7 @@ CONFIG_CRYPTO_CRCT10DIF=y
>>  # CONFIG_CRYPTO_TWOFISH_AVX_X86_64 is not set
>>  # CONFIG_CRYPTO_TWOFISH_X86_64 is not set
>>  # CONFIG_CRYPTO_TWOFISH_X86_64_3WAY is not set
>> +CONFIG_DEVMEM=y
>
> are you really using devmem ?

This is used by flashrom, and is mentioned in the cover letter of this
patch list. If we remove flashrom this can also be removed but then
users have no way to update the BIOS on the APU2. Again, this goes to
the other issue mentioned of what should be included within the
profile. Should it be as minimal as possible (no packages/wireless),
or should it support all board features out of box?

>
>>  # CONFIG_EFI is not set
>>  CONFIG_FB=y
>>  CONFIG_FB_CMDLINE=y
>> @@ -91,6 +92,14 @@ CONFIG_GART_IOMMU=y
>>  CONFIG_GENERIC_BUG_RELATIVE_POINTERS=y
>>  CONFIG_GENERIC_CPU=y
>>  CONFIG_GENERIC_PENDING_IRQ=y
>> +CONFIG_GPIOLIB=y
>> +# CONFIG_GPIO_104_IDIO_16 is not set
>> +# CONFIG_GPIO_AMDPT is not set
>> +# CONFIG_GPIO_F7188X is not set
>> +# CONFIG_GPIO_INTEL_MID is not set
>> +# CONFIG_GPIO_IT87 is not set
>> +# CONFIG_GPIO_LYNXPOINT is not set
>> +CONFIG_GPIO_SYSFS=y
>
> is this nt set in the generic config already ?

It is not. https://github.com/lede-project/source/blob/master/target/linux/generic/config-4.4#L1233

>
>>  CONFIG_HAVE_ACPI_APEI=y
>>  CONFIG_HAVE_ACPI_APEI_NMI=y
>>  # CONFIG_HAVE_AOUT is not set
>> @@ -119,6 +128,8 @@ CONFIG_HVC_DRIVER=y
>>  CONFIG_HW_RANDOM_AMD=y
>>  CONFIG_HW_RANDOM_INTEL=y
>>  CONFIG_HW_RANDOM_VIRTIO=y
>> +CONFIG_HWMON=y
>> +#CONFIG_HWMON_DEBUG_CHIP is not set
>>  CONFIG_HYPERVISOR_GUEST=y
>>  # CONFIG_I7300_IDLE is not set
>>  # CONFIG_IA32_EMULATION is not set
>> @@ -135,6 +146,7 @@ CONFIG_ITCO_WDT=y
>>  # CONFIG_KVM_DEBUG_FS is not set
>>  CONFIG_KVM_GUEST=y
>>  # CONFIG_LCD_CLASS_DEVICE is not set
>> +CONFIG_LEDS_APU2=y
>
> should go in the subtarget building the apu2

As mentioned in the IRC chat earlier, this needs to be in the kernel
config for the LEDs to be accessible during boot. Rest assured, the
driver does do platform checks to ensure it only loads on the APU2
board.

>
>>  # CONFIG_LEGACY_VSYSCALL_EMULATE is not set
>>  # CONFIG_LEGACY_VSYSCALL_NATIVE is not set
>>  CONFIG_LEGACY_VSYSCALL_NONE=y
>> diff --git a/target/linux/x86/64/profiles/001-PCEngines.mk b/target/linux/x86/64/profiles/001-PCEngines.mk
>> new file mode 100644
>> index 0000000..9ac651b
>> --- /dev/null
>> +++ b/target/linux/x86/64/profiles/001-PCEngines.mk
>> @@ -0,0 +1,20 @@
>> +#
>> +# Copyright (C) 2016 OpenWrt.org
>> +#
>> +# This is free software, licensed under the GNU General Public License v2.
>> +# See /LICENSE for more information.
>> +#
>> +
>> +define Profile/APU2
>> +  NAME:=PC Engines APU2
>> +  PACKAGES:=flashrom hwclock libsensors lm-sensors wpad-mini \
>> +     kmod-ath9k kmod-ath10k kmod-gpio-button-hotplug kmod-gpio-nct5104d \
>> +     kmod-hwmon-k10temp kmod-leds-gpio kmod-sp5100_tco \
>> +     kmod-usb-core kmod-usb-ohci kmod-usb2 kmod-usb3
>> +
>> +endef
>> +
>> +define Profile/APU2/Description
>> +     PC Engines APU2 Embedded Board
>> +endef
>> +$(eval $(call Profile,APU2))
>
> since introducing the new target making code, the default packahes can
> be encoded directy inside the image making code. please mae use of that
> feature.
>

Hmm, I was unaware of this change. Do you have any examples of it in
use I can use to base this on?

> [...]
>
>> diff --git a/target/linux/x86/config-4.4 b/target/linux/x86/config-4.4
>> index 894fed0..c6c9bf6 100644
>> --- a/target/linux/x86/config-4.4
>> +++ b/target/linux/x86/config-4.4
>> @@ -229,6 +229,7 @@ CONFIG_ILLEGAL_POINTER_VALUE=0
>>  CONFIG_INITRAMFS_SOURCE=""
>>  CONFIG_INPUT=y
>>  CONFIG_INPUT_KEYBOARD=y
>> +CONFIG_INPUT_PCSPKR=y
>
> is this really needed ?

If we want to use the PC Buzzer, this will be needed. This is also
covered in the cover letter.

- Chris B