[U-Boot] [PATCH V2 1/2] pci: Support parsing PCI controller DT subnodes

[U-Boot] [PATCH V2 1/2] pci: Support parsing PCI controller DT subnodes

[Marek Vasut]

The PCI controller can have DT subnodes describing extra properties
of particular PCI devices, ie. a PHY attached to an EHCI controller
on a PCI bus. This patch parses those DT subnodes and assigns a node
to the PCI device instance, so that the driver can extract details
from that node and ie. configure the PHY using the PHY subsystem.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@konsulko.com>
---
V2: Move the whole machinery to pci_bind_bus_devices(), right after
    the driver instance platform data are updated. This reduces the
    number of times the DT is traversed and works for both DT nodes
    with and without compat string.
---
 drivers/pci/pci-uclass.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
index e9671d9b76..cf3e38a6f2 100644
--- a/drivers/pci/pci-uclass.c
+++ b/drivers/pci/pci-uclass.c
@@ -733,6 +733,7 @@ int pci_bind_bus_devices(struct udevice *bus)
 	ulong vendor, device;
 	ulong header_type;
 	pci_dev_t bdf, end;
+	ofnode node;
 	bool found_multi;
 	int ret;
 
@@ -803,6 +804,20 @@ int pci_bind_bus_devices(struct udevice *bus)
 		pplat->vendor = vendor;
 		pplat->device = device;
 		pplat->class = class;
+
+		/* Associate potention OF node */
+		dev_for_each_subnode(node, bus) {
+			phys_addr_t df, size;
+			df = ofnode_get_addr_size(node, "reg", &size);
+			if (df == FDT_ADDR_T_NONE)
+				continue;
+
+			if (PCI_FUNC(df) == PCI_FUNC(bdf) &&
+			    PCI_DEV(df) == PCI_DEV(bdf)) {
+				dev->node = node;
+				break;
+			}
+		}
 	}
 
 	return 0;
-- 
2.16.2

[U-Boot] [PATCH V2 2/2] pci: Update documentation to make 'compatible' string optional

[Marek Vasut]

Reword the documentation to make it clear the compatible string is now
optional, yet still matching on it takes precedence over PCI IDs and
PCI classes.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@konsulko.com>
---
V2: New patch
---
 doc/driver-model/pci-info.txt | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/doc/driver-model/pci-info.txt b/doc/driver-model/pci-info.txt
index e1701d1fbc..14364c5c75 100644
--- a/doc/driver-model/pci-info.txt
+++ b/doc/driver-model/pci-info.txt
@@ -34,11 +34,15 @@ under that bus.
 Note that this is all done on a lazy basis, as needed, so until something is
 touched on PCI (eg: a call to pci_find_devices()) it will not be probed.
 
-PCI devices can appear in the flattened device tree. If they do this serves to
-specify the driver to use for the device. In this case they will be bound at
-first. Each PCI device node must have a compatible string list as well as a
-<reg> property, as defined by the IEEE Std 1275-1994 PCI bus binding document
-v2.1. Note we must describe PCI devices with the same bus hierarchy as the
+PCI devices can appear in the flattened device tree. If they do, their node
+often contains extra information which cannot be derived from the PCI IDs or
+PCI class of the device. Each PCI device node must have a <reg> property, as
+defined by the IEEE Std 1275-1994 PCI bus binding document v2.1. Compatible
+string list is optional and generally not needed, since PCI is discoverable
+bus, albeit there are justified exceptions. If the compatible string is
+present, matching on it takes precedence over PCI IDs and PCI classes.
+
+Note we must describe PCI devices with the same bus hierarchy as the
 hardware, otherwise driver model cannot detect the correct parent/children
 relationship during PCI bus enumeration thus PCI devices won't be bound to
 their drivers accordingly. A working example like below:
-- 
2.16.2

[U-Boot] [PATCH V2 1/2] pci: Support parsing PCI controller DT subnodes

[Marek Vasut]

On 08/24/2018 08:27 PM, Marek Vasut wrote:
> The PCI controller can have DT subnodes describing extra properties
> of particular PCI devices, ie. a PHY attached to an EHCI controller
> on a PCI bus. This patch parses those DT subnodes and assigns a node
> to the PCI device instance, so that the driver can extract details
> from that node and ie. configure the PHY using the PHY subsystem.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@konsulko.com>

Well, bump ?

This is the only missing patch to get my hardware working properly.

> ---
> V2: Move the whole machinery to pci_bind_bus_devices(), right after
>     the driver instance platform data are updated. This reduces the
>     number of times the DT is traversed and works for both DT nodes
>     with and without compat string.
> ---
>  drivers/pci/pci-uclass.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
> index e9671d9b76..cf3e38a6f2 100644
> --- a/drivers/pci/pci-uclass.c
> +++ b/drivers/pci/pci-uclass.c
> @@ -733,6 +733,7 @@ int pci_bind_bus_devices(struct udevice *bus)
>  	ulong vendor, device;
>  	ulong header_type;
>  	pci_dev_t bdf, end;
> +	ofnode node;
>  	bool found_multi;
>  	int ret;
>  
> @@ -803,6 +804,20 @@ int pci_bind_bus_devices(struct udevice *bus)
>  		pplat->vendor = vendor;
>  		pplat->device = device;
>  		pplat->class = class;
> +
> +		/* Associate potention OF node */
> +		dev_for_each_subnode(node, bus) {
> +			phys_addr_t df, size;
> +			df = ofnode_get_addr_size(node, "reg", &size);
> +			if (df == FDT_ADDR_T_NONE)
> +				continue;
> +
> +			if (PCI_FUNC(df) == PCI_FUNC(bdf) &&
> +			    PCI_DEV(df) == PCI_DEV(bdf)) {
> +				dev->node = node;
> +				break;
> +			}
> +		}
>  	}
>  
>  	return 0;
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH V2 1/2] pci: Support parsing PCI controller DT subnodes

[Bin Meng]

+Simon

Hi Marek,

On Wed, Aug 29, 2018 at 10:22 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>
> On 08/24/2018 08:27 PM, Marek Vasut wrote:
> > The PCI controller can have DT subnodes describing extra properties
> > of particular PCI devices, ie. a PHY attached to an EHCI controller
> > on a PCI bus. This patch parses those DT subnodes and assigns a node
> > to the PCI device instance, so that the driver can extract details
> > from that node and ie. configure the PHY using the PHY subsystem.
> >
> > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> > Cc: Simon Glass <sjg@chromium.org>
> > Cc: Tom Rini <trini@konsulko.com>
>
> Well, bump ?
>
> This is the only missing patch to get my hardware working properly.

I don't think we ever had an agreement on the v1 patch. Simon had a
long email that pointed out what Linux does seems like a 'fallback' to
find a node with no compatible string.

Back to this, if we have to go with this way, please create a test
case to cover this scenario.

Regards,
Bin

[U-Boot] [PATCH V2 1/2] pci: Support parsing PCI controller DT subnodes

[Marek Vasut]

On 08/29/2018 05:15 PM, Bin Meng wrote:
> +Simon
> 
> Hi Marek,
> 
> On Wed, Aug 29, 2018 at 10:22 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>
>> On 08/24/2018 08:27 PM, Marek Vasut wrote:
>>> The PCI controller can have DT subnodes describing extra properties
>>> of particular PCI devices, ie. a PHY attached to an EHCI controller
>>> on a PCI bus. This patch parses those DT subnodes and assigns a node
>>> to the PCI device instance, so that the driver can extract details
>>> from that node and ie. configure the PHY using the PHY subsystem.
>>>
>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>> Cc: Simon Glass <sjg@chromium.org>
>>> Cc: Tom Rini <trini@konsulko.com>
>>
>> Well, bump ?
>>
>> This is the only missing patch to get my hardware working properly.
> 
> I don't think we ever had an agreement on the v1 patch. Simon had a
> long email that pointed out what Linux does seems like a 'fallback' to
> find a node with no compatible string.
> 
> Back to this, if we have to go with this way, please create a test
> case to cover this scenario.

The fact that it works on a particular board is not tested enough?
Do we need a custom, special, synthetic test ?

Anyway, any feedback on the patch ? Did you test it ? I again only see
"do this random stuff and that random stuff" , but zero actual feedback.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH V2 1/2] pci: Support parsing PCI controller DT subnodes

[Alexander Graf]

On 29.08.18 19:07, Marek Vasut wrote:
> On 08/29/2018 05:15 PM, Bin Meng wrote:
>> +Simon
>>
>> Hi Marek,
>>
>> On Wed, Aug 29, 2018 at 10:22 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>
>>> On 08/24/2018 08:27 PM, Marek Vasut wrote:
>>>> The PCI controller can have DT subnodes describing extra properties
>>>> of particular PCI devices, ie. a PHY attached to an EHCI controller
>>>> on a PCI bus. This patch parses those DT subnodes and assigns a node
>>>> to the PCI device instance, so that the driver can extract details
>>>> from that node and ie. configure the PHY using the PHY subsystem.
>>>>
>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>> Cc: Simon Glass <sjg@chromium.org>
>>>> Cc: Tom Rini <trini@konsulko.com>
>>>
>>> Well, bump ?
>>>
>>> This is the only missing patch to get my hardware working properly.
>>
>> I don't think we ever had an agreement on the v1 patch. Simon had a
>> long email that pointed out what Linux does seems like a 'fallback' to
>> find a node with no compatible string.
>>
>> Back to this, if we have to go with this way, please create a test
>> case to cover this scenario.
> 
> The fact that it works on a particular board is not tested enough?
> Do we need a custom, special, synthetic test ?

I agree that we should include test coverage for this case in some
automated fashion, so that nobody breaks support for it by accident.

What's the easiest way to get us there?

> Anyway, any feedback on the patch ? Did you test it ? I again only see
> "do this random stuff and that random stuff" , but zero actual feedback.

I think "potention" is a typo in your patch. Apart from that the logic
feels very sound to me though. It's perfectly valid in DT to refer to
subdevices by reg only.


Alex

[U-Boot] [PATCH V2 2/2] pci: Update documentation to make 'compatible' string optional

[Simon Glass]

Hi Marek,

On 24 August 2018 at 12:27, Marek Vasut <marek.vasut@gmail.com> wrote:
> Reword the documentation to make it clear the compatible string is now
> optional, yet still matching on it takes precedence over PCI IDs and
> PCI classes.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@konsulko.com>
> ---
> V2: New patch
> ---
>  doc/driver-model/pci-info.txt | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/doc/driver-model/pci-info.txt b/doc/driver-model/pci-info.txt
> index e1701d1fbc..14364c5c75 100644
> --- a/doc/driver-model/pci-info.txt
> +++ b/doc/driver-model/pci-info.txt
> @@ -34,11 +34,15 @@ under that bus.
>  Note that this is all done on a lazy basis, as needed, so until something is
>  touched on PCI (eg: a call to pci_find_devices()) it will not be probed.
>
> -PCI devices can appear in the flattened device tree. If they do this serves to
> -specify the driver to use for the device. In this case they will be bound at
> -first. Each PCI device node must have a compatible string list as well as a
> -<reg> property, as defined by the IEEE Std 1275-1994 PCI bus binding document
> -v2.1. Note we must describe PCI devices with the same bus hierarchy as the
> +PCI devices can appear in the flattened device tree. If they do, their node
> +often contains extra information which cannot be derived from the PCI IDs or
> +PCI class of the device. Each PCI device node must have a <reg> property, as
> +defined by the IEEE Std 1275-1994 PCI bus binding document v2.1. Compatible
> +string list is optional and generally not needed, since PCI is discoverable
> +bus, albeit there are justified exceptions. If the compatible string is
> +present, matching on it takes precedence over PCI IDs and PCI classes.
> +
> +Note we must describe PCI devices with the same bus hierarchy as the
>  hardware, otherwise driver model cannot detect the correct parent/children
>  relationship during PCI bus enumeration thus PCI devices won't be bound to
>  their drivers accordingly. A working example like below:
> --
> 2.16.2
>

Are we really saying that compatible strings are 'generally not needed'?

device tree pci supplement 2.1 talks about some new formats for the
compatible string, but doesn't say it is not needed. I much prefer a
compatible string since it is easy to find the driver in the source
code.

Can way say that a compatible string is preferred, but in extremis you
can avoid it by...


Regards,
Simon

[U-Boot] [PATCH V2 2/2] pci: Update documentation to make 'compatible' string optional

[Marek Vasut]

On 08/30/2018 02:29 AM, Simon Glass wrote:
> Hi Marek,

Hi,

> On 24 August 2018 at 12:27, Marek Vasut <marek.vasut@gmail.com> wrote:
>> Reword the documentation to make it clear the compatible string is now
>> optional, yet still matching on it takes precedence over PCI IDs and
>> PCI classes.
>>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Tom Rini <trini@konsulko.com>
>> ---
>> V2: New patch
>> ---
>>  doc/driver-model/pci-info.txt | 14 +++++++++-----
>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/doc/driver-model/pci-info.txt b/doc/driver-model/pci-info.txt
>> index e1701d1fbc..14364c5c75 100644
>> --- a/doc/driver-model/pci-info.txt
>> +++ b/doc/driver-model/pci-info.txt
>> @@ -34,11 +34,15 @@ under that bus.
>>  Note that this is all done on a lazy basis, as needed, so until something is
>>  touched on PCI (eg: a call to pci_find_devices()) it will not be probed.
>>
>> -PCI devices can appear in the flattened device tree. If they do this serves to
>> -specify the driver to use for the device. In this case they will be bound at
>> -first. Each PCI device node must have a compatible string list as well as a
>> -<reg> property, as defined by the IEEE Std 1275-1994 PCI bus binding document
>> -v2.1. Note we must describe PCI devices with the same bus hierarchy as the
>> +PCI devices can appear in the flattened device tree. If they do, their node
>> +often contains extra information which cannot be derived from the PCI IDs or
>> +PCI class of the device. Each PCI device node must have a <reg> property, as
>> +defined by the IEEE Std 1275-1994 PCI bus binding document v2.1. Compatible
>> +string list is optional and generally not needed, since PCI is discoverable
>> +bus, albeit there are justified exceptions. If the compatible string is
>> +present, matching on it takes precedence over PCI IDs and PCI classes.
>> +
>> +Note we must describe PCI devices with the same bus hierarchy as the
>>  hardware, otherwise driver model cannot detect the correct parent/children
>>  relationship during PCI bus enumeration thus PCI devices won't be bound to
>>  their drivers accordingly. A working example like below:
>> --
>> 2.16.2
>>
> 
> Are we really saying that compatible strings are 'generally not needed'?

Yes, PCI is a discoverable bus.

> device tree pci supplement 2.1 talks about some new formats for the
> compatible string, but doesn't say it is not needed. I much prefer a
> compatible string since it is easy to find the driver in the source
> code.

But it duplicates (badly) what the PCI IDs and classes are used for
since PCI's inception.

> Can way say that a compatible string is preferred, but in extremis you
> can avoid it by...

No, see above, PCI is discoverable by design.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH V2 1/2] pci: Support parsing PCI controller DT subnodes

[Bin Meng]

Hi Marek,

On Thu, Aug 30, 2018 at 1:07 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>
> On 08/29/2018 05:15 PM, Bin Meng wrote:
> > +Simon
> >
> > Hi Marek,
> >
> > On Wed, Aug 29, 2018 at 10:22 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>
> >> On 08/24/2018 08:27 PM, Marek Vasut wrote:
> >>> The PCI controller can have DT subnodes describing extra properties
> >>> of particular PCI devices, ie. a PHY attached to an EHCI controller
> >>> on a PCI bus. This patch parses those DT subnodes and assigns a node
> >>> to the PCI device instance, so that the driver can extract details
> >>> from that node and ie. configure the PHY using the PHY subsystem.
> >>>
> >>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> >>> Cc: Simon Glass <sjg@chromium.org>
> >>> Cc: Tom Rini <trini@konsulko.com>
> >>
> >> Well, bump ?
> >>
> >> This is the only missing patch to get my hardware working properly.
> >
> > I don't think we ever had an agreement on the v1 patch. Simon had a
> > long email that pointed out what Linux does seems like a 'fallback' to
> > find a node with no compatible string.
> >
> > Back to this, if we have to go with this way, please create a test
> > case to cover this scenario.
>
> The fact that it works on a particular board is not tested enough?
> Do we need a custom, special, synthetic test ?
>

I believe that's always been the requirement against the DM code
changes. I was requested in the past when I changed something in the
DM and I see other people were asked to do so. Like Alex said, it does
not mean this patch was not tested enough, but to ensure future
commits won't break this.

> Anyway, any feedback on the patch ? Did you test it ? I again only see
> "do this random stuff and that random stuff" , but zero actual feedback.
>

If "this and that random stuff" means test case I asked for, please
check my proposal on the v1 patch thread which indicated that a proper
test case should be created. You seems to have missed that.

Regards,
Bin

[U-Boot] [PATCH V2 1/2] pci: Support parsing PCI controller DT subnodes

[Marek Vasut]

On 08/30/2018 03:32 PM, Bin Meng wrote:
> Hi Marek,
> 
> On Thu, Aug 30, 2018 at 1:07 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>>
>> On 08/29/2018 05:15 PM, Bin Meng wrote:
>>> +Simon
>>>
>>> Hi Marek,
>>>
>>> On Wed, Aug 29, 2018 at 10:22 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>
>>>> On 08/24/2018 08:27 PM, Marek Vasut wrote:
>>>>> The PCI controller can have DT subnodes describing extra properties
>>>>> of particular PCI devices, ie. a PHY attached to an EHCI controller
>>>>> on a PCI bus. This patch parses those DT subnodes and assigns a node
>>>>> to the PCI device instance, so that the driver can extract details
>>>>> from that node and ie. configure the PHY using the PHY subsystem.
>>>>>
>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>
>>>> Well, bump ?
>>>>
>>>> This is the only missing patch to get my hardware working properly.
>>>
>>> I don't think we ever had an agreement on the v1 patch. Simon had a
>>> long email that pointed out what Linux does seems like a 'fallback' to
>>> find a node with no compatible string.
>>>
>>> Back to this, if we have to go with this way, please create a test
>>> case to cover this scenario.
>>
>> The fact that it works on a particular board is not tested enough?
>> Do we need a custom, special, synthetic test ?
>>
> 
> I believe that's always been the requirement against the DM code
> changes. I was requested in the past when I changed something in the
> DM and I see other people were asked to do so. Like Alex said, it does
> not mean this patch was not tested enough, but to ensure future
> commits won't break this.

So, do you have any suggestion how to implement this test ? It seems
Alex posed the same question. It doesn't seem to be trivial in the
context of sandbox.

>> Anyway, any feedback on the patch ? Did you test it ? I again only see
>> "do this random stuff and that random stuff" , but zero actual feedback.
>>
> 
> If "this and that random stuff" means test case I asked for, please
> check my proposal on the v1 patch thread which indicated that a proper
> test case should be created. You seems to have missed that.

So, any feedback on this actual patch ?

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH V2 2/2] pci: Update documentation to make 'compatible' string optional

[Simon Glass]

Hi Marek,

On 30 August 2018 at 04:20, Marek Vasut <marek.vasut@gmail.com> wrote:
> On 08/30/2018 02:29 AM, Simon Glass wrote:
>> Hi Marek,
>
> Hi,
>
>> On 24 August 2018 at 12:27, Marek Vasut <marek.vasut@gmail.com> wrote:
>>> Reword the documentation to make it clear the compatible string is now
>>> optional, yet still matching on it takes precedence over PCI IDs and
>>> PCI classes.
>>>
>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>> Cc: Simon Glass <sjg@chromium.org>
>>> Cc: Tom Rini <trini@konsulko.com>
>>> ---
>>> V2: New patch
>>> ---
>>>  doc/driver-model/pci-info.txt | 14 +++++++++-----
>>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/doc/driver-model/pci-info.txt b/doc/driver-model/pci-info.txt
>>> index e1701d1fbc..14364c5c75 100644
>>> --- a/doc/driver-model/pci-info.txt
>>> +++ b/doc/driver-model/pci-info.txt
>>> @@ -34,11 +34,15 @@ under that bus.
>>>  Note that this is all done on a lazy basis, as needed, so until something is
>>>  touched on PCI (eg: a call to pci_find_devices()) it will not be probed.
>>>
>>> -PCI devices can appear in the flattened device tree. If they do this serves to
>>> -specify the driver to use for the device. In this case they will be bound at
>>> -first. Each PCI device node must have a compatible string list as well as a
>>> -<reg> property, as defined by the IEEE Std 1275-1994 PCI bus binding document
>>> -v2.1. Note we must describe PCI devices with the same bus hierarchy as the
>>> +PCI devices can appear in the flattened device tree. If they do, their node
>>> +often contains extra information which cannot be derived from the PCI IDs or
>>> +PCI class of the device. Each PCI device node must have a <reg> property, as
>>> +defined by the IEEE Std 1275-1994 PCI bus binding document v2.1. Compatible
>>> +string list is optional and generally not needed, since PCI is discoverable
>>> +bus, albeit there are justified exceptions. If the compatible string is
>>> +present, matching on it takes precedence over PCI IDs and PCI classes.
>>> +
>>> +Note we must describe PCI devices with the same bus hierarchy as the
>>>  hardware, otherwise driver model cannot detect the correct parent/children
>>>  relationship during PCI bus enumeration thus PCI devices won't be bound to
>>>  their drivers accordingly. A working example like below:
>>> --
>>> 2.16.2
>>>
>>
>> Are we really saying that compatible strings are 'generally not needed'?
>
> Yes, PCI is a discoverable bus.
>
>> device tree pci supplement 2.1 talks about some new formats for the
>> compatible string, but doesn't say it is not needed. I much prefer a
>> compatible string since it is easy to find the driver in the source
>> code.
>
> But it duplicates (badly) what the PCI IDs and classes are used for
> since PCI's inception.
>
>> Can way say that a compatible string is preferred, but in extremis you
>> can avoid it by...
>
> No, see above, PCI is discoverable by design.

I feel that these two things are orthogonal.

You can probe the bus and find a device. That is the 'discoverable' part.

But it is not automatically configurable. If it it were, there would
be no DT node and no settings in the DT for the device. But from your
patch, in some cases we need more information, and the DT node
provides that.

So to get the settings to pass to the driver, you have to find the
device-tree node to use for the device. The only way U-Boot supports
is to use the 'reg' property, which specifies the PCI address. (We
don't support a compatible string starting with "pci...". We could
support that, but it is more code for essentially the same purpose.)

So we are not talking about the discoverability, which is already
supported by U-Boot. We are talking about the configuration of the
device, via settings passed to the driver.

In fact the only issue here is whether to require a compatible string
for PCI nodes or allow matching solely based on the 'reg' property. Is
the latter widely used in Linux? Presumably not on x86, which doesn't
even use DT.

Regards,
Simon

[U-Boot] [PATCH V2 1/2] pci: Support parsing PCI controller DT subnodes

[Simon Glass]

Hi Marek,

On 30 August 2018 at 07:42, Marek Vasut <marek.vasut@gmail.com> wrote:
> On 08/30/2018 03:32 PM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Thu, Aug 30, 2018 at 1:07 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>
>>> On 08/29/2018 05:15 PM, Bin Meng wrote:
>>>> +Simon
>>>>
>>>> Hi Marek,
>>>>
>>>> On Wed, Aug 29, 2018 at 10:22 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>
>>>>> On 08/24/2018 08:27 PM, Marek Vasut wrote:
>>>>>> The PCI controller can have DT subnodes describing extra properties
>>>>>> of particular PCI devices, ie. a PHY attached to an EHCI controller
>>>>>> on a PCI bus. This patch parses those DT subnodes and assigns a node
>>>>>> to the PCI device instance, so that the driver can extract details
>>>>>> from that node and ie. configure the PHY using the PHY subsystem.
>>>>>>
>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>
>>>>> Well, bump ?
>>>>>
>>>>> This is the only missing patch to get my hardware working properly.
>>>>
>>>> I don't think we ever had an agreement on the v1 patch. Simon had a
>>>> long email that pointed out what Linux does seems like a 'fallback' to
>>>> find a node with no compatible string.
>>>>
>>>> Back to this, if we have to go with this way, please create a test
>>>> case to cover this scenario.
>>>
>>> The fact that it works on a particular board is not tested enough?
>>> Do we need a custom, special, synthetic test ?
>>>
>>
>> I believe that's always been the requirement against the DM code
>> changes. I was requested in the past when I changed something in the
>> DM and I see other people were asked to do so. Like Alex said, it does
>> not mean this patch was not tested enough, but to ensure future
>> commits won't break this.
>
> So, do you have any suggestion how to implement this test ? It seems
> Alex posed the same question. It doesn't seem to be trivial in the
> context of sandbox.

I suppose you need a PCI_DEVICE() declaration for sandbox, with an
associated DT node and no compatible string. Then check that you can
locate the device and that it read a DT property correctly.

>
>>> Anyway, any feedback on the patch ? Did you test it ? I again only see
>>> "do this random stuff and that random stuff" , but zero actual feedback.
>>>
>>
>> If "this and that random stuff" means test case I asked for, please
>> check my proposal on the v1 patch thread which indicated that a proper
>> test case should be created. You seems to have missed that.
>
> So, any feedback on this actual patch ?

What is 'potention'?

Is there any check needed that it does not attach the same DT node to
two different devices? Or perhaps that cannot happen, since we
shouldn't expect two nodes to share a BDF?

I think it looks OK, assuming this is the way we want to go.

Regards,
Simon

[U-Boot] [PATCH V2 2/2] pci: Update documentation to make 'compatible' string optional

[Marek Vasut]

On 09/01/2018 11:45 PM, Simon Glass wrote:
> Hi Marek,
> 
> On 30 August 2018 at 04:20, Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 08/30/2018 02:29 AM, Simon Glass wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>> On 24 August 2018 at 12:27, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>> Reword the documentation to make it clear the compatible string is now
>>>> optional, yet still matching on it takes precedence over PCI IDs and
>>>> PCI classes.
>>>>
>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>> Cc: Simon Glass <sjg@chromium.org>
>>>> Cc: Tom Rini <trini@konsulko.com>
>>>> ---
>>>> V2: New patch
>>>> ---
>>>>  doc/driver-model/pci-info.txt | 14 +++++++++-----
>>>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/doc/driver-model/pci-info.txt b/doc/driver-model/pci-info.txt
>>>> index e1701d1fbc..14364c5c75 100644
>>>> --- a/doc/driver-model/pci-info.txt
>>>> +++ b/doc/driver-model/pci-info.txt
>>>> @@ -34,11 +34,15 @@ under that bus.
>>>>  Note that this is all done on a lazy basis, as needed, so until something is
>>>>  touched on PCI (eg: a call to pci_find_devices()) it will not be probed.
>>>>
>>>> -PCI devices can appear in the flattened device tree. If they do this serves to
>>>> -specify the driver to use for the device. In this case they will be bound at
>>>> -first. Each PCI device node must have a compatible string list as well as a
>>>> -<reg> property, as defined by the IEEE Std 1275-1994 PCI bus binding document
>>>> -v2.1. Note we must describe PCI devices with the same bus hierarchy as the
>>>> +PCI devices can appear in the flattened device tree. If they do, their node
>>>> +often contains extra information which cannot be derived from the PCI IDs or
>>>> +PCI class of the device. Each PCI device node must have a <reg> property, as
>>>> +defined by the IEEE Std 1275-1994 PCI bus binding document v2.1. Compatible
>>>> +string list is optional and generally not needed, since PCI is discoverable
>>>> +bus, albeit there are justified exceptions. If the compatible string is
>>>> +present, matching on it takes precedence over PCI IDs and PCI classes.
>>>> +
>>>> +Note we must describe PCI devices with the same bus hierarchy as the
>>>>  hardware, otherwise driver model cannot detect the correct parent/children
>>>>  relationship during PCI bus enumeration thus PCI devices won't be bound to
>>>>  their drivers accordingly. A working example like below:
>>>> --
>>>> 2.16.2
>>>>
>>>
>>> Are we really saying that compatible strings are 'generally not needed'?
>>
>> Yes, PCI is a discoverable bus.
>>
>>> device tree pci supplement 2.1 talks about some new formats for the
>>> compatible string, but doesn't say it is not needed. I much prefer a
>>> compatible string since it is easy to find the driver in the source
>>> code.
>>
>> But it duplicates (badly) what the PCI IDs and classes are used for
>> since PCI's inception.
>>
>>> Can way say that a compatible string is preferred, but in extremis you
>>> can avoid it by...
>>
>> No, see above, PCI is discoverable by design.
> 
> I feel that these two things are orthogonal.
> 
> You can probe the bus and find a device. That is the 'discoverable' part.
> 
> But it is not automatically configurable. If it it were, there would
> be no DT node and no settings in the DT for the device. But from your
> patch, in some cases we need more information, and the DT node
> provides that.

Pretty much, you can have stuff on the PCI card which needs extra info.

> So to get the settings to pass to the driver, you have to find the
> device-tree node to use for the device. The only way U-Boot supports
> is to use the 'reg' property, which specifies the PCI address. (We
> don't support a compatible string starting with "pci...". We could
> support that, but it is more code for essentially the same purpose.)

Yes

> So we are not talking about the discoverability, which is already
> supported by U-Boot. We are talking about the configuration of the
> device, via settings passed to the driver.

Yes

> In fact the only issue here is whether to require a compatible string
> for PCI nodes or allow matching solely based on the 'reg' property. Is
> the latter widely used in Linux? Presumably not on x86, which doesn't
> even use DT.

I only see the compatible string used for bridges, the rest of the
subdevices match on reg property.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH V2 1/2] pci: Support parsing PCI controller DT subnodes

[Marek Vasut]

On 09/01/2018 11:50 PM, Simon Glass wrote:
> Hi Marek,
> 
> On 30 August 2018 at 07:42, Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 08/30/2018 03:32 PM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Thu, Aug 30, 2018 at 1:07 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>
>>>> On 08/29/2018 05:15 PM, Bin Meng wrote:
>>>>> +Simon
>>>>>
>>>>> Hi Marek,
>>>>>
>>>>> On Wed, Aug 29, 2018 at 10:22 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>
>>>>>> On 08/24/2018 08:27 PM, Marek Vasut wrote:
>>>>>>> The PCI controller can have DT subnodes describing extra properties
>>>>>>> of particular PCI devices, ie. a PHY attached to an EHCI controller
>>>>>>> on a PCI bus. This patch parses those DT subnodes and assigns a node
>>>>>>> to the PCI device instance, so that the driver can extract details
>>>>>>> from that node and ie. configure the PHY using the PHY subsystem.
>>>>>>>
>>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>>
>>>>>> Well, bump ?
>>>>>>
>>>>>> This is the only missing patch to get my hardware working properly.
>>>>>
>>>>> I don't think we ever had an agreement on the v1 patch. Simon had a
>>>>> long email that pointed out what Linux does seems like a 'fallback' to
>>>>> find a node with no compatible string.
>>>>>
>>>>> Back to this, if we have to go with this way, please create a test
>>>>> case to cover this scenario.
>>>>
>>>> The fact that it works on a particular board is not tested enough?
>>>> Do we need a custom, special, synthetic test ?
>>>>
>>>
>>> I believe that's always been the requirement against the DM code
>>> changes. I was requested in the past when I changed something in the
>>> DM and I see other people were asked to do so. Like Alex said, it does
>>> not mean this patch was not tested enough, but to ensure future
>>> commits won't break this.
>>
>> So, do you have any suggestion how to implement this test ? It seems
>> Alex posed the same question. It doesn't seem to be trivial in the
>> context of sandbox.
> 
> I suppose you need a PCI_DEVICE() declaration for sandbox, with an
> associated DT node and no compatible string. Then check that you can
> locate the device and that it read a DT property correctly.

Is there any example of this stuff already ?

>>>> Anyway, any feedback on the patch ? Did you test it ? I again only see
>>>> "do this random stuff and that random stuff" , but zero actual feedback.
>>>>
>>>
>>> If "this and that random stuff" means test case I asked for, please
>>> check my proposal on the v1 patch thread which indicated that a proper
>>> test case should be created. You seems to have missed that.
>>
>> So, any feedback on this actual patch ?
> 
> What is 'potention'?

potential typo .

> Is there any check needed that it does not attach the same DT node to
> two different devices? Or perhaps that cannot happen, since we
> shouldn't expect two nodes to share a BDF?

I guess it could happen and I didn't find a good solution to this even
in Linux. The current take on this possibility seems to be "let's live
with it".

> I think it looks OK, assuming this is the way we want to go.
> 
> Regards,
> Simon
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH V2 1/2] pci: Support parsing PCI controller DT subnodes

[Simon Glass]

Hi Marek,

On 1 September 2018 at 16:45, Marek Vasut <marek.vasut@gmail.com> wrote:
>
> On 09/01/2018 11:50 PM, Simon Glass wrote:
> > Hi Marek,
> >
> > On 30 August 2018 at 07:42, Marek Vasut <marek.vasut@gmail.com> wrote:
> >> On 08/30/2018 03:32 PM, Bin Meng wrote:
> >>> Hi Marek,
> >>>
> >>> On Thu, Aug 30, 2018 at 1:07 AM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>
> >>>> On 08/29/2018 05:15 PM, Bin Meng wrote:
> >>>>> +Simon
> >>>>>
> >>>>> Hi Marek,
> >>>>>
> >>>>> On Wed, Aug 29, 2018 at 10:22 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>
> >>>>>> On 08/24/2018 08:27 PM, Marek Vasut wrote:
> >>>>>>> The PCI controller can have DT subnodes describing extra properties
> >>>>>>> of particular PCI devices, ie. a PHY attached to an EHCI controller
> >>>>>>> on a PCI bus. This patch parses those DT subnodes and assigns a node
> >>>>>>> to the PCI device instance, so that the driver can extract details
> >>>>>>> from that node and ie. configure the PHY using the PHY subsystem.
> >>>>>>>
> >>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> >>>>>>> Cc: Simon Glass <sjg@chromium.org>
> >>>>>>> Cc: Tom Rini <trini@konsulko.com>
> >>>>>>
> >>>>>> Well, bump ?
> >>>>>>
> >>>>>> This is the only missing patch to get my hardware working properly.
> >>>>>
> >>>>> I don't think we ever had an agreement on the v1 patch. Simon had a
> >>>>> long email that pointed out what Linux does seems like a 'fallback' to
> >>>>> find a node with no compatible string.
> >>>>>
> >>>>> Back to this, if we have to go with this way, please create a test
> >>>>> case to cover this scenario.
> >>>>
> >>>> The fact that it works on a particular board is not tested enough?
> >>>> Do we need a custom, special, synthetic test ?
> >>>>
> >>>
> >>> I believe that's always been the requirement against the DM code
> >>> changes. I was requested in the past when I changed something in the
> >>> DM and I see other people were asked to do so. Like Alex said, it does
> >>> not mean this patch was not tested enough, but to ensure future
> >>> commits won't break this.
> >>
> >> So, do you have any suggestion how to implement this test ? It seems
> >> Alex posed the same question. It doesn't seem to be trivial in the
> >> context of sandbox.
> >
> > I suppose you need a PCI_DEVICE() declaration for sandbox, with an
> > associated DT node and no compatible string. Then check that you can
> > locate the device and that it read a DT property correctly.
>
> Is there any example of this stuff already ?

See the bottom of swap_case.c. You might be able to add a new one of those,

If you look at pci-controller2 in test.dts it has a device with a
compatible string. You could try adding a second device with no
compatible string.

>
> >>>> Anyway, any feedback on the patch ? Did you test it ? I again only see
> >>>> "do this random stuff and that random stuff" , but zero actual feedback.
> >>>>
> >>>
> >>> If "this and that random stuff" means test case I asked for, please
> >>> check my proposal on the v1 patch thread which indicated that a proper
> >>> test case should be created. You seems to have missed that.
> >>
> >> So, any feedback on this actual patch ?
> >
> > What is 'potention'?
>
> potential typo .
>
> > Is there any check needed that it does not attach the same DT node to
> > two different devices? Or perhaps that cannot happen, since we
> > shouldn't expect two nodes to share a BDF?
>
> I guess it could happen and I didn't find a good solution to this even
> in Linux. The current take on this possibility seems to be "let's live
> with it".

OK.

>
> > I think it looks OK, assuming this is the way we want to go.
> >
> > Regards,
> > Simon
> >
>
>
> --
> Best regards,
> Marek Vasut

Regards,
Simon

[U-Boot] [PATCH V2 2/2] pci: Update documentation to make 'compatible' string optional

[Simon Glass]

Hi,

On 1 September 2018 at 16:41, Marek Vasut <marek.vasut@gmail.com> wrote:
> On 09/01/2018 11:45 PM, Simon Glass wrote:
>> Hi Marek,
>>
>> On 30 August 2018 at 04:20, Marek Vasut <marek.vasut@gmail.com> wrote:
>>> On 08/30/2018 02:29 AM, Simon Glass wrote:
>>>> Hi Marek,
>>>
>>> Hi,
>>>
>>>> On 24 August 2018 at 12:27, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>> Reword the documentation to make it clear the compatible string is now
>>>>> optional, yet still matching on it takes precedence over PCI IDs and
>>>>> PCI classes.
>>>>>
>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>> ---
>>>>> V2: New patch
>>>>> ---
>>>>>  doc/driver-model/pci-info.txt | 14 +++++++++-----
>>>>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/doc/driver-model/pci-info.txt b/doc/driver-model/pci-info.txt
>>>>> index e1701d1fbc..14364c5c75 100644
>>>>> --- a/doc/driver-model/pci-info.txt
>>>>> +++ b/doc/driver-model/pci-info.txt
>>>>> @@ -34,11 +34,15 @@ under that bus.
>>>>>  Note that this is all done on a lazy basis, as needed, so until something is
>>>>>  touched on PCI (eg: a call to pci_find_devices()) it will not be probed.
>>>>>
>>>>> -PCI devices can appear in the flattened device tree. If they do this serves to
>>>>> -specify the driver to use for the device. In this case they will be bound at
>>>>> -first. Each PCI device node must have a compatible string list as well as a
>>>>> -<reg> property, as defined by the IEEE Std 1275-1994 PCI bus binding document
>>>>> -v2.1. Note we must describe PCI devices with the same bus hierarchy as the
>>>>> +PCI devices can appear in the flattened device tree. If they do, their node
>>>>> +often contains extra information which cannot be derived from the PCI IDs or
>>>>> +PCI class of the device. Each PCI device node must have a <reg> property, as
>>>>> +defined by the IEEE Std 1275-1994 PCI bus binding document v2.1. Compatible
>>>>> +string list is optional and generally not needed, since PCI is discoverable
>>>>> +bus, albeit there are justified exceptions. If the compatible string is
>>>>> +present, matching on it takes precedence over PCI IDs and PCI classes.
>>>>> +
>>>>> +Note we must describe PCI devices with the same bus hierarchy as the
>>>>>  hardware, otherwise driver model cannot detect the correct parent/children
>>>>>  relationship during PCI bus enumeration thus PCI devices won't be bound to
>>>>>  their drivers accordingly. A working example like below:
>>>>> --
>>>>> 2.16.2
>>>>>
>>>>
>>>> Are we really saying that compatible strings are 'generally not needed'?
>>>
>>> Yes, PCI is a discoverable bus.
>>>
>>>> device tree pci supplement 2.1 talks about some new formats for the
>>>> compatible string, but doesn't say it is not needed. I much prefer a
>>>> compatible string since it is easy to find the driver in the source
>>>> code.
>>>
>>> But it duplicates (badly) what the PCI IDs and classes are used for
>>> since PCI's inception.
>>>
>>>> Can way say that a compatible string is preferred, but in extremis you
>>>> can avoid it by...
>>>
>>> No, see above, PCI is discoverable by design.
>>
>> I feel that these two things are orthogonal.
>>
>> You can probe the bus and find a device. That is the 'discoverable' part.
>>
>> But it is not automatically configurable. If it it were, there would
>> be no DT node and no settings in the DT for the device. But from your
>> patch, in some cases we need more information, and the DT node
>> provides that.
>
> Pretty much, you can have stuff on the PCI card which needs extra info.
>
>> So to get the settings to pass to the driver, you have to find the
>> device-tree node to use for the device. The only way U-Boot supports
>> is to use the 'reg' property, which specifies the PCI address. (We
>> don't support a compatible string starting with "pci...". We could
>> support that, but it is more code for essentially the same purpose.)
>
> Yes
>
>> So we are not talking about the discoverability, which is already
>> supported by U-Boot. We are talking about the configuration of the
>> device, via settings passed to the driver.
>
> Yes
>
>> In fact the only issue here is whether to require a compatible string
>> for PCI nodes or allow matching solely based on the 'reg' property. Is
>> the latter widely used in Linux? Presumably not on x86, which doesn't
>> even use DT.
>
> I only see the compatible string used for bridges, the rest of the
> subdevices match on reg property.

Where are you looking?

Regards,
Simon

[U-Boot] [PATCH V2 2/2] pci: Update documentation to make 'compatible' string optional

[Marek Vasut]

On 09/02/2018 03:07 AM, Simon Glass wrote:
> Hi,
> 
> On 1 September 2018 at 16:41, Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 09/01/2018 11:45 PM, Simon Glass wrote:
>>> Hi Marek,
>>>
>>> On 30 August 2018 at 04:20, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>> On 08/30/2018 02:29 AM, Simon Glass wrote:
>>>>> Hi Marek,
>>>>
>>>> Hi,
>>>>
>>>>> On 24 August 2018 at 12:27, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>> Reword the documentation to make it clear the compatible string is now
>>>>>> optional, yet still matching on it takes precedence over PCI IDs and
>>>>>> PCI classes.
>>>>>>
>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>> ---
>>>>>> V2: New patch
>>>>>> ---
>>>>>>  doc/driver-model/pci-info.txt | 14 +++++++++-----
>>>>>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/doc/driver-model/pci-info.txt b/doc/driver-model/pci-info.txt
>>>>>> index e1701d1fbc..14364c5c75 100644
>>>>>> --- a/doc/driver-model/pci-info.txt
>>>>>> +++ b/doc/driver-model/pci-info.txt
>>>>>> @@ -34,11 +34,15 @@ under that bus.
>>>>>>  Note that this is all done on a lazy basis, as needed, so until something is
>>>>>>  touched on PCI (eg: a call to pci_find_devices()) it will not be probed.
>>>>>>
>>>>>> -PCI devices can appear in the flattened device tree. If they do this serves to
>>>>>> -specify the driver to use for the device. In this case they will be bound at
>>>>>> -first. Each PCI device node must have a compatible string list as well as a
>>>>>> -<reg> property, as defined by the IEEE Std 1275-1994 PCI bus binding document
>>>>>> -v2.1. Note we must describe PCI devices with the same bus hierarchy as the
>>>>>> +PCI devices can appear in the flattened device tree. If they do, their node
>>>>>> +often contains extra information which cannot be derived from the PCI IDs or
>>>>>> +PCI class of the device. Each PCI device node must have a <reg> property, as
>>>>>> +defined by the IEEE Std 1275-1994 PCI bus binding document v2.1. Compatible
>>>>>> +string list is optional and generally not needed, since PCI is discoverable
>>>>>> +bus, albeit there are justified exceptions. If the compatible string is
>>>>>> +present, matching on it takes precedence over PCI IDs and PCI classes.
>>>>>> +
>>>>>> +Note we must describe PCI devices with the same bus hierarchy as the
>>>>>>  hardware, otherwise driver model cannot detect the correct parent/children
>>>>>>  relationship during PCI bus enumeration thus PCI devices won't be bound to
>>>>>>  their drivers accordingly. A working example like below:
>>>>>> --
>>>>>> 2.16.2
>>>>>>
>>>>>
>>>>> Are we really saying that compatible strings are 'generally not needed'?
>>>>
>>>> Yes, PCI is a discoverable bus.
>>>>
>>>>> device tree pci supplement 2.1 talks about some new formats for the
>>>>> compatible string, but doesn't say it is not needed. I much prefer a
>>>>> compatible string since it is easy to find the driver in the source
>>>>> code.
>>>>
>>>> But it duplicates (badly) what the PCI IDs and classes are used for
>>>> since PCI's inception.
>>>>
>>>>> Can way say that a compatible string is preferred, but in extremis you
>>>>> can avoid it by...
>>>>
>>>> No, see above, PCI is discoverable by design.
>>>
>>> I feel that these two things are orthogonal.
>>>
>>> You can probe the bus and find a device. That is the 'discoverable' part.
>>>
>>> But it is not automatically configurable. If it it were, there would
>>> be no DT node and no settings in the DT for the device. But from your
>>> patch, in some cases we need more information, and the DT node
>>> provides that.
>>
>> Pretty much, you can have stuff on the PCI card which needs extra info.
>>
>>> So to get the settings to pass to the driver, you have to find the
>>> device-tree node to use for the device. The only way U-Boot supports
>>> is to use the 'reg' property, which specifies the PCI address. (We
>>> don't support a compatible string starting with "pci...". We could
>>> support that, but it is more code for essentially the same purpose.)
>>
>> Yes
>>
>>> So we are not talking about the discoverability, which is already
>>> supported by U-Boot. We are talking about the configuration of the
>>> device, via settings passed to the driver.
>>
>> Yes
>>
>>> In fact the only issue here is whether to require a compatible string
>>> for PCI nodes or allow matching solely based on the 'reg' property. Is
>>> the latter widely used in Linux? Presumably not on x86, which doesn't
>>> even use DT.
>>
>> I only see the compatible string used for bridges, the rest of the
>> subdevices match on reg property.
> 
> Where are you looking?

Roughly 'git grep -A 10 pci arch/arm*/boot/dts' in Linux

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH V2 2/2] pci: Update documentation to make 'compatible' string optional

[Simon Glass]

Hi Marek,

On 2 September 2018 at 12:26, Marek Vasut <marek.vasut@gmail.com> wrote:
> On 09/02/2018 03:07 AM, Simon Glass wrote:
>> Hi,
>>
>> On 1 September 2018 at 16:41, Marek Vasut <marek.vasut@gmail.com> wrote:
>>> On 09/01/2018 11:45 PM, Simon Glass wrote:
>>>> Hi Marek,
>>>>
>>>> On 30 August 2018 at 04:20, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>> On 08/30/2018 02:29 AM, Simon Glass wrote:
>>>>>> Hi Marek,
>>>>>
>>>>> Hi,
>>>>>
>>>>>> On 24 August 2018 at 12:27, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>> Reword the documentation to make it clear the compatible string is now
>>>>>>> optional, yet still matching on it takes precedence over PCI IDs and
>>>>>>> PCI classes.
>>>>>>>
>>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>>> ---
>>>>>>> V2: New patch
>>>>>>> ---
>>>>>>>  doc/driver-model/pci-info.txt | 14 +++++++++-----
>>>>>>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/doc/driver-model/pci-info.txt b/doc/driver-model/pci-info.txt
>>>>>>> index e1701d1fbc..14364c5c75 100644
>>>>>>> --- a/doc/driver-model/pci-info.txt
>>>>>>> +++ b/doc/driver-model/pci-info.txt
>>>>>>> @@ -34,11 +34,15 @@ under that bus.
>>>>>>>  Note that this is all done on a lazy basis, as needed, so until something is
>>>>>>>  touched on PCI (eg: a call to pci_find_devices()) it will not be probed.
>>>>>>>
>>>>>>> -PCI devices can appear in the flattened device tree. If they do this serves to
>>>>>>> -specify the driver to use for the device. In this case they will be bound at
>>>>>>> -first. Each PCI device node must have a compatible string list as well as a
>>>>>>> -<reg> property, as defined by the IEEE Std 1275-1994 PCI bus binding document
>>>>>>> -v2.1. Note we must describe PCI devices with the same bus hierarchy as the
>>>>>>> +PCI devices can appear in the flattened device tree. If they do, their node
>>>>>>> +often contains extra information which cannot be derived from the PCI IDs or
>>>>>>> +PCI class of the device. Each PCI device node must have a <reg> property, as
>>>>>>> +defined by the IEEE Std 1275-1994 PCI bus binding document v2.1. Compatible
>>>>>>> +string list is optional and generally not needed, since PCI is discoverable
>>>>>>> +bus, albeit there are justified exceptions. If the compatible string is
>>>>>>> +present, matching on it takes precedence over PCI IDs and PCI classes.
>>>>>>> +
>>>>>>> +Note we must describe PCI devices with the same bus hierarchy as the
>>>>>>>  hardware, otherwise driver model cannot detect the correct parent/children
>>>>>>>  relationship during PCI bus enumeration thus PCI devices won't be bound to
>>>>>>>  their drivers accordingly. A working example like below:
>>>>>>> --
>>>>>>> 2.16.2
>>>>>>>
>>>>>>
>>>>>> Are we really saying that compatible strings are 'generally not needed'?
>>>>>
>>>>> Yes, PCI is a discoverable bus.
>>>>>
>>>>>> device tree pci supplement 2.1 talks about some new formats for the
>>>>>> compatible string, but doesn't say it is not needed. I much prefer a
>>>>>> compatible string since it is easy to find the driver in the source
>>>>>> code.
>>>>>
>>>>> But it duplicates (badly) what the PCI IDs and classes are used for
>>>>> since PCI's inception.
>>>>>
>>>>>> Can way say that a compatible string is preferred, but in extremis you
>>>>>> can avoid it by...
>>>>>
>>>>> No, see above, PCI is discoverable by design.
>>>>
>>>> I feel that these two things are orthogonal.
>>>>
>>>> You can probe the bus and find a device. That is the 'discoverable' part.
>>>>
>>>> But it is not automatically configurable. If it it were, there would
>>>> be no DT node and no settings in the DT for the device. But from your
>>>> patch, in some cases we need more information, and the DT node
>>>> provides that.
>>>
>>> Pretty much, you can have stuff on the PCI card which needs extra info.
>>>
>>>> So to get the settings to pass to the driver, you have to find the
>>>> device-tree node to use for the device. The only way U-Boot supports
>>>> is to use the 'reg' property, which specifies the PCI address. (We
>>>> don't support a compatible string starting with "pci...". We could
>>>> support that, but it is more code for essentially the same purpose.)
>>>
>>> Yes
>>>
>>>> So we are not talking about the discoverability, which is already
>>>> supported by U-Boot. We are talking about the configuration of the
>>>> device, via settings passed to the driver.
>>>
>>> Yes
>>>
>>>> In fact the only issue here is whether to require a compatible string
>>>> for PCI nodes or allow matching solely based on the 'reg' property. Is
>>>> the latter widely used in Linux? Presumably not on x86, which doesn't
>>>> even use DT.
>>>
>>> I only see the compatible string used for bridges, the rest of the
>>> subdevices match on reg property.
>>
>> Where are you looking?
>
> Roughly 'git grep -A 10 pci arch/arm*/boot/dts' in Linux

I don't really understand why we need 'status = "okay"' on a bridge,
since it should be enumerated anyway.

But I did find some bridges (I think( with settings like reset-gpios
and no compatible strings.

I'm OK with supporting this if you want to.

Tom do you have any thoughts on this?

Regards,
Simon

[U-Boot] [PATCH V2 1/2] pci: Support parsing PCI controller DT subnodes

[Marek Vasut]

On 09/02/2018 03:07 AM, Simon Glass wrote:
> Hi Marek,

Hi,

> On 1 September 2018 at 16:45, Marek Vasut <marek.vasut@gmail.com> wrote:
>>
>> On 09/01/2018 11:50 PM, Simon Glass wrote:
>>> Hi Marek,
>>>
>>> On 30 August 2018 at 07:42, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>> On 08/30/2018 03:32 PM, Bin Meng wrote:
>>>>> Hi Marek,
>>>>>
>>>>> On Thu, Aug 30, 2018 at 1:07 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>
>>>>>> On 08/29/2018 05:15 PM, Bin Meng wrote:
>>>>>>> +Simon
>>>>>>>
>>>>>>> Hi Marek,
>>>>>>>
>>>>>>> On Wed, Aug 29, 2018 at 10:22 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>>>
>>>>>>>> On 08/24/2018 08:27 PM, Marek Vasut wrote:
>>>>>>>>> The PCI controller can have DT subnodes describing extra properties
>>>>>>>>> of particular PCI devices, ie. a PHY attached to an EHCI controller
>>>>>>>>> on a PCI bus. This patch parses those DT subnodes and assigns a node
>>>>>>>>> to the PCI device instance, so that the driver can extract details
>>>>>>>>> from that node and ie. configure the PHY using the PHY subsystem.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>>>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>>>>
>>>>>>>> Well, bump ?
>>>>>>>>
>>>>>>>> This is the only missing patch to get my hardware working properly.
>>>>>>>
>>>>>>> I don't think we ever had an agreement on the v1 patch. Simon had a
>>>>>>> long email that pointed out what Linux does seems like a 'fallback' to
>>>>>>> find a node with no compatible string.
>>>>>>>
>>>>>>> Back to this, if we have to go with this way, please create a test
>>>>>>> case to cover this scenario.
>>>>>>
>>>>>> The fact that it works on a particular board is not tested enough?
>>>>>> Do we need a custom, special, synthetic test ?
>>>>>>
>>>>>
>>>>> I believe that's always been the requirement against the DM code
>>>>> changes. I was requested in the past when I changed something in the
>>>>> DM and I see other people were asked to do so. Like Alex said, it does
>>>>> not mean this patch was not tested enough, but to ensure future
>>>>> commits won't break this.
>>>>
>>>> So, do you have any suggestion how to implement this test ? It seems
>>>> Alex posed the same question. It doesn't seem to be trivial in the
>>>> context of sandbox.
>>>
>>> I suppose you need a PCI_DEVICE() declaration for sandbox, with an
>>> associated DT node and no compatible string. Then check that you can
>>> locate the device and that it read a DT property correctly.
>>
>> Is there any example of this stuff already ?
> 
> See the bottom of swap_case.c. You might be able to add a new one of those,
> 
> If you look at pci-controller2 in test.dts it has a device with a
> compatible string. You could try adding a second device with no
> compatible string.

And how does that test anything ?

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH V2 1/2] pci: Support parsing PCI controller DT subnodes

[Simon Glass]

Hi Marek,

On 10 September 2018 at 01:38, Marek Vasut <marek.vasut@gmail.com> wrote:
>
> On 09/02/2018 03:07 AM, Simon Glass wrote:
> > Hi Marek,
>
> Hi,
>
> > On 1 September 2018 at 16:45, Marek Vasut <marek.vasut@gmail.com> wrote:
> >>
> >> On 09/01/2018 11:50 PM, Simon Glass wrote:
> >>> Hi Marek,
> >>>
> >>> On 30 August 2018 at 07:42, Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>> On 08/30/2018 03:32 PM, Bin Meng wrote:
> >>>>> Hi Marek,
> >>>>>
> >>>>> On Thu, Aug 30, 2018 at 1:07 AM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>
> >>>>>> On 08/29/2018 05:15 PM, Bin Meng wrote:
> >>>>>>> +Simon
> >>>>>>>
> >>>>>>> Hi Marek,
> >>>>>>>
> >>>>>>> On Wed, Aug 29, 2018 at 10:22 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>>>
> >>>>>>>> On 08/24/2018 08:27 PM, Marek Vasut wrote:
> >>>>>>>>> The PCI controller can have DT subnodes describing extra properties
> >>>>>>>>> of particular PCI devices, ie. a PHY attached to an EHCI controller
> >>>>>>>>> on a PCI bus. This patch parses those DT subnodes and assigns a node
> >>>>>>>>> to the PCI device instance, so that the driver can extract details
> >>>>>>>>> from that node and ie. configure the PHY using the PHY subsystem.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> >>>>>>>>> Cc: Simon Glass <sjg@chromium.org>
> >>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
> >>>>>>>>
> >>>>>>>> Well, bump ?
> >>>>>>>>
> >>>>>>>> This is the only missing patch to get my hardware working properly.
> >>>>>>>
> >>>>>>> I don't think we ever had an agreement on the v1 patch. Simon had a
> >>>>>>> long email that pointed out what Linux does seems like a 'fallback' to
> >>>>>>> find a node with no compatible string.
> >>>>>>>
> >>>>>>> Back to this, if we have to go with this way, please create a test
> >>>>>>> case to cover this scenario.
> >>>>>>
> >>>>>> The fact that it works on a particular board is not tested enough?
> >>>>>> Do we need a custom, special, synthetic test ?
> >>>>>>
> >>>>>
> >>>>> I believe that's always been the requirement against the DM code
> >>>>> changes. I was requested in the past when I changed something in the
> >>>>> DM and I see other people were asked to do so. Like Alex said, it does
> >>>>> not mean this patch was not tested enough, but to ensure future
> >>>>> commits won't break this.
> >>>>
> >>>> So, do you have any suggestion how to implement this test ? It seems
> >>>> Alex posed the same question. It doesn't seem to be trivial in the
> >>>> context of sandbox.
> >>>
> >>> I suppose you need a PCI_DEVICE() declaration for sandbox, with an
> >>> associated DT node and no compatible string. Then check that you can
> >>> locate the device and that it read a DT property correctly.
> >>
> >> Is there any example of this stuff already ?
> >
> > See the bottom of swap_case.c. You might be able to add a new one of those,
> >
> > If you look at pci-controller2 in test.dts it has a device with a
> > compatible string. You could try adding a second device with no
> > compatible string.
>
> And how does that test anything ?

You can test that your code actually attaches the DT node to the
probed device. Without you code the test would fail. Wit it, it would
pass.

Regards,
Simon

[U-Boot] [PATCH V2 1/2] pci: Support parsing PCI controller DT subnodes

[Marek Vasut]

On 09/14/2018 06:41 AM, Simon Glass wrote:
> Hi Marek,
> 
> On 10 September 2018 at 01:38, Marek Vasut <marek.vasut@gmail.com> wrote:
>>
>> On 09/02/2018 03:07 AM, Simon Glass wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>> On 1 September 2018 at 16:45, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>
>>>> On 09/01/2018 11:50 PM, Simon Glass wrote:
>>>>> Hi Marek,
>>>>>
>>>>> On 30 August 2018 at 07:42, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>> On 08/30/2018 03:32 PM, Bin Meng wrote:
>>>>>>> Hi Marek,
>>>>>>>
>>>>>>> On Thu, Aug 30, 2018 at 1:07 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>>>
>>>>>>>> On 08/29/2018 05:15 PM, Bin Meng wrote:
>>>>>>>>> +Simon
>>>>>>>>>
>>>>>>>>> Hi Marek,
>>>>>>>>>
>>>>>>>>> On Wed, Aug 29, 2018 at 10:22 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>>>>>
>>>>>>>>>> On 08/24/2018 08:27 PM, Marek Vasut wrote:
>>>>>>>>>>> The PCI controller can have DT subnodes describing extra properties
>>>>>>>>>>> of particular PCI devices, ie. a PHY attached to an EHCI controller
>>>>>>>>>>> on a PCI bus. This patch parses those DT subnodes and assigns a node
>>>>>>>>>>> to the PCI device instance, so that the driver can extract details
>>>>>>>>>>> from that node and ie. configure the PHY using the PHY subsystem.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>>>>>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>>>>>>
>>>>>>>>>> Well, bump ?
>>>>>>>>>>
>>>>>>>>>> This is the only missing patch to get my hardware working properly.
>>>>>>>>>
>>>>>>>>> I don't think we ever had an agreement on the v1 patch. Simon had a
>>>>>>>>> long email that pointed out what Linux does seems like a 'fallback' to
>>>>>>>>> find a node with no compatible string.
>>>>>>>>>
>>>>>>>>> Back to this, if we have to go with this way, please create a test
>>>>>>>>> case to cover this scenario.
>>>>>>>>
>>>>>>>> The fact that it works on a particular board is not tested enough?
>>>>>>>> Do we need a custom, special, synthetic test ?
>>>>>>>>
>>>>>>>
>>>>>>> I believe that's always been the requirement against the DM code
>>>>>>> changes. I was requested in the past when I changed something in the
>>>>>>> DM and I see other people were asked to do so. Like Alex said, it does
>>>>>>> not mean this patch was not tested enough, but to ensure future
>>>>>>> commits won't break this.
>>>>>>
>>>>>> So, do you have any suggestion how to implement this test ? It seems
>>>>>> Alex posed the same question. It doesn't seem to be trivial in the
>>>>>> context of sandbox.
>>>>>
>>>>> I suppose you need a PCI_DEVICE() declaration for sandbox, with an
>>>>> associated DT node and no compatible string. Then check that you can
>>>>> locate the device and that it read a DT property correctly.
>>>>
>>>> Is there any example of this stuff already ?
>>>
>>> See the bottom of swap_case.c. You might be able to add a new one of those,
>>>
>>> If you look at pci-controller2 in test.dts it has a device with a
>>> compatible string. You could try adding a second device with no
>>> compatible string.
>>
>> And how does that test anything ?
> 
> You can test that your code actually attaches the DT node to the
> probed device. Without you code the test would fail. Wit it, it would
> pass.

Well it won't, because the sandbox swap_case.c requires the compatible.
This all seems like a big hack to support virtual PCI devices.

The driver binds with a compatible and then pins the read/write config
reg accessors to emulate their return values. Those include PCI IDs. So
you cannot instantiate virtual PCI device without this compatible string
and thus also cannot write such a test easily.

Now I also understand where this whole discussion about compatible
strings came from though.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH V2 1/2] pci: Support parsing PCI controller DT subnodes

