| Message ID | 20200513150324.330435-1-eugen.hristev@microchip.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | package/linux-tools: introduce spi linux tools | expand |
Hi Eugen, On Wed, May 13 2020, Eugen Hristev wrote: > Add new linux tools package : spi. This is present in the Linux Kernel since > 4.5. > It now includes spidev_test and spidev_fdx tools. We have a package for spidev_test already. Consider adding spidev_fdx to that package instead. baruch > Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com> > --- > > Inspired by the linux-tools iio mk.in file. > > package/linux-tools/Config.in | 7 ++++++ > package/linux-tools/linux-tool-spi.mk.in | 29 ++++++++++++++++++++++++ > 2 files changed, 36 insertions(+) > create mode 100644 package/linux-tools/linux-tool-spi.mk.in > > diff --git a/package/linux-tools/Config.in b/package/linux-tools/Config.in > index ceb58c668a..60df99dc4a 100644 > --- a/package/linux-tools/Config.in > +++ b/package/linux-tools/Config.in > @@ -108,6 +108,13 @@ comment "selftests needs BR2_PACKAGE_BUSYBOX_SHOW_OTHERS and a toolchain w/ dyna > depends on BR2_USE_MMU > depends on !BR2_PACKAGE_BUSYBOX_SHOW_OTHERS || BR2_STATIC_LIBS > > +config BR2_PACKAGE_LINUX_TOOLS_SPI > + bool "spi" > + select BR2_PACKAGE_LINUX_TOOLS > + help > + spi is a collection of tools to test and measure performances > + of SPI (Serial Peripheral Interface) bus devices. > + > config BR2_PACKAGE_LINUX_TOOLS_TMON > bool "tmon" > select BR2_PACKAGE_LINUX_TOOLS > diff --git a/package/linux-tools/linux-tool-spi.mk.in b/package/linux-tools/linux-tool-spi.mk.in > new file mode 100644 > index 0000000000..f6c4298380 > --- /dev/null > +++ b/package/linux-tools/linux-tool-spi.mk.in > @@ -0,0 +1,29 @@ > +################################################################################ > +# > +# spi > +# > +################################################################################ > + > +LINUX_TOOLS += spi > + > +SPI_MAKE_OPTS = $(LINUX_MAKE_FLAGS) > + > +define SPI_BUILD_CMDS > + $(Q)if ! grep install $(LINUX_DIR)/tools/spi/Makefile >/dev/null 2>&1 ; then \ > + echo "Your kernel version is too old and does not have install section in the spi tools." ; \ > + echo "At least kernel 4.5 must be used." ; \ > + exit 1 ; \ > + fi > + > + $(TARGET_MAKE_ENV) $(MAKE) -C $(LINUX_DIR)/tools/spi \ > + $(SPI_MAKE_OPTS) > +endef > + > +# DESTDIR used since kernel version 4.14 > +define SPI_INSTALL_TARGET_CMDS > + $(TARGET_MAKE_ENV) $(MAKE) -C $(LINUX_DIR)/tools/spi \ > + $(SPI_MAKE_OPTS) \ > + INSTALL_ROOT=$(TARGET_DIR) \ > + DESTDIR=$(TARGET_DIR) \ > + install > +endef
On 13.05.2020 18:21, Baruch Siach wrote: > Hi Eugen, > > On Wed, May 13 2020, Eugen Hristev wrote: >> Add new linux tools package : spi. This is present in the Linux Kernel since >> 4.5. >> It now includes spidev_test and spidev_fdx tools. > > We have a package for spidev_test already. Consider adding spidev_fdx to > that package instead. > Hi Baruch, While I understand your point, you should also consider that all linux tools have a specific way to build in buildroot, and this I think should be consistent. And spi tools would be not just spidev_test and might include future applications, which may or may not have their place in the same spot as spidev_test . Eugen > baruch > >> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com> >> --- >> >> Inspired by the linux-tools iio mk.in file. >> >> package/linux-tools/Config.in | 7 ++++++ >> package/linux-tools/linux-tool-spi.mk.in | 29 ++++++++++++++++++++++++ >> 2 files changed, 36 insertions(+) >> create mode 100644 package/linux-tools/linux-tool-spi.mk.in >> >> diff --git a/package/linux-tools/Config.in b/package/linux-tools/Config.in >> index ceb58c668a..60df99dc4a 100644 >> --- a/package/linux-tools/Config.in >> +++ b/package/linux-tools/Config.in >> @@ -108,6 +108,13 @@ comment "selftests needs BR2_PACKAGE_BUSYBOX_SHOW_OTHERS and a toolchain w/ dyna >> depends on BR2_USE_MMU >> depends on !BR2_PACKAGE_BUSYBOX_SHOW_OTHERS || BR2_STATIC_LIBS >> >> +config BR2_PACKAGE_LINUX_TOOLS_SPI >> + bool "spi" >> + select BR2_PACKAGE_LINUX_TOOLS >> + help >> + spi is a collection of tools to test and measure performances >> + of SPI (Serial Peripheral Interface) bus devices. >> + >> config BR2_PACKAGE_LINUX_TOOLS_TMON >> bool "tmon" >> select BR2_PACKAGE_LINUX_TOOLS >> diff --git a/package/linux-tools/linux-tool-spi.mk.in b/package/linux-tools/linux-tool-spi.mk.in >> new file mode 100644 >> index 0000000000..f6c4298380 >> --- /dev/null >> +++ b/package/linux-tools/linux-tool-spi.mk.in >> @@ -0,0 +1,29 @@ >> +################################################################################ >> +# >> +# spi >> +# >> +################################################################################ >> + >> +LINUX_TOOLS += spi >> + >> +SPI_MAKE_OPTS = $(LINUX_MAKE_FLAGS) >> + >> +define SPI_BUILD_CMDS >> + $(Q)if ! grep install $(LINUX_DIR)/tools/spi/Makefile >/dev/null 2>&1 ; then \ >> + echo "Your kernel version is too old and does not have install section in the spi tools." ; \ >> + echo "At least kernel 4.5 must be used." ; \ >> + exit 1 ; \ >> + fi >> + >> + $(TARGET_MAKE_ENV) $(MAKE) -C $(LINUX_DIR)/tools/spi \ >> + $(SPI_MAKE_OPTS) >> +endef >> + >> +# DESTDIR used since kernel version 4.14 >> +define SPI_INSTALL_TARGET_CMDS >> + $(TARGET_MAKE_ENV) $(MAKE) -C $(LINUX_DIR)/tools/spi \ >> + $(SPI_MAKE_OPTS) \ >> + INSTALL_ROOT=$(TARGET_DIR) \ >> + DESTDIR=$(TARGET_DIR) \ >> + install >> +endef > > > -- > ~. .~ Tk Open Systems > =}------------------------------------------------ooO--U--Ooo------------{= > - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il - >
Hi Eugen, On Wed, May 13 2020, Eugen.Hristev@microchip.com wrote: > On 13.05.2020 18:21, Baruch Siach wrote: >> On Wed, May 13 2020, Eugen Hristev wrote: >>> Add new linux tools package : spi. This is present in the Linux Kernel since >>> 4.5. >>> It now includes spidev_test and spidev_fdx tools. >> >> We have a package for spidev_test already. Consider adding spidev_fdx to >> that package instead. > > While I understand your point, you should also consider that all linux > tools have a specific way to build in buildroot, and this I think should > be consistent. Agreed. > And spi tools would be not just spidev_test and might include future > applications, which may or may not have their place in the same spot as > spidev_test . In that case I think you should remove the spidev_test package. baruch >>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com> >>> --- >>> >>> Inspired by the linux-tools iio mk.in file. >>> >>> package/linux-tools/Config.in | 7 ++++++ >>> package/linux-tools/linux-tool-spi.mk.in | 29 ++++++++++++++++++++++++ >>> 2 files changed, 36 insertions(+) >>> create mode 100644 package/linux-tools/linux-tool-spi.mk.in >>> >>> diff --git a/package/linux-tools/Config.in b/package/linux-tools/Config.in >>> index ceb58c668a..60df99dc4a 100644 >>> --- a/package/linux-tools/Config.in >>> +++ b/package/linux-tools/Config.in >>> @@ -108,6 +108,13 @@ comment "selftests needs BR2_PACKAGE_BUSYBOX_SHOW_OTHERS and a toolchain w/ dyna >>> depends on BR2_USE_MMU >>> depends on !BR2_PACKAGE_BUSYBOX_SHOW_OTHERS || BR2_STATIC_LIBS >>> >>> +config BR2_PACKAGE_LINUX_TOOLS_SPI >>> + bool "spi" >>> + select BR2_PACKAGE_LINUX_TOOLS >>> + help >>> + spi is a collection of tools to test and measure performances >>> + of SPI (Serial Peripheral Interface) bus devices. >>> + >>> config BR2_PACKAGE_LINUX_TOOLS_TMON >>> bool "tmon" >>> select BR2_PACKAGE_LINUX_TOOLS >>> diff --git a/package/linux-tools/linux-tool-spi.mk.in b/package/linux-tools/linux-tool-spi.mk.in >>> new file mode 100644 >>> index 0000000000..f6c4298380 >>> --- /dev/null >>> +++ b/package/linux-tools/linux-tool-spi.mk.in >>> @@ -0,0 +1,29 @@ >>> +################################################################################ >>> +# >>> +# spi >>> +# >>> +################################################################################ >>> + >>> +LINUX_TOOLS += spi >>> + >>> +SPI_MAKE_OPTS = $(LINUX_MAKE_FLAGS) >>> + >>> +define SPI_BUILD_CMDS >>> + $(Q)if ! grep install $(LINUX_DIR)/tools/spi/Makefile >/dev/null 2>&1 ; then \ >>> + echo "Your kernel version is too old and does not have install section in the spi tools." ; \ >>> + echo "At least kernel 4.5 must be used." ; \ >>> + exit 1 ; \ >>> + fi >>> + >>> + $(TARGET_MAKE_ENV) $(MAKE) -C $(LINUX_DIR)/tools/spi \ >>> + $(SPI_MAKE_OPTS) >>> +endef >>> + >>> +# DESTDIR used since kernel version 4.14 >>> +define SPI_INSTALL_TARGET_CMDS >>> + $(TARGET_MAKE_ENV) $(MAKE) -C $(LINUX_DIR)/tools/spi \ >>> + $(SPI_MAKE_OPTS) \ >>> + INSTALL_ROOT=$(TARGET_DIR) \ >>> + DESTDIR=$(TARGET_DIR) \ >>> + install >>> +endef
On 2020-05-13 20:46 +0300, Baruch Siach spake thusly: > Hi Eugen, > > On Wed, May 13 2020, Eugen.Hristev@microchip.com wrote: > > On 13.05.2020 18:21, Baruch Siach wrote: > >> On Wed, May 13 2020, Eugen Hristev wrote: > >>> Add new linux tools package : spi. This is present in the Linux Kernel since > >>> 4.5. > >>> It now includes spidev_test and spidev_fdx tools. > >> We have a package for spidev_test already. Consider adding spidev_fdx to > >> that package instead. > > While I understand your point, you should also consider that all linux > > tools have a specific way to build in buildroot, and this I think should > > be consistent. > Agreed. > > And spi tools would be not just spidev_test and might include future > > applications, which may or may not have their place in the same spot as > > spidev_test . > In that case I think you should remove the spidev_test package. Yes, I like it that we can drop spidev_test. But do it multi-step: - intropduce spidev as a linux-tool - drop the standalone spidev_test Do not forget to add legacy handling for it, though; see Config.in.legacy. Regards, Yann E. MORIN.
On 13.05.2020 22:58, Yann E. MORIN wrote: > On 2020-05-13 20:46 +0300, Baruch Siach spake thusly: >> Hi Eugen, >> >> On Wed, May 13 2020, Eugen.Hristev@microchip.com wrote: >>> On 13.05.2020 18:21, Baruch Siach wrote: >>>> On Wed, May 13 2020, Eugen Hristev wrote: >>>>> Add new linux tools package : spi. This is present in the Linux Kernel since >>>>> 4.5. >>>>> It now includes spidev_test and spidev_fdx tools. >>>> We have a package for spidev_test already. Consider adding spidev_fdx to >>>> that package instead. >>> While I understand your point, you should also consider that all linux >>> tools have a specific way to build in buildroot, and this I think should >>> be consistent. >> Agreed. >>> And spi tools would be not just spidev_test and might include future >>> applications, which may or may not have their place in the same spot as >>> spidev_test . >> In that case I think you should remove the spidev_test package. > > Yes, I like it that we can drop spidev_test. > > But do it multi-step: > > - intropduce spidev as a linux-tool > - drop the standalone spidev_test > > Do not forget to add legacy handling for it, though; see Config.in.legacy. Hi Yann and Baruch, Coming back to this, I noticed that the spidev_test package in buildroot is different than the spidev_test from kernel tools. From spidev_test package: Usage: spidev_test [-DsbdlHOLC3] -D --device device to use (default /dev/spidev1.1) -s --speed max speed (Hz) -d --delay delay (usec) -b --bpw bits per word -i --input input data from a file (e.g. "test.bin") -o --output output data to a file (e.g. "results.bin") -l --loop loopback -H --cpha clock phase -O --cpol clock polarity -L --lsb least significant bit first -C --cs-high chip select active high -3 --3wire SI/SO signals shared -v --verbose Verbose (show tx buffer) -p Send data (e.g. "1234\xde\xad") -N --no-cs no chip select -R --ready slave pulls low to pause -2 --dual dual transfer -4 --quad quad transfer # From kernel tools: https://elixir.bootlin.com/linux/v5.7/source/tools/spi/spidev_test.c#L198 Looks it's a different program, or I am missing something ? In that case, I would think we cannot supersede the existing spidev_test package with the kernel tools. I would think both should be in the distribution, but how to differentiate them ? Thanks, Eugen > > Regards, > Yann E. MORIN. > > -- > .-----------------.--------------------.------------------.--------------------. > | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | > | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | > | +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no | > | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | > '------------------------------^-------^------------------^--------------------' >
Hi Eugen, On Thu, Jun 04 2020, Eugen.Hristev@microchip.com wrote: > On 13.05.2020 22:58, Yann E. MORIN wrote: >> On 2020-05-13 20:46 +0300, Baruch Siach spake thusly: >>> Hi Eugen, >>> >>> On Wed, May 13 2020, Eugen.Hristev@microchip.com wrote: >>>> On 13.05.2020 18:21, Baruch Siach wrote: >>>>> On Wed, May 13 2020, Eugen Hristev wrote: >>>>>> Add new linux tools package : spi. This is present in the Linux Kernel since >>>>>> 4.5. >>>>>> It now includes spidev_test and spidev_fdx tools. >>>>> We have a package for spidev_test already. Consider adding spidev_fdx to >>>>> that package instead. >>>> While I understand your point, you should also consider that all linux >>>> tools have a specific way to build in buildroot, and this I think should >>>> be consistent. >>> Agreed. >>>> And spi tools would be not just spidev_test and might include future >>>> applications, which may or may not have their place in the same spot as >>>> spidev_test . >>> In that case I think you should remove the spidev_test package. >> >> Yes, I like it that we can drop spidev_test. >> >> But do it multi-step: >> >> - intropduce spidev as a linux-tool >> - drop the standalone spidev_test >> >> Do not forget to add legacy handling for it, though; see Config.in.legacy. > > Hi Yann and Baruch, > > Coming back to this, I noticed that the spidev_test package in buildroot > is different than the spidev_test from kernel tools. > > From spidev_test package: > > Usage: spidev_test [-DsbdlHOLC3] > -D --device device to use (default /dev/spidev1.1) > -s --speed max speed (Hz) > -d --delay delay (usec) > -b --bpw bits per word > -i --input input data from a file (e.g. "test.bin") > -o --output output data to a file (e.g. "results.bin") > -l --loop loopback > -H --cpha clock phase > -O --cpol clock polarity > -L --lsb least significant bit first > -C --cs-high chip select active high > -3 --3wire SI/SO signals shared > -v --verbose Verbose (show tx buffer) > -p Send data (e.g. "1234\xde\xad") > -N --no-cs no chip select > -R --ready slave pulls low to pause > -2 --dual dual transfer > -4 --quad quad transfer > > From kernel tools: > > https://elixir.bootlin.com/linux/v5.7/source/tools/spi/spidev_test.c#L198 > > Looks it's a different program, or I am missing something ? > > In that case, I would think we cannot supersede the existing spidev_test > package with the kernel tools. > > I would think both should be in the distribution, but how to > differentiate them ? The file you link to is a newer version. You can find the version that buildroot uses here: https://elixir.bootlin.com/linux/v4.10/source/tools/spi/spidev_test.c Note SPIDEV_TEST_VERSION. In my opinion, we can update to the latest version like any other upstream package. baruch
Eugen, Baruch, All, On 2020-06-04 18:35 +0300, Baruch Siach spake thusly: > On Thu, Jun 04 2020, Eugen.Hristev@microchip.com wrote: [--SNIP--] > > Coming back to this, I noticed that the spidev_test package in buildroot > > is different than the spidev_test from kernel tools. > The file you link to is a newer version. You can find the version that > buildroot uses here: > https://elixir.bootlin.com/linux/v4.10/source/tools/spi/spidev_test.c > > Note SPIDEV_TEST_VERSION. > > In my opinion, we can update to the latest version like any other > upstream package. This is not so clear-cut. As first approximation, I would have agreed. And that is what I also initially suggested earlied in the thread. However, that would be a regression to remove spidev_test in favour of the one in linux-tools. Indeed, it is today possible to build spidev_test even without building a kernel. By moving it as a linux-tools, that would no longer be possible. And second, depending on the kernel version being used, there could be a dependency on the jkernel headers version. If kernel >= 3.15, spidev_tool would require a toolchain with headers >= 3.15 too. But moving it as a linux-tools also makes it on-par with the other ones, so this is acceptable too. So, we have a few options (in my order of preference, but we can discuss it): 1. introduce spidev_test as a linux-tools, drop the old one, and handle legacy; 2. introduce spidev_test as a lijnux-tools, keep the old one for when a kernel is not being built. 3. introduce spidev_test as a lijnux-tools, keep the old one for when the new linux-tools version is not enabled. Option (1) makes it totally like any other linux-tools, and I am OK with the regression of not being able to build it without a kernel. But I understand that this is a regression. Option (2) is my close-to-first second prefered option, as it does not provide a rgression, at the expense of a bit of possible confusion for users. Option (3) I do not like much, because it is even more confusing. In either case, care must be taken to account for the kernel headers version in the toolchain (which might be a prebuilt one older than the selected kernel): - if the kernel is 3.15 or later, spidev_test requires a toolchain with headers at least 3.15 However, we do not have a way to know that situation... So either we bite the bullet, and consider 3.15 are madnatory for spidev_test in linux-tools, or we do a test at build time, whether spidev_test uses SPI_TX_QUAD/SPI_RX_QUAD or not, and bail with an explicit error if thios is the case and the toolchain uses older kernel headers... Regards, Yann E. MORIN.
diff --git a/package/linux-tools/Config.in b/package/linux-tools/Config.in index ceb58c668a..60df99dc4a 100644 --- a/package/linux-tools/Config.in +++ b/package/linux-tools/Config.in @@ -108,6 +108,13 @@ comment "selftests needs BR2_PACKAGE_BUSYBOX_SHOW_OTHERS and a toolchain w/ dyna depends on BR2_USE_MMU depends on !BR2_PACKAGE_BUSYBOX_SHOW_OTHERS || BR2_STATIC_LIBS +config BR2_PACKAGE_LINUX_TOOLS_SPI + bool "spi" + select BR2_PACKAGE_LINUX_TOOLS + help + spi is a collection of tools to test and measure performances + of SPI (Serial Peripheral Interface) bus devices. + config BR2_PACKAGE_LINUX_TOOLS_TMON bool "tmon" select BR2_PACKAGE_LINUX_TOOLS diff --git a/package/linux-tools/linux-tool-spi.mk.in b/package/linux-tools/linux-tool-spi.mk.in new file mode 100644 index 0000000000..f6c4298380 --- /dev/null +++ b/package/linux-tools/linux-tool-spi.mk.in @@ -0,0 +1,29 @@ +################################################################################ +# +# spi +# +################################################################################ + +LINUX_TOOLS += spi + +SPI_MAKE_OPTS = $(LINUX_MAKE_FLAGS) + +define SPI_BUILD_CMDS + $(Q)if ! grep install $(LINUX_DIR)/tools/spi/Makefile >/dev/null 2>&1 ; then \ + echo "Your kernel version is too old and does not have install section in the spi tools." ; \ + echo "At least kernel 4.5 must be used." ; \ + exit 1 ; \ + fi + + $(TARGET_MAKE_ENV) $(MAKE) -C $(LINUX_DIR)/tools/spi \ + $(SPI_MAKE_OPTS) +endef + +# DESTDIR used since kernel version 4.14 +define SPI_INSTALL_TARGET_CMDS + $(TARGET_MAKE_ENV) $(MAKE) -C $(LINUX_DIR)/tools/spi \ + $(SPI_MAKE_OPTS) \ + INSTALL_ROOT=$(TARGET_DIR) \ + DESTDIR=$(TARGET_DIR) \ + install +endef
Add new linux tools package : spi. This is present in the Linux Kernel since 4.5. It now includes spidev_test and spidev_fdx tools. Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com> --- Inspired by the linux-tools iio mk.in file. package/linux-tools/Config.in | 7 ++++++ package/linux-tools/linux-tool-spi.mk.in | 29 ++++++++++++++++++++++++ 2 files changed, 36 insertions(+) create mode 100644 package/linux-tools/linux-tool-spi.mk.in