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

[John Crispin]

Hi,

did not see the cover letter until just now.

regarding the new image building code ... just ignore that comment,
looks like x86 was not converted yet to the new code pattern

	John

On 15/10/2016 10:16, Chris Blake wrote:
> 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
>