diff mbox

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

Message ID 10b29adcdb0a31eb8f8071b271da267e44ad8a04.1454349430.git.jpinto@synopsys.com
State Superseded
Headers show

Commit Message

Joao Pinto Feb. 1, 2016, 6:07 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.
This patch is composed by:

-MAINTAINERS file was updated to include the new driver
-Documentation/devicetree/bindings/pci was updated to include the new
driver documentation
-New driver called pcie-synopsys

Signed-off-by: Joao Pinto <jpinto@synopsys.com>
---
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-synopsys.txt      |  33 +++
 MAINTAINERS                                        |   7 +
 drivers/pci/host/Kconfig                           |   8 +
 drivers/pci/host/Makefile                          |   1 +
 drivers/pci/host/pcie-synopsys.c                   | 257 +++++++++++++++++++++
 5 files changed, 306 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/pcie-synopsys.txt
 create mode 100644 drivers/pci/host/pcie-synopsys.c

Comments

Arnd Bergmann Feb. 2, 2016, 8:25 p.m. UTC | #1
On Monday 01 February 2016 18:07:45 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.
> This patch is composed by:
> 
> -MAINTAINERS file was updated to include the new driver
> -Documentation/devicetree/bindings/pci was updated to include the new
> driver documentation
> -New driver called pcie-synopsys
> 
> Signed-off-by: Joao Pinto <jpinto@synopsys.com>

I must have completely missed all the earlier submissions and just happened
to see this one now that it was merged.

I have a couple of comments, maybe you can send a follow up patch to address
them:

> +----------------------------------
> +
> +This is the reference platform driver to be used in the Synopsys PCI Root
> +Complex IP Prototyping Kit.
> +
> +Required properties:
> +- compatible: set to "snps,pcie-synopsys";

Please also include the exact version of the designware PCIe part that
is used in here, such as "snps,dw-pcie-1.234". I'm sure you can find
out the version.

"snps,pcie-synopsys" by itself is about the worst possible compatible
string because it says nothing about the specific hardware. Half the
machines we support all have a designware PCIe host that was made
by synopsys. Please use the name of the SoC for the most specific
string.

> +- reg: base address and length of the pcie controller registers.
> +- #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,pcie-synopsys";
> +		reg = <0xdffff000 0x1000>;
> +		#address-cells = <3>;
> +		#size-cells = <2>;
> +		device_type = "pci";
> +		ranges = <0x00000800 0 0xd0000000 0xd0000000 0 0x00002000>,

This line looks misplaced here and does not match the documentation. It's
probably a leftover from an earlier version that had the config space
in the ranges, so you need to remove that.

> +/* This handler was created for future work */
> +static irqreturn_t synopsys_pcie_irq_handler(int irq, void *arg)
> +{
> +	return IRQ_NONE;
> +}

It's not harmful, but we generally don't add code "for future work",
better remove it for now.

> +static void synopsys_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;
> +		}
> +		mdelay(100);
> +	}
> +}

That mdelay() needs to be an msleep(). You should never waste
a whole second worth of CPU time in a driver.

	Arnd
Bjorn Helgaas Feb. 2, 2016, 11:14 p.m. UTC | #2
On Tue, Feb 02, 2016 at 09:25:25PM +0100, Arnd Bergmann wrote:
> On Monday 01 February 2016 18:07:45 Joao Pinto wrote:
> > +static void synopsys_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;
> > +		}
> > +		mdelay(100);
> > +	}
> > +}
> 
> That mdelay() needs to be an msleep(). You should never waste
> a whole second worth of CPU time in a driver.

There are six other copies of this code, and two of them (exynos and
spear) have the same problem.

Bjorn
Arnd Bergmann Feb. 2, 2016, 11:27 p.m. UTC | #3
On Tuesday 02 February 2016 17:14:49 Bjorn Helgaas wrote:
> On Tue, Feb 02, 2016 at 09:25:25PM +0100, Arnd Bergmann wrote:
> > On Monday 01 February 2016 18:07:45 Joao Pinto wrote:
> > > +static void synopsys_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;
> > > +           }
> > > +           mdelay(100);
> > > +   }
> > > +}
> > 
> > That mdelay() needs to be an msleep(). You should never waste
> > a whole second worth of CPU time in a driver.
> 
> There are six other copies of this code, and two of them (exynos and
> spear) have the same problem.

