Re: [PATCH v5 3/4] clk: introduce the common clock framework

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


On Sun, Mar 04, 2012 at 04:12:21PM -0800, Turquette, Mike wrote:
> >>
> >> I believe this patch already does what you suggest, but I might be
> >> missing your point.
> >
> > In include/linux/clk-private.h you expose struct clk outside the core.
> > This has to be done to make static initializers possible. There is a big
> > warning in this file that it must not be included from files implementing
> > struct clk_ops. You can simply avoid this warning by declaring struct clk
> > with only a single member:
> >
> > include/linux/clk.h:
> >
> > struct clk {
> >        struct clk_internal *internal;
> > };
> >
> > This way everybody knows struct clk (thus can embed it in their static
> > initializers), but doesn't know anything about the internal members. Now
> > in drivers/clk/clk.c you declare struct clk_internal exactly like struct
> > clk was declared before:
> >
> > struct clk_internal {
> >        const char              *name;
> >        const struct clk_ops    *ops;
> >        struct clk_hw           *hw;
> >        struct clk              *parent;
> >        char                    **parent_names;
> >        struct clk              **parents;
> >        u8                      num_parents;
> >        unsigned long           rate;
> >        unsigned long           flags;
> >        unsigned int            enable_count;
> >        unsigned int            prepare_count;
> >        struct hlist_head       children;
> >        struct hlist_node       child_node;
> >        unsigned int            notifier_count;
> > #ifdef CONFIG_COMMON_CLK_DEBUG
> >        struct dentry           *dentry;
> > #endif
> > };
> >
> > An instance of struct clk_internal will be allocated in
> > __clk_init/clk_register. Now the private data stays completely inside
> > the core and noone can abuse it.
> 
> Hi Sascha,
> 
> I see the disconnect here.  For OMAP (and possibly other platforms) at
> least some clock data is necessary during early boot, before the
> regular allocation methods are available (timers for instance).

We had this problem on i.MX aswell. It turned out that the timer clock
is the only clock that is needed so early. We solved this by moving the
clock init to the system timer init function.

> Due
> to this my idea of static initialization was to take care of
> everything that would normally require an allocator, which includes
> the internals of struct clk; thus exposing struct clk is useful here
> as you can still use the clock framework during very early boot.  Your
> method above doesn't satisfy this requirement.  I'm not sure what the
> purpose would be of statically allocating your version of struct clk,
> which will ultimately need to allocate memory for the clock internals
> anyways.  Can you elaborate the benefit of this approach over just
> using the clk_foo_register functions?

As said the benefit is that you do not have to expose the internal
layout of struct clk outside the clock framework. Note that in the
file you referenced here:

http://git.linaro.org/gitweb?p=people/mturquette/linux.git;a=blob;f=arch/arm/mach-omap2/clock44xx_data.c;h=7f833a7b2dca84a52c2bd1e7c8d9cfe560771258;hb=v3.3-rc5-clkv5-omap#l205

You violate what you have in a comment above clk-private.h:

/* __clk_init is only exposed via clk-private.h and is intended for use with
 * very large numbers of clocks that need to be statically initialized. It is
 * a layering violation to include clk-private.h from any code which implements
 * a clock's .ops; as such any statically initialized clock data MUST be in
 * separate C file from the logic that implements it's operations.
 */

Well, the file is work in progress, you probably fix this before sending
it out, but I bet people will include clk-private.h and nobody else
notices it.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[Other Archives]     [Linux Kernel Newbies]     [Linux Driver Development]     [Fedora Kernel]     [Linux Kernel Testers]     [Linux SH]     [Linux Omap]     [Linux Kbuild]     [Linux Tape]     [Linux Input]     [Linux Kernel Janitors]     [Linux Kernel Packagers]     [Linux Doc]     [Linux Man Pages]     [Linux API]     [Linux Memory Management]     [Linux Modules]     [Linux Standards]     [Kernel Announce]     [Netdev]     [Git]     [Linux PCI]     Linux CAN Development     [Linux I2C]     [Linux RDMA]     [Linux NUMA]     [Netfilter]     [Netfilter Devel]     [SELinux]     [Bugtraq]     [FIO]     [Linux Perf Users]     [Linux Serial]     [Linux PPP]     [Linux ISDN]     [Linux Next]     [Kernel Stable Commits]     [Linux Tip Commits]     [Kernel MM Commits]     [Linux Security Module]     [Filesystem Development]     [Ext3 Filesystem]     [Linux bcache]     [Ext4 Filesystem]     [Linux BTRFS]     [Linux CEPH Filesystem]     [Linux XFS]     [XFS]     [Linux NFS]     [Linux CIFS]     [Ecryptfs]     [Linux NILFS]     [Linux Cachefs]     [Reiser FS]     [Initramfs]     [Linux FB Devel]     [Linux OpenGL]     [DRI Devel]     [Fastboot]     [Linux RT Users]     [Linux RT Stable]     [eCos]     [Corosync]     [Linux Clusters]     [LVS Devel]     [Hot Plug]     [Linux Virtualization]     [KVM]     [KVM PPC]     [KVM ia64]     [Linux Containers]     [Linux Hexagon]     [Linux Cgroups]     [Util Linux]     [Wireless]     [Linux Bluetooth]     [Bluez Devel]     [Ethernet Bridging]     [Embedded Linux]     [Barebox]     [Linux MMC]     [Linux IIO]     [Sparse]     [Smatch]     [Linux Arch]     [x86 Platform Driver]     [Linux ACPI]     [Linux IBM ACPI]     [LM Sensors]     [CPU Freq]     [Linux Power Management]     [Linmodems]     [Linux DCCP]     [Linux SCTP]     [ALSA Devel]     [Linux USB]     [Linux PA RISC]     [Linux Samsung SOC]     [MIPS Linux]     [IBM S/390 Linux]     [ARM Linux]     [ARM Kernel]     [ARM MSM]     [Tegra Devel]     [Sparc Linux]     [Linux Security]     [Linux Sound]     [Linux Media]     [Video 4 Linux]     [Linux IRDA Users]     [Linux for the blind]     [Linux RAID]     [Linux ATA RAID]     [Device Mapper]     [Linux SCSI]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Linux IDE]     [Linux SMP]     [Linux AXP]     [Linux Alpha]     [Linux M68K]     [Linux ia64]     [Linux 8086]     [Linux x86_64]     [Linux Config]     [Linux Apps]     [Linux MSDOS]     [Linux X.25]     [Linux Crypto]     [DM Crypt]     [Linux Trace Users]     [Linux Btrace]     [Linux Watchdog]     [Utrace Devel]     [Linux C Programming]     [Linux Assembly]     [Dash]     [DWARVES]     [Hail Devel]     [Linux Kernel Debugger]     [Linux gcc]     [Gcc Help]     [X.Org]     [Wine]

Add to Google Powered by Linux

[Older Kernel Discussion]     [Yosemite National Park Forum]     [Large Format Photos]     [Gimp]     [Yosemite Photos]     [Stuff]