* Re: [RFC PATCH 02/13] of: Remove excessive printks to reduce clutter
From: Frank Rowand @ 2016-10-27 16:04 UTC (permalink / raw)
To: Rob Herring
Cc: Pantelis Antoniou, Pantelis Antoniou, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <CAL_Jsq+TWZw+VcbYZp_xmFR-yitY8bn+gqe1Q2N42UFZAYsZKg@mail.gmail.com>
On 10/27/16 05:21, Rob Herring wrote:
> On Tue, Oct 25, 2016 at 3:58 PM, <frowand.list@gmail.com> wrote:
>> From: Frank Rowand <frank.rowand@am.sony.com>
>
> Maybe some should be debug?
>
>> Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
>> ---
>> drivers/of/resolver.c | 28 ----------------------------
>> 1 file changed, 28 deletions(-)
>>
>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
>> index 4ff0220d7aa2..93a7ca0bf98c 100644
>> --- a/drivers/of/resolver.c
>> +++ b/drivers/of/resolver.c
>> @@ -116,8 +116,6 @@ static int __of_adjust_phandle_ref(struct device_node *node,
>>
>> propval = kmalloc(rprop->length, GFP_KERNEL);
>> if (!propval) {
>> - pr_err("%s: Could not copy value of '%s'\n",
>> - __func__, rprop->name);
>> return -ENOMEM;
>> }
>
> I would remove the brackets in this patch rather than separately.
>
>> memcpy(propval, rprop->value, rprop->length);
> .
>
OK, I will collapse the "remove braces" patch into this patch.
^ permalink raw reply
* Re: [RFC PATCH 01/13] of: Remove comments that state the obvious
From: Frank Rowand @ 2016-10-27 16:02 UTC (permalink / raw)
To: Rob Herring
Cc: Pantelis Antoniou, Pantelis Antoniou,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <CAL_JsqL1aPq3rJLngvpQav-4-AKBd+u=+GX5NNpP9Kbc4zehpw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 10/27/16 05:18, Rob Herring wrote:
> On Tue, Oct 25, 2016 at 3:58 PM, <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> From: Frank Rowand <frank.rowand-mEdOJwZ7QcZBDgjK7y7TUQ@public.gmane.org>
>>
>> Remove comments that state the obvious, to reduce clutter
>
> I'm probably not the best reviewer, have you ever seen a comment in my code. :)
>
>>
>> Signed-off-by: Frank Rowand <frank.rowand-mEdOJwZ7QcZBDgjK7y7TUQ@public.gmane.org>
>> ---
>> drivers/of/resolver.c | 31 ++-----------------------------
>> 1 file changed, 2 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
>> index 46325d6394cf..4ff0220d7aa2 100644
>> --- a/drivers/of/resolver.c
>> +++ b/drivers/of/resolver.c
>
>> @@ -75,8 +73,6 @@ static phandle of_get_tree_max_phandle(void)
>>
>> /*
>> * Adjust a subtree's phandle values by a given delta.
>> - * Makes sure not to just adjust the device node's phandle value,
>> - * but modify the phandle properties values as well.
>
> What's missing here is the why? Why do we adjust phandle values?
Yes! I will add that.
>
>> */
>> static void __of_adjust_tree_phandles(struct device_node *node,
>> int phandle_delta)
>> @@ -85,32 +81,25 @@ static void __of_adjust_tree_phandles(struct device_node *node,
>> struct property *prop;
>> phandle phandle;
>>
>> - /* first adjust the node's phandle direct value */
>
> Seems somewhat useful.
ok
>
>> if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL)
>> node->phandle += phandle_delta;
>>
>> - /* now adjust phandle & linux,phandle values */
>
> Seems somewhat useful.
ok
>
>> for_each_property_of_node(node, prop) {
>>
>> - /* only look for these two */
>> if (of_prop_cmp(prop->name, "phandle") != 0 &&
>> of_prop_cmp(prop->name, "linux,phandle") != 0)
>> continue;
>>
>> - /* must be big enough */
>> if (prop->length < 4)
>> continue;
>>
>> - /* read phandle value */
>> phandle = be32_to_cpup(prop->value);
>> - if (phandle == OF_PHANDLE_ILLEGAL) /* unresolved */
>> + if (phandle == OF_PHANDLE_ILLEGAL)
>> continue;
>>
>> - /* adjust */
>> *(uint32_t *)prop->value = cpu_to_be32(node->phandle);
>> }
>>
>> - /* now do the children recursively */
>> for_each_child_of_node(node, child)
>> __of_adjust_tree_phandles(child, phandle_delta);
>> }
>
>> @@ -254,7 +239,6 @@ static int __of_adjust_tree_phandle_references(struct device_node *node,
>>
>> for (i = 0; i < count; i++) {
>> off = be32_to_cpu(((__be32 *)rprop->value)[i]);
>> - /* make sure the offset doesn't overstep (even wrap) */
>
> Seems somewhat useful.
Seems obvious to me, but if others disagree I will leave it. (I was somewhat
aggressive in removing comments).
>
>> if (off >= sprop->length ||
>> (off + 4) > sprop->length) {
>> pr_err("%s: Illegal property '%s' @%s\n",
>
>> @@ -349,10 +328,8 @@ int of_resolve_phandles(struct device_node *resolve)
>> resolve_sym = NULL;
>> resolve_fix = NULL;
>>
>> - /* this may fail (if no fixups are required) */
>
> Seem somewhat useful.
A later patch moves the "root_sym = ..." to just above the use of
root_sym. At that location a failed of_find_node_by_path() is a
real failure.
>
>> root_sym = of_find_node_by_path("/__symbols__");
>>
>> - /* locate the symbols & fixups nodes on resolve */
>> for_each_child_of_node(resolve, child) {
>>
>> if (!resolve_sym &&
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH V4 0/5] firmware: Add support for TI System Control Interface (TI-SCI) protocol driver
From: Kevin Hilman @ 2016-10-27 16:01 UTC (permalink / raw)
To: Tero Kristo
Cc: Nishanth Menon, Santosh Shilimkar, Dave Gerlach, Lokesh Vutla,
Andrew Davis, Russell King, Russ Dill, Sudeep Holla,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-pm-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Rob Herring
In-Reply-To: <2694e4b5-20d5-f590-c6a6-55ff4cb8c16b-l0cyMroinI0@public.gmane.org>
Tero Kristo <t-kristo-l0cyMroinI0@public.gmane.org> writes:
> On 19/10/16 02:08, Nishanth Menon wrote:
>> Version 4 of the series is basically a rebase to v4.9-rc1, no functional
>> changes.
>
> Any final comments on this series, or shall I send a pull-req forward?
> Very minimal changes compared to v3 so should be good to go imo.
Not a comment on the series, but a question on TI-SCI.
Will the uC firmware be open like it was on AM335x?
Kevin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v6 5/5] ARM: DTS: da850: Add usb phy node
From: David Lechner @ 2016-10-27 15:55 UTC (permalink / raw)
To: Sekhar Nori, Kevin Hilman, Rob Herring, Mark Rutland
Cc: Axel Haslam, Sergei Shtylyov, devicetree, linux-arm-kernel,
linux-kernel
In-Reply-To: <d55f19f2-beb3-a681-a6e9-c1d2c303173c@ti.com>
On 10/26/2016 05:26 AM, Sekhar Nori wrote:
> On Wednesday 26 October 2016 08:36 AM, David Lechner wrote:
>> Add a node for the new usb phy driver.
>
> changed this to:
>
> Add a node for usb phy device. This device
> controls both the USB 1.1 and USB 2.0 PHYs.
>
> mainly because the node is for the device, not the driver.
>
>>
>> Signed-off-by: David Lechner <david@lechnology.com>
>
> Applied to v4.10/dt
>
> Thanks,
> Sekhar
>
I found a better way to represent this device as a child of the syscon
node. How should we handle the change? Should I submit a new patch that
applies on top of this one or will you drop this patch and I should send
a new one to take it's place?
Assuming that you agree that this is better:
cfgchip: cfgchip@1417c {
compatible = "ti,da830-cfgchip", "syscon", "simple-mfd";
reg = <0x1417c 0x14>;
usb_phy: usb-phy {
compatible = "ti,da830-usb-phy";
#phy-cells = <1>;
status = "disabled";
};
};
Since the phy consists entirely as registers in the syscon device, we
should make it a child of the syscon device instead of a child of the
soc node.
^ permalink raw reply
* Re: Add Allwinner Q8 tablets hardware manager
From: Pantelis Antoniou @ 2016-10-27 15:52 UTC (permalink / raw)
To: Hans de Goede
Cc: Mark Rutland, Rob Herring, Arnd Bergmann, Greg Kroah-Hartman,
Maxime Ripard, Chen-Yu Tsai,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, devicetree
In-Reply-To: <4cfbedb2-0249-e881-3577-dda0b0d4cabe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Hi Hans,
> On Oct 26, 2016, at 14:46 , Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 4/5] ARM: davinci: enable LEDs default-on trigger in default config
From: David Lechner @ 2016-10-27 15:49 UTC (permalink / raw)
To: Sekhar Nori, Kevin Hilman
Cc: Rob Herring, Mark Rutland, Russell King,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <669b14c2-7f62-35bc-c8a3-6dde05a99fb1-l0cyMroinI0@public.gmane.org>
On 10/27/2016 06:29 AM, Sekhar Nori wrote:
> On Saturday 22 October 2016 12:06 AM, David Lechner wrote:
>> The LEDs default-on trigger is nice to have. For example, it can be used
>> to configure a LED as a power indicator.
>>
>> Signed-off-by: David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>
>> ---
>> arch/arm/configs/davinci_all_defconfig | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/arm/configs/davinci_all_defconfig b/arch/arm/configs/davinci_all_defconfig
>> index 9d7f0bc..e380743 100644
>> --- a/arch/arm/configs/davinci_all_defconfig
>> +++ b/arch/arm/configs/davinci_all_defconfig
>> @@ -181,6 +181,7 @@ CONFIG_LEDS_GPIO=m
>> CONFIG_LEDS_TRIGGERS=y
>> CONFIG_LEDS_TRIGGER_TIMER=m
>> CONFIG_LEDS_TRIGGER_HEARTBEAT=m
>> +CONFIG_LEDS_TRIGGER_DEFAULT_ON=y
>
> Can this be module like rest of the triggers? It will come on later, but
> not sure if you care about the difference that much. Having it a module
> will be better for those boards that don't need it.
>
> Thanks,
> Sekhar
>
Yes, module is OK here.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 2/3] drm: zte: add initial vou drm driver
From: Shawn Guo @ 2016-10-27 15:32 UTC (permalink / raw)
To: Gustavo Padovan, Shawn Guo
Cc: Mark Rutland, devicetree, Daniel Vetter, Baoyou Xie, Emil Velikov,
dri-devel, Rob Herring, Jun Nie, linux-arm-kernel
In-Reply-To: <20161020122958.GC10198@joana>
Hi Gustavo,
Thanks for looking at the patch.
> 2016-10-20 Shawn Guo <shawn.guo@linaro.org>:
>
> > It adds the initial ZTE VOU display controller DRM driver. There are
> > still some features to be added, like overlay plane, scaling, and more
> > output devices support. But it's already useful with dual CRTCs and
> > HDMI monitor working.
> >
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
<snip>
> > +static void zx_hdmi_connector_destroy(struct drm_connector *connector)
> > +{
> > + drm_connector_unregister(connector);
>
> drm_connector_unregister() is not needed anymore. DRM core will call it
> for you.
>
> > + drm_connector_cleanup(connector);
> > +}
> > +
> > +static const struct drm_connector_funcs zx_hdmi_connector_funcs = {
> > + .dpms = drm_atomic_helper_connector_dpms,
> > + .fill_modes = drm_helper_probe_single_connector_modes,
> > + .detect = zx_hdmi_connector_detect,
> > + .destroy = zx_hdmi_connector_destroy,
>
> Then here you can use drm_connector_cleanup() directly.
Okay, will do.
> > + .reset = drm_atomic_helper_connector_reset,
> > + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> > + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> > +};
<snip>
> > +static void zx_crtc_atomic_begin(struct drm_crtc *crtc,
> > + struct drm_crtc_state *state)
> > +{
> > + struct drm_pending_vblank_event *event = crtc->state->event;
> > +
> > + if (!event)
> > + return;
> > +
> > + crtc->state->event = NULL;
> > +
> > + spin_lock_irq(&crtc->dev->event_lock);
> > + if (drm_crtc_vblank_get(crtc) == 0)
> > + drm_crtc_arm_vblank_event(crtc, event);
> > + else
> > + drm_crtc_send_vblank_event(crtc, event);
> > + spin_unlock_irq(&crtc->dev->event_lock);
>
> I think you may want to send the vblank event to userspace after you
> commit your planes, and not before.
I guess you are suggesting that the code should be implemented in
.atomic_flush hook instead, right?
> > +}
> > +
> > +static const struct drm_crtc_helper_funcs zx_crtc_helper_funcs = {
> > + .enable = zx_crtc_enable,
> > + .disable = zx_crtc_disable,
> > + .atomic_begin = zx_crtc_atomic_begin,
> > +};
> > +
> > +static const struct drm_crtc_funcs zx_crtc_funcs = {
> > + .destroy = drm_crtc_cleanup,
> > + .set_config = drm_atomic_helper_set_config,
> > + .page_flip = drm_atomic_helper_page_flip,
> > + .reset = drm_atomic_helper_crtc_reset,
> > + .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
> > + .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
> > +};
> > +
> > +static int zx_crtc_init(struct drm_device *drm, struct zx_vou_hw *vou,
> > + enum vou_chn_type chn_type)
> > +{
> > + struct device *dev = vou->dev;
> > + struct zx_layer_data data;
> > + struct zx_crtc *zcrtc;
> > + int ret;
> > +
> > + zcrtc = devm_kzalloc(dev, sizeof(*zcrtc), GFP_KERNEL);
> > + if (!zcrtc)
> > + return -ENOMEM;
> > +
> > + zcrtc->vou = vou;
> > + zcrtc->chn_type = chn_type;
> > +
> > + if (chn_type == VOU_CHN_MAIN) {
> > + data.layer = vou->osd + MAIN_GL_OFFSET;
> > + data.csc = vou->osd + MAIN_CSC_OFFSET;
> > + data.hbsc = vou->osd + MAIN_HBSC_OFFSET;
> > + data.rsz = vou->otfppu + MAIN_RSZ_OFFSET;
> > + zcrtc->chnreg = vou->osd + OSD_MAIN_CHN;
> > + zcrtc->regs = &main_crtc_regs;
> > + zcrtc->bits = &main_crtc_bits;
> > + } else {
> > + data.layer = vou->osd + AUX_GL_OFFSET;
> > + data.csc = vou->osd + AUX_CSC_OFFSET;
> > + data.hbsc = vou->osd + AUX_HBSC_OFFSET;
> > + data.rsz = vou->otfppu + AUX_RSZ_OFFSET;
> > + zcrtc->chnreg = vou->osd + OSD_AUX_CHN;
> > + zcrtc->regs = &aux_crtc_regs;
> > + zcrtc->bits = &aux_crtc_bits;
> > + }
> > +
> > + zcrtc->pixclk = devm_clk_get(dev, (chn_type == VOU_CHN_MAIN) ?
> > + "main_wclk" : "aux_wclk");
> > + if (IS_ERR(zcrtc->pixclk)) {
> > + ret = PTR_ERR(zcrtc->pixclk);
> > + dev_err(dev, "failed to get pix clk: %d\n", ret);
>
> DRM_ERROR() - here and in other places in your patch
I will follow Sean's suggestion to use DRM_DEV_* for these messages.
Shawn
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [RFC PATCH 0/5] Add an overlay manager to handle board capes
From: Hans de Goede @ 2016-10-27 15:13 UTC (permalink / raw)
To: Rob Herring, Antoine Tenart
Cc: Maxime Ripard, Pantelis Antoniou, Mark Rutland, Stephen Boyd,
Thomas Petazzoni,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Frank Rowand,
Dmitry Shmidt
In-Reply-To: <CAL_JsqL9yWBj0yYE54XGi87YPGugGAACzr=CuW6dk5kk3EuyCA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
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
> <antoine.tenart-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> 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>;
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"
"""
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.
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.
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 ...
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.
Regards,
Hans
>
> Another thing to consider is different sources of overlays. Besides in
> the filesystem, overlays could be built into the kernel (already
> supported), embedded in the dtb (as the other overlay mgr did) or we
> could extend FDT format to append them.
>
>> The proposed design is a library which can be used by detector drivers
>> to parse headers and load the corresponding overlay. Helpers are
>> provided for this purpose. The whole thing is divided into 3 entities:
>>
>> - The parser which is project-specific (to allow supporting headers
>> already into the wild). It registers a function parsing an header's
>> data and filling one or more strings which will be used to find
>> matching dtbo on the fs.
>>
>> - The overlay manager helpers allowing to parse a header to retrieve
>> the previously mentioned strings and to load a compatible overlay.
>>
>> - The detectors which are used to detect capes and get their description
>> (to be parsed).
>
> What about things like power has to be turned on first to detect
> boards and read their ID? I think this needs to be tied into the
> driver model. Though, don't go sticking cape mgr nodes into DT. Maybe
> a driver gets bound to a connector node, but we've got to sort out
> connector bindings first.
>
>> An example of parser and detector is given, compatible with what's done
>> for the C.H.I.P. As the w1 framework is really bad (and we should
>> probably do something about that) the detector code is far from being
>> perfect; but that's not related to what we try to achieve here.
>>
>> The actual implementation has a limitation: the detectors cannot be
>> built-in the kernel image as they would likely detect capes at boot time
>> but will fail to get their corresponding dt overlays as the fs isn't
>> mounted yet. The only case this can work is when dt overlays are
>> built-in firmwares. This isn't an issue for the C.H.I.P. use case right
>> now. There was a discussion about making an helper to wait for the
>> rootfs to be mount but the answer was "this is the driver's problem".
>
> I thought there are firmware loading calls that will wait. I think
> this all needs to work asynchronously both for firmware loading and
> because w1 is really slow.
>
>> I'd like to get comments, specifically from people using custom cape
>> managers, to see if this could fill their needs (with I guess some
>> modifications).
>
> Having 2 would certainly give a better sense this is generic.
>
> Rob
>
> [1] https://patchwork.ozlabs.org/patch/667805/
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [alsa-devel] [linux-sunxi] [PATCH v5 4/7] ASoC: sunxi: Add sun8i I2S driver
From: Jean-Francois Moine @ 2016-10-27 15:13 UTC (permalink / raw)
To: Maxime Ripard
Cc: devicetree, Linux-ALSA, Dave Airlie, linux-sunxi, Liam Girdwood,
Rob Herring, Chen-Yu Tsai, Mark Brown, dri-devel,
linux-arm-kernel
In-Reply-To: <20161024123449.npo4grdlbrj4tcsj@lukather>
On Mon, 24 Oct 2016 14:34:49 +0200
Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Hi,
>
> On Sun, Oct 23, 2016 at 09:45:03AM +0200, Jean-Francois Moine wrote:
> > On Sun, 23 Oct 2016 09:33:16 +0800
> > Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org> wrote:
> >
> > > > Note: This driver is closed to the sun4i-i2s except that:
> > > > - it handles the H3
> > >
> > > If it's close to sun4i-i2s, you should probably rework that one to support
> > > the newer SoCs.
> > >
> > > > - it creates the sound card (with sun4i-i2s, the sound card is created
> > > > by the CODECs)
> > >
> > > I think this is wrong. I2S is only the DAI. You typically have a separate
> > > platform driver for the whole card, or just use simple-card.
> >
> > An other device is not needed. The layout is simple:
> > I2S_controller (CPU DAI) <-> HDMI_CODEC (CODEC DAI)
> > The HDMI CODEC is supported by the HDMI video driver (only one device),
> > so, it cannot be the card device.
> > ASoC does not use the CPU DAI device (I2S_controller), so, it is
> > natural to use it to handle the card.
>
> Still, duplicating the driver is not the solution. I agree with
> Chen-Yu that we want to leverage the driver that is already there.
Hi Maxime and Chen-Yu,
After looking at the sun4i-i2s, I found 2 solutions for re-using its
code in the DE2 HDMI context:
1) either to split the sun4i-i2s driver into common I/O functions and
slave CPU DAI,
2) or to move the sun4i-i2s into a master CPU DAI.
(
some explanation about 'master' and 'slave': the master is the
component the device of which is also the sound card.
As the sound card uses the 'drvdata' of the device, this drvdata pointer
cannot be used by the master.
In the actual implementations:
- sun4i-i2s
master: card dev = codec dev, drvdata -> card
slave: i2s dev (CPU DAI), drvdata -> i2s data
- sun8i-i2s
master: card dev = i2s dev (CPU DAI), drvdata -> card
slave: codec dev (hdmi), drvdata -> codec data (audio/video)
)
In the case 1, there is no functional change, just a source split.
The sun8i-i2s will then use the common I/O functions.
In the case 2, the CODECs using the sun4i-i2s would have to move to
slave CODEC DAI, i.e. the card is created by the sun4i-i2s code.
In the 4.9, there is only one codec (sun4i-codec), so, the change
is just to move the card creation and the use of drvdata in both
codes.
In either cases, I could not check if this changes raise some
regression on the sun4i SoCs side. Then, I'd be glad to know:
- which solution suits you?
- are you ready to do and test the needed changes at the sun4i side?
--
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC PATCH 1/5] of: introduce the overlay manager
From: Pantelis Antoniou @ 2016-10-27 15:07 UTC (permalink / raw)
To: Antoine Tenart
Cc: maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
mark.rutland-5wv7dgnIgG8, sboyd-sgV2jX0FEOL9JmXXK+q4OQ,
thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20161026145756.21689-2-antoine.tenart-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Hi Antoine,
> On Oct 26, 2016, at 17:57 , Antoine Tenart <antoine.tenart-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>
> The overlay manager is an in-kernel library helping to handle dt overlay
> loading when using capes.
>
Code related comments
> Signed-off-by: Antoine Tenart <antoine.tenart-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
> drivers/of/Kconfig | 2 +
> drivers/of/Makefile | 1 +
> drivers/of/overlay-manager/Kconfig | 6 +
> drivers/of/overlay-manager/Makefile | 1 +
> drivers/of/overlay-manager/overlay-manager.c | 199 +++++++++++++++++++++++++++
> include/linux/overlay-manager.h | 38 +++++
> 6 files changed, 247 insertions(+)
> create mode 100644 drivers/of/overlay-manager/Kconfig
> create mode 100644 drivers/of/overlay-manager/Makefile
> create mode 100644 drivers/of/overlay-manager/overlay-manager.c
> create mode 100644 include/linux/overlay-manager.h
>
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index bc07ad30c9bf..e57aeaf0bf4f 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -116,4 +116,6 @@ config OF_OVERLAY
> config OF_NUMA
> bool
>
> +source "drivers/of/overlay-manager/Kconfig"
> +
> endif # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index d7efd9d458aa..d738fd41271f 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -16,3 +16,4 @@ obj-$(CONFIG_OF_OVERLAY) += overlay.o
> obj-$(CONFIG_OF_NUMA) += of_numa.o
>
> obj-$(CONFIG_OF_UNITTEST) += unittest-data/
> +obj-y += overlay-manager/
> diff --git a/drivers/of/overlay-manager/Kconfig b/drivers/of/overlay-manager/Kconfig
> new file mode 100644
> index 000000000000..eeb76054dcb8
> --- /dev/null
> +++ b/drivers/of/overlay-manager/Kconfig
> @@ -0,0 +1,6 @@
> +config OF_OVERLAY_MGR
> + bool "Device Tree Overlay Manager"
> + depends on OF_OVERLAY
> + help
> + Enable the overlay manager to handle automatic overlay loading when
> + devices are detected.
> diff --git a/drivers/of/overlay-manager/Makefile b/drivers/of/overlay-manager/Makefile
> new file mode 100644
> index 000000000000..86d2b53950e7
> --- /dev/null
> +++ b/drivers/of/overlay-manager/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_OF_OVERLAY_MGR) += overlay-manager.o
> diff --git a/drivers/of/overlay-manager/overlay-manager.c b/drivers/of/overlay-manager/overlay-manager.c
> new file mode 100644
> index 000000000000..a725d7e24d38
> --- /dev/null
> +++ b/drivers/of/overlay-manager/overlay-manager.c
> @@ -0,0 +1,199 @@
> +/*
> + * Copyright (C) 2016 - Antoine Tenart <antoine.tenart-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/firmware.h>
> +#include <linux/list.h>
> +#include <linux/of.h>
> +#include <linux/of_fdt.h>
> +#include <linux/overlay-manager.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +struct overlay_mgr_overlay {
> + struct list_head list;
> + char *name;
> +};
> +
> +LIST_HEAD(overlay_mgr_overlays);
> +LIST_HEAD(overlay_mgr_formats);
> +DEFINE_SPINLOCK(overlay_mgr_lock);
> +DEFINE_SPINLOCK(overlay_mgr_format_lock);
> +
Is there a reason for using spinlocks here? OF uses a mutex for
locking since we don’t expect OF manipulation occurring in a
non schedulable context.
> +/*
> + * overlay_mgr_register_format()
> + *
> + * Adds a new format candidate to the list of supported formats. The registered
> + * formats are used to parse the headers stored on the dips.
> + */
> +int overlay_mgr_register_format(struct overlay_mgr_format *candidate)
> +{
> + struct overlay_mgr_format *format;
> + int err = 0;
> +
> + spin_lock(&overlay_mgr_format_lock);
> +
> + /* Check if the format is already registered */
> + list_for_each_entry(format, &overlay_mgr_formats, list) {
> + if (!strcpy(format->name, candidate->name)) {
> + err = -EEXIST;
> + goto err;
> + }
> + }
> +
> + list_add_tail(&candidate->list, &overlay_mgr_formats);
> +
> +err:
> + spin_unlock(&overlay_mgr_format_lock);
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(overlay_mgr_register_format);
> +
> +/*
> + * overlay_mgr_parse()
> + *
> + * Parse raw data with registered format parsers. Fills the candidate string if
> + * one parser understood the raw data format.
> + */
> +int overlay_mgr_parse(struct device *dev, void *data, char ***candidates,
> + unsigned *n)
> +{
The two argument format is not very readable. Perhaps use a structure instead?
> + struct list_head *pos, *tmp;
> + struct overlay_mgr_format *format;
> +
> + list_for_each_safe(pos, tmp, &overlay_mgr_formats) {
> + format = list_entry(pos, struct overlay_mgr_format, list);
> +
> + format->parse(dev, data, candidates, n);
> + if (n > 0)
> + return 0;
> + }
> +
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(overlay_mgr_parse);
> +
> +static int overlay_mgr_check_overlay(struct device_node *node)
> +{
> + struct property *p;
> + const char *str = NULL;
> +
> + p = of_find_property(node, "compatible", NULL);
> + if (!p)
> + return -EINVAL;
> +
> + do {
> + str = of_prop_next_string(p, str);
> + if (of_machine_is_compatible(str))
> + return 0;
I think this is very similar to the way of_match_node works.
Find a way to use that instead?
> + } while (str);
> +
> + return -EINVAL;
> +}
> +
> +/*
> + * _overlay_mgr_insert()
> + *
> + * Try to request and apply an overlay given a candidate name.
> + */
> +static int _overlay_mgr_apply(struct device *dev, char *candidate)
> +{
> + struct overlay_mgr_overlay *overlay;
> + struct device_node *node;
> + const struct firmware *firmware;
> + char *firmware_name;
> + int err = 0;
> +
> + spin_lock(&overlay_mgr_lock);
> +
> + list_for_each_entry(overlay, &overlay_mgr_overlays, list) {
> + if (!strcmp(overlay->name, candidate)) {
> + dev_err(dev, "overlay already loaded\n");
> + err = -EEXIST;
> + goto err_lock;
> + }
> + }
> +
> + overlay = devm_kzalloc(dev, sizeof(*overlay), GFP_KERNEL);
> + if (!overlay) {
> + err = -ENOMEM;
> + goto err_lock;
> + }
> +
spinlock and possibly sleeping?
> + overlay->name = candidate;
> +
> + firmware_name = kasprintf(GFP_KERNEL, "overlay-%s.dtbo", candidate);
> + if (!firmware_name) {
> + err = -ENOMEM;
> + goto err_free;
> + }
> +
> + dev_info(dev, "requesting firmware '%s'\n", firmware_name);
> +
> + err = request_firmware_direct(&firmware, firmware_name, dev);
> + if (err) {
> + dev_info(dev, "failed to request firmware '%s'\n",
> + firmware_name);
> + goto err_free;
> + }
> +
Same here.
> + of_fdt_unflatten_tree((unsigned long *)firmware->data, NULL, &node);
> + if (!node) {
> + dev_err(dev, "failed to unflatted tree\n");
> + err = -EINVAL;
> + goto err_fw;
> + }
> +
> + of_node_set_flag(node, OF_DETACHED);
> +
> + err = of_resolve_phandles(node);
> + if (err) {
> + dev_err(dev, "failed to resolve phandles: %d\n", err);
> + goto err_fw;
> + }
> +
> + err = overlay_mgr_check_overlay(node);
> + if (err) {
> + dev_err(dev, "overlay checks failed: %d\n", err);
> + goto err_fw;
> + }
> +
> + err = of_overlay_create(node);
> + if (err < 0) {
> + dev_err(dev, "failed to create overlay: %d\n", err);
> + goto err_fw;
> + }
> +
> + list_add_tail(&overlay->list, &overlay_mgr_overlays);
> +
> + dev_info(dev, "loaded firmware '%s'\n", firmware_name);
> +
> + spin_unlock(&overlay_mgr_lock);
> + return 0;
> +
> +err_fw:
> + release_firmware(firmware);
> +err_free:
> + devm_kfree(dev, overlay);
> +err_lock:
> + spin_unlock(&overlay_mgr_lock);
> + return err;
> +}
> +
> +int overlay_mgr_apply(struct device *dev, char **candidates, unsigned n)
> +{
> + int i, ret;
> +
> + for (i=0; i < n; i++) {
> + ret = _overlay_mgr_apply(dev, candidates[i]);
> + if (!ret)
> + return 0;
> + }
> +
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(overlay_mgr_apply);
> diff --git a/include/linux/overlay-manager.h b/include/linux/overlay-manager.h
> new file mode 100644
> index 000000000000..8adcc4f5ddf6
> --- /dev/null
> +++ b/include/linux/overlay-manager.h
> @@ -0,0 +1,38 @@
> +/*
> + * Copyright (C) 2016 - Antoine Tenart <antoine.tenart-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#ifndef __OVERLAY_MGR_H__
> +#define __OVERLAY_MGR_H__
> +
> +#include <linux/device.h>
> +#include <linux/list.h>
> +#include <linux/sizes.h>
> +
> +#define OVERLAY_MGR_DIP_MAX_SZ SZ_128
> +
> +struct overlay_mgr_format {
> + struct list_head list;
> + char *name;
> + int (*parse)(struct device *dev, void *data, char ***candidates,
> + unsigned *n);
> +};
> +
> +int overlay_mgr_register_format(struct overlay_mgr_format *candidate);
> +int overlay_mgr_parse(struct device *dev, void *data, char ***candidates,
> + unsigned *n);
> +int overlay_mgr_apply(struct device *dev, char **candidates, unsigned n);
> +
> +#define dip_convert(field) \
> + ( \
> + (sizeof(field) == 1) ? field : \
> + (sizeof(field) == 2) ? be16_to_cpu(field) : \
> + (sizeof(field) == 4) ? be32_to_cpu(field) : \
> + -1 \
> + )
> +
> +#endif /* __OVERLAY_MGR_H__ */
> --
> 2.10.1
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/2] PCI: hisi: add PCIe driver support for HiSilicon STB SoCs
From: Arnd Bergmann @ 2016-10-27 15:06 UTC (permalink / raw)
To: Ruqiang Ju
Cc: bhelgaas, robh+dt, mark.rutland, linux-pci, devicetree,
linux-kernel, bin.chen, elder, hermit.wangheming, yanhaifeng,
xuejiancheng
In-Reply-To: <1477014336-12385-1-git-send-email-juruqiang@huawei.com>
On Friday, October 21, 2016 9:45:36 AM CEST Ruqiang Ju wrote:
> Add PCIe controller drvier for HiSilicon STB SoCs,
> the controller is based on the DesignWare's PCIe core.
>
> Signed-off-by: Ruqiang Ju <juruqiang@huawei.com>
> ---
> .../bindings/pci/hisilicon-histb-pcie.txt | 66 +++
> drivers/pci/host/Kconfig | 8 +
> drivers/pci/host/Makefile | 1 +
> drivers/pci/host/pcie-histb.c | 448 +++++++++++++++++++++
> 4 files changed, 523 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pci/hisilicon-histb-pcie.txt
> create mode 100644 drivers/pci/host/pcie-histb.c
>
> diff --git a/Documentation/devicetree/bindings/pci/hisilicon-histb-pcie.txt b/Documentation/devicetree/bindings/pci/hisilicon-histb-pcie.txt
> new file mode 100644
> index 0000000..952f1db
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/hisilicon-histb-pcie.txt
> @@ -0,0 +1,66 @@
> +HiSilicon STB PCIe host bridge DT description
> +
> +HiSilicon PCIe host controller is based on Designware PCI core.
> +It shares common functions with PCIe Designware core driver and inherits
> +common properties defined in
> +Documentation/devicetree/bindings/pci/designware-pci.txt.
> +
> +Additional properties are described here:
> +
> +Required properties
> +- compatible: Should be one of the following strings
> + "hisilicon,histb-pcie",
> + "hisilicon,hi3798cv200-pcie"
> +- reg: Should contain sysctl, rc_dbi, config registers location and length.
> +- reg-names: Must include the following entries:
> + "sysctrl": system control registers of PCIe controller;
> + "rc_dbi": configuration space of PCIe controller;
Please use "-" instead of "_" in identifiers.
> + "config": configuration transaction space of PCIe controller.
> +- interrupts: MSI interrupt.
> +- interrupt-names: Must include "msi" entries.
Shouldn't this be handled using the msi-map property?
> +- clocks: List of phandle and clock specifier pairs as listed
> + in clock-names property.
> +- clock-name: Must include the following entries:
> + "aux_clk": auxiliary gate clock;
> + "pipe_clk": pipe gate clock;
> + "sys_clk": sys gate clock;
> + "bus_clk": bus gate clock.
drop the redundant _clk postfix.
> +- resets: List of phandle and reset specifier pairs as listed
> + in reset-names property
> +- reset-names: Must include the following entries:
> + "soft_reset": soft reset;
> + "sys_reset": sys reset;
> + "bus_rest": bus reset.
drop the _rest and _reset postfix.
> +Optional properties:
> +- power-gpios: pcie device power control gpio if needed;
> +- power-gpios-active-high: must include this propty
> + if active level is high.
> +- status: Either "ok" or "disabled".
> +
> +Example:
> + pcie@f9860000 {
> + compatible = "hisilicon,histb-pcie", "snps,dw-pcie";
> + reg = <0xf9860000 0x1000>,
> + <0xf0000000 0x2000>,
> + <0xf2000000 0x01000000>;
> + reg-names = "sysctrl", "rc_dbi", "config";
Could it be that the "sysctrl" is not actually part of the PCIe
block but instead part of the system controller block?
Please use a syscon reference instead for those.
> + #address-cells = <3>;
> + #size-cells = <2>;
> + device_type = "pci";
> + num-lanes = <1>;
> + ranges=<0x81000000 0 0 0xf4000000 0 0x00010000
> + 0x82000000 0 0xf3000000 0xf3000000 0 0x01000000>;
That is very short for the memory window. Is there no
prefetchable 64-bit window in the hardware?
> + /* check if the link is up or not */
> + while (!dw_pcie_link_up(pp)) {
> + mdelay(10);
> + count++;
> + if (count == 50) {
> + dev_err(pp->dev, "PCIe Link Fail\n");
> + return -EINVAL;
> + }
> + }
> +
That is a very long delay here. Please don't do that.
> + dev_info(pp->dev, "Link up\n");
> +
> + return 0;
> +}
> +
> +static void histb_pcie_host_init(struct pcie_port *pp)
> +{
> + histb_pcie_establish_link(pp);
> +
> + if (IS_ENABLED(CONFIG_PCI_MSI))
> + dw_pcie_msi_init(pp);
> +}
Just make it depen on PCI_MSI_IRQ_DOMAIN and drop the IS_ENABLED
check.
> +#ifdef CONFIG_PCI_MSI
> +static irqreturn_t histb_pcie_msi_irq_handler(int irq, void *arg)
> +{
> + struct pcie_port *pp = arg;
> +
> + return dw_handle_msi_irq(pp);
> +}
> +#endif
This seems misplaced here. How do the other dw-pcie drivers handle
it? If there is a chance that this driver gets reused on a future
product that has a GICv3, please write the driver so it can already
use that.
Arnd
^ permalink raw reply
* Re: [PATCH v7, 0/8] Add MediaTek USB3 DRD Driver
From: Greg Kroah-Hartman @ 2016-10-27 15:05 UTC (permalink / raw)
To: Chunfeng Yun
Cc: Felipe Balbi, Mathias Nyman, Matthias Brugger, Oliver Neukum,
Alan Stern, Rob Herring, Mark Rutland, Ian Campbell,
Sergei Shtylyov, Pawel Moll, Kumar Gala, Sascha Hauer,
Alan Cooper, linux-usb-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <1476844107-31087-1-git-send-email-chunfeng.yun-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
On Wed, Oct 19, 2016 at 10:28:19AM +0800, Chunfeng Yun wrote:
> These patches introduce the MediaTek USB3 dual-role controller
> driver.
>
> The driver can be configured as Dual-Role Device (DRD),
> Peripheral Only and Host Only (xHCI) modes. It works well
> with Mass Storage, RNDIS and g_zero on FS/HS and SS. And it is
> tested on MT8173 platform which only contains USB2.0 device IP,
> and on MT6290 platform which contains USB3.0 device IP.
>
> Change in v7:
> 1. split dual-role driver into four patchs
> 2. remove QMU done tasklet
> 3. add a bool in xhci_hcd_mtk to signal absence of IPPC
Given a lack of objection from anyone, I've now merged these to my tree
to get them a spin in the 0-day build-bot.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [SPAM][PATCH 2/3] irqchip: mtk-cirq: Add mediatek mtk-cirq implement
From: Yingjoe Chen @ 2016-10-27 15:02 UTC (permalink / raw)
To: Youlin Pei
Cc: Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA,
hongkun.cao-NuS5LvNUpcJWk0Htik3J/w, Jason Cooper,
srv_heupstream-NuS5LvNUpcJWk0Htik3J/w, Marc Zyngier,
erin.lo-NuS5LvNUpcJWk0Htik3J/w,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Russell King, Rob Herring,
linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Matthias Brugger, Thomas Gleixner, yong.wu-NuS5LvNUpcJWk0Htik3J/w
In-Reply-To: <1476335194-26604-3-git-send-email-youlin.pei-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
On Thu, 2016-10-13 at 13:06 +0800, Youlin Pei wrote:
> This commit add the mtk-cirq implement for mt2701.
>
> Signed-off-by: Youlin Pei <youlin.pei-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> ---
> drivers/irqchip/Makefile | 2 +-
> drivers/irqchip/irq-mtk-cirq.c | 257 ++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 258 insertions(+), 1 deletion(-)
> create mode 100644 drivers/irqchip/irq-mtk-cirq.c
>
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 4c203b6..eee95c6 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -59,7 +59,7 @@ obj-$(CONFIG_BCM7120_L2_IRQ) += irq-bcm7120-l2.o
> obj-$(CONFIG_BRCMSTB_L2_IRQ) += irq-brcmstb-l2.o
> obj-$(CONFIG_KEYSTONE_IRQ) += irq-keystone.o
> obj-$(CONFIG_MIPS_GIC) += irq-mips-gic.o
> -obj-$(CONFIG_ARCH_MEDIATEK) += irq-mtk-sysirq.o
> +obj-$(CONFIG_ARCH_MEDIATEK) += irq-mtk-sysirq.o irq-mtk-cirq.o
> obj-$(CONFIG_ARCH_DIGICOLOR) += irq-digicolor.o
> obj-$(CONFIG_RENESAS_H8300H_INTC) += irq-renesas-h8300h.o
> obj-$(CONFIG_RENESAS_H8S_INTC) += irq-renesas-h8s.o
> diff --git a/drivers/irqchip/irq-mtk-cirq.c b/drivers/irqchip/irq-mtk-cirq.c
> new file mode 100644
> index 0000000..544767d
> --- /dev/null
> +++ b/drivers/irqchip/irq-mtk-cirq.c
> @@ -0,0 +1,257 @@
> +/*
> + * Copyright (c) 2016 MediaTek Inc.
> + * Author: Youlin.Pei <youlin.pei-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_address.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/syscore_ops.h>
> +
> +#define CIRQ_MASK_SET 0xC0
> +#define CIRQ_MASK_CLR 0x100
> +#define CIRQ_SENS_SET 0x180
> +#define CIRQ_SENS_CLR 0x1C0
nit: please use lower case for hex value
> +#define CIRQ_POL_SET 0x240
> +#define CIRQ_POL_CLR 0x280
> +#define CIRQ_CONTROL 0x300
> +
> +#define CIRQ_EN 0x1
nit: please align
> +#define CIRQ_EDGE 0x2
> +#define CIRQ_FLUSH 0x4
> +
> +#define CIRQ_IRQ_NUM 0x200
> +
> +struct mtk_cirq_chip_data {
> + void __iomem *base;
> + unsigned int ext_irq_start;
> +};
> +
<deleted..>
> +
> +static int __init mtk_cirq_of_init(struct device_node *node,
> + struct device_node *parent)
> +{
> + struct irq_domain *domain, *domain_parent;
> + int ret;
> +
> + domain_parent = irq_find_host(parent);
> + if (!domain_parent) {
> + pr_err("mtk_cirq: interrupt-parent not found\n");
> + return -EINVAL;
> + }
> +
> + cirq_data = kzalloc(sizeof(*cirq_data), GFP_KERNEL);
> + if (!cirq_data)
> + return -ENOMEM;
> +
> + cirq_data->base = of_iomap(node, 0);
> + if (!cirq_data->base) {
> + pr_err("mtk_cirq: unable to map cirq register\n");
> + ret = -ENXIO;
> + goto out_free;
> + }
> +
> + if (of_property_read_u32(node, "mediatek,ext-irq-start",
> + &cirq_data->ext_irq_start)) {
> + ret = -EINVAL;
> + goto out_free;
Please propagate error returned from of_property_read_u32
Should goto out_unmap when fail here.
Joe.C
> + }
> +
> + domain = irq_domain_add_hierarchy(domain_parent, 0, CIRQ_IRQ_NUM, node,
> + &cirq_domain_ops, cirq_data);
> + if (!domain) {
> + ret = -ENOMEM;
> + goto out_unmap;
> + }
> +
> + mtk_cirq_syscore_init();
> +
> + return 0;
> +
> +out_unmap:
> + iounmap(cirq_data->base);
> +out_free:
> + kfree(cirq_data);
> + return ret;
> +}
> +
> +IRQCHIP_DECLARE(mtk_cirq, "mediatek,mt2701-cirq", mtk_cirq_of_init);
^ permalink raw reply
* Re: [RFC PATCH 1/5] of: introduce the overlay manager
From: Pantelis Antoniou @ 2016-10-27 14:56 UTC (permalink / raw)
To: Antoine Tenart
Cc: Maxime Ripard, Mark Rutland, sboyd, Thomas Petazzoni,
linux-arm-kernel, linux-kernel @ vger . kernel . org, devicetree
In-Reply-To: <20161026145756.21689-2-antoine.tenart@free-electrons.com>
Hi Antoine,
> On Oct 26, 2016, at 17:57 , Antoine Tenart <antoine.tenart@free-electrons.com> wrote:
>
> The overlay manager is an in-kernel library helping to handle dt overlay
> loading when using capes.
>
All in all a nice idea. Comments inline.
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> ---
> drivers/of/Kconfig | 2 +
> drivers/of/Makefile | 1 +
> drivers/of/overlay-manager/Kconfig | 6 +
> drivers/of/overlay-manager/Makefile | 1 +
> drivers/of/overlay-manager/overlay-manager.c | 199 +++++++++++++++++++++++++++
> include/linux/overlay-manager.h | 38 +++++
> 6 files changed, 247 insertions(+)
> create mode 100644 drivers/of/overlay-manager/Kconfig
> create mode 100644 drivers/of/overlay-manager/Makefile
> create mode 100644 drivers/of/overlay-manager/overlay-manager.c
> create mode 100644 include/linux/overlay-manager.h
>
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index bc07ad30c9bf..e57aeaf0bf4f 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -116,4 +116,6 @@ config OF_OVERLAY
> config OF_NUMA
> bool
>
> +source "drivers/of/overlay-manager/Kconfig"
> +
> endif # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index d7efd9d458aa..d738fd41271f 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -16,3 +16,4 @@ obj-$(CONFIG_OF_OVERLAY) += overlay.o
> obj-$(CONFIG_OF_NUMA) += of_numa.o
>
> obj-$(CONFIG_OF_UNITTEST) += unittest-data/
> +obj-y += overlay-manager/
> diff --git a/drivers/of/overlay-manager/Kconfig b/drivers/of/overlay-manager/Kconfig
> new file mode 100644
> index 000000000000..eeb76054dcb8
> --- /dev/null
> +++ b/drivers/of/overlay-manager/Kconfig
> @@ -0,0 +1,6 @@
> +config OF_OVERLAY_MGR
> + bool "Device Tree Overlay Manager"
> + depends on OF_OVERLAY
> + help
> + Enable the overlay manager to handle automatic overlay loading when
> + devices are detected.
> diff --git a/drivers/of/overlay-manager/Makefile b/drivers/of/overlay-manager/Makefile
> new file mode 100644
> index 000000000000..86d2b53950e7
> --- /dev/null
> +++ b/drivers/of/overlay-manager/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_OF_OVERLAY_MGR) += overlay-manager.o
> diff --git a/drivers/of/overlay-manager/overlay-manager.c b/drivers/of/overlay-manager/overlay-manager.c
> new file mode 100644
> index 000000000000..a725d7e24d38
> --- /dev/null
> +++ b/drivers/of/overlay-manager/overlay-manager.c
> @@ -0,0 +1,199 @@
> +/*
> + * Copyright (C) 2016 - Antoine Tenart <antoine.tenart@free-electrons.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/firmware.h>
> +#include <linux/list.h>
> +#include <linux/of.h>
> +#include <linux/of_fdt.h>
> +#include <linux/overlay-manager.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +struct overlay_mgr_overlay {
> + struct list_head list;
> + char *name;
> +};
> +
> +LIST_HEAD(overlay_mgr_overlays);
> +LIST_HEAD(overlay_mgr_formats);
> +DEFINE_SPINLOCK(overlay_mgr_lock);
> +DEFINE_SPINLOCK(overlay_mgr_format_lock);
> +
> +/*
> + * overlay_mgr_register_format()
> + *
> + * Adds a new format candidate to the list of supported formats. The registered
> + * formats are used to parse the headers stored on the dips.
> + */
> +int overlay_mgr_register_format(struct overlay_mgr_format *candidate)
> +{
> + struct overlay_mgr_format *format;
> + int err = 0;
> +
> + spin_lock(&overlay_mgr_format_lock);
> +
> + /* Check if the format is already registered */
> + list_for_each_entry(format, &overlay_mgr_formats, list) {
> + if (!strcpy(format->name, candidate->name)) {
> + err = -EEXIST;
> + goto err;
> + }
> + }
> +
> + list_add_tail(&candidate->list, &overlay_mgr_formats);
> +
> +err:
> + spin_unlock(&overlay_mgr_format_lock);
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(overlay_mgr_register_format);
> +
> +/*
> + * overlay_mgr_parse()
> + *
> + * Parse raw data with registered format parsers. Fills the candidate string if
> + * one parser understood the raw data format.
> + */
> +int overlay_mgr_parse(struct device *dev, void *data, char ***candidates,
> + unsigned *n)
> +{
> + struct list_head *pos, *tmp;
> + struct overlay_mgr_format *format;
> +
> + list_for_each_safe(pos, tmp, &overlay_mgr_formats) {
> + format = list_entry(pos, struct overlay_mgr_format, list);
> +
> + format->parse(dev, data, candidates, n);
> + if (n > 0)
> + return 0;
> + }
> +
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(overlay_mgr_parse);
> +
> +static int overlay_mgr_check_overlay(struct device_node *node)
> +{
> + struct property *p;
> + const char *str = NULL;
> +
> + p = of_find_property(node, "compatible", NULL);
> + if (!p)
> + return -EINVAL;
> +
> + do {
> + str = of_prop_next_string(p, str);
> + if (of_machine_is_compatible(str))
> + return 0;
> + } while (str);
> +
> + return -EINVAL;
> +}
> +
> +/*
> + * _overlay_mgr_insert()
> + *
> + * Try to request and apply an overlay given a candidate name.
> + */
> +static int _overlay_mgr_apply(struct device *dev, char *candidate)
> +{
> + struct overlay_mgr_overlay *overlay;
> + struct device_node *node;
> + const struct firmware *firmware;
> + char *firmware_name;
> + int err = 0;
> +
> + spin_lock(&overlay_mgr_lock);
> +
> + list_for_each_entry(overlay, &overlay_mgr_overlays, list) {
> + if (!strcmp(overlay->name, candidate)) {
> + dev_err(dev, "overlay already loaded\n");
> + err = -EEXIST;
> + goto err_lock;
> + }
> + }
> +
> + overlay = devm_kzalloc(dev, sizeof(*overlay), GFP_KERNEL);
> + if (!overlay) {
> + err = -ENOMEM;
> + goto err_lock;
> + }
> +
> + overlay->name = candidate;
> +
> + firmware_name = kasprintf(GFP_KERNEL, "overlay-%s.dtbo", candidate);
> + if (!firmware_name) {
> + err = -ENOMEM;
> + goto err_free;
> + }
> +
> + dev_info(dev, "requesting firmware '%s'\n", firmware_name);
> +
> + err = request_firmware_direct(&firmware, firmware_name, dev);
> + if (err) {
> + dev_info(dev, "failed to request firmware '%s'\n",
> + firmware_name);
> + goto err_free;
> + }
> +
> + of_fdt_unflatten_tree((unsigned long *)firmware->data, NULL, &node);
> + if (!node) {
> + dev_err(dev, "failed to unflatted tree\n");
> + err = -EINVAL;
> + goto err_fw;
> + }
> +
> + of_node_set_flag(node, OF_DETACHED);
> +
> + err = of_resolve_phandles(node);
> + if (err) {
> + dev_err(dev, "failed to resolve phandles: %d\n", err);
> + goto err_fw;
> + }
> +
> + err = overlay_mgr_check_overlay(node);
> + if (err) {
> + dev_err(dev, "overlay checks failed: %d\n", err);
> + goto err_fw;
> + }
> +
> + err = of_overlay_create(node);
> + if (err < 0) {
> + dev_err(dev, "failed to create overlay: %d\n", err);
> + goto err_fw;
> + }
> +
> + list_add_tail(&overlay->list, &overlay_mgr_overlays);
> +
> + dev_info(dev, "loaded firmware '%s'\n", firmware_name);
> +
> + spin_unlock(&overlay_mgr_lock);
> + return 0;
> +
> +err_fw:
> + release_firmware(firmware);
> +err_free:
> + devm_kfree(dev, overlay);
> +err_lock:
> + spin_unlock(&overlay_mgr_lock);
> + return err;
> +}
> +
> +int overlay_mgr_apply(struct device *dev, char **candidates, unsigned n)
> +{
> + int i, ret;
> +
> + for (i=0; i < n; i++) {
> + ret = _overlay_mgr_apply(dev, candidates[i]);
> + if (!ret)
> + return 0;
> + }
> +
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(overlay_mgr_apply);
> diff --git a/include/linux/overlay-manager.h b/include/linux/overlay-manager.h
> new file mode 100644
> index 000000000000..8adcc4f5ddf6
> --- /dev/null
> +++ b/include/linux/overlay-manager.h
> @@ -0,0 +1,38 @@
> +/*
> + * Copyright (C) 2016 - Antoine Tenart <antoine.tenart@free-electrons.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#ifndef __OVERLAY_MGR_H__
> +#define __OVERLAY_MGR_H__
> +
> +#include <linux/device.h>
> +#include <linux/list.h>
> +#include <linux/sizes.h>
> +
> +#define OVERLAY_MGR_DIP_MAX_SZ SZ_128
> +
This should not be here; if it’s general kernel plumbing no mention to
specific boards/archs are relevant.
> +struct overlay_mgr_format {
> + struct list_head list;
> + char *name;
> + int (*parse)(struct device *dev, void *data, char ***candidates,
> + unsigned *n);
> +};
> +
> +int overlay_mgr_register_format(struct overlay_mgr_format *candidate);
> +int overlay_mgr_parse(struct device *dev, void *data, char ***candidates,
> + unsigned *n);
> +int overlay_mgr_apply(struct device *dev, char **candidates, unsigned n);
> +
> +#define dip_convert(field) \
> + ( \
> + (sizeof(field) == 1) ? field : \
> + (sizeof(field) == 2) ? be16_to_cpu(field) : \
> + (sizeof(field) == 4) ? be32_to_cpu(field) : \
> + -1 \
> + )
> +
Same as above.
> +#endif /* __OVERLAY_MGR_H__ */
> --
> 2.10.1
Regards
— Pantelis
^ permalink raw reply
* Re: [PATCH 1/2] PCI: rockchip: Add three new resets as required properties
From: Rob Herring @ 2016-10-27 14:54 UTC (permalink / raw)
To: Shawn Lin
Cc: Heiko Stuebner, Bjorn Helgaas, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Brian Norris,
linux-pci-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1477017836-19317-2-git-send-email-shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
On Fri, Oct 21, 2016 at 10:43:55AM +0800, Shawn Lin wrote:
> pm_rst, aclk_rst, pclk_rst was controlled by rom code so the
> software wasn't needed to control it again in theory. But it
> didn't work properly, so we do need to do it again and add a
> enough delay between the assert of pm_rst and the deassert of
> pm_rst. The Soc intergrated with this controller, rk3399 is still
> under MP test internally, so the backward compatibility won't be
> a big deal.
>
> Signed-off-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>
> ---
>
> .../devicetree/bindings/pci/rockchip-pcie.txt | 11 ++--
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> drivers/pci/host/pcie-rockchip.c | 62 ++++++++++++++++++++++
> 2 files changed, 70 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> index ba67b39..71aeda1 100644
> --- a/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/rockchip-pcie.txt
> @@ -26,13 +26,16 @@ Required properties:
> - "sys"
> - "legacy"
> - "client"
> -- resets: Must contain five entries for each entry in reset-names.
> +- resets: Must contain seven entries for each entry in reset-names.
> See ../reset/reset.txt for details.
> - reset-names: Must include the following names
> - "core"
> - "mgmt"
> - "mgmt-sticky"
> - "pipe"
> + - "pm"
> + - "aclk"
> + - "pclk"
> - pinctrl-names : The pin control state names
> - pinctrl-0: The "default" pinctrl state
> - #interrupt-cells: specifies the number of cells needed to encode an
> @@ -86,8 +89,10 @@ pcie0: pcie@f8000000 {
> reg = <0x0 0xf8000000 0x0 0x2000000>, <0x0 0xfd000000 0x0 0x1000000>;
> reg-names = "axi-base", "apb-base";
> resets = <&cru SRST_PCIE_CORE>, <&cru SRST_PCIE_MGMT>,
> - <&cru SRST_PCIE_MGMT_STICKY>, <&cru SRST_PCIE_PIPE>;
> - reset-names = "core", "mgmt", "mgmt-sticky", "pipe";
> + <&cru SRST_PCIE_MGMT_STICKY>, <&cru SRST_PCIE_PIPE> ,
> + <&cru SRST_PCIE_PM>, <&cru SRST_P_PCIE>, <&cru SRST_A_PCIE>;
> + reset-names = "core", "mgmt", "mgmt-sticky", "pipe",
> + "pm", "pclk", "aclk";
> phys = <&pcie_phy>;
> phy-names = "pcie-phy";
> pinctrl-names = "default";
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> index e0b22da..e04f69b 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -190,6 +190,9 @@ struct rockchip_pcie {
> struct reset_control *mgmt_rst;
> struct reset_control *mgmt_sticky_rst;
> struct reset_control *pipe_rst;
> + struct reset_control *pm_rst;
> + struct reset_control *aclk_rst;
> + struct reset_control *pclk_rst;
> struct clk *aclk_pcie;
> struct clk *aclk_perf_pcie;
> struct clk *hclk_pcie;
> @@ -408,6 +411,44 @@ static int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
>
> gpiod_set_value(rockchip->ep_gpio, 0);
>
> + err = reset_control_assert(rockchip->aclk_rst);
> + if (err) {
> + dev_err(dev, "assert aclk_rst err %d\n", err);
> + return err;
> + }
> +
> + err = reset_control_assert(rockchip->pclk_rst);
> + if (err) {
> + dev_err(dev, "assert pclk_rst err %d\n", err);
> + return err;
> + }
> +
> + err = reset_control_assert(rockchip->pm_rst);
> + if (err) {
> + dev_err(dev, "assert pm_rst err %d\n", err);
> + return err;
> + }
> +
> + udelay(10);
> +
> + err = reset_control_deassert(rockchip->pm_rst);
> + if (err) {
> + dev_err(dev, "deassert pm_rst err %d\n", err);
> + return err;
> + }
> +
> + err = reset_control_deassert(rockchip->aclk_rst);
> + if (err) {
> + dev_err(dev, "deassert mgmt_sticky_rst err %d\n", err);
> + return err;
> + }
> +
> + err = reset_control_deassert(rockchip->pclk_rst);
> + if (err) {
> + dev_err(dev, "deassert mgmt_sticky_rst err %d\n", err);
> + return err;
> + }
> +
> err = phy_init(rockchip->phy);
> if (err < 0) {
> dev_err(dev, "fail to init phy, err %d\n", err);
> @@ -781,6 +822,27 @@ static int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
> return PTR_ERR(rockchip->pipe_rst);
> }
>
> + rockchip->pm_rst = devm_reset_control_get(dev, "pm");
> + if (IS_ERR(rockchip->pm_rst)) {
> + if (PTR_ERR(rockchip->pm_rst) != -EPROBE_DEFER)
> + dev_err(dev, "missing pm reset property in node\n");
> + return PTR_ERR(rockchip->pm_rst);
> + }
> +
> + rockchip->pclk_rst = devm_reset_control_get(dev, "pclk");
> + if (IS_ERR(rockchip->pclk_rst)) {
> + if (PTR_ERR(rockchip->pclk_rst) != -EPROBE_DEFER)
> + dev_err(dev, "missing pclk reset property in node\n");
> + return PTR_ERR(rockchip->pclk_rst);
> + }
> +
> + rockchip->aclk_rst = devm_reset_control_get(dev, "aclk");
> + if (IS_ERR(rockchip->aclk_rst)) {
> + if (PTR_ERR(rockchip->aclk_rst) != -EPROBE_DEFER)
> + dev_err(dev, "missing aclk reset property in node\n");
> + return PTR_ERR(rockchip->aclk_rst);
> + }
> +
> rockchip->ep_gpio = devm_gpiod_get(dev, "ep", GPIOD_OUT_HIGH);
> if (IS_ERR(rockchip->ep_gpio)) {
> dev_err(dev, "missing ep-gpios property in node\n");
> --
> 2.3.7
>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC PATCH 1/5] of: introduce the overlay manager
From: Antoine Tenart @ 2016-10-27 14:54 UTC (permalink / raw)
To: Mathieu Poirier
Cc: Mark Rutland, Thomas Petazzoni, devicetree@vger.kernel.org,
pantelis.antoniou, Antoine Tenart, sboyd,
linux-kernel@vger.kernel.org, Maxime Ripard,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <CANLsYkxF4_fZwaBe+_0twMhND3TfpnmkXA_1q=AN_j9=cQ5ukw@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 752 bytes --]
On Thu, Oct 27, 2016 at 08:49:29AM -0600, Mathieu Poirier wrote:
>
> I agree - something like this should have attracted more reviews. I
> suggest providing a better explanation, i.e what you are doing and why
> in more details. I reviewed your set and I'm still not sure of
> exactly what it does, hence not being able to comment on the validity
> of the approach.
>
> I'm pretty sure there is value in what you are suggesting, it's a
> matter of getting people to understand the approach and motivation.
That's what I tried to do in the cover-letter. What do you think is
missing that I should add in it?
Thanks,
Antoine
--
Antoine Ténart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: Add Allwinner Q8 tablets hardware manager
From: Hans de Goede @ 2016-10-27 14:53 UTC (permalink / raw)
To: Pierre-Hugues Husson
Cc: Mark Rutland, devicetree, Arnd Bergmann, Greg Kroah-Hartman,
Pantelis Antoniou, Chen-Yu Tsai, Rob Herring, Maxime Ripard,
linux-arm-kernel
In-Reply-To: <CAJ-oXjTfRDvSKs9zZBxt1V2MR5C+XnsqfZBRTWwRrJXx-_7r=Q@mail.gmail.com>
Hi,
On 27-10-16 14:57, Pierre-Hugues Husson wrote:
> 2016-10-27 11:14 GMT+02:00 Hans de Goede <hdegoede@redhat.com>:
>> In my experience with these cheap boards, there is a mix of auto-probing +
>> device / revision specific os-image modifications. I keep coming back to
>> the touchscreen controller firmware (but also the orientation), for the
>> gsl1680 controller I need at least 2 different firmware files (per gsl1680
>> revision) to make all q8 tablets I have working. This is simply not solved
>> by the vendor android code, they just shove the right firmware into the
>> os-image. Likewise for the touchscreen orientation (x-mirored, y-mirored,
>> etc) too is just a hard-coded setting in the os-image.
> Reading your patch, it looks like to handle the two different firmware
> files, you're simply adding a command-line switch, there is no
> detection involved.
> Am I understanding correctly?
No, the firmware-name (and matching resolution as different firmwares
report different axis-ranges for the same digitizer) is selected
primarily by the touchscreen_variant which sets: touchscreen_fw_name,
touchscreen_width and touchscreen_height.
The touchscreen_variant module option defaults to -1 which means "auto",
when it is auto it gets set based on the touchscreen / accelerometer
combination (which more or less uniquely identifies boards sofar),
likewise all the other touchscreen module options default to -1,
but can be overridden from the commandline.
The intention is for things to just work, the commandline options are
there as a fallback.
> If this is the case, two things:
> 1. I'm not too sure having the user choose this via cmdline is the
> right way. I think I'd rather have it set by userspace. (though that's
> not a strong opinion).
> Or if cmdline is being changed... how about having DTS (or just an
> overlay on top of it) being changed instead?
>
> 2. This could still be declared by DTS. For instance, assuming your
> i2c-probe-stop-at-first-match:
> &i2c0 {
> touchscreen1: gsl1680@40 {
> reg = <0x40>;
> compatible = "silead,gsl1680";
> enable-gpios = <&pio 7 1 GPIO_ACTIVE_HIGH>; /* PH1 */
> touchscreen-size = <1024 600>;
> touchscreen-fw = "gsl1680-a082-q8-700.fw";
> filter-names = "touchscreen_variant";
> filter-0 = "none", "gsl1680-a082-q8-700";
> id = <0xa0820000>;
> status = "disabled";
> };
> touchscreen2: gsl1680@40 {
> reg = <0x40>;
> compatible = "silead,gsl1680";
> enable-gpios = <&pio 7 1 GPIO_ACTIVE_HIGH>; /* PH1 */
> touchscreen-size = <480 800>;
> touchscreen-fw = "gsl1680-a082-q8-a70.fw";
> filter-names = "touchscreen_variant";
> filter-0 = "gsl1680-a082-q8-a70";
> id = <0xa0820000>;
> status = "disabled";
> };
> touchscreen2: gsl1680@40 {
> reg = <0x40>;
> compatible = "silead,gsl1680";
> enable-gpios = <&pio 7 1 GPIO_ACTIVE_HIGH>; /* PH1 */
> touchscreen-size = <960 640>;
> touchscreen-fw = "gsl1680-b482-q8-d702.fw";
> filter-names = "touchscreen_variant";
> filter-0 = "gsl1680-b482-q8-d702";
> id = <0xb4820000>;
> status = "disabled";
> };
> i2c-probe-stop-at-first-match = <&touchscreen1>,
> <&touchscreen2>, <&touchscreen3>;
> }
>
> With "none" value being the value when the "touchscreen_variant"
> option is not defined in cmdline.
>
> Please note that I'm not too sure whether SILEAD_REG_ID represents an
> OTP which can be changed by OEM, or if it's more of a hardware
> revision. Depending on this, this would either fit into a id =
> <0xa0820000> DTS line, or a compatible = "silead,gsl1680_a082",
> "silead,gsl1680"; DTS line.
>
>> Sofar I've only seen this with one type of touchscreen so an easy cop-out
>> would be to add an "optional-vddio-supply" to the the bindings for the
>> specific touchscreen use and put all the necessary logic in the driver.
>>
>> This does require propagating the learned need for the regulator
>> from the drivers detect() callback to probe() or alternatively I'm
>> thinking we should just use probe() instead of detect()to begin with,
>> that will save a lot of duplication with things
>> like code for enable gpio-s and regulators.
>>
>> So assuming we go for the cop-out option for 3. (I'm ok with that),
>> this would be a pretty clean solution adding just the 2 new:
>> i2c-probe-stop-at-first-match and i2c-probe-all properties to
>> the i2c-bus bindings. One problem here is that we may want to have
>> multiple i2c-probe-stop-at-first-match phandle lists on a single bus
>> (e.g. try 3 touchscreens + 6 accelerometers on the same bus, stop at
>> first touchscreen / first accelerometer), anyone have any ideas for
>> that?
> How about something like:
>
> &i2c1 {
> touchscreen1....
> touchscreen2....
> touchscreen3....
> accelerometer1....
> accelerometer2....
> accelerometer3....
> accelerometer4....
>
> select-one {
> compatible = "i2c-select;
> group-names = "touchscreen", "accelerometer";
> group-0 = <&touchscreen1>, <&touchscreen2>, <&touchscreen3>;
> group-1 = <&accelerometer1>, <&accelerometer2>,
> <&accelerometer3>, <&accelerometer4>;
> };
> };
We could just have:
i2c-probe-stop-at-first-match-0 = <&touchscreen1>, <&touchscreen2>, <&touchscreen3>;
i2c-probe-stop-at-first-match-1 = <&accelerometer1>, <&accelerometer2>;
And have the i2c bus code look for an i2c-probe-stop-at-first-match-[i++] property
until it is not found. Having a child-node with its own compatible for this
feels wrong, as it uses a hierarchy where there really is none.
>>> When it comes to detection, I've witnessed various things.
>>> It can be kernel-side or bootloader-side "global setting" reading (like an
>>> ADC/resistor value, or an OTP), it can be bootloader doing the
>>> "brute-force", or it can be the kernel doing all the probes.
>>>
>>> For instance, as of today, on a Spreadtrum ODM tree, the bootloader will
>>> detect the screen by testing all knowns screens, the screen-drivers declare
>>> a get_id function, and the bootloader probes until the get_id matches the id
>>> declared by the screen driver.
>>> And then the bootloader tells the kernel, via cmdline, which screen is
>>> actually there (but auto-detection is also coded in kernel).
>>> Finally all possible sensors/touchscreen/camera are declared in DTS, and
>>> probe will filter-out N/C ones in the kernel.
>>>
>>> Now the big difference between my experience and what Hans is trying to
>>> do, is that I've always worked with devices with "safely" queriable IDs,
>>> either on i2c or dsi. I've never encountered SPI. This makes probing
>>> inherently more dangerous, but I believe the question roughly remains the
>>> same.
>>
>>
>> I'm dealing with i2c too, Mark mistakenly used SPI in his reply,
>> which I think is what got you thinking I've SPI.
> Right, so let's concentrate on reasonable bus-es first then. (I can
> think of I2C and DSI)
>
>> See above, I think that we can make this work by delegating the actual
>> detection to the driver (so each compatible can have a different detect
>> method / code).
>> So with this we can remove a big part of drivers/misc/q8-hardwaremgr.c, but
>> not all
>> of it. We still need board specific code somewhere to deal with things like
>> picking
>> the right touchscreen firmware and touchscreen orientation. This is all
>> somewhat
>> gsl1680 specific.
>> I actually have the same problem on x86 where the ACPI description of the
>> device
>> basically says: "There is a gsl1680 at this bus at this address" and does
>> not say
>> anything about firmware / orientation (again this is simply hardcoded
>> in the os-image these devices ship with).
>>
>> For x86 my plan is to have an array of configs in the driver and select the
>> right
>> one based on DMI strings, which is in essence putting board specific info in
>> the
>> driver.
>>
>> I can imagine mirroring this for ARM, and have an array of configs in the
>> driver
>> there too (for cases where cannot simply hardcode everything in dt only) and
>> have
>> some board specific code (activated by of_machine_is_compatible()) to select
>> the
>> right config.
> I do believe this can all be done in DTS
Well x86 does not have DTS.
> and at the moment, what
> you're describing seem to happen often enough to be worth writing
> generic code for.
Let me quote some of the auto-code currently in q8-hardwaremgr.c :
/*
* These accelerometer based heuristics select the best
* default based on known q8 tablets.
*/
switch (data->accelerometer.model) {
case da280:
if (data->accelerometer.addr == 0x27)
; /* No-op */
else if (data->has_rda599x)
data->touchscreen_invert_x = 1;
else
data->touchscreen_invert_y = 1;
break;
case dmard09:
data->touchscreen_invert_x = 1;
break;
case mxc6225:
data->touchscreen_variant = 1;
break;
}
(Non set data->touchscreen_foo are left at 0).
So this would require us to be able to filter (to use your example)
on if another i2c device is found and on which address it is found,
that does not even take the rda559x check into account and is
going to cause interesting ordering issues, how do we know when
we can actually do the filtering if some of the variables we are
filtering on are set by other auto-detected paths. Which auto-detect /
i2c-probe-stop-at-first-match list do we execute first ? Worse
actually for accelerometer orientation I will likely need to
set the mount-matrix based on the detected touchscreen ...
The rda559x here is a sdio wifi chip, which is also connected to the
i2c, and currently is detected through i2c to be able to separately
identify 2 q8 boards which share the same touchscreen + accelerometer
combination and who knows what other checks I or other people can
come up with to differentiate board variants which do not have
a simple eeprom to uniquely id them.
So as said before, no this cannot be all done in dt without
adding a turing complete language to dt, and that is just to
select which touchscreen_variant to use.
Then there also the probem of the combinatorial explosion having
not only 2 firmware files but also invert-x and invert-y flags causes:
We have 2 revisions with each 2 different firmware-files (more actually
but I've reduced the set since some firmwares are compatible) with each
both the x- and / or y axis as normal or inverted, for a total of:
2 (revision) * 2 (firmware-files) * 2 (x-inverted or not) * 2 (y...) = 16
touchscreen variants, which means dt nodes for touchscreen1 to touchscreen16
and that is just the silead gsl1680, some of these tablets also have
elan or zeitec touchscreen controllers.
Now imagine what happens if a new board comes out which needs a 3th firmware
file... I hope you can understand this is not a route I want to go.
Another problem is that if a user encounters the need for a new firmware
variant he can now not easily try this (where as before we had
module options to separately override firmware-name, the size, etc.
As written in my previous mail, this is all rather gsl1680 specific,
and esp. being able to override the firmware-name, the size, etc.
through module options is going to be useful (to ask endusers to test
stuff without recompiling) on x86 too. So we will likely want to add
most of the necessary stuff to the silead driver anyways.
> But then, I can't really tell which makes the most sense between
> source-based and devicetree-based.
> I prefer doing it in device-tree, since it means that any OEM can have
> his device supported by only providing DTB, and won't need to provide
> kernel patches.
If the OEM provides a DTB the OEM can just directly have the right
parameters in there without relying on any auto-detection, this is
already supported and the e.g. gsl1680 driver already happily
works on several tablets where there is not so much hardware
variance.
Even if the OEM needs to deal with e.g. different touchscreens on
different board revisions, hopefully the simple auto-detect code will
be enough, and he does not need e.g. different firmware-name settings
for otherwise the same touchscreen controller. If that is not the
case then he the OEM will have to provide a separate static
(non probing) DTB per variant.
>> 2) miscellaneous extra config on top of figuring out which ICs are
>> connected,
>> basically the kind of stuff many vendors simply hard-code in their device
>> specific os-image. This one is much more difficult to deal with and I think
>> we need to figure this out on a case by case basis. This will require board
>> specific code (just like the kernel has tons of DMI string activated board
>> specific code on x86) and what is the best code for this place to live will
>> be a case by case thing too.
>
> With things like mount-matrix devicetree property, the goal is to have
> such informations in the DTS.
Right and all the info I'm talking about can already be in the DTS and
is already specified this way for various existing boards, this is
obviously how we want things to work, this is the normal case /
the straight code path.
Now lets get back to your mount-matrix example, the problem here is 2 board
variants where the same accelerometer is used, but on a newer revision
of the board it is mounted with a different orientation and otherwise
almost nothing is changed on the board, certainly not something as
useful as an id eeprom.
Lets assume that we can however still somehow differ the 2 revisions,
then try to imagine how many different ways there are to differ
between 2 board revisions if there is no easy way to do so,
some crazy examples:
-The 2nd revision has an external loopback on unused audio out / in
pins for testing purposes, we could play + record sound and do
a (rough) waveform match to see if the loopback is present
-On the 2nd revision a pin from a pin compatible part which
allows putting it in fully compatible mode, or allow new features
mode, is now hooked up to a gpio instead of hardwired to compatible
mode, we could change the device to new features mode and try
to read/modify/write some register bit on the chip which is only
writable in this mode
-Etc.
Now try to design a way to express this in dt and we're back to
needing a turing complete language (with a library for accessing
various busses) again.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH 1/2] PCI: hisi: add PCIe driver support for HiSilicon STB SoCs
From: Rob Herring @ 2016-10-27 14:52 UTC (permalink / raw)
To: Ruqiang Ju
Cc: bhelgaas, mark.rutland, linux-pci, devicetree, linux-kernel,
bin.chen, elder, hermit.wangheming, yanhaifeng, xuejiancheng
In-Reply-To: <1477014336-12385-1-git-send-email-juruqiang@huawei.com>
On Fri, Oct 21, 2016 at 09:45:36AM +0800, Ruqiang Ju wrote:
> Add PCIe controller drvier for HiSilicon STB SoCs,
> the controller is based on the DesignWare's PCIe core.
>
> Signed-off-by: Ruqiang Ju <juruqiang@huawei.com>
> ---
> .../bindings/pci/hisilicon-histb-pcie.txt | 66 +++
> drivers/pci/host/Kconfig | 8 +
> drivers/pci/host/Makefile | 1 +
> drivers/pci/host/pcie-histb.c | 448 +++++++++++++++++++++
> 4 files changed, 523 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pci/hisilicon-histb-pcie.txt
> create mode 100644 drivers/pci/host/pcie-histb.c
>
> diff --git a/Documentation/devicetree/bindings/pci/hisilicon-histb-pcie.txt b/Documentation/devicetree/bindings/pci/hisilicon-histb-pcie.txt
> new file mode 100644
> index 0000000..952f1db
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/hisilicon-histb-pcie.txt
> @@ -0,0 +1,66 @@
> +HiSilicon STB PCIe host bridge DT description
> +
> +HiSilicon PCIe host controller is based on Designware PCI core.
> +It shares common functions with PCIe Designware core driver and inherits
> +common properties defined in
> +Documentation/devicetree/bindings/pci/designware-pci.txt.
> +
> +Additional properties are described here:
> +
> +Required properties
> +- compatible: Should be one of the following strings
> + "hisilicon,histb-pcie",
This should either be a fallback compatible or be dropped.
> + "hisilicon,hi3798cv200-pcie"
> +- reg: Should contain sysctl, rc_dbi, config registers location and length.
> +- reg-names: Must include the following entries:
> + "sysctrl": system control registers of PCIe controller;
> + "rc_dbi": configuration space of PCIe controller;
> + "config": configuration transaction space of PCIe controller.
> +- interrupts: MSI interrupt.
> +- interrupt-names: Must include "msi" entries.
> +- clocks: List of phandle and clock specifier pairs as listed
> + in clock-names property.
> +- clock-name: Must include the following entries:
> + "aux_clk": auxiliary gate clock;
> + "pipe_clk": pipe gate clock;
> + "sys_clk": sys gate clock;
> + "bus_clk": bus gate clock.
> +- resets: List of phandle and reset specifier pairs as listed
> + in reset-names property
> +- reset-names: Must include the following entries:
> + "soft_reset": soft reset;
> + "sys_reset": sys reset;
> + "bus_rest": bus reset.
> +
> +Optional properties:
> +- power-gpios: pcie device power control gpio if needed;
> +- power-gpios-active-high: must include this propty
> + if active level is high.
This should be dropped. The power-gpios property should indicate the
active state.
> +- status: Either "ok" or "disabled".
No need to explicitly document this.
> +
> +Example:
> + pcie@f9860000 {
> + compatible = "hisilicon,histb-pcie", "snps,dw-pcie";
> + reg = <0xf9860000 0x1000>,
> + <0xf0000000 0x2000>,
> + <0xf2000000 0x01000000>;
> + reg-names = "sysctrl", "rc_dbi", "config";
> + #address-cells = <3>;
> + #size-cells = <2>;
> + device_type = "pci";
> + num-lanes = <1>;
> + ranges=<0x81000000 0 0 0xf4000000 0 0x00010000
> + 0x82000000 0 0xf3000000 0xf3000000 0 0x01000000>;
> + interrupts = <GIC_SPI 128 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-names = "msi";
> + #interrupt-cells = <1>;
> + interrupt-map-mask = <0 0 0 7>;
> + interrupt-map = <0x0 0 0 1 &gic 0 131 4>;
> + clocks = <&crg PCIE_AUX_CLK>,
> + <&crg PCIE_PIPE_CLK>,
> + <&crg PCIE_SYS_CLK>,
> + <&crg PCIE_BUS_CLK>;
> + clock-names = "aux_clk", "pipe_clk", "sys_clk", "bus_clk";
> + resets = <&crg 0x18c 6>, <&crg 0x18c 5>, <&crg 0x18c 4>;
> + reset-names = "soft_reset", "sys_reset", "bus_reset";
> + };
^ permalink raw reply
* Re: [RFC PATCH 1/5] of: introduce the overlay manager
From: Mathieu Poirier @ 2016-10-27 14:49 UTC (permalink / raw)
To: Antoine Tenart
Cc: Mark Rutland, Thomas Petazzoni, devicetree@vger.kernel.org,
pantelis.antoniou, sboyd, linux-kernel@vger.kernel.org,
Maxime Ripard, linux-arm-kernel@lists.infradead.org
In-Reply-To: <20161027140342.kidk4gqxwbtphrbs@kwain>
On 27 October 2016 at 08:03, Antoine Tenart
<antoine.tenart@free-electrons.com> wrote:
> Hello Mathieu,
>
> On Wed, Oct 26, 2016 at 10:29:59AM -0600, Mathieu Poirier wrote:
>>
>> Please find my comments below.
>
> Thanks for the comments. I expected more distant reviews, on the overall
> architecture to know if this could fit the needs of others. But anyway
> your comments are helpful if we ever decide to go with an overlay
> manager like this one.
I agree - something like this should have attracted more reviews. I
suggest providing a better explanation, i.e what you are doing and why
in more details. I reviewed your set and I'm still not sure of
exactly what it does, hence not being able to comment on the validity
of the approach.
I'm pretty sure there is value in what you are suggesting, it's a
matter of getting people to understand the approach and motivation.
>
>> On 26 October 2016 at 08:57, Antoine Tenart
>> <antoine.tenart@free-electrons.com> wrote:
>> > +
>> > +/*
>> > + * overlay_mgr_register_format()
>> > + *
>> > + * Adds a new format candidate to the list of supported formats. The registered
>> > + * formats are used to parse the headers stored on the dips.
>> > + */
>> > +int overlay_mgr_register_format(struct overlay_mgr_format *candidate)
>> > +{
>> > + struct overlay_mgr_format *format;
>> > + int err = 0;
>> > +
>> > + spin_lock(&overlay_mgr_format_lock);
>> > +
>> > + /* Check if the format is already registered */
>> > + list_for_each_entry(format, &overlay_mgr_formats, list) {
>> > + if (!strcpy(format->name, candidate->name)) {
>>
>> This function is public to the rest of the kernel - limiting the
>> lenght of ->name and using strncpy() is probably a good idea.
>
> I totally agree. This was in fact something I wanted to do.
>
>> > +
>> > +/*
>> > + * overlay_mgr_parse()
>> > + *
>> > + * Parse raw data with registered format parsers. Fills the candidate string if
>> > + * one parser understood the raw data format.
>> > + */
>> > +int overlay_mgr_parse(struct device *dev, void *data, char ***candidates,
>>
>> I'm pretty sure there is another way to proceed than using 3 levels of
>> references. It makes the code hard to read and a prime candidate for
>> errors.
>
> Sure. I guess we could allocate an array of fixed-length strings which
> would be less flexible but I don't think we need something that flexible
> here.
>
>>
>> > +
>> > + format->parse(dev, data, candidates, n);
>>
>> ->parse() returns an error code. It is probably a good idea to check
>> it. If it isn't needed then a comment explaining why it is the case
>> would be appreciated.
>
> So the point of the parse function is to determine if the data read from
> a source is a compatible header of a given format. Returning an error
> doesn't mean the header won't be recognized by another one.
>
> We could maybe handle this better, by returning an error iif different
> that -EINVAL. Or we could have one function to check the compatibility
> and one to parse it, if compatible.
>
>> > +static int _overlay_mgr_apply(struct device *dev, char *candidate)
>> > +{
>> > + struct overlay_mgr_overlay *overlay;
>> > + struct device_node *node;
>> > + const struct firmware *firmware;
>> > + char *firmware_name;
>> > + int err = 0;
>> > +
>> > + spin_lock(&overlay_mgr_lock);
>> > +
>> > + list_for_each_entry(overlay, &overlay_mgr_overlays, list) {
>> > + if (!strcmp(overlay->name, candidate)) {
>> > + dev_err(dev, "overlay already loaded\n");
>> > + err = -EEXIST;
>> > + goto err_lock;
>> > + }
>> > + }
>> > +
>> > + overlay = devm_kzalloc(dev, sizeof(*overlay), GFP_KERNEL);
>>
>> Function devm_kzalloc() can sleep but you're holding a spinlock - I'm
>> surprised the kernel didn't complain here. Allocate the memory before
>> holding the lock. If the overly is already loaded simply free it on
>> the error path.
>
> Right.
>
> Thanks,
>
> Antoine
>
> --
> Antoine Ténart, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v3 2/3] drm: zte: add initial vou drm driver
From: Shawn Guo @ 2016-10-27 14:42 UTC (permalink / raw)
To: Sean Paul, Shawn Guo
Cc: Mark Rutland, devicetree@vger.kernel.org, Daniel Vetter,
Baoyou Xie, Emil Velikov, dri-devel, Rob Herring, Jun Nie,
Linux ARM Kernel
In-Reply-To: <CAOw6vbKbdWJ8eJD3oXwPd6-ykaOu+QP6isqAz8HkjS0p+2wX=w@mail.gmail.com>
> On Thu, Oct 20, 2016 at 3:30 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> > It adds the initial ZTE VOU display controller DRM driver. There are
> > still some features to be added, like overlay plane, scaling, and more
> > output devices support. But it's already useful with dual CRTCs and
> > HDMI monitor working.
> >
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
<snip>
> > +static int zx_drm_bind(struct device *dev)
> > +{
> > + struct drm_device *drm;
> > + struct zx_drm_private *priv;
> > + int ret;
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + drm = drm_dev_alloc(&zx_drm_driver, dev);
> > + if (!drm)
> > + return -ENOMEM;
> > +
> > + drm->dev_private = priv;
> > + dev_set_drvdata(dev, drm);
> > +
> > + drm_mode_config_init(drm);
> > + drm->mode_config.min_width = 16;
> > + drm->mode_config.min_height = 16;
> > + drm->mode_config.max_width = 4096;
> > + drm->mode_config.max_height = 4096;
> > + drm->mode_config.funcs = &zx_drm_mode_config_funcs;
> > +
> > + ret = component_bind_all(dev, drm);
> > + if (ret) {
> > + dev_err(dev, "failed to bind all components: %d\n", ret);
>
> Consider using the new DRM_DEV_* messages instead of the plain old dev_*
Okay, I will switch to DRM_DEV_* for log messages.
>
> > + goto out_unregister;
> > + }
> > +
> > + ret = drm_vblank_init(drm, drm->mode_config.num_crtc);
> > + if (ret < 0) {
> > + dev_err(dev, "failed to init vblank: %d\n", ret);
> > + goto out_unbind;
> > + }
> > +
> > + /*
> > + * We will manage irq handler on our own. In this case, irq_enabled
> > + * need to be true for using vblank core support.
> > + */
> > + drm->irq_enabled = true;
> > +
> > + drm_mode_config_reset(drm);
> > + drm_kms_helper_poll_init(drm);
> > +
> > + priv->fbdev = drm_fbdev_cma_init(drm, 32, drm->mode_config.num_crtc,
> > + drm->mode_config.num_connector);
> > + if (IS_ERR(priv->fbdev)) {
> > + ret = PTR_ERR(priv->fbdev);
> > + dev_err(dev, "failed to init cma fbdev: %d\n", ret);
> > + priv->fbdev = NULL;
> > + goto out_poll_fini;
> > + }
> > +
> > + ret = drm_dev_register(drm, 0);
> > + if (ret)
> > + goto out_fbdev_fini;
> > +
> > + return 0;
> > +
> > +out_fbdev_fini:
> > + if (priv->fbdev) {
> > + drm_fbdev_cma_fini(priv->fbdev);
> > + priv->fbdev = NULL;
> > + }
> > +out_poll_fini:
> > + drm_kms_helper_poll_fini(drm);
> > + drm_mode_config_cleanup(drm);
> > + drm_vblank_cleanup(drm);
> > +out_unbind:
> > + component_unbind_all(dev, drm);
> > +out_unregister:
> > + dev_set_drvdata(dev, NULL);
> > + drm->dev_private = NULL;
> > + drm_dev_unref(drm);
> > + return ret;
> > +}
<snip>
> > +static int zx_hdmi_i2c_read(struct zx_hdmi *hdmi, struct i2c_msg *msg)
> > +{
> > + int len = msg->len;
> > + u8 *buf = msg->buf;
> > + int retry = 0;
> > + int ret = 0;
> > +
> > + /* Bits [9:8] of bytes */
> > + hdmi_writeb(hdmi, ZX_DDC_DIN_CNT2, (len >> 8) & 0xff);
> > + /* Bits [7:0] of bytes */
> > + hdmi_writeb(hdmi, ZX_DDC_DIN_CNT1, len & 0xff);
> > +
> > + /* Clear FIFO */
> > + hdmi_writeb_mask(hdmi, ZX_DDC_CMD, DDC_CMD_MASK, DDC_CMD_CLEAR_FIFO);
> > +
> > + /* Kick off the read */
> > + hdmi_writeb_mask(hdmi, ZX_DDC_CMD, DDC_CMD_MASK,
> > + DDC_CMD_SEQUENTIAL_READ);
> > +
> > + while (len > 0) {
> > + int cnt, i;
> > +
> > + /* FIFO needs some time to get ready */
> > + usleep_range(500, 1000);
> > +
> > + cnt = hdmi_readb(hdmi, ZX_DDC_DOUT_CNT) & DDC_DOUT_CNT_MASK;
> > + if (cnt == 0) {
> > + if (++retry > 5) {
> > + dev_err(hdmi->dev, "DDC FIFO read timed out!");
> > + ret = -ETIMEDOUT;
> > + break;
>
> Why not just return -ETIMEDOUT here?
Yes. Stupid me.
> > + }
> > + continue;
> > + }
> > +
> > + for (i = 0; i < cnt; i++)
> > + *buf++ = hdmi_readb(hdmi, ZX_DDC_DATA);
> > + len -= cnt;
> > + }
> > +
> > + return ret;
> > +}
<snip>
> > +static int zx_hdmi_bind(struct device *dev, struct device *master, void *data)
> > +{
> > + struct platform_device *pdev = to_platform_device(dev);
> > + struct drm_device *drm = data;
> > + struct resource *res;
> > + struct zx_hdmi *hdmi;
> > + int irq;
> > + int ret;
> > +
> > + hdmi = devm_kzalloc(dev, sizeof(*hdmi), GFP_KERNEL);
> > + if (!hdmi)
> > + return -ENOMEM;
> > +
> > + hdmi->dev = dev;
> > + hdmi->drm = drm;
> > + hdmi->inf = &vou_inf_hdmi;
> > +
> > + dev_set_drvdata(dev, hdmi);
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + hdmi->mmio = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(hdmi->mmio)) {
> > + ret = PTR_ERR(hdmi->mmio);
> > + dev_err(dev, "failed to remap hdmi region: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + irq = platform_get_irq(pdev, 0);
> > + if (irq < 0)
> > + return irq;
> > +
> > + hdmi->cec_clk = devm_clk_get(hdmi->dev, "osc_cec");
> > + if (IS_ERR(hdmi->cec_clk)) {
> > + ret = PTR_ERR(hdmi->cec_clk);
> > + dev_err(dev, "failed to get cec_clk: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + hdmi->osc_clk = devm_clk_get(hdmi->dev, "osc_clk");
> > + if (IS_ERR(hdmi->osc_clk)) {
> > + ret = PTR_ERR(hdmi->osc_clk);
> > + dev_err(dev, "failed to get osc_clk: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + hdmi->xclk = devm_clk_get(hdmi->dev, "xclk");
> > + if (IS_ERR(hdmi->xclk)) {
> > + ret = PTR_ERR(hdmi->xclk);
> > + dev_err(dev, "failed to get xclk: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + zx_hdmi_hw_init(hdmi);
> > +
> > + ret = clk_prepare_enable(hdmi->cec_clk);
> > + if (ret) {
> > + dev_err(dev, "failed to enable cec_clk: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + ret = clk_prepare_enable(hdmi->osc_clk);
> > + if (ret) {
> > + dev_err(dev, "failed to enable osc_clk: %d\n", ret);
> > + goto disable_cec_clk;
> > + }
> > +
> > + ret = clk_prepare_enable(hdmi->xclk);
> > + if (ret) {
> > + dev_err(dev, "failed to enable xclk: %d\n", ret);
> > + goto disable_osc_clk;
> > + }
>
> Perhaps add a TODO above hdmi_hw_init() to move it and the clock
> enables to .enable and conversely implement .disable?
Instead of leaving a TODO item there, I would like to change it right
away in the next version.
> > +
> > +
> > + ret = zx_hdmi_ddc_register(hdmi);
> > + if (ret) {
> > + dev_err(dev, "failed to register ddc: %d\n", ret);
> > + goto disable_xclk;
> > + }
> > +
> > + ret = zx_hdmi_register(drm, hdmi);
> > + if (ret) {
> > + dev_err(dev, "failed to register hdmi: %d\n", ret);
> > + goto disable_xclk;
> > + }
> > +
> > + ret = devm_request_threaded_irq(dev, irq, zx_hdmi_irq_handler,
> > + zx_hdmi_irq_thread, IRQF_SHARED,
> > + dev_name(dev), hdmi);
> > + if (ret) {
> > + dev_err(dev, "failed to request threaded irq: %d\n", ret);
> > + goto disable_xclk;
> > + }
> > +
> > + return 0;
> > +
> > +disable_xclk:
> > + clk_disable_unprepare(hdmi->xclk);
> > +disable_osc_clk:
> > + clk_disable_unprepare(hdmi->osc_clk);
> > +disable_cec_clk:
> > + clk_disable_unprepare(hdmi->cec_clk);
> > + return ret;
> > +}
<snip>
> > +static int zx_gl_plane_atomic_check(struct drm_plane *plane,
> > + struct drm_plane_state *plane_state)
> > +{
> > + struct drm_framebuffer *fb = plane_state->fb;
> > + struct drm_crtc *crtc = plane_state->crtc;
> > + struct drm_crtc_state *crtc_state;
> > + struct drm_rect clip;
> > +
> > + if (!crtc || !fb)
> > + return 0;
> > +
> > + crtc_state = drm_atomic_get_existing_crtc_state(plane_state->state,
> > + crtc);
> > + if (WARN_ON(!crtc_state))
> > + return -EINVAL;
> > +
> > + /* plane must match crtc enable state */
> > + if (crtc_state->enable != !!plane_state->crtc)
> > + return -EINVAL;
> > +
> > + /* nothing to check when disabling or disabled */
> > + if (!crtc_state->enable)
> > + return 0;
>
> I think you can shuffle things around here to read a bit clearer:
>
> if (!crtc_state->enable)
> return 0;
>
> if (!plane_state->crtc)
> return -EINVAL
Indeed.
> > +
> > + clip.x1 = 0;
> > + clip.y1 = 0;
> > + clip.x2 = crtc_state->adjusted_mode.hdisplay;
> > + clip.y2 = crtc_state->adjusted_mode.vdisplay;
> > +
> > + return drm_plane_helper_check_state(plane_state, &clip,
> > + DRM_PLANE_HELPER_NO_SCALING,
> > + DRM_PLANE_HELPER_NO_SCALING,
> > + false, true);
> > +}
<snip>
> > +/* Sub-module offset */
> > +#define MAIN_GL_OFFSET 0x130
> > +#define MAIN_CSC_OFFSET 0x580
> > +#define MAIN_HBSC_OFFSET 0x820
> > +#define MAIN_RSZ_OFFSET 0x600 /* OTFPPU sub-module */
> > +
> > +#define AUX_GL_OFFSET 0x200
> > +#define AUX_CSC_OFFSET 0x5d0
> > +#define AUX_HBSC_OFFSET 0x860
> > +#define AUX_RSZ_OFFSET 0x800
> > +
> > +/* OSD (GPC_GLOBAL) registers */
> > +#define OSD_INT_STA 0x04
> > +#define OSD_INT_CLRSTA 0x08
> > +#define OSD_INT_MSK 0x0c
> > +#define OSD_INT_AUX_UPT BIT(14)
> > +#define OSD_INT_MAIN_UPT BIT(13)
> > +#define OSD_INT_GL1_LBW BIT(10)
> > +#define OSD_INT_GL0_LBW BIT(9)
> > +#define OSD_INT_VL2_LBW BIT(8)
> > +#define OSD_INT_VL1_LBW BIT(7)
> > +#define OSD_INT_VL0_LBW BIT(6)
> > +#define OSD_INT_BUS_ERR BIT(3)
> > +#define OSD_INT_CFG_ERR BIT(2)
> > +#define OSD_INT_ERROR (\
> > + OSD_INT_GL1_LBW | OSD_INT_GL0_LBW | \
> > + OSD_INT_VL2_LBW | OSD_INT_VL1_LBW | OSD_INT_VL0_LBW | \
> > + OSD_INT_BUS_ERR | OSD_INT_CFG_ERR \
> > +)
> > +#define OSD_INT_ENABLE (OSD_INT_ERROR | OSD_INT_AUX_UPT | OSD_INT_MAIN_UPT)
> > +#define OSD_CTRL0 0x10
> > +#define OSD_CTRL0_GL0_EN BIT(7)
> > +#define OSD_CTRL0_GL0_SEL BIT(6)
> > +#define OSD_CTRL0_GL1_EN BIT(5)
> > +#define OSD_CTRL0_GL1_SEL BIT(4)
> > +#define OSD_RST_CLR 0x1c
> > +#define RST_PER_FRAME BIT(19)
> > +
> > +/* Main/Aux channel registers */
> > +#define OSD_MAIN_CHN 0x470
> > +#define OSD_AUX_CHN 0x4d0
> > +#define CHN_CTRL0 0x00
> > +#define CHN_ENABLE BIT(0)
> > +#define CHN_CTRL1 0x04
> > +#define CHN_SCREEN_W_SHIFT 18
> > +#define CHN_SCREEN_W_MASK (0x1fff << CHN_SCREEN_W_SHIFT)
> > +#define CHN_SCREEN_H_SHIFT 5
> > +#define CHN_SCREEN_H_MASK (0x1fff << CHN_SCREEN_H_SHIFT)
> > +#define CHN_UPDATE 0x08
> > +
> > +/* TIMING_CTRL registers */
> > +#define TIMING_TC_ENABLE 0x04
> > +#define AUX_TC_EN BIT(1)
> > +#define MAIN_TC_EN BIT(0)
> > +#define FIR_MAIN_ACTIVE 0x08
> > +#define FIR_AUX_ACTIVE 0x0c
> > +#define V_ACTIVE_SHIFT 16
> > +#define V_ACTIVE_MASK (0xffff << V_ACTIVE_SHIFT)
> > +#define H_ACTIVE_SHIFT 0
> > +#define H_ACTIVE_MASK (0xffff << H_ACTIVE_SHIFT)
> > +#define FIR_MAIN_H_TIMING 0x10
> > +#define FIR_MAIN_V_TIMING 0x14
> > +#define FIR_AUX_H_TIMING 0x18
> > +#define FIR_AUX_V_TIMING 0x1c
> > +#define SYNC_WIDE_SHIFT 22
> > +#define SYNC_WIDE_MASK (0x3ff << SYNC_WIDE_SHIFT)
> > +#define BACK_PORCH_SHIFT 11
> > +#define BACK_PORCH_MASK (0x7ff << BACK_PORCH_SHIFT)
> > +#define FRONT_PORCH_SHIFT 0
> > +#define FRONT_PORCH_MASK (0x7ff << FRONT_PORCH_SHIFT)
> > +#define TIMING_CTRL 0x20
> > +#define AUX_POL_SHIFT 3
> > +#define AUX_POL_MASK (0x7 << AUX_POL_SHIFT)
> > +#define MAIN_POL_SHIFT 0
> > +#define MAIN_POL_MASK (0x7 << MAIN_POL_SHIFT)
> > +#define POL_DE_SHIFT 2
> > +#define POL_VSYNC_SHIFT 1
> > +#define POL_HSYNC_SHIFT 0
> > +#define TIMING_INT_CTRL 0x24
> > +#define TIMING_INT_STATE 0x28
> > +#define TIMING_INT_AUX_FRAME BIT(3)
> > +#define TIMING_INT_MAIN_FRAME BIT(1)
> > +#define TIMING_INT_AUX_FRAME_SEL_VSW (0x2 << 10)
> > +#define TIMING_INT_MAIN_FRAME_SEL_VSW (0x2 << 6)
> > +#define TIMING_INT_ENABLE (\
> > + TIMING_INT_MAIN_FRAME_SEL_VSW | TIMING_INT_AUX_FRAME_SEL_VSW | \
> > + TIMING_INT_MAIN_FRAME | TIMING_INT_AUX_FRAME \
> > +)
> > +#define TIMING_MAIN_SHIFT 0x2c
> > +#define TIMING_AUX_SHIFT 0x30
> > +#define H_SHIFT_VAL 0x0048
> > +#define TIMING_MAIN_PI_SHIFT 0x68
> > +#define TIMING_AUX_PI_SHIFT 0x6c
> > +#define H_PI_SHIFT_VAL 0x000f
> > +
> > +#define V_ACTIVE(x) (((x) << V_ACTIVE_SHIFT) & V_ACTIVE_MASK)
> > +#define H_ACTIVE(x) (((x) << H_ACTIVE_SHIFT) & H_ACTIVE_MASK)
> > +
> > +#define SYNC_WIDE(x) (((x) << SYNC_WIDE_SHIFT) & SYNC_WIDE_MASK)
> > +#define BACK_PORCH(x) (((x) << BACK_PORCH_SHIFT) & BACK_PORCH_MASK)
> > +#define FRONT_PORCH(x) (((x) << FRONT_PORCH_SHIFT) & FRONT_PORCH_MASK)
> > +
> > +/* DTRC registers */
> > +#define DTRC_F0_CTRL 0x2c
> > +#define DTRC_F1_CTRL 0x5c
> > +#define DTRC_DECOMPRESS_BYPASS BIT(17)
> > +#define DTRC_DETILE_CTRL 0x68
> > +#define TILE2RASTESCAN_BYPASS_MODE BIT(30)
> > +#define DETILE_ARIDR_MODE_MASK (0x3 << 0)
> > +#define DETILE_ARID_ALL 0
> > +#define DETILE_ARID_IN_ARIDR 1
> > +#define DETILE_ARID_BYP_BUT_ARIDR 2
> > +#define DETILE_ARID_IN_ARIDR2 3
> > +#define DTRC_ARID 0x6c
> > +#define DTRC_ARID3_SHIFT 24
> > +#define DTRC_ARID3_MASK (0xff << DTRC_ARID3_SHIFT)
> > +#define DTRC_ARID2_SHIFT 16
> > +#define DTRC_ARID2_MASK (0xff << DTRC_ARID2_SHIFT)
> > +#define DTRC_ARID1_SHIFT 8
> > +#define DTRC_ARID1_MASK (0xff << DTRC_ARID1_SHIFT)
> > +#define DTRC_ARID0_SHIFT 0
> > +#define DTRC_ARID0_MASK (0xff << DTRC_ARID0_SHIFT)
> > +#define DTRC_DEC2DDR_ARID 0x70
> > +
> > +#define DTRC_ARID3(x) (((x) << DTRC_ARID3_SHIFT) & DTRC_ARID3_MASK)
> > +#define DTRC_ARID2(x) (((x) << DTRC_ARID2_SHIFT) & DTRC_ARID2_MASK)
> > +#define DTRC_ARID1(x) (((x) << DTRC_ARID1_SHIFT) & DTRC_ARID1_MASK)
> > +#define DTRC_ARID0(x) (((x) << DTRC_ARID0_SHIFT) & DTRC_ARID0_MASK)
> > +
> > +/* VOU_CTRL registers */
> > +#define VOU_INF_EN 0x00
> > +#define VOU_INF_CH_SEL 0x04
> > +#define VOU_INF_DATA_SEL 0x08
> > +#define VOU_SOFT_RST 0x14
> > +#define VOU_CLK_SEL 0x18
> > +#define VOU_CLK_GL1_SEL BIT(5)
> > +#define VOU_CLK_GL0_SEL BIT(4)
> > +#define VOU_CLK_REQEN 0x20
> > +#define VOU_CLK_EN 0x24
> > +
> > +/* OTFPPU_CTRL registers */
> > +#define OTFPPU_RSZ_DATA_SOURCE 0x04
> > +
>
> I find the register definitions pretty distracting here (and elsewhere
> in the driver), I suppose my personal preference would be to hide them
> in the headers.
I do not have a strong preference on this. Okay, will do that to make
it easier for you to look at the driver :)
> > +#define GL_NUM 2
> > +#define VL_NUM 3
> > +
> > +enum vou_chn_type {
> > + VOU_CHN_MAIN,
> > + VOU_CHN_AUX,
> > +};
> > +
> > +struct zx_crtc_regs {
> > + u32 fir_active;
> > + u32 fir_htiming;
> > + u32 fir_vtiming;
> > + u32 timing_shift;
> > + u32 timing_pi_shift;
> > +};
<snip>
> > +static inline struct drm_crtc *zx_find_crtc(struct drm_device *drm, int pipe)
> > +{
> > + struct drm_crtc *crtc;
> > + int i = 0;
> > +
> > + list_for_each_entry(crtc, &drm->mode_config.crtc_list, head)
> > + if (i++ == pipe)
> > + return crtc;
> > +
> > + return NULL;
> > +}
>
> We have drm_plane_from_index, it seems like at least 2 drivers would
> benefit from drm_crtc_from_index. Either way, you should probably
> change this code to compare pipe to crtc->index instead of rolling
> your own index counter.
Right. I will change it to use crtc->index at this point. And after
the driver gets landed, I will cook up a patch for drm_crtc_from_index.
> > +int zx_vou_enable_vblank(struct drm_device *drm, unsigned int pipe)
> > +{
> > + struct drm_crtc *crtc;
> > + struct zx_vou_hw *vou;
> > +
> > + crtc = zx_find_crtc(drm, pipe);
> > + if (!crtc)
> > + return 0;
> > +
> > + vou = crtc_to_vou(crtc);
> > +
> > + if (pipe == 0)
> > + zx_writel_mask(vou->timing + TIMING_INT_CTRL,
> > + TIMING_INT_MAIN_FRAME, TIMING_INT_MAIN_FRAME);
> > + else
> > + zx_writel_mask(vou->timing + TIMING_INT_CTRL,
> > + TIMING_INT_AUX_FRAME, TIMING_INT_AUX_FRAME);
> > +
>
> It seems like this could also benefit from crtc_bits/crtc_regs
Yes, you're right.
Thanks for all the comments.
Shawn
> > + return 0;
> > +}
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [RFC PATCH 13/13] of: Remove unused variable overlay_symbols
From: Pantelis Antoniou @ 2016-10-27 14:41 UTC (permalink / raw)
To: frowand.list; +Cc: Rob Herring, devicetree, linux-kernel
In-Reply-To: <1477429146-27039-14-git-send-email-frowand.list@gmail.com>
Hi Frank,
> On Oct 25, 2016, at 23:59 , frowand.list@gmail.com wrote:
>
> From: Frank Rowand <frank.rowand@am.sony.com>
>
> This unused variable is a reminder that symbols in overlays are
> not available to subsequent overlays. If such a feature is
> desired then there are several ways it could be implemented.
>
Please don’t apply that. There’s a patch that actually imports
the symbol table from overlays that subsequent operations
work.
Please see:
https://patchwork.kernel.org/patch/9104701/
> Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
> ---
> drivers/of/resolver.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
> index 3f7cf569c7ea..b48d16200ccd 100644
> --- a/drivers/of/resolver.c
> +++ b/drivers/of/resolver.c
> @@ -272,7 +272,7 @@ static int adjust_local_phandle_references(struct device_node *local_fixups,
> int of_resolve_phandles(struct device_node *overlay)
> {
> struct device_node *child, *local_fixups, *refnode;
> - struct device_node *tree_symbols, *overlay_symbols, *overlay_fixups;
> + struct device_node *tree_symbols, *overlay_fixups;
> struct property *prop;
> const char *refpath;
> phandle phandle, phandle_delta;
> @@ -302,12 +302,9 @@ int of_resolve_phandles(struct device_node *overlay)
> if (err)
> goto err_out;
>
> - overlay_symbols = NULL;
> overlay_fixups = NULL;
>
> for_each_child_of_node(overlay, child) {
> - if (!of_node_cmp(child->name, "__symbols__"))
> - overlay_symbols = child;
> if (!of_node_cmp(child->name, "__fixups__"))
> overlay_fixups = child;
> }
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Regards
— Pantelis
^ permalink raw reply
* Re: Device tree hardware auto-configuration
From: Pantelis Antoniou @ 2016-10-27 14:39 UTC (permalink / raw)
To: Hans de Goede
Cc: Palacios, Hector,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
mark.rutland-5wv7dgnIgG8@public.gmane.org,
pawel.moll-5wv7dgnIgG8@public.gmane.org,
ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org,
galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org
In-Reply-To: <cd2a1ee8-388e-f10d-2c0d-3c0e2b46439d-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Hi Hans,
> On Oct 25, 2016, at 13:15 , Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
> Hi,
>
> On 25-10-16 12:07, Palacios, Hector wrote:
>> Hello,
>>
>> Last ELCE 2016 I attended the conference "Device Tree hardware
>> auto-configuration" by Hans de Goede. Although the room was not very big, it was fully crowded, sign of the big interest in the embedded sector to find a convenient solution to manage hardware changes based on self-detection of the platform.
>>
>> Hans also started a thread recently going over this [1].
>>
>> Examples of what we try to solve are:
>> - The same hardware platform has some hardware differences, for example because there are different revisions of the hardware.
>> - A platform has several slightly different models but a 95% of the device tree
>> is the same (a different GPIO is used to power a regulator, a different model of touch screen controller is used...) and we want to avoid multiple yet very similar DTs.
>>
>> There is no agreement on where these things should be solved.
>> Neither U-Boot nor the kernel are straightforward options.
>>
>> I would like to share an idea. It is just a sketch and I do apologize if it
>> looks naive, but maybe it can be used as foundation for a better solution.
>> In the DT, we have the 'compatible' property to match nodes to drivers.
>> In a similar way, I was wondering if we could have a new property 'hardware-compatible' that matches nodes to variants of the hardware.
>>
>> A simple example:
>> - Board model A with an I2C touchscreen controller
>> - Board model B with a different I2C touchscreen controller (same address)
>>
>> &i2c1 {
>> /* Touchscreen for model A */
>> touch_modelA@28 {
>> compatible = "aaa,touch";
>> hardware-compatible = "model" "A"
>> reg = <0x28>;
>> };
>>
>> /* Touchscreen for model B */
>> touch_modelB@28 {
>> compatible = "bbb,touch";
>> hardware-compatible = "model" "B"
>> reg = <0x28>;
>> };
>> };
>>
>> Then, it would be a question of fixing up the DT at run-time with the hardware identified properties (not with the touch nodes themselves). Let's say we identify that we're running on model A, revision 3 (in e-fuses, eeprom, or by heuristics), so we fixup the DT with root-level properties about the hardware variant:
>>
>> / {
>> hardware,model = "A";
>> hardware,revision = "3";
>> };
>>
>> The names of the properties (after the 'hardware,' prefix) could be anything.
>> The match would need to be made between the remaining property name and its value. The DT fixup might be done by the boot loader or by a misc kernel driver as in Hans' patch. Then the driver's probe would only be called if the hardware-compatible property values are satisfied.
>>
>> The idea behind is to not heavily fixup the DT with devices, but instead limit the fixup to a reduced set of properties. Or in other words, you can still tell what hardware you are supposed to have by looking at the DT, and the fixup (whether done the boot loader or the kernel) simply fills in the variables that determine it.
>>
>> As I said, it's just a sketch-idea that I had floating around since ELCE, and that I wanted to share.
>
> This idea has been discussed before, the more or less common name for it now is devicetree quirks, see:
>
> http://www.spinics.net/lists/devicetree/msg69490.html
> http://www.spinics.net/lists/devicetree/msg69494.html
>
Yeah, so this works for sure. We know we have to come up with something, but there’s
no agreement yet on what that will be.
I think we can start by defining what parts are applicable for a quirks like mechanism
and which parts are better suited by a detection mechanism and application like a manager
later in the boot sequence.
The quirks method has the drawback that you need to do it very early so the kernel is
not up yet.
> For an initial attempt at implementing this.
>
> Regards,
>
> Hans
Regards
— Pantelis
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 5/7] [media] ir-lirc-codec: don't wait any transmitting time for tx only devices
From: Sean Young @ 2016-10-27 14:36 UTC (permalink / raw)
To: Andi Shyti
Cc: Mauro Carvalho Chehab, Rob Herring, Mark Rutland, linux-media,
devicetree, linux-kernel, Andi Shyti, David Härdeman
In-Reply-To: <20161027074401.wxg5icc6hcpwnfsf@gangnam.samsung>
On Thu, Oct 27, 2016 at 04:44:01PM +0900, Andi Shyti wrote:
> Hi Sean,
>
> it's been a while :)
>
> I was going through your review fixing what needs to be fixed,
> but...
>
> > > @@ -153,7 +153,7 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, const char __user *buf,
> > > }
> > >
> > > ret = dev->tx_ir(dev, txbuf, count);
> > > - if (ret < 0)
> > > + if (ret < 0 || dev->driver_type == RC_DRIVER_IR_RAW_TX)
> >
> > Just because a driver only does transmit doesn't mean its transmit ABI
> > should change.
> >
> > Now this bit of code is pretty horrible. It ensures that the call to write()
> > takes at least as long as the length of the transmit IR by sleeping. That's
> > not much of a guarantee that the IR has been sent.
> >
> > Note that in the case of ir-spi, since your spi transfer is sync no sleep
> > should be introduced here.
> >
> > The gap calculation in lirc checks that if the call to write() took _longer_
> > than expected wait before sending the next IR code (when either multiple
> > IR codes or repeats are specified). Introducing the sleep in the kernel
> > here does not help at all, lirc already ensures that it waits as long as
> > the IR is long (see schedule_repeat_timer in lirc).
> >
> > This change was introduced in 3.10, commit f8e00d5.
>
> ... I'm not sure what can be done here. I get your point and I
> understand that this indeed is a kind of fake sync point and by
> doing this I
My original plan was to send a patch which just removes the silly wait,
but on further investigating debian stable and testing still carry a
lirc version that depend on it, so that's not going to fly.
> How about creating two different functions:
>
> - ir_lirc_transmit_ir where we actually do what the function
> already does
> - ir_lirc_transmit_no_sync where the function we don't wait
> because the the sync is done on a different level (for example
> in the SPI case).
>
> SPI does approximately the same thing.
Since we have to be able to switch between waiting and not waiting,
we need some sort of ABI for this. I think this warrants a new ioctl;
I'm not sure how else it can be done. I'll be sending out a patch
shortly.
Sean
^ permalink raw reply
* Re: [RFC PATCH 0/5] Add an overlay manager to handle board capes
From: Antoine Tenart @ 2016-10-27 14:25 UTC (permalink / raw)
To: Rob Herring
Cc: Mark Rutland, Thomas Petazzoni, Dmitry Shmidt,
devicetree@vger.kernel.org, Pantelis Antoniou, Antoine Tenart,
Stephen Boyd, linux-kernel@vger.kernel.org, Hans de Goede,
Maxime Ripard, Frank Rowand, linux-arm-kernel@lists.infradead.org
In-Reply-To: <CAL_JsqL9yWBj0yYE54XGi87YPGugGAACzr=CuW6dk5kk3EuyCA@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 4193 bytes --]
Hi Rob,
On Thu, Oct 27, 2016 at 08:41:56AM -0500, Rob Herring wrote:
> Please Cc the maintainers of drivers/of/.
>
> + Frank R, Hans, Dmitry S
Yes, sorry for that.
> On Wed, Oct 26, 2016 at 9:57 AM, Antoine Tenart
> <antoine.tenart@free-electrons.com> wrote:
> >
> > 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.
We now have overlay support in U-Boot so we could modify the device tree
from the bootloader and not use the command line. But you can argue the
boot loader can't always be upgraded (to support overlays, or to improve
it). So I guess we can think of using the command line as a input
source.
> Another overlay manager was proposed not to long ago[1] as well.
Thanks for the hint.
> Another thing to consider is different sources of overlays. Besides in
> the filesystem, overlays could be built into the kernel (already
> supported), embedded in the dtb (as the other overlay mgr did) or we
> could extend FDT format to append them.
Sure. Using this series it should be quite easy to support other
sources. We would need to improve the function loading the overlay, to
try other sources. This could even comes in following up patches.
> > The proposed design is a library which can be used by detector drivers
> > to parse headers and load the corresponding overlay. Helpers are
> > provided for this purpose. The whole thing is divided into 3 entities:
> >
> > - The parser which is project-specific (to allow supporting headers
> > already into the wild). It registers a function parsing an header's
> > data and filling one or more strings which will be used to find
> > matching dtbo on the fs.
> >
> > - The overlay manager helpers allowing to parse a header to retrieve
> > the previously mentioned strings and to load a compatible overlay.
> >
> > - The detectors which are used to detect capes and get their description
> > (to be parsed).
>
> What about things like power has to be turned on first to detect
> boards and read their ID? I think this needs to be tied into the
> driver model. Though, don't go sticking cape mgr nodes into DT. Maybe
> a driver gets bound to a connector node, but we've got to sort out
> connector bindings first.
Right. I don't know yet how to handle this. Do you have an existing
example in mind of such a power requirement?
> > An example of parser and detector is given, compatible with what's done
> > for the C.H.I.P. As the w1 framework is really bad (and we should
> > probably do something about that) the detector code is far from being
> > perfect; but that's not related to what we try to achieve here.
> >
> > The actual implementation has a limitation: the detectors cannot be
> > built-in the kernel image as they would likely detect capes at boot time
> > but will fail to get their corresponding dt overlays as the fs isn't
> > mounted yet. The only case this can work is when dt overlays are
> > built-in firmwares. This isn't an issue for the C.H.I.P. use case right
> > now. There was a discussion about making an helper to wait for the
> > rootfs to be mount but the answer was "this is the driver's problem".
>
> I thought there are firmware loading calls that will wait. I think
> this all needs to work asynchronously both for firmware loading and
> because w1 is really slow.
There is an asynchronous one, but it will also fail if the rootfs isn't
mounted yet.
Thanks!
Antoine
--
Antoine Ténart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 1/6] dt/bindings: adjust bindings for Layerscape SCFG MSI
From: Mark Rutland @ 2016-10-27 14:18 UTC (permalink / raw)
To: Leo Li
Cc: devicetree@vger.kernel.org, Marc Zyngier, Stuart Yoder,
linux-kernel@vger.kernel.org, M.H. Lian, Mingkai Hu, Scott Wood,
Shawn Guo, linux-arm-kernel@lists.infradead.org
In-Reply-To: <DB4PR04MB07813D4DA5D5567229394A7D8FAB0@DB4PR04MB0781.eurprd04.prod.outlook.com>
On Wed, Oct 26, 2016 at 10:09:07PM +0000, Leo Li wrote:
>
> > -----Original Message-----
> > From: Mark Rutland [mailto:mark.rutland@arm.com]
> > Sent: Wednesday, October 26, 2016 5:31 AM
> > To: M.H. Lian <minghuan.lian@nxp.com>
> > Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > devicetree@vger.kernel.org; Marc Zyngier <marc.zyngier@arm.com>; Stuart
> > Yoder <stuart.yoder@nxp.com>; Leo Li <leoyang.li@nxp.com>; Scott Wood
> > <scott.wood@nxp.com>; Shawn Guo <shawnguo@kernel.org>; Mingkai Hu
> > <mingkai.hu@nxp.com>
> > Subject: Re: [PATCH 1/6] dt/bindings: adjust bindings for Layerscape SCFG MSI
> >
> > On Tue, Oct 25, 2016 at 08:35:40PM +0800, Minghuan Lian wrote:
> > > -- compatible: should be "fsl,<soc-name>-msi" to identify
> > > - Layerscape PCIe MSI controller block such as:
> > > - "fsl,1s1021a-msi"
> > > - "fsl,1s1043a-msi"
> > > +- compatible: should be "fsl,ls-scfg-msi"
> >
> > This breaks old DTBs, and throws away information which you describe above as
> > valuable. So another NAK for that.
>
> I agree with you that we should maintain the backward compatibility.
> But on the other hand, I just found that there is a silly typo in the
> original binding that "ls" is wrongly spelled as "1s" and they look
> too close to be noticed in previous patch reviews. :(
Sure, that's annoying, but we're stuck with it.
> The driver and all the DTSes used the binding with the typo which
> covered up the problem. So even if we want to keep the
> "fsl,<soc-name>-msi" binding, we probably want to fix the typo, right?
> And that breaks the backward compatibility too.
Regardless of what we do, we should *not* break compatibility. The old
strings must remain.
However, we can *add* correctly-spelt variants, and mark the existing
strings as deprecated (in both the binding and driver). The in-kernel
dts can be updated to use the correctly-spelt strings.
Thanks,
Mark.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox