[PATCH linux v3 16/18] drivers/fsi: Set up CFAMs for communication

[Christopher Bostic]

On Tue, Oct 11, 2016 at 7:34 PM, Jeremy Kerr <jk at ozlabs.org> wrote:
> Hi Chris,
>
>> Issue break command to reset CFAM logic and set ID as appropriate
>> for a particular master type.  Begin definition of the slave
>> engine register set and accessors.
>>
>> V3 - Remove definition of BREAK command from fsi-master.h
>>
>>    - Remove definitions of fsi_master_fake for set_smode and
>>      break
>>
>>    - Remove master->set_smode master dependent function and
>>      moved to a generic base master set_smode.
>>
>>    - Add fsi_master_link_enable with master type dependencies
>>
>> Signed-off-by: Chris Bostic <cbostic at us.ibm.com>
>
> Looking better, I still have a few comments though:
>
> The most significant part of this patch is to add the send_break and
> link_enable callbacks, but we're also adding the smode setup. Because of
> that, I'd suggest splitting this into two:
>
>  - one patch to update the fsi_master API to add the new callbacks, and
>    their implementations; and
>
>  - another patch to do the cfam setup
>

OK will do that.

>> +/*
>> + * Issue a break commad on this link
>
>  *command
>

Oops, will fix.

>> + */
>> +static int fsi_master_break(struct fsi_master *master, int link)
>> +{
>> +     int rc = 0;
>> +
>> +     if (master->send_break)
>> +             rc = master->send_break(master, link);
>> +
>> +     return rc;
>> +}
>> +
>> +static void set_smode_defaults(struct fsi_master *master, uint32_t *smode)
>> +{
>> +     *smode = FSI_SMODE_WSC | FSI_SMODE_ECRC
>> +             | fsi_smode_echodly(0xf) | fsi_smode_senddly(0xf)
>> +             | fsi_smode_lbcrr(1) | fsi_smode_sid(0);
>> +}
>
> I assume that the smode value will depend on the master later, hence the
> need for a function for this (otherwise we'd just want a const
> uint32_t). However, can you just return the smode rather than setting a
> pointer and returning void?

OK will fix.

>
>> +static int fsi_master_set_smode(struct fsi_master *master, int link)
>> +{
>> +     uint32_t smode;
>> +     int rc;
>> +
>> +     set_smode_defaults(master, &smode);
>> +     rc = master->write(master, link, 0, FSI_SLAVE_BASE + FSI_SMODE,
>> +                     &smode, sizeof(smode));
>> +
>> +     return rc;
>> +}
>> +
>
> On reading this a little further, I'm a little confused about whether
> the smode register is part of the master or part of a slave. I've
> assumed the latter (as there are no registers on a GPIO master), but
> wouldn't that need to know the slave ID or base address?
>
> [Or is the absence of that a factor of only supporting one slave per
> link at the moment? If that's the case, can you rename this to
> fsi_slave_set_smode, and add the slave ID parameter that we'll need
> later?  That will clarify things a bit]
>

Its the latter, due to having only one slave per link at the moment.
Will rename and add slave ID as a parameter.