[Bin Meng]

Hi Marek,

On Tue, Sep 18, 2018 at 8:01 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>
> On 09/14/2018 06:41 AM, Simon Glass wrote:
> > Hi Marek,
> >
> > On 10 September 2018 at 01:38, Marek Vasut <marek.vasut@gmail.com> wrote:
> >>
> >> On 09/02/2018 03:07 AM, Simon Glass wrote:
> >>> Hi Marek,
> >>
> >> Hi,
> >>
> >>> On 1 September 2018 at 16:45, Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>
> >>>> On 09/01/2018 11:50 PM, Simon Glass wrote:
> >>>>> Hi Marek,
> >>>>>
> >>>>> On 30 August 2018 at 07:42, Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>> On 08/30/2018 03:32 PM, Bin Meng wrote:
> >>>>>>> Hi Marek,
> >>>>>>>
> >>>>>>> On Thu, Aug 30, 2018 at 1:07 AM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>>>
> >>>>>>>> On 08/29/2018 05:15 PM, Bin Meng wrote:
> >>>>>>>>> +Simon
> >>>>>>>>>
> >>>>>>>>> Hi Marek,
> >>>>>>>>>
> >>>>>>>>> On Wed, Aug 29, 2018 at 10:22 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>>>>>
> >>>>>>>>>> On 08/24/2018 08:27 PM, Marek Vasut wrote:
> >>>>>>>>>>> The PCI controller can have DT subnodes describing extra properties
> >>>>>>>>>>> of particular PCI devices, ie. a PHY attached to an EHCI controller
> >>>>>>>>>>> on a PCI bus. This patch parses those DT subnodes and assigns a node
> >>>>>>>>>>> to the PCI device instance, so that the driver can extract details
> >>>>>>>>>>> from that node and ie. configure the PHY using the PHY subsystem.
> >>>>>>>>>>>
> >>>>>>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> >>>>>>>>>>> Cc: Simon Glass <sjg@chromium.org>
> >>>>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
> >>>>>>>>>>
> >>>>>>>>>> Well, bump ?
> >>>>>>>>>>
> >>>>>>>>>> This is the only missing patch to get my hardware working properly.
> >>>>>>>>>
> >>>>>>>>> I don't think we ever had an agreement on the v1 patch. Simon had a
> >>>>>>>>> long email that pointed out what Linux does seems like a 'fallback' to
> >>>>>>>>> find a node with no compatible string.
> >>>>>>>>>
> >>>>>>>>> Back to this, if we have to go with this way, please create a test
> >>>>>>>>> case to cover this scenario.
> >>>>>>>>
> >>>>>>>> The fact that it works on a particular board is not tested enough?
> >>>>>>>> Do we need a custom, special, synthetic test ?
> >>>>>>>>
> >>>>>>>
> >>>>>>> I believe that's always been the requirement against the DM code
> >>>>>>> changes. I was requested in the past when I changed something in the
> >>>>>>> DM and I see other people were asked to do so. Like Alex said, it does
> >>>>>>> not mean this patch was not tested enough, but to ensure future
> >>>>>>> commits won't break this.
> >>>>>>
> >>>>>> So, do you have any suggestion how to implement this test ? It seems
> >>>>>> Alex posed the same question. It doesn't seem to be trivial in the
> >>>>>> context of sandbox.
> >>>>>
> >>>>> I suppose you need a PCI_DEVICE() declaration for sandbox, with an
> >>>>> associated DT node and no compatible string. Then check that you can
> >>>>> locate the device and that it read a DT property correctly.
> >>>>
> >>>> Is there any example of this stuff already ?
> >>>
> >>> See the bottom of swap_case.c. You might be able to add a new one of those,
> >>>
> >>> If you look at pci-controller2 in test.dts it has a device with a
> >>> compatible string. You could try adding a second device with no
> >>> compatible string.
> >>
> >> And how does that test anything ?
> >
> > You can test that your code actually attaches the DT node to the
> > probed device. Without you code the test would fail. Wit it, it would
> > pass.
>
> Well it won't, because the sandbox swap_case.c requires the compatible.
> This all seems like a big hack to support virtual PCI devices.
>

The sandbox swap_case.c indeed supports dynamic driver binding, just
like real PCI devices. Please check doc/driver-model/pci-info.txt
(since you were modifying the same doc, I guess you missed that part
..)

> The driver binds with a compatible and then pins the read/write config
> reg accessors to emulate their return values. Those include PCI IDs. So
> you cannot instantiate virtual PCI device without this compatible string
> and thus also cannot write such a test easily.
>
> Now I also understand where this whole discussion about compatible
> strings came from though.

Regards,
Bin

[U-Boot] [PATCH V2 1/2] pci: Support parsing PCI controller DT subnodes

[Simon Glass]

Hi Marek,

On 18 September 2018 at 13:36, Marek Vasut <marek.vasut@gmail.com> wrote:
>
> On 09/14/2018 06:41 AM, Simon Glass wrote:
> > Hi Marek,
> >
> > On 10 September 2018 at 01:38, Marek Vasut <marek.vasut@gmail.com> wrote:
> >>
> >> On 09/02/2018 03:07 AM, Simon Glass wrote:
> >>> Hi Marek,
> >>
> >> Hi,
> >>
> >>> On 1 September 2018 at 16:45, Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>
> >>>> On 09/01/2018 11:50 PM, Simon Glass wrote:
> >>>>> Hi Marek,
> >>>>>
> >>>>> On 30 August 2018 at 07:42, Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>> On 08/30/2018 03:32 PM, Bin Meng wrote:
> >>>>>>> Hi Marek,
> >>>>>>>
> >>>>>>> On Thu, Aug 30, 2018 at 1:07 AM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>>>
> >>>>>>>> On 08/29/2018 05:15 PM, Bin Meng wrote:
> >>>>>>>>> +Simon
> >>>>>>>>>
> >>>>>>>>> Hi Marek,
> >>>>>>>>>
> >>>>>>>>> On Wed, Aug 29, 2018 at 10:22 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>>>>>
> >>>>>>>>>> On 08/24/2018 08:27 PM, Marek Vasut wrote:
> >>>>>>>>>>> The PCI controller can have DT subnodes describing extra properties
> >>>>>>>>>>> of particular PCI devices, ie. a PHY attached to an EHCI controller
> >>>>>>>>>>> on a PCI bus. This patch parses those DT subnodes and assigns a node
> >>>>>>>>>>> to the PCI device instance, so that the driver can extract details
> >>>>>>>>>>> from that node and ie. configure the PHY using the PHY subsystem.
> >>>>>>>>>>>
> >>>>>>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> >>>>>>>>>>> Cc: Simon Glass <sjg@chromium.org>
> >>>>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
> >>>>>>>>>>
> >>>>>>>>>> Well, bump ?
> >>>>>>>>>>
> >>>>>>>>>> This is the only missing patch to get my hardware working properly.
> >>>>>>>>>
> >>>>>>>>> I don't think we ever had an agreement on the v1 patch. Simon had a
> >>>>>>>>> long email that pointed out what Linux does seems like a 'fallback' to
> >>>>>>>>> find a node with no compatible string.
> >>>>>>>>>
> >>>>>>>>> Back to this, if we have to go with this way, please create a test
> >>>>>>>>> case to cover this scenario.
> >>>>>>>>
> >>>>>>>> The fact that it works on a particular board is not tested enough?
> >>>>>>>> Do we need a custom, special, synthetic test ?
> >>>>>>>>
> >>>>>>>
> >>>>>>> I believe that's always been the requirement against the DM code
> >>>>>>> changes. I was requested in the past when I changed something in the
> >>>>>>> DM and I see other people were asked to do so. Like Alex said, it does
> >>>>>>> not mean this patch was not tested enough, but to ensure future
> >>>>>>> commits won't break this.
> >>>>>>
> >>>>>> So, do you have any suggestion how to implement this test ? It seems
> >>>>>> Alex posed the same question. It doesn't seem to be trivial in the
> >>>>>> context of sandbox.
> >>>>>
> >>>>> I suppose you need a PCI_DEVICE() declaration for sandbox, with an
> >>>>> associated DT node and no compatible string. Then check that you can
> >>>>> locate the device and that it read a DT property correctly.
> >>>>
> >>>> Is there any example of this stuff already ?
> >>>
> >>> See the bottom of swap_case.c. You might be able to add a new one of those,
> >>>
> >>> If you look at pci-controller2 in test.dts it has a device with a
> >>> compatible string. You could try adding a second device with no
> >>> compatible string.
> >>
> >> And how does that test anything ?
> >
> > You can test that your code actually attaches the DT node to the
> > probed device. Without you code the test would fail. Wit it, it would
> > pass.
>
> Well it won't, because the sandbox swap_case.c requires the compatible.
> This all seems like a big hack to support virtual PCI devices.
>
> The driver binds with a compatible and then pins the read/write config
> reg accessors to emulate their return values. Those include PCI IDs. So
> you cannot instantiate virtual PCI device without this compatible string
> and thus also cannot write such a test easily.
>
> Now I also understand where this whole discussion about compatible
> strings came from though.

The compatible string is needed for the emulation driver but not for
the thing that connects to it. However as things stand you can't
attach an emulator to a bus without nesting it under the device which
it attaches to.

I suspect the best answer is to move the emulator so it is a direct
child of the bus. You would need to update sandbox_pci_get_emul() to
call device_find_first_child() on 'bus' instead of 'dev'.

Regards,
Simon

[U-Boot] [PATCH V2 1/2] pci: Support parsing PCI controller DT subnodes

[Marek Vasut]

On 09/18/2018 03:52 PM, Bin Meng wrote:
> Hi Marek,
> 
> On Tue, Sep 18, 2018 at 8:01 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>
>> On 09/14/2018 06:41 AM, Simon Glass wrote:
>>> Hi Marek,
>>>
>>> On 10 September 2018 at 01:38, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>
>>>> On 09/02/2018 03:07 AM, Simon Glass wrote:
>>>>> Hi Marek,
>>>>
>>>> Hi,
>>>>
>>>>> On 1 September 2018 at 16:45, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>
>>>>>> On 09/01/2018 11:50 PM, Simon Glass wrote:
>>>>>>> Hi Marek,
>>>>>>>
>>>>>>> On 30 August 2018 at 07:42, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>>> On 08/30/2018 03:32 PM, Bin Meng wrote:
>>>>>>>>> Hi Marek,
>>>>>>>>>
>>>>>>>>> On Thu, Aug 30, 2018 at 1:07 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>>>>>
>>>>>>>>>> On 08/29/2018 05:15 PM, Bin Meng wrote:
>>>>>>>>>>> +Simon
>>>>>>>>>>>
>>>>>>>>>>> Hi Marek,
>>>>>>>>>>>
>>>>>>>>>>> On Wed, Aug 29, 2018 at 10:22 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On 08/24/2018 08:27 PM, Marek Vasut wrote:
>>>>>>>>>>>>> The PCI controller can have DT subnodes describing extra properties
>>>>>>>>>>>>> of particular PCI devices, ie. a PHY attached to an EHCI controller
>>>>>>>>>>>>> on a PCI bus. This patch parses those DT subnodes and assigns a node
>>>>>>>>>>>>> to the PCI device instance, so that the driver can extract details
>>>>>>>>>>>>> from that node and ie. configure the PHY using the PHY subsystem.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>>>>>>>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>>>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>>>>>>>>
>>>>>>>>>>>> Well, bump ?
>>>>>>>>>>>>
>>>>>>>>>>>> This is the only missing patch to get my hardware working properly.
>>>>>>>>>>>
>>>>>>>>>>> I don't think we ever had an agreement on the v1 patch. Simon had a
>>>>>>>>>>> long email that pointed out what Linux does seems like a 'fallback' to
>>>>>>>>>>> find a node with no compatible string.
>>>>>>>>>>>
>>>>>>>>>>> Back to this, if we have to go with this way, please create a test
>>>>>>>>>>> case to cover this scenario.
>>>>>>>>>>
>>>>>>>>>> The fact that it works on a particular board is not tested enough?
>>>>>>>>>> Do we need a custom, special, synthetic test ?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I believe that's always been the requirement against the DM code
>>>>>>>>> changes. I was requested in the past when I changed something in the
>>>>>>>>> DM and I see other people were asked to do so. Like Alex said, it does
>>>>>>>>> not mean this patch was not tested enough, but to ensure future
>>>>>>>>> commits won't break this.
>>>>>>>>
>>>>>>>> So, do you have any suggestion how to implement this test ? It seems
>>>>>>>> Alex posed the same question. It doesn't seem to be trivial in the
>>>>>>>> context of sandbox.
>>>>>>>
>>>>>>> I suppose you need a PCI_DEVICE() declaration for sandbox, with an
>>>>>>> associated DT node and no compatible string. Then check that you can
>>>>>>> locate the device and that it read a DT property correctly.
>>>>>>
>>>>>> Is there any example of this stuff already ?
>>>>>
>>>>> See the bottom of swap_case.c. You might be able to add a new one of those,
>>>>>
>>>>> If you look at pci-controller2 in test.dts it has a device with a
>>>>> compatible string. You could try adding a second device with no
>>>>> compatible string.
>>>>
>>>> And how does that test anything ?
>>>
>>> You can test that your code actually attaches the DT node to the
>>> probed device. Without you code the test would fail. Wit it, it would
>>> pass.
>>
>> Well it won't, because the sandbox swap_case.c requires the compatible.
>> This all seems like a big hack to support virtual PCI devices.
>>
> 
> The sandbox swap_case.c indeed supports dynamic driver binding, just
> like real PCI devices. Please check doc/driver-model/pci-info.txt
> (since you were modifying the same doc, I guess you missed that part
> ..)

Any specific part I am looking for ?

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH V2 1/2] pci: Support parsing PCI controller DT subnodes

[Marek Vasut]

On 09/18/2018 03:52 PM, Simon Glass wrote:
> Hi Marek,
> 
> On 18 September 2018 at 13:36, Marek Vasut <marek.vasut@gmail.com> wrote:
>>
>> On 09/14/2018 06:41 AM, Simon Glass wrote:
>>> Hi Marek,
>>>
>>> On 10 September 2018 at 01:38, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>
>>>> On 09/02/2018 03:07 AM, Simon Glass wrote:
>>>>> Hi Marek,
>>>>
>>>> Hi,
>>>>
>>>>> On 1 September 2018 at 16:45, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>
>>>>>> On 09/01/2018 11:50 PM, Simon Glass wrote:
>>>>>>> Hi Marek,
>>>>>>>
>>>>>>> On 30 August 2018 at 07:42, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>>> On 08/30/2018 03:32 PM, Bin Meng wrote:
>>>>>>>>> Hi Marek,
>>>>>>>>>
>>>>>>>>> On Thu, Aug 30, 2018 at 1:07 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>>>>>
>>>>>>>>>> On 08/29/2018 05:15 PM, Bin Meng wrote:
>>>>>>>>>>> +Simon
>>>>>>>>>>>
>>>>>>>>>>> Hi Marek,
>>>>>>>>>>>
>>>>>>>>>>> On Wed, Aug 29, 2018 at 10:22 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On 08/24/2018 08:27 PM, Marek Vasut wrote:
>>>>>>>>>>>>> The PCI controller can have DT subnodes describing extra properties
>>>>>>>>>>>>> of particular PCI devices, ie. a PHY attached to an EHCI controller
>>>>>>>>>>>>> on a PCI bus. This patch parses those DT subnodes and assigns a node
>>>>>>>>>>>>> to the PCI device instance, so that the driver can extract details
>>>>>>>>>>>>> from that node and ie. configure the PHY using the PHY subsystem.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>>>>>>>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>>>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>>>>>>>>
>>>>>>>>>>>> Well, bump ?
>>>>>>>>>>>>
>>>>>>>>>>>> This is the only missing patch to get my hardware working properly.
>>>>>>>>>>>
>>>>>>>>>>> I don't think we ever had an agreement on the v1 patch. Simon had a
>>>>>>>>>>> long email that pointed out what Linux does seems like a 'fallback' to
>>>>>>>>>>> find a node with no compatible string.
>>>>>>>>>>>
>>>>>>>>>>> Back to this, if we have to go with this way, please create a test
>>>>>>>>>>> case to cover this scenario.
>>>>>>>>>>
>>>>>>>>>> The fact that it works on a particular board is not tested enough?
>>>>>>>>>> Do we need a custom, special, synthetic test ?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I believe that's always been the requirement against the DM code
>>>>>>>>> changes. I was requested in the past when I changed something in the
>>>>>>>>> DM and I see other people were asked to do so. Like Alex said, it does
>>>>>>>>> not mean this patch was not tested enough, but to ensure future
>>>>>>>>> commits won't break this.
>>>>>>>>
>>>>>>>> So, do you have any suggestion how to implement this test ? It seems
>>>>>>>> Alex posed the same question. It doesn't seem to be trivial in the
>>>>>>>> context of sandbox.
>>>>>>>
>>>>>>> I suppose you need a PCI_DEVICE() declaration for sandbox, with an
>>>>>>> associated DT node and no compatible string. Then check that you can
>>>>>>> locate the device and that it read a DT property correctly.
>>>>>>
>>>>>> Is there any example of this stuff already ?
>>>>>
>>>>> See the bottom of swap_case.c. You might be able to add a new one of those,
>>>>>
>>>>> If you look at pci-controller2 in test.dts it has a device with a
>>>>> compatible string. You could try adding a second device with no
>>>>> compatible string.
>>>>
>>>> And how does that test anything ?
>>>
>>> You can test that your code actually attaches the DT node to the
>>> probed device. Without you code the test would fail. Wit it, it would
>>> pass.
>>
>> Well it won't, because the sandbox swap_case.c requires the compatible.
>> This all seems like a big hack to support virtual PCI devices.
>>
>> The driver binds with a compatible and then pins the read/write config
>> reg accessors to emulate their return values. Those include PCI IDs. So
>> you cannot instantiate virtual PCI device without this compatible string
>> and thus also cannot write such a test easily.
>>
>> Now I also understand where this whole discussion about compatible
>> strings came from though.
> 
> The compatible string is needed for the emulation driver but not for
> the thing that connects to it. However as things stand you can't
> attach an emulator to a bus without nesting it under the device which
> it attaches to.
> 
> I suspect the best answer is to move the emulator so it is a direct
> child of the bus. You would need to update sandbox_pci_get_emul() to
> call device_find_first_child() on 'bus' instead of 'dev'.

Sounds to me _way_ out of scope for this patchset.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH V2 1/2] pci: Support parsing PCI controller DT subnodes

[Bin Meng]

Hi Marek,

On Wed, Sep 19, 2018 at 4:19 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>
> On 09/18/2018 03:52 PM, Bin Meng wrote:
> > Hi Marek,
> >
> > On Tue, Sep 18, 2018 at 8:01 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>
> >> On 09/14/2018 06:41 AM, Simon Glass wrote:
> >>> Hi Marek,
> >>>
> >>> On 10 September 2018 at 01:38, Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>
> >>>> On 09/02/2018 03:07 AM, Simon Glass wrote:
> >>>>> Hi Marek,
> >>>>
> >>>> Hi,
> >>>>
> >>>>> On 1 September 2018 at 16:45, Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>
> >>>>>> On 09/01/2018 11:50 PM, Simon Glass wrote:
> >>>>>>> Hi Marek,
> >>>>>>>
> >>>>>>> On 30 August 2018 at 07:42, Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>>> On 08/30/2018 03:32 PM, Bin Meng wrote:
> >>>>>>>>> Hi Marek,
> >>>>>>>>>
> >>>>>>>>> On Thu, Aug 30, 2018 at 1:07 AM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>>>>>
> >>>>>>>>>> On 08/29/2018 05:15 PM, Bin Meng wrote:
> >>>>>>>>>>> +Simon
> >>>>>>>>>>>
> >>>>>>>>>>> Hi Marek,
> >>>>>>>>>>>
> >>>>>>>>>>> On Wed, Aug 29, 2018 at 10:22 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>>>>>>>
> >>>>>>>>>>>> On 08/24/2018 08:27 PM, Marek Vasut wrote:
> >>>>>>>>>>>>> The PCI controller can have DT subnodes describing extra properties
> >>>>>>>>>>>>> of particular PCI devices, ie. a PHY attached to an EHCI controller
> >>>>>>>>>>>>> on a PCI bus. This patch parses those DT subnodes and assigns a node
> >>>>>>>>>>>>> to the PCI device instance, so that the driver can extract details
> >>>>>>>>>>>>> from that node and ie. configure the PHY using the PHY subsystem.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> >>>>>>>>>>>>> Cc: Simon Glass <sjg@chromium.org>
> >>>>>>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
> >>>>>>>>>>>>
> >>>>>>>>>>>> Well, bump ?
> >>>>>>>>>>>>
> >>>>>>>>>>>> This is the only missing patch to get my hardware working properly.
> >>>>>>>>>>>
> >>>>>>>>>>> I don't think we ever had an agreement on the v1 patch. Simon had a
> >>>>>>>>>>> long email that pointed out what Linux does seems like a 'fallback' to
> >>>>>>>>>>> find a node with no compatible string.
> >>>>>>>>>>>
> >>>>>>>>>>> Back to this, if we have to go with this way, please create a test
> >>>>>>>>>>> case to cover this scenario.
> >>>>>>>>>>
> >>>>>>>>>> The fact that it works on a particular board is not tested enough?
> >>>>>>>>>> Do we need a custom, special, synthetic test ?
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> I believe that's always been the requirement against the DM code
> >>>>>>>>> changes. I was requested in the past when I changed something in the
> >>>>>>>>> DM and I see other people were asked to do so. Like Alex said, it does
> >>>>>>>>> not mean this patch was not tested enough, but to ensure future
> >>>>>>>>> commits won't break this.
> >>>>>>>>
> >>>>>>>> So, do you have any suggestion how to implement this test ? It seems
> >>>>>>>> Alex posed the same question. It doesn't seem to be trivial in the
> >>>>>>>> context of sandbox.
> >>>>>>>
> >>>>>>> I suppose you need a PCI_DEVICE() declaration for sandbox, with an
> >>>>>>> associated DT node and no compatible string. Then check that you can
> >>>>>>> locate the device and that it read a DT property correctly.
> >>>>>>
> >>>>>> Is there any example of this stuff already ?
> >>>>>
> >>>>> See the bottom of swap_case.c. You might be able to add a new one of those,
> >>>>>
> >>>>> If you look at pci-controller2 in test.dts it has a device with a
> >>>>> compatible string. You could try adding a second device with no
> >>>>> compatible string.
> >>>>
> >>>> And how does that test anything ?
> >>>
> >>> You can test that your code actually attaches the DT node to the
> >>> probed device. Without you code the test would fail. Wit it, it would
> >>> pass.
> >>
> >> Well it won't, because the sandbox swap_case.c requires the compatible.
> >> This all seems like a big hack to support virtual PCI devices.
> >>
> >
> > The sandbox swap_case.c indeed supports dynamic driver binding, just
> > like real PCI devices. Please check doc/driver-model/pci-info.txt
> > (since you were modifying the same doc, I guess you missed that part
> > ..)
>
> Any specific part I am looking for ?

In the pci-info.txt, search for "The sandbox PCI drivers also support
dynamic driver binding". The arch/sandbox/dts/test.dts already has one
PCI controller and two swap_case devices setup for this testing. You
can start from there.

Regards,
Bin

[U-Boot] [PATCH V2 1/2] pci: Support parsing PCI controller DT subnodes

[Bin Meng]

Hi Marek,

On Wed, Sep 19, 2018 at 4:21 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>
> On 09/18/2018 03:52 PM, Simon Glass wrote:
> > Hi Marek,
> >
> > On 18 September 2018 at 13:36, Marek Vasut <marek.vasut@gmail.com> wrote:
> >>
> >> On 09/14/2018 06:41 AM, Simon Glass wrote:
> >>> Hi Marek,
> >>>
> >>> On 10 September 2018 at 01:38, Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>
> >>>> On 09/02/2018 03:07 AM, Simon Glass wrote:
> >>>>> Hi Marek,
> >>>>
> >>>> Hi,
> >>>>
> >>>>> On 1 September 2018 at 16:45, Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>
> >>>>>> On 09/01/2018 11:50 PM, Simon Glass wrote:
> >>>>>>> Hi Marek,
> >>>>>>>
> >>>>>>> On 30 August 2018 at 07:42, Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>>> On 08/30/2018 03:32 PM, Bin Meng wrote:
> >>>>>>>>> Hi Marek,
> >>>>>>>>>
> >>>>>>>>> On Thu, Aug 30, 2018 at 1:07 AM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>>>>>
> >>>>>>>>>> On 08/29/2018 05:15 PM, Bin Meng wrote:
> >>>>>>>>>>> +Simon
> >>>>>>>>>>>
> >>>>>>>>>>> Hi Marek,
> >>>>>>>>>>>
> >>>>>>>>>>> On Wed, Aug 29, 2018 at 10:22 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>>>>>>>
> >>>>>>>>>>>> On 08/24/2018 08:27 PM, Marek Vasut wrote:
> >>>>>>>>>>>>> The PCI controller can have DT subnodes describing extra properties
> >>>>>>>>>>>>> of particular PCI devices, ie. a PHY attached to an EHCI controller
> >>>>>>>>>>>>> on a PCI bus. This patch parses those DT subnodes and assigns a node
> >>>>>>>>>>>>> to the PCI device instance, so that the driver can extract details
> >>>>>>>>>>>>> from that node and ie. configure the PHY using the PHY subsystem.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> >>>>>>>>>>>>> Cc: Simon Glass <sjg@chromium.org>
> >>>>>>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
> >>>>>>>>>>>>
> >>>>>>>>>>>> Well, bump ?
> >>>>>>>>>>>>
> >>>>>>>>>>>> This is the only missing patch to get my hardware working properly.
> >>>>>>>>>>>
> >>>>>>>>>>> I don't think we ever had an agreement on the v1 patch. Simon had a
> >>>>>>>>>>> long email that pointed out what Linux does seems like a 'fallback' to
> >>>>>>>>>>> find a node with no compatible string.
> >>>>>>>>>>>
> >>>>>>>>>>> Back to this, if we have to go with this way, please create a test
> >>>>>>>>>>> case to cover this scenario.
> >>>>>>>>>>
> >>>>>>>>>> The fact that it works on a particular board is not tested enough?
> >>>>>>>>>> Do we need a custom, special, synthetic test ?
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> I believe that's always been the requirement against the DM code
> >>>>>>>>> changes. I was requested in the past when I changed something in the
> >>>>>>>>> DM and I see other people were asked to do so. Like Alex said, it does
> >>>>>>>>> not mean this patch was not tested enough, but to ensure future
> >>>>>>>>> commits won't break this.
> >>>>>>>>
> >>>>>>>> So, do you have any suggestion how to implement this test ? It seems
> >>>>>>>> Alex posed the same question. It doesn't seem to be trivial in the
> >>>>>>>> context of sandbox.
> >>>>>>>
> >>>>>>> I suppose you need a PCI_DEVICE() declaration for sandbox, with an
> >>>>>>> associated DT node and no compatible string. Then check that you can
> >>>>>>> locate the device and that it read a DT property correctly.
> >>>>>>
> >>>>>> Is there any example of this stuff already ?
> >>>>>
> >>>>> See the bottom of swap_case.c. You might be able to add a new one of those,
> >>>>>
> >>>>> If you look at pci-controller2 in test.dts it has a device with a
> >>>>> compatible string. You could try adding a second device with no
> >>>>> compatible string.
> >>>>
> >>>> And how does that test anything ?
> >>>
> >>> You can test that your code actually attaches the DT node to the
> >>> probed device. Without you code the test would fail. Wit it, it would
> >>> pass.
> >>
> >> Well it won't, because the sandbox swap_case.c requires the compatible.
> >> This all seems like a big hack to support virtual PCI devices.
> >>
> >> The driver binds with a compatible and then pins the read/write config
> >> reg accessors to emulate their return values. Those include PCI IDs. So
> >> you cannot instantiate virtual PCI device without this compatible string
> >> and thus also cannot write such a test easily.
> >>
> >> Now I also understand where this whole discussion about compatible
> >> strings came from though.
> >
> > The compatible string is needed for the emulation driver but not for
> > the thing that connects to it. However as things stand you can't
> > attach an emulator to a bus without nesting it under the device which
> > it attaches to.
> >
> > I suspect the best answer is to move the emulator so it is a direct
> > child of the bus. You would need to update sandbox_pci_get_emul() to
> > call device_find_first_child() on 'bus' instead of 'dev'.
>
> Sounds to me _way_ out of scope for this patchset.

Dynamic binding is already supported on Sandbox. I guess Simon may
have missed the part.

Regards,
Bin

[U-Boot] [PATCH V2 1/2] pci: Support parsing PCI controller DT subnodes

[Marek Vasut]

