[PATCH v2] PCI: altera: Retrain link in rootport mode only

[PATCH v2] PCI: altera: Retrain link in rootport mode only

[Ley Foon Tan]

Altera PCIe IP can be configured as rootport or device and they might have
same vendor ID. It will cause the system hang issue if Altera PCIe is in
endpoint mode and work with other PCIe rootport that from other vendors.
So, add the rootport mode checking in link retrain fixup function.

Signed-off-by: Ley Foon Tan <lftan@altera.com>
---
v2: change to check PCIe type is PCI_EXP_TYPE_ROOT_PORT
---
 drivers/pci/host/pcie-altera.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pci/host/pcie-altera.c b/drivers/pci/host/pcie-altera.c
index 58eef99..33b6968 100644
--- a/drivers/pci/host/pcie-altera.c
+++ b/drivers/pci/host/pcie-altera.c
@@ -139,6 +139,9 @@ static void altera_pcie_retrain(struct pci_dev *dev)
 	u16 linkcap, linkstat;
 	struct altera_pcie *pcie = dev->bus->sysdata;
 
+	if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
+		return;
+
 	if (!altera_pcie_link_is_up(pcie))
 		return;
 
-- 
1.8.2.1

Re: [PATCH v2] PCI: altera: Retrain link in rootport mode only

[Bjorn Helgaas]

On Fri, Aug 19, 2016 at 04:24:38PM +0800, Ley Foon Tan wrote:
> Altera PCIe IP can be configured as rootport or device and they might have
> same vendor ID. It will cause the system hang issue if Altera PCIe is in
> endpoint mode and work with other PCIe rootport that from other vendors.
> So, add the rootport mode checking in link retrain fixup function.
>
> Signed-off-by: Ley Foon Tan <lftan@altera.com>
> ---
> v2: change to check PCIe type is PCI_EXP_TYPE_ROOT_PORT
> ---
>  drivers/pci/host/pcie-altera.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/pci/host/pcie-altera.c b/drivers/pci/host/pcie-altera.c
> index 58eef99..33b6968 100644
> --- a/drivers/pci/host/pcie-altera.c
> +++ b/drivers/pci/host/pcie-altera.c
> @@ -139,6 +139,9 @@ static void altera_pcie_retrain(struct pci_dev *dev)
>  	u16 linkcap, linkstat;
>  	struct altera_pcie *pcie = dev->bus->sysdata;
>  
> +	if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
> +		return;
> +
>  	if (!altera_pcie_link_is_up(pcie))
>  		return;

Instead of making this a PCI fixup, can you make an
altera_pcie_host_init() function, call it from altera_pcie_probe(),
and do the link retrain there?  Then you wouldn't need to worry about
whether this is a Root Port or an Endpoint, plus it would make the
altera driver structure more like the other drivers.

You would call altera_pcie_host_init() before pci_scan_root_bus(), so
you wouldn't have a pci_dev yet, so you wouldn't be able to use
pcie_capability_set_word() to set the PCI_EXP_LNKCTL_RL bit.  But I
assume there's some device-dependent way to access it using
cra_writel()?

Bjorn

Re: [PATCH v2] PCI: altera: Retrain link in rootport mode only

[Ley Foon Tan]

On Mon, Aug 22, 2016 at 11:47 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Fri, Aug 19, 2016 at 04:24:38PM +0800, Ley Foon Tan wrote:
>> Altera PCIe IP can be configured as rootport or device and they might have
>> same vendor ID. It will cause the system hang issue if Altera PCIe is in
>> endpoint mode and work with other PCIe rootport that from other vendors.
>> So, add the rootport mode checking in link retrain fixup function.
>>
>> Signed-off-by: Ley Foon Tan <lftan@altera.com>
>> ---
>> v2: change to check PCIe type is PCI_EXP_TYPE_ROOT_PORT
>> ---
>>  drivers/pci/host/pcie-altera.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/pci/host/pcie-altera.c b/drivers/pci/host/pcie-altera.c
>> index 58eef99..33b6968 100644
>> --- a/drivers/pci/host/pcie-altera.c
>> +++ b/drivers/pci/host/pcie-altera.c
>> @@ -139,6 +139,9 @@ static void altera_pcie_retrain(struct pci_dev *dev)
>>       u16 linkcap, linkstat;
>>       struct altera_pcie *pcie = dev->bus->sysdata;
>>
>> +     if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
>> +             return;
>> +
>>       if (!altera_pcie_link_is_up(pcie))
>>               return;
>
> Instead of making this a PCI fixup, can you make an
> altera_pcie_host_init() function, call it from altera_pcie_probe(),
> and do the link retrain there?  Then you wouldn't need to worry about
> whether this is a Root Port or an Endpoint, plus it would make the
> altera driver structure more like the other drivers.
>
> You would call altera_pcie_host_init() before pci_scan_root_bus(), so
> you wouldn't have a pci_dev yet, so you wouldn't be able to use
> pcie_capability_set_word() to set the PCI_EXP_LNKCTL_RL bit.  But I
> assume there's some device-dependent way to access it using
> cra_writel()?
We can't use cra_write() to set PCI_EXP_LNKCTL_RL bit. We can use
pci_bus_find_capability() and pci_bus_read_config_word() with struct
pci_bus instead.
But this only can be called after pci_scan_root_bus(). Found
iproc_pcie_check_link() have similar implementation.
Tested this method is working. Do you think it is okay? If yes, then I
will send in next revision.

Thanks.

Regards
Ley Foon

Re: [PATCH v2] PCI: altera: Retrain link in rootport mode only

[Bjorn Helgaas]

[+cc Ray, Scott, Jon, bcm-kernel-feedback-list]

On Wed, Aug 24, 2016 at 03:07:52PM +0800, Ley Foon Tan wrote:
> On Mon, Aug 22, 2016 at 11:47 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Fri, Aug 19, 2016 at 04:24:38PM +0800, Ley Foon Tan wrote:
> >> Altera PCIe IP can be configured as rootport or device and they might have
> >> same vendor ID. It will cause the system hang issue if Altera PCIe is in
> >> endpoint mode and work with other PCIe rootport that from other vendors.
> >> So, add the rootport mode checking in link retrain fixup function.
> >>
> >> Signed-off-by: Ley Foon Tan <lftan@altera.com>
> >> ---
> >> v2: change to check PCIe type is PCI_EXP_TYPE_ROOT_PORT
> >> ---
> >>  drivers/pci/host/pcie-altera.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/pci/host/pcie-altera.c b/drivers/pci/host/pcie-altera.c
> >> index 58eef99..33b6968 100644
> >> --- a/drivers/pci/host/pcie-altera.c
> >> +++ b/drivers/pci/host/pcie-altera.c
> >> @@ -139,6 +139,9 @@ static void altera_pcie_retrain(struct pci_dev *dev)
> >>       u16 linkcap, linkstat;
> >>       struct altera_pcie *pcie = dev->bus->sysdata;
> >>
> >> +     if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
> >> +             return;
> >> +
> >>       if (!altera_pcie_link_is_up(pcie))
> >>               return;
> >
> > Instead of making this a PCI fixup, can you make an
> > altera_pcie_host_init() function, call it from altera_pcie_probe(),
> > and do the link retrain there?  Then you wouldn't need to worry about
> > whether this is a Root Port or an Endpoint, plus it would make the
> > altera driver structure more like the other drivers.
> >
> > You would call altera_pcie_host_init() before pci_scan_root_bus(), so
> > you wouldn't have a pci_dev yet, so you wouldn't be able to use
> > pcie_capability_set_word() to set the PCI_EXP_LNKCTL_RL bit.  But I
> > assume there's some device-dependent way to access it using
> > cra_writel()?
> We can't use cra_write() to set PCI_EXP_LNKCTL_RL bit. 

Why not?  I don't mean it has to be cra_write(), but isn't there some
way you can write that bit before we scan the root bus?  It doesn't
make sense that we have to scan the bus before we can train the link.

We want to be able to tell the PCI core "all the device-specific root
complex initialization has been done, here are the config accessors
you need, please scan for devices."  I want to keep device-specific
things like this quirk directly in the driver and out of the
enumeration process.

> We can use
> pci_bus_find_capability() and pci_bus_read_config_word() with struct
> pci_bus instead.
> But this only can be called after pci_scan_root_bus().

> Found
> iproc_pcie_check_link() have similar implementation.

You're right, and I don't like iproc_pcie_check_link() either, for the
same reasons.

The iproc_pcie_check_link() is a little better because it's called
before enumeration:

  pci_create_root_bus()
  iproc_pcie_check_link()
  pci_scan_child_bus()

But it would be a lot better if iproc_pcie_check_link() were done
first, before pci_create_root_bus().  Then it would be more like the
structure of other drivers, and we could use pci_scan_root_bus()
instead.

Comments, iproc folks?

Bjorn

Re: [PATCH v2] PCI: altera: Retrain link in rootport mode only

[Scott Branden]

Hi Bjorn,

Ray is off this week and will likely have some comments next week.

On 16-08-24 10:54 AM, Bjorn Helgaas wrote:
> [+cc Ray, Scott, Jon, bcm-kernel-feedback-list]
>
> On Wed, Aug 24, 2016 at 03:07:52PM +0800, Ley Foon Tan wrote:
>> On Mon, Aug 22, 2016 at 11:47 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>> On Fri, Aug 19, 2016 at 04:24:38PM +0800, Ley Foon Tan wrote:
>>>> Altera PCIe IP can be configured as rootport or device and they might have
>>>> same vendor ID. It will cause the system hang issue if Altera PCIe is in
>>>> endpoint mode and work with other PCIe rootport that from other vendors.
>>>> So, add the rootport mode checking in link retrain fixup function.
>>>>
>>>> Signed-off-by: Ley Foon Tan <lftan@altera.com>
>>>> ---
>>>> v2: change to check PCIe type is PCI_EXP_TYPE_ROOT_PORT
>>>> ---
>>>>  drivers/pci/host/pcie-altera.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/host/pcie-altera.c b/drivers/pci/host/pcie-altera.c
>>>> index 58eef99..33b6968 100644
>>>> --- a/drivers/pci/host/pcie-altera.c
>>>> +++ b/drivers/pci/host/pcie-altera.c
>>>> @@ -139,6 +139,9 @@ static void altera_pcie_retrain(struct pci_dev *dev)
>>>>       u16 linkcap, linkstat;
>>>>       struct altera_pcie *pcie = dev->bus->sysdata;
>>>>
>>>> +     if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
>>>> +             return;
>>>> +
>>>>       if (!altera_pcie_link_is_up(pcie))
>>>>               return;
>>>
>>> Instead of making this a PCI fixup, can you make an
>>> altera_pcie_host_init() function, call it from altera_pcie_probe(),
>>> and do the link retrain there?  Then you wouldn't need to worry about
>>> whether this is a Root Port or an Endpoint, plus it would make the
>>> altera driver structure more like the other drivers.
>>>
>>> You would call altera_pcie_host_init() before pci_scan_root_bus(), so
>>> you wouldn't have a pci_dev yet, so you wouldn't be able to use
>>> pcie_capability_set_word() to set the PCI_EXP_LNKCTL_RL bit.  But I
>>> assume there's some device-dependent way to access it using
>>> cra_writel()?
>> We can't use cra_write() to set PCI_EXP_LNKCTL_RL bit.
>
> Why not?  I don't mean it has to be cra_write(), but isn't there some
> way you can write that bit before we scan the root bus?  It doesn't
> make sense that we have to scan the bus before we can train the link.
>
> We want to be able to tell the PCI core "all the device-specific root
> complex initialization has been done, here are the config accessors
> you need, please scan for devices."  I want to keep device-specific
> things like this quirk directly in the driver and out of the
> enumeration process.
>
>> We can use
>> pci_bus_find_capability() and pci_bus_read_config_word() with struct
>> pci_bus instead.
>> But this only can be called after pci_scan_root_bus().
>
>> Found
>> iproc_pcie_check_link() have similar implementation.
>
> You're right, and I don't like iproc_pcie_check_link() either, for the
> same reasons.
>
> The iproc_pcie_check_link() is a little better because it's called
> before enumeration:
>
>   pci_create_root_bus()
>   iproc_pcie_check_link()
>   pci_scan_child_bus()
>
> But it would be a lot better if iproc_pcie_check_link() were done
> first, before pci_create_root_bus().  Then it would be more like the
> structure of other drivers, and we could use pci_scan_root_bus()
> instead.
>
> Comments, iproc folks?
>
> Bjorn
>

Regards,
  Scott

Re: [PATCH v2] PCI: altera: Retrain link in rootport mode only

[Ley Foon Tan]

