On Fri, Aug 5, 2016 at 3:37 PM, Rob Herring <robh@xxxxxxxxxx> wrote: > On Fri, Aug 5, 2016 at 7:46 AM, Vladimir Zapolskiy > <vladimir_zapolskiy@xxxxxxxxxx> wrote: >> Hi John, >> >> On 08/04/2016 02:05 AM, John Stultz wrote: >>> >>> Now that Andy's reboot reason core driver has landed, I wanted >>> to resubmit a reworked version of my SRAM based reboot reason >>> driver. >>> >>> This allows the kernel to communicate to the bootloader what mode >>> it should reboot to using some reserved memory. >>> >>> Feedback would be very much appreciated! >> >> >> in my opinion the taken approach is wrong, and I've already explained >> why and how to rework your driver to shrink the change, please see >> https://lkml.org/lkml/2016/1/27/133 >> >> In this case I think that a SRAM device node should just contain >> a plain description of partitions, compatible = "sram-reboot-mode" is >> clearly not a device on "SRAM bus", it is not a device at all, so >> please let's separate policy from mechanism > > Having a 2nd node for the driver is still not a device on a bus. It > adds unneeded complexity to the binding IMO. > > The current approach also follows the model ramoops is using. Right > now it's using reserved-memory, but that could easily be extended to > SRAM region as well. > >> Because my proposed alternative approach separates policy from >> mechanism, it for instanse allows to avoid overlappings on SRAM areas, >> and still other drivers may serve as consumers of partitions on SRAM. > > You could still have multiple consumers and having a compatible string > doesn't necessarily imply a driver. Though multiple consumers without > something arbitrating access sounds like broken design to me. So after running into some issues implementing the feedback that Bjorn suggested, I realized we were going to need to not only extend the sram driver to probe children, but we'd also have to make it a mfd so it wouldn't reserve the entire range and the reboot reason driver could map the memory. That on top of the fact that we're already duplicating much of the syscon-reboot-mode driver to work on sram, I decided to just start over and use the syscon driver, which works fine here. All that is needed is just adding it to the dts. I know that its not exactly correct usage of the syscon driver, but it starts to feel crazy almost completely duplicating the syscon driver just to have it named sram. thanks -john -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
