linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Michael Walle <michael@walle.cc>
To: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: "Richard Weinberger" <richard@nod.at>,
	"Vignesh Raghavendra" <vigneshr@ti.com>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
	"Srinivas Kandagatla" <srinivas.kandagatla@linaro.org>,
	"Shawn Guo" <shawnguo@kernel.org>, "Li Yang" <leoyang.li@nxp.com>,
	"Rafał Miłecki" <rafal@milecki.pl>,
	"Frank Rowand" <frowand.list@gmail.com>,
	linux-mtd@lists.infradead.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	"Ahmad Fatoum" <a.fatoum@pengutronix.de>,
	"Philipp Zabel" <p.zabel@pengutronix.de>,
	"Robert Marko" <robert.marko@sartura.hr>,
	"Thomas Petazzoni" <thomas.petazzoni@bootlin.com>
Subject: Re: [PATCH v2 00/20] nvmem: core: introduce NVMEM layouts
Date: Fri, 23 Sep 2022 19:28:45 +0200	[thread overview]
Message-ID: <0c0522a2c7e4b2401955acf5bcd4eb8e@walle.cc> (raw)
In-Reply-To: <20220923174759.299e504e@xps-13>

Hi,

Am 2022-09-23 17:47, schrieb Miquel Raynal:
> I have a few additional questions regarding the bindings.
> 
> michael@walle.cc wrote on Fri,  2 Sep 2022 00:18:37 +0200:
> 
>> This is now the third attempt to fetch the MAC addresses from the VPD
>> for the Kontron sl28 boards. Previous discussions can be found here:
>> https://lore.kernel.org/lkml/20211228142549.1275412-1-michael@walle.cc/
>> 
>> 
>> NVMEM cells are typically added by board code or by the devicetree. 
>> But
>> as the cells get more complex, there is (valid) push back from the
>> devicetree maintainers to not put that handling in the devicetree.
>> 
>> Therefore, introduce NVMEM layouts. They operate on the NVMEM device 
>> and
>> can add cells during runtime. That way it is possible to add more 
>> complex
>> cells than it is possible right now with the offset/length/bits
>> description in the device tree. For example, you can have post 
>> processing
>> for individual cells (think of endian swapping, or ethernet offset
>> handling).
>> 
>> The imx-ocotp driver is the only user of the global post processing 
>> hook,
>> convert it to nvmem layouts and drop the global post pocessing hook. 
>> Please
>> note, that this change is only compile-time tested.
>> 
>> You can also have cells which have no static offset, like the
>> ones in an u-boot environment. The last patches will convert the 
>> current
>> u-boot environment driver to a NVMEM layout and lifting the 
>> restriction
>> that it only works with mtd devices. But as it will change the 
>> required
>> compatible strings, it is marked as RFC for now. It also needs to have
>> its device tree schema update which is left out here. These two 
>> patches
>> are not expected to be applied, but rather to show another example of
>> how to use the layouts.
>> 
>> For now, the layouts are selected by a specific compatible string in a
>> device tree. E.g. the VPD on the kontron sl28 do (within a SPI flash 
>> node):
>>   compatible = "kontron,sl28-vpd", "user-otp";
>> or if you'd use the u-boot environment (within an MTD patition):
>>   compatible = "u-boot,env", "nvmem";
>> 
>> The "user-otp" (or "nvmem") will lead to a NVMEM device, the
>> "kontron,sl28-vpd" (or "u-boot,env") will then apply the specific 
>> layout
>> on top of the NVMEM device.
> 
> So if I understand correctly, there should be:
> - one DT node defining the storage medium eeprom/mtd/whatever,
> - another DT node defining the nvmem device with two compatibles, the
>   "nvmem" (or "user-otp") and the layout.
> Is this correct? Actually I was a bit surprised because generally
> speaking, DT maintainers (rightfully) do not want to describe how we
> use devices, the nvmem abstraction looks like a Linux thing when on top
> of mtd devices for instance, so I just wanted to confirm this point.

