* Re: [PATCH] mtd: spi-nor: core: Set mtd->eraseregions for non-uniform erase map
2023-12-25 8:03 [PATCH] mtd: spi-nor: core: Set mtd->eraseregions for non-uniform erase map tkuw584924
@ 2024-01-05 12:23 ` Michael Walle
2024-01-12 7:14 ` Takahiro Kuwano
2024-01-12 9:43 ` Tudor Ambarus
2024-01-12 10:12 ` Tudor Ambarus
2 siblings, 1 reply; 13+ messages in thread
From: Michael Walle @ 2024-01-05 12:23 UTC (permalink / raw)
To: tkuw584924
Cc: linux-mtd, tudor.ambarus, pratyush, miquel.raynal, richard,
vigneshr, Bacem.Daassi, Takahiro Kuwano
Hi,
> -static void spi_nor_set_mtd_info(struct spi_nor *nor)
> +static int spi_nor_set_mtd_eraseregions(struct spi_nor *nor)
> +{
> + struct spi_nor_erase_map *map = &nor->params->erase_map;
> + struct spi_nor_erase_region *region = map->regions;
> + struct mtd_info *mtd = &nor->mtd;
> + struct mtd_erase_region_info *mtd_region;
> + u32 erase_size;
> + u8 erase_mask;
> + int n_regions, i, j;
> +
> + for (i = 0; !spi_nor_region_is_last(®ion[i]); i++)
> + ;
Please put that into a helper which returns the number of regions.
FWIW, I really dislike the magic around encoding all sorts of stuff
into the offset. It makes the code just hard to read.
> +
> + n_regions = i + 1;
> + mtd_region = devm_kcalloc(nor->dev, n_regions, sizeof(*mtd_region),
> + GFP_KERNEL);
Who's the owner? mtd->dev or nor->dev?
> + if (!mtd_region)
> + return -ENOMEM;
> +
> + for (i = 0; i < n_regions; i++) {
> + if (region[i].offset & SNOR_OVERLAID_REGION) {
Btw. what is an overlaid region? I couldn't find any comment
about it.
> + erase_size = region[i].size;
> + } else {
> + erase_mask = region[i].offset & SNOR_ERASE_TYPE_MASK;
> +
> + for (j = SNOR_ERASE_TYPE_MAX - 1; j >= 0; j--) {
> + if (erase_mask & BIT(j)) {
> + erase_size = map->erase_type[j].size;
> + break;
> + }
> + }
> + }
> + mtd_region[i].erasesize = erase_size;
> + mtd_region[i].numblocks = div64_ul(region[i].size, erase_size);
> + mtd_region[i].offset = region[i].offset &
> + ~SNOR_ERASE_FLAGS_MASK;
> + }
> +
> + mtd->numeraseregions = n_regions;
> + mtd->eraseregions = mtd_region;
> +
> + return 0;
> +}
> +
> +static int spi_nor_set_mtd_info(struct spi_nor *nor)
> {
> struct mtd_info *mtd = &nor->mtd;
> struct device *dev = nor->dev;
> @@ -3439,6 +3483,11 @@ static void spi_nor_set_mtd_info(struct spi_nor
> *nor)
> mtd->_resume = spi_nor_resume;
> mtd->_get_device = spi_nor_get_device;
> mtd->_put_device = spi_nor_put_device;
> +
> + if (spi_nor_has_uniform_erase(nor))
> + return 0;
> +
> + return spi_nor_set_mtd_eraseregions(nor);
mtd->erasesize is set somewhere else, please move it into this
function, because it will also have a special case for the
non_uniform flashes. Maybe we'll need our own erasesize stored
together with the opcode.
Also this should be written as
if (!spi_nor_has_uniform_erase(nor))
spi_nor_set_mtd_eraseregions(nor);
return 0;
-michael
> }
>
> static int spi_nor_hw_reset(struct spi_nor *nor)
> @@ -3531,7 +3580,9 @@ int spi_nor_scan(struct spi_nor *nor, const char
> *name,
> return ret;
>
> /* No mtd_info fields should be used up to this point. */
> - spi_nor_set_mtd_info(nor);
> + ret = spi_nor_set_mtd_info(nor);
> + if (ret)
> + return ret;
>
> dev_info(dev, "%s (%lld Kbytes)\n", info->name,
> (long long)mtd->size >> 10);
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] mtd: spi-nor: core: Set mtd->eraseregions for non-uniform erase map
2024-01-05 12:23 ` Michael Walle
@ 2024-01-12 7:14 ` Takahiro Kuwano
2024-01-12 9:22 ` Tudor Ambarus
2024-01-19 6:29 ` Takahiro Kuwano
0 siblings, 2 replies; 13+ messages in thread
From: Takahiro Kuwano @ 2024-01-12 7:14 UTC (permalink / raw)
To: Michael Walle
Cc: linux-mtd, tudor.ambarus, pratyush, miquel.raynal, richard,
vigneshr, Bacem.Daassi, Takahiro Kuwano
On 1/5/2024 9:23 PM, Michael Walle wrote:
> Hi,
>
>> -static void spi_nor_set_mtd_info(struct spi_nor *nor)
>> +static int spi_nor_set_mtd_eraseregions(struct spi_nor *nor)
>> +{
>> + struct spi_nor_erase_map *map = &nor->params->erase_map;
>> + struct spi_nor_erase_region *region = map->regions;
>> + struct mtd_info *mtd = &nor->mtd;
>> + struct mtd_erase_region_info *mtd_region;
>> + u32 erase_size;
>> + u8 erase_mask;
>> + int n_regions, i, j;
>> +
>> + for (i = 0; !spi_nor_region_is_last(®ion[i]); i++)
>> + ;
>
> Please put that into a helper which returns the number of regions.
>
Yes, I will do it.
> FWIW, I really dislike the magic around encoding all sorts of stuff
> into the offset. It makes the code just hard to read.
>
>
>> +
>> + n_regions = i + 1;
>> + mtd_region = devm_kcalloc(nor->dev, n_regions, sizeof(*mtd_region),
>> + GFP_KERNEL);
>
> Who's the owner? mtd->dev or nor->dev?
>
I think it should be nor->dev.
The mtd device is not yet registered at this point.
>> + if (!mtd_region)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < n_regions; i++) {
>> + if (region[i].offset & SNOR_OVERLAID_REGION) {
>
> Btw. what is an overlaid region? I couldn't find any comment
> about it.
>
It is the remaining part of regular sector that overlaid by 4KB sectors.
In SEMPER case, regular sector is 256KB. If 32 x 4KB sectors are overlaid on
bottom address, 128KB is overlaid region. The erase opcode for this region is
same as 256KB sectors.
>> + erase_size = region[i].size;
>> + } else {
>> + erase_mask = region[i].offset & SNOR_ERASE_TYPE_MASK;
>> +
>> + for (j = SNOR_ERASE_TYPE_MAX - 1; j >= 0; j--) {
>> + if (erase_mask & BIT(j)) {
>> + erase_size = map->erase_type[j].size;
>> + break;
>> + }
>> + }
>> + }
>> + mtd_region[i].erasesize = erase_size;
>> + mtd_region[i].numblocks = div64_ul(region[i].size, erase_size);
>> + mtd_region[i].offset = region[i].offset &
>> + ~SNOR_ERASE_FLAGS_MASK;
>> + }
>> +
>> + mtd->numeraseregions = n_regions;
>> + mtd->eraseregions = mtd_region;
>> +
>> + return 0;
>> +}
>> +
>> +static int spi_nor_set_mtd_info(struct spi_nor *nor)
>> {
>> struct mtd_info *mtd = &nor->mtd;
>> struct device *dev = nor->dev;
>> @@ -3439,6 +3483,11 @@ static void spi_nor_set_mtd_info(struct spi_nor *nor)
>> mtd->_resume = spi_nor_resume;
>> mtd->_get_device = spi_nor_get_device;
>> mtd->_put_device = spi_nor_put_device;
>> +
>> + if (spi_nor_has_uniform_erase(nor))
>> + return 0;
>> +
>> + return spi_nor_set_mtd_eraseregions(nor);
>
> mtd->erasesize is set somewhere else, please move it into this
> function, because it will also have a special case for the
> non_uniform flashes. Maybe we'll need our own erasesize stored
> together with the opcode.
>
Let me introduce params->erasesize which set through SFDP parse, then
mtd->erasesize = nor->params->erasesize;
like as other 'size' params.
> Also this should be written as
>
> if (!spi_nor_has_uniform_erase(nor))
> spi_nor_set_mtd_eraseregions(nor);
>
> return 0;
>
> -michael
>
OK, thanks!
>> }
>>
>> static int spi_nor_hw_reset(struct spi_nor *nor)
>> @@ -3531,7 +3580,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>> return ret;
>>
>> /* No mtd_info fields should be used up to this point. */
>> - spi_nor_set_mtd_info(nor);
>> + ret = spi_nor_set_mtd_info(nor);
>> + if (ret)
>> + return ret;
>>
>> dev_info(dev, "%s (%lld Kbytes)\n", info->name,
>> (long long)mtd->size >> 10);
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] mtd: spi-nor: core: Set mtd->eraseregions for non-uniform erase map
2024-01-12 7:14 ` Takahiro Kuwano
@ 2024-01-12 9:22 ` Tudor Ambarus
2024-01-19 6:29 ` Takahiro Kuwano
1 sibling, 0 replies; 13+ messages in thread
From: Tudor Ambarus @ 2024-01-12 9:22 UTC (permalink / raw)
To: Takahiro Kuwano, Michael Walle
Cc: linux-mtd, pratyush, miquel.raynal, richard, vigneshr,
Bacem.Daassi, Takahiro Kuwano
On 1/12/24 07:14, Takahiro Kuwano wrote:
> On 1/5/2024 9:23 PM, Michael Walle wrote:
>> Hi,
>>
>>> -static void spi_nor_set_mtd_info(struct spi_nor *nor)
>>> +static int spi_nor_set_mtd_eraseregions(struct spi_nor *nor)
>>> +{
>>> + struct spi_nor_erase_map *map = &nor->params->erase_map;
>>> + struct spi_nor_erase_region *region = map->regions;
>>> + struct mtd_info *mtd = &nor->mtd;
>>> + struct mtd_erase_region_info *mtd_region;
>>> + u32 erase_size;
>>> + u8 erase_mask;
>>> + int n_regions, i, j;
>>> +
>>> + for (i = 0; !spi_nor_region_is_last(®ion[i]); i++)
>>> + ;
>>
>> Please put that into a helper which returns the number of regions.
>>
> Yes, I will do it.
>
>> FWIW, I really dislike the magic around encoding all sorts of stuff
>> into the offset. It makes the code just hard to read.
>>
>>
>>> +
>>> + n_regions = i + 1;
>>> + mtd_region = devm_kcalloc(nor->dev, n_regions, sizeof(*mtd_region),
>>> + GFP_KERNEL);
>>
>> Who's the owner? mtd->dev or nor->dev?
>>
> I think it should be nor->dev.
> The mtd device is not yet registered at this point.
>
>>> + if (!mtd_region)
>>> + return -ENOMEM;
>>> +
>>> + for (i = 0; i < n_regions; i++) {
>>> + if (region[i].offset & SNOR_OVERLAID_REGION) {
>>
>> Btw. what is an overlaid region? I couldn't find any comment
>> about it.
>>
> It is the remaining part of regular sector that overlaid by 4KB sectors.
> In SEMPER case, regular sector is 256KB. If 32 x 4KB sectors are overlaid on
> bottom address, 128KB is overlaid region. The erase opcode for this region is
> same as 256KB sectors.
>
In other words, the overlaid region is what remains from a sector that's
not covered by smaller erase types.
_____
128KB <- 128KB overlaid region
_____
4KB
_____
..... 30 other 4KB sectors
_____
4KB
_____
the overlaid region does not have a dedicated 128 KB erase command and
instead relies on the bigger erase type, 256KB. When the 256KB erase is
issued on the 128KB overlaid region, just the 128KB of the overlaid
region are erased.
Did I remember correctly? Maybe we can describe this somewhere ...
Cheers,
ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] mtd: spi-nor: core: Set mtd->eraseregions for non-uniform erase map
2024-01-12 7:14 ` Takahiro Kuwano
2024-01-12 9:22 ` Tudor Ambarus
@ 2024-01-19 6:29 ` Takahiro Kuwano
2024-01-19 6:55 ` Tudor Ambarus
1 sibling, 1 reply; 13+ messages in thread
From: Takahiro Kuwano @ 2024-01-19 6:29 UTC (permalink / raw)
To: Michael Walle
Cc: linux-mtd, tudor.ambarus, pratyush, miquel.raynal, richard,
vigneshr, Bacem.Daassi, Takahiro Kuwano
On 1/12/2024 4:14 PM, Takahiro Kuwano wrote:
> On 1/5/2024 9:23 PM, Michael Walle wrote:
>> Hi,
>>
>>> -static void spi_nor_set_mtd_info(struct spi_nor *nor)
>>> +static int spi_nor_set_mtd_eraseregions(struct spi_nor *nor)
>>> +{
>>> + struct spi_nor_erase_map *map = &nor->params->erase_map;
>>> + struct spi_nor_erase_region *region = map->regions;
>>> + struct mtd_info *mtd = &nor->mtd;
>>> + struct mtd_erase_region_info *mtd_region;
>>> + u32 erase_size;
>>> + u8 erase_mask;
>>> + int n_regions, i, j;
>>> +
>>> + for (i = 0; !spi_nor_region_is_last(®ion[i]); i++)
>>> + ;
>>
>> Please put that into a helper which returns the number of regions.
>>
> Yes, I will do it.
>
>> FWIW, I really dislike the magic around encoding all sorts of stuff
>> into the offset. It makes the code just hard to read.
>>
>>
>>> +
>>> + n_regions = i + 1;
>>> + mtd_region = devm_kcalloc(nor->dev, n_regions, sizeof(*mtd_region),
>>> + GFP_KERNEL);
>>
>> Who's the owner? mtd->dev or nor->dev?
>>
> I think it should be nor->dev.
> The mtd device is not yet registered at this point.
>
>>> + if (!mtd_region)
>>> + return -ENOMEM;
>>> +
>>> + for (i = 0; i < n_regions; i++) {
>>> + if (region[i].offset & SNOR_OVERLAID_REGION) {
>>
>> Btw. what is an overlaid region? I couldn't find any comment
>> about it.
>>
> It is the remaining part of regular sector that overlaid by 4KB sectors.
> In SEMPER case, regular sector is 256KB. If 32 x 4KB sectors are overlaid on
> bottom address, 128KB is overlaid region. The erase opcode for this region is
> same as 256KB sectors.
>
>>> + erase_size = region[i].size;
>>> + } else {
>>> + erase_mask = region[i].offset & SNOR_ERASE_TYPE_MASK;
>>> +
>>> + for (j = SNOR_ERASE_TYPE_MAX - 1; j >= 0; j--) {
>>> + if (erase_mask & BIT(j)) {
>>> + erase_size = map->erase_type[j].size;
>>> + break;
>>> + }
>>> + }
>>> + }
>>> + mtd_region[i].erasesize = erase_size;
>>> + mtd_region[i].numblocks = div64_ul(region[i].size, erase_size);
>>> + mtd_region[i].offset = region[i].offset &
>>> + ~SNOR_ERASE_FLAGS_MASK;
>>> + }
>>> +
>>> + mtd->numeraseregions = n_regions;
>>> + mtd->eraseregions = mtd_region;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int spi_nor_set_mtd_info(struct spi_nor *nor)
>>> {
>>> struct mtd_info *mtd = &nor->mtd;
>>> struct device *dev = nor->dev;
>>> @@ -3439,6 +3483,11 @@ static void spi_nor_set_mtd_info(struct spi_nor *nor)
>>> mtd->_resume = spi_nor_resume;
>>> mtd->_get_device = spi_nor_get_device;
>>> mtd->_put_device = spi_nor_put_device;
>>> +
>>> + if (spi_nor_has_uniform_erase(nor))
>>> + return 0;
>>> +
>>> + return spi_nor_set_mtd_eraseregions(nor);
>>
>> mtd->erasesize is set somewhere else, please move it into this
>> function, because it will also have a special case for the
>> non_uniform flashes. Maybe we'll need our own erasesize stored
>> together with the opcode.
>>
> Let me introduce params->erasesize which set through SFDP parse, then
>
> mtd->erasesize = nor->params->erasesize;
>
> like as other 'size' params.
>
I tried to implement this, but found mtd->erasesize is set in some fixup hooks
in manufacturer driver and need to think carefully about changing them.
Let me do this later in another series of patches.
Thanks,
Takahiro
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] mtd: spi-nor: core: Set mtd->eraseregions for non-uniform erase map
2024-01-19 6:29 ` Takahiro Kuwano
@ 2024-01-19 6:55 ` Tudor Ambarus
2024-01-19 8:35 ` Takahiro Kuwano
2024-01-19 12:49 ` Michael Walle
0 siblings, 2 replies; 13+ messages in thread
From: Tudor Ambarus @ 2024-01-19 6:55 UTC (permalink / raw)
To: Takahiro Kuwano, Michael Walle
Cc: linux-mtd, pratyush, miquel.raynal, richard, vigneshr,
Bacem.Daassi, Takahiro Kuwano
On 1/19/24 06:29, Takahiro Kuwano wrote:
> On 1/12/2024 4:14 PM, Takahiro Kuwano wrote:
>> On 1/5/2024 9:23 PM, Michael Walle wrote:
>>> Hi,
>>>
>>>> -static void spi_nor_set_mtd_info(struct spi_nor *nor)
>>>> +static int spi_nor_set_mtd_eraseregions(struct spi_nor *nor)
>>>> +{
>>>> + struct spi_nor_erase_map *map = &nor->params->erase_map;
>>>> + struct spi_nor_erase_region *region = map->regions;
>>>> + struct mtd_info *mtd = &nor->mtd;
>>>> + struct mtd_erase_region_info *mtd_region;
>>>> + u32 erase_size;
>>>> + u8 erase_mask;
>>>> + int n_regions, i, j;
>>>> +
>>>> + for (i = 0; !spi_nor_region_is_last(®ion[i]); i++)
>>>> + ;
>>>
>>> Please put that into a helper which returns the number of regions.
>>>
>> Yes, I will do it.
>>
>>> FWIW, I really dislike the magic around encoding all sorts of stuff
>>> into the offset. It makes the code just hard to read.
>>>
>>>
>>>> +
>>>> + n_regions = i + 1;
>>>> + mtd_region = devm_kcalloc(nor->dev, n_regions, sizeof(*mtd_region),
>>>> + GFP_KERNEL);
>>>
>>> Who's the owner? mtd->dev or nor->dev?
>>>
>> I think it should be nor->dev.
>> The mtd device is not yet registered at this point.
>>
>>>> + if (!mtd_region)
>>>> + return -ENOMEM;
>>>> +
>>>> + for (i = 0; i < n_regions; i++) {
>>>> + if (region[i].offset & SNOR_OVERLAID_REGION) {
>>>
>>> Btw. what is an overlaid region? I couldn't find any comment
>>> about it.
>>>
>> It is the remaining part of regular sector that overlaid by 4KB sectors.
>> In SEMPER case, regular sector is 256KB. If 32 x 4KB sectors are overlaid on
>> bottom address, 128KB is overlaid region. The erase opcode for this region is
>> same as 256KB sectors.
>>
>>>> + erase_size = region[i].size;
>>>> + } else {
>>>> + erase_mask = region[i].offset & SNOR_ERASE_TYPE_MASK;
>>>> +
>>>> + for (j = SNOR_ERASE_TYPE_MAX - 1; j >= 0; j--) {
>>>> + if (erase_mask & BIT(j)) {
>>>> + erase_size = map->erase_type[j].size;
>>>> + break;
>>>> + }
>>>> + }
>>>> + }
>>>> + mtd_region[i].erasesize = erase_size;
>>>> + mtd_region[i].numblocks = div64_ul(region[i].size, erase_size);
>>>> + mtd_region[i].offset = region[i].offset &
>>>> + ~SNOR_ERASE_FLAGS_MASK;
>>>> + }
>>>> +
>>>> + mtd->numeraseregions = n_regions;
>>>> + mtd->eraseregions = mtd_region;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int spi_nor_set_mtd_info(struct spi_nor *nor)
>>>> {
>>>> struct mtd_info *mtd = &nor->mtd;
>>>> struct device *dev = nor->dev;
>>>> @@ -3439,6 +3483,11 @@ static void spi_nor_set_mtd_info(struct spi_nor *nor)
>>>> mtd->_resume = spi_nor_resume;
>>>> mtd->_get_device = spi_nor_get_device;
>>>> mtd->_put_device = spi_nor_put_device;
>>>> +
>>>> + if (spi_nor_has_uniform_erase(nor))
>>>> + return 0;
>>>> +
>>>> + return spi_nor_set_mtd_eraseregions(nor);
>>>
>>> mtd->erasesize is set somewhere else, please move it into this
>>> function, because it will also have a special case for the
>>> non_uniform flashes. Maybe we'll need our own erasesize stored
>>> together with the opcode.
>>>
>> Let me introduce params->erasesize which set through SFDP parse, then
>>
>> mtd->erasesize = nor->params->erasesize;
>>
>> like as other 'size' params.
>>
> I tried to implement this, but found mtd->erasesize is set in some fixup hooks
> in manufacturer driver and need to think carefully about changing them.
> Let me do this later in another series of patches.
>
Do you mean xilinx? That's a dead weight, I haven't seen patches for it
since its introduction. I'm thinking of getting rid of the xilinx s3an
flashes from the SPI NOR. Michael, Pratyush?
Cheers,
ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] mtd: spi-nor: core: Set mtd->eraseregions for non-uniform erase map
2024-01-19 6:55 ` Tudor Ambarus
@ 2024-01-19 8:35 ` Takahiro Kuwano
2024-01-19 12:49 ` Michael Walle
1 sibling, 0 replies; 13+ messages in thread
From: Takahiro Kuwano @ 2024-01-19 8:35 UTC (permalink / raw)
To: Tudor Ambarus, Michael Walle
Cc: linux-mtd, pratyush, miquel.raynal, richard, vigneshr,
Bacem.Daassi, Takahiro Kuwano
On 1/19/2024 3:55 PM, Tudor Ambarus wrote:
>
>
> On 1/19/24 06:29, Takahiro Kuwano wrote:
>> On 1/12/2024 4:14 PM, Takahiro Kuwano wrote:
>>> On 1/5/2024 9:23 PM, Michael Walle wrote:
>>>> Hi,
>>>>
>>>>> -static void spi_nor_set_mtd_info(struct spi_nor *nor)
>>>>> +static int spi_nor_set_mtd_eraseregions(struct spi_nor *nor)
>>>>> +{
>>>>> + struct spi_nor_erase_map *map = &nor->params->erase_map;
>>>>> + struct spi_nor_erase_region *region = map->regions;
>>>>> + struct mtd_info *mtd = &nor->mtd;
>>>>> + struct mtd_erase_region_info *mtd_region;
>>>>> + u32 erase_size;
>>>>> + u8 erase_mask;
>>>>> + int n_regions, i, j;
>>>>> +
>>>>> + for (i = 0; !spi_nor_region_is_last(®ion[i]); i++)
>>>>> + ;
>>>>
>>>> Please put that into a helper which returns the number of regions.
>>>>
>>> Yes, I will do it.
>>>
>>>> FWIW, I really dislike the magic around encoding all sorts of stuff
>>>> into the offset. It makes the code just hard to read.
>>>>
>>>>
>>>>> +
>>>>> + n_regions = i + 1;
>>>>> + mtd_region = devm_kcalloc(nor->dev, n_regions, sizeof(*mtd_region),
>>>>> + GFP_KERNEL);
>>>>
>>>> Who's the owner? mtd->dev or nor->dev?
>>>>
>>> I think it should be nor->dev.
>>> The mtd device is not yet registered at this point.
>>>
>>>>> + if (!mtd_region)
>>>>> + return -ENOMEM;
>>>>> +
>>>>> + for (i = 0; i < n_regions; i++) {
>>>>> + if (region[i].offset & SNOR_OVERLAID_REGION) {
>>>>
>>>> Btw. what is an overlaid region? I couldn't find any comment
>>>> about it.
>>>>
>>> It is the remaining part of regular sector that overlaid by 4KB sectors.
>>> In SEMPER case, regular sector is 256KB. If 32 x 4KB sectors are overlaid on
>>> bottom address, 128KB is overlaid region. The erase opcode for this region is
>>> same as 256KB sectors.
>>>
>>>>> + erase_size = region[i].size;
>>>>> + } else {
>>>>> + erase_mask = region[i].offset & SNOR_ERASE_TYPE_MASK;
>>>>> +
>>>>> + for (j = SNOR_ERASE_TYPE_MAX - 1; j >= 0; j--) {
>>>>> + if (erase_mask & BIT(j)) {
>>>>> + erase_size = map->erase_type[j].size;
>>>>> + break;
>>>>> + }
>>>>> + }
>>>>> + }
>>>>> + mtd_region[i].erasesize = erase_size;
>>>>> + mtd_region[i].numblocks = div64_ul(region[i].size, erase_size);
>>>>> + mtd_region[i].offset = region[i].offset &
>>>>> + ~SNOR_ERASE_FLAGS_MASK;
>>>>> + }
>>>>> +
>>>>> + mtd->numeraseregions = n_regions;
>>>>> + mtd->eraseregions = mtd_region;
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static int spi_nor_set_mtd_info(struct spi_nor *nor)
>>>>> {
>>>>> struct mtd_info *mtd = &nor->mtd;
>>>>> struct device *dev = nor->dev;
>>>>> @@ -3439,6 +3483,11 @@ static void spi_nor_set_mtd_info(struct spi_nor *nor)
>>>>> mtd->_resume = spi_nor_resume;
>>>>> mtd->_get_device = spi_nor_get_device;
>>>>> mtd->_put_device = spi_nor_put_device;
>>>>> +
>>>>> + if (spi_nor_has_uniform_erase(nor))
>>>>> + return 0;
>>>>> +
>>>>> + return spi_nor_set_mtd_eraseregions(nor);
>>>>
>>>> mtd->erasesize is set somewhere else, please move it into this
>>>> function, because it will also have a special case for the
>>>> non_uniform flashes. Maybe we'll need our own erasesize stored
>>>> together with the opcode.
>>>>
>>> Let me introduce params->erasesize which set through SFDP parse, then
>>>
>>> mtd->erasesize = nor->params->erasesize;
>>>
>>> like as other 'size' params.
>>>
>> I tried to implement this, but found mtd->erasesize is set in some fixup hooks
>> in manufacturer driver and need to think carefully about changing them.
>> Let me do this later in another series of patches.
>>
>
> Do you mean xilinx? That's a dead weight, I haven't seen patches for it
> since its introduction. I'm thinking of getting rid of the xilinx s3an
> flashes from the SPI NOR. Michael, Pratyush?
>
spansion also. Changing it may impact to some older parts. I need time to test
and debug with different Flash parts.
> Cheers,
> ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] mtd: spi-nor: core: Set mtd->eraseregions for non-uniform erase map
2024-01-19 6:55 ` Tudor Ambarus
2024-01-19 8:35 ` Takahiro Kuwano
@ 2024-01-19 12:49 ` Michael Walle
1 sibling, 0 replies; 13+ messages in thread
From: Michael Walle @ 2024-01-19 12:49 UTC (permalink / raw)
To: Tudor Ambarus
Cc: Takahiro Kuwano, linux-mtd, pratyush, miquel.raynal, richard,
vigneshr, Bacem.Daassi, Takahiro Kuwano
Hi,
> Do you mean xilinx? That's a dead weight, I haven't seen patches for it
> since its introduction. I'm thinking of getting rid of the xilinx s3an
> flashes from the SPI NOR. Michael, Pratyush?
Yes, please! Then all these special handling with the not-power-of-2
sectors
can be dropped.
-michael
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] mtd: spi-nor: core: Set mtd->eraseregions for non-uniform erase map
2023-12-25 8:03 [PATCH] mtd: spi-nor: core: Set mtd->eraseregions for non-uniform erase map tkuw584924
2024-01-05 12:23 ` Michael Walle
@ 2024-01-12 9:43 ` Tudor Ambarus
2024-01-12 10:12 ` Tudor Ambarus
2 siblings, 0 replies; 13+ messages in thread
From: Tudor Ambarus @ 2024-01-12 9:43 UTC (permalink / raw)
To: tkuw584924, linux-mtd
Cc: pratyush, michael, miquel.raynal, richard, vigneshr, Bacem.Daassi,
Takahiro Kuwano
On 12/25/23 08:03, tkuw584924@gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>
> Some of Infineon SPI NOR flash devices support hybrid sector layout that
> overlays 4KB sectors on a 256KB sector and SPI NOR framework recognizes
> that by parsing SMPT and construct params->erase_map. The hybrid sector
> layout is similar to CFI flash devices that have small sectors on top
> and/or bottom address. In case of CFI flash devices, the erase map
> information is parsed through CFI table and populated into
> mtd->eraseregions so that users can create MTD partitions that aligned with
> small sector boundaries. This patch provides the same capability to SPI
> NOR flash devices that have non-uniform erase map.
>
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
> The s25hs512t has 32 x 4KB, 1 x 128KB, and 255 x 256KB sectors by default.
> Let's think about creating three MTD partitions that align to region
> boundary.
>
> BEFORE applying this patch, 1st and 2nd region are forced to read-only:
>
> ---kernel log---
> ...
> spi-nor spi0.0: s25hs512t (65536 Kbytes)
> 3 fixed-partitions partitions found on MTD device spi0.0
> Creating 3 MTD partitions on "spi0.0":
> 0x000000000000-0x000000020000 : "4KB x 32"
> mtd: partition "4KB x 32" doesn't end on an erase/write block -- force read-only
> 0x000000020000-0x000000040000 : "128KB x 1"
> mtd: partition "128KB x 1" doesn't start on an erase/write block boundary -- force read-only
> 0x000000040000-0x000004000000 : "256KB x 255"
> ...
>
> ---MTD Info---
> zynq> mtd_debug info /dev/mtd0
> mtd.type = MTD_NORFLASH
> mtd.flags = MTD_CAP_ROM
> mtd.size = 131072 (128K)
> mtd.erasesize = 262144 (256K)
> mtd.writesize = 16
> mtd.oobsize = 0
> regions = 0
>
> zynq> mtd_debug info /dev/mtd1
> mtd.type = MTD_NORFLASH
> mtd.flags = MTD_CAP_ROM
> mtd.size = 131072 (128K)
> mtd.erasesize = 262144 (256K)
> mtd.writesize = 16
> mtd.oobsize = 0
> regions = 0
>
> zynq> mtd_debug info /dev/mtd2
> mtd.type = MTD_NORFLASH
> mtd.flags = MTD_CAP_NANDFLASH
> mtd.size = 66846720 (63M)
> mtd.erasesize = 262144 (256K)
> mtd.writesize = 16
> mtd.oobsize = 0
> regions = 0
>
>
>
> AFTER applying this patch, erasesize is correctly informed and no read-only:
>
> ---kernel log---
> ...
> spi-nor spi0.0: s25hs512t (65536 Kbytes)
> 3 fixed-partitions partitions found on MTD device spi0.0
> Creating 3 MTD partitions on "spi0.0":
> 0x000000000000-0x000000020000 : "4KB x 32"
> 0x000000020000-0x000000040000 : "128KB x 1"
> 0x000000040000-0x000004000000 : "256KB x 255"
> ...
>
> ---MTD Info---
> zynq> mtd_debug info /dev/mtd0
> mtd.type = MTD_NORFLASH
> mtd.flags = MTD_CAP_NANDFLASH
> mtd.size = 131072 (128K)
> mtd.erasesize = 4096 (4K)
> mtd.writesize = 16
> mtd.oobsize = 0
> regions = 0
>
> zynq> mtd_debug info /dev/mtd1
> mtd.type = MTD_NORFLASH
> mtd.flags = MTD_CAP_NANDFLASH
> mtd.size = 131072 (128K)
> mtd.erasesize = 131072 (128K)
> mtd.writesize = 16
> mtd.oobsize = 0
> regions = 0
>
> zynq> mtd_debug info /dev/mtd2
> mtd.type = MTD_NORFLASH
> mtd.flags = MTD_CAP_NANDFLASH
> mtd.size = 66846720 (63M)
> mtd.erasesize = 262144 (256K)
> mtd.writesize = 16
> mtd.oobsize = 0
> regions = 0
>
nice!
> ---
> drivers/mtd/spi-nor/core.c | 55 ++++++++++++++++++++++++++++++++++++--
> 1 file changed, 53 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 1b0c6770c14e..e512491733a8 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3408,7 +3408,51 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
> return info;
> }
>
> -static void spi_nor_set_mtd_info(struct spi_nor *nor)
> +static int spi_nor_set_mtd_eraseregions(struct spi_nor *nor)
> +{
> + struct spi_nor_erase_map *map = &nor->params->erase_map;
> + struct spi_nor_erase_region *region = map->regions;
shall we have some consts above?
> + struct mtd_info *mtd = &nor->mtd;
some prefer a reverse xmas tree, thus put the definition from above
below the declaration from below.
> + struct mtd_erase_region_info *mtd_region;
> + u32 erase_size;
> + u8 erase_mask;
put the u8 last to avoid stack padding
> + int n_regions, i, j;
unsigned int
> +
> + for (i = 0; !spi_nor_region_is_last(®ion[i]); i++)
> + ;
> +
> + n_regions = i + 1;
all this just to get the number of regions? how about saving the number
of regions somewhere and use it everywhere?
> + mtd_region = devm_kcalloc(nor->dev, n_regions, sizeof(*mtd_region),
> + GFP_KERNEL);
> + if (!mtd_region)
> + return -ENOMEM;
> +
> + for (i = 0; i < n_regions; i++) {
> + if (region[i].offset & SNOR_OVERLAID_REGION) {
> + erase_size = region[i].size;
> + } else {
> + erase_mask = region[i].offset & SNOR_ERASE_TYPE_MASK;
> +
> + for (j = SNOR_ERASE_TYPE_MAX - 1; j >= 0; j--) {
> + if (erase_mask & BIT(j)) {
> + erase_size = map->erase_type[j].size;
> + break;
> + }
> + }
this for, determining the erase size, deserves a dedicated method. Too
many indentation levels.
> + }
> + mtd_region[i].erasesize = erase_size;
> + mtd_region[i].numblocks = div64_ul(region[i].size, erase_size);
> + mtd_region[i].offset = region[i].offset &
> + ~SNOR_ERASE_FLAGS_MASK;
> + }
> +
> + mtd->numeraseregions = n_regions;
> + mtd->eraseregions = mtd_region;
> +
> + return 0;
> +}
> +
> +static int spi_nor_set_mtd_info(struct spi_nor *nor)
> {
> struct mtd_info *mtd = &nor->mtd;
> struct device *dev = nor->dev;
> @@ -3439,6 +3483,11 @@ static void spi_nor_set_mtd_info(struct spi_nor *nor)
> mtd->_resume = spi_nor_resume;
> mtd->_get_device = spi_nor_get_device;
> mtd->_put_device = spi_nor_put_device;
> +
> + if (spi_nor_has_uniform_erase(nor))
> + return 0;
> +
> + return spi_nor_set_mtd_eraseregions(nor);
> }
>
> static int spi_nor_hw_reset(struct spi_nor *nor)
> @@ -3531,7 +3580,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
> return ret;
>
> /* No mtd_info fields should be used up to this point. */
> - spi_nor_set_mtd_info(nor);
> + ret = spi_nor_set_mtd_info(nor);
> + if (ret)
> + return ret;
>
> dev_info(dev, "%s (%lld Kbytes)\n", info->name,
> (long long)mtd->size >> 10);
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] mtd: spi-nor: core: Set mtd->eraseregions for non-uniform erase map
2023-12-25 8:03 [PATCH] mtd: spi-nor: core: Set mtd->eraseregions for non-uniform erase map tkuw584924
2024-01-05 12:23 ` Michael Walle
2024-01-12 9:43 ` Tudor Ambarus
@ 2024-01-12 10:12 ` Tudor Ambarus
2024-01-12 12:01 ` Michael Walle
2 siblings, 1 reply; 13+ messages in thread
From: Tudor Ambarus @ 2024-01-12 10:12 UTC (permalink / raw)
To: tkuw584924, linux-mtd
Cc: pratyush, michael, miquel.raynal, richard, vigneshr, Bacem.Daassi,
Takahiro Kuwano
On 12/25/23 08:03, tkuw584924@gmail.com wrote:
> From: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
>
> Some of Infineon SPI NOR flash devices support hybrid sector layout that
> overlays 4KB sectors on a 256KB sector and SPI NOR framework recognizes
> that by parsing SMPT and construct params->erase_map. The hybrid sector
> layout is similar to CFI flash devices that have small sectors on top
> and/or bottom address. In case of CFI flash devices, the erase map
> information is parsed through CFI table and populated into
> mtd->eraseregions so that users can create MTD partitions that aligned with
> small sector boundaries. This patch provides the same capability to SPI
> NOR flash devices that have non-uniform erase map.
>
> Signed-off-by: Takahiro Kuwano <Takahiro.Kuwano@infineon.com>
> ---
> The s25hs512t has 32 x 4KB, 1 x 128KB, and 255 x 256KB sectors by default.
> Let's think about creating three MTD partitions that align to region
> boundary.
>
> BEFORE applying this patch, 1st and 2nd region are forced to read-only:
>
> ---kernel log---
> ...
> spi-nor spi0.0: s25hs512t (65536 Kbytes)
> 3 fixed-partitions partitions found on MTD device spi0.0
> Creating 3 MTD partitions on "spi0.0":
> 0x000000000000-0x000000020000 : "4KB x 32"
> mtd: partition "4KB x 32" doesn't end on an erase/write block -- force read-only
> 0x000000020000-0x000000040000 : "128KB x 1"
> mtd: partition "128KB x 1" doesn't start on an erase/write block boundary -- force read-only
> 0x000000040000-0x000004000000 : "256KB x 255"
> ...
>
> ---MTD Info---
> zynq> mtd_debug info /dev/mtd0
> mtd.type = MTD_NORFLASH
> mtd.flags = MTD_CAP_ROM
> mtd.size = 131072 (128K)
> mtd.erasesize = 262144 (256K)
> mtd.writesize = 16
> mtd.oobsize = 0
> regions = 0
>
> zynq> mtd_debug info /dev/mtd1
> mtd.type = MTD_NORFLASH
> mtd.flags = MTD_CAP_ROM
> mtd.size = 131072 (128K)
> mtd.erasesize = 262144 (256K)
> mtd.writesize = 16
> mtd.oobsize = 0
> regions = 0
>
> zynq> mtd_debug info /dev/mtd2
> mtd.type = MTD_NORFLASH
> mtd.flags = MTD_CAP_NANDFLASH
> mtd.size = 66846720 (63M)
> mtd.erasesize = 262144 (256K)
> mtd.writesize = 16
> mtd.oobsize = 0
> regions = 0
>
>
>
> AFTER applying this patch, erasesize is correctly informed and no read-only:
>
> ---kernel log---
> ...
> spi-nor spi0.0: s25hs512t (65536 Kbytes)
> 3 fixed-partitions partitions found on MTD device spi0.0
> Creating 3 MTD partitions on "spi0.0":
> 0x000000000000-0x000000020000 : "4KB x 32"
> 0x000000020000-0x000000040000 : "128KB x 1"
> 0x000000040000-0x000004000000 : "256KB x 255"
> ...
>
> ---MTD Info---
> zynq> mtd_debug info /dev/mtd0
> mtd.type = MTD_NORFLASH
> mtd.flags = MTD_CAP_NANDFLASH
> mtd.size = 131072 (128K)
> mtd.erasesize = 4096 (4K)
> mtd.writesize = 16
> mtd.oobsize = 0
> regions = 0
>
> zynq> mtd_debug info /dev/mtd1
> mtd.type = MTD_NORFLASH
> mtd.flags = MTD_CAP_NANDFLASH
> mtd.size = 131072 (128K)
> mtd.erasesize = 131072 (128K)
> mtd.writesize = 16
> mtd.oobsize = 0
> regions = 0
>
> zynq> mtd_debug info /dev/mtd2
> mtd.type = MTD_NORFLASH
> mtd.flags = MTD_CAP_NANDFLASH
> mtd.size = 66846720 (63M)
> mtd.erasesize = 262144 (256K)
> mtd.writesize = 16
> mtd.oobsize = 0
> regions = 0
>
nice!
> ---
> drivers/mtd/spi-nor/core.c | 55 ++++++++++++++++++++++++++++++++++++--
> 1 file changed, 53 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 1b0c6770c14e..e512491733a8 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3408,7 +3408,51 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
> return info;
> }
>
> -static void spi_nor_set_mtd_info(struct spi_nor *nor)
> +static int spi_nor_set_mtd_eraseregions(struct spi_nor *nor)
> +{
> + struct spi_nor_erase_map *map = &nor->params->erase_map;
> + struct spi_nor_erase_region *region = map->regions;
shall we have some consts above?
> + struct mtd_info *mtd = &nor->mtd;
some prefer a reverse xmas tree, thus put the definition from above
below the declaration from below.
> + struct mtd_erase_region_info *mtd_region;
> + u32 erase_size;
> + u8 erase_mask;
put the u8 last to avoid stack padding
> + int n_regions, i, j;
unsigned int
> +
> + for (i = 0; !spi_nor_region_is_last(®ion[i]); i++)
> + ;
> +
> + n_regions = i + 1;
all this just to get the number of regions? how about saving the number
of regions somewhere and use it everywhere?
> + mtd_region = devm_kcalloc(nor->dev, n_regions, sizeof(*mtd_region),
> + GFP_KERNEL);
> + if (!mtd_region)
> + return -ENOMEM;
> +
> + for (i = 0; i < n_regions; i++) {
> + if (region[i].offset & SNOR_OVERLAID_REGION) {
> + erase_size = region[i].size;
> + } else {
> + erase_mask = region[i].offset & SNOR_ERASE_TYPE_MASK;
> +
> + for (j = SNOR_ERASE_TYPE_MAX - 1; j >= 0; j--) {
> + if (erase_mask & BIT(j)) {
> + erase_size = map->erase_type[j].size;
> + break;
> + }
> + }
this for, determining the erase size, deserves a dedicated method. Too
many indentation levels.
Cheers,
ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] mtd: spi-nor: core: Set mtd->eraseregions for non-uniform erase map
2024-01-12 10:12 ` Tudor Ambarus
@ 2024-01-12 12:01 ` Michael Walle
2024-01-12 12:22 ` Tudor Ambarus
0 siblings, 1 reply; 13+ messages in thread
From: Michael Walle @ 2024-01-12 12:01 UTC (permalink / raw)
To: Tudor Ambarus
Cc: tkuw584924, linux-mtd, pratyush, miquel.raynal, richard, vigneshr,
Bacem.Daassi, Takahiro Kuwano
>> + struct mtd_erase_region_info *mtd_region;
>> + u32 erase_size;
>> + u8 erase_mask;
>
> put the u8 last to avoid stack padding
I don't think that is a thing. Even if it were, it might clash
with the RCT.
I couldn't find anything about how automatic variables are placed
in memory. I'd say its not specified in the standard and the compiler
is free to do optimizations here or just keep the contents in
registers (?!).
Any stytle recommendations for spi-nor? I prefer RCT, but if we want
to say declaration order doesn't matter, I'm fine with that too.
>> +
>> + for (i = 0; !spi_nor_region_is_last(®ion[i]); i++)
>> + ;
>> +
>> + n_regions = i + 1;
>
> all this just to get the number of regions? how about saving the number
> of regions somewhere and use it everywhere?
This whole region thing should be rewritten to not store these magic
bits in the offset.
-michael
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] mtd: spi-nor: core: Set mtd->eraseregions for non-uniform erase map
2024-01-12 12:01 ` Michael Walle
@ 2024-01-12 12:22 ` Tudor Ambarus
2024-01-12 12:28 ` Michael Walle
0 siblings, 1 reply; 13+ messages in thread
From: Tudor Ambarus @ 2024-01-12 12:22 UTC (permalink / raw)
To: Michael Walle
Cc: tkuw584924, linux-mtd, pratyush, miquel.raynal, richard, vigneshr,
Bacem.Daassi, Takahiro Kuwano
On 1/12/24 12:01, Michael Walle wrote:
>>> + struct mtd_erase_region_info *mtd_region;
>>> + u32 erase_size;
>>> + u8 erase_mask;
>>
>> put the u8 last to avoid stack padding
>
> I don't think that is a thing. Even if it were, it might clash
> with the RCT.
RCT as in reverse Christmas tree? if we put u8 at the end we'll respect
RCT and without padding holes in the stack.
>
> I couldn't find anything about how automatic variables are placed
> in memory. I'd say its not specified in the standard and the compiler
> is free to do optimizations here or just keep the contents in
> registers (?!).
>
I can't tell.
> Any stytle recommendations for spi-nor? I prefer RCT, but if we want
> to say declaration order doesn't matter, I'm fine with that too.
RCT or CT with stack padding in mind :). But that's just common sense, I
guess.
>
>>> +
>>> + for (i = 0; !spi_nor_region_is_last(®ion[i]); i++)
>>> + ;
>>> +
>>> + n_regions = i + 1;
>>
>> all this just to get the number of regions? how about saving the number
>> of regions somewhere and use it everywhere?
>
> This whole region thing should be rewritten to not store these magic
> bits in the offset.
>
yeah, probably.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] mtd: spi-nor: core: Set mtd->eraseregions for non-uniform erase map
2024-01-12 12:22 ` Tudor Ambarus
@ 2024-01-12 12:28 ` Michael Walle
0 siblings, 0 replies; 13+ messages in thread
From: Michael Walle @ 2024-01-12 12:28 UTC (permalink / raw)
To: Tudor Ambarus
Cc: tkuw584924, linux-mtd, pratyush, miquel.raynal, richard, vigneshr,
Bacem.Daassi, Takahiro Kuwano
>>>> + struct mtd_erase_region_info *mtd_region;
>>>> + u32 erase_size;
>>>> + u8 erase_mask;
>>>
>>> put the u8 last to avoid stack padding
>>
>> I don't think that is a thing. Even if it were, it might clash
>> with the RCT.
>
> RCT as in reverse Christmas tree?
yes
> if we put u8 at the end we'll respect RCT and without padding holes
> in the stack.
It might clash, not in this particular example.
>> I couldn't find anything about how automatic variables are placed
>> in memory. I'd say its not specified in the standard and the compiler
>> is free to do optimizations here or just keep the contents in
>> registers (?!).
>>
>
> I can't tell.
>
>> Any stytle recommendations for spi-nor? I prefer RCT, but if we want
>> to say declaration order doesn't matter, I'm fine with that too.
>
> RCT or CT with stack padding in mind :). But that's just common sense,
> I
> guess.
Again, that's not a thing. So IMHO order doesn't matter so we should
just
say RCT, i.e. the same as netdev.
-michael
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply [flat|nested] 13+ messages in thread