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

Groups > linux.kernel > #1681173 > unrolled thread

Re: [PATCH v9 0/3] Tango PCIe controller support

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 v9 0/3] Tango PCIe controller support Bjorn Helgaas <helgaas@kernel.org> - 2017-07-04 22:30 +0200
    Re: [PATCH v9 0/3] Tango PCIe controller support Mason <slash.tmp@free.fr> - 2017-07-05 01:00 +0200
      Re: [PATCH v9 0/3] Tango PCIe controller support Bjorn Helgaas <helgaas@kernel.org> - 2017-07-05 20:10 +0200
        Re: [PATCH v9 0/3] Tango PCIe controller support Mason <slash.tmp@free.fr> - 2017-07-05 22:50 +0200
          Re: [PATCH v9 0/3] Tango PCIe controller support Bjorn Helgaas <helgaas@kernel.org> - 2017-07-05 23:40 +0200
            Re: [PATCH v9 0/3] Tango PCIe controller support Mason <slash.tmp@free.fr> - 2017-07-06 00:10 +0200
              Re: [PATCH v9 0/3] Tango PCIe controller support Bjorn Helgaas <helgaas@kernel.org> - 2017-07-06 05:50 +0200
                Re: [PATCH v9 0/3] Tango PCIe controller support Mason <slash.tmp@free.fr> - 2017-07-06 14:30 +0200
                  Re: [PATCH v9 0/3] Tango PCIe controller support Marc Zyngier <marc.zyngier@arm.com> - 2017-07-06 14:50 +0200
                  Re: [PATCH v9 0/3] Tango PCIe controller support Bjorn Helgaas <helgaas@kernel.org> - 2017-07-06 21:50 +0200

#1681173 — Re: [PATCH v9 0/3] Tango PCIe controller support

On Tue, Jun 20, 2017 at 10:12:57AM +0200, Marc Gonzalez wrote:
> Marc Z pointed out that posting partial series is not ideal.
> Collect last-minute fixups into a single patch series.
> 
> - Bump series to v9 to avoid any ambiguity
> - Add Rob's Ack on patch 1
> 
> Marc Gonzalez (3):
>   PCI: Add DT binding for tango PCIe controller
>   PCI: Add tango PCIe host bridge support
>   PCI: Add tango MSI controller support
> 
>  .../devicetree/bindings/pci/tango-pcie.txt         |  29 ++
>  drivers/pci/host/Kconfig                           |   8 +
>  drivers/pci/host/Makefile                          |   1 +
>  drivers/pci/host/pcie-tango.c                      | 390 +++++++++++++++++++++
>  include/linux/pci_ids.h                            |   2 +
>  5 files changed, 430 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/tango-pcie.txt
>  create mode 100644 drivers/pci/host/pcie-tango.c

I made the trivial changes I mentioned, added a dependency on
CONFIG_BROKEN (for the config/MMIO muxing issue), and put these on
pci/host-tango.  I can't build or test this, so I probably broke
something in the process.  I think the combination of the boot-time
warning, the taint, and CONFIG_BROKEN is a reasonable amount of
warning that a user should expect issues.

Can you take a look and see if it works for you?

https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/host-tango

Bjorn

[toc] | [next] | [standalone]

#1681208

On 04/07/2017 22:24, Bjorn Helgaas wrote:

> On Tue, Jun 20, 2017 at 10:12:57AM +0200, Marc Gonzalez wrote:
>
>> Marc Z pointed out that posting partial series is not ideal.
>> Collect last-minute fixups into a single patch series.
>>
>> - Bump series to v9 to avoid any ambiguity
>> - Add Rob's Ack on patch 1
>>
>> Marc Gonzalez (3):
>>   PCI: Add DT binding for tango PCIe controller
>>   PCI: Add tango PCIe host bridge support
>>   PCI: Add tango MSI controller support
>>
>>  .../devicetree/bindings/pci/tango-pcie.txt         |  29 ++
>>  drivers/pci/host/Kconfig                           |   8 +
>>  drivers/pci/host/Makefile                          |   1 +
>>  drivers/pci/host/pcie-tango.c                      | 390 +++++++++++++++++++++
>>  include/linux/pci_ids.h                            |   2 +
>>  5 files changed, 430 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/pci/tango-pcie.txt
>>  create mode 100644 drivers/pci/host/pcie-tango.c
> 
> I made the trivial changes I mentioned, added a dependency on
> CONFIG_BROKEN (for the config/MMIO muxing issue), and put these on
> pci/host-tango.  I can't build or test this, so I probably broke
> something in the process.  I think the combination of the boot-time
> warning, the taint, and CONFIG_BROKEN is a reasonable amount of
> warning that a user should expect issues.
> 
> Can you take a look and see if it works for you?
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/host-tango

Thanks. I'll take it for a spin ASAP.

TAINT_CRAP... Smirk. I didn't see that one in the docs:
https://www.kernel.org/doc/html/latest/admin-guide/tainted-kernels.html

Oh wait... TAINT_CRAP is "C" => a staging driver has been loaded

The one issue I anticipate with "depends on BROKEN" is
when I add support for revision 2, which isn't broken.

Regards.

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

#1681692

On Wed, Jul 05, 2017 at 12:55:37AM +0200, Mason wrote:
> On 04/07/2017 22:24, Bjorn Helgaas wrote:
> 
> > On Tue, Jun 20, 2017 at 10:12:57AM +0200, Marc Gonzalez wrote:
> >
> >> Marc Z pointed out that posting partial series is not ideal.
> >> Collect last-minute fixups into a single patch series.
> >>
> >> - Bump series to v9 to avoid any ambiguity
> >> - Add Rob's Ack on patch 1
> >>
> >> Marc Gonzalez (3):
> >>   PCI: Add DT binding for tango PCIe controller
> >>   PCI: Add tango PCIe host bridge support
> >>   PCI: Add tango MSI controller support
> >>
> >>  .../devicetree/bindings/pci/tango-pcie.txt         |  29 ++
> >>  drivers/pci/host/Kconfig                           |   8 +
> >>  drivers/pci/host/Makefile                          |   1 +
> >>  drivers/pci/host/pcie-tango.c                      | 390 +++++++++++++++++++++
> >>  include/linux/pci_ids.h                            |   2 +
> >>  5 files changed, 430 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/pci/tango-pcie.txt
> >>  create mode 100644 drivers/pci/host/pcie-tango.c
> > 
> > I made the trivial changes I mentioned, added a dependency on
> > CONFIG_BROKEN (for the config/MMIO muxing issue), and put these on
> > pci/host-tango.  I can't build or test this, so I probably broke
> > something in the process.  I think the combination of the boot-time
> > warning, the taint, and CONFIG_BROKEN is a reasonable amount of
> > warning that a user should expect issues.
> > 
> > Can you take a look and see if it works for you?
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/host-tango
> 
> Thanks. I'll take it for a spin ASAP.
> 
> TAINT_CRAP... Smirk. I didn't see that one in the docs:
> https://www.kernel.org/doc/html/latest/admin-guide/tainted-kernels.html
> 
> Oh wait... TAINT_CRAP is "C" => a staging driver has been loaded

I wish it had a less pejorative, more descriptive name.  But it seems like
the closest to this situation.

> The one issue I anticipate with "depends on BROKEN" is
> when I add support for revision 2, which isn't broken.

How about this:

  - Rename PCIE_TANGO to PCIE_TANGO_REV1
  - PCIE_TANGO_REV1 depends on BROKEN
  - Add rev2 support later, enabled by PCIE_TANGO
  - PCIE_TANGO_REV1 depends on PCIE_TANGO && BROKEN

I updated pci/host-tango along these lines (without rev2 support,
obviously).

I forgot to ask for a MAINTAINERS update.  Can you send that, too,
please?

Which reminds me -- are these two addreses

  Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
  Mason <slash.tmp@free.fr>

different names for the same person?  Conversations are easier for me
if I can keep who's who straight :)

Bjorn

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

#1681789

On 05/07/2017 20:03, Bjorn Helgaas wrote:

> On Wed, Jul 05, 2017 at 12:55:37AM +0200, Mason wrote:
>
>> On 04/07/2017 22:24, Bjorn Helgaas wrote:
>>
>>> I made the trivial changes I mentioned, added a dependency on
>>> CONFIG_BROKEN (for the config/MMIO muxing issue), and put these on
>>> pci/host-tango.  I can't build or test this, so I probably broke
>>> something in the process.  I think the combination of the boot-time
>>> warning, the taint, and CONFIG_BROKEN is a reasonable amount of
>>> warning that a user should expect issues.
>>>
>>> Can you take a look and see if it works for you?
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/host-tango
>>
>> Thanks. I'll take it for a spin ASAP.
>>
>> TAINT_CRAP... Smirk. I didn't see that one in the docs:
>> https://www.kernel.org/doc/html/latest/admin-guide/tainted-kernels.html
>>
>> Oh wait... TAINT_CRAP is "C" => a staging driver has been loaded
> 
> I wish it had a less pejorative, more descriptive name.  But it seems like
> the closest to this situation.

Maybe it is not too late to submit a patch to Linus
renaming TAINT_CRAP?

Here are a few candidates, off the top of my head:

TAINT_STAGING
TAINT_STAGING_DRIVER
TAINT_BROKEN_HW
TAINT_BROKEN_HARDWARE
TAINT_USE_AT_YOUR_OWN_RISK

>> The one issue I anticipate with "depends on BROKEN" is
>> when I add support for revision 2, which isn't broken.
> 
> How about this:
> 
>   - Rename PCIE_TANGO to PCIE_TANGO_REV1
>   - PCIE_TANGO_REV1 depends on BROKEN
>   - Add rev2 support later, enabled by PCIE_TANGO
>   - PCIE_TANGO_REV1 depends on PCIE_TANGO && BROKEN
> 
> I updated pci/host-tango along these lines (without rev2 support,
> obviously).

And support for REV1 wouldn't be compiled in, unless
BROKEN is selected? Yes, I think that could fly.

Don't you think the naming should follow the DT
convention of using the first SoC embedding the
IP (for the compatible string) ?

PCIE_TANGO_REV1 vs PCIE_TANGO_SMP8759

> I forgot to ask for a MAINTAINERS update.  Can you send that, too,
> please?

There's a "catch-all" rule for everything tango-related:

ARM/TANGO ARCHITECTURE
M:	Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
L:	linux-arm-kernel@lists.infradead.org
S:	Maintained
N:	tango

Is that enough?

> Which reminds me -- are these two addreses
> 
>   Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>   Mason <slash.tmp@free.fr>
> 
> different names for the same person?  Conversations are easier for me
> if I can keep who's who straight :)

Well, there are many voices inside my head, but yes,
respectively professional and personal addresses.

Regards.

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

#1681860

On Wed, Jul 05, 2017 at 10:39:19PM +0200, Mason wrote:
> On 05/07/2017 20:03, Bjorn Helgaas wrote:
> 
> > On Wed, Jul 05, 2017 at 12:55:37AM +0200, Mason wrote:
> >
> >> On 04/07/2017 22:24, Bjorn Helgaas wrote:
> >>
> >>> I made the trivial changes I mentioned, added a dependency on
> >>> CONFIG_BROKEN (for the config/MMIO muxing issue), and put these on
> >>> pci/host-tango.  I can't build or test this, so I probably broke
> >>> something in the process.  I think the combination of the boot-time
> >>> warning, the taint, and CONFIG_BROKEN is a reasonable amount of
> >>> warning that a user should expect issues.
> >>>
> >>> Can you take a look and see if it works for you?
> >>>
> >>> https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/host-tango
> >>
> >> Thanks. I'll take it for a spin ASAP.
> >>
> >> TAINT_CRAP... Smirk. I didn't see that one in the docs:
> >> https://www.kernel.org/doc/html/latest/admin-guide/tainted-kernels.html
> >>
> >> Oh wait... TAINT_CRAP is "C" => a staging driver has been loaded
> > 
> > I wish it had a less pejorative, more descriptive name.  But it seems like
> > the closest to this situation.
> 
> Maybe it is not too late to submit a patch to Linus
> renaming TAINT_CRAP?
> 
> Here are a few candidates, off the top of my head:
> 
> TAINT_STAGING
> TAINT_STAGING_DRIVER
> TAINT_BROKEN_HW
> TAINT_BROKEN_HARDWARE
> TAINT_USE_AT_YOUR_OWN_RISK

I personally wouldn't object, but it's not a PCI thing so that can all
be separate from this driver.

> >> The one issue I anticipate with "depends on BROKEN" is
> >> when I add support for revision 2, which isn't broken.
> > 
> > How about this:
> > 
> >   - Rename PCIE_TANGO to PCIE_TANGO_REV1
> >   - PCIE_TANGO_REV1 depends on BROKEN
> >   - Add rev2 support later, enabled by PCIE_TANGO
> >   - PCIE_TANGO_REV1 depends on PCIE_TANGO && BROKEN
> > 
> > I updated pci/host-tango along these lines (without rev2 support,
> > obviously).
> 
> And support for REV1 wouldn't be compiled in, unless
> BROKEN is selected? Yes, I think that could fly.

Right.

> Don't you think the naming should follow the DT
> convention of using the first SoC embedding the
> IP (for the compatible string) ?
> 
> PCIE_TANGO_REV1 vs PCIE_TANGO_SMP8759

Sounds reasonable.  So v2 will be something other than SMP8759?
I renamed it to CONFIG_PCIE_TANGO_SMP8759.

> > I forgot to ask for a MAINTAINERS update.  Can you send that, too,
> > please?
> 
> There's a "catch-all" rule for everything tango-related:
> 
> ARM/TANGO ARCHITECTURE
> M:	Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> L:	linux-arm-kernel@lists.infradead.org
> S:	Maintained
> N:	tango
> 
> Is that enough?

Yep, sorry I didn't notice that.  That's enough for
scripts/get_maintainer.pl to work, which is what I'm looking for.

If you confirm that
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?h=pci/host-tango&id=d752a8b29345
works for you, I'll include it in my v4.13 pull request.

Bjorn

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

#1681902

On 05/07/2017 23:34, Bjorn Helgaas wrote:

> On Wed, Jul 05, 2017 at 10:39:19PM +0200, Mason wrote:
>
>> On 05/07/2017 20:03, Bjorn Helgaas wrote:
>>
>>> On Wed, Jul 05, 2017 at 12:55:37AM +0200, Mason wrote:
>>>
>>>> On 04/07/2017 22:24, Bjorn Helgaas wrote:
>>>>
>>>>> I made the trivial changes I mentioned, added a dependency on
>>>>> CONFIG_BROKEN (for the config/MMIO muxing issue), and put these on
>>>>> pci/host-tango.  I can't build or test this, so I probably broke
>>>>> something in the process.  I think the combination of the boot-time
>>>>> warning, the taint, and CONFIG_BROKEN is a reasonable amount of
>>>>> warning that a user should expect issues.
>>>>>
>>>>> Can you take a look and see if it works for you?
>>>>>
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/host-tango
>>>>
>>>> Thanks. I'll take it for a spin ASAP.
>>>>
>>>> TAINT_CRAP... Smirk. I didn't see that one in the docs:
>>>> https://www.kernel.org/doc/html/latest/admin-guide/tainted-kernels.html
>>>>
>>>> Oh wait... TAINT_CRAP is "C" => a staging driver has been loaded
>>>
>>> I wish it had a less pejorative, more descriptive name.  But it seems like
>>> the closest to this situation.
>>
>> Maybe it is not too late to submit a patch to Linus
>> renaming TAINT_CRAP?
>>
>> Here are a few candidates, off the top of my head:
>>
>> TAINT_STAGING
>> TAINT_STAGING_DRIVER
>> TAINT_BROKEN_HW
>> TAINT_BROKEN_HARDWARE
>> TAINT_USE_AT_YOUR_OWN_RISK
> 
> I personally wouldn't object, but it's not a PCI thing so that can all
> be separate from this driver.

Yes, of course. I was just asking for your (and anyone's)
opinion, as a Linux dev.

>>>> The one issue I anticipate with "depends on BROKEN" is
>>>> when I add support for revision 2, which isn't broken.
>>>
>>> How about this:
>>>
>>>   - Rename PCIE_TANGO to PCIE_TANGO_REV1
>>>   - PCIE_TANGO_REV1 depends on BROKEN
>>>   - Add rev2 support later, enabled by PCIE_TANGO
>>>   - PCIE_TANGO_REV1 depends on PCIE_TANGO && BROKEN
>>>
>>> I updated pci/host-tango along these lines (without rev2 support,
>>> obviously).
>>
>> And support for REV1 wouldn't be compiled in, unless
>> BROKEN is selected? Yes, I think that could fly.
> 
> Right.
> 
>> Don't you think the naming should follow the DT
>> convention of using the first SoC embedding the
>> IP (for the compatible string) ?
>>
>> PCIE_TANGO_REV1 vs PCIE_TANGO_SMP8759
> 
> Sounds reasonable.  So v2 will be something other than SMP8759?
> I renamed it to CONFIG_PCIE_TANGO_SMP8759.

Right, HW bugs are fixed in newer chips. Old chips rarely
get bug fixes, apparently.

> If you confirm that
> https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?h=pci/host-tango&id=d752a8b29345
> works for you, I'll include it in my v4.13 pull request.

There were a few nits I wanted to address:

- Since we added suppress_bind_attrs = true, probe()
can only be called at init, so I wanted to mark __init
all the probe functions, to save space.

- I left the definition of MSI_MAX in the wrong patch

- You put a pointer to the pdev in the struct tango_pcie.
I think this is redundant, since the pdev already has a
pointer to the struct, as drvdata.
So I wanted to change tango_msi_probe() to take a pdev
as argument (to make it more like an actual probe function)
and derive pcie from pdev, instead of the other way around.

Can I send you a patch series with these changes on Friday?

Regards.

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

#1682032

On Wed, Jul 05, 2017 at 11:59:33PM +0200, Mason wrote:
> On 05/07/2017 23:34, Bjorn Helgaas wrote:
> 
> > On Wed, Jul 05, 2017 at 10:39:19PM +0200, Mason wrote:
> >
> >> On 05/07/2017 20:03, Bjorn Helgaas wrote:

> > If you confirm that
> > https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/commit/?h=pci/host-tango&id=d752a8b29345
> > works for you, I'll include it in my v4.13 pull request.
> 
> There were a few nits I wanted to address:
> 
> - Since we added suppress_bind_attrs = true, probe()
> can only be called at init, so I wanted to mark __init
> all the probe functions, to save space.
> 
> - I left the definition of MSI_MAX in the wrong patch
> 
> - You put a pointer to the pdev in the struct tango_pcie.
> I think this is redundant, since the pdev already has a
> pointer to the struct, as drvdata.
> So I wanted to change tango_msi_probe() to take a pdev
> as argument (to make it more like an actual probe function)
> and derive pcie from pdev, instead of the other way around.

I don't think tango_msi_probe() is really a "probe" function.  It's
all part of the tango driver, and it's not claiming a separate piece
of hardware.  So I would keep the name and structure similar to these:

  advk_pcie_init_msi_irq_domain()
  nwl_pcie_init_msi_irq_domain()

BTW, those functions use irq_domain_add_linear(), while you are one of
the very few callers of irq_domain_create_linear().  Why the difference?
If your code does basically the same thing, it's very helpful to me if
it *looks* basically the same.

> Can I send you a patch series with these changes on Friday?

I was planning to ask Linus to pull my branch tomorrow or Friday
because I'm going on vacation next week and I don't want to leave
right after he pulls it.  So the sooner the better.

Bjorn

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

#1682382

On 06/07/2017 05:39, Bjorn Helgaas wrote:

> On Wed, Jul 05, 2017 at 11:59:33PM +0200, Mason wrote:
>
>> There were a few nits I wanted to address:
>>
>> - Since we added suppress_bind_attrs = true, probe()
>> can only be called at init, so I wanted to mark __init
>> all the probe functions, to save space.
>>
>> - I left the definition of MSI_MAX in the wrong patch
>>
>> - You put a pointer to the pdev in the struct tango_pcie.
>> I think this is redundant, since the pdev already has a
>> pointer to the struct, as drvdata.
>> So I wanted to change tango_msi_probe() to take a pdev
>> as argument (to make it more like an actual probe function)
>> and derive pcie from pdev, instead of the other way around.
> 
> I don't think tango_msi_probe() is really a "probe" function.  It's
> all part of the tango driver, and it's not claiming a separate piece
> of hardware.

I agree that tango_msi_probe() is not a probe function;
it is merely a piece of the actual probe function (static
with single call site). I split the probe function in two,
because it seemed to make sense at the time.

Perhaps it's better to inline tango_msi_probe? That would
avoid the issues of that function's name and parameters.

If you think it's better to keep the two pieces separate,
I can rename the MSI part to tango_msi_init() or some such.
But I'd like to avoid adding unnecessary fields to the struct.

>  So I would keep the name and structure similar to these:
> 
>   advk_pcie_init_msi_irq_domain()
>   nwl_pcie_init_msi_irq_domain()
> 
> BTW, those functions use irq_domain_add_linear(), while you are one of
> the very few callers of irq_domain_create_linear().  Why the difference?
> If your code does basically the same thing, it's very helpful to me if
> it *looks* basically the same.

It was a suggestion from Marc Z on 2017-03-23.

<QUOTE>
+ irq_dom = irq_domain_add_linear(NULL, MSI_COUNT, &msi_domain_ops, pcie);

Use irq_domain_create_linear, pass the same fwnode.
</QUOTE>

It seems odd to pass NULL as the first argument.
(As I had first done, when copying the Altera driver.)

>> Can I send you a patch series with these changes on Friday?
> 
> I was planning to ask Linus to pull my branch tomorrow or Friday
> because I'm going on vacation next week and I don't want to leave
> right after he pulls it.  So the sooner the better.

I'm not at the office today, but I'll do it first thing
tomorrow. If it works out, great. If I'm too late, well
there's 4.14 to look forward to.

Regards.

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

#1682404

On 06/07/17 13:26, Mason wrote:
> On 06/07/2017 05:39, Bjorn Helgaas wrote:
> 
>> On Wed, Jul 05, 2017 at 11:59:33PM +0200, Mason wrote:
>>
>>> There were a few nits I wanted to address:
>>>
>>> - Since we added suppress_bind_attrs = true, probe()
>>> can only be called at init, so I wanted to mark __init
>>> all the probe functions, to save space.
>>>
>>> - I left the definition of MSI_MAX in the wrong patch
>>>
>>> - You put a pointer to the pdev in the struct tango_pcie.
>>> I think this is redundant, since the pdev already has a
>>> pointer to the struct, as drvdata.
>>> So I wanted to change tango_msi_probe() to take a pdev
>>> as argument (to make it more like an actual probe function)
>>> and derive pcie from pdev, instead of the other way around.
>>
>> I don't think tango_msi_probe() is really a "probe" function.  It's
>> all part of the tango driver, and it's not claiming a separate piece
>> of hardware.
> 
> I agree that tango_msi_probe() is not a probe function;
> it is merely a piece of the actual probe function (static
> with single call site). I split the probe function in two,
> because it seemed to make sense at the time.
> 
> Perhaps it's better to inline tango_msi_probe? That would
> avoid the issues of that function's name and parameters.
> 
> If you think it's better to keep the two pieces separate,
> I can rename the MSI part to tango_msi_init() or some such.
> But I'd like to avoid adding unnecessary fields to the struct.
> 
>>  So I would keep the name and structure similar to these:
>>
>>   advk_pcie_init_msi_irq_domain()
>>   nwl_pcie_init_msi_irq_domain()
>>
>> BTW, those functions use irq_domain_add_linear(), while you are one of
>> the very few callers of irq_domain_create_linear().  Why the difference?
>> If your code does basically the same thing, it's very helpful to me if
>> it *looks* basically the same.

irq_domain_add_linear() can only take an of_node as the identifier for
the domain, while the _create_ variants use a fwnode. Given that an
of+node is also a fwnode, the former is now deprecated in favour of the
latter.

> 
> It was a suggestion from Marc Z on 2017-03-23.
> 
> <QUOTE>
> + irq_dom = irq_domain_add_linear(NULL, MSI_COUNT, &msi_domain_ops, pcie);
> 
> Use irq_domain_create_linear, pass the same fwnode.
> </QUOTE>
> 
> It seems odd to pass NULL as the first argument.
> (As I had first done, when copying the Altera driver.)

Indeed, as it creates a "default" domain, which is almost always wrong.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

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

#1682677

On Thu, Jul 06, 2017 at 02:26:44PM +0200, Mason wrote:
> On 06/07/2017 05:39, Bjorn Helgaas wrote:
> 
> > On Wed, Jul 05, 2017 at 11:59:33PM +0200, Mason wrote:
> >
> >> There were a few nits I wanted to address:
> >>
> >> - Since we added suppress_bind_attrs = true, probe()
> >> can only be called at init, so I wanted to mark __init
> >> all the probe functions, to save space.
> >>
> >> - I left the definition of MSI_MAX in the wrong patch

I moved this.

> >> - You put a pointer to the pdev in the struct tango_pcie.
> >> I think this is redundant, since the pdev already has a
> >> pointer to the struct, as drvdata.
> >> So I wanted to change tango_msi_probe() to take a pdev
> >> as argument (to make it more like an actual probe function)
> >> and derive pcie from pdev, instead of the other way around.
> > 
> > I don't think tango_msi_probe() is really a "probe" function.  It's
> > all part of the tango driver, and it's not claiming a separate piece
> > of hardware.
> 
> I agree that tango_msi_probe() is not a probe function;
> it is merely a piece of the actual probe function (static
> with single call site). I split the probe function in two,
> because it seemed to make sense at the time.
> 
> Perhaps it's better to inline tango_msi_probe? That would
> avoid the issues of that function's name and parameters.
> 
> If you think it's better to keep the two pieces separate,
> I can rename the MSI part to tango_msi_init() or some such.
> But I'd like to avoid adding unnecessary fields to the struct.

I think it's better to follow the structure of existing drivers unless
your hardware dictates a different model.  Same with adding fields to
the struct.  If you have a better way of doing it that works for all
the drivers, great -- but please change all the drivers to do it that
way.  If it's a matter of saving one pointer per system, by making the
code look different than other drivers, that's not so great.

> >  So I would keep the name and structure similar to these:
> > 
> >   advk_pcie_init_msi_irq_domain()
> >   nwl_pcie_init_msi_irq_domain()
> > 
> > BTW, those functions use irq_domain_add_linear(), while you are one of
> > the very few callers of irq_domain_create_linear().  Why the difference?
> > If your code does basically the same thing, it's very helpful to me if
> > it *looks* basically the same.
> 
> It was a suggestion from Marc Z on 2017-03-23.
> 
> <QUOTE>
> + irq_dom = irq_domain_add_linear(NULL, MSI_COUNT, &msi_domain_ops, pcie);
> 
> Use irq_domain_create_linear, pass the same fwnode.
> </QUOTE>
> 
> It seems odd to pass NULL as the first argument.
> (As I had first done, when copying the Altera driver.)

I'm a little queasy about the MSI stuff.  It doesn't feel very settled
yet, and I don't want to keep tweaking it at this stage.

How about we merge the base patch for v4.13 and deal with MSIs for
v4.14?  I need confirmation that the base patch works ASAP.  Or if
it's not really useful by itself, we can defer it all until v4.14.

For v4.14, I'd really like to see some unification of naming and
structure across the drivers in how they handle IRQ domains, and
then have tango follow whatever pattern that ends up being.

Right now we don't have much consistency in the names of legacy and
MSI IRQ domains, what device_node they're associated with, how we
handle the 0-3 vs 1-4 legacy numbering, pci_msi_create_irq_domain()
usage, etc.  Some of this may be dictated by different hardware
requirements, but I doubt all of it is.

Bjorn


P.S. Notes about current IRQ domain usage below, just for reference
about where I'm seeing inconsistencies.

    advk_pcie_probe			# "marvell,armada-3700-pcie"
      advk_pcie_init_irq_domain
        pcie_intc_node =  of_get_next_child(node, NULL)
        irq_domain_add_linear(pcie_intc_node, ...)
      advk_pcie_init_msi_irq_domain
        pcie->msi_inner_domain = irq_domain_add_linear(NULL, ...)
        pcie->msi_domain = pci_msi_create_irq_domain(...)

    altera_pcie_probe			# "altr,pcie-root-port-1.0"
      altera_pcie_init_irq_domain
        pcie->irq_domain = irq_domain_add_linear(node, ...)
    altera_msi_probe			# "altr,msi-1.0"
      altera_allocate_domains
        msi->inner_domain = irq_domain_add_linear(NULL, ...)
        msi->msi_domain = pci_msi_create_irq_domain(fwnode, ...)

    iproc_pcie_pltfm_probe		# "brcm,iproc-pcie", etc
      iproc_pcie_setup
        iproc_pcie_msi_enable
          iproc_msi_init
            iproc_msi_alloc_domains
              msi->inner_domain = irq_domain_add_linear(NULL, ...)
              msi->msi_domain = pci_msi_create_irq_domain(...)

    rcar_pcie_probe			# "renesas,pcie-r8a7779", etc
      rcar_pcie_enable_msi
        msi->domain = irq_domain_add_linear(dev->of_node, ...)

    rockchip_pcie_probe			# "rockchip,rk3399-pcie"
      rockchip_pcie_init_irq_domain
        intc = of_get_next_child(dev->of_node, NULL)
        rockchip->irq_domain = irq_domain_add_linear(intc, ...)
  
    xilinx_pcie_probe			# "xlnx,axi-pcie-host-1.00.a"
      xilinx_pcie_init_irq_domain
        pcie_intc_node = of_get_next_child(node, NULL)
        port->leg_domain = irq_domain_add_linear(pcie_intc_node, ...)
        port->msi_domain = irq_domain_add_linear(node, ...)

    nwl_pcie_probe			# "xlnx,nwl-pcie-2.11"
      nwl_pcie_init_irq_domain
        legacy_intc_node = of_get_next_child(node, NULL)
        pcie->legacy_irq_domain = irq_domain_add_linear(legacy_intc_node, ...)
        nwl_pcie_init_msi_irq_domain
          msi->dev_domain = irq_domain_add_linear(NULL, ...)
          msi->msi_domain = pci_msi_create_irq_domain(fwnode, ...)

    faraday_pci_probe			# "faraday,ftpci100", etc
      faraday_pci_setup_cascaded_irq
        intc = of_get_next_child(p->dev->of_node, NULL)
        p->irqdomain = irq_domain_add_linear(intc, ...)

    hv_pci_probe
      hv_pcie_init_irq_domain
        hbus->irq_domain = pci_msi_create_irq_domain(...)

    tegra_pcie_probe			# "nvidia,tegra210-pcie", etc
      tegra_pcie_enable_msi
        msi->domain = irq_domain_add_linear(dev->of_node, ...)

    xgene_pcie_probe_bridge		# "apm,xgene-pcie"
    xgene_msi_probe			# "apm,xgene1-msi"
      xgene_allocate_domains
        msi->inner_domain = irq_domain_add_linear(NULL, ...)
        msi->msi_domain = pci_msi_create_irq_domain(...)

    vmd_probe				# [8086:201d]
      vmd_enable_domain
        vmd->irq_domain = pci_msi_create_irq_domain(NULL, ...)

    dra7xx_pcie_probe			# "ti,dra7-pcie", etc
      dra7xx_add_pcie_port
        dra7xx_pcie_init_irq_domain
          pcie_intc_node =  of_get_next_child(node, NULL)
          dra7xx->irq_domain = irq_domain_add_linear(pcie_intc_node, ...)

    dw_pcie_host_init
      pp->ops->msi_host_init
        ks_dw_pcie_msi_host_init	# .msi_host_init
          pp->irq_domain = irq_domain_add_linear(ks_pcie->msi_intc_np, ...)

    ks_pcie_probe			# "ti,keystone-pcie"
      ks_add_pcie_port
        ks_dw_pcie_host_init
          ks_pcie->legacy_irq_domain = irq_domain_add_linear(ks_pcie->legacy_intc_np, ...)

    *_add_pcie_port
      dw_pcie_host_init
        pp->irq_domain = irq_domain_add_linear(dev->of_node, ...) # generic

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web