| Message ID | 1497319166-17287-3-git-send-email-peter.chen@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
[...] > + > +/** > + * of_pwrseq_on - Carry out power sequence on for device node > + * > + * @np: the device node would like to power on > + * > + * Carry out a single device power on. If multiple devices > + * need to be handled, use of_pwrseq_on_list() instead. > + * > + * Return a pointer to the power sequence instance on success, > + * or an error code otherwise. > + */ > +struct pwrseq *of_pwrseq_on(struct device_node *np) > +{ > + struct pwrseq *pwrseq; > + int ret; > + > + pwrseq = pwrseq_find_available_instance(np); > + if (!pwrseq) > + return ERR_PTR(-ENOENT); In case the pwrseq instance hasn't been registered yet, then there is no way to deal with -EPROBE_DEFER properly here. I haven't been following the discussions in-depth during all iterations, so perhaps you have already discussed why doing it like this. Anyway, that means all pwrseq instances needs to be registered an early boot level, to be safe. To me, that seems like poor design choice. [...] Otherwise I think this looks okay to me. Kind regards Uffe
On Tue, Jun 13, 2017 at 12:24:42PM +0200, Ulf Hansson wrote: > [...] > > > + > > +/** > > + * of_pwrseq_on - Carry out power sequence on for device node > > + * > > + * @np: the device node would like to power on > > + * > > + * Carry out a single device power on. If multiple devices > > + * need to be handled, use of_pwrseq_on_list() instead. > > + * > > + * Return a pointer to the power sequence instance on success, > > + * or an error code otherwise. > > + */ > > +struct pwrseq *of_pwrseq_on(struct device_node *np) > > +{ > > + struct pwrseq *pwrseq; > > + int ret; > > + > > + pwrseq = pwrseq_find_available_instance(np); > > + if (!pwrseq) > > + return ERR_PTR(-ENOENT); > > In case the pwrseq instance hasn't been registered yet, then there is > no way to deal with -EPROBE_DEFER properly here. > > I haven't been following the discussions in-depth during all > iterations, so perhaps you have already discussed why doing it like > this. Yes, it has been discussed. In order to compare with compatible string at dts, we need to have one registered pwrseq instance for each pwrseq library, this pre-registered one is allocated using postcore_initcall, and the new (eg, second) instance is registered after pwrseq_get has succeeded. Peter > > Anyway, that means all pwrseq instances needs to be registered an > early boot level, to be safe. To me, that seems like poor design > choice. > > Otherwise I think this looks okay to me. >
On 14 June 2017 at 03:53, Peter Chen <hzpeterchen@gmail.com> wrote: > On Tue, Jun 13, 2017 at 12:24:42PM +0200, Ulf Hansson wrote: >> [...] >> >> > + >> > +/** >> > + * of_pwrseq_on - Carry out power sequence on for device node >> > + * >> > + * @np: the device node would like to power on >> > + * >> > + * Carry out a single device power on. If multiple devices >> > + * need to be handled, use of_pwrseq_on_list() instead. >> > + * >> > + * Return a pointer to the power sequence instance on success, >> > + * or an error code otherwise. >> > + */ >> > +struct pwrseq *of_pwrseq_on(struct device_node *np) >> > +{ >> > + struct pwrseq *pwrseq; >> > + int ret; >> > + >> > + pwrseq = pwrseq_find_available_instance(np); >> > + if (!pwrseq) >> > + return ERR_PTR(-ENOENT); >> >> In case the pwrseq instance hasn't been registered yet, then there is >> no way to deal with -EPROBE_DEFER properly here. >> >> I haven't been following the discussions in-depth during all >> iterations, so perhaps you have already discussed why doing it like >> this. > > Yes, it has been discussed. In order to compare with compatible string > at dts, we need to have one registered pwrseq instance for each > pwrseq library, this pre-registered one is allocated using > postcore_initcall, and the new (eg, second) instance is registered > after pwrseq_get has succeeded. I understand you need one compatible per pwrseq library, but how does that have anything to do with -EPROBE_DEFER? My point is that, if a driver calls of_pwrseq_on() (which calls pwrseq_find_available_instance()), but the corresponding pwrseq library and instance has not yet been registered for that device. Then how will you handle -EPROBE_DEFER? I guess you simply can't, which is why *all* pwrseq libraries needs to be registered in early boot phase, like at postcore_initcall(). Right? If that is the case, I really don't like it. Moreover, I have found yet another severe problem but reviewing the code: In the struct pwrseq, you have a "bool used", which you are setting to "true" once the pwrseq has been hooked up with the device, when a driver calls of_pwrseq_on(). Setting that variable to true, will also prevent another driver from using the same instance of the pwrseq for its device. So, to cope with multiple users, you register a new instance of the same pwrseq library that got hooked up, once the ->get() callback is about to complete. The problem the occurs, when there is another driver calling of_pwrseq_on() in between, meaning that the new instance has not yet been registered. This will simply fail, won't it? Sorry for jumping in late, however to me it seems like there is still some pieces missing to make this work. [...] Kind regards Uffe
On Wed, Jun 14, 2017 at 10:53:29AM +0200, Ulf Hansson wrote: > On 14 June 2017 at 03:53, Peter Chen <hzpeterchen@gmail.com> wrote: > > On Tue, Jun 13, 2017 at 12:24:42PM +0200, Ulf Hansson wrote: > >> [...] > >> > >> > + > >> > +/** > >> > + * of_pwrseq_on - Carry out power sequence on for device node > >> > + * > >> > + * @np: the device node would like to power on > >> > + * > >> > + * Carry out a single device power on. If multiple devices > >> > + * need to be handled, use of_pwrseq_on_list() instead. > >> > + * > >> > + * Return a pointer to the power sequence instance on success, > >> > + * or an error code otherwise. > >> > + */ > >> > +struct pwrseq *of_pwrseq_on(struct device_node *np) > >> > +{ > >> > + struct pwrseq *pwrseq; > >> > + int ret; > >> > + > >> > + pwrseq = pwrseq_find_available_instance(np); > >> > + if (!pwrseq) > >> > + return ERR_PTR(-ENOENT); > >> > >> In case the pwrseq instance hasn't been registered yet, then there is > >> no way to deal with -EPROBE_DEFER properly here. > >> > >> I haven't been following the discussions in-depth during all > >> iterations, so perhaps you have already discussed why doing it like > >> this. > > > > Yes, it has been discussed. In order to compare with compatible string > > at dts, we need to have one registered pwrseq instance for each > > pwrseq library, this pre-registered one is allocated using > > postcore_initcall, and the new (eg, second) instance is registered > > after pwrseq_get has succeeded. > > I understand you need one compatible per pwrseq library, but how does > that have anything to do with -EPROBE_DEFER? > > My point is that, if a driver calls of_pwrseq_on() (which calls > pwrseq_find_available_instance()), but the corresponding pwrseq > library and instance has not yet been registered for that device. Then > how will you handle -EPROBE_DEFER? I guess you simply can't, which is > why *all* pwrseq libraries needs to be registered in early boot phase, > like at postcore_initcall(). Right? > > If that is the case, I really don't like it. > Yes, you are right. This is the limitation for this power sequence library, the registration for the 1st power sequence instance must be finished before device driver uses it. I am appreciated that you can supply some suggestions for it. > Moreover, I have found yet another severe problem but reviewing the code: > In the struct pwrseq, you have a "bool used", which you are setting to > "true" once the pwrseq has been hooked up with the device, when a > driver calls of_pwrseq_on(). Setting that variable to true, will also > prevent another driver from using the same instance of the pwrseq for > its device. So, to cope with multiple users, you register a new > instance of the same pwrseq library that got hooked up, once the > ->get() callback is about to complete. > > The problem the occurs, when there is another driver calling > of_pwrseq_on() in between, meaning that the new instance has not yet > been registered. This will simply fail, won't it? Yes, you are right, thanks for pointing that, I will add mutex_lock for of_pwrseq_on. > > Sorry for jumping in late, however to me it seems like there is still > some pieces missing to make this work. > > [...] > > Kind regards > Uffe
On 15 June 2017 at 08:58, Peter Chen <hzpeterchen@gmail.com> wrote: > On Wed, Jun 14, 2017 at 10:53:29AM +0200, Ulf Hansson wrote: >> On 14 June 2017 at 03:53, Peter Chen <hzpeterchen@gmail.com> wrote: >> > On Tue, Jun 13, 2017 at 12:24:42PM +0200, Ulf Hansson wrote: >> >> [...] >> >> >> >> > + >> >> > +/** >> >> > + * of_pwrseq_on - Carry out power sequence on for device node >> >> > + * >> >> > + * @np: the device node would like to power on >> >> > + * >> >> > + * Carry out a single device power on. If multiple devices >> >> > + * need to be handled, use of_pwrseq_on_list() instead. >> >> > + * >> >> > + * Return a pointer to the power sequence instance on success, >> >> > + * or an error code otherwise. >> >> > + */ >> >> > +struct pwrseq *of_pwrseq_on(struct device_node *np) >> >> > +{ >> >> > + struct pwrseq *pwrseq; >> >> > + int ret; >> >> > + >> >> > + pwrseq = pwrseq_find_available_instance(np); >> >> > + if (!pwrseq) >> >> > + return ERR_PTR(-ENOENT); >> >> >> >> In case the pwrseq instance hasn't been registered yet, then there is >> >> no way to deal with -EPROBE_DEFER properly here. >> >> >> >> I haven't been following the discussions in-depth during all >> >> iterations, so perhaps you have already discussed why doing it like >> >> this. >> > >> > Yes, it has been discussed. In order to compare with compatible string >> > at dts, we need to have one registered pwrseq instance for each >> > pwrseq library, this pre-registered one is allocated using >> > postcore_initcall, and the new (eg, second) instance is registered >> > after pwrseq_get has succeeded. >> >> I understand you need one compatible per pwrseq library, but how does >> that have anything to do with -EPROBE_DEFER? >> >> My point is that, if a driver calls of_pwrseq_on() (which calls >> pwrseq_find_available_instance()), but the corresponding pwrseq >> library and instance has not yet been registered for that device. Then >> how will you handle -EPROBE_DEFER? I guess you simply can't, which is >> why *all* pwrseq libraries needs to be registered in early boot phase, >> like at postcore_initcall(). Right? >> >> If that is the case, I really don't like it. >> > > Yes, you are right. This is the limitation for this power sequence > library, the registration for the 1st power sequence instance must > be finished before device driver uses it. I am appreciated that > you can supply some suggestions for it. In general this kind of problems is solved by first parsing the DTB, which means you will find out whether there is a resource (a pwrseq) required for the device. Then you try to fetch that resource, and if that fails, it means the resource is not yet available, and hence you want to retry later and should return -EPROBE_DEFER. In this case, of_pwrseq_on() needs to be converted to start looking for a pwrseq compatible in it's child node - I guess. Then if that is found, you try to fetch the instance of the corresponding library. Failing to fetch the library instance should then cause a return -EPROBE_DEFER. > >> Moreover, I have found yet another severe problem but reviewing the code: >> In the struct pwrseq, you have a "bool used", which you are setting to >> "true" once the pwrseq has been hooked up with the device, when a >> driver calls of_pwrseq_on(). Setting that variable to true, will also >> prevent another driver from using the same instance of the pwrseq for >> its device. So, to cope with multiple users, you register a new >> instance of the same pwrseq library that got hooked up, once the >> ->get() callback is about to complete. >> >> The problem the occurs, when there is another driver calling >> of_pwrseq_on() in between, meaning that the new instance has not yet >> been registered. This will simply fail, won't it? > > Yes, you are right, thanks for pointing that, I will add mutex_lock for > of_pwrseq_on. Another option is to entirely skip to two step approach. In other words, make the library to cope with multiple users via the same registered library instance. [...] Kind regards Uffe
On Thu, Jun 15, 2017 at 10:11:45AM +0200, Ulf Hansson wrote: > > Yes, you are right. This is the limitation for this power sequence > > library, the registration for the 1st power sequence instance must > > be finished before device driver uses it. I am appreciated that > > you can supply some suggestions for it. > > In general this kind of problems is solved by first parsing the DTB, > which means you will find out whether there is a resource (a pwrseq) > required for the device. Then you try to fetch that resource, and if > that fails, it means the resource is not yet available, and hence you > want to retry later and should return -EPROBE_DEFER. > > In this case, of_pwrseq_on() needs to be converted to start looking > for a pwrseq compatible in it's child node - I guess. Then if that is > found, you try to fetch the instance of the corresponding library. > Failing to fetch the library instance should then cause a return > -EPROBE_DEFER. The most difficulty for this is we can't know whether the requested pwrseq instance will be registered or not, the kernel configuration for this pwrseq library may not be chosen at all. > > > > >> Moreover, I have found yet another severe problem but reviewing the code: > >> In the struct pwrseq, you have a "bool used", which you are setting to > >> "true" once the pwrseq has been hooked up with the device, when a > >> driver calls of_pwrseq_on(). Setting that variable to true, will also > >> prevent another driver from using the same instance of the pwrseq for > >> its device. So, to cope with multiple users, you register a new > >> instance of the same pwrseq library that got hooked up, once the > >> ->get() callback is about to complete. > >> > >> The problem the occurs, when there is another driver calling > >> of_pwrseq_on() in between, meaning that the new instance has not yet > >> been registered. This will simply fail, won't it? > > > > Yes, you are right, thanks for pointing that, I will add mutex_lock for > > of_pwrseq_on. > > Another option is to entirely skip to two step approach. > > In other words, make the library to cope with multiple users via the > same registered library instance. > No, the pwrseq instance stores dtb information (clock, gpio, etc), it needs to be per device.
On 15 June 2017 at 11:11, Peter Chen <hzpeterchen@gmail.com> wrote: > On Thu, Jun 15, 2017 at 10:11:45AM +0200, Ulf Hansson wrote: >> > Yes, you are right. This is the limitation for this power sequence >> > library, the registration for the 1st power sequence instance must >> > be finished before device driver uses it. I am appreciated that >> > you can supply some suggestions for it. >> >> In general this kind of problems is solved by first parsing the DTB, >> which means you will find out whether there is a resource (a pwrseq) >> required for the device. Then you try to fetch that resource, and if >> that fails, it means the resource is not yet available, and hence you >> want to retry later and should return -EPROBE_DEFER. >> >> In this case, of_pwrseq_on() needs to be converted to start looking >> for a pwrseq compatible in it's child node - I guess. Then if that is >> found, you try to fetch the instance of the corresponding library. >> Failing to fetch the library instance should then cause a return >> -EPROBE_DEFER. > > The most difficulty for this is we can't know whether the requested > pwrseq instance will be registered or not, the kernel configuration > for this pwrseq library may not be chosen at all. In such case it is still correct to return -EPROBE_DEFER, because the driver that tries to probe its device will fail unless it can run the needed pwrseq. Right? > >> >> > >> >> Moreover, I have found yet another severe problem but reviewing the code: >> >> In the struct pwrseq, you have a "bool used", which you are setting to >> >> "true" once the pwrseq has been hooked up with the device, when a >> >> driver calls of_pwrseq_on(). Setting that variable to true, will also >> >> prevent another driver from using the same instance of the pwrseq for >> >> its device. So, to cope with multiple users, you register a new >> >> instance of the same pwrseq library that got hooked up, once the >> >> ->get() callback is about to complete. >> >> >> >> The problem the occurs, when there is another driver calling >> >> of_pwrseq_on() in between, meaning that the new instance has not yet >> >> been registered. This will simply fail, won't it? >> > >> > Yes, you are right, thanks for pointing that, I will add mutex_lock for >> > of_pwrseq_on. >> >> Another option is to entirely skip to two step approach. >> >> In other words, make the library to cope with multiple users via the >> same registered library instance. >> > > No, the pwrseq instance stores dtb information (clock, gpio, etc), it > needs to be per device. I think you misunderstood my suggestion here. Of course you need to allocate one pwrseq data per device. However, my point is that you shouldn't need more than one instance of the library functions to be registered in the list of available pwrseq libraries. Kind regards Uffe
On Thu, Jun 15, 2017 at 11:35:20AM +0200, Ulf Hansson wrote: > On 15 June 2017 at 11:11, Peter Chen <hzpeterchen@gmail.com> wrote: > > On Thu, Jun 15, 2017 at 10:11:45AM +0200, Ulf Hansson wrote: > >> > Yes, you are right. This is the limitation for this power sequence > >> > library, the registration for the 1st power sequence instance must > >> > be finished before device driver uses it. I am appreciated that > >> > you can supply some suggestions for it. > >> > >> In general this kind of problems is solved by first parsing the DTB, > >> which means you will find out whether there is a resource (a pwrseq) > >> required for the device. Then you try to fetch that resource, and if > >> that fails, it means the resource is not yet available, and hence you > >> want to retry later and should return -EPROBE_DEFER. > >> > >> In this case, of_pwrseq_on() needs to be converted to start looking > >> for a pwrseq compatible in it's child node - I guess. Then if that is > >> found, you try to fetch the instance of the corresponding library. > >> Failing to fetch the library instance should then cause a return > >> -EPROBE_DEFER. > > > > The most difficulty for this is we can't know whether the requested > > pwrseq instance will be registered or not, the kernel configuration > > for this pwrseq library may not be chosen at all. > > In such case it is still correct to return -EPROBE_DEFER, because the > driver that tries to probe its device will fail unless it can run the > needed pwrseq. Right? > Unlike the MMC design, there is no dts entry to indicate whether this device needs pwrseq or not at this design, it will only carry out power on sequence after matching. So, return -EPROBE_DEFER may not work since this device may never need pwrseq. > > > >> > >> > > >> >> Moreover, I have found yet another severe problem but reviewing the code: > >> >> In the struct pwrseq, you have a "bool used", which you are setting to > >> >> "true" once the pwrseq has been hooked up with the device, when a > >> >> driver calls of_pwrseq_on(). Setting that variable to true, will also > >> >> prevent another driver from using the same instance of the pwrseq for > >> >> its device. So, to cope with multiple users, you register a new > >> >> instance of the same pwrseq library that got hooked up, once the > >> >> ->get() callback is about to complete. > >> >> > >> >> The problem the occurs, when there is another driver calling > >> >> of_pwrseq_on() in between, meaning that the new instance has not yet > >> >> been registered. This will simply fail, won't it? > >> > > >> > Yes, you are right, thanks for pointing that, I will add mutex_lock for > >> > of_pwrseq_on. > >> > >> Another option is to entirely skip to two step approach. > >> > >> In other words, make the library to cope with multiple users via the > >> same registered library instance. > >> > > > > No, the pwrseq instance stores dtb information (clock, gpio, etc), it > > needs to be per device. > > I think you misunderstood my suggestion here. Of course you need to > allocate one pwrseq data per device. > > However, my point is that you shouldn't need more than one instance of > the library functions to be registered in the list of available pwrseq > libraries. > This additional instance is used to store compatible information for this pwrseq library, it is used for the next matching between device and pwrseq library, it just likes we need the first pwrseq instance registered at boot stage.
On Thu, Jun 15, 2017 at 06:06:04PM +0800, Peter Chen wrote: > On Thu, Jun 15, 2017 at 11:35:20AM +0200, Ulf Hansson wrote: > > On 15 June 2017 at 11:11, Peter Chen <hzpeterchen@gmail.com> wrote: > > > On Thu, Jun 15, 2017 at 10:11:45AM +0200, Ulf Hansson wrote: > > >> > Yes, you are right. This is the limitation for this power sequence > > >> > library, the registration for the 1st power sequence instance must > > >> > be finished before device driver uses it. I am appreciated that > > >> > you can supply some suggestions for it. > > >> > > >> In general this kind of problems is solved by first parsing the DTB, > > >> which means you will find out whether there is a resource (a pwrseq) > > >> required for the device. Then you try to fetch that resource, and if > > >> that fails, it means the resource is not yet available, and hence you > > >> want to retry later and should return -EPROBE_DEFER. > > >> > > >> In this case, of_pwrseq_on() needs to be converted to start looking > > >> for a pwrseq compatible in it's child node - I guess. Then if that is > > >> found, you try to fetch the instance of the corresponding library. > > >> Failing to fetch the library instance should then cause a return > > >> -EPROBE_DEFER. > > > > > > The most difficulty for this is we can't know whether the requested > > > pwrseq instance will be registered or not, the kernel configuration > > > for this pwrseq library may not be chosen at all. > > > > In such case it is still correct to return -EPROBE_DEFER, because the > > driver that tries to probe its device will fail unless it can run the > > needed pwrseq. Right? > > > > Unlike the MMC design, there is no dts entry to indicate whether this > device needs pwrseq or not at this design, it will only carry out power > on sequence after matching. So, return -EPROBE_DEFER may not work since > this device may never need pwrseq. > Ulf, since it is the use case limitation, it can't work like device driver. Do you have more comments for it, thanks. Peter > > > > > >> > > >> > > > >> >> Moreover, I have found yet another severe problem but reviewing the code: > > >> >> In the struct pwrseq, you have a "bool used", which you are setting to > > >> >> "true" once the pwrseq has been hooked up with the device, when a > > >> >> driver calls of_pwrseq_on(). Setting that variable to true, will also > > >> >> prevent another driver from using the same instance of the pwrseq for > > >> >> its device. So, to cope with multiple users, you register a new > > >> >> instance of the same pwrseq library that got hooked up, once the > > >> >> ->get() callback is about to complete. > > >> >> > > >> >> The problem the occurs, when there is another driver calling > > >> >> of_pwrseq_on() in between, meaning that the new instance has not yet > > >> >> been registered. This will simply fail, won't it? > > >> > > > >> > Yes, you are right, thanks for pointing that, I will add mutex_lock for > > >> > of_pwrseq_on. > > >> > > >> Another option is to entirely skip to two step approach. > > >> > > >> In other words, make the library to cope with multiple users via the > > >> same registered library instance. > > >> > > > > > > No, the pwrseq instance stores dtb information (clock, gpio, etc), it > > > needs to be per device. > > > > I think you misunderstood my suggestion here. Of course you need to > > allocate one pwrseq data per device. > > > > However, my point is that you shouldn't need more than one instance of > > the library functions to be registered in the list of available pwrseq > > libraries. > > > > This additional instance is used to store compatible information for > this pwrseq library, it is used for the next matching between device > and pwrseq library, it just likes we need the first pwrseq instance > registered at boot stage. > > -- > > Best Regards, > Peter Chen > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 15 June 2017 at 12:06, Peter Chen <hzpeterchen@gmail.com> wrote: > On Thu, Jun 15, 2017 at 11:35:20AM +0200, Ulf Hansson wrote: >> On 15 June 2017 at 11:11, Peter Chen <hzpeterchen@gmail.com> wrote: >> > On Thu, Jun 15, 2017 at 10:11:45AM +0200, Ulf Hansson wrote: >> >> > Yes, you are right. This is the limitation for this power sequence >> >> > library, the registration for the 1st power sequence instance must >> >> > be finished before device driver uses it. I am appreciated that >> >> > you can supply some suggestions for it. >> >> >> >> In general this kind of problems is solved by first parsing the DTB, >> >> which means you will find out whether there is a resource (a pwrseq) >> >> required for the device. Then you try to fetch that resource, and if >> >> that fails, it means the resource is not yet available, and hence you >> >> want to retry later and should return -EPROBE_DEFER. >> >> >> >> In this case, of_pwrseq_on() needs to be converted to start looking >> >> for a pwrseq compatible in it's child node - I guess. Then if that is >> >> found, you try to fetch the instance of the corresponding library. >> >> Failing to fetch the library instance should then cause a return >> >> -EPROBE_DEFER. >> > >> > The most difficulty for this is we can't know whether the requested >> > pwrseq instance will be registered or not, the kernel configuration >> > for this pwrseq library may not be chosen at all. >> >> In such case it is still correct to return -EPROBE_DEFER, because the >> driver that tries to probe its device will fail unless it can run the >> needed pwrseq. Right? >> > > Unlike the MMC design, there is no dts entry to indicate whether this > device needs pwrseq or not at this design, it will only carry out power > on sequence after matching. So, return -EPROBE_DEFER may not work since > this device may never need pwrseq. Then, how will you really be able to fetch the correct pwrseq library instance for the device node? Suppose their is a *list* of pwrseq library instances available. In pwrseq_find_available_instance() you call of_match_node(table, np). The "table" there corresponds to the compatible for the pwrseq library and the np is the device node provided by the caller of of_pwrseq_on(). Why is this match done? Why can't the match be done before trying to fetch a library instance and then in a second step, really try to fetch the instance? If only the second step fails, returning -EPROBE_DEFER can be done, no? BTW, I didn't compatible for the generic pwrseq library being documented in this series. > >> > >> >> >> >> > >> >> >> Moreover, I have found yet another severe problem but reviewing the code: >> >> >> In the struct pwrseq, you have a "bool used", which you are setting to >> >> >> "true" once the pwrseq has been hooked up with the device, when a >> >> >> driver calls of_pwrseq_on(). Setting that variable to true, will also >> >> >> prevent another driver from using the same instance of the pwrseq for >> >> >> its device. So, to cope with multiple users, you register a new >> >> >> instance of the same pwrseq library that got hooked up, once the >> >> >> ->get() callback is about to complete. >> >> >> >> >> >> The problem the occurs, when there is another driver calling >> >> >> of_pwrseq_on() in between, meaning that the new instance has not yet >> >> >> been registered. This will simply fail, won't it? >> >> > >> >> > Yes, you are right, thanks for pointing that, I will add mutex_lock for >> >> > of_pwrseq_on. >> >> >> >> Another option is to entirely skip to two step approach. >> >> >> >> In other words, make the library to cope with multiple users via the >> >> same registered library instance. >> >> >> > >> > No, the pwrseq instance stores dtb information (clock, gpio, etc), it >> > needs to be per device. >> >> I think you misunderstood my suggestion here. Of course you need to >> allocate one pwrseq data per device. >> >> However, my point is that you shouldn't need more than one instance of >> the library functions to be registered in the list of available pwrseq >> libraries. >> > > This additional instance is used to store compatible information for > this pwrseq library, it is used for the next matching between device > and pwrseq library, it just likes we need the first pwrseq instance > registered at boot stage. Why can't the compatible information be a static table, known by the pwrseq core library? Then when of_pwrseq_on() is called, that static table is parsed and matched, then a corresponding pwrseq library instance tries to be fetched. Kind regards Uffe
On Mon, Jun 19, 2017 at 10:09:58AM +0200, Ulf Hansson wrote: > On 15 June 2017 at 12:06, Peter Chen <hzpeterchen@gmail.com> wrote: > > On Thu, Jun 15, 2017 at 11:35:20AM +0200, Ulf Hansson wrote: > >> On 15 June 2017 at 11:11, Peter Chen <hzpeterchen@gmail.com> wrote: > >> > On Thu, Jun 15, 2017 at 10:11:45AM +0200, Ulf Hansson wrote: > >> >> > Yes, you are right. This is the limitation for this power sequence > >> >> > library, the registration for the 1st power sequence instance must > >> >> > be finished before device driver uses it. I am appreciated that > >> >> > you can supply some suggestions for it. > >> >> > >> >> In general this kind of problems is solved by first parsing the DTB, > >> >> which means you will find out whether there is a resource (a pwrseq) > >> >> required for the device. Then you try to fetch that resource, and if > >> >> that fails, it means the resource is not yet available, and hence you > >> >> want to retry later and should return -EPROBE_DEFER. > >> >> > >> >> In this case, of_pwrseq_on() needs to be converted to start looking > >> >> for a pwrseq compatible in it's child node - I guess. Then if that is > >> >> found, you try to fetch the instance of the corresponding library. > >> >> Failing to fetch the library instance should then cause a return > >> >> -EPROBE_DEFER. > >> > > >> > The most difficulty for this is we can't know whether the requested > >> > pwrseq instance will be registered or not, the kernel configuration > >> > for this pwrseq library may not be chosen at all. > >> > >> In such case it is still correct to return -EPROBE_DEFER, because the > >> driver that tries to probe its device will fail unless it can run the > >> needed pwrseq. Right? > >> > > > > Unlike the MMC design, there is no dts entry to indicate whether this > > device needs pwrseq or not at this design, it will only carry out power > > on sequence after matching. So, return -EPROBE_DEFER may not work since > > this device may never need pwrseq. > > Then, how will you really be able to fetch the correct pwrseq library > instance for the device node? > > Suppose their is a *list* of pwrseq library instances available. In > pwrseq_find_available_instance() you call of_match_node(table, np). > The "table" there corresponds to the compatible for the pwrseq library > and the np is the device node provided by the caller of > of_pwrseq_on(). > > Why is this match done? The compatible in table is from the source code, and the compatible in np is from the dts. This is the current match way, I comment your suggestion below. > > Why can't the match be done before trying to fetch a library instance How? If there is no pwrseq instance, how can we do match? > and then in a second step, really try to fetch the instance? If only > the second step fails, returning -EPROBE_DEFER can be done, no? > > BTW, I didn't compatible for the generic pwrseq library being > documented in this series. > > > > >> > > >> >> > >> >> > > >> >> >> Moreover, I have found yet another severe problem but reviewing the code: > >> >> >> In the struct pwrseq, you have a "bool used", which you are setting to > >> >> >> "true" once the pwrseq has been hooked up with the device, when a > >> >> >> driver calls of_pwrseq_on(). Setting that variable to true, will also > >> >> >> prevent another driver from using the same instance of the pwrseq for > >> >> >> its device. So, to cope with multiple users, you register a new > >> >> >> instance of the same pwrseq library that got hooked up, once the > >> >> >> ->get() callback is about to complete. > >> >> >> > >> >> >> The problem the occurs, when there is another driver calling > >> >> >> of_pwrseq_on() in between, meaning that the new instance has not yet > >> >> >> been registered. This will simply fail, won't it? > >> >> > > >> >> > Yes, you are right, thanks for pointing that, I will add mutex_lock for > >> >> > of_pwrseq_on. > >> >> > >> >> Another option is to entirely skip to two step approach. > >> >> > >> >> In other words, make the library to cope with multiple users via the > >> >> same registered library instance. > >> >> > >> > > >> > No, the pwrseq instance stores dtb information (clock, gpio, etc), it > >> > needs to be per device. > >> > >> I think you misunderstood my suggestion here. Of course you need to > >> allocate one pwrseq data per device. > >> > >> However, my point is that you shouldn't need more than one instance of > >> the library functions to be registered in the list of available pwrseq > >> libraries. > >> > > > > This additional instance is used to store compatible information for > > this pwrseq library, it is used for the next matching between device > > and pwrseq library, it just likes we need the first pwrseq instance > > registered at boot stage. > > Why can't the compatible information be a static table, known by the > pwrseq core library? > > Then when of_pwrseq_on() is called, that static table is parsed and > matched, then a corresponding pwrseq library instance tries to be > fetched. > So, you suggest allocating and registering pwrseq instance on the demand? Eg, we maintain a power sequence static table, including compatible and allocate function. static const struct pwrseq_match_table pwrseq_match_table_list[] = { { PWRSEQ_DEV(0x0204, 0x6025), .alloc_instance = pwrseq_AA_alloc_instance }, { PWRSEQ_DEV(0x0204, 0x6026), .alloc_instance = pwrseq_BB_alloc_instance }, { PWRSEQ_DEV(0xffff, 0xffff), .alloc_instance = pwrseq_generic_alloc_instance }, }; And pwrseq_AA{BB}_alloc_instance are defined at each pwrseq library, and are exported. Since the pwrseq_match_table_list is static, we can always do match, and will not return -EPROBE_DEFER anymore, one problem for this is we need always compile all pwrseq libraries. Any good suggestions?
[...] >> > >> > Unlike the MMC design, there is no dts entry to indicate whether this >> > device needs pwrseq or not at this design, it will only carry out power >> > on sequence after matching. So, return -EPROBE_DEFER may not work since >> > this device may never need pwrseq. >> >> Then, how will you really be able to fetch the correct pwrseq library >> instance for the device node? >> >> Suppose their is a *list* of pwrseq library instances available. In >> pwrseq_find_available_instance() you call of_match_node(table, np). >> The "table" there corresponds to the compatible for the pwrseq library >> and the np is the device node provided by the caller of >> of_pwrseq_on(). >> >> Why is this match done? > > The compatible in table is from the source code, and the compatible in > np is from the dts. This is the current match way, I comment your > suggestion below. > >> >> Why can't the match be done before trying to fetch a library instance > > How? If there is no pwrseq instance, how can we do match? > >> and then in a second step, really try to fetch the instance? If only >> the second step fails, returning -EPROBE_DEFER can be done, no? >> >> BTW, I didn't compatible for the generic pwrseq library being >> documented in this series. Seems like you need to update the DT documentation for the below compatible, which is used for the generic pwrseq library. Perhaps this is what puzzles me a bit on *why* the match is done. +static const struct of_device_id generic_id_table[] = { + { .compatible = "generic",}, + { /* sentinel */ } +}; [...] >> > >> > This additional instance is used to store compatible information for >> > this pwrseq library, it is used for the next matching between device >> > and pwrseq library, it just likes we need the first pwrseq instance >> > registered at boot stage. >> >> Why can't the compatible information be a static table, known by the >> pwrseq core library? >> >> Then when of_pwrseq_on() is called, that static table is parsed and >> matched, then a corresponding pwrseq library instance tries to be >> fetched. >> > > So, you suggest allocating and registering pwrseq instance on the > demand? Eg, we maintain a power sequence static table, including > compatible and allocate function. Yes, something like that. > > static const struct pwrseq_match_table pwrseq_match_table_list[] = { > { PWRSEQ_DEV(0x0204, 0x6025), .alloc_instance = pwrseq_AA_alloc_instance }, > { PWRSEQ_DEV(0x0204, 0x6026), .alloc_instance = pwrseq_BB_alloc_instance }, > { PWRSEQ_DEV(0xffff, 0xffff), .alloc_instance = pwrseq_generic_alloc_instance }, What does the PWRSEQ_DEV() macro do? > }; > > And pwrseq_AA{BB}_alloc_instance are defined at each pwrseq library, and > are exported. With "exported", I guess you mean shared via a common pwrseq header? > > Since the pwrseq_match_table_list is static, we can always do match, and > will not return -EPROBE_DEFER anymore, one problem for this is we need > always compile all pwrseq libraries. Any good suggestions? You never returned -EPROBE_DEFER in the first case. That's why I complained. :-) So, in case the OF match doesn't succeed, there are no reason to propagate an error, but instead just bail out and returning 0 to the caller. If the OF match succeeds, it means the device requires a pwrseq library to be used. Then, pwrseq_XX_alloc_instance() will be called, on demand and which tries to fetch the resources (clocks, gpios etc). If any of those attempts fetching a resource fails, its corresponding error code should be propagated to the caller - including -EPROBE_DEFER. Regarding the "always compile all pwrseq libraries"; no we don't need to do that. Instead we only need a to have a stub function for pwrseq_XX_alloc_instance, in case its corresponding Kconfig option is unset. That stub, should of course return an error code. Kind regards Uffe
On Mon, Jun 19, 2017 at 11:48:05AM +0200, Ulf Hansson wrote: > [...] > > >> > > >> > Unlike the MMC design, there is no dts entry to indicate whether this > >> > device needs pwrseq or not at this design, it will only carry out power > >> > on sequence after matching. So, return -EPROBE_DEFER may not work since > >> > this device may never need pwrseq. > >> > >> Then, how will you really be able to fetch the correct pwrseq library > >> instance for the device node? > >> > >> Suppose their is a *list* of pwrseq library instances available. In > >> pwrseq_find_available_instance() you call of_match_node(table, np). > >> The "table" there corresponds to the compatible for the pwrseq library > >> and the np is the device node provided by the caller of > >> of_pwrseq_on(). > >> > >> Why is this match done? > > > > The compatible in table is from the source code, and the compatible in > > np is from the dts. This is the current match way, I comment your > > suggestion below. > > > >> > >> Why can't the match be done before trying to fetch a library instance > > > > How? If there is no pwrseq instance, how can we do match? > > > >> and then in a second step, really try to fetch the instance? If only > >> the second step fails, returning -EPROBE_DEFER can be done, no? > >> > >> BTW, I didn't compatible for the generic pwrseq library being > >> documented in this series. > > Seems like you need to update the DT documentation for the below > compatible, which is used for the generic pwrseq library. Perhaps this > is what puzzles me a bit on *why* the match is done. > > +static const struct of_device_id generic_id_table[] = { > + { .compatible = "generic",}, > + { /* sentinel */ } > +}; Sorry, I should update this "generic" compatible at 1st binding-doc patch. > > [...] > > >> > > >> > This additional instance is used to store compatible information for > >> > this pwrseq library, it is used for the next matching between device > >> > and pwrseq library, it just likes we need the first pwrseq instance > >> > registered at boot stage. > >> > >> Why can't the compatible information be a static table, known by the > >> pwrseq core library? > >> > >> Then when of_pwrseq_on() is called, that static table is parsed and > >> matched, then a corresponding pwrseq library instance tries to be > >> fetched. > >> > > > > So, you suggest allocating and registering pwrseq instance on the > > demand? Eg, we maintain a power sequence static table, including > > compatible and allocate function. > > Yes, something like that. > > > > > static const struct pwrseq_match_table pwrseq_match_table_list[] = { > > { PWRSEQ_DEV(0x0204, 0x6025), .alloc_instance = pwrseq_AA_alloc_instance }, > > { PWRSEQ_DEV(0x0204, 0x6026), .alloc_instance = pwrseq_BB_alloc_instance }, > > { PWRSEQ_DEV(0xffff, 0xffff), .alloc_instance = pwrseq_generic_alloc_instance }, > > What does the PWRSEQ_DEV() macro do? In fact, this should be compatible string, I exampled it as USB vid,pid wrongly. > > Since the pwrseq_match_table_list is static, we can always do match, and > > will not return -EPROBE_DEFER anymore, one problem for this is we need > > always compile all pwrseq libraries. Any good suggestions? > > You never returned -EPROBE_DEFER in the first case. That's why I complained. :-) > > So, in case the OF match doesn't succeed, there are no reason to > propagate an error, but instead just bail out and returning 0 to the > caller. > > If the OF match succeeds, it means the device requires a pwrseq > library to be used. Then, pwrseq_XX_alloc_instance() will be called, > on demand and which tries to fetch the resources (clocks, gpios etc). > If any of those attempts fetching a resource fails, its corresponding > error code should be propagated to the caller - including > -EPROBE_DEFER. > > Regarding the "always compile all pwrseq libraries"; no we don't need > to do that. Instead we only need a to have a stub function for > pwrseq_XX_alloc_instance, in case its corresponding Kconfig option is > unset. That stub, should of course return an error code. > I will have a updated version for your suggestion, thanks.
diff --git a/Documentation/power/power-sequence/design.rst b/Documentation/power/power-sequence/design.rst new file mode 100644 index 0000000..554608e --- /dev/null +++ b/Documentation/power/power-sequence/design.rst @@ -0,0 +1,54 @@ +==================================== +Power Sequence Library +==================================== + +:Date: Feb, 2017 +:Author: Peter Chen <peter.chen@nxp.com> + + +Introduction +============ + +We have an well-known problem that the device needs to do a power +sequence before it can be recognized by related host, the typical +examples are hard-wired mmc devices and usb devices. The host controller +can't know what kinds of this device is in its bus if the power +sequence has not done, since the related devices driver's probe calling +is determined by runtime according to eunumeration results. Besides, +the devices may have custom power sequence, so the power sequence library +which is independent with the devices is needed. + +Design +============ + +The power sequence library includes the core file and customer power +sequence library. The core file exports interfaces are called by +host controller driver for power sequence and customer power sequence +library files to register its power sequence instance to global +power sequence list. The custom power sequence library creates power +sequence instance and implement custom power sequence. + +Since the power sequence describes hardware design, the description is +located at board description file, eg, device tree dts file. And +a specific power sequence belongs to device, so its description +is under the device node, please refer to: +Documentation/devicetree/bindings/power/pwrseq/pwrseq-generic.txt + +Custom power sequence library allocates one power sequence instance at +bootup periods using postcore_initcall, this static allocated instance is +used to compare with device-tree (DT) node to see if this library can be +used for the node or not. When the result is matched, the core API will +try to get resourses (->get, implemented at each library) for power +sequence, if all resources are got, it will try to allocate another +instance for next possible request from host driver. + +Then, the host controller driver can carry out power sequence on for this +DT node, the library will do corresponding operations, like open clocks, +toggle gpio, etc. The power sequence off routine will close and free the +resources, and is called when the parent is removed. And the power +sequence suspend and resume routine can be called at host driver's +suspend and resume routine if needed. + +The exported interfaces +.. kernel-doc:: drivers/power/pwrseq/core.c + :export: diff --git a/MAINTAINERS b/MAINTAINERS index f7d568b..93b07aa 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -10188,6 +10188,15 @@ F: include/linux/pm_* F: include/linux/powercap.h F: drivers/powercap/ +POWER SEQUENCE LIBRARY +M: Peter Chen <Peter.Chen@nxp.com> +T: git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git +L: linux-pm@vger.kernel.org +S: Maintained +F: Documentation/devicetree/bindings/power/pwrseq/ +F: drivers/power/pwrseq/ +F: include/linux/power/pwrseq.h + POWER SUPPLY CLASS/SUBSYSTEM and DRIVERS M: Sebastian Reichel <sre@kernel.org> L: linux-pm@vger.kernel.org diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig index 63454b5..c1bb046 100644 --- a/drivers/power/Kconfig +++ b/drivers/power/Kconfig @@ -1,3 +1,4 @@ source "drivers/power/avs/Kconfig" source "drivers/power/reset/Kconfig" source "drivers/power/supply/Kconfig" +source "drivers/power/pwrseq/Kconfig" diff --git a/drivers/power/Makefile b/drivers/power/Makefile index ff35c71..7db8035 100644 --- a/drivers/power/Makefile +++ b/drivers/power/Makefile @@ -1,3 +1,4 @@ obj-$(CONFIG_POWER_AVS) += avs/ obj-$(CONFIG_POWER_RESET) += reset/ obj-$(CONFIG_POWER_SUPPLY) += supply/ +obj-$(CONFIG_POWER_SEQUENCE) += pwrseq/ diff --git a/drivers/power/pwrseq/Kconfig b/drivers/power/pwrseq/Kconfig new file mode 100644 index 0000000..c6b3569 --- /dev/null +++ b/drivers/power/pwrseq/Kconfig @@ -0,0 +1,20 @@ +# +# Power Sequence library +# + +menuconfig POWER_SEQUENCE + bool "Power sequence control" + help + It is used for drivers which needs to do power sequence + (eg, turn on clock, toggle reset gpio) before the related + devices can be found by hardware, eg, USB bus. + +if POWER_SEQUENCE + +config PWRSEQ_GENERIC + bool "Generic power sequence control" + depends on OF + help + This is the generic power sequence control library, and is + supposed to support common power sequence usage. +endif diff --git a/drivers/power/pwrseq/Makefile b/drivers/power/pwrseq/Makefile new file mode 100644 index 0000000..ad82389 --- /dev/null +++ b/drivers/power/pwrseq/Makefile @@ -0,0 +1,2 @@ +obj-$(CONFIG_POWER_SEQUENCE) += core.o +obj-$(CONFIG_PWRSEQ_GENERIC) += pwrseq_generic.o diff --git a/drivers/power/pwrseq/core.c b/drivers/power/pwrseq/core.c new file mode 100644 index 0000000..3d19e62 --- /dev/null +++ b/drivers/power/pwrseq/core.c @@ -0,0 +1,335 @@ +/* + * core.c power sequence core file + * + * Copyright (C) 2016 Freescale Semiconductor, Inc. + * Author: Peter Chen <peter.chen@nxp.com> + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 of + * the License as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. + */ + +#include <linux/list.h> +#include <linux/mutex.h> +#include <linux/of.h> +#include <linux/slab.h> +#include <linux/power/pwrseq.h> + +static DEFINE_MUTEX(pwrseq_list_mutex); +static LIST_HEAD(pwrseq_list); + +static int pwrseq_get(struct device_node *np, struct pwrseq *p) +{ + if (p && p->get) + return p->get(np, p); + + return -ENOTSUPP; +} + +static int pwrseq_on(struct pwrseq *p) +{ + if (p && p->on) + return p->on(p); + + return -ENOTSUPP; +} + +static void pwrseq_off(struct pwrseq *p) +{ + if (p && p->off) + p->off(p); +} + +static void pwrseq_put(struct pwrseq *p) +{ + if (p && p->put) + p->put(p); +} + +/** + * pwrseq_register - Add pwrseq instance to global pwrseq list + * + * @pwrseq: the pwrseq instance + */ +void pwrseq_register(struct pwrseq *pwrseq) +{ + mutex_lock(&pwrseq_list_mutex); + list_add(&pwrseq->node, &pwrseq_list); + mutex_unlock(&pwrseq_list_mutex); +} +EXPORT_SYMBOL_GPL(pwrseq_register); + +/** + * pwrseq_unregister - Remove pwrseq instance from global pwrseq list + * + * @pwrseq: the pwrseq instance + */ +void pwrseq_unregister(struct pwrseq *pwrseq) +{ + mutex_lock(&pwrseq_list_mutex); + list_del(&pwrseq->node); + mutex_unlock(&pwrseq_list_mutex); +} +EXPORT_SYMBOL_GPL(pwrseq_unregister); + +static struct pwrseq *pwrseq_find_available_instance(struct device_node *np) +{ + struct pwrseq *pwrseq; + + mutex_lock(&pwrseq_list_mutex); + list_for_each_entry(pwrseq, &pwrseq_list, node) { + if (pwrseq->used) + continue; + + /* compare compatible string for pwrseq node */ + if (of_match_node(pwrseq->pwrseq_of_match_table, np)) { + pwrseq->used = true; + mutex_unlock(&pwrseq_list_mutex); + return pwrseq; + } + + /* return generic pwrseq instance */ + if (!strcmp(pwrseq->pwrseq_of_match_table->compatible, + "generic")) { + pr_debug("using generic pwrseq instance for %s\n", + np->full_name); + pwrseq->used = true; + mutex_unlock(&pwrseq_list_mutex); + return pwrseq; + } + } + mutex_unlock(&pwrseq_list_mutex); + pr_debug("Can't find any pwrseq instances for %s\n", np->full_name); + + return NULL; +} + +/** + * of_pwrseq_on - Carry out power sequence on for device node + * + * @np: the device node would like to power on + * + * Carry out a single device power on. If multiple devices + * need to be handled, use of_pwrseq_on_list() instead. + * + * Return a pointer to the power sequence instance on success, + * or an error code otherwise. + */ +struct pwrseq *of_pwrseq_on(struct device_node *np) +{ + struct pwrseq *pwrseq; + int ret; + + pwrseq = pwrseq_find_available_instance(np); + if (!pwrseq) + return ERR_PTR(-ENOENT); + + ret = pwrseq_get(np, pwrseq); + if (ret) { + /* Mark current pwrseq as unused */ + pwrseq->used = false; + return ERR_PTR(ret); + } + + ret = pwrseq_on(pwrseq); + if (ret) + goto pwr_put; + + return pwrseq; + +pwr_put: + pwrseq_put(pwrseq); + return ERR_PTR(ret); +} +EXPORT_SYMBOL_GPL(of_pwrseq_on); + +/** + * of_pwrseq_off - Carry out power sequence off for this pwrseq instance + * + * @pwrseq: the pwrseq instance which related device would like to be off + * + * This API is used to power off single device, it is the opposite + * operation for of_pwrseq_on. + */ +void of_pwrseq_off(struct pwrseq *pwrseq) +{ + pwrseq_off(pwrseq); + pwrseq_put(pwrseq); +} +EXPORT_SYMBOL_GPL(of_pwrseq_off); + +/** + * of_pwrseq_on_list - Carry out power sequence on for list + * + * @np: the device node would like to power on + * @head: the list head for pwrseq list on this bus + * + * This API is used to power on multiple devices at single bus. + * If there are several devices on bus (eg, USB bus), uses this + * this API. Otherwise, use of_pwrseq_on instead. After the device + * is powered on successfully, it will be added to pwrseq list for + * this bus. The caller needs to use mutex_lock for concurrent. + * + * Return 0 on success, or an error value otherwise. + */ +int of_pwrseq_on_list(struct device_node *np, struct list_head *head) +{ + struct pwrseq *pwrseq; + struct pwrseq_list_per_dev *pwrseq_list_node; + + pwrseq_list_node = kzalloc(sizeof(*pwrseq_list_node), GFP_KERNEL); + if (!pwrseq_list_node) + return -ENOMEM; + + pwrseq = of_pwrseq_on(np); + if (IS_ERR(pwrseq)) { + kfree(pwrseq_list_node); + return PTR_ERR(pwrseq); + } + + pwrseq_list_node->pwrseq = pwrseq; + list_add(&pwrseq_list_node->list, head); + + return 0; +} +EXPORT_SYMBOL_GPL(of_pwrseq_on_list); + +/** + * of_pwrseq_off_list - Carry out power sequence off for the list + * + * @head: the list head for pwrseq instance list on this bus + * + * This API is used to power off all devices on this bus, it is + * the opposite operation for of_pwrseq_on_list. + * The caller needs to use mutex_lock for concurrent. + */ +void of_pwrseq_off_list(struct list_head *head) +{ + struct pwrseq *pwrseq; + struct pwrseq_list_per_dev *pwrseq_list_node, *tmp_node; + + list_for_each_entry_safe(pwrseq_list_node, tmp_node, head, list) { + pwrseq = pwrseq_list_node->pwrseq; + of_pwrseq_off(pwrseq); + list_del(&pwrseq_list_node->list); + kfree(pwrseq_list_node); + } +} +EXPORT_SYMBOL_GPL(of_pwrseq_off_list); + +/** + * pwrseq_suspend - Carry out power sequence suspend for this pwrseq instance + * + * @pwrseq: the pwrseq instance + * + * This API is used to do suspend operation on pwrseq instance. + * + * Return 0 on success, or an error value otherwise. + */ +int pwrseq_suspend(struct pwrseq *p) +{ + int ret = 0; + + if (p && p->suspend) + ret = p->suspend(p); + else + return ret; + + if (!ret) + p->suspended = true; + else + pr_err("%s failed\n", __func__); + + return ret; +} +EXPORT_SYMBOL_GPL(pwrseq_suspend); + +/** + * pwrseq_resume - Carry out power sequence resume for this pwrseq instance + * + * @pwrseq: the pwrseq instance + * + * This API is used to do resume operation on pwrseq instance. + * + * Return 0 on success, or an error value otherwise. + */ +int pwrseq_resume(struct pwrseq *p) +{ + int ret = 0; + + if (p && p->resume) + ret = p->resume(p); + else + return ret; + + if (!ret) + p->suspended = false; + else + pr_err("%s failed\n", __func__); + + return ret; +} +EXPORT_SYMBOL_GPL(pwrseq_resume); + +/** + * pwrseq_suspend_list - Carry out power sequence suspend for list + * + * @head: the list head for pwrseq instance list on this bus + * + * This API is used to do suspend on all power sequence instances on this bus. + * The caller needs to use mutex_lock for concurrent. + */ +int pwrseq_suspend_list(struct list_head *head) +{ + struct pwrseq *pwrseq; + struct pwrseq_list_per_dev *pwrseq_list_node; + int ret = 0; + + list_for_each_entry(pwrseq_list_node, head, list) { + ret = pwrseq_suspend(pwrseq_list_node->pwrseq); + if (ret) + break; + } + + if (ret) { + list_for_each_entry(pwrseq_list_node, head, list) { + pwrseq = pwrseq_list_node->pwrseq; + if (pwrseq->suspended) + pwrseq_resume(pwrseq); + } + } + + return ret; +} +EXPORT_SYMBOL_GPL(pwrseq_suspend_list); + +/** + * pwrseq_resume_list - Carry out power sequence resume for the list + * + * @head: the list head for pwrseq instance list on this bus + * + * This API is used to do resume on all power sequence instances on this bus. + * The caller needs to use mutex_lock for concurrent. + */ +int pwrseq_resume_list(struct list_head *head) +{ + struct pwrseq_list_per_dev *pwrseq_list_node; + int ret = 0; + + list_for_each_entry(pwrseq_list_node, head, list) { + ret = pwrseq_resume(pwrseq_list_node->pwrseq); + if (ret) + break; + } + + return ret; +} +EXPORT_SYMBOL_GPL(pwrseq_resume_list); diff --git a/drivers/power/pwrseq/pwrseq_generic.c b/drivers/power/pwrseq/pwrseq_generic.c new file mode 100644 index 0000000..4e7c090 --- /dev/null +++ b/drivers/power/pwrseq/pwrseq_generic.c @@ -0,0 +1,234 @@ +/* + * pwrseq_generic.c Generic power sequence handling + * + * Copyright (C) 2016 Freescale Semiconductor, Inc. + * Author: Peter Chen <peter.chen@nxp.com> + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 of + * the License as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. + */ + +#include <linux/clk.h> +#include <linux/delay.h> +#include <linux/gpio.h> +#include <linux/gpio/consumer.h> +#include <linux/of.h> +#include <linux/of_gpio.h> +#include <linux/slab.h> + +#include <linux/power/pwrseq.h> + +struct pwrseq_generic { + struct pwrseq pwrseq; + struct gpio_desc *gpiod_reset; + struct clk *clks[PWRSEQ_MAX_CLKS]; + u32 duration_us; + bool suspended; +}; + +#define to_generic_pwrseq(p) container_of(p, struct pwrseq_generic, pwrseq) + +static int pwrseq_generic_alloc_instance(void); +static const struct of_device_id generic_id_table[] = { + { .compatible = "generic",}, + { /* sentinel */ } +}; + +static int pwrseq_generic_suspend(struct pwrseq *pwrseq) +{ + struct pwrseq_generic *pwrseq_gen = to_generic_pwrseq(pwrseq); + int clk; + + for (clk = PWRSEQ_MAX_CLKS - 1; clk >= 0; clk--) + clk_disable_unprepare(pwrseq_gen->clks[clk]); + + pwrseq_gen->suspended = true; + return 0; +} + +static int pwrseq_generic_resume(struct pwrseq *pwrseq) +{ + struct pwrseq_generic *pwrseq_gen = to_generic_pwrseq(pwrseq); + int clk, ret = 0; + + for (clk = 0; clk < PWRSEQ_MAX_CLKS && pwrseq_gen->clks[clk]; clk++) { + ret = clk_prepare_enable(pwrseq_gen->clks[clk]); + if (ret) { + pr_err("Can't enable clock, ret=%d\n", ret); + goto err_disable_clks; + } + } + + pwrseq_gen->suspended = false; + return ret; + +err_disable_clks: + while (--clk >= 0) + clk_disable_unprepare(pwrseq_gen->clks[clk]); + + return ret; +} + +static void pwrseq_generic_put(struct pwrseq *pwrseq) +{ + struct pwrseq_generic *pwrseq_gen = to_generic_pwrseq(pwrseq); + int clk; + + if (pwrseq_gen->gpiod_reset) + gpiod_put(pwrseq_gen->gpiod_reset); + + for (clk = 0; clk < PWRSEQ_MAX_CLKS; clk++) + clk_put(pwrseq_gen->clks[clk]); + + pwrseq_unregister(&pwrseq_gen->pwrseq); + kfree(pwrseq_gen); +} + +static void pwrseq_generic_off(struct pwrseq *pwrseq) +{ + struct pwrseq_generic *pwrseq_gen = to_generic_pwrseq(pwrseq); + int clk; + + if (pwrseq_gen->suspended) + return; + + for (clk = PWRSEQ_MAX_CLKS - 1; clk >= 0; clk--) + clk_disable_unprepare(pwrseq_gen->clks[clk]); +} + +static int pwrseq_generic_on(struct pwrseq *pwrseq) +{ + struct pwrseq_generic *pwrseq_gen = to_generic_pwrseq(pwrseq); + int clk, ret = 0; + struct gpio_desc *gpiod_reset = pwrseq_gen->gpiod_reset; + + for (clk = 0; clk < PWRSEQ_MAX_CLKS && pwrseq_gen->clks[clk]; clk++) { + ret = clk_prepare_enable(pwrseq_gen->clks[clk]); + if (ret) { + pr_err("Can't enable clock, ret=%d\n", ret); + goto err_disable_clks; + } + } + + if (gpiod_reset) { + u32 duration_us = pwrseq_gen->duration_us; + + if (duration_us <= 10) + udelay(10); + else + usleep_range(duration_us, duration_us + 100); + gpiod_set_value(gpiod_reset, 0); + } + + return ret; + +err_disable_clks: + while (--clk >= 0) + clk_disable_unprepare(pwrseq_gen->clks[clk]); + + return ret; +} + +static int pwrseq_generic_get(struct device_node *np, struct pwrseq *pwrseq) +{ + struct pwrseq_generic *pwrseq_gen = to_generic_pwrseq(pwrseq); + enum of_gpio_flags flags; + int reset_gpio, clk, ret = 0; + + for (clk = 0; clk < PWRSEQ_MAX_CLKS; clk++) { + pwrseq_gen->clks[clk] = of_clk_get(np, clk); + if (IS_ERR(pwrseq_gen->clks[clk])) { + ret = PTR_ERR(pwrseq_gen->clks[clk]); + if (ret != -ENOENT) + goto err_put_clks; + pwrseq_gen->clks[clk] = NULL; + break; + } + } + + reset_gpio = of_get_named_gpio_flags(np, "reset-gpios", 0, &flags); + if (gpio_is_valid(reset_gpio)) { + unsigned long gpio_flags; + + if (flags & OF_GPIO_ACTIVE_LOW) + gpio_flags = GPIOF_ACTIVE_LOW | GPIOF_OUT_INIT_LOW; + else + gpio_flags = GPIOF_OUT_INIT_HIGH; + + ret = gpio_request_one(reset_gpio, gpio_flags, + "pwrseq-reset-gpios"); + if (ret) + goto err_put_clks; + + pwrseq_gen->gpiod_reset = gpio_to_desc(reset_gpio); + of_property_read_u32(np, "reset-duration-us", + &pwrseq_gen->duration_us); + } else if (reset_gpio == -ENOENT) { + ; /* no such gpio */ + } else { + ret = reset_gpio; + pr_err("Failed to get reset gpio on %s, err = %d\n", + np->full_name, reset_gpio); + goto err_put_clks; + } + + /* allocate new one for later pwrseq instance request */ + ret = pwrseq_generic_alloc_instance(); + if (ret) + goto err_put_gpio; + + return 0; + +err_put_gpio: + if (pwrseq_gen->gpiod_reset) + gpiod_put(pwrseq_gen->gpiod_reset); +err_put_clks: + while (--clk >= 0) + clk_put(pwrseq_gen->clks[clk]); + return ret; +} + +/** + * pwrseq_generic_alloc_instance - power sequence instance allocation + * + * This function is used to allocate one generic power sequence instance, + * it is called when the system boots up and after one power sequence + * instance is got successfully. + * + * Return zero on success or an error code otherwise. + */ +static int pwrseq_generic_alloc_instance(void) +{ + struct pwrseq_generic *pwrseq_gen; + + pwrseq_gen = kzalloc(sizeof(*pwrseq_gen), GFP_KERNEL); + if (!pwrseq_gen) + return -ENOMEM; + + pwrseq_gen->pwrseq.pwrseq_of_match_table = generic_id_table; + pwrseq_gen->pwrseq.get = pwrseq_generic_get; + pwrseq_gen->pwrseq.on = pwrseq_generic_on; + pwrseq_gen->pwrseq.off = pwrseq_generic_off; + pwrseq_gen->pwrseq.put = pwrseq_generic_put; + pwrseq_gen->pwrseq.suspend = pwrseq_generic_suspend; + pwrseq_gen->pwrseq.resume = pwrseq_generic_resume; + + pwrseq_register(&pwrseq_gen->pwrseq); + return 0; +} + +/* Allocate one pwrseq instance during boots up */ +static int __init pwrseq_generic_register(void) +{ + return pwrseq_generic_alloc_instance(); +} +postcore_initcall(pwrseq_generic_register) diff --git a/include/linux/power/pwrseq.h b/include/linux/power/pwrseq.h new file mode 100644 index 0000000..cbc344c --- /dev/null +++ b/include/linux/power/pwrseq.h @@ -0,0 +1,81 @@ +#ifndef __LINUX_PWRSEQ_H +#define __LINUX_PWRSEQ_H + +#include <linux/of.h> + +#define PWRSEQ_MAX_CLKS 3 + +/** + * struct pwrseq - the power sequence structure + * @pwrseq_of_match_table: the OF device id table this pwrseq library supports + * @node: the list pointer to be added to pwrseq list + * @get: the API is used to get pwrseq instance from the device node + * @on: do power on for this pwrseq instance + * @off: do power off for this pwrseq instance + * @put: release the resources on this pwrseq instance + * @suspend: do suspend operation on this pwrseq instance + * @resume: do resume operation on this pwrseq instance + * @used: this pwrseq instance is used by device + */ +struct pwrseq { + const struct of_device_id *pwrseq_of_match_table; + struct list_head node; + int (*get)(struct device_node *np, struct pwrseq *p); + int (*on)(struct pwrseq *p); + void (*off)(struct pwrseq *p); + void (*put)(struct pwrseq *p); + int (*suspend)(struct pwrseq *p); + int (*resume)(struct pwrseq *p); + bool used; + bool suspended; +}; + +/* used for power sequence instance list in one driver */ +struct pwrseq_list_per_dev { + struct pwrseq *pwrseq; + struct list_head list; +}; + +#if IS_ENABLED(CONFIG_POWER_SEQUENCE) +void pwrseq_register(struct pwrseq *pwrseq); +void pwrseq_unregister(struct pwrseq *pwrseq); +struct pwrseq *of_pwrseq_on(struct device_node *np); +void of_pwrseq_off(struct pwrseq *pwrseq); +int of_pwrseq_on_list(struct device_node *np, struct list_head *head); +void of_pwrseq_off_list(struct list_head *head); +int pwrseq_suspend(struct pwrseq *p); +int pwrseq_resume(struct pwrseq *p); +int pwrseq_suspend_list(struct list_head *head); +int pwrseq_resume_list(struct list_head *head); +#else +static inline void pwrseq_register(struct pwrseq *pwrseq) {} +static inline void pwrseq_unregister(struct pwrseq *pwrseq) {} +static inline struct pwrseq *of_pwrseq_on(struct device_node *np) +{ + return NULL; +} +static void of_pwrseq_off(struct pwrseq *pwrseq) {} +static int of_pwrseq_on_list(struct device_node *np, struct list_head *head) +{ + return 0; +} +static void of_pwrseq_off_list(struct list_head *head) {} +static int pwrseq_suspend(struct pwrseq *p) +{ + return 0; +} +static int pwrseq_resume(struct pwrseq *p) +{ + return 0; +} +static int pwrseq_suspend_list(struct list_head *head) +{ + return 0; +} +static int pwrseq_resume_list(struct list_head *head) +{ + return 0; +} +#endif /* CONFIG_POWER_SEQUENCE */ + +#endif /* __LINUX_PWRSEQ_H */