Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1394119 > unrolled thread

Re: [PATCH v3 09/11] serial: 8250_lpss: move Quark code from PCI driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 09/11] serial: 8250_lpss: move Quark code from PCI  driver Bryan O'Donoghue <pure.logic@nexus-software.ie> - 2016-05-04 11:40 +0200
    Re: [PATCH v3 09/11] serial: 8250_lpss: move Quark code from PCI driver Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-05-04 11:50 +0200
      Re: [PATCH v3 09/11] serial: 8250_lpss: move Quark code from PCI  driver Bryan O'Donoghue <pure.logic@nexus-software.ie> - 2016-05-04 12:00 +0200
        Re: [PATCH v3 09/11] serial: 8250_lpss: move Quark code from PCI driver Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-05-04 12:10 +0200
          Re: [PATCH v3 09/11] serial: 8250_lpss: move Quark code from PCI  driver Bryan O'Donoghue <pure.logic@nexus-software.ie> - 2016-05-04 13:10 +0200
            Re: [PATCH v3 09/11] serial: 8250_lpss: move Quark code from PCI  driver Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2016-05-04 13:20 +0200
              Re: [PATCH v3 09/11] serial: 8250_lpss: move Quark code from PCI  driver Bryan O'Donoghue <pure.logic@nexus-software.ie> - 2016-05-04 16:40 +0200
                Re: [PATCH v3 09/11] serial: 8250_lpss: move Quark code from PCI  driver Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2016-05-04 17:00 +0200
              Re: [PATCH v3 09/11] serial: 8250_lpss: move Quark code from PCI  driver Bryan O'Donoghue <pure.logic@nexus-software.ie> - 2016-05-04 17:00 +0200
                Re: [PATCH v3 09/11] serial: 8250_lpss: move Quark code from PCI  driver Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2016-05-04 19:50 +0200
                  Re: [PATCH v3 09/11] serial: 8250_lpss: move Quark code from PCI  driver Bryan O'Donoghue <pure.logic@nexus-software.ie> - 2016-05-05 19:50 +0200
                    Re: [PATCH v3 09/11] serial: 8250_lpss: move Quark code from PCI  driver Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2016-05-06 12:40 +0200
        Re: [PATCH v3 09/11] serial: 8250_lpss: move Quark code from PCI  driver Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-04 15:20 +0200

#1394119 — Re: [PATCH v3 09/11] serial: 8250_lpss: move Quark code from PCI driver

On Wed, 2016-04-27 at 16:48 +0300, Andy Shevchenko wrote:
> Intel Quark has DesignWare UART. Move the code from 8250_pci to
> 8250_lpss.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/tty/serial/8250/8250_lpss.c | 11 +++++++++++
>  drivers/tty/serial/8250/8250_pci.c  | 15 +--------------
>  2 files changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_lpss.c
> b/drivers/tty/serial/8250/8250_lpss.c
> index 3112e8b..af34189 100644
> --- a/drivers/tty/serial/8250/8250_lpss.c
> +++ b/drivers/tty/serial/8250/8250_lpss.c

Andy,

If you are going to start removing working PCI devices from the PCI
config table in favour of a shim in SERIAL_8250_LPSS then the very
minimum should be some sort of dependency link between SERIAL_8250_LPSS
and CONFIG_SERIAL_8250_PCI in kconfig.

A user could reasonably read the QRK datasheet - switch on
CONFIG_SERIAL_8250_PCI and then wonder why no console output happened
on boot. S/he shouldn't have to know that devices were moved from the
PCI driver to an LPSS shim driver or that the 8250_lpss driver now
needs to be selected instead of the intuitively correct 8250_pci
driver.

So assuming you agree with that (profound and sublime) logic and will
make that dependency linkage then this is fine for me from a QRK POV.

Reviewed-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>

[toc] | [next] | [standalone]

#1394120 — Re: [PATCH v3 09/11] serial: 8250_lpss: move Quark code from PCI driver

