[linux,15/15] drivers/fsi: Initialize CFAMs for read/write access

From: Chris Bostic <cbostic@us.ibm.com>

Send break command to CFAMs to reset their logic.  If break
fails this serves as an indication there is no CFAM present.
Set various slave engine mode register defaults.

Add new fields to struct fsi_master to manage hardware
workarounds for break ids, target addresses and cfam IDs.

Limit each link scan to one CFAM.  Due to limitations in
hardware break commands only one can be recognized per link.

Signed-off-by: Chris Bostic <cbostic@us.ibm.com>
---
 drivers/fsi/fsi-cfam.h        |  1 +
 drivers/fsi/fsi-core.c        | 58 +++++++++++++++++++++++++++++++++++++++-
 drivers/fsi/fsi-master-fake.c |  3 +++
 drivers/fsi/fsi-master-gpio.c |  3 +++
 drivers/fsi/fsi-master.h      |  5 ++++
 drivers/fsi/fsi-slave.h       | 62 +++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 131 insertions(+), 1 deletion(-)
 create mode 100644 drivers/fsi/fsi-slave.h

Hi Chris,

> Send break command to CFAMs to reset their logic.  If break
> fails this serves as an indication there is no CFAM present.
> Set various slave engine mode register defaults.
> 
> Add new fields to struct fsi_master to manage hardware
> workarounds for break ids, target addresses and cfam IDs.
> 
> Limit each link scan to one CFAM.  Due to limitations in
> hardware break commands only one can be recognized per link.

I'm not sure what the restriction is there. We can only send a break
over one link? Or something else?

> +/*
> + * Issue a break command to this link
> + */
> +static int fsi_master_break(struct fsi_master *master, int link)
> +{
> +	int rc;
> +	uint32_t cmd = FSI_CMD_BREAK;
> +
> +	rc = master->write(master, link, master->break_id, master->break_offset,
> +			   &cmd, sizeof(cmd));
> +	if (rc) {
> +		dev_warn(master->dev, "break to %02x:%02x failed\n",
> +			 link, master->break_id);
> +		return -ENODEV;
> +	}
> +	udelay(200);		/* wait for logic reset to take effect */
> +
> +	rc = master->read(master, link, master->break_id,
> +			  FSI_SLAVE_BASE + FSI_SMODE, &cmd, sizeof(cmd));
> +	dev_info(master->dev, "smode after break:%08x rc:%d\n", cmd, rc);
> +	if (rc) {
> +		dev_warn(master->dev, "read smode after break failed\n");
> +		return -ENODEV;
> +	}
> +
> +	return rc;
> +}

This seems very specific to one particular master implementation. What
is a GPIO-based FSI master supposed to do here? Decode the write to
specific master addresses and then perform the GPIO sequences for a
proper break?

Instead, the break should be added as an operation on a struct
fsi_master:

  struct fsi_master {
  	struct device	*dev;
  	int		idx;
  	int		n_links;
  	int		(*read)(struct fsi_master *, int link,
  				uint8_t slave, uint32_t addr,
  				void *val, size_t size);
  	int		(*write)(struct fsi_master *, int link,
  				uint8_t slave, uint32_t addr,
  				const void *val, size_t size);
+ 	int		(*break)(struct fsi_master *, int link);
  };

- then the FSI master implementations can do what's required for
sending a break. Then, the core's defintion of fsi_master_break()
becomes:

static int fsi_master_break(struct fsi_master *master, int link)
{
	int rc = 0;

	if (master->break)
		master->break(master, link)

	return rc;
}

Cheers,


Jeremy

Hi Chris,

