Re: [RFC net-next 00/15] Add support for Silicon Labs CPC

[Andrew Lunn <andrew@lunn.ch>]

> I think Andrew pulled Greybus in the discussion because there is some
> overlap between Greybus and CPC:
>   - Greybus has bundles and CPorts, CPC only has "endpoints", which
>     would be the equivalent of a bundle with a single cport
>   - discoverability of Greybus bundles/CPC endpoints by the host
>   - multiple bundles/endpoints might coexist in the same
>     module/CPC-enabled device
>   - bundles/endpoints are independent from each other and each has its
>     own dedicated driver
> 
> Greybus goes a step further and specs some protocols like GPIO or UART.
> CPC doesn't spec what goes over endpoints because it's geared towards
> radio applications and as you said, it's very device/stack specific.

Is it device specific? Look at your Bluetooth implementation. I don't
see anything device specific in it. That should work for any of the
vendors of similar chips to yours.

For 802.15.4, Linux defines:

struct ieee802154_ops {
        struct module   *owner;
        int             (*start)(struct ieee802154_hw *hw);
        void            (*stop)(struct ieee802154_hw *hw);
        int             (*xmit_sync)(struct ieee802154_hw *hw,
                                     struct sk_buff *skb);
        int             (*xmit_async)(struct ieee802154_hw *hw,
                                      struct sk_buff *skb);
        int             (*ed)(struct ieee802154_hw *hw, u8 *level);
        int             (*set_channel)(struct ieee802154_hw *hw, u8 page,
                                       u8 channel);
        int             (*set_hw_addr_filt)(struct ieee802154_hw *hw,
                                            struct ieee802154_hw_addr_filt *filt,
                                            unsigned long changed);
        int             (*set_txpower)(struct ieee802154_hw *hw, s32 mbm);
        int             (*set_lbt)(struct ieee802154_hw *hw, bool on);
        int             (*set_cca_mode)(struct ieee802154_hw *hw,
                                        const struct wpan_phy_cca *cca);
        int             (*set_cca_ed_level)(struct ieee802154_hw *hw, s32 mbm);
        int             (*set_csma_params)(struct ieee802154_hw *hw,
                                           u8 min_be, u8 max_be, u8 retries);
        int             (*set_frame_retries)(struct ieee802154_hw *hw,
                                             s8 retries);
        int             (*set_promiscuous_mode)(struct ieee802154_hw *hw,
                                                const bool on);
};

Many of these are optional, but this gives an abstract representation
of a device, which is should be possible to turn into a protocol
talked over a transport bus like SPI or SDIO.

This also comes back to my point of there being at least four vendors
of devices like yours. Linux does not want four or more
implementations of this, each 90% the same, just a different way of
converting this structure of operations into messages over a transport
bus.

You have to define the protocol. Mainline needs that so when the next
vendor comes along, we can point at your protocol and say that is how
it has to be implemented in Mainline. Make your firmware on the SoC
understand it.  You have the advantage that you are here first, you
get to define that protocol, but you do need to clearly define it.

You have listed how your implementation is similar to Greybus. You say
what is not so great is streaming, i.e. the bulk data transfer needed
to implement xmit_sync() and xmit_async() above. Greybus is too much
RPC based. RPCs are actually what you want for most of the operations
listed above, but i agree for data, in order to keep the transport
fully loaded, you want double buffering. However, that appears to be
possible with the current Greybus code.

gb_operation_unidirectional_timeout() says:

 * Note that successful send of a unidirectional operation does not imply that
 * the request as actually reached the remote end of the connection.
 */

So long as you are doing your memory management correctly, i don't see
why you cannot implement double buffering in the transport driver.

I also don't see why you cannot extend the Greybus upper API and add a
true gb_operation_unidirectional_async() call.

You also said that lots of small transfers are inefficient, and you
wanted to combine small high level messages into one big transport
layer message. This is something you frequently see with USB Ethernet
dongles. The Ethernet driver puts a number of small Ethernet packets
into one USB URB. The USB layer itself has no idea this is going on. I
don't see why the same cannot be done here, greybus itself does not
need to be aware of the packet consolidation.

	Andrew