[PATCH v3 06/13] nvmem: microchip-otpc: add tag-based packet lookup
Claudiu Beznea
claudiu.beznea at tuxon.dev
Sat Jul 25 07:27:05 PDT 2026
Hi, Varshini,
On 7/22/26 08:50, Varshini.Rajendran at microchip.com wrote:
> Hi Claudiu,
>
> Thanks for taking the time to review.
>
> On 22/07/26 12:44 am, Claudiu Beznea wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>> the content is safe
>>
>> Hi, Varshini,
>>
>> On 6/30/26 12:35, Varshini Rajendran wrote:
>>> Add support for accessing OTP packets by their 4-byte ASCII tag while
>>> preserving backward compatibility with the existing ID-based lookup.
>>>
>>> The OTP memory layout can vary across devices and may change over time,
>>> making the packet ID approach unreliable when the memory map is not
>>> known in advance. The packet tag provides a reliable way to identify
>>> and access packets without prior knowledge of the OTP memory layout.
>>>
>>> Two offset encoding are now supported:
>>> 1. Legacy ID-based: offset = OTP_PKT(id) = id * 4
>>> Used in DT as: reg = <OTP_PKT(1) 76>;
>>> 2. TAG-based: offset = 4-byte ASCII packet tag
>>> Used in DT as: reg = <0x41435354 0x4c>; (tag "ACST")
>>>
>>
>> I think this:
>>
>>> The driver resolves offsets matching valid legacy selectors (multiples
>>> of 4 within the packet count) through ID lookup, falling back to tag
>>> lookup for other values. This ensures existing device trees continue
>>> to work while enabling new tag-based access.
>>
>> should fall in a different patch?
>
> You mean moving the "invoking the new tag-based method with the legacy
> method as a fallback" part alone? Only add the functions in this patch
> and invoking them in the next one - Did I get it right?
Ah, apologies for confusion, I referred to the wrong section. I wanted to refer
to this section:
>>> The driver also validates OTP memory accessibility and emulation mode
>>> status. When the boot packet is not configured, emulation mode allows
>>> access to the other packets. When both are not available an
>>> informational message is logged.
That is handled through this code:
+
+ tmp = readl_relaxed(otpc->base + MCHP_OTPC_MR);
+ emul_enable = tmp & MCHP_OTPC_MR_EMUL;
+ if (emul_enable)
+ dev_info(otpc->dev, "Emulation mode enabled\n");
+
ret = mchp_otpc_init_packets_list(otpc, &size);
if (ret)
return ret;
+ if (!size) {
+ dev_warn(otpc->dev, "Cannot access OTP memory\n");
+ if (!emul_enable)
+ dev_info(otpc->dev, "Boot packet not programmed and emulation mode disabled\n");
+ }
+
This I think should go in a different patch.
>
>>
>>>
>>> During probe, packet meta data including the tag is read and cached.
>>> The driver also validates OTP memory accessibility and emulation mode
>>> status. When the boot packet is not configured, emulation mode allows
>>> access to the other packets. When both are not available an
>>> informational message is logged.
>>>
>>> The stride of the nvmem memory is set to 1 in order to support tag based
>>> offsets, comment in the header file is updated accordingly.
>>>
>>> Signed-off-by: Varshini Rajendran <varshini.rajendran at microchip.com>
>>> ---
>>> drivers/nvmem/microchip-otpc.c | 143 ++++++++++++++++--
>>> .../nvmem/microchip,sama7g5-otpc.h | 4 +-
>>> 2 files changed, 136 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/nvmem/microchip-otpc.c b/drivers/nvmem/microchip-
>>> otpc.c
>>> index df979e8549fd..bf8589048e17 100644
>>> --- a/drivers/nvmem/microchip-otpc.c
>>> +++ b/drivers/nvmem/microchip-otpc.c
>>> @@ -18,16 +18,20 @@
>>> #define MCHP_OTPC_CR_READ BIT(6)
>>> #define MCHP_OTPC_MR (0x4)
>>> #define MCHP_OTPC_MR_ADDR GENMASK(31, 16)
>>> +#define MCHP_OTPC_MR_EMUL BIT(7)
>>> #define MCHP_OTPC_AR (0x8)
>>> #define MCHP_OTPC_SR (0xc)
>>> #define MCHP_OTPC_SR_READ BIT(6)
>>> #define MCHP_OTPC_HR (0x20)
>>> #define MCHP_OTPC_HR_SIZE GENMASK(15, 8)
>>> +#define MCHP_OTPC_HR_PACKET_TYPE GENMASK(2, 0)
>>
>> Nit: in SAMA7D65 manual this is simply packet. Maybe rename it:
>> MCHP_OTPC_HR_PACKET to match the manual.
>>
>>> #define MCHP_OTPC_DR (0x24)
>>>
>>> #define MCHP_OTPC_NAME "mchp-otpc"
>>> #define MCHP_OTPC_SIZE (11 * 1024)
>>>
>>> +#define PACKET_TYPE_REGULAR 1
>>
>> I would move this close to MCHP_OTPC_HR_PACKET_TYPE and name it
>> something like:
>> MCHP_OTPC_HR_PACKET_REGULAR to match the datasheet.
>>
>>> +
>>> /**
>>> * struct mchp_otpc - OTPC private data structure
>>> * @base: base address
>>> @@ -47,11 +51,15 @@ struct mchp_otpc {
>>> * @list: list head
>>> * @id: packet ID
>>> * @offset: packet offset (in words) in OTP memory
>>> + * @type: type of the packet
>>> + * @tag: 4-byte ASCII tag of the packet
>>> */
>>> struct mchp_otpc_packet {
>>> struct list_head list;
>>> u32 id;
>>> u32 offset;
>>> + u32 type;
>>
>> This can be dropped for now since it's used only in the initialization
>> path.
>>
>>> + u32 tag;
>>> };
>>>
>>> static struct mchp_otpc_packet *mchp_otpc_id_to_packet(struct
>>> mchp_otpc *otpc,
>>> @@ -70,6 +78,56 @@ static struct mchp_otpc_packet
>>> *mchp_otpc_id_to_packet(struct mchp_otpc *otpc,
>>> return NULL;
>>> }
>>>
>>> +/**
>>> + * mchp_otpc_tag_to_packet() - find packet by tag
>>> + * @otpc: OTPC private data
>>> + * @tag: 4-byte ASCII tag to search for
>>> + *
>>> + * Return: pointer to packet if found, NULL otherwise
>>> + */
>>
>> I'm not sure we need this description since the function is simple
>> enough. I
>> would drop it.
>>
>>> +static struct mchp_otpc_packet *mchp_otpc_tag_to_packet(struct
>>> mchp_otpc *otpc,
>>> + u32 tag)
>>> +{
>>> + struct mchp_otpc_packet *packet;
>>> +
>>> + list_for_each_entry(packet, &otpc->packets, list) {
>>> + if (packet->tag == tag)
>>> + return packet;
>>> + }
>>> +
>>> + return NULL;
>>> +}
>>> +
>>> +/**
>>> + * mchp_otpc_resolve_packet() - resolve offset to packet
>>> + * @otpc: OTPC private data
>>> + * @off: NVMEM offset (legacy ID-based or TAG-based)
>>> + *
>>> + * Legacy offsets (multiples of 4 within valid ID range) are resolved
>>> + * through ID lookup. Other offsets are treated as 4-byte ASCII tags.
>>> + *
>>> + * Return: pointer to packet if found, NULL otherwise
>>> + */
>>
>> Same here.
>>
>>> +static struct mchp_otpc_packet *mchp_otpc_resolve_packet(struct
>>> mchp_otpc *otpc,
>>> + u32 off)
>>> +{
>>> + /*
>>> + * Legacy id based packet access: offset = id * 4
>>> + * Inside the driver we use continuous unsigned integer numbers
>>> + * for packet id, thus divide off by 4 before passing it to
>>> + * mchp_otpc_id_to_packet().
>>> + */
>>> + u32 id = off / 4;
>>> +
>>> + if (!(off % 4) && id < otpc->npackets)
>>
>> The tag can be anything, no? Can't the tag satisfy this condition and the
>> execution to wrongly use mchp_otpc_id_to_packet() ?
>
> The tag is a FourCC code. So the minimum value would be 0x20202020 which
> way too big than the number of packets the OTP could hold. So this
> condition pretty much never fails. If you want me to add this
> "0x20202020" value as an additional check I can do that.
Please mention FourCC in patch description.
Also, I think one would manage to access a packet with a tag that is not in
FourCC format (tag >= 0x20202020). E.g. if the memory footprint is:
Header 0
-----------
Payload 0:
tag0 = 0x20202020
-----------
Header 1
-----------
Payload 1:
tag1 = 0x00002020
------------
Header 2
------------
Payload 2:
tag2 = 0x00000004
-----------
Header 3
-----------
Payload 3:
tag3 = 0x00002020
-----------
Header 4
-----------
Payload 4:
tag3 = 0x00002020
Since the initialization function mchp_otpc_read_packet_tag() don't validate the
FourCC format with the minimal FourCC value, it will create a list with 5
packets as follows:
packet0:
- id = 0
- tag = 0x20202020
packet1:
- id = 0
- tag = 0x00002020
packet2:
- id = 2
- tag = 0x00000004
packet3:
- id = 3
- tag = 0x00002020
packet4:
- id = 4
- tag = 0x00002020
Thus, reaching mchp_otpc_read() with off=0x00002020 will allow the user to get
packet1 using tag method (even though the user flashed multiple packets with the
same tag; I'm not sure if we should validate this).
At the same time if off=0x00000004 (corresponding to packet2) the code will
return the payload of packet1 as it will resolve the packet using
mchp_otpc_id_to_packet().
Please correct me if I'm wrong.
Thank you,
Claudiu
More information about the linux-arm-kernel
mailing list