On Thu, Aug 25, 2016 at 1:54 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> [+cc Ray, Scott, Jon, bcm-kernel-feedback-list]
>
> On Wed, Aug 24, 2016 at 03:07:52PM +0800, Ley Foon Tan wrote:
>> On Mon, Aug 22, 2016 at 11:47 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> > On Fri, Aug 19, 2016 at 04:24:38PM +0800, Ley Foon Tan wrote:
>> >> Altera PCIe IP can be configured as rootport or device and they might have
>> >> same vendor ID. It will cause the system hang issue if Altera PCIe is in
>> >> endpoint mode and work with other PCIe rootport that from other vendors.
>> >> So, add the rootport mode checking in link retrain fixup function.
>> >>
>> >> Signed-off-by: Ley Foon Tan <lftan@altera.com>
>> >> ---
>> >> v2: change to check PCIe type is PCI_EXP_TYPE_ROOT_PORT
>> >> ---
>> >>  drivers/pci/host/pcie-altera.c | 3 +++
>> >>  1 file changed, 3 insertions(+)
>> >>
>> >> diff --git a/drivers/pci/host/pcie-altera.c b/drivers/pci/host/pcie-altera.c
>> >> index 58eef99..33b6968 100644
>> >> --- a/drivers/pci/host/pcie-altera.c
>> >> +++ b/drivers/pci/host/pcie-altera.c
>> >> @@ -139,6 +139,9 @@ static void altera_pcie_retrain(struct pci_dev *dev)
>> >>       u16 linkcap, linkstat;
>> >>       struct altera_pcie *pcie = dev->bus->sysdata;
>> >>
>> >> +     if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
>> >> +             return;
>> >> +
>> >>       if (!altera_pcie_link_is_up(pcie))
>> >>               return;
>> >
>> > Instead of making this a PCI fixup, can you make an
>> > altera_pcie_host_init() function, call it from altera_pcie_probe(),
>> > and do the link retrain there?  Then you wouldn't need to worry about
>> > whether this is a Root Port or an Endpoint, plus it would make the
>> > altera driver structure more like the other drivers.
>> >
>> > You would call altera_pcie_host_init() before pci_scan_root_bus(), so
>> > you wouldn't have a pci_dev yet, so you wouldn't be able to use
>> > pcie_capability_set_word() to set the PCI_EXP_LNKCTL_RL bit.  But I
>> > assume there's some device-dependent way to access it using
>> > cra_writel()?
>> We can't use cra_write() to set PCI_EXP_LNKCTL_RL bit.
>
> Why not?  I don't mean it has to be cra_write(), but isn't there some
> way you can write that bit before we scan the root bus?  It doesn't
> make sense that we have to scan the bus before we can train the link.
>
> We want to be able to tell the PCI core "all the device-specific root
> complex initialization has been done, here are the config accessors
> you need, please scan for devices."  I want to keep device-specific
> things like this quirk directly in the driver and out of the
> enumeration process.
We don't have internal register bit to trigger link retrain, but need to set
PCI_EXP_LNKCTL_RL bit in Link Control register of PCIe Capabilities Structure.
So, this requires the altera_pcie_cfg_read() and altera_pcie_cfg_write().
I can restructure the altera_pcie_cfg_read() and
altera_pcie_cfg_write() and have
new _altera_pcie_cfg_read() and _altera_pcie_cfg_write() that avoid
the dependency of struct pci_bus. By doing this, we can retrain the link before
pci_scan_root_bus and remove _FIXUP()

Will send new v3 patch, please take a look.

>
>> We can use
>> pci_bus_find_capability() and pci_bus_read_config_word() with struct
>> pci_bus instead.
>> But this only can be called after pci_scan_root_bus().
>
>> Found
>> iproc_pcie_check_link() have similar implementation.
>
> You're right, and I don't like iproc_pcie_check_link() either, for the
> same reasons.
>
> The iproc_pcie_check_link() is a little better because it's called
> before enumeration:
>
>   pci_create_root_bus()
>   iproc_pcie_check_link()
>   pci_scan_child_bus()
>
> But it would be a lot better if iproc_pcie_check_link() were done
> first, before pci_create_root_bus().  Then it would be more like the
> structure of other drivers, and we could use pci_scan_root_bus()
> instead.
>
> Comments, iproc folks?
>
> Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] PCI: altera: Retrain link in rootport mode only

[Ray Jui]

Hi Bjorn,

On 8/24/2016 10:54 AM, Bjorn Helgaas wrote:
> [+cc Ray, Scott, Jon, bcm-kernel-feedback-list]
>
> On Wed, Aug 24, 2016 at 03:07:52PM +0800, Ley Foon Tan wrote:
>> On Mon, Aug 22, 2016 at 11:47 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>> On Fri, Aug 19, 2016 at 04:24:38PM +0800, Ley Foon Tan wrote:
>>>> Altera PCIe IP can be configured as rootport or device and they might have
>>>> same vendor ID. It will cause the system hang issue if Altera PCIe is in
>>>> endpoint mode and work with other PCIe rootport that from other vendors.
>>>> So, add the rootport mode checking in link retrain fixup function.
>>>>
>>>> Signed-off-by: Ley Foon Tan <lftan@altera.com>
>>>> ---
>>>> v2: change to check PCIe type is PCI_EXP_TYPE_ROOT_PORT
>>>> ---
>>>>  drivers/pci/host/pcie-altera.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/host/pcie-altera.c b/drivers/pci/host/pcie-altera.c
>>>> index 58eef99..33b6968 100644
>>>> --- a/drivers/pci/host/pcie-altera.c
>>>> +++ b/drivers/pci/host/pcie-altera.c
>>>> @@ -139,6 +139,9 @@ static void altera_pcie_retrain(struct pci_dev *dev)
>>>>       u16 linkcap, linkstat;
>>>>       struct altera_pcie *pcie = dev->bus->sysdata;
>>>>
>>>> +     if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
>>>> +             return;
>>>> +
>>>>       if (!altera_pcie_link_is_up(pcie))
>>>>               return;
>>>
>>> Instead of making this a PCI fixup, can you make an
>>> altera_pcie_host_init() function, call it from altera_pcie_probe(),
>>> and do the link retrain there?  Then you wouldn't need to worry about
>>> whether this is a Root Port or an Endpoint, plus it would make the
>>> altera driver structure more like the other drivers.
>>>
>>> You would call altera_pcie_host_init() before pci_scan_root_bus(), so
>>> you wouldn't have a pci_dev yet, so you wouldn't be able to use
>>> pcie_capability_set_word() to set the PCI_EXP_LNKCTL_RL bit.  But I
>>> assume there's some device-dependent way to access it using
>>> cra_writel()?
>> We can't use cra_write() to set PCI_EXP_LNKCTL_RL bit.
>
> Why not?  I don't mean it has to be cra_write(), but isn't there some
> way you can write that bit before we scan the root bus?  It doesn't
> make sense that we have to scan the bus before we can train the link.
>
> We want to be able to tell the PCI core "all the device-specific root
> complex initialization has been done, here are the config accessors
> you need, please scan for devices."  I want to keep device-specific
> things like this quirk directly in the driver and out of the
> enumeration process.
>
>> We can use
>> pci_bus_find_capability() and pci_bus_read_config_word() with struct
>> pci_bus instead.
>> But this only can be called after pci_scan_root_bus().
>
>> Found
>> iproc_pcie_check_link() have similar implementation.
>
> You're right, and I don't like iproc_pcie_check_link() either, for the
> same reasons.
>
> The iproc_pcie_check_link() is a little better because it's called
> before enumeration:
>
>   pci_create_root_bus()
>   iproc_pcie_check_link()
>   pci_scan_child_bus()
>
> But it would be a lot better if iproc_pcie_check_link() were done
> first, before pci_create_root_bus().  Then it would be more like the
> structure of other drivers, and we could use pci_scan_root_bus()
> instead.
>
> Comments, iproc folks?
>
> Bjorn
>

