Re: [PATCH V5 1/3] pci: Add PCIe driver for Samsung Exynos
On Fri, Jun 14, 2013 at 02:38:49PM +0200, Arnd Bergmann wrote: On Friday 14 June 2013 12:53:11 Thierry Reding wrote: I think the biggest missing piece is pci_common_exit(), the counterpart of pci_common_init(), to cleanup a host bridge on ARM. I haven't looked in detail at the other architectures, but I suspect there must be some code to call when a host bridge is removed. Looking at drivers/pci/remove.c, it seems like pci_remove_root_bus() might be what we're looking at. It isn't exported so it can't be used by modules, but that can be changed. Is that how a host bridge is typically removed from the system? It's fairly new to have host bridges in loadable modules, so I'm pretty sure it's not supported by the core yet, but it also doesn't seem hard to do. I think you are right with regard to pci_remove_root_bus, and Bjorn might be able to provide more information. Ideally we should be able to load and unload the pci host driver in a loop indefinitely without crashing or exposing any races or leaks, but I would not bet on that working without bugs in the core, since that goes beyond the normal pci (device) hotplug case. I've done some preliminary testing on Tegra using sysfs to unbind and rebind the device from and to the driver. Some code needs to be added for this to work, but it doesn't crash and PCI even continues to work after unbinding and rebinding (tested using gigabit ethernet). However I haven't looked for leaks yet, and I'm pretty sure some more code is required to undo some of what pci_common_init() does on ARM. Looking more closely, I think most (if not all) remaining leaks could be fixed by keeping the list of pci_sys_data structures around and cleaning them up properly. Given the experimental nature of this I don't want to make that part of the driver for 3.12 and I've opted to just disable any means of removing the driver for now. But I do want to get back to this after the driver has been merged. Thierry pgpERyN2YZRUT.pgp Description: PGP signature
Re: [PATCH V5 1/3] pci: Add PCIe driver for Samsung Exynos
On Wednesday, June 19, 2013 9:43 PM, Arnd Bergmann wrote:
On Wednesday 19 June 2013, Jingoo Han wrote:
Then, do you mean the following?
static int __exit exynos_pcie_remove(struct platform_device *pdev)
{
struct pcie_port *pp = platform_get_drvdata(pdev);
clk_disable_unprepare(pp-bus_clk);
clk_disable_unprepare(pp-clk);
return 0;
}
static struct platform_driver exynos_pcie_driver = {
.remove = __exit_p(exynos_pcie_remove),
[.]
/* Exynos PCIe driver does not allow module unload */
static int __init pcie_init(void)
{
hook_fault_code(16 + 6, exynos_pcie_abort, SIGBUS, 0,
imprecise external abort);
platform_driver_probe(exynos_pcie_driver, exynos_pcie_probe);
return 0;
}
subsys_initcall(pcie_init);
MODULE_AUTHOR(Jingoo Han jg1@samsung.com);
MODULE_DESCRIPTION(Samsung PCIe host controller driver);
MODULE_LICENSE(GPLv2);
Yes, this looks good. I would probably use platform_driver_register
rather than platform_driver_probe, but that is your choice. using
platform_driver_probe() mean you cannot deal with deferred probing.
Hi Arnd,
Thank you for your reply. :)
I will send PATCH v6, soon.
I really appreciate your comments.
Hi Thomas Abraham, Kukjin Kim,
Thank you for your support.
It is very helpful.
Best regards,
Jingoo Han
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V5 1/3] pci: Add PCIe driver for Samsung Exynos
On Wednesday 19 June 2013, Jingoo Han wrote:
Then, do you mean the following?
static int __exit exynos_pcie_remove(struct platform_device *pdev)
{
struct pcie_port *pp = platform_get_drvdata(pdev);
clk_disable_unprepare(pp-bus_clk);
clk_disable_unprepare(pp-clk);
return 0;
}
static struct platform_driver exynos_pcie_driver = {
.remove = __exit_p(exynos_pcie_remove),
[.]
/* Exynos PCIe driver does not allow module unload */
static int __init pcie_init(void)
{
hook_fault_code(16 + 6, exynos_pcie_abort, SIGBUS, 0,
imprecise external abort);
platform_driver_probe(exynos_pcie_driver, exynos_pcie_probe);
return 0;
}
subsys_initcall(pcie_init);
MODULE_AUTHOR(Jingoo Han jg1@samsung.com);
MODULE_DESCRIPTION(Samsung PCIe host controller driver);
MODULE_LICENSE(GPLv2);
Yes, this looks good. I would probably use platform_driver_register
rather than platform_driver_probe, but that is your choice. using
platform_driver_probe() mean you cannot deal with deferred probing.
Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V5 1/3] pci: Add PCIe driver for Samsung Exynos
On Tuesday 18 June 2013, Jingoo Han wrote:
On Monday, June 17, 2013 9:45 PM, Arnd Bergmann wrote:
On Monday 17 June 2013 18:45:52 Jingoo Han wrote:
On Friday, June 14, 2013 9:54 PM, Arnd Bergmann wrote:
Please look up the documentation about inbound viewport and describe
in a code comment what it does. I /assume/ that this is how DMA accesses
from the bus get translated into AXI bus transactions. If so, you have
to let the window translate addresses from RAM. If it's something else,
then you should document what it is and how it needs to be set up.
One of our hardware engineer confirmed it.
He said that these inbound functions are unnecessary.
Also, I checked that PCIe works properly without these functions.
So, I will remove these inbound functions.
Ok, good. So DMA just gets translated 1:1 independent of the
inbound viewport? Have you tested this with PCI device using
DMA?
According to our hardware engineer,
He said that
That's correct. We tested it by doing a memory write from the device.
A device DMA is a series of memory r/w so I expect it to work same way.
Also, he thought that I already tested too, since it works after removing
the functions.
It could be that the default setting just creates a 1:1 map so that would
work, but if you tested it, that should be good enough.
I looked at the pci-mvebu driver,
Then I fixed it as the pci-mvebu driver did.
Also, I added 'global_io_offset'.
@@ -909,6 +909,12 @@ static int __init exynos_pcie_probe(struct
platform_device *pdev)
if (restype == IORESOURCE_IO) {
of_pci_range_to_resource(range, np, pp-io);
pp-io.name = I/O;
+ pp-io.start = max_t(resource_size_t,
+ PCIBIOS_MIN_IO,
+ range.pci_addr +
global_io_offset);
+ pp-io.end = min_t(resource_size_t,
+ IO_SPACE_LIMIT,
+ range.pci_addr + range.size +
global_io_offset);
pp-config.io_size = resource_size(pp-io);
pp-config.io_bus_addr = range.pci_addr;
In this case, boot message is as below:
PCI host bridge to bus :00
pci_bus :00: root bus resource [io 0x1000-0x1]
pci_bus :00: root bus resource [mem 0x40011000-0x5fff]
[.]
PCI host bridge to bus 0001:00
pci_bus 0001:00: root bus resource [io 0x1-0x2] (bus address
[0x-0x1])
pci_bus 0001:00: root bus resource [mem 0x60011000-0x7fff]
Ok, very good.
I will remove a 'remove' callback. Is it OK?
Or what should I do?
I think you should keep the remove function, but add a comment explaining that
you don't allow module unload and that in order to allow it, the remove function
will have to remove all pci buses and devices under the host bridge.
Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V5 1/3] pci: Add PCIe driver for Samsung Exynos
On Tuesday, June 18, 2013 10:57 PM, Arnd Bergmann wrote:
On Tuesday 18 June 2013, Jingoo Han wrote:
On Monday, June 17, 2013 9:45 PM, Arnd Bergmann wrote:
On Monday 17 June 2013 18:45:52 Jingoo Han wrote:
On Friday, June 14, 2013 9:54 PM, Arnd Bergmann wrote:
[.]
I will remove a 'remove' callback. Is it OK?
Or what should I do?
I think you should keep the remove function, but add a comment explaining that
you don't allow module unload and that in order to allow it, the remove
function
will have to remove all pci buses and devices under the host bridge.
Then, do you mean the following?
static int __exit exynos_pcie_remove(struct platform_device *pdev)
{
struct pcie_port *pp = platform_get_drvdata(pdev);
clk_disable_unprepare(pp-bus_clk);
clk_disable_unprepare(pp-clk);
return 0;
}
static struct platform_driver exynos_pcie_driver = {
.remove = __exit_p(exynos_pcie_remove),
[.]
/* Exynos PCIe driver does not allow module unload */
static int __init pcie_init(void)
{
hook_fault_code(16 + 6, exynos_pcie_abort, SIGBUS, 0,
imprecise external abort);
platform_driver_probe(exynos_pcie_driver, exynos_pcie_probe);
return 0;
}
subsys_initcall(pcie_init);
MODULE_AUTHOR(Jingoo Han jg1@samsung.com);
MODULE_DESCRIPTION(Samsung PCIe host controller driver);
MODULE_LICENSE(GPLv2);
Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-pci in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V5 1/3] pci: Add PCIe driver for Samsung Exynos
On Friday, June 14, 2013 9:54 PM, Arnd Bergmann wrote:
On Friday 14 June 2013 17:18:46 Jingoo Han wrote:
On Thursday, June 13, 2013 11:14 PM, Arnd Bergmann wrote:
On Thursday 13 June 2013 22:22:31 Jingoo Han wrote:
+struct pcie_port {
+ struct device *dev;
+ u8 controller;
+ u8 root_bus_nr;
+ void __iomem*dbi_base;
+ void __iomem*elbi_base;
+ void __iomem*phy_base;
+ void __iomem*purple_base;
+ phys_addr_t cfg0_base;
+ void __iomem*va_cfg0_base;
+ phys_addr_t cfg1_base;
+ void __iomem*va_cfg1_base;
+ phys_addr_t io_base;
+ phys_addr_t mem_base;
+ spinlock_t conf_lock;
+ struct resource cfg;
+ struct resource io;
+ struct resource mem;
+ struct pcie_port_info config;
+ struct clk *clk;
+ struct clk *bus_clk;
+ int irq;
+ int reset_gpio;
+};
You mentioned that you'd use u64 for the resources here but did not.
Do you mean the following?
+ u64 cfg0_base;
+ u64 cfg1_base;
+ u64 io_base;
+ u64 mem_base;
Yes, anything you need to program into a 64 bit hardware register.
OK,
+static void exynos_pcie_prog_viewport_cfg1(struct pcie_port *pp, u32
busdev)
+{
+ u32 val;
+ void __iomem *dbi_base = pp-dbi_base;
+
+ /* Program viewport 1 : OUTBOUND : CFG1 */
+ val = PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX1;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_VIEWPORT);
+ writel_rc(pp, PCIE_ATU_TYPE_CFG1, dbi_base + PCIE_ATU_CR1);
+ val = PCIE_ATU_ENABLE;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_CR2);
+ writel_rc(pp, (u64)pp-cfg1_base, dbi_base +
PCIE_ATU_LOWER_BASE);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_BASE);
+ writel_rc(pp, (u64)pp-cfg1_base + pp-config.cfg1_size - 1,
+ dbi_base + PCIE_ATU_LIMIT);
+ writel_rc(pp, busdev, dbi_base + PCIE_ATU_LOWER_TARGET);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_TARGET);
+}
And you still don't set up the UPPER halves, which was really the point
of my comment. Same thing for all the other ones.
Do you mean the following?
+ writel_rc(pp, pp-cfg1_base, dbi_base + PCIE_ATU_LOWER_BASE);
+ writel_rc(pp, (pp-cfg1_base 32), dbi_base + PCIE_ATU_UPPER_BASE);
Right. Note that you could achieve the same using phys_addr_t instead of
u64, but it's more complicated to get that to work for both 32 and 64 bit
phys_addr_t types, since you cannot shift a 32 bit value by 32 bits in C.
OK,
+static void exynos_pcie_prog_viewport_mem_inbound(struct pcie_port *pp)
+{
+ u32 val;
+ void __iomem *dbi_base = pp-dbi_base;
+
+ /* Program viewport 0 : INBOUND : MEM */
+ val = PCIE_ATU_REGION_INBOUND | PCIE_ATU_REGION_INDEX0;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_VIEWPORT);
+ writel_rc(pp, PCIE_ATU_TYPE_MEM, dbi_base + PCIE_ATU_CR1);
+ val = PCIE_ATU_ENABLE | PCIE_ATU_BAR_MODE_ENABLE;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_CR2);
+ writel_rc(pp, (u64)pp-config.mem_bus_addr,
+ dbi_base + PCIE_ATU_LOWER_BASE);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_BASE);
+ writel_rc(pp, (u64)pp-config.mem_bus_addr +
pp-config.mem_size - 1,
+ dbi_base + PCIE_ATU_LIMIT);
+ writel_rc(pp, (u64)pp-mem_base, dbi_base +
PCIE_ATU_LOWER_TARGET);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_TARGET);
+}
This makes even less sense than before, it seems like now you only
allow DMA between PCI devices but not to main memory.
Sorry, I cannot understand it.
Could you give me more details?
Also, pseudo-code will be very helpful.
Please look up the documentation about inbound viewport and describe
in a code comment what it does. I /assume/ that this is how DMA accesses
from the bus get translated into AXI bus transactions. If so, you have
to let the window translate addresses from RAM. If it's something else,
then you should document what it is and how it needs to be set up.
One of our hardware engineer confirmed it.
He said that these inbound functions are unnecessary.
Also, I checked that PCIe works properly without these functions.
So, I will remove these inbound functions.
+static int exynos_pcie_setup(int nr, struct pci_sys_data *sys)
+{
+ struct pcie_port *pp;
+
+ pp
Re: [PATCH V5 1/3] pci: Add PCIe driver for Samsung Exynos
On Monday 17 June 2013 18:45:52 Jingoo Han wrote:
On Friday, June 14, 2013 9:54 PM, Arnd Bergmann wrote:
Please look up the documentation about inbound viewport and describe
in a code comment what it does. I /assume/ that this is how DMA accesses
from the bus get translated into AXI bus transactions. If so, you have
to let the window translate addresses from RAM. If it's something else,
then you should document what it is and how it needs to be set up.
One of our hardware engineer confirmed it.
He said that these inbound functions are unnecessary.
Also, I checked that PCIe works properly without these functions.
So, I will remove these inbound functions.
Ok, good. So DMA just gets translated 1:1 independent of the
inbound viewport? Have you tested this with PCI device using
DMA?
static int exynos_pcie_setup(int nr, struct pci_sys_data *sys)
{
struct pcie_port *pp;
pp = sys_to_pcie(sys);
if (!pp)
return 0;
if (global_io_offset SZ_1M pp-config.io_size 0) {
sys-io_offset = global_io_offset - pp-config.io_bus_addr; /*
normally 0 */
pci_ioremap_io(sys-io_offset, pp-io.start);
global_io_offset += SZ_64K;
}
sys-mem_offset = pp-mem.start - pp-config.mem_bus_addr; /* normally
0 */
pci_add_resource_offset(sys-resources, pp-io, sys-io_offset);
pci_add_resource_offset(sys-resources, pp-mem, sys-mem_offset);
return 1;
}
This is what I meant, yes.
In this case, boot message is as below:
PCI host bridge to bus :00
pci_bus :00: root bus resource [io 0x40001000-0x40010fff]
pci_bus :00: root bus resource [mem 0x40011000-0x5fff]
pci_bus :00: No busn resource found for root bus, will use [bus 00-ff]
pci :00:00.0: [144d:a549] type 01 class 0x060400
[.]
PCI host bridge to bus 0001:00
pci_bus 0001:00: root bus resource [io 0x60001000-0x60010fff] (bus address
[0x5fff1000-0x6000
0fff])
pci_bus 0001:00: root bus resource [mem 0x60011000-0x7fff]
pci_bus 0001:00: No busn resource found for root bus, will use [bus 00-ff]
pci 0001:00:00.0: [144d:a549] type 01 class 0x060400
The io resources here look wrong. I would have expected
pci_bus :00: root bus resource [io 0x001000-0x00]
pci_bus 0001:00: root bus resource [io 0x01-0x01] (bus address
[0x00-0x00])
Please have a look at the pci-mvebu driver and how it calculates its
'realio' resource.
+static int __exit exynos_pcie_remove(struct platform_device *pdev)
+{
+ struct pcie_port *pp = platform_get_drvdata(pdev);
+
+ clk_disable_unprepare(pp-bus_clk);
+ clk_disable_unprepare(pp-clk);
+
+ return 0;
+}
You also don't remove the PCI devices here, as mentioned in an earlier
review.
I reviewed Marvell PCIe driver and Tegra PCIe driver; however,
I cannot know what you mean.
Could let me know which additional functions are needed?
The mvebu driver does not allow module unload. I haven't looked at the
tegra driver. If you allow unloading the driver and provide a 'remove'
callback, that callback needs to clean up the entire bus and remove
all child devices that were added as well as undo everything the
probe function did. I think it would be great if you can do that, although
it might not be easy. The simplest solution would be to not support
unloading though.
As the mvebu driver uses platform_driver_probe(), the Exynos driver uses
platform_driver_probe(). Thus, I will not provide a 'remove' callback.
Well, the important part is not to provide a module_exit() function, which
will ensure the driver cannot be unloaded.
Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V5 1/3] pci: Add PCIe driver for Samsung Exynos
On Monday, June 17, 2013 9:45 PM, Arnd Bergmann wrote:
On Monday 17 June 2013 18:45:52 Jingoo Han wrote:
On Friday, June 14, 2013 9:54 PM, Arnd Bergmann wrote:
Please look up the documentation about inbound viewport and describe
in a code comment what it does. I /assume/ that this is how DMA accesses
from the bus get translated into AXI bus transactions. If so, you have
to let the window translate addresses from RAM. If it's something else,
then you should document what it is and how it needs to be set up.
One of our hardware engineer confirmed it.
He said that these inbound functions are unnecessary.
Also, I checked that PCIe works properly without these functions.
So, I will remove these inbound functions.
Ok, good. So DMA just gets translated 1:1 independent of the
inbound viewport? Have you tested this with PCI device using
DMA?
According to our hardware engineer,
He said that
That's correct. We tested it by doing a memory write from the device.
A device DMA is a series of memory r/w so I expect it to work same way.
Also, he thought that I already tested too, since it works after removing
the functions.
static int exynos_pcie_setup(int nr, struct pci_sys_data *sys)
{
struct pcie_port *pp;
pp = sys_to_pcie(sys);
if (!pp)
return 0;
if (global_io_offset SZ_1M pp-config.io_size 0) {
sys-io_offset = global_io_offset - pp-config.io_bus_addr; /*
normally 0 */
pci_ioremap_io(sys-io_offset, pp-io.start);
global_io_offset += SZ_64K;
}
sys-mem_offset = pp-mem.start - pp-config.mem_bus_addr; /* normally
0 */
pci_add_resource_offset(sys-resources, pp-io, sys-io_offset);
pci_add_resource_offset(sys-resources, pp-mem, sys-mem_offset);
return 1;
}
This is what I meant, yes.
In this case, boot message is as below:
PCI host bridge to bus :00
pci_bus :00: root bus resource [io 0x40001000-0x40010fff]
pci_bus :00: root bus resource [mem 0x40011000-0x5fff]
pci_bus :00: No busn resource found for root bus, will use [bus 00-ff]
pci :00:00.0: [144d:a549] type 01 class 0x060400
[.]
PCI host bridge to bus 0001:00
pci_bus 0001:00: root bus resource [io 0x60001000-0x60010fff] (bus address
[0x5fff1000-0x6000
0fff])
pci_bus 0001:00: root bus resource [mem 0x60011000-0x7fff]
pci_bus 0001:00: No busn resource found for root bus, will use [bus 00-ff]
pci 0001:00:00.0: [144d:a549] type 01 class 0x060400
The io resources here look wrong. I would have expected
pci_bus :00: root bus resource [io 0x001000-0x00]
pci_bus 0001:00: root bus resource [io 0x01-0x01] (bus address
[0x00-0x00])
Please have a look at the pci-mvebu driver and how it calculates its
'realio' resource.
I looked at the pci-mvebu driver,
Then I fixed it as the pci-mvebu driver did.
Also, I added 'global_io_offset'.
@@ -909,6 +909,12 @@ static int __init exynos_pcie_probe(struct platform_device
*pdev)
if (restype == IORESOURCE_IO) {
of_pci_range_to_resource(range, np, pp-io);
pp-io.name = I/O;
+ pp-io.start = max_t(resource_size_t,
+ PCIBIOS_MIN_IO,
+ range.pci_addr +
global_io_offset);
+ pp-io.end = min_t(resource_size_t,
+ IO_SPACE_LIMIT,
+ range.pci_addr + range.size +
global_io_offset);
pp-config.io_size = resource_size(pp-io);
pp-config.io_bus_addr = range.pci_addr;
In this case, boot message is as below:
PCI host bridge to bus :00
pci_bus :00: root bus resource [io 0x1000-0x1]
pci_bus :00: root bus resource [mem 0x40011000-0x5fff]
[.]
PCI host bridge to bus 0001:00
pci_bus 0001:00: root bus resource [io 0x1-0x2] (bus address
[0x-0x1])
pci_bus 0001:00: root bus resource [mem 0x60011000-0x7fff]
+static int __exit exynos_pcie_remove(struct platform_device *pdev)
+{
+ struct pcie_port *pp = platform_get_drvdata(pdev);
+
+ clk_disable_unprepare(pp-bus_clk);
+ clk_disable_unprepare(pp-clk);
+
+ return 0;
+}
You also don't remove the PCI devices here, as mentioned in an earlier
review.
I reviewed Marvell PCIe driver and Tegra PCIe driver; however,
I cannot know what you mean.
Could let me know which additional functions are needed?
The mvebu driver does not allow module unload. I haven't looked at the
tegra driver. If you allow unloading the driver and provide a 'remove'
callback, that callback needs to clean up the entire bus and remove
all child devices that were added as well as undo
Re: [PATCH V5 1/3] pci: Add PCIe driver for Samsung Exynos
On Monday, June 17, 2013 9:45 PM, Arnd Bergmann wrote:
On Monday 17 June 2013 18:45:52 Jingoo Han wrote:
On Friday, June 14, 2013 9:54 PM, Arnd Bergmann wrote:
[.]
+static int __exit exynos_pcie_remove(struct platform_device *pdev)
+{
+ struct pcie_port *pp = platform_get_drvdata(pdev);
+
+ clk_disable_unprepare(pp-bus_clk);
+ clk_disable_unprepare(pp-clk);
+
+ return 0;
+}
You also don't remove the PCI devices here, as mentioned in an earlier
review.
I reviewed Marvell PCIe driver and Tegra PCIe driver; however,
I cannot know what you mean.
Could let me know which additional functions are needed?
The mvebu driver does not allow module unload. I haven't looked at the
tegra driver. If you allow unloading the driver and provide a 'remove'
callback, that callback needs to clean up the entire bus and remove
all child devices that were added as well as undo everything the
probe function did. I think it would be great if you can do that, although
it might not be easy. The simplest solution would be to not support
unloading though.
As the mvebu driver uses platform_driver_probe(), the Exynos driver uses
platform_driver_probe(). Thus, I will not provide a 'remove' callback.
Well, the important part is not to provide a module_exit() function, which
will ensure the driver cannot be unloaded.
Aha, I see.
I will not provide a module_exit() function), as the mvebu driver does.
Best regards,
Jingoo Han
Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V5 1/3] pci: Add PCIe driver for Samsung Exynos
On Thursday, June 13, 2013 11:14 PM, Arnd Bergmann wrote:
On Thursday 13 June 2013 22:22:31 Jingoo Han wrote:
+struct pcie_port_info {
+ u32 cfg0_size;
+ u32 cfg1_size;
+ u32 io_size;
+ u32 mem_size;
+ phys_addr_t io_bus_addr;
+ phys_addr_t mem_bus_addr;
+};
+
+struct pcie_port {
+ struct device *dev;
+ u8 controller;
+ u8 root_bus_nr;
+ void __iomem*dbi_base;
+ void __iomem*elbi_base;
+ void __iomem*phy_base;
+ void __iomem*purple_base;
+ phys_addr_t cfg0_base;
+ void __iomem*va_cfg0_base;
+ phys_addr_t cfg1_base;
+ void __iomem*va_cfg1_base;
+ phys_addr_t io_base;
+ phys_addr_t mem_base;
+ spinlock_t conf_lock;
+ struct resource cfg;
+ struct resource io;
+ struct resource mem;
+ struct pcie_port_info config;
+ struct clk *clk;
+ struct clk *bus_clk;
+ int irq;
+ int reset_gpio;
+};
You mentioned that you'd use u64 for the resources here but did not.
Do you mean the following?
+ u64 cfg0_base;
+ u64 cfg1_base;
+ u64 io_base;
+ u64 mem_base;
+
+static void exynos_pcie_prog_viewport_cfg0(struct pcie_port *pp, u32
busdev)
+{
+ u32 val;
+ void __iomem *dbi_base = pp-dbi_base;
+
+ /* Program viewport 0 : OUTBOUND : CFG0 */
+ val = PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX0;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_VIEWPORT);
+ writel_rc(pp, (u64)pp-cfg0_base, dbi_base + PCIE_ATU_LOWER_BASE);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_BASE);
+ writel_rc(pp, (u64)pp-cfg0_base + pp-config.cfg0_size - 1,
+ dbi_base + PCIE_ATU_LIMIT);
+ writel_rc(pp, busdev, dbi_base + PCIE_ATU_LOWER_TARGET);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_TARGET);
+ writel_rc(pp, PCIE_ATU_TYPE_CFG0, dbi_base + PCIE_ATU_CR1);
+ val = PCIE_ATU_ENABLE;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_CR2);
+}
+
+static void exynos_pcie_prog_viewport_cfg1(struct pcie_port *pp, u32
busdev)
+{
+ u32 val;
+ void __iomem *dbi_base = pp-dbi_base;
+
+ /* Program viewport 1 : OUTBOUND : CFG1 */
+ val = PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX1;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_VIEWPORT);
+ writel_rc(pp, PCIE_ATU_TYPE_CFG1, dbi_base + PCIE_ATU_CR1);
+ val = PCIE_ATU_ENABLE;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_CR2);
+ writel_rc(pp, (u64)pp-cfg1_base, dbi_base + PCIE_ATU_LOWER_BASE);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_BASE);
+ writel_rc(pp, (u64)pp-cfg1_base + pp-config.cfg1_size - 1,
+ dbi_base + PCIE_ATU_LIMIT);
+ writel_rc(pp, busdev, dbi_base + PCIE_ATU_LOWER_TARGET);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_TARGET);
+}
And you still don't set up the UPPER halves, which was really the point
of my comment. Same thing for all the other ones.
Do you mean the following?
+ writel_rc(pp, pp-cfg1_base, dbi_base + PCIE_ATU_LOWER_BASE);
+ writel_rc(pp, (pp-cfg1_base 32), dbi_base + PCIE_ATU_UPPER_BASE);
+static void exynos_pcie_prog_viewport_mem_inbound(struct pcie_port *pp)
+{
+ u32 val;
+ void __iomem *dbi_base = pp-dbi_base;
+
+ /* Program viewport 0 : INBOUND : MEM */
+ val = PCIE_ATU_REGION_INBOUND | PCIE_ATU_REGION_INDEX0;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_VIEWPORT);
+ writel_rc(pp, PCIE_ATU_TYPE_MEM, dbi_base + PCIE_ATU_CR1);
+ val = PCIE_ATU_ENABLE | PCIE_ATU_BAR_MODE_ENABLE;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_CR2);
+ writel_rc(pp, (u64)pp-config.mem_bus_addr,
+ dbi_base + PCIE_ATU_LOWER_BASE);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_BASE);
+ writel_rc(pp, (u64)pp-config.mem_bus_addr + pp-config.mem_size - 1,
+ dbi_base + PCIE_ATU_LIMIT);
+ writel_rc(pp, (u64)pp-mem_base, dbi_base + PCIE_ATU_LOWER_TARGET);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_TARGET);
+}
This makes even less sense than before, it seems like now you only
allow DMA between PCI devices but not to main memory.
Sorry, I cannot understand it.
Could you give me more details?
Also, pseudo-code will be very helpful.
+static void exynos_pcie_prog_viewport_io_inbound(struct pcie_port *pp)
+{
+ u32 val;
+ void __iomem *dbi_base = pp-dbi_base;
+
+ /* Program viewport 1 : INBOUND : IO */
+ val = PCIE_ATU_REGION_INBOUND | PCIE_ATU_REGION_INDEX1;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_VIEWPORT);
+ writel_rc(pp, PCIE_ATU_TYPE_IO, dbi_base + PCIE_ATU_CR1);
+ val =
Re: [PATCH V5 1/3] pci: Add PCIe driver for Samsung Exynos
On Fri, Jun 14, 2013 at 05:18:46PM +0900, Jingoo Han wrote:
On Thursday, June 13, 2013 11:14 PM, Arnd Bergmann wrote:
On Thursday 13 June 2013 22:22:31 Jingoo Han wrote:
[...]
+static int __exit exynos_pcie_remove(struct platform_device *pdev)
+{
+ struct pcie_port *pp = platform_get_drvdata(pdev);
+
+ clk_disable_unprepare(pp-bus_clk);
+ clk_disable_unprepare(pp-clk);
+
+ return 0;
+}
You also don't remove the PCI devices here, as mentioned in an earlier
review.
I reviewed Marvell PCIe driver and Tegra PCIe driver; however,
I cannot know what you mean.
Could let me know which additional functions are needed?
We don't currently do that on Tegra either. pci-mvebu doesn't do that
either, but they don't implement the driver's .remove() in the first
place.
I think the biggest missing piece is pci_common_exit(), the counterpart
of pci_common_init(), to cleanup a host bridge on ARM. I haven't looked
in detail at the other architectures, but I suspect there must be some
code to call when a host bridge is removed.
Looking at drivers/pci/remove.c, it seems like pci_remove_root_bus()
might be what we're looking at. It isn't exported so it can't be used by
modules, but that can be changed. Is that how a host bridge is typically
removed from the system?
Thierry
pgpKsIRD8UENN.pgp
Description: PGP signature
Re: [PATCH V5 1/3] pci: Add PCIe driver for Samsung Exynos
On Friday 14 June 2013 12:53:11 Thierry Reding wrote: I think the biggest missing piece is pci_common_exit(), the counterpart of pci_common_init(), to cleanup a host bridge on ARM. I haven't looked in detail at the other architectures, but I suspect there must be some code to call when a host bridge is removed. Looking at drivers/pci/remove.c, it seems like pci_remove_root_bus() might be what we're looking at. It isn't exported so it can't be used by modules, but that can be changed. Is that how a host bridge is typically removed from the system? It's fairly new to have host bridges in loadable modules, so I'm pretty sure it's not supported by the core yet, but it also doesn't seem hard to do. I think you are right with regard to pci_remove_root_bus, and Bjorn might be able to provide more information. Ideally we should be able to load and unload the pci host driver in a loop indefinitely without crashing or exposing any races or leaks, but I would not bet on that working without bugs in the core, since that goes beyond the normal pci (device) hotplug case. Arnd -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V5 1/3] pci: Add PCIe driver for Samsung Exynos
Exynos5440 has a PCIe controller which can be used as Root Complex.
This driver supports a PCIe controller as Root Complex mode.
Signed-off-by: Surendranath Gurivireddy Balla suren.re...@samsung.com
Signed-off-by: Siva Reddy Kallam siva.kal...@samsung.com
Signed-off-by: Jingoo Han jg1@samsung.com
---
Tested on Exynos5440.
.../devicetree/bindings/pci/exynos-pcie.txt| 71 ++
drivers/pci/host/Kconfig |5 +
drivers/pci/host/Makefile |1 +
drivers/pci/host/pci-exynos.c | 1078
4 files changed, 1155 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pci/exynos-pcie.txt
create mode 100644 drivers/pci/host/pci-exynos.c
diff --git a/Documentation/devicetree/bindings/pci/exynos-pcie.txt
b/Documentation/devicetree/bindings/pci/exynos-pcie.txt
new file mode 100644
index 000..638e68c
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/exynos-pcie.txt
@@ -0,0 +1,71 @@
+* Samsung Exynos PCIe interface
+
+Required properties:
+-compatible: should be samsung,exynos5440-pcie
+-reg: base addresses and lengths of the pcie conteroller,
+ the phy controller, additional register for the phy controller.
+- interrupts: interrupt values for level interrupt,
+ pulse interrupt, special interrupt.
+- clocks: from common clock binding: handle to pci clock.
+- clock-names: from common clock binding: Shall be pcie, pcie_bus.
+- #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
+- #interrupt-cells: set to 1
+- interrupt-map-mask and interrupt-map: standard PCI properties
+ to define the mapping of the PCIe interface to interrupt
+ numbers.
+- reset-gpio: gpio pin number of power good signal
+
+Example:
+
+SoC specific DT Entry:
+
+ pcie0@29 {
+ compatible = samsung,exynos5440-pcie;
+ reg = 0x29 0x1000
+ 0x27 0x1000
+ 0x271000 0x40;
+ interrupts = 0 20 0, 0 21 0, 0 22 0;
+ clocks = clock 28, clock 27;
+ clock-names = pcie, pcie_bus;
+ #address-cells = 3;
+ #size-cells = 2;
+ device_type = pci;
+ ranges = 0x0800 0 0x4000 0x4000 0 0x1000 /*
configuration space */
+ 0x8100 0 0 0x40001000 0 0x0001 /*
downstream I/O */
+ 0x8200 0 0x40011000 0x40011000 0 0x1ffef000; /*
non-prefetchable memory */
+ #interrupt-cells = 1;
+ interrupt-map-mask = 0 0 0 0;
+ interrupt-map = 0x0 0 gic 53;
+ };
+
+ pcie1@2a {
+ compatible = samsung,exynos5440-pcie;
+ reg = 0x2a 0x1000
+ 0x272000 0x1000
+ 0x271040 0x40;
+ interrupts = 0 23 0, 0 24 0, 0 25 0;
+ clocks = clock 29, clock 27;
+ clock-names = pcie, pcie_bus;
+ #address-cells = 3;
+ #size-cells = 2;
+ device_type = pci;
+ ranges = 0x0800 0 0x6000 0x6000 0 0x1000 /*
configuration space */
+ 0x8100 0 0 0x60001000 0 0x0001 /*
downstream I/O */
+ 0x8200 0 0x60011000 0x60011000 0 0x1ffef000; /*
non-prefetchable memory */
+ #interrupt-cells = 1;
+ interrupt-map-mask = 0 0 0 0;
+ interrupt-map = 0x0 0 gic 56;
+ };
+
+Board specific DT Entry:
+
+ pcie0@29 {
+ reset-gpio = pin_ctrl 5 0;
+ };
+
+ pcie1@2a {
+ reset-gpio = pin_ctrl 22 0;
+ };
diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index 1f1d67f..fec0f1f 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -5,4 +5,9 @@ config PCI_MVEBU
bool Marvell EBU PCIe controller
depends on ARCH_MVEBU || ARCH_KIRKWOOD
+config PCI_EXYNOS
+ bool Samsung Exynos PCIe controller
+ depends on SOC_EXYNOS5440
+ select PCIEPORTBUS
+
endmenu
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 5ea2d8b..31d77ad 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -1 +1,2 @@
obj-$(CONFIG_PCI_MVEBU) += pci-mvebu.o
+obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o
diff --git a/drivers/pci/host/pci-exynos.c b/drivers/pci/host/pci-exynos.c
new file mode 100644
index 000..abfa3dc
--- /dev/null
+++ b/drivers/pci/host/pci-exynos.c
@@ -0,0 +1,1078 @@
+/*
+ * PCIe host controller driver for Samsung EXYNOS SoCs
+ *
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * http://www.samsung.com
+ *
+ * Author: Jingoo Han jg1@samsung.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under
Re: [PATCH V5 1/3] pci: Add PCIe driver for Samsung Exynos
On Thursday 13 June 2013 22:22:31 Jingoo Han wrote:
+struct pcie_port_info {
+ u32 cfg0_size;
+ u32 cfg1_size;
+ u32 io_size;
+ u32 mem_size;
+ phys_addr_t io_bus_addr;
+ phys_addr_t mem_bus_addr;
+};
+
+struct pcie_port {
+ struct device *dev;
+ u8 controller;
+ u8 root_bus_nr;
+ void __iomem*dbi_base;
+ void __iomem*elbi_base;
+ void __iomem*phy_base;
+ void __iomem*purple_base;
+ phys_addr_t cfg0_base;
+ void __iomem*va_cfg0_base;
+ phys_addr_t cfg1_base;
+ void __iomem*va_cfg1_base;
+ phys_addr_t io_base;
+ phys_addr_t mem_base;
+ spinlock_t conf_lock;
+ struct resource cfg;
+ struct resource io;
+ struct resource mem;
+ struct pcie_port_info config;
+ struct clk *clk;
+ struct clk *bus_clk;
+ int irq;
+ int reset_gpio;
+};
You mentioned that you'd use u64 for the resources here but did not.
+
+static void exynos_pcie_prog_viewport_cfg0(struct pcie_port *pp, u32 busdev)
+{
+ u32 val;
+ void __iomem *dbi_base = pp-dbi_base;
+
+ /* Program viewport 0 : OUTBOUND : CFG0 */
+ val = PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX0;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_VIEWPORT);
+ writel_rc(pp, (u64)pp-cfg0_base, dbi_base + PCIE_ATU_LOWER_BASE);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_BASE);
+ writel_rc(pp, (u64)pp-cfg0_base + pp-config.cfg0_size - 1,
+ dbi_base + PCIE_ATU_LIMIT);
+ writel_rc(pp, busdev, dbi_base + PCIE_ATU_LOWER_TARGET);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_TARGET);
+ writel_rc(pp, PCIE_ATU_TYPE_CFG0, dbi_base + PCIE_ATU_CR1);
+ val = PCIE_ATU_ENABLE;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_CR2);
+}
+
+static void exynos_pcie_prog_viewport_cfg1(struct pcie_port *pp, u32 busdev)
+{
+ u32 val;
+ void __iomem *dbi_base = pp-dbi_base;
+
+ /* Program viewport 1 : OUTBOUND : CFG1 */
+ val = PCIE_ATU_REGION_OUTBOUND | PCIE_ATU_REGION_INDEX1;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_VIEWPORT);
+ writel_rc(pp, PCIE_ATU_TYPE_CFG1, dbi_base + PCIE_ATU_CR1);
+ val = PCIE_ATU_ENABLE;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_CR2);
+ writel_rc(pp, (u64)pp-cfg1_base, dbi_base + PCIE_ATU_LOWER_BASE);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_BASE);
+ writel_rc(pp, (u64)pp-cfg1_base + pp-config.cfg1_size - 1,
+ dbi_base + PCIE_ATU_LIMIT);
+ writel_rc(pp, busdev, dbi_base + PCIE_ATU_LOWER_TARGET);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_TARGET);
+}
And you still don't set up the UPPER halves, which was really the point
of my comment. Same thing for all the other ones.
+static void exynos_pcie_prog_viewport_mem_inbound(struct pcie_port *pp)
+{
+ u32 val;
+ void __iomem *dbi_base = pp-dbi_base;
+
+ /* Program viewport 0 : INBOUND : MEM */
+ val = PCIE_ATU_REGION_INBOUND | PCIE_ATU_REGION_INDEX0;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_VIEWPORT);
+ writel_rc(pp, PCIE_ATU_TYPE_MEM, dbi_base + PCIE_ATU_CR1);
+ val = PCIE_ATU_ENABLE | PCIE_ATU_BAR_MODE_ENABLE;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_CR2);
+ writel_rc(pp, (u64)pp-config.mem_bus_addr,
+ dbi_base + PCIE_ATU_LOWER_BASE);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_BASE);
+ writel_rc(pp, (u64)pp-config.mem_bus_addr + pp-config.mem_size - 1,
+ dbi_base + PCIE_ATU_LIMIT);
+ writel_rc(pp, (u64)pp-mem_base, dbi_base + PCIE_ATU_LOWER_TARGET);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_TARGET);
+}
This makes even less sense than before, it seems like now you only
allow DMA between PCI devices but not to main memory.
+static void exynos_pcie_prog_viewport_io_inbound(struct pcie_port *pp)
+{
+ u32 val;
+ void __iomem *dbi_base = pp-dbi_base;
+
+ /* Program viewport 1 : INBOUND : IO */
+ val = PCIE_ATU_REGION_INBOUND | PCIE_ATU_REGION_INDEX1;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_VIEWPORT);
+ writel_rc(pp, PCIE_ATU_TYPE_IO, dbi_base + PCIE_ATU_CR1);
+ val = PCIE_ATU_ENABLE | PCIE_ATU_BAR_MODE_ENABLE;
+ writel_rc(pp, val, dbi_base + PCIE_ATU_CR2);
+ writel_rc(pp, (u64)pp-config.io_bus_addr,
+ dbi_base + PCIE_ATU_LOWER_BASE);
+ writel_rc(pp, 0, dbi_base + PCIE_ATU_UPPER_BASE);
+ writel_rc(pp, (u64)pp-config.io_bus_addr + pp-config.io_size - 1,
+ dbi_base + PCIE_ATU_LIMIT);
+ writel_rc(pp, (u64)pp-io_base, dbi_base + PCIE_ATU_LOWER_TARGET);
+ writel_rc(pp, 0,
