On Thu, 2022-01-13 at 04:12 +0000, Joel Stanley wrote:
> 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>
Thank you :)
>
> 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);
>
The idea is to not modify any other fields in this register and write 1's only
to INT_MASK.
In theory the other fields are RO, but I already found that touching reserved
fields in other registers may make the HW upset.
I'll check whether clearing unconditionally works here.
> > + 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 :)
Agreed - however, you can think of it as a way to document the programming
sequence :)
>
> > + 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?
Yes, it was my intention - the register is initialized with "random" values so
it requires to be cleared when we first accessed it (if we just RMW it, the HW
gets upset).
>
> Reading the rest of your driver you only call _init_regs after
> _controller_enable, so I guess you're fine.
aspeed_peci_init_regs() is always called after controller reset, any other
register programming is going to happen later on (with RMW).
>
> > +}
> > +
> > +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.
I don't understand why should we treat HEAD differently that the rest of the
buffer. Ultimately (looking at this from the PECI protocol level) this is just a
part of TX packet.
Overall, this is the "hot path" of this particular driver - the ifdef is here to
avoid dumping all packets all the time (which would happen if dynamic debug is
not present).
Thanks
-Iwona
>
> > + priv->status = 0;
> > + writel(ASPEED_PECI_CMD_FIRE, priv->base + ASPEED_PECI_CMD);
> > + spin_unlock_irq(&priv->lock);
> > +
