Hi Guenter,
Thanks for your review, and I think it still not too late
for nitpicking as it isn't merged to next branch. :)
We have amend the code a bit, so probably we fixed some of
the minor issues against V10. But some of them are really
personal taste, if you still insist on that, I will be ok
to modify them.
I will push patch to fix them after your feedback. :)
On 2016/9/1 2:17, Guenter Roeck wrote:
> On Fri, Aug 19, 2016 at 09:34:58AM +0800, Shawn Lin wrote:
[...]
>> + rockchip_pcie_enable_interrupts(port);
>> +
>
> There is no matching disable_interrupts call in error handling after this.
> Is that ok ?
>
From test, probably ok since the genpd will be off and all of them wil
be cleared.
> Also, is it ok to enable interrupts this early (before the rest of the
> initialization is complete) ?
>
The training starts, so we some client irq should be handled now, and we
already register isr.
>> + err = rockchip_pcie_init_irq_domain(port);
>> + if (err < 0)
>> + goto err_vpcie;
>> +
>> + err = of_pci_get_host_bridge_resources(dev->of_node, 0, 0xff,
>> + &res, &io_base);
>> + if (err)
>> + goto err_vpcie;
>> +
>> + err = devm_request_pci_bus_resources(dev, &res);
>> + if (err)
>> + goto err_vpcie;
>> +
>> + /* Get the I/O and memory ranges from DT */
>> + resource_list_for_each_entry(win, &res) {
>> + switch (resource_type(win->res)) {
>> + case IORESOURCE_IO:
>> + io = win->res;
>> + io->name = "I/O";
>> + io_size = resource_size(io);
>> + io_bus_addr = io->start - win->offset;
>> + err = pci_remap_iospace(io, io_base);
>> + if (err) {
>> + dev_warn(port->dev, "error %d: failed to map resource %pR\n",
>> + err, io);
>> + continue;
>> + }
>> + break;
>> + case IORESOURCE_MEM:
>> + mem = win->res;
>> + mem->name = "MEM";
>> + mem_size = resource_size(mem);
>> + mem_bus_addr = mem->start - win->offset;
>> + break;
>> + case IORESOURCE_BUS:
>> + busn = win->res;
>> + break;
>> + default:
>> + continue;
>> + }
>> + }
>> +
>> + if (mem_size)
>
> While strictly speaking not needed, I would recommend { }
> here to improve readability.
>
>> + for (reg_no = 0; reg_no < (mem_size >> 20); reg_no++) {
>
> This doesn't support mem sizes smaller than 1 << 20 (and clips the size
> if it is not aligned to 1M). Is this intentional ?
yes, we don't support.
>
>> + err = rockchip_pcie_prog_ob_atu(port, reg_no + 1,
>> + AXI_WRAPPER_MEM_WRITE,
>> + 20 - 1,
>> + mem_bus_addr +
>> + (reg_no << 20),
>> + 0);
>> + if (err) {
>> + dev_err(dev, "program RC mem outbound ATU failed\n");
>> + goto err_vpcie;
>> + }
>> + }
>
>> + err = rockchip_pcie_prog_ob_atu(port,
>> + reg_no + 1 + offset,
>> + AXI_WRAPPER_IO_WRITE,
>> + 20 - 1,
>> + io_bus_addr +
>> + (reg_no << 20),
>> + 0);
>> + if (err) {
>> + dev_err(dev, "program RC io outbound ATU failed\n");
>> + goto err_vpcie;
>> + }
>> + }
>> +
>> + pci_bus_add_devices(bus);
>> +
>> + dev_warn(dev, "only 32-bit config accesses supported; smaller writes may corrupt adjacent RW1C fields\n");
>> +
>
> A warning which is always displayed seems to be a bit useless. If this is a
> concern, maybe the driver should provide its own config space access functions
> and map smaller accesses into 32 bit accesses. But isn't that already done ?
>
No, that is just for normal code path. When users comfigure it via some
user-space tool, it is sane to make them know this limitation. So we was
suggested to do this.
>> + return err;
>> +
>> +err_vpcie:
>> + if (!IS_ERR(port->vpcie3v3))
>> + regulator_disable(port->vpcie3v3);
>> + if (!IS_ERR(port->vpcie1v8))
>> + regulator_disable(port->vpcie1v8);
>> + if (!IS_ERR(port->vpcie0v9))
>> + regulator_disable(port->vpcie0v9);
>> +err_set_vpcie:
>> + clk_disable_unprepare(port->clk_pcie_pm);
>> +err_pcie_pm:
>> + clk_disable_unprepare(port->hclk_pcie);
>> +err_hclk_pcie:
>> + clk_disable_unprepare(port->aclk_perf_pcie);
>> +err_aclk_perf_pcie:
>> + clk_disable_unprepare(port->aclk_pcie);
>> +err_aclk_pcie:
>> + return err;
>> +}
>> +
>> +static const struct of_device_id rockchip_pcie_of_match[] = {
>> + { .compatible = "rockchip,rk3399-pcie", },
>> + {}
>> +};
>> +
>> +static struct platform_driver rockchip_pcie_driver = {
>> + .driver = {
>> + .name = "rockchip-pcie",
>> + .of_match_table = rockchip_pcie_of_match,
>> + },
>> + .probe = rockchip_pcie_probe,
>> +
>> +};
>> +builtin_platform_driver(rockchip_pcie_driver);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Best Regards
Shawn Lin
