[v9,6/6] mtd: spi-nor: add support for Macronix Octal flash

Commit Message

Alvin Zhou July 18, 2024, 3:46 a.m. UTC

From: AlvinZhou <[e-mail]>

Adding Manufacture ID 0xC2 in last of ID table because of
Octal Flash need manufacturer fixup for enabling/disabling
Octal DTR mode.

Signed-off-by: JaimeLiao <[e-mail]>
Signed-off-by: AlvinZhou <[e-mail]>
---
 drivers/mtd/spi-nor/macronix.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Tudor Ambarus Sept. 23, 2024, 6:53 a.m. UTC | #1

Hi, Alvin,

I quickly skimmed over the previous 5 patches and they are looking fine.

I don't get this patch however.

On 7/18/24 4:46 AM, AlvinZhou wrote:
> From: AlvinZhou <[e-mail]>
> 
> Adding Manufacture ID 0xC2 in last of ID table because of
> Octal Flash need manufacturer fixup for enabling/disabling
> Octal DTR mode.
> 
> Signed-off-by: JaimeLiao <[e-mail]>
> Signed-off-by: AlvinZhou <[e-mail]>
> ---
>  drivers/mtd/spi-nor/macronix.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
> index f039819a5252..1a8ccebdfe0e 100644
> --- a/drivers/mtd/spi-nor/macronix.c
> +++ b/drivers/mtd/spi-nor/macronix.c
> @@ -200,7 +200,9 @@ static const struct flash_info macronix_nor_parts[] = {
>  		.name = "mx25l3255e",
>  		.size = SZ_4M,
>  		.no_sfdp_flags = SECT_4K,
> -	}
> +	},
> +	/* Need the manufacturer fixups, Keep this last */
> +	{ .id = SNOR_ID(0xc2) }
>  };
>  

Could you please elaborate why you need just the manufacturer id here? I
would have expected to see a specific flash entry instead.

Thanks,
ta

Michael Walle Sept. 23, 2024, 7:18 a.m. UTC | #2

Hi,

> > --- a/drivers/mtd/spi-nor/macronix.c
> > +++ b/drivers/mtd/spi-nor/macronix.c
> > @@ -200,7 +200,9 @@ static const struct flash_info macronix_nor_parts[] = {
> >  		.name = "mx25l3255e",
> >  		.size = SZ_4M,
> >  		.no_sfdp_flags = SECT_4K,
> > -	}
> > +	},
> > +	/* Need the manufacturer fixups, Keep this last */
> > +	{ .id = SNOR_ID(0xc2) }
> >  };
> >  
>
> Could you please elaborate why you need just the manufacturer id here? I
> would have expected to see a specific flash entry instead.

I can't find the thread anymore as this is going on for forever, but
that was my suggestion. That's a catch all to have the manufacturer
fixup applied for any macronix flashes instead of having to list
each flash id here.

-michael

Alvin Zhou Sept. 24, 2024, 3:25 a.m. UTC | #3

Hi Tudor,

Tudor Ambarus <[e-mail]> 於 2024年9月23日 週一 下午2:54寫道：
>
> Hi, Alvin,
>
> I quickly skimmed over the previous 5 patches and they are looking fine.
>
> I don't get this patch however.
>
> On 7/18/24 4:46 AM, AlvinZhou wrote:
> > From: AlvinZhou <[e-mail]>
> >
> > Adding Manufacture ID 0xC2 in last of ID table because of
> > Octal Flash need manufacturer fixup for enabling/disabling
> > Octal DTR mode.
> >
> > Signed-off-by: JaimeLiao <[e-mail]>
> > Signed-off-by: AlvinZhou <[e-mail]>
> > ---
> >  drivers/mtd/spi-nor/macronix.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
> > index f039819a5252..1a8ccebdfe0e 100644
> > --- a/drivers/mtd/spi-nor/macronix.c
> > +++ b/drivers/mtd/spi-nor/macronix.c
> > @@ -200,7 +200,9 @@ static const struct flash_info macronix_nor_parts[] = {
> >               .name = "mx25l3255e",
> >               .size = SZ_4M,
> >               .no_sfdp_flags = SECT_4K,
> > -     }
> > +     },
> > +     /* Need the manufacturer fixups, Keep this last */
> > +     { .id = SNOR_ID(0xc2) }
> >  };
> >
>
> Could you please elaborate why you need just the manufacturer id here? I
> would have expected to see a specific flash entry instead.

Grateful to Michael for the valuable suggestion. This addition of the
Macronix manufacturer ID enables the fixup functions such as
macronix_nor_set_octal_dtr to be executed without the need to
create separate ID entries for each Octal DTR NOR Flash in the
flash_info.

>
> Thanks,
> ta

Thanks,
Alvin

Tudor Ambarus Sept. 24, 2024, 6:26 a.m. UTC | #4

On 9/24/24 4:25 AM, Alvin Zhou wrote:
> Hi Tudor,
> 
> Tudor Ambarus <[e-mail]> 於 2024年9月23日 週一 下午2:54寫道：
>>
>> Hi, Alvin,
>>
>> I quickly skimmed over the previous 5 patches and they are looking fine.
>>
>> I don't get this patch however.
>>
>> On 7/18/24 4:46 AM, AlvinZhou wrote:
>>> From: AlvinZhou <[e-mail]>
>>>
>>> Adding Manufacture ID 0xC2 in last of ID table because of
>>> Octal Flash need manufacturer fixup for enabling/disabling
>>> Octal DTR mode.
>>>
>>> Signed-off-by: JaimeLiao <[e-mail]>
>>> Signed-off-by: AlvinZhou <[e-mail]>
>>> ---
>>>  drivers/mtd/spi-nor/macronix.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
>>> index f039819a5252..1a8ccebdfe0e 100644
>>> --- a/drivers/mtd/spi-nor/macronix.c
>>> +++ b/drivers/mtd/spi-nor/macronix.c
>>> @@ -200,7 +200,9 @@ static const struct flash_info macronix_nor_parts[] = {
>>>               .name = "mx25l3255e",
>>>               .size = SZ_4M,
>>>               .no_sfdp_flags = SECT_4K,
>>> -     }
>>> +     },
>>> +     /* Need the manufacturer fixups, Keep this last */
>>> +     { .id = SNOR_ID(0xc2) }
>>>  };
>>>
>>
>> Could you please elaborate why you need just the manufacturer id here? I
>> would have expected to see a specific flash entry instead.
> 
> Grateful to Michael for the valuable suggestion. This addition of the
> Macronix manufacturer ID enables the fixup functions such as
> macronix_nor_set_octal_dtr to be executed without the need to
> create separate ID entries for each Octal DTR NOR Flash in the
> flash_info.
> 

Ah, nice. Okay then. I'm going to review the rest of the patches. They
look promising ;).

Tudor Ambarus Sept. 24, 2024, 6:36 a.m. UTC | #5

