| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 3 Mar 2014 23:43:21 +0530
From: Srikanth Thokala <sthokal@...inx.com>
To: Jason Gunthorpe <jgunthorpe@...idianresearch.com>
Cc: Srikanth Thokala <sthokal@...inx.com>,
Bjorn Helgaas <bhelgaas@...gle.com>,
Michal Simek <michal.simek@...inx.com>,
Grant Likely <grant.likely@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
devicetree@...r.kernel.org
Subject: Re: [PATCH v2] pcie: Add Xilinx PCIe Host Bridge IP driver
On Mon, Mar 3, 2014 at 11:13 PM, Jason Gunthorpe
<jgunthorpe@...idianresearch.com> wrote:
> On Mon, Mar 03, 2014 at 07:10:36PM +0530, Srikanth Thokala wrote:
>
>> +Required properties:
>> +- #address-cells: Address representation for root ports, set to <3>
>> +- #size-cells: Size representation for root ports, set to <2>
>> +- compatible: Should contain "xlnx,axi-pcie-host-1.00.a"
>> +- reg: Should contain AXI PCIe registers location and length
>> +- interrupts: Should contain AXI PCIe interrupt
>> +- ranges: ranges for the PCI memory regions
>> + Please refer to the standard PCI bus binding document for a more
>> + detailed explanation
>> +
>> +Example:
>> +++++++++
>> +
>> + pci_express: axi-pcie@...00000 {
>> + #address-cells = <3>;
>> + #size-cells = <2>;
>> + compatible = "xlnx,axi-pcie-host-1.00.a";
>> + reg = < 0x50000000 0x10000000 >;
>> + interrupts = < 0 52 4 >;
>> + ranges = < 0x02000000 0 0x60000000 0x60000000 0 0x10000000 >;
>> + };
>
> That is much shorter, more notes:
> - It looks like the driver (HW?) has no support for IO space?
Yes, there is no support for IO space.
> - How are INTA/B/C/D handled? Typically I'd like to see an
> interrupt-mapping block describing them.
/
/
CPU <--- PCIe IP <---
>
> There are a few options here, if the HW can decode A/B/C/D then
> it probably needs another irq_domain for INTx, and have the IRQ
> handler decode like it does for MSI.
> - The stanza needs a
> device_type = "pci";
>
>> + for (i = 0; i < port->mem_resno; i++)
>> + pci_add_resource_offset(&sys->resources, &port->mem[i],
>> + sys->mem_offset);
>
> The sys->mem_offset is probably wrong, please review Arnd's comments
> on the recent threads with Will and Liviu regarding how this is
> supposed to work with DT.
>
> You may want to work with Liviu to use his patch set which provides
> more generic support. Also, the driver needs to put every one of these
> ports in a PCI domain, Liviu is working on generic support for that
> too.
Sure, I will check and come back to you.
>
>> +static struct pci_bus __init *xilinx_pcie_scan_bus(int nr,
>> + struct pci_sys_data *sys)
>> +{
>> + struct xilinx_pcie_port *port = sys_to_pcie(sys);
>> + struct pci_bus *bus;
>> +
>> + if (port) {
>> + port->root_busno = sys->busnr;
>> + bus = pci_scan_root_bus(NULL, sys->busnr, &xilinx_pcie_ops,
>> + sys, &sys->resources);
>
> The NULL is probably wrong, please review the recent thread with Lucas
> and Jingoo 'PCI irq mapping fixes and cleanups'
Ok.
>
>> +static int xilinx_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
>> +{
>> + struct xilinx_pcie_port *port = sys_to_pcie(dev->bus->sysdata);
>> +
>> + if (!port)
>> + return -EINVAL;
>> +
>> + return port->irq;
>> +}
>
> It is preferred to use DT mechanisms via of_irq_parse_and_map_pci
> (interrupt-mapping, etc) to define the slot IRQ, drivers should not
> provide a map_irq unless absolutely necessary.
Ok.
>
>> + /* make sure it is root port before touching header */
>> + pcie_write(port, PCI_COMMAND_MASTER, PCI_COMMAND);
>
> Bit of a confusing comment
I will correct it.
>
>> + /* Check if PCIe link is up? */
>> + port->link_up = xilinx_pcie_is_link_up(port);
>> + if (!port->link_up)
>> + dev_info(port->dev, "PCIe Link is DOWN\n");
>> + else
>> + dev_info(port->dev, "PCIe Link is UP\n");
>
> Don't forget to think about hot plug
Ok, you mean using the 'rescan' correct?
>
>> + /* Register Interrupt Handler */
>> + err = request_irq(port->irq, xilinx_pcie_intr_handler,
>> + IRQF_SHARED, "xilinx-pcie", port);
>
> Use devm? This is leaking on error cases.
>
> Review other resources too...
>
>> + /* Request and map I/O memory */
>> + io = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + port->phys_reg_base = io->start;
>> + port->reg_size = resource_size(io);
>
> Here it uses the platform resource..
>
>> + /* Map the IRQ */
>> + port->irq = irq_of_parse_and_map(node, 0);
>
> .. And here it doesn't, probably should be consistent and just use the
> platform resources.
Ok, I will correct it.
Srikanth
>
> Jason
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists