[v6,2/9] PCI: mediatek: Fixup class ID for MT7622 as PCI_CLASS_BRIDGE_PCI

Commit Message

Honghui Zhang Oct. 8, 2018, 3:24 a.m. UTC

From: Honghui Zhang <[e-mail]>

The PCIe controller of MT7622 has TYPE 1 configuration space type, but
the HW default class type values is invalid.

The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID and class
type for MT7622") have set the class ID for MT7622 as
PCI_CLASS_BRIDGE_HOST, but it's not workable for MT7622:

In __pci_bus_assign_resources, the framework only setup bridge's
resource window only if class type is PCI_CLASS_BRIDGE_PCI. Or it
will leave the subordinary PCIe device's MMIO window un-touched.

Fixup the class type to PCI_CLASS_BRIDGE_PCI as most of the controller
driver do.

Signed-off-by: Honghui Zhang <[e-mail]>
Acked-by: Ryder Lee <[e-mail]>
---
 drivers/pci/controller/pcie-mediatek.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Lorenzo Pieralisi Oct. 8, 2018, 5:23 p.m. UTC | #1

On Mon, Oct 08, 2018 at 11:24:41AM +0800, [e-mail] wrote:
> From: Honghui Zhang <[e-mail]>
> 
> The PCIe controller of MT7622 has TYPE 1 configuration space type, but
> the HW default class type values is invalid.
> 
> The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID and class
> type for MT7622") have set the class ID for MT7622 as
> PCI_CLASS_BRIDGE_HOST, but it's not workable for MT7622:
> 
> In __pci_bus_assign_resources, the framework only setup bridge's
> resource window only if class type is PCI_CLASS_BRIDGE_PCI. Or it
> will leave the subordinary PCIe device's MMIO window un-touched.
> 
> Fixup the class type to PCI_CLASS_BRIDGE_PCI as most of the controller
> driver do.

I think that this patch is correct but the commit log fails to pin point
the problem. The IP you are programming is a root port, that's why you
have to have the proper class code, the patch looks fine but I would
like to peek Bjorn's brain on this since it is a fundamental concept.

If the kernel does not assign resources unless it detects a
PCI_CLASS_BRIDGE_PCI this means that for components that are
actually PCI_CLASS_BRIDGE_HOST their register set must come
preprogrammed unless I am missing something.

I would like to get to the bottom of this since it is a fundamental
enumeration concept.

Thanks,
Lorenzo

> 
> Signed-off-by: Honghui Zhang <[e-mail]>
> Acked-by: Ryder Lee <[e-mail]>
> ---
>  drivers/pci/controller/pcie-mediatek.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> index 288b8e2..bcdac9b 100644
> --- a/drivers/pci/controller/pcie-mediatek.c
> +++ b/drivers/pci/controller/pcie-mediatek.c
> @@ -432,7 +432,7 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
>  		val = PCI_VENDOR_ID_MEDIATEK;
>  		writew(val, port->base + PCIE_CONF_VEND_ID);
>  
> -		val = PCI_CLASS_BRIDGE_HOST;
> +		val = PCI_CLASS_BRIDGE_PCI;
>  		writew(val, port->base + PCIE_CONF_CLASS_ID);
>  	}
>  
> -- 
> 2.6.4
>

Honghui Zhang Oct. 9, 2018, 3:08 a.m. UTC | #2

On Mon, 2018-10-08 at 18:23 +0100, Lorenzo Pieralisi wrote:
> On Mon, Oct 08, 2018 at 11:24:41AM +0800, [e-mail] wrote:
> > From: Honghui Zhang <[e-mail]>
> > 
> > The PCIe controller of MT7622 has TYPE 1 configuration space type, but
> > the HW default class type values is invalid.
> > 
> > The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID and class
> > type for MT7622") have set the class ID for MT7622 as
> > PCI_CLASS_BRIDGE_HOST, but it's not workable for MT7622:
> > 
> > In __pci_bus_assign_resources, the framework only setup bridge's
> > resource window only if class type is PCI_CLASS_BRIDGE_PCI. Or it
> > will leave the subordinary PCIe device's MMIO window un-touched.
> > 
> > Fixup the class type to PCI_CLASS_BRIDGE_PCI as most of the controller
> > driver do.
> 
> I think that this patch is correct but the commit log fails to pin point
> the problem. The IP you are programming is a root port, that's why you
> have to have the proper class code, the patch looks fine but I would
> like to peek Bjorn's brain on this since it is a fundamental concept.
> 

I'm a bit confused with the concepts of PCI_CLASS_BRIDGE_HOST and
PCI_CLASS_BRIDGE_PCI, from PCI express spec,
4.0r1.0(PCI_Express_Base_4.0r1.0_September-27-2017-c), Host Bridge is
"part of a Root Complex that connects a host CPU or CPUs to a
Hierarchy". While Root Complex defines as "A defined System Element that
includes at least one Host Bridge, Root port, or Root complex Integrated
Endpoint".

But according to my understanding, most of the root port IPs integrated
with a "PCI_CLASS_BRIDGE_PCI", which has type 1 configuration space and
could be saw as a pci device when using lspci.

And for MT7622, it integrated with block of internal control registers,
type 1 configuration space, and is considered as a root complex.

I'm not sure which CLASS type it should have:
From PCI_Code-ID_r_1_10__v8-Nov_2017, class type of
0x0604(PCI_CLASS_BRIDGE_PCI) is defined as a PCI-to-PCI bridge, not
literally suitable for MT7622(which is a root complex)(In my personal
opinion). But it is the only workable CLASS type for MT7622 in current
kernel.

