From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefan Roese <sr@denx.de>
Date: Sat, 30 Jan 2021 10:01:42 +0100
Subject: [PATCH] env: sf: single function env_sf_save()
In-Reply-To: <07cc1fb0-6d2a-4aee-1cba-ce710b21caa6@men.de>
References: <20210128072108.32657-1-Harry.Waschkeit@men.de>
 <67584036-a592-61ee-c9e6-e06af5fb0f7a@denx.de>
 <f0d224d6-afc3-16b9-dd47-e86b67848c7a@men.de>
 <3860d3d2-8cbc-94a2-4760-14978a259e51@denx.de>
 <aba72d58-d0c6-fcbd-5a54-4d565a025e66@men.de>
 <9e91f5da-eb43-1719-659d-3f7be1be5e76@denx.de>
 <07cc1fb0-6d2a-4aee-1cba-ce710b21caa6@men.de>
Message-ID: <eef97c5e-acaa-0a88-aa9a-d3396b57123c@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Harry,

On 29.01.21 18:18, Harry Waschkeit wrote:
> Hi again Stefan,
> 
> On 29.01.21 08:16, Stefan Roese wrote:
>> On 28.01.21 12:21, Harry Waschkeit wrote:
>>
>> <snip>
>>
>>>>> Even though an "if (CONFIG_IS_ENABLED(...))" would statically 
>>>>> evaluate to '0' without active redundancy in environments, the 
>>>>> parser sees the syntax error of the non-existing structure element.
>>>>>
>>>>> So, I don't see a chance for avoiding the #if construct here, too.
>>>>> Let me know if I miss something :-)
>>>>
>>>> I agree its not easy or perhaps even impossible. One idea would be
>>>> to introduce a union in the struct with "flags" also defines in the
>>>> non-redundant case but not being used here. But this would result
>>>> in very non-intuitive code AFAICT. So better not go this way.
>>>
>>> That would feel very strange to me, too.
>>>
>>>> Perhaps somebody else has a quick idea on how to handle this without
>>>> introduncing #ifdef's here?
>>>
>>> It would be possible to introduce a macro like env_set_flags(envp, 
>>> flag) in env.h that only evaluates to something in case of active 
>>> redundancy, sure.
>>>
>>> But that's not even half of a solution, because also variables 
>>> env_offset and env_new_offset which are set in that section are only 
>>> defined if necessary, i.e. if CONFIG_SYS_REDUNDAND_ENVIRONMENT is set 
>>> (btw also protected by #if while not possible otherwise).
>>>
>>> The trade-off here is between code duplication - which I removed by 
>>> combining two very similar separate functions for the two situations 
>>> w/ and w/o redundancy in one - and in-line pre-compiler protected 
>>> regions within one function.
>>>
>>> While I fully agree that the latter should be avoided as far as 
>>> possible, it is not avoidable here afaics.
>>>
>>> And avoiding code duplication here outweighs the few pre-compiler 
>>> occurrences in my eyes, but that's just my ? 0,02 :-)
>>
>> I also agree (now). If nobody else comes up with a better idea, then
>> please proceed with the current implementation to remove the code
>> duplication.
> 
> sorry for the newbie question, I'm not familiar (yet) with the normal 
> procedure and I don't want to keep that ball from rolling ;-/

No Problem. Please double-check if not 100% sure. ;)

> As far as I understood you finally agreed on my patch, but you didn't 
> give it a "reviewed-by" all the same; so do you still expect me to 
> change my patch in some way or are you waiting for a "reviewed-by" from 
> someone else to give that a go?
> 
> Why I am also asking: I have another small patch in he pipe that bases 
> on the changed sources and that adds "env erase" support on top of that :-)

I didn't do a thorough review yet. Let me try to do this quickly...

Thanks,
Stefan

