diff mbox

[04/20] mmc: mmci: Move signal directions bits into DT include file

Message ID 1395404057-27835-5-git-send-email-ulf.hansson@linaro.org
State New
Headers show

Commit Message

Ulf Hansson March 21, 2014, 12:14 p.m. UTC 
These bits is currently used from platform data, but will be needed
from DT as well, so let's make them available.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 include/dt-bindings/mmc/mmci.h |   21 +++++++++++++++++++++
 include/linux/amba/mmci.h      |   14 +-------------
 2 files changed, 22 insertions(+), 13 deletions(-)
 create mode 100644 include/dt-bindings/mmc/mmci.h

Comments

Linus Walleij March 25, 2014, 9:22 p.m. UTC | #1 
On Fri, Mar 21, 2014 at 1:14 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:

> These bits is currently used from platform data, but will be needed
> from DT as well, so let's make them available.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
(...)
> +#define MCI_ST_DATA31DIREN     (1 << 5)
> +#define MCI_ST_FBCLKEN         (1 << 7)
> +#define MCI_ST_DATA74DIREN     (1 << 8)

Isn't MCI_ST_FBCLKEN = feedback clock enable and nothing
to do with directions really?

Or can you convince me ...

Maybe we should just have single strings for these
things in the binding instead, like:

@@ -118,6 +119,10 @@
                        bus-width = <4>;
                        mmc-cap-sd-highspeed;
                        mmc-cap-mmc-highspeed;
+                       st,signal-direction-data2;
+                       st,signal-direction-cmd;
+                       st,signal-direction-data0;
+                       st,feedback-clock-enable;
                        vmmc-supply = <&ab8500_ldo_aux3_reg>;
                        vqmmc-supply = <&vmmci>;
                        pinctrl-names = "default", "sleep";

And for FBCLKEN is that really something that should
be configured... in the device tree after all?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson March 25, 2014, 11:05 p.m. UTC | #2 
On 25 March 2014 22:22, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Mar 21, 2014 at 1:14 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
>> These bits is currently used from platform data, but will be needed
>> from DT as well, so let's make them available.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> (...)
>> +#define MCI_ST_DATA31DIREN     (1 << 5)
>> +#define MCI_ST_FBCLKEN         (1 << 7)
>> +#define MCI_ST_DATA74DIREN     (1 << 8)
>
> Isn't MCI_ST_FBCLKEN = feedback clock enable and nothing
> to do with directions really?

You do have a point here, but then we are about to go into the details. :-)

What MCI_ST_FBCLKEN means, is that the internal logic in the primecell
can decide whether to use the signal from clockout pin or the
feebackclock pin, when its latching incoming signals on the data bus.

That said, we for sure need a dt binding for bit as well, since this
is not possible to figure out how pins are routed in runtime.

Also, I do believe it is very closely related to the other sigdir
pins, since it's all about how pins is being routed, even if it's as
you say - in this case a matter of signal directions.

So, I guess we have two options here; Either move the MCI_ST_FBCLKEN
out of the signal-direction binding and invent a separate binding for
it, or just keep them as is.

>
> Or can you convince me ...
>
> Maybe we should just have single strings for these
> things in the binding instead, like:
>
> @@ -118,6 +119,10 @@
>                         bus-width = <4>;
>                         mmc-cap-sd-highspeed;
>                         mmc-cap-mmc-highspeed;
> +                       st,signal-direction-data2;
> +                       st,signal-direction-cmd;
> +                       st,signal-direction-data0;
> +                       st,feedback-clock-enable;
>                         vmmc-supply = <&ab8500_ldo_aux3_reg>;
>                         vqmmc-supply = <&vmmci>;
>                         pinctrl-names = "default", "sleep";

I guess this is an option. To me it just seems a bit silly to have one
separate binding for each single bit, but maybe it becomes clearer?

>
> And for FBCLKEN is that really something that should
> be configured... in the device tree after all?

We need a DT binding, see above.

>
> Yours,
> Linus Walleij

Thanks for reviewing.

