diff mbox

[U-Boot,v3] spi: ich: Configure SPI BIOS parameters

Message ID 20170209092655.10594-1-sr@denx.de
State Deferred
Delegated to: Bin Meng
Headers show

Commit Message

Stefan Roese Feb. 9, 2017, 9:26 a.m. UTC 
Without configuring these registers in the SPI controller, the Linux
MTD device driver is not able to correctly read/write to the SPI
NOR chip at all. In fact, the chip is not detected at all.

Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Bin Meng <bmeng.cn@gmail.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Jagan Teki <jteki@openedev.com>
---
v3:
- Moved defines from C-file into header file
- Changed some enum's into macros in ich.h for consistency
- Used these newly introduced defines

v2:
- Moved code into the ICH SPI driver as suggested by Simon and Bin

 drivers/spi/ich.c | 18 ++++++++++++++++++
 drivers/spi/ich.h | 45 ++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 56 insertions(+), 7 deletions(-)

Comments

Bin Meng Feb. 10, 2017, 5:45 a.m. UTC | #1 
Hi Stefan,

On Thu, Feb 9, 2017 at 5:26 PM, Stefan Roese <sr@denx.de> wrote:
> Without configuring these registers in the SPI controller, the Linux
> MTD device driver is not able to correctly read/write to the SPI
> NOR chip at all. In fact, the chip is not detected at all.
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Jagan Teki <jteki@openedev.com>
> ---
> v3:
> - Moved defines from C-file into header file
> - Changed some enum's into macros in ich.h for consistency
> - Used these newly introduced defines
>
> v2:
> - Moved code into the ICH SPI driver as suggested by Simon and Bin
>
>  drivers/spi/ich.c | 18 ++++++++++++++++++
>  drivers/spi/ich.h | 45 ++++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 56 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
> index caf0103dc3..0418455a79 100644
> --- a/drivers/spi/ich.c
> +++ b/drivers/spi/ich.c
> @@ -111,6 +111,17 @@ static int ich9_can_do_33mhz(struct udevice *dev)
>         return speed == 1;
>  }
>
> +/*
> + * Configure SPI controller so that the Linux MTD driver can fully
> + * access the SPI NOR chip
> + */
> +static void spi_controller_config(struct ich_spi_priv *ctlr)
> +{
> +       ich_writew(ctlr, SPI_OPTYPE, ctlr->optype);
> +       ich_writel(ctlr, SPI_OPMENU_LOWER, ctlr->opmenu);
> +       ich_writel(ctlr, SPI_OPMENU_UPPER, ctlr->opmenu + sizeof(u32));
> +}
> +
>  static int ich_init_controller(struct udevice *dev,
>                                struct ich_spi_platdata *plat,
>                                struct ich_spi_priv *ctlr)
> @@ -172,6 +183,13 @@ static int ich_init_controller(struct udevice *dev,
>
>         ich_set_bbar(ctlr, 0);
>
> +       /*
> +        * Configure the SPI-NOR controller in a way that the Linux
> +        * MTD SPI-NOR device driver has full read-write access to
> +        * the SPI-NOR chips
> +        */
> +       spi_controller_config(ctlr);
> +
>         return 0;
>  }
>
> diff --git a/drivers/spi/ich.h b/drivers/spi/ich.h
> index bd0a820809..7282181247 100644
> --- a/drivers/spi/ich.h
> +++ b/drivers/spi/ich.h
> @@ -102,13 +102,6 @@ enum {
>  };
>
>  enum {
> -       SPI_OPCODE_TYPE_READ_NO_ADDRESS =       0,
> -       SPI_OPCODE_TYPE_WRITE_NO_ADDRESS =      1,
> -       SPI_OPCODE_TYPE_READ_WITH_ADDRESS =     2,
> -       SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS =    3
> -};
> -
> -enum {
>         ICH_MAX_CMD_LEN         = 5,
>  };
>
> @@ -127,6 +120,44 @@ struct spi_trans {
>  #define SPI_OPCODE_WREN                0x06
>  #define SPI_OPCODE_FAST_READ   0x0b
>
> +#define SPI_OPCODE_TYPE_READ_NO_ADDRESS                0
> +#define SPI_OPCODE_TYPE_WRITE_NO_ADDRESS       1
> +#define SPI_OPCODE_TYPE_READ_WITH_ADDRESS      2
> +#define SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS     3
> +

Thanks for changing this.

> +#define SPI_OPMENU_0   0x01 /* WRSR: Write Status Register */
> +#define SPI_OPTYPE_0   SPI_OPCODE_TYPE_WRITE_NO_ADDRESS
> +
> +#define SPI_OPMENU_1   0x02 /* BYPR: Byte Program */
> +#define SPI_OPTYPE_1   SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS
> +
> +#define SPI_OPMENU_2   0x03 /* READ: Read Data */
> +#define SPI_OPTYPE_2   SPI_OPCODE_TYPE_READ_WITH_ADDRESS
> +
> +#define SPI_OPMENU_3   0x05 /* RDSR: Read Status Register */
> +#define SPI_OPTYPE_3   SPI_OPCODE_TYPE_READ_NO_ADDRESS
> +
> +#define SPI_OPMENU_4   0x20 /* SE20: Sector Erase 0x20 */
> +#define SPI_OPTYPE_4   SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS
> +
> +#define SPI_OPMENU_5   0x9f /* RDID: Read ID */
> +#define SPI_OPTYPE_5   SPI_OPCODE_TYPE_READ_NO_ADDRESS
> +
> +#define SPI_OPMENU_6   0xd8 /* BED8: Block Erase 0xd8 */
> +#define SPI_OPTYPE_6   SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS
> +
> +#define SPI_OPMENU_7   0x0b /* FAST: Fast Read */
> +#define SPI_OPTYPE_7   SPI_OPCODE_TYPE_READ_WITH_ADDRESS
> +

These are SPI flash commands. Should we name them similarly like
SPI_OPCODE_WREN and SPI_OPCODE_FAST_READ?

> +#define SPI_OPTYPE ((SPI_OPTYPE_7 << 14) | (SPI_OPTYPE_6 << 12) | \
> +                   (SPI_OPTYPE_5 << 10) | (SPI_OPTYPE_4 <<  8) | \
> +                   (SPI_OPTYPE_3 <<  6) | (SPI_OPTYPE_2 <<  4) | \
> +                   (SPI_OPTYPE_1 <<  2) | (SPI_OPTYPE_0 <<  0))
> +#define SPI_OPMENU_UPPER ((SPI_OPMENU_7 << 24) | (SPI_OPMENU_6 << 16) | \
> +                         (SPI_OPMENU_5 <<  8) | (SPI_OPMENU_4 <<  0))
> +#define SPI_OPMENU_LOWER ((SPI_OPMENU_3 << 24) | (SPI_OPMENU_2 << 16) | \
> +                         (SPI_OPMENU_1 <<  8) | (SPI_OPMENU_0 <<  0))
> +

What if the board mounts a flash with a different SPI flash command
set? Will this work?

>  enum ich_version {
>         ICHV_7,
>         ICHV_9,
> --

Regards,
Bin
Stefan Roese Feb. 10, 2017, 10:17 a.m. UTC | #2 
Hi Bin,

On 10.02.2017 06:45, Bin Meng wrote:

<snip>

>> @@ -127,6 +120,44 @@ struct spi_trans {
>>  #define SPI_OPCODE_WREN                0x06
>>  #define SPI_OPCODE_FAST_READ   0x0b
>>
>> +#define SPI_OPCODE_TYPE_READ_NO_ADDRESS                0
>> +#define SPI_OPCODE_TYPE_WRITE_NO_ADDRESS       1
>> +#define SPI_OPCODE_TYPE_READ_WITH_ADDRESS      2
>> +#define SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS     3
>> +
>
> Thanks for changing this.
>
>> +#define SPI_OPMENU_0   0x01 /* WRSR: Write Status Register */
>> +#define SPI_OPTYPE_0   SPI_OPCODE_TYPE_WRITE_NO_ADDRESS
>> +
>> +#define SPI_OPMENU_1   0x02 /* BYPR: Byte Program */
>> +#define SPI_OPTYPE_1   SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS
>> +
>> +#define SPI_OPMENU_2   0x03 /* READ: Read Data */
>> +#define SPI_OPTYPE_2   SPI_OPCODE_TYPE_READ_WITH_ADDRESS
>> +
>> +#define SPI_OPMENU_3   0x05 /* RDSR: Read Status Register */
>> +#define SPI_OPTYPE_3   SPI_OPCODE_TYPE_READ_NO_ADDRESS
>> +
>> +#define SPI_OPMENU_4   0x20 /* SE20: Sector Erase 0x20 */
>> +#define SPI_OPTYPE_4   SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS
>> +
>> +#define SPI_OPMENU_5   0x9f /* RDID: Read ID */
>> +#define SPI_OPTYPE_5   SPI_OPCODE_TYPE_READ_NO_ADDRESS
>> +
>> +#define SPI_OPMENU_6   0xd8 /* BED8: Block Erase 0xd8 */
>> +#define SPI_OPTYPE_6   SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS
>> +
>> +#define SPI_OPMENU_7   0x0b /* FAST: Fast Read */
>> +#define SPI_OPTYPE_7   SPI_OPCODE_TYPE_READ_WITH_ADDRESS
>> +
>
> These are SPI flash commands. Should we name them similarly like
> SPI_OPCODE_WREN and SPI_OPCODE_FAST_READ?

I've thought about this as well but neglected doing this (for now).
If you think its "better" that way, I'll send a v4 shortly. Just
let me know.

>> +#define SPI_OPTYPE ((SPI_OPTYPE_7 << 14) | (SPI_OPTYPE_6 << 12) | \
>> +                   (SPI_OPTYPE_5 << 10) | (SPI_OPTYPE_4 <<  8) | \
>> +                   (SPI_OPTYPE_3 <<  6) | (SPI_OPTYPE_2 <<  4) | \
>> +                   (SPI_OPTYPE_1 <<  2) | (SPI_OPTYPE_0 <<  0))
>> +#define SPI_OPMENU_UPPER ((SPI_OPMENU_7 << 24) | (SPI_OPMENU_6 << 16) | \
>> +                         (SPI_OPMENU_5 <<  8) | (SPI_OPMENU_4 <<  0))
>> +#define SPI_OPMENU_LOWER ((SPI_OPMENU_3 << 24) | (SPI_OPMENU_2 << 16) | \
>> +                         (SPI_OPMENU_1 <<  8) | (SPI_OPMENU_0 <<  0))
>> +
>
> What if the board mounts a flash with a different SPI flash command
> set? Will this work?