Although not yet tested, I suppose we can do iproc_pcie_check_link 
before calling pci_scan_root_bus so we can get rid of separate calls to 
pci_create_root_bus and pci_scan_child_bus. But then we need to create 
some dummy bus in the iproc_pcie_check_link function to allow access to 
the root bus for link check, which was the primary reason why we did 
pci_create_root_bus before iproc_pcie_check_link, i.e., to avoid the use 
of dummy root bus.

Thanks,

Ray

Re: [PATCH v2] PCI: altera: Retrain link in rootport mode only

[Bjorn Helgaas]

On Mon, Aug 29, 2016 at 05:37:09PM -0700, Ray Jui wrote:
> Hi Bjorn,
> 
> On 8/24/2016 10:54 AM, Bjorn Helgaas wrote:
> >[+cc Ray, Scott, Jon, bcm-kernel-feedback-list]
> >
> >On Wed, Aug 24, 2016 at 03:07:52PM +0800, Ley Foon Tan wrote:
> >>On Mon, Aug 22, 2016 at 11:47 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >>>On Fri, Aug 19, 2016 at 04:24:38PM +0800, Ley Foon Tan wrote:
> >>>>Altera PCIe IP can be configured as rootport or device and they might have
> >>>>same vendor ID. It will cause the system hang issue if Altera PCIe is in
> >>>>endpoint mode and work with other PCIe rootport that from other vendors.
> >>>>So, add the rootport mode checking in link retrain fixup function.
> >>>>
> >>>>Signed-off-by: Ley Foon Tan <lftan@altera.com>
> >>>>---
> >>>>v2: change to check PCIe type is PCI_EXP_TYPE_ROOT_PORT
> >>>>---
> >>>> drivers/pci/host/pcie-altera.c | 3 +++
> >>>> 1 file changed, 3 insertions(+)
> >>>>
> >>>>diff --git a/drivers/pci/host/pcie-altera.c b/drivers/pci/host/pcie-altera.c
> >>>>index 58eef99..33b6968 100644
> >>>>--- a/drivers/pci/host/pcie-altera.c
> >>>>+++ b/drivers/pci/host/pcie-altera.c
> >>>>@@ -139,6 +139,9 @@ static void altera_pcie_retrain(struct pci_dev *dev)
> >>>>      u16 linkcap, linkstat;
> >>>>      struct altera_pcie *pcie = dev->bus->sysdata;
> >>>>
> >>>>+     if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
> >>>>+             return;
> >>>>+
> >>>>      if (!altera_pcie_link_is_up(pcie))
> >>>>              return;
> >>>
> >>>Instead of making this a PCI fixup, can you make an
> >>>altera_pcie_host_init() function, call it from altera_pcie_probe(),
> >>>and do the link retrain there?  Then you wouldn't need to worry about
> >>>whether this is a Root Port or an Endpoint, plus it would make the
> >>>altera driver structure more like the other drivers.
> >>>
> >>>You would call altera_pcie_host_init() before pci_scan_root_bus(), so
> >>>you wouldn't have a pci_dev yet, so you wouldn't be able to use
> >>>pcie_capability_set_word() to set the PCI_EXP_LNKCTL_RL bit.  But I
> >>>assume there's some device-dependent way to access it using
> >>>cra_writel()?
> >>We can't use cra_write() to set PCI_EXP_LNKCTL_RL bit.
> >
> >Why not?  I don't mean it has to be cra_write(), but isn't there some
> >way you can write that bit before we scan the root bus?  It doesn't
> >make sense that we have to scan the bus before we can train the link.
> >
> >We want to be able to tell the PCI core "all the device-specific root
> >complex initialization has been done, here are the config accessors
> >you need, please scan for devices."  I want to keep device-specific
> >things like this quirk directly in the driver and out of the
> >enumeration process.
> >
> >>We can use
> >>pci_bus_find_capability() and pci_bus_read_config_word() with struct
> >>pci_bus instead.
> >>But this only can be called after pci_scan_root_bus().
> >
> >>Found
> >>iproc_pcie_check_link() have similar implementation.
> >
> >You're right, and I don't like iproc_pcie_check_link() either, for the
> >same reasons.
> >
> >The iproc_pcie_check_link() is a little better because it's called
> >before enumeration:
> >
> >  pci_create_root_bus()
> >  iproc_pcie_check_link()
> >  pci_scan_child_bus()
> >
> >But it would be a lot better if iproc_pcie_check_link() were done
> >first, before pci_create_root_bus().  Then it would be more like the
> >structure of other drivers, and we could use pci_scan_root_bus()
> >instead.
> 
> Although not yet tested, I suppose we can do iproc_pcie_check_link
> before calling pci_scan_root_bus so we can get rid of separate calls
> to pci_create_root_bus and pci_scan_child_bus. But then we need to
> create some dummy bus in the iproc_pcie_check_link function to allow
> access to the root bus for link check, which was the primary reason
> why we did pci_create_root_bus before iproc_pcie_check_link, i.e.,
> to avoid the use of dummy root bus.

I don't want a dummy root bus.

There should be some way to structure that code so you can write the
class code and the link status stuff without having a struct pci_bus.
The only reason you need the struct pci_bus in the first place is so
you can extract the struct iproc_pcie *, and you already have that in
iproc_pcie_check_link().

No, you won't be able to use pci_bus_find_capability(), but presumably
you already *know* where the capability is, since you know exactly
what device this is.

Bjorn

Re: [PATCH v2] PCI: altera: Retrain link in rootport mode only

[Ray Jui]

On 8/30/2016 6:37 AM, Bjorn Helgaas wrote:
> On Mon, Aug 29, 2016 at 05:37:09PM -0700, Ray Jui wrote:
>> Hi Bjorn,
>>
>> On 8/24/2016 10:54 AM, Bjorn Helgaas wrote:
>>> [+cc Ray, Scott, Jon, bcm-kernel-feedback-list]
>>>
>>> On Wed, Aug 24, 2016 at 03:07:52PM +0800, Ley Foon Tan wrote:
>>>> On Mon, Aug 22, 2016 at 11:47 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>>>> On Fri, Aug 19, 2016 at 04:24:38PM +0800, Ley Foon Tan wrote:
>>>>>> Altera PCIe IP can be configured as rootport or device and they might have
>>>>>> same vendor ID. It will cause the system hang issue if Altera PCIe is in
>>>>>> endpoint mode and work with other PCIe rootport that from other vendors.
>>>>>> So, add the rootport mode checking in link retrain fixup function.
>>>>>>
>>>>>> Signed-off-by: Ley Foon Tan <lftan@altera.com>
>>>>>> ---
>>>>>> v2: change to check PCIe type is PCI_EXP_TYPE_ROOT_PORT
>>>>>> ---
>>>>>> drivers/pci/host/pcie-altera.c | 3 +++
>>>>>> 1 file changed, 3 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/pci/host/pcie-altera.c b/drivers/pci/host/pcie-altera.c
>>>>>> index 58eef99..33b6968 100644
>>>>>> --- a/drivers/pci/host/pcie-altera.c
>>>>>> +++ b/drivers/pci/host/pcie-altera.c
>>>>>> @@ -139,6 +139,9 @@ static void altera_pcie_retrain(struct pci_dev *dev)
>>>>>>      u16 linkcap, linkstat;
>>>>>>      struct altera_pcie *pcie = dev->bus->sysdata;
>>>>>>
>>>>>> +     if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
>>>>>> +             return;
>>>>>> +
>>>>>>      if (!altera_pcie_link_is_up(pcie))
>>>>>>              return;
>>>>>
>>>>> Instead of making this a PCI fixup, can you make an
>>>>> altera_pcie_host_init() function, call it from altera_pcie_probe(),
>>>>> and do the link retrain there?  Then you wouldn't need to worry about
>>>>> whether this is a Root Port or an Endpoint, plus it would make the
>>>>> altera driver structure more like the other drivers.
>>>>>
>>>>> You would call altera_pcie_host_init() before pci_scan_root_bus(), so
>>>>> you wouldn't have a pci_dev yet, so you wouldn't be able to use
>>>>> pcie_capability_set_word() to set the PCI_EXP_LNKCTL_RL bit.  But I
>>>>> assume there's some device-dependent way to access it using
>>>>> cra_writel()?
>>>> We can't use cra_write() to set PCI_EXP_LNKCTL_RL bit.
>>>
>>> Why not?  I don't mean it has to be cra_write(), but isn't there some
>>> way you can write that bit before we scan the root bus?  It doesn't
>>> make sense that we have to scan the bus before we can train the link.
>>>
>>> We want to be able to tell the PCI core "all the device-specific root
>>> complex initialization has been done, here are the config accessors
>>> you need, please scan for devices."  I want to keep device-specific
>>> things like this quirk directly in the driver and out of the
>>> enumeration process.
>>>
>>>> We can use
>>>> pci_bus_find_capability() and pci_bus_read_config_word() with struct
>>>> pci_bus instead.
>>>> But this only can be called after pci_scan_root_bus().
>>>
>>>> Found
>>>> iproc_pcie_check_link() have similar implementation.
>>>
>>> You're right, and I don't like iproc_pcie_check_link() either, for the
>>> same reasons.
>>>
>>> The iproc_pcie_check_link() is a little better because it's called
>>> before enumeration:
>>>
>>>  pci_create_root_bus()
>>>  iproc_pcie_check_link()
>>>  pci_scan_child_bus()
>>>
>>> But it would be a lot better if iproc_pcie_check_link() were done
>>> first, before pci_create_root_bus().  Then it would be more like the
>>> structure of other drivers, and we could use pci_scan_root_bus()
>>> instead.
>>
>> Although not yet tested, I suppose we can do iproc_pcie_check_link
>> before calling pci_scan_root_bus so we can get rid of separate calls
>> to pci_create_root_bus and pci_scan_child_bus. But then we need to
>> create some dummy bus in the iproc_pcie_check_link function to allow
>> access to the root bus for link check, which was the primary reason
>> why we did pci_create_root_bus before iproc_pcie_check_link, i.e.,
>> to avoid the use of dummy root bus.
>
> I don't want a dummy root bus.

Okay we are on the same page for this.

> There should be some way to structure that code so you can write the
> class code and the link status stuff without having a struct pci_bus.
> The only reason you need the struct pci_bus in the first place is so
> you can extract the struct iproc_pcie *, and you already have that in
> iproc_pcie_check_link().
>
> No, you won't be able to use pci_bus_find_capability(), but presumably
> you already *know* where the capability is, since you know exactly
> what device this is.

I'll need to review the check link function carefully and do some 
experiment to see what I can do to determine the link status without 
accessing any of the configuration registers, which is what you seem to 
imply here.

Thanks.

>
> Bjorn
>

Re: [PATCH v2] PCI: altera: Retrain link in rootport mode only

[Bjorn Helgaas]

On Tue, Aug 30, 2016 at 09:36:52AM -0700, Ray Jui wrote:
> 
> 
> On 8/30/2016 6:37 AM, Bjorn Helgaas wrote:
> >On Mon, Aug 29, 2016 at 05:37:09PM -0700, Ray Jui wrote:
> >>Hi Bjorn,
> >>
> >>On 8/24/2016 10:54 AM, Bjorn Helgaas wrote:
> >>>[+cc Ray, Scott, Jon, bcm-kernel-feedback-list]
> >>>
> >>>On Wed, Aug 24, 2016 at 03:07:52PM +0800, Ley Foon Tan wrote:
> >>>>On Mon, Aug 22, 2016 at 11:47 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >>>>>On Fri, Aug 19, 2016 at 04:24:38PM +0800, Ley Foon Tan wrote:
> >>>>>>Altera PCIe IP can be configured as rootport or device and they might have
> >>>>>>same vendor ID. It will cause the system hang issue if Altera PCIe is in
> >>>>>>endpoint mode and work with other PCIe rootport that from other vendors.
> >>>>>>So, add the rootport mode checking in link retrain fixup function.
> >>>>>>
> >>>>>>Signed-off-by: Ley Foon Tan <lftan@altera.com>
> >>>>>>---
> >>>>>>v2: change to check PCIe type is PCI_EXP_TYPE_ROOT_PORT
> >>>>>>---
> >>>>>>drivers/pci/host/pcie-altera.c | 3 +++
> >>>>>>1 file changed, 3 insertions(+)
> >>>>>>
> >>>>>>diff --git a/drivers/pci/host/pcie-altera.c b/drivers/pci/host/pcie-altera.c
> >>>>>>index 58eef99..33b6968 100644
> >>>>>>--- a/drivers/pci/host/pcie-altera.c
> >>>>>>+++ b/drivers/pci/host/pcie-altera.c
> >>>>>>@@ -139,6 +139,9 @@ static void altera_pcie_retrain(struct pci_dev *dev)
> >>>>>>     u16 linkcap, linkstat;
> >>>>>>     struct altera_pcie *pcie = dev->bus->sysdata;
> >>>>>>
> >>>>>>+     if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
> >>>>>>+             return;
> >>>>>>+
> >>>>>>     if (!altera_pcie_link_is_up(pcie))
> >>>>>>             return;
> >>>>>
> >>>>>Instead of making this a PCI fixup, can you make an
> >>>>>altera_pcie_host_init() function, call it from altera_pcie_probe(),
> >>>>>and do the link retrain there?  Then you wouldn't need to worry about
> >>>>>whether this is a Root Port or an Endpoint, plus it would make the
> >>>>>altera driver structure more like the other drivers.
> >>>>>
> >>>>>You would call altera_pcie_host_init() before pci_scan_root_bus(), so
> >>>>>you wouldn't have a pci_dev yet, so you wouldn't be able to use
> >>>>>pcie_capability_set_word() to set the PCI_EXP_LNKCTL_RL bit.  But I
> >>>>>assume there's some device-dependent way to access it using
> >>>>>cra_writel()?
> >>>>We can't use cra_write() to set PCI_EXP_LNKCTL_RL bit.
> >>>
> >>>Why not?  I don't mean it has to be cra_write(), but isn't there some
> >>>way you can write that bit before we scan the root bus?  It doesn't
> >>>make sense that we have to scan the bus before we can train the link.
> >>>
> >>>We want to be able to tell the PCI core "all the device-specific root
> >>>complex initialization has been done, here are the config accessors
> >>>you need, please scan for devices."  I want to keep device-specific
> >>>things like this quirk directly in the driver and out of the
> >>>enumeration process.
> >>>
> >>>>We can use
> >>>>pci_bus_find_capability() and pci_bus_read_config_word() with struct
> >>>>pci_bus instead.
> >>>>But this only can be called after pci_scan_root_bus().
> >>>
> >>>>Found
> >>>>iproc_pcie_check_link() have similar implementation.
> >>>
> >>>You're right, and I don't like iproc_pcie_check_link() either, for the
> >>>same reasons.
> >>>
> >>>The iproc_pcie_check_link() is a little better because it's called
> >>>before enumeration:
> >>>
> >>> pci_create_root_bus()
> >>> iproc_pcie_check_link()
> >>> pci_scan_child_bus()
> >>>
> >>>But it would be a lot better if iproc_pcie_check_link() were done
> >>>first, before pci_create_root_bus().  Then it would be more like the
> >>>structure of other drivers, and we could use pci_scan_root_bus()
> >>>instead.
> >>
> >>Although not yet tested, I suppose we can do iproc_pcie_check_link
> >>before calling pci_scan_root_bus so we can get rid of separate calls
> >>to pci_create_root_bus and pci_scan_child_bus. But then we need to
> >>create some dummy bus in the iproc_pcie_check_link function to allow
> >>access to the root bus for link check, which was the primary reason
> >>why we did pci_create_root_bus before iproc_pcie_check_link, i.e.,
> >>to avoid the use of dummy root bus.
> >
> >I don't want a dummy root bus.
> 
> Okay we are on the same page for this.
> 
> >There should be some way to structure that code so you can write the
> >class code and the link status stuff without having a struct pci_bus.
> >The only reason you need the struct pci_bus in the first place is so
> >you can extract the struct iproc_pcie *, and you already have that in
> >iproc_pcie_check_link().
> >
> >No, you won't be able to use pci_bus_find_capability(), but presumably
> >you already *know* where the capability is, since you know exactly
> >what device this is.
> 
> I'll need to review the check link function carefully and do some
> experiment to see what I can do to determine the link status without
> accessing any of the configuration registers, which is what you seem
> to imply here.

No, that's not what I'm trying to say.  You can access the
configuration registers if you need to.  But you shouldn't need a
struct pci_bus to do that.  All you do with the struct pci_bus is get
the corresponding struct iproc_pcie.

It will require some restructuring, of course, e.g., making low-level
accessors that take the struct iproc_pcie, and wrappers around them
that take a struct pci_bus.  The usual config accesses can go through
the wrapper, and the iproc-internal accesses can use the low-level
accessors directly.

Bjorn

Re: [PATCH v2] PCI: altera: Retrain link in rootport mode only

[Ray Jui]

