[PATCH v2 0/2] misc: pch_phub: Two cleanups

[PATCH v2 0/2] misc: pch_phub: Two cleanups

[Uwe Kleine-König (The Capable Hub)]

Hello,

v1 of this series is available at
https://lore.kernel.org/cover.1779360001.git.u.kleine-koenig@baylibre.com.

Sashiko[1] found a few issues that are addressed here:

 - Drop various members from pch_phub_reg (patch #1)
 - Also handle ioh_type comparisons against magic constants
 - Using an enum shifts the values used to identify the different
   hardware variants. For the corner case where new pci ids are added at
   runtime to the driver 0 is special. I think this doesn't matter in
   practise but I noted that in the commit log.

@Greg: I already received mail that you picked up v1 of this series. I
hope it's not too late to drop it still?! Please tell me if you don't
want to drop it any more, then I send incremental patches for the issues
addressed here.

Best regards
Uwe

[1] https://sashiko.dev/#/patchset/cover.1779360001.git.u.kleine-koenig%40baylibre.com

Uwe Kleine-König (The Capable Hub) (2):
  misc: pch_phub: Drop two unused functions
  misc: pch_phub: Introduce an enum for device indentification

 drivers/misc/pch_phub.c | 181 +++++-----------------------------------
 1 file changed, 19 insertions(+), 162 deletions(-)


base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
-- 
2.47.3

[PATCH v2 1/2] misc: pch_phub: Drop two unused functions

[Uwe Kleine-König (The Capable Hub)]

The two functions are unused since commit 34afa1d657d4
("misc/pch_phub.c: use generic power management") but the compiler
didn't warn about it because the same commit marked the functions as
__maybe_unsed.

Dropping these functions also makes several members in the driver data
unused that are also dropped.

Signed-off-by: Uwe Kleine-König (The Capable Hub) <u.kleine-koenig@baylibre.com>
---
 drivers/misc/pch_phub.c | 151 ----------------------------------------
 1 file changed, 151 deletions(-)

diff --git a/drivers/misc/pch_phub.c b/drivers/misc/pch_phub.c
index fd147fd2800f..2f08957a0cd6 100644
--- a/drivers/misc/pch_phub.c
+++ b/drivers/misc/pch_phub.c
@@ -85,20 +85,6 @@
 
 /**
  * struct pch_phub_reg - PHUB register structure
- * @phub_id_reg:			PHUB_ID register val
- * @q_pri_val_reg:			QUEUE_PRI_VAL register val
- * @rc_q_maxsize_reg:			RC_QUEUE_MAXSIZE register val
- * @bri_q_maxsize_reg:			BRI_QUEUE_MAXSIZE register val
- * @comp_resp_timeout_reg:		COMP_RESP_TIMEOUT register val
- * @bus_slave_control_reg:		BUS_SLAVE_CONTROL_REG register val
- * @deadlock_avoid_type_reg:		DEADLOCK_AVOID_TYPE register val
- * @intpin_reg_wpermit_reg0:		INTPIN_REG_WPERMIT register 0 val
- * @intpin_reg_wpermit_reg1:		INTPIN_REG_WPERMIT register 1 val
- * @intpin_reg_wpermit_reg2:		INTPIN_REG_WPERMIT register 2 val
- * @intpin_reg_wpermit_reg3:		INTPIN_REG_WPERMIT register 3 val
- * @int_reduce_control_reg:		INT_REDUCE_CONTROL registers val
- * @clkcfg_reg:				CLK CFG register val
- * @funcsel_reg:			Function select register value
  * @pch_phub_base_address:		Register base address
  * @pch_phub_extrom_base_address:	external rom base address
  * @pch_mac_start_address:		MAC address area start address
@@ -107,20 +93,6 @@
  * @pdev:				pointer to pci device struct
  */
 struct pch_phub_reg {
-	u32 phub_id_reg;
-	u32 q_pri_val_reg;
-	u32 rc_q_maxsize_reg;
-	u32 bri_q_maxsize_reg;
-	u32 comp_resp_timeout_reg;
-	u32 bus_slave_control_reg;
-	u32 deadlock_avoid_type_reg;
-	u32 intpin_reg_wpermit_reg0;
-	u32 intpin_reg_wpermit_reg1;
-	u32 intpin_reg_wpermit_reg2;
-	u32 intpin_reg_wpermit_reg3;
-	u32 int_reduce_control_reg[MAX_NUM_INT_REDUCE_CONTROL_REG];
-	u32 clkcfg_reg;
-	u32 funcsel_reg;
 	void __iomem *pch_phub_base_address;
 	void __iomem *pch_phub_extrom_base_address;
 	u32 pch_mac_start_address;
@@ -149,129 +121,6 @@ static void pch_phub_read_modify_write_reg(struct pch_phub_reg *chip,
 	iowrite32(((ioread32(reg_addr) & ~mask)) | data, reg_addr);
 }
 
-/* pch_phub_save_reg_conf - saves register configuration */
-static void __maybe_unused pch_phub_save_reg_conf(struct pci_dev *pdev)
-{
-	unsigned int i;
-	struct pch_phub_reg *chip = pci_get_drvdata(pdev);
-
-	void __iomem *p = chip->pch_phub_base_address;
-
-	chip->phub_id_reg = ioread32(p + PCH_PHUB_ID_REG);
-	chip->q_pri_val_reg = ioread32(p + PCH_PHUB_QUEUE_PRI_VAL_REG);
-	chip->rc_q_maxsize_reg = ioread32(p + PCH_PHUB_RC_QUEUE_MAXSIZE_REG);
-	chip->bri_q_maxsize_reg = ioread32(p + PCH_PHUB_BRI_QUEUE_MAXSIZE_REG);
-	chip->comp_resp_timeout_reg =
-				ioread32(p + PCH_PHUB_COMP_RESP_TIMEOUT_REG);
-	chip->bus_slave_control_reg =
-				ioread32(p + PCH_PHUB_BUS_SLAVE_CONTROL_REG);
-	chip->deadlock_avoid_type_reg =
-				ioread32(p + PCH_PHUB_DEADLOCK_AVOID_TYPE_REG);
-	chip->intpin_reg_wpermit_reg0 =
-				ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG0);
-	chip->intpin_reg_wpermit_reg1 =
-				ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG1);
-	chip->intpin_reg_wpermit_reg2 =
-				ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG2);
-	chip->intpin_reg_wpermit_reg3 =
-				ioread32(p + PCH_PHUB_INTPIN_REG_WPERMIT_REG3);
-	dev_dbg(&pdev->dev, "%s : "
-		"chip->phub_id_reg=%x, "
-		"chip->q_pri_val_reg=%x, "
-		"chip->rc_q_maxsize_reg=%x, "
-		"chip->bri_q_maxsize_reg=%x, "
-		"chip->comp_resp_timeout_reg=%x, "
-		"chip->bus_slave_control_reg=%x, "
-		"chip->deadlock_avoid_type_reg=%x, "
-		"chip->intpin_reg_wpermit_reg0=%x, "
-		"chip->intpin_reg_wpermit_reg1=%x, "
-		"chip->intpin_reg_wpermit_reg2=%x, "
-		"chip->intpin_reg_wpermit_reg3=%x\n", __func__,
-		chip->phub_id_reg,
-		chip->q_pri_val_reg,
-		chip->rc_q_maxsize_reg,
-		chip->bri_q_maxsize_reg,
-		chip->comp_resp_timeout_reg,
-		chip->bus_slave_control_reg,
-		chip->deadlock_avoid_type_reg,
-		chip->intpin_reg_wpermit_reg0,
-		chip->intpin_reg_wpermit_reg1,
-		chip->intpin_reg_wpermit_reg2,
-		chip->intpin_reg_wpermit_reg3);
-	for (i = 0; i < MAX_NUM_INT_REDUCE_CONTROL_REG; i++) {
-		chip->int_reduce_control_reg[i] =
-		    ioread32(p + PCH_PHUB_INT_REDUCE_CONTROL_REG_BASE + 4 * i);
-		dev_dbg(&pdev->dev, "%s : "
-			"chip->int_reduce_control_reg[%d]=%x\n",
-			__func__, i, chip->int_reduce_control_reg[i]);
-	}
-	chip->clkcfg_reg = ioread32(p + CLKCFG_REG_OFFSET);
-	if ((chip->ioh_type == 2) || (chip->ioh_type == 4))
-		chip->funcsel_reg = ioread32(p + FUNCSEL_REG_OFFSET);
-}
-
-/* pch_phub_restore_reg_conf - restore register configuration */
-static void __maybe_unused pch_phub_restore_reg_conf(struct pci_dev *pdev)
-{
-	unsigned int i;
-	struct pch_phub_reg *chip = pci_get_drvdata(pdev);
-	void __iomem *p;
-	p = chip->pch_phub_base_address;
-
-	iowrite32(chip->phub_id_reg, p + PCH_PHUB_ID_REG);
-	iowrite32(chip->q_pri_val_reg, p + PCH_PHUB_QUEUE_PRI_VAL_REG);
-	iowrite32(chip->rc_q_maxsize_reg, p + PCH_PHUB_RC_QUEUE_MAXSIZE_REG);
-	iowrite32(chip->bri_q_maxsize_reg, p + PCH_PHUB_BRI_QUEUE_MAXSIZE_REG);
-	iowrite32(chip->comp_resp_timeout_reg,
-					p + PCH_PHUB_COMP_RESP_TIMEOUT_REG);
-	iowrite32(chip->bus_slave_control_reg,
-					p + PCH_PHUB_BUS_SLAVE_CONTROL_REG);
-	iowrite32(chip->deadlock_avoid_type_reg,
-					p + PCH_PHUB_DEADLOCK_AVOID_TYPE_REG);
-	iowrite32(chip->intpin_reg_wpermit_reg0,
-					p + PCH_PHUB_INTPIN_REG_WPERMIT_REG0);
-	iowrite32(chip->intpin_reg_wpermit_reg1,
-					p + PCH_PHUB_INTPIN_REG_WPERMIT_REG1);
-	iowrite32(chip->intpin_reg_wpermit_reg2,
-					p + PCH_PHUB_INTPIN_REG_WPERMIT_REG2);
-	iowrite32(chip->intpin_reg_wpermit_reg3,
-					p + PCH_PHUB_INTPIN_REG_WPERMIT_REG3);
-	dev_dbg(&pdev->dev, "%s : "
-		"chip->phub_id_reg=%x, "
-		"chip->q_pri_val_reg=%x, "
-		"chip->rc_q_maxsize_reg=%x, "
-		"chip->bri_q_maxsize_reg=%x, "
-		"chip->comp_resp_timeout_reg=%x, "
-		"chip->bus_slave_control_reg=%x, "
-		"chip->deadlock_avoid_type_reg=%x, "
-		"chip->intpin_reg_wpermit_reg0=%x, "
-		"chip->intpin_reg_wpermit_reg1=%x, "
-		"chip->intpin_reg_wpermit_reg2=%x, "
-		"chip->intpin_reg_wpermit_reg3=%x\n", __func__,
-		chip->phub_id_reg,
-		chip->q_pri_val_reg,
-		chip->rc_q_maxsize_reg,
-		chip->bri_q_maxsize_reg,
-		chip->comp_resp_timeout_reg,
-		chip->bus_slave_control_reg,
-		chip->deadlock_avoid_type_reg,
-		chip->intpin_reg_wpermit_reg0,
-		chip->intpin_reg_wpermit_reg1,
-		chip->intpin_reg_wpermit_reg2,
-		chip->intpin_reg_wpermit_reg3);
-	for (i = 0; i < MAX_NUM_INT_REDUCE_CONTROL_REG; i++) {
-		iowrite32(chip->int_reduce_control_reg[i],
-			p + PCH_PHUB_INT_REDUCE_CONTROL_REG_BASE + 4 * i);
-		dev_dbg(&pdev->dev, "%s : "
-			"chip->int_reduce_control_reg[%d]=%x\n",
-			__func__, i, chip->int_reduce_control_reg[i]);
-	}
-
-	iowrite32(chip->clkcfg_reg, p + CLKCFG_REG_OFFSET);
-	if ((chip->ioh_type == 2) || (chip->ioh_type == 4))
-		iowrite32(chip->funcsel_reg, p + FUNCSEL_REG_OFFSET);
-}
-
 /**
  * pch_phub_read_serial_rom() - Reading Serial ROM
  * @chip:		Pointer to the PHUB register structure
-- 
2.47.3

[PATCH v2 2/2] misc: pch_phub: Introduce an enum for device indentification

[Uwe Kleine-König (The Capable Hub)]

Instead of using magic constants give them names that make the code more
idiomatic. While touching the pci_device_id array, use named
initializers to assign .driver_data.

One side effect of this change is that the device identifying constants
now start at zero instead of one before. This is only relevant when a
PCI ID is added dynamically to the driver. Such a device is now handled
as EG20T while it resulted in a probe error before. To restore the old
behaviour the enum values can be shifted by just assigning
PCH_EG20T = 1.

Signed-off-by: Uwe Kleine-König (The Capable Hub) <u.kleine-koenig@baylibre.com>
---
 drivers/misc/pch_phub.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/misc/pch_phub.c b/drivers/misc/pch_phub.c
index 2f08957a0cd6..4d3bb411927e 100644
--- a/drivers/misc/pch_phub.c
+++ b/drivers/misc/pch_phub.c
@@ -316,7 +316,7 @@ static int pch_phub_write_gbe_mac_addr(struct pch_phub_reg *chip, u8 *data)
 	int retval;
 	int i;
 
-	if ((chip->ioh_type == 1) || (chip->ioh_type == 5)) /* EG20T or ML7831*/
+	if ((chip->ioh_type == PCH_EG20T) || (chip->ioh_type == PCH_ML7831))
 		retval = pch_phub_gbe_serial_rom_conf(chip);
 	else	/* ML7223 */
 		retval = pch_phub_gbe_serial_rom_conf_mp(chip);
@@ -509,6 +509,14 @@ static const struct bin_attribute pch_bin_attr = {
 	.write = pch_phub_bin_write,
 };
 
+enum {
+	PCH_EG20T,
+	PCH_ML7213,
+	PCH_ML7223M,
+	PCH_ML7223N,
+	PCH_ML7831,
+};
+
 static int pch_phub_probe(struct pci_dev *pdev,
 				    const struct pci_device_id *id)
 {
@@ -551,7 +559,7 @@ static int pch_phub_probe(struct pci_dev *pdev,
 
 	chip->pdev = pdev; /* Save pci device struct */
 
-	if (id->driver_data == 1) { /* EG20T PCH */
+	if (id->driver_data == PCH_EG20T) { /* EG20T PCH */
 		const char *board_name;
 		unsigned int prefetch = 0x000affaa;
 
@@ -599,7 +607,7 @@ static int pch_phub_probe(struct pci_dev *pdev,
 					CLKCFG_UART_MASK);
 			}
 		}
-	} else if (id->driver_data == 2) { /* ML7213 IOH */
+	} else if (id->driver_data == PCH_ML7213) { /* ML7213 IOH */
 		ret = sysfs_create_bin_file(&pdev->dev.kobj, &pch_bin_attr);
 		if (ret)
 			goto err_sysfs_create;
@@ -612,7 +620,7 @@ static int pch_phub_probe(struct pci_dev *pdev,
 		iowrite32(0x000affa0, chip->pch_phub_base_address + 0x14);
 		chip->pch_opt_rom_start_address =\
 						 PCH_PHUB_ROM_START_ADDR_ML7213;
-	} else if (id->driver_data == 3) { /* ML7223 IOH Bus-m*/
+	} else if (id->driver_data == PCH_ML7223M) { /* ML7223 IOH Bus-m*/
 		/* set the prefech value
 		 * Device8(GbE)
 		 */
@@ -622,7 +630,7 @@ static int pch_phub_probe(struct pci_dev *pdev,
 		chip->pch_opt_rom_start_address =\
 						 PCH_PHUB_ROM_START_ADDR_ML7223;
 		chip->pch_mac_start_address = PCH_PHUB_MAC_START_ADDR_ML7223;
-	} else if (id->driver_data == 4) { /* ML7223 IOH Bus-n*/
+	} else if (id->driver_data == PCH_ML7223N) { /* ML7223 IOH Bus-n*/
 		ret = sysfs_create_file(&pdev->dev.kobj,
 					&dev_attr_pch_mac.attr);
 		if (ret)
@@ -639,7 +647,7 @@ static int pch_phub_probe(struct pci_dev *pdev,
 		chip->pch_opt_rom_start_address =\
 						 PCH_PHUB_ROM_START_ADDR_ML7223;
 		chip->pch_mac_start_address = PCH_PHUB_MAC_START_ADDR_ML7223;
-	} else if (id->driver_data == 5) { /* ML7831 */
+	} else if (id->driver_data == PCH_ML7831) { /* ML7831 */
 		ret = sysfs_create_file(&pdev->dev.kobj,
 					&dev_attr_pch_mac.attr);
 		if (ret)
@@ -703,11 +711,11 @@ static int __maybe_unused pch_phub_resume(struct device *dev_d)
 }
 
 static const struct pci_device_id pch_phub_pcidev_id[] = {
-	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_PCH1_PHUB),       1,  },
-	{ PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ROHM_ML7213_PHUB), 2,  },
-	{ PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ROHM_ML7223_mPHUB), 3,  },
-	{ PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ROHM_ML7223_nPHUB), 4,  },
-	{ PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ROHM_ML7831_PHUB), 5,  },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_PCH1_PHUB),        .driver_data = PCH_EG20T },
+	{ PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ROHM_ML7213_PHUB),  .driver_data = PCH_ML7213 },
+	{ PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ROHM_ML7223_mPHUB), .driver_data = PCH_ML7223M },
+	{ PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ROHM_ML7223_nPHUB), .driver_data = PCH_ML7223N },
+	{ PCI_VDEVICE(ROHM, PCI_DEVICE_ID_ROHM_ML7831_PHUB),  .driver_data = PCH_ML7831 },
 	{ }
 };
 MODULE_DEVICE_TABLE(pci, pch_phub_pcidev_id);
