Re: [U-Boot] [PATCH 1/4] sf: Enable prints on erase and write functions
+Mike who is the maintainer
On Wed, Dec 12, 2012 at 7:16 AM, Jagan Teki wrote:
> Hi Simon,
>
> On Wed, Dec 12, 2012 at 8:31 PM, Simon Glass wrote:
>> Hi Jagan,
>>
>> On Wed, Dec 12, 2012 at 6:43 AM, Jagan Teki wrote:
>>> Hi Simon,
>>>
>>> On Wed, Dec 12, 2012 at 12:01 PM, Simon Glass wrote:
Hi Jagan,
On Mon, Dec 10, 2012 at 10:37 PM, Jagan Teki
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 wrote:
>>
>> Hi,
>>
>> On Mon, Dec 10, 2012 at 6:41 AM, Jagannadha Sutradharudu Teki
>> 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
>> > ---
>> > 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@lists.denx.de
>> > http://lists.denx.de/mailman/listinfo/u-boot
>>
>> Regards,
>> Simon
>
>
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/4] sf: Enable prints on erase and write functions
Hi Simon,
On Wed, Dec 12, 2012 at 8:31 PM, Simon Glass wrote:
> Hi Jagan,
>
> On Wed, Dec 12, 2012 at 6:43 AM, Jagan Teki wrote:
>> Hi Simon,
>>
>> On Wed, Dec 12, 2012 at 12:01 PM, Simon Glass wrote:
>>> Hi Jagan,
>>>
>>> On Mon, Dec 10, 2012 at 10:37 PM, Jagan Teki
>>> 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?
Thanks,
Jagan.
>
> Regards,
> Simon
>
>>
>> Thanks,
>> Jagan.
>>>
>>> Regards,
>>> Simon
>>>
Thanks,
Jagan.
On Tue, Dec 11, 2012 at 12:56 AM, Simon Glass wrote:
>
> Hi,
>
> On Mon, Dec 10, 2012 at 6:41 AM, Jagannadha Sutradharudu Teki
> 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
> > ---
> > 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@lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot
>
> Regards,
> Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/4] sf: Enable prints on erase and write functions
Hi Jagan,
On Wed, Dec 12, 2012 at 6:43 AM, Jagan Teki wrote:
> Hi Simon,
>
> On Wed, Dec 12, 2012 at 12:01 PM, Simon Glass wrote:
>> Hi Jagan,
>>
>> On Mon, Dec 10, 2012 at 10:37 PM, Jagan Teki
>> 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.
Regards,
Simon
>
> Thanks,
> Jagan.
>>
>> Regards,
>> Simon
>>
>>>
>>> Thanks,
>>> Jagan.
>>>
>>>
>>> On Tue, Dec 11, 2012 at 12:56 AM, Simon Glass wrote:
Hi,
On Mon, Dec 10, 2012 at 6:41 AM, Jagannadha Sutradharudu Teki
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
> ---
> 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@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
Regards,
Simon
>>>
>>>
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/4] sf: Enable prints on erase and write functions
Hi Simon,
On Wed, Dec 12, 2012 at 12:01 PM, Simon Glass wrote:
> Hi Jagan,
>
> On Mon, Dec 10, 2012 at 10:37 PM, Jagan Teki 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.
Thanks,
Jagan.
>
> Regards,
> Simon
>
>>
>> Thanks,
>> Jagan.
>>
>>
>> On Tue, Dec 11, 2012 at 12:56 AM, Simon Glass wrote:
>>>
>>> Hi,
>>>
>>> On Mon, Dec 10, 2012 at 6:41 AM, Jagannadha Sutradharudu Teki
>>> 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
>>> > ---
>>> > 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@lists.denx.de
>>> > http://lists.denx.de/mailman/listinfo/u-boot
>>>
>>> Regards,
>>> Simon
>>
>>
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/4] sf: Enable prints on erase and write functions
Hi Jagan,
On Mon, Dec 10, 2012 at 10:37 PM, Jagan Teki 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?
Regards,
Simon
>
> Thanks,
> Jagan.
>
>
> On Tue, Dec 11, 2012 at 12:56 AM, Simon Glass wrote:
>>
>> Hi,
>>
>> On Mon, Dec 10, 2012 at 6:41 AM, Jagannadha Sutradharudu Teki
>> 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
>> > ---
>> > 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@lists.denx.de
>> > http://lists.denx.de/mailman/listinfo/u-boot
>>
>> Regards,
>> Simon
>
>
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/4] sf: Enable prints on erase and write functions
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.
Thanks,
Jagan.
On Tue, Dec 11, 2012 at 12:56 AM, Simon Glass wrote:
> Hi,
>
> On Mon, Dec 10, 2012 at 6:41 AM, Jagannadha Sutradharudu Teki
> 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
> > ---
> > 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@lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot
>
> Regards,
> Simon
>
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 1/4] sf: Enable prints on erase and write functions
Hi,
On Mon, Dec 10, 2012 at 6:41 AM, Jagannadha Sutradharudu Teki
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
> ---
> 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@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
