* Tero Kristo <t-kristo@xxxxxx> [150326 03:55]:
> On 03/26/2015 09:24 AM, Tero Kristo wrote:
> >On 03/26/2015 01:17 AM, Tony Lindgren wrote:
> >>* Tero Kristo <t-kristo@xxxxxx> [150325 08:12]:
> >>>
> >>>Splits the clock provider init out of the PRM driver and moves it to
> >>>clock driver. This is needed so that once the PRCM drivers are
> >>>separated,
> >>>they can logically just access the clock driver not needing to go
> >>>through
> >>>common PRM code. This would be wrong in the case of control module for
> >>>example.
> >>...
> >>
> >>>--- a/arch/arm/mach-omap2/clock.c
> >>>+++ b/arch/arm/mach-omap2/clock.c
> >>...
> >>>-u32 omap2_clk_readl(struct clk_hw_omap *clk, void __iomem *reg)
> >>>+u32 omap2_clk_memmap_readl(void __iomem *reg)
> >>> {
> >>>- u32 val;
> >>>+ struct clk_omap_reg *r = (struct clk_omap_reg *)®
> >>>
> >>>- if (clk->flags & MEMMAP_ADDRESSING) {
> >>>- struct clk_omap_reg *r = (struct clk_omap_reg *)®
> >>>- val = readl_relaxed(clk_memmaps[r->index] + r->offset);
> >>>- } else {
> >>>- val = readl_relaxed(reg);
> >>>- }
> >>>+ return readl_relaxed(clk_memmaps[r->index] + r->offset);
> >>>+}
> >>
> >>The cast from void __iomem *reg to struct clk_omap_reg *r looks still
> >>nasty.. Why don't you add the IO address into struct clk_omap_reg:
> >>
> >>struct clk_omap_reg {
> >> u16 offset;
> >> u16 index;
> >> struct regmap *regmap;
> >> void __iomem *addr;
> >>};
> >>...
> >>
> >>Then populate it during init and then have the clock code use it
> >>directly if available? Then it seems you would not need the
> >>static struct clk_iomap *clk_memmaps[CLK_MAX_MEMMAPS] at all?
> >
> >Doing a change like this should probably be planned, but it is a larger
> >modification. Currently none of the low-level clock APIs support this,
> >but instead expect a direct iomem pointer against which they can do
> >arithmetic operations. The major problem is the companion clocks, which
> >just XOR some bits in the registers to get ICLK / IDLEST register offset
> >from FCLK.
> >
> >So, for now, clock code just uses the void __iomem pointer as a storage
> >class for struct clk_omap_reg, on which arithmetic operations can be done.
Well how about keep the check if (clk->flags & MEMMAP_ADDRESSING) at
least? Maybe WARN_ON(!(clk->flags & MEMMAP_ADDRESSING))?
Otherwise this could be a nightmare to debug if anything goes wrong.
> I did this change as a trial, and this is the diff required to get it
> working:
>
> arch/arm/mach-omap2/clkt_iclk.c | 20 ++++++++---------
> arch/arm/mach-omap2/clock.c | 47
> +++++++++++++++++----------------------
> arch/arm/mach-omap2/clock.h | 8 +++----
> arch/arm/mach-omap2/clock2430.c | 5 +++--
> arch/arm/mach-omap2/clock34xx.c | 36 ++++++++++++++----------------
> arch/arm/mach-omap2/clock3517.c | 20 ++++++++---------
> arch/arm/mach-omap2/cm.h | 4 +++-
> arch/arm/mach-omap2/cm2xxx.c | 9 +++-----
> arch/arm/mach-omap2/cm3xxx.c | 10 +++------
> drivers/clk/ti/clk.c | 10 +++++----
> drivers/clk/ti/divider.c | 24 ++++++++++++++------
> drivers/clk/ti/dpll.c | 11 ++++-----
> drivers/clk/ti/gate.c | 21 +++++++++++------
> drivers/clk/ti/interface.c | 9 ++++----
> drivers/clk/ti/mux.c | 22 ++++++++++++------
> include/linux/clk/ti.h | 5 +++--
> 16 files changed, 137 insertions(+), 124 deletions(-)
>
> I think we should probably keep this out of this set now and do this while
> moving the OMAP core clock support code under clock driver... just to keep
> it more easily manageable.
OK fine with me to do that as a follow-up patch.
Regards,
Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