On Wed, May 4, 2016 at 12:31 PM, Bryan O'Donoghue
<pure.logic@nexus-software.ie> wrote:
> On Wed, 2016-04-27 at 16:48 +0300, Andy Shevchenko wrote:
>> Intel Quark has DesignWare UART. Move the code from 8250_pci to
>> 8250_lpss.
>>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> ---
>>  drivers/tty/serial/8250/8250_lpss.c | 11 +++++++++++
>>  drivers/tty/serial/8250/8250_pci.c  | 15 +--------------
>>  2 files changed, 12 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/tty/serial/8250/8250_lpss.c
>> b/drivers/tty/serial/8250/8250_lpss.c
>> index 3112e8b..af34189 100644
>> --- a/drivers/tty/serial/8250/8250_lpss.c
>> +++ b/drivers/tty/serial/8250/8250_lpss.c
>
> Andy,
>
> If you are going to start removing working PCI devices from the PCI
> config table in favour of a shim in SERIAL_8250_LPSS then the very
> minimum should be some sort of dependency link between SERIAL_8250_LPSS
> and CONFIG_SERIAL_8250_PCI in kconfig.
>
> A user could reasonably read the QRK datasheet - switch on
> CONFIG_SERIAL_8250_PCI and then wonder why no console output happened
> on boot. S/he shouldn't have to know that devices were moved from the
> PCI driver to an LPSS shim driver or that the 8250_lpss driver now
> needs to be selected instead of the intuitively correct 8250_pci
> driver.

