[PATCH v2 0/2] phy: add hix5hd2-sata-phy driver

[PATCH v2 0/2] phy: add hix5hd2-sata-phy driver

[Zhangfei Gao]

v2:
Suggested by Mark
Remove hisilicon,reg-init and init the register in phy-hix5hd2-sata.c
Change property name accordingly.

Jiancheng Xue (2):
  Documentation: Document Hisilicon hix5hd2 sata PHY
  phy: add hix5hd2-sata-phy driver

 .../devicetree/bindings/phy/hix5hd2-sata-phy.txt   |   22 +++
 drivers/phy/Kconfig                                |    8 +
 drivers/phy/Makefile                               |    1 +
 drivers/phy/phy-hix5hd2-sata.c                     |  190 ++++++++++++++++++++
 4 files changed, 221 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/hix5hd2-sata-phy.txt
 create mode 100644 drivers/phy/phy-hix5hd2-sata.c

-- 
1.7.9.5

[PATCH v2 1/2] Documentation: Document Hisilicon hix5hd2 sata PHY

[Zhangfei Gao]

From: Jiancheng Xue <xuejiancheng@huawei.com>

Add necessary binding documentation SATA PHY on Hisilicon hix5hd2 soc.

Signed-off-by: Jiancheng Xue <xuejiancheng@huawei.com>
Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
---
 .../devicetree/bindings/phy/hix5hd2-sata-phy.txt   |   22 ++++++++++++++++++++
 1 file changed, 22 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/hix5hd2-sata-phy.txt

diff --git a/Documentation/devicetree/bindings/phy/hix5hd2-sata-phy.txt b/Documentation/devicetree/bindings/phy/hix5hd2-sata-phy.txt
new file mode 100644
index 0000000..296168b
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/hix5hd2-sata-phy.txt
@@ -0,0 +1,22 @@
+Hisilicon hix5hd2 SATA PHY
+-----------------------
+
+Required properties:
+- compatible: should be "hisilicon,hix5hd2-sata-phy"
+- reg: offset and length of the PHY registers
+- #phy-cells: must be 0
+Refer to phy/phy-bindings.txt for the generic PHY binding properties
+
+Optional Properties:
+- hisilicon,peripheral-syscon: phandle of syscon used to control peripheral.
+- hisilicon,power-reg: offset and bit number within peripheral-syscon,
+	register of controlling sata power supply.
+
+Example:
+	sata_phy: phy@f9900000 {
+		compatible = "hisilicon,hix5hd2-sata-phy";
+		reg = <0xf9900000 0x10000>;
+		#phy-cells = <0>;
+		hisilicon,peripheral-syscon = <&peripheral_ctrl>;
+		hisilicon,power-reg = <0x8 10>;
+	};
-- 
1.7.9.5

[PATCH v2 2/2] phy: add hix5hd2-sata-phy driver

[Zhangfei Gao]

From: Jiancheng Xue <xuejiancheng@huawei.com>

Add hix5hd2-sata-phy driver on Hisilicon hix5hd2 soc.

Signed-off-by: Jiancheng Xue <xuejiancheng@huawei.com>
Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
---
 drivers/phy/Kconfig            |    8 ++
 drivers/phy/Makefile           |    1 +
 drivers/phy/phy-hix5hd2-sata.c |  192 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 201 insertions(+)
 create mode 100644 drivers/phy/phy-hix5hd2-sata.c

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index 16a2f06..782953d 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -109,6 +109,14 @@ config PHY_EXYNOS5250_SATA
 	  SATA 3.0 Gb/s, SATA 6.0 Gb/s speeds. It supports one SATA host
 	  port to accept one SATA device.
 
+config PHY_HIX5HD2_SATA
+	tristate "HIX5HD2 SATA PHY Driver"
+	depends on ARCH_HIX5HD2 && OF
+	select GENERIC_PHY
+	select MFD_SYSCON
+	help
+	  Support for SATA PHY on Hisilicon hix5hd2 Soc.
+
 config PHY_SUN4I_USB
 	tristate "Allwinner sunxi SoC USB PHY driver"
 	depends on ARCH_SUNXI && HAS_IOMEM && OF
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index b4f1d57..54f04d0 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_OMAP_USB2)			+= phy-omap-usb2.o
 obj-$(CONFIG_TI_PIPE3)			+= phy-ti-pipe3.o
 obj-$(CONFIG_TWL4030_USB)		+= phy-twl4030-usb.o
 obj-$(CONFIG_PHY_EXYNOS5250_SATA)	+= phy-exynos5250-sata.o
+obj-$(CONFIG_PHY_HIX5HD2_SATA)		+= phy-hix5hd2-sata.o
 obj-$(CONFIG_PHY_SUN4I_USB)		+= phy-sun4i-usb.o
 obj-$(CONFIG_PHY_SAMSUNG_USB2)		+= phy-exynos-usb2.o
 phy-exynos-usb2-y			+= phy-samsung-usb2.o
