[PATCH v3 0/6] ARM: berlin: add AHCI support

[PATCH v3 0/6] ARM: berlin: add AHCI support

[Antoine Ténart]

This series adds the support for Berlin SoCs AHCI controllers. The
controller allows to use the SATA host interface and, for example, the
eSATA port on the BG2Q.

The series adds a PHY driver to control the two SATA ports available.

Also enable the eSATA interface on the BG2Q DMP.

Changes since v2:
	- modeled each PHY as a sub-node
	- cosmetic fixups

Changes since v1:
        - added a PHY driver, allowing to enable each port
          individually and removed the 'force-port-map' property
        - made the drivers a bit less magic :)
        - wrote a function to select and configure registers in the
          AHCI driver
        - removed BG2 / BG2CD nodes

Antoine Ténart (6):
  phy: add a driver for the Berlin SATA PHY
  Documentation: bindings: add the Berlin SATA PHY
  ata: ahci: add AHCI support for the Berlin BG2Q
  Documentation: bindings: add the berlin-ahci compatible to the ahci
    platform
  ARM: berlin: add the AHCI node for the BG2Q
  ARM: berlin: enable the eSATA interface on the BG2Q DMP

 .../devicetree/bindings/ata/ahci-berlin.txt        |  18 ++
 .../devicetree/bindings/phy/berlin-sata-phy.txt    |  34 ++++
 arch/arm/boot/dts/berlin2q-marvell-dmp.dts         |  10 +
 arch/arm/boot/dts/berlin2q.dtsi                    |  25 +++
 drivers/ata/Kconfig                                |  10 +
 drivers/ata/Makefile                               |   1 +
 drivers/ata/ahci_berlin.c                          | 202 +++++++++++++++++++++
 drivers/phy/Kconfig                                |   5 +
 drivers/phy/Makefile                               |   1 +
 drivers/phy/phy-berlin-sata.c                      | 180 ++++++++++++++++++
 10 files changed, 486 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ata/ahci-berlin.txt
 create mode 100644 Documentation/devicetree/bindings/phy/berlin-sata-phy.txt
 create mode 100644 drivers/ata/ahci_berlin.c
 create mode 100644 drivers/phy/phy-berlin-sata.c

-- 
1.9.1

[PATCH v3 1/6] phy: add a driver for the Berlin SATA PHY

[Antoine Ténart]

The Berlin SoC has a two SATA ports. Add a PHY driver to handle them.

Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com>
---
 drivers/phy/Kconfig           |   5 ++
 drivers/phy/Makefile          |   1 +
 drivers/phy/phy-berlin-sata.c | 180 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 186 insertions(+)
 create mode 100644 drivers/phy/phy-berlin-sata.c

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index 4906c27fa3bd..b31b1986fda4 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -15,6 +15,11 @@ config GENERIC_PHY
 	  phy users can obtain reference to the PHY. All the users of this
 	  framework should select this config.
 
+config PHY_BERLIN_SATA
+	bool
+	depends on ARCH_BERLIN && OF
+	select GENERIC_PHY
+
 config PHY_EXYNOS_MIPI_VIDEO
 	tristate "S5P/EXYNOS SoC series MIPI CSI-2/DSI PHY driver"
 	depends on HAS_IOMEM
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index 7728518572a4..40278706ac1b 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -3,6 +3,7 @@
 #
 
 obj-$(CONFIG_GENERIC_PHY)		+= phy-core.o
+obj-$(CONFIG_PHY_BERLIN_SATA)		+= phy-berlin-sata.o
 obj-$(CONFIG_BCM_KONA_USB2_PHY)		+= phy-bcm-kona-usb2.o
 obj-$(CONFIG_PHY_EXYNOS_DP_VIDEO)	+= phy-exynos-dp-video.o
 obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO)	+= phy-exynos-mipi-video.o
diff --git a/drivers/phy/phy-berlin-sata.c b/drivers/phy/phy-berlin-sata.c
new file mode 100644
index 000000000000..b08f76329a34
--- /dev/null
+++ b/drivers/phy/phy-berlin-sata.c
@@ -0,0 +1,180 @@
+/*
+ * Marvell Berlin SATA PHY driver
+ *
+ * Copyright (C) 2014 Marvell Technology Group Ltd.
+ *
+ * Antoine Ténart <antoine.tenart@free-electrons.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/module.h>
+#include <linux/phy/phy.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+
+#define HOST_VSA_ADDR		0x0
+#define HOST_VSA_DATA		0x4
+
+#define CONTROL_REGISTER	0x0
+#define MBUS_SIZE_CONTROL	0x4
+
+#define POWER_DOWN_SATA0		BIT(6)
+#define POWER_DOWN_SATA1		BIT(14)
+#define MBUS_WRITE_REQUEST_SIZE_128	(BIT(2) << 16)
+#define MBUS_READ_REQUEST_SIZE_128	(BIT(2) << 19)
+
+#define BERLIN_SATA_PHY_NB	2
+
+#define to_berlin_sata_phy_priv(desc)	\
+	container_of((desc), struct phy_berlin_priv, phys[(desc)->index])
+
+struct phy_berlin_desc {
+	struct phy	*phy;
+	u32		val;
+	unsigned	index;
+};
+
+struct phy_berlin_priv {
+	void __iomem	*base;
+	spinlock_t	lock;
+	struct phy_berlin_desc	phys[BERLIN_SATA_PHY_NB];
+};
+
+static int phy_berlin_sata_power_on(struct phy *phy)
+{
+	struct phy_berlin_desc *desc = phy_get_drvdata(phy);
+	struct phy_berlin_priv *priv = to_berlin_sata_phy_priv(desc);
+	u32 regval;
+
+	spin_lock(&priv->lock);
+
+	/* Power up PHY */
+	writel(CONTROL_REGISTER, priv->base + HOST_VSA_ADDR);
+	regval = readl(priv->base + HOST_VSA_DATA);
+	regval &= ~(desc->val);
+	writel(regval, priv->base + HOST_VSA_DATA);
+
+	/* Configure MBus */
+	writel(MBUS_SIZE_CONTROL, priv->base + HOST_VSA_ADDR);
+	regval = readl(priv->base + HOST_VSA_DATA);
+	regval |= MBUS_WRITE_REQUEST_SIZE_128 | MBUS_READ_REQUEST_SIZE_128;
+	writel(regval, priv->base + HOST_VSA_DATA);
+
+	spin_unlock(&priv->lock);
+
+	return 0;
+}
+
+static int phy_berlin_sata_power_off(struct phy *phy)
+{
+	struct phy_berlin_desc *desc = phy_get_drvdata(phy);
+	struct phy_berlin_priv *priv = to_berlin_sata_phy_priv(desc);
+	u32 regval;
+
+	spin_lock(&priv->lock);
+
+	/* Power down PHY */
+	writel(CONTROL_REGISTER, priv->base + HOST_VSA_ADDR);
+	regval = readl(priv->base + HOST_VSA_DATA);
+	regval |= desc->val;
+	writel(regval, priv->base + HOST_VSA_DATA);
+
+	spin_unlock(&priv->lock);
+
+	return 0;
+}
+
+static struct phy_ops phy_berlin_sata_ops = {
+	.power_on	= phy_berlin_sata_power_on,
+	.power_off	= phy_berlin_sata_power_off,
+	.owner		= THIS_MODULE,
+};
+
+static struct phy_berlin_desc desc[] = {
+	{ .val = POWER_DOWN_SATA0 },
+	{ .val = POWER_DOWN_SATA1 },
+	{ },
+};
+
+static int phy_berlin_sata_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct phy *phy;
+	struct phy_provider *phy_provider;
+	struct phy_berlin_priv *priv;
+	struct resource *res;
+	struct device_node *child;
+	u8 phy_id;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap(dev, res->start, resource_size(res));
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	dev_set_drvdata(dev, priv);
+	spin_lock_init(&priv->lock);
+
+	for_each_child_of_node(dev->of_node, child) {
+		if (of_property_read_u8(child, "reg", &phy_id))
+			return -EINVAL;
+
+		if (phy_id >= BERLIN_SATA_PHY_NB) {
+			dev_err(dev, "invalid reg in node %s\n", child->name);
+			return -EINVAL;
+		}
+
+		phy = devm_phy_create(dev, &phy_berlin_sata_ops, NULL);
+		if (IS_ERR(phy)) {
+			dev_err(dev, "failed to create PHY for node %s\n",
+				child->name);
+			return PTR_ERR(phy);
+		}
+
+		/*
+		 * By default the PHY node is used to request and match a PHY.
+		 * We describe one PHY per sub-node here. Use the right node.
+		 */
+		phy->dev.of_node = child;
+
+		priv->phys[phy_id].phy = phy;
+		priv->phys[phy_id].val = desc[phy_id].val;
+		priv->phys[phy_id].index = phy_id;
+		phy_set_drvdata(phy, &priv->phys[phy_id]);
+
+		/* Make sure the PHY is off */
+		phy_berlin_sata_power_off(phy);
+
+		phy_provider = devm_of_phy_provider_register(&phy->dev,
+							     of_phy_simple_xlate);
+		if (IS_ERR(phy_provider))
+			return PTR_ERR(phy_provider);
+	}
+
+	return 0;
+}
+
+static const struct of_device_id phy_berlin_sata_of_match[] = {
+	{ .compatible = "marvell,berlin-sata-phy" },
+	{ },
+};
+
+static struct platform_driver phy_berlin_sata_driver = {
+	.probe	= phy_berlin_sata_probe,
+	.driver	= {
+		.name		= "phy-berlin-sata",
+		.owner		= THIS_MODULE,
+		.of_match_table	= phy_berlin_sata_of_match,
+	},
+};
+module_platform_driver(phy_berlin_sata_driver);
+
+MODULE_DESCRIPTION("Marvell Berlin SATA PHY driver");
+MODULE_AUTHOR("Antoine Ténart <antoine.tenart@free-electrons.com>");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

Re: [PATCH v3 1/6] phy: add a driver for the Berlin SATA PHY

[Kishon Vijay Abraham I]

Hi,

