* [PATCH 1/2] mfd: max8925: request resource region @ 2012-05-07 3:10 Haojian Zhuang 2012-05-07 3:10 ` [PATCH 2/2] ARM: mmp: add io head file Haojian Zhuang ` (2 more replies) 0 siblings, 3 replies; 41+ messages in thread From: Haojian Zhuang @ 2012-05-07 3:10 UTC (permalink / raw) To: linux-arm-kernel If resource region isn't requested, component devices will fail to request their resources. Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com> --- drivers/mfd/max8925-core.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/drivers/mfd/max8925-core.c b/drivers/mfd/max8925-core.c index ca881ef..37aadcd 100644 --- a/drivers/mfd/max8925-core.c +++ b/drivers/mfd/max8925-core.c @@ -578,6 +578,10 @@ int __devinit max8925_device_init(struct max8925_chip *chip, { int ret; + if (!request_region(0, 0xff, "max8925")) { + dev_err(chip->dev, "Failed to register resource region\n"); + return -EBUSY; + } max8925_irq_init(chip, chip->i2c->irq, pdata); if (pdata && (pdata->power || pdata->touch)) { -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 2/2] ARM: mmp: add io head file 2012-05-07 3:10 [PATCH 1/2] mfd: max8925: request resource region Haojian Zhuang @ 2012-05-07 3:10 ` Haojian Zhuang 2012-05-07 9:23 ` Arnd Bergmann 2012-05-07 7:58 ` [PATCH 1/2] mfd: max8925: request resource region Russell King - ARM Linux 2012-05-07 8:12 ` Samuel Ortiz 2 siblings, 1 reply; 41+ messages in thread From: Haojian Zhuang @ 2012-05-07 3:10 UTC (permalink / raw) To: linux-arm-kernel We need IO_SPACE_LIMIT definition. Otherwise, we can't register max8925 PMIC. Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com> --- arch/arm/Kconfig | 1 + arch/arm/mach-mmp/include/mach/io.h | 13 +++++++++++++ 2 files changed, 14 insertions(+), 0 deletions(-) create mode 100644 arch/arm/mach-mmp/include/mach/io.h diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 36586dba..d503539 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -636,6 +636,7 @@ config ARCH_MMP select PLAT_PXA select SPARSE_IRQ select GENERIC_ALLOCATOR + select NEED_MACH_IO_H help Support for Marvell's PXA168/PXA910(MMP) and MMP2 processor line. diff --git a/arch/arm/mach-mmp/include/mach/io.h b/arch/arm/mach-mmp/include/mach/io.h new file mode 100644 index 0000000..ad25061 --- /dev/null +++ b/arch/arm/mach-mmp/include/mach/io.h @@ -0,0 +1,13 @@ +/* + * arch/arm/mach-mmp/include/mach/io.h + * + */ +#ifndef __ARCH_MMP_IO_H +#define __ARCH_MMP_IO_H + +#define IO_SPACE_LIMIT 0xffffffff + +#define __io(a) ((a) & IO_SPACE_LIMIT) + +#endif + -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 2/2] ARM: mmp: add io head file 2012-05-07 3:10 ` [PATCH 2/2] ARM: mmp: add io head file Haojian Zhuang @ 2012-05-07 9:23 ` Arnd Bergmann 0 siblings, 0 replies; 41+ messages in thread From: Arnd Bergmann @ 2012-05-07 9:23 UTC (permalink / raw) To: linux-arm-kernel On Monday 07 May 2012, Haojian Zhuang wrote: > +#ifndef __ARCH_MMP_IO_H > +#define __ARCH_MMP_IO_H > + > +#define IO_SPACE_LIMIT 0xffffffff > + > +#define __io(a) ((a) & IO_SPACE_LIMIT) > + > +#endif NAK If you don't have ISA or PCI devices, don't define these. More importantly, never define them to an incorrect value. This definition will cause an oops by NULL pointer dereferences for any access to ISA devices such as writing to /dev/port, or loading a module that probes the ISA ports. Arnd ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-07 3:10 [PATCH 1/2] mfd: max8925: request resource region Haojian Zhuang 2012-05-07 3:10 ` [PATCH 2/2] ARM: mmp: add io head file Haojian Zhuang @ 2012-05-07 7:58 ` Russell King - ARM Linux 2012-05-07 8:18 ` Haojian Zhuang 2012-05-07 9:01 ` Mark Brown 2012-05-07 8:12 ` Samuel Ortiz 2 siblings, 2 replies; 41+ messages in thread From: Russell King - ARM Linux @ 2012-05-07 7:58 UTC (permalink / raw) To: linux-arm-kernel On Mon, May 07, 2012 at 11:10:48AM +0800, Haojian Zhuang wrote: > If resource region isn't requested, component devices will fail to request > their resources. > > Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com> This looks wrong. This driver looks broken. IO regions are for PC IO stuff, not for I2C buses. There is no resource reservation system in the kernel for registers on I2C devices. ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-07 7:58 ` [PATCH 1/2] mfd: max8925: request resource region Russell King - ARM Linux @ 2012-05-07 8:18 ` Haojian Zhuang 2012-05-07 8:59 ` Russell King - ARM Linux 2012-05-07 9:01 ` Mark Brown 1 sibling, 1 reply; 41+ messages in thread From: Haojian Zhuang @ 2012-05-07 8:18 UTC (permalink / raw) To: linux-arm-kernel On Mon, May 7, 2012 at 3:58 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Mon, May 07, 2012 at 11:10:48AM +0800, Haojian Zhuang wrote: >> If resource region isn't requested, component devices will fail to request >> their resources. >> >> Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com> > > This looks wrong. > > This driver looks broken. ?IO regions are for PC IO stuff, not for I2C > buses. ?There is no resource reservation system in the kernel for > registers on I2C devices. There's some components in PMIC devices. I used IO resources to identify these components, and these IO resources are register offset in PMIC. But if I choose MEM resources, it may conflict with real memory resources. So I choose IO resources. Do you have any other implementation instead? Thanks Haojian ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-07 8:18 ` Haojian Zhuang @ 2012-05-07 8:59 ` Russell King - ARM Linux 0 siblings, 0 replies; 41+ messages in thread From: Russell King - ARM Linux @ 2012-05-07 8:59 UTC (permalink / raw) To: linux-arm-kernel On Mon, May 07, 2012 at 04:18:40PM +0800, Haojian Zhuang wrote: > On Mon, May 7, 2012 at 3:58 PM, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: > > On Mon, May 07, 2012 at 11:10:48AM +0800, Haojian Zhuang wrote: > >> If resource region isn't requested, component devices will fail to request > >> their resources. > >> > >> Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com> > > > > This looks wrong. > > > > This driver looks broken. ?IO regions are for PC IO stuff, not for I2C > > buses. ?There is no resource reservation system in the kernel for > > registers on I2C devices. > > There's some components in PMIC devices. I used IO resources to > identify these components, > and these IO resources are register offset in PMIC. But if I choose > MEM resources, it may > conflict with real memory resources. So I choose IO resources. Do you > have any other implementation > instead? Which is wrong as I've said above. This needs discussing and fixing; this driver should never have been merged with this abuse in. ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-07 7:58 ` [PATCH 1/2] mfd: max8925: request resource region Russell King - ARM Linux 2012-05-07 8:18 ` Haojian Zhuang @ 2012-05-07 9:01 ` Mark Brown 2012-05-07 9:08 ` Russell King - ARM Linux 1 sibling, 1 reply; 41+ messages in thread From: Mark Brown @ 2012-05-07 9:01 UTC (permalink / raw) To: linux-arm-kernel On Mon, May 07, 2012 at 08:58:00AM +0100, Russell King - ARM Linux wrote: > On Mon, May 07, 2012 at 11:10:48AM +0800, Haojian Zhuang wrote: > > If resource region isn't requested, component devices will fail to request > > their resources. > > Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com> > This looks wrong. > This driver looks broken. IO regions are for PC IO stuff, not for I2C > buses. There is no resource reservation system in the kernel for > registers on I2C devices. They've commonly been used for this, it's a fairly sane way for an MFD to communicate with its subdevices. The fix in this series isn't good, though - providing a parent resource with a suitable range for all the device resources should do the job much more sensibly. This isn't great but the infrastructure seems to do the right thing with it for now and it only requires a bit of reinterpretation of what IORESOURCE_IO means. ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-07 9:01 ` Mark Brown @ 2012-05-07 9:08 ` Russell King - ARM Linux 2012-05-07 9:47 ` Mark Brown 0 siblings, 1 reply; 41+ messages in thread From: Russell King - ARM Linux @ 2012-05-07 9:08 UTC (permalink / raw) To: linux-arm-kernel On Mon, May 07, 2012 at 10:01:28AM +0100, Mark Brown wrote: > On Mon, May 07, 2012 at 08:58:00AM +0100, Russell King - ARM Linux wrote: > > On Mon, May 07, 2012 at 11:10:48AM +0800, Haojian Zhuang wrote: > > > If resource region isn't requested, component devices will fail to request > > > their resources. > > > > Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com> > > > This looks wrong. > > > This driver looks broken. IO regions are for PC IO stuff, not for I2C > > buses. There is no resource reservation system in the kernel for > > registers on I2C devices. > > They've commonly been used for this, it's a fairly sane way for an MFD > to communicate with its subdevices. The fix in this series isn't good, > though - providing a parent resource with a suitable range for all the > device resources should do the job much more sensibly. This isn't great > but the infrastructure seems to do the right thing with it for now and > it only requires a bit of reinterpretation of what IORESOURCE_IO means. It's an abuse. And it won't work unless PCMCIA, PCI or ISA is enabled. This abuse must stop, and it must stop right now. And I really don't care about "it's commonly been used for XYZ" because it's a totally fucked idea. What if you have two devices both claiming IO regions at 0? Hint: it fails. It's buggered beyond belief and it needs to die right now, no questions about it. And anyone who supports this idea needs to be... ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-07 9:08 ` Russell King - ARM Linux @ 2012-05-07 9:47 ` Mark Brown 2012-05-07 10:14 ` Arnd Bergmann 0 siblings, 1 reply; 41+ messages in thread From: Mark Brown @ 2012-05-07 9:47 UTC (permalink / raw) To: linux-arm-kernel On Mon, May 07, 2012 at 10:08:58AM +0100, Russell King - ARM Linux wrote: > On Mon, May 07, 2012 at 10:01:28AM +0100, Mark Brown wrote: > > They've commonly been used for this, it's a fairly sane way for an MFD > > to communicate with its subdevices. The fix in this series isn't good, > > though - providing a parent resource with a suitable range for all the > > device resources should do the job much more sensibly. This isn't great > > but the infrastructure seems to do the right thing with it for now and > > it only requires a bit of reinterpretation of what IORESOURCE_IO means. > It's an abuse. And it won't work unless PCMCIA, PCI or ISA is enabled. What causes it to fail if these are disabled? I've got a bunch of systems here which don't appear to have any of those enabled as far as I can tell (though I don't know exactly what I'm looking for with ISA) but seem to be using this quite happily for some time now. > This abuse must stop, and it must stop right now. And I really don't > care about "it's commonly been used for XYZ" because it's a totally fucked > idea. I'd agree we should do something a bit nicer (though just having a separate resource tree for the chip registers like I suggested does seem like a reasonable first approach there). > What if you have two devices both claiming IO regions at 0? Hint: it > fails. Don't think I've got any examples with regions beginning at 0 but other regions seem to not run into any problems with overlap. Note that all these drivers do with the regions is use them to look up the base register. > It's buggered beyond belief and it needs to die right now, no questions > about it. And anyone who supports this idea needs to be... We have a bunch of drivers which have been in production for some time now. Simply saying the above without offering a constructive alternative doesn't really help move anything forward here. We could add a new resource type but it's not clear to me that there's any need to do so as it should be possible to arrange to avoid conflicts between the different address ranges, and obviously there's no little space in the bitmask used for resource types. -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120507/dfc545ad/attachment.sig> ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-07 9:47 ` Mark Brown @ 2012-05-07 10:14 ` Arnd Bergmann 2012-05-07 10:23 ` Haojian Zhuang 2012-05-07 10:37 ` Mark Brown 0 siblings, 2 replies; 41+ messages in thread From: Arnd Bergmann @ 2012-05-07 10:14 UTC (permalink / raw) To: linux-arm-kernel On Monday 07 May 2012, Mark Brown wrote: > > What if you have two devices both claiming IO regions at 0? Hint: it > > fails. > > Don't think I've got any examples with regions beginning at 0 but other > regions seem to not run into any problems with overlap. Note that all > these drivers do with the regions is use them to look up the base > register. ISA devices start at 0. Using a definition for IORESOURCE_IO of "PCI port number for relatively large values, but either ISA or PCMCIA or an arbitrary MFD for relatively small values" is absolutely crazy. FWIW, there are resources you define in include/linux/mfd/wm831x/core.h that have values between 0x4000 and 0x8000, which is exactly the range occupied by PCI devices on my thinkpad. It's only a matter of time until someone puts conflicting devices into one machine. Arnd ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-07 10:14 ` Arnd Bergmann @ 2012-05-07 10:23 ` Haojian Zhuang 2012-05-07 11:21 ` Arnd Bergmann 2012-05-07 10:37 ` Mark Brown 1 sibling, 1 reply; 41+ messages in thread From: Haojian Zhuang @ 2012-05-07 10:23 UTC (permalink / raw) To: linux-arm-kernel On Mon, May 7, 2012 at 6:14 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Monday 07 May 2012, Mark Brown wrote: >> > What if you have two devices both claiming IO regions at 0? ?Hint: it >> > fails. >> >> Don't think I've got any examples with regions beginning at 0 but other >> regions seem to not run into any problems with overlap. ?Note that all >> these drivers do with the regions is use them to look up the base >> register. > > ISA devices start at 0. Using a definition for IORESOURCE_IO of "PCI > port number for relatively large values, but either ISA or PCMCIA or > an arbitrary MFD for relatively small values" is absolutely crazy. > > FWIW, there are resources you define in include/linux/mfd/wm831x/core.h > that have values between 0x4000 and 0x8000, which is exactly the > range occupied by PCI devices on my thinkpad. It's only a matter of > time until someone puts conflicting devices into one machine. > > ? ? ? ?Arnd Even we append a new IORESOURCE type. We still meet similar issue. So it means that we can't use any IORESOURCE for this kind of i2c usage. Is it right? Regards Haojian ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-07 10:23 ` Haojian Zhuang @ 2012-05-07 11:21 ` Arnd Bergmann 2012-05-07 11:29 ` Mark Brown 0 siblings, 1 reply; 41+ messages in thread From: Arnd Bergmann @ 2012-05-07 11:21 UTC (permalink / raw) To: linux-arm-kernel On Monday 07 May 2012, Haojian Zhuang wrote: > Even we append a new IORESOURCE type. We still meet similar issue. So > it means that we can't use any IORESOURCE for this kind of i2c usage. Is > it right? Yes, basically the concept of IORESOURCE is incompatible here, unless you want to add a new IORESOURCE type for each one of these drivers, or you find a way to put all these devices into one global register space at unique addresses, which doesn't sound any better. Can you explain why you need this kind of arbitration to start with? Can't you just ensure that each client of the max8925 only sees a fixed set of nonconflicting registers, and provide a higher-level abstractions for the registers that are indeed shared between clients? Wouldn't you always know which drivers potentially coexist here? Arnd ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-07 11:21 ` Arnd Bergmann @ 2012-05-07 11:29 ` Mark Brown [not found] ` <201205071319.48768.arnd@arndb.de> 0 siblings, 1 reply; 41+ messages in thread From: Mark Brown @ 2012-05-07 11:29 UTC (permalink / raw) To: linux-arm-kernel On Mon, May 07, 2012 at 11:21:53AM +0000, Arnd Bergmann wrote: > Can you explain why you need this kind of arbitration to start with? > Can't you just ensure that each client of the max8925 only sees a fixed > set of nonconflicting registers, and provide a higher-level abstractions > for the registers that are indeed shared between clients? This is nothing to do with arbitration or sharing. It's for the case where you have a set of IP blocks on the chip (and possibly over a series of different chips) all with the same register map within the IP block - you need a way to tell the function driver for the IP block where it is in the chip register map. A similar thing happens (without issue) for the interrupts within the chip. You'd never expect any collisions to need arbitrating, it's purely about telling the function driver where to find the IP without having to open code this. Anything which is actually shared would be handled in the MFD core for the device normally, or with some other API like genirq. -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120507/c9432b9c/attachment-0001.sig> ^ permalink raw reply [flat|nested] 41+ messages in thread
[parent not found: <201205071319.48768.arnd@arndb.de>]
* [PATCH 1/2] mfd: max8925: request resource region [not found] ` <201205071319.48768.arnd@arndb.de> @ 2012-05-07 14:02 ` Samuel Ortiz 2012-05-07 14:15 ` Mark Brown 1 sibling, 0 replies; 41+ messages in thread From: Samuel Ortiz @ 2012-05-07 14:02 UTC (permalink / raw) To: linux-arm-kernel Hi Arnd, On Mon, May 07, 2012 at 01:19:48PM +0000, Arnd Bergmann wrote: > On Monday 07 May 2012, Mark Brown wrote: > > On Mon, May 07, 2012 at 11:21:53AM +0000, Arnd Bergmann wrote: > > > > > Can you explain why you need this kind of arbitration to start with? > > > Can't you just ensure that each client of the max8925 only sees a fixed > > > set of nonconflicting registers, and provide a higher-level abstractions > > > for the registers that are indeed shared between clients? > > > > This is nothing to do with arbitration or sharing. It's for the case > > where you have a set of IP blocks on the chip (and possibly over a > > series of different chips) all with the same register map within the IP > > block - you need a way to tell the function driver for the IP block > > where it is in the chip register map. A similar thing happens (without > > issue) for the interrupts within the chip. > > > > You'd never expect any collisions to need arbitrating, it's purely about > > telling the function driver where to find the IP without having to open > > code this. Anything which is actually shared would be handled in the > > MFD core for the device normally, or with some other API like genirq. > > The patch that we are discussing adds a call to 'request_region' -- > that is the arbitration interface and that is what is causing the > conflicts. > > Using a 'struct resource' of type IORESOURCE_IO to pass information > about non-PIO registers to child devices is inconsistent, ugly and > confusing, but I agree it doesn't actually result in bugs. The problems > start when you use those resources in combination with request_region, > request_resource and ioport_map. I agree with the latter and decided to not push this patch forward. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region [not found] ` <201205071319.48768.arnd@arndb.de> 2012-05-07 14:02 ` Samuel Ortiz @ 2012-05-07 14:15 ` Mark Brown 2012-05-07 15:15 ` Arnd Bergmann 1 sibling, 1 reply; 41+ messages in thread From: Mark Brown @ 2012-05-07 14:15 UTC (permalink / raw) To: linux-arm-kernel On Mon, May 07, 2012 at 01:19:48PM +0000, Arnd Bergmann wrote: > On Monday 07 May 2012, Mark Brown wrote: > > You'd never expect any collisions to need arbitrating, it's purely about > > telling the function driver where to find the IP without having to open > > code this. Anything which is actually shared would be handled in the > > MFD core for the device normally, or with some other API like genirq. > The patch that we are discussing adds a call to 'request_region' -- > that is the arbitration interface and that is what is causing the > conflicts. Oh dear - that's clearly broken, I'd not actually read the original patch just the thread that followed. > Using a 'struct resource' of type IORESOURCE_IO to pass information > about non-PIO registers to child devices is inconsistent, ugly and > confusing, but I agree it doesn't actually result in bugs. The problems I actually don't find it at all confusing, but I think that's mostly because at a high level I look at _IO as being about a register space and this is just a different register space. > start when you use those resources in combination with request_region, > request_resource and ioport_map. Right, yes - that's clearly never going to work. -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120507/840207e1/attachment-0001.sig> ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-07 14:15 ` Mark Brown @ 2012-05-07 15:15 ` Arnd Bergmann 2012-05-07 15:28 ` Mark Brown 0 siblings, 1 reply; 41+ messages in thread From: Arnd Bergmann @ 2012-05-07 15:15 UTC (permalink / raw) To: linux-arm-kernel On Monday 07 May 2012, Mark Brown wrote: > > Using a 'struct resource' of type IORESOURCE_IO to pass information > > about non-PIO registers to child devices is inconsistent, ugly and > > confusing, but I agree it doesn't actually result in bugs. The problems > > I actually don't find it at all confusing, but I think that's mostly > because at a high level I look at _IO as being about a register space > and this is just a different register space. At least it managed to confuse Haojian ;-) The part that confuses me most about the abuse of IORESOURCE_IO is that these have no parent resource and are not registered or listed in /proc/ioports. Arnd ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-07 15:15 ` Arnd Bergmann @ 2012-05-07 15:28 ` Mark Brown 0 siblings, 0 replies; 41+ messages in thread From: Mark Brown @ 2012-05-07 15:28 UTC (permalink / raw) To: linux-arm-kernel On Mon, May 07, 2012 at 03:15:08PM +0000, Arnd Bergmann wrote: > The part that confuses me most about the abuse of IORESOURCE_IO is that > these have no parent resource and are not registered or listed in > /proc/ioports. Well, if you're going to get confused about that sort of stuff there's also the fact that we've got platform devices which aren't the sort of simple memory mapped things that the platform bus is supposed to be for since any other bus for this sort of stuff would just be a straight copy of the platform bus. -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120507/081ec910/attachment.sig> ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-07 10:14 ` Arnd Bergmann 2012-05-07 10:23 ` Haojian Zhuang @ 2012-05-07 10:37 ` Mark Brown [not found] ` <201205071314.51886.arnd@arndb.de> 2012-05-07 18:57 ` Russell King - ARM Linux 1 sibling, 2 replies; 41+ messages in thread From: Mark Brown @ 2012-05-07 10:37 UTC (permalink / raw) To: linux-arm-kernel On Mon, May 07, 2012 at 10:14:03AM +0000, Arnd Bergmann wrote: > On Monday 07 May 2012, Mark Brown wrote: > > Don't think I've got any examples with regions beginning at 0 but other > > regions seem to not run into any problems with overlap. Note that all > > these drivers do with the regions is use them to look up the base > > register. > ISA devices start at 0. Using a definition for IORESOURCE_IO of "PCI > port number for relatively large values, but either ISA or PCMCIA or > an arbitrary MFD for relatively small values" is absolutely crazy. So what you're saying is that it's nothing to do with zero and just about plain conflicts - there's nothing magic about zero (which is what Russell seemed to be suggesting)? For whatever reason in the MFD usage the conflicts don't seem to be triggering - I suspect it's because the struct resource is simply inspected by the driver rather than doing whatever it is that PCI/ISA devices do with them, though I've not checked recently. > FWIW, there are resources you define in include/linux/mfd/wm831x/core.h > that have values between 0x4000 and 0x8000, which is exactly the > range occupied by PCI devices on my thinkpad. It's only a matter of > time until someone puts conflicting devices into one machine. Yes, I agree it will be a problem if they're part of the same resource tree and used via the same API but if we allow multiple trees of I/O resources then does that avoid the issue? It seems like it ought to be something we can do, if not we'll have to define a new resource type and like I say it's a pretty full bitmask at the minute... -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120507/72995196/attachment.sig> ^ permalink raw reply [flat|nested] 41+ messages in thread
[parent not found: <201205071314.51886.arnd@arndb.de>]
* [PATCH 1/2] mfd: max8925: request resource region [not found] ` <201205071314.51886.arnd@arndb.de> @ 2012-05-07 14:06 ` Mark Brown 2012-05-07 15:09 ` Arnd Bergmann 0 siblings, 1 reply; 41+ messages in thread From: Mark Brown @ 2012-05-07 14:06 UTC (permalink / raw) To: linux-arm-kernel On Mon, May 07, 2012 at 01:14:51PM +0000, Arnd Bergmann wrote: > There are two distinct problems here: > * conflicts in request_resource() between stuff that is in fundamentally > different. > * defining the __io() address space to have a zero offset, which causes > NULL pointer dereferences in legacy ISA drivers. Right, though in the MFD case neither of these things should be relevant as the resources should never actually be used in this way. > My feeling is that the resource model is just hasn't moved along > with the times (predates and doesn't really map to the device model) and > is used in a consistent way (request_resource, allocate_resource and > request_region operate on the same types, but in different ways). > It's not clear to me whether it makes sense to continue this path > for new kinds of resources. This is true. On the other hand there's some infrastructure around resources which is pretty helpful, though now I look at it most of this is actually specific to platform devices rather than being a platform device wrapper for a generic thing. Things like platform_get_resource() are pretty good to use, and in the context of platform devices the whole resource conflict thing is normally pretty much irrelevant. > My understanding is also that the uses in MFD (e.g. max8925 and wm831x) > are only interested in the aspect of passing information to child devices > rather than arbitrating between conflicting accesses. If that's the case, > a separate mechanism that doesn't use a global numbering scheme might > be more appropriate. Given what I'm saying about platform devices above perhaps we should be factoring some of the platform device stuff up to struct device level. Another option would be to work on separating the management of the number spaces and the interfaces for getting the numbers back out to make it easier to add more number spaces. -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120507/5b4129d5/attachment.sig> ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-07 14:06 ` Mark Brown @ 2012-05-07 15:09 ` Arnd Bergmann 2012-05-07 15:17 ` Mark Brown 0 siblings, 1 reply; 41+ messages in thread From: Arnd Bergmann @ 2012-05-07 15:09 UTC (permalink / raw) To: linux-arm-kernel On Monday 07 May 2012, Mark Brown wrote: > On Mon, May 07, 2012 at 01:14:51PM +0000, Arnd Bergmann wrote: > > > There are two distinct problems here: > > > * conflicts in request_resource() between stuff that is in fundamentally > > different. > > * defining the __io() address space to have a zero offset, which causes > > NULL pointer dereferences in legacy ISA drivers. > > Right, though in the MFD case neither of these things should be relevant > as the resources should never actually be used in this way. Well, particularly patch 2 of this series introduces those problems, but yes, it should be a separate issue. > > My feeling is that the resource model is just hasn't moved along > > with the times (predates and doesn't really map to the device model) and > > is used in a consistent way (request_resource, allocate_resource and > > request_region operate on the same types, but in different ways). > > It's not clear to me whether it makes sense to continue this path > > for new kinds of resources. > > This is true. On the other hand there's some infrastructure around > resources which is pretty helpful, though now I look at it most of this > is actually specific to platform devices rather than being a platform > device wrapper for a generic thing. Things like platform_get_resource() > are pretty good to use, and in the context of platform devices the whole > resource conflict thing is normally pretty much irrelevant. Right. > > My understanding is also that the uses in MFD (e.g. max8925 and wm831x) > > are only interested in the aspect of passing information to child devices > > rather than arbitrating between conflicting accesses. If that's the case, > > a separate mechanism that doesn't use a global numbering scheme might > > be more appropriate. > > Given what I'm saying about platform devices above perhaps we should be > factoring some of the platform device stuff up to struct device level. > Another option would be to work on separating the management of the > number spaces and the interfaces for getting the numbers back out to > make it easier to add more number spaces. Isn't that what devres is for? We should be able to just attach arbitrary data to a device with this, e.g. a struct regmap to use for doing I/O that a driver can use. Maybe we should add some wrappers around that to make it more obvious to use. Arnd ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-07 15:09 ` Arnd Bergmann @ 2012-05-07 15:17 ` Mark Brown 2012-05-07 19:26 ` Arnd Bergmann 0 siblings, 1 reply; 41+ messages in thread From: Mark Brown @ 2012-05-07 15:17 UTC (permalink / raw) To: linux-arm-kernel On Mon, May 07, 2012 at 03:09:54PM +0000, Arnd Bergmann wrote: > On Monday 07 May 2012, Mark Brown wrote: > > Given what I'm saying about platform devices above perhaps we should be > > factoring some of the platform device stuff up to struct device level. > > Another option would be to work on separating the management of the > > number spaces and the interfaces for getting the numbers back out to > > make it easier to add more number spaces. > Isn't that what devres is for? We should be able to just attach arbitrary > data to a device with this, e.g. a struct regmap to use for doing I/O > that a driver can use. Maybe we should add some wrappers around that > to make it more obvious to use. Not as far as I understand it, it's more about making error handling and unregistration code less error prone by automating the freeing of things when a device goes away or probe() fails. The managed versions wrap the unmanaged allocators but don't generally otherwise change the interface except by adding a struct device. Unless I'm missing something, of course. -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120507/6dcfa6bf/attachment.sig> ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-07 15:17 ` Mark Brown @ 2012-05-07 19:26 ` Arnd Bergmann 2012-05-07 19:58 ` Mark Brown 2012-05-08 8:17 ` Mark Brown 0 siblings, 2 replies; 41+ messages in thread From: Arnd Bergmann @ 2012-05-07 19:26 UTC (permalink / raw) To: linux-arm-kernel On Monday 07 May 2012, Mark Brown wrote: > On Mon, May 07, 2012 at 03:09:54PM +0000, Arnd Bergmann wrote: > > On Monday 07 May 2012, Mark Brown wrote: > > > > Given what I'm saying about platform devices above perhaps we should be > > > factoring some of the platform device stuff up to struct device level. > > > Another option would be to work on separating the management of the > > > number spaces and the interfaces for getting the numbers back out to > > > make it easier to add more number spaces. > > > Isn't that what devres is for? We should be able to just attach arbitrary > > data to a device with this, e.g. a struct regmap to use for doing I/O > > that a driver can use. Maybe we should add some wrappers around that > > to make it more obvious to use. > > Not as far as I understand it, it's more about making error handling and > unregistration code less error prone by automating the freeing of > things when a device goes away or probe() fails. The managed versions > wrap the unmanaged allocators but don't generally otherwise change the > interface except by adding a struct device. Unless I'm missing > something, of course. Yes, it also does that, but have a look at how drivers/pci/pci.c uses devres to attach auxiliary attributes to a device. We can do the same thing for other devices to attach any data. Arnd ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-07 19:26 ` Arnd Bergmann @ 2012-05-07 19:58 ` Mark Brown 2012-05-08 8:17 ` Mark Brown 1 sibling, 0 replies; 41+ messages in thread From: Mark Brown @ 2012-05-07 19:58 UTC (permalink / raw) To: linux-arm-kernel On Mon, May 07, 2012 at 07:26:08PM +0000, Arnd Bergmann wrote: > On Monday 07 May 2012, Mark Brown wrote: > > On Mon, May 07, 2012 at 03:09:54PM +0000, Arnd Bergmann wrote: > > > On Monday 07 May 2012, Mark Brown wrote: > > > > Given what I'm saying about platform devices above perhaps we should be > > > > factoring some of the platform device stuff up to struct device level. > > > Isn't that what devres is for? We should be able to just attach arbitrary > > > data to a device with this, e.g. a struct regmap to use for doing I/O > > > that a driver can use. Maybe we should add some wrappers around that > > > to make it more obvious to use. > > Not as far as I understand it, it's more about making error handling and > > unregistration code less error prone by automating the freeing of > > things when a device goes away or probe() fails. The managed versions > Yes, it also does that, but have a look at how drivers/pci/pci.c uses > devres to attach auxiliary attributes to a device. We can do the same > thing for other devices to attach any data. Right, but that's essentially just the bus doing the equivalent of devm_kzalloc() to allocate some private data which is a separate thing. It doesn't really address the problem of passing information from registration code to the device through the bus which is what the resource API is currently doing for these devices. We probably should be making a bit more use of devres for this than we are (using it to bounce through a regmap isn't a bad idea, I might just do that though a lot of the time you need the parent internal data struct anyway) but it's a separate thing. Something like pulling the type out of the flags in struct resource to give us a bit more space (or replace the bits with a pointer, though that's even more invasive) would address this more... -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120507/564ed995/attachment.sig> ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-07 19:26 ` Arnd Bergmann 2012-05-07 19:58 ` Mark Brown @ 2012-05-08 8:17 ` Mark Brown 2012-05-08 14:44 ` Arnd Bergmann 1 sibling, 1 reply; 41+ messages in thread From: Mark Brown @ 2012-05-08 8:17 UTC (permalink / raw) To: linux-arm-kernel On Mon, May 07, 2012 at 07:26:08PM +0000, Arnd Bergmann wrote: > Yes, it also does that, but have a look at how drivers/pci/pci.c uses > devres to attach auxiliary attributes to a device. We can do the same > thing for other devices to attach any data. Actually, thinking about this further devres is definitely not what we want here: when the device is unbound it'll free all the resources it has for the device which is definitely not what we want for things like this. We need the data to persist for as long as the device exists. -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120508/cc6b4ac2/attachment-0001.sig> ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-08 8:17 ` Mark Brown @ 2012-05-08 14:44 ` Arnd Bergmann 2012-05-08 15:31 ` Mark Brown 0 siblings, 1 reply; 41+ messages in thread From: Arnd Bergmann @ 2012-05-08 14:44 UTC (permalink / raw) To: linux-arm-kernel On Tuesday 08 May 2012, Mark Brown wrote: > On Mon, May 07, 2012 at 07:26:08PM +0000, Arnd Bergmann wrote: > > > Yes, it also does that, but have a look at how drivers/pci/pci.c uses > > devres to attach auxiliary attributes to a device. We can do the same > > thing for other devices to attach any data. > > Actually, thinking about this further devres is definitely not what we > want here: when the device is unbound it'll free all the resources it > has for the device which is definitely not what we want for things like > this. We need the data to persist for as long as the device exists. Yes, good point. How about a IORESOURCE_REGMAP type then with the following semantics: Each struct regmap gets an embedded resource that gets a unique range in the IORESOURCE_REGMAP space using allocate_resource, and each device using that can have its own sub-resources registered to that. Then we add a helper function that pulls out the regmap from the resource using something like container_of(res->parent, struct regmap, resource) and calculates the register number by subtracting the parent->start from res->start. Arnd ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-08 14:44 ` Arnd Bergmann @ 2012-05-08 15:31 ` Mark Brown 2012-05-09 12:43 ` Arnd Bergmann 0 siblings, 1 reply; 41+ messages in thread From: Mark Brown @ 2012-05-08 15:31 UTC (permalink / raw) To: linux-arm-kernel On Tue, May 08, 2012 at 02:44:30PM +0000, Arnd Bergmann wrote: > How about a IORESOURCE_REGMAP type then with the following semantics: The trick is allocating a bit for that flag, though I was thinking earlier on we might be able to get away with carving it out of the bus specific bits since I think anything using this wouldn't normally be using resources for anything else except interrupts. It does mean that the resource type doesn't do the right thing though. > Each struct regmap gets an embedded resource that gets a unique > range in the IORESOURCE_REGMAP space using allocate_resource, > and each device using that can have its own sub-resources registered > to that. > Then we add a helper function that pulls out the regmap from the resource > using something like container_of(res->parent, struct regmap, resource) > and calculates the register number by subtracting the parent->start from > res->start. That feels complicated with the subtraction there, but modulo that this is roughly what's happening now. We would I think want this to work for non-regmap users too, it's not yet clear that every device with registers is going to fit into regmap (though it's looking more likely as things go on). -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120508/d4e2a8c4/attachment.sig> ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-08 15:31 ` Mark Brown @ 2012-05-09 12:43 ` Arnd Bergmann 2012-05-09 14:13 ` Mark Brown 2012-05-09 16:07 ` Russell King - ARM Linux 0 siblings, 2 replies; 41+ messages in thread From: Arnd Bergmann @ 2012-05-09 12:43 UTC (permalink / raw) To: linux-arm-kernel On Tuesday 08 May 2012, Mark Brown wrote: > On Tue, May 08, 2012 at 02:44:30PM +0000, Arnd Bergmann wrote: > > > How about a IORESOURCE_REGMAP type then with the following semantics: > > The trick is allocating a bit for that flag, though I was thinking > earlier on we might be able to get away with carving it out of the bus > specific bits since I think anything using this wouldn't normally be > using resources for anything else except interrupts. It does mean that > the resource type doesn't do the right thing though. Right, or we could try to kill IORESOURCE_BUS, which is hardly used anywhere and the users either get it wrong (bfin net2272), use it only in one file (acpi, broadcom-pci) or only for printing the contents (pnp). > > Each struct regmap gets an embedded resource that gets a unique > > range in the IORESOURCE_REGMAP space using allocate_resource, > > and each device using that can have its own sub-resources registered > > to that. > > > Then we add a helper function that pulls out the regmap from the resource > > using something like container_of(res->parent, struct regmap, resource) > > and calculates the register number by subtracting the parent->start from > > res->start. > > That feels complicated with the subtraction there, but modulo that this > is roughly what's happening now. We would I think want this to work for > non-regmap users too, it's not yet clear that every device with > registers is going to fit into regmap (though it's looking more likely > as things go on). I think the subtraction in some form is needed if you want to register the resources, to guarantee that they are non-conflicting. Registering them has the advantage that we can print them in a similar way to what we do for /proc/{ioports,iomem}. Arnd ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-09 12:43 ` Arnd Bergmann @ 2012-05-09 14:13 ` Mark Brown 2012-05-09 14:19 ` Arnd Bergmann 2012-05-09 16:18 ` Russell King - ARM Linux 2012-05-09 16:07 ` Russell King - ARM Linux 1 sibling, 2 replies; 41+ messages in thread From: Mark Brown @ 2012-05-09 14:13 UTC (permalink / raw) To: linux-arm-kernel On Wed, May 09, 2012 at 12:43:13PM +0000, Arnd Bergmann wrote: > On Tuesday 08 May 2012, Mark Brown wrote: > > The trick is allocating a bit for that flag, though I was thinking > > earlier on we might be able to get away with carving it out of the bus > > specific bits since I think anything using this wouldn't normally be > > using resources for anything else except interrupts. It does mean that > > the resource type doesn't do the right thing though. > Right, or we could try to kill IORESOURCE_BUS, which is hardly used > anywhere and the users either get it wrong (bfin net2272), use it > only in one file (acpi, broadcom-pci) or only for printing the > contents (pnp). Yeah, I looked at that - PCMCIA seems to be the major sticking point and the usage there appeared totally sane, though it's possible that if I stare at it hard enough I might convince myself that it only needs seven bits and we can steal one. > > That feels complicated with the subtraction there, but modulo that this > > is roughly what's happening now. We would I think want this to work for > > non-regmap users too, it's not yet clear that every device with > > registers is going to fit into regmap (though it's looking more likely > > as things go on). > I think the subtraction in some form is needed if you want to register > the resources, to guarantee that they are non-conflicting. Registering > them has the advantage that we can print them in a similar way to what > we do for /proc/{ioports,iomem}. It does result in the really odd situation that the number that gets stored in the resource is the register number plus an offset. If anything I'd expect that the resources would be defined so you had to add the parent base address with the child resource being the offset within the parent resource. Though to be honest the parent resource is almost always going to be numbered from zero so it's not going to make much difference most of the time. -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120509/759ff3cc/attachment.sig> ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-09 14:13 ` Mark Brown @ 2012-05-09 14:19 ` Arnd Bergmann 2012-05-09 14:42 ` Mark Brown 2012-05-09 16:18 ` Russell King - ARM Linux 1 sibling, 1 reply; 41+ messages in thread From: Arnd Bergmann @ 2012-05-09 14:19 UTC (permalink / raw) To: linux-arm-kernel On Wednesday 09 May 2012, Mark Brown wrote: > It does result in the really odd situation that the number that gets > stored in the resource is the register number plus an offset. If > anything I'd expect that the resources would be defined so you had to > add the parent base address with the child resource being the offset > within the parent resource. Though to be honest the parent resource is > almost always going to be numbered from zero so it's not going to make > much difference most of the time. But that's not how any of the other resources work: AFAICT, each resource type makes up an address space with unique ranges assigned to each sibling and sub-ranges inside of that for its children. Arnd ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-09 14:19 ` Arnd Bergmann @ 2012-05-09 14:42 ` Mark Brown 2012-05-09 15:03 ` Arnd Bergmann 0 siblings, 1 reply; 41+ messages in thread From: Mark Brown @ 2012-05-09 14:42 UTC (permalink / raw) To: linux-arm-kernel On Wed, May 09, 2012 at 02:19:50PM +0000, Arnd Bergmann wrote: > On Wednesday 09 May 2012, Mark Brown wrote: > > It does result in the really odd situation that the number that gets > > stored in the resource is the register number plus an offset. If > > anything I'd expect that the resources would be defined so you had to > > add the parent base address with the child resource being the offset > > within the parent resource. Though to be honest the parent resource is > > almost always going to be numbered from zero so it's not going to make > > much difference most of the time. > But that's not how any of the other resources work: AFAICT, each resource > type makes up an address space with unique ranges assigned to each > sibling and sub-ranges inside of that for its children. That's what I'd expect, but you're saying that the values in the child resources are being stored as base+actual values (where base is some random number) rather than either actual values or as offsets within the base range either of which would seem less surprising. Presumably you'd also need to walk up the tree to subtract all the bases if there was more than one resource. Perhaps I'm just totally failing to understand what you're saying? -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120509/70a9fd82/attachment.sig> ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-09 14:42 ` Mark Brown @ 2012-05-09 15:03 ` Arnd Bergmann 2012-05-09 15:28 ` Mark Brown 0 siblings, 1 reply; 41+ messages in thread From: Arnd Bergmann @ 2012-05-09 15:03 UTC (permalink / raw) To: linux-arm-kernel On Wednesday 09 May 2012, Mark Brown wrote: > That's what I'd expect, but you're saying that the values in the child > resources are being stored as base+actual values (where base is some > random number) rather than either actual values or as offsets within the > base range either of which would seem less surprising. Presumably you'd > also need to walk up the tree to subtract all the bases if there was > more than one resource. > > Perhaps I'm just totally failing to understand what you're saying? I mean that each device providing resources of the new type would need to register its resources to the same root resource with non-conclicting addresses. Having resources hanging in the air without a parent pointer is not good, although we seem to be doing that for a lot of platform devices these days, where we just pass the struct resource from platform code to a driver without ever registering it at each end. Arnd ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-09 15:03 ` Arnd Bergmann @ 2012-05-09 15:28 ` Mark Brown 2012-05-09 16:27 ` Arnd Bergmann 0 siblings, 1 reply; 41+ messages in thread From: Mark Brown @ 2012-05-09 15:28 UTC (permalink / raw) To: linux-arm-kernel On Wed, May 09, 2012 at 03:03:22PM +0000, Arnd Bergmann wrote: > I mean that each device providing resources of the new type would need > to register its resources to the same root resource with non-conclicting Oh, right - I see what you mean. I was thinking of creating a per-chip parent for each tree rather than trying to make them a unified tree, though I guess it's useful for diagnostics. Multi-level resources might be slightly fun with this though I'm not sure how realistic they are. > addresses. Having resources hanging in the air without a parent pointer > is not good, although we seem to be doing that for a lot of platform > devices these days, where we just pass the struct resource from platform > code to a driver without ever registering it at each end. They should all be being registered by the platform core when the platform device is registered I think? -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120509/c99a5363/attachment-0001.sig> ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-09 15:28 ` Mark Brown @ 2012-05-09 16:27 ` Arnd Bergmann 0 siblings, 0 replies; 41+ messages in thread From: Arnd Bergmann @ 2012-05-09 16:27 UTC (permalink / raw) To: linux-arm-kernel On Wednesday 09 May 2012, Mark Brown wrote: > Show Details > On Wed, May 09, 2012 at 03:03:22PM +0000, Arnd Bergmann wrote: > > > I mean that each device providing resources of the new type would need > > to register its resources to the same root resource with non-conclicting > > Oh, right - I see what you mean. I was thinking of creating a > per-chip parent for each tree rather than trying to make them a unified > tree, though I guess it's useful for diagnostics. Multi-level resources > might be slightly fun with this though I'm not sure how realistic they > are. Well, as Russell just pointed out, that's not actually a requirement, so we probably don't want to do that after all. Arnd ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-09 14:13 ` Mark Brown 2012-05-09 14:19 ` Arnd Bergmann @ 2012-05-09 16:18 ` Russell King - ARM Linux 2012-05-09 16:30 ` Mark Brown 1 sibling, 1 reply; 41+ messages in thread From: Russell King - ARM Linux @ 2012-05-09 16:18 UTC (permalink / raw) To: linux-arm-kernel On Wed, May 09, 2012 at 03:13:16PM +0100, Mark Brown wrote: > On Wed, May 09, 2012 at 12:43:13PM +0000, Arnd Bergmann wrote: > > On Tuesday 08 May 2012, Mark Brown wrote: > > > > The trick is allocating a bit for that flag, though I was thinking > > > earlier on we might be able to get away with carving it out of the bus > > > specific bits since I think anything using this wouldn't normally be > > > using resources for anything else except interrupts. It does mean that > > > the resource type doesn't do the right thing though. > > > Right, or we could try to kill IORESOURCE_BUS, which is hardly used > > anywhere and the users either get it wrong (bfin net2272), use it > > only in one file (acpi, broadcom-pci) or only for printing the > > contents (pnp). > > Yeah, I looked at that - PCMCIA seems to be the major sticking point and > the usage there appeared totally sane, though it's possible that if I > stare at it hard enough I might convince myself that it only needs seven > bits and we can steal one. Err what? PCMCIA doesn't use IORESOURCE_BUS. (Don't get it confused with IORESOURCE_BUSY which is totally different). IORESOURCE_BUS seems to be used for: net2272: specifying the register index shift value acpi: to get the start/end PCI bus number values pnp: for ACPI bus numbers x86: PCI bus numbering So it looks like net2272 is abusing this resource type for its own purposes... The others look like proper use of this resource type. ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-09 16:18 ` Russell King - ARM Linux @ 2012-05-09 16:30 ` Mark Brown 0 siblings, 0 replies; 41+ messages in thread From: Mark Brown @ 2012-05-09 16:30 UTC (permalink / raw) To: linux-arm-kernel On Wed, May 09, 2012 at 05:18:39PM +0100, Russell King - ARM Linux wrote: > On Wed, May 09, 2012 at 03:13:16PM +0100, Mark Brown wrote: > > Yeah, I looked at that - PCMCIA seems to be the major sticking point and > > the usage there appeared totally sane, though it's possible that if I > > stare at it hard enough I might convince myself that it only needs seven > > bits and we can steal one. > Err what? PCMCIA doesn't use IORESOURCE_BUS. (Don't get it confused with > IORESOURCE_BUSY which is totally different). I've actually got it confused with IORESOURCE_BITS (because it's there for bus-specific usage, as opposed to bus numbers) which it does use - sorry. -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120509/40e330c3/attachment.sig> ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-09 12:43 ` Arnd Bergmann 2012-05-09 14:13 ` Mark Brown @ 2012-05-09 16:07 ` Russell King - ARM Linux 2012-05-09 16:26 ` Arnd Bergmann 1 sibling, 1 reply; 41+ messages in thread From: Russell King - ARM Linux @ 2012-05-09 16:07 UTC (permalink / raw) To: linux-arm-kernel On Wed, May 09, 2012 at 12:43:13PM +0000, Arnd Bergmann wrote: > I think the subtraction in some form is needed if you want to register > the resources, to guarantee that they are non-conflicting. Registering > them has the advantage that we can print them in a similar way to what > we do for /proc/{ioports,iomem}. You don't need to - you only need them non-conflicting if they're part of the same tree. It seems to me that having all device register resources being part of the same tree is the wrong approach, it's certainly the wrong model for this kind of thing. Instead, we should be having per-device resources trees. So, for instance, an I2C device with two functions, function 1 say owns register indices 0-0x3f and function 2 owns 0x40-0x7f. We should have: struct i2c_blah_driver_info { struct resource reg_res; }; priv->reg_res.start = 0; priv->reg_res.end = 0x7f; priv->reg_res.flags = 0; subdev1->resource[0].start = 0; subdev1->resource[0].end = 0x3f; subdev1->resource[0].flags = 0; ret = request_resource(&priv->reg_res, &subdev1->resource[0]); /* check ret */ subdev2->resource[0].start = 0x40; subdev2->resource[0].end = 0x7f; subdev2->resource[0].flags = 0; ret = request_resource(&priv->reg_res, &subdev2->resource[0]); /* check ret */ This means that we end up with lots of per-device resource trees rooted at the top-level-device driver, each effectively with their own separate name spaces. If we care about marking resources busy to avoid drivers conflicting, I would also suggest that we augment the resource interfaces so that we can have mark_resource_busy(dev, res, drivername) which takes the 'bus' resource and adds the IORESOURCE_BUSY resource below it. I'd also suggest at this point that we don't have a corresponding release function for this as I believe we should be using devm stuff internally for this now. Do we care about making them appear in procfs? I suspect at this stage that would be wrong. If we care at all, they should probably be per- top-level-device sysfs files - and we should have a function which sysfs can use along with the root resource to create that output. ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-09 16:07 ` Russell King - ARM Linux @ 2012-05-09 16:26 ` Arnd Bergmann 2012-05-09 16:27 ` Russell King - ARM Linux 0 siblings, 1 reply; 41+ messages in thread From: Arnd Bergmann @ 2012-05-09 16:26 UTC (permalink / raw) To: linux-arm-kernel On Wednesday 09 May 2012, Russell King - ARM Linux wrote: > Instead, we should be having per-device resources trees. So, for instance, > an I2C device with two functions, function 1 say owns register indices > 0-0x3f and function 2 owns 0x40-0x7f. We should have: > > struct i2c_blah_driver_info { > struct resource reg_res; > }; > > priv->reg_res.start = 0; > priv->reg_res.end = 0x7f; > priv->reg_res.flags = 0; > > subdev1->resource[0].start = 0; > subdev1->resource[0].end = 0x3f; > subdev1->resource[0].flags = 0; > > ret = request_resource(&priv->reg_res, &subdev1->resource[0]); > /* check ret */ > > subdev2->resource[0].start = 0x40; > subdev2->resource[0].end = 0x7f; > subdev2->resource[0].flags = 0; > > ret = request_resource(&priv->reg_res, &subdev2->resource[0]); > /* check ret */ > > This means that we end up with lots of per-device resource trees rooted > at the top-level-device driver, each effectively with their own separate > name spaces. Ah, I had not realized that this is possible. In this case, it certainly makes sense to have a separate space for each device that provides a register range. > Do we care about making them appear in procfs? I suspect at this stage > that would be wrong. If we care at all, they should probably be per- > top-level-device sysfs files - and we should have a function which sysfs > can use along with the root resource to create that output. If the resources have separate roots, I see no way how to make them all appear in the same procfs file. Having them represented in sysfs the way you describe sounds useful but not important. Arnd ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-09 16:26 ` Arnd Bergmann @ 2012-05-09 16:27 ` Russell King - ARM Linux 0 siblings, 0 replies; 41+ messages in thread From: Russell King - ARM Linux @ 2012-05-09 16:27 UTC (permalink / raw) To: linux-arm-kernel On Wed, May 09, 2012 at 04:26:27PM +0000, Arnd Bergmann wrote: > On Wednesday 09 May 2012, Russell King - ARM Linux wrote: > > Do we care about making them appear in procfs? I suspect at this stage > > that would be wrong. If we care at all, they should probably be per- > > top-level-device sysfs files - and we should have a function which sysfs > > can use along with the root resource to create that output. > > If the resources have separate roots, I see no way how to make them > all appear in the same procfs file. Having them represented in sysfs > the way you describe sounds useful but not important. I'd agree with that. I included that statement as food for thought if someone _did_ want this. ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-07 10:37 ` Mark Brown [not found] ` <201205071314.51886.arnd@arndb.de> @ 2012-05-07 18:57 ` Russell King - ARM Linux 2012-05-07 19:27 ` Mark Brown 1 sibling, 1 reply; 41+ messages in thread From: Russell King - ARM Linux @ 2012-05-07 18:57 UTC (permalink / raw) To: linux-arm-kernel On Mon, May 07, 2012 at 11:37:29AM +0100, Mark Brown wrote: > On Mon, May 07, 2012 at 10:14:03AM +0000, Arnd Bergmann wrote: > > On Monday 07 May 2012, Mark Brown wrote: > > > > Don't think I've got any examples with regions beginning at 0 but other > > > regions seem to not run into any problems with overlap. Note that all > > > these drivers do with the regions is use them to look up the base > > > register. > > > ISA devices start at 0. Using a definition for IORESOURCE_IO of "PCI > > port number for relatively large values, but either ISA or PCMCIA or > > an arbitrary MFD for relatively small values" is absolutely crazy. > > So what you're saying is that it's nothing to do with zero and just > about plain conflicts - there's nothing magic about zero (which is what > Russell seemed to be suggesting)? You've totally missed my point if that's what you think I was saying. The resource system as it stands handles two types of resources: 1. MMIO resources, of type IORESOURCE_MEM, registered against the iomem_resource root resource 2. PCI/ISA IO resources, of type IORESOURCE_IO, registered against the ioport_resource root resource The two root resources are hard coded into the BUSY-marking request/release APIs. If you don't have PCMCIA/ISA/PCI (and their derivatives) then the ioport resource ends up being zero sized, and therefore child resources fail to register against it (we want this behaviour to prevent ISA drivers working on platforms without ISA.) The problem comes if you start abusing IORESOURCE_IO - which has _always_ meant "this is a PCI/ISA IO resource", and then you have someone adding calls to request_region() etc. That will make stuff look at the ioport_resource root, and it won't work. If you _do_ have PCMCIA/ISA/PCI, then request_region() may well succeed. However, if you have two drivers playing this game, with overlapping start/end, _even_ if they're totally unrelated devices which would never clash, the APIs will fail just because of the _numeric_ overlap. The resource management subsystem really doesn't have any real knowledge of anything beyond "MMIO" and "PCI/ISA IO" resources. So, using IORESOURCE_IO for device-internal register spaces is an abuse of what this resource type was meant for, and leads to people getting confused about how to deal with this resource type (that's proven by Haojian's patch.) We really don't have a resource handling subsystem that sanely deals with device private register spaces. You can bodge around it by using request_resource() with specific parents, which themselves are disconnected from the above two root resources, but at the end of the day you're bodging around to try and get the resource stuff to do something it was never designed to. In any case, if the resource tree is disconnected from the main two parent resources, then the resource probably shouldn't have either IORESOURCE_IO nor IORESOURCE_MEM set in it. It's neither of those two resource types. ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-07 18:57 ` Russell King - ARM Linux @ 2012-05-07 19:27 ` Mark Brown 0 siblings, 0 replies; 41+ messages in thread From: Mark Brown @ 2012-05-07 19:27 UTC (permalink / raw) To: linux-arm-kernel On Mon, May 07, 2012 at 07:57:35PM +0100, Russell King - ARM Linux wrote: > On Mon, May 07, 2012 at 11:37:29AM +0100, Mark Brown wrote: > > So what you're saying is that it's nothing to do with zero and just > > about plain conflicts - there's nothing magic about zero (which is what > > Russell seemed to be suggesting)? > You've totally missed my point if that's what you think I was saying. I don't think I missed anything. I understood you were saying you think this is a terrible idea; I had thought you were saying that it was an even worse idea for a value of zero for some reason (ie, that that broke things further). > The resource system as it stands handles two types of resources: > 1. MMIO resources, of type IORESOURCE_MEM, registered against the > iomem_resource root resource > 2. PCI/ISA IO resources, of type IORESOURCE_IO, registered against the > ioport_resource root resource > The two root resources are hard coded into the BUSY-marking request/release > APIs. There's also IRQ, DMA and BUS resource types defined, though not handled by kernel/resource.c. > The problem comes if you start abusing IORESOURCE_IO - which has _always_ > meant "this is a PCI/ISA IO resource", and then you have someone adding > calls to request_region() etc. That will make stuff look at the > ioport_resource root, and it won't work. Yes, the request_region() usage is totally broken - I hadn't realised that the original patch was doing that. I had thought it was just peering at the start of the address range and using that as the base address for register I/O which is not wonderful but not actively broken on the client side. > In any case, if the resource tree is disconnected from the main two parent > resources, then the resource probably shouldn't have either IORESOURCE_IO > nor IORESOURCE_MEM set in it. It's neither of those two resource types. There is something of a shortage of other options as things stand... I guess using _BUS or something would at least eliminate the possibility of confusion with the management code would be a quick, short term improvement. -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 836 bytes Desc: Digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120507/bb26f1ae/attachment-0001.sig> ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/2] mfd: max8925: request resource region 2012-05-07 3:10 [PATCH 1/2] mfd: max8925: request resource region Haojian Zhuang 2012-05-07 3:10 ` [PATCH 2/2] ARM: mmp: add io head file Haojian Zhuang 2012-05-07 7:58 ` [PATCH 1/2] mfd: max8925: request resource region Russell King - ARM Linux @ 2012-05-07 8:12 ` Samuel Ortiz 2 siblings, 0 replies; 41+ messages in thread From: Samuel Ortiz @ 2012-05-07 8:12 UTC (permalink / raw) To: linux-arm-kernel Hi Haojian, On Mon, May 07, 2012 at 11:10:48AM +0800, Haojian Zhuang wrote: > If resource region isn't requested, component devices will fail to request > their resources. > > Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com> > --- > drivers/mfd/max8925-core.c | 4 ++++ > 1 files changed, 4 insertions(+), 0 deletions(-) Applied to my for-next branch, thanks. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ ^ permalink raw reply [flat|nested] 41+ messages in thread
end of thread, other threads:[~2012-05-09 16:30 UTC | newest] Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-05-07 3:10 [PATCH 1/2] mfd: max8925: request resource region Haojian Zhuang 2012-05-07 3:10 ` [PATCH 2/2] ARM: mmp: add io head file Haojian Zhuang 2012-05-07 9:23 ` Arnd Bergmann 2012-05-07 7:58 ` [PATCH 1/2] mfd: max8925: request resource region Russell King - ARM Linux 2012-05-07 8:18 ` Haojian Zhuang 2012-05-07 8:59 ` Russell King - ARM Linux 2012-05-07 9:01 ` Mark Brown 2012-05-07 9:08 ` Russell King - ARM Linux 2012-05-07 9:47 ` Mark Brown 2012-05-07 10:14 ` Arnd Bergmann 2012-05-07 10:23 ` Haojian Zhuang 2012-05-07 11:21 ` Arnd Bergmann 2012-05-07 11:29 ` Mark Brown [not found] ` <201205071319.48768.arnd@arndb.de> 2012-05-07 14:02 ` Samuel Ortiz 2012-05-07 14:15 ` Mark Brown 2012-05-07 15:15 ` Arnd Bergmann 2012-05-07 15:28 ` Mark Brown 2012-05-07 10:37 ` Mark Brown [not found] ` <201205071314.51886.arnd@arndb.de> 2012-05-07 14:06 ` Mark Brown 2012-05-07 15:09 ` Arnd Bergmann 2012-05-07 15:17 ` Mark Brown 2012-05-07 19:26 ` Arnd Bergmann 2012-05-07 19:58 ` Mark Brown 2012-05-08 8:17 ` Mark Brown 2012-05-08 14:44 ` Arnd Bergmann 2012-05-08 15:31 ` Mark Brown 2012-05-09 12:43 ` Arnd Bergmann 2012-05-09 14:13 ` Mark Brown 2012-05-09 14:19 ` Arnd Bergmann 2012-05-09 14:42 ` Mark Brown 2012-05-09 15:03 ` Arnd Bergmann 2012-05-09 15:28 ` Mark Brown 2012-05-09 16:27 ` Arnd Bergmann 2012-05-09 16:18 ` Russell King - ARM Linux 2012-05-09 16:30 ` Mark Brown 2012-05-09 16:07 ` Russell King - ARM Linux 2012-05-09 16:26 ` Arnd Bergmann 2012-05-09 16:27 ` Russell King - ARM Linux 2012-05-07 18:57 ` Russell King - ARM Linux 2012-05-07 19:27 ` Mark Brown 2012-05-07 8:12 ` Samuel Ortiz
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.