diff --git a/drivers/phy/phy-hix5hd2-sata.c b/drivers/phy/phy-hix5hd2-sata.c
new file mode 100644
index 0000000..6f1e3ea
--- /dev/null
+++ b/drivers/phy/phy-hix5hd2-sata.c
@@ -0,0 +1,192 @@
+/*
+ * Copyright (c) 2014 Linaro Ltd.
+ * Copyright (c) 2014 Hisilicon Limited.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+#include <linux/phy/phy.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include <linux/delay.h>
+
+#define SATA_PHY0_CTLL		0xa0
+#define MPLL_MULTIPLIER_SHIFT	1
+#define MPLL_MULTIPLIER_MASK	0xfe
+#define MPLL_MULTIPLIER_50M	0x3c
+#define MPLL_MULTIPLIER_100M	0x1e
+#define PHY_RESET		BIT(0)
+#define REF_SSP_EN		BIT(9)
+#define SSC_EN			BIT(10)
+#define REF_USE_PAD		BIT(23)
+
+#define SATA_PORT_PHYCTL	0x174
+#define SPEED_MODE_MASK		0x6f0000
+#define HALF_RATE_SHIFT		16
+#define PHY_CONFIG_SHIFT	18
+#define GEN2_EN_SHIFT		21
+#define SPEED_CTRL		BIT(20)
+
+#define SATA_PORT_PHYCTL1	0x148
+#define AMPLITUDE_MASK		0x3ffffe
+#define AMPLITUDE_GEN3		0x68
+#define AMPLITUDE_GEN3_SHIFT	15
+#define AMPLITUDE_GEN2		0x56
+#define AMPLITUDE_GEN2_SHIFT	8
+#define AMPLITUDE_GEN1		0x56
+#define AMPLITUDE_GEN1_SHIFT	1
+
+#define SATA_PORT_PHYCTL2	0x14c
+#define PREEMPH_MASK		0x3ffff
+#define PREEMPH_GEN3		0x20
+#define PREEMPH_GEN3_SHIFT	12
+#define PREEMPH_GEN2		0x15
+#define PREEMPH_GEN2_SHIFT	6
+#define PREEMPH_GEN1		0x5
+#define PREEMPH_GEN1_SHIFT	0
+
+struct hix5hd2_priv {
+	void __iomem	*base;
+	struct regmap	*peri_ctrl;
+};
+
+enum phy_speed_mode {
+	SPEED_MODE_GEN1 = 0,
+	SPEED_MODE_GEN2 = 1,
+	SPEED_MODE_GEN3 = 2,
+};
+
+static int hix5hd2_sata_phy_init(struct phy *phy)
+{
+	struct hix5hd2_priv *priv = phy_get_drvdata(phy);
+	u32 val, data[2];
+	int ret;
+
+	if (priv->peri_ctrl) {
+		ret = of_property_read_u32_array(phy->dev.of_node,
+						 "hisilicon,power-reg",
+						 &data[0], 2);
+		if (ret) {
+			dev_err(&phy->dev, "Fail read hisilicon,power-reg\n");
+			return ret;
+		}
+
+		regmap_update_bits(priv->peri_ctrl, data[0],
+				   BIT(data[1]), BIT(data[1]));
+	}
+
+	/* reset phy */
+	val = readl_relaxed(priv->base + SATA_PHY0_CTLL);
+	val &= ~(MPLL_MULTIPLIER_MASK | REF_USE_PAD);
+	val |= MPLL_MULTIPLIER_50M << MPLL_MULTIPLIER_SHIFT |
+	       REF_SSP_EN | PHY_RESET;
+	writel_relaxed(val, priv->base + SATA_PHY0_CTLL);
+	msleep(20);
+	val &= ~PHY_RESET;
+	writel_relaxed(val, priv->base + SATA_PHY0_CTLL);
+
+	val = readl_relaxed(priv->base + SATA_PORT_PHYCTL1);
+	val &= ~AMPLITUDE_MASK;
+	val |= AMPLITUDE_GEN3 << AMPLITUDE_GEN3_SHIFT |
+	       AMPLITUDE_GEN2 << AMPLITUDE_GEN2_SHIFT |
+	       AMPLITUDE_GEN1 << AMPLITUDE_GEN1_SHIFT;
+	writel_relaxed(val, priv->base + SATA_PORT_PHYCTL1);
+
+	val = readl_relaxed(priv->base + SATA_PORT_PHYCTL2);
+	val &= ~PREEMPH_MASK;
+	val |= PREEMPH_GEN3 << PREEMPH_GEN3_SHIFT |
+	       PREEMPH_GEN2 << PREEMPH_GEN2_SHIFT |
+	       PREEMPH_GEN1 << PREEMPH_GEN1_SHIFT;
+	writel_relaxed(val, priv->base + SATA_PORT_PHYCTL2);
+
+	/* ensure PHYCTRL setting takes effect */
+	val = readl_relaxed(priv->base + SATA_PORT_PHYCTL);
+	val &= ~SPEED_MODE_MASK;
+	val |= SPEED_MODE_GEN1 << HALF_RATE_SHIFT |
+	      SPEED_MODE_GEN1 << PHY_CONFIG_SHIFT |
+	      SPEED_MODE_GEN1 << GEN2_EN_SHIFT | SPEED_CTRL;
+	writel_relaxed(val, priv->base + SATA_PORT_PHYCTL);
+
+	msleep(20);
+	val &= ~SPEED_MODE_MASK;
+	val |= SPEED_MODE_GEN3 << HALF_RATE_SHIFT |
+	      SPEED_MODE_GEN3 << PHY_CONFIG_SHIFT |
+	      SPEED_MODE_GEN3 << GEN2_EN_SHIFT | SPEED_CTRL;
+	writel_relaxed(val, priv->base + SATA_PORT_PHYCTL);
+
+	val &= ~(SPEED_MODE_MASK | SPEED_CTRL);
+	val |= SPEED_MODE_GEN2 << HALF_RATE_SHIFT |
+	      SPEED_MODE_GEN2 << PHY_CONFIG_SHIFT |
+	      SPEED_MODE_GEN2 << GEN2_EN_SHIFT;
+	writel_relaxed(val, priv->base + SATA_PORT_PHYCTL);
+
+	return 0;
+}
+
+static struct phy_ops hix5hd2_sata_phy_ops = {
+	.init		= hix5hd2_sata_phy_init,
+	.owner		= THIS_MODULE,
+};
+
+static int hix5hd2_sata_phy_probe(struct platform_device *pdev)
+{
+	struct phy_provider *phy_provider;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	struct phy *phy;
+	struct hix5hd2_priv *priv;
+
+	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 (!priv->base)
+		return -ENOMEM;
+
+	priv->peri_ctrl = syscon_regmap_lookup_by_phandle(dev->of_node,
+						"hisilicon,peripheral-syscon");
+	if (IS_ERR(priv->peri_ctrl))
+		priv->peri_ctrl = NULL;
+
+	phy = devm_phy_create(dev, &hix5hd2_sata_phy_ops, NULL);
+	if (IS_ERR(phy)) {
+		dev_err(dev, "failed to create PHY\n");
+		return PTR_ERR(phy);
+	}
+
+	phy_set_drvdata(phy, priv);
+	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+	if (IS_ERR(phy_provider))
+		return PTR_ERR(phy_provider);
+
+	return 0;
+}
+
+static const struct of_device_id hix5hd2_sata_phy_of_match[] = {
+	{.compatible = "hisilicon,hix5hd2-sata-phy",},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, hix5hd2_sata_phy_of_match);
+
+static struct platform_driver hix5hd2_sata_phy_driver = {
+	.probe	= hix5hd2_sata_phy_probe,
+	.driver = {
+		.name	= "hix5hd2-sata-phy",
+		.owner	= THIS_MODULE,
+		.of_match_table	= hix5hd2_sata_phy_of_match,
+	}
+};
+module_platform_driver(hix5hd2_sata_phy_driver);
+
+MODULE_AUTHOR("Jiancheng Xue <xuejiancheng@huawei.com>");
+MODULE_DESCRIPTION("HISILICON HIX5HD2 SATA PHY driver");
+MODULE_ALIAS("platform:hix5hd2-sata-phy");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [PATCH v2 1/2] Documentation: Document Hisilicon hix5hd2 sata PHY

[Arnd Bergmann]

On Wednesday 25 June 2014, Zhangfei Gao wrote:
> From: Jiancheng Xue <xuejiancheng@huawei.com>
> 
> Add necessary binding documentation SATA PHY on Hisilicon hix5hd2 soc.
> 
> Signed-off-by: Jiancheng Xue <xuejiancheng@huawei.com>
> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>

We have had a couple of PHY drivers that were initially meant to be
single-purpose but then turned out to be for hardware that could
be put into multiple modes (SATA, gbit ethernet, PCIe, ...). Are
you sure that this particular device is SATA-only?

	Arnd

Re: [PATCH v2 1/2] Documentation: Document Hisilicon hix5hd2 sata PHY

[zhangfei]

On 06/25/2014 08:16 PM, Arnd Bergmann wrote:
> On Wednesday 25 June 2014, Zhangfei Gao wrote:
>> From: Jiancheng Xue <xuejiancheng@huawei.com>
>>
>> Add necessary binding documentation SATA PHY on Hisilicon hix5hd2 soc.
>>
>> Signed-off-by: Jiancheng Xue <xuejiancheng@huawei.com>
>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>
> We have had a couple of PHY drivers that were initially meant to be
> single-purpose but then turned out to be for hardware that could
> be put into multiple modes (SATA, gbit ethernet, PCIe, ...). Are
> you sure that this particular device is SATA-only?
>

Yes, double confimed.
Hix5hd2 phy diver only support sata phy, in fact, current soc only 
support synposis phy, since some internal physical link.
And hix5hd2 ethernet phy is already in hix5hd2_gmac.c.
No pcie support on hix5hd2 platfrom.

Thanks

Re: [PATCH v2 1/2] Documentation: Document Hisilicon hix5hd2 sata PHY

[Arnd Bergmann]

On Wednesday 25 June 2014 20:41:26 zhangfei wrote:
> On 06/25/2014 08:16 PM, Arnd Bergmann wrote:
> > On Wednesday 25 June 2014, Zhangfei Gao wrote:
> >> From: Jiancheng Xue <xuejiancheng@huawei.com>
> >>
> >> Add necessary binding documentation SATA PHY on Hisilicon hix5hd2 soc.
> >>
> >> Signed-off-by: Jiancheng Xue <xuejiancheng@huawei.com>
> >> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> >
> > We have had a couple of PHY drivers that were initially meant to be
> > single-purpose but then turned out to be for hardware that could
> > be put into multiple modes (SATA, gbit ethernet, PCIe, ...). Are
> > you sure that this particular device is SATA-only?
> >
> 
> Yes, double confimed.
> Hix5hd2 phy diver only support sata phy, in fact, current soc only 
> support synposis phy, since some internal physical link.
> And hix5hd2 ethernet phy is already in hix5hd2_gmac.c.
> No pcie support on hix5hd2 platfrom.

Sorry if I my question was unclear, I did not mean the implementation
of this phy in hix5hd2, but the Synopsys phy itself. I was mainly
asking because of the "compatible" string, which should otherwise
not include "sata" but be more generic.

Since you mention that this is a synopsys part, it may however be
good to also define a compatible string for the generic device,
to allow other SoCs to share this driver, and to make it easier
for people to find out that we already have one so they don't have
to write another driver for the same hardware.