> If the kernel does not assign resources unless it detects a
> PCI_CLASS_BRIDGE_PCI this means that for components that are
> actually PCI_CLASS_BRIDGE_HOST their register set must come
> preprogrammed unless I am missing something.
> 

In the function pci_request_resource_alignment, it will by pass the
resource assignment for PCI_CLASS_BRIDGE_HOST, though I'm not figured
out why.

> I would like to get to the bottom of this since it is a fundamental
> enumeration concept.
> 

Do you like me to re-write the commit message for this patch and put the
above information in? Or just not mention the PCI_CLASS_BRIDGE_HOST
assign resource routine?

Thanks

> Thanks,
> Lorenzo
> 
> > 
> > Signed-off-by: Honghui Zhang <[e-mail]>
> > Acked-by: Ryder Lee <[e-mail]>
> > ---
> >  drivers/pci/controller/pcie-mediatek.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> > index 288b8e2..bcdac9b 100644
> > --- a/drivers/pci/controller/pcie-mediatek.c
> > +++ b/drivers/pci/controller/pcie-mediatek.c
> > @@ -432,7 +432,7 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
> >  		val = PCI_VENDOR_ID_MEDIATEK;
> >  		writew(val, port->base + PCIE_CONF_VEND_ID);
> >  
> > -		val = PCI_CLASS_BRIDGE_HOST;
> > +		val = PCI_CLASS_BRIDGE_PCI;
> >  		writew(val, port->base + PCIE_CONF_CLASS_ID);
> >  	}
> >  
> > -- 
> > 2.6.4
> >

Lorenzo Pieralisi Oct. 11, 2018, 11:38 a.m. UTC | #3

On Tue, Oct 09, 2018 at 11:08:15AM +0800, Honghui Zhang wrote:
> On Mon, 2018-10-08 at 18:23 +0100, Lorenzo Pieralisi wrote:
> > On Mon, Oct 08, 2018 at 11:24:41AM +0800, [e-mail] wrote:
> > > From: Honghui Zhang <[e-mail]>
> > > 
> > > The PCIe controller of MT7622 has TYPE 1 configuration space type, but
> > > the HW default class type values is invalid.
> > > 
> > > The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID and class
> > > type for MT7622") have set the class ID for MT7622 as
> > > PCI_CLASS_BRIDGE_HOSTe, but it's not workable for MT7622:
> > > 
> > > In __pci_bus_assign_resources, the framework only setup bridge's
> > > resource window only if class type is PCI_CLASS_BRIDGE_PCI. Or it
> > > will leave the subordinary PCIe device's MMIO window un-touched.

Can you please provide me with a full log of the issue ?

What is the bug this patch is actually fixing ?

> > > Fixup the class type to PCI_CLASS_BRIDGE_PCI as most of the controller
> > > driver do.
> > 
> > I think that this patch is correct but the commit log fails to pin point
> > the problem. The IP you are programming is a root port, that's why you
> > have to have the proper class code, the patch looks fine but I would
> > like to peek Bjorn's brain on this since it is a fundamental concept.
> > 
> 
> I'm a bit confused with the concepts of PCI_CLASS_BRIDGE_HOST and
> PCI_CLASS_BRIDGE_PCI, from PCI express spec,
> 4.0r1.0(PCI_Express_Base_4.0r1.0_September-27-2017-c), Host Bridge is
> "part of a Root Complex that connects a host CPU or CPUs to a
> Hierarchy". While Root Complex defines as "A defined System Element that
> includes at least one Host Bridge, Root port, or Root complex Integrated
> Endpoint".
> 
> But according to my understanding, most of the root port IPs integrated
> with a "PCI_CLASS_BRIDGE_PCI", which has type 1 configuration space and
> could be saw as a pci device when using lspci.
> 
> And for MT7622, it integrated with block of internal control registers,
> type 1 configuration space, and is considered as a root complex.

I assume you mean a type 1 config header here. I do not think it
is mandatory for a host bridge to have a type 1 config header
(and related bridge windows + primary/secondary/subordinate bus
numbers) but I do not know how the IP you are programming is
designed.

If the host bridge needs its memory window to be configured you can
easily do that in the driver (with driver specific code) without
requiring the generic PCI resource core to do that for you, the host
bridge is the root of the bus I do not see any reason why it should
be up to core PCI code to assign it, it is preprogrammed.

> I'm not sure which CLASS type it should have:
> From PCI_Code-ID_r_1_10__v8-Nov_2017, class type of
> 0x0604(PCI_CLASS_BRIDGE_PCI) is defined as a PCI-to-PCI bridge, not
> literally suitable for MT7622(which is a root complex)(In my personal
> opinion). But it is the only workable CLASS type for MT7622 in current
> kernel.
> 
> > If the kernel does not assign resources unless it detects a
> > PCI_CLASS_BRIDGE_PCI this means that for components that are
> > actually PCI_CLASS_BRIDGE_HOST their register set must come
> > preprogrammed unless I am missing something.
> > 
> 
> In the function pci_request_resource_alignment, it will by pass the
> resource assignment for PCI_CLASS_BRIDGE_HOST, though I'm not figured
> out why.
> 
> > I would like to get to the bottom of this since it is a fundamental
> > enumeration concept.
> > 
> 
> Do you like me to re-write the commit message for this patch and put the
> above information in? Or just not mention the PCI_CLASS_BRIDGE_HOST
> assign resource routine?

I want to understand where the problem is and whether it is right to
define the component as a PCI bridge rather than a host bridge.

Lorenzo

Honghui Zhang Oct. 12, 2018, 8:01 a.m. UTC | #4

On Thu, 2018-10-11 at 12:38 +0100, Lorenzo Pieralisi wrote:
> On Tue, Oct 09, 2018 at 11:08:15AM +0800, Honghui Zhang wrote:
> > On Mon, 2018-10-08 at 18:23 +0100, Lorenzo Pieralisi wrote:
> > > On Mon, Oct 08, 2018 at 11:24:41AM +0800, [e-mail] wrote:
> > > > From: Honghui Zhang <[e-mail]>
> > > > 
> > > > The PCIe controller of MT7622 has TYPE 1 configuration space type, but
> > > > the HW default class type values is invalid.
> > > > 
> > > > The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID and class
> > > > type for MT7622") have set the class ID for MT7622 as
> > > > PCI_CLASS_BRIDGE_HOSTe, but it's not workable for MT7622:
> > > > 
> > > > In __pci_bus_assign_resources, the framework only setup bridge's
> > > > resource window only if class type is PCI_CLASS_BRIDGE_PCI. Or it
> > > > will leave the subordinary PCIe device's MMIO window un-touched.
> 
> Can you please provide me with a full log of the issue ?
> 
> What is the bug this patch is actually fixing ?
> 
> > > > Fixup the class type to PCI_CLASS_BRIDGE_PCI as most of the controller
> > > > driver do.
> > > 
> > > I think that this patch is correct but the commit log fails to pin point
> > > the problem. The IP you are programming is a root port, that's why you
> > > have to have the proper class code, the patch looks fine but I would
> > > like to peek Bjorn's brain on this since it is a fundamental concept.
> > > 
> > 
> > I'm a bit confused with the concepts of PCI_CLASS_BRIDGE_HOST and
> > PCI_CLASS_BRIDGE_PCI, from PCI express spec,
> > 4.0r1.0(PCI_Express_Base_4.0r1.0_September-27-2017-c), Host Bridge is
> > "part of a Root Complex that connects a host CPU or CPUs to a
> > Hierarchy". While Root Complex defines as "A defined System Element that
> > includes at least one Host Bridge, Root port, or Root complex Integrated
> > Endpoint".
> > 
> > But according to my understanding, most of the root port IPs integrated
> > with a "PCI_CLASS_BRIDGE_PCI", which has type 1 configuration space and
> > could be saw as a pci device when using lspci.
> > 
> > And for MT7622, it integrated with block of internal control registers,
> > type 1 configuration space, and is considered as a root complex.
> 
> I assume you mean a type 1 config header here. I do not think it
> is mandatory for a host bridge to have a type 1 config header
> (and related bridge windows + primary/secondary/subordinate bus
> numbers) but I do not know how the IP you are programming is
> designed.

Yes, MT7622 has the type 1 config header and related bridge windows and
primary/secondary/subordinate bus numbers.

> 
> If the host bridge needs its memory window to be configured you can
> easily do that in the driver (with driver specific code) without
> requiring the generic PCI resource core to do that for you, the host
> bridge is the root of the bus I do not see any reason why it should
> be up to core PCI code to assign it, it is preprogrammed.
> 
Thanks for explain this.
Yes, the MT7622 bridge needs its memory window to be configured but I
prefer the PCI resource core to do this for the following reasons:
1. MTK have MT7622 and MT2712, they pretty much using the same IP, but
have different memory window base. Currently we using device tree to
pass the memory window base. Take using of PCI resource core code to
parse and assign those resource will release the burden of driver.
2. I do not want to re-implement the resource parse code to get the
memory base from device node. And hard code the memory base in driver is
not quite elegant.
3. Most of the host driver which I referenced are using the PCI resource
core to help with it's memory window configure, I guess following the
majority way maybe better even they may have slit different of the IP
design from MTK.
4. Passing the memory base in device node make it more flexible to
change the memory window base (in the HW acceptable range) in case of
some special EP device required some special PCI address range.
5. MT7622 and MT2712 have the pretty much same IP only MT7622's HW has
set the class type to an un-proper class type. Set it to the same values
as MT2712 is an easy way to fix.

thanks.
> > I'm not sure which CLASS type it should have:
> > From PCI_Code-ID_r_1_10__v8-Nov_2017, class type of
> > 0x0604(PCI_CLASS_BRIDGE_PCI) is defined as a PCI-to-PCI bridge, not
> > literally suitable for MT7622(which is a root complex)(In my personal
> > opinion). But it is the only workable CLASS type for MT7622 in current
> > kernel.
> > 
> > > If the kernel does not assign resources unless it detects a
> > > PCI_CLASS_BRIDGE_PCI this means that for components that are
> > > actually PCI_CLASS_BRIDGE_HOST their register set must come
> > > preprogrammed unless I am missing something.
> > > 
> > 
> > In the function pci_request_resource_alignment, it will by pass the
> > resource assignment for PCI_CLASS_BRIDGE_HOST, though I'm not figured
> > out why.
> > 
> > > I would like to get to the bottom of this since it is a fundamental
> > > enumeration concept.
> > > 
> > 
> > Do you like me to re-write the commit message for this patch and put the
> > above information in? Or just not mention the PCI_CLASS_BRIDGE_HOST
> > assign resource routine?
> 
> I want to understand where the problem is and whether it is right to
> define the component as a PCI bridge rather than a host bridge.
> 

I will update the commit message (put some of the above reasons in the
commit message) and send a new version of this patch if it's acceptable
for you.

Thanks
> Lorenzo

Lorenzo Pieralisi Oct. 12, 2018, 10:22 a.m. UTC | #5

On Fri, Oct 12, 2018 at 04:01:29PM +0800, Honghui Zhang wrote:
> On Thu, 2018-10-11 at 12:38 +0100, Lorenzo Pieralisi wrote:
> > On Tue, Oct 09, 2018 at 11:08:15AM +0800, Honghui Zhang wrote:
> > > On Mon, 2018-10-08 at 18:23 +0100, Lorenzo Pieralisi wrote:
> > > > On Mon, Oct 08, 2018 at 11:24:41AM +0800, [e-mail] wrote:
> > > > > From: Honghui Zhang <[e-mail]>
> > > > > 
> > > > > The PCIe controller of MT7622 has TYPE 1 configuration space type, but
> > > > > the HW default class type values is invalid.
> > > > > 
> > > > > The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID and class
> > > > > type for MT7622") have set the class ID for MT7622 as
> > > > > PCI_CLASS_BRIDGE_HOSTe, but it's not workable for MT7622:
> > > > > 
> > > > > In __pci_bus_assign_resources, the framework only setup bridge's
> > > > > resource window only if class type is PCI_CLASS_BRIDGE_PCI. Or it
> > > > > will leave the subordinary PCIe device's MMIO window un-touched.
> > 
> > Can you please provide me with a full log of the issue ?
> > 
> > What is the bug this patch is actually fixing ?
> > 
> > > > > Fixup the class type to PCI_CLASS_BRIDGE_PCI as most of the controller
> > > > > driver do.
> > > > 
> > > > I think that this patch is correct but the commit log fails to pin point
> > > > the problem. The IP you are programming is a root port, that's why you
> > > > have to have the proper class code, the patch looks fine but I would
> > > > like to peek Bjorn's brain on this since it is a fundamental concept.
> > > > 
> > > 
> > > I'm a bit confused with the concepts of PCI_CLASS_BRIDGE_HOST and
> > > PCI_CLASS_BRIDGE_PCI, from PCI express spec,
> > > 4.0r1.0(PCI_Express_Base_4.0r1.0_September-27-2017-c), Host Bridge is
> > > "part of a Root Complex that connects a host CPU or CPUs to a
> > > Hierarchy". While Root Complex defines as "A defined System Element that
> > > includes at least one Host Bridge, Root port, or Root complex Integrated
> > > Endpoint".
> > > 
> > > But according to my understanding, most of the root port IPs integrated
> > > with a "PCI_CLASS_BRIDGE_PCI", which has type 1 configuration space and
> > > could be saw as a pci device when using lspci.
> > > 
> > > And for MT7622, it integrated with block of internal control registers,
> > > type 1 configuration space, and is considered as a root complex.
> > 
> > I assume you mean a type 1 config header here. I do not think it
> > is mandatory for a host bridge to have a type 1 config header
> > (and related bridge windows + primary/secondary/subordinate bus
> > numbers) but I do not know how the IP you are programming is
> > designed.
> 
> Yes, MT7622 has the type 1 config header and related bridge windows and
> primary/secondary/subordinate bus numbers.
> 
> > 
> > If the host bridge needs its memory window to be configured you can
> > easily do that in the driver (with driver specific code) without
> > requiring the generic PCI resource core to do that for you, the host
> > bridge is the root of the bus I do not see any reason why it should
> > be up to core PCI code to assign it, it is preprogrammed.
> > 
> Thanks for explain this.
> Yes, the MT7622 bridge needs its memory window to be configured but I
> prefer the PCI resource core to do this for the following reasons:
> 1. MTK have MT7622 and MT2712, they pretty much using the same IP, but
> have different memory window base. Currently we using device tree to
> pass the memory window base. Take using of PCI resource core code to
> parse and assign those resource will release the burden of driver.
> 2. I do not want to re-implement the resource parse code to get the
> memory base from device node. And hard code the memory base in driver is
> not quite elegant.
> 3. Most of the host driver which I referenced are using the PCI resource
> core to help with it's memory window configure, I guess following the
> majority way maybe better even they may have slit different of the IP
> design from MTK.
> 4. Passing the memory base in device node make it more flexible to
> change the memory window base (in the HW acceptable range) in case of
> some special EP device required some special PCI address range.
> 5. MT7622 and MT2712 have the pretty much same IP only MT7622's HW has
> set the class type to an un-proper class type. Set it to the same values
> as MT2712 is an easy way to fix.

We do what needs to be done. From what you are saying, your IP
implements a config 1 type header and that defines it as a PCI-to-PCI
bridge (eg a root port, of sorts).

The points you are making above are software, I understand them but
that's not what we are discussing here.

I want you to define the class of your IP according to what your IP
is and it seems _reasonable_ to me to define the IP as a PCI-to-PCI
bridge class from the information you are providing.

I would like you to:

1) Rewrite the commit log and explain why your IP needs class
   reprogramming. I do not care about driver software complexity,
   I want you to describe how HW works. I do not want you to define
   a class for your IP because that makes the driver simpler, I want
   you to define a class for your IP to describe to the kernel what
   that IP really is, which is different things.
2) Define and report the bug you are fixing in the commit log
3) Provide a Fixes: tag pointing at the faulty commit

Thanks for your time providing information.

Lorenzo

