Re: [PATCH v3 9/9] clk: imx: Add the pcc reset controller support on imx8ulp

[Abel Vesa <abel.vesa@nxp.com>]

On 21-09-14 16:07:50, Jacky Bai wrote:
> > Subject: Re: [PATCH v3 9/9] clk: imx: Add the pcc reset controller support on
> > imx8ulp
> > 
> > On 21-09-14 14:52:08, Jacky Bai wrote:
> > > diff --git a/drivers/clk/imx/clk-imx8ulp.c
> > > b/drivers/clk/imx/clk-imx8ulp.c index 6aad04114658..6699437e17b8
> > > 100644
> > > --- a/drivers/clk/imx/clk-imx8ulp.c
> > > +++ b/drivers/clk/imx/clk-imx8ulp.c
> > > @@ -9,6 +9,7 @@
> > >  #include <linux/module.h>
> > >  #include <linux/of_device.h>
> > >  #include <linux/platform_device.h>
> > > +#include <linux/reset-controller.h>
> > >  #include <linux/slab.h>
> > >
> > >  #include "clk.h"
> > > @@ -48,6 +49,99 @@ static const char * const nic_per_divplat[] = {
> > > "nic_per_divplat" };  static const char * const lpav_axi_div[] = {
> > > "lpav_axi_div" };  static const char * const lpav_bus_div[] = {
> > > "lpav_bus_div" };
> > >
> > > +struct pcc_reset_dev {
> > > +	void __iomem *base;
> > > +	struct reset_controller_dev rcdev;
> > > +	const u32 *resets;
> > > +	/* Set to imx_ccm_lock to protect register access shared with clock
> > control */
> > > +	spinlock_t *lock;
> > > +};
> > > +
> > > +#define PCC_SW_RST	BIT(28)
> > > +#define to_pcc_reset_dev(_rcdev)	container_of(_rcdev, struct
> > pcc_reset_dev, rcdev)
> > > +
> > > +static const u32 pcc3_resets[] = {
> > > +	0xa8, 0xac, 0xc8, 0xcc, 0xd0,
> > > +	0xd4, 0xd8, 0xdc, 0xe0, 0xe4,
> > > +	0xe8, 0xec, 0xf0
> > > +};
> > > +
> > > +static const u32 pcc4_resets[] = {
> > > +	0x4, 0x8, 0xc, 0x10, 0x14,
> > > +	0x18, 0x1c, 0x20, 0x24, 0x34,
> > > +	0x38, 0x3c, 0x40, 0x44, 0x48,
> > > +	0x4c, 0x54
> > > +};
> > > +
> > > +static const u32 pcc5_resets[] = {
> > > +	0xa0, 0xa4, 0xa8, 0xac, 0xb0,
> > > +	0xb4, 0xbc, 0xc0, 0xc8, 0xcc,
> > > +	0xd0, 0xf0, 0xf4, 0xf8
> > > +};
> > > +
> > 
> > I believe this could be avoided entirely by having something like:
> > 
> > PCCx_RESETS_OFFSET + (index * 4)
> > 
> > and then:
> > 
> > #define PCC3_RESETS_OFFSET	0xa8
> > #define PCC4_RESETS_OFFSET	0x4
> > #define PCC5_RESETS_OFFSET	0xa0
> > 
> > #define PCC3_RESETS_NUM		13
> > #define PCC4_RESETS_NUM		17
> > #define PCC5_RESETS_NUM		14
> > 
> > Then we could pass on the PCCx_RESETS_OFFSET and the
> > PCCx_RESETS_NUM and that's it.
> > 
> 
> As you can see that the PCC offset that has reset is not consecutive in the register region.
> So it will introduce hole in the index. And we can NOT use linear formula to calculate the offset directly.
> 

You are right, they don't seem to be consecutive, on a closer look.

I don't have any other comments, so:

Reviewed-by: Abel Vesa <abel.vesa@nxp.com>

> BR
> Jacky Bai
> 
> > > +static int imx8ulp_pcc_assert(struct reset_controller_dev *rcdev,
> > > +unsigned long id) {
> > > +	struct pcc_reset_dev *pcc_reset = to_pcc_reset_dev(rcdev);
> > > +	u32 offset = pcc_reset->resets[id];
> > > +	unsigned long flags;
> > > +	u32 val;
> > > +
> > > +	spin_lock_irqsave(pcc_reset->lock, flags);
> > > +
> > > +	val = readl(pcc_reset->base + offset);
> > > +	val &= ~PCC_SW_RST;
> > > +	writel(val, pcc_reset->base + offset);
> > > +
> > > +	spin_unlock_irqrestore(pcc_reset->lock, flags);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int imx8ulp_pcc_deassert(struct reset_controller_dev *rcdev,
> > > +unsigned long id) {
> > > +	struct pcc_reset_dev *pcc_reset = to_pcc_reset_dev(rcdev);
> > > +	u32 offset = pcc_reset->resets[id];
> > > +	unsigned long flags;
> > > +	u32 val;
> > > +
> > > +	spin_lock_irqsave(pcc_reset->lock, flags);
> > > +
> > > +	val = readl(pcc_reset->base + offset);
> > > +	val |= PCC_SW_RST;
> > > +	writel(val, pcc_reset->base + offset);
> > > +
> > > +	spin_unlock_irqrestore(pcc_reset->lock, flags);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct reset_control_ops imx8ulp_pcc_reset_ops = {
> > > +	.assert = imx8ulp_pcc_assert,
> > > +	.deassert = imx8ulp_pcc_deassert,
> > > +};
> > > +
> > > +static int imx8ulp_pcc_reset_init(struct platform_device *pdev, void
> > __iomem *base,
> > > +	 const u32 *resets, unsigned int nr_resets) {
> > > +	struct device_node *np = pdev->dev.of_node;
> > > +	struct device *dev = &pdev->dev;
> > > +	struct pcc_reset_dev *pcc_reset;
> > > +
> > > +	pcc_reset = devm_kzalloc(dev, sizeof(*pcc_reset), GFP_KERNEL);
> > > +	if (!pcc_reset)
> > > +		return -ENOMEM;
> > > +
> > > +	pcc_reset->base = base;
> > > +	pcc_reset->lock = &imx_ccm_lock;
> > > +	pcc_reset->resets = resets;
> > > +	pcc_reset->rcdev.owner = THIS_MODULE;
> > > +	pcc_reset->rcdev.nr_resets = nr_resets;
> > > +	pcc_reset->rcdev.ops = &imx8ulp_pcc_reset_ops;
> > > +	pcc_reset->rcdev.of_node = np;
> > > +
> > > +	return devm_reset_controller_register(dev, &pcc_reset->rcdev); }
> > > +
> > 
> > ...
> > 
> > > --
> > > 2.26.2
> > >