* Adding depends-on DT binding to break cyclic dependencies @ 2019-08-22 6:54 Saravana Kannan 2019-08-27 20:18 ` Saravana Kannan 2019-08-29 16:28 ` Rob Herring 0 siblings, 2 replies; 8+ messages in thread From: Saravana Kannan @ 2019-08-22 6:54 UTC (permalink / raw) To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Frank Rowand Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML, Android Kernel Team Hi Rob, Frank, Greg and I got together during ELC and had an extensive and very productive discussion about my "postboot supplier state cleanup" patch series [1]. The three of us are on the same page now -- the series as it stands is the direction we want to go in, with some minor refactoring, documentation and naming changes. However, one of the things Frank is concerned about (and Greg and I agree) in the current patch series is that the "cyclic dependency breaking" logic has been pushed off to individual drivers using the edit_links() callback. The concern being, there are going to be multiple device specific ad hoc implementations to break a cyclic dependency. Also, if a device can be part of a cyclic dependency, the driver for that device has to check for specific system/products in which the device is part of a cyclic dependency (because it might not always be part of a cycle), and then potentially have cycle/product specific code to break the cycle (since the cycle can be different on each system/product). One way to avoid all of the device/driver specific code and simplify my patch series by a non-trivial amount would be by adding a "depends-on" DT binding that can ONLY be used to break cycles. We can document it as such and reject any attempts to use it for other purposes. When a depends-on property is present in a device node, that specific device's supplier list will be parsed ONLY from the depends-on property and the other properties won't be parsed for deriving dependency information for that device. Frank, Greg and I like this usage model for a new depends-on DT binding. Is this something you'd be willing to accept? Thanks, Saravana [1] - https://lore.kernel.org/lkml/20190731221721.187713-1-saravanak@google.com/ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Adding depends-on DT binding to break cyclic dependencies 2019-08-22 6:54 Adding depends-on DT binding to break cyclic dependencies Saravana Kannan @ 2019-08-27 20:18 ` Saravana Kannan 2019-08-29 16:28 ` Rob Herring 1 sibling, 0 replies; 8+ messages in thread From: Saravana Kannan @ 2019-08-27 20:18 UTC (permalink / raw) To: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Frank Rowand Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML, Android Kernel Team On Wed, Aug 21, 2019 at 11:54 PM Saravana Kannan <saravanak@google.com> wrote: > > Hi Rob, > > Frank, Greg and I got together during ELC and had an extensive and > very productive discussion about my "postboot supplier state cleanup" > patch series [1]. The three of us are on the same page now -- the > series as it stands is the direction we want to go in, with some minor > refactoring, documentation and naming changes. > > However, one of the things Frank is concerned about (and Greg and I > agree) in the current patch series is that the "cyclic dependency > breaking" logic has been pushed off to individual drivers using the > edit_links() callback. > > The concern being, there are going to be multiple device specific ad > hoc implementations to break a cyclic dependency. Also, if a device > can be part of a cyclic dependency, the driver for that device has to > check for specific system/products in which the device is part of a > cyclic dependency (because it might not always be part of a cycle), > and then potentially have cycle/product specific code to break the > cycle (since the cycle can be different on each system/product). > > One way to avoid all of the device/driver specific code and simplify > my patch series by a non-trivial amount would be by adding a > "depends-on" DT binding that can ONLY be used to break cycles. We can > document it as such and reject any attempts to use it for other > purposes. When a depends-on property is present in a device node, that > specific device's supplier list will be parsed ONLY from the > depends-on property and the other properties won't be parsed for > deriving dependency information for that device. > > Frank, Greg and I like this usage model for a new depends-on DT > binding. Is this something you'd be willing to accept? > > Thanks, > Saravana > > [1] - https://lore.kernel.org/lkml/20190731221721.187713-1-saravanak@google.com/ Friendly reminder. -Saravana ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Adding depends-on DT binding to break cyclic dependencies 2019-08-22 6:54 Adding depends-on DT binding to break cyclic dependencies Saravana Kannan 2019-08-27 20:18 ` Saravana Kannan @ 2019-08-29 16:28 ` Rob Herring 2019-08-30 4:58 ` Saravana Kannan 1 sibling, 1 reply; 8+ messages in thread From: Rob Herring @ 2019-08-29 16:28 UTC (permalink / raw) To: Saravana Kannan Cc: Mark Rutland, Greg Kroah-Hartman, Frank Rowand, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML, Android Kernel Team On Thu, Aug 22, 2019 at 1:55 AM Saravana Kannan <saravanak@google.com> wrote: > > Hi Rob, > > Frank, Greg and I got together during ELC and had an extensive and > very productive discussion about my "postboot supplier state cleanup" > patch series [1]. The three of us are on the same page now -- the > series as it stands is the direction we want to go in, with some minor > refactoring, documentation and naming changes. > > However, one of the things Frank is concerned about (and Greg and I > agree) in the current patch series is that the "cyclic dependency > breaking" logic has been pushed off to individual drivers using the > edit_links() callback. I would think the core can detect this condition. There's nothing device specific once you have the dependency tree. You still need to know what device needs to probe first and the drivers are going to have that knowledge anyways. So wouldn't it be enough to allow probe to proceed for devices in the loop. Once 1 driver succeeds, then you can enforce the dependencies on the rest. > The concern being, there are going to be multiple device specific ad > hoc implementations to break a cyclic dependency. Also, if a device > can be part of a cyclic dependency, the driver for that device has to > check for specific system/products in which the device is part of a > cyclic dependency (because it might not always be part of a cycle), > and then potentially have cycle/product specific code to break the > cycle (since the cycle can be different on each system/product). > > One way to avoid all of the device/driver specific code and simplify > my patch series by a non-trivial amount would be by adding a > "depends-on" DT binding that can ONLY be used to break cycles. We can > document it as such and reject any attempts to use it for other > purposes. When a depends-on property is present in a device node, that > specific device's supplier list will be parsed ONLY from the > depends-on property and the other properties won't be parsed for > deriving dependency information for that device. Seems like only ignoring the dependencies with a cycle would be sufficient. For example, consider a clock controller which has 2 clock inputs from other clock controllers where one has a cycle and one doesn't. Also consider it has a regulator dependency. We only need to ignore the dependency for 1 of the clock inputs. The rest of the dependencies should be honored. > Frank, Greg and I like this usage model for a new depends-on DT > binding. Is this something you'd be willing to accept? To do the above, it needs to be inverted. Convince me that cycles are really a problem anywhere besides clocks. I'd be more comfortable with a clock specific property if we only need it for clocks and I'm having a hard time imagining cycles for other dependencies. Rob ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Adding depends-on DT binding to break cyclic dependencies 2019-08-29 16:28 ` Rob Herring @ 2019-08-30 4:58 ` Saravana Kannan 2019-08-30 14:34 ` Rob Herring 0 siblings, 1 reply; 8+ messages in thread From: Saravana Kannan @ 2019-08-30 4:58 UTC (permalink / raw) To: Rob Herring Cc: Mark Rutland, Greg Kroah-Hartman, Frank Rowand, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML, Android Kernel Team On Thu, Aug 29, 2019 at 9:28 AM Rob Herring <robh+dt@kernel.org> wrote: > > On Thu, Aug 22, 2019 at 1:55 AM Saravana Kannan <saravanak@google.com> wrote: > > > > Hi Rob, > > > > Frank, Greg and I got together during ELC and had an extensive and > > very productive discussion about my "postboot supplier state cleanup" > > patch series [1]. The three of us are on the same page now -- the > > series as it stands is the direction we want to go in, with some minor > > refactoring, documentation and naming changes. > > > > However, one of the things Frank is concerned about (and Greg and I > > agree) in the current patch series is that the "cyclic dependency > > breaking" logic has been pushed off to individual drivers using the > > edit_links() callback. > > I would think the core can detect this condition. There's nothing > device specific once you have the dependency tree. You still need to > know what device needs to probe first and the drivers are going to > have that knowledge anyways. So wouldn't it be enough to allow probe > to proceed for devices in the loop. The problem is that device links don't allow creating a cyclic dependency graph -- for good reasons. The last link in the cycle would be rejected. I don't think trying to change device link to allow cyclic links is the right direction to go in nor is it a can of worms I want to open. So, we'll need some other way of tracking the loop and then allowing only those devices in a loop to attempt probing even if their suppliers haven't probed. And then if a device ends up being in more than one loop, things could get even more complicated. And after one of the devices in the loop probes, we still need to somehow figure out the "bad" link and delete it so the last "good" link can be made before all the suppliers have their sync_state() called (because the "good" link hasn't been created yet). That all gets pretty messy. If we are never going to accept any DT changes, then I'd rather go with edit_links() and keep the complexity within the one off weird hardware where there are cycles instead of over complicating the driver core. > Once 1 driver succeeds, then you > can enforce the dependencies on the rest. > > > The concern being, there are going to be multiple device specific ad > > hoc implementations to break a cyclic dependency. Also, if a device > > can be part of a cyclic dependency, the driver for that device has to > > check for specific system/products in which the device is part of a > > cyclic dependency (because it might not always be part of a cycle), > > and then potentially have cycle/product specific code to break the > > cycle (since the cycle can be different on each system/product). > > > > One way to avoid all of the device/driver specific code and simplify > > my patch series by a non-trivial amount would be by adding a > > "depends-on" DT binding that can ONLY be used to break cycles. We can > > document it as such and reject any attempts to use it for other > > purposes. When a depends-on property is present in a device node, that > > specific device's supplier list will be parsed ONLY from the > > depends-on property and the other properties won't be parsed for > > deriving dependency information for that device. > > Seems like only ignoring the dependencies with a cycle would be > sufficient. No, we need to only ignore the "bad" dependency. We can't ignore all the dependencies in the cycle because that would cause the suppliers to clean up the hardware state before the consumers are ready. > For example, consider a clock controller which has 2 clock > inputs from other clock controllers where one has a cycle and one > doesn't. Also consider it has a regulator dependency. We only need to > ignore the dependency for 1 of the clock inputs. The rest of the > dependencies should be honored. Agreed. In this case, if the device used the depends-on property, it'll have to list the 1 clock controller and the regulator. > > Frank, Greg and I like this usage model for a new depends-on DT > > binding. Is this something you'd be willing to accept? > > To do the above, it needs to be inverted. I understand you are basically asking for a "does-not-depend-on" property (like a black list). But I think a whitelist on the rare occasions that we need to override would give more flexibility than a blacklist. It'll also mean we don't have to check every supplier with the entire black list each time. > Convince me that cycles are really a problem anywhere besides clocks. I wouldn't be surprised at all if interconnects end up with cyclic dependencies as support for more of them are added. > I'd be more comfortable with a clock specific property if we only need > it for clocks and I'm having a hard time imagining cycles for other > dependencies. I definitely don't want per-supplier type override. That's just making things too complicated and adding too many DT properties if we need to override more than clocks. -Saravana ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Adding depends-on DT binding to break cyclic dependencies 2019-08-30 4:58 ` Saravana Kannan @ 2019-08-30 14:34 ` Rob Herring 2019-08-31 0:32 ` Saravana Kannan 0 siblings, 1 reply; 8+ messages in thread From: Rob Herring @ 2019-08-30 14:34 UTC (permalink / raw) To: Saravana Kannan Cc: Mark Rutland, Greg Kroah-Hartman, Frank Rowand, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML, Android Kernel Team On Thu, Aug 29, 2019 at 11:58 PM Saravana Kannan <saravanak@google.com> wrote: > > On Thu, Aug 29, 2019 at 9:28 AM Rob Herring <robh+dt@kernel.org> wrote: > > > > On Thu, Aug 22, 2019 at 1:55 AM Saravana Kannan <saravanak@google.com> wrote: > > > > > > Hi Rob, > > > > > > Frank, Greg and I got together during ELC and had an extensive and > > > very productive discussion about my "postboot supplier state cleanup" > > > patch series [1]. The three of us are on the same page now -- the > > > series as it stands is the direction we want to go in, with some minor > > > refactoring, documentation and naming changes. > > > > > > However, one of the things Frank is concerned about (and Greg and I > > > agree) in the current patch series is that the "cyclic dependency > > > breaking" logic has been pushed off to individual drivers using the > > > edit_links() callback. > > > > I would think the core can detect this condition. There's nothing > > device specific once you have the dependency tree. You still need to > > know what device needs to probe first and the drivers are going to > > have that knowledge anyways. So wouldn't it be enough to allow probe > > to proceed for devices in the loop. > > The problem is that device links don't allow creating a cyclic > dependency graph -- for good reasons. The last link in the cycle would > be rejected. I don't think trying to change device link to allow > cyclic links is the right direction to go in nor is it a can of worms > I want to open. > > So, we'll need some other way of tracking the loop and then allowing > only those devices in a loop to attempt probing even if their > suppliers haven't probed. And then if a device ends up being in more > than one loop, things could get even more complicated. And after one > of the devices in the loop probes, we still need to somehow figure out > the "bad" link and delete it so the last "good" link can be made > before all the suppliers have their sync_state() called (because the > "good" link hasn't been created yet). That all gets pretty messy. If > we are never going to accept any DT changes, then I'd rather go with > edit_links() and keep the complexity within the one off weird hardware > where there are cycles instead of over complicating the driver core. If it is so complex, then it should not be in DT. Things like describing clocks at the gate/mux/divider level turned out to be too complex to get right or complete, so we avoid that. > > Once 1 driver succeeds, then you > > can enforce the dependencies on the rest. > > > > > The concern being, there are going to be multiple device specific ad > > > hoc implementations to break a cyclic dependency. Also, if a device > > > can be part of a cyclic dependency, the driver for that device has to > > > check for specific system/products in which the device is part of a > > > cyclic dependency (because it might not always be part of a cycle), > > > and then potentially have cycle/product specific code to break the > > > cycle (since the cycle can be different on each system/product). > > > > > > One way to avoid all of the device/driver specific code and simplify > > > my patch series by a non-trivial amount would be by adding a > > > "depends-on" DT binding that can ONLY be used to break cycles. We can > > > document it as such and reject any attempts to use it for other > > > purposes. When a depends-on property is present in a device node, that > > > specific device's supplier list will be parsed ONLY from the > > > depends-on property and the other properties won't be parsed for > > > deriving dependency information for that device. > > > > Seems like only ignoring the dependencies with a cycle would be > > sufficient. > > No, we need to only ignore the "bad" dependency. We can't ignore all > the dependencies in the cycle because that would cause the suppliers > to clean up the hardware state before the consumers are ready. I misunderstood. I now see this is back to duplicating all the data that's already there which I've already said no to. > > For example, consider a clock controller which has 2 clock > > inputs from other clock controllers where one has a cycle and one > > doesn't. Also consider it has a regulator dependency. We only need to > > ignore the dependency for 1 of the clock inputs. The rest of the > > dependencies should be honored. > > Agreed. In this case, if the device used the depends-on property, > it'll have to list the 1 clock controller and the regulator. > > > > Frank, Greg and I like this usage model for a new depends-on DT > > > binding. Is this something you'd be willing to accept? > > > > To do the above, it needs to be inverted. > > I understand you are basically asking for a "does-not-depend-on" > property (like a black list). But I think a whitelist on the rare > occasions that we need to override would give more flexibility than a > blacklist. It'll also mean we don't have to check every supplier with > the entire black list each time. So if we have 10 dependencies and 1 to ignore, we're going to list the 9 dependencies? And if I want to know where the cycle is, I have to figure out which 1 of the 10 dependencies you omitted. Pick black or white list with what's going to be shortest in the common case. Also, when new dependencies are added to a node, we'd have to check that the dependency is added to 'depends-on' (or was it not on purpose). > > Convince me that cycles are really a problem anywhere besides clocks. > > I wouldn't be surprised at all if interconnects end up with cyclic > dependencies as support for more of them are added. Perhaps, though I'm doubtful the drivers for them could be both required at probe and built as modules. > > I'd be more comfortable with a clock specific property if we only need > > it for clocks and I'm having a hard time imagining cycles for other > > dependencies. > > I definitely don't want per-supplier type override. That's just making > things too complicated and adding too many DT properties if we need to > override more than clocks. I'm all for generic properties, but not if we only have hypothetical cases for anything beyond clocks. I know clocks are a somewhat common problem because it has come up before. I'm just asking for specific example that's not clocks. Part of the problem is we can't really police how a generic property is used. Maybe Linux is only using it for cycles, but then u-boot folks may think it works well for other reasons. Maybe a dtc check could mitigate that. Warn on any dependencies that are not a cycle. Actually, you could add a check for any cycles first and then we can see where all they exist. Rob ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Adding depends-on DT binding to break cyclic dependencies 2019-08-30 14:34 ` Rob Herring @ 2019-08-31 0:32 ` Saravana Kannan 2019-08-31 5:01 ` Saravana Kannan 0 siblings, 1 reply; 8+ messages in thread From: Saravana Kannan @ 2019-08-31 0:32 UTC (permalink / raw) To: Rob Herring Cc: Mark Rutland, Greg Kroah-Hartman, Frank Rowand, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML, Android Kernel Team On Fri, Aug 30, 2019 at 7:35 AM Rob Herring <robh+dt@kernel.org> wrote: > > On Thu, Aug 29, 2019 at 11:58 PM Saravana Kannan <saravanak@google.com> wrote: > > > > On Thu, Aug 29, 2019 at 9:28 AM Rob Herring <robh+dt@kernel.org> wrote: > > > > > > On Thu, Aug 22, 2019 at 1:55 AM Saravana Kannan <saravanak@google.com> wrote: > > > > > > > > Hi Rob, > > > > > > > > Frank, Greg and I got together during ELC and had an extensive and > > > > very productive discussion about my "postboot supplier state cleanup" > > > > patch series [1]. The three of us are on the same page now -- the > > > > series as it stands is the direction we want to go in, with some minor > > > > refactoring, documentation and naming changes. > > > > > > > > However, one of the things Frank is concerned about (and Greg and I > > > > agree) in the current patch series is that the "cyclic dependency > > > > breaking" logic has been pushed off to individual drivers using the > > > > edit_links() callback. > > > > > > I would think the core can detect this condition. There's nothing > > > device specific once you have the dependency tree. You still need to > > > know what device needs to probe first and the drivers are going to > > > have that knowledge anyways. So wouldn't it be enough to allow probe > > > to proceed for devices in the loop. > > > > The problem is that device links don't allow creating a cyclic > > dependency graph -- for good reasons. The last link in the cycle would > > be rejected. I don't think trying to change device link to allow > > cyclic links is the right direction to go in nor is it a can of worms > > I want to open. > > > > So, we'll need some other way of tracking the loop and then allowing > > only those devices in a loop to attempt probing even if their > > suppliers haven't probed. And then if a device ends up being in more > > than one loop, things could get even more complicated. And after one > > of the devices in the loop probes, we still need to somehow figure out > > the "bad" link and delete it so the last "good" link can be made > > before all the suppliers have their sync_state() called (because the > > "good" link hasn't been created yet). That all gets pretty messy. If > > we are never going to accept any DT changes, then I'd rather go with > > edit_links() and keep the complexity within the one off weird hardware > > where there are cycles instead of over complicating the driver core. > > If it is so complex, then it should not be in DT. I'm talking about existing simple DT bindings that says what clocks/interconnects/regulators a device needs. Not sure what you mean by "it should not be in DT". > Things like > describing clocks at the gate/mux/divider level turned out to be too > complex to get right or complete, so we avoid that. Right, and this patch series has nothing to do with describing complex internals of a device. > > > > Once 1 driver succeeds, then you > > > can enforce the dependencies on the rest. > > > > > > > The concern being, there are going to be multiple device specific ad > > > > hoc implementations to break a cyclic dependency. Also, if a device > > > > can be part of a cyclic dependency, the driver for that device has to > > > > check for specific system/products in which the device is part of a > > > > cyclic dependency (because it might not always be part of a cycle), > > > > and then potentially have cycle/product specific code to break the > > > > cycle (since the cycle can be different on each system/product). > > > > > > > > One way to avoid all of the device/driver specific code and simplify > > > > my patch series by a non-trivial amount would be by adding a > > > > "depends-on" DT binding that can ONLY be used to break cycles. We can > > > > document it as such and reject any attempts to use it for other > > > > purposes. When a depends-on property is present in a device node, that > > > > specific device's supplier list will be parsed ONLY from the > > > > depends-on property and the other properties won't be parsed for > > > > deriving dependency information for that device. > > > > > > Seems like only ignoring the dependencies with a cycle would be > > > sufficient. > > > > No, we need to only ignore the "bad" dependency. We can't ignore all > > the dependencies in the cycle because that would cause the suppliers > > to clean up the hardware state before the consumers are ready. > > I misunderstood. I now see this is back to duplicating all the data > that's already there which I've already said no to. What I proposed earlier was using depends-on for all devices. And I'm glad you rejected it because I like my current set of patches better than the earlier one. The question here is whether we can allow this for the rare occasions where there is a cycle. Frank, Greg and I think it was a reasonable compromise when we talked about it in person. > > > > For example, consider a clock controller which has 2 clock > > > inputs from other clock controllers where one has a cycle and one > > > doesn't. Also consider it has a regulator dependency. We only need to > > > ignore the dependency for 1 of the clock inputs. The rest of the > > > dependencies should be honored. > > > > Agreed. In this case, if the device used the depends-on property, > > it'll have to list the 1 clock controller and the regulator. > > > > > > Frank, Greg and I like this usage model for a new depends-on DT > > > > binding. Is this something you'd be willing to accept? > > > > > > To do the above, it needs to be inverted. > > > > I understand you are basically asking for a "does-not-depend-on" > > property (like a black list). But I think a whitelist on the rare > > occasions that we need to override would give more flexibility than a > > blacklist. It'll also mean we don't have to check every supplier with > > the entire black list each time. > > So if we have 10 dependencies and 1 to ignore, we're going to list the > 9 dependencies? And if I want to know where the cycle is, I have to > figure out which 1 of the 10 dependencies you omitted. Pick black or > white list with what's going to be shortest in the common case. Sure, but how often are we even going to encounter these? > Also, when new dependencies are added to a node, we'd have to check > that the dependency is added to 'depends-on' (or was it not on > purpose). Even if it's a "doesnt-depend-on" you'll still have that confusion of whether the new dependency should be added to that blacklist. > > > Convince me that cycles are really a problem anywhere besides clocks. > > > > I wouldn't be surprised at all if interconnects end up with cyclic > > dependencies as support for more of them are added. > > Perhaps, though I'm doubtful the drivers for them could be both > required at probe and built as modules. This is 100% true/possible on SDM845. They are needed for some consumers to write to registers. And both the consumer and the interconnect provider can be loaded as modules. In fact, some paths in the provider is left on from boot so that the CPU and other devices can reach memory. You asked for examples of where else there could be cycles, I gave you one. > > > I'd be more comfortable with a clock specific property if we only need > > > it for clocks and I'm having a hard time imagining cycles for other > > > dependencies. > > > > I definitely don't want per-supplier type override. That's just making > > things too complicated and adding too many DT properties if we need to > > override more than clocks. > > I'm all for generic properties, but not if we only have hypothetical > cases for anything beyond clocks. I know clocks are a somewhat common > problem because it has come up before. I'm just asking for specific > example that's not clocks. I gave you another example above and you are just dismissing it saying it probably can't be a module, etc. Not sure what more I can tell. > Part of the problem is we can't really police how a generic property > is used. Maybe Linux is only using it for cycles, but then u-boot > folks may think it works well for other reasons. Any property could be misused. We can't control downstream stuff or other projects. > Maybe a dtc check > could mitigate that. Warn on any dependencies that are not a cycle. > Actually, you could add a check for any cycles first and then we can > see where all they exist. Sorry, I'm not going to sign up to make dtc changes to be able to add DT bindings. -Saravana ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Adding depends-on DT binding to break cyclic dependencies 2019-08-31 0:32 ` Saravana Kannan @ 2019-08-31 5:01 ` Saravana Kannan 2020-02-20 7:03 ` Saravana Kannan 0 siblings, 1 reply; 8+ messages in thread From: Saravana Kannan @ 2019-08-31 5:01 UTC (permalink / raw) To: Rob Herring Cc: Mark Rutland, Greg Kroah-Hartman, Frank Rowand, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML, Android Kernel Team On Fri, Aug 30, 2019 at 5:32 PM Saravana Kannan <saravanak@google.com> wrote: > > On Fri, Aug 30, 2019 at 7:35 AM Rob Herring <robh+dt@kernel.org> wrote: > > > > On Thu, Aug 29, 2019 at 11:58 PM Saravana Kannan <saravanak@google.com> wrote: > > > > > > On Thu, Aug 29, 2019 at 9:28 AM Rob Herring <robh+dt@kernel.org> wrote: > > > > > > > > On Thu, Aug 22, 2019 at 1:55 AM Saravana Kannan <saravanak@google.com> wrote: > > > > > > > > > > Hi Rob, > > > > > > > > > > Frank, Greg and I got together during ELC and had an extensive and > > > > > very productive discussion about my "postboot supplier state cleanup" > > > > > patch series [1]. The three of us are on the same page now -- the > > > > > series as it stands is the direction we want to go in, with some minor > > > > > refactoring, documentation and naming changes. > > > > > > > > > > However, one of the things Frank is concerned about (and Greg and I > > > > > agree) in the current patch series is that the "cyclic dependency > > > > > breaking" logic has been pushed off to individual drivers using the > > > > > edit_links() callback. > > > > > > > > I would think the core can detect this condition. There's nothing > > > > device specific once you have the dependency tree. You still need to > > > > know what device needs to probe first and the drivers are going to > > > > have that knowledge anyways. So wouldn't it be enough to allow probe > > > > to proceed for devices in the loop. > > > > > > The problem is that device links don't allow creating a cyclic > > > dependency graph -- for good reasons. The last link in the cycle would > > > be rejected. I don't think trying to change device link to allow > > > cyclic links is the right direction to go in nor is it a can of worms > > > I want to open. > > > > > > So, we'll need some other way of tracking the loop and then allowing > > > only those devices in a loop to attempt probing even if their > > > suppliers haven't probed. And then if a device ends up being in more > > > than one loop, things could get even more complicated. And after one > > > of the devices in the loop probes, we still need to somehow figure out > > > the "bad" link and delete it so the last "good" link can be made > > > before all the suppliers have their sync_state() called (because the > > > "good" link hasn't been created yet). That all gets pretty messy. If > > > we are never going to accept any DT changes, then I'd rather go with > > > edit_links() and keep the complexity within the one off weird hardware > > > where there are cycles instead of over complicating the driver core. > > > > If it is so complex, then it should not be in DT. > > I'm talking about existing simple DT bindings that says what > clocks/interconnects/regulators a device needs. Not sure what you mean > by "it should not be in DT". > > > Things like > > describing clocks at the gate/mux/divider level turned out to be too > > complex to get right or complete, so we avoid that. > > Right, and this patch series has nothing to do with describing complex > internals of a device. > > > > > > > Once 1 driver succeeds, then you > > > > can enforce the dependencies on the rest. > > > > > > > > > The concern being, there are going to be multiple device specific ad > > > > > hoc implementations to break a cyclic dependency. Also, if a device > > > > > can be part of a cyclic dependency, the driver for that device has to > > > > > check for specific system/products in which the device is part of a > > > > > cyclic dependency (because it might not always be part of a cycle), > > > > > and then potentially have cycle/product specific code to break the > > > > > cycle (since the cycle can be different on each system/product). > > > > > > > > > > One way to avoid all of the device/driver specific code and simplify > > > > > my patch series by a non-trivial amount would be by adding a > > > > > "depends-on" DT binding that can ONLY be used to break cycles. We can > > > > > document it as such and reject any attempts to use it for other > > > > > purposes. When a depends-on property is present in a device node, that > > > > > specific device's supplier list will be parsed ONLY from the > > > > > depends-on property and the other properties won't be parsed for > > > > > deriving dependency information for that device. > > > > > > > > Seems like only ignoring the dependencies with a cycle would be > > > > sufficient. > > > > > > No, we need to only ignore the "bad" dependency. We can't ignore all > > > the dependencies in the cycle because that would cause the suppliers > > > to clean up the hardware state before the consumers are ready. > > > > I misunderstood. I now see this is back to duplicating all the data > > that's already there which I've already said no to. > > What I proposed earlier was using depends-on for all devices. And I'm > glad you rejected it because I like my current set of patches better > than the earlier one. The question here is whether we can allow this > for the rare occasions where there is a cycle. Frank, Greg and I think > it was a reasonable compromise when we talked about it in person. > > > > > > > For example, consider a clock controller which has 2 clock > > > > inputs from other clock controllers where one has a cycle and one > > > > doesn't. Also consider it has a regulator dependency. We only need to > > > > ignore the dependency for 1 of the clock inputs. The rest of the > > > > dependencies should be honored. > > > > > > Agreed. In this case, if the device used the depends-on property, > > > it'll have to list the 1 clock controller and the regulator. > > > > > > > > Frank, Greg and I like this usage model for a new depends-on DT > > > > > binding. Is this something you'd be willing to accept? > > > > > > > > To do the above, it needs to be inverted. > > > > > > I understand you are basically asking for a "does-not-depend-on" > > > property (like a black list). But I think a whitelist on the rare > > > occasions that we need to override would give more flexibility than a > > > blacklist. It'll also mean we don't have to check every supplier with > > > the entire black list each time. > > > > So if we have 10 dependencies and 1 to ignore, we're going to list the > > 9 dependencies? And if I want to know where the cycle is, I have to > > figure out which 1 of the 10 dependencies you omitted. Pick black or > > white list with what's going to be shortest in the common case. > > Sure, but how often are we even going to encounter these? > > > Also, when new dependencies are added to a node, we'd have to check > > that the dependency is added to 'depends-on' (or was it not on > > purpose). > > Even if it's a "doesnt-depend-on" you'll still have that confusion of > whether the new dependency should be added to that blacklist. > > > > > Convince me that cycles are really a problem anywhere besides clocks. > > > > > > I wouldn't be surprised at all if interconnects end up with cyclic > > > dependencies as support for more of them are added. > > > > Perhaps, though I'm doubtful the drivers for them could be both > > required at probe and built as modules. > > This is 100% true/possible on SDM845. They are needed for some > consumers to write to registers. And both the consumer and the > interconnect provider can be loaded as modules. In fact, some paths in > the provider is left on from boot so that the CPU and other devices > can reach memory. You asked for examples of where else there could be > cycles, I gave you one. > > > > > I'd be more comfortable with a clock specific property if we only need > > > > it for clocks and I'm having a hard time imagining cycles for other > > > > dependencies. > > > > > > I definitely don't want per-supplier type override. That's just making > > > things too complicated and adding too many DT properties if we need to > > > override more than clocks. > > > > I'm all for generic properties, but not if we only have hypothetical > > cases for anything beyond clocks. I know clocks are a somewhat common > > problem because it has come up before. I'm just asking for specific > > example that's not clocks. > > I gave you another example above and you are just dismissing it saying > it probably can't be a module, etc. Not sure what more I can tell. > > > Part of the problem is we can't really police how a generic property > > is used. Maybe Linux is only using it for cycles, but then u-boot > > folks may think it works well for other reasons. > > Any property could be misused. We can't control downstream stuff or > other projects. > > > Maybe a dtc check > > could mitigate that. Warn on any dependencies that are not a cycle. > > Actually, you could add a check for any cycles first and then we can > > see where all they exist. > > Sorry, I'm not going to sign up to make dtc changes to be able to add > DT bindings. Thinking more about this, I don't think it's worth holding back the entire series trying to solve the cyclic dependencies. At least as of today, the clock framework has a hack (hack because it expects globally unique clock names that causes problems for some corner cases) that allows the supplier (the one that gets a few clocks from the consumers) to get away with not listing the consumer clocks in the supplier's clock list. So, the SDM845 avoid cycles in DT (despite having them in the hardware) that way. So we can take our time trying to solve this in a generic fashion (new DT property/binding, edit_links(), letting devices probe, etc). In the meantime, maybe we'll run into more cycle issues that'll give us a better idea of which solution would be better as a generic solution. I'll send a new patch series addressing the concern about platfom_device and dropping the attempt to solve cycles. Thanks, Saravana ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Adding depends-on DT binding to break cyclic dependencies 2019-08-31 5:01 ` Saravana Kannan @ 2020-02-20 7:03 ` Saravana Kannan 0 siblings, 0 replies; 8+ messages in thread From: Saravana Kannan @ 2020-02-20 7:03 UTC (permalink / raw) To: Rob Herring Cc: Mark Rutland, Greg Kroah-Hartman, Frank Rowand, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML, Android Kernel Team On Fri, Aug 30, 2019 at 10:01 PM Saravana Kannan <saravanak@google.com> wrote: > > So we can take our time trying to solve this in a generic fashion (new > DT property/binding, edit_links(), letting devices probe, etc). In the > meantime, maybe we'll run into more cycle issues that'll give us a > better idea of which solution would be better as a generic solution. Mainly reviving an old thread to say this to Rob and Frank: Thanks for pushing back on "depends-on" and asking me to use the existing bindings instead. Saved a whole bunch of time when I actually tried to use of_devlink. Didn't have to add stupid "depends-on" for all the existing dependencies. But then I've also been meaning to send an RFC for this following, so rolling it into the same email. Thanks for also pushing back on all the earlier "meh" solutions for solving the cyclic dependency issue. I think I have a pretty good proposal now. While trying to solve the "dependencies of child nodes need to be proxied by the parents till the child devices are created" problem, I ended up having to add a "SYNC_STATE_ONLY" device link flag that treats those dependencies as "optional for probing". It also allows cycles (because it only affects sync state behavior). Also, dependencies of child nodes (whether they are actually devices or not) are always treated as "optional for probe" dependencies by of_devlink. So, how does this affect cyclic dependencies? Obviously, when two devices have cyclic dependencies, they don't have cyclic probe dependencies. Then they'd never probe even if of_devlink is not in the picture. At least one of the dependencies is only relevant for some "post-probe" functionality. So let's take a simple example: dev_a: device-a@xxxx { compatible = "fizzbuzz"; } dev_b: device-b@yyyy { compatible = "fizzbazz"; supplier-property-1 = <&dev_a>; supplier-property-2 = <&dev_c>; } dev_c: device-c@zzzz { compatible = "fizzfizz"; supplier-property-1 = <&dev_a>; supplier-property-3 = <&dev_b>; } Let's say dev_c only doesn't depend on dev_b for probing but needs it only for some functionality "foo" (Eg: thermal management, secure video playback, etc. Nothing OS specific). If the DT nodes are written as above, then there'll be a cycle with of_devlink and neither dev_b or dev_c will probe. However, if we can write dev_c DT as: dev_c: device-c@zzzz { compatible = "fizzfizz"; supplier-property-1 = <&dev_a>; foo { /* No compatible property */ supplier-property-2 = <&dev_b>; } } Then of_devlink will automatically treat dev_b as an optional requirement for dev_c. I think this is also nice from a DT perspective because it gives a clear representation of the dependency without really breaking or adding any DT rules. If you need some DT bindings only for a subset functionality, just list them under a child node with a meaningful name for that functionality. For this to work, the framework that supports "supplier-property-2" will have to add APIs to "get" the supplier by passing a DT node (instead of just struct device), but: 1) That is already supported by quite a few frameworks. 2) That shouldn't be too hard to add where necessary. And the driver needs to handle the child node explicitly (kinda obvious). Thoughts? Like the proposal? -Saravana ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-02-20 7:03 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-22 6:54 Adding depends-on DT binding to break cyclic dependencies Saravana Kannan 2019-08-27 20:18 ` Saravana Kannan 2019-08-29 16:28 ` Rob Herring 2019-08-30 4:58 ` Saravana Kannan 2019-08-30 14:34 ` Rob Herring 2019-08-31 0:32 ` Saravana Kannan 2019-08-31 5:01 ` Saravana Kannan 2020-02-20 7:03 ` Saravana Kannan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).