Frankly, I can't tell for sure but its very likely. As you might
have guessed, these defines are taken from coreboot where they are
usually board specific and written in the last stage before booting
into the OS (IIRC). It would definitely make sense to add a mechanism
to configure these BIOS parameters in a board-specific way. So that
boards can choose to (optionally) provide different values.

An easy way to do this, would be to make the newly created function
spi_controller_config() a wear default. This way, board can always
overwrite these default values (and / or do other stuff) in their
board specific code. This could also be added in a follow up patch
once this is needed though. I still have to see such a case of an
"incompatible" SPI flash on x86, and IIRC all values in coreboot
were identical.

Thanks,
Stefan
Bin Meng Feb. 13, 2017, 8:45 a.m. UTC | #3 
Hi Stefan,

On Fri, Feb 10, 2017 at 6:17 PM, Stefan Roese <sr@denx.de> wrote:
> Hi Bin,
>
> On 10.02.2017 06:45, Bin Meng wrote:
>
> <snip>
>
>
>>> @@ -127,6 +120,44 @@ struct spi_trans {
>>>  #define SPI_OPCODE_WREN                0x06
>>>  #define SPI_OPCODE_FAST_READ   0x0b
>>>
>>> +#define SPI_OPCODE_TYPE_READ_NO_ADDRESS                0
>>> +#define SPI_OPCODE_TYPE_WRITE_NO_ADDRESS       1
>>> +#define SPI_OPCODE_TYPE_READ_WITH_ADDRESS      2
>>> +#define SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS     3
>>> +
>>
>>
>> Thanks for changing this.
>>
>>> +#define SPI_OPMENU_0   0x01 /* WRSR: Write Status Register */
>>> +#define SPI_OPTYPE_0   SPI_OPCODE_TYPE_WRITE_NO_ADDRESS
>>> +
>>> +#define SPI_OPMENU_1   0x02 /* BYPR: Byte Program */
>>> +#define SPI_OPTYPE_1   SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS
>>> +
>>> +#define SPI_OPMENU_2   0x03 /* READ: Read Data */
>>> +#define SPI_OPTYPE_2   SPI_OPCODE_TYPE_READ_WITH_ADDRESS
>>> +
>>> +#define SPI_OPMENU_3   0x05 /* RDSR: Read Status Register */
>>> +#define SPI_OPTYPE_3   SPI_OPCODE_TYPE_READ_NO_ADDRESS
>>> +
>>> +#define SPI_OPMENU_4   0x20 /* SE20: Sector Erase 0x20 */
>>> +#define SPI_OPTYPE_4   SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS
>>> +
>>> +#define SPI_OPMENU_5   0x9f /* RDID: Read ID */
>>> +#define SPI_OPTYPE_5   SPI_OPCODE_TYPE_READ_NO_ADDRESS
>>> +
>>> +#define SPI_OPMENU_6   0xd8 /* BED8: Block Erase 0xd8 */
>>> +#define SPI_OPTYPE_6   SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS
>>> +
>>> +#define SPI_OPMENU_7   0x0b /* FAST: Fast Read */
>>> +#define SPI_OPTYPE_7   SPI_OPCODE_TYPE_READ_WITH_ADDRESS
>>> +
>>
>>
>> These are SPI flash commands. Should we name them similarly like
>> SPI_OPCODE_WREN and SPI_OPCODE_FAST_READ?
>
>
> I've thought about this as well but neglected doing this (for now).
> If you think its "better" that way, I'll send a v4 shortly. Just
> let me know.
>

OK, let's leave it for now.

>>> +#define SPI_OPTYPE ((SPI_OPTYPE_7 << 14) | (SPI_OPTYPE_6 << 12) | \
>>> +                   (SPI_OPTYPE_5 << 10) | (SPI_OPTYPE_4 <<  8) | \
>>> +                   (SPI_OPTYPE_3 <<  6) | (SPI_OPTYPE_2 <<  4) | \
>>> +                   (SPI_OPTYPE_1 <<  2) | (SPI_OPTYPE_0 <<  0))
>>> +#define SPI_OPMENU_UPPER ((SPI_OPMENU_7 << 24) | (SPI_OPMENU_6 << 16) |
>>> \
>>> +                         (SPI_OPMENU_5 <<  8) | (SPI_OPMENU_4 <<  0))
>>> +#define SPI_OPMENU_LOWER ((SPI_OPMENU_3 << 24) | (SPI_OPMENU_2 << 16) |
>>> \
>>> +                         (SPI_OPMENU_1 <<  8) | (SPI_OPMENU_0 <<  0))
>>> +
>>
>>
>> What if the board mounts a flash with a different SPI flash command
>> set? Will this work?
>
>
> Frankly, I can't tell for sure but its very likely. As you might
> have guessed, these defines are taken from coreboot where they are
> usually board specific and written in the last stage before booting
> into the OS (IIRC). It would definitely make sense to add a mechanism
> to configure these BIOS parameters in a board-specific way. So that
> boards can choose to (optionally) provide different values.
>
> An easy way to do this, would be to make the newly created function
> spi_controller_config() a wear default. This way, board can always
> overwrite these default values (and / or do other stuff) in their
> board specific code. This could also be added in a follow up patch
> once this is needed though. I still have to see such a case of an
> "incompatible" SPI flash on x86, and IIRC all values in coreboot
> were identical.