> Thanks,
> Harry
> 
>>
>> Thanks,
>> Stefan
>>
>>>
>>> Viele Gr??e,
>>> Harry
>>>
>>>>
>>>> Thanks,
>>>> Stefan
>>>>
>>>>> Cheers,
>>>>> Harry
>>>>>
>>>>>> Thanks,
>>>>>> Stefan
>>>>>>
>>>>>>> ????? /* Is the sector larger than the env (i.e. embedded) */
>>>>>>> ????? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>>>>> ????????? saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>>>>>>> -??????? saved_offset = env_new_offset + CONFIG_ENV_SIZE;
>>>>>>> +??????? saved_offset = offset + CONFIG_ENV_SIZE;
>>>>>>> ????????? saved_buffer = memalign(ARCH_DMA_MINALIGN, saved_size);
>>>>>>> ????????? if (!saved_buffer) {
>>>>>>> ????????????? ret = -ENOMEM;
>>>>>>> ????????????? goto done;
>>>>>>> ????????? }
>>>>>>> -??????? ret = spi_flash_read(env_flash, saved_offset,
>>>>>>> -??????????????????? saved_size, saved_buffer);
>>>>>>> +??????? ret = spi_flash_read(env_flash, saved_offset, saved_size,
>>>>>>> +???????????????????? saved_buffer);
>>>>>>> ????????? if (ret)
>>>>>>> ????????????? goto done;
>>>>>>> ????? }
>>>>>>> @@ -109,35 +118,39 @@ static int env_sf_save(void)
>>>>>>> ????? sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>>>>>>> ????? puts("Erasing SPI flash...");
>>>>>>> -??? ret = spi_flash_erase(env_flash, env_new_offset,
>>>>>>> -??????????????? sector * CONFIG_ENV_SECT_SIZE);
>>>>>>> +??? ret = spi_flash_erase(env_flash, offset,
>>>>>>> +????????????????? sector * CONFIG_ENV_SECT_SIZE);
>>>>>>> ????? if (ret)
>>>>>>> ????????? goto done;
>>>>>>> ????? puts("Writing to SPI flash...");
>>>>>>> -??? ret = spi_flash_write(env_flash, env_new_offset,
>>>>>>> -??????? CONFIG_ENV_SIZE, &env_new);
>>>>>>> +??? ret = spi_flash_write(env_flash, offset,
>>>>>>> +????????????????? CONFIG_ENV_SIZE, &env_new);
>>>>>>> ????? if (ret)
>>>>>>> ????????? goto done;
>>>>>>> ????? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>>>>> -??????? ret = spi_flash_write(env_flash, saved_offset,
>>>>>>> -??????????????????? saved_size, saved_buffer);
>>>>>>> +??????? ret = spi_flash_write(env_flash, saved_offset, saved_size,
>>>>>>> +????????????????????? saved_buffer);
>>>>>>> ????????? if (ret)
>>>>>>> ????????????? goto done;
>>>>>>> ????? }
>>>>>>> -??? ret = spi_flash_write(env_flash, env_offset + 
>>>>>>> offsetof(env_t, flags),
>>>>>>> -??????????????? sizeof(env_new.flags), &flag);
>>>>>>> -??? if (ret)
>>>>>>> -??????? goto done;
>>>>>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>>>>>> +??????? ret = spi_flash_write(env_flash, env_offset + 
>>>>>>> offsetof(env_t, flags),
>>>>>>> +????????????????????? sizeof(env_new.flags), &flag);
>>>>>>> +??????? if (ret)
>>>>>>> +??????????? goto done;
>>>>>>> -??? puts("done\n");
>>>>>>> +??????? puts("done\n");
>>>>>>> -??? gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID : 
>>>>>>> ENV_REDUND;
>>>>>>> +??????? gd->env_valid = gd->env_valid == ENV_REDUND ? ENV_VALID 
>>>>>>> : ENV_REDUND;
>>>>>>> -??? printf("Valid environment: %d\n", (int)gd->env_valid);
>>>>>>> +??????? printf("Valid environment: %d\n", (int)gd->env_valid);
>>>>>>> +#else
>>>>>>> +??????? puts("done\n");
>>>>>>> +#endif
>>>>>>> ?? done:
>>>>>>> ????? if (saved_buffer)
>>>>>>> @@ -146,6 +159,7 @@ static int env_sf_save(void)
>>>>>>> ????? return ret;
>>>>>>> ? }
>>>>>>> +#if CONFIG_IS_ENABLED(SYS_REDUNDAND_ENVIRONMENT)
>>>>>>> ? static int env_sf_load(void)
>>>>>>> ? {
>>>>>>> ????? int ret;
>>>>>>> @@ -183,66 +197,6 @@ out:
>>>>>>> ????? return ret;
>>>>>>> ? }
>>>>>>> ? #else
>>>>>>> -static int env_sf_save(void)
>>>>>>> -{
>>>>>>> -??? u32??? saved_size, saved_offset, sector;
>>>>>>> -??? char??? *saved_buffer = NULL;
>>>>>>> -??? int??? ret = 1;
>>>>>>> -??? env_t??? env_new;
>>>>>>> -
>>>>>>> -??? ret = setup_flash_device();
>>>>>>> -??? if (ret)
>>>>>>> -??????? return ret;
>>>>>>> -
>>>>>>> -??? /* Is the sector larger than the env (i.e. embedded) */
>>>>>>> -??? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>>>>> -??????? saved_size = CONFIG_ENV_SECT_SIZE - CONFIG_ENV_SIZE;
>>>>>>> -??????? saved_offset = CONFIG_ENV_OFFSET + CONFIG_ENV_SIZE;
>>>>>>> -??????? saved_buffer = malloc(saved_size);
>>>>>>> -??????? if (!saved_buffer)
>>>>>>> -??????????? goto done;
>>>>>>> -
>>>>>>> -??????? ret = spi_flash_read(env_flash, saved_offset,
>>>>>>> -??????????? saved_size, saved_buffer);
>>>>>>> -??????? if (ret)
>>>>>>> -??????????? goto done;
>>>>>>> -??? }
>>>>>>> -
>>>>>>> -??? ret = env_export(&env_new);
>>>>>>> -??? if (ret)
>>>>>>> -??????? goto done;
>>>>>>> -
>>>>>>> -??? sector = DIV_ROUND_UP(CONFIG_ENV_SIZE, CONFIG_ENV_SECT_SIZE);
>>>>>>> -
>>>>>>> -??? puts("Erasing SPI flash...");
>>>>>>> -??? ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET,
>>>>>>> -??????? sector * CONFIG_ENV_SECT_SIZE);
>>>>>>> -??? if (ret)
>>>>>>> -??????? goto done;
>>>>>>> -
>>>>>>> -??? puts("Writing to SPI flash...");
>>>>>>> -??? ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET,
>>>>>>> -??????? CONFIG_ENV_SIZE, &env_new);
>>>>>>> -??? if (ret)
>>>>>>> -??????? goto done;
>>>>>>> -
>>>>>>> -??? if (CONFIG_ENV_SECT_SIZE > CONFIG_ENV_SIZE) {
>>>>>>> -??????? ret = spi_flash_write(env_flash, saved_offset,
>>>>>>> -??????????? saved_size, saved_buffer);
>>>>>>> -??????? if (ret)
>>>>>>> -??????????? goto done;
>>>>>>> -??? }
>>>>>>> -
>>>>>>> -??? ret = 0;
>>>>>>> -??? puts("done\n");
>>>>>>> -
>>>>>>> - done:
>>>>>>> -??? if (saved_buffer)
>>>>>>> -??????? free(saved_buffer);
>>>>>>> -
>>>>>>> -??? return ret;
>>>>>>> -}
>>>>>>> -
>>>>>>> ? static int env_sf_load(void)
>>>>>>> ? {
>>>>>>> ????? int ret;
>>>>>>> @@ -258,8 +212,8 @@ static int env_sf_load(void)
>>>>>>> ????? if (ret)
>>>>>>> ????????? goto out;
>>>>>>> -??? ret = spi_flash_read(env_flash,
>>>>>>> -??????? CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE, buf);
>>>>>>> +??? ret = spi_flash_read(env_flash, CONFIG_ENV_OFFSET, 
>>>>>>> CONFIG_ENV_SIZE,
>>>>>>> +???????????????? buf);
>>>>>>> ????? if (ret) {
>>>>>>> ????????? env_set_default("spi_flash_read() failed", 0);
>>>>>>> ????????? goto err_read;
>>>>>>> @@ -292,7 +246,7 @@ static int env_sf_init(void)
>>>>>>> ????? env_t *env_ptr = (env_t *)env_sf_get_env_addr();
>>>>>>> ????? if (crc32(0, env_ptr->data, ENV_SIZE) == env_ptr->crc) {
>>>>>>> -??????? gd->env_addr??? = (ulong)&(env_ptr->data);
>>>>>>> +??????? gd->env_addr??? = (ulong)&env_ptr->data;
>>>>>>> ????????? gd->env_valid??? = 1;
>>>>>>> ????? } else {
>>>>>>> ????????? gd->env_addr = (ulong)&default_environment[0];
>>>>>>>
>>>>>>
>>>>>>
>>>>>> Viele Gr??e,
>>>>>> Stefan
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>> Viele Gr??e,
>>>> Stefan
>>>>
>>>
>>>
>>
>>
>> Viele Gr??e,
>> Stefan
>>


Viele Gr??e,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de