| Message ID | 1350911580-20307-1-git-send-email-sourav.poddar@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Sourav, On Mon, Oct 22, 2012 at 06:43:00PM +0530, Sourav Poddar wrote: > Adapt keypad to use pinctrl framework. > > Tested on omap4430 sdp with 3.7-rc1 kernel. I do not see anything in the driver that would directly use pinctrl. Is there a better place to select default pin configuration; maybe when instantiating platform device? Thanks. > > Cc: Felipe Balbi <balbi@ti.com> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Signed-off-by: Sourav Poddar <sourav.poddar@ti.com> > --- > v1->v2 > - Added "PROBE_DEFER" check > drivers/input/keyboard/omap4-keypad.c | 11 +++++++++++ > 1 files changed, 11 insertions(+), 0 deletions(-) > > diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c > index c05f98c..502b832 100644 > --- a/drivers/input/keyboard/omap4-keypad.c > +++ b/drivers/input/keyboard/omap4-keypad.c > @@ -31,6 +31,7 @@ > #include <linux/input.h> > #include <linux/slab.h> > #include <linux/pm_runtime.h> > +#include <linux/pinctrl/consumer.h> > > #include <linux/platform_data/omap4-keypad.h> > > @@ -76,6 +77,7 @@ enum { > > struct omap4_keypad { > struct input_dev *input; > + struct pinctrl *pins; > > void __iomem *base; > unsigned int irq; > @@ -298,6 +300,15 @@ static int __devinit omap4_keypad_probe(struct platform_device *pdev) > goto err_release_mem; > } > > + keypad_data->pins = devm_pinctrl_get_select_default(&pdev->dev); > + if (IS_ERR(keypad_data->pins)) { > + if (PTR_ERR(keypad_data->pins) == -EPROBE_DEFER) > + return -EPROBE_DEFER; > + > + dev_warn(&pdev->dev, "did not get pins for keypad error: %li\n", > + PTR_ERR(keypad_data->pins)); > + keypad_data->pins = NULL; > + } > > /* > * Enable clocks for the keypad module so that we can read > -- > 1.7.1 >
On Mon, Oct 22, 2012 at 5:50 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > Hi Sourav, > > On Mon, Oct 22, 2012 at 06:43:00PM +0530, Sourav Poddar wrote: >> Adapt keypad to use pinctrl framework. >> >> Tested on omap4430 sdp with 3.7-rc1 kernel. > > I do not see anything in the driver that would directly use pinctrl. Is > there a better place to select default pin configuration; maybe when > instantiating platform device? The option is to use pinctrl hogs. Then the pins will be taken, muxed and configured by the pin controller itself. Another option (not implemented) is to use bus notifiers. (I wrote about this in some other thread but can't find it now.) Each approach above come with its own set of problems. If the driver need to handle multiple states like default/idle/sleep it is IMO better to put the handling into the driver, so if that is what is going to happen also to this driver it could be a good idea to actually implement that code upfront and include in this submission so as to show that this driver is really going to exercise its pins. But it's also a question of conformity: if other drivers in the system is using different states and this is the only one using a single "default" state, then it doesn't make sense to have just one driver get its pins using hogs, it's just inconsistent. So Sourav, please tell us a bit about your plans for this and other drivers! Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Dimitry, On 10/22/2012 05:50 PM, Dmitry Torokhov wrote: > Hi Sourav, > > On Mon, Oct 22, 2012 at 06:43:00PM +0530, Sourav Poddar wrote: >> Adapt keypad to use pinctrl framework. >> >> Tested on omap4430 sdp with 3.7-rc1 kernel. > > I do not see anything in the driver that would directly use pinctrl. Is > there a better place to select default pin configuration; maybe when > instantiating platform device? Why? The devm_pinctrl_get_select_default is using the pinctrl. That's why it is named "get_select_default" and not "get" only. This API ensure that the pin will be set to the default state, and this is all we need to do during the probe. There is no point to change the mux if the driver is not probed, because potentially the pin can be use be another driver. So probe is the right place to do that for my point of view. Moreover with DT we don't have that board static config like we had before to do that kind of pin mux init. But, maybe I'm missing your point. Regards, Benoit > > Thanks. > >> >> Cc: Felipe Balbi <balbi@ti.com> >> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> >> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com> >> --- >> v1->v2 >> - Added "PROBE_DEFER" check >> drivers/input/keyboard/omap4-keypad.c | 11 +++++++++++ >> 1 files changed, 11 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c >> index c05f98c..502b832 100644 >> --- a/drivers/input/keyboard/omap4-keypad.c >> +++ b/drivers/input/keyboard/omap4-keypad.c >> @@ -31,6 +31,7 @@ >> #include <linux/input.h> >> #include <linux/slab.h> >> #include <linux/pm_runtime.h> >> +#include <linux/pinctrl/consumer.h> >> >> #include <linux/platform_data/omap4-keypad.h> >> >> @@ -76,6 +77,7 @@ enum { >> >> struct omap4_keypad { >> struct input_dev *input; >> + struct pinctrl *pins; >> >> void __iomem *base; >> unsigned int irq; >> @@ -298,6 +300,15 @@ static int __devinit omap4_keypad_probe(struct platform_device *pdev) >> goto err_release_mem; >> } >> >> + keypad_data->pins = devm_pinctrl_get_select_default(&pdev->dev); >> + if (IS_ERR(keypad_data->pins)) { >> + if (PTR_ERR(keypad_data->pins) == -EPROBE_DEFER) >> + return -EPROBE_DEFER; >> + >> + dev_warn(&pdev->dev, "did not get pins for keypad error: %li\n", >> + PTR_ERR(keypad_data->pins)); >> + keypad_data->pins = NULL; >> + } >> >> /* >> * Enable clocks for the keypad module so that we can read >> -- >> 1.7.1 >> > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Linus, On 10/23/2012 11:13 AM, Linus Walleij wrote: > On Mon, Oct 22, 2012 at 5:50 PM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: >> Hi Sourav, >> >> On Mon, Oct 22, 2012 at 06:43:00PM +0530, Sourav Poddar wrote: >>> Adapt keypad to use pinctrl framework. >>> >>> Tested on omap4430 sdp with 3.7-rc1 kernel. >> >> I do not see anything in the driver that would directly use pinctrl. Is >> there a better place to select default pin configuration; maybe when >> instantiating platform device? > > The option is to use pinctrl hogs. Then the pins will be taken, > muxed and configured by the pin controller itself. > > Another option (not implemented) is to use bus notifiers. > > (I wrote about this in some other thread but can't find it now.) > > Each approach above come with its own set of problems. > > If the driver need to handle multiple states like default/idle/sleep > it is IMO better to put the handling into the driver, so if that > is what is going to happen also to this driver it could be a good > idea to actually implement that code upfront and include in > this submission so as to show that this driver is really going > to exercise its pins. > > But it's also a question of conformity: if other drivers in the > system is using different states and this is the only one > using a single "default" state, then it doesn't make sense > to have just one driver get its pins using hogs, it's just > inconsistent. > > So Sourav, please tell us a bit about your plans for this > and other drivers! Yeah, this idea is to handle pinctrl from all the drivers, and potentially change the mode during suspend when it is relevant. Regards, Benoit -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Tue, Oct 23, 2012 at 12:04:01PM +0200, Linus Walleij wrote: > On Tue, Oct 23, 2012 at 11:35 AM, Benoit Cousson <b-cousson@ti.com> wrote: > > On 10/23/2012 11:13 AM, Linus Walleij wrote: > > >> So Sourav, please tell us a bit about your plans for this > >> and other drivers! > > > > Yeah, this idea is to handle pinctrl from all the drivers, and > > potentially change the mode during suspend when it is relevant. > > I'm leaning toward the same approach for ux500. > > But it appears that shmobile prefer to get all resources using > bus notifiers. > > So we need to form some kind of consensus ... or live with > the fact that different systems do it different ways. Which will > explode the day we need to use a driver on two systems, > each using the other approach :-) I much prefer having drivers explicitly manage all their resources, which would mean that pinctrl calls need to be done on probe() and, if necessary, during suspend()/resume(). Using bus notifiers for that is quite a hack IMHO.
On Tue, Oct 23, 2012 at 11:35 AM, Benoit Cousson <b-cousson@ti.com> wrote: > On 10/23/2012 11:13 AM, Linus Walleij wrote: >> So Sourav, please tell us a bit about your plans for this >> and other drivers! > > Yeah, this idea is to handle pinctrl from all the drivers, and > potentially change the mode during suspend when it is relevant. I'm leaning toward the same approach for ux500. But it appears that shmobile prefer to get all resources using bus notifiers. So we need to form some kind of consensus ... or live with the fact that different systems do it different ways. Which will explode the day we need to use a driver on two systems, each using the other approach :-) Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 23 Oct 2012 13:03:33 +0300, Felipe Balbi wrote: > > But it appears that shmobile prefer to get all resources using > > bus notifiers. > > > > So we need to form some kind of consensus ... or live with > > the fact that different systems do it different ways. Which will > > explode the day we need to use a driver on two systems, > > each using the other approach :-) > > I much prefer having drivers explicitly manage all their resources, > which would mean that pinctrl calls need to be done on probe() and, if > necessary, during suspend()/resume(). > > Using bus notifiers for that is quite a hack IMHO. Agreed. Just like drivers do their ioremap, request_irq and others, they should also request their pin resources using the pinctrl API. Hiding this behind a bus notifier is not nice. Best regards, Thomas
Hi, On Tue, Oct 23, 2012 at 12:29:28PM +0200, Linus Walleij wrote: > On Tue, Oct 23, 2012 at 12:23 PM, Thomas Petazzoni > <thomas.petazzoni@free-electrons.com> wrote: > > > > On Tue, 23 Oct 2012 13:03:33 +0300, Felipe Balbi wrote: > > > >> > But it appears that shmobile prefer to get all resources using > >> > bus notifiers. > >> > > >> > So we need to form some kind of consensus ... or live with > >> > the fact that different systems do it different ways. Which will > >> > explode the day we need to use a driver on two systems, > >> > each using the other approach :-) > >> > >> I much prefer having drivers explicitly manage all their resources, > >> which would mean that pinctrl calls need to be done on probe() and, if > >> necessary, during suspend()/resume(). > >> > >> Using bus notifiers for that is quite a hack IMHO. > > > > Agreed. Just like drivers do their ioremap, request_irq and others, > > they should also request their pin resources using the pinctrl API. > > Hiding this behind a bus notifier is not nice. > > So the biggest implementation of the notifier approach to resource > handling is the SH clock thing: > drivers/base/power/clock_ops.c that's different right ? It's just creating the list of clocks, device drivers still have to call pm_clk_add(). That's ok, I guess, otherwise all struct device would allocate memory which hardly ever used (so far).
On Tue, Oct 23, 2012 at 12:23 PM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > > On Tue, 23 Oct 2012 13:03:33 +0300, Felipe Balbi wrote: > >> > But it appears that shmobile prefer to get all resources using >> > bus notifiers. >> > >> > So we need to form some kind of consensus ... or live with >> > the fact that different systems do it different ways. Which will >> > explode the day we need to use a driver on two systems, >> > each using the other approach :-) >> >> I much prefer having drivers explicitly manage all their resources, >> which would mean that pinctrl calls need to be done on probe() and, if >> necessary, during suspend()/resume(). >> >> Using bus notifiers for that is quite a hack IMHO. > > Agreed. Just like drivers do their ioremap, request_irq and others, > they should also request their pin resources using the pinctrl API. > Hiding this behind a bus notifier is not nice. So the biggest implementation of the notifier approach to resource handling is the SH clock thing: drivers/base/power/clock_ops.c It's made to be generic but AFAICT only SH is using this. So according to that paradigm most device resources should be handled that way if I understand correctly the basic idea. So let's get Rafael, Paul and Magnus in here to beat us up a bit :-) Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Tue, Oct 23, 2012 at 12:45:33PM +0200, Linus Walleij wrote: > On Tue, Oct 23, 2012 at 12:29 PM, Felipe Balbi <balbi@ti.com> wrote: > > On Tue, Oct 23, 2012 at 12:29:28PM +0200, Linus Walleij wrote: > > >> So the biggest implementation of the notifier approach to resource > >> handling is the SH clock thing: > >> drivers/base/power/clock_ops.c > > > > that's different right ? It's just creating the list of clocks, device > > drivers still have to call pm_clk_add(). > > > > That's ok, I guess, otherwise all struct device would allocate memory > > which hardly ever used (so far). > > Hm so I have had this idea of runtime PM core helping out > with pins, so I could add something like > > pm_pins_fetch() > pm_pins_default() > pm_pins_idle() > pm_pins_sleep() > > So if one is using the pin states defined in <linux/pinctrl/pinctrl-state.h> > then the PM core can help out in keeping track of the pins > and states, and the driver will just tell the PM core what > to do and when. > > Would this fit the bill for everyone's code consolidation needs? > It would sure work for us... > > It however require that no custom states are used and that we > keep to the state semantics I just happen to think is most > common. From a quick read, it looks ok. I guess problems will only how up when we actually end up with a silicon errata or something similar which mandates that we change pin's state at a particular location. Not sure if we have those yet.
On Tue, Oct 23, 2012 at 12:29 PM, Felipe Balbi <balbi@ti.com> wrote: > On Tue, Oct 23, 2012 at 12:29:28PM +0200, Linus Walleij wrote: >> So the biggest implementation of the notifier approach to resource >> handling is the SH clock thing: >> drivers/base/power/clock_ops.c > > that's different right ? It's just creating the list of clocks, device > drivers still have to call pm_clk_add(). > > That's ok, I guess, otherwise all struct device would allocate memory > which hardly ever used (so far). Hm so I have had this idea of runtime PM core helping out with pins, so I could add something like pm_pins_fetch() pm_pins_default() pm_pins_idle() pm_pins_sleep() So if one is using the pin states defined in <linux/pinctrl/pinctrl-state.h> then the PM core can help out in keeping track of the pins and states, and the driver will just tell the PM core what to do and when. Would this fit the bill for everyone's code consolidation needs? It would sure work for us... It however require that no custom states are used and that we keep to the state semantics I just happen to think is most common. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 23 Oct 2012 12:45:33 +0200, Linus Walleij wrote: > Hm so I have had this idea of runtime PM core helping out > with pins, so I could add something like > > pm_pins_fetch() > pm_pins_default() > pm_pins_idle() > pm_pins_sleep() > > So if one is using the pin states defined in > <linux/pinctrl/pinctrl-state.h> then the PM core can help out in > keeping track of the pins and states, and the driver will just tell > the PM core what to do and when. > > Would this fit the bill for everyone's code consolidation needs? > It would sure work for us... That surely would work but is kind of non-obvious when reading a driver's code: that's the problem with bus notifier, they do things a bit "behind your back" without you noticing. Having the driver request its own pinctrl state, and switch between states upon suspend/resume is a lot more explicit, IMO. Thomas
On 10/23/2012 12:03 AM, Felipe Balbi wrote: > Hi, > > I much prefer having drivers explicitly manage all their resources, > which would mean that pinctrl calls need to be done on probe() and, if > necessary, during suspend()/resume(). Per-driver resource management is certainly convenient when you are dealing with a single system, but it becomes difficult to maintain for drivers that are shared among many platforms. The industry trend for many years has been consolidation around a single programming model per class of device. For example, SDHCI, EHCI, ATA. This trend will only accelerate, as the cost of developing controller IP and associated drivers increases. Such drivers need to be as platform-agnostic as possible. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
HI, On Tue, Oct 23, 2012 at 07:02:09AM -1000, Mitch Bradley wrote: > On 10/23/2012 12:03 AM, Felipe Balbi wrote: > > Hi, > > > > I much prefer having drivers explicitly manage all their resources, > > which would mean that pinctrl calls need to be done on probe() and, if > > necessary, during suspend()/resume(). > > > Per-driver resource management is certainly convenient when you are > dealing with a single system, but it becomes difficult to maintain for > drivers that are shared among many platforms. why ? look at drivers/usb/dwc3/, we're using that on OMAP, exynos, PCIe and a couple of different FPGA implementations inside TI. Not to mention what other licensees of that IP core might have internally. So far no problesm with resources at all. We have frameworks exactly to hide the differences. > The industry trend for many years has been consolidation around a single > programming model per class of device. For example, SDHCI, EHCI, ATA. > This trend will only accelerate, as the cost of developing controller IP > and associated drivers increases. Such drivers need to be as > platform-agnostic as possible. that's why we have pinctrl framework to abstract the details about pin muxing.
Perhaps I misunderstood what you were suggesting. I thought that, when you said "explicitly manage all their resources", you meant that the driver should know the platform-specific details about clocks and power domains. That is one possible interpretation of the word "explicit". Now I see that you meant that the driver should explicitly call abstracted functions. On 10/23/2012 7:20 AM, Felipe Balbi wrote: > HI, > > On Tue, Oct 23, 2012 at 07:02:09AM -1000, Mitch Bradley wrote: >> On 10/23/2012 12:03 AM, Felipe Balbi wrote: >>> Hi, >>> >>> I much prefer having drivers explicitly manage all their resources, >>> which would mean that pinctrl calls need to be done on probe() and, if >>> necessary, during suspend()/resume(). >> >> >> Per-driver resource management is certainly convenient when you are >> dealing with a single system, but it becomes difficult to maintain for >> drivers that are shared among many platforms. > > why ? look at drivers/usb/dwc3/, we're using that on OMAP, exynos, PCIe > and a couple of different FPGA implementations inside TI. Not to mention > what other licensees of that IP core might have internally. > > So far no problesm with resources at all. > > We have frameworks exactly to hide the differences. > >> The industry trend for many years has been consolidation around a single >> programming model per class of device. For example, SDHCI, EHCI, ATA. >> This trend will only accelerate, as the cost of developing controller IP >> and associated drivers increases. Such drivers need to be as >> platform-agnostic as possible. > > that's why we have pinctrl framework to abstract the details about pin > muxing. > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, (please don't top-post) On Tue, Oct 23, 2012 at 07:51:22AM -1000, Mitch Bradley wrote: > Perhaps I misunderstood what you were suggesting. I thought that, when > you said "explicitly manage all their resources", you meant that the > driver should know the platform-specific details about clocks and power > domains. That is one possible interpretation of the word "explicit". we had two suggestions in the thread: 1) handle it in driver source code (explict) 2) handle at bus notifiers level (hidden) archives would've helped you clear up that confusion ;-)
On Tue, Oct 23, 2012 at 11:18:12AM +0200, Benoit Cousson wrote: > Hi Dimitry, > > On 10/22/2012 05:50 PM, Dmitry Torokhov wrote: > > Hi Sourav, > > > > On Mon, Oct 22, 2012 at 06:43:00PM +0530, Sourav Poddar wrote: > >> Adapt keypad to use pinctrl framework. > >> > >> Tested on omap4430 sdp with 3.7-rc1 kernel. > > > > I do not see anything in the driver that would directly use pinctrl. Is > > there a better place to select default pin configuration; maybe when > > instantiating platform device? > > Why? > The devm_pinctrl_get_select_default is using the pinctrl. No, I guess we ihave different understanding of what "directly use" means. This particular driver does directly use interrupts: it requests it and performs some actions when interrupt arrives. Same goes for IO memory - it gets requested, then we access it. With pinctrl we do not do anything - we just ask another layer to configure it and that is it. I have seen just in a few days 3 or 4 drivers having exactly the same change - call to devm_pinctrl_get_select_default(), and I guess I will receive the same patches for the rest of input drivers shortly. This suggests that the operation is done at the wrong level. Do the pin configuration as you parse DT data, the same way you set up i2c devices registers in of_i2c.c, and leave the individual drivers that do not care about specifics alone. > That's why it is named "get_select_default" and not "get" only. > This API ensure that the pin will be set to the default state, and this > is all we need to do during the probe. Why during the probe and not by default? Realistically, the driver will be loaded as long as there is a matching device and pins will need to be configured. > > There is no point to change the mux if the driver is not probed, because > potentially the pin can be use be another driver. What other driver would use it? Who would chose what driver to load? Thanks.
Hi, On Tue, Oct 23, 2012 at 01:02:49PM -0700, Dmitry Torokhov wrote: > On Tue, Oct 23, 2012 at 11:18:12AM +0200, Benoit Cousson wrote: > > Hi Dimitry, > > > > On 10/22/2012 05:50 PM, Dmitry Torokhov wrote: > > > Hi Sourav, > > > > > > On Mon, Oct 22, 2012 at 06:43:00PM +0530, Sourav Poddar wrote: > > >> Adapt keypad to use pinctrl framework. > > >> > > >> Tested on omap4430 sdp with 3.7-rc1 kernel. > > > > > > I do not see anything in the driver that would directly use pinctrl. Is > > > there a better place to select default pin configuration; maybe when > > > instantiating platform device? > > > > Why? > > The devm_pinctrl_get_select_default is using the pinctrl. > > No, I guess we ihave different understanding of what "directly use" means. > This particular driver does directly use interrupts: it requests it and > performs some actions when interrupt arrives. Same goes for IO memory - > it gets requested, then we access it. With pinctrl we do not do anything > - we just ask another layer to configure it and that is it. this is true for almost anything we do: - we ask another layer to allocate memory for us - we ask another layer to call our ISR once the IRQ line is asserted - we ask another layer to handle the input events we just received - we ask another layer to transfer data through DMA for us - we ask another layer to turn regulators on and off. and so on. This is just how abstractions work, we group common parts in a framework so that users don't need to know the details, but still need to tell the framework when to fiddle with those resources. > I have seen just in a few days 3 or 4 drivers having exactly the same > change - call to devm_pinctrl_get_select_default(), and I guess I will > receive the same patches for the rest of input drivers shortly. > This suggests that the operation is done at the wrong level. Do the > pin configuration as you parse DT data, the same way you set up i2c > devices registers in of_i2c.c, and leave the individual drivers that do > not care about specifics alone. Makes no sense to hide that from drivers. The idea here is that driver should know when it needs its pins to muxed correctly. 95% of the time it will be done during probe() but then again, so what ? doing that when parsing DT, or on bus notifiers is just plain wrong. Drivers should be required to handle all of their resources. > > That's why it is named "get_select_default" and not "get" only. > > This API ensure that the pin will be set to the default state, and this > > is all we need to do during the probe. > > Why during the probe and not by default? Realistically, the driver will > be loaded as long as there is a matching device and pins will need to be > configured. likewise memory will be allocated when matching happens, IRQs will be allocated, regulators will be turned on. So why don't we do all that by default ? Because it is wrong. > > There is no point to change the mux if the driver is not probed, because > > potentially the pin can be use be another driver. > > What other driver would use it? Who would chose what driver to load? Well, you _do_ know that on a SoC we have a limited amount of pins right ? Considering the amont of features which are packed inside a single die, it's not farfetched to assume we will have a lot less pins then we actually need, so we need muxers behind each pin in order to choose which functionality we want. If it happens that keypad's pins are shared with another IP (e.g. GPIO), we need to give the final user (a OEM/ODM) the choice of using those pins as either keypad or GPIOs, thus the need for pinctrl framework and the calls in the drivers.
On Tue, Oct 23, 2012 at 10:02 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > I have seen just in a few days 3 or 4 drivers having exactly the same > change - call to devm_pinctrl_get_select_default(), and I guess I will > receive the same patches for the rest of input drivers shortly. > This suggests that the operation is done at the wrong level. Do the > pin configuration as you parse DT data, the same way you set up i2c > devices registers in of_i2c.c, and leave the individual drivers that do > not care about specifics alone. Exactly this can be done with pinctrl hogs. The problem with that is that it removes the cross-reference between the device and it's pinctrl handle (also from the device tree). Instead the pinctrl handle gets referenced to the pin controller itself. So from a modelling perpective this looks a bit ugly. So we have two kinds of ugly: - Sprinke devm_pinctrl_get_select_default() over all drivers which makes pinctrl handles properly reference their devices - Use hogs and loose coupling between pinctrl handles and their devices A third alternative as outlined is to use notifiers and some resource core in drivers/base/* >> That's why it is named "get_select_default" and not "get" only. >> This API ensure that the pin will be set to the default state, and this >> is all we need to do during the probe. > > Why during the probe and not by default? Realistically, the driver will > be loaded as long as there is a matching device and pins will need to be > configured. Hogs will do it by default but will disassociate the pinctrl from its device as described. >> There is no point to change the mux if the driver is not probed, because >> potentially the pin can be use be another driver. > > What other driver would use it? Who would chose what driver to load? I don't know about this specific driver, but for the SKE keypad driver we have a runtime case switching around the pins. We recently patched the pinctrl core for a specific usecase like this, and in that case both drivers are loaded, but one will be disabled at runtime and the other become active. In the ux500, if you need to perform deep debugging on a running system (an ordinary cell phone, say) you can at runtime convert the SD card output into an STM trace port and start monitoring different messages coming out on a MIPI-type bus. So basically it is not an SD card slot anymore, it turns into something else, the silicon core for MMC/SD is decoupled from its pins, and they are re-routed to the STM tracer. And you plug a special piece of hardware into the SD-card slot and it has a USB cord or something collection standard MIPI traces. We have the same for the SKE keypad actually. We can shunt the STM tracing signals out on this as well, you "just" unmount the physical keypad and start using the lines on the PCB as a trace collecting mechanism. Thus we need - at runtime, in systems produced in the the millions, it's not "just for fun" - to recouple pins from one IP block to another and turn it into a totally different thing. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 24, 2012 at 11:37:04AM +0300, Felipe Balbi wrote: > Hi, > > On Tue, Oct 23, 2012 at 01:02:49PM -0700, Dmitry Torokhov wrote: > > On Tue, Oct 23, 2012 at 11:18:12AM +0200, Benoit Cousson wrote: > > > Hi Dimitry, > > > > > > On 10/22/2012 05:50 PM, Dmitry Torokhov wrote: > > > > Hi Sourav, > > > > > > > > On Mon, Oct 22, 2012 at 06:43:00PM +0530, Sourav Poddar wrote: > > > >> Adapt keypad to use pinctrl framework. > > > >> > > > >> Tested on omap4430 sdp with 3.7-rc1 kernel. > > > > > > > > I do not see anything in the driver that would directly use pinctrl. Is > > > > there a better place to select default pin configuration; maybe when > > > > instantiating platform device? > > > > > > Why? > > > The devm_pinctrl_get_select_default is using the pinctrl. > > > > No, I guess we ihave different understanding of what "directly use" means. > > This particular driver does directly use interrupts: it requests it and > > performs some actions when interrupt arrives. Same goes for IO memory - > > it gets requested, then we access it. With pinctrl we do not do anything > > - we just ask another layer to configure it and that is it. > > this is true for almost anything we do: > > - we ask another layer to allocate memory for us > - we ask another layer to call our ISR once the IRQ line is asserted > - we ask another layer to handle the input events we just received > - we ask another layer to transfer data through DMA for us > - we ask another layer to turn regulators on and off. But we are _directly_ _using_ all of these. You allocate memory and you (the driver) stuff data into that memory. You ask for DMA and you take the DMAed data and work with it. Not so with pinctrl in omap keypad and other drivers I have seen so far. > > and so on. This is just how abstractions work, we group common parts in > a framework so that users don't need to know the details, but still need > to tell the framework when to fiddle with those resources. > > > I have seen just in a few days 3 or 4 drivers having exactly the same > > change - call to devm_pinctrl_get_select_default(), and I guess I will > > receive the same patches for the rest of input drivers shortly. > > This suggests that the operation is done at the wrong level. Do the > > pin configuration as you parse DT data, the same way you set up i2c > > devices registers in of_i2c.c, and leave the individual drivers that do > > not care about specifics alone. > > Makes no sense to hide that from drivers. The idea here is that driver > should know when it needs its pins to muxed correctly. The driver also needs memory controller to be initialized, gpio chip be ready and registered, DMA subsystem ready, input core reade, etc, etc, etc. You however do not have every driver explicitly initialize any of that; you expect certain working environment to be already operable. The driver does manage resources it controls, it has ultimate knowledge about, pin configuration is normally is not it. We just need to know that we wired/muxed properly, otherwise we won't work. So please let parent layers deal with it. > 95% of the time > it will be done during probe() but then again, so what ? > > doing that when parsing DT, or on bus notifiers is just plain wrong. > Drivers should be required to handle all of their resources. All of _their_ resources, exactly. They do not own nor control pins so they should not be bothered with them either. Look, when you see that potentially _every_ driver in the system needs to set up the same object that it doe snot use otherwise you should realize that individual driver is not the proper place to do that. > > > > That's why it is named "get_select_default" and not "get" only. > > > This API ensure that the pin will be set to the default state, and this > > > is all we need to do during the probe. > > > > Why during the probe and not by default? Realistically, the driver will > > be loaded as long as there is a matching device and pins will need to be > > configured. > > likewise memory will be allocated when matching happens, IRQs will be > allocated, regulators will be turned on. So why don't we do all that by > default ? Because it is wrong. No, because we do not know how. The generic layer does not know the ISR to install, how much memory to allocate, etc. Having regulator turned on before getting to probe might not be a bad idea. > > > > There is no point to change the mux if the driver is not probed, because > > > potentially the pin can be use be another driver. > > > > What other driver would use it? Who would chose what driver to load? > > Well, you _do_ know that on a SoC we have a limited amount of pins > right ? > > Considering the amont of features which are packed inside a single die, > it's not farfetched to assume we will have a lot less pins then we > actually need, so we need muxers behind each pin in order to choose > which functionality we want. > > If it happens that keypad's pins are shared with another IP (e.g. GPIO), > we need to give the final user (a OEM/ODM) the choice of using those > pins as either keypad or GPIOs, thus the need for pinctrl framework and > the calls in the drivers. Right, so please walk me through, step by step, how an OEM/ODM woudl select a particular configuration. Do you expect it to happen at runtime, or do you expect relevant data be put in DT? Thanks.
On Wed, Oct 24, 2012 at 02:54:23PM +0200, Linus Walleij wrote: > On Tue, Oct 23, 2012 at 10:02 PM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > > I have seen just in a few days 3 or 4 drivers having exactly the same > > change - call to devm_pinctrl_get_select_default(), and I guess I will > > receive the same patches for the rest of input drivers shortly. > > This suggests that the operation is done at the wrong level. Do the > > pin configuration as you parse DT data, the same way you set up i2c > > devices registers in of_i2c.c, and leave the individual drivers that do > > not care about specifics alone. > > Exactly this can be done with pinctrl hogs. > > The problem with that is that it removes the cross-reference > between the device and it's pinctrl handle (also from the device > tree). Instead the pinctrl handle gets referenced to the pin controller > itself. So from a modelling perpective this looks a bit ugly. > > So we have two kinds of ugly: > > - Sprinke devm_pinctrl_get_select_default() over all drivers > which makes pinctrl handles properly reference their devices > > - Use hogs and loose coupling between pinctrl handles and their > devices > > A third alternative as outlined is to use notifiers and some > resource core in drivers/base/* OK, so with drivers/base/, have you considered doing default pinctrl selection in bus's probe() methods? Yo would select the default configuration before starting probing the device and maybe select idle when probe fails or device is unbound? That would still keep the link between device object and pinctrl and there less busses than device drivers out there. Thanks.
On Wed, Oct 24, 2012 at 6:14 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Wed, Oct 24, 2012 at 11:37:04AM +0300, Felipe Balbi wrote: >> - we ask another layer to allocate memory for us >> - we ask another layer to call our ISR once the IRQ line is asserted >> - we ask another layer to handle the input events we just received >> - we ask another layer to transfer data through DMA for us >> - we ask another layer to turn regulators on and off. > > But we are _directly_ _using_ all of these. You allocate memory and you > (the driver) stuff data into that memory. You ask for DMA and you take > the DMAed data and work with it. Not so with pinctrl in omap keypad and > other drivers I have seen so far. Consult: drivers/tty/serial/amba-pl011.c drivers/spi/spi-pl022.c drivers/i2c/busses/i2c-nomadik.c for more complex pinctrl use cases. These are my dogfood drivers ... Most of these will request more than one state and switch the driver between these different states at runtime, in these examples for power saving there are states named "default", "sleep" and in the I2C driver also "idle". These examples are more typical to how the ux500 platform will look, also the SKE input driver will move the devise to sleep/default states but we need to merge PM code before we can do that. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Wed, Oct 24, 2012 at 09:14:29AM -0700, Dmitry Torokhov wrote: <snip> > > > No, I guess we ihave different understanding of what "directly use" means. > > > This particular driver does directly use interrupts: it requests it and > > > performs some actions when interrupt arrives. Same goes for IO memory - > > > it gets requested, then we access it. With pinctrl we do not do anything > > > - we just ask another layer to configure it and that is it. > > > > this is true for almost anything we do: > > > > - we ask another layer to allocate memory for us > > - we ask another layer to call our ISR once the IRQ line is asserted > > - we ask another layer to handle the input events we just received > > - we ask another layer to transfer data through DMA for us > > - we ask another layer to turn regulators on and off. > > But we are _directly_ _using_ all of these. You allocate memory and you > (the driver) stuff data into that memory. You ask for DMA and you take > the DMAed data and work with it. Not so with pinctrl in omap keypad and > other drivers I have seen so far. of course we are. If we don't mux the pins to their correct setting, we can't realy use the HW. So while you don't see any SW control of the requested pins, we're still making use of them. > > and so on. This is just how abstractions work, we group common parts in > > a framework so that users don't need to know the details, but still need > > to tell the framework when to fiddle with those resources. > > > > > I have seen just in a few days 3 or 4 drivers having exactly the same > > > change - call to devm_pinctrl_get_select_default(), and I guess I will > > > receive the same patches for the rest of input drivers shortly. > > > This suggests that the operation is done at the wrong level. Do the > > > pin configuration as you parse DT data, the same way you set up i2c > > > devices registers in of_i2c.c, and leave the individual drivers that do > > > not care about specifics alone. > > > > Makes no sense to hide that from drivers. The idea here is that driver > > should know when it needs its pins to muxed correctly. > > The driver also needs memory controller to be initialized, gpio chip be > ready and registered, DMA subsystem ready, input core reade, etc, etc, > etc. You however do not have every driver explicitly initialize any of > that; you expect certain working environment to be already operable. The > driver does manage resources it controls, it has ultimate knowledge > about, pin configuration is normally is not it. We just need to know > that we wired/muxed properly, otherwise we won't work. So please let > parent layers deal with it. > > > 95% of the time > > it will be done during probe() but then again, so what ? > > > > doing that when parsing DT, or on bus notifiers is just plain wrong. > > Drivers should be required to handle all of their resources. > > All of _their_ resources, exactly. They do not own nor control pins so > they should not be bothered with them either. Look, when you see that except that they *own* the pins. Now that the muxer has been setup properly, this particular IP owns the pins. > potentially _every_ driver in the system needs to set up the same object > that it doe snot use otherwise you should realize that individual driver > is not the proper place to do that. fair enough, but IMHO, we're not there yet. We can't make that claim yet. Besides, we don't know what's the default pin state in a system. It might be that certain pins start out in a way which consumes less power due to the internal construction of the SoC. If we set pins up before driver probes, and probe fails or the driver is never really used, then we could be falling into a situation where we're wasting power. Granted, you can undo everything you did before, but I guess keeping track of everything we setup before probe() just to remove a couple of lines from drivers is wrong. > > > > That's why it is named "get_select_default" and not "get" only. > > > > This API ensure that the pin will be set to the default state, and this > > > > is all we need to do during the probe. > > > > > > Why during the probe and not by default? Realistically, the driver will > > > be loaded as long as there is a matching device and pins will need to be > > > configured. > > > > likewise memory will be allocated when matching happens, IRQs will be > > allocated, regulators will be turned on. So why don't we do all that by > > default ? Because it is wrong. > > No, because we do not know how. The generic layer does not know the ISR > to install, how much memory to allocate, etc. Having regulator turned on > before getting to probe might not be a bad idea. what if your driver never probes ? Will you really leave regulators on consuming extra, valuable power ? > > > > There is no point to change the mux if the driver is not probed, because > > > > potentially the pin can be use be another driver. > > > > > > What other driver would use it? Who would chose what driver to load? > > > > Well, you _do_ know that on a SoC we have a limited amount of pins > > right ? > > > > Considering the amont of features which are packed inside a single die, > > it's not farfetched to assume we will have a lot less pins then we > > actually need, so we need muxers behind each pin in order to choose > > which functionality we want. > > > > If it happens that keypad's pins are shared with another IP (e.g. GPIO), > > we need to give the final user (a OEM/ODM) the choice of using those > > pins as either keypad or GPIOs, thus the need for pinctrl framework and > > the calls in the drivers. > > Right, so please walk me through, step by step, how an OEM/ODM woudl > select a particular configuration. Do you expect it to happen at > runtime, or do you expect relevant data be put in DT? It depends, I've seen both happening, really. Also note that DT migration is still not complete, meaning that most (all ?) OEM/ODMs are still using the legacy board-file-based approach and it will still take them a few years to move to DT-based boot. Another point to consider is community boards such as beaglebone which have tens of different "capes" to support. Depending on the cape, pins might have to be remuxed, so instead of adding all that code to platform support, just leave it all in drivers. Depending on the "cape" different drivers will probe() and those drivers should know how to mux pins for themselves. Note that these are only the two easy examples that came to my mind, I'm sure we can discuss this for a long, but is it valid ? For a single line of code ?
Hi, On Wed, Oct 24, 2012 at 09:18:01AM -0700, Dmitry Torokhov wrote: > On Wed, Oct 24, 2012 at 02:54:23PM +0200, Linus Walleij wrote: > > On Tue, Oct 23, 2012 at 10:02 PM, Dmitry Torokhov > > <dmitry.torokhov@gmail.com> wrote: > > > > > I have seen just in a few days 3 or 4 drivers having exactly the same > > > change - call to devm_pinctrl_get_select_default(), and I guess I will > > > receive the same patches for the rest of input drivers shortly. > > > This suggests that the operation is done at the wrong level. Do the > > > pin configuration as you parse DT data, the same way you set up i2c > > > devices registers in of_i2c.c, and leave the individual drivers that do > > > not care about specifics alone. > > > > Exactly this can be done with pinctrl hogs. > > > > The problem with that is that it removes the cross-reference > > between the device and it's pinctrl handle (also from the device > > tree). Instead the pinctrl handle gets referenced to the pin controller > > itself. So from a modelling perpective this looks a bit ugly. > > > > So we have two kinds of ugly: > > > > - Sprinke devm_pinctrl_get_select_default() over all drivers > > which makes pinctrl handles properly reference their devices > > > > - Use hogs and loose coupling between pinctrl handles and their > > devices > > > > A third alternative as outlined is to use notifiers and some > > resource core in drivers/base/* > > OK, so with drivers/base/, have you considered doing default pinctrl > selection in bus's probe() methods? Yo would select the default > configuration before starting probing the device and maybe select idle > when probe fails or device is unbound? That would still keep the link > between device object and pinctrl and there less busses than device > drivers out there. it starts to become confusing after a while. I mean, there's a reason why all drivers explictly call pm_runtim_enable(), right ? From a first thought, one could think of just yanking that into bus' probe() as you may suggest, but sometimes the device is already enabled, so we need extra tricks: pm_runtime_set_active(); pm_runtime_enable(); pm_runtime_get(); the same could happen with pinctrl eventually. What if a device needs to do something else (an errata fix as an example) before requesting pinctrl's default state ?
On Wed, Oct 24, 2012 at 6:18 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Wed, Oct 24, 2012 at 02:54:23PM +0200, Linus Walleij wrote: >> >> A third alternative as outlined is to use notifiers and some >> resource core in drivers/base/* > > OK, so with drivers/base/, have you considered doing default pinctrl > selection in bus's probe() methods? Yo would select the default > configuration before starting probing the device and maybe select idle > when probe fails or device is unbound? That would still keep the link > between device object and pinctrl and there less busses than device > drivers out there. One of our major important busses is the AMBA (PrimeCell) bus. As it happens, this bus actually already do limited resource handling by requesting the silicon block clock for the device, which is necessary to perform auto-probing on the bus level. (You won't be able to read the auto-detect registers unless the silicon is clocked...) When it comes to pin control is turns out in the AMBA drivers we have that we need to do more complex stuff than just select a default configuration. (I assure this is not just for fun, it is saving considerable amounts of power). So the examples I outlined just in the previous mail: drivers/tty/serial/amba-pl011.c drivers/spi/spi-pl022.c drivers/i2c/busses/i2c-nomadik.c Hm it turns out that Wolfram has not yet merged the i2c patch, here it is: http://marc.info/?l=linux-i2c&m=134986995731695&w=2 There are complex state switches involved. It can arguably be centralized, but then it needs to go into drivers/base/[power] or similar, not into a specific piece of bus code, because the needs won't be any different for e.g. a platform device. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 24, 2012 at 6:52 PM, Felipe Balbi <balbi@ti.com> wrote: > On Wed, Oct 24, 2012 at 09:14:29AM -0700, Dmitry Torokhov wrote: >> All of _their_ resources, exactly. They do not own nor control pins so >> they should not be bothered with them either. Look, when you see that > > except that they *own* the pins. Now that the muxer has been setup > properly, this particular IP owns the pins. This is true. It is then also reflected in the device model. And in debugfs, which is very helpful when debugging. If I cat /sys/kernel/debug/pinctrl/pinctrl-db8500/pins I can see a list of all pins on this ASIC and what device is using it. It is all due to the fact that each driver use [devm_]pinctrl_get(&dev) ad the struct device * pointer is used with dev_name() to print the corresponding device name. When using hogs it just says the name of the pin controller and the pins aren't really connected to their real consumers :-P Example: Pinmux settings per pin Format: pin (name): mux_owner gpio_owner hog? pin 0 (GPIO0_AJ5): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 1 (GPIO1_AJ3): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 2 (GPIO2_AH4): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 3 (GPIO3_AH3): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 4 (GPIO4_AH6): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 5 (GPIO5_AG6): (MUX UNCLAIMED) DB8500:5 pin 6 (GPIO6_AF6): pinctrl-db8500 (GPIO UNCLAIMED) function ipgpio group ipgpio0_c_1 pin 7 (GPIO7_AG5): pinctrl-db8500 (GPIO UNCLAIMED) function ipgpio group ipgpio1_c_1 pin 8 (GPIO8_AD5): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 9 (GPIO9_AE4): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 10 (GPIO10_AF5): nmk-i2c.2 (GPIO UNCLAIMED) function i2c2 group i2c2_b_2 pin 11 (GPIO11_AG4): nmk-i2c.2 (GPIO UNCLAIMED) function i2c2 group i2c2_b_2 pin 12 (GPIO12_AC4): pinctrl-db8500 (GPIO UNCLAIMED) function msp0 group msp0txrx_a_1 pin 13 (GPIO13_AF3): pinctrl-db8500 (GPIO UNCLAIMED) function msp0 group msp0tfstck_a_1 pin 14 (GPIO14_AE3): pinctrl-db8500 (GPIO UNCLAIMED) function msp0 group msp0tfstck_a_1 pin 15 (GPIO15_AC3): pinctrl-db8500 (GPIO UNCLAIMED) function msp0 group msp0txrx_a_1 pin 16 (GPIO16_AD3): nmk-i2c.1 (GPIO UNCLAIMED) function i2c1 group i2c1_b_2 pin 17 (GPIO17_AD4): nmk-i2c.1 (GPIO UNCLAIMED) function i2c1 group i2c1_b_2 As you can see pins 6,7,12-15 are using hogs. Sure I can see the name of the function but that is just a string albeit helpful. Pins 10,11,16,17 are requested directly from the i2c driver and shows up connected to its device. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 24, 2012 at 6:57 PM, Felipe Balbi <balbi@ti.com> wrote: > On Wed, Oct 24, 2012 at 09:18:01AM -0700, Dmitry Torokhov wrote: >> OK, so with drivers/base/, have you considered doing default pinctrl >> selection in bus's probe() methods? Yo would select the default >> configuration before starting probing the device and maybe select idle >> when probe fails or device is unbound? That would still keep the link >> between device object and pinctrl and there less busses than device >> drivers out there. > > it starts to become confusing after a while. I mean, there's a reason > why all drivers explictly call pm_runtim_enable(), right ? > > From a first thought, one could think of just yanking that into bus' > probe() as you may suggest, but sometimes the device is already enabled, > so we need extra tricks: > > pm_runtime_set_active(); > pm_runtime_enable(); > pm_runtime_get(); > > the same could happen with pinctrl eventually. What if a device needs to > do something else (an errata fix as an example) before requesting > pinctrl's default state ? I can confirm this. Just the ordering between enabling/disabling resources like clock/pins/powerdomain screw things up for us and even if we can surely centralize parts of this code as such into the drivers/base or pm_* namespace we would still need explicit calls from the driver. I'm thinking that maybe the best helpers are actually static inline functions in the <linux/pinctrl/consumer.h> header rather than moving anything into drivers/base/* if the code duplication is the real problem. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, October 24, 2012 06:51:47 PM Linus Walleij wrote: > On Wed, Oct 24, 2012 at 6:14 PM, Dmitry Torokhov > > <dmitry.torokhov@gmail.com> wrote: > > On Wed, Oct 24, 2012 at 11:37:04AM +0300, Felipe Balbi wrote: > >> - we ask another layer to allocate memory for us > >> - we ask another layer to call our ISR once the IRQ line is asserted > >> - we ask another layer to handle the input events we just received > >> - we ask another layer to transfer data through DMA for us > >> - we ask another layer to turn regulators on and off. > > > > But we are _directly_ _using_ all of these. You allocate memory and you > > (the driver) stuff data into that memory. You ask for DMA and you take > > the DMAed data and work with it. Not so with pinctrl in omap keypad and > > other drivers I have seen so far. > > Consult: > drivers/tty/serial/amba-pl011.c OK. > drivers/spi/spi-pl022.c Default/sleep transitions could be moved into bus code. > drivers/i2c/busses/i2c-nomadik.c Don't see pinctrl in linux-next. > for more complex pinctrl use cases. These are my dogfood drivers ... > Most of these will request more than one state and switch the driver > between these different states at runtime, in these examples for power > saving there are states named "default", "sleep" and in the I2C driver > also "idle". > > These examples are more typical to how the ux500 platform will > look, also the SKE input driver will move the devise to sleep/default > states but we need to merge PM code before we can do that. I do not say that no drivers should ever touch pinctrl, just that most of them do not have to if you have other layers to the right thing for them. Thanks.
On Wednesday, October 24, 2012 07:52:16 PM Felipe Balbi wrote: > Hi, > > On Wed, Oct 24, 2012 at 09:14:29AM -0700, Dmitry Torokhov wrote: > > <snip> > > > > > No, I guess we ihave different understanding of what "directly use" > > > > means. > > > > This particular driver does directly use interrupts: it requests it > > > > and > > > > performs some actions when interrupt arrives. Same goes for IO memory > > > > - > > > > it gets requested, then we access it. With pinctrl we do not do > > > > anything > > > > - we just ask another layer to configure it and that is it. > > > > > > this is true for almost anything we do: > > > > > > - we ask another layer to allocate memory for us > > > - we ask another layer to call our ISR once the IRQ line is asserted > > > - we ask another layer to handle the input events we just received > > > - we ask another layer to transfer data through DMA for us > > > - we ask another layer to turn regulators on and off. > > > > But we are _directly_ _using_ all of these. You allocate memory and you > > (the driver) stuff data into that memory. You ask for DMA and you take > > the DMAed data and work with it. Not so with pinctrl in omap keypad and > > other drivers I have seen so far. > > of course we are. If we don't mux the pins to their correct setting, we > can't realy use the HW. So while you don't see any SW control of the > requested pins, we're still making use of them. So we make use of CPU too, and the main power supply, and memory chips. > > > > and so on. This is just how abstractions work, we group common parts in > > > a framework so that users don't need to know the details, but still need > > > to tell the framework when to fiddle with those resources. > > > > > > > I have seen just in a few days 3 or 4 drivers having exactly the same > > > > change - call to devm_pinctrl_get_select_default(), and I guess I will > > > > receive the same patches for the rest of input drivers shortly. > > > > This suggests that the operation is done at the wrong level. Do the > > > > pin configuration as you parse DT data, the same way you set up i2c > > > > devices registers in of_i2c.c, and leave the individual drivers that > > > > do > > > > not care about specifics alone. > > > > > > Makes no sense to hide that from drivers. The idea here is that driver > > > should know when it needs its pins to muxed correctly. > > > > The driver also needs memory controller to be initialized, gpio chip be > > ready and registered, DMA subsystem ready, input core reade, etc, etc, > > etc. You however do not have every driver explicitly initialize any of > > that; you expect certain working environment to be already operable. The > > driver does manage resources it controls, it has ultimate knowledge > > about, pin configuration is normally is not it. We just need to know > > that we wired/muxed properly, otherwise we won't work. So please let > > parent layers deal with it. > > > > > 95% of the time > > > it will be done during probe() but then again, so what ? > > > > > > doing that when parsing DT, or on bus notifiers is just plain wrong. > > > Drivers should be required to handle all of their resources. > > > > All of _their_ resources, exactly. They do not own nor control pins so > > they should not be bothered with them either. Look, when you see that > > except that they *own* the pins. Now that the muxer has been setup > properly, this particular IP owns the pins. > > > potentially _every_ driver in the system needs to set up the same object > > that it doe snot use otherwise you should realize that individual driver > > is not the proper place to do that. > > fair enough, but IMHO, we're not there yet. We can't make that claim > yet. Besides, we don't know what's the default pin state in a system. It > might be that certain pins start out in a way which consumes less power > due to the internal construction of the SoC. If we set pins up before > driver probes, and probe fails or the driver is never really used, then > we could be falling into a situation where we're wasting power. So what about moving this into bus code - have bus's probe() request default pin config before probe, revert to original setup when unbinding or probe fails. You can even plug PM switching into bus code as well. > > Granted, you can undo everything you did before, Right, the same way as we undo every other initialization when something goes wrong. > but I guess keeping > track of everything we setup before probe() just to remove a couple of > lines from drivers is wrong. If it was just about a couple lines in a couple of drivers that would be fine. But the way I see it eventually every driver will need to do this. > > > > > > That's why it is named "get_select_default" and not "get" only. > > > > > This API ensure that the pin will be set to the default state, and > > > > > this > > > > > is all we need to do during the probe. > > > > > > > > Why during the probe and not by default? Realistically, the driver > > > > will > > > > be loaded as long as there is a matching device and pins will need to > > > > be > > > > configured. > > > > > > likewise memory will be allocated when matching happens, IRQs will be > > > allocated, regulators will be turned on. So why don't we do all that by > > > default ? Because it is wrong. > > > > No, because we do not know how. The generic layer does not know the ISR > > to install, how much memory to allocate, etc. Having regulator turned on > > before getting to probe might not be a bad idea. > > what if your driver never probes ? Will you really leave regulators on > consuming extra, valuable power ? If we do it right in probe() we won't consume unless the dirver is bound. > > > > > > There is no point to change the mux if the driver is not probed, > > > > > because > > > > > potentially the pin can be use be another driver. > > > > > > > > What other driver would use it? Who would chose what driver to load? > > > > > > Well, you _do_ know that on a SoC we have a limited amount of pins > > > right ? > > > > > > Considering the amont of features which are packed inside a single die, > > > it's not farfetched to assume we will have a lot less pins then we > > > actually need, so we need muxers behind each pin in order to choose > > > which functionality we want. > > > > > > If it happens that keypad's pins are shared with another IP (e.g. GPIO), > > > we need to give the final user (a OEM/ODM) the choice of using those > > > pins as either keypad or GPIOs, thus the need for pinctrl framework and > > > the calls in the drivers. > > > > Right, so please walk me through, step by step, how an OEM/ODM woudl > > select a particular configuration. Do you expect it to happen at > > runtime, or do you expect relevant data be put in DT? > > It depends, I've seen both happening, really. Also note that DT > migration is still not complete, meaning that most (all ?) OEM/ODMs are > still using the legacy board-file-based approach and it will still take > them a few years to move to DT-based boot. > > Another point to consider is community boards such as beaglebone which > have tens of different "capes" to support. Depending on the cape, pins > might have to be remuxed, so instead of adding all that code to platform > support, just leave it all in drivers. Depending on the "cape" different > drivers will probe() and those drivers should know how to mux pins for > themselves. > > Note that these are only the two easy examples that came to my mind, I'm > sure we can discuss this for a long, but is it valid ? For a single line > of code ? Multiply by hundred of drivers - yes.
Hi Dmitry, On 10/24/2012 06:14 PM, Dmitry Torokhov wrote: > On Wed, Oct 24, 2012 at 11:37:04AM +0300, Felipe Balbi wrote: >> Hi, >> >> On Tue, Oct 23, 2012 at 01:02:49PM -0700, Dmitry Torokhov wrote: >>> On Tue, Oct 23, 2012 at 11:18:12AM +0200, Benoit Cousson wrote: >>>> Hi Dimitry, >>>> >>>> On 10/22/2012 05:50 PM, Dmitry Torokhov wrote: >>>>> Hi Sourav, >>>>> >>>>> On Mon, Oct 22, 2012 at 06:43:00PM +0530, Sourav Poddar wrote: >>>>>> Adapt keypad to use pinctrl framework. >>>>>> >>>>>> Tested on omap4430 sdp with 3.7-rc1 kernel. >>>>> >>>>> I do not see anything in the driver that would directly use pinctrl. Is >>>>> there a better place to select default pin configuration; maybe when >>>>> instantiating platform device? >>>> >>>> Why? >>>> The devm_pinctrl_get_select_default is using the pinctrl. >>> >>> No, I guess we ihave different understanding of what "directly use" means. >>> This particular driver does directly use interrupts: it requests it and >>> performs some actions when interrupt arrives. Same goes for IO memory - >>> it gets requested, then we access it. With pinctrl we do not do anything >>> - we just ask another layer to configure it and that is it. >> >> this is true for almost anything we do: >> >> - we ask another layer to allocate memory for us >> - we ask another layer to call our ISR once the IRQ line is asserted >> - we ask another layer to handle the input events we just received >> - we ask another layer to transfer data through DMA for us >> - we ask another layer to turn regulators on and off. > > But we are _directly_ _using_ all of these. You allocate memory and you > (the driver) stuff data into that memory. You ask for DMA and you take > the DMAed data and work with it. Not so with pinctrl in omap keypad and > other drivers I have seen so far. That's not really true. You select a pin mode and thanks to that you get the signal from an external pin that goes to your IP. And thanks to that the IP is doing what your are expecting it to do. Without that your IP will not get any input signal, which will make it a little bit useless, isn't it? >> and so on. This is just how abstractions work, we group common parts in >> a framework so that users don't need to know the details, but still need >> to tell the framework when to fiddle with those resources. >> >>> I have seen just in a few days 3 or 4 drivers having exactly the same >>> change - call to devm_pinctrl_get_select_default(), and I guess I will >>> receive the same patches for the rest of input drivers shortly. >>> This suggests that the operation is done at the wrong level. Do the >>> pin configuration as you parse DT data, the same way you set up i2c >>> devices registers in of_i2c.c, and leave the individual drivers that do >>> not care about specifics alone. >> >> Makes no sense to hide that from drivers. The idea here is that driver >> should know when it needs its pins to muxed correctly. > > The driver also needs memory controller to be initialized, gpio chip be > ready and registered, DMA subsystem ready, input core reade, etc, etc, > etc. You however do not have every driver explicitly initialize any of > that; you expect certain working environment to be already operable. The > driver does manage resources it controls, it has ultimate knowledge > about, pin configuration is normally is not it. We just need to know > that we wired/muxed properly, otherwise we won't work. So please let > parent layers deal with it. > >> 95% of the time >> it will be done during probe() but then again, so what ? >> >> doing that when parsing DT, or on bus notifiers is just plain wrong. >> Drivers should be required to handle all of their resources. > > All of _their_ resources, exactly. They do not own nor control pins so > they should not be bothered with them either. Look, when you see that > potentially _every_ driver in the system needs to set up the same object > that it doe snot use otherwise you should realize that individual driver > is not the proper place to do that. What your are missing as well in that case is the explicit dependency that this API is creating with the pinctrl driver that we are going to miss otherwise. Hence the following code. + if (PTR_ERR(keypad_data->pins) == -EPROBE_DEFER) + return -EPROBE_DEFER; If the pinctrl is not already there you defer the probe until it is there. Moreover, as already said, we are probably at some point going to handle as well the low power mode and thus change the pin mode upon idle/suspend. And again, selecting a pin mode during probe is doing something with the pins when and only when it is useful. It is not different than getting an IRQ or DMA request at probe time. You get it, use it for registration and that all you are doing with it. It is no different here. Doing that during device creation does not make sense, since that device might never be used. Is it like allocating the memory by default for every devices at boot time just in case a driver will probe it at some time or registering every IRQs at boot time just in case a driver will use it... That's just pointless. We are wasting resources for nothing and thus potentially power and that will not help the Earth getting any better. Regards, Benoit -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, October 24, 2012 07:57:49 PM Felipe Balbi wrote: > Hi, > > On Wed, Oct 24, 2012 at 09:18:01AM -0700, Dmitry Torokhov wrote: > > On Wed, Oct 24, 2012 at 02:54:23PM +0200, Linus Walleij wrote: > > > On Tue, Oct 23, 2012 at 10:02 PM, Dmitry Torokhov > > > > > > <dmitry.torokhov@gmail.com> wrote: > > > > I have seen just in a few days 3 or 4 drivers having exactly the same > > > > change - call to devm_pinctrl_get_select_default(), and I guess I will > > > > receive the same patches for the rest of input drivers shortly. > > > > This suggests that the operation is done at the wrong level. Do the > > > > pin configuration as you parse DT data, the same way you set up i2c > > > > devices registers in of_i2c.c, and leave the individual drivers that > > > > do > > > > not care about specifics alone. > > > > > > Exactly this can be done with pinctrl hogs. > > > > > > The problem with that is that it removes the cross-reference > > > between the device and it's pinctrl handle (also from the device > > > tree). Instead the pinctrl handle gets referenced to the pin controller > > > itself. So from a modelling perpective this looks a bit ugly. > > > > > > So we have two kinds of ugly: > > > > > > - Sprinke devm_pinctrl_get_select_default() over all drivers > > > > > > which makes pinctrl handles properly reference their devices > > > > > > - Use hogs and loose coupling between pinctrl handles and their > > > > > > devices > > > > > > A third alternative as outlined is to use notifiers and some > > > resource core in drivers/base/* > > > > OK, so with drivers/base/, have you considered doing default pinctrl > > selection in bus's probe() methods? Yo would select the default > > configuration before starting probing the device and maybe select idle > > when probe fails or device is unbound? That would still keep the link > > between device object and pinctrl and there less busses than device > > drivers out there. > > it starts to become confusing after a while. I mean, there's a reason > why all drivers explictly call pm_runtim_enable(), right ? Right. Because not all of them support runtime PM and quite usually their PM methods are not ready to go until initialization is complete. And again, the driver here controls its own behavior. > > From a first thought, one could think of just yanking that into bus' > probe() as you may suggest, but sometimes the device is already enabled, > so we need extra tricks: > > pm_runtime_set_active(); > pm_runtime_enable(); > pm_runtime_get(); > > the same could happen with pinctrl eventually. What if a device needs to > do something else (an errata fix as an example) before requesting > pinctrl's default state ? That is a valid concern and we'll need to find a compromise here. As I said, I am not saying that no driver should ever touch pinctrl. I can see, for example, input drivers actually disabling pins until they are ->open()ed, in addition of doing that at [runtime] suspend/resume. But it would be nice if we could have "dumb" drivers not care about pins. Thanks.
Hi, On Wed, Oct 24, 2012 at 10:28:46AM -0700, Dmitry Torokhov wrote: > > for more complex pinctrl use cases. These are my dogfood drivers ... > > Most of these will request more than one state and switch the driver > > between these different states at runtime, in these examples for power > > saving there are states named "default", "sleep" and in the I2C driver > > also "idle". > > > > These examples are more typical to how the ux500 platform will > > look, also the SKE input driver will move the devise to sleep/default > > states but we need to merge PM code before we can do that. > > I do not say that no drivers should ever touch pinctrl, just that most > of them do not have to if you have other layers to the right thing for > them. It will be a much bigger mess. Some drivers don't need to care about pinctrl because drivers/base handle it for them, while some others will need a way to tell drivers/base "hey, don't touch pinctrl at all because I know what I'm doing" and that has to happen before probe() too, otherwise it's already too late and, according to what you suggest, drivers/base will already have touched pinctrl. The only way I see would be to add an extra "dont_touch_my_pins" field to every driver structure in the kernel. Clearly what you say is nonsense.
Hi, On Wed, Oct 24, 2012 at 10:58:53AM -0700, Dmitry Torokhov wrote: > On Wednesday, October 24, 2012 07:57:49 PM Felipe Balbi wrote: > > Hi, > > > > On Wed, Oct 24, 2012 at 09:18:01AM -0700, Dmitry Torokhov wrote: > > > On Wed, Oct 24, 2012 at 02:54:23PM +0200, Linus Walleij wrote: > > > > On Tue, Oct 23, 2012 at 10:02 PM, Dmitry Torokhov > > > > > > > > <dmitry.torokhov@gmail.com> wrote: > > > > > I have seen just in a few days 3 or 4 drivers having exactly the same > > > > > change - call to devm_pinctrl_get_select_default(), and I guess I will > > > > > receive the same patches for the rest of input drivers shortly. > > > > > This suggests that the operation is done at the wrong level. Do the > > > > > pin configuration as you parse DT data, the same way you set up i2c > > > > > devices registers in of_i2c.c, and leave the individual drivers that > > > > > do > > > > > not care about specifics alone. > > > > > > > > Exactly this can be done with pinctrl hogs. > > > > > > > > The problem with that is that it removes the cross-reference > > > > between the device and it's pinctrl handle (also from the device > > > > tree). Instead the pinctrl handle gets referenced to the pin controller > > > > itself. So from a modelling perpective this looks a bit ugly. > > > > > > > > So we have two kinds of ugly: > > > > > > > > - Sprinke devm_pinctrl_get_select_default() over all drivers > > > > > > > > which makes pinctrl handles properly reference their devices > > > > > > > > - Use hogs and loose coupling between pinctrl handles and their > > > > > > > > devices > > > > > > > > A third alternative as outlined is to use notifiers and some > > > > resource core in drivers/base/* > > > > > > OK, so with drivers/base/, have you considered doing default pinctrl > > > selection in bus's probe() methods? Yo would select the default > > > configuration before starting probing the device and maybe select idle > > > when probe fails or device is unbound? That would still keep the link > > > between device object and pinctrl and there less busses than device > > > drivers out there. > > > > it starts to become confusing after a while. I mean, there's a reason > > why all drivers explictly call pm_runtim_enable(), right ? > > Right. Because not all of them support runtime PM and quite usually their > PM methods are not ready to go until initialization is complete. And again, > the driver here controls its own behavior. likewise not all devices will need pin muxing, those which do (granted, an increasing number of them since transistor size continue to shrink, allowing chip manufacturers to pack more features inside a single die, while the number of external pins/balls remain the same), will call pinctrl to setup muxing right. > > From a first thought, one could think of just yanking that into bus' > > probe() as you may suggest, but sometimes the device is already enabled, > > so we need extra tricks: > > > > pm_runtime_set_active(); > > pm_runtime_enable(); > > pm_runtime_get(); > > > > the same could happen with pinctrl eventually. What if a device needs to > > do something else (an errata fix as an example) before requesting > > pinctrl's default state ? > > That is a valid concern and we'll need to find a compromise here. As I said, WHAT ?? Silicon erratas are not a valid concern ? Power waste isn't a valid concern ? Tell that to the millions of devices shipped with Linux everyday. Power usage if it's the top concern in any product, is right there as the top five. Likewise for silicon erratas. Let's face it, just like SW, HW has bugs; the difference is that no company will continue to do several spins of an ASIC just because some SW engineer doesn't get concerned about a silicon bug. It's just too expensive to re-spin the ASIC. And even if we get another revision of the ASIC, we still need to support the older version as there might be cellphones, laser welding machines, IPTVs and whatever product already shipped. > I am not saying that no driver should ever touch pinctrl. I can see, for > example, input drivers actually disabling pins until they are ->open()ed, > in addition of doing that at [runtime] suspend/resume. But it would be nice > if we could have "dumb" drivers not care about pins. Like I replied on another sub-thread, this will just create exceptions to the rule which is far more messy than having a couple of extra lines of code in a few drivers. We can even argue that eventually all drivers will need to toggle pins in runtime in order to save that extra microwatt of runtime power consumption, so why bother adding exceptions ? In fact, we already have the exception: drivers which don't need to fiddle with pin muxing, just don't use pinctrl. The ones you're receiving today are the one which, for whatever reason, need to make sure pin muxing is right. If it's not toggling in runtime, it might just be because $AUTHOR decided that it would be best to do thing in small steps (don't we all agree with that ?). Maybe he thought that changing pins in runtime could cause problems, so let's get bare minimum in mainline and work towards optimizations in parallel. All in all, I don't see why you're complaining so much about a couple of lines of code. Even if it needs to be added to all drivers in tree. So what ? As long as pinctrl works fine in multiple platforms, what is done for e.g. OMAP2 should work fine in OMAP3/4/5. An even more complex example: what works on OMAP, should work on Exynos, Tegra, etc, if the same driver is used accross those platforms.
On Wednesday, October 24, 2012 10:10:42 PM Felipe Balbi wrote: > > > > > > That is a valid concern and we'll need to find a compromise here. As I > > said, > WHAT ?? Silicon erratas are not a valid concern ? Power waste isn't a > valid concern ? Tell that to the millions of devices shipped with Linux > everyday. Power usage if it's the top concern in any product, is right > there as the top five. Likewise for silicon erratas. I think we should come back to this discussion when you get more coffee and start parsing other party e-mails properly.
Hi, On Wed, Oct 24, 2012 at 12:38:44PM -0700, Dmitry Torokhov wrote: > On Wednesday, October 24, 2012 10:10:42 PM Felipe Balbi wrote: > > > > > > > > > That is a valid concern and we'll need to find a compromise here. As I > > > said, > > WHAT ?? Silicon erratas are not a valid concern ? Power waste isn't a > > valid concern ? Tell that to the millions of devices shipped with Linux > > everyday. Power usage if it's the top concern in any product, is right > > there as the top five. Likewise for silicon erratas. > > I think we should come back to this discussion when you get more coffee > and start parsing other party e-mails properly. indeed. I really misparsed it. My bad. comments withdrawn.
On Wed, Oct 24, 2012 at 09:58:19PM +0300, Felipe Balbi wrote: > need a way to tell drivers/base "hey, don't touch pinctrl at all because > I know what I'm doing" and that has to happen before probe() too, > otherwise it's already too late and, according to what you suggest, > drivers/base will already have touched pinctrl. The only way I see would > be to add an extra "dont_touch_my_pins" field to every driver structure > in the kernel. Clearly what you say is nonsense. I suspect that's not actually a big deal and that if we went down this route we'd have the driver take over control from the core code during probe() with the core still setting up the default state. Personally I do think we want to be factoring bolierplate out of drivers, if they're not doing anything constructive with pinctrl they should be able to avoid having code for it. There definitely are issues to work through but it seems like we ought to be able to do something. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Thu, Oct 25, 2012 at 09:59:01PM +0100, Mark Brown wrote: > On Wed, Oct 24, 2012 at 09:58:19PM +0300, Felipe Balbi wrote: > > > need a way to tell drivers/base "hey, don't touch pinctrl at all because > > I know what I'm doing" and that has to happen before probe() too, > > otherwise it's already too late and, according to what you suggest, > > drivers/base will already have touched pinctrl. The only way I see would > > be to add an extra "dont_touch_my_pins" field to every driver structure > > in the kernel. Clearly what you say is nonsense. > > I suspect that's not actually a big deal and that if we went down this > route we'd have the driver take over control from the core code during > probe() with the core still setting up the default state. > > Personally I do think we want to be factoring bolierplate out of > drivers, if they're not doing anything constructive with pinctrl they > should be able to avoid having code for it. There definitely are issues > to work through but it seems like we ought to be able to do something. IMHO this will come back to bite you in the *ss. Specially when the same driver is shared among multiple revisions of the same SoC or multiple different SoCs. Hypothetical situation: OMAP4 has keypad as the default pin mode and low power is handled by the HW, so keypad could have pinctlr "boilerplate" factored out. Then comes OMAP5 and low power mode has to be handled by SW for whatever reason (maybe there are more than one low power mode). Then we will need to patch omap4-keypad.c to remove "dont_touch_my_pins" flag and add pinctrl support. When you think of the possibilities of every single driver going throught that it sounds a lot nicer to not make that decision IMHO and keep pinctrl explicit. This is not like module_*_driver() macro.
On Fri, Oct 26, 2012 at 09:20:36AM +0300, Felipe Balbi wrote: > On Thu, Oct 25, 2012 at 09:59:01PM +0100, Mark Brown wrote: > > I suspect that's not actually a big deal and that if we went down this > > route we'd have the driver take over control from the core code during > > probe() with the core still setting up the default state. > > Personally I do think we want to be factoring bolierplate out of > > drivers, if they're not doing anything constructive with pinctrl they > > should be able to avoid having code for it. There definitely are issues > > to work through but it seems like we ought to be able to do something. > IMHO this will come back to bite you in the *ss. Specially when the same > driver is shared among multiple revisions of the same SoC or multiple > different SoCs. I'm not entirely sure you fully understood the proposal... > Hypothetical situation: OMAP4 has keypad as the default pin mode and low > power is handled by the HW, so keypad could have pinctlr "boilerplate" > factored out. Then comes OMAP5 and low power mode has to be handled by > SW for whatever reason (maybe there are more than one low power mode). > Then we will need to patch omap4-keypad.c to remove "dont_touch_my_pins" > flag and add pinctrl support. This isn't going to make any practical difference at all, as soon as the driver starts using pinctrl explicitly a flag gets set in the driver and the default code does nothing more. The only difference will be that we may get a default configuration applied prior to probe. You could have the driver explicitly set the flag, it would just be one extra line, but it seems marginally nicer to just do it. > When you think of the possibilities of every single driver going > throught that it sounds a lot nicer to not make that decision IMHO and > keep pinctrl explicit. I'm just not seeing any impact on drivers that do something interesting here.
On Wed, Oct 24, 2012 at 7:28 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: >> drivers/spi/spi-pl022.c > > Default/sleep transitions could be moved into bus code. No that's not a good idea as long as we have both the platform bus and the AMBA bus doing essentially the same thing. We will then be having two copies of the same code in two different busses running out of sync. There may be other busses too. But I could prepare static helpers in <linux/pinctrl/consumer.h> that any bus could use. Or any driver. Probably any driver, because of this: As noted the bus cannot really execute the pinctrl calls to e.g. put a drivers pins into "sleep". Since if e.g. the bus is walking the suspend() ladder, shall it put the pins into sleep *before* or *after* calling the suspend() hook in the driver? The answer is that it does not know. Because drivers have different needs. Depending on how the hardware and system is done. I already tried to make this point: pinctrl_set_state(state_sleep); clk_disable(); power_off_voltage_domain(); May for some drivers have to be: clk_disable(); power_off_voltage_domain(); pinctrl_set_state(state_sleep); (etc) I'm not making this up, it is a very real phenomenon on the Ux500 and I don't think we are unique. Moving this handling to bus code or anywhere else invariably implies that resource acquisition/release order does not matter, and my point is that it does. >> drivers/i2c/busses/i2c-nomadik.c > > Don't see pinctrl in linux-next. This code is here: http://marc.info/?l=linux-i2c&m=134986995731695&w=2 Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Fri, Oct 26, 2012 at 05:03:16PM +0100, Mark Brown wrote: > On Fri, Oct 26, 2012 at 09:20:36AM +0300, Felipe Balbi wrote: > > On Thu, Oct 25, 2012 at 09:59:01PM +0100, Mark Brown wrote: > > > > I suspect that's not actually a big deal and that if we went down this > > > route we'd have the driver take over control from the core code during > > > probe() with the core still setting up the default state. > > > > Personally I do think we want to be factoring bolierplate out of > > > drivers, if they're not doing anything constructive with pinctrl they > > > should be able to avoid having code for it. There definitely are issues > > > to work through but it seems like we ought to be able to do something. > > > IMHO this will come back to bite you in the *ss. Specially when the same > > driver is shared among multiple revisions of the same SoC or multiple > > different SoCs. > > I'm not entirely sure you fully understood the proposal... > > > Hypothetical situation: OMAP4 has keypad as the default pin mode and low > > power is handled by the HW, so keypad could have pinctlr "boilerplate" > > factored out. Then comes OMAP5 and low power mode has to be handled by > > SW for whatever reason (maybe there are more than one low power mode). > > Then we will need to patch omap4-keypad.c to remove "dont_touch_my_pins" > > flag and add pinctrl support. > > This isn't going to make any practical difference at all, as soon as the > driver starts using pinctrl explicitly a flag gets set in the driver and > the default code does nothing more. The only difference will be that we > may get a default configuration applied prior to probe. > > You could have the driver explicitly set the flag, it would just be one > extra line, but it seems marginally nicer to just do it. You didn't get the whole picture I'm afraid. Consider the situation where the same e.g. keypad driver is being used on OMAP4 and OMAP5 and those have different requirements towards pinctrl. Let's just assume for the sake of argument that OMAP4 would set the flag on the driver structure which would tell drivers/base/ to handle pinctrl default automatically and let's assume that's just fine for OMAP4 since sleep mode is handled by HW (all of this is just for the sake of argument, I'm not claiming this is how OMAP4/5 work). Now, we need to add OMAP5 support *to the same keypad driver*. Unfortunately, OMAP5 needs to handle pinctrl explicitly for whatever reason (SW-controlled sleep mode, errata fix, whatever). This will mean that we will have to remove the flag from the keypad driver because that's not valid anymore for OMAP5. What I'm claiming below is that quite possibly all drivers will go through that: - start with transparent pinctrl default mode by letting drivers/base/ automagically take care of that for us. - remove the 'handle-pinctrl-automatically-for-me' flag because of new requirements on different version of the IP. This is why I think hiding things from drivers makes no sense. Also consider the situations Linus W exposed on another subthread. If you change ordering of certain calls, you will really break the functionality of the IP. Because we can't make sure this won't work automagically in all cases (just like we can't make sure $size memory allocation is enough for all drivers) we don't hide that from the driver. We require driver to manage its resources properly. > > When you think of the possibilities of every single driver going > > throught that it sounds a lot nicer to not make that decision IMHO and > > keep pinctrl explicit. > > I'm just not seeing any impact on drivers that do something interesting > here. that's because you didn't consider enough possibilities. See above. How can you make sure that this will work for at least 50% of the drivers ? You just can't. We don't know the implementation details of every arch/soc/platform supported by Linux today to make that decision. IMHO, it's best to require drivers to explicitly setup pin muxing by calling into pinctrl framework.
On Mon, Oct 29, 2012 at 09:49:01PM +0200, Felipe Balbi wrote: > On Fri, Oct 26, 2012 at 05:03:16PM +0100, Mark Brown wrote: > > You could have the driver explicitly set the flag, it would just be one > > extra line, but it seems marginally nicer to just do it. > You didn't get the whole picture I'm afraid. Consider the situation > where the same e.g. keypad driver is being used on OMAP4 and OMAP5 and > those have different requirements towards pinctrl. No, I'm pretty certain that I do. > Now, we need to add OMAP5 support *to the same keypad driver*. > Unfortunately, OMAP5 needs to handle pinctrl explicitly for whatever > reason (SW-controlled sleep mode, errata fix, whatever). > This will mean that we will have to remove the flag from the keypad > driver because that's not valid anymore for OMAP5. This isn't a problem; either the pinctrl code for OMAP5 will do something sensible for OMAP4 in which case there's no difference to the resulting code or you're going to have to have conditional code for the two devices anyway and you're no worse off. > This is why I think hiding things from drivers makes no sense. Also > consider the situations Linus W exposed on another subthread. If you > change ordering of certain calls, you will really break the > functionality of the IP. Because we can't make sure this won't work > automagically in all cases (just like we can't make sure $size memory > allocation is enough for all drivers) we don't hide that from the > driver. We require driver to manage its resources properly. We need some place to put the SoC integration; power domains seem like the obvious place to me but YMMV. Nothing about having this out of the drivers requires that this be done by individual subsystems in isolation from each other. Half the point here is that for the reusable IPs this stuff often isn't driver specific at all, it's often more about the SoC integration than it is about the driver and so you'll get a consistent pattern for most IPs on the SoC. > How can you make sure that this will work for at least 50% of the > drivers ? You just can't. We don't know the implementation details of > every arch/soc/platform supported by Linux today to make that decision. Well, we've managed to get along for rather a long time with essentially all architectures implementing this stuff by doing static setup for the pins on boot. That does suggest we can get a reasonably long way with something simple, and it does seem to match up with how things usually look at an electrical level too. It seems fairly obvious that if we need to add identical bolier plate code to lots of drivers we're doing something wrong, it's just churn for little practical gain and a problem if we ever decide to change how this stuff works in the future.
On Sun, Oct 28, 2012 at 09:12:52PM +0100, Linus Walleij wrote: > The answer is that it does not know. Because drivers have > different needs. Depending on how the hardware and > system is done. ... > I'm not making this up, it is a very real phenomenon on the > Ux500 and I don't think we are unique. > Moving this handling to bus code or anywhere else > invariably implies that resource acquisition/release order > does not matter, and my point is that it does. Doing this in the buses is definitely wrong, as you say it's not bus specific. I do however think we can usefully do this stuff in a SoC specific place like a power domain, keeping the SoC integration code together and out of the drivers. IME the SoCs where you need to do different things for different IPs shoudl mostly still get some reuse out of such an approach. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Tue, Oct 30, 2012 at 11:24:10AM +0000, Mark Brown wrote: > > This is why I think hiding things from drivers makes no sense. Also > > consider the situations Linus W exposed on another subthread. If you > > change ordering of certain calls, you will really break the > > functionality of the IP. Because we can't make sure this won't work > > automagically in all cases (just like we can't make sure $size memory > > allocation is enough for all drivers) we don't hide that from the > > driver. We require driver to manage its resources properly. > > We need some place to put the SoC integration; power domains seem like > the obvious place to me but YMMV. Nothing about having this out of the except that pin muxing has nothing to do with power domain. To me that sounds like an abuse of the API. > drivers requires that this be done by individual subsystems in isolation > from each other. Half the point here is that for the reusable IPs this > stuff often isn't driver specific at all, it's often more about the SoC > integration than it is about the driver and so you'll get a consistent > pattern for most IPs on the SoC. and all of that SoC-specific detail is already hidden behind power domains, runtime PM, pinctrl, clk API, regulator framework, etc. > > How can you make sure that this will work for at least 50% of the > > drivers ? You just can't. We don't know the implementation details of > > every arch/soc/platform supported by Linux today to make that decision. > > Well, we've managed to get along for rather a long time with essentially > all architectures implementing this stuff by doing static setup for the > pins on boot. That does suggest we can get a reasonably long way with and this is one of the issues we're all trying to solve today so we have single zImage approach for the ARM port. > something simple, and it does seem to match up with how things usually > look at an electrical level too. simple ? I really doubt it. Just look at the amount of code duplication the ARM port had (still has in some places) to handle platform-specific details. It turned out that drivers weren't very portable when they had to do platform-specific initialization, we were all abusing platform_data to pass strings and/or function pointers down to drivers and so on. I'm concerned if we hide pinctrl under e.g. power domains (as said above, it sounds like an abuse of the API to me) we will end up with a situation like above. Maybe not as bad, but still weird-looking. > It seems fairly obvious that if we need to add identical bolier plate > code to lots of drivers we're doing something wrong, it's just churn for > little practical gain and a problem if we ever decide to change how this > stuff works in the future. I wouldn't consider it boilerplate if you remember that each driver might have different requirements regarding how all of those details need to be handled. We have to consider power consumption, ordering of calls, proper IP setup, IP reuse on multiple platforms (even multiple ARCHes), etc etc, and to get that right outside of the driver - who's the only class that actually knows what it needs to do with its resources - will just be too complex and error-prone. I would strongly suggest starting with explicit calls to pinctrl, clk API, etc and if we can really prove later that 95% of the users are "standard", then we can factor code out, but making that assumption now is quite risky IMHO.
On Tue, Oct 30, 2012 at 12:34 PM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Sun, Oct 28, 2012 at 09:12:52PM +0100, Linus Walleij wrote: >> Moving this handling to bus code or anywhere else >> invariably implies that resource acquisition/release order >> does not matter, and my point is that it does. > > Doing this in the buses is definitely wrong, as you say it's not bus > specific. I do however think we can usefully do this stuff in a SoC > specific place like a power domain, keeping the SoC integration code > together and out of the drivers. IME the SoCs where you need to do > different things for different IPs shoudl mostly still get some reuse > out of such an approach. Talking to Kevin Hilman today he was also stressing that power domains is a good thing for handling resources, especially when replacing prior hacks in the custom clk code. However he pointed specifically to clocks and voltages, which may be true. What worries me is when knowledge of the hardware which is traditionally a concern for the device driver start to bubble up to the power domain (or better renamed resource domain if use like this, as Felipe points out). I worry that we will end up with power/resource domain code that start to look like this: suspend() switch (device.id) { case DEV_FOO: clk_disable(); pinctrl_set_state(); power_domain_off(); case DEV_BAR: pinctrl_set_state(); clk_disable(); // Always-on domain case DEV_BAZ: pinctrl_set_state(); clk_disable(); power_domain_off(); case ... Mutate the above with silicon errata, specific tweaks etc that Felipe was mentioning. What is happening is that device-specific behaviour which traditionally handled in the driver is now inside the power/resource domain. I agree that if the domain was doing the same thing for each piece of hardware, this would be the right thing to do, and I think the in-kernel examples are all "simple", e.g. arch/arm/mach-omap2/powerdomain* is all about power domains and nothing else, and the notifiers used by SHmobile is all about clock gating and nothing else. Another effect is that moving all resource handling to power domains is that if we do that for a widely shared device driver like the PL011 that mandates that power domains need to be implemented for U300, Ux500, Nomadik, SPEAr 13xx, 3xx, 6xx, Versatile, Versatile Express, Integrator (AP & CP) and BCM2835. Probably more. None of which has power domains (upstream) as of today. Some of which (U300, Ux500, Nomadik, SPEAr, Integrator, BCM2835) implement pin control. Basically power (resource) domains have the side-effect of in this light not doing one thing (power domains) but instead imposing all-or-nothing imperialistic characteristics. While avoiding a set of distributed, optional pinctrl hooks, it mandates a central piece of upfront powerdomain boilerplate to be present in each and every platform that wants to control its pins. I think the lesser of two evils is the distributed approach, and then I'm talking about pinctrl only, disregarding the fact that clocks and power domains are basically subject to the same kind of argument. I still buy into the concept of using power domains for exactly power domains only. Arguably this is an elegance opinion... I worry that the per-SoC power domain implementation which will live in arch/arm/mach-* as of today will become the new board file problem, overburdening the arch/* tree. Maybe I'm mistaken as to the size of these things, but just doing ls arch/arm/mach-omap2/powerdomain* makes me start worrying. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 30, 2012 at 01:49:49PM +0200, Felipe Balbi wrote: > On Tue, Oct 30, 2012 at 11:24:10AM +0000, Mark Brown wrote: > > We need some place to put the SoC integration; power domains seem like > > the obvious place to me but YMMV. Nothing about having this out of the > except that pin muxing has nothing to do with power domain. To me that > sounds like an abuse of the API. Well, we can call the API Archibald if we like... what I mean is something that sits in the system below the device in pretty much exactly the way that power domains do. > > drivers requires that this be done by individual subsystems in isolation > > from each other. Half the point here is that for the reusable IPs this > > stuff often isn't driver specific at all, it's often more about the SoC > > integration than it is about the driver and so you'll get a consistent > > pattern for most IPs on the SoC. > and all of that SoC-specific detail is already hidden behind power > domains, runtime PM, pinctrl, clk API, regulator framework, etc. That's all getting rather open coded, especially if you're going to get down to a situation where you have varying ordering constraints between the different APIs on different SoCs. > > > How can you make sure that this will work for at least 50% of the > > > drivers ? You just can't. We don't know the implementation details of > > > every arch/soc/platform supported by Linux today to make that decision. > > Well, we've managed to get along for rather a long time with essentially > > all architectures implementing this stuff by doing static setup for the > > pins on boot. That does suggest we can get a reasonably long way with > and this is one of the issues we're all trying to solve today so we have > single zImage approach for the ARM port. I don't see the relevance of single zImage here; device tree is supposed to solve that one. > > something simple, and it does seem to match up with how things usually > > look at an electrical level too. > simple ? I really doubt it. Just look at the amount of code duplication > the ARM port had (still has in some places) to handle platform-specific > details. What I'm saying here is that I'm concerned about us ending up with more code duplication... > It turned out that drivers weren't very portable when they had to do > platform-specific initialization, we were all abusing platform_data to > pass strings and/or function pointers down to drivers and so on. > I'm concerned if we hide pinctrl under e.g. power domains (as said > above, it sounds like an abuse of the API to me) we will end up with a > situation like above. Maybe not as bad, but still weird-looking. Well, nothing's going to stop that happening if people are determined enough - one could equally see that there'll be flags getting passed to control the ordering of calls if things are open coded. I would expect that with a power domain style approach any data would be being passed directly and bypassing the driver completely. > > It seems fairly obvious that if we need to add identical bolier plate > > code to lots of drivers we're doing something wrong, it's just churn for > > little practical gain and a problem if we ever decide to change how this > > stuff works in the future. > I wouldn't consider it boilerplate if you remember that each driver > might have different requirements regarding how all of those details > need to be handled. Essentially all the patches I'm seeing adding pinctrl are totally mindless, most of them are just doing the initial setup on boot and not doing any active management at runtime at all. > We have to consider power consumption, ordering of calls, proper IP > setup, IP reuse on multiple platforms (even multiple ARCHes), etc etc, > and to get that right outside of the driver - who's the only class that > actually knows what it needs to do with its resources - will just be too > complex and error-prone. A big part of my point here is that it's not at all clear to me that it is the driver which knows what's going on. For SoC-specific IPs you can be confident about how the SoC integration looks but for IPs that get reused widely this becomes less true. > I would strongly suggest starting with explicit calls to pinctrl, clk > API, etc and if we can really prove later that 95% of the users are > "standard", then we can factor code out, but making that assumption now > is quite risky IMHO. Like I say I think we're already seeing a pattern here, the code going into most drivers looks *very* similar with lots of the drivers simply calling get_select_default().
On Tue, Oct 30, 2012 at 12:49 PM, Felipe Balbi <balbi@ti.com> wrote: > On Tue, Oct 30, 2012 at 11:24:10AM +0000, Mark Brown wrote: >> We need some place to put the SoC integration; power domains seem like >> the obvious place to me but YMMV. Nothing about having this out of the > > except that pin muxing has nothing to do with power domain. To me that > sounds like an abuse of the API. It could be renamed to "power resources" or something as long as it's related to resource handling related to the PM calls. But I worry that it violates the Unix principle to do one thing and one thing only. A device power resource framework goes in the opposite direction, trying to do a lot of unrelated things in a central place as opposed to distributing the task. >> drivers requires that this be done by individual subsystems in isolation >> from each other. Half the point here is that for the reusable IPs this >> stuff often isn't driver specific at all, it's often more about the SoC >> integration than it is about the driver and so you'll get a consistent >> pattern for most IPs on the SoC. > > and all of that SoC-specific detail is already hidden behind power > domains, runtime PM, pinctrl, clk API, regulator framework, etc. I agree. pinctrl has already done a fair job at trying to be abstract in the states requested from the core, in <linux/pinctrl/pinctrl-state.h>. And I accept the idea to try to centralize more as well, maybe as a helpful struct and some inlines for the pinctrl core. I think this is enough, and pushing all handles into central code creates a problem elsewhere. (But I'm not so certain ... so I might just change opinion one of those days depending on what arguments will be made.) Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 30, 2012 at 3:07 PM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > Essentially all the patches I'm seeing adding pinctrl are totally > mindless, most of them are just doing the initial setup on boot and not > doing any active management at runtime at all. None of the Ux500 pinctrl patches are like that. I concludes in some other mails that all Ux500 drivers will need to handle atleast two states (default and sleep) and some a third state (idle). And this is what we're doing in our patches. Arguably it can all be pushed to power domains, but that will as said mandate all affected systems to implement power domains, and effectively moving code from drivers/* to arch/arm/* in our case atleast. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 30, 2012 at 03:02:23PM +0100, Linus Walleij wrote: > I worry that we will end up with power/resource domain > code that start to look like this: > suspend() > switch (device.id) { > case DEV_FOO: > clk_disable(); > pinctrl_set_state(); > power_domain_off(); > case DEV_BAR: Well, if we're doing that then either we'd presumably either get the same result if we open code it in the drivers or whoever wrote the resource domain code for the platform should be creating multiple domains. > Another effect is that moving all resource handling to > power domains is that if we do that for a widely shared > device driver like the PL011 that mandates that power > domains need to be implemented for U300, Ux500, > Nomadik, SPEAr 13xx, 3xx, 6xx, Versatile, Versatile > Express, Integrator (AP & CP) and BCM2835. Probably > more. > Basically power (resource) domains have the side-effect > of in this light not doing one thing (power domains) but > instead imposing all-or-nothing imperialistic characteristics. For me that's happening anyway with explicit control, just in different places - for example the Freescale guys have had issues with IPs shared between m68k and i.MX requiring that stub clocks be provided on m68k for things that are always on there and similarly with all the platforms that get affected when some widely used chip acquires regulator support. It seems easier to organise things if the platform has responsibility for coordinating this stuff than if we add stuff in the drivers. > I worry that the per-SoC power domain implementation > which will live in arch/arm/mach-* as of today will become > the new board file problem, overburdening the arch/* tree. > Maybe I'm mistaken as to the size of these things, > but just doing ls arch/arm/mach-omap2/powerdomain* > makes me start worrying. One thing to remember is that OMAP has some of the most featureful hardware out there in terms of software control for power so it's unlikely to ever get much more complex than that. IME most SoCs are very much simpler than that and should be able to punt lots of stuff to device tree data.
On Tue, Oct 30, 2012 at 03:16:02PM +0100, Linus Walleij wrote: > On Tue, Oct 30, 2012 at 3:07 PM, Mark Brown > > Essentially all the patches I'm seeing adding pinctrl are totally > > mindless, most of them are just doing the initial setup on boot and not > > doing any active management at runtime at all. > I concludes in some other mails that all Ux500 drivers > will need to handle atleast two states (default and sleep) > and some a third state (idle). Right, that's the other common option and obviously it's just an extension of the simple hogs which maps very nicely onto the existing PM states for devices. > And this is what we're doing in our patches. > Arguably it can all be pushed to power domains, but that > will as said mandate all affected systems to implement > power domains, and effectively moving code from > drivers/* to arch/arm/* in our case atleast. As soon as they start adding clock support and so on, yes. Obviously if they don't want to use any of the features that are offloaded like this then they could happily ignore it.
Hi, On Tue, Oct 30, 2012 at 02:07:15PM +0000, Mark Brown wrote: > On Tue, Oct 30, 2012 at 01:49:49PM +0200, Felipe Balbi wrote: > > On Tue, Oct 30, 2012 at 11:24:10AM +0000, Mark Brown wrote: > > > > We need some place to put the SoC integration; power domains seem like > > > the obvious place to me but YMMV. Nothing about having this out of the > > > except that pin muxing has nothing to do with power domain. To me that > > sounds like an abuse of the API. > > Well, we can call the API Archibald if we like... what I mean is I'm sure you know it's not at all about the name and much more about the semantics ;-) > something that sits in the system below the device in pretty much > exactly the way that power domains do. > > > > drivers requires that this be done by individual subsystems in isolation > > > from each other. Half the point here is that for the reusable IPs this > > > stuff often isn't driver specific at all, it's often more about the SoC > > > integration than it is about the driver and so you'll get a consistent > > > pattern for most IPs on the SoC. > > > and all of that SoC-specific detail is already hidden behind power > > domains, runtime PM, pinctrl, clk API, regulator framework, etc. > > That's all getting rather open coded, especially if you're going to get > down to a situation where you have varying ordering constraints between > the different APIs on different SoCs. however we need to consider those cases right ? Otherwise we will endup pushing something to mainline which will have to be reverted couple weeks later after a big rant from Linus ;-) > > > > How can you make sure that this will work for at least 50% of the > > > > drivers ? You just can't. We don't know the implementation details of > > > > every arch/soc/platform supported by Linux today to make that decision. > > > > Well, we've managed to get along for rather a long time with essentially > > > all architectures implementing this stuff by doing static setup for the > > > pins on boot. That does suggest we can get a reasonably long way with > > > and this is one of the issues we're all trying to solve today so we have > > single zImage approach for the ARM port. > > I don't see the relevance of single zImage here; device tree is supposed > to solve that one. DT is part of the deal. DT alone will solve nothing. > > > something simple, and it does seem to match up with how things usually > > > look at an electrical level too. > > > simple ? I really doubt it. Just look at the amount of code duplication > > the ARM port had (still has in some places) to handle platform-specific > > details. > > What I'm saying here is that I'm concerned about us ending up with more > code duplication... a fair concern. > > It turned out that drivers weren't very portable when they had to do > > platform-specific initialization, we were all abusing platform_data to > > pass strings and/or function pointers down to drivers and so on. > > > I'm concerned if we hide pinctrl under e.g. power domains (as said > > above, it sounds like an abuse of the API to me) we will end up with a > > situation like above. Maybe not as bad, but still weird-looking. > > Well, nothing's going to stop that happening if people are determined > enough - one could equally see that there'll be flags getting passed to > control the ordering of calls if things are open coded. I would expect > that with a power domain style approach any data would be being passed > directly and bypassing the driver completely. situations like that would be a lot more rare in open coded case, don't you think ? Also a lot more local, since they will show up on a driver source code which is used in a small set of use cases, instead of being part of the pm domain implementation for the entire platform. > > > It seems fairly obvious that if we need to add identical bolier plate > > > code to lots of drivers we're doing something wrong, it's just churn for > > > little practical gain and a problem if we ever decide to change how this > > > stuff works in the future. > > > I wouldn't consider it boilerplate if you remember that each driver > > might have different requirements regarding how all of those details > > need to be handled. > > Essentially all the patches I'm seeing adding pinctrl are totally > mindless, most of them are just doing the initial setup on boot and not > doing any active management at runtime at all. have you considered that might be just a first step ? I have mentioned this before. We first add the bare minimum and work on PM optimizations later. You can be sure most likely those mindless patches you're seeing are probably building blocks for upcoming patches adding sleep/idle modes. > > We have to consider power consumption, ordering of calls, proper IP > > setup, IP reuse on multiple platforms (even multiple ARCHes), etc etc, > > and to get that right outside of the driver - who's the only class that > > actually knows what it needs to do with its resources - will just be too > > complex and error-prone. > > A big part of my point here is that it's not at all clear to me that it > is the driver which knows what's going on. For SoC-specific IPs you can > be confident about how the SoC integration looks but for IPs that get > reused widely this becomes less true. I don't think so. As long as we keep the meaning of the 'default' pinctrl state to mean that this is the working state for the IP, underlying pinctrl-$arch implementation should know how to set muxing up properly, no ? > > I would strongly suggest starting with explicit calls to pinctrl, clk > > API, etc and if we can really prove later that 95% of the users are > > "standard", then we can factor code out, but making that assumption now > > is quite risky IMHO. > > Like I say I think we're already seeing a pattern here, the code going > into most drivers looks *very* similar with lots of the drivers simply > calling get_select_default(). might be true, but we don't really know if those aren't building blocks for upcoming PM optimizations IMO.
On Tue, Oct 30, 2012 at 05:16:42PM +0200, Felipe Balbi wrote: > On Tue, Oct 30, 2012 at 02:07:15PM +0000, Mark Brown wrote: > > > and all of that SoC-specific detail is already hidden behind power > > > domains, runtime PM, pinctrl, clk API, regulator framework, etc. > > That's all getting rather open coded, especially if you're going to get > > down to a situation where you have varying ordering constraints between > > the different APIs on different SoCs. > however we need to consider those cases right ? Otherwise we will endup > pushing something to mainline which will have to be reverted couple > weeks later after a big rant from Linus ;-) I'm not sure there's much risk of that either way; if anything it seems like it ought to be cleaner to have the stuff owned by the SoCs. > > > and this is one of the issues we're all trying to solve today so we have > > > single zImage approach for the ARM port. > > I don't see the relevance of single zImage here; device tree is supposed > > to solve that one. > DT is part of the deal. DT alone will solve nothing. If DT isn't relevant I'm not sure what you're saying about single zImage? The only relevance I can see for that is the data and configuration bloating the kernel, something that DT is supposed to address - this is the main use case where DT has benefits. > > Well, nothing's going to stop that happening if people are determined > > enough - one could equally see that there'll be flags getting passed to > > control the ordering of calls if things are open coded. I would expect > > that with a power domain style approach any data would be being passed > > directly and bypassing the driver completely. > situations like that would be a lot more rare in open coded case, don't > you think ? Also a lot more local, since they will show up on a driver > source code which is used in a small set of use cases, instead of being > part of the pm domain implementation for the entire platform. I don't see how open coding helps prevent people doing silly things, it seems like it'd have at most neutral impact (and of course it does require going round all the drivers every time someone comes up with a new idea for doing things which is a bit tedious). > > Essentially all the patches I'm seeing adding pinctrl are totally > > mindless, most of them are just doing the initial setup on boot and not > > doing any active management at runtime at all. > have you considered that might be just a first step ? I have mentioned > this before. We first add the bare minimum and work on PM optimizations > later. You can be sure most likely those mindless patches you're seeing > are probably building blocks for upcoming patches adding sleep/idle > modes. The sleep/idle modes are just a basic extension of the same idea, I'd not anticipate much difference there (and indeed anything where pinmux power saving makes a meaningful impact will I suspect need to be using runtime PM to allow SoC wide power savings anyway). > > A big part of my point here is that it's not at all clear to me that it > > is the driver which knows what's going on. For SoC-specific IPs you can > > be confident about how the SoC integration looks but for IPs that get > > reused widely this becomes less true. > I don't think so. As long as we keep the meaning of the 'default' > pinctrl state to mean that this is the working state for the IP, > underlying pinctrl-$arch implementation should know how to set muxing up > properly, no ? But then this comes round to the mindless code that ought to be factored out :) Only the more interesting cases that do something unusual really register here.
Hi, On Tue, Oct 30, 2012 at 03:58:21PM +0000, Mark Brown wrote: > > > > and this is one of the issues we're all trying to solve today so we have > > > > single zImage approach for the ARM port. > > > > I don't see the relevance of single zImage here; device tree is supposed > > > to solve that one. > > > DT is part of the deal. DT alone will solve nothing. > > If DT isn't relevant I'm not sure what you're saying about single I didn't say DT is irrelevant. I said it's not the *only* thing. > zImage? The only relevance I can see for that is the data and > configuration bloating the kernel, something that DT is supposed to > address - this is the main use case where DT has benefits. > > > > Well, nothing's going to stop that happening if people are determined > > > enough - one could equally see that there'll be flags getting passed to > > > control the ordering of calls if things are open coded. I would expect > > > that with a power domain style approach any data would be being passed > > > directly and bypassing the driver completely. > > > situations like that would be a lot more rare in open coded case, don't > > you think ? Also a lot more local, since they will show up on a driver > > source code which is used in a small set of use cases, instead of being > > part of the pm domain implementation for the entire platform. > > I don't see how open coding helps prevent people doing silly things, it > seems like it'd have at most neutral impact (and of course it does > require going round all the drivers every time someone comes up with a > new idea for doing things which is a bit tedious). > > > > Essentially all the patches I'm seeing adding pinctrl are totally > > > mindless, most of them are just doing the initial setup on boot and not > > > doing any active management at runtime at all. > > > have you considered that might be just a first step ? I have mentioned > > this before. We first add the bare minimum and work on PM optimizations > > later. You can be sure most likely those mindless patches you're seeing > > are probably building blocks for upcoming patches adding sleep/idle > > modes. > > The sleep/idle modes are just a basic extension of the same idea, I'd > not anticipate much difference there (and indeed anything where pinmux > power saving makes a meaningful impact will I suspect need to be using > runtime PM to allow SoC wide power savings anyway). > > > > A big part of my point here is that it's not at all clear to me that it > > > is the driver which knows what's going on. For SoC-specific IPs you can > > > be confident about how the SoC integration looks but for IPs that get > > > reused widely this becomes less true. > > > I don't think so. As long as we keep the meaning of the 'default' > > pinctrl state to mean that this is the working state for the IP, > > underlying pinctrl-$arch implementation should know how to set muxing up > > properly, no ? > > But then this comes round to the mindless code that ought to be factored > out :) Only the more interesting cases that do something unusual really > register here. fair enough. I see your point. Not saying I agree though, just that this discussion has been flying for far too long, so feel free to provide patches implementing what you're defending here ;-) Guess code will speak for itself. On way or another, we need OMAP keypad driver working in mainline and I don't think your arguments are strong enough to keep $SUBJECT from being merged, unless you can provide something stable/tested for v3.8 merge window.
On Tue, Oct 30, 2012 at 07:25:13PM +0200, Felipe Balbi wrote: > On Tue, Oct 30, 2012 at 03:58:21PM +0000, Mark Brown wrote: > > > > But then this comes round to the mindless code that ought to be factored > > out :) Only the more interesting cases that do something unusual really > > register here. > > fair enough. I see your point. Not saying I agree though, just that this > discussion has been flying for far too long, so feel free to provide > patches implementing what you're defending here ;-) > > Guess code will speak for itself. On way or another, we need OMAP keypad > driver working in mainline Are you saying that introducing pincrtl infrastructure actually _broke_ the driver in mainline? Thanks.
On Tue, Oct 30, 2012 at 07:25:13PM +0200, Felipe Balbi wrote: > On Tue, Oct 30, 2012 at 03:58:21PM +0000, Mark Brown wrote: > > But then this comes round to the mindless code that ought to be factored > > out :) Only the more interesting cases that do something unusual really > > register here. > fair enough. I see your point. Not saying I agree though, just that this > discussion has been flying for far too long, so feel free to provide > patches implementing what you're defending here ;-) > Guess code will speak for itself. On way or another, we need OMAP keypad > driver working in mainline and I don't think your arguments are strong > enough to keep $SUBJECT from being merged, unless you can provide > something stable/tested for v3.8 merge window. Ship me an OMAP5 system and I might see what I can do :) More seriously the amount of time we seem to have been spending recently on changes which end up requiring us to go through essentially every driver and add code to them (often several times) doesn't seem like we're doing a good job here. pinctrl is really noticable because it's new but it's not the only thing. As a subsystem maintainer this code just makes me want to add new subsystem features to pull the code out of drivers but obviously that's not something that should be being done at the subsystem level.
Hi, On Tue, Oct 30, 2012 at 11:20:07AM -0700, Dmitry Torokhov wrote: > On Tue, Oct 30, 2012 at 07:25:13PM +0200, Felipe Balbi wrote: > > On Tue, Oct 30, 2012 at 03:58:21PM +0000, Mark Brown wrote: > > > > > > But then this comes round to the mindless code that ought to be factored > > > out :) Only the more interesting cases that do something unusual really > > > register here. > > > > fair enough. I see your point. Not saying I agree though, just that this > > discussion has been flying for far too long, so feel free to provide > > patches implementing what you're defending here ;-) > > > > Guess code will speak for itself. On way or another, we need OMAP keypad > > driver working in mainline > > Are you saying that introducing pincrtl infrastructure actually _broke_ > the driver in mainline? no dude, I'm saying we need pinctrl working for this driver because we need to remove OMAP-specific MUX settings. One way or another, this has to work.
On Tue, Oct 30, 2012 at 7:37 PM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > More seriously the amount of time we seem to have been spending recently > on changes which end up requiring us to go through essentially every > driver and add code to them (often several times) doesn't seem like > we're doing a good job here. If this is your main concern you should be made aware that there are people out there planning to supplant the existing DT probe paths that are now being added to each and every ARM-related driver with an ACPI probe path as ARM servers come into the picture. > pinctrl is really noticable because it's > new but it's not the only thing. As a subsystem maintainer this code > just makes me want to add new subsystem features to pull the code out of > drivers but obviously that's not something that should be being done at > the subsystem level. We did manage to drag the power/voltage domain per se out of the AMBA bus, and recommend that people (like us) do that business using the power domains. I think most people (including OMAP) have bought into the concept of using the runtime PM framework and power domains to control the power domain switches. It's this wider concept of using the loose concept "PM resource domains" to control also clocks and pins that is at stake, and so far the runtime PM core people (Rafael and Magnus) has not said much so I think we need some kind of indication from them as to what is to happen, long-term, with drivers handling their own clocks and pins. Should it be centralized or not? If it's to be centralized it needs to become a large piece of infrastructure refactoring and needs the attention of Linaro and the like to happen. I've CC:ed a few people into this thread so we get some traction, we need more subsystem maintainers in here. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, October 30, 2012 10:51:11 PM Linus Walleij wrote: > On Tue, Oct 30, 2012 at 7:37 PM, Mark Brown > <broonie@opensource.wolfsonmicro.com> wrote: > > > More seriously the amount of time we seem to have been spending recently > > on changes which end up requiring us to go through essentially every > > driver and add code to them (often several times) doesn't seem like > > we're doing a good job here. > > If this is your main concern you should be made aware that there > are people out there planning to supplant the existing DT probe paths > that are now being added to each and every ARM-related driver > with an ACPI probe path as ARM servers come into the picture. That's correct. > > pinctrl is really noticable because it's > > new but it's not the only thing. As a subsystem maintainer this code > > just makes me want to add new subsystem features to pull the code out of > > drivers but obviously that's not something that should be being done at > > the subsystem level. > > We did manage to drag the power/voltage domain per se out > of the AMBA bus, and recommend that people (like us) do that > business using the power domains. > > I think most people (including OMAP) have bought > into the concept of using the runtime PM framework and power > domains to control the power domain switches. > > It's this wider concept of using the loose concept "PM resource > domains" to control also clocks and pins that is at stake, and so > far the runtime PM core people (Rafael and Magnus) has not said > much so I think we need some kind of indication from them as to > what is to happen, long-term, with drivers handling their own clocks > and pins. Should it be centralized or not? If it's to be centralized it > needs to become a large piece of infrastructure refactoring and > needs the attention of Linaro and the like to happen. Well, I personally think it should be centralized somehow. I'm not quite sure how to achieve that, though. Thanks, Rafael
On 21:12 Sun 28 Oct , Linus Walleij wrote: > On Wed, Oct 24, 2012 at 7:28 PM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > >> drivers/spi/spi-pl022.c > > > > Default/sleep transitions could be moved into bus code. > > No that's not a good idea as long as we have both the platform bus > and the AMBA bus doing essentially the same thing. We will then be > having two copies of the same code in two different busses running > out of sync. There may be other busses too. > > But I could prepare static helpers in <linux/pinctrl/consumer.h> > that any bus could use. Or any driver. Probably any driver, > because of this: > > As noted the bus cannot really execute the pinctrl calls to > e.g. put a drivers pins into "sleep". Since if e.g. the bus is walking > the suspend() ladder, shall it put the pins into sleep *before* > or *after* calling the suspend() hook in the driver? > > The answer is that it does not know. Because drivers have > different needs. Depending on how the hardware and > system is done. > > I already tried to make this point: > > pinctrl_set_state(state_sleep); > clk_disable(); > power_off_voltage_domain(); > > May for some drivers have to be: > > clk_disable(); > power_off_voltage_domain(); > pinctrl_set_state(state_sleep); > > (etc) > > I'm not making this up, it is a very real phenomenon on the > Ux500 and I don't think we are unique. I agree with Linus you can not handle pinctrl as bus levell as each driver need different requirement before enabling the pin as example you may need to clock the ip before muxing the pin and invertly on some IP the pinctrl is optionnal, on other mandatory you can not expect every driver to have the same need and requirement yes we will have a few code duplication but today it's few lines and those few lines will be place at different init stage on the drivers ditto for remove or sleep/idle > > Moving this handling to bus code or anywhere else > invariably implies that resource acquisition/release order > does not matter, and my point is that it does. 100% agreed Best Regards, J. > > >> drivers/i2c/busses/i2c-nomadik.c > > > > Don't see pinctrl in linux-next. > > This code is here: > http://marc.info/?l=linux-i2c&m=134986995731695&w=2 > > Yours, > Linus Walleij > _______________________________________________ > devicetree-discuss mailing list > devicetree-discuss@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/devicetree-discuss -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Linus Walleij <linus.walleij@linaro.org> writes: > On Tue, Oct 30, 2012 at 12:34 PM, Mark Brown > <broonie@opensource.wolfsonmicro.com> wrote: >> On Sun, Oct 28, 2012 at 09:12:52PM +0100, Linus Walleij wrote: > >>> Moving this handling to bus code or anywhere else >>> invariably implies that resource acquisition/release order >>> does not matter, and my point is that it does. >> >> Doing this in the buses is definitely wrong, as you say it's not bus >> specific. I do however think we can usefully do this stuff in a SoC >> specific place like a power domain, keeping the SoC integration code >> together and out of the drivers. IME the SoCs where you need to do >> different things for different IPs shoudl mostly still get some reuse >> out of such an approach. > > Talking to Kevin Hilman today he was also stressing that > power domains is a good thing for handling resources, especially > when replacing prior hacks in the custom clk code. However > he pointed specifically to clocks and voltages, which may > be true. > > What worries me is when knowledge of the hardware which > is traditionally a concern for the device driver start to > bubble up to the power domain (or better renamed resource > domain if use like this, as Felipe points out). > > I worry that we will end up with power/resource domain > code that start to look like this: > > suspend() > switch (device.id) { > case DEV_FOO: > clk_disable(); > pinctrl_set_state(); > power_domain_off(); > case DEV_BAR: > pinctrl_set_state(); > clk_disable(); > // Always-on domain > case DEV_BAZ: > pinctrl_set_state(); > clk_disable(); > power_domain_off(); > case ... > > Mutate the above with silicon errata, specific tweaks etc that > Felipe was mentioning. like this, as well as a bunch more. This is why we have a generic description of IP blocks (omap_hwmod) which abstracts all of these differences and keeps the PM domain layer rather simple. I agree with Mark. Either you have to take care of this with conditional code in the driver, and the drivers become bloated with a mess of SoC integration details, or you hide it away in SoC-specific code that can handle this, and keep the drivers portable. Now that we have PM domains (PM domains didn't exist when we created omap_device/omap_hwmod), I suspect the cleanest way to do this is to create separate PM domains for each "class" of devices that have different set of behavior. > What is happening is that device-specific behaviour which > traditionally handled in the driver is now inside the > power/resource domain. > > piece of hardware, this would be the right thing to do, > and I think the in-kernel examples are all "simple", > e.g. arch/arm/mach-omap2/powerdomain* is all about > power domains and nothing else, FYI... that code isn't the same as PM domain. That code is for the *hardware* powerdomains, not the software concept of "PM domain." In OMAP, PM domain is implmented at the omap_device level. And omap_device is the abstraction of an IP block that knows about all the PM related register settings, clocks, HW powerdomain, voltage domain, PM related pin-muxing etc. etc. All of these things are abstracted in an omap_device, so that the PM domain implementation for OMAP looks rather simple (c.f. omap_device_pm_domain in arch/arm/plat-omap/omap_device.c.) Note that the callbacks just call omap_device_enable(), omap_device_disable() and all the HW ugliness, SoC-specific integration mess is hidden away. [...] > I think the lesser of two evils is the distributed approach, > and then I'm talking about pinctrl only, disregarding the > fact that clocks and power domains are basically subject to > the same kind of argument. I still buy into the concept of > using power domains for exactly power domains only. > Arguably this is an elegance opinion... The pinctrl examples I've seen mentioned so far are all PM related (sleep, idle, wakeup, etc.) so to me I think they still belong in PM domains (and that's how we handle the PM related pins in OMAP.) > I worry that the per-SoC power domain implementation > which will live in arch/arm/mach-* as of today will become > the new board file problem, overburdening the arch/* tree. > Maybe I'm mistaken as to the size of these things, > but just doing ls arch/arm/mach-omap2/powerdomain* > makes me start worrying. Yes, I agree that this means more code/data in arch/arm/mach-*, but IMO, that's really where it belongs. It really is SoC integration details, and driver should really not know about it. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 31, 2012 at 9:10 PM, Kevin Hilman <khilman@deeprootsystems.com> wrote: > Linus Walleij <linus.walleij@linaro.org> writes: > > > piece of hardware, this would be the right thing to do, > > and I think the in-kernel examples are all "simple", > > e.g. arch/arm/mach-omap2/powerdomain* is all about > > power domains and nothing else, > > FYI... that code isn't the same as PM domain. This sort of points to a core problem here. Our terminologies are ambiguous that we cannot understand each others code. As long as <linux/pm_domain.h> begins: /* * pm_domain.h - Definitions and headers related to device power domains. * But arguably that should just be patched (I think there are a few remnants in the code still implying that these things are only about power). > > That code is for the > *hardware* powerdomains, not the software concept of "PM domain." In > OMAP, PM domain is implmented at the omap_device level. And omap_device > is the abstraction of an IP block that knows about all the PM related > register settings, clocks, HW powerdomain, voltage domain, PM related > pin-muxing etc. etc. All of these things are abstracted in an > omap_device, so that the PM domain implementation for OMAP looks rather > simple (c.f. omap_device_pm_domain in arch/arm/plat-omap/omap_device.c.) OK following now... > > > I think the lesser of two evils is the distributed approach, > > The pinctrl examples I've seen mentioned so far are all PM related > > (sleep, idle, wakeup, etc.) so to me I think they still belong in > PM domains (and that's how we handle the PM related pins in OMAP.) Well, the pinctrl grabbers in these drivers are using these states also for platforms that do not even select CONFIG_PM. For example mach-nomadik is quite happy that the PL011 driver is thusly muxing in its pins. And would require refactoring to use PM domains. So basically this requirement comes down to: - When dealing with a SoC IP block driver - That need to multiplex pins - Then your SoC must select CONFIG_PM and CONFIG_PM_RUNTIME and CONFIG_PM_GENERIC_DOMAINS and implement proper domain handling hooks. Is this correct? And for Mark, Dmitry, does this correspond to your view? It's actually something that needs to be acknowledged by the ARM SoC maintainers, because they will be the ones telling all subarch maintainers to go implement full PM handling with these three frameworks whenever an SoC driver want to handle pins. Bascially all subsystem maintainers dealing with embedded SoCs need to be aligned on this as well... And IIUC not only pins but also silicon block clocks? I can surely fix these for "my" systems, but it really needs to be enforced widely or it will be a mess. > > > I worry that the per-SoC power domain implementation > > which will live in arch/arm/mach-* as of today will become > > the new board file problem, overburdening the arch/* tree. > > Maybe I'm mistaken as to the size of these things, > > but just doing ls arch/arm/mach-omap2/powerdomain* > > makes me start worrying. > > Yes, I agree that this means more code/data in arch/arm/mach-*, but > IMO, that's really where it belongs. It really is SoC integration > details, and driver should really not know about it. OK we need feedback from ARM SoC on this. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Linus Walleij <linus.walleij@linaro.org> writes: > On Wed, Oct 31, 2012 at 9:10 PM, Kevin Hilman > <khilman@deeprootsystems.com> wrote: > >> Linus Walleij <linus.walleij@linaro.org> writes: > > >> >> > piece of hardware, this would be the right thing to do, >> > and I think the in-kernel examples are all "simple", >> > e.g. arch/arm/mach-omap2/powerdomain* is all about >> > power domains and nothing else, >> >> FYI... that code isn't the same as PM domain. > > > This sort of points to a core problem here. Our terminologies are > ambiguous that we cannot understand each others code. As long > as <linux/pm_domain.h> begins: > > /* > * pm_domain.h - Definitions and headers related to device power domains. > * > > But arguably that should just be patched (I think there are a few > remnants in the code still implying that these things are only about > power). Agreed. The terminology is confusing, and any situations like this in the code/comments/docs should be patched. When PM domains were introduced, I was the first to complain that we shouldn't use the term power domain so as not to be confused with HW concepts, so we settled on the term 'PM domain.' Ultimately, it's just a configurable grouping of devices whose callbacks happen during PM transitions. >> >> That code is for the >> *hardware* powerdomains, not the software concept of "PM domain." In >> OMAP, PM domain is implmented at the omap_device level. And omap_device >> is the abstraction of an IP block that knows about all the PM related >> register settings, clocks, HW powerdomain, voltage domain, PM related >> pin-muxing etc. etc. All of these things are abstracted in an >> omap_device, so that the PM domain implementation for OMAP looks rather >> simple (c.f. omap_device_pm_domain in arch/arm/plat-omap/omap_device.c.) > > > OK following now... > >> >> > I think the lesser of two evils is the distributed approach, >> >> The pinctrl examples I've seen mentioned so far are all PM related >> >> (sleep, idle, wakeup, etc.) so to me I think they still belong in >> PM domains (and that's how we handle the PM related pins in OMAP.) > > > Well, the pinctrl grabbers in these drivers are using these states also > for platforms that do not even select CONFIG_PM. For example > mach-nomadik is quite happy that the PL011 driver is thusly > muxing in its pins. And would require refactoring to use PM > domains. If CONFIG_PM is disabled, then is it safe to assume that the pins in question are probably only done once at init time. I assume during ->probe(). ? > > So basically this requirement comes down to: > > - When dealing with a SoC IP block driver > > - That need to multiplex pins > > - Then your SoC must select CONFIG_PM and > CONFIG_PM_RUNTIME andb > CONFIG_PM_GENERIC_DOMAINS and implement > proper domain handling hooks. > > Is this correct? I would say yes. Currently, PM domains are the way to hook SoC-specific integration details into PM transitions. However, if what we want/need are only ways to introduce SoC-specific integration details into non-PM transitions (e.g. probe/remove), maybe bus notifiers would suffice here. e.g. you'd get a bus notifier when the device is added/attached and any init-time pinctrl setup could be done then. This still keeps drivers clean of SoC-specific integration data/code, and also allows that to happen whether or not PM features are enabled. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 01, 2012 at 09:54:00AM +0100, Linus Walleij wrote: > Well, the pinctrl grabbers in these drivers are using these states also > for platforms that do not even select CONFIG_PM. For example > mach-nomadik is quite happy that the PL011 driver is thusly > muxing in its pins. And would require refactoring to use PM > domains. > So basically this requirement comes down to: > - When dealing with a SoC IP block driver > - That need to multiplex pins > - Then your SoC must select CONFIG_PM and > CONFIG_PM_RUNTIME and > CONFIG_PM_GENERIC_DOMAINS and implement > proper domain handling hooks. > Is this correct? And for Mark, Dmitry, does this correspond to > your view? For the pin hogging I'd actually been thinking separately that we should just have the device core do a devm_pinctrl_get_set_default() prior to probing the device and store the result in the struct device. That would immediately remove almost all of the current pinctrl users, users that do need to do things with the data or check the result can then pick up the pinctrl pointer from the device struct. > It's actually something that needs to be acknowledged by the > ARM SoC maintainers, because they will be the ones telling > all subarch maintainers to go implement full PM handling > with these three frameworks whenever an SoC driver want > to handle pins. Well, they're going to have to implement it somewhere anyway - either in the drivers or in the SoC stuff. > And IIUC not only pins but also silicon block clocks? > I can surely fix these for "my" systems, but it really needs > to be enforced widely or it will be a mess. We definitely need to decide if it's something that should be open coded everywhere.
On Thu, Nov 1, 2012 at 12:42 PM, Kevin Hilman <khilman@deeprootsystems.com> wrote: > [Me] >> Well, the pinctrl grabbers in these drivers are using these states also >> for platforms that do not even select CONFIG_PM. For example >> mach-nomadik is quite happy that the PL011 driver is thusly >> muxing in its pins. And would require refactoring to use PM >> domains. > > If CONFIG_PM is disabled, then is it safe to assume that the pins in > question are probably only done once at init time. I assume during > ->probe(). ? Sadly no. Consider drivers/tty/serial/amba-pl011.c Many ARM platforms have several instances of PL011, and not all of them have CONFIG_PM & friends, so it's a good example. Here the driver will probe and currently fetch a pinctrl handle and looks up two states: "default", which refers to the situation you describe, and optionally "sleep" which will put pins into a low-power state. The driver will currently put the pins into the "sleep" state when .shutdown() is called by something (userspace or in-kernel users). So in the new suggested scheme using runtime PM, this would have to be replaced by pm_runtime_get[_sync]() and pm_runtime_put() hints and the current pin handling deleted, and for each platform using this driver instead implement a PM domain to do the same thing. Else you loose this runtime power optimization. This is what I refer to the all-or-nothing charcter of runtime PM domains... but maybe it's a good thing, I haven't quite made my mind up about it. > (...) if what we want/need are only ways to introduce SoC-specific > integration details into non-PM transitions (e.g. probe/remove), maybe > bus notifiers would suffice here. e.g. you'd get a bus notifier when > the device is added/attached and any init-time pinctrl setup could be > done then. This still keeps drivers clean of SoC-specific integration > data/code, and also allows that to happen whether or not PM features are > enabled. It doesn't cut it for any of our drivers as shown above, but it would work for the patch in $SUBJECT. It sounds like the way silicon clocks are handled on SH am I right? Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 1, 2012 at 1:07 PM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Thu, Nov 01, 2012 at 09:54:00AM +0100, Linus Walleij wrote: > For the pin hogging I'd actually been thinking separately that we should > just have the device core do a devm_pinctrl_get_set_default() prior to > probing the device and store the result in the struct device. That > would immediately remove almost all of the current pinctrl users, users > that do need to do things with the data or check the result can then > pick up the pinctrl pointer from the device struct. I never thought of that. This sounds like it would work. And the good thing is that this is a clean cut so we will centralized code without having to decide right now how to handle the pm idle/sleep cases. Talking here with Kevin Hilman on my left and Grant Likely on my right (they're physically here) there is some worry about stashing stuff into struct device. What if I retrieve this in the pinctrl subsystem using bus notifiers and then expose the struct pinctrl * to the clients by using pinctrl_get() and when you get such a handle in your probe() you know that the pinctrl subsystem has already fetched the handle and set it to "default" at that point? I just worry whether there is a fringe case where the default state is not be be default-selected in probe(), the API semantics does not mandate that. But I think this is the case for all in-kernel drivers so we wouldn't be breaking anything by doing this right now. And platforms can just leave the "default" state undefined in that case. >> It's actually something that needs to be acknowledged by the >> ARM SoC maintainers, because they will be the ones telling >> all subarch maintainers to go implement full PM handling >> with these three frameworks whenever an SoC driver want >> to handle pins. > > Well, they're going to have to implement it somewhere anyway - either in > the drivers or in the SoC stuff. Sure I just worry about it being done is several different ways and creating a mess so they need to be involved to block other approaches. >> I can surely fix these for "my" systems, but it really needs >> to be enforced widely or it will be a mess. > > We definitely need to decide if it's something that should be open coded > everywhere. If I prepare a patch to move the fetch+set defaul to the pinctrl core using notifiers, we centralize one piece and we get the currently floating patches out of the way. Then what to do with sleep and idle is a question we need to handle next. Requiring PM domains for this is one approach, albeit a bit controversial. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 01, 2012 at 03:01:28PM +0100, Linus Walleij wrote: > On Thu, Nov 1, 2012 at 1:07 PM, Mark Brown > > > For the pin hogging I'd actually been thinking separately that we should > > just have the device core do a devm_pinctrl_get_set_default() prior to > > probing the device and store the result in the struct device. That > What if I retrieve this in the pinctrl subsystem using > bus notifiers and then expose the struct pinctrl * to > the clients by using pinctrl_get() and when you get > such a handle in your probe() you know that the > pinctrl subsystem has already fetched the handle and > set it to "default" at that point? I'm not sure I parse a problem from the above? > I just worry whether there is a fringe case where the default > state is not be be default-selected in probe(), the API > semantics does not mandate that. But I think this is the case > for all in-kernel drivers so we wouldn't be breaking anything > by doing this right now. And platforms can just leave the > "default" state undefined in that case. Yes, that had been my thinking too though I'd really expect that the platform ought to be able to think of something sensible to do by default. > Then what to do with sleep and idle is a question we need > to handle next. Requiring PM domains for this is one > approach, albeit a bit controversial. Yup. Notifiers are another option again I guess. There's far fewer drivers doing anything at all with that so it's a bit less urgent.
On Tue, Oct 30, 2012 at 10:51:11PM +0100, Linus Walleij wrote: > On Tue, Oct 30, 2012 at 7:37 PM, Mark Brown > > More seriously the amount of time we seem to have been spending recently > > on changes which end up requiring us to go through essentially every > > driver and add code to them (often several times) doesn't seem like > > we're doing a good job here. > If this is your main concern you should be made aware that there > are people out there planning to supplant the existing DT probe paths > that are now being added to each and every ARM-related driver > with an ACPI probe path as ARM servers come into the picture. That's different as we're adding support for a new external interface which will need per device configuration parsing rather than reorganising things within the kernel; I'd expect it won't supplant DT but rather sit alongside it as it's more a requirement for the server market than for the embedded market.
On Thu, Nov 1, 2012 at 3:01 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Thu, Nov 1, 2012 at 1:07 PM, Mark Brown > <broonie@opensource.wolfsonmicro.com> wrote: >> On Thu, Nov 01, 2012 at 09:54:00AM +0100, Linus Walleij wrote: > >> For the pin hogging I'd actually been thinking separately that we should >> just have the device core do a devm_pinctrl_get_set_default() prior to >> probing the device and store the result in the struct device. That >> would immediately remove almost all of the current pinctrl users, users >> that do need to do things with the data or check the result can then >> pick up the pinctrl pointer from the device struct. > > I never thought of that. This sounds like it would work. So I've looked closer at this. > And the good thing is that this is a clean cut so we > will centralized code without having to decide right now > how to handle the pm idle/sleep cases. > > Talking here with Kevin Hilman on my left and Grant > Likely on my right (they're physically here) there is some > worry about stashing stuff into struct device. > > What if I retrieve this in the pinctrl subsystem using > bus notifiers and then expose the struct pinctrl * to > the clients by using pinctrl_get() and when you get > such a handle in your probe() you know that the > pinctrl subsystem has already fetched the handle and > set it to "default" at that point? I have sent out an RFC for this approach. It actually works quite well on the U300 for example. However as stated in the patch we run into another problem: what if the pinctrl provider returns -EDEFER_PROBE? (The same will be true for clocks, regulators etc I guess...) If I use notifiers like this, I cannot return -EPROBE_DEFER to the core. :-( The PM domains seem to be built on the notification mechanism as well (AFAICT), so it will not allow drivers to defer their probes, as it is an optional layer after all. As a matter of fact it seems that there is an implicit assumption in PM domains that the resources that are taken there cannot defer any probing, so they are assumed to always be present. Is this correct? If so, any resources that may be deferred (such as pinctrl) can never be handled in PM domains. Only simple stuff that the SoC controls directly e.g through some fixed register pokes to voltage domains. So then the only option that remains to centralize pinctrl handling is indeed to do that in the device core, before probe(). I need to know Greg's feelings about that. At the same time this sort of give me the feeling that we might be doing something wrong. The distributed nature of deferred probes will become quite elusive if the deferral request comes from some place before probe() is even called on the driver. Please check my RFC patch and tell me above errors in the above reasoning.... Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/input/keyboard/omap4-keypad.c b/drivers/input/keyboard/omap4-keypad.c index c05f98c..502b832 100644 --- a/drivers/input/keyboard/omap4-keypad.c +++ b/drivers/input/keyboard/omap4-keypad.c @@ -31,6 +31,7 @@ #include <linux/input.h> #include <linux/slab.h> #include <linux/pm_runtime.h> +#include <linux/pinctrl/consumer.h> #include <linux/platform_data/omap4-keypad.h> @@ -76,6 +77,7 @@ enum { struct omap4_keypad { struct input_dev *input; + struct pinctrl *pins; void __iomem *base; unsigned int irq; @@ -298,6 +300,15 @@ static int __devinit omap4_keypad_probe(struct platform_device *pdev) goto err_release_mem; } + keypad_data->pins = devm_pinctrl_get_select_default(&pdev->dev); + if (IS_ERR(keypad_data->pins)) { + if (PTR_ERR(keypad_data->pins) == -EPROBE_DEFER) + return -EPROBE_DEFER; + + dev_warn(&pdev->dev, "did not get pins for keypad error: %li\n", + PTR_ERR(keypad_data->pins)); + keypad_data->pins = NULL; + } /* * Enable clocks for the keypad module so that we can read
Adapt keypad to use pinctrl framework. Tested on omap4430 sdp with 3.7-rc1 kernel. Cc: Felipe Balbi <balbi@ti.com> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com> --- v1->v2 - Added "PROBE_DEFER" check drivers/input/keyboard/omap4-keypad.c | 11 +++++++++++ 1 files changed, 11 insertions(+), 0 deletions(-)