| Message ID | 1406298233-27876-1-git-send-email-pawel.moll@arm.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 2014-07-25 at 15:23 +0100, Pawel Moll wrote: > The host devices without a parent were "forcefully adopted" > by platform bus. This patch removes this assignment. In > effect the dev_dev may be NULL now, which means ISA. > > Cc: James E.J. Bottomley <JBottomley@parallels.com> > Cc: linux-scsi@vger.kernel.org > Signed-off-by: Pawel Moll <pawel.moll@arm.com> > --- > > This patch is a part of effort to remove references to platform_bus > and make it static. > > James, could you please have a look and advice if the change is > correct? Would you happen to know the "real reasons" behind > using the root platform_bus device a parent? Yes, for DMA purposes, the parent cannot now be NULL; we'll get a panic in the DMA transfers if it is. A lot of the legacy ISA device on x86 and I thought some ARM SOC devices don't pass in the parent device, so we hang them off a known parent. You can grep for it; these are the devices that will begin to panic if you apply this patch: arch/ia64/hp/sim/simscsi.c: error = scsi_add_host(host, NULL); drivers/scsi/a2091.c: error = scsi_add_host(instance, NULL); drivers/scsi/a3000.c: error = scsi_add_host(instance, NULL); drivers/scsi/aha152x.c: if( scsi_add_host(shpnt, NULL) ) { drivers/scsi/gdth.c: error = scsi_add_host(shp, NULL); drivers/scsi/gdth.c: error = scsi_add_host(shp, NULL); drivers/scsi/gvp11.c: error = scsi_add_host(instance, NULL); drivers/scsi/imm.c: err = scsi_add_host(host, NULL); drivers/scsi/pcmcia/fdomain_stub.c: if (scsi_add_host(host, NULL)) drivers/scsi/pcmcia/nsp_cs.c: ret = scsi_add_host (host, NULL); drivers/scsi/pcmcia/qlogic_stub.c: if (scsi_add_host(shost, NULL)) drivers/scsi/pcmcia/sym53c500_cs.c: if (scsi_add_host(host, NULL)) drivers/scsi/ppa.c: err = scsi_add_host(host, NULL); drivers/scsi/qlogicfas.c: if (scsi_add_host(hreg, NULL)) drivers/scsi/scsi_module.c: error = scsi_add_host(shost, NULL); drivers/scsi/sgiwd93.c: err = scsi_add_host(host, NULL); Note I've picked up scsi_module, so anything that uses the SCSI module interface also has this problem. James
On Fri, 2014-07-25 at 15:46 +0100, James Bottomley wrote: > On Fri, 2014-07-25 at 15:23 +0100, Pawel Moll wrote: > > The host devices without a parent were "forcefully adopted" > > by platform bus. This patch removes this assignment. In > > effect the dev_dev may be NULL now, which means ISA. > > > > Cc: James E.J. Bottomley <JBottomley@parallels.com> > > Cc: linux-scsi@vger.kernel.org > > Signed-off-by: Pawel Moll <pawel.moll@arm.com> > > --- > > > > This patch is a part of effort to remove references to platform_bus > > and make it static. > > > > James, could you please have a look and advice if the change is > > correct? Would you happen to know the "real reasons" behind > > using the root platform_bus device a parent? > > Yes, for DMA purposes, the parent cannot now be NULL; we'll get a panic > in the DMA transfers if it is. That's what I though at the beginning as well, but then I crawled through get_dma_ops(struct device *dev) and it seems that on most architectures (all but two, if I remember correctly) it will return a default set of DMA ops if the dev == NULL, eg. static inline struct dma_map_ops *get_dma_ops(struct device *dev) { #ifndef CONFIG_X86_DEV_DMA_OPS return dma_ops; #else if (unlikely(!dev) || !dev->archdata.dma_ops) return dma_ops; else return dev->archdata.dma_ops; #endif } in arch/x86/include/asm/dma-mapping.h. Now, there seem to be only a handful of places where dev_dma is dereferenced: scsi_dma_map and scsi_dma_unmap in drivers/scsi/scsi_lib_dma.c, where all the calls seem to be "NULL resistant" and __scsi_alloc_queue in __scsi_alloc_queue which will oops as you said. Anyway, if you are saying that dev_dma must not be NULL at any circumstances, I'll either have to find some kind of replacement for platform_bus or convince Greg that platform_bus must not be made static ;-) > A lot of the legacy ISA device on x86 > and I thought some ARM SOC devices don't pass in the parent device, so > we hang them off a known parent. That's another thing I'm not sure - once assigned, is the dma_dev related to the parent in any way? Even more - is the shost_gendev.parent used at all? Doesn't seem to be. If it's only about a place in the device model hierarchy, leaving parent as NULL will make such device a virtual one, which it probably should be... Paweł
On Fri, Jul 25, 2014 at 07:46:56AM -0700, James Bottomley wrote: > On Fri, 2014-07-25 at 15:23 +0100, Pawel Moll wrote: > > The host devices without a parent were "forcefully adopted" > > by platform bus. This patch removes this assignment. In > > effect the dev_dev may be NULL now, which means ISA. > > > > Cc: James E.J. Bottomley <JBottomley@parallels.com> > > Cc: linux-scsi@vger.kernel.org > > Signed-off-by: Pawel Moll <pawel.moll@arm.com> > > --- > > > > This patch is a part of effort to remove references to platform_bus > > and make it static. > > > > James, could you please have a look and advice if the change is > > correct? Would you happen to know the "real reasons" behind > > using the root platform_bus device a parent? > > Yes, for DMA purposes, the parent cannot now be NULL; we'll get a panic > in the DMA transfers if it is. A lot of the legacy ISA device on x86 > and I thought some ARM SOC devices don't pass in the parent device, so > we hang them off a known parent. The "generic" platform bus device is not a "known parent". I don't understand the difference between just setting the parent to be NULL, which will then have a "proper" parent pointer filled in by the driver core when the device is registered, or faking it out here. What is the difference? In the end, the device always ends up with a parent pointer, right? thanks, greg k-h
On Fri, Jul 25, 2014 at 03:23:53PM +0100, Pawel Moll wrote: > ... describing the root of the device tree, so one can write > a platform driver initializing the platform. Wait, what do you mean by "one can write a platform driver initializing the platform"? I don't understand your end goal here... thanks, greg k-h
On Fri, Jul 25, 2014 at 03:23:53PM +0100, Pawel Moll wrote: > ... describing the root of the device tree, so one can write > a platform driver initializing the platform. > > Signed-off-by: Pawel Moll <pawel.moll@arm.com> > --- > drivers/base/platform.c | 20 ++++++++++++++------ > include/linux/platform_device.h | 2 +- > 2 files changed, 15 insertions(+), 7 deletions(-) > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > index eee48c4..9caffa7 100644 > --- a/drivers/base/platform.c > +++ b/drivers/base/platform.c > @@ -30,8 +30,8 @@ > /* For automatically allocated device IDs */ > static DEFINE_IDA(platform_devid_ida); > > -struct device platform_bus = { > - .init_name = "platform", > +struct platform_device platform_bus = { > + .name = "platform", > }; > EXPORT_SYMBOL_GPL(platform_bus); > > @@ -300,7 +300,7 @@ int platform_device_add(struct platform_device *pdev) > return -EINVAL; > > if (!pdev->dev.parent) > - pdev->dev.parent = &platform_bus; > + pdev->dev.parent = &platform_bus.dev; > > pdev->dev.bus = &platform_bus_type; > > @@ -946,12 +946,20 @@ int __init platform_bus_init(void) > > early_platform_cleanup(); > > - error = device_register(&platform_bus); > + dev_set_name(&platform_bus.dev, "%s", platform_bus.name); > + error = device_register(&platform_bus.dev); > if (error) > return error; > error = bus_register(&platform_bus_type); > - if (error) > - device_unregister(&platform_bus); > + if (!error) { > +#ifdef CONFIG_OF > + platform_bus.dev.of_node = of_allnodes; > +#endif Why are you doing this? The original code didn't do it and all was fine, right? What changes here? thanks, greg k-h
On Sat, 2014-07-26 at 13:11 -0700, Greg Kroah-Hartman wrote: > On Fri, Jul 25, 2014 at 07:46:56AM -0700, James Bottomley wrote: > > On Fri, 2014-07-25 at 15:23 +0100, Pawel Moll wrote: > > > The host devices without a parent were "forcefully adopted" > > > by platform bus. This patch removes this assignment. In > > > effect the dev_dev may be NULL now, which means ISA. > > > > > > Cc: James E.J. Bottomley <JBottomley@parallels.com> > > > Cc: linux-scsi@vger.kernel.org > > > Signed-off-by: Pawel Moll <pawel.moll@arm.com> > > > --- > > > > > > This patch is a part of effort to remove references to platform_bus > > > and make it static. > > > > > > James, could you please have a look and advice if the change is > > > correct? Would you happen to know the "real reasons" behind > > > using the root platform_bus device a parent? > > > > Yes, for DMA purposes, the parent cannot now be NULL; we'll get a panic > > in the DMA transfers if it is. A lot of the legacy ISA device on x86 > > and I thought some ARM SOC devices don't pass in the parent device, so > > we hang them off a known parent. > > The "generic" platform bus device is not a "known parent". I don't > understand the difference between just setting the parent to be NULL, > which will then have a "proper" parent pointer filled in by the driver > core when the device is registered, or faking it out here. What is the > difference? If you set the parent to NULL, the host template dma_dev will end up NULL as well and that will trigger a NULL deref panic in the dma segment routines. If you want to remove platform_bus, we have to have a well known device to set dma_dev to at scsi_host_add time. > In the end, the device always ends up with a parent pointer, right? The parent pointer isn't the problem ... assigning the correct dma device is. James
On Fri, Jul 25, 2014 at 03:23:49PM +0100, Pawel Moll wrote: > The bus devices created to be parents for other peripherals > were using platform_bus as a parent, not being platform > devices themselves. Remove the references, making them > virtual devices instead. > > Cc: Shawn Guo <shawn.guo@freescale.com> > Cc: Sascha Hauer <kernel@pengutronix.de> > Cc: Russell King <linux@arm.linux.org.uk> > Cc: linux-arm-kernel@lists.infradead.org > Signed-off-by: Pawel Moll <pawel.moll@arm.com> Acked-by: Shawn Guo <shawn.guo@freescale.com>
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 Sat, 2014-07-26 at 21:13 +0100, Greg Kroah-Hartman wrote: > > @@ -946,12 +946,20 @@ int __init platform_bus_init(void) > > > > early_platform_cleanup(); > > > > - error = device_register(&platform_bus); > > + dev_set_name(&platform_bus.dev, "%s", platform_bus.name); > > + error = device_register(&platform_bus.dev); > > if (error) > > return error; > > error = bus_register(&platform_bus_type); > > - if (error) > > - device_unregister(&platform_bus); > > + if (!error) { > > +#ifdef CONFIG_OF > > + platform_bus.dev.of_node = of_allnodes; > > +#endif > > Why are you doing this? The original code didn't do it and all was > fine, right? What changes here? You mean the #ifdef? It wasn't there, but Olof figured out that it breaks !CONFIG_OF builds: http://article.gmane.org/gmane.linux.ports.tegra/18473 as of_allnodes is only defined when CONFIG_OF. I had a choice of #ifdefing the assignment above or providing a dummy symbol. The latter doesn't seem sensibly, as there should be no other users for it (the symbol). Pawel
On Sat, 2014-07-26 at 21:12 +0100, Greg Kroah-Hartman wrote: > On Fri, Jul 25, 2014 at 03:23:53PM +0100, Pawel Moll wrote: > > ... describing the root of the device tree, so one can write > > a platform driver initializing the platform. > > Wait, what do you mean by "one can write a platform driver initializing > the platform"? I don't understand your end goal here... Bad wording, sorry. The goal is to have a platform driver (as in platform bus) that will initialize my platform (as in: board, machine, hardware). My platform (as in: the board) will be represented by the root platform bus device (as in: the bus ;-) with compatible value matching the one passed in the device tree's root. The tree: 8<---------------------------- / { compatible = "my,board"; } 8<---------------------------- The driver: 8<---------------------------- static struct of_device_id my_board_match[] = { { .compatible = "my,board", }, {}, }; static struct platform_driver my_board_driver = { .driver = { .name = "my_board", .owner = THIS_MODULE, .of_match_table = of_match_ptr(my_board_match), }, .probe = my_board_probe, .remove = my_board_remove, }; module_platform_driver(my_board_driver); 8<---------------------------- I'll work on better commit message for the next spin. Paweł
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 Sun, 2014-07-27 at 16:07 +0100, Greg Kroah-Hartman wrote: > Ah, ok, it's a scsi core thing, not a driver core thing, that's less > confusing now. For a "fallback" of a platform device, if you switch the > lines around you should be fine, something like this patch perhaps: > > diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c > index 3cbb57a8b846..d8d3b294f5bc 100644 > --- a/drivers/scsi/hosts.c > +++ b/drivers/scsi/hosts.c > @@ -218,16 +218,16 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev, > goto fail; > > if (!shost->shost_gendev.parent) > - shost->shost_gendev.parent = dev ? dev : &platform_bus; > - if (!dma_dev) > - dma_dev = shost->shost_gendev.parent; > - > - shost->dma_dev = dma_dev; > + shost->shost_gendev.parent = dev; > > error = device_add(&shost->shost_gendev); > if (error) > goto out; > > + if (!dma_dev) > + dma_dev = shost->shost_gendev.parent; > + shost->dma_dev = dma_dev; > + > pm_runtime_set_active(&shost->shost_gendev); > pm_runtime_enable(&shost->shost_gendev); > device_enable_async_suspend(&shost->shost_gendev); But this will still make shost->dma_dev == NULL in the cases James quoted: On Fri, 2014-07-25 at 15:46 +0100, James Bottomley wrote: > Yes, for DMA purposes, the parent cannot now be NULL; we'll get a panic > in the DMA transfers if it is. A lot of the legacy ISA device on x86 > and I thought some ARM SOC devices don't pass in the parent device, so > we hang them off a known parent. > > You can grep for it; these are the devices that will begin to panic if > you apply this patch: > > arch/ia64/hp/sim/simscsi.c: error = scsi_add_host(host, NULL); > drivers/scsi/a2091.c: error = scsi_add_host(instance, NULL); > drivers/scsi/a3000.c: error = scsi_add_host(instance, NULL); > drivers/scsi/aha152x.c: if( scsi_add_host(shpnt, NULL) ) { > drivers/scsi/gdth.c: error = scsi_add_host(shp, NULL); > drivers/scsi/gdth.c: error = scsi_add_host(shp, NULL); > drivers/scsi/gvp11.c: error = scsi_add_host(instance, NULL); > drivers/scsi/imm.c: err = scsi_add_host(host, NULL); > drivers/scsi/pcmcia/fdomain_stub.c: if (scsi_add_host(host, NULL)) > drivers/scsi/pcmcia/nsp_cs.c: ret = scsi_add_host (host, NULL); > drivers/scsi/pcmcia/qlogic_stub.c: if (scsi_add_host(shost, NULL)) > drivers/scsi/pcmcia/sym53c500_cs.c: if (scsi_add_host(host, NULL)) > drivers/scsi/ppa.c: err = scsi_add_host(host, NULL); > drivers/scsi/qlogicfas.c: if (scsi_add_host(hreg, NULL)) > drivers/scsi/scsi_module.c: error = scsi_add_host(shost, NULL); > drivers/scsi/sgiwd93.c: err = scsi_add_host(host, NULL); Maybe the DMA API should be coping with NULL device? It seems to handle it in a half of the methods and fails in the other half... 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
On Fri, 2014-07-25 at 15:23 +0100, Pawel Moll wrote: > The code selecting a device for the sdhci host has been > continuously tweaked (4b711cb13843f5082e82970dd1e8031383134a65 > "mmc: sdhci-pltfm: Add structure for host-specific data" and > a4d2177f00a5252d825236c5124bc1e9918bdb41 "mmc: sdhci-pltfm: dt > device does not pass parent to sdhci_alloc_host") while there > does not seem to be any reason to use platform device's parent > in the first place. > > The comment saying "Some PCI-based MFD need the parent here" > seem to refer to Timberdale FPGA driver (the only MFD driver > registering SDHCI cell, drivers/mfd/timberdale.c) but again, > the only situation when parent device matter is runtime PM, > which is not implemented for Timberdale. > > Cc: Chris Ball <chris@printf.net> > Cc: Anton Vorontsov <anton@enomsg.org> > Cc: Ulf Hansson <ulf.hansson@linaro.org> > Cc: linux-mmc@vger.kernel.org > Cc: linuxppc-dev@lists.ozlabs.org > Signed-off-by: Pawel Moll <pawel.moll@arm.com> > --- > > This patch is a part of effort to remove references to platform_bus > and make it static. > > Chris, Anton, Ulf - could you please advise if the assumptions > above are correct or if I'm completely wrong? Do you know what > where the real reasons to use parent originally? The PCI comment > seems like a red herring to me... Can I take the silence as a suggestion that the change looks ok-ish for you? Paweł
On 8 August 2014 18:36, Pawel Moll <pawel.moll@arm.com> wrote: > On Fri, 2014-07-25 at 15:23 +0100, Pawel Moll wrote: >> The code selecting a device for the sdhci host has been >> continuously tweaked (4b711cb13843f5082e82970dd1e8031383134a65 >> "mmc: sdhci-pltfm: Add structure for host-specific data" and >> a4d2177f00a5252d825236c5124bc1e9918bdb41 "mmc: sdhci-pltfm: dt >> device does not pass parent to sdhci_alloc_host") while there >> does not seem to be any reason to use platform device's parent >> in the first place. >> >> The comment saying "Some PCI-based MFD need the parent here" >> seem to refer to Timberdale FPGA driver (the only MFD driver >> registering SDHCI cell, drivers/mfd/timberdale.c) but again, >> the only situation when parent device matter is runtime PM, >> which is not implemented for Timberdale. >> >> Cc: Chris Ball <chris@printf.net> >> Cc: Anton Vorontsov <anton@enomsg.org> >> Cc: Ulf Hansson <ulf.hansson@linaro.org> >> Cc: linux-mmc@vger.kernel.org >> Cc: linuxppc-dev@lists.ozlabs.org >> Signed-off-by: Pawel Moll <pawel.moll@arm.com> >> --- >> >> This patch is a part of effort to remove references to platform_bus >> and make it static. >> >> Chris, Anton, Ulf - could you please advise if the assumptions >> above are correct or if I'm completely wrong? Do you know what >> where the real reasons to use parent originally? The PCI comment >> seems like a red herring to me... > > Can I take the silence as a suggestion that the change looks ok-ish for > you? Sorry for the delay. I suppose this make sense, but I really don't know for sure. I guess we need some testing in linux-next, to get some confidence. Kind regards Uffe > > Paweł > > -- > To unsubscribe from this list: send the line "unsubscribe linux-mmc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2014-08-11 at 10:07 +0100, Ulf Hansson wrote: > On 8 August 2014 18:36, Pawel Moll <pawel.moll@arm.com> wrote: > > On Fri, 2014-07-25 at 15:23 +0100, Pawel Moll wrote: > >> The code selecting a device for the sdhci host has been > >> continuously tweaked (4b711cb13843f5082e82970dd1e8031383134a65 > >> "mmc: sdhci-pltfm: Add structure for host-specific data" and > >> a4d2177f00a5252d825236c5124bc1e9918bdb41 "mmc: sdhci-pltfm: dt > >> device does not pass parent to sdhci_alloc_host") while there > >> does not seem to be any reason to use platform device's parent > >> in the first place. > >> > >> The comment saying "Some PCI-based MFD need the parent here" > >> seem to refer to Timberdale FPGA driver (the only MFD driver > >> registering SDHCI cell, drivers/mfd/timberdale.c) but again, > >> the only situation when parent device matter is runtime PM, > >> which is not implemented for Timberdale. > >> > >> Cc: Chris Ball <chris@printf.net> > >> Cc: Anton Vorontsov <anton@enomsg.org> > >> Cc: Ulf Hansson <ulf.hansson@linaro.org> > >> Cc: linux-mmc@vger.kernel.org > >> Cc: linuxppc-dev@lists.ozlabs.org > >> Signed-off-by: Pawel Moll <pawel.moll@arm.com> > >> --- > >> > >> This patch is a part of effort to remove references to platform_bus > >> and make it static. > >> > >> Chris, Anton, Ulf - could you please advise if the assumptions > >> above are correct or if I'm completely wrong? Do you know what > >> where the real reasons to use parent originally? The PCI comment > >> seems like a red herring to me... > > > > Can I take the silence as a suggestion that the change looks ok-ish for > > you? > > Sorry for the delay. I suppose this make sense, but I really don't > know for sure. > > I guess we need some testing in linux-next, to get some confidence. Would you take it into -next then? Unless I'm completely wrong there should be no impact on any in-tree driver... Cheers! Pawel
On 11 August 2014 11:15, Pawel Moll <pawel.moll@arm.com> wrote: > On Mon, 2014-08-11 at 10:07 +0100, Ulf Hansson wrote: >> On 8 August 2014 18:36, Pawel Moll <pawel.moll@arm.com> wrote: >> > On Fri, 2014-07-25 at 15:23 +0100, Pawel Moll wrote: >> >> The code selecting a device for the sdhci host has been >> >> continuously tweaked (4b711cb13843f5082e82970dd1e8031383134a65 >> >> "mmc: sdhci-pltfm: Add structure for host-specific data" and >> >> a4d2177f00a5252d825236c5124bc1e9918bdb41 "mmc: sdhci-pltfm: dt >> >> device does not pass parent to sdhci_alloc_host") while there >> >> does not seem to be any reason to use platform device's parent >> >> in the first place. >> >> >> >> The comment saying "Some PCI-based MFD need the parent here" >> >> seem to refer to Timberdale FPGA driver (the only MFD driver >> >> registering SDHCI cell, drivers/mfd/timberdale.c) but again, >> >> the only situation when parent device matter is runtime PM, >> >> which is not implemented for Timberdale. >> >> >> >> Cc: Chris Ball <chris@printf.net> >> >> Cc: Anton Vorontsov <anton@enomsg.org> >> >> Cc: Ulf Hansson <ulf.hansson@linaro.org> >> >> Cc: linux-mmc@vger.kernel.org >> >> Cc: linuxppc-dev@lists.ozlabs.org >> >> Signed-off-by: Pawel Moll <pawel.moll@arm.com> >> >> --- >> >> >> >> This patch is a part of effort to remove references to platform_bus >> >> and make it static. >> >> >> >> Chris, Anton, Ulf - could you please advise if the assumptions >> >> above are correct or if I'm completely wrong? Do you know what >> >> where the real reasons to use parent originally? The PCI comment >> >> seems like a red herring to me... >> > >> > Can I take the silence as a suggestion that the change looks ok-ish for >> > you? >> >> Sorry for the delay. I suppose this make sense, but I really don't >> know for sure. >> >> I guess we need some testing in linux-next, to get some confidence. > > Would you take it into -next then? Unless I'm completely wrong there > should be no impact on any in-tree driver... I will take it; though I think it's best to queue it for 3.18 to get some more testing. Kind regards Uffe
On Mon, Aug 11, 2014 at 10:15:50AM +0100, Pawel Moll wrote: > On Mon, 2014-08-11 at 10:07 +0100, Ulf Hansson wrote: > > Sorry for the delay. I suppose this make sense, but I really don't > > know for sure. > > > > I guess we need some testing in linux-next, to get some confidence. > > Would you take it into -next then? Unless I'm completely wrong there > should be no impact on any in-tree driver... But not until 3.17-rc1 has been released. linux-next should not receive new material other than bug fixes while a merge window is open.
On 11 August 2014 11:32, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 11 August 2014 11:15, Pawel Moll <pawel.moll@arm.com> wrote: >> On Mon, 2014-08-11 at 10:07 +0100, Ulf Hansson wrote: >>> On 8 August 2014 18:36, Pawel Moll <pawel.moll@arm.com> wrote: >>> > On Fri, 2014-07-25 at 15:23 +0100, Pawel Moll wrote: >>> >> The code selecting a device for the sdhci host has been >>> >> continuously tweaked (4b711cb13843f5082e82970dd1e8031383134a65 >>> >> "mmc: sdhci-pltfm: Add structure for host-specific data" and >>> >> a4d2177f00a5252d825236c5124bc1e9918bdb41 "mmc: sdhci-pltfm: dt >>> >> device does not pass parent to sdhci_alloc_host") while there >>> >> does not seem to be any reason to use platform device's parent >>> >> in the first place. >>> >> >>> >> The comment saying "Some PCI-based MFD need the parent here" >>> >> seem to refer to Timberdale FPGA driver (the only MFD driver >>> >> registering SDHCI cell, drivers/mfd/timberdale.c) but again, >>> >> the only situation when parent device matter is runtime PM, >>> >> which is not implemented for Timberdale. >>> >> >>> >> Cc: Chris Ball <chris@printf.net> >>> >> Cc: Anton Vorontsov <anton@enomsg.org> >>> >> Cc: Ulf Hansson <ulf.hansson@linaro.org> >>> >> Cc: linux-mmc@vger.kernel.org >>> >> Cc: linuxppc-dev@lists.ozlabs.org >>> >> Signed-off-by: Pawel Moll <pawel.moll@arm.com> >>> >> --- >>> >> >>> >> This patch is a part of effort to remove references to platform_bus >>> >> and make it static. >>> >> >>> >> Chris, Anton, Ulf - could you please advise if the assumptions >>> >> above are correct or if I'm completely wrong? Do you know what >>> >> where the real reasons to use parent originally? The PCI comment >>> >> seems like a red herring to me... >>> > >>> > Can I take the silence as a suggestion that the change looks ok-ish for >>> > you? >>> >>> Sorry for the delay. I suppose this make sense, but I really don't >>> know for sure. >>> >>> I guess we need some testing in linux-next, to get some confidence. >> >> Would you take it into -next then? Unless I'm completely wrong there >> should be no impact on any in-tree driver... > > I will take it; though I think it's best to queue it for 3.18 to get > some more testing. This patch causes a compiler warning, could you please fix it. Kind regards Uffe
diff --git a/arch/arm/mach-imx/devices/devices.c b/arch/arm/mach-imx/devices/devices.c index 1b4366a..8eab544 100644 --- a/arch/arm/mach-imx/devices/devices.c +++ b/arch/arm/mach-imx/devices/devices.c @@ -24,12 +24,10 @@ struct device mxc_aips_bus = { .init_name = "mxc_aips", - .parent = &platform_bus, }; struct device mxc_ahb_bus = { .init_name = "mxc_ahb", - .parent = &platform_bus, }; int __init mxc_device_init(void)
The bus devices created to be parents for other peripherals were using platform_bus as a parent, not being platform devices themselves. Remove the references, making them virtual devices instead. Cc: Shawn Guo <shawn.guo@freescale.com> Cc: Sascha Hauer <kernel@pengutronix.de> Cc: Russell King <linux@arm.linux.org.uk> Cc: linux-arm-kernel@lists.infradead.org Signed-off-by: Pawel Moll <pawel.moll@arm.com> --- This patch is a part of effort to remove references to platform_bus and make it static. Shawn, Sascha - could you, please, have a look if such change is acceptable for you? arch/arm/mach-imx/devices/devices.c | 2 -- 1 file changed, 2 deletions(-)