[PATCH v2 0/4] ata: libahci: Allow using a regulator for each port

[PATCH v2 0/4] ata: libahci: Allow using a regulator for each port

[Gregory CLEMENT]

Hi,

The current implementation of the libahci allows using one PHY per
port but we still have one single regulator for the whole
controller. This series adds the support of multiple regulators.

This is the second version of the series.

The previous version depended of a patch set for the regulator
framework which was not accepted. Then I needed to do this in a
different way that it was done for the PHY. As result the regulator
need to be associated to each port as the host node level using the
property target-port<n>-supply, where <n> is the port number. But it
is however mandatory to have one port sub-node for each port
associated to a regulator.

Since the 1st version I also took into account the comments from Hans
and added a patch to use the new binding for the device tree of the
Armada 38x GP. This last patch should be merged through the mvebu
tree.

Thanks,

Gr??gory

Changelog:

 v1 -> v2:
- Kept the case when no child node are present under the ahci node
- Fix the test done under the label disable_target_pwrs
- No more use an of_ version of the regulator framework and instead
  associate each regulator of a port with an unique name.
- Added the acked-by on the clean-up patch

Gregory CLEMENT (4):
  ata: libahci: Clean-up the ahci_platform_en/disable_phys functions
  Documentation: bindings: Add the regulator property to the sub-nodes
    AHCI bindings
  ata: libahci: Allow using multiple regulators
  ARM: mvebu: Armada 385 GP: Add regulators to the SATA port

 .../devicetree/bindings/ata/ahci-platform.txt      |   9 +-
 arch/arm/boot/dts/armada-388-gp.dts                | 126 ++++++++++++
 drivers/ata/ahci.h                                 |   2 +-
 drivers/ata/ahci_imx.c                             |  14 +-
 drivers/ata/libahci_platform.c                     | 224 ++++++++++++++-------
 include/linux/ahci_platform.h                      |   2 +
 6 files changed, 294 insertions(+), 83 deletions(-)

-- 
1.9.1

[PATCH v2 1/4] ata: libahci: Clean-up the ahci_platform_en/disable_phys functions

[Gregory CLEMENT]

The phy_ functions handle the NULL pointer case, so there is no need
to skip them if there is a NULL pointer. Moreover, after the error
label there is already no check on the pointer. This patch removes the
unnecessary tests and brings some consistency.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
Acked-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/ata/libahci_platform.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index 0b03f9056692..a147aaadca85 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -54,9 +54,6 @@ static int ahci_platform_enable_phys(struct ahci_host_priv *hpriv)
 	int rc, i;
 
 	for (i = 0; i < hpriv->nports; i++) {
-		if (!hpriv->phys[i])
-			continue;
-
 		rc = phy_init(hpriv->phys[i]);
 		if (rc)
 			goto disable_phys;
@@ -89,9 +86,6 @@ static void ahci_platform_disable_phys(struct ahci_host_priv *hpriv)
 	int i;
 
 	for (i = 0; i < hpriv->nports; i++) {
-		if (!hpriv->phys[i])
-			continue;
-
 		phy_power_off(hpriv->phys[i]);
 		phy_exit(hpriv->phys[i]);
 	}
-- 
1.9.1

[PATCH v2 2/4] Documentation: bindings: Add the regulator property to the sub-nodes AHCI bindings

[Gregory CLEMENT]

It is now possible to use a regulator property for each port of the
AHCI controller.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 Documentation/devicetree/bindings/ata/ahci-platform.txt | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt
index 4ab09f2202d4..87416d71c758 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.txt
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt
@@ -37,9 +37,10 @@ Required properties when using sub-nodes:
 
 
 Sub-nodes required properties:
-- reg               : the port number
-- phys              : reference to the SATA PHY node
-
+- reg					: the port number
+And at least one of the following properties:
+- phys              	: reference to the SATA PHY node
+- target-port<n>-supply	: regulator for SATA target power for port <n>
 
 Examples:
         sata at ffe08000 {
@@ -68,10 +69,12 @@ With sub-nodes:
 		sata0: sata-port at 0 {
 			reg = <0>;
 			phys = <&sata_phy 0>;
+			target-port0-supply = <&reg_sata0>;
 		};
 
 		sata1: sata-port at 1 {
 			reg = <1>;
 			phys = <&sata_phy 1>;
+			target-port1-supply = <&reg_sata1>;;
 		};
 	};
-- 
1.9.1

[PATCH v2 2/4] Documentation: bindings: Add the regulator property to the sub-nodes AHCI bindings

[Hans de Goede]

Hi,

On 09-01-15 11:39, Gregory CLEMENT wrote:
> It is now possible to use a regulator property for each port of the
> AHCI controller.
>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>   Documentation/devicetree/bindings/ata/ahci-platform.txt | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt
> index 4ab09f2202d4..87416d71c758 100644
> --- a/Documentation/devicetree/bindings/ata/ahci-platform.txt
> +++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt
> @@ -37,9 +37,10 @@ Required properties when using sub-nodes:
>
>
>   Sub-nodes required properties:
> -- reg               : the port number
> -- phys              : reference to the SATA PHY node
> -
> +- reg					: the port number
> +And at least one of the following properties:
> +- phys              	: reference to the SATA PHY node
> +- target-port<n>-supply	: regulator for SATA target power for port <n>
>
>   Examples:
>           sata at ffe08000 {
> @@ -68,10 +69,12 @@ With sub-nodes:
>   		sata0: sata-port at 0 {
>   			reg = <0>;
>   			phys = <&sata_phy 0>;
> +			target-port0-supply = <&reg_sata0>;
>   		};
>

I'm sorry, but I've to NAK this, I did not follow the regulator discussion.,
thinking that it was just about the use of some utility function vs diy or
some such, I had no idea this would impact the dt-binding.

The -port%d- bit is completely unnecessary from a dt pov. Devicetree is
supposed to be OS agnostic, we cannot make the dt-binding ugly just because
the regulator subsys in Linux does not provide the necessary functionality.

I'm afraid you will have to get back to the regulator subsys people and tell
them that we really need an of_regulator_get, so that we can tell the
regulator subsys we want a regulator from a specific of_node.

Please CC me when you re-open the discussion with the regulator people.

Regards,

Hans

[PATCH v2 2/4] Documentation: bindings: Add the regulator property to the sub-nodes AHCI bindings

[Gregory CLEMENT]

Hi Hans,

On 09/01/2015 16:46, Hans de Goede wrote:
> Hi,
> 
> On 09-01-15 11:39, Gregory CLEMENT wrote:
>> It is now possible to use a regulator property for each port of the
>> AHCI controller.
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> ---
>>   Documentation/devicetree/bindings/ata/ahci-platform.txt | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt
>> index 4ab09f2202d4..87416d71c758 100644
>> --- a/Documentation/devicetree/bindings/ata/ahci-platform.txt
>> +++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt
>> @@ -37,9 +37,10 @@ Required properties when using sub-nodes:
>>
>>
>>   Sub-nodes required properties:
>> -- reg               : the port number
>> -- phys              : reference to the SATA PHY node
>> -
>> +- reg					: the port number
>> +And at least one of the following properties:
>> +- phys              	: reference to the SATA PHY node
>> +- target-port<n>-supply	: regulator for SATA target power for port <n>
>>
>>   Examples:
>>           sata at ffe08000 {
>> @@ -68,10 +69,12 @@ With sub-nodes:
>>   		sata0: sata-port at 0 {
>>   			reg = <0>;
>>   			phys = <&sata_phy 0>;
>> +			target-port0-supply = <&reg_sata0>;
>>   		};
>>
> 
> I'm sorry, but I've to NAK this, I did not follow the regulator discussion.,
> thinking that it was just about the use of some utility function vs diy or
> some such, I had no idea this would impact the dt-binding.
> 
> The -port%d- bit is completely unnecessary from a dt pov. Devicetree is
> supposed to be OS agnostic, we cannot make the dt-binding ugly just because
> the regulator subsys in Linux does not provide the necessary functionality.
> 
> I'm afraid you will have to get back to the regulator subsys people and tell
> them that we really need an of_regulator_get, so that we can tell the
> regulator subsys we want a regulator from a specific of_node.

I didn't cc you especially, but I cc the linux-ide mailing list. The
email was "[PATCH 2/2] regulator: core: Add the device tree version to
the regulator_get family" (see
http://thread.gmane.org/gmane.linux.kernel/1856154).

Mark Brown didn't want a firmware specific interface. The regulator
framework allows associating only a regulator to a device and
currently, at least from the point of view of the device tree, the
port subnodes are not devices. He suggested making the child node
device. However, when I had a look on it, it didn't seem an easy
task. As you know better this code, what do you think of it?

(I added Mark Bronw in CC of this email so he can add more explanation
if needed)


Thanks,

Gregory

> 
> Please CC me when you re-open the discussion with the regulator people.
> 
> Regards,
> 
> Hans
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH v2 2/4] Documentation: bindings: Add the regulator property to the sub-nodes AHCI bindings

[Chen-Yu Tsai]

Hi,

On Sat, Jan 10, 2015 at 12:20 AM, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:
> Hi Hans,
>
> On 09/01/2015 16:46, Hans de Goede wrote:
>> Hi,
>>
>> On 09-01-15 11:39, Gregory CLEMENT wrote:
>>> It is now possible to use a regulator property for each port of the
>>> AHCI controller.
>>>
>>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>>> ---
>>>   Documentation/devicetree/bindings/ata/ahci-platform.txt | 9 ++++++---
>>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt
>>> index 4ab09f2202d4..87416d71c758 100644
>>> --- a/Documentation/devicetree/bindings/ata/ahci-platform.txt
>>> +++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt
>>> @@ -37,9 +37,10 @@ Required properties when using sub-nodes:
>>>
>>>
>>>   Sub-nodes required properties:
>>> -- reg               : the port number
>>> -- phys              : reference to the SATA PHY node
>>> -
>>> +- reg                                       : the port number
>>> +And at least one of the following properties:
>>> +- phys                      : reference to the SATA PHY node
>>> +- target-port<n>-supply     : regulator for SATA target power for port <n>
>>>
>>>   Examples:
>>>           sata at ffe08000 {
>>> @@ -68,10 +69,12 @@ With sub-nodes:
>>>              sata0: sata-port at 0 {
>>>                      reg = <0>;
>>>                      phys = <&sata_phy 0>;
>>> +                    target-port0-supply = <&reg_sata0>;
>>>              };
>>>
>>
>> I'm sorry, but I've to NAK this, I did not follow the regulator discussion.,
>> thinking that it was just about the use of some utility function vs diy or
>> some such, I had no idea this would impact the dt-binding.
>>
>> The -port%d- bit is completely unnecessary from a dt pov. Devicetree is
>> supposed to be OS agnostic, we cannot make the dt-binding ugly just because
>> the regulator subsys in Linux does not provide the necessary functionality.
>>
>> I'm afraid you will have to get back to the regulator subsys people and tell
>> them that we really need an of_regulator_get, so that we can tell the
>> regulator subsys we want a regulator from a specific of_node.
>
> I didn't cc you especially, but I cc the linux-ide mailing list. The
> email was "[PATCH 2/2] regulator: core: Add the device tree version to
> the regulator_get family" (see
> http://thread.gmane.org/gmane.linux.kernel/1856154).
>
> Mark Brown didn't want a firmware specific interface. The regulator
> framework allows associating only a regulator to a device and
> currently, at least from the point of view of the device tree, the
> port subnodes are not devices. He suggested making the child node
> device. However, when I had a look on it, it didn't seem an easy
> task. As you know better this code, what do you think of it?
>
> (I added Mark Bronw in CC of this email so he can add more explanation
> if needed)

Since the AHCI library code already supports the generic phy subsystem,
with one phy possible for each port node, you could possibly add a
generic phy that just takes a regulator, and hook it up that way.

I don't know if the extra layer of indirection is good or not.
Just offering an alternative.


Regards,
ChenYu

>>
>> Please CC me when you re-open the discussion with the regulator people.
>>
>> Regards,
>>
>> Hans
>>
>
>
> --
> Gregory Clement, Free Electrons
> Kernel, drivers, real-time and embedded Linux
> development, consulting, training and support.
> http://free-electrons.com
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/4] Documentation: bindings: Add the regulator property to the sub-nodes AHCI bindings

[Mark Brown]

On Sat, Jan 10, 2015 at 12:29:32AM +0800, Chen-Yu Tsai wrote:

> Since the AHCI library code already supports the generic phy subsystem,
> with one phy possible for each port node, you could possibly add a
> generic phy that just takes a regulator, and hook it up that way.

> I don't know if the extra layer of indirection is good or not.
> Just offering an alternative.

Or if the supply is for the device at the other end of the link (which
is what it sounded like) then use that.  This just sounds like the same
problem we have for all the enumerable buses in embedded systems where
we need to be able to understand that the device exists prior to it
being fully ready to appear in the system.  Having the link/slot be a
device in Linux does indeed seem to be a common way people think about
doing this, it sounds like for this one it might be the most direct.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150109/e840d768/attachment.sig>

[PATCH v2 2/4] Documentation: bindings: Add the regulator property to the sub-nodes AHCI bindings

[Hans de Goede]

Hi,

On 09-01-15 18:11, Mark Brown wrote:
> On Sat, Jan 10, 2015 at 12:29:32AM +0800, Chen-Yu Tsai wrote:
>
>> Since the AHCI library code already supports the generic phy subsystem,
>> with one phy possible for each port node, you could possibly add a
>> generic phy that just takes a regulator, and hook it up that way.
>
>> I don't know if the extra layer of indirection is good or not.
>> Just offering an alternative.
>
> Or if the supply is for the device at the other end of the link (which
> is what it sounded like) then use that.  This just sounds like the same
> problem we have for all the enumerable buses in embedded systems where
> we need to be able to understand that the device exists prior to it
> being fully ready to appear in the system.  Having the link/slot be a
> device in Linux does indeed seem to be a common way people think about
> doing this, it sounds like for this one it might be the most direct.

I think we should be careful to not think too much from an implementation
pov here, the purpose of the devicetree description is to describe the hardware,
as is.

If I understand things correctly then the board Gregory is working on has
pairs of sata + satapower connectors on it, and what is on the other side
if anything is unknown, as such the proposed device tree description of
having a ahci controller node with port child nodes, with each port having
a regulator:

         sata at f7e90000 {
                 compatible = "marvell,berlin2q-achi", "generic-ahci";
                 reg = <0xe90000 0x1000>;
                 interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
                 clocks = <&chip CLKID_SATA>;
                 #address-cells = <1>;
                 #size-cells = <0>;

                 sata0: sata-port at 0 {
                         reg = <0>;
                         phys = <&sata_phy 0>;
			target-supply = <&reg_sata0>;
                 };

                 sata1: sata-port at 1 {
                         reg = <1>;
                         phys = <&sata_phy 1>;
			target-supply = <&reg_sata1>;
                 };
         };

Seems to match the hardware pretty exactly, and also matches how we've
been describing similar devices with only one sata port + power plug sofar,
so from a consistency pov it also is a good model.

So model wise I believe the above to be pretty good, and getting the modeling
right is the most important thing with devicetree, since it is an ABI which
once deployed is set in stone. Also is is os agnostic, and is also used by the
BSD-s etc, so implementation details should explicitly NOT be taken into
account when doing the modeling.

So once we come to the conclusion that the above model is the right model,
then the question becomes how to implement support for this, and this becomes
purely a Linux *internal* API discussion, *but* the model comes first!

So supporting this model requires having a regulator_get API which allows
specifying which of_node to get the regulator from, e.g. the proposed
of_regulator_get function. I know you (Mark) do not like this, but all
other subsystems have an of_foo_get function taking an of_node as argument,
I do not see how the regulator subsys is so special that it cannot have one,
and also notice that this is only a kernel internal API which we can always
change later.

Regards,

Hans

[PATCH v2 2/4] Documentation: bindings: Add the regulator property to the sub-nodes AHCI bindings

[Mark Brown]

On Sat, Jan 10, 2015 at 11:20:13AM +0100, Hans de Goede wrote:
> On 09-01-15 18:11, Mark Brown wrote:

> >Or if the supply is for the device at the other end of the link (which
> >is what it sounded like) then use that.  This just sounds like the same
> >problem we have for all the enumerable buses in embedded systems where
> >we need to be able to understand that the device exists prior to it
> >being fully ready to appear in the system.  Having the link/slot be a
> >device in Linux does indeed seem to be a common way people think about
> >doing this, it sounds like for this one it might be the most direct.

> I think we should be careful to not think too much from an implementation
> pov here, the purpose of the devicetree description is to describe the hardware,
> as is.

I don't think anyone is talking about changing the DT here, only the way
it's represented inside Linux.

>                 sata0: sata-port at 0 {
>                         reg = <0>;
>                         phys = <&sata_phy 0>;
> 			target-supply = <&reg_sata0>;
>                 };
> 
>                 sata1: sata-port at 1 {
>                         reg = <1>;
>                         phys = <&sata_phy 1>;
> 			target-supply = <&reg_sata1>;
>                 };
>         };

> Seems to match the hardware pretty exactly, and also matches how we've
> been describing similar devices with only one sata port + power plug sofar,
> so from a consistency pov it also is a good model.

Right, I think that makes sense - it also looks to me like these things
should be representable as devices within Linux.

> So supporting this model requires having a regulator_get API which allows
> specifying which of_node to get the regulator from, e.g. the proposed

It requires nothing of the sort.

> of_regulator_get function. I know you (Mark) do not like this, but all
> other subsystems have an of_foo_get function taking an of_node as argument,
> I do not see how the regulator subsys is so special that it cannot have one,
> and also notice that this is only a kernel internal API which we can always
> change later.

Two things there.  One is that mostly those APIs are legacy APIs from
before we made DT a first class citizen and generally they shouldn't be
used these days.  The other is that at least for regulators we have
constant problems with people abusing the API in various ways, as a
result the API has a goal of not helping undesirable usage patterns in
order to help people spot problems.  Having an API which makes it easy
create broken bindings means that it is much more likely that people
will do just that.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150110/70c1fc6a/attachment.sig>

[PATCH v2 2/4] Documentation: bindings: Add the regulator property to the sub-nodes AHCI bindings

[Hans de Goede]

Hi,

On 10-01-15 12:17, Mark Brown wrote:
> On Sat, Jan 10, 2015 at 11:20:13AM +0100, Hans de Goede wrote:
>> On 09-01-15 18:11, Mark Brown wrote:
>
>>> Or if the supply is for the device at the other end of the link (which
>>> is what it sounded like) then use that.  This just sounds like the same
>>> problem we have for all the enumerable buses in embedded systems where
>>> we need to be able to understand that the device exists prior to it
>>> being fully ready to appear in the system.  Having the link/slot be a
>>> device in Linux does indeed seem to be a common way people think about
>>> doing this, it sounds like for this one it might be the most direct.
>
>> I think we should be careful to not think too much from an implementation
>> pov here, the purpose of the devicetree description is to describe the hardware,
>> as is.
>
> I don't think anyone is talking about changing the DT here, only the way
> it's represented inside Linux.
>
>>                  sata0: sata-port at 0 {
>>                          reg = <0>;
>>                          phys = <&sata_phy 0>;
>> 			target-supply = <&reg_sata0>;
>>                  };
>>
>>                  sata1: sata-port at 1 {
>>                          reg = <1>;
>>                          phys = <&sata_phy 1>;
>> 			target-supply = <&reg_sata1>;
>>                  };
>>          };
>
>> Seems to match the hardware pretty exactly, and also matches how we've
>> been describing similar devices with only one sata port + power plug sofar,
>> so from a consistency pov it also is a good model.
>
> Right, I think that makes sense

Good, as said I think getting the dt bit rights is the most important
thing here. So if we agree that the above dt example looks sane, lets see
how we can best implement that.

 > it also looks to me like these things
> should be representable as devices within Linux.

I guess we could manually instantiate platform devices for each of the
subnodes which represent an sata port here, yes that should not be
hard to do, it feels like a bit overkill though.

So for our this should likely look something like this:

         if (IS_ENABLED(CONFIG_OF_ADDRESS) && dev->of_node) {
                 for_each_child_of_node(dev->of_node, np) {
                         pdev[i++] = of_platform_device_create(np, NULL, NULL);
                 }
         }

and then loop over the pdev[] array and get the regulator, and phy for each.

I wonder if of_platform_device_create can deal with nodes with
#size-cells = <0>; is 0 though, if it cannot maybe we need to teach it.

>> So supporting this model requires having a regulator_get API which allows
>> specifying which of_node to get the regulator from, e.g. the proposed
>
> It requires nothing of the sort.

You're proposed solution of instantiating devices does more or less exactly
what I say is required, it is a way to pass an of_node into regulator_get,
but you're hiding the node inside dev->of_node. I can see the appeal in
that.

>> of_regulator_get function. I know you (Mark) do not like this, but all
>> other subsystems have an of_foo_get function taking an of_node as argument,
>> I do not see how the regulator subsys is so special that it cannot have one,
>> and also notice that this is only a kernel internal API which we can always
>> change later.
>
> Two things there.  One is that mostly those APIs are legacy APIs from
> before we made DT a first class citizen and generally they shouldn't be
> used these days.  The other is that at least for regulators we have
> constant problems with people abusing the API in various ways, as a
> result the API has a goal of not helping undesirable usage patterns in
> order to help people spot problems.  Having an API which makes it easy
> create broken bindings means that it is much more likely that people
> will do just that.

Hmm I can see where you're coming from, and your proposed solution may
also be useful for when we get similar boards requiring explicit regulator
handling with the sata ports described in ACPI tables.

Gregory, can you give the setup using per sata port devices a try, and
see how that works out code wise ?

Regards,

Hans

[PATCH v2 2/4] Documentation: bindings: Add the regulator property to the sub-nodes AHCI bindings

[Gregory CLEMENT]

On 10/01/2015 14:51, Hans de Goede wrote:
> Hi,
> 
> On 10-01-15 12:17, Mark Brown wrote:
>> On Sat, Jan 10, 2015 at 11:20:13AM +0100, Hans de Goede wrote:
>>> On 09-01-15 18:11, Mark Brown wrote:
>>
>>>> Or if the supply is for the device at the other end of the link (which
>>>> is what it sounded like) then use that.  This just sounds like the same
>>>> problem we have for all the enumerable buses in embedded systems where
>>>> we need to be able to understand that the device exists prior to it
>>>> being fully ready to appear in the system.  Having the link/slot be a
>>>> device in Linux does indeed seem to be a common way people think about
>>>> doing this, it sounds like for this one it might be the most direct.
>>
>>> I think we should be careful to not think too much from an implementation
>>> pov here, the purpose of the devicetree description is to describe the hardware,
>>> as is.
>>
>> I don't think anyone is talking about changing the DT here, only the way
>> it's represented inside Linux.
>>
>>>                  sata0: sata-port at 0 {
>>>                          reg = <0>;
>>>                          phys = <&sata_phy 0>;
>>> 			target-supply = <&reg_sata0>;
>>>                  };
>>>
>>>                  sata1: sata-port at 1 {
>>>                          reg = <1>;
>>>                          phys = <&sata_phy 1>;
>>> 			target-supply = <&reg_sata1>;
>>>                  };
>>>          };
>>
>>> Seems to match the hardware pretty exactly, and also matches how we've
>>> been describing similar devices with only one sata port + power plug sofar,
>>> so from a consistency pov it also is a good model.
>>
>> Right, I think that makes sense
> 
> Good, as said I think getting the dt bit rights is the most important
> thing here. So if we agree that the above dt example looks sane, lets see
> how we can best implement that.
> 
>  > it also looks to me like these things
>> should be representable as devices within Linux.
> 
> I guess we could manually instantiate platform devices for each of the
> subnodes which represent an sata port here, yes that should not be
> hard to do, it feels like a bit overkill though.
> 
> So for our this should likely look something like this:
> 
>          if (IS_ENABLED(CONFIG_OF_ADDRESS) && dev->of_node) {
>                  for_each_child_of_node(dev->of_node, np) {
>                          pdev[i++] = of_platform_device_create(np, NULL, NULL);
>                  }
>          }
> 
> and then loop over the pdev[] array and get the regulator, and phy for each.
> 
> I wonder if of_platform_device_create can deal with nodes with
> #size-cells = <0>; is 0 though, if it cannot maybe we need to teach it.
> 
>>> So supporting this model requires having a regulator_get API which allows
>>> specifying which of_node to get the regulator from, e.g. the proposed
>>
>> It requires nothing of the sort.
> 
> You're proposed solution of instantiating devices does more or less exactly
> what I say is required, it is a way to pass an of_node into regulator_get,
> but you're hiding the node inside dev->of_node. I can see the appeal in
> that.
> 
>>> of_regulator_get function. I know you (Mark) do not like this, but all
>>> other subsystems have an of_foo_get function taking an of_node as argument,
>>> I do not see how the regulator subsys is so special that it cannot have one,
>>> and also notice that this is only a kernel internal API which we can always
>>> change later.
>>
>> Two things there.  One is that mostly those APIs are legacy APIs from
>> before we made DT a first class citizen and generally they shouldn't be
>> used these days.  The other is that at least for regulators we have
>> constant problems with people abusing the API in various ways, as a
>> result the API has a goal of not helping undesirable usage patterns in
>> order to help people spot problems.  Having an API which makes it easy
>> create broken bindings means that it is much more likely that people
>> will do just that.
> 
> Hmm I can see where you're coming from, and your proposed solution may
> also be useful for when we get similar boards requiring explicit regulator
> handling with the sata ports described in ACPI tables.
> 
> Gregory, can you give the setup using per sata port devices a try, and
> see how that works out code wise ?

OK, I am taking care of it.


Thanks,

Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH v2 3/4] ata: libahci: Allow using multiple regulators

[Gregory CLEMENT]

The current implementation of the libahci allows using multiple PHYs
but not multiple regulators. This patch adds the support of multiple
regulators. Due to some design choice of the regulator framework it is
not possible to declare the regulator under each port sub-node. They
need to be associated to each port as the host node level using the
property target-port<n>-supply, where <n> is the port number. But it
is however mandatory to have one port sub-node for each port associated
to a regulator.

There was only one driver which used directly the regulator field of
the ahci_host_priv structure. To preserve the bisectability the change
in the ahci_imx driver was done in the same patch.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/ata/ahci.h             |   2 +-
 drivers/ata/ahci_imx.c         |  14 +--
 drivers/ata/libahci_platform.c | 218 +++++++++++++++++++++++++++++------------
 include/linux/ahci_platform.h  |   2 +
 4 files changed, 162 insertions(+), 74 deletions(-)

diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 40f0e34f17af..275358ae0b3f 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -333,7 +333,7 @@ struct ahci_host_priv {
 	u32			em_msg_type;	/* EM message type */
 	bool			got_runtime_pm; /* Did we do pm_runtime_get? */
 	struct clk		*clks[AHCI_MAX_CLKS]; /* Optional */
-	struct regulator	*target_pwr;	/* Optional */
+	struct regulator	**target_pwrs;	/* Optional */
 	/*
 	 * If platform uses PHYs. There is a 1:1 relation between the port number and
 	 * the PHY position in this array.
diff --git a/drivers/ata/ahci_imx.c b/drivers/ata/ahci_imx.c
index 35d51c59a370..41632e57d46f 100644
--- a/drivers/ata/ahci_imx.c
+++ b/drivers/ata/ahci_imx.c
@@ -221,11 +221,9 @@ static int imx_sata_enable(struct ahci_host_priv *hpriv)
 	if (imxpriv->no_device)
 		return 0;
 
-	if (hpriv->target_pwr) {
-		ret = regulator_enable(hpriv->target_pwr);
-		if (ret)
-			return ret;
-	}
+	ret = ahci_platform_enable_regulators(hpriv);
+	if (ret)
+		return ret;
 
 	ret = clk_prepare_enable(imxpriv->sata_ref_clk);
 	if (ret < 0)
@@ -270,8 +268,7 @@ static int imx_sata_enable(struct ahci_host_priv *hpriv)
 disable_clk:
 	clk_disable_unprepare(imxpriv->sata_ref_clk);
 disable_regulator:
-	if (hpriv->target_pwr)
-		regulator_disable(hpriv->target_pwr);
+	ahci_platform_disable_regulators(hpriv);
 
 	return ret;
 }
@@ -291,8 +288,7 @@ static void imx_sata_disable(struct ahci_host_priv *hpriv)
 
 	clk_disable_unprepare(imxpriv->sata_ref_clk);
 
-	if (hpriv->target_pwr)
-		regulator_disable(hpriv->target_pwr);
+	ahci_platform_disable_regulators(hpriv);
 }
 
 static void ahci_imx_error_handler(struct ata_port *ap)
diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index a147aaadca85..a42fd62cbdfa 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -138,6 +138,59 @@ void ahci_platform_disable_clks(struct ahci_host_priv *hpriv)
 EXPORT_SYMBOL_GPL(ahci_platform_disable_clks);
 
 /**
+ * ahci_platform_enable_regulators - Enable regulators
+ * @hpriv: host private area to store config values
+ *
+ * This function enables all the regulators found in
+ * hpriv->target_pwrs, if any.  If a regulator fails to be enabled, it
+ * disables all the regulators already enabled in reverse order and
+ * returns an error.
+ *
+ * RETURNS:
+ * 0 on success otherwise a negative error code
+ */
+int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv)
+{
+	int rc, i;
+
+	for (i = 0; i < hpriv->nports; i++) {
+		if (!hpriv->target_pwrs[i])
+			continue;
+
+		rc = regulator_enable(hpriv->target_pwrs[i]);
+		if (rc)
+			goto disable_target_pwrs;
+	}
+
+	return 0;
+
+disable_target_pwrs:
+	while (--i >= 0)
+		if (hpriv->target_pwrs[i])
+			regulator_disable(hpriv->target_pwrs[i]);
+
+	return rc;
+}
+EXPORT_SYMBOL_GPL(ahci_platform_enable_regulators);
+
+/**
+ * ahci_platform_disable_regulators - Disable regulators
+ * @hpriv: host private area to store config values
+ *
+ * This function disables all regulators found in hpriv->target_pwrs.
+ */
+void ahci_platform_disable_regulators(struct ahci_host_priv *hpriv)
+{
+	int i;
+
+	for (i = 0; i < hpriv->nports; i++) {
+		if (!hpriv->target_pwrs[i])
+			continue;
+		regulator_disable(hpriv->target_pwrs[i]);
+	}
+}
+EXPORT_SYMBOL_GPL(ahci_platform_disable_regulators);
+/**
  * ahci_platform_enable_resources - Enable platform resources
  * @hpriv: host private area to store config values
  *
@@ -157,11 +210,9 @@ int ahci_platform_enable_resources(struct ahci_host_priv *hpriv)
 {
 	int rc;
 
-	if (hpriv->target_pwr) {
-		rc = regulator_enable(hpriv->target_pwr);
-		if (rc)
-			return rc;
-	}
+	rc = ahci_platform_enable_regulators(hpriv);
+	if (rc)
+		return rc;
 
 	rc = ahci_platform_enable_clks(hpriv);
 	if (rc)
@@ -177,8 +228,8 @@ disable_clks:
 	ahci_platform_disable_clks(hpriv);
 
 disable_regulator:
-	if (hpriv->target_pwr)
-		regulator_disable(hpriv->target_pwr);
+	ahci_platform_disable_regulators(hpriv);
+
 	return rc;
 }
 EXPORT_SYMBOL_GPL(ahci_platform_enable_resources);
@@ -199,8 +250,7 @@ void ahci_platform_disable_resources(struct ahci_host_priv *hpriv)
 
 	ahci_platform_disable_clks(hpriv);
 
-	if (hpriv->target_pwr)
-		regulator_disable(hpriv->target_pwr);
+	ahci_platform_disable_regulators(hpriv);
 }
 EXPORT_SYMBOL_GPL(ahci_platform_disable_resources);
 
@@ -218,6 +268,65 @@ static void ahci_platform_put_resources(struct device *dev, void *res)
 		clk_put(hpriv->clks[c]);
 }
 
+static int ahci_platform_get_phy(struct ahci_host_priv *hpriv, u32 port,
+				struct device *dev, struct device_node *node)
+{
+	int rc;
+
+	hpriv->phys[port] = devm_of_phy_get(dev, node, NULL);
+
+	if (!IS_ERR(hpriv->phys[port]))
+		return 0;
+
+	rc = PTR_ERR(hpriv->phys[port]);
+	switch (rc) {
+	case -ENOSYS:
+		/* No PHY support. Check if PHY is required. */
+		if (of_find_property(node, "phys", NULL)) {
+			dev_err(dev,
+				"couldn't get PHY in node %s: ENOSYS\n",
+				node->name);
+			break;
+		}
+	case -ENODEV:
+		/* continue normally */
+		hpriv->phys[port] = NULL;
+		rc = 0;
+		break;
+
+	default:
+		dev_err(dev,
+			"couldn't get PHY in node %s: %d\n",
+			node->name, rc);
+
+		break;
+	}
+
+	return rc;
+}
+
+static int ahci_platform_get_regulator(struct ahci_host_priv *hpriv, u32 port,
+				struct device *dev, struct device_node *node)
+{
+	struct regulator *target_pwr;
+	int rc = 0;
+	/*
+	 * No more than 32 ports (AHCI_MAX_PORTS), so we need 13
+	 * characters for target-portXX plus the null terminated one
+	 */
+	char name[14];
+
+	sprintf(name, "target-port%d", port);
+	target_pwr = devm_regulator_get_optional(dev, name);
+
+	if (!IS_ERR(target_pwr))
+		hpriv->target_pwrs[port] = target_pwr;
+	else
+		rc = PTR_ERR(target_pwr);
+
+	return rc;
+}
+
 /**
  * ahci_platform_get_resources - Get platform resources
  * @pdev: platform device to get resources for
@@ -240,7 +349,7 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
 	struct ahci_host_priv *hpriv;
 	struct clk *clk;
 	struct device_node *child;
-	int i, enabled_ports = 0, rc = -ENOMEM;
+	int i, sz, enabled_ports = 0, rc = -ENOMEM, child_nodes;
 	u32 mask_port_map = 0;
 
 	if (!devres_open_group(dev, NULL, GFP_KERNEL))
@@ -261,14 +370,6 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
 		goto err_out;
 	}
 
-	hpriv->target_pwr = devm_regulator_get_optional(dev, "target");
-	if (IS_ERR(hpriv->target_pwr)) {
-		rc = PTR_ERR(hpriv->target_pwr);
-		if (rc == -EPROBE_DEFER)
-			goto err_out;
-		hpriv->target_pwr = NULL;
-	}
-
 	for (i = 0; i < AHCI_MAX_CLKS; i++) {
 		/*
 		 * For now we must use clk_get(dev, NULL) for the first clock,
@@ -290,17 +391,30 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
 		hpriv->clks[i] = clk;
 	}
 
-	hpriv->nports = of_get_child_count(dev->of_node);
+	hpriv->nports = child_nodes = of_get_child_count(dev->of_node);
 
-	if (hpriv->nports) {
-		hpriv->phys = devm_kzalloc(dev,
-					   hpriv->nports * sizeof(*hpriv->phys),
-					   GFP_KERNEL);
-		if (!hpriv->phys) {
-			rc = -ENOMEM;
-			goto err_out;
-		}
+	/*
+	 * If no sub-node was found, we still need to set nports to
+	 * one in order to be able to use the
+	 * ahci_platform_[en|dis]able_[phys|regulators] functions.
+	 */
+	if (!child_nodes)
+		hpriv->nports = 1;
 
+	sz = hpriv->nports * sizeof(*hpriv->phys);
+	hpriv->phys = devm_kzalloc(dev, sz, GFP_KERNEL);
+	if (!hpriv->phys) {
+		rc = -ENOMEM;
+		goto err_out;
+	}
+	sz = hpriv->nports * sizeof(*hpriv->target_pwrs);
+	hpriv->target_pwrs = devm_kzalloc(dev, sz, GFP_KERNEL);
+	if (!hpriv->target_pwrs) {
+		rc = -ENOMEM;
+		goto err_out;
+	}
+
+	if (child_nodes) {
 		for_each_child_of_node(dev->of_node, child) {
 			u32 port;
 
@@ -319,14 +433,14 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
 
 			mask_port_map |= BIT(port);
 
-			hpriv->phys[port] = devm_of_phy_get(dev, child, NULL);
-			if (IS_ERR(hpriv->phys[port])) {
-				rc = PTR_ERR(hpriv->phys[port]);
-				dev_err(dev,
-					"couldn't get PHY in node %s: %d\n",
-					child->name, rc);
+			rc = ahci_platform_get_regulator(hpriv, port,
+							 dev, child);
+			if (rc == -EPROBE_DEFER)
+				goto err_out;
+
+			rc = ahci_platform_get_phy(hpriv, port, dev, child);
+			if (rc)
 				goto err_out;
-			}
 
 			enabled_ports++;
 		}
@@ -343,38 +457,14 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev)
 		 * If no sub-node was found, keep this for device tree
 		 * compatibility
 		 */