Good point. Maybe move one (correct) copy into the main driver file and
have it called by the other drivers?

IIRC there were some problems in drivers that did a similar
function from the config space accessors where you cannot call
msleep(), but this driver is not one of them.

	Arnd
Bjorn Helgaas Feb. 3, 2016, 6:05 p.m. UTC | #4
Hi Joao & Arnd,

On Tue, Feb 02, 2016 at 09:25:25PM +0100, Arnd Bergmann wrote:
> On Monday 01 February 2016 18:07:45 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.
> > This patch is composed by:
> > 
> > -MAINTAINERS file was updated to include the new driver
> > -Documentation/devicetree/bindings/pci was updated to include the new
> > driver documentation
> > -New driver called pcie-synopsys
> > 
> > Signed-off-by: Joao Pinto <jpinto@synopsys.com>
> 
> I must have completely missed all the earlier submissions and just happened
> to see this one now that it was merged.
> 
> I have a couple of comments, maybe you can send a follow up patch to address
> them:

These seem pretty easy to address, so I pulled these patches out of
for-linus temporarily while we fix them.

Joao, you can just fix the mdelay() in your driver; don't worry about 
fixing other drivers with the same problem or consolidating them.

Bjorn

> > +----------------------------------
> > +
> > +This is the reference platform driver to be used in the Synopsys PCI Root
> > +Complex IP Prototyping Kit.
> > +
> > +Required properties:
> > +- compatible: set to "snps,pcie-synopsys";
> 
> Please also include the exact version of the designware PCIe part that
> is used in here, such as "snps,dw-pcie-1.234". I'm sure you can find
> out the version.
> 
> "snps,pcie-synopsys" by itself is about the worst possible compatible
> string because it says nothing about the specific hardware. Half the
> machines we support all have a designware PCIe host that was made
> by synopsys. Please use the name of the SoC for the most specific
> string.
> 
> > +- reg: base address and length of the pcie controller registers.
> > +- #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,pcie-synopsys";
> > +		reg = <0xdffff000 0x1000>;
> > +		#address-cells = <3>;
> > +		#size-cells = <2>;
> > +		device_type = "pci";
> > +		ranges = <0x00000800 0 0xd0000000 0xd0000000 0 0x00002000>,
> 
> This line looks misplaced here and does not match the documentation. It's
> probably a leftover from an earlier version that had the config space
> in the ranges, so you need to remove that.
> 
> > +/* This handler was created for future work */
> > +static irqreturn_t synopsys_pcie_irq_handler(int irq, void *arg)
> > +{
> > +	return IRQ_NONE;
> > +}
> 
> It's not harmful, but we generally don't add code "for future work",
> better remove it for now.
> 
> > +static void synopsys_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;
> > +		}
> > +		mdelay(100);
> > +	}
> > +}
> 
> That mdelay() needs to be an msleep(). You should never waste
> a whole second worth of CPU time in a driver.
> 
> 	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. 3, 2016, 6:12 p.m. UTC | #5
Hi Bjorn,

Are these tasks enough?

- replace mdelay() for msleep()
- remove synopsys_pcie_irq_handler()
- replace "snps,pcie-synopsys" for "snps,pcie-synopsys-ipk"?
- rename the driver to pcie-synopsys-ipk?
- update the devicetree documentation referring that the ranges also include the
config space

Joao

On 2/3/2016 6:05 PM, Bjorn Helgaas wrote:
> Hi Joao & Arnd,
> 
> On Tue, Feb 02, 2016 at 09:25:25PM +0100, Arnd Bergmann wrote:
>> On Monday 01 February 2016 18:07:45 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.
>>> This patch is composed by:
>>>
>>> -MAINTAINERS file was updated to include the new driver
>>> -Documentation/devicetree/bindings/pci was updated to include the new
>>> driver documentation
>>> -New driver called pcie-synopsys
>>>
>>> Signed-off-by: Joao Pinto <jpinto@synopsys.com>
>>
>> I must have completely missed all the earlier submissions and just happened
>> to see this one now that it was merged.
>>
>> I have a couple of comments, maybe you can send a follow up patch to address
>> them:
> 
> These seem pretty easy to address, so I pulled these patches out of
> for-linus temporarily while we fix them.
> 
> Joao, you can just fix the mdelay() in your driver; don't worry about 
> fixing other drivers with the same problem or consolidating them.
> 
> Bjorn
> 
>>> +----------------------------------
>>> +
>>> +This is the reference platform driver to be used in the Synopsys PCI Root
>>> +Complex IP Prototyping Kit.
>>> +
>>> +Required properties:
>>> +- compatible: set to "snps,pcie-synopsys";
>>
>> Please also include the exact version of the designware PCIe part that
>> is used in here, such as "snps,dw-pcie-1.234". I'm sure you can find
>> out the version.
>>
>> "snps,pcie-synopsys" by itself is about the worst possible compatible
>> string because it says nothing about the specific hardware. Half the
>> machines we support all have a designware PCIe host that was made
>> by synopsys. Please use the name of the SoC for the most specific
>> string.
>>
>>> +- reg: base address and length of the pcie controller registers.
>>> +- #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,pcie-synopsys";
>>> +		reg = <0xdffff000 0x1000>;
>>> +		#address-cells = <3>;
>>> +		#size-cells = <2>;
>>> +		device_type = "pci";
>>> +		ranges = <0x00000800 0 0xd0000000 0xd0000000 0 0x00002000>,
>>
>> This line looks misplaced here and does not match the documentation. It's
>> probably a leftover from an earlier version that had the config space
>> in the ranges, so you need to remove that.
>>
>>> +/* This handler was created for future work */
>>> +static irqreturn_t synopsys_pcie_irq_handler(int irq, void *arg)
>>> +{
>>> +	return IRQ_NONE;
>>> +}
>>
>> It's not harmful, but we generally don't add code "for future work",
>> better remove it for now.
>>
>>> +static void synopsys_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;
>>> +		}
>>> +		mdelay(100);
>>> +	}
>>> +}
>>
>> That mdelay() needs to be an msleep(). You should never waste
>> a whole second worth of CPU time in a driver.
>>
>> 	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
Bjorn Helgaas Feb. 3, 2016, 6:38 p.m. UTC | #6
On Wed, Feb 03, 2016 at 06:12:00PM +0000, Joao Pinto wrote:
> Hi Bjorn,
> 
> Are these tasks enough?
> 
> - replace mdelay() for msleep()
> - remove synopsys_pcie_irq_handler()

Above are fine with me.

> - replace "snps,pcie-synopsys" for "snps,pcie-synopsys-ipk"?

This is a question for Arnd.

> - rename the driver to pcie-synopsys-ipk?

It doesn't seem necessary to me to include both "synopsys" and "ipk" in the
filename and the driver name.  Take a look at what the existing drivers do,
and do something similar.

> - update the devicetree documentation referring that the ranges also include the
> config space

Another one for Arnd.


