[U-Boot] [PATCH 1/4] sf: Enable prints on erase and write functions

[Simon Glass]

+Mike who is the maintainer

On Wed, Dec 12, 2012 at 7:16 AM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
> Hi Simon,
>
> On Wed, Dec 12, 2012 at 8:31 PM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Jagan,
>>
>> On Wed, Dec 12, 2012 at 6:43 AM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
>>> Hi Simon,
>>>
>>> On Wed, Dec 12, 2012 at 12:01 PM, Simon Glass <sjg at chromium.org> wrote:
>>>> Hi Jagan,
>>>>
>>>> On Mon, Dec 10, 2012 at 10:37 PM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
>>>>> Hi Simon,
>>>>>
>>>>> I understand your concern.
>>>>>
>>>>> But currently there is no prints a/f reading/writing/erasing the SPI flash.
>>>>> User's are unable to confirm whether that particular sf commands are
>>>>> properly done/not.
>>>>
>>>> Well if there is no error, then I suppose it worked ok. We should
>>>> definitely print errors, and progress information can sometimes be
>>>> useful for very long operations. But serial and LCD output is slow, so
>>>> it can affect run time, quite apart from the verbosity, so IMO the
>>>> less the better.
>>>>
>>>> Would it not be possible to put this message into cmd_sf.c?
>>>
>>> Yes it will possible to do this on cmd_sf.
>>> But I am not understanding what is the side effect, if we put in this area..
>>> will you please elaborate.
>>
>> Well if someone writes some code that calls the spi_flash interface
>> from else where, such as:
>>
>> http://patchwork.ozlabs.org/patch/190164/
>>
>> or defines CONFIG_ENV_IS_IN_SPI_FLASH
>>
>> then your patch will start printing messages when none are required.
>>
>> By putting it in cmd_sf.c, and perhaps evening making it optional
>> through a CONFIG_SF_VERBOSE, you make it possible for people to keep
>> the existing behavior.
>
> Thanks for your information.
> Now I understood the whole scenario's.
>
> OK, can I move the prints on cmd_sf.c with CONFIG_SF_VERBOSE?

That would be my preference, but Mike might have thoughts on this.

Regards,
Simon

>
> Thanks,
> Jagan.
>
>>
>> Regards,
>> Simon
>>
>>>
>>> Thanks,
>>> Jagan.
>>>>
>>>> Regards,
>>>> Simon
>>>>
>>>>>
>>>>> Thanks,
>>>>> Jagan.
>>>>>
>>>>>
>>>>> On Tue, Dec 11, 2012 at 12:56 AM, Simon Glass <sjg at chromium.org> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On Mon, Dec 10, 2012 at 6:41 AM, Jagannadha Sutradharudu Teki
>>>>>> <jagannadh.teki at gmail.com> wrote:
>>>>>> > This patch provides to enabled the prints on erase and write
>>>>>> > functions to make sure that how many bytes erase/write into flash
>>>>>> > device.
>>>>>> >
>>>>>> > Signed-off-by: Jagannadha Sutradharudu Teki <jagannadh.teki at gmail.com>
>>>>>> > ---
>>>>>> >  drivers/mtd/spi/spi_flash.c |    4 ++--
>>>>>> >  1 files changed, 2 insertions(+), 2 deletions(-)
>>>>>> >
>>>>>> > diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
>>>>>> > index 00aece9..464c2ab 100644
>>>>>> > --- a/drivers/mtd/spi/spi_flash.c
>>>>>> > +++ b/drivers/mtd/spi/spi_flash.c
>>>>>> > @@ -115,7 +115,7 @@ int spi_flash_cmd_write_multi(struct spi_flash
>>>>>> > *flash, u32 offset,
>>>>>> >                 byte_addr = 0;
>>>>>> >         }
>>>>>> >
>>>>>> > -       debug("SF: program %s %zu bytes @ %#x\n",
>>>>>> > +       printf("SF: program %s %zu bytes @ %#x\n",
>>>>>> >               ret ? "failure" : "success", len, offset);
>>>>>>
>>>>>> I don't think we want this - it will make programming very slow and
>>>>>> verbose.
>>>>>>
>>>>>> >
>>>>>> >         spi_release_bus(flash->spi);
>>>>>> > @@ -235,7 +235,7 @@ int spi_flash_cmd_erase(struct spi_flash *flash, u32
>>>>>> > offset, size_t len)
>>>>>> >                         goto out;
>>>>>> >         }
>>>>>> >
>>>>>> > -       debug("SF: Successfully erased %zu bytes @ %#x\n", len, start);
>>>>>> > +       printf("SF: Successfully erased %zu bytes @ %#x\n", len, start);
>>>>>>
>>>>>> You may want to put this code into cmd_sf instead, where it is
>>>>>> reasonable to add messages. You are changing core spi code which might
>>>>>> be used from many places.
>>>>>>
>>>>>> >
>>>>>> >   out:
>>>>>> >         spi_release_bus(flash->spi);
>>>>>> > --
>>>>>> > 1.7.0.4
>>>>>> >
>>>>>> > _______________________________________________
>>>>>> > U-Boot mailing list
>>>>>> > U-Boot at lists.denx.de
>>>>>> > http://lists.denx.de/mailman/listinfo/u-boot
>>>>>>
>>>>>> Regards,
>>>>>> Simon
>>>>>
>>>>>