mbox series

[RESEND,v3,0/2] serial: 8250: Fixes for Oxford Semiconductor 950 UARTs

Message ID alpine.DEB.2.21.2203310114210.44113@angie.orcam.me.uk
Headers show
Series serial: 8250: Fixes for Oxford Semiconductor 950 UARTs | expand

Message

Maciej W. Rozycki March 31, 2022, 7:11 a.m. UTC 
Hi,

 Resending as requested; with EndRun participants removed, as clearly they 
have not been interested.

 Here's v3 of the outstanding fixes for Oxford Semiconductor 950 UARTs.  
As the change for the default FIFO rx trigger level has been already 
merged with commit d7aff291d069 ("serial: 8250: Define RX trigger levels 
for OxSemi 950 devices") only one patch of the original series remains.

 However in the course of preparing v3 of that change I have noticed that 
the EndRun device is actually also an OxSemi 952 device in disguise (note 
that the OxSemi chips have fully customer-programmable PCI vendor:device 
ID values).  Therefore it requires a similar fix to the base baud rate as 
with commit 6cbe45d8ac93 ("serial: 8250: Correct the clock for OxSemi PCIe 
devices"), and also duplicate code can be removed.

 I have therefore added a fix for the EndRun device as 1/2 in this version 
and the original outstanding change is now 2/2, updated accordingly, also 
for a change in the handling of the MCR made with commit b4a29b94804c 
("serial: 8250: Move Alpha-specific quirk out of the core").

 As noted in the course of v2 review I don't believe the Linux kernel has 
a policy for any of its subsystems to require rewriting parts of existing 
code to fix bugs or internal API deficiencies as a prerequisite for bug 
fix (or even functional improvement) acceptance.  Therefore I consider 
this v3 of the series final and I am not going to continue pursuing this 
submission any further unless there is an actual technical defect (a bug, 
a coding style issue, etc.) within this series itself.

 Please apply.

  Maciej

Comments

Maciej W. Rozycki April 13, 2022, 10:53 p.m. UTC | #1 
On Thu, 31 Mar 2022, Maciej W. Rozycki wrote:

>  Here's v3 of the outstanding fixes for Oxford Semiconductor 950 UARTs.  
> As the change for the default FIFO rx trigger level has been already 
> merged with commit d7aff291d069 ("serial: 8250: Define RX trigger levels 
> for OxSemi 950 devices") only one patch of the original series remains.

 Ping for:
<https://lore.kernel.org/lkml/alpine.DEB.2.21.2203310114210.44113@angie.orcam.me.uk/>

  Maciej
Andy Shevchenko April 14, 2022, 12:45 p.m. UTC | #2 
On Thu, Apr 14, 2022 at 1:53 AM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
> On Thu, 31 Mar 2022, Maciej W. Rozycki wrote:
>
> >  Here's v3 of the outstanding fixes for Oxford Semiconductor 950 UARTs.
> > As the change for the default FIFO rx trigger level has been already
> > merged with commit d7aff291d069 ("serial: 8250: Define RX trigger levels
> > for OxSemi 950 devices") only one patch of the original series remains.
>
>  Ping for:
> <https://lore.kernel.org/lkml/alpine.DEB.2.21.2203310114210.44113@angie.orcam.me.uk/>

I still didn't get the answer why BOTHER can't be used instead of
spreading the old hack. You mentioned fractional baud rates and
something else, and I asked why do you need them and from where you
got the limitation of 16-bit values for dividers when using BOTHER.
Maciej W. Rozycki April 14, 2022, 1:47 p.m. UTC | #3 
On Thu, 14 Apr 2022, Andy Shevchenko wrote:

> > >  Here's v3 of the outstanding fixes for Oxford Semiconductor 950 UARTs.
> > > As the change for the default FIFO rx trigger level has been already
> > > merged with commit d7aff291d069 ("serial: 8250: Define RX trigger levels
> > > for OxSemi 950 devices") only one patch of the original series remains.
> >
> >  Ping for:
> > <https://lore.kernel.org/lkml/alpine.DEB.2.21.2203310114210.44113@angie.orcam.me.uk/>
> 
> I still didn't get the answer why BOTHER can't be used instead of
> spreading the old hack.

 I just fail to see any sense in repeating myself over and over.

> You mentioned fractional baud rates and
> something else, and I asked why do you need them and from where you
> got the limitation of 16-bit values for dividers when using BOTHER.

 Sigh, I have documented it there with the original submission 10 months 
ago and then repeated with every reiteration:

>  Finally the 16-bit UART_DIV_MAX limitation of the baud rate requested
> with `serial8250_get_baud_rate' makes the standard rates of 200bps and
> lower inaccessible in the regular way with the baud base of 15625000.
> That could be avoided by tweaking our 8250 driver core appropriately, but
> I have figured out with modern serial port usage that would not be the
> best use of my time.  Someone who does have a real need to use an Oxford
> device at these low rates can step in and make the necessary chances.

 To put it shortly: the `spd_cust' feature is out there and it works, and 
contrary to what you assert requires no maintenance effort if you just 
leave it alone, while the alternative has various shortcomings that do 
require effort if they were to be addressed.  So please just get over it 
and let users choose what suits them best while letting developers focus 
on other stuff that keeps waiting.  If someone is happy with what BOTHER 
offers, then by no means I keep them from using it.

 I fail to understand really why a piece of code to correct and improve 
broken UART baud rate calculation has to be stuck in limbo for almost a 
year.  There is nothing wrong with this code and it has a proper change 
description and my observation has been that actually broken code often 
with half a sentence serving as justification gets accepted with no fuss 
all the time. :(

  Maciej
Andy Shevchenko April 14, 2022, 1:55 p.m. UTC | #4 
On Thu, Apr 14, 2022 at 4:47 PM Maciej W. Rozycki <macro@orcam.me.uk> wrote:
> On Thu, 14 Apr 2022, Andy Shevchenko wrote:

>  I fail to understand really why a piece of code to correct and improve
> broken UART baud rate calculation has to be stuck in limbo for almost a
> year.  There is nothing wrong with this code and it has a proper change
> description and my observation has been that actually broken code often
> with half a sentence serving as justification gets accepted with no fuss
> all the time. :(

If you remove those 3 or so lines of the code (that are pushing old
SPD_CUST hack) I would be happy to Ack your patches immediately.
Otherwise it's up to maintainers, if they are fine on that. I think
it's a step back advertising something that should have not existed
from day 1.
Greg KH April 15, 2022, 9:13 a.m. UTC | #5 
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 <macro@orcam.me.uk>
> 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
Greg KH April 15, 2022, 9:13 a.m. UTC | #6 
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
Maciej W. Rozycki April 17, 2022, 11:02 p.m. UTC | #7 
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
Maciej W. Rozycki April 17, 2022, 11:03 p.m. UTC | #8 
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
Maciej W. Rozycki April 21, 2022, 9:38 p.m. UTC | #9 
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