> thanks.
> > > I'm not sure which CLASS type it should have:
> > > From PCI_Code-ID_r_1_10__v8-Nov_2017, class type of
> > > 0x0604(PCI_CLASS_BRIDGE_PCI) is defined as a PCI-to-PCI bridge, not
> > > literally suitable for MT7622(which is a root complex)(In my personal
> > > opinion). But it is the only workable CLASS type for MT7622 in current
> > > kernel.
> > > 
> > > > If the kernel does not assign resources unless it detects a
> > > > PCI_CLASS_BRIDGE_PCI this means that for components that are
> > > > actually PCI_CLASS_BRIDGE_HOST their register set must come
> > > > preprogrammed unless I am missing something.
> > > > 
> > > 
> > > In the function pci_request_resource_alignment, it will by pass the
> > > resource assignment for PCI_CLASS_BRIDGE_HOST, though I'm not figured
> > > out why.
> > > 
> > > > I would like to get to the bottom of this since it is a fundamental
> > > > enumeration concept.
> > > > 
> > > 
> > > Do you like me to re-write the commit message for this patch and put the
> > > above information in? Or just not mention the PCI_CLASS_BRIDGE_HOST
> > > assign resource routine?
> > 
> > I want to understand where the problem is and whether it is right to
> > define the component as a PCI bridge rather than a host bridge.
> > 
> 
> I will update the commit message (put some of the above reasons in the
> commit message) and send a new version of this patch if it's acceptable
> for you.
> 
> Thanks
> > Lorenzo
> 
> 
>

Bjorn Helgaas Oct. 12, 2018, 2:12 p.m. UTC | #6

On Fri, Oct 12, 2018 at 11:22:30AM +0100, Lorenzo Pieralisi wrote:
> On Fri, Oct 12, 2018 at 04:01:29PM +0800, Honghui Zhang wrote:
>> On Thu, 2018-10-11 at 12:38 +0100, Lorenzo Pieralisi wrote:
>>> On Tue, Oct 09, 2018 at 11:08:15AM +0800, Honghui Zhang wrote:
>>>> On Mon, 2018-10-08 at 18:23 +0100, Lorenzo Pieralisi wrote:
>>>>> On Mon, Oct 08, 2018 at 11:24:41AM +0800, [e-mail] wrote:
>>>>>> From: Honghui Zhang <[e-mail]>
>>>>>> 
>>>>>> The PCIe controller of MT7622 has TYPE 1 configuration
>>>>>> space type, but the HW default class type values is
>>>>>> invalid.
>>>>>> 
>>>>>> The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID
>>>>>> and class type for MT7622") have set the class ID for
>>>>>> MT7622 as PCI_CLASS_BRIDGE_HOSTe, but it's not workable
>>>>>> for MT7622:
>>>>>> 
>>>>>> In __pci_bus_assign_resources, the framework only setup
>>>>>> bridge's resource window only if class type is
>>>>>> PCI_CLASS_BRIDGE_PCI. Or it will leave the subordinary PCIe
>>>>>> device's MMIO window un-touched.

I think __pci_bus_assign_resources() should be testing dev->hdr_type
instead of dev->class.  The connection between "Header Type" and the
layout of the rest of the header is very explicit (PCI r3.0 sec 6.1,
PCIe r4.0 sec 7.5.1.1.9), and the reason for the switch statement in
__pci_bus_assign_resources() is precisely to determine which layout to
use.

There are several other uses of dev->class in setup-bus.c that I think
should also be changed to use dev->hdr_type.

If we make these changes in setup-bus.c, I suspect the class code you
assign won't matter too much.  There are a few other tests of the
class code to figure out whether to leave certain things untouched.
These seem a little hacky to me, but we're probably stuck with them
for now, so you should look and see whether they apply to your
situation.

>>>> And for MT7622, it integrated with block of internal control
>>>> registers, type 1 configuration space, and is considered as a
>>>> root complex.
>>> 
>>> I assume you mean a type 1 config header here. I do not think it
>>> is mandatory for a host bridge to have a type 1 config header (and
>>> related bridge windows + primary/secondary/subordinate bus
>>> numbers) but I do not know how the IP you are programming is
>>> designed.

It is definitely not mandatory for a host bridge to have a type 1
header.  I'm not even sure that would make sense: the "Primary Bus
Number" would not apply to a host bridge (since a host bridge's
primary bus is some sort of CPU bus, not a PCI bus), and a type 1
device cannot perform address translation between its primary and
secondary buses, while a host bridge can.

A Root Port is a type 1 device where the primary bus is logically
internal to the Root Complex.  A host bridge bridges from the CPU bus
to that internal bus and might perform address translation.  The Root
Port must be a PCI device.  A host bridge, being a bridge *to* the PCI
domain, is not itself generally programmed via PCI config space and
might not even be visible as a device in PCI config space.

Bjorn

Honghui Zhang Oct. 15, 2018, 2:04 a.m. UTC | #7

