| 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: Tue, 09 Jul 2013 10:13:56 +0900
From: Jingoo Han <jg1.han@...sung.com>
To: 'Arnd Bergmann' <arnd@...db.de>
Cc: 'Bjorn Helgaas' <bhelgaas@...gle.com>, linux-pci@...r.kernel.org,
linux-samsung-soc@...r.kernel.org,
'Kukjin Kim' <kgene.kim@...sung.com>,
'Pratyush Anand' <pratyush.anand@...com>,
'Mohit KUMAR' <Mohit.KUMAR@...com>,
'Sean Cross' <xobs@...agi.com>,
'SRIKANTH TUMKUR SHIVANAND' <ts.srikanth@...sung.com>,
linux-kernel@...r.kernel.org, Jingoo Han <jg1.han@...sung.com>
Subject: Re: [PATCH] pci: exynos: split into two parts such as Synopsys part
and Exynos part
On Friday, July 05, 2013 7:46 PM, Arnd Bergmann wrote:
> On Friday 05 July 2013, Jingoo Han wrote:
>
> > --- /dev/null
> > +++ b/drivers/pci/host/pcie-exynos.c
>
> > +
> > +/* PCIe ELBI registers */
> > +#define PCIE_IRQ_PULSE 0x000
> > +#define IRQ_INTA_ASSERT (0x1 << 0)
> > +#define IRQ_INTB_ASSERT (0x1 << 2)
> > +#define IRQ_INTC_ASSERT (0x1 << 4)
> > +#define IRQ_INTD_ASSERT (0x1 << 6)
> > +#define PCIE_IRQ_LEVEL 0x004
> > +#define PCIE_IRQ_SPECIAL 0x008
> > +#define PCIE_IRQ_EN_PULSE 0x00c
> > +#define PCIE_IRQ_EN_LEVEL 0x010
> > +#define PCIE_IRQ_EN_SPECIAL 0x014
> > +#define PCIE_PWR_RESET 0x018
> > +#define PCIE_CORE_RESET 0x01c
> > +#define PCIE_CORE_RESET_ENABLE (0x1 << 0)
> > +#define PCIE_STICKY_RESET 0x020
> > +#define PCIE_NONSTICKY_RESET 0x024
> > +#define PCIE_APP_INIT_RESET 0x028
> > +#define PCIE_APP_LTSSM_ENABLE 0x02c
> > +#define PCIE_ELBI_RDLH_LINKUP 0x064
> > +#define PCIE_ELBI_LTSSM_ENABLE 0x1
> > +#define PCIE_ELBI_SLV_AWMISC 0x11c
> > +#define PCIE_ELBI_SLV_ARMISC 0x120
> > +#define PCIE_ELBI_SLV_DBI_ENABLE (0x1 << 21)
> > +
> > +/* PCIe Purple registers */
> > +#define PCIE_PHY_GLOBAL_RESET 0x000
> > +#define PCIE_PHY_COMMON_RESET 0x004
> > +#define PCIE_PHY_CMN_REG 0x008
> > +#define PCIE_PHY_MAC_RESET 0x00c
> > +#define PCIE_PHY_PLL_LOCKED 0x010
> > +#define PCIE_PHY_TRSVREG_RESET 0x020
> > +#define PCIE_PHY_TRSV_RESET 0x024
> > +
> > +/* PCIe PHY registers */
> > +#define PCIE_PHY_IMPEDANCE 0x004
> > +#define PCIE_PHY_PLL_DIV_0 0x008
> > +#define PCIE_PHY_PLL_BIAS 0x00c
> > +#define PCIE_PHY_DCC_FEEDBACK 0x014
> > +#define PCIE_PHY_PLL_DIV_1 0x05c
> > +#define PCIE_PHY_TRSV0_EMP_LVL 0x084
> > +#define PCIE_PHY_TRSV0_DRV_LVL 0x088
> > +#define PCIE_PHY_TRSV0_RXCDR 0x0ac
> > +#define PCIE_PHY_TRSV0_LVCC 0x0dc
> > +#define PCIE_PHY_TRSV1_EMP_LVL 0x144
> > +#define PCIE_PHY_TRSV1_RXCDR 0x16c
> > +#define PCIE_PHY_TRSV1_LVCC 0x19c
> > +#define PCIE_PHY_TRSV2_EMP_LVL 0x204
> > +#define PCIE_PHY_TRSV2_RXCDR 0x22c
> > +#define PCIE_PHY_TRSV2_LVCC 0x25c
> > +#define PCIE_PHY_TRSV3_EMP_LVL 0x2c4
> > +#define PCIE_PHY_TRSV3_RXCDR 0x2ec
> > +#define PCIE_PHY_TRSV3_LVCC 0x31c
>
> Are you sure these are all exynos specific? Maybe they are licensed
> from someone else?
Samsung specific.
I asked our hardware engineer, and he confirmed it.
>
> > +
> > + pp->dbi_base = devm_ioremap(&pdev->dev, pp->cfg.start,
> > + resource_size(&pp->cfg));
> > + if (!pp->dbi_base) {
> > + dev_err(&pdev->dev, "error with ioremap\n");
> > + return -ENOMEM;
> > + }
> > +
> > + pp->root_bus_nr = -1;
> > + pp->ops = &exynos_pcie_host_ops;
> > +
> > + spin_lock_init(&pp->conf_lock);
> > + dw_pcie_host_init(pp);
> > + pp->va_cfg0_base = devm_ioremap(&pdev->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(&pdev->dev, pp->cfg1_base,
> > + pp->config.cfg1_size);
> > + if (!pp->va_cfg1_base) {
> > + dev_err(pp->dev, "error with ioremap\n");
> > + return -ENOMEM;
> > + }
>
> I think the configuration space handling should go into the
> dw_pcie_host_init function, as that part would be needed by all drivers.
OK.
I will move it to dw_pcie_host_init().
>
> > +static int __init exynos_pcie_probe(struct platform_device *pdev)
> > +{
> > + struct pcie_port *pp;
> > + struct device_node *np = pdev->dev.of_node;
> > + struct of_pci_range range;
> > + struct of_pci_range_parser parser;
> > + int ret;
> > +
> > + pp = devm_kzalloc(&pdev->dev, sizeof(*pp), GFP_KERNEL);
> > + if (!pp) {
> > + dev_err(&pdev->dev, "no memory for pcie port\n");
> > + return -ENOMEM;
> > + }
> > +
> > + pp->dev = &pdev->dev;
> > +
> > + if (of_pci_range_parser_init(&parser, np)) {
> > + dev_err(&pdev->dev, "missing ranges property\n");
> > + return -EINVAL;
> > + }
> > +
> > + /* Get the I/O and memory ranges from DT */
> > + for_each_of_pci_range(&parser, &range) {
> > + unsigned long restype = range.flags & IORESOURCE_TYPE_BITS;
> > + if (restype == IORESOURCE_IO) {
> > + of_pci_range_to_resource(&range, np, &pp->io);
> > + pp->io.name = "I/O";
> > + pp->io.start = max_t(resource_size_t,
> > + PCIBIOS_MIN_IO,
> > + range.pci_addr + global_io_offset);
> > + pp->io.end = min_t(resource_size_t,
> > + IO_SPACE_LIMIT,
> > + range.pci_addr + range.size
> > + + global_io_offset);
> > + pp->config.io_size = resource_size(&pp->io);
> > + pp->config.io_bus_addr = range.pci_addr;
> > + }
> > + if (restype == IORESOURCE_MEM) {
> > + of_pci_range_to_resource(&range, np, &pp->mem);
> > + pp->mem.name = "MEM";
> > + pp->config.mem_size = resource_size(&pp->mem);
> > + pp->config.mem_bus_addr = range.pci_addr;
> > + }
> > + if (restype == 0) {
> > + of_pci_range_to_resource(&range, np, &pp->cfg);
> > + pp->config.cfg0_size = resource_size(&pp->cfg)/2;
> > + pp->config.cfg1_size = resource_size(&pp->cfg)/2;
> > + }
> > + }
>
> Same for these: it would be better to split up the probe function here
> and move all of the above into the common code.
OK.
I will move it to dw_pcie_host_init().
>
> > + pp->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
>
> I assume the reset logic would need to remain exynos specific
>
> > + pp->clk = devm_clk_get(&pdev->dev, "pcie");
> > + if (IS_ERR(pp->clk)) {
> > + dev_err(&pdev->dev, "Failed to get pcie rc clock\n");
> > + return PTR_ERR(pp->clk);
> > + }
> > + ret = clk_prepare_enable(pp->clk);
> > + if (ret)
> > + return ret;
> > +
> > + pp->bus_clk = devm_clk_get(&pdev->dev, "pcie_bus");
> > + if (IS_ERR(pp->bus_clk)) {
> > + dev_err(&pdev->dev, "Failed to get pcie bus clock\n");
> > + ret = PTR_ERR(pp->bus_clk);
> > + goto fail_clk;
> > + }
> > + ret = clk_prepare_enable(pp->bus_clk);
> > + if (ret)
> > + goto fail_clk;
>
> For the clocks, I think we can keep them in the common code, but
> make them optional, i.e. do not fail the probe function if the clocks
> don't exist, i.e. you get -ENOENT from devm_clk_get. We should however
> still fail here if devm_clk_get returns a different error.
>
> I also noticed an existing bug in your code here: if the clock controller
> is not yet initialized, devm_clk_get() will return -EPROBE_DEFER,
> which is broken in combination with platform_driver_probe. In order
> to fix that, just change the code to use platform_driver_register
> so it has the option to retry the probe after the clock controller
> is initialized.
> That should be a separate patch.
If I should do it, I will send a separate patch.
>
> > +static int exynos_pcie_abort(unsigned long addr, unsigned int fsr,
> > + struct pt_regs *regs)
> > +{
> > + unsigned long pc = instruction_pointer(regs);
> > + unsigned long instr = *(unsigned long *)pc;
> > +
> > + WARN_ONCE(1, "pcie abort\n");
> > +
> > + /*
> > + * If the instruction being executed was a read,
> > + * make it look like it read all-ones.
> > + */
> > + if ((instr & 0x0c100000) == 0x04100000) {
> > + int reg = (instr >> 12) & 15;
> > + unsigned long val;
> > +
> > + if (instr & 0x00400000)
> > + val = 255;
> > + else
> > + val = -1;
> > +
> > + regs->uregs[reg] = val;
> > + regs->ARM_pc += 4;
> > + return 0;
> > + }
> > +
> > + if ((instr & 0x0e100090) == 0x00100090) {
> > + int reg = (instr >> 12) & 15;
> > +
> > + regs->uregs[reg] = -1;
> > + regs->ARM_pc += 4;
> > + return 0;
> > + }
> > +
> > + return 1;
> > +}
>
> Another observation that is only partially related: the abort handler logic
> is not specific to either exynos nor synopsys. However it is specific
> to ARM and should not be part of a driver that is potentially architecture
> independent. I would recommend moving this to a separate file
> "arm-pci-abort.c" or something like that. I notice that a lot of the other
> ARM PCI implementations have the exact same code but also need to reset
> the abort in the bridge. Can you confirm that a) you don't need to tell
> the root port about the abort and b) the abort handler is actually required
> for exynos? How often do you run into this?
There is no special reason to add it.
I just referenced other ARM PCI drivers.
Also, I have never seen calling this abort function.
Then, I will remove it.
Thank you for your opinion. :)
Your comment is very helpful.
Best regards,
Jingoo Han
--
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