[PATCH v2 3/6] clk: samsung: add plls used in s3c2416 and s3c2443

[tomasz.figa@gmail.com (Tomasz Figa)]

On Thursday 11 of July 2013 10:50:39 Heiko St?bner wrote:
> Am Donnerstag, 11. Juli 2013, 10:16:41 schrieb Tomasz Figa:
> > Hi Heiko,
> > 
> > On Wednesday 10 of July 2013 00:59:08 Heiko St?bner wrote:
> > > This adds support for pll2126x, pll3000x, pll6552x and pll6553x.
> > > 
> > > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > > ---
> > > 
> > >  drivers/clk/samsung/clk-pll.c |  280
> > > 
> > > +++++++++++++++++++++++++++++++++++++++++ drivers/clk/samsung/clk-pll.h
> > > |
> > > 
> > >  8 ++
> > >  2 files changed, 288 insertions(+)
> > 
> > Generally the patch looks good, but I have some comments to the part
> > related to 655xx PLLs.
> > 
> > I had a patch adding support for them too, but we can go with yours, since
> > the way of registration has been changed by Yadwinder's patches and mine
> > would have to be updated anyway.
> > 
> > > diff --git a/drivers/clk/samsung/clk-pll.c
> > > b/drivers/clk/samsung/clk-pll.c index 0afaec6..35c15a1 100644
> > > --- a/drivers/clk/samsung/clk-pll.c
> > > +++ b/drivers/clk/samsung/clk-pll.c
> > > @@ -323,6 +323,73 @@ struct clk * __init
> > > samsung_clk_register_pll46xx(const char *name, }
> > > 
> > >  /*
> > > 
> > > + * PLL2126x Clock Type
> > > + */
> > > +
> > > +#define PLL2126X_MDIV_MASK	(0xFF)
> > > +#define PLL2126X_PDIV_MASK	(0x3)
> > > +#define PLL2126X_SDIV_MASK	(0x3)
> > > +#define PLL2126X_MDIV_SHIFT	(16)
> > > +#define PLL2126X_PDIV_SHIFT	(8)
> > > +#define PLL2126X_SDIV_SHIFT	(0)
> 
> +#define PLL2126X_PDIV_MASK	(0x3F)
> 
> is the correct value.
> 
> > > +
> > > +struct samsung_clk_pll2126x {
> > > +	struct clk_hw		hw;
> > > +	const void __iomem	*con_reg;
> > > +};
> > > +
> > > +#define to_clk_pll2126x(_hw) container_of(_hw, struct
> > > samsung_clk_pll2126x, hw) +
> > > +static unsigned long samsung_pll2126x_recalc_rate(struct clk_hw *hw,
> > > +				unsigned long parent_rate)
> > > +{
> > > +	struct samsung_clk_pll2126x *pll = to_clk_pll2126x(hw);
> > > +	u32 pll_con, mdiv, pdiv, sdiv;
> > > +	u64 fvco = parent_rate;
> > > +
> > > +	pll_con = __raw_readl(pll->con_reg);
> > > +	mdiv = (pll_con >> PLL2126X_MDIV_SHIFT) & PLL2126X_MDIV_MASK;
> > > +	pdiv = (pll_con >> PLL2126X_PDIV_SHIFT) & PLL2126X_PDIV_MASK;
> > > +	sdiv = (pll_con >> PLL2126X_SDIV_SHIFT) & PLL2126X_SDIV_MASK;
> > > +
> > > +	fvco *= (mdiv + 8);
> > > +	do_div(fvco, (pdiv + 2) << sdiv);
> > > +
> > > +	return (unsigned long)fvco;
> > > +}
> > > +
> > > +static const struct clk_ops samsung_pll2126x_clk_ops = {
> > > +	.recalc_rate = samsung_pll2126x_recalc_rate,
> > > +};
> > > +
> > > +struct clk * __init samsung_clk_register_pll2126x(const char *name,
> > > +			const char *pname, const void __iomem *con_reg)
> > > +{
> > > +	struct samsung_clk_pll2126x *pll;
> > > +	struct clk *clk;
> > > +	struct clk_init_data init;
> > > +
> > > +	pll = kzalloc(sizeof(*pll), GFP_KERNEL);
> > > +	if (!pll)
> > > +		return ERR_PTR(-ENOMEM);
> > > +
> > > +	init.name = name;
> > > +	init.ops = &samsung_pll2126x_clk_ops;
> > > +	init.flags = CLK_GET_RATE_NOCACHE;
> > > +	init.parent_names = &pname;
> > > +	init.num_parents = 1;
> > > +
> > > +	pll->hw.init = &init;
> > > +	pll->con_reg = con_reg;
> > > +
> > > +	clk = samsung_register_pll(&pll->hw);
> > > +	if (IS_ERR(clk))
> > > +		kfree(pll);
> > > +
> > > +	return clk;
> > > +}
> > > +
> > > +/*
> > > 
> > >   * PLL2550x Clock Type
> > >   */
> > > 
> > > @@ -396,3 +463,216 @@ struct clk * __init
> > > samsung_clk_register_pll2550x(const char *name,
> > > 
> > >  	return clk;
> > >  
> > >  }
> > > 
> > > +
> > > +/*
> > > + * PLL3000x Clock Type
> > > + */
> > > +
> > > +#define PLL3000X_MDIV_MASK	(0xFF)
> > > +#define PLL3000X_PDIV_MASK	(0x3)
> > > +#define PLL3000X_SDIV_MASK	(0x3)
> > > +#define PLL3000X_MDIV_SHIFT	(16)
> > > +#define PLL3000X_PDIV_SHIFT	(8)
> > > +#define PLL3000X_SDIV_SHIFT	(0)
> 
> these are correct.
> 
> > > +
> > > +struct samsung_clk_pll3000x {
> > > +	struct clk_hw		hw;
> > > +	const void __iomem	*con_reg;
> > > +};
> > > +
> > > +#define to_clk_pll3000x(_hw) container_of(_hw, struct
> > > samsung_clk_pll3000x, hw) +
> > > +static unsigned long samsung_pll3000x_recalc_rate(struct clk_hw *hw,
> > > +				unsigned long parent_rate)
> > > +{
> > > +	struct samsung_clk_pll3000x *pll = to_clk_pll3000x(hw);
> > > +	u32 pll_con, mdiv, pdiv, sdiv;
> > > +	u64 fvco = parent_rate;
> > > +
> > > +	pll_con = __raw_readl(pll->con_reg);
> > > +	mdiv = (pll_con >> PLL3000X_MDIV_SHIFT) & PLL3000X_MDIV_MASK;
> > > +	pdiv = (pll_con >> PLL3000X_PDIV_SHIFT) & PLL3000X_PDIV_MASK;
> > > +	sdiv = (pll_con >> PLL3000X_SDIV_SHIFT) & PLL3000X_SDIV_MASK;
> > > +
> > > +	fvco *= (2 * (mdiv + 8));
> > > +	do_div(fvco, pdiv << sdiv);
> > > +
> > > +	return (unsigned long)fvco;
> > > +}
> > > +
> > > +static const struct clk_ops samsung_pll3000x_clk_ops = {
> > > +	.recalc_rate = samsung_pll3000x_recalc_rate,
> > > +};
> > > +
> > > +struct clk * __init samsung_clk_register_pll3000x(const char *name,
> > > +			const char *pname, const void __iomem *con_reg)
> > > +{
> > > +	struct samsung_clk_pll3000x *pll;
> > > +	struct clk *clk;
> > > +	struct clk_init_data init;
> > > +
> > > +	pll = kzalloc(sizeof(*pll), GFP_KERNEL);
> > > +	if (!pll)
> > > +		return ERR_PTR(-ENOMEM);
> > > +
> > > +	init.name = name;
> > > +	init.ops = &samsung_pll3000x_clk_ops;
> > > +	init.flags = CLK_GET_RATE_NOCACHE;
> > > +	init.parent_names = &pname;
> > > +	init.num_parents = 1;
> > > +
> > > +	pll->hw.init = &init;
> > > +	pll->con_reg = con_reg;
> > > +
> > > +	clk = samsung_register_pll(&pll->hw);
> > > +	if (IS_ERR(clk))
> > > +		kfree(pll);
> > > +
> > > +	return clk;
> > > +}
> > > +
> > > +/*
> > > + * PLL6552x Clock Type
> > > + */
> > > +
> > > +#define PLL6552X_MDIV_MASK	(0x3FF)
> > > +#define PLL6552X_PDIV_MASK	(0x3F)
> > > +#define PLL6552X_SDIV_MASK	(0x7)
> > > +#define PLL6552X_MDIV_SHIFT	(14)
> > > +#define PLL6552X_PDIV_SHIFT	(5)
> > > +#define PLL6552X_SDIV_SHIFT	(0)
> > 
> > Are you sure about those bitfields?
> > 
> > In S3C6410 User's Manual they are different. You can look at my patch for
> > a
> > comparison:
> > 
> > http://thread.gmane.org/gmane.linux.usb.general/87571/focus=88344
> 
> The numbers where taken from the previous pll code, but I now again checked
> them against the datasheet of the s3c2416 and the s3c2450.
> 
> When comparing with your patch, it really seems that the bit offsets in the
> register are different for the pdiv and mdiv - the above values are correct
> according the the datasheet (and also produce the expected results in the
> clock tree).

