[Nouveau] [PATCH 4/9] pci: implement pcie speed change for tesla

Roy Spliet
2015-10-12T21:10:15Z

Hey Karol.

Thanks, great work. A few questions and nit-picks in-line, but I reckon
on the technical side all is fine.
Cheers,

Roy

Op 12-10-15 om 21:27 schreef Karol Herbst:
> Signed-off-by: Karol Herbst <nouveau at karolherbst.de>
> ---
>   drm/nouveau/nvkm/subdev/pci/g84.c   | 113
++++++++++++++++++++++++++++++++++++
>   drm/nouveau/nvkm/subdev/pci/g94.c   |  10 ++++
>   drm/nouveau/nvkm/subdev/pci/gf100.c |   5 ++
>   drm/nouveau/nvkm/subdev/pci/gf106.c |   5 ++
>   drm/nouveau/nvkm/subdev/pci/gk104.c |   2 +
>   drm/nouveau/nvkm/subdev/pci/priv.h  |   9 +++
>   6 files changed, 144 insertions(+)
>
> diff --git a/drm/nouveau/nvkm/subdev/pci/g84.c
b/drm/nouveau/nvkm/subdev/pci/g84.c
> index 3faa6bf..e53208e 100644
> --- a/drm/nouveau/nvkm/subdev/pci/g84.c
> +++ b/drm/nouveau/nvkm/subdev/pci/g84.c
> @@ -25,6 +25,109 @@
>
>   #include <core/pci.h>
>
> +u8
Is there a particular reason for making this u8 over u32 or unsigned
int?
> +g84_get_pcie_version_supported(struct nvkm_pci *pci)
> +{
> +	u32 chipset = pci->subdev.device->chipset, reg_v;
> +
> +	/* these cards report wrong information about what they support */
> +	if (chipset == 0x84 || chipset == 0x86)
> +		return 1;
> +
> +	reg_v = nvkm_pci_rd32(pci, 0x460) & 0x200;
> +	if (reg_v == 0x200)
> +		return 2;
> +	return 1;
> +}
> +
> +void
> +g84_set_pcie_version(struct nvkm_pci *pci, u8 ver)
> +{
> +	struct nvkm_device *device = pci->subdev.device;
> +
> +	if (ver > 1)
> +		ver = 1;
> +	else
> +		ver = 0;
> +
> +	nvkm_mask(device, 0x00154c, 0x1, ver);
The above five lines can also be written as the following one-liner:
nvkm_mask(device, 0x00154c, 0x1, (ver > 1));
or alternatively
nvkm_mask(device, 0x00154c, 0x1, (ver >= 2 ? 0x1 : 0x0));

I'd personally find that easier to read. However, I accept this is also
just a bit of bike-shedding..
> +}
> +
> +u8
> +g84_get_pcie_version(struct nvkm_pci *pci)
> +{
> +	struct nvkm_device *device = pci->subdev.device;
> +	return (nvkm_rd32(device, 0x00154c) & 0x1) + 1;
> +}
> +
> +static void
> +g84_set_pcie_cap_speed(struct nvkm_pci *pci, bool full_speed)
> +{
> +	struct nvkm_device *device = pci->subdev.device;
> +	nvkm_mask(device, 0x00154c, 0x80, full_speed ? 0x80 : 0x0);
> +}
> +
> +enum nvkm_pcie_speed
> +g84_get_real_speed(struct nvkm_pci *pci)
> +{
> +	u32 reg_v = nvkm_pci_rd32(pci, 0x88) & 0x30000;
> +	switch (reg_v) {
> +		case 0x30000:
> +			return NVKM_PCIE_SPEED_8_0;
> +		case 0x20000:
> +			return NVKM_PCIE_SPEED_5_0;
> +		case 0x10000:
> +		default:
> +			return NVKM_PCIE_SPEED_2_5;
> +	}
> +}
> +
> +enum nvkm_pcie_speed
> +g84_get_pcie_max_speed(struct nvkm_pci *pci)
> +{
> +	u32 reg_v = nvkm_pci_rd32(pci, 0x460) & 0x3300;
> +	if (reg_v == 0x2200)
> +		return NVKM_PCIE_SPEED_5_0;
> +	return NVKM_PCIE_SPEED_2_5;
> +}
> +
> +void
> +g84_set_pcie_sta_speed(struct nvkm_pci *pci, enum nvkm_pcie_speed speed)
> +{
> +	u32 mask_value;
> +
> +	if (speed == NVKM_PCIE_SPEED_5_0)
> +		mask_value = 0x20;
> +	else
> +		mask_value = 0x10;
> +
> +	nvkm_pci_mask(pci, 0x460, 0x30, mask_value);
> +	nvkm_pci_mask(pci, 0x460, 0x1, 0x1);
> +}
> +
> +int
> +g84_pcie_speed_init(struct nvkm_pci *pci)
> +{
> +	if (g84_get_real_speed(pci) == NVKM_PCIE_SPEED_2_5)
> +		g84_set_pcie_cap_speed(pci, false);
> +	else
> +		g84_set_pcie_cap_speed(pci, true);
> +	return 0;
> +}
> +
> +int
> +g84_pcie_speed_set(struct nvkm_pci *pci, enum nvkm_pcie_speed req_speed,
u8 req_width)
> +{
> +	if (req_speed == NVKM_PCIE_SPEED_5_0)
> +		g84_set_pcie_cap_speed(pci, true);
> +	else
> +		g84_set_pcie_cap_speed(pci, false);
> +
> +	g84_set_pcie_sta_speed(pci, req_speed);
> +
> +	return 0;
> +}
> +
>   void
>   g84_pci_init(struct nvkm_pci *pci)
>   {
> @@ -55,6 +158,16 @@ g84_pci_func = {
>   	.wr08 = nv40_pci_wr08,
>   	.wr32 = nv40_pci_wr32,
>   	.msi_rearm = nv46_pci_msi_rearm,
> +
> +	.pcie_speed_init = g84_pcie_speed_init,
> +	.pcie_speed_set = g84_pcie_speed_set,
> +
> +	.pcie_max_speed = g84_get_pcie_max_speed,
> +	.pcie_current_speed = g84_get_real_speed,
I'd aim for consistency here, by naming the function
g84_get_current_speed(), or
if you like it short g84_get_cur_speed(). Either way, try to keep names
consistent and descriptive.

As far as I can tell the same goes for gk104 and gf106.
> +
> +	.set_pcie_version = g84_set_pcie_version,
> +	.get_pcie_version = g84_get_pcie_version,
> +	.get_pcie_version_supported = g84_get_pcie_version_supported,
>   };
>
>   int
> diff --git a/drm/nouveau/nvkm/subdev/pci/g94.c
b/drm/nouveau/nvkm/subdev/pci/g94.c
> index cd311ee..1431041 100644
> --- a/drm/nouveau/nvkm/subdev/pci/g94.c
> +++ b/drm/nouveau/nvkm/subdev/pci/g94.c
> @@ -30,6 +30,16 @@ g94_pci_func = {
>   	.wr08 = nv40_pci_wr08,
>   	.wr32 = nv40_pci_wr32,
>   	.msi_rearm = nv40_pci_msi_rearm,
> +
> +	.pcie_speed_init = g84_pcie_speed_init,
> +	.pcie_speed_set = g84_pcie_speed_set,
> +
> +	.pcie_max_speed = g84_get_pcie_max_speed,
> +	.pcie_current_speed = g84_get_real_speed,
> +
> +	.set_pcie_version = g84_set_pcie_version,
> +	.get_pcie_version = g84_get_pcie_version,
> +	.get_pcie_version_supported = g84_get_pcie_version_supported,
>   };
>
>   int
> diff --git a/drm/nouveau/nvkm/subdev/pci/gf100.c
b/drm/nouveau/nvkm/subdev/pci/gf100.c
> index 25e1ae7..bd88dd5 100644
> --- a/drm/nouveau/nvkm/subdev/pci/gf100.c
> +++ b/drm/nouveau/nvkm/subdev/pci/gf100.c
> @@ -36,6 +36,11 @@ gf100_pci_func = {
>   	.wr08 = nv40_pci_wr08,
>   	.wr32 = nv40_pci_wr32,
>   	.msi_rearm = gf100_pci_msi_rearm,
> +
> +	.pcie_max_speed = g84_get_pcie_max_speed,
> +	.pcie_current_speed = g84_get_real_speed,
> +
> +	.get_pcie_version_supported = g84_get_pcie_version_supported,
>   };
>
>   int
> diff --git a/drm/nouveau/nvkm/subdev/pci/gf106.c
b/drm/nouveau/nvkm/subdev/pci/gf106.c
> index a3d451e..e2223b9 100644
> --- a/drm/nouveau/nvkm/subdev/pci/gf106.c
> +++ b/drm/nouveau/nvkm/subdev/pci/gf106.c
> @@ -29,6 +29,11 @@ gf106_pci_func = {
>   	.wr08 = nv40_pci_wr08,
>   	.wr32 = nv40_pci_wr32,
>   	.msi_rearm = nv40_pci_msi_rearm,
> +
> +	.pcie_max_speed = g84_get_pcie_max_speed,
> +	.pcie_current_speed = g84_get_real_speed,
> +
> +	.get_pcie_version_supported = g84_get_pcie_version_supported,
>   };
>
>   int
> diff --git a/drm/nouveau/nvkm/subdev/pci/gk104.c
b/drm/nouveau/nvkm/subdev/pci/gk104.c
> index 95ecc51..7b6bb5b 100644
> --- a/drm/nouveau/nvkm/subdev/pci/gk104.c
> +++ b/drm/nouveau/nvkm/subdev/pci/gk104.c
> @@ -29,6 +29,8 @@ gk104_pci_func = {
>   	.wr08 = nv40_pci_wr08,
>   	.wr32 = nv40_pci_wr32,
>   	.msi_rearm = nv40_pci_msi_rearm,
> +
> +	.pcie_current_speed = g84_get_real_speed,
>   };
>
>   int
> diff --git a/drm/nouveau/nvkm/subdev/pci/priv.h
b/drm/nouveau/nvkm/subdev/pci/priv.h
> index 11c266f..9c62164 100644
> --- a/drm/nouveau/nvkm/subdev/pci/priv.h
> +++ b/drm/nouveau/nvkm/subdev/pci/priv.h
> @@ -33,5 +33,14 @@ void nv46_pci_msi_rearm(struct nvkm_pci *);
>
>   void g84_pci_init(struct nvkm_pci *pci);
>
> +void g84_set_pcie_version(struct nvkm_pci *, u8);
> +u8 g84_get_pcie_version(struct nvkm_pci *);
> +u8 g84_get_pcie_version_supported(struct nvkm_pci *);
> +void g84_set_pcie_sta_speed(struct nvkm_pci *, enum nvkm_pcie_speed);
> +enum nvkm_pcie_speed g84_get_real_speed(struct nvkm_pci *);
> +enum nvkm_pcie_speed g84_get_pcie_max_speed(struct nvkm_pci *);
> +int g84_pcie_speed_init(struct nvkm_pci *);
> +int g84_pcie_speed_set(struct nvkm_pci *, enum nvkm_pcie_speed, u8);
> +
>   int nvkm_pci_pcie_init(struct nvkm_pci *pci);
>   #endif