[PATCH 0/2] serial: 8250: Add support for 8250/16550 as MFD function

[PATCH 0/2] serial: 8250: Add support for 8250/16550 as MFD function

[Esben Haabendal]

This series adds a driver for adding 8250/16550 UART ports as functions to
MFD devices.

Esben Haabendal (2):
  serial: 8250: Allow port registration without UPF_BOOT_AUTOCONF
  serial: 8250: Add support for 8250/16550 as MFD function

 drivers/tty/serial/8250/8250_core.c |  36 ++++++-----
 drivers/tty/serial/8250/8250_mfd.c  | 119 ++++++++++++++++++++++++++++++++++++
 drivers/tty/serial/8250/Kconfig     |  12 ++++
 drivers/tty/serial/8250/Makefile    |   1 +
 include/linux/serial_8250.h         |   2 +
 5 files changed, 155 insertions(+), 15 deletions(-)
 create mode 100644 drivers/tty/serial/8250/8250_mfd.c

-- 
2.4.11

[PATCH 1/2] serial: 8250: Allow port registration without UPF_BOOT_AUTOCONF

[Esben Haabendal]

With serial8250_register_8250_port() forcing UPF_BOOT_AUTOCONF bit on, it
is not possible to register a port without having
serial8250_request_std_resource() called.

For adding a 8250 port to an MFD device, this is problematic, as the
request_mem_region() call will fail, as the MFD device (and rightly  so)
has requested the region.  For this case, the 8250 port should accept
having passed mapbase and membase, and just use that.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/tty/serial/8250/8250_core.c | 36 +++++++++++++++++++++---------------
 include/linux/serial_8250.h         |  2 ++
 2 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index e441221..ddbb0a0 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -957,20 +957,8 @@ static void serial_8250_overrun_backoff_work(struct work_struct *work)
 	spin_unlock_irqrestore(&port->lock, flags);
 }
 
-/**
- *	serial8250_register_8250_port - register a serial port
- *	@up: serial port template
- *
- *	Configure the serial port specified by the request. If the
- *	port exists and is in use, it is hung up and unregistered
- *	first.
- *
- *	The port is then probed and if necessary the IRQ is autodetected
- *	If this fails an error is returned.
- *
- *	On success the port is ready to use and the line number is returned.
- */
-int serial8250_register_8250_port(struct uart_8250_port *up)
+int __serial8250_register_8250_port(struct uart_8250_port *up,
+				    unsigned int extra_flags)
 {
 	struct uart_8250_port *uart;
 	int ret = -ENOSPC;
@@ -993,7 +981,7 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
 		uart->port.fifosize     = up->port.fifosize;
 		uart->port.regshift     = up->port.regshift;
 		uart->port.iotype       = up->port.iotype;
-		uart->port.flags        = up->port.flags | UPF_BOOT_AUTOCONF;
+		uart->port.flags        = up->port.flags | extra_flags;
 		uart->bugs		= up->bugs;
 		uart->port.mapbase      = up->port.mapbase;
 		uart->port.mapsize      = up->port.mapsize;
@@ -1086,6 +1074,24 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
 
 	return ret;
 }