Can you find out what the Synopsys/Designware product designation for
this PHY is? It would be good to rename the driver and binding
if possible. I would still recommend leaving the
"hisilicon,hix5hd2-sata-phy" string and the "hisilicon,peripheral-syscon"
property in place, since that indicates a minor variation of the
generic part.

	Arnd

Re: [PATCH v2 1/2] Documentation: Document Hisilicon hix5hd2 sata PHY

[zhangfei]

On 06/25/2014 09:32 PM, Arnd Bergmann wrote:
> On Wednesday 25 June 2014 20:41:26 zhangfei wrote:
>> On 06/25/2014 08:16 PM, Arnd Bergmann wrote:
>>> On Wednesday 25 June 2014, Zhangfei Gao wrote:
>>>> From: Jiancheng Xue <xuejiancheng@huawei.com>
>>>>
>>>> Add necessary binding documentation SATA PHY on Hisilicon hix5hd2 soc.
>>>>
>>>> Signed-off-by: Jiancheng Xue <xuejiancheng@huawei.com>
>>>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>>>
>>> We have had a couple of PHY drivers that were initially meant to be
>>> single-purpose but then turned out to be for hardware that could
>>> be put into multiple modes (SATA, gbit ethernet, PCIe, ...). Are
>>> you sure that this particular device is SATA-only?
>>>
>>
>> Yes, double confimed.
>> Hix5hd2 phy diver only support sata phy, in fact, current soc only
>> support synposis phy, since some internal physical link.
>> And hix5hd2 ethernet phy is already in hix5hd2_gmac.c.
>> No pcie support on hix5hd2 platfrom.
>
> Sorry if I my question was unclear, I did not mean the implementation
> of this phy in hix5hd2, but the Synopsys phy itself. I was mainly
> asking because of the "compatible" string, which should otherwise
> not include "sata" but be more generic.

This phy is specifically for sata.

>
> Since you mention that this is a synopsys part, it may however be
> good to also define a compatible string for the generic device,
> to allow other SoCs to share this driver, and to make it easier
> for people to find out that we already have one so they don't have
> to write another driver for the same hardware.

http://www.synopsys.com/dw/ipdir.php?ds=dwc_sata_phy
name can be: dwc_sata_phy
However, not find register from the website, not sure whether the 
register are totally same, or there is some change.
Other soc from hisilicon using the same ip can share this driver.

>
> Can you find out what the Synopsys/Designware product designation for
> this PHY is? It would be good to rename the driver and binding
> if possible. I would still recommend leaving the
> "hisilicon,hix5hd2-sata-phy" string and the "hisilicon,peripheral-syscon"
> property in place, since that indicates a minor variation of the
> generic part.

"hisilicon,peripheral-syscon" is privide power supply, etc.

Do you mean change the compatible to "hisilicon,dwc-sata-phy" or 
multiple compatible?

Thanks

Re: [PATCH v2 1/2] Documentation: Document Hisilicon hix5hd2 sata PHY

[Arnd Bergmann]

On Thursday 26 June 2014 21:48:54 zhangfei wrote:
> 
> On 06/25/2014 09:32 PM, Arnd Bergmann wrote:
> > On Wednesday 25 June 2014 20:41:26 zhangfei wrote:
> >> On 06/25/2014 08:16 PM, Arnd Bergmann wrote:
> >>> On Wednesday 25 June 2014, Zhangfei Gao wrote:
> >>>> From: Jiancheng Xue <xuejiancheng@huawei.com>
> >>>>
> >>>> Add necessary binding documentation SATA PHY on Hisilicon hix5hd2 soc.
> >>>>
> >>>> Signed-off-by: Jiancheng Xue <xuejiancheng@huawei.com>
> >>>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> >>>
> >>> We have had a couple of PHY drivers that were initially meant to be
> >>> single-purpose but then turned out to be for hardware that could
> >>> be put into multiple modes (SATA, gbit ethernet, PCIe, ...). Are
> >>> you sure that this particular device is SATA-only?
> >>>
> >>
> >> Yes, double confimed.
> >> Hix5hd2 phy diver only support sata phy, in fact, current soc only
> >> support synposis phy, since some internal physical link.
> >> And hix5hd2 ethernet phy is already in hix5hd2_gmac.c.
> >> No pcie support on hix5hd2 platfrom.
> >
> > Sorry if I my question was unclear, I did not mean the implementation
> > of this phy in hix5hd2, but the Synopsys phy itself. I was mainly
> > asking because of the "compatible" string, which should otherwise
> > not include "sata" but be more generic.
> 
> This phy is specifically for sata.

Ok.

> > Since you mention that this is a synopsys part, it may however be
> > good to also define a compatible string for the generic device,
> > to allow other SoCs to share this driver, and to make it easier
> > for people to find out that we already have one so they don't have
> > to write another driver for the same hardware.
> 
> http://www.synopsys.com/dw/ipdir.php?ds=dwc_sata_phy
> name can be: dwc_sata_phy
> However, not find register from the website, not sure whether the 
> register are totally same, or there is some change.
> Other soc from hisilicon using the same ip can share this driver.

I found a list of versions under "Downloads and Documentation", but
unfortunately the actual downloads are not accessible without a
customer account.

