From: Szuying Chen <[email protected]>
Signed-off-by: Szuying Chen <[email protected]>
---
Hi,
>> From: Szuying Chen <[email protected]>
>>
>> +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.
>> + ret = PTR_ERR(nvm);
>> + break;
>> + }
>> +
>> + ret = usb4_switch_nvm_read(sw, ASMEDIA_NVM_VERSION, &val, sizeof(val));
>> + if (ret)
>> + break;
>> +
>> + nvm->nvm_asm.major = (((u8)val) << 0x10 | ((u8)(val >> 0x8)) << 0x8 | (u8)(val >> 0x10));
>> + ret = usb4_switch_nvm_read(sw, ASMEDIA_NVM_DATE, &val, sizeof(val));
>> + if (ret)
>> + break;
>> +
>> + nvm->nvm_asm.minor = (((u8)val) << 0x10 | ((u8)(val >> 0x8)) << 0x8 | (u8)(val >> 0x10));
>> + nvm_size = SZ_512K;
>> + ret = tb_nvm_add_active(nvm, nvm_size, tb_switch_nvm_read);
>> + if (ret)
>> + break;
>> +
>> + ret = tb_nvm_add_non_active(nvm, NVM_MAX_SIZE, tb_switch_nvm_write);
>> + if (ret)
>> + break;
>> +
>> + sw->nvm = nvm;
>> + break;
>> +
>> + case NVM_WRITE:
>> + const u8 *buf = sw->nvm->buf;
>> +
>> + if (!buf) {
>> + ret = -EINVAL;
>> + break;
>> + }
>> + image_size = sw->nvm->buf_data_size;
>> + if (image_size < NVM_MIN_SIZE || image_size > NVM_MAX_SIZE) {
>> + ret = -EINVAL;
>> + break;
>> + }
>> + ret = usb4_switch_nvm_write(sw, 0, buf, image_size);
>> + if (!ret)
>> + sw->nvm->flushed = true;
>> +
>> + break;
>> +
>> + default:
>> + break;
>> + }
>> +
>> + if ((mode == NVM_ADD) && (ret != 0))
>> + tb_nvm_free(sw->nvm);
>> +
>> + return ret;
>> +}
>> @@ -1953,6 +1971,9 @@ static ssize_t nvm_version_show(struct device *dev,
>> ret = -ENODATA;
>> else if (!sw->nvm)
>> ret = -EAGAIN;
>> + /*ASMedia NVM version show format xxxxxx_xxxxxx */
>> + else if (sw->config.vendor_id == 0x174C)
>> + ret = sprintf(buf, "%06x.%06x\n", sw->nvm->nvm_asm.major, sw->nvm->nvm_asm.minor);
>
>And yes, we can make the nvm->major/minor to be 32-bit integers too for
>both Intel and ASMedia and continue to use the %x.%x formatting.
>
Thanks to Mika and Mario for the suggestion, I'll fix it in next patch.
>> else
>> ret = sprintf(buf, "%x.%x\n", sw->nvm->major, sw->nvm->minor);
>>
>> @@ -2860,6 +2881,7 @@ int tb_switch_add(struct tb_switch *sw)
>> tb_sw_dbg(sw, "uid: %#llx\n", sw->uid);
>>
>> tb_check_quirks(sw);
>> + tb_nvm_validate(sw, NVM_UPGRADE);
>>
>> ret = tb_switch_set_uuid(sw);
>> if (ret) {
>> diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
>> index 5db76de40cc1..7f5c8ae731a0 100644
>> --- a/drivers/thunderbolt/tb.h
>> +++ b/drivers/thunderbolt/tb.h
>> @@ -28,6 +28,15 @@
>> #define NVM_VERSION 0x08
>> #define NVM_FLASH_SIZE 0x45
>>
>> +/* ASMedia specific NVM offsets */
>> +#define ASMEDIA_NVM_VERSION 0x28
>> +#define ASMEDIA_NVM_DATE 0x1c
>
>Didn't I already commented about these? Are my emails somehow lost or
>they just get ignored?
>
Sorry, I miss it. I've checked and I will fix it in next patch.
Hi,
On Wed, Aug 17, 2022 at 06:24:50PM +0800, Szuying Chen wrote:
> From: Szuying Chen <[email protected]>
>
> Signed-off-by: Szuying Chen <[email protected]>
> ---
> Hi,
>
> >> From: Szuying Chen <[email protected]>
> >>
>
> >> +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.