diff mbox series

[RFC] mtd: spi: Drop redundent SPI flash driver

Message ID 20200514131935.8583-1-jagan@amarulasolutions.com
State New
Headers show
Series [RFC] mtd: spi: Drop redundent SPI flash driver | expand

Commit Message

Jagan Teki May 14, 2020, 1:19 p.m. UTC 
UCLASS_SPI_FLASH driver at driver/mtd/spi is a generic
spi flash driver to probe jedec,spi-nor flash chips.

Technically a probe call in U_BOOT_DRIVER is local to that
driver and not applicable to use it another driver or in
another code.

The apollolake SPL code using the generic probe by adding
extra SPI flash driver, which make more confusion in terms
of code readability and driver model structure.

The fact that apollolake SPL requires a separate SPI flash
driver to handle of-platdata via bind call, so move the
bind call in the generic flash driver and drop the driver
from apollolake code.

I hope this wouldn't break generic code usage flash chips
otherwise, we can handle this via driver data or a separate
spi driver in drivers/mtd/spi.

Cc: Bin Meng <bmeng.cn at gmail.com>
Cc: Simon Glass <sjg at chromium.org>
Cc: Vignesh R <vigneshr at ti.com>
Signed-off-by: Jagan Teki <jagan at amarulasolutions.com>
---
 arch/x86/cpu/apollolake/spl.c | 60 -----------------------------------
 drivers/mtd/spi/sf_probe.c    | 29 ++++++++++++++++-
 include/spi_flash.h           | 12 -------
 3 files changed, 28 insertions(+), 73 deletions(-)

Comments

Simon Glass May 25, 2020, 5:04 p.m. UTC | #1 
Hi Jagan,

On Thu, 14 May 2020 at 07:19, Jagan Teki <jagan at amarulasolutions.com> wrote:
>
> UCLASS_SPI_FLASH driver at driver/mtd/spi is a generic
> spi flash driver to probe jedec,spi-nor flash chips.
>
> Technically a probe call in U_BOOT_DRIVER is local to that
> driver and not applicable to use it another driver or in
> another code.
>
> The apollolake SPL code using the generic probe by adding
> extra SPI flash driver, which make more confusion in terms
> of code readability and driver model structure.
>
> The fact that apollolake SPL requires a separate SPI flash
> driver to handle of-platdata via bind call, so move the
> bind call in the generic flash driver and drop the driver
> from apollolake code.
>
> I hope this wouldn't break generic code usage flash chips
> otherwise, we can handle this via driver data or a separate
> spi driver in drivers/mtd/spi.
>
> Cc: Bin Meng <bmeng.cn at gmail.com>
> Cc: Simon Glass <sjg at chromium.org>
> Cc: Vignesh R <vigneshr at ti.com>
> Signed-off-by: Jagan Teki <jagan at amarulasolutions.com>
> ---
>  arch/x86/cpu/apollolake/spl.c | 60 -----------------------------------
>  drivers/mtd/spi/sf_probe.c    | 29 ++++++++++++++++-
>  include/spi_flash.h           | 12 -------
>  3 files changed, 28 insertions(+), 73 deletions(-)

Unfortunately this stops TPL from booting.

Could you expose the 'read' method properly, perhaps?

Regards,
Simon
Jagan Teki May 25, 2020, 5:31 p.m. UTC | #2 
On Mon, May 25, 2020 at 10:34 PM Simon Glass <sjg at chromium.org> wrote:
>
> Hi Jagan,
>
> On Thu, 14 May 2020 at 07:19, Jagan Teki <jagan at amarulasolutions.com> wrote:
> >
> > UCLASS_SPI_FLASH driver at driver/mtd/spi is a generic
> > spi flash driver to probe jedec,spi-nor flash chips.
> >
> > Technically a probe call in U_BOOT_DRIVER is local to that
> > driver and not applicable to use it another driver or in
> > another code.
> >
> > The apollolake SPL code using the generic probe by adding
> > extra SPI flash driver, which make more confusion in terms
> > of code readability and driver model structure.
> >
> > The fact that apollolake SPL requires a separate SPI flash
> > driver to handle of-platdata via bind call, so move the
> > bind call in the generic flash driver and drop the driver
> > from apollolake code.
> >
> > I hope this wouldn't break generic code usage flash chips
> > otherwise, we can handle this via driver data or a separate
> > spi driver in drivers/mtd/spi.
> >
> > Cc: Bin Meng <bmeng.cn at gmail.com>
> > Cc: Simon Glass <sjg at chromium.org>
> > Cc: Vignesh R <vigneshr at ti.com>
> > Signed-off-by: Jagan Teki <jagan at amarulasolutions.com>
> > ---
> >  arch/x86/cpu/apollolake/spl.c | 60 -----------------------------------
> >  drivers/mtd/spi/sf_probe.c    | 29 ++++++++++++++++-
> >  include/spi_flash.h           | 12 -------
> >  3 files changed, 28 insertions(+), 73 deletions(-)
>
> Unfortunately this stops TPL from booting.
>
> Could you expose the 'read' method properly, perhaps?

Not sure why it requires, read call seems very much similar to sf isn't it?

Jagan.
diff mbox series

Patch

diff --git a/arch/x86/cpu/apollolake/spl.c b/arch/x86/cpu/apollolake/spl.c
index d32f2a9898..2e6013d04c 100644
--- a/arch/x86/cpu/apollolake/spl.c
+++ b/arch/x86/cpu/apollolake/spl.c
@@ -65,66 +65,6 @@  SPL_LOAD_IMAGE_METHOD("Mapped SPI", 2, BOOT_DEVICE_SPI_MMAP, rom_load_image);
 
 #if CONFIG_IS_ENABLED(SPI_FLASH_SUPPORT)
 