On Fri, 2018-10-12 at 11:22 +0100, Lorenzo Pieralisi wrote:
> On Fri, Oct 12, 2018 at 04:01:29PM +0800, Honghui Zhang wrote:
> > On Thu, 2018-10-11 at 12:38 +0100, Lorenzo Pieralisi wrote:
> > > On Tue, Oct 09, 2018 at 11:08:15AM +0800, Honghui Zhang wrote:
> > > > On Mon, 2018-10-08 at 18:23 +0100, Lorenzo Pieralisi wrote:
> > > > > On Mon, Oct 08, 2018 at 11:24:41AM +0800, [e-mail] wrote:
> > > > > > From: Honghui Zhang <[e-mail]>
> > > > > > 
> > > > > > The PCIe controller of MT7622 has TYPE 1 configuration space type, but
> > > > > > the HW default class type values is invalid.
> > > > > > 
> > > > > > The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID and class
> > > > > > type for MT7622") have set the class ID for MT7622 as
> > > > > > PCI_CLASS_BRIDGE_HOSTe, but it's not workable for MT7622:
> > > > > > 
> > > > > > In __pci_bus_assign_resources, the framework only setup bridge's
> > > > > > resource window only if class type is PCI_CLASS_BRIDGE_PCI. Or it
> > > > > > will leave the subordinary PCIe device's MMIO window un-touched.
> > > 
> > > Can you please provide me with a full log of the issue ?
> > > 
> > > What is the bug this patch is actually fixing ?
> > > 
> > > > > > Fixup the class type to PCI_CLASS_BRIDGE_PCI as most of the controller
> > > > > > driver do.
> > > > > 
> > > > > I think that this patch is correct but the commit log fails to pin point
> > > > > the problem. The IP you are programming is a root port, that's why you
> > > > > have to have the proper class code, the patch looks fine but I would
> > > > > like to peek Bjorn's brain on this since it is a fundamental concept.
> > > > > 
> > > > 
> > > > I'm a bit confused with the concepts of PCI_CLASS_BRIDGE_HOST and
> > > > PCI_CLASS_BRIDGE_PCI, from PCI express spec,
> > > > 4.0r1.0(PCI_Express_Base_4.0r1.0_September-27-2017-c), Host Bridge is
> > > > "part of a Root Complex that connects a host CPU or CPUs to a
> > > > Hierarchy". While Root Complex defines as "A defined System Element that
> > > > includes at least one Host Bridge, Root port, or Root complex Integrated
> > > > Endpoint".
> > > > 
> > > > But according to my understanding, most of the root port IPs integrated
> > > > with a "PCI_CLASS_BRIDGE_PCI", which has type 1 configuration space and
> > > > could be saw as a pci device when using lspci.
> > > > 
> > > > And for MT7622, it integrated with block of internal control registers,
> > > > type 1 configuration space, and is considered as a root complex.
> > > 
> > > I assume you mean a type 1 config header here. I do not think it
> > > is mandatory for a host bridge to have a type 1 config header
> > > (and related bridge windows + primary/secondary/subordinate bus
> > > numbers) but I do not know how the IP you are programming is
> > > designed.
> > 
> > Yes, MT7622 has the type 1 config header and related bridge windows and
> > primary/secondary/subordinate bus numbers.
> > 
> > > 
> > > If the host bridge needs its memory window to be configured you can
> > > easily do that in the driver (with driver specific code) without
> > > requiring the generic PCI resource core to do that for you, the host
> > > bridge is the root of the bus I do not see any reason why it should
> > > be up to core PCI code to assign it, it is preprogrammed.
> > > 
> > Thanks for explain this.
> > Yes, the MT7622 bridge needs its memory window to be configured but I
> > prefer the PCI resource core to do this for the following reasons:
> > 1. MTK have MT7622 and MT2712, they pretty much using the same IP, but
> > have different memory window base. Currently we using device tree to
> > pass the memory window base. Take using of PCI resource core code to
> > parse and assign those resource will release the burden of driver.
> > 2. I do not want to re-implement the resource parse code to get the
> > memory base from device node. And hard code the memory base in driver is
> > not quite elegant.
> > 3. Most of the host driver which I referenced are using the PCI resource
> > core to help with it's memory window configure, I guess following the
> > majority way maybe better even they may have slit different of the IP
> > design from MTK.
> > 4. Passing the memory base in device node make it more flexible to
> > change the memory window base (in the HW acceptable range) in case of
> > some special EP device required some special PCI address range.
> > 5. MT7622 and MT2712 have the pretty much same IP only MT7622's HW has
> > set the class type to an un-proper class type. Set it to the same values
> > as MT2712 is an easy way to fix.
> 
> We do what needs to be done. From what you are saying, your IP
> implements a config 1 type header and that defines it as a PCI-to-PCI
> bridge (eg a root port, of sorts).
> 
> The points you are making above are software, I understand them but
> that's not what we are discussing here.
> 
> I want you to define the class of your IP according to what your IP
> is and it seems _reasonable_ to me to define the IP as a PCI-to-PCI
> bridge class from the information you are providing.
> 
> I would like you to:
> 
> 1) Rewrite the commit log and explain why your IP needs class
>    reprogramming. I do not care about driver software complexity,
>    I want you to describe how HW works. I do not want you to define
>    a class for your IP because that makes the driver simpler, I want
>    you to define a class for your IP to describe to the kernel what
>    that IP really is, which is different things.
> 2) Define and report the bug you are fixing in the commit log
> 3) Provide a Fixes: tag pointing at the faulty commit

Thanks for your explain, I will follow your suggest in the next version.

Thanks.

> Thanks for your time providing information.
> 
> Lorenzo
> 
> > thanks.
> > > > I'm not sure which CLASS type it should have:
> > > > From PCI_Code-ID_r_1_10__v8-Nov_2017, class type of
> > > > 0x0604(PCI_CLASS_BRIDGE_PCI) is defined as a PCI-to-PCI bridge, not
> > > > literally suitable for MT7622(which is a root complex)(In my personal
> > > > opinion). But it is the only workable CLASS type for MT7622 in current
> > > > kernel.
> > > > 
> > > > > If the kernel does not assign resources unless it detects a
> > > > > PCI_CLASS_BRIDGE_PCI this means that for components that are
> > > > > actually PCI_CLASS_BRIDGE_HOST their register set must come
> > > > > preprogrammed unless I am missing something.
> > > > > 
> > > > 
> > > > In the function pci_request_resource_alignment, it will by pass the
> > > > resource assignment for PCI_CLASS_BRIDGE_HOST, though I'm not figured
> > > > out why.
> > > > 
> > > > > I would like to get to the bottom of this since it is a fundamental
> > > > > enumeration concept.
> > > > > 
> > > > 
> > > > Do you like me to re-write the commit message for this patch and put the
> > > > above information in? Or just not mention the PCI_CLASS_BRIDGE_HOST
> > > > assign resource routine?
> > > 
> > > I want to understand where the problem is and whether it is right to
> > > define the component as a PCI bridge rather than a host bridge.
> > > 
> > 
> > I will update the commit message (put some of the above reasons in the
> > commit message) and send a new version of this patch if it's acceptable
> > for you.
> > 
> > Thanks
> > > Lorenzo
> > 
> > 
> >