On 9/24/24 7:26 AM, Tudor Ambarus wrote:
> 
> 
> On 9/24/24 4:25 AM, Alvin Zhou wrote:
>> Hi Tudor,
>>
>> Tudor Ambarus <[e-mail]> 於 2024年9月23日 週一 下午2:54寫道：
>>>
>>> Hi, Alvin,
>>>
>>> I quickly skimmed over the previous 5 patches and they are looking fine.
>>>
>>> I don't get this patch however.
>>>
>>> On 7/18/24 4:46 AM, AlvinZhou wrote:
>>>> From: AlvinZhou <[e-mail]>
>>>>
>>>> Adding Manufacture ID 0xC2 in last of ID table because of
>>>> Octal Flash need manufacturer fixup for enabling/disabling
>>>> Octal DTR mode.
>>>>
>>>> Signed-off-by: JaimeLiao <[e-mail]>
>>>> Signed-off-by: AlvinZhou <[e-mail]>
>>>> ---
>>>>  drivers/mtd/spi-nor/macronix.c | 4 +++-
>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
>>>> index f039819a5252..1a8ccebdfe0e 100644
>>>> --- a/drivers/mtd/spi-nor/macronix.c
>>>> +++ b/drivers/mtd/spi-nor/macronix.c
>>>> @@ -200,7 +200,9 @@ static const struct flash_info macronix_nor_parts[] = {
>>>>               .name = "mx25l3255e",
>>>>               .size = SZ_4M,
>>>>               .no_sfdp_flags = SECT_4K,
>>>> -     }
>>>> +     },
>>>> +     /* Need the manufacturer fixups, Keep this last */
>>>> +     { .id = SNOR_ID(0xc2) }
>>>>  };
>>>>
>>>
>>> Could you please elaborate why you need just the manufacturer id here? I
>>> would have expected to see a specific flash entry instead.
>>
>> Grateful to Michael for the valuable suggestion. This addition of the
>> Macronix manufacturer ID enables the fixup functions such as
>> macronix_nor_set_octal_dtr to be executed without the need to
>> create separate ID entries for each Octal DTR NOR Flash in the
>> flash_info.
>>
> 
> Ah, nice. Okay then. I'm going to review the rest of the patches. They
> look promising ;).

ah, but then you'll always have a matched ID, so you break the generic
flash probing for macronix. Is that correct?

Tudor Ambarus Sept. 24, 2024, 7:17 a.m. UTC | #6

On 9/24/24 7:36 AM, Tudor Ambarus wrote:
> 
> 
> On 9/24/24 7:26 AM, Tudor Ambarus wrote:
>>
>>
>> On 9/24/24 4:25 AM, Alvin Zhou wrote:
>>> Hi Tudor,
>>>
>>> Tudor Ambarus <[e-mail]> 於 2024年9月23日 週一 下午2:54寫道：
>>>>
>>>> Hi, Alvin,
>>>>
>>>> I quickly skimmed over the previous 5 patches and they are looking fine.
>>>>
>>>> I don't get this patch however.
>>>>
>>>> On 7/18/24 4:46 AM, AlvinZhou wrote:
>>>>> From: AlvinZhou <[e-mail]>
>>>>>
>>>>> Adding Manufacture ID 0xC2 in last of ID table because of
>>>>> Octal Flash need manufacturer fixup for enabling/disabling
>>>>> Octal DTR mode.
>>>>>
>>>>> Signed-off-by: JaimeLiao <[e-mail]>
>>>>> Signed-off-by: AlvinZhou <[e-mail]>
>>>>> ---
>>>>>  drivers/mtd/spi-nor/macronix.c | 4 +++-
>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
>>>>> index f039819a5252..1a8ccebdfe0e 100644
>>>>> --- a/drivers/mtd/spi-nor/macronix.c
>>>>> +++ b/drivers/mtd/spi-nor/macronix.c
>>>>> @@ -200,7 +200,9 @@ static const struct flash_info macronix_nor_parts[] = {
>>>>>               .name = "mx25l3255e",
>>>>>               .size = SZ_4M,
>>>>>               .no_sfdp_flags = SECT_4K,
>>>>> -     }
>>>>> +     },
>>>>> +     /* Need the manufacturer fixups, Keep this last */
>>>>> +     { .id = SNOR_ID(0xc2) }
>>>>>  };
>>>>>
>>>>
>>>> Could you please elaborate why you need just the manufacturer id here? I
>>>> would have expected to see a specific flash entry instead.
>>>
>>> Grateful to Michael for the valuable suggestion. This addition of the
>>> Macronix manufacturer ID enables the fixup functions such as
>>> macronix_nor_set_octal_dtr to be executed without the need to
>>> create separate ID entries for each Octal DTR NOR Flash in the
>>> flash_info.
>>>
>>
>> Ah, nice. Okay then. I'm going to review the rest of the patches. They
>> look promising ;).
> 
> ah, but then you'll always have a matched ID, so you break the generic
> flash probing for macronix. Is that correct?

