diff mbox series

[2/2] thunderbolt: thunderbolt: add nvm specific operations for

Message ID 20220823090423.5249-1-chensiying21@gmail.com (mailing list archive)
State New, archived
Headers show
Series None | expand

Commit Message

Szuying Chen Aug. 23, 2022, 9:04 a.m. UTC 
From: Szuying Chen <Chloe_Chen@asmedia.com.tw>

The patch depends on patch 1/2. Add nvm specific operations for ASMedia.
And add tb_switch_nvm_upgradable() of enable firmware upgrade.

Signed-off-by: Szuying Chen <Chloe_Chen@asmedia.com.tw>
---
 drivers/thunderbolt/nvm.c    | 46 ++++++++++++++++++++++++++++++++++++
 drivers/thunderbolt/switch.c |  3 +++
 drivers/thunderbolt/tb.h     |  2 ++
 3 files changed, 51 insertions(+)

--
2.34.1

Comments

Greg KH Aug. 23, 2022, 9:08 a.m. UTC | #1 
On Tue, Aug 23, 2022 at 05:04:23PM +0800, Szuying Chen wrote:
> From: Szuying Chen <Chloe_Chen@asmedia.com.tw>
> 
> The patch depends on patch 1/2.

That's always implicit based on the naming scheme here, you never have
to say this in the commit message (hint, look at all of the existing
patches on the mailing list for how they are structured, there are
thousands of examples.)

> Add nvm specific operations for ASMedia.
> And add tb_switch_nvm_upgradable() of enable firmware upgrade.
> 
> Signed-off-by: Szuying Chen <Chloe_Chen@asmedia.com.tw>
> ---

