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@xxxxxx>:
>>> On Monday 16 January 2017 03:43 PM, Bartosz Golaszewski wrote:
>>>> 2017-01-13 20:25 GMT+01:00 David Lechner <david@xxxxxxxxxxxxxx>:
>>>>>
>>>>> 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