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

[Simon Glass]

Hi Jagan,

On Thu, Dec 13, 2012 at 8:08 AM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
> Hi Simon,
>
> On Thu, Dec 13, 2012 at 4:03 AM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Jagan,
>>
>> On Wed, Dec 12, 2012 at 8:21 AM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
>>> Hi Simon,
>>>
>>> On Wed, Dec 12, 2012 at 8:53 PM, Simon Glass <sjg at chromium.org> wrote:
>>>> Hi Jagan,
>>>>
>>>> On Wed, Dec 12, 2012 at 7:20 AM, Jagan Teki <jagannadh.teki at gmail.com> wrote:
>>>>> Hi Simon,
>>>>>
>>>>> On Wed, Dec 12, 2012 at 12:05 PM, Simon Glass <sjg at chromium.org> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On Mon, Dec 10, 2012 at 6:42 AM, Jagannadha Sutradharudu Teki
>>>>>> <jagannadh.teki at 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 at 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 at lists.denx.de
>>>>>>> http://lists.denx.de/mailman/listinfo/u-boot
>>>>>>
>>>>>> Regards,
>>>>>> Simon