Honghui Zhang Oct. 15, 2018, 2:42 a.m. UTC | #8

On Fri, 2018-10-12 at 09:12 -0500, Bjorn Helgaas wrote:
> On Fri, Oct 12, 2018 at 11:22:30AM +0100, Lorenzo Pieralisi wrote:
> > On Fri, Oct 12, 2018 at 04:01:29PM +0800, Honghui Zhang wrote:
> >> On Thu, 2018-10-11 at 12:38 +0100, Lorenzo Pieralisi wrote:
> >>> On Tue, Oct 09, 2018 at 11:08:15AM +0800, Honghui Zhang wrote:
> >>>> On Mon, 2018-10-08 at 18:23 +0100, Lorenzo Pieralisi wrote:
> >>>>> On Mon, Oct 08, 2018 at 11:24:41AM +0800, [e-mail] wrote:
> >>>>>> From: Honghui Zhang <[e-mail]>
> >>>>>> 
> >>>>>> The PCIe controller of MT7622 has TYPE 1 configuration
> >>>>>> space type, but the HW default class type values is
> >>>>>> invalid.
> >>>>>> 
> >>>>>> The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID
> >>>>>> and class type for MT7622") have set the class ID for
> >>>>>> MT7622 as PCI_CLASS_BRIDGE_HOSTe, but it's not workable
> >>>>>> for MT7622:
> >>>>>> 
> >>>>>> In __pci_bus_assign_resources, the framework only setup
> >>>>>> bridge's resource window only if class type is
> >>>>>> PCI_CLASS_BRIDGE_PCI. Or it will leave the subordinary PCIe
> >>>>>> device's MMIO window un-touched.
> 
> I think __pci_bus_assign_resources() should be testing dev->hdr_type
> instead of dev->class.  The connection between "Header Type" and the
> layout of the rest of the header is very explicit (PCI r3.0 sec 6.1,
> PCIe r4.0 sec 7.5.1.1.9), and the reason for the switch statement in
> __pci_bus_assign_resources() is precisely to determine which layout to
> use.
> 
> There are several other uses of dev->class in setup-bus.c that I think
> should also be changed to use dev->hdr_type.
> 
> If we make these changes in setup-bus.c, I suspect the class code you
> assign won't matter too much.  There are a few other tests of the
> class code to figure out whether to leave certain things untouched.
> These seem a little hacky to me, but we're probably stuck with them
> for now, so you should look and see whether they apply to your
> situation.

If these change could be made in the PCI core, then the class code is no
matter what will be workable for MT7622.

As Lorenzo point it out, it's more reasonable for MT7622 to defined as a
PCI-to-PCI class code since the IP is defined as that. I intend to
following Lorenzo's suggest to update the commit message and re-send
this patch set for current solution.

> 
> >>>> And for MT7622, it integrated with block of internal control
> >>>> registers, type 1 configuration space, and is considered as a
> >>>> root complex.
> >>> 
> >>> I assume you mean a type 1 config header here. I do not think it
> >>> is mandatory for a host bridge to have a type 1 config header (and
> >>> related bridge windows + primary/secondary/subordinate bus
> >>> numbers) but I do not know how the IP you are programming is
> >>> designed.
> 
> It is definitely not mandatory for a host bridge to have a type 1
> header.  I'm not even sure that would make sense: the "Primary Bus
> Number" would not apply to a host bridge (since a host bridge's
> primary bus is some sort of CPU bus, not a PCI bus), and a type 1
> device cannot perform address translation between its primary and
> secondary buses, while a host bridge can.
> 
> A Root Port is a type 1 device where the primary bus is logically
> internal to the Root Complex.  A host bridge bridges from the CPU bus
> to that internal bus and might perform address translation.  The Root
> Port must be a PCI device.  A host bridge, being a bridge *to* the PCI
> domain, is not itself generally programmed via PCI config space and
> might not even be visible as a device in PCI config space.
> 
Thanks for the explain. Per my understanding, MT7622 is more like a
complex of Root Port and PCI-to-PCI bridge. It has type 1 header and has
the ability to translate address between its primary and secondary
buses. I guess apply the class type as PCI_CLASS_BRIDGE_PCI is
reasonable way to make its integrated internal bridge workable.

Thanks.
> Bjorn

Bjorn Helgaas Oct. 15, 2018, 6:34 p.m. UTC | #9

