Re: [PATCH v2 1/2] serial: 8250: Add proper clock handling for OxSemi PCIe devices

From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: "Maciej W. Rozycki" <macro@orcam.me.uk>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>,
	Andy Shevchenko <andy@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jiri Slaby <jirislaby@kernel.org>,
	"open list:SERIAL DRIVERS" <linux-serial@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 1/2] serial: 8250: Add proper clock handling for OxSemi PCIe devices
Date: Thu, 24 Mar 2022 13:28:53 +0200	[thread overview]
Message-ID: <YjxV9bsfdzdtgM6K@smile.fi.intel.com> (raw)
In-Reply-To: <alpine.DEB.2.21.2203232126220.52439@angie.orcam.me.uk>

On Wed, Mar 23, 2022 at 09:59:28PM +0000, Maciej W. Rozycki wrote:
> On Fri, 18 Mar 2022, Andy Shevchenko wrote:
> 
> > >  It does allow one to program the full clock divider range of the OxSemi 
> > > devices.  I find it appropriate according to my engineer's code of good 
> > > practices.  And it doesn't cause any burden for non-OxSemi code.
> > 
> > How BOTHER does prevent you doing the same?
> 
>  It does not allow you to set arbitrary serial port clock rates.  You can 
> only set integer baud rates,

Why do you need fractional baud rates? What is the practical use of that, please?

>	and then only those that do not exceed the [1;65535] clock divisor
>	range.

Can you be more specific as I can't see how it's possible in practice? In
several 8250 drivers we are able to set whatever we want to the limits of
the hardware.

> > >  So I have had a look at how it has been done for other drivers and I have 
> > > now convinced myself against such a split.  The primary reason for this 
> > > conclusion is that there is no basic infrastructure for such a split and 
> > > the ultimate result is code duplication with no clear benefit to justify 
> > > it.
> > 
> > Justification for split is to keep certain quirks out of the scope of the
> > generic driver. I'm not sure what duplication you are talking about if the
> > LOC statistics shows otherwise.
> 
>  All the init/remove code is almost the same across all the devices.

Each of the platform has its own constraints and what you see as a repetition
is just a similarity. If you have an idea of the common probe function, please
share.

Also, don't forget the memory footprint case at run time. In embedded world
we do not need 8250 code that is not supported by the platform in question.

The split allows to disable / remove the code that is not needed.

> And suspend/resume and PCI error handling code has been removed from the
> split off devices,

This is managed by PCI core. Any specifics, please?

>	and for the functional regression to be fixed:
> 
> 1. this code would have to be replicated, or
> 
> 2. handlers from the generic 8250_pci.c driver exported and referred to, 
>    or
> 
> 3. some kind of a helper library (or a core module) created providing this 
>    stuff to 8250_*.c drivers as required.

Which functional regression? You mean if it will be found then it needs to
be fixed in several places?

>  I guess the latter is the minimum that could convince me this driver
> framework is usable for implementing device-specific drivers (as I find 
> the other variants rather miserable hacks).
> 
>  Plus there would have to be clear information provided to the users as 
> otherwise people will be rather confused as to why 3 out of their 4 16x50 
> PCI/e serial cards work with 8250_pci.c while the remaining one does not 
> (probably broken, or is it?).

The default configuration after the split assumes that the driver is enabled
by the very same kernel configuration. Otherwise distributions will choose
what they consider better for their users and customers.

> > You may not want to get the idea, it's fine. The rationale is simple:
> > isolate quirks for certain platform(s) in one place. Each platform
> > in a separate module.
> 
>  What is a platform in your terminology?  A PCI/e option card you can 
> install in about any modern computer?  I usually think of platforms as 
> specific families of computers rather than option cards.  Variants of 
> otherwise the same device are usually handled with a single driver in 
> Linux.

It might be PCIe card, it might be soldered on the motherboard, it can
be part of the SoC.

By platform I assume the certain SoC + certain discrete components wired
in a certain way (PCB level). In your case it's a motherboard with PCIe
serial port card.

-- 
With Best Regards,
Andy Shevchenko