Re: [PATCH v8 04/16] clk: npcm8xx: add clock controller

[Stephen Boyd <sboyd@kernel.org>]

Quoting Tomer Maimon (2022-07-12 00:28:30)
> On Mon, 11 Jul 2022 at 22:55, Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Tomer Maimon (2022-07-11 05:35:07)
> > > +        */
> > > +       int onecell_idx;
> > > +};
> > > +
> > > +struct npcm8xx_clk_pll_data {
> > > +       u32 reg;
> > > +       const char *name;
> > > +       const char *parent_name;
> >
> > Any reason why we're not using clk_parent_data or direct clk_hw
> > pointers?
> For more historical reasons, I did the same method as done in the
> NPCM7XX driver.
> The clk_init_data struct can use * const *parent_names,
> https://elixir.bootlin.com/linux/v5.19-rc6/source/include/linux/clk-provider.h#L289
> Is it problematic?

It will need to be changed to not use global string matching. Ideally
new drivers use clk_parent_data or clk_hw pointers directly. It's faster
and preferred.

> >
> > > +       NPCM8XX_CLK_S_AHB, CLK_DIVIDER_READ_ONLY, 0, NPCM8XX_CLK_SPI0},
> > > +       /* bit 10-6 SPI0CKDV*/
> > > +       {NPCM8XX_CLKDIV3, 1, 5, NPCM8XX_CLK_S_SPIX,
> > > +       NPCM8XX_CLK_S_AHB, CLK_DIVIDER_READ_ONLY, 0, NPCM8XX_CLK_SPIX},
> > > +       /* bit 5-1 SPIXCKDV*/
> > > +
> > > +       {NPCM8XX_CLKDIV4, 28, 4, NPCM8XX_CLK_S_RG, NPCM8XX_CLK_S_RG_MUX,
> > > +       CLK_DIVIDER_READ_ONLY, 0, NPCM8XX_CLK_RG},
> > > +       /* bit 31-28 RGREFDIV*/
> > > +       {NPCM8XX_CLKDIV4, 12, 4, NPCM8XX_CLK_S_RCP, NPCM8XX_CLK_S_RCP_MUX,
> > > +       CLK_DIVIDER_READ_ONLY, 0, NPCM8XX_CLK_RCP},
> > > +       /* bit 15-12 RCPREFDIV*/
> > > +       {NPCM8XX_THRTL_CNT, 0, 2, NPCM8XX_CLK_S_TH, NPCM8XX_CLK_S_CPU_MUX,
> > > +       CLK_DIVIDER_READ_ONLY | CLK_DIVIDER_POWER_OF_TWO, 0, NPCM8XX_CLK_TH},
> > > +       /* bit 1-0 TH_DIV*/
> > > +};
> > > +
> > > +static DEFINE_SPINLOCK(npcm8xx_clk_lock);
> > > +
> > > +static int npcm8xx_clk_probe(struct platform_device *pdev)
> > > +{
> > > +       struct clk_hw_onecell_data *npcm8xx_clk_data;
> > > +       struct device *dev = &pdev->dev;
> > > +       struct device_node *np = dev->of_node;
> > > +       void __iomem *clk_base;
> > > +       struct resource res;
> > > +       struct clk_hw *hw;
> > > +       int i, err;
> > > +
> > > +       npcm8xx_clk_data = devm_kzalloc(dev, struct_size(npcm8xx_clk_data, hws,
> > > +                                                        NPCM8XX_NUM_CLOCKS),
> > > +                                       GFP_KERNEL);
> > > +       if (!npcm8xx_clk_data)
> > > +               return -ENOMEM;
> > > +
> > > +       err = of_address_to_resource(np, 0, &res);
> >
> > Why can't we use platform_get_resource()?
> >
> > > +       if (err) {
> > > +               dev_err(dev, "Failed to get resource, ret %d\n", err);
> > > +               return err;
> > > +       }
> > > +
> > > +       clk_base = ioremap(res.start, resource_size(&res));
> >
> > And use devm_platform_ioremap_resource()?
> Clock and reset driver use the same memory register map 0xF0801000 - 0xF0801FFF.
> For historical reasons the registers of both modules are mixed in the
> memory range 0xF0801000 - 0xF0801FFF this is why we can't have a
> separate region for each module.
> In case I will use devm_platform_ioremap_resource function the reset
> ioremap will fail so the driver using the method above.

So the clk and reset driver should be the same driver, or one driver
should register the other and use the auxiliary bus to express the
relationship. That way we know that the drivers are tightly coupled and
aren't going to stomp over each other.

> >
> > > +       if (!clk_base) {
> > > +               dev_err(&pdev->dev, "Failed to remap I/O memory\n");
> > > +               return -ENOMEM;
> > > +       }
> > > +
> > > +       npcm8xx_clk_data->num = NPCM8XX_NUM_CLOCKS;
> > > +
> > > +       for (i = 0; i < NPCM8XX_NUM_CLOCKS; i++)
> > > +               npcm8xx_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER);
> > > +
> > > +       /* Reference 25MHz clock */
> >
> > Does this exist on the board? If so, I'd make a fixed rate clk in the
> > dts and have 'refclk' be an input in the binding for this clk controller.
> No, it is an internal clock in the SoC, this is why it is in the driver.

Ok. I suppose that could be inside the 'soc' node for this device as a
fixed rate clk but registering it here is also fine.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel