| Message ID | 1460043146-27918-1-git-send-email-vigneshr@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
On Thu, Apr 07, 2016 at 09:02:26PM +0530, Vignesh R wrote: > In case of DT boot, don't read default speed and mode for SPI from > CONFIG_*, instead read from DT node. This will make sure that boards > with multiple SPI/QSPI controllers can be probed at different > bus frequencies and modes. > > Signed-off-by: Vignesh R <vigneshr@ti.com> > --- > cmd/sf.c | 2 ++ > drivers/spi/spi-uclass.c | 8 ++++++-- > 2 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/cmd/sf.c b/cmd/sf.c > index 42862d9d921a..4c40d5104422 100644 > --- a/cmd/sf.c > +++ b/cmd/sf.c > @@ -88,6 +88,8 @@ static int do_spi_flash_probe(int argc, char * const argv[]) > #ifdef CONFIG_DM_SPI_FLASH > struct udevice *new, *bus_dev; > int ret; > + /* In DM mode defaults wiil be taken from DT */ > + speed = -1, mode = -1; > #else > struct spi_flash *new; > #endif > diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c > index 5561f36762f9..55ebb5ef19b7 100644 > --- a/drivers/spi/spi-uclass.c > +++ b/drivers/spi/spi-uclass.c > @@ -264,6 +264,7 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode, > struct udevice **busp, struct spi_slave **devp) > { > struct udevice *bus, *dev; > + struct dm_spi_slave_platdata *plat; > bool created = false; > int ret; > > @@ -280,8 +281,6 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode, > * SPI flash chip - we will bind to the correct driver. > */ > if (ret == -ENODEV && drv_name) { > - struct dm_spi_slave_platdata *plat; > - > debug("%s: Binding new device '%s', busnum=%d, cs=%d, driver=%s\n", > __func__, dev_name, busnum, cs, drv_name); > ret = device_bind_driver(bus, drv_name, dev_name, &dev); > @@ -308,6 +307,11 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode, > slave->dev = dev; > } > > + plat = dev_get_parent_platdata(dev); > + if (speed == -1) > + speed = plat->max_hz; > + if (mode == -1) > + mode = plat->mode; > ret = spi_set_speed_mode(bus, speed, mode); > if (ret) > goto err; I like the concept but if it can fail, passing -1 for speed/mode seems like a bad idea and we should just fail.
On 04/09/2016 01:15 AM, Tom Rini wrote: > On Thu, Apr 07, 2016 at 09:02:26PM +0530, Vignesh R wrote: > >> In case of DT boot, don't read default speed and mode for SPI from >> CONFIG_*, instead read from DT node. This will make sure that boards >> with multiple SPI/QSPI controllers can be probed at different >> bus frequencies and modes. >> >> Signed-off-by: Vignesh R <vigneshr@ti.com> >> --- >> cmd/sf.c | 2 ++ >> drivers/spi/spi-uclass.c | 8 ++++++-- >> 2 files changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/cmd/sf.c b/cmd/sf.c >> index 42862d9d921a..4c40d5104422 100644 >> --- a/cmd/sf.c >> +++ b/cmd/sf.c >> @@ -88,6 +88,8 @@ static int do_spi_flash_probe(int argc, char * const argv[]) >> #ifdef CONFIG_DM_SPI_FLASH >> struct udevice *new, *bus_dev; >> int ret; >> + /* In DM mode defaults wiil be taken from DT */ >> + speed = -1, mode = -1; >> #else >> struct spi_flash *new; >> #endif >> diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c >> index 5561f36762f9..55ebb5ef19b7 100644 >> --- a/drivers/spi/spi-uclass.c >> +++ b/drivers/spi/spi-uclass.c >> @@ -264,6 +264,7 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode, >> struct udevice **busp, struct spi_slave **devp) >> { >> struct udevice *bus, *dev; >> + struct dm_spi_slave_platdata *plat; >> bool created = false; >> int ret; >> >> @@ -280,8 +281,6 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode, >> * SPI flash chip - we will bind to the correct driver. >> */ >> if (ret == -ENODEV && drv_name) { >> - struct dm_spi_slave_platdata *plat; >> - >> debug("%s: Binding new device '%s', busnum=%d, cs=%d, driver=%s\n", >> __func__, dev_name, busnum, cs, drv_name); >> ret = device_bind_driver(bus, drv_name, dev_name, &dev); >> @@ -308,6 +307,11 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode, >> slave->dev = dev; >> } >> >> + plat = dev_get_parent_platdata(dev); >> + if (speed == -1) >> + speed = plat->max_hz; >> + if (mode == -1) >> + mode = plat->mode; >> ret = spi_set_speed_mode(bus, speed, mode); >> if (ret) >> goto err; > > I like the concept but if it can fail, passing -1 for speed/mode seems > like a bad idea and we should just fail. > I didn't quite understand your comment. Could you explain what can fail? I don't think mode/speed will be -1 when calling spi_set_speed_mode() after above change. speed/mode will either be command line arg passed to sf probe cmd or CONFIG_SF_DEFAULT_* in case of non DM driver or mode/speed specified in DT node in case of DM.
On Monday 11 April 2016 10:12 AM, Vignesh R wrote: > > > On 04/09/2016 01:15 AM, Tom Rini wrote: >> On Thu, Apr 07, 2016 at 09:02:26PM +0530, Vignesh R wrote: >> >>> In case of DT boot, don't read default speed and mode for SPI from >>> CONFIG_*, instead read from DT node. This will make sure that boards >>> with multiple SPI/QSPI controllers can be probed at different >>> bus frequencies and modes. >>> >>> Signed-off-by: Vignesh R <vigneshr@ti.com> >>> --- >>> cmd/sf.c | 2 ++ >>> drivers/spi/spi-uclass.c | 8 ++++++-- >>> 2 files changed, 8 insertions(+), 2 deletions(-) >>> >>> diff --git a/cmd/sf.c b/cmd/sf.c >>> index 42862d9d921a..4c40d5104422 100644 >>> --- a/cmd/sf.c >>> +++ b/cmd/sf.c >>> @@ -88,6 +88,8 @@ static int do_spi_flash_probe(int argc, char * const argv[]) >>> #ifdef CONFIG_DM_SPI_FLASH >>> struct udevice *new, *bus_dev; >>> int ret; >>> + /* In DM mode defaults wiil be taken from DT */ >>> + speed = -1, mode = -1; speed and mode are uint and it is assigned with -1 which is not making sense. Assigning 0 will be a better option. Regards Mugunthan V N >>> #else >>> struct spi_flash *new; >>> #endif >>> diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c >>> index 5561f36762f9..55ebb5ef19b7 100644 >>> --- a/drivers/spi/spi-uclass.c >>> +++ b/drivers/spi/spi-uclass.c >>> @@ -264,6 +264,7 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode, >>> struct udevice **busp, struct spi_slave **devp) >>> { >>> struct udevice *bus, *dev; >>> + struct dm_spi_slave_platdata *plat; >>> bool created = false; >>> int ret; >>> >>> @@ -280,8 +281,6 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode, >>> * SPI flash chip - we will bind to the correct driver. >>> */ >>> if (ret == -ENODEV && drv_name) { >>> - struct dm_spi_slave_platdata *plat; >>> - >>> debug("%s: Binding new device '%s', busnum=%d, cs=%d, driver=%s\n", >>> __func__, dev_name, busnum, cs, drv_name); >>> ret = device_bind_driver(bus, drv_name, dev_name, &dev); >>> @@ -308,6 +307,11 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode, >>> slave->dev = dev; >>> } >>> >>> + plat = dev_get_parent_platdata(dev); >>> + if (speed == -1) >>> + speed = plat->max_hz; >>> + if (mode == -1) >>> + mode = plat->mode; >>> ret = spi_set_speed_mode(bus, speed, mode); >>> if (ret) >>> goto err; >> >> I like the concept but if it can fail, passing -1 for speed/mode seems >> like a bad idea and we should just fail. >> > I didn't quite understand your comment. Could you explain what can fail? > I don't think mode/speed will be -1 when calling spi_set_speed_mode() > after above change. speed/mode will either be command line arg passed to > sf probe cmd or CONFIG_SF_DEFAULT_* in case of non DM driver or > mode/speed specified in DT node in case of DM. > > >
On 04/11/2016 11:18 AM, Mugunthan V N wrote: > On Monday 11 April 2016 10:12 AM, Vignesh R wrote: >> >> >> On 04/09/2016 01:15 AM, Tom Rini wrote: >>> On Thu, Apr 07, 2016 at 09:02:26PM +0530, Vignesh R wrote: >>> >>>> In case of DT boot, don't read default speed and mode for SPI from >>>> CONFIG_*, instead read from DT node. This will make sure that boards >>>> with multiple SPI/QSPI controllers can be probed at different >>>> bus frequencies and modes. >>>> >>>> Signed-off-by: Vignesh R <vigneshr@ti.com> >>>> --- >>>> cmd/sf.c | 2 ++ >>>> drivers/spi/spi-uclass.c | 8 ++++++-- >>>> 2 files changed, 8 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/cmd/sf.c b/cmd/sf.c >>>> index 42862d9d921a..4c40d5104422 100644 >>>> --- a/cmd/sf.c >>>> +++ b/cmd/sf.c >>>> @@ -88,6 +88,8 @@ static int do_spi_flash_probe(int argc, char * const argv[]) >>>> #ifdef CONFIG_DM_SPI_FLASH >>>> struct udevice *new, *bus_dev; >>>> int ret; >>>> + /* In DM mode defaults wiil be taken from DT */ >>>> + speed = -1, mode = -1; > > speed and mode are uint and it is assigned with -1 which is not making > sense. Assigning 0 will be a better option. Ah, thanks! I will fix this up in the next version.
diff --git a/cmd/sf.c b/cmd/sf.c index 42862d9d921a..4c40d5104422 100644 --- a/cmd/sf.c +++ b/cmd/sf.c @@ -88,6 +88,8 @@ static int do_spi_flash_probe(int argc, char * const argv[]) #ifdef CONFIG_DM_SPI_FLASH struct udevice *new, *bus_dev; int ret; + /* In DM mode defaults wiil be taken from DT */ + speed = -1, mode = -1; #else struct spi_flash *new; #endif diff --git a/drivers/spi/spi-uclass.c b/drivers/spi/spi-uclass.c index 5561f36762f9..55ebb5ef19b7 100644 --- a/drivers/spi/spi-uclass.c +++ b/drivers/spi/spi-uclass.c @@ -264,6 +264,7 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode, struct udevice **busp, struct spi_slave **devp) { struct udevice *bus, *dev; + struct dm_spi_slave_platdata *plat; bool created = false; int ret; @@ -280,8 +281,6 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode, * SPI flash chip - we will bind to the correct driver. */ if (ret == -ENODEV && drv_name) { - struct dm_spi_slave_platdata *plat; - debug("%s: Binding new device '%s', busnum=%d, cs=%d, driver=%s\n", __func__, dev_name, busnum, cs, drv_name); ret = device_bind_driver(bus, drv_name, dev_name, &dev); @@ -308,6 +307,11 @@ int spi_get_bus_and_cs(int busnum, int cs, int speed, int mode, slave->dev = dev; } + plat = dev_get_parent_platdata(dev); + if (speed == -1) + speed = plat->max_hz; + if (mode == -1) + mode = plat->mode; ret = spi_set_speed_mode(bus, speed, mode); if (ret) goto err;
In case of DT boot, don't read default speed and mode for SPI from CONFIG_*, instead read from DT node. This will make sure that boards with multiple SPI/QSPI controllers can be probed at different bus frequencies and modes. Signed-off-by: Vignesh R <vigneshr@ti.com> --- cmd/sf.c | 2 ++ drivers/spi/spi-uclass.c | 8 ++++++-- 2 files changed, 8 insertions(+), 2 deletions(-)