Re: [U-Boot] [PATCH v2 2/2] mmc: Add bcm2835 sdhost controller

Tue, 23 Jan 2018 08:04:29 -0800 

On 01/18/2018 05:41 AM, Jaehoon Chung wrote:

On 01/18/2018 08:33 AM, Alexander Graf wrote:

The BCM2835 family of SoCs has 2 different SD controllers: One based on
the SDHCI spec and a custom, home-grown one.

This patch implements a driver for the latter based on the Linux driver.
This is needed so that we can make use of device trees that assume driver
presence of both SD controllers.

Signed-off-by: Alexander Graf <ag...@suse.de>

---

v1 -> v2:

   - Remove hand written pinctrl support
   - Checkpatch fixes
---
  MAINTAINERS                  |   1 +
  drivers/mmc/Kconfig          |  14 +
  drivers/mmc/Makefile         |   1 +
  drivers/mmc/bcm2835_sdhost.c | 994 +++++++++++++++++++++++++++++++++++++++++++
  4 files changed, 1010 insertions(+)
  create mode 100644 drivers/mmc/bcm2835_sdhost.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 1f2545191b..728d38aebf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -97,6 +97,7 @@ S:    Orphaned (Since 2017-07)
  F:    arch/arm/mach-bcm283x/
  F:    drivers/gpio/bcm2835_gpio.c
  F:    drivers/mmc/bcm2835_sdhci.c
+F:     drivers/mmc/bcm2835_sdhost.c
  F:    drivers/serial/serial_bcm283x_mu.c
  F:    drivers/video/bcm2835.c
  F:    include/dm/platform_data/serial_bcm283x_mu.h
diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
index ab0627a8af..9b90db908b 100644
--- a/drivers/mmc/Kconfig
+++ b/drivers/mmc/Kconfig
@@ -256,6 +256,20 @@ config MMC_UNIPHIER
          This selects support for the Matsushita SD/MMC Host Controller on
          SocioNext UniPhier and Renesas RCar SoCs.
+config MMC_BCM2835 
+       bool "BCM2835 family custom SD/MMC Host Controller support"
+       depends on ARCH_BCM283X
+       depends on BLK && DM_MMC
+       depends on OF_CONTROL
+       default y
+       help
+         This selects support for the custom SD host controller in the BCM2835
+         family of devices.
+
+         If you have a BCM2835 platform with SD or MMC devices, say Y here.
+
+         If unsure, say N.
+
  config MMC_SANDBOX
        bool "Sandbox MMC support"
        depends on SANDBOX
diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile
index 64b6f21c61..42113e2603 100644
--- a/drivers/mmc/Makefile
+++ b/drivers/mmc/Makefile
@@ -64,3 +64,4 @@ obj-$(CONFIG_MMC_SDHCI_ZYNQ)          += zynq_sdhci.o
obj-$(CONFIG_MMC_SUNXI) += sunxi_mmc.o 
  obj-$(CONFIG_MMC_UNIPHIER)            += uniphier-sd.o
+obj-$(CONFIG_MMC_BCM2835)              += bcm2835_sdhost.o
diff --git a/drivers/mmc/bcm2835_sdhost.c b/drivers/mmc/bcm2835_sdhost.c
new file mode 100644
index 0000000000..ad9c7adb5a
--- /dev/null
+++ b/drivers/mmc/bcm2835_sdhost.c
@@ -0,0 +1,994 @@
+/*
+ * bcm2835 sdhost driver.
+ *
+ * The 2835 has two SD controllers: The Arasan sdhci controller
+ * (supported by the iproc driver) and a custom sdhost controller
+ * (supported by this driver).
+ *
+ * The sdhci controller supports both sdcard and sdio.  The sdhost
+ * controller supports the sdcard only, but has better performance.
+ * Also note that the rpi3 has sdio wifi, so driving the sdcard with
+ * the sdhost controller allows to use the sdhci controller for wifi
+ * support.
+ *
+ * The configuration is done by devicetree via pin muxing.  Both
+ * SD controller are available on the same pins (2 pin groups = pin 22
+ * to 27 + pin 48 to 53).  So it's possible to use both SD controllers
+ * at the same time with different pin groups.
+ *
+ * This code was ported to U-Boot by
+ *  Alexander Graf <ag...@suse.de>
+ * and is based on drivers/mmc/host/bcm2835.c in Linux which is written by
+ *  Phil Elwell <p...@raspberrypi.org>
+ *  Copyright (C) 2015-2016 Raspberry Pi (Trading) Ltd.
+ * which is based on
+ *  mmc-bcm2835.c by Gellert Weisz
+ * which is, in turn, based on
+ *  sdhci-bcm2708.c by Broadcom
+ *  sdhci-bcm2835.c by Stephen Warren and Oleksandr Tymoshenko
+ *  sdhci.c and sdhci-pci.c by Pierre Ossman
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.

Can be replaced to SPDX-License-Identifier?
Thanks a lot for the very in-depth review. I think I've fixed most of your comments :). 

[...]


+
+       if (!host->data)
+               return 0;
+       if (intmask & (SDHSTS_CRC16_ERROR | SDHSTS_FIFO_ERROR))
+               r = -EILSEQ;
+       if (intmask & SDHSTS_REW_TIME_OUT)
+               r = -ETIMEDOUT;
+
+       if (r)
+               printf("%s:%d %d\n", __func__, __LINE__, r);
+
+       return r;
+}
+
+static void bcm2835_busy_irq(struct bcm2835_host *host)
+{
+       if (WARN_ON(!host->cmd)) {
+               bcm2835_dumpregs(host);
+               return;
+       }
+
+       if (WARN_ON(!host->use_busy)) {
+               bcm2835_dumpregs(host);
+               return;
+       }

Really need to use WARN_ON? for what?
We shouldn't get here without use_busy set. If we did, something went terribly wrong. I guess the alternative is an assert(), but that only runs when debug is defined. 


Alex

_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Reply via email to