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

[Christopher Bostic]

On Thu, Oct 6, 2016 at 1:53 AM, Jeremy Kerr <jk at 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