On 8/30/2016 10:00 AM, Bjorn Helgaas wrote:
> On Tue, Aug 30, 2016 at 09:36:52AM -0700, Ray Jui wrote:
>>
>>
>> On 8/30/2016 6:37 AM, Bjorn Helgaas wrote:
>>> On Mon, Aug 29, 2016 at 05:37:09PM -0700, Ray Jui wrote:
>>>> Hi Bjorn,
>>>>
>>>> On 8/24/2016 10:54 AM, Bjorn Helgaas wrote:
>>>>> [+cc Ray, Scott, Jon, bcm-kernel-feedback-list]
>>>>>
>>>>> On Wed, Aug 24, 2016 at 03:07:52PM +0800, Ley Foon Tan wrote:
>>>>>> On Mon, Aug 22, 2016 at 11:47 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>>>>>> On Fri, Aug 19, 2016 at 04:24:38PM +0800, Ley Foon Tan wrote:
>>>>>>>> Altera PCIe IP can be configured as rootport or device and they might have
>>>>>>>> same vendor ID. It will cause the system hang issue if Altera PCIe is in
>>>>>>>> endpoint mode and work with other PCIe rootport that from other vendors.
>>>>>>>> So, add the rootport mode checking in link retrain fixup function.
>>>>>>>>
>>>>>>>> Signed-off-by: Ley Foon Tan <lftan@altera.com>
>>>>>>>> ---
>>>>>>>> v2: change to check PCIe type is PCI_EXP_TYPE_ROOT_PORT
>>>>>>>> ---
>>>>>>>> drivers/pci/host/pcie-altera.c | 3 +++
>>>>>>>> 1 file changed, 3 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/pci/host/pcie-altera.c b/drivers/pci/host/pcie-altera.c
>>>>>>>> index 58eef99..33b6968 100644
>>>>>>>> --- a/drivers/pci/host/pcie-altera.c
>>>>>>>> +++ b/drivers/pci/host/pcie-altera.c
>>>>>>>> @@ -139,6 +139,9 @@ static void altera_pcie_retrain(struct pci_dev *dev)
>>>>>>>>     u16 linkcap, linkstat;
>>>>>>>>     struct altera_pcie *pcie = dev->bus->sysdata;
>>>>>>>>
>>>>>>>> +     if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
>>>>>>>> +             return;
>>>>>>>> +
>>>>>>>>     if (!altera_pcie_link_is_up(pcie))
>>>>>>>>             return;
>>>>>>>
>>>>>>> Instead of making this a PCI fixup, can you make an
>>>>>>> altera_pcie_host_init() function, call it from altera_pcie_probe(),
>>>>>>> and do the link retrain there?  Then you wouldn't need to worry about
>>>>>>> whether this is a Root Port or an Endpoint, plus it would make the
>>>>>>> altera driver structure more like the other drivers.
>>>>>>>
>>>>>>> You would call altera_pcie_host_init() before pci_scan_root_bus(), so
>>>>>>> you wouldn't have a pci_dev yet, so you wouldn't be able to use
>>>>>>> pcie_capability_set_word() to set the PCI_EXP_LNKCTL_RL bit.  But I
>>>>>>> assume there's some device-dependent way to access it using
>>>>>>> cra_writel()?
>>>>>> We can't use cra_write() to set PCI_EXP_LNKCTL_RL bit.
>>>>>
>>>>> Why not?  I don't mean it has to be cra_write(), but isn't there some
>>>>> way you can write that bit before we scan the root bus?  It doesn't
>>>>> make sense that we have to scan the bus before we can train the link.
>>>>>
>>>>> We want to be able to tell the PCI core "all the device-specific root
>>>>> complex initialization has been done, here are the config accessors
>>>>> you need, please scan for devices."  I want to keep device-specific
>>>>> things like this quirk directly in the driver and out of the
>>>>> enumeration process.
>>>>>
>>>>>> We can use
>>>>>> pci_bus_find_capability() and pci_bus_read_config_word() with struct
>>>>>> pci_bus instead.
>>>>>> But this only can be called after pci_scan_root_bus().
>>>>>
>>>>>> Found
>>>>>> iproc_pcie_check_link() have similar implementation.
>>>>>
>>>>> You're right, and I don't like iproc_pcie_check_link() either, for the
>>>>> same reasons.
>>>>>
>>>>> The iproc_pcie_check_link() is a little better because it's called
>>>>> before enumeration:
>>>>>
>>>>> pci_create_root_bus()
>>>>> iproc_pcie_check_link()
>>>>> pci_scan_child_bus()
>>>>>
>>>>> But it would be a lot better if iproc_pcie_check_link() were done
>>>>> first, before pci_create_root_bus().  Then it would be more like the
>>>>> structure of other drivers, and we could use pci_scan_root_bus()
>>>>> instead.
>>>>
>>>> Although not yet tested, I suppose we can do iproc_pcie_check_link
>>>> before calling pci_scan_root_bus so we can get rid of separate calls
>>>> to pci_create_root_bus and pci_scan_child_bus. But then we need to
>>>> create some dummy bus in the iproc_pcie_check_link function to allow
>>>> access to the root bus for link check, which was the primary reason
>>>> why we did pci_create_root_bus before iproc_pcie_check_link, i.e.,
>>>> to avoid the use of dummy root bus.
>>>
>>> I don't want a dummy root bus.
>>
>> Okay we are on the same page for this.
>>
>>> There should be some way to structure that code so you can write the
>>> class code and the link status stuff without having a struct pci_bus.
>>> The only reason you need the struct pci_bus in the first place is so
>>> you can extract the struct iproc_pcie *, and you already have that in
>>> iproc_pcie_check_link().
>>>
>>> No, you won't be able to use pci_bus_find_capability(), but presumably
>>> you already *know* where the capability is, since you know exactly
>>> what device this is.
>>
>> I'll need to review the check link function carefully and do some
>> experiment to see what I can do to determine the link status without
>> accessing any of the configuration registers, which is what you seem
>> to imply here.
>
> No, that's not what I'm trying to say.  You can access the
> configuration registers if you need to.  But you shouldn't need a
> struct pci_bus to do that.  All you do with the struct pci_bus is get
> the corresponding struct iproc_pcie.
>
> It will require some restructuring, of course, e.g., making low-level
> accessors that take the struct iproc_pcie, and wrappers around them
> that take a struct pci_bus.  The usual config accesses can go through
> the wrapper, and the iproc-internal accesses can use the low-level
> accessors directly.
>
> Bjorn
>

Okay I got it. Thanks for the clarifications. I'll look into this when I 
have a chance.

Ray

Re: [PATCH v2] PCI: altera: Retrain link in rootport mode only

[Ray Jui]

Hi Bjorn,