I am OK with the weak implementation, but I know Simon dislikes weak
:) Maybe he has a better idea.

Let's hear what others think.

Regards,
Bin
Simon Glass Feb. 22, 2017, 3:59 a.m. UTC | #4 
Hi,

On 13 February 2017 at 01:45, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Stefan,
>
> On Fri, Feb 10, 2017 at 6:17 PM, Stefan Roese <sr@denx.de> wrote:
>> Hi Bin,
>>
>> On 10.02.2017 06:45, Bin Meng wrote:
>>
>> <snip>
>>
>>
>>>> @@ -127,6 +120,44 @@ struct spi_trans {
>>>>  #define SPI_OPCODE_WREN                0x06
>>>>  #define SPI_OPCODE_FAST_READ   0x0b
>>>>
>>>> +#define SPI_OPCODE_TYPE_READ_NO_ADDRESS                0
>>>> +#define SPI_OPCODE_TYPE_WRITE_NO_ADDRESS       1
>>>> +#define SPI_OPCODE_TYPE_READ_WITH_ADDRESS      2
>>>> +#define SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS     3
>>>> +
>>>
>>>
>>> Thanks for changing this.
>>>
>>>> +#define SPI_OPMENU_0   0x01 /* WRSR: Write Status Register */
>>>> +#define SPI_OPTYPE_0   SPI_OPCODE_TYPE_WRITE_NO_ADDRESS
>>>> +
>>>> +#define SPI_OPMENU_1   0x02 /* BYPR: Byte Program */
>>>> +#define SPI_OPTYPE_1   SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS
>>>> +
>>>> +#define SPI_OPMENU_2   0x03 /* READ: Read Data */
>>>> +#define SPI_OPTYPE_2   SPI_OPCODE_TYPE_READ_WITH_ADDRESS
>>>> +
>>>> +#define SPI_OPMENU_3   0x05 /* RDSR: Read Status Register */
>>>> +#define SPI_OPTYPE_3   SPI_OPCODE_TYPE_READ_NO_ADDRESS
>>>> +
>>>> +#define SPI_OPMENU_4   0x20 /* SE20: Sector Erase 0x20 */
>>>> +#define SPI_OPTYPE_4   SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS
>>>> +
>>>> +#define SPI_OPMENU_5   0x9f /* RDID: Read ID */
>>>> +#define SPI_OPTYPE_5   SPI_OPCODE_TYPE_READ_NO_ADDRESS
>>>> +
>>>> +#define SPI_OPMENU_6   0xd8 /* BED8: Block Erase 0xd8 */
>>>> +#define SPI_OPTYPE_6   SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS
>>>> +
>>>> +#define SPI_OPMENU_7   0x0b /* FAST: Fast Read */
>>>> +#define SPI_OPTYPE_7   SPI_OPCODE_TYPE_READ_WITH_ADDRESS
>>>> +
>>>
>>>
>>> These are SPI flash commands. Should we name them similarly like
>>> SPI_OPCODE_WREN and SPI_OPCODE_FAST_READ?
>>
>>
>> I've thought about this as well but neglected doing this (for now).
>> If you think its "better" that way, I'll send a v4 shortly. Just
>> let me know.
>>
>
> OK, let's leave it for now.
>
>>>> +#define SPI_OPTYPE ((SPI_OPTYPE_7 << 14) | (SPI_OPTYPE_6 << 12) | \
>>>> +                   (SPI_OPTYPE_5 << 10) | (SPI_OPTYPE_4 <<  8) | \
>>>> +                   (SPI_OPTYPE_3 <<  6) | (SPI_OPTYPE_2 <<  4) | \
>>>> +                   (SPI_OPTYPE_1 <<  2) | (SPI_OPTYPE_0 <<  0))
>>>> +#define SPI_OPMENU_UPPER ((SPI_OPMENU_7 << 24) | (SPI_OPMENU_6 << 16) |
>>>> \
>>>> +                         (SPI_OPMENU_5 <<  8) | (SPI_OPMENU_4 <<  0))
>>>> +#define SPI_OPMENU_LOWER ((SPI_OPMENU_3 << 24) | (SPI_OPMENU_2 << 16) |
>>>> \
>>>> +                         (SPI_OPMENU_1 <<  8) | (SPI_OPMENU_0 <<  0))
>>>> +
>>>
>>>
>>> What if the board mounts a flash with a different SPI flash command
>>> set? Will this work?
>>
>>
>> Frankly, I can't tell for sure but its very likely. As you might
>> have guessed, these defines are taken from coreboot where they are
>> usually board specific and written in the last stage before booting
>> into the OS (IIRC). It would definitely make sense to add a mechanism
>> to configure these BIOS parameters in a board-specific way. So that
>> boards can choose to (optionally) provide different values.
>>
>> An easy way to do this, would be to make the newly created function
>> spi_controller_config() a wear default. This way, board can always
>> overwrite these default values (and / or do other stuff) in their
>> board specific code. This could also be added in a follow up patch
>> once this is needed though. I still have to see such a case of an
>> "incompatible" SPI flash on x86, and IIRC all values in coreboot
>> were identical.
>
> I am OK with the weak implementation, but I know Simon dislikes weak
> :) Maybe he has a better idea.
>
> Let's hear what others think.

