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

2016-10-16 Thread Chris Blake 
Hey Stijn,

Thanks for the feedback, will get this fixed up for the next RFC as well.

Please keep the feedback coming. :)

- Chris B

On Sat, Oct 15, 2016 at 9:41 AM, Stijn Tintel  wrote:
> On 14-10-16 20: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 
>> ---
>>  target/linux/x86/64/config-default |  12 +
>>  target/linux/x86/64/profiles/001-PCEngines.mk  |  20 +
>>  target/linux/x86/base-files/etc/board.d/01_leds|  22 ++
>>  target/linux/x86/base-files/etc/board.d/02_network |  26 ++
>>  target/linux/x86/base-files/etc/diag.sh|  37 ++
>>  target/linux/x86/base-files/lib/x86.sh |  13 +
>>  target/linux/x86/config-4.4|   1 +
>>  .../linux/x86/files/drivers/gpio/gpio-nct5104d.c   | 432 
>> +
>>  target/linux/x86/files/drivers/leds/leds-apu2.c| 371 ++
>>  target/linux/x86/modules.mk|  36 ++
>>  .../x86/patches-4.4/800-add-apu2-led-driver.patch  |  29 ++
>>  .../801-sp5100_tco-add-apu2-support.patch  |  91 +
>>  .../patches-4.4/802-add-nct5104d-gpio-driver.patch |  27 ++
>>  13 files changed, 1117 insertions(+)
>>  create mode 100644 target/linux/x86/64/profiles/001-PCEngines.mk
>>  create mode 100755 target/linux/x86/base-files/etc/board.d/01_leds
>>  create mode 100755 target/linux/x86/base-files/etc/board.d/02_network
>>  create mode 100755 target/linux/x86/base-files/etc/diag.sh
>>  create mode 100755 target/linux/x86/base-files/lib/x86.sh
>>  create mode 100644 target/linux/x86/files/drivers/gpio/gpio-nct5104d.c
>>  create mode 100644 target/linux/x86/files/drivers/leds/leds-apu2.c
>>  create mode 100644 target/linux/x86/modules.mk
>>  create mode 100644 
>> target/linux/x86/patches-4.4/800-add-apu2-led-driver.patch
>>  create mode 100644 
>> target/linux/x86/patches-4.4/801-sp5100_tco-add-apu2-support.patch
>>  create mode 100644 
>> target/linux/x86/patches-4.4/802-add-nct5104d-gpio-driver.patch
> Please use the correct prefix for patches, see
> https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/PATCHES
> I would add the watchdog patches like this instead of squashing them,
> created with git format-patch:
> target/linux/generic/patches-4.4/097-0001-sp5100_tco-Add-AMD-Mullins-platform-support.patch
> target/linux/generic/patches-4.4/097-0002-sp5100_tco-Add-AMD-Mullins-platform-support.patch
> target/linux/generic/patches-4.4/097-0003-sp5100_tco-fix-the-device-check-for-SB800-and-later-chipsets.patch
> target/linux/generic/patches-4.4/097-0004-watchdog-sp5100_tco-properly-check-for-new-register-.patch
>
> Thanks,
> Stijn

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

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

2016-10-15 Thread Stijn Tintel 
On 14-10-16 20: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 
> ---
>  target/linux/x86/64/config-default |  12 +
>  target/linux/x86/64/profiles/001-PCEngines.mk  |  20 +
>  target/linux/x86/base-files/etc/board.d/01_leds|  22 ++
>  target/linux/x86/base-files/etc/board.d/02_network |  26 ++
>  target/linux/x86/base-files/etc/diag.sh|  37 ++
>  target/linux/x86/base-files/lib/x86.sh |  13 +
>  target/linux/x86/config-4.4|   1 +
>  .../linux/x86/files/drivers/gpio/gpio-nct5104d.c   | 432 
> +
>  target/linux/x86/files/drivers/leds/leds-apu2.c| 371 ++
>  target/linux/x86/modules.mk|  36 ++
>  .../x86/patches-4.4/800-add-apu2-led-driver.patch  |  29 ++
>  .../801-sp5100_tco-add-apu2-support.patch  |  91 +
>  .../patches-4.4/802-add-nct5104d-gpio-driver.patch |  27 ++
>  13 files changed, 1117 insertions(+)
>  create mode 100644 target/linux/x86/64/profiles/001-PCEngines.mk
>  create mode 100755 target/linux/x86/base-files/etc/board.d/01_leds
>  create mode 100755 target/linux/x86/base-files/etc/board.d/02_network
>  create mode 100755 target/linux/x86/base-files/etc/diag.sh
>  create mode 100755 target/linux/x86/base-files/lib/x86.sh
>  create mode 100644 target/linux/x86/files/drivers/gpio/gpio-nct5104d.c
>  create mode 100644 target/linux/x86/files/drivers/leds/leds-apu2.c
>  create mode 100644 target/linux/x86/modules.mk
>  create mode 100644 target/linux/x86/patches-4.4/800-add-apu2-led-driver.patch
>  create mode 100644 
> target/linux/x86/patches-4.4/801-sp5100_tco-add-apu2-support.patch
>  create mode 100644 
> target/linux/x86/patches-4.4/802-add-nct5104d-gpio-driver.patch
Please use the correct prefix for patches, see
https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/generic/PATCHES
I would add the watchdog patches like this instead of squashing them,
created with git format-patch:
target/linux/generic/patches-4.4/097-0001-sp5100_tco-Add-AMD-Mullins-platform-support.patch
target/linux/generic/patches-4.4/097-0002-sp5100_tco-Add-AMD-Mullins-platform-support.patch
target/linux/generic/patches-4.4/097-0003-sp5100_tco-fix-the-device-check-for-SB800-and-later-chipsets.patch
target/linux/generic/patches-4.4/097-0004-watchdog-sp5100_tco-properly-check-for-new-register-.patch

Thanks,
Stijn

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev

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

2016-10-15 Thread 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  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 
>>
>> [...]
>>
>>> 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 000..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

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

2016-10-15 Thread John Crispin 
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

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 

[...]

> 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 ?

>  # 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 ?


>  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

>  # 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 000..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.


[...]

> 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 ?

John

___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev