Re: [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support
Hi Samuel, On Wed, Jul 17, 2013 at 10:07:00PM +0100, Samuel Ortiz wrote: Hi Lorenzo, On Tue, Jul 16, 2013 at 05:05:42PM +0100, Lorenzo Pieralisi wrote: Hello, version v5 of VExpress SPC driver, please read on the changelog for major changes and explanations. The probing scheme is unchanged, since after trying the early platform devices approach it appeared that the end result was no better than the current one. The only clean solution relies either on changing how secondaries are brought up in the kernel (later than now) or enable early platform device registration through DT. Please check this thread for the related discussion: https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-June/036542.html The interface was adapted to regmap and again reverted to old driver for the following reasons: - Power down registers locking is hairy and requires arch spinlocks in the MCPM back end to work properly, normal spinlocks cannot be used - Regmap adds unnecessary code to manage SPC since it is just a bunch of registers used to control power management flags, the overhead is just not worth it (talking about power down registers, not the vexpress config interface) - The locking scheme behind regmap requires all registers in the map to be protected with the same lock, which is not exactly what we want here - Given the reasons above, adding a regmap interface buys us nothing from a driver readability and maintainability perspective (again just talking about the power interface, a few registers) because for the SPC it would simply not be used /drivers/mfd is probably not the right place for this code as it stands (but probably will be when the entire driver, with DVFS and config interface, is complete). Could you please elaborate on how will the SPC driver extend into an MFD driver? Reading through the thread I noticed Nico explained details properly, I was about to mention a possible solution to the directory issue but I am pretty sure that what he did will turn out for the best. Usually, or better, historically, these pieces of code that program PMICs lived in arch/arm/mach-* directories and that's something we could have done as well (create a static mapping and write some functions to peek and poke a few registers), but we thought that it was not the proper way to go. On top of that, the SPC is part of a component whose register space maps disparate functions (config interface for voltage, clocks, energy probes, frequency scaling and power states management) and basically that's the reason we struggled to partition it properly (with further complexity implied by the way requests - config and frequency scaling - have to be serialized). I hope the end result is reasonable, and overall I think it was a debate that was worth having. Thank you, Lorenzo ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support
On Tue, 2013-07-16 at 17:05 +0100, Lorenzo Pieralisi wrote: /drivers/mfd is probably not the right place for this code as it stands (but probably will be when the entire driver, with DVFS and config interface, is complete). Not that it really matters now, but my vexpress-sysreg rework will - most likely - leave only skeleton in the MFD (registering mfd_cells) and other stuff is going to be spread all around. Then I'm planning to move the remaining of the vexpress-specific initialization to drivers/platform/arm/vexpress.c, so maybe sticking vexpress-spc.c to this (non-existing yet) directory would be the right thing to do? Other than that: Acked-by: Pawel Moll pawel.m...@arm.com Thanks! Paweł ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support
On Wed, Jul 17, 2013 at 10:18:25AM +0100, Pawel Moll wrote: On Tue, 2013-07-16 at 17:05 +0100, Lorenzo Pieralisi wrote: /drivers/mfd is probably not the right place for this code as it stands (but probably will be when the entire driver, with DVFS and config interface, is complete). Not that it really matters now, but my vexpress-sysreg rework will - most likely - leave only skeleton in the MFD (registering mfd_cells) and other stuff is going to be spread all around. Then I'm planning to move the remaining of the vexpress-specific initialization to drivers/platform/arm/vexpress.c, so maybe sticking vexpress-spc.c to this (non-existing yet) directory would be the right thing to do? Done. I do not think there is a point in splitting the patch to create the dir and make infrastructure, so I squashed everything in the original patch. I have not added any maintainer for that dir/file, I guess it can wait till you finish the rework so that you can add yourself there. If that's all I need to change I do not even think that reposting is necessary. It does matter though, since it implies changes on who is in charge of ack/nack'ing this code, if it is no more an mfd matter. I will wait to check all interested/concerned parties opinions, that are always welcome. Other than that: Acked-by: Pawel Moll pawel.m...@arm.com Thank you !!! Lorenzo ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support
On Wed, 17 Jul 2013, Pawel Moll wrote: On Tue, 2013-07-16 at 17:05 +0100, Lorenzo Pieralisi wrote: /drivers/mfd is probably not the right place for this code as it stands (but probably will be when the entire driver, with DVFS and config interface, is complete). Not that it really matters now, but my vexpress-sysreg rework will - most likely - leave only skeleton in the MFD (registering mfd_cells) and other stuff is going to be spread all around. Then I'm planning to move the remaining of the vexpress-specific initialization to drivers/platform/arm/vexpress.c, so maybe sticking vexpress-spc.c to this (non-existing yet) directory would be the right thing to do? I don't like this idea. We worked hard to shrink platform specific directories such as arch/arm/mach-*/ as much as possible. Simply moving stuff to drivers/platform/arm/* doesn't make the situation any much better. If this is really miscelaneous code that really doesn't fit anywhere else, it should rather go into drivers/misc/ as a last resort. Nicolas ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support
On Wed, 2013-07-17 at 13:33 +0100, Nicolas Pitre wrote: If this is really miscelaneous code that really doesn't fit anywhere else, it should rather go into drivers/misc/ as a last resort. Interestingly enough drivers/misc was my first choice for all the vexpress stuff, but it wasn't received well... Anyway, the SPC driver as it is now seem to be a power management system driver. Maybe a relevant directory would be in place? Wouldn't PSCI belong there as well? (there are two psci.c files in arch/arm and arch/arm64, surprisingly similar ones ;-) The bottom line is: today it is not an MFD driver. Paweł ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support
On Wed, 17 Jul 2013, Pawel Moll wrote: On Wed, 2013-07-17 at 13:33 +0100, Nicolas Pitre wrote: If this is really miscelaneous code that really doesn't fit anywhere else, it should rather go into drivers/misc/ as a last resort. Interestingly enough drivers/misc was my first choice for all the vexpress stuff, but it wasn't received well... Anyway, the SPC driver as it is now seem to be a power management system driver. Maybe a relevant directory would be in place? Wouldn't PSCI belong there as well? (there are two psci.c files in arch/arm and arch/arm64, surprisingly similar ones ;-) The bottom line is: today it is not an MFD driver. But we know it will, right? So better save some churn by storing the initial code where it would end up anyway once complete. Nicolas ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support
On Wed, 2013-07-17 at 15:16 +0100, Nicolas Pitre wrote: On Wed, 17 Jul 2013, Pawel Moll wrote: On Wed, 2013-07-17 at 13:33 +0100, Nicolas Pitre wrote: If this is really miscelaneous code that really doesn't fit anywhere else, it should rather go into drivers/misc/ as a last resort. Interestingly enough drivers/misc was my first choice for all the vexpress stuff, but it wasn't received well... Anyway, the SPC driver as it is now seem to be a power management system driver. Maybe a relevant directory would be in place? Wouldn't PSCI belong there as well? (there are two psci.c files in arch/arm and arch/arm64, surprisingly similar ones ;-) The bottom line is: today it is not an MFD driver. But we know it will, right? So better save some churn by storing the initial code where it would end up anyway once complete. Not in that form, no. The code living in mfd will just register mfd_cells while functional parts are going to live elsewhere. This is how I understand what Samuel asked me to do and that's what is happening to vexpress-sysreg now. Paweł ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support
On Wed, 17 Jul 2013, Pawel Moll wrote: On Wed, 2013-07-17 at 15:16 +0100, Nicolas Pitre wrote: On Wed, 17 Jul 2013, Pawel Moll wrote: On Wed, 2013-07-17 at 13:33 +0100, Nicolas Pitre wrote: If this is really miscelaneous code that really doesn't fit anywhere else, it should rather go into drivers/misc/ as a last resort. Interestingly enough drivers/misc was my first choice for all the vexpress stuff, but it wasn't received well... Anyway, the SPC driver as it is now seem to be a power management system driver. Maybe a relevant directory would be in place? Wouldn't PSCI belong there as well? (there are two psci.c files in arch/arm and arch/arm64, surprisingly similar ones ;-) The bottom line is: today it is not an MFD driver. But we know it will, right? So better save some churn by storing the initial code where it would end up anyway once complete. Not in that form, no. The code living in mfd will just register mfd_cells while functional parts are going to live elsewhere. This is how I understand what Samuel asked me to do and that's what is happening to vexpress-sysreg now. A drivers/pm/ or drivers/power/ could be created, but that implies sort of a more defined subsystem with a common API and the SPC code doesn't fit that as it is only providing services to actual drivers on top of it. The sanest location at this point might simply be drivers/platform/vexpress_spc.c or drivers/platform/vexpress/spc.c depending on whether or not more such driver glue is expected in the vexpress future. No point putting arm in the path, especially if this is later reused on arm64. Nicolas ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support
On Wed, Jul 17, 2013 at 11:57:55AM -0400, Nicolas Pitre wrote: The sanest location at this point might simply be drivers/platform/vexpress_spc.c or drivers/platform/vexpress/spc.c depending on whether or not more such driver glue is expected in the vexpress future. No point putting arm in the path, especially if this is later reused on arm64. You wouldn't be making that argument if it were arch/arm64 and arch/arm32 - you'd probably be arguing that arm made perfect sense. Don't get too hung up on names please, it's really not worth the time and effort being soo pedantic, and being soo pedantic leads to pointless churn when someone comes along and wants to pedantically change the names because it no longer matches the users. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support
On Wed, 17 Jul 2013, Russell King - ARM Linux wrote: On Wed, Jul 17, 2013 at 11:57:55AM -0400, Nicolas Pitre wrote: The sanest location at this point might simply be drivers/platform/vexpress_spc.c or drivers/platform/vexpress/spc.c depending on whether or not more such driver glue is expected in the vexpress future. No point putting arm in the path, especially if this is later reused on arm64. You wouldn't be making that argument if it were arch/arm64 and arch/arm32 - you'd probably be arguing that arm made perfect sense. Well... in a sense: yes. But in the end, having per arch directories under drivers/ is silly. We already have per arch directories under arch/already. Don't get too hung up on names please, it's really not worth the time and effort being soo pedantic, and being soo pedantic leads to pointless churn when someone comes along and wants to pedantically change the names because it no longer matches the users. At this point I don't really care about the name. I just want the damn thing merged upstream. But after several iterations to either fit one or another maintainers taste, each rework ends up in that maintainer saying: Now that you've reworked the code, I still don't like it since this no longer fits in my subsystem tree. In fact what we'd need at this point is drivers/code_that_no_subsystem_maintainers_wants/. This is becoming overly ridiculous. Nicolas ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support
Hi Lorenzo, On Tue, Jul 16, 2013 at 05:05:42PM +0100, Lorenzo Pieralisi wrote: Hello, version v5 of VExpress SPC driver, please read on the changelog for major changes and explanations. The probing scheme is unchanged, since after trying the early platform devices approach it appeared that the end result was no better than the current one. The only clean solution relies either on changing how secondaries are brought up in the kernel (later than now) or enable early platform device registration through DT. Please check this thread for the related discussion: https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-June/036542.html The interface was adapted to regmap and again reverted to old driver for the following reasons: - Power down registers locking is hairy and requires arch spinlocks in the MCPM back end to work properly, normal spinlocks cannot be used - Regmap adds unnecessary code to manage SPC since it is just a bunch of registers used to control power management flags, the overhead is just not worth it (talking about power down registers, not the vexpress config interface) - The locking scheme behind regmap requires all registers in the map to be protected with the same lock, which is not exactly what we want here - Given the reasons above, adding a regmap interface buys us nothing from a driver readability and maintainability perspective (again just talking about the power interface, a few registers) because for the SPC it would simply not be used /drivers/mfd is probably not the right place for this code as it stands (but probably will be when the entire driver, with DVFS and config interface, is complete). Could you please elaborate on how will the SPC driver extend into an MFD driver? Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support
Hi Pawel, On Wed, Jul 17, 2013 at 03:20:11PM +0100, Pawel Moll wrote: On Wed, 2013-07-17 at 15:16 +0100, Nicolas Pitre wrote: On Wed, 17 Jul 2013, Pawel Moll wrote: On Wed, 2013-07-17 at 13:33 +0100, Nicolas Pitre wrote: If this is really miscelaneous code that really doesn't fit anywhere else, it should rather go into drivers/misc/ as a last resort. Interestingly enough drivers/misc was my first choice for all the vexpress stuff, but it wasn't received well... Anyway, the SPC driver as it is now seem to be a power management system driver. Maybe a relevant directory would be in place? Wouldn't PSCI belong there as well? (there are two psci.c files in arch/arm and arch/arm64, surprisingly similar ones ;-) The bottom line is: today it is not an MFD driver. But we know it will, right? So better save some churn by storing the initial code where it would end up anyway once complete. Not in that form, no. The code living in mfd will just register mfd_cells while functional parts are going to live elsewhere. This is how I understand what Samuel asked me to do and that's what is happening to vexpress-sysreg now. Very good, I'll happily apply such changes. If I understand the IP correctly, the SPC driver will stay as a set of runtime APIs for controlling the SPC features. If that's the case, then misc sounds like a more appropriate place for this driver. drivers/power/ really is for power supplies and charging drivers. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support
Hi Nicolas, On Wed, Jul 17, 2013 at 02:29:02PM -0400, Nicolas Pitre wrote: On Wed, 17 Jul 2013, Russell King - ARM Linux wrote: On Wed, Jul 17, 2013 at 11:57:55AM -0400, Nicolas Pitre wrote: The sanest location at this point might simply be drivers/platform/vexpress_spc.c or drivers/platform/vexpress/spc.c depending on whether or not more such driver glue is expected in the vexpress future. No point putting arm in the path, especially if this is later reused on arm64. You wouldn't be making that argument if it were arch/arm64 and arch/arm32 - you'd probably be arguing that arm made perfect sense. Well... in a sense: yes. But in the end, having per arch directories under drivers/ is silly. We already have per arch directories under arch/already. Don't get too hung up on names please, it's really not worth the time and effort being soo pedantic, and being soo pedantic leads to pointless churn when someone comes along and wants to pedantically change the names because it no longer matches the users. At this point I don't really care about the name. I just want the damn thing merged upstream. But after several iterations to either fit one or another maintainers taste, each rework ends up in that maintainer saying: Now that you've reworked the code, I still don't like it since this no longer fits in my subsystem tree. FWIW, we asked Pawel to rework the sysreg and config parts of the vexpress driver, make it an actual MFD driver, and spread the remaining bits of the code into their respective subsystems. I don't think this is an eccentric requirement. In fact what we'd need at this point is drivers/code_that_no_subsystem_maintainers_wants/. Which is what some people think drivers/mfd/ is... I don't mind merging Lorenzo's SPC driver as it is if he can explain to me how it will eventually evolve into an actual MFD driver. If that's not the case, I don't see how I could justify merging it through the MFD tree. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support
On Wed, 17 Jul 2013, Samuel Ortiz wrote: On Wed, Jul 17, 2013 at 02:29:02PM -0400, Nicolas Pitre wrote: At this point I don't really care about the name. I just want the damn thing merged upstream. But after several iterations to either fit one or another maintainers taste, each rework ends up in that maintainer saying: Now that you've reworked the code, I still don't like it since this no longer fits in my subsystem tree. FWIW, we asked Pawel to rework the sysreg and config parts of the vexpress driver, make it an actual MFD driver, and spread the remaining bits of the code into their respective subsystems. I don't think this is an eccentric requirement. I agree. However the code that Lorenzo just posted can't be deprived of any more sysreg/config parts. They are simply nonexistent. Even the larger code you reviewed before is completely useless without _additional_ drivers to go on top which are indeed waiting after this code to be merged before they are submitted to their respective subsystems. And those additional drivers are way more interesting than this dumb register access arbitrator. Because this is fundamentally the only thing it does. In fact what we'd need at this point is drivers/code_that_no_subsystem_maintainers_wants/. Which is what some people think drivers/mfd/ is... Does mfd still stand for Multi Function Device? I don't mind merging Lorenzo's SPC driver as it is if he can explain to me how it will eventually evolve into an actual MFD driver. If that's not the case, I don't see how I could justify merging it through the MFD tree. Maybe the misunderstanding is about what actually is a MFD driver. Given your persisting reluctance, I may only conclude that this is indeed not a MFD driver after all. So I'll follow existing precedents in the kernel and move Lorenzo's code to drivers/platform/vexpress/. Nicolas ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [RFC PATCH v5 0/1] drivers: mfd: Versatile Express SPC support
On Tue, 16 Jul 2013, Lorenzo Pieralisi wrote: Hello, version v5 of VExpress SPC driver, please read on the changelog for major changes and explanations. The probing scheme is unchanged, since after trying the early platform devices approach it appeared that the end result was no better than the current one. The only clean solution relies either on changing how secondaries are brought up in the kernel (later than now) or enable early platform device registration through DT. Please check this thread for the related discussion: https://lists.ozlabs.org/pipermail/devicetree-discuss/2013-June/036542.html The interface was adapted to regmap and again reverted to old driver for the following reasons: - Power down registers locking is hairy and requires arch spinlocks in the MCPM back end to work properly, normal spinlocks cannot be used - Regmap adds unnecessary code to manage SPC since it is just a bunch of registers used to control power management flags, the overhead is just not worth it (talking about power down registers, not the vexpress config interface) - The locking scheme behind regmap requires all registers in the map to be protected with the same lock, which is not exactly what we want here - Given the reasons above, adding a regmap interface buys us nothing from a driver readability and maintainability perspective (again just talking about the power interface, a few registers) because for the SPC it would simply not be used /drivers/mfd is probably not the right place for this code as it stands (but probably will be when the entire driver, with DVFS and config interface, is complete). Thank you for the review in advance, Lorenzo I've integrated this patch in my MCPM backend for TC2 patch series. ACKs from concerned/interested people would be appreciated so I could send this for ARM-SOC inclusion right away. Nicolas ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