+
+/**
+ *	serial8250_register_8250_port - register a serial port
+ *	@up: serial port template
+ *
+ *	Configure the serial port specified by the request. If the
+ *	port exists and is in use, it is hung up and unregistered
+ *	first.
+ *
+ *	The port is then probed and if necessary the IRQ is autodetected
+ *	If this fails an error is returned.
+ *
+ *	On success the port is ready to use and the line number is returned.
+ */
+int serial8250_register_8250_port(struct uart_8250_port *up)
+{
+	return __serial8250_register_8250_port(up, UPF_BOOT_AUTOCONF);
+}
 EXPORT_SYMBOL(serial8250_register_8250_port);
 
 /**
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index 5a655ba..9d1fe2e 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -145,6 +145,8 @@ static inline struct uart_8250_port *up_to_u8250p(struct uart_port *up)
 	return container_of(up, struct uart_8250_port, port);
 }
 
+extern int __serial8250_register_8250_port(struct uart_8250_port *,
+					   unsigned int);
 int serial8250_register_8250_port(struct uart_8250_port *);
 void serial8250_unregister_port(int line);
 void serial8250_suspend_port(int line);
-- 
2.4.11

Re: [PATCH 1/2] serial: 8250: Allow port registration without UPF_BOOT_AUTOCONF

[Andy Shevchenko]

On Fri, Apr 26, 2019 at 10:40:37AM +0200, Esben Haabendal wrote:
> With serial8250_register_8250_port() forcing UPF_BOOT_AUTOCONF bit on, it
> is not possible to register a port without having
> serial8250_request_std_resource() called.
> 
> For adding a 8250 port to an MFD device, this is problematic, as the
> request_mem_region() call will fail, as the MFD device (and rightly  so)
> has requested the region.  For this case, the 8250 port should accept
> having passed mapbase and membase, and just use that.

You need to simple set port type and use UPF_FIXED_TYPE.
No need for this patch.

> 
> Signed-off-by: Esben Haabendal <esben@geanix.com>
> ---
>  drivers/tty/serial/8250/8250_core.c | 36 +++++++++++++++++++++---------------
>  include/linux/serial_8250.h         |  2 ++
>  2 files changed, 23 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index e441221..ddbb0a0 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -957,20 +957,8 @@ static void serial_8250_overrun_backoff_work(struct work_struct *work)
>  	spin_unlock_irqrestore(&port->lock, flags);
>  }
>  
> -/**
> - *	serial8250_register_8250_port - register a serial port
> - *	@up: serial port template
> - *
> - *	Configure the serial port specified by the request. If the
> - *	port exists and is in use, it is hung up and unregistered
> - *	first.
> - *
> - *	The port is then probed and if necessary the IRQ is autodetected
> - *	If this fails an error is returned.
> - *
> - *	On success the port is ready to use and the line number is returned.
> - */
> -int serial8250_register_8250_port(struct uart_8250_port *up)
> +int __serial8250_register_8250_port(struct uart_8250_port *up,
> +				    unsigned int extra_flags)
>  {
>  	struct uart_8250_port *uart;
>  	int ret = -ENOSPC;
> @@ -993,7 +981,7 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
>  		uart->port.fifosize     = up->port.fifosize;
>  		uart->port.regshift     = up->port.regshift;
>  		uart->port.iotype       = up->port.iotype;
> -		uart->port.flags        = up->port.flags | UPF_BOOT_AUTOCONF;
> +		uart->port.flags        = up->port.flags | extra_flags;
>  		uart->bugs		= up->bugs;
>  		uart->port.mapbase      = up->port.mapbase;
>  		uart->port.mapsize      = up->port.mapsize;
> @@ -1086,6 +1074,24 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
>  
>  	return ret;
>  }
> +
> +/**
> + *	serial8250_register_8250_port - register a serial port
> + *	@up: serial port template
> + *
> + *	Configure the serial port specified by the request. If the
> + *	port exists and is in use, it is hung up and unregistered
> + *	first.
> + *
> + *	The port is then probed and if necessary the IRQ is autodetected
> + *	If this fails an error is returned.
> + *
> + *	On success the port is ready to use and the line number is returned.
> + */
> +int serial8250_register_8250_port(struct uart_8250_port *up)
> +{
> +	return __serial8250_register_8250_port(up, UPF_BOOT_AUTOCONF);
> +}
>  EXPORT_SYMBOL(serial8250_register_8250_port);
>  
>  /**
> diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
> index 5a655ba..9d1fe2e 100644
> --- a/include/linux/serial_8250.h
> +++ b/include/linux/serial_8250.h
> @@ -145,6 +145,8 @@ static inline struct uart_8250_port *up_to_u8250p(struct uart_port *up)
>  	return container_of(up, struct uart_8250_port, port);
>  }
>  
> +extern int __serial8250_register_8250_port(struct uart_8250_port *,
> +					   unsigned int);
>  int serial8250_register_8250_port(struct uart_8250_port *);
>  void serial8250_unregister_port(int line);
>  void serial8250_suspend_port(int line);
> -- 
> 2.4.11
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] serial: 8250: Allow port registration without UPF_BOOT_AUTOCONF

[Esben Haabendal]

Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

> On Fri, Apr 26, 2019 at 10:40:37AM +0200, Esben Haabendal wrote:
>> With serial8250_register_8250_port() forcing UPF_BOOT_AUTOCONF bit on, it
>> is not possible to register a port without having
>> serial8250_request_std_resource() called.
>> 
>> For adding a 8250 port to an MFD device, this is problematic, as the
>> request_mem_region() call will fail, as the MFD device (and rightly  so)
>> has requested the region.  For this case, the 8250 port should accept
>> having passed mapbase and membase, and just use that.
>
> You need to simple set port type and use UPF_FIXED_TYPE.
> No need for this patch.

The reason for this patch is to be able to do exactly that (set port
type and UPF_FIXED_TYPE) without having UPF_BOOT_AUTOCONF added.

In the current serial8250_register_8250_port() there is:

    uart->port.flags        = up->port.flags | UPF_BOOT_AUTOCONF;

So, even though I set UPF_FIXED_TYPE, I get
UPF_FIXED_TYPE|UPF_BOOT_AUTOCONF.  So I need this patch.

I think it is unfortunate that UPF_BOOT_AUTOCONF is or'ed to flags like
that, but changing that will surely break stuff.

/Esben

>
>> 
>> Signed-off-by: Esben Haabendal <esben@geanix.com>
>> ---
>>  drivers/tty/serial/8250/8250_core.c | 36 +++++++++++++++++++++---------------
>>  include/linux/serial_8250.h         |  2 ++
>>  2 files changed, 23 insertions(+), 15 deletions(-)
>> 
>> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
>> index e441221..ddbb0a0 100644
>> --- a/drivers/tty/serial/8250/8250_core.c
>> +++ b/drivers/tty/serial/8250/8250_core.c
>> @@ -957,20 +957,8 @@ static void serial_8250_overrun_backoff_work(struct work_struct *work)
>>  	spin_unlock_irqrestore(&port->lock, flags);
>>  }
>>  
>> -/**
>> - *	serial8250_register_8250_port - register a serial port
>> - *	@up: serial port template
>> - *
>> - *	Configure the serial port specified by the request. If the
>> - *	port exists and is in use, it is hung up and unregistered
>> - *	first.
>> - *
>> - *	The port is then probed and if necessary the IRQ is autodetected
>> - *	If this fails an error is returned.
>> - *
>> - *	On success the port is ready to use and the line number is returned.
>> - */
>> -int serial8250_register_8250_port(struct uart_8250_port *up)
>> +int __serial8250_register_8250_port(struct uart_8250_port *up,
>> +				    unsigned int extra_flags)
>>  {
>>  	struct uart_8250_port *uart;
>>  	int ret = -ENOSPC;
>> @@ -993,7 +981,7 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
>>  		uart->port.fifosize     = up->port.fifosize;
>>  		uart->port.regshift     = up->port.regshift;
>>  		uart->port.iotype       = up->port.iotype;
>> -		uart->port.flags        = up->port.flags | UPF_BOOT_AUTOCONF;
>> +		uart->port.flags        = up->port.flags | extra_flags;
>>  		uart->bugs		= up->bugs;
>>  		uart->port.mapbase      = up->port.mapbase;
>>  		uart->port.mapsize      = up->port.mapsize;
>> @@ -1086,6 +1074,24 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
>>  
>>  	return ret;
>>  }
>> +
>> +/**
>> + *	serial8250_register_8250_port - register a serial port
>> + *	@up: serial port template
>> + *
>> + *	Configure the serial port specified by the request. If the
>> + *	port exists and is in use, it is hung up and unregistered
>> + *	first.
>> + *
>> + *	The port is then probed and if necessary the IRQ is autodetected
>> + *	If this fails an error is returned.
>> + *
>> + *	On success the port is ready to use and the line number is returned.
>> + */
>> +int serial8250_register_8250_port(struct uart_8250_port *up)
>> +{
>> +	return __serial8250_register_8250_port(up, UPF_BOOT_AUTOCONF);
>> +}
>>  EXPORT_SYMBOL(serial8250_register_8250_port);
>>  
>>  /**
>> diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
>> index 5a655ba..9d1fe2e 100644
>> --- a/include/linux/serial_8250.h
>> +++ b/include/linux/serial_8250.h
>> @@ -145,6 +145,8 @@ static inline struct uart_8250_port *up_to_u8250p(struct uart_port *up)
>>  	return container_of(up, struct uart_8250_port, port);
>>  }
>>  
>> +extern int __serial8250_register_8250_port(struct uart_8250_port *,
>> +					   unsigned int);
>>  int serial8250_register_8250_port(struct uart_8250_port *);
>>  void serial8250_unregister_port(int line);
>>  void serial8250_suspend_port(int line);
>> -- 
>> 2.4.11
>> 

Re: [PATCH 1/2] serial: 8250: Allow port registration without UPF_BOOT_AUTOCONF

[Andy Shevchenko]

On Fri, Apr 26, 2019 at 06:54:05PM +0200, Esben Haabendal wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> > On Fri, Apr 26, 2019 at 10:40:37AM +0200, Esben Haabendal wrote:
> >> With serial8250_register_8250_port() forcing UPF_BOOT_AUTOCONF bit on, it
> >> is not possible to register a port without having
> >> serial8250_request_std_resource() called.
> >> 
> >> For adding a 8250 port to an MFD device, this is problematic, as the
> >> request_mem_region() call will fail, as the MFD device (and rightly  so)
> >> has requested the region.  For this case, the 8250 port should accept
> >> having passed mapbase and membase, and just use that.
> >
> > You need to simple set port type and use UPF_FIXED_TYPE.
> > No need for this patch.
> 
> The reason for this patch is to be able to do exactly that (set port
> type and UPF_FIXED_TYPE) without having UPF_BOOT_AUTOCONF added.
> 
> In the current serial8250_register_8250_port() there is:
> 
>     uart->port.flags        = up->port.flags | UPF_BOOT_AUTOCONF;
> 
> So, even though I set UPF_FIXED_TYPE, I get
> UPF_FIXED_TYPE|UPF_BOOT_AUTOCONF. 

Yes.

> So I need this patch.

Why? I don't see any problems to have these flags set. Moreover, some drivers
are used as MFD counterparts with exactly same bits set.

> I think it is unfortunate that UPF_BOOT_AUTOCONF is or'ed to flags like
> that, but changing that will surely break stuff.

True.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] serial: 8250: Allow port registration without UPF_BOOT_AUTOCONF

[Esben Haabendal]

Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

> On Fri, Apr 26, 2019 at 06:54:05PM +0200, Esben Haabendal wrote:
>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>> > On Fri, Apr 26, 2019 at 10:40:37AM +0200, Esben Haabendal wrote:
>> >> With serial8250_register_8250_port() forcing UPF_BOOT_AUTOCONF bit on, it
>> >> is not possible to register a port without having
>> >> serial8250_request_std_resource() called.
>> >> 
>> >> For adding a 8250 port to an MFD device, this is problematic, as the
>> >> request_mem_region() call will fail, as the MFD device (and rightly  so)
>> >> has requested the region.  For this case, the 8250 port should accept
>> >> having passed mapbase and membase, and just use that.
>> >
>> > You need to simple set port type and use UPF_FIXED_TYPE.
>> > No need for this patch.
>> 
>> The reason for this patch is to be able to do exactly that (set port
>> type and UPF_FIXED_TYPE) without having UPF_BOOT_AUTOCONF added.
>> 
>> In the current serial8250_register_8250_port() there is:
>> 
>>     uart->port.flags        = up->port.flags | UPF_BOOT_AUTOCONF;
>> 
>> So, even though I set UPF_FIXED_TYPE, I get
>> UPF_FIXED_TYPE|UPF_BOOT_AUTOCONF. 
>
> Yes.
>
>> So I need this patch.
>
> Why? I don't see any problems to have these flags set.

The problem with having UPF_BOOT_AUTOCONF is the call to
serial8250_request_std_resource().  It calls request_mem_region(), which
fails if the MFD driver already have requested the memory region for the
MFD device.  And I believe that is a valid thing to do.

To workaround this, I first thought I could just avoid setting
port->mapbase, causing serial8250_request_std_resource() to be a no-op.
But this breaks with more than one UART port, as uart_match_port() will
match the same line for all such UART ports, causing all but the last
one to be removed.

That said, the purpose of UPF_BOOT_AUTOCONF (for 8250 driver) is to
request and map the register memory.  So when that is already done by
the parent MFD driver, I think it is silly to workaround problems caused
by UPF_BOOT_AUTOCONF being force setted, when it really shouldn't.

> Moreover, some drivers
> are used as MFD counterparts with exactly same bits set.
>
>> I think it is unfortunate that UPF_BOOT_AUTOCONF is or'ed to flags like
>> that, but changing that will surely break stuff.
>
> True.

Re: [PATCH 1/2] serial: 8250: Allow port registration without UPF_BOOT_AUTOCONF

[Enrico Weigelt, metux IT consult]

On 27.04.19 10:58, Esben Haabendal wrote:

Hi folks,

> That said, the purpose of UPF_BOOT_AUTOCONF (for 8250 driver) is to> request and map the register memory.  So when that is already done by>
the parent MFD driver, I think it is silly to workaround problems
caused> by UPF_BOOT_AUTOCONF being force setted, when it really shouldn't.
I tend to agree. Maybe we should give serial8250_register_8250_port()
some flags for controlling this, or add another function for those
cases.

A minimal-invasive approach could be introducing an
serial8250_register_8250_port_ext() with extra parameters, and let
serial8250_register_8250_port() just call it.


By the way: the mem-mapping code pathes in serial8250 are a bit strange
anyways, eg. serial8250_port_size() is always called when map/unmap is
done, and this function does special logic for certain devices.
IMHO, it this should be called only once on device init and later just
use the already computed value:

https://github.com/metux/linux/commit/7ec63b699c910228b92cfb27eb8edfda90fdfd63

(haven't sent this queue yet, but feel free to cherry-pick if you like)


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [PATCH 1/2] serial: 8250: Allow port registration without UPF_BOOT_AUTOCONF

[Esben Haabendal]

"Enrico Weigelt, metux IT consult" <lkml@metux.net> writes:

> On 27.04.19 10:58, Esben Haabendal wrote:
>
> Hi folks,
>
>> That said, the purpose of UPF_BOOT_AUTOCONF (for 8250 driver) is to
>> request and map the register memory.  So when that is already done by
>> the parent MFD driver, I think it is silly to workaround problems
>> caused by UPF_BOOT_AUTOCONF being force setted, when it really
>> shouldn't.
> I tend to agree. Maybe we should give serial8250_register_8250_port()
> some flags for controlling this, or add another function for those
> cases.

Changing serial8250_register_8250_port() would break existing drivers,
as I have seen that some explicitly rely on the automtic addition of
UPF_BOOT_AUTOCONF.

> A minimal-invasive approach could be introducing an
> serial8250_register_8250_port_ext() with extra parameters, and let
> serial8250_register_8250_port() just call it.

So basically a rename of __serial8250_register_8250_port() in my patch
to serial8250_register_8250_port_ext()?  Fine with me.  Should we give
it an EXPORT_SYMBOL() also, as it is just as valid to use in modules as
the current serial8250_register_8250_port()?

/Esben

Re: [PATCH 1/2] serial: 8250: Allow port registration without UPF_BOOT_AUTOCONF

[Andy Shevchenko]

On Sat, Apr 27, 2019 at 12:01 PM Esben Haabendal <esben@haabendal.dk> wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> > On Fri, Apr 26, 2019 at 06:54:05PM +0200, Esben Haabendal wrote:
> >> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> >> The reason for this patch is to be able to do exactly that (set port
> >> type and UPF_FIXED_TYPE) without having UPF_BOOT_AUTOCONF added.
> >>
> >> In the current serial8250_register_8250_port() there is:
> >>
> >>     uart->port.flags        = up->port.flags | UPF_BOOT_AUTOCONF;
> >>
> >> So, even though I set UPF_FIXED_TYPE, I get
> >> UPF_FIXED_TYPE|UPF_BOOT_AUTOCONF.
> >
> > Yes.
> >
> >> So I need this patch.
> >
> > Why? I don't see any problems to have these flags set.
>
> The problem with having UPF_BOOT_AUTOCONF is the call to
> serial8250_request_std_resource().  It calls request_mem_region(), which
> fails if the MFD driver already have requested the memory region for the
> MFD device.

If it's MFD, why it requested the region for its child?
Isn't it a bug in MFD driver?

>  And I believe that is a valid thing to do.
>
> To workaround this, I first thought I could just avoid setting
> port->mapbase, causing serial8250_request_std_resource() to be a no-op.
> But this breaks with more than one UART port, as uart_match_port() will
> match the same line for all such UART ports, causing all but the last
> one to be removed.
>
> That said, the purpose of UPF_BOOT_AUTOCONF (for 8250 driver) is to
> request and map the register memory.  So when that is already done by
> the parent MFD driver, I think it is silly to workaround problems caused
> by UPF_BOOT_AUTOCONF being force setted, when it really shouldn't.


--
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] serial: 8250: Allow port registration without UPF_BOOT_AUTOCONF

[Esben Haabendal]

Andy Shevchenko <andy.shevchenko@gmail.com> writes:

> On Sat, Apr 27, 2019 at 12:01 PM Esben Haabendal <esben@haabendal.dk> wrote:
>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>> > On Fri, Apr 26, 2019 at 06:54:05PM +0200, Esben Haabendal wrote:
>> >> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>> >> The reason for this patch is to be able to do exactly that (set port
>> >> type and UPF_FIXED_TYPE) without having UPF_BOOT_AUTOCONF added.
>> >>
>> >> In the current serial8250_register_8250_port() there is:
>> >>
>> >>     uart->port.flags        = up->port.flags | UPF_BOOT_AUTOCONF;
>> >>
>> >> So, even though I set UPF_FIXED_TYPE, I get
>> >> UPF_FIXED_TYPE|UPF_BOOT_AUTOCONF.
>> >
>> > Yes.
>> >
>> >> So I need this patch.
>> >
>> > Why? I don't see any problems to have these flags set.
>>
>> The problem with having UPF_BOOT_AUTOCONF is the call to
>> serial8250_request_std_resource().  It calls request_mem_region(), which
>> fails if the MFD driver already have requested the memory region for the
>> MFD device.
>
> If it's MFD, why it requested the region for its child?
> Isn't it a bug in MFD driver?

It is a PCI driver, which calls pci_request_regions().  The PCI device
carries a lot of different functions, which uses small slices of the PCI
memory region(s).  With the resources being a tree structure, I don't
think it is a bug when a parent driver requests the entire memory
region.

It would be nice if child drivers requesting memory would pass the
parent memory resource.  Maybe 8250 driver could be changed to accept a
struct resource pointer instead of a simple mapbase value, allowing to
setup the resource with parent pointing to the MFD memory resource.

/Esben

Re: [PATCH 1/2] serial: 8250: Allow port registration without UPF_BOOT_AUTOCONF

[Andy Shevchenko]

On Mon, Apr 29, 2019 at 9:27 AM Esben Haabendal <esben@haabendal.dk> wrote:
> Andy Shevchenko <andy.shevchenko@gmail.com> writes:
> > On Sat, Apr 27, 2019 at 12:01 PM Esben Haabendal <esben@haabendal.dk> wrote:
> >> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> >> > On Fri, Apr 26, 2019 at 06:54:05PM +0200, Esben Haabendal wrote:
> >> >> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> >> >> The reason for this patch is to be able to do exactly that (set port
> >> >> type and UPF_FIXED_TYPE) without having UPF_BOOT_AUTOCONF added.
> >> >>
> >> >> In the current serial8250_register_8250_port() there is:
> >> >>
> >> >>     uart->port.flags        = up->port.flags | UPF_BOOT_AUTOCONF;
> >> >>
> >> >> So, even though I set UPF_FIXED_TYPE, I get
> >> >> UPF_FIXED_TYPE|UPF_BOOT_AUTOCONF.
> >> >
> >> > Yes.
> >> >
> >> >> So I need this patch.
> >> >
> >> > Why? I don't see any problems to have these flags set.
> >>
> >> The problem with having UPF_BOOT_AUTOCONF is the call to
> >> serial8250_request_std_resource().  It calls request_mem_region(), which
> >> fails if the MFD driver already have requested the memory region for the
> >> MFD device.
> >
> > If it's MFD, why it requested the region for its child?
> > Isn't it a bug in MFD driver?
>
> It is a PCI driver, which calls pci_request_regions().  The PCI device
> carries a lot of different functions, which uses small slices of the PCI
> memory region(s).  With the resources being a tree structure, I don't
> think it is a bug when a parent driver requests the entire memory
> region.

If it's MFD driver, it's not its business to do something
child-related on child behalf.
In any case, Linux device resource model uses exclusive region
slicing. If you do like above, you call for a problems.

Btw, we have PCI MFD driver which enumerates 8250 (more precisely
8250_dw) w/o any issues.

> It would be nice if child drivers requesting memory would pass the
> parent memory resource.  Maybe 8250 driver could be changed to accept a
> struct resource pointer instead of a simple mapbase value, allowing to
> setup the resource with parent pointing to the MFD memory resource.

I don't see the problem in certain driver, I guess you are trying to
workaround existin Linux device resource model.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] serial: 8250: Allow port registration without UPF_BOOT_AUTOCONF

[Esben Haabendal]

Andy Shevchenko <andy.shevchenko@gmail.com> writes:

> On Mon, Apr 29, 2019 at 9:27 AM Esben Haabendal <esben@haabendal.dk> wrote:
>> Andy Shevchenko <andy.shevchenko@gmail.com> writes:
>> > On Sat, Apr 27, 2019 at 12:01 PM Esben Haabendal <esben@haabendal.dk> wrote:
>> >> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>> >> > On Fri, Apr 26, 2019 at 06:54:05PM +0200, Esben Haabendal wrote:
>> >> >> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>> >> >> The reason for this patch is to be able to do exactly that (set port
>> >> >> type and UPF_FIXED_TYPE) without having UPF_BOOT_AUTOCONF added.
>> >> >>
>> >> >> In the current serial8250_register_8250_port() there is:
>> >> >>
>> >> >>     uart->port.flags        = up->port.flags | UPF_BOOT_AUTOCONF;
>> >> >>
>> >> >> So, even though I set UPF_FIXED_TYPE, I get
>> >> >> UPF_FIXED_TYPE|UPF_BOOT_AUTOCONF.
>> >> >
>> >> > Yes.
>> >> >
>> >> >> So I need this patch.
>> >> >
>> >> > Why? I don't see any problems to have these flags set.
>> >>
>> >> The problem with having UPF_BOOT_AUTOCONF is the call to
>> >> serial8250_request_std_resource().  It calls request_mem_region(), which
>> >> fails if the MFD driver already have requested the memory region for the
>> >> MFD device.
>> >
>> > If it's MFD, why it requested the region for its child?
>> > Isn't it a bug in MFD driver?
>>
>> It is a PCI driver, which calls pci_request_regions().  The PCI device
>> carries a lot of different functions, which uses small slices of the PCI
>> memory region(s).  With the resources being a tree structure, I don't
>> think it is a bug when a parent driver requests the entire memory
>> region.
>
> If it's MFD driver, it's not its business to do something
> child-related on child behalf.

The MFD driver is not doing anything on behalf of the child.  Being the
parent (MFD), it is requesting the memory region of the MFD/parent
device, using pci_request_regions(), similar to how it is done in
fx. janz-cmodio.c.

> In any case, Linux device resource model uses exclusive region
> slicing. If you do like above, you call for a problems.

Linux device resource model supports a parent/child/sibling
structure, specifically to allow for doing something like this.
Requesting memory resources are not restricted to use the iomem_resource
root.

For example, drivers/pcmcia/soc_common.c:soc_pcmcia_add_one()

	ret = request_resource(&iomem_resource, &skt->res_skt);
	if (ret)
		goto out_err_1;

	ret = request_resource(&skt->res_skt, &skt->res_io);
	if (ret)
		goto out_err_2;

As an example of a parent/child memory resource request.

The problem is that the 8250 driver only allows for requesting memory
region from the root (iomem_resource), as it uses a single
resource_size_t mapbase value for specifying the memory resource instead
of a full struct resource, which would allow passing in a parent.

So maybe we should go down that direction intead, extending 8250 driver
to replace mapbase with a resource struct instead?

> Btw, we have PCI MFD driver which enumerates 8250 (more precisely
> 8250_dw) w/o any issues.

I am aware of that (sm501.c).  It avoids the problem by not requesting
the parent memory region (sm->io_res), and requesting all child memory
regions directly in root instead of relative to the sm->io_res parent.

But as resoure management is designed for managing a parent/child
resource tree, this looks much more like a workaround than the right
solution.

>> It would be nice if child drivers requesting memory would pass the
>> parent memory resource.  Maybe 8250 driver could be changed to accept a
>> struct resource pointer instead of a simple mapbase value, allowing to
>> setup the resource with parent pointing to the MFD memory resource.
>
> I don't see the problem in certain driver, I guess you are trying to
> workaround existin Linux device resource model.

No, I actually try to do the right thing in relation to Linux device
resource model.  But 8250 is just not behaving very well in that
respect, not having been made really aware of the resource model.

And sm501.c MFD driver ignores the way resource model is designed, and
just make things work.

/Esben

Re: [PATCH 1/2] serial: 8250: Allow port registration without UPF_BOOT_AUTOCONF

[Enrico Weigelt, metux IT consult]

On 29.04.19 11:29, Esben Haabendal wrote:

> So maybe we should go down that direction intead, extending 8250 driver
> to replace mapbase with a resource struct instead?

Yeah, I already was up to do that. But that would be a pretty massive
change, as the actual use of fields like mapsize/mapbase in the drivers
is so diverse. Some even don't use mapsize.

That's why I started harmonizing that step by step. In little steps,
that are easier to review (unfortunately, don't have the corresponding
hardware to do real tests).

Finally, I'd like to have all drivers just use appropriate helpers to
declare their IO area and let the core do all the mapping stuff. Then we
IMHO could easily switch to struct resource internally.

> No, I actually try to do the right thing in relation to Linux device
> resource model.  But 8250 is just not behaving very well in that
> respect, not having been made really aware of the resource model.

I see it the same way. The serial (especially 8250) drivers are a tricky
thing, and I'd like to make it simpler / easier to understand.

But I can understand that Greg is pretty reluctant to any changes, as
some actual hardware seems to be hard to get. (OTOH, if it's so rare,
could we risk some potential breaks and just wait for anybody
complaining ?)


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [PATCH 1/2] serial: 8250: Allow port registration without UPF_BOOT_AUTOCONF

[Andy Shevchenko]

On Mon, Apr 29, 2019 at 11:29:05AM +0200, Esben Haabendal wrote:
> Andy Shevchenko <andy.shevchenko@gmail.com> writes:
> > On Mon, Apr 29, 2019 at 9:27 AM Esben Haabendal <esben@haabendal.dk> wrote:
> >> Andy Shevchenko <andy.shevchenko@gmail.com> writes:
> >> > On Sat, Apr 27, 2019 at 12:01 PM Esben Haabendal <esben@haabendal.dk> wrote:
> >> >> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> >> >> > On Fri, Apr 26, 2019 at 06:54:05PM +0200, Esben Haabendal wrote:
> >> >> >> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

> So maybe we should go down that direction intead, extending 8250 driver
> to replace mapbase with a resource struct instead?
> 
> > Btw, we have PCI MFD driver which enumerates 8250 (more precisely
> > 8250_dw) w/o any issues.
> 
> I am aware of that (sm501.c).  It avoids the problem by not requesting
> the parent memory region (sm->io_res), and requesting all child memory
> regions directly in root instead of relative to the sm->io_res parent.
> 
> But as resoure management is designed for managing a parent/child
> resource tree, this looks much more like a workaround than the right
> solution.
> 
> >> It would be nice if child drivers requesting memory would pass the
> >> parent memory resource.  Maybe 8250 driver could be changed to accept a
> >> struct resource pointer instead of a simple mapbase value, allowing to
> >> setup the resource with parent pointing to the MFD memory resource.
> >
> > I don't see the problem in certain driver, I guess you are trying to
> > workaround existin Linux device resource model.
> 
> No, I actually try to do the right thing in relation to Linux device
> resource model.  But 8250 is just not behaving very well in that
> respect, not having been made really aware of the resource model.

The point here is that. MFD driver can re-use existing platform drivers which
may be used standalone. They and only they are the right owners of the
requesting *their* resources.

When parent request resources on the behalf of its child it simple will break
this flexibility.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] serial: 8250: Allow port registration without UPF_BOOT_AUTOCONF

[Esben Haabendal]

Andy Shevchenko <andy.shevchenko@gmail.com> writes:

> On Mon, Apr 29, 2019 at 11:29:05AM +0200, Esben Haabendal wrote:
>> Andy Shevchenko <andy.shevchenko@gmail.com> writes:
>> > On Mon, Apr 29, 2019 at 9:27 AM Esben Haabendal <esben@haabendal.dk> wrote:
>> >> Andy Shevchenko <andy.shevchenko@gmail.com> writes:
>> >> > On Sat, Apr 27, 2019 at 12:01 PM Esben Haabendal <esben@haabendal.dk>
>> >> > wrote:
>> >> >> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>> >> >> > On Fri, Apr 26, 2019 at 06:54:05PM +0200, Esben Haabendal wrote:
>> >> >> >> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>
>> So maybe we should go down that direction intead, extending 8250 driver
>> to replace mapbase with a resource struct instead?
>> 
>> > Btw, we have PCI MFD driver which enumerates 8250 (more precisely
>> > 8250_dw) w/o any issues.
>> 
>> I am aware of that (sm501.c).  It avoids the problem by not requesting
>> the parent memory region (sm->io_res), and requesting all child memory
>> regions directly in root instead of relative to the sm->io_res parent.
>> 
>> But as resoure management is designed for managing a parent/child
>> resource tree, this looks much more like a workaround than the right
>> solution.
>> 
>> >> It would be nice if child drivers requesting memory would pass the
>> >> parent memory resource.  Maybe 8250 driver could be changed to accept a
>> >> struct resource pointer instead of a simple mapbase value, allowing to
>> >> setup the resource with parent pointing to the MFD memory resource.
>> >
>> > I don't see the problem in certain driver, I guess you are trying to
>> > workaround existin Linux device resource model.
>> 
>> No, I actually try to do the right thing in relation to Linux device
>> resource model.  But 8250 is just not behaving very well in that
>> respect, not having been made really aware of the resource model.
>
> The point here is that. MFD driver can re-use existing platform drivers which
> may be used standalone. They and only they are the right owners of the
> requesting *their* resources.
>
> When parent request resources on the behalf of its child it simple will break
> this flexibility.

I hear what you say.  And I agree, parent/mfd drivers should not request
resources on behalf of it's children.

But on the other side, something is broken by design in mfd, if it is
not possible for parent/mfd driver to request the resources for itself,
which naturally contains the resources for all it's mfd
functions/childs.

Please take a look at mfd_add_device() in drivers/mfd/mfd-core.c, and
the use of the cell and mem_base arguments in particular.

        for (r = 0; r < cell->num_resources; r++) {
                res[r].name = cell->resources[r].name;
                res[r].flags = cell->resources[r].flags;

                /* Find out base to use */
                if ((cell->resources[r].flags & IORESOURCE_MEM) && mem_base) {
                        res[r].parent = mem_base;
                        res[r].start = mem_base->start +
                                cell->resources[r].start;
                        res[r].end = mem_base->start +
                                cell->resources[r].end;

It really is a core part of mfd drivers that you request a memory
resource for the mfd driver, and then have one or more child memory
resources requsted with the parent memory resource as base.

Many mfd drivers handle resources like this, like: htc/pasic3.c,
sta2x11-mfd.c and intel-lpss.c.
Ofcourse, the sm501.c driver does not, as it does not use the mfd API at
all, and is thus not a good example of a mfd driver, and there is
therefore no good examples of using 8250 with an mfd driver.

/Esben

[PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Esben Haabendal]

The serial8250-mfd driver is for adding 8250/16550 UART ports as functions
to an MFD driver.

When calling mfd_add_device(), platform_data should be a pointer to a
struct plat_serial8250_port, with proper settings like .flags, .type,
.iotype, .regshift and .uartclk.  Memory (or ioport) and IRQ should be
passed as cell resources.

Do not include UPF_BOOT_AUTOCONF in platform_data.flags.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/tty/serial/8250/8250_mfd.c | 119 +++++++++++++++++++++++++++++++++++++
 drivers/tty/serial/8250/Kconfig    |  12 ++++
 drivers/tty/serial/8250/Makefile   |   1 +
 3 files changed, 132 insertions(+)
 create mode 100644 drivers/tty/serial/8250/8250_mfd.c

diff --git a/drivers/tty/serial/8250/8250_mfd.c b/drivers/tty/serial/8250/8250_mfd.c
new file mode 100644
index 0000000..eae1566
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_mfd.c
@@ -0,0 +1,119 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *  Serial Port driver for 8250/16550-type MFD sub devices
+ *
+ *  This mimics the serial8250_probe of 8250_core.c, while allowing
+ *  use without UPF_BOOT_AUTOCONF, which is problematic for MFD, as
+ *  the request_mem_region() will typically fail as the region is
+ *  already requested by the MFD device.
+ *
+ *  Memory and irq are passed as platform resources, which allows easy
+ *  use together with (devm_)mfd_add_devices().
+ *
+ *  Other parameters are passed as struct plat_serial8250_port in
+ *  device platform_data.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/serial_8250.h>
+
+struct serial8250_mfd_data {
+	int			line;
+};
+
+static int serial8250_mfd_probe(struct platform_device *pdev)
+{
+	struct plat_serial8250_port *pdata = dev_get_platdata(&pdev->dev);
+	struct serial8250_mfd_data *data;
+	struct uart_8250_port up;
+	struct resource *r;
+	void __iomem *membase;
+
+	if (!pdata)
+		return -ENODEV;
+
+	memset(&up, 0, sizeof(up));
+
+	switch (pdata->iotype) {
+	case UPIO_AU:
+	case UPIO_TSI:
+	case UPIO_MEM32:
+	case UPIO_MEM32BE:
+	case UPIO_MEM16:
+	case UPIO_MEM:
+		r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		if (!r)
+			return -ENODEV;
+		membase = devm_ioremap_nocache(&pdev->dev,
+					       r->start, resource_size(r));
+		if (!membase)
+			return -ENOMEM;
+		up.port.mapbase = r->start;
+		up.port.membase = membase;
+		break;
+	case UPIO_HUB6:
+	case UPIO_PORT:
+		r = platform_get_resource(pdev, IORESOURCE_IO, 0);
+		if (!r)
+			return -ENODEV;
+		up.port.iobase = r->start;
+		break;
+	}
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	up.port.irq = platform_get_irq(pdev, 0);
+	if (up.port.irq < 0)
+		up.port.irq = 0; /* no interrupt -> use polling */
+
+	/* Register with 8250_core.c */
+	up.port.irqflags = pdata->irqflags;
+	up.port.uartclk = pdata->uartclk;
+	up.port.regshift = pdata->regshift;
+	up.port.iotype = pdata->iotype;
+	up.port.flags = pdata->flags;
+	up.port.hub6 = pdata->hub6;
+	up.port.private_data = pdata->private_data;
+	up.port.type = pdata->type;
+	up.port.serial_in = pdata->serial_in;
+	up.port.serial_out = pdata->serial_out;
+	up.port.handle_irq = pdata->handle_irq;
+	up.port.handle_break = pdata->handle_break;
+	up.port.set_termios = pdata->set_termios;
+	up.port.set_ldisc = pdata->set_ldisc;
+	up.port.get_mctrl = pdata->get_mctrl;
+	up.port.pm = pdata->pm;
+	up.port.dev = &pdev->dev;
+	data->line = __serial8250_register_8250_port(&up, 0);
+	if (data->line < 0)
+		return data->line;
+
+	platform_set_drvdata(pdev, data);
+	return 0;
+}
+
+static int serial8250_mfd_remove(struct platform_device *pdev)
+{
+	struct serial8250_mfd_data *data = platform_get_drvdata(pdev);
+
+	serial8250_unregister_port(data->line);
+	return 0;
+}
+
+static struct platform_driver serial8250_mfd_driver = {
+	.probe = serial8250_mfd_probe,
+	.remove = serial8250_mfd_remove,
+	.driver = {
+		.name = "serial8250-mfd",
+	},
+};
+
+module_platform_driver(serial8250_mfd_driver);
+
+MODULE_AUTHOR("Esben Haabendal <esben@geanix.com>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Driver for 8250/16550-type MFD sub devices");
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 15c2c54..ef1572b 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -58,6 +58,18 @@ config SERIAL_8250_PNP
 	  This builds standard PNP serial support. You may be able to
 	  disable this feature if you only need legacy serial support.
 
+config SERIAL_8250_MFD
+	bool "8250/16550 MFD function support"
+	depends on SERIAL_8250 && MFD_CORE
+	default n
+	help
+	  This builds support for using 8250/16550-type UARTs as MFD
+	  functions.
+
+	  MFD drivers needing this should select it automatically.
+
+	  If unsure, say N.
+
 config SERIAL_8250_FINTEK
 	bool "Support for Fintek F81216A LPC to 4 UART RS485 API"
 	depends on SERIAL_8250
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index 18751bc..da8e139 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -6,6 +6,7 @@
 obj-$(CONFIG_SERIAL_8250)		+= 8250.o 8250_base.o
 8250-y					:= 8250_core.o
 8250-$(CONFIG_SERIAL_8250_PNP)		+= 8250_pnp.o
+8250-$(CONFIG_SERIAL_8250_MFD)		+= 8250_mfd.o
 8250_base-y				:= 8250_port.o
 8250_base-$(CONFIG_SERIAL_8250_DMA)	+= 8250_dma.o
 8250_base-$(CONFIG_SERIAL_8250_FINTEK)	+= 8250_fintek.o
-- 
2.4.11

Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Lee Jones]