> > Can you find out what the Synopsys/Designware product designation for
> > this PHY is? It would be good to rename the driver and binding
> > if possible. I would still recommend leaving the
> > "hisilicon,hix5hd2-sata-phy" string and the "hisilicon,peripheral-syscon"
> > property in place, since that indicates a minor variation of the
> > generic part.
> 
> "hisilicon,peripheral-syscon" is privide power supply, etc.
> 
> Do you mean change the compatible to "hisilicon,dwc-sata-phy" or 
> multiple compatible?

Multiple compatible strings.

"hisilicon,dwc-sata-phy" would be too generic anyway, it should include the
name of the chip that first implements this, or the exact version of the
IP block, preferably both.

Ideally if you can find out the version that hisilicon uses, it could
look something like

compatible = "hisilicon,hi15hd2-sata-phy", "snps,dwc-sata-phy-2.3a", ""snps,dwc-sata-phy";

Note also the specific version of the hisilicon chip: you must not use
wildcards in compatible strings but should always use a real product number.
("1" instead of "x"). If multiple chips are mutually compatible, they
can all use the number of the first one here.

	Arnd

Re: [PATCH v2 1/2] Documentation: Document Hisilicon hix5hd2 sata PHY

[zhangfei]

On 06/27/2014 04:53 AM, Arnd Bergmann wrote:
> On Thursday 26 June 2014 21:48:54 zhangfei wrote:

>>> Since you mention that this is a synopsys part, it may however be
>>> good to also define a compatible string for the generic device,
>>> to allow other SoCs to share this driver, and to make it easier
>>> for people to find out that we already have one so they don't have
>>> to write another driver for the same hardware.
>>
>> http://www.synopsys.com/dw/ipdir.php?ds=dwc_sata_phy
>> name can be: dwc_sata_phy
>> However, not find register from the website, not sure whether the
>> register are totally same, or there is some change.
>> Other soc from hisilicon using the same ip can share this driver.
>
> I found a list of versions under "Downloads and Documentation", but
> unfortunately the actual downloads are not accessible without a
> customer account.
>
>>> Can you find out what the Synopsys/Designware product designation for
>>> this PHY is? It would be good to rename the driver and binding
>>> if possible. I would still recommend leaving the
>>> "hisilicon,hix5hd2-sata-phy" string and the "hisilicon,peripheral-syscon"
>>> property in place, since that indicates a minor variation of the
>>> generic part.
>>
>> "hisilicon,peripheral-syscon" is privide power supply, etc.
>>
>> Do you mean change the compatible to "hisilicon,dwc-sata-phy" or
>> multiple compatible?
>
> Multiple compatible strings.
>
> "hisilicon,dwc-sata-phy" would be too generic anyway, it should include the
> name of the chip that first implements this, or the exact version of the
> IP block, preferably both.
>
> Ideally if you can find out the version that hisilicon uses, it could
> look something like
>
> compatible = "hisilicon,hi15hd2-sata-phy", "snps,dwc-sata-phy-2.3a", ""snps,dwc-sata-phy";
>
> Note also the specific version of the hisilicon chip: you must not use
> wildcards in compatible strings but should always use a real product number.
> ("1" instead of "x"). If multiple chips are mutually compatible, they
> can all use the number of the first one here.
>

Sorry for the confusion.

The phy is rather an analog controller, without standard register.
Instead, the phy interface is just some pin / analog interface.
The register is in fact hix5hd2 register, controls all the analog 
output, including reset, power, speed, para tunning.

Even the same phy is used in other soc, they can not share this driver, 
since the connection must be different, as well as internal soc register 
layout.
Only if the same controller & phy are reused in other hisilicon soc, 
this driver can be shared.

Since what we control is hix5hd2 controller itself, so it may not 
suitable to put snps here.


And about hix5hd2 name: x is not wildcard.

Currently hix5hd2 is series of hi3716c v200, hi3719c v100, hi3718c v100.
They are same soc, except minus pin assembles different.

However, not all hi37x is in this series, for example hi3716c v100 is a 
different soc.

In the future hi3719m, hi3718m may also plan to add to hix5hd2 series.
The difference will be different cpu core number, different gpu core 
number. Also use different ethernet controller.


So we may still keep "hisilicon,hix5hd2-sata-phy" unchanged.
What do you think?

Thanks

Re: [PATCH v2 1/2] Documentation: Document Hisilicon hix5hd2 sata PHY

[Arnd Bergmann]

On Friday 27 June 2014 11:37:18 zhangfei wrote:
> >
> 
> Sorry for the confusion.
> 
> The phy is rather an analog controller, without standard register.
> Instead, the phy interface is just some pin / analog interface.
> The register is in fact hix5hd2 register, controls all the analog 
> output, including reset, power, speed, para tunning.

Ok, thanks for the explanation.

> Even the same phy is used in other soc, they can not share this driver, 
> since the connection must be different, as well as internal soc register 
> layout.
> Only if the same controller & phy are reused in other hisilicon soc, 
> this driver can be shared.
> 
> Since what we control is hix5hd2 controller itself, so it may not 
> suitable to put snps here.

Makes sense.

> And about hix5hd2 name: x is not wildcard.
> 
> Currently hix5hd2 is series of hi3716c v200, hi3719c v100, hi3718c v100.
> They are same soc, except minus pin assembles different.
> 
> However, not all hi37x is in this series, for example hi3716c v100 is a 
> different soc.
> 
> In the future hi3719m, hi3718m may also plan to add to hix5hd2 series.
> The difference will be different cpu core number, different gpu core 
> number. Also use different ethernet controller.

Ah, I think you explained this before, sorry for misremembering it.
 
> So we may still keep "hisilicon,hix5hd2-sata-phy" unchanged.
> What do you think?