Well how about crossing that bridge when we come to it? For now,
perhaps this is good enough. You are right, I'm not keen on weak
functions as I see them as ad-hoc APIs. Better, I believe, to think
about it and define a real API. For example in this case I wonder if
the remove() method of the SPI driver could do something? Or perhaps
add another method like finalise()?

Regards,
Simon
Stefan Roese Feb. 22, 2017, 7:07 a.m. UTC | #5 
Hi Simon,

On 22.02.2017 04:59, Simon Glass wrote:

<snip>

>>>> What if the board mounts a flash with a different SPI flash command
>>>> set? Will this work?
>>>
>>>
>>> Frankly, I can't tell for sure but its very likely. As you might
>>> have guessed, these defines are taken from coreboot where they are
>>> usually board specific and written in the last stage before booting
>>> into the OS (IIRC). It would definitely make sense to add a mechanism
>>> to configure these BIOS parameters in a board-specific way. So that
>>> boards can choose to (optionally) provide different values.
>>>
>>> An easy way to do this, would be to make the newly created function
>>> spi_controller_config() a wear default. This way, board can always
>>> overwrite these default values (and / or do other stuff) in their
>>> board specific code. This could also be added in a follow up patch
>>> once this is needed though. I still have to see such a case of an
>>> "incompatible" SPI flash on x86, and IIRC all values in coreboot
>>> were identical.
>>
>> I am OK with the weak implementation, but I know Simon dislikes weak
>> :) Maybe he has a better idea.
>>
>> Let's hear what others think.
>
> Well how about crossing that bridge when we come to it? For now,
> perhaps this is good enough.

Thanks.

> You are right, I'm not keen on weak
> functions as I see them as ad-hoc APIs. Better, I believe, to think
> about it and define a real API. For example in this case I wonder if
> the remove() method of the SPI driver could do something? Or perhaps
> add another method like finalise()?

Okay, lets think about this in more detail, once its necessary.

Thanks,
Stefan
Bin Meng April 1, 2017, 2:02 a.m. UTC | #6 
Hi Stefan, Simon,

On Wed, Feb 22, 2017 at 3:07 PM, Stefan Roese <sr@denx.de> wrote:
> Hi Simon,
>
> On 22.02.2017 04:59, Simon Glass wrote:
>
> <snip>
>
>>>>> What if the board mounts a flash with a different SPI flash command
>>>>> set? Will this work?
>>>>
>>>>
>>>>
>>>> Frankly, I can't tell for sure but its very likely. As you might
>>>> have guessed, these defines are taken from coreboot where they are
>>>> usually board specific and written in the last stage before booting
>>>> into the OS (IIRC). It would definitely make sense to add a mechanism
>>>> to configure these BIOS parameters in a board-specific way. So that
>>>> boards can choose to (optionally) provide different values.
>>>>
>>>> An easy way to do this, would be to make the newly created function
>>>> spi_controller_config() a wear default. This way, board can always
>>>> overwrite these default values (and / or do other stuff) in their
>>>> board specific code. This could also be added in a follow up patch
>>>> once this is needed though. I still have to see such a case of an
>>>> "incompatible" SPI flash on x86, and IIRC all values in coreboot
>>>> were identical.
>>>
>>>
>>> I am OK with the weak implementation, but I know Simon dislikes weak
>>> :) Maybe he has a better idea.
>>>
>>> Let's hear what others think.
>>
>>
>> Well how about crossing that bridge when we come to it? For now,
>> perhaps this is good enough.
>
>
> Thanks.
>
>> You are right, I'm not keen on weak
>> functions as I see them as ad-hoc APIs. Better, I believe, to think
>> about it and define a real API. For example in this case I wonder if
>> the remove() method of the SPI driver could do something? Or perhaps
>> add another method like finalise()?
>
>
> Okay, lets think about this in more detail, once its necessary.
>