On Fri, 26 Apr 2019, Esben Haabendal wrote:

> The serial8250-mfd driver is for adding 8250/16550 UART ports as functions
> to an MFD driver.
> 
> When calling mfd_add_device(), platform_data should be a pointer to a
> struct plat_serial8250_port, with proper settings like .flags, .type,
> .iotype, .regshift and .uartclk.  Memory (or ioport) and IRQ should be
> passed as cell resources.

What?  No, please!

If you *must* create a whole driver just to be able to use
platform_*() helpers (which I don't think you should), then please
call it something else.  This doesn't have anything to do with MFD.

> Do not include UPF_BOOT_AUTOCONF in platform_data.flags.
> 
> Signed-off-by: Esben Haabendal <esben@geanix.com>
> ---
>  drivers/tty/serial/8250/8250_mfd.c | 119 +++++++++++++++++++++++++++++++++++++
>  drivers/tty/serial/8250/Kconfig    |  12 ++++
>  drivers/tty/serial/8250/Makefile   |   1 +
>  3 files changed, 132 insertions(+)
>  create mode 100644 drivers/tty/serial/8250/8250_mfd.c
> 
> diff --git a/drivers/tty/serial/8250/8250_mfd.c b/drivers/tty/serial/8250/8250_mfd.c
> new file mode 100644
> index 0000000..eae1566
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_mfd.c
> @@ -0,0 +1,119 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + *  Serial Port driver for 8250/16550-type MFD sub devices
> + *
> + *  This mimics the serial8250_probe of 8250_core.c, while allowing
> + *  use without UPF_BOOT_AUTOCONF, which is problematic for MFD, as
> + *  the request_mem_region() will typically fail as the region is
> + *  already requested by the MFD device.
> + *
> + *  Memory and irq are passed as platform resources, which allows easy
> + *  use together with (devm_)mfd_add_devices().
> + *
> + *  Other parameters are passed as struct plat_serial8250_port in
> + *  device platform_data.
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/serial_8250.h>
> +
> +struct serial8250_mfd_data {
> +	int			line;
> +};
> +
> +static int serial8250_mfd_probe(struct platform_device *pdev)
> +{
> +	struct plat_serial8250_port *pdata = dev_get_platdata(&pdev->dev);
> +	struct serial8250_mfd_data *data;
> +	struct uart_8250_port up;
> +	struct resource *r;
> +	void __iomem *membase;
> +
> +	if (!pdata)
> +		return -ENODEV;
> +
> +	memset(&up, 0, sizeof(up));
> +
> +	switch (pdata->iotype) {
> +	case UPIO_AU:
> +	case UPIO_TSI:
> +	case UPIO_MEM32:
> +	case UPIO_MEM32BE:
> +	case UPIO_MEM16:
> +	case UPIO_MEM:
> +		r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +		if (!r)
> +			return -ENODEV;
> +		membase = devm_ioremap_nocache(&pdev->dev,
> +					       r->start, resource_size(r));
> +		if (!membase)
> +			return -ENOMEM;
> +		up.port.mapbase = r->start;
> +		up.port.membase = membase;
> +		break;
> +	case UPIO_HUB6:
> +	case UPIO_PORT:
> +		r = platform_get_resource(pdev, IORESOURCE_IO, 0);
> +		if (!r)
> +			return -ENODEV;
> +		up.port.iobase = r->start;
> +		break;
> +	}
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	up.port.irq = platform_get_irq(pdev, 0);
> +	if (up.port.irq < 0)
> +		up.port.irq = 0; /* no interrupt -> use polling */
> +
> +	/* Register with 8250_core.c */
> +	up.port.irqflags = pdata->irqflags;
> +	up.port.uartclk = pdata->uartclk;
> +	up.port.regshift = pdata->regshift;
> +	up.port.iotype = pdata->iotype;
> +	up.port.flags = pdata->flags;
> +	up.port.hub6 = pdata->hub6;
> +	up.port.private_data = pdata->private_data;
> +	up.port.type = pdata->type;
> +	up.port.serial_in = pdata->serial_in;
> +	up.port.serial_out = pdata->serial_out;
> +	up.port.handle_irq = pdata->handle_irq;
> +	up.port.handle_break = pdata->handle_break;
> +	up.port.set_termios = pdata->set_termios;
> +	up.port.set_ldisc = pdata->set_ldisc;
> +	up.port.get_mctrl = pdata->get_mctrl;
> +	up.port.pm = pdata->pm;
> +	up.port.dev = &pdev->dev;
> +	data->line = __serial8250_register_8250_port(&up, 0);
> +	if (data->line < 0)
> +		return data->line;
> +
> +	platform_set_drvdata(pdev, data);
> +	return 0;
> +}
> +
> +static int serial8250_mfd_remove(struct platform_device *pdev)
> +{
> +	struct serial8250_mfd_data *data = platform_get_drvdata(pdev);
> +
> +	serial8250_unregister_port(data->line);
> +	return 0;
> +}
> +
> +static struct platform_driver serial8250_mfd_driver = {
> +	.probe = serial8250_mfd_probe,
> +	.remove = serial8250_mfd_remove,
> +	.driver = {
> +		.name = "serial8250-mfd",
> +	},
> +};
> +
> +module_platform_driver(serial8250_mfd_driver);
> +
> +MODULE_AUTHOR("Esben Haabendal <esben@geanix.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Driver for 8250/16550-type MFD sub devices");
> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> index 15c2c54..ef1572b 100644
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -58,6 +58,18 @@ config SERIAL_8250_PNP
>  	  This builds standard PNP serial support. You may be able to
>  	  disable this feature if you only need legacy serial support.
>  
> +config SERIAL_8250_MFD
> +	bool "8250/16550 MFD function support"
> +	depends on SERIAL_8250 && MFD_CORE
> +	default n
> +	help
> +	  This builds support for using 8250/16550-type UARTs as MFD
> +	  functions.
> +
> +	  MFD drivers needing this should select it automatically.
> +
> +	  If unsure, say N.
> +
>  config SERIAL_8250_FINTEK
>  	bool "Support for Fintek F81216A LPC to 4 UART RS485 API"
>  	depends on SERIAL_8250
> diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
> index 18751bc..da8e139 100644
> --- a/drivers/tty/serial/8250/Makefile
> +++ b/drivers/tty/serial/8250/Makefile
> @@ -6,6 +6,7 @@
>  obj-$(CONFIG_SERIAL_8250)		+= 8250.o 8250_base.o
>  8250-y					:= 8250_core.o
>  8250-$(CONFIG_SERIAL_8250_PNP)		+= 8250_pnp.o
> +8250-$(CONFIG_SERIAL_8250_MFD)		+= 8250_mfd.o
>  8250_base-y				:= 8250_port.o
>  8250_base-$(CONFIG_SERIAL_8250_DMA)	+= 8250_dma.o
>  8250_base-$(CONFIG_SERIAL_8250_FINTEK)	+= 8250_fintek.o

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Esben Haabendal]

