Re: [RFC PATCH v2 4/6] mmc: sdhci: host: add new f_sdh30
2014-06-27 18:12 GMT+08:00 Mark Rutland mark.rutl...@arm.com:
On Fri, Jun 27, 2014 at 04:32:21AM +0100, Vincent Yang wrote:
2014-06-26 19:03 GMT+08:00 Mark Rutland mark.rutl...@arm.com:
On Thu, Jun 26, 2014 at 07:23:30AM +0100, Vincent Yang wrote:
This patch adds new host controller driver for
Fujitsu SDHCI controller f_sdh30.
Signed-off-by: Vincent Yang vincent.y...@tw.fujitsu.com
---
.../devicetree/bindings/mmc/sdhci-fujitsu.txt | 35 +++
drivers/mmc/host/Kconfig | 7 +
drivers/mmc/host/Makefile | 1 +
drivers/mmc/host/sdhci_f_sdh30.c | 346
+
drivers/mmc/host/sdhci_f_sdh30.h | 40 +++
5 files changed, 429 insertions(+)
create mode 100644
Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt
create mode 100644 drivers/mmc/host/sdhci_f_sdh30.c
create mode 100644 drivers/mmc/host/sdhci_f_sdh30.h
diff --git a/Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt
b/Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt
new file mode 100644
index 000..40add438
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt
@@ -0,0 +1,35 @@
+* Fujitsu SDHCI controller
+
+This file documents differences between the core properties in mmc.txt
+and the properties used by the sdhci_f_sdh30 driver.
+
+Required properties:
+- compatible: fujitsu,f_sdh30
Please use '-' rather than '_' in compatible strings.
Hi Mark,
Yes, I'll update it to '-' in next version.
This seems to be the name of the driver. What is the precise name of the
IP block?
The name of the IP block is F_SDH30.
That's why it uses fujitsu,f_sdh30
Hmm. I'd still be tempted to use fujitsu,f-sdh30.
Hi Mark,
Sure, I'll update it to fujitsu,f-sdh30 in next version.
[...]
+ if (!of_property_read_u32(pdev-dev.of_node, vendor-hs200,
+ priv-vendor_hs200))
+ dev_info(dev, Applying vendor-hs200 setting\n);
+ else
+ priv-vendor_hs200 = 0;
This wasn't in the binding document, and a grep for vendor-hs200 in a
v3.16-rc2 tree found me nothing.
Please document this.
Yes, it is a setting for a vendor specific register.
I'll update it in next version.
It would be nice to know exactly what this is. We usually shy clear of
placing register values in dt. I can wait until the next posting if
you're goin to document that.
I'm thinking about removing this register value in dt.
I'll update it in next version.
+ if (!of_property_read_u32(pdev-dev.of_node, bus-width,
bus_width)) {
+ if (bus_width == 8) {
+ dev_info(dev, Applying 8 bit bus width\n);
+ host-mmc-caps |= MMC_CAP_8_BIT_DATA;
+ }
+ }
What if bus-width is not 8, or is not present?
In both cases, it will not touch host-mmc-caps at all. Then
sdhci_add_host()
will handle it and set MMC_CAP_4_BIT_DATA as default:
[...]
/*
* A controller may support 8-bit width, but the board itself
* might not have the pins brought out. Boards that support
* 8-bit width must set mmc-caps |= MMC_CAP_8_BIT_DATA; in
* their platform code before calling sdhci_add_host(), and we
* won't assume 8-bit width for hosts without that CAP.
*/
if (!(host-quirks SDHCI_QUIRK_FORCE_1_BIT_DATA))
mmc-caps |= MMC_CAP_4_BIT_DATA;
Ok, but does it make sense for a dts to have:
bus-width = 1;
If so, we should presumably do something.
If not, we should at least print a warning that the dtb doesn't make
sense.
I'll print a warning for invalid values in next version.
Thanks a lot for your review!
Best regards,
Vincent Yang
Cheers,
Mark.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH v2 4/6] mmc: sdhci: host: add new f_sdh30
On Fri, Jun 27, 2014 at 04:32:21AM +0100, Vincent Yang wrote:
2014-06-26 19:03 GMT+08:00 Mark Rutland mark.rutl...@arm.com:
On Thu, Jun 26, 2014 at 07:23:30AM +0100, Vincent Yang wrote:
This patch adds new host controller driver for
Fujitsu SDHCI controller f_sdh30.
Signed-off-by: Vincent Yang vincent.y...@tw.fujitsu.com
---
.../devicetree/bindings/mmc/sdhci-fujitsu.txt | 35 +++
drivers/mmc/host/Kconfig | 7 +
drivers/mmc/host/Makefile | 1 +
drivers/mmc/host/sdhci_f_sdh30.c | 346
+
drivers/mmc/host/sdhci_f_sdh30.h | 40 +++
5 files changed, 429 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt
create mode 100644 drivers/mmc/host/sdhci_f_sdh30.c
create mode 100644 drivers/mmc/host/sdhci_f_sdh30.h
diff --git a/Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt
b/Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt
new file mode 100644
index 000..40add438
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt
@@ -0,0 +1,35 @@
+* Fujitsu SDHCI controller
+
+This file documents differences between the core properties in mmc.txt
+and the properties used by the sdhci_f_sdh30 driver.
+
+Required properties:
+- compatible: fujitsu,f_sdh30
Please use '-' rather than '_' in compatible strings.
Hi Mark,
Yes, I'll update it to '-' in next version.
This seems to be the name of the driver. What is the precise name of the
IP block?
The name of the IP block is F_SDH30.
That's why it uses fujitsu,f_sdh30
Hmm. I'd still be tempted to use fujitsu,f-sdh30.
[...]
+ if (!of_property_read_u32(pdev-dev.of_node, vendor-hs200,
+ priv-vendor_hs200))
+ dev_info(dev, Applying vendor-hs200 setting\n);
+ else
+ priv-vendor_hs200 = 0;
This wasn't in the binding document, and a grep for vendor-hs200 in a
v3.16-rc2 tree found me nothing.
Please document this.
Yes, it is a setting for a vendor specific register.
I'll update it in next version.
It would be nice to know exactly what this is. We usually shy clear of
placing register values in dt. I can wait until the next posting if
you're goin to document that.
+ if (!of_property_read_u32(pdev-dev.of_node, bus-width,
bus_width)) {
+ if (bus_width == 8) {
+ dev_info(dev, Applying 8 bit bus width\n);
+ host-mmc-caps |= MMC_CAP_8_BIT_DATA;
+ }
+ }
What if bus-width is not 8, or is not present?
In both cases, it will not touch host-mmc-caps at all. Then sdhci_add_host()
will handle it and set MMC_CAP_4_BIT_DATA as default:
[...]
/*
* A controller may support 8-bit width, but the board itself
* might not have the pins brought out. Boards that support
* 8-bit width must set mmc-caps |= MMC_CAP_8_BIT_DATA; in
* their platform code before calling sdhci_add_host(), and we
* won't assume 8-bit width for hosts without that CAP.
*/
if (!(host-quirks SDHCI_QUIRK_FORCE_1_BIT_DATA))
mmc-caps |= MMC_CAP_4_BIT_DATA;
Ok, but does it make sense for a dts to have:
bus-width = 1;
If so, we should presumably do something.
If not, we should at least print a warning that the dtb doesn't make
sense.
Cheers,
Mark.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH v2 4/6] mmc: sdhci: host: add new f_sdh30
On Thu, Jun 26, 2014 at 07:23:30AM +0100, Vincent Yang wrote:
This patch adds new host controller driver for
Fujitsu SDHCI controller f_sdh30.
Signed-off-by: Vincent Yang vincent.y...@tw.fujitsu.com
---
.../devicetree/bindings/mmc/sdhci-fujitsu.txt | 35 +++
drivers/mmc/host/Kconfig | 7 +
drivers/mmc/host/Makefile | 1 +
drivers/mmc/host/sdhci_f_sdh30.c | 346
+
drivers/mmc/host/sdhci_f_sdh30.h | 40 +++
5 files changed, 429 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt
create mode 100644 drivers/mmc/host/sdhci_f_sdh30.c
create mode 100644 drivers/mmc/host/sdhci_f_sdh30.h
diff --git a/Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt
b/Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt
new file mode 100644
index 000..40add438
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt
@@ -0,0 +1,35 @@
+* Fujitsu SDHCI controller
+
+This file documents differences between the core properties in mmc.txt
+and the properties used by the sdhci_f_sdh30 driver.
+
+Required properties:
+- compatible: fujitsu,f_sdh30
Please use '-' rather than '_' in compatible strings.
This seems to be the name of the driver. What is the precise name of the
IP block?
+- voltage-ranges : two cells are required, first cell specifies minimum
+ slot voltage (mV), second cell specifies maximum slot voltage (mV).
+ Several ranges could be specified.
Describe this as a list of pairs, it's confusing otherwise.
I'm not sure I follow what having multiple pairs implies.
+Optional properties:
+- gpios: This is one optional gpio for controlling a power mux which
+ switches between two power supplies. 3.3V is selected when gpio is high,
+ and 1.8V is selected when gpio is low. This voltage is used for signal
+ level.
Give this a more descriptive name, like power-mux-gpios. That will match
up with the style of cd-gpios and wp-gpios.
+- clocks: Must contain an entry for each entry in clock-names. It is a
+ list of phandles and clock-specifier pairs.
+ See ../clocks/clock-bindings.txt for details.
+- clock-names: Should contain the following two entries:
+ sd_sd4clk - clock primarily used for tuning process
+ sd_bclk - base clock for sdhci controller
+
+Example:
+
+ sdhci1: sdio@3660 {
+ compatible = fujitsu,f_sdh30;
+ reg = 0 0x3660 0x1000;
+ interrupts = 0 172 0x4,
+0 173 0x4;
+ voltage-ranges = 1800 1800 3300 3300;
Place brackets around each pair to make this clearer:
voltage-ranges = 1800 1800, 3300 3300;
[...]
+ if (!of_property_read_u32(pdev-dev.of_node, vendor-hs200,
+ priv-vendor_hs200))
+ dev_info(dev, Applying vendor-hs200 setting\n);
+ else
+ priv-vendor_hs200 = 0;
This wasn't in the binding document, and a grep for vendor-hs200 in a
v3.16-rc2 tree found me nothing.
Please document this.
+
+ if (!of_property_read_u32(pdev-dev.of_node, bus-width,
bus_width)) {
+ if (bus_width == 8) {
+ dev_info(dev, Applying 8 bit bus width\n);
+ host-mmc-caps |= MMC_CAP_8_BIT_DATA;
+ }
+ }
What if bus-width is not 8, or is not present?
+
+ ret = mmc_of_parse_voltage(pdev-dev.of_node, host-ocr_mask);
+ if (ret) {
+ dev_err(dev, %s: parse voltage error\n, __func__);
+ goto err_voltage;
+ }
+
+ host-hw_name = DRIVER_NAME;
+ host-ops = sdhci_f_sdh30_ops;
+ host-irq = irq;
+
+ host-ioaddr = of_iomap(pdev-dev.of_node, 0);
+ if (!host-ioaddr) {
+ dev_err(dev, %s: failed to remap registers\n, __func__);
+ ret = -ENXIO;
+ goto err_remap;
+ }
+
+ priv-clk_sd4 = of_clk_get(pdev-dev.of_node, 0);
+ if (!IS_ERR(priv-clk_sd4)) {
+ ret = clk_prepare_enable(priv-clk_sd4);
+ if (ret 0) {
+ dev_err(dev, Failed to enable sd4 clock: %d\n, ret);
+ goto err_clk1;
+ }
+ }
+ priv-clk_b = of_clk_get(pdev-dev.of_node, 1);
+ if (!IS_ERR(priv-clk_b)) {
+ ret = clk_prepare_enable(priv-clk_b);
+ if (ret 0) {
+ dev_err(dev, Failed to enable clk_b clock: %d\n,
ret);
+ goto err_clk2;
+ }
+ }
Given you gave these names, get these by name rather than index. It's
less surprising and more flexible.
Thanks,
Mark.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH v2 4/6] mmc: sdhci: host: add new f_sdh30
2014-06-26 19:03 GMT+08:00 Mark Rutland mark.rutl...@arm.com:
On Thu, Jun 26, 2014 at 07:23:30AM +0100, Vincent Yang wrote:
This patch adds new host controller driver for
Fujitsu SDHCI controller f_sdh30.
Signed-off-by: Vincent Yang vincent.y...@tw.fujitsu.com
---
.../devicetree/bindings/mmc/sdhci-fujitsu.txt | 35 +++
drivers/mmc/host/Kconfig | 7 +
drivers/mmc/host/Makefile | 1 +
drivers/mmc/host/sdhci_f_sdh30.c | 346
+
drivers/mmc/host/sdhci_f_sdh30.h | 40 +++
5 files changed, 429 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt
create mode 100644 drivers/mmc/host/sdhci_f_sdh30.c
create mode 100644 drivers/mmc/host/sdhci_f_sdh30.h
diff --git a/Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt
b/Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt
new file mode 100644
index 000..40add438
--- /dev/null
+++ b/Documentation/devicetree/bindings/mmc/sdhci-fujitsu.txt
@@ -0,0 +1,35 @@
+* Fujitsu SDHCI controller
+
+This file documents differences between the core properties in mmc.txt
+and the properties used by the sdhci_f_sdh30 driver.
+
+Required properties:
+- compatible: fujitsu,f_sdh30
Please use '-' rather than '_' in compatible strings.
Hi Mark,
Yes, I'll update it to '-' in next version.
This seems to be the name of the driver. What is the precise name of the
IP block?
The name of the IP block is F_SDH30.
That's why it uses fujitsu,f_sdh30
+- voltage-ranges : two cells are required, first cell specifies minimum
+ slot voltage (mV), second cell specifies maximum slot voltage (mV).
+ Several ranges could be specified.
Describe this as a list of pairs, it's confusing otherwise.
I'm not sure I follow what having multiple pairs implies.
Yes, I'll update it in next version.
+Optional properties:
+- gpios: This is one optional gpio for controlling a power mux which
+ switches between two power supplies. 3.3V is selected when gpio is high,
+ and 1.8V is selected when gpio is low. This voltage is used for signal
+ level.
Give this a more descriptive name, like power-mux-gpios. That will match
up with the style of cd-gpios and wp-gpios.
Yes, I'll update it in next version.
+- clocks: Must contain an entry for each entry in clock-names. It is a
+ list of phandles and clock-specifier pairs.
+ See ../clocks/clock-bindings.txt for details.
+- clock-names: Should contain the following two entries:
+ sd_sd4clk - clock primarily used for tuning process
+ sd_bclk - base clock for sdhci controller
+
+Example:
+
+ sdhci1: sdio@3660 {
+ compatible = fujitsu,f_sdh30;
+ reg = 0 0x3660 0x1000;
+ interrupts = 0 172 0x4,
+0 173 0x4;
+ voltage-ranges = 1800 1800 3300 3300;
Place brackets around each pair to make this clearer:
voltage-ranges = 1800 1800, 3300 3300;
Yes, I'll update it in next version.
[...]
+ if (!of_property_read_u32(pdev-dev.of_node, vendor-hs200,
+ priv-vendor_hs200))
+ dev_info(dev, Applying vendor-hs200 setting\n);
+ else
+ priv-vendor_hs200 = 0;
This wasn't in the binding document, and a grep for vendor-hs200 in a
v3.16-rc2 tree found me nothing.
Please document this.
Yes, it is a setting for a vendor specific register.
I'll update it in next version.
+
+ if (!of_property_read_u32(pdev-dev.of_node, bus-width,
bus_width)) {
+ if (bus_width == 8) {
+ dev_info(dev, Applying 8 bit bus width\n);
+ host-mmc-caps |= MMC_CAP_8_BIT_DATA;
+ }
+ }
What if bus-width is not 8, or is not present?
In both cases, it will not touch host-mmc-caps at all. Then sdhci_add_host()
will handle it and set MMC_CAP_4_BIT_DATA as default:
[...]
/*
* A controller may support 8-bit width, but the board itself
* might not have the pins brought out. Boards that support
* 8-bit width must set mmc-caps |= MMC_CAP_8_BIT_DATA; in
* their platform code before calling sdhci_add_host(), and we
* won't assume 8-bit width for hosts without that CAP.
*/
if (!(host-quirks SDHCI_QUIRK_FORCE_1_BIT_DATA))
mmc-caps |= MMC_CAP_4_BIT_DATA;
[...]
+
+ ret = mmc_of_parse_voltage(pdev-dev.of_node, host-ocr_mask);
+ if (ret) {
+ dev_err(dev, %s: parse voltage error\n, __func__);
+ goto err_voltage;
+ }
+
+ host-hw_name = DRIVER_NAME;
+ host-ops = sdhci_f_sdh30_ops;
+ host-irq = irq;
+
+ host-ioaddr = of_iomap(pdev-dev.of_node, 0);
+ if (!host-ioaddr) {
+ dev_err(dev, %s: failed to remap registers\n, __func__);
+ ret = -ENXIO;
+ goto err_remap;
+ }
+
