Re: [PATCH] mmc: OCTEON: Add host driver for OCTEON MMC controller
From: David Daney
Date: Mon Jan 19 2015 - 14:02:01 EST
On 01/19/2015 06:28 AM, Mark Rutland wrote:
Hi,
On Mon, Jan 19, 2015 at 01:46:36PM +0000, Aleksey Makarov wrote:
The OCTEON MMC controller is currently found on cn61XX and cnf71XX
devices. Device parameters are configured from device tree data.
Currenly supported are eMMC, MMC and SD devices.
Does that mean there are other devices that this could support in
future, or that this supports everything it possibly can already?
It is software, so obviously there are some changes that could be made
to support other devices. However, the bus host hardware is designed
such that it can run only certain fixed sequences of the MMC and SD
protocols. Therefore it is probable that support for some types of
devices would be impossible...
[...]
diff --git a/Documentation/devicetree/bindings/mmc/octeon-mmc.txt b/Documentation/devicetree/bindings/mmc/octeon-mmc.txt
new file mode 100644
index 0000000..ce373cf
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/octeon-mmc.txt
@@ -0,0 +1,51 @@
+* 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
+slots appear as children of the main controller node.
+
+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 : Two entries:
+ 1) The MMC controller interrupt line.
+ 2) The MMC DMA engine interrupt line.
Is the DMA engine part of the MMC controller block, or is it a
separate/generic component that happens to be used in combination with
the MMC controller?
The DMA engine is an integral part of the MMC bus controller block.
+- #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.
+- cavium,bus-max-width : The number of data lines present in the slot.
+- spi-max-frequency : The maximum operating frequency of the slot.
+
+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
No clocks in the DT?
No. The reference clock rates can be probed at runtime, so they are not
specified in the DT.
+
+Example:
+ mmc@1180000002000 {
+ compatible = "cavium,octeon-6130-mmc";
+ reg = <0x11800 0x00002000 0x0 0x100>,
+ <0x11800 0x00000168 0x0 0x20>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ /* EMM irq, DMA irq */
+ interrupts = <1 19>, <0 63>;
+
+ /* The board only has a single MMC slot */
+ mmc-slot@0 {
+ compatible = "cavium,octeon-6130-mmc-slot";
+ reg = <0>;
+ spi-max-frequency = <20000000>;
+ /* bus width can be 1, 4 or 8 */
+ cavium,bus-max-width = <8>;
+ cd-gpios = <&gpio 9 0>;
+ wp-gpios = <&gpio 10 0>;
+ power-gpios = <&gpio 8 0>;
+ };
+ };
[...]
+
+/**
+ * Unlock a single line in the L2 cache.
+ *
+ * @addr Physical address to unlock
+ *
+ * Return Zero on success
+ */
+static void l2c_unlock_line(u64 addr)
+{
+ char *addr_ptr = phys_to_ptr(addr);
+ asm volatile (
+ "cache 23, %[line]" /* Unlock the line */
+ :: [line] "m" (*addr_ptr));
+}
+
+/**
+ * Unlock a memory region in the L2 cache
+ *
+ * @start - start address to unlock
+ * @len - length to unlock in bytes
+ */
+static void l2c_unlock_mem_region(u64 start, u64 len)
+{
+ u64 end;
+
+ /* Round start/end to cache line boundaries */
+ end = ALIGN(start + len - 1, CVMX_CACHE_LINE_SIZE);
+ start = ALIGN(start, CVMX_CACHE_LINE_SIZE);
+
+ while (start <= end) {
+ l2c_unlock_line(start);
+ start += CVMX_CACHE_LINE_SIZE;
+ }
+}
Why do you need to mess with the cache in this way within an MMC driver?
Due to an imperfection in the design of the MMC bus hardware, The 2nd.
to last cache block of a DMA read must be locked into the L2 Cache.
Otherwise, data corruption may occur. Please note that this MMC bus
hardware block is only ever encountered on SoCs where the L2 Cache is
resident on the same piece of silicon.
To me it seems somewhat surprising that you need to do that,
Your surprise was shared by many.
and again
surprising that you have custom functions within the MMC driver.
It is only used by this file, so we decided to put it here to keep
everything in one place.
[...]
+static int octeon_mmc_probe(struct platform_device *pdev)
+{
+ union cvmx_mio_emm_cfg emm_cfg;
+ struct octeon_mmc_host *host;
+ struct resource *res;
+ void __iomem *base;
+ int mmc_irq[9];
+ int i;
+ int ret = 0;
+ struct device_node *node = pdev->dev.of_node;
+ int found = 0;
+ bool cn78xx_style;
+ u64 t;
+ enum of_gpio_flags f;
+
+ host = devm_kzalloc(&pdev->dev, sizeof(*host), GFP_KERNEL);
+ if (!host)
+ return -ENOMEM;
+
+ spin_lock_init(&host->irq_handler_lock);
+ sema_init(&host->mmc_serializer, 1);
+
+ cn78xx_style = of_device_is_compatible(node, "cavium,octeon-7890-mmc");
+ if (cn78xx_style) {
+ host->need_bootbus_lock = false;
+ host->big_dma_addr = true;
+ host->need_irq_handler_lock = true;
+ /*
+ * First seven are the EMM_INT bits 0..6, then two for
+ * the EMM_DMA_INT bits
+ */
+ for (i = 0; i < 9; i++) {
+ mmc_irq[i] = platform_get_irq(pdev, i);
+ if (mmc_irq[i] < 0)
+ return mmc_irq[i];
The documentation describes two interrupts,
The documentation is incorrect for the case of cn78xx_style. The
documentation should be (and will be) corrected to reflect what each of
these lines is used for.
yet here you try to acquire
nine. Please document which interrupts you expect (i.e. in what order
and what the logical function of each is), and when you expect them.
If some interrupts might not always be present, use interrupt-names.
[...]
+ base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(base))
+ return PTR_ERR(base);
+ host->base = (u64)base;
I take it this can only be used on a 64-bit platform?
Yes.
[...]
+
+ platform_set_drvdata(pdev, host);
+
+ node = of_get_next_child(pdev->dev.of_node, NULL);
+ while (node) {
You should use for_each_child_of_node here.
Also, you'll want to skip nodes that aren't compatible with
"cavium,octeon-6130-mmc-slot", and you'll probably want to log a warning
in that case.
OK, I think those functions did not exist when the code was originally
written. We will try to improve it here.
+ int r;
+ u32 slot;
+
+ found = 0;
+
+ r = of_property_read_u32(node, "reg", &slot);
+ if (!r) {
If you use for_each_child_of_node, you can just skip ahead in this case,
rather than having 90% of this block conditional.
e.g.
for_each_child_of_node(pdev->dev.of_node, node) {
if (!of_device_is_compatible(node, "cavium,octeon-6130-mmc-slot") {
pr_warn("sub node isn't slot: %s\n", of_node_full_name(node));
continue;
}
if (of_property_read_u32(node, "reg", &slot) != 0) {
pr_warn("Missing or invalid reg property on %s\n",
of_node_full_name(node));
continue;
}
other_stuff();
not_indented();
}
+ int ro_gpio, cd_gpio, pwr_gpio;
+ bool ro_low, cd_low, pwr_low;
+ u32 bus_width, max_freq, cmd_skew, dat_skew;
+
+ r = of_property_read_u32(node, "cavium,bus-max-width",
+ &bus_width);
+ if (r) {
+ pr_info("Bus width not found for slot %d\n",
+ slot);
+ bus_width = 8;
If the default assumption is 8 lanes, document that in the binding.
Otherwise, make this mandatory and skip the slot if this is missing from
the DT.
+ } else {
+ switch (bus_width) {
+ case 1:
+ case 4:
+ case 8:
+ break;
+ default:
+ bus_width = 8;
+ break;
+ }
+ }
Silently modifying this value is not a good idea. If you get an invalid
value, warn and skip the node. If the DT was wrong then you have no
chance of guessing better anyway.
+
+ r = of_property_read_u32(node, "cavium,cmd-clk-skew",
+ &cmd_skew);
+ if (r)
+ cmd_skew = 0;
+
+ r = of_property_read_u32(node, "cavium,dat-clk-skew",
+ &dat_skew);
+ if (r)
+ dat_skew = 0;
+
These weren't in the binding. They need to be documented.
+ r = of_property_read_u32(node, "spi-max-frequency",
+ &max_freq);
+ if (r) {
+ max_freq = 52000000;
+ pr_info("no spi-max-frequency for slot %d, defautling to %d\n",
s/defautling/defaulting/
+ slot, max_freq);
+ }
+
+ ro_gpio = of_get_named_gpio_flags(node, "wp-gpios",
+ 0, &f);
+ ro_low = (ro_gpio >= 0 && f == OF_GPIO_ACTIVE_LOW);
+ cd_gpio = of_get_named_gpio_flags(node, "cd-gpios",
+ 0, &f);
+ cd_low = (cd_gpio >= 0 && f == OF_GPIO_ACTIVE_LOW);
+ pwr_gpio = of_get_named_gpio_flags(node, "power-gpios",
+ 0, &f);
+ pwr_low = (pwr_gpio >= 0 && f == OF_GPIO_ACTIVE_LOW);
+
+ ret = octeon_init_slot(host, slot, bus_width, max_freq,
+ ro_gpio, cd_gpio, pwr_gpio,
+ ro_low, cd_low, pwr_low,
+ cmd_skew, dat_skew);
+ octeon_mmc_dbg("init slot %d, ret = %d\n", slot, ret);
+ if (ret)
+ goto err;
+ }
+ node = of_get_next_child(pdev->dev.of_node, node);
+ }
+
+ return ret;
+
+err:
+ /* Disable MMC controller */
+ emm_cfg.s.bus_ena = 0;
+ cvmx_write_csr(host->base + OCT_MIO_EMM_CFG, emm_cfg.u64);
+
+ if (host->global_pwr_gpio >= 0) {
+ dev_dbg(&pdev->dev, "Global power off\n");
+ gpio_set_value_cansleep(host->global_pwr_gpio,
+ host->global_pwr_gpio_low);
+ gpio_free(host->global_pwr_gpio);
+ }
+
+ return ret;
+}
It's probably worth having some logging in this error path.
Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/