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@xxxxxxxxx>
> > > ---
> > >
> > > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