Yes, please keep this string, it's good.

Thanks for your patience,

	Arnd

Re: [PATCH v2 1/2] Documentation: Document Hisilicon hix5hd2 sata PHY

[Kishon Vijay Abraham I]

Hi,

On Wednesday 25 June 2014 02:44 PM, Zhangfei Gao wrote:
> From: Jiancheng Xue <xuejiancheng@huawei.com>
> 
> Add necessary binding documentation SATA PHY on Hisilicon hix5hd2 soc.
> 
> Signed-off-by: Jiancheng Xue <xuejiancheng@huawei.com>
> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> ---
>  .../devicetree/bindings/phy/hix5hd2-sata-phy.txt   |   22 ++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/hix5hd2-sata-phy.txt
> 
> diff --git a/Documentation/devicetree/bindings/phy/hix5hd2-sata-phy.txt b/Documentation/devicetree/bindings/phy/hix5hd2-sata-phy.txt
> new file mode 100644
> index 0000000..296168b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/hix5hd2-sata-phy.txt

Generally we have a single file for a SoC while adding phy binding
documentation. So just make it hix5hd2-phy.txt.

Thanks
Kishon

Re: [PATCH v2 2/2] phy: add hix5hd2-sata-phy driver

[Kishon Vijay Abraham I]

Hi,

On Wednesday 25 June 2014 02:44 PM, Zhangfei Gao wrote:
> From: Jiancheng Xue <xuejiancheng@huawei.com>
> 
> Add hix5hd2-sata-phy driver on Hisilicon hix5hd2 soc.
> 
> Signed-off-by: Jiancheng Xue <xuejiancheng@huawei.com>
> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> ---
>  drivers/phy/Kconfig            |    8 ++
>  drivers/phy/Makefile           |    1 +
>  drivers/phy/phy-hix5hd2-sata.c |  192 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 201 insertions(+)
>  create mode 100644 drivers/phy/phy-hix5hd2-sata.c
> 
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 16a2f06..782953d 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -109,6 +109,14 @@ config PHY_EXYNOS5250_SATA
>  	  SATA 3.0 Gb/s, SATA 6.0 Gb/s speeds. It supports one SATA host
>  	  port to accept one SATA device.
>  
> +config PHY_HIX5HD2_SATA
> +	tristate "HIX5HD2 SATA PHY Driver"
> +	depends on ARCH_HIX5HD2 && OF

