Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1630467 > unrolled thread

Re: [PATCH 1/2] PCI: mediatek: Add Mediatek PCIe host controller support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/2] PCI: mediatek: Add Mediatek PCIe host controller support Arnd Bergmann <arnd@arndb.de> - 2017-04-25 14:40 +0200
    Re: [PATCH 1/2] PCI: mediatek: Add Mediatek PCIe host controller  support Ryder Lee <ryder.lee@mediatek.com> - 2017-04-26 10:20 +0200
      Re: [PATCH 1/2] PCI: mediatek: Add Mediatek PCIe host controller support Arnd Bergmann <arnd@arndb.de> - 2017-04-27 21:00 +0200
        Re: [PATCH 1/2] PCI: mediatek: Add Mediatek PCIe host controller  support Ryder Lee <ryder.lee@mediatek.com> - 2017-04-28 04:50 +0200

#1630467 — Re: [PATCH 1/2] PCI: mediatek: Add Mediatek PCIe host controller support

On Sun, Apr 23, 2017 at 10:19 AM, Ryder Lee <ryder.lee@mediatek.com> wrote:

> +static inline bool mtk_pcie_link_is_up(struct mtk_pcie_port *port)
> +{
> +       return !!(readl_relaxed(port->base + PCIE_LINK_STATUS) &
> +                 PCIE_PORT_LINKUP);
> +}

If this is not performance critical, please use the regular readl() instead
of readl_relaxed().

> +static bool mtk_pcie_valid_device(struct mtk_pcie *pcie,
> +                                 struct pci_bus *bus, int devfn)
> +{
> +       struct mtk_pcie_port *port;
> +       struct pci_dev *dev;
> +       struct pci_bus *pbus;
> +
> +       /* if there is no link, then there is no device */
> +       list_for_each_entry(port, &pcie->ports, list) {
> +               if (bus->number == 0 && port->index == PCI_SLOT(devfn) &&
> +                   mtk_pcie_link_is_up(port)) {
> +                       return true;
> +               } else if (bus->number != 0) {
> +                       pbus = bus;
> +                       do {
> +                               dev = pbus->self;
> +                               if (port->index == PCI_SLOT(dev->devfn) &&
> +                                   mtk_pcie_link_is_up(port)) {
> +                                       return true;
> +                               }
> +                               pbus = dev->bus;
> +                       } while (dev->bus->number != 0);
> +               }
> +       }
> +
> +       return false;
> +}




> +static int mtk_pcie_hw_rd_cfg(struct mtk_pcie *pcie, u32 bus, u32 devfn,
> +                             int where, int size, u32 *val)
> +{
> +       writel(PCIE_CONF_ADDR(where, PCI_FUNC(devfn), PCI_SLOT(devfn), bus),
> +              pcie->base + PCIE_CFG_ADDR);
> +
> +       *val = 0;
> +
> +       switch (size) {
> +       case 1:
> +               *val = readb(pcie->base + PCIE_CFG_DATA + (where & 3));
> +               break;
> +       case 2:
> +               *val = readw(pcie->base + PCIE_CFG_DATA + (where & 2));
> +               break;
> +       case 4:
> +               *val = readl(pcie->base + PCIE_CFG_DATA);
> +               break;
> +       }
> +
> +       return PCIBIOS_SUCCESSFUL;
> +}

This is a fairly standard set of read/write operations. Can you change
the pci_ops
to use pci_generic_config_read/pci_generic_config_write and an appropriate
map function instead?

