| Message ID | 1453998832-27383-12-git-send-email-jonathanh@nvidia.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 28 January 2016 at 17:33, Jon Hunter <jonathanh@nvidia.com> wrote: > Adds generic PM support to the PMC driver where the PM domains are > populated from device-tree and the PM domain consumer devices are > bound to their relevant PM domains via device-tree as well. > > Update the tegra_powergate_sequence_power_up() API so that internally > it calls the same tegra_powergate_xxx functions that are used by the > tegra generic power domain code for consistency. > > This is based upon work by Thierry Reding <treding@nvidia.com> > and Vince Hsu <vinceh@nvidia.com>. > > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > --- > drivers/soc/tegra/pmc.c | 470 ++++++++++++++++++++++++++-- > include/dt-bindings/power/tegra-powergate.h | 36 +++ > include/soc/tegra/pmc.h | 39 +-- I suggest you split the header changes into a separate patch. Moreover, these new DT definitions should be documented in the patch describing the new powergate DT bindings. At least a simple list providing the available options. [...] > > +static void tegra_powergate_disable_clocks(struct tegra_powergate *pg) > +{ > + unsigned int i; > + > + for (i = 0; i < pg->num_clks; i++) > + clk_disable_unprepare(pg->clks[i]); > +} > + > +static int tegra_powergate_enable_clocks(struct tegra_powergate *pg) > +{ > + unsigned int i; > + int err; > + > + for (i = 0; i < pg->num_clks; i++) { > + err = clk_prepare_enable(pg->clks[i]); > + if (err) > + goto out; > + } > + > + return 0; > + > +out: > + while (i--) > + clk_disable_unprepare(pg->clks[i]); > + > + return err; > +} I have seen similar code around in other PM domains, dealing with enabling/disabling a *list* of clocks. Perhaps we should invent a new clock API that helps with this to prevents code duplication!? [...] > /** > * tegra_powergate_power_on() - power on partition > * @id: partition ID > @@ -319,35 +512,20 @@ EXPORT_SYMBOL(tegra_powergate_remove_clamping); > int tegra_powergate_sequence_power_up(unsigned int id, struct clk *clk, > struct reset_control *rst) There seems to be two viable ways for a driver to control tegra powergates. 1) $Subject patch enables the use of runtime PM. 2) The current tegra_powergate_sequence_power_up() and tegra_powergate_power_off() API. It seems fragile to allow both options, but perhaps your are protecting this with some lock to prevent concurrent accesses? Also, I assume you need the two options in a transition phase, before you have deployed runtime PM for these drivers? [...] > +static int tegra_powergate_of_get_clks(struct device *dev, > + struct tegra_powergate *pg) > +{ > + struct clk *clk; > + unsigned int i; > + int err; > + > + pg->num_clks = of_count_phandle_with_args(pg->of_node, "clocks", > + "#clock-cells"); > + if (pg->num_clks == 0) > + return -ENODEV; > + > + pg->clks = devm_kcalloc(dev, pg->num_clks, sizeof(clk), GFP_KERNEL); > + if (!pg->clks) > + return -ENOMEM; > + > + for (i = 0; i < pg->num_clks; i++) { > + pg->clks[i] = of_clk_get(pg->of_node, i); > + if (IS_ERR(pg->clks[i])) { > + err = PTR_ERR(pg->clks[i]); > + goto err; > + } > + } > + > + return 0; > + > +err: > + while (i--) > + clk_put(pg->clks[i]); > + > + pg->num_clks = 0; > + > + return err; > +} Fetching clocks like above function does, seems to be a quite common case. As I suggested to add an enable/disable API for a clock list, the similar can be done for creating the clock list. Just an idea... [...] > + > +static void tegra_powergate_remove(struct tegra_pmc *pmc) > +{ > + struct tegra_powergate *pg, *n; > + > + list_for_each_entry_safe(pg, n, &pmc->powergates_list, node) { The tegra powergate driver will hold a list of nvidia powergates domains, and the generic PM domain will hold a list of all generic PM domains. Perhaps there's a way to allow the generic PM domain to control this by itself. If we for example used the struct device corresponding to the powergate driver, genpd could use it to distinguish between various instances of genpd structs..!? Maybe it would simplify the way to deal with removing domains? > + of_genpd_del_provider(pg->of_node); > + > + if (pg->parent) { > + if (WARN_ON(pm_genpd_remove_subdomain(pg->parent, > + &pg->genpd))) > + return; > + > + pg->parent = NULL; > + } > + > + if (WARN_ON(pm_genpd_remove(&pg->genpd))) > + return; > + > + while (pg->num_clks--) > + clk_put(pg->clks[pg->num_clks]); > + > + while (pg->num_resets--) > + reset_control_put(pg->resets[pg->num_resets]); > + > + list_del(&pg->node); > + } > +} > + [...] 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 04/02/16 15:44, Ulf Hansson wrote: > On 28 January 2016 at 17:33, Jon Hunter <jonathanh@nvidia.com> wrote: >> Adds generic PM support to the PMC driver where the PM domains are >> populated from device-tree and the PM domain consumer devices are >> bound to their relevant PM domains via device-tree as well. >> >> Update the tegra_powergate_sequence_power_up() API so that internally >> it calls the same tegra_powergate_xxx functions that are used by the >> tegra generic power domain code for consistency. >> >> This is based upon work by Thierry Reding <treding@nvidia.com> >> and Vince Hsu <vinceh@nvidia.com>. >> >> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >> --- >> drivers/soc/tegra/pmc.c | 470 ++++++++++++++++++++++++++-- >> include/dt-bindings/power/tegra-powergate.h | 36 +++ >> include/soc/tegra/pmc.h | 39 +-- > > I suggest you split the header changes into a separate patch. > > Moreover, these new DT definitions should be documented in the patch > describing the new powergate DT bindings. At least a simple list > providing the available options. Ok. > [...] > >> >> +static void tegra_powergate_disable_clocks(struct tegra_powergate *pg) >> +{ >> + unsigned int i; >> + >> + for (i = 0; i < pg->num_clks; i++) >> + clk_disable_unprepare(pg->clks[i]); >> +} >> + >> +static int tegra_powergate_enable_clocks(struct tegra_powergate *pg) >> +{ >> + unsigned int i; >> + int err; >> + >> + for (i = 0; i < pg->num_clks; i++) { >> + err = clk_prepare_enable(pg->clks[i]); >> + if (err) >> + goto out; >> + } >> + >> + return 0; >> + >> +out: >> + while (i--) >> + clk_disable_unprepare(pg->clks[i]); >> + >> + return err; >> +} > > I have seen similar code around in other PM domains, dealing with > enabling/disabling a *list* of clocks. > Perhaps we should invent a new clock API that helps with this to > prevents code duplication!? Yes, I have been thinking about that as well. I will have a look at that. > [...] > >> /** >> * tegra_powergate_power_on() - power on partition >> * @id: partition ID >> @@ -319,35 +512,20 @@ EXPORT_SYMBOL(tegra_powergate_remove_clamping); >> int tegra_powergate_sequence_power_up(unsigned int id, struct clk *clk, >> struct reset_control *rst) > > There seems to be two viable ways for a driver to control tegra powergates. > > 1) > $Subject patch enables the use of runtime PM. > > 2) > The current tegra_powergate_sequence_power_up() and > tegra_powergate_power_off() API. > > It seems fragile to allow both options, but perhaps your are > protecting this with some lock to prevent concurrent accesses? There is a lock protecting accesses to the PMC registers which ultimately control the power domain. However, may be it would be better to ensure that any power-domain registered with genpd cannot be controlled by the legacy APIs. I have added a bitmap to mark valid power-domains to ensure that only valid power domains can be controlled by these legacy APIs. I could mark the power-domain invalid after registering with genpd to ensure that it cannot be accessed by the legacy APIs. > Also, I assume you need the two options in a transition phase, before > you have deployed runtime PM for these drivers? Right and some of the legacy APIs are entrenched in some drivers. So to keep the patch set manageable it seems best to get some support in place then start migrating the drivers. > [...] > >> +static int tegra_powergate_of_get_clks(struct device *dev, >> + struct tegra_powergate *pg) >> +{ >> + struct clk *clk; >> + unsigned int i; >> + int err; >> + >> + pg->num_clks = of_count_phandle_with_args(pg->of_node, "clocks", >> + "#clock-cells"); >> + if (pg->num_clks == 0) >> + return -ENODEV; >> + >> + pg->clks = devm_kcalloc(dev, pg->num_clks, sizeof(clk), GFP_KERNEL); >> + if (!pg->clks) >> + return -ENOMEM; >> + >> + for (i = 0; i < pg->num_clks; i++) { >> + pg->clks[i] = of_clk_get(pg->of_node, i); >> + if (IS_ERR(pg->clks[i])) { >> + err = PTR_ERR(pg->clks[i]); >> + goto err; >> + } >> + } >> + >> + return 0; >> + >> +err: >> + while (i--) >> + clk_put(pg->clks[i]); >> + >> + pg->num_clks = 0; >> + >> + return err; >> +} > > Fetching clocks like above function does, seems to be a quite common case. > > As I suggested to add an enable/disable API for a clock list, the > similar can be done for creating the clock list. > > Just an idea... Ok. > [...] > >> + >> +static void tegra_powergate_remove(struct tegra_pmc *pmc) >> +{ >> + struct tegra_powergate *pg, *n; >> + >> + list_for_each_entry_safe(pg, n, &pmc->powergates_list, node) { > > The tegra powergate driver will hold a list of nvidia powergates > domains, and the generic PM domain will hold a list of all generic PM > domains. > > Perhaps there's a way to allow the generic PM domain to control this > by itself. If we for example used the struct device corresponding to > the powergate driver, genpd could use it to distinguish between > various instances of genpd structs..!? Maybe it would simplify the way > to deal with removing domains? Yes, that would be ideal. However, would have require changing genpd_init()? I am not sure how genpd would be able to access the device struct for the powergate driver because we don't provide this via any API I am aware of? And I am guessing that you don't wish to expose the gpd_list to the world either. If there is an easy way, I am open to it, but looking at it today, I am not sure I see a simple way in which we could add a new API to do this. However, may be I am missing something! Cheers Jon -- 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
[...] >> >>> /** >>> * tegra_powergate_power_on() - power on partition >>> * @id: partition ID >>> @@ -319,35 +512,20 @@ EXPORT_SYMBOL(tegra_powergate_remove_clamping); >>> int tegra_powergate_sequence_power_up(unsigned int id, struct clk *clk, >>> struct reset_control *rst) >> >> There seems to be two viable ways for a driver to control tegra powergates. >> >> 1) >> $Subject patch enables the use of runtime PM. >> >> 2) >> The current tegra_powergate_sequence_power_up() and >> tegra_powergate_power_off() API. >> >> It seems fragile to allow both options, but perhaps your are >> protecting this with some lock to prevent concurrent accesses? > > There is a lock protecting accesses to the PMC registers which > ultimately control the power domain. However, may be it would be better > to ensure that any power-domain registered with genpd cannot be > controlled by the legacy APIs. I have added a bitmap to mark valid > power-domains to ensure that only valid power domains can be controlled > by these legacy APIs. I could mark the power-domain invalid after > registering with genpd to ensure that it cannot be accessed by the > legacy APIs. That seems like a good way of making it more robust! > >> Also, I assume you need the two options in a transition phase, before >> you have deployed runtime PM for these drivers? > > Right and some of the legacy APIs are entrenched in some drivers. So to > keep the patch set manageable it seems best to get some support in place > then start migrating the drivers. Thanks for elaborating on this! I get and like the idea of moving forward! [...] >>> + >>> +static void tegra_powergate_remove(struct tegra_pmc *pmc) >>> +{ >>> + struct tegra_powergate *pg, *n; >>> + >>> + list_for_each_entry_safe(pg, n, &pmc->powergates_list, node) { >> >> The tegra powergate driver will hold a list of nvidia powergates >> domains, and the generic PM domain will hold a list of all generic PM >> domains. >> >> Perhaps there's a way to allow the generic PM domain to control this >> by itself. If we for example used the struct device corresponding to >> the powergate driver, genpd could use it to distinguish between >> various instances of genpd structs..!? Maybe it would simplify the way >> to deal with removing domains? > > Yes, that would be ideal. However, would have require changing > genpd_init()? I am not sure how genpd would be able to access the device > struct for the powergate driver because we don't provide this via any > API I am aware of? And I am guessing that you don't wish to expose the > gpd_list to the world either. > > If there is an easy way, I am open to it, but looking at it today, I am > not sure I see a simple way in which we could add a new API to do this. > However, may be I am missing something! If we add a new __pm_genpd_init() API, that could require a struct device to be provided. That API will thus invoke the existing pm_genpd_init() but also deal with the extra things needed here. I would also allow such an API to return an error code. Correspondingly, pm_genpd_remove() could be required to be provided with a struct device. Existing users of pm_genpd_init() can then convert to __pm_genpd_init() whenever suitable. Of course, another option is just to add new member in the genpd struct for the struct *device. The caller of pm_genpd_init() could check it, but allow it to be NULL. Although, the pm_genpd_remove() API would require that pointer to the struct device to be set... What do 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 10/02/16 18:25, Ulf Hansson wrote: [snip] >>> Perhaps there's a way to allow the generic PM domain to control this >>> by itself. If we for example used the struct device corresponding to >>> the powergate driver, genpd could use it to distinguish between >>> various instances of genpd structs..!? Maybe it would simplify the way >>> to deal with removing domains? >> >> Yes, that would be ideal. However, would have require changing >> genpd_init()? I am not sure how genpd would be able to access the device >> struct for the powergate driver because we don't provide this via any >> API I am aware of? And I am guessing that you don't wish to expose the >> gpd_list to the world either. >> >> If there is an easy way, I am open to it, but looking at it today, I am >> not sure I see a simple way in which we could add a new API to do this. >> However, may be I am missing something! > > If we add a new __pm_genpd_init() API, that could require a struct > device to be provided. That API will thus invoke the existing > pm_genpd_init() but also deal with the extra things needed here. > > I would also allow such an API to return an error code. > > Correspondingly, pm_genpd_remove() could be required to be provided > with a struct device. > > Existing users of pm_genpd_init() can then convert to > __pm_genpd_init() whenever suitable. > > Of course, another option is just to add new member in the genpd > struct for the struct *device. The caller of pm_genpd_init() could > check it, but allow it to be NULL. Although, the pm_genpd_remove() API > would require that pointer to the struct device to be set... > > What do you think? Yes, sounds good. May be it is simpler just to add a new member and let the platform genpd driver handle it. I am wondering if in addition to pm_genpd_remove(), we then just have a function called pm_genpd_remove_tail(), which allows you to pass the struct device pointer and will remove the last pm-domain from the gpd_list and return the genpd pointer if successful. Internally, it will call pm_genpd_remove(). It seems to me that if there are nested pm-domains, then we probably want to remove them starting from the tail as opposed to the head. How does that sound? Cheers Jon -- 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 11 February 2016 at 10:13, Jon Hunter <jonathanh@nvidia.com> wrote: > > On 10/02/16 18:25, Ulf Hansson wrote: > > [snip] > >>>> Perhaps there's a way to allow the generic PM domain to control this >>>> by itself. If we for example used the struct device corresponding to >>>> the powergate driver, genpd could use it to distinguish between >>>> various instances of genpd structs..!? Maybe it would simplify the way >>>> to deal with removing domains? >>> >>> Yes, that would be ideal. However, would have require changing >>> genpd_init()? I am not sure how genpd would be able to access the device >>> struct for the powergate driver because we don't provide this via any >>> API I am aware of? And I am guessing that you don't wish to expose the >>> gpd_list to the world either. >>> >>> If there is an easy way, I am open to it, but looking at it today, I am >>> not sure I see a simple way in which we could add a new API to do this. >>> However, may be I am missing something! >> >> If we add a new __pm_genpd_init() API, that could require a struct >> device to be provided. That API will thus invoke the existing >> pm_genpd_init() but also deal with the extra things needed here. >> >> I would also allow such an API to return an error code. >> >> Correspondingly, pm_genpd_remove() could be required to be provided >> with a struct device. >> >> Existing users of pm_genpd_init() can then convert to >> __pm_genpd_init() whenever suitable. >> >> Of course, another option is just to add new member in the genpd >> struct for the struct *device. The caller of pm_genpd_init() could >> check it, but allow it to be NULL. Although, the pm_genpd_remove() API >> would require that pointer to the struct device to be set... >> >> What do you think? > > Yes, sounds good. May be it is simpler just to add a new member and let > the platform genpd driver handle it. > > I am wondering if in addition to pm_genpd_remove(), we then just have a > function called pm_genpd_remove_tail(), which allows you to pass the > struct device pointer and will remove the last pm-domain from the > gpd_list and return the genpd pointer if successful. Internally, it will > call pm_genpd_remove(). It seems to me that if there are nested > pm-domains, then we probably want to remove them starting from the tail > as opposed to the head. > > How does that sound? Why not make pm_genpd_remove() to behave as you describe for pm_genpd_remove_tail()? That's probably the only sane way to remove genpds anyhow!? 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 11/02/16 09:57, Ulf Hansson wrote: > On 11 February 2016 at 10:13, Jon Hunter <jonathanh@nvidia.com> wrote: >> >> On 10/02/16 18:25, Ulf Hansson wrote: >> >> [snip] >> >>>>> Perhaps there's a way to allow the generic PM domain to control this >>>>> by itself. If we for example used the struct device corresponding to >>>>> the powergate driver, genpd could use it to distinguish between >>>>> various instances of genpd structs..!? Maybe it would simplify the way >>>>> to deal with removing domains? >>>> >>>> Yes, that would be ideal. However, would have require changing >>>> genpd_init()? I am not sure how genpd would be able to access the device >>>> struct for the powergate driver because we don't provide this via any >>>> API I am aware of? And I am guessing that you don't wish to expose the >>>> gpd_list to the world either. >>>> >>>> If there is an easy way, I am open to it, but looking at it today, I am >>>> not sure I see a simple way in which we could add a new API to do this. >>>> However, may be I am missing something! >>> >>> If we add a new __pm_genpd_init() API, that could require a struct >>> device to be provided. That API will thus invoke the existing >>> pm_genpd_init() but also deal with the extra things needed here. >>> >>> I would also allow such an API to return an error code. >>> >>> Correspondingly, pm_genpd_remove() could be required to be provided >>> with a struct device. >>> >>> Existing users of pm_genpd_init() can then convert to >>> __pm_genpd_init() whenever suitable. >>> >>> Of course, another option is just to add new member in the genpd >>> struct for the struct *device. The caller of pm_genpd_init() could >>> check it, but allow it to be NULL. Although, the pm_genpd_remove() API >>> would require that pointer to the struct device to be set... >>> >>> What do you think? >> >> Yes, sounds good. May be it is simpler just to add a new member and let >> the platform genpd driver handle it. >> >> I am wondering if in addition to pm_genpd_remove(), we then just have a >> function called pm_genpd_remove_tail(), which allows you to pass the >> struct device pointer and will remove the last pm-domain from the >> gpd_list and return the genpd pointer if successful. Internally, it will >> call pm_genpd_remove(). It seems to me that if there are nested >> pm-domains, then we probably want to remove them starting from the tail >> as opposed to the head. >> >> How does that sound? > > Why not make pm_genpd_remove() to behave as you describe for > pm_genpd_remove_tail()? > That's probably the only sane way to remove genpds anyhow!? Simply to offer flexibility. I could see that for some devices that have no dependencies between pm-domains and have a static list of pm-domains, they can simply call pm_genpd_remove() for a given pm-domain. However, that said, I can envision a case where a single pm-domain would be removed by itself and so may be there is no benefit? Cheers Jon -- 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 11/02/16 10:13, Jon Hunter wrote: > > On 11/02/16 09:57, Ulf Hansson wrote: >> On 11 February 2016 at 10:13, Jon Hunter <jonathanh@nvidia.com> wrote: >>> >>> On 10/02/16 18:25, Ulf Hansson wrote: >>> >>> [snip] >>> >>>>>> Perhaps there's a way to allow the generic PM domain to control this >>>>>> by itself. If we for example used the struct device corresponding to >>>>>> the powergate driver, genpd could use it to distinguish between >>>>>> various instances of genpd structs..!? Maybe it would simplify the way >>>>>> to deal with removing domains? >>>>> >>>>> Yes, that would be ideal. However, would have require changing >>>>> genpd_init()? I am not sure how genpd would be able to access the device >>>>> struct for the powergate driver because we don't provide this via any >>>>> API I am aware of? And I am guessing that you don't wish to expose the >>>>> gpd_list to the world either. >>>>> >>>>> If there is an easy way, I am open to it, but looking at it today, I am >>>>> not sure I see a simple way in which we could add a new API to do this. >>>>> However, may be I am missing something! >>>> >>>> If we add a new __pm_genpd_init() API, that could require a struct >>>> device to be provided. That API will thus invoke the existing >>>> pm_genpd_init() but also deal with the extra things needed here. >>>> >>>> I would also allow such an API to return an error code. >>>> >>>> Correspondingly, pm_genpd_remove() could be required to be provided >>>> with a struct device. >>>> >>>> Existing users of pm_genpd_init() can then convert to >>>> __pm_genpd_init() whenever suitable. >>>> >>>> Of course, another option is just to add new member in the genpd >>>> struct for the struct *device. The caller of pm_genpd_init() could >>>> check it, but allow it to be NULL. Although, the pm_genpd_remove() API >>>> would require that pointer to the struct device to be set... >>>> >>>> What do you think? >>> >>> Yes, sounds good. May be it is simpler just to add a new member and let >>> the platform genpd driver handle it. >>> >>> I am wondering if in addition to pm_genpd_remove(), we then just have a >>> function called pm_genpd_remove_tail(), which allows you to pass the >>> struct device pointer and will remove the last pm-domain from the >>> gpd_list and return the genpd pointer if successful. Internally, it will >>> call pm_genpd_remove(). It seems to me that if there are nested >>> pm-domains, then we probably want to remove them starting from the tail >>> as opposed to the head. >>> >>> How does that sound? >> >> Why not make pm_genpd_remove() to behave as you describe for >> pm_genpd_remove_tail()? >> That's probably the only sane way to remove genpds anyhow!? > > Simply to offer flexibility. I could see that for some devices that have > no dependencies between pm-domains and have a static list of pm-domains, > they can simply call pm_genpd_remove() for a given pm-domain. However, > that said, I can envision a case where a single pm-domain would be > removed by itself and so may be there is no benefit? By the way, do you think that instead of passing the struct device * to pm_genpd_remove(), we should just have a void *dev_id in the same way the request_irq()/free_irq() work? In other words, it would allow people to use the struct device or struct device_node, etc? Cheers Jon -- 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 11 February 2016 at 11:13, Jon Hunter <jonathanh@nvidia.com> wrote: > > On 11/02/16 09:57, Ulf Hansson wrote: >> On 11 February 2016 at 10:13, Jon Hunter <jonathanh@nvidia.com> wrote: >>> >>> On 10/02/16 18:25, Ulf Hansson wrote: >>> >>> [snip] >>> >>>>>> Perhaps there's a way to allow the generic PM domain to control this >>>>>> by itself. If we for example used the struct device corresponding to >>>>>> the powergate driver, genpd could use it to distinguish between >>>>>> various instances of genpd structs..!? Maybe it would simplify the way >>>>>> to deal with removing domains? >>>>> >>>>> Yes, that would be ideal. However, would have require changing >>>>> genpd_init()? I am not sure how genpd would be able to access the device >>>>> struct for the powergate driver because we don't provide this via any >>>>> API I am aware of? And I am guessing that you don't wish to expose the >>>>> gpd_list to the world either. >>>>> >>>>> If there is an easy way, I am open to it, but looking at it today, I am >>>>> not sure I see a simple way in which we could add a new API to do this. >>>>> However, may be I am missing something! >>>> >>>> If we add a new __pm_genpd_init() API, that could require a struct >>>> device to be provided. That API will thus invoke the existing >>>> pm_genpd_init() but also deal with the extra things needed here. >>>> >>>> I would also allow such an API to return an error code. >>>> >>>> Correspondingly, pm_genpd_remove() could be required to be provided >>>> with a struct device. >>>> >>>> Existing users of pm_genpd_init() can then convert to >>>> __pm_genpd_init() whenever suitable. >>>> >>>> Of course, another option is just to add new member in the genpd >>>> struct for the struct *device. The caller of pm_genpd_init() could >>>> check it, but allow it to be NULL. Although, the pm_genpd_remove() API >>>> would require that pointer to the struct device to be set... >>>> >>>> What do you think? >>> >>> Yes, sounds good. May be it is simpler just to add a new member and let >>> the platform genpd driver handle it. >>> >>> I am wondering if in addition to pm_genpd_remove(), we then just have a >>> function called pm_genpd_remove_tail(), which allows you to pass the >>> struct device pointer and will remove the last pm-domain from the >>> gpd_list and return the genpd pointer if successful. Internally, it will >>> call pm_genpd_remove(). It seems to me that if there are nested >>> pm-domains, then we probably want to remove them starting from the tail >>> as opposed to the head. >>> >>> How does that sound? >> >> Why not make pm_genpd_remove() to behave as you describe for >> pm_genpd_remove_tail()? >> That's probably the only sane way to remove genpds anyhow!? > > Simply to offer flexibility. I could see that for some devices that have > no dependencies between pm-domains and have a static list of pm-domains, > they can simply call pm_genpd_remove() for a given pm-domain. However, > that said, I can envision a case where a single pm-domain would be > removed by itself and so may be there is no benefit? If I understand correctly, you agree to try with the most simple approach first and thus without providing too much flexibility. Anyway, I am looking forward to review your next version of the patchset! :-) 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
[...] >>> >>> Why not make pm_genpd_remove() to behave as you describe for >>> pm_genpd_remove_tail()? >>> That's probably the only sane way to remove genpds anyhow!? >> >> Simply to offer flexibility. I could see that for some devices that have >> no dependencies between pm-domains and have a static list of pm-domains, >> they can simply call pm_genpd_remove() for a given pm-domain. However, >> that said, I can envision a case where a single pm-domain would be >> removed by itself and so may be there is no benefit? > > By the way, do you think that instead of passing the struct device * to > pm_genpd_remove(), we should just have a void *dev_id in the same way > the request_irq()/free_irq() work? In other words, it would allow people > to use the struct device or struct device_node, etc? Hmm. Do you think that would make a difference for the power controller drivers? I am thinking that genpd might perhaps benefit from being able to use the device pointer for other purposes as well!? Giving a void *, will prevent that, won't it? 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 11/02/16 10:37, Ulf Hansson wrote: > [...] > >>>> >>>> Why not make pm_genpd_remove() to behave as you describe for >>>> pm_genpd_remove_tail()? >>>> That's probably the only sane way to remove genpds anyhow!? >>> >>> Simply to offer flexibility. I could see that for some devices that have >>> no dependencies between pm-domains and have a static list of pm-domains, >>> they can simply call pm_genpd_remove() for a given pm-domain. However, >>> that said, I can envision a case where a single pm-domain would be >>> removed by itself and so may be there is no benefit? >> >> By the way, do you think that instead of passing the struct device * to >> pm_genpd_remove(), we should just have a void *dev_id in the same way >> the request_irq()/free_irq() work? In other words, it would allow people >> to use the struct device or struct device_node, etc? > > Hmm. Do you think that would make a difference for the power controller drivers? > > I am thinking that genpd might perhaps benefit from being able to use > the device pointer for other purposes as well!? > Giving a void *, will prevent that, won't it? Yes it will. Ok, let's stick with struct device for now. Cheers Jon -- 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 11/02/16 10:28, Ulf Hansson wrote: > On 11 February 2016 at 11:13, Jon Hunter <jonathanh@nvidia.com> wrote: >> >> On 11/02/16 09:57, Ulf Hansson wrote: >>> On 11 February 2016 at 10:13, Jon Hunter <jonathanh@nvidia.com> wrote: >>>> >>>> On 10/02/16 18:25, Ulf Hansson wrote: >>>> >>>> [snip] >>>> >>>>>>> Perhaps there's a way to allow the generic PM domain to control this >>>>>>> by itself. If we for example used the struct device corresponding to >>>>>>> the powergate driver, genpd could use it to distinguish between >>>>>>> various instances of genpd structs..!? Maybe it would simplify the way >>>>>>> to deal with removing domains? >>>>>> >>>>>> Yes, that would be ideal. However, would have require changing >>>>>> genpd_init()? I am not sure how genpd would be able to access the device >>>>>> struct for the powergate driver because we don't provide this via any >>>>>> API I am aware of? And I am guessing that you don't wish to expose the >>>>>> gpd_list to the world either. >>>>>> >>>>>> If there is an easy way, I am open to it, but looking at it today, I am >>>>>> not sure I see a simple way in which we could add a new API to do this. >>>>>> However, may be I am missing something! >>>>> >>>>> If we add a new __pm_genpd_init() API, that could require a struct >>>>> device to be provided. That API will thus invoke the existing >>>>> pm_genpd_init() but also deal with the extra things needed here. >>>>> >>>>> I would also allow such an API to return an error code. >>>>> >>>>> Correspondingly, pm_genpd_remove() could be required to be provided >>>>> with a struct device. >>>>> >>>>> Existing users of pm_genpd_init() can then convert to >>>>> __pm_genpd_init() whenever suitable. >>>>> >>>>> Of course, another option is just to add new member in the genpd >>>>> struct for the struct *device. The caller of pm_genpd_init() could >>>>> check it, but allow it to be NULL. Although, the pm_genpd_remove() API >>>>> would require that pointer to the struct device to be set... >>>>> >>>>> What do you think? >>>> >>>> Yes, sounds good. May be it is simpler just to add a new member and let >>>> the platform genpd driver handle it. >>>> >>>> I am wondering if in addition to pm_genpd_remove(), we then just have a >>>> function called pm_genpd_remove_tail(), which allows you to pass the >>>> struct device pointer and will remove the last pm-domain from the >>>> gpd_list and return the genpd pointer if successful. Internally, it will >>>> call pm_genpd_remove(). It seems to me that if there are nested >>>> pm-domains, then we probably want to remove them starting from the tail >>>> as opposed to the head. >>>> >>>> How does that sound? >>> >>> Why not make pm_genpd_remove() to behave as you describe for >>> pm_genpd_remove_tail()? >>> That's probably the only sane way to remove genpds anyhow!? >> >> Simply to offer flexibility. I could see that for some devices that have >> no dependencies between pm-domains and have a static list of pm-domains, >> they can simply call pm_genpd_remove() for a given pm-domain. However, >> that said, I can envision a case where a single pm-domain would be >> removed by itself and so may be there is no benefit? > > If I understand correctly, you agree to try with the most simple > approach first and thus without providing too much flexibility. > > Anyway, I am looking forward to review your next version of the patchset! :-) So one snag I hit with this, is that in order to remove a pm-domain, I first need to check if it is a subdomain of any other domains and if so remove it as a subdomain first. Before, this was simple because I called pm_genpd_remove_subdomain if the domain had a parent, and then called pm_genpd_remove(). With the proposal we have discussed, I no longer have any knowledge of whether the pm-domain is a subdomain or not because pm_genpd_remove() would remove the tail and then return it. So this means that I now need to handle the removal of the subdomain within pm_genpd_remove(), which is ok, but creates more changes as I need to parse the slave_links list, etc. I am wondering if it would be better to add a simple function called pm_genpd_list_get_tail(struct device *dev) that would return the last pm-domain register for a given device and then simply call pm_genpd_remove_subdomain() (if needed) and pm_genpd_remove()? Let me know what you think. Cheers Jon -- 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: > On 28 January 2016 at 17:33, Jon Hunter <jonathanh@nvidia.com> wrote: >> Adds generic PM support to the PMC driver where the PM domains are >> populated from device-tree and the PM domain consumer devices are >> bound to their relevant PM domains via device-tree as well. >> >> Update the tegra_powergate_sequence_power_up() API so that internally >> it calls the same tegra_powergate_xxx functions that are used by the >> tegra generic power domain code for consistency. >> >> This is based upon work by Thierry Reding <treding@nvidia.com> >> and Vince Hsu <vinceh@nvidia.com>. >> >> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> [...] >> +static void tegra_powergate_disable_clocks(struct tegra_powergate *pg) >> +{ >> + unsigned int i; >> + >> + for (i = 0; i < pg->num_clks; i++) >> + clk_disable_unprepare(pg->clks[i]); >> +} >> + >> +static int tegra_powergate_enable_clocks(struct tegra_powergate *pg) >> +{ >> + unsigned int i; >> + int err; >> + >> + for (i = 0; i < pg->num_clks; i++) { >> + err = clk_prepare_enable(pg->clks[i]); >> + if (err) >> + goto out; >> + } >> + >> + return 0; >> + >> +out: >> + while (i--) >> + clk_disable_unprepare(pg->clks[i]); >> + >> + return err; >> +} > > I have seen similar code around in other PM domains, dealing with > enabling/disabling a *list* of clocks. > Perhaps we should invent a new clock API that helps with this to > prevents code duplication!? What about the pm_clk_* API which was built for tracking clocks associated with devices for runtime PM. IOW, you could pm_clk_add(pg->pmc->dev, pg->clks[i]) and then your _enable_clocks() would become pm_clk_suspend() an dyour _disable_clocks() would become pm_clk_resume(). I might not be following the mapping between PMC and PGs though so not sure pg->pmc->dev is the right struct device, but you get the idea. 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
On 12/02/16 23:14, Kevin Hilman wrote: > Ulf Hansson <ulf.hansson@linaro.org> writes: > >> On 28 January 2016 at 17:33, Jon Hunter <jonathanh@nvidia.com> wrote: >>> Adds generic PM support to the PMC driver where the PM domains are >>> populated from device-tree and the PM domain consumer devices are >>> bound to their relevant PM domains via device-tree as well. >>> >>> Update the tegra_powergate_sequence_power_up() API so that internally >>> it calls the same tegra_powergate_xxx functions that are used by the >>> tegra generic power domain code for consistency. >>> >>> This is based upon work by Thierry Reding <treding@nvidia.com> >>> and Vince Hsu <vinceh@nvidia.com>. >>> >>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > > [...] > >>> +static void tegra_powergate_disable_clocks(struct tegra_powergate *pg) >>> +{ >>> + unsigned int i; >>> + >>> + for (i = 0; i < pg->num_clks; i++) >>> + clk_disable_unprepare(pg->clks[i]); >>> +} >>> + >>> +static int tegra_powergate_enable_clocks(struct tegra_powergate *pg) >>> +{ >>> + unsigned int i; >>> + int err; >>> + >>> + for (i = 0; i < pg->num_clks; i++) { >>> + err = clk_prepare_enable(pg->clks[i]); >>> + if (err) >>> + goto out; >>> + } >>> + >>> + return 0; >>> + >>> +out: >>> + while (i--) >>> + clk_disable_unprepare(pg->clks[i]); >>> + >>> + return err; >>> +} >> >> I have seen similar code around in other PM domains, dealing with >> enabling/disabling a *list* of clocks. >> Perhaps we should invent a new clock API that helps with this to >> prevents code duplication!? > > What about the pm_clk_* API which was built for tracking clocks > associated with devices for runtime PM. > > IOW, you could pm_clk_add(pg->pmc->dev, pg->clks[i]) and then your > _enable_clocks() would become pm_clk_suspend() an dyour > _disable_clocks() would become pm_clk_resume(). Very interesting, I was not aware of this. > I might not be following the mapping between PMC and PGs though so not > sure pg->pmc->dev is the right struct device, but you get the idea. Yes, so this will not work here as-is, because the pmc->dev is common to all pm-domains (it is the device that creates all the pm-domains). So to make this work, I would need to create a device for each pm-domain and add the clocks to that. I see that this works very well for normal drivers, but it does not feel so natural for pm-domains where we don't have a device struct today. By the way, the rockchip pm-domains implementation is very much in the same boat as tegra, where there are multiple clocks per pm-domain and it is handled by a simple list. So I am not sure if you think that we should be turning all pm-domains registered by pm_genpd_init() into a device and then we can make use of these pm_clk_XXXX() APIs? I have implemented the generic clk APIs that Ulf and I discussed for handling multiple clocks, but if we think that this is a better way, then I will hold off for now. Cheers Jon -- 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 11 February 2016 at 17:38, Jon Hunter <jonathanh@nvidia.com> wrote: > > On 11/02/16 10:28, Ulf Hansson wrote: >> On 11 February 2016 at 11:13, Jon Hunter <jonathanh@nvidia.com> wrote: >>> >>> On 11/02/16 09:57, Ulf Hansson wrote: >>>> On 11 February 2016 at 10:13, Jon Hunter <jonathanh@nvidia.com> wrote: >>>>> >>>>> On 10/02/16 18:25, Ulf Hansson wrote: >>>>> >>>>> [snip] >>>>> >>>>>>>> Perhaps there's a way to allow the generic PM domain to control this >>>>>>>> by itself. If we for example used the struct device corresponding to >>>>>>>> the powergate driver, genpd could use it to distinguish between >>>>>>>> various instances of genpd structs..!? Maybe it would simplify the way >>>>>>>> to deal with removing domains? >>>>>>> >>>>>>> Yes, that would be ideal. However, would have require changing >>>>>>> genpd_init()? I am not sure how genpd would be able to access the device >>>>>>> struct for the powergate driver because we don't provide this via any >>>>>>> API I am aware of? And I am guessing that you don't wish to expose the >>>>>>> gpd_list to the world either. >>>>>>> >>>>>>> If there is an easy way, I am open to it, but looking at it today, I am >>>>>>> not sure I see a simple way in which we could add a new API to do this. >>>>>>> However, may be I am missing something! >>>>>> >>>>>> If we add a new __pm_genpd_init() API, that could require a struct >>>>>> device to be provided. That API will thus invoke the existing >>>>>> pm_genpd_init() but also deal with the extra things needed here. >>>>>> >>>>>> I would also allow such an API to return an error code. >>>>>> >>>>>> Correspondingly, pm_genpd_remove() could be required to be provided >>>>>> with a struct device. >>>>>> >>>>>> Existing users of pm_genpd_init() can then convert to >>>>>> __pm_genpd_init() whenever suitable. >>>>>> >>>>>> Of course, another option is just to add new member in the genpd >>>>>> struct for the struct *device. The caller of pm_genpd_init() could >>>>>> check it, but allow it to be NULL. Although, the pm_genpd_remove() API >>>>>> would require that pointer to the struct device to be set... >>>>>> >>>>>> What do you think? >>>>> >>>>> Yes, sounds good. May be it is simpler just to add a new member and let >>>>> the platform genpd driver handle it. >>>>> >>>>> I am wondering if in addition to pm_genpd_remove(), we then just have a >>>>> function called pm_genpd_remove_tail(), which allows you to pass the >>>>> struct device pointer and will remove the last pm-domain from the >>>>> gpd_list and return the genpd pointer if successful. Internally, it will >>>>> call pm_genpd_remove(). It seems to me that if there are nested >>>>> pm-domains, then we probably want to remove them starting from the tail >>>>> as opposed to the head. >>>>> >>>>> How does that sound? >>>> >>>> Why not make pm_genpd_remove() to behave as you describe for >>>> pm_genpd_remove_tail()? >>>> That's probably the only sane way to remove genpds anyhow!? >>> >>> Simply to offer flexibility. I could see that for some devices that have >>> no dependencies between pm-domains and have a static list of pm-domains, >>> they can simply call pm_genpd_remove() for a given pm-domain. However, >>> that said, I can envision a case where a single pm-domain would be >>> removed by itself and so may be there is no benefit? >> >> If I understand correctly, you agree to try with the most simple >> approach first and thus without providing too much flexibility. >> >> Anyway, I am looking forward to review your next version of the patchset! :-) > > So one snag I hit with this, is that in order to remove a pm-domain, I > first need to check if it is a subdomain of any other domains and if so > remove it as a subdomain first. Before, this was simple because I called > pm_genpd_remove_subdomain if the domain had a parent, and then called > pm_genpd_remove(). You certainly have a point here. One more thing, we not only have to check whether the domain is a subdomain. we also need to check if the domain is a parent (master) for other subdomains. It being a parent, prevents us from removing it until all its subdomains are removed. > > With the proposal we have discussed, I no longer have any knowledge of > whether the pm-domain is a subdomain or not because pm_genpd_remove() > would remove the tail and then return it. So this means that I now need > to handle the removal of the subdomain within pm_genpd_remove(), which > is ok, but creates more changes as I need to parse the slave_links list, > etc. > > I am wondering if it would be better to add a simple function called > pm_genpd_list_get_tail(struct device *dev) that would return the last > pm-domain register for a given device and then simply call > pm_genpd_remove_subdomain() (if needed) and pm_genpd_remove()? That should work. Please go ahead and have try! 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
[...] >> >> What about the pm_clk_* API which was built for tracking clocks >> associated with devices for runtime PM. >> >> IOW, you could pm_clk_add(pg->pmc->dev, pg->clks[i]) and then your >> _enable_clocks() would become pm_clk_suspend() an dyour >> _disable_clocks() would become pm_clk_resume(). > > Very interesting, I was not aware of this. > >> I might not be following the mapping between PMC and PGs though so not >> sure pg->pmc->dev is the right struct device, but you get the idea. > > Yes, so this will not work here as-is, because the pmc->dev is common to > all pm-domains (it is the device that creates all the pm-domains). So to > make this work, I would need to create a device for each pm-domain and > add the clocks to that. > > I see that this works very well for normal drivers, but it does not feel > so natural for pm-domains where we don't have a device struct today. By > the way, the rockchip pm-domains implementation is very much in the same > boat as tegra, where there are multiple clocks per pm-domain and it is > handled by a simple list. So I am not sure if you think that we should > be turning all pm-domains registered by pm_genpd_init() into a device > and then we can make use of these pm_clk_XXXX() APIs? > > I have implemented the generic clk APIs that Ulf and I discussed for > handling multiple clocks, but if we think that this is a better way, > then I will hold off for now. I think Kevin has a point that we already have PM clocks to build upon. Could we perhaps try to extend that API instead to suite this needs as well? I do realize that it will make this patchset more complicated. As I stated earlier, this was just an idea I had, so to be clear I won't hold back an ack for this patchset, if you decide to deal with this in separate "improvement" step. 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 18/02/16 16:00, Ulf Hansson wrote: > [...] > >>> >>> What about the pm_clk_* API which was built for tracking clocks >>> associated with devices for runtime PM. >>> >>> IOW, you could pm_clk_add(pg->pmc->dev, pg->clks[i]) and then your >>> _enable_clocks() would become pm_clk_suspend() an dyour >>> _disable_clocks() would become pm_clk_resume(). >> >> Very interesting, I was not aware of this. >> >>> I might not be following the mapping between PMC and PGs though so not >>> sure pg->pmc->dev is the right struct device, but you get the idea. >> >> Yes, so this will not work here as-is, because the pmc->dev is common to >> all pm-domains (it is the device that creates all the pm-domains). So to >> make this work, I would need to create a device for each pm-domain and >> add the clocks to that. >> >> I see that this works very well for normal drivers, but it does not feel >> so natural for pm-domains where we don't have a device struct today. By >> the way, the rockchip pm-domains implementation is very much in the same >> boat as tegra, where there are multiple clocks per pm-domain and it is >> handled by a simple list. So I am not sure if you think that we should >> be turning all pm-domains registered by pm_genpd_init() into a device >> and then we can make use of these pm_clk_XXXX() APIs? >> >> I have implemented the generic clk APIs that Ulf and I discussed for >> handling multiple clocks, but if we think that this is a better way, >> then I will hold off for now. > > I think Kevin has a point that we already have PM clocks to build upon. > Could we perhaps try to extend that API instead to suite this needs as well? We certainly could and I am not against it, however, it means that we need to create a device structure for each pm-domain. If you and Kevin are ok with me adding this to pm_genpd_init(), then I can give it a try. > I do realize that it will make this patchset more complicated. As I > stated earlier, this was just an idea I had, so to be clear I won't > hold back an ack for this patchset, if you decide to deal with this in > separate "improvement" step. Thanks Jon -- 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 Hunter <jonathanh@nvidia.com> writes: > On 18/02/16 16:00, Ulf Hansson wrote: >> [...] >> >>>> >>>> What about the pm_clk_* API which was built for tracking clocks >>>> associated with devices for runtime PM. >>>> >>>> IOW, you could pm_clk_add(pg->pmc->dev, pg->clks[i]) and then your >>>> _enable_clocks() would become pm_clk_suspend() an dyour >>>> _disable_clocks() would become pm_clk_resume(). >>> >>> Very interesting, I was not aware of this. >>> >>>> I might not be following the mapping between PMC and PGs though so not >>>> sure pg->pmc->dev is the right struct device, but you get the idea. >>> >>> Yes, so this will not work here as-is, because the pmc->dev is common to >>> all pm-domains (it is the device that creates all the pm-domains). So to >>> make this work, I would need to create a device for each pm-domain and >>> add the clocks to that. >>> >>> I see that this works very well for normal drivers, but it does not feel >>> so natural for pm-domains where we don't have a device struct today. By >>> the way, the rockchip pm-domains implementation is very much in the same >>> boat as tegra, where there are multiple clocks per pm-domain and it is >>> handled by a simple list. So I am not sure if you think that we should >>> be turning all pm-domains registered by pm_genpd_init() into a device >>> and then we can make use of these pm_clk_XXXX() APIs? >>> >>> I have implemented the generic clk APIs that Ulf and I discussed for >>> handling multiple clocks, but if we think that this is a better way, >>> then I will hold off for now. >> >> I think Kevin has a point that we already have PM clocks to build upon. >> Could we perhaps try to extend that API instead to suite this needs as well? > > We certainly could and I am not against it, however, it means that we > need to create a device structure for each pm-domain. If you and Kevin > are ok with me adding this to pm_genpd_init(), then I can give it a try. At this point, I'm thinking that the added complexity of a per-pm-domain struct device isn't justified. Managing simple lists of clocks in the SoC specific PM domains is quite easy to review and maintain, IMO. So I recommend just keeping it that way for now. If it starts to get unwieldy for tegra, rockchip and any others, we can revisit a common way of doing it then. 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
diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c index ecb4f66819fd..915dc37e0d92 100644 --- a/drivers/soc/tegra/pmc.c +++ b/drivers/soc/tegra/pmc.c @@ -19,6 +19,8 @@ #define pr_fmt(fmt) "tegra-pmc: " fmt +#include <dt-bindings/power/tegra-powergate.h> + #include <linux/kernel.h> #include <linux/clk.h> #include <linux/clk/tegra.h> @@ -31,7 +33,9 @@ #include <linux/iopoll.h> #include <linux/of.h> #include <linux/of_address.h> +#include <linux/of_platform.h> #include <linux/platform_device.h> +#include <linux/pm_domain.h> #include <linux/reboot.h> #include <linux/reset.h> #include <linux/seq_file.h> @@ -102,6 +106,19 @@ #define GPU_RG_CNTRL 0x2d4 +struct tegra_powergate { + struct generic_pm_domain genpd; + struct generic_pm_domain *parent; + struct tegra_pmc *pmc; + unsigned int id; + struct list_head node; + struct device_node *of_node; + struct clk **clks; + unsigned int num_clks; + struct reset_control **resets; + unsigned int num_resets; +}; + struct tegra_pmc_soc { unsigned int num_powergates; const char *const *powergates; @@ -134,6 +151,7 @@ struct tegra_pmc_soc { * @lp0_vec_phys: physical base address of the LP0 warm boot code * @lp0_vec_size: size of the LP0 warm boot code * @powergates_valid: Bitmap of valid power gates + * @powergates_list: list of power gates * @powergates_lock: mutex for power gate register access */ struct tegra_pmc { @@ -160,6 +178,7 @@ struct tegra_pmc { u32 lp0_vec_size; DECLARE_BITMAP(powergates_valid, TEGRA_POWERGATE_MAX); + struct list_head powergates_list; struct mutex powergates_lock; }; @@ -168,6 +187,12 @@ static struct tegra_pmc *pmc = &(struct tegra_pmc) { .suspend_mode = TEGRA_SUSPEND_NONE, }; +static inline struct tegra_powergate * +to_powergate(struct generic_pm_domain *domain) +{ + return container_of(domain, struct tegra_powergate, genpd); +} + static u32 tegra_pmc_readl(unsigned long offset) { return readl(pmc->base + offset); @@ -218,6 +243,174 @@ static int tegra_powergate_set(unsigned int id, bool new_state) return err; } +static void tegra_powergate_disable_clocks(struct tegra_powergate *pg) +{ + unsigned int i; + + for (i = 0; i < pg->num_clks; i++) + clk_disable_unprepare(pg->clks[i]); +} + +static int tegra_powergate_enable_clocks(struct tegra_powergate *pg) +{ + unsigned int i; + int err; + + for (i = 0; i < pg->num_clks; i++) { + err = clk_prepare_enable(pg->clks[i]); + if (err) + goto out; + } + + return 0; + +out: + while (i--) + clk_disable_unprepare(pg->clks[i]); + + return err; +} + +static int tegra_powergate_reset_assert(struct tegra_powergate *pg) +{ + unsigned int i; + int err; + + for (i = 0; i < pg->num_resets; i++) { + err = reset_control_assert(pg->resets[i]); + if (err) + return err; + } + + return 0; +} + +static int tegra_powergate_reset_deassert(struct tegra_powergate *pg) +{ + unsigned int i; + int err; + + for (i = 0; i < pg->num_resets; i++) { + err = reset_control_deassert(pg->resets[i]); + if (err) + return err; + } + + return 0; +} + +static int tegra_powergate_power_up(struct tegra_powergate *pg, + bool disable_clocks) +{ + int err; + + err = tegra_powergate_reset_assert(pg); + if (err) + return err; + + usleep_range(10, 20); + + err = tegra_powergate_set(pg->id, true); + if (err < 0) + return err; + + usleep_range(10, 20); + + err = tegra_powergate_enable_clocks(pg); + if (err) + goto err_clks; + + usleep_range(10, 20); + + tegra_powergate_remove_clamping(pg->id); + + usleep_range(10, 20); + + err = tegra_powergate_reset_deassert(pg); + if (err) + goto err_reset; + + usleep_range(10, 20); + + if (disable_clocks) + tegra_powergate_disable_clocks(pg); + + return 0; + +err_reset: + tegra_powergate_disable_clocks(pg); + usleep_range(10, 20); +err_clks: + tegra_powergate_set(pg->id, false); + + return err; +} + +static int tegra_powergate_power_down(struct tegra_powergate *pg) +{ + int err; + + err = tegra_powergate_enable_clocks(pg); + if (err) + return err; + + usleep_range(10, 20); + + err = tegra_powergate_reset_assert(pg); + if (err) + goto err_reset; + + usleep_range(10, 20); + + tegra_powergate_disable_clocks(pg); + + usleep_range(10, 20); + + err = tegra_powergate_set(pg->id, false); + if (err) + goto err_powergate; + + return 0; + +err_powergate: + tegra_powergate_enable_clocks(pg); + usleep_range(10, 20); + tegra_powergate_reset_deassert(pg); + usleep_range(10, 20); +err_reset: + tegra_powergate_disable_clocks(pg); + + return err; +} + +static int tegra_genpd_power_on(struct generic_pm_domain *domain) +{ + struct tegra_powergate *pg = to_powergate(domain); + struct tegra_pmc *pmc = pg->pmc; + int err; + + err = tegra_powergate_power_up(pg, true); + if (err) + dev_err(pmc->dev, "failed to turn on PM domain %s: %d\n", + pg->of_node->name, err); + + return err; +} + +static int tegra_genpd_power_off(struct generic_pm_domain *domain) +{ + struct tegra_powergate *pg = to_powergate(domain); + struct tegra_pmc *pmc = pg->pmc; + int err; + + err = tegra_powergate_power_down(pg); + if (err) + dev_err(pmc->dev, "failed to turn off PM domain %s: %d\n", + pg->of_node->name, err); + + return err; +} + /** * tegra_powergate_power_on() - power on partition * @id: partition ID @@ -319,35 +512,20 @@ EXPORT_SYMBOL(tegra_powergate_remove_clamping); int tegra_powergate_sequence_power_up(unsigned int id, struct clk *clk, struct reset_control *rst) { - int ret; - - reset_control_assert(rst); - - ret = tegra_powergate_power_on(id); - if (ret) - goto err_power; - - ret = clk_prepare_enable(clk); - if (ret) - goto err_clk; - - usleep_range(10, 20); + struct tegra_powergate pg; + int err; - ret = tegra_powergate_remove_clamping(id); - if (ret) - goto err_clamp; + pg.id = id; + pg.clks = &clk; + pg.num_clks = 1; + pg.resets = &rst; + pg.num_resets = 1; - usleep_range(10, 20); - reset_control_deassert(rst); + err = tegra_powergate_power_up(&pg, false); + if (err) + pr_err("failed to turn on partition %d: %d\n", id, err); - return 0; - -err_clamp: - clk_disable_unprepare(clk); -err_clk: - tegra_powergate_power_off(id); -err_power: - return ret; + return err; } EXPORT_SYMBOL(tegra_powergate_sequence_power_up); @@ -487,6 +665,233 @@ static int tegra_powergate_debugfs_init(void) return 0; } +static int tegra_powergate_of_get_clks(struct device *dev, + struct tegra_powergate *pg) +{ + struct clk *clk; + unsigned int i; + int err; + + pg->num_clks = of_count_phandle_with_args(pg->of_node, "clocks", + "#clock-cells"); + if (pg->num_clks == 0) + return -ENODEV; + + pg->clks = devm_kcalloc(dev, pg->num_clks, sizeof(clk), GFP_KERNEL); + if (!pg->clks) + return -ENOMEM; + + for (i = 0; i < pg->num_clks; i++) { + pg->clks[i] = of_clk_get(pg->of_node, i); + if (IS_ERR(pg->clks[i])) { + err = PTR_ERR(pg->clks[i]); + goto err; + } + } + + return 0; + +err: + while (i--) + clk_put(pg->clks[i]); + + pg->num_clks = 0; + + return err; +} + +static int tegra_powergate_of_get_resets(struct device *dev, + struct tegra_powergate *pg) +{ + struct reset_control *rst; + unsigned int i; + int err; + + pg->num_resets = of_count_phandle_with_args(pg->of_node, "resets", + "#reset-cells"); + if (pg->num_resets == 0) + return -ENODEV; + + pg->resets = devm_kcalloc(dev, pg->num_resets, sizeof(rst), GFP_KERNEL); + if (!pg->resets) + return -ENOMEM; + + for (i = 0; i < pg->num_resets; i++) { + pg->resets[i] = of_reset_control_get_by_index(pg->of_node, i); + if (IS_ERR(pg->resets[i])) { + err = PTR_ERR(pg->resets[i]); + goto err; + } + } + + return 0; + +err: + while (i--) + reset_control_put(pg->resets[i]); + + pg->num_resets = 0; + + return err; +} + +static struct tegra_powergate * +tegra_powergate_add_one(struct tegra_pmc *pmc, struct device_node *np, + struct generic_pm_domain *parent) +{ + struct tegra_powergate *pg; + unsigned int id; + bool off; + int err; + + /* + * If the powergate ID is missing or invalid then return NULL + * to skip this one and allow any others to be created. + */ + err = of_property_read_u32(np, "nvidia,powergate", &id); + if (err) { + dev_WARN(pmc->dev, "no powergate ID for node %s\n", np->name); + return NULL; + } + + if (!tegra_powergate_is_valid(id)) { + dev_WARN(pmc->dev, "powergate ID invalid for %s\n", np->name); + return NULL; + } + + pg = devm_kzalloc(pmc->dev, sizeof(*pg), GFP_KERNEL); + if (!pg) { + err = -ENOMEM; + goto error; + } + + pg->id = id; + pg->of_node = np; + pg->parent = parent; + pg->genpd.name = np->name; + pg->genpd.power_off = tegra_genpd_power_off; + pg->genpd.power_on = tegra_genpd_power_on; + pg->pmc = pmc; + + err = tegra_powergate_of_get_clks(pmc->dev, pg); + if (err) + goto error; + + err = tegra_powergate_of_get_resets(pmc->dev, pg); + if (err) + goto remove_clks; + + off = !tegra_powergate_is_powered(pg->id); + + pm_genpd_init(&pg->genpd, NULL, off); + + if (pg->parent) { + err = pm_genpd_add_subdomain(pg->parent, &pg->genpd); + if (err) + goto remove_domain_and_resets; + } + + err = of_genpd_add_provider_simple(pg->of_node, &pg->genpd); + if (err) + goto remove_subdomain; + + list_add_tail(&pg->node, &pmc->powergates_list); + + dev_dbg(pmc->dev, "added power domain %s\n", pg->genpd.name); + + return pg; + +remove_subdomain: + WARN_ON(pm_genpd_remove_subdomain(pg->parent, &pg->genpd)); +remove_domain_and_resets: + WARN_ON(pm_genpd_remove(&pg->genpd)); + while (pg->num_resets--) + reset_control_put(pg->resets[pg->num_resets]); +remove_clks: + while (pg->num_clks--) + clk_put(pg->clks[pg->num_clks]); +error: + dev_err(pmc->dev, "failed to create power domain for node %s\n", + np->name); + + return ERR_PTR(err); +} + +static int tegra_powergate_add(struct tegra_pmc *pmc, struct device_node *np, + struct generic_pm_domain *parent) +{ + struct tegra_powergate *pg; + struct device_node *child; + int err = 0; + + for_each_child_of_node(np, child) { + pg = tegra_powergate_add_one(pmc, child, parent); + if (IS_ERR(pg)) { + err = PTR_ERR(pg); + goto err; + } + + if (pg) + err = tegra_powergate_add(pmc, child, pg->parent); + +err: + of_node_put(child); + + if (err) + break; + } + + return err; +} + +static void tegra_powergate_remove(struct tegra_pmc *pmc) +{ + struct tegra_powergate *pg, *n; + + list_for_each_entry_safe(pg, n, &pmc->powergates_list, node) { + of_genpd_del_provider(pg->of_node); + + if (pg->parent) { + if (WARN_ON(pm_genpd_remove_subdomain(pg->parent, + &pg->genpd))) + return; + + pg->parent = NULL; + } + + if (WARN_ON(pm_genpd_remove(&pg->genpd))) + return; + + while (pg->num_clks--) + clk_put(pg->clks[pg->num_clks]); + + while (pg->num_resets--) + reset_control_put(pg->resets[pg->num_resets]); + + list_del(&pg->node); + } +} + +static int tegra_powergate_init(struct tegra_pmc *pmc) +{ + struct device_node *np; + int err; + + INIT_LIST_HEAD(&pmc->powergates_list); + + np = of_get_child_by_name(pmc->dev->of_node, "powergates"); + if (!np) + return 0; + + err = tegra_powergate_add(pmc, np, NULL); + if (err) + tegra_powergate_remove(pmc); + + of_node_put(np); + + return err; +} + static int tegra_io_rail_prepare(unsigned int id, unsigned long *request, unsigned long *status, unsigned int *bit) { @@ -880,24 +1285,31 @@ static int tegra_pmc_probe(struct platform_device *pdev) tegra_pmc_init_tsense_reset(pmc); + err = tegra_powergate_init(pmc); + if (err < 0) + goto error; + if (IS_ENABLED(CONFIG_DEBUG_FS)) { err = tegra_powergate_debugfs_init(); if (err < 0) - goto error; + goto remove_powergates; } err = register_restart_handler(&tegra_pmc_restart_handler); if (err) { - debugfs_remove(pmc->debugfs); dev_err(&pdev->dev, "unable to register restart handler, %d\n", err); - goto error; + goto remove_debugfs; } iounmap(base); return 0; +remove_debugfs: + debugfs_remove(pmc->debugfs); +remove_powergates: + tegra_powergate_remove(pmc); error: mutex_lock(&pmc->powergates_lock); pmc->base = base; diff --git a/include/dt-bindings/power/tegra-powergate.h b/include/dt-bindings/power/tegra-powergate.h new file mode 100644 index 000000000000..bcab501badfc --- /dev/null +++ b/include/dt-bindings/power/tegra-powergate.h @@ -0,0 +1,36 @@ +#ifndef _DT_BINDINGS_POWER_TEGRA_POWERGATE_H +#define _DT_BINDINGS_POWER_TEGRA_POWERGATE_H + +#define TEGRA_POWERGATE_CPU 0 +#define TEGRA_POWERGATE_3D 1 +#define TEGRA_POWERGATE_VENC 2 +#define TEGRA_POWERGATE_PCIE 3 +#define TEGRA_POWERGATE_VDEC 4 +#define TEGRA_POWERGATE_L2 5 +#define TEGRA_POWERGATE_MPE 6 +#define TEGRA_POWERGATE_HEG 7 +#define TEGRA_POWERGATE_SATA 8 +#define TEGRA_POWERGATE_CPU1 9 +#define TEGRA_POWERGATE_CPU2 10 +#define TEGRA_POWERGATE_CPU3 11 +#define TEGRA_POWERGATE_CELP 12 +#define TEGRA_POWERGATE_3D1 13 +#define TEGRA_POWERGATE_CPU0 14 +#define TEGRA_POWERGATE_C0NC 15 +#define TEGRA_POWERGATE_C1NC 16 +#define TEGRA_POWERGATE_SOR 17 +#define TEGRA_POWERGATE_DIS 18 +#define TEGRA_POWERGATE_DISB 19 +#define TEGRA_POWERGATE_XUSBA 20 +#define TEGRA_POWERGATE_XUSBB 21 +#define TEGRA_POWERGATE_XUSBC 22 +#define TEGRA_POWERGATE_VIC 23 +#define TEGRA_POWERGATE_IRAM 24 +#define TEGRA_POWERGATE_NVDEC 25 +#define TEGRA_POWERGATE_NVJPG 26 +#define TEGRA_POWERGATE_AUD 27 +#define TEGRA_POWERGATE_DFD 28 +#define TEGRA_POWERGATE_VE2 29 +#define TEGRA_POWERGATE_MAX TEGRA_POWERGATE_VE2 + +#endif diff --git a/include/soc/tegra/pmc.h b/include/soc/tegra/pmc.h index e9e53473a63e..c028557365ad 100644 --- a/include/soc/tegra/pmc.h +++ b/include/soc/tegra/pmc.h @@ -19,6 +19,8 @@ #ifndef __SOC_TEGRA_PMC_H__ #define __SOC_TEGRA_PMC_H__ +#include <dt-bindings/power/tegra-powergate.h> + #include <linux/reboot.h> #include <soc/tegra/pm.h> @@ -39,43 +41,8 @@ int tegra_pmc_cpu_remove_clamping(unsigned int cpuid); #endif /* CONFIG_SMP */ /* - * powergate and I/O rail APIs + * I/O rail APIs */ - -#define TEGRA_POWERGATE_CPU 0 -#define TEGRA_POWERGATE_3D 1 -#define TEGRA_POWERGATE_VENC 2 -#define TEGRA_POWERGATE_PCIE 3 -#define TEGRA_POWERGATE_VDEC 4 -#define TEGRA_POWERGATE_L2 5 -#define TEGRA_POWERGATE_MPE 6 -#define TEGRA_POWERGATE_HEG 7 -#define TEGRA_POWERGATE_SATA 8 -#define TEGRA_POWERGATE_CPU1 9 -#define TEGRA_POWERGATE_CPU2 10 -#define TEGRA_POWERGATE_CPU3 11 -#define TEGRA_POWERGATE_CELP 12 -#define TEGRA_POWERGATE_3D1 13 -#define TEGRA_POWERGATE_CPU0 14 -#define TEGRA_POWERGATE_C0NC 15 -#define TEGRA_POWERGATE_C1NC 16 -#define TEGRA_POWERGATE_SOR 17 -#define TEGRA_POWERGATE_DIS 18 -#define TEGRA_POWERGATE_DISB 19 -#define TEGRA_POWERGATE_XUSBA 20 -#define TEGRA_POWERGATE_XUSBB 21 -#define TEGRA_POWERGATE_XUSBC 22 -#define TEGRA_POWERGATE_VIC 23 -#define TEGRA_POWERGATE_IRAM 24 -#define TEGRA_POWERGATE_NVDEC 25 -#define TEGRA_POWERGATE_NVJPG 26 -#define TEGRA_POWERGATE_AUD 27 -#define TEGRA_POWERGATE_DFD 28 -#define TEGRA_POWERGATE_VE2 29 -#define TEGRA_POWERGATE_MAX TEGRA_POWERGATE_VE2 - -#define TEGRA_POWERGATE_3D0 TEGRA_POWERGATE_3D - #define TEGRA_IO_RAIL_CSIA 0 #define TEGRA_IO_RAIL_CSIB 1 #define TEGRA_IO_RAIL_DSI 2
Adds generic PM support to the PMC driver where the PM domains are populated from device-tree and the PM domain consumer devices are bound to their relevant PM domains via device-tree as well. Update the tegra_powergate_sequence_power_up() API so that internally it calls the same tegra_powergate_xxx functions that are used by the tegra generic power domain code for consistency. This is based upon work by Thierry Reding <treding@nvidia.com> and Vince Hsu <vinceh@nvidia.com>. Signed-off-by: Jon Hunter <jonathanh@nvidia.com> --- drivers/soc/tegra/pmc.c | 470 ++++++++++++++++++++++++++-- include/dt-bindings/power/tegra-powergate.h | 36 +++ include/soc/tegra/pmc.h | 39 +-- 3 files changed, 480 insertions(+), 65 deletions(-) create mode 100644 include/dt-bindings/power/tegra-powergate.h