What do you mean by two nodes? Two separate ones or one being a
subnode of the other?

There is only one (storage) node and nvmem cells as subnodes.
The two compatibles aren't strictly needed. But it will simplify
the drivers in linux greatly. Otherwise the storage driver would
need to know for which compatibles it needs to register a
nvmem device. E.g. MTD devices determine that by the "nvmem"
compatible. The OTP driver will probe by "{user,factory}-otp".
If you'd only have one compatible, the storage driver would need
a list of all the layouts so it can register a nvmem device.

But also from a device tree POV this makes sense IMHO because
the second compatible is a more specific one. With only
the more generic compatible you just get a nvmem device without
any cells - or only the cells described in the device tree.

Regarding "describe how the devices are used": then there shouldn't
be nvmem (cell) bindings at all, because you are actually describing
how you are using the nvmem provider. So IMHO having for example
the compatible "kontron,sl28-vpd" is actually fits more than having
a nvmem provider compatible with cells subnodes.

> Then, as we have an nvmem device described in the DT, why not just
> pointing at the nvmem device from the cell consumer, rather than still
> having the need to define all the cells that the nvmem device will
> produce in the DT?

See also
https://lore.kernel.org/linux-devicetree/4bf16e18-1591-8bc9-7c46-649391de3761@linaro.org/

> Maybe an example to show what I mean. Here is the current way:
> 
> nvmem_provider: nvmem-provider {
> 	properties;
> 
> 	mycell: my_cell {
> 		[properties;]
> 	};
> };
> 
> And we point to a cell with:
> 
> 	nvmem-cells = <&mycell>;
> 
> But, as for the tlv tables, there are many cells that will be produced,
> and the driver may anyway just ask for the cell name (eg. performing a
> lookup of the "mac-address" cell name), so why bothering to describe 
> all
> the cells in the DT, like:
> 
> 	nvmem-cells-providers = <&nvmem_provider>;
> 
> What do you think?

Ok, you even go one step further and even removing the argument
of the phandle and you are proposing to use the nvmem-cell-name,
right? That might work with simple cells created by a layout. But
what if there are two consumers with different names for the same
cell? Consumer bindings might already be present, e.g. the ethernet
bindings will use "mac-address". What if there is another binding
which want to use that cell but doesn't name it "mac-address"?
IMHO to reference a nvmem cell you shouldn't rely on the consumer.

Also keep in mind, that the number of arguments is fixed and given
by the "#.*-cells" property found on the target node. Therefore,
that won't work if you have cells where one has an argument and
another don't.

> Maybe for the mac addresses this is a bit limiting as, in practice, we
> often have base mac addresses available and using:
> 
> 	nvmem-cells = <&mycell INDEX>;
> 
> allows to dynamically create many different mac addresses, but I wonder
> if the approach would be interesting for other cell types. Just an open
> question.

So how would your idea work with that? Maybe we could support both?
But again, I'm not sure if it is a good idea to mix the consumer
with the provider.

-michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

      reply	other threads:[~2022-09-23 17:29 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-01 22:18 [PATCH v2 00/20] nvmem: core: introduce NVMEM layouts Michael Walle
2022-09-01 22:18 ` [PATCH v2 01/20] net: add helper eth_addr_add() Michael Walle
2022-09-01 23:22   ` Randy Dunlap
2022-09-02  7:27     ` Michael Walle
2022-09-01 22:18 ` [PATCH v2 02/20] of: base: add of_parse_phandle_with_optional_args() Michael Walle
2022-09-01 22:18 ` [PATCH v2 03/20] nvmem: core: add an index parameter to the cell Michael Walle
2022-09-09  8:12   ` Srinivas Kandagatla
2022-09-01 22:18 ` [PATCH v2 04/20] nvmem: core: move struct nvmem_cell_info to nvmem-provider.h Michael Walle
2022-09-01 22:18 ` [PATCH v2 05/20] nvmem: core: drop the removal of the cells in nvmem_add_cells() Michael Walle
2022-09-01 22:18 ` [PATCH v2 06/20] nvmem: core: add nvmem_add_one_cell() Michael Walle
2022-09-09  8:52   ` Srinivas Kandagatla
2022-09-01 22:18 ` [PATCH v2 07/20] nvmem: core: use nvmem_add_one_cell() in nvmem_add_cells_from_of() Michael Walle
2022-09-01 22:18 ` [PATCH v2 08/20] nvmem: core: introduce NVMEM layouts Michael Walle
2022-09-01 22:18 ` [PATCH v2 09/20] nvmem: core: add per-cell post processing Michael Walle
2022-09-01 22:18 ` [PATCH v2 10/20] nvmem: core: allow to modify a cell before adding it Michael Walle
2022-09-01 22:18 ` [PATCH v2 11/20] nvmem: imx-ocotp: replace global post processing with layouts Michael Walle
2022-09-07 12:31   ` Michael Walle
2022-09-01 22:18 ` [PATCH v2 12/20] nvmem: cell: drop global cell_post_process Michael Walle
2022-09-01 22:18 ` [PATCH v2 13/20] nvmem: core: drop priv pointer in post process callback Michael Walle
2022-09-09  8:52   ` Srinivas Kandagatla
2022-09-09  8:58     ` Michael Walle
2022-09-09  9:08       ` Srinivas Kandagatla
2022-09-09  9:39         ` Michael Walle
2022-09-01 22:18 ` [PATCH v2 14/20] dt-bindings: mtd: relax the nvmem compatible string Michael Walle
2022-09-08 12:21   ` Krzysztof Kozlowski
2022-09-01 22:18 ` [PATCH v2 15/20] dt-bindings: nvmem: add YAML schema for the sl28 vpd layout Michael Walle
2022-09-08 12:22   ` Krzysztof Kozlowski
2022-09-12 19:20   ` Rob Herring
2022-09-13 14:21     ` Michael Walle
2022-09-22  9:22       ` Miquel Raynal
2022-09-01 22:18 ` [PATCH v2 16/20] nvmem: layouts: add sl28vpd layout Michael Walle
2022-09-01 22:18 ` [PATCH v2 17/20] nvmem: core: export nvmem device size Michael Walle
2022-09-01 22:18 ` [PATCH v2 18/20] arm64: dts: ls1028a: sl28: get MAC addresses from VPD Michael Walle
2022-09-01 22:18 ` [RFC PATCH v2 19/20] nvmem: layouts: rewrite the u-boot-env driver as a NVMEM layout Michael Walle
2022-09-21 10:37   ` Miquel Raynal
2022-09-01 22:18 ` [RFC PATCH v2 20/20] nvmem: layouts: u-boot-env: add device node Michael Walle
2022-09-21  9:58 ` [PATCH v2 00/20] nvmem: core: introduce NVMEM layouts Miquel Raynal
2022-09-22 21:22   ` Srinivas Kandagatla
2022-09-23  8:31     ` Miquel Raynal
2022-09-23 15:47 ` Miquel Raynal
2022-09-23 17:28   ` Michael Walle [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0c0522a2c7e4b2401955acf5bcd4eb8e@walle.cc \
    --to=michael@walle.cc \
    --cc=a.fatoum@pengutronix.de \
    --cc=devicetree@vger.kernel.org \
    --cc=frowand.list@gmail.com \
    --cc=krzysztof.kozlowski+dt@linaro.org \
    --cc=leoyang.li@nxp.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=miquel.raynal@bootlin.com \
    --cc=p.zabel@pengutronix.de \
    --cc=rafal@milecki.pl \
    --cc=richard@nod.at \
    --cc=robert.marko@sartura.hr \
    --cc=robh+dt@kernel.org \
    --cc=shawnguo@kernel.org \
    --cc=srinivas.kandagatla@linaro.org \
    --cc=thomas.petazzoni@bootlin.com \
    --cc=vigneshr@ti.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).