> +static int mtk_pcie_enable_ports(struct mtk_pcie *pcie)
> +{
> +       struct device *dev = pcie->dev;
> +       struct mtk_pcie_port *port, *tmp;
> +       int err, linkup = 0;
> +
> +       list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
> +               err = clk_prepare_enable(port->sys_ck);
> +               if (err) {
> +                       dev_err(dev, "failed to enable port%d clock\n",
> +                               port->index);
> +                       continue;
> +               }
> +
> +               /* assert RC */
> +               reset_control_assert(port->reset);
> +               /* de-assert RC */
> +               reset_control_deassert(port->reset);
> +
> +               /* power on PHY */
> +               err = phy_power_on(port->phy);
> +               if (err) {
> +                       dev_err(dev, "failed to power on port%d phy\n",
> +                               port->index);
> +                       goto err_phy_on;
> +               }
> +
> +               mtk_pcie_assert_ports(port);
> +

Similar to the comment I had for the binding, I wonder if it would be
better to keep all the information about the ports in one place and
then just deal with it at the root level.

Alternatively, we could decide to standardize on the properties
you have added to the pcie port node, but then I would handle
them in the pcieport driver rather than in the host bridge driver.

> +/*
> + * This IP lacks interrupt status register to check or map INTx from
> + * different devices at the same time.
> + */
> +static int __init mtk_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
> +{
> +       struct mtk_pcie *pcie = dev->bus->sysdata;
> +       struct mtk_pcie_port *port;
> +
> +       list_for_each_entry(port, &pcie->ports, list)
> +               if (port->index == slot)
> +                       return port->irq;
> +
> +       return -1;
> +}

This looks odd, what is it needed for specifically? It looks like
it's broken for devices behind bridges, and the interrupt mapping
should normally come from the interrupt-map property, without
the need for a driver specific map_irq override.

> +static int mtk_pcie_register_ports(struct mtk_pcie *pcie)
> +{
> +       struct pci_bus *bus, *child;
> +
> +       bus = pci_scan_root_bus(pcie->dev, 0, &mtk_pcie_ops, pcie,
> +                               &pcie->resources);

Can you use the new pci_register_host_bridge() method instead of
pci_scan_root_bus() here?

       ARnd

[toc] | [next] | [standalone]

#1631227 — Re: [PATCH 1/2] PCI: mediatek: Add Mediatek PCIe host controller support

Hi,

On Tue, 2017-04-25 at 14:38 +0200, Arnd Bergmann wrote:
> On Sun, Apr 23, 2017 at 10:19 AM, Ryder Lee <ryder.lee@mediatek.com> wrote:
> 
> > +static inline bool mtk_pcie_link_is_up(struct mtk_pcie_port *port)
> > +{
> > +       return !!(readl_relaxed(port->base + PCIE_LINK_STATUS) &
> > +                 PCIE_PORT_LINKUP);
> > +}
> 
> If this is not performance critical, please use the regular readl() instead
> of readl_relaxed().

I will correct it.

> > +static bool mtk_pcie_valid_device(struct mtk_pcie *pcie,
> > +                                 struct pci_bus *bus, int devfn)
> > +{
> > +       struct mtk_pcie_port *port;
> > +       struct pci_dev *dev;
> > +       struct pci_bus *pbus;
> > +
> > +       /* if there is no link, then there is no device */
> > +       list_for_each_entry(port, &pcie->ports, list) {
> > +               if (bus->number == 0 && port->index == PCI_SLOT(devfn) &&
> > +                   mtk_pcie_link_is_up(port)) {
> > +                       return true;
> > +               } else if (bus->number != 0) {
> > +                       pbus = bus;
> > +                       do {
> > +                               dev = pbus->self;
> > +                               if (port->index == PCI_SLOT(dev->devfn) &&
> > +                                   mtk_pcie_link_is_up(port)) {
> > +                                       return true;
> > +                               }
> > +                               pbus = dev->bus;
> > +                       } while (dev->bus->number != 0);
> > +               }
> > +       }
> > +
> > +       return false;
> > +}
> 
> 
> 
> 
> > +static int mtk_pcie_hw_rd_cfg(struct mtk_pcie *pcie, u32 bus, u32 devfn,
> > +                             int where, int size, u32 *val)
> > +{
> > +       writel(PCIE_CONF_ADDR(where, PCI_FUNC(devfn), PCI_SLOT(devfn), bus),
> > +              pcie->base + PCIE_CFG_ADDR);
> > +
> > +       *val = 0;
> > +
> > +       switch (size) {
> > +       case 1:
> > +               *val = readb(pcie->base + PCIE_CFG_DATA + (where & 3));
> > +               break;
> > +       case 2:
> > +               *val = readw(pcie->base + PCIE_CFG_DATA + (where & 2));
> > +               break;
> > +       case 4:
> > +               *val = readl(pcie->base + PCIE_CFG_DATA);
> > +               break;
> > +       }
> > +
> > +       return PCIBIOS_SUCCESSFUL;
> > +}
> 
> This is a fairly standard set of read/write operations. Can you change
> the pci_ops
> to use pci_generic_config_read/pci_generic_config_write and an appropriate
> map function instead?

OK I will add a .map_bus() like this:
{ .

  writel(PCIE_CONF_ADDR(where, fun, slot, bus), base + PCIE_CFG_ADDR);

  return base + PCIE_CFG_DATA + (where & 3);

}
> > +static int mtk_pcie_enable_ports(struct mtk_pcie *pcie)
> > +{
> > +       struct device *dev = pcie->dev;
> > +       struct mtk_pcie_port *port, *tmp;
> > +       int err, linkup = 0;
> > +
> > +       list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
> > +               err = clk_prepare_enable(port->sys_ck);
> > +               if (err) {
> > +                       dev_err(dev, "failed to enable port%d clock\n",
> > +                               port->index);
> > +                       continue;
> > +               }
> > +
> > +               /* assert RC */
> > +               reset_control_assert(port->reset);
> > +               /* de-assert RC */
> > +               reset_control_deassert(port->reset);
> > +
> > +               /* power on PHY */
> > +               err = phy_power_on(port->phy);
> > +               if (err) {
> > +                       dev_err(dev, "failed to power on port%d phy\n",
> > +                               port->index);
> > +                       goto err_phy_on;
> > +               }
> > +
> > +               mtk_pcie_assert_ports(port);
> > +
> 
> Similar to the comment I had for the binding, I wonder if it would be
> better to keep all the information about the ports in one place and
> then just deal with it at the root level.
> 
> Alternatively, we could decide to standardize on the properties
> you have added to the pcie port node, but then I would handle
> them in the pcieport driver rather than in the host bridge driver.

Sorry, I'm not sure what you want me to do here.

I could move all clock operation in root level. But we need to keep the
reset and PHY operation sequence in the loop, In addition, we could
easily free resources if ports link fail.

How about moving this function to mtk_pcie_parse_and_add_res()? 


> > +/*
> > + * This IP lacks interrupt status register to check or map INTx from
> > + * different devices at the same time.
> > + */
> > +static int __init mtk_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
> > +{
> > +       struct mtk_pcie *pcie = dev->bus->sysdata;
> > +       struct mtk_pcie_port *port;
> > +
> > +       list_for_each_entry(port, &pcie->ports, list)
> > +               if (port->index == slot)
> > +                       return port->irq;
> > +
> > +       return -1;
> > +}
> 
> This looks odd, what is it needed for specifically? It looks like
> it's broken for devices behind bridges, and the interrupt mapping
> should normally come from the interrupt-map property, without
> the need for a driver specific map_irq override.

Our hardware just has a GIC for each port and lacks interrupt status for
host driver to distinguish INTx. So I return port IRQ here.


> > +static int mtk_pcie_register_ports(struct mtk_pcie *pcie)
> > +{
> > +       struct pci_bus *bus, *child;
> > +
> > +       bus = pci_scan_root_bus(pcie->dev, 0, &mtk_pcie_ops, pcie,
> > +                               &pcie->resources);
> 
> Can you use the new pci_register_host_bridge() method instead of
> pci_scan_root_bus() here?

May I know what's difference between pci_scan_root_bus() and using
pci_register_host_bridge() directly? What situation should we use it?
It seems that just tegra use this new method currently.

I'm not sure whether I can still use pci_scan_root_bus() here? 

>        ARnd


Thanks for the review!

[toc] | [prev] | [next] | [standalone]

#1632335

On Wed, Apr 26, 2017 at 10:10 AM, Ryder Lee <ryder.lee@mediatek.com> wrote:
> On Tue, 2017-04-25 at 14:38 +0200, Arnd Bergmann wrote:
>> On Sun, Apr 23, 2017 at 10:19 AM, Ryder Lee <ryder.lee@mediatek.com> wrote:

>> > +static int mtk_pcie_enable_ports(struct mtk_pcie *pcie)
>> > +{
>> > +       struct device *dev = pcie->dev;
>> > +       struct mtk_pcie_port *port, *tmp;
>> > +       int err, linkup = 0;
>> > +
>> > +       list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
>> > +               err = clk_prepare_enable(port->sys_ck);
>> > +               if (err) {
>> > +                       dev_err(dev, "failed to enable port%d clock\n",
>> > +                               port->index);
>> > +                       continue;
>> > +               }
>> > +
>> > +               /* assert RC */
>> > +               reset_control_assert(port->reset);
>> > +               /* de-assert RC */
>> > +               reset_control_deassert(port->reset);
>> > +
>> > +               /* power on PHY */
>> > +               err = phy_power_on(port->phy);
>> > +               if (err) {
>> > +                       dev_err(dev, "failed to power on port%d phy\n",
>> > +                               port->index);
>> > +                       goto err_phy_on;
>> > +               }
>> > +
>> > +               mtk_pcie_assert_ports(port);
>> > +
>>
>> Similar to the comment I had for the binding, I wonder if it would be
>> better to keep all the information about the ports in one place and
>> then just deal with it at the root level.
>>
>> Alternatively, we could decide to standardize on the properties
>> you have added to the pcie port node, but then I would handle
>> them in the pcieport driver rather than in the host bridge driver.
>
> Sorry, I'm not sure what you want me to do here.
>
> I could move all clock operation in root level. But we need to keep the
> reset and PHY operation sequence in the loop, In addition, we could
> easily free resources if ports link fail.
>
> How about moving this function to mtk_pcie_parse_and_add_res()?

That could work, please try it out and see if the code gets better or
worse. This may depend on what we end up doing with the DT
properties.

>> > +/*
>> > + * This IP lacks interrupt status register to check or map INTx from
>> > + * different devices at the same time.
>> > + */
>> > +static int __init mtk_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
>> > +{
>> > +       struct mtk_pcie *pcie = dev->bus->sysdata;
>> > +       struct mtk_pcie_port *port;
>> > +
>> > +       list_for_each_entry(port, &pcie->ports, list)
>> > +               if (port->index == slot)
>> > +                       return port->irq;
>> > +
>> > +       return -1;
>> > +}
>>
>> This looks odd, what is it needed for specifically? It looks like
>> it's broken for devices behind bridges, and the interrupt mapping
>> should normally come from the interrupt-map property, without
>> the need for a driver specific map_irq override.
>
> Our hardware just has a GIC for each port and lacks interrupt status for
> host driver to distinguish INTx. So I return port IRQ here.

You should still be able to express this with standard interrupt-map
DT property, without having to resort to your own map_irq
callback handler.

In the interrupt-map-mask, you can ignore the interrupt line
only list the devfn of the root ports for each entry.

>> > +static int mtk_pcie_register_ports(struct mtk_pcie *pcie)
>> > +{
>> > +       struct pci_bus *bus, *child;
>> > +
>> > +       bus = pci_scan_root_bus(pcie->dev, 0, &mtk_pcie_ops, pcie,
>> > +                               &pcie->resources);
>>
>> Can you use the new pci_register_host_bridge() method instead of
>> pci_scan_root_bus() here?
>
> May I know what's difference between pci_scan_root_bus() and using
> pci_register_host_bridge() directly? What situation should we use it?
> It seems that just tegra use this new method currently.

We introduced the new function for tegra for now, in the long run
I would hope we can convert all other drivers to it as well, to make it
easier to add further parameters.

The new function also has a cleaner way of dealing with the memory
allocations, similar to how other subsystems work.

     Arnd

[toc] | [prev] | [next] | [standalone]

#1632504 — Re: [PATCH 1/2] PCI: mediatek: Add Mediatek PCIe host controller support

Hi,

On Thu, 2017-04-27 at 20:55 +0200, Arnd Bergmann wrote:
> On Wed, Apr 26, 2017 at 10:10 AM, Ryder Lee <ryder.lee@mediatek.com> wrote:
> > On Tue, 2017-04-25 at 14:38 +0200, Arnd Bergmann wrote:
> >> On Sun, Apr 23, 2017 at 10:19 AM, Ryder Lee <ryder.lee@mediatek.com> wrote:
> 
> >> > +static int mtk_pcie_enable_ports(struct mtk_pcie *pcie)
> >> > +{
> >> > +       struct device *dev = pcie->dev;
> >> > +       struct mtk_pcie_port *port, *tmp;
> >> > +       int err, linkup = 0;
> >> > +
> >> > +       list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
> >> > +               err = clk_prepare_enable(port->sys_ck);
> >> > +               if (err) {
> >> > +                       dev_err(dev, "failed to enable port%d clock\n",
> >> > +                               port->index);
> >> > +                       continue;
> >> > +               }
> >> > +
> >> > +               /* assert RC */
> >> > +               reset_control_assert(port->reset);
> >> > +               /* de-assert RC */
> >> > +               reset_control_deassert(port->reset);
> >> > +
> >> > +               /* power on PHY */
> >> > +               err = phy_power_on(port->phy);
> >> > +               if (err) {
> >> > +                       dev_err(dev, "failed to power on port%d phy\n",
> >> > +                               port->index);
> >> > +                       goto err_phy_on;
> >> > +               }
> >> > +
> >> > +               mtk_pcie_assert_ports(port);
> >> > +
> >>
> >> Similar to the comment I had for the binding, I wonder if it would be
> >> better to keep all the information about the ports in one place and
> >> then just deal with it at the root level.
> >>
> >> Alternatively, we could decide to standardize on the properties
> >> you have added to the pcie port node, but then I would handle
> >> them in the pcieport driver rather than in the host bridge driver.
> >
> > Sorry, I'm not sure what you want me to do here.
> >
> > I could move all clock operation in root level. But we need to keep the
> > reset and PHY operation sequence in the loop, In addition, we could
> > easily free resources if ports link fail.
> >
> > How about moving this function to mtk_pcie_parse_and_add_res()?
> 
> That could work, please try it out and see if the code gets better or
> worse. This may depend on what we end up doing with the DT
> properties.

I will try it on next version, and we can continue our discussion on
that series.

> >> > +/*
> >> > + * This IP lacks interrupt status register to check or map INTx from
> >> > + * different devices at the same time.
> >> > + */
> >> > +static int __init mtk_pcie_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
> >> > +{
> >> > +       struct mtk_pcie *pcie = dev->bus->sysdata;
> >> > +       struct mtk_pcie_port *port;
> >> > +
> >> > +       list_for_each_entry(port, &pcie->ports, list)
> >> > +               if (port->index == slot)
> >> > +                       return port->irq;
> >> > +
> >> > +       return -1;
> >> > +}
> >>
> >> This looks odd, what is it needed for specifically? It looks like
> >> it's broken for devices behind bridges, and the interrupt mapping
> >> should normally come from the interrupt-map property, without
> >> the need for a driver specific map_irq override.
> >
> > Our hardware just has a GIC for each port and lacks interrupt status for
> > host driver to distinguish INTx. So I return port IRQ here.
> 
> You should still be able to express this with standard interrupt-map
> DT property, without having to resort to your own map_irq
> callback handler.
> 
> In the interrupt-map-mask, you can ignore the interrupt line
> only list the devfn of the root ports for each entry.

Okay, I will fix it.

> >> > +static int mtk_pcie_register_ports(struct mtk_pcie *pcie)
> >> > +{
> >> > +       struct pci_bus *bus, *child;
> >> > +
> >> > +       bus = pci_scan_root_bus(pcie->dev, 0, &mtk_pcie_ops, pcie,
> >> > +                               &pcie->resources);
> >>
> >> Can you use the new pci_register_host_bridge() method instead of
> >> pci_scan_root_bus() here?
> >
> > May I know what's difference between pci_scan_root_bus() and using
> > pci_register_host_bridge() directly? What situation should we use it?
> > It seems that just tegra use this new method currently.
> 
> We introduced the new function for tegra for now, in the long run
> I would hope we can convert all other drivers to it as well, to make it
> easier to add further parameters.
> 
> The new function also has a cleaner way of dealing with the memory
> allocations, similar to how other subsystems work.

Sounds good. I will change to use that.

Thanks!

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web