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

[Varshini.Rajendran]

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@xxxxxxxxxxxxx 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@xxxxxxxxxxxxx>
>>>> ---
>>>>    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.

But either way, the behavior is deterministic based on what is queried:

- If it is a valid tag (fourCC code), then it will always exceed any 
realistic packet count, so they reliably fall through to tag lookup.

- If it is a Legacy ID, the lookup will always use id regardless of what 
tag is programmed.

If the tag is programmed as a non-FourCC code, then legacy id access is 
the only way for the user to query the packet.

> 
> Please correct me if I'm wrong.
> 
> Thank you,
> Claudiu


-- 
Thanks,
Varshini Rajendran.