From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> To: honghui.zhang@mediatek.com Cc: bhelgaas@google.com, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, linux-pci@vger.kernel.org, ryder.lee@mediatek.com, linux-kernel@vger.kernel.org, youlin.pei@mediatek.com, yt.shen@mediatek.com, jianjun.wang@mediatek.com Subject: Re: [PATCH] PCI: mediatek: Use devm_of_pci_get_host_bridge_resources() to parse DT Date: Wed, 7 Nov 2018 12:03:33 +0000 [thread overview] Message-ID: <20181107120333.GB11586@e107981-ln.cambridge.arm.com> (raw) In-Reply-To: <1539833014-1738-1-git-send-email-honghui.zhang@mediatek.com> On Thu, Oct 18, 2018 at 11:23:34AM +0800, honghui.zhang@mediatek.com wrote: > From: Honghui Zhang <honghui.zhang@mediatek.com> > > Use the devm_of_pci_get_host_bridge_resources() API in place of the PCI OF > DT parser. > > Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com> > --- > drivers/pci/controller/pcie-mediatek.c | 109 +++++++++------------------------ > 1 file changed, 29 insertions(+), 80 deletions(-) > > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c > index 2a1f97c..6632d4e 100644 > --- a/drivers/pci/controller/pcie-mediatek.c > +++ b/drivers/pci/controller/pcie-mediatek.c > @@ -197,29 +197,20 @@ struct mtk_pcie_port { > * @dev: pointer to PCIe device > * @base: IO mapped register base > * @free_ck: free-run reference clock > - * @io: IO resource > - * @pio: PIO resource > * @mem: non-prefetchable memory resource > - * @busn: bus range > - * @offset: IO / Memory offset > * @ports: pointer to PCIe port information > * @soc: pointer to SoC-dependent operations > + * @busnr: root bus number > */ > struct mtk_pcie { > struct device *dev; > void __iomem *base; > struct clk *free_ck; > > - struct resource io; > - struct resource pio; > struct resource mem; > - struct resource busn; > - struct { > - resource_size_t mem; > - resource_size_t io; > - } offset; > struct list_head ports; > const struct mtk_pcie_soc *soc; > + int busnr; If you move the code initializing and probing the pci_host_bridge into mtk_pcie_setup() (and rename that function) this variable becomes useless. It should be an unsigned int, by the way. > }; > > static void mtk_pcie_subsys_powerdown(struct mtk_pcie *pcie) > @@ -1045,55 +1036,42 @@ static int mtk_pcie_setup(struct mtk_pcie *pcie) > { > struct device *dev = pcie->dev; > struct device_node *node = dev->of_node, *child; > - struct of_pci_range_parser parser; > - struct of_pci_range range; > - struct resource res; > struct mtk_pcie_port *port, *tmp; > + struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie); > + struct list_head *windows = &host->windows; > + struct resource_entry *win, *tmp_win; > + resource_size_t io_base; > int err; > > - if (of_pci_range_parser_init(&parser, node)) { > - dev_err(dev, "missing \"ranges\" property\n"); > - return -EINVAL; > - } > + err = devm_of_pci_get_host_bridge_resources(dev, 0, 0xff, > + windows, &io_base); > + if (err) > + return err; > > - for_each_of_pci_range(&parser, &range) { > - err = of_pci_range_to_resource(&range, node, &res); > - if (err < 0) > - return err; > + err = devm_request_pci_bus_resources(dev, windows); > + if (err < 0) > + return err; > > - switch (res.flags & IORESOURCE_TYPE_BITS) { > + /* Get the I/O and memory ranges from DT */ > + resource_list_for_each_entry_safe(win, tmp_win, windows) { > + switch (resource_type(win->res)) { > case IORESOURCE_IO: > - pcie->offset.io = res.start - range.pci_addr; > - > - memcpy(&pcie->pio, &res, sizeof(res)); > - pcie->pio.name = node->full_name; > - > - pcie->io.start = range.cpu_addr; > - pcie->io.end = range.cpu_addr + range.size - 1; > - pcie->io.flags = IORESOURCE_MEM; > - pcie->io.name = "I/O"; > - > - memcpy(&res, &pcie->io, sizeof(res)); > - break; > - > + err = devm_pci_remap_iospace(dev, win->res, io_base); > + if (err) { > + dev_warn(dev, "error %d: failed to map resource %pR\n", > + err, win->res); > + resource_list_destroy_entry(win); > + } Missing a break. > case IORESOURCE_MEM: > - pcie->offset.mem = res.start - range.pci_addr; > - > - memcpy(&pcie->mem, &res, sizeof(res)); > + memcpy(&pcie->mem, win->res, sizeof(*win->res)); > pcie->mem.name = "non-prefetchable"; > break; > + case IORESOURCE_BUS: > + pcie->busnr = win->res->start; > + break; > } > } > > - err = of_pci_parse_bus_range(node, &pcie->busn); > - if (err < 0) { > - dev_err(dev, "failed to parse bus ranges property: %d\n", err); > - pcie->busn.name = node->name; > - pcie->busn.start = 0; > - pcie->busn.end = 0xff; > - pcie->busn.flags = IORESOURCE_BUS; > - } > - > for_each_available_child_of_node(node, child) { > int slot; > > @@ -1125,28 +1103,6 @@ static int mtk_pcie_setup(struct mtk_pcie *pcie) > return 0; > } > > -static int mtk_pcie_request_resources(struct mtk_pcie *pcie) > -{ > - struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie); > - struct list_head *windows = &host->windows; > - struct device *dev = pcie->dev; > - int err; > - > - pci_add_resource_offset(windows, &pcie->pio, pcie->offset.io); > - pci_add_resource_offset(windows, &pcie->mem, pcie->offset.mem); > - pci_add_resource(windows, &pcie->busn); > - > - err = devm_request_pci_bus_resources(dev, windows); > - if (err < 0) > - return err; > - > - err = devm_pci_remap_iospace(dev, &pcie->pio, pcie->io.start); > - if (err < 0) > - return err; > - > - return 0; > -} > - > static int mtk_pcie_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -1169,11 +1125,7 @@ static int mtk_pcie_probe(struct platform_device *pdev) > if (err) > return err; > > - err = mtk_pcie_request_resources(pcie); > - if (err) > - goto put_resources; > - > - host->busnr = pcie->busn.start; > + host->busnr = pcie->busnr; > host->dev.parent = pcie->dev; > host->ops = pcie->soc->ops; > host->map_irq = of_irq_parse_and_map_pci; > @@ -1181,12 +1133,9 @@ static int mtk_pcie_probe(struct platform_device *pdev) > host->sysdata = pcie; > > err = pci_host_probe(host); > - if (err) > - goto put_resources; > - > - return 0; > + if (!err) > + return 0; > No, leave the error path. err = pci_host_probe(host); if (err) goto put_resources; return 0; put_resources: ... it is easier to follow. Thanks, Lorenzo > -put_resources: > if (!list_empty(&pcie->ports)) > mtk_pcie_put_resources(pcie); > > -- > 2.6.4 >
WARNING: multiple messages have this Message-ID (diff)
From: lorenzo.pieralisi@arm.com (Lorenzo Pieralisi) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH] PCI: mediatek: Use devm_of_pci_get_host_bridge_resources() to parse DT Date: Wed, 7 Nov 2018 12:03:33 +0000 [thread overview] Message-ID: <20181107120333.GB11586@e107981-ln.cambridge.arm.com> (raw) In-Reply-To: <1539833014-1738-1-git-send-email-honghui.zhang@mediatek.com> On Thu, Oct 18, 2018 at 11:23:34AM +0800, honghui.zhang at mediatek.com wrote: > From: Honghui Zhang <honghui.zhang@mediatek.com> > > Use the devm_of_pci_get_host_bridge_resources() API in place of the PCI OF > DT parser. > > Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com> > --- > drivers/pci/controller/pcie-mediatek.c | 109 +++++++++------------------------ > 1 file changed, 29 insertions(+), 80 deletions(-) > > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c > index 2a1f97c..6632d4e 100644 > --- a/drivers/pci/controller/pcie-mediatek.c > +++ b/drivers/pci/controller/pcie-mediatek.c > @@ -197,29 +197,20 @@ struct mtk_pcie_port { > * @dev: pointer to PCIe device > * @base: IO mapped register base > * @free_ck: free-run reference clock > - * @io: IO resource > - * @pio: PIO resource > * @mem: non-prefetchable memory resource > - * @busn: bus range > - * @offset: IO / Memory offset > * @ports: pointer to PCIe port information > * @soc: pointer to SoC-dependent operations > + * @busnr: root bus number > */ > struct mtk_pcie { > struct device *dev; > void __iomem *base; > struct clk *free_ck; > > - struct resource io; > - struct resource pio; > struct resource mem; > - struct resource busn; > - struct { > - resource_size_t mem; > - resource_size_t io; > - } offset; > struct list_head ports; > const struct mtk_pcie_soc *soc; > + int busnr; If you move the code initializing and probing the pci_host_bridge into mtk_pcie_setup() (and rename that function) this variable becomes useless. It should be an unsigned int, by the way. > }; > > static void mtk_pcie_subsys_powerdown(struct mtk_pcie *pcie) > @@ -1045,55 +1036,42 @@ static int mtk_pcie_setup(struct mtk_pcie *pcie) > { > struct device *dev = pcie->dev; > struct device_node *node = dev->of_node, *child; > - struct of_pci_range_parser parser; > - struct of_pci_range range; > - struct resource res; > struct mtk_pcie_port *port, *tmp; > + struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie); > + struct list_head *windows = &host->windows; > + struct resource_entry *win, *tmp_win; > + resource_size_t io_base; > int err; > > - if (of_pci_range_parser_init(&parser, node)) { > - dev_err(dev, "missing \"ranges\" property\n"); > - return -EINVAL; > - } > + err = devm_of_pci_get_host_bridge_resources(dev, 0, 0xff, > + windows, &io_base); > + if (err) > + return err; > > - for_each_of_pci_range(&parser, &range) { > - err = of_pci_range_to_resource(&range, node, &res); > - if (err < 0) > - return err; > + err = devm_request_pci_bus_resources(dev, windows); > + if (err < 0) > + return err; > > - switch (res.flags & IORESOURCE_TYPE_BITS) { > + /* Get the I/O and memory ranges from DT */ > + resource_list_for_each_entry_safe(win, tmp_win, windows) { > + switch (resource_type(win->res)) { > case IORESOURCE_IO: > - pcie->offset.io = res.start - range.pci_addr; > - > - memcpy(&pcie->pio, &res, sizeof(res)); > - pcie->pio.name = node->full_name; > - > - pcie->io.start = range.cpu_addr; > - pcie->io.end = range.cpu_addr + range.size - 1; > - pcie->io.flags = IORESOURCE_MEM; > - pcie->io.name = "I/O"; > - > - memcpy(&res, &pcie->io, sizeof(res)); > - break; > - > + err = devm_pci_remap_iospace(dev, win->res, io_base); > + if (err) { > + dev_warn(dev, "error %d: failed to map resource %pR\n", > + err, win->res); > + resource_list_destroy_entry(win); > + } Missing a break. > case IORESOURCE_MEM: > - pcie->offset.mem = res.start - range.pci_addr; > - > - memcpy(&pcie->mem, &res, sizeof(res)); > + memcpy(&pcie->mem, win->res, sizeof(*win->res)); > pcie->mem.name = "non-prefetchable"; > break; > + case IORESOURCE_BUS: > + pcie->busnr = win->res->start; > + break; > } > } > > - err = of_pci_parse_bus_range(node, &pcie->busn); > - if (err < 0) { > - dev_err(dev, "failed to parse bus ranges property: %d\n", err); > - pcie->busn.name = node->name; > - pcie->busn.start = 0; > - pcie->busn.end = 0xff; > - pcie->busn.flags = IORESOURCE_BUS; > - } > - > for_each_available_child_of_node(node, child) { > int slot; > > @@ -1125,28 +1103,6 @@ static int mtk_pcie_setup(struct mtk_pcie *pcie) > return 0; > } > > -static int mtk_pcie_request_resources(struct mtk_pcie *pcie) > -{ > - struct pci_host_bridge *host = pci_host_bridge_from_priv(pcie); > - struct list_head *windows = &host->windows; > - struct device *dev = pcie->dev; > - int err; > - > - pci_add_resource_offset(windows, &pcie->pio, pcie->offset.io); > - pci_add_resource_offset(windows, &pcie->mem, pcie->offset.mem); > - pci_add_resource(windows, &pcie->busn); > - > - err = devm_request_pci_bus_resources(dev, windows); > - if (err < 0) > - return err; > - > - err = devm_pci_remap_iospace(dev, &pcie->pio, pcie->io.start); > - if (err < 0) > - return err; > - > - return 0; > -} > - > static int mtk_pcie_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -1169,11 +1125,7 @@ static int mtk_pcie_probe(struct platform_device *pdev) > if (err) > return err; > > - err = mtk_pcie_request_resources(pcie); > - if (err) > - goto put_resources; > - > - host->busnr = pcie->busn.start; > + host->busnr = pcie->busnr; > host->dev.parent = pcie->dev; > host->ops = pcie->soc->ops; > host->map_irq = of_irq_parse_and_map_pci; > @@ -1181,12 +1133,9 @@ static int mtk_pcie_probe(struct platform_device *pdev) > host->sysdata = pcie; > > err = pci_host_probe(host); > - if (err) > - goto put_resources; > - > - return 0; > + if (!err) > + return 0; > No, leave the error path. err = pci_host_probe(host); if (err) goto put_resources; return 0; put_resources: ... it is easier to follow. Thanks, Lorenzo > -put_resources: > if (!list_empty(&pcie->ports)) > mtk_pcie_put_resources(pcie); > > -- > 2.6.4 >
next prev parent reply other threads:[~2018-11-07 12:03 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-10-18 3:23 [PATCH] PCI: mediatek: Use devm_of_pci_get_host_bridge_resources() to parse DT honghui.zhang 2018-10-18 3:23 ` honghui.zhang at mediatek.com 2018-10-18 3:23 ` honghui.zhang 2018-11-02 6:36 ` [SPAM][PATCH] " Ryder Lee 2018-11-02 6:36 ` Ryder Lee 2018-11-07 3:19 ` [PATCH] " Honghui Zhang 2018-11-07 3:19 ` Honghui Zhang 2018-11-07 12:03 ` Lorenzo Pieralisi [this message] 2018-11-07 12:03 ` Lorenzo Pieralisi 2018-11-08 2:53 ` Honghui Zhang 2018-11-08 2:53 ` Honghui Zhang 2018-11-08 2:53 ` Honghui Zhang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20181107120333.GB11586@e107981-ln.cambridge.arm.com \ --to=lorenzo.pieralisi@arm.com \ --cc=bhelgaas@google.com \ --cc=honghui.zhang@mediatek.com \ --cc=jianjun.wang@mediatek.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mediatek@lists.infradead.org \ --cc=linux-pci@vger.kernel.org \ --cc=ryder.lee@mediatek.com \ --cc=youlin.pei@mediatek.com \ --cc=yt.shen@mediatek.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.