On 09/19/2018 11:26 AM, Bin Meng wrote:
> Hi Marek,
> 
> On Wed, Sep 19, 2018 at 4:21 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>
>> On 09/18/2018 03:52 PM, Simon Glass wrote:
>>> Hi Marek,
>>>
>>> On 18 September 2018 at 13:36, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>
>>>> On 09/14/2018 06:41 AM, Simon Glass wrote:
>>>>> Hi Marek,
>>>>>
>>>>> On 10 September 2018 at 01:38, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>
>>>>>> On 09/02/2018 03:07 AM, Simon Glass wrote:
>>>>>>> Hi Marek,
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>>> On 1 September 2018 at 16:45, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>>>
>>>>>>>> On 09/01/2018 11:50 PM, Simon Glass wrote:
>>>>>>>>> Hi Marek,
>>>>>>>>>
>>>>>>>>> On 30 August 2018 at 07:42, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>>>>> On 08/30/2018 03:32 PM, Bin Meng wrote:
>>>>>>>>>>> Hi Marek,
>>>>>>>>>>>
>>>>>>>>>>> On Thu, Aug 30, 2018 at 1:07 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On 08/29/2018 05:15 PM, Bin Meng wrote:
>>>>>>>>>>>>> +Simon
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hi Marek,
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Wed, Aug 29, 2018 at 10:22 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 08/24/2018 08:27 PM, Marek Vasut wrote:
>>>>>>>>>>>>>>> The PCI controller can have DT subnodes describing extra properties
>>>>>>>>>>>>>>> of particular PCI devices, ie. a PHY attached to an EHCI controller
>>>>>>>>>>>>>>> on a PCI bus. This patch parses those DT subnodes and assigns a node
>>>>>>>>>>>>>>> to the PCI device instance, so that the driver can extract details
>>>>>>>>>>>>>>> from that node and ie. configure the PHY using the PHY subsystem.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>>>>>>>>>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>>>>>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Well, bump ?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This is the only missing patch to get my hardware working properly.
>>>>>>>>>>>>>
>>>>>>>>>>>>> I don't think we ever had an agreement on the v1 patch. Simon had a
>>>>>>>>>>>>> long email that pointed out what Linux does seems like a 'fallback' to
>>>>>>>>>>>>> find a node with no compatible string.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Back to this, if we have to go with this way, please create a test
>>>>>>>>>>>>> case to cover this scenario.
>>>>>>>>>>>>
>>>>>>>>>>>> The fact that it works on a particular board is not tested enough?
>>>>>>>>>>>> Do we need a custom, special, synthetic test ?
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I believe that's always been the requirement against the DM code
>>>>>>>>>>> changes. I was requested in the past when I changed something in the
>>>>>>>>>>> DM and I see other people were asked to do so. Like Alex said, it does
>>>>>>>>>>> not mean this patch was not tested enough, but to ensure future
>>>>>>>>>>> commits won't break this.
>>>>>>>>>>
>>>>>>>>>> So, do you have any suggestion how to implement this test ? It seems
>>>>>>>>>> Alex posed the same question. It doesn't seem to be trivial in the
>>>>>>>>>> context of sandbox.
>>>>>>>>>
>>>>>>>>> I suppose you need a PCI_DEVICE() declaration for sandbox, with an
>>>>>>>>> associated DT node and no compatible string. Then check that you can
>>>>>>>>> locate the device and that it read a DT property correctly.
>>>>>>>>
>>>>>>>> Is there any example of this stuff already ?
>>>>>>>
>>>>>>> See the bottom of swap_case.c. You might be able to add a new one of those,
>>>>>>>
>>>>>>> If you look at pci-controller2 in test.dts it has a device with a
>>>>>>> compatible string. You could try adding a second device with no
>>>>>>> compatible string.
>>>>>>
>>>>>> And how does that test anything ?
>>>>>
>>>>> You can test that your code actually attaches the DT node to the
>>>>> probed device. Without you code the test would fail. Wit it, it would
>>>>> pass.
>>>>
>>>> Well it won't, because the sandbox swap_case.c requires the compatible.
>>>> This all seems like a big hack to support virtual PCI devices.
>>>>
>>>> The driver binds with a compatible and then pins the read/write config
>>>> reg accessors to emulate their return values. Those include PCI IDs. So
>>>> you cannot instantiate virtual PCI device without this compatible string
>>>> and thus also cannot write such a test easily.
>>>>
>>>> Now I also understand where this whole discussion about compatible
>>>> strings came from though.
>>>
>>> The compatible string is needed for the emulation driver but not for
>>> the thing that connects to it. However as things stand you can't
>>> attach an emulator to a bus without nesting it under the device which
>>> it attaches to.
>>>
>>> I suspect the best answer is to move the emulator so it is a direct
>>> child of the bus. You would need to update sandbox_pci_get_emul() to
>>> call device_find_first_child() on 'bus' instead of 'dev'.
>>
>> Sounds to me _way_ out of scope for this patchset.
> 
> Dynamic binding is already supported on Sandbox. I guess Simon may
> have missed the part.

Well, where is an example of that ? Because I am not seeing one.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH V2 1/2] pci: Support parsing PCI controller DT subnodes

[Bin Meng]

Hi Marek,

On Wed, Sep 19, 2018 at 5:34 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>
> On 09/19/2018 11:26 AM, Bin Meng wrote:
> > Hi Marek,
> >
> > On Wed, Sep 19, 2018 at 4:21 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>
> >> On 09/18/2018 03:52 PM, Simon Glass wrote:
> >>> Hi Marek,
> >>>
> >>> On 18 September 2018 at 13:36, Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>
> >>>> On 09/14/2018 06:41 AM, Simon Glass wrote:
> >>>>> Hi Marek,
> >>>>>
> >>>>> On 10 September 2018 at 01:38, Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>
> >>>>>> On 09/02/2018 03:07 AM, Simon Glass wrote:
> >>>>>>> Hi Marek,
> >>>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>>> On 1 September 2018 at 16:45, Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>>>
> >>>>>>>> On 09/01/2018 11:50 PM, Simon Glass wrote:
> >>>>>>>>> Hi Marek,
> >>>>>>>>>
> >>>>>>>>> On 30 August 2018 at 07:42, Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>>>>> On 08/30/2018 03:32 PM, Bin Meng wrote:
> >>>>>>>>>>> Hi Marek,
> >>>>>>>>>>>
> >>>>>>>>>>> On Thu, Aug 30, 2018 at 1:07 AM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>>>>>>>
> >>>>>>>>>>>> On 08/29/2018 05:15 PM, Bin Meng wrote:
> >>>>>>>>>>>>> +Simon
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Hi Marek,
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> On Wed, Aug 29, 2018 at 10:22 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> On 08/24/2018 08:27 PM, Marek Vasut wrote:
> >>>>>>>>>>>>>>> The PCI controller can have DT subnodes describing extra properties
> >>>>>>>>>>>>>>> of particular PCI devices, ie. a PHY attached to an EHCI controller
> >>>>>>>>>>>>>>> on a PCI bus. This patch parses those DT subnodes and assigns a node
> >>>>>>>>>>>>>>> to the PCI device instance, so that the driver can extract details
> >>>>>>>>>>>>>>> from that node and ie. configure the PHY using the PHY subsystem.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> >>>>>>>>>>>>>>> Cc: Simon Glass <sjg@chromium.org>
> >>>>>>>>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Well, bump ?
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> This is the only missing patch to get my hardware working properly.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> I don't think we ever had an agreement on the v1 patch. Simon had a
> >>>>>>>>>>>>> long email that pointed out what Linux does seems like a 'fallback' to
> >>>>>>>>>>>>> find a node with no compatible string.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Back to this, if we have to go with this way, please create a test
> >>>>>>>>>>>>> case to cover this scenario.
> >>>>>>>>>>>>
> >>>>>>>>>>>> The fact that it works on a particular board is not tested enough?
> >>>>>>>>>>>> Do we need a custom, special, synthetic test ?
> >>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> I believe that's always been the requirement against the DM code
> >>>>>>>>>>> changes. I was requested in the past when I changed something in the
> >>>>>>>>>>> DM and I see other people were asked to do so. Like Alex said, it does
> >>>>>>>>>>> not mean this patch was not tested enough, but to ensure future
> >>>>>>>>>>> commits won't break this.
> >>>>>>>>>>
> >>>>>>>>>> So, do you have any suggestion how to implement this test ? It seems
> >>>>>>>>>> Alex posed the same question. It doesn't seem to be trivial in the
> >>>>>>>>>> context of sandbox.
> >>>>>>>>>
> >>>>>>>>> I suppose you need a PCI_DEVICE() declaration for sandbox, with an
> >>>>>>>>> associated DT node and no compatible string. Then check that you can
> >>>>>>>>> locate the device and that it read a DT property correctly.
> >>>>>>>>
> >>>>>>>> Is there any example of this stuff already ?
> >>>>>>>
> >>>>>>> See the bottom of swap_case.c. You might be able to add a new one of those,
> >>>>>>>
> >>>>>>> If you look at pci-controller2 in test.dts it has a device with a
> >>>>>>> compatible string. You could try adding a second device with no
> >>>>>>> compatible string.
> >>>>>>
> >>>>>> And how does that test anything ?
> >>>>>
> >>>>> You can test that your code actually attaches the DT node to the
> >>>>> probed device. Without you code the test would fail. Wit it, it would
> >>>>> pass.
> >>>>
> >>>> Well it won't, because the sandbox swap_case.c requires the compatible.
> >>>> This all seems like a big hack to support virtual PCI devices.
> >>>>
> >>>> The driver binds with a compatible and then pins the read/write config
> >>>> reg accessors to emulate their return values. Those include PCI IDs. So
> >>>> you cannot instantiate virtual PCI device without this compatible string
> >>>> and thus also cannot write such a test easily.
> >>>>
> >>>> Now I also understand where this whole discussion about compatible
> >>>> strings came from though.
> >>>
> >>> The compatible string is needed for the emulation driver but not for
> >>> the thing that connects to it. However as things stand you can't
> >>> attach an emulator to a bus without nesting it under the device which
> >>> it attaches to.
> >>>
> >>> I suspect the best answer is to move the emulator so it is a direct
> >>> child of the bus. You would need to update sandbox_pci_get_emul() to
> >>> call device_find_first_child() on 'bus' instead of 'dev'.
> >>
> >> Sounds to me _way_ out of scope for this patchset.
> >
> > Dynamic binding is already supported on Sandbox. I guess Simon may
> > have missed the part.
>
> Well, where is an example of that ? Because I am not seeing one.
>

I already pointed out in the previous email. In
arch/sandbox/dts/test.dts, the 2nd PCI controller has two swap_case
devices and the 3rd controller has one.
In swap_case.c, U_BOOT_PCI_DEVICE() is there which is also a clear
sign that the driver supports dynamic binding. Of course, the driver
supports "compatible" too as you noticed.

Regards,
Bin

[U-Boot] [PATCH V2 1/2] pci: Support parsing PCI controller DT subnodes

[Marek Vasut]

On 09/19/2018 11:41 AM, Bin Meng wrote:
> Hi Marek,
> 
> On Wed, Sep 19, 2018 at 5:34 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>
>> On 09/19/2018 11:26 AM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Wed, Sep 19, 2018 at 4:21 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>
>>>> On 09/18/2018 03:52 PM, Simon Glass wrote:
>>>>> Hi Marek,
>>>>>
>>>>> On 18 September 2018 at 13:36, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>
>>>>>> On 09/14/2018 06:41 AM, Simon Glass wrote:
>>>>>>> Hi Marek,
>>>>>>>
>>>>>>> On 10 September 2018 at 01:38, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>>>
>>>>>>>> On 09/02/2018 03:07 AM, Simon Glass wrote:
>>>>>>>>> Hi Marek,
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>>> On 1 September 2018 at 16:45, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>>>>>
>>>>>>>>>> On 09/01/2018 11:50 PM, Simon Glass wrote:
>>>>>>>>>>> Hi Marek,
>>>>>>>>>>>
>>>>>>>>>>> On 30 August 2018 at 07:42, Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>>>>>>> On 08/30/2018 03:32 PM, Bin Meng wrote:
>>>>>>>>>>>>> Hi Marek,
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Thu, Aug 30, 2018 at 1:07 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 08/29/2018 05:15 PM, Bin Meng wrote:
>>>>>>>>>>>>>>> +Simon
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Hi Marek,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On Wed, Aug 29, 2018 at 10:22 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On 08/24/2018 08:27 PM, Marek Vasut wrote:
>>>>>>>>>>>>>>>>> The PCI controller can have DT subnodes describing extra properties
>>>>>>>>>>>>>>>>> of particular PCI devices, ie. a PHY attached to an EHCI controller
>>>>>>>>>>>>>>>>> on a PCI bus. This patch parses those DT subnodes and assigns a node
>>>>>>>>>>>>>>>>> to the PCI device instance, so that the driver can extract details
>>>>>>>>>>>>>>>>> from that node and ie. configure the PHY using the PHY subsystem.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>>>>>>>>>>>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>>>>>>>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Well, bump ?
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> This is the only missing patch to get my hardware working properly.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I don't think we ever had an agreement on the v1 patch. Simon had a
>>>>>>>>>>>>>>> long email that pointed out what Linux does seems like a 'fallback' to
>>>>>>>>>>>>>>> find a node with no compatible string.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Back to this, if we have to go with this way, please create a test
>>>>>>>>>>>>>>> case to cover this scenario.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> The fact that it works on a particular board is not tested enough?
>>>>>>>>>>>>>> Do we need a custom, special, synthetic test ?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> I believe that's always been the requirement against the DM code
>>>>>>>>>>>>> changes. I was requested in the past when I changed something in the
>>>>>>>>>>>>> DM and I see other people were asked to do so. Like Alex said, it does
>>>>>>>>>>>>> not mean this patch was not tested enough, but to ensure future
>>>>>>>>>>>>> commits won't break this.
>>>>>>>>>>>>
>>>>>>>>>>>> So, do you have any suggestion how to implement this test ? It seems
>>>>>>>>>>>> Alex posed the same question. It doesn't seem to be trivial in the
>>>>>>>>>>>> context of sandbox.
>>>>>>>>>>>
>>>>>>>>>>> I suppose you need a PCI_DEVICE() declaration for sandbox, with an
>>>>>>>>>>> associated DT node and no compatible string. Then check that you can
>>>>>>>>>>> locate the device and that it read a DT property correctly.
>>>>>>>>>>
>>>>>>>>>> Is there any example of this stuff already ?
>>>>>>>>>
>>>>>>>>> See the bottom of swap_case.c. You might be able to add a new one of those,
>>>>>>>>>
>>>>>>>>> If you look at pci-controller2 in test.dts it has a device with a
>>>>>>>>> compatible string. You could try adding a second device with no
>>>>>>>>> compatible string.
>>>>>>>>
>>>>>>>> And how does that test anything ?
>>>>>>>
>>>>>>> You can test that your code actually attaches the DT node to the
>>>>>>> probed device. Without you code the test would fail. Wit it, it would
>>>>>>> pass.
>>>>>>
>>>>>> Well it won't, because the sandbox swap_case.c requires the compatible.
>>>>>> This all seems like a big hack to support virtual PCI devices.
>>>>>>
>>>>>> The driver binds with a compatible and then pins the read/write config
>>>>>> reg accessors to emulate their return values. Those include PCI IDs. So
>>>>>> you cannot instantiate virtual PCI device without this compatible string
>>>>>> and thus also cannot write such a test easily.
>>>>>>
>>>>>> Now I also understand where this whole discussion about compatible
>>>>>> strings came from though.
>>>>>
>>>>> The compatible string is needed for the emulation driver but not for
>>>>> the thing that connects to it. However as things stand you can't
>>>>> attach an emulator to a bus without nesting it under the device which
>>>>> it attaches to.
>>>>>
>>>>> I suspect the best answer is to move the emulator so it is a direct
>>>>> child of the bus. You would need to update sandbox_pci_get_emul() to
>>>>> call device_find_first_child() on 'bus' instead of 'dev'.
>>>>
>>>> Sounds to me _way_ out of scope for this patchset.
>>>
>>> Dynamic binding is already supported on Sandbox. I guess Simon may
>>> have missed the part.
>>
>> Well, where is an example of that ? Because I am not seeing one.
>>
> 
> I already pointed out in the previous email. In
> arch/sandbox/dts/test.dts, the 2nd PCI controller has two swap_case
> devices and the 3rd controller has one.

