Re: [PATCH v5 1/5] PCI: designware: ensure ATU is enabled before IO/conf space accesses
On Fri, Dec 18, 2015 at 6:08 PM, Stanimir Varbanov wrote: > There is no guarantees that enabling ATU will hit the hardware > immediately, and subsequent accesses to configuration / IO spaces > are reliable. So fixing this by read back PCIE_ATU_CR2 register > just after writing. > > Without such a fix the PCI device enumeration during kernel boot > is not reliable, and reading configuration space for particular > PCI device on the bus returns zero aka no device. > > Signed-off-by: Stanimir Varbanov Acked-by: Pratyush Anand -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/5] PCI: designware: add memory barrier after enabling region
On Thu, Dec 17, 2015 at 9:15 PM, Stanimir Varbanov wrote: > > On 12/11/2015 06:05 AM, Pratyush Anand wrote: > > On Wed, Dec 9, 2015 at 3:53 PM, Russell King - ARM Linux > > wrote: > > > > [...] > > > >>>>> dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2); > >>>>> + /* > >>>>> + * ensure that the ATU enable has been happaned before accessing > >>>>> + * pci configuration/io spaces through dw_pcie_cfg_[read|write]. > >>>>> + */ > >>>>> + wmb(); > >>>>> } > >>>>> > >>> > >>> > >>> My understnading is that since writel() of dw_pcie_writel_rc() in > >>> above code and readl(), writel() of dw_pcie_cfg_[read|write]() (which > >>> will follow) goes through same device (ie PCIe host here). So, it is > >>> guaranteed that 1st writel() will be executed before later > >>> readl()/writel(). If that is true then we do not need any explicit > >>> barrier here. > >>> > >>> Arnd, Russel: whats your opinion here. > >> ^l > > > > Sorry :( > > > >> > >> writel() has a barrier _before_ the access but not after. > >> > >> The fact is that there's nothing which guarantees that the write will hit > >> the hardware in a timely manner (forget any rules about PCI config space, > >> the PCI ordering rules apply to the PCI bus, not to the ARM buses.) > >> > >> If you need this write to have hit the hardware before continuing, you > >> need to read back from the same register. > > > > OK, so better to replace wmb() with read back of control register. > > Would the patch be acceptable if I replace wmb with read? For me it would be fine. ~Pratyush -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/5] PCI: designware: add memory barrier after enabling region
On Wed, Dec 9, 2015 at 3:53 PM, Russell King - ARM Linux wrote: [...] >> > > dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2); >> > > + /* >> > > + * ensure that the ATU enable has been happaned before accessing >> > > + * pci configuration/io spaces through dw_pcie_cfg_[read|write]. >> > > + */ >> > > + wmb(); >> > > } >> > > >> >> >> My understnading is that since writel() of dw_pcie_writel_rc() in >> above code and readl(), writel() of dw_pcie_cfg_[read|write]() (which >> will follow) goes through same device (ie PCIe host here). So, it is >> guaranteed that 1st writel() will be executed before later >> readl()/writel(). If that is true then we do not need any explicit >> barrier here. >> >> Arnd, Russel: whats your opinion here. > ^l Sorry :( > > writel() has a barrier _before_ the access but not after. > > The fact is that there's nothing which guarantees that the write will hit > the hardware in a timely manner (forget any rules about PCI config space, > the PCI ordering rules apply to the PCI bus, not to the ARM buses.) > > If you need this write to have hit the hardware before continuing, you > need to read back from the same register. OK, so better to replace wmb() with read back of control register. > > I'm just looking at this driver, trying to decipher what it's doing. It > _looks_ to me like it's reprogramming one of the outbound windows (IO?) > so that configuration space can be accessed. Doesn't this have the > effect of disabling access to the IO segment of the PCI bus from the > host CPU? > > What protections are there against other CPUs in the system issuing a > PCI I/O read/write while this outbound window is programmed as > configuration space? Yes, that is an issue with this driver. Most of the host controller has 4 or more viewpoints, and it is very easy to handle for them. But there are few which has only two viewpoints. Do not know how to solve it, so that it works for all. ~Pratyush -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/5] PCI: designware: add memory barrier after enabling region
On Tue, Dec 8, 2015 at 2:31 PM, Stanimir Varbanov wrote: > > On 12/03/2015 03:35 PM, Stanimir Varbanov wrote: > > Add 'write memory' barrier after enable region in PCIE_ATU_CR2 > > register. The barrier is needed to ensure that the region enable > > request has been reached it's destination at time when we > > read/write to PCI configuration space. > > > > Without this barrier PCI device enumeration during kernel boot > > is not reliable, and reading configuration space for particular > > PCI device on the bus returns zero aka no device. > > Anand, Jingoo, what is your opinion? > > > > > Signed-off-by: Stanimir Varbanov > > --- > > drivers/pci/host/pcie-designware.c |5 + > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/pci/host/pcie-designware.c > > b/drivers/pci/host/pcie-designware.c > > index 02a7452bdf23..ed4dc2e2553b 100644 > > --- a/drivers/pci/host/pcie-designware.c > > +++ b/drivers/pci/host/pcie-designware.c > > @@ -164,6 +164,11 @@ static void dw_pcie_prog_outbound_atu(struct pcie_port > > *pp, int index, > > dw_pcie_writel_rc(pp, upper_32_bits(pci_addr), PCIE_ATU_UPPER_TARGET); > > dw_pcie_writel_rc(pp, type, PCIE_ATU_CR1); > > dw_pcie_writel_rc(pp, PCIE_ATU_ENABLE, PCIE_ATU_CR2); > > + /* > > + * ensure that the ATU enable has been happaned before accessing > > + * pci configuration/io spaces through dw_pcie_cfg_[read|write]. > > + */ > > + wmb(); > > } > > My understnading is that since writel() of dw_pcie_writel_rc() in above code and readl(), writel() of dw_pcie_cfg_[read|write]() (which will follow) goes through same device (ie PCIe host here). So, it is guaranteed that 1st writel() will be executed before later readl()/writel(). If that is true then we do not need any explicit barrier here. Arnd, Russel: whats your opinion here. ~Pratyush -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 0/8] Watchdog: introduce ARM SBSA watchdog driver
Hi Fu Wei, On 25/08/2015:01:01:15 AM, fu@linaro.org wrote: > From: Fu Wei > > This patchset: > (1)Introduce Documentation/devicetree/bindings/watchdog/sbsa-gwdt.txt > for FDT info of SBSA Generic Watchdog, and give two examples of > adding SBSA Generic Watchdog device node into the dts files: > foundation-v8.dts and amd-seattle-soc.dtsi. > > (2)Introduce "pretimeout" into the watchdog framework, and update > Documentation/watchdog/watchdog-kernel-api.txt to introduce: > (1)the new elements in the watchdog_device and watchdog_ops struct; > (2)the new API "watchdog_init_timeouts". > > (3)Introduce ARM SBSA watchdog driver: > a.Use linux kernel watchdog framework; > b.Work with FDT on ARM64; > c.Use "pretimeout" in watchdog framework; > d.Support getting timeout and pretimeout from parameter and FDT > at the driver init stage. > e.In the first timeout, do panic to save system context; > f.In the second stage, user can still feed the dog without > cleaning WS0. By this feature, we can avoid the panic infinite > loops, while backing up a large system context in a server. > g.In the second stage, can trigger WS1 by setting pretimeout = 0 > if necessary. > > (4)Introduce ACPI GTDT parser: drivers/acpi/gtdt.c > Parse SBSA Generic Watchdog Structure in GTDT table of ACPI, > and create a platform device with that information. > This platform device can be used by This Watchdog driver. > drivers/clocksource/arm_arch_timer.c is simplified by this GTDT support. > > This patchset has been tested with watchdog daemon > (ACPI/FDT, module/build-in) on the following platforms: > (1)ARM Foundation v8 model > I tested it with kdump on fedora-arm64 Seattle platform. I enabled watchdog using systemd (with 30s timeout), insured that watchdog is active and then crashed the system. I can see that kdump kernel loads sbsa_wdt and activates watchdog, still vmcore copy is done successfully. My test kernel is here [1] ~Pratyush [1] https://github.com/pratyushanand/linux/commits/wdt/sbsa-test-kexec -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 3/6] PCI: designware: Add ARM64 support
Hi Zhou,
On Tue, Sep 22, 2015 at 8:22 AM, Zhou Wang wrote:
> On 2015/9/15 20:49, Zhou Wang wrote:
>> This patch tries to unify ARM32 and ARM64 PCIe in designware driver. Delete
>> function dw_pcie_setup, dw_pcie_scan_bus, dw_pcie_map_irq and struct hw_pci,
>> move related operations to dw_pcie_host_init.
>>
>> This patch also try to use of_pci_get_host_bridge_resources for ARM32 and
>> ARM64
>> according to the suggestion for Gabriele[1]
>>
>> This patch reverts commit f4c55c5a3f7f ("PCI: designware: Program ATU with
>> untranslated address") based on 1/6 in this series. we delete *_mod_base in
>> pcie-designware. This was discussed in [2]
>>
>> I have compiled the driver with multi_v7_defconfig. However, I don't have
>> ARM32 PCIe related board to do test. It will be appreciated if someone could
>> help to test it.
>>
>> Signed-off-by: Zhou Wang
>> Signed-off-by: Gabriele Paoloni
>> Signed-off-by: Arnd Bergmann
>> Tested-by: James Morse
>> Tested-by: Gabriel Fernandez
>> Tested-by: Minghuan Lian
Sorry for delayed response. Since I moved from ST, I do not have any
test board having designware PCIe controller :(, and also I do
designware work only in my hobby time, so it took time to understand
few modifications. Anyway, all modifications looks fine to me. So,
Acked-by: Pratyush Anand
PS: Does someone know about a economical test board from any vendor
with designware PCIe IP?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/8] ARM64: add SBSA Generic Watchdog device node in foundation-v8.dts
On 15/09/2015:04:43:32 PM, Dave Young wrote:
> On 08/25/15 at 01:01am, fu@linaro.org wrote:
> > From: Fu Wei
> >
> > This can be a example of adding SBSA Generic Watchdog device node
> > into some dts files for the Soc which contains SBSA Generic Watchdog.
> >
> > Acked-by: Arnd Bergmann
> > Signed-off-by: Fu Wei
> > ---
> > arch/arm64/boot/dts/arm/foundation-v8.dts | 7 +++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/arm/foundation-v8.dts
> > b/arch/arm64/boot/dts/arm/foundation-v8.dts
> > index 4eac8dc..824431f 100644
> > --- a/arch/arm64/boot/dts/arm/foundation-v8.dts
> > +++ b/arch/arm64/boot/dts/arm/foundation-v8.dts
> > @@ -237,4 +237,11 @@
> > };
> > };
> > };
> > + watchdog@2a44 {
> > + compatible = "arm,sbsa-gwdt";
> > + reg = <0x0 0x2a44 0 0x1000>,
> > + <0x0 0x2a45 0 0x1000>;
> > + interrupts = <0 27 4>;
> > + timeout-sec = <10 5>;
>
> I assume 10 is timeout, 5 is pre timeout, but in the driver code the default
> value is 30/10, I think the example dts[i] should use same default values as
> in code.
>
> BTW, for kdump kernel Pratyush is working on kdump on wdt enabled system.
> Basiclly we expect one configure longer timeout, and kick it in shorter
> period so we can get a chance to save vmcore. 10s sounds too short for the
> case..
Specially if D-cache is not enabled in ARM64 kexec-tool/purgatory then its more
than 2 min. Geoff has yet not agreed [1] to take D-cache support in purgatory.
[1] https://www.mail-archive.com/kexec@lists.infradead.org/msg12881.html
~Pratyush
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 5/7] Watchdog: introduce ARM SBSA watchdog driver
On 10/09/2015:06:45:17 PM, Jon Masters wrote: > On 06/03/2015 02:53 PM, Timur Tabi wrote: > > On 06/03/2015 01:25 PM, Guenter Roeck wrote: > >> In general the idea here would be to use a crashdump kernel, which, > >> when loaded, would reset the watchdog before it fires. This kernel > >> would then write a core dump to a specified location. And this is what we do in fedora or RHEL. There had been some work ongoing in fedora [1][2] which will help to reset any active watchdog in kdump kernel(if the watchdog driver has been registered to watchdog_class). It will eventually help a watchdog on ARM64 platform as well. [1] https://lists.fedoraproject.org/pipermail/kexec/2015-September/002295.html [2] https://github.com/pratyushanand/kexec-tools/commits/watchdog_fmaster ~Pratyush -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/4] PCI: st: Provide support for the sti PCIe controller
Hi Gabriel,
Looks good to me.
On Thu, Aug 27, 2015 at 6:04 PM, Gabriel Fernandez
wrote:
> sti pcie is built around a Synopsis Designware PCIe IP.
>
> Signed-off-by: Fabrice Gasnier
> Signed-off-by: Gabriel Fernandez
> +static int st_pcie_link_up(struct pcie_port *pp)
> +{
> + u32 status;
> + int link_up;
nit: why not bool
> + int count = 0;
[...]
> +static void st_pcie_board_reset(struct pcie_port *pp)
> +{
> + struct st_pcie *pcie = to_st_pcie(pp);
> +
> + if (!gpio_is_valid(pcie->reset_gpio))
> + return;
> +
> + if (gpio_direction_output(pcie->reset_gpio, 0)) {
> + dev_err(pp->dev, "Cannot set PERST# (gpio %u) to output\n",
> + pcie->reset_gpio);
> + return;
> + }
> +
> + /* From PCIe spec */
> + msleep(2);
> + gpio_direction_output(pcie->reset_gpio, 1);
> +
> + /*
> +* PCIe specification states that you should not issue any config
> +* requests until 100ms after asserting reset, so we enforce that here
> +*/
> + msleep(100);
IIRC, specification says to wait after link training completes. So
shouldn't it be after st_pcie_enable_ltssm. Moreover, I wonder why
others do not need it.
Reviewed-by: Pratyush Anand
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 2/6] ARM/PCI: remove align_resource in pci_sys_data
On Fri, Aug 7, 2015 at 11:36 AM, Zhou Wang wrote: > [+cc jingooh...@gmail.com] > > On 2015/8/6 16:09, Zhou Wang wrote: >> From: gabriele paoloni >> >> This patch is needed in order to unify the PCIe designware framework for ARM >> and >> ARM64 architectures. In the PCIe designware unification process we are >> calling >> pci_create_root_bus() passing a "sysdata" parameter that is the same for both >> ARM and ARM64 and is of type "struct pcie_port*". In the ARM case this will >> cause a problem with the function pcibios_align_resource(); in fact this will >> cast "dev->sysdata" to "struct pci_sys_data*", whereas designware had passed >> a >> "struct pcie_port*" pointer. >> >> This patch solves the issue by removing "align_resource" from "pci_sys_data" >> struct and defining a static global function pointer in "bios32.c" >> >> Signed-off-by: Gabriele Paoloni Acked-by: Pratyush Anand -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/6] PCI: designware: move calculation of bus addresses to DRA7xx
On Fri, Aug 7, 2015 at 11:34 AM, Zhou Wang wrote: > [+cc jingooh...@gmail.com] > > On 2015/8/6 16:09, Zhou Wang wrote: >> From: gabriele paoloni >> >> Commit f4c55c5a3f7f "PCI: designware: Program ATU with untranslated >> address" added the calculation of PCI BUS addresses in designware, >> storing them in new fields added in "struct pcie_port". This >> calculation is done for every designware user even if is only >> applicable to DRA7xx. >> This patch moves the calculation of the bus addresses to the DRA7xx >> driver and is needed to allow the rework of designware to use >> the new DT parsing API. >> >> Signed-off-by: Gabriele Paoloni >> Signed-off-by: Zhou Wang Looks fine to me Acked-by: Pratyush Anand -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/4] PCI: designware: Add ARM64 support
Hi Zhou Wang,
Thanks for unifying arm and arm64 code.
On Wed, Jun 3, 2015 at 2:05 PM, Zhou Wang wrote:
>
> This patch tries to unify ARM32 and ARM64 PCIe in designware driver. Delete
> function dw_pcie_setup, dw_pcie_scan_bus, dw_pcie_map_irq and struct hw_pci,
> move related operations to dw_pcie_host_init. Also set pp->root_bus_nr = 0 in
> each PCIe host driver which is based on pcie-designware.
>
> I am not very clear about I/O resource management:
Following discussion may help to understand it, specially in the
context of designware.
http://marc.info/?l=linux-pci&m=138621989417562&w=2
> > if (global_io_offset < SZ_1M && pp->io_size > 0) {
> > pci_ioremap_io(global_io_offset, pp->io_base);
> > global_io_offset += SZ_64K;
> > pci_add_resource_offset(&res, &pp->io,
> > global_io_offset - pp->io_bus_addr);
> > }
> so just move steps in dw_pcie_setup to dw_pcie_host_init.
>
> I have compiled the driver with multi_v7_defconfig. However, I don't have
> ARM32 PCIe related board to do test. It will be appreciated if someone could
> help to test it.
>
> Signed-off-by: Zhou Wang
> Tested-by: Fabrice Gasnier
> ---
> drivers/pci/host/pci-dra7xx.c | 1 +
> drivers/pci/host/pci-exynos.c | 2 +-
> drivers/pci/host/pci-imx6.c| 2 +-
> drivers/pci/host/pci-keystone.c| 2 +-
> drivers/pci/host/pci-layerscape.c | 2 +-
> drivers/pci/host/pcie-designware.c | 128
> +++--
> drivers/pci/host/pcie-spear13xx.c | 2 +-
> 7 files changed, 56 insertions(+), 83 deletions(-)
>
[...]
> diff --git a/drivers/pci/host/pci-layerscape.c
> b/drivers/pci/host/pci-layerscape.c
> index 4a6e62f..5c7a9c4 100644
> --- a/drivers/pci/host/pci-layerscape.c
> +++ b/drivers/pci/host/pci-layerscape.c
> @@ -101,7 +101,7 @@ static int ls_add_pcie_port(struct ls_pcie *pcie)
> pp = &pcie->pp;
> pp->dev = pcie->dev;
> pp->dbi_base = pcie->dbi;
> - pp->root_bus_nr = -1;
> + pp->root_bus_nr = 0;
similar change for pcie-spear13xx.c should be needed as well.
> pp->ops = &ls_pcie_host_ops;
>
> ret = dw_pcie_host_init(pp);
> diff --git a/drivers/pci/host/pcie-designware.c
> b/drivers/pci/host/pcie-designware.c
> index 2e9f84f..b3f0ac7 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
[...]
> -#ifdef CONFIG_PCI_MSI
> - dw_pcie_msi_chip.dev = pp->dev;
> - dw_pci.msi_ctrl = &dw_pcie_msi_chip;
> +#ifdef CONFIG_ARM
> + /*
> +* FIXME: we should really be able to use
> +* of_pci_get_host_bridge_resources on arm32 as well,
> +* but the conversion needs some more testing
> +*/
> + if (global_io_offset < SZ_1M && pp->io_size > 0) {
> + pci_ioremap_io(global_io_offset, pp->io_base);
> + global_io_offset += SZ_64K;
> + pci_add_resource_offset(&res, &pp->io,
> + global_io_offset - pp->io_bus_addr);
> + }
> + pci_add_resource_offset(&res, &pp->mem,
> + pp->mem.start - pp->mem_bus_addr);
> + pci_add_resource(&res, &pp->busn);
> +#else
> + ret = of_pci_get_host_bridge_resources(np, 0, 0xff, &res,
> &pp->io_base);
> + if (ret)
> + return ret;
> +#endif
> +
> + bus = pci_create_root_bus(pp->dev, pp->root_bus_nr, &dw_pcie_ops,
> + pp, &res);
> + if (!bus)
> + return -ENOMEM;
> +
> +#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN
> + bus->msi = container_of(&pp->irq_domain, struct msi_controller,
> domain);
> +#else
> + bus->msi = &dw_pcie_msi_chip;
> #endif
>
> - dw_pci.nr_controllers = 1;
> - dw_pci.private_data = (void **)&pp;
> + pci_scan_child_bus(bus);
> + if (pp->ops->scan_bus)
> + pp->ops->scan_bus(pp);
>
> - pci_common_init_dev(pp->dev, &dw_pci);
> +#ifdef CONFIG_ARM
> + /* support old dtbs that incorrectly describe IRQs */
> + pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
> +#endif
> +
> + pci_assign_unassigned_bus_resources(bus);
> + pci_bus_add_devices(bus);
As James and Lorenzo has suggested, of_pci_get_host_bridge_resources should
work for ARM as well. I would suggest to move that just after cfg
resource parsing and
then to remove following piece of code.
for_each_of_pci_range(&parser, &range) {
...
}
ret = of_pci_parse_bus_range(np, &pp->busn);
Then you can have a loop like
list_for_each_entry(entry, &res, node) {
struct resource *res_temp = entry->res;
if (resource_type(res_temp) == IORESOURCE_IO) {
} else if (resource_type(res_temp) == IORESOURCE_MEM) {
}
}
where you can fill, xx_size, xx_bus_addr, xx_mod_base etc.
You can also remove global_io_offset variable.
Re: [PATCH] PCIe: Designware: Do not allow config space through ranges
On Monday 22 December 2014 02:00 PM, Arnd Bergmann wrote:
On Monday 22 December 2014 12:09:04 Pratyush Anand wrote:
Config space is need to be allocated using reg property. This patch
removes possibility of config space allocation through ranges property.
To: Mohit Kumar
To: Jingo Han
To: Lucas Stach
To: Shawn Guo
Cc: linux-...@vger.kernel.org
Cc: devicetree@vger.kernel.org
Signed-off-by: Pratyush Anand
---
arch/arm/boot/dts/imx6qdl.dtsi | 3 +--
arch/arm/boot/dts/imx6sx.dtsi | 8
changing the DT files is probably ok.
diff --git a/drivers/pci/host/pcie-designware.c
b/drivers/pci/host/pcie-designware.c
index df781cdf13c1..0b22b42e1ff9 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -409,19 +409,6 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
pp->mem_mod_base = of_read_number(parser.range -
parser.np + na, ns);
}
- if (restype == 0) {
- of_pci_range_to_resource(&range, np, &pp->cfg);
- pp->cfg0_size = resource_size(&pp->cfg)/2;
- pp->cfg1_size = resource_size(&pp->cfg)/2;
- pp->cfg0_base = pp->cfg.start;
- pp->cfg1_base = pp->cfg.start + pp->cfg0_size;
-
- /* Find the untranslated configuration space address */
- pp->cfg0_mod_base = of_read_number(parser.range -
- parser.np + na, ns);
- pp->cfg1_mod_base = pp->cfg0_mod_base +
- pp->cfg0_size;
- }
}
Since we've allowed this for so long, it may be better to leave this
in for compatibility with old dts files. It would however be good to
print a warning message. We could also limit it to those machines that
are known to have been used with this property in the past.
I think, only imx6 is using "config from ranges", for which I made the
change in this patch. Modification need to be tested and acked by IMX6
maintainers.
I do not see any other driver using it. However, I have kept all
designware user in CC here.
~Pratyush
Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] PCIe: Designware: Do not allow config space through ranges
Config space is need to be allocated using reg property. This patch
removes possibility of config space allocation through ranges property.
To: Mohit Kumar
To: Jingo Han
To: Lucas Stach
To: Shawn Guo
Cc: linux-...@vger.kernel.org
Cc: devicetree@vger.kernel.org
Signed-off-by: Pratyush Anand
---
arch/arm/boot/dts/imx6qdl.dtsi | 3 +--
arch/arm/boot/dts/imx6sx.dtsi | 8
drivers/pci/host/pci-imx6.c| 2 +-
drivers/pci/host/pcie-designware.c | 13 -
4 files changed, 6 insertions(+), 20 deletions(-)
diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
index 4fc03b7f1cee..f21570847d08 100644
--- a/arch/arm/boot/dts/imx6qdl.dtsi
+++ b/arch/arm/boot/dts/imx6qdl.dtsi
@@ -143,8 +143,7 @@
#address-cells = <3>;
#size-cells = <2>;
device_type = "pci";
- ranges = <0x0800 0 0x01f0 0x01f0 0
0x0008 /* configuration space */
- 0x8100 0 0 0x01f8 0
0x0001 /* downstream I/O */
+ ranges = 0x8100 0 0 0x01f8 0
0x0001 /* downstream I/O */
0x8200 0 0x0100 0x0100 0
0x00f0>; /* non-prefetchable memory */
num-lanes = <1>;
interrupts = ;
diff --git a/arch/arm/boot/dts/imx6sx.dtsi b/arch/arm/boot/dts/imx6sx.dtsi
index 7a24fee1e7ae..72593a77455e 100644
--- a/arch/arm/boot/dts/imx6sx.dtsi
+++ b/arch/arm/boot/dts/imx6sx.dtsi
@@ -1197,14 +1197,14 @@
pcie: pcie@0x0800 {
compatible = "fsl,imx6sx-pcie", "snps,dw-pcie";
- reg = <0x08ffc000 0x4000>; /* DBI */
+ reg = <0x08ffc000 0x4000>, /* DBI */
+ <0x08f0 0x8>;
+ reg-names = "dbi", "config";
#address-cells = <3>;
#size-cells = <2>;
device_type = "pci";
- /* configuration space */
- ranges = <0x0800 0 0x08f0 0x08f0 0
0x0008
/* downstream I/O */
- 0x8100 0 0 0x08f8 0
0x0001
+ ranges = 0x8100 0 0 0x08f8 0
0x0001
/* non-prefetchable memory */
0x8200 0 0x0800 0x0800 0
0x00f0>;
num-lanes = <1>;
diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index fdb95367721e..dae452984ed3 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -572,7 +572,7 @@ static int __init imx6_pcie_probe(struct platform_device
*pdev)
hook_fault_code(16 + 6, imx6q_pcie_abort_handler, SIGBUS, 0,
"imprecise external abort");
- dbi_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
pp->dbi_base = devm_ioremap_resource(&pdev->dev, dbi_base);
if (IS_ERR(pp->dbi_base))
return PTR_ERR(pp->dbi_base);
diff --git a/drivers/pci/host/pcie-designware.c
b/drivers/pci/host/pcie-designware.c
index df781cdf13c1..0b22b42e1ff9 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -409,19 +409,6 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
pp->mem_mod_base = of_read_number(parser.range -
parser.np + na, ns);
}
- if (restype == 0) {
- of_pci_range_to_resource(&range, np, &pp->cfg);
- pp->cfg0_size = resource_size(&pp->cfg)/2;
- pp->cfg1_size = resource_size(&pp->cfg)/2;
- pp->cfg0_base = pp->cfg.start;
- pp->cfg1_base = pp->cfg.start + pp->cfg0_size;
-
- /* Find the untranslated configuration space address */
- pp->cfg0_mod_base = of_read_number(parser.range -
- parser.np + na, ns);
- pp->cfg1_mod_base = pp->cfg0_mod_base +
- pp->cfg0_size;
- }
}
ret = of_pci_parse_bus_range(np, &pp->busn);
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] PCI: st: Provide support for the sti PCIe controller
On Wednesday 17 December 2014 04:04 PM, Gabriel FERNANDEZ wrote:
sti pcie is built around a Synopsis Designware PCIe IP.
Signed-off-by: Fabrice Gasnier
Signed-off-by: Gabriel Fernandez
---
drivers/pci/host/Kconfig | 5 +
drivers/pci/host/Makefile | 1 +
drivers/pci/host/pci-st.c | 713 ++
3 files changed, 719 insertions(+)
create mode 100644 drivers/pci/host/pci-st.c
[...]
+static int st_pcie_abort_handler(unsigned long addr, unsigned int fsr,
+struct pt_regs *regs)
+{
+ return 0;
+}
+
You should have modification here to populate registers having cfg read
values by 0x.
I would suggest to have a look here:
drivers/pci/host/pci-keystone.c:keystone_pcie_fault
+/*
+ * The PCI express core IP expects the following arrangement on it's address
+ * bus (slv_haddr) when driving config cycles.
+ * bus_number [31:24]
+ * dev_number [23:19]
+ * func_number [18:16]
+ * unused [15:12]
+ * ext_reg_number [11:8]
+ * reg_number [7:2]
+ *
+ * Bits [15:12] are unused.
+ *
+ * In the glue logic there is a 64K region of address space that can be
+ * written/read to generate config cycles. The base address of this is
+ * controlled by CFG_BASE_ADDRESS. There are 8 16 bit registers called
+ * FUNC0_BDF_NUM to FUNC8_BDF_NUM. These split the bottom half of the 64K
+ * window into 8 regions at 4K boundaries. These control the bus,device and
+ * function number you are trying to talk to.
+ *
+ * The decision on whether to generate a type 0 or type 1 access is controlled
+ * by bits 15:12 of the address you write to. If they are zero, then a type 0
+ * is generated, if anything else it will be a type 1. Thus the bottom 4K
+ * region controlled by FUNC0_BDF_NUM can only generate type 0, all the others
+ * can only generate type 1.
+ *
+ * We only use FUNC0_BDF_NUM and FUNC1_BDF_NUM. Which one you use is selected
+ * by bit 12 of the address you write to. The selected register is then used
+ * for the top 16 bits of the slv_haddr to form the bus/dev/func, bit 15:12 are
+ * wired to zero, and bits 11:2 form the address of the register you want to
+ * read in config space.
+ *
+ * We always write FUNC0_BDF_NUM as a 32 bit write. So if we want type 1
+ * accesses we have to shift by 16 so in effect we are writing to FUNC1_BDF_NUM
+ */
+static inline u32 bdf_num(int bus, int devfn, int is_root_bus)
+{
+ return ((bus << 8) | devfn) << (is_root_bus ? 0 : 16);
+}
+
+static inline unsigned config_addr(int where, int is_root_bus)
+{
+ return (where & ~3) | (!is_root_bus << 12);
+}
+
+static int st_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
+unsigned int devfn, int where, int size,
+u32 *val)
+{
+ u32 data;
+ u32 bdf;
+ struct st_pcie *pcie = to_st_pcie(pp);
+ int is_root_bus = pci_is_root_bus(bus);
+ int retry_count = 0;
+ int ret;
+ void __iomem *addr;
+
+ /*
+* Prerequisite
+* PCI express devices will respond to all config type 0 cycles, since
+* they are point to point links. Thus to avoid probing for multiple
+* devices on the root, dw-pcie already check for us if it is on the
+* root bus / other slots. Also, dw-pcie checks for the link being up
+* as we will hang if we issue a config request and the link is down.
+* A switch will reject requests for slots it knows do not exist.
+*/
+ bdf = bdf_num(bus->number, devfn, is_root_bus);
+ addr = pcie->config_area + config_addr(where,
+ bus->parent->number == pp->root_bus_nr);
+retry:
+ /* Set the config packet devfn */
+ writel_relaxed(bdf, pp->dbi_base + FUNC0_BDF_NUM);
+ readl_relaxed(pp->dbi_base + FUNC0_BDF_NUM);
+
+ ret = dw_pcie_cfg_read(addr, where, size, &data);
+
+ /*
+* This is intended to help with when we are probing the bus. The
+* problem is that the wrapper logic doesn't have any way to
+* interrogate if the configuration request failed or not.
+* On the ARM we actually get a real bus error.
+*
+* Unfortunately this means it is impossible to tell the difference
+* between when a device doesn't exist (the switch will return a UR
+* completion) or the device does exist but isn't yet ready to accept
+* configuration requests (the device will return a CRS completion)
+*
+* The result of this is that we will miss devices when probing.
+*
+* So if we are trying to read the dev/vendor id on devfn 0 and we
+* appear to get zero back, then we retry the request. We know that
+* zero can never be a valid device/vendor id. The specification says
+* we must retry for up to a second before we decide the device is
Not sure if re
Re: [PATCH 2/5] PCI: st: Add Device Tree bindings for sti pcie
On Wednesday 17 December 2014 04:04 PM, Gabriel FERNANDEZ wrote:
sti pcie is built around a Synopsis Designware PCIe IP.
Signed-off-by: Fabrice Gasnier
Signed-off-by: Gabriel Fernandez
---
Documentation/devicetree/bindings/pci/st-pcie.txt | 53 +++
1 file changed, 53 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pci/st-pcie.txt
diff --git a/Documentation/devicetree/bindings/pci/st-pcie.txt
b/Documentation/devicetree/bindings/pci/st-pcie.txt
new file mode 100644
index 000..bd3488f
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/st-pcie.txt
@@ -0,0 +1,53 @@
+STMicroelectronics STi PCIe controller
+
+This PCIe host controller is based on the Synopsis Designware PCIe IP
+and thus inherits all the common properties defined in designware-pcie.txt.
+
+Required properties:
+ - compatible: "st,stih407-pcie"
+ - reg: base address and length of the pcie controller, mem-window address
+ and length available to the controller.
+ - interrupts: A list of interrupt outputs of the controller.
+ - interrupt-names: Must include the following entries:
+ "msi": STi interrupt that is asserted when an MSI is received
+ - st,syscfg : should be a phandle of the syscfg node. Also contains syscfg
+ offset for IP configuration.
+ - resets, reset-names: the power-down and soft-reset lines of PCIe IP.
+ Associated names must be "powerdown" and "softreset".
+ - phys, phy-names: the phandle for the PHY device.
+ Associated name must be "pcie_phy"
+
+Optional properties:
+ - reset-gpio: a GPIO spec to define which pin is connected to the bus reset.
+
+Example:
+
+pcie0: pcie@9b0 {
+ compatible = "st,stih407-pcie", "snps,dw-pcie";
+ device_type = "pci";
+ reg = <0x09b0 0x4000>,/* dbi cntrl registers */
+ <0x2fff 0x0001>,/* configuration space */
+ <0x4000 0x8000>;/* lmi mem window */
+ reg-names = "dbi", "config", "mem-window";
+ st,syscfg = <&syscfg_core 0xd8 0xe0>;
+ #address-cells = <3>;
+ #size-cells = <2>;
+ ranges = <0x0800 0 0x2fff 0x2fff 0 0x0001 /*
configuration space */
Pass it through reg property.
See: arch/arm/boot/dts/spear1310.dtsi or any other dw pcie's dtsi.
@Jingoo, Mohit: I would suggest following changes so that no upcoming
platform can add it through ranges.
diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
index 4fc03b7f1cee..f21570847d08 100644
--- a/arch/arm/boot/dts/imx6qdl.dtsi
+++ b/arch/arm/boot/dts/imx6qdl.dtsi
@@ -143,8 +143,7 @@
#address-cells = <3>;
#size-cells = <2>;
device_type = "pci";
- ranges = <0x0800 0 0x01f0 0x01f0 0
0x0008 /* configuration space */
- 0x8100 0 0 0x01f8 0
0x0001 /* downstream I/O */
+ ranges = 0x8100 0 0 0x01f8 0
0x0001 /* downstream I/O */
0x8200 0 0x0100 0x0100 0
0x00f0>; /* non-prefetchable memory */
num-lanes = <1>;
interrupts = ;
diff --git a/arch/arm/boot/dts/imx6sx.dtsi b/arch/arm/boot/dts/imx6sx.dtsi
index 7a24fee1e7ae..72593a77455e 100644
--- a/arch/arm/boot/dts/imx6sx.dtsi
+++ b/arch/arm/boot/dts/imx6sx.dtsi
@@ -1197,14 +1197,14 @@
pcie: pcie@0x0800 {
compatible = "fsl,imx6sx-pcie", "snps,dw-pcie";
- reg = <0x08ffc000 0x4000>; /* DBI */
+ reg = <0x08ffc000 0x4000>, /* DBI */
+ <0x08f0 0x8>;
+ reg-names = "dbi", "config";
#address-cells = <3>;
#size-cells = <2>;
device_type = "pci";
- /* configuration space */
- ranges = <0x0800 0 0x08f0 0x08f0 0
0x0008
/* downstream I/O */
- 0x8100 0 0 0x08f8 0
0x0001
+ ranges = 0x8100 0 0 0x08f8 0
0x0001
/* non-prefetchable memory */
0x8200 0 0x0800 0x0800 0
0x00f0>;
num-lanes = <1>;
diff --git a/drivers/pci/host/pcie-designware.c
b/drivers/pci/host/pcie-designware.c
index df781cdf13c1..0b22b42e1ff9 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -409,19 +409,6 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
pp->mem_mod_base = of_read_number(parser.range -
parser.np +
na, ns);
}
- if (restype == 0) {
-
Re: [PATCH 0/3] dra7: Add PCIe support
On Sat, Jul 5, 2014 at 11:02 PM, Bjorn Helgaas wrote: > On Wed, Jun 25, 2014 at 11:26:44PM +0530, Kishon Vijay Abraham I wrote: >> [1] is split into separate series in order for individual subsystem >> Maintainers to pick up the patches. This series handles the PCIe >> support for DRA7. >> >> Rebased to 3.16-rc2. >> >> [1] -> https://lkml.org/lkml/2014/5/29/258 >> >> Kishon Vijay Abraham I (3): >> PCI: designware: Configuration space should be specified in 'reg' >> PCI: designware: use untranslated address while programming ATU >> PCI: host: pcie-dra7xx: add support for pcie-dra7xx controller > > Mohit, I see your ack for [1/3], but not for [2/3]; are you OK with that, > too? > > Pratyush, you had some questions about [2/3]; are you happy with that one? OK for me. ~Pratyush -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] PCI: designware: use untranslated address while programming ATU
Hi Kishon,
On Thu, Jun 26, 2014 at 02:10:02PM +0800, Kishon Vijay Abraham I wrote:
> Hi Pratyush,
>
> On Thursday 26 June 2014 11:07 AM, Pratyush Anand wrote:
> > Hi Kishon,
> >
> > Few things, if you can help me to understand:
> >
> > On Wed, Jun 25, 2014 at 11:26 PM, Kishon Vijay Abraham I
> > wrote:
> >> In DRA7, the cpu sees 32bit address, but the pcie controller can see only
> >> 28bit
> >> address. So whenever the cpu issues a read/write request, the 4 most
> >> significant bits are used by L3 to determine the target controller.
> >> For example, the cpu reserves 0x2000_ - 0x2FFF_ for PCIe
> >> controller but
> >> the PCIe controller will see only (0x000_ - 0xFFF_FFF). So for
> >> programming
> >> the outbound translation window the *base* should be programmed as
> >> 0x000_.
> >> Whenever we try to write to say 0x2000_, it will be translated to
> >> whatever
> >> we have programmed in the translation window with base as 0x000_.
> >>
> >> This is needed when the dt node is modelled something like below
> >> axi {
> >> compatible = "simple-bus";
> >> #size-cells = <1>;
> >> #address-cells = <1>;
> >> ranges = <0x00x2000 0x1000 // 28-bit bus
> >> 0x5100 0x5100 0x3000>;
> >> pcie@5100 {
> >> reg = <0x1000 0x2000>, <0x51002000 0x14c>, <0x5100
> >> 0x2000>;
> >> reg-names = "config", "ti_conf", "rc_dbics";
> >
> > So for DRA7, config base which will be coming from reg property should
> > be 0x1000 and size 0x2000, no?
>
> right. The first element in 'reg' and 'reg-names' specify exactly that.
> >
> >> #address-cells = <3>;
> >> #size-cells = <2>;
> >> ranges = <0x8100 0 0 0x03000 0 0x0001
> >
> > range type 0x8100 tells that it is IO
> >
> >> 0x8200 0 0x20013000 0x13000 0 0xffed000>;
> >
> > range type 0x8100 tells that it is mem
> >
> >
> >> };
> >> };
> >>
> >> Here the CPU address for configuration space is 0x20013000 and the
> >> controller
> >> address for configuration space is 0x13000. The controller address should
> > be
> >
> > If above understanding is correct then:
> >
> > Aren't these addresses(0x20013000 and 0x13000) from mem space
> > instead of configuration space.
>
> Sorry. I didn't get you. Configuration space is different from mem space and
> IO
> space. We specify only the configuration space in "reg", the IO space and
> memory space should be specified in ranges.
>
> In my case
> configuration space range: 0x20001000 - 0x20002fff
> IO space range:0x20003000 - 0x20012fff
> Mem space range: 0x20013000 - 0x2fff
>
> Here only the configuration space is obtained from 'reg' and 'IO' and 'MEM'
> space is obtained from ranges.
> >
> > If yes, then how can you get two addresses (CPU and Controller address)
> > from reg property for configuration space?
>
> I used platform_get_resource_byname() to get CPU address and of_get_address()
> to get the untranslated controller address.
This is what I am not able to understand that how does
platform_get_resource_byname gives correct CPU address from reg =
<0x1000 0x2000>?
cfg_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config");
how cfg_res->start is now 0x20001000? Shouldn't cfg_res->start be
0x1000?
What am I missing?
Thanks for explaining.
~Pratyush
> Thanks
> Kishon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] PCI: designware: use untranslated address while programming ATU
Hi Kishon,
Few things, if you can help me to understand:
On Wed, Jun 25, 2014 at 11:26 PM, Kishon Vijay Abraham I wrote:
> In DRA7, the cpu sees 32bit address, but the pcie controller can see only
> 28bit
> address. So whenever the cpu issues a read/write request, the 4 most
> significant bits are used by L3 to determine the target controller.
> For example, the cpu reserves 0x2000_ - 0x2FFF_ for PCIe controller
> but
> the PCIe controller will see only (0x000_ - 0xFFF_FFF). So for programming
> the outbound translation window the *base* should be programmed as 0x000_.
> Whenever we try to write to say 0x2000_, it will be translated to whatever
> we have programmed in the translation window with base as 0x000_.
>
> This is needed when the dt node is modelled something like below
> axi {
> compatible = "simple-bus";
> #size-cells = <1>;
> #address-cells = <1>;
> ranges = <0x00x2000 0x1000 // 28-bit bus
> 0x5100 0x5100 0x3000>;
> pcie@5100 {
> reg = <0x1000 0x2000>, <0x51002000 0x14c>, <0x5100
> 0x2000>;
> reg-names = "config", "ti_conf", "rc_dbics";
So for DRA7, config base which will be coming from reg property should
be 0x1000 and size 0x2000, no?
> #address-cells = <3>;
> #size-cells = <2>;
> ranges = <0x8100 0 0 0x03000 0 0x0001
range type 0x8100 tells that it is IO
> 0x8200 0 0x20013000 0x13000 0 0xffed000>;
range type 0x8100 tells that it is mem
> };
> };
>
> Here the CPU address for configuration space is 0x20013000 and the controller
> address for configuration space is 0x13000. The controller address should
be
If above understanding is correct then:
Aren't these addresses(0x20013000 and 0x13000) from mem space
instead of configuration space.
If yes, then how can you get two addresses (CPU and Controller address)
from reg property for configuration space?
~Pratyush
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/8] Add Keystone PCIe controller driver
On Sat, Jun 21, 2014 at 03:05:30AM +0800, Arnd Bergmann wrote: > On Friday 20 June 2014 13:11:37 Santosh Shilimkar wrote: > > > > > Arnd suggestion was to have the version 3.65 code in generic place since > > its IP specific and just in case some other vendor using the same version > > can leverage the code. Sorry, I do not follow PCIe mailing list these days, doing something else now. So coming to this topic a bit delayed. > > > > Concern here seems toe really those name of the files. I can't think of > > any other appropriate name. > > We should definitely keep the version in the DT "compatible" strings > wherever we know it. Regarding a better file name, I have no idea. In my opinion, we do not need any of dw-v3_65 files, as code in these files will not be usable by other vendors. Anything which is implemented in application space, will not be same across all IP users. For example, MSI0_IRQ_ENABLE_SET has been defined at offset 0x108 in keystone PCIe application space.Other vendor may not have this register at the same offset. Moreover, other vendors are not even obliged to implement MSI Enable signals in same way, so internal bit definition of the register may change. Therefore code is not reusable if all register offset and bit definitions are not same across vendors. So, in case of DW driver none of the code which are accessed using va_app_base should go to common area. Regards Pratyush > > Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/8] Add Keystone PCIe controller driver
On Sat, Jun 21, 2014 at 05:17:07AM +0800, Murali Karicheri wrote: > > Sorry, my previous response was in html and not sure it has made to the > list. I did > get an error as well. So resending my response. > > On 6/18/2014 6:14 AM, Mohit KUMAR DCG wrote: > > Hello Murali, > > > [...] > *pos = pos0; > > @@ -349,7 +353,10 @@ static int dw_msi_setup_irq(struct msi_chip *chip, > struct pci_dev *pdev, > > */ > > desc->msi_attrib.multiple = msgvec; > > -msg.address_lo = virt_to_phys((void *)pp->msi_data); > > +if (pp->ops->get_msi_data) > > +msg.address_lo = pp->ops->get_msi_data(pp); > > +else > > +msg.address_lo = virt_to_phys((void *)pp->msi_data); > > msg.address_hi = 0x0; > > msg.data = pos; > > > What about this code? This requires get_msi_data() as well pp->msi_data is set in dw_pcie_msi_init, which is a global function called from vendor specific code. You can have your own keystone_pcie_msi_init and then you do not need above changes. > > > -- 3rd to use pp->ops->msi_set/clear if defined. > Why not API enhancement and refactor the code in a single patch? Yes, can be. You can send changes in 2 or 3 patches as you wish, but I believe that should be able to solve problem in best way. Regards Pratyush > > Murali > > Pls let us know for any issue or have different opinion. > > > > Regards > > Mohit > > > > > > > > > >> -- > >> 1.7.9.5 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/8] PCI: designware: refactor host init code to re-use on v3.65 DW pci hw
On Sat, Jun 21, 2014 at 02:47:27AM +0800, Murali Karicheri wrote:
> On 6/18/2014 3:05 AM, Pratyush Anand wrote:
> > Hi Murali,
> >
> > On Wed, Jun 11, 2014 at 02:51:21AM +0800, Murali Karicheri wrote:
[...]
> Pratyush,
>
> Thanks for the comments.
>
> This is IP specific code and another driver that has this version of the
> IP will be able to
> re-use the code. This is also being discussed in another thread from
> Bjorn and Jingoo.
Anything which is implemented in application space, will not be same
across all IP users. For example, you have MSI0_IRQ_ENABLE_SET at
offset 0x108 in application space.Other vendor may not have this
register at the same offset. Moreover, other vendors are not even
obliged to implement MSI Enable signals in same way, so internal bit
definition of the register may change.
In summary: Code is only reusable if all register offset and bit
definitions are same across vendors. In case of DW driver none of the
code which are accessed using va_app_base should go to common area.
> Please discuss this in that thread.
Sorry, I am not following pci mailing list. Doing something else these
days. So if you can help me with the thread link.
Regards
Pratyush
>
> >> initialization code. So refactor the msi host controller code into a
> >> separate function. Other common functions across both hw versions
> >> are moved to dw_pcie_common_host_init() and called from the newer and
> >> v3.65 hw host initialization code.
> >>
> >> Signed-off-by: Murali Karicheri
> >>
> >> CC: Santosh Shilimkar
> >> CC: Russell King
> >> CC: Grant Likely
> >> CC: Rob Herring
> >> CC: Mohit Kumar
> >> CC: Jingoo Han
> >> CC: Bjorn Helgaas
> >> CC: Pratyush Anand
> >> CC: Richard Zhu
> >> CC: Kishon Vijay Abraham I
> >> CC: Marek Vasut
> >> CC: Arnd Bergmann
> >> CC: Pawel Moll
> >> CC: Mark Rutland
> >> CC: Ian Campbell
> >> CC: Kumar Gala
> >> CC: Randy Dunlap
> >> CC: Grant Likely
> >>
> >> ---
> >> drivers/pci/host/pcie-designware.c | 136
> >> ++--
> >> 1 file changed, 101 insertions(+), 35 deletions(-)
> >>
> >> diff --git a/drivers/pci/host/pcie-designware.c
> >> b/drivers/pci/host/pcie-designware.c
> >> index e8f5d8d..e4bd19a 100644
> >> --- a/drivers/pci/host/pcie-designware.c
> >> +++ b/drivers/pci/host/pcie-designware.c
> >> @@ -389,13 +389,19 @@ static const struct irq_domain_ops msi_domain_ops = {
> >>.map = dw_pcie_msi_map,
> >> };
> >>
> >> -int __init dw_pcie_host_init(struct pcie_port *pp)
> >> +/*
> >> + * dw_pcie_parse_resource() - Function to parse common resources
> >> + *
> >> + * @pp: ptr to pcie port
> >> + *
> >> + * Parse the range property for MEM, IO and cfg resources, and map
> >> + * the cfg register space.
> >> + */
> > Why do you need this function? If you have some extra resource, you
> > can ioremap that before you call dw_pcie_host_init.
>
> I assume you are referring to dw_pcie_parse_resource(). Keystone PCIe
> driver needs
> this to parse the range property for IORESOURCE_MEM and IORESOURCE_IO
> resources.
> So refactored this into a function and called from host_init()
> code for v3.65 and dw_pcie_host_init . But for Keystone PCI driver, no
> need to ioremap
> cfg1 and cfg2 as it doesn't have ATU ports. So host_init() code has to
> be little different.
> Idea is to have IP specific host_init() and refactor and re-use the
> common code on both.
>
> >
> >> +int __init dw_pcie_parse_resource(struct pcie_port *pp)
> >> {
> >>struct device_node *np = pp->dev->of_node;
> >>struct of_pci_range range;
> >>struct of_pci_range_parser parser;
> >> - u32 val;
> >> - int i;
> >>
> >>if (of_pci_range_parser_init(&parser, np)) {
> >>dev_err(pp->dev, "missing ranges property\n");
> >> @@ -431,41 +437,29 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
> >>pp->config.cfg1_size = resource_size(&pp->cfg)/2;
> >>}
> >>}
> >> -
> >> - if (!pp->dbi_base) {
> >> - pp->dbi_base = devm_ioremap(pp->dev, pp->cfg.start,
> >> - resource_size(&pp->cfg));
> >> - if (!pp->dbi_base) {
> >> - dev_err(pp->dev, &
Re: [PATCH v2 2/8] PCI: designware: refactor host init code to re-use on v3.65 DW pci hw
Hi Murali,
On Wed, Jun 11, 2014 at 02:51:21AM +0800, Murali Karicheri wrote:
> Current DW PCI host init code has code specific to newer hw such as
> ATU port specific resource parsing and map. v3.65 DW PCI host has
OK, Older version did not had standard viewport implementation, so patch 1
of this series will help you to take care for that.
> MSI controller in application space and requires different controller
Since MSI controller is implemented in application space, so this may
not be same for different older version dw controller users.
Therefore, I would suggest to implement all application specific code
in your keystone driver only.
> initialization code. So refactor the msi host controller code into a
> separate function. Other common functions across both hw versions
> are moved to dw_pcie_common_host_init() and called from the newer and
> v3.65 hw host initialization code.
>
> Signed-off-by: Murali Karicheri
>
> CC: Santosh Shilimkar
> CC: Russell King
> CC: Grant Likely
> CC: Rob Herring
> CC: Mohit Kumar
> CC: Jingoo Han
> CC: Bjorn Helgaas
> CC: Pratyush Anand
> CC: Richard Zhu
> CC: Kishon Vijay Abraham I
> CC: Marek Vasut
> CC: Arnd Bergmann
> CC: Pawel Moll
> CC: Mark Rutland
> CC: Ian Campbell
> CC: Kumar Gala
> CC: Randy Dunlap
> CC: Grant Likely
>
> ---
> drivers/pci/host/pcie-designware.c | 136
> ++--
> 1 file changed, 101 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-designware.c
> b/drivers/pci/host/pcie-designware.c
> index e8f5d8d..e4bd19a 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -389,13 +389,19 @@ static const struct irq_domain_ops msi_domain_ops = {
> .map = dw_pcie_msi_map,
> };
>
> -int __init dw_pcie_host_init(struct pcie_port *pp)
> +/*
> + * dw_pcie_parse_resource() - Function to parse common resources
> + *
> + * @pp: ptr to pcie port
> + *
> + * Parse the range property for MEM, IO and cfg resources, and map
> + * the cfg register space.
> + */
Why do you need this function? If you have some extra resource, you
can ioremap that before you call dw_pcie_host_init.
> +int __init dw_pcie_parse_resource(struct pcie_port *pp)
> {
> struct device_node *np = pp->dev->of_node;
> struct of_pci_range range;
> struct of_pci_range_parser parser;
> - u32 val;
> - int i;
>
> if (of_pci_range_parser_init(&parser, np)) {
> dev_err(pp->dev, "missing ranges property\n");
> @@ -431,41 +437,29 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
> pp->config.cfg1_size = resource_size(&pp->cfg)/2;
> }
> }
> -
> - if (!pp->dbi_base) {
> - pp->dbi_base = devm_ioremap(pp->dev, pp->cfg.start,
> - resource_size(&pp->cfg));
> - if (!pp->dbi_base) {
> - dev_err(pp->dev, "error with ioremap\n");
> - return -ENOMEM;
> - }
> - }
> -
> - pp->cfg0_base = pp->cfg.start;
> - pp->cfg1_base = pp->cfg.start + pp->config.cfg0_size;
> pp->mem_base = pp->mem.start;
>
> - pp->va_cfg0_base = devm_ioremap(pp->dev, pp->cfg0_base,
> - pp->config.cfg0_size);
> - if (!pp->va_cfg0_base) {
> - dev_err(pp->dev, "error with ioremap in function\n");
> - return -ENOMEM;
> - }
> - pp->va_cfg1_base = devm_ioremap(pp->dev, pp->cfg1_base,
> - pp->config.cfg1_size);
> - if (!pp->va_cfg1_base) {
> - dev_err(pp->dev, "error with ioremap\n");
> - return -ENOMEM;
> - }
> + return 0;
> +}
>
> - if (of_property_read_u32(np, "num-lanes", &pp->lanes)) {
> - dev_err(pp->dev, "Failed to parse the number of lanes\n");
> - return -EINVAL;
> - }
> +/*
> + * dw_pcie_msi_host_init() - Function to initialize msi host controller
> + *
> + * @pp: ptr to pcie port
> + * @np: device node ptr to msi irq controller
> + * @irq_msi_ops: ptr to MSI irq_domain_ops struct
> + *
> + * Function register irq domain for msi irq controller and create mappings
> + * for MSI irqs.
> + */
May be you can only do following to support your MSI chip:
Initialize pp->irq_domain into your add_pcie_port function before you
call dw_pcie_host_init.
In dw_pcie_host_init,
if (IS_ENABLED(CONFIG_PCI_MSI)
Re: [PATCH v2 1/8] PCI: designware: add rd[wr]_other_conf API
On Wed, Jun 11, 2014 at 02:51:20AM +0800, Murali Karicheri wrote:
> v3.65 version of the designware h/w, requires application space
> registers to be configured to access the remote EP config space.
> To support this, add rd[wr]_other_conf API in the pcie_host_opts
>
> Signed-off-by: Murali Karicheri
>
> CC: Santosh Shilimkar
> CC: Russell King
> CC: Grant Likely
> CC: Rob Herring
> CC: Mohit Kumar
> CC: Jingoo Han
> CC: Bjorn Helgaas
> CC: Pratyush Anand
> CC: Richard Zhu
> CC: Kishon Vijay Abraham I
> CC: Marek Vasut
> CC: Arnd Bergmann
> CC: Pawel Moll
> CC: Mark Rutland
> CC: Ian Campbell
> CC: Kumar Gala
> CC: Randy Dunlap
> CC: Grant Likely
>
> ---
> drivers/pci/host/pcie-designware.c | 12 ++--
> drivers/pci/host/pcie-designware.h |4
> 2 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/host/pcie-designware.c
> b/drivers/pci/host/pcie-designware.c
> index c4e3732..e8f5d8d 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -654,7 +654,11 @@ static int dw_pcie_rd_conf(struct pci_bus *bus, u32
> devfn, int where,
>
> spin_lock_irqsave(&pp->conf_lock, flags);
> if (bus->number != pp->root_bus_nr)
> - ret = dw_pcie_rd_other_conf(pp, bus, devfn,
> + if (pp->ops->rd_other_conf)
> + ret = pp->ops->rd_other_conf(pp, bus, devfn,
> + where, size, val);
> + else
> + ret = dw_pcie_rd_other_conf(pp, bus, devfn,
> where, size, val);
> else
> ret = dw_pcie_rd_own_conf(pp, where, size, val);
> @@ -680,7 +684,11 @@ static int dw_pcie_wr_conf(struct pci_bus *bus, u32
> devfn,
>
> spin_lock_irqsave(&pp->conf_lock, flags);
> if (bus->number != pp->root_bus_nr)
> - ret = dw_pcie_wr_other_conf(pp, bus, devfn,
> + if (pp->ops->wr_other_conf)
> + ret = pp->ops->wr_other_conf(pp, bus, devfn,
> + where, size, val);
> + else
> + ret = dw_pcie_wr_other_conf(pp, bus, devfn,
> where, size, val);
> else
> ret = dw_pcie_wr_own_conf(pp, where, size, val);
> diff --git a/drivers/pci/host/pcie-designware.h
> b/drivers/pci/host/pcie-designware.h
> index 3063b35..2d6dd66 100644
> --- a/drivers/pci/host/pcie-designware.h
> +++ b/drivers/pci/host/pcie-designware.h
> @@ -62,6 +62,10 @@ struct pcie_host_ops {
> u32 val, void __iomem *dbi_base);
> int (*rd_own_conf)(struct pcie_port *pp, int where, int size, u32 *val);
> int (*wr_own_conf)(struct pcie_port *pp, int where, int size, u32 val);
> + int (*rd_other_conf)(struct pcie_port *pp, struct pci_bus *bus,
> + unsigned int devfn, int where, int size, u32 *val);
> + int (*wr_other_conf)(struct pcie_port *pp, struct pci_bus *bus,
> + unsigned int devfn, int where, int size, u32 val);
> int (*link_up)(struct pcie_port *pp);
> void (*host_init)(struct pcie_port *pp);
> };
Reviewed-by: Pratyush Anand
> --
> 1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V8 0/9] PCI: Add SPEAr13xx PCie support
Hi Arnd/Bjorn, On Mon, May 19, 2014 at 07:55:12PM +0800, Mohit KUMAR wrote: > Hello Arnd, > > > -Original Message- > > From: Mohit KUMAR DCG > > Sent: Tuesday, April 15, 2014 5:20 PM > > To: spear-devel; linux-ker...@vger.kernel.org; devicetree@vger.kernel.org; > > linux-...@vger.kernel.org; linux-arm-ker...@lists.infradead.org > > Cc: Mohit KUMAR DCG > > Subject: [PATCH V8 0/9] PCI: Add SPEAr13xx PCie support > > > > Patch# 1 and 2: Improvement and fixes for SPEAr13xx support. > > Patch# 3,4 and 6: Add DT bindings for spear1310/40-miphy, misc and pcie > > node Patch# 5: Add spear1310/40-miphy driver and support for spear1310/40 > > miphy wrapper. > > Patch# 7-9: Add SPEAr13xx PCIe driver and dt support. > > > > These pathes are tested with linux-3.15 tag of arm-soc tree. > > Tested with SPEAr1310 evaluation board with: > > - INTEL PRO 100/100 EP card > > - USB xhci gen2 card > > - Above cards connected through LeCROY PTC switch > > > > Modifications for SATA are tested with SPEAr1340-evb board > > > > I think 3.16 windows is about to close, are you planning to merge this series? > Pls let me know if something pending on our part. If you could let us know the status of this patch series inclusion. Regards Pratyush > > Regards > Mohit > > > Changes since v7: > > - Rebase over for-linus-3.15 tag of arm-soc tree so that complete patch > > series > > can be applied cleanly. > > - Incorporated few comments on comment style and superfluous comments > > Changes since v6: > > - Split miphy driver for SPEAr1310 and SPEAr1340 > > - Some cleanup and incorporated other minor comments Changes since v5: > > - Split DT bindings for misc, miphy-40lp and pcie node into sepearte patches > > - Merge config options PCIE_SPEAR13XX and PCI_MSI into defconfig patch > > - Incorporated other minor comments > > Changes since v4: > > - Uses per device function pointers passed from .data field to > > the of_device_id instead of of_device_is_compatible. > > - Incorporated other minor comments from v4 > > > > Changes since v3: > > - Phy driver renamed to phy-miphy40lp > > - ahci phy hook patch used as suggested by Arnd > > - Incorporated other minor comments from v3 > > > > Changes since v2: > > - Incorporated comments to move SPEAr13xx PCIe and SATA phy specific > > routines to > > the phy framework > > - Modify ahci driver to include phy hooks > > - phy-core driver modifications for subsys_initcall() > > > > Changes since v1: > > - Few patches of the series are already accepted and applied to mainline > > e.g. > > pcie designware driver improvements,fixes for IO translation bug, PCIe dw > > driver maintainer. So dropped these from v2. > > - Incorporated comment to move the common/reset PCIe code to the > > seperate driver > > - PCIe and SATA share common PHY configuration registers, so move SATA > > platform code to the system config driver Fourth patch is improves pcie > > designware driver and fixes the IO translation bug. IO translation bug fix > > leads to the working of PCIe EP devices connected to RC through switch. > > > > Mohit Kumar (1): > > SPEAr13xx: defconfig: Update > > > > Pratyush Anand (8): > > clk: SPEAr13XX: Fix pcie clock name > > SPEAr13XX: Fix static mapping table > > phy: SPEAr1310/40-miphy: Add binding information > > SPEAr: misc: Add binding information > > phy: SPEAr1310/40-miphy: Add phy driver for PCIe and SATA > > SPEAr13XX: Add binding information for PCIe controller > > SPEAr13XX: dts: Add PCIe node information > > pcie: SPEAr13xx: Add designware wrapper support > > > > .../devicetree/bindings/arm/spear-misc.txt |9 + > > .../devicetree/bindings/pci/spear13xx-pcie.txt | 14 + > > .../devicetree/bindings/phy/st-spear1310-miphy.txt | 12 + > > .../devicetree/bindings/phy/st-spear1340-miphy.txt | 11 + > > MAINTAINERS|6 + > > arch/arm/boot/dts/spear1310-evb.dts|4 + > > arch/arm/boot/dts/spear1310.dtsi | 93 +- > > arch/arm/boot/dts/spear1340-evb.dts|4 + > > arch/arm/boot/dts/spear1340.dtsi | 30 ++- > > arch/arm/boot/dts/spear13xx.dtsi |9 +- > > arch/arm/configs/spear13xx_defconfig | 16 + > > arch/arm/mach-spear/Kconfig|4 + > > arch/arm/mach-spea
Re: [PATCH V4 0/8]PCI:Add SPEAr13xx PCie support
Hi Arnd, Do you see any more improvement in this series. Else we will send V5 (probably the final one) with modifications for Kishon's comment. Regards Pratyush On Thu, Feb 6, 2014 at 10:14 AM, Pratyush Anand wrote: > First three patches are improvement and fixes for SPEAr13xx support. > Patches 4-6 add miphy40lp skelten driver and support for spear1310/40 miphy > wrapper. Patch 7 add support for SPEAr13xx PCIe. > > These pathes are tested with linux-3.14-rc1 with following patch on the top of > it: > Author: Balaji T K > Date: Mon Jan 20 16:41:27 2014 +0200 > > ata: ahci_platform: Manage SATA PHY > > Tested with SPEAr1310 evaluation board: > - INTEL PRO 100/100 EP card > - USB xhci gen2 card > - Above cards connected through LeCROY PTC switch > > Modifications for SATA are tested with SPEAr1340-evb board > > Changes since v3: > - Phy driver renamed to phy-miphy40lp > - ahci phy hook patch used as suggested by Arnd > - Incorporated other minor comments from v3 > > Changes since v2: > - Incorporated comments to move SPEAr13xx PCIe and SATA phy specific routines > to > the phy framework > - Modify ahci driver to include phy hooks > - phy-core driver modifications for subsys_initcall() > > Changes since v1: > - Few patches of the series are already accepted and applied to mainline e.g. > pcie designware driver improvements,fixes for IO translation bug, PCIe dw > driver maintainer. So dropped these from v2. > - Incorporated comment to move the common/reset PCIe code to the seperate > driver > - PCIe and SATA share common PHY configuration registers, so move SATA > platform code to the system config driver > Fourth patch is improves pcie designware driver and fixes the IO > translation bug. IO translation bug fix leads to the working of PCIe EP > devices > connected to RC through switch. > > PCIe driver support for SPEAr1310/40 platform board is added. > > These patches are tested with SPEAr1310 evaluation board: > - INTEL PRO 100/100 EP card > - USB xhci gen2 card > - Above cards connected through LeCROY PTC switch > > Cc: linux-arm-ker...@lists.infradead.org > Cc: devicetree@vger.kernel.org > Cc: linux-...@vger.kernel.org > Cc: linux-...@vger.kernel.org > Cc: spear-de...@list.st.com > Cc: linux-ker...@vger.kernel.org > > Mohit Kumar (2): > SPEAr13xx: defconfig: Update > MAINTAINERS: Add ST SPEAr13xx PCIe driver maintainer > > Pratyush Anand (6): > clk: SPEAr13xx: Fix pcie clock name > SPEAr13xx: Fix static mapping table > phy: st-miphy-40lp: Add skeleton driver > SPEAr13xx: Fixup: Move SPEAr1340 SATA platform code to phy driver > phy: st-miphy-40lp: Add SPEAr1310 and SPEAr1340 PCIe phy support > pcie: SPEAr13xx: Add designware pcie support > > .../devicetree/bindings/arm/spear-misc.txt | 4 + > .../devicetree/bindings/pci/spear13xx-pcie.txt | 7 + > .../devicetree/bindings/phy/st-miphy40lp.txt | 12 + > MAINTAINERS| 6 + > arch/arm/boot/dts/spear1310-evb.dts| 4 + > arch/arm/boot/dts/spear1310.dtsi | 96 +++- > arch/arm/boot/dts/spear1340-evb.dts| 4 + > arch/arm/boot/dts/spear1340.dtsi | 32 +- > arch/arm/boot/dts/spear13xx.dtsi | 10 +- > arch/arm/configs/spear13xx_defconfig | 15 + > arch/arm/mach-spear/Kconfig| 3 + > arch/arm/mach-spear/include/mach/spear.h | 4 +- > arch/arm/mach-spear/spear1340.c| 127 + > arch/arm/mach-spear/spear13xx.c| 2 +- > drivers/clk/spear/spear1310_clock.c| 6 +- > drivers/clk/spear/spear1340_clock.c| 2 +- > drivers/pci/host/Kconfig | 5 + > drivers/pci/host/Makefile | 1 + > drivers/pci/host/pcie-spear13xx.c | 414 > drivers/phy/Kconfig| 6 + > drivers/phy/Makefile | 1 + > drivers/phy/phy-miphy40lp.c| 544 > + > 22 files changed, 1166 insertions(+), 139 deletions(-) > create mode 100644 Documentation/devicetree/bindings/arm/spear-misc.txt > create mode 100644 Documentation/devicetree/bindings/pci/spear13xx-pcie.txt > create mode 100644 Documentation/devicetree/bindings/phy/st-miphy40lp.txt > create mode 100644 drivers/pci/host/pcie-spear13xx.c > create mode 100644 drivers/phy/phy-miphy40lp.c > > -- > 1.8.1.2 > > > ___ > linux-arm-kernel mailing list > linux-arm-ker...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 5/8] SPEAr13xx: Fixup: Move SPEAr1340 SATA platform code to phy driver
On Thu, Feb 06, 2014 at 04:01:25PM +0800, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Thursday 06 February 2014 12:30 PM, Pratyush Anand wrote:
> > On Thu, Feb 06, 2014 at 02:32:42PM +0800, Kishon Vijay Abraham I wrote:
> >> Hi,
> >>
> >> On Thursday 06 February 2014 10:14 AM, Pratyush Anand wrote:
> >>> ahci driver needs some platform specific functions which are called at
> >>> init, exit, suspend and resume conditions. Till now these functions were
> >>> present in a platform driver with a fixme notes.
> >>>
> >>> Similar functions modifying same set of registers will also be needed in
> >>> case of PCIe phy init/exit.
> >>>
> >>> So move all these SATA platform code to phy-miphy40lp driver.
> >>>
> >>> Signed-off-by: Pratyush Anand
> >>> Tested-by: Mohit Kumar
> >>> Cc: Viresh Kumar
> >>> Cc: Tejun Heo
> >>> Cc: Arnd Bergmann
> >>> Cc: Kishon Vijay Abraham I
> >>> Cc: spear-de...@list.st.com
> >>> Cc: linux-arm-ker...@lists.infradead.org
> >>> Cc: devicetree@vger.kernel.org
> >>> Cc: linux-...@vger.kernel.org
> >>> Cc: linux-ker...@vger.kernel.org
> >>> ---
> >>> .../devicetree/bindings/arm/spear-misc.txt | 4 +
> >>> arch/arm/boot/dts/spear1310-evb.dts| 4 +
> >>> arch/arm/boot/dts/spear1310.dtsi | 39 +++-
> >>> arch/arm/boot/dts/spear1340-evb.dts| 4 +
> >>> arch/arm/boot/dts/spear1340.dtsi | 13 +-
> >>> arch/arm/boot/dts/spear13xx.dtsi | 5 +
> >>> arch/arm/mach-spear/Kconfig| 2 +
> >>> arch/arm/mach-spear/spear1340.c| 127 +
> >>> drivers/phy/phy-miphy40lp.c| 204
> >>> -
> >>
> >> It would be better if you can split this patch. Keep arch/ in separate
> >> patch
> >> and drivers/ in separate patch.
> >
> > Code is actually moving from arch to driver. Therefore I kept it in
> > same patch.
> >
> >>> 9 files changed, 266 insertions(+), 136 deletions(-)
> >>> create mode 100644 Documentation/devicetree/bindings/arm/spear-misc.txt
> >>>
> >> .
> >> .
> >>
> >> .
> >> .
> >>> static const char * const spear1340_dt_board_compat[] = {
> >>> diff --git a/drivers/phy/phy-miphy40lp.c b/drivers/phy/phy-miphy40lp.c
> >>> index d478c14..cc7f45d 100644
> >>> --- a/drivers/phy/phy-miphy40lp.c
> >>> +++ b/drivers/phy/phy-miphy40lp.c
> >>> @@ -8,6 +8,7 @@
> >>> * it under the terms of the GNU General Public License version 2 as
> >>> * published by the Free Software Foundation.
> >>> *
> >>> + * 04/02/2014: Adding support of SATA mode for SPEAr1340.
> >>> */
> >>>
> >>> #include
> >>> @@ -19,6 +20,60 @@
> >>> #include
> >>> #include
> >>>
> >>> +/* SPEAr1340 Registers */
> >>> +/* Power Management Registers */
> >>> +#define SPEAR1340_PCM_CFG0x100
> >>> + #define SPEAR1340_PCM_CFG_SATA_POWER_EN 0x800
> >>> +#define SPEAR1340_PCM_WKUP_CFG 0x104
> >>> +#define SPEAR1340_SWITCH_CTR 0x108
> >>> +
> >>> +#define SPEAR1340_PERIP1_SW_RST 0x318
> >>> + #define SPEAR1340_PERIP1_SW_RST_SATA0x1000
> >>> +#define SPEAR1340_PERIP2_SW_RST 0x31C
> >>> +#define SPEAR1340_PERIP3_SW_RST 0x320
> >>> +
> >>> +/* PCIE - SATA configuration registers */
> >>> +#define SPEAR1340_PCIE_SATA_CFG 0x424
> >>> + /* PCIE CFG MASks */
> >>> + #define SPEAR1340_PCIE_CFG_DEVICE_PRESENT (1 << 11)
> >>
> >> use BIT() wherever possible.
> >
> > OK.
> >
> >>> + #define SPEAR1340_PCIE_CFG_POWERUP_RESET(1 << 10)
> >>> + #define SPEAR1340_PCIE_CFG_CORE_CLK_EN (1 << 9)
> >>> + #define SPEAR1340_PCIE_CFG_AUX_CLK_EN (1 << 8)
> >>> + #define SPEAR1340_SATA_CFG_TX_CLK_EN(1 << 4)
> >>> + #define SPEAR1340_SATA_CFG_RX_CLK_EN(1 << 3)
> >>> + #define SPEAR13
Re: [PATCH V4 5/8] SPEAr13xx: Fixup: Move SPEAr1340 SATA platform code to phy driver
On Thu, Feb 06, 2014 at 02:32:42PM +0800, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Thursday 06 February 2014 10:14 AM, Pratyush Anand wrote:
> > ahci driver needs some platform specific functions which are called at
> > init, exit, suspend and resume conditions. Till now these functions were
> > present in a platform driver with a fixme notes.
> >
> > Similar functions modifying same set of registers will also be needed in
> > case of PCIe phy init/exit.
> >
> > So move all these SATA platform code to phy-miphy40lp driver.
> >
> > Signed-off-by: Pratyush Anand
> > Tested-by: Mohit Kumar
> > Cc: Viresh Kumar
> > Cc: Tejun Heo
> > Cc: Arnd Bergmann
> > Cc: Kishon Vijay Abraham I
> > Cc: spear-de...@list.st.com
> > Cc: linux-arm-ker...@lists.infradead.org
> > Cc: devicetree@vger.kernel.org
> > Cc: linux-...@vger.kernel.org
> > Cc: linux-ker...@vger.kernel.org
> > ---
> > .../devicetree/bindings/arm/spear-misc.txt | 4 +
> > arch/arm/boot/dts/spear1310-evb.dts| 4 +
> > arch/arm/boot/dts/spear1310.dtsi | 39 +++-
> > arch/arm/boot/dts/spear1340-evb.dts| 4 +
> > arch/arm/boot/dts/spear1340.dtsi | 13 +-
> > arch/arm/boot/dts/spear13xx.dtsi | 5 +
> > arch/arm/mach-spear/Kconfig| 2 +
> > arch/arm/mach-spear/spear1340.c| 127 +
> > drivers/phy/phy-miphy40lp.c| 204
> > -
>
> It would be better if you can split this patch. Keep arch/ in separate patch
> and drivers/ in separate patch.
Code is actually moving from arch to driver. Therefore I kept it in
same patch.
> > 9 files changed, 266 insertions(+), 136 deletions(-)
> > create mode 100644 Documentation/devicetree/bindings/arm/spear-misc.txt
> >
> .
> .
>
> .
> .
> > static const char * const spear1340_dt_board_compat[] = {
> > diff --git a/drivers/phy/phy-miphy40lp.c b/drivers/phy/phy-miphy40lp.c
> > index d478c14..cc7f45d 100644
> > --- a/drivers/phy/phy-miphy40lp.c
> > +++ b/drivers/phy/phy-miphy40lp.c
> > @@ -8,6 +8,7 @@
> > * it under the terms of the GNU General Public License version 2 as
> > * published by the Free Software Foundation.
> > *
> > + * 04/02/2014: Adding support of SATA mode for SPEAr1340.
> > */
> >
> > #include
> > @@ -19,6 +20,60 @@
> > #include
> > #include
> >
> > +/* SPEAr1340 Registers */
> > +/* Power Management Registers */
> > +#define SPEAR1340_PCM_CFG 0x100
> > + #define SPEAR1340_PCM_CFG_SATA_POWER_EN 0x800
> > +#define SPEAR1340_PCM_WKUP_CFG 0x104
> > +#define SPEAR1340_SWITCH_CTR 0x108
> > +
> > +#define SPEAR1340_PERIP1_SW_RST0x318
> > + #define SPEAR1340_PERIP1_SW_RST_SATA0x1000
> > +#define SPEAR1340_PERIP2_SW_RST0x31C
> > +#define SPEAR1340_PERIP3_SW_RST0x320
> > +
> > +/* PCIE - SATA configuration registers */
> > +#define SPEAR1340_PCIE_SATA_CFG0x424
> > + /* PCIE CFG MASks */
> > + #define SPEAR1340_PCIE_CFG_DEVICE_PRESENT (1 << 11)
>
> use BIT() wherever possible.
OK.
> > + #define SPEAR1340_PCIE_CFG_POWERUP_RESET(1 << 10)
> > + #define SPEAR1340_PCIE_CFG_CORE_CLK_EN (1 << 9)
> > + #define SPEAR1340_PCIE_CFG_AUX_CLK_EN (1 << 8)
> > + #define SPEAR1340_SATA_CFG_TX_CLK_EN(1 << 4)
> > + #define SPEAR1340_SATA_CFG_RX_CLK_EN(1 << 3)
> > + #define SPEAR1340_SATA_CFG_POWERUP_RESET(1 << 2)
> > + #define SPEAR1340_SATA_CFG_PM_CLK_EN(1 << 1)
> > + #define SPEAR1340_PCIE_SATA_SEL_PCIE(0)
> > + #define SPEAR1340_PCIE_SATA_SEL_SATA(1)
> > + #define SPEAR1340_PCIE_SATA_CFG_MASK0xF1F
> > + #define SPEAR1340_PCIE_CFG_VAL (SPEAR1340_PCIE_SATA_SEL_PCIE | \
> > + SPEAR1340_PCIE_CFG_AUX_CLK_EN | \
> > + SPEAR1340_PCIE_CFG_CORE_CLK_EN | \
> > + SPEAR1340_PCIE_CFG_POWERUP_RESET | \
> > + SPEAR1340_PCIE_CFG_DEVICE_PRESENT)
> > + #define SPEAR1340_SATA_CFG_VAL (SPEAR1340_PCIE_SATA_SEL_SATA | \
> > + SPEAR1340_SATA_CFG_PM_CLK_EN | \
> > + SPEAR1340_SATA_CFG_POWERUP_RESET | \
> > +
Re: [PATCH V4 4/8] phy: st-miphy-40lp: Add skeleton driver
On Thu, Feb 06, 2014 at 02:23:20PM +0800, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Thursday 06 February 2014 11:44 AM, Pratyush Anand wrote:
> > Hi Kishon,
> >
> > On Thu, Feb 06, 2014 at 02:01:45PM +0800, Kishon Vijay Abraham I wrote:
> >> Hi,
> >>
> >> On Thursday 06 February 2014 10:14 AM, Pratyush Anand wrote:
> >>> ST miphy-40lp supports PCIe, SATA and Super Speed USB. This driver adds
[...]
> >>> +
> >>> + phy_provider = devm_of_phy_provider_register(dev, st_miphy40lp_xlate);
> >>> + if (IS_ERR(phy_provider)) {
> >>> + dev_err(dev, "failed to register phy provider\n");
> >>> + return PTR_ERR(phy_provider);
> >>> + }
> >>
> >> phy_provider_register should be the last step in registering the PHY. Or
> >> your
> >> PHY call backs can be called before you create the PHY. Btw in your case
> >> you
> >
> > But every one else like phy-exynos-mipi-video or phy-omap-usb2 or any
> > other did it same way. First phy_provider_register and then
> > phy_create.
>
> That's a bug which we figured out very late. Will get it fixed in this -rc
> cycle.
Ok..I ll correct in mine too. :)
Rgds
Pratyush
>
> Thanks
> Kishon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 4/8] phy: st-miphy-40lp: Add skeleton driver
Hi Kishon,
On Thu, Feb 06, 2014 at 02:01:45PM +0800, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Thursday 06 February 2014 10:14 AM, Pratyush Anand wrote:
> > ST miphy-40lp supports PCIe, SATA and Super Speed USB. This driver adds
> > skeleton support for the same.
> >
> > Currently phy ops are returning -EINVAL. They can be elaborated
> > depending on the SOC being supported in future.
> >
> > Signed-off-by: Pratyush Anand
> > Tested-by: Mohit Kumar
> > Cc: Arnd Bergmann
> > Cc: Kishon Vijay Abraham I
> > Cc: spear-de...@list.st.com
> > Cc: linux-arm-ker...@lists.infradead.org
> > Cc: devicetree@vger.kernel.org
> > Cc: linux-ker...@vger.kernel.org
> > ---
> > .../devicetree/bindings/phy/st-miphy40lp.txt | 12 ++
> > drivers/phy/Kconfig| 6 +
> > drivers/phy/Makefile | 1 +
> > drivers/phy/phy-miphy40lp.c| 174
> > +
> > 4 files changed, 193 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/phy/st-miphy40lp.txt
> > create mode 100644 drivers/phy/phy-miphy40lp.c
> >
> > diff --git a/Documentation/devicetree/bindings/phy/st-miphy40lp.txt
> > b/Documentation/devicetree/bindings/phy/st-miphy40lp.txt
> > new file mode 100644
> > index 000..d0c7096
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/st-miphy40lp.txt
> > @@ -0,0 +1,12 @@
> > +Required properties:
> > +- compatible : should be "st,miphy40lp-phy"
> > + Other supported soc specific compatible:
> > + "st,spear1310-miphy"
> > + "st,spear1340-miphy"
> > +- reg : offset and length of the PHY register set.
> > +- misc: phandle for the syscon node to access misc registers
> > +- phy-id: Instance id of the phy.
> > +- #phy-cells : from the generic PHY bindings, must be 1.
> > + - 1st cell: phandle to the phy node.
> > + - 2nd cell: 0 if phy (in 1st cell) is to be used for SATA, 1 for PCIe
> > + and 2 for Super Speed USB.
> > diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> > index afa2354..2f58993 100644
> > --- a/drivers/phy/Kconfig
> > +++ b/drivers/phy/Kconfig
> > @@ -64,4 +64,10 @@ config BCM_KONA_USB2_PHY
> > help
> > Enable this to support the Broadcom Kona USB 2.0 PHY.
> >
> > +config PHY_ST_MIPHY40LP
> > + tristate "ST MIPHY 40LP driver"
> > + help
> > + Support for ST MIPHY 40LP which can be used for PCIe, SATA and Super
> > Speed USB.
> > + select GENERIC_PHY
> > +
> > endmenu
> > diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> > index b57c253..c061091 100644
> > --- a/drivers/phy/Makefile
> > +++ b/drivers/phy/Makefile
> > @@ -9,3 +9,4 @@ obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO) +=
> > phy-exynos-mipi-video.o
> > obj-$(CONFIG_PHY_MVEBU_SATA) += phy-mvebu-sata.o
> > obj-$(CONFIG_OMAP_USB2)+= phy-omap-usb2.o
> > obj-$(CONFIG_TWL4030_USB) += phy-twl4030-usb.o
> > +obj-$(CONFIG_PHY_ST_MIPHY40LP) += phy-miphy40lp.o
> > diff --git a/drivers/phy/phy-miphy40lp.c b/drivers/phy/phy-miphy40lp.c
> > new file mode 100644
> > index 000..d478c14
> > --- /dev/null
> > +++ b/drivers/phy/phy-miphy40lp.c
> > @@ -0,0 +1,174 @@
> > +/*
> > + * ST MiPHY-40LP PHY driver
> > + *
> > + * Copyright (C) 2014 ST Microelectronics
> > + * Pratyush Anand
> > + *
> > + * 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
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +enum phy_mode {
> > + SATA,
> > + PCIE,
> > + SS_USB,
> > +};
> > +
> > +struct st_miphy40lp_priv {
> > + /* regmap for any soc specific misc registers */
> > + struct regmap *misc;
> > + /* phy struct pointer */
> > + struct phy *phy;
> > + /* device node pointer */
> > + struct device_node *np;
> > + /* phy mode: 0 for SATA 1 for PCIe and 2 for SS-USB */
> > + enum phy_mode mode;
> > + /* instance id of this phy */
> > + u32 id;
> > +};
> > +
&
[PATCH V4 4/8] phy: st-miphy-40lp: Add skeleton driver
ST miphy-40lp supports PCIe, SATA and Super Speed USB. This driver adds
skeleton support for the same.
Currently phy ops are returning -EINVAL. They can be elaborated
depending on the SOC being supported in future.
Signed-off-by: Pratyush Anand
Tested-by: Mohit Kumar
Cc: Arnd Bergmann
Cc: Kishon Vijay Abraham I
Cc: spear-de...@list.st.com
Cc: linux-arm-ker...@lists.infradead.org
Cc: devicetree@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
---
.../devicetree/bindings/phy/st-miphy40lp.txt | 12 ++
drivers/phy/Kconfig| 6 +
drivers/phy/Makefile | 1 +
drivers/phy/phy-miphy40lp.c| 174 +
4 files changed, 193 insertions(+)
create mode 100644 Documentation/devicetree/bindings/phy/st-miphy40lp.txt
create mode 100644 drivers/phy/phy-miphy40lp.c
diff --git a/Documentation/devicetree/bindings/phy/st-miphy40lp.txt
b/Documentation/devicetree/bindings/phy/st-miphy40lp.txt
new file mode 100644
index 000..d0c7096
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/st-miphy40lp.txt
@@ -0,0 +1,12 @@
+Required properties:
+- compatible : should be "st,miphy40lp-phy"
+ Other supported soc specific compatible:
+ "st,spear1310-miphy"
+ "st,spear1340-miphy"
+- reg : offset and length of the PHY register set.
+- misc: phandle for the syscon node to access misc registers
+- phy-id: Instance id of the phy.
+- #phy-cells : from the generic PHY bindings, must be 1.
+ - 1st cell: phandle to the phy node.
+ - 2nd cell: 0 if phy (in 1st cell) is to be used for SATA, 1 for PCIe
+ and 2 for Super Speed USB.
diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index afa2354..2f58993 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -64,4 +64,10 @@ config BCM_KONA_USB2_PHY
help
Enable this to support the Broadcom Kona USB 2.0 PHY.
+config PHY_ST_MIPHY40LP
+ tristate "ST MIPHY 40LP driver"
+ help
+ Support for ST MIPHY 40LP which can be used for PCIe, SATA and Super
Speed USB.
+ select GENERIC_PHY
+
endmenu
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index b57c253..c061091 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -9,3 +9,4 @@ obj-$(CONFIG_PHY_EXYNOS_MIPI_VIDEO) +=
phy-exynos-mipi-video.o
obj-$(CONFIG_PHY_MVEBU_SATA) += phy-mvebu-sata.o
obj-$(CONFIG_OMAP_USB2)+= phy-omap-usb2.o
obj-$(CONFIG_TWL4030_USB) += phy-twl4030-usb.o
+obj-$(CONFIG_PHY_ST_MIPHY40LP) += phy-miphy40lp.o
diff --git a/drivers/phy/phy-miphy40lp.c b/drivers/phy/phy-miphy40lp.c
new file mode 100644
index 000..d478c14
--- /dev/null
+++ b/drivers/phy/phy-miphy40lp.c
@@ -0,0 +1,174 @@
+/*
+ * ST MiPHY-40LP PHY driver
+ *
+ * Copyright (C) 2014 ST Microelectronics
+ * Pratyush Anand
+ *
+ * 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
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+enum phy_mode {
+ SATA,
+ PCIE,
+ SS_USB,
+};
+
+struct st_miphy40lp_priv {
+ /* regmap for any soc specific misc registers */
+ struct regmap *misc;
+ /* phy struct pointer */
+ struct phy *phy;
+ /* device node pointer */
+ struct device_node *np;
+ /* phy mode: 0 for SATA 1 for PCIe and 2 for SS-USB */
+ enum phy_mode mode;
+ /* instance id of this phy */
+ u32 id;
+};
+
+static int miphy40lp_init(struct phy *phy)
+{
+ return -EINVAL;
+}
+
+static int miphy40lp_exit(struct phy *phy)
+{
+ return -EINVAL;
+}
+
+static int miphy40lp_power_off(struct phy *phy)
+{
+ return -EINVAL;
+}
+
+static int miphy40lp_power_on(struct phy *phy)
+{
+ return -EINVAL;
+}
+
+static const struct of_device_id st_miphy40lp_of_match[] = {
+ { .compatible = "st,miphy40lp-phy" },
+ { },
+};
+MODULE_DEVICE_TABLE(of, st_miphy40lp_of_match);
+
+static struct phy_ops st_miphy40lp_ops = {
+ .init = miphy40lp_init,
+ .exit = miphy40lp_exit,
+ .power_off = miphy40lp_power_off,
+ .power_on = miphy40lp_power_on,
+ .owner = THIS_MODULE,
+};
+
+#ifdef CONFIG_PM_SLEEP
+static int miphy40lp_suspend(struct device *dev)
+{
+ return -EINVAL;
+}
+
+static int miphy40lp_resume(struct device *dev)
+{
+ return -EINVAL;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(miphy40lp_pm_ops, miphy40lp_suspend,
+ miphy40lp_resume);
+
+static struct phy *st_miphy40lp_xlate(struct device *dev,
+ struct of_phandle_args *args)
+{
+ struct st_miphy40lp_priv *phypriv = dev_ge
[PATCH V4 5/8] SPEAr13xx: Fixup: Move SPEAr1340 SATA platform code to phy driver
ahci driver needs some platform specific functions which are called at
init, exit, suspend and resume conditions. Till now these functions were
present in a platform driver with a fixme notes.
Similar functions modifying same set of registers will also be needed in
case of PCIe phy init/exit.
So move all these SATA platform code to phy-miphy40lp driver.
Signed-off-by: Pratyush Anand
Tested-by: Mohit Kumar
Cc: Viresh Kumar
Cc: Tejun Heo
Cc: Arnd Bergmann
Cc: Kishon Vijay Abraham I
Cc: spear-de...@list.st.com
Cc: linux-arm-ker...@lists.infradead.org
Cc: devicetree@vger.kernel.org
Cc: linux-...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
---
.../devicetree/bindings/arm/spear-misc.txt | 4 +
arch/arm/boot/dts/spear1310-evb.dts| 4 +
arch/arm/boot/dts/spear1310.dtsi | 39 +++-
arch/arm/boot/dts/spear1340-evb.dts| 4 +
arch/arm/boot/dts/spear1340.dtsi | 13 +-
arch/arm/boot/dts/spear13xx.dtsi | 5 +
arch/arm/mach-spear/Kconfig| 2 +
arch/arm/mach-spear/spear1340.c| 127 +
drivers/phy/phy-miphy40lp.c| 204 -
9 files changed, 266 insertions(+), 136 deletions(-)
create mode 100644 Documentation/devicetree/bindings/arm/spear-misc.txt
diff --git a/Documentation/devicetree/bindings/arm/spear-misc.txt
b/Documentation/devicetree/bindings/arm/spear-misc.txt
new file mode 100644
index 000..aacd36a
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/spear-misc.txt
@@ -0,0 +1,4 @@
+* SPEAr Misc configuration
+** misc node required properties:
+- compatible Should be "st,spear1340-misc", "syscon".
+- reg: Address range of misc space
diff --git a/arch/arm/boot/dts/spear1310-evb.dts
b/arch/arm/boot/dts/spear1310-evb.dts
index b56a801..d42c84b 100644
--- a/arch/arm/boot/dts/spear1310-evb.dts
+++ b/arch/arm/boot/dts/spear1310-evb.dts
@@ -106,6 +106,10 @@
status = "okay";
};
+ miphy@eb80 {
+ status = "okay";
+ };
+
cf@b280 {
status = "okay";
};
diff --git a/arch/arm/boot/dts/spear1310.dtsi b/arch/arm/boot/dts/spear1310.dtsi
index 122ae94..64e7dd5 100644
--- a/arch/arm/boot/dts/spear1310.dtsi
+++ b/arch/arm/boot/dts/spear1310.dtsi
@@ -29,24 +29,57 @@
#gpio-cells = <2>;
};
- ahci@b100 {
+ miphy0: miphy@eb80 {
+ compatible = "st,miphy", "st,spear1310-miphy";
+ reg = <0xeb80 0x4000>;
+ misc = <&misc>;
+ phy-id = <0>;
+ #phy-cells = <1>;
+ status = "disabled";
+ };
+
+ miphy1: miphy@eb804000 {
+ compatible = "st,miphy", "st,spear1310-miphy";
+ reg = <0xeb804000 0x4000>;
+ misc = <&misc>;
+ phy-id = <1>;
+ #phy-cells = <1>;
+ status = "disabled";
+ };
+
+ miphy2: miphy@eb808000 {
+ compatible = "st,miphy", "st,spear1310-miphy";
+ reg = <0xeb808000 0x4000>;
+ misc = <&misc>;
+ phy-id = <2>;
+ #phy-cells = <1>;
+ status = "disabled";
+ };
+
+ ahci0: ahci@b100 {
compatible = "snps,spear-ahci";
reg = <0xb100 0x1>;
interrupts = <0 68 0x4>;
+ phys = <&miphy0 0>;
+ phy-names = "sata-phy";
status = "disabled";
};
- ahci@b180 {
+ ahci1: ahci@b180 {
compatible = "snps,spear-ahci";
reg = <0xb180 0x1>;
interrupts = <0 69 0x4>;
+ phys = <&miphy1 0>;
+ phy-names = "sata-phy";
status = "disabled";
};
- ahci@b400 {
+ ahci2: ahci@b400 {
compatible = "snps,spear-ahci";
reg = <0xb400 0x1>;
interrupts = <0 70 0x4>;
+ phys = <&miphy2 0>;
+
[PATCH V4 0/8]PCI:Add SPEAr13xx PCie support
First three patches are improvement and fixes for SPEAr13xx support. Patches 4-6 add miphy40lp skelten driver and support for spear1310/40 miphy wrapper. Patch 7 add support for SPEAr13xx PCIe. These pathes are tested with linux-3.14-rc1 with following patch on the top of it: Author: Balaji T K Date: Mon Jan 20 16:41:27 2014 +0200 ata: ahci_platform: Manage SATA PHY Tested with SPEAr1310 evaluation board: - INTEL PRO 100/100 EP card - USB xhci gen2 card - Above cards connected through LeCROY PTC switch Modifications for SATA are tested with SPEAr1340-evb board Changes since v3: - Phy driver renamed to phy-miphy40lp - ahci phy hook patch used as suggested by Arnd - Incorporated other minor comments from v3 Changes since v2: - Incorporated comments to move SPEAr13xx PCIe and SATA phy specific routines to the phy framework - Modify ahci driver to include phy hooks - phy-core driver modifications for subsys_initcall() Changes since v1: - Few patches of the series are already accepted and applied to mainline e.g. pcie designware driver improvements,fixes for IO translation bug, PCIe dw driver maintainer. So dropped these from v2. - Incorporated comment to move the common/reset PCIe code to the seperate driver - PCIe and SATA share common PHY configuration registers, so move SATA platform code to the system config driver Fourth patch is improves pcie designware driver and fixes the IO translation bug. IO translation bug fix leads to the working of PCIe EP devices connected to RC through switch. PCIe driver support for SPEAr1310/40 platform board is added. These patches are tested with SPEAr1310 evaluation board: - INTEL PRO 100/100 EP card - USB xhci gen2 card - Above cards connected through LeCROY PTC switch Cc: linux-arm-ker...@lists.infradead.org Cc: devicetree@vger.kernel.org Cc: linux-...@vger.kernel.org Cc: linux-...@vger.kernel.org Cc: spear-de...@list.st.com Cc: linux-ker...@vger.kernel.org Mohit Kumar (2): SPEAr13xx: defconfig: Update MAINTAINERS: Add ST SPEAr13xx PCIe driver maintainer Pratyush Anand (6): clk: SPEAr13xx: Fix pcie clock name SPEAr13xx: Fix static mapping table phy: st-miphy-40lp: Add skeleton driver SPEAr13xx: Fixup: Move SPEAr1340 SATA platform code to phy driver phy: st-miphy-40lp: Add SPEAr1310 and SPEAr1340 PCIe phy support pcie: SPEAr13xx: Add designware pcie support .../devicetree/bindings/arm/spear-misc.txt | 4 + .../devicetree/bindings/pci/spear13xx-pcie.txt | 7 + .../devicetree/bindings/phy/st-miphy40lp.txt | 12 + MAINTAINERS| 6 + arch/arm/boot/dts/spear1310-evb.dts| 4 + arch/arm/boot/dts/spear1310.dtsi | 96 +++- arch/arm/boot/dts/spear1340-evb.dts| 4 + arch/arm/boot/dts/spear1340.dtsi | 32 +- arch/arm/boot/dts/spear13xx.dtsi | 10 +- arch/arm/configs/spear13xx_defconfig | 15 + arch/arm/mach-spear/Kconfig| 3 + arch/arm/mach-spear/include/mach/spear.h | 4 +- arch/arm/mach-spear/spear1340.c| 127 + arch/arm/mach-spear/spear13xx.c| 2 +- drivers/clk/spear/spear1310_clock.c| 6 +- drivers/clk/spear/spear1340_clock.c| 2 +- drivers/pci/host/Kconfig | 5 + drivers/pci/host/Makefile | 1 + drivers/pci/host/pcie-spear13xx.c | 414 drivers/phy/Kconfig| 6 + drivers/phy/Makefile | 1 + drivers/phy/phy-miphy40lp.c| 544 + 22 files changed, 1166 insertions(+), 139 deletions(-) create mode 100644 Documentation/devicetree/bindings/arm/spear-misc.txt create mode 100644 Documentation/devicetree/bindings/pci/spear13xx-pcie.txt create mode 100644 Documentation/devicetree/bindings/phy/st-miphy40lp.txt create mode 100644 drivers/pci/host/pcie-spear13xx.c create mode 100644 drivers/phy/phy-miphy40lp.c -- 1.8.1.2 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 6/8] SPEAr13xx: Fixup: Move SPEAr1340 SATA platform code to phy driver
On Thu, Jan 30, 2014 at 09:21:00PM +0800, Arnd Bergmann wrote:
> On Thursday 30 January 2014, Mohit Kumar wrote:
> >
> > diff --git a/Documentation/devicetree/bindings/phy/spear13xx-miphy.txt
> > b/Documentation/devicetree/bindings/phy/spear13xx-miphy.txt
> > new file mode 100644
> > index 000..208b37d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/spear13xx-miphy.txt
> > @@ -0,0 +1,8 @@
> > +Required properties:
> > +- compatible : should be "st,spear1340-sata-pcie-phy".
>
> Just for confirmation: This phy is by design only capable of driving
> sata or pcie, but nothing else if reused in a different SoC, right?
>
> If the phy is actually more generic than that, I'd suggest changing
> the name, otherwise it's ok.
OK, we will give a generic name as it can be used for sata/pcie/usb3.0.
Although, phy register definition may vary from version to version,
but that can be managed,as and when support of new soc is added.
>
> > +- reg : offset and length of the PHY register set.
> > +- misc: phandle for the syscon node to access misc registers
> > +- #phy-cells : from the generic PHY bindings, must be 2.
> > + - 1st arg: phandle to the phy node.
> > + - 2nd arg: 0 if phy (in 1st arg) is to be used for sata else 1.
> > + - 3rd arg: Instance id of the phy (in 1st arg).
>
> I would count "arg" differently: There are three cells, and the first
> cell is the phandle, while the second and third cells contain the first
> and second argument.
Ok..will modify accordingly.
>
> The third cell seems redundant, more on that below.
>
> > + ahci0: ahci@b100 {
> > compatible = "snps,spear-ahci";
> > reg = <0xb100 0x1>;
> > interrupts = <0 68 0x4>;
> > + phys = <&miphy0 0 0>;
> > + phy-names = "ahci-phy";
> > status = "disabled";
> > };
> >
> > - ahci@b180 {
> > + ahci1: ahci@b180 {
> > compatible = "snps,spear-ahci";
> > reg = <0xb180 0x1>;
> > interrupts = <0 69 0x4>;
> > + phys = <&miphy1 0 1>;
> > + phy-names = "ahci-phy";
> > status = "disabled";
> > };
> >
> > - ahci@b400 {
> > + ahci2: ahci@b400 {
> > compatible = "snps,spear-ahci";
> > reg = <0xb400 0x1>;
> > interrupts = <0 70 0x4>;
> > + phys = <&miphy2 0 2>;
> > + phy-names = "ahci-phy";
> > status = "disabled";
> > };
>
> In each case, the number of the phy 'miphyX' is identical to the
> third cell, and I suspect this is by design. In the driver, the
> 'id' field is set in the xlate function, but I could not find any
> place where it actually gets used, so unless you know that it's
> needed, I'd suggest simply removing it.
It has not been used in this patch, as SATA support is currently only
for SPEAr1340, where we have only one instance.
Will be using it in PCIe for SPEAr1310 where 3 instances are present.
>
> Even if you need it, it may be better to have the instance encoded
> in the phy node itself, since it's a property of the phy hardware
> (e.g. if you have to pass the number into a generic register that
> is global to all phys.
Ok..ll do that.
>
> Alternatively, you could have a different representation, where you
> have a single DT device node representing all three PHYs, with
> "reg = <0xeb80 0xc000>;" In that case, all sata devices would
> point to the same phy node and pass the instance id so the phy
> driver can operated the correct register set.
Instance ID is mainly needed to manipulate wrapper register present
within SPEAr13xx misc space. We have a single register in misc space
having bit fields controlling all 3 phys, and there we need this id.
>
> > +static int spear1340_sata_miphy_init(struct spear13xx_phy_priv *phypriv)
> > +{
> > + regmap_update_bits(phypriv->misc, SPEAR1340_PCIE_SATA_CFG,
> > + SPEAR1340_PCIE_SATA_CFG_MASK, SPEAR1340_SATA_CFG_VAL);
> > + regmap_update_bits(phypriv->misc, SPEAR1340_PCIE_MIPHY_CFG,
> > + SPEAR1340_PCIE_MIPHY_CFG_MASK,
> > + SPEAR1340_PCIE_SATA_MIPHY_CFG_SATA_25M_CRYSTAL_CLK);
> > + /* Switch on sata power domain */
> > + regmap_update_bits(phypriv->misc, SPEAR1340_PCM_CFG,
> > + SPEAR1340_PCM_CFG_SATA_POWER_EN,
> > + SPEAR1340_PCM_CFG_SATA_POWER_EN);
> > + msleep(20);
> > + /* Disable PCIE SATA Controller reset */
> > + regmap_update_bits(phypriv->misc, SPEAR1340_PERIP1_SW_RST,
> > + SPEAR1340_PERIP1_SW_RST_SATA, 0);
> > + msleep(20);
> > +
> > + return 0;
> > +}
>
> I guess some of the parts above can eventually get moved into other
> drivers (reset controller, p
Re: [PATCH V2 4/8] SPEAr13xx: Fixup: Move SPEAr1340 SATA platform code to system cfg driver
Hi Arnd, On Sat, Jan 25, 2014 at 2:23 AM, Arnd Bergmann wrote: > > On Friday 24 January 2014, Pratyush Anand wrote: > > On Thu, Jan 23, 2014 at 08:22:54PM +0800, Arnd Bergmann wrote: > > > On Thursday 23 January 2014, Mohit Kumar wrote: > > > > > > I assume you'd want a phandle pointing to the syscon device in here > > > as well? > > > > Since there is only one syscon device in the whole DT, so do I really > > need to add phandle? Currently I am using > > syscon_regmap_lookup_by_compatible to find syscon device. > > I'd much rather use syscon_regmap_lookup_by_phandle than > syscon_regmap_lookup_by_compatible, all the time, since this makes > the relationship between the devices explicit. > > The phandle method also allows you to pass regmap indexes in the > same property, which can be handy if two variants of the chip have > the same registers at a different offset. > > > > > +/* SPEAr1340 Registers */ > > > > +/* Power Management Registers */ [...] > > > Looking at the actual code now, this very much looks like it ought to > > > be a "phy" driver and get put in drivers/phy/. > > > > Actually these registers are part of common system configurations > > register space (called as misc space) for SPEAr SOC. So we opted for > > syscon framework. > > The use of syscon for this is good, I have no objection to that, and > was suggesting that you create a logical "phy" device that uses the > misc syscon device as a backend. > > > PHY registers space starts from 0xEB80, which can be > > programmed for various phy specific functions like power management, > > tx/rx settings, comparator settings etc. In most of the cases phy > > works with default settings, however there are few exceptions for > > which we will be adding a phy driver for further improvement of SPEAr > > drivers. > > I see. So while the code you have here could be expressed as a phy driver > by itself, there is another part of the SoC that controls the actual > phy. How about if you add the phy device node to DT, and write a driver > that doesn't actually program the phy registers for now, but does contain > the code that you have posted here. That would give you flexibility for > future extensions and at the same time let you remove all SPEAr specific > code from the actual AHCI driver by using the generic ahci-platform > driver. OK.. -- will move all these code to a phy driver. -- so, no need of a new cfg node as of now. -- will pass syscon phandle to phy driver. -- currently will keep ahci platform plugin (as it is here) in phy driver. will remove when generic ahci driver is in place. Regards Pratyush > > Arnd > > ___ > linux-arm-kernel mailing list > linux-arm-ker...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 4/8] SPEAr13xx: Fixup: Move SPEAr1340 SATA platform code to system cfg driver
Hi Arnd,
Thanks for your valuable comments.
On Thu, Jan 23, 2014 at 08:22:54PM +0800, Arnd Bergmann wrote:
> On Thursday 23 January 2014, Mohit Kumar wrote:
> > diff --git a/arch/arm/boot/dts/spear13xx.dtsi
> > b/arch/arm/boot/dts/spear13xx.dtsi
> > index 3518803..2b4e58e 100644
> > --- a/arch/arm/boot/dts/spear13xx.dtsi
> > +++ b/arch/arm/boot/dts/spear13xx.dtsi
> > @@ -78,6 +78,10 @@
> > status = "disabled";
> > };
> >
> > + cfg {
> > + compatible = "st,spear13xx-cfg";
> > + };
> > +
> > ahb {
> > #address-cells = <1>;
> > #size-cells = <1>;
>
> I only saw some of the patches, and did not get a patch with the binding
> description for this device. Please forward that patch to me, or add it
> to the series if you didn't have one.
It was not there.
Will add a patch for the same in v3.
>
> I assume you'd want a phandle pointing to the syscon device in here
> as well?
Since there is only one syscon device in the whole DT, so do I really
need to add phandle? Currently I am using
syscon_regmap_lookup_by_compatible to find syscon device.
>
> Regarding the naming, please do not use 'xx' wildcards in DT compatible
> strings. Instead, use the exact model name of the first supported
> version of the hardware (e.g. spear1300 or spear600) that remains
> compatible. If there are minor variations between the versions,
> use a list with the most specific version as well as the older ones
> it's compatible with.
Ok..ll take care.
>
> > @@ -221,6 +225,11 @@
> > 0xd800 0xd800 0x0100
> > 0xe000 0xe000 0x1000>;
> >
> > + misc: misc@e070 {
> > + compatible = "st,spear13xx-misc", "syscon";
> > + reg = <0xe070 0x1000>;
> > + };
> > +
>
> Same here. Also, I would make this 'misc: syscon@e070', since 'misc'
> does not seem like an appropriate device name.
Ok.
>
>
> > +/* SPEAr1340 Registers */
> > +/* Power Management Registers */
> > +#define SPEAR1340_PCM_CFG 0x100
> > + #define SPEAR1340_PCM_CFG_SATA_POWER_EN 0x800
> > +#define SPEAR1340_PCM_WKUP_CFG 0x104
> > +#define SPEAR1340_SWITCH_CTR 0x108
> > +
> > +#define SPEAR1340_PERIP1_SW_RST0x318
> > + #define SPEAR1340_PERIP1_SW_RST_SATA0x1000
> > +#define SPEAR1340_PERIP2_SW_RST0x31C
> > +#define SPEAR1340_PERIP3_SW_RST0x320
> > +
> > +/* PCIE - SATA configuration registers */
> > +#define SPEAR1340_PCIE_SATA_CFG0x424
> > + /* PCIE CFG MASks */
> > + #define SPEAR1340_PCIE_CFG_DEVICE_PRESENT (1 << 11)
> > + #define SPEAR1340_PCIE_CFG_POWERUP_RESET(1 << 10)
> > + #define SPEAR1340_PCIE_CFG_CORE_CLK_EN (1 << 9)
> > + #define SPEAR1340_PCIE_CFG_AUX_CLK_EN (1 << 8)
> > + #define SPEAR1340_SATA_CFG_TX_CLK_EN(1 << 4)
> > + #define SPEAR1340_SATA_CFG_RX_CLK_EN(1 << 3)
> > + #define SPEAR1340_SATA_CFG_POWERUP_RESET(1 << 2)
> > + #define SPEAR1340_SATA_CFG_PM_CLK_EN(1 << 1)
> > + #define SPEAR1340_PCIE_SATA_SEL_PCIE(0)
> > + #define SPEAR1340_PCIE_SATA_SEL_SATA(1)
> > + #define SPEAR1340_PCIE_SATA_CFG_MASK0xF1F
> > + #define SPEAR1340_PCIE_CFG_VAL (SPEAR1340_PCIE_SATA_SEL_PCIE | \
> > + SPEAR1340_PCIE_CFG_AUX_CLK_EN | \
> > + SPEAR1340_PCIE_CFG_CORE_CLK_EN | \
> > + SPEAR1340_PCIE_CFG_POWERUP_RESET | \
> > + SPEAR1340_PCIE_CFG_DEVICE_PRESENT)
> > + #define SPEAR1340_SATA_CFG_VAL (SPEAR1340_PCIE_SATA_SEL_SATA | \
> > + SPEAR1340_SATA_CFG_PM_CLK_EN | \
> > + SPEAR1340_SATA_CFG_POWERUP_RESET | \
> > + SPEAR1340_SATA_CFG_RX_CLK_EN | \
> > + SPEAR1340_SATA_CFG_TX_CLK_EN)
> > +
> > +#define SPEAR1340_PCIE_MIPHY_CFG 0x428
> > + #define SPEAR1340_MIPHY_OSC_BYPASS_EXT (1 << 31)
> > + #define SPEAR1340_MIPHY_CLK_REF_DIV2(1 << 27)
> > + #define SPEAR1340_MIPHY_CLK_REF_DIV4(2 << 27)
> > + #define SPEAR1340_MIPHY_CLK_REF_DIV8(3 << 27)
> > + #define SPEAR1340_MIPHY_PLL_RATIO_TOP(x)(x << 0)
> > + #define SPEAR1340_PCIE_MIPHY_CFG_MASK 0xF8FF
> > + #define SPEAR1340_PCIE_SATA_MIPHY_CFG_SATA \
> > + (SPEAR1340_MIPHY_OSC_BYPASS_EXT | \
> > + SPEAR1340_MIPHY_CLK_REF_DIV2 | \
> > + SPEAR1340_MIPHY_PLL_RATIO_TOP(60))
> > + #define SPEAR1340_PCIE_SATA_MIPHY_CFG_SATA_25M_CRYSTAL_CLK \
> > + (SPEAR1340_MIPHY_PLL_RATIO_TOP(120))
> > + #define SPEAR1340_PCIE_SATA_MIPHY_CFG_PCIE \
> > + (SPEAR1340_MIPHY_OSC_BYPASS_EXT | \
> > + SP
Re: [PATCH V2 3/8] ahci: Add a driver_data field to struct ahci_platform_data
On Thu, Jan 23, 2014 at 07:36:44PM +0800, Tejun Heo wrote:
> On Thu, Jan 23, 2014 at 04:02:43PM +0530, Mohit Kumar wrote:
> > diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h
> > index 73a2500..76d35e8 100644
> > --- a/include/linux/ahci_platform.h
> > +++ b/include/linux/ahci_platform.h
> > @@ -28,6 +28,7 @@ struct ahci_platform_data {
> > const struct ata_port_info *ata_port_info;
> > unsigned int force_port_map;
> > unsigned int mask_port_map;
> > + void *driver_data;
>
> Please use private_data instead for consistency with other ata data
> structures.
Ok.. ll do that in V3.
Thanks for your review.
Regards
Pratyush
>
> Thanks.
>
> --
> tejun
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Memory mapping of config space of pcie devices
On Sun, Dec 15, 2013 at 04:18:23AM +0800, Bjorn Helgaas wrote: > [+cc devicetree, linux-arm-kernel] > > On Sat, Dec 14, 2013 at 1:05 PM, shiv prakash Agarwal > wrote: > > On Sun, Dec 15, 2013 at 12:29 AM, Bjorn Helgaas wrote: > >> On Sat, Dec 14, 2013 at 11:32 AM, shiv prakash Agarwal > >> wrote: > >>> Hi All, > >>> > >>> How devices config spaces are mapped to host memory? > >>> Is it being handled by core driver? I could not locate. > >> > >> The PCI core does not map config space into memory. That's not even > >> possible for the legacy config access methods, e.g., using I/O ports > >> 0xcf8 and 0xcfc [1]. > >> > >> If you're wondering about how Linux uses ECAM, that's mostly in > >> arch/x86/pci/mmconfig*. That code does ioremap the ECAM area into > >> kernel virtual space, but only for access via pci_read_config_word(), > >> pci_write_config_word(), etc. > >> > >> Bjorn > >> > >> [1] > >> http://en.wikipedia.org/wiki/PCI_configuration_space#Software_implementation > > > > Thanks Bjorn for quick reply, > > > > Above reference says: > > During system initialization, firmware determines the base address for > > this “stolen” address region and communicates it to the root complex > > and to the operating system. This communication method is > > implementation-specific, and not defined in the PCI Express > > specification. > > > > How above is implemented on ARM? > > On x86, we learn the ECAM address via ACPI (the MCFG table or a host > bridge _CBA method). I don't know how this is done on ARM. I could > imagine it being done via device tree, but I really don't know. > Unfortunately both the address discovery and the actual ECAM accesses > are in arch-specific code. Yes, it comes from device tree. DT sends CPU address range allocated for cfg transfers. pci_read_config_word and pci_write_config_word send bdf and offset. Address translation unit of your RC driver should be programmed in such a way that it translates cpu address to the cfg address based on bdf and offset. You can see one such implementation in drivers/pci/host/pcie-designware.c Regards Pratyush > > Bjorn > -- > 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 devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] PCI: designware: Add irq_create_mapping()
On Wed, Oct 09, 2013 at 06:49:15PM +0800, Jingoo Han wrote:
> On Wednesday, October 09, 2013 7:27 PM, Kishon Vijay Abraham I wrote:
> > On Wednesday 09 October 2013 03:35 PM, Jingoo Han wrote:
> > > On Wednesday, October 09, 2013 6:48 PM, Kishon Vijay Abraham I wrote:
> > >> On Wednesday 09 October 2013 02:47 PM, Jingoo Han wrote:
> > >>> On Wednesday, October 09, 2013 6:06 PM, Kishon Vijay Abraham I wrote:
> > On Wednesday 09 October 2013 01:39 PM, Jingoo Han wrote:
> > > Without irq_create_mapping(), the correct irq number cannot be
> > > provided. In this case, it makes problem such as NULL deference.
> > > Thus, irq_create_mapping() should be added for MSI.
> > >
> > > Signed-off-by: Jingoo Han
> > > Cc: Kishon Vijay Abraham I
> > > ---
> > > Tested on Exynos5440.
> > >
> > > drivers/pci/host/pcie-designware.c | 10 --
> > > drivers/pci/host/pcie-designware.h |1 +
> > > 2 files changed, 5 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/pci/host/pcie-designware.c
> > > b/drivers/pci/host/pcie-designware.c
> > > index 8963017..e536bb6 100644
> > > --- a/drivers/pci/host/pcie-designware.c
> > > +++ b/drivers/pci/host/pcie-designware.c
> > > @@ -237,6 +237,8 @@ static int assign_irq(int no_irqs, struct
> > > msi_desc *desc, int *pos)
> > > }
> > > }
> > >
> > > + pp->msi_irq_start = irq_create_mapping(pp->irq_domain, 0);
> > > +
> >
> > I think irq_create_mapping should be done for all the MSI irq lines
> > instead of
> > only the first line. So you might have to do for MAX_MSI_IRQS lines.
> > >>
> > >> Maybe it should be only till MAX_MSI_IRQS-1?
> >
> > I meant something like this,
> >
> > for (i = 0; i < MAX_MSI_IRQS; i++)
> > irq_create_mapping(pp->irq_domain, i);
> >
> > That didn't give me any issues though.
>
> However, no driver calls irq_create_mapping() like this.
> For example, Tegra PCI driver gives 'hwirq' as single offset value
> to irq_create_mapping() without any loop.
>
> static int tegra_msi_setup_irq(struct msi_chip *chip, struct pci_dev *pdev,
>struct msi_desc *desc)
> {
> hwirq = tegra_msi_alloc(msi);
> irq = irq_create_mapping(msi->domain, hwirq);
>
> Maybe, the following can be used, it uses 'pos0' as the offset value.
>
> pp->msi_irq_start = irq_create_mapping(pp->irq_domain, pos0);
> irq = pp->msi_irq_start;
>
Documentation/IRQ-domain.txt says that "The irq_create_mapping()
function must be called *atleast once* before any call to
irq_find_mapping(), lest the descriptor will not be allocated."
So for sure current code need to be modified.
Now, you can create the mapping statically during initialization and
then use it dynamically whenever needed. In that case what Kishon
suggested is right, something like following should work.
for (i = 0; i < MAX_MSI_IRQS; i++)
irq_create_mapping(pp->irq_domain, i);
pp->msi_irq_start = irq_find_mapping(pp->irq_domain, 0);
But, I am not sure that created mapping is continuous. I mean,
difference between irq returned by
irq_find_mapping(pp->irq_domain, 0)
&
irq_find_mapping(pp->irq_domain, 9)
is always 9. If that is not the case then, static assignment of
msi_irq_start will not work. May be you need something as follows:
diff --git a/drivers/pci/host/pcie-designware.c
b/drivers/pci/host/pcie-designware.c
index 8963017..e6749e8 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -157,7 +157,7 @@ static struct irq_chip dw_msi_irq_chip = {
void dw_handle_msi_irq(struct pcie_port *pp)
{
unsigned long val;
- int i, pos;
+ int i, pos, irq;
for (i = 0; i < MAX_MSI_CTRLS; i++) {
dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_STATUS + i * 12, 4,
@@ -165,8 +165,9 @@ void dw_handle_msi_irq(struct pcie_port *pp)
if (val) {
pos = 0;
while ((pos = find_next_bit(&val, 32, pos)) != 32) {
- generic_handle_irq(pp->msi_irq_start
- + (i * 32) + pos);
+ irq = irq_find_mapping(pp->irq_domain,
+ i * 32 + pos);
+ generic_handle_irq(irq);
pos++;
}
}
@@ -237,9 +238,8 @@ static int assign_irq(int no_irqs, struct msi_desc *desc,
int *pos)
}
}
- irq = (pp->msi_irq_start + pos0);
-
- if ((irq + no_irqs) > (pp->msi_irq_start + MAX_MSI_IRQS-1))
+ irq = irq_find_mapping(pp->irq_domain, pos0);
+ if (!irq)
goto no_valid_irq;
i = 0;
@@ -270,6 +270,7 @@ static void clear_irq(unsigned int irq)
struct irq_desc *desc;