> On 2/3/2016 6:05 PM, Bjorn Helgaas wrote:
> > Hi Joao & Arnd,
> > 
> > On Tue, Feb 02, 2016 at 09:25:25PM +0100, Arnd Bergmann wrote:
> >> On Monday 01 February 2016 18:07:45 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.
> >>> This patch is composed by:
> >>>
> >>> -MAINTAINERS file was updated to include the new driver
> >>> -Documentation/devicetree/bindings/pci was updated to include the new
> >>> driver documentation
> >>> -New driver called pcie-synopsys
> >>>
> >>> Signed-off-by: Joao Pinto <jpinto@synopsys.com>
> >>
> >> I must have completely missed all the earlier submissions and just happened
> >> to see this one now that it was merged.
> >>
> >> I have a couple of comments, maybe you can send a follow up patch to address
> >> them:
> > 
> > These seem pretty easy to address, so I pulled these patches out of
> > for-linus temporarily while we fix them.
> > 
> > Joao, you can just fix the mdelay() in your driver; don't worry about 
> > fixing other drivers with the same problem or consolidating them.
> > 
> > Bjorn
> > 
> >>> +----------------------------------
> >>> +
> >>> +This is the reference platform driver to be used in the Synopsys PCI Root
> >>> +Complex IP Prototyping Kit.
> >>> +
> >>> +Required properties:
> >>> +- compatible: set to "snps,pcie-synopsys";
> >>
> >> Please also include the exact version of the designware PCIe part that
> >> is used in here, such as "snps,dw-pcie-1.234". I'm sure you can find
> >> out the version.
> >>
> >> "snps,pcie-synopsys" by itself is about the worst possible compatible
> >> string because it says nothing about the specific hardware. Half the
> >> machines we support all have a designware PCIe host that was made
> >> by synopsys. Please use the name of the SoC for the most specific
> >> string.
> >>
> >>> +- reg: base address and length of the pcie controller registers.
> >>> +- #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,pcie-synopsys";
> >>> +		reg = <0xdffff000 0x1000>;
> >>> +		#address-cells = <3>;
> >>> +		#size-cells = <2>;
> >>> +		device_type = "pci";
> >>> +		ranges = <0x00000800 0 0xd0000000 0xd0000000 0 0x00002000>,
> >>
> >> This line looks misplaced here and does not match the documentation. It's
> >> probably a leftover from an earlier version that had the config space
> >> in the ranges, so you need to remove that.
> >>
> >>> +/* This handler was created for future work */
> >>> +static irqreturn_t synopsys_pcie_irq_handler(int irq, void *arg)
> >>> +{
> >>> +	return IRQ_NONE;
> >>> +}
> >>
> >> It's not harmful, but we generally don't add code "for future work",
> >> better remove it for now.
> >>
> >>> +static void synopsys_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;
> >>> +		}
> >>> +		mdelay(100);
> >>> +	}
> >>> +}
> >>
> >> That mdelay() needs to be an msleep(). You should never waste
> >> a whole second worth of CPU time in a driver.
> >>
> >> 	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
>
Arnd Bergmann Feb. 3, 2016, 9:01 p.m. UTC | #7
On Wednesday 03 February 2016 12:38:44 Bjorn Helgaas wrote:
> On Wed, Feb 03, 2016 at 06:12:00PM +0000, Joao Pinto wrote:
> 
> > - replace "snps,pcie-synopsys" for "snps,pcie-synopsys-ipk"?
> 
> This is a question for Arnd.
> 
> > - rename the driver to pcie-synopsys-ipk?
> 
> It doesn't seem necessary to me to include both "synopsys" and "ipk" in the
> filename and the driver name.  Take a look at what the existing drivers do,
> and do something similar.

The "synopsys" can go away, it's already in the vendor field of the
string. "ipk" is still a bit unspecific, I was hoping to see a specific
chip and/or version of the PCIe part. Something like

	compatible = "snps,ipk2040-pcie", "snps,ipk-pcie", "snps,dw-pcie-1.23", "snps,dw-pcie";

which would indicate that there is a chip called "ipk2040" in a family called "ipk",
and this includes the designware pcie implementation in version 1.23.

> > - update the devicetree documentation referring that the ranges also include the
> > config space
> 
> Another one for Arnd.

This one is wrong, the ranges should *not* include the config space, and if they
currently do, you must change the driver. The generic dw-pcie driver still
accepts the config space in the ranges for backwards compatibility with some
of the earlier front-ends that mistakenly did this, but new driver should not
do the same, and we should probably add some code in the common driver to
prevent it for front-ends other than the ones we have to keep compatibility with.

	Arnd
Joao Pinto Feb. 4, 2016, 11:10 a.m. UTC | #8
Hi Arnd and Bjorn,

On 2/3/2016 9:01 PM, Arnd Bergmann wrote:
> On Wednesday 03 February 2016 12:38:44 Bjorn Helgaas wrote:
>> On Wed, Feb 03, 2016 at 06:12:00PM +0000, Joao Pinto wrote:
>>
>>> - replace "snps,pcie-synopsys" for "snps,pcie-synopsys-ipk"?
>>
>> This is a question for Arnd.
>>
>>> - rename the driver to pcie-synopsys-ipk?
>>
>> It doesn't seem necessary to me to include both "synopsys" and "ipk" in the
>> filename and the driver name.  Take a look at what the existing drivers do,
>> and do something similar.
> 
> The "synopsys" can go away, it's already in the vendor field of the
> string. "ipk" is still a bit unspecific, I was hoping to see a specific
> chip and/or version of the PCIe part. Something like
> 
> 	compatible = "snps,ipk2040-pcie", "snps,ipk-pcie", "snps,dw-pcie-1.23", "snps,dw-pcie";
> 
> which would indicate that there is a chip called "ipk2040" in a family called "ipk",
> and this includes the designware pcie implementation in version 1.23.
> 

"snps,dw-pcie" seems a good idea!