On 8/30/2016 10:04 AM, Ray Jui wrote:
>
>
> On 8/30/2016 10:00 AM, Bjorn Helgaas wrote:
>> On Tue, Aug 30, 2016 at 09:36:52AM -0700, Ray Jui wrote:
>>>
>>>
>>> On 8/30/2016 6:37 AM, Bjorn Helgaas wrote:
>>>> On Mon, Aug 29, 2016 at 05:37:09PM -0700, Ray Jui wrote:
>>>>> Hi Bjorn,
>>>>>
>>>>> On 8/24/2016 10:54 AM, Bjorn Helgaas wrote:
>>>>>> [+cc Ray, Scott, Jon, bcm-kernel-feedback-list]
>>>>>>
>>>>>> On Wed, Aug 24, 2016 at 03:07:52PM +0800, Ley Foon Tan wrote:
>>>>>>> On Mon, Aug 22, 2016 at 11:47 PM, Bjorn Helgaas
>>>>>>> <helgaas@kernel.org> wrote:
>>>>>>>> On Fri, Aug 19, 2016 at 04:24:38PM +0800, Ley Foon Tan wrote:
>>>>>>>>> Altera PCIe IP can be configured as rootport or device and they
>>>>>>>>> might have
>>>>>>>>> same vendor ID. It will cause the system hang issue if Altera
>>>>>>>>> PCIe is in
>>>>>>>>> endpoint mode and work with other PCIe rootport that from other
>>>>>>>>> vendors.
>>>>>>>>> So, add the rootport mode checking in link retrain fixup function.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Ley Foon Tan <lftan@altera.com>
>>>>>>>>> ---
>>>>>>>>> v2: change to check PCIe type is PCI_EXP_TYPE_ROOT_PORT
>>>>>>>>> ---
>>>>>>>>> drivers/pci/host/pcie-altera.c | 3 +++
>>>>>>>>> 1 file changed, 3 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/pci/host/pcie-altera.c
>>>>>>>>> b/drivers/pci/host/pcie-altera.c
>>>>>>>>> index 58eef99..33b6968 100644
>>>>>>>>> --- a/drivers/pci/host/pcie-altera.c
>>>>>>>>> +++ b/drivers/pci/host/pcie-altera.c
>>>>>>>>> @@ -139,6 +139,9 @@ static void altera_pcie_retrain(struct
>>>>>>>>> pci_dev *dev)
>>>>>>>>>     u16 linkcap, linkstat;
>>>>>>>>>     struct altera_pcie *pcie = dev->bus->sysdata;
>>>>>>>>>
>>>>>>>>> +     if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
>>>>>>>>> +             return;
>>>>>>>>> +
>>>>>>>>>     if (!altera_pcie_link_is_up(pcie))
>>>>>>>>>             return;
>>>>>>>>
>>>>>>>> Instead of making this a PCI fixup, can you make an
>>>>>>>> altera_pcie_host_init() function, call it from altera_pcie_probe(),
>>>>>>>> and do the link retrain there?  Then you wouldn't need to worry
>>>>>>>> about
>>>>>>>> whether this is a Root Port or an Endpoint, plus it would make the
>>>>>>>> altera driver structure more like the other drivers.
>>>>>>>>
>>>>>>>> You would call altera_pcie_host_init() before
>>>>>>>> pci_scan_root_bus(), so
>>>>>>>> you wouldn't have a pci_dev yet, so you wouldn't be able to use
>>>>>>>> pcie_capability_set_word() to set the PCI_EXP_LNKCTL_RL bit.  But I
>>>>>>>> assume there's some device-dependent way to access it using
>>>>>>>> cra_writel()?
>>>>>>> We can't use cra_write() to set PCI_EXP_LNKCTL_RL bit.
>>>>>>
>>>>>> Why not?  I don't mean it has to be cra_write(), but isn't there some
>>>>>> way you can write that bit before we scan the root bus?  It doesn't
>>>>>> make sense that we have to scan the bus before we can train the link.
>>>>>>
>>>>>> We want to be able to tell the PCI core "all the device-specific root
>>>>>> complex initialization has been done, here are the config accessors
>>>>>> you need, please scan for devices."  I want to keep device-specific
>>>>>> things like this quirk directly in the driver and out of the
>>>>>> enumeration process.
>>>>>>
>>>>>>> We can use
>>>>>>> pci_bus_find_capability() and pci_bus_read_config_word() with struct
>>>>>>> pci_bus instead.
>>>>>>> But this only can be called after pci_scan_root_bus().
>>>>>>
>>>>>>> Found
>>>>>>> iproc_pcie_check_link() have similar implementation.
>>>>>>
>>>>>> You're right, and I don't like iproc_pcie_check_link() either, for
>>>>>> the
>>>>>> same reasons.
>>>>>>
>>>>>> The iproc_pcie_check_link() is a little better because it's called
>>>>>> before enumeration:
>>>>>>
>>>>>> pci_create_root_bus()
>>>>>> iproc_pcie_check_link()
>>>>>> pci_scan_child_bus()
>>>>>>
>>>>>> But it would be a lot better if iproc_pcie_check_link() were done
>>>>>> first, before pci_create_root_bus().  Then it would be more like the
>>>>>> structure of other drivers, and we could use pci_scan_root_bus()
>>>>>> instead.
>>>>>
>>>>> Although not yet tested, I suppose we can do iproc_pcie_check_link
>>>>> before calling pci_scan_root_bus so we can get rid of separate calls
>>>>> to pci_create_root_bus and pci_scan_child_bus. But then we need to
>>>>> create some dummy bus in the iproc_pcie_check_link function to allow
>>>>> access to the root bus for link check, which was the primary reason
>>>>> why we did pci_create_root_bus before iproc_pcie_check_link, i.e.,
>>>>> to avoid the use of dummy root bus.
>>>>
>>>> I don't want a dummy root bus.
>>>
>>> Okay we are on the same page for this.
>>>
>>>> There should be some way to structure that code so you can write the
>>>> class code and the link status stuff without having a struct pci_bus.
>>>> The only reason you need the struct pci_bus in the first place is so
>>>> you can extract the struct iproc_pcie *, and you already have that in
>>>> iproc_pcie_check_link().
>>>>
>>>> No, you won't be able to use pci_bus_find_capability(), but presumably
>>>> you already *know* where the capability is, since you know exactly
>>>> what device this is.
>>>
>>> I'll need to review the check link function carefully and do some
>>> experiment to see what I can do to determine the link status without
>>> accessing any of the configuration registers, which is what you seem
>>> to imply here.
>>
>> No, that's not what I'm trying to say.  You can access the
>> configuration registers if you need to.  But you shouldn't need a
>> struct pci_bus to do that.  All you do with the struct pci_bus is get
>> the corresponding struct iproc_pcie.
>>
>> It will require some restructuring, of course, e.g., making low-level
>> accessors that take the struct iproc_pcie, and wrappers around them
>> that take a struct pci_bus.  The usual config accesses can go through
>> the wrapper, and the iproc-internal accesses can use the low-level
>> accessors directly.
>>
>> Bjorn
>>
>
> Okay I got it. Thanks for the clarifications. I'll look into this when I
> have a chance.
>
> Ray

By looking at the code closer, I found that we have an issue here, i.e., 
currently the iProc PCIe driver accesses the configuration registers 
using the standard struct pci_ops (i.e., pci_generic_config_read32 and 
pci_generic_config_write32). There's no existing iProc specific 
low-level code for configuration register access (well, there actually 
was previously, until we switched to use the generic code). As a result, 
unless we diverge from using the generic code, I cannot think of a way 
to make this work.

Any comment on this?

Regards,

Ray