On Mon, Oct 15, 2018 at 10:42:23AM +0800, Honghui Zhang wrote:
> On Fri, 2018-10-12 at 09:12 -0500, Bjorn Helgaas wrote:
> > On Fri, Oct 12, 2018 at 11:22:30AM +0100, Lorenzo Pieralisi wrote:
> > > On Fri, Oct 12, 2018 at 04:01:29PM +0800, Honghui Zhang wrote:
> > >> On Thu, 2018-10-11 at 12:38 +0100, Lorenzo Pieralisi wrote:
> > >>> On Tue, Oct 09, 2018 at 11:08:15AM +0800, Honghui Zhang wrote:
> > >>>> On Mon, 2018-10-08 at 18:23 +0100, Lorenzo Pieralisi wrote:
> > >>>>> On Mon, Oct 08, 2018 at 11:24:41AM +0800, [e-mail] wrote:
> > >>>>>> From: Honghui Zhang <[e-mail]>
> > >>>>>> 
> > >>>>>> The PCIe controller of MT7622 has TYPE 1 configuration
> > >>>>>> space type, but the HW default class type values is
> > >>>>>> invalid.
> > >>>>>> 
> > >>>>>> The commit 101c92dc80c8 ("PCI: mediatek: Set up vendor ID
> > >>>>>> and class type for MT7622") have set the class ID for
> > >>>>>> MT7622 as PCI_CLASS_BRIDGE_HOSTe, but it's not workable
> > >>>>>> for MT7622:
> > >>>>>> 
> > >>>>>> In __pci_bus_assign_resources, the framework only setup
> > >>>>>> bridge's resource window only if class type is
> > >>>>>> PCI_CLASS_BRIDGE_PCI. Or it will leave the subordinary PCIe
> > >>>>>> device's MMIO window un-touched.
> > 
> > I think __pci_bus_assign_resources() should be testing dev->hdr_type
> > instead of dev->class.  The connection between "Header Type" and the
> > layout of the rest of the header is very explicit (PCI r3.0 sec 6.1,
> > PCIe r4.0 sec 7.5.1.1.9), and the reason for the switch statement in
> > __pci_bus_assign_resources() is precisely to determine which layout to
> > use.
> > 
> > There are several other uses of dev->class in setup-bus.c that I think
> > should also be changed to use dev->hdr_type.
> > 
> > If we make these changes in setup-bus.c, I suspect the class code you
> > assign won't matter too much.  There are a few other tests of the
> > class code to figure out whether to leave certain things untouched.
> > These seem a little hacky to me, but we're probably stuck with them
> > for now, so you should look and see whether they apply to your
> > situation.
> 
> If these change could be made in the PCI core, then the class code is no
> matter what will be workable for MT7622.
> 
> As Lorenzo point it out, it's more reasonable for MT7622 to defined as a
> PCI-to-PCI class code since the IP is defined as that. I intend to
> following Lorenzo's suggest to update the commit message and re-send
> this patch set for current solution.
> 
> > >>>> And for MT7622, it integrated with block of internal control
> > >>>> registers, type 1 configuration space, and is considered as a
> > >>>> root complex.
> > >>> 
> > >>> I assume you mean a type 1 config header here. I do not think it
> > >>> is mandatory for a host bridge to have a type 1 config header (and
> > >>> related bridge windows + primary/secondary/subordinate bus
> > >>> numbers) but I do not know how the IP you are programming is
> > >>> designed.
> > 
> > It is definitely not mandatory for a host bridge to have a type 1
> > header.  I'm not even sure that would make sense: the "Primary Bus
> > Number" would not apply to a host bridge (since a host bridge's
> > primary bus is some sort of CPU bus, not a PCI bus), and a type 1
> > device cannot perform address translation between its primary and
> > secondary buses, while a host bridge can.
> > 
> > A Root Port is a type 1 device where the primary bus is logically
> > internal to the Root Complex.  A host bridge bridges from the CPU bus
> > to that internal bus and might perform address translation.  The Root
> > Port must be a PCI device.  A host bridge, being a bridge *to* the PCI
> > domain, is not itself generally programmed via PCI config space and
> > might not even be visible as a device in PCI config space.
> > 
> Thanks for the explain. Per my understanding, MT7622 is more like a
> complex of Root Port and PCI-to-PCI bridge. It has type 1 header and has
> the ability to translate address between its primary and secondary
> buses.

Nope.  Logically speaking, the PCI device in question is a Root Port,
which is a bridge between a primary PCI bus (probably bus 0) that is
internal to the Root Complex, and a secondary PCI bus.  This bridge,
like all other PCI-to-PCI bridges, does no address translation.

The Root Complex *also* contains a host bridge from whatever the
upstream CPU bus is and the logical PCI bus 0 internal to the Root
Complex.  This host bridge can perform address translation.

The Root Port PCI device might also have device-specific PCI config
registers, and those might offer some control over the host bridge
part of the Root Complex.  There is probably an MMIO (non-PCI config
space) way to configure the Root Complex, since you may not be able to
access the PCI config space before the Root Complex is configured.

> I guess apply the class type as PCI_CLASS_BRIDGE_PCI is
> reasonable way to make its integrated internal bridge workable.

Nope, that's not the way this process works.  You proposed setting
the class code as "a way to make this work".  I did a bunch of
research to figure out the best way of solving this and pointed out
a defect in the PCI core.  Fixing that defect will solve your problem
and possibly others.

The correct next step is for you to make that PCI core change, test it
and make sure it works, and post that as part of your series.  If, in
addition, you want to change the class code of your device, you can
also do that, but that's a secondary, optional step as far as I'm
concerned.

Bjorn

Patch

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 288b8e2..bcdac9b 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -432,7 +432,7 @@  static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
 		val = PCI_VENDOR_ID_MEDIATEK;
 		writew(val, port->base + PCIE_CONF_VEND_ID);
 
-		val = PCI_CLASS_BRIDGE_HOST;
+		val = PCI_CLASS_BRIDGE_PCI;
 		writew(val, port->base + PCIE_CONF_CLASS_ID);
 	}