The EndRun PTP/1588 dual serial port device is based on the Oxford
Semiconductor OXPCIe952 UART device with the PCI vendor:device ID set
for EndRun Technologies and uses the same sequence to determine the
number of ports available. Despite that we have duplicate code
specific to the EndRun device.
Remove redundant code then and factor out OxSemi Tornado device
detection. Also correct the baud base like with commit 6cbe45d8ac93
("serial: 8250: Correct the clock for OxSemi PCIe devices") for the
value of 3906250 rather than 4000000, obtained by dividing the 62.5MHz
clock input by the default oversampling rate of 16. Finally move the
EndRun vendor:device ID to <linux/pci_ids.h>.
Signed-off-by: Maciej W. Rozycki <[email protected]>
Fixes: 1bc8cde46a159 ("8250_pci: Added driver for Endrun Technologies PTP PCIe card.")
---
New change in v3.
---
drivers/tty/serial/8250/8250_pci.c | 79 +++++++++++--------------------------
include/linux/pci_ids.h | 3 +
2 files changed, 28 insertions(+), 54 deletions(-)
linux-serial-8250-oxsemi-endrun.diff
Index: linux-macro/drivers/tty/serial/8250/8250_pci.c
===================================================================
--- linux-macro.orig/drivers/tty/serial/8250/8250_pci.c
+++ linux-macro/drivers/tty/serial/8250/8250_pci.c
@@ -994,41 +994,26 @@ static void pci_ite887x_exit(struct pci_
}
/*
- * EndRun Technologies.
- * Determine the number of ports available on the device.
+ * Oxford Semiconductor Inc.
+ * Check if an OxSemi device is part of the Tornado range of devices.
*/
-#define PCI_VENDOR_ID_ENDRUN 0x7401
-#define PCI_DEVICE_ID_ENDRUN_1588 0xe100
-
-static int pci_endrun_init(struct pci_dev *dev)
+static bool pci_oxsemi_tornado_p(struct pci_dev *dev)
{
- u8 __iomem *p;
- unsigned long deviceID;
- unsigned int number_uarts = 0;
+ /* OxSemi Tornado devices are all 0xCxxx */
+ if (dev->vendor == PCI_VENDOR_ID_OXSEMI &&
+ (dev->device & 0xf000) != 0xc000)
+ return false;
- /* EndRun device is all 0xexxx */
+ /* EndRun devices are all 0xExxx */
if (dev->vendor == PCI_VENDOR_ID_ENDRUN &&
- (dev->device & 0xf000) != 0xe000)
- return 0;
-
- p = pci_iomap(dev, 0, 5);
- if (p == NULL)
- return -ENOMEM;
+ (dev->device & 0xf000) != 0xe000)
+ return false;
- deviceID = ioread32(p);
- /* EndRun device */
- if (deviceID == 0x07000200) {
- number_uarts = ioread8(p + 4);
- pci_dbg(dev, "%d ports detected on EndRun PCI Express device\n", number_uarts);
- }
- pci_iounmap(dev, p);
- return number_uarts;
+ return true;
}
/*
- * Oxford Semiconductor Inc.
- * Check that device is part of the Tornado range of devices, then determine
- * the number of ports available on the device.
+ * Determine the number of ports available on a Tornado device.
*/
static int pci_oxsemi_tornado_init(struct pci_dev *dev)
{
@@ -1036,9 +1021,7 @@ static int pci_oxsemi_tornado_init(struc
unsigned long deviceID;
unsigned int number_uarts = 0;
- /* OxSemi Tornado devices are all 0xCxxx */
- if (dev->vendor == PCI_VENDOR_ID_OXSEMI &&
- (dev->device & 0xF000) != 0xC000)
+ if (!pci_oxsemi_tornado_p(dev))
return 0;
p = pci_iomap(dev, 0, 5);
@@ -1049,7 +1032,10 @@ static int pci_oxsemi_tornado_init(struc
/* Tornado device */
if (deviceID == 0x07000200) {
number_uarts = ioread8(p + 4);
- pci_dbg(dev, "%d ports detected on Oxford PCI Express device\n", number_uarts);
+ pci_dbg(dev, "%d ports detected on %s PCI Express device\n",
+ number_uarts,
+ dev->vendor == PCI_VENDOR_ID_ENDRUN ?
+ "EndRun" : "Oxford");
}
pci_iounmap(dev, p);
return number_uarts;
@@ -2244,7 +2230,7 @@ static struct pci_serial_quirk pci_seria
.device = PCI_ANY_ID,
.subvendor = PCI_ANY_ID,
.subdevice = PCI_ANY_ID,
- .init = pci_endrun_init,
+ .init = pci_oxsemi_tornado_init,
.setup = pci_default_setup,
},
/*
@@ -2667,7 +2653,6 @@ enum pci_board_num_t {
pbn_panacom2,
pbn_panacom4,
pbn_plx_romulus,
- pbn_endrun_2_4000000,
pbn_oxsemi,
pbn_oxsemi_1_3906250,
pbn_oxsemi_2_3906250,
@@ -3190,20 +3175,6 @@ static struct pciserial_board pci_boards
},
/*
- * EndRun Technologies
- * Uses the size of PCI Base region 0 to
- * signal now many ports are available
- * 2 port 952 Uart support
- */
- [pbn_endrun_2_4000000] = {
- .flags = FL_BASE0,
- .num_ports = 2,
- .base_baud = 4000000,
- .uart_offset = 0x200,
- .first_offset = 0x1000,
- },
-
- /*
* This board uses the size of PCI Base region 0 to
* signal now many ports are available
*/
@@ -4123,13 +4094,6 @@ static const struct pci_device_id serial
0x10b5, 0x106a, 0, 0,
pbn_plx_romulus },
/*
- * EndRun Technologies. PCI express device range.
- * EndRun PTP/1588 has 2 Native UARTs.
- */
- { PCI_VENDOR_ID_ENDRUN, PCI_DEVICE_ID_ENDRUN_1588,
- PCI_ANY_ID, PCI_ANY_ID, 0, 0,
- pbn_endrun_2_4000000 },
- /*
* Quatech cards. These actually have configurable clocks but for
* now we just use the default.
*
@@ -4390,6 +4354,13 @@ static const struct pci_device_id serial
{ PCI_VENDOR_ID_DIGI, PCIE_DEVICE_ID_NEO_2_OX_IBM,
PCI_SUBVENDOR_ID_IBM, PCI_ANY_ID, 0, 0,
pbn_oxsemi_2_3906250 },
+ /*
+ * EndRun Technologies. PCI express device range.
+ * EndRun PTP/1588 has 2 Native UARTs utilizing OxSemi 952.
+ */
+ { PCI_VENDOR_ID_ENDRUN, PCI_DEVICE_ID_ENDRUN_1588,
+ PCI_ANY_ID, PCI_ANY_ID, 0, 0,
+ pbn_oxsemi_2_3906250 },
/*
* SBS Technologies, Inc. P-Octal and PMC-OCTPRO cards,
Index: linux-macro/include/linux/pci_ids.h
===================================================================
--- linux-macro.orig/include/linux/pci_ids.h
+++ linux-macro/include/linux/pci_ids.h
@@ -2622,6 +2622,9 @@
#define PCI_DEVICE_ID_DCI_PCCOM8 0x0002
#define PCI_DEVICE_ID_DCI_PCCOM2 0x0004
+#define PCI_VENDOR_ID_ENDRUN 0x7401
+#define PCI_DEVICE_ID_ENDRUN_1588 0xe100
+
#define PCI_VENDOR_ID_INTEL 0x8086
#define PCI_DEVICE_ID_INTEL_EESSC 0x0008
#define PCI_DEVICE_ID_INTEL_PXHD_0 0x0320
On Thu, Mar 31, 2022 at 08:11:42AM +0100, Maciej W. Rozycki wrote:
> The EndRun PTP/1588 dual serial port device is based on the Oxford
> Semiconductor OXPCIe952 UART device with the PCI vendor:device ID set
> for EndRun Technologies and uses the same sequence to determine the
> number of ports available. Despite that we have duplicate code
> specific to the EndRun device.
>
> Remove redundant code then and factor out OxSemi Tornado device
> detection. Also correct the baud base like with commit 6cbe45d8ac93
> ("serial: 8250: Correct the clock for OxSemi PCIe devices") for the
> value of 3906250 rather than 4000000, obtained by dividing the 62.5MHz
> clock input by the default oversampling rate of 16. Finally move the
> EndRun vendor:device ID to <linux/pci_ids.h>.
That's a lot of different things happening all the same commit. Please
break this out into one-patch-per-logical-change as is required.
thanks,
greg k-h
On Thu, Mar 31, 2022 at 08:11:42AM +0100, Maciej W. Rozycki wrote:
> The EndRun PTP/1588 dual serial port device is based on the Oxford
> Semiconductor OXPCIe952 UART device with the PCI vendor:device ID set
> for EndRun Technologies and uses the same sequence to determine the
> number of ports available. Despite that we have duplicate code
> specific to the EndRun device.
>
> Remove redundant code then and factor out OxSemi Tornado device
> detection. Also correct the baud base like with commit 6cbe45d8ac93
> ("serial: 8250: Correct the clock for OxSemi PCIe devices") for the
> value of 3906250 rather than 4000000, obtained by dividing the 62.5MHz
> clock input by the default oversampling rate of 16. Finally move the
> EndRun vendor:device ID to <linux/pci_ids.h>.
>
> Signed-off-by: Maciej W. Rozycki <[email protected]>
> Fixes: 1bc8cde46a159 ("8250_pci: Added driver for Endrun Technologies PTP PCIe card.")
> ---
> New change in v3.
> ---
> drivers/tty/serial/8250/8250_pci.c | 79 +++++++++++--------------------------
> include/linux/pci_ids.h | 3 +
> 2 files changed, 28 insertions(+), 54 deletions(-)
>
> linux-serial-8250-oxsemi-endrun.diff
> Index: linux-macro/drivers/tty/serial/8250/8250_pci.c
> ===================================================================
> --- linux-macro.orig/drivers/tty/serial/8250/8250_pci.c
> +++ linux-macro/drivers/tty/serial/8250/8250_pci.c
> @@ -994,41 +994,26 @@ static void pci_ite887x_exit(struct pci_
> }
>
> /*
> - * EndRun Technologies.
> - * Determine the number of ports available on the device.
> + * Oxford Semiconductor Inc.
> + * Check if an OxSemi device is part of the Tornado range of devices.
> */
> -#define PCI_VENDOR_ID_ENDRUN 0x7401
> -#define PCI_DEVICE_ID_ENDRUN_1588 0xe100
> -
> -static int pci_endrun_init(struct pci_dev *dev)
> +static bool pci_oxsemi_tornado_p(struct pci_dev *dev)
> {
> - u8 __iomem *p;
> - unsigned long deviceID;
> - unsigned int number_uarts = 0;
> + /* OxSemi Tornado devices are all 0xCxxx */
> + if (dev->vendor == PCI_VENDOR_ID_OXSEMI &&
> + (dev->device & 0xf000) != 0xc000)
> + return false;
>
> - /* EndRun device is all 0xexxx */
> + /* EndRun devices are all 0xExxx */
> if (dev->vendor == PCI_VENDOR_ID_ENDRUN &&
> - (dev->device & 0xf000) != 0xe000)
> - return 0;
> -
> - p = pci_iomap(dev, 0, 5);
> - if (p == NULL)
> - return -ENOMEM;
> + (dev->device & 0xf000) != 0xe000)
> + return false;
>
> - deviceID = ioread32(p);
> - /* EndRun device */
> - if (deviceID == 0x07000200) {
> - number_uarts = ioread8(p + 4);
> - pci_dbg(dev, "%d ports detected on EndRun PCI Express device\n", number_uarts);
> - }
> - pci_iounmap(dev, p);
> - return number_uarts;
> + return true;
> }
>
> /*
> - * Oxford Semiconductor Inc.
> - * Check that device is part of the Tornado range of devices, then determine
> - * the number of ports available on the device.
> + * Determine the number of ports available on a Tornado device.
> */
> static int pci_oxsemi_tornado_init(struct pci_dev *dev)
> {
> @@ -1036,9 +1021,7 @@ static int pci_oxsemi_tornado_init(struc
> unsigned long deviceID;
> unsigned int number_uarts = 0;
>
> - /* OxSemi Tornado devices are all 0xCxxx */
> - if (dev->vendor == PCI_VENDOR_ID_OXSEMI &&
> - (dev->device & 0xF000) != 0xC000)
> + if (!pci_oxsemi_tornado_p(dev))
> return 0;
>
> p = pci_iomap(dev, 0, 5);
> @@ -1049,7 +1032,10 @@ static int pci_oxsemi_tornado_init(struc
> /* Tornado device */
> if (deviceID == 0x07000200) {
> number_uarts = ioread8(p + 4);
> - pci_dbg(dev, "%d ports detected on Oxford PCI Express device\n", number_uarts);
> + pci_dbg(dev, "%d ports detected on %s PCI Express device\n",
> + number_uarts,
> + dev->vendor == PCI_VENDOR_ID_ENDRUN ?
> + "EndRun" : "Oxford");
> }
> pci_iounmap(dev, p);
> return number_uarts;
> @@ -2244,7 +2230,7 @@ static struct pci_serial_quirk pci_seria
> .device = PCI_ANY_ID,
> .subvendor = PCI_ANY_ID,
> .subdevice = PCI_ANY_ID,
> - .init = pci_endrun_init,
> + .init = pci_oxsemi_tornado_init,
> .setup = pci_default_setup,
> },
> /*
> @@ -2667,7 +2653,6 @@ enum pci_board_num_t {
> pbn_panacom2,
> pbn_panacom4,
> pbn_plx_romulus,
> - pbn_endrun_2_4000000,
> pbn_oxsemi,
> pbn_oxsemi_1_3906250,
> pbn_oxsemi_2_3906250,
> @@ -3190,20 +3175,6 @@ static struct pciserial_board pci_boards
> },
>
> /*
> - * EndRun Technologies
> - * Uses the size of PCI Base region 0 to
> - * signal now many ports are available
> - * 2 port 952 Uart support
> - */
> - [pbn_endrun_2_4000000] = {
> - .flags = FL_BASE0,
> - .num_ports = 2,
> - .base_baud = 4000000,
> - .uart_offset = 0x200,
> - .first_offset = 0x1000,
> - },
> -
> - /*
> * This board uses the size of PCI Base region 0 to
> * signal now many ports are available
> */
> @@ -4123,13 +4094,6 @@ static const struct pci_device_id serial
> 0x10b5, 0x106a, 0, 0,
> pbn_plx_romulus },
> /*
> - * EndRun Technologies. PCI express device range.
> - * EndRun PTP/1588 has 2 Native UARTs.
> - */
> - { PCI_VENDOR_ID_ENDRUN, PCI_DEVICE_ID_ENDRUN_1588,
> - PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> - pbn_endrun_2_4000000 },
> - /*
> * Quatech cards. These actually have configurable clocks but for
> * now we just use the default.
> *
> @@ -4390,6 +4354,13 @@ static const struct pci_device_id serial
> { PCI_VENDOR_ID_DIGI, PCIE_DEVICE_ID_NEO_2_OX_IBM,
> PCI_SUBVENDOR_ID_IBM, PCI_ANY_ID, 0, 0,
> pbn_oxsemi_2_3906250 },
> + /*
> + * EndRun Technologies. PCI express device range.
> + * EndRun PTP/1588 has 2 Native UARTs utilizing OxSemi 952.
> + */
> + { PCI_VENDOR_ID_ENDRUN, PCI_DEVICE_ID_ENDRUN_1588,
> + PCI_ANY_ID, PCI_ANY_ID, 0, 0,
> + pbn_oxsemi_2_3906250 },
>
> /*
> * SBS Technologies, Inc. P-Octal and PMC-OCTPRO cards,
> Index: linux-macro/include/linux/pci_ids.h
> ===================================================================
> --- linux-macro.orig/include/linux/pci_ids.h
> +++ linux-macro/include/linux/pci_ids.h
> @@ -2622,6 +2622,9 @@
> #define PCI_DEVICE_ID_DCI_PCCOM8 0x0002
> #define PCI_DEVICE_ID_DCI_PCCOM2 0x0004
>
> +#define PCI_VENDOR_ID_ENDRUN 0x7401
> +#define PCI_DEVICE_ID_ENDRUN_1588 0xe100
As per the top of this file, this should not be needed here as you are
only using it in one file. Please leave it as-is.
thanks,
greg k-h
On Fri, 15 Apr 2022, Greg Kroah-Hartman wrote:
> > Remove redundant code then and factor out OxSemi Tornado device
> > detection. Also correct the baud base like with commit 6cbe45d8ac93
> > ("serial: 8250: Correct the clock for OxSemi PCIe devices") for the
> > value of 3906250 rather than 4000000, obtained by dividing the 62.5MHz
> > clock input by the default oversampling rate of 16. Finally move the
> > EndRun vendor:device ID to <linux/pci_ids.h>.
>
> That's a lot of different things happening all the same commit. Please
> break this out into one-patch-per-logical-change as is required.
The baud base fix is completely swallowed by the next change for EndRun
devices, but I guess someone may want to backport it on its own, however
unlikely.
I have posted v4 then with this change split off (and the other removed)
as per your request. I have also reconsidered the changes made in 2/2 and
split it into three, so that drivers/tty/serial/8250/8250_port.c updates
are separate and self-contained.
In the course of the respin, I have realised exporting the ICR access
helpers caused a code generation regression, so I have removed the inline
function specifier so as to let the compiler choose whether to inline the
functions or not. I have also realised that the change to the console
restorer is actually a fix for a preexisting bug in handling of the AFE
bit, so I have annotated the change accordingly.
Thank you for your review.
Maciej
On Fri, 15 Apr 2022, Greg Kroah-Hartman wrote:
> > Index: linux-macro/include/linux/pci_ids.h
> > ===================================================================
> > --- linux-macro.orig/include/linux/pci_ids.h
> > +++ linux-macro/include/linux/pci_ids.h
> > @@ -2622,6 +2622,9 @@
> > #define PCI_DEVICE_ID_DCI_PCCOM8 0x0002
> > #define PCI_DEVICE_ID_DCI_PCCOM2 0x0004
> >
> > +#define PCI_VENDOR_ID_ENDRUN 0x7401
> > +#define PCI_DEVICE_ID_ENDRUN_1588 0xe100
>
> As per the top of this file, this should not be needed here as you are
> only using it in one file. Please leave it as-is.
I find this requirement silly, but here it's not the place to discuss it,
so I have removed this part as requested. At least it's not inline magic
numbers here.
Maciej
On Mon, Apr 18, 2022 at 12:02:54AM +0100, Maciej W. Rozycki wrote:
> On Fri, 15 Apr 2022, Greg Kroah-Hartman wrote:
>
> > > Index: linux-macro/include/linux/pci_ids.h
> > > ===================================================================
> > > --- linux-macro.orig/include/linux/pci_ids.h
> > > +++ linux-macro/include/linux/pci_ids.h
> > > @@ -2622,6 +2622,9 @@
> > > #define PCI_DEVICE_ID_DCI_PCCOM8 0x0002
> > > #define PCI_DEVICE_ID_DCI_PCCOM2 0x0004
> > >
> > > +#define PCI_VENDOR_ID_ENDRUN 0x7401
> > > +#define PCI_DEVICE_ID_ENDRUN_1588 0xe100
> >
> > As per the top of this file, this should not be needed here as you are
> > only using it in one file. Please leave it as-is.
>
> I find this requirement silly, but here it's not the place to discuss it,
You have not had to deal with merge issues in this file before. Think
about every single PCI driver author updating this single file. That
just does not work at the scale we run at, sorry. I put this rule into
place 15+ years ago for that reason.
thanks,
greg k-h
On Wed, 20 Apr 2022, Greg Kroah-Hartman wrote:
> > > As per the top of this file, this should not be needed here as you are
> > > only using it in one file. Please leave it as-is.
> >
> > I find this requirement silly, but here it's not the place to discuss it,
>
> You have not had to deal with merge issues in this file before. Think
> about every single PCI driver author updating this single file. That
> just does not work at the scale we run at, sorry. I put this rule into
> place 15+ years ago for that reason.
Fair enough, I missed this practical aspect. Thanks for straightening me
out.
Maciej