Lee Jones <lee.jones@linaro.org> writes:

> On Fri, 26 Apr 2019, Esben Haabendal wrote:
>
>> The serial8250-mfd driver is for adding 8250/16550 UART ports as functions
>> to an MFD driver.
>> 
>> When calling mfd_add_device(), platform_data should be a pointer to a
>> struct plat_serial8250_port, with proper settings like .flags, .type,
>> .iotype, .regshift and .uartclk.  Memory (or ioport) and IRQ should be
>> passed as cell resources.
>
> What?  No, please!
>
> If you *must* create a whole driver just to be able to use
> platform_*() helpers (which I don't think you should), then please
> call it something else.  This doesn't have anything to do with MFD.

True.

I really don't think it is a good idea to create a whole driver just to
be able to use platform_get_*() helpers.  And if I am forced to do this,
because I am unable to convince Andy to improve the standard serial8250
driver to support that, it should be called MFD.  The driver would be
generally usable for all usecases where platform_get_*() works.

I don't have any idea what to call such a driver.  It really would just
be a fork of the current serial8250 driver, just allowing use of
platform_get_*(), supporting exactly the same hardware.

I am still hoping that we can find a way to improve serial8250 to be
usable in these cases.

/Esben

Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Lee Jones]

On Tue, 07 May 2019, Esben Haabendal wrote:

