Hi Hans, > On Oct 26, 2016, at 14:46 , Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > > Hi, > > On 24-10-16 19:39, Mark Rutland wrote: >> On Fri, Oct 14, 2016 at 09:53:31AM +0200, Hans de Goede wrote: >>> Hi Rob, Mark, et al., >> >> Hi Hans, >> >> Apologies for the delay in replying to this. > > No worries, I believe that 1 week is actually a pretty good > turn around time, esp. directly after a conference. > Conferences are a deadline killer. Just got around taking a look. >> I'd like to be clear that I do understand that there is a problem that >> needs to be addressed here. However, I do not believe that the *current* >> in-kernel approach is correct. More on that below. > > Ok. > >>> Mark, I know that we discussed this at ELCE and you clearly indicated >>> that according to you this does not belong in the kernel. I was a bit >>> surprised by this part of the discussion. >>> >>> I had posted a RFC earlier and Rob had indicated that given that the q8 >>> tablets are a special case, as my code uses actual probing rather then some >>> pre-arranged id mechanism with say an eeprom, that doing this in a >>> non-generic manner would be ok for my special case. >> >> To some extent, Rob and I may have differing views here; I'm not >> entirely sure what Rob's view is, and I cannot talk on his behalf. I >> certainly must apologise for having not commented on said RFC, however. >> >>> So on to why I believe that the kernel is the best place to do this, at least >>> for my special use-case: >>> >>> 1. Configurability >>> >>> Since the q8 tablets do not have any id mechanism I'm probing i2c busses to >>> generate i2c client dt nodes with the right address and compatible. >>> Some info in these nodes (e.g. touchscreen firmware-name) is not probable at >>> all and gets set by heuristics. This heuristics may get things wrong. >>> So my current implementation offers kernel cmdline options to override this. >> >> As I mentioned at ELCE, one major concern I have with this approach is >> this probing, which in part relies on a collection of heuristics. > > This is quite use-case specific, anyways, the probing is a 2 step process: > > 1) Identify which hardware there is in the tablet, this is pretty > reliable, we only detect a fix set of known possible touchscreens > and accelerometers, at known addresses and almost all have an id > register to check. > > 2) Determine *defaults* for various none probable settings, like guessing > which firmware to load into the touchscreen controllers, as there are > at least 2 ways the gsl1680 is wired up on these tablets and this > requires 2 different firmware files. This uses heuristics, to, as said, > determine the defaults all of the non-probable bits are overidable > through config options (currently kernel module options). Getting these > wrong is not dangerous to the hardware, but will work in a non-functional > or misbehaving (wrong coordinates) touchscreen. > > Note with the models I've access to so far the heuristics score 100% > but I'm not sure how representative the 16 models I've access to are > (they are all different and have been bought over a span of multiple > years). > >> I'm worried that this is very fragile, and sets us up for having to >> maintain a much larger collection of heuristics and/or a database of >> particular boards in future. This is fragile, defeats much of the gain >> from DT. > > I understand your worries, as said I'm confident the actual probing > is safe and getting the heuristics wrong will result in misbehavior, > but not in any hardware damage or such. > >> Worse, this could be completely incompatible with some arbitrary board >> that comes by in future, > > I assume you mean an arbitrary q8 tablet here, as the probe code does > bind by board/machine compatible, so for a really arbitrary board > this code will never activate. > >> because the kernel assumed something that was >> not true, which it would not have done if things were explicitly >> described to the kernel. > > I understand your worry, but moving the probing code to say u-boot > will not change any of this, the kernel will get the explicit > description created by the u-boot probe code, but it would be > just as wrong. > > So maybe we need to answer 2 questions in a row: > > 1) Do we want such probe code at all ? > > My answer to this is yes, these (cheap) tablets are interesting to > e.g. the maker community and I would like them to run mainline > (and run mainline well), but given the way there are put together > this require some code to dynamically adapt to the batch of the > month somewhere > > 2) Where do we put this code ? > > If we agree on 1 (I assume we do) then this becomes the real > question, at which point your worries about the kernel assuming > something which is not true because the probe code got it wrong > may become true regardless where the code lives. > > So wrt this worries is all I can do is ask you to trust me to > not mess things up, just like we all trust driver authors, etc. > all the time to not mess things up. > >> As I mentioned at ELCE, I'm not opposed to the concept of the kernel >> applying overlays or fixups based on a well-defined set of criteria; >> having some of that embedded in the DT itself would be remarkably >> helpful. However, I am very much not keen on loosely defined criteria as >> with here, as it couples the DT and kernel, and creates problems longer >> term, as described above. > > Right, so again I think we need to split the discussion in 2 steps: > > 1) How do we apply the fixups, currently I'm using free-form changes > done from C-code. I can envision moving to something like the quirk > mechanism suggested by Pantelis in the past. Note this is not a perfect > fit for my rather corner-case use-case, but I can understand that in > general you want the variants to be described in dt, and activated > in some way, rather then have c-code make free-form changes to the dt > We’ve had this discussion before, so I guess here it goes again. I think the biggest objection is the programmatic way of applying every quirk by ‘hand’. If there was a way to keep the probing mechanism but just spit out a ‘model’ number we could reasonably map it to an overlay to apply with a generic overlay manager. >From an internal s/w standpoint having an expansion board or soldered parts makes no difference. > 2) How do we select which fixups to apply. Again I can understand > you wanting some well defined mechanism for this, but again my > use-case is special and simply does not allow for this as there > is no id-eeprom to read or some such. > Yes there is no EEPROM but you might be able to map probing results to a fake ‘model’ number. Let me expand a bit: Assume that you have a number of probing steps, for example A, B, C each returning true or false, and C being executed only when B is ‘true’ you could do this to generate a bit field that identifies it. For example let’s assume that model FOO’s probing steps are A false, B true, C false -> 010 Model’s BAR A true, B false, C don’t care -> 10x Mapping these to models could be Model FOO, (010 & 111) == 010 (all three probing steps must match) Model BAR, (10x & 110) = 100 (the first two probing steps must match) >>> Although having to specify kernel cmdline options is not the plug and play >>> experience I want to give end-users most distros do have documentation on >>> how to do this and doing this is relatively easy for end-users. Moving this >>> to the bootloader means moving to some bootloader specific config mechanism >>> which is going to be quite hard (and possibly dangerous) for users to use. >> >> I have to ask, why is it more dangerous? > > Because for normal end users meddling with the bootloader / with u-boot's > environment is like flashing a PC BIOS. Most (non technical) end users will > want to install u-boot once (or not at all) and then just have it work. > Users do *not* want to deal with the bootloader. They don’t even want to know that it’s there. As far they're concerned if you need to drop in the bootloader to do something -> broken. >> Perhaps more difficult, but that can be solved, > > More difficult means not doable for many users. > +1 >> if the manual >> corrections are simply a set of options to be passed to the kernel, I >> don't see why the bootloader cannot pick this up. >> >>> 2. Upgradability >>> >>> Most users treat the bootloader like they treat an x86 machine BIOS/EFI, >>> they will never upgrade it unless they really have to. This means that it >>> will be very difficult to get users to actual benefit from bug-fixes / >>> improvements done to the probing code. Where as the kernel on boards running >>> e.g. Debian or Fedora gets regular updates together with the rest of the >>> system. >> >> Given that DTBs are supposed to remain supported, users should find >> themselves with a system that continues to work, but may not have all >> the bells and whistles it could, much like elsewhere. >> >> While it's true that we have issues in this area, I don't think that's >> an argument for putting things into the kernel for this specific set of >> boards. > > It is an argument to put much of the dynamic (dt) hardware support in > the kernel in general. > >>> 3. Infrastructure >>> >>> The kernel simply has better infrastructure for doing these kind of things. >> >> At least on the DT front, a lot of work has gone into improving the >> infrastructure, e.g. the work that Free Electrons have done [1]. I >> appreciate that the infrastructure for things like poking SPI may not be >> as advanced. >> >> Which bits of infrastructure do you find lacking today? > > Nothing really specific (I've not yet tried porting the probe code > to u-boot), but I just find working within the kernel easier in general, > since there really just is a lot more infrastructure. Note I'm the > upstream u-boot maintainer for the allwinner SoC support, so this > is not due to me being unfamiliar with u-boot. > >>> Yes we could improve the bootloader, but the kernel is also improving >>> and likely at a faster rate. Besides that the purpose of the >>> bootloader is mostly to be simple and small, load the kernel and get >>> out of the way, not to be a general purpose os kernel. So it will >>> simply always have less infrastructure and that is a good thing, >>> otherwise we will be writing another general purpose os which is a >>> waste of effort. >> >> I think this conflates a number of details. Yes, we'd like firmware and >> bootloaders to be small, and yes, their infrastructure and feature >> support will be smaller than a general purpose OS. >> >> That doesn't imply that they cannot have features necessary to boostrap >> an OS. >> >> It's also not strictly necessary that the firmware or bootloader have >> the capability to do all this probing, as that could be contained in >> another part (e.g. a U-Boot application which is run once to detect the >> board details, logging this into a file). >> >> It's also possible to ship an intermediary stage (e.g. like the >> impedance matcher) which can be upgradeable independently of the kernel. > > Yes there are other solutions, but they all involve a lot more > moving pieces (and thus will break) then a single isolated .c file > in the kernel, which is all this series adds. > > Esp the intermediate solution just adds a ton of complexity with 0 > gain. Simple is the way to go. Putting things in the kernel is the simplest solution for the users, for the distro maintainers and the board support people. > >>> 4. This is not a new board file >>> >>> Mark, at ELCE I got the feeling that your biggest worry / objection is >>> that this will bring back board files, but that is not the case, if you >>> look at the actual code it is nothing like a board-file at all. Where a >>> board file was instantiating device after device after device from c-code, >>> with maybe a few if-s in there to deal with board revisions. This code is >>> all about figuring out which accelerometer and touchscreen there are, >>> to then add nodes to the dt for this 2 devices, almost all the code is >>> probing, the actual dt modifying code is tiny and no direct device >>> instantiation is done at all. >> >> Sorry, but I must disagree. This code: >> >> (a) identifies a set of boards based on a top-level compatible string. >> i.e. it's sole purpose is to handle those boards. >> >> (b) assumes non-general properties of those boards (e.g. that poking >> certain SPI endpoints is safe). >> >> (c) applies arbitrary properties to the DT, applying in-built knowledge >> of those boards (in addition to deep structural knowledge of the >> DTB in question). >> >> To me, given the implicit knowledge, that qualifies as a "board file". >> >> As I mentioned at ELCE, if this had no knowledge of the boards in >> question, I would be less concerned. e.g. if there was a well-defined >> identification mechanism, describe in the DT, with fixups also defined >> in the DT. > > And as I tried to explain before, for this specific use-case describing > all this board specific knowledge in a generic manner in dt is simply > impossible, unless we add a turing complete language to dt aka aml. > > I've a feeling that you're mixing this, rather special, use-case with > the more generic use-case of daughter-boards for various small-board-computers > I agree that for the SBC use-case it makes sense to try and come up with > a shared core / dt bindings for much of this. Note that even this boards > will still need a board (or board-family) specific method for getting > the actual id from a daughter-board, but this board specific code could > then pass the id to some more general hw-manager core which starts applying > dt changes based on the id. But this assumes there is a single id to > uniquely identify the extensions, which in my case there simply is not. > ^ read above. >>> 5. This is an exception, not the rule >>> >>> Yes this is introducing board (family of boards) specific c-code into the >>> kernel, so in a way it is reminiscent of board files. But sometimes this is >>> necessary, just look at all the vendor / platform specific code in the kernel >>> in drivers/platform/x86, or all the places where DMI strings are used to >>> uniquely identify a x86 board and adjust behavior. >>> >>> But this really is the exception, not the rule. I've written/upstreamed a >>> number of drivers for q8 tablets hardware and if you look at e.g. the >>> silead touchscreen driver then in linux-next this is already used for 5 >>> ARM dts files where no board specific C-code is involved at all. >>> >>> So this again is very different from the board file era, where C-code >>> had to be used to fill device specific platform-data structs, here all >>> necessary info is contained in the dt-binding and there are many users >>> who do not need any board specific C-code in the kernel at all. >>> >>> So dt is working as it should and is avoiding board specific C-code for >>> the majority of the cases. But sometimes hardware is not as we ideally >>> would like it to be; and for those *exceptions* we are sometimes going >>> to need C-code in the kernel, just like there is "board" specific C-code >>> in the x86 code. >> >> Your talk convinced me that we're both going to see more variation >> within this family of boards, and that we'll see more families of boards >> following a similar patter. Given that, I think that we need a more >> general solution, as I commented on the RFC. >> >> That doesn't necessarily mean that this can't happen in the kernel, but >> it certainly needs to be more strictly defined, e.g. with match criteria >> and fixups explicit in the DTB. > > The only answer I've to: "with match criteria and fixups explicit in the DTB" > is: ok, give my a turing complete language inside DTB then, anything else > will not suffice. So either we are doomed to reinvent ACPI; or we must > accept some board(family) specific C-code in the kernel. > Please don’t let DT stagnate. We don’t have to repeat mistakes and we don’t have to remain true to the spirit of what DT was supposed to be more than a decade ago. The problems we deal now are different, the industry is different and the users are different. We have to evolve along with them. > Regards, > > Hans Regards — Pantelis-- 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
