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@xxxxxxx>
> 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
> > >
