[U-Boot] [PATCH] PATI: fix broken SPI access

[U-Boot] [PATCH] PATI: fix broken SPI access

[David Müller]

fix broken SPI access by adding/activating BOARD_EARLY_INIT_F
functionality and calling spi_init_f() from there.

Signed-off-by: David M?ller <d.mueller@elsoft.ch>
---
 board/mpl/pati/pati.c  | 5 +++++
 include/configs/PATI.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/board/mpl/pati/pati.c b/board/mpl/pati/pati.c
index 5d701a7..b9d88ee 100644
--- a/board/mpl/pati/pati.c
+++ b/board/mpl/pati/pati.c
@@ -311,6 +311,11 @@ void user_led1(int led_on)
 	sysconf->sc_sgpiodt2=reg; /* Data register */
 }
 
+int board_early_init_f(void)
+{
+	spi_init_f();
+	return 0;
+}
 
 /****************************************************************
  * Last Stage Init
diff --git a/include/configs/PATI.h b/include/configs/PATI.h
index 65ab65d..3ca204e 100644
--- a/include/configs/PATI.h
+++ b/include/configs/PATI.h
@@ -98,6 +98,7 @@
 
 #define CONFIG_SYS_BAUDRATE_TABLE	{ 9600, 19200, 38400, 57600, 115200, 1250000 }
 
+#define CONFIG_BOARD_EARLY_INIT_F
 
 /***********************************************************************
  * Last Stage Init
-- 
1.8.4

[U-Boot] [PATCH] PATI: fix broken SPI access

[Jagan Teki]

On 30 September 2014 16:53, David M?ller <d.mueller@elsoft.ch> wrote:
> fix broken SPI access by adding/activating BOARD_EARLY_INIT_F
> functionality and calling spi_init_f() from there.
>
> Signed-off-by: David M?ller <d.mueller@elsoft.ch>
> ---
>  board/mpl/pati/pati.c  | 5 +++++
>  include/configs/PATI.h | 1 +
>  2 files changed, 6 insertions(+)
>
> diff --git a/board/mpl/pati/pati.c b/board/mpl/pati/pati.c
> index 5d701a7..b9d88ee 100644
> --- a/board/mpl/pati/pati.c
> +++ b/board/mpl/pati/pati.c
> @@ -311,6 +311,11 @@ void user_led1(int led_on)
>         sysconf->sc_sgpiodt2=reg; /* Data register */
>  }
>
> +int board_early_init_f(void)
> +{
> +       spi_init_f();

Why you need to do this, spi_init_f is trying to call from
arch/powerpc/lib/board.c
any specific reason, I couldn't understand the fix you mentioned on
the commit body.

> +       return 0;
> +}
>
>  /****************************************************************
>   * Last Stage Init
> diff --git a/include/configs/PATI.h b/include/configs/PATI.h
> index 65ab65d..3ca204e 100644
> --- a/include/configs/PATI.h
> +++ b/include/configs/PATI.h
> @@ -98,6 +98,7 @@
>
>  #define CONFIG_SYS_BAUDRATE_TABLE      { 9600, 19200, 38400, 57600, 115200, 1250000 }
>
> +#define CONFIG_BOARD_EARLY_INIT_F
>
>  /***********************************************************************
>   * Last Stage Init
> --

thanks!
-- 
Jagan.

[U-Boot] [PATCH] PATI: fix broken SPI access

[David Müller (ELSOFT AG)]

Jagan Teki wrote:
> On 30 September 2014 16:53, David M?ller <d.mueller@elsoft.ch> wrote:
>> +int board_early_init_f(void)
>> +{
>> +       spi_init_f();
> 
> Why you need to do this, spi_init_f is trying to call from
> arch/powerpc/lib/board.c
> any specific reason, I couldn't understand the fix you mentioned on
> the commit body.

There is an EEPROM attached to the SPI channel containing vital board
data. Calling spi_init_f() from arch/powerpc/lib/board.c will be too late.

[U-Boot] [PATCH] PATI: fix broken SPI access

[Jagan Teki]

On 30 September 2014 18:41, David M?ller (ELSOFT AG)
<d.mueller@elsoft.ch> wrote:
> Jagan Teki wrote:
>> On 30 September 2014 16:53, David M?ller <d.mueller@elsoft.ch> wrote:
>>> +int board_early_init_f(void)
>>> +{
>>> +       spi_init_f();
>>
>> Why you need to do this, spi_init_f is trying to call from
>> arch/powerpc/lib/board.c
>> any specific reason, I couldn't understand the fix you mentioned on
>> the commit body.
>
> There is an EEPROM attached to the SPI channel containing vital board
> data. Calling spi_init_f() from arch/powerpc/lib/board.c will be too late.

Sorry, this looks an other issue - but anyway we're trying to remove
spi_init* stuff
from drivers/spi/* in future and I don't think it's a good idea to use that.

thanks!
-- 
Jagan.

[U-Boot] [PATCH] PATI: fix broken SPI access

[Tom Rini]

On Tue, Sep 30, 2014 at 08:06:43PM +0530, Jagan Teki wrote:
> On 30 September 2014 18:41, David M?ller (ELSOFT AG)
> <d.mueller@elsoft.ch> wrote:
> > Jagan Teki wrote:
> >> On 30 September 2014 16:53, David M?ller <d.mueller@elsoft.ch> wrote:
> >>> +int board_early_init_f(void)
> >>> +{
> >>> +       spi_init_f();
> >>
> >> Why you need to do this, spi_init_f is trying to call from
> >> arch/powerpc/lib/board.c
> >> any specific reason, I couldn't understand the fix you mentioned on
> >> the commit body.
> >
> > There is an EEPROM attached to the SPI channel containing vital board
> > data. Calling spi_init_f() from arch/powerpc/lib/board.c will be too late.
> 
> Sorry, this looks an other issue - but anyway we're trying to remove
> spi_init* stuff
> from drivers/spi/* in future and I don't think it's a good idea to use that.

It's also not a good idea to say that we'll leave a board broken until
something better comes along.  There should be a comment added to the
code here making it clear _why_ we need this done early.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140930/28549947/attachment.pgp>

[U-Boot] [PATCH] PATI: fix broken SPI access

[David Müller (ELSOFT AG)]

Jagan Teki wrote:
> On 30 September 2014 18:41, David M?ller (ELSOFT AG)
> <d.mueller@elsoft.ch> wrote:
>> Jagan Teki wrote:
>>> On 30 September 2014 16:53, David M?ller <d.mueller@elsoft.ch> wrote:
>>>> +int board_early_init_f(void)
>>>> +{
>>>> +       spi_init_f();
>>>
>>> Why you need to do this, spi_init_f is trying to call from
>>> arch/powerpc/lib/board.c
>>> any specific reason, I couldn't understand the fix you mentioned on
>>> the commit body.
>>
>> There is an EEPROM attached to the SPI channel containing vital board
>> data. Calling spi_init_f() from arch/powerpc/lib/board.c will be too late.
> 
> Sorry, this looks an other issue - but anyway we're trying to remove

What kind of "other issue" do you mean?

> spi_init* stuff
> from drivers/spi/* in future and I don't think it's a good idea to use that.

In this case what is the proposed way of initializing the SPI subsystem?

[U-Boot] [PATCH] PATI: fix broken SPI access

[Jagan Teki]

On 30 September 2014 20:28, David M?ller (ELSOFT AG)
<d.mueller@elsoft.ch> wrote:
> Jagan Teki wrote:
>> On 30 September 2014 18:41, David M?ller (ELSOFT AG)
>> <d.mueller@elsoft.ch> wrote:
>>> Jagan Teki wrote:
>>>> On 30 September 2014 16:53, David M?ller <d.mueller@elsoft.ch> wrote:
>>>>> +int board_early_init_f(void)
>>>>> +{
>>>>> +       spi_init_f();
>>>>
>>>> Why you need to do this, spi_init_f is trying to call from
>>>> arch/powerpc/lib/board.c
>>>> any specific reason, I couldn't understand the fix you mentioned on
>>>> the commit body.
>>>
>>> There is an EEPROM attached to the SPI channel containing vital board
>>> data. Calling spi_init_f() from arch/powerpc/lib/board.c will be too late.
>>
>> Sorry, this looks an other issue - but anyway we're trying to remove
>
> What kind of "other issue" do you mean?
>
>> spi_init* stuff
>> from drivers/spi/* in future and I don't think it's a good idea to use that.
>
> In this case what is the proposed way of initializing the SPI subsystem?
>

Usually spi got initialized through setup_slave() from the cmd
interface which is of
dynamic probe ie the reason most of drivers have spi_init() with no
functionality code.

On the other hand few are using spi_init() or spi_init_f() from arch
code or from
any other kind to initialize the spi.

Finally spi_init_f() from your case should use from arch(at least) as
this call is more generic
and it shouldn't be a call from board.

And more over we're working on dm-spi where spi_init() calls stuff
would disappear in future.

thanks!
-- 
Jagan.

[U-Boot] PATI: fix broken SPI access

[Tom Rini]

On Tue, Sep 30, 2014 at 01:23:54PM +0200, David M?ller (ELSOFT AG) wrote:

> fix broken SPI access by adding/activating BOARD_EARLY_INIT_F
> functionality and calling spi_init_f() from there.
> 
> Signed-off-by: David M?ller <d.mueller@elsoft.ch>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20141010/02b55947/attachment.pgp>

[U-Boot] [PATCH] PATI: fix broken SPI access

[Tom Rini]

On Tue, Sep 30, 2014 at 10:48:11AM -0400, Tom Rini wrote:
> On Tue, Sep 30, 2014 at 08:06:43PM +0530, Jagan Teki wrote:
> > On 30 September 2014 18:41, David M?ller (ELSOFT AG)
> > <d.mueller@elsoft.ch> wrote:
> > > Jagan Teki wrote:
> > >> On 30 September 2014 16:53, David M?ller <d.mueller@elsoft.ch> wrote:
> > >>> +int board_early_init_f(void)
> > >>> +{
> > >>> +       spi_init_f();
> > >>
> > >> Why you need to do this, spi_init_f is trying to call from
> > >> arch/powerpc/lib/board.c
> > >> any specific reason, I couldn't understand the fix you mentioned on
> > >> the commit body.
> > >
> > > There is an EEPROM attached to the SPI channel containing vital board
> > > data. Calling spi_init_f() from arch/powerpc/lib/board.c will be too late.
> > 
> > Sorry, this looks an other issue - but anyway we're trying to remove
> > spi_init* stuff
> > from drivers/spi/* in future and I don't think it's a good idea to use that.
> 
> It's also not a good idea to say that we'll leave a board broken until
> something better comes along.  There should be a comment added to the
> code here making it clear _why_ we need this done early.

And again, for the record, I wanted to _fix_ things today so we can
clean them up tomorrow, rather than keep something broken so we can fix
it later.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20141010/cb2a5b66/attachment.pgp>

[U-Boot] [PATCH] PATI: fix broken SPI access

[Jagan Teki]

On 10 October 2014 21:44, Tom Rini <trini@ti.com> wrote:
> On Tue, Sep 30, 2014 at 10:48:11AM -0400, Tom Rini wrote:
>> On Tue, Sep 30, 2014 at 08:06:43PM +0530, Jagan Teki wrote:
>> > On 30 September 2014 18:41, David M?ller (ELSOFT AG)
>> > <d.mueller@elsoft.ch> wrote:
>> > > Jagan Teki wrote:
>> > >> On 30 September 2014 16:53, David M?ller <d.mueller@elsoft.ch> wrote:
>> > >>> +int board_early_init_f(void)
>> > >>> +{
>> > >>> +       spi_init_f();
>> > >>
>> > >> Why you need to do this, spi_init_f is trying to call from
>> > >> arch/powerpc/lib/board.c
>> > >> any specific reason, I couldn't understand the fix you mentioned on
>> > >> the commit body.
>> > >
>> > > There is an EEPROM attached to the SPI channel containing vital board
>> > > data. Calling spi_init_f() from arch/powerpc/lib/board.c will be too late.
>> >
>> > Sorry, this looks an other issue - but anyway we're trying to remove
>> > spi_init* stuff
>> > from drivers/spi/* in future and I don't think it's a good idea to use that.
>>
>> It's also not a good idea to say that we'll leave a board broken until
>> something better comes along.  There should be a comment added to the
>> code here making it clear _why_ we need this done early.
>
> And again, for the record, I wanted to _fix_ things today so we can
> clean them up tomorrow, rather than keep something broken so we can fix
> it later.

OK, got your point.

thanks!
-- 
Jagan.