[PATCH 00/14] staging: clocking-wizard: Implement many TODOs

[PATCH 00/14] staging: clocking-wizard: Implement many TODOs

[James Kelly]

This series of patches attempts to implement many of the outstanding TODOs
for the Xilinx Clocking Wizard IP driver that has been languishing in the
staging tree for some time.  I had a need for this driver so I thought it
appropriate to give it some love.  Hopefully others will find these patches
useful.

Highlights include:
- Support for fractional ratios when available in hardware.
- Support for clk_round_rate and clk_set_rate kernel APIs.
- Automatic set rate of internal clocks when rate of output clock is set.
- Automatic set rate of input clock and internal clocks when rate of 
  output clock is set.

A CCF clock provider has been implemented within the driver to support
the new functionality as it was not possible to do this with the existing
clock providers.  There is also code to handle a limitation of Clocking
Wizard IP which prevents changes to the clock ratios if the PLL is not
locked.  Great care has to be taken to ensure the PLL will always lock as
there is no way for the driver to recover if the PLL fails to lock under
transient conditions that may drive the PLL VCO frequency out of range.

The patches were built on the current staging-next branch.

The patches also work with the xlnx_rebase_v4.14 branch of the Xilinx
linux tree at https://github.com/Xilinx/linux-xlnx.git - this branch is
used by the current release (2018.1) of the Xilinx development tools.
Patches corresponding to the following staging tree commits are required as
prerequisites before applying this patch series to xlnx_rebase_v4.14:

667063acb81931e2f8fd0cb91df9fcccad131d9a 
regmap: add iopoll-like polling macro for regmap_field
1dbb3344d9e9cd6e72da23f4058f3e6e926614b6 
staging: clocking-wizard: add SPDX identifier
09956d59bad5f5bb238efb805f0767060e624378 
staging: clocking-wizard: remove redundant license text
a08f06bb7a0743a7fc8d571899c93d882468096e 
seq_file: Introduce DEFINE_SHOW_ATTRIBUTE() helper macro

Testing has been done on a Digilent Zybo-Z7 development board.  This uses a
32-bit ARM architecture Zynq-7020 SoC. Testing used the 2018.1 release of
the Xilinx development tools which has v6.0 of the Clocking Wizard IP.

The patches are also applicable to, but are currently untested on:
- 64-bit ARM architecture (Zynq Ultrascale+ MPSoC etc.)
- Microblaze architecture (7-Series, Ultrascale, Ultrascale+ FPGAs)
Others with access to suitable hardware will need to test these platforms.

Potential users of this driver may use the Xilinx device tree generator
from https://github.com/Xilinx/device-tree-xlnx.git either directly or via
the development tools provided by Xilinx.  There are two issues with the
DTG that any potential testers of these patches should be aware of:
- The DTG generates a negative value for the device-tree speed-grade
  property.  The 7th patch has a hack to handle this quirk until Xilinx
  fix their DTG.
- The 2018.1 DTG changed the device-tree clock-output-names property so
  it no longer provides any information about how the Clocking Wizard IP
  is actually configured.  These patches will still work with the new
  2018.1 DTG behaviour but the names of the output clocks will no longer
  match those used in the Clocking Wizard IP customization, and the
  maximum number of clocks will always be created even if not used or
  FPGA.  A warning will also be issued stating that there are too many
  clock output names.  Further details can be found in the Xilinx Community
  forums at https://bit.ly/2jmFIRf.

The original driver author appears to have left Xilinx so I have not
included them as an addressee for these patches and instead directed
them to another Xilinx Linux maintainer.

James Kelly (14):
  staging: clocking-wizard: Add principles of operation
  staging: clocking-wizard: Reverse order of internal clocks
  staging: clocking-wizard: Split probe function
  staging: clocking-wizard: Cosmetic cleanups
  staging: clocking-wizard: Implement CCF clock provider
  staging: clocking-wizard: Swap CCF clock providers
  staging: clocking-wizard: Add hardware constaints
  staging: clocking-wizard: Support fractional ratios
  staging: clocking-wizard: Provide more information in debugfs
  staging: clocking-wizard: Support clk_round_rate
  staging: clocking-wizard: Support clk_set_rate
  staging: clocking-wizard: Automatically set internal clock rates
  staging: clocking-wizard: Automatically set input clock rate
  staging: clocking-wizard: Add debugfs entries to facilitate testing.

 drivers/staging/clocking-wizard/TODO               |    7 +-
 .../clocking-wizard/clk-xlnx-clock-wizard.c        | 1406 +++++++++++++++++---
 drivers/staging/clocking-wizard/dt-binding.txt     |   19 +-
 3 files changed, 1234 insertions(+), 198 deletions(-)

-- 
2.15.1 (Apple Git-101)

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 01/14] staging: clocking-wizard: Add principles of operation

[James Kelly]

Add a description for how the Xilinx Clocking Wizard IP works to guide
subsequent patches.

Signed-off-by: James Kelly <jamespeterkelly@gmail.com>
---
 .../clocking-wizard/clk-xlnx-clock-wizard.c        | 52 ++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
index cae7e6e695b0..babbed42f96d 100644
--- a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
+++ b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
@@ -5,6 +5,58 @@
  *  Copyright (C) 2013 - 2014 Xilinx
  *
  *  Sören Brinkmann <soren.brinkmann@xilinx.com>
+ *
+ * Principles of Operation:
+ *
+ * The Xilinx clocking wizard IP implements a clock complex that can be
+ * modelled as a collection of dividers and a PLL multiplier arranged in
+ * the following configuration:
+ *
+ *                         +-------------------------------> clk_fbout
+ *                         |
+ *          fin  +-----+   |    +-----+  vco   +-----+
+ * clk_in1 ----->| DIV |---+--->| PLL |---+--->| DIV |-----> clk_out1
+ *               +-----+  pfd   +-----+   |    +-----+
+ *                                        |
+ *                                        |    +-----+
+ *                                        +--->| DIV |-----> clk_out2
+ *                                        |    +-----+
+ *                                        |
+ *                                        |      ...
+ *                                        |    +-----+
+ *                                        +--->| DIV |-----> clk_outn
+ *                                             +-----+
+ *
+ * Each divider and the PLL multiplier correspond to a distinct common
+ * clock framework struct clk.
+ *
+ * The number of clock outputs depends the clock primitive type (MMCM or PLL)
+ * and FPGA family and can range from 2 to 7, not including clk_fbout.
+ * Xilinx documentation is inconsistent in the numbering of these outputs.
+ * The clocking wizard uses 1 thru n whereas the clocking primitives wrapped
+ * by the wizard use 0 through n-1.
+ *
+ * This driver publishes the n output clocks in the device tree using addresses
+ * 0 through n-1.  The remaining two clocks (DIV and PLL) are not published in
+ * the device tree but can be obtained using calls to clk_get_parent on one
+ * of the output clocks.
+ *
+ * There are constraints on the input rate (fin), phase-frequency
+ * detector rate (pfd), the voltage controlled oscillator rate (vco)
+ * and output clock rates. These depend on FPGA family, clock primitive type
+ * and chip speed grade.
+ *
+ * The available ratios for the dividers and PLL multiplier depend on
+ * FPGA family and clock primitive type.  MMCM primitves support fractional
+ * ratios for the PLL multipler and first output divider, whereas PLL
+ * primitives do not.  Fractional ratios have a resolution of 0.125 (1/8) or 3
+ * fractional bits.
+ *
+ * Clock ratios can be dynamically changed via two different register
+ * interfaces depending on how the "Write to DRP" configuration option is set
+ * when the clocking wizard IP is customized.  This driver requires that
+ * the "Write to DRP" configuration option is disabled in customization
+ * as it currently uses the higher-level of the two register interfaces.
  */
 
 #include <linux/platform_device.h>
-- 
2.15.1 (Apple Git-101)

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 01/14] staging: clocking-wizard: Add principles of operation

[Michal Simek]

Hi,

On 7.5.2018 03:20, James Kelly wrote:
> Add a description for how the Xilinx Clocking Wizard IP works to guide
> subsequent patches.
> 
> Signed-off-by: James Kelly <jamespeterkelly@gmail.com>
> ---
>  .../clocking-wizard/clk-xlnx-clock-wizard.c        | 52 ++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
> 
> diff --git a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> index cae7e6e695b0..babbed42f96d 100644
> --- a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> +++ b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> @@ -5,6 +5,58 @@
>   *  Copyright (C) 2013 - 2014 Xilinx
>   *
>   *  Sören Brinkmann <soren.brinkmann@xilinx.com>
> + *
> + * Principles of Operation:
> + *
> + * The Xilinx clocking wizard IP implements a clock complex that can be
> + * modelled as a collection of dividers and a PLL multiplier arranged in
> + * the following configuration:
> + *
> + *                         +-------------------------------> clk_fbout
> + *                         |
> + *          fin  +-----+   |    +-----+  vco   +-----+
> + * clk_in1 ----->| DIV |---+--->| PLL |---+--->| DIV |-----> clk_out1
> + *               +-----+  pfd   +-----+   |    +-----+
> + *                                        |
> + *                                        |    +-----+
> + *                                        +--->| DIV |-----> clk_out2
> + *                                        |    +-----+
> + *                                        |
> + *                                        |      ...
> + *                                        |    +-----+
> + *                                        +--->| DIV |-----> clk_outn
> + *                                             +-----+
> + *
> + * Each divider and the PLL multiplier correspond to a distinct common
> + * clock framework struct clk.
> + *
> + * The number of clock outputs depends the clock primitive type (MMCM or PLL)
> + * and FPGA family and can range from 2 to 7, not including clk_fbout.
> + * Xilinx documentation is inconsistent in the numbering of these outputs.
> + * The clocking wizard uses 1 thru n whereas the clocking primitives wrapped
> + * by the wizard use 0 through n-1.
> + *
> + * This driver publishes the n output clocks in the device tree using addresses
> + * 0 through n-1.  The remaining two clocks (DIV and PLL) are not published in
> + * the device tree but can be obtained using calls to clk_get_parent on one
> + * of the output clocks.
> + *
> + * There are constraints on the input rate (fin), phase-frequency
> + * detector rate (pfd), the voltage controlled oscillator rate (vco)
> + * and output clock rates. These depend on FPGA family, clock primitive type
> + * and chip speed grade.
> + *
> + * The available ratios for the dividers and PLL multiplier depend on
> + * FPGA family and clock primitive type.  MMCM primitves support fractional
> + * ratios for the PLL multipler and first output divider, whereas PLL
> + * primitives do not.  Fractional ratios have a resolution of 0.125 (1/8) or 3
> + * fractional bits.
> + *
> + * Clock ratios can be dynamically changed via two different register
> + * interfaces depending on how the "Write to DRP" configuration option is set
> + * when the clocking wizard IP is customized.  This driver requires that
> + * the "Write to DRP" configuration option is disabled in customization
> + * as it currently uses the higher-level of the two register interfaces.
>   */
>  
>  #include <linux/platform_device.h>
> 

There are some double spaces in the text without any reason.


Thanks,
Michal

Re: [PATCH 01/14] staging: clocking-wizard: Add principles of operation

[James Kelly]

[-- Attachment #1: Type: text/plain, Size: 4520 bytes --]

Hi Michal,

On 11 May 2018 at 16:04, Michal Simek <michal.simek@xilinx.com> wrote:

> Hi,
>
> On 7.5.2018 03:20, James Kelly wrote:
> > Add a description for how the Xilinx Clocking Wizard IP works to guide
> > subsequent patches.
> >
> > Signed-off-by: James Kelly <jamespeterkelly@gmail.com>
> > ---
> >  .../clocking-wizard/clk-xlnx-clock-wizard.c        | 52
> ++++++++++++++++++++++
> >  1 file changed, 52 insertions(+)
> >
> > diff --git a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> > index cae7e6e695b0..babbed42f96d 100644
> > --- a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> > +++ b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> > @@ -5,6 +5,58 @@
> >   *  Copyright (C) 2013 - 2014 Xilinx
> >   *
> >   *  Sören Brinkmann <soren.brinkmann@xilinx.com>
> > + *
> > + * Principles of Operation:
> > + *
> > + * The Xilinx clocking wizard IP implements a clock complex that can be
> > + * modelled as a collection of dividers and a PLL multiplier arranged in
> > + * the following configuration:
> > + *
> > + *                         +-------------------------------> clk_fbout
> > + *                         |
> > + *          fin  +-----+   |    +-----+  vco   +-----+
> > + * clk_in1 ----->| DIV |---+--->| PLL |---+--->| DIV |-----> clk_out1
> > + *               +-----+  pfd   +-----+   |    +-----+
> > + *                                        |
> > + *                                        |    +-----+
> > + *                                        +--->| DIV |-----> clk_out2
> > + *                                        |    +-----+
> > + *                                        |
> > + *                                        |      ...
> > + *                                        |    +-----+
> > + *                                        +--->| DIV |-----> clk_outn
> > + *                                             +-----+
> > + *
> > + * Each divider and the PLL multiplier correspond to a distinct common
> > + * clock framework struct clk.
> > + *
> > + * The number of clock outputs depends the clock primitive type (MMCM
> or PLL)
> > + * and FPGA family and can range from 2 to 7, not including clk_fbout.
> > + * Xilinx documentation is inconsistent in the numbering of these
> outputs.
> > + * The clocking wizard uses 1 thru n whereas the clocking primitives
> wrapped
> > + * by the wizard use 0 through n-1.
> > + *
> > + * This driver publishes the n output clocks in the device tree using
> addresses
> > + * 0 through n-1.  The remaining two clocks (DIV and PLL) are not
> published in
> > + * the device tree but can be obtained using calls to clk_get_parent on
> one
> > + * of the output clocks.
> > + *
> > + * There are constraints on the input rate (fin), phase-frequency
> > + * detector rate (pfd), the voltage controlled oscillator rate (vco)
> > + * and output clock rates. These depend on FPGA family, clock primitive
> type
> > + * and chip speed grade.
> > + *
> > + * The available ratios for the dividers and PLL multiplier depend on
> > + * FPGA family and clock primitive type.  MMCM primitves support
> fractional
> > + * ratios for the PLL multipler and first output divider, whereas PLL
> > + * primitives do not.  Fractional ratios have a resolution of 0.125
> (1/8) or 3
> > + * fractional bits.
> > + *
> > + * Clock ratios can be dynamically changed via two different register
> > + * interfaces depending on how the "Write to DRP" configuration option
> is set
> > + * when the clocking wizard IP is customized.  This driver requires that
> > + * the "Write to DRP" configuration option is disabled in customization
> > + * as it currently uses the higher-level of the two register interfaces.
> >   */
> >
> >  #include <linux/platform_device.h>
> >
>
> There are some double spaces in the text without any reason.
>
>
> Thanks,
> Michal
>
>
Thanks for the feedback.

Actually they are there for a reason ... back when I learned to type on
typewriters
it was normal to use double spaces between sentences.  I sometimes fall
back into
that habit when using monospace fonts because I am used to that look.

However, I'm happy to change to the modern convention as it would appear the
the world has moved on - at least according to Wikipedia:
https://en.wikipedia.org/wiki/Sentence_spacing.

Will be fixed in version 2.

James

[-- Attachment #2: Type: text/html, Size: 6181 bytes --]

Re: [PATCH 01/14] staging: clocking-wizard: Add principles of operation

[Michal Simek]

Hi James,

On 11.5.2018 09:31, James Kelly wrote:
> Hi Michal,
> 
> On 11 May 2018 at 16:04, Michal Simek <michal.simek@xilinx.com> wrote:
> 
>> Hi,
>>
>> On 7.5.2018 03:20, James Kelly wrote:
>>> Add a description for how the Xilinx Clocking Wizard IP works to guide
>>> subsequent patches.
>>>
>>> Signed-off-by: James Kelly <jamespeterkelly@gmail.com>
>>> ---
>>>  .../clocking-wizard/clk-xlnx-clock-wizard.c        | 52
>> ++++++++++++++++++++++
>>>  1 file changed, 52 insertions(+)
>>>
>>> diff --git a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
>> b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
>>> index cae7e6e695b0..babbed42f96d 100644
>>> --- a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
>>> +++ b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
>>> @@ -5,6 +5,58 @@
>>>   *  Copyright (C) 2013 - 2014 Xilinx
>>>   *
>>>   *  Sören Brinkmann <soren.brinkmann@xilinx.com>
>>> + *
>>> + * Principles of Operation:
>>> + *
>>> + * The Xilinx clocking wizard IP implements a clock complex that can be
>>> + * modelled as a collection of dividers and a PLL multiplier arranged in
>>> + * the following configuration:
>>> + *
>>> + *                         +-------------------------------> clk_fbout
>>> + *                         |
>>> + *          fin  +-----+   |    +-----+  vco   +-----+
>>> + * clk_in1 ----->| DIV |---+--->| PLL |---+--->| DIV |-----> clk_out1
>>> + *               +-----+  pfd   +-----+   |    +-----+
>>> + *                                        |
>>> + *                                        |    +-----+
>>> + *                                        +--->| DIV |-----> clk_out2
>>> + *                                        |    +-----+
>>> + *                                        |
>>> + *                                        |      ...
>>> + *                                        |    +-----+
>>> + *                                        +--->| DIV |-----> clk_outn
>>> + *                                             +-----+
>>> + *
>>> + * Each divider and the PLL multiplier correspond to a distinct common
>>> + * clock framework struct clk.
>>> + *
>>> + * The number of clock outputs depends the clock primitive type (MMCM
>> or PLL)
>>> + * and FPGA family and can range from 2 to 7, not including clk_fbout.
>>> + * Xilinx documentation is inconsistent in the numbering of these
>> outputs.
>>> + * The clocking wizard uses 1 thru n whereas the clocking primitives
>> wrapped
>>> + * by the wizard use 0 through n-1.
>>> + *
>>> + * This driver publishes the n output clocks in the device tree using
>> addresses
>>> + * 0 through n-1.  The remaining two clocks (DIV and PLL) are not
>> published in
>>> + * the device tree but can be obtained using calls to clk_get_parent on
>> one
>>> + * of the output clocks.
>>> + *
>>> + * There are constraints on the input rate (fin), phase-frequency
>>> + * detector rate (pfd), the voltage controlled oscillator rate (vco)
>>> + * and output clock rates. These depend on FPGA family, clock primitive
>> type
>>> + * and chip speed grade.
>>> + *
>>> + * The available ratios for the dividers and PLL multiplier depend on
>>> + * FPGA family and clock primitive type.  MMCM primitves support
>> fractional
>>> + * ratios for the PLL multipler and first output divider, whereas PLL
>>> + * primitives do not.  Fractional ratios have a resolution of 0.125
>> (1/8) or 3
>>> + * fractional bits.
>>> + *
>>> + * Clock ratios can be dynamically changed via two different register
>>> + * interfaces depending on how the "Write to DRP" configuration option
>> is set
>>> + * when the clocking wizard IP is customized.  This driver requires that
>>> + * the "Write to DRP" configuration option is disabled in customization
>>> + * as it currently uses the higher-level of the two register interfaces.
>>>   */
>>>
>>>  #include <linux/platform_device.h>
>>>
>>
>> There are some double spaces in the text without any reason.
>>
>>
>> Thanks,
>> Michal
>>
>>
> Thanks for the feedback.
> 
> Actually they are there for a reason ... back when I learned to type on
> typewriters
> it was normal to use double spaces between sentences.  I sometimes fall
> back into
> that habit when using monospace fonts because I am used to that look.
> 
> However, I'm happy to change to the modern convention as it would appear the
> the world has moved on - at least according to Wikipedia:
> https://en.wikipedia.org/wiki/Sentence_spacing.
> 
> Will be fixed in version 2.

I am not a person to judge what's the right style because I am not
native speaker. But I am parsing all double spaces and spaces
before/after tabs because this is quite common issue that people have
incorrect editor setting.

Thanks,
Michal

Re: [PATCH 01/14] staging: clocking-wizard: Add principles of operation

[Dan Carpenter]

On Fri, May 11, 2018 at 08:04:01AM +0200, Michal Simek wrote:
> 
> There are some double spaces in the text without any reason.
> 

Are you talking about two spaces after a period??  Kernel style guide
has no official position on this.  It's up to the author.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 02/14] staging: clocking-wizard: Reverse order of internal clocks

[James Kelly]

The order of the internal clocks does not match how the hardware
is arranged.  We need to fix this before we can add new function.

Swap the order of the internal multiplier and divider clocks so that the
divider is the parent of the multiplier.

Signed-off-by: James Kelly <jamespeterkelly@gmail.com>
---
 .../clocking-wizard/clk-xlnx-clock-wizard.c        | 37 +++++++++++-----------
 1 file changed, 18 insertions(+), 19 deletions(-)

diff --git a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
index babbed42f96d..1d42eabdd956 100644
--- a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
+++ b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
@@ -84,8 +84,8 @@
 #define WZRD_CLKOUT_DIVIDE_MASK		(0xff << WZRD_DIVCLK_DIVIDE_SHIFT)
 
 enum clk_wzrd_int_clks {
-	wzrd_clk_mul,
 	wzrd_clk_mul_div,
+	wzrd_clk_mul,
 	wzrd_clk_int_max
 };
 
@@ -243,41 +243,40 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 	if (reg)
 		dev_warn(&pdev->dev, "fractional div/mul not supported\n");
 
-	/* register multiplier */
+	/* register div */
 	reg = (readl(clk_wzrd->base + WZRD_CLK_CFG_REG(0)) &
-		     WZRD_CLKFBOUT_MULT_MASK) >> WZRD_CLKFBOUT_MULT_SHIFT;
-	clk_name = kasprintf(GFP_KERNEL, "%s_mul", dev_name(&pdev->dev));
+			WZRD_DIVCLK_DIVIDE_MASK) >> WZRD_DIVCLK_DIVIDE_SHIFT;
+	clk_name = kasprintf(GFP_KERNEL, "%s_mul_div", dev_name(&pdev->dev));
 	if (!clk_name) {
 		ret = -ENOMEM;
 		goto err_disable_clk;
 	}
-	clk_wzrd->clks_internal[wzrd_clk_mul] = clk_register_fixed_factor(
+	clk_wzrd->clks_internal[wzrd_clk_mul_div] = clk_register_fixed_factor(
 			&pdev->dev, clk_name,
 			__clk_get_name(clk_wzrd->clk_in1),
-			0, reg, 1);
+			0, 1, reg);
 	kfree(clk_name);
-	if (IS_ERR(clk_wzrd->clks_internal[wzrd_clk_mul])) {
-		dev_err(&pdev->dev, "unable to register fixed-factor clock\n");
-		ret = PTR_ERR(clk_wzrd->clks_internal[wzrd_clk_mul]);
+	if (IS_ERR(clk_wzrd->clks_internal[wzrd_clk_mul_div])) {
+		dev_err(&pdev->dev, "unable to register divider clock\n");
+		ret = PTR_ERR(clk_wzrd->clks_internal[wzrd_clk_mul_div]);
 		goto err_disable_clk;
 	}
 
-	/* register div */
+	/* register multiplier */
 	reg = (readl(clk_wzrd->base + WZRD_CLK_CFG_REG(0)) &
-			WZRD_DIVCLK_DIVIDE_MASK) >> WZRD_DIVCLK_DIVIDE_SHIFT;
-	clk_name = kasprintf(GFP_KERNEL, "%s_mul_div", dev_name(&pdev->dev));
+		     WZRD_CLKFBOUT_MULT_MASK) >> WZRD_CLKFBOUT_MULT_SHIFT;
+	clk_name = kasprintf(GFP_KERNEL, "%s_mul", dev_name(&pdev->dev));
 	if (!clk_name) {
 		ret = -ENOMEM;
 		goto err_rm_int_clk;
 	}
-
-	clk_wzrd->clks_internal[wzrd_clk_mul_div] = clk_register_fixed_factor(
+	clk_wzrd->clks_internal[wzrd_clk_mul] = clk_register_fixed_factor(
 			&pdev->dev, clk_name,
-			__clk_get_name(clk_wzrd->clks_internal[wzrd_clk_mul]),
-			0, 1, reg);
-	if (IS_ERR(clk_wzrd->clks_internal[wzrd_clk_mul_div])) {
-		dev_err(&pdev->dev, "unable to register divider clock\n");
-		ret = PTR_ERR(clk_wzrd->clks_internal[wzrd_clk_mul_div]);
+			__clk_get_name(clk_wzrd->clks_internal[wzrd_clk_mul_div]),
+			0, reg, 1);
+	if (IS_ERR(clk_wzrd->clks_internal[wzrd_clk_mul])) {
+		dev_err(&pdev->dev, "unable to register fixed-factor clock\n");
+		ret = PTR_ERR(clk_wzrd->clks_internal[wzrd_clk_mul]);
 		goto err_rm_int_clk;
 	}
 
-- 
2.15.1 (Apple Git-101)

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 03/14] staging: clocking-wizard: Split probe function

[James Kelly]

Split probe function so that we can add more code in future patches
without it becoming too big.

Signed-off-by: James Kelly <jamespeterkelly@gmail.com>
---
 .../clocking-wizard/clk-xlnx-clock-wizard.c        | 63 +++++++++++++++++-----
 1 file changed, 51 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
index 1d42eabdd956..4dec1bfc303a 100644
--- a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
+++ b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
@@ -180,14 +180,12 @@ static int __maybe_unused clk_wzrd_resume(struct device *dev)
 static SIMPLE_DEV_PM_OPS(clk_wzrd_dev_pm_ops, clk_wzrd_suspend,
 			 clk_wzrd_resume);
 
-static int clk_wzrd_probe(struct platform_device *pdev)
+static int clk_wzrd_get_device_tree_data(struct device *dev)
 {
-	int i, ret;
-	u32 reg;
+	int ret;
 	unsigned long rate;
-	const char *clk_name;
 	struct clk_wzrd *clk_wzrd;
-	struct resource *mem;
+	struct platform_device *pdev = to_platform_device(dev);
 	struct device_node *np = pdev->dev.of_node;
 
 	clk_wzrd = devm_kzalloc(&pdev->dev, sizeof(*clk_wzrd), GFP_KERNEL);
@@ -195,11 +193,6 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	platform_set_drvdata(pdev, clk_wzrd);
 
-	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	clk_wzrd->base = devm_ioremap_resource(&pdev->dev, mem);
-	if (IS_ERR(clk_wzrd->base))
-		return PTR_ERR(clk_wzrd->base);
-
 	ret = of_property_read_u32(np, "speed-grade", &clk_wzrd->speed_grade);
 	if (!ret) {
 		if (clk_wzrd->speed_grade < 1 || clk_wzrd->speed_grade > 3) {
@@ -231,10 +224,36 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 	if (rate > WZRD_ACLK_MAX_FREQ) {
 		dev_err(&pdev->dev, "s_axi_aclk frequency (%lu) too high\n",
 			rate);
-		ret = -EINVAL;
-		goto err_disable_clk;
+		clk_disable_unprepare(clk_wzrd->axi_clk);
+		return -EINVAL;
 	}
 
+	return 0;
+}
+
+static int clk_wzrd_regmap_alloc(struct device *dev)
+{
+	struct resource *mem;
+	struct clk_wzrd *clk_wzrd = dev_get_drvdata(dev);
+	struct platform_device *pdev = to_platform_device(dev);
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	clk_wzrd->base = devm_ioremap_resource(&pdev->dev, mem);
+	if (IS_ERR(clk_wzrd->base))
+		return PTR_ERR(clk_wzrd->base);
+
+	return 0;
+}
+
+static int clk_wzrd_register_ccf(struct device *dev)
+{
+	int i, ret;
+	u32 reg;
+	const char *clk_name;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct clk_wzrd *clk_wzrd = dev_get_drvdata(dev);
+	struct device_node *np = pdev->dev.of_node;
+
 	/* we don't support fractional div/mul yet */
 	reg = readl(clk_wzrd->base + WZRD_CLK_CFG_REG(0)) &
 		    WZRD_CLKFBOUT_FRAC_EN;
@@ -342,6 +361,26 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 	return ret;
 }
 
+static int clk_wzrd_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct device *dev = &pdev->dev;
+
+	ret = clk_wzrd_get_device_tree_data(dev);
+	if (ret)
+		return ret;
+
+	ret = clk_wzrd_regmap_alloc(dev);
+	if (ret)
+		return ret;
+
+	ret = clk_wzrd_register_ccf(dev);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static int clk_wzrd_remove(struct platform_device *pdev)
 {
 	int i;
-- 
2.15.1 (Apple Git-101)

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 03/14] staging: clocking-wizard: Split probe function

[Dan Carpenter]

On Mon, May 07, 2018 at 11:20:29AM +1000, James Kelly wrote:
> +static int clk_wzrd_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct device *dev = &pdev->dev;
> +
> +	ret = clk_wzrd_get_device_tree_data(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_wzrd_regmap_alloc(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_wzrd_register_ccf(dev);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

The error handling is a terrible layer violation now...  Every
allocation function should have a matching free function.  But now
instead of that if clk_wzrd_register_ccf() fails then it starts cleaning
up the clk_wzrd->axi_clk that was allocated in
clk_wzrd_get_device_tree_data().

regards,
dan carpenter


_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 03/14] staging: clocking-wizard: Split probe function

[Dan Carpenter]

On Mon, May 14, 2018 at 04:47:26PM +0300, Dan Carpenter wrote:
> On Mon, May 07, 2018 at 11:20:29AM +1000, James Kelly wrote:
> > +static int clk_wzrd_probe(struct platform_device *pdev)
> > +{
> > +	int ret;
> > +	struct device *dev = &pdev->dev;
> > +
> > +	ret = clk_wzrd_get_device_tree_data(dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = clk_wzrd_regmap_alloc(dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = clk_wzrd_register_ccf(dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> 
> The error handling is a terrible layer violation now...  Every
> allocation function should have a matching free function.  But now
> instead of that if clk_wzrd_register_ccf() fails then it starts cleaning
> up the clk_wzrd->axi_clk that was allocated in
> clk_wzrd_get_device_tree_data().
> 

Oh...  Duh to me.  It's also buggy because clk_wzrd_regmap_alloc() can
fail and then ->axi_clk isn't disabled.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 03/14] staging: clocking-wizard: Split probe function

[James Kelly]

Dan,

> On 14 May 2018, at 11:47 pm, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> On Mon, May 07, 2018 at 11:20:29AM +1000, James Kelly wrote:
>> +static int clk_wzrd_probe(struct platform_device *pdev)
>> +{
>> +	int ret;
>> +	struct device *dev = &pdev->dev;
>> +
>> +	ret = clk_wzrd_get_device_tree_data(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = clk_wzrd_regmap_alloc(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = clk_wzrd_register_ccf(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
> 
> The error handling is a terrible layer violation now...  Every
> allocation function should have a matching free function.  But now
> instead of that if clk_wzrd_register_ccf() fails then it starts cleaning
> up the clk_wzrd->axi_clk that was allocated in
> clk_wzrd_get_device_tree_data().
> 
> regards,
> dan carpenter
> 
> 

This gets cleaned up in patch 6.  There seemed little point in putting a whole lot of
code in patch 3 only to rip it out again in patch 6.  I was trying to keep the patches
simple.

I could defer breaking up the probe function until after the changes in patch 6 ?

James
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 03/14] staging: clocking-wizard: Split probe function

[Dan Carpenter]

On Tue, May 15, 2018 at 05:30:15AM +1000, James Kelly wrote:
> Dan,
> 
> > On 14 May 2018, at 11:47 pm, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > 
> > On Mon, May 07, 2018 at 11:20:29AM +1000, James Kelly wrote:
> >> +static int clk_wzrd_probe(struct platform_device *pdev)
> >> +{
> >> +	int ret;
> >> +	struct device *dev = &pdev->dev;
> >> +
> >> +	ret = clk_wzrd_get_device_tree_data(dev);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	ret = clk_wzrd_regmap_alloc(dev);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	ret = clk_wzrd_register_ccf(dev);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	return 0;
> > 
> > The error handling is a terrible layer violation now...  Every
> > allocation function should have a matching free function.  But now
> > instead of that if clk_wzrd_register_ccf() fails then it starts cleaning
> > up the clk_wzrd->axi_clk that was allocated in
> > clk_wzrd_get_device_tree_data().
> > 
> > regards,
> > dan carpenter
> > 
> > 
> 
> This gets cleaned up in patch 6.  There seemed little point in putting a whole lot of
> code in patch 3 only to rip it out again in patch 6.  I was trying to keep the patches
> simple.

I later posted that it's actually buggy so it's not allow to introduce
a bug and then fix it.

I really quarrel with the word "simple".  As a reviewer it's my job to
spot bugs being introduced.  It took me a long time to review this patch
until I spotted the bug.  It's better to not take short cuts, but just
write error handling in the standard way.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 04/14] staging: clocking-wizard: Cosmetic cleanups

[James Kelly]

Do some cosmetic cleanups before we start adding lots of code.

- Shorten clk_wzrd identifier to cw to keep lines short
- Replace &pdev->dev with dev to keep lines short
- Remove convenience variable np as it was only used once in function
- Add some tabs to make clk_wzrd structure definitions easier to read
- Use #define for clock names in case we want to change them later

Signed-off-by: James Kelly <jamespeterkelly@gmail.com>
---
 .../clocking-wizard/clk-xlnx-clock-wizard.c        | 210 ++++++++++-----------
 1 file changed, 105 insertions(+), 105 deletions(-)

diff --git a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
index 4dec1bfc303a..3b66ac3b5ed0 100644
--- a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
+++ b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
@@ -70,6 +70,8 @@
 
 #define WZRD_NUM_OUTPUTS	7
 #define WZRD_ACLK_MAX_FREQ	250000000UL
+#define WZRD_CLKNAME_AXI	"s_axi_aclk"
+#define WZRD_CLKNAME_IN1	"clk_in1"
 
 #define WZRD_CLK_CFG_REG(n)	(0x200 + 4 * (n))
 
@@ -102,15 +104,15 @@ enum clk_wzrd_int_clks {
  * @suspended:		Flag indicating power state of the device
  */
 struct clk_wzrd {
-	struct clk_onecell_data clk_data;
-	struct notifier_block nb;
-	void __iomem *base;
-	struct clk *clk_in1;
-	struct clk *axi_clk;
-	struct clk *clks_internal[wzrd_clk_int_max];
-	struct clk *clkout[WZRD_NUM_OUTPUTS];
-	unsigned int speed_grade;
-	bool suspended;
+	struct clk_onecell_data		clk_data;
+	struct notifier_block		nb;
+	void __iomem			*base;
+	struct clk			*clk_in1;
+	struct clk			*axi_clk;
+	struct clk			*clks_internal[wzrd_clk_int_max];
+	struct clk			*clkout[WZRD_NUM_OUTPUTS];
+	unsigned int			speed_grade;
+	bool				suspended;
 };
 
 #define to_clk_wzrd(_nb) container_of(_nb, struct clk_wzrd, nb)
@@ -127,14 +129,14 @@ static int clk_wzrd_clk_notifier(struct notifier_block *nb, unsigned long event,
 {
 	unsigned long max;
 	struct clk_notifier_data *ndata = data;
-	struct clk_wzrd *clk_wzrd = to_clk_wzrd(nb);
+	struct clk_wzrd *cw = to_clk_wzrd(nb);
 
-	if (clk_wzrd->suspended)
+	if (cw->suspended)
 		return NOTIFY_OK;
 
-	if (ndata->clk == clk_wzrd->clk_in1)
-		max = clk_wzrd_max_freq[clk_wzrd->speed_grade - 1];
-	else if (ndata->clk == clk_wzrd->axi_clk)
+	if (ndata->clk == cw->clk_in1)
+		max = clk_wzrd_max_freq[cw->speed_grade - 1];
+	else if (ndata->clk == cw->axi_clk)
 		max = WZRD_ACLK_MAX_FREQ;
 	else
 		return NOTIFY_DONE;	/* should never happen */
@@ -153,10 +155,10 @@ static int clk_wzrd_clk_notifier(struct notifier_block *nb, unsigned long event,
 
 static int __maybe_unused clk_wzrd_suspend(struct device *dev)
 {
-	struct clk_wzrd *clk_wzrd = dev_get_drvdata(dev);
+	struct clk_wzrd *cw = dev_get_drvdata(dev);
 
-	clk_disable_unprepare(clk_wzrd->axi_clk);
-	clk_wzrd->suspended = true;
+	clk_disable_unprepare(cw->axi_clk);
+	cw->suspended = true;
 
 	return 0;
 }
@@ -164,15 +166,15 @@ static int __maybe_unused clk_wzrd_suspend(struct device *dev)
 static int __maybe_unused clk_wzrd_resume(struct device *dev)
 {
 	int ret;
-	struct clk_wzrd *clk_wzrd = dev_get_drvdata(dev);
+	struct clk_wzrd *cw = dev_get_drvdata(dev);
 
-	ret = clk_prepare_enable(clk_wzrd->axi_clk);
+	ret = clk_prepare_enable(cw->axi_clk);
 	if (ret) {
 		dev_err(dev, "unable to enable s_axi_aclk\n");
 		return ret;
 	}
 
-	clk_wzrd->suspended = false;
+	cw->suspended = false;
 
 	return 0;
 }
@@ -184,47 +186,46 @@ static int clk_wzrd_get_device_tree_data(struct device *dev)
 {
 	int ret;
 	unsigned long rate;
-	struct clk_wzrd *clk_wzrd;
-	struct platform_device *pdev = to_platform_device(dev);
-	struct device_node *np = pdev->dev.of_node;
+	struct clk_wzrd *cw;
 
-	clk_wzrd = devm_kzalloc(&pdev->dev, sizeof(*clk_wzrd), GFP_KERNEL);
-	if (!clk_wzrd)
+	cw = devm_kzalloc(dev, sizeof(*cw), GFP_KERNEL);
+	if (!cw)
 		return -ENOMEM;
-	platform_set_drvdata(pdev, clk_wzrd);
+	dev_set_drvdata(dev, cw);
 
-	ret = of_property_read_u32(np, "speed-grade", &clk_wzrd->speed_grade);
+	ret = of_property_read_u32(dev->of_node, "speed-grade",
+				   &cw->speed_grade);
 	if (!ret) {
-		if (clk_wzrd->speed_grade < 1 || clk_wzrd->speed_grade > 3) {
-			dev_warn(&pdev->dev, "invalid speed grade '%d'\n",
-				 clk_wzrd->speed_grade);
-			clk_wzrd->speed_grade = 0;
+		if (cw->speed_grade < 1 || cw->speed_grade > 3) {
+			dev_warn(dev, "invalid speed grade '%d'\n",
+				 cw->speed_grade);
+			cw->speed_grade = 0;
 		}
 	}
 
-	clk_wzrd->clk_in1 = devm_clk_get(&pdev->dev, "clk_in1");
-	if (IS_ERR(clk_wzrd->clk_in1)) {
-		if (clk_wzrd->clk_in1 != ERR_PTR(-EPROBE_DEFER))
-			dev_err(&pdev->dev, "clk_in1 not found\n");
-		return PTR_ERR(clk_wzrd->clk_in1);
+	cw->clk_in1 = devm_clk_get(dev, WZRD_CLKNAME_IN1);
+	if (IS_ERR(cw->clk_in1)) {
+		if (cw->clk_in1 != ERR_PTR(-EPROBE_DEFER))
+			dev_err(dev, "Clock %s not found\n", WZRD_CLKNAME_IN1);
+		return PTR_ERR(cw->clk_in1);
 	}
 
-	clk_wzrd->axi_clk = devm_clk_get(&pdev->dev, "s_axi_aclk");
-	if (IS_ERR(clk_wzrd->axi_clk)) {
-		if (clk_wzrd->axi_clk != ERR_PTR(-EPROBE_DEFER))
-			dev_err(&pdev->dev, "s_axi_aclk not found\n");
-		return PTR_ERR(clk_wzrd->axi_clk);
+	cw->axi_clk = devm_clk_get(dev, WZRD_CLKNAME_AXI);
+	if (IS_ERR(cw->axi_clk)) {
+		if (cw->axi_clk != ERR_PTR(-EPROBE_DEFER))
+			dev_err(dev, "Clock %s not found\n", WZRD_CLKNAME_AXI);
+		return PTR_ERR(cw->axi_clk);
 	}
-	ret = clk_prepare_enable(clk_wzrd->axi_clk);
+	ret = clk_prepare_enable(cw->axi_clk);
 	if (ret) {
-		dev_err(&pdev->dev, "enabling s_axi_aclk failed\n");
+		dev_err(dev, "enabling %s failed\n", WZRD_CLKNAME_AXI);
 		return ret;
 	}
-	rate = clk_get_rate(clk_wzrd->axi_clk);
+	rate = clk_get_rate(cw->axi_clk);
 	if (rate > WZRD_ACLK_MAX_FREQ) {
-		dev_err(&pdev->dev, "s_axi_aclk frequency (%lu) too high\n",
+		dev_err(dev, "%s frequency (%lu) too high\n", WZRD_CLKNAME_AXI,
 			rate);
-		clk_disable_unprepare(clk_wzrd->axi_clk);
+		clk_disable_unprepare(cw->axi_clk);
 		return -EINVAL;
 	}
 
@@ -234,13 +235,12 @@ static int clk_wzrd_get_device_tree_data(struct device *dev)
 static int clk_wzrd_regmap_alloc(struct device *dev)
 {
 	struct resource *mem;
-	struct clk_wzrd *clk_wzrd = dev_get_drvdata(dev);
-	struct platform_device *pdev = to_platform_device(dev);
+	struct clk_wzrd *cw = dev_get_drvdata(dev);
 
-	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	clk_wzrd->base = devm_ioremap_resource(&pdev->dev, mem);
-	if (IS_ERR(clk_wzrd->base))
-		return PTR_ERR(clk_wzrd->base);
+	mem = platform_get_resource(to_platform_device(dev), IORESOURCE_MEM, 0);
+	cw->base = devm_ioremap_resource(dev, mem);
+	if (IS_ERR(cw->base))
+		return PTR_ERR(cw->base);
 
 	return 0;
 }
@@ -250,52 +250,50 @@ static int clk_wzrd_register_ccf(struct device *dev)
 	int i, ret;
 	u32 reg;
 	const char *clk_name;
-	struct platform_device *pdev = to_platform_device(dev);
-	struct clk_wzrd *clk_wzrd = dev_get_drvdata(dev);
-	struct device_node *np = pdev->dev.of_node;
+	struct clk_wzrd *cw = dev_get_drvdata(dev);
 
 	/* we don't support fractional div/mul yet */
-	reg = readl(clk_wzrd->base + WZRD_CLK_CFG_REG(0)) &
+	reg = readl(cw->base + WZRD_CLK_CFG_REG(0)) &
 		    WZRD_CLKFBOUT_FRAC_EN;
-	reg |= readl(clk_wzrd->base + WZRD_CLK_CFG_REG(2)) &
+	reg |= readl(cw->base + WZRD_CLK_CFG_REG(2)) &
 		     WZRD_CLKOUT0_FRAC_EN;
 	if (reg)
-		dev_warn(&pdev->dev, "fractional div/mul not supported\n");
+		dev_warn(dev, "fractional div/mul not supported\n");
 
 	/* register div */
-	reg = (readl(clk_wzrd->base + WZRD_CLK_CFG_REG(0)) &
-			WZRD_DIVCLK_DIVIDE_MASK) >> WZRD_DIVCLK_DIVIDE_SHIFT;
-	clk_name = kasprintf(GFP_KERNEL, "%s_mul_div", dev_name(&pdev->dev));
+	reg = (readl(cw->base + WZRD_CLK_CFG_REG(0)) &
+	       WZRD_DIVCLK_DIVIDE_MASK) >> WZRD_DIVCLK_DIVIDE_SHIFT;
+	clk_name = kasprintf(GFP_KERNEL, "%s_mul_div", dev_name(dev));
 	if (!clk_name) {
 		ret = -ENOMEM;
 		goto err_disable_clk;
 	}
-	clk_wzrd->clks_internal[wzrd_clk_mul_div] = clk_register_fixed_factor(
-			&pdev->dev, clk_name,
-			__clk_get_name(clk_wzrd->clk_in1),
+	cw->clks_internal[wzrd_clk_mul_div] = clk_register_fixed_factor(
+			dev, clk_name,
+			__clk_get_name(cw->clk_in1),
 			0, 1, reg);
 	kfree(clk_name);
-	if (IS_ERR(clk_wzrd->clks_internal[wzrd_clk_mul_div])) {
-		dev_err(&pdev->dev, "unable to register divider clock\n");
-		ret = PTR_ERR(clk_wzrd->clks_internal[wzrd_clk_mul_div]);
+	if (IS_ERR(cw->clks_internal[wzrd_clk_mul_div])) {
+		dev_err(dev, "unable to register divider clock\n");
+		ret = PTR_ERR(cw->clks_internal[wzrd_clk_mul_div]);
 		goto err_disable_clk;
 	}
 
 	/* register multiplier */
-	reg = (readl(clk_wzrd->base + WZRD_CLK_CFG_REG(0)) &
-		     WZRD_CLKFBOUT_MULT_MASK) >> WZRD_CLKFBOUT_MULT_SHIFT;
-	clk_name = kasprintf(GFP_KERNEL, "%s_mul", dev_name(&pdev->dev));
+	reg = (readl(cw->base + WZRD_CLK_CFG_REG(0)) &
+	       WZRD_CLKFBOUT_MULT_MASK) >> WZRD_CLKFBOUT_MULT_SHIFT;
+	clk_name = kasprintf(GFP_KERNEL, "%s_mul", dev_name(dev));
 	if (!clk_name) {
 		ret = -ENOMEM;
 		goto err_rm_int_clk;
 	}
-	clk_wzrd->clks_internal[wzrd_clk_mul] = clk_register_fixed_factor(
-			&pdev->dev, clk_name,
-			__clk_get_name(clk_wzrd->clks_internal[wzrd_clk_mul_div]),
+	cw->clks_internal[wzrd_clk_mul] = clk_register_fixed_factor(
+			dev, clk_name,
+			__clk_get_name(cw->clks_internal[wzrd_clk_mul_div]),
 			0, reg, 1);
-	if (IS_ERR(clk_wzrd->clks_internal[wzrd_clk_mul])) {
-		dev_err(&pdev->dev, "unable to register fixed-factor clock\n");
-		ret = PTR_ERR(clk_wzrd->clks_internal[wzrd_clk_mul]);
+	if (IS_ERR(cw->clks_internal[wzrd_clk_mul])) {
+		dev_err(dev, "unable to register fixed-factor clock\n");
+		ret = PTR_ERR(cw->clks_internal[wzrd_clk_mul]);
 		goto err_rm_int_clk;
 	}
 
@@ -303,60 +301,62 @@ static int clk_wzrd_register_ccf(struct device *dev)
 	for (i = WZRD_NUM_OUTPUTS - 1; i >= 0 ; i--) {
 		const char *clkout_name;
 
-		if (of_property_read_string_index(np, "clock-output-names", i,
+		if (of_property_read_string_index(dev->of_node,
+						  "clock-output-names", i,
 						  &clkout_name)) {
-			dev_err(&pdev->dev,
+			dev_err(dev,
 				"clock output name not specified\n");
 			ret = -EINVAL;
 			goto err_rm_int_clks;
 		}
-		reg = readl(clk_wzrd->base + WZRD_CLK_CFG_REG(2) + i * 12);
+		reg = readl(cw->base + WZRD_CLK_CFG_REG(2) + i * 12);
 		reg &= WZRD_CLKOUT_DIVIDE_MASK;
 		reg >>= WZRD_CLKOUT_DIVIDE_SHIFT;
-		clk_wzrd->clkout[i] = clk_register_fixed_factor(&pdev->dev,
-				clkout_name, clk_name, 0, 1, reg);
-		if (IS_ERR(clk_wzrd->clkout[i])) {
+		cw->clkout[i] = clk_register_fixed_factor(dev, clkout_name,
+							  clk_name, 0, 1, reg);
+		if (IS_ERR(cw->clkout[i])) {
 			int j;
 
 			for (j = i + 1; j < WZRD_NUM_OUTPUTS; j++)
-				clk_unregister(clk_wzrd->clkout[j]);
-			dev_err(&pdev->dev,
+				clk_unregister(cw->clkout[j]);
+			dev_err(dev,
 				"unable to register divider clock\n");
-			ret = PTR_ERR(clk_wzrd->clkout[i]);
+			ret = PTR_ERR(cw->clkout[i]);
 			goto err_rm_int_clks;
 		}
 	}
 
 	kfree(clk_name);
 
-	clk_wzrd->clk_data.clks = clk_wzrd->clkout;
-	clk_wzrd->clk_data.clk_num = ARRAY_SIZE(clk_wzrd->clkout);
-	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_wzrd->clk_data);
+	cw->clk_data.clks = cw->clkout;
+	cw->clk_data.clk_num = ARRAY_SIZE(cw->clkout);
+	of_clk_add_provider(dev->of_node, of_clk_src_onecell_get,
+			    &cw->clk_data);
 
-	if (clk_wzrd->speed_grade) {
-		clk_wzrd->nb.notifier_call = clk_wzrd_clk_notifier;
+	if (cw->speed_grade) {
+		cw->nb.notifier_call = clk_wzrd_clk_notifier;
 
-		ret = clk_notifier_register(clk_wzrd->clk_in1,
-					    &clk_wzrd->nb);
+		ret = clk_notifier_register(cw->clk_in1,
+					    &cw->nb);
 		if (ret)
-			dev_warn(&pdev->dev,
+			dev_warn(dev,
 				 "unable to register clock notifier\n");
 
-		ret = clk_notifier_register(clk_wzrd->axi_clk, &clk_wzrd->nb);
+		ret = clk_notifier_register(cw->axi_clk, &cw->nb);
 		if (ret)
-			dev_warn(&pdev->dev,
+			dev_warn(dev,
 				 "unable to register clock notifier\n");
 	}
 
 	return 0;
 
 err_rm_int_clks:
-	clk_unregister(clk_wzrd->clks_internal[1]);
+	clk_unregister(cw->clks_internal[1]);
 err_rm_int_clk:
 	kfree(clk_name);
-	clk_unregister(clk_wzrd->clks_internal[0]);
+	clk_unregister(cw->clks_internal[0]);
 err_disable_clk:
-	clk_disable_unprepare(clk_wzrd->axi_clk);
+	clk_disable_unprepare(cw->axi_clk);
 
 	return ret;
 }
@@ -384,21 +384,21 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 static int clk_wzrd_remove(struct platform_device *pdev)
 {
 	int i;
-	struct clk_wzrd *clk_wzrd = platform_get_drvdata(pdev);
+	struct clk_wzrd *cw = platform_get_drvdata(pdev);
 
 	of_clk_del_provider(pdev->dev.of_node);
 
 	for (i = 0; i < WZRD_NUM_OUTPUTS; i++)
-		clk_unregister(clk_wzrd->clkout[i]);
+		clk_unregister(cw->clkout[i]);
 	for (i = 0; i < wzrd_clk_int_max; i++)
-		clk_unregister(clk_wzrd->clks_internal[i]);
+		clk_unregister(cw->clks_internal[i]);
 
-	if (clk_wzrd->speed_grade) {
-		clk_notifier_unregister(clk_wzrd->axi_clk, &clk_wzrd->nb);
-		clk_notifier_unregister(clk_wzrd->clk_in1, &clk_wzrd->nb);
+	if (cw->speed_grade) {
+		clk_notifier_unregister(cw->axi_clk, &cw->nb);
+		clk_notifier_unregister(cw->clk_in1, &cw->nb);
 	}
 
-	clk_disable_unprepare(clk_wzrd->axi_clk);
+	clk_disable_unprepare(cw->axi_clk);
 
 	return 0;
 }
-- 
2.15.1 (Apple Git-101)

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 05/14] staging: clocking-wizard: Implement CCF clock provider

[James Kelly]

The CCF clock providers that are currently used by the driver are not
capable of supporting the Clocking Wizard IP register interface for
fractional ratios, nor are they able to enforce constraints require to
ensure the PLL will always lock.

None of the common CCF clock providers seem to be a good fit so we
implement a new CCF clock provider within the driver that can be expanded
upon in subsequent patches.  The initial implementation supports multiply
or divide by fixed integer ratios.

The CCF clock provider uses:
- devm kernel APIs for clock registration to simplify error recovery and
  module unloading.
- regmap kernel APIs for memory mapped I/O. Regmap is also able to manage
  prepare/enable/disable/unprepare of the AXI clock for us.

Note that use of the new CCF clock provider is deferred to a subsequent
patch.

Signed-off-by: James Kelly <jamespeterkelly@gmail.com>
---
 .../clocking-wizard/clk-xlnx-clock-wizard.c        | 164 +++++++++++++++++++++
 1 file changed, 164 insertions(+)

diff --git a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
index 3b66ac3b5ed0..ba9b1dc93d50 100644
--- a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
+++ b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
@@ -3,6 +3,7 @@
  * Xilinx 'Clocking Wizard' driver
  *
  *  Copyright (C) 2013 - 2014 Xilinx
+ *  Copyright (C) 2018 James Kelly
  *
  *  Sören Brinkmann <soren.brinkmann@xilinx.com>
  *
@@ -67,11 +68,13 @@
 #include <linux/of.h>
 #include <linux/module.h>
 #include <linux/err.h>
+#include <linux/regmap.h>
 
 #define WZRD_NUM_OUTPUTS	7
 #define WZRD_ACLK_MAX_FREQ	250000000UL
 #define WZRD_CLKNAME_AXI	"s_axi_aclk"
 #define WZRD_CLKNAME_IN1	"clk_in1"
+#define WZRD_FLAG_MULTIPLY	BIT(0)
 
 #define WZRD_CLK_CFG_REG(n)	(0x200 + 4 * (n))
 
@@ -91,28 +94,90 @@ enum clk_wzrd_int_clks {
 	wzrd_clk_int_max
 };
 
+enum clk_wzrd_clk {
+	WZRD_CLK_DIV,
+	WZRD_CLK_PLL,
+	WZRD_CLK_OUT,
+	WZRD_CLK_COUNT
+};
+
+/*
+ * Register map extracted from Xilinx document listed below.
+ *
+ * PG065 Clocking Wizard v6.0 LogiCORE IP Product Guide
+ */
+static const struct regmap_config clk_wzrd_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32
+};
+
+static const struct reg_field clk_wzrd_status_locked = REG_FIELD(0x004,  0,  0);
+static const struct reg_field clk_wzrd_divclk_divide = REG_FIELD(0x200,  0,  7);
+static const struct reg_field clk_wzrd_clkfbout_mult = REG_FIELD(0x200,  8, 15);
+static const struct reg_field clk_wzrd_clkfbout_frac = REG_FIELD(0x200, 16, 25);
+static const struct reg_field clk_wzrd_clkout_divide[WZRD_NUM_OUTPUTS] = {
+	REG_FIELD(0x208, 0, 7),
+	REG_FIELD(0x214, 0, 7),
+	REG_FIELD(0x220, 0, 7),
+	REG_FIELD(0x22C, 0, 7),
+	REG_FIELD(0x238, 0, 7),
+	REG_FIELD(0x244, 0, 7),
+	REG_FIELD(0x250, 0, 7)
+};
+
+static const struct reg_field clk_wzrd_clkout0_frac = REG_FIELD(0x208, 8, 17);
+static const struct reg_field clk_wzrd_reconfig     = REG_FIELD(0x25C, 0,  1);
+
+/**
+ * struct clk_wzrd_clk_data - Clocking Wizard component clock provider data
+ *
+ * @hw:			handle between common and hardware-specific interfaces
+ * @flags:		hardware specific flags
+ * @int_field:		pointer to regmap field for integer part
+ *
+ * Flags:
+ * WZRD_FLAG_MULTIPLY	Clock is a multiplier rather than a divider
+ */
+struct clk_wzrd_clk_data {
+	struct clk_hw			hw;
+	unsigned long			flags;
+	struct regmap_field		*int_field;
+};
+
+#define to_clk_wzrd_clk_data(_hw) \
+		container_of(_hw, struct clk_wzrd_clk_data, hw)
+
 /**
  * struct clk_wzrd:
  * @clk_data:		Clock data
  * @nb:			Notifier block
  * @base:		Memory base
+ * @regmap:		Register map for hardware
  * @clk_in1:		Handle to input clock 'clk_in1'
  * @axi_clk:		Handle to input clock 's_axi_aclk'
  * @clks_internal:	Internal clocks
  * @clkout:		Output clocks
  * @speed_grade:	Speed grade of the device
  * @suspended:		Flag indicating power state of the device
+ * @div:		Divider internal clock provider data
+ * @pll:		Phase locked loop internal clock provider data
+ * @clkout_data:	Array of output clock provider data
  */
 struct clk_wzrd {
 	struct clk_onecell_data		clk_data;
 	struct notifier_block		nb;
 	void __iomem			*base;
+	struct regmap			*regmap;
 	struct clk			*clk_in1;
 	struct clk			*axi_clk;
 	struct clk			*clks_internal[wzrd_clk_int_max];
 	struct clk			*clkout[WZRD_NUM_OUTPUTS];
 	unsigned int			speed_grade;
 	bool				suspended;
+	struct clk_wzrd_clk_data	div_data;
+	struct clk_wzrd_clk_data	pll_data;
+	struct clk_wzrd_clk_data	clkout_data[0];
 };
 
 #define to_clk_wzrd(_nb) container_of(_nb, struct clk_wzrd, nb)
@@ -124,6 +189,105 @@ static const unsigned long clk_wzrd_max_freq[] = {
 	1066000000UL
 };
 
+static unsigned long clk_wzrd_get_ratio(struct clk_wzrd_clk_data *cwc)
+{
+	u32 int_part;
+
+	regmap_field_read(cwc->int_field, &int_part);
+
+	return int_part;
+}
+
+static unsigned long clk_wzrd_calc_rate(unsigned long parent_rate,
+					unsigned long ratio,
+					unsigned long flags)
+{
+	if (flags & WZRD_FLAG_MULTIPLY)
+		return parent_rate * ratio;
+
+	return DIV_ROUND_CLOSEST(parent_rate, ratio);
+}
+
+static unsigned long clk_wzrd_recalc_rate(struct clk_hw *hw,
+					  unsigned long parent_rate)
+{
+	unsigned long ratio;
+	struct clk_wzrd_clk_data *cwc = to_clk_wzrd_clk_data(hw);
+
+	ratio = clk_wzrd_get_ratio(cwc);
+
+	/* Hardware giving invalid information */
+	if (ratio == 0)
+		return 0;
+
+	return clk_wzrd_calc_rate(parent_rate, ratio, cwc->flags);
+}
+
+static const struct clk_ops clk_wzrd_clk_ops = {
+	.recalc_rate = clk_wzrd_recalc_rate,
+};
+
+static int clk_wzrd_register_clk(struct device *dev, const char *name,
+				 enum clk_wzrd_clk type, unsigned int instance,
+				 unsigned long flags)
+{
+	int ret;
+	struct clk_init_data init;
+	const struct reg_field *int_reg_field;
+	struct clk_wzrd_clk_data *cwc;
+	const char *parent_name;
+	struct clk_wzrd *cw = dev_get_drvdata(dev);
+
+	init.ops = &clk_wzrd_clk_ops;
+	init.flags = flags;
+
+	switch (type) {
+	case WZRD_CLK_DIV:
+		cwc = &cw->div_data;
+		int_reg_field = &clk_wzrd_divclk_divide;
+		parent_name = __clk_get_name(cw->clk_in1);
+		break;
+	case WZRD_CLK_PLL:
+		cwc = &cw->pll_data;
+		cwc->flags |= WZRD_FLAG_MULTIPLY;
+		int_reg_field = &clk_wzrd_clkfbout_mult;
+		parent_name = clk_hw_get_name(&cw->div_data.hw);
+		break;
+	case WZRD_CLK_OUT:
+		if (instance >= WZRD_NUM_OUTPUTS) {
+			ret = -EINVAL;
+			goto err;
+		}
+		cwc = &cw->clkout_data[instance];
+		int_reg_field = &clk_wzrd_clkout_divide[instance];
+		parent_name = clk_hw_get_name(&cw->pll_data.hw);
+		break;
+	default:
+		ret = -EINVAL;
+		goto err;
+	}
+
+	init.name = name;
+	init.parent_names = &parent_name;
+	init.num_parents = 1;
+	cwc->hw.init = &init;
+	cwc->int_field = devm_regmap_field_alloc(dev, cw->regmap,
+						 *int_reg_field);
+	if (IS_ERR(cwc->int_field)) {
+		ret = PTR_ERR(cwc->int_field);
+		goto err;
+	}
+
+	ret = devm_clk_hw_register(dev, &cwc->hw);
+	if (ret)
+		goto err;
+
+	return 0;
+err:
+	dev_err(dev, "Unable to register component clock %s\n", name);
+	return ret;
+}
+
 static int clk_wzrd_clk_notifier(struct notifier_block *nb, unsigned long event,
 				 void *data)
 {
-- 
2.15.1 (Apple Git-101)

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 05/14] staging: clocking-wizard: Implement CCF clock provider

[Michal Simek]

On 7.5.2018 03:20, James Kelly wrote:
> The CCF clock providers that are currently used by the driver are not
> capable of supporting the Clocking Wizard IP register interface for
> fractional ratios, nor are they able to enforce constraints require to
> ensure the PLL will always lock.
> 
> None of the common CCF clock providers seem to be a good fit so we
> implement a new CCF clock provider within the driver that can be expanded
> upon in subsequent patches.  The initial implementation supports multiply
> or divide by fixed integer ratios.
> 
> The CCF clock provider uses:
> - devm kernel APIs for clock registration to simplify error recovery and
>   module unloading.
> - regmap kernel APIs for memory mapped I/O. Regmap is also able to manage
>   prepare/enable/disable/unprepare of the AXI clock for us.
> 
> Note that use of the new CCF clock provider is deferred to a subsequent
> patch.
> 
> Signed-off-by: James Kelly <jamespeterkelly@gmail.com>
> ---
>  .../clocking-wizard/clk-xlnx-clock-wizard.c        | 164 +++++++++++++++++++++
>  1 file changed, 164 insertions(+)
> 
> diff --git a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> index 3b66ac3b5ed0..ba9b1dc93d50 100644
> --- a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> +++ b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> @@ -3,6 +3,7 @@
>   * Xilinx 'Clocking Wizard' driver
>   *
>   *  Copyright (C) 2013 - 2014 Xilinx
> + *  Copyright (C) 2018 James Kelly
>   *
>   *  Sören Brinkmann <soren.brinkmann@xilinx.com>
>   *
> @@ -67,11 +68,13 @@
>  #include <linux/of.h>
>  #include <linux/module.h>
>  #include <linux/err.h>
> +#include <linux/regmap.h>
>  
>  #define WZRD_NUM_OUTPUTS	7
>  #define WZRD_ACLK_MAX_FREQ	250000000UL
>  #define WZRD_CLKNAME_AXI	"s_axi_aclk"
>  #define WZRD_CLKNAME_IN1	"clk_in1"
> +#define WZRD_FLAG_MULTIPLY	BIT(0)
>  
>  #define WZRD_CLK_CFG_REG(n)	(0x200 + 4 * (n))
>  
> @@ -91,28 +94,90 @@ enum clk_wzrd_int_clks {
>  	wzrd_clk_int_max
>  };
>  
> +enum clk_wzrd_clk {
> +	WZRD_CLK_DIV,
> +	WZRD_CLK_PLL,
> +	WZRD_CLK_OUT,
> +	WZRD_CLK_COUNT
> +};
> +
> +/*
> + * Register map extracted from Xilinx document listed below.
> + *
> + * PG065 Clocking Wizard v6.0 LogiCORE IP Product Guide
> + */
> +static const struct regmap_config clk_wzrd_regmap_config = {
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32
> +};
> +
> +static const struct reg_field clk_wzrd_status_locked = REG_FIELD(0x004,  0,  0);
> +static const struct reg_field clk_wzrd_divclk_divide = REG_FIELD(0x200,  0,  7);
> +static const struct reg_field clk_wzrd_clkfbout_mult = REG_FIELD(0x200,  8, 15);
> +static const struct reg_field clk_wzrd_clkfbout_frac = REG_FIELD(0x200, 16, 25);
> +static const struct reg_field clk_wzrd_clkout_divide[WZRD_NUM_OUTPUTS] = {
> +	REG_FIELD(0x208, 0, 7),
> +	REG_FIELD(0x214, 0, 7),
> +	REG_FIELD(0x220, 0, 7),
> +	REG_FIELD(0x22C, 0, 7),
> +	REG_FIELD(0x238, 0, 7),
> +	REG_FIELD(0x244, 0, 7),
> +	REG_FIELD(0x250, 0, 7)
> +};
> +
> +static const struct reg_field clk_wzrd_clkout0_frac = REG_FIELD(0x208, 8, 17);
> +static const struct reg_field clk_wzrd_reconfig     = REG_FIELD(0x25C, 0,  1);
> +
> +/**
> + * struct clk_wzrd_clk_data - Clocking Wizard component clock provider data
> + *
> + * @hw:			handle between common and hardware-specific interfaces
> + * @flags:		hardware specific flags
> + * @int_field:		pointer to regmap field for integer part
> + *
> + * Flags:
> + * WZRD_FLAG_MULTIPLY	Clock is a multiplier rather than a divider
> + */
> +struct clk_wzrd_clk_data {
> +	struct clk_hw			hw;
> +	unsigned long			flags;
> +	struct regmap_field		*int_field;
> +};
> +
> +#define to_clk_wzrd_clk_data(_hw) \
> +		container_of(_hw, struct clk_wzrd_clk_data, hw)
> +
>  /**
>   * struct clk_wzrd:
>   * @clk_data:		Clock data
>   * @nb:			Notifier block
>   * @base:		Memory base
> + * @regmap:		Register map for hardware
>   * @clk_in1:		Handle to input clock 'clk_in1'
>   * @axi_clk:		Handle to input clock 's_axi_aclk'
>   * @clks_internal:	Internal clocks
>   * @clkout:		Output clocks
>   * @speed_grade:	Speed grade of the device
>   * @suspended:		Flag indicating power state of the device
> + * @div:		Divider internal clock provider data
> + * @pll:		Phase locked loop internal clock provider data
> + * @clkout_data:	Array of output clock provider data
>   */
>  struct clk_wzrd {
>  	struct clk_onecell_data		clk_data;
>  	struct notifier_block		nb;
>  	void __iomem			*base;
> +	struct regmap			*regmap;
>  	struct clk			*clk_in1;
>  	struct clk			*axi_clk;
>  	struct clk			*clks_internal[wzrd_clk_int_max];
>  	struct clk			*clkout[WZRD_NUM_OUTPUTS];
>  	unsigned int			speed_grade;
>  	bool				suspended;
> +	struct clk_wzrd_clk_data	div_data;
> +	struct clk_wzrd_clk_data	pll_data;

Names don't fit with what it is added to kernel-doc above.

drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c:152: info:
Scanning doc for struct
drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c:181: warning:
Function parameter or member 'div_data' not described in 'clk_wzrd'
drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c:181: warning:
Function parameter or member 'pll_data' not described in 'clk_wzrd'

Anyway. Shubhrajyoti is going to test this series that's why please give
us some time for it.

Thanks,
Michal

Re: [PATCH 05/14] staging: clocking-wizard: Implement CCF clock provider

[James Kelly]

[-- Attachment #1: Type: text/plain, Size: 6663 bytes --]

Hi Michal

On 11 May 2018 at 16:06, Michal Simek <michal.simek@xilinx.com> wrote:

> On 7.5.2018 03:20, James Kelly wrote:
> > The CCF clock providers that are currently used by the driver are not
> > capable of supporting the Clocking Wizard IP register interface for
> > fractional ratios, nor are they able to enforce constraints require to
> > ensure the PLL will always lock.
> >
> > None of the common CCF clock providers seem to be a good fit so we
> > implement a new CCF clock provider within the driver that can be expanded
> > upon in subsequent patches.  The initial implementation supports multiply
> > or divide by fixed integer ratios.
> >
> > The CCF clock provider uses:
> > - devm kernel APIs for clock registration to simplify error recovery and
> >   module unloading.
> > - regmap kernel APIs for memory mapped I/O. Regmap is also able to manage
> >   prepare/enable/disable/unprepare of the AXI clock for us.
> >
> > Note that use of the new CCF clock provider is deferred to a subsequent
> > patch.
> >
> > Signed-off-by: James Kelly <jamespeterkelly@gmail.com>
> > ---
> >  .../clocking-wizard/clk-xlnx-clock-wizard.c        | 164
> +++++++++++++++++++++
> >  1 file changed, 164 insertions(+)
> >
> > diff --git a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> > index 3b66ac3b5ed0..ba9b1dc93d50 100644
> > --- a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> > +++ b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> > @@ -3,6 +3,7 @@
> >   * Xilinx 'Clocking Wizard' driver
> >   *
> >   *  Copyright (C) 2013 - 2014 Xilinx
> > + *  Copyright (C) 2018 James Kelly
> >   *
> >   *  Sören Brinkmann <soren.brinkmann@xilinx.com>
> >   *
> > @@ -67,11 +68,13 @@
> >  #include <linux/of.h>
> >  #include <linux/module.h>
> >  #include <linux/err.h>
> > +#include <linux/regmap.h>
> >
> >  #define WZRD_NUM_OUTPUTS     7
> >  #define WZRD_ACLK_MAX_FREQ   250000000UL
> >  #define WZRD_CLKNAME_AXI     "s_axi_aclk"
> >  #define WZRD_CLKNAME_IN1     "clk_in1"
> > +#define WZRD_FLAG_MULTIPLY   BIT(0)
> >
> >  #define WZRD_CLK_CFG_REG(n)  (0x200 + 4 * (n))
> >
> > @@ -91,28 +94,90 @@ enum clk_wzrd_int_clks {
> >       wzrd_clk_int_max
> >  };
> >
> > +enum clk_wzrd_clk {
> > +     WZRD_CLK_DIV,
> > +     WZRD_CLK_PLL,
> > +     WZRD_CLK_OUT,
> > +     WZRD_CLK_COUNT
> > +};
> > +
> > +/*
> > + * Register map extracted from Xilinx document listed below.
> > + *
> > + * PG065 Clocking Wizard v6.0 LogiCORE IP Product Guide
> > + */
> > +static const struct regmap_config clk_wzrd_regmap_config = {
> > +     .reg_bits = 32,
> > +     .reg_stride = 4,
> > +     .val_bits = 32
> > +};
> > +
> > +static const struct reg_field clk_wzrd_status_locked =
> REG_FIELD(0x004,  0,  0);
> > +static const struct reg_field clk_wzrd_divclk_divide =
> REG_FIELD(0x200,  0,  7);
> > +static const struct reg_field clk_wzrd_clkfbout_mult =
> REG_FIELD(0x200,  8, 15);
> > +static const struct reg_field clk_wzrd_clkfbout_frac = REG_FIELD(0x200,
> 16, 25);
> > +static const struct reg_field clk_wzrd_clkout_divide[WZRD_NUM_OUTPUTS]
> = {
> > +     REG_FIELD(0x208, 0, 7),
> > +     REG_FIELD(0x214, 0, 7),
> > +     REG_FIELD(0x220, 0, 7),
> > +     REG_FIELD(0x22C, 0, 7),
> > +     REG_FIELD(0x238, 0, 7),
> > +     REG_FIELD(0x244, 0, 7),
> > +     REG_FIELD(0x250, 0, 7)
> > +};
> > +
> > +static const struct reg_field clk_wzrd_clkout0_frac = REG_FIELD(0x208,
> 8, 17);
> > +static const struct reg_field clk_wzrd_reconfig     = REG_FIELD(0x25C,
> 0,  1);
> > +
> > +/**
> > + * struct clk_wzrd_clk_data - Clocking Wizard component clock provider
> data
> > + *
> > + * @hw:                      handle between common and
> hardware-specific interfaces
> > + * @flags:           hardware specific flags
> > + * @int_field:               pointer to regmap field for integer part
> > + *
> > + * Flags:
> > + * WZRD_FLAG_MULTIPLY        Clock is a multiplier rather than a divider
> > + */
> > +struct clk_wzrd_clk_data {
> > +     struct clk_hw                   hw;
> > +     unsigned long                   flags;
> > +     struct regmap_field             *int_field;
> > +};
> > +
> > +#define to_clk_wzrd_clk_data(_hw) \
> > +             container_of(_hw, struct clk_wzrd_clk_data, hw)
> > +
> >  /**
> >   * struct clk_wzrd:
> >   * @clk_data:                Clock data
> >   * @nb:                      Notifier block
> >   * @base:            Memory base
> > + * @regmap:          Register map for hardware
> >   * @clk_in1:         Handle to input clock 'clk_in1'
> >   * @axi_clk:         Handle to input clock 's_axi_aclk'
> >   * @clks_internal:   Internal clocks
> >   * @clkout:          Output clocks
> >   * @speed_grade:     Speed grade of the device
> >   * @suspended:               Flag indicating power state of the device
> > + * @div:             Divider internal clock provider data
> > + * @pll:             Phase locked loop internal clock provider data
> > + * @clkout_data:     Array of output clock provider data
> >   */
> >  struct clk_wzrd {
> >       struct clk_onecell_data         clk_data;
> >       struct notifier_block           nb;
> >       void __iomem                    *base;
> > +     struct regmap                   *regmap;
> >       struct clk                      *clk_in1;
> >       struct clk                      *axi_clk;
> >       struct clk                      *clks_internal[wzrd_clk_int_max];
> >       struct clk                      *clkout[WZRD_NUM_OUTPUTS];
> >       unsigned int                    speed_grade;
> >       bool                            suspended;
> > +     struct clk_wzrd_clk_data        div_data;
> > +     struct clk_wzrd_clk_data        pll_data;
>
> Names don't fit with what it is added to kernel-doc above.
>
> drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c:152: info:
> Scanning doc for struct
> drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c:181: warning:
> Function parameter or member 'div_data' not described in 'clk_wzrd'
> drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c:181: warning:
> Function parameter or member 'pll_data' not described in 'clk_wzrd'
>
> Anyway. Shubhrajyoti is going to test this series that's why please give
> us some time for it.
>
> Thanks,
> Michal
>
>
Thanks for your feedback on this and the other patches in this series.

I will incorporate your suggestions into version 2 which I plan to send
to the list early next week.

James

[-- Attachment #2: Type: text/html, Size: 8663 bytes --]

Re: [PATCH 05/14] staging: clocking-wizard: Implement CCF clock provider

[Michal Simek]

On 11.5.2018 09:58, James Kelly wrote:
> Hi Michal
> 
> On 11 May 2018 at 16:06, Michal Simek <michal.simek@xilinx.com> wrote:
> 
>> On 7.5.2018 03:20, James Kelly wrote:
>>> The CCF clock providers that are currently used by the driver are not
>>> capable of supporting the Clocking Wizard IP register interface for
>>> fractional ratios, nor are they able to enforce constraints require to
>>> ensure the PLL will always lock.
>>>
>>> None of the common CCF clock providers seem to be a good fit so we
>>> implement a new CCF clock provider within the driver that can be expanded
>>> upon in subsequent patches.  The initial implementation supports multiply
>>> or divide by fixed integer ratios.
>>>
>>> The CCF clock provider uses:
>>> - devm kernel APIs for clock registration to simplify error recovery and
>>>   module unloading.
>>> - regmap kernel APIs for memory mapped I/O. Regmap is also able to manage
>>>   prepare/enable/disable/unprepare of the AXI clock for us.
>>>
>>> Note that use of the new CCF clock provider is deferred to a subsequent
>>> patch.
>>>
>>> Signed-off-by: James Kelly <jamespeterkelly@gmail.com>
>>> ---
>>>  .../clocking-wizard/clk-xlnx-clock-wizard.c        | 164
>> +++++++++++++++++++++
>>>  1 file changed, 164 insertions(+)
>>>
>>> diff --git a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
>> b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
>>> index 3b66ac3b5ed0..ba9b1dc93d50 100644
>>> --- a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
>>> +++ b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
>>> @@ -3,6 +3,7 @@
>>>   * Xilinx 'Clocking Wizard' driver
>>>   *
>>>   *  Copyright (C) 2013 - 2014 Xilinx
>>> + *  Copyright (C) 2018 James Kelly
>>>   *
>>>   *  Sören Brinkmann <soren.brinkmann@xilinx.com>
>>>   *
>>> @@ -67,11 +68,13 @@
>>>  #include <linux/of.h>
>>>  #include <linux/module.h>
>>>  #include <linux/err.h>
>>> +#include <linux/regmap.h>
>>>
>>>  #define WZRD_NUM_OUTPUTS     7
>>>  #define WZRD_ACLK_MAX_FREQ   250000000UL
>>>  #define WZRD_CLKNAME_AXI     "s_axi_aclk"
>>>  #define WZRD_CLKNAME_IN1     "clk_in1"
>>> +#define WZRD_FLAG_MULTIPLY   BIT(0)
>>>
>>>  #define WZRD_CLK_CFG_REG(n)  (0x200 + 4 * (n))
>>>
>>> @@ -91,28 +94,90 @@ enum clk_wzrd_int_clks {
>>>       wzrd_clk_int_max
>>>  };
>>>
>>> +enum clk_wzrd_clk {
>>> +     WZRD_CLK_DIV,
>>> +     WZRD_CLK_PLL,
>>> +     WZRD_CLK_OUT,
>>> +     WZRD_CLK_COUNT
>>> +};
>>> +
>>> +/*
>>> + * Register map extracted from Xilinx document listed below.
>>> + *
>>> + * PG065 Clocking Wizard v6.0 LogiCORE IP Product Guide
>>> + */
>>> +static const struct regmap_config clk_wzrd_regmap_config = {
>>> +     .reg_bits = 32,
>>> +     .reg_stride = 4,
>>> +     .val_bits = 32
>>> +};
>>> +
>>> +static const struct reg_field clk_wzrd_status_locked =
>> REG_FIELD(0x004,  0,  0);
>>> +static const struct reg_field clk_wzrd_divclk_divide =
>> REG_FIELD(0x200,  0,  7);
>>> +static const struct reg_field clk_wzrd_clkfbout_mult =
>> REG_FIELD(0x200,  8, 15);
>>> +static const struct reg_field clk_wzrd_clkfbout_frac = REG_FIELD(0x200,
>> 16, 25);
>>> +static const struct reg_field clk_wzrd_clkout_divide[WZRD_NUM_OUTPUTS]
>> = {
>>> +     REG_FIELD(0x208, 0, 7),
>>> +     REG_FIELD(0x214, 0, 7),
>>> +     REG_FIELD(0x220, 0, 7),
>>> +     REG_FIELD(0x22C, 0, 7),
>>> +     REG_FIELD(0x238, 0, 7),
>>> +     REG_FIELD(0x244, 0, 7),
>>> +     REG_FIELD(0x250, 0, 7)
>>> +};
>>> +
>>> +static const struct reg_field clk_wzrd_clkout0_frac = REG_FIELD(0x208,
>> 8, 17);
>>> +static const struct reg_field clk_wzrd_reconfig     = REG_FIELD(0x25C,
>> 0,  1);
>>> +
>>> +/**
>>> + * struct clk_wzrd_clk_data - Clocking Wizard component clock provider
>> data
>>> + *
>>> + * @hw:                      handle between common and
>> hardware-specific interfaces
>>> + * @flags:           hardware specific flags
>>> + * @int_field:               pointer to regmap field for integer part
>>> + *
>>> + * Flags:
>>> + * WZRD_FLAG_MULTIPLY        Clock is a multiplier rather than a divider
>>> + */
>>> +struct clk_wzrd_clk_data {
>>> +     struct clk_hw                   hw;
>>> +     unsigned long                   flags;
>>> +     struct regmap_field             *int_field;
>>> +};
>>> +
>>> +#define to_clk_wzrd_clk_data(_hw) \
>>> +             container_of(_hw, struct clk_wzrd_clk_data, hw)
>>> +
>>>  /**
>>>   * struct clk_wzrd:
>>>   * @clk_data:                Clock data
>>>   * @nb:                      Notifier block
>>>   * @base:            Memory base
>>> + * @regmap:          Register map for hardware
>>>   * @clk_in1:         Handle to input clock 'clk_in1'
>>>   * @axi_clk:         Handle to input clock 's_axi_aclk'
>>>   * @clks_internal:   Internal clocks
>>>   * @clkout:          Output clocks
>>>   * @speed_grade:     Speed grade of the device
>>>   * @suspended:               Flag indicating power state of the device
>>> + * @div:             Divider internal clock provider data
>>> + * @pll:             Phase locked loop internal clock provider data
>>> + * @clkout_data:     Array of output clock provider data
>>>   */
>>>  struct clk_wzrd {
>>>       struct clk_onecell_data         clk_data;
>>>       struct notifier_block           nb;
>>>       void __iomem                    *base;
>>> +     struct regmap                   *regmap;
>>>       struct clk                      *clk_in1;
>>>       struct clk                      *axi_clk;
>>>       struct clk                      *clks_internal[wzrd_clk_int_max];
>>>       struct clk                      *clkout[WZRD_NUM_OUTPUTS];
>>>       unsigned int                    speed_grade;
>>>       bool                            suspended;
>>> +     struct clk_wzrd_clk_data        div_data;
>>> +     struct clk_wzrd_clk_data        pll_data;
>>
>> Names don't fit with what it is added to kernel-doc above.
>>
>> drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c:152: info:
>> Scanning doc for struct
>> drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c:181: warning:
>> Function parameter or member 'div_data' not described in 'clk_wzrd'
>> drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c:181: warning:
>> Function parameter or member 'pll_data' not described in 'clk_wzrd'
>>
>> Anyway. Shubhrajyoti is going to test this series that's why please give
>> us some time for it.
>>
>> Thanks,
>> Michal
>>
>>
> Thanks for your feedback on this and the other patches in this series.
> 
> I will incorporate your suggestions into version 2 which I plan to send
> to the list early next week.

It will be great if you can wait for my colleague's feedback from
testing. Recently we have done similar changes in this driver to extend
functionality that's why it shouldn't be hard to test/compare and comment.

Thanks,
Michal

[PATCH 06/14] staging: clocking-wizard: Swap CCF clock providers

[James Kelly]

Replace existing CCF clock providers with new clock provider that can
be enhanced to meet our needs.

AXI clock prepare/enable/disable/unprepare is now managed by regmap APIs.

Unregistering of clk instances now handled by devm APIs.

Drop warning that fractional ratios are not supported because it used
undocumented register fields and it won't be needed anymore.

Signed-off-by: James Kelly <jamespeterkelly@gmail.com>
---
 .../clocking-wizard/clk-xlnx-clock-wizard.c        | 211 +++++++--------------
 1 file changed, 71 insertions(+), 140 deletions(-)

diff --git a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
index ba9b1dc93d50..1dbeda92fb9a 100644
--- a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
+++ b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
@@ -76,24 +76,6 @@
 #define WZRD_CLKNAME_IN1	"clk_in1"
 #define WZRD_FLAG_MULTIPLY	BIT(0)
 
-#define WZRD_CLK_CFG_REG(n)	(0x200 + 4 * (n))
-
-#define WZRD_CLKOUT0_FRAC_EN	BIT(18)
-#define WZRD_CLKFBOUT_FRAC_EN	BIT(26)
-
-#define WZRD_CLKFBOUT_MULT_SHIFT	8
-#define WZRD_CLKFBOUT_MULT_MASK		(0xff << WZRD_CLKFBOUT_MULT_SHIFT)
-#define WZRD_DIVCLK_DIVIDE_SHIFT	0
-#define WZRD_DIVCLK_DIVIDE_MASK		(0xff << WZRD_DIVCLK_DIVIDE_SHIFT)
-#define WZRD_CLKOUT_DIVIDE_SHIFT	0
-#define WZRD_CLKOUT_DIVIDE_MASK		(0xff << WZRD_DIVCLK_DIVIDE_SHIFT)
-
-enum clk_wzrd_int_clks {
-	wzrd_clk_mul_div,
-	wzrd_clk_mul,
-	wzrd_clk_int_max
-};
-
 enum clk_wzrd_clk {
 	WZRD_CLK_DIV,
 	WZRD_CLK_PLL,
@@ -149,14 +131,13 @@ struct clk_wzrd_clk_data {
 		container_of(_hw, struct clk_wzrd_clk_data, hw)
 
 /**
- * struct clk_wzrd:
- * @clk_data:		Clock data
+ * struct clk_wzrd - Platform driver data for clocking wizard device
+ *
+ * @clk_data:		Clock data accessible to other devices via device tree
  * @nb:			Notifier block
- * @base:		Memory base
  * @regmap:		Register map for hardware
  * @clk_in1:		Handle to input clock 'clk_in1'
  * @axi_clk:		Handle to input clock 's_axi_aclk'
- * @clks_internal:	Internal clocks
  * @clkout:		Output clocks
  * @speed_grade:	Speed grade of the device
  * @suspended:		Flag indicating power state of the device
@@ -167,11 +148,9 @@ struct clk_wzrd_clk_data {
 struct clk_wzrd {
 	struct clk_onecell_data		clk_data;
 	struct notifier_block		nb;
-	void __iomem			*base;
 	struct regmap			*regmap;
 	struct clk			*clk_in1;
 	struct clk			*axi_clk;
-	struct clk			*clks_internal[wzrd_clk_int_max];
 	struct clk			*clkout[WZRD_NUM_OUTPUTS];
 	unsigned int			speed_grade;
 	bool				suspended;
@@ -179,7 +158,6 @@ struct clk_wzrd {
 	struct clk_wzrd_clk_data	pll_data;
 	struct clk_wzrd_clk_data	clkout_data[0];
 };
-
 #define to_clk_wzrd(_nb) container_of(_nb, struct clk_wzrd, nb)
 
 /* maximum frequencies for input/output clocks per speed grade */
@@ -321,7 +299,6 @@ static int __maybe_unused clk_wzrd_suspend(struct device *dev)
 {
 	struct clk_wzrd *cw = dev_get_drvdata(dev);
 
-	clk_disable_unprepare(cw->axi_clk);
 	cw->suspended = true;
 
 	return 0;
@@ -329,15 +306,8 @@ static int __maybe_unused clk_wzrd_suspend(struct device *dev)
 
 static int __maybe_unused clk_wzrd_resume(struct device *dev)
 {
-	int ret;
 	struct clk_wzrd *cw = dev_get_drvdata(dev);
 
-	ret = clk_prepare_enable(cw->axi_clk);
-	if (ret) {
-		dev_err(dev, "unable to enable s_axi_aclk\n");
-		return ret;
-	}
-
 	cw->suspended = false;
 
 	return 0;
@@ -348,17 +318,32 @@ static SIMPLE_DEV_PM_OPS(clk_wzrd_dev_pm_ops, clk_wzrd_suspend,
 
 static int clk_wzrd_get_device_tree_data(struct device *dev)
 {
-	int ret;
-	unsigned long rate;
+	int num_outputs, ret;
 	struct clk_wzrd *cw;
+	struct device_node *node = dev->of_node;
+
+	num_outputs = of_property_count_strings(node,
+						"clock-output-names");
+	if (num_outputs < 1) {
+		dev_err(dev, "No clock output names in device tree\n");
+		if (num_outputs < 0)
+			return num_outputs;
+		else
+			return -EINVAL;
+	}
+	if (num_outputs > WZRD_NUM_OUTPUTS) {
+		dev_warn(dev, "Too many clock output names in device tree\n");
+		num_outputs = WZRD_NUM_OUTPUTS;
+	}
 
-	cw = devm_kzalloc(dev, sizeof(*cw), GFP_KERNEL);
+	cw = devm_kzalloc(dev, sizeof(*cw) + num_outputs *
+			  sizeof(struct clk_wzrd_clk_data), GFP_KERNEL);
 	if (!cw)
 		return -ENOMEM;
 	dev_set_drvdata(dev, cw);
+	cw->clk_data.clk_num = num_outputs;
 
-	ret = of_property_read_u32(dev->of_node, "speed-grade",
-				   &cw->speed_grade);
+	ret = of_property_read_u32(node, "speed-grade", &cw->speed_grade);
 	if (!ret) {
 		if (cw->speed_grade < 1 || cw->speed_grade > 3) {
 			dev_warn(dev, "invalid speed grade '%d'\n",
@@ -380,18 +365,12 @@ static int clk_wzrd_get_device_tree_data(struct device *dev)
 			dev_err(dev, "Clock %s not found\n", WZRD_CLKNAME_AXI);
 		return PTR_ERR(cw->axi_clk);
 	}
-	ret = clk_prepare_enable(cw->axi_clk);
+	ret = clk_set_max_rate(cw->axi_clk, WZRD_ACLK_MAX_FREQ);
 	if (ret) {
-		dev_err(dev, "enabling %s failed\n", WZRD_CLKNAME_AXI);
+		dev_err(dev, "Unable to set clock %s to valid range\n",
+			WZRD_CLKNAME_AXI);
 		return ret;
 	}
-	rate = clk_get_rate(cw->axi_clk);
-	if (rate > WZRD_ACLK_MAX_FREQ) {
-		dev_err(dev, "%s frequency (%lu) too high\n", WZRD_CLKNAME_AXI,
-			rate);
-		clk_disable_unprepare(cw->axi_clk);
-		return -EINVAL;
-	}
 
 	return 0;
 }
@@ -399,12 +378,18 @@ static int clk_wzrd_get_device_tree_data(struct device *dev)
 static int clk_wzrd_regmap_alloc(struct device *dev)
 {
 	struct resource *mem;
+	void __iomem *base;
 	struct clk_wzrd *cw = dev_get_drvdata(dev);
 
 	mem = platform_get_resource(to_platform_device(dev), IORESOURCE_MEM, 0);
-	cw->base = devm_ioremap_resource(dev, mem);
-	if (IS_ERR(cw->base))
-		return PTR_ERR(cw->base);
+	base = devm_ioremap_resource(dev, mem);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	cw->regmap = devm_regmap_init_mmio_clk(dev, WZRD_CLKNAME_AXI,
+					       base, &clk_wzrd_regmap_config);
+	if (IS_ERR(cw->regmap))
+		return PTR_ERR(cw->regmap);
 
 	return 0;
 }
@@ -412,115 +397,68 @@ static int clk_wzrd_regmap_alloc(struct device *dev)
 static int clk_wzrd_register_ccf(struct device *dev)
 {
 	int i, ret;
-	u32 reg;
-	const char *clk_name;
+	const char *clk_div_name;
+	const char *clk_pll_name;
+
 	struct clk_wzrd *cw = dev_get_drvdata(dev);
 
-	/* we don't support fractional div/mul yet */
-	reg = readl(cw->base + WZRD_CLK_CFG_REG(0)) &
-		    WZRD_CLKFBOUT_FRAC_EN;
-	reg |= readl(cw->base + WZRD_CLK_CFG_REG(2)) &
-		     WZRD_CLKOUT0_FRAC_EN;
-	if (reg)
-		dev_warn(dev, "fractional div/mul not supported\n");
-
-	/* register div */
-	reg = (readl(cw->base + WZRD_CLK_CFG_REG(0)) &
-	       WZRD_DIVCLK_DIVIDE_MASK) >> WZRD_DIVCLK_DIVIDE_SHIFT;
-	clk_name = kasprintf(GFP_KERNEL, "%s_mul_div", dev_name(dev));
-	if (!clk_name) {
-		ret = -ENOMEM;
-		goto err_disable_clk;
-	}
-	cw->clks_internal[wzrd_clk_mul_div] = clk_register_fixed_factor(
-			dev, clk_name,
-			__clk_get_name(cw->clk_in1),
-			0, 1, reg);
-	kfree(clk_name);
-	if (IS_ERR(cw->clks_internal[wzrd_clk_mul_div])) {
-		dev_err(dev, "unable to register divider clock\n");
-		ret = PTR_ERR(cw->clks_internal[wzrd_clk_mul_div]);
-		goto err_disable_clk;
-	}
+	clk_div_name = kasprintf(GFP_KERNEL, "%s_div", dev_name(dev));
+	if (!clk_div_name)
+		return -ENOMEM;
+	ret = clk_wzrd_register_clk(dev, clk_div_name, WZRD_CLK_DIV, 0, 0);
+	if (ret)
+		goto err_free_div_name;
 
-	/* register multiplier */
-	reg = (readl(cw->base + WZRD_CLK_CFG_REG(0)) &
-	       WZRD_CLKFBOUT_MULT_MASK) >> WZRD_CLKFBOUT_MULT_SHIFT;
-	clk_name = kasprintf(GFP_KERNEL, "%s_mul", dev_name(dev));
-	if (!clk_name) {
+	clk_pll_name = kasprintf(GFP_KERNEL, "%s_pll", dev_name(dev));
+	if (!clk_pll_name) {
 		ret = -ENOMEM;
-		goto err_rm_int_clk;
-	}
-	cw->clks_internal[wzrd_clk_mul] = clk_register_fixed_factor(
-			dev, clk_name,
-			__clk_get_name(cw->clks_internal[wzrd_clk_mul_div]),
-			0, reg, 1);
-	if (IS_ERR(cw->clks_internal[wzrd_clk_mul])) {
-		dev_err(dev, "unable to register fixed-factor clock\n");
-		ret = PTR_ERR(cw->clks_internal[wzrd_clk_mul]);
-		goto err_rm_int_clk;
+		goto err_free_div_name;
 	}
+	ret = clk_wzrd_register_clk(dev, clk_pll_name, WZRD_CLK_PLL, 0, 0);
+	if (ret)
+		goto err_free_pll_name;
 
-	/* register div per output */
-	for (i = WZRD_NUM_OUTPUTS - 1; i >= 0 ; i--) {
+	for (i = cw->clk_data.clk_num - 1; i >= 0 ; i--) {
 		const char *clkout_name;
 
 		if (of_property_read_string_index(dev->of_node,
 						  "clock-output-names", i,
 						  &clkout_name)) {
-			dev_err(dev,
-				"clock output name not specified\n");
+			dev_err(dev, "clock output %d name not specified\n", i);
 			ret = -EINVAL;
-			goto err_rm_int_clks;
+			goto err_free_pll_name;
 		}
-		reg = readl(cw->base + WZRD_CLK_CFG_REG(2) + i * 12);
-		reg &= WZRD_CLKOUT_DIVIDE_MASK;
-		reg >>= WZRD_CLKOUT_DIVIDE_SHIFT;
-		cw->clkout[i] = clk_register_fixed_factor(dev, clkout_name,
-							  clk_name, 0, 1, reg);
-		if (IS_ERR(cw->clkout[i])) {
-			int j;
-
-			for (j = i + 1; j < WZRD_NUM_OUTPUTS; j++)
-				clk_unregister(cw->clkout[j]);
-			dev_err(dev,
-				"unable to register divider clock\n");
-			ret = PTR_ERR(cw->clkout[i]);
-			goto err_rm_int_clks;
-		}
-	}
 
-	kfree(clk_name);
+		ret = clk_wzrd_register_clk(dev, clkout_name, WZRD_CLK_OUT,
+					    i, 0);
+		if (ret)
+			goto err_free_pll_name;
+
+		cw->clkout[i] = cw->clkout_data[i].hw.clk;
+	}
 
 	cw->clk_data.clks = cw->clkout;
-	cw->clk_data.clk_num = ARRAY_SIZE(cw->clkout);
 	of_clk_add_provider(dev->of_node, of_clk_src_onecell_get,
 			    &cw->clk_data);
 
 	if (cw->speed_grade) {
 		cw->nb.notifier_call = clk_wzrd_clk_notifier;
 
-		ret = clk_notifier_register(cw->clk_in1,
-					    &cw->nb);
-		if (ret)
-			dev_warn(dev,
-				 "unable to register clock notifier\n");
+		ret = clk_notifier_register(cw->clk_in1, &cw->nb);
+			if (ret)
+				dev_warn(dev, "Unable to register input clock notifier\n");
 
 		ret = clk_notifier_register(cw->axi_clk, &cw->nb);
-		if (ret)
-			dev_warn(dev,
-				 "unable to register clock notifier\n");
+			if (ret)
+				dev_warn(dev, "Unable to register AXI clock notifier\n");
 	}
 
-	return 0;
+	ret = 0;
 
-err_rm_int_clks:
-	clk_unregister(cw->clks_internal[1]);
-err_rm_int_clk:
-	kfree(clk_name);
-	clk_unregister(cw->clks_internal[0]);
-err_disable_clk:
-	clk_disable_unprepare(cw->axi_clk);
+err_free_pll_name:
+	kfree(clk_pll_name);
+err_free_div_name:
+	kfree(clk_div_name);
 
 	return ret;
 }
@@ -547,23 +485,15 @@ static int clk_wzrd_probe(struct platform_device *pdev)
 
 static int clk_wzrd_remove(struct platform_device *pdev)
 {
-	int i;
 	struct clk_wzrd *cw = platform_get_drvdata(pdev);
 
 	of_clk_del_provider(pdev->dev.of_node);
 
-	for (i = 0; i < WZRD_NUM_OUTPUTS; i++)
-		clk_unregister(cw->clkout[i]);
-	for (i = 0; i < wzrd_clk_int_max; i++)
-		clk_unregister(cw->clks_internal[i]);
-
 	if (cw->speed_grade) {
 		clk_notifier_unregister(cw->axi_clk, &cw->nb);
 		clk_notifier_unregister(cw->clk_in1, &cw->nb);
 	}
 
-	clk_disable_unprepare(cw->axi_clk);
-
 	return 0;
 }
 
@@ -577,6 +507,7 @@ static struct platform_driver clk_wzrd_driver = {
 	.driver = {
 		.name = "clk-wizard",
 		.of_match_table = clk_wzrd_ids,
+		.owner = THIS_MODULE,
 		.pm = &clk_wzrd_dev_pm_ops,
 	},
 	.probe = clk_wzrd_probe,
-- 
2.15.1 (Apple Git-101)

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 06/14] staging: clocking-wizard: Swap CCF clock providers

[Michal Simek]

On 7.5.2018 03:20, James Kelly wrote:
> Replace existing CCF clock providers with new clock provider that can
> be enhanced to meet our needs.
> 
> AXI clock prepare/enable/disable/unprepare is now managed by regmap APIs.
> 
> Unregistering of clk instances now handled by devm APIs.
> 
> Drop warning that fractional ratios are not supported because it used
> undocumented register fields and it won't be needed anymore.
> 
> Signed-off-by: James Kelly <jamespeterkelly@gmail.com>
> ---
>  .../clocking-wizard/clk-xlnx-clock-wizard.c        | 211 +++++++--------------
>  1 file changed, 71 insertions(+), 140 deletions(-)
> 
> diff --git a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> index ba9b1dc93d50..1dbeda92fb9a 100644
> --- a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> +++ b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> @@ -76,24 +76,6 @@
>  #define WZRD_CLKNAME_IN1	"clk_in1"
>  #define WZRD_FLAG_MULTIPLY	BIT(0)
>  
> -#define WZRD_CLK_CFG_REG(n)	(0x200 + 4 * (n))
> -
> -#define WZRD_CLKOUT0_FRAC_EN	BIT(18)
> -#define WZRD_CLKFBOUT_FRAC_EN	BIT(26)
> -
> -#define WZRD_CLKFBOUT_MULT_SHIFT	8
> -#define WZRD_CLKFBOUT_MULT_MASK		(0xff << WZRD_CLKFBOUT_MULT_SHIFT)
> -#define WZRD_DIVCLK_DIVIDE_SHIFT	0
> -#define WZRD_DIVCLK_DIVIDE_MASK		(0xff << WZRD_DIVCLK_DIVIDE_SHIFT)
> -#define WZRD_CLKOUT_DIVIDE_SHIFT	0
> -#define WZRD_CLKOUT_DIVIDE_MASK		(0xff << WZRD_DIVCLK_DIVIDE_SHIFT)
> -
> -enum clk_wzrd_int_clks {
> -	wzrd_clk_mul_div,
> -	wzrd_clk_mul,
> -	wzrd_clk_int_max
> -};
> -
>  enum clk_wzrd_clk {
>  	WZRD_CLK_DIV,
>  	WZRD_CLK_PLL,
> @@ -149,14 +131,13 @@ struct clk_wzrd_clk_data {
>  		container_of(_hw, struct clk_wzrd_clk_data, hw)
>  
>  /**
> - * struct clk_wzrd:
> - * @clk_data:		Clock data
> + * struct clk_wzrd - Platform driver data for clocking wizard device
> + *
> + * @clk_data:		Clock data accessible to other devices via device tree
>   * @nb:			Notifier block
> - * @base:		Memory base
>   * @regmap:		Register map for hardware
>   * @clk_in1:		Handle to input clock 'clk_in1'
>   * @axi_clk:		Handle to input clock 's_axi_aclk'
> - * @clks_internal:	Internal clocks
>   * @clkout:		Output clocks
>   * @speed_grade:	Speed grade of the device
>   * @suspended:		Flag indicating power state of the device
> @@ -167,11 +148,9 @@ struct clk_wzrd_clk_data {
>  struct clk_wzrd {
>  	struct clk_onecell_data		clk_data;
>  	struct notifier_block		nb;
> -	void __iomem			*base;
>  	struct regmap			*regmap;
>  	struct clk			*clk_in1;
>  	struct clk			*axi_clk;
> -	struct clk			*clks_internal[wzrd_clk_int_max];
>  	struct clk			*clkout[WZRD_NUM_OUTPUTS];
>  	unsigned int			speed_grade;
>  	bool				suspended;
> @@ -179,7 +158,6 @@ struct clk_wzrd {
>  	struct clk_wzrd_clk_data	pll_data;
>  	struct clk_wzrd_clk_data	clkout_data[0];
>  };
> -
>  #define to_clk_wzrd(_nb) container_of(_nb, struct clk_wzrd, nb)
>  
>  /* maximum frequencies for input/output clocks per speed grade */
> @@ -321,7 +299,6 @@ static int __maybe_unused clk_wzrd_suspend(struct device *dev)
>  {
>  	struct clk_wzrd *cw = dev_get_drvdata(dev);
>  
> -	clk_disable_unprepare(cw->axi_clk);
>  	cw->suspended = true;
>  
>  	return 0;
> @@ -329,15 +306,8 @@ static int __maybe_unused clk_wzrd_suspend(struct device *dev)
>  
>  static int __maybe_unused clk_wzrd_resume(struct device *dev)
>  {
> -	int ret;
>  	struct clk_wzrd *cw = dev_get_drvdata(dev);
>  
> -	ret = clk_prepare_enable(cw->axi_clk);
> -	if (ret) {
> -		dev_err(dev, "unable to enable s_axi_aclk\n");
> -		return ret;
> -	}
> -
>  	cw->suspended = false;
>  
>  	return 0;
> @@ -348,17 +318,32 @@ static SIMPLE_DEV_PM_OPS(clk_wzrd_dev_pm_ops, clk_wzrd_suspend,
>  
>  static int clk_wzrd_get_device_tree_data(struct device *dev)
>  {
> -	int ret;
> -	unsigned long rate;
> +	int num_outputs, ret;
>  	struct clk_wzrd *cw;
> +	struct device_node *node = dev->of_node;
> +
> +	num_outputs = of_property_count_strings(node,
> +						"clock-output-names");
> +	if (num_outputs < 1) {
> +		dev_err(dev, "No clock output names in device tree\n");
> +		if (num_outputs < 0)
> +			return num_outputs;
> +		else
> +			return -EINVAL;
> +	}
> +	if (num_outputs > WZRD_NUM_OUTPUTS) {
> +		dev_warn(dev, "Too many clock output names in device tree\n");
> +		num_outputs = WZRD_NUM_OUTPUTS;
> +	}
>  
> -	cw = devm_kzalloc(dev, sizeof(*cw), GFP_KERNEL);
> +	cw = devm_kzalloc(dev, sizeof(*cw) + num_outputs *
> +			  sizeof(struct clk_wzrd_clk_data), GFP_KERNEL);
>  	if (!cw)
>  		return -ENOMEM;
>  	dev_set_drvdata(dev, cw);
> +	cw->clk_data.clk_num = num_outputs;
>  
> -	ret = of_property_read_u32(dev->of_node, "speed-grade",
> -				   &cw->speed_grade);
> +	ret = of_property_read_u32(node, "speed-grade", &cw->speed_grade);
>  	if (!ret) {
>  		if (cw->speed_grade < 1 || cw->speed_grade > 3) {
>  			dev_warn(dev, "invalid speed grade '%d'\n",
> @@ -380,18 +365,12 @@ static int clk_wzrd_get_device_tree_data(struct device *dev)
>  			dev_err(dev, "Clock %s not found\n", WZRD_CLKNAME_AXI);
>  		return PTR_ERR(cw->axi_clk);
>  	}
> -	ret = clk_prepare_enable(cw->axi_clk);
> +	ret = clk_set_max_rate(cw->axi_clk, WZRD_ACLK_MAX_FREQ);
>  	if (ret) {
> -		dev_err(dev, "enabling %s failed\n", WZRD_CLKNAME_AXI);
> +		dev_err(dev, "Unable to set clock %s to valid range\n",
> +			WZRD_CLKNAME_AXI);
>  		return ret;
>  	}
> -	rate = clk_get_rate(cw->axi_clk);
> -	if (rate > WZRD_ACLK_MAX_FREQ) {
> -		dev_err(dev, "%s frequency (%lu) too high\n", WZRD_CLKNAME_AXI,
> -			rate);
> -		clk_disable_unprepare(cw->axi_clk);
> -		return -EINVAL;
> -	}
>  
>  	return 0;
>  }
> @@ -399,12 +378,18 @@ static int clk_wzrd_get_device_tree_data(struct device *dev)
>  static int clk_wzrd_regmap_alloc(struct device *dev)
>  {
>  	struct resource *mem;
> +	void __iomem *base;
>  	struct clk_wzrd *cw = dev_get_drvdata(dev);
>  
>  	mem = platform_get_resource(to_platform_device(dev), IORESOURCE_MEM, 0);
> -	cw->base = devm_ioremap_resource(dev, mem);
> -	if (IS_ERR(cw->base))
> -		return PTR_ERR(cw->base);
> +	base = devm_ioremap_resource(dev, mem);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	cw->regmap = devm_regmap_init_mmio_clk(dev, WZRD_CLKNAME_AXI,
> +					       base, &clk_wzrd_regmap_config);
> +	if (IS_ERR(cw->regmap))
> +		return PTR_ERR(cw->regmap);
>  
>  	return 0;
>  }
> @@ -412,115 +397,68 @@ static int clk_wzrd_regmap_alloc(struct device *dev)
>  static int clk_wzrd_register_ccf(struct device *dev)
>  {
>  	int i, ret;
> -	u32 reg;
> -	const char *clk_name;
> +	const char *clk_div_name;
> +	const char *clk_pll_name;
> +
>  	struct clk_wzrd *cw = dev_get_drvdata(dev);
>  
> -	/* we don't support fractional div/mul yet */
> -	reg = readl(cw->base + WZRD_CLK_CFG_REG(0)) &
> -		    WZRD_CLKFBOUT_FRAC_EN;
> -	reg |= readl(cw->base + WZRD_CLK_CFG_REG(2)) &
> -		     WZRD_CLKOUT0_FRAC_EN;
> -	if (reg)
> -		dev_warn(dev, "fractional div/mul not supported\n");
> -
> -	/* register div */
> -	reg = (readl(cw->base + WZRD_CLK_CFG_REG(0)) &
> -	       WZRD_DIVCLK_DIVIDE_MASK) >> WZRD_DIVCLK_DIVIDE_SHIFT;
> -	clk_name = kasprintf(GFP_KERNEL, "%s_mul_div", dev_name(dev));
> -	if (!clk_name) {
> -		ret = -ENOMEM;
> -		goto err_disable_clk;
> -	}
> -	cw->clks_internal[wzrd_clk_mul_div] = clk_register_fixed_factor(
> -			dev, clk_name,
> -			__clk_get_name(cw->clk_in1),
> -			0, 1, reg);
> -	kfree(clk_name);
> -	if (IS_ERR(cw->clks_internal[wzrd_clk_mul_div])) {
> -		dev_err(dev, "unable to register divider clock\n");
> -		ret = PTR_ERR(cw->clks_internal[wzrd_clk_mul_div]);
> -		goto err_disable_clk;
> -	}
> +	clk_div_name = kasprintf(GFP_KERNEL, "%s_div", dev_name(dev));
> +	if (!clk_div_name)
> +		return -ENOMEM;
> +	ret = clk_wzrd_register_clk(dev, clk_div_name, WZRD_CLK_DIV, 0, 0);
> +	if (ret)
> +		goto err_free_div_name;
>  
> -	/* register multiplier */
> -	reg = (readl(cw->base + WZRD_CLK_CFG_REG(0)) &
> -	       WZRD_CLKFBOUT_MULT_MASK) >> WZRD_CLKFBOUT_MULT_SHIFT;
> -	clk_name = kasprintf(GFP_KERNEL, "%s_mul", dev_name(dev));
> -	if (!clk_name) {
> +	clk_pll_name = kasprintf(GFP_KERNEL, "%s_pll", dev_name(dev));
> +	if (!clk_pll_name) {
>  		ret = -ENOMEM;
> -		goto err_rm_int_clk;
> -	}
> -	cw->clks_internal[wzrd_clk_mul] = clk_register_fixed_factor(
> -			dev, clk_name,
> -			__clk_get_name(cw->clks_internal[wzrd_clk_mul_div]),
> -			0, reg, 1);
> -	if (IS_ERR(cw->clks_internal[wzrd_clk_mul])) {
> -		dev_err(dev, "unable to register fixed-factor clock\n");
> -		ret = PTR_ERR(cw->clks_internal[wzrd_clk_mul]);
> -		goto err_rm_int_clk;
> +		goto err_free_div_name;
>  	}
> +	ret = clk_wzrd_register_clk(dev, clk_pll_name, WZRD_CLK_PLL, 0, 0);
> +	if (ret)
> +		goto err_free_pll_name;
>  
> -	/* register div per output */
> -	for (i = WZRD_NUM_OUTPUTS - 1; i >= 0 ; i--) {
> +	for (i = cw->clk_data.clk_num - 1; i >= 0 ; i--) {
>  		const char *clkout_name;
>  
>  		if (of_property_read_string_index(dev->of_node,
>  						  "clock-output-names", i,
>  						  &clkout_name)) {
> -			dev_err(dev,
> -				"clock output name not specified\n");
> +			dev_err(dev, "clock output %d name not specified\n", i);
>  			ret = -EINVAL;
> -			goto err_rm_int_clks;
> +			goto err_free_pll_name;
>  		}
> -		reg = readl(cw->base + WZRD_CLK_CFG_REG(2) + i * 12);
> -		reg &= WZRD_CLKOUT_DIVIDE_MASK;
> -		reg >>= WZRD_CLKOUT_DIVIDE_SHIFT;
> -		cw->clkout[i] = clk_register_fixed_factor(dev, clkout_name,
> -							  clk_name, 0, 1, reg);
> -		if (IS_ERR(cw->clkout[i])) {
> -			int j;
> -
> -			for (j = i + 1; j < WZRD_NUM_OUTPUTS; j++)
> -				clk_unregister(cw->clkout[j]);
> -			dev_err(dev,
> -				"unable to register divider clock\n");
> -			ret = PTR_ERR(cw->clkout[i]);
> -			goto err_rm_int_clks;
> -		}
> -	}
>  
> -	kfree(clk_name);
> +		ret = clk_wzrd_register_clk(dev, clkout_name, WZRD_CLK_OUT,
> +					    i, 0);
> +		if (ret)
> +			goto err_free_pll_name;
> +
> +		cw->clkout[i] = cw->clkout_data[i].hw.clk;
> +	}
>  
>  	cw->clk_data.clks = cw->clkout;
> -	cw->clk_data.clk_num = ARRAY_SIZE(cw->clkout);
>  	of_clk_add_provider(dev->of_node, of_clk_src_onecell_get,
>  			    &cw->clk_data);
>  
>  	if (cw->speed_grade) {
>  		cw->nb.notifier_call = clk_wzrd_clk_notifier;
>  
> -		ret = clk_notifier_register(cw->clk_in1,
> -					    &cw->nb);
> -		if (ret)
> -			dev_warn(dev,
> -				 "unable to register clock notifier\n");
> +		ret = clk_notifier_register(cw->clk_in1, &cw->nb);
> +			if (ret)
> +				dev_warn(dev, "Unable to register input clock notifier\n");
>  
>  		ret = clk_notifier_register(cw->axi_clk, &cw->nb);
> -		if (ret)
> -			dev_warn(dev,
> -				 "unable to register clock notifier\n");
> +			if (ret)
> +				dev_warn(dev, "Unable to register AXI clock notifier\n");
>  	}
>  
> -	return 0;
> +	ret = 0;
>  
> -err_rm_int_clks:
> -	clk_unregister(cw->clks_internal[1]);
> -err_rm_int_clk:
> -	kfree(clk_name);
> -	clk_unregister(cw->clks_internal[0]);
> -err_disable_clk:
> -	clk_disable_unprepare(cw->axi_clk);
> +err_free_pll_name:
> +	kfree(clk_pll_name);
> +err_free_div_name:
> +	kfree(clk_div_name);
>  
>  	return ret;
>  }
> @@ -547,23 +485,15 @@ static int clk_wzrd_probe(struct platform_device *pdev)
>  
>  static int clk_wzrd_remove(struct platform_device *pdev)
>  {
> -	int i;
>  	struct clk_wzrd *cw = platform_get_drvdata(pdev);
>  
>  	of_clk_del_provider(pdev->dev.of_node);
>  
> -	for (i = 0; i < WZRD_NUM_OUTPUTS; i++)
> -		clk_unregister(cw->clkout[i]);
> -	for (i = 0; i < wzrd_clk_int_max; i++)
> -		clk_unregister(cw->clks_internal[i]);
> -
>  	if (cw->speed_grade) {
>  		clk_notifier_unregister(cw->axi_clk, &cw->nb);
>  		clk_notifier_unregister(cw->clk_in1, &cw->nb);
>  	}
>  
> -	clk_disable_unprepare(cw->axi_clk);
> -
>  	return 0;
>  }
>  
> @@ -577,6 +507,7 @@ static struct platform_driver clk_wzrd_driver = {
>  	.driver = {
>  		.name = "clk-wizard",
>  		.of_match_table = clk_wzrd_ids,
> +		.owner = THIS_MODULE,

This is not needed and coccinelle check this.
drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c:510:3-8: No need
to set .owner here. The core will do it.


Also there is one more warning.
drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c:391:1-3:
WARNING: PTR_ERR_OR_ZERO can be used

Thanks,
Michal

Re: [PATCH 06/14] staging: clocking-wizard: Swap CCF clock providers

[Michal Simek]

On 7.5.2018 03:20, James Kelly wrote:
> Replace existing CCF clock providers with new clock provider that can
> be enhanced to meet our needs.
> 
> AXI clock prepare/enable/disable/unprepare is now managed by regmap APIs.
> 
> Unregistering of clk instances now handled by devm APIs.
> 
> Drop warning that fractional ratios are not supported because it used
> undocumented register fields and it won't be needed anymore.
> 
> Signed-off-by: James Kelly <jamespeterkelly@gmail.com>
> ---
>  .../clocking-wizard/clk-xlnx-clock-wizard.c        | 211 +++++++--------------
>  1 file changed, 71 insertions(+), 140 deletions(-)
> 
> diff --git a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> index ba9b1dc93d50..1dbeda92fb9a 100644
> --- a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> +++ b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> @@ -76,24 +76,6 @@
>  #define WZRD_CLKNAME_IN1	"clk_in1"
>  #define WZRD_FLAG_MULTIPLY	BIT(0)
>  
> -#define WZRD_CLK_CFG_REG(n)	(0x200 + 4 * (n))
> -
> -#define WZRD_CLKOUT0_FRAC_EN	BIT(18)
> -#define WZRD_CLKFBOUT_FRAC_EN	BIT(26)
> -
> -#define WZRD_CLKFBOUT_MULT_SHIFT	8
> -#define WZRD_CLKFBOUT_MULT_MASK		(0xff << WZRD_CLKFBOUT_MULT_SHIFT)
> -#define WZRD_DIVCLK_DIVIDE_SHIFT	0
> -#define WZRD_DIVCLK_DIVIDE_MASK		(0xff << WZRD_DIVCLK_DIVIDE_SHIFT)
> -#define WZRD_CLKOUT_DIVIDE_SHIFT	0
> -#define WZRD_CLKOUT_DIVIDE_MASK		(0xff << WZRD_DIVCLK_DIVIDE_SHIFT)
> -
> -enum clk_wzrd_int_clks {
> -	wzrd_clk_mul_div,
> -	wzrd_clk_mul,
> -	wzrd_clk_int_max
> -};
> -
>  enum clk_wzrd_clk {
>  	WZRD_CLK_DIV,
>  	WZRD_CLK_PLL,
> @@ -149,14 +131,13 @@ struct clk_wzrd_clk_data {
>  		container_of(_hw, struct clk_wzrd_clk_data, hw)
>  
>  /**
> - * struct clk_wzrd:
> - * @clk_data:		Clock data
> + * struct clk_wzrd - Platform driver data for clocking wizard device
> + *
> + * @clk_data:		Clock data accessible to other devices via device tree
>   * @nb:			Notifier block
> - * @base:		Memory base
>   * @regmap:		Register map for hardware
>   * @clk_in1:		Handle to input clock 'clk_in1'
>   * @axi_clk:		Handle to input clock 's_axi_aclk'
> - * @clks_internal:	Internal clocks
>   * @clkout:		Output clocks
>   * @speed_grade:	Speed grade of the device
>   * @suspended:		Flag indicating power state of the device
> @@ -167,11 +148,9 @@ struct clk_wzrd_clk_data {
>  struct clk_wzrd {
>  	struct clk_onecell_data		clk_data;
>  	struct notifier_block		nb;
> -	void __iomem			*base;
>  	struct regmap			*regmap;
>  	struct clk			*clk_in1;
>  	struct clk			*axi_clk;
> -	struct clk			*clks_internal[wzrd_clk_int_max];
>  	struct clk			*clkout[WZRD_NUM_OUTPUTS];
>  	unsigned int			speed_grade;
>  	bool				suspended;
> @@ -179,7 +158,6 @@ struct clk_wzrd {
>  	struct clk_wzrd_clk_data	pll_data;
>  	struct clk_wzrd_clk_data	clkout_data[0];
>  };
> -
>  #define to_clk_wzrd(_nb) container_of(_nb, struct clk_wzrd, nb)

Also this is introducing new checkpatch warning.

CHECK: Please use a blank line after function/struct/union/enum declarations
#350: FILE: drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c:350:
+};
+#define to_clk_wzrd(_nb) container_of(_nb, struct clk_wzrd, nb)


M

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 07/14] staging: clocking-wizard: Add hardware constaints

[James Kelly]

Existing constraints in the driver were overly simplistic and do not
accurately represent the diversity of Clocking Wizard IP implementations.

Constant data added to express the constraints around available
rates, divider/multiplier ratios and hardware features.  These were
extracted from the relevant Xilinx documentation. Further details on the
sources used for these constraints can be found in the driver comments.

Two device tree properties were added to choose the correct constraints
for available rates, divider/multiplier ratios and hardware features.
The default for the new "xlnx,family" property is Kintex-7 and the default
for the new "xlnx,primitive" property is MMCM.  These defaults have been
chosen to match the current driver behaviour which uses the Kintex-7 rates
for speed-grade and a number of clock outputs that corresonds to an MMCM.

Speed grade now defaults to the least restrictive (fastest) speed grade.
Speed grade is now represented within the driver as an enum to enable
possible future support of non-integer speed grades, however it has not
been implemented here to avoid breaking the existing device tree bindings.

Signed-off-by: James Kelly <jamespeterkelly@gmail.com>
---
 .../clocking-wizard/clk-xlnx-clock-wizard.c        | 307 ++++++++++++++++++---
 drivers/staging/clocking-wizard/dt-binding.txt     |  12 +-
 2 files changed, 276 insertions(+), 43 deletions(-)

diff --git a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
index 1dbeda92fb9a..3e670cdc072c 100644
--- a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
+++ b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
@@ -70,12 +70,179 @@
 #include <linux/err.h>
 #include <linux/regmap.h>
 
-#define WZRD_NUM_OUTPUTS	7
-#define WZRD_ACLK_MAX_FREQ	250000000UL
+#define WZRD_MAX_OUTPUTS	7
+#define KHz			1000UL
+#define MHz			1000000UL
+#define WZRD_ACLK_MAX_FREQ	(250 * MHz)
 #define WZRD_CLKNAME_AXI	"s_axi_aclk"
 #define WZRD_CLKNAME_IN1	"clk_in1"
 #define WZRD_FLAG_MULTIPLY	BIT(0)
 
+/*
+ * Clock rate constraints extracted from Xilinx data sheets listed below.
+ * The minimum rates depend on family and clock type and the maximum rates
+ * also depend on speed grade and supply voltage.
+ *
+ * At this time the driver does not support the low voltage speed grades (-xL)
+ * and it does not model the impact of supply voltage.
+ *
+ * The constraints are enforced by the common clock framework when a client
+ * issues a request to set the rate of a clock.
+ *
+ * DS181 Artix-7 FPGAs Data Sheet: DC and AC Switching Characteristics
+ * DS182 Kintex-7 FPGAs Data Sheet: DC and AC Switching Characteristics
+ * DS183 Virtex-7 T and XT FPGAs Data Sheet: DC and AC Switching Characteristics
+ * DS187 Zynq-7000 All Programmable SoC
+ *       (Z-7007S, Z-7012S, Z-7014S, Z-7010, Z-7015, and Z-7020):
+ *       DC and AC Switching Characteristics
+ * DS191 Zynq-7000 All Programmable SoC
+ *       (Z-7030, Z-7035, Z-7045, and Z-7100):
+ *       DC and AC Switching Characteristics
+ * DS892 Kintex UltraScale FPGAs Data Sheet: DC and AC Switching Characteristics
+ * DS893 Virtex UltraScale FPGAs Data Sheet: DC and AC Switching Characteristics
+ * DS922 Kintex UltraScale+ FPGAs Data Sheet:
+ *       DC and AC Switching Characteristics
+ * DS923 Virtex UltraScale+ FPGA Data Sheet: DC and AC Switching Characteristics
+ * DS925 Zynq UltraScale+ MPSoC Data Sheet: DC and AC Switching Characteristics
+ */
+
+enum clk_wzrd_rate {
+	WZRD_RATE_FIN,		// Clock wizard input frequency
+	WZRD_RATE_PFD,		// Input to phase frequency detector
+	WZRD_RATE_VCO,		// Output of voltage controlled oscillator
+	WZRD_RATE_OUT,		// Clock wizard output frequency
+	WZRD_RATE_COUNT
+};
+
+enum clk_wzrd_speed {
+	WZRD_SPEED_1,		// Speed grade 1
+	WZRD_SPEED_2,		// Speed grade 2
+	WZRD_SPEED_3,		// Speed grade 3
+	WZRD_SPEED_COUNT
+};
+
+enum clk_wzrd_family {
+	WZRD_FAMILY_ARTIX7,		// Includes low-end Zynq-7000
+	WZRD_FAMILY_KINTEX7,		// Includes Vertex-7, high-end Zynq-7000
+	WZRD_FAMILY_ULTRASCALE,		// Kintex and Vertex Ultrascale
+	WZRD_FAMILY_ULTRASCALEPLUS,	// Kintex, Vertex, Zynq Ultrascale+
+	WZRD_FAMILY_COUNT
+};
+
+static const char *clk_wzrd_family_name[WZRD_FAMILY_COUNT] = {
+	"Artix-7",
+	"Kintex-7",
+	"Ultrascale",
+	"Ultrascale+"
+};
+
+enum clk_wzrd_type {
+	WZRD_TYPE_MMCM,		// Mixed Mode Clock Manager
+	WZRD_TYPE_PLL,		// Phased Lock Loop
+	WZRD_TYPE_COUNT
+};
+
+static const char *clk_wzrd_type_name[WZRD_TYPE_COUNT] = {
+	"MMCM",
+	"PLL"
+};
+
+enum clk_wzrd_cell {
+	WZRD_CELL_MMCME2,	// 7 series MMCM
+	WZRD_CELL_PLLE2,	// 7 series PLL
+	WZRD_CELL_MMCME3,	// Ultrascale MMCM
+	WZRD_CELL_PLLE3,	// Ultrascale PLL
+	WZRD_CELL_MMCME4,	// Ultrascale+ MMCM
+	WZRD_CELL_PLLE4,	// Ultrascale+ PLL
+	WZRD_CELL_COUNT
+};
+
+struct clk_wzrd_chip {
+	enum clk_wzrd_cell	cell;
+	int			output_count;
+	unsigned long		min[WZRD_RATE_COUNT];
+	unsigned long		max[WZRD_SPEED_COUNT][WZRD_RATE_COUNT];
+};
+
+static const struct clk_wzrd_chip
+		chip_constraints[WZRD_FAMILY_COUNT][WZRD_TYPE_COUNT] = {
+	// Artix-7 DS181
+	// Zynq-7000 DS187
+	{ { .cell = WZRD_CELL_MMCME2,
+	    .output_count = 7,
+	    //            FIN        PFD         VCO         OUT
+	    .min = {    10 * MHz,  10 * MHz,  600 * MHz, 4690 * KHz },
+	    .max = { { 800 * MHz, 450 * MHz, 1200 * MHz,  800 * MHz },
+		     { 800 * MHz, 500 * MHz, 1440 * MHz,  800 * MHz },
+		     { 800 * MHz, 550 * MHz, 1600 * MHz,  800 * MHz } } },
+	  { .cell = WZRD_CELL_PLLE2,
+	    .output_count = 6,
+	    //            FIN        PFD         VCO         OUT
+	    .min = {    19 * MHz,  19 * MHz,  800 * MHz, 6250 * KHz },
+	    .max = { { 800 * MHz, 450 * MHz, 1600 * MHz,  800 * MHz },
+		     { 800 * MHz, 500 * MHz, 1866 * MHz,  800 * MHz },
+		     { 800 * MHz, 550 * MHz, 2133 * MHz,  800 * MHz } } } },
+
+	// Kintex-7 DS182
+	// Vertex-7 DS183
+	// Zynq-7000 DS191
+	{ { .cell = WZRD_CELL_MMCME2,
+	    .output_count = 7,
+	    //             FIN        PFD         VCO         OUT
+	    .min = {     10 * MHz,  10 * MHz,  600 * MHz, 4690 * KHz },
+	    .max = { {  800 * MHz, 450 * MHz, 1200 * MHz,  800 * MHz },
+		     {  933 * MHz, 500 * MHz, 1440 * MHz,  933 * MHz },
+		     { 1066 * MHz, 550 * MHz, 1600 * MHz, 1066 * MHz } } },
+	  { .cell = WZRD_CELL_PLLE2,
+	    .output_count = 6,
+	    //             FIN        PFD         VCO         OUT
+	    .min = {     19 * MHz,  19 * MHz,  800 * MHz, 6250 * KHz },
+	    .max = { {  800 * MHz, 450 * MHz, 1600 * MHz,  800 * MHz },
+		     {  933 * MHz, 500 * MHz, 1866 * MHz,  933 * MHz },
+		     { 1066 * MHz, 550 * MHz, 2133 * MHz, 1066 * MHz } } } },
+
+	// Kintex Ultrascale DS892
+	// Vertex Ultrascale DS893
+	{ { .cell = WZRD_CELL_MMCME3,
+	    .output_count = 7,
+	    //             FIN        PFD         VCO         OUT
+	    .min = {     10 * MHz,  10 * MHz,  600 * MHz, 4690 * KHz },
+	    .max = { {  800 * MHz, 450 * MHz, 1200 * MHz,  630 * MHz },
+		     {  933 * MHz, 500 * MHz, 1440 * MHz,  725 * MHz },
+		     { 1066 * MHz, 550 * MHz, 1600 * MHz,  850 * MHz } } },
+	  { .cell = WZRD_CELL_PLLE3,
+	    .output_count = 2,
+	    //             FIN           PFD         VCO         OUT
+	    .min = {     70 * MHz,     70 * MHz,  600 * MHz, 4690 * KHz },
+	    .max = { {  800 * MHz,    600 * MHz, 1200 * MHz,  630 * MHz },
+		     {  933 * MHz, 667500 * KHz, 1335 * MHz,  725 * MHz },
+		     { 1066 * MHz, 667500 * KHz, 1335 * MHz,  850 * MHz } } } },
+
+	// Kintex Ultascale+ DS922
+	// Vertex Ultrascale+ DS923
+	// Zynq Ultrascale+ DS925
+	{ { .cell = WZRD_CELL_MMCME4,
+	    .output_count = 7,
+	    //             FIN        PFD         VCO         OUT
+	    .min = {     10 * MHz,  10 * MHz,  800 * MHz, 6250 * KHz },
+	    .max = { {  800 * MHz, 450 * MHz, 1600 * MHz,  667 * MHz },
+		     {  933 * MHz, 500 * MHz, 1600 * MHz,  725 * MHz },
+		     { 1066 * MHz, 550 * MHz, 1600 * MHz,  891 * MHz } } },
+	  { .cell = WZRD_CELL_PLLE4,
+	    .output_count = 2,
+	    //             FIN           PFD         VCO         OUT
+	    .min = {     70 * MHz,     70 * MHz,  750 * MHz, 5860 * KHz },
+	    .max = { {  800 * MHz, 667500 * KHz, 1500 * MHz,  667 * MHz },
+		     {  933 * MHz, 667500 * KHz, 1500 * MHz,  725 * MHz },
+		     { 1066 * MHz, 667500 * KHz, 1500 * MHz,  891 * MHz } } } }
+};
+
+/*
+ * Clock ratio constraints extracted from Xilinx documents listed below.
+ *
+ * UG472 7 Series FPGAs Clocking Resources User Guide
+ * UG572 UltraScale Architecture Clocking Resources User Guide
+ */
 enum clk_wzrd_clk {
 	WZRD_CLK_DIV,
 	WZRD_CLK_PLL,
@@ -83,6 +250,26 @@ enum clk_wzrd_clk {
 	WZRD_CLK_COUNT
 };
 
+struct clk_wzrd_ratio {
+	u16 min;
+	u16 max;
+};
+
+static const struct clk_wzrd_ratio
+		ratio_constraints[WZRD_CELL_COUNT][WZRD_CLK_COUNT] = {
+#define ENTRY(div_min, div_max, pll_min, pll_max, out_min, out_max) \
+	{ { .min = div_min, .max = div_max }, \
+	  { .min = pll_min, .max = pll_max }, \
+	  { .min = out_min, .max = out_max } }
+	ENTRY(1, 106, 2,  64, 1, 128),	// MMCME2 UG472
+	ENTRY(1,  56, 2,  64, 1, 128),	// PLLE2 UG472
+	ENTRY(1, 106, 2,  64, 1, 128),	// MMCME3 UG572
+	ENTRY(1,  15, 1,  19, 1, 128),	// PLLE3 UG572
+	ENTRY(1, 106, 2, 128, 1, 128),	// MMCME4 UG572
+	ENTRY(1,  15, 2,  21, 1, 128)	// PLLE4 UG572
+#undef ENTRY
+};
+
 /*
  * Register map extracted from Xilinx document listed below.
  *
@@ -98,7 +285,7 @@ static const struct reg_field clk_wzrd_status_locked = REG_FIELD(0x004,  0,  0);
 static const struct reg_field clk_wzrd_divclk_divide = REG_FIELD(0x200,  0,  7);
 static const struct reg_field clk_wzrd_clkfbout_mult = REG_FIELD(0x200,  8, 15);
 static const struct reg_field clk_wzrd_clkfbout_frac = REG_FIELD(0x200, 16, 25);
-static const struct reg_field clk_wzrd_clkout_divide[WZRD_NUM_OUTPUTS] = {
+static const struct reg_field clk_wzrd_clkout_divide[WZRD_MAX_OUTPUTS] = {
 	REG_FIELD(0x208, 0, 7),
 	REG_FIELD(0x214, 0, 7),
 	REG_FIELD(0x220, 0, 7),
@@ -143,6 +330,7 @@ struct clk_wzrd_clk_data {
  * @suspended:		Flag indicating power state of the device
  * @div:		Divider internal clock provider data
  * @pll:		Phase locked loop internal clock provider data
+ * @chip:		Chip data including rate constraints
  * @clkout_data:	Array of output clock provider data
  */
 struct clk_wzrd {
@@ -151,22 +339,16 @@ struct clk_wzrd {
 	struct regmap			*regmap;
 	struct clk			*clk_in1;
 	struct clk			*axi_clk;
-	struct clk			*clkout[WZRD_NUM_OUTPUTS];
-	unsigned int			speed_grade;
+	struct clk			*clkout[WZRD_MAX_OUTPUTS];
+	enum clk_wzrd_speed		speed_grade;
 	bool				suspended;
 	struct clk_wzrd_clk_data	div_data;
 	struct clk_wzrd_clk_data	pll_data;
+	const struct clk_wzrd_chip	*chip;
 	struct clk_wzrd_clk_data	clkout_data[0];
 };
 #define to_clk_wzrd(_nb) container_of(_nb, struct clk_wzrd, nb)
 
-/* maximum frequencies for input/output clocks per speed grade */
-static const unsigned long clk_wzrd_max_freq[] = {
-	800000000UL,
-	933000000UL,
-	1066000000UL
-};
-
 static unsigned long clk_wzrd_get_ratio(struct clk_wzrd_clk_data *cwc)
 {
 	u32 int_part;
@@ -232,7 +414,7 @@ static int clk_wzrd_register_clk(struct device *dev, const char *name,
 		parent_name = clk_hw_get_name(&cw->div_data.hw);
 		break;
 	case WZRD_CLK_OUT:
-		if (instance >= WZRD_NUM_OUTPUTS) {
+		if (instance >= cw->chip->output_count) {
 			ret = -EINVAL;
 			goto err;
 		}
@@ -269,23 +451,26 @@ static int clk_wzrd_register_clk(struct device *dev, const char *name,
 static int clk_wzrd_clk_notifier(struct notifier_block *nb, unsigned long event,
 				 void *data)
 {
-	unsigned long max;
+	unsigned long min, max;
 	struct clk_notifier_data *ndata = data;
 	struct clk_wzrd *cw = to_clk_wzrd(nb);
 
 	if (cw->suspended)
 		return NOTIFY_OK;
 
-	if (ndata->clk == cw->clk_in1)
-		max = clk_wzrd_max_freq[cw->speed_grade - 1];
-	else if (ndata->clk == cw->axi_clk)
+	if (ndata->clk == cw->clk_in1) {
+		max = cw->chip->max[cw->speed_grade][WZRD_RATE_FIN];
+		min = cw->chip->min[WZRD_RATE_FIN];
+	} else if (ndata->clk == cw->axi_clk) {
 		max = WZRD_ACLK_MAX_FREQ;
-	else
+		min = 0;
+	} else {
 		return NOTIFY_DONE;	/* should never happen */
+	}
 
 	switch (event) {
 	case PRE_RATE_CHANGE:
-		if (ndata->new_rate > max)
+		if (ndata->new_rate > max || ndata->new_rate < min)
 			return NOTIFY_BAD;
 		return NOTIFY_OK;
 	case POST_RATE_CHANGE:
@@ -318,10 +503,30 @@ static SIMPLE_DEV_PM_OPS(clk_wzrd_dev_pm_ops, clk_wzrd_suspend,
 
 static int clk_wzrd_get_device_tree_data(struct device *dev)
 {
-	int num_outputs, ret;
+	int num_outputs, ret, speed_grade;
+	enum clk_wzrd_family family;
+	enum clk_wzrd_type type;
 	struct clk_wzrd *cw;
 	struct device_node *node = dev->of_node;
 
+	ret = of_property_read_u32(node, "xlnx,family", &family);
+	if (ret) {
+		dev_warn(dev, "xlnx,family not found in device tree\n");
+		family = WZRD_FAMILY_KINTEX7;
+	} else if (family >= WZRD_FAMILY_COUNT) {
+		dev_warn(dev, "Unknown xlnx,family found in device tree\n");
+		family = WZRD_FAMILY_KINTEX7;
+	}
+
+	ret = of_property_read_u32(node, "xlnx,primitive", &type);
+	if (ret) {
+		dev_warn(dev, "xlnx,primitive not found in device tree\n");
+		type = WZRD_TYPE_MMCM;
+	} else if (type >= WZRD_TYPE_COUNT) {
+		dev_warn(dev, "Unknown xlnx,primitive found in device tree\n");
+		type = WZRD_TYPE_MMCM;
+	}
+
 	num_outputs = of_property_count_strings(node,
 						"clock-output-names");
 	if (num_outputs < 1) {
@@ -331,9 +536,9 @@ static int clk_wzrd_get_device_tree_data(struct device *dev)
 		else
 			return -EINVAL;
 	}
-	if (num_outputs > WZRD_NUM_OUTPUTS) {
+	if (num_outputs > chip_constraints[family][type].output_count) {
 		dev_warn(dev, "Too many clock output names in device tree\n");
-		num_outputs = WZRD_NUM_OUTPUTS;
+		num_outputs = chip_constraints[family][type].output_count;
 	}
 
 	cw = devm_kzalloc(dev, sizeof(*cw) + num_outputs *
@@ -341,15 +546,26 @@ static int clk_wzrd_get_device_tree_data(struct device *dev)
 	if (!cw)
 		return -ENOMEM;
 	dev_set_drvdata(dev, cw);
+	cw->chip = &chip_constraints[family][type];
 	cw->clk_data.clk_num = num_outputs;
 
-	ret = of_property_read_u32(node, "speed-grade", &cw->speed_grade);
+	/*
+	 * Xilinx device tree generator puts negative values for speed grade
+	 * in the device tree.  To handle this we define speed_grade as an
+	 * int rather than unsigned and fix up the sign.  If no speed grade
+	 * is defined or the speed grade is invalid we use the least restrictive
+	 * (fastest) speed grade.
+	 */
+	cw->speed_grade = WZRD_SPEED_3;
+	ret = of_property_read_s32(node, "speed-grade", &speed_grade);
 	if (!ret) {
-		if (cw->speed_grade < 1 || cw->speed_grade > 3) {
-			dev_warn(dev, "invalid speed grade '%d'\n",
-				 cw->speed_grade);
-			cw->speed_grade = 0;
-		}
+		if (speed_grade < 0)
+			speed_grade = -speed_grade;
+
+		if (speed_grade < 1 || speed_grade > 3)
+			dev_warn(dev, "Invalid speed grade %d\n", speed_grade);
+		else
+			cw->speed_grade = speed_grade - 1;
 	}
 
 	cw->clk_in1 = devm_clk_get(dev, WZRD_CLKNAME_IN1);
@@ -358,6 +574,12 @@ static int clk_wzrd_get_device_tree_data(struct device *dev)
 			dev_err(dev, "Clock %s not found\n", WZRD_CLKNAME_IN1);
 		return PTR_ERR(cw->clk_in1);
 	}
+	ret = clk_set_rate_range(cw->clk_in1, cw->chip->min[WZRD_RATE_FIN],
+				 cw->chip->max[cw->speed_grade][WZRD_RATE_FIN]);
+	if (ret) {
+		dev_err(dev, "Unable to constrain input clock to valid range\n");
+		return ret;
+	}
 
 	cw->axi_clk = devm_clk_get(dev, WZRD_CLKNAME_AXI);
 	if (IS_ERR(cw->axi_clk)) {
@@ -367,11 +589,16 @@ static int clk_wzrd_get_device_tree_data(struct device *dev)
 	}
 	ret = clk_set_max_rate(cw->axi_clk, WZRD_ACLK_MAX_FREQ);
 	if (ret) {
-		dev_err(dev, "Unable to set clock %s to valid range\n",
+		dev_err(dev, "Unable to constrain clock %s to valid range\n",
 			WZRD_CLKNAME_AXI);
 		return ret;
 	}
 
+	dev_info(dev, "Chip Family: %s, Type: %s, Speed Grade: %d\n",
+		 clk_wzrd_family_name[family],
+		 clk_wzrd_type_name[type],
+		 cw->speed_grade + 1);
+
 	return 0;
 }
 
@@ -441,17 +668,15 @@ static int clk_wzrd_register_ccf(struct device *dev)
 	of_clk_add_provider(dev->of_node, of_clk_src_onecell_get,
 			    &cw->clk_data);
 
-	if (cw->speed_grade) {
-		cw->nb.notifier_call = clk_wzrd_clk_notifier;
+	cw->nb.notifier_call = clk_wzrd_clk_notifier;
 
-		ret = clk_notifier_register(cw->clk_in1, &cw->nb);
-			if (ret)
-				dev_warn(dev, "Unable to register input clock notifier\n");
+	ret = clk_notifier_register(cw->clk_in1, &cw->nb);
+	if (ret)
+		dev_warn(dev, "Unable to register input clock notifier\n");
 
-		ret = clk_notifier_register(cw->axi_clk, &cw->nb);
-			if (ret)
-				dev_warn(dev, "Unable to register AXI clock notifier\n");
-	}
+	ret = clk_notifier_register(cw->axi_clk, &cw->nb);
+	if (ret)
+		dev_warn(dev, "Unable to register AXI clock notifier\n");
 
 	ret = 0;
 
@@ -489,10 +714,8 @@ static int clk_wzrd_remove(struct platform_device *pdev)
 
 	of_clk_del_provider(pdev->dev.of_node);
 
-	if (cw->speed_grade) {
-		clk_notifier_unregister(cw->axi_clk, &cw->nb);
-		clk_notifier_unregister(cw->clk_in1, &cw->nb);
-	}
+	clk_notifier_unregister(cw->axi_clk, &cw->nb);
+	clk_notifier_unregister(cw->clk_in1, &cw->nb);
 
 	return 0;
 }
diff --git a/drivers/staging/clocking-wizard/dt-binding.txt b/drivers/staging/clocking-wizard/dt-binding.txt
index 723271e93316..6e7859a6e751 100644
--- a/drivers/staging/clocking-wizard/dt-binding.txt
+++ b/drivers/staging/clocking-wizard/dt-binding.txt
@@ -15,7 +15,15 @@ Required properties:
  - clock-output-names: Names for the output clocks
 
 Optional properties:
- - speed-grade: Speed grade of the device (valid values are 1..3)
+ - speed-grade: Speed grade of the device. Valid values are 1..3, default is 3
+ - xlnx,family: Device family
+	0 - Artix-7 and low-end Zynq-7000
+	1 - Kintex-7, Vertex-7 and high-end Zynq-7000 (default)
+	2 - Kintex, Vertex Ultrascale
+	3 - Kintex, Vertex, Zynq Ultrascale+
+ - xlnx,primitive: Clocking Wizard primitive
+	0 - MMCM (default)
+	1 - PLL
 
 Example:
 	clock-generator@40040000 {
@@ -27,4 +35,6 @@ Example:
 		clock-output-names = "clk_out0", "clk_out1", "clk_out2",
 				     "clk_out3", "clk_out4", "clk_out5",
 				     "clk_out6", "clk_out7";
+		xlnx,family = <0>;
+		xlnx,primitive = <0>;
 	};
-- 
2.15.1 (Apple Git-101)

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 08/14] staging: clocking-wizard: Support fractional ratios

[James Kelly]

Update clock provider to support fraction divider and multiplier ratios.
Ratios are now fixed point unsigned numbers with the lower WZRD_FRAC_BITS
containing the fractional part of the ratio.

Fractional ratios are only supported on MMCM primitives and only for the
PLL multiplier and first output divider.

Use DIV_ROUND_CLOSEST_ULL for division to provide best precision.

Signed-off-by: James Kelly <jamespeterkelly@gmail.com>
---
 drivers/staging/clocking-wizard/TODO               |  5 +-
 .../clocking-wizard/clk-xlnx-clock-wizard.c        | 68 ++++++++++++++++++++--
 2 files changed, 66 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/clocking-wizard/TODO b/drivers/staging/clocking-wizard/TODO
index ebe99db7d153..53c9941fcc35 100644
--- a/drivers/staging/clocking-wizard/TODO
+++ b/drivers/staging/clocking-wizard/TODO
@@ -1,11 +1,10 @@
 TODO:
-	- support for fractional multiplier
-	- support for fractional divider (output 0 only)
 	- support for set_rate() operations (may benefit from Stephen Boyd's
 	  refactoring of the clk primitives: https://lkml.org/lkml/2014/9/5/766)
 	- review arithmetic
 	  - overflow after multiplication?
-	  - maximize accuracy before divisions
+	- test on 64-bit ARM and Microblaze architectures.
+	- support clk_set_phase
 
 Patches to:
 	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
diff --git a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
index 3e670cdc072c..c892c0d46801 100644
--- a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
+++ b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
@@ -74,9 +74,13 @@
 #define KHz			1000UL
 #define MHz			1000000UL
 #define WZRD_ACLK_MAX_FREQ	(250 * MHz)
+#define WZRD_FRAC_BITS		3
+#define WZRD_FRAC_MASK		(BIT(WZRD_FRAC_BITS) - 1)
+#define WZRD_FRAC_SCALE		(1000 >> WZRD_FRAC_BITS)
 #define WZRD_CLKNAME_AXI	"s_axi_aclk"
 #define WZRD_CLKNAME_IN1	"clk_in1"
 #define WZRD_FLAG_MULTIPLY	BIT(0)
+#define WZRD_FLAG_FRAC		BIT(1)
 
 /*
  * Clock rate constraints extracted from Xilinx data sheets listed below.
@@ -304,14 +308,17 @@ static const struct reg_field clk_wzrd_reconfig     = REG_FIELD(0x25C, 0,  1);
  * @hw:			handle between common and hardware-specific interfaces
  * @flags:		hardware specific flags
  * @int_field:		pointer to regmap field for integer part
+ * @frac_field:		pointer to regmap field for fractional part
  *
  * Flags:
  * WZRD_FLAG_MULTIPLY	Clock is a multiplier rather than a divider
+ * WZRD_FLAG_FRAC	Clock ratio can be fractional
  */
 struct clk_wzrd_clk_data {
 	struct clk_hw			hw;
 	unsigned long			flags;
 	struct regmap_field		*int_field;
+	struct regmap_field		*frac_field;
 };
 
 #define to_clk_wzrd_clk_data(_hw) \
@@ -349,23 +356,56 @@ struct clk_wzrd {
 };
 #define to_clk_wzrd(_nb) container_of(_nb, struct clk_wzrd, nb)
 
+/*
+ * The following functions use or generate fixed point ratios
+ *
+ * The lower WZRD_FRAC_BITS of fixed point ratios are the fractional
+ * part and the remaining bits are the integer part.
+ *
+ * When doing division we make sure to round once only and at
+ * ether the decimal point or the end of the fixed point number
+ * depending on whether the hardware can multiply/divide by only
+ * integer ratios, or can multiply/divide by fixed point ratios.
+ */
+
 static unsigned long clk_wzrd_get_ratio(struct clk_wzrd_clk_data *cwc)
 {
 	u32 int_part;
+	unsigned long ratio;
 
 	regmap_field_read(cwc->int_field, &int_part);
+	ratio = int_part << WZRD_FRAC_BITS;
+
+	if (cwc->flags & WZRD_FLAG_FRAC) {
+		u32 frac_part;
 
-	return int_part;
+		regmap_field_read(cwc->frac_field, &frac_part);
+		ratio |= frac_part / WZRD_FRAC_SCALE;
+	}
+
+	return ratio;
 }
 
 static unsigned long clk_wzrd_calc_rate(unsigned long parent_rate,
 					unsigned long ratio,
 					unsigned long flags)
 {
-	if (flags & WZRD_FLAG_MULTIPLY)
-		return parent_rate * ratio;
+	unsigned long long t;
 
-	return DIV_ROUND_CLOSEST(parent_rate, ratio);
+	if (flags & WZRD_FLAG_MULTIPLY) {
+		t = (unsigned long long)parent_rate * ratio;
+		return (unsigned long)DIV_ROUND_CLOSEST_ULL(t, BIT(WZRD_FRAC_BITS));
+	}
+
+	if (flags & WZRD_FLAG_FRAC) {
+		/* Round at least significant bit */
+		t = (unsigned long long)parent_rate << WZRD_FRAC_BITS;
+		return (unsigned long)DIV_ROUND_CLOSEST_ULL(t, ratio);
+	}
+
+	/* Round at decimal point */
+	t = (unsigned long long)parent_rate;
+	return (unsigned long)DIV_ROUND_CLOSEST_ULL(t, ratio >> WZRD_FRAC_BITS);
 }
 
 static unsigned long clk_wzrd_recalc_rate(struct clk_hw *hw,
@@ -394,6 +434,7 @@ static int clk_wzrd_register_clk(struct device *dev, const char *name,
 	int ret;
 	struct clk_init_data init;
 	const struct reg_field *int_reg_field;
+	const struct reg_field *frac_reg_field;
 	struct clk_wzrd_clk_data *cwc;
 	const char *parent_name;
 	struct clk_wzrd *cw = dev_get_drvdata(dev);
@@ -411,6 +452,10 @@ static int clk_wzrd_register_clk(struct device *dev, const char *name,
 		cwc = &cw->pll_data;
 		cwc->flags |= WZRD_FLAG_MULTIPLY;
 		int_reg_field = &clk_wzrd_clkfbout_mult;
+		if (cw->chip->cell % 2 == 0) {
+			frac_reg_field = &clk_wzrd_clkfbout_frac;
+			cwc->flags |= WZRD_FLAG_FRAC;
+		}
 		parent_name = clk_hw_get_name(&cw->div_data.hw);
 		break;
 	case WZRD_CLK_OUT:
@@ -419,6 +464,12 @@ static int clk_wzrd_register_clk(struct device *dev, const char *name,
 			goto err;
 		}
 		cwc = &cw->clkout_data[instance];
+		if (instance == 0) {
+			if (cw->chip->cell % 2 == 0) {
+				cwc->flags |= WZRD_FLAG_FRAC;
+				frac_reg_field = &clk_wzrd_clkout0_frac;
+			}
+		}
 		int_reg_field = &clk_wzrd_clkout_divide[instance];
 		parent_name = clk_hw_get_name(&cw->pll_data.hw);
 		break;
@@ -438,6 +489,15 @@ static int clk_wzrd_register_clk(struct device *dev, const char *name,
 		goto err;
 	}
 
+	if (cwc->flags & WZRD_FLAG_FRAC) {
+		cwc->frac_field = devm_regmap_field_alloc(dev, cw->regmap,
+							  *frac_reg_field);
+		if (IS_ERR(cwc->frac_field)) {
+			ret = PTR_ERR(cwc->frac_field);
+			goto err;
+		}
+	}
+
 	ret = devm_clk_hw_register(dev, &cwc->hw);
 	if (ret)
 		goto err;
-- 
2.15.1 (Apple Git-101)

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 08/14] staging: clocking-wizard: Support fractional ratios

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3999 bytes --]

Hi James,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on staging/staging-testing]
[also build test WARNING on v4.17-rc3 next-20180504]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/James-Kelly/staging-clocking-wizard-Implement-many-TODOs/20180507-102148
config: x86_64-randconfig-x009-201818 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   drivers/staging//clocking-wizard/clk-xlnx-clock-wizard.c: In function 'clk_wzrd_register_clk.constprop':
>> drivers/staging//clocking-wizard/clk-xlnx-clock-wizard.c:493:21: warning: 'frac_reg_field' may be used uninitialized in this function [-Wmaybe-uninitialized]
      cwc->frac_field = devm_regmap_field_alloc(dev, cw->regmap,
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             *frac_reg_field);
             ~~~~~~~~~~~~~~~~

vim +/frac_reg_field +493 drivers/staging//clocking-wizard/clk-xlnx-clock-wizard.c

   429	
   430	static int clk_wzrd_register_clk(struct device *dev, const char *name,
   431					 enum clk_wzrd_clk type, unsigned int instance,
   432					 unsigned long flags)
   433	{
   434		int ret;
   435		struct clk_init_data init;
   436		const struct reg_field *int_reg_field;
   437		const struct reg_field *frac_reg_field;
   438		struct clk_wzrd_clk_data *cwc;
   439		const char *parent_name;
   440		struct clk_wzrd *cw = dev_get_drvdata(dev);
   441	
   442		init.ops = &clk_wzrd_clk_ops;
   443		init.flags = flags;
   444	
   445		switch (type) {
   446		case WZRD_CLK_DIV:
   447			cwc = &cw->div_data;
   448			int_reg_field = &clk_wzrd_divclk_divide;
   449			parent_name = __clk_get_name(cw->clk_in1);
   450			break;
   451		case WZRD_CLK_PLL:
   452			cwc = &cw->pll_data;
   453			cwc->flags |= WZRD_FLAG_MULTIPLY;
   454			int_reg_field = &clk_wzrd_clkfbout_mult;
   455			if (cw->chip->cell % 2 == 0) {
   456				frac_reg_field = &clk_wzrd_clkfbout_frac;
   457				cwc->flags |= WZRD_FLAG_FRAC;
   458			}
   459			parent_name = clk_hw_get_name(&cw->div_data.hw);
   460			break;
   461		case WZRD_CLK_OUT:
   462			if (instance >= cw->chip->output_count) {
   463				ret = -EINVAL;
   464				goto err;
   465			}
   466			cwc = &cw->clkout_data[instance];
   467			if (instance == 0) {
   468				if (cw->chip->cell % 2 == 0) {
   469					cwc->flags |= WZRD_FLAG_FRAC;
   470					frac_reg_field = &clk_wzrd_clkout0_frac;
   471				}
   472			}
   473			int_reg_field = &clk_wzrd_clkout_divide[instance];
   474			parent_name = clk_hw_get_name(&cw->pll_data.hw);
   475			break;
   476		default:
   477			ret = -EINVAL;
   478			goto err;
   479		}
   480	
   481		init.name = name;
   482		init.parent_names = &parent_name;
   483		init.num_parents = 1;
   484		cwc->hw.init = &init;
   485		cwc->int_field = devm_regmap_field_alloc(dev, cw->regmap,
   486							 *int_reg_field);
   487		if (IS_ERR(cwc->int_field)) {
   488			ret = PTR_ERR(cwc->int_field);
   489			goto err;
   490		}
   491	
   492		if (cwc->flags & WZRD_FLAG_FRAC) {
 > 493			cwc->frac_field = devm_regmap_field_alloc(dev, cw->regmap,
   494								  *frac_reg_field);
   495			if (IS_ERR(cwc->frac_field)) {
   496				ret = PTR_ERR(cwc->frac_field);
   497				goto err;
   498			}
   499		}
   500	
   501		ret = devm_clk_hw_register(dev, &cwc->hw);
   502		if (ret)
   503			goto err;
   504	
   505		return 0;
   506	err:
   507		dev_err(dev, "Unable to register component clock %s\n", name);
   508		return ret;
   509	}
   510	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29802 bytes --]

[-- Attachment #3: Type: text/plain, Size: 169 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 09/14] staging: clocking-wizard: Provide more information in debugfs

[James Kelly]

Publish clock divider/multiplier ratios and flags specific to our
clock provider implementation as these are not available via the
debugfs entries provided by the common clock framework.

Signed-off-by: James Kelly <jamespeterkelly@gmail.com>
---
 .../clocking-wizard/clk-xlnx-clock-wizard.c        | 57 ++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
index c892c0d46801..8929913045e7 100644
--- a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
+++ b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
@@ -69,6 +69,8 @@
 #include <linux/module.h>
 #include <linux/err.h>
 #include <linux/regmap.h>
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
 
 #define WZRD_MAX_OUTPUTS	7
 #define KHz			1000UL
@@ -423,8 +425,63 @@ static unsigned long clk_wzrd_recalc_rate(struct clk_hw *hw,
 	return clk_wzrd_calc_rate(parent_rate, ratio, cwc->flags);
 }
 
+#ifdef CONFIG_DEBUG_FS
+
+static int clk_wzrd_flags_show(struct seq_file *s, void *data)
+{
+	struct clk_wzrd_clk_data *cwc = s->private;
+
+	seq_puts(s, "Flags:\n");
+	if (cwc->flags & WZRD_FLAG_MULTIPLY)
+		seq_puts(s, "WZRD_FLAG_MULTIPLY\n");
+	if (cwc->flags & WZRD_FLAG_FRAC)
+		seq_puts(s, "WZRD_FLAG_FRAC\n");
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(clk_wzrd_flags);
+
+static int clk_wzrd_ratio_show(struct seq_file *s, void *data)
+{
+	struct clk_wzrd_clk_data *cwc = s->private;
+	unsigned int int_part, frac_part = 0;
+
+	regmap_field_read(cwc->int_field, &int_part);
+	if (cwc->flags & WZRD_FLAG_FRAC) {
+		regmap_field_read(cwc->frac_field, &frac_part);
+		seq_printf(s, "%u.%u\n", int_part, frac_part);
+	} else {
+		seq_printf(s, "%u\n", int_part);
+	}
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(clk_wzrd_ratio);
+
+static int clk_wzrd_debug_init(struct clk_hw *hw, struct dentry *dentry)
+{
+	struct dentry *d;
+	struct clk_wzrd_clk_data *cwc = to_clk_wzrd_clk_data(hw);
+
+	d = debugfs_create_file_unsafe("clk_hw_flags", 0444, dentry, cwc,
+				       &clk_wzrd_flags_fops);
+	if (IS_ERR(d))
+		return PTR_ERR(d);
+
+	d = debugfs_create_file_unsafe("clk_ratio", 0444, dentry, cwc,
+				       &clk_wzrd_ratio_fops);
+	if (IS_ERR(d))
+		return PTR_ERR(d);
+
+	return 0;
+}
+#endif
+
 static const struct clk_ops clk_wzrd_clk_ops = {
 	.recalc_rate = clk_wzrd_recalc_rate,
+#ifdef CONFIG_DEBUG_FS
+	.debug_init = clk_wzrd_debug_init,
+#endif
 };
 
 static int clk_wzrd_register_clk(struct device *dev, const char *name,
-- 
2.15.1 (Apple Git-101)

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 09/14] staging: clocking-wizard: Provide more information in debugfs

[Michal Simek]

On 7.5.2018 03:20, James Kelly wrote:
> Publish clock divider/multiplier ratios and flags specific to our
> clock provider implementation as these are not available via the
> debugfs entries provided by the common clock framework.
> 
> Signed-off-by: James Kelly <jamespeterkelly@gmail.com>
> ---
>  .../clocking-wizard/clk-xlnx-clock-wizard.c        | 57 ++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
> 
> diff --git a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> index c892c0d46801..8929913045e7 100644
> --- a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> +++ b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> @@ -69,6 +69,8 @@
>  #include <linux/module.h>
>  #include <linux/err.h>
>  #include <linux/regmap.h>
> +#include <linux/debugfs.h>
> +#include <linux/seq_file.h>
>  
>  #define WZRD_MAX_OUTPUTS	7
>  #define KHz			1000UL
> @@ -423,8 +425,63 @@ static unsigned long clk_wzrd_recalc_rate(struct clk_hw *hw,
>  	return clk_wzrd_calc_rate(parent_rate, ratio, cwc->flags);
>  }
>  
> +#ifdef CONFIG_DEBUG_FS
> +
> +static int clk_wzrd_flags_show(struct seq_file *s, void *data)
> +{
> +	struct clk_wzrd_clk_data *cwc = s->private;
> +
> +	seq_puts(s, "Flags:\n");
> +	if (cwc->flags & WZRD_FLAG_MULTIPLY)
> +		seq_puts(s, "WZRD_FLAG_MULTIPLY\n");
> +	if (cwc->flags & WZRD_FLAG_FRAC)
> +		seq_puts(s, "WZRD_FLAG_FRAC\n");
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(clk_wzrd_flags);
> +
> +static int clk_wzrd_ratio_show(struct seq_file *s, void *data)
> +{
> +	struct clk_wzrd_clk_data *cwc = s->private;
> +	unsigned int int_part, frac_part = 0;
> +
> +	regmap_field_read(cwc->int_field, &int_part);
> +	if (cwc->flags & WZRD_FLAG_FRAC) {
> +		regmap_field_read(cwc->frac_field, &frac_part);
> +		seq_printf(s, "%u.%u\n", int_part, frac_part);
> +	} else {
> +		seq_printf(s, "%u\n", int_part);
> +	}
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(clk_wzrd_ratio);
> +
> +static int clk_wzrd_debug_init(struct clk_hw *hw, struct dentry *dentry)
> +{
> +	struct dentry *d;
> +	struct clk_wzrd_clk_data *cwc = to_clk_wzrd_clk_data(hw);
> +
> +	d = debugfs_create_file_unsafe("clk_hw_flags", 0444, dentry, cwc,
> +				       &clk_wzrd_flags_fops);
> +	if (IS_ERR(d))
> +		return PTR_ERR(d);
> +
> +	d = debugfs_create_file_unsafe("clk_ratio", 0444, dentry, cwc,
> +				       &clk_wzrd_ratio_fops);
> +	if (IS_ERR(d))
> +		return PTR_ERR(d);

Would be good to keep coccinelle happy.

drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c:735:1-3:
WARNING: PTR_ERR_OR_ZERO can be used

Thanks,
Michal
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 10/14] staging: clocking-wizard: Support clk_round_rate

[James Kelly]

Add support for the clk_round_rate API to our CCF clock provider.

Signed-off-by: James Kelly <jamespeterkelly@gmail.com>
---
 .../clocking-wizard/clk-xlnx-clock-wizard.c        | 107 +++++++++++++++++++++
 1 file changed, 107 insertions(+)

diff --git a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
index 8929913045e7..8828dac6faaf 100644
--- a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
+++ b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
@@ -83,6 +83,7 @@
 #define WZRD_CLKNAME_IN1	"clk_in1"
 #define WZRD_FLAG_MULTIPLY	BIT(0)
 #define WZRD_FLAG_FRAC		BIT(1)
+#define WZRD_FLAG_ADJUST_MIN	BIT(2)
 
 /*
  * Clock rate constraints extracted from Xilinx data sheets listed below.
@@ -309,16 +310,19 @@ static const struct reg_field clk_wzrd_reconfig     = REG_FIELD(0x25C, 0,  1);
  *
  * @hw:			handle between common and hardware-specific interfaces
  * @flags:		hardware specific flags
+ * @ratio_limit:	pointer to divider/multiplier ratio limits
  * @int_field:		pointer to regmap field for integer part
  * @frac_field:		pointer to regmap field for fractional part
  *
  * Flags:
  * WZRD_FLAG_MULTIPLY	Clock is a multiplier rather than a divider
  * WZRD_FLAG_FRAC	Clock ratio can be fractional
+ * WZRD_FLAG_ADJUST_MIN	When clock is fractional minimum ratio increases by 1
  */
 struct clk_wzrd_clk_data {
 	struct clk_hw			hw;
 	unsigned long			flags;
+	const struct clk_wzrd_ratio	*ratio_limit;
 	struct regmap_field		*int_field;
 	struct regmap_field		*frac_field;
 };
@@ -425,6 +429,86 @@ static unsigned long clk_wzrd_recalc_rate(struct clk_hw *hw,
 	return clk_wzrd_calc_rate(parent_rate, ratio, cwc->flags);
 }
 
+static unsigned long clk_wzrd_calc_ratio(unsigned long parent_rate,
+					 unsigned long req_rate,
+					 unsigned long flags)
+{
+	unsigned long long t;
+	unsigned long n, d;
+
+	if (flags & WZRD_FLAG_MULTIPLY) {
+		n = req_rate;
+		d = parent_rate;
+	} else {
+		n = parent_rate;
+		d = req_rate;
+	}
+
+	if (flags & WZRD_FLAG_FRAC) {
+		/* Round at least significant bit */
+		t = (unsigned long long)n << WZRD_FRAC_BITS;
+		return (unsigned long)DIV_ROUND_CLOSEST_ULL(t, d);
+	}
+
+	/* Round at decimal point */
+	t = (unsigned long long)n;
+	return (unsigned long)DIV_ROUND_CLOSEST_ULL(t, d) << WZRD_FRAC_BITS;
+}
+
+static unsigned long clk_wzrd_limit_calc_ratio(struct clk_wzrd_clk_data *cwc,
+					       unsigned long parent_rate,
+					       unsigned long req_rate)
+{
+	unsigned long ratio;
+
+	ratio = clk_wzrd_calc_ratio(parent_rate, req_rate, cwc->flags);
+
+	/*
+	 * Some fractional multiple/divide hardware cannot do fractional
+	 * multiply/divide between the minimum ratio limit and the
+	 * minimum ratio limit + 1, as indicated by WZRD_FLAG_ADJUST_MIN.
+	 * If we get a ratio in this range we have to recalculate the
+	 * ratio so it is not fractional and rounded to an integer.
+	 */
+	if (ratio >> WZRD_FRAC_BITS == cwc->ratio_limit->min &&
+	    cwc->flags & WZRD_FLAG_ADJUST_MIN && cwc->flags & WZRD_FLAG_FRAC)
+		ratio = clk_wzrd_calc_ratio(parent_rate, req_rate,
+					    cwc->flags & ~WZRD_FLAG_FRAC);
+
+	ratio = clamp_val(ratio, cwc->ratio_limit->min << WZRD_FRAC_BITS,
+			  cwc->ratio_limit->max << WZRD_FRAC_BITS);
+
+	return ratio;
+}
+
+static inline unsigned long clk_wzrd_round_rate(struct clk_wzrd_clk_data *cwc,
+						unsigned long parent_rate,
+						unsigned long req_rate)
+{
+	unsigned long ratio = clk_wzrd_limit_calc_ratio(cwc, parent_rate,
+							req_rate);
+	return clk_wzrd_calc_rate(parent_rate, ratio, cwc->flags);
+}
+
+static int clk_wzrd_determine_rate(struct clk_hw *hw,
+				   struct clk_rate_request *req)
+{
+	struct clk_wzrd_clk_data *cwc = to_clk_wzrd_clk_data(hw);
+
+	if (cwc->flags & WZRD_FLAG_MULTIPLY && req->best_parent_rate == 0)
+		return -EINVAL;
+
+	if (!(cwc->flags & WZRD_FLAG_MULTIPLY) && req->rate == 0)
+		return -EINVAL;
+
+	req->rate = clk_wzrd_round_rate(cwc, req->best_parent_rate, req->rate);
+
+	if (req->rate < req->min_rate || req->rate > req->max_rate)
+		return -EINVAL;
+
+	return 0;
+}
+
 #ifdef CONFIG_DEBUG_FS
 
 static int clk_wzrd_flags_show(struct seq_file *s, void *data)
@@ -436,6 +520,8 @@ static int clk_wzrd_flags_show(struct seq_file *s, void *data)
 		seq_puts(s, "WZRD_FLAG_MULTIPLY\n");
 	if (cwc->flags & WZRD_FLAG_FRAC)
 		seq_puts(s, "WZRD_FLAG_FRAC\n");
+	if (cwc->flags & WZRD_FLAG_ADJUST_MIN)
+		seq_puts(s, "WZRD_FLAG_ADJUST_MIN\n");
 
 	return 0;
 }
@@ -463,6 +549,16 @@ static int clk_wzrd_debug_init(struct clk_hw *hw, struct dentry *dentry)
 	struct dentry *d;
 	struct clk_wzrd_clk_data *cwc = to_clk_wzrd_clk_data(hw);
 
+	d = debugfs_create_u16("clk_ratio_min", 0444, dentry,
+			       (u16 *)&cwc->ratio_limit->min);
+	if (IS_ERR(d))
+		return PTR_ERR(d);
+
+	d = debugfs_create_u16("clk_ratio_max", 0444, dentry,
+			       (u16 *)&cwc->ratio_limit->max);
+	if (IS_ERR(d))
+		return PTR_ERR(d);
+
 	d = debugfs_create_file_unsafe("clk_hw_flags", 0444, dentry, cwc,
 				       &clk_wzrd_flags_fops);
 	if (IS_ERR(d))
@@ -479,6 +575,7 @@ static int clk_wzrd_debug_init(struct clk_hw *hw, struct dentry *dentry)
 
 static const struct clk_ops clk_wzrd_clk_ops = {
 	.recalc_rate = clk_wzrd_recalc_rate,
+	.determine_rate = clk_wzrd_determine_rate,
 #ifdef CONFIG_DEBUG_FS
 	.debug_init = clk_wzrd_debug_init,
 #endif
@@ -492,6 +589,7 @@ static int clk_wzrd_register_clk(struct device *dev, const char *name,
 	struct clk_init_data init;
 	const struct reg_field *int_reg_field;
 	const struct reg_field *frac_reg_field;
+	enum clk_wzrd_rate rate_constraint;
 	struct clk_wzrd_clk_data *cwc;
 	const char *parent_name;
 	struct clk_wzrd *cw = dev_get_drvdata(dev);
@@ -503,6 +601,7 @@ static int clk_wzrd_register_clk(struct device *dev, const char *name,
 	case WZRD_CLK_DIV:
 		cwc = &cw->div_data;
 		int_reg_field = &clk_wzrd_divclk_divide;
+		rate_constraint = WZRD_RATE_PFD;
 		parent_name = __clk_get_name(cw->clk_in1);
 		break;
 	case WZRD_CLK_PLL:
@@ -513,6 +612,7 @@ static int clk_wzrd_register_clk(struct device *dev, const char *name,
 			frac_reg_field = &clk_wzrd_clkfbout_frac;
 			cwc->flags |= WZRD_FLAG_FRAC;
 		}
+		rate_constraint = WZRD_RATE_VCO;
 		parent_name = clk_hw_get_name(&cw->div_data.hw);
 		break;
 	case WZRD_CLK_OUT:
@@ -524,10 +624,12 @@ static int clk_wzrd_register_clk(struct device *dev, const char *name,
 		if (instance == 0) {
 			if (cw->chip->cell % 2 == 0) {
 				cwc->flags |= WZRD_FLAG_FRAC;
+				cwc->flags |= WZRD_FLAG_ADJUST_MIN;
 				frac_reg_field = &clk_wzrd_clkout0_frac;
 			}
 		}
 		int_reg_field = &clk_wzrd_clkout_divide[instance];
+		rate_constraint = WZRD_RATE_OUT;
 		parent_name = clk_hw_get_name(&cw->pll_data.hw);
 		break;
 	default:
@@ -539,6 +641,7 @@ static int clk_wzrd_register_clk(struct device *dev, const char *name,
 	init.parent_names = &parent_name;
 	init.num_parents = 1;
 	cwc->hw.init = &init;
+	cwc->ratio_limit = &ratio_constraints[cw->chip->cell][type];
 	cwc->int_field = devm_regmap_field_alloc(dev, cw->regmap,
 						 *int_reg_field);
 	if (IS_ERR(cwc->int_field)) {
@@ -559,6 +662,10 @@ static int clk_wzrd_register_clk(struct device *dev, const char *name,
 	if (ret)
 		goto err;
 
+	clk_hw_set_rate_range(&cwc->hw,
+			      cw->chip->min[rate_constraint],
+			      cw->chip->max[cw->speed_grade][rate_constraint]);
+
 	return 0;
 err:
 	dev_err(dev, "Unable to register component clock %s\n", name);
-- 
2.15.1 (Apple Git-101)

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 10/14] staging: clocking-wizard: Support clk_round_rate

[Michal Simek]

On 7.5.2018 03:20, James Kelly wrote:
> Add support for the clk_round_rate API to our CCF clock provider.
> 
> Signed-off-by: James Kelly <jamespeterkelly@gmail.com>
> ---
>  .../clocking-wizard/clk-xlnx-clock-wizard.c        | 107 +++++++++++++++++++++
>  1 file changed, 107 insertions(+)
> 
> diff --git a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> index 8929913045e7..8828dac6faaf 100644
> --- a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> +++ b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> @@ -83,6 +83,7 @@
>  #define WZRD_CLKNAME_IN1	"clk_in1"
>  #define WZRD_FLAG_MULTIPLY	BIT(0)
>  #define WZRD_FLAG_FRAC		BIT(1)
> +#define WZRD_FLAG_ADJUST_MIN	BIT(2)
>  
>  /*
>   * Clock rate constraints extracted from Xilinx data sheets listed below.
> @@ -309,16 +310,19 @@ static const struct reg_field clk_wzrd_reconfig     = REG_FIELD(0x25C, 0,  1);
>   *
>   * @hw:			handle between common and hardware-specific interfaces
>   * @flags:		hardware specific flags
> + * @ratio_limit:	pointer to divider/multiplier ratio limits
>   * @int_field:		pointer to regmap field for integer part
>   * @frac_field:		pointer to regmap field for fractional part
>   *
>   * Flags:
>   * WZRD_FLAG_MULTIPLY	Clock is a multiplier rather than a divider
>   * WZRD_FLAG_FRAC	Clock ratio can be fractional
> + * WZRD_FLAG_ADJUST_MIN	When clock is fractional minimum ratio increases by 1


If you look at this as man page
 ./scripts/kernel-doc -man -v
drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c | nroff -man | less

Then you will get:

Description
       Flags: WZRD_FLAG_MULTIPLY   Clock is a multiplier rather than a
divider
       WZRD_FLAG_FRAC       Clock ratio can be fractional
WZRD_FLAG_ADJUST_MIN
       When clock is fractional minimum ratio increases by 1


Which doesn't look nice.


Thanks,
Michal
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 10/14] staging: clocking-wizard: Support clk_round_rate

[Dan Carpenter]

On Mon, May 07, 2018 at 11:20:36AM +1000, James Kelly wrote:
> @@ -463,6 +549,16 @@ static int clk_wzrd_debug_init(struct clk_hw *hw, struct dentry *dentry)
>  	struct dentry *d;
>  	struct clk_wzrd_clk_data *cwc = to_clk_wzrd_clk_data(hw);
>  
> +	d = debugfs_create_u16("clk_ratio_min", 0444, dentry,
> +			       (u16 *)&cwc->ratio_limit->min);
> +	if (IS_ERR(d))
> +		return PTR_ERR(d);
> +
> +	d = debugfs_create_u16("clk_ratio_max", 0444, dentry,
> +			       (u16 *)&cwc->ratio_limit->max);
> +	if (IS_ERR(d))
> +		return PTR_ERR(d);


All these debugfs stuff should be tests for NULL.

 *
 * If debugfs is not enabled in the kernel, the value -%ENODEV will be
 * returned.  It is not wise to check for this value, but rather, check for
 * %NULL or !%NULL instead as to eliminate the need for #ifdef in the calling
 * code.

regards,
dan carpenter

Re: [PATCH 10/14] staging: clocking-wizard: Support clk_round_rate

[Greg Kroah-Hartman]

On Tue, May 15, 2018 at 10:52:58AM +0300, Dan Carpenter wrote:
> On Mon, May 07, 2018 at 11:20:36AM +1000, James Kelly wrote:
> > @@ -463,6 +549,16 @@ static int clk_wzrd_debug_init(struct clk_hw *hw, struct dentry *dentry)
> >  	struct dentry *d;
> >  	struct clk_wzrd_clk_data *cwc = to_clk_wzrd_clk_data(hw);
> >  
> > +	d = debugfs_create_u16("clk_ratio_min", 0444, dentry,
> > +			       (u16 *)&cwc->ratio_limit->min);
> > +	if (IS_ERR(d))
> > +		return PTR_ERR(d);
> > +
> > +	d = debugfs_create_u16("clk_ratio_max", 0444, dentry,
> > +			       (u16 *)&cwc->ratio_limit->max);
> > +	if (IS_ERR(d))
> > +		return PTR_ERR(d);
> 
> 
> All these debugfs stuff should be tests for NULL.
> 
>  *
>  * If debugfs is not enabled in the kernel, the value -%ENODEV will be
>  * returned.  It is not wise to check for this value, but rather, check for
>  * %NULL or !%NULL instead as to eliminate the need for #ifdef in the calling
>  * code.

No, you should not check for anything, debugfs calls can work just fine
by ignoring the return value.  You should not care what debugfs does, or
does not do, when you call it.

I should update the documentation a bit better, but no one seems to even
read it anyway :(

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 11/14] staging: clocking-wizard: Support clk_set_rate

[James Kelly]

Provide initial support for CCF clk_set_rate API on all clock components.

Clock consumers that want to set the first divider or PLL clock will need
to use clk_get_parent on one of the output clocks as there is no support
for CLK_SET_RATE_PARENT yet.

Care must be taken when setting the first divider clock to ensure that
the PLL clock rate will remain within a valid range for the VCO, as it
is impossible to subsequently update any clock if the PLL does not lock.
A subsequent patch will address this issue.

Signed-off-by: James Kelly <jamespeterkelly@gmail.com>
---
 drivers/staging/clocking-wizard/TODO               |   4 +-
 .../clocking-wizard/clk-xlnx-clock-wizard.c        | 115 +++++++++++++++++++++
 2 files changed, 117 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/clocking-wizard/TODO b/drivers/staging/clocking-wizard/TODO
index 53c9941fcc35..50193bdd61e1 100644
--- a/drivers/staging/clocking-wizard/TODO
+++ b/drivers/staging/clocking-wizard/TODO
@@ -1,8 +1,8 @@
 TODO:
-	- support for set_rate() operations (may benefit from Stephen Boyd's
-	  refactoring of the clk primitives: https://lkml.org/lkml/2014/9/5/766)
 	- review arithmetic
 	  - overflow after multiplication?
+	- implement CLK_SET_RATE_PARENT to set internal clocks
+	- implement CLK_SET_RATE_PARENT to set input clock
 	- test on 64-bit ARM and Microblaze architectures.
 	- support clk_set_phase
 
diff --git a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
index 8828dac6faaf..455ee9887c77 100644
--- a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
+++ b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
@@ -76,6 +76,7 @@
 #define KHz			1000UL
 #define MHz			1000000UL
 #define WZRD_ACLK_MAX_FREQ	(250 * MHz)
+#define WZRD_PLL_LOCK_TIMEOUT	1000		// usec
 #define WZRD_FRAC_BITS		3
 #define WZRD_FRAC_MASK		(BIT(WZRD_FRAC_BITS) - 1)
 #define WZRD_FRAC_SCALE		(1000 >> WZRD_FRAC_BITS)
@@ -85,6 +86,8 @@
 #define WZRD_FLAG_FRAC		BIT(1)
 #define WZRD_FLAG_ADJUST_MIN	BIT(2)
 
+struct clk_wzrd;
+
 /*
  * Clock rate constraints extracted from Xilinx data sheets listed below.
  * The minimum rates depend on family and clock type and the maximum rates
@@ -310,6 +313,7 @@ static const struct reg_field clk_wzrd_reconfig     = REG_FIELD(0x25C, 0,  1);
  *
  * @hw:			handle between common and hardware-specific interfaces
  * @flags:		hardware specific flags
+ * @cw;			pointer to platform device data
  * @ratio_limit:	pointer to divider/multiplier ratio limits
  * @int_field:		pointer to regmap field for integer part
  * @frac_field:		pointer to regmap field for fractional part
@@ -322,6 +326,7 @@ static const struct reg_field clk_wzrd_reconfig     = REG_FIELD(0x25C, 0,  1);
 struct clk_wzrd_clk_data {
 	struct clk_hw			hw;
 	unsigned long			flags;
+	struct clk_wzrd			*cw;
 	const struct clk_wzrd_ratio	*ratio_limit;
 	struct regmap_field		*int_field;
 	struct regmap_field		*frac_field;
@@ -344,6 +349,9 @@ struct clk_wzrd_clk_data {
  * @div:		Divider internal clock provider data
  * @pll:		Phase locked loop internal clock provider data
  * @chip:		Chip data including rate constraints
+ * @pll_locked:		Phase locked loop locked status regmap field
+ * @reconfig:		Reconfiguration regmap field
+ * @dev:		Handle to device
  * @clkout_data:	Array of output clock provider data
  */
 struct clk_wzrd {
@@ -358,6 +366,9 @@ struct clk_wzrd {
 	struct clk_wzrd_clk_data	div_data;
 	struct clk_wzrd_clk_data	pll_data;
 	const struct clk_wzrd_chip	*chip;
+	struct regmap_field		*pll_locked;
+	struct regmap_field		*reconfig;
+	struct device			*dev;
 	struct clk_wzrd_clk_data	clkout_data[0];
 };
 #define to_clk_wzrd(_nb) container_of(_nb, struct clk_wzrd, nb)
@@ -509,6 +520,97 @@ static int clk_wzrd_determine_rate(struct clk_hw *hw,
 	return 0;
 }
 
+static bool clk_wzrd_set_ratio(struct clk_wzrd_clk_data *cwc,
+			       unsigned long parent_rate,
+			       unsigned long rate)
+{
+	unsigned long old_ratio = clk_wzrd_get_ratio(cwc);
+	unsigned long ratio = clk_wzrd_limit_calc_ratio(cwc, parent_rate, rate);
+
+	if (ratio == old_ratio)
+		return false;
+
+	regmap_field_write(cwc->int_field, ratio >> WZRD_FRAC_BITS);
+	if (cwc->flags & WZRD_FLAG_FRAC)
+		regmap_field_write(cwc->frac_field, (ratio & WZRD_FRAC_MASK) *
+				   WZRD_FRAC_SCALE);
+
+	return true;
+}
+
+/*
+ * Wait for PLL to become locked
+ *
+ * The main reason the PLL will not be locked is because the parent clock
+ * changed rate.  It is entirely possible that the PLL will never lock and
+ * the only way to fix this is to change the parent clock rate, as dynamic
+ * reconfiguration of the first divider and PLL multiplier is not possible
+ * when the PLL is not locked!
+ *
+ * Returns true if PLL is already locked or obtains lock within the timeout.
+ */
+static inline bool clk_wzrd_wait_pll_lock(struct clk_wzrd *cw)
+{
+	u32 status;
+
+	return !regmap_field_read_poll_timeout(cw->pll_locked, status,
+					       (status == 1), 100,
+					       WZRD_PLL_LOCK_TIMEOUT);
+}
+
+/*
+ * Wait for dynamic reconfiguration of Clocking Wizard IP to complete
+ *
+ * According to the Xilinx documentation the reconfiguration is
+ * not completed until the PLL is locked so we use the same timeout
+ * logic as we used for clk_wzrd_wait_pll_lock.
+ *
+ * Returns true if reconfiguration succeeded and the PLL locked.
+ */
+static inline bool clk_wzrd_wait_reconfigure(struct clk_wzrd *cw)
+{
+	u32 status;
+
+	return !regmap_field_read_poll_timeout(cw->reconfig, status,
+					       ((status & 1) == 0),
+					       100, WZRD_PLL_LOCK_TIMEOUT);
+}
+
+static inline int clk_wzrd_reconfigure(struct clk_wzrd *cw)
+{
+	regmap_field_write(cw->reconfig, 3);
+
+	if (!clk_wzrd_wait_reconfigure(cw)) {
+		dev_err(cw->dev, "Reconfiguration failed\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int clk_wzrd_set_rate(struct clk_hw *hw, unsigned long req_rate,
+			     unsigned long parent_rate)
+{
+	struct clk_wzrd_clk_data *cwc = to_clk_wzrd_clk_data(hw);
+	struct clk_wzrd *cw = cwc->cw;
+
+	if (cwc->flags & WZRD_FLAG_MULTIPLY && parent_rate == 0)
+		return -EINVAL;
+
+	if (!(cwc->flags & WZRD_FLAG_MULTIPLY) && req_rate == 0)
+		return -EINVAL;
+
+	if (!clk_wzrd_wait_pll_lock(cw)) {
+		dev_err(cw->dev, "Set rate not possible - PLL not locked\n");
+		return -EIO;
+	}
+
+	if (!clk_wzrd_set_ratio(cwc, parent_rate, req_rate))
+		return 0;
+
+	return clk_wzrd_reconfigure(cw);
+}
+
 #ifdef CONFIG_DEBUG_FS
 
 static int clk_wzrd_flags_show(struct seq_file *s, void *data)
@@ -576,6 +678,7 @@ static int clk_wzrd_debug_init(struct clk_hw *hw, struct dentry *dentry)
 static const struct clk_ops clk_wzrd_clk_ops = {
 	.recalc_rate = clk_wzrd_recalc_rate,
 	.determine_rate = clk_wzrd_determine_rate,
+	.set_rate = clk_wzrd_set_rate,
 #ifdef CONFIG_DEBUG_FS
 	.debug_init = clk_wzrd_debug_init,
 #endif
@@ -641,6 +744,7 @@ static int clk_wzrd_register_clk(struct device *dev, const char *name,
 	init.parent_names = &parent_name;
 	init.num_parents = 1;
 	cwc->hw.init = &init;
+	cwc->cw = cw;
 	cwc->ratio_limit = &ratio_constraints[cw->chip->cell][type];
 	cwc->int_field = devm_regmap_field_alloc(dev, cw->regmap,
 						 *int_reg_field);
@@ -770,6 +874,7 @@ static int clk_wzrd_get_device_tree_data(struct device *dev)
 	if (!cw)
 		return -ENOMEM;
 	dev_set_drvdata(dev, cw);
+	cw->dev = dev;
 	cw->chip = &chip_constraints[family][type];
 	cw->clk_data.clk_num = num_outputs;
 
@@ -842,6 +947,16 @@ static int clk_wzrd_regmap_alloc(struct device *dev)
 	if (IS_ERR(cw->regmap))
 		return PTR_ERR(cw->regmap);
 
+	cw->pll_locked = devm_regmap_field_alloc(dev, cw->regmap,
+						 clk_wzrd_status_locked);
+	if (IS_ERR(cw->pll_locked))
+		return PTR_ERR(cw->pll_locked);
+
+	cw->reconfig = devm_regmap_field_alloc(dev, cw->regmap,
+					       clk_wzrd_reconfig);
+	if (IS_ERR(cw->reconfig))
+		return PTR_ERR(cw->reconfig);
+
 	return 0;
 }
 
-- 
2.15.1 (Apple Git-101)

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 11/14] staging: clocking-wizard: Support clk_set_rate

[Michal Simek]

On 7.5.2018 03:20, James Kelly wrote:
> Provide initial support for CCF clk_set_rate API on all clock components.
> 
> Clock consumers that want to set the first divider or PLL clock will need
> to use clk_get_parent on one of the output clocks as there is no support
> for CLK_SET_RATE_PARENT yet.
> 
> Care must be taken when setting the first divider clock to ensure that
> the PLL clock rate will remain within a valid range for the VCO, as it
> is impossible to subsequently update any clock if the PLL does not lock.
> A subsequent patch will address this issue.
> 
> Signed-off-by: James Kelly <jamespeterkelly@gmail.com>
> ---
>  drivers/staging/clocking-wizard/TODO               |   4 +-
>  .../clocking-wizard/clk-xlnx-clock-wizard.c        | 115 +++++++++++++++++++++
>  2 files changed, 117 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/clocking-wizard/TODO b/drivers/staging/clocking-wizard/TODO
> index 53c9941fcc35..50193bdd61e1 100644
> --- a/drivers/staging/clocking-wizard/TODO
> +++ b/drivers/staging/clocking-wizard/TODO
> @@ -1,8 +1,8 @@
>  TODO:
> -	- support for set_rate() operations (may benefit from Stephen Boyd's
> -	  refactoring of the clk primitives: https://lkml.org/lkml/2014/9/5/766)
>  	- review arithmetic
>  	  - overflow after multiplication?
> +	- implement CLK_SET_RATE_PARENT to set internal clocks
> +	- implement CLK_SET_RATE_PARENT to set input clock
>  	- test on 64-bit ARM and Microblaze architectures.
>  	- support clk_set_phase
>  
> diff --git a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> index 8828dac6faaf..455ee9887c77 100644
> --- a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> +++ b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
> @@ -76,6 +76,7 @@
>  #define KHz			1000UL
>  #define MHz			1000000UL
>  #define WZRD_ACLK_MAX_FREQ	(250 * MHz)
> +#define WZRD_PLL_LOCK_TIMEOUT	1000		// usec
>  #define WZRD_FRAC_BITS		3
>  #define WZRD_FRAC_MASK		(BIT(WZRD_FRAC_BITS) - 1)
>  #define WZRD_FRAC_SCALE		(1000 >> WZRD_FRAC_BITS)
> @@ -85,6 +86,8 @@
>  #define WZRD_FLAG_FRAC		BIT(1)
>  #define WZRD_FLAG_ADJUST_MIN	BIT(2)
>  
> +struct clk_wzrd;
> +
>  /*
>   * Clock rate constraints extracted from Xilinx data sheets listed below.
>   * The minimum rates depend on family and clock type and the maximum rates
> @@ -310,6 +313,7 @@ static const struct reg_field clk_wzrd_reconfig     = REG_FIELD(0x25C, 0,  1);
>   *
>   * @hw:			handle between common and hardware-specific interfaces
>   * @flags:		hardware specific flags
> + * @cw;			pointer to platform device data

s/;/:/

M
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 12/14] staging: clocking-wizard: Automatically set internal clock rates

[James Kelly]

Allow CLK_SET_RATE_PARENT to be optionally enabled on one of the output
clocks.  This will automatically choose the "best" rates for the first
divider and PLL multiplier.  Best is defined as those first divider and
PLL multplier rates that minimise the error in the rate of the output clock
that has CLK_SET_RATE_PARENT enabled.  The current implementation uses a
constrained brute force search for the best parent rate that stops when a
rate is within 10ppm of that requested.

The output clock for which CLK_SET_RATE_PARENT should be enabled is
specified using a new device-tree property named "set-parent-output".
This is an optional property and if not present this feature is disabled.

If first divider clock is updated before the PLL multiplier clock adjust
the PLL multiplier to keep PLL lock.

Signed-off-by: James Kelly <jamespeterkelly@gmail.com>
---
 drivers/staging/clocking-wizard/TODO               |   1 -
 .../clocking-wizard/clk-xlnx-clock-wizard.c        | 151 ++++++++++++++++++++-
 drivers/staging/clocking-wizard/dt-binding.txt     |   3 +
 3 files changed, 149 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/clocking-wizard/TODO b/drivers/staging/clocking-wizard/TODO
index 50193bdd61e1..bf7435c5b67e 100644
--- a/drivers/staging/clocking-wizard/TODO
+++ b/drivers/staging/clocking-wizard/TODO
@@ -1,7 +1,6 @@
 TODO:
 	- review arithmetic
 	  - overflow after multiplication?
-	- implement CLK_SET_RATE_PARENT to set internal clocks
 	- implement CLK_SET_RATE_PARENT to set input clock
 	- test on 64-bit ARM and Microblaze architectures.
 	- support clk_set_phase
diff --git a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
index 455ee9887c77..f706c3d6496e 100644
--- a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
+++ b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
@@ -315,6 +315,8 @@ static const struct reg_field clk_wzrd_reconfig     = REG_FIELD(0x25C, 0,  1);
  * @flags:		hardware specific flags
  * @cw;			pointer to platform device data
  * @ratio_limit:	pointer to divider/multiplier ratio limits
+ * @min_parent:		minimum parent clk rate
+ * @max_parent:		maximum parent clk rate
  * @int_field:		pointer to regmap field for integer part
  * @frac_field:		pointer to regmap field for fractional part
  *
@@ -328,6 +330,8 @@ struct clk_wzrd_clk_data {
 	unsigned long			flags;
 	struct clk_wzrd			*cw;
 	const struct clk_wzrd_ratio	*ratio_limit;
+	unsigned long			min_parent;
+	unsigned long			max_parent;
 	struct regmap_field		*int_field;
 	struct regmap_field		*frac_field;
 };
@@ -501,10 +505,77 @@ static inline unsigned long clk_wzrd_round_rate(struct clk_wzrd_clk_data *cwc,
 	return clk_wzrd_calc_rate(parent_rate, ratio, cwc->flags);
 }
 
+static inline unsigned long clk_wzrd_parent_rate(struct clk_wzrd_clk_data *cwc,
+						 unsigned long rate,
+						 unsigned long ratio)
+{
+	return clk_wzrd_calc_rate(rate, ratio, cwc->flags ^
+				  WZRD_FLAG_MULTIPLY);
+}
+
+/*
+ * Search for the best parent rate
+ *
+ * This has the potential to run a long time if our parent clocks also
+ * search for their best parent rate.
+ */
+static bool clk_wzrd_best_parent_rate(struct clk_wzrd_clk_data *cwc,
+				      struct clk_rate_request *req)
+{
+	unsigned long ratio, min_ratio, max_ratio;
+	unsigned long min_parent_rate = cwc->min_parent;
+	unsigned long max_parent_rate = cwc->max_parent;
+	unsigned long best_delta = ULONG_MAX;
+	unsigned long inc = cwc->flags & WZRD_FLAG_FRAC ? 1 :
+			BIT(WZRD_FRAC_BITS);
+
+	/*
+	 * Search by testing parent rates that corresponds to all possible
+	 * ratios that are within our parent rate constraints.
+	 */
+	min_ratio = clk_wzrd_limit_calc_ratio(cwc, min_parent_rate, req->rate);
+	max_ratio = clk_wzrd_limit_calc_ratio(cwc, max_parent_rate, req->rate);
+
+	if (cwc->flags & WZRD_FLAG_MULTIPLY)
+		swap(min_ratio, max_ratio);
+
+	for (ratio = max_ratio; ratio >= min_ratio; ratio -= inc) {
+		unsigned long parent_rate, rate, delta;
+
+		parent_rate = clk_wzrd_parent_rate(cwc, req->rate, ratio);
+
+		/*
+		 * Figure out what rate we will actually end up with.  This
+		 * may take a while if our parent can set their parent rate.
+		 */
+		parent_rate = clk_hw_round_rate(req->best_parent_hw,
+						parent_rate);
+		if (parent_rate > max_parent_rate ||
+		    parent_rate < min_parent_rate || parent_rate == 0)
+			continue;
+
+		rate = clk_wzrd_round_rate(cwc, parent_rate, req->rate);
+		if (rate < req->min_rate || rate > req->max_rate)
+			continue;
+
+		delta = rate > req->rate ? rate - req->rate : req->rate - rate;
+		if (delta < best_delta) {
+			req->best_parent_rate = parent_rate;
+			best_delta = delta;
+
+			if ((unsigned long long)best_delta * 100000 < rate)
+				return false;
+		}
+	}
+
+	return best_delta == ULONG_MAX;
+}
+
 static int clk_wzrd_determine_rate(struct clk_hw *hw,
 				   struct clk_rate_request *req)
 {
 	struct clk_wzrd_clk_data *cwc = to_clk_wzrd_clk_data(hw);
+	unsigned long flags = clk_hw_get_flags(hw);
 
 	if (cwc->flags & WZRD_FLAG_MULTIPLY && req->best_parent_rate == 0)
 		return -EINVAL;
@@ -512,6 +583,9 @@ static int clk_wzrd_determine_rate(struct clk_hw *hw,
 	if (!(cwc->flags & WZRD_FLAG_MULTIPLY) && req->rate == 0)
 		return -EINVAL;
 
+	if (flags & CLK_SET_RATE_PARENT && clk_wzrd_best_parent_rate(cwc, req))
+		return -EINVAL;
+
 	req->rate = clk_wzrd_round_rate(cwc, req->best_parent_rate, req->rate);
 
 	if (req->rate < req->min_rate || req->rate > req->max_rate)
@@ -588,11 +662,40 @@ static inline int clk_wzrd_reconfigure(struct clk_wzrd *cw)
 	return 0;
 }
 
+/*
+ * Check if the PLL multiplier ratio needs adjustment to ensure that the VCO
+ * frequency remains within the valid range.  This is necessary to ensure the
+ * PLL will still lock when the first divider ratio is updated before the PLL
+ * multiplier ratio is updated as is the want of the CCF when using
+ * CLK_SET_RATE_PARENT.
+ *
+ * Returns true if the PLL multiplier ratio had to be adjusted.
+ */
+static bool clk_wzrd_check_pll_rate(struct clk_wzrd *cw,
+				    unsigned long input_rate)
+{
+	unsigned long div_rate, pll_rate, clamped_rate;
+	struct clk_wzrd_clk_data *out_cwc = &cw->clkout_data[0];
+
+	div_rate = clk_wzrd_recalc_rate(&cw->div_data.hw, input_rate);
+	pll_rate = clk_wzrd_recalc_rate(&cw->pll_data.hw, div_rate);
+	clamped_rate = clamp_val(pll_rate, out_cwc->min_parent,
+				 out_cwc->max_parent);
+
+	if (pll_rate == clamped_rate)
+		return false;
+
+	clk_wzrd_set_ratio(&cw->pll_data, div_rate, clamped_rate);
+
+	return true;
+}
+
 static int clk_wzrd_set_rate(struct clk_hw *hw, unsigned long req_rate,
 			     unsigned long parent_rate)
 {
 	struct clk_wzrd_clk_data *cwc = to_clk_wzrd_clk_data(hw);
 	struct clk_wzrd *cw = cwc->cw;
+	bool reconfigure = false;
 
 	if (cwc->flags & WZRD_FLAG_MULTIPLY && parent_rate == 0)
 		return -EINVAL;
@@ -605,7 +708,12 @@ static int clk_wzrd_set_rate(struct clk_hw *hw, unsigned long req_rate,
 		return -EIO;
 	}
 
-	if (!clk_wzrd_set_ratio(cwc, parent_rate, req_rate))
+	reconfigure = clk_wzrd_set_ratio(cwc, parent_rate, req_rate);
+
+	if (cwc == &cw->div_data)
+		reconfigure |= clk_wzrd_check_pll_rate(cw, parent_rate);
+
+	if (!reconfigure)
 		return 0;
 
 	return clk_wzrd_reconfigure(cw);
@@ -661,6 +769,16 @@ static int clk_wzrd_debug_init(struct clk_hw *hw, struct dentry *dentry)
 	if (IS_ERR(d))
 		return PTR_ERR(d);
 
+	d = debugfs_create_u32("clk_parent_rate_min", 0444, dentry,
+			       (u32 *)&cwc->min_parent);
+	if (IS_ERR(d))
+		return PTR_ERR(d);
+
+	d = debugfs_create_u32("clk_parent_rate_max", 0444, dentry,
+			       (u32 *)&cwc->max_parent);
+	if (IS_ERR(d))
+		return PTR_ERR(d);
+
 	d = debugfs_create_file_unsafe("clk_hw_flags", 0444, dentry, cwc,
 				       &clk_wzrd_flags_fops);
 	if (IS_ERR(d))
@@ -831,7 +949,7 @@ static SIMPLE_DEV_PM_OPS(clk_wzrd_dev_pm_ops, clk_wzrd_suspend,
 
 static int clk_wzrd_get_device_tree_data(struct device *dev)
 {
-	int num_outputs, ret, speed_grade;
+	int num_outputs, ret, speed_grade, i;
 	enum clk_wzrd_family family;
 	enum clk_wzrd_type type;
 	struct clk_wzrd *cw;
@@ -897,6 +1015,16 @@ static int clk_wzrd_get_device_tree_data(struct device *dev)
 			cw->speed_grade = speed_grade - 1;
 	}
 
+	cw->div_data.min_parent = cw->chip->min[WZRD_RATE_FIN];
+	cw->div_data.max_parent = cw->chip->max[cw->speed_grade][WZRD_RATE_FIN];
+	cw->pll_data.min_parent = cw->chip->min[WZRD_RATE_PFD];
+	cw->pll_data.max_parent = cw->chip->max[cw->speed_grade][WZRD_RATE_PFD];
+	for (i = 0; i < num_outputs; i++) {
+		cw->clkout_data[i].min_parent = cw->chip->min[WZRD_RATE_VCO];
+		cw->clkout_data[i].max_parent =
+				cw->chip->max[cw->speed_grade][WZRD_RATE_VCO];
+	}
+
 	cw->clk_in1 = devm_clk_get(dev, WZRD_CLKNAME_IN1);
 	if (IS_ERR(cw->clk_in1)) {
 		if (cw->clk_in1 != ERR_PTR(-EPROBE_DEFER))
@@ -963,11 +1091,22 @@ static int clk_wzrd_regmap_alloc(struct device *dev)
 static int clk_wzrd_register_ccf(struct device *dev)
 {
 	int i, ret;
+	unsigned int set_output;
 	const char *clk_div_name;
 	const char *clk_pll_name;
 
+	unsigned long pll_flags = 0, out_flags = 0;
 	struct clk_wzrd *cw = dev_get_drvdata(dev);
 
+	ret = of_property_read_u32(dev->of_node, "set-parent-output",
+				   &set_output);
+	if (!ret) {
+		if (set_output >= cw->clk_data.clk_num)
+			dev_warn(dev, "set-parent-output invalid\n");
+		else
+			pll_flags = CLK_SET_RATE_PARENT;
+	}
+
 	clk_div_name = kasprintf(GFP_KERNEL, "%s_div", dev_name(dev));
 	if (!clk_div_name)
 		return -ENOMEM;
@@ -980,7 +1119,8 @@ static int clk_wzrd_register_ccf(struct device *dev)
 		ret = -ENOMEM;
 		goto err_free_div_name;
 	}
-	ret = clk_wzrd_register_clk(dev, clk_pll_name, WZRD_CLK_PLL, 0, 0);
+	ret = clk_wzrd_register_clk(dev, clk_pll_name, WZRD_CLK_PLL, 0,
+				    pll_flags);
 	if (ret)
 		goto err_free_pll_name;
 
@@ -995,8 +1135,9 @@ static int clk_wzrd_register_ccf(struct device *dev)
 			goto err_free_pll_name;
 		}
 
-		ret = clk_wzrd_register_clk(dev, clkout_name, WZRD_CLK_OUT,
-					    i, 0);
+		out_flags = (i == set_output) ? pll_flags : 0;
+		ret = clk_wzrd_register_clk(dev, clkout_name, WZRD_CLK_OUT, i,
+					    out_flags);
 		if (ret)
 			goto err_free_pll_name;
 
diff --git a/drivers/staging/clocking-wizard/dt-binding.txt b/drivers/staging/clocking-wizard/dt-binding.txt
index 6e7859a6e751..37133a3f2ee9 100644
--- a/drivers/staging/clocking-wizard/dt-binding.txt
+++ b/drivers/staging/clocking-wizard/dt-binding.txt
@@ -24,6 +24,8 @@ Optional properties:
  - xlnx,primitive: Clocking Wizard primitive
 	0 - MMCM (default)
 	1 - PLL
+ - set-parent-output: Set rate on this output can set parent rates
+	Valid values are 0..n-1, where n is number of clock outputs.
 
 Example:
 	clock-generator@40040000 {
@@ -37,4 +39,5 @@ Example:
 				     "clk_out6", "clk_out7";
 		xlnx,family = <0>;
 		xlnx,primitive = <0>;
+		set-parent-output <0>;
 	};
-- 
2.15.1 (Apple Git-101)

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 13/14] staging: clocking-wizard: Automatically set input clock rate

[James Kelly]

Allow CLK_SET_RATE_PARENT to be optionally enabled on first divider clock.
This has the potential to set the rate of one output clock with more
precision.  On Zynq-7000 this is typically achieved using a PS FCLK as
input to the Clocking Wizard IP.

This feature is enabled by the optional device-tree property
"set-input-rate-range".  The presence of this property in the device tree
enables the feature.  This feature is only active if the
"set-parent-output" device-tree property is also present as it depends on
the feature enabled by that property.

The value of the "set-input-rate-range" allows the input rate to be
constrained to a narrower range than the hardware supports as this is
useful in some circumstances.

The input rate is then further constrained to ensure the PLL can always
lock when the input rate is changed.

Signed-off-by: James Kelly <jamespeterkelly@gmail.com>
---
 drivers/staging/clocking-wizard/TODO               |  1 -
 .../clocking-wizard/clk-xlnx-clock-wizard.c        | 89 +++++++++++++++++++++-
 drivers/staging/clocking-wizard/dt-binding.txt     |  4 +
 3 files changed, 89 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/clocking-wizard/TODO b/drivers/staging/clocking-wizard/TODO
index bf7435c5b67e..2a563ca67cd2 100644
--- a/drivers/staging/clocking-wizard/TODO
+++ b/drivers/staging/clocking-wizard/TODO
@@ -1,7 +1,6 @@
 TODO:
 	- review arithmetic
 	  - overflow after multiplication?
-	- implement CLK_SET_RATE_PARENT to set input clock
 	- test on 64-bit ARM and Microblaze architectures.
 	- support clk_set_phase
 
diff --git a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
index f706c3d6496e..bb64da849d9b 100644
--- a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
+++ b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
@@ -356,6 +356,8 @@ struct clk_wzrd_clk_data {
  * @pll_locked:		Phase locked loop locked status regmap field
  * @reconfig:		Reconfiguration regmap field
  * @dev:		Handle to device
+ * @min_input:		Current minimum input rate to ensure PLL lock
+ * @max_input:		Current maximum input rate to ensure PLL lock
  * @clkout_data:	Array of output clock provider data
  */
 struct clk_wzrd {
@@ -373,6 +375,8 @@ struct clk_wzrd {
 	struct regmap_field		*pll_locked;
 	struct regmap_field		*reconfig;
 	struct device			*dev;
+	unsigned long			min_input;
+	unsigned long			max_input;
 	struct clk_wzrd_clk_data	clkout_data[0];
 };
 #define to_clk_wzrd(_nb) container_of(_nb, struct clk_wzrd, nb)
@@ -529,6 +533,15 @@ static bool clk_wzrd_best_parent_rate(struct clk_wzrd_clk_data *cwc,
 	unsigned long inc = cwc->flags & WZRD_FLAG_FRAC ? 1 :
 			BIT(WZRD_FRAC_BITS);
 
+	/*
+	 * Apply extra constraint to ensure PLL lock when looking for the
+	 * best rate at the input of the Clocking Wizard IP.
+	 */
+	if (cwc == &cwc->cw->div_data) {
+		min_parent_rate = cwc->cw->min_input;
+		max_parent_rate = cwc->cw->max_input;
+	}
+
 	/*
 	 * Search by testing parent rates that corresponds to all possible
 	 * ratios that are within our parent rate constraints.
@@ -594,6 +607,30 @@ static int clk_wzrd_determine_rate(struct clk_hw *hw,
 	return 0;
 }
 
+static void clk_wzrd_update_input_constraint(struct clk_wzrd *cw)
+{
+	unsigned long min_pfd, max_pfd;
+	struct clk_wzrd_clk_data *out_cwc = &cw->clkout_data[0];
+	struct clk_wzrd_clk_data *pll_cwc = &cw->pll_data;
+	struct clk_wzrd_clk_data *div_cwc = &cw->div_data;
+	unsigned long current_pll_ratio = clk_wzrd_get_ratio(pll_cwc);
+	unsigned long current_div_ratio = clk_wzrd_get_ratio(div_cwc);
+
+	min_pfd = max(clk_wzrd_parent_rate(pll_cwc, out_cwc->min_parent,
+					   current_pll_ratio),
+		      pll_cwc->min_parent);
+	max_pfd = min(clk_wzrd_parent_rate(pll_cwc, out_cwc->max_parent,
+					   current_pll_ratio),
+		      pll_cwc->max_parent);
+
+	cw->min_input = max(clk_wzrd_parent_rate(div_cwc, min_pfd,
+						 current_div_ratio),
+			    div_cwc->min_parent);
+	cw->max_input = min(clk_wzrd_parent_rate(div_cwc, max_pfd,
+						 current_div_ratio),
+			    div_cwc->max_parent);
+}
+
 static bool clk_wzrd_set_ratio(struct clk_wzrd_clk_data *cwc,
 			       unsigned long parent_rate,
 			       unsigned long rate)
@@ -716,6 +753,9 @@ static int clk_wzrd_set_rate(struct clk_hw *hw, unsigned long req_rate,
 	if (!reconfigure)
 		return 0;
 
+	if (cwc == &cw->div_data || cwc == &cw->pll_data)
+		clk_wzrd_update_input_constraint(cw);
+
 	return clk_wzrd_reconfigure(cw);
 }
 
@@ -779,6 +819,18 @@ static int clk_wzrd_debug_init(struct clk_hw *hw, struct dentry *dentry)
 	if (IS_ERR(d))
 		return PTR_ERR(d);
 
+	if (cwc == &cwc->cw->div_data) {
+		d = debugfs_create_u32("clk_input_rate_min", 0444, dentry,
+				       (u32 *)&cwc->cw->min_input);
+		if (IS_ERR(d))
+			return PTR_ERR(d);
+
+		d = debugfs_create_u32("clk_input_rate_max", 0444, dentry,
+				       (u32 *)&cwc->cw->max_input);
+		if (IS_ERR(d))
+			return PTR_ERR(d);
+	}
+
 	d = debugfs_create_file_unsafe("clk_hw_flags", 0444, dentry, cwc,
 				       &clk_wzrd_flags_fops);
 	if (IS_ERR(d))
@@ -905,8 +957,8 @@ static int clk_wzrd_clk_notifier(struct notifier_block *nb, unsigned long event,
 		return NOTIFY_OK;
 
 	if (ndata->clk == cw->clk_in1) {
-		max = cw->chip->max[cw->speed_grade][WZRD_RATE_FIN];
-		min = cw->chip->min[WZRD_RATE_FIN];
+		max = cw->max_input;
+		min = cw->min_input;
 	} else if (ndata->clk == cw->axi_clk) {
 		max = WZRD_ACLK_MAX_FREQ;
 		min = 0;
@@ -1025,6 +1077,28 @@ static int clk_wzrd_get_device_tree_data(struct device *dev)
 				cw->chip->max[cw->speed_grade][WZRD_RATE_VCO];
 	}
 
+	if (of_find_property(node, "set-input-rate-range", NULL)) {
+		unsigned int rate;
+
+		ret = of_property_read_u32_index(node, "set-input-rate-range",
+						 0, &rate);
+		if (ret || rate < cw->div_data.min_parent ||
+		    rate > cw->div_data.max_parent) {
+			dev_err(dev, "Invalid set input minimum rate\n");
+			return -EINVAL;
+		}
+		cw->div_data.min_parent = rate;
+
+		ret = of_property_read_u32_index(node, "set-input-rate-range",
+						 1, &rate);
+		if (ret || rate < cw->div_data.min_parent ||
+		    rate > cw->div_data.max_parent) {
+			dev_err(dev, "Invalid set input maximum rate\n");
+			return -EINVAL;
+		}
+		cw->div_data.max_parent = rate;
+	}
+
 	cw->clk_in1 = devm_clk_get(dev, WZRD_CLKNAME_IN1);
 	if (IS_ERR(cw->clk_in1)) {
 		if (cw->clk_in1 != ERR_PTR(-EPROBE_DEFER))
@@ -1055,6 +1129,8 @@ static int clk_wzrd_get_device_tree_data(struct device *dev)
 		 clk_wzrd_family_name[family],
 		 clk_wzrd_type_name[type],
 		 cw->speed_grade + 1);
+	dev_info(dev, "Input rate range min: %lu max: %lu\n",
+		 cw->div_data.min_parent, cw->div_data.max_parent);
 
 	return 0;
 }
@@ -1095,7 +1171,7 @@ static int clk_wzrd_register_ccf(struct device *dev)
 	const char *clk_div_name;
 	const char *clk_pll_name;
 
-	unsigned long pll_flags = 0, out_flags = 0;
+	unsigned long div_flags = 0, pll_flags = 0, out_flags = 0;
 	struct clk_wzrd *cw = dev_get_drvdata(dev);
 
 	ret = of_property_read_u32(dev->of_node, "set-parent-output",
@@ -1107,10 +1183,13 @@ static int clk_wzrd_register_ccf(struct device *dev)
 			pll_flags = CLK_SET_RATE_PARENT;
 	}
 
+	if (of_find_property(dev->of_node, "set-input-rate-range", NULL))
+		div_flags = pll_flags;
 	clk_div_name = kasprintf(GFP_KERNEL, "%s_div", dev_name(dev));
 	if (!clk_div_name)
 		return -ENOMEM;
-	ret = clk_wzrd_register_clk(dev, clk_div_name, WZRD_CLK_DIV, 0, 0);
+	ret = clk_wzrd_register_clk(dev, clk_div_name, WZRD_CLK_DIV, 0,
+				    div_flags);
 	if (ret)
 		goto err_free_div_name;
 
@@ -1144,6 +1223,8 @@ static int clk_wzrd_register_ccf(struct device *dev)
 		cw->clkout[i] = cw->clkout_data[i].hw.clk;
 	}
 
+	clk_wzrd_update_input_constraint(cw);
+
 	cw->clk_data.clks = cw->clkout;
 	of_clk_add_provider(dev->of_node, of_clk_src_onecell_get,
 			    &cw->clk_data);
diff --git a/drivers/staging/clocking-wizard/dt-binding.txt b/drivers/staging/clocking-wizard/dt-binding.txt
index 37133a3f2ee9..9dfc4e71b489 100644
--- a/drivers/staging/clocking-wizard/dt-binding.txt
+++ b/drivers/staging/clocking-wizard/dt-binding.txt
@@ -26,6 +26,9 @@ Optional properties:
 	1 - PLL
  - set-parent-output: Set rate on this output can set parent rates
 	Valid values are 0..n-1, where n is number of clock outputs.
+ - set-input-rate-range: Set rate on set-parent-output can set input rate.
+	Array of two 32-bit values, where 1st element is minimum input rate
+	constraint and 2nd element is maximum input rate constraint.
 
 Example:
 	clock-generator@40040000 {
@@ -40,4 +43,5 @@ Example:
 		xlnx,family = <0>;
 		xlnx,primitive = <0>;
 		set-parent-output <0>;
+		set-input-rate-range <10000000 80000000>;
 	};
-- 
2.15.1 (Apple Git-101)

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH 14/14] staging: clocking-wizard: Add debugfs entries to facilitate testing.

[James Kelly]

Adds test_round_rate and test_set_rate entries to debugfs so that the
driver can be tested by a user space test application.

It would appear that patches that allow access to clk_set_rate from user
space are controversial so there is no expectation that this patch can
be merged.

Signed-off-by: James Kelly <jamespeterkelly@gmail.com>
---
 .../clocking-wizard/clk-xlnx-clock-wizard.c        | 53 ++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
index bb64da849d9b..c37f0e4451b4 100644
--- a/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
+++ b/drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c
@@ -308,6 +308,12 @@ static const struct reg_field clk_wzrd_clkout_divide[WZRD_MAX_OUTPUTS] = {
 static const struct reg_field clk_wzrd_clkout0_frac = REG_FIELD(0x208, 8, 17);
 static const struct reg_field clk_wzrd_reconfig     = REG_FIELD(0x25C, 0,  1);
 
+#ifdef CONFIG_DEBUG_FS
+struct clk_wzrd_debug {
+	unsigned long round_rate;
+};
+#endif
+
 /**
  * struct clk_wzrd_clk_data - Clocking Wizard component clock provider data
  *
@@ -319,6 +325,7 @@ static const struct reg_field clk_wzrd_reconfig     = REG_FIELD(0x25C, 0,  1);
  * @max_parent:		maximum parent clk rate
  * @int_field:		pointer to regmap field for integer part
  * @frac_field:		pointer to regmap field for fractional part
+ * @debug:		debug information
  *
  * Flags:
  * WZRD_FLAG_MULTIPLY	Clock is a multiplier rather than a divider
@@ -334,6 +341,9 @@ struct clk_wzrd_clk_data {
 	unsigned long			max_parent;
 	struct regmap_field		*int_field;
 	struct regmap_field		*frac_field;
+#ifdef CONFIG_DEBUG_FS
+	struct clk_wzrd_debug		debug;
+#endif
 };
 
 #define to_clk_wzrd_clk_data(_hw) \
@@ -794,6 +804,39 @@ static int clk_wzrd_ratio_show(struct seq_file *s, void *data)
 }
 DEFINE_SHOW_ATTRIBUTE(clk_wzrd_ratio);
 
+static int clk_wzrd_test_get_round_rate(void *data, u64 *val)
+{
+	struct clk_wzrd_clk_data *cwc = data;
+
+	*val = cwc->debug.round_rate;
+	return 0;
+}
+
+static int clk_wzrd_test_set_round_rate(void *data, u64 val)
+{
+	long round_rate;
+	struct clk_wzrd_clk_data *cwc = data;
+
+	round_rate = clk_round_rate(cwc->hw.clk, (unsigned long)val);
+
+	if (round_rate < 0)
+		return round_rate;
+
+	cwc->debug.round_rate = round_rate;
+	return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(fops_test_round_rate, clk_wzrd_test_get_round_rate,
+			 clk_wzrd_test_set_round_rate, "%llu\n");
+
+static int clk_wzrd_test_set_set_rate(void *data, u64 val)
+{
+	struct clk_wzrd_clk_data *cwc = data;
+
+	return clk_set_rate(cwc->hw.clk, (unsigned long)val);
+}
+DEFINE_DEBUGFS_ATTRIBUTE(fops_test_set_rate, NULL,
+			 clk_wzrd_test_set_set_rate, "%llu\n");
+
 static int clk_wzrd_debug_init(struct clk_hw *hw, struct dentry *dentry)
 {
 	struct dentry *d;
@@ -841,6 +884,16 @@ static int clk_wzrd_debug_init(struct clk_hw *hw, struct dentry *dentry)
 	if (IS_ERR(d))
 		return PTR_ERR(d);
 
+	d = debugfs_create_file_unsafe("test_round_rate", 0644, dentry, cwc,
+				       &fops_test_round_rate);
+	if (IS_ERR(d))
+		return PTR_ERR(d);
+
+	d = debugfs_create_file_unsafe("test_set_rate", 0200, dentry, cwc,
+				       &fops_test_set_rate);
+	if (IS_ERR(d))
+		return PTR_ERR(d);
+
 	return 0;
 }
 #endif
-- 
2.15.1 (Apple Git-101)

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 00/14] staging: clocking-wizard: Implement many TODOs

[Greg Kroah-Hartman]

On Mon, May 07, 2018 at 11:20:26AM +1000, James Kelly wrote:
> This series of patches attempts to implement many of the outstanding TODOs
> for the Xilinx Clocking Wizard IP driver that has been languishing in the
> staging tree for some time.  I had a need for this driver so I thought it
> appropriate to give it some love.  Hopefully others will find these patches
> useful.
> 
> Highlights include:
> - Support for fractional ratios when available in hardware.
> - Support for clk_round_rate and clk_set_rate kernel APIs.
> - Automatic set rate of internal clocks when rate of output clock is set.
> - Automatic set rate of input clock and internal clocks when rate of 
>   output clock is set.
> 
> A CCF clock provider has been implemented within the driver to support
> the new functionality as it was not possible to do this with the existing
> clock providers.  There is also code to handle a limitation of Clocking
> Wizard IP which prevents changes to the clock ratios if the PLL is not
> locked.  Great care has to be taken to ensure the PLL will always lock as
> there is no way for the driver to recover if the PLL fails to lock under
> transient conditions that may drive the PLL VCO frequency out of range.
> 
> The patches were built on the current staging-next branch.
> 
> The patches also work with the xlnx_rebase_v4.14 branch of the Xilinx
> linux tree at https://github.com/Xilinx/linux-xlnx.git - this branch is
> used by the current release (2018.1) of the Xilinx development tools.
> Patches corresponding to the following staging tree commits are required as
> prerequisites before applying this patch series to xlnx_rebase_v4.14:
> 
> 667063acb81931e2f8fd0cb91df9fcccad131d9a 
> regmap: add iopoll-like polling macro for regmap_field
> 1dbb3344d9e9cd6e72da23f4058f3e6e926614b6 
> staging: clocking-wizard: add SPDX identifier
> 09956d59bad5f5bb238efb805f0767060e624378 
> staging: clocking-wizard: remove redundant license text
> a08f06bb7a0743a7fc8d571899c93d882468096e 
> seq_file: Introduce DEFINE_SHOW_ATTRIBUTE() helper macro
> 
> Testing has been done on a Digilent Zybo-Z7 development board.  This uses a
> 32-bit ARM architecture Zynq-7020 SoC. Testing used the 2018.1 release of
> the Xilinx development tools which has v6.0 of the Clocking Wizard IP.
> 
> The patches are also applicable to, but are currently untested on:
> - 64-bit ARM architecture (Zynq Ultrascale+ MPSoC etc.)
> - Microblaze architecture (7-Series, Ultrascale, Ultrascale+ FPGAs)
> Others with access to suitable hardware will need to test these platforms.
> 
> Potential users of this driver may use the Xilinx device tree generator
> from https://github.com/Xilinx/device-tree-xlnx.git either directly or via
> the development tools provided by Xilinx.  There are two issues with the
> DTG that any potential testers of these patches should be aware of:
> - The DTG generates a negative value for the device-tree speed-grade
>   property.  The 7th patch has a hack to handle this quirk until Xilinx
>   fix their DTG.
> - The 2018.1 DTG changed the device-tree clock-output-names property so
>   it no longer provides any information about how the Clocking Wizard IP
>   is actually configured.  These patches will still work with the new
>   2018.1 DTG behaviour but the names of the output clocks will no longer
>   match those used in the Clocking Wizard IP customization, and the
>   maximum number of clocks will always be created even if not used or
>   FPGA.  A warning will also be issued stating that there are too many
>   clock output names.  Further details can be found in the Xilinx Community
>   forums at https://bit.ly/2jmFIRf.
> 
> The original driver author appears to have left Xilinx so I have not
> included them as an addressee for these patches and instead directed
> them to another Xilinx Linux maintainer.

Can you please fix up the issues reported in this series, rebase on my
latest tree, and resend so that I can apply them?

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 00/14] staging: clocking-wizard: Implement many TODOs

[James Kelly]

Greg,


> On 14 May 2018, at 10:02 pm, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> Can you please fix up the issues reported in this series, rebase on my
> latest tree, and resend so that I can apply them?
> 
> thanks,
> 
> greg k-h

I’m working on a version 2 patch set and will post when it is ready.

James
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel