| Message ID | 1540739690-23750-6-git-send-email-aisheng.dong@nxp.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | soc: imx: add scu power domain driver | expand |
[...] } > + > +static struct imx_sc_pm_domain * > +imx_scu_add_pm_domain(struct device *dev, int idx, > + const struct imx_sc_pd_range *pd_ranges) > +{ > + struct imx_sc_pm_domain *sc_pd; > + int ret; > + > + sc_pd = devm_kzalloc(dev, sizeof(*sc_pd), GFP_KERNEL); > + if (!sc_pd) > + return ERR_PTR(-ENOMEM); > + > + sc_pd->rsrc = pd_ranges->rsrc + idx; > + sc_pd->pd.power_off = imx_sc_pd_power_off; > + sc_pd->pd.power_on = imx_sc_pd_power_on; So, this means you are going to register one genpd per device (one for each uart, pwm etc), but there is actually a better option. What you seems to be needing is a way to "power on/off" devices, rather than the PM domain. Right? The PM domain, is managed internally by the FW, no? In any case, it looks like what you should do is to implement the ->attach|detach_dev() callback for the genpd and use the regular of_genpd_add_provider_simple(). From within the ->attach_dev(), you should get the OF node for the device that is being attached and then parse the power-domain cell containing the "resource id" and store that in the per device struct generic_pm_domain_data (we have void pointer there for storing these kind of things). Additionally, you need to implement the ->stop() and ->start() callbacks of genpd, which is where you "power on/off" devices, rather than using the above ->power_on|off() callbacks. Please have a look a drivers/soc/ti/ti_sci_pm_domains.c. Apologize, I should have pointed you towards that reference already in the earlier version. > + > + if (pd_ranges->postfix) > + snprintf(sc_pd->name, sizeof(sc_pd->name), > + "%s%i", pd_ranges->name, idx); > + else > + snprintf(sc_pd->name, sizeof(sc_pd->name), > + "%s", pd_ranges->name); > + > + sc_pd->pd.name = sc_pd->name; > + > + if (sc_pd->rsrc >= IMX_SC_R_LAST) { > + dev_warn(dev, "invalid pd %s rsrc id %d found", > + sc_pd->name, sc_pd->rsrc); > + > + devm_kfree(dev, sc_pd); > + return NULL; > + } > + > + ret = pm_genpd_init(&sc_pd->pd, NULL, true); > + if (ret) { > + dev_warn(dev, "failed to init pd %s rsrc id %d", > + sc_pd->name, sc_pd->rsrc); > + devm_kfree(dev, sc_pd); > + return NULL; > + } > + > + return sc_pd; > +} [...] Kind regards Uffe
[...] > } > > + > > +static struct imx_sc_pm_domain * > > +imx_scu_add_pm_domain(struct device *dev, int idx, > > + const struct imx_sc_pd_range *pd_ranges) { > > + struct imx_sc_pm_domain *sc_pd; > > + int ret; > > + > > + sc_pd = devm_kzalloc(dev, sizeof(*sc_pd), GFP_KERNEL); > > + if (!sc_pd) > > + return ERR_PTR(-ENOMEM); > > + > > + sc_pd->rsrc = pd_ranges->rsrc + idx; > > + sc_pd->pd.power_off = imx_sc_pd_power_off; > > + sc_pd->pd.power_on = imx_sc_pd_power_on; > > So, this means you are going to register one genpd per device (one for each > uart, pwm etc), but there is actually a better option. > > What you seems to be needing is a way to "power on/off" devices, rather than > the PM domain. Right? The PM domain, is managed internally by the FW, no? > Yes, you're right. > In any case, it looks like what you should do is to implement the > ->attach|detach_dev() callback for the genpd and use the regular > of_genpd_add_provider_simple(). From within the ->attach_dev(), you should > get the OF node for the device that is being attached and then parse the > power-domain cell containing the "resource id" and store that in the per device > struct generic_pm_domain_data (we have void pointer there for storing these > kind of things). > > Additionally, you need to implement the ->stop() and ->start() callbacks of > genpd, which is where you "power on/off" devices, rather than using the above > ->power_on|off() callbacks. > > Please have a look a drivers/soc/ti/ti_sci_pm_domains.c. Apologize, I should > have pointed you towards that reference already in the earlier version. > Appreciated for such detailed explanation. lt's a good suggestion and I really like to switch to it. However, after digged a bit more, i found a few blocking issues: 1. The .attach_dev() of power domain infrastructure still does not support multi domains case. It looks like there's no way for .attach_dev() to understand which sub 'power domain' the device is attaching for one global power domain provider like ti_sci as you suggested. Secondly, the struct device *dev passed into is a virtual PD device. It does not help for parsing the real device resource id. So framework probably needs some extension to support multi power domain cases. 2. It also breaks most of current drivers as the driver probe sequence behavior changed If removing .power_on() and .power_off() callback and use .start() and .stop() instead. genpd_dev_pm_attach will only power up the domain and attach device, but will not call .start() which relies on device runtime pm. That means the device power is still not up before running driver probe function. For SCU enabled platforms, all device drivers accessing registers/clock without power domain enabled will trigger a HW access error. That means we need fix most of drivers probe sequence with proper runtime pm. So I'm a bit wondering whether we should still keep the exist way. In summary, we probably may not be able to try ti_sci way before fixing above two issues. Do you think I should wait for them to be fixed or if we could use the current way at first? I might be a little intend to the second way as we now have a lot upstreaming work pending on this. But please let me know if you have a different idea. Regards Dong Aisheng > > + > > + if (pd_ranges->postfix) > > + snprintf(sc_pd->name, sizeof(sc_pd->name), > > + "%s%i", pd_ranges->name, idx); > > + else > > + snprintf(sc_pd->name, sizeof(sc_pd->name), > > + "%s", pd_ranges->name); > > + > > + sc_pd->pd.name = sc_pd->name; > > + > > + if (sc_pd->rsrc >= IMX_SC_R_LAST) { > > + dev_warn(dev, "invalid pd %s rsrc id %d found", > > + sc_pd->name, sc_pd->rsrc); > > + > > + devm_kfree(dev, sc_pd); > > + return NULL; > > + } > > + > > + ret = pm_genpd_init(&sc_pd->pd, NULL, true); > > + if (ret) { > > + dev_warn(dev, "failed to init pd %s rsrc id %d", > > + sc_pd->name, sc_pd->rsrc); > > + devm_kfree(dev, sc_pd); > > + return NULL; > > + } > > + > > + return sc_pd; > > +} > > [...] > > Kind regards > Uffe
On 30 October 2018 at 14:20, A.s. Dong <aisheng.dong@nxp.com> wrote: > [...] > >> } >> > + >> > +static struct imx_sc_pm_domain * >> > +imx_scu_add_pm_domain(struct device *dev, int idx, >> > + const struct imx_sc_pd_range *pd_ranges) { >> > + struct imx_sc_pm_domain *sc_pd; >> > + int ret; >> > + >> > + sc_pd = devm_kzalloc(dev, sizeof(*sc_pd), GFP_KERNEL); >> > + if (!sc_pd) >> > + return ERR_PTR(-ENOMEM); >> > + >> > + sc_pd->rsrc = pd_ranges->rsrc + idx; >> > + sc_pd->pd.power_off = imx_sc_pd_power_off; >> > + sc_pd->pd.power_on = imx_sc_pd_power_on; >> >> So, this means you are going to register one genpd per device (one for each >> uart, pwm etc), but there is actually a better option. >> >> What you seems to be needing is a way to "power on/off" devices, rather than >> the PM domain. Right? The PM domain, is managed internally by the FW, no? >> > > Yes, you're right. > >> In any case, it looks like what you should do is to implement the >> ->attach|detach_dev() callback for the genpd and use the regular >> of_genpd_add_provider_simple(). From within the ->attach_dev(), you should >> get the OF node for the device that is being attached and then parse the >> power-domain cell containing the "resource id" and store that in the per device >> struct generic_pm_domain_data (we have void pointer there for storing these >> kind of things). >> >> Additionally, you need to implement the ->stop() and ->start() callbacks of >> genpd, which is where you "power on/off" devices, rather than using the above >> ->power_on|off() callbacks. >> >> Please have a look a drivers/soc/ti/ti_sci_pm_domains.c. Apologize, I should >> have pointed you towards that reference already in the earlier version. >> > > Appreciated for such detailed explanation. lt's a good suggestion and I really like to > switch to it. However, after digged a bit more, i found a few blocking issues: > 1. The .attach_dev() of power domain infrastructure still does not support multi domains case. > > It looks like there's no way for .attach_dev() to understand which sub 'power domain' > the device is attaching for one global power domain provider like ti_sci as you suggested. > Secondly, the struct device *dev passed into is a virtual PD device. It does not help > for parsing the real device resource id. > > So framework probably needs some extension to support multi power domain cases. Right, you are correct. Let me think about this and see what I can come up with - unless you already have something you want to propose? > > 2. It also breaks most of current drivers as the driver probe sequence behavior changed > If removing .power_on() and .power_off() callback and use .start() and .stop() instead. > > genpd_dev_pm_attach will only power up the domain and attach device, but will not call .start() > which relies on device runtime pm. That means the device power is still not up before > running driver probe function. For SCU enabled platforms, all device drivers accessing registers/clock > without power domain enabled will trigger a HW access error. That means we need fix most > of drivers probe sequence with proper runtime pm. So I'm a bit wondering whether we should > still keep the exist way. I see what you are trying to say, but I am not sure why you consider this as they would break? How did they work in the first place? Anyway, the problem you are talking about have so far been addressed by making buses/drivers to call pm_runtime_enable() and pm_runtime_get_sync() - before trying to probe their devices. Do you have an idea of the amount of drivers that needs to fixed, in regards to this? I guess an option could be to, that via the ->attach_dev() callback perform the same operations as also being done during ->start(). Of course, you may need to keep a flag about this being done, so that operations that need to be balanced with get/put or the like is managed correctly, the next time ->start|stop() becomes called. > > In summary, we probably may not be able to try ti_sci way before fixing above two issues. > > Do you think I should wait for them to be fixed or if we could use the current way at first? > I might be a little intend to the second way as we now have a lot upstreaming work pending on > this. But please let me know if you have a different idea. As this is also about deploying a DTB with a correctly modeled HW, I think we need to fix the above issues. I am happy to help, in one way or the other. [...] Kind regards Uffe
> -----Original Message----- > From: Ulf Hansson [mailto:ulf.hansson@linaro.org] > Sent: Wednesday, October 31, 2018 12:00 AM [...] > On 30 October 2018 at 14:20, A.s. Dong <aisheng.dong@nxp.com> wrote: > > [...] > > > >> } > >> > + > >> > +static struct imx_sc_pm_domain * > >> > +imx_scu_add_pm_domain(struct device *dev, int idx, > >> > + const struct imx_sc_pd_range *pd_ranges) { > >> > + struct imx_sc_pm_domain *sc_pd; > >> > + int ret; > >> > + > >> > + sc_pd = devm_kzalloc(dev, sizeof(*sc_pd), GFP_KERNEL); > >> > + if (!sc_pd) > >> > + return ERR_PTR(-ENOMEM); > >> > + > >> > + sc_pd->rsrc = pd_ranges->rsrc + idx; > >> > + sc_pd->pd.power_off = imx_sc_pd_power_off; > >> > + sc_pd->pd.power_on = imx_sc_pd_power_on; > >> > >> So, this means you are going to register one genpd per device (one > >> for each uart, pwm etc), but there is actually a better option. > >> > >> What you seems to be needing is a way to "power on/off" devices, > >> rather than the PM domain. Right? The PM domain, is managed internally > by the FW, no? > >> > > > > Yes, you're right. > > > >> In any case, it looks like what you should do is to implement the > >> ->attach|detach_dev() callback for the genpd and use the regular > >> of_genpd_add_provider_simple(). From within the ->attach_dev(), you > >> should get the OF node for the device that is being attached and then > >> parse the power-domain cell containing the "resource id" and store > >> that in the per device struct generic_pm_domain_data (we have void > >> pointer there for storing these kind of things). > >> > >> Additionally, you need to implement the ->stop() and ->start() > >> callbacks of genpd, which is where you "power on/off" devices, rather > >> than using the above > >> ->power_on|off() callbacks. > >> > >> Please have a look a drivers/soc/ti/ti_sci_pm_domains.c. Apologize, I > >> should have pointed you towards that reference already in the earlier > version. > >> > > > > Appreciated for such detailed explanation. lt's a good suggestion and > > I really like to switch to it. However, after digged a bit more, i found a few > blocking issues: > > 1. The .attach_dev() of power domain infrastructure still does not support > multi domains case. > > > > It looks like there's no way for .attach_dev() to understand which sub 'power > domain' > > the device is attaching for one global power domain provider like ti_sci as > you suggested. > > Secondly, the struct device *dev passed into is a virtual PD device. > > It does not help for parsing the real device resource id. > > > > So framework probably needs some extension to support multi power > domain cases. > > Right, you are correct. > > Let me think about this and see what I can come up with - unless you already > have something you want to propose? > I have thought we may need add two more params (real dev and pd index) for .attach_dev().detach_dev() and .start()/.stop() for multi PDs case. But I'm not sure if they are proper. > > > > 2. It also breaks most of current drivers as the driver probe sequence > > behavior changed If removing .power_on() and .power_off() callback and > use .start() and .stop() instead. > > > > genpd_dev_pm_attach will only power up the domain and attach device, > > but will not call .start() which relies on device runtime pm. That > > means the device power is still not up before running driver probe > > function. For SCU enabled platforms, all device drivers accessing > > registers/clock without power domain enabled will trigger a HW access > > error. That means we need fix most of drivers probe sequence with proper > runtime pm. So I'm a bit wondering whether we should still keep the exist way. > > I see what you are trying to say, but I am not sure why you consider this as > they would break? How did they work in the first place? > Most drivers does not require power domains before. E.g. GPIO, I2C, PWM, MMC. Or even we need power domain(PUs/PCIE), they're enabled in .power_on()/.power_off() which has been already up before probe. > Anyway, the problem you are talking about have so far been addressed by > making buses/drivers to call pm_runtime_enable() and > pm_runtime_get_sync() - before trying to probe their devices. > Yes, called them early before accessing clock and hw registers. > Do you have an idea of the amount of drivers that needs to fixed, in regards to > this? > So far all enabled devices I tested need fixes. GPIO/I2C/FEC/MMC and etc. Suppose other need the same fix as we have not met this situation before. > I guess an option could be to, that via the ->attach_dev() callback perform the > same operations as also being done during ->start(). Of course, you may need > to keep a flag about this being done, so that operations that need to be > balanced with get/put or the like is managed correctly, the next time > ->start|stop() becomes called. > That might be a way to try. The follow seems may introduce a bit complexities and driver needs to maintainer the runtime status along with framework in order to keep the balance? > > > > In summary, we probably may not be able to try ti_sci way before fixing > above two issues. > > > > Do you think I should wait for them to be fixed or if we could use the current > way at first? > > I might be a little intend to the second way as we now have a lot > > upstreaming work pending on this. But please let me know if you have a > different idea. > > As this is also about deploying a DTB with a correctly modeled HW, I think we > need to fix the above issues. > DTB are the same with #power-domain-cells 1 (resource ID) in either case. So DTB don't need change and driver could be optimized. > I am happy to help, in one way or the other. > Thanks. Regards Dong Aisheng > [...] > > Kind regards > Uffe
On 31 October 2018 at 02:45, A.s. Dong <aisheng.dong@nxp.com> wrote: >> -----Original Message----- >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org] >> Sent: Wednesday, October 31, 2018 12:00 AM > [...] >> On 30 October 2018 at 14:20, A.s. Dong <aisheng.dong@nxp.com> wrote: >> > [...] >> > >> >> } >> >> > + >> >> > +static struct imx_sc_pm_domain * >> >> > +imx_scu_add_pm_domain(struct device *dev, int idx, >> >> > + const struct imx_sc_pd_range *pd_ranges) { >> >> > + struct imx_sc_pm_domain *sc_pd; >> >> > + int ret; >> >> > + >> >> > + sc_pd = devm_kzalloc(dev, sizeof(*sc_pd), GFP_KERNEL); >> >> > + if (!sc_pd) >> >> > + return ERR_PTR(-ENOMEM); >> >> > + >> >> > + sc_pd->rsrc = pd_ranges->rsrc + idx; >> >> > + sc_pd->pd.power_off = imx_sc_pd_power_off; >> >> > + sc_pd->pd.power_on = imx_sc_pd_power_on; >> >> >> >> So, this means you are going to register one genpd per device (one >> >> for each uart, pwm etc), but there is actually a better option. >> >> >> >> What you seems to be needing is a way to "power on/off" devices, >> >> rather than the PM domain. Right? The PM domain, is managed internally >> by the FW, no? >> >> >> > >> > Yes, you're right. >> > >> >> In any case, it looks like what you should do is to implement the >> >> ->attach|detach_dev() callback for the genpd and use the regular >> >> of_genpd_add_provider_simple(). From within the ->attach_dev(), you >> >> should get the OF node for the device that is being attached and then >> >> parse the power-domain cell containing the "resource id" and store >> >> that in the per device struct generic_pm_domain_data (we have void >> >> pointer there for storing these kind of things). >> >> >> >> Additionally, you need to implement the ->stop() and ->start() >> >> callbacks of genpd, which is where you "power on/off" devices, rather >> >> than using the above >> >> ->power_on|off() callbacks. >> >> >> >> Please have a look a drivers/soc/ti/ti_sci_pm_domains.c. Apologize, I >> >> should have pointed you towards that reference already in the earlier >> version. >> >> >> > >> > Appreciated for such detailed explanation. lt's a good suggestion and >> > I really like to switch to it. However, after digged a bit more, i found a few >> blocking issues: >> > 1. The .attach_dev() of power domain infrastructure still does not support >> multi domains case. >> > >> > It looks like there's no way for .attach_dev() to understand which sub 'power >> domain' >> > the device is attaching for one global power domain provider like ti_sci as >> you suggested. >> > Secondly, the struct device *dev passed into is a virtual PD device. >> > It does not help for parsing the real device resource id. >> > >> > So framework probably needs some extension to support multi power >> domain cases. >> >> Right, you are correct. >> >> Let me think about this and see what I can come up with - unless you already >> have something you want to propose? >> > > I have thought we may need add two more params (real dev and pd index) for > .attach_dev().detach_dev() and .start()/.stop() for multi PDs case. > But I'm not sure if they are proper. Right. I don't think we need to change the definition of the callbacks. However, from within the callbacks (or at least the ->attach_dev()) we need access to the data you suggest above. To make that data available, genpd needs to store it in the per device struct generic_pm_domain_data, but of course before invoking the ->attach_dev() callback. > > >> > >> > 2. It also breaks most of current drivers as the driver probe sequence >> > behavior changed If removing .power_on() and .power_off() callback and >> use .start() and .stop() instead. >> > >> > genpd_dev_pm_attach will only power up the domain and attach device, >> > but will not call .start() which relies on device runtime pm. That >> > means the device power is still not up before running driver probe >> > function. For SCU enabled platforms, all device drivers accessing >> > registers/clock without power domain enabled will trigger a HW access >> > error. That means we need fix most of drivers probe sequence with proper >> runtime pm. So I'm a bit wondering whether we should still keep the exist way. >> >> I see what you are trying to say, but I am not sure why you consider this as >> they would break? How did they work in the first place? >> > > Most drivers does not require power domains before. E.g. GPIO, I2C, PWM, MMC. > Or even we need power domain(PUs/PCIE), they're enabled in .power_on()/.power_off() > which has been already up before probe. > >> Anyway, the problem you are talking about have so far been addressed by >> making buses/drivers to call pm_runtime_enable() and >> pm_runtime_get_sync() - before trying to probe their devices. >> > > Yes, called them early before accessing clock and hw registers. > >> Do you have an idea of the amount of drivers that needs to fixed, in regards to >> this? >> > > So far all enabled devices I tested need fixes. GPIO/I2C/FEC/MMC and etc. > Suppose other need the same fix as we have not met this situation before. Okay, I see. > >> I guess an option could be to, that via the ->attach_dev() callback perform the >> same operations as also being done during ->start(). Of course, you may need >> to keep a flag about this being done, so that operations that need to be >> balanced with get/put or the like is managed correctly, the next time >> ->start|stop() becomes called. >> > > That might be a way to try. The follow seems may introduce a bit complexities > and driver needs to maintainer the runtime status along with framework in order to > keep the balance? Yeah, it's likely not going to be very elegant, but should be rather self contained in the PM domain driver and could serve as a way forward - while working on fixing the drivers long term. > >> > >> > In summary, we probably may not be able to try ti_sci way before fixing >> above two issues. >> > >> > Do you think I should wait for them to be fixed or if we could use the current >> way at first? >> > I might be a little intend to the second way as we now have a lot >> > upstreaming work pending on this. But please let me know if you have a >> different idea. >> >> As this is also about deploying a DTB with a correctly modeled HW, I think we >> need to fix the above issues. >> > > DTB are the same with #power-domain-cells 1 (resource ID) in either case. > So DTB don't need change and driver could be optimized. You are correct. Although, how does $subject patch solves the case where a device may have multiple PM domains attached to it? Or is left as a future improvement? Kind regards Uffe
> -----Original Message----- > From: Ulf Hansson [mailto:ulf.hansson@linaro.org] > Sent: Wednesday, October 31, 2018 11:56 PM [...] > On 31 October 2018 at 02:45, A.s. Dong <aisheng.dong@nxp.com> wrote: > >> -----Original Message----- > >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org] > >> Sent: Wednesday, October 31, 2018 12:00 AM > > [...] > >> On 30 October 2018 at 14:20, A.s. Dong <aisheng.dong@nxp.com> wrote: > >> > [...] > >> > > >> >> } > >> >> > + > >> >> > +static struct imx_sc_pm_domain * imx_scu_add_pm_domain(struct > >> >> > +device *dev, int idx, > >> >> > + const struct imx_sc_pd_range *pd_ranges) { > >> >> > + struct imx_sc_pm_domain *sc_pd; > >> >> > + int ret; > >> >> > + > >> >> > + sc_pd = devm_kzalloc(dev, sizeof(*sc_pd), GFP_KERNEL); > >> >> > + if (!sc_pd) > >> >> > + return ERR_PTR(-ENOMEM); > >> >> > + > >> >> > + sc_pd->rsrc = pd_ranges->rsrc + idx; > >> >> > + sc_pd->pd.power_off = imx_sc_pd_power_off; > >> >> > + sc_pd->pd.power_on = imx_sc_pd_power_on; > >> >> > >> >> So, this means you are going to register one genpd per device (one > >> >> for each uart, pwm etc), but there is actually a better option. > >> >> > >> >> What you seems to be needing is a way to "power on/off" devices, > >> >> rather than the PM domain. Right? The PM domain, is managed > >> >> internally > >> by the FW, no? > >> >> > >> > > >> > Yes, you're right. > >> > > >> >> In any case, it looks like what you should do is to implement the > >> >> ->attach|detach_dev() callback for the genpd and use the regular > >> >> of_genpd_add_provider_simple(). From within the ->attach_dev(), > >> >> you should get the OF node for the device that is being attached > >> >> and then parse the power-domain cell containing the "resource id" > >> >> and store that in the per device struct generic_pm_domain_data (we > >> >> have void pointer there for storing these kind of things). > >> >> > >> >> Additionally, you need to implement the ->stop() and ->start() > >> >> callbacks of genpd, which is where you "power on/off" devices, > >> >> rather than using the above > >> >> ->power_on|off() callbacks. > >> >> > >> >> Please have a look a drivers/soc/ti/ti_sci_pm_domains.c. > >> >> Apologize, I should have pointed you towards that reference > >> >> already in the earlier > >> version. > >> >> > >> > > >> > Appreciated for such detailed explanation. lt's a good suggestion > >> > and I really like to switch to it. However, after digged a bit > >> > more, i found a few > >> blocking issues: > >> > 1. The .attach_dev() of power domain infrastructure still does not > >> > support > >> multi domains case. > >> > > >> > It looks like there's no way for .attach_dev() to understand which > >> > sub 'power > >> domain' > >> > the device is attaching for one global power domain provider like > >> > ti_sci as > >> you suggested. > >> > Secondly, the struct device *dev passed into is a virtual PD device. > >> > It does not help for parsing the real device resource id. > >> > > >> > So framework probably needs some extension to support multi power > >> domain cases. > >> > >> Right, you are correct. > >> > >> Let me think about this and see what I can come up with - unless you > >> already have something you want to propose? > >> > > > > I have thought we may need add two more params (real dev and pd index) > > for > > .attach_dev().detach_dev() and .start()/.stop() for multi PDs case. > > But I'm not sure if they are proper. > > Right. > > I don't think we need to change the definition of the callbacks. > However, from within the callbacks (or at least the ->attach_dev()) we need > access to the data you suggest above. > > To make that data available, genpd needs to store it in the per device struct > generic_pm_domain_data, but of course before invoking the > ->attach_dev() callback. > Yes, that's an option. Currently gpd_data is allocated in genpd_add_device() which looks like to be designed to attach device to the specified power domain passed in, so it's unware of whether it's multi domains or not. Then we may still need fine tune those functions in different abstract layers. > > > > > >> > > >> > 2. It also breaks most of current drivers as the driver probe > >> > sequence behavior changed If removing .power_on() and .power_off() > >> > callback and > >> use .start() and .stop() instead. > >> > > >> > genpd_dev_pm_attach will only power up the domain and attach > >> > device, but will not call .start() which relies on device runtime > >> > pm. That means the device power is still not up before running > >> > driver probe function. For SCU enabled platforms, all device > >> > drivers accessing registers/clock without power domain enabled will > >> > trigger a HW access error. That means we need fix most of drivers > >> > probe sequence with proper > >> runtime pm. So I'm a bit wondering whether we should still keep the exist > way. > >> > >> I see what you are trying to say, but I am not sure why you consider > >> this as they would break? How did they work in the first place? > >> > > > > Most drivers does not require power domains before. E.g. GPIO, I2C, PWM, > MMC. > > Or even we need power domain(PUs/PCIE), they're enabled in > > .power_on()/.power_off() which has been already up before probe. > > > >> Anyway, the problem you are talking about have so far been addressed > >> by making buses/drivers to call pm_runtime_enable() and > >> pm_runtime_get_sync() - before trying to probe their devices. > >> > > > > Yes, called them early before accessing clock and hw registers. > > > >> Do you have an idea of the amount of drivers that needs to fixed, in > >> regards to this? > >> > > > > So far all enabled devices I tested need fixes. GPIO/I2C/FEC/MMC and etc. > > Suppose other need the same fix as we have not met this situation before. > > Okay, I see. > > > > >> I guess an option could be to, that via the ->attach_dev() callback > >> perform the same operations as also being done during ->start(). Of > >> course, you may need to keep a flag about this being done, so that > >> operations that need to be balanced with get/put or the like is > >> managed correctly, the next time > >> ->start|stop() becomes called. > >> > > > > That might be a way to try. The follow seems may introduce a bit > > complexities and driver needs to maintainer the runtime status along > > with framework in order to keep the balance? > > Yeah, it's likely not going to be very elegant, but should be rather self > contained in the PM domain driver and could serve as a way forward - while > working on fixing the drivers long term. > > > > >> > > >> > In summary, we probably may not be able to try ti_sci way before > >> > fixing > >> above two issues. > >> > > >> > Do you think I should wait for them to be fixed or if we could use > >> > the current > >> way at first? > >> > I might be a little intend to the second way as we now have a lot > >> > upstreaming work pending on this. But please let me know if you > >> > have a > >> different idea. > >> > >> As this is also about deploying a DTB with a correctly modeled HW, I > >> think we need to fix the above issues. > >> > > > > DTB are the same with #power-domain-cells 1 (resource ID) in either case. > > So DTB don't need change and driver could be optimized. > > You are correct. > > Although, how does $subject patch solves the case where a device may have > multiple PM domains attached to it? Or is left as a future improvement? > IIRC this patch does not have such issue for multi PM domains as we have individual power domains for each resources. Devices will be attached to each individual domains in __genpd_dev_pm_attach rather than the single one global virtual SCU domain. Furthermore we're using domains .power_on()/ power_off() callback which means the power will be up before the probe. (no the issue like when using .attach_dev()/.detach_dev() and run .start()/stop() via runtime pm). Actually I'm a bit wondering that if abstracting SCU domains into one global virtual domain is certainly better than individual domains. For SCU based SoCs, the power domain service is provided by SCU firmware, users don't have to care too much about the HW internals. And according to SCU definition, each devices is associated with a power domain, devices should treat it as a separate domain and have to enable it before accessing HW. So one single global domain looks like not exactly as SCU firmware defines. Not quite sure, but a bit feel like that.... Anyway, give us more time to fine tune it in the future might be a choice IMHO. Thanks Regards Dong Aisheng > Kind regards > Uffe
On 31 October 2018 at 18:24, A.s. Dong <aisheng.dong@nxp.com> wrote: >> -----Original Message----- >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org] >> Sent: Wednesday, October 31, 2018 11:56 PM > [...] >> On 31 October 2018 at 02:45, A.s. Dong <aisheng.dong@nxp.com> wrote: >> >> -----Original Message----- >> >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org] >> >> Sent: Wednesday, October 31, 2018 12:00 AM >> > [...] >> >> On 30 October 2018 at 14:20, A.s. Dong <aisheng.dong@nxp.com> wrote: >> >> > [...] >> >> > >> >> >> } >> >> >> > + >> >> >> > +static struct imx_sc_pm_domain * imx_scu_add_pm_domain(struct >> >> >> > +device *dev, int idx, >> >> >> > + const struct imx_sc_pd_range *pd_ranges) { >> >> >> > + struct imx_sc_pm_domain *sc_pd; >> >> >> > + int ret; >> >> >> > + >> >> >> > + sc_pd = devm_kzalloc(dev, sizeof(*sc_pd), GFP_KERNEL); >> >> >> > + if (!sc_pd) >> >> >> > + return ERR_PTR(-ENOMEM); >> >> >> > + >> >> >> > + sc_pd->rsrc = pd_ranges->rsrc + idx; >> >> >> > + sc_pd->pd.power_off = imx_sc_pd_power_off; >> >> >> > + sc_pd->pd.power_on = imx_sc_pd_power_on; >> >> >> >> >> >> So, this means you are going to register one genpd per device (one >> >> >> for each uart, pwm etc), but there is actually a better option. >> >> >> >> >> >> What you seems to be needing is a way to "power on/off" devices, >> >> >> rather than the PM domain. Right? The PM domain, is managed >> >> >> internally >> >> by the FW, no? >> >> >> >> >> > >> >> > Yes, you're right. >> >> > >> >> >> In any case, it looks like what you should do is to implement the >> >> >> ->attach|detach_dev() callback for the genpd and use the regular >> >> >> of_genpd_add_provider_simple(). From within the ->attach_dev(), >> >> >> you should get the OF node for the device that is being attached >> >> >> and then parse the power-domain cell containing the "resource id" >> >> >> and store that in the per device struct generic_pm_domain_data (we >> >> >> have void pointer there for storing these kind of things). >> >> >> >> >> >> Additionally, you need to implement the ->stop() and ->start() >> >> >> callbacks of genpd, which is where you "power on/off" devices, >> >> >> rather than using the above >> >> >> ->power_on|off() callbacks. >> >> >> >> >> >> Please have a look a drivers/soc/ti/ti_sci_pm_domains.c. >> >> >> Apologize, I should have pointed you towards that reference >> >> >> already in the earlier >> >> version. >> >> >> >> >> > >> >> > Appreciated for such detailed explanation. lt's a good suggestion >> >> > and I really like to switch to it. However, after digged a bit >> >> > more, i found a few >> >> blocking issues: >> >> > 1. The .attach_dev() of power domain infrastructure still does not >> >> > support >> >> multi domains case. >> >> > >> >> > It looks like there's no way for .attach_dev() to understand which >> >> > sub 'power >> >> domain' >> >> > the device is attaching for one global power domain provider like >> >> > ti_sci as >> >> you suggested. >> >> > Secondly, the struct device *dev passed into is a virtual PD device. >> >> > It does not help for parsing the real device resource id. >> >> > >> >> > So framework probably needs some extension to support multi power >> >> domain cases. >> >> >> >> Right, you are correct. >> >> >> >> Let me think about this and see what I can come up with - unless you >> >> already have something you want to propose? >> >> >> > >> > I have thought we may need add two more params (real dev and pd index) >> > for >> > .attach_dev().detach_dev() and .start()/.stop() for multi PDs case. >> > But I'm not sure if they are proper. >> >> Right. >> >> I don't think we need to change the definition of the callbacks. >> However, from within the callbacks (or at least the ->attach_dev()) we need >> access to the data you suggest above. >> >> To make that data available, genpd needs to store it in the per device struct >> generic_pm_domain_data, but of course before invoking the >> ->attach_dev() callback. >> > > Yes, that's an option. > Currently gpd_data is allocated in genpd_add_device() which looks like to be > designed to attach device to the specified power domain passed in, so it's > unware of whether it's multi domains or not. Then we may still need fine > tune those functions in different abstract layers. Yes, something like that. I don't think it should be that hard, I can have a stab at it of you like? > >> > >> > >> >> > >> >> > 2. It also breaks most of current drivers as the driver probe >> >> > sequence behavior changed If removing .power_on() and .power_off() >> >> > callback and >> >> use .start() and .stop() instead. >> >> > >> >> > genpd_dev_pm_attach will only power up the domain and attach >> >> > device, but will not call .start() which relies on device runtime >> >> > pm. That means the device power is still not up before running >> >> > driver probe function. For SCU enabled platforms, all device >> >> > drivers accessing registers/clock without power domain enabled will >> >> > trigger a HW access error. That means we need fix most of drivers >> >> > probe sequence with proper >> >> runtime pm. So I'm a bit wondering whether we should still keep the exist >> way. >> >> >> >> I see what you are trying to say, but I am not sure why you consider >> >> this as they would break? How did they work in the first place? >> >> >> > >> > Most drivers does not require power domains before. E.g. GPIO, I2C, PWM, >> MMC. >> > Or even we need power domain(PUs/PCIE), they're enabled in >> > .power_on()/.power_off() which has been already up before probe. >> > >> >> Anyway, the problem you are talking about have so far been addressed >> >> by making buses/drivers to call pm_runtime_enable() and >> >> pm_runtime_get_sync() - before trying to probe their devices. >> >> >> > >> > Yes, called them early before accessing clock and hw registers. >> > >> >> Do you have an idea of the amount of drivers that needs to fixed, in >> >> regards to this? >> >> >> > >> > So far all enabled devices I tested need fixes. GPIO/I2C/FEC/MMC and etc. >> > Suppose other need the same fix as we have not met this situation before. >> >> Okay, I see. >> >> > >> >> I guess an option could be to, that via the ->attach_dev() callback >> >> perform the same operations as also being done during ->start(). Of >> >> course, you may need to keep a flag about this being done, so that >> >> operations that need to be balanced with get/put or the like is >> >> managed correctly, the next time >> >> ->start|stop() becomes called. >> >> >> > >> > That might be a way to try. The follow seems may introduce a bit >> > complexities and driver needs to maintainer the runtime status along >> > with framework in order to keep the balance? >> >> Yeah, it's likely not going to be very elegant, but should be rather self >> contained in the PM domain driver and could serve as a way forward - while >> working on fixing the drivers long term. >> >> > >> >> > >> >> > In summary, we probably may not be able to try ti_sci way before >> >> > fixing >> >> above two issues. >> >> > >> >> > Do you think I should wait for them to be fixed or if we could use >> >> > the current >> >> way at first? >> >> > I might be a little intend to the second way as we now have a lot >> >> > upstreaming work pending on this. But please let me know if you >> >> > have a >> >> different idea. >> >> >> >> As this is also about deploying a DTB with a correctly modeled HW, I >> >> think we need to fix the above issues. >> >> >> > >> > DTB are the same with #power-domain-cells 1 (resource ID) in either case. >> > So DTB don't need change and driver could be optimized. >> >> You are correct. >> >> Although, how does $subject patch solves the case where a device may have >> multiple PM domains attached to it? Or is left as a future improvement? >> > > IIRC this patch does not have such issue for multi PM domains as we have > individual power domains for each resources. Devices will be attached to > each individual domains in __genpd_dev_pm_attach rather than the single > one global virtual SCU domain. Furthermore we're using domains .power_on()/ > power_off() callback which means the power will be up before the probe. > (no the issue like when using .attach_dev()/.detach_dev() and run .start()/stop() > via runtime pm). I don't follow your reasoning here, sorry. You stated earlier, that there was a need to support multiple PM domains per device. How can that problem be resolved by using one genpd per device? What am I missing? > > Actually I'm a bit wondering that if abstracting SCU domains into one > global virtual domain is certainly better than individual domains. For SCU based SoCs, > the power domain service is provided by SCU firmware, users don't have to care > too much about the HW internals. And according to SCU definition, each devices is > associated with a power domain, devices should treat it as a separate domain > and have to enable it before accessing HW. So one single global domain looks like > not exactly as SCU firmware defines. Not quite sure, but a bit feel like that.... I don't agree with you here, sorry. As also pointed out by Rob earlier, in regards to the DT bindings. Simply put, I doubt the SoC deploys separate power rails/islands for each of the available devices. Instead, it seems like the SCU interface, allows its clients to controls "power" to each of the available *devices* - and not power domains. Anyway, we are debating how to implement this in software and not in DT. :-) That said, if you strongly feel that the short cut, about registering one genpd per device and thus suffer from the overhead it gives, is the best approach to start with, feel free to do that. Kind regards Uffe
> -----Original Message----- > From: Ulf Hansson [mailto:ulf.hansson@linaro.org] > Sent: Thursday, November 1, 2018 5:49 AM [...] > > On 31 October 2018 at 18:24, A.s. Dong <aisheng.dong@nxp.com> wrote: > >> -----Original Message----- > >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org] > >> Sent: Wednesday, October 31, 2018 11:56 PM > > [...] > >> On 31 October 2018 at 02:45, A.s. Dong <aisheng.dong@nxp.com> wrote: > >> >> -----Original Message----- > >> >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org] > >> >> Sent: Wednesday, October 31, 2018 12:00 AM > >> > [...] > >> >> On 30 October 2018 at 14:20, A.s. Dong <aisheng.dong@nxp.com> > wrote: > >> >> > [...] > >> >> > > >> >> >> } > >> >> >> > + > >> >> >> > +static struct imx_sc_pm_domain * > >> >> >> > +imx_scu_add_pm_domain(struct device *dev, int idx, > >> >> >> > + const struct imx_sc_pd_range *pd_ranges) > { > >> >> >> > + struct imx_sc_pm_domain *sc_pd; > >> >> >> > + int ret; > >> >> >> > + > >> >> >> > + sc_pd = devm_kzalloc(dev, sizeof(*sc_pd), GFP_KERNEL); > >> >> >> > + if (!sc_pd) > >> >> >> > + return ERR_PTR(-ENOMEM); > >> >> >> > + > >> >> >> > + sc_pd->rsrc = pd_ranges->rsrc + idx; > >> >> >> > + sc_pd->pd.power_off = imx_sc_pd_power_off; > >> >> >> > + sc_pd->pd.power_on = imx_sc_pd_power_on; > >> >> >> > >> >> >> So, this means you are going to register one genpd per device > >> >> >> (one for each uart, pwm etc), but there is actually a better option. > >> >> >> > >> >> >> What you seems to be needing is a way to "power on/off" > >> >> >> devices, rather than the PM domain. Right? The PM domain, is > >> >> >> managed internally > >> >> by the FW, no? > >> >> >> > >> >> > > >> >> > Yes, you're right. > >> >> > > >> >> >> In any case, it looks like what you should do is to implement > >> >> >> the > >> >> >> ->attach|detach_dev() callback for the genpd and use the > >> >> >> ->attach|regular > >> >> >> of_genpd_add_provider_simple(). From within the ->attach_dev(), > >> >> >> you should get the OF node for the device that is being > >> >> >> attached and then parse the power-domain cell containing the > "resource id" > >> >> >> and store that in the per device struct generic_pm_domain_data > >> >> >> (we have void pointer there for storing these kind of things). > >> >> >> > >> >> >> Additionally, you need to implement the ->stop() and ->start() > >> >> >> callbacks of genpd, which is where you "power on/off" devices, > >> >> >> rather than using the above > >> >> >> ->power_on|off() callbacks. > >> >> >> > >> >> >> Please have a look a drivers/soc/ti/ti_sci_pm_domains.c. > >> >> >> Apologize, I should have pointed you towards that reference > >> >> >> already in the earlier > >> >> version. > >> >> >> > >> >> > > >> >> > Appreciated for such detailed explanation. lt's a good > >> >> > suggestion and I really like to switch to it. However, after > >> >> > digged a bit more, i found a few > >> >> blocking issues: > >> >> > 1. The .attach_dev() of power domain infrastructure still does > >> >> > not support > >> >> multi domains case. > >> >> > > >> >> > It looks like there's no way for .attach_dev() to understand > >> >> > which sub 'power > >> >> domain' > >> >> > the device is attaching for one global power domain provider > >> >> > like ti_sci as > >> >> you suggested. > >> >> > Secondly, the struct device *dev passed into is a virtual PD device. > >> >> > It does not help for parsing the real device resource id. > >> >> > > >> >> > So framework probably needs some extension to support multi > >> >> > power > >> >> domain cases. > >> >> > >> >> Right, you are correct. > >> >> > >> >> Let me think about this and see what I can come up with - unless > >> >> you already have something you want to propose? > >> >> > >> > > >> > I have thought we may need add two more params (real dev and pd > >> > index) for > >> > .attach_dev().detach_dev() and .start()/.stop() for multi PDs case. > >> > But I'm not sure if they are proper. > >> > >> Right. > >> > >> I don't think we need to change the definition of the callbacks. > >> However, from within the callbacks (or at least the ->attach_dev()) > >> we need access to the data you suggest above. > >> > >> To make that data available, genpd needs to store it in the per > >> device struct generic_pm_domain_data, but of course before invoking > >> the > >> ->attach_dev() callback. > >> > > > > Yes, that's an option. > > Currently gpd_data is allocated in genpd_add_device() which looks like > > to be designed to attach device to the specified power domain passed > > in, so it's unware of whether it's multi domains or not. Then we may > > still need fine tune those functions in different abstract layers. > > Yes, something like that. I don't think it should be that hard, I can have a stab > at it of you like? > Yes, that's fine to me. I just don't have too much time to handle it right now. I can help do some basic function test later. May not be stress as we still have no devices using multi domains supported In kernel. So I can construct simple case to test. > > > >> > > >> > > >> >> > > >> >> > 2. It also breaks most of current drivers as the driver probe > >> >> > sequence behavior changed If removing .power_on() and > >> >> > .power_off() callback and > >> >> use .start() and .stop() instead. > >> >> > > >> >> > genpd_dev_pm_attach will only power up the domain and attach > >> >> > device, but will not call .start() which relies on device > >> >> > runtime pm. That means the device power is still not up before > >> >> > running driver probe function. For SCU enabled platforms, all > >> >> > device drivers accessing registers/clock without power domain > >> >> > enabled will trigger a HW access error. That means we need fix > >> >> > most of drivers probe sequence with proper > >> >> runtime pm. So I'm a bit wondering whether we should still keep > >> >> the exist > >> way. > >> >> > >> >> I see what you are trying to say, but I am not sure why you > >> >> consider this as they would break? How did they work in the first place? > >> >> > >> > > >> > Most drivers does not require power domains before. E.g. GPIO, I2C, > >> > PWM, > >> MMC. > >> > Or even we need power domain(PUs/PCIE), they're enabled in > >> > .power_on()/.power_off() which has been already up before probe. > >> > > >> >> Anyway, the problem you are talking about have so far been > >> >> addressed by making buses/drivers to call pm_runtime_enable() and > >> >> pm_runtime_get_sync() - before trying to probe their devices. > >> >> > >> > > >> > Yes, called them early before accessing clock and hw registers. > >> > > >> >> Do you have an idea of the amount of drivers that needs to fixed, > >> >> in regards to this? > >> >> > >> > > >> > So far all enabled devices I tested need fixes. GPIO/I2C/FEC/MMC and etc. > >> > Suppose other need the same fix as we have not met this situation before. > >> > >> Okay, I see. > >> > >> > > >> >> I guess an option could be to, that via the ->attach_dev() > >> >> callback perform the same operations as also being done during > >> >> ->start(). Of course, you may need to keep a flag about this being > >> >> done, so that operations that need to be balanced with get/put or > >> >> the like is managed correctly, the next time > >> >> ->start|stop() becomes called. > >> >> > >> > > >> > That might be a way to try. The follow seems may introduce a bit > >> > complexities and driver needs to maintainer the runtime status > >> > along with framework in order to keep the balance? > >> > >> Yeah, it's likely not going to be very elegant, but should be rather > >> self contained in the PM domain driver and could serve as a way > >> forward - while working on fixing the drivers long term. > >> > >> > > >> >> > > >> >> > In summary, we probably may not be able to try ti_sci way before > >> >> > fixing > >> >> above two issues. > >> >> > > >> >> > Do you think I should wait for them to be fixed or if we could > >> >> > use the current > >> >> way at first? > >> >> > I might be a little intend to the second way as we now have a > >> >> > lot upstreaming work pending on this. But please let me know if > >> >> > you have a > >> >> different idea. > >> >> > >> >> As this is also about deploying a DTB with a correctly modeled HW, > >> >> I think we need to fix the above issues. > >> >> > >> > > >> > DTB are the same with #power-domain-cells 1 (resource ID) in either case. > >> > So DTB don't need change and driver could be optimized. > >> > >> You are correct. > >> > >> Although, how does $subject patch solves the case where a device may > >> have multiple PM domains attached to it? Or is left as a future > improvement? > >> > > > > IIRC this patch does not have such issue for multi PM domains as we > > have individual power domains for each resources. Devices will be > > attached to each individual domains in __genpd_dev_pm_attach rather > > than the single one global virtual SCU domain. Furthermore we're using > > domains .power_on()/ > > power_off() callback which means the power will be up before the probe. > > (no the issue like when using .attach_dev()/.detach_dev() and run > > .start()/stop() via runtime pm). > > I don't follow your reasoning here, sorry. You stated earlier, that there was a > need to support multiple PM domains per device. > > How can that problem be resolved by using one genpd per device? What am I > missing? Sorry I may be not clear before. We still support using multiple power domains. For example: Mipi_csi0 { ... Power_domains = <&PD SC_R_IPI_CH0>, <&PD SC_R_MIPI_CSI0>; Power_domains_names = "ss", "csi"; }; The difference is that each SC resource has individual power domains in driver And __genpd_dev_pm_attach will bind the virtual devices to the correct individual domain and do properly power_on/off of that domains. So it does not have the issue that .attach_dev() has, also does not have issue that power domain is still not up before running probe. (The detailed using is just like Tegra xHCI you pointed to me before) > > > > > Actually I'm a bit wondering that if abstracting SCU domains into one > > global virtual domain is certainly better than individual domains. For > > SCU based SoCs, the power domain service is provided by SCU firmware, > > users don't have to care too much about the HW internals. And > > according to SCU definition, each devices is associated with a power > > domain, devices should treat it as a separate domain and have to > > enable it before accessing HW. So one single global domain looks like not > exactly as SCU firmware defines. Not quite sure, but a bit feel like that.... > > I don't agree with you here, sorry. As also pointed out by Rob earlier, in regards > to the DT bindings. Simply put, I doubt the SoC deploys separate power > rails/islands for each of the available devices. Instead, it seems like the SCU > interface, allows its clients to controls "power" to each of the available > *devices* - and not power domains. > Yes, that's the trick part. For me, I feel like devices are unware of HW internals but using SCU power domains service. So devices think it's seaparte domains may not be strictly wrong. Anyway, I'm also agree with the later way (signal global power domains with .attach_dev()/.start() to power on device). Just because we met those two Issues blocking us to do it right now. > Anyway, we are debating how to implement this in software and not in DT. :-) > > That said, if you strongly feel that the short cut, about registering one genpd > per device and thus suffer from the overhead it gives, is the best approach to > start with, feel free to do that. > I just feel like that starting with current way could give us more time to fine tune it later without blocking other drivers upstreaming. And maybe we can do better work once we have real multi domains users in kernel. Anyway, I can do either, so please me know if you still think we should strictly start with signal global power domain way. Regards Dong Aisheng > Kind regards > Uffe
On 1 November 2018 at 02:28, A.s. Dong <aisheng.dong@nxp.com> wrote: >> -----Original Message----- >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org] >> Sent: Thursday, November 1, 2018 5:49 AM > [...] >> >> On 31 October 2018 at 18:24, A.s. Dong <aisheng.dong@nxp.com> wrote: >> >> -----Original Message----- >> >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org] >> >> Sent: Wednesday, October 31, 2018 11:56 PM >> > [...] >> >> On 31 October 2018 at 02:45, A.s. Dong <aisheng.dong@nxp.com> wrote: >> >> >> -----Original Message----- >> >> >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org] >> >> >> Sent: Wednesday, October 31, 2018 12:00 AM >> >> > [...] >> >> >> On 30 October 2018 at 14:20, A.s. Dong <aisheng.dong@nxp.com> >> wrote: >> >> >> > [...] >> >> >> > >> >> >> >> } >> >> >> >> > + >> >> >> >> > +static struct imx_sc_pm_domain * >> >> >> >> > +imx_scu_add_pm_domain(struct device *dev, int idx, >> >> >> >> > + const struct imx_sc_pd_range *pd_ranges) >> { >> >> >> >> > + struct imx_sc_pm_domain *sc_pd; >> >> >> >> > + int ret; >> >> >> >> > + >> >> >> >> > + sc_pd = devm_kzalloc(dev, sizeof(*sc_pd), GFP_KERNEL); >> >> >> >> > + if (!sc_pd) >> >> >> >> > + return ERR_PTR(-ENOMEM); >> >> >> >> > + >> >> >> >> > + sc_pd->rsrc = pd_ranges->rsrc + idx; >> >> >> >> > + sc_pd->pd.power_off = imx_sc_pd_power_off; >> >> >> >> > + sc_pd->pd.power_on = imx_sc_pd_power_on; >> >> >> >> >> >> >> >> So, this means you are going to register one genpd per device >> >> >> >> (one for each uart, pwm etc), but there is actually a better option. >> >> >> >> >> >> >> >> What you seems to be needing is a way to "power on/off" >> >> >> >> devices, rather than the PM domain. Right? The PM domain, is >> >> >> >> managed internally >> >> >> by the FW, no? >> >> >> >> >> >> >> > >> >> >> > Yes, you're right. >> >> >> > >> >> >> >> In any case, it looks like what you should do is to implement >> >> >> >> the >> >> >> >> ->attach|detach_dev() callback for the genpd and use the >> >> >> >> ->attach|regular >> >> >> >> of_genpd_add_provider_simple(). From within the ->attach_dev(), >> >> >> >> you should get the OF node for the device that is being >> >> >> >> attached and then parse the power-domain cell containing the >> "resource id" >> >> >> >> and store that in the per device struct generic_pm_domain_data >> >> >> >> (we have void pointer there for storing these kind of things). >> >> >> >> >> >> >> >> Additionally, you need to implement the ->stop() and ->start() >> >> >> >> callbacks of genpd, which is where you "power on/off" devices, >> >> >> >> rather than using the above >> >> >> >> ->power_on|off() callbacks. >> >> >> >> >> >> >> >> Please have a look a drivers/soc/ti/ti_sci_pm_domains.c. >> >> >> >> Apologize, I should have pointed you towards that reference >> >> >> >> already in the earlier >> >> >> version. >> >> >> >> >> >> >> > >> >> >> > Appreciated for such detailed explanation. lt's a good >> >> >> > suggestion and I really like to switch to it. However, after >> >> >> > digged a bit more, i found a few >> >> >> blocking issues: >> >> >> > 1. The .attach_dev() of power domain infrastructure still does >> >> >> > not support >> >> >> multi domains case. >> >> >> > >> >> >> > It looks like there's no way for .attach_dev() to understand >> >> >> > which sub 'power >> >> >> domain' >> >> >> > the device is attaching for one global power domain provider >> >> >> > like ti_sci as >> >> >> you suggested. >> >> >> > Secondly, the struct device *dev passed into is a virtual PD device. >> >> >> > It does not help for parsing the real device resource id. >> >> >> > >> >> >> > So framework probably needs some extension to support multi >> >> >> > power >> >> >> domain cases. >> >> >> >> >> >> Right, you are correct. >> >> >> >> >> >> Let me think about this and see what I can come up with - unless >> >> >> you already have something you want to propose? >> >> >> >> >> > >> >> > I have thought we may need add two more params (real dev and pd >> >> > index) for >> >> > .attach_dev().detach_dev() and .start()/.stop() for multi PDs case. >> >> > But I'm not sure if they are proper. >> >> >> >> Right. >> >> >> >> I don't think we need to change the definition of the callbacks. >> >> However, from within the callbacks (or at least the ->attach_dev()) >> >> we need access to the data you suggest above. >> >> >> >> To make that data available, genpd needs to store it in the per >> >> device struct generic_pm_domain_data, but of course before invoking >> >> the >> >> ->attach_dev() callback. >> >> >> > >> > Yes, that's an option. >> > Currently gpd_data is allocated in genpd_add_device() which looks like >> > to be designed to attach device to the specified power domain passed >> > in, so it's unware of whether it's multi domains or not. Then we may >> > still need fine tune those functions in different abstract layers. >> >> Yes, something like that. I don't think it should be that hard, I can have a stab >> at it of you like? >> > > Yes, that's fine to me. I just don't have too much time to handle it right now. > I can help do some basic function test later. May not be stress as we still have > no devices using multi domains supported In kernel. > So I can construct simple case to test. Okay, I am adding it to my TODO list for genpd. > >> > >> >> > >> >> > >> >> >> > >> >> >> > 2. It also breaks most of current drivers as the driver probe >> >> >> > sequence behavior changed If removing .power_on() and >> >> >> > .power_off() callback and >> >> >> use .start() and .stop() instead. >> >> >> > >> >> >> > genpd_dev_pm_attach will only power up the domain and attach >> >> >> > device, but will not call .start() which relies on device >> >> >> > runtime pm. That means the device power is still not up before >> >> >> > running driver probe function. For SCU enabled platforms, all >> >> >> > device drivers accessing registers/clock without power domain >> >> >> > enabled will trigger a HW access error. That means we need fix >> >> >> > most of drivers probe sequence with proper >> >> >> runtime pm. So I'm a bit wondering whether we should still keep >> >> >> the exist >> >> way. >> >> >> >> >> >> I see what you are trying to say, but I am not sure why you >> >> >> consider this as they would break? How did they work in the first place? >> >> >> >> >> > >> >> > Most drivers does not require power domains before. E.g. GPIO, I2C, >> >> > PWM, >> >> MMC. >> >> > Or even we need power domain(PUs/PCIE), they're enabled in >> >> > .power_on()/.power_off() which has been already up before probe. >> >> > >> >> >> Anyway, the problem you are talking about have so far been >> >> >> addressed by making buses/drivers to call pm_runtime_enable() and >> >> >> pm_runtime_get_sync() - before trying to probe their devices. >> >> >> >> >> > >> >> > Yes, called them early before accessing clock and hw registers. >> >> > >> >> >> Do you have an idea of the amount of drivers that needs to fixed, >> >> >> in regards to this? >> >> >> >> >> > >> >> > So far all enabled devices I tested need fixes. GPIO/I2C/FEC/MMC and etc. >> >> > Suppose other need the same fix as we have not met this situation before. >> >> >> >> Okay, I see. >> >> >> >> > >> >> >> I guess an option could be to, that via the ->attach_dev() >> >> >> callback perform the same operations as also being done during >> >> >> ->start(). Of course, you may need to keep a flag about this being >> >> >> done, so that operations that need to be balanced with get/put or >> >> >> the like is managed correctly, the next time >> >> >> ->start|stop() becomes called. >> >> >> >> >> > >> >> > That might be a way to try. The follow seems may introduce a bit >> >> > complexities and driver needs to maintainer the runtime status >> >> > along with framework in order to keep the balance? >> >> >> >> Yeah, it's likely not going to be very elegant, but should be rather >> >> self contained in the PM domain driver and could serve as a way >> >> forward - while working on fixing the drivers long term. >> >> >> >> > >> >> >> > >> >> >> > In summary, we probably may not be able to try ti_sci way before >> >> >> > fixing >> >> >> above two issues. >> >> >> > >> >> >> > Do you think I should wait for them to be fixed or if we could >> >> >> > use the current >> >> >> way at first? >> >> >> > I might be a little intend to the second way as we now have a >> >> >> > lot upstreaming work pending on this. But please let me know if >> >> >> > you have a >> >> >> different idea. >> >> >> >> >> >> As this is also about deploying a DTB with a correctly modeled HW, >> >> >> I think we need to fix the above issues. >> >> >> >> >> > >> >> > DTB are the same with #power-domain-cells 1 (resource ID) in either case. >> >> > So DTB don't need change and driver could be optimized. >> >> >> >> You are correct. >> >> >> >> Although, how does $subject patch solves the case where a device may >> >> have multiple PM domains attached to it? Or is left as a future >> improvement? >> >> >> > >> > IIRC this patch does not have such issue for multi PM domains as we >> > have individual power domains for each resources. Devices will be >> > attached to each individual domains in __genpd_dev_pm_attach rather >> > than the single one global virtual SCU domain. Furthermore we're using >> > domains .power_on()/ >> > power_off() callback which means the power will be up before the probe. >> > (no the issue like when using .attach_dev()/.detach_dev() and run >> > .start()/stop() via runtime pm). >> >> I don't follow your reasoning here, sorry. You stated earlier, that there was a >> need to support multiple PM domains per device. >> >> How can that problem be resolved by using one genpd per device? What am I >> missing? > > Sorry I may be not clear before. > We still support using multiple power domains. > For example: > Mipi_csi0 { > ... > Power_domains = <&PD SC_R_IPI_CH0>, > <&PD SC_R_MIPI_CSI0>; > Power_domains_names = "ss", "csi"; > }; > > The difference is that each SC resource has individual power domains in driver > And __genpd_dev_pm_attach will bind the virtual devices to the correct individual > domain and do properly power_on/off of that domains. > So it does not have the issue that .attach_dev() has, also does not have issue that > power domain is still not up before running probe. > (The detailed using is just like Tegra xHCI you pointed to me before) Aha, I see. Thanks for clarifying. > >> >> > >> > Actually I'm a bit wondering that if abstracting SCU domains into one >> > global virtual domain is certainly better than individual domains. For >> > SCU based SoCs, the power domain service is provided by SCU firmware, >> > users don't have to care too much about the HW internals. And >> > according to SCU definition, each devices is associated with a power >> > domain, devices should treat it as a separate domain and have to >> > enable it before accessing HW. So one single global domain looks like not >> exactly as SCU firmware defines. Not quite sure, but a bit feel like that.... >> >> I don't agree with you here, sorry. As also pointed out by Rob earlier, in regards >> to the DT bindings. Simply put, I doubt the SoC deploys separate power >> rails/islands for each of the available devices. Instead, it seems like the SCU >> interface, allows its clients to controls "power" to each of the available >> *devices* - and not power domains. >> > > Yes, that's the trick part. For me, I feel like devices are unware of HW internals > but using SCU power domains service. So devices think it's seaparte domains > may not be strictly wrong. > Anyway, I'm also agree with the later way (signal global power domains with > .attach_dev()/.start() to power on device). Just because we met those two > Issues blocking us to do it right now. > >> Anyway, we are debating how to implement this in software and not in DT. :-) >> >> That said, if you strongly feel that the short cut, about registering one genpd >> per device and thus suffer from the overhead it gives, is the best approach to >> start with, feel free to do that. >> > > I just feel like that starting with current way could give us more time to fine tune > it later without blocking other drivers upstreaming. And maybe we can do better > work once we have real multi domains users in kernel. > > Anyway, I can do either, so please me know if you still think we should strictly > start with signal global power domain way. As stated, feel free to pick whatever option that makes sense to you. However, if you decide to start with the approach taken in $subject patch, it would be nice if the new drivers/firmware/imx/scu-pd.c file, could have some comments in the top, about what things are needed to convert to the "single global domain", just so we don't forget about it. Kind regards Uffe
> -----Original Message----- > From: Ulf Hansson [mailto:ulf.hansson@linaro.org] > Sent: Thursday, November 1, 2018 5:52 PM [...] > > On 1 November 2018 at 02:28, A.s. Dong <aisheng.dong@nxp.com> wrote: > >> -----Original Message----- > >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org] > >> Sent: Thursday, November 1, 2018 5:49 AM > > [...] > >> > >> On 31 October 2018 at 18:24, A.s. Dong <aisheng.dong@nxp.com> wrote: > >> >> -----Original Message----- > >> >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org] > >> >> Sent: Wednesday, October 31, 2018 11:56 PM > >> > [...] > >> >> On 31 October 2018 at 02:45, A.s. Dong <aisheng.dong@nxp.com> > wrote: > >> >> >> -----Original Message----- > >> >> >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org] > >> >> >> Sent: Wednesday, October 31, 2018 12:00 AM > >> >> > [...] > >> >> >> On 30 October 2018 at 14:20, A.s. Dong <aisheng.dong@nxp.com> > >> wrote: > >> >> >> > [...] > >> >> >> > > >> >> >> >> } > >> >> >> >> > + > >> >> >> >> > +static struct imx_sc_pm_domain * > >> >> >> >> > +imx_scu_add_pm_domain(struct device *dev, int idx, > >> >> >> >> > + const struct imx_sc_pd_range > >> >> >> >> > +*pd_ranges) > >> { > >> >> >> >> > + struct imx_sc_pm_domain *sc_pd; > >> >> >> >> > + int ret; > >> >> >> >> > + > >> >> >> >> > + sc_pd = devm_kzalloc(dev, sizeof(*sc_pd), > GFP_KERNEL); > >> >> >> >> > + if (!sc_pd) > >> >> >> >> > + return ERR_PTR(-ENOMEM); > >> >> >> >> > + > >> >> >> >> > + sc_pd->rsrc = pd_ranges->rsrc + idx; > >> >> >> >> > + sc_pd->pd.power_off = imx_sc_pd_power_off; > >> >> >> >> > + sc_pd->pd.power_on = imx_sc_pd_power_on; > >> >> >> >> > >> >> >> >> So, this means you are going to register one genpd per > >> >> >> >> device (one for each uart, pwm etc), but there is actually a better > option. > >> >> >> >> > >> >> >> >> What you seems to be needing is a way to "power on/off" > >> >> >> >> devices, rather than the PM domain. Right? The PM domain, is > >> >> >> >> managed internally > >> >> >> by the FW, no? > >> >> >> >> > >> >> >> > > >> >> >> > Yes, you're right. > >> >> >> > > >> >> >> >> In any case, it looks like what you should do is to > >> >> >> >> implement the > >> >> >> >> ->attach|detach_dev() callback for the genpd and use the > >> >> >> >> ->attach|regular > >> >> >> >> of_genpd_add_provider_simple(). From within the > >> >> >> >> ->attach_dev(), you should get the OF node for the device > >> >> >> >> that is being attached and then parse the power-domain cell > >> >> >> >> containing the > >> "resource id" > >> >> >> >> and store that in the per device struct > >> >> >> >> generic_pm_domain_data (we have void pointer there for storing > these kind of things). > >> >> >> >> > >> >> >> >> Additionally, you need to implement the ->stop() and > >> >> >> >> ->start() callbacks of genpd, which is where you "power > >> >> >> >> on/off" devices, rather than using the above > >> >> >> >> ->power_on|off() callbacks. > >> >> >> >> > >> >> >> >> Please have a look a drivers/soc/ti/ti_sci_pm_domains.c. > >> >> >> >> Apologize, I should have pointed you towards that reference > >> >> >> >> already in the earlier > >> >> >> version. > >> >> >> >> > >> >> >> > > >> >> >> > Appreciated for such detailed explanation. lt's a good > >> >> >> > suggestion and I really like to switch to it. However, after > >> >> >> > digged a bit more, i found a few > >> >> >> blocking issues: > >> >> >> > 1. The .attach_dev() of power domain infrastructure still > >> >> >> > does not support > >> >> >> multi domains case. > >> >> >> > > >> >> >> > It looks like there's no way for .attach_dev() to understand > >> >> >> > which sub 'power > >> >> >> domain' > >> >> >> > the device is attaching for one global power domain provider > >> >> >> > like ti_sci as > >> >> >> you suggested. > >> >> >> > Secondly, the struct device *dev passed into is a virtual PD device. > >> >> >> > It does not help for parsing the real device resource id. > >> >> >> > > >> >> >> > So framework probably needs some extension to support multi > >> >> >> > power > >> >> >> domain cases. > >> >> >> > >> >> >> Right, you are correct. > >> >> >> > >> >> >> Let me think about this and see what I can come up with - > >> >> >> unless you already have something you want to propose? > >> >> >> > >> >> > > >> >> > I have thought we may need add two more params (real dev and pd > >> >> > index) for > >> >> > .attach_dev().detach_dev() and .start()/.stop() for multi PDs case. > >> >> > But I'm not sure if they are proper. > >> >> > >> >> Right. > >> >> > >> >> I don't think we need to change the definition of the callbacks. > >> >> However, from within the callbacks (or at least the > >> >> ->attach_dev()) we need access to the data you suggest above. > >> >> > >> >> To make that data available, genpd needs to store it in the per > >> >> device struct generic_pm_domain_data, but of course before > >> >> invoking the > >> >> ->attach_dev() callback. > >> >> > >> > > >> > Yes, that's an option. > >> > Currently gpd_data is allocated in genpd_add_device() which looks > >> > like to be designed to attach device to the specified power domain > >> > passed in, so it's unware of whether it's multi domains or not. > >> > Then we may still need fine tune those functions in different abstract > layers. > >> > >> Yes, something like that. I don't think it should be that hard, I can > >> have a stab at it of you like? > >> > > > > Yes, that's fine to me. I just don't have too much time to handle it right now. > > I can help do some basic function test later. May not be stress as we > > still have no devices using multi domains supported In kernel. > > So I can construct simple case to test. > > Okay, I am adding it to my TODO list for genpd. > Great. > > > >> > > >> >> > > >> >> > > >> >> >> > > >> >> >> > 2. It also breaks most of current drivers as the driver probe > >> >> >> > sequence behavior changed If removing .power_on() and > >> >> >> > .power_off() callback and > >> >> >> use .start() and .stop() instead. > >> >> >> > > >> >> >> > genpd_dev_pm_attach will only power up the domain and attach > >> >> >> > device, but will not call .start() which relies on device > >> >> >> > runtime pm. That means the device power is still not up > >> >> >> > before running driver probe function. For SCU enabled > >> >> >> > platforms, all device drivers accessing registers/clock > >> >> >> > without power domain enabled will trigger a HW access error. > >> >> >> > That means we need fix most of drivers probe sequence with > >> >> >> > proper > >> >> >> runtime pm. So I'm a bit wondering whether we should still keep > >> >> >> the exist > >> >> way. > >> >> >> > >> >> >> I see what you are trying to say, but I am not sure why you > >> >> >> consider this as they would break? How did they work in the first > place? > >> >> >> > >> >> > > >> >> > Most drivers does not require power domains before. E.g. GPIO, > >> >> > I2C, PWM, > >> >> MMC. > >> >> > Or even we need power domain(PUs/PCIE), they're enabled in > >> >> > .power_on()/.power_off() which has been already up before probe. > >> >> > > >> >> >> Anyway, the problem you are talking about have so far been > >> >> >> addressed by making buses/drivers to call pm_runtime_enable() > >> >> >> and > >> >> >> pm_runtime_get_sync() - before trying to probe their devices. > >> >> >> > >> >> > > >> >> > Yes, called them early before accessing clock and hw registers. > >> >> > > >> >> >> Do you have an idea of the amount of drivers that needs to > >> >> >> fixed, in regards to this? > >> >> >> > >> >> > > >> >> > So far all enabled devices I tested need fixes. GPIO/I2C/FEC/MMC and > etc. > >> >> > Suppose other need the same fix as we have not met this situation > before. > >> >> > >> >> Okay, I see. > >> >> > >> >> > > >> >> >> I guess an option could be to, that via the ->attach_dev() > >> >> >> callback perform the same operations as also being done during > >> >> >> ->start(). Of course, you may need to keep a flag about this > >> >> >> ->being > >> >> >> done, so that operations that need to be balanced with get/put > >> >> >> or the like is managed correctly, the next time > >> >> >> ->start|stop() becomes called. > >> >> >> > >> >> > > >> >> > That might be a way to try. The follow seems may introduce a bit > >> >> > complexities and driver needs to maintainer the runtime status > >> >> > along with framework in order to keep the balance? > >> >> > >> >> Yeah, it's likely not going to be very elegant, but should be > >> >> rather self contained in the PM domain driver and could serve as a > >> >> way forward - while working on fixing the drivers long term. > >> >> > >> >> > > >> >> >> > > >> >> >> > In summary, we probably may not be able to try ti_sci way > >> >> >> > before fixing > >> >> >> above two issues. > >> >> >> > > >> >> >> > Do you think I should wait for them to be fixed or if we > >> >> >> > could use the current > >> >> >> way at first? > >> >> >> > I might be a little intend to the second way as we now have a > >> >> >> > lot upstreaming work pending on this. But please let me know > >> >> >> > if you have a > >> >> >> different idea. > >> >> >> > >> >> >> As this is also about deploying a DTB with a correctly modeled > >> >> >> HW, I think we need to fix the above issues. > >> >> >> > >> >> > > >> >> > DTB are the same with #power-domain-cells 1 (resource ID) in either > case. > >> >> > So DTB don't need change and driver could be optimized. > >> >> > >> >> You are correct. > >> >> > >> >> Although, how does $subject patch solves the case where a device > >> >> may have multiple PM domains attached to it? Or is left as a > >> >> future > >> improvement? > >> >> > >> > > >> > IIRC this patch does not have such issue for multi PM domains as we > >> > have individual power domains for each resources. Devices will be > >> > attached to each individual domains in __genpd_dev_pm_attach rather > >> > than the single one global virtual SCU domain. Furthermore we're > >> > using domains .power_on()/ > >> > power_off() callback which means the power will be up before the probe. > >> > (no the issue like when using .attach_dev()/.detach_dev() and run > >> > .start()/stop() via runtime pm). > >> > >> I don't follow your reasoning here, sorry. You stated earlier, that > >> there was a need to support multiple PM domains per device. > >> > >> How can that problem be resolved by using one genpd per device? What > >> am I missing? > > > > Sorry I may be not clear before. > > We still support using multiple power domains. > > For example: > > Mipi_csi0 { > > ... > > Power_domains = <&PD SC_R_IPI_CH0>, > > <&PD SC_R_MIPI_CSI0>; > > Power_domains_names = "ss", "csi"; }; > > > > The difference is that each SC resource has individual power domains > > in driver And __genpd_dev_pm_attach will bind the virtual devices to > > the correct individual domain and do properly power_on/off of that > domains. > > So it does not have the issue that .attach_dev() has, also does not > > have issue that power domain is still not up before running probe. > > (The detailed using is just like Tegra xHCI you pointed to me before) > > Aha, I see. Thanks for clarifying. > > > > >> > >> > > >> > Actually I'm a bit wondering that if abstracting SCU domains into > >> > one global virtual domain is certainly better than individual > >> > domains. For SCU based SoCs, the power domain service is provided > >> > by SCU firmware, users don't have to care too much about the HW > >> > internals. And according to SCU definition, each devices is > >> > associated with a power domain, devices should treat it as a > >> > separate domain and have to enable it before accessing HW. So one > >> > single global domain looks like not > >> exactly as SCU firmware defines. Not quite sure, but a bit feel like that.... > >> > >> I don't agree with you here, sorry. As also pointed out by Rob > >> earlier, in regards to the DT bindings. Simply put, I doubt the SoC > >> deploys separate power rails/islands for each of the available > >> devices. Instead, it seems like the SCU interface, allows its clients > >> to controls "power" to each of the available > >> *devices* - and not power domains. > >> > > > > Yes, that's the trick part. For me, I feel like devices are unware of > > HW internals but using SCU power domains service. So devices think > > it's seaparte domains may not be strictly wrong. > > Anyway, I'm also agree with the later way (signal global power domains > > with > > .attach_dev()/.start() to power on device). Just because we met those > > two Issues blocking us to do it right now. > > > >> Anyway, we are debating how to implement this in software and not in > >> DT. :-) > >> > >> That said, if you strongly feel that the short cut, about registering > >> one genpd per device and thus suffer from the overhead it gives, is > >> the best approach to start with, feel free to do that. > >> > > > > I just feel like that starting with current way could give us more > > time to fine tune it later without blocking other drivers upstreaming. > > And maybe we can do better work once we have real multi domains users in > kernel. > > > > Anyway, I can do either, so please me know if you still think we > > should strictly start with signal global power domain way. > > As stated, feel free to pick whatever option that makes sense to you. > > However, if you decide to start with the approach taken in $subject patch, it > would be nice if the new drivers/firmware/imx/scu-pd.c file, could have some > comments in the top, about what things are needed to convert to the "single > global domain", just so we don't forget about it. > Sounds good to me. Thanks for the suggestion. I will do it. Regards Dong Aisheng > Kind regards > Uffe
diff --git a/drivers/firmware/imx/Kconfig b/drivers/firmware/imx/Kconfig index b170c28..6a7a7c2 100644 --- a/drivers/firmware/imx/Kconfig +++ b/drivers/firmware/imx/Kconfig @@ -9,3 +9,9 @@ config IMX_SCU This driver manages the IPC interface between host CPU and the SCU firmware running on M4. + +config IMX_SCU_PD + bool "IMX SCU Power Domain driver" + depends on IMX_SCU + help + The System Controller Firmware (SCFW) based power domain driver. diff --git a/drivers/firmware/imx/Makefile b/drivers/firmware/imx/Makefile index 0ac04df..1b2e15b 100644 --- a/drivers/firmware/imx/Makefile +++ b/drivers/firmware/imx/Makefile @@ -1,2 +1,3 @@ # SPDX-License-Identifier: GPL-2.0 -obj-$(CONFIG_IMX_SCU) += imx-scu.o misc.o +obj-$(CONFIG_IMX_SCU) += imx-scu.o misc.o +obj-$(CONFIG_IMX_SCU_PD) += scu-pd.o diff --git a/drivers/firmware/imx/scu-pd.c b/drivers/firmware/imx/scu-pd.c new file mode 100644 index 0000000..868938d --- /dev/null +++ b/drivers/firmware/imx/scu-pd.c @@ -0,0 +1,302 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (C) 2016 Freescale Semiconductor, Inc. + * Copyright 2017-2018 NXP + * Dong Aisheng <aisheng.dong@nxp.com> + * + * Implementation of the SCU based Power Domains + */ + +#include <dt-bindings/firmware/imx/rsrc.h> +#include <linux/firmware/imx/sci.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/of_platform.h> +#include <linux/platform_device.h> +#include <linux/pm.h> +#include <linux/pm_domain.h> +#include <linux/slab.h> + +/* SCU Power Mode Protocol definition */ +struct imx_sc_msg_req_set_resource_power_mode { + struct imx_sc_rpc_msg hdr; + u16 resource; + u8 mode; +} __packed; + +#define IMX_SCU_PD_NAME_SIZE 20 +struct imx_sc_pm_domain { + struct generic_pm_domain pd; + char name[IMX_SCU_PD_NAME_SIZE]; + u32 rsrc; +}; + +struct imx_sc_pd_range { + char *name; + u32 rsrc; + u8 num; + bool postfix; +}; + +struct imx_sc_pd_soc { + const struct imx_sc_pd_range *pd_ranges; + u8 num_ranges; +}; + +static const struct imx_sc_pd_range imx8qxp_scu_pd_ranges[] = { + /* LSIO SS */ + { "lsio-pwm", IMX_SC_R_PWM_0, 8, 1 }, + { "lsio-gpio", IMX_SC_R_GPIO_0, 8, 1 }, + { "lsio-gpt", IMX_SC_R_GPT_0, 5, 1 }, + { "lsio-kpp", IMX_SC_R_KPP, 1, 0 }, + { "lsio-fspi", IMX_SC_R_FSPI_0, 2, 1 }, + { "lsio-mu", IMX_SC_R_MU_0A, 14, 1 }, + + /* CONN SS */ + { "con-usb", IMX_SC_R_USB_0, 2, 1 }, + { "con-usb0phy", IMX_SC_R_USB_0_PHY, 1, 0 }, + { "con-usb2", IMX_SC_R_USB_2, 1, 0 }, + { "con-usb2phy", IMX_SC_R_USB_2_PHY, 1, 0 }, + { "con-sdhc", IMX_SC_R_SDHC_0, 3, 1 }, + { "con-enet", IMX_SC_R_ENET_0, 2, 1 }, + { "con-nand", IMX_SC_R_NAND, 1, 0 }, + { "con-mlb", IMX_SC_R_MLB_0, 1, 1 }, + + /* Audio DMA SS */ + { "adma-audio-pll0", IMX_SC_R_AUDIO_PLL_0, 1, 0 }, + { "adma-audio-pll1", IMX_SC_R_AUDIO_PLL_1, 1, 0 }, + { "adma-audio-clk-0", IMX_SC_R_AUDIO_CLK_0, 1, 0 }, + { "adma-dma0-ch", IMX_SC_R_DMA_0_CH0, 16, 1 }, + { "adma-dma1-ch", IMX_SC_R_DMA_1_CH0, 16, 1 }, + { "adma-dma2-ch", IMX_SC_R_DMA_2_CH0, 5, 1 }, + { "adma-asrc0", IMX_SC_R_ASRC_0, 1, 0 }, + { "adma-asrc1", IMX_SC_R_ASRC_1, 1, 0 }, + { "adma-esai0", IMX_SC_R_ESAI_0, 1, 0 }, + { "adma-spdif0", IMX_SC_R_SPDIF_0, 1, 0 }, + { "adma-sai", IMX_SC_R_SAI_0, 3, 1 }, + { "adma-amix", IMX_SC_R_AMIX, 1, 0 }, + { "adma-mqs0", IMX_SC_R_MQS_0, 1, 0 }, + { "adma-dsp", IMX_SC_R_DSP, 1, 0 }, + { "adma-dsp-ram", IMX_SC_R_DSP_RAM, 1, 0 }, + { "adma-can", IMX_SC_R_CAN_0, 3, 1 }, + { "adma-ftm", IMX_SC_R_FTM_0, 2, 1 }, + { "adma-lpi2c", IMX_SC_R_I2C_0, 4, 1 }, + { "adma-adc", IMX_SC_R_ADC_0, 1, 1 }, + { "adma-lcd", IMX_SC_R_LCD_0, 1, 1 }, + { "adma-lcd0-pwm", IMX_SC_R_LCD_0_PWM_0, 1, 1 }, + { "adma-lpuart", IMX_SC_R_UART_0, 4, 1 }, + { "adma-lpspi", IMX_SC_R_SPI_0, 4, 1 }, + + /* VPU SS */ + { "vpu", IMX_SC_R_VPU, 1, 0 }, + { "vpu-pid", IMX_SC_R_VPU_PID0, 8, 1 }, + { "vpu-dec0", IMX_SC_R_VPU_DEC_0, 1, 0 }, + { "vpu-enc0", IMX_SC_R_VPU_ENC_0, 1, 0 }, + + /* GPU SS */ + { "gpu0-pid", IMX_SC_R_GPU_0_PID0, 4, 1 }, + + /* HSIO SS */ + { "hsio-pcie-b", IMX_SC_R_PCIE_B, 1, 0 }, + { "hsio-serdes-1", IMX_SC_R_SERDES_1, 1, 0 }, + { "hsio-gpio", IMX_SC_R_HSIO_GPIO, 1, 0 }, + + /* MIPI/LVDS SS */ + { "mipi0", IMX_SC_R_MIPI_0, 1, 0 }, + { "mipi0-pwm0", IMX_SC_R_MIPI_0_PWM_0, 1, 0 }, + { "mipi0-i2c", IMX_SC_R_MIPI_0_I2C_0, 2, 1 }, + { "lvds0", IMX_SC_R_LVDS_0, 1, 0 }, + + /* DC SS */ + { "dc0", IMX_SC_R_DC_0, 1, 0 }, + { "dc0-pll", IMX_SC_R_DC_0_PLL_0, 2, 1 }, +}; + +static const struct imx_sc_pd_soc imx8qxp_scu_pd = { + .pd_ranges = imx8qxp_scu_pd_ranges, + .num_ranges = ARRAY_SIZE(imx8qxp_scu_pd_ranges), +}; + +static struct imx_sc_ipc *pm_ipc_handle; + +static inline struct imx_sc_pm_domain * +to_imx_sc_pd(struct generic_pm_domain *genpd) +{ + return container_of(genpd, struct imx_sc_pm_domain, pd); +} + +static int imx_sc_pd_power(struct generic_pm_domain *domain, bool power_on) +{ + struct imx_sc_msg_req_set_resource_power_mode msg; + struct imx_sc_rpc_msg *hdr = &msg.hdr; + struct imx_sc_pm_domain *pd; + int ret; + + pd = to_imx_sc_pd(domain); + + hdr->ver = IMX_SC_RPC_VERSION; + hdr->svc = IMX_SC_RPC_SVC_PM; + hdr->func = IMX_SC_PM_FUNC_SET_RESOURCE_POWER_MODE; + hdr->size = 2; + + msg.resource = pd->rsrc; + msg.mode = power_on ? IMX_SC_PM_PW_MODE_ON : IMX_SC_PM_PW_MODE_LP; + + ret = imx_scu_call_rpc(pm_ipc_handle, &msg, true); + if (ret) + dev_err(&domain->dev, "failed to power %s resource %d ret %d\n", + power_on ? "up" : "off", pd->rsrc, ret); + + return ret; +} + +static int imx_sc_pd_power_on(struct generic_pm_domain *domain) +{ + return imx_sc_pd_power(domain, true); +} + +static int imx_sc_pd_power_off(struct generic_pm_domain *domain) +{ + return imx_sc_pd_power(domain, false); +} + +static struct generic_pm_domain *imx_scu_pd_xlate(struct of_phandle_args *spec, + void *data) +{ + struct generic_pm_domain *domain = ERR_PTR(-ENOENT); + struct genpd_onecell_data *pd_data = data; + unsigned int i; + + for (i = 0; i < pd_data->num_domains; i++) { + struct imx_sc_pm_domain *sc_pd; + + sc_pd = to_imx_sc_pd(pd_data->domains[i]); + if (sc_pd->rsrc == spec->args[0]) { + domain = &sc_pd->pd; + break; + } + } + + return domain; +} + +static struct imx_sc_pm_domain * +imx_scu_add_pm_domain(struct device *dev, int idx, + const struct imx_sc_pd_range *pd_ranges) +{ + struct imx_sc_pm_domain *sc_pd; + int ret; + + sc_pd = devm_kzalloc(dev, sizeof(*sc_pd), GFP_KERNEL); + if (!sc_pd) + return ERR_PTR(-ENOMEM); + + sc_pd->rsrc = pd_ranges->rsrc + idx; + sc_pd->pd.power_off = imx_sc_pd_power_off; + sc_pd->pd.power_on = imx_sc_pd_power_on; + + if (pd_ranges->postfix) + snprintf(sc_pd->name, sizeof(sc_pd->name), + "%s%i", pd_ranges->name, idx); + else + snprintf(sc_pd->name, sizeof(sc_pd->name), + "%s", pd_ranges->name); + + sc_pd->pd.name = sc_pd->name; + + if (sc_pd->rsrc >= IMX_SC_R_LAST) { + dev_warn(dev, "invalid pd %s rsrc id %d found", + sc_pd->name, sc_pd->rsrc); + + devm_kfree(dev, sc_pd); + return NULL; + } + + ret = pm_genpd_init(&sc_pd->pd, NULL, true); + if (ret) { + dev_warn(dev, "failed to init pd %s rsrc id %d", + sc_pd->name, sc_pd->rsrc); + devm_kfree(dev, sc_pd); + return NULL; + } + + return sc_pd; +} + +static int imx_scu_init_pm_domains(struct device *dev, + const struct imx_sc_pd_soc *pd_soc) +{ + const struct imx_sc_pd_range *pd_ranges = pd_soc->pd_ranges; + struct generic_pm_domain **domains; + struct genpd_onecell_data *pd_data; + struct imx_sc_pm_domain *sc_pd; + u32 count = 0; + int i, j; + + for (i = 0; i < pd_soc->num_ranges; i++) + count += pd_ranges[i].num; + + domains = devm_kcalloc(dev, count, sizeof(*domains), GFP_KERNEL); + if (!domains) + return -ENOMEM; + + pd_data = devm_kzalloc(dev, sizeof(*pd_data), GFP_KERNEL); + if (!pd_data) + return -ENOMEM; + + count = 0; + for (i = 0; i < pd_soc->num_ranges; i++) { + for (j = 0; j < pd_ranges[i].num; j++) { + sc_pd = imx_scu_add_pm_domain(dev, j, &pd_ranges[i]); + if (IS_ERR_OR_NULL(sc_pd)) + continue; + + domains[count++] = &sc_pd->pd; + dev_dbg(dev, "added power domain %s\n", sc_pd->pd.name); + } + } + + pd_data->domains = domains; + pd_data->num_domains = count; + pd_data->xlate = imx_scu_pd_xlate; + + of_genpd_add_provider_onecell(dev->of_node, pd_data); + + return 0; +} + +static int imx_sc_pd_probe(struct platform_device *pdev) +{ + const struct imx_sc_pd_soc *pd_soc; + int ret; + + ret = imx_scu_get_handle(&pm_ipc_handle); + if (ret) + return ret; + + pd_soc = of_device_get_match_data(&pdev->dev); + if (!pd_soc) + return -ENODEV; + + return imx_scu_init_pm_domains(&pdev->dev, pd_soc); +} + +static const struct of_device_id imx_sc_pd_match[] = { + { .compatible = "fsl,imx8qxp-scu-pd", &imx8qxp_scu_pd}, + { /* sentinel */ } +}; + +static struct platform_driver imx_sc_pd_driver = { + .driver = { + .name = "imx-scu-pd", + .of_match_table = imx_sc_pd_match, + }, + .probe = imx_sc_pd_probe, +}; +builtin_platform_driver(imx_sc_pd_driver); + +MODULE_AUTHOR("Dong Aisheng <aisheng.dong@nxp.com>"); +MODULE_DESCRIPTION("IMX SCU Power Domain driver"); +MODULE_LICENSE("GPL v2");
Some i.MX SoCs contain a system controller that is responsible for controlling the state of the IPs that are present. Communication between the host processor running an OS and the system controller happens through a SCU protocol. This patch adds SCU protocol based power domains drivers. Cc: Ulf Hansson <ulf.hansson@linaro.org> Cc: Rob Herring <robh+dt@kernel.org> Cc: Shawn Guo <shawnguo@kernel.org> Cc: Sascha Hauer <kernel@pengutronix.de> Cc: Fabio Estevam <fabio.estevam@nxp.com> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> Cc: Kevin Hilman <khilman@kernel.org> Cc: linux-pm@vger.kernel.org Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> --- ChangeLog: v7->v8: * update to #power-domain-cells 1 binding v6->v7: * still using generic "fsl,scu-pd" to driver binding incase the future SoC is compatible * remove unnecessary cast v5->v6: * only compatible string name updated from fsl,scu-pd to fsl,imx8qxp-scu-pd accordingly v4->v5: * minor improvements according to Sascha's suggestions v3->v4: * update firmware headfile patch from include/soc/imx to include/linux/firmware/imx v2->v3: * name of structures/enums updated with imx_sc prefix v1->v2: * move into drivers/firmware/imx * Implement sc_pm_set_resource_power_mode() API in the driver instead of call it via SCU API according to Sascha's suggestion --- drivers/firmware/imx/Kconfig | 6 + drivers/firmware/imx/Makefile | 3 +- drivers/firmware/imx/scu-pd.c | 302 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 310 insertions(+), 1 deletion(-) create mode 100644 drivers/firmware/imx/scu-pd.c