On Wednesday 14 May 2014 03:18 PM, Antoine Ténart wrote:
> The Berlin SoC has a two SATA ports. Add a PHY driver to handle them.
> 
> Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com>
> ---
>  drivers/phy/Kconfig           |   5 ++
>  drivers/phy/Makefile          |   1 +
>  drivers/phy/phy-berlin-sata.c | 180 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 186 insertions(+)
>  create mode 100644 drivers/phy/phy-berlin-sata.c
> 
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 4906c27fa3bd..b31b1986fda4 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -15,6 +15,11 @@ config GENERIC_PHY
>  	  phy users can obtain reference to the PHY. All the users of this
>  	  framework should select this config.
>  
> +config PHY_BERLIN_SATA
> +	bool
> +	depends on ARCH_BERLIN && OF
> +	select GENERIC_PHY
> +
>  config PHY_EXYNOS_MIPI_VIDEO
>  	tristate "S5P/EXYNOS SoC series MIPI CSI-2/DSI PHY driver"
>  	depends on HAS_IOMEM
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index 7728518572a4..40278706ac1b 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -3,6 +3,7 @@
>  #
>  
>  obj-$(CONFIG_GENERIC_PHY)		+= phy-core.o
> +obj-$(CONFIG_PHY_BERLIN_SATA)		+= phy-berlin-sata.o
>  obj-$(CONFIG_BCM_KONA_USB2_PHY)		+= phy-bcm-kona-usb2.o
>  obj-$(CONFIG_PHY_EXYNOS_DP_VIDEO)	+= phy-exynos-dp-video.o
>  obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO)	+= phy-exynos-mipi-video.o
> diff --git a/drivers/phy/phy-berlin-sata.c b/drivers/phy/phy-berlin-sata.c
> new file mode 100644
> index 000000000000..b08f76329a34
> --- /dev/null
> +++ b/drivers/phy/phy-berlin-sata.c
> @@ -0,0 +1,180 @@
> +/*
> + * Marvell Berlin SATA PHY driver
> + *
> + * Copyright (C) 2014 Marvell Technology Group Ltd.
> + *
> + * Antoine Ténart <antoine.tenart@free-electrons.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/phy/phy.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +
> +#define HOST_VSA_ADDR		0x0
> +#define HOST_VSA_DATA		0x4
> +
> +#define CONTROL_REGISTER	0x0
> +#define MBUS_SIZE_CONTROL	0x4
> +
> +#define POWER_DOWN_SATA0		BIT(6)
> +#define POWER_DOWN_SATA1		BIT(14)
> +#define MBUS_WRITE_REQUEST_SIZE_128	(BIT(2) << 16)
> +#define MBUS_READ_REQUEST_SIZE_128	(BIT(2) << 19)
> +
> +#define BERLIN_SATA_PHY_NB	2
> +
> +#define to_berlin_sata_phy_priv(desc)	\
> +	container_of((desc), struct phy_berlin_priv, phys[(desc)->index])
> +
> +struct phy_berlin_desc {
> +	struct phy	*phy;
> +	u32		val;
> +	unsigned	index;
> +};
> +
> +struct phy_berlin_priv {
> +	void __iomem	*base;
> +	spinlock_t	lock;
> +	struct phy_berlin_desc	phys[BERLIN_SATA_PHY_NB];

Can't we do away with hardcoding BERLIN_SATA_PHY_NB?
> +};
> +
> +static int phy_berlin_sata_power_on(struct phy *phy)
> +{
> +	struct phy_berlin_desc *desc = phy_get_drvdata(phy);
> +	struct phy_berlin_priv *priv = to_berlin_sata_phy_priv(desc);
> +	u32 regval;
> +
> +	spin_lock(&priv->lock);
> +
> +	/* Power up PHY */
> +	writel(CONTROL_REGISTER, priv->base + HOST_VSA_ADDR);
> +	regval = readl(priv->base + HOST_VSA_DATA);
> +	regval &= ~(desc->val);
> +	writel(regval, priv->base + HOST_VSA_DATA);
> +
> +	/* Configure MBus */
> +	writel(MBUS_SIZE_CONTROL, priv->base + HOST_VSA_ADDR);
> +	regval = readl(priv->base + HOST_VSA_DATA);
> +	regval |= MBUS_WRITE_REQUEST_SIZE_128 | MBUS_READ_REQUEST_SIZE_128;
> +	writel(regval, priv->base + HOST_VSA_DATA);
> +
> +	spin_unlock(&priv->lock);
> +
> +	return 0;
> +}
> +
> +static int phy_berlin_sata_power_off(struct phy *phy)
> +{
> +	struct phy_berlin_desc *desc = phy_get_drvdata(phy);
> +	struct phy_berlin_priv *priv = to_berlin_sata_phy_priv(desc);
> +	u32 regval;
> +
> +	spin_lock(&priv->lock);
> +
> +	/* Power down PHY */
> +	writel(CONTROL_REGISTER, priv->base + HOST_VSA_ADDR);
> +	regval = readl(priv->base + HOST_VSA_DATA);
> +	regval |= desc->val;
> +	writel(regval, priv->base + HOST_VSA_DATA);
> +
> +	spin_unlock(&priv->lock);
> +
> +	return 0;
> +}
> +
> +static struct phy_ops phy_berlin_sata_ops = {
> +	.power_on	= phy_berlin_sata_power_on,
> +	.power_off	= phy_berlin_sata_power_off,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static struct phy_berlin_desc desc[] = {
> +	{ .val = POWER_DOWN_SATA0 },
> +	{ .val = POWER_DOWN_SATA1 },
> +	{ },
> +};
> +
> +static int phy_berlin_sata_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct phy *phy;
> +	struct phy_provider *phy_provider;
> +	struct phy_berlin_priv *priv;
> +	struct resource *res;
> +	struct device_node *child;
> +	u8 phy_id;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	priv->base = devm_ioremap(dev, res->start, resource_size(res));
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	dev_set_drvdata(dev, priv);
> +	spin_lock_init(&priv->lock);
> +
> +	for_each_child_of_node(dev->of_node, child) {
> +		if (of_property_read_u8(child, "reg", &phy_id))
> +			return -EINVAL;
> +
> +		if (phy_id >= BERLIN_SATA_PHY_NB) {
> +			dev_err(dev, "invalid reg in node %s\n", child->name);
> +			return -EINVAL;
> +		}
> +
> +		phy = devm_phy_create(dev, &phy_berlin_sata_ops, NULL);
> +		if (IS_ERR(phy)) {
> +			dev_err(dev, "failed to create PHY for node %s\n",
> +				child->name);
> +			return PTR_ERR(phy);
> +		}
> +
> +		/*
> +		 * By default the PHY node is used to request and match a PHY.
> +		 * We describe one PHY per sub-node here. Use the right node.
> +		 */
> +		phy->dev.of_node = child;
> +
> +		priv->phys[phy_id].phy = phy;
> +		priv->phys[phy_id].val = desc[phy_id].val;
> +		priv->phys[phy_id].index = phy_id;
> +		phy_set_drvdata(phy, &priv->phys[phy_id]);
> +
> +		/* Make sure the PHY is off */
> +		phy_berlin_sata_power_off(phy);
> +
> +		phy_provider = devm_of_phy_provider_register(&phy->dev,
> +							     of_phy_simple_xlate);
> +		if (IS_ERR(phy_provider))
> +			return PTR_ERR(phy_provider);

was this intentional? registering multiple PHY providers?

Thanks
Kishon

Re: [PATCH v3 1/6] phy: add a driver for the Berlin SATA PHY

[Antoine Ténart]

Hi,

On Wed, May 14, 2014 at 03:43:03PM +0530, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Wednesday 14 May 2014 03:18 PM, Antoine Ténart wrote:

[…]

> > +#define to_berlin_sata_phy_priv(desc)	\
> > +	container_of((desc), struct phy_berlin_priv, phys[(desc)->index])
> > +
> > +struct phy_berlin_desc {
> > +	struct phy	*phy;
> > +	u32		val;
> > +	unsigned	index;
> > +};
> > +
> > +struct phy_berlin_priv {
> > +	void __iomem	*base;
> > +	spinlock_t	lock;
> > +	struct phy_berlin_desc	phys[BERLIN_SATA_PHY_NB];
> 
> Can't we do away with hardcoding BERLIN_SATA_PHY_NB?

We can't if we want to be able to use the container_of macro in
to_berlin_sata_phy_priv(). And we want a common structure to store the
common spinlock and base address.

[…]

> > +		/*
> > +		 * By default the PHY node is used to request and match a PHY.
> > +		 * We describe one PHY per sub-node here. Use the right node.
> > +		 */
> > +		phy->dev.of_node = child;
> > +
> > +		priv->phys[phy_id].phy = phy;
> > +		priv->phys[phy_id].val = desc[phy_id].val;
> > +		priv->phys[phy_id].index = phy_id;
> > +		phy_set_drvdata(phy, &priv->phys[phy_id]);
> > +
> > +		/* Make sure the PHY is off */
> > +		phy_berlin_sata_power_off(phy);
> > +
> > +		phy_provider = devm_of_phy_provider_register(&phy->dev,
> > +							     of_phy_simple_xlate);
> > +		if (IS_ERR(phy_provider))
> > +			return PTR_ERR(phy_provider);
> 
> was this intentional? registering multiple PHY providers?

Yes. Each sub-node describe a PHY and register as a PHY provider. This
allow to reference the PHY as <&sata_phy0> and not <&sata_phy 0>. It
would be confusing to have a sub-node sata_phy0 and to use its parent
to access it.

Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH v3 1/6] phy: add a driver for the Berlin SATA PHY

[Kishon Vijay Abraham I]

Hi,

On Wednesday 14 May 2014 03:51 PM, Antoine Ténart wrote:
> Hi,
> 
> On Wed, May 14, 2014 at 03:43:03PM +0530, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Wednesday 14 May 2014 03:18 PM, Antoine Ténart wrote:
> 
> […]
> 
>>> +#define to_berlin_sata_phy_priv(desc)	\
>>> +	container_of((desc), struct phy_berlin_priv, phys[(desc)->index])
>>> +
>>> +struct phy_berlin_desc {
>>> +	struct phy	*phy;
>>> +	u32		val;
>>> +	unsigned	index;
>>> +};
>>> +
>>> +struct phy_berlin_priv {
>>> +	void __iomem	*base;
>>> +	spinlock_t	lock;
>>> +	struct phy_berlin_desc	phys[BERLIN_SATA_PHY_NB];
>>
>> Can't we do away with hardcoding BERLIN_SATA_PHY_NB?
> 
> We can't if we want to be able to use the container_of macro in
> to_berlin_sata_phy_priv(). And we want a common structure to store the
> common spinlock and base address.

to get phy_berlin_priv, you can use dev_get_drvdata(phy->dev->parent).

> 
> […]
> 
>>> +		/*
>>> +		 * By default the PHY node is used to request and match a PHY.
>>> +		 * We describe one PHY per sub-node here. Use the right node.
>>> +		 */
>>> +		phy->dev.of_node = child;
>>> +
>>> +		priv->phys[phy_id].phy = phy;
>>> +		priv->phys[phy_id].val = desc[phy_id].val;
>>> +		priv->phys[phy_id].index = phy_id;
>>> +		phy_set_drvdata(phy, &priv->phys[phy_id]);
>>> +
>>> +		/* Make sure the PHY is off */
>>> +		phy_berlin_sata_power_off(phy);
>>> +
>>> +		phy_provider = devm_of_phy_provider_register(&phy->dev,
>>> +							     of_phy_simple_xlate);
>>> +		if (IS_ERR(phy_provider))
>>> +			return PTR_ERR(phy_provider);
>>
>> was this intentional? registering multiple PHY providers?
> 
> Yes. Each sub-node describe a PHY and register as a PHY provider. This
> allow to reference the PHY as <&sata_phy0> and not <&sata_phy 0>. It
> would be confusing to have a sub-node sata_phy0 and to use its parent
> to access it.

It is still a single PHY provider, so you can't register multiple PHY
providers. So in this case <&sata_phy 0> is not that bad.

However phy-core.c should be patched with the following (only compile tested)

From 2517d4c0ad7f13abc2613516ef2b222a1fbcb550 Mon Sep 17 00:00:00 2001
From: Kishon Vijay Abraham I <kishon@ti.com>
Date: Thu, 15 May 2014 11:32:57 +0530
Subject: [PATCH] phy: phy-core: Support modelling multi-PHY PHY nodes as
 subnodes of PHY PROVIDER

When a single PHY provider implementing multiple PHYs is represented in dt,
the PHYs should be modelled as sub nodes of the PHY PROVIDER node.

So the consumers using the PHY will have the phandle of the sub node rather
than the phandle of the PHY PROVIDER. Amended *of_phy_provider_lookup* in
PHY core to return the PHY PROVIDER even when the phandle of the PHY PROVIDERs
sub node is provided.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/phy/phy-core.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index 03cf8fb..124c6ec 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -83,10 +83,18 @@ static struct phy *phy_lookup(struct device *device, const
char *port)
 static struct phy_provider *of_phy_provider_lookup(struct device_node *node)
 {
        struct phy_provider *phy_provider;
+       struct device_node *child;

        list_for_each_entry(phy_provider, &phy_provider_list, list) {
-               if (phy_provider->dev->of_node == node)
+               if (phy_provider->dev->of_node != node) {
+                       for_each_child_of_node(phy_provider->dev->of_node,
+                               child) {
+                               if (child == node)
+                                       return phy_provider;
+                       }
+               } else {
                        return phy_provider;
+               }
        }

        return ERR_PTR(-EPROBE_DEFER);
-- 
1.7.9.5

Thanks
Kishon

Re: [PATCH v3 1/6] phy: add a driver for the Berlin SATA PHY

[Arnd Bergmann]

On Wednesday 14 May 2014 11:48:57 Antoine Ténart wrote:
> +static int phy_berlin_sata_power_on(struct phy *phy)
> +{
> +       struct phy_berlin_desc *desc = phy_get_drvdata(phy);
> +       struct phy_berlin_priv *priv = to_berlin_sata_phy_priv(desc);
> +       u32 regval;
> +
> +       spin_lock(&priv->lock);
> +
> +       /* Power up PHY */
> +       writel(CONTROL_REGISTER, priv->base + HOST_VSA_ADDR);
> +       regval = readl(priv->base + HOST_VSA_DATA);
> +       regval &= ~(desc->val);
> +       writel(regval, priv->base + HOST_VSA_DATA);
> +
> +       /* Configure MBus */
> +       writel(MBUS_SIZE_CONTROL, priv->base + HOST_VSA_ADDR);
> +       regval = readl(priv->base + HOST_VSA_DATA);
> +       regval |= MBUS_WRITE_REQUEST_SIZE_128 | MBUS_READ_REQUEST_SIZE_128;
> +       writel(regval, priv->base + HOST_VSA_DATA);
> +
> +       spin_unlock(&priv->lock);
> +
> +       return 0;
> +}
> +
> +static int phy_berlin_sata_power_off(struct phy *phy)
> +{
> +       struct phy_berlin_desc *desc = phy_get_drvdata(phy);
> +       struct phy_berlin_priv *priv = to_berlin_sata_phy_priv(desc);
> +       u32 regval;
> +
> +       spin_lock(&priv->lock);
> +
> +       /* Power down PHY */
> +       writel(CONTROL_REGISTER, priv->base + HOST_VSA_ADDR);
> +       regval = readl(priv->base + HOST_VSA_DATA);
> +       regval |= desc->val;
> +       writel(regval, priv->base + HOST_VSA_DATA);
> +
> +       spin_unlock(&priv->lock);
> +
> +       return 0;

I don't get this part: you have a reference to the phy here,
but then you go poking the phy registers from the SATA driver
rather than calling a PHY API function.


> +                * By default the PHY node is used to request and match a PHY.
> +                * We describe one PHY per sub-node here. Use the right node.
> +                */
> +               phy->dev.of_node = child;
> +
> +               priv->phys[phy_id].phy = phy;
> +               priv->phys[phy_id].val = desc[phy_id].val;
> +               priv->phys[phy_id].index = phy_id;
> +               phy_set_drvdata(phy, &priv->phys[phy_id]);

And here, you set a driver specific value into a structure used by the
PHY.

Both of these are layering violations. You should either use the PHY
interfaces correctly so the SATA driver doesn't have to know about the
specific, or not use a PHY device node at all and do everything in
the SATA front-end.

	Arnd

Re: [PATCH v3 1/6] phy: add a driver for the Berlin SATA PHY

[Antoine Ténart]

Arnd,

On Wed, May 14, 2014 at 03:02:34PM +0200, Arnd Bergmann wrote:
> On Wednesday 14 May 2014 11:48:57 Antoine Ténart wrote:
> > +static int phy_berlin_sata_power_on(struct phy *phy)
> > +{
> > +       struct phy_berlin_desc *desc = phy_get_drvdata(phy);
> > +       struct phy_berlin_priv *priv = to_berlin_sata_phy_priv(desc);
> > +       u32 regval;
> > +
> > +       spin_lock(&priv->lock);
> > +
> > +       /* Power up PHY */
> > +       writel(CONTROL_REGISTER, priv->base + HOST_VSA_ADDR);
> > +       regval = readl(priv->base + HOST_VSA_DATA);
> > +       regval &= ~(desc->val);
> > +       writel(regval, priv->base + HOST_VSA_DATA);
> > +
> > +       /* Configure MBus */
> > +       writel(MBUS_SIZE_CONTROL, priv->base + HOST_VSA_ADDR);
> > +       regval = readl(priv->base + HOST_VSA_DATA);
> > +       regval |= MBUS_WRITE_REQUEST_SIZE_128 | MBUS_READ_REQUEST_SIZE_128;
> > +       writel(regval, priv->base + HOST_VSA_DATA);
> > +
> > +       spin_unlock(&priv->lock);
> > +
> > +       return 0;
> > +}
> > +
> > +static int phy_berlin_sata_power_off(struct phy *phy)
> > +{
> > +       struct phy_berlin_desc *desc = phy_get_drvdata(phy);
> > +       struct phy_berlin_priv *priv = to_berlin_sata_phy_priv(desc);
> > +       u32 regval;
> > +
> > +       spin_lock(&priv->lock);
> > +
> > +       /* Power down PHY */
> > +       writel(CONTROL_REGISTER, priv->base + HOST_VSA_ADDR);
> > +       regval = readl(priv->base + HOST_VSA_DATA);
> > +       regval |= desc->val;
> > +       writel(regval, priv->base + HOST_VSA_DATA);
> > +
> > +       spin_unlock(&priv->lock);
> > +
> > +       return 0;
> 
> I don't get this part: you have a reference to the phy here,
> but then you go poking the phy registers from the SATA driver
> rather than calling a PHY API function.

The v1 only introduced an AHCI driver. I somewhat agree the PHY
operations done in the AHCI driver could be in there.

I can move the initialization done in the AHCI driver here, but I'll
still need the driver: the Berlin AHCI needs to call the framework
generic functions with a custom mask and has custom pm_ops. So I'll
end up with a nearly empty AHCI driver, not able to control the port
parameters.

Or I can put all this in the AHCI driver, but then we'll need to
describe the PHYs there (to be able to enable each PHY independently)
and add bindings to the SATA ones.

What do you think? I prefer the first solution, but we'll have SATA
port related configuration in the PHY and a very tiny AHCI driver
because I can't really use the default behaviour of the ahci_platform.

> > +                * By default the PHY node is used to request and match a PHY.
> > +                * We describe one PHY per sub-node here. Use the right node.
> > +                */
> > +               phy->dev.of_node = child;
> > +
> > +               priv->phys[phy_id].phy = phy;
> > +               priv->phys[phy_id].val = desc[phy_id].val;
> > +               priv->phys[phy_id].index = phy_id;
> > +               phy_set_drvdata(phy, &priv->phys[phy_id]);
> 
> And here, you set a driver specific value into a structure used by the
> PHY.

Values in priv->phys[] are related to the PHYs. phy_set_drvdata() allows
to store PHY related data, which is what I'm doing there. Nearly all PHY
drivers are doing this.

Or am I missing something?

> 
> Both of these are layering violations. You should either use the PHY
> interfaces correctly so the SATA driver doesn't have to know about the
> specific, or not use a PHY device node at all and do everything in
> the SATA front-end.

To be sure: you mean using the PHY init() interface in the AHCI driver?

Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH v3 1/6] phy: add a driver for the Berlin SATA PHY

[Arnd Bergmann]

On Wednesday 14 May 2014 16:50:02 Antoine Ténart wrote:
> On Wed, May 14, 2014 at 03:02:34PM +0200, Arnd Bergmann wrote:
> > On Wednesday 14 May 2014 11:48:57 Antoine Ténart wrote:
> > > +static int phy_berlin_sata_power_on(struct phy *phy)
> > > +{
> > > +       struct phy_berlin_desc *desc = phy_get_drvdata(phy);
> > > +       struct phy_berlin_priv *priv = to_berlin_sata_phy_priv(desc);
> > > +       u32 regval;
> > > +
> > > +       spin_lock(&priv->lock);
> > > +
> > > +       /* Power up PHY */
> > > +       writel(CONTROL_REGISTER, priv->base + HOST_VSA_ADDR);
> > > +       regval = readl(priv->base + HOST_VSA_DATA);
> > > +       regval &= ~(desc->val);
> > > +       writel(regval, priv->base + HOST_VSA_DATA);
> > > +
> > > +       /* Configure MBus */
> > > +       writel(MBUS_SIZE_CONTROL, priv->base + HOST_VSA_ADDR);
> > > +       regval = readl(priv->base + HOST_VSA_DATA);
> > > +       regval |= MBUS_WRITE_REQUEST_SIZE_128 | MBUS_READ_REQUEST_SIZE_128;
> > > +       writel(regval, priv->base + HOST_VSA_DATA);
> > > +
> > > +       spin_unlock(&priv->lock);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int phy_berlin_sata_power_off(struct phy *phy)
> > > +{
> > > +       struct phy_berlin_desc *desc = phy_get_drvdata(phy);
> > > +       struct phy_berlin_priv *priv = to_berlin_sata_phy_priv(desc);
> > > +       u32 regval;
> > > +
> > > +       spin_lock(&priv->lock);
> > > +
> > > +       /* Power down PHY */
> > > +       writel(CONTROL_REGISTER, priv->base + HOST_VSA_ADDR);
> > > +       regval = readl(priv->base + HOST_VSA_DATA);
> > > +       regval |= desc->val;
> > > +       writel(regval, priv->base + HOST_VSA_DATA);
> > > +
> > > +       spin_unlock(&priv->lock);
> > > +
> > > +       return 0;
> > 
> > I don't get this part: you have a reference to the phy here,
> > but then you go poking the phy registers from the SATA driver
> > rather than calling a PHY API function.
> 
> The v1 only introduced an AHCI driver. I somewhat agree the PHY
> operations done in the AHCI driver could be in there.
> 
> I can move the initialization done in the AHCI driver here, but I'll
> still need the driver: the Berlin AHCI needs to call the framework
> generic functions with a custom mask and has custom pm_ops. So I'll
> end up with a nearly empty AHCI driver, not able to control the port
> parameters.
> 
> Or I can put all this in the AHCI driver, but then we'll need to
> describe the PHYs there (to be able to enable each PHY independently)
> and add bindings to the SATA ones.
> 
> What do you think? I prefer the first solution, but we'll have SATA
> port related configuration in the PHY and a very tiny AHCI driver
> because I can't really use the default behaviour of the ahci_platform.

I just noticed I quoted the wrong driver with my comment, but I think
you got what I meant.

Why do you need a custom mask? Is that something you could pass
as the argument in the phy descriptor using #phy-cells=<1>?

> > > +                * By default the PHY node is used to request and match a PHY.
> > > +                * We describe one PHY per sub-node here. Use the right node.
> > > +                */
> > > +               phy->dev.of_node = child;
> > > +
> > > +               priv->phys[phy_id].phy = phy;
> > > +               priv->phys[phy_id].val = desc[phy_id].val;
> > > +               priv->phys[phy_id].index = phy_id;
> > > +               phy_set_drvdata(phy, &priv->phys[phy_id]);
> > 
> > And here, you set a driver specific value into a structure used by the
> > PHY.
> 
> Values in priv->phys[] are related to the PHYs. phy_set_drvdata() allows
> to store PHY related data, which is what I'm doing there. Nearly all PHY
> drivers are doing this.
> 
> Or am I missing something?

This part is really ok, I got confused when I replied to the wrong email.
Sorry about this.

> > Both of these are layering violations. You should either use the PHY
> > interfaces correctly so the SATA driver doesn't have to know about the
> > specific, or not use a PHY device node at all and do everything in
> > the SATA front-end.
> 
> To be sure: you mean using the PHY init() interface in the AHCI driver?

If this PHY is specific to the ahci-berlin hardware and not shared with
anything else, you don't really need to split out a phy driver. That
would somewhat simplify what you ahve here.

The alternative is to make it as generic as you can. If you can manage
to move all the phy code into phy-berlin-sata driver, it should be
possible to just extend the ahci-platform driver resume function to
reinitialize the phy if there is one.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/6] phy: add a driver for the Berlin SATA PHY

[Antoine Ténart]

On Wed, May 14, 2014 at 05:31:24PM +0200, Arnd Bergmann wrote:
> On Wednesday 14 May 2014 16:50:02 Antoine Ténart wrote:
> > On Wed, May 14, 2014 at 03:02:34PM +0200, Arnd Bergmann wrote:
> > > On Wednesday 14 May 2014 11:48:57 Antoine Ténart wrote:
> > > > +static int phy_berlin_sata_power_on(struct phy *phy)
> > > > +{
> > > > +       struct phy_berlin_desc *desc = phy_get_drvdata(phy);
> > > > +       struct phy_berlin_priv *priv = to_berlin_sata_phy_priv(desc);
> > > > +       u32 regval;
> > > > +
> > > > +       spin_lock(&priv->lock);
> > > > +
> > > > +       /* Power up PHY */
> > > > +       writel(CONTROL_REGISTER, priv->base + HOST_VSA_ADDR);
> > > > +       regval = readl(priv->base + HOST_VSA_DATA);
> > > > +       regval &= ~(desc->val);
> > > > +       writel(regval, priv->base + HOST_VSA_DATA);
> > > > +
> > > > +       /* Configure MBus */
> > > > +       writel(MBUS_SIZE_CONTROL, priv->base + HOST_VSA_ADDR);
> > > > +       regval = readl(priv->base + HOST_VSA_DATA);
> > > > +       regval |= MBUS_WRITE_REQUEST_SIZE_128 | MBUS_READ_REQUEST_SIZE_128;
> > > > +       writel(regval, priv->base + HOST_VSA_DATA);
> > > > +
> > > > +       spin_unlock(&priv->lock);
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +static int phy_berlin_sata_power_off(struct phy *phy)
> > > > +{
> > > > +       struct phy_berlin_desc *desc = phy_get_drvdata(phy);
> > > > +       struct phy_berlin_priv *priv = to_berlin_sata_phy_priv(desc);
> > > > +       u32 regval;
> > > > +
> > > > +       spin_lock(&priv->lock);
> > > > +
> > > > +       /* Power down PHY */
> > > > +       writel(CONTROL_REGISTER, priv->base + HOST_VSA_ADDR);
> > > > +       regval = readl(priv->base + HOST_VSA_DATA);
> > > > +       regval |= desc->val;
> > > > +       writel(regval, priv->base + HOST_VSA_DATA);
> > > > +
> > > > +       spin_unlock(&priv->lock);
> > > > +
> > > > +       return 0;
> > > 
> > > I don't get this part: you have a reference to the phy here,
> > > but then you go poking the phy registers from the SATA driver
> > > rather than calling a PHY API function.
> > 
> > The v1 only introduced an AHCI driver. I somewhat agree the PHY
> > operations done in the AHCI driver could be in there.
> > 
> > I can move the initialization done in the AHCI driver here, but I'll
> > still need the driver: the Berlin AHCI needs to call the framework
> > generic functions with a custom mask and has custom pm_ops. So I'll
> > end up with a nearly empty AHCI driver, not able to control the port
> > parameters.
> > 
> > Or I can put all this in the AHCI driver, but then we'll need to
> > describe the PHYs there (to be able to enable each PHY independently)
> > and add bindings to the SATA ones.
> > 
> > What do you think? I prefer the first solution, but we'll have SATA
> > port related configuration in the PHY and a very tiny AHCI driver
> > because I can't really use the default behaviour of the ahci_platform.
> 
> I just noticed I quoted the wrong driver with my comment, but I think
> you got what I meant.
> 
> Why do you need a custom mask? Is that something you could pass
> as the argument in the phy descriptor using #phy-cells=<1>?

I meant a custom mask in the AHCI driver, when calling the
ahci_platform_init_host() function. Otherwise we'll have problems on the
BG2Q DMP (it only has one PHY available, and not initializing it is not
enough).

> 
> > > > +                * By default the PHY node is used to request and match a PHY.
> > > > +                * We describe one PHY per sub-node here. Use the right node.
> > > > +                */
> > > > +               phy->dev.of_node = child;
> > > > +
> > > > +               priv->phys[phy_id].phy = phy;
> > > > +               priv->phys[phy_id].val = desc[phy_id].val;
> > > > +               priv->phys[phy_id].index = phy_id;
> > > > +               phy_set_drvdata(phy, &priv->phys[phy_id]);
> > > 
> > > And here, you set a driver specific value into a structure used by the
> > > PHY.
> > 
> > Values in priv->phys[] are related to the PHYs. phy_set_drvdata() allows
> > to store PHY related data, which is what I'm doing there. Nearly all PHY
> > drivers are doing this.
> > 
> > Or am I missing something?
> 
> This part is really ok, I got confused when I replied to the wrong email.
> Sorry about this.
> 
> > > Both of these are layering violations. You should either use the PHY
> > > interfaces correctly so the SATA driver doesn't have to know about the
> > > specific, or not use a PHY device node at all and do everything in
> > > the SATA front-end.
> > 
> > To be sure: you mean using the PHY init() interface in the AHCI driver?
> 
> If this PHY is specific to the ahci-berlin hardware and not shared with
> anything else, you don't really need to split out a phy driver. That
> would somewhat simplify what you ahve here.

I don't have lots of info about that, but we set the PHY to
PHY_MODE_SATA in the AHCI driver. So I guess there are other modes.

Maybe Jisheng can help us with this?

> 
> The alternative is to make it as generic as you can. If you can manage
> to move all the phy code into phy-berlin-sata driver, it should be
> possible to just extend the ahci-platform driver resume function to
> reinitialize the phy if there is one.

It is possible to move all the PHY code the phy-berlin-sata. Then I'll
need to hack a bit the AHCI framework so it can handle more than one
PHY. But as I said, I'll still need to set a custom mask, and adding a
quirk to the AHCI platform or framework does not seem to be a very good
idea, imho.

Or I can add a computed mask to the ahci-platform driver, like I did in
the Berlin one, but I don't know what would be the consequences. For
each PHY I have:

+	mask |= 1 << i;


Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH v3 1/6] phy: add a driver for the Berlin SATA PHY

[Arnd Bergmann]

On Wednesday 14 May 2014 17:49:29 Antoine Ténart wrote:
> On Wed, May 14, 2014 at 05:31:24PM +0200, Arnd Bergmann wrote:
> > On Wednesday 14 May 2014 16:50:02 Antoine Ténart wrote:
> > > On Wed, May 14, 2014 at 03:02:34PM +0200, Arnd Bergmann wrote:
> > > > On Wednesday 14 May 2014 11:48:57 Antoine Ténart wrote:
> > > > I don't get this part: you have a reference to the phy here,
> > > > but then you go poking the phy registers from the SATA driver
> > > > rather than calling a PHY API function.
> > > 
> > > The v1 only introduced an AHCI driver. I somewhat agree the PHY
> > > operations done in the AHCI driver could be in there.
> > > 
> > > I can move the initialization done in the AHCI driver here, but I'll
> > > still need the driver: the Berlin AHCI needs to call the framework
> > > generic functions with a custom mask and has custom pm_ops. So I'll
> > > end up with a nearly empty AHCI driver, not able to control the port
> > > parameters.
> > > 
> > > Or I can put all this in the AHCI driver, but then we'll need to
> > > describe the PHYs there (to be able to enable each PHY independently)
> > > and add bindings to the SATA ones.
> > > 
> > > What do you think? I prefer the first solution, but we'll have SATA
> > > port related configuration in the PHY and a very tiny AHCI driver
> > > because I can't really use the default behaviour of the ahci_platform.
> > 
> > I just noticed I quoted the wrong driver with my comment, but I think
> > you got what I meant.
> > 
> > Why do you need a custom mask? Is that something you could pass
> > as the argument in the phy descriptor using #phy-cells=<1>?
> 
> I meant a custom mask in the AHCI driver, when calling the
> ahci_platform_init_host() function. Otherwise we'll have problems on the
> BG2Q DMP (it only has one PHY available, and not initializing it is not
> enough).

Ah, I see what you mean now. Of course, this could also be an optional
property in the generic AHCI binding, which would require that you
specify the available ports in DT for nonstandard masks.

Just to confirm: The HOST_PORTS_IMPL register on BG2Q DMP has an
value that we can't use here to determine the available ports, right?

> > > > > +                * By default the PHY node is used to request and match a PHY.
> > > > > +                * We describe one PHY per sub-node here. Use the right node.
> > > > > +                */
> > > > > +               phy->dev.of_node = child;
> > > > > +
> > > > > +               priv->phys[phy_id].phy = phy;
> > > > > +               priv->phys[phy_id].val = desc[phy_id].val;
> > > > > +               priv->phys[phy_id].index = phy_id;
> > > > > +               phy_set_drvdata(phy, &priv->phys[phy_id]);
> > > > 
> > > > And here, you set a driver specific value into a structure used by the
> > > > PHY.
> > > 
> > > Values in priv->phys[] are related to the PHYs. phy_set_drvdata() allows
> > > to store PHY related data, which is what I'm doing there. Nearly all PHY
> > > drivers are doing this.
> > > 
> > > Or am I missing something?
> > 
> > This part is really ok, I got confused when I replied to the wrong email.
> > Sorry about this.
> > 
> > > > Both of these are layering violations. You should either use the PHY
> > > > interfaces correctly so the SATA driver doesn't have to know about the
> > > > specific, or not use a PHY device node at all and do everything in
> > > > the SATA front-end.
> > > 
> > > To be sure: you mean using the PHY init() interface in the AHCI driver?
> > 
> > If this PHY is specific to the ahci-berlin hardware and not shared with
> > anything else, you don't really need to split out a phy driver. That
> > would somewhat simplify what you ahve here.
> 
> I don't have lots of info about that, but we set the PHY to
> PHY_MODE_SATA in the AHCI driver. So I guess there are other modes.

Ok, good point. This does sound a lot like there are multiple modes.

This would also imply that the "compatible" string should not have
"sata" in it either, but describe a generic PHY. You can pass the mode
as part of the phy descriptor from DT then.

> > The alternative is to make it as generic as you can. If you can manage
> > to move all the phy code into phy-berlin-sata driver, it should be
> > possible to just extend the ahci-platform driver resume function to
> > reinitialize the phy if there is one.
> 
> It is possible to move all the PHY code the phy-berlin-sata. Then I'll
> need to hack a bit the AHCI framework so it can handle more than one
> PHY. But as I said, I'll still need to set a custom mask, and adding a
> quirk to the AHCI platform or framework does not seem to be a very good
> idea, imho.
> 
> Or I can add a computed mask to the ahci-platform driver, like I did in
> the Berlin one, but I don't know what would be the consequences. For
> each PHY I have:
> 
> +	mask |= 1 << i;

It does sound generic enough that the same would be done on other
ahci-platform variants with a generic phy attached, even though
currently each one has only one phy.

We can probably define this as an extension in some form.
Do you think we need to handle the case where the first port is
unavailable but some of the other ports should be used?

	Arnd

Re: [PATCH v3 1/6] phy: add a driver for the Berlin SATA PHY

[Antoine Ténart]

On Wed, May 14, 2014 at 06:11:24PM +0200, Arnd Bergmann wrote:
> On Wednesday 14 May 2014 17:49:29 Antoine Ténart wrote:
> > On Wed, May 14, 2014 at 05:31:24PM +0200, Arnd Bergmann wrote:
> > > On Wednesday 14 May 2014 16:50:02 Antoine Ténart wrote:
> > > > On Wed, May 14, 2014 at 03:02:34PM +0200, Arnd Bergmann wrote:
> > > > > On Wednesday 14 May 2014 11:48:57 Antoine Ténart wrote:
> > > > > I don't get this part: you have a reference to the phy here,
> > > > > but then you go poking the phy registers from the SATA driver
> > > > > rather than calling a PHY API function.
> > > > 
> > > > The v1 only introduced an AHCI driver. I somewhat agree the PHY
> > > > operations done in the AHCI driver could be in there.
> > > > 
> > > > I can move the initialization done in the AHCI driver here, but I'll
> > > > still need the driver: the Berlin AHCI needs to call the framework
> > > > generic functions with a custom mask and has custom pm_ops. So I'll
> > > > end up with a nearly empty AHCI driver, not able to control the port
> > > > parameters.
> > > > 
> > > > Or I can put all this in the AHCI driver, but then we'll need to
> > > > describe the PHYs there (to be able to enable each PHY independently)
> > > > and add bindings to the SATA ones.
> > > > 
> > > > What do you think? I prefer the first solution, but we'll have SATA
> > > > port related configuration in the PHY and a very tiny AHCI driver
> > > > because I can't really use the default behaviour of the ahci_platform.
> > > 
> > > I just noticed I quoted the wrong driver with my comment, but I think
> > > you got what I meant.
> > > 
> > > Why do you need a custom mask? Is that something you could pass
> > > as the argument in the phy descriptor using #phy-cells=<1>?
> > 
> > I meant a custom mask in the AHCI driver, when calling the
> > ahci_platform_init_host() function. Otherwise we'll have problems on the
> > BG2Q DMP (it only has one PHY available, and not initializing it is not
> > enough).
> 
> Ah, I see what you mean now. Of course, this could also be an optional
> property in the generic AHCI binding, which would require that you
> specify the available ports in DT for nonstandard masks.
> 
> Just to confirm: The HOST_PORTS_IMPL register on BG2Q DMP has an
> value that we can't use here to determine the available ports, right?

Right. The register is here but saying there are 2 available ports,
which is true for the BG2Q SoC but not true for the BG2Q DMP board. So
the AHCI framework reads the register and performs a misconfiguration of
the SATA.

> 
> > > > > > +                * By default the PHY node is used to request and match a PHY.
> > > > > > +                * We describe one PHY per sub-node here. Use the right node.
> > > > > > +                */
> > > > > > +               phy->dev.of_node = child;
> > > > > > +
> > > > > > +               priv->phys[phy_id].phy = phy;
> > > > > > +               priv->phys[phy_id].val = desc[phy_id].val;
> > > > > > +               priv->phys[phy_id].index = phy_id;
> > > > > > +               phy_set_drvdata(phy, &priv->phys[phy_id]);
> > > > > 
> > > > > And here, you set a driver specific value into a structure used by the
> > > > > PHY.
> > > > 
> > > > Values in priv->phys[] are related to the PHYs. phy_set_drvdata() allows
> > > > to store PHY related data, which is what I'm doing there. Nearly all PHY
> > > > drivers are doing this.
> > > > 
> > > > Or am I missing something?
> > > 
> > > This part is really ok, I got confused when I replied to the wrong email.
> > > Sorry about this.
> > > 
> > > > > Both of these are layering violations. You should either use the PHY
> > > > > interfaces correctly so the SATA driver doesn't have to know about the
> > > > > specific, or not use a PHY device node at all and do everything in
> > > > > the SATA front-end.
> > > > 
> > > > To be sure: you mean using the PHY init() interface in the AHCI driver?
> > > 
> > > If this PHY is specific to the ahci-berlin hardware and not shared with
> > > anything else, you don't really need to split out a phy driver. That
> > > would somewhat simplify what you ahve here.
> > 
> > I don't have lots of info about that, but we set the PHY to
> > PHY_MODE_SATA in the AHCI driver. So I guess there are other modes.
> 
> Ok, good point. This does sound a lot like there are multiple modes.
> 
> This would also imply that the "compatible" string should not have
> "sata" in it either, but describe a generic PHY. You can pass the mode
> as part of the phy descriptor from DT then.

Yes sure, with something like:

	phys = <&foo_phy0 FOO_SATA>;

> 
> > > The alternative is to make it as generic as you can. If you can manage
> > > to move all the phy code into phy-berlin-sata driver, it should be
> > > possible to just extend the ahci-platform driver resume function to
> > > reinitialize the phy if there is one.
> > 
> > It is possible to move all the PHY code the phy-berlin-sata. Then I'll
> > need to hack a bit the AHCI framework so it can handle more than one
> > PHY. But as I said, I'll still need to set a custom mask, and adding a
> > quirk to the AHCI platform or framework does not seem to be a very good
> > idea, imho.
> > 
> > Or I can add a computed mask to the ahci-platform driver, like I did in
> > the Berlin one, but I don't know what would be the consequences. For
> > each PHY I have:
> > 
> > +	mask |= 1 << i;
> 
> It does sound generic enough that the same would be done on other
> ahci-platform variants with a generic phy attached, even though
> currently each one has only one phy.
> 
> We can probably define this as an extension in some form.
> Do you think we need to handle the case where the first port is
> unavailable but some of the other ports should be used?

The Berlin AHCI driver handles the case. I don't know if we'll come
across a use case for this but supporting it should'nt be difficult. So
why not ... :)

I think we can either add a binding enabling an automatic mask computation
in the framework or match the compatible and do this in the ahci-platform
driver. Or we can make this the default behaviour when multiple PHYs are
used in an AHCI node, but I'll have to check if that's possible.

Maybe the SATA subsystem maintainer has an opinion on the matter?


Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH v3 1/6] phy: add a driver for the Berlin SATA PHY

[Sebastian Hesselbarth]

On 05/14/2014 06:57 PM, Antoine Ténart wrote:
> On Wed, May 14, 2014 at 06:11:24PM +0200, Arnd Bergmann wrote:
>> On Wednesday 14 May 2014 17:49:29 Antoine Ténart wrote:
>>> On Wed, May 14, 2014 at 05:31:24PM +0200, Arnd Bergmann wrote:
>>>> Why do you need a custom mask? Is that something you could pass
>>>> as the argument in the phy descriptor using #phy-cells=<1>?
>>>
>>> I meant a custom mask in the AHCI driver, when calling the
>>> ahci_platform_init_host() function. Otherwise we'll have problems on the
>>> BG2Q DMP (it only has one PHY available, and not initializing it is not
>>> enough).
>>
>> Ah, I see what you mean now. Of course, this could also be an optional
>> property in the generic AHCI binding, which would require that you
>> specify the available ports in DT for nonstandard masks.
>>
>> Just to confirm: The HOST_PORTS_IMPL register on BG2Q DMP has an
>> value that we can't use here to determine the available ports, right?
> 
> Right. The register is here but saying there are 2 available ports,
> which is true for the BG2Q SoC but not true for the BG2Q DMP board. So
> the AHCI framework reads the register and performs a misconfiguration of
> the SATA.

*Disclaimer:* All PHY IP related comments below are *pure* guessing. I
have no clue how that PHY really looks like or how it is wired up.

I think it is time to sum this up a little bit and help Antoine carry
on this patches.

From what I understand from the conversation, we have a single PHY
register set dealing with both SATA ports available on the SoC.
Also, from the name of the PHY bits we assume the PHY may be able
to work in different modes than just SATA. And we currently have
an AHCI-compatible SATA IP that supports up to two ports, with one
actually connected to a SATA plug on the DMP board.

Now, thinking about the PHY binding and the (possible) multi-protocol
support, it can be possible that on BG2Q there is a generic 2-lane
LVDS PHY that can be configured to support SATA or PCIe. Both are
electrically and bit-level compatible, so they could be internally
wired-up with AHCI and PCIe controller.

From a DT point-of-view, we need a way to (a) link each SATA or PCIe
port to the PHY, (b) specify the PHY lane to be used, and (c) specify
the protocol to be used on that lane. If I got it right, Arnd already
mentioned to use the phy-specifier to deal with it:

e.g. phy = <&genphy 0 MODE_SATA> or phy = <&genphy 1 MODE_PCIE>

Let's assume we have one dual-port SATA controller and one PCIe
controller with either x1 or x2 support. The only sane DT binding,
I can think of then would be:

berlin2q.dtsi:

genphy: lvds@ea00ff {
	compatible = "marvell,berlin-lvds-phy";
	reg = <0xea00ff 0x100>;
	#phy-cells = <2>;
};

sata: sata@ab00ff {
	compatible = "ahci-platform";
	reg = <0xab00ff 0x100>;
	
	sata0: sata-port@0 {
		reg = <0>;
		phy = <&genphy 0 MODE_SATA>;
		status = "disabled";
	};

	sata1: sata-port@1 {
		reg = <1>;
		phy = <&genphy 1 MODE_SATA>;
		status = "disabled";
	};
};

pcie: pcie@ab01ff {
	compatible = "marvell,berlin-pcie";
	reg = <0xab01ff 0x100>;

	pcie0: pcie-port@0 {
		reg = <0>;
		/* set phy on a per-board basis */
		/* PCIe x1 on Lane 0 : phy = <&genphy 0 MODE_PCIE>; */
		/* PCIe x2 on Lane 0 and 1 : phy = <&genphy 0 MODE_PCIE>, <&genphy 1
MODE_PCIE>; */
		status = "disabled";
	};
};

berlin2q-dmp.dts:

&sata1 {
	status = "okay";
};

&pcie0 {
	phy = <&genphy 1 MODE_PCIE>;
};

berlin2q-foo.dts:

&pcie0 {
	phy = <&genphy 0 MODE_PCIE>, <&genphy 1 MODE_PCIE>;
};

For the driver, Antoine then would have to squeeze all PHY register
mangling in phy-berlin2.c and see how to make ahci-platform aware of
individual port nodes (I haven't looked up if it already exists, sorry)
and announce only enabled port child nodes, right?

Sebastian

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/6] phy: add a driver for the Berlin SATA PHY

[Arnd Bergmann]

On Wednesday 14 May 2014 19:57:46 Sebastian Hesselbarth wrote:
> On 05/14/2014 06:57 PM, Antoine Ténart wrote:
> > On Wed, May 14, 2014 at 06:11:24PM +0200, Arnd Bergmann wrote:
> >> On Wednesday 14 May 2014 17:49:29 Antoine Ténart wrote:
> >>> On Wed, May 14, 2014 at 05:31:24PM +0200, Arnd Bergmann wrote:
> 
> From what I understand from the conversation, we have a single PHY
> register set dealing with both SATA ports available on the SoC.
> Also, from the name of the PHY bits we assume the PHY may be able
> to work in different modes than just SATA. And we currently have
> an AHCI-compatible SATA IP that supports up to two ports, with one
> actually connected to a SATA plug on the DMP board.
> 
> Now, thinking about the PHY binding and the (possible) multi-protocol
> support, it can be possible that on BG2Q there is a generic 2-lane
> LVDS PHY that can be configured to support SATA or PCIe. Both are
> electrically and bit-level compatible, so they could be internally
> wired-up with AHCI and PCIe controller.

Sounds like a reasonable guess. We have other PHY drivers doing the
same thing already.

> From a DT point-of-view, we need a way to (a) link each SATA or PCIe
> port to the PHY, (b) specify the PHY lane to be used, and (c) specify
> the protocol to be used on that lane. If I got it right, Arnd already
> mentioned to use the phy-specifier to deal with it:
> 
> e.g. phy = <&genphy 0 MODE_SATA> or phy = <&genphy 1 MODE_PCIE>

Right.

> Let's assume we have one dual-port SATA controller and one PCIe
> controller with either x1 or x2 support. The only sane DT binding,
> I can think of then would be:
> 
> berlin2q.dtsi:
> 
> genphy: lvds@ea00ff {
> 	compatible = "marvell,berlin-lvds-phy";
> 	reg = <0xea00ff 0x100>;
> 	#phy-cells = <2>;
> };
> 
> sata: sata@ab00ff {
> 	compatible = "ahci-platform";
> 	reg = <0xab00ff 0x100>;
> 	
> 	sata0: sata-port@0 {
> 		reg = <0>;
> 		phy = <&genphy 0 MODE_SATA>;
> 		status = "disabled";
> 	};
> 
> 	sata1: sata-port@1 {
> 		reg = <1>;
> 		phy = <&genphy 1 MODE_SATA>;
> 		status = "disabled";
> 	};
> };
> 
> pcie: pcie@ab01ff {
> 	compatible = "marvell,berlin-pcie";
> 	reg = <0xab01ff 0x100>;
> 
> 	pcie0: pcie-port@0 {
> 		reg = <0>;
> 		/* set phy on a per-board basis */
> 		/* PCIe x1 on Lane 0 : phy = <&genphy 0 MODE_PCIE>; */
> 		/* PCIe x2 on Lane 0 and 1 : phy = <&genphy 0 MODE_PCIE>, <&genphy 1
> MODE_PCIE>; */
> 		status = "disabled";
> 	};
> };
> 
> berlin2q-dmp.dts:
> 
> &sata1 {
> 	status = "okay";
> };
> 
> &pcie0 {
> 	phy = <&genphy 1 MODE_PCIE>;
> };
> 
> berlin2q-foo.dts:
> 
> &pcie0 {
> 	phy = <&genphy 0 MODE_PCIE>, <&genphy 1 MODE_PCIE>;
> };

Exactly. I would also be fine with keeping the sub-nodes of the
phy device as in v3 and using #phy-cells=<1> instead of #phy-cells.
The result would be pretty much the same, it just depends on how
closely connected the two logical phys are.

IIRC for the ST microelectronics PHY we recently reviewed, the
same PHY could be driving multiple SATA ports, or a single
multi-lane PCIe. In that case, it makes more sense to have
#phy-cells=<2>, like your example.
 
> For the driver, Antoine then would have to squeeze all PHY register
> mangling in phy-berlin2.c and see how to make ahci-platform aware of
> individual port nodes (I haven't looked up if it already exists, sorry)
> and announce only enabled port child nodes, right?

I've been thinking some more about this aspect. I don't actually have
a strong opinion on whether it's better to use the generic ahci-platform
driver, or to keep the multi-phy support as a special variant for
berlin. If we do the latter, it would however be good to define the
binding in a way that lets us later merge things into the generic phy
driver in case we get more of the same.

	Arnd

Re: [PATCH v3 1/6] phy: add a driver for the Berlin SATA PHY

[Sebastian Hesselbarth]

On 05/14/2014 08:12 PM, Arnd Bergmann wrote:
> On Wednesday 14 May 2014 19:57:46 Sebastian Hesselbarth wrote:
>> On 05/14/2014 06:57 PM, Antoine Ténart wrote:
>>> On Wed, May 14, 2014 at 06:11:24PM +0200, Arnd Bergmann wrote:
>>>> On Wednesday 14 May 2014 17:49:29 Antoine Ténart wrote:
>>>>> On Wed, May 14, 2014 at 05:31:24PM +0200, Arnd Bergmann wrote:
>>
>> From what I understand from the conversation, we have a single PHY
>> register set dealing with both SATA ports available on the SoC.
>> Also, from the name of the PHY bits we assume the PHY may be able
>> to work in different modes than just SATA. And we currently have
>> an AHCI-compatible SATA IP that supports up to two ports, with one
>> actually connected to a SATA plug on the DMP board.
>>
>> Now, thinking about the PHY binding and the (possible) multi-protocol
>> support, it can be possible that on BG2Q there is a generic 2-lane
>> LVDS PHY that can be configured to support SATA or PCIe. Both are
>> electrically and bit-level compatible, so they could be internally
>> wired-up with AHCI and PCIe controller.
> 
> Sounds like a reasonable guess. We have other PHY drivers doing the
> same thing already.

Well, I based that on what I know about FPGA LVDS transceivers, so
I wasn't guessing out of the blue ;)

>> From a DT point-of-view, we need a way to (a) link each SATA or PCIe
>> port to the PHY, (b) specify the PHY lane to be used, and (c) specify
>> the protocol to be used on that lane. If I got it right, Arnd already
>> mentioned to use the phy-specifier to deal with it:
>>
>> e.g. phy = <&genphy 0 MODE_SATA> or phy = <&genphy 1 MODE_PCIE>
> 
> Right.
> 
>> Let's assume we have one dual-port SATA controller and one PCIe
>> controller with either x1 or x2 support. The only sane DT binding,
>> I can think of then would be:
>>
>> berlin2q.dtsi:
>>
>> genphy: lvds@ea00ff {
>> 	compatible = "marvell,berlin-lvds-phy";
>> 	reg = <0xea00ff 0x100>;
>> 	#phy-cells = <2>;
>> };
>>
>> sata: sata@ab00ff {
>> 	compatible = "ahci-platform";
>> 	reg = <0xab00ff 0x100>;
>> 	
>> 	sata0: sata-port@0 {
>> 		reg = <0>;
>> 		phy = <&genphy 0 MODE_SATA>;
>> 		status = "disabled";
>> 	};
>>
>> 	sata1: sata-port@1 {
>> 		reg = <1>;
>> 		phy = <&genphy 1 MODE_SATA>;
>> 		status = "disabled";
>> 	};
>> };
>>
>> pcie: pcie@ab01ff {
>> 	compatible = "marvell,berlin-pcie";
>> 	reg = <0xab01ff 0x100>;
>>
>> 	pcie0: pcie-port@0 {
>> 		reg = <0>;
>> 		/* set phy on a per-board basis */
>> 		/* PCIe x1 on Lane 0 : phy = <&genphy 0 MODE_PCIE>; */
>> 		/* PCIe x2 on Lane 0 and 1 : phy = <&genphy 0 MODE_PCIE>, <&genphy 1
>> MODE_PCIE>; */
>> 		status = "disabled";
>> 	};
>> };
>>
>> berlin2q-dmp.dts:
>>
>> &sata1 {
>> 	status = "okay";
>> };
>>
>> &pcie0 {
>> 	phy = <&genphy 1 MODE_PCIE>;
>> };
>>
>> berlin2q-foo.dts:
>>
>> &pcie0 {
>> 	phy = <&genphy 0 MODE_PCIE>, <&genphy 1 MODE_PCIE>;
>> };
> 
> Exactly. I would also be fine with keeping the sub-nodes of the
> phy device as in v3 and using #phy-cells=<1> instead of #phy-cells.
> The result would be pretty much the same, it just depends on how
> closely connected the two logical phys are.
> 
> IIRC for the ST microelectronics PHY we recently reviewed, the
> same PHY could be driving multiple SATA ports, or a single
> multi-lane PCIe. In that case, it makes more sense to have
> #phy-cells=<2>, like your example.

I guess the final call is up to Kishon, but I personally prefer
#phy-cells = <2>. It makes no functional difference though.

>> For the driver, Antoine then would have to squeeze all PHY register
>> mangling in phy-berlin2.c and see how to make ahci-platform aware of
>> individual port nodes (I haven't looked up if it already exists, sorry)
>> and announce only enabled port child nodes, right?
> 
> I've been thinking some more about this aspect. I don't actually have
> a strong opinion on whether it's better to use the generic ahci-platform
> driver, or to keep the multi-phy support as a special variant for
> berlin. If we do the latter, it would however be good to define the
> binding in a way that lets us later merge things into the generic phy
> driver in case we get more of the same.

Hmm, IMHO multi-phy support is orthogonal to ahci-platform, isn't it?
ahci-platform needs to know about the phy property and calls some
helper that deals with the phy-specifier?

About a generic _phy_ driver, I am not so sure if berlin is the best
template right now ;)

So, my call would be:
- make ahci-platform aware of port sub-nodes and phy properties
- have a berlin specific PHY driver

Sebastian

Re: [PATCH v3 1/6] phy: add a driver for the Berlin SATA PHY

[Arnd Bergmann]

On Wednesday 14 May 2014 20:42:16 Sebastian Hesselbarth wrote:
> >> For the driver, Antoine then would have to squeeze all PHY register
> >> mangling in phy-berlin2.c and see how to make ahci-platform aware of
> >> individual port nodes (I haven't looked up if it already exists, sorry)
> >> and announce only enabled port child nodes, right?
> > 
> > I've been thinking some more about this aspect. I don't actually have
> > a strong opinion on whether it's better to use the generic ahci-platform
> > driver, or to keep the multi-phy support as a special variant for
> > berlin. If we do the latter, it would however be good to define the
> > binding in a way that lets us later merge things into the generic phy
> > driver in case we get more of the same.
> 
> Hmm, IMHO multi-phy support is orthogonal to ahci-platform, isn't it?
> ahci-platform needs to know about the phy property and calls some
> helper that deals with the phy-specifier?
> 
> About a generic _phy_ driver, I am not so sure if berlin is the best
> template right now 
> 
> So, my call would be:
> - make ahci-platform aware of port sub-nodes and phy properties
> - have a berlin specific PHY driver

I'm not sure if we need sub-nodes per port, it should be enough
to have an array of phys, plus a way to match them up with the
ports.

	Arnd

Re: [PATCH v3 1/6] phy: add a driver for the Berlin SATA PHY

[Sebastian Hesselbarth]

On 05/14/2014 08:51 PM, Arnd Bergmann wrote:
> On Wednesday 14 May 2014 20:42:16 Sebastian Hesselbarth wrote:
>>>> For the driver, Antoine then would have to squeeze all PHY register
>>>> mangling in phy-berlin2.c and see how to make ahci-platform aware of
>>>> individual port nodes (I haven't looked up if it already exists, sorry)
>>>> and announce only enabled port child nodes, right?
>>>
>>> I've been thinking some more about this aspect. I don't actually have
>>> a strong opinion on whether it's better to use the generic ahci-platform
>>> driver, or to keep the multi-phy support as a special variant for
>>> berlin. If we do the latter, it would however be good to define the
>>> binding in a way that lets us later merge things into the generic phy
>>> driver in case we get more of the same.
>>
>> Hmm, IMHO multi-phy support is orthogonal to ahci-platform, isn't it?
>> ahci-platform needs to know about the phy property and calls some
>> helper that deals with the phy-specifier?
>>
>> About a generic _phy_ driver, I am not so sure if berlin is the best
>> template right now 
>>
>> So, my call would be:
>> - make ahci-platform aware of port sub-nodes and phy properties
>> - have a berlin specific PHY driver
> 
> I'm not sure if we need sub-nodes per port, it should be enough
> to have an array of phys, plus a way to match them up with the
> ports.

Actually, I'd love to see sub-nodes per port as it will allow to
disabled unused ports on a per-board basis.

I have this in mind for a long time for Kirkwood's SATA node already:
Consider a board where you have the one available SATA plug connected
to port 1. How would that work out with status = "disabled"/"okay" that
doesn't allow array of strings obviously?

Sebastian

Re: [PATCH v3 1/6] phy: add a driver for the Berlin SATA PHY

[Arnd Bergmann]

On Wednesday 14 May 2014 20:56:29 Sebastian Hesselbarth wrote:
> On 05/14/2014 08:51 PM, Arnd Bergmann wrote:
> > On Wednesday 14 May 2014 20:42:16 Sebastian Hesselbarth wrote:
> >>>> For the driver, Antoine then would have to squeeze all PHY register
> >>>> mangling in phy-berlin2.c and see how to make ahci-platform aware of
> >>>> individual port nodes (I haven't looked up if it already exists, sorry)
> >>>> and announce only enabled port child nodes, right?
> >>>
> >>> I've been thinking some more about this aspect. I don't actually have
> >>> a strong opinion on whether it's better to use the generic ahci-platform
> >>> driver, or to keep the multi-phy support as a special variant for
> >>> berlin. If we do the latter, it would however be good to define the
> >>> binding in a way that lets us later merge things into the generic phy
> >>> driver in case we get more of the same.
> >>
> >> Hmm, IMHO multi-phy support is orthogonal to ahci-platform, isn't it?
> >> ahci-platform needs to know about the phy property and calls some
> >> helper that deals with the phy-specifier?
> >>
> >> About a generic _phy_ driver, I am not so sure if berlin is the best
> >> template right now 
> >>
> >> So, my call would be:
> >> - make ahci-platform aware of port sub-nodes and phy properties
> >> - have a berlin specific PHY driver
> > 
> > I'm not sure if we need sub-nodes per port, it should be enough
> > to have an array of phys, plus a way to match them up with the
> > ports.
> 
> Actually, I'd love to see sub-nodes per port as it will allow to
> disabled unused ports on a per-board basis.
> 
> I have this in mind for a long time for Kirkwood's SATA node already:
> Consider a board where you have the one available SATA plug connected
> to port 1. How would that work out with status = "disabled"/"okay" that
> doesn't allow array of strings obviously?

A simple bit mask would work fine, but I see your point. Doing
status="disabled" per port sounds nice.

	Arnd

Re: [PATCH v3 1/6] phy: add a driver for the Berlin SATA PHY

[Kishon Vijay Abraham I]

Hi,

On Thursday 15 May 2014 12:12 AM, Sebastian Hesselbarth wrote:
> On 05/14/2014 08:12 PM, Arnd Bergmann wrote:
>> On Wednesday 14 May 2014 19:57:46 Sebastian Hesselbarth wrote:
>>> On 05/14/2014 06:57 PM, Antoine Ténart wrote:
>>>> On Wed, May 14, 2014 at 06:11:24PM +0200, Arnd Bergmann wrote:
>>>>> On Wednesday 14 May 2014 17:49:29 Antoine Ténart wrote:
>>>>>> On Wed, May 14, 2014 at 05:31:24PM +0200, Arnd Bergmann wrote:
>>>
>>> From what I understand from the conversation, we have a single PHY
>>> register set dealing with both SATA ports available on the SoC.
>>> Also, from the name of the PHY bits we assume the PHY may be able
>>> to work in different modes than just SATA. And we currently have
>>> an AHCI-compatible SATA IP that supports up to two ports, with one
>>> actually connected to a SATA plug on the DMP board.
>>>
>>> Now, thinking about the PHY binding and the (possible) multi-protocol
>>> support, it can be possible that on BG2Q there is a generic 2-lane
>>> LVDS PHY that can be configured to support SATA or PCIe. Both are
>>> electrically and bit-level compatible, so they could be internally
>>> wired-up with AHCI and PCIe controller.
>>
>> Sounds like a reasonable guess. We have other PHY drivers doing the
>> same thing already.
> 
> Well, I based that on what I know about FPGA LVDS transceivers, so
> I wasn't guessing out of the blue ;)
> 
>>> From a DT point-of-view, we need a way to (a) link each SATA or PCIe
>>> port to the PHY, (b) specify the PHY lane to be used, and (c) specify
>>> the protocol to be used on that lane. If I got it right, Arnd already
>>> mentioned to use the phy-specifier to deal with it:
>>>
>>> e.g. phy = <&genphy 0 MODE_SATA> or phy = <&genphy 1 MODE_PCIE>
>>
>> Right.
>>
>>> Let's assume we have one dual-port SATA controller and one PCIe
>>> controller with either x1 or x2 support. The only sane DT binding,
>>> I can think of then would be:
>>>
>>> berlin2q.dtsi:
>>>
>>> genphy: lvds@ea00ff {
>>> 	compatible = "marvell,berlin-lvds-phy";
>>> 	reg = <0xea00ff 0x100>;
>>> 	#phy-cells = <2>;
>>> };
>>>
>>> sata: sata@ab00ff {
>>> 	compatible = "ahci-platform";
>>> 	reg = <0xab00ff 0x100>;
>>> 	
>>> 	sata0: sata-port@0 {
>>> 		reg = <0>;
>>> 		phy = <&genphy 0 MODE_SATA>;
>>> 		status = "disabled";
>>> 	};
>>>
>>> 	sata1: sata-port@1 {
>>> 		reg = <1>;
>>> 		phy = <&genphy 1 MODE_SATA>;
>>> 		status = "disabled";
>>> 	};
>>> };
>>>
>>> pcie: pcie@ab01ff {
>>> 	compatible = "marvell,berlin-pcie";
>>> 	reg = <0xab01ff 0x100>;
>>>
>>> 	pcie0: pcie-port@0 {
>>> 		reg = <0>;
>>> 		/* set phy on a per-board basis */
>>> 		/* PCIe x1 on Lane 0 : phy = <&genphy 0 MODE_PCIE>; */
>>> 		/* PCIe x2 on Lane 0 and 1 : phy = <&genphy 0 MODE_PCIE>, <&genphy 1
>>> MODE_PCIE>; */
>>> 		status = "disabled";
>>> 	};
>>> };
>>>
>>> berlin2q-dmp.dts:
>>>
>>> &sata1 {
>>> 	status = "okay";
>>> };
>>>
>>> &pcie0 {
>>> 	phy = <&genphy 1 MODE_PCIE>;
>>> };
>>>
>>> berlin2q-foo.dts:
>>>
>>> &pcie0 {
>>> 	phy = <&genphy 0 MODE_PCIE>, <&genphy 1 MODE_PCIE>;
>>> };
>>
>> Exactly. I would also be fine with keeping the sub-nodes of the
>> phy device as in v3 and using #phy-cells=<1> instead of #phy-cells.
>> The result would be pretty much the same, it just depends on how
>> closely connected the two logical phys are.

huh.. even with sub-nodes you'll need #phy-cells=<2> if we use a single *PHY
PROVIDER*. Because with just PHYs node pointer we won't be able to get the PHY.
We'll need PHY providers node pointer.

However I'd prefer to have sub-nodes for each individual PHYs and register a
single PHY PROVIDER.

Thanks
Kishon

Re: [PATCH v3 1/6] phy: add a driver for the Berlin SATA PHY

[Sebastian Hesselbarth]

On 05/15/2014 08:45 AM, Kishon Vijay Abraham I wrote:
> On Thursday 15 May 2014 12:12 AM, Sebastian Hesselbarth wrote:
>> On 05/14/2014 08:12 PM, Arnd Bergmann wrote:
>>> On Wednesday 14 May 2014 19:57:46 Sebastian Hesselbarth wrote:
>>>> On 05/14/2014 06:57 PM, Antoine Ténart wrote:
>>>>> On Wed, May 14, 2014 at 06:11:24PM +0200, Arnd Bergmann wrote:
>>>>>> On Wednesday 14 May 2014 17:49:29 Antoine Ténart wrote:
>>>>>>> On Wed, May 14, 2014 at 05:31:24PM +0200, Arnd Bergmann wrote:
[...]
>>>> Now, thinking about the PHY binding and the (possible) multi-protocol
>>>> support, it can be possible that on BG2Q there is a generic 2-lane
>>>> LVDS PHY that can be configured to support SATA or PCIe. Both are
>>>> electrically and bit-level compatible, so they could be internally
>>>> wired-up with AHCI and PCIe controller.
>>>
>>> Sounds like a reasonable guess. We have other PHY drivers doing the
>>> same thing already.
[...]
>>>> From a DT point-of-view, we need a way to (a) link each SATA or PCIe
>>>> port to the PHY, (b) specify the PHY lane to be used, and (c) specify
>>>> the protocol to be used on that lane. If I got it right, Arnd already
>>>> mentioned to use the phy-specifier to deal with it:
>>>>
>>>> e.g. phy = <&genphy 0 MODE_SATA> or phy = <&genphy 1 MODE_PCIE>
>>>
>>> Right.
>>>
>>>> Let's assume we have one dual-port SATA controller and one PCIe
>>>> controller with either x1 or x2 support. The only sane DT binding,
>>>> I can think of then would be:
>>>>
>>>> berlin2q.dtsi:
>>>>
>>>> genphy: lvds@ea00ff {
>>>> 	compatible = "marvell,berlin-lvds-phy";
>>>> 	reg = <0xea00ff 0x100>;
>>>> 	#phy-cells = <2>;
>>>> };
>>>>
>>>> sata: sata@ab00ff {
>>>> 	compatible = "ahci-platform";
>>>> 	reg = <0xab00ff 0x100>;
>>>> 	
>>>> 	sata0: sata-port@0 {
>>>> 		reg = <0>;
>>>> 		phy = <&genphy 0 MODE_SATA>;
>>>> 		status = "disabled";
>>>> 	};
>>>>
>>>> 	sata1: sata-port@1 {
>>>> 		reg = <1>;
>>>> 		phy = <&genphy 1 MODE_SATA>;
>>>> 		status = "disabled";
>>>> 	};
>>>> };
>>>>
>>>> pcie: pcie@ab01ff {
>>>> 	compatible = "marvell,berlin-pcie";
>>>> 	reg = <0xab01ff 0x100>;
>>>>
>>>> 	pcie0: pcie-port@0 {
>>>> 		reg = <0>;
>>>> 		/* set phy on a per-board basis */
>>>> 		/* PCIe x1 on Lane 0 : phy = <&genphy 0 MODE_PCIE>; */
>>>> 		/* PCIe x2 on Lane 0 and 1 : phy = <&genphy 0 MODE_PCIE>, <&genphy 1
>>>> MODE_PCIE>; */
>>>> 		status = "disabled";
>>>> 	};
>>>> };
>>>>
>>>> berlin2q-dmp.dts:
>>>>
>>>> &sata1 {
>>>> 	status = "okay";
>>>> };
>>>>
>>>> &pcie0 {
>>>> 	phy = <&genphy 1 MODE_PCIE>;
>>>> };
>>>>
>>>> berlin2q-foo.dts:
>>>>
>>>> &pcie0 {
>>>> 	phy = <&genphy 0 MODE_PCIE>, <&genphy 1 MODE_PCIE>;
>>>> };
>>>
>>> Exactly. I would also be fine with keeping the sub-nodes of the
>>> phy device as in v3 and using #phy-cells=<1> instead of #phy-cells.
>>> The result would be pretty much the same, it just depends on how
>>> closely connected the two logical phys are.
> 
> huh.. even with sub-nodes you'll need #phy-cells=<2> if we use a single *PHY
> PROVIDER*. Because with just PHYs node pointer we won't be able to get the PHY.
> We'll need PHY providers node pointer.
> 
> However I'd prefer to have sub-nodes for each individual PHYs and register a
> single PHY PROVIDER.

Depends on what you call PHY. In the example above the PHY is what
allows you to control both lanes.

So you want sub-nodes for each individual lane given the nomenclature
of the example?

Or like it is used in the example above, a single PHY node with an index
in the phy-specifier to pick an individual lane.

IMHO, having both phy-specifier index _and_ PHY sub-node per lane
has no benefit at all. You cannot even use the PHY sub-nodes for any
setup properties, as they depend on the consumer claiming the lane.

Sebastian

Re: [PATCH v3 1/6] phy: add a driver for the Berlin SATA PHY

[Kishon Vijay Abraham I]

Hi,

On Thursday 15 May 2014 12:32 PM, Sebastian Hesselbarth wrote:
> On 05/15/2014 08:45 AM, Kishon Vijay Abraham I wrote:
>> On Thursday 15 May 2014 12:12 AM, Sebastian Hesselbarth wrote:
>>> On 05/14/2014 08:12 PM, Arnd Bergmann wrote:
>>>> On Wednesday 14 May 2014 19:57:46 Sebastian Hesselbarth wrote:
>>>>> On 05/14/2014 06:57 PM, Antoine Ténart wrote:
>>>>>> On Wed, May 14, 2014 at 06:11:24PM +0200, Arnd Bergmann wrote:
>>>>>>> On Wednesday 14 May 2014 17:49:29 Antoine Ténart wrote:
>>>>>>>> On Wed, May 14, 2014 at 05:31:24PM +0200, Arnd Bergmann wrote:
> [...]
>>>>> Now, thinking about the PHY binding and the (possible) multi-protocol
>>>>> support, it can be possible that on BG2Q there is a generic 2-lane
>>>>> LVDS PHY that can be configured to support SATA or PCIe. Both are
>>>>> electrically and bit-level compatible, so they could be internally
>>>>> wired-up with AHCI and PCIe controller.
>>>>
>>>> Sounds like a reasonable guess. We have other PHY drivers doing the
>>>> same thing already.
> [...]
>>>>> From a DT point-of-view, we need a way to (a) link each SATA or PCIe
>>>>> port to the PHY, (b) specify the PHY lane to be used, and (c) specify
>>>>> the protocol to be used on that lane. If I got it right, Arnd already
>>>>> mentioned to use the phy-specifier to deal with it:
>>>>>
>>>>> e.g. phy = <&genphy 0 MODE_SATA> or phy = <&genphy 1 MODE_PCIE>
>>>>
>>>> Right.
>>>>
>>>>> Let's assume we have one dual-port SATA controller and one PCIe
>>>>> controller with either x1 or x2 support. The only sane DT binding,
>>>>> I can think of then would be:
>>>>>
>>>>> berlin2q.dtsi:
>>>>>
>>>>> genphy: lvds@ea00ff {
>>>>> 	compatible = "marvell,berlin-lvds-phy";
>>>>> 	reg = <0xea00ff 0x100>;
>>>>> 	#phy-cells = <2>;
>>>>> };
>>>>>
>>>>> sata: sata@ab00ff {
>>>>> 	compatible = "ahci-platform";
>>>>> 	reg = <0xab00ff 0x100>;
>>>>> 	
>>>>> 	sata0: sata-port@0 {
>>>>> 		reg = <0>;
>>>>> 		phy = <&genphy 0 MODE_SATA>;
>>>>> 		status = "disabled";
>>>>> 	};
>>>>>
>>>>> 	sata1: sata-port@1 {
>>>>> 		reg = <1>;
>>>>> 		phy = <&genphy 1 MODE_SATA>;
>>>>> 		status = "disabled";
>>>>> 	};
>>>>> };
>>>>>
>>>>> pcie: pcie@ab01ff {
>>>>> 	compatible = "marvell,berlin-pcie";
>>>>> 	reg = <0xab01ff 0x100>;
>>>>>
>>>>> 	pcie0: pcie-port@0 {
>>>>> 		reg = <0>;
>>>>> 		/* set phy on a per-board basis */
>>>>> 		/* PCIe x1 on Lane 0 : phy = <&genphy 0 MODE_PCIE>; */
>>>>> 		/* PCIe x2 on Lane 0 and 1 : phy = <&genphy 0 MODE_PCIE>, <&genphy 1
>>>>> MODE_PCIE>; */
>>>>> 		status = "disabled";
>>>>> 	};
>>>>> };
>>>>>
>>>>> berlin2q-dmp.dts:
>>>>>
>>>>> &sata1 {
>>>>> 	status = "okay";
>>>>> };
>>>>>
>>>>> &pcie0 {
>>>>> 	phy = <&genphy 1 MODE_PCIE>;
>>>>> };
>>>>>
>>>>> berlin2q-foo.dts:
>>>>>
>>>>> &pcie0 {
>>>>> 	phy = <&genphy 0 MODE_PCIE>, <&genphy 1 MODE_PCIE>;
>>>>> };
>>>>
>>>> Exactly. I would also be fine with keeping the sub-nodes of the
>>>> phy device as in v3 and using #phy-cells=<1> instead of #phy-cells.
>>>> The result would be pretty much the same, it just depends on how
>>>> closely connected the two logical phys are.
>>
>> huh.. even with sub-nodes you'll need #phy-cells=<2> if we use a single *PHY
>> PROVIDER*. Because with just PHYs node pointer we won't be able to get the PHY.
>> We'll need PHY providers node pointer.
>>
>> However I'd prefer to have sub-nodes for each individual PHYs and register a
>> single PHY PROVIDER.
> 
> Depends on what you call PHY. In the example above the PHY is what
> allows you to control both lanes.
> 
> So you want sub-nodes for each individual lane given the nomenclature
> of the example?
> 
> Or like it is used in the example above, a single PHY node with an index
> in the phy-specifier to pick an individual lane.
> 
> IMHO, having both phy-specifier index _and_ PHY sub-node per lane
> has no benefit at all. You cannot even use the PHY sub-nodes for any
> setup properties, as they depend on the consumer claiming the lane.

IMO the dt data should completely describe the HW. So just by looking at the
PHY node, we won't be able to tell the no of PHYs implemented in the IP if we
have a single PHY node (In this case the lanes in the IP).

However if you think it's an overkill for having sub-nodes for each lane then
single PHY node is fine too.

Thanks
Kishon

Re: [PATCH v3 1/6] phy: add a driver for the Berlin SATA PHY

[Sebastian Hesselbarth]

On 05/15/2014 10:46 AM, Kishon Vijay Abraham I wrote:
> On Thursday 15 May 2014 12:32 PM, Sebastian Hesselbarth wrote:
>> On 05/15/2014 08:45 AM, Kishon Vijay Abraham I wrote:
>>> On Thursday 15 May 2014 12:12 AM, Sebastian Hesselbarth wrote:
>>>> On 05/14/2014 08:12 PM, Arnd Bergmann wrote:
>>>>> On Wednesday 14 May 2014 19:57:46 Sebastian Hesselbarth wrote:
>>>>>> Let's assume we have one dual-port SATA controller and one PCIe
>>>>>> controller with either x1 or x2 support. The only sane DT binding,
>>>>>> I can think of then would be:
>>>>>>
>>>>>> berlin2q.dtsi:
>>>>>>
>>>>>> genphy: lvds@ea00ff {
>>>>>> 	compatible = "marvell,berlin-lvds-phy";
>>>>>> 	reg = <0xea00ff 0x100>;
>>>>>> 	#phy-cells = <2>;
>>>>>> };
[...]
>>
>> Depends on what you call PHY. In the example above the PHY is what
>> allows you to control both lanes.
>>
>> So you want sub-nodes for each individual lane given the nomenclature
>> of the example?
>>
>> Or like it is used in the example above, a single PHY node with an index
>> in the phy-specifier to pick an individual lane.
>>
>> IMHO, having both phy-specifier index _and_ PHY sub-node per lane
>> has no benefit at all. You cannot even use the PHY sub-nodes for any
>> setup properties, as they depend on the consumer claiming the lane.
>
> IMO the dt data should completely describe the HW. So just by looking at the
> PHY node, we won't be able to tell the no of PHYs implemented in the IP if we
> have a single PHY node (In this case the lanes in the IP).
>
> However if you think it's an overkill for having sub-nodes for each lane then
> single PHY node is fine too.

Yeah, I see your point. I just wonder how many Marvell PHYs we may hit
that require the _same_ magic setup inside but have _different_ number
of lanes. And even if, we can deal with it using a different compatible
string.

Currently, I feel a single PHY provider node and a set of compatibles
will be most likely, i.e. no per-lane sub-nodes. OTOH, the per-lane
sub-nodes is more generic as it allows us to deal PHYs that may
suddenly skip one lane in the numbering scheme. The difference for the
driver is marginal, i.e. some SoC-specific struct with a field for the
number of lanes vs. of_count_child_nodes() and a reg = <n> property for
the per-lane sub-nodes.

I used to agree to "DT should descibe HW", but with no datasheet
available, it quickly becomes fuzzy what it really looks like.

Anyway, I'll have some discussion with Antoine and Alexandre to sort out
the differences and the things in common for the PHY and SoCs in
question.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/6] phy: add a driver for the Berlin SATA PHY

[Kishon Vijay Abraham I]

On Thursday 15 May 2014 02:47 PM, Sebastian Hesselbarth wrote:
> On 05/15/2014 10:46 AM, Kishon Vijay Abraham I wrote:
>> On Thursday 15 May 2014 12:32 PM, Sebastian Hesselbarth wrote:
>>> On 05/15/2014 08:45 AM, Kishon Vijay Abraham I wrote:
>>>> On Thursday 15 May 2014 12:12 AM, Sebastian Hesselbarth wrote:
>>>>> On 05/14/2014 08:12 PM, Arnd Bergmann wrote:
>>>>>> On Wednesday 14 May 2014 19:57:46 Sebastian Hesselbarth wrote:
>>>>>>> Let's assume we have one dual-port SATA controller and one PCIe
>>>>>>> controller with either x1 or x2 support. The only sane DT binding,
>>>>>>> I can think of then would be:
>>>>>>>
>>>>>>> berlin2q.dtsi:
>>>>>>>
>>>>>>> genphy: lvds@ea00ff {
>>>>>>>     compatible = "marvell,berlin-lvds-phy";
>>>>>>>     reg = <0xea00ff 0x100>;
>>>>>>>     #phy-cells = <2>;
>>>>>>> };
> [...]
>>>
>>> Depends on what you call PHY. In the example above the PHY is what
>>> allows you to control both lanes.
>>>
>>> So you want sub-nodes for each individual lane given the nomenclature
>>> of the example?
>>>
>>> Or like it is used in the example above, a single PHY node with an index
>>> in the phy-specifier to pick an individual lane.
>>>
>>> IMHO, having both phy-specifier index _and_ PHY sub-node per lane
>>> has no benefit at all. You cannot even use the PHY sub-nodes for any
>>> setup properties, as they depend on the consumer claiming the lane.
>>
>> IMO the dt data should completely describe the HW. So just by looking at the
>> PHY node, we won't be able to tell the no of PHYs implemented in the IP if we
>> have a single PHY node (In this case the lanes in the IP).
>>
>> However if you think it's an overkill for having sub-nodes for each lane then
>> single PHY node is fine too.
> 
> Yeah, I see your point. I just wonder how many Marvell PHYs we may hit
> that require the _same_ magic setup inside but have _different_ number
> of lanes. And even if, we can deal with it using a different compatible
> string.
> 
> Currently, I feel a single PHY provider node and a set of compatibles
> will be most likely, i.e. no per-lane sub-nodes. OTOH, the per-lane
> sub-nodes is more generic as it allows us to deal PHYs that may
> suddenly skip one lane in the numbering scheme. The difference for the
> driver is marginal, i.e. some SoC-specific struct with a field for the
> number of lanes vs. of_count_child_nodes() and a reg = <n> property for
> the per-lane sub-nodes.
> 
> I used to agree to "DT should descibe HW", but with no datasheet
> available, it quickly becomes fuzzy what it really looks like.
> 
> Anyway, I'll have some discussion with Antoine and Alexandre to sort out
> the differences and the things in common for the PHY and SoCs in
> question.

cool, thanks.

-Kishon

[PATCH v3 2/6] Documentation: bindings: add the Berlin SATA PHY

[Antoine Ténart]

The Berlin SATA PHY drives the PHY related to the SATA interface and
allows to power up/down each PHY independently. Add the corresponding
documentation.

Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com>
---
 .../devicetree/bindings/phy/berlin-sata-phy.txt    | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/berlin-sata-phy.txt

diff --git a/Documentation/devicetree/bindings/phy/berlin-sata-phy.txt b/Documentation/devicetree/bindings/phy/berlin-sata-phy.txt
new file mode 100644
index 000000000000..b1e11a25775a
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/berlin-sata-phy.txt
@@ -0,0 +1,34 @@
+Berlin SATA PHY
+---------------
+
+Required properties:
+- compatible: "marvell,berlin-sata-phy"
+- #address-cells: number of cells to encode an address, must be 1
+- #size-cells: number of cells representing the size of an address, must be 0
+- reg: address and length of the register
+
+The SATA PHY node is a provider and should contain sub-nodes representing the
+PHYs it handles, one per PHY.
+
+Required sub-node properties:
+- reg: the PHY described. 0 or 1.
+- #phy-cells: from the generic PHY bindings, must be 0
+
+Example:
+	sata_phy: phy@f7e900a0 {
+		compatible = "marvell,berlin-sata-phy";
+		reg = <0xf7e900a0 0x10>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		status = "okay";
+
+		sata_phy0: phy@0 {
+			reg = <0>;
+			#phy-cells = <0>;
+		};
+
+		sata_phy1: phy@1 {
+			reg = <1>;
+			#phy-cells = <0>;
+		};
+	};
-- 
1.9.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 3/6] ata: ahci: add AHCI support for the Berlin BG2Q

[Antoine Ténart]

Add support for the Berlin BG2Q AHCI SATA controller allowing to
interface with devices like external hard drives.

Signed-off-by: Antoine Ténart <antoine.tenart-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/ata/Kconfig       |  10 +++
 drivers/ata/Makefile      |   1 +
 drivers/ata/ahci_berlin.c | 202 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 213 insertions(+)
 create mode 100644 drivers/ata/ahci_berlin.c

diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index c2706047337f..37e6817b31f4 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -97,6 +97,16 @@ config SATA_AHCI_PLATFORM
 
 	  If unsure, say N.
 
+config AHCI_BERLIN
+	tristate "Marvell Berlin AHCI SATA support"
+	depends on ARCH_BERLIN
+	select PHY_BERLIN_SATA
+	help
+	  This option enables support for the Marvell Berlin SoC's
+	  onboard AHCI SATA.
+
+	  If unsure, say N.
+
 config AHCI_DA850
 	tristate "DaVinci DA850 AHCI SATA support"
 	depends on ARCH_DAVINCI_DA850
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index 44c8016e565c..7fb78d1e0a44 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_SATA_INIC162X)	+= sata_inic162x.o
 obj-$(CONFIG_SATA_SIL24)	+= sata_sil24.o
 obj-$(CONFIG_SATA_DWC)		+= sata_dwc_460ex.o
 obj-$(CONFIG_SATA_HIGHBANK)	+= sata_highbank.o libahci.o
+obj-$(CONFIG_AHCI_BERLIN)	+= ahci_berlin.o libahci.o libahci_platform.o
 obj-$(CONFIG_AHCI_DA850)	+= ahci_da850.o libahci.o libahci_platform.o
 obj-$(CONFIG_AHCI_IMX)		+= ahci_imx.o libahci.o libahci_platform.o
 obj-$(CONFIG_AHCI_SUNXI)	+= ahci_sunxi.o libahci.o libahci_platform.o
diff --git a/drivers/ata/ahci_berlin.c b/drivers/ata/ahci_berlin.c
new file mode 100644
index 000000000000..973c07e54a6a
--- /dev/null
+++ b/drivers/ata/ahci_berlin.c
@@ -0,0 +1,202 @@
+/*
+ * Marvell Berlin AHCI SATA platform driver
+ *
+ * Copyright (C) 2014 Marvell Technology Group Ltd.
+ *
+ * Antoine Ténart <antoine.tenart-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
+ * Jisheng Zhang <jszhang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/ahci_platform.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+
+#include "ahci.h"
+
+#define BERLIN_SATA_NPORTS	2
+
+#define PORT_VSR_ADDR		0x78
+#define PORT_VSR_DATA		0x7c
+
+#define PHY_BASE		0x200
+
+/* register 0x01 */
+#define REF_FREF_SEL_25		BIT(0)
+#define PHY_MODE_SATA		(0x0 << 5)
+
+/* register 0x02 */
+#define USE_MAX_PLL_RATE	BIT(12)
+
+/* register 0x23 */
+#define DATA_BIT_WIDTH_10	(0x0 << 10)
+#define DATA_BIT_WIDTH_20	(0x1 << 10)
+#define DATA_BIT_WIDTH_40	(0x2 << 10)
+
+/* register 0x25 */
+#define PHY_GEN_MAX_1_5		(0x0 << 10)
+#define PHY_GEN_MAX_3_0		(0x1 << 10)
+#define PHY_GEN_MAX_6_0		(0x2 << 10)
+
+struct berlin_ahci_priv {
+	struct ahci_host_priv	*hpriv;
+	struct phy		*phys[BERLIN_SATA_NPORTS];
+};
+
+static inline void ahci_berlin_reg_setbits(void __iomem *ctrl_reg, u32 reg,
+					   u32 mask, u32 val)
+{
+	u32 regval;
+
+	/* select register */
+	writel(PHY_BASE + reg, ctrl_reg + PORT_VSR_ADDR);
+
+	/* set bits */
+	regval = readl(ctrl_reg + PORT_VSR_DATA);
+	regval &= ~mask;
+	regval |= val;
+	writel(regval, ctrl_reg + PORT_VSR_DATA);
+}
+
+static void ahci_berlin_port_init(struct ahci_host_priv *hpriv,
+				  unsigned int port)
+{
+	struct berlin_ahci_priv *berlin_priv = hpriv->plat_data;
+	void __iomem *ctrl_reg = hpriv->mmio + 0x100 + (port * 0x80);
+
+	/* power on the PHY */
+	phy_power_on(berlin_priv->phys[port]);
+
+	/* set PHY mode to SATA, ref freq to 25 MHz */
+	ahci_berlin_reg_setbits(ctrl_reg, 0x1, 0xff, REF_FREF_SEL_25 | PHY_MODE_SATA);
+
+	/* set PHY up to 6 Gbps */
+	ahci_berlin_reg_setbits(ctrl_reg, 0x25, 0xc00, PHY_GEN_MAX_6_0);
+
+	/* set 40 bits width */
+	ahci_berlin_reg_setbits(ctrl_reg, 0x23,  0xc00, DATA_BIT_WIDTH_40);
+
+	/* use max pll rate */
+	ahci_berlin_reg_setbits(ctrl_reg, 0x2, 0x0, USE_MAX_PLL_RATE);
+
+	/* set the controller speed */
+	writel(0x31, ctrl_reg + PORT_SCR_CTL);
+}
+
+static const struct ata_port_info ahci_berlin_port_info = {
+	.flags		= AHCI_FLAG_COMMON | ATA_FLAG_NCQ,
+	.pio_mask	= ATA_PIO4,
+	.udma_mask	= ATA_UDMA6,
+	.port_ops	= &ahci_platform_ops,
+};
+
+static int ahci_berlin_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct ahci_host_priv *hpriv;
+	struct berlin_ahci_priv *berlin_priv;
+	int ret, i = 0;
+	unsigned mask = 0;
+
+	hpriv = ahci_platform_get_resources(pdev);
+	if (IS_ERR(hpriv)) {
+		dev_err(dev, "cannot get AHCI resources\n");
+		return PTR_ERR(hpriv);
+	}
+
+	berlin_priv = devm_kzalloc(dev, sizeof(*berlin_priv), GFP_KERNEL);
+	if (!berlin_priv)
+		return -ENOMEM;
+
+	hpriv->plat_data = berlin_priv;
+
+	ret = ahci_platform_enable_resources(hpriv);
+	if (ret) {
+		dev_err(dev, "cannot enable resources: %d\n", ret);
+		return ret;
+	}
+
+	for (i = 0; i < BERLIN_SATA_NPORTS; i++) {
+		char phy_name[6];
+
+		sprintf(phy_name, "port%d", i);
+
+		berlin_priv->phys[i] = devm_phy_get(dev, phy_name);
+		if (IS_ERR(berlin_priv->phys[i])) {
+			dev_warn(dev, "%s not available\n", phy_name);
+			continue;
+		}
+
+		mask |= 1 << i;
+		ahci_berlin_port_init(hpriv, i);
+	}
+
+	ret = ahci_platform_init_host(pdev, hpriv, &ahci_berlin_port_info,
+				      0, mask);
+	if (ret) {
+		dev_err(dev, "host init failed: %d\n", ret);
+		goto disable_resources;
+	}
+
+	return 0;
+
+disable_resources:
+	ahci_platform_disable_resources(hpriv);
+	return ret;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int ahci_berlin_resume(struct device *dev)
+{
+	struct ata_host *host = dev_get_drvdata(dev);
+	struct ahci_host_priv *hpriv = host->private_data;
+	struct berlin_ahci_priv *berlin_priv = hpriv->plat_data;
+	int ret, i;
+
+	ret = ahci_platform_enable_resources(hpriv);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < BERLIN_SATA_NPORTS; i++) {
+		if (!IS_ERR(berlin_priv->phys[i]))
+			ahci_berlin_port_init(hpriv, i);
+	}
+
+	ret = ahci_platform_resume_host(dev);
+	if (ret) {
+		ahci_platform_disable_resources(hpriv);
+		return ret;
+	}
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(ahci_berlin_pm_ops, ahci_platform_suspend,
+			 ahci_berlin_resume);
+
+static const struct of_device_id ahci_berlin_of_match[] = {
+	{ .compatible = "marvell,berlin-ahci" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ahci_berlin_of_match);
+
+static struct platform_driver ahci_berlin_driver = {
+	.probe	= ahci_berlin_probe,
+	.remove	= ata_platform_remove_one,
+	.driver	= {
+		.name		= "ahci-berlin",
+		.owner		= THIS_MODULE,
+		.of_match_table = ahci_berlin_of_match,
+		.pm		= &ahci_berlin_pm_ops,
+	},
+};
+module_platform_driver(ahci_berlin_driver);
+
+MODULE_DESCRIPTION("Marvell Berlin AHCI SATA driver");
+MODULE_AUTHOR("Antoine Ténart <antoine.tenart-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>");
+MODULE_LICENSE("GPL");
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 4/6] Documentation: bindings: add the berlin-ahci compatible to the ahci platform

[Antoine Ténart]

The berlin-ahci driver allows Berlin SoCs to support their AHCI SATA controller.
Add the compatible to the device tree bindings documentation.

Signed-off-by: Antoine Ténart <antoine.tenart-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 Documentation/devicetree/bindings/ata/ahci-berlin.txt | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ata/ahci-berlin.txt

diff --git a/Documentation/devicetree/bindings/ata/ahci-berlin.txt b/Documentation/devicetree/bindings/ata/ahci-berlin.txt
new file mode 100644
index 000000000000..10362fdc5b8e
--- /dev/null
+++ b/Documentation/devicetree/bindings/ata/ahci-berlin.txt
@@ -0,0 +1,18 @@
+* Marvell Berlin SATA nodes
+
+Required properties:
+- compatible: "marvell,berlin-ahci"
+- interrupts: interrupt mapping for the SATA IRQ
+- reg: address and length of the register
+- phys: references to the SATA PHY nodes
+- phy-names: should be "port0" or/and "port1"
+
+Example:
+	ahci: sata@f7e90000 {
+		compatible = "marvell,berlin-ahci";
+		reg = <0xf7e90000 0x1000>;
+		interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
+		phys = <&sata_phy0>, <&sata_phy1>;
+		phy-names = "port0", "port1";
+		status = "okay";
+	};
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 5/6] ARM: berlin: add the AHCI node for the BG2Q

[Antoine Ténart]

The BG2Q has an AHCI SATA controller. Add the corresponding nodes
(AHCI, SATA PHYs) in its device tree.

Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com>
---
 arch/arm/boot/dts/berlin2q.dtsi | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arch/arm/boot/dts/berlin2q.dtsi b/arch/arm/boot/dts/berlin2q.dtsi
index 07452a7483fa..53f8d9763589 100644
--- a/arch/arm/boot/dts/berlin2q.dtsi
+++ b/arch/arm/boot/dts/berlin2q.dtsi
@@ -183,6 +183,31 @@
 			};
 		};
 
+		ahci: sata@e90000 {
+			compatible = "marvell,berlin-ahci";
+			reg = <0xe90000 0x1000>;
+			interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
+			status = "disabled";
+		};
+
+		sata_phy: phy@e900a0 {
+			compatible = "marvell,berlin-sata-phy";
+			reg = <0xe900a0 0x10>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled";
+
+			sata_phy0: phy@0 {
+				#phy-cells = <0>;
+				reg = <0>;
+			};
+
+			sata_phy1: phy@1 {
+				#phy-cells = <0>;
+				reg = <1>;
+			};
+		};
+
 		apb@fc0000 {
 			compatible = "simple-bus";
 			#address-cells = <1>;
-- 
1.9.1

[PATCH v3 6/6] ARM: berlin: enable the eSATA interface on the BG2Q DMP

[Antoine Ténart]

The BG2Q has an AHCI SATA controller with an eSATA interface. Enable it.
Also use the SATA PHY 0, the BG2Q DMP does not has the SATA PHY 1.

Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com>
---
 arch/arm/boot/dts/berlin2q-marvell-dmp.dts | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/berlin2q-marvell-dmp.dts b/arch/arm/boot/dts/berlin2q-marvell-dmp.dts
index 2da9c41e29d8..e5c9012babc6 100644
--- a/arch/arm/boot/dts/berlin2q-marvell-dmp.dts
+++ b/arch/arm/boot/dts/berlin2q-marvell-dmp.dts
@@ -26,3 +26,13 @@
 &uart0 {
 	status = "okay";
 };
+
+&ahci {
+	phys = <&sata_phy0>;
+	phy-names = "port0";
+	status = "okay";
+};
+
+&sata_phy {
+	status = "okay";
+};
-- 
1.9.1