[PATCH 0/4] Unify PCI clock detection in the HPT37x/HPT3x2N drivers

[PATCH 0/4] Unify PCI clock detection in the HPT37x/HPT3x2N drivers

[Sergey Shtylyov]

Here are 4 patches against the 'for-next' branch of Damien Le Moal's
'libata.git' repo.
The PCI clock frequency detection code in the HighPoint HPT37x/HPT3x2N drivers
has needlessly diverged, so trying to unify it...

Sergey Shtylyov (4):
  ata: pata_hpt37x: move claculating PCI clock from hpt37x_clock_slot()
  ata: pata_hpt37x: factor out hpt37x_pci_clock()
  ata: pata_hpt37x: merge hpt374_read_freq() to hpt37x_pci_clock()
  ata: pata_hpt3x2n: pass base DPLL frequency to hpt3x2n_pci_clock()

 drivers/ata/pata_hpt37x.c  | 113 +++++++++++++++++++------------------
 drivers/ata/pata_hpt3x2n.c |  19 ++++---
 2 files changed, 70 insertions(+), 62 deletions(-)

-- 
2.26.3

[PATCH 1/4] ata: pata_hpt37x: move claculating PCI clock from hpt37x_clock_slot()

[Sergey Shtylyov]

hpt37x_init_one() incorrectly calls an averaged f_CNT register value 'freq'
and hpt37x_clock_slot() takes that value as the 'freq' parameter -- rename
the former variable to 'fcnt' and move the actual code calculating the PCI
clock from hpt37x_clock_slot() to hpt37x_init_one(), along with adding the
frequency clamping code, in preparation for the factoring out the PCI clock
detection, so that this driver would become more like the 'pata_hpt3x2n'
driver...

Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
 drivers/ata/pata_hpt37x.c | 45 ++++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 17 deletions(-)

diff --git a/drivers/ata/pata_hpt37x.c b/drivers/ata/pata_hpt37x.c
index 156f304ef051..80564ea50966 100644
--- a/drivers/ata/pata_hpt37x.c
+++ b/drivers/ata/pata_hpt37x.c
@@ -23,7 +23,7 @@
 #include <linux/libata.h>
 
 #define DRV_NAME	"pata_hpt37x"
-#define DRV_VERSION	"0.6.25"
+#define DRV_VERSION	"0.6.28"
 
 struct hpt_clock {
 	u8	xfer_speed;
@@ -592,21 +592,19 @@ static struct ata_port_operations hpt374_fn1_port_ops = {
 
 /**
  *	hpt37x_clock_slot	-	Turn timing to PC clock entry
- *	@freq: Reported frequency timing
- *	@base: Base timing
+ *	@freq: Reported frequency in MHz
  *
- *	Turn the timing data intoa clock slot (0 for 33, 1 for 40, 2 for 50
+ *	Turn the timing data into a clock slot (0 for 33, 1 for 40, 2 for 50
  *	and 3 for 66Mhz)
  */
 
-static int hpt37x_clock_slot(unsigned int freq, unsigned int base)
+static int hpt37x_clock_slot(unsigned int freq)
 {
-	unsigned int f = (base * freq) / 192;	/* Mhz */
-	if (f < 40)
+	if (freq < 40)
 		return 0;	/* 33Mhz slot */
-	if (f < 45)
+	if (freq < 45)
 		return 1;	/* 40Mhz slot */
-	if (f < 55)
+	if (freq < 55)
 		return 2;	/* 50Mhz slot */
 	return 3;		/* 60Mhz slot */
 }
@@ -770,7 +768,8 @@ static int hpt37x_init_one(struct pci_dev *dev, const struct pci_device_id *id)
 	u8 rev = dev->revision;
 	u8 irqmask;
 	u8 mcr1;
-	u32 freq;
+	unsigned int freq; /* MHz */
+	u32 fcnt;
 	int prefer_dpll = 1;
 
 	unsigned long iobase = pci_resource_start(dev, 4);
@@ -903,13 +902,13 @@ static int hpt37x_init_one(struct pci_dev *dev, const struct pci_device_id *id)
 	 */
 
 	if (chip_table == &hpt374) {
-		freq = hpt374_read_freq(dev);
-		if (freq == 0)
+		fcnt = hpt374_read_freq(dev);
+		if (fcnt == 0)
 			return -ENODEV;
 	} else
-		freq = inl(iobase + 0x90);
+		fcnt = inl(iobase + 0x90);
 
-	if ((freq >> 12) != 0xABCDE) {
+	if ((fcnt >> 12) != 0xABCDE) {
 		int i;
 		u16 sr;
 		u32 total = 0;
@@ -922,16 +921,28 @@ static int hpt37x_init_one(struct pci_dev *dev, const struct pci_device_id *id)
 			total += sr & 0x1FF;
 			udelay(15);
 		}
-		freq = total / 128;
+		fcnt = total / 128;
 	}
-	freq &= 0x1FF;
+	fcnt &= 0x1FF;
+
+	freq = (fcnt * chip_table->base) / 192;	/* Mhz */
+
+	/* Clamp to bands */
+	if (freq < 40)
+		freq = 33;
+	else if (freq < 45)
+		freq = 40;
+	else if (freq < 55)
+		freq = 50;
+	else
+		freq = 66;
 
 	/*
 	 *	Turn the frequency check into a band and then find a timing
 	 *	table to match it.
 	 */
 
-	clock_slot = hpt37x_clock_slot(freq, chip_table->base);
+	clock_slot = hpt37x_clock_slot(freq);
 	if (chip_table->clocks[clock_slot] == NULL || prefer_dpll) {
 		/*
 		 *	We need to try PLL mode instead
-- 
2.26.3

[PATCH 2/4] ata: pata_hpt37x: factor out hpt37x_pci_clock()

[Sergey Shtylyov]

Factor out the PCI clock frequency detection code into hpt37x_pci_clock(),
so that this driver becomes more like 'pata_hpt3x2n'.  Note that I decided
to change the way HPT374 is identified to using the PCI device ID...

Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
 drivers/ata/pata_hpt37x.c | 94 +++++++++++++++++++++------------------
 1 file changed, 51 insertions(+), 43 deletions(-)

diff --git a/drivers/ata/pata_hpt37x.c b/drivers/ata/pata_hpt37x.c
index 80564ea50966..38fc7f3d593c 100644
--- a/drivers/ata/pata_hpt37x.c
+++ b/drivers/ata/pata_hpt37x.c
@@ -23,7 +23,7 @@
 #include <linux/libata.h>
 
 #define DRV_NAME	"pata_hpt37x"
-#define DRV_VERSION	"0.6.28"
+#define DRV_VERSION	"0.6.29"
 
 struct hpt_clock {
 	u8	xfer_speed;
@@ -664,6 +664,53 @@ static u32 hpt374_read_freq(struct pci_dev *pdev)
 	return freq;
 }
 
+static int hpt37x_pci_clock(struct pci_dev *pdev, unsigned int base)
+{
+	unsigned int freq;
+	u32 fcnt;
+
+	/*
+	 * Some devices do not let this value be accessed via PCI space
+	 * according to the old driver. In addition we must use the value
+	 * from FN 0 on the HPT374.
+	 */
+	if (pdev->device == PCI_DEVICE_ID_TTI_HPT374) {
+		fcnt = hpt374_read_freq(pdev);
+		if (!fcnt)
+			return 0;
+	} else	{
+		fcnt = inl(pci_resource_start(pdev, 4) + 0x90);
+	}
+
+	if ((fcnt >> 12) != 0xABCDE) {
+		u32 total = 0;
+		int i;
+		u16 sr;
+
+		dev_warn(&pdev->dev, "BIOS clock data not set\n");
+
+		/* This is the process the HPT371 BIOS is reported to use */
+		for (i = 0; i < 128; i++) {
+			pci_read_config_word(pdev, 0x78, &sr);
+			total += sr & 0x1FF;
+			udelay(15);
+		}
+		fcnt = total / 128;
+	}
+	fcnt &= 0x1FF;
+
+	freq = (fcnt * base) / 192;	/* in MHz */
+
+	/* Clamp to bands */
+	if (freq < 40)
+		return 33;
+	if (freq < 45)
+		return 40;
+	if (freq < 55)
+		return 50;
+	return 66;
+}
+
 /**
  *	hpt37x_init_one		-	Initialise an HPT37X/302
  *	@dev: PCI device
@@ -769,7 +816,6 @@ static int hpt37x_init_one(struct pci_dev *dev, const struct pci_device_id *id)
 	u8 irqmask;
 	u8 mcr1;
 	unsigned int freq; /* MHz */
-	u32 fcnt;
 	int prefer_dpll = 1;
 
 	unsigned long iobase = pci_resource_start(dev, 4);
@@ -895,47 +941,9 @@ static int hpt37x_init_one(struct pci_dev *dev, const struct pci_device_id *id)
 	if (chip_table == &hpt372a)
 		outb(0x0e, iobase + 0x9c);
 
-	/*
-	 * Some devices do not let this value be accessed via PCI space
-	 * according to the old driver. In addition we must use the value
-	 * from FN 0 on the HPT374.
-	 */
-
-	if (chip_table == &hpt374) {
-		fcnt = hpt374_read_freq(dev);
-		if (fcnt == 0)
-			return -ENODEV;
-	} else
-		fcnt = inl(iobase + 0x90);
-
-	if ((fcnt >> 12) != 0xABCDE) {
-		int i;
-		u16 sr;
-		u32 total = 0;
-
-		dev_warn(&dev->dev, "BIOS has not set timing clocks\n");
-
-		/* This is the process the HPT371 BIOS is reported to use */
-		for (i = 0; i < 128; i++) {
-			pci_read_config_word(dev, 0x78, &sr);
-			total += sr & 0x1FF;
-			udelay(15);
-		}
-		fcnt = total / 128;
-	}
-	fcnt &= 0x1FF;
-
-	freq = (fcnt * chip_table->base) / 192;	/* Mhz */
-
-	/* Clamp to bands */
-	if (freq < 40)
-		freq = 33;
-	else if (freq < 45)
-		freq = 40;
-	else if (freq < 55)
-		freq = 50;
-	else
-		freq = 66;
+	freq = hpt37x_pci_clock(dev, chip_table->base);
+	if (!freq)
+		return -ENODEV;
 
 	/*
 	 *	Turn the frequency check into a band and then find a timing
-- 
2.26.3

[PATCH 3/4] ata: pata_hpt37x: merge hpt374_read_freq() to hpt37x_pci_clock()

[Sergey Shtylyov]

With hpt374_read_freq() implemented as a separate function, there's
some code duplication going on, not to mention that this function is
named incorrectly: it returns the f_CNT register value saved by BIOS,
not the PCI clock frequency.
Folding hpt374_read_freq() into hpt37x_pci_clock() saves 20 bytes of
object code with x86_64 gcc 10.3.1...

Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
 drivers/ata/pata_hpt37x.c | 34 ++++++++++------------------------
 1 file changed, 10 insertions(+), 24 deletions(-)

diff --git a/drivers/ata/pata_hpt37x.c b/drivers/ata/pata_hpt37x.c
index 38fc7f3d593c..d1a3d99d5d0a 100644
--- a/drivers/ata/pata_hpt37x.c
+++ b/drivers/ata/pata_hpt37x.c
@@ -23,7 +23,7 @@
 #include <linux/libata.h>
 
 #define DRV_NAME	"pata_hpt37x"
-#define DRV_VERSION	"0.6.29"
+#define DRV_VERSION	"0.6.30"
 
 struct hpt_clock {
 	u8	xfer_speed;
@@ -644,26 +644,6 @@ static int hpt37x_calibrate_dpll(struct pci_dev *dev)
 	return 0;
 }
 
-static u32 hpt374_read_freq(struct pci_dev *pdev)
-{
-	u32 freq;
-	unsigned long io_base = pci_resource_start(pdev, 4);
-
-	if (PCI_FUNC(pdev->devfn) & 1) {
-		struct pci_dev *pdev_0;
-
-		pdev_0 = pci_get_slot(pdev->bus, pdev->devfn - 1);
-		/* Someone hot plugged the controller on us ? */
-		if (pdev_0 == NULL)
-			return 0;
-		io_base = pci_resource_start(pdev_0, 4);
-		freq = inl(io_base + 0x90);
-		pci_dev_put(pdev_0);
-	} else
-		freq = inl(io_base + 0x90);
-	return freq;
-}
-
 static int hpt37x_pci_clock(struct pci_dev *pdev, unsigned int base)
 {
 	unsigned int freq;
@@ -674,10 +654,16 @@ static int hpt37x_pci_clock(struct pci_dev *pdev, unsigned int base)
 	 * according to the old driver. In addition we must use the value
 	 * from FN 0 on the HPT374.
 	 */
-	if (pdev->device == PCI_DEVICE_ID_TTI_HPT374) {
-		fcnt = hpt374_read_freq(pdev);
-		if (!fcnt)
+	if (pdev->device == PCI_DEVICE_ID_TTI_HPT374 &&
+	    (PCI_FUNC(pdev->devfn) & 1)) {
+		struct pci_dev *pdev_fn0;
+
+		pdev_fn0 = pci_get_slot(pdev->bus, pdev->devfn - 1);
+		/* Someone hot plugged the controller on us? */
+		if (!pdev_fn0)
 			return 0;
+		fcnt = inl(pci_resource_start(pdev_fn0, 4) + 0x90);
+		pci_dev_put(pdev_fn0);
 	} else	{
 		fcnt = inl(pci_resource_start(pdev, 4) + 0x90);
 	}
-- 
2.26.3

[PATCH 4/4] ata: pata_hpt3x2n: pass base DPLL frequency to hpt3x2n_pci_clock()

[Sergey Shtylyov]

Currently, the base DPLL frequency is hardcoded in hpt3x2n_pci_clock().
Align with the updated 'pata_hpt37x' driver, where this frequency is a
parameter to hpt37x_pci_clock().

While at it, also do the following to align with the 'pata_hpt37x' driver:
- fix the 'freq' local variable's type;
- remove the 'iobase' local variable;
- extend the comment to the inl() call;
- move the 'total' local variable's declaration.

Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
 drivers/ata/pata_hpt3x2n.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/ata/pata_hpt3x2n.c b/drivers/ata/pata_hpt3x2n.c
index 1f6afd8ee29b..d1595e17dca2 100644
--- a/drivers/ata/pata_hpt3x2n.c
+++ b/drivers/ata/pata_hpt3x2n.c
@@ -24,7 +24,7 @@
 #include <linux/libata.h>
 
 #define DRV_NAME	"pata_hpt3x2n"
-#define DRV_VERSION	"0.3.18"
+#define DRV_VERSION	"0.3.19"
 
 enum {
 	PCI66		=	(1 << 1),
@@ -403,17 +403,20 @@ static int hpt3xn_calibrate_dpll(struct pci_dev *dev)
 	return 0;
 }
 
-static int hpt3x2n_pci_clock(struct pci_dev *pdev)
+static int hpt3x2n_pci_clock(struct pci_dev *pdev, unsigned int base)
 {
-	unsigned long freq;
+	unsigned int freq;
 	u32 fcnt;
-	unsigned long iobase = pci_resource_start(pdev, 4);
 
-	fcnt = inl(iobase + 0x90);	/* Not PCI readable for some chips */
+	/*
+	 * Some devices do not let this value be accessed via PCI space
+	 * according to the old driver.
+	 */
+	fcnt = inl(pci_resource_start(pdev, 4) + 0x90);
 	if ((fcnt >> 12) != 0xABCDE) {
+		u32 total = 0;
 		int i;
 		u16 sr;
-		u32 total = 0;
 
 		dev_warn(&pdev->dev, "BIOS clock data not set\n");
 
@@ -427,7 +430,7 @@ static int hpt3x2n_pci_clock(struct pci_dev *pdev)
 	}
 	fcnt &= 0x1FF;
 
-	freq = (fcnt * 77) / 192;
+	freq = (fcnt * base) / 192;	/* in MHz */
 
 	/* Clamp to bands */
 	if (freq < 40)
@@ -559,7 +562,7 @@ static int hpt3x2n_init_one(struct pci_dev *dev, const struct pci_device_id *id)
 	 * 50 for UDMA100. Right now we always use 66
 	 */
 
-	pci_mhz = hpt3x2n_pci_clock(dev);
+	pci_mhz = hpt3x2n_pci_clock(dev, 77);
 
 	f_low = (pci_mhz * 48) / 66;	/* PCI Mhz for 66Mhz DPLL */
 	f_high = f_low + 2;		/* Tolerance */
-- 
2.26.3

Re: [PATCH 0/4] Unify PCI clock detection in the HPT37x/HPT3x2N drivers

[Damien Le Moal]

On 5/16/22 05:41, Sergey Shtylyov wrote:
> Here are 4 patches against the 'for-next' branch of Damien Le Moal's
> 'libata.git' repo.
> The PCI clock frequency detection code in the HighPoint HPT37x/HPT3x2N drivers
> has needlessly diverged, so trying to unify it...
> 
> Sergey Shtylyov (4):
>   ata: pata_hpt37x: move claculating PCI clock from hpt37x_clock_slot()
>   ata: pata_hpt37x: factor out hpt37x_pci_clock()
>   ata: pata_hpt37x: merge hpt374_read_freq() to hpt37x_pci_clock()
>   ata: pata_hpt3x2n: pass base DPLL frequency to hpt3x2n_pci_clock()
> 
>  drivers/ata/pata_hpt37x.c  | 113 +++++++++++++++++++------------------
>  drivers/ata/pata_hpt3x2n.c |  19 ++++---
>  2 files changed, 70 insertions(+), 62 deletions(-)
> 

Applied to for-5.20. Thanks !


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 0/4] Unify PCI clock detection in the HPT37x/HPT3x2N drivers

[Sergey Shtylyov]

On 6/8/22 9:47 AM, Damien Le Moal wrote:
[...]
>> Here are 4 patches against the 'for-next' branch of Damien Le Moal's
>> 'libata.git' repo.
>> The PCI clock frequency detection code in the HighPoint HPT37x/HPT3x2N drivers
>> has needlessly diverged, so trying to unify it...
>>
>> Sergey Shtylyov (4):
>>   ata: pata_hpt37x: move claculating PCI clock from hpt37x_clock_slot()
>>   ata: pata_hpt37x: factor out hpt37x_pci_clock()
>>   ata: pata_hpt37x: merge hpt374_read_freq() to hpt37x_pci_clock()
>>   ata: pata_hpt3x2n: pass base DPLL frequency to hpt3x2n_pci_clock()
>>
>>  drivers/ata/pata_hpt37x.c  | 113 +++++++++++++++++++------------------
>>  drivers/ata/pata_hpt3x2n.c |  19 ++++---
>>  2 files changed, 70 insertions(+), 62 deletions(-)
>>
> 
> Applied to for-5.20. Thanks !

   Thanks, better late than never... :-)

MBR, Sergey