diff mbox

[U-Boot,3/4] sf: Add configuration register writing

Message ID 1355150521-3339-4-git-send-email-jagannadh.teki@gmail.com
State Superseded
Delegated to: Mike Frysinger
Headers show

Commit Message

Jagan Teki Dec. 10, 2012, 2:42 p.m. UTC
This patch provides support to program a flash config register.

Configuration register contains the control bits used to configure
the different configurations and security features of a device.

User need to set these bits through spi_flash_cmd_write_config()
based on their usage.

Signed-off-by: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com>
---
 drivers/mtd/spi/spi_flash.c          |   27 +++++++++++++++++++++++++++
 drivers/mtd/spi/spi_flash_internal.h |    3 +++
 2 files changed, 30 insertions(+), 0 deletions(-)

Comments

Simon Glass Dec. 12, 2012, 6:35 a.m. UTC | #1
Hi,

On Mon, Dec 10, 2012 at 6:42 AM, Jagannadha Sutradharudu Teki
<jagannadh.teki@gmail.com> wrote:
> This patch provides support to program a flash config register.
>
> Configuration register contains the control bits used to configure
> the different configurations and security features of a device.
>
> User need to set these bits through spi_flash_cmd_write_config()
> based on their usage.
>
> Signed-off-by: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com>
> ---
>  drivers/mtd/spi/spi_flash.c          |   27 +++++++++++++++++++++++++++
>  drivers/mtd/spi/spi_flash_internal.h |    3 +++
>  2 files changed, 30 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
> index 800ed8b..a8f0af0 100644
> --- a/drivers/mtd/spi/spi_flash.c
> +++ b/drivers/mtd/spi/spi_flash.c
> @@ -274,6 +274,33 @@ int spi_flash_cmd_write_status(struct spi_flash *flash, u8 sr)
>         return 0;
>  }
>
> +int spi_flash_cmd_write_config(struct spi_flash *flash, u16 cr)
> +{
> +       u8 cmd;
> +       int ret;
> +
> +       ret = spi_flash_cmd_write_enable(flash);
> +       if (ret < 0) {
> +               debug("SF: enabling write failed\n");
> +               return ret;
> +       }
> +
> +       cmd = CMD_WRITE_STATUS;
> +       ret = spi_flash_cmd_write(flash->spi, &cmd, 1, &cr, 2);

Does this assume a particular endianness? Perhaps should put the
values into a byte array instead?

> +       if (ret) {
> +               debug("SF: fail to write config register\n");
> +               return ret;
> +       }
> +
> +       ret = spi_flash_cmd_wait_ready(flash, SPI_FLASH_PROG_TIMEOUT);
> +       if (ret < 0) {
> +               debug("SF: write config register timed out\n");
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
>  /*
>   * The following table holds all device probe functions
>   *
> diff --git a/drivers/mtd/spi/spi_flash_internal.h b/drivers/mtd/spi/spi_flash_internal.h
> index 141cfa8..9287778 100644
> --- a/drivers/mtd/spi/spi_flash_internal.h
> +++ b/drivers/mtd/spi/spi_flash_internal.h
> @@ -77,6 +77,9 @@ static inline int spi_flash_cmd_write_disable(struct spi_flash *flash)
>  /* Program the status register. */
>  int spi_flash_cmd_write_status(struct spi_flash *flash, u8 sr);
>
> +/* Program the config register. */
> +int spi_flash_cmd_write_config(struct spi_flash *flash, u16 cr);
> +
>  /*
>   * Same as spi_flash_cmd_read() except it also claims/releases the SPI
>   * bus. Used as common part of the ->read() operation.
> --
> 1.7.0.4
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

Regards,
Simon
Jagan Teki Dec. 12, 2012, 3:20 p.m. UTC | #2
Hi Simon,

On Wed, Dec 12, 2012 at 12:05 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi,
>
> On Mon, Dec 10, 2012 at 6:42 AM, Jagannadha Sutradharudu Teki
> <jagannadh.teki@gmail.com> wrote:
>> This patch provides support to program a flash config register.
>>
>> Configuration register contains the control bits used to configure
>> the different configurations and security features of a device.
>>
>> User need to set these bits through spi_flash_cmd_write_config()
>> based on their usage.
>>
>> Signed-off-by: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com>
>> ---
>>  drivers/mtd/spi/spi_flash.c          |   27 +++++++++++++++++++++++++++
>>  drivers/mtd/spi/spi_flash_internal.h |    3 +++
>>  2 files changed, 30 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
>> index 800ed8b..a8f0af0 100644
>> --- a/drivers/mtd/spi/spi_flash.c
>> +++ b/drivers/mtd/spi/spi_flash.c
>> @@ -274,6 +274,33 @@ int spi_flash_cmd_write_status(struct spi_flash *flash, u8 sr)
>>         return 0;
>>  }
>>
>> +int spi_flash_cmd_write_config(struct spi_flash *flash, u16 cr)
>> +{
>> +       u8 cmd;
>> +       int ret;
>> +
>> +       ret = spi_flash_cmd_write_enable(flash);
>> +       if (ret < 0) {
>> +               debug("SF: enabling write failed\n");
>> +               return ret;
>> +       }
>> +
>> +       cmd = CMD_WRITE_STATUS;
>> +       ret = spi_flash_cmd_write(flash->spi, &cmd, 1, &cr, 2);
>
> Does this assume a particular endianness? Perhaps should put the
> values into a byte array instead?

Yes it follows the endianness.

On my QPP patch,
ret = spi_flash_cmd_write_config(flash, 0x0200);

where 02 is config and 00 is status register
WRR have CMD+status+config => for CMD+16 data format.

Let me know if you need any info.

Thanks,
Jagan.

>
>> +       if (ret) {
>> +               debug("SF: fail to write config register\n");
>> +               return ret;
>> +       }
>> +
>> +       ret = spi_flash_cmd_wait_ready(flash, SPI_FLASH_PROG_TIMEOUT);
>> +       if (ret < 0) {
>> +               debug("SF: write config register timed out\n");
>> +               return ret;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>>  /*
>>   * The following table holds all device probe functions
>>   *
>> diff --git a/drivers/mtd/spi/spi_flash_internal.h b/drivers/mtd/spi/spi_flash_internal.h
>> index 141cfa8..9287778 100644
>> --- a/drivers/mtd/spi/spi_flash_internal.h
>> +++ b/drivers/mtd/spi/spi_flash_internal.h
>> @@ -77,6 +77,9 @@ static inline int spi_flash_cmd_write_disable(struct spi_flash *flash)
>>  /* Program the status register. */
>>  int spi_flash_cmd_write_status(struct spi_flash *flash, u8 sr);
>>
>> +/* Program the config register. */
>> +int spi_flash_cmd_write_config(struct spi_flash *flash, u16 cr);
>> +
>>  /*
>>   * Same as spi_flash_cmd_read() except it also claims/releases the SPI
>>   * bus. Used as common part of the ->read() operation.
>> --
>> 1.7.0.4
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot@lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>
> Regards,
> Simon
Simon Glass Dec. 12, 2012, 3:23 p.m. UTC | #3
Hi Jagan,

On Wed, Dec 12, 2012 at 7:20 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote:
> Hi Simon,
>
> On Wed, Dec 12, 2012 at 12:05 PM, Simon Glass <sjg@chromium.org> wrote:
>> Hi,
>>
>> On Mon, Dec 10, 2012 at 6:42 AM, Jagannadha Sutradharudu Teki
>> <jagannadh.teki@gmail.com> wrote:
>>> This patch provides support to program a flash config register.
>>>
>>> Configuration register contains the control bits used to configure
>>> the different configurations and security features of a device.
>>>
>>> User need to set these bits through spi_flash_cmd_write_config()
>>> based on their usage.
>>>
>>> Signed-off-by: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com>
>>> ---
>>>  drivers/mtd/spi/spi_flash.c          |   27 +++++++++++++++++++++++++++
>>>  drivers/mtd/spi/spi_flash_internal.h |    3 +++
>>>  2 files changed, 30 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
>>> index 800ed8b..a8f0af0 100644
>>> --- a/drivers/mtd/spi/spi_flash.c
>>> +++ b/drivers/mtd/spi/spi_flash.c
>>> @@ -274,6 +274,33 @@ int spi_flash_cmd_write_status(struct spi_flash *flash, u8 sr)
>>>         return 0;
>>>  }
>>>
>>> +int spi_flash_cmd_write_config(struct spi_flash *flash, u16 cr)
>>> +{
>>> +       u8 cmd;
>>> +       int ret;
>>> +
>>> +       ret = spi_flash_cmd_write_enable(flash);
>>> +       if (ret < 0) {
>>> +               debug("SF: enabling write failed\n");
>>> +               return ret;
>>> +       }
>>> +
>>> +       cmd = CMD_WRITE_STATUS;
>>> +       ret = spi_flash_cmd_write(flash->spi, &cmd, 1, &cr, 2);
>>
>> Does this assume a particular endianness? Perhaps should put the
>> values into a byte array instead?
>
> Yes it follows the endianness.

My concern was more that u16 is being used to send two bytes. How about:

u8 data[2];

put_unaligned_le16(cr, data)

>
> On my QPP patch,
> ret = spi_flash_cmd_write_config(flash, 0x0200);
>
> where 02 is config and 00 is status register
> WRR have CMD+status+config => for CMD+16 data format.
>
> Let me know if you need any info.

OK.

Regards,
Simon

>
> Thanks,
> Jagan.
>
>>
>>> +       if (ret) {
>>> +               debug("SF: fail to write config register\n");
>>> +               return ret;
>>> +       }
>>> +
>>> +       ret = spi_flash_cmd_wait_ready(flash, SPI_FLASH_PROG_TIMEOUT);
>>> +       if (ret < 0) {
>>> +               debug("SF: write config register timed out\n");
>>> +               return ret;
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +
>>>  /*
>>>   * The following table holds all device probe functions
>>>   *
>>> diff --git a/drivers/mtd/spi/spi_flash_internal.h b/drivers/mtd/spi/spi_flash_internal.h
>>> index 141cfa8..9287778 100644
>>> --- a/drivers/mtd/spi/spi_flash_internal.h
>>> +++ b/drivers/mtd/spi/spi_flash_internal.h
>>> @@ -77,6 +77,9 @@ static inline int spi_flash_cmd_write_disable(struct spi_flash *flash)
>>>  /* Program the status register. */
>>>  int spi_flash_cmd_write_status(struct spi_flash *flash, u8 sr);
>>>
>>> +/* Program the config register. */
>>> +int spi_flash_cmd_write_config(struct spi_flash *flash, u16 cr);
>>> +
>>>  /*
>>>   * Same as spi_flash_cmd_read() except it also claims/releases the SPI
>>>   * bus. Used as common part of the ->read() operation.
>>> --
>>> 1.7.0.4
>>>
>>> _______________________________________________
>>> U-Boot mailing list
>>> U-Boot@lists.denx.de
>>> http://lists.denx.de/mailman/listinfo/u-boot
>>
>> Regards,
>> Simon
Jagan Teki Dec. 12, 2012, 4:21 p.m. UTC | #4
Hi Simon,

On Wed, Dec 12, 2012 at 8:53 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi Jagan,
>
> On Wed, Dec 12, 2012 at 7:20 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote:
>> Hi Simon,
>>
>> On Wed, Dec 12, 2012 at 12:05 PM, Simon Glass <sjg@chromium.org> wrote:
>>> Hi,
>>>
>>> On Mon, Dec 10, 2012 at 6:42 AM, Jagannadha Sutradharudu Teki
>>> <jagannadh.teki@gmail.com> wrote:
>>>> This patch provides support to program a flash config register.
>>>>
>>>> Configuration register contains the control bits used to configure
>>>> the different configurations and security features of a device.
>>>>
>>>> User need to set these bits through spi_flash_cmd_write_config()
>>>> based on their usage.
>>>>
>>>> Signed-off-by: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com>
>>>> ---
>>>>  drivers/mtd/spi/spi_flash.c          |   27 +++++++++++++++++++++++++++
>>>>  drivers/mtd/spi/spi_flash_internal.h |    3 +++
>>>>  2 files changed, 30 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
>>>> index 800ed8b..a8f0af0 100644
>>>> --- a/drivers/mtd/spi/spi_flash.c
>>>> +++ b/drivers/mtd/spi/spi_flash.c
>>>> @@ -274,6 +274,33 @@ int spi_flash_cmd_write_status(struct spi_flash *flash, u8 sr)
>>>>         return 0;
>>>>  }
>>>>
>>>> +int spi_flash_cmd_write_config(struct spi_flash *flash, u16 cr)
>>>> +{
>>>> +       u8 cmd;
>>>> +       int ret;
>>>> +
>>>> +       ret = spi_flash_cmd_write_enable(flash);
>>>> +       if (ret < 0) {
>>>> +               debug("SF: enabling write failed\n");
>>>> +               return ret;
>>>> +       }
>>>> +
>>>> +       cmd = CMD_WRITE_STATUS;
>>>> +       ret = spi_flash_cmd_write(flash->spi, &cmd, 1, &cr, 2);
>>>
>>> Does this assume a particular endianness? Perhaps should put the
>>> values into a byte array instead?
>>
>> Yes it follows the endianness.
>
> My concern was more that u16 is being used to send two bytes. How about:
>
> u8 data[2];

Means I need to send status on data[1] and config on data[0].?

>
> put_unaligned_le16(cr, data)

I couldn't understand this, may be for endianness.?

Thanks,
Jagan.

>
>>
>> On my QPP patch,
>> ret = spi_flash_cmd_write_config(flash, 0x0200);
>>
>> where 02 is config and 00 is status register
>> WRR have CMD+status+config => for CMD+16 data format.
>>
>> Let me know if you need any info.
>
> OK.
>
> Regards,
> Simon
>
>>
>> Thanks,
>> Jagan.
>>
>>>
>>>> +       if (ret) {
>>>> +               debug("SF: fail to write config register\n");
>>>> +               return ret;
>>>> +       }
>>>> +
>>>> +       ret = spi_flash_cmd_wait_ready(flash, SPI_FLASH_PROG_TIMEOUT);
>>>> +       if (ret < 0) {
>>>> +               debug("SF: write config register timed out\n");
>>>> +               return ret;
>>>> +       }
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>>  /*
>>>>   * The following table holds all device probe functions
>>>>   *
>>>> diff --git a/drivers/mtd/spi/spi_flash_internal.h b/drivers/mtd/spi/spi_flash_internal.h
>>>> index 141cfa8..9287778 100644
>>>> --- a/drivers/mtd/spi/spi_flash_internal.h
>>>> +++ b/drivers/mtd/spi/spi_flash_internal.h
>>>> @@ -77,6 +77,9 @@ static inline int spi_flash_cmd_write_disable(struct spi_flash *flash)
>>>>  /* Program the status register. */
>>>>  int spi_flash_cmd_write_status(struct spi_flash *flash, u8 sr);
>>>>
>>>> +/* Program the config register. */
>>>> +int spi_flash_cmd_write_config(struct spi_flash *flash, u16 cr);
>>>> +
>>>>  /*
>>>>   * Same as spi_flash_cmd_read() except it also claims/releases the SPI
>>>>   * bus. Used as common part of the ->read() operation.
>>>> --
>>>> 1.7.0.4
>>>>
>>>> _______________________________________________
>>>> U-Boot mailing list
>>>> U-Boot@lists.denx.de
>>>> http://lists.denx.de/mailman/listinfo/u-boot
>>>
>>> Regards,
>>> Simon
Simon Glass Dec. 12, 2012, 10:33 p.m. UTC | #5
Hi Jagan,

On Wed, Dec 12, 2012 at 8:21 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote:
> Hi Simon,
>
> On Wed, Dec 12, 2012 at 8:53 PM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Jagan,
>>
>> On Wed, Dec 12, 2012 at 7:20 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote:
>>> Hi Simon,
>>>
>>> On Wed, Dec 12, 2012 at 12:05 PM, Simon Glass <sjg@chromium.org> wrote:
>>>> Hi,
>>>>
>>>> On Mon, Dec 10, 2012 at 6:42 AM, Jagannadha Sutradharudu Teki
>>>> <jagannadh.teki@gmail.com> wrote:
>>>>> This patch provides support to program a flash config register.
>>>>>
>>>>> Configuration register contains the control bits used to configure
>>>>> the different configurations and security features of a device.
>>>>>
>>>>> User need to set these bits through spi_flash_cmd_write_config()
>>>>> based on their usage.
>>>>>
>>>>> Signed-off-by: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com>
>>>>> ---
>>>>>  drivers/mtd/spi/spi_flash.c          |   27 +++++++++++++++++++++++++++
>>>>>  drivers/mtd/spi/spi_flash_internal.h |    3 +++
>>>>>  2 files changed, 30 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
>>>>> index 800ed8b..a8f0af0 100644
>>>>> --- a/drivers/mtd/spi/spi_flash.c
>>>>> +++ b/drivers/mtd/spi/spi_flash.c
>>>>> @@ -274,6 +274,33 @@ int spi_flash_cmd_write_status(struct spi_flash *flash, u8 sr)
>>>>>         return 0;
>>>>>  }
>>>>>
>>>>> +int spi_flash_cmd_write_config(struct spi_flash *flash, u16 cr)
>>>>> +{
>>>>> +       u8 cmd;
>>>>> +       int ret;
>>>>> +
>>>>> +       ret = spi_flash_cmd_write_enable(flash);
>>>>> +       if (ret < 0) {
>>>>> +               debug("SF: enabling write failed\n");
>>>>> +               return ret;
>>>>> +       }
>>>>> +
>>>>> +       cmd = CMD_WRITE_STATUS;
>>>>> +       ret = spi_flash_cmd_write(flash->spi, &cmd, 1, &cr, 2);
>>>>
>>>> Does this assume a particular endianness? Perhaps should put the
>>>> values into a byte array instead?
>>>
>>> Yes it follows the endianness.
>>
>> My concern was more that u16 is being used to send two bytes. How about:
>>
>> u8 data[2];
>
> Means I need to send status on data[1] and config on data[0].?
>
>>
>> put_unaligned_le16(cr, data)
>
> I couldn't understand this, may be for endianness.?

Yes that's right. Just checking that your code will work correctly on
a big-endian machine. Will it? It is normally not a good idea to cast
a short into a char[2].

Regards,
Simon

>
> Thanks,
> Jagan.
>
>>
>>>
>>> On my QPP patch,
>>> ret = spi_flash_cmd_write_config(flash, 0x0200);
>>>
>>> where 02 is config and 00 is status register
>>> WRR have CMD+status+config => for CMD+16 data format.
>>>
>>> Let me know if you need any info.
>>
>> OK.
>>
>> Regards,
>> Simon
>>
>>>
>>> Thanks,
>>> Jagan.
>>>
>>>>
>>>>> +       if (ret) {
>>>>> +               debug("SF: fail to write config register\n");
>>>>> +               return ret;
>>>>> +       }
>>>>> +
>>>>> +       ret = spi_flash_cmd_wait_ready(flash, SPI_FLASH_PROG_TIMEOUT);
>>>>> +       if (ret < 0) {
>>>>> +               debug("SF: write config register timed out\n");
>>>>> +               return ret;
>>>>> +       }
>>>>> +
>>>>> +       return 0;
>>>>> +}
>>>>> +
>>>>>  /*
>>>>>   * The following table holds all device probe functions
>>>>>   *
>>>>> diff --git a/drivers/mtd/spi/spi_flash_internal.h b/drivers/mtd/spi/spi_flash_internal.h
>>>>> index 141cfa8..9287778 100644
>>>>> --- a/drivers/mtd/spi/spi_flash_internal.h
>>>>> +++ b/drivers/mtd/spi/spi_flash_internal.h
>>>>> @@ -77,6 +77,9 @@ static inline int spi_flash_cmd_write_disable(struct spi_flash *flash)
>>>>>  /* Program the status register. */
>>>>>  int spi_flash_cmd_write_status(struct spi_flash *flash, u8 sr);
>>>>>
>>>>> +/* Program the config register. */
>>>>> +int spi_flash_cmd_write_config(struct spi_flash *flash, u16 cr);
>>>>> +
>>>>>  /*
>>>>>   * Same as spi_flash_cmd_read() except it also claims/releases the SPI
>>>>>   * bus. Used as common part of the ->read() operation.
>>>>> --
>>>>> 1.7.0.4
>>>>>
>>>>> _______________________________________________
>>>>> U-Boot mailing list
>>>>> U-Boot@lists.denx.de
>>>>> http://lists.denx.de/mailman/listinfo/u-boot
>>>>
>>>> Regards,
>>>> Simon
Jagan Teki Dec. 13, 2012, 4:08 p.m. UTC | #6
Hi Simon,

On Thu, Dec 13, 2012 at 4:03 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Jagan,
>
> On Wed, Dec 12, 2012 at 8:21 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote:
>> Hi Simon,
>>
>> On Wed, Dec 12, 2012 at 8:53 PM, Simon Glass <sjg@chromium.org> wrote:
>>> Hi Jagan,
>>>
>>> On Wed, Dec 12, 2012 at 7:20 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote:
>>>> Hi Simon,
>>>>
>>>> On Wed, Dec 12, 2012 at 12:05 PM, Simon Glass <sjg@chromium.org> wrote:
>>>>> Hi,
>>>>>
>>>>> On Mon, Dec 10, 2012 at 6:42 AM, Jagannadha Sutradharudu Teki
>>>>> <jagannadh.teki@gmail.com> wrote:
>>>>>> This patch provides support to program a flash config register.
>>>>>>
>>>>>> Configuration register contains the control bits used to configure
>>>>>> the different configurations and security features of a device.
>>>>>>
>>>>>> User need to set these bits through spi_flash_cmd_write_config()
>>>>>> based on their usage.
>>>>>>
>>>>>> Signed-off-by: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com>
>>>>>> ---
>>>>>>  drivers/mtd/spi/spi_flash.c          |   27 +++++++++++++++++++++++++++
>>>>>>  drivers/mtd/spi/spi_flash_internal.h |    3 +++
>>>>>>  2 files changed, 30 insertions(+), 0 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
>>>>>> index 800ed8b..a8f0af0 100644
>>>>>> --- a/drivers/mtd/spi/spi_flash.c
>>>>>> +++ b/drivers/mtd/spi/spi_flash.c
>>>>>> @@ -274,6 +274,33 @@ int spi_flash_cmd_write_status(struct spi_flash *flash, u8 sr)
>>>>>>         return 0;
>>>>>>  }
>>>>>>
>>>>>> +int spi_flash_cmd_write_config(struct spi_flash *flash, u16 cr)
>>>>>> +{
>>>>>> +       u8 cmd;
>>>>>> +       int ret;
>>>>>> +
>>>>>> +       ret = spi_flash_cmd_write_enable(flash);
>>>>>> +       if (ret < 0) {
>>>>>> +               debug("SF: enabling write failed\n");
>>>>>> +               return ret;
>>>>>> +       }
>>>>>> +
>>>>>> +       cmd = CMD_WRITE_STATUS;
>>>>>> +       ret = spi_flash_cmd_write(flash->spi, &cmd, 1, &cr, 2);
>>>>>
>>>>> Does this assume a particular endianness? Perhaps should put the
>>>>> values into a byte array instead?
>>>>
>>>> Yes it follows the endianness.
>>>
>>> My concern was more that u16 is being used to send two bytes. How about:
>>>
>>> u8 data[2];
>>
>> Means I need to send status on data[1] and config on data[0].?
>>
>>>
>>> put_unaligned_le16(cr, data)
>>
>> I couldn't understand this, may be for endianness.?
>
> Yes that's right. Just checking that your code will work correctly on
> a big-endian machine. Will it? It is normally not a good idea to cast
> a short into a char[2].

OK.

Let me explain my senario here.
in below function I am passing 0x0200
spi_flash_cmd_write_config(flash, 0x0200);

out of 2-byte data 0x0200 first 00 will pass as a status register value
and 0x02 will pass as a config value.it's took the bytes from LSB
onwards.

Can you please explain how put_unaligned_le16(cr, data will work
for this case?

Thanks,
Jagan.

>
> Regards,
> Simon
>
>>
>> Thanks,
>> Jagan.
>>
>>>
>>>>
>>>> On my QPP patch,
>>>> ret = spi_flash_cmd_write_config(flash, 0x0200);
>>>>
>>>> where 02 is config and 00 is status register
>>>> WRR have CMD+status+config => for CMD+16 data format.
>>>>
>>>> Let me know if you need any info.
>>>
>>> OK.
>>>
>>> Regards,
>>> Simon
>>>
>>>>
>>>> Thanks,
>>>> Jagan.
>>>>
>>>>>
>>>>>> +       if (ret) {
>>>>>> +               debug("SF: fail to write config register\n");
>>>>>> +               return ret;
>>>>>> +       }
>>>>>> +
>>>>>> +       ret = spi_flash_cmd_wait_ready(flash, SPI_FLASH_PROG_TIMEOUT);
>>>>>> +       if (ret < 0) {
>>>>>> +               debug("SF: write config register timed out\n");
>>>>>> +               return ret;
>>>>>> +       }
>>>>>> +
>>>>>> +       return 0;
>>>>>> +}
>>>>>> +
>>>>>>  /*
>>>>>>   * The following table holds all device probe functions
>>>>>>   *
>>>>>> diff --git a/drivers/mtd/spi/spi_flash_internal.h b/drivers/mtd/spi/spi_flash_internal.h
>>>>>> index 141cfa8..9287778 100644
>>>>>> --- a/drivers/mtd/spi/spi_flash_internal.h
>>>>>> +++ b/drivers/mtd/spi/spi_flash_internal.h
>>>>>> @@ -77,6 +77,9 @@ static inline int spi_flash_cmd_write_disable(struct spi_flash *flash)
>>>>>>  /* Program the status register. */
>>>>>>  int spi_flash_cmd_write_status(struct spi_flash *flash, u8 sr);
>>>>>>
>>>>>> +/* Program the config register. */
>>>>>> +int spi_flash_cmd_write_config(struct spi_flash *flash, u16 cr);
>>>>>> +
>>>>>>  /*
>>>>>>   * Same as spi_flash_cmd_read() except it also claims/releases the SPI
>>>>>>   * bus. Used as common part of the ->read() operation.
>>>>>> --
>>>>>> 1.7.0.4
>>>>>>
>>>>>> _______________________________________________
>>>>>> U-Boot mailing list
>>>>>> U-Boot@lists.denx.de
>>>>>> http://lists.denx.de/mailman/listinfo/u-boot
>>>>>
>>>>> Regards,
>>>>> Simon
Simon Glass Dec. 14, 2012, 1:20 a.m. UTC | #7
Hi Jagan,

On Thu, Dec 13, 2012 at 8:08 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote:
> Hi Simon,
>
> On Thu, Dec 13, 2012 at 4:03 AM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Jagan,
>>
>> On Wed, Dec 12, 2012 at 8:21 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote:
>>> Hi Simon,
>>>
>>> On Wed, Dec 12, 2012 at 8:53 PM, Simon Glass <sjg@chromium.org> wrote:
>>>> Hi Jagan,
>>>>
>>>> On Wed, Dec 12, 2012 at 7:20 AM, Jagan Teki <jagannadh.teki@gmail.com> wrote:
>>>>> Hi Simon,
>>>>>
>>>>> On Wed, Dec 12, 2012 at 12:05 PM, Simon Glass <sjg@chromium.org> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On Mon, Dec 10, 2012 at 6:42 AM, Jagannadha Sutradharudu Teki
>>>>>> <jagannadh.teki@gmail.com> wrote:
>>>>>>> This patch provides support to program a flash config register.
>>>>>>>
>>>>>>> Configuration register contains the control bits used to configure
>>>>>>> the different configurations and security features of a device.
>>>>>>>
>>>>>>> User need to set these bits through spi_flash_cmd_write_config()
>>>>>>> based on their usage.
>>>>>>>
>>>>>>> Signed-off-by: Jagannadha Sutradharudu Teki <jagannadh.teki@gmail.com>
>>>>>>> ---
>>>>>>>  drivers/mtd/spi/spi_flash.c          |   27 +++++++++++++++++++++++++++
>>>>>>>  drivers/mtd/spi/spi_flash_internal.h |    3 +++
>>>>>>>  2 files changed, 30 insertions(+), 0 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
>>>>>>> index 800ed8b..a8f0af0 100644
>>>>>>> --- a/drivers/mtd/spi/spi_flash.c
>>>>>>> +++ b/drivers/mtd/spi/spi_flash.c
>>>>>>> @@ -274,6 +274,33 @@ int spi_flash_cmd_write_status(struct spi_flash *flash, u8 sr)
>>>>>>>         return 0;
>>>>>>>  }
>>>>>>>
>>>>>>> +int spi_flash_cmd_write_config(struct spi_flash *flash, u16 cr)
>>>>>>> +{
>>>>>>> +       u8 cmd;
>>>>>>> +       int ret;
>>>>>>> +
>>>>>>> +       ret = spi_flash_cmd_write_enable(flash);
>>>>>>> +       if (ret < 0) {
>>>>>>> +               debug("SF: enabling write failed\n");
>>>>>>> +               return ret;
>>>>>>> +       }
>>>>>>> +
>>>>>>> +       cmd = CMD_WRITE_STATUS;
>>>>>>> +       ret = spi_flash_cmd_write(flash->spi, &cmd, 1, &cr, 2);
>>>>>>
>>>>>> Does this assume a particular endianness? Perhaps should put the
>>>>>> values into a byte array instead?
>>>>>
>>>>> Yes it follows the endianness.
>>>>
>>>> My concern was more that u16 is being used to send two bytes. How about:
>>>>
>>>> u8 data[2];
>>>
>>> Means I need to send status on data[1] and config on data[0].?
>>>
>>>>
>>>> put_unaligned_le16(cr, data)
>>>
>>> I couldn't understand this, may be for endianness.?
>>
>> Yes that's right. Just checking that your code will work correctly on
>> a big-endian machine. Will it? It is normally not a good idea to cast
>> a short into a char[2].
>
> OK.
>
> Let me explain my senario here.
> in below function I am passing 0x0200
> spi_flash_cmd_write_config(flash, 0x0200);
>
> out of 2-byte data 0x0200 first 00 will pass as a status register value
> and 0x02 will pass as a config value.it's took the bytes from LSB
> onwards.
>
> Can you please explain how put_unaligned_le16(cr, data will work
> for this case?

Possibly...

The prototype is:

int spi_flash_cmd_write(struct spi_slave *spi, const u8 *cmd, size_t cmd_len,
		const void *data, size_t data_len);

You are doing:

u16 value;

ret = spi_flash_cmd_write(flash->spi, &cmd, 1, &cr, 2);

Passing a u16 cast to a u8 * is probably not endian safe.

Instead I think you should do:

u8 cr[2];

(get bytes into cr)
ret = spi_flash_cmd_write(flash->spi, &cmd, 1, cr, 2);

Regards,
Simon

>
> Thanks,
> Jagan.
>
>>
>> Regards,
>> Simon
>>
>>>
>>> Thanks,
>>> Jagan.
>>>
>>>>
>>>>>
>>>>> On my QPP patch,
>>>>> ret = spi_flash_cmd_write_config(flash, 0x0200);
>>>>>
>>>>> where 02 is config and 00 is status register
>>>>> WRR have CMD+status+config => for CMD+16 data format.
>>>>>
>>>>> Let me know if you need any info.
>>>>
>>>> OK.
>>>>
>>>> Regards,
>>>> Simon
>>>>
>>>>>
>>>>> Thanks,
>>>>> Jagan.
>>>>>
>>>>>>
>>>>>>> +       if (ret) {
>>>>>>> +               debug("SF: fail to write config register\n");
>>>>>>> +               return ret;
>>>>>>> +       }
>>>>>>> +
>>>>>>> +       ret = spi_flash_cmd_wait_ready(flash, SPI_FLASH_PROG_TIMEOUT);
>>>>>>> +       if (ret < 0) {
>>>>>>> +               debug("SF: write config register timed out\n");
>>>>>>> +               return ret;
>>>>>>> +       }
>>>>>>> +
>>>>>>> +       return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>>  /*
>>>>>>>   * The following table holds all device probe functions
>>>>>>>   *
>>>>>>> diff --git a/drivers/mtd/spi/spi_flash_internal.h b/drivers/mtd/spi/spi_flash_internal.h
>>>>>>> index 141cfa8..9287778 100644
>>>>>>> --- a/drivers/mtd/spi/spi_flash_internal.h
>>>>>>> +++ b/drivers/mtd/spi/spi_flash_internal.h
>>>>>>> @@ -77,6 +77,9 @@ static inline int spi_flash_cmd_write_disable(struct spi_flash *flash)
>>>>>>>  /* Program the status register. */
>>>>>>>  int spi_flash_cmd_write_status(struct spi_flash *flash, u8 sr);
>>>>>>>
>>>>>>> +/* Program the config register. */
>>>>>>> +int spi_flash_cmd_write_config(struct spi_flash *flash, u16 cr);
>>>>>>> +
>>>>>>>  /*
>>>>>>>   * Same as spi_flash_cmd_read() except it also claims/releases the SPI
>>>>>>>   * bus. Used as common part of the ->read() operation.
>>>>>>> --
>>>>>>> 1.7.0.4
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> U-Boot mailing list
>>>>>>> U-Boot@lists.denx.de
>>>>>>> http://lists.denx.de/mailman/listinfo/u-boot
>>>>>>
>>>>>> Regards,
>>>>>> Simon
diff mbox

Patch

diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
index 800ed8b..a8f0af0 100644
--- a/drivers/mtd/spi/spi_flash.c
+++ b/drivers/mtd/spi/spi_flash.c
@@ -274,6 +274,33 @@  int spi_flash_cmd_write_status(struct spi_flash *flash, u8 sr)
 	return 0;
 }
 
+int spi_flash_cmd_write_config(struct spi_flash *flash, u16 cr)
+{
+	u8 cmd;
+	int ret;
+
+	ret = spi_flash_cmd_write_enable(flash);
+	if (ret < 0) {
+		debug("SF: enabling write failed\n");
+		return ret;
+	}
+
+	cmd = CMD_WRITE_STATUS;
+	ret = spi_flash_cmd_write(flash->spi, &cmd, 1, &cr, 2);
+	if (ret) {
+		debug("SF: fail to write config register\n");
+		return ret;
+	}
+
+	ret = spi_flash_cmd_wait_ready(flash, SPI_FLASH_PROG_TIMEOUT);
+	if (ret < 0) {
+		debug("SF: write config register timed out\n");
+		return ret;
+	}
+
+	return 0;
+}
+
 /*
  * The following table holds all device probe functions
  *
diff --git a/drivers/mtd/spi/spi_flash_internal.h b/drivers/mtd/spi/spi_flash_internal.h
index 141cfa8..9287778 100644
--- a/drivers/mtd/spi/spi_flash_internal.h
+++ b/drivers/mtd/spi/spi_flash_internal.h
@@ -77,6 +77,9 @@  static inline int spi_flash_cmd_write_disable(struct spi_flash *flash)
 /* Program the status register. */
 int spi_flash_cmd_write_status(struct spi_flash *flash, u8 sr);
 
+/* Program the config register. */
+int spi_flash_cmd_write_config(struct spi_flash *flash, u16 cr);
+
 /*
  * Same as spi_flash_cmd_read() except it also claims/releases the SPI
  * bus. Used as common part of the ->read() operation.