Hi Srinivas,
> -----Original Message-----
> From: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> Sent: 2021年9月3日 20:38
> To: Joakim Zhang <qiangqing.zhang@xxxxxxx>; Rob Herring
> <robh@xxxxxxxxxx>
> Cc: shawnguo@xxxxxxxxxx; kernel@xxxxxxxxxxxxxx; dl-linux-imx
> <linux-imx@xxxxxxx>; devicetree@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxx>
> >> Sent: 2021年8月18日 3:58
> >> To: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> >> Cc: Joakim Zhang <qiangqing.zhang@xxxxxxx>; shawnguo@xxxxxxxxxx;
> >> kernel@xxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>;
> >> devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> >> 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@xxxxxxx>
> >>>> ---
> >>>> 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
