[PATCH v3 06/13] nvmem: microchip-otpc: add tag-based packet lookup
Claudiu Beznea
claudiu.beznea at tuxon.dev
Sun Jul 26 23:40:11 PDT 2026
Hi, Varshini,
On 7/27/26 09:24, Varshini.Rajendran at microchip.com wrote:
> Hi Claudiu,
>
> On 25/07/26 7:57 pm, Claudiu Beznea wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>> the content is safe
>>
>> 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.
>
> Yes. I will do this.
>
>>
>> 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().
>
>
> Yes. That is correct. But since the tag will technically be a FourCC
> code, this problem will not arise. For caution, I can validate the tag
> if it is a FourCC code, before creating the table of contents for the
> packets.
That would be good. Please add this validation.
Thank you,
Claudiu
More information about the linux-arm-kernel
mailing list