Re: [PATCH v3 06/13] nvmem: microchip-otpc: add tag-based packet lookup

Hi, Varshini,

On 7/27/26 09:24, [email protected] 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, [email protected] 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 <[email protected]>
>>>>> ---
>>>>>     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