MIME-Version: 1.0
References: <20211123134425.3875656-1-michael@walle.cc> <YaZ5JNCFeKcdIfu8@robh.at.kernel.org>
 <bc05fb3483cef4869ae390096bb95985@walle.cc>
In-Reply-To: <bc05fb3483cef4869ae390096bb95985@walle.cc>
From:   Rob Herring <robh@kernel.org>
Date:   Wed, 1 Dec 2021 10:31:32 -0600
Message-ID: <CAL_JsqL55mZJ6jUyQACer2pKMNDV08-FgwBREsJVgitnuF18Cg@mail.gmail.com>
Subject: Re: [RFC PATCH] dt-bindings: nvmem: add transformation support
To:     Michael Walle <michael@walle.cc>
Cc:     devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
        Frank Rowand <frowand.list@gmail.com>,
        Ansuel Smith <ansuelsmth@gmail.com>,
        Andrew Lunn <andrew@lunn.ch>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Dec 1, 2021 at 8:30 AM Michael Walle <michael@walle.cc> wrote:
>
> Hi Rob,
>
> Am 2021-11-30 20:19, schrieb Rob Herring:
> > On Tue, Nov 23, 2021 at 02:44:25PM +0100, Michael Walle wrote:
> ..
>
> >> Introduce a transformation property. This is intended to be just an
> >> enumeration of operations. If there will be a new operation, support
> >> for
> >> it has to be added to the nvmem core.
> >>
> >> A transformation might have multiple output values, like in the base
> >> mac
> >> address case. It reads the mac address from the nvmem storage and
> >> generates multiple individual addresses, i.e. on our board we reserve
> >> 8
> >> consecutive addresses. These addresses then can be assigned to
> >> different
> >> network interfaces. To make it possible to reference different values
> >> we
> >> need to introduce an argument to the phandle. This additional argument
> >> is then an index into a list of values.
> >
> > I still don't think trying to encode transformations of data into the
> > DT
> > is right approach.
> >
> >>
> >> Example:
> >>   mac_addresses: base-mac-address@10 {
> >>     #nvmem-cell-cells = <1>;
> >>     reg = <10 6>;
> >>     transformation = <NVMEM_T_ETH_OFFSET 0 1 7>;
> >>   }
> >>
> >>   &eth0 {
> >>     nvmem-cells = <&mac_addresses 0>;
> >>     nvmem-cell-names = "mac-address";
> >>   };
> >>
> >>   &eth1 {
> >>     nvmem-cells = <&mac_addresses 2>;
> >>     nvmem-cell-names = "mac-address";
> >>   };
> >>
> >> The NVMEM_T_ETH_OFFSET transformation takes N additional (dt) cells
> >> and
> >> will generate N values. In this example BASE_MAC+0, BASE_MAC+1,
> >> BASE_MAC+7.
> >> An nvmem consumer can then reference the nvmem cell with an index. So
> >> eth0
> >> will get BASE_MAC+0 and eth1 will get BASE_MAC+7.
> >>
> >> This should be sufficient flexible for many different transformations
> >> without having to touch the bindings except for adding documentation
> >> and
> >> checks for new transformations.
> >
> > The content and number of cells is supposed to be opaque to the client
> > and interpreted by the provider. That's sort of true here, but not
> > really because the interpretation is tied to 'transformation'. So I'm
> > okay with adding cells, but not fixing the interpretation of them. A
> > compatible should determine how the cells are interpreted.
>
> What do you mean by "adding cells"? The additional argument to the
> phandle?

Yes.

>
> So an example would be:
>
> ethernet_base_mac: base-mac-address@100 {
>      #nvmem-cell-cells = <1>;
>      compatible = "nvmem-ethernet-address";
>      reg = <0x100 0x6>;
> };
>
> &eth0 {
>      nvmem-cells = <&ethernet_base_mac 0>;
>      nvmem-cell-names = "mac-address";
> };
>
> &eth1 {
>      nvmem-cells = <&ethernet_base_mac 7>;
>      nvmem-cell-names = "mac-address";
> };
>
> Right? Any suggestions for a better compatible name?

I think the compatible should be something platform specific (or
specific to whatever entity defined the format (SoC vendor, OEM, s/w
platform (e.g. OpenWRT, u-boot, etc.)) and perhaps up a level defining
the whole nvmem region. Then the handler for that compatible can
select whatever 'generic' mac address transformation it wants or
implement its own custom one. IOW, specific compatibles can use
generic implementations rather than generic compatibles using a
generic implementation.

Rob