| Message ID | 20241126134529.936451-3-va@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add spidev nodes for SPI controllers | expand |
On Wed, Nov 27, 2024 at 03:54:52PM +0000, Jon Hunter wrote: > On the Tegra Jetson boards we have a 40-pin expansion header similar to what > is found on boards like Raspberry Pi and allows users to connect various > cards to. By having a pseudo device we can interact with different SPI > devices. > Yes by default nothing is connected and so there is nothing to talk to. > However, this does enable us to do SPI loopback testing for example. > So I am wondering if it would be acceptable to having some generic dummy > device-tree compatible string for this? I guess it does not need to be Tegra > specific. I understand what he's trying to accomplish, it's the same thing as what everyone who wants to put a raw spidev compatible in their DT is trying to do. The way to do this would be something like a DT overlay that describes whatever is actually connected, or just customise the DT locally.
On Wed, Nov 27, 2024 at 05:24:01PM +0000, Jon Hunter wrote: > On 27/11/2024 16:09, Mark Brown wrote: > > I understand what he's trying to accomplish, it's the same thing as > > what everyone who wants to put a raw spidev compatible in their DT is > > trying to do. The way to do this would be something like a DT overlay > > that describes whatever is actually connected, or just customise the DT > > locally. > We could certainly use an overlay, but how do we handle the kernel side? My > understanding is that per patch 3/3 we need to reference a compatible string > the kernel is aware of. I guess we could use an existing one, but feels like > a massive hack. It would be nice if there is something generic we can use > for this like 'linux,spidev'. > I see that ACPI has something and it does print a warning that this should > not be used in production systems. You can put 'spidev' in as the compatible and get the warning, we don't require specific compatibles if the Linux device ID is good enough. If you genuinely just have bare wires you're probably able to cope with the warning. If something is actually connected you should use the compatible for whatever that is, if spidev makes sense for it then that'd be OK to add to spidev.
Hi Mark, On 27/11/2024 17:31, Mark Brown wrote: > On Wed, Nov 27, 2024 at 05:24:01PM +0000, Jon Hunter wrote: >> On 27/11/2024 16:09, Mark Brown wrote: > >>> I understand what he's trying to accomplish, it's the same thing as >>> what everyone who wants to put a raw spidev compatible in their DT is >>> trying to do. The way to do this would be something like a DT overlay >>> that describes whatever is actually connected, or just customise the DT >>> locally. > >> We could certainly use an overlay, but how do we handle the kernel side? My >> understanding is that per patch 3/3 we need to reference a compatible string >> the kernel is aware of. I guess we could use an existing one, but feels like >> a massive hack. It would be nice if there is something generic we can use >> for this like 'linux,spidev'. > >> I see that ACPI has something and it does print a warning that this should >> not be used in production systems. > > You can put 'spidev' in as the compatible and get the warning, we don't > require specific compatibles if the Linux device ID is good enough. If > you genuinely just have bare wires you're probably able to cope with the > warning. If something is actually connected you should use the > compatible for whatever that is, if spidev makes sense for it then > that'd be OK to add to spidev. We finally got back to this. Looks like just having 'spidev' as the compatible does not work. Apparently, it use to work and yes you would get the warning, but that no longer seems to be the case. I see a few others have been doing similar things and hacking their device-trees in different ways [0]. I completely agree that ideally we would have a proper compatible string for this because after all device-tree describes the hardware. One use-case that we use is external loop back for verifying SPI by simply connecting MOSI to the MISO. Would it be acceptable to have a compatible string for external loopback connections? Thanks Jon [0] https://stackoverflow.com/questions/53634892/linux-spidev-why-it-shouldnt-be-directly-in-devicetree
On Tue, Mar 25, 2025 at 10:36:29AM +0000, Jon Hunter wrote: > On 27/11/2024 17:31, Mark Brown wrote: > > You can put 'spidev' in as the compatible and get the warning, we don't > > require specific compatibles if the Linux device ID is good enough. If > > you genuinely just have bare wires you're probably able to cope with the > > warning. If something is actually connected you should use the > > compatible for whatever that is, if spidev makes sense for it then > > that'd be OK to add to spidev. > We finally got back to this. Looks like just having 'spidev' as the > compatible does not work. Apparently, it use to work and yes you would get > the warning, but that no longer seems to be the case. I see a few others > have been doing similar things and hacking their device-trees in different > ways [0]. Huh, OK. I don't recall any deliberate SPI change for that. > I completely agree that ideally we would have a proper compatible string for > this because after all device-tree describes the hardware. One use-case that > we use is external loop back for verifying SPI by simply connecting MOSI to > the MISO. Would it be acceptable to have a compatible string for external > loopback connections? That sounds fine.
On Tue, Mar 25, 2025 at 12:10:19PM +0000, Mark Brown wrote: > On Tue, Mar 25, 2025 at 10:36:29AM +0000, Jon Hunter wrote: > > On 27/11/2024 17:31, Mark Brown wrote: > > > > You can put 'spidev' in as the compatible and get the warning, we don't > > > require specific compatibles if the Linux device ID is good enough. If > > > you genuinely just have bare wires you're probably able to cope with the > > > warning. If something is actually connected you should use the > > > compatible for whatever that is, if spidev makes sense for it then > > > that'd be OK to add to spidev. > > > We finally got back to this. Looks like just having 'spidev' as the > > compatible does not work. Apparently, it use to work and yes you would get > > the warning, but that no longer seems to be the case. I see a few others > > have been doing similar things and hacking their device-trees in different > > ways [0]. > > Huh, OK. I don't recall any deliberate SPI change for that. People in the discussion that Jon linked to indicated that it was this patch that caused the "regression": commit 6840615f85f6046039ebc4989870ddb12892b7fc Author: Mark Brown <broonie@kernel.org> Date: Thu Sep 23 18:00:23 2021 +0100 spi: spidev: Add SPI ID table Currently autoloading for SPI devices does not use the DT ID table, it uses SPI modalises. Supporting OF modalises is going to be difficult if not impractical, an attempt was made but has been reverted, so ensure that module autoloading works for this driver by adding an id_table listing the SPI IDs for everything. Fixes: 96c8395e2166 ("spi: Revert modalias changes") Signed-off-by: Mark Brown <broonie@kernel.org> Link: https://lore.kernel.org/r/20210923170023.1683-1-broonie@kernel.org Signed-off-by: Mark Brown <broonie@kernel.org> If you say that the regression wasn't deliberate, maybe we should look at fixing this so that people don't have to work around stuff? Thierry
On Tue, Mar 25, 2025 at 01:45:26PM +0100, Thierry Reding wrote: > On Tue, Mar 25, 2025 at 12:10:19PM +0000, Mark Brown wrote: > > On Tue, Mar 25, 2025 at 10:36:29AM +0000, Jon Hunter wrote: > > > On 27/11/2024 17:31, Mark Brown wrote: > > > > > > You can put 'spidev' in as the compatible and get the warning, we don't > > > > require specific compatibles if the Linux device ID is good enough. If > > > > you genuinely just have bare wires you're probably able to cope with the > > > > warning. If something is actually connected you should use the > > > > compatible for whatever that is, if spidev makes sense for it then > > > > that'd be OK to add to spidev. > > > > > We finally got back to this. Looks like just having 'spidev' as the > > > compatible does not work. Apparently, it use to work and yes you would get > > > the warning, but that no longer seems to be the case. I see a few others > > > have been doing similar things and hacking their device-trees in different > > > ways [0]. > > > > Huh, OK. I don't recall any deliberate SPI change for that. > > People in the discussion that Jon linked to indicated that it was this > patch that caused the "regression": > > commit 6840615f85f6046039ebc4989870ddb12892b7fc > Author: Mark Brown <broonie@kernel.org> > Date: Thu Sep 23 18:00:23 2021 +0100 > spi: spidev: Add SPI ID table > > Currently autoloading for SPI devices does not use the DT ID table, it uses > SPI modalises. Supporting OF modalises is going to be difficult if not > impractical, an attempt was made but has been reverted, so ensure that > module autoloading works for this driver by adding an id_table listing the > SPI IDs for everything. > > Fixes: 96c8395e2166 ("spi: Revert modalias changes") > Signed-off-by: Mark Brown <broonie@kernel.org> > Link: https://lore.kernel.org/r/20210923170023.1683-1-broonie@kernel.org > Signed-off-by: Mark Brown <broonie@kernel.org> > > If you say that the regression wasn't deliberate, maybe we should look > at fixing this so that people don't have to work around stuff? Hm... there's also this: commit f6f6a6320eeeb3e80e1393f727f898f8ca976bfd Author: Javier Martinez Canillas <javierm@redhat.com> Date: Fri Nov 19 13:11:39 2021 +0100 spi: docs: improve the SPI userspace API documentation This doc is fairly outdated and only uses legacy device instantiation terminology. Let us update it and also mention the OF and ACPI device tables, to make easier for users to figure out how should be defined. Also, mention that devices bind could be done in user-space now using the "driver_override" sysfs entry. Suggested-by: Ralph Siemsen <ralph.siemsen@linaro.org> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Link: https://lore.kernel.org/r/20211119121139.2412761-1-javierm@redhat.com Signed-off-by: Mark Brown <broonie@kernel.org> That explicitly says that the linux,spidev or spidev compatible strings are no longer supported, but also mentions the sysfs interface to bind spidev to a given device. Jon, I take it that the current sysfs is not enough for our use-case because it only works if a device is predefined (i.e. the sysfs doesn't allow specifying the chipselect)? Maybe we could enhance sysfs to allow adding a generic spidev client, similar to how other busses (USB, PCI, I think) allow using configfs to create these devices at runtime. I2C has a different mechanism that could perhaps be used to achieve something similar, except that it exposes each controller as a device with a set of defined IOCTLs. That would be another option, but given that there's already a spidev interface that doesn't seem like a good fit for SPI. Mark, would another alternative be to add something like a sysfs export attribute? Something that you'd write a controller/chipselect pair to in order to create a spidev device? That has the benefit of removing this entirely from device tree where it doesn't belong, but still makes this option available to users that would otherwise have to resort to hacks. Thierry
On Tue, Mar 25, 2025 at 01:45:26PM +0100, Thierry Reding wrote: > On Tue, Mar 25, 2025 at 12:10:19PM +0000, Mark Brown wrote: > > On Tue, Mar 25, 2025 at 10:36:29AM +0000, Jon Hunter wrote: > > > We finally got back to this. Looks like just having 'spidev' as the > > > compatible does not work. Apparently, it use to work and yes you would get > > > the warning, but that no longer seems to be the case. I see a few others > > > have been doing similar things and hacking their device-trees in different > > > ways [0]. > > Huh, OK. I don't recall any deliberate SPI change for that. > People in the discussion that Jon linked to indicated that it was this > patch that caused the "regression": ... > If you say that the regression wasn't deliberate, maybe we should look > at fixing this so that people don't have to work around stuff? I mean, we don't want people to be doing this which is why it generates the warning - it's more tolerated than supported. I don't super mind keeping it so long as it's not getting in the way of all the fragility with the enumeration stuff.
On Tue, Mar 25, 2025 at 02:05:01PM +0100, Thierry Reding wrote: > Mark, would another alternative be to add something like a sysfs export > attribute? Something that you'd write a controller/chipselect pair to in > order to create a spidev device? That has the benefit of removing this > entirely from device tree where it doesn't belong, but still makes this > option available to users that would otherwise have to resort to hacks. Possibly? I think I've lost track of what the use case is here, usually for the spidev stuff DT overlays seem like they're the right thing but perhaps this is different? If we are doing this at runtime sysfs seems like a reasonable way to trigger it, though you'd still need the DT to describe the controller and the chipselects that are available.
On Tue, Mar 25, 2025 at 03:55:02PM +0000, Mark Brown wrote: > On Tue, Mar 25, 2025 at 02:05:01PM +0100, Thierry Reding wrote: > > > Mark, would another alternative be to add something like a sysfs export > > attribute? Something that you'd write a controller/chipselect pair to in > > order to create a spidev device? That has the benefit of removing this > > entirely from device tree where it doesn't belong, but still makes this > > option available to users that would otherwise have to resort to hacks. > > Possibly? I think I've lost track of what the use case is here, usually > for the spidev stuff DT overlays seem like they're the right thing but > perhaps this is different? If we are doing this at runtime sysfs seems > like a reasonable way to trigger it, though you'd still need the DT to > describe the controller and the chipselects that are available. Heh... it's exactly the opposite for me. I feel like I don't understand the need for spidev with a specific compatible string. If you've got a compatible string (or in other words you have a device with a very specific SPI chip connected), then why would you want to access it from userspace? Isn't a proper kernel driver the better option in most cases? That usually allows for better abstraction via some other subsystem. I suppose there are cases where there may not be a subsystem, or for other reasons it's more convenient to have direct access from userspace to avoid the roundtrip. Or maybe users could be concerned about safety? I think the more common use-case is for things like these semi-standard expansion headers you see on embedded devices that expose things like GPIOs, I2C and SPI pins. In some cases users might end up installing off-the-shelf expansion boards and they will be happy to add device tree overlays to add whatever is available on that board. Again, for these I suspect a kernel driver matching on that compatible string is more appropriate. In other cases users may just want to connect something completely custom or just have a way to poke whatever they connect. There may not be a proper driver for it. Or it could perhaps even be used temporarily as a way to write a userspace driver conveniently before porting it to Linux. The way I imagine it, exporting would involve writing a chip-select to a specific SPI controller's "export" sysfs attribute to have a SPI device created for that particular chip-select and bind it to spidev. Thierry
On Tue, Mar 25, 2025 at 05:38:57PM +0100, Thierry Reding wrote: > On Tue, Mar 25, 2025 at 03:55:02PM +0000, Mark Brown wrote: > > Possibly? I think I've lost track of what the use case is here, usually > > for the spidev stuff DT overlays seem like they're the right thing but > > perhaps this is different? If we are doing this at runtime sysfs seems > > like a reasonable way to trigger it, though you'd still need the DT to > > describe the controller and the chipselects that are available. > Heh... it's exactly the opposite for me. I feel like I don't understand > the need for spidev with a specific compatible string. If you've got a > compatible string (or in other words you have a device with a very > specific SPI chip connected), then why would you want to access it from > userspace? Isn't a proper kernel driver the better option in most cases? > That usually allows for better abstraction via some other subsystem. I > suppose there are cases where there may not be a subsystem, or for other > reasons it's more convenient to have direct access from userspace to > avoid the roundtrip. Or maybe users could be concerned about safety? A lot of things would be happier with a driver, yes. One of the use cases that did make sense to me longer term was DSP/coprocessor type things with flexible functions where distributing the firmware and application that talks to it together makes sense. > In other cases users may just want to connect something completely > custom or just have a way to poke whatever they connect. There may not > be a proper driver for it. Or it could perhaps even be used temporarily > as a way to write a userspace driver conveniently before porting it to > Linux. > The way I imagine it, exporting would involve writing a chip-select to a > specific SPI controller's "export" sysfs attribute to have a SPI device > created for that particular chip-select and bind it to spidev. My general feeling with those is that if you're building for them you're probably either already modifiying your kernel or easily able to cope with doing so.
On 25/03/2025 17:05, Mark Brown wrote: ... >> The way I imagine it, exporting would involve writing a chip-select to a >> specific SPI controller's "export" sysfs attribute to have a SPI device >> created for that particular chip-select and bind it to spidev. > > My general feeling with those is that if you're building for them you're > probably either already modifiying your kernel or easily able to cope > with doing so. That's definitely what we do today, modify the kernel directly to achieve what we need. I am trying to avoid carrying too many out of tree patches for stuff like this and have something in the kernel that works by default. This is even more important for 3rd party Linux distros that will not accept non-upstream code. Our devkits, very much like Raspberry PI, allow users to connect various hardware for development and so having an easy way to connect a SPI device is useful. For any production systems, users will definitely want a proper device and device-tree bindings. So I am just trying to explore what would be acceptable. If it is acceptable to have a sysfs interface for creating a SPI device at runtime, then we can look into that. Cheers! Jon
On Wed, Mar 26, 2025 at 12:16:53PM +0000, Jon Hunter wrote: > On 25/03/2025 17:05, Mark Brown wrote: > > > The way I imagine it, exporting would involve writing a chip-select to a > > > specific SPI controller's "export" sysfs attribute to have a SPI device > > > created for that particular chip-select and bind it to spidev. > > My general feeling with those is that if you're building for them you're > > probably either already modifiying your kernel or easily able to cope > > with doing so. > That's definitely what we do today, modify the kernel directly to achieve > what we need. I am trying to avoid carrying too many out of tree patches for > stuff like this and have something in the kernel that works by default. This > is even more important for 3rd party Linux distros that will not accept > non-upstream code. Overlays should work well for that case too! > Our devkits, very much like Raspberry PI, allow users to connect various > hardware for development and so having an easy way to connect a SPI device > is useful. For any production systems, users will definitely want a proper > device and device-tree bindings. So I am just trying to explore what would > be acceptable. If it is acceptable to have a sysfs interface for creating a > SPI device at runtime, then we can look into that. The main issue I see with the sysfs thing is that you have to describe the presence of the device somehow which currently needs a device of some kind there, it's not like I2C where you can just use the address.
On 27/03/2025 15:33, Mark Brown wrote: > On Wed, Mar 26, 2025 at 12:16:53PM +0000, Jon Hunter wrote: >> On 25/03/2025 17:05, Mark Brown wrote: > >>>> The way I imagine it, exporting would involve writing a chip-select to a >>>> specific SPI controller's "export" sysfs attribute to have a SPI device >>>> created for that particular chip-select and bind it to spidev. > >>> My general feeling with those is that if you're building for them you're >>> probably either already modifiying your kernel or easily able to cope >>> with doing so. > >> That's definitely what we do today, modify the kernel directly to achieve >> what we need. I am trying to avoid carrying too many out of tree patches for >> stuff like this and have something in the kernel that works by default. This >> is even more important for 3rd party Linux distros that will not accept >> non-upstream code. > > Overlays should work well for that case too! If you mean device-tree overlays, I don't see how that will work today. Unless we are to use one of the existing compatible strings, but that feels wrong because we are not using any of those devices and like I mentioned, just using 'spidev' alone in device-tree does not work with the latest kernels. Jon
On Mon, Mar 31, 2025 at 01:34:21PM +0100, Jon Hunter wrote: > On 27/03/2025 15:33, Mark Brown wrote: > > On Wed, Mar 26, 2025 at 12:16:53PM +0000, Jon Hunter wrote: > > > That's definitely what we do today, modify the kernel directly to achieve > > > what we need. I am trying to avoid carrying too many out of tree patches for > > > stuff like this and have something in the kernel that works by default. This > > > is even more important for 3rd party Linux distros that will not accept > > > non-upstream code. > > Overlays should work well for that case too! > If you mean device-tree overlays, I don't see how that will work today. > Unless we are to use one of the existing compatible strings, but that feels > wrong because we are not using any of those devices and like I mentioned, > just using 'spidev' alone in device-tree does not work with the latest > kernels. Why would you need to use a compatible string for anything? I'd expect the overlay to add the entire new device, compatible and all.
On 31/03/2025 13:44, Mark Brown wrote: > On Mon, Mar 31, 2025 at 01:34:21PM +0100, Jon Hunter wrote: >> On 27/03/2025 15:33, Mark Brown wrote: >>> On Wed, Mar 26, 2025 at 12:16:53PM +0000, Jon Hunter wrote: > >>>> That's definitely what we do today, modify the kernel directly to achieve >>>> what we need. I am trying to avoid carrying too many out of tree patches for >>>> stuff like this and have something in the kernel that works by default. This >>>> is even more important for 3rd party Linux distros that will not accept >>>> non-upstream code. > >>> Overlays should work well for that case too! > >> If you mean device-tree overlays, I don't see how that will work today. >> Unless we are to use one of the existing compatible strings, but that feels >> wrong because we are not using any of those devices and like I mentioned, >> just using 'spidev' alone in device-tree does not work with the latest >> kernels. > > Why would you need to use a compatible string for anything? I'd expect > the overlay to add the entire new device, compatible and all. We need a compatible string for the SPI device in device-tree. Sorry, but I am not sure how we can add a SPI device without one. Can you confirm what you mean by 'overlay'? To me an overlay, is purely a device-tree overlay and even if we have a device-tree overlay, we are still missing the kernel driver part that matches the compatible string. I could be completely missing your point here, but it is not obvious to me what you are suggesting? Jon
On Mon, Mar 31, 2025 at 02:11:35PM +0100, Jon Hunter wrote: > On 31/03/2025 13:44, Mark Brown wrote: > > Why would you need to use a compatible string for anything? I'd expect > > the overlay to add the entire new device, compatible and all. > We need a compatible string for the SPI device in device-tree. Sorry, but I > am not sure how we can add a SPI device without one. > Can you confirm what you mean by 'overlay'? To me an overlay, is purely a > device-tree overlay and even if we have a device-tree overlay, we are still > missing the kernel driver part that matches the compatible string. I would expect the overlay to be a device tree overlay and to add the entire definition of the device, not just patch the compatible in an existing half constructed device. I would expect the driver portion of supporting the device to happen as normal, if spidev makes sense for whatever devices these are then then that could be upstreamed without adding them to any board.
diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml index 88abb5c174f3..c8b39a513fc5 100644 --- a/Documentation/devicetree/bindings/trivial-devices.yaml +++ b/Documentation/devicetree/bindings/trivial-devices.yaml @@ -293,6 +293,8 @@ properties: - national,lm85 # I2C ±0.33°C Accurate, 12-Bit + Sign Temperature Sensor and Thermal Window Comparator - national,lm92 + # NVIDIA Tegra SPIDEV Controller device + - nvidia,tegra-spidev # Nuvoton Temperature Sensor - nuvoton,w83773g # OKI ML86V7667 video decoder
Add DeviceTree schema for NVIDIA'S SPIDEV controller. The schema supports both master and slave modes for data transfer purposes. Specifies required properties: "compatible", "reg", and "spi-max-frequency". Signed-off-by: Vishwaroop A <va@nvidia.com> --- Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++ 1 file changed, 2 insertions(+)