Upstream work for OpenFSI patches

[Christopher Bostic]

On 3/23/17 7:14 AM, Jeremy Kerr wrote:
> Hi Chris & all,
>
> Over the last week or so, I've (re-)started our upstreaming efforts for
> the fsi patches, by addressing the reviews we've had so far, and
> settling some of the open items we've had that effect the future
> userspace ABI of the FSI code.
>
> I think the patches at a point where we're ready to resubmit upstream,
> so I've pushed a branch here:
>
>    https://github.com/jk-ozlabs/linux/commits/openbmc/fsi
>
> - this is currently based on the OpenBMC 4.10 tree, but will be easily
> applicable to vanilla upstream. I've kept the "DEBUG" patches at the top
> of the tree, but these are not intended to be upstreamed (unless someone
> convinces us otherwise!). This tree may be rebased as development
> continues, and I'll likely start a branch which is based on mainline.
>
> There are some fairly significant changes from our first submission,
> notably:
>
>   - endianness: the _read() and _write() APIs are now all *bus endian*,
>     so will be the same on all platforms (the previous fsi patches
>     exposed as (BMC/FSP) CPU endian, which is variable).
>
>     This includes the sysfs raw file, so the byte ordering matches what's
>     transferred on the bus, regardless of CPU endianness:
>
>        # hexdump -n 4 -C fsi0/slave at 00:00/raw
>        00000000  c0 01 0d 12                        |....|
>        00000004
>
>     This will likely mean that drivers and accesses to 'raw' from
>     userspace may need to convert to bus endian.
>
>   - device tree: I've removed the "ibm," prefix for the fsi core and GPIO
>     master compatibility strings, as they're not describing IBM-specific
>     hardware. I've also fixed some errors with our proposed binding
>     specification.
>
>   - device model: we now create separate struct devices for each FSI
>     master, which fits better with the Linux device model, and allows us
>     to add sysfs attributes that are implemented by the fsi core
>
>   - sysfs: there are now sysfs facilities for break and term, and the raw
>     file supports reads and writes of arbitrary sizes.
>
>   - GPIO master: I've split the xfer() logic out a little, so that the
>     response handling & DPOLL retry mechanism is more obvious
>
>   - GPIO master: simplifications for message construction
>
>   - GPIO master: fixes for some CRC calculations
>
>   - GPIO master: we now issue TERM in response to DPOLL busy-loops
>
>   - Error handling: rather than handle errors on (potentially) an entire
>     cascaded read or write, the error handling is now down on a per-slave
>     basis, where we try to reestablish communication in a more "gradual"
>     manner, rather than sending a break immediately.
>
>     (we may need to add a hook to percolate error recovery up to a
>     slave's master, but I haven't seen any need for that at present)

Hi Jeremy,

Have you run much testing on bad bus accesses to verify recovery from 
errors?
>   - Hub master: this is now implemented as a fsi engine driver, as the
>     fsi_slave_{read,write}() functions are exported (and the port count
>     is available in the hMFSI configuration register)
I noticed you moved the FSI master control register definitions into 
fsi-master-hub.c   Though its not presently used, the cmFSI engine also 
shares a common control register setup for the most part and its 
possible there could be other implementations using the same scheme in 
the future.   Would it be preferable to move these into fsi-master.h or 
some other common master code?
>     This means we need fewer special-cases in the fsi core.
>
>   - Tracepoints: I've added tracepoints for FSI core read & write, and
>     another set for low-level GPIO in/out operations.
>
> I've kept the original authorship attribution on each patch, but do let
> me know if anything seems amiss.
>
> I've tested on one-core (heavily) and two-core (lightly) p9 platforms,
> and all seems to look okay so far.
>
> There are a couple of pending items, but those shouldn't need to hold us
> up:
>
>   - IRQ support needs more work, but there doesn't seem to be any
>     consumers of that so far
As long as we migrate to the final IIC driver solution before switching 
over to your new code then there is no interrupt support required.  
Otherwise we'll need to consider when your changes are phased in.
>   - tests for unbind/rebind
>   - updating MAINTAINERS
>
> So, please take a look at the series and let me know if there is
> anything that still needs doing (particularly fixes that have come in
> since the first series), and we can work out our plan for upstream
> progression.
The community had wanted crc calculations moved from drivers/fsi to the 
kernel/lib path on a previous patch set submission I sent out. Do you 
think this is something that will be required in your changes?

The community also didn't want the fake FSI master driver included.   Is 
this something we should keep?

All else looks good.
Thanks,

Chris
>
> Cheers,
>
>
> Jeremy
>