> static int fsi_master_break(struct fsi_master *master, int link)
> {
> 	int rc = 0;
> 
> 	if (master->break)
> 		master->break(master, link)

Sorry, that should be:
 		rc = master->break(master, link);

Cheers,


Jeremy

On Thu, Oct 6, 2016 at 1:53 AM, Jeremy Kerr <jk@ozlabs.org> wrote:
> Hi Chris,
>
>> Send break command to CFAMs to reset their logic.  If break
>> fails this serves as an indication there is no CFAM present.
>> Set various slave engine mode register defaults.
>>
>> Add new fields to struct fsi_master to manage hardware
>> workarounds for break ids, target addresses and cfam IDs.
>>
>> Limit each link scan to one CFAM.  Due to limitations in
>> hardware break commands only one can be recognized per link.
>
> I'm not sure what the restriction is there. We can only send a break
> over one link? Or something else?
>
A break can be issued on any available link.

Due to hardware bugs that vary depending on the combination of
master type and cfam type the break command must be issued to
different ID's and addresses accordingly.  This was the reason
for adding the cfam_id, break_id, etc.. to the fsi_master struct.

For example, an FSP master must issue a break to the base
address of the 3rd cfam slot on the link.  Offset 0x00600000
on a link that covers 0x00800000 of address space.
There are different requirements for cascaded masters and their
downstream cfams.   Due to this break command limitation
there cannot be more than one cfam per link.  There are also
similar issues with 'terminate' commands that are issued to
reset a cfam out of its locked bus error state.

Additionally, some cfams such as the P8/P9 have a much
larger address window that takes up nearly all of the 0x00800000
address space for a given link.  As such there can only
ever be one cfam on that particular link.

Once the hardware issues are resolved related to break and
terminate commands then some links would be able to support
multiple cfams.

Since the first fsi code delivery will deal with basic one cfam per
link configuration platforms to start with I don't think this is
something to gate release of a first pass fsi device driver.

>> +/*
>> + * Issue a break command to this link
>> + */
>> +static int fsi_master_break(struct fsi_master *master, int link)
>> +{
>> +     int rc;
>> +     uint32_t cmd = FSI_CMD_BREAK;
>> +
>> +     rc = master->write(master, link, master->break_id, master->break_offset,
>> +                        &cmd, sizeof(cmd));
>> +     if (rc) {
>> +             dev_warn(master->dev, "break to %02x:%02x failed\n",
>> +                      link, master->break_id);
>> +             return -ENODEV;
>> +     }
>> +     udelay(200);            /* wait for logic reset to take effect */
>> +
>> +     rc = master->read(master, link, master->break_id,
>> +                       FSI_SLAVE_BASE + FSI_SMODE, &cmd, sizeof(cmd));
>> +     dev_info(master->dev, "smode after break:%08x rc:%d\n", cmd, rc);
>> +     if (rc) {
>> +             dev_warn(master->dev, "read smode after break failed\n");
>> +             return -ENODEV;
>> +     }
>> +
>> +     return rc;
>> +}
>
> This seems very specific to one particular master implementation. What
> is a GPIO-based FSI master supposed to do here? Decode the write to
> specific master addresses and then perform the GPIO sequences for a
> proper break?
>
> Instead, the break should be added as an operation on a struct
> fsi_master:
>
>   struct fsi_master {
>         struct device   *dev;
>         int             idx;
>         int             n_links;
>         int             (*read)(struct fsi_master *, int link,
>                                 uint8_t slave, uint32_t addr,
>                                 void *val, size_t size);
>         int             (*write)(struct fsi_master *, int link,
>                                 uint8_t slave, uint32_t addr,
>                                 const void *val, size_t size);
> +       int             (*break)(struct fsi_master *, int link);
>   };
>
> - then the FSI master implementations can do what's required for
> sending a break. Then, the core's defintion of fsi_master_break()
> becomes:
>
> static int fsi_master_break(struct fsi_master *master, int link)
> {
>         int rc = 0;
>
>         if (master->break)
>                 master->break(master, link)
>
>         return rc;
> }

OK, I'll make that change.

>
> Cheers,
>
>
> Jeremy

Hi Chris,

> A break can be issued on any available link.

OK.

> Due to hardware bugs that vary depending on the combination of
> master type and cfam type the break command must be issued to
> different ID's and addresses accordingly.  This was the reason for
> adding the cfam_id, break_id, etc.. to the fsi_master struct.

But (according to the docs I have) a break is just a logical 1 for 256
clocks - it contains no addressing ID / information. What do the
cfam_ids and break_ids represent "on the wire"?

Cheers,


Jeremy

On Thu, Oct 6, 2016 at 8:21 PM, Jeremy Kerr <jk@ozlabs.org> wrote:
> Hi Chris,
>
>> A break can be issued on any available link.
>
> OK.
>
>> Due to hardware bugs that vary depending on the combination of
>> master type and cfam type the break command must be issued to
>> different ID's and addresses accordingly.  This was the reason for
>> adding the cfam_id, break_id, etc.. to the fsi_master struct.
>
> But (according to the docs I have) a break is just a logical 1 for 256
> clocks - it contains no addressing ID / information. What do the
> cfam_ids and break_ids represent "on the wire"?

Right, at the protocol level a break is simply 256 1's.   One of the
cascaded masters on P8,P9 though has quirks where you must
adjust the target address and ID when writing to the actual hardware.
I don't know the hardware specifics as to what the bug is but
the logic responsible for the translation of a firmware initiated MMIO
onto a link that results in the 256 1's has a bug that corrupts that
message in some way.  Note that this is even an issue for the gpio
master since the bug I just described is in the P8,P9 cascaded master
hardware.

Sounds like we need  to schedule a meeting to probably
go over all the hardware quirks that need to be dealt with.  Exchanging
emails probably isn't the quickest way.

Long story short we'll still need a master->break( ) operation defined
for each master type.

-Chris

>
> Cheers,
>
>
> Jeremy