RE: [PATCH V1 1/4] bindings: nvmem: introduce "reverse-data" property

[Joakim Zhang <qiangqing.zhang@nxp.com>]

Hi Srinivas,

> -----Original Message-----
> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Sent: 2021年9月3日 20:38
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; Rob Herring
> <robh@kernel.org>
> Cc: shawnguo@kernel.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH V1 1/4] bindings: nvmem: introduce "reverse-data"
> property
> 
> Hi Joakim,
> 
> On 18/08/2021 08:54, Joakim Zhang wrote:
> >
> >> -----Original Message-----
> >> From: Rob Herring <robh@kernel.org>
> >> Sent: 2021年8月18日 3:58
> >> To: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> >> Cc: Joakim Zhang <qiangqing.zhang@nxp.com>; shawnguo@kernel.org;
> >> kernel@pengutronix.de; dl-linux-imx <linux-imx@nxp.com>;
> >> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> >> Subject: Re: [PATCH V1 1/4] bindings: nvmem: introduce "reverse-data"
> >> property
> >>
> >> On Wed, Aug 11, 2021 at 11:16:49AM +0100, Srinivas Kandagatla wrote:
> >>>
> >>>
> >>> On 10/08/2021 08:35, Joakim Zhang wrote:
> >>>> Introduce "reverse-data" property for nvmem provider to reverse buffer.
> >>>>
> >>>> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> >>>> ---
> >>>>    Documentation/devicetree/bindings/nvmem/nvmem.yaml | 5 +++++
> >>>>    1 file changed, 5 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.yaml
> >>>> b/Documentation/devicetree/bindings/nvmem/nvmem.yaml
> >>>> index b8dc3d2b6e92..bc745083fc64 100644
> >>>> --- a/Documentation/devicetree/bindings/nvmem/nvmem.yaml
> >>>> +++ b/Documentation/devicetree/bindings/nvmem/nvmem.yaml
> >>>> @@ -61,6 +61,11 @@ patternProperties:
> >>>>                  description:
> >>>>                    Size in bit within the address range specified by
> reg.
> >>>> +      reverse-data:
> >>>> +        $ref: /schemas/types.yaml#/definitions/flag
> >>>> +        description:
> >>>> +          Reverse the data that read from the storage device.
> >>>> +
> >>>
> >>> This new property is only going to solve one of the reverse order
> >>> issue here.
> >>> If I remember correctly we have mac-address stored in various formats ex:
> >>> from old thread I can see
> >>>
> >>> Type 1: Octets in ASCII without delimiters. (Swapped/non-Swapped)
> >>> Type
> >>> 2: Octets in ASCII with delimiters like (":", ",", ".", "-"... so
> >>> on)
> >>> (Swapped/non-Swapped)
> >>> Type 3: Is the one which stores mac address in Type1/2 but this has
> >>> to be incremented to be used on other instances of eth.
> >>> Type 4: Octets as bytes/u8, swapped/non-swapped
> >>>
> >>> I think its right time to consider adding compatibles to nvmem-cells
> >>> to be able to specify encoding information and handle post processing.
> >>
> >> Yes. Trying to handle this with never ending new properties will end
> >> up with a mess. At some point, you just need code to parse the data.
> >
> > Thanks, Rob.
> >
> > Hi Srinivas,
> >
> Firstly Sorry for taking so long to reply as I was on vacation.
> 
> > Do you plan to implement it?
> 
> No, Am not planning to do this. But am happy to walk-thru the ideas that I
> have.
> 
> >
> > Or need me follow up? If yes, please input your insights to point me how to
> work for it.
> 
> Do we have some kind of meta data/information in nvmem memory to indicate
> the storage encoding?

No, none of these.

> Am I correct to say that this is only issue with mac-address nvmem cell?

I think, yes.
 
> Irrespective of where this encoding info comes from we have 2 options.
> 
> Option 1: Add callback to handle mac-address post-processing with in the
> provider driver.