>>> - update the devicetree documentation referring that the ranges also include the
>>> config space
>>
>> Another one for Arnd.
> 
> This one is wrong, the ranges should *not* include the config space, and if they
> currently do, you must change the driver. The generic dw-pcie driver still
> accepts the config space in the ranges for backwards compatibility with some
> of the earlier front-ends that mistakenly did this, but new driver should not
> do the same, and we should probably add some code in the common driver to
> prevent it for front-ends other than the ones we have to keep compatibility with.
> 

I am going to remove the config space from the ranges and test the driver.

> 	Arnd
> 

Thanks.
Joao Pinto Feb. 4, 2016, 11:14 a.m. UTC | #9
Hi Bjorn,

On 2/3/2016 6:38 PM, Bjorn Helgaas wrote:
> On Wed, Feb 03, 2016 at 06:12:00PM +0000, Joao Pinto wrote:
>> Hi Bjorn,
>>
>> Are these tasks enough?
>>
>> - replace mdelay() for msleep()
>> - remove synopsys_pcie_irq_handler()
> 
> Above are fine with me.

Ok, I'll to that.

Thanks.
Arnd Bergmann Feb. 4, 2016, 1:12 p.m. UTC | #10
On Thursday 04 February 2016 11:10:55 Joao Pinto wrote:
> > The "synopsys" can go away, it's already in the vendor field of the
> > string. "ipk" is still a bit unspecific, I was hoping to see a specific
> > chip and/or version of the PCIe part. Something like
> > 
> >       compatible = "snps,ipk2040-pcie", "snps,ipk-pcie", "snps,dw-pcie-1.23", "snps,dw-pcie";
> > 
> > which would indicate that there is a chip called "ipk2040" in a family called "ipk",
> > and this includes the designware pcie implementation in version 1.23.
> > 
> 
> "snps,dw-pcie" seems a good idea!
> 

Just to clarify: I really meant you should put all four of the strings
into the compatible property.

	Arnd
Joao Pinto Feb. 4, 2016, 2:09 p.m. UTC | #11
Hi Bjorn and Arnd,

Removing the irq_handler causes the irq request to fail because in
request_threaded_irq() both handler and thread_fn are NULL.

What's the typical procedure for this?

Joao

On 2/4/2016 11:14 AM, Joao Pinto wrote:
> Hi Bjorn,
> 
> On 2/3/2016 6:38 PM, Bjorn Helgaas wrote:
>> On Wed, Feb 03, 2016 at 06:12:00PM +0000, Joao Pinto wrote:
>>> Hi Bjorn,
>>>
>>> Are these tasks enough?
>>>
>>> - replace mdelay() for msleep()
>>> - remove synopsys_pcie_irq_handler()
>>
>> Above are fine with me.
> 
> Ok, I'll to that.
> 
> Thanks.
>
Arnd Bergmann Feb. 4, 2016, 3:22 p.m. UTC | #12
On Thursday 04 February 2016 14:09:26 Joao Pinto wrote:
> Hi Bjorn and Arnd,
> 
> Removing the irq_handler causes the irq request to fail because in
> request_threaded_irq() both handler and thread_fn are NULL.
> 
> What's the typical procedure for this?
> 

Don't call request_irq? ;-)

	Arnd
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/pci/pcie-synopsys.txt b/Documentation/devicetree/bindings/pci/pcie-synopsys.txt
new file mode 100644
index 0000000..f18507c
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/pcie-synopsys.txt
@@ -0,0 +1,33 @@ 
+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,pcie-synopsys";
+- reg: base address and length of the pcie controller registers.
+- #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,pcie-synopsys";
+		reg = <0xdffff000 0x1000>;
+		#address-cells = <3>;
+		#size-cells = <2>;
+		device_type = "pci";
+		ranges = <0x00000800 0 0xd0000000 0xd0000000 0 0x00002000>,
+			 <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..d2e4506 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-snpsdev.txt
+F:	drivers/pci/host/pcie-snpsdev.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..58c5bb1 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_SYNOPSYS
+	bool "Platform Driver for Synopsys Device"
+	depends on ARC && OF
+	select PCIEPORTBUS
+	select PCIE_DW
+	help
+	  Say Y here if you want to enable PCIe controller support on the
+	  Synopsys IP Prototyping Kits.
 endmenu
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 9d4d3c6..bb79b29 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_SYNOPSYS) += pcie-synopsys.o
diff --git a/drivers/pci/host/pcie-synopsys.c b/drivers/pci/host/pcie-synopsys.c
new file mode 100644
index 0000000..9702e79
--- /dev/null
+++ b/drivers/pci/host/pcie-synopsys.c
@@ -0,0 +1,257 @@ 
+/*
+ * 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_synopsys_pcie(x)	container_of(x, struct synopsys_pcie, pp)
+
+struct synopsys_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 */
+
+/* This handler was created for future work */
+static irqreturn_t synopsys_pcie_irq_handler(int irq, void *arg)
+{
+	return IRQ_NONE;
+}
+
+static irqreturn_t synopsys_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 synopsys_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 synopsys_pcie_deassert_core_reset(struct pcie_port *pp)
+{
+	return 0;
+}
+
+static void synopsys_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;
+		}
+		mdelay(100);
+	}
+}
+
+/*
+ * synopsys_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 synopsys_pcie_host_init(struct pcie_port *pp)
+{
+	/* Initialize Phy (Reset/poweron/control-inputs ) */
+	synopsys_pcie_init_phy(pp);
+
+	synopsys_pcie_deassert_core_reset(pp);
+
+	dw_pcie_setup_rc(pp);
+
+	synopsys_pcie_establish_link(pp);
+
+	if (IS_ENABLED(CONFIG_PCI_MSI))
+		dw_pcie_msi_init(pp);
+}
+
+static int synopsys_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
+ * synopsys_pcie_link_up: the function which initiates the phy link up procedure
+ * synopsys_pcie_host_init: the function which does the host/RC Root port
+ * initialization.
+ */
+static struct pcie_host_ops synopsys_pcie_host_ops = {
+	.link_up = synopsys_pcie_link_up,
+	.host_init = synopsys_pcie_host_init,
+};
+
+/**
+ * synopsys_add_pcie_port
+ * This function
+ * a. installs the interrupt handler
+ * b. registers host operations in the pcie_port structure
+ */
+static int synopsys_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;
+
+	ret = devm_request_irq(&pdev->dev, pp->irq, synopsys_pcie_irq_handler,
+				IRQF_SHARED, "synopsys-pcie", pp);
+
+	if (ret) {
+		dev_err(&pdev->dev, "failed to request irq\n");
+		return ret;
+	}
+
+	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,
+					synopsys_pcie_msi_irq_handler,
+					IRQF_SHARED, "synopsys-pcie-msi", pp);
+		if (ret) {
+			dev_err(&pdev->dev, "failed to request msi irq\n");
+			return ret;
+		}
+	}
+
+	pp->root_bus_nr = -1;
+	pp->ops = &synopsys_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;
+}
+
+/**
+ * synopsys_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 synopsys_pcie_probe(struct platform_device *pdev)
+{
+	struct synopsys_pcie *synopsys_pcie;
+	struct pcie_port *pp;
+	struct resource *res;  /* Resource from DT */
+	int ret;
+
+	synopsys_pcie = devm_kzalloc(&pdev->dev, sizeof(*synopsys_pcie),
+					GFP_KERNEL);
+	if (!synopsys_pcie)
+		return -ENOMEM;
+
+	pp = &synopsys_pcie->pp;
+	pp->dev = &pdev->dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	if (!res)
+		return -ENODEV;
+
+	synopsys_pcie->mem_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(synopsys_pcie->mem_base))
+		return PTR_ERR(synopsys_pcie->mem_base);
+
+	pp->dbi_base = synopsys_pcie->mem_base;
+
+	ret = synopsys_add_pcie_port(pp, pdev);
+	if (ret < 0)
+		return ret;
+
+	platform_set_drvdata(pdev, synopsys_pcie);
+
+	return 0;
+}
+
+static const struct of_device_id synopsys_pcie_of_match[] = {
+	{ .compatible = "snps,pcie-synopsys", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, synopsys_pcie_of_match);
+
+static struct platform_driver synopsys_pcie_driver = {
+	.driver = {
+		.name	= "pcie-synopsys",
+		.of_match_table = synopsys_pcie_of_match,
+	},
+	.probe = synopsys_pcie_probe,
+};
+
+module_platform_driver(synopsys_pcie_driver);
+
+MODULE_AUTHOR("Manjunath Bettegowda <manjumb@synopsys.com>");
+MODULE_DESCRIPTION("Synopsys PCIe host controller driver");
+MODULE_LICENSE("GPL v2");