What's next step for this patch? I see Stefan has done some patches on dm core.

Regards,
Bin
Stefan Roese April 1, 2017, 8:11 a.m. UTC | #7 
Hi Bin,

On 01.04.2017 04:02, Bin Meng wrote:
>>>>>> What if the board mounts a flash with a different SPI flash command
>>>>>> set? Will this work?
>>>>>
>>>>>
>>>>>
>>>>> Frankly, I can't tell for sure but its very likely. As you might
>>>>> have guessed, these defines are taken from coreboot where they are
>>>>> usually board specific and written in the last stage before booting
>>>>> into the OS (IIRC). It would definitely make sense to add a mechanism
>>>>> to configure these BIOS parameters in a board-specific way. So that
>>>>> boards can choose to (optionally) provide different values.
>>>>>
>>>>> An easy way to do this, would be to make the newly created function
>>>>> spi_controller_config() a wear default. This way, board can always
>>>>> overwrite these default values (and / or do other stuff) in their
>>>>> board specific code. This could also be added in a follow up patch
>>>>> once this is needed though. I still have to see such a case of an
>>>>> "incompatible" SPI flash on x86, and IIRC all values in coreboot
>>>>> were identical.
>>>>
>>>>
>>>> I am OK with the weak implementation, but I know Simon dislikes weak
>>>> :) Maybe he has a better idea.
>>>>
>>>> Let's hear what others think.
>>>
>>>
>>> Well how about crossing that bridge when we come to it? For now,
>>> perhaps this is good enough.
>>
>>
>> Thanks.
>>
>>> You are right, I'm not keen on weak
>>> functions as I see them as ad-hoc APIs. Better, I believe, to think
>>> about it and define a real API. For example in this case I wonder if
>>> the remove() method of the SPI driver could do something? Or perhaps
>>> add another method like finalise()?
>>
>>
>> Okay, lets think about this in more detail, once its necessary.
>>
>
> What's next step for this patch? I see Stefan has done some patches
> on dm core.

Correct, and this one would fit perfectly into this new DM removal /
finalization framework. Additionally, I've just noticed yesterday,
that this SPI patch is enough for the Linux Intel SPI NOR driver
to correctly detect the NOR chip. But writing is not possible, since
the SPI NOR seem to be protected. I still need to investigate here
to fix this.

So I suggest to postpone this SPI patch until I have a newer, better
version, perhaps already based on this DM remove framework.

Thanks,
Stefan
diff mbox

Patch

diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
index caf0103dc3..0418455a79 100644
--- a/drivers/spi/ich.c
+++ b/drivers/spi/ich.c
@@ -111,6 +111,17 @@  static int ich9_can_do_33mhz(struct udevice *dev)
 	return speed == 1;
 }
 
+/*
+ * Configure SPI controller so that the Linux MTD driver can fully
+ * access the SPI NOR chip
+ */
+static void spi_controller_config(struct ich_spi_priv *ctlr)
+{
+	ich_writew(ctlr, SPI_OPTYPE, ctlr->optype);
+	ich_writel(ctlr, SPI_OPMENU_LOWER, ctlr->opmenu);
+	ich_writel(ctlr, SPI_OPMENU_UPPER, ctlr->opmenu + sizeof(u32));
+}
+
 static int ich_init_controller(struct udevice *dev,
 			       struct ich_spi_platdata *plat,
 			       struct ich_spi_priv *ctlr)
