Re: [PATCH v1 4/6] clk: microchip: mpfs: fix periph clk parentage

[<Padmarao.Begari@microchip.com>]

> On Tue, 2022-10-25 at 08:58 +0100, Conor Dooley wrote:
> Not all "periph" clocks are children of the AHB clock, some have the
> AXI
> clock as their parent & the mtimer clock is derived from the external
> reference clock directly. Stop assuming the AHB clock to be the
> parent
> of all "periph" clocks and define their correct parents instead.
> 
> Fixes: 2f27c9219e ("clk: Add Microchip PolarFire SoC clock driver")
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  drivers/clk/microchip/mpfs_clk.c        |  4 +-
>  drivers/clk/microchip/mpfs_clk.h        |  4 +-
>  drivers/clk/microchip/mpfs_clk_periph.c | 72 +++++++++++++--------
> ----
>  3 files changed, 42 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/clk/microchip/mpfs_clk.c
> b/drivers/clk/microchip/mpfs_clk.c
> index f16f716f00..08f8bfcecb 100644
> --- a/drivers/clk/microchip/mpfs_clk.c
> +++ b/drivers/clk/microchip/mpfs_clk.c
> @@ -19,7 +19,6 @@
>  static int mpfs_clk_probe(struct udevice *dev)
>  {
>  	struct clk *parent_clk = dev_get_priv(dev);
> -	struct clk clk_ahb = { .id = CLK_AHB };
>  	struct clk clk_msspll = { .id = CLK_MSSPLL };
>  	void __iomem *base;
>  	void __iomem *msspll_base;
> @@ -56,8 +55,7 @@ static int mpfs_clk_probe(struct udevice *dev)
>  	if (ret)
>  		return ret;
>  
> -	clk_request(dev, &clk_ahb);
> -	ret = mpfs_clk_register_periphs(base, &clk_ahb);
> +	ret = mpfs_clk_register_periphs(base, dev);
>  
>  	return ret;
>  }
> diff --git a/drivers/clk/microchip/mpfs_clk.h
> b/drivers/clk/microchip/mpfs_clk.h
> index cb7d303e67..72288cc971 100644
> --- a/drivers/clk/microchip/mpfs_clk.h
> +++ b/drivers/clk/microchip/mpfs_clk.h
> @@ -27,10 +27,10 @@ int mpfs_clk_register_msspll(void __iomem *base,
> struct clk *parent);
>   * mpfs_clk_register_periphs() - register peripheral clocks
>   *
>   * @base: base address of the mpfs system register.
> - * @parent: a pointer to parent clock.
> + * @dev: udevice representing the clock controller.
>   * Return: zero on success, or a negative error code.
>   */
> -int mpfs_clk_register_periphs(void __iomem *base, struct clk
> *parent);
> +int mpfs_clk_register_periphs(void __iomem *base, struct udevice
> *dev);
>  /**
>   * divider_get_val() - get the clock divider value
>   *
> diff --git a/drivers/clk/microchip/mpfs_clk_periph.c
> b/drivers/clk/microchip/mpfs_clk_periph.c
> index 1488ef503e..e23eb552c3 100644
> --- a/drivers/clk/microchip/mpfs_clk_periph.c
> +++ b/drivers/clk/microchip/mpfs_clk_periph.c
> @@ -29,12 +29,14 @@
>  /**
>   * struct mpfs_periph_clock - per instance of peripheral clock
>   * @id: index of a peripheral clock
> + * @parent_id: index of the parent clock
>   * @name: name of a peripheral clock
>   * @shift: shift to a peripheral clock bit field
>   * @flags: common clock framework flags
>   */
>  struct mpfs_periph_clock {
>  	unsigned int id;
> +	unsigned int parent_id;
>  	const char *name;
>  	u8 shift;
>  	unsigned long flags;
> @@ -104,46 +106,47 @@ static ulong mpfs_periph_clk_recalc_rate(struct
> clk *hw)
>  
>  }
>  
> -#define CLK_PERIPH(_id, _name, _shift, _flags) {	\
> +#define CLK_PERIPH(_id, _name, _parent_id, _shift, _flags) {	\
>  		.periph.id = _id,			\
> +		.periph.parent_id = _parent_id,		\
>  		.periph.name = _name,			\
>  		.periph.shift = _shift,			\
>  		.periph.flags = _flags,			\
>  	}
>  
>  static struct mpfs_periph_hw_clock mpfs_periph_clks[] = {
> -	CLK_PERIPH(CLK_ENVM, "clk_periph_envm", 0, CLK_IS_CRITICAL),
> -	CLK_PERIPH(CLK_MAC0, "clk_periph_mac0", 1, 0),
> -	CLK_PERIPH(CLK_MAC1, "clk_periph_mac1", 2, 0),
> -	CLK_PERIPH(CLK_MMC, "clk_periph_mmc", 3, 0),
> -	CLK_PERIPH(CLK_TIMER, "clk_periph_timer", 4, 0),
> -	CLK_PERIPH(CLK_MMUART0, "clk_periph_mmuart0", 5, 0),
> -	CLK_PERIPH(CLK_MMUART1, "clk_periph_mmuart1", 6, 0),
> -	CLK_PERIPH(CLK_MMUART2, "clk_periph_mmuart2", 7, 0),
> -	CLK_PERIPH(CLK_MMUART3, "clk_periph_mmuart3", 8, 0),
> -	CLK_PERIPH(CLK_MMUART4, "clk_periph_mmuart4", 9, 0),
> -	CLK_PERIPH(CLK_SPI0, "clk_periph_spi0", 10, 0),
> -	CLK_PERIPH(CLK_SPI1, "clk_periph_spi1", 11, 0),
> -	CLK_PERIPH(CLK_I2C0, "clk_periph_i2c0", 12, 0),
> -	CLK_PERIPH(CLK_I2C1, "clk_periph_i2c1", 13, 0),
> -	CLK_PERIPH(CLK_CAN0, "clk_periph_can0", 14, 0),
> -	CLK_PERIPH(CLK_CAN1, "clk_periph_can1", 15, 0),
> -	CLK_PERIPH(CLK_USB, "clk_periph_usb", 16, 0),
> -	CLK_PERIPH(CLK_RTC, "clk_periph_rtc", 18, 0),
> -	CLK_PERIPH(CLK_QSPI, "clk_periph_qspi", 19, 0),
> -	CLK_PERIPH(CLK_GPIO0, "clk_periph_gpio0", 20, 0),
> -	CLK_PERIPH(CLK_GPIO1, "clk_periph_gpio1", 21, 0),
> -	CLK_PERIPH(CLK_GPIO2, "clk_periph_gpio2", 22, 0),
> -	CLK_PERIPH(CLK_DDRC, "clk_periph_ddrc", 23, CLK_IS_CRITICAL),
> -	CLK_PERIPH(CLK_FIC0, "clk_periph_fic0", 24, 0),
> -	CLK_PERIPH(CLK_FIC1, "clk_periph_fic1", 25, 0),
> -	CLK_PERIPH(CLK_FIC2, "clk_periph_fic2", 26, 0),
> -	CLK_PERIPH(CLK_FIC3, "clk_periph_fic3", 27, 0),
> -	CLK_PERIPH(CLK_ATHENA, "clk_periph_athena", 28, 0),
> -	CLK_PERIPH(CLK_CFM, "clk_periph_cfm", 29, 0),
> +	CLK_PERIPH(CLK_ENVM, "clk_periph_envm", CLK_AHB, 0,
> CLK_IS_CRITICAL),
> +	CLK_PERIPH(CLK_MAC0, "clk_periph_mac0", CLK_AHB, 1, 0),
> +	CLK_PERIPH(CLK_MAC1, "clk_periph_mac1", CLK_AHB, 2, 0),
> +	CLK_PERIPH(CLK_MMC, "clk_periph_mmc", CLK_AHB, 3, 0),
> +	CLK_PERIPH(CLK_TIMER, "clk_periph_timer", CLK_RTCREF, 4, 0),
> +	CLK_PERIPH(CLK_MMUART0, "clk_periph_mmuart0", CLK_AHB, 5, 0),
> +	CLK_PERIPH(CLK_MMUART1, "clk_periph_mmuart1", CLK_AHB, 6, 0),
> +	CLK_PERIPH(CLK_MMUART2, "clk_periph_mmuart2", CLK_AHB, 7, 0),
> +	CLK_PERIPH(CLK_MMUART3, "clk_periph_mmuart3", CLK_AHB, 8, 0),
> +	CLK_PERIPH(CLK_MMUART4, "clk_periph_mmuart4", CLK_AHB, 9, 0),
> +	CLK_PERIPH(CLK_SPI0, "clk_periph_spi0", CLK_AHB, 10, 0),
> +	CLK_PERIPH(CLK_SPI1, "clk_periph_spi1", CLK_AHB, 11, 0),
> +	CLK_PERIPH(CLK_I2C0, "clk_periph_i2c0", CLK_AHB, 12, 0),
> +	CLK_PERIPH(CLK_I2C1, "clk_periph_i2c1", CLK_AHB, 13, 0),
> +	CLK_PERIPH(CLK_CAN0, "clk_periph_can0", CLK_AHB, 14, 0),
> +	CLK_PERIPH(CLK_CAN1, "clk_periph_can1", CLK_AHB, 15, 0),
> +	CLK_PERIPH(CLK_USB, "clk_periph_usb", CLK_AHB, 16, 0),
> +	CLK_PERIPH(CLK_RTC, "clk_periph_rtc", CLK_AHB, 18, 0),
> +	CLK_PERIPH(CLK_QSPI, "clk_periph_qspi", CLK_AHB, 19, 0),
> +	CLK_PERIPH(CLK_GPIO0, "clk_periph_gpio0", CLK_AHB, 20, 0),
> +	CLK_PERIPH(CLK_GPIO1, "clk_periph_gpio1", CLK_AHB, 21, 0),
> +	CLK_PERIPH(CLK_GPIO2, "clk_periph_gpio2", CLK_AHB, 22, 0),
> +	CLK_PERIPH(CLK_DDRC, "clk_periph_ddrc", CLK_AHB, 23,
> CLK_IS_CRITICAL),
> +	CLK_PERIPH(CLK_FIC0, "clk_periph_fic0", CLK_AXI, 24, 0),
> +	CLK_PERIPH(CLK_FIC1, "clk_periph_fic1", CLK_AXI, 25, 0),
> +	CLK_PERIPH(CLK_FIC2, "clk_periph_fic2", CLK_AXI, 26, 0),
> +	CLK_PERIPH(CLK_FIC3, "clk_periph_fic3", CLK_AXI, 27, 0),
> +	CLK_PERIPH(CLK_ATHENA, "clk_periph_athena", CLK_AXI, 28, 0),
> +	CLK_PERIPH(CLK_CFM, "clk_periph_cfm", CLK_AHB, 29, 0),
>  };
>  
> -int mpfs_clk_register_periphs(void __iomem *base, struct clk
> *parent)
> +int mpfs_clk_register_periphs(void __iomem *base, struct udevice
> *dev)
>  {
>  	int ret;
>  	int i, id, num_clks;
> @@ -152,11 +155,14 @@ int mpfs_clk_register_periphs(void __iomem
> *base, struct clk *parent)
>  
>  	num_clks = ARRAY_SIZE(mpfs_periph_clks);
>  	for (i = 0; i < num_clks; i++)  {
> +		struct clk parent = { .id =
> mpfs_periph_clks[i].periph.parent_id };
> +
> +		clk_request(dev, &parent);
>  		hw = &mpfs_periph_clks[i].hw;
>  		mpfs_periph_clks[i].sys_base = base;
> -		mpfs_periph_clks[i].prate = clk_get_rate(parent);
> +		mpfs_periph_clks[i].prate = clk_get_rate(&parent);
>  		name = mpfs_periph_clks[i].periph.name;
> -		ret = clk_register(hw, MPFS_PERIPH_CLOCK, name, parent-
> >dev->name);
> +		ret = clk_register(hw, MPFS_PERIPH_CLOCK, name,
> parent.dev->name);
>  		if (ret)
>  			ERR_PTR(ret);
>  		id = mpfs_periph_clks[i].periph.id;

Reviewed-by: Padmarao Begari <padmarao.begari@microchip.com>
Tested-by: Padmarao Begari <padmarao.begari@microchip.com>