-		struct phy *phy = devm_phy_get(dev, "sata-phy");
-		if (!IS_ERR(phy)) {
-			hpriv->phys = devm_kzalloc(dev, sizeof(*hpriv->phys),
-						   GFP_KERNEL);
-			if (!hpriv->phys) {
-				rc = -ENOMEM;
-				goto err_out;
-			}
-
-			hpriv->phys[0] = phy;
-			hpriv->nports = 1;
-		} else {
-			rc = PTR_ERR(phy);
-			switch (rc) {
-				case -ENOSYS:
-					/* No PHY support. Check if PHY is required. */
-					if (of_find_property(dev->of_node, "phys", NULL)) {
-						dev_err(dev, "couldn't get sata-phy: ENOSYS\n");
-						goto err_out;
-					}
-				case -ENODEV:
-					/* continue normally */
-					hpriv->phys = NULL;
-					break;
-
-				default:
-					goto err_out;
+		rc = ahci_platform_get_phy(hpriv, 0, dev, dev->of_node);
+		if (rc)
+			goto err_out;
 
-			}
-		}
+		rc = ahci_platform_get_regulator(hpriv, 0, dev, dev->of_node);
+		if (rc == -EPROBE_DEFER)
+			goto err_out;
 	}
-
 	pm_runtime_enable(dev);
 	pm_runtime_get_sync(dev);
 	hpriv->got_runtime_pm = true;
diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
index 642d6ae4030c..f65b33809170 100644
--- a/include/linux/ahci_platform.h
+++ b/include/linux/ahci_platform.h
@@ -24,6 +24,8 @@ struct platform_device;
 
 int ahci_platform_enable_clks(struct ahci_host_priv *hpriv);
 void ahci_platform_disable_clks(struct ahci_host_priv *hpriv);
