diff mbox

[v8,2/2] add new platform driver for PCI RC

Message ID c7af1603d4f18d11cb0ccee1c07708a1455ea4ed.1454600622.git.jpinto@synopsys.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Joao Pinto Feb. 4, 2016, 3:52 p.m. UTC
This patch adds a new driver that will be the reference platform driver
for all PCI RC IP Protoyping Kits based on ARC SDP.

Signed-off-by: Joao Pinto <jpinto@synopsys.com>
---
Change v7 -> v8 (Bjorn Helgaas and Arnd Bergmann):
- driver name was changed from pcie-synopsys to pcie-dw-pltfm
- mdelay() replaced for msleep() in the new driver
- Devicetree bindings for the new driver was updated (config space removed
from ranges)
- Unnecessary synopsys_pcie_irq_handler() was removed
- Driver compatibility strings updated
Change v6 -> v7 (Bjorn Helgaas):
- driver name was changed from pcie-snpsdev to pcie-synopsys
- driver internals (functions and certain variables) also changed name
accordingly
- devicetree bindings documentation also changed accordingly
- quirk function dw_pcie_link_retrain() was removed (tests were made
successfully without it)
- driver was changed to meet pci standards (link up confirmation routine,
init rc functions, etc.)
- EPROBE_DEFER were removed
Change v5 -> v6:
- Nothing changed (just to keep up with patch set version).
Change v4 -> v5:
- Nothing changed (just to keep up with patch set version).
Changes v3 -> v4 (Bjorn Helgaas):
- ARCH dependencies were added to the drivers/pci/host/kconfig for the
PCIE_SNPSDEV.
Changes v2 -> v3 (Bjorn Helgaas):
- link init stuff was moved to a snpsdev_pcie_establish_link() function in
pcie-snpsdev
- pcie-snpsdev driver declaration was changed to be more 
standard (Bjorn Helgaas)
- pcie-designware' dw_pcie_link_retrain() now use standard registers from
pci-regs.h (Bjorn Helgaas)
- pcie-snpsdev.txt was complemented with more info (Mark Rutland)
Changes v1 -> v2 (Bjorn Helgaas):
- Fixups snpsdev_pcie_fixup_bridge() and snpsdev_pcie_fixup_res() were removed
from the driver (these functions were for specific tests only and not usefull
in mainline)
- Driver' comments were reviewed (fix Typos and excessive comments removal)
- Removed unnecessary definitions in the driver source (PCIE_PHY_CTRL and
PCIE_PHY_STAT)

 .../devicetree/bindings/pci/pcie-dw-pltfm.txt      |  36 +++
 MAINTAINERS                                        |   7 +
 drivers/pci/host/Kconfig                           |   8 +
 drivers/pci/host/Makefile                          |   1 +
 drivers/pci/host/pcie-dw-pltfm.c                   | 244 +++++++++++++++++++++
 5 files changed, 296 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/pcie-dw-pltfm.txt
 create mode 100644 drivers/pci/host/pcie-dw-pltfm.c

Comments

Bjorn Helgaas Feb. 4, 2016, 6:19 p.m. UTC | #1
On Thu, Feb 04, 2016 at 03:52:10PM +0000, Joao Pinto wrote:
> This patch adds a new driver that will be the reference platform driver
> for all PCI RC IP Protoyping Kits based on ARC SDP.
> 
> Signed-off-by: Joao Pinto <jpinto@synopsys.com>
> ---
> Change v7 -> v8 (Bjorn Helgaas and Arnd Bergmann):
> - driver name was changed from pcie-synopsys to pcie-dw-pltfm

"pcie-dw-pltfm" seems worse to me.  We have eight existing drivers
that call dw_pcie_host_init(), and they're all platform_drivers.
"pcie-dw-pltfm" could apply equally well to any of them.

I think I see what happened: I wrote "It doesn't seem necessary to me
to include both 'synopsys' and 'ipk' in the filename and the driver
name."  I meant that using one of them should be sufficient, not that
*both* should be removed.

I don't know the SoC landscape, but from Arnd's comment, it sounds
like "synopsys" might be too generic because many of the other drivers
are connected with Synopsys.  I don't know what "ipk" means, but maybe
that could work.  It's convenient if the name *means* something, and
if "ipk" stands for "IP Prototyping Kit", that sounds pretty generic.
Is "haps" or "haps_dx" a name people would associate with this
hardware?  I guess it'd be nice if the driver name were related to the
DT compat strings, so "ipk" is better from that perspective.

My pci/host-synopsys branch (which is still published even though I
removed it from for-linus) contains your v7 series plus some fixes.
This v8 series lost the fixes.  For example:

> +static irqreturn_t dw_pltfm_pcie_msi_irq_handler(int irq, void *arg)
> +{
> +	struct pcie_port *pp = arg;
> +
> +	dw_handle_msi_irq(pp);
> +
> +	return dw_handle_msi_irq(pp);
> +}

This is a bug.  You should call dw_handle_msi_irq() *once* and return
the value it returns.  I already fixed this in my pci/host-synopsys
branch, so you should start with that, and then apply your v7-v8
changes.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joao Pinto Feb. 4, 2016, 6:31 p.m. UTC | #2
Hi Bjorn,

