[DISCUSS] sfp: sfp controller concept

[DISCUSS] sfp: sfp controller concept

[Vadym Kochan]

Hi,

I'd like to discuss a concept of introduction additional entity into SFP
subsystem called SFP controller. But lets start with the issue.

Issue
=====

There are boards with SFP ports whose GPIO pins are not connected directly to
the SoC but to the I2C CPLD device which has ability to read/write statuses of
each SFP via I2C read/write commands.

Of course there is already a solution - implement GPIO chip and convert GPIO
numbers & states into internal representation (I2C registers). But it requires
additional GPIO-related handling like:

1) Describe GPIO pins and IN/OUT mapping in DTS

2) Consider this mapping also in CPLD driver

3) IRQ - for me this is not clear how to simulate
   sending IRQ via GPIO chip.

I started to think that may be it would be good to introduce
some generic sfp_controller which might be registered by such CPLD
driver which may provide states of connected modules through the
callback by mapping direct SFP states into it's CPLD registers and
call some sfp_state_update() which will trigger the SFP state
machine. So this driver will check/provide on direct SFP defined
states without considering the GPIO things.

How it may look
===============

Device tree:

sfp0: sfp0 {
        compatible = "sff,sfp";
        i2c-bus = <&i2c0_sfp0>;
        /* ref to controller device */
        ctl = <&cpld>;
        /* this index will be used by sfp controller */
        idx = <0>;
};

SFP controller interface:

There might be added generic sfp-ctl.c which implements the basic sfp controller infra:

    1) register/unregister sfp controller driver

    2) lookup sfp controller by fwnode on SFP node parsing/probing

The relation between modules might be:

    sfp.c <-> sfp-ctl.c <- driver <-> CPLD or some device

Flows:
1) CPLD driver prope:
    driver -> sfp_controller_register()

2) SFP instance probe:
    sfp.c -> sfp-ctl.c:sfp_controller_add_socket()
             creates assoctation between idx and sfp instance.
                                      
3) SFP get state:
    sfp.c -> sfp_ctl_get_state() -> sfp-ctl.c:sfp_controller_get_state() -> driver ops -> get_state

4) SFP state updated:
    driver -> sfp-ctl.c:sfp_controller_notify() -> sfp.c:sfp_state_update()
              finds struct sfp* instance by idx

------------------------------------------------------------------
/* public */

enum {
       GPIO_MODDEF0,
       GPIO_LOS,
       GPIO_TX_FAULT,
       GPIO_TX_DISABLE,
       GPIO_RATE_SELECT,
       GPIO_MAX,

       /* SFP controller should check/provide on these states */
       SFP_F_PRESENT = BIT(GPIO_MODDEF0),
       SFP_F_LOS = BIT(GPIO_LOS),
       SFP_F_TX_FAULT = BIT(GPIO_TX_FAULT),
       SFP_F_TX_DISABLE = BIT(GPIO_TX_DISABLE),
       SFP_F_RATE_SELECT = BIT(GPIO_RATE_SELECT),
};

struct sfp_controller_ops {
	unsigned int (*get_state)(struct sfp_controller *sfp_ctl, int sfp_idx);

	void (*set_state)(struct sfp_controller *sfp_ctl, int sfp_idx,
			  unsigned int state);
};

/* implemented by sfp-ctl.c */
struct sfp_controller *
sfp_controller_register(struct device *dev,
			struct sfp_controller_ops *sfp_ctl_ops,
			int flags);

/* implemented by sfp-ctl.c */
void sfp_controller_unregister(struct sfp_controller *sfp_ctl);

/* implemented by sfp.c */
sfp_state_update(struct sfp *sfp, int state);

/* internal */

/* implemented by sfp-ctl.c */
struct sfp_controller *sfp_controller_find_fwnode(struct fwnode_handle *fwnode);

/* implemented by sfp-ctl.c */
void sfp_controller_put(struct sfp_controller *ctl);

/* implemented by sfp-ctl.c */
unsigned int sfp_controller_get_state(struct sfp_controller *ctl, int idx);

/* implemented by sfp-ctl.c */
void sfp_controller_set_state(struct sfp_controller *ctl, int idx);

/* implemented by sfp-ctl.c */
/* This might be used as ability to notify changed SFP state to sfp-ctl.c by driver
which then may find struct sfp* instance by idx and call sfp_state_update()
which is handled by sfp.c */
void sfp_controller_notify(struct sfp_controller *ctl, int idx, int state);
--------------------------------------------------------------------

Currently I do not see how to properly define sfp_state_update(...) func
which may be triggered by sfp controller to notify SFP state machine. May be additional
interface is needed which may provide to controller the sfp* instance and it's idx:

int sfp_controller_add_socket(struct sfp_controller *ctl, struct sfp *sfp, int idx);

void sfp_controller_del_socket(struct sfp_controller *ctl, struct sfp *sfp);

so the driver may create mapping between struct sfp* and it's idx and call the:

sfp_state_update(struct sfp *sfp, int state);

Regards,
Vadym Kochan