> > > +struct samsung_clk_pll6552x {
> > > +	struct clk_hw		hw;
> > > +	const void __iomem	*con_reg;
> > > +};
> > > +
> > > +#define to_clk_pll6552x(_hw) container_of(_hw, struct
> > > samsung_clk_pll6552x, hw) +
> > > +static unsigned long samsung_pll6552x_recalc_rate(struct clk_hw *hw,
> > > +				unsigned long parent_rate)
> > > +{
> > > +	struct samsung_clk_pll6552x *pll = to_clk_pll6552x(hw);
> > > +	u32 pll_con, mdiv, pdiv, sdiv;
> > > +	u64 fvco = parent_rate;
> > > +
> > > +	pll_con = __raw_readl(pll->con_reg);
> > > +	mdiv = (pll_con >> PLL6552X_MDIV_SHIFT) & PLL6552X_MDIV_MASK;
> > > +	pdiv = (pll_con >> PLL6552X_PDIV_SHIFT) & PLL6552X_PDIV_MASK;
> > > +	sdiv = (pll_con >> PLL6552X_SDIV_SHIFT) & PLL6552X_SDIV_MASK;
> > > +
> > > +	fvco *= mdiv;
> > > +	do_div(fvco, (pdiv << sdiv));
> > > +
> > > +	return (unsigned long)fvco;
> > > +}
> > > +
> > > +static const struct clk_ops samsung_pll6552x_clk_ops = {
> > > +	.recalc_rate = samsung_pll6552x_recalc_rate,
> > > +};
> > > +
> > > +struct clk * __init samsung_clk_register_pll6552x(const char *name,
> > > +			const char *pname, const void __iomem *con_reg)
> > > +{
> > > +	struct samsung_clk_pll6552x *pll;
> > > +	struct clk *clk;
> > > +	struct clk_init_data init;
> > > +
> > > +	pll = kzalloc(sizeof(*pll), GFP_KERNEL);
> > > +	if (!pll)
> > > +		return ERR_PTR(-ENOMEM);
> > > +
> > > +	init.name = name;
> > > +	init.ops = &samsung_pll6552x_clk_ops;
> > > +	init.flags = CLK_GET_RATE_NOCACHE;
> > > +	init.parent_names = &pname;
> > > +	init.num_parents = 1;
> > > +
> > > +	pll->hw.init = &init;
> > > +	pll->con_reg = con_reg;
> > > +
> > > +	clk = samsung_register_pll(&pll->hw);
> > > +	if (IS_ERR(clk))
> > > +		kfree(pll);
> > > +
> > > +	return clk;
> > > +}
> > > +
> > > +/*
> > > + * PLL6553x Clock Type
> > > + */
> > > +
> > > +#define PLL6553X_MDIV_MASK	(0x7F)
> > > +#define PLL6553X_PDIV_MASK	(0x1F)
> > > +#define PLL6553X_SDIV_MASK	(0x3)
> > > +#define PLL6553X_KDIV_MASK	(0xFFFF)
> > > +#define PLL6553X_MDIV_SHIFT	(16)
> > > +#define PLL6553X_PDIV_SHIFT	(8)
> > > +#define PLL6553X_SDIV_SHIFT	(0)
> > 
> > Same about those bitfields. They seem to be different on S3C64xx.
> 
> Here it seems the values were off in the original code. According to the
> datasheet the values in your patch are correct. Thanks for the catch.
> 
> +#define PLL6553X_MDIV_MASK	(0xFF)
> +#define PLL6553X_PDIV_MASK	(0x3F)
> +#define PLL6553X_SDIV_MASK	(0x7)
> 
> 
> This leaves the problem on what to do with the 6552X and its different bit
> offsets. Is the pll in question really a 6552X? In the s3c2416 manual its
> name is explicitly stated.

So is it in S3C6410 User's Manual.

If you look at the old plat/pll.h header, you can see that none of S3C2416_PLL 
(which I believe corresponds to your 6552X) and S3C6400_PLL (which is the 
PLL6552x that can be found on S3C64xx) is labelled as PLL6552x explicitly.

Best regards,
Tomasz