Sorry, I am not very familiar with nvmem framework, what's this "callback" mean?
Do you also want to introduce a common callback for different vendor drivers to
work for mac-address post-processing? Extend the " struct nvmem_config"?

> Pros:
> - It can deal with vendor specific non-standard encodings, and code is mostly
> with-in vendor specific nvmem provider driver and bindings.
> - will keep nvmem core simple w.r.t handling data.
> 
> Cons:
> - provider driver implement callback and new bindings.
> - might need to add a nvmem-cell-type binding to be able differentiate the cell
> types and handle post-processing.

Ahhh, I am not quite understand how to implement for it? Could you please give some
draft hints?

If we extend the struct nvmem_config, add a callback to handle mac address, how can we
determine which is the mac-address device node? There is no device node info from .reg_read
callback.

> Option 2: nvmem core handles the post processing.
> 
> Pros:
> - provider driver does not need to implement callbacks
> 
> Cons:
> - We have to find a way to define vendor specific non-standard encoding
> information in generic bindings which is going to be a challenge and high chance
> of ending up in to much of clutter in generic bindings.
> 
> Finally, The way I look at this is that once we start adding post-processing in
> nvmem core then we might endup with code that will not be really used for
> most of the usecases and might endup with cases that might not be possible to
> handle in the core.
> 
> 
> Does Option 1 work for you?

Yes, I also prefer to implement it in specific driver, as you mention above, these code are for
very rarely use cases.

If we chose Option 1, I want to implement it totally in specific driver(imx-ocotp.c), and I have a draft,
could it be acdeptable?

--- a/drivers/nvmem/imx-ocotp.c
+++ b/drivers/nvmem/imx-ocotp.c
@@ -76,6 +76,8 @@
 #define IMX_OCOTP_WR_UNLOCK            0x3E770000
 #define IMX_OCOTP_READ_LOCKED_VAL      0xBADABADA

+#define IMX_OCOTP_MAC_MAX              0x2     /* The maximum numbers of MAC instance */
+
 static DEFINE_MUTEX(ocotp_mutex);

 struct ocotp_priv {
@@ -93,11 +95,18 @@ struct ocotp_ctrl_reg {
        u32 bm_rel_shadows;
 };

+struct mac_config {
+       u32 offset;
+       u32 size;
+       bool reverse_byte;
+};
+
 struct ocotp_params {
        unsigned int nregs;
        unsigned int bank_address_words;
        void (*set_timing)(struct ocotp_priv *priv);
        struct ocotp_ctrl_reg ctrl;
+       struct mac_config mac[IMX_OCOTP_MAC_MAX];
 };

 static int imx_ocotp_wait_for_busy(struct ocotp_priv *priv, u32 flags)
@@ -211,6 +220,20 @@ static int imx_ocotp_read(void *context, unsigned int offset,
        }

        index = offset % 4;
+
+       /* Handle MAC address reverse byte if required */
+       for (i = 0; i < IMX_OCOTP_MAC_MAX; i++) {
+               if (offset == priv->params->mac[i].offset &&
+                   bytes == priv->params->mac[i].size &&
+                   priv->params->mac[i].reverse_byte) {
+                       u8 *org = &p[index];
+                       int j;
+
+                       for (j = 0; j < bytes/2; j++)
+                               swap(org[j], org[bytes-j-1]);
+               }
+       }
+
        memcpy(val, &p[index], bytes);

 read_end:
@@ -556,6 +579,12 @@ static const struct ocotp_params imx8mp_params = {
        .bank_address_words = 0,
        .set_timing = imx_ocotp_set_imx6_timing,
        .ctrl = IMX_OCOTP_BM_CTRL_8MP,
+       .mac[0].offset = 0x90,
+       .mac[0].size = 6,
+       .mac[0].reverse_byte = true,
+       .mac[1].offset = 0x96,
+       .mac[1].size = 6,
+       .mac[1].reverse_byte = true,
 };

 static const struct of_device_id imx_ocotp_dt_ids[] = {


Best Regards,
Joakim Zhang