[PATCH 00/10] ARM: da850-lcdk: add SATA support

[PATCH 00/10] ARM: da850-lcdk: add SATA support

[Bartosz Golaszewski]

This series contains all the changes necessary to make SATA work on
the da850-lcdk board.

The first patch adds a clock lookup entry required for the ahci core
to retrieve a functional clock.

The second enables relevant config options for all davinci boards.

The third adds device tree bindings for the ahci_da850 driver.

The fourth adds a workaround for a SATA controller instability we
detected after increasing the PLL0 frequency for proper LCD
controller support.

Patches 5 through 7 extend the ahci_da850 driver - add DT support,
un-hardcode the clock multiplier value and add a workaround for
a quirk present on the da850 SATA controller.

Patches 8-10 add the device tree changes required to probe the driver.

I'm posting the series as a whole to give all reviewers the full
picture and visibility of the changes required, if needed I can resend
the patches separately.

Bartosz Golaszewski (10):
  ARM: davinci: add a clock lookup entry for the SATA clock
  ARM: davinci_all_defconfig: enable SATA modules
  devicetree: bindings: add bindings for ahci-da850
  sata: hardreset: retry if phys link is down
  sata: ahci_da850: add device tree match table
  sata: ahci_da850: implement a softreset quirk
  sata: ahci_da850: add support for the da850,clk_multiplier DT property
  ARM: dts: da850: add pinmux settings for the SATA controller
  ARM: dts: da850: add the SATA node
  ARM: dts: da850-lcdk: enable the SATA node

 .../devicetree/bindings/ata/ahci-da850.txt         |  21 ++++
 arch/arm/boot/dts/da850-lcdk.dts                   |   5 +
 arch/arm/boot/dts/da850.dtsi                       |  30 ++++++
 arch/arm/configs/davinci_all_defconfig             |   2 +
 arch/arm/mach-davinci/da8xx-dt.c                   |   1 +
 drivers/ata/ahci_da850.c                           | 112 +++++++++++++++++++--
 drivers/ata/libata-core.c                          |  16 ++-
 include/linux/libata.h                             |   4 +-
 8 files changed, 177 insertions(+), 14 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/ata/ahci-da850.txt

-- 
2.9.3

[PATCH 01/10] ARM: davinci: add a clock lookup entry for the SATA clock

[Bartosz Golaszewski]

This entry is needed for the ahci driver to get a functional clock.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/mach-davinci/da8xx-dt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c
index 9ee44da..b83e5d1 100644
--- a/arch/arm/mach-davinci/da8xx-dt.c
+++ b/arch/arm/mach-davinci/da8xx-dt.c
@@ -42,6 +42,7 @@ static struct of_dev_auxdata da850_auxdata_lookup[] __initdata = {
 	OF_DEV_AUXDATA("ti,da830-ohci", 0x01e25000, "ohci-da8xx", NULL),
 	OF_DEV_AUXDATA("ti,da830-musb", 0x01e00000, "musb-da8xx", NULL),
 	OF_DEV_AUXDATA("ti,da830-usb-phy", 0x01c1417c, "da8xx-usb-phy", NULL),
+	OF_DEV_AUXDATA("ti,da850-ahci", 0x01e18000, "ahci_da850", NULL),
 	{}
 };
 
-- 
2.9.3

[PATCH 02/10] ARM: davinci_all_defconfig: enable SATA modules

[Bartosz Golaszewski]

Add the da850-ahci driver to davinci defconfig.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/configs/davinci_all_defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/configs/davinci_all_defconfig b/arch/arm/configs/davinci_all_defconfig
index 8806754..a1b9c58 100644
--- a/arch/arm/configs/davinci_all_defconfig
+++ b/arch/arm/configs/davinci_all_defconfig
@@ -78,6 +78,8 @@ CONFIG_IDE=m
 CONFIG_BLK_DEV_PALMCHIP_BK3710=m
 CONFIG_SCSI=m
 CONFIG_BLK_DEV_SD=m
+CONFIG_ATA=m
+CONFIG_AHCI_DA850=m
 CONFIG_NETDEVICES=y
 CONFIG_NETCONSOLE=y
 CONFIG_TUN=m
-- 
2.9.3

[PATCH 03/10] devicetree: bindings: add bindings for ahci-da850

[Bartosz Golaszewski]

Add DT bindings for the TI DA850 AHCI SATA controller.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 .../devicetree/bindings/ata/ahci-da850.txt          | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ata/ahci-da850.txt

diff --git a/Documentation/devicetree/bindings/ata/ahci-da850.txt b/Documentation/devicetree/bindings/ata/ahci-da850.txt
new file mode 100644
index 0000000..d07c241
--- /dev/null
+++ b/Documentation/devicetree/bindings/ata/ahci-da850.txt
@@ -0,0 +1,21 @@
+Device tree binding for the TI DA850 AHCI SATA Controller
+---------------------------------------------------------
+
+Required properties:
+  - compatible: must be "ti,da850-ahci"
+  - reg: physical base addresses and sizes of the controller's register areas
+  - interrupts: interrupt specifier (refer to the interrupt binding)
+
+Optional properties:
+  - clocks: clock specifier (refer to the common clock binding)
+  - da850,clk_multiplier: the multiplier for the reference clock needed
+                          for 1.5GHz PLL output
+
+Example:
+
+	sata: ahci@0x218000 {
+		compatible = "ti,da850-ahci";
+		reg = <0x218000 0x2000>, <0x22c018 0x4>;
+		interrupts = <67>;
+		da850,clk_multiplier = <7>;
+	};
-- 
2.9.3

[PATCH 04/10] sata: hardreset: retry if phys link is down

[Bartosz Golaszewski]

The sata core driver already retries to resume the link because some
controllers ignore writes to the SControl register.

We have a use case with the da850 SATA controller where at PLL0
frequency of 456MHz (needed to properly service the LCD controller)
the chip becomes unstable and the hardreset operation is ignored the
first time 50% of times.

Retrying just the resume operation doesn't work - we need to issue
the phy/wake reset again to make it work.

