Re: [PATCH v5 05/13] peci: Add peci-aspeed controller driver

[Joel Stanley]

On Wed, 12 Jan 2022 at 23:06, Iwona Winiarska <iwona.winiarska@xxxxxxxxx> wrote:
>
> From: Jae Hyun Yoo <jae.hyun.yoo@xxxxxxxxxxxxxxx>
>
> ASPEED AST24xx/AST25xx/AST26xx SoCs support the PECI electrical
> interface (a.k.a PECI wire) that provides a communication channel with
> Intel processors.
> This driver allows BMC to discover devices connected to it and
> communicate with them using PECI protocol.
>
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@xxxxxxxxxxxxxxx>
> Co-developed-by: Iwona Winiarska <iwona.winiarska@xxxxxxxxx>
> Signed-off-by: Iwona Winiarska <iwona.winiarska@xxxxxxxxx>
> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>

The driver looks good to me. I would be happy to see it merged in its
current state.

Reviewed-by: Joel Stanley <joel@xxxxxxxxx>

I've a few questions below that can be followed up later if need be.

> +
> +static void aspeed_peci_init_regs(struct aspeed_peci *priv)
> +{
> +       u32 val;
> +
> +       /* Clear interrupts */
> +       val = readl(priv->base + ASPEED_PECI_INT_STS) | ASPEED_PECI_INT_MASK;

Should that be & MASK?

As you're just sanitising the registers, you could clear the status
unconditionally:

 writel(ASPEED_PECI_INT_MASK, priv->base + ASPEED_PECI_INT_STS);

> +       writel(val, priv->base + ASPEED_PECI_INT_STS);
> +
> +       /* Set timing negotiation mode and enable interrupts */
> +       val = FIELD_PREP(ASPEED_PECI_TIMING_NEGO_SEL_MASK, ASPEED_PECI_1ST_BIT_OF_ADDR_NEGO);

That's a complicated way to set val to zero :)

> +       val |= ASPEED_PECI_INT_MASK;
> +       writel(val, priv->base + ASPEED_PECI_INT_CTRL);
> +
> +       val = FIELD_PREP(ASPEED_PECI_CTRL_SAMPLING_MASK, ASPEED_PECI_RD_SAMPLING_POINT_DEFAULT);
> +       writel(val, priv->base + ASPEED_PECI_CTRL);

This will clear the rest of the ctrl register, including the divisor
settings. Was that your intention?

Reading the rest of your driver you only call _init_regs after
_controller_enable, so I guess you're fine.

> +}
> +
> +static int aspeed_peci_check_idle(struct aspeed_peci *priv)
> +{
> +       u32 cmd_sts = readl(priv->base + ASPEED_PECI_CMD);
> +       int ret;
> +
> +       /*
> +        * Under normal circumstances, we expect to be idle here.
> +        * In case there were any errors/timeouts that led to the situation
> +        * where the hardware is not in idle state - we need to reset and
> +        * reinitialize it to avoid potential controller hang.
> +        */
> +       if (FIELD_GET(ASPEED_PECI_CMD_STS_MASK, cmd_sts)) {
> +               reset_control_assert(priv->rst);
> +
> +               ret = reset_control_deassert(priv->rst);
> +               if (ret) {
> +                       dev_err(priv->dev, "cannot deassert reset control\n");
> +                       return ret;
> +               }
> +
> +               aspeed_peci_init_regs(priv);
> +
> +               ret = clk_set_rate(priv->clk, priv->clk_frequency);
> +               if (ret < 0) {
> +                       dev_err(priv->dev, "cannot set clock frequency\n");
> +                       return ret;
> +               }
> +
> +               aspeed_peci_controller_enable(priv);
> +       }
> +
> +       return readl_poll_timeout(priv->base + ASPEED_PECI_CMD,
> +                                 cmd_sts,
> +                                 !(cmd_sts & ASPEED_PECI_CMD_IDLE_MASK),
> +                                 ASPEED_PECI_IDLE_CHECK_INTERVAL_US,
> +                                 ASPEED_PECI_IDLE_CHECK_TIMEOUT_US);
> +}
> +
> +static int aspeed_peci_xfer(struct peci_controller *controller,
> +                           u8 addr, struct peci_request *req)
> +{
> +       struct aspeed_peci *priv = dev_get_drvdata(controller->dev.parent);
> +       unsigned long timeout = msecs_to_jiffies(priv->cmd_timeout_ms);
> +       u32 peci_head;
> +       int ret;
> +
> +       if (req->tx.len > ASPEED_PECI_DATA_BUF_SIZE_MAX ||
> +           req->rx.len > ASPEED_PECI_DATA_BUF_SIZE_MAX)
> +               return -EINVAL;
> +
> +       /* Check command sts and bus idle state */
> +       ret = aspeed_peci_check_idle(priv);
> +       if (ret)
> +               return ret; /* -ETIMEDOUT */
> +
> +       spin_lock_irq(&priv->lock);
> +       reinit_completion(&priv->xfer_complete);
> +
> +       peci_head = FIELD_PREP(ASPEED_PECI_TARGET_ADDR_MASK, addr) |
> +                   FIELD_PREP(ASPEED_PECI_WR_LEN_MASK, req->tx.len) |
> +                   FIELD_PREP(ASPEED_PECI_RD_LEN_MASK, req->rx.len);
> +
> +       writel(peci_head, priv->base + ASPEED_PECI_RW_LENGTH);
> +
> +       memcpy_toio(priv->base + ASPEED_PECI_WR_DATA0, req->tx.buf, min_t(u8, req->tx.len, 16));
> +       if (req->tx.len > 16)
> +               memcpy_toio(priv->base + ASPEED_PECI_WR_DATA4, req->tx.buf + 16,
> +                           req->tx.len - 16);
> +
> +#if IS_ENABLED(CONFIG_DYNAMIC_DEBUG)
> +       dev_dbg(priv->dev, "HEAD : %#08x\n", peci_head);
> +       print_hex_dump_bytes("TX : ", DUMP_PREFIX_NONE, req->tx.buf, req->tx.len);
> +#endif

The ifdef is unfortunate. Could you do this?

dev_dbg(priv->dev, "HEAD : %#08x\n", peci_head);
if (IS_ENABLED(CONFIG_DYNAMIC_DEBUG))
       print_hex_dump_bytes("TX : ", DUMP_PREFIX_NONE, req->tx.buf,
req->tx.len);

Not a biggie though, don't let this hold up merging.

> +       priv->status = 0;
> +       writel(ASPEED_PECI_CMD_FIRE, priv->base + ASPEED_PECI_CMD);
> +       spin_unlock_irq(&priv->lock);
> +