On 16 March 2015 at 16:06, Aleksey Makarov <aleksey.makarov@xxxxxxxxxx> wrote:
The OCTEON MMC controller is currently found on cn61XX and cnf71XX
devices. Device parameters are configured from device tree data.
eMMC, MMC and SD devices are supported.
Tested-by: Aaro Koskinen <aaro.koskinen@xxxxxx>
Signed-off-by: Chandrakala Chavva <cchavva@xxxxxxxxxxxxxxxxxx>
Signed-off-by: David Daney <david.daney@xxxxxxxxxx>
Signed-off-by: Aleksey Makarov <aleksey.makarov@xxxxxxxxxx>
Signed-off-by: Leonid Rosenboim <lrosenboim@xxxxxxxxxxxxxxxxxx>
Signed-off-by: Peter Swain <pswain@xxxxxxxxxx>
Signed-off-by: Aaron Williams <aaron.williams@xxxxxxxxxx>
---
.../devicetree/bindings/mmc/octeon-mmc.txt | 69 +
drivers/mmc/host/Kconfig | 10 +
drivers/mmc/host/Makefile | 1 +
drivers/mmc/host/octeon_mmc.c | 1415 ++++++++++++++++++++
4 files changed, 1495 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mmc/octeon-mmc.txt
create mode 100644 drivers/mmc/host/octeon_mmc.c
This patch should be applied on top of the patchset
"MIPS: OCTEON: flash: syncronize bootbus access"
https://lkml.kernel.org/g/<1425565893-30614-1-git-send-email-aleksey.makarov@xxxxxxxxxx>
v3:
https://lkml.kernel.org/g/<1425567033-31236-1-git-send-email-aleksey.makarov@xxxxxxxxxx>
Changes in v4:
- The sparse error discovered by Aaro Koskinen has been fixed
- Other sparse warnings have been silenced
Changes in v3:
- Rebased to v4.0-rc2
- Use gpiod_*() functions instead of legacy gpio
- Cosmetic changes
Changes in v2: All the fixes suggested by Mark Rutland were implemented:
- Device tree parsing has been fixed
- Device tree docs have been fixed
- Comment about errata workaroud has been added
diff --git a/Documentation/devicetree/bindings/mmc/octeon-mmc.txt b/Documentation/devicetree/bindings/mmc/octeon-mmc.txt
new file mode 100644
index 0000000..40dd7f1
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/octeon-mmc.txt
@@ -0,0 +1,69 @@
+* OCTEON SD/MMC Host Controller
+
+This controller is present on some members of the Cavium OCTEON SoC
+family, provide an interface for eMMC, MMC and SD devices. There is a
+single controller that may have several "slots" connected. These
Even it it may have several slots, that's not being supported by the
mmc core from a MMC/SD/SDIO protocol perspective.
We have hade some discussions around this historocally, and I doubt
that we ever will be adding this.
So, with that in mind I would rather see that you remove the concept
of "slot" entirely and thus don't do the DT configuration within a
child node.
+slots appear as children of the main controller node.
+The DMA engine is an integral part of the controller block.
+
+Required properties:
+- compatible : Should be "cavium,octeon-6130-mmc" or "cavium,octeon-7890-mmc"
+- reg : Two entries:
+ 1) The base address of the MMC controller register bank.
+ 2) The base address of the MMC DMA engine register bank.
+- interrupts :
+ For "cavium,octeon-6130-mmc": two entries:
+ 1) The MMC controller interrupt line.
+ 2) The MMC DMA engine interrupt line.
+ For "cavium,octeon-7890-mmc": nine entries:
+ 1) The next block transfer of a multiblock transfer has completed (BUF_DONE)
+ 2) Operation completed successfully (CMD_DONE).
+ 3) DMA transfer completed successfully (DMA_DONE).
+ 4) Operation encountered an error (CMD_ERR).
+ 5) DMA transfer encountered an error (DMA_ERR).
+ 6) Switch operation completed successfully (SWITCH_DONE).
+ 7) Switch operation encountered an error (SWITCH_ERR).
+ 8) Internal DMA engine request completion interrupt (DONE).
+ 9) Internal DMA FIFO underflow (FIFO).
So are you really having that many irq lines to the controller?
+- #address-cells : Must be <1>
+- #size-cells : Must be <0>
+
+Required properties of child nodes:
+- compatible : Should be "cavium,octeon-6130-mmc-slot".
+- reg : The slot number.
+
+Optional properties of child nodes:
+- cd-gpios : Specify GPIOs for card detection
+- wp-gpios : Specify GPIOs for write protection
+- power-gpios : Specify GPIOs for power control
Please consider to modell these through gpio regulators instead. In
that way you well automatically get the ocr mask a well.
+- cavium,bus-max-width : The number of data lines present in the slot.
+ Default is 8.
+- spi-max-frequency : The maximum operating frequency of the slot.
+ Default is 52000000.
Please use the common mmc binding for both the above.
+- cavium,cmd-clk-skew : the amount of delay (in pS) past the clock edge
+ to sample the command pin.
+- cavium,dat-clk-skew : the amount of delay (in pS) past the clock edge
+ to sample the data pin.
Is this SoC specifc?
[...]
[...]diff --git a/drivers/mmc/host/octeon_mmc.c b/drivers/mmc/host/octeon_mmc.c
new file mode 100644
index 0000000..a3f10c6
--- /dev/null
+++ b/drivers/mmc/host/octeon_mmc.c
+
+#include <asm/byteorder.h>
+#include <asm/octeon/octeon.h>
+#include <asm/octeon/cvmx-mio-defs.h>
Please don't have these kind of SOC specific includes.
+
+ struct octeon_mmc_slot *slot[OCTEON_MAX_MMC];
See my upper comment around slots, and please remove all realted code.
+};
+
+struct octeon_mmc_slot {
+ struct mmc_host *mmc; /* slot-level mmc_core object */
+ struct octeon_mmc_host *host; /* common hw for all 4 slots */
+
+ unsigned int clock;
+ unsigned int sclock;
+
+ u64 cached_switch;
+ u64 cached_rca;
+
+ unsigned int cmd_cnt; /* sample delay */
+ unsigned int dat_cnt; /* sample delay */
+
+ int bus_width;
+ int bus_id;
+
+ struct gpio_desc *ro_gpiod;
+ struct gpio_desc *cd_gpiod;
These shouldn't be needed since it's already handle by the mmc core
(mmc slot-gpio).
+ struct gpio_desc *pwr_gpiod;
As stated convert to gpio regulators instead.
+ ret = of_property_read_u32(node, "reg", &id);
+ if (ret) {
+ dev_err(dev, "Missing or invalid reg property on %s\n",
+ of_node_full_name(node));
+ return ret;
+ }
+
+ if (id >= OCTEON_MAX_MMC || host->slot[id]) {
+ dev_err(dev, "Invalid reg property on %s\n",
+ of_node_full_name(node));
+ return -EINVAL;
+ }
+
+ mmc = mmc_alloc_host(sizeof(struct octeon_mmc_slot), dev);
+ if (!mmc) {
+ dev_err(dev, "alloc host failed\n");
+ return -ENOMEM;
+ }
+
+ slot = mmc_priv(mmc);
+ slot->mmc = mmc;
+ slot->host = host;
+
I would like you to use mmc_of_parse() somewhere here to support the
common mmc DT bindings.
[...]
So this was I quick review, unfortunate I don't have more time right
now. But please go ahead and adress my comments, then I will look into
the next version more thoroughly.