If ata_phys_link_offline() returns true in sata_link_hardreset(),
retry a couple times before really giving up.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/ata/libata-core.c | 16 ++++++++++++----
 include/linux/libata.h    |  4 +++-
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 9cd0a2d..3b848a3 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -3985,8 +3985,8 @@ int sata_link_hardreset(struct ata_link *link, const unsigned long *timing,
 			unsigned long deadline,
 			bool *online, int (*check_ready)(struct ata_link *))
 {
+	int rc, retry = ATA_LINK_RESET_TRIES;
 	u32 scontrol;
-	int rc;
 
 	DPRINTK("ENTER\n");
 
@@ -4009,7 +4009,7 @@ int sata_link_hardreset(struct ata_link *link, const unsigned long *timing,
 
 		sata_set_spd(link);
 	}
-
+retry:
 	/* issue phy wake/reset */
 	if ((rc = sata_scr_read(link, SCR_CONTROL, &scontrol)))
 		goto out;
@@ -4028,9 +4028,17 @@ int sata_link_hardreset(struct ata_link *link, const unsigned long *timing,
 	rc = sata_link_resume(link, timing, deadline);
 	if (rc)
 		goto out;
-	/* if link is offline nothing more to do */
-	if (ata_phys_link_offline(link))
+
+	if (ata_phys_link_offline(link)) {
+		if (retry--) {
+			ata_link_warn(link,
+				      "link still offline after hardreset - retrying\n");
+			goto retry;
+		}
+
+		/* if link is still offline nothing more to do */
 		goto out;
+	}
 
 	/* Link is online.  From this point, -ENODEV too is an error. */
 	if (online)
diff --git a/include/linux/libata.h b/include/linux/libata.h
index c170be5..2c840c0 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -392,8 +392,10 @@ enum {
 	/* max tries if error condition is still set after ->error_handler */
 	ATA_EH_MAX_TRIES	= 5,
 
-	/* sometimes resuming a link requires several retries */
+	/* sometimes resuming a link requires several retries... */
 	ATA_LINK_RESUME_TRIES	= 5,
+	/* ... and sometimes we need to retry the whole reset procedure */
+	ATA_LINK_RESET_TRIES	= 5,
 
 	/* how hard are we gonna try to probe/recover devices */
 	ATA_PROBE_MAX_TRIES	= 3,
-- 
2.9.3

[PATCH 05/10] sata: ahci_da850: add device tree match table

[Bartosz Golaszewski]

We're using device tree for da850-lcdk. Add the match table to allow
to probe the driver.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/ata/ahci_da850.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/ata/ahci_da850.c b/drivers/ata/ahci_da850.c
index 267a3d3..5930af81 100644
--- a/drivers/ata/ahci_da850.c
+++ b/drivers/ata/ahci_da850.c
@@ -105,11 +105,18 @@ static int ahci_da850_probe(struct platform_device *pdev)
 static SIMPLE_DEV_PM_OPS(ahci_da850_pm_ops, ahci_platform_suspend,
 			 ahci_platform_resume);
 
+static const struct of_device_id ahci_da850_of_match[] = {
+	{ .compatible = "ti,da850-ahci", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ahci_da850_of_match);
+
 static struct platform_driver ahci_da850_driver = {
 	.probe = ahci_da850_probe,
 	.remove = ata_platform_remove_one,
 	.driver = {
 		.name = DRV_NAME,
+		.of_match_table = ahci_da850_of_match,
 		.pm = &ahci_da850_pm_ops,
 	},
 };
-- 
2.9.3

[PATCH 06/10] sata: ahci_da850: implement a softreset quirk

[Bartosz Golaszewski]

There's an issue with the da850 SATA controller: if port multiplier
support is compiled in, but we're connecting the drive directly to
the SATA port on the board, the drive can't be detected.

To make SATA work on the da850-lcdk board: first try to softreset
with pmp - if the operation fails with -EBUSY, retry without pmp.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/ata/ahci_da850.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/ahci_da850.c b/drivers/ata/ahci_da850.c
index 5930af81..bb9eb4c 100644
--- a/drivers/ata/ahci_da850.c
+++ b/drivers/ata/ahci_da850.c
@@ -54,11 +54,31 @@ static void da850_sata_init(struct device *dev, void __iomem *pwrdn_reg,
 	writel(val, ahci_base + SATA_P0PHYCR_REG);
 }
 
+static int ahci_da850_softreset(struct ata_link *link,
+				unsigned int *class, unsigned long deadline)
+{
+	int pmp, ret;
+
+	pmp = sata_srst_pmp(link);
+
+	ret = ahci_do_softreset(link, class, pmp, deadline, ahci_check_ready);
+	if (pmp && ret == -EBUSY)
+		return ahci_do_softreset(link, class, 0,
+					 deadline, ahci_check_ready);
+
+	return ret;
+}
+
+static struct ata_port_operations ahci_da850_port_ops = {
+	.inherits = &ahci_platform_ops,
+	.softreset = ahci_da850_softreset,
+};
+
 static const struct ata_port_info ahci_da850_port_info = {
 	.flags		= AHCI_FLAG_COMMON,
 	.pio_mask	= ATA_PIO4,
 	.udma_mask	= ATA_UDMA6,
-	.port_ops	= &ahci_platform_ops,
+	.port_ops	= &ahci_da850_port_ops,
 };
 
 static struct scsi_host_template ahci_platform_sht = {
-- 
2.9.3

[PATCH 07/10] sata: ahci_da850: add support for the da850,clk_multiplier DT property

[Bartosz Golaszewski]

Currently the clock multiplier is hardcoded in the driver for
the da850-evm board. Make it configurable over DT, but keep the
previous value as default in case the property is missing.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/ata/ahci_da850.c | 83 +++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 75 insertions(+), 8 deletions(-)

diff --git a/drivers/ata/ahci_da850.c b/drivers/ata/ahci_da850.c
index bb9eb4c..cd04caf 100644
--- a/drivers/ata/ahci_da850.c
+++ b/drivers/ata/ahci_da850.c
@@ -28,17 +28,70 @@
 #define SATA_PHY_TXSWING(x)	((x) << 19)
 #define SATA_PHY_ENPLL(x)	((x) << 31)
 
+struct da850_sata_mpy_mapping {
+	unsigned int multiplier;
+	unsigned int regval;
+};
+
+static const struct da850_sata_mpy_mapping da850_sata_mpy_table[] = {
+	{
+		.multiplier	= 5,
+		.regval		= 0x01,
+	},
+	{
+		.multiplier	= 6,
+		.regval		= 0x02,
+	},
+	{
+		.multiplier	= 8,
+		.regval		= 0x04,
+	},
+	{
+		.multiplier	= 10,
+		.regval		= 0x05,
+	},
+	{
+		.multiplier	= 12,
+		.regval		= 0x06,
+	},
+	/* TODO Add 12.5 multiplier. */
+	{
+		.multiplier	= 15,
+		.regval		= 0x08,
+	},
+	{
+		.multiplier	= 20,
+		.regval		= 0x09,
+	},
+	{
+		.multiplier	= 25,
+		.regval		= 0x0a,
+	}
+};
+
+static const struct da850_sata_mpy_mapping *
+da850_sata_get_mpy(unsigned int multiplier)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(da850_sata_mpy_table); i++)
+		if (da850_sata_mpy_table[i].multiplier == multiplier)
+			return &da850_sata_mpy_table[i];
+
+	return NULL;
+}
+
 /*
  * The multiplier needed for 1.5GHz PLL output.
  *
- * NOTE: This is currently hardcoded to be suitable for 100MHz crystal
- * frequency (which is used by DA850 EVM board) and may need to be changed
- * if you would like to use this driver on some other board.
+ * This is the default value suitable for the 100MHz crystal frequency
+ * used by DA850 EVM board, which doesn't use DT.
  */
-#define DA850_SATA_CLK_MULTIPLIER	7
+#define DA850_SATA_CLK_MULTIPLIER_DEFAULT	15
 
 static void da850_sata_init(struct device *dev, void __iomem *pwrdn_reg,
-			    void __iomem *ahci_base)
+			    void __iomem *ahci_base,
+			    const struct da850_sata_mpy_mapping *mpy)
 {
 	unsigned int val;
 
@@ -47,7 +100,7 @@ static void da850_sata_init(struct device *dev, void __iomem *pwrdn_reg,
 	val &= ~BIT(0);
 	writel(val, pwrdn_reg);
 
-	val = SATA_PHY_MPY(DA850_SATA_CLK_MULTIPLIER + 1) | SATA_PHY_LOS(1) |
+	val = SATA_PHY_MPY(mpy->regval) | SATA_PHY_LOS(1) |
 	      SATA_PHY_RXCDR(4) | SATA_PHY_RXEQ(1) | SATA_PHY_TXSWING(3) |
 	      SATA_PHY_ENPLL(1);
 
@@ -87,10 +140,12 @@ static struct scsi_host_template ahci_platform_sht = {
 
 static int ahci_da850_probe(struct platform_device *pdev)
 {
+	const struct da850_sata_mpy_mapping *mpy;
 	struct device *dev = &pdev->dev;
 	struct ahci_host_priv *hpriv;
-	struct resource *res;
+	unsigned int multiplier;
 	void __iomem *pwrdn_reg;
+	struct resource *res;
 	int rc;
 
 	hpriv = ahci_platform_get_resources(pdev);
@@ -109,7 +164,19 @@ static int ahci_da850_probe(struct platform_device *pdev)
 	if (!pwrdn_reg)
 		goto disable_resources;
 
-	da850_sata_init(dev, pwrdn_reg, hpriv->mmio);
+	rc = of_property_read_u32(dev->of_node,
+				  "da850,clk_multiplier", &multiplier);
+	if (rc)
+		multiplier = DA850_SATA_CLK_MULTIPLIER_DEFAULT;
+
+	mpy = da850_sata_get_mpy(multiplier);
+	if (!mpy) {
+		dev_err(dev, "invalid multiplier value: %u\n", multiplier);
+		rc = -EINVAL;
+		goto disable_resources;
+	}
+
+	da850_sata_init(dev, pwrdn_reg, hpriv->mmio, mpy);
 
 	rc = ahci_platform_init_host(pdev, hpriv, &ahci_da850_port_info,
 				     &ahci_platform_sht);
-- 
2.9.3

[PATCH 08/10] ARM: dts: da850: add pinmux settings for the SATA controller

[Bartosz Golaszewski]

Add pinmux sub-nodes for all muxed SATA pins.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/boot/dts/da850.dtsi | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 104155d..1f6a47d 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -78,6 +78,30 @@
 					0x10 0x00220000 0x00ff0000
 				>;
 			};
+			sata_cp_det_pin: pinmux_sata_cp_det_pin {
+				pinctrl-single,bits = <
+					/* SATA_CP_DET */
+					0x0c 0x00000000 0xf0000000
+				>;
+			};
+			sata_mp_switch_pin: pinmux_sata_mp_switch_pin {
+				pinctrl-single,bits = <
+					/* SATA_MP_SWITCH */
+					0x0c 0x00000000 0x0f000000
+				>;
+			};
+			sata_cp_pod_pin: pinmux_sata_cp_pod_pin {
+				pinctrl-single,bits = <
+					/* SATA_CP_POD */
+					0x10 0x40000000 0xf0000000
+				>;
+			};
+			sata_led_pin: pinmux_sata_led_pin {
+				pinctrl-single,bits = <
+					/* SATA_LED */
+					0x10 0x04000000 0x0f000000
+				>;
+			};
 			i2c0_pins: pinmux_i2c0_pins {
 				pinctrl-single,bits = <
 					/* I2C0_SDA,I2C0_SCL */
-- 
2.9.3

[PATCH 09/10] ARM: dts: da850: add the SATA node

[Bartosz Golaszewski]

Add the SATA node to the da850 device tree.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/boot/dts/da850.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 1f6a47d..f5086b1 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -427,6 +427,12 @@
 			phy-names = "usb-phy";
 			status = "disabled";
 		};
+		sata: ahci@0x218000 {
+			compatible = "ti,da850-ahci";
+			reg = <0x218000 0x2000>, <0x22c018 0x4>;
+			interrupts = <67>;
+			status = "disabled";
+		};
 		mdio: mdio@224000 {
 			compatible = "ti,davinci_mdio";
 			#address-cells = <1>;
-- 
2.9.3

[PATCH 10/10] ARM: dts: da850-lcdk: enable the SATA node

[Bartosz Golaszewski]

Enable the SATA node for da850-lcdk. We omit the pinctrl property on
purpose - the muxed SATA pins are not hooked up to anything
SATA-related on the lcdk.

The REFCLKN/P rate on the board is 100MHz, so we need a multiplier of
15 for 1.5GHz PLL rate.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/boot/dts/da850-lcdk.dts | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/da850-lcdk.dts b/arch/arm/boot/dts/da850-lcdk.dts
index afcb482..1e638da 100644
--- a/arch/arm/boot/dts/da850-lcdk.dts
+++ b/arch/arm/boot/dts/da850-lcdk.dts
@@ -105,6 +105,11 @@
 	status = "okay";
 };
 
+&sata {
+	status = "okay";
+	da850,clk_multiplier = <15>;
+};
+
 &mdio {
 	pinctrl-names = "default";
 	pinctrl-0 = <&mdio_pins>;
-- 
2.9.3

Re: [PATCH 00/10] ARM: da850-lcdk: add SATA support

[Sekhar Nori]

On Friday 13 January 2017 06:07 PM, Bartosz Golaszewski wrote:
> This series contains all the changes necessary to make SATA work on
> the da850-lcdk board.
> 
> The first patch adds a clock lookup entry required for the ahci core
> to retrieve a functional clock.
> 
> The second enables relevant config options for all davinci boards.
> 
> The third adds device tree bindings for the ahci_da850 driver.
> 
> The fourth adds a workaround for a SATA controller instability we
> detected after increasing the PLL0 frequency for proper LCD
> controller support.
> 
> Patches 5 through 7 extend the ahci_da850 driver - add DT support,
> un-hardcode the clock multiplier value and add a workaround for
> a quirk present on the da850 SATA controller.
> 
> Patches 8-10 add the device tree changes required to probe the driver.
> 
> I'm posting the series as a whole to give all reviewers the full
> picture and visibility of the changes required, if needed I can resend
> the patches separately.

I just tested this series on my LCDK board using a Western Digital SATA
HDD and it works great with some basic read / write tests. Thanks!

For the non-platform patches which I wont be queuing:

Tested-by: Sekhar Nori <nsekhar@ti.com>

I will take a look at the series closely next week.

Thanks,
Sekhar

Re: [PATCH 03/10] devicetree: bindings: add bindings for ahci-da850

[David Lechner]

On 01/13/2017 06:37 AM, Bartosz Golaszewski wrote:
> Add DT bindings for the TI DA850 AHCI SATA controller.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  .../devicetree/bindings/ata/ahci-da850.txt          | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/ata/ahci-da850.txt
>
> diff --git a/Documentation/devicetree/bindings/ata/ahci-da850.txt b/Documentation/devicetree/bindings/ata/ahci-da850.txt
> new file mode 100644
> index 0000000..d07c241
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ata/ahci-da850.txt
> @@ -0,0 +1,21 @@
> +Device tree binding for the TI DA850 AHCI SATA Controller
> +---------------------------------------------------------
> +
> +Required properties:
> +  - compatible: must be "ti,da850-ahci"
> +  - reg: physical base addresses and sizes of the controller's register areas
> +  - interrupts: interrupt specifier (refer to the interrupt binding)
> +
> +Optional properties:
> +  - clocks: clock specifier (refer to the common clock binding)
> +  - da850,clk_multiplier: the multiplier for the reference clock needed
> +                          for 1.5GHz PLL output

A clock multiplier property seems redundant if you are specifying a 
clock. It should be possible to get the rate from the clock to determine 
which multiplier is needed.

> +
> +Example:
> +
> +	sata: ahci@0x218000 {
> +		compatible = "ti,da850-ahci";
> +		reg = <0x218000 0x2000>, <0x22c018 0x4>;
> +		interrupts = <67>;
> +		da850,clk_multiplier = <7>;
> +	};
>

Re: [PATCH 07/10] sata: ahci_da850: add support for the da850,clk_multiplier DT property

[David Lechner]

On 01/13/2017 06:38 AM, Bartosz Golaszewski wrote:
> Currently the clock multiplier is hardcoded in the driver for
> the da850-evm board. Make it configurable over DT, but keep the
> previous value as default in case the property is missing.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/ata/ahci_da850.c | 83 +++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 75 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/ata/ahci_da850.c b/drivers/ata/ahci_da850.c
> index bb9eb4c..cd04caf 100644
> --- a/drivers/ata/ahci_da850.c
> +++ b/drivers/ata/ahci_da850.c
> @@ -28,17 +28,70 @@
>  #define SATA_PHY_TXSWING(x)	((x) << 19)
>  #define SATA_PHY_ENPLL(x)	((x) << 31)
>
> +struct da850_sata_mpy_mapping {
> +	unsigned int multiplier;
> +	unsigned int regval;
> +};
> +
> +static const struct da850_sata_mpy_mapping da850_sata_mpy_table[] = {
> +	{
> +		.multiplier	= 5,
> +		.regval		= 0x01,
> +	},
> +	{
> +		.multiplier	= 6,
> +		.regval		= 0x02,
> +	},
> +	{
> +		.multiplier	= 8,
> +		.regval		= 0x04,
> +	},
> +	{
> +		.multiplier	= 10,
> +		.regval		= 0x05,
> +	},
> +	{
> +		.multiplier	= 12,
> +		.regval		= 0x06,
> +	},
> +	/* TODO Add 12.5 multiplier. */

Looks like you should be using an enum here for the multiplier field.

> +	{
> +		.multiplier	= 15,
> +		.regval		= 0x08,
> +	},
> +	{
> +		.multiplier	= 20,
> +		.regval		= 0x09,
> +	},
> +	{
> +		.multiplier	= 25,
> +		.regval		= 0x0a,
> +	}
> +};
> +
> +static const struct da850_sata_mpy_mapping *
> +da850_sata_get_mpy(unsigned int multiplier)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(da850_sata_mpy_table); i++)
> +		if (da850_sata_mpy_table[i].multiplier == multiplier)
> +			return &da850_sata_mpy_table[i];
> +
> +	return NULL;
> +}
> +
>  /*
>   * The multiplier needed for 1.5GHz PLL output.
>   *
> - * NOTE: This is currently hardcoded to be suitable for 100MHz crystal
> - * frequency (which is used by DA850 EVM board) and may need to be changed
> - * if you would like to use this driver on some other board.
> + * This is the default value suitable for the 100MHz crystal frequency
> + * used by DA850 EVM board, which doesn't use DT.

As I said in a reply on another patch, it seems like it would be better 
to use a clock that represents the crystal and use clk_get_rate() and 
calculate the multiplier from that.

For example, we have done something like this in 
usb20_phy_clk_set_parent() in arch/arm/mach-davinci/usb-da8xx.c.

>   */
> -#define DA850_SATA_CLK_MULTIPLIER	7
> +#define DA850_SATA_CLK_MULTIPLIER_DEFAULT	15
>
>  static void da850_sata_init(struct device *dev, void __iomem *pwrdn_reg,
> -			    void __iomem *ahci_base)
> +			    void __iomem *ahci_base,
> +			    const struct da850_sata_mpy_mapping *mpy)
>  {
>  	unsigned int val;
>
> @@ -47,7 +100,7 @@ static void da850_sata_init(struct device *dev, void __iomem *pwrdn_reg,
>  	val &= ~BIT(0);
>  	writel(val, pwrdn_reg);
>
> -	val = SATA_PHY_MPY(DA850_SATA_CLK_MULTIPLIER + 1) | SATA_PHY_LOS(1) |
> +	val = SATA_PHY_MPY(mpy->regval) | SATA_PHY_LOS(1) |
>  	      SATA_PHY_RXCDR(4) | SATA_PHY_RXEQ(1) | SATA_PHY_TXSWING(3) |
>  	      SATA_PHY_ENPLL(1);
>
> @@ -87,10 +140,12 @@ static struct scsi_host_template ahci_platform_sht = {
>
>  static int ahci_da850_probe(struct platform_device *pdev)
>  {
> +	const struct da850_sata_mpy_mapping *mpy;
>  	struct device *dev = &pdev->dev;
>  	struct ahci_host_priv *hpriv;
> -	struct resource *res;
> +	unsigned int multiplier;
>  	void __iomem *pwrdn_reg;
> +	struct resource *res;
>  	int rc;
>
>  	hpriv = ahci_platform_get_resources(pdev);
> @@ -109,7 +164,19 @@ static int ahci_da850_probe(struct platform_device *pdev)
>  	if (!pwrdn_reg)
>  		goto disable_resources;
>
> -	da850_sata_init(dev, pwrdn_reg, hpriv->mmio);
> +	rc = of_property_read_u32(dev->of_node,
> +				  "da850,clk_multiplier", &multiplier);
> +	if (rc)
> +		multiplier = DA850_SATA_CLK_MULTIPLIER_DEFAULT;
> +
> +	mpy = da850_sata_get_mpy(multiplier);
> +	if (!mpy) {
> +		dev_err(dev, "invalid multiplier value: %u\n", multiplier);
> +		rc = -EINVAL;
> +		goto disable_resources;
> +	}
> +
> +	da850_sata_init(dev, pwrdn_reg, hpriv->mmio, mpy);
>
>  	rc = ahci_platform_init_host(pdev, hpriv, &ahci_da850_port_info,
>  				     &ahci_platform_sht);
>

Re: [PATCH 09/10] ARM: dts: da850: add the SATA node

[David Lechner]

On 01/13/2017 06:38 AM, Bartosz Golaszewski wrote:
> Add the SATA node to the da850 device tree.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  arch/arm/boot/dts/da850.dtsi | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
> index 1f6a47d..f5086b1 100644
> --- a/arch/arm/boot/dts/da850.dtsi
> +++ b/arch/arm/boot/dts/da850.dtsi
> @@ -427,6 +427,12 @@
>  			phy-names = "usb-phy";
>  			status = "disabled";
>  		};
> +		sata: ahci@0x218000 {

0x needs to be omitted.

	sata: ahci@218000 {

> +			compatible = "ti,da850-ahci";
> +			reg = <0x218000 0x2000>, <0x22c018 0x4>;
> +			interrupts = <67>;
> +			status = "disabled";
> +		};
>  		mdio: mdio@224000 {
>  			compatible = "ti,davinci_mdio";
>  			#address-cells = <1>;
>

Re: [PATCH 04/10] sata: hardreset: retry if phys link is down

[Tejun Heo]

Hello,

On Fri, Jan 13, 2017 at 01:37:58PM +0100, Bartosz Golaszewski wrote:
> The sata core driver already retries to resume the link because some
> controllers ignore writes to the SControl register.
> 
> We have a use case with the da850 SATA controller where at PLL0
> frequency of 456MHz (needed to properly service the LCD controller)
> the chip becomes unstable and the hardreset operation is ignored the
> first time 50% of times.
> 
> Retrying just the resume operation doesn't work - we need to issue
> the phy/wake reset again to make it work.
> 
> If ata_phys_link_offline() returns true in sata_link_hardreset(),
> retry a couple times before really giving up.

I think it'd be better to implement the driver specific implementation
rather than changing the behavior for everybody.

Thanks.

-- 
tejun

Re: [PATCH 06/10] sata: ahci_da850: implement a softreset quirk

[Tejun Heo]

On Fri, Jan 13, 2017 at 01:38:00PM +0100, Bartosz Golaszewski wrote:
> +static int ahci_da850_softreset(struct ata_link *link,
> +				unsigned int *class, unsigned long deadline)
> +{
> +	int pmp, ret;
> +
> +	pmp = sata_srst_pmp(link);
> +
> +	ret = ahci_do_softreset(link, class, pmp, deadline, ahci_check_ready);
> +	if (pmp && ret == -EBUSY)
> +		return ahci_do_softreset(link, class, 0,
> +					 deadline, ahci_check_ready);
> +
> +	return ret;
> +}

Please add some comments explaining what's going on.

Thanks.

-- 
tejun

Re: [PATCH 09/10] ARM: dts: da850: add the SATA node

[Bartosz Golaszewski]

2017-01-13 20:36 GMT+01:00 David Lechner <david@lechnology.com>:
> On 01/13/2017 06:38 AM, Bartosz Golaszewski wrote:
>>
>> Add the SATA node to the da850 device tree.
>>
>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>> ---
>>  arch/arm/boot/dts/da850.dtsi | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>> index 1f6a47d..f5086b1 100644
>> --- a/arch/arm/boot/dts/da850.dtsi
>> +++ b/arch/arm/boot/dts/da850.dtsi
>> @@ -427,6 +427,12 @@
>>                         phy-names = "usb-phy";
>>                         status = "disabled";
>>                 };
>> +               sata: ahci@0x218000 {
>
>
> 0x needs to be omitted.
>
>         sata: ahci@218000 {
>

Will fix in v2.

Thanks,
Bartosz Golaszewski

Re: [PATCH 03/10] devicetree: bindings: add bindings for ahci-da850

[Bartosz Golaszewski]

2017-01-13 20:25 GMT+01:00 David Lechner <david@lechnology.com>:
> On 01/13/2017 06:37 AM, Bartosz Golaszewski wrote:
>>
>> Add DT bindings for the TI DA850 AHCI SATA controller.
>>
>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>> ---
>>  .../devicetree/bindings/ata/ahci-da850.txt          | 21
>> +++++++++++++++++++++
>>  1 file changed, 21 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/ata/ahci-da850.txt
>>
>> diff --git a/Documentation/devicetree/bindings/ata/ahci-da850.txt
>> b/Documentation/devicetree/bindings/ata/ahci-da850.txt
>> new file mode 100644
>> index 0000000..d07c241
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/ata/ahci-da850.txt
>> @@ -0,0 +1,21 @@
>> +Device tree binding for the TI DA850 AHCI SATA Controller
>> +---------------------------------------------------------
>> +
>> +Required properties:
>> +  - compatible: must be "ti,da850-ahci"
>> +  - reg: physical base addresses and sizes of the controller's register
>> areas
>> +  - interrupts: interrupt specifier (refer to the interrupt binding)
>> +
>> +Optional properties:
>> +  - clocks: clock specifier (refer to the common clock binding)
>> +  - da850,clk_multiplier: the multiplier for the reference clock needed
>> +                          for 1.5GHz PLL output
>
>
> A clock multiplier property seems redundant if you are specifying a clock.
> It should be possible to get the rate from the clock to determine which
> multiplier is needed.
>

I probably should have named it differently. This is not a multiplier
of a clock derived from PLL0 or PLL1. Instead it's a value set by
writing to the Port PHY Control Register (MPY bits) of the SATA
controller that configures the multiplier for the external low-jitter
clock. On the lcdk the signals (REFCLKP, REFCLKN) are provided by
CDCM61001 (SATA OSCILLATOR component on the schematics).

I'll find a better name and comment the property accordingly.

FYI: the da850 platform does not use the common clock framework, so I
don't specify the clock property on the sata node in the device tree.
Instead I add the clock lookup entry in patch [01/10]. This is
transparent for AHCI which can get the clock as usual by calling
clk_get() in ahci_platform_get_resources().

Thanks,
Bartosz Golaszewski

Re: [PATCH 06/10] sata: ahci_da850: implement a softreset quirk

[Bartosz Golaszewski]

2017-01-16 0:12 GMT+01:00 Tejun Heo <tj@kernel.org>:
> On Fri, Jan 13, 2017 at 01:38:00PM +0100, Bartosz Golaszewski wrote:
>> +static int ahci_da850_softreset(struct ata_link *link,
>> +                             unsigned int *class, unsigned long deadline)
>> +{
>> +     int pmp, ret;
>> +
>> +     pmp = sata_srst_pmp(link);
>> +
>> +     ret = ahci_do_softreset(link, class, pmp, deadline, ahci_check_ready);
>> +     if (pmp && ret == -EBUSY)
>> +             return ahci_do_softreset(link, class, 0,
>> +                                      deadline, ahci_check_ready);
>> +
>> +     return ret;
>> +}
>
> Please add some comments explaining what's going on.

Sure, I'll add some explanation in v2.

Thanks,
Bartosz Golaszewski

Re: [PATCH 04/10] sata: hardreset: retry if phys link is down

[Bartosz Golaszewski]

2017-01-16 0:10 GMT+01:00 Tejun Heo <tj@kernel.org>:
> Hello,
>
> On Fri, Jan 13, 2017 at 01:37:58PM +0100, Bartosz Golaszewski wrote:
>> The sata core driver already retries to resume the link because some
>> controllers ignore writes to the SControl register.
>>
>> We have a use case with the da850 SATA controller where at PLL0
>> frequency of 456MHz (needed to properly service the LCD controller)
>> the chip becomes unstable and the hardreset operation is ignored the
>> first time 50% of times.
>>
>> Retrying just the resume operation doesn't work - we need to issue
>> the phy/wake reset again to make it work.
>>
>> If ata_phys_link_offline() returns true in sata_link_hardreset(),
>> retry a couple times before really giving up.
>
> I think it'd be better to implement the driver specific implementation
> rather than changing the behavior for everybody.
>
> Thanks.
>

For v2 I created a new ahci-locally exported function:
ahci_do_hardreset() that allows to retrieve the online state of the
link and used it in the da850-specific hardreset implementation.

Hope that'll be good.

Thanks,
Bartosz Golaszewski

Re: [PATCH 03/10] devicetree: bindings: add bindings for ahci-da850

[Sekhar Nori]

On Monday 16 January 2017 03:43 PM, Bartosz Golaszewski wrote:
> 2017-01-13 20:25 GMT+01:00 David Lechner <david@lechnology.com>:
>> On 01/13/2017 06:37 AM, Bartosz Golaszewski wrote:
>>>
>>> Add DT bindings for the TI DA850 AHCI SATA controller.
>>>
>>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>> ---
>>>  .../devicetree/bindings/ata/ahci-da850.txt          | 21
>>> +++++++++++++++++++++
>>>  1 file changed, 21 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/ata/ahci-da850.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/ata/ahci-da850.txt
>>> b/Documentation/devicetree/bindings/ata/ahci-da850.txt
>>> new file mode 100644
>>> index 0000000..d07c241
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/ata/ahci-da850.txt
>>> @@ -0,0 +1,21 @@
>>> +Device tree binding for the TI DA850 AHCI SATA Controller
>>> +---------------------------------------------------------
>>> +
>>> +Required properties:
>>> +  - compatible: must be "ti,da850-ahci"
>>> +  - reg: physical base addresses and sizes of the controller's register
>>> areas
>>> +  - interrupts: interrupt specifier (refer to the interrupt binding)
>>> +
>>> +Optional properties:
>>> +  - clocks: clock specifier (refer to the common clock binding)
>>> +  - da850,clk_multiplier: the multiplier for the reference clock needed
>>> +                          for 1.5GHz PLL output
>>
>>
>> A clock multiplier property seems redundant if you are specifying a clock.
>> It should be possible to get the rate from the clock to determine which
>> multiplier is needed.
>>
> 
> I probably should have named it differently. This is not a multiplier
> of a clock derived from PLL0 or PLL1. Instead it's a value set by
> writing to the Port PHY Control Register (MPY bits) of the SATA
> controller that configures the multiplier for the external low-jitter
> clock. On the lcdk the signals (REFCLKP, REFCLKN) are provided by
> CDCM61001 (SATA OSCILLATOR component on the schematics).
> 
> I'll find a better name and comment the property accordingly.
> 
> FYI: the da850 platform does not use the common clock framework, so I
> don't specify the clock property on the sata node in the device tree.
> Instead I add the clock lookup entry in patch [01/10]. This is
> transparent for AHCI which can get the clock as usual by calling
> clk_get() in ahci_platform_get_resources().

I think David's point is that the SATA_REFCLK needs to be modeled as a
actual clock input to the IP. You should be able to get the rate using
clk_get_rate() and make the MPY bits calculation depending on the
incoming rate.

You should be able to model the clock even when not using common clock
framework.

DA850 AHCI does not use a con_id at the moment (it assumes a single
clock), and that needs to change.

Thanks,
Sekhar

Re: [PATCH 03/10] devicetree: bindings: add bindings for ahci-da850

[Bartosz Golaszewski]

2017-01-16 13:45 GMT+01:00 Sekhar Nori <nsekhar@ti.com>:
> On Monday 16 January 2017 03:43 PM, Bartosz Golaszewski wrote:
>> 2017-01-13 20:25 GMT+01:00 David Lechner <david@lechnology.com>:
>>>
>>> A clock multiplier property seems redundant if you are specifying a clock.
>>> It should be possible to get the rate from the clock to determine which
>>> multiplier is needed.
>>>
>>
>> I probably should have named it differently. This is not a multiplier
>> of a clock derived from PLL0 or PLL1. Instead it's a value set by
>> writing to the Port PHY Control Register (MPY bits) of the SATA
>> controller that configures the multiplier for the external low-jitter
>> clock. On the lcdk the signals (REFCLKP, REFCLKN) are provided by
>> CDCM61001 (SATA OSCILLATOR component on the schematics).
>>
>> I'll find a better name and comment the property accordingly.
>>
>> FYI: the da850 platform does not use the common clock framework, so I
>> don't specify the clock property on the sata node in the device tree.
>> Instead I add the clock lookup entry in patch [01/10]. This is
>> transparent for AHCI which can get the clock as usual by calling
>> clk_get() in ahci_platform_get_resources().
>
> I think David's point is that the SATA_REFCLK needs to be modeled as a
> actual clock input to the IP. You should be able to get the rate using
> clk_get_rate() and make the MPY bits calculation depending on the
> incoming rate.
>
> You should be able to model the clock even when not using common clock
> framework.
>
> DA850 AHCI does not use a con_id at the moment (it assumes a single
> clock), and that needs to change.
>

It's true that once davinci gets ported (is this planned?) to using
the common clock framework, we could just create a fixed-clock node in
da850-lcdk for the SATA oscillator, so the new property is redundant.

What I don't get is how should I model a clock that is not
configurable and is board-specific? Is hard-coding the relevant rate
in da850.c with a huge FIXME the right way?

Thanks,
Bartosz Golaszewski

Re: [PATCH 03/10] devicetree: bindings: add bindings for ahci-da850

[David Lechner]

On 01/16/2017 08:30 AM, Bartosz Golaszewski wrote:
> 2017-01-16 13:45 GMT+01:00 Sekhar Nori <nsekhar@ti.com>:
>> On Monday 16 January 2017 03:43 PM, Bartosz Golaszewski wrote:
>>> 2017-01-13 20:25 GMT+01:00 David Lechner <david@lechnology.com>:
>>>>
>>>> A clock multiplier property seems redundant if you are specifying a clock.
>>>> It should be possible to get the rate from the clock to determine which
>>>> multiplier is needed.
>>>>
>>>
>>> I probably should have named it differently. This is not a multiplier
>>> of a clock derived from PLL0 or PLL1. Instead it's a value set by
>>> writing to the Port PHY Control Register (MPY bits) of the SATA
>>> controller that configures the multiplier for the external low-jitter
>>> clock. On the lcdk the signals (REFCLKP, REFCLKN) are provided by
>>> CDCM61001 (SATA OSCILLATOR component on the schematics).
>>>
>>> I'll find a better name and comment the property accordingly.
>>>
>>> FYI: the da850 platform does not use the common clock framework, so I
>>> don't specify the clock property on the sata node in the device tree.
>>> Instead I add the clock lookup entry in patch [01/10]. This is
>>> transparent for AHCI which can get the clock as usual by calling
>>> clk_get() in ahci_platform_get_resources().
>>
>> I think David's point is that the SATA_REFCLK needs to be modeled as a
>> actual clock input to the IP. You should be able to get the rate using
>> clk_get_rate() and make the MPY bits calculation depending on the
>> incoming rate.
>>
>> You should be able to model the clock even when not using common clock
>> framework.
>>
>> DA850 AHCI does not use a con_id at the moment (it assumes a single
>> clock), and that needs to change.
>>
>
> It's true that once davinci gets ported (is this planned?) to using
> the common clock framework, we could just create a fixed-clock node in
> da850-lcdk for the SATA oscillator, so the new property is redundant.
>

I have some commits[1] where I started on converting da850 to use the 
common clock framework. But, I don't know anything about other davinci 
family devices, so I don't think I could really take that to completion 
without lots of help.

[1]: https://github.com/dlech/ev3dev-kernel/commits/wip-20160509

> What I don't get is how should I model a clock that is not
> configurable and is board-specific? Is hard-coding the relevant rate
> in da850.c with a huge FIXME the right way?

In arch/arm/mach-davinci/usb-da8xx.c, there is a "usb_refclkin" that is 
very similar to the situation with the sata refclk. You could do 
something like this to register the clock...

---

diff --git a/arch/arm/mach-davinci/devices-da8xx.c 
b/arch/arm/mach-davinci/devices-da8xx.c
index c2457b3..790efce9 100644
--- a/arch/arm/mach-davinci/devices-da8xx.c
+++ b/arch/arm/mach-davinci/devices-da8xx.c
@@ -1023,6 +1023,34 @@ int __init da8xx_register_spi_bus(int instance, 
unsigned num_chipselect)
  }

  #ifdef CONFIG_ARCH_DAVINCI_DA850
+
+static struct clk sata_refclkin = {
+       .name           = "sata_refclkin",
+       .set_rate       = davinci_simple_set_rate,
+};
+
+static struct clk_lookup sata_refclkin_lookup =
+       CLK(NULL, "sata_refclkin", &sata_refclkin);
+
+/**
+ * da8xx_register_sata_refclkin - register SATA_REFCLKIN clock
+ *
+ * @rate: The clock rate in Hz
+ */
+int __init da850_register_sata_refclkin(int rate)
+{
+       int ret;
+
+       sata_refclkin.rate = rate;
+       ret = clk_register(&sata_refclkin);
+       if (ret)
+               return ret;
+
+       clkdev_add(&sata_refclkin_lookup);
+
+       return 0;
+}
+
  static struct resource da850_sata_resources[] = {
         {
                 .start  = DA850_SATA_BASE,
@@ -1055,8 +1083,11 @@ static struct platform_device da850_sata_device = {

  int __init da850_register_sata(unsigned long refclkpn)
  {
-       /* please see comment in drivers/ata/ahci_da850.c */
-       BUG_ON(refclkpn != 100 * 1000 * 1000);
+       int err;
+
+       err = da850_register_sata_refclkin(refclkpn);
+       if (err)
+               return err;

         return platform_device_register(&da850_sata_device);
  }

---

Then to get things working from device tree, add this...

---

diff --git a/arch/arm/mach-davinci/da8xx-dt.c 
b/arch/arm/mach-davinci/da8xx-dt.c
index d2be194..b54bdd6 100644
--- a/arch/arm/mach-davinci/da8xx-dt.c
+++ b/arch/arm/mach-davinci/da8xx-dt.c
@@ -60,6 +60,14 @@ static void __init da850_init_machine(void)
                 pr_warn("%s: registering USB 1.1 PHY clock failed: %d",
                         __func__, ret);

+       if (of_machine_is_compatible("ti,da850-evm") ||
+           of_machine_is_compatible("ti,da850-lcdk")) {
+               ret = da850_register_sata_refclkin(100000000);
+               if (ret)
+                       pr_warn("%s: registering SATA_REFCLK clock 
failed: %d",
+                               __func__, ret);
+       }
+
         of_platform_default_populate(NULL, da850_auxdata_lookup, NULL);
         davinci_pm_init();
         pdata_quirks_init();

---

Re: [PATCH 03/10] devicetree: bindings: add bindings for ahci-da850

[Sekhar Nori]

On Tuesday 17 January 2017 12:17 AM, David Lechner wrote:
> On 01/16/2017 08:30 AM, Bartosz Golaszewski wrote:
>> 2017-01-16 13:45 GMT+01:00 Sekhar Nori <nsekhar@ti.com>:
>>> On Monday 16 January 2017 03:43 PM, Bartosz Golaszewski wrote:
>>>> 2017-01-13 20:25 GMT+01:00 David Lechner <david@lechnology.com>:
>>>>>
>>>>> A clock multiplier property seems redundant if you are specifying a
>>>>> clock.
>>>>> It should be possible to get the rate from the clock to determine
>>>>> which
>>>>> multiplier is needed.
>>>>>
>>>>
>>>> I probably should have named it differently. This is not a multiplier
>>>> of a clock derived from PLL0 or PLL1. Instead it's a value set by
>>>> writing to the Port PHY Control Register (MPY bits) of the SATA
>>>> controller that configures the multiplier for the external low-jitter
>>>> clock. On the lcdk the signals (REFCLKP, REFCLKN) are provided by
>>>> CDCM61001 (SATA OSCILLATOR component on the schematics).
>>>>
>>>> I'll find a better name and comment the property accordingly.
>>>>
>>>> FYI: the da850 platform does not use the common clock framework, so I
>>>> don't specify the clock property on the sata node in the device tree.
>>>> Instead I add the clock lookup entry in patch [01/10]. This is
>>>> transparent for AHCI which can get the clock as usual by calling
>>>> clk_get() in ahci_platform_get_resources().
>>>
>>> I think David's point is that the SATA_REFCLK needs to be modeled as a
>>> actual clock input to the IP. You should be able to get the rate using
>>> clk_get_rate() and make the MPY bits calculation depending on the
>>> incoming rate.
>>>
>>> You should be able to model the clock even when not using common clock
>>> framework.
>>>
>>> DA850 AHCI does not use a con_id at the moment (it assumes a single
>>> clock), and that needs to change.
>>>
>>
>> It's true that once davinci gets ported (is this planned?) to using
>> the common clock framework, we could just create a fixed-clock node in
>> da850-lcdk for the SATA oscillator, so the new property is redundant.
>>
> 
> I have some commits[1] where I started on converting da850 to use the
> common clock framework. But, I don't know anything about other davinci
> family devices, so I don't think I could really take that to completion
> without lots of help.

I can help with testing, reviewing and filling in any missing
information. But I wont have time to write the code itself.

> 
> [1]: https://github.com/dlech/ev3dev-kernel/commits/wip-20160509

I see that you have made a copy of the keystone PSC driver. I think you
will need pretty strong reasons to not use the same driver with some
customization for DaVinci.

>> What I don't get is how should I model a clock that is not
>> configurable and is board-specific? Is hard-coding the relevant rate
>> in da850.c with a huge FIXME the right way?
> 
> In arch/arm/mach-davinci/usb-da8xx.c, there is a "usb_refclkin" that is
> very similar to the situation with the sata refclk. You could do
> something like this to register the clock...
> 
> ---
> 
> diff --git a/arch/arm/mach-davinci/devices-da8xx.c
> b/arch/arm/mach-davinci/devices-da8xx.c
> index c2457b3..790efce9 100644
> --- a/arch/arm/mach-davinci/devices-da8xx.c
> +++ b/arch/arm/mach-davinci/devices-da8xx.c
> @@ -1023,6 +1023,34 @@ int __init da8xx_register_spi_bus(int instance,
> unsigned num_chipselect)
>  }
> 
>  #ifdef CONFIG_ARCH_DAVINCI_DA850
> +
> +static struct clk sata_refclkin = {
> +       .name           = "sata_refclkin",
> +       .set_rate       = davinci_simple_set_rate,
> +};
> +
> +static struct clk_lookup sata_refclkin_lookup =
> +       CLK(NULL, "sata_refclkin", &sata_refclkin);
> +
> +/**
> + * da8xx_register_sata_refclkin - register SATA_REFCLKIN clock
> + *
> + * @rate: The clock rate in Hz
> + */
> +int __init da850_register_sata_refclkin(int rate)
> +{
> +       int ret;
> +
> +       sata_refclkin.rate = rate;
> +       ret = clk_register(&sata_refclkin);
> +       if (ret)
> +               return ret;
> +
> +       clkdev_add(&sata_refclkin_lookup);
> +
> +       return 0;
> +}
> +
>  static struct resource da850_sata_resources[] = {
>         {
>                 .start  = DA850_SATA_BASE,
> @@ -1055,8 +1083,11 @@ static struct platform_device da850_sata_device = {
> 
>  int __init da850_register_sata(unsigned long refclkpn)
>  {
> -       /* please see comment in drivers/ata/ahci_da850.c */
> -       BUG_ON(refclkpn != 100 * 1000 * 1000);
> +       int err;
> +
> +       err = da850_register_sata_refclkin(refclkpn);
> +       if (err)
> +               return err;
> 
>         return platform_device_register(&da850_sata_device);
>  }
> 
> ---
> 
> Then to get things working from device tree, add this...
> 
> ---
> 
> diff --git a/arch/arm/mach-davinci/da8xx-dt.c
> b/arch/arm/mach-davinci/da8xx-dt.c
> index d2be194..b54bdd6 100644
> --- a/arch/arm/mach-davinci/da8xx-dt.c
> +++ b/arch/arm/mach-davinci/da8xx-dt.c
> @@ -60,6 +60,14 @@ static void __init da850_init_machine(void)
>                 pr_warn("%s: registering USB 1.1 PHY clock failed: %d",
>                         __func__, ret);
> 
> +       if (of_machine_is_compatible("ti,da850-evm") ||
> +           of_machine_is_compatible("ti,da850-lcdk")) {
> +               ret = da850_register_sata_refclkin(100000000);
> +               if (ret)
> +                       pr_warn("%s: registering SATA_REFCLK clock
> failed: %d",
> +                               __func__, ret);
> +       }
> +
>         of_platform_default_populate(NULL, da850_auxdata_lookup, NULL);
>         davinci_pm_init();
>         pdata_quirks_init();

This approach is fine.

Thanks,
Sekhar

Re: [PATCH 00/10] ARM: da850-lcdk: add SATA support

[Bartosz Golaszewski]

2017-01-13 13:37 GMT+01:00 Bartosz Golaszewski <bgolaszewski@baylibre.com>:
> This series contains all the changes necessary to make SATA work on
> the da850-lcdk board.
>
> The first patch adds a clock lookup entry required for the ahci core
> to retrieve a functional clock.
>
> The second enables relevant config options for all davinci boards.
>
> The third adds device tree bindings for the ahci_da850 driver.
>
> The fourth adds a workaround for a SATA controller instability we
> detected after increasing the PLL0 frequency for proper LCD
> controller support.
>
> Patches 5 through 7 extend the ahci_da850 driver - add DT support,
> un-hardcode the clock multiplier value and add a workaround for
> a quirk present on the da850 SATA controller.
>
> Patches 8-10 add the device tree changes required to probe the driver.
>
> I'm posting the series as a whole to give all reviewers the full
> picture and visibility of the changes required, if needed I can resend
> the patches separately.
>

Superseded by v2.

davinci common clock framework (was Re: [PATCH 03/10] devicetree: bindings: add bindings for ahci-da850)

[David Lechner]

On 01/17/2017 06:00 AM, Sekhar Nori wrote:
> On Tuesday 17 January 2017 12:17 AM, David Lechner wrote:
>> On 01/16/2017 08:30 AM, Bartosz Golaszewski wrote:
>>> 2017-01-16 13:45 GMT+01:00 Sekhar Nori <nsekhar@ti.com>:
>>>> On Monday 16 January 2017 03:43 PM, Bartosz Golaszewski wrote:
>>>
>>> It's true that once davinci gets ported (is this planned?) to using
>>> the common clock framework, we could just create a fixed-clock node in
>>> da850-lcdk for the SATA oscillator, so the new property is redundant.
>>>
>>
>> I have some commits[1] where I started on converting da850 to use the
>> common clock framework. But, I don't know anything about other davinci
>> family devices, so I don't think I could really take that to completion
>> without lots of help.
>
> I can help with testing, reviewing and filling in any missing
> information. But I wont have time to write the code itself.
>
>>
>> [1]: https://github.com/dlech/ev3dev-kernel/commits/wip-20160509
>
> I see that you have made a copy of the keystone PSC driver. I think you
> will need pretty strong reasons to not use the same driver with some
> customization for DaVinci.
>

It has been a while since I looked at this, but as I recall, the device 
tree bindings for keystone are horrible and make no sense. So, I made 
new bindings that make more sense. But since we can't break backwards 
compatibility in device tree, I made a new driver rather than having the 
mess of supporting two very different bindings in one driver. I don't 
know if that is a strong enough reason, but that is why I did it. :-)