-- 
2.47.3

Re: [PATCH v2 0/2] misc: pch_phub: Two cleanups

[Greg Kroah-Hartman]

On Sun, May 24, 2026 at 12:38:24PM +0200, Uwe Kleine-König (The Capable Hub) wrote:
> Hello,
> 
> v1 of this series is available at
> https://lore.kernel.org/cover.1779360001.git.u.kleine-koenig@baylibre.com.
> 
> Sashiko[1] found a few issues that are addressed here:
> 
>  - Drop various members from pch_phub_reg (patch #1)
>  - Also handle ioh_type comparisons against magic constants
>  - Using an enum shifts the values used to identify the different
>    hardware variants. For the corner case where new pci ids are added at
>    runtime to the driver 0 is special. I think this doesn't matter in
>    practise but I noted that in the commit log.
> 
> @Greg: I already received mail that you picked up v1 of this series. I
> hope it's not too late to drop it still?! Please tell me if you don't
> want to drop it any more, then I send incremental patches for the issues
> addressed here.

Just send an incremental set of patches please.

thanks,

greg k-h

Re: [PATCH v2 0/2] misc: pch_phub: Two cleanups

[Uwe Kleine-König (The Capable Hub)]

[-- Attachment #1: Type: text/plain, Size: 1212 bytes --]

Hello Greg,

On Sun, May 24, 2026 at 03:51:05PM +0200, Greg Kroah-Hartman wrote:
> On Sun, May 24, 2026 at 12:38:24PM +0200, Uwe Kleine-König (The Capable Hub) wrote:
> > v1 of this series is available at
> > https://lore.kernel.org/cover.1779360001.git.u.kleine-koenig@baylibre.com.
> > 
> > Sashiko[1] found a few issues that are addressed here:
> > 
> >  - Drop various members from pch_phub_reg (patch #1)
> >  - Also handle ioh_type comparisons against magic constants
> >  - Using an enum shifts the values used to identify the different
> >    hardware variants. For the corner case where new pci ids are added at
> >    runtime to the driver 0 is special. I think this doesn't matter in
> >    practise but I noted that in the commit log.
> > 
> > @Greg: I already received mail that you picked up v1 of this series. I
> > hope it's not too late to drop it still?! Please tell me if you don't
> > want to drop it any more, then I send incremental patches for the issues
> > addressed here.
> 
> Just send an incremental set of patches please.

FTR: Should be in your inbox. See
https://lore.kernel.org/lkml/cover.1779785111.git.u.kleine-koenig@baylibre.com.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]