Hi Geert-san,
Thank you for your review!
> From: Geert Uytterhoeven, Sent: Tuesday, September 8, 2020 8:20 PM
>
> Hi Shimoda-san,
>
> On Mon, Sep 7, 2020 at 11:20 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@xxxxxxxxxxx> wrote:
> > Add support for R-Car V3U (R8A779A0) SoC power areas and
> > register access, because register specification differs
> > than R-Car Gen2/3.
> >
> > Inspired by patches in the BSP by Tho Vu.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
>
> Thanks for your patch!
>
> > --- /dev/null
> > +++ b/drivers/soc/renesas/r8a779a0-sysc.c
> > @@ -0,0 +1,460 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Renesas R-Car V3U System Controller
> > + *
> > + * Copyright (C) 2020 Renesas Electronics Corp.
> > + */
> > +
> > +#include <linux/bits.h>
> > +#include <linux/clk/renesas.h>
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mm.h>
> > +#include <linux/of_address.h>
> > +#include <linux/pm_domain.h>
> > +#include <linux/slab.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/io.h>
> > +#include <linux/soc/renesas/rcar-sysc.h>
> > +#include <linux/sys_soc.h>
> > +#include <linux/syscore_ops.h>
>
> The above 3 includes are not needed.
I got it.
> > +
> > +#include <dt-bindings/power/r8a779a0-sysc.h>
> > +
> > +#include "rcar-sysc.h"
>
> You don't reuse the rcar-sysc driver itself, but you do reuse its header
> file. As the comments in rcar-sysc.h refer to registers that have been
> renamed (e.g. PWR*), and SYSCEXTMASK no longer exists, it might be
> easier for the casual reader to drop the include, copy the PD_*
> definitions, and define new r8a779a0_sysc_area and r8a779a0_sysc_info
> structures instead, using the new naming.
I understood it. I'll do that.
> > +
> > +static struct rcar_sysc_area r8a779a0_areas[] __initdata = {
> > + { "always-on", 0, 0, R8A779A0_PD_ALWAYS_ON, -1, PD_ALWAYS_ON },
> > + { "a3e0", 0x1500, 0, R8A779A0_PD_A3E0, R8A779A0_PD_ALWAYS_ON, PD_SCU },
>
> I think you can drop:
> - chan_offs: it's always 0x1000 + pdr * 64,
> - chan_bit: it's always zero,
I got it.
> > +/* SYSC Common */
> > +#define SYSCSR 0x000 /* SYSC Status Register */
> > +#define SYSCPONSR(x) (0x800 + ((x) * 0x4)) /* Power-ON Status Register 0 */
> > +#define SYSCPOFFSR(x) (0x808 + ((x) * 0x4)) /* Power-OFF Status Register */
> > +#define SYSCISCR(x) (0x810 + ((x) * 0x4)) /* Interrupt Status/Clear Register */
> > +#define SYSCIER(x) (0x820 + ((x) * 0x4)) /* Interrupt Enable Register */
> > +#define SYSCIMR(x) (0x830 + ((x) * 0x4)) /* Interrupt Mask Register */
> > +
> > +/* Power Domain Registers */
> > +#define PDRSR(n) (0x1000 + ((n) * 0x40))
> > +#define PDRONCR(n) (0x1004 + ((n) * 0x40))
> > +#define PDROFFCR(n) (0x1008 + ((n) * 0x40))
> > +#define PDRESR(n) (0x100C + ((n) * 0x40))
>
> While PDRESRn is described, and shown in a figure, it was forgotten in
> the Table 9.2 ("List of Registers (Power Domain Registers for each power
> domain)").
You're right.
> > +
> > +/* Power State */
> > +#define PW_ACTIVE 1 /* Active setting */
>
> "/* PWRON/PWROFF */"?
I'll fix these lines like below:
/* PWRON/PWROFF */
#define PWRON_PWROFF BIT(0) /* Power-ON/OFF request */
> > +
> > +/* PDRSR */
> > +#define PDRSR_OFF BIT(0) /* Power-OFF state */
> > +#define PDRSR_ON BIT(4) /* Power-ON state */
> > +#define PDRSR_OFF_STATE BIT(8) /* Processing Power-OFF sequence */
> > +#define PDRSR_ON_STATE BIT(12) /* Processing Power-ON sequence */
> > +
> > +#define SYSCSR_PONENB 1 /* Ready for power resume requests */
> > +#define SYSCSR_POFFENB 0 /* Ready for power shutoff requests */
>
> These two bits are now combined into a single BUSY bit mask:
> (doh, all bits sets is not busy?!?)
>
> #define SYSCSR_BUSY GENMASK(1, 0) /* All bit sets is not busy */
I got it. I'll fix it.
> > +
> > +#define SYSCSR_RETRIES 1000
> > +#define SYSCSR_DELAY_US 10
> > +
> > +#define PDRESR_RETRIES 1000
> > +#define PDRESR_DELAY_US 10
> > +
> > +#define SYSCISR_RETRIES 1000
> > +#define SYSCISR_DELAY_US 10
> > +
> > +#define R8A779A0_NUM_PD_ALWAYS_ON 64 /* Always-on power area */
>
> Just use R8A779A0_PD_ALWAYS_ON instead?
I'll fix it.
> > +
> > +#define NUM_DOMAINS_EACH_REG 32
>
> BITS_PER_TYPE(u32)?
I'll fix it.
> > +
> > +struct rcar_sysc_ch {
> > + u16 chan_offs;
> > + u8 chan_bit;
> > + u8 isr_bit;
> > +};
>
> As chan_offs is unused, and chan_bit is always zero, I think all use of
> this struct can just be replaced by "unsigned int pdr"?
I'll fix it.
> > +
> > +static void __iomem *rcar_sysc_base;
>
> s/rcar/r8a779a0/ everywhere?
I think so. I'll rename it.
> > +static DEFINE_SPINLOCK(rcar_sysc_lock); /* SMP CPUs + I/O devices */
> > +
> > +static int rcar_sysc_pwr_on_off(const struct rcar_sysc_ch *sysc_ch, bool on)
> > +{
> > + unsigned int sr_bit, reg_offs;
>
> sr_bit is no longer needed.
I'll drop it.
> > + int k;
> > +
> > + if (on) {
> > + sr_bit = SYSCSR_PONENB;
> > + reg_offs = PDRONCR(sysc_ch->isr_bit);
> > + } else {
> > + sr_bit = SYSCSR_POFFENB;
> > + reg_offs = PDROFFCR(sysc_ch->isr_bit);
> > + }
> > +
> > + /* Wait until SYSC is ready to accept a power request */
> > + for (k = 0; k < SYSCSR_RETRIES; k++) {
> > + if (ioread32(rcar_sysc_base + SYSCSR) & BIT(sr_bit))
>
> if ((ioread32(rcar_sysc_base + SYSCSR) & SYSCSR_BUSY) == SYSCSR_BUSY)
>
> > + break;
> > + udelay(SYSCSR_DELAY_US);
> > + }
>
> Perhaps you can use readl_poll_timeout()?
I think so. I'll fix it.
> > +
> > + if (k == SYSCSR_RETRIES)
> > + return -EAGAIN;
> > +
> > + /* Submit power shutoff or power resume request */
> > + iowrite32(PW_ACTIVE, rcar_sysc_base + reg_offs);
> > +
> > + return 0;
> > +}
> > +
> > +static int clear_irq_flags(unsigned int reg_idx, unsigned int isr_mask)
> > +{
> > + int k = 0;
> > +
> > + iowrite32(isr_mask, rcar_sysc_base + SYSCISCR(reg_idx));
> > +
> > + for (k = 0; k < SYSCISR_RETRIES; k++) {
> > + if ((ioread32(rcar_sysc_base + SYSCISCR(reg_idx)) & isr_mask) == 0)
> > + break;
> > +
> > + udelay(SYSCISR_DELAY_US);
> > + }
>
> readl_poll_timeout()?
Yes, I'll use it.
> > +
> > + if (k == SYSCISR_RETRIES) {
> > + pr_err("\n %s : Can not clear IRQ flags in SYSCISCR", __func__);
> > + return -EIO;
> > + }
> > +
> > + return 0;
> > +}
>
> > +static bool has_cpg_mstp;
>
> Please drop this and all related code, R-Car V3U does not use the legacy
> CPG/MSSR PM Domain.
I'll drop it.
> > +static const struct of_device_id rcar_sysc_matches[] __initconst = {
> > +#ifdef CONFIG_SYSC_R8A779A0
>
> Please drop the #ifdef, as compilation of this file already depends on
> this symbol.
Oops. I got it.
Best regards,
Yoshihiro Shimoda