> Lee Jones <lee.jones@linaro.org> writes:
> 
> > On Fri, 26 Apr 2019, Esben Haabendal wrote:
> >
> >> The serial8250-mfd driver is for adding 8250/16550 UART ports as functions
> >> to an MFD driver.
> >> 
> >> When calling mfd_add_device(), platform_data should be a pointer to a
> >> struct plat_serial8250_port, with proper settings like .flags, .type,
> >> .iotype, .regshift and .uartclk.  Memory (or ioport) and IRQ should be
> >> passed as cell resources.
> >
> > What?  No, please!
> >
> > If you *must* create a whole driver just to be able to use
> > platform_*() helpers (which I don't think you should), then please
> > call it something else.  This doesn't have anything to do with MFD.
> 
> True.
> 
> I really don't think it is a good idea to create a whole driver just to
> be able to use platform_get_*() helpers.  And if I am forced to do this,
> because I am unable to convince Andy to improve the standard serial8250
> driver to support that, it should be called MFD.  The driver would be

I assume you mean "shouldn't"?

> generally usable for all usecases where platform_get_*() works.
> 
> I don't have any idea what to call such a driver.  It really would just
> be a fork of the current serial8250 driver, just allowing use of
> platform_get_*(), supporting exactly the same hardware.
> 
> I am still hoping that we can find a way to improve serial8250 to be
> usable in these cases.

Me too.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Esben Haabendal]

Lee Jones <lee.jones@linaro.org> writes:

> On Tue, 07 May 2019, Esben Haabendal wrote:
>
>> Lee Jones <lee.jones@linaro.org> writes:
>> 
>> > On Fri, 26 Apr 2019, Esben Haabendal wrote:
>> >
>> >> The serial8250-mfd driver is for adding 8250/16550 UART ports as functions
>> >> to an MFD driver.
>> >> 
>> >> When calling mfd_add_device(), platform_data should be a pointer to a
>> >> struct plat_serial8250_port, with proper settings like .flags, .type,
>> >> .iotype, .regshift and .uartclk.  Memory (or ioport) and IRQ should be
>> >> passed as cell resources.
>> >
>> > What?  No, please!
>> >
>> > If you *must* create a whole driver just to be able to use
>> > platform_*() helpers (which I don't think you should), then please
>> > call it something else.  This doesn't have anything to do with MFD.
>> 
>> True.
>> 
>> I really don't think it is a good idea to create a whole driver just to
>> be able to use platform_get_*() helpers.  And if I am forced to do this,
>> because I am unable to convince Andy to improve the standard serial8250
>> driver to support that, it should be called MFD.  The driver would be
>
> I assume you mean "shouldn't"?

Of-course.

>> generally usable for all usecases where platform_get_*() works.
>> 
>> I don't have any idea what to call such a driver.  It really would just
>> be a fork of the current serial8250 driver, just allowing use of
>> platform_get_*(), supporting exactly the same hardware.
>> 
>> I am still hoping that we can find a way to improve serial8250 to be
>> usable in these cases.
>
> Me too.

Unfortunately, I don't seem to be able to convince Andy to accept
something like that.

I might have to do this out-of-tree :(

/Esben

Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Lee Jones]

On Tue, 14 May 2019, Esben Haabendal wrote:

> Lee Jones <lee.jones@linaro.org> writes:
> 
> > On Tue, 07 May 2019, Esben Haabendal wrote:
> >
> >> Lee Jones <lee.jones@linaro.org> writes:
> >> 
> >> > On Fri, 26 Apr 2019, Esben Haabendal wrote:
> >> >
> >> >> The serial8250-mfd driver is for adding 8250/16550 UART ports as functions
> >> >> to an MFD driver.
> >> >> 
> >> >> When calling mfd_add_device(), platform_data should be a pointer to a
> >> >> struct plat_serial8250_port, with proper settings like .flags, .type,
> >> >> .iotype, .regshift and .uartclk.  Memory (or ioport) and IRQ should be
> >> >> passed as cell resources.
> >> >
> >> > What?  No, please!
> >> >
> >> > If you *must* create a whole driver just to be able to use
> >> > platform_*() helpers (which I don't think you should), then please
> >> > call it something else.  This doesn't have anything to do with MFD.
> >> 
> >> True.
> >> 
> >> I really don't think it is a good idea to create a whole driver just to
> >> be able to use platform_get_*() helpers.  And if I am forced to do this,
> >> because I am unable to convince Andy to improve the standard serial8250
> >> driver to support that, it should be called MFD.  The driver would be
> >
> > I assume you mean "shouldn't"?
> 
> Of-course.
> 
> >> generally usable for all usecases where platform_get_*() works.
> >> 
> >> I don't have any idea what to call such a driver.  It really would just
> >> be a fork of the current serial8250 driver, just allowing use of
> >> platform_get_*(), supporting exactly the same hardware.
> >> 
> >> I am still hoping that we can find a way to improve serial8250 to be
> >> usable in these cases.
> >
> > Me too.
> 
> Unfortunately, I don't seem to be able to convince Andy to accept
> something like that.

Andy is not he Maintainer.

What are Greg and Jiri's opinions?

> I might have to do this out-of-tree :(

Well that would suck!

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Greg Kroah-Hartman]

On Tue, May 14, 2019 at 11:47:41AM +0100, Lee Jones wrote:
> On Tue, 14 May 2019, Esben Haabendal wrote:
> 
> > Lee Jones <lee.jones@linaro.org> writes:
> > 
> > > On Tue, 07 May 2019, Esben Haabendal wrote:
> > >
> > >> Lee Jones <lee.jones@linaro.org> writes:
> > >> 
> > >> > On Fri, 26 Apr 2019, Esben Haabendal wrote:
> > >> >
> > >> >> The serial8250-mfd driver is for adding 8250/16550 UART ports as functions
> > >> >> to an MFD driver.
> > >> >> 
> > >> >> When calling mfd_add_device(), platform_data should be a pointer to a
> > >> >> struct plat_serial8250_port, with proper settings like .flags, .type,
> > >> >> .iotype, .regshift and .uartclk.  Memory (or ioport) and IRQ should be
> > >> >> passed as cell resources.
> > >> >
> > >> > What?  No, please!
> > >> >
> > >> > If you *must* create a whole driver just to be able to use
> > >> > platform_*() helpers (which I don't think you should), then please
> > >> > call it something else.  This doesn't have anything to do with MFD.
> > >> 
> > >> True.
> > >> 
> > >> I really don't think it is a good idea to create a whole driver just to
> > >> be able to use platform_get_*() helpers.  And if I am forced to do this,
> > >> because I am unable to convince Andy to improve the standard serial8250
> > >> driver to support that, it should be called MFD.  The driver would be
> > >
> > > I assume you mean "shouldn't"?
> > 
> > Of-course.
> > 
> > >> generally usable for all usecases where platform_get_*() works.
> > >> 
> > >> I don't have any idea what to call such a driver.  It really would just
> > >> be a fork of the current serial8250 driver, just allowing use of
> > >> platform_get_*(), supporting exactly the same hardware.
> > >> 
> > >> I am still hoping that we can find a way to improve serial8250 to be
> > >> usable in these cases.
> > >
> > > Me too.
> > 
> > Unfortunately, I don't seem to be able to convince Andy to accept
> > something like that.
> 
> Andy is not he Maintainer.
> 
> What are Greg and Jiri's opinions?

I've been ignoring all of this at the moment because of the 5.2-rc merge
window.  I'll look at it after -rc1 is out.

thanks,

greg k-h

Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Esben Haabendal]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> On Tue, May 14, 2019 at 11:47:41AM +0100, Lee Jones wrote:
>> On Tue, 14 May 2019, Esben Haabendal wrote:
>> 
>> > Lee Jones <lee.jones@linaro.org> writes:
>> > 
>> > > On Tue, 07 May 2019, Esben Haabendal wrote:
>> > >
>> > >> Lee Jones <lee.jones@linaro.org> writes:
>> > >> 
>> > >> > On Fri, 26 Apr 2019, Esben Haabendal wrote:
>> > >> >
>> > >> >> The serial8250-mfd driver is for adding 8250/16550 UART ports as functions
>> > >> >> to an MFD driver.
>> > >> >> 
>> > >> >> When calling mfd_add_device(), platform_data should be a pointer to a
>> > >> >> struct plat_serial8250_port, with proper settings like .flags, .type,
>> > >> >> .iotype, .regshift and .uartclk.  Memory (or ioport) and IRQ should be
>> > >> >> passed as cell resources.
>> > >> >
>> > >> > What?  No, please!
>> > >> >
>> > >> > If you *must* create a whole driver just to be able to use
>> > >> > platform_*() helpers (which I don't think you should), then please
>> > >> > call it something else.  This doesn't have anything to do with MFD.
>> > >> 
>> > >> True.
>> > >> 
>> > >> I really don't think it is a good idea to create a whole driver just to
>> > >> be able to use platform_get_*() helpers.  And if I am forced to do this,
>> > >> because I am unable to convince Andy to improve the standard serial8250
>> > >> driver to support that, it should be called MFD.  The driver would be
>> > >
>> > > I assume you mean "shouldn't"?
>> > 
>> > Of-course.
>> > 
>> > >> generally usable for all usecases where platform_get_*() works.
>> > >> 
>> > >> I don't have any idea what to call such a driver.  It really would just
>> > >> be a fork of the current serial8250 driver, just allowing use of
>> > >> platform_get_*(), supporting exactly the same hardware.
>> > >> 
>> > >> I am still hoping that we can find a way to improve serial8250 to be
>> > >> usable in these cases.
>> > >
>> > > Me too.
>> > 
>> > Unfortunately, I don't seem to be able to convince Andy to accept
>> > something like that.
>> 
>> Andy is not he Maintainer.
>> 
>> What are Greg and Jiri's opinions?
>
> I've been ignoring all of this at the moment because of the 5.2-rc merge
> window.  I'll look at it after -rc1 is out.
>
> thanks,
> greg k-h

Great, thanks!

I will try ad hold back with this thread until you get back to it.

/Esben

Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Greg Kroah-Hartman]

On Tue, May 14, 2019 at 02:41:35PM +0200, Esben Haabendal wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> 
> > On Tue, May 14, 2019 at 11:47:41AM +0100, Lee Jones wrote:
> >> On Tue, 14 May 2019, Esben Haabendal wrote:
> >> 
> >> > Lee Jones <lee.jones@linaro.org> writes:
> >> > 
> >> > > On Tue, 07 May 2019, Esben Haabendal wrote:
> >> > >
> >> > >> Lee Jones <lee.jones@linaro.org> writes:
> >> > >> 
> >> > >> > On Fri, 26 Apr 2019, Esben Haabendal wrote:
> >> > >> >
> >> > >> >> The serial8250-mfd driver is for adding 8250/16550 UART ports as functions
> >> > >> >> to an MFD driver.
> >> > >> >> 
> >> > >> >> When calling mfd_add_device(), platform_data should be a pointer to a
> >> > >> >> struct plat_serial8250_port, with proper settings like .flags, .type,
> >> > >> >> .iotype, .regshift and .uartclk.  Memory (or ioport) and IRQ should be
> >> > >> >> passed as cell resources.
> >> > >> >
> >> > >> > What?  No, please!
> >> > >> >
> >> > >> > If you *must* create a whole driver just to be able to use
> >> > >> > platform_*() helpers (which I don't think you should), then please
> >> > >> > call it something else.  This doesn't have anything to do with MFD.
> >> > >> 
> >> > >> True.
> >> > >> 
> >> > >> I really don't think it is a good idea to create a whole driver just to
> >> > >> be able to use platform_get_*() helpers.  And if I am forced to do this,
> >> > >> because I am unable to convince Andy to improve the standard serial8250
> >> > >> driver to support that, it should be called MFD.  The driver would be
> >> > >
> >> > > I assume you mean "shouldn't"?
> >> > 
> >> > Of-course.
> >> > 
> >> > >> generally usable for all usecases where platform_get_*() works.
> >> > >> 
> >> > >> I don't have any idea what to call such a driver.  It really would just
> >> > >> be a fork of the current serial8250 driver, just allowing use of
> >> > >> platform_get_*(), supporting exactly the same hardware.
> >> > >> 
> >> > >> I am still hoping that we can find a way to improve serial8250 to be
> >> > >> usable in these cases.
> >> > >
> >> > > Me too.
> >> > 
> >> > Unfortunately, I don't seem to be able to convince Andy to accept
> >> > something like that.
> >> 
> >> Andy is not he Maintainer.
> >> 
> >> What are Greg and Jiri's opinions?
> >
> > I've been ignoring all of this at the moment because of the 5.2-rc merge
> > window.  I'll look at it after -rc1 is out.
> >
> > thanks,
> > greg k-h
> 
> Great, thanks!
> 
> I will try ad hold back with this thread until you get back to it.

Ok, I have no idea what is going on here, sorry.  This is a really long
and meandering thread, and I can't even find the original patches in my
queue.

So can you resend things and we can start over?  :)

But note, using a mfd for a uart seems VERY odd to me...

thanks,

greg k-h

Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Esben Haabendal]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

>> I will try ad hold back with this thread until you get back to it.
>
> Ok, I have no idea what is going on here, sorry.  This is a really long
> and meandering thread, and I can't even find the original patches in my
> queue.
>
> So can you resend things and we can start over?  :)

Will do.

> But note, using a mfd for a uart seems VERY odd to me...

Ok.  In my case, I have a pcie card with an fpga which includes 5 uart
ports, 3 ethernet interfaces and a number of custom IP blocks.
I believe that an mfd driver for that pcie card in that case.

/Esben

Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Greg Kroah-Hartman]

On Tue, May 21, 2019 at 01:11:08PM +0200, Esben Haabendal wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> 
> >> I will try ad hold back with this thread until you get back to it.
> >
> > Ok, I have no idea what is going on here, sorry.  This is a really long
> > and meandering thread, and I can't even find the original patches in my
> > queue.
> >
> > So can you resend things and we can start over?  :)
> 
> Will do.
> 
> > But note, using a mfd for a uart seems VERY odd to me...
> 
> Ok.  In my case, I have a pcie card with an fpga which includes 5 uart
> ports, 3 ethernet interfaces and a number of custom IP blocks.
> I believe that an mfd driver for that pcie card in that case.

I believe you need to fix that fpga to expose individual pci devices
such that you can properly bind the individual devices to the expected
drivers :)

Seriously, who makes such a broken fpga device that goes against the PCI
spec that way?  Well, not so much as "goes against it", as "ignores all
of the proper ideas of the past 20 years for working with PCI devices".

thanks,

greg k-h

Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Esben Haabendal]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> On Tue, May 21, 2019 at 01:11:08PM +0200, Esben Haabendal wrote:
>> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
>> 
>> >> I will try ad hold back with this thread until you get back to it.
>> >
>> > Ok, I have no idea what is going on here, sorry.  This is a really long
>> > and meandering thread, and I can't even find the original patches in my
>> > queue.
>> >
>> > So can you resend things and we can start over?  :)
>> 
>> Will do.
>> 
>> > But note, using a mfd for a uart seems VERY odd to me...
>> 
>> Ok.  In my case, I have a pcie card with an fpga which includes 5 uart
>> ports, 3 ethernet interfaces and a number of custom IP blocks.
>> I believe that an mfd driver for that pcie card in that case.
>
> I believe you need to fix that fpga to expose individual pci devices
> such that you can properly bind the individual devices to the expected
> drivers :)

Well, that is really out-of-scope of what I am doing here.

> Seriously, who makes such a broken fpga device that goes against the PCI
> spec that way?  Well, not so much as "goes against it", as "ignores all
> of the proper ideas of the past 20 years for working with PCI devices".

Might be.  But that is the firmware I have to work with here, and I
still hope we can find a good solution for implementing a driver without
having to maintain out-of-tree patches.

/Esben

Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Greg Kroah-Hartman]

On Tue, May 21, 2019 at 01:50:25PM +0200, Esben Haabendal wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> 
> > On Tue, May 21, 2019 at 01:11:08PM +0200, Esben Haabendal wrote:
> >> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> >> 
> >> >> I will try ad hold back with this thread until you get back to it.
> >> >
> >> > Ok, I have no idea what is going on here, sorry.  This is a really long
> >> > and meandering thread, and I can't even find the original patches in my
> >> > queue.
> >> >
> >> > So can you resend things and we can start over?  :)
> >> 
> >> Will do.
> >> 
> >> > But note, using a mfd for a uart seems VERY odd to me...
> >> 
> >> Ok.  In my case, I have a pcie card with an fpga which includes 5 uart
> >> ports, 3 ethernet interfaces and a number of custom IP blocks.
> >> I believe that an mfd driver for that pcie card in that case.
> >
> > I believe you need to fix that fpga to expose individual pci devices
> > such that you can properly bind the individual devices to the expected
> > drivers :)
> 
> Well, that is really out-of-scope of what I am doing here.

Not really, if you have control over the fpga firmware (and odds are you
do), just fix that and instantly your device works with all kernels, no
need to change anything.

Why not do this?

> > Seriously, who makes such a broken fpga device that goes against the PCI
> > spec that way?  Well, not so much as "goes against it", as "ignores all
> > of the proper ideas of the past 20 years for working with PCI devices".
> 
> Might be.  But that is the firmware I have to work with here, and I
> still hope we can find a good solution for implementing a driver without
> having to maintain out-of-tree patches.

As this hardware will not work on any operating system as-is, why not
fix the firmware to keep from having to support a one-off device that no
one else would be crazy enough to create?  :)

thanks,

greg k-h

Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Esben Haabendal]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> On Tue, May 21, 2019 at 01:50:25PM +0200, Esben Haabendal wrote:
>> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
>> 
>> > On Tue, May 21, 2019 at 01:11:08PM +0200, Esben Haabendal wrote:
>> >> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
>> >> 
>> >> >> I will try ad hold back with this thread until you get back to it.
>> >> >
>> >> > Ok, I have no idea what is going on here, sorry.  This is a really long
>> >> > and meandering thread, and I can't even find the original patches in my
>> >> > queue.
>> >> >
>> >> > So can you resend things and we can start over?  :)
>> >> 
>> >> Will do.
>> >> 
>> >> > But note, using a mfd for a uart seems VERY odd to me...
>> >> 
>> >> Ok.  In my case, I have a pcie card with an fpga which includes 5 uart
>> >> ports, 3 ethernet interfaces and a number of custom IP blocks.
>> >> I believe that an mfd driver for that pcie card in that case.
>> >
>> > I believe you need to fix that fpga to expose individual pci devices
>> > such that you can properly bind the individual devices to the expected
>> > drivers :)
>> 
>> Well, that is really out-of-scope of what I am doing here.
>
> Not really, if you have control over the fpga firmware (and odds are you
> do), just fix that and instantly your device works with all kernels, no
> need to change anything.
>
> Why not do this?

Because I do not have control over fpga firmware.

>> > Seriously, who makes such a broken fpga device that goes against the PCI
>> > spec that way?  Well, not so much as "goes against it", as "ignores all
>> > of the proper ideas of the past 20 years for working with PCI devices".
>> 
>> Might be.  But that is the firmware I have to work with here, and I
>> still hope we can find a good solution for implementing a driver without
>> having to maintain out-of-tree patches.
>
> As this hardware will not work on any operating system as-is, why not
> fix the firmware to keep from having to support a one-off device that no
> one else would be crazy enough to create?  :)

Clearly, someone has been crazy enough.  Hopefully, we can be smart
enough to make Linux fit to it.

/Esben

Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Greg Kroah-Hartman]

On Tue, May 21, 2019 at 04:31:52PM +0200, Esben Haabendal wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> 
> > On Tue, May 21, 2019 at 01:50:25PM +0200, Esben Haabendal wrote:
> >> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> >> 
> >> > On Tue, May 21, 2019 at 01:11:08PM +0200, Esben Haabendal wrote:
> >> >> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> >> >> 
> >> >> >> I will try ad hold back with this thread until you get back to it.
> >> >> >
> >> >> > Ok, I have no idea what is going on here, sorry.  This is a really long
> >> >> > and meandering thread, and I can't even find the original patches in my
> >> >> > queue.
> >> >> >
> >> >> > So can you resend things and we can start over?  :)
> >> >> 
> >> >> Will do.
> >> >> 
> >> >> > But note, using a mfd for a uart seems VERY odd to me...
> >> >> 
> >> >> Ok.  In my case, I have a pcie card with an fpga which includes 5 uart
> >> >> ports, 3 ethernet interfaces and a number of custom IP blocks.
> >> >> I believe that an mfd driver for that pcie card in that case.
> >> >
> >> > I believe you need to fix that fpga to expose individual pci devices
> >> > such that you can properly bind the individual devices to the expected
> >> > drivers :)
> >> 
> >> Well, that is really out-of-scope of what I am doing here.
> >
> > Not really, if you have control over the fpga firmware (and odds are you
> > do), just fix that and instantly your device works with all kernels, no
> > need to change anything.
> >
> > Why not do this?
> 
> Because I do not have control over fpga firmware.

