Add Allwinner Q8 tablets hardware manager

[Pantelis Antoniou]

Hi Hans,

> On Oct 26, 2016, at 14:46 , Hans de Goede <hdegoede at redhat.com> 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