By "second" you mean pci1: or pci2: ? Because pci1: is second , after
pci0 . It'd really help if you were clearer in what you refer to.

> In swap_case.c, U_BOOT_PCI_DEVICE() is there which is also a clear
> sign that the driver supports dynamic binding. Of course, the driver
> supports "compatible" too as you noticed.

Are you talking about sandbox,dev-info DT property here ?

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH V2 1/2] pci: Support parsing PCI controller DT subnodes

[Bin Meng]

Hi Marek,

On Wed, Sep 19, 2018 at 9:28 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>
> On 09/19/2018 11:41 AM, Bin Meng wrote:
> > Hi Marek,
> >
> > On Wed, Sep 19, 2018 at 5:34 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>
> >> On 09/19/2018 11:26 AM, Bin Meng wrote:
> >>> Hi Marek,
> >>>
> >>> On Wed, Sep 19, 2018 at 4:21 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>
> >>>> On 09/18/2018 03:52 PM, Simon Glass wrote:
> >>>>> Hi Marek,
> >>>>>
> >>>>> On 18 September 2018 at 13:36, Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>
> >>>>>> On 09/14/2018 06:41 AM, Simon Glass wrote:
> >>>>>>> Hi Marek,
> >>>>>>>
> >>>>>>> On 10 September 2018 at 01:38, Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>>>
> >>>>>>>> On 09/02/2018 03:07 AM, Simon Glass wrote:
> >>>>>>>>> Hi Marek,
> >>>>>>>>
> >>>>>>>> Hi,
> >>>>>>>>
> >>>>>>>>> On 1 September 2018 at 16:45, Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>>>>>
> >>>>>>>>>> On 09/01/2018 11:50 PM, Simon Glass wrote:
> >>>>>>>>>>> Hi Marek,
> >>>>>>>>>>>
> >>>>>>>>>>> On 30 August 2018 at 07:42, Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>>>>>>> On 08/30/2018 03:32 PM, Bin Meng wrote:
> >>>>>>>>>>>>> Hi Marek,
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> On Thu, Aug 30, 2018 at 1:07 AM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> On 08/29/2018 05:15 PM, Bin Meng wrote:
> >>>>>>>>>>>>>>> +Simon
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Hi Marek,
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> On Wed, Aug 29, 2018 at 10:22 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> On 08/24/2018 08:27 PM, Marek Vasut wrote:
> >>>>>>>>>>>>>>>>> The PCI controller can have DT subnodes describing extra properties
> >>>>>>>>>>>>>>>>> of particular PCI devices, ie. a PHY attached to an EHCI controller
> >>>>>>>>>>>>>>>>> on a PCI bus. This patch parses those DT subnodes and assigns a node
> >>>>>>>>>>>>>>>>> to the PCI device instance, so that the driver can extract details
> >>>>>>>>>>>>>>>>> from that node and ie. configure the PHY using the PHY subsystem.
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> >>>>>>>>>>>>>>>>> Cc: Simon Glass <sjg@chromium.org>
> >>>>>>>>>>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> Well, bump ?
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> This is the only missing patch to get my hardware working properly.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> I don't think we ever had an agreement on the v1 patch. Simon had a
> >>>>>>>>>>>>>>> long email that pointed out what Linux does seems like a 'fallback' to
> >>>>>>>>>>>>>>> find a node with no compatible string.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Back to this, if we have to go with this way, please create a test
> >>>>>>>>>>>>>>> case to cover this scenario.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> The fact that it works on a particular board is not tested enough?
> >>>>>>>>>>>>>> Do we need a custom, special, synthetic test ?
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> I believe that's always been the requirement against the DM code
> >>>>>>>>>>>>> changes. I was requested in the past when I changed something in the
> >>>>>>>>>>>>> DM and I see other people were asked to do so. Like Alex said, it does
> >>>>>>>>>>>>> not mean this patch was not tested enough, but to ensure future
> >>>>>>>>>>>>> commits won't break this.
> >>>>>>>>>>>>
> >>>>>>>>>>>> So, do you have any suggestion how to implement this test ? It seems
> >>>>>>>>>>>> Alex posed the same question. It doesn't seem to be trivial in the
> >>>>>>>>>>>> context of sandbox.
> >>>>>>>>>>>
> >>>>>>>>>>> I suppose you need a PCI_DEVICE() declaration for sandbox, with an
> >>>>>>>>>>> associated DT node and no compatible string. Then check that you can
> >>>>>>>>>>> locate the device and that it read a DT property correctly.
> >>>>>>>>>>
> >>>>>>>>>> Is there any example of this stuff already ?
> >>>>>>>>>
> >>>>>>>>> See the bottom of swap_case.c. You might be able to add a new one of those,
> >>>>>>>>>
> >>>>>>>>> If you look at pci-controller2 in test.dts it has a device with a
> >>>>>>>>> compatible string. You could try adding a second device with no
> >>>>>>>>> compatible string.
> >>>>>>>>
> >>>>>>>> And how does that test anything ?
> >>>>>>>
> >>>>>>> You can test that your code actually attaches the DT node to the
> >>>>>>> probed device. Without you code the test would fail. Wit it, it would
> >>>>>>> pass.
> >>>>>>
> >>>>>> Well it won't, because the sandbox swap_case.c requires the compatible.
> >>>>>> This all seems like a big hack to support virtual PCI devices.
> >>>>>>
> >>>>>> The driver binds with a compatible and then pins the read/write config
> >>>>>> reg accessors to emulate their return values. Those include PCI IDs. So
> >>>>>> you cannot instantiate virtual PCI device without this compatible string
> >>>>>> and thus also cannot write such a test easily.
> >>>>>>
> >>>>>> Now I also understand where this whole discussion about compatible
> >>>>>> strings came from though.
> >>>>>
> >>>>> The compatible string is needed for the emulation driver but not for
> >>>>> the thing that connects to it. However as things stand you can't
> >>>>> attach an emulator to a bus without nesting it under the device which
> >>>>> it attaches to.
> >>>>>
> >>>>> I suspect the best answer is to move the emulator so it is a direct
> >>>>> child of the bus. You would need to update sandbox_pci_get_emul() to
> >>>>> call device_find_first_child() on 'bus' instead of 'dev'.
> >>>>
> >>>> Sounds to me _way_ out of scope for this patchset.
> >>>
> >>> Dynamic binding is already supported on Sandbox. I guess Simon may
> >>> have missed the part.
> >>
> >> Well, where is an example of that ? Because I am not seeing one.
> >>
> >
> > I already pointed out in the previous email. In
> > arch/sandbox/dts/test.dts, the 2nd PCI controller has two swap_case
> > devices and the 3rd controller has one.
>
> By "second" you mean pci1: or pci2: ? Because pci1: is second , after
> pci0 . It'd really help if you were clearer in what you refer to.
>

It's pci1. You can see there is no subnode under pci1 there yet if you
type 'pci 1' from the U-Boot shell you see two PCI devices.

> > In swap_case.c, U_BOOT_PCI_DEVICE() is there which is also a clear
> > sign that the driver supports dynamic binding. Of course, the driver
> > supports "compatible" too as you noticed.
>
> Are you talking about sandbox,dev-info DT property here ?

This is the property Sandbox uses to make the dynamic binding work.
You can bypass this. The key here is that swap_case driver supports
both "compatible" and dynamic binding, so you can write test cases to
cover this newly added ofnode scenario.

Regards,
Bin

[U-Boot] [PATCH V2 1/2] pci: Support parsing PCI controller DT subnodes

[Marek Vasut]

On 09/20/2018 03:47 AM, Bin Meng wrote:
[...]

>>>>>>> I suspect the best answer is to move the emulator so it is a direct
>>>>>>> child of the bus. You would need to update sandbox_pci_get_emul() to
>>>>>>> call device_find_first_child() on 'bus' instead of 'dev'.
>>>>>>
>>>>>> Sounds to me _way_ out of scope for this patchset.
>>>>>
>>>>> Dynamic binding is already supported on Sandbox. I guess Simon may
>>>>> have missed the part.
>>>>
>>>> Well, where is an example of that ? Because I am not seeing one.
>>>>
>>>
>>> I already pointed out in the previous email. In
>>> arch/sandbox/dts/test.dts, the 2nd PCI controller has two swap_case
>>> devices and the 3rd controller has one.
>>
>> By "second" you mean pci1: or pci2: ? Because pci1: is second , after
>> pci0 . It'd really help if you were clearer in what you refer to.
>>
> 
> It's pci1. You can see there is no subnode under pci1 there yet if you
> type 'pci 1' from the U-Boot shell you see two PCI devices.

I'd really appreciate it if you could be more precise when referring to
things.

>>> In swap_case.c, U_BOOT_PCI_DEVICE() is there which is also a clear
>>> sign that the driver supports dynamic binding. Of course, the driver
>>> supports "compatible" too as you noticed.
>>
>> Are you talking about sandbox,dev-info DT property here ?
> 
> This is the property Sandbox uses to make the dynamic binding work.
> You can bypass this.

Why would I want to bypass this ?

> The key here is that swap_case driver supports
> both "compatible" and dynamic binding, so you can write test cases to
> cover this newly added ofnode scenario.

That's great, and after spending even more time on this (probably days
by now), I just keep finding more and more limitations of the virtual
PCI subsystem which makes writing this testcase really hard. And none of
that really helps fixing the real problem on my real hardware, which
really stays broken.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH V2 1/2] pci: Support parsing PCI controller DT subnodes

[Simon Glass]

Hi Marek,

On 20 September 2018 at 17:56, Marek Vasut <marek.vasut@gmail.com> wrote:
> On 09/20/2018 03:47 AM, Bin Meng wrote:
> [...]
>
>>>>>>>> I suspect the best answer is to move the emulator so it is a direct
>>>>>>>> child of the bus. You would need to update sandbox_pci_get_emul() to
>>>>>>>> call device_find_first_child() on 'bus' instead of 'dev'.
>>>>>>>
>>>>>>> Sounds to me _way_ out of scope for this patchset.
>>>>>>
>>>>>> Dynamic binding is already supported on Sandbox. I guess Simon may
>>>>>> have missed the part.
>>>>>
>>>>> Well, where is an example of that ? Because I am not seeing one.
>>>>>
>>>>
>>>> I already pointed out in the previous email. In
>>>> arch/sandbox/dts/test.dts, the 2nd PCI controller has two swap_case
>>>> devices and the 3rd controller has one.
>>>
>>> By "second" you mean pci1: or pci2: ? Because pci1: is second , after
>>> pci0 . It'd really help if you were clearer in what you refer to.
>>>
>>
>> It's pci1. You can see there is no subnode under pci1 there yet if you
>> type 'pci 1' from the U-Boot shell you see two PCI devices.
>
> I'd really appreciate it if you could be more precise when referring to
> things.
>
>>>> In swap_case.c, U_BOOT_PCI_DEVICE() is there which is also a clear
>>>> sign that the driver supports dynamic binding. Of course, the driver
>>>> supports "compatible" too as you noticed.
>>>
>>> Are you talking about sandbox,dev-info DT property here ?
>>
>> This is the property Sandbox uses to make the dynamic binding work.
>> You can bypass this.
>
> Why would I want to bypass this ?
>
>> The key here is that swap_case driver supports
>> both "compatible" and dynamic binding, so you can write test cases to
>> cover this newly added ofnode scenario.
>
> That's great, and after spending even more time on this (probably days
> by now), I just keep finding more and more limitations of the virtual
> PCI subsystem which makes writing this testcase really hard. And none of
> that really helps fixing the real problem on my real hardware, which
> really stays broken.

You could fix that with a 5-minute patch to add a compatible string :-)

The problem is that you want to do it a certain way. Yes the test
system has limitations but it is better than what we had before (no
tests). We should expand its capability as we add new functionality.

Regards,
Simon

[U-Boot] [PATCH V2 1/2] pci: Support parsing PCI controller DT subnodes

[Marek Vasut]

On 09/26/2018 07:42 AM, Simon Glass wrote:
> Hi Marek,
> 
> On 20 September 2018 at 17:56, Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 09/20/2018 03:47 AM, Bin Meng wrote:
>> [...]
>>
>>>>>>>>> I suspect the best answer is to move the emulator so it is a direct
>>>>>>>>> child of the bus. You would need to update sandbox_pci_get_emul() to
>>>>>>>>> call device_find_first_child() on 'bus' instead of 'dev'.
>>>>>>>>
>>>>>>>> Sounds to me _way_ out of scope for this patchset.
>>>>>>>
>>>>>>> Dynamic binding is already supported on Sandbox. I guess Simon may
>>>>>>> have missed the part.
>>>>>>
>>>>>> Well, where is an example of that ? Because I am not seeing one.
>>>>>>
>>>>>
>>>>> I already pointed out in the previous email. In
>>>>> arch/sandbox/dts/test.dts, the 2nd PCI controller has two swap_case
>>>>> devices and the 3rd controller has one.
>>>>
>>>> By "second" you mean pci1: or pci2: ? Because pci1: is second , after
>>>> pci0 . It'd really help if you were clearer in what you refer to.
>>>>
>>>
>>> It's pci1. You can see there is no subnode under pci1 there yet if you
>>> type 'pci 1' from the U-Boot shell you see two PCI devices.
>>
>> I'd really appreciate it if you could be more precise when referring to
>> things.
>>
>>>>> In swap_case.c, U_BOOT_PCI_DEVICE() is there which is also a clear
>>>>> sign that the driver supports dynamic binding. Of course, the driver
>>>>> supports "compatible" too as you noticed.
>>>>
>>>> Are you talking about sandbox,dev-info DT property here ?
>>>
>>> This is the property Sandbox uses to make the dynamic binding work.
>>> You can bypass this.
>>
>> Why would I want to bypass this ?
>>
>>> The key here is that swap_case driver supports
>>> both "compatible" and dynamic binding, so you can write test cases to
>>> cover this newly added ofnode scenario.
>>
>> That's great, and after spending even more time on this (probably days
>> by now), I just keep finding more and more limitations of the virtual
>> PCI subsystem which makes writing this testcase really hard. And none of
>> that really helps fixing the real problem on my real hardware, which
>> really stays broken.
> 
> You could fix that with a 5-minute patch to add a compatible string :-)

No, that's a hack.

> The problem is that you want to do it a certain way. Yes the test
> system has limitations but it is better than what we had before (no
> tests). We should expand its capability as we add new functionality.
> 
> Regards,
> Simon
> 


-- 
Best regards,
Marek Vasut