Date:   Mon, 16 Jan 2017 12:47:59 -0600
From:   David Lechner <david@...hnology.com>
To:     Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Sekhar Nori <nsekhar@...com>
Cc:     Kevin Hilman <khilman@...nel.org>,
        Patrick Titiano <ptitiano@...libre.com>,
        Michael Turquette <mturquette@...libre.com>,
        Tejun Heo <tj@...nel.org>, Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Russell King <linux@...linux.org.uk>,
        linux-ide@...r.kernel.org,
        linux-devicetree <devicetree@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        arm-soc <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 03/10] devicetree: bindings: add bindings for ahci-da850

On 01/16/2017 08:30 AM, Bartosz Golaszewski wrote:
> 2017-01-16 13:45 GMT+01:00 Sekhar Nori <nsekhar@...com>:
>> On Monday 16 January 2017 03:43 PM, Bartosz Golaszewski wrote:
>>> 2017-01-13 20:25 GMT+01:00 David Lechner <david@...hnology.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();

---

Powered by blists - more mailing lists