| Message ID | 1406298233-27876-2-git-send-email-pawel.moll@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 7/25/2014 10:23 AM, Pawel Moll wrote: > The code was creating "srom" class devices using > platform_bus as a parent. As they are not really > platform devices, make them virtual, using NULL instead. > > Cc: Chris Metcalf<cmetcalf@tilera.com> > Signed-off-by: Pawel Moll<pawel.moll@arm.com> > --- > drivers/char/tile-srom.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Can you clarify the point of this change a bit? The SROM devices in question are real devices (bits of silicon on the processor die), not some kind of virtual construct. In addition, we also have user binaries in the wild that know to look for /sys/devices/platform/srom/ paths, so I'm pretty reluctant to change this path without good reason.
On Thu, Jul 31, 2014 at 04:24:37PM -0400, Chris Metcalf wrote: > On 7/25/2014 10:23 AM, Pawel Moll wrote: > >The code was creating "srom" class devices using > >platform_bus as a parent. As they are not really > >platform devices, make them virtual, using NULL instead. > > > >Cc: Chris Metcalf<cmetcalf@tilera.com> > >Signed-off-by: Pawel Moll<pawel.moll@arm.com> > >--- > > drivers/char/tile-srom.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > Can you clarify the point of this change a bit? The SROM devices > in question are real devices (bits of silicon on the processor die), not > some kind of virtual construct. Then tie them to the "real" parent device that they live on, don't try to hang them under the platform bus where they don't belong. > In addition, we also have user binaries in the wild that know to look > for /sys/devices/platform/srom/ paths, That's never a good idea, you should be iterating over your bus's devices, to find your devices, not at a specific location within the /sys/devices/ tree, as that is guaranteed to move around over time. It's also why we have those symlinks and lists of devices in your bus directory. > so I'm pretty reluctant to change this path without good reason. Because srom is not a platform device, so why would you put it at the root of the platform device "tree"? thanks, greg kh
On Thu, 2014-07-31 at 21:24 +0100, Chris Metcalf wrote: > On 7/25/2014 10:23 AM, Pawel Moll wrote: > > The code was creating "srom" class devices using > > platform_bus as a parent. As they are not really > > platform devices, make them virtual, using NULL instead. > > > > Cc: Chris Metcalf<cmetcalf@tilera.com> > > Signed-off-by: Pawel Moll<pawel.moll@arm.com> > > --- > > drivers/char/tile-srom.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > Can you clarify the point of this change a bit? Theoretically speaking there shouldn't be any need to export the platform bus root, as all devices should be registered via the platform API (platform_device_register & co.) > The SROM devices > in question are real devices (bits of silicon on the processor die), not > some kind of virtual construct. ... but the driver seems to be accessing then through hypervisor calls only? One could say that you this make them virtual ;-) > In addition, we also have user binaries > in the wild that know to look for /sys/devices/platform/srom/ paths, > so I'm pretty reluctant to change this path without good reason. So what is the srom class for then if not for device discovery? And why do they look for them in the first place? To get relevant character device's data, if I understand it right? Maybe you could just register a simple "proper" platform device for all the sroms and then hang the class devices from it? I can type some code doing this if it sound reasonably? Pawel
On 8/1/2014 1:21 PM, Pawel Moll wrote: > On Thu, 2014-07-31 at 21:24 +0100, Chris Metcalf wrote: >> On 7/25/2014 10:23 AM, Pawel Moll wrote: >>> The code was creating "srom" class devices using >>> platform_bus as a parent. As they are not really >>> platform devices, make them virtual, using NULL instead. >>> >>> Cc: Chris Metcalf<cmetcalf@tilera.com> >>> Signed-off-by: Pawel Moll<pawel.moll@arm.com> >>> --- >>> drivers/char/tile-srom.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >> Can you clarify the point of this change a bit? > Theoretically speaking there shouldn't be any need to export the > platform bus root, as all devices should be registered via the platform > API (platform_device_register & co.) So, perhaps the right fix is to just use platform_device_register() etc for this device, rather than making it virtual? I think what I'm missing is why the platform bus isn't the right bus for this device. The driver-model/platform.txt doc says it's "used to integrate peripherals on many system-on-chip processors", which is how it's being used here. It's directly addressable via MMIO from the cores. I grant you there's some confusion about our use of hypervisor calls here, but effectively this is just a consequence of our use of the Tilera hypervisor for kernel isolation; it's more like invoking the BIOS on an Intel box, than it is about modern virtualization. The current tilegx series hardware always contains this device, so there's no FDT-like model for discovering it dynamically; we just always enable it on tilegx hardware. >> In addition, we also have user binaries >> in the wild that know to look for /sys/devices/platform/srom/ paths, >> so I'm pretty reluctant to change this path without good reason. > So what is the srom class for then if not for device discovery? And why > do they look for them in the first place? To get relevant character > device's data, if I understand it right? > > Maybe you could just register a simple "proper" platform device for all > the sroms and then hang the class devices from it? I can type some code > doing this if it sound reasonably? I'm not sure exactly what you mean by device discovery here. The subdirectories under /sys/devices/platform/srom/ correspond to partitions in the SPI-ROM, which are software constructs created by the Tilera hypervisor. By default we have three, where the first holds boot data that the chip can use to boot out of hardware, and the other two are smaller partitions for boot- and user-specific data. We use the /sys files primarily to get the page size and sector size for the sroms, and also export other interesting information like the total size of the particular srom device. Thank you for volunteering to write a bit of code; if that's the best way to clarify this for us, fantastic, or else pointing us at existing good practices or documentation would be great too.
On Tue, Aug 05, 2014 at 04:08:40PM -0400, Chris Metcalf wrote: > On 8/1/2014 1:21 PM, Pawel Moll wrote: > >On Thu, 2014-07-31 at 21:24 +0100, Chris Metcalf wrote: > >>On 7/25/2014 10:23 AM, Pawel Moll wrote: > >>>The code was creating "srom" class devices using > >>>platform_bus as a parent. As they are not really > >>>platform devices, make them virtual, using NULL instead. > >>> > >>>Cc: Chris Metcalf<cmetcalf@tilera.com> > >>>Signed-off-by: Pawel Moll<pawel.moll@arm.com> > >>>--- > >>> drivers/char/tile-srom.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>Can you clarify the point of this change a bit? > >Theoretically speaking there shouldn't be any need to export the > >platform bus root, as all devices should be registered via the platform > >API (platform_device_register & co.) > > So, perhaps the right fix is to just use platform_device_register() > etc for this device, rather than making it virtual? Sure, that's fine with me if you want to make it a platform device, but note that a platform device doesn't have a minor associated with it, you will have to still have your existing srom_class and the rest. Right now you aren't creating any real "devices" in the kernel, other than the one you use for your minor number, which is not a "real" device. struct srom_dev should be a platform device and then this will get "easier" overall, sorry I missed that when the original code was submitted. thanks, greg k-h
diff --git a/drivers/char/tile-srom.c b/drivers/char/tile-srom.c index bd37747..be88699 100644 --- a/drivers/char/tile-srom.c +++ b/drivers/char/tile-srom.c @@ -350,7 +350,7 @@ static int srom_setup_minor(struct srom_dev *srom, int index) SROM_PAGE_SIZE_OFF, sizeof(srom->page_size)) < 0) return -EIO; - dev = device_create(srom_class, &platform_bus, + dev = device_create(srom_class, NULL, MKDEV(srom_major, index), srom, "%d", index); return PTR_ERR_OR_ZERO(dev); }
The code was creating "srom" class devices using platform_bus as a parent. As they are not really platform devices, make them virtual, using NULL instead. Cc: Chris Metcalf <cmetcalf@tilera.com> Signed-off-by: Pawel Moll <pawel.moll@arm.com> --- drivers/char/tile-srom.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)