[PATCH linux dev-5.2] fsi: Add ast2600 master driver

[Jeremy Kerr]

Hi Joel,

> The ast2600 BMC has a pair of FSI masters in it, behind an AHB to OPB
> bridge.
> 
> Signed-off-by: Joel Stanley <joel at jms.id.au>
> ---
> This driver is enough to expose FSI functionality to the FSI core that
> is equivalent to the GPIO master. It has some rough edges that need to
> be worked on before it is sent upstream, and lacks support for ipoll,
> interrupts and DMA operations.

Neat!

I'm happy for this to go into the development tree pretty-much as-is,
but we may want to do a few cleanups before going upstream. I've put
few comments inline, but most of those can wait for that upstreaming.

Except maybe the locking (mentioned below), we should sort that out
sooner rather than later.

> +config FSI_MASTER_ASPEED
> +	tristate "FSI ASPEED master"
> +	help
> +	 This option enables a FSI master that is present behind an OPB
> bridge
> +	 in the AST2600.
> +

Are we OK with using "ASPEED" as the identifier here? Given it's a
vendor name, rather than a specific part or design...

</bikeshed>

> +/* TODO: these are copied from the hub master. Put them in a common
> header */

Yeah, I can see a bunch of potential overlap between this and the hub
master, particularly on finicky things like error recovery. However, it
might be best to do a refactor once we have the inner workings of the
OPB master sorted out, and probably after we've got a better idea oq
the DMA implementation.

> It has only been tested with OPB0 on the first master in the ast2600.

I don't expect there to be much difference for OPB1, right?

> +/* half world */
> +#define  STATUS_HW_ACK	BIT(0)
> +/* full word */
> +#define  STATUS_FW_ACK	BIT(1)

Can we match these to the XFER_SIZE definitions (both in value and
name)?

> +static u32 opb_write(void __iomem *base, uint32_t addr, uint32_t val,
> +		     size_t size)
> +{
> +	u32 reg, ret, status;
> +
> +	/* TODO: implement other sizes, see 0x18 */
> +	WARN_ON(size != 4);
> +
> +	writel(0x1, base + OPB0_SELECT);
> +	writel(CMD_WRITE, base + OPB0_RW);
> +	writel(XFER_WORD, base + OPB0_XFER_SIZE);
> +	writel(addr, base + OPB0_FSI_ADDR);
> +	writel(val, base + OPB0_FSI_DATA_W);
> +	writel(0x1, base + OPB_IRQ_CLEAR);
> +	writel(0x1, base + OPB_TRIGGER);

We'll likely need locking here to serialise access to the OPB state,
especially if/when we add OPB1 support. The GPIO master does it at the
master ops level, which should work here too.

> +
> +	ret = readl_poll_timeout(base + OPB_IRQ_STATUS, reg,
> +				(reg & OPB0_XFER_ACK_EN) != 0,
> +				1, 10000);

I know the 10000 was kind of arbitrary, but we might want to match it in
opb_read.

Any thoughts about making this interrupt-driven?

> +
> +	status = readl(base + OPB0_STATUS);
> +
> +	trace_fsi_master_aspeed_opb_write(base, addr, val, size,
> +			status, reg);
> +
> +	/* Return error when poll timed out */
> +	if (ret)
> +		return ret;
> +
> +	/* Command failed, master will reset */

s/master/caller/ maybe? What do you mean here? It might be worth
mentioning that this return value matches the conditional in
check_errors()


> +static int aspeed_master_read(struct fsi_master *master, int link,
> +			uint8_t id, uint32_t addr, void *val, size_t size)
> +{
> +	struct fsi_master_aspeed *aspeed = to_fsi_master_aspeed(master);
> +	int ret;
> +	u32 data;
> +
> +	if (id != 0)
> +		return -EINVAL;

This will preclude 23-bit addressing mode (see fsi_slave_calc_addr()),
is that OK?

> +	addr += link * FSI_HUB_LINK_SIZE;

Will we ever need to support multiple links?

> +static int aspeed_master_term(struct fsi_master *master, int link, uint8_t id)
> +{
> +	struct fsi_master_aspeed *aspeed = to_fsi_master_aspeed(master);
> +	uint32_t addr;
> +	__be32 cmd;
> +	int rc;
> +
> +	addr = 0x4;
> +	cmd = cpu_to_be32(0xecc00000);
> +
> +	dev_dbg(aspeed->dev, "sending term to link %d slave %d\n", link, id);
> +
> +	rc = aspeed_master_write(master, link, id, addr, &cmd, 4);
> +
> +	dev_dbg(aspeed->dev, "term done (%d)\n", rc);
> +
> +	return rc;
> +}
> +
> +static int aspeed_master_break(struct fsi_master *master, int link)
> +{
> +	struct fsi_master_aspeed *aspeed = to_fsi_master_aspeed(master);
> +	uint32_t addr;
> +	__be32 cmd;
> +	int rc;
> +
> +	addr = 0x0;
> +	cmd = cpu_to_be32(0xc0de0000);
> +
> +	dev_dbg(aspeed->dev, "sending break to link %d\n", link);
> +
> +	rc = aspeed_master_write(master, link, 0, addr, &cmd, 4);
> +
> +	dev_dbg(aspeed->dev, "break done (%d)\n", rc);
> +
> +	return rc;
> +}

So I did some digging to figure out by we needed address 0x4 rather than
0x0 there, and it turns out that's only required for hub masters
(apparently, using address 0 caused the *upstream* link of the hub to
consider it a break).

So, I think we can use both 0x0 here, as we're guaranteed to be the
top-level master - or until we consolidate this code with the hub
master. But maybe we should make it consistent across TERM and BREAK?
Does TERM work at both addresses?

> +	/* TODO(joel): The hub had this cleanup. Should we be claiming a range?
> +	 * fsi_slave_release_range(fsi_dev->slave, FSI_HUB_LINK_OFFSET,
> +	 *		FSI_HUB_LINK_SIZE * links);
> +	 */

No, we don't need to claim a range, as we're not using any of an
upstream CFAM's address space.

> +TRACE_EVENT(fsi_master_aspeed_opb_read,
> +	TP_PROTO(void __iomem *base, uint32_t addr, size_t size, uint32_t result, uint32_t status, uint32_t irq_status),
> +	TP_ARGS(base, addr, size, result, status, irq_status),
> +	TP_STRUCT__entry(
> +		__field(void *,    base)
> +		__field(uint32_t,  addr)
> +		__field(size_t,    size)
> +		__field(uint32_t,  result)
> +		__field(uint32_t,  status)
> +		__field(uint32_t,  irq_status)
> +		),

Did we end up using the status register at all, or just the irq_status?

Cheers,


Jeremy