| Message ID | 20210712150242.146545-4-sebastian.reichel@collabora.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | GE Healthcare PPD firmware upgrade driver for ACHC | expand |
On Mon, Jul 12, 2021 at 05:02:42PM +0200, Sebastian Reichel wrote: > General Electric Healthcare's PPD has a secondary processor from > NXP's Kinetis K20 series. That device has two SPI chip selects: > > The main interface's behaviour depends on the loaded firmware > and is currently unused. > > The secondary interface can be used to update the firmware using > EzPort protocol. This is implemented by this driver using the > kernel's firmware API. It's not done during probe time, since > the device has non-volatile memory and flashing lasts almost 3 > minutes. In thinking about this some more, why does it matter? Spin up a workqueue when probing and do the firmware loading then. That way you do not end up creating yet another custom user/kernel api just to do something as trivial as loading the firmware for a device. And I think the firmware loader even handles async loading, or at least it used to, maybe not anymore, it's a complex api, I recommend reading the docs... > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > --- > .../ABI/testing/sysfs-driver-ge-achc | 14 + > drivers/misc/Kconfig | 11 + > drivers/misc/Makefile | 2 + > drivers/misc/gehc-achc.c | 136 +++++ > drivers/misc/nxp-ezport.c | 468 ++++++++++++++++++ > drivers/misc/nxp-ezport.h | 9 + Why is there two different modules here for the same piece of hardware? > drivers/spi/spidev.c | 1 - > 7 files changed, 640 insertions(+), 1 deletion(-) > create mode 100644 Documentation/ABI/testing/sysfs-driver-ge-achc > create mode 100644 drivers/misc/gehc-achc.c > create mode 100644 drivers/misc/nxp-ezport.c > create mode 100644 drivers/misc/nxp-ezport.h > > diff --git a/Documentation/ABI/testing/sysfs-driver-ge-achc b/Documentation/ABI/testing/sysfs-driver-ge-achc > new file mode 100644 > index 000000000000..f02812731082 > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-driver-ge-achc > @@ -0,0 +1,14 @@ > +What: /sys/bus/spi/<dev>/update_firmware > +Date: Jul 2021 > +Contact: sebastian.reichel@collabora.com > +Description: Write 1 to this file to update the ACHC microcontroller > + firmware via the EzPort interface. For this the kernel > + will load "achc.bin" via the firmware API (so usually > + from /lib/firmware). The write will block until the FW > + has either been flashed successfully or an error occured. > + > +What: /sys/bus/spi/<dev>/reset > +Date: Jul 2021 > +Contact: sebastian.reichel@collabora.com > +Description: Write 1 to this file to reset the microcontroller via the > + reset GPIO. The write will block until the reset completes. > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig > index f4fb5c52b863..1ebf7000671d 100644 > --- a/drivers/misc/Kconfig > +++ b/drivers/misc/Kconfig > @@ -208,6 +208,17 @@ config CS5535_CLOCK_EVENT_SRC > MFGPTs have a better resolution and max interval than the > generic PIT, and are suitable for use as high-res timers. > > +config GEHC_ACHC > + tristate "GEHC ACHC support" > + depends on SPI && SYSFS > + select FW_LOADER > + help > + Support for GE ACHC microcontroller, that is part of the GE > + PPD device. > + > + To compile this driver as a module, choose M here: the > + module will be called gehc-achc. > + > config HP_ILO > tristate "Channel interface driver for the HP iLO processor" > depends on PCI > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile > index e92a56d4442f..b50180796931 100644 > --- a/drivers/misc/Makefile > +++ b/drivers/misc/Makefile > @@ -24,6 +24,8 @@ obj-$(CONFIG_KGDB_TESTS) += kgdbts.o > obj-$(CONFIG_SGI_XP) += sgi-xp/ > obj-$(CONFIG_SGI_GRU) += sgi-gru/ > obj-$(CONFIG_CS5535_MFGPT) += cs5535-mfgpt.o > +obj-$(CONFIG_GEHC_ACHC) += gehc-achc-mod.o > +gehc-achc-mod-objs := gehc-achc.o nxp-ezport.o Yeah, you are creating 2 modules for one Kconfig option, why not put these together into 1? Makes the code smaller. > obj-$(CONFIG_HP_ILO) += hpilo.o > obj-$(CONFIG_APDS9802ALS) += apds9802als.o > obj-$(CONFIG_ISL29003) += isl29003.o > diff --git a/drivers/misc/gehc-achc.c b/drivers/misc/gehc-achc.c > new file mode 100644 > index 000000000000..893940e7ca4e > --- /dev/null > +++ b/drivers/misc/gehc-achc.c > @@ -0,0 +1,136 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * datasheet: https://www.nxp.com/docs/en/data-sheet/K20P144M120SF3.pdf > + * > + * Copyright (C) 2018-2021 Collabora > + * Copyright (C) 2018-2021 GE Healthcare > + */ > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/spi/spi.h> > +#include <linux/of.h> > +#include "nxp-ezport.h" > + > +#define ACHC_MAX_FREQ_HZ 300000 > + > +struct achc_data { > + struct spi_device *main; > + struct spi_device *ezport; > + struct gpio_desc *reset; > + > + struct mutex device_lock; /* avoid concurrent device access */ > +}; > + > +static ssize_t update_firmware_store(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct achc_data *achc = dev_get_drvdata(dev); > + int ret; > + > + if (count != 1 || buf[0] != '1') > + return -EINVAL; There a core kernel call to see if the data written to a sysfs file is "true/false" I would recommend, if you stick with this, to use that instead. thanks, greg k-h
Hi Greg, On Mon, Jul 12, 2021 at 07:46:05PM +0200, Greg Kroah-Hartman wrote: > On Mon, Jul 12, 2021 at 05:02:42PM +0200, Sebastian Reichel wrote: > > General Electric Healthcare's PPD has a secondary processor from > > NXP's Kinetis K20 series. That device has two SPI chip selects: > > > > The main interface's behaviour depends on the loaded firmware > > and is currently unused. > > > > The secondary interface can be used to update the firmware using > > EzPort protocol. This is implemented by this driver using the > > kernel's firmware API. It's not done during probe time, since > > the device has non-volatile memory and flashing lasts almost 3 > > minutes. > > In thinking about this some more, why does it matter? Spin up a > workqueue when probing and do the firmware loading then. That way you > do not end up creating yet another custom user/kernel api just to do > something as trivial as loading the firmware for a device. > > And I think the firmware loader even handles async loading, or at least > it used to, maybe not anymore, it's a complex api, I recommend reading > the docs... Flashing the firmware during boot instead of on-demand is not a good idea for two reasons: 1. This will wear the flash memory of the microcontroller for no good reason. 2. Even if the flashing happens in the background, the microcontroller will not be usable while it is being programmed. Thus the system boot takes 3 minutes instead of 30 seconds. Note, that this is not the first device with this kind of problem. I've seen this before for e.g. atmel touchscreen [0] basically does the same via (undocumented) mxt_update_fw_store(). [0] drivers/input/touchscreen/atmel_mxt_ts.c > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > > --- > > .../ABI/testing/sysfs-driver-ge-achc | 14 + > > drivers/misc/Kconfig | 11 + > > drivers/misc/Makefile | 2 + > > drivers/misc/gehc-achc.c | 136 +++++ > > drivers/misc/nxp-ezport.c | 468 ++++++++++++++++++ > > drivers/misc/nxp-ezport.h | 9 + > > Why is there two different modules here for the same piece of hardware? It's only one module, but I put all the generic ezport code into its own file for easy code reuse. I can put it all into gehc-achc and thus reduce the chance of code reuse if somebody else needs to do ezport flashing. Since both files are built into the same module the size savings of merging both files are minimal. > > drivers/spi/spidev.c | 1 - > > 7 files changed, 640 insertions(+), 1 deletion(-) > > create mode 100644 Documentation/ABI/testing/sysfs-driver-ge-achc > > create mode 100644 drivers/misc/gehc-achc.c > > create mode 100644 drivers/misc/nxp-ezport.c > > create mode 100644 drivers/misc/nxp-ezport.h > > > > diff --git a/Documentation/ABI/testing/sysfs-driver-ge-achc b/Documentation/ABI/testing/sysfs-driver-ge-achc > > new file mode 100644 > > index 000000000000..f02812731082 > > --- /dev/null > > +++ b/Documentation/ABI/testing/sysfs-driver-ge-achc > > @@ -0,0 +1,14 @@ > > +What: /sys/bus/spi/<dev>/update_firmware > > +Date: Jul 2021 > > +Contact: sebastian.reichel@collabora.com > > +Description: Write 1 to this file to update the ACHC microcontroller > > + firmware via the EzPort interface. For this the kernel > > + will load "achc.bin" via the firmware API (so usually > > + from /lib/firmware). The write will block until the FW > > + has either been flashed successfully or an error occured. > > + > > +What: /sys/bus/spi/<dev>/reset > > +Date: Jul 2021 > > +Contact: sebastian.reichel@collabora.com > > +Description: Write 1 to this file to reset the microcontroller via the > > + reset GPIO. The write will block until the reset completes. > > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig > > index f4fb5c52b863..1ebf7000671d 100644 > > --- a/drivers/misc/Kconfig > > +++ b/drivers/misc/Kconfig > > @@ -208,6 +208,17 @@ config CS5535_CLOCK_EVENT_SRC > > MFGPTs have a better resolution and max interval than the > > generic PIT, and are suitable for use as high-res timers. > > > > +config GEHC_ACHC > > + tristate "GEHC ACHC support" > > + depends on SPI && SYSFS > > + select FW_LOADER > > + help > > + Support for GE ACHC microcontroller, that is part of the GE > > + PPD device. > > + > > + To compile this driver as a module, choose M here: the > > + module will be called gehc-achc. > > + > > config HP_ILO > > tristate "Channel interface driver for the HP iLO processor" > > depends on PCI > > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile > > index e92a56d4442f..b50180796931 100644 > > --- a/drivers/misc/Makefile > > +++ b/drivers/misc/Makefile > > @@ -24,6 +24,8 @@ obj-$(CONFIG_KGDB_TESTS) += kgdbts.o > > obj-$(CONFIG_SGI_XP) += sgi-xp/ > > obj-$(CONFIG_SGI_GRU) += sgi-gru/ > > obj-$(CONFIG_CS5535_MFGPT) += cs5535-mfgpt.o > > +obj-$(CONFIG_GEHC_ACHC) += gehc-achc-mod.o > > +gehc-achc-mod-objs := gehc-achc.o nxp-ezport.o > > Yeah, you are creating 2 modules for one Kconfig option, why not put > these together into 1? Makes the code smaller. Actually the above builds both C files into objects and then both objects into 1 module. It's a common pattern in some parts of the kernel, e.g. drm. > > obj-$(CONFIG_HP_ILO) += hpilo.o > > obj-$(CONFIG_APDS9802ALS) += apds9802als.o > > obj-$(CONFIG_ISL29003) += isl29003.o > > diff --git a/drivers/misc/gehc-achc.c b/drivers/misc/gehc-achc.c > > new file mode 100644 > > index 000000000000..893940e7ca4e > > --- /dev/null > > +++ b/drivers/misc/gehc-achc.c > > @@ -0,0 +1,136 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * datasheet: https://www.nxp.com/docs/en/data-sheet/K20P144M120SF3.pdf > > + * > > + * Copyright (C) 2018-2021 Collabora > > + * Copyright (C) 2018-2021 GE Healthcare > > + */ > > + > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/spi/spi.h> > > +#include <linux/of.h> > > +#include "nxp-ezport.h" > > + > > +#define ACHC_MAX_FREQ_HZ 300000 > > + > > +struct achc_data { > > + struct spi_device *main; > > + struct spi_device *ezport; > > + struct gpio_desc *reset; > > + > > + struct mutex device_lock; /* avoid concurrent device access */ > > +}; > > + > > +static ssize_t update_firmware_store(struct device *dev, struct device_attribute *attr, > > + const char *buf, size_t count) > > +{ > > + struct achc_data *achc = dev_get_drvdata(dev); > > + int ret; > > + > > + if (count != 1 || buf[0] != '1') > > + return -EINVAL; > > There a core kernel call to see if the data written to a sysfs file is > "true/false" I would recommend, if you stick with this, to use that > instead. I have problems understanding that sentence, it seems to be missing a word. As far as I can tell -EINVAL is usually used when the user supplied data is not formatted correctly for sysfs store routines, e.g. lot's of them are returning -EINVAL like this: rc = kstrtoul(buf, 0, &value); if (rc) return rc; Did I miss something? Thanks, -- Sebastian
On Tue, Jul 13, 2021 at 01:20:16AM +0200, Sebastian Reichel wrote: > Hi Greg, > > On Mon, Jul 12, 2021 at 07:46:05PM +0200, Greg Kroah-Hartman wrote: > > On Mon, Jul 12, 2021 at 05:02:42PM +0200, Sebastian Reichel wrote: > > > General Electric Healthcare's PPD has a secondary processor from > > > NXP's Kinetis K20 series. That device has two SPI chip selects: > > > > > > The main interface's behaviour depends on the loaded firmware > > > and is currently unused. > > > > > > The secondary interface can be used to update the firmware using > > > EzPort protocol. This is implemented by this driver using the > > > kernel's firmware API. It's not done during probe time, since > > > the device has non-volatile memory and flashing lasts almost 3 > > > minutes. > > > > In thinking about this some more, why does it matter? Spin up a > > workqueue when probing and do the firmware loading then. That way you > > do not end up creating yet another custom user/kernel api just to do > > something as trivial as loading the firmware for a device. > > > > And I think the firmware loader even handles async loading, or at least > > it used to, maybe not anymore, it's a complex api, I recommend reading > > the docs... > > Flashing the firmware during boot instead of on-demand is not > a good idea for two reasons: > > 1. This will wear the flash memory of the microcontroller for no > good reason. Why would you boot with this hardware and not want the firmware loaded? > 2. Even if the flashing happens in the background, the microcontroller > will not be usable while it is being programmed. Thus the system > boot takes 3 minutes instead of 30 seconds. Why would the system boot depend on the firmware being loaded? You have to wait sometime, might as well get the first 30 seconds of waiting out of the way while the rest of the system boots :) > Note, that this is not the first device with this kind of problem. > I've seen this before for e.g. atmel touchscreen [0] basically does > the same via (undocumented) mxt_update_fw_store(). > > [0] drivers/input/touchscreen/atmel_mxt_ts.c Yes, but everyone else uses the normal firmware api method of doing this, don't take one odd driver as being the "rule" :) > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > > > --- > > > .../ABI/testing/sysfs-driver-ge-achc | 14 + > > > drivers/misc/Kconfig | 11 + > > > drivers/misc/Makefile | 2 + > > > drivers/misc/gehc-achc.c | 136 +++++ > > > drivers/misc/nxp-ezport.c | 468 ++++++++++++++++++ > > > drivers/misc/nxp-ezport.h | 9 + > > > > Why is there two different modules here for the same piece of hardware? > > It's only one module, but I put all the generic ezport code into its > own file for easy code reuse. I can put it all into gehc-achc and > thus reduce the chance of code reuse if somebody else needs to do > ezport flashing. Since both files are built into the same module > the size savings of merging both files are minimal. Sorry, you are right, I read the code and Makefile wrong. But, from an organizing point of view, having 2 random files in that directory that are not named alike being built together into one module is not obvious at all. For now, keep it to one file. If future users come along that need this method, split the code then, no need to do extra work at this point in time for unknown uses. > > > +static ssize_t update_firmware_store(struct device *dev, struct device_attribute *attr, > > > + const char *buf, size_t count) > > > +{ > > > + struct achc_data *achc = dev_get_drvdata(dev); > > > + int ret; > > > + > > > + if (count != 1 || buf[0] != '1') > > > + return -EINVAL; > > > > There a core kernel call to see if the data written to a sysfs file is > > "true/false" I would recommend, if you stick with this, to use that > > instead. > > I have problems understanding that sentence, it seems to be missing > a word. > > As far as I can tell -EINVAL is usually used when the user supplied > data is not formatted correctly for sysfs store routines, e.g. lot's > of them are returning -EINVAL like this: > > rc = kstrtoul(buf, 0, &value); > if (rc) > return rc; > > Did I miss something? I was trying to say "use kstrtoul()" instead of trying to hand-roll much the same thing. Sorry for not being specific. thanks, greg k-h
Hi, On Tue, Jul 13, 2021 at 07:41:51AM +0200, Greg Kroah-Hartman wrote: > On Tue, Jul 13, 2021 at 01:20:16AM +0200, Sebastian Reichel wrote: > > On Mon, Jul 12, 2021 at 07:46:05PM +0200, Greg Kroah-Hartman wrote: > > > On Mon, Jul 12, 2021 at 05:02:42PM +0200, Sebastian Reichel wrote: > > > > General Electric Healthcare's PPD has a secondary processor from > > > > NXP's Kinetis K20 series. That device has two SPI chip selects: > > > > > > > > The main interface's behaviour depends on the loaded firmware > > > > and is currently unused. > > > > > > > > The secondary interface can be used to update the firmware using > > > > EzPort protocol. This is implemented by this driver using the > > > > kernel's firmware API. It's not done during probe time, since > > > > the device has non-volatile memory and flashing lasts almost 3 > > > > minutes. > > > > > > In thinking about this some more, why does it matter? Spin up a > > > workqueue when probing and do the firmware loading then. That way you > > > do not end up creating yet another custom user/kernel api just to do > > > something as trivial as loading the firmware for a device. > > > > > > And I think the firmware loader even handles async loading, or at least > > > it used to, maybe not anymore, it's a complex api, I recommend reading > > > the docs... > > > > Flashing the firmware during boot instead of on-demand is not > > a good idea for two reasons: > > > > 1. This will wear the flash memory of the microcontroller for no > > good reason. > > Why would you boot with this hardware and not want the firmware > loaded? As written in the commit message this code is updateing the firmware in non-volatile memory, so the previously flashed FW will be used until a new one is flashed. The datasheet for the memory being programmed states, that it has a guaranteed programming endurance of 10.000 times. So programming it at every boot instead of manually when a new FW should be flashed (i.e. once every few years) shortens the device life time considerably. > > 2. Even if the flashing happens in the background, the microcontroller > > will not be usable while it is being programmed. Thus the system > > boot takes 3 minutes instead of 30 seconds. > > Why would the system boot depend on the firmware being loaded? > > You have to wait sometime, might as well get the first 30 seconds of > waiting out of the way while the rest of the system boots :) The microcontroller collects sensor data for the system and is essential for the overall functionality of the medical device. While the firmware flashing is on-going the device cannot be used normally, since the microcontroller will not process and forward the data. > > Note, that this is not the first device with this kind of problem. > > I've seen this before for e.g. atmel touchscreen [0] basically does > > the same via (undocumented) mxt_update_fw_store(). > > > > [0] drivers/input/touchscreen/atmel_mxt_ts.c > > Yes, but everyone else uses the normal firmware api method of doing > this, don't take one odd driver as being the "rule" :) Most FW updates happen to volatile memory (= firmware data is lost on power loss) and must be programmed at boot time. So I'm not looking for the average FW update driver, but for the odd driver also updating FW in non-volatile memory. Those often have the same limitations as the Kinetis K20 has: * limited amount of programming cycles available * slow programming speed > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > > > > --- > > > > .../ABI/testing/sysfs-driver-ge-achc | 14 + > > > > drivers/misc/Kconfig | 11 + > > > > drivers/misc/Makefile | 2 + > > > > drivers/misc/gehc-achc.c | 136 +++++ > > > > drivers/misc/nxp-ezport.c | 468 ++++++++++++++++++ > > > > drivers/misc/nxp-ezport.h | 9 + > > > > > > Why is there two different modules here for the same piece of hardware? > > > > It's only one module, but I put all the generic ezport code into its > > own file for easy code reuse. I can put it all into gehc-achc and > > thus reduce the chance of code reuse if somebody else needs to do > > ezport flashing. Since both files are built into the same module > > the size savings of merging both files are minimal. > > Sorry, you are right, I read the code and Makefile wrong. > > But, from an organizing point of view, having 2 random files in that > directory that are not named alike being built together into one module > is not obvious at all. > > For now, keep it to one file. If future users come along that need this > method, split the code then, no need to do extra work at this point in > time for unknown uses. Ok, I will put it all into gehc-achc. I doubt somebody will find ezport code in there in the future, but that's not my problem :) > > > > +static ssize_t update_firmware_store(struct device *dev, struct device_attribute *attr, > > > > + const char *buf, size_t count) > > > > +{ > > > > + struct achc_data *achc = dev_get_drvdata(dev); > > > > + int ret; > > > > + > > > > + if (count != 1 || buf[0] != '1') > > > > + return -EINVAL; > > > > > > There a core kernel call to see if the data written to a sysfs file is > > > "true/false" I would recommend, if you stick with this, to use that > > > instead. > > > > I have problems understanding that sentence, it seems to be missing > > a word. > > > > As far as I can tell -EINVAL is usually used when the user supplied > > data is not formatted correctly for sysfs store routines, e.g. lot's > > of them are returning -EINVAL like this: > > > > rc = kstrtoul(buf, 0, &value); > > if (rc) > > return rc; > > > > Did I miss something? > > I was trying to say "use kstrtoul()" instead of trying to hand-roll much > the same thing. Sorry for not being specific. Ok. Thanks, -- Sebastian
On Tue, Jul 13, 2021 at 12:46:00PM +0200, Sebastian Reichel wrote: > Hi, > > On Tue, Jul 13, 2021 at 07:41:51AM +0200, Greg Kroah-Hartman wrote: > > On Tue, Jul 13, 2021 at 01:20:16AM +0200, Sebastian Reichel wrote: > > > On Mon, Jul 12, 2021 at 07:46:05PM +0200, Greg Kroah-Hartman wrote: > > > > On Mon, Jul 12, 2021 at 05:02:42PM +0200, Sebastian Reichel wrote: > > > > > General Electric Healthcare's PPD has a secondary processor from > > > > > NXP's Kinetis K20 series. That device has two SPI chip selects: > > > > > > > > > > The main interface's behaviour depends on the loaded firmware > > > > > and is currently unused. > > > > > > > > > > The secondary interface can be used to update the firmware using > > > > > EzPort protocol. This is implemented by this driver using the > > > > > kernel's firmware API. It's not done during probe time, since > > > > > the device has non-volatile memory and flashing lasts almost 3 > > > > > minutes. > > > > > > > > In thinking about this some more, why does it matter? Spin up a > > > > workqueue when probing and do the firmware loading then. That way you > > > > do not end up creating yet another custom user/kernel api just to do > > > > something as trivial as loading the firmware for a device. > > > > > > > > And I think the firmware loader even handles async loading, or at least > > > > it used to, maybe not anymore, it's a complex api, I recommend reading > > > > the docs... > > > > > > Flashing the firmware during boot instead of on-demand is not > > > a good idea for two reasons: > > > > > > 1. This will wear the flash memory of the microcontroller for no > > > good reason. > > > > Why would you boot with this hardware and not want the firmware > > loaded? > > As written in the commit message this code is updateing the firmware > in non-volatile memory, so the previously flashed FW will be used > until a new one is flashed. > > The datasheet for the memory being programmed states, that it has a > guaranteed programming endurance of 10.000 times. So programming it > at every boot instead of manually when a new FW should be flashed > (i.e. once every few years) shortens the device life time considerably. Ah, ok, that was not obvious at all. This is much more like a "BIOS update" in that it is infrequent. You might want to document the heck out of this so others are not confused like me. thanks, greg k-h
diff --git a/Documentation/ABI/testing/sysfs-driver-ge-achc b/Documentation/ABI/testing/sysfs-driver-ge-achc new file mode 100644 index 000000000000..f02812731082 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-driver-ge-achc @@ -0,0 +1,14 @@ +What: /sys/bus/spi/<dev>/update_firmware +Date: Jul 2021 +Contact: sebastian.reichel@collabora.com +Description: Write 1 to this file to update the ACHC microcontroller + firmware via the EzPort interface. For this the kernel + will load "achc.bin" via the firmware API (so usually + from /lib/firmware). The write will block until the FW + has either been flashed successfully or an error occured. + +What: /sys/bus/spi/<dev>/reset +Date: Jul 2021 +Contact: sebastian.reichel@collabora.com +Description: Write 1 to this file to reset the microcontroller via the + reset GPIO. The write will block until the reset completes. diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index f4fb5c52b863..1ebf7000671d 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -208,6 +208,17 @@ config CS5535_CLOCK_EVENT_SRC MFGPTs have a better resolution and max interval than the generic PIT, and are suitable for use as high-res timers. +config GEHC_ACHC + tristate "GEHC ACHC support" + depends on SPI && SYSFS + select FW_LOADER + help + Support for GE ACHC microcontroller, that is part of the GE + PPD device. + + To compile this driver as a module, choose M here: the + module will be called gehc-achc. + config HP_ILO tristate "Channel interface driver for the HP iLO processor" depends on PCI diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index e92a56d4442f..b50180796931 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -24,6 +24,8 @@ obj-$(CONFIG_KGDB_TESTS) += kgdbts.o obj-$(CONFIG_SGI_XP) += sgi-xp/ obj-$(CONFIG_SGI_GRU) += sgi-gru/ obj-$(CONFIG_CS5535_MFGPT) += cs5535-mfgpt.o +obj-$(CONFIG_GEHC_ACHC) += gehc-achc-mod.o +gehc-achc-mod-objs := gehc-achc.o nxp-ezport.o obj-$(CONFIG_HP_ILO) += hpilo.o obj-$(CONFIG_APDS9802ALS) += apds9802als.o obj-$(CONFIG_ISL29003) += isl29003.o diff --git a/drivers/misc/gehc-achc.c b/drivers/misc/gehc-achc.c new file mode 100644 index 000000000000..893940e7ca4e --- /dev/null +++ b/drivers/misc/gehc-achc.c @@ -0,0 +1,136 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * datasheet: https://www.nxp.com/docs/en/data-sheet/K20P144M120SF3.pdf + * + * Copyright (C) 2018-2021 Collabora + * Copyright (C) 2018-2021 GE Healthcare + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/spi/spi.h> +#include <linux/of.h> +#include "nxp-ezport.h" + +#define ACHC_MAX_FREQ_HZ 300000 + +struct achc_data { + struct spi_device *main; + struct spi_device *ezport; + struct gpio_desc *reset; + + struct mutex device_lock; /* avoid concurrent device access */ +}; + +static ssize_t update_firmware_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + struct achc_data *achc = dev_get_drvdata(dev); + int ret; + + if (count != 1 || buf[0] != '1') + return -EINVAL; + + mutex_lock(&achc->device_lock); + ret = ezport_flash(achc->ezport, achc->reset, "achc.bin"); + mutex_unlock(&achc->device_lock); + + if (ret < 0) + return ret; + + return count; +} +static DEVICE_ATTR_WO(update_firmware); + +static ssize_t reset_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + struct achc_data *achc = dev_get_drvdata(dev); + + if (count != 1 || buf[0] != '1') + return -EINVAL; + + mutex_lock(&achc->device_lock); + ezport_reset(achc->reset); + mutex_unlock(&achc->device_lock); + + return count; +} +static DEVICE_ATTR_WO(reset); + +static struct attribute *gehc_achc_attrs[] = { + &dev_attr_update_firmware.attr, + &dev_attr_reset.attr, + NULL, +}; +ATTRIBUTE_GROUPS(gehc_achc); + +static void unregister_ezport(void *data) +{ + struct spi_device *ezport = data; + + spi_unregister_device(ezport); +} + +static int gehc_achc_probe(struct spi_device *spi) +{ + struct achc_data *achc; + int ezport_reg, ret; + + spi->max_speed_hz = ACHC_MAX_FREQ_HZ; + spi->bits_per_word = 8; + spi->mode = SPI_MODE_0; + + achc = devm_kzalloc(&spi->dev, sizeof(*achc), GFP_KERNEL); + if (!achc) + return -ENOMEM; + spi_set_drvdata(spi, achc); + achc->main = spi; + + mutex_init(&achc->device_lock); + + ret = of_property_read_u32_index(spi->dev.of_node, "reg", 1, &ezport_reg); + if (ret) + return dev_err_probe(&spi->dev, ret, "missing second reg entry!\n"); + + achc->ezport = spi_new_ancillary_device(spi, ezport_reg); + if (IS_ERR(achc->ezport)) + return PTR_ERR(achc->ezport); + + ret = devm_add_action_or_reset(&spi->dev, unregister_ezport, achc->ezport); + if (ret) + return ret; + + achc->reset = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_LOW); + if (IS_ERR(achc->reset)) + return dev_err_probe(&spi->dev, PTR_ERR(achc->reset), "Could not get reset gpio\n"); + + return 0; +} + +static const struct spi_device_id gehc_achc_id[] = { + { "ge,achc", 0 }, + { } +}; +MODULE_DEVICE_TABLE(spi, gehc_achc_id); + +static const struct of_device_id gehc_achc_of_match[] = { + { .compatible = "ge,achc" }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, gehc_achc_of_match); + +static struct spi_driver gehc_achc_spi_driver = { + .driver = { + .name = "gehc-achc", + .of_match_table = gehc_achc_of_match, + .dev_groups = gehc_achc_groups, + }, + .probe = gehc_achc_probe, + .id_table = gehc_achc_id, +}; +module_spi_driver(gehc_achc_spi_driver); + +MODULE_DESCRIPTION("GEHC ACHC driver"); +MODULE_AUTHOR("Sebastian Reichel <sebastian.reichel@collabora.com>"); +MODULE_LICENSE("GPL"); diff --git a/drivers/misc/nxp-ezport.c b/drivers/misc/nxp-ezport.c new file mode 100644 index 000000000000..1096af5690c8 --- /dev/null +++ b/drivers/misc/nxp-ezport.c @@ -0,0 +1,468 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * This library implements NXP's EzPort protocol used to + * flash the firmware of Kinetis microcontrollers, such + * as https://www.nxp.com/part/MK20FN1M0VMD12 and is + * intended to be used as ancillary SPI device. + * + * Copyright (C) 2018-2021 Collabora + * Copyright (C) 2018-2021 GE Healthcare + */ + +#include <linux/delay.h> +#include <linux/firmware.h> +#include <linux/kernel.h> +#include <linux/gpio/consumer.h> +#include <linux/spi/spi.h> +#include "nxp-ezport.h" + +#define EZPORT_RESET_DELAY_MS 100 +#define EZPORT_STARTUP_DELAY_MS 200 +#define EZPORT_WRITE_WAIT_MS 10 +#define EZPORT_TRANSFER_SIZE 2048 + +#define EZPORT_CMD_SP 0x02 /* flash section program */ +#define EZPORT_CMD_RDSR 0x05 /* read status register */ +#define EZPORT_CMD_WREN 0x06 /* write enable */ +#define EZPORT_CMD_FAST_READ 0x0b /* flash read data at high speed */ +#define EZPORT_CMD_RESET 0xb9 /* reset chip */ +#define EZPORT_CMD_BE 0xc7 /* bulk erase */ +#define EZPORT_CMD_SE 0xd8 /* sector erase */ + +#define EZPORT_SECTOR_SIZE 4096 +#define EZPORT_SECTOR_MASK (EZPORT_SECTOR_SIZE - 1) + +#define EZPORT_STATUS_WIP BIT(0) /* write in progress */ +#define EZPORT_STATUS_WEN BIT(1) /* write enable */ +#define EZPORT_STATUS_BEDIS BIT(2) /* bulk erase disable */ +#define EZPORT_STATUS_FLEXRAM BIT(3) /* FlexRAM mode */ +#define EZPORT_STATUS_WEF BIT(6) /* write error flag */ +#define EZPORT_STATUS_FS BIT(7) /* flash security */ + +/** + * ezport_reset - reset ezport device + * @reset: the gpio connected to the device reset pin + * + * Context: can sleep + */ +void ezport_reset(struct gpio_desc *reset) +{ + gpiod_set_value(reset, 1); + msleep(EZPORT_RESET_DELAY_MS); + gpiod_set_value(reset, 0); + msleep(EZPORT_STARTUP_DELAY_MS); +} + +static int ezport_start_programming(struct spi_device *spi, struct gpio_desc *reset) +{ + struct spi_message msg; + struct spi_transfer assert_cs = { + .cs_change = 1, + }; + struct spi_transfer release_cs = { }; + int ret; + + spi_bus_lock(spi->master); + + /* assert chip select */ + spi_message_init(&msg); + spi_message_add_tail(&assert_cs, &msg); + ret = spi_sync_locked(spi, &msg); + if (ret) + goto fail; + + msleep(EZPORT_STARTUP_DELAY_MS); + + /* reset with asserted chip select to switch into programming mode */ + ezport_reset(reset); + + /* release chip select */ + spi_message_init(&msg); + spi_message_add_tail(&release_cs, &msg); + ret = spi_sync_locked(spi, &msg); + +fail: + spi_bus_unlock(spi->master); + return ret; +} + +static void ezport_stop_programming(struct spi_device *spi, struct gpio_desc *reset) +{ + /* reset without asserted chip select to return into normal mode */ + spi_bus_lock(spi->master); + ezport_reset(reset); + spi_bus_unlock(spi->master); +} + +static int ezport_get_status_register(struct spi_device *spi) +{ + int ret; + + ret = spi_w8r8(spi, EZPORT_CMD_RDSR); + if (ret < 0) + return ret; + if (ret == 0xff) { + dev_err(&spi->dev, "Invalid EzPort status, EzPort is not functional!\n"); + return -EINVAL; + } + + return ret; +} + +static int ezport_soft_reset(struct spi_device *spi) +{ + u8 cmd = EZPORT_CMD_RESET; + int ret; + + ret = spi_write(spi, &cmd, 1); + if (ret < 0) + return ret; + + msleep(EZPORT_STARTUP_DELAY_MS); + + return 0; +} + +static int ezport_send_simple(struct spi_device *spi, u8 cmd) +{ + int ret; + + ret = spi_write(spi, &cmd, 1); + if (ret < 0) + return ret; + + return ezport_get_status_register(spi); +} + +static int ezport_wait_write(struct spi_device *spi, u32 retries) +{ + int ret; + u32 i; + + for (i = 0; i < retries; i++) { + ret = ezport_get_status_register(spi); + if (ret > 0 && !(ret & EZPORT_STATUS_WIP)) + break; + msleep(EZPORT_WRITE_WAIT_MS); + } + + return ret; +} + +static int ezport_write_enable(struct spi_device *spi) +{ + int ret = 0, retries = 3; + + for (retries = 0; retries < 3; retries++) { + ret = ezport_send_simple(spi, EZPORT_CMD_WREN); + if (ret > 0 && ret & EZPORT_STATUS_WEN) + break; + } + + if (!(ret & EZPORT_STATUS_WEN)) { + dev_err(&spi->dev, "EzPort write enable timed out\n"); + return -ETIMEDOUT; + } + return 0; +} + +static int ezport_bulk_erase(struct spi_device *spi) +{ + int ret; + static const u8 cmd = EZPORT_CMD_BE; + + dev_dbg(&spi->dev, "EzPort bulk erase...\n"); + + ret = ezport_write_enable(spi); + if (ret < 0) + return ret; + + ret = spi_write(spi, &cmd, 1); + if (ret < 0) + return ret; + + ret = ezport_wait_write(spi, 1000); + if (ret < 0) + return ret; + + return 0; +} + +static int ezport_section_erase(struct spi_device *spi, u32 address) +{ + u8 query[] = {EZPORT_CMD_SE, (address >> 16) & 0xff, (address >> 8) & 0xff, address & 0xff}; + int ret; + + dev_dbg(&spi->dev, "Ezport section erase @ 0x%06x...\n", address); + + if (address & EZPORT_SECTOR_MASK) + return -EINVAL; + + ret = ezport_write_enable(spi); + if (ret < 0) + return ret; + + ret = spi_write(spi, query, sizeof(query)); + if (ret < 0) + return ret; + + return ezport_wait_write(spi, 200); +} + +static int ezport_flash_transfer(struct spi_device *spi, u32 address, + const u8 *payload, size_t payload_size) +{ + struct spi_transfer xfers[2] = {}; + u8 *command; + int ret; + + dev_dbg(&spi->dev, "EzPort write %zu bytes @ 0x%06x...\n", payload_size, address); + + command = kmalloc(4, GFP_KERNEL | GFP_DMA); + if (!command) + return -ENOMEM; + + command[0] = EZPORT_CMD_SP; + command[1] = address >> 16; + command[2] = address >> 8; + command[3] = address >> 0; + + xfers[0].tx_buf = command; + xfers[0].len = 4; + + xfers[1].tx_buf = payload; + xfers[1].len = payload_size; + + ret = spi_sync_transfer(spi, xfers, 2); + kfree(command); + if (ret < 0) + return ret; + + return ezport_wait_write(spi, 40); +} + +static int ezport_flash_compare(struct spi_device *spi, u32 address, + const u8 *payload, size_t payload_size) +{ + struct spi_transfer xfers[2] = {}; + u8 *buffer; + int ret; + + buffer = kmalloc(payload_size + 4, GFP_KERNEL | GFP_DMA); + if (!buffer) + return -ENOMEM; + + buffer[0] = EZPORT_CMD_FAST_READ; + buffer[1] = address >> 16; + buffer[2] = address >> 8; + buffer[3] = address >> 0; + + xfers[0].tx_buf = buffer; + xfers[0].len = 4; + + xfers[1].rx_buf = buffer + 4; + xfers[1].len = payload_size; + + ret = spi_sync_transfer(spi, xfers, 2); + if (ret) + goto err; + + ret = memcmp(payload, buffer + 4, payload_size); + if (ret) { + ret = -EBADMSG; + dev_dbg(&spi->dev, "Verfication failure @ %06x", address); + print_hex_dump_bytes("fw: ", DUMP_PREFIX_OFFSET, payload, payload_size); + print_hex_dump_bytes("dev: ", DUMP_PREFIX_OFFSET, buffer + 4, payload_size); + } + +err: + kfree(buffer); + return ret; +} + +static int ezport_firmware_compare_data(struct spi_device *spi, + const u8 *data, size_t size) +{ + int ret; + u32 address = 0; + u32 transfer_size; + + dev_dbg(&spi->dev, "EzPort compare data with %zu bytes...\n", size); + + ret = ezport_get_status_register(spi); + if (ret < 0) + return ret; + + if (ret & EZPORT_STATUS_FS) { + dev_info(&spi->dev, "Device is in secure mode (status=0x%02x)!\n", ret); + dev_info(&spi->dev, "FW verification is not possible\n"); + return -EACCES; + } + + while (size - address > 0) { + transfer_size = min((u32) EZPORT_TRANSFER_SIZE, size - address); + + ret = ezport_flash_compare(spi, address, data+address, transfer_size); + if (ret) + return ret; + + address += transfer_size; + } + + return 0; +} + +static int ezport_firmware_flash_data(struct spi_device *spi, + const u8 *data, size_t size) +{ + int ret; + u32 address = 0; + u32 transfer_size; + + dev_dbg(&spi->dev, "EzPort flash data with %zu bytes...\n", size); + + ret = ezport_get_status_register(spi); + if (ret < 0) + return ret; + + if (ret & EZPORT_STATUS_FS) { + ret = ezport_bulk_erase(spi); + if (ret < 0) + return ret; + if (ret & EZPORT_STATUS_FS) + return -EINVAL; + } + + while (size - address > 0) { + if (!(address & EZPORT_SECTOR_MASK)) { + ret = ezport_section_erase(spi, address); + if (ret < 0) + return ret; + if (ret & EZPORT_STATUS_WIP || ret & EZPORT_STATUS_WEF) + return -EIO; + } + + transfer_size = min((u32) EZPORT_TRANSFER_SIZE, size - address); + + ret = ezport_flash_transfer(spi, address, + data+address, transfer_size); + if (ret < 0) + return ret; + else if (ret & EZPORT_STATUS_WIP) + return -ETIMEDOUT; + else if (ret & EZPORT_STATUS_WEF) + return -EIO; + + address += transfer_size; + } + + ret = ezport_soft_reset(spi); + if (ret < 0) + dev_warn(&spi->dev, "EzPort reset failed!\n"); + + dev_dbg(&spi->dev, "EzPort verify flashed data...\n"); + ret = ezport_firmware_compare_data(spi, data, size); + + /* allow missing FW verfication in secure mode */ + if (ret == -EACCES) + ret = 0; + + if (ret < 0) + dev_err(&spi->dev, "Failed to verify flashed data: %d\n", ret); + + return ret; +} + +static int ezport_firmware_load(struct spi_device *spi, const char *fwname) +{ + const struct firmware *fw; + int ret; + + ret = request_firmware(&fw, fwname, &spi->dev); + if (ret) { + dev_err(&spi->dev, "Could not get firmware: %d\n", ret); + return ret; + } + + ret = ezport_firmware_flash_data(spi, fw->data, fw->size); + + release_firmware(fw); + + return ret; +} + +static int ezport_firmware_verify(struct spi_device *spi, const char *fwname) +{ + const struct firmware *fw; + int ret; + + ret = request_firmware(&fw, fwname, &spi->dev); + if (ret) { + dev_err(&spi->dev, "Could not get firmware: %d\n", ret); + return ret; + } + + ret = ezport_firmware_compare_data(spi, fw->data, fw->size); + + release_firmware(fw); + + return ret; +} + +/** + * ezport_flash - flash device firmware + * @spi: SPI device for NXP EzPort interface + * @reset: the gpio connected to the device reset pin + * @fwname: filename of the firmware that should be flashed + * + * Context: can sleep + * + * Return: 0 on success; negative errno on failure + */ +int ezport_flash(struct spi_device *spi, struct gpio_desc *reset, const char *fwname) +{ + int ret; + + ret = ezport_start_programming(spi, reset); + if (ret) + return ret; + + ret = ezport_firmware_load(spi, fwname); + + ezport_stop_programming(spi, reset); + + if (ret) + dev_err(&spi->dev, "Failed to flash firmware: %d\n", ret); + else + dev_dbg(&spi->dev, "Finished FW flashing!\n"); + + return ret; +} + +/** + * ezport_verify - verify device firmware + * @spi: SPI device for NXP EzPort interface + * @reset: the gpio connected to the device reset pin + * @fwname: filename of the firmware that should be compared + * + * Context: can sleep + * + * Return: 0 on success; negative errno on failure + */ +int ezport_verify(struct spi_device *spi, struct gpio_desc *reset, const char *fwname) +{ + int ret; + + ret = ezport_start_programming(spi, reset); + if (ret) + return ret; + + ret = ezport_firmware_verify(spi, fwname); + + ezport_stop_programming(spi, reset); + + if (ret) + dev_err(&spi->dev, "Failed to verify firmware: %d\n", ret); + + return ret; +} diff --git a/drivers/misc/nxp-ezport.h b/drivers/misc/nxp-ezport.h new file mode 100644 index 000000000000..fc48af82409b --- /dev/null +++ b/drivers/misc/nxp-ezport.h @@ -0,0 +1,9 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +#ifndef __NXP_EZPORT_H +#define __NXP_EZPORT_H + +void ezport_reset(struct gpio_desc *reset); +int ezport_flash(struct spi_device *spi, struct gpio_desc *reset, const char *fwname); +int ezport_verify(struct spi_device *spi, struct gpio_desc *reset, const char *fwname); + +#endif diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c index 24e9469ea35b..6dc29ce3b4bf 100644 --- a/drivers/spi/spidev.c +++ b/drivers/spi/spidev.c @@ -677,7 +677,6 @@ static struct class *spidev_class; static const struct of_device_id spidev_dt_ids[] = { { .compatible = "rohm,dh2228fv" }, { .compatible = "lineartechnology,ltc2488" }, - { .compatible = "ge,achc" }, { .compatible = "semtech,sx1301" }, { .compatible = "lwn,bk4" }, { .compatible = "dh,dhcom-board" },
General Electric Healthcare's PPD has a secondary processor from NXP's Kinetis K20 series. That device has two SPI chip selects: The main interface's behaviour depends on the loaded firmware and is currently unused. The secondary interface can be used to update the firmware using EzPort protocol. This is implemented by this driver using the kernel's firmware API. It's not done during probe time, since the device has non-volatile memory and flashing lasts almost 3 minutes. Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> --- .../ABI/testing/sysfs-driver-ge-achc | 14 + drivers/misc/Kconfig | 11 + drivers/misc/Makefile | 2 + drivers/misc/gehc-achc.c | 136 +++++ drivers/misc/nxp-ezport.c | 468 ++++++++++++++++++ drivers/misc/nxp-ezport.h | 9 + drivers/spi/spidev.c | 1 - 7 files changed, 640 insertions(+), 1 deletion(-) create mode 100644 Documentation/ABI/testing/sysfs-driver-ge-achc create mode 100644 drivers/misc/gehc-achc.c create mode 100644 drivers/misc/nxp-ezport.c create mode 100644 drivers/misc/nxp-ezport.h