Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1030317AbcJ0UvW (ORCPT ); Thu, 27 Oct 2016 16:51:22 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56310 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751152AbcJ0UvQ (ORCPT ); Thu, 27 Oct 2016 16:51:16 -0400 Subject: Re: [RFC PATCH 0/5] Add an overlay manager to handle board capes To: Rob Herring References: <20161026145756.21689-1-antoine.tenart@free-electrons.com> <4ca9db09-e52c-11ec-133b-8f193b9b7174@redhat.com> Cc: Antoine Tenart , Maxime Ripard , Pantelis Antoniou , Mark Rutland , Stephen Boyd , Thomas Petazzoni , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , "devicetree@vger.kernel.org" , Frank Rowand , Dmitry Shmidt From: Hans de Goede Message-ID: Date: Thu, 27 Oct 2016 22:51:09 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Thu, 27 Oct 2016 20:51:16 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6409 Lines: 166 Hi, On 27-10-16 19:30, Rob Herring wrote: > On Thu, Oct 27, 2016 at 10:13 AM, Hans de Goede wrote: >> Hi, >> >> On 27-10-16 15:41, Rob Herring wrote: >>> >>> Please Cc the maintainers of drivers/of/. >>> >>> + Frank R, Hans, Dmitry S >>> >>> On Wed, Oct 26, 2016 at 9:57 AM, Antoine Tenart >>> wrote: >>>> >>>> Hi all, >>>> >>>> Many boards now come with dips and compatible capes; among others the >>>> C.H.I.P, or Beaglebones. All these boards have a kernel implementing an >>>> out-of-tree "cape manager" which is used to detected capes, retrieve >>>> their description and apply a corresponding overlay. This series is an >>>> attempt to start a discussion, with an implementation of such a manager >>>> which is somehow generic (i.e. formats or cape detectors can be added). >>>> Other use cases could make use of this manager to dynamically load dt >>>> overlays based on some input / hw presence. >>> >>> >>> I'd like to see an input source be the kernel command line and/or a DT >>> chosen property. Another overlay manager was proposed not to long >>> ago[1] as well. There's also the Allwinner tablet use case from Hans >>> where i2c devices are probed and detected. That's not using overlays >>> currently, but maybe could. >> >> >> Actually I'm currently thinking in a different direction, which I >> think will be good for the boards where some ICs are frequently >> replaced by 2nd (and 3th and 4th) sources, rather then that we're >> dealing with an extension connector with capes / daughter boards. >> >> Although there is some overlap I'm starting to think that we need to >> treat these 2 cases differently. Let me quickly copy and paste >> the basic idea I've for the 2nd source touchscreen / accelerometer >> chip case: >> >> """ >> The kernel actually already has a detect() method in struct i2c_driver, >> we could use that (we would need to implement it in drivers which do not >> have it yet). Note on second thought it seems it may be better to use >> probe() for this, see below. >> >> Then we could have something like this in dt: >> >> &i2c0 { >> touchscreen1: gsl1680@40 { >> reg = <0x40>; >> compatible = "silead,gsl1680"; >> enable-gpios = <&pio 7 1 GPIO_ACTIVE_HIGH>; /* PH1 */ >> status = "disabled"; >> }; >> >> touchscreen2: ektf2127@15 { >> reg = <0x15>; > > Do you ever have different devices with the same address? That would > be somewhat problematic as really these should be > "touchscreen@". Yes that happens (sometimes). > >> compatible = "elan,ektf2127"; >> enable-gpios = <&pio 7 1 GPIO_ACTIVE_HIGH>; /* PH1 */ >> status = "disabled"; >> }; >> >> i2c-probe-stop-at-first-match-0 = <&touchscreen1>, <&touchscreen2>; >> i2c-probe-stop-at-first-match-1 = <&accelerometer1>, <&accelerometer2>; >> } >> >> Which would make the i2c subsys call detect (*) on each device, until >> a device is found. Likewise we could have a "i2c-probe-all" property >> which also walks a list of phandles but does not stop on the first >> match. >> >> ... >> >> *) Yes this sounds Linux specific, but it really is just "execute >> to-be-probed >> device compatible specific detection method" >> """ > > Yeah, not a fan of these properties at first glance. Why can't you > just fail probe on the non-existent devices? That is possible and in the other thread on this there are some links to some boards which actually already do this, but from a dt pov it feels wrong. If we know only one of a set of options will ever be there we ought to describe things like this in the dt. Functionality wise this has 2 advantages: 1) We stop probing needlessly once a device is found, in some cases the majority of the board variants has dev a, and some have dev b / c. Then putting a first in the to-probe list will save probing b / c on most boards. 2) Not all i2c chips are easily identifiable, so in some cases one may want to put dev x as last to probe, because the probe solely consists of: "Does something ack i2c transfers at this address". >> This does not 100% solve all q8 issues (see the "Add Allwinner Q8 tablets >> hardware manager" thread), but does solve quite a bit of the use-case >> and this matches what many vendor os-images (typically android) are >> actually doing for these kind of boards. > > BTW, I've been meaning to ask you if you are looking at the Android > side of things as well? No, I purely use android os images / SDKs as a source of how the hw works, I do not have any intentions to try and get android up and running with mainline on these boards. >> As for the bits this does not solve, those are mostly board specific details >> which cannot be probed at all, and on x86 are typically solved in the device >> driver by doing a dmi check to identify the board and then apply a board >> specific workaround in the driver. >> >> I've come to believe that we should similarly delegate dealing this to >> device >> drivers in the devicetree case. Note that dt should still of course fully >> describe the hardware for normal hardware, the driver would just need to >> care >> about weird board quirks in certain exceptions. > > Which is fine IMO, though I do think we should look at those cases > carefully to ensure they stay the exception. Ack. >> A more interesting problem here is that dt does not have something like >> DMI, there is the machine compatible, but that typically does not contain >> board revision info (where as DMI often does). I believe that this is >> actually something which should be fixed at the bootloader level >> have it prepend a new machine compatible which contains revision info. >> >> Hmm, if we make the bootloader prepend a new machine compatible which >> contains >> revision info, we could then trigger quirks on this and in some cases avoid >> the need for dealing with board quirks in the driver ... > > That would work. Board and chip versions both need better handling in > kernel IMO. > > QCom has a whole scheme around version numbering in compatible > strings. (Unfortunately, bootloaders only support their previous way > of doing things.) > >> Note this is all very specific to dealing with board (revision) variants, >> for add-ons having the bootloader add info to the machine compatible does >> not seem the right solution. > > Agreed. Regards, Hans