depends on HAS_IOMEM?
> +	select GENERIC_PHY
> +	select MFD_SYSCON
> +	help
> +	  Support for SATA PHY on Hisilicon hix5hd2 Soc.
> +
>  config PHY_SUN4I_USB
>  	tristate "Allwinner sunxi SoC USB PHY driver"
>  	depends on ARCH_SUNXI && HAS_IOMEM && OF
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index b4f1d57..54f04d0 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_OMAP_USB2)			+= phy-omap-usb2.o
>  obj-$(CONFIG_TI_PIPE3)			+= phy-ti-pipe3.o
>  obj-$(CONFIG_TWL4030_USB)		+= phy-twl4030-usb.o
>  obj-$(CONFIG_PHY_EXYNOS5250_SATA)	+= phy-exynos5250-sata.o
> +obj-$(CONFIG_PHY_HIX5HD2_SATA)		+= phy-hix5hd2-sata.o
>  obj-$(CONFIG_PHY_SUN4I_USB)		+= phy-sun4i-usb.o
>  obj-$(CONFIG_PHY_SAMSUNG_USB2)		+= phy-exynos-usb2.o
>  phy-exynos-usb2-y			+= phy-samsung-usb2.o
> diff --git a/drivers/phy/phy-hix5hd2-sata.c b/drivers/phy/phy-hix5hd2-sata.c
> new file mode 100644
> index 0000000..6f1e3ea
> --- /dev/null
> +++ b/drivers/phy/phy-hix5hd2-sata.c
> @@ -0,0 +1,192 @@
> +/*
> + * Copyright (c) 2014 Linaro Ltd.
> + * Copyright (c) 2014 Hisilicon Limited.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
> +#include <linux/phy/phy.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +#include <linux/delay.h>

can this be arranged alphabetically? Helps while adding new header files.
> +
> +#define SATA_PHY0_CTLL		0xa0
> +#define MPLL_MULTIPLIER_SHIFT	1
> +#define MPLL_MULTIPLIER_MASK	0xfe
> +#define MPLL_MULTIPLIER_50M	0x3c
> +#define MPLL_MULTIPLIER_100M	0x1e
> +#define PHY_RESET		BIT(0)
> +#define REF_SSP_EN		BIT(9)
> +#define SSC_EN			BIT(10)
> +#define REF_USE_PAD		BIT(23)
> +
> +#define SATA_PORT_PHYCTL	0x174
> +#define SPEED_MODE_MASK		0x6f0000
> +#define HALF_RATE_SHIFT		16
> +#define PHY_CONFIG_SHIFT	18
> +#define GEN2_EN_SHIFT		21
> +#define SPEED_CTRL		BIT(20)
> +
> +#define SATA_PORT_PHYCTL1	0x148
> +#define AMPLITUDE_MASK		0x3ffffe
> +#define AMPLITUDE_GEN3		0x68
> +#define AMPLITUDE_GEN3_SHIFT	15
> +#define AMPLITUDE_GEN2		0x56
> +#define AMPLITUDE_GEN2_SHIFT	8
> +#define AMPLITUDE_GEN1		0x56
> +#define AMPLITUDE_GEN1_SHIFT	1
> +
.
.
<snip>
.
.
> +
> +static int hix5hd2_sata_phy_probe(struct platform_device *pdev)
> +{
> +	struct phy_provider *phy_provider;
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	struct phy *phy;
> +	struct hix5hd2_priv *priv;
> +
> +	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 (!priv->base)
> +		return -ENOMEM;
> +
> +	priv->peri_ctrl = syscon_regmap_lookup_by_phandle(dev->of_node,
> +						"hisilicon,peripheral-syscon");
> +	if (IS_ERR(priv->peri_ctrl))
> +		priv->peri_ctrl = NULL;

if syscon_regmap_lookup_by_phandle returns EPROBE_DEFER, you have to return
EPROBE_DEFER no?

Rest looks fine to me.

Thanks
Kishon

Re: [PATCH v2 2/2] phy: add hix5hd2-sata-phy driver

[Kishon Vijay Abraham I]

Hi,

On Wednesday 02 July 2014 04:55 PM, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Wednesday 25 June 2014 02:44 PM, Zhangfei Gao wrote:
>> From: Jiancheng Xue <xuejiancheng@huawei.com>
>>
>> Add hix5hd2-sata-phy driver on Hisilicon hix5hd2 soc.
>>
>> Signed-off-by: Jiancheng Xue <xuejiancheng@huawei.com>
>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>> ---
>>  drivers/phy/Kconfig            |    8 ++
>>  drivers/phy/Makefile           |    1 +
>>  drivers/phy/phy-hix5hd2-sata.c |  192 ++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 201 insertions(+)
>>  create mode 100644 drivers/phy/phy-hix5hd2-sata.c
>>
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> index 16a2f06..782953d 100644
>> --- a/drivers/phy/Kconfig
>> +++ b/drivers/phy/Kconfig
>> @@ -109,6 +109,14 @@ config PHY_EXYNOS5250_SATA
>>  	  SATA 3.0 Gb/s, SATA 6.0 Gb/s speeds. It supports one SATA host
>>  	  port to accept one SATA device.
>>  
>> +config PHY_HIX5HD2_SATA
>> +	tristate "HIX5HD2 SATA PHY Driver"
>> +	depends on ARCH_HIX5HD2 && OF
> 
> depends on HAS_IOMEM?
>> +	select GENERIC_PHY
>> +	select MFD_SYSCON
>> +	help
>> +	  Support for SATA PHY on Hisilicon hix5hd2 Soc.
>> +
>>  config PHY_SUN4I_USB
>>  	tristate "Allwinner sunxi SoC USB PHY driver"
>>  	depends on ARCH_SUNXI && HAS_IOMEM && OF
>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
>> index b4f1d57..54f04d0 100644
>> --- a/drivers/phy/Makefile
>> +++ b/drivers/phy/Makefile
>> @@ -12,6 +12,7 @@ obj-$(CONFIG_OMAP_USB2)			+= phy-omap-usb2.o
>>  obj-$(CONFIG_TI_PIPE3)			+= phy-ti-pipe3.o
>>  obj-$(CONFIG_TWL4030_USB)		+= phy-twl4030-usb.o
>>  obj-$(CONFIG_PHY_EXYNOS5250_SATA)	+= phy-exynos5250-sata.o
>> +obj-$(CONFIG_PHY_HIX5HD2_SATA)		+= phy-hix5hd2-sata.o
>>  obj-$(CONFIG_PHY_SUN4I_USB)		+= phy-sun4i-usb.o
>>  obj-$(CONFIG_PHY_SAMSUNG_USB2)		+= phy-exynos-usb2.o
>>  phy-exynos-usb2-y			+= phy-samsung-usb2.o
>> diff --git a/drivers/phy/phy-hix5hd2-sata.c b/drivers/phy/phy-hix5hd2-sata.c
>> new file mode 100644
>> index 0000000..6f1e3ea
>> --- /dev/null
>> +++ b/drivers/phy/phy-hix5hd2-sata.c
>> @@ -0,0 +1,192 @@
>> +/*
>> + * Copyright (c) 2014 Linaro Ltd.
>> + * Copyright (c) 2014 Hisilicon Limited.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/regmap.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/io.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/delay.h>
> 
> can this be arranged alphabetically? Helps while adding new header files.
>> +
>> +#define SATA_PHY0_CTLL		0xa0
>> +#define MPLL_MULTIPLIER_SHIFT	1
>> +#define MPLL_MULTIPLIER_MASK	0xfe
>> +#define MPLL_MULTIPLIER_50M	0x3c
>> +#define MPLL_MULTIPLIER_100M	0x1e
>> +#define PHY_RESET		BIT(0)
>> +#define REF_SSP_EN		BIT(9)
>> +#define SSC_EN			BIT(10)
>> +#define REF_USE_PAD		BIT(23)
>> +
>> +#define SATA_PORT_PHYCTL	0x174
>> +#define SPEED_MODE_MASK		0x6f0000
>> +#define HALF_RATE_SHIFT		16
>> +#define PHY_CONFIG_SHIFT	18
>> +#define GEN2_EN_SHIFT		21
>> +#define SPEED_CTRL		BIT(20)
>> +
>> +#define SATA_PORT_PHYCTL1	0x148
>> +#define AMPLITUDE_MASK		0x3ffffe
>> +#define AMPLITUDE_GEN3		0x68
>> +#define AMPLITUDE_GEN3_SHIFT	15
>> +#define AMPLITUDE_GEN2		0x56
>> +#define AMPLITUDE_GEN2_SHIFT	8
>> +#define AMPLITUDE_GEN1		0x56
>> +#define AMPLITUDE_GEN1_SHIFT	1
>> +
> .
> .
> <snip>
> .
> .
>> +
>> +static int hix5hd2_sata_phy_probe(struct platform_device *pdev)
>> +{
>> +	struct phy_provider *phy_provider;
>> +	struct device *dev = &pdev->dev;
>> +	struct resource *res;
>> +	struct phy *phy;
>> +	struct hix5hd2_priv *priv;
>> +
>> +	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));

It should be devm_ioremap_resource().

-Kishon
>> +	if (!priv->base)
>> +		return -ENOMEM;
>> +
>> +	priv->peri_ctrl = syscon_regmap_lookup_by_phandle(dev->of_node,
>> +						"hisilicon,peripheral-syscon");
>> +	if (IS_ERR(priv->peri_ctrl))
>> +		priv->peri_ctrl = NULL;
> 
> if syscon_regmap_lookup_by_phandle returns EPROBE_DEFER, you have to return
> EPROBE_DEFER no?
> 
> Rest looks fine to me.
> 
> Thanks
> Kishon
> 

Re: [PATCH v2 2/2] phy: add hix5hd2-sata-phy driver

[zhangfei]

Hi, Kishon

On 07/02/2014 07:26 PM, Kishon Vijay Abraham I wrote:

>>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>>> index 16a2f06..782953d 100644
>>> --- a/drivers/phy/Kconfig
>>> +++ b/drivers/phy/Kconfig
>>> @@ -109,6 +109,14 @@ config PHY_EXYNOS5250_SATA
>>>   	  SATA 3.0 Gb/s, SATA 6.0 Gb/s speeds. It supports one SATA host
>>>   	  port to accept one SATA device.
>>>
>>> +config PHY_HIX5HD2_SATA
>>> +	tristate "HIX5HD2 SATA PHY Driver"
>>> +	depends on ARCH_HIX5HD2 && OF
>>
>> depends on HAS_IOMEM?

Yes, will add it.
HAS_IOMEM is select by default and not notice it at all.
>>> +	select GENERIC_PHY
>>> +	select MFD_SYSCON
>>> +	help
>>> +	  Support for SATA PHY on Hisilicon hix5hd2 Soc.
>>> +
>>>   config PHY_SUN4I_USB
>>>   	tristate "Allwinner sunxi SoC USB PHY driver"
>>>   	depends on ARCH_SUNXI && HAS_IOMEM && OF
>>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
>>> index b4f1d57..54f04d0 100644
>>> --- a/drivers/phy/Makefile
>>> +++ b/drivers/phy/Makefile
>>> @@ -12,6 +12,7 @@ obj-$(CONFIG_OMAP_USB2)			+= phy-omap-usb2.o
>>>   obj-$(CONFIG_TI_PIPE3)			+= phy-ti-pipe3.o
>>>   obj-$(CONFIG_TWL4030_USB)		+= phy-twl4030-usb.o
>>>   obj-$(CONFIG_PHY_EXYNOS5250_SATA)	+= phy-exynos5250-sata.o
>>> +obj-$(CONFIG_PHY_HIX5HD2_SATA)		+= phy-hix5hd2-sata.o
>>>   obj-$(CONFIG_PHY_SUN4I_USB)		+= phy-sun4i-usb.o
>>>   obj-$(CONFIG_PHY_SAMSUNG_USB2)		+= phy-exynos-usb2.o
>>>   phy-exynos-usb2-y			+= phy-samsung-usb2.o
>>> diff --git a/drivers/phy/phy-hix5hd2-sata.c b/drivers/phy/phy-hix5hd2-sata.c
>>> new file mode 100644
>>> index 0000000..6f1e3ea
>>> --- /dev/null
>>> +++ b/drivers/phy/phy-hix5hd2-sata.c
>>> @@ -0,0 +1,192 @@
>>> +/*
>>> + * Copyright (c) 2014 Linaro Ltd.
>>> + * Copyright (c) 2014 Hisilicon Limited.
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + */
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/mfd/syscon.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/phy/phy.h>
>>> +#include <linux/io.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/delay.h>
>>
>> can this be arranged alphabetically? Helps while adding new header files.
Sure

>>> +static int hix5hd2_sata_phy_probe(struct platform_device *pdev)
>>> +{
>>> +	struct phy_provider *phy_provider;
>>> +	struct device *dev = &pdev->dev;
>>> +	struct resource *res;
>>> +	struct phy *phy;
>>> +	struct hix5hd2_priv *priv;
>>> +
>>> +	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));
>
> It should be devm_ioremap_resource().
Since these memory region shared by other device, devm_ioremap_resource 
will fail, since devm_request_mem_region.

>
> -Kishon
>>> +	if (!priv->base)
>>> +		return -ENOMEM;
>>> +
>>> +	priv->peri_ctrl = syscon_regmap_lookup_by_phandle(dev->of_node,
>>> +						"hisilicon,peripheral-syscon");
>>> +	if (IS_ERR(priv->peri_ctrl))
>>> +		priv->peri_ctrl = NULL;
>>
>> if syscon_regmap_lookup_by_phandle returns EPROBE_DEFER, you have to return
>> EPROBE_DEFER no?
Sure,
However, postcore_initcall(syscon_init) is bound to init before 
module_platform_driver probe, so EPROBE_DEFER will not occur.

>>
>> Rest looks fine to me.
>>
>> Thanks
>> Kishon
>>