Answering myself: it's fine. Generic flash probe just fills a name,
which we don't really care about.

Miquel Raynal Sept. 25, 2024, 9:57 a.m. UTC | #7

Hi Alvin,

[e-mail] wrote on Tue, 24 Sep 2024 08:17:26 +0100:

> On 9/24/24 7:36 AM, Tudor Ambarus wrote:
> > 
> > 
> > On 9/24/24 7:26 AM, Tudor Ambarus wrote:  
> >>
> >>
> >> On 9/24/24 4:25 AM, Alvin Zhou wrote:  
> >>> Hi Tudor,
> >>>
> >>> Tudor Ambarus <[e-mail]> 於 2024年9月23日 週一 下午2:54寫道：  
> >>>>
> >>>> Hi, Alvin,
> >>>>
> >>>> I quickly skimmed over the previous 5 patches and they are looking fine.
> >>>>
> >>>> I don't get this patch however.
> >>>>
> >>>> On 7/18/24 4:46 AM, AlvinZhou wrote:  
> >>>>> From: AlvinZhou <[e-mail]>
> >>>>>
> >>>>> Adding Manufacture ID 0xC2 in last of ID table because of
> >>>>> Octal Flash need manufacturer fixup for enabling/disabling
> >>>>> Octal DTR mode.
> >>>>>
> >>>>> Signed-off-by: JaimeLiao <[e-mail]>
> >>>>> Signed-off-by: AlvinZhou <[e-mail]>
> >>>>> ---
> >>>>>  drivers/mtd/spi-nor/macronix.c | 4 +++-
> >>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
> >>>>> index f039819a5252..1a8ccebdfe0e 100644
> >>>>> --- a/drivers/mtd/spi-nor/macronix.c
> >>>>> +++ b/drivers/mtd/spi-nor/macronix.c
> >>>>> @@ -200,7 +200,9 @@ static const struct flash_info macronix_nor_parts[] = {
> >>>>>               .name = "mx25l3255e",
> >>>>>               .size = SZ_4M,
> >>>>>               .no_sfdp_flags = SECT_4K,
> >>>>> -     }
> >>>>> +     },
> >>>>> +     /* Need the manufacturer fixups, Keep this last */
> >>>>> +     { .id = SNOR_ID(0xc2) }
> >>>>>  };
> >>>>>  
> >>>>
> >>>> Could you please elaborate why you need just the manufacturer id here? I
> >>>> would have expected to see a specific flash entry instead.  
> >>>
> >>> Grateful to Michael for the valuable suggestion. This addition of the
> >>> Macronix manufacturer ID enables the fixup functions such as
> >>> macronix_nor_set_octal_dtr to be executed without the need to
> >>> create separate ID entries for each Octal DTR NOR Flash in the
> >>> flash_info.
> >>>  
> >>
> >> Ah, nice. Okay then. I'm going to review the rest of the patches. They
> >> look promising ;).  
> > 
> > ah, but then you'll always have a matched ID, so you break the generic
> > flash probing for macronix. Is that correct?  
> 
> Answering myself: it's fine. Generic flash probe just fills a name,
> which we don't really care about.

I was also a bit surprised by the diff, would you mind filling a more
complete with details on the actual goal of this entry (having all
Macronix flashes to get the fixups, without creating separate ID
entries for each of the flashes)

Thanks,
Miquèl

Alvin Zhou Sept. 26, 2024, 3:06 a.m. UTC | #8

Hi Miquel,

