| Message ID | 20160819235653.26355-4-nm@ti.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
+ Jon [...] > + > +static int ti_sci_pm_domains_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + struct ti_sci_genpd_data *ti_sci_genpd; > + > + ti_sci_genpd = devm_kzalloc(dev, sizeof(*ti_sci_genpd), GFP_KERNEL); > + if (!ti_sci_genpd) > + return -ENOMEM; > + > + ti_sci_genpd->ti_sci = devm_ti_sci_get_handle(dev); > + if (IS_ERR(ti_sci_genpd->ti_sci)) > + return PTR_ERR(ti_sci_genpd->ti_sci); > + > + ti_sci_genpd->dev = dev; > + > + INIT_LIST_HEAD(&ti_sci_genpd->pd_list); > + mutex_init(&ti_sci_genpd->pd_list_mutex); > + > + return __of_genpd_add_provider(np, of_ti_sci_genpd_xlate_onecell, > + ti_sci_genpd); Jon Hunter are working on adding robust method to be able to remove initialized genpds [1]. In that series we intend to remove the __of_genpd_add_provider() API, and instead only have of_genpd_add_provider_onecell() and of_genpd_add_provider_simple(). Could you please convert to use any of these APIs instead? Kind regards Uffe [1] http://www.spinics.net/lists/arm-kernel/msg524151.html -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 08/25/2016 02:27 AM, Ulf Hansson wrote: > + Jon > > [...] > >> + >> +static int ti_sci_pm_domains_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct device_node *np = dev->of_node; >> + struct ti_sci_genpd_data *ti_sci_genpd; >> + >> + ti_sci_genpd = devm_kzalloc(dev, sizeof(*ti_sci_genpd), GFP_KERNEL); >> + if (!ti_sci_genpd) >> + return -ENOMEM; >> + >> + ti_sci_genpd->ti_sci = devm_ti_sci_get_handle(dev); >> + if (IS_ERR(ti_sci_genpd->ti_sci)) >> + return PTR_ERR(ti_sci_genpd->ti_sci); >> + >> + ti_sci_genpd->dev = dev; >> + >> + INIT_LIST_HEAD(&ti_sci_genpd->pd_list); >> + mutex_init(&ti_sci_genpd->pd_list_mutex); >> + >> + return __of_genpd_add_provider(np, of_ti_sci_genpd_xlate_onecell, >> + ti_sci_genpd); > > Jon Hunter are working on adding robust method to be able to remove > initialized genpds [1]. > > In that series we intend to remove the __of_genpd_add_provider() API, > and instead only have of_genpd_add_provider_onecell() and > of_genpd_add_provider_simple(). Could you please convert to use any of > these APIs instead? Thanks for pointing this out. I took at look at the series you've linked and the short answer is that I see no good way to directly convert what we've done to use those APIs. On this platform each device has it's power state controlled through the SCI protocol described in the cover letter. The system makes a request for powering on or off the device using a unique ID number for each device, as provided in patches 1 and 2. These operations map to those of a genpd, so we decided to do a 1:1 device to genpd mapping, where each device has it's own genpd. The split that we took from the provided simple and onecell xlate functions arises from this mapping. The IDs are not necessarily linear and also they are not necessarily defined in a fixed way for all SoCs, they are entirely data driven based on the provided device ID. To make use of these IDs, I created a new xlate function that takes a onecell value but instead dynamically allocates a new genpd, at probe time, to give us a genpd that contains the necessary SoC specific data for that device that probed and is mapped directly to the device. This lets us only create the genpds we need without having to redefine a static list of all possible genpds and duplicate the data. So my question back would be, how critical is it to be able to drop the ability to provide custom xlate functions? Regards, Dave > > Kind regards > Uffe > > [1] > http://www.spinics.net/lists/arm-kernel/msg524151.html > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Jon, Ulf, On 08/26/2016 06:37 PM, Dave Gerlach wrote: > Hi, > On 08/25/2016 02:27 AM, Ulf Hansson wrote: >> + Jon >> >> [...] >> >>> + >>> +static int ti_sci_pm_domains_probe(struct platform_device *pdev) >>> +{ >>> + struct device *dev = &pdev->dev; >>> + struct device_node *np = dev->of_node; >>> + struct ti_sci_genpd_data *ti_sci_genpd; >>> + >>> + ti_sci_genpd = devm_kzalloc(dev, sizeof(*ti_sci_genpd), GFP_KERNEL); >>> + if (!ti_sci_genpd) >>> + return -ENOMEM; >>> + >>> + ti_sci_genpd->ti_sci = devm_ti_sci_get_handle(dev); >>> + if (IS_ERR(ti_sci_genpd->ti_sci)) >>> + return PTR_ERR(ti_sci_genpd->ti_sci); >>> + >>> + ti_sci_genpd->dev = dev; >>> + >>> + INIT_LIST_HEAD(&ti_sci_genpd->pd_list); >>> + mutex_init(&ti_sci_genpd->pd_list_mutex); >>> + >>> + return __of_genpd_add_provider(np, of_ti_sci_genpd_xlate_onecell, >>> + ti_sci_genpd); >> >> Jon Hunter are working on adding robust method to be able to remove >> initialized genpds [1]. >> >> In that series we intend to remove the __of_genpd_add_provider() API, >> and instead only have of_genpd_add_provider_onecell() and >> of_genpd_add_provider_simple(). Could you please convert to use any of >> these APIs instead? > > Thanks for pointing this out. I took at look at the series you've linked and the > short answer is that I see no good way to directly convert what we've done to > use those APIs. > > On this platform each device has it's power state controlled through the SCI > protocol described in the cover letter. The system makes a request for powering > on or off the device using a unique ID number for each device, as provided in > patches 1 and 2. These operations map to those of a genpd, so we decided to do a > 1:1 device to genpd mapping, where each device has it's own genpd. > > The split that we took from the provided simple and onecell xlate functions > arises from this mapping. The IDs are not necessarily linear and also they are > not necessarily defined in a fixed way for all SoCs, they are entirely data > driven based on the provided device ID. To make use of these IDs, I created a > new xlate function that takes a onecell value but instead dynamically allocates > a new genpd, at probe time, to give us a genpd that contains the necessary SoC > specific data for that device that probed and is mapped directly to the device. > This lets us only create the genpds we need without having to redefine a static > list of all possible genpds and duplicate the data. > > So my question back would be, how critical is it to be able to drop the ability > to provide custom xlate functions? After thinking about this a bit more, I believe I see a way we can use the of_genpd_add_provider_onecell, although not optimally. The device IDs, and therefore the genpd IDs (in the last email I mentioned we are doing a 1:1 device to genpd mapping) are fixed and defined by the hardware, and are not linear, there can be gaps and we won't necessarily always start at 0 even though we do on this SoC. However, we could build an array of genpds that map to our IDs similar to how several have done it, like in drivers/soc/bcm/raspberrypi-power.c, but because our IDs can have gaps, there will be unused struct generic_pm_domains that get allocated and are never touched because the index of the element doesn't correspond to a genpd ID. Based on the ID set provided in patch 2 of this series I see 12 gaps, so we'd be wasting space the size of 12 genpds. The ID mapping will change on future SoCs, so this number could be larger. Do you think this is an acceptable solution? It allows us to play nice with the new genpd framework changes at the cost of wasting some space allocated to filler genpds. Regards, Dave > > Regards, > Dave > >> >> Kind regards >> Uffe >> >> [1] >> http://www.spinics.net/lists/arm-kernel/msg524151.html >> > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 30 August 2016 at 21:43, Dave Gerlach <d-gerlach@ti.com> wrote: > Jon, Ulf, > > On 08/26/2016 06:37 PM, Dave Gerlach wrote: >> >> Hi, >> On 08/25/2016 02:27 AM, Ulf Hansson wrote: >>> >>> + Jon >>> >>> [...] >>> >>>> + >>>> +static int ti_sci_pm_domains_probe(struct platform_device *pdev) >>>> +{ >>>> + struct device *dev = &pdev->dev; >>>> + struct device_node *np = dev->of_node; >>>> + struct ti_sci_genpd_data *ti_sci_genpd; >>>> + >>>> + ti_sci_genpd = devm_kzalloc(dev, sizeof(*ti_sci_genpd), >>>> GFP_KERNEL); >>>> + if (!ti_sci_genpd) >>>> + return -ENOMEM; >>>> + >>>> + ti_sci_genpd->ti_sci = devm_ti_sci_get_handle(dev); >>>> + if (IS_ERR(ti_sci_genpd->ti_sci)) >>>> + return PTR_ERR(ti_sci_genpd->ti_sci); >>>> + >>>> + ti_sci_genpd->dev = dev; >>>> + >>>> + INIT_LIST_HEAD(&ti_sci_genpd->pd_list); >>>> + mutex_init(&ti_sci_genpd->pd_list_mutex); >>>> + >>>> + return __of_genpd_add_provider(np, >>>> of_ti_sci_genpd_xlate_onecell, >>>> + ti_sci_genpd); >>> >>> >>> Jon Hunter are working on adding robust method to be able to remove >>> initialized genpds [1]. >>> >>> In that series we intend to remove the __of_genpd_add_provider() API, >>> and instead only have of_genpd_add_provider_onecell() and >>> of_genpd_add_provider_simple(). Could you please convert to use any of >>> these APIs instead? >> >> >> Thanks for pointing this out. I took at look at the series you've linked >> and the >> short answer is that I see no good way to directly convert what we've done >> to >> use those APIs. >> >> On this platform each device has it's power state controlled through the >> SCI >> protocol described in the cover letter. The system makes a request for >> powering >> on or off the device using a unique ID number for each device, as provided >> in >> patches 1 and 2. These operations map to those of a genpd, so we decided >> to do a >> 1:1 device to genpd mapping, where each device has it's own genpd. >> >> The split that we took from the provided simple and onecell xlate >> functions >> arises from this mapping. The IDs are not necessarily linear and also they >> are >> not necessarily defined in a fixed way for all SoCs, they are entirely >> data >> driven based on the provided device ID. To make use of these IDs, I >> created a >> new xlate function that takes a onecell value but instead dynamically >> allocates >> a new genpd, at probe time, to give us a genpd that contains the necessary >> SoC >> specific data for that device that probed and is mapped directly to the >> device. >> This lets us only create the genpds we need without having to redefine a >> static >> list of all possible genpds and duplicate the data. >> >> So my question back would be, how critical is it to be able to drop the >> ability >> to provide custom xlate functions? > > > After thinking about this a bit more, I believe I see a way we can use the > of_genpd_add_provider_onecell, although not optimally. The device IDs, and > therefore the genpd IDs (in the last email I mentioned we are doing a 1:1 > device to genpd mapping) are fixed and defined by the hardware, and are not > linear, there can be gaps and we won't necessarily always start at 0 even > though we do on this SoC. However, we could build an array of genpds that > map to our IDs similar to how several have done it, like in > drivers/soc/bcm/raspberrypi-power.c, but because our IDs can have gaps, > there will be unused struct generic_pm_domains that get allocated and are > never touched because the index of the element doesn't correspond to a genpd > ID. There are a couple of ways to solve your problem without having to create one genpd instance for each of the platform devices. As a matter of fact, I think that approach should be avoided if possible. Genpd isn't designed for these kind purposes, even if it can be done like that. Here are some ideas which could be used to solve the problem differently. *) Assuming each platform device has a driver for it!? Then why don't you implement the ->runtime_suspend|resume() callbacks on the driver level and call the SCI interface to power on/off the device from there? This ought to be the most straight forward solution. **) You may also use genpd's (struct generic_pm_domain) ->attach|detach_dev() callbacks to create the device specific data (the SCI device ID). The ->attach_dev() callback are invoked when a device gets attached to its PM domain (genpd) at probe time. Currently these callbacks are already being used by SoCs that uses the PM clk API from a genpd client. That is needed to create the device specific PM clock list. We would have to do something similar here for the SCI device IDs. Then, you must also assign/implement the ->start() and ->stop() callbacks of the struct gpd_dev_ops, which is a part of the genpd struct. These callbacks can then easily invoke SoC specific code/APIs and perhaps that is the issue with my first suggested approach!? > > Based on the ID set provided in patch 2 of this series I see 12 gaps, so > we'd be wasting space the size of 12 genpds. The ID mapping will change on > future SoCs, so this number could be larger. Do you think this is an > acceptable solution? It allows us to play nice with the new genpd framework > changes at the cost of wasting some space allocated to filler genpds. There are other issues as well, which mostly has do to with a unnecessary long execution path, involving taking mutexes etc in genpd. All you actually need, is to be able to power on/off a device from a driver's ->runtime_suspend|resume() callback. Don't you think? Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/30/2016 03:26 PM, Ulf Hansson wrote: > On 30 August 2016 at 21:43, Dave Gerlach <d-gerlach@ti.com> wrote: >> Jon, Ulf, >> >> On 08/26/2016 06:37 PM, Dave Gerlach wrote: >>> >>> Hi, >>> On 08/25/2016 02:27 AM, Ulf Hansson wrote: >>>> >>>> + Jon >>>> >>>> [...] >>>> >>>>> + >>>>> +static int ti_sci_pm_domains_probe(struct platform_device *pdev) >>>>> +{ >>>>> + struct device *dev = &pdev->dev; >>>>> + struct device_node *np = dev->of_node; >>>>> + struct ti_sci_genpd_data *ti_sci_genpd; >>>>> + >>>>> + ti_sci_genpd = devm_kzalloc(dev, sizeof(*ti_sci_genpd), >>>>> GFP_KERNEL); >>>>> + if (!ti_sci_genpd) >>>>> + return -ENOMEM; >>>>> + >>>>> + ti_sci_genpd->ti_sci = devm_ti_sci_get_handle(dev); >>>>> + if (IS_ERR(ti_sci_genpd->ti_sci)) >>>>> + return PTR_ERR(ti_sci_genpd->ti_sci); >>>>> + >>>>> + ti_sci_genpd->dev = dev; >>>>> + >>>>> + INIT_LIST_HEAD(&ti_sci_genpd->pd_list); >>>>> + mutex_init(&ti_sci_genpd->pd_list_mutex); >>>>> + >>>>> + return __of_genpd_add_provider(np, >>>>> of_ti_sci_genpd_xlate_onecell, >>>>> + ti_sci_genpd); >>>> >>>> >>>> Jon Hunter are working on adding robust method to be able to remove >>>> initialized genpds [1]. >>>> >>>> In that series we intend to remove the __of_genpd_add_provider() API, >>>> and instead only have of_genpd_add_provider_onecell() and >>>> of_genpd_add_provider_simple(). Could you please convert to use any of >>>> these APIs instead? >>> >>> >>> Thanks for pointing this out. I took at look at the series you've linked >>> and the >>> short answer is that I see no good way to directly convert what we've done >>> to >>> use those APIs. >>> >>> On this platform each device has it's power state controlled through the >>> SCI >>> protocol described in the cover letter. The system makes a request for >>> powering >>> on or off the device using a unique ID number for each device, as provided >>> in >>> patches 1 and 2. These operations map to those of a genpd, so we decided >>> to do a >>> 1:1 device to genpd mapping, where each device has it's own genpd. >>> >>> The split that we took from the provided simple and onecell xlate >>> functions >>> arises from this mapping. The IDs are not necessarily linear and also they >>> are >>> not necessarily defined in a fixed way for all SoCs, they are entirely >>> data >>> driven based on the provided device ID. To make use of these IDs, I >>> created a >>> new xlate function that takes a onecell value but instead dynamically >>> allocates >>> a new genpd, at probe time, to give us a genpd that contains the necessary >>> SoC >>> specific data for that device that probed and is mapped directly to the >>> device. >>> This lets us only create the genpds we need without having to redefine a >>> static >>> list of all possible genpds and duplicate the data. >>> >>> So my question back would be, how critical is it to be able to drop the >>> ability >>> to provide custom xlate functions? >> >> >> After thinking about this a bit more, I believe I see a way we can use the >> of_genpd_add_provider_onecell, although not optimally. The device IDs, and >> therefore the genpd IDs (in the last email I mentioned we are doing a 1:1 >> device to genpd mapping) are fixed and defined by the hardware, and are not >> linear, there can be gaps and we won't necessarily always start at 0 even >> though we do on this SoC. However, we could build an array of genpds that >> map to our IDs similar to how several have done it, like in >> drivers/soc/bcm/raspberrypi-power.c, but because our IDs can have gaps, >> there will be unused struct generic_pm_domains that get allocated and are >> never touched because the index of the element doesn't correspond to a genpd >> ID. > > There are a couple of ways to solve your problem without having to > create one genpd instance for each of the platform devices. As a > matter of fact, I think that approach should be avoided if possible. > Genpd isn't designed for these kind purposes, even if it can be done > like that. > Yes, ok. For K2G and future devices, the kernel will just request the device over the TI_SCI protocol and the firmware will guarantee everything that it needs it on. So we thought the 1:1 device to genpd mapping made sense in that regard, but maybe it is a stretch of the framework in the wrong way. > Here are some ideas which could be used to solve the problem differently. > > *) > Assuming each platform device has a driver for it!? > Then why don't you implement the ->runtime_suspend|resume() callbacks > on the driver level and call the SCI interface to power on/off the > device from there? This ought to be the most straight forward > solution. Straightforward yes but not a realistic option as we are using shared drivers from other platforms so sticking in platform specific code won't work. > > > **) > You may also use genpd's (struct generic_pm_domain) > ->attach|detach_dev() callbacks to create the device specific data > (the SCI device ID). The ->attach_dev() callback are invoked when a > device gets attached to its PM domain (genpd) at probe time. > > Currently these callbacks are already being used by SoCs that uses the > PM clk API from a genpd client. That is needed to create the device > specific PM clock list. We would have to do something similar here for > the SCI device IDs. > > Then, you must also assign/implement the ->start() and ->stop() > callbacks of the struct gpd_dev_ops, which is a part of the genpd > struct. These callbacks can then easily invoke SoC specific code/APIs > and perhaps that is the issue with my first suggested approach!? I've taken a look at what you have suggested here and I think it could work for us, thanks for the suggestion, I will give it a shot, I think that this will work just as well from a functional perspective. > >> >> Based on the ID set provided in patch 2 of this series I see 12 gaps, so >> we'd be wasting space the size of 12 genpds. The ID mapping will change on >> future SoCs, so this number could be larger. Do you think this is an >> acceptable solution? It allows us to play nice with the new genpd framework >> changes at the cost of wasting some space allocated to filler genpds. > > There are other issues as well, which mostly has do to with a > unnecessary long execution path, involving taking mutexes etc in > genpd. > > All you actually need, is to be able to power on/off a device from a > driver's ->runtime_suspend|resume() callback. Don't you think? Yes, but I thought the point of these frameworks was that they let us avoid doing it manually with platform specific code inside the drivers. I'll look at the callbacks in the genpd framework instead, that seems like a good place to do it. Regards, Dave > > Kind regards > Uffe > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dave Gerlach <d-gerlach@ti.com> writes: > On 08/30/2016 03:26 PM, Ulf Hansson wrote: >> On 30 August 2016 at 21:43, Dave Gerlach <d-gerlach@ti.com> wrote: >>> Jon, Ulf, >>> >>> On 08/26/2016 06:37 PM, Dave Gerlach wrote: >>>> >>>> Hi, >>>> On 08/25/2016 02:27 AM, Ulf Hansson wrote: >>>>> >>>>> + Jon >>>>> >>>>> [...] >>>>> >>>>>> + >>>>>> +static int ti_sci_pm_domains_probe(struct platform_device *pdev) >>>>>> +{ >>>>>> + struct device *dev = &pdev->dev; >>>>>> + struct device_node *np = dev->of_node; >>>>>> + struct ti_sci_genpd_data *ti_sci_genpd; >>>>>> + >>>>>> + ti_sci_genpd = devm_kzalloc(dev, sizeof(*ti_sci_genpd), >>>>>> GFP_KERNEL); >>>>>> + if (!ti_sci_genpd) >>>>>> + return -ENOMEM; >>>>>> + >>>>>> + ti_sci_genpd->ti_sci = devm_ti_sci_get_handle(dev); >>>>>> + if (IS_ERR(ti_sci_genpd->ti_sci)) >>>>>> + return PTR_ERR(ti_sci_genpd->ti_sci); >>>>>> + >>>>>> + ti_sci_genpd->dev = dev; >>>>>> + >>>>>> + INIT_LIST_HEAD(&ti_sci_genpd->pd_list); >>>>>> + mutex_init(&ti_sci_genpd->pd_list_mutex); >>>>>> + >>>>>> + return __of_genpd_add_provider(np, >>>>>> of_ti_sci_genpd_xlate_onecell, >>>>>> + ti_sci_genpd); >>>>> >>>>> >>>>> Jon Hunter are working on adding robust method to be able to remove >>>>> initialized genpds [1]. >>>>> >>>>> In that series we intend to remove the __of_genpd_add_provider() API, >>>>> and instead only have of_genpd_add_provider_onecell() and >>>>> of_genpd_add_provider_simple(). Could you please convert to use any of >>>>> these APIs instead? >>>> >>>> >>>> Thanks for pointing this out. I took at look at the series you've linked >>>> and the >>>> short answer is that I see no good way to directly convert what we've done >>>> to >>>> use those APIs. >>>> >>>> On this platform each device has it's power state controlled through the >>>> SCI >>>> protocol described in the cover letter. The system makes a request for >>>> powering >>>> on or off the device using a unique ID number for each device, as provided >>>> in >>>> patches 1 and 2. These operations map to those of a genpd, so we decided >>>> to do a >>>> 1:1 device to genpd mapping, where each device has it's own genpd. >>>> >>>> The split that we took from the provided simple and onecell xlate >>>> functions >>>> arises from this mapping. The IDs are not necessarily linear and also they >>>> are >>>> not necessarily defined in a fixed way for all SoCs, they are entirely >>>> data >>>> driven based on the provided device ID. To make use of these IDs, I >>>> created a >>>> new xlate function that takes a onecell value but instead dynamically >>>> allocates >>>> a new genpd, at probe time, to give us a genpd that contains the necessary >>>> SoC >>>> specific data for that device that probed and is mapped directly to the >>>> device. >>>> This lets us only create the genpds we need without having to redefine a >>>> static >>>> list of all possible genpds and duplicate the data. >>>> >>>> So my question back would be, how critical is it to be able to drop the >>>> ability >>>> to provide custom xlate functions? >>> >>> >>> After thinking about this a bit more, I believe I see a way we can use the >>> of_genpd_add_provider_onecell, although not optimally. The device IDs, and >>> therefore the genpd IDs (in the last email I mentioned we are doing a 1:1 >>> device to genpd mapping) are fixed and defined by the hardware, and are not >>> linear, there can be gaps and we won't necessarily always start at 0 even >>> though we do on this SoC. However, we could build an array of genpds that >>> map to our IDs similar to how several have done it, like in >>> drivers/soc/bcm/raspberrypi-power.c, but because our IDs can have gaps, >>> there will be unused struct generic_pm_domains that get allocated and are >>> never touched because the index of the element doesn't correspond to a genpd >>> ID. >> >> There are a couple of ways to solve your problem without having to >> create one genpd instance for each of the platform devices. As a >> matter of fact, I think that approach should be avoided if possible. >> Genpd isn't designed for these kind purposes, even if it can be done >> like that. >> > > Yes, ok. For K2G and future devices, the kernel will just request the > device over the TI_SCI protocol and the firmware will guarantee > everything that it needs it on. So we thought the 1:1 device to genpd > mapping made sense in that regard, but maybe it is a stretch of the > framework in the wrong way. > >> Here are some ideas which could be used to solve the problem differently. >> >> *) >> Assuming each platform device has a driver for it!? >> Then why don't you implement the ->runtime_suspend|resume() callbacks >> on the driver level and call the SCI interface to power on/off the >> device from there? This ought to be the most straight forward >> solution. > > Straightforward yes but not a realistic option as we are using shared > drivers from other platforms so sticking in platform specific code > won't work. > >> >> >> **) >> You may also use genpd's (struct generic_pm_domain) >> ->attach|detach_dev() callbacks to create the device specific data >> (the SCI device ID). The ->attach_dev() callback are invoked when a >> device gets attached to its PM domain (genpd) at probe time. >> >> Currently these callbacks are already being used by SoCs that uses the >> PM clk API from a genpd client. That is needed to create the device >> specific PM clock list. We would have to do something similar here for >> the SCI device IDs. >> >> Then, you must also assign/implement the ->start() and ->stop() >> callbacks of the struct gpd_dev_ops, which is a part of the genpd >> struct. These callbacks can then easily invoke SoC specific code/APIs >> and perhaps that is the issue with my first suggested approach!? > > I've taken a look at what you have suggested here and I think it could > work for us, thanks for the suggestion, I will give it a shot, I think > that this will work just as well from a functional perspective. > >> >>> >>> Based on the ID set provided in patch 2 of this series I see 12 gaps, so >>> we'd be wasting space the size of 12 genpds. The ID mapping will change on >>> future SoCs, so this number could be larger. Do you think this is an >>> acceptable solution? It allows us to play nice with the new genpd framework >>> changes at the cost of wasting some space allocated to filler genpds. >> >> There are other issues as well, which mostly has do to with a >> unnecessary long execution path, involving taking mutexes etc in >> genpd. >> >> All you actually need, is to be able to power on/off a device from a >> driver's ->runtime_suspend|resume() callback. Don't you think? > > Yes, but I thought the point of these frameworks was that they let us > avoid doing it manually with platform specific code inside the > drivers. I'll look at the callbacks in the genpd framework instead, > that seems like a good place to do it. One more idea... Since you don't really have a domain (a group of devices), what you really have is each device having an independent power switch, so as Ulf suggested, what you really need is for all the devices to share the same set of runtime PM callbacks that call SCI. The only difference is the unique ID. Rather than using all of genpd, you could also just use a pm_domain which is what genpd is built on top of (and also omap_device, which you're probably familiar with also.) That would allow you to keep the drivers completely generic, yet share all the SCI specific "domain" code inside a pm_domain. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> >> Here are some ideas which could be used to solve the problem differently. >> >> *) >> Assuming each platform device has a driver for it!? >> Then why don't you implement the ->runtime_suspend|resume() callbacks >> on the driver level and call the SCI interface to power on/off the >> device from there? This ought to be the most straight forward >> solution. > > > Straightforward yes but not a realistic option as we are using shared > drivers from other platforms so sticking in platform specific code won't > work. Agree, but... Historically, I think this was *one* of the reasons to why omap's hw_mod PM domain was invented. At that point we did not have the common clk framework, the pinctrl framework, the phy framework, syscon, etc. These device resources can now be handled by the drivers themselves via generic interfaces and thus they remains to be portable. My point is, let's be sure you don't drop this approach unless it's really SoC specific code needed to deal with the device. > >> >> >> **) >> You may also use genpd's (struct generic_pm_domain) >> ->attach|detach_dev() callbacks to create the device specific data >> (the SCI device ID). The ->attach_dev() callback are invoked when a >> device gets attached to its PM domain (genpd) at probe time. >> >> Currently these callbacks are already being used by SoCs that uses the >> PM clk API from a genpd client. That is needed to create the device >> specific PM clock list. We would have to do something similar here for >> the SCI device IDs. >> >> Then, you must also assign/implement the ->start() and ->stop() >> callbacks of the struct gpd_dev_ops, which is a part of the genpd >> struct. These callbacks can then easily invoke SoC specific code/APIs >> and perhaps that is the issue with my first suggested approach!? > > > I've taken a look at what you have suggested here and I think it could work > for us, thanks for the suggestion, I will give it a shot, I think that this > will work just as well from a functional perspective. Okay, great! > >> >>> >>> Based on the ID set provided in patch 2 of this series I see 12 gaps, so >>> we'd be wasting space the size of 12 genpds. The ID mapping will change >>> on >>> future SoCs, so this number could be larger. Do you think this is an >>> acceptable solution? It allows us to play nice with the new genpd >>> framework >>> changes at the cost of wasting some space allocated to filler genpds. >> >> >> There are other issues as well, which mostly has do to with a >> unnecessary long execution path, involving taking mutexes etc in >> genpd. >> >> All you actually need, is to be able to power on/off a device from a >> driver's ->runtime_suspend|resume() callback. Don't you think? > > > Yes, but I thought the point of these frameworks was that they let us avoid > doing it manually with platform specific code inside the drivers. I'll look > at the callbacks in the genpd framework instead, that seems like a good > place to do it. BTW, as you would need to store your device specific data somewhere, we probably need to extend the struct pm_subsys_data or the "struct pm_domain_data", to hold a "void *data". Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[...] > > One more idea... > > Since you don't really have a domain (a group of devices), what you > really have is each device having an independent power switch, so as Ulf > suggested, what you really need is for all the devices to share the same > set of runtime PM callbacks that call SCI. The only difference is the > unique ID. > > Rather than using all of genpd, you could also just use a pm_domain > which is what genpd is built on top of (and also omap_device, which > you're probably familiar with also.) Even if this would work as well, the downside would be that you need to re-invent the parts related to the DT parsing, the probing/removal and attaching/detaching of the device to the PM domain. You probably don't want to go there... :-) > > That would allow you to keep the drivers completely generic, yet share > all the SCI specific "domain" code inside a pm_domain. > > Kevin Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Ulf Hansson <ulf.hansson@linaro.org> writes: > [...] > >> >> One more idea... >> >> Since you don't really have a domain (a group of devices), what you >> really have is each device having an independent power switch, so as Ulf >> suggested, what you really need is for all the devices to share the same >> set of runtime PM callbacks that call SCI. The only difference is the >> unique ID. >> >> Rather than using all of genpd, you could also just use a pm_domain >> which is what genpd is built on top of (and also omap_device, which >> you're probably familiar with also.) > > Even if this would work as well, the downside would be that you need > to re-invent the parts related to the DT parsing, the probing/removal > and attaching/detaching of the device to the PM domain. > > You probably don't want to go there... :-) All you'd need to read from DT would be the device-specific ID for TI-SCI, and that could be done at bind time with a notifier. The, in that same notifier, if a TI-SCI ID exists, it would get added to the pm_domain. Anyways, your original proposal is much preferred if it can work. I'm just throwing out another option because I really don't like one genpd per device. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 09/08/2016 12:38 PM, Kevin Hilman wrote: > Ulf Hansson <ulf.hansson@linaro.org> writes: > >> [...] >> >>> >>> One more idea... >>> >>> Since you don't really have a domain (a group of devices), what you >>> really have is each device having an independent power switch, so as Ulf >>> suggested, what you really need is for all the devices to share the same >>> set of runtime PM callbacks that call SCI. The only difference is the >>> unique ID. >>> >>> Rather than using all of genpd, you could also just use a pm_domain >>> which is what genpd is built on top of (and also omap_device, which >>> you're probably familiar with also.) >> >> Even if this would work as well, the downside would be that you need >> to re-invent the parts related to the DT parsing, the probing/removal >> and attaching/detaching of the device to the PM domain. >> >> You probably don't want to go there... :-) > > All you'd need to read from DT would be the device-specific ID for > TI-SCI, and that could be done at bind time with a notifier. The, in > that same notifier, if a TI-SCI ID exists, it would get added to the > pm_domain. > > Anyways, your original proposal is much preferred if it can work. I'm > just throwing out another option because I really don't like one genpd > per device. > > Kevin > I am first trying to leverage the dev_attach/detach and start/stop callbacks that Ulf suggested without creating a single genpd per device and it looks like it will work for us. I appreciate the alternative suggestions but I agree we'd like to leverage as much of the existing genpd framework as we can and avoid going down the omap_device style implementation path. Regards, Dave -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 8 September 2016 at 19:38, Kevin Hilman <khilman@baylibre.com> wrote: > Ulf Hansson <ulf.hansson@linaro.org> writes: > >> [...] >> >>> >>> One more idea... >>> >>> Since you don't really have a domain (a group of devices), what you >>> really have is each device having an independent power switch, so as Ulf >>> suggested, what you really need is for all the devices to share the same >>> set of runtime PM callbacks that call SCI. The only difference is the >>> unique ID. >>> >>> Rather than using all of genpd, you could also just use a pm_domain >>> which is what genpd is built on top of (and also omap_device, which >>> you're probably familiar with also.) >> >> Even if this would work as well, the downside would be that you need >> to re-invent the parts related to the DT parsing, the probing/removal >> and attaching/detaching of the device to the PM domain. >> >> You probably don't want to go there... :-) > > All you'd need to read from DT would be the device-specific ID for > TI-SCI, and that could be done at bind time with a notifier. The, in > that same notifier, if a TI-SCI ID exists, it would get added to the > pm_domain. > > Anyways, your original proposal is much preferred if it can work. I'm > just throwing out another option because I really don't like one genpd > per device. Okay, then we are in full agreement! BTW, I have just been trying to convince other people working on Rockchip SoCs, to also avoid using one genpd per device. Feel free to join those discussions [1] as well. :-) Kind regards Uffe [1] https://lkml.org/lkml/2016/9/1/377 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" 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/MAINTAINERS b/MAINTAINERS index ea14b08c30bb..448f6801bd78 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11575,6 +11575,7 @@ F: drivers/firmware/ti_sci* F: include/linux/soc/ti/ti_sci_protocol.h F: Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt F: include/dt-bindings/genpd/k2g.h +F: drivers/soc/ti/ti_sci_pm_domains.c THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER M: Hans Verkuil <hverkuil@xs4all.nl> diff --git a/arch/arm/mach-keystone/Kconfig b/arch/arm/mach-keystone/Kconfig index 8ff61be1a29f..34ef4b60adc6 100644 --- a/arch/arm/mach-keystone/Kconfig +++ b/arch/arm/mach-keystone/Kconfig @@ -11,6 +11,7 @@ config ARCH_KEYSTONE select MIGHT_HAVE_PCI select PCI_DOMAINS if PCI select PINCTRL + select PM_GENERIC_DOMAINS if PM help Support for boards based on the Texas Instruments Keystone family of SoCs. diff --git a/drivers/soc/ti/Kconfig b/drivers/soc/ti/Kconfig index 3557c5e32a93..39e152abe6b9 100644 --- a/drivers/soc/ti/Kconfig +++ b/drivers/soc/ti/Kconfig @@ -38,4 +38,16 @@ config WKUP_M3_IPC to communicate and use the Wakeup M3 for PM features like suspend resume and boots it using wkup_m3_rproc driver. +config TI_SCI_PM_DOMAINS + tristate "TI SCI PM Domains Driver" + depends on TI_SCI_PROTOCOL + depends on PM_GENERIC_DOMAINS + help + Generic power domain implementation for TI device implementing + the TI SCI protocol. + + To compile this as a module, choose M here. The module will be + called ti_sci_pm_domains. Note this is needed early in boot before + rootfs may be available. + endif # SOC_TI diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile index 48ff3a79634f..7d572736c86e 100644 --- a/drivers/soc/ti/Makefile +++ b/drivers/soc/ti/Makefile @@ -5,3 +5,4 @@ obj-$(CONFIG_KEYSTONE_NAVIGATOR_QMSS) += knav_qmss.o knav_qmss-y := knav_qmss_queue.o knav_qmss_acc.o obj-$(CONFIG_KEYSTONE_NAVIGATOR_DMA) += knav_dma.o obj-$(CONFIG_WKUP_M3_IPC) += wkup_m3_ipc.o +obj-$(CONFIG_TI_SCI_PM_DOMAINS) += ti_sci_pm_domains.o diff --git a/drivers/soc/ti/ti_sci_pm_domains.c b/drivers/soc/ti/ti_sci_pm_domains.c new file mode 100644 index 000000000000..ab6b20201731 --- /dev/null +++ b/drivers/soc/ti/ti_sci_pm_domains.c @@ -0,0 +1,222 @@ +/* + * TI SCI Generic Power Domain Driver + * + * Copyright (C) 2015-2016 Texas Instruments Incorporated - http://www.ti.com/ + * J Keerthy <j-keerthy@ti.com> + * Dave Gerlach <d-gerlach@ti.com> + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/err.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pm_domain.h> +#include <linux/slab.h> +#include <linux/soc/ti/ti_sci_protocol.h> + +#define TI_GENPD_NAME_LENGTH 16 + +/** + * struct ti_sci_genpd_data: holds data needed for every power domain + * @ti_sci: handle to TI SCI protocol driver that provides ops to + * communicate with system control processor. + * @dev: pointer to dev for the driver for devm allocs + * @pd_list_mutex: Mutex for protecting the global list of power domains + * @pd_list: list that hols all power domains as they are allocated + */ +struct ti_sci_genpd_data { + const struct ti_sci_handle *ti_sci; + struct device *dev; + struct mutex pd_list_mutex; /* Protect master list of domains */ + struct list_head pd_list; +}; + +/** + * struct ti_sci_pm_domain: TI specific data needed for each power domain + * @idx: index of the power domain that identifies it with the system + * control processor. + * @pd: generic_pm_domain for use with the genpd framework + * @node: list_head for tracking all domains in a list + * @parent: pointer to the global data defined for all domains + */ +struct ti_sci_pm_domain { + int idx; + struct generic_pm_domain pd; + struct list_head node; + struct ti_sci_genpd_data *parent; +}; + +#define genpd_to_ti_sci_pd(gpd) container_of(gpd, struct ti_sci_pm_domain, pd) + +/** + * pd_power_off(): generic_pm_domain power_off hook + * @domain: generic_pm_domain struct provided by genpd framework of the + * pd to be shut off + * + * This hook uses the put_device dev_op provided by ti_sci to power off the + * device associated to the power domain provided. + */ +static int pd_power_off(struct generic_pm_domain *domain) +{ + struct ti_sci_pm_domain *ti_sci_pd = genpd_to_ti_sci_pd(domain); + const struct ti_sci_handle *ti_sci = ti_sci_pd->parent->ti_sci; + + return ti_sci->ops.dev_ops.put_device(ti_sci, + ti_sci_pd->idx); +} + +/** + * pd_power_on(): generic_pm_domain power_on hook + * @domain: generic_pm_domain struct provided by genpd framework of the + * pd to be powered on + * + * This hook uses the get_device dev_op provided by ti_sci to power on the + * device associated to the power domain provided. + */ +static int pd_power_on(struct generic_pm_domain *domain) +{ + struct ti_sci_pm_domain *ti_sci_pd = genpd_to_ti_sci_pd(domain); + const struct ti_sci_handle *ti_sci = ti_sci_pd->parent->ti_sci; + + return ti_sci->ops.dev_ops.get_device(ti_sci, + ti_sci_pd->idx); +} + +/** + * of_ti_sci_genpd_xlate_onecell() - Xlate function using a single index. + * @genpdspec: OF phandle args to define an index to be communicated over + * TI SCI to the system control processor to identify it + * @data: xlate function private data - pointer to the ti_sci_genpd_data + * struct containing global sci pm domain data + * + * This is xlate function takes a single cell as an index representing the + * id to be passed to the system control processor. As each device in the + * device tree probes a single pm domain will be created specifically for it + * based on the index passed in the pweor-domains property. If no pm domain + * yet exists for that index it is created, otherwise it is looked up and + * returned. Through this 1 to 1 association between power domains and + * devices the genpd framework will work to directly control devices + * through the pm_runtime framework. + */ +static struct generic_pm_domain *of_ti_sci_genpd_xlate_onecell( + struct of_phandle_args *genpdspec, + void *data) +{ + struct ti_sci_genpd_data *ti_sci_genpd = data; + const struct ti_sci_handle *ti_sci = ti_sci_genpd->ti_sci; + struct device *dev = ti_sci_genpd->dev; + unsigned int idx = genpdspec->args[0]; + struct ti_sci_pm_domain *ti_sci_pd = NULL, *pd; + char *name; + int ret = 0; + + if (genpdspec->args_count != 1) + return ERR_PTR(-EINVAL); + + /* + * Check the validity of the requested idx, if the index is not valid + * the PMMC will return a NAK here and we will not allocate it. + */ + ret = ti_sci->ops.dev_ops.is_valid(ti_sci, idx); + if (ret) + return ERR_PTR(-EINVAL); + + mutex_lock(&ti_sci_genpd->pd_list_mutex); + list_for_each_entry(pd, &ti_sci_genpd->pd_list, node) { + if (pd->idx == idx) { + ti_sci_pd = pd; + goto unlock_and_return; + } + } + + ti_sci_pd = devm_kzalloc(dev, + sizeof(*ti_sci_pd), + GFP_KERNEL); + if (!ti_sci_pd) { + ret = -ENOMEM; + goto unlock_and_return; + } + + ti_sci_pd->idx = idx; + ti_sci_pd->pd.power_off = pd_power_off; + ti_sci_pd->pd.power_on = pd_power_on; + + name = devm_kzalloc(dev, TI_GENPD_NAME_LENGTH, GFP_KERNEL); + if (!name) { + devm_kfree(dev, ti_sci_pd); + ret = -ENOMEM; + goto unlock_and_return; + } + + snprintf(name, TI_GENPD_NAME_LENGTH, "pd-%d", idx); + ti_sci_pd->pd.name = name; + + ti_sci_pd->parent = ti_sci_genpd; + /* + * Init each pd as is_off so we always call pd_power_on + * to make sure reference counting is properly maintained + * on the SCI side + */ + pm_genpd_init(&ti_sci_pd->pd, NULL, true); + + list_add(&ti_sci_pd->node, &ti_sci_genpd->pd_list); + +unlock_and_return: + mutex_unlock(&ti_sci_genpd->pd_list_mutex); + + if (ret) + return ERR_PTR(ret); + else + return &ti_sci_pd->pd; +} + +static const struct of_device_id ti_sci_pm_domain_matches[] = { + { .compatible = "ti,sci-pm-domains", }, + { }, +}; +MODULE_DEVICE_TABLE(of, ti_sci_pm_domain_matches); + +static int ti_sci_pm_domains_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct device_node *np = dev->of_node; + struct ti_sci_genpd_data *ti_sci_genpd; + + ti_sci_genpd = devm_kzalloc(dev, sizeof(*ti_sci_genpd), GFP_KERNEL); + if (!ti_sci_genpd) + return -ENOMEM; + + ti_sci_genpd->ti_sci = devm_ti_sci_get_handle(dev); + if (IS_ERR(ti_sci_genpd->ti_sci)) + return PTR_ERR(ti_sci_genpd->ti_sci); + + ti_sci_genpd->dev = dev; + + INIT_LIST_HEAD(&ti_sci_genpd->pd_list); + mutex_init(&ti_sci_genpd->pd_list_mutex); + + return __of_genpd_add_provider(np, of_ti_sci_genpd_xlate_onecell, + ti_sci_genpd); +} + +static struct platform_driver ti_sci_pm_domains_driver = { + .probe = ti_sci_pm_domains_probe, + .driver = { + .name = "ti_sci_pm_domains", + .of_match_table = ti_sci_pm_domain_matches, + }, +}; +module_platform_driver(ti_sci_pm_domains_driver); +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("TI System Control Interface(SCI) Power Domain driver"); +MODULE_AUTHOR("Dave Gerlach");