Re: RE: [PATCH v4] thunderbolt: thunderbolt: add vendor's NVM formats

[Mika Westerberg <mika.westerberg@linux.intel.com>]

Hi,

On Wed, Aug 17, 2022 at 06:24:50PM +0800, Szuying Chen wrote:
> From: Szuying Chen <Chloe_Chen@asmedia.com.tw>
> 
> Signed-off-by: Szuying Chen <Chloe_Chen@asmedia.com.tw>
> ---
> Hi,
> 
> >> From: Szuying Chen <Chloe_Chen@asmedia.com.tw>
> >>
> 
> >> +static int asmedia_nvm_validate(struct tb_switch *sw, unsigned int mode)
> >> +{
> >> +	struct tb_nvm *nvm;
> >> +	u32 val;
> >> +	u32 nvm_size;
> >> +	int ret = 0;
> >> +	unsigned int image_size;
> >> +
> >> +	switch (mode) {
> >> +	case NVM_UPGRADE:
> >> +		if (sw->no_nvm_upgrade)
> >> +			sw->no_nvm_upgrade = false;
> >> +
> >> +		break;
> >> +
> >> +	case NVM_ADD:
> >> +		nvm = tb_nvm_alloc(&sw->dev);
> >
> >This function does not only "validate" but it also creates the NVMem
> >devices and whatnot.
> >
> >Do you have some public description of the ASMedia format that I could
> >take a look? Perhaps we can find some simpler way of validating the
> >thing that works accross different vendors.
> >
> 
> ASMedia NVM format include rom file, firmware and security
> configuration information. And active firmware depend on this
> information for processing. We don't need to do any validation during
> firmware upgrade, so we haven't public description of the ASMedia
> format.
> 
> I think I use "validate" is not fit. This function mainly to create
> the NVMem devices and write. I will rename in the next patch.

So instead what you now do, I suggest that we move all the vendor
support out to nvm.c, that includes Intel too. What I mean by this is
that the tb_switch_nvm_add() would then look something like this:

tb_switch_nvm_add(sw)
{
	if (!nvm_readable(sw))
		return 0;

	nvm = tb_switch_nvm_alloc(sw);
	if (IS_ERR(nvm)) {
		if (PTR_ERR(nvm) == -EOPNOTSUPP) {
			dev_info(&sw->dev,
				"NVM format of vendor %#x is not known, disabling NVM upgrade\n",
				sw->config.vendor_id);
			return 0;
		}

		return PTR_ERR(nvm);
	}

	ret = tb_nvm_add_active(nvm, nvm->size, tb_switch_nvm_read);
	if (ret)
		...

	if (!sw->no_nvm_upgrade) {
		ret = tb_nvm_add_non_active(nvm, NVM_MAX_SIZE, tb_switch_nvm_write);
		if (ret)
			...
	}

	sw->nvm = nvm;
	...
}

And the tb_switch_nvm_alloc() resides in nvm.c and that one goes over an
array of supported vendors matching sw->config.vendor_id and if it finds
the match it will set nvm->vops to point the vendor specific operations
and in addition it will will populate rest of the nvm fields like this:

static const struct {
	u16 vendor;
	const struct tb_nvm_vendor_ops *vops;
} switch_nvm_vendors[] = {
	{ 0x8086, &intel_switch_nvm_ops },
	{ 0x8087, &intel_switch_nvm_ops },
	{ 0x174c, &asmedia_switch_nvm_ops },
};

tb_switch_nvm_alloc(sw)
{
	struct tb_nvm_vendor_ops *ops = NULL;
	int i;

	for (i = 0; i < ARRAY_SIZE(switch_nvm_vendors); i++) {
		if (switch_nvm_vendors[i].vendor == sw->config.vendor_id)
			vops = &switch_nvm_vendors[i].vops;
			break;
	}

	if (!vops)
		return ERR_PTR(-EOPNOTSUPP);

	nvm = tb_nvm_alloc(&sw->dev);
	if (IS_ERR(nvm))
		...

	nvm->vops = vops;
	ret = vops->populate(nvm);
	if (ret)
		...

	...
}

Then we would have all the vendor specific things in
intel_switch_nvm_ops and asmedia_switch_nvm_ops accordingly and the rest
of the code is generic USB4 stuff. We need to do the same for retimers
too at some point.