Miquel Raynal <[e-mail]> 於 2024年9月25日 週三 下午5:57寫道：
>
> Hi Alvin,
>
> [e-mail] wrote on Tue, 24 Sep 2024 08:17:26 +0100:
>
> > On 9/24/24 7:36 AM, Tudor Ambarus wrote:
> > >
> > >
> > > On 9/24/24 7:26 AM, Tudor Ambarus wrote:
> > >>
> > >>
> > >> On 9/24/24 4:25 AM, Alvin Zhou wrote:
> > >>> Hi Tudor,
> > >>>
> > >>> Tudor Ambarus <[e-mail]> 於 2024年9月23日 週一 下午2:54寫道：
> > >>>>
> > >>>> Hi, Alvin,
> > >>>>
> > >>>> I quickly skimmed over the previous 5 patches and they are looking fine.
> > >>>>
> > >>>> I don't get this patch however.
> > >>>>
> > >>>> On 7/18/24 4:46 AM, AlvinZhou wrote:
> > >>>>> From: AlvinZhou <[e-mail]>
> > >>>>>
> > >>>>> Adding Manufacture ID 0xC2 in last of ID table because of
> > >>>>> Octal Flash need manufacturer fixup for enabling/disabling
> > >>>>> Octal DTR mode.
> > >>>>>
> > >>>>> Signed-off-by: JaimeLiao <[e-mail]>
> > >>>>> Signed-off-by: AlvinZhou <[e-mail]>
> > >>>>> ---
> > >>>>>  drivers/mtd/spi-nor/macronix.c | 4 +++-
> > >>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
> > >>>>>
> > >>>>> diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
> > >>>>> index f039819a5252..1a8ccebdfe0e 100644
> > >>>>> --- a/drivers/mtd/spi-nor/macronix.c
> > >>>>> +++ b/drivers/mtd/spi-nor/macronix.c
> > >>>>> @@ -200,7 +200,9 @@ static const struct flash_info macronix_nor_parts[] = {
> > >>>>>               .name = "mx25l3255e",
> > >>>>>               .size = SZ_4M,
> > >>>>>               .no_sfdp_flags = SECT_4K,
> > >>>>> -     }
> > >>>>> +     },
> > >>>>> +     /* Need the manufacturer fixups, Keep this last */
> > >>>>> +     { .id = SNOR_ID(0xc2) }
> > >>>>>  };
> > >>>>>
> > >>>>
> > >>>> Could you please elaborate why you need just the manufacturer id here? I
> > >>>> would have expected to see a specific flash entry instead.
> > >>>
> > >>> Grateful to Michael for the valuable suggestion. This addition of the
> > >>> Macronix manufacturer ID enables the fixup functions such as
> > >>> macronix_nor_set_octal_dtr to be executed without the need to
> > >>> create separate ID entries for each Octal DTR NOR Flash in the
> > >>> flash_info.
> > >>>
> > >>
> > >> Ah, nice. Okay then. I'm going to review the rest of the patches. They
> > >> look promising ;).
> > >
> > > ah, but then you'll always have a matched ID, so you break the generic
> > > flash probing for macronix. Is that correct?
> >
> > Answering myself: it's fine. Generic flash probe just fills a name,
> > which we don't really care about.
>
> I was also a bit surprised by the diff, would you mind filling a more
> complete with details on the actual goal of this entry (having all
> Macronix flashes to get the fixups, without creating separate ID
> entries for each of the flashes)

Thank you for the reminder, I will incorporate more detailed
explanations.

>
> Thanks,
> Miquèl

Thanks,
Alvin

Patch

diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
index f039819a5252..1a8ccebdfe0e 100644
--- a/drivers/mtd/spi-nor/macronix.c
+++ b/drivers/mtd/spi-nor/macronix.c
@@ -200,7 +200,9 @@  static const struct flash_info macronix_nor_parts[] = {
 		.name = "mx25l3255e",
 		.size = SZ_4M,
 		.no_sfdp_flags = SECT_4K,
-	}
+	},
+	/* Need the manufacturer fixups, Keep this last */
+	{ .id = SNOR_ID(0xc2) }
 };
 
 static int macronix_nor_octal_dtr_en(struct spi_nor *nor)