Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add extended parameter "disable_no_sc" for linuxspi programmer #1100

Merged
merged 5 commits into from
Sep 24, 2022

Conversation

s-wakaba
Copy link
Contributor

The current version of AVRDUDE cannot be used as "linuxspi programmer" on linux systems running on Allwinner SoCs which are used for some Raspberry Pi like singleboard computers (e.g. OrangePi, NanoPi). The reason is failure of "ioctl()" system call to set SPI_NO_CS option to the SPI device descriptor. This patch adds "disablbe_no_cs" extended parameter for linuxspi programmer to suppress this system call failure.
In addition, "build.sh" is also modified since the "uname -m" command returns "aarch*" instead of "arm*" as architecture name for these SoCs.

@mcuee mcuee added the enhancement New feature or request label Sep 19, 2022
@stefanrueger
Copy link
Collaborator

Thank you for this PR @s-wakaba!

It is certainly competently coded. And it's good to see more SoCs covered!

I don't have an OrangePi (or a NanoPi for that matter) so cannot check, but here is a question: Is there a way to detect at compile-time whether SPI_NO_CS will need setting, eg, something that tells us it is a linux system running on Allwinner SoCs. Then this could be coded, perhaps using #if ..., so it works automagically, ie, the user would not need to known about this flag (or about the cause why this flag needs to be specified), would not need to remember to supply the flag and there would be no need to document this flag.

@stefanrueger
Copy link
Collaborator

In addition, "build.sh" is also modified since the "uname -m" command returns "aarch*" instead of "arm*" as architecture name

This enables both linuxspi and linuxgpio for aarch. Have you tested both?

@mcuee
Copy link
Collaborator

mcuee commented Sep 22, 2022

I think the change to build.sh should be correct for 64bit ARM OS.

As for SPI_NO_CS option, it is defined here.
https://elixir.bootlin.com/linux/latest/ident/SPI_NO_CS
https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/spi/spi.h

And indeed it is not defined for AllWinner ARM SoCs (linux-sunxi). So I agree that a compiler-time flag should be used.

@s-wakaba
Copy link
Contributor Author

Thank you for positive comments.

I don't know how to detect the SPI_NO_CS availability on compile-time. One practical idea is trying to run tiny code actually, but I cannot be convinced that manipuration of the SPI I/O is safe and the user has the permission (using /dev/spidev* requires root on Armbian system). Another idea is refering kernel information for example uname -r result. However, I think making and mainteining correct, necessary and sufficient kernel version list is difficult.
If there are SoCs which have multiple SPI I/Fs and part of them support SPI_NO_CS, how should I do?

In addition, some softwares (typically Arduino IDE) include compiled avrdude. I think it's difficult for these distributers to prepare packages for each SoCs.

An explicit option has a little merit that the user know that the SPI CS pin outputs meaningless signals and it shouldn't be used.
If the option to suppress using SPI_NO_CS can be set in the avrdude.conf file, I thing it's a better idea. Linux distro maintainers also can make avrdude package with the modified config file.

I'll try to use linuxgpio programmer on a Allwinner SoC system.

@s-wakaba
Copy link
Contributor Author

linuxgpio programmer works on Nano Pi Neo 2 + Armbian(Ubuntu 22.04.1 based) + ATtiny85.
I tried some pin combinations and no problem.

@stefanrueger
Copy link
Collaborator

linuxgpio programmer works on Nano Pi Neo 2 + Armbian(Ubuntu 22.04.1 based) + ATtiny85.

Brilliant, thanks.

OK, we can go with the suggestion of the extended parameter.

I like the fact that your code makes the user aware of the extended parameter and how to use it. I have pushed a change onto your code which lowers this message from conditioned to -v (MSG_NOTICE) to unconditional (MSG_INFO). Nevertheless it would be good to describe the additional parameter in avrdude.1 and doc/avrdude.texi. Also used a newly introduced function cfg_malloc() that allocates memory, clears it and if out of memory exits.

While I was at it I also changed the style of error messages of the existing linuxspi.c to match style. I hope that's OK.

Please review and test.

@stefanrueger
Copy link
Collaborator

@s-wakaba Also wondering whether it should rather be

        if(ioctl_errno == EINVAL && !PDATA(pgm)->disable_no_cs)
            avrdude_message(MSG_INFO, "%s: try -x disable_no_cs\n", progname);

ie, && instead of || because if -x disable_no_cs was already set and there was still an error, this message is not useful.

@s-wakaba
Copy link
Contributor Author

@stefanrueger Thank you for agreeing to my proposal!

In the case of using ||, the hint message will tell the users to try disable_no_cs option if errno of failure inctl() is EINVAL (22, means "Invalid argument"), even if disable_no_cs has already been set. My code using && is intended to prompt the trial of disable_no_cs only in more limited conditions: ioctl() has returned EINVAL AND disable_no_cs has NOT been set. One concern is that there may be systems which return errno other than EINVAL on calling ioctl() with unsupported SPI_NO_CS flag. Only case I tried, linux-sunxi kernel for Allwinner SoCs, returned EINVAL. Please consider which method is better.

If you update the entire linuxspi.c file, it would be great if you could add perror() (or similar effect functions) every time a system call fails. I had a few times with confusing errors (e.g. invalid permission and incorrect device file: phisical GPIO pins on my board assigned not avrdude default /dev/gpiochip0 but gpiochip1) and searched the error messages into the source code before I got avrdude working on a new SoC board.

@stefanrueger
Copy link
Collaborator

Please consider which method is better

The choice is between

  • ioctl_errno == EINVAL && !PDATA(pgm)->disable_no_cs
  • !PDATA(pgm)->disable_no_cs
    If we are confident, EINVAL is triggered on missing SPI_NO_CS support (which is a reasonable assumption) then I'd prefer the former, otherwise I'd prefer the latter.

Please could you test the changes using strerror(). As there are quite a few PRs ready to go I might merge this soon with others. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants