| Message ID | 20250313144331.70591-1-francesco@dolcini.it (mailing list archive) |
|---|---|
| Headers | show |
| Series | platform: toradex: Add toradex embedded controller | expand |
On Thu, Mar 13, 2025 at 03:43:29PM +0100, Francesco Dolcini wrote: > From: Francesco Dolcini <francesco.dolcini@toradex.com> > > This series adds support for the Toradex Embedded Controller, currently used > on Toradex SMARC iMX95 and iMX8MP boards, with more to come in the future. How many do you have that will come with like 99% guarantee? > The EC provides board power-off, reset and GPIO expander functionalities. > > Sending it as an RFC to gather initial feedback on it before investing more > time in testing and adding the remaining functionalities, with that said both > the code and the binding are in condition to be wholly reviewed. This doesn't explain why you need a separate folder.
Hi Francesco, On 13-Mar-25 3:43 PM, Francesco Dolcini wrote: > From: Francesco Dolcini <francesco.dolcini@toradex.com> > > This series adds support for the Toradex Embedded Controller, currently used > on Toradex SMARC iMX95 and iMX8MP boards, with more to come in the future. > > The EC provides board power-off, reset and GPIO expander functionalities. > > Sending it as an RFC to gather initial feedback on it before investing more > time in testing and adding the remaining functionalities, with that said both > the code and the binding are in condition to be wholly reviewed. > > Emanuele Ghidoli (2): > dt-bindings: firmware: add toradex,embedded-controller > platform: toradex: add preliminary support for Embedded Controller Thank you for your patches. 2 remarks, as Andy already hinted at drivers/platform/arm64/ likely is a better location for this. But as the commit first adding that directory indicates: https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=363c8aea25728604537b170a1cc24e2f46844896 The reason for having ARM EC drivers there is that these are for x86-pc-like laptops with all the typical laptops bells and whistles like EC handled battery charging limits / spk/mic mute-leds built into keys on the keyboards. Special key handling (like mute, kbd backlight) done by the EC etc. Since all the experience for dealing with those laptop-esque features and exporting them to userspace with a consistent userspace API is in hands of the maintainers of drivers/platform/x86 it was decided to add a new drivers/platform/arm64 directory maintained by the same folks. If this EC driver's only functionality is: "The EC provides board power-off, reset and GPIO expander functionalities." I'm not sure that drivers/platform/arm64 is the best place for this. Also you mention GPIO expander, but that does not seem to be supported yet? The GPIO functionality really should be in its own GPIO driver. So it seems to me that what would be a better fit here would be: 1. A drivers/mfd/ MFD driver with the regmap stuff, registering "board-reset" and "gpio" cells 2. A drivers/power/reset/ driver for the "board-reset" cell, quoting from the Kconfig help text for that dir: menuconfig POWER_RESET bool "Board level reset or power off" help Provides a number of drivers which either reset a complete board or shut it down, by manipulating the main power supply on the board. Say Y here to enable board reset and power off which seems to match exactly with your current EC driver functionality 3. A drivers/gpio/ driver to expose the GPIOs using the standard GPIO framework, this can be added later in a follow up patch. Regards, Hans
On Thu, Mar 13, 2025 at 04:54:59PM +0200, Andy Shevchenko wrote: > On Thu, Mar 13, 2025 at 03:43:29PM +0100, Francesco Dolcini wrote: > > From: Francesco Dolcini <francesco.dolcini@toradex.com> > > > > This series adds support for the Toradex Embedded Controller, currently used > > on Toradex SMARC iMX95 and iMX8MP boards, with more to come in the future. > > How many do you have that will come with like 99% guarantee? None? What I know is that - Toradex is building SoM since 20 years and not planning to stop any time soon - Recently we decided to get into the SMARC market - From an engineering side we decided that it was the best decision to have a small microcontroller to act as an embedded controller, sitting between the SoC and the PMIC, handling reset, power-up/down sequence, strapping options, I/Os and potentially more - In our roadmap we have more SMARC based products planned - The firmware interface is designed to be generic and handle future boards - We expect to use the same driver for any upcoming board using such embedded controller, the EC firmware and the DT will be the only differences. But, I do not have a third product in my hands now, so, I have no such a thing as 99% guarantee. Honestly I have only one thing in my future with such a high probability ;-) > > The EC provides board power-off, reset and GPIO expander functionalities. > > > > Sending it as an RFC to gather initial feedback on it before investing more > > time in testing and adding the remaining functionalities, with that said both > > the code and the binding are in condition to be wholly reviewed. > > This doesn't explain why you need a separate folder. Can you be more specific here? You mean the `toradex` directory, within `drivers/platform`? The only reason is that maybe we'll break the driver in multiple files, but we can as well just get rid of it. We did not think much at it. BTW, the idea to have this driver in such a way was partially inspired by drivers/platform/cznic/, that was merged a few months ago. Francesco
On Mon, Mar 17, 2025 at 10:31:27AM +0100, Francesco Dolcini wrote: > On Thu, Mar 13, 2025 at 04:54:59PM +0200, Andy Shevchenko wrote: > > On Thu, Mar 13, 2025 at 03:43:29PM +0100, Francesco Dolcini wrote: > > > From: Francesco Dolcini <francesco.dolcini@toradex.com> > > > > > > This series adds support for the Toradex Embedded Controller, currently used > > > on Toradex SMARC iMX95 and iMX8MP boards, with more to come in the future. > > > > How many do you have that will come with like 99% guarantee? > > None? What I know is that > > - Toradex is building SoM since 20 years and not planning to stop any > time soon > - Recently we decided to get into the SMARC market > - From an engineering side we decided that it was the best decision to > have a small microcontroller to act as an embedded controller, sitting > between the SoC and the PMIC, handling reset, power-up/down sequence, > strapping options, I/Os and potentially more > - In our roadmap we have more SMARC based products planned > - The firmware interface is designed to be generic and handle future > boards > - We expect to use the same driver for any upcoming board using such > embedded controller, the EC firmware and the DT will be the only > differences. > > But, I do not have a third product in my hands now, so, I have no such a > thing as 99% guarantee. Honestly I have only one thing in my future with > such a high probability ;-) So, perhaps it is not a good start for a brand new folder right now as we might not see any new products. > > > The EC provides board power-off, reset and GPIO expander functionalities. > > > > > > Sending it as an RFC to gather initial feedback on it before investing more > > > time in testing and adding the remaining functionalities, with that said both > > > the code and the binding are in condition to be wholly reviewed. > > > > This doesn't explain why you need a separate folder. > > Can you be more specific here? You mean the `toradex` directory, within > `drivers/platform`? The only reason is that maybe we'll break the driver > in multiple files, but we can as well just get rid of it. We did not > think much at it. Yes, kinda. I mean the folder in par of, e.g., Chrome OS related HW. > BTW, the idea to have this driver in such a way was partially inspired > by drivers/platform/cznic/, that was merged a few months ago. Ah, I see. They have already 3 devices on the market.
Hello Hans, first thanks for the feedback. On Thu, Mar 13, 2025 at 04:08:14PM +0100, Hans de Goede wrote: > On 13-Mar-25 3:43 PM, Francesco Dolcini wrote: > > From: Francesco Dolcini <francesco.dolcini@toradex.com> > > > > This series adds support for the Toradex Embedded Controller, currently used > > on Toradex SMARC iMX95 and iMX8MP boards, with more to come in the future. > > > > The EC provides board power-off, reset and GPIO expander functionalities. > > > > Sending it as an RFC to gather initial feedback on it before investing more > > time in testing and adding the remaining functionalities, with that said both > > the code and the binding are in condition to be wholly reviewed. > > > > Emanuele Ghidoli (2): > > dt-bindings: firmware: add toradex,embedded-controller > > platform: toradex: add preliminary support for Embedded Controller > > Thank you for your patches. > > 2 remarks, as Andy already hinted at drivers/platform/arm64/ likely > is a better location for this. Ack. This driver is not going to be specific of ARM64, but today we have only ARM64 systems that would benefit from it. We might as well use it on a RISCV based SoM in a few years. With that said we'll move it there, we can always move it out if anything changes on this regard. > The reason for having ARM EC drivers there is that these are for > x86-pc-like laptops with all the typical laptops bells and whistles > like EC handled battery charging limits / spk/mic mute-leds built > into keys on the keyboards. Special key handling (like mute, kbd > backlight) done by the EC etc. > > Since all the experience for dealing with those laptop-esque features > and exporting them to userspace with a consistent userspace API is > in hands of the maintainers of drivers/platform/x86 it was decided to > add a new drivers/platform/arm64 directory maintained by the same folks. > > If this EC driver's only functionality is: "The EC provides board > power-off, reset and GPIO expander functionalities." I'm not sure > that drivers/platform/arm64 is the best place for this. The directory decision / architecture was mainly inspired by `drivers/platform/cznic`. This EC is used on a SMARC SoM [1][2], so we are not talking about a laptop nor a device with a keyboard. But we do have a power button, a LID switch, some handling and coordination of low power mode and more. This device is between the SOC, the PMIC, and various IOs used for low-power, power-up/down, boot configuration (selecting the boot device), ... The short term goal is just the 2 basic functionalities mentioned in the cover letter available to the driver: - power/reset (already implemented) - GPIO (working on it as we speak) Starting small, and adding features when/if required. > Also you mention GPIO expander, but that does not seem to be > supported yet? Correct, coming soon. > 1. A drivers/mfd/ MFD driver with the regmap stuff, > registering "board-reset" and "gpio" cells So, we considered the idea of going with an MFD driver, but looking at drivers/platform/cznic, that is doing something relatively close to what we are doing (just more feature rich, as of now), drivers/platform/ seemed a better fit. I am not 100% sure what's Andy opinion on this topic, from what I can understand his concerns are about the toradex directory (that we'll get rid of), not the drivers/platform/ parent you are concerned about. Francesco
On Mon, Mar 17, 2025 at 11:08:56AM +0100, Francesco Dolcini wrote: > On Thu, Mar 13, 2025 at 04:08:14PM +0100, Hans de Goede wrote: > > On 13-Mar-25 3:43 PM, Francesco Dolcini wrote: ... > > 1. A drivers/mfd/ MFD driver with the regmap stuff, > > registering "board-reset" and "gpio" cells > > So, we considered the idea of going with an MFD driver, but looking at > drivers/platform/cznic, that is doing something relatively close to what > we are doing (just more feature rich, as of now), drivers/platform/ > seemed a better fit. > > I am not 100% sure what's Andy opinion on this topic, from what I can > understand his concerns are about the toradex directory (that we'll get > rid of), not the drivers/platform/ parent you are concerned about. Yes, my point is to have this inside existing folder whatever you decide with the maintainers of the respective subsystem to be.
On Mon, Mar 17, 2025 at 12:14:33PM +0200, Andy Shevchenko wrote: > On Mon, Mar 17, 2025 at 11:08:56AM +0100, Francesco Dolcini wrote: > > On Thu, Mar 13, 2025 at 04:08:14PM +0100, Hans de Goede wrote: > > > On 13-Mar-25 3:43 PM, Francesco Dolcini wrote: ... > > > 1. A drivers/mfd/ MFD driver with the regmap stuff, > > > registering "board-reset" and "gpio" cells > > > > So, we considered the idea of going with an MFD driver, but looking at > > drivers/platform/cznic, that is doing something relatively close to what > > we are doing (just more feature rich, as of now), drivers/platform/ > > seemed a better fit. > > > > I am not 100% sure what's Andy opinion on this topic, from what I can > > understand his concerns are about the toradex directory (that we'll get > > rid of), not the drivers/platform/ parent you are concerned about. > > Yes, my point is to have this inside existing folder whatever you decide > with the maintainers of the respective subsystem to be. But if you have more than one module, it might be still good to have at some level, and drivers/platform might be too low for it (too broad). So, either path/to/foo/toradex.c, or path/to/foo/toradex/*.[ch].
Hi, On 17-Mar-25 11:08, Francesco Dolcini wrote: > Hello Hans, > first thanks for the feedback. > > On Thu, Mar 13, 2025 at 04:08:14PM +0100, Hans de Goede wrote: >> On 13-Mar-25 3:43 PM, Francesco Dolcini wrote: >>> From: Francesco Dolcini <francesco.dolcini@toradex.com> >>> >>> This series adds support for the Toradex Embedded Controller, currently used >>> on Toradex SMARC iMX95 and iMX8MP boards, with more to come in the future. >>> >>> The EC provides board power-off, reset and GPIO expander functionalities. >>> >>> Sending it as an RFC to gather initial feedback on it before investing more >>> time in testing and adding the remaining functionalities, with that said both >>> the code and the binding are in condition to be wholly reviewed. >>> >>> Emanuele Ghidoli (2): >>> dt-bindings: firmware: add toradex,embedded-controller >>> platform: toradex: add preliminary support for Embedded Controller >> >> Thank you for your patches. >> >> 2 remarks, as Andy already hinted at drivers/platform/arm64/ likely >> is a better location for this. > > Ack. > > This driver is not going to be specific of ARM64, but today we have only > ARM64 systems that would benefit from it. We might as well use it on a > RISCV based SoM in a few years. > > With that said we'll move it there, we can always move it out if > anything changes on this regard. > >> The reason for having ARM EC drivers there is that these are for >> x86-pc-like laptops with all the typical laptops bells and whistles >> like EC handled battery charging limits / spk/mic mute-leds built >> into keys on the keyboards. Special key handling (like mute, kbd >> backlight) done by the EC etc. >> >> Since all the experience for dealing with those laptop-esque features >> and exporting them to userspace with a consistent userspace API is >> in hands of the maintainers of drivers/platform/x86 it was decided to >> add a new drivers/platform/arm64 directory maintained by the same folks. >> >> If this EC driver's only functionality is: "The EC provides board >> power-off, reset and GPIO expander functionalities." I'm not sure >> that drivers/platform/arm64 is the best place for this. > > The directory decision / architecture was mainly inspired by > `drivers/platform/cznic`. > > This EC is used on a SMARC SoM [1][2], so we are not talking about a > laptop nor a device with a keyboard. > > But we do have a power button, a LID switch, some handling and > coordination of low power mode and more. > This device is between the SOC, the PMIC, and various IOs used for > low-power, power-up/down, boot configuration (selecting the boot > device), ... > > The short term goal is just the 2 basic functionalities mentioned in > the cover letter available to the driver: > - power/reset (already implemented) > - GPIO (working on it as we speak) > > Starting small, and adding features when/if required. > >> Also you mention GPIO expander, but that does not seem to be >> supported yet? > Correct, coming soon. > >> 1. A drivers/mfd/ MFD driver with the regmap stuff, >> registering "board-reset" and "gpio" cells > > So, we considered the idea of going with an MFD driver, but looking at > drivers/platform/cznic, that is doing something relatively close to what > we are doing (just more feature rich, as of now), drivers/platform/ > seemed a better fit. So looking at drivers/platform/cznic this does not look like a good example how to do things. I see multiple sub-drivers each in their own file, but it is all directly linked using symbols from the modules to each other. The cznic code looks like a classic example where a MFD approach creating child devices for each functions and having separate drivers bind to each function would be a much cleaner way to do things, especially since they now also have #ifdef's to allow disabling some functions where as with the MFD approach one can simply e.g. not build the GPIO driver at all. Especially if you plan to re-use the baseboard / EC with possible SOMs of a different architecture, in which case drivers/platform/aarch64 also seems like a bad fit. The example of the MFD approach is that e.g. your GPIO driver can live under drivers/gpio and thus will be reviewed by the GPIO subsystem maintainers benefiting from all their experience with GPIO drivers. But if Andy and Ilpo are happy to take this as a more monolithic driver under drivers/platform/aarch64 I'm not going to nack that approach. Regards, Hans
On Mon, Mar 17, 2025 at 11:39:14AM +0100, Hans de Goede wrote: > On 17-Mar-25 11:08, Francesco Dolcini wrote: ... > But if Andy and Ilpo are happy to take this as a more monolithic > driver under drivers/platform/aarch64 I'm not going to nack that > approach. I'm okay with the choice as long as it's suffice the purpose. I agree that aarch64 maybe not a good choice after all, but we should start from somewhere. Later on we can move to agnostic folder if needed. The question here is more about MFD/not-MFD. If the former becomes the case, it would need to be under drivers/mfd as Lee asked for that (or even required).
Hi, On 17-Mar-25 13:06, Andy Shevchenko wrote: > On Mon, Mar 17, 2025 at 11:39:14AM +0100, Hans de Goede wrote: >> On 17-Mar-25 11:08, Francesco Dolcini wrote: > > ... > >> But if Andy and Ilpo are happy to take this as a more monolithic >> driver under drivers/platform/aarch64 I'm not going to nack that >> approach. > > I'm okay with the choice as long as it's suffice the purpose. > I agree that aarch64 maybe not a good choice after all, but > we should start from somewhere. Later on we can move to agnostic > folder if needed. The question here is more about MFD/not-MFD. > If the former becomes the case, it would need to be under drivers/mfd > as Lee asked for that (or even required). Right if we go with MFD (which IMHO would be the cleaner approach) then the base driver registering the regmap + cells / child devices would live under drivers/mfd, there reboot/shutdown driver under drivers/power/reset/ , gpio under drivers/gpio and any new functionalities in the correct places for those functionalities. Regards, Hans
From: Francesco Dolcini <francesco.dolcini@toradex.com> This series adds support for the Toradex Embedded Controller, currently used on Toradex SMARC iMX95 and iMX8MP boards, with more to come in the future. The EC provides board power-off, reset and GPIO expander functionalities. Sending it as an RFC to gather initial feedback on it before investing more time in testing and adding the remaining functionalities, with that said both the code and the binding are in condition to be wholly reviewed. Emanuele Ghidoli (2): dt-bindings: firmware: add toradex,embedded-controller platform: toradex: add preliminary support for Embedded Controller .../firmware/toradex,embedded-controller.yaml | 44 +++++ MAINTAINERS | 7 + drivers/platform/Kconfig | 2 + drivers/platform/Makefile | 1 + drivers/platform/toradex/Kconfig | 18 ++ drivers/platform/toradex/Makefile | 1 + drivers/platform/toradex/toradex-ec.c | 155 ++++++++++++++++++ 7 files changed, 228 insertions(+) create mode 100644 Documentation/devicetree/bindings/firmware/toradex,embedded-controller.yaml create mode 100644 drivers/platform/toradex/Kconfig create mode 100644 drivers/platform/toradex/Makefile create mode 100644 drivers/platform/toradex/toradex-ec.c