Hi,
On Wed, Aug 17, 2022 at 06:24:50PM +0800, Szuying Chen wrote:
> From: Szuying Chen <Chloe_Chen@xxxxxxxxxxxxxx>
>
> Signed-off-by: Szuying Chen <Chloe_Chen@xxxxxxxxxxxxxx>
> ---
> Hi,
>
> >> From: Szuying Chen <Chloe_Chen@xxxxxxxxxxxxxx>
> >>
>
> >> +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.