@@ -172,6 +183,13 @@  static int ich_init_controller(struct udevice *dev,
 
 	ich_set_bbar(ctlr, 0);
 
+	/*
+	 * Configure the SPI-NOR controller in a way that the Linux
+	 * MTD SPI-NOR device driver has full read-write access to
+	 * the SPI-NOR chips
+	 */
+	spi_controller_config(ctlr);
+
 	return 0;
 }
 
diff --git a/drivers/spi/ich.h b/drivers/spi/ich.h
index bd0a820809..7282181247 100644
--- a/drivers/spi/ich.h
+++ b/drivers/spi/ich.h
@@ -102,13 +102,6 @@  enum {
 };
 
 enum {
-	SPI_OPCODE_TYPE_READ_NO_ADDRESS =	0,
-	SPI_OPCODE_TYPE_WRITE_NO_ADDRESS =	1,
-	SPI_OPCODE_TYPE_READ_WITH_ADDRESS =	2,
-	SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS =	3
-};
-
-enum {
 	ICH_MAX_CMD_LEN		= 5,
 };
 
@@ -127,6 +120,44 @@  struct spi_trans {
 #define SPI_OPCODE_WREN		0x06
 #define SPI_OPCODE_FAST_READ	0x0b
 
+#define SPI_OPCODE_TYPE_READ_NO_ADDRESS		0
+#define SPI_OPCODE_TYPE_WRITE_NO_ADDRESS	1
+#define SPI_OPCODE_TYPE_READ_WITH_ADDRESS	2
+#define SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS	3
+
+#define SPI_OPMENU_0	0x01 /* WRSR: Write Status Register */
+#define SPI_OPTYPE_0	SPI_OPCODE_TYPE_WRITE_NO_ADDRESS
+
+#define SPI_OPMENU_1	0x02 /* BYPR: Byte Program */
+#define SPI_OPTYPE_1	SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS
+
+#define SPI_OPMENU_2	0x03 /* READ: Read Data */
+#define SPI_OPTYPE_2	SPI_OPCODE_TYPE_READ_WITH_ADDRESS
+
+#define SPI_OPMENU_3	0x05 /* RDSR: Read Status Register */
+#define SPI_OPTYPE_3	SPI_OPCODE_TYPE_READ_NO_ADDRESS
+
+#define SPI_OPMENU_4	0x20 /* SE20: Sector Erase 0x20 */
+#define SPI_OPTYPE_4	SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS
+
+#define SPI_OPMENU_5	0x9f /* RDID: Read ID */
+#define SPI_OPTYPE_5	SPI_OPCODE_TYPE_READ_NO_ADDRESS
+
+#define SPI_OPMENU_6	0xd8 /* BED8: Block Erase 0xd8 */
+#define SPI_OPTYPE_6	SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS
+
+#define SPI_OPMENU_7	0x0b /* FAST: Fast Read */
+#define SPI_OPTYPE_7	SPI_OPCODE_TYPE_READ_WITH_ADDRESS
+
+#define SPI_OPTYPE ((SPI_OPTYPE_7 << 14) | (SPI_OPTYPE_6 << 12) | \
+		    (SPI_OPTYPE_5 << 10) | (SPI_OPTYPE_4 <<  8) | \
+		    (SPI_OPTYPE_3 <<  6) | (SPI_OPTYPE_2 <<  4) | \
+		    (SPI_OPTYPE_1 <<  2) | (SPI_OPTYPE_0 <<  0))
+#define SPI_OPMENU_UPPER ((SPI_OPMENU_7 << 24) | (SPI_OPMENU_6 << 16) | \
+			  (SPI_OPMENU_5 <<  8) | (SPI_OPMENU_4 <<  0))
+#define SPI_OPMENU_LOWER ((SPI_OPMENU_3 << 24) | (SPI_OPMENU_2 << 16) | \
+			  (SPI_OPMENU_1 <<  8) | (SPI_OPMENU_0 <<  0))
+
 enum ich_version {
 	ICHV_7,
 	ICHV_9,