| Message ID | 1412076234-2946-1-git-send-email-d.mueller@elsoft.ch |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
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 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.
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!
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.
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?
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!
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!
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.
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!
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
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(+)