[PATCH v4 02/15] clk: Allow drivers to pass in a regmap

[Mike Turquette]

Quoting Stephen Boyd (2014-01-09 23:05:17)
> On 01/09, Mike Turquette wrote:
> > If we're going to use these wrappers, why make it regmap specific? The
> > struct clk_desc patches[1][2] can achieve this, but in a more generic
> > way.
> > 
> 
> I think you're suggesting a way to avoid adding a
> clk_register_regmap() function? But won't we need to write the
> same code:
> 
>         if (dev && dev_get_regmap(dev, NULL))
>                 [clk_type]->regmap = dev_get_regmap(dev, NULL);
>         else if (dev && dev->parent)
>                 [clk_type]->regmap = dev_get_regmap(dev->parent, NULL);
> 
> everytime we want to assign the regmap pointer to a different clock type?
> A macro might work for this little snippet, but it wouldn't have
> any type safety.

Hi Stephen,

Avoiding a new clk_register_regmap() function is one thing worth doing.
But additionally I think that this regmap infrastructure in the core
code still is not the right way forward.

Looking at patch #3 I can already see how those helper functions will
grow just like the basic clock type implementations. Take a look at
drivers/clk/clk-divider.c and check out the HIWORD_MASK stuff. That
really shouldn't belong there, but multiple platforms are using the
Designware IP that needs this so in the Name Of Consolidation it was
done. Basically that code is gross.

A more relevant example: look at the gate clock with a busy bit
implementation in arch/arm/mach-imx/clk-gate2.c. The same could be done
for your clk_enable_regmap() function: someone could subclass it and add
a busy bit or status bit to poll on. But should they? The extra layer of
indirection buys us nothing and takes a clean piece of code and
complicates it.

I'd rather your .enable, .disable and .is_enabled implementations live
in drivers/clk/msm/ so that we can avoid this sort of feature creep in
the future. It's perfectly fine if others implement the same code in
their clock drivers.

With all of that said, then there is no point in keeping struct regmap
*regmap inside of struct clk_hw. It can be moved inside of your struct
clk_rcg, struct clk_branch and struct clk_pll definitions.

> 
> > > 
> > > 
> > > 2) Interfaces: Add a void *data in struct clk_hw that can point to
> > > whatever I want and still have the same clk_regmap_register() and
> > > devm_clk_regmap_register()
> > > 
> > > Example:
> > > 
> > > struct clk_hw {
> > >         struct clk *clk;
> > >         const struct clk_init_data *init;
> > >         void *data;
> > > };
> > > 
> > > struct clk_regmap {
> > >         struct regmap *regmap;
> > >         unsigned int enable_reg;
> > >         unsigned int enable_mask;
> > >         bool enable_is_inverted;
> > > };
> > > 
> > > struct clk_branch {
> > >         u32     hwcg_reg;
> > >         u32     halt_reg;
> > >         u8      hwcg_bit;
> > >         u8      halt_bit;
> > >         u8      halt_check;
> > > 
> > >         struct clk_hw;
> > > };
> > > 
> > > static struct clk_branch gsbi1_uart_clk = {
> > >         .halt_reg = 0x2fcc,
> > >         .halt_bit = 10,
> > >         .hw = {
> > >                 .data = &(struct clk_regmap){
> > >                         .enable_reg = 0x29d4,
> > >                         .enable_mask = BIT(9),
> > >                  };
> > >                 .init = &(struct clk_init_data){
> > >                         .name = "gsbi1_uart_clk",
> > >                         .parent_names = (const char *[]){
> > >                                 "gsbi1_uart_src",
> > >                         },
> > >                         .num_parents = 1,
> > >                         .ops = &clk_branch_ops,
> > >                         .flags = CLK_SET_RATE_PARENT,
> > >                 },
> > >         },
> > > };
> > > 
> > > I guess option 2 is less likely given your comment about clk_hw being
> > > nothing more than a traversal mechanism.
> > 
> > Instead of private data, how about a .register() callback function that
> > can point to anything you like? The clk_desc patches implement this and
> > it would suffice for registering regmap ops or anything else, without
> > polluting struct clk_hw.
> > 
> > [1] http://www.spinics.net/lists/linux-omap/msg101822.html
> > [2] http://www.spinics.net/lists/linux-omap/msg101698.html
> > 
> > So you could statically define gsbi1_uart_clk with:
> > 
> > static struct clk_branch_desc gsbi1_uart_clk_desc = {
> >       .halt_reg = 0x2fcc,
> >       .halt_bit = 10,
> >       .enable_reg = 0x29d4,
> >       .enable_mask = BIT(9),
> >       .desc = {
> >               .name = "gsbi1_uart_clk",
> >               .parent_names = (const char *[]){
> >                       "gsbi1_uart_src",
> >               },
> >               .num_parents = 1,
> >               .ops = &clk_branch_ops,
> >               .flags = CLK_SET_RATE_PARENT,
> >       },
> > };
> > 
> > And then register it with:
> > 
> > clk_register_desc(NULL, &gsbi1_uart_clk_desc.desc);
> > 
> > This is very analogous to the way that you use use &gsbi1_uart_clk.hw
> > but it is more generic and also doesn't pollute clk_hw any further. I
> > also think your static data is quite a bit prettier using this method.
> > 
> > Thoughts?
> 
> Is the plan to allocate a struct clk_branch at runtime and then
> copy all the fields over one by one? I'm trying to avoid that
> because it takes more time and more runtime memory. If I had to
> go the descriptor route I would probably avoid copying any fields
> and just point to the descriptor from struct clk_branch, i.e.
> 
>  struct clk_branch {
>         struct clk_branch_desc *desc;
>         struct clk_hw;
>  };
> 
> but that still seems wasteful to allocate a bunch of little
> pointer wrappers when I could have just embedded the clk_hw
> struct inside the clk_branch struct from the start.
> 

The pointer copy stuff was done so that every instance of struct
clk_init_data could be marked as __initdata.

> It feels another key point is being missed though. The regmap
> pointer and the enable_reg/enable_mask is embedded in clk_hw to
> allow the same code to be used by different types of surrounding
> structs. Each struct: clk_pll, clk_rcg, and clk_branch in this
> series use the regmap interface to enable/disable the clock and
> they can easily do so by passing something that's always
> available from struct clk_hw (be it via a wrapper struct, private
> data member, or addition of new fields to clk_hw). If the regmap
> members move into each specific type of clock we can't just pass
> a single pointer to the enable/disable regmap functions anymore.
> This is the reason why I suggested a driver data pointer or
> container struct so that everything regmap related is contained
> within one type.

I get understand the goal of reuse and I applaud it. But even if we did
pack the regmap data into struct clk_hw it would start to grow out of
control as I mentioned in a previous email. In order for this to work
you have to package more than just struct regmap *regmap into struct
clk_hw. You need the register offset as well as any relevant masks and
bit field values. And then you need more stuff for muxes and dividers.
And hey now you need busy/poll register values and another platform
needs to program shadow registers before flipping the Go Bit ... the
list goes on and on.

So in summary: consolidation is over-rated. You can put your regmap
functions into drivers/clk/msm/ and not convert over to struct clk_desc
by the way, just to remove any confusion on that point.

> 
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation