| Message ID | 1440487486-6154-5-git-send-email-wxt@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Caesar Wang <wxt@rock-chips.com> writes: > We can add more domains node in the future. > This patch add the needed clocks into power-controller. > As the discuess about all the device clocks being listed in > the power-domains itself. > > There are several reasons as follows: > > Firstly, the clocks need be turned off to save power when > the system enter the suspend state. So we need to enumerate > the clocks in the dts. In order to power domain can turn on and off. Yes, but this is the job of device drivers which are runtime PM adapted to gate their own clocks. I agree these clocks need to be enumerated in the DTS, but they should be in the device nodes. > Secondly, the reset-circuit should reset be synchronous on RK3288, > then sync revoked. So we need to enable clocks of all devices. > In other words, we have to enable the clocks before you operate them > if all the device clocks are included in someone domians. Yes, this is pretty common for reset. > Someone wish was to get the clocks by reading the clocks from the > device nodes, We can do that but we can solve the above issues. I don't follow this sentence. Are you saying doing that will not solve the above issues? Why not? Please explain. If there are non-device clocks that also need to be enabled before asserting reset, then those are candidates for the power-domain node, but not device clocks. > Anyway, the best ideas we can fix it in the future SoCs. I don't think this is an SoC design issue as this is needed when you have synchronous reset. My concern is primarily around how to describe this in the DT. Kevin
Kevin, On Tue, Aug 25, 2015 at 2:07 PM, Kevin Hilman <khilman@kernel.org> wrote: > Caesar Wang <wxt@rock-chips.com> writes: > >> We can add more domains node in the future. >> This patch add the needed clocks into power-controller. >> As the discuess about all the device clocks being listed in >> the power-domains itself. >> >> There are several reasons as follows: >> >> Firstly, the clocks need be turned off to save power when >> the system enter the suspend state. So we need to enumerate >> the clocks in the dts. In order to power domain can turn on and off. > > Yes, but this is the job of device drivers which are runtime PM adapted > to gate their own clocks. I agree these clocks need to be enumerated in > the DTS, but they should be in the device nodes. I _think_ what Caesar means is that the alternative to this patch is to leave the clocks on all the time as they were during the early days of Rockchip (AKA last year). If the clocks are on all the time then the power domain patches can work fine. However, once you start letting clocks turn off then you need to make sure that the power domain code turns the back on temporarily. >> Secondly, the reset-circuit should reset be synchronous on RK3288, >> then sync revoked. So we need to enable clocks of all devices. >> In other words, we have to enable the clocks before you operate them >> if all the device clocks are included in someone domians. > > Yes, this is pretty common for reset. > >> Someone wish was to get the clocks by reading the clocks from the >> device nodes, We can do that but we can solve the above issues. > > I don't follow this sentence. Are you saying doing that will not solve > the above issues? Why not? Please explain. > > If there are non-device clocks that also need to be enabled before > asserting reset, then those are candidates for the power-domain node, > but not device clocks. It's been a long time and I don't know that I've reviewed every revision of this series, but I think there was a proposal that we shouldn't list clocks here. Instead we should search through and find all devices that refer to this power domain, reach in and find their clocks, and turn them on. Did I get that right? To put things in a concrete way, for pd_vio we'd go through the entire device tree ourselves and find all properties that look like "power-domains = <&power RK3288_PD_VIO>;". We'd then find the parent of those properties and look for a property named "clocks". We'd then iterate over all those clocks and turn those on. Did I get that right? The above doesn't seem like a terribly great idea to me for a number of reasons, including: 1. If I remember correctly, it's important to turn on clocks for devices even if they're not something you're using / have a driver for. If you don't then the device won't get reset properly and this can affect things like suspend/resume because the hardware in the SoC will query all devices at suspend time to make sure they're ready. If a device is wedged because its clock wasn't on at the right them then it will cause problems. 2. If we absolutely need to turn all clocks and we get clocks from device tree nodes on then it means we need device tree nodes for every device in the domain. These would be needed even if there are no accepted bindings for this device yet. So we'd need to do one of: A) Block power domain patches on feature complete bindings for all drivers; B) Make up non-approved compatible strings for all devices and throw them into the DTS; C) Add nodes in the DTS without a compatible string just to satisfy the power domain requirements. None of these seem terribly appealing. 3. It is entirely possible that there are clocks that will be listed in the individual devices that aren't needed for powering on the power domain. I'd tend to believe that PCLK_EDP_CTRL (the pixel clock) doesn't really need to be turned on when adjusting the "VIO" power domain. Right now Caesar has it listed, but it probably isn't needed (Caesar: can you confirm?). 4. It seems just slightly brittle to be reaching into other device nodes and making assumptions about their properties. Yeah, it's probably safe to assume that "clocks" has a list of clocks and "power-domains" will point to something whose first entry is a phandle, but it still seems just a tad bit like violating an abstraction barrier. Anyway, perhaps I'm misunderstanding, or perhaps my concerns are simply not for important things. If so feel free to yell at me. ;) >> Anyway, the best ideas we can fix it in the future SoCs. > > I don't think this is an SoC design issue as this is needed when you > have synchronous reset. My concern is primarily around how to describe > this in the DT. I suppose the SoC could override things and make sure clocks are on in this case? ...or if a clock is off it could defer powering it up somehow until the clock came on? ...dunno how this actually looks in hardware. In any case letting devices get wedged doesn't seem ideal... -Doug
Doug Anderson <dianders@chromium.org> writes: > Kevin, > > On Tue, Aug 25, 2015 at 2:07 PM, Kevin Hilman <khilman@kernel.org> wrote: >> Caesar Wang <wxt@rock-chips.com> writes: >> >>> We can add more domains node in the future. >>> This patch add the needed clocks into power-controller. >>> As the discuess about all the device clocks being listed in >>> the power-domains itself. >>> >>> There are several reasons as follows: >>> >>> Firstly, the clocks need be turned off to save power when >>> the system enter the suspend state. So we need to enumerate >>> the clocks in the dts. In order to power domain can turn on and off. >> >> Yes, but this is the job of device drivers which are runtime PM adapted >> to gate their own clocks. I agree these clocks need to be enumerated in >> the DTS, but they should be in the device nodes. > > I _think_ what Caesar means is that the alternative to this patch is > to leave the clocks on all the time as they were during the early days > of Rockchip (AKA last year). If the clocks are on all the time then > the power domain patches can work fine. However, once you start > letting clocks turn off then you need to make sure that the power > domain code turns the back on temporarily. Yup, I understand that part, and many SoCs have this same "feature". >>> Secondly, the reset-circuit should reset be synchronous on RK3288, >>> then sync revoked. So we need to enable clocks of all devices. >>> In other words, we have to enable the clocks before you operate them >>> if all the device clocks are included in someone domians. >> >> Yes, this is pretty common for reset. >> >>> Someone wish was to get the clocks by reading the clocks from the >>> device nodes, We can do that but we can solve the above issues. >> >> I don't follow this sentence. Are you saying doing that will not solve >> the above issues? Why not? Please explain. >> >> If there are non-device clocks that also need to be enabled before >> asserting reset, then those are candidates for the power-domain node, >> but not device clocks. > > It's been a long time and I don't know that I've reviewed every > revision of this series, but I think there was a proposal that we > shouldn't list clocks here. Instead we should search through and find > all devices that refer to this power domain, reach in and find their > clocks, and turn them on. Did I get that right? Yes... > To put things in a > concrete way, for pd_vio we'd go through the entire device tree > ourselves and find all properties that look like "power-domains = > <&power RK3288_PD_VIO>;". We'd then find the parent of those > properties and look for a property named "clocks". We'd then iterate > over all those clocks and turn those on. Did I get that right? ... but you make it sound like more work than it is. The genpd already keeps a list of devices that refer to the power domain. In fact, the genpd 'attach' method can be platform-specific, so could be used to keep track of a list (or a subset) of clocks which are needed for reset. > The above doesn't seem like a terribly great idea to me for a number > of reasons, including: > > 1. If I remember correctly, it's important to turn on clocks for > devices even if they're not something you're using / have a driver > for. If you don't then the device won't get reset properly and this > can affect things like suspend/resume because the hardware in the SoC > will query all devices at suspend time to make sure they're ready. Correct. This condition also exists in the clock framework when unused clocks are disabled, or if you have drivers but PM_RUNTIME is disabled (which can happen from userspace on a per-device basis) so it needs to be understood and managed already. > If > a device is wedged because its clock wasn't on at the right them then > it will cause problems. Right. I'm not arguing that the power domain doesn't have to deal with device clocks. It has to for sync reset. The objection I have have is where these clocks are described. > 2. If we absolutely need to turn all clocks and we get clocks from > device tree nodes on then it means we need device tree nodes for every > device in the domain. Isn't that the end goal? > These would be needed even if there are no > accepted bindings for this device yet. So we'd need to do one of: A) > Block power domain patches on feature complete bindings for all > drivers; B) Make up non-approved compatible strings for all devices > and throw them into the DTS; C) Add nodes in the DTS without a > compatible string just to satisfy the power domain requirements. None > of these seem terribly appealing. well, I think we can be slightly more accomodating than that and go for somewhere in between: D) In the power-domain DTS, clearly describe why each clock is needed by the power-domain. In particular list out clocks that are not device clocks (and why they need to be asserted for reset) and separate those from device clocks which are only listed in the power-domain because there is not *yet* a driver/binding for that device node. Doing it that way also makes it clear that when a new driver/binding is added, the clocks should be removed from the power-domain node and put into the device node. Also, this addresses my primary concern that the DTS acurately describes the hardware. IOW, in hardware, most of these clocks are are properties of devices, not power-domains, and the DT should reflect that. IMO, if it's not describing the hardware, and is a placeholder until a driver/binding is in place, it should be properly documented. > 3. It is entirely possible that there are clocks that will be listed > in the individual devices that aren't needed for powering on the power > domain. I'd tend to believe that PCLK_EDP_CTRL (the pixel clock) > doesn't really need to be turned on when adjusting the "VIO" power > domain. Right now Caesar has it listed, but it probably isn't needed > (Caesar: can you confirm?). Yes, I suspect there are several of those, which is also why I'd like to see the clocks in the power-domain node described in detail, and exacly why they're needed to be enabled. Also, in the current proposed DTS, clocks that are listed in both device nodes and the power domain are also suspicous, especially when the device node doesn't have a power-domain property. (c.f. vop[bl] nodes and ACLK_VOP[01] clocks.) For that matter, this series doesn't add any devices to the power domains, which makes it even more confusing about how this is meant to work. IOW, with no devices belonging to power-domains, how are the power-domain power_on/power_off callbacks being called? > 4. It seems just slightly brittle to be reaching into other device > nodes and making assumptions about their properties. Yeah, it's > probably safe to assume that "clocks" has a list of clocks and > "power-domains" will point to something whose first entry is a > phandle, but it still seems just a tad bit like violating an > abstraction barrier. shmobile is already doing this with platform-specific genpd attach callbacks, and using the pm_clk API. I don't see any issues with that. > Anyway, perhaps I'm misunderstanding, or perhaps my concerns are > simply not for important things. If so feel free to yell at me. ;) No need for me to yell. Your concerns are perfectly valid, and it's not my style anyways. ;) Thanks for the feedback, Kevin
On Tue, Aug 25, 2015 at 3:45 PM, Kevin Hilman <khilman@kernel.org> wrote: > Doug Anderson <dianders@chromium.org> writes: > >> To put things in a >> concrete way, for pd_vio we'd go through the entire device tree >> ourselves and find all properties that look like "power-domains = >> <&power RK3288_PD_VIO>;". We'd then find the parent of those >> properties and look for a property named "clocks". We'd then iterate >> over all those clocks and turn those on. Did I get that right? > > ... but you make it sound like more work than it is. The genpd already > keeps a list of devices that refer to the power domain. In fact, the > genpd 'attach' method can be platform-specific, so could be used to keep > track of a list (or a subset) of clocks which are needed for reset. That is not really workable: the attach and detach happen in probe/remove path; if you do not have driver for the device you will miss the clocks for it. And the quirk of this SoC is that we need to turn all clocks during domain transition, regardless of whether there is a driver for all devices. Thanks.
Kevin, On Tue, Aug 25, 2015 at 3:45 PM, Kevin Hilman <khilman@kernel.org> wrote: >> To put things in a >> concrete way, for pd_vio we'd go through the entire device tree >> ourselves and find all properties that look like "power-domains = >> <&power RK3288_PD_VIO>;". We'd then find the parent of those >> properties and look for a property named "clocks". We'd then iterate >> over all those clocks and turn those on. Did I get that right? > > ... but you make it sound like more work than it is. The genpd already > keeps a list of devices that refer to the power domain. In fact, the > genpd 'attach' method can be platform-specific, so could be used to keep > track of a list (or a subset) of clocks which are needed for reset. OK. I'll need to dig through and figure out how this works. So this list will include devices whose drivers are compiled out and also devices that are marked as 'status = "disabled";' in the device tree? It would need to do this in case someone had a board that didn't use one of these peripherals (since we still need to clock it as we make the domain go up and down). I took a quick gander and didn't see code that would do this. Can you give me a quick pointer? We also need to really make sure that there are no probe order issues... >> 2. If we absolutely need to turn all clocks and we get clocks from >> device tree nodes on then it means we need device tree nodes for every >> device in the domain. > > Isn't that the end goal? In the ideal world, yes. ...but it's possibly years before all drivers are added, or possibly they never will be. To name a concrete example, unless I'm mistaken, the "hardware crypto" device present in exynos5250 still isn't anywhere in upstream despite several years having passed. There are just so many devices in these SoCs that aren't used. >> These would be needed even if there are no >> accepted bindings for this device yet. So we'd need to do one of: A) >> Block power domain patches on feature complete bindings for all >> drivers; B) Make up non-approved compatible strings for all devices >> and throw them into the DTS; C) Add nodes in the DTS without a >> compatible string just to satisfy the power domain requirements. None >> of these seem terribly appealing. > > well, I think we can be slightly more accomodating than that and go for > somewhere in between: > > D) In the power-domain DTS, clearly describe why each clock is needed by > the power-domain. In particular list out clocks that are not device > clocks (and why they need to be asserted for reset) and separate those > from device clocks which are only listed in the power-domain because > there is not *yet* a driver/binding for that device node. > > Doing it that way also makes it clear that when a new driver/binding is > added, the clocks should be removed from the power-domain node and put > into the device node. > > Also, this addresses my primary concern that the DTS acurately describes > the hardware. IOW, in hardware, most of these clocks are are properties > of devices, not power-domains, and the DT should reflect that. > > IMO, if it's not describing the hardware, and is a placeholder until a > driver/binding is in place, it should be properly documented. Agreed that more documentation about why each clock is needed would go a long way to helping. I'd say that the clocks are properties of the device, but I'd again assert that not all clocks that are owned by the device are relevant to turning power domains on and off. If you really wanted to be correct, you'd add a property like this to devices: power-on-clocks = <..>; ...this would be a list of clocks needed to power on the device properly. If such a property was added that would erase my objection to that. Note that you could say that if "power-on-clocks" wasn't present then you could fall back to "clocks". I do worry a little bit about over-engineering, so I guess this could just be a future improvement... >> 3. It is entirely possible that there are clocks that will be listed >> in the individual devices that aren't needed for powering on the power >> domain. I'd tend to believe that PCLK_EDP_CTRL (the pixel clock) >> doesn't really need to be turned on when adjusting the "VIO" power >> domain. Right now Caesar has it listed, but it probably isn't needed >> (Caesar: can you confirm?). > > Yes, I suspect there are several of those, which is also why I'd like to > see the clocks in the power-domain node described in detail, and exacly > why they're needed to be enabled. I guess my point was that this was a reason for doing something like "power-on-clocks" because it would let you avoid turning on PCLK_EDP_CTRL (which is listed in the "clocks" of the EDP device but maybe not needed while powering on/off the domain). ...or just have the list as part of the power domain. :-P > Also, in the current proposed DTS, clocks that are listed in both device > nodes and the power domain are also suspicous, especially when the > device node doesn't have a power-domain property. (c.f. vop[bl] nodes > and ACLK_VOP[01] clocks.) For that matter, this series doesn't add any > devices to the power domains, which makes it even more confusing about > how this is meant to work. IOW, with no devices belonging to > power-domains, how are the power-domain power_on/power_off callbacks > being called? I totally believe that our current hardware definition isn't so correct (though it does seem to work). I'd be that devices that never turn off often don't have a power domain defined (though they technically have one) because that power domain always happens to be on and no amount of specifying will allow it to turn off. An advantage of what you're pushing for is that it really forces us to think about exactly what clocks are needed and forces us to specify power domains more correctly. >> 4. It seems just slightly brittle to be reaching into other device >> nodes and making assumptions about their properties. Yeah, it's >> probably safe to assume that "clocks" has a list of clocks and >> "power-domains" will point to something whose first entry is a >> phandle, but it still seems just a tad bit like violating an >> abstraction barrier. > > shmobile is already doing this with platform-specific genpd attach > callbacks, and using the pm_clk API. I don't see any issues with that. I can see it working. It's more of a feeling of unease. ...but that's not a good enough reason to reject something... -Doug
? 2015?08?26? 06:45, Kevin Hilman ??: > Doug Anderson <dianders@chromium.org> writes: > >> Kevin, >> >> On Tue, Aug 25, 2015 at 2:07 PM, Kevin Hilman <khilman@kernel.org> wrote: >>> Caesar Wang <wxt@rock-chips.com> writes: >>> >>>> We can add more domains node in the future. >>>> This patch add the needed clocks into power-controller. >>>> As the discuess about all the device clocks being listed in >>>> the power-domains itself. >>>> >>>> There are several reasons as follows: >>>> >>>> Firstly, the clocks need be turned off to save power when >>>> the system enter the suspend state. So we need to enumerate >>>> the clocks in the dts. In order to power domain can turn on and off. >>> Yes, but this is the job of device drivers which are runtime PM adapted >>> to gate their own clocks. I agree these clocks need to be enumerated in >>> the DTS, but they should be in the device nodes. >> I _think_ what Caesar means is that the alternative to this patch is >> to leave the clocks on all the time as they were during the early days >> of Rockchip (AKA last year). If the clocks are on all the time then >> the power domain patches can work fine. However, once you start >> letting clocks turn off then you need to make sure that the power >> domain code turns the back on temporarily. > Yup, I understand that part, and many SoCs have this same "feature". > >>>> Secondly, the reset-circuit should reset be synchronous on RK3288, >>>> then sync revoked. So we need to enable clocks of all devices. >>>> In other words, we have to enable the clocks before you operate them >>>> if all the device clocks are included in someone domians. >>> Yes, this is pretty common for reset. >>> >>>> Someone wish was to get the clocks by reading the clocks from the >>>> device nodes, We can do that but we can solve the above issues. >>> I don't follow this sentence. Are you saying doing that will not solve >>> the above issues? Why not? Please explain. >>> >>> If there are non-device clocks that also need to be enabled before >>> asserting reset, then those are candidates for the power-domain node, >>> but not device clocks. >> It's been a long time and I don't know that I've reviewed every >> revision of this series, but I think there was a proposal that we >> shouldn't list clocks here. Instead we should search through and find >> all devices that refer to this power domain, reach in and find their >> clocks, and turn them on. Did I get that right? > Yes... Sounds resonable, the domain(e.g. "pd_vio"...) idle will be abnormal but you have registered all devices. Why? AFAIK, you need turn on the noc/ip clocks if we are operating the "pd_vio" domain to enter the idle status. if the noc is same clock with ip side. We need turn on the IP side clocks. Otherwise we need turn on the noc clocks. >> To put things in a >> concrete way, for pd_vio we'd go through the entire device tree >> ourselves and find all properties that look like "power-domains = >> <&power RK3288_PD_VIO>;". We'd then find the parent of those >> properties and look for a property named "clocks". We'd then iterate >> over all those clocks and turn those on. Did I get that right? > ... but you make it sound like more work than it is. The genpd already > keeps a list of devices that refer to the power domain. In fact, the > genpd 'attach' method can be platform-specific, so could be used to keep > track of a list (or a subset) of clocks which are needed for reset. > >> The above doesn't seem like a terribly great idea to me for a number >> of reasons, including: >> >> 1. If I remember correctly, it's important to turn on clocks for >> devices even if they're not something you're using / have a driver >> for. If you don't then the device won't get reset properly and this >> can affect things like suspend/resume because the hardware in the SoC >> will query all devices at suspend time to make sure they're ready. > Correct. This condition also exists in the clock framework when unused > clocks are disabled, or if you have drivers but PM_RUNTIME is disabled > (which can happen from userspace on a per-device basis) so it needs to > be understood and managed already. > >> If >> a device is wedged because its clock wasn't on at the right them then >> it will cause problems. > Right. I'm not arguing that the power domain doesn't have to deal with > device clocks. It has to for sync reset. The objection I have have is > where these clocks are described. > >> 2. If we absolutely need to turn all clocks and we get clocks from >> device tree nodes on then it means we need device tree nodes for every >> device in the domain. > Isn't that the end goal? > >> These would be needed even if there are no >> accepted bindings for this device yet. So we'd need to do one of: A) >> Block power domain patches on feature complete bindings for all >> drivers; B) Make up non-approved compatible strings for all devices >> and throw them into the DTS; C) Add nodes in the DTS without a >> compatible string just to satisfy the power domain requirements. None >> of these seem terribly appealing. > well, I think we can be slightly more accomodating than that and go for > somewhere in between: > > D) In the power-domain DTS, clearly describe why each clock is needed by > the power-domain. In particular list out clocks that are not device > clocks (and why they need to be asserted for reset) and separate those > from device clocks which are only listed in the power-domain because > there is not *yet* a driver/binding for that device node. > > Doing it that way also makes it clear that when a new driver/binding is > added, the clocks should be removed from the power-domain node and put > into the device node. > > Also, this addresses my primary concern that the DTS acurately describes > the hardware. IOW, in hardware, most of these clocks are are properties > of devices, not power-domains, and the DT should reflect that. > > IMO, if it's not describing the hardware, and is a placeholder until a > driver/binding is in place, it should be properly documented. > >> 3. It is entirely possible that there are clocks that will be listed >> in the individual devices that aren't needed for powering on the power >> domain. I'd tend to believe that PCLK_EDP_CTRL (the pixel clock) >> doesn't really need to be turned on when adjusting the "VIO" power >> domain. Right now Caesar has it listed, but it probably isn't needed >> (Caesar: can you confirm?). > Yes, I suspect there are several of those, which is also why I'd like to > see the clocks in the power-domain node described in detail, and exacly > why they're needed to be enabled. > > Also, in the current proposed DTS, clocks that are listed in both device > nodes and the power domain are also suspicous, especially when the > device node doesn't have a power-domain property. (c.f. vop[bl] nodes > and ACLK_VOP[01] clocks.) For that matter, this series doesn't add any > devices to the power domains, which makes it even more confusing about > how this is meant to work. IOW, with no devices belonging to > power-domains, how are the power-domain power_on/power_off callbacks > being called? Okay, As the cover-letter lists. We can add the EDP/VOP domain node to test in next-kernel. Also, the chromium-3.14 has some devices to complete it. https://chromium.googlesource.com/chromiumos/third_party/kernel/+/v3.14 ---- Thanks, Caesar >> 4. It seems just slightly brittle to be reaching into other device >> nodes and making assumptions about their properties. Yeah, it's >> probably safe to assume that "clocks" has a list of clocks and >> "power-domains" will point to something whose first entry is a >> phandle, but it still seems just a tad bit like violating an >> abstraction barrier. > shmobile is already doing this with platform-specific genpd attach > callbacks, and using the pm_clk API. I don't see any issues with that. > >> Anyway, perhaps I'm misunderstanding, or perhaps my concerns are >> simply not for important things. If so feel free to yell at me. ;) > No need for me to yell. Your concerns are perfectly valid, and it's not > my style anyways. ;) > > Thanks for the feedback, > > Kevin > > > > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
Dmitry Torokhov <dmitry.torokhov@gmail.com> writes: > On Tue, Aug 25, 2015 at 3:45 PM, Kevin Hilman <khilman@kernel.org> wrote: >> Doug Anderson <dianders@chromium.org> writes: >> >>> To put things in a >>> concrete way, for pd_vio we'd go through the entire device tree >>> ourselves and find all properties that look like "power-domains = >>> <&power RK3288_PD_VIO>;". We'd then find the parent of those >>> properties and look for a property named "clocks". We'd then iterate >>> over all those clocks and turn those on. Did I get that right? >> >> ... but you make it sound like more work than it is. The genpd already >> keeps a list of devices that refer to the power domain. In fact, the >> genpd 'attach' method can be platform-specific, so could be used to keep >> track of a list (or a subset) of clocks which are needed for reset. > > That is not really workable: the attach and detach happen in > probe/remove path; if you do not have driver for the device you will > miss the clocks for it. And in my proposal, I suggested that clocks without drivers are good candidates to list in the domain, with the caveat that the be called out (documented) as being device clocks that are missing a driver, so when a driver shows up they can be moved accordingly, and in a way that actually describes the hardware. > And the quirk of this SoC is that we need to > turn all clocks during domain transition, regardless of whether there > is a driver for all devices. I understand. And that "quirk" is not unique to rockchip socs. Kevin
Kevin, On Thu, Aug 27, 2015 at 5:24 PM, Kevin Hilman <khilman@kernel.org> wrote: >> That is not really workable: the attach and detach happen in >> probe/remove path; if you do not have driver for the device you will >> miss the clocks for it. > > And in my proposal, I suggested that clocks without drivers are > good candidates to list in the domain, with the caveat that the be > called out (documented) as being device clocks that are missing a > driver, so when a driver shows up they can be moved accordingly, and in > a way that actually describes the hardware. What happens if someone disables the driver using the CONFIG subsystem? What happens if this is a device that someone has set to 'status = "disabled";' in the device tree? Even if the device is disabled in one of those two ways, we still need the clocks to be turned on. ...so if we turn on/off the VIO domain we need to turn on the EDP clock even if there's no EDP in the current board / config. We might turn on/off VIO for one of the other devices in the VIO domain for one of the other devices in VIO that we are using. -Doug
Hi Doug, Quoting Doug Anderson (2015-08-27 19:03:20) > Kevin, > > On Thu, Aug 27, 2015 at 5:24 PM, Kevin Hilman <khilman@kernel.org> wrote: > >> That is not really workable: the attach and detach happen in > >> probe/remove path; if you do not have driver for the device you will > >> miss the clocks for it. > > > > And in my proposal, I suggested that clocks without drivers are > > good candidates to list in the domain, with the caveat that the be > > called out (documented) as being device clocks that are missing a > > driver, so when a driver shows up they can be moved accordingly, and in > > a way that actually describes the hardware. > > What happens if someone disables the driver using the CONFIG subsystem? Kevin asked me to chime in on this thread, as I have a half-baked idea that might solve the problem posed by your question above. One thing I have been considering for a while is a fallback compatible string that can be used for an IP block when either there is no driver loaded or no driver exists at all. Something like "generic-ip-block". The purpose of this compatible string is to allow us to model resource consumption in dts accurately, regardless of whether or not a proper driver has been written in Linux. This idea was born out of the simple-fb binding/driver discussion last year[0]. Obviously such a binding would not enable any of the logic or function of that IP block; that would require a proper driver. But it would allow us to properly link system-wide resources that are consumed: the generic-ip could consume clocks and regulators, it could belong to power domains, etc. For this reason I have also thought that "generic-resource-consumer" is an accurate compatible string. This spares us from having to encode nasty details into the power domain binding, which is exactly what would happen if you needed a dedicated list of clocks in the power domain node that were not claimed by device nodes/drivers. Note that a real driver might exist for an IP block, but if that driver is disabled in Kconfig AND the corresponding dt node has this fallback compatible string, then we could be OK, from the perspective of the power domain problem. > > What happens if this is a device that someone has set to 'status = > "disabled";' in the device tree? If someone does that, then I think we should let that break power domain transitions. > > Even if the device is disabled in one of those two ways, we still need > the clocks to be turned on. ...so if we turn on/off the VIO domain we > need to turn on the EDP clock even if there's no EDP in the current > board / config. We might turn on/off VIO for one of the other devices > in the VIO domain for one of the other devices in VIO that we are > using. I'm hesitant to mention this but I am working on a patch series to implement a clock "handoff" mechanism (also inspired by the simplefb discussion). This allows us to set a per-clock flag that tells the framework to enable that clock at registration time, and then the first clock consumer driver to come along and claim that clock inherits that clock enable reference count. I'm working on v2 that lets us set this flag from DT, but I really only plan to do this for special cases. For the normal case the flag should be set in the Linux clock provider driver. In the mean time v1 is under discussion[1]. [0] http://lkml.kernel.org/r/<1407914239-12054-5-git-send-email-libv@skynet.be> [1] http://lkml.kernel.org/r/<1438974570-20812-1-git-send-email-mturquette@baylibre.com> Regards, Mike > > -Doug > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
Mike, On Fri, Aug 28, 2015 at 1:02 PM, Michael Turquette <mturquette@linaro.org> wrote: > Hi Doug, > > Quoting Doug Anderson (2015-08-27 19:03:20) >> Kevin, >> >> On Thu, Aug 27, 2015 at 5:24 PM, Kevin Hilman <khilman@kernel.org> wrote: >> >> That is not really workable: the attach and detach happen in >> >> probe/remove path; if you do not have driver for the device you will >> >> miss the clocks for it. >> > >> > And in my proposal, I suggested that clocks without drivers are >> > good candidates to list in the domain, with the caveat that the be >> > called out (documented) as being device clocks that are missing a >> > driver, so when a driver shows up they can be moved accordingly, and in >> > a way that actually describes the hardware. >> >> What happens if someone disables the driver using the CONFIG subsystem? > > Kevin asked me to chime in on this thread, as I have a half-baked idea > that might solve the problem posed by your question above. > > One thing I have been considering for a while is a fallback compatible > string that can be used for an IP block when either there is no driver > loaded or no driver exists at all. Something like "generic-ip-block". > > The purpose of this compatible string is to allow us to model resource > consumption in dts accurately, regardless of whether or not a proper > driver has been written in Linux. This idea was born out of the > simple-fb binding/driver discussion last year[0]. > > Obviously such a binding would not enable any of the logic or function > of that IP block; that would require a proper driver. But it would allow > us to properly link system-wide resources that are consumed: the > generic-ip could consume clocks and regulators, it could belong to power > domains, etc. For this reason I have also thought that > "generic-resource-consumer" is an accurate compatible string. > > This spares us from having to encode nasty details into the power domain > binding, which is exactly what would happen if you needed a dedicated > list of clocks in the power domain node that were not claimed by device > nodes/drivers. > > Note that a real driver might exist for an IP block, but if that driver > is disabled in Kconfig AND the corresponding dt node has this fallback > compatible string, then we could be OK, from the perspective of the > power domain problem. OK, so that could solve the "Kconfig" problem. >> What happens if this is a device that someone has set to 'status = >> "disabled";' in the device tree? > > If someone does that, then I think we should let that break power domain > transitions. So if you're on a board that doesn't use EDP but uses LVDS then their suspend/resume should be broken? Specifically, it's my understanding that on this SoC: 1. There's a single "video IO" power domain that contains things like the video output processor, EDP, LVDS, HDMI, etc. 2. When you turn on/off the power domain it's important to clock all devices in the domain during the reset. 3. If you don't clock all devices during the reset they get left in a "wedged" state. 4. If you try to do things like suspend/resume it will query all devices. If they've been left in the wedged state then suspend/resume will be blocked. ...so this problem is still not solved. >> Even if the device is disabled in one of those two ways, we still need >> the clocks to be turned on. ...so if we turn on/off the VIO domain we >> need to turn on the EDP clock even if there's no EDP in the current >> board / config. We might turn on/off VIO for one of the other devices >> in the VIO domain for one of the other devices in VIO that we are >> using. > > I'm hesitant to mention this but I am working on a patch series to > implement a clock "handoff" mechanism (also inspired by the simplefb > discussion). This allows us to set a per-clock flag that tells the > framework to enable that clock at registration time, and then the first > clock consumer driver to come along and claim that clock inherits that > clock enable reference count. > > I'm working on v2 that lets us set this flag from DT, but I really only > plan to do this for special cases. For the normal case the flag should > be set in the Linux clock provider driver. In the mean time v1 is under > discussion[1]. In this case if we're using LVDS and HDMI on a system, we don't want the EDP clock always on (that just wastes power). We just need to clock it for the power domain transition. Note that nothing in the above solves the problem that not all clocks for a given device need to be turned on during power domain transitions. If we're trying to describe hardware perfectly, we'd really need to add to each device: clocks = <clk1>, <clk2>, <clk3>, ... clock-names = "name1", "name2", "name3", ... clocks-for-power-domain-transition = "name2", "name3"; ...since perhaps the clock named "name1" isn't required for power domain transitions and so shouldn't be turned on. Note that once we're going through and adding a special list, it doesn't seem that different than just adding the list in the power domain node instead (other than being more complex).
Michael Turquette <mturquette@linaro.org> writes: > Hi Doug, > > Quoting Doug Anderson (2015-08-27 19:03:20) >> Kevin, >> >> On Thu, Aug 27, 2015 at 5:24 PM, Kevin Hilman <khilman@kernel.org> wrote: >> >> That is not really workable: the attach and detach happen in >> >> probe/remove path; if you do not have driver for the device you will >> >> miss the clocks for it. >> > >> > And in my proposal, I suggested that clocks without drivers are >> > good candidates to list in the domain, with the caveat that the be >> > called out (documented) as being device clocks that are missing a >> > driver, so when a driver shows up they can be moved accordingly, and in >> > a way that actually describes the hardware. >> >> What happens if someone disables the driver using the CONFIG subsystem? > > Kevin asked me to chime in on this thread, as I have a half-baked idea > that might solve the problem posed by your question above. > > One thing I have been considering for a while is a fallback compatible > string that can be used for an IP block when either there is no driver > loaded or no driver exists at all. Something like "generic-ip-block". > > The purpose of this compatible string is to allow us to model resource > consumption in dts accurately, regardless of whether or not a proper > driver has been written in Linux. This idea was born out of the > simple-fb binding/driver discussion last year[0]. > > Obviously such a binding would not enable any of the logic or function > of that IP block; that would require a proper driver. But it would allow > us to properly link system-wide resources that are consumed: the > generic-ip could consume clocks and regulators, it could belong to power > domains, etc. For this reason I have also thought that > "generic-resource-consumer" is an accurate compatible string. > > This spares us from having to encode nasty details into the power domain > binding, which is exactly what would happen if you needed a dedicated > list of clocks in the power domain node that were not claimed by device > nodes/drivers. > > Note that a real driver might exist for an IP block, but if that driver > is disabled in Kconfig AND the corresponding dt node has this fallback > compatible string, then we could be OK, from the perspective of the > power domain problem. > >> >> What happens if this is a device that someone has set to 'status = >> "disabled";' in the device tree? > > If someone does that, then I think we should let that break power domain > transitions. > Well, the catch here is that status=disabled doesn't necessarily mean that the IP block isn't present. It can mean "not used", "not needed", "not wired up to external iface" etc., but if the IP block is physcially present, then its clocks are still needed for a power domain reset. If a node is marked as status=disabled, then even the fallback compatible doesn't help, as the fallback driver won't be probed/loaded either. :( Speaking of loading, the module loading/unloading complicates this even further. Even if a pm-domain is managing the clocks from a device node, if a driver is unloaded (and thus removed from the pm domain), the pm domain driver would stop managing that clock and then the domain reset would stop working again. Ugh. The more I think through all these corner cases, and also the fact that we never seem to be able to get a fully accurate description of how the SoCs actually work at this level (due to missing/poor/wrong docs), I think the having both the power domain and the device nodes claiming clocks is the most flexible solution. It's using the clock consumer binding in a perfectly acceptable way, and the clock fwk usecounting will manage usecounting etc. However, I still stick by my earlier insistence that the DT nodes be thorougly documented though, and be very specific why the clocks are needed by the power domain, and especially which clocks are device clocks, etc. I (and hopefully others) will be looking closely at the DTs that come through and make that clocks listed in the domains are well described. @Doug: thanks for taking the time to spell out the variou corner cases. Kevin
Quoting Doug Anderson (2015-08-28 14:08:52) > Mike, > > On Fri, Aug 28, 2015 at 1:02 PM, Michael Turquette > <mturquette@linaro.org> wrote: > > Hi Doug, > > > > Quoting Doug Anderson (2015-08-27 19:03:20) > >> Kevin, > >> > >> On Thu, Aug 27, 2015 at 5:24 PM, Kevin Hilman <khilman@kernel.org> wrote: > >> >> That is not really workable: the attach and detach happen in > >> >> probe/remove path; if you do not have driver for the device you will > >> >> miss the clocks for it. > >> > > >> > And in my proposal, I suggested that clocks without drivers are > >> > good candidates to list in the domain, with the caveat that the be > >> > called out (documented) as being device clocks that are missing a > >> > driver, so when a driver shows up they can be moved accordingly, and in > >> > a way that actually describes the hardware. > >> > >> What happens if someone disables the driver using the CONFIG subsystem? > > > > Kevin asked me to chime in on this thread, as I have a half-baked idea > > that might solve the problem posed by your question above. > > > > One thing I have been considering for a while is a fallback compatible > > string that can be used for an IP block when either there is no driver > > loaded or no driver exists at all. Something like "generic-ip-block". > > > > The purpose of this compatible string is to allow us to model resource > > consumption in dts accurately, regardless of whether or not a proper > > driver has been written in Linux. This idea was born out of the > > simple-fb binding/driver discussion last year[0]. > > > > Obviously such a binding would not enable any of the logic or function > > of that IP block; that would require a proper driver. But it would allow > > us to properly link system-wide resources that are consumed: the > > generic-ip could consume clocks and regulators, it could belong to power > > domains, etc. For this reason I have also thought that > > "generic-resource-consumer" is an accurate compatible string. > > > > This spares us from having to encode nasty details into the power domain > > binding, which is exactly what would happen if you needed a dedicated > > list of clocks in the power domain node that were not claimed by device > > nodes/drivers. > > > > Note that a real driver might exist for an IP block, but if that driver > > is disabled in Kconfig AND the corresponding dt node has this fallback > > compatible string, then we could be OK, from the perspective of the > > power domain problem. > > OK, so that could solve the "Kconfig" problem. > > > >> What happens if this is a device that someone has set to 'status = > >> "disabled";' in the device tree? > > > > If someone does that, then I think we should let that break power domain > > transitions. > > So if you're on a board that doesn't use EDP but uses LVDS then their > suspend/resume should be broken? > > Specifically, it's my understanding that on this SoC: > > 1. There's a single "video IO" power domain that contains things like > the video output processor, EDP, LVDS, HDMI, etc. > > 2. When you turn on/off the power domain it's important to clock all > devices in the domain during the reset. > > 3. If you don't clock all devices during the reset they get left in a > "wedged" state. > > 4. If you try to do things like suspend/resume it will query all > devices. If they've been left in the wedged state then suspend/resume > will be blocked. > > ...so this problem is still not solved. I talked to Kevin about this and we both agree that the power domain's use of the clock consumer binding is totally valid. So I still think there might be a use for the generic-ip binding, but I also think the solution in this original patch also looks sane. A paraphrase of my discussion with Kevin is that we can think of both the device (EDP, LVDS) and the power domain as clock consumers, so basically there is nothing weird going on here. Feel free to add: Reviewed-by: Michael Turquette <mturquette@baylibre.com> I do also agree with Kevin that we have a lot of layering issues to consider here with description of PM hardware, and there are temptations to take the shortest route to get things working. Truly modeling relationships is the best way forward and we need to keep that in mind over the long haul. > > >> Even if the device is disabled in one of those two ways, we still need > >> the clocks to be turned on. ...so if we turn on/off the VIO domain we > >> need to turn on the EDP clock even if there's no EDP in the current > >> board / config. We might turn on/off VIO for one of the other devices > >> in the VIO domain for one of the other devices in VIO that we are > >> using. > > > > I'm hesitant to mention this but I am working on a patch series to > > implement a clock "handoff" mechanism (also inspired by the simplefb > > discussion). This allows us to set a per-clock flag that tells the > > framework to enable that clock at registration time, and then the first > > clock consumer driver to come along and claim that clock inherits that > > clock enable reference count. > > > > I'm working on v2 that lets us set this flag from DT, but I really only > > plan to do this for special cases. For the normal case the flag should > > be set in the Linux clock provider driver. In the mean time v1 is under > > discussion[1]. > > In this case if we're using LVDS and HDMI on a system, we don't want > the EDP clock always on (that just wastes power). We just need to > clock it for the power domain transition. > > > Note that nothing in the above solves the problem that not all clocks > for a given device need to be turned on during power domain > transitions. If we're trying to describe hardware perfectly, we'd > really need to add to each device: > > clocks = <clk1>, <clk2>, <clk3>, ... > clock-names = "name1", "name2", "name3", ... > clocks-for-power-domain-transition = "name2", "name3"; > > ...since perhaps the clock named "name1" isn't required for power > domain transitions and so shouldn't be turned on. Note that once > we're going through and adding a special list, it doesn't seem that > different than just adding the list in the power domain node instead > (other than being more complex). Yes, this is a broader issue with mapping clocks onto consumer use cases. The same is exactly true for any consumer driver that consumes multiple clocks and enables/disables them differently (e.g. interface clocks versus functional clocks). How to convey that in DT? Regards, Mike
diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi index 22316d0..161931d 100644 --- a/arch/arm/boot/dts/rk3288.dtsi +++ b/arch/arm/boot/dts/rk3288.dtsi @@ -44,6 +44,7 @@ #include <dt-bindings/pinctrl/rockchip.h> #include <dt-bindings/clock/rk3288-cru.h> #include <dt-bindings/thermal/thermal.h> +#include <dt-bindings/power-domain/rk3288.h> #include "skeleton.dtsi" / { @@ -590,8 +591,65 @@ }; pmu: power-management@ff730000 { - compatible = "rockchip,rk3288-pmu", "syscon"; + compatible = "rockchip,rk3288-pmu", "syscon", "simple-mfd"; reg = <0xff730000 0x100>; + + power: power-controller { + compatible = "rockchip,rk3288-power-controller"; + #power-domain-cells = <1>; + #address-cells = <1>; + #size-cells = <0>; + + pd_gpu { + reg = <RK3288_PD_GPU>; + clocks = <&cru ACLK_GPU>; + }; + + pd_hevc { + reg = <RK3288_PD_HEVC>; + clocks = <&cru ACLK_HEVC>, + <&cru SCLK_HEVC_CABAC>, + <&cru SCLK_HEVC_CORE>, + <&cru HCLK_HEVC>; + }; + + pd_vio { + reg = <RK3288_PD_VIO>; + clocks = <&cru ACLK_IEP>, + <&cru ACLK_ISP>, + <&cru ACLK_RGA>, + <&cru ACLK_VIP>, + <&cru ACLK_VOP0>, + <&cru ACLK_VOP1>, + <&cru DCLK_VOP0>, + <&cru DCLK_VOP1>, + <&cru HCLK_IEP>, + <&cru HCLK_ISP>, + <&cru HCLK_RGA>, + <&cru HCLK_VIP>, + <&cru HCLK_VOP0>, + <&cru HCLK_VOP1>, + <&cru PCLK_EDP_CTRL>, + <&cru PCLK_HDMI_CTRL>, + <&cru PCLK_LVDS_PHY>, + <&cru PCLK_MIPI_CSI>, + <&cru PCLK_MIPI_DSI0>, + <&cru PCLK_MIPI_DSI1>, + <&cru SCLK_EDP_24M>, + <&cru SCLK_EDP>, + <&cru SCLK_HDMI_CEC>, + <&cru SCLK_HDMI_HDCP>, + <&cru SCLK_ISP_JPE>, + <&cru SCLK_ISP>, + <&cru SCLK_RGA>; + }; + + pd_video { + reg = <RK3288_PD_VIDEO>; + clocks = <&cru ACLK_VCODEC>, + <&cru HCLK_VCODEC>; + }; + }; }; sgrf: syscon@ff740000 {