That is taken care of since default is set to SERIAL_8250 (you even
don't need to have PCI driver enabled!).
Doesn't work for you?

>
> So assuming you agree with that (profound and sublime) logic and will
> make that dependency linkage then this is fine for me from a QRK POV.
>
> Reviewed-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>

Thanks!

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1394128

On Wed, 2016-05-04 at 12:42 +0300, Andy Shevchenko wrote:
> On Wed, May 4, 2016 at 12:31 PM, Bryan O'Donoghue
> <pure.logic@nexus-software.ie> wrote:
> > Andy,
> > 
> > If you are going to start removing working PCI devices from the PCI
> > config table in favour of a shim in SERIAL_8250_LPSS then the very
> > minimum should be some sort of dependency link between
> > SERIAL_8250_LPSS
> > and CONFIG_SERIAL_8250_PCI in kconfig.
> > 
> > A user could reasonably read the QRK datasheet - switch on
> > CONFIG_SERIAL_8250_PCI and then wonder why no console output
> > happened
> > on boot. S/he shouldn't have to know that devices were moved from
> > the
> > PCI driver to an LPSS shim driver or that the 8250_lpss driver now
> > needs to be selected instead of the intuitively correct 8250_pci
> > driver.
> 
> That is taken care of since default is set to SERIAL_8250 (you even
> don't need to have PCI driver enabled!).
> Doesn't work for you?

The default may be set to SERIAL_8250 but, without the QRK specific
entry in 8250_pci.c you won't get console output.

So if you are going to remove the QRK entry from 8250_pci.c and stuff
it into 8250_lpss.c then 8250_lpss needs to be selected by
CONFIG_SERIAL_8250_PCI.

Otherwise the person doing the config needs to know that stuff was
moved from one file to another - even though it's a PCI device (not an
LPSS/ACPI enumerated device) - which seems like an unreasonable level
of knowledge to assume on the part of the user.

[toc] | [prev] | [next] | [standalone]

#1394130 — Re: [PATCH v3 09/11] serial: 8250_lpss: move Quark code from PCI driver

On Wed, May 4, 2016 at 12:51 PM, Bryan O'Donoghue
<pure.logic@nexus-software.ie> wrote:
> On Wed, 2016-05-04 at 12:42 +0300, Andy Shevchenko wrote:
>> On Wed, May 4, 2016 at 12:31 PM, Bryan O'Donoghue
>> <pure.logic@nexus-software.ie> wrote:
>> > Andy,
>> >
>> > If you are going to start removing working PCI devices from the PCI
>> > config table in favour of a shim in SERIAL_8250_LPSS then the very
>> > minimum should be some sort of dependency link between
>> > SERIAL_8250_LPSS
>> > and CONFIG_SERIAL_8250_PCI in kconfig.
>> >
>> > A user could reasonably read the QRK datasheet - switch on
>> > CONFIG_SERIAL_8250_PCI and then wonder why no console output
>> > happened
>> > on boot. S/he shouldn't have to know that devices were moved from
>> > the
>> > PCI driver to an LPSS shim driver or that the 8250_lpss driver now
>> > needs to be selected instead of the intuitively correct 8250_pci
>> > driver.
>>
>> That is taken care of since default is set to SERIAL_8250 (you even
>> don't need to have PCI driver enabled!).
>> Doesn't work for you?
>
> The default may be set to SERIAL_8250 but, without the QRK specific
> entry in 8250_pci.c you won't get console output.
>
> So if you are going to remove the QRK entry from 8250_pci.c and stuff
> it into 8250_lpss.c then 8250_lpss needs to be selected by
> CONFIG_SERIAL_8250_PCI.

Why?!

Now it should be enough to have SERIAL_8250 set to non-n to have
8250_lpss compiled.
Can you check it?

>
> Otherwise the person doing the config needs to know that stuff was
> moved from one file to another - even though it's a PCI device (not an
> LPSS/ACPI enumerated device) - which seems like an unreasonable level
> of knowledge to assume on the part of the user.
>

I understand that, that's why Heikki proposed to do something sane for
8250_mid and I use it as an example for this one.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1394150

On Wed, 2016-05-04 at 13:03 +0300, Andy Shevchenko wrote:
> On Wed, May 4, 2016 at 12:51 PM, Bryan O'Donoghue
> <pure.logic@nexus-software.ie> wrote:
> > On Wed, 2016-05-04 at 12:42 +0300, Andy Shevchenko wrote:
> > > On Wed, May 4, 2016 at 12:31 PM, Bryan O'Donoghue
> > > <pure.logic@nexus-software.ie> wrote:
> > > > Andy,
> > > > 
> > > > If you are going to start removing working PCI devices from the
> > > > PCI
> > > > config table in favour of a shim in SERIAL_8250_LPSS then the
> > > > very
> > > > minimum should be some sort of dependency link between
> > > > SERIAL_8250_LPSS
> > > > and CONFIG_SERIAL_8250_PCI in kconfig.
> > > > 
> > > > A user could reasonably read the QRK datasheet - switch on
> > > > CONFIG_SERIAL_8250_PCI and then wonder why no console output
> > > > happened
> > > > on boot. S/he shouldn't have to know that devices were moved
> > > > from
> > > > the
> > > > PCI driver to an LPSS shim driver or that the 8250_lpss driver
> > > > now
> > > > needs to be selected instead of the intuitively correct
> > > > 8250_pci
> > > > driver.
> > > 
> > > That is taken care of since default is set to SERIAL_8250 (you
> > > even
> > > don't need to have PCI driver enabled!).
> > > Doesn't work for you?
> > 
> > The default may be set to SERIAL_8250 but, without the QRK specific
> > entry in 8250_pci.c you won't get console output.
> > 
> > So if you are going to remove the QRK entry from 8250_pci.c and
> > stuff
> > it into 8250_lpss.c then 8250_lpss needs to be selected by
> > CONFIG_SERIAL_8250_PCI.
> 
> Why?!
> 
> Now it should be enough to have SERIAL_8250 set to non-n to have
> 8250_lpss compiled.
> Can you check it?

I'm sure that's true.

My point to you is that - its a highly non-intuitive thing to do on a
reading of the datasheet for this part.

LPSS is, ostensibly at least, for passing processor resources via APCI.

If you look at a QRK datasheet it says "enumerate all this stuff via
PCI" - there's not a single mention of LPSS. Its reasonable, correct
and currently required for QRK to set CONFIG_8250_PCI.

To move away from a valid/standard PCI probe routine into a new special
LPSS/PCI shim (which the hardware doesn't actually mandate) I do think
you should to setup the dependency CONFIG_8250_PCI => CONFIG_8250_LPSS.

---
bod

[toc] | [prev] | [next] | [standalone]

#1394159

On Wed, 2016-05-04 at 12:01 +0100, Bryan O'Donoghue wrote:
> On Wed, 2016-05-04 at 13:03 +0300, Andy Shevchenko wrote:
> > 
> > On Wed, May 4, 2016 at 12:51 PM, Bryan O'Donoghue
> > <pure.logic@nexus-software.ie> wrote:
> > > 
> > > The default may be set to SERIAL_8250 but, without the QRK
> > > specific
> > > entry in 8250_pci.c you won't get console output.

That's, by the way, not true.

> > > 
> > > So if you are going to remove the QRK entry from 8250_pci.c and
> > > stuff
> > > it into 8250_lpss.c then 8250_lpss needs to be selected by
> > > CONFIG_SERIAL_8250_PCI.
> > Why?!
> > 
> > Now it should be enough to have SERIAL_8250 set to non-n to have
> > 8250_lpss compiled.
> > Can you check it?
> I'm sure that's true.
> 
> My point to you is that - its a highly non-intuitive thing to do on a
> reading of the datasheet for this part.
> 
> LPSS is, ostensibly at least, for passing processor resources via
> APCI.
> 
> If you look at a QRK datasheet it says "enumerate all this stuff via
> PCI" - there's not a single mention of LPSS. Its reasonable, correct
> and currently required for QRK to set CONFIG_8250_PCI.

User has no such item even visible until enable CONFIG_EXPERT.

Heikki sent an answer to you (and to the list, but by some reason it's
not yet there) an hour ago.

> 
> To move away from a valid/standard PCI probe routine into a new
> special
> LPSS/PCI shim (which the hardware doesn't actually mandate) I do think
> you should to setup the dependency CONFIG_8250_PCI =>
> CONFIG_8250_LPSS.

No, this is what we try avoiding, thus it will not happen.

If user selects CONFIG_SERIAL_8250_PCI, the CONFIG_SERIAL_8250_LPSS will
be selected as well since it has same dependencies.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

#1394373

On Wed, 2016-05-04 at 14:20 +0300, Andy Shevchenko wrote:
> On Wed, 2016-05-04 at 12:01 +0100, Bryan O'Donoghue wrote:
> > On Wed, 2016-05-04 at 13:03 +0300, Andy Shevchenko wrote:
> > > 
> > > On Wed, May 4, 2016 at 12:51 PM, Bryan O'Donoghue
> > > <pure.logic@nexus-software.ie> wrote:
> > > > 
> > > > The default may be set to SERIAL_8250 but, without the QRK
> > > > specific
> > > > entry in 8250_pci.c you won't get console output.
> 
> That's, by the way, not true.

Since when ? We don't have an I/O bar so mapping the MMIO bar @ the
right register width is required.

> 
> > > > 
> > > > So if you are going to remove the QRK entry from 8250_pci.c and
> > > > stuff
> > > > it into 8250_lpss.c then 8250_lpss needs to be selected by
> > > > CONFIG_SERIAL_8250_PCI.
> > > Why?!
> > > 
> > > Now it should be enough to have SERIAL_8250 set to non-n to have
> > > 8250_lpss compiled.
> > > Can you check it?
> > I'm sure that's true.
> > 
> > My point to you is that - its a highly non-intuitive thing to do on
> > a
> > reading of the datasheet for this part.
> > 
> > LPSS is, ostensibly at least, for passing processor resources via
> > APCI.
> > 
> > If you look at a QRK datasheet it says "enumerate all this stuff
> > via
> > PCI" - there's not a single mention of LPSS. Its reasonable,
> > correct
> > and currently required for QRK to set CONFIG_8250_PCI.
> 
> User has no such item even visible until enable CONFIG_EXPERT.
> 
> Heikki sent an answer to you (and to the list, but by some reason
> it's
> not yet there) an hour ago.
> 
> > 
> > To move away from a valid/standard PCI probe routine into a new
> > special
> > LPSS/PCI shim (which the hardware doesn't actually mandate) I do
> > think
> > you should to setup the dependency CONFIG_8250_PCI =>
> > CONFIG_8250_LPSS.
> 
> No, this is what we try avoiding, thus it will not happen.
> 
> If user selects CONFIG_SERIAL_8250_PCI, the CONFIG_SERIAL_8250_LPSS
> will
> be selected as well since it has same dependencies.

Hmm. I think what you mean to say is that a user (expert or not)
*would* select SERIAL_8250_LPSS since (at least in your branch
 09c4268121a39eb3973823dd9225b650df726f67) both options may be
individually selected/deselected.

[toc] | [prev] | [next] | [standalone]

#1394420

On Wed, 2016-05-04 at 15:37 +0100, Bryan O'Donoghue wrote:
> On Wed, 2016-05-04 at 14:20 +0300, Andy Shevchenko wrote:
> > 
> > On Wed, 2016-05-04 at 12:01 +0100, Bryan O'Donoghue wrote:
> > > 
> > > On Wed, 2016-05-04 at 13:03 +0300, Andy Shevchenko wrote:
> > > > 
> > > > 
> > > > On Wed, May 4, 2016 at 12:51 PM, Bryan O'Donoghue
> > > > <pure.logic@nexus-software.ie> wrote:
> > > > > 
> > > > > 
> > > > > The default may be set to SERIAL_8250 but, without the QRK
> > > > > specific
> > > > > entry in 8250_pci.c you won't get console output.
> > That's, by the way, not true.
> Since when ? We don't have an I/O bar so mapping the MMIO bar @ the
> right register width is required.

Since this series.
8250_lpss will be enabled as long as user doesn't enable EXPERT and
_explicitly_ _disables_ it.

Same is applied to SERIAL_8250_PCI. If you look at the default kernel
configurations such as i386_default you don't find that option there.

Btw, I have to clean up such in my branches.

> 
> > > > > So if you are going to remove the QRK entry from 8250_pci.c
> > > > > and
> > > > > stuff
> > > > > it into 8250_lpss.c then 8250_lpss needs to be selected by
> > > > > CONFIG_SERIAL_8250_PCI.
> > > > Why?!
> > > > 
> > > > Now it should be enough to have SERIAL_8250 set to non-n to have
> > > > 8250_lpss compiled.
> > > > Can you check it?
> > > I'm sure that's true.
> > > 
> > > My point to you is that - its a highly non-intuitive thing to do
> > > on
> > > a
> > > reading of the datasheet for this part.
> > > 
> > > LPSS is, ostensibly at least, for passing processor resources via
> > > APCI.
> > > 
> > > If you look at a QRK datasheet it says "enumerate all this stuff
> > > via
> > > PCI" - there's not a single mention of LPSS. Its reasonable,
> > > correct
> > > and currently required for QRK to set CONFIG_8250_PCI.
> > User has no such item even visible until enable CONFIG_EXPERT.
> > 
> > Heikki sent an answer to you (and to the list, but by some reason
> > it's
> > not yet there) an hour ago.
> > 
> > > 
> > > 
> > > To move away from a valid/standard PCI probe routine into a new
> > > special
> > > LPSS/PCI shim (which the hardware doesn't actually mandate) I do
> > > think
> > > you should to setup the dependency CONFIG_8250_PCI =>
> > > CONFIG_8250_LPSS.
> > No, this is what we try avoiding, thus it will not happen.
> > 
> > If user selects CONFIG_SERIAL_8250_PCI, the CONFIG_SERIAL_8250_LPSS
> > will
> > be selected as well since it has same dependencies.
> Hmm. I think what you mean to say is that a user (expert or not)
> *would* select SERIAL_8250_LPSS since (at least in your branch
>  09c4268121a39eb3973823dd9225b650df726f67) both options may be
> individually selected/deselected.

So, currently it works in such way that user enables SERIAL_8250 and
_dependencies_, which are PCI (for SERIAL_8250_PCI) or PCI && X86 (for
SERIAL_8250_LPSS) and drivers will be built automatically on the same
level (m or y) as SERIAL_8250.

Nevertheless, user may _disable_ them if needed using EXPERT option.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

#1394416

On Wed, 2016-05-04 at 14:20 +0300, Andy Shevchenko wrote:

> > To move away from a valid/standard PCI probe routine into a new
> > special
> > LPSS/PCI shim (which the hardware doesn't actually mandate) I do
> > think
> > you should to setup the dependency CONFIG_8250_PCI =>
> > CONFIG_8250_LPSS.
> 
> No, this is what we try avoiding

Fine.

Could you then select CONFIG_SERIAL_8250_LPSS when
CONFIG_X86_INTEL_QUARK is true - since it will be a dependency.

> If user selects CONFIG_SERIAL_8250_PCI, the CONFIG_SERIAL_8250_LPSS
> will
> be selected as well since it has same dependencies.

I still think the change is not an obvious one i.e. LPSS (as an ACPI
enumeration concept) is not a requirement to enumerate on Quark X1000.

So I understand why you want to separate out the code from 8250_pci -
however I think the *minimum* here should be a descriptive comment in
kconfig listing which PCI-enumerated SoCs now require the 8250_LPSS
work-around if just selecting 8250_LPSS isn't possible.

So how about listing out those SoCs - something like

"Selecting this option will enable handling of the extra features 
 present on the UART found on Intel Braswell SoC and various  other
 Intel platforms."

=>

"Selecting this option will enable handling of the extra features 
 present on the UART found on
 - Intel Braswell SoC
 - Intel Quark x1000 SoC
 - etc
"
If you make those changes - please add.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@nexus-software.ie>

[toc] | [prev] | [next] | [standalone]

#1394578

On Wed, 2016-05-04 at 15:51 +0100, Bryan O'Donoghue wrote:
> On Wed, 2016-05-04 at 14:20 +0300, Andy Shevchenko wrote:
> 
> > 
> > > 
> > > To move away from a valid/standard PCI probe routine into a new
> > > special
> > > LPSS/PCI shim (which the hardware doesn't actually mandate) I do
> > > think
> > > you should to setup the dependency CONFIG_8250_PCI =>
> > > CONFIG_8250_LPSS.
> > No, this is what we try avoiding
> Fine.
> 
> Could you then select CONFIG_SERIAL_8250_LPSS when
> CONFIG_X86_INTEL_QUARK is true - since it will be a dependency.

Answered to this in the other email, but can repeat my question. Do you
propose a new behaviour? Otherwise how does it work right now?

> 
> > 
> > If user selects CONFIG_SERIAL_8250_PCI, the CONFIG_SERIAL_8250_LPSS
> > will
> > be selected as well since it has same dependencies.
> I still think the change is not an obvious one i.e. LPSS (as an ACPI
> enumeration concept)

LPSS is a hardware concept. It might be not exactly one island on the
SoC, but it pretty much includes all those serial bus controllers and
DMA.

>  is not a requirement to enumerate on Quark X1000.
> 
> So I understand why you want to separate out the code from 8250_pci -
> however I think the *minimum* here should be a descriptive comment in
> kconfig listing which PCI-enumerated SoCs now require the 8250_LPSS
> work-around if just selecting 8250_LPSS isn't possible.
> 
> So how about listing out those SoCs - something like
> 
> "Selecting this option will enable handling of the extra features 
>  present on the UART found on Intel Braswell SoC and various  other
>  Intel platforms."
> 
> =>
> 
> "Selecting this option will enable handling of the extra features 
>  present on the UART found on
>  - Intel Braswell SoC
>  - Intel Quark x1000 SoC
>  - etc
> "
> If you make those changes - please add.

That would work for me. Will update it in next version. I'm still give a
time to answer for the questions above. I want us to be on the same
page.

> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@nexus-software.ie>

Thanks for review!

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

#1395253

On Wed, 2016-05-04 at 20:43 +0300, Andy Shevchenko wrote:
> > Could you then select CONFIG_SERIAL_8250_LPSS when
> > CONFIG_X86_INTEL_QUARK is true - since it will be a dependency.
> 
> Answered to this in the other email, but can repeat my question. Do
> you
> propose a new behaviour? Otherwise how does it work right now?

I just mean when someone selects CONFIG_X86_INTEL_QUARK that, that
kconfig option will select CONFIG_SERIAL_8250_LPSS automatically.

Maybe that's a bad idea .. I can't think of another driver that would
be selected like that - though OTOH now that we're moving our perfectly
normal PCI device into this LPSS shim my *feeling* is that it should be
selected automagically.

Feel free not to do that though - so long as you document the change in
kconfig so that a casual reader understands the entry is moved I think
it should suffice.

[toc] | [prev] | [next] | [standalone]

#1395762

On Thu, 2016-05-05 at 18:49 +0100, Bryan O'Donoghue wrote:
> On Wed, 2016-05-04 at 20:43 +0300, Andy Shevchenko wrote:
> > 
> > > 
> > > Could you then select CONFIG_SERIAL_8250_LPSS when
> > > CONFIG_X86_INTEL_QUARK is true - since it will be a dependency.
> > Answered to this in the other email, but can repeat my question. Do
> > you
> > propose a new behaviour? Otherwise how does it work right now?
> I just mean when someone selects CONFIG_X86_INTEL_QUARK that, that
> kconfig option will select CONFIG_SERIAL_8250_LPSS automatically.
> 
> Maybe that's a bad idea .. I can't think of another driver that would
> be selected like that - though OTOH now that we're moving our
> perfectly
> normal PCI device into this LPSS shim my *feeling* is that it should
> be
> selected automagically.

It's still a PCI driver, just separated from 8250_pci.c.

> Feel free not to do that though - so long as you document the change
> in
> kconfig so that a casual reader understands the entry is moved I think
> it should suffice.

I've already updated (locally) help notice as you suggested.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[toc] | [prev] | [next] | [standalone]

#1394278

Hi Bryan,

On Wed, May 04, 2016 at 10:51:17AM +0100, Bryan O'Donoghue wrote:
> On Wed, 2016-05-04 at 12:42 +0300, Andy Shevchenko wrote:
> > On Wed, May 4, 2016 at 12:31 PM, Bryan O'Donoghue
> > <pure.logic@nexus-software.ie> wrote:
> > > Andy,
> > > 
> > > If you are going to start removing working PCI devices from the PCI
> > > config table in favour of a shim in SERIAL_8250_LPSS then the very
> > > minimum should be some sort of dependency link between
> > > SERIAL_8250_LPSS
> > > and CONFIG_SERIAL_8250_PCI in kconfig.
> > > 
> > > A user could reasonably read the QRK datasheet - switch on
> > > CONFIG_SERIAL_8250_PCI and then wonder why no console output
> > > happened
> > > on boot. S/he shouldn't have to know that devices were moved from
> > > the
> > > PCI driver to an LPSS shim driver or that the 8250_lpss driver now
> > > needs to be selected instead of the intuitively correct 8250_pci
> > > driver.
> > 
> > That is taken care of since default is set to SERIAL_8250 (you even
> > don't need to have PCI driver enabled!).
> > Doesn't work for you?
> 
> The default may be set to SERIAL_8250 but, without the QRK specific
> entry in 8250_pci.c you won't get console output.
> 
> So if you are going to remove the QRK entry from 8250_pci.c and stuff
> it into 8250_lpss.c then 8250_lpss needs to be selected by
> CONFIG_SERIAL_8250_PCI.
> 
> Otherwise the person doing the config needs to know that stuff was
> moved from one file to another - even though it's a PCI device (not an
> LPSS/ACPI enumerated device) - which seems like an unreasonable level
> of knowledge to assume on the part of the user.

The only way the user get's to de-select CONFIG_SERIAL_8250_PCI or
CONFIG_SERIAL_8250_LPSS is if S/he also selects CONFIG_EXPERT, and if
S/he does that then we can assume S/he has the knowledge.

I'm against binding these separated drivers to CONFIG_SERIAL_8250_PCI
because doing that will very fast mean that we also remove the
possibility to de-select CONFIG_SERIAL_8250_PCI when only, for
example, CONFIG_SERIAL_8250_LPSS was wanted.


Thanks,

-- 
heikki

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web