>>  static int fsi_slave_init(struct fsi_master *master,
>>               int link, uint8_t slave_id)
>>  {
>> -     struct fsi_slave *slave;
>> +     struct fsi_slave *slave = NULL;
>>       uint32_t chip_id;
>>       int rc;
>>
>> +     /*
>> +      * todo: Due to CFAM hardware issues related to BREAK commands we're
>> +      * limited to only one CFAM per link. Once hardware fixes are available
>> +      * this restriction can be removed.
>> +      */
>> +     if (slave_id > 0)
>> +             return 0;
>> +
>> +     rc = fsi_master_break(master, link);
>> +     if (rc) {
>> +             dev_warn(master->dev, "no slave detected at %02x:%02x\n",
>> +                             link, slave_id);
>> +             return -ENODEV;
>> +     }
>
> Shoudln't we do the break as part of a master init (and then build
> slave IDs)? Seeing as it will reset the slave IDs, this operation can't
> be confined to only a single slave (as the name slave_init would imply.
>
>> +
>> +     rc = fsi_master_set_smode(master, link);
>> +     if (rc) {
>> +             dev_warn(master->dev, "failed to set smode id, rc:%d\n", rc);
>> +             return -ENODEV;
>> +     }
>> +
>

Good point, will do that.

> You'll probably be better returning rc for both of these cases, no need
> to squash it to ENODEV (and if you do, EIO might be more appropriate).
>

The main way to detect if there is a real and functional slave on the other
end of the link, at least in the past, was to see if the break command failed
or not.  If break failed then there was nothing present to respond to the
read of the slave smode register.  rc is just the result of the slave smode
read.  That was the reasoning for returning ENODEV.  Maybe it makes
more sense to place that value in the break function itself.  Let me know.

>>  /* FSI master support */
>>
>> +static int fsi_master_link_enable(struct fsi_master *master, int link)
>> +{
>> +     int rc;
>> +
>> +     if (master->link_enable)
>> +             rc = master->link_enable(master, link);
>> +
>> +     return rc;
>> +}
>> +
>
> OK, looks good for a link_enable function of the masters. However, can
> you add the new callbacks (link_enable & send_break) and their wrappers
> in a separate earlier patch?

Is this the same change you discuss above where you'd like to see
link_enable and send_break code in a separate patch from the set_smode
code? Or is this a third patch to spell out sub details on link_enable and
send_break?

>
>>  static int fsi_master_scan(struct fsi_master *master)
>>  {
>> -     int link, slave_id;
>> +     int link, slave_id, rc;
>> +
>> +     for (link = 0; link < master->n_links; link++) {
>> +             rc = fsi_master_link_enable(master, link);
>> +             if (rc)
>> +                     continue;
>
> Shouldn't we fail on that?
>

Yes, but what that means right now is uncertain.  I can put in a dev_dbg( )
to track the fail but I don't want to exit immediately on that error since there
may still be links to deal with in the for loop.

>>
>> -     for (link = 0; link < master->n_links; link++)
>>               for (slave_id = 0; slave_id < FSI_N_SLAVES; slave_id++)
>>                       fsi_slave_init(master, link, slave_id);
>> +     }
>>
>>       return 0;
>>
>> diff --git a/drivers/fsi/fsi-master-fake.c b/drivers/fsi/fsi-master-fake.c
>> index ec1ed5e..c7ad631 100644
>> --- a/drivers/fsi/fsi-master-fake.c
>> +++ b/drivers/fsi/fsi-master-fake.c
>> @@ -63,6 +63,8 @@ static int fsi_master_fake_probe(struct platform_device *pdev)
>>       master->n_links = 1;
>>       master->read = fsi_master_fake_read;
>>       master->write = fsi_master_fake_write;
>> +     master->send_break = NULL;
>> +     master->link_enable = NULL;
>
> master is kzalloc()ed, no need to set to NULL.
>

Will remove.

>> +/*
>> + * Issue a break command on link
>> + */
>> +static int fsi_master_gpio_break(struct fsi_master *_master, int link)
>> +{
>> +     struct fsi_master_gpio *master = to_fsi_master_gpio(_master);
>> +
>> +     if (link != 0)
>> +             return -ENODEV;
>> +
>> +     /* todo: send the break pattern over gpio */
>> +     (void)master;
>> +
>> +     return 0;
>> +}
>> +
>> +static int fsi_master_gpio_link_enable(struct fsi_master *_master, int link)
>> +{
>> +     struct fsi_master_gpio *master = to_fsi_master_gpio(_master);
>> +
>> +     if (link != 0)
>> +             return -ENODEV;
>> +
>> +     /* todo: set the enable pin */
>> +     (void)master;
>> +
>> +     return 0;
>> +}
>> +
>
> Looks good.
>
>> diff --git a/drivers/fsi/fsi-slave.h b/drivers/fsi/fsi-slave.h
>> new file mode 100644
>> index 0000000..ea7a760
>> --- /dev/null
>> +++ b/drivers/fsi/fsi-slave.h
>> @@ -0,0 +1,62 @@
>> +/*
>> + * FSI slave engine definitions.
>> + *
>
> Why does this need to be in a header file? Isn't it only used by the FSI
> core?
>

I suppose this is me not being familiar with the common coding practices
of the Linux community.  I would typically put constants and structs in an
associated header file even if there was no intent to have them be used
by anything outside the core.   Is it a requirement that this all be moved
into fsi-core.c as a means of limiting accessibility?

Thanks,
Chris

> Cheers,
>
>
> Jeremy