As the documentation asked you to, you did not version this patch set,
nor say what is different from your previous ones :(

thanks,

greg k-h
Mika Westerberg Aug. 23, 2022, 9:46 a.m. UTC | #2 
Hi,

On Tue, Aug 23, 2022 at 05:04:23PM +0800, Szuying Chen wrote:
> From: Szuying Chen <Chloe_Chen@asmedia.com.tw>
> 
> The patch depends on patch 1/2. Add nvm specific operations for ASMedia.
> And add tb_switch_nvm_upgradable() of enable firmware upgrade.

Again the $subject and please call it NVM (capital letters) everywhere.

> 
> Signed-off-by: Szuying Chen <Chloe_Chen@asmedia.com.tw>
> ---
>  drivers/thunderbolt/nvm.c    | 46 ++++++++++++++++++++++++++++++++++++
>  drivers/thunderbolt/switch.c |  3 +++
>  drivers/thunderbolt/tb.h     |  2 ++
>  3 files changed, 51 insertions(+)
> 
> diff --git a/drivers/thunderbolt/nvm.c b/drivers/thunderbolt/nvm.c
> index 5c7c2a284497..d2ef22f8b19b 100644
> --- a/drivers/thunderbolt/nvm.c
> +++ b/drivers/thunderbolt/nvm.c
> @@ -15,6 +15,9 @@
>  /* Switch NVM support */
>  #define NVM_CSS		0x10
> 
> +/* ASMedia specific NVM offsets */
> +#define ASMEDIA_NVM_VERSION	0x1c
> +
>  static DEFINE_IDA(nvm_ida);
> 
>  static inline int nvm_read(struct tb_switch *sw, unsigned int address,
> @@ -116,11 +119,39 @@ static int intel_nvm_validate(struct tb_switch *sw)
>  	return image_size;
>  }
> 
> +static int asmedia_nvm_version(struct tb_switch *sw)
> +{
> +	struct tb_nvm *nvm = sw->nvm;
> +	u64 val;
> +	int ret;
> +
> +	ret = nvm_read(sw, ASMEDIA_NVM_VERSION, &val, sizeof(val));
> +	if (ret)
> +		return ret;
> +
> +	/* ASMedia NVM version show format include Firmware version and date xxxxxx.xxxxxx */
> +	nvm->major = (((u8)val >> 0x18) << 0x10 | ((u8)(val >> 0x20)) << 0x8 | (u8)(val >> 0x28));
> +	nvm->minor = (((u8)val) << 0x10 | ((u8)(val >> 0x8)) << 0x8 | (u8)(val >> 0x10));

Can you use some helperes to make this look nicer?

> +	nvm->nvm_size = SZ_512K;

Some explanation perhaps why 512K? Is it always the case even for the
future NVMs? Can it be read somewhere?

> +
> +	return 0;
> +}
> +
> +static void tb_switch_set_nvm_upgrade(struct tb_switch *sw)
> +{
> +	sw->no_nvm_upgrade = false;
> +}
> +
>  struct tb_nvm_vendor_ops intel_switch_nvm_ops = {
>  	.version = intel_nvm_version,
>  	.validate = intel_nvm_validate,
>  };
> 
> +struct tb_nvm_vendor_ops asmedia_switch_nvm_ops = {

const

> +	.nvm_upgrade = tb_switch_set_nvm_upgrade,
> +	.version = asmedia_nvm_version,
> +};
> +
>  struct switch_nvm_vendor {
>  	u16 vendor;
>  	const struct tb_nvm_vendor_ops *vops;
> @@ -129,8 +160,23 @@ struct switch_nvm_vendor {
>  static const struct switch_nvm_vendor switch_nvm_vendors[] = {
>  	{ 0x8086, &intel_switch_nvm_ops },
>  	{ 0x8087, &intel_switch_nvm_ops },
> +	{ 0x174c, &asmedia_switch_nvm_ops },

Ordering, I commented this already last time :(

>  };
> 

If you export it outside of this file, please write kernel-doc.

> +void tb_switch_nvm_upgradable(struct tb_switch *sw)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(switch_nvm_vendors); i++) {
> +		const struct switch_nvm_vendor *v = &switch_nvm_vendors[i];
> +
> +		if (v->vendor == sw->config.vendor_id) {
> +			if (v->vops->nvm_upgrade)
> +				v->vops->nvm_upgrade(sw);
> +		}
> +	}
> +}
> +
>  /**
>   * tb_switch_nvm_alloc() - alloc nvm and set nvm->vops to point
>   * the vendor specific operations.
> diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
> index d257219cb66e..23df70290ca2 100644
> --- a/drivers/thunderbolt/switch.c
> +++ b/drivers/thunderbolt/switch.c
> @@ -2829,6 +2829,9 @@ int tb_switch_add(struct tb_switch *sw)
>  			return ret;
>  	}
> 
> +	/* Enable the NVM firmware upgrade for specific vendors */
> +	tb_switch_nvm_upgradable(sw);
> +
>  	ret = device_add(&sw->dev);
>  	if (ret) {
>  		dev_err(&sw->dev, "failed to add device: %d\n", ret);
> diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> index 73ae2e093a92..8e2887e9c669 100644
> --- a/drivers/thunderbolt/tb.h
> +++ b/drivers/thunderbolt/tb.h
> @@ -75,6 +75,7 @@ enum tb_nvm_write_ops {
>  struct tb_nvm_vendor_ops {
>  	int (*version)(struct tb_switch *sw);
>  	int (*validate)(struct tb_switch *sw);
> +	void (*nvm_upgrade)(struct tb_switch *sw);

Kernel-doc for this too, well and the rest.

>  };
> 
>  #define TB_SWITCH_KEY_SIZE		32
> @@ -748,6 +749,7 @@ static inline void tb_domain_put(struct tb *tb)
>  	put_device(&tb->dev);
>  }
> 
> +void tb_switch_nvm_upgradable(struct tb_switch *sw);

Same comment about the location. Also is it "upgradeable"? 

>  struct tb_nvm *tb_switch_nvm_alloc(struct tb_switch *sw);
>  struct tb_nvm *tb_nvm_alloc(struct device *dev);
>  int tb_nvm_add_active(struct tb_nvm *nvm, size_t size, nvmem_reg_read_t reg_read);
> --
> 2.34.1
diff mbox series

Patch

diff --git a/drivers/thunderbolt/nvm.c b/drivers/thunderbolt/nvm.c
index 5c7c2a284497..d2ef22f8b19b 100644
--- a/drivers/thunderbolt/nvm.c
+++ b/drivers/thunderbolt/nvm.c
@@ -15,6 +15,9 @@ 
 /* Switch NVM support */
 #define NVM_CSS		0x10

+/* ASMedia specific NVM offsets */
+#define ASMEDIA_NVM_VERSION	0x1c
+
 static DEFINE_IDA(nvm_ida);

 static inline int nvm_read(struct tb_switch *sw, unsigned int address,
@@ -116,11 +119,39 @@  static int intel_nvm_validate(struct tb_switch *sw)
 	return image_size;
 }

+static int asmedia_nvm_version(struct tb_switch *sw)
+{
+	struct tb_nvm *nvm = sw->nvm;
+	u64 val;
+	int ret;
+
+	ret = nvm_read(sw, ASMEDIA_NVM_VERSION, &val, sizeof(val));
+	if (ret)
+		return ret;
+
+	/* ASMedia NVM version show format include Firmware version and date xxxxxx.xxxxxx */
+	nvm->major = (((u8)val >> 0x18) << 0x10 | ((u8)(val >> 0x20)) << 0x8 | (u8)(val >> 0x28));
+	nvm->minor = (((u8)val) << 0x10 | ((u8)(val >> 0x8)) << 0x8 | (u8)(val >> 0x10));
+	nvm->nvm_size = SZ_512K;
+
+	return 0;
+}
+
+static void tb_switch_set_nvm_upgrade(struct tb_switch *sw)
+{
+	sw->no_nvm_upgrade = false;
+}
+
 struct tb_nvm_vendor_ops intel_switch_nvm_ops = {
 	.version = intel_nvm_version,
 	.validate = intel_nvm_validate,
 };

+struct tb_nvm_vendor_ops asmedia_switch_nvm_ops = {
+	.nvm_upgrade = tb_switch_set_nvm_upgrade,
+	.version = asmedia_nvm_version,
+};
+
 struct switch_nvm_vendor {
 	u16 vendor;
 	const struct tb_nvm_vendor_ops *vops;
@@ -129,8 +160,23 @@  struct switch_nvm_vendor {
 static const struct switch_nvm_vendor switch_nvm_vendors[] = {
 	{ 0x8086, &intel_switch_nvm_ops },
 	{ 0x8087, &intel_switch_nvm_ops },
+	{ 0x174c, &asmedia_switch_nvm_ops },
 };

+void tb_switch_nvm_upgradable(struct tb_switch *sw)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(switch_nvm_vendors); i++) {
+		const struct switch_nvm_vendor *v = &switch_nvm_vendors[i];
+
+		if (v->vendor == sw->config.vendor_id) {
+			if (v->vops->nvm_upgrade)
+				v->vops->nvm_upgrade(sw);
+		}
+	}
+}
+
 /**
  * tb_switch_nvm_alloc() - alloc nvm and set nvm->vops to point
  * the vendor specific operations.
diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index d257219cb66e..23df70290ca2 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -2829,6 +2829,9 @@  int tb_switch_add(struct tb_switch *sw)
 			return ret;
 	}

+	/* Enable the NVM firmware upgrade for specific vendors */
+	tb_switch_nvm_upgradable(sw);
+
 	ret = device_add(&sw->dev);
 	if (ret) {
 		dev_err(&sw->dev, "failed to add device: %d\n", ret);
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index 73ae2e093a92..8e2887e9c669 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -75,6 +75,7 @@  enum tb_nvm_write_ops {
 struct tb_nvm_vendor_ops {
 	int (*version)(struct tb_switch *sw);
 	int (*validate)(struct tb_switch *sw);
+	void (*nvm_upgrade)(struct tb_switch *sw);
 };

 #define TB_SWITCH_KEY_SIZE		32
@@ -748,6 +749,7 @@  static inline void tb_domain_put(struct tb *tb)
 	put_device(&tb->dev);
 }

+void tb_switch_nvm_upgradable(struct tb_switch *sw);
 struct tb_nvm *tb_switch_nvm_alloc(struct tb_switch *sw);
 struct tb_nvm *tb_nvm_alloc(struct device *dev);
 int tb_nvm_add_active(struct tb_nvm *nvm, size_t size, nvmem_reg_read_t reg_read);