Please tell me your ideas on how to proceed, I happy to change to
whatever you like. :-)

Kind regards
Ulf Hansson
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij March 28, 2014, 9:03 p.m. UTC | #3 
On Wed, Mar 26, 2014 at 12:05 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 25 March 2014 22:22, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Fri, Mar 21, 2014 at 1:14 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>
>>> These bits is currently used from platform data, but will be needed
>>> from DT as well, so let's make them available.
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> (...)
>>> +#define MCI_ST_DATA31DIREN     (1 << 5)
>>> +#define MCI_ST_FBCLKEN         (1 << 7)
>>> +#define MCI_ST_DATA74DIREN     (1 << 8)
>>
>> Isn't MCI_ST_FBCLKEN = feedback clock enable and nothing
>> to do with directions really?
>
> You do have a point here, but then we are about to go into the details. :-)
>
> What MCI_ST_FBCLKEN means, is that the internal logic in the primecell
> can decide whether to use the signal from clockout pin or the
> feebackclock pin, when its latching incoming signals on the data bus.
>
> That said, we for sure need a dt binding for bit as well, since this
> is not possible to figure out how pins are routed in runtime.
>
> Also, I do believe it is very closely related to the other sigdir
> pins, since it's all about how pins is being routed, even if it's as
> you say - in this case a matter of signal directions.
>
> So, I guess we have two options here; Either move the MCI_ST_FBCLKEN
> out of the signal-direction binding and invent a separate binding for
> it, or just keep them as is.

If they are very closely related I guess that will become very
clear when you document it in the requested explicit bindings,
so no strong opinion here. I'd probably ACK either version.

>> Maybe we should just have single strings for these
>> things in the binding instead, like:
>>
>> @@ -118,6 +119,10 @@
>>                         bus-width = <4>;
>>                         mmc-cap-sd-highspeed;
>>                         mmc-cap-mmc-highspeed;
>> +                       st,signal-direction-data2;
>> +                       st,signal-direction-cmd;
>> +                       st,signal-direction-data0;
>> +                       st,feedback-clock-enable;
>>                         vmmc-supply = <&ab8500_ldo_aux3_reg>;
>>                         vqmmc-supply = <&vmmci>;
>>                         pinctrl-names = "default", "sleep";
>
> I guess this is an option. To me it just seems a bit silly to have one
> separate binding for each single bit, but maybe it becomes clearer?

The idea with device trees is to be very human-readable, so I guess
if you have preprocessor macro things |:ing the arguments together
to a singular 32bit value it serves the same purpose. Arguably this
version is even easier for a human to read though.

What we should avoid is using magic number assignments to
save space (e.g. have magic numbers in the tree rather than
readable strings) so I lean toward this, but it's admittedly in a
grey area, so again no strong opinion.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson March 31, 2014, 1:56 p.m. UTC | #4 
On 28 March 2014 22:03, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Mar 26, 2014 at 12:05 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 25 March 2014 22:22, Linus Walleij <linus.walleij@linaro.org> wrote:
>>> On Fri, Mar 21, 2014 at 1:14 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>
>>>> These bits is currently used from platform data, but will be needed
>>>> from DT as well, so let's make them available.
>>>>
>>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> (...)
>>>> +#define MCI_ST_DATA31DIREN     (1 << 5)
>>>> +#define MCI_ST_FBCLKEN         (1 << 7)
>>>> +#define MCI_ST_DATA74DIREN     (1 << 8)
>>>
>>> Isn't MCI_ST_FBCLKEN = feedback clock enable and nothing
>>> to do with directions really?
>>
>> You do have a point here, but then we are about to go into the details. :-)
>>
>> What MCI_ST_FBCLKEN means, is that the internal logic in the primecell
>> can decide whether to use the signal from clockout pin or the
>> feebackclock pin, when its latching incoming signals on the data bus.
>>
>> That said, we for sure need a dt binding for bit as well, since this
>> is not possible to figure out how pins are routed in runtime.
>>
>> Also, I do believe it is very closely related to the other sigdir
>> pins, since it's all about how pins is being routed, even if it's as
>> you say - in this case a matter of signal directions.
>>
>> So, I guess we have two options here; Either move the MCI_ST_FBCLKEN
>> out of the signal-direction binding and invent a separate binding for
>> it, or just keep them as is.
>
> If they are very closely related I guess that will become very
> clear when you document it in the requested explicit bindings,
> so no strong opinion here. I'd probably ACK either version.
>
>>> Maybe we should just have single strings for these
>>> things in the binding instead, like:
>>>
>>> @@ -118,6 +119,10 @@
>>>                         bus-width = <4>;
>>>                         mmc-cap-sd-highspeed;
>>>                         mmc-cap-mmc-highspeed;
>>> +                       st,signal-direction-data2;
>>> +                       st,signal-direction-cmd;
>>> +                       st,signal-direction-data0;
>>> +                       st,feedback-clock-enable;
>>>                         vmmc-supply = <&ab8500_ldo_aux3_reg>;
>>>                         vqmmc-supply = <&vmmci>;
>>>                         pinctrl-names = "default", "sleep";
>>
>> I guess this is an option. To me it just seems a bit silly to have one
>> separate binding for each single bit, but maybe it becomes clearer?
>
> The idea with device trees is to be very human-readable, so I guess
> if you have preprocessor macro things |:ing the arguments together
> to a singular 32bit value it serves the same purpose. Arguably this
> version is even easier for a human to read though.
>
> What we should avoid is using magic number assignments to
> save space (e.g. have magic numbers in the tree rather than
> readable strings) so I lean toward this, but it's admittedly in a
> grey area, so again no strong opinion.

Thanks for your advise. I seems like a good idea to move to use
readable strings instead.

In that way, we also get the clean cut between signal directions and
the feedback clock pin.

Kind regards
Ulf Hansson

>
> Yours,
> Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/include/dt-bindings/mmc/mmci.h b/include/dt-bindings/mmc/mmci.h
new file mode 100644
index 0000000..8c55254
--- /dev/null
+++ b/include/dt-bindings/mmc/mmci.h
@@ -0,0 +1,21 @@ 
+/*
+ * This header provides constants for the mmci bindings.
+ *
+ */
+
+#ifndef _DT_BINDINGS_MMC_MMCI_H
+#define _DT_BINDINGS_MMC_MMCI_H
+
+/*
+ * Bus signal direction bits.
+ * The ST Micro version does not have ROD and reuse the voltage registers
+ * for direction settings.
+ */
+#define MCI_ST_DATA2DIREN	(1 << 2)
+#define MCI_ST_CMDDIREN		(1 << 3)
+#define MCI_ST_DATA0DIREN	(1 << 4)
+#define MCI_ST_DATA31DIREN	(1 << 5)
+#define MCI_ST_FBCLKEN		(1 << 7)
+#define MCI_ST_DATA74DIREN	(1 << 8)
+
+#endif
diff --git a/include/linux/amba/mmci.h b/include/linux/amba/mmci.h
index 32a89cf..47ff176 100644
--- a/include/linux/amba/mmci.h
+++ b/include/linux/amba/mmci.h
@@ -5,19 +5,7 @@ 
 #define AMBA_MMCI_H
 
 #include <linux/mmc/host.h>
-
-
-/*
- * These defines is places here due to access is needed from machine
- * configuration files. The ST Micro version does not have ROD and
- * reuse the voltage registers for direction settings.
- */
-#define MCI_ST_DATA2DIREN	(1 << 2)
-#define MCI_ST_CMDDIREN		(1 << 3)
-#define MCI_ST_DATA0DIREN	(1 << 4)
-#define MCI_ST_DATA31DIREN	(1 << 5)
-#define MCI_ST_FBCLKEN		(1 << 7)
-#define MCI_ST_DATA74DIREN	(1 << 8)
+#include <dt-bindings/mmc/mmci.h>
 
 /* Just some dummy forwarding */
 struct dma_chan;