Who does?  Why did they create it this way if it can not be accessed by
an operating system as-is?  Has it passed the PCI tests?  Do you have a
link to where you can get this crazy device?

> >> > Seriously, who makes such a broken fpga device that goes against the PCI
> >> > spec that way?  Well, not so much as "goes against it", as "ignores all
> >> > of the proper ideas of the past 20 years for working with PCI devices".
> >> 
> >> Might be.  But that is the firmware I have to work with here, and I
> >> still hope we can find a good solution for implementing a driver without
> >> having to maintain out-of-tree patches.
> >
> > As this hardware will not work on any operating system as-is, why not
> > fix the firmware to keep from having to support a one-off device that no
> > one else would be crazy enough to create?  :)
> 
> Clearly, someone has been crazy enough.  Hopefully, we can be smart
> enough to make Linux fit to it.

Sometimes you need to go tell the hardware/firmware people not to do
foolish things.  You can not always fix their problems in software.
Please push back on this.

thanks,

greg k-h

Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Enrico Weigelt, metux IT consult]

On 21.05.19 16:43, Greg Kroah-Hartman wrote:

Hi,

> Sometimes you need to go tell the hardware/firmware people not to do
> foolish things.  You can not always fix their problems in software.
> Please push back on this.

I've often been in the same situation. It's hopeless with those folks.
Even worse: you give'em a clear spec on how the register interface
shall look like, and they make something really weird out of it
(shuffled byte orders, multiplexing irqs over a single gpio line and
leave the other's empty instead of just one gpio per irq, ...)

The kernel is full of buggy hardware, because hw folks seem not really
capable of doing their homework :(

Actually, the whole existance of these hundreds of different uart
devices, IMHO, is a clear sing of hw folks not doing the homework.


By the way: I've somewhat lost track of what the patch was actually
about ... :o


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [PATCH 0/2] serial: 8250: Add support for 8250/16550 as MFD function

[Andy Shevchenko]

On Fri, Apr 26, 2019 at 10:40:36AM +0200, Esben Haabendal wrote:
> This series adds a driver for adding 8250/16550 UART ports as functions to
> MFD devices.
> 

The patch 2/2 disappeared.

> Esben Haabendal (2):
>   serial: 8250: Allow port registration without UPF_BOOT_AUTOCONF
>   serial: 8250: Add support for 8250/16550 as MFD function
> 
>  drivers/tty/serial/8250/8250_core.c |  36 ++++++-----
>  drivers/tty/serial/8250/8250_mfd.c  | 119 ++++++++++++++++++++++++++++++++++++
>  drivers/tty/serial/8250/Kconfig     |  12 ++++
>  drivers/tty/serial/8250/Makefile    |   1 +
>  include/linux/serial_8250.h         |   2 +
>  5 files changed, 155 insertions(+), 15 deletions(-)
>  create mode 100644 drivers/tty/serial/8250/8250_mfd.c
> 
> -- 
> 2.4.11
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/2] serial: 8250: Add support for 8250/16550 as MFD function

[Esben Haabendal]

Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

> On Fri, Apr 26, 2019 at 10:40:36AM +0200, Esben Haabendal wrote:
>> This series adds a driver for adding 8250/16550 UART ports as functions to
>> MFD devices.
>> 
>
> The patch 2/2 disappeared.

Yes, seems like that.  I have resent it by itself.  Or do I have to
resend the series as a whole?

/Esben

Re: [PATCH 0/2] serial: 8250: Add support for 8250/16550 as MFD function

[Andy Shevchenko]

On Fri, Apr 26, 2019 at 8:16 PM Esben Haabendal <esben@haabendal.dk> wrote:
>
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>
> > On Fri, Apr 26, 2019 at 10:40:36AM +0200, Esben Haabendal wrote:
> >> This series adds a driver for adding 8250/16550 UART ports as functions to
> >> MFD devices.
> >>
> >
> > The patch 2/2 disappeared.
>
> Yes, seems like that.  I have resent it by itself.  Or do I have to
> resend the series as a whole?

Series, but first check up on existing comments and try to address them first.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function

[Esben Haabendal]

The serial8250-mfd driver is for adding 8250/16550 UART ports as functions
to an MFD driver.

When calling mfd_add_device(), platform_data should be a pointer to a
struct plat_serial8250_port, with proper settings like .flags, .type,
.iotype, .regshift and .uartclk.  Memory (or ioport) and IRQ should be
passed as cell resources.

Do not include UPF_BOOT_AUTOCONF in platform_data.flags.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/tty/serial/8250/8250_mfd.c | 119 +++++++++++++++++++++++++++++++++++++
 drivers/tty/serial/8250/Kconfig    |  12 ++++
 drivers/tty/serial/8250/Makefile   |   1 +
 3 files changed, 132 insertions(+)
 create mode 100644 drivers/tty/serial/8250/8250_mfd.c

diff --git a/drivers/tty/serial/8250/8250_mfd.c b/drivers/tty/serial/8250/8250_mfd.c
new file mode 100644
index 0000000..eae1566
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_mfd.c
@@ -0,0 +1,119 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *  Serial Port driver for 8250/16550-type MFD sub devices
+ *
+ *  This mimics the serial8250_probe of 8250_core.c, while allowing
+ *  use without UPF_BOOT_AUTOCONF, which is problematic for MFD, as
+ *  the request_mem_region() will typically fail as the region is
+ *  already requested by the MFD device.
+ *
+ *  Memory and irq are passed as platform resources, which allows easy
+ *  use together with (devm_)mfd_add_devices().
+ *
+ *  Other parameters are passed as struct plat_serial8250_port in
+ *  device platform_data.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/serial_8250.h>
+
+struct serial8250_mfd_data {
+	int			line;
+};
+
+static int serial8250_mfd_probe(struct platform_device *pdev)
+{
+	struct plat_serial8250_port *pdata = dev_get_platdata(&pdev->dev);
+	struct serial8250_mfd_data *data;
+	struct uart_8250_port up;
+	struct resource *r;
+	void __iomem *membase;
+
+	if (!pdata)
+		return -ENODEV;
+
+	memset(&up, 0, sizeof(up));
+
+	switch (pdata->iotype) {
+	case UPIO_AU:
+	case UPIO_TSI:
+	case UPIO_MEM32:
+	case UPIO_MEM32BE:
+	case UPIO_MEM16:
+	case UPIO_MEM:
+		r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		if (!r)
+			return -ENODEV;
+		membase = devm_ioremap_nocache(&pdev->dev,
+					       r->start, resource_size(r));
+		if (!membase)
+			return -ENOMEM;
+		up.port.mapbase = r->start;
+		up.port.membase = membase;
+		break;
+	case UPIO_HUB6:
+	case UPIO_PORT:
+		r = platform_get_resource(pdev, IORESOURCE_IO, 0);
+		if (!r)
+			return -ENODEV;
+		up.port.iobase = r->start;
+		break;
+	}
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	up.port.irq = platform_get_irq(pdev, 0);
+	if (up.port.irq < 0)
+		up.port.irq = 0; /* no interrupt -> use polling */
+
+	/* Register with 8250_core.c */
+	up.port.irqflags = pdata->irqflags;
+	up.port.uartclk = pdata->uartclk;
+	up.port.regshift = pdata->regshift;
+	up.port.iotype = pdata->iotype;
+	up.port.flags = pdata->flags;
+	up.port.hub6 = pdata->hub6;
+	up.port.private_data = pdata->private_data;
+	up.port.type = pdata->type;
+	up.port.serial_in = pdata->serial_in;
+	up.port.serial_out = pdata->serial_out;
+	up.port.handle_irq = pdata->handle_irq;
+	up.port.handle_break = pdata->handle_break;
+	up.port.set_termios = pdata->set_termios;
+	up.port.set_ldisc = pdata->set_ldisc;
+	up.port.get_mctrl = pdata->get_mctrl;
+	up.port.pm = pdata->pm;
+	up.port.dev = &pdev->dev;
+	data->line = __serial8250_register_8250_port(&up, 0);
+	if (data->line < 0)
+		return data->line;
+
+	platform_set_drvdata(pdev, data);
+	return 0;
+}
+
+static int serial8250_mfd_remove(struct platform_device *pdev)
+{
+	struct serial8250_mfd_data *data = platform_get_drvdata(pdev);
+
+	serial8250_unregister_port(data->line);
+	return 0;
+}
+
+static struct platform_driver serial8250_mfd_driver = {
+	.probe = serial8250_mfd_probe,
+	.remove = serial8250_mfd_remove,
+	.driver = {
+		.name = "serial8250-mfd",
+	},
+};
+
+module_platform_driver(serial8250_mfd_driver);
+
+MODULE_AUTHOR("Esben Haabendal <esben@geanix.com>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Driver for 8250/16550-type MFD sub devices");
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 15c2c54..ef1572b 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -58,6 +58,18 @@ config SERIAL_8250_PNP
 	  This builds standard PNP serial support. You may be able to
 	  disable this feature if you only need legacy serial support.
 
+config SERIAL_8250_MFD
+	bool "8250/16550 MFD function support"
+	depends on SERIAL_8250 && MFD_CORE
+	default n
+	help
+	  This builds support for using 8250/16550-type UARTs as MFD
+	  functions.
+
+	  MFD drivers needing this should select it automatically.
+
+	  If unsure, say N.
+
 config SERIAL_8250_FINTEK
 	bool "Support for Fintek F81216A LPC to 4 UART RS485 API"
 	depends on SERIAL_8250
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index 18751bc..da8e139 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -6,6 +6,7 @@
 obj-$(CONFIG_SERIAL_8250)		+= 8250.o 8250_base.o
 8250-y					:= 8250_core.o
 8250-$(CONFIG_SERIAL_8250_PNP)		+= 8250_pnp.o
+8250-$(CONFIG_SERIAL_8250_MFD)		+= 8250_mfd.o
 8250_base-y				:= 8250_port.o
 8250_base-$(CONFIG_SERIAL_8250_DMA)	+= 8250_dma.o
 8250_base-$(CONFIG_SERIAL_8250_FINTEK)	+= 8250_fintek.o
-- 
2.4.11