From mboxrd@z Thu Jan 1 00:00:00 1970 From: Brian Norris Subject: Re: [PATCH 1/3] mtd: create a partition type device tree binding Date: Sun, 15 Nov 2015 20:12:44 -0800 Message-ID: <20151116041244.GA4021@brian-ubuntu> References: <1446123152-22666-1-git-send-email-linus.walleij@linaro.org> <20151029162908.GA13275@localhost> <20151030175145.GF13239@google.com> <20151113220039.GA74382@google.com> Mime-Version: 1.0 Return-path: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=gnFHOi91Q0nf3nBYTql+g/OL0lhFtD7p4oB5ZrzIJCU=; b=hjKlBnuP/f6WKOFZOA0qW4Gw+dKHtUtRVU/IjaSvfaAk/iHnmFAIg9g5EXtj/5jyBA GcFoH759d50uX1UOyWrMmtdKrhh5Ehmi+LC9Mq8mlWnK5QPh8i89tBXBQN0KpFDriGxV ms10ELkPx361ivWA64naCzVN6AKJhFZ9UdxZ5yFWpzRIgasoGYEBz4cvN713dGiZ0oLH fkqtzvinjlGfGwoGTKqg9AMUDmfVzqzrlc+IGmbixlBSgq4F5WoQSm3GPLnUxS1SSfBJ /GOb4jhR1v3BZb/LRGThqZuYIrRDqm2U8qMuzDH5AtYAEPBZvzxmHeL5krCHjwtIpTcZ w5Vg== Content-Disposition: inline In-Reply-To: Sender: devicetree-spec-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-ID: Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Linus Walleij Cc: Rob Herring , "devicetree-spec-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , David Woodhouse , "linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" , "linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" , "devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Jason Gunthorpe , Liviu Dudau , =?utf-8?B?UmFmYcWCIE1pxYJlY2tp?= , Hauke Mehrtens , Michal Suchanek On Sat, Nov 14, 2015 at 11:46:59AM +0100, Linus Walleij wrote: > On Fri, Nov 13, 2015 at 11:00 PM, Brian Norris > wrote: > > (...) > > (2) we should define a comptible property for the hard-coded > > partitioning case (e.g., compatible = "partitions") > (...) > > If we went with option (2), then ofpart.c could just check only for > > 'compatible = "partitions"' (or similar), and if not found bail out. > > So this "hard-coded partitioning case" the case is where all partitions > are defined in the device tree as described in > Documentation/devicetree/bindings/mtd/partition.txt ? Right. > Or is it a way to indicate that this array > static const char * const part_probe_types_def[] = { > "cmdlinepart", "RedBoot", "ofpart", "ofoldpart", NULL }; > in physmap_of.c should be used? No. At this point, I would consider that to be a legacy method. We still have to support this for many cases (including the non-DT case; but hopefully we can do better than that soon), and that would be an option. > Sorry if I don't fully follow, I'm a bit stupid when it comes to the MTD > helicopter view of the situation.... :( Yeah...sorry if I wasn't too clear. And I definitely don't blame you for not understanding the mess that MTD often is :( > > I think option (2) makes more sense. But it would require an update to > > the binding and code for 4.4, since [1] was only introduced during this > > release cycle. > > Does that mean all old device trees that specify no compatible > string all of a sudden stop working? We can't break the DT ABI, so I > guess not. No, that's not what I was intending. The binding before commmit fe2585e9c29a ("doc: dt: mtd: support partitions in a special 'partitions' subnode") should still stay working as-is. That is, we don't mess with the way things worked for anything that doesn't have a 'partitions' subnode. But now that we have a 'partitions' subnode (in 4.4-rc1), I'm just suggesting that we enforce this always have a compatible property, so we can be more clear on the difference between: partitions { // do I have a // compatible = "partitons"; // here? partition@0 { label = "foo-partition"; reg = <0 0x100000>; }; }; and partitions { compatible = "arm,arm-flash-structure"; subnode { // what if we need something here eventually? }; }; This would require some modifications to partitions.txt and to drivers/mtd/ofpart.c. > A bit confused here, I can't really see what I should do with the patch... Hopefully that cleared up a bit? The code changes for my suggestion would just be something like this, I think. (Not tested in any way.) diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c index 669c3452f278..6811bc5440a4 100644 --- a/drivers/mtd/ofpart.c +++ b/drivers/mtd/ofpart.c @@ -50,6 +50,10 @@ static int parse_ofpart_partitions(struct mtd_info *master, master->name, mtd_node->full_name); ofpart_node = mtd_node; dedicated = false; + } else { + /* The "partitions" subnode may belong to some other parser */ + if (!of_device_is_compatible(ofpart_node, "partitions")) + return 0; } /* First count the subnodes */ I was just bringing this up for discussion, since it's related to your/Rob's new proposal. I'll send a proper (and tested) patch, along with a doc update, if that looks reasonable. Brian