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

[Tomer Maimon]

Hi Stephen,

Thanks for your reply.

On Mon, 18 Jul 2022 at 22:14, Stephen Boyd <sboyd at kernel.org> wrote:
>
> Quoting Tomer Maimon (2022-07-12 00:28:30)
> > On Mon, 11 Jul 2022 at 22:55, Stephen Boyd <sboyd at 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.
Will be modified V10.
>
> > >
> > > > +       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.
I think it is very problematic to use the same driver for the reset
and the clocks also because The NPCM reset driver is an old driver
that was used also to the older NPCM BMC SoC so it will be problematic
to use the clock and reset driver in the same space.
indeed the reset and clocks are using the same memory region but they
are not using the same registers, is it not enough?
Please be aware that the NPCM reset driver is checking that it is
using the reset registers before calling I/O functions.
> > >
> > > > +       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.

Best regards,

Tomer