| Message ID | 1432042454-19234-2-git-send-email-javier.martinez@collabora.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Javier, On Tue, May 19, 2015 at 03:34:11PM +0200, Javier Martinez Canillas wrote: > Google Chromebooks have a SPI flash that is used to store firmware and > different system parameters and data (i.e: Google Binary Block flags). > > Since there isn't a driver for it yet, the spidev interface is used to > access the flash from user-space (i.e: using the flashrom tool). > > Add a "google,spi-flash" compatible string so the Device Tree sources > use it instead of the "spidev" compatible which does not describe the > real HW and is just a Linux implementation detail. > > A generic "google,spi-flash" OF device ID is used instead of the actual > vendor/model because these chips are commodity parts that are sourced > from multiple vendors. So specifying the exact vendor and model in the > DTS will add a maintenance burden with no real gain (the parts are 100% > compatible anyways) and will likely result in it simply being wrong for > a sizeable fraction of the machines. The compatible string and dt binding should be documented somewhere under Documentation/devicetree/bindings/. Also, please keep the dt list on Cc for dt related patches. baruch
Hello Baruch, On 05/19/2015 09:53 PM, Baruch Siach wrote: > Hi Javier, > > On Tue, May 19, 2015 at 03:34:11PM +0200, Javier Martinez Canillas wrote: >> Google Chromebooks have a SPI flash that is used to store firmware and >> different system parameters and data (i.e: Google Binary Block flags). >> >> Since there isn't a driver for it yet, the spidev interface is used to >> access the flash from user-space (i.e: using the flashrom tool). >> >> Add a "google,spi-flash" compatible string so the Device Tree sources >> use it instead of the "spidev" compatible which does not describe the >> real HW and is just a Linux implementation detail. >> >> A generic "google,spi-flash" OF device ID is used instead of the actual >> vendor/model because these chips are commodity parts that are sourced >> from multiple vendors. So specifying the exact vendor and model in the >> DTS will add a maintenance burden with no real gain (the parts are 100% >> compatible anyways) and will likely result in it simply being wrong for >> a sizeable fraction of the machines. > > The compatible string and dt binding should be documented somewhere under > Documentation/devicetree/bindings/. Also, please keep the dt list on Cc for dt > related patches. > Yes, I didn't add a binding doc because this is mostly a RFC to see if Mark finds the approach feasible but yes I should had included anyways, sorry about that. I'll add when posting as a proper patch if he agrees with the solution. > baruch > Best regards, Javier
On Tue, May 19, 2015 at 03:34:11PM +0200, Javier Martinez Canillas wrote: > Google Chromebooks have a SPI flash that is used to store firmware and > different system parameters and data (i.e: Google Binary Block flags). > --- > drivers/spi/spidev.c | 1 + > 1 file changed, 1 insertion(+) This is adding a binding with no documentation, documentation is mandatory for all bindings.
Hello Mark, On 05/20/2015 12:13 PM, Mark Brown wrote: > On Tue, May 19, 2015 at 03:34:11PM +0200, Javier Martinez Canillas wrote: >> Google Chromebooks have a SPI flash that is used to store firmware and >> different system parameters and data (i.e: Google Binary Block flags). > >> --- >> drivers/spi/spidev.c | 1 + >> 1 file changed, 1 insertion(+) > > This is adding a binding with no documentation, documentation is > mandatory for all bindings. > Yes, I missed... sorry about that. Do you agree with the approach though so I can re-spin the patches adding the missing DT binding? Best regards, Javier
On Wed, May 20, 2015 at 12:18:13PM +0200, Javier Martinez Canillas wrote: > On 05/20/2015 12:13 PM, Mark Brown wrote: > > This is adding a binding with no documentation, documentation is > > mandatory for all bindings. > Yes, I missed... sorry about that. Do you agree with the approach > though so I can re-spin the patches adding the missing DT binding? It's probably OK but I didn't really drill through since the binding was missing. If these parts are commodity as described it seems surprising that they aren't compatible with any existing kernel driver.
Hello Mark, On 05/20/2015 12:37 PM, Mark Brown wrote: > On Wed, May 20, 2015 at 12:18:13PM +0200, Javier Martinez Canillas wrote: >> On 05/20/2015 12:13 PM, Mark Brown wrote: > >> > This is adding a binding with no documentation, documentation is >> > mandatory for all bindings. > >> Yes, I missed... sorry about that. Do you agree with the approach >> though so I can re-spin the patches adding the missing DT binding? > > It's probably OK but I didn't really drill through since the binding was > missing. If these parts are commodity as described it seems surprising > that they aren't compatible with any existing kernel driver. > The ChromeOS user-space just uses flashrom to send a raw stream of bytes via spidev to the SPI NOR flash chip. There is drivers/mtd/spi-nor/spi-nor.c but AFAIU there are some limitations when interfacing the flash through the MTD layer, for example there isn't a way to set the SPI flash write protection through MTD. But I'll do some investigation before re-spinning the patches. BTW, the other "rohm,dh2228fv" compatible string in spidev added by commit 8fad805bdc52 ("spi: spidev: Add Rohm DH2228FV DAC compatible string"), also does not have a documented DT binding so that should be fixed as well. Best regards, Javier
On Wed, May 20, 2015 at 01:21:53PM +0200, Javier Martinez Canillas wrote: > The ChromeOS user-space just uses flashrom to send a raw stream of bytes > via spidev to the SPI NOR flash chip. There is drivers/mtd/spi-nor/spi-nor.c > but AFAIU there are some limitations when interfacing the flash through > the MTD layer, for example there isn't a way to set the SPI flash write > protection through MTD. But I'll do some investigation before re-spinning > the patches. OK, that's a bit concerning - perhaps what's needed here is to extend MTD and so on (if only to find some way for it to coexist with spidev for the time being)? > BTW, the other "rohm,dh2228fv" compatible string in spidev added by commit > 8fad805bdc52 ("spi: spidev: Add Rohm DH2228FV DAC compatible string"), also > does not have a documented DT binding so that should be fixed as well. wv :)
diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c index dd616ff0ffc5..b18cf7bdc385 100644 --- a/drivers/spi/spidev.c +++ b/drivers/spi/spidev.c @@ -693,6 +693,7 @@ static struct class *spidev_class; #ifdef CONFIG_OF static const struct of_device_id spidev_dt_ids[] = { { .compatible = "rohm,dh2228fv" }, + { .compatible = "google,spi-flash" }, {}, }; MODULE_DEVICE_TABLE(of, spidev_dt_ids);
Google Chromebooks have a SPI flash that is used to store firmware and different system parameters and data (i.e: Google Binary Block flags). Since there isn't a driver for it yet, the spidev interface is used to access the flash from user-space (i.e: using the flashrom tool). Add a "google,spi-flash" compatible string so the Device Tree sources use it instead of the "spidev" compatible which does not describe the real HW and is just a Linux implementation detail. A generic "google,spi-flash" OF device ID is used instead of the actual vendor/model because these chips are commodity parts that are sourced from multiple vendors. So specifying the exact vendor and model in the DTS will add a maintenance burden with no real gain (the parts are 100% compatible anyways) and will likely result in it simply being wrong for a sizeable fraction of the machines. Also, it would require to duplicate a DTS since the machine is the same besides using a different SPI flash part. Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> --- drivers/spi/spidev.c | 1 + 1 file changed, 1 insertion(+)