Re: [DISCUSS] sfp: sfp controller concept

[Russell King - ARM Linux admin]

On Fri, Sep 11, 2020 at 09:19:14PM +0300, Vadym Kochan wrote:
> Hi,
> 
> I'd like to discuss a concept of introduction additional entity into SFP
> subsystem called SFP controller. But lets start with the issue.
> 
> Issue
> =====
> 
> There are boards with SFP ports whose GPIO pins are not connected directly to
> the SoC but to the I2C CPLD device which has ability to read/write statuses of
> each SFP via I2C read/write commands.
> 
> Of course there is already a solution - implement GPIO chip and convert GPIO
> numbers & states into internal representation (I2C registers). But it requires
> additional GPIO-related handling like:
> 
> 1) Describe GPIO pins and IN/OUT mapping in DTS
> 
> 2) Consider this mapping also in CPLD driver
> 
> 3) IRQ - for me this is not clear how to simulate
>    sending IRQ via GPIO chip.
> 
> I started to think that may be it would be good to introduce
> some generic sfp_controller which might be registered by such CPLD
> driver which may provide states of connected modules through the
> callback by mapping direct SFP states into it's CPLD registers and
> call some sfp_state_update() which will trigger the SFP state
> machine. So this driver will check/provide on direct SFP defined
> states without considering the GPIO things.

The driver already has the basis for splitting the control signals -
this is why there is sfp->get_state()/sfp->set_state(). However, until
there is hardware that requires something that isn't a GPIO, there was
no point taking it further.

> How it may look
> ===============
> 
> Device tree:
> 
> sfp0: sfp0 {
>         compatible = "sff,sfp";
>         i2c-bus = <&i2c0_sfp0>;
>         /* ref to controller device */
>         ctl = <&cpld>;
>         /* this index will be used by sfp controller */
>         idx = <0>;
> };
> 
> SFP controller interface:
> 
> There might be added generic sfp-ctl.c which implements the basic sfp controller infra:
> 
>     1) register/unregister sfp controller driver
> 
>     2) lookup sfp controller by fwnode on SFP node parsing/probing
> 
> The relation between modules might be:
> 
>     sfp.c <-> sfp-ctl.c <- driver <-> CPLD or some device
> 
> Flows:
> 1) CPLD driver prope:
>     driver -> sfp_controller_register()
> 
> 2) SFP instance probe:
>     sfp.c -> sfp-ctl.c:sfp_controller_add_socket()
>              creates assoctation between idx and sfp instance.
>                                       
> 3) SFP get state:
>     sfp.c -> sfp_ctl_get_state() -> sfp-ctl.c:sfp_controller_get_state() -> driver ops -> get_state
> 
> 4) SFP state updated:
>     driver -> sfp-ctl.c:sfp_controller_notify() -> sfp.c:sfp_state_update()
>               finds struct sfp* instance by idx

You are missing one of the most important things to consider - the SFP
and SFP controller become different drivers. How do they get associated,
and how does the probing between both work? What happens when one of
those drivers is unbound and the resources it provides are taken away?

> Currently I do not see how to properly define sfp_state_update(...) func
> which may be triggered by sfp controller to notify SFP state machine. May be additional
> interface is needed which may provide to controller the sfp* instance and it's idx:
> 
> int sfp_controller_add_socket(struct sfp_controller *ctl, struct sfp *sfp, int idx);
> 
> void sfp_controller_del_socket(struct sfp_controller *ctl, struct sfp *sfp);

I don't see how an index helps. What does the index define?

There's also the issue that the SFP cage driver needs to know which
hardware signals are implemented, so it knows whether to make use of the
soft signals that are available via the I2C bus on the module. I don't
see that having been addressed in your proposal.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [DISCUSS] sfp: sfp controller concept

[Andrew Lunn]

On Fri, Sep 11, 2020 at 09:19:14PM +0300, Vadym Kochan wrote:
> Hi,
> 
> I'd like to discuss a concept of introduction additional entity into SFP
> subsystem called SFP controller. But lets start with the issue.
> 
> Issue
> =====
> 
> There are boards with SFP ports whose GPIO pins are not connected directly to
> the SoC but to the I2C CPLD device which has ability to read/write statuses of
> each SFP via I2C read/write commands.

> 
> Of course there is already a solution - implement GPIO chip and convert GPIO
> numbers & states into internal representation (I2C registers). But it requires
> additional GPIO-related handling like:
> 
> 1) Describe GPIO pins and IN/OUT mapping in DTS
> 
> 2) Consider this mapping also in CPLD driver
> 
> 3) IRQ - for me this is not clear how to simulate
>    sending IRQ via GPIO chip.

Hi Vadym

I2C GPIO expanders do work O.K. for this use case. See for example
vf610-zii-dev-rev-c.dts. It has a semtech,sx1503q expander. And the
two SFF on that board are connected to it. The sx1503q can generate an
interrupt when one of its inputs changes state. But for the SFP core,
that is optional, it can also poll the GPIOs if interrupts are not
supported.

	Andrew