-static int apl_flash_std_read(struct udevice *dev, u32 offset, size_t len,
-			      void *buf)
-{
-	struct spi_flash *flash = dev_get_uclass_priv(dev);
-	struct mtd_info *mtd = &flash->mtd;
-	size_t retlen;
-
-	return log_ret(mtd->_read(mtd, offset, len, &retlen, buf));
-}
-
-static int apl_flash_probe(struct udevice *dev)
-{
-	return spi_flash_std_probe(dev);
-}
-
-/*
- * Manually set the parent of the SPI flash to SPI, since dtoc doesn't. We also
- * need to allocate the parent_platdata since by the time this function is
- * called device_bind() has already gone past that step.
- */
-static int apl_flash_bind(struct udevice *dev)
-{
-	if (CONFIG_IS_ENABLED(OF_PLATDATA)) {
-		struct dm_spi_slave_platdata *plat;
-		struct udevice *spi;
-		int ret;
-
-		ret = uclass_first_device_err(UCLASS_SPI, &spi);
-		if (ret)
-			return ret;
-		dev->parent = spi;
-
-		plat = calloc(sizeof(*plat), 1);
-		if (!plat)
-			return -ENOMEM;
-		dev->parent_platdata = plat;
-	}
-
-	return 0;
-}
-
-static const struct dm_spi_flash_ops apl_flash_ops = {
-	.read		= apl_flash_std_read,
-};
-
-static const struct udevice_id apl_flash_ids[] = {
-	{ .compatible = "jedec,spi-nor" },
-	{ }
-};
-
-U_BOOT_DRIVER(winbond_w25q128fw) = {
-	.name		= "winbond_w25q128fw",
-	.id		= UCLASS_SPI_FLASH,
-	.of_match	= apl_flash_ids,
-	.bind		= apl_flash_bind,
-	.probe		= apl_flash_probe,
-	.priv_auto_alloc_size = sizeof(struct spi_flash),
-	.ops		= &apl_flash_ops,
-};
-
 /* This uses a SPI flash device to read the next phase */
 static int spl_fast_spi_load_image(struct spl_image_info *spl_image,
 				   struct spl_boot_device *bootdev)
diff --git a/drivers/mtd/spi/sf_probe.c b/drivers/mtd/spi/sf_probe.c
index f167bfab8a..06dac57daf 100644
--- a/drivers/mtd/spi/sf_probe.c
+++ b/drivers/mtd/spi/sf_probe.c
@@ -129,7 +129,7 @@  static int spi_flash_std_erase(struct udevice *dev, u32 offset, size_t len)
 	return mtd->_erase(mtd, &instr);
 }
 
-int spi_flash_std_probe(struct udevice *dev)
+static int spi_flash_std_probe(struct udevice *dev)
 {
 	struct spi_slave *slave = dev_get_parent_priv(dev);
 	struct spi_flash *flash;
@@ -154,6 +154,32 @@  static const struct dm_spi_flash_ops spi_flash_std_ops = {
 	.erase = spi_flash_std_erase,
 };
 
+/*
+ * Manually set the parent of the SPI flash to SPI, since dtoc doesn't. We also
+ * need to allocate the parent_platdata since by the time this function is
+ * called device_bind() has already gone past that step.
+ */
+static int spi_flash_bind(struct udevice *dev)
+{
+	if (CONFIG_IS_ENABLED(OF_PLATDATA)) {
+		struct dm_spi_slave_platdata *plat;
+		struct udevice *spi;
+		int ret;
+
+		ret = uclass_first_device_err(UCLASS_SPI, &spi);
+		if (ret)
+			return ret;
+		dev->parent = spi;
+
+		plat = calloc(sizeof(*plat), 1);
+		if (!plat)
+			return -ENOMEM;
+		dev->parent_platdata = plat;
+	}
+
+	return 0;
+}
+
 static const struct udevice_id spi_flash_std_ids[] = {
 	{ .compatible = "jedec,spi-nor" },
 	{ }
@@ -163,6 +189,7 @@  U_BOOT_DRIVER(spi_flash_std) = {
 	.name		= "spi_flash_std",
 	.id		= UCLASS_SPI_FLASH,
 	.of_match	= spi_flash_std_ids,
+	.bind		= spi_flash_bind,
 	.probe		= spi_flash_std_probe,
 	.remove		= spi_flash_std_remove,
 	.priv_auto_alloc_size = sizeof(struct spi_flash),
diff --git a/include/spi_flash.h b/include/spi_flash.h
index d9b2af856c..3b33b970ef 100644
--- a/include/spi_flash.h
+++ b/include/spi_flash.h
@@ -75,18 +75,6 @@  int spi_flash_write_dm(struct udevice *dev, u32 offset, size_t len,
  */
 int spi_flash_erase_dm(struct udevice *dev, u32 offset, size_t len);
 
-/**
- * spi_flash_std_probe() - Probe a SPI flash device
- *
- * This is the standard internal method for probing a SPI flash device to
- * determine its type. It can be used in chip-specific drivers which need to
- * do this, typically with of-platdata
- *
- * @dev: SPI-flash device to probe
- * @return 0 if OK, -ve on error
- */
-int spi_flash_std_probe(struct udevice *dev);
-
 int spi_flash_probe_bus_cs(unsigned int busnum, unsigned int cs,
 			   unsigned int max_hz, unsigned int spi_mode,
 			   struct udevice **devp);