+int ahci_platform_enable_regulators(struct ahci_host_priv *hpriv);
+void ahci_platform_disable_regulators(struct ahci_host_priv *hpriv);
 int ahci_platform_enable_resources(struct ahci_host_priv *hpriv);
 void ahci_platform_disable_resources(struct ahci_host_priv *hpriv);
 struct ahci_host_priv *ahci_platform_get_resources(
-- 
1.9.1

[PATCH v2 4/4] ARM: mvebu: Armada 385 GP: Add regulators to the SATA port

[Gregory CLEMENT]

Add the regulators to each SATA port.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/boot/dts/armada-388-gp.dts | 126 ++++++++++++++++++++++++++++++++++++
 1 file changed, 126 insertions(+)

diff --git a/arch/arm/boot/dts/armada-388-gp.dts b/arch/arm/boot/dts/armada-388-gp.dts
index 4df22bf91683..c599c71bc5ce 100644
--- a/arch/arm/boot/dts/armada-388-gp.dts
+++ b/arch/arm/boot/dts/armada-388-gp.dts
@@ -173,6 +173,16 @@
 				status = "okay";
 				#address-cells = <1>;
 				#size-cells = <0>;
+				target-port0-supply = <&reg_5v_sata0>;
+				target-port1-supply = <&reg_5v_sata1>;
+
+				sata0: sata-port at 0 {
+					reg = <0>;
+				};
+
+				sata1: sata-port at 1 {
+					reg = <1>;
+				};
 			};
 
 			sata at e0000 {
@@ -181,6 +191,16 @@
 				status = "okay";
 				#address-cells = <1>;
 				#size-cells = <0>;
+				target-port1-supply = <&reg_5v_sata3>;
+				target-port0-supply = <&reg_5v_sata2>;
+
+				sata2: sata-port at 0 {
+					reg = <0>;
+				};
+
+				sata3: sata-port at 1 {
+					reg = <1>;
+				};
 			};
 
 			sdhci at d8000 {
@@ -278,6 +298,112 @@
 		regulator-always-on;
 		gpio = <&expander0 4 GPIO_ACTIVE_HIGH>;
 	};
+
+	reg_sata0: pwr-sata0 {
+		compatible = "regulator-fixed";
+		regulator-name = "pwr_en_sata0";
+		enable-active-high;
+		regulator-always-on;
+
+	};
+
+	reg_5v_sata0: v5-sata0 {
+		compatible = "regulator-fixed";
+		regulator-name = "v5.0-sata0";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		regulator-always-on;
+		vin-supply = <&reg_sata0>;
+	};
+
+	reg_12v_sata0: v12-sata0 {
+		compatible = "regulator-fixed";
+		regulator-name = "v12.0-sata0";
+		regulator-min-microvolt = <12000000>;
+		regulator-max-microvolt = <12000000>;
+		regulator-always-on;
+		vin-supply = <&reg_sata0>;
+	};
+
+	reg_sata1: pwr-sata1 {
+		regulator-name = "pwr_en_sata1";
+		compatible = "regulator-fixed";
+		regulator-min-microvolt = <12000000>;
+		regulator-max-microvolt = <12000000>;
+		enable-active-high;
+		regulator-always-on;
+		gpio = <&expander0 3 GPIO_ACTIVE_HIGH>;
+	};
+
+	reg_5v_sata1: v5-sata1 {
+		compatible = "regulator-fixed";
+		regulator-name = "v5.0-sata1";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		regulator-always-on;
+		vin-supply = <&reg_sata1>;
+	};
+
+	reg_12v_sata1: v12-sata1 {
+		compatible = "regulator-fixed";
+		regulator-name = "v12.0-sata1";
+		regulator-min-microvolt = <12000000>;
+		regulator-max-microvolt = <12000000>;
+		regulator-always-on;
+		vin-supply = <&reg_sata1>;
+	};
+
+	reg_sata2: pwr-sata2 {
+		compatible = "regulator-fixed";
+		regulator-name = "pwr_en_sata2";
+		enable-active-high;
+		regulator-always-on;
+		gpio = <&expander0 11 GPIO_ACTIVE_HIGH>;
+	};
+
+	reg_5v_sata2: v5-sata2 {
+		compatible = "regulator-fixed";
+		regulator-name = "v5.0-sata2";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		regulator-always-on;
+		vin-supply = <&reg_sata2>;
+	};
+
+	reg_12v_sata2: v12-sata2 {
+		compatible = "regulator-fixed";
+		regulator-name = "v12.0-sata2";
+		regulator-min-microvolt = <12000000>;
+		regulator-max-microvolt = <12000000>;
+		regulator-always-on;
+		vin-supply = <&reg_sata2>;
+	};
+
+	reg_sata3: pwr-sata3 {
+		compatible = "regulator-fixed";
+		regulator-name = "pwr_en_sata3";
+		enable-active-high;
+		regulator-always-on;
+		gpio = <&expander0 12 GPIO_ACTIVE_HIGH>;
+	};
+
+	reg_5v_sata3: v5-sata3 {
+		compatible = "regulator-fixed";
+		regulator-name = "v5.0-sata3";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		regulator-always-on;
+		vin-supply = <&reg_sata3>;
+	};
+
+	reg_12v_sata3: v12-sata3 {
+		compatible = "regulator-fixed";
+		regulator-name = "v12.0-sata3";
+		regulator-min-microvolt = <12000000>;
+		regulator-max-microvolt = <12000000>;
+		regulator-always-on;
+		vin-supply = <&reg_sata3>;
+	};
 };
 
 &pinctrl {
-- 
1.9.1