| Message ID | cover.1574922435.git.shubhrajyoti.datta@xilinx.com |
|---|---|
| Headers | show |
| Series | clk: clk-wizard: clock-wizard: Driver updates | expand |
On Thu, Nov 28, 2019 at 12:06:15PM +0530, shubhrajyoti.datta@gmail.com wrote: > From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > > Incase there are more than one instance of the clocking wizard. > And if the output name given is the same then the probe fails. > Fix the same by appending the device name to the output name to > make it unique. > > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > --- > drivers/clk/clk-xlnx-clock-wizard.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/clk-xlnx-clock-wizard.c > index 75ea745..9993543 100644 > --- a/drivers/clk/clk-xlnx-clock-wizard.c > +++ b/drivers/clk/clk-xlnx-clock-wizard.c > @@ -555,6 +555,9 @@ static int clk_wzrd_probe(struct platform_device *pdev) > ret = -ENOMEM; > goto err_disable_clk; > } > + outputs = of_property_count_strings(np, "clock-output-names"); > + if (outputs == 1) > + flags = CLK_SET_RATE_PARENT; > clk_wzrd->clks_internal[wzrd_clk_mul] = clk_register_fixed_factor > (&pdev->dev, clk_name, > __clk_get_name(clk_wzrd->clk_in1), > @@ -566,9 +569,6 @@ static int clk_wzrd_probe(struct platform_device *pdev) > goto err_disable_clk; > } > > - outputs = of_property_count_strings(np, "clock-output-names"); > - if (outputs == 1) > - flags = CLK_SET_RATE_PARENT; > clk_name = kasprintf(GFP_KERNEL, "%s_mul_div", dev_name(&pdev->dev)); > if (!clk_name) { > ret = -ENOMEM; > @@ -591,6 +591,7 @@ static int clk_wzrd_probe(struct platform_device *pdev) > /* register div per output */ > for (i = outputs - 1; i >= 0 ; i--) { > const char *clkout_name; > + const char *clkout_name_wiz; > > if (of_property_read_string_index(np, "clock-output-names", i, > &clkout_name)) { > @@ -599,9 +600,11 @@ static int clk_wzrd_probe(struct platform_device *pdev) > ret = -EINVAL; > goto err_rm_int_clks; > } > + clkout_name_wiz = kasprintf(GFP_KERNEL, "%s_%s", > + dev_name(&pdev->dev), clkout_name); If this kasprintf() crashes then clk_wzrd_register_divf() will fail. But that was a headache to review. Just add a check for NULL. We need a kfree() as well. One alternative would be to just declare a buffer on the stack and use snprintf(). We don't need to keep the name around after the call to clk_wzrd_register_divf(). regards, dan carpenter
On Thu, Nov 28, 2019 at 1:15 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Thu, Nov 28, 2019 at 12:06:15PM +0530, shubhrajyoti.datta@gmail.com wrote: > > From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > > > > Incase there are more than one instance of the clocking wizard. > > And if the output name given is the same then the probe fails. > > Fix the same by appending the device name to the output name to > > make it unique. > > > > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > > --- > > drivers/clk/clk-xlnx-clock-wizard.c | 13 ++++++++----- > > 1 file changed, 8 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/clk-xlnx-clock-wizard.c > > index 75ea745..9993543 100644 > > --- a/drivers/clk/clk-xlnx-clock-wizard.c > > +++ b/drivers/clk/clk-xlnx-clock-wizard.c > > @@ -555,6 +555,9 @@ static int clk_wzrd_probe(struct platform_device *pdev) > > ret = -ENOMEM; > > goto err_disable_clk; > > } > > + outputs = of_property_count_strings(np, "clock-output-names"); > > + if (outputs == 1) > > + flags = CLK_SET_RATE_PARENT; > > clk_wzrd->clks_internal[wzrd_clk_mul] = clk_register_fixed_factor > > (&pdev->dev, clk_name, > > __clk_get_name(clk_wzrd->clk_in1), > > @@ -566,9 +569,6 @@ static int clk_wzrd_probe(struct platform_device *pdev) > > goto err_disable_clk; > > } > > > > - outputs = of_property_count_strings(np, "clock-output-names"); > > - if (outputs == 1) > > - flags = CLK_SET_RATE_PARENT; > > clk_name = kasprintf(GFP_KERNEL, "%s_mul_div", dev_name(&pdev->dev)); > > if (!clk_name) { > > ret = -ENOMEM; > > @@ -591,6 +591,7 @@ static int clk_wzrd_probe(struct platform_device *pdev) > > /* register div per output */ > > for (i = outputs - 1; i >= 0 ; i--) { > > const char *clkout_name; > > + const char *clkout_name_wiz; > > > > if (of_property_read_string_index(np, "clock-output-names", i, > > &clkout_name)) { > > @@ -599,9 +600,11 @@ static int clk_wzrd_probe(struct platform_device *pdev) > > ret = -EINVAL; > > goto err_rm_int_clks; > > } > > + clkout_name_wiz = kasprintf(GFP_KERNEL, "%s_%s", > > + dev_name(&pdev->dev), clkout_name); > > If this kasprintf() crashes then clk_wzrd_register_divf() will fail. > But that was a headache to review. Just add a check for NULL. We need > a kfree() as well. > > One alternative would be to just declare a buffer on the stack and use > snprintf(). We don't need to keep the name around after the call to > clk_wzrd_register_divf(). Will fix in next version. > > regards, > dan carpenter >
On Fri, Nov 29, 2019 at 05:37:57PM +0530, Shubhrajyoti Datta wrote: > On Thu, Nov 28, 2019 at 1:15 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > > > On Thu, Nov 28, 2019 at 12:06:15PM +0530, shubhrajyoti.datta@gmail.com wrote: > > > From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > > > > > > Incase there are more than one instance of the clocking wizard. > > > And if the output name given is the same then the probe fails. > > > Fix the same by appending the device name to the output name to > > > make it unique. > > > > > > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > > > --- > > > drivers/clk/clk-xlnx-clock-wizard.c | 13 ++++++++----- > > > 1 file changed, 8 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/clk-xlnx-clock-wizard.c > > > index 75ea745..9993543 100644 > > > --- a/drivers/clk/clk-xlnx-clock-wizard.c > > > +++ b/drivers/clk/clk-xlnx-clock-wizard.c > > > @@ -555,6 +555,9 @@ static int clk_wzrd_probe(struct platform_device *pdev) > > > ret = -ENOMEM; > > > goto err_disable_clk; > > > } > > > + outputs = of_property_count_strings(np, "clock-output-names"); > > > + if (outputs == 1) > > > + flags = CLK_SET_RATE_PARENT; > > > clk_wzrd->clks_internal[wzrd_clk_mul] = clk_register_fixed_factor > > > (&pdev->dev, clk_name, > > > __clk_get_name(clk_wzrd->clk_in1), > > > @@ -566,9 +569,6 @@ static int clk_wzrd_probe(struct platform_device *pdev) > > > goto err_disable_clk; > > > } > > > > > > - outputs = of_property_count_strings(np, "clock-output-names"); > > > - if (outputs == 1) > > > - flags = CLK_SET_RATE_PARENT; > > > clk_name = kasprintf(GFP_KERNEL, "%s_mul_div", dev_name(&pdev->dev)); > > > if (!clk_name) { > > > ret = -ENOMEM; > > > @@ -591,6 +591,7 @@ static int clk_wzrd_probe(struct platform_device *pdev) > > > /* register div per output */ > > > for (i = outputs - 1; i >= 0 ; i--) { > > > const char *clkout_name; > > > + const char *clkout_name_wiz; > > > > > > if (of_property_read_string_index(np, "clock-output-names", i, > > > &clkout_name)) { > > > @@ -599,9 +600,11 @@ static int clk_wzrd_probe(struct platform_device *pdev) > > > ret = -EINVAL; > > > goto err_rm_int_clks; > > > } > > > + clkout_name_wiz = kasprintf(GFP_KERNEL, "%s_%s", > > > + dev_name(&pdev->dev), clkout_name); > > > > If this kasprintf() crashes then clk_wzrd_register_divf() will fail. I meant if kasprintf() returns NULL not crashes... :/ > > But that was a headache to review. Just add a check for NULL. We need > > a kfree() as well. regards, dan carpenter
On Thu, Nov 28, 2019 at 12:06:09PM +0530, shubhrajyoti.datta@gmail.com wrote: > From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > > Move the clocking wizard driver from staging to clk. > > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > --- > drivers/clk/Kconfig | 6 + > drivers/clk/Makefile | 1 + > drivers/clk/clk-xlnx-clock-wizard.c | 335 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 342 insertions(+) > create mode 100644 drivers/clk/clk-xlnx-clock-wizard.c I don't see anything moved here. Rob
Quoting shubhrajyoti.datta@gmail.com (2019-11-27 22:36:16) > From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > > Delete the driver from the staging as it is in drivers/clk. > > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> Can all these patches in this series apply to the staging paths and be picked up by Greg? Then when the driver is ready to be moved out of staging I would like to see one patch that removes the driver from staging and adds it to drivers/clk/ so we can be certain the diff is minimal. Feel free to add me and linux-clk to the review of the clocking-wizard driver patches. I will review the driver patches that way.
Quoting shubhrajyoti.datta@gmail.com (2019-11-27 22:36:17) > From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > > After 90b6c5c73 (clk: Remove CLK_IS_BASIC clk flag) > The CLK_IS_BASIC is deleted. Adapt the driver for the same. I don't see any CLK_IS_BASIC in the tree right now, so did it get reintroduced by this patch series? Can you squash this into whatever patch introduces CLK_IS_BASIC usage?
Quoting shubhrajyoti.datta@gmail.com (2019-11-27 22:36:10) > From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > > Update description for the clocking wizard structure > > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > --- Reviewed-by: Stephen Boyd <sboyd@kernel.org>
Quoting shubhrajyoti.datta@gmail.com (2019-11-27 22:36:11) > From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > > The patch adds support for dynamic reconfiguration of clock output rate. > Output clocks are registered as dividers and set rate callback function > is used for dynamic reconfiguration. > > Based on the initial work from Chirag. > > Signed-off-by: Chirag Parekh <chirag.parekh@xilinx.com> > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > Signed-off-by: Michal Simek <michal.simek@xilinx.com> Are these more like Co-developed-by: tags? > diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/clk-xlnx-clock-wizard.c > index ef9125d..870e7fb 100644 > --- a/drivers/clk/clk-xlnx-clock-wizard.c > +++ b/drivers/clk/clk-xlnx-clock-wizard.c > @@ -29,8 +29,23 @@ > #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_WIDTH 8 > #define WZRD_CLKOUT_DIVIDE_MASK (0xff << WZRD_DIVCLK_DIVIDE_SHIFT) > > +#define WZRD_DR_MAX_INT_DIV_VALUE 255 > +#define WZRD_DR_NUM_RETRIES 10000 > +#define WZRD_DR_STATUS_REG_OFFSET 0x04 > +#define WZRD_DR_LOCK_BIT_MASK 0x00000001 > +#define WZRD_DR_INIT_REG_OFFSET 0x25C > +#define WZRD_DR_DIV_TO_PHASE_OFFSET 4 > +#define WZRD_DR_BEGIN_DYNA_RECONF 0x03 > + > +/* Get the mask from width */ > +#define div_mask(width) ((1 << (width)) - 1) > + > +/* Extract divider instance from clock hardware instance */ > +#define to_clk_wzrd_divider(_hw) container_of(_hw, struct clk_wzrd_divider, hw) > + > enum clk_wzrd_int_clks { > wzrd_clk_mul, > wzrd_clk_mul_div, > @@ -62,6 +77,29 @@ struct clk_wzrd { > bool suspended; > }; > > +/** > + * struct clk_wzrd_divider - clock divider specific to clk_wzrd > + * > + * @hw: handle between common and hardware-specific interfaces > + * @base: base address of register containing the divider > + * @offset: offset address of register containing the divider > + * @shift: shift to the divider bit field > + * @width: width of the divider bit field > + * @flags: clk_wzrd divider flags > + * @table: array of value/divider pairs, last entry should have div = 0 > + * @lock: register lock > + */ > +struct clk_wzrd_divider { > + struct clk_hw hw; > + void __iomem *base; > + u16 offset; > + u8 shift; > + u8 width; > + u8 flags; > + const struct clk_div_table *table; > + spinlock_t *lock; /* divider lock */ Is this register lock used by anything? I'm mostly wondering if dividers are in a shared register with some other clk so we need to have this lock. Otherwise it adds more conditional locking code for no gain and should be removed to simplify. > +}; > + > #define to_clk_wzrd(_nb) container_of(_nb, struct clk_wzrd, nb) > > /* maximum frequencies for input/output clocks per speed grade */ > @@ -71,6 +109,164 @@ static const unsigned long clk_wzrd_max_freq[] = { > 1066000000UL > }; > > +/* spin lock variable for clk_wzrd */ > +static DEFINE_SPINLOCK(clkwzrd_lock); > + > +static unsigned long clk_wzrd_recalc_rate(struct clk_hw *hw, > + unsigned long parent_rate) > +{ > + struct clk_wzrd_divider *divider = to_clk_wzrd_divider(hw); > + void __iomem *div_addr = > + (void __iomem *)((u64)divider->base + divider->offset); Is this casting necessary? > + unsigned int val; > + > + val = readl(div_addr) >> divider->shift; > + val &= div_mask(divider->width); > + > + return divider_recalc_rate(hw, parent_rate, val, divider->table, > + divider->flags, divider->width); > +} > + > +static int clk_wzrd_dynamic_reconfig(struct clk_hw *hw, unsigned long rate, > + unsigned long parent_rate) > +{ > + int err = 0; > + u16 retries; > + u32 value; > + unsigned long flags = 0; > + struct clk_wzrd_divider *divider = to_clk_wzrd_divider(hw); > + void __iomem *div_addr = > + (void __iomem *)((u64)divider->base + divider->offset); > + > + if (divider->lock) > + spin_lock_irqsave(divider->lock, flags); > + else > + __acquire(divider->lock); > + > + value = DIV_ROUND_CLOSEST(parent_rate, rate); > + > + /* Cap the value to max */ > + if (value > WZRD_DR_MAX_INT_DIV_VALUE) > + value = WZRD_DR_MAX_INT_DIV_VALUE; Please use min(). > + > + /* Set divisor and clear phase offset */ > + writel(value, div_addr); > + writel(0x00, div_addr + WZRD_DR_DIV_TO_PHASE_OFFSET); > + > + /* Check status register */ > + retries = WZRD_DR_NUM_RETRIES; > + while (retries--) { > + if (readl(divider->base + WZRD_DR_STATUS_REG_OFFSET) & > + WZRD_DR_LOCK_BIT_MASK) > + break; > + } Is this readl_poll_timeout()? > + > + if (retries == 0) { > + err = -ETIMEDOUT; > + goto err_reconfig; > + } > + > + /* Initiate reconfiguration */ > + writel(WZRD_DR_BEGIN_DYNA_RECONF, > + divider->base + WZRD_DR_INIT_REG_OFFSET); > + > + /* Check status register */ > + retries = WZRD_DR_NUM_RETRIES; > + while (retries--) { > + if (readl(divider->base + WZRD_DR_STATUS_REG_OFFSET) & > + WZRD_DR_LOCK_BIT_MASK) > + break; > + } > + > + if (retries == 0) > + err = -ETIMEDOUT; readl_poll_timeout()? > + > +err_reconfig: > + if (divider->lock) > + spin_unlock_irqrestore(divider->lock, flags); > + else > + __release(divider->lock); > + > + return err; > +} > + > +static long clk_wzrd_round_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long *prate) > +{ > + u8 div; > + > + /* > + * since we donot change parent rate we just round rate to closest s/donot/don't/ > + * achievable > + */ > + div = DIV_ROUND_CLOSEST(*prate, rate); > + > + return (*prate / div); > +} > + > +static const struct clk_ops clk_wzrd_clk_divider_ops = { > + .round_rate = clk_wzrd_round_rate, > + .set_rate = clk_wzrd_dynamic_reconfig, > + .recalc_rate = clk_wzrd_recalc_rate, > +}; > + > +static struct clk *clk_wzrd_register_divider(struct device *dev, > + const char *name, > + const char *parent_name, > + unsigned long flags, > + void __iomem *base, u16 offset, > + u8 shift, u8 width, > + u8 clk_divider_flags, > + const struct clk_div_table *table, > + spinlock_t *lock) > +{ > + struct clk_wzrd_divider *div; > + struct clk_hw *hw; > + struct clk_init_data init; > + int ret; > + > + if (clk_divider_flags & CLK_DIVIDER_HIWORD_MASK) { Do you use HIWORD mask things? I thought that was mostly rockchip specific quirk. If not, please remove this code. > + if (width + shift > 16) { > + pr_warn("divider value exceeds LOWORD field\n"); > + return ERR_PTR(-EINVAL); > + } > + } > + > + /* allocate the divider */ > + div = kzalloc(sizeof(*div), GFP_KERNEL); > + if (!div) > + return ERR_PTR(-ENOMEM); > + > + init.name = name; > + if (clk_divider_flags & CLK_DIVIDER_READ_ONLY) > + init.ops = &clk_divider_ro_ops; > + else > + init.ops = &clk_wzrd_clk_divider_ops; > + init.flags = flags | CLK_IS_BASIC; Oh yeah, don't add this flag. This code won't compile so please compile test each patch in a series to make sure we don't have compilation bisection holes. > + init.parent_names = (parent_name ? &parent_name : NULL); > + init.num_parents = (parent_name ? 1 : 0); Is it sometimes the root of the tree? Seems unlikely so probably just assume there is a parent all the time. > + > + /* struct clk_divider assignments */ Drop useless comment please. > + div->base = base; > + div->offset = offset; > + div->shift = shift; > + div->width = width; > + div->flags = clk_divider_flags; > + div->lock = lock; > + div->hw.init = &init; > + div->table = table; > + > + /* register the clock */ Drop useless comment please. > + hw = &div->hw; > + ret = clk_hw_register(dev, hw); > + if (ret) { > + kfree(div); > + hw = ERR_PTR(ret); > + } > + > + return hw->clk; > +} > + > static int clk_wzrd_clk_notifier(struct notifier_block *nb, unsigned long event, > void *data) > { > @@ -241,11 +437,14 @@ static int clk_wzrd_probe(struct platform_device *pdev) > ret = -EINVAL; > goto err_rm_int_clks; > } > - reg = readl(clk_wzrd->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); > + clk_wzrd->clkout[i] = clk_wzrd_register_divider(&pdev->dev, > + clkout_name, > + clk_name, 0, > + clk_wzrd->base, (WZRD_CLK_CFG_REG(2) + i * 12), Please remove useless parenthesis. > + WZRD_CLKOUT_DIVIDE_SHIFT, > + WZRD_CLKOUT_DIVIDE_WIDTH, > + CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ALLOW_ZERO, > + NULL, &clkwzrd_lock); > if (IS_ERR(clk_wzrd->clkout[i])) { > int j; > > -- > 2.1.1 >
Quoting shubhrajyoti.datta@gmail.com (2019-11-27 22:36:13) > diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/clk-xlnx-clock-wizard.c > index bc0354a..4c6155b 100644 > --- a/drivers/clk/clk-xlnx-clock-wizard.c > +++ b/drivers/clk/clk-xlnx-clock-wizard.c > @@ -493,6 +493,7 @@ static int clk_wzrd_probe(struct platform_device *pdev) > const char *clk_name; > struct clk_wzrd *clk_wzrd; > struct resource *mem; > + int outputs; > struct device_node *np = pdev->dev.of_node; > > clk_wzrd = devm_kzalloc(&pdev->dev, sizeof(*clk_wzrd), GFP_KERNEL); > @@ -583,7 +584,7 @@ static int clk_wzrd_probe(struct platform_device *pdev) > } > > /* register div per output */ > - for (i = WZRD_NUM_OUTPUTS - 1; i >= 0 ; i--) { > + for (i = outputs - 1; i >= 0 ; i--) { Where is 'outputs' assigned in this patch? > const char *clkout_name; > > if (of_property_read_string_index(np, "clock-output-names", i, > @@ -614,7 +615,7 @@ static int clk_wzrd_probe(struct platform_device *pdev) > if (IS_ERR(clk_wzrd->clkout[i])) { > int j; > > - for (j = i + 1; j < WZRD_NUM_OUTPUTS; j++) > + for (j = i + 1; j < outputs; j++) > clk_unregister(clk_wzrd->clkout[j]); > dev_err(&pdev->dev, > "unable to register divider clock\n");
Quoting shubhrajyoti.datta@gmail.com (2019-11-27 22:36:14) > From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > > Update the fixed factor clock registration to register the divisors. > > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > --- > drivers/clk/clk-xlnx-clock-wizard.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/clk-xlnx-clock-wizard.c > index 4c6155b..75ea745 100644 > --- a/drivers/clk/clk-xlnx-clock-wizard.c > +++ b/drivers/clk/clk-xlnx-clock-wizard.c > @@ -491,9 +491,11 @@ static int clk_wzrd_probe(struct platform_device *pdev) > u32 reg, reg_f, mult; > unsigned long rate; > const char *clk_name; > + void __iomem *ctrl_reg; > struct clk_wzrd *clk_wzrd; > struct resource *mem; > int outputs; > + unsigned long flags = 0; > struct device_node *np = pdev->dev.of_node; > > clk_wzrd = devm_kzalloc(&pdev->dev, sizeof(*clk_wzrd), GFP_KERNEL); > @@ -564,19 +566,22 @@ static int clk_wzrd_probe(struct platform_device *pdev) > goto err_disable_clk; > } > > - /* register div */ > - reg = (readl(clk_wzrd->base + WZRD_CLK_CFG_REG(0)) & > - WZRD_DIVCLK_DIVIDE_MASK) >> WZRD_DIVCLK_DIVIDE_SHIFT; > + outputs = of_property_count_strings(np, "clock-output-names"); > + if (outputs == 1) > + flags = CLK_SET_RATE_PARENT; What does the number of clk outputs have to do with the ability to change the rate of a parent clk? The commit text doesn't inform me of what this is for either. Please help us understand. > clk_name = kasprintf(GFP_KERNEL, "%s_mul_div", dev_name(&pdev->dev)); > if (!clk_name) { > ret = -ENOMEM; > goto err_rm_int_clk; > } >
On Mon, Jan 6, 2020 at 1:30 AM Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting shubhrajyoti.datta@gmail.com (2019-11-27 22:36:14) > > From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > > > > Update the fixed factor clock registration to register the divisors. > > > > Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> > > --- > > drivers/clk/clk-xlnx-clock-wizard.c | 17 +++++++++++------ > > 1 file changed, 11 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/clk/clk-xlnx-clock-wizard.c b/drivers/clk/clk-xlnx-clock-wizard.c > > index 4c6155b..75ea745 100644 > > --- a/drivers/clk/clk-xlnx-clock-wizard.c > > +++ b/drivers/clk/clk-xlnx-clock-wizard.c > > @@ -491,9 +491,11 @@ static int clk_wzrd_probe(struct platform_device *pdev) > > u32 reg, reg_f, mult; > > unsigned long rate; > > const char *clk_name; > > + void __iomem *ctrl_reg; > > struct clk_wzrd *clk_wzrd; > > struct resource *mem; > > int outputs; > > + unsigned long flags = 0; > > struct device_node *np = pdev->dev.of_node; > > > > clk_wzrd = devm_kzalloc(&pdev->dev, sizeof(*clk_wzrd), GFP_KERNEL); > > @@ -564,19 +566,22 @@ static int clk_wzrd_probe(struct platform_device *pdev) > > goto err_disable_clk; > > } > > > > - /* register div */ > > - reg = (readl(clk_wzrd->base + WZRD_CLK_CFG_REG(0)) & > > - WZRD_DIVCLK_DIVIDE_MASK) >> WZRD_DIVCLK_DIVIDE_SHIFT; > > + outputs = of_property_count_strings(np, "clock-output-names"); > > + if (outputs == 1) > > + flags = CLK_SET_RATE_PARENT; > > What does the number of clk outputs have to do with the ability to > change the rate of a parent clk? The commit text doesn't inform me of > what this is for either. Please help us understand. If there are multiple clocks then changing the rate of the parent changes the rate of all the outputs so we donot allow changing the rate of the parent if there are multiple clocks. If there is only one output then that is not an issue. I will update the description in the next version. > > > clk_name = kasprintf(GFP_KERNEL, "%s_mul_div", dev_name(&pdev->dev)); > > if (!clk_name) { > > ret = -ENOMEM; > > goto err_rm_int_clk; > > } > >
From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com> In the thread [1] Greg suggested that we move the driver to the clk from the staging. Add patches to address the concerns regarding the fractional and set rate support in the TODO. The patch set does the following - Trivial fixes for kernel doc. - Move the driver to the clk folder - Add capability to set rate. - Add fractional support. - Add support for configurable outputs. - Make the output names unique so that multiple instances do not crib. Changes in the v3: Added the cover-letter. Add patches for rate setting and fractional support Add patches for warning. Remove the driver from staging as suggested [1] https://spinics.net/lists/linux-driver-devel/msg117326.html Shubhrajyoti Datta (10): dt-bindings: add documentation of xilinx clocking wizard clk: clock-wizard: Move the clockwizard to clk clk: clock-wizard: Fix kernel-doc warning clk: clock-wizard: Add support for dynamic reconfiguration clk: clock-wizard: Add support for fractional support clk: clock-wizard: Remove the hardcoding of the clock outputs clk: clock-wizard: Update the fixed factor divisors clk: clock-wizard: Make the output names unique staging: clocking-wizard: Delete the driver from the staging clk: clock-wizard: Fix the compilation failure .../bindings/clock/xlnx,clocking-wizard.txt | 32 + drivers/clk/Kconfig | 6 + drivers/clk/Makefile | 1 + drivers/clk/clk-xlnx-clock-wizard.c | 710 +++++++++++++++++++++ drivers/staging/Kconfig | 2 - drivers/staging/Makefile | 1 - drivers/staging/clocking-wizard/Kconfig | 10 - drivers/staging/clocking-wizard/Makefile | 2 - drivers/staging/clocking-wizard/TODO | 12 - .../clocking-wizard/clk-xlnx-clock-wizard.c | 335 ---------- drivers/staging/clocking-wizard/dt-binding.txt | 30 - 11 files changed, 749 insertions(+), 392 deletions(-) create mode 100644 Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.txt create mode 100644 drivers/clk/clk-xlnx-clock-wizard.c delete mode 100644 drivers/staging/clocking-wizard/Kconfig delete mode 100644 drivers/staging/clocking-wizard/Makefile delete mode 100644 drivers/staging/clocking-wizard/TODO delete mode 100644 drivers/staging/clocking-wizard/clk-xlnx-clock-wizard.c delete mode 100644 drivers/staging/clocking-wizard/dt-binding.txt