On 2/4/2016 6:19 PM, Bjorn Helgaas wrote:
> On Thu, Feb 04, 2016 at 03:52:10PM +0000, Joao Pinto wrote:
>> This patch adds a new driver that will be the reference platform driver
>> for all PCI RC IP Protoyping Kits based on ARC SDP.
>>
>> Signed-off-by: Joao Pinto <jpinto@synopsys.com>
>> ---
>> Change v7 -> v8 (Bjorn Helgaas and Arnd Bergmann):
>> - driver name was changed from pcie-synopsys to pcie-dw-pltfm
> 
> "pcie-dw-pltfm" seems worse to me.  We have eight existing drivers
> that call dw_pcie_host_init(), and they're all platform_drivers.
> "pcie-dw-pltfm" could apply equally well to any of them.
> 
> I think I see what happened: I wrote "It doesn't seem necessary to me
> to include both 'synopsys' and 'ipk' in the filename and the driver
> name."  I meant that using one of them should be sufficient, not that
> *both* should be removed.
> 
> I don't know the SoC landscape, but from Arnd's comment, it sounds
> like "synopsys" might be too generic because many of the other drivers
> are connected with Synopsys.  I don't know what "ipk" means, but maybe
> that could work.  It's convenient if the name *means* something, and
> if "ipk" stands for "IP Prototyping Kit", that sounds pretty generic.
> Is "haps" or "haps_dx" a name people would associate with this
> hardware?  I guess it'd be nice if the driver name were related to the
> DT compat strings, so "ipk" is better from that perspective.

Synopsys has a product called IP Prototyping Kit which is a bundle of HAPSDX +
PCIE RC IP + drivers + Development Board. This driver was implemented originally
to serve this IPK but it can be used by SoC that use the Synopsys PCIe RC IP.
"ipk" would say that the driver is usable only in the IP Prototyping Kits which
is not 100% true, it is usable in any SoC with Synopsys IP or in limit serve as
a base for specific SoC drivers.
Suggestions: "pcie-dw-soc-agnostic", "pcie-dw-ipk", "pcie-dw-haps-prototyping"

What do you think?

> 
> My pci/host-synopsys branch (which is still published even though I
> removed it from for-linus) contains your v7 series plus some fixes.
> This v8 series lost the fixes.  For example:
> 
>> +static irqreturn_t dw_pltfm_pcie_msi_irq_handler(int irq, void *arg)
>> +{
>> +	struct pcie_port *pp = arg;
>> +
>> +	dw_handle_msi_irq(pp);
>> +
>> +	return dw_handle_msi_irq(pp);
>> +}
> 
> This is a bug.  You should call dw_handle_msi_irq() *once* and return
> the value it returns.  I already fixed this in my pci/host-synopsys
> branch, so you should start with that, and then apply your v7-v8
> changes.

I am going to fetch your repo and get the host/pcie-synopsys to generate the
next patch version.

> 
> Bjorn
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas Feb. 4, 2016, 11:43 p.m. UTC | #3
On Thu, Feb 04, 2016 at 06:31:05PM +0000, Joao Pinto wrote:
> On 2/4/2016 6:19 PM, Bjorn Helgaas wrote:
> > On Thu, Feb 04, 2016 at 03:52:10PM +0000, Joao Pinto wrote:
> >> This patch adds a new driver that will be the reference platform driver
> >> for all PCI RC IP Protoyping Kits based on ARC SDP.
> >>
> >> Signed-off-by: Joao Pinto <jpinto@synopsys.com>
> >> ---
> >> Change v7 -> v8 (Bjorn Helgaas and Arnd Bergmann):
> >> - driver name was changed from pcie-synopsys to pcie-dw-pltfm
> > 
> > "pcie-dw-pltfm" seems worse to me.  We have eight existing drivers
> > that call dw_pcie_host_init(), and they're all platform_drivers.
> > "pcie-dw-pltfm" could apply equally well to any of them.
> > 
> > I think I see what happened: I wrote "It doesn't seem necessary to me
> > to include both 'synopsys' and 'ipk' in the filename and the driver
> > name."  I meant that using one of them should be sufficient, not that
> > *both* should be removed.
> > 
> > I don't know the SoC landscape, but from Arnd's comment, it sounds
> > like "synopsys" might be too generic because many of the other drivers
> > are connected with Synopsys.  I don't know what "ipk" means, but maybe
> > that could work.  It's convenient if the name *means* something, and
> > if "ipk" stands for "IP Prototyping Kit", that sounds pretty generic.
> > Is "haps" or "haps_dx" a name people would associate with this
> > hardware?  I guess it'd be nice if the driver name were related to the
> > DT compat strings, so "ipk" is better from that perspective.
> 
> Synopsys has a product called IP Prototyping Kit which is a bundle of
> HAPSDX + PCIE RC IP + drivers + Development Board. This driver was
> implemented originally to serve this IPK but it can be used by SoC that
> use the Synopsys PCIe RC IP.  "ipk" would say that the driver is usable
> only in the IP Prototyping Kits which is not 100% true, it is usable in
> any SoC with Synopsys IP or in limit serve as a base for specific SoC
> drivers.  Suggestions: "pcie-dw-soc-agnostic", "pcie-dw-ipk",
> "pcie-dw-haps-prototyping"
> 
> What do you think?

I don't think the "dw" part is relevant (none of the other
DesignWare-based drivers includes it in the driver or file name).

How do people typically refer to this board?

I really like "synopsys" because it fits the pattern of being
recognizable and pronounceable like "altera", "designware", "qcom",
"keystone", "layerscape", "tegra", etc.  But I can't tell whether it's
too generic.

"ipk" or "haps" would be fine with me.  I think it's OK if it doesn't
cover 100% of the possible systems.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joao Pinto Feb. 5, 2016, 10:44 a.m. UTC | #4
Hi,

On 2/4/2016 11:43 PM, Bjorn Helgaas wrote:
>> What do you think?
> 
> I don't think the "dw" part is relevant (none of the other
> DesignWare-based drivers includes it in the driver or file name).
> 
> How do people typically refer to this board?
> 
> I really like "synopsys" because it fits the pattern of being
> recognizable and pronounceable like "altera", "designware", "qcom",
> "keystone", "layerscape", "tegra", etc.  But I can't tell whether it's
> too generic.
> 
> "ipk" or "haps" would be fine with me.  I think it's OK if it doesn't
> cover 100% of the possible systems.

I think we should follow the iproc example: pcie-iproc-platform.c
In this case we would have pcie-designware-platform.c
I think this would be the best name because the driver is a non soc specific
designware platform driver.

Arnd and Bjorn agree on this name?

> 
> Bjorn
> 

Joao

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Feb. 5, 2016, 2:39 p.m. UTC | #5
On Friday 05 February 2016 10:44:29 Joao Pinto wrote:
> Hi,
> 
> On 2/4/2016 11:43 PM, Bjorn Helgaas wrote:
> >> What do you think?
> > 
> > I don't think the "dw" part is relevant (none of the other
> > DesignWare-based drivers includes it in the driver or file name).
> > 
> > How do people typically refer to this board?
> > 
> > I really like "synopsys" because it fits the pattern of being
> > recognizable and pronounceable like "altera", "designware", "qcom",
> > "keystone", "layerscape", "tegra", etc.  But I can't tell whether it's
> > too generic.
> > 
> > "ipk" or "haps" would be fine with me.  I think it's OK if it doesn't
> > cover 100% of the possible systems.
> 
> I think we should follow the iproc example: pcie-iproc-platform.c
> In this case we would have pcie-designware-platform.c
> I think this would be the best name because the driver is a non soc specific
> designware platform driver.
> 
> Arnd and Bjorn agree on this name?

Sorry, I did not realize that your submission was for the generic dw-pcie
implementation rather than a particular product integrating it.

I think in this case, we should do this completely differently:

How about putting all the new code into drivers/pci/host/pcie-designware.c
as functions that can be used by the other drivers in absence of a chip
specific handler?

Instead of providing a new instance of struct pcie_host_ops, maybe add
it as a default implementation in dw_pcie_link_up() and dw_pcie_host_init()
for drivers that don't provide their own. "hisi_pcie_host_ops" currently
provides no host_init() callback function, so you will have to change
the hisi frontend to a provide nop-function.

For all other drivers, check if they can be changed to use your generic
implementation and remove their private callbacks if possible.

I think the MSI implementation should be split out into a separate file
though, as not everyone uses this.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joao Pinto Feb. 5, 2016, 2:51 p.m. UTC | #6
On 2/5/2016 2:39 PM, Arnd Bergmann wrote:
> On Friday 05 February 2016 10:44:29 Joao Pinto wrote:
>> Hi,
>>
>> On 2/4/2016 11:43 PM, Bjorn Helgaas wrote:
>>>> What do you think?
>>>
>>> I don't think the "dw" part is relevant (none of the other
>>> DesignWare-based drivers includes it in the driver or file name).
>>>
>>> How do people typically refer to this board?
>>>
>>> I really like "synopsys" because it fits the pattern of being
>>> recognizable and pronounceable like "altera", "designware", "qcom",
>>> "keystone", "layerscape", "tegra", etc.  But I can't tell whether it's
>>> too generic.
>>>
>>> "ipk" or "haps" would be fine with me.  I think it's OK if it doesn't
>>> cover 100% of the possible systems.
>>
>> I think we should follow the iproc example: pcie-iproc-platform.c
>> In this case we would have pcie-designware-platform.c
>> I think this would be the best name because the driver is a non soc specific
>> designware platform driver.
>>
>> Arnd and Bjorn agree on this name?
> 
> Sorry, I did not realize that your submission was for the generic dw-pcie
> implementation rather than a particular product integrating it.
> 

It is a driver that is useful for PCIe RC prototyping and to be a reference
platform driver for DesignWare PCIe RC, I don't know if merging some of the
driver's code into pcie-designware is really necessary depends on the usefulness
of it. I would suggest that bigger step to be done in a 2nd stage since I will
be around to maintain what's necessary. Agree?

I made a patch that is applicable to Bjorn's host/pcie-synopsys that tries to
match your suggestions and Bjorn's. Could you please comment check it?

> I think in this case, we should do this completely differently:
> 
> How about putting all the new code into drivers/pci/host/pcie-designware.c
> as functions that can be used by the other drivers in absence of a chip
> specific handler?
> 
> Instead of providing a new instance of struct pcie_host_ops, maybe add
> it as a default implementation in dw_pcie_link_up() and dw_pcie_host_init()
> for drivers that don't provide their own. "hisi_pcie_host_ops" currently
> provides no host_init() callback function, so you will have to change
> the hisi frontend to a provide nop-function.
> 
> For all other drivers, check if they can be changed to use your generic
> implementation and remove their private callbacks if possible.
> 
> I think the MSI implementation should be split out into a separate file
> though, as not everyone uses this.
> 
> 	Arnd
> 

Joao


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Feb. 5, 2016, 3:43 p.m. UTC | #7
On Friday 05 February 2016 14:51:39 Joao Pinto wrote:
> 
> It is a driver that is useful for PCIe RC prototyping and to be a reference
> platform driver for DesignWare PCIe RC, I don't know if merging some of the
> driver's code into pcie-designware is really necessary depends on the usefulness
> of it. I would suggest that bigger step to be done in a 2nd stage since I will
> be around to maintain what's necessary. Agree?
> 
> I made a patch that is applicable to Bjorn's host/pcie-synopsys that tries to
> match your suggestions and Bjorn's. Could you please comment check it?

I think it would be useful to do this as generic as possible, and you seem
to be the right person to integrate it into the generic driver as you have
access to the hardware documents. Normally the folks writing the drivers
are just guessing based on what little information they get out of manuals
or vendor-provided drivers, but I'm sure that not all of that makes sense.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joao Pinto Feb. 5, 2016, 3:50 p.m. UTC | #8
On 2/5/2016 3:43 PM, Arnd Bergmann wrote:
> On Friday 05 February 2016 14:51:39 Joao Pinto wrote:
>>
>> It is a driver that is useful for PCIe RC prototyping and to be a reference
>> platform driver for DesignWare PCIe RC, I don't know if merging some of the
>> driver's code into pcie-designware is really necessary depends on the usefulness
>> of it. I would suggest that bigger step to be done in a 2nd stage since I will
>> be around to maintain what's necessary. Agree?
>>
>> I made a patch that is applicable to Bjorn's host/pcie-synopsys that tries to
>> match your suggestions and Bjorn's. Could you please comment check it?
> 
> I think it would be useful to do this as generic as possible, and you seem
> to be the right person to integrate it into the generic driver as you have
> access to the hardware documents. Normally the folks writing the drivers
> are just guessing based on what little information they get out of manuals
> or vendor-provided drivers, but I'm sure that not all of that makes sense.

Arnd, sure and I would love to be useful and improve the dw pci ecosystem, but
maybe now we should submit the driver as suggested in "[PATCH] synopsys pcie rc
generic platform driver update" and then make in a future intervention.

Bjorn, could you please comment? Should we go now with this driver version and
in the future we make the changes suggested by Arnd?

> 
> 	Arnd
> 

Joao

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas Feb. 5, 2016, 11:32 p.m. UTC | #9
On Fri, Feb 05, 2016 at 03:39:05PM +0100, Arnd Bergmann wrote:
> On Friday 05 February 2016 10:44:29 Joao Pinto wrote:
> > Hi,
> > 
> > On 2/4/2016 11:43 PM, Bjorn Helgaas wrote:
> > >> What do you think?
> > > 
> > > I don't think the "dw" part is relevant (none of the other
> > > DesignWare-based drivers includes it in the driver or file name).
> > > 
> > > How do people typically refer to this board?
> > > 
> > > I really like "synopsys" because it fits the pattern of being
> > > recognizable and pronounceable like "altera", "designware", "qcom",
> > > "keystone", "layerscape", "tegra", etc.  But I can't tell whether it's
> > > too generic.
> > > 
> > > "ipk" or "haps" would be fine with me.  I think it's OK if it doesn't
> > > cover 100% of the possible systems.
> > 
> > I think we should follow the iproc example: pcie-iproc-platform.c
> > In this case we would have pcie-designware-platform.c
> > I think this would be the best name because the driver is a non soc specific
> > designware platform driver.
> > 
> > Arnd and Bjorn agree on this name?
> 
> Sorry, I did not realize that your submission was for the generic dw-pcie
> implementation rather than a particular product integrating it.
> 
> I think in this case, we should do this completely differently:
> 
> How about putting all the new code into drivers/pci/host/pcie-designware.c
> as functions that can be used by the other drivers in absence of a chip
> specific handler?
> 
> Instead of providing a new instance of struct pcie_host_ops, maybe add
> it as a default implementation in dw_pcie_link_up() and dw_pcie_host_init()
> for drivers that don't provide their own. "hisi_pcie_host_ops" currently
> provides no host_init() callback function, so you will have to change
> the hisi frontend to a provide nop-function.
> 
> For all other drivers, check if they can be changed to use your generic
> implementation and remove their private callbacks if possible.
> 
> I think the MSI implementation should be split out into a separate file
> though, as not everyone uses this.

I'm not sure I understand what you're proposing, Arnd, so let me
ramble and you can direct me back on course.

Currently drivers/pci/host/pcie-designware.c is not usable by itself;
it doesn't register a platform_driver.

There's hardly any code in Joao's patches; it looks like they add a
minimal wrapper around the functionality in pcie-designware.c and
register it as a platform_driver.

Are you suggesting that we should just add that functionality directly
in pcie-designware.c so that file could both be a minimal driver with
the functionality of Joao's patches, *and* continue to provide the
shared code used by all the existing DesignWare-based drivers?  Maybe
the platform_driver registration part could be controlled by its own
separate Kconfig option.

For example, he could make dw_pcie_link_up() look like:

  int dw_pcie_link_up(struct pcie_port *pp)
  {
    u32 val;

    if (pp->ops->link_up)
      return pp->ops->link_up(pp);

    val = readl(pp->dbi_base + PCIE_PHY_DEBUG_R1);
    return val & PCIE_PHY_DEBUG_R1_LINK_UP;
  }

That seems like it would make sense to me.  It would resolve the
filename question, since there wouldn't be a new file.  And if this is
merely a driver for the generic DesignWare core without any
extensions, I'm happy with some sort of "dw"-based driver name and
compatibility string.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Feb. 8, 2016, 12:31 p.m. UTC | #10
On Friday 05 February 2016 17:32:48 Bjorn Helgaas wrote:
> On Fri, Feb 05, 2016 at 03:39:05PM +0100, Arnd Bergmann wrote:
> > On Friday 05 February 2016 10:44:29 Joao Pinto wrote:

> > I think in this case, we should do this completely differently:
> > 
> > How about putting all the new code into drivers/pci/host/pcie-designware.c
> > as functions that can be used by the other drivers in absence of a chip
> > specific handler?
> > 
> > Instead of providing a new instance of struct pcie_host_ops, maybe add
> > it as a default implementation in dw_pcie_link_up() and dw_pcie_host_init()
> > for drivers that don't provide their own. "hisi_pcie_host_ops" currently
> > provides no host_init() callback function, so you will have to change
> > the hisi frontend to a provide nop-function.
> > 
> > For all other drivers, check if they can be changed to use your generic
> > implementation and remove their private callbacks if possible.
> > 
> > I think the MSI implementation should be split out into a separate file
> > though, as not everyone uses this.
> 
> I'm not sure I understand what you're proposing, Arnd, so let me
> ramble and you can direct me back on course.
> 
> Currently drivers/pci/host/pcie-designware.c is not usable by itself;
> it doesn't register a platform_driver.
> 
> There's hardly any code in Joao's patches; it looks like they add a
> minimal wrapper around the functionality in pcie-designware.c and
> register it as a platform_driver.
> 
> Are you suggesting that we should just add that functionality directly
> in pcie-designware.c so that file could both be a minimal driver with
> the functionality of Joao's patches, *and* continue to provide the
> shared code used by all the existing DesignWare-based drivers?  Maybe
> the platform_driver registration part could be controlled by its own
> separate Kconfig option.

Either way is fine, we just have to be a little careful about the
initialization ordering.

> For example, he could make dw_pcie_link_up() look like:
> 
>   int dw_pcie_link_up(struct pcie_port *pp)
>   {
>     u32 val;
> 
>     if (pp->ops->link_up)
>       return pp->ops->link_up(pp);
> 
>     val = readl(pp->dbi_base + PCIE_PHY_DEBUG_R1);
>     return val & PCIE_PHY_DEBUG_R1_LINK_UP;
>   }

This is definitely good (after checking that all existing drivers
either work with the generic version, or provide their own callbacks
already).

> That seems like it would make sense to me.  It would resolve the
> filename question, since there wouldn't be a new file.  And if this is
> merely a driver for the generic DesignWare core without any
> extensions, I'm happy with some sort of "dw"-based driver name and
> compatibility string.

The important part I think is that the new driver should not require
and code that is seen as soc-specific: If it works with any implementation
of pci-dw rather than a specific system, the driver should know how
to do the right thing.

It may be helpful to move the actual matching on the compatible string
and calling of the generic probe function into another module, if we
are going forward with loadable PCI host drivers as posted by 
Paul Gortmaker today. Otherwise we end up with a device being bound
to the generic driver when a more specific one exists and both
are loadable modules, because the generic driver is always loaded
first.  As long as both drivers are built-in, it works fine because
we first look for a driver matching the most specific compatible string.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joao Pinto Feb. 8, 2016, 12:52 p.m. UTC | #11
Hi Bjorn and Arnd,

I agree with you in adding the platform register mechanism to the
pcie-designware core driver. This feature is excellent for who wants to quickly
have a taste of a pcie-designware platform drive for their Synopsys PCIe RC
controller.

Do you have some example for me to check in order to follow the same logic.
Adding something like this in pcie-designware:

#ifdef CONFIG_PCIE_DW_PLAT
<add compatibility and platform register code>
#endif

would be acceptable? maybe the platform tweeks should be external makeing it
cleaner like Arnd suggested. An example to follow would be nice.

Thanks,
Joao

On 2/8/2016 12:31 PM, Arnd Bergmann wrote:
> On Friday 05 February 2016 17:32:48 Bjorn Helgaas wrote:
>> On Fri, Feb 05, 2016 at 03:39:05PM +0100, Arnd Bergmann wrote:
>>> On Friday 05 February 2016 10:44:29 Joao Pinto wrote:
> 
>>> I think in this case, we should do this completely differently:
>>>
>>> How about putting all the new code into drivers/pci/host/pcie-designware.c
>>> as functions that can be used by the other drivers in absence of a chip
>>> specific handler?
>>>
>>> Instead of providing a new instance of struct pcie_host_ops, maybe add
>>> it as a default implementation in dw_pcie_link_up() and dw_pcie_host_init()
>>> for drivers that don't provide their own. "hisi_pcie_host_ops" currently
>>> provides no host_init() callback function, so you will have to change
>>> the hisi frontend to a provide nop-function.
>>>
>>> For all other drivers, check if they can be changed to use your generic
>>> implementation and remove their private callbacks if possible.
>>>
>>> I think the MSI implementation should be split out into a separate file
>>> though, as not everyone uses this.
>>
>> I'm not sure I understand what you're proposing, Arnd, so let me
>> ramble and you can direct me back on course.
>>
>> Currently drivers/pci/host/pcie-designware.c is not usable by itself;
>> it doesn't register a platform_driver.
>>
>> There's hardly any code in Joao's patches; it looks like they add a
>> minimal wrapper around the functionality in pcie-designware.c and
>> register it as a platform_driver.
>>
>> Are you suggesting that we should just add that functionality directly
>> in pcie-designware.c so that file could both be a minimal driver with
>> the functionality of Joao's patches, *and* continue to provide the
>> shared code used by all the existing DesignWare-based drivers?  Maybe
>> the platform_driver registration part could be controlled by its own
>> separate Kconfig option.
> 
> Either way is fine, we just have to be a little careful about the
> initialization ordering.
> 
>> For example, he could make dw_pcie_link_up() look like:
>>
>>   int dw_pcie_link_up(struct pcie_port *pp)
>>   {
>>     u32 val;
>>
>>     if (pp->ops->link_up)
>>       return pp->ops->link_up(pp);
>>
>>     val = readl(pp->dbi_base + PCIE_PHY_DEBUG_R1);
>>     return val & PCIE_PHY_DEBUG_R1_LINK_UP;
>>   }
> 
> This is definitely good (after checking that all existing drivers
> either work with the generic version, or provide their own callbacks
> already).
> 
>> That seems like it would make sense to me.  It would resolve the
>> filename question, since there wouldn't be a new file.  And if this is
>> merely a driver for the generic DesignWare core without any
>> extensions, I'm happy with some sort of "dw"-based driver name and
>> compatibility string.
> 
> The important part I think is that the new driver should not require
> and code that is seen as soc-specific: If it works with any implementation
> of pci-dw rather than a specific system, the driver should know how
> to do the right thing.
> 
> It may be helpful to move the actual matching on the compatible string
> and calling of the generic probe function into another module, if we
> are going forward with loadable PCI host drivers as posted by 
> Paul Gortmaker today. Otherwise we end up with a device being bound
> to the generic driver when a more specific one exists and both
> are loadable modules, because the generic driver is always loaded
> first.  As long as both drivers are built-in, it works fine because
> we first look for a driver matching the most specific compatible string.
> 
> 	Arnd
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/pci/pcie-dw-pltfm.txt b/Documentation/devicetree/bindings/pci/pcie-dw-pltfm.txt
new file mode 100644
index 0000000..ee2a877
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/pcie-dw-pltfm.txt
@@ -0,0 +1,36 @@ 
+Synopsys PCI RC IP Prototyping Kit
+----------------------------------
+
+This is the reference platform driver to be used in the Synopsys PCI Root
+Complex IP Prototyping Kit.
+
+Required properties:
+- compatible: set to "snps,dw-pcie" or "snps,ipk-pcie";
+- reg: base address and length of the pcie controller registers and
+configuration address space
+- reg-names: Must be "config" for the PCIe configuration space.
+- #address-cells: set to <3>
+- #size-cells: set to <2>
+- device_type: set to "pci"
+- ranges: ranges for the PCI memory and I/O regions.
+- interrupts: one interrupt source for MSI interrupts, followed by interrupt
+	source for hardware related interrupts.
+- #interrupt-cells: set to <1>
+- num-lanes: set to <1>;
+
+Example configuration:
+
+	pcie: pcie@0xdffff000 {
+		compatible = "snps,dw-pcie";
+		reg = <0xdffff000 0x1000
+		       0xd0000000 0x2000>;
+		reg-names = "csr", "config";
+		#address-cells = <3>;
+		#size-cells = <2>;
+		device_type = "pci";
+		ranges = <0x81000000 0 0x00000000 0xde000000 0 0x00010000
+			  0x82000000 0 0xd0400000 0xd0400000 0 0x0d000000>;
+		interrupts = <25>, <24>;
+		#interrupt-cells = <1>;
+		num-lanes = <1>;
+	};
diff --git a/MAINTAINERS b/MAINTAINERS
index e9caa4b..8362189 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8230,6 +8230,13 @@  S:	Maintained
 F:	Documentation/devicetree/bindings/pci/hisilicon-pcie.txt
 F:	drivers/pci/host/pcie-hisi.c
 
+PCI DRIVER FOR SYNOPSYS PROTOTYPING DEVICE
+M:	Joao Pinto <jpinto@synopsys.com>
+L:	linux-pci@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/pci/pcie-dw-pltfm.txt
+F:	drivers/pci/host/pcie-dw-pltfm.c
+
 PCMCIA SUBSYSTEM
 P:	Linux PCMCIA Team
 L:	linux-pcmcia@lists.infradead.org
diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index f131ba9..61cdc72 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -172,4 +172,12 @@  config PCI_HISI
 	help
 	  Say Y here if you want PCIe controller support on HiSilicon HIP05 SoC
 
+config PCIE_DW_PLAT
+	bool "Platform Driver for Synopsys PCIe controller"
+	depends on ARC && OF
+	select PCIEPORTBUS
+	select PCIE_DW
+	help
+	  Say Y here if you want to enable Synopsys PCIe controller platform
+	  driver.
 endmenu
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 9d4d3c6..8c84ba4 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -20,3 +20,4 @@  obj-$(CONFIG_PCIE_IPROC_BCMA) += pcie-iproc-bcma.o
 obj-$(CONFIG_PCIE_ALTERA) += pcie-altera.o
 obj-$(CONFIG_PCIE_ALTERA_MSI) += pcie-altera-msi.o
 obj-$(CONFIG_PCI_HISI) += pcie-hisi.o
+obj-$(CONFIG_PCIE_DW_PLAT) += pcie-dw-pltfm.o
diff --git a/drivers/pci/host/pcie-dw-pltfm.c b/drivers/pci/host/pcie-dw-pltfm.c
new file mode 100644
index 0000000..0924ec4
--- /dev/null
+++ b/drivers/pci/host/pcie-dw-pltfm.c
@@ -0,0 +1,244 @@ 
+/*
+ * PCIe RC driver for Synopsys Designware Core
+ *
+ * Copyright (C) 2015-2016 Synopsys, Inc. (www.synopsys.com)
+ *
+ * Authors: Manjunath Bettegowda <manjumb@synopsys.com>,
+ *	    Jie Deng <jiedeng@synopsys.com>
+ *	    Joao Pinto <jpinto@synopsys.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_gpio.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/resource.h>
+#include <linux/signal.h>
+#include <linux/types.h>
+
+#include "pcie-designware.h"
+
+#define to_dw_pltfm_pcie(x)	container_of(x, struct dw_pltfm_pcie, pp)
+
+struct dw_pltfm_pcie {
+	void __iomem		*mem_base; /* Memory Base to access Core's [RC]
+					    * Config Space Layout
+					    */
+	struct pcie_port	pp;        /* RC Root Port specific structure -
+					    * DWC_PCIE_RC stuff
+					    */
+};
+
+#define PCI_EQUAL_CONTROL_PHY 0x00000707
+#define PCIE_PHY_DEBUG_R1_LINK_UP 0x00000010
+
+/* PCIe Port Logic registers (memory-mapped) */
+#define PLR_OFFSET 0x700
+#define PCIE_PHY_DEBUG_R0 (PLR_OFFSET + 0x28) /* 0x728 */
+#define PCIE_PHY_DEBUG_R1 (PLR_OFFSET + 0x2c) /* 0x72c */
+
+static irqreturn_t dw_pltfm_pcie_msi_irq_handler(int irq, void *arg)
+{
+	struct pcie_port *pp = arg;
+
+	dw_handle_msi_irq(pp);
+
+	return dw_handle_msi_irq(pp);
+}
+
+static void dw_pltfm_pcie_init_phy(struct pcie_port *pp)
+{
+	/* write Lane 0 Equalization Control fields register */
+	writel(PCI_EQUAL_CONTROL_PHY, pp->dbi_base + 0x154);
+}
+
+static int dw_pltfm_pcie_deassert_core_reset(struct pcie_port *pp)
+{
+	return 0;
+}
+
+static void dw_pltfm_pcie_establish_link(struct pcie_port *pp)
+{
+	int retries = 0;
+
+	/* check if the link is up or not */
+	for (retries = 0; retries < 10; retries++) {
+		if (dw_pcie_link_up(pp)) {
+			dev_info(pp->dev, "Link up\n");
+			return;
+		}
+		msleep(100);
+	}
+}
+
+/*
+ * dw_pltfm_pcie_host_init()
+ * Platform specific host/RC initialization
+ *	a. Assert the core reset
+ *	b. Assert and deassert phy reset and initialize the phy
+ *	c. De-Assert the core reset
+ *	d. Initializet the Root Port (BARs/Memory Or IO/ Interrupt/ Commnad Reg)
+ *	e. Initiate Link startup procedure
+ *
+ */
+static void dw_pltfm_pcie_host_init(struct pcie_port *pp)
+{
+	/* Initialize Phy (Reset/poweron/control-inputs ) */
+	dw_pltfm_pcie_init_phy(pp);
+
+	dw_pltfm_pcie_deassert_core_reset(pp);
+
+	dw_pcie_setup_rc(pp);
+
+	dw_pltfm_pcie_establish_link(pp);
+
+	if (IS_ENABLED(CONFIG_PCI_MSI))
+		dw_pcie_msi_init(pp);
+}
+
+static int dw_pltfm_pcie_link_up(struct pcie_port *pp)
+{
+	u32 val;
+
+	val = readl(pp->dbi_base + PCIE_PHY_DEBUG_R1);
+	return val & PCIE_PHY_DEBUG_R1_LINK_UP;
+}
+
+/**
+ * This is RC operation structure
+ * dw_pltfm_pcie_link_up: the function which initiates the phy link up procedure
+ * dw_pltfm_pcie_host_init: the function which does the host/RC Root port
+ * initialization.
+ */
+static struct pcie_host_ops dw_pltfm_pcie_host_ops = {
+	.link_up = dw_pltfm_pcie_link_up,
+	.host_init = dw_pltfm_pcie_host_init,
+};
+
+/**
+ * dw_pltfm_add_pcie_port
+ * This function
+ * a. installs the interrupt handler
+ * b. registers host operations in the pcie_port structure
+ */
+static int dw_pltfm_add_pcie_port(struct pcie_port *pp,
+				 struct platform_device *pdev)
+{
+	int ret;
+
+	pp->irq = platform_get_irq(pdev, 1);
+
+	if (pp->irq < 0)
+		return pp->irq;
+
+	if (IS_ENABLED(CONFIG_PCI_MSI)) {
+		pp->msi_irq = platform_get_irq(pdev, 0);
+
+		if (pp->msi_irq < 0)
+			return pp->msi_irq;
+
+		ret = devm_request_irq(&pdev->dev, pp->msi_irq,
+					dw_pltfm_pcie_msi_irq_handler,
+					IRQF_SHARED, "dw-pltfm-pcie-msi", pp);
+		if (ret) {
+			dev_err(&pdev->dev, "failed to request msi irq\n");
+			return ret;
+		}
+	}
+
+	pp->root_bus_nr = -1;
+	pp->ops = &dw_pltfm_pcie_host_ops;
+
+	/* Below function:
+	 * Checks for range property from DT
+	 * Gets the IO and MEMORY and CONFIG-Space ranges from DT
+	 * Does IOREMAPS on the physical addresses
+	 * Gets the num-lanes from DT
+	 * Gets MSI capability from DT
+	 * Calls the platform specific host initialization
+	 * Program the correct class, BAR0, Link width, in Config space
+	 * Then it calls pci common init routine
+	 * Then it calls function to assign "unassigned resources"
+	 */
+	ret = dw_pcie_host_init(pp);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to initialize host\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+/**
+ * dw_pltfm_pcie_probe()
+ * This function gets called as part of pcie registration. if the id matches
+ * the platform driver framework will call this function.
+ *
+ * @pdev: Pointer to the platform_device structure
+ *
+ * Returns zero on success; Negative errorno on failure
+ */
+static int dw_pltfm_pcie_probe(struct platform_device *pdev)
+{
+	struct dw_pltfm_pcie *dw_pltfm_pcie;
+	struct pcie_port *pp;
+	struct resource *res;  /* Resource from DT */
+	int ret;
+
+	dw_pltfm_pcie = devm_kzalloc(&pdev->dev, sizeof(*dw_pltfm_pcie),
+					GFP_KERNEL);
+	if (!dw_pltfm_pcie)
+		return -ENOMEM;
+
+	pp = &dw_pltfm_pcie->pp;
+	pp->dev = &pdev->dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	if (!res)
+		return -ENODEV;
+
+	dw_pltfm_pcie->mem_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(dw_pltfm_pcie->mem_base))
+		return PTR_ERR(dw_pltfm_pcie->mem_base);
+
+	pp->dbi_base = dw_pltfm_pcie->mem_base;
+
+	ret = dw_pltfm_add_pcie_port(pp, pdev);
+	if (ret < 0)
+		return ret;
+
+	platform_set_drvdata(pdev, dw_pltfm_pcie);
+
+	return 0;
+}
+
+static const struct of_device_id dw_pltfm_pcie_of_match[] = {
+	{ .compatible = "snps,dw-pcie", },
+	{ .compatible = "snps,ipk-pcie", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, dw_pltfm_pcie_of_match);
+
+static struct platform_driver dw_pltfm_pcie_driver = {
+	.driver = {
+		.name	= "dw-pcie",
+		.of_match_table = dw_pltfm_pcie_of_match,
+	},
+	.probe = dw_pltfm_pcie_probe,
+};
+
+module_platform_driver(dw_pltfm_pcie_driver);
+
+MODULE_AUTHOR("Manjunath Bettegowda <manjumb@synopsys.com>");
+MODULE_DESCRIPTION("Synopsys PCIe host controller driver");
+MODULE_LICENSE("GPL v2");