Re: [PATCH 18/24] thunderbolt: Store Thunderbolt generation in the switch structure

[Lukas Wunner <lukas@wunner.de>]

On Thu, May 18, 2017 at 05:39:08PM +0300, Mika Westerberg wrote:
> In some cases it is useful to know what is the Thunderbolt generation
> the switch supports. This introduces a new field to struct switch that
> stores the generation of the switch based on the device ID.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reviewed-by: Yehezkel Bernat <yehezkel.bernat@intel.com>
> Reviewed-by: Michael Jamet <michael.jamet@intel.com>
> ---
>  drivers/thunderbolt/switch.c | 24 ++++++++++++++++++++++++
>  drivers/thunderbolt/tb.h     |  2 ++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
> index 396e00ab7723..9c91d397d3b3 100644
> --- a/drivers/thunderbolt/switch.c
> +++ b/drivers/thunderbolt/switch.c
> @@ -382,6 +382,28 @@ struct device_type tb_switch_type = {
>  	.release = tb_switch_release,
>  };
>  
> +static void tb_switch_set_generation(struct tb_switch *sw)
> +{
> +	switch (sw->config.device_id) {
> +	case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_BRIDGE:
> +	case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_2C_BRIDGE:
> +	case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_4C_BRIDGE:
> +	case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_2C_BRIDGE:
> +	case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_BRIDGE:
> +		sw->generation = 3;
> +		break;
> +
> +	case PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_BRIDGE:
> +	case PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_BRIDGE:
> +		sw->generation = 2;
> +		break;

This is missing PCI_DEVICE_ID_INTEL_WIN_RIDGE_2C_BRIDGE (0x157e).


> +
> +	default:
> +		sw->generation = 1;
> +		break;

If someone adds an entry for, say, a new TB3 controller to nhi_ids[]
but forgets to update this function, the controller is assigned the
wrong generation number.  It might be better to make TB3 the default
and list each TB1 controller instead since it's less likely for Intel
to introduce an older gen chip.

Generally I think it's problematic to require that multiple files
are touched whenever a new controller is added.  Isn't the generation
number or link speed (10/20/40) stored in some register in PCI config
space (VSEC 0x1234) or TB config space?

Thanks,

Lukas

> +	}
> +}
> +
>  /**
>   * tb_switch_alloc() - allocate a switch
>   * @tb: Pointer to the owning domain
> @@ -442,6 +464,8 @@ struct tb_switch *tb_switch_alloc(struct tb *tb, struct device *parent,
>  	}
>  	sw->cap_plug_events = cap;
>  
> +	tb_switch_set_generation(sw);
> +
>  	device_initialize(&sw->dev);
>  	sw->dev.parent = parent;
>  	sw->dev.bus = &tb_bus_type;
> diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> index 0be989069941..b3cda7605619 100644
> --- a/drivers/thunderbolt/tb.h
> +++ b/drivers/thunderbolt/tb.h
> @@ -25,6 +25,7 @@
>   * @device: Device ID of the switch
>   * @vendor_name: Name of the vendor (or %NULL if not known)
>   * @device_name: Name of the device (or %NULL if not known)
> + * @generation: Switch Thunderbolt generation
>   * @cap_plug_events: Offset to the plug events capability (%0 if not found)
>   * @is_unplugged: The switch is going away
>   * @drom: DROM of the switch (%NULL if not found)
> @@ -40,6 +41,7 @@ struct tb_switch {
>  	u16 device;
>  	const char *vendor_name;
>  	const char *device_name;
> +	unsigned int generation;
>  	int cap_plug_events;
>  	bool is_unplugged;
>  	u8 *drom;
> -- 
> 2.11.0
>