Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 09/03/2018 01:35 AM, Simon Glass wrote: > Hi Marek, > > On 2 September 2018 at 12:24, Marek Vasut wrote: >> On 09/02/2018 03:07 AM, Simon Glass wrote: >>> Hi Marek, >>> >>> On 1 September 2018 at 16:43, Marek Vasut wrote: On 09/01/2018 11:45 PM, Simon Glass wrote: > Hi Marek, > > On 30 August 2018 at 03:25, Marek Vasut wrote: >> >> On 08/30/2018 02:29 AM, Simon Glass wrote: >>> Hi Marek, >> >> Hi, >> >> [...] >> > If you have both EHCI and a xHCI controller which can occupy the same > BFD, then how would you supply in the DT options needed by the > controller itself? Don't you need two nodes in that case? For the PHY case, it's controller-type-independent. >>> >>> What do you mean? Your example of why you can't use compatible strings >>> says you might have two different PHYs. But I think you should answer >>> my questions: >>> > If you have both EHCI and a xHCI controller which can occupy the same > BFD, then how would you supply in the DT options needed by the > controller itself? Don't you need two nodes in that case? >> >> You need only one node (if the PHY works with both controller options), >> which contains "reg" and "phy" properties. The driver matching is done >> on the PCI ID/class and the node is associated with the driver based on >> the "reg" property. > > I think you need two nodes if there are DT options that are different > for each PHY. In fact I think this is impossible to do with the reg > scheme. > > In effect the PHYs are different. They have different drivers, > assuming drivers are needed. So I feel that using a common address to > match two different devices is actually just weird. I think I lost you. But this discussion is really hypothetical. You _can_ have a USB PHY which can attach to both USB 2 and USB 3 controller, in which case you would have only one DT node to describe it. >>> >>> Can you point to an example of this? Otherwise it seems hypothetical. >> >> Nope, it's just electrically viable. > > OK, well I'm not sure that we can make any claims about any of this > unless it is used somewhere. > >> >>> As a counter-example, see exynos54xx.dtsi. >> >> What exactly am I looking for ? > > You can see a USB3 PHY which is attached to a USB3 host. > >> >>> I believe the correct way to do this is to enable/disable DT nodes. Do >>> you have any pointers to suggest that the same node should be used for >>> two devices? >> >> Nope > > Then I believe that is incorrect, and it should be handled using two > separate notes, with only one enabled at run-time. I don't think you can claim it's either correct or incorrect if there is no such hardware. -- Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Marek, On 2 September 2018 at 12:24, Marek Vasut wrote: > On 09/02/2018 03:07 AM, Simon Glass wrote: >> Hi Marek, >> >> On 1 September 2018 at 16:43, Marek Vasut wrote: >>> On 09/01/2018 11:45 PM, Simon Glass wrote: Hi Marek, On 30 August 2018 at 03:25, Marek Vasut wrote: > > On 08/30/2018 02:29 AM, Simon Glass wrote: >> Hi Marek, > > Hi, > > [...] > If you have both EHCI and a xHCI controller which can occupy the same BFD, then how would you supply in the DT options needed by the controller itself? Don't you need two nodes in that case? >>> >>> For the PHY case, it's controller-type-independent. >> >> What do you mean? Your example of why you can't use compatible strings >> says you might have two different PHYs. But I think you should answer >> my questions: >> If you have both EHCI and a xHCI controller which can occupy the same BFD, then how would you supply in the DT options needed by the controller itself? Don't you need two nodes in that case? > > You need only one node (if the PHY works with both controller options), > which contains "reg" and "phy" properties. The driver matching is done > on the PCI ID/class and the node is associated with the driver based on > the "reg" property. I think you need two nodes if there are DT options that are different for each PHY. In fact I think this is impossible to do with the reg scheme. In effect the PHYs are different. They have different drivers, assuming drivers are needed. So I feel that using a common address to match two different devices is actually just weird. >>> >>> I think I lost you. But this discussion is really hypothetical. You >>> _can_ have a USB PHY which can attach to both USB 2 and USB 3 >>> controller, in which case you would have only one DT node to describe it. >> >> Can you point to an example of this? Otherwise it seems hypothetical. > > Nope, it's just electrically viable. OK, well I'm not sure that we can make any claims about any of this unless it is used somewhere. > >> As a counter-example, see exynos54xx.dtsi. > > What exactly am I looking for ? You can see a USB3 PHY which is attached to a USB3 host. > >> I believe the correct way to do this is to enable/disable DT nodes. Do >> you have any pointers to suggest that the same node should be used for >> two devices? > > Nope Then I believe that is incorrect, and it should be handled using two separate notes, with only one enabled at run-time. Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 09/02/2018 03:07 AM, Simon Glass wrote: > Hi Marek, > > On 1 September 2018 at 16:43, Marek Vasut wrote: >> On 09/01/2018 11:45 PM, Simon Glass wrote: >>> Hi Marek, >>> >>> On 30 August 2018 at 03:25, Marek Vasut wrote: On 08/30/2018 02:29 AM, Simon Glass wrote: > Hi Marek, Hi, [...] >>> If you have both EHCI and a xHCI controller which can occupy the same >>> BFD, then how would you supply in the DT options needed by the >>> controller itself? Don't you need two nodes in that case? >> >> For the PHY case, it's controller-type-independent. > > What do you mean? Your example of why you can't use compatible strings > says you might have two different PHYs. But I think you should answer > my questions: > >>> If you have both EHCI and a xHCI controller which can occupy the same >>> BFD, then how would you supply in the DT options needed by the >>> controller itself? Don't you need two nodes in that case? You need only one node (if the PHY works with both controller options), which contains "reg" and "phy" properties. The driver matching is done on the PCI ID/class and the node is associated with the driver based on the "reg" property. >>> >>> I think you need two nodes if there are DT options that are different >>> for each PHY. In fact I think this is impossible to do with the reg >>> scheme. >>> >>> In effect the PHYs are different. They have different drivers, >>> assuming drivers are needed. So I feel that using a common address to >>> match two different devices is actually just weird. >> >> I think I lost you. But this discussion is really hypothetical. You >> _can_ have a USB PHY which can attach to both USB 2 and USB 3 >> controller, in which case you would have only one DT node to describe it. > > Can you point to an example of this? Otherwise it seems hypothetical. Nope, it's just electrically viable. > As a counter-example, see exynos54xx.dtsi. What exactly am I looking for ? > I believe the correct way to do this is to enable/disable DT nodes. Do > you have any pointers to suggest that the same node should be used for > two devices? Nope -- Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Marek, On 1 September 2018 at 16:43, Marek Vasut wrote: > On 09/01/2018 11:45 PM, Simon Glass wrote: >> Hi Marek, >> >> On 30 August 2018 at 03:25, Marek Vasut wrote: >>> >>> On 08/30/2018 02:29 AM, Simon Glass wrote: Hi Marek, >>> >>> Hi, >>> >>> [...] >>> >> If you have both EHCI and a xHCI controller which can occupy the same >> BFD, then how would you supply in the DT options needed by the >> controller itself? Don't you need two nodes in that case? > > For the PHY case, it's controller-type-independent. What do you mean? Your example of why you can't use compatible strings says you might have two different PHYs. But I think you should answer my questions: >> If you have both EHCI and a xHCI controller which can occupy the same >> BFD, then how would you supply in the DT options needed by the >> controller itself? Don't you need two nodes in that case? >>> >>> You need only one node (if the PHY works with both controller options), >>> which contains "reg" and "phy" properties. The driver matching is done >>> on the PCI ID/class and the node is associated with the driver based on >>> the "reg" property. >> >> I think you need two nodes if there are DT options that are different >> for each PHY. In fact I think this is impossible to do with the reg >> scheme. >> >> In effect the PHYs are different. They have different drivers, >> assuming drivers are needed. So I feel that using a common address to >> match two different devices is actually just weird. > > I think I lost you. But this discussion is really hypothetical. You > _can_ have a USB PHY which can attach to both USB 2 and USB 3 > controller, in which case you would have only one DT node to describe it. Can you point to an example of this? Otherwise it seems hypothetical. As a counter-example, see exynos54xx.dtsi. I believe the correct way to do this is to enable/disable DT nodes. Do you have any pointers to suggest that the same node should be used for two devices? Also, what settings are you actually adding to your DT node? Can you please point to it again? Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 09/01/2018 11:45 PM, Simon Glass wrote: > Hi Marek, > > On 30 August 2018 at 03:25, Marek Vasut wrote: >> >> On 08/30/2018 02:29 AM, Simon Glass wrote: >>> Hi Marek, >> >> Hi, >> >> [...] >> > If you have both EHCI and a xHCI controller which can occupy the same > BFD, then how would you supply in the DT options needed by the > controller itself? Don't you need two nodes in that case? For the PHY case, it's controller-type-independent. >>> >>> What do you mean? Your example of why you can't use compatible strings >>> says you might have two different PHYs. But I think you should answer >>> my questions: >>> > If you have both EHCI and a xHCI controller which can occupy the same > BFD, then how would you supply in the DT options needed by the > controller itself? Don't you need two nodes in that case? >> >> You need only one node (if the PHY works with both controller options), >> which contains "reg" and "phy" properties. The driver matching is done >> on the PCI ID/class and the node is associated with the driver based on >> the "reg" property. > > I think you need two nodes if there are DT options that are different > for each PHY. In fact I think this is impossible to do with the reg > scheme. > > In effect the PHYs are different. They have different drivers, > assuming drivers are needed. So I feel that using a common address to > match two different devices is actually just weird. I think I lost you. But this discussion is really hypothetical. You _can_ have a USB PHY which can attach to both USB 2 and USB 3 controller, in which case you would have only one DT node to describe it. -- Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Marek, On 30 August 2018 at 03:25, Marek Vasut wrote: > > On 08/30/2018 02:29 AM, Simon Glass wrote: > > Hi Marek, > > Hi, > > [...] > > >>> If you have both EHCI and a xHCI controller which can occupy the same > >>> BFD, then how would you supply in the DT options needed by the > >>> controller itself? Don't you need two nodes in that case? > >> > >> For the PHY case, it's controller-type-independent. > > > > What do you mean? Your example of why you can't use compatible strings > > says you might have two different PHYs. But I think you should answer > > my questions: > > > >>> If you have both EHCI and a xHCI controller which can occupy the same > >>> BFD, then how would you supply in the DT options needed by the > >>> controller itself? Don't you need two nodes in that case? > > You need only one node (if the PHY works with both controller options), > which contains "reg" and "phy" properties. The driver matching is done > on the PCI ID/class and the node is associated with the driver based on > the "reg" property. I think you need two nodes if there are DT options that are different for each PHY. In fact I think this is impossible to do with the reg scheme. In effect the PHYs are different. They have different drivers, assuming drivers are needed. So I feel that using a common address to match two different devices is actually just weird. - Simon > > > [...] > > > >>> In any case, re Bin's list of things that need doing, I worry about > >>> having different code for sandbox than other archs. It invalidates our > >>> sandbox testing. Granted, we have to support the PCI emulators, but > >>> that's OK since that code is not used except in sandbox. We still want > >>> to support compatible strings in the DT for PCI devices, right? > >> > >> We do, since there seems to be usecase for those too. > > > > OK, well let's make sure we have some compatible notes too in sandbox, > > so we retain testing. > > I'm not changing anything in sandbox, so the original case is covered as is. > > -- > Best regards, > Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/30/2018 02:29 AM, Simon Glass wrote: > Hi Marek, Hi, [...] >>> If you have both EHCI and a xHCI controller which can occupy the same >>> BFD, then how would you supply in the DT options needed by the >>> controller itself? Don't you need two nodes in that case? >> >> For the PHY case, it's controller-type-independent. > > What do you mean? Your example of why you can't use compatible strings > says you might have two different PHYs. But I think you should answer > my questions: > >>> If you have both EHCI and a xHCI controller which can occupy the same >>> BFD, then how would you supply in the DT options needed by the >>> controller itself? Don't you need two nodes in that case? You need only one node (if the PHY works with both controller options), which contains "reg" and "phy" properties. The driver matching is done on the PCI ID/class and the node is associated with the driver based on the "reg" property. > [...] > >>> In any case, re Bin's list of things that need doing, I worry about >>> having different code for sandbox than other archs. It invalidates our >>> sandbox testing. Granted, we have to support the PCI emulators, but >>> that's OK since that code is not used except in sandbox. We still want >>> to support compatible strings in the DT for PCI devices, right? >> >> We do, since there seems to be usecase for those too. > > OK, well let's make sure we have some compatible notes too in sandbox, > so we retain testing. I'm not changing anything in sandbox, so the original case is covered as is. -- Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Marek,
On 23 August 2018 at 06:58, Marek Vasut wrote:
> On 08/23/2018 12:45 PM, Simon Glass wrote:
> [...]
Why is pci_bus_find_devfn() failing?
>>>
>>> Because this function is a hack to force-bind drivers to PCI devices
>>> which are described in the DT with a compatible string. This does not
>>> apply to this case.
>>
>> pci_bus_find_devfn() does not check compatible strings. It checks the
>> PCI devfn stuff. If the node is in the DT and has the correct devfn,
>> then pci_bus_find_devfn() should find it.
>>
>> It does require that the node produced a device, which does indeed
>> require a compatible string, I think.
>
> Well, yeah, I skipped the middle step for the sake of brevity.
> Ultimately, you need a compat string there.
>
>> We support either using DT with a compatible string or using
>> PCI_DEVICE() without a DT node. But you already know that.
>
> Right
>
>>> If this function fails (correctly, no force-binding needs to happen
>>> because there is no compatible string, we want to match on PCI IDs),
>>> the code will continue and match on PCI ID/class with
>>> pci_find_and_bind_driver() . That function is where we need to verify if
>>> there isn't a PCI controller subnode in DT with a matching BFD which
>>> should be associated with the new driver instance.
>>
>> So all of this is to avoid having a compatible string in the DT node for PCI?
>
> Yes, because it is redundant and possibly harmful. And because I want to
> parse existing DTs without special U-Boot modifications or
> hacking/twisting U-Boot in some obscure way which doesn't scale.
>
>> Earlier I said:
>>
So can you just add a compatible string and everything works?
>>
>> and you replied:
>>
>>> No, because that makes no sense. The code can very well match the driver
>>> on the PCI IDs or classes, the DT node is supplementary information and
>>> it needs to be made available to the matched driver if present in the
>>> DT. The DT node is matched on the BFD (the reg property in the DT).
>>
>> Yes the DT node is matched on the BFD. Yes we can match the node as you say.
>>
>>> The compatible string would only add redundancy and can in fact be
>>> harmful in case the PCI device could be replaced (ie. swapping USB EHCI
>>> controller for USB xHCI controller while retaining the USB PHY ; the
>>> controller itself could be discovered on the PCI bus, but the PHY can
>>> not and must be described in the DT).
>>
>> This seems a bit odd. Without the compatible string we don't know what
>> driver to use, except through the PCI_DEVICE() mechanism. Is that what
>> you are using?
>
> Of course, you can match on PCI IDs or PCI class already, PCI is a
> discoverable bus.
>
>> So is this a reason why we need this 3rd option? For your example are
>> you saying that there are two different settings for a device, which
>> result in using a different driver?
>
> What I am saying is the following needs to be supported:
>
> pci-controller@100 {
> ...
> usb@1,0 {
> reg = <0x800 0 0 0 0>;
> phys = < 0>;
> phy-names = "usb";
> };
> ...
> };
>
> You want to match the PCI device in BFD x.1.0 using PCI ID (ie. the
> driver that matches in this case is *hci-pci) and then associate the
> node usb@1,0 , which contains PHY information for that *hci-pci driver,
> with it.
>
>> How would DT normally handle this?
>
> That's how DTs do it, even for other than r8a devices. There are iMX
> devices which use the same approach for associating ethernet MAC with a
> PCI NIC for example.
>
>> Overlays? Having two nodes and marking one status = "disabled"? This
>> problem does not seem unique to PCI. I could have a similar problem
>> with USB or even with a standard SoC having a memory-mapped USB
>> controller that can support both protocols (controlled by a setting
>> somewhere).
>
> You don't need overlays at all, the base DT is sufficient as is.
>
Everything up to here seems OK.
>> If you have both EHCI and a xHCI controller which can occupy the same
>> BFD, then how would you supply in the DT options needed by the
>> controller itself? Don't you need two nodes in that case?
>
> For the PHY case, it's controller-type-independent.
What do you mean? Your example of why you can't use compatible strings
says you might have two different PHYs. But I think you should answer
my questions:
>> If you have both EHCI and a xHCI controller which can occupy the same
>> BFD, then how would you supply in the DT options needed by the
>> controller itself? Don't you need two nodes in that case?
>
>> Also, what if these two devices each had their own PHY and there were
>> settings in that PHY DT node? You would need a separate node for each
>> one, right?
>>
>> I suggest that we don't NEED this third way. The question in my mind
>> is whether it is necessary and doesn't just add confusion.
>>
[...]
>> In any case, re Bin's list of things that need doing, I worry about
>> having different code
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/23/2018 12:45 PM, Simon Glass wrote:
[...]
>>> Why is pci_bus_find_devfn() failing?
>>
>> Because this function is a hack to force-bind drivers to PCI devices
>> which are described in the DT with a compatible string. This does not
>> apply to this case.
>
> pci_bus_find_devfn() does not check compatible strings. It checks the
> PCI devfn stuff. If the node is in the DT and has the correct devfn,
> then pci_bus_find_devfn() should find it.
>
> It does require that the node produced a device, which does indeed
> require a compatible string, I think.
Well, yeah, I skipped the middle step for the sake of brevity.
Ultimately, you need a compat string there.
> We support either using DT with a compatible string or using
> PCI_DEVICE() without a DT node. But you already know that.
Right
>> If this function fails (correctly, no force-binding needs to happen
>> because there is no compatible string, we want to match on PCI IDs),
>> the code will continue and match on PCI ID/class with
>> pci_find_and_bind_driver() . That function is where we need to verify if
>> there isn't a PCI controller subnode in DT with a matching BFD which
>> should be associated with the new driver instance.
>
> So all of this is to avoid having a compatible string in the DT node for PCI?
Yes, because it is redundant and possibly harmful. And because I want to
parse existing DTs without special U-Boot modifications or
hacking/twisting U-Boot in some obscure way which doesn't scale.
> Earlier I said:
>
>>> So can you just add a compatible string and everything works?
>
> and you replied:
>
>> No, because that makes no sense. The code can very well match the driver
>> on the PCI IDs or classes, the DT node is supplementary information and
>> it needs to be made available to the matched driver if present in the
>> DT. The DT node is matched on the BFD (the reg property in the DT).
>
> Yes the DT node is matched on the BFD. Yes we can match the node as you say.
>
>> The compatible string would only add redundancy and can in fact be
>> harmful in case the PCI device could be replaced (ie. swapping USB EHCI
>> controller for USB xHCI controller while retaining the USB PHY ; the
>> controller itself could be discovered on the PCI bus, but the PHY can
>> not and must be described in the DT).
>
> This seems a bit odd. Without the compatible string we don't know what
> driver to use, except through the PCI_DEVICE() mechanism. Is that what
> you are using?
Of course, you can match on PCI IDs or PCI class already, PCI is a
discoverable bus.
> So is this a reason why we need this 3rd option? For your example are
> you saying that there are two different settings for a device, which
> result in using a different driver?
What I am saying is the following needs to be supported:
pci-controller@100 {
...
usb@1,0 {
reg = <0x800 0 0 0 0>;
phys = < 0>;
phy-names = "usb";
};
...
};
You want to match the PCI device in BFD x.1.0 using PCI ID (ie. the
driver that matches in this case is *hci-pci) and then associate the
node usb@1,0 , which contains PHY information for that *hci-pci driver,
with it.
> How would DT normally handle this?
That's how DTs do it, even for other than r8a devices. There are iMX
devices which use the same approach for associating ethernet MAC with a
PCI NIC for example.
> Overlays? Having two nodes and marking one status = "disabled"? This
> problem does not seem unique to PCI. I could have a similar problem
> with USB or even with a standard SoC having a memory-mapped USB
> controller that can support both protocols (controlled by a setting
> somewhere).
You don't need overlays at all, the base DT is sufficient as is.
> If you have both EHCI and a xHCI controller which can occupy the same
> BFD, then how would you supply in the DT options needed by the
> controller itself? Don't you need two nodes in that case?
For the PHY case, it's controller-type-independent.
> Also, what if these two devices each had their own PHY and there were
> settings in that PHY DT node? You would need a separate node for each
> one, right?
>
> I suggest that we don't NEED this third way. The question in my mind
> is whether it is necessary and doesn't just add confusion.
>
> The code in Linux seems to be:
>
> struct device_node *of_pci_find_child_device(struct device_node *parent,
> unsigned int devfn)
> {
> struct device_node *node, *node2;
>
> for_each_child_of_node(parent, node) {
> if (__of_pci_pci_compare(node, devfn))
> return node;
>/*
>* Some OFs create a parent node "multifunc-device" as
>* a fake root for all functions of a multi-function
>* device we go down them as well.
>*/
>if (!strcmp(node->name, "multifunc-device")) {
>for_each_child_of_node(node,
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Marek,
On 22 August 2018 at 14:19, Marek Vasut wrote:
> On 08/22/2018 08:08 PM, Simon Glass wrote:
> [...]
>>> This is explained in the patch description and the thread again. Please
>>> read the thread before replying. Take a look at the r8a7794.dtsi and its
>>> PCI bindings, there are PCI controller subnodes which add extra
>>> information for PCI devices on the bus. These nodes do not have a
>>> compatible string, only a BFD.
>>>
>>> This is perfectly valid, since you can match a driver on the PCI IDs or
>>> classes (PCI_DEVICE()), but the driver doesn't have a DT node associated
>>> with it. If there is a DT node with a matching BFD, it is associated
>>> with the driver instance by this patch.
>>>
>>> This allows ie. the EHCI PCI driver to access that DT node and extract
>>> information about PHYs from the DT (in case of the r8a7794).
>>
>> But the code to do this already exists in pci_bind_bus_devices().
>>
>> I know I am late to this thread but no one is going to read 45 messages,
>> sorry.
>
> But then what is the point of having ML discussions archived at all if I
> have to explain the same thing over and over to everyone who comes into
> the thread ?
It is an in-built admonition for participants to avoid generating more
heat than light :-)
I thought someone pulled me into this thread, but perhaps I am mistaken.
>
>> Why is pci_bus_find_devfn() failing?
>
> Because this function is a hack to force-bind drivers to PCI devices
> which are described in the DT with a compatible string. This does not
> apply to this case.
pci_bus_find_devfn() does not check compatible strings. It checks the
PCI devfn stuff. If the node is in the DT and has the correct devfn,
then pci_bus_find_devfn() should find it.
It does require that the node produced a device, which does indeed
require a compatible string, I think.
We support either using DT with a compatible string or using
PCI_DEVICE() without a DT node. But you already know that.
>
> If this function fails (correctly, no force-binding needs to happen
> because there is no compatible string, we want to match on PCI IDs),
> the code will continue and match on PCI ID/class with
> pci_find_and_bind_driver() . That function is where we need to verify if
> there isn't a PCI controller subnode in DT with a matching BFD which
> should be associated with the new driver instance.
So all of this is to avoid having a compatible string in the DT node for PCI?
Earlier I said:
>> So can you just add a compatible string and everything works?
and you replied:
> No, because that makes no sense. The code can very well match the driver
> on the PCI IDs or classes, the DT node is supplementary information and
> it needs to be made available to the matched driver if present in the
> DT. The DT node is matched on the BFD (the reg property in the DT).
Yes the DT node is matched on the BFD. Yes we can match the node as you say.
> The compatible string would only add redundancy and can in fact be
> harmful in case the PCI device could be replaced (ie. swapping USB EHCI
> controller for USB xHCI controller while retaining the USB PHY ; the
> controller itself could be discovered on the PCI bus, but the PHY can
> not and must be described in the DT).
This seems a bit odd. Without the compatible string we don't know what
driver to use, except through the PCI_DEVICE() mechanism. Is that what
you are using?
So is this a reason why we need this 3rd option? For your example are
you saying that there are two different settings for a device, which
result in using a different driver? How would DT normally handle this?
Overlays? Having two nodes and marking one status = "disabled"? This
problem does not seem unique to PCI. I could have a similar problem
with USB or even with a standard SoC having a memory-mapped USB
controller that can support both protocols (controlled by a setting
somewhere).
If you have both EHCI and a xHCI controller which can occupy the same
BFD, then how would you supply in the DT options needed by the
controller itself? Don't you need two nodes in that case?
Also, what if these two devices each had their own PHY and there were
settings in that PHY DT node? You would need a separate node for each
one, right?
I suggest that we don't NEED this third way. The question in my mind
is whether it is necessary and doesn't just add confusion.
The code in Linux seems to be:
struct device_node *of_pci_find_child_device(struct device_node *parent,
unsigned int devfn)
{
struct device_node *node, *node2;
for_each_child_of_node(parent, node) {
if (__of_pci_pci_compare(node, devfn))
return node;
/*
* Some OFs create a parent node "multifunc-device" as
* a fake root for all functions of a multi-function
* device we go down them as well.
*/
if (!strcmp(node->name, "multifunc-device")) {
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/23/2018 04:11 AM, Bin Meng wrote: > Hi Marek, Hello Bin, > On Wed, Aug 22, 2018 at 5:57 PM, Marek Vasut wrote: >> On 08/22/2018 04:14 AM, Bin Meng wrote: >> [...] >> I said in the previous discussion I am willing to update the >> documentation to match the implementation, but that's about it. >> >>> So far almost all PCI device drivers in U-Boot supports both >>> scenarios, except PCI UART which currently only supports scenario#1. >>> If you declare what you do in this patch is pureblood then you should >>> revoke the other 2 at the same time. Leaving such in the mainstream >>> only creates chaos in the future and we should avoid doing that, given >>> we already had lots of discussion here. >> >> No. The compatible is valid for some PCI subdevs, ie. bridges, so that >> has to stay. You also need compatible for sandbox, you said that >> yourself. And declaring a PCI device with BFD in DT is needed, ie. for >> the r8a7794-style bindings. >> > > OK, now new comments :) So you admit "compatible" can be valid for > some cases. _some_ select ones, and that is a _very_ important distinction. > I have to point out that your theory on PCI device probing > is self-contradictory.You refuse to add a "compatible" string to your > PCI device because you said the vendor/device id/class provides enough > information to bind the driver, then why do you want to specify your > devices in the device tree in the first place. Because of the USB PHY , which is attached to that device and can not be probed on the PCI bus, unlike the device itself ? >>> >>> You can create a new EHCI PCI driver to match your USB EHCI >>> controller's vendor id/device id, and do any additional PHY setup in >>> that driver. BTW what's the vendor id and device id of the EHCI >>> controller on your board? I can have a look up at the PCI database. >> >> You can not. >> >> 1033:00e0 -- https://pci-ids.ucw.cz/read/PC/1033/00e0 >> >> You can buy that controller as a discrete chip on a PCI card or have one >> in an SoC on a PCI bus, I have both. > > Hardware vendors like to create such fantastic stuff in their SoCs, > sigh. So how about subvendor and subdevice id? The embedded one can't > be the same as the the discrete one. If they are really all the same, > then why can't we add a specific compatible string to describe such > device? The compatible idea was invented to describe devices that > cannot be discovered via some probeable ways. I cannot if you look at the r8a7794 PHY phandles in the DT. The PHY connects to two devices and this is well modeled in DT. > According to your > theory, "Each PCI device already has a PCI ID and class which is used > to identify it and based on which the drivers bind to it, so a DT > compatible is NOT needed and is actually redundant and harmful.", I > would argue why not just creating a dedicate PCI device driver using > PCI ID and class information to do the driver binding and start from > there. Because the same device with the same PCI ID can be used without that PHY . > As you mentioned PCI bus is probable bus like USB, everything > can be self-contained in the PCI device driver itself. There is no > need to create nothing in the device tree. If you want an example, > check 8250_pci.c in the Linux kernel. Everything that is needed to > configure the driver is included in the driver itself. It does not > read anything from the device tree. Well, see above for why this approach doesn't work. >>> >>> Please explain why it does not work. I see Intel e1000 driver in the >>> Linux kernel has lots of PHY configuration within the driver. It does >>> not use any external PHY (eg: Marvell 88exxx) driver that Linux >>> already has. The Ethernet PHY are not probeable on the PCI bus too. >> >> That's a different thing, the ethernet MACs have an internal MDIO >> interface and you can scan the MDIO bus and read out PHY registers 0/1 >> to get the PHY ID. >> > > Yes, I know. But my point is that Linux drivers are not always > consistent. There is diversity and Linux is fine with that. There is > also diversity when it comes to U-Boot PCI support and we have 2 > supported PCI scenarios, the 1st of which can satisfy your use case. > >> This does not apply to this particular usecase here. In this case, the >> controller can be either embedded or discrete, with PHY or without PHY. >> > > Does the embedded one have no register that can identify the PHY presence? No, why would it ? They are two completely separate blocks . [...] -- Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Marek, On Wed, Aug 22, 2018 at 5:57 PM, Marek Vasut wrote: > On 08/22/2018 04:14 AM, Bin Meng wrote: > [...] > I said in the previous discussion I am willing to update the > documentation to match the implementation, but that's about it. > >> So far almost all PCI device drivers in U-Boot supports both >> scenarios, except PCI UART which currently only supports scenario#1. >> If you declare what you do in this patch is pureblood then you should >> revoke the other 2 at the same time. Leaving such in the mainstream >> only creates chaos in the future and we should avoid doing that, given >> we already had lots of discussion here. > > No. The compatible is valid for some PCI subdevs, ie. bridges, so that > has to stay. You also need compatible for sandbox, you said that > yourself. And declaring a PCI device with BFD in DT is needed, ie. for > the r8a7794-style bindings. > OK, now new comments :) So you admit "compatible" can be valid for some cases. >>> >>> _some_ select ones, and that is a _very_ important distinction. >>> I have to point out that your theory on PCI device probing is self-contradictory.You refuse to add a "compatible" string to your PCI device because you said the vendor/device id/class provides enough information to bind the driver, then why do you want to specify your devices in the device tree in the first place. >>> >>> Because of the USB PHY , which is attached to that device and can not be >>> probed on the PCI bus, unlike the device itself ? >>> >> >> You can create a new EHCI PCI driver to match your USB EHCI >> controller's vendor id/device id, and do any additional PHY setup in >> that driver. BTW what's the vendor id and device id of the EHCI >> controller on your board? I can have a look up at the PCI database. > > You can not. > > 1033:00e0 -- https://pci-ids.ucw.cz/read/PC/1033/00e0 > > You can buy that controller as a discrete chip on a PCI card or have one > in an SoC on a PCI bus, I have both. Hardware vendors like to create such fantastic stuff in their SoCs, sigh. So how about subvendor and subdevice id? The embedded one can't be the same as the the discrete one. If they are really all the same, then why can't we add a specific compatible string to describe such device? The compatible idea was invented to describe devices that cannot be discovered via some probeable ways. > According to your theory, "Each PCI device already has a PCI ID and class which is used to identify it and based on which the drivers bind to it, so a DT compatible is NOT needed and is actually redundant and harmful.", I would argue why not just creating a dedicate PCI device driver using PCI ID and class information to do the driver binding and start from there. >>> >>> Because the same device with the same PCI ID can be used without that PHY . >>> As you mentioned PCI bus is probable bus like USB, everything can be self-contained in the PCI device driver itself. There is no need to create nothing in the device tree. If you want an example, check 8250_pci.c in the Linux kernel. Everything that is needed to configure the driver is included in the driver itself. It does not read anything from the device tree. >>> >>> Well, see above for why this approach doesn't work. >> >> Please explain why it does not work. I see Intel e1000 driver in the >> Linux kernel has lots of PHY configuration within the driver. It does >> not use any external PHY (eg: Marvell 88exxx) driver that Linux >> already has. The Ethernet PHY are not probeable on the PCI bus too. > > That's a different thing, the ethernet MACs have an internal MDIO > interface and you can scan the MDIO bus and read out PHY registers 0/1 > to get the PHY ID. > Yes, I know. But my point is that Linux drivers are not always consistent. There is diversity and Linux is fine with that. There is also diversity when it comes to U-Boot PCI support and we have 2 supported PCI scenarios, the 1st of which can satisfy your use case. > This does not apply to this particular usecase here. In this case, the > controller can be either embedded or discrete, with PHY or without PHY. > Does the embedded one have no register that can identify the PHY presence? > So again, how is the state of PCI more incomplete with this patch than > without it ? > Yes, you probably don't care about this. But I care about this as it impacts some other things like PCI uart driver which is used by some x86 boards. >>> >>> Sandbox changes impact PCI UART driver and x86 boards how ? >> >> See above. It's not about sandbox! > > So what is it about ? And how does this core change impact anything ? > Does it change behavior for some board or break some tests for you ? > >> * Create DM PCI test case against the DT node assignment >> *
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/22/2018 08:08 PM, Simon Glass wrote: [...] >> This is explained in the patch description and the thread again. Please >> read the thread before replying. Take a look at the r8a7794.dtsi and its >> PCI bindings, there are PCI controller subnodes which add extra >> information for PCI devices on the bus. These nodes do not have a >> compatible string, only a BFD. >> >> This is perfectly valid, since you can match a driver on the PCI IDs or >> classes (PCI_DEVICE()), but the driver doesn't have a DT node associated >> with it. If there is a DT node with a matching BFD, it is associated >> with the driver instance by this patch. >> >> This allows ie. the EHCI PCI driver to access that DT node and extract >> information about PHYs from the DT (in case of the r8a7794). > > But the code to do this already exists in pci_bind_bus_devices(). > > I know I am late to this thread but no one is going to read 45 messages, > sorry. But then what is the point of having ML discussions archived at all if I have to explain the same thing over and over to everyone who comes into the thread ? > Why is pci_bus_find_devfn() failing? Because this function is a hack to force-bind drivers to PCI devices which are described in the DT with a compatible string. This does not apply to this case. If this function fails (correctly, no force-binding needs to happen because there is no compatible string, we want to match on PCI IDs), the code will continue and match on PCI ID/class with pci_find_and_bind_driver() . That function is where we need to verify if there isn't a PCI controller subnode in DT with a matching BFD which should be associated with the new driver instance. -- Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Marek, On 20 August 2018 at 14:15, Marek Vasut wrote: > > On 08/20/2018 09:29 PM, Simon Glass wrote: > [...] > > > So what is the conclusion here ? Patch the design document and apply > > this patch as is ? > > > > I think we should see Simon's comments before we move forward. The > proposal I made before should come in a series, not just > documentation. > >>> > >>> This thread is too long :-) > >>> > >>> From what I understand, Marek and Bin are discussing whether a > >>> compatible string is needed to bind a driver. > >>> > >>> Generally it is. But with PCI and USB we have a search mechanism which > >>> can be used instead. > >>> > >>> The patch Marek submitted does not seems at all desirable to me. > >> > >> Can you explain why ? > > > > We already have a compatible string as the standard way to attach > > drivers to devices. > > > > For PCI, we already have PCI_DEVICE() and friends for when we can > > attach a driver for a PCI device without using a compatible string. > > > > Both of these are defined in the DT specification. > > > > The patch seems to be a rework of PCI_DEVICE() and I cannot why it is > > necessary. > > This is explained in the patch description and the thread again. Please > read the thread before replying. Take a look at the r8a7794.dtsi and its > PCI bindings, there are PCI controller subnodes which add extra > information for PCI devices on the bus. These nodes do not have a > compatible string, only a BFD. > > This is perfectly valid, since you can match a driver on the PCI IDs or > classes (PCI_DEVICE()), but the driver doesn't have a DT node associated > with it. If there is a DT node with a matching BFD, it is associated > with the driver instance by this patch. > > This allows ie. the EHCI PCI driver to access that DT node and extract > information about PHYs from the DT (in case of the r8a7794). But the code to do this already exists in pci_bind_bus_devices(). I know I am late to this thread but no one is going to read 45 messages, sorry. Why is pci_bus_find_devfn() failing? Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/22/2018 04:14 AM, Bin Meng wrote: [...] I said in the previous discussion I am willing to update the documentation to match the implementation, but that's about it. > So far almost all PCI device drivers in U-Boot supports both > scenarios, except PCI UART which currently only supports scenario#1. > If you declare what you do in this patch is pureblood then you should > revoke the other 2 at the same time. Leaving such in the mainstream > only creates chaos in the future and we should avoid doing that, given > we already had lots of discussion here. No. The compatible is valid for some PCI subdevs, ie. bridges, so that has to stay. You also need compatible for sandbox, you said that yourself. And declaring a PCI device with BFD in DT is needed, ie. for the r8a7794-style bindings. >>> >>> OK, now new comments :) So you admit "compatible" can be valid for >>> some cases. >> >> _some_ select ones, and that is a _very_ important distinction. >> >>> I have to point out that your theory on PCI device probing >>> is self-contradictory.You refuse to add a "compatible" string to your >>> PCI device because you said the vendor/device id/class provides enough >>> information to bind the driver, then why do you want to specify your >>> devices in the device tree in the first place. >> >> Because of the USB PHY , which is attached to that device and can not be >> probed on the PCI bus, unlike the device itself ? >> > > You can create a new EHCI PCI driver to match your USB EHCI > controller's vendor id/device id, and do any additional PHY setup in > that driver. BTW what's the vendor id and device id of the EHCI > controller on your board? I can have a look up at the PCI database. You can not. 1033:00e0 -- https://pci-ids.ucw.cz/read/PC/1033/00e0 You can buy that controller as a discrete chip on a PCI card or have one in an SoC on a PCI bus, I have both. >>> According to your >>> theory, "Each PCI device already has a PCI ID and class which is used >>> to identify it and based on which the drivers bind to it, so a DT >>> compatible is NOT needed and is actually redundant and harmful.", I >>> would argue why not just creating a dedicate PCI device driver using >>> PCI ID and class information to do the driver binding and start from >>> there. >> >> Because the same device with the same PCI ID can be used without that PHY . >> >>> As you mentioned PCI bus is probable bus like USB, everything >>> can be self-contained in the PCI device driver itself. There is no >>> need to create nothing in the device tree. If you want an example, >>> check 8250_pci.c in the Linux kernel. Everything that is needed to >>> configure the driver is included in the driver itself. It does not >>> read anything from the device tree. >> >> Well, see above for why this approach doesn't work. > > Please explain why it does not work. I see Intel e1000 driver in the > Linux kernel has lots of PHY configuration within the driver. It does > not use any external PHY (eg: Marvell 88exxx) driver that Linux > already has. The Ethernet PHY are not probeable on the PCI bus too. That's a different thing, the ethernet MACs have an internal MDIO interface and you can scan the MDIO bus and read out PHY registers 0/1 to get the PHY ID. This does not apply to this particular usecase here. In this case, the controller can be either embedded or discrete, with PHY or without PHY. So again, how is the state of PCI more incomplete with this patch than without it ? >>> Yes, you probably don't care about this. But I care about this >>> as it impacts some other things like PCI uart driver which is used by >>> some x86 boards. >> >> Sandbox changes impact PCI UART driver and x86 boards how ? > > See above. It's not about sandbox! So what is it about ? And how does this core change impact anything ? Does it change behavior for some board or break some tests for you ? > * Create DM PCI test case against the DT node assignment > * Remove all compatible string in U-Boot's PCI device drivers: eg: > ehci_pci_ids[], xhci_pci_ids[], etc. IOW, all PCI device drivers > should only use U_BOOT_PCI_DEVICE(), aka the original U-Boot option 2 > * Fork a "pci-ns16550" driver to support U_BOOT_PCI_DEVICE(), as > currently PCI ns16550 device driver uses "compatible" string to do the > matching, and update crownbay.dts and galileo.dts (so far I only know > two boards are using PCI ns16550 serial port) > * Make sure all DM PCI test cases are not broken > * Document all of the above changes in doc/driver-model/pci-info.txt I think you're just adding completely orthogonal stuff to this 5-liner patch into the list and overly complicate the situation. Sure, if you want to do all this, go ahead, but I don't see how this prevents this
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Marek, On Tue, Aug 21, 2018 at 6:28 PM, Marek Vasut wrote: > On 08/21/2018 09:16 AM, Bin Meng wrote: >> Hi Marek, >> >> On Tue, Aug 21, 2018 at 1:43 PM, Marek Vasut wrote: >>> On 08/21/2018 06:56 AM, Bin Meng wrote: >>> [...] The proposal I made is: * Keep pci-uclass driver's post_bind() and child_post_bind() only for Sandbox configuration * Keep the call to pci_bus_find_devfn() in pci_bind_bus_devices() only for Sandbox configuration * Sandbox is special. We should limit the mechanism of matching PCI emulation device via "compatible" to sandbox only >>> >>> The above three points can be done separately and I don't care about >>> this too much. >>> >> >> That's my concern. Only doing this created confusing and incomplete >> design. > > Incomplete how ? I explained several times. I have to repeat here once again: Currently U-Boot supports two scenarios for PCI driver binding: - Declare a PCI device in the device tree. That requires specifying a 'compatible' string as well as 'reg' property as defined by the 'PCI Bus Binding' spec. DM uses the 'compatible' string to bind the driver for the device. - Don't declare a PCI device in the device tree. Instead, using U_BOOT_PCI_DEVICE() to declare a device and driver mapping. You are creating a 3rd scenario which is a mix up of both scenario#1 and #2 in this patch, yet without any documentation, plus any additional changes to impacted drivers. >>> >>> I am extending both, it's not a "third scenario". >>> >>> I said in the previous discussion I am willing to update the >>> documentation to match the implementation, but that's about it. >>> So far almost all PCI device drivers in U-Boot supports both scenarios, except PCI UART which currently only supports scenario#1. If you declare what you do in this patch is pureblood then you should revoke the other 2 at the same time. Leaving such in the mainstream only creates chaos in the future and we should avoid doing that, given we already had lots of discussion here. >>> >>> No. The compatible is valid for some PCI subdevs, ie. bridges, so that >>> has to stay. You also need compatible for sandbox, you said that >>> yourself. And declaring a PCI device with BFD in DT is needed, ie. for >>> the r8a7794-style bindings. >>> >> >> OK, now new comments :) So you admit "compatible" can be valid for >> some cases. > > _some_ select ones, and that is a _very_ important distinction. > >> I have to point out that your theory on PCI device probing >> is self-contradictory.You refuse to add a "compatible" string to your >> PCI device because you said the vendor/device id/class provides enough >> information to bind the driver, then why do you want to specify your >> devices in the device tree in the first place. > > Because of the USB PHY , which is attached to that device and can not be > probed on the PCI bus, unlike the device itself ? > You can create a new EHCI PCI driver to match your USB EHCI controller's vendor id/device id, and do any additional PHY setup in that driver. BTW what's the vendor id and device id of the EHCI controller on your board? I can have a look up at the PCI database. >> According to your >> theory, "Each PCI device already has a PCI ID and class which is used >> to identify it and based on which the drivers bind to it, so a DT >> compatible is NOT needed and is actually redundant and harmful.", I >> would argue why not just creating a dedicate PCI device driver using >> PCI ID and class information to do the driver binding and start from >> there. > > Because the same device with the same PCI ID can be used without that PHY . > >> As you mentioned PCI bus is probable bus like USB, everything >> can be self-contained in the PCI device driver itself. There is no >> need to create nothing in the device tree. If you want an example, >> check 8250_pci.c in the Linux kernel. Everything that is needed to >> configure the driver is included in the driver itself. It does not >> read anything from the device tree. > > Well, see above for why this approach doesn't work. Please explain why it does not work. I see Intel e1000 driver in the Linux kernel has lots of PHY configuration within the driver. It does not use any external PHY (eg: Marvell 88exxx) driver that Linux already has. The Ethernet PHY are not probeable on the PCI bus too. > >>> So again, how is the state of PCI more incomplete with this patch than >>> without it ? >>> >> Yes, you probably don't care about this. But I care about this >> as it impacts some other things like PCI uart driver which is used by >> some x86 boards. > > Sandbox changes impact PCI UART driver and x86 boards how ? See above. It's not about sandbox! >>> >>> So what is it about ? And how does this core change impact anything ? >>>
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/21/2018 08:29 PM, Simon Glass wrote:
> Hi Marek,
>
> On 21 August 2018 at 12:26, Marek Vasut wrote:
>>
>> On 08/21/2018 07:32 PM, Simon Glass wrote:
>>> Hi Bin,
>>>
>>> On 20 August 2018 at 21:46, Bin Meng wrote:
Hi Simon,
On Tue, Aug 21, 2018 at 3:29 AM, Simon Glass wrote:
> Hi Marek,
>
> On 20 August 2018 at 12:42, Marek Vasut wrote:
>>
>> On 08/20/2018 06:57 PM, Simon Glass wrote:
>>> Hi Bin,
>>>
>>> On 16 August 2018 at 19:51, Bin Meng wrote:
Hi Marek,
On Thu, Aug 16, 2018 at 7:47 PM, Marek Vasut
wrote:
> On 08/15/2018 01:25 PM, Tom Rini wrote:
>> On Wed, Aug 15, 2018 at 06:19:25PM +0800, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Wed, Aug 15, 2018 at 5:22 PM, Marek Vasut
>>> wrote:
On 08/14/2018 11:40 AM, Bin Meng wrote:
> Hi Marek,
>
> On Tue, Aug 14, 2018 at 4:55 PM, Marek Vasut
> wrote:
>> On 08/14/2018 03:46 AM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Mon, Aug 13, 2018 at 9:46 PM, Marek Vasut
>>> wrote:
On 08/13/2018 04:24 AM, Bin Meng wrote:
> Hi Marek,
>
> On Fri, Aug 10, 2018 at 8:38 PM, Marek Vasut
> wrote:
>> On 08/10/2018 02:01 PM, Tom Rini wrote:
>>> On Wed, Aug 08, 2018 at 09:37:25PM +0200, Marek Vasut wrote:
On 08/08/2018 05:32 PM, Bin Meng wrote:
> Hi Marek,
>
> On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut
> wrote:
>> On 08/08/2018 03:39 PM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut
>>> wrote:
On 08/08/2018 03:14 PM, Bin Meng wrote:
> Hi Marek,
>
> On Wed, Aug 8, 2018 at 9:03 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
>>
>> Cc: Simon Glass
>> ---
>> drivers/pci/pci-uclass.c | 14 ++
>> 1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/pci/pci-uclass.c
>> b/drivers/pci/pci-uclass.c
>> index 46e9c71bdf..306bea0dbf 100644
>> --- a/drivers/pci/pci-uclass.c
>> +++ b/drivers/pci/pci-uclass.c
>> @@ -662,6 +662,8 @@ static int
>> pci_find_and_bind_driver(struct udevice *parent,
>> for (id = entry->match;
>> id->vendor || id->subvendor ||
>> id->class_mask;
>> id++) {
>> + ofnode node;
>> +
>> if (!pci_match_one_id(id,
>> find_id))
>> continue;
>>
>> @@ -691,6 +693,18 @@ static int
>> pci_find_and_bind_driver(struct udevice *parent,
>> goto error;
>> debug("%s: Match found:
>> %s\n", __func__, drv->name);
>> dev->driver_data =
>> find_id->driver_data;
>> +
>> + dev_for_each_subnode(node,
>> parent) {
>> + phys_addr_t df, size;
>> + df =
>> ofnode_get_addr_size(node, "reg", );
>> +
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Marek,
On 21 August 2018 at 12:26, Marek Vasut wrote:
>
> On 08/21/2018 07:32 PM, Simon Glass wrote:
> > Hi Bin,
> >
> > On 20 August 2018 at 21:46, Bin Meng wrote:
> >> Hi Simon,
> >>
> >> On Tue, Aug 21, 2018 at 3:29 AM, Simon Glass wrote:
> >>> Hi Marek,
> >>>
> >>> On 20 August 2018 at 12:42, Marek Vasut wrote:
>
> On 08/20/2018 06:57 PM, Simon Glass wrote:
> > Hi Bin,
> >
> > On 16 August 2018 at 19:51, Bin Meng wrote:
> >> Hi Marek,
> >>
> >> On Thu, Aug 16, 2018 at 7:47 PM, Marek Vasut
> >> wrote:
> >>> On 08/15/2018 01:25 PM, Tom Rini wrote:
> On Wed, Aug 15, 2018 at 06:19:25PM +0800, Bin Meng wrote:
> > Hi Marek,
> >
> > On Wed, Aug 15, 2018 at 5:22 PM, Marek Vasut
> > wrote:
> >> On 08/14/2018 11:40 AM, Bin Meng wrote:
> >>> Hi Marek,
> >>>
> >>> On Tue, Aug 14, 2018 at 4:55 PM, Marek Vasut
> >>> wrote:
> On 08/14/2018 03:46 AM, Bin Meng wrote:
> > Hi Marek,
> >
> > On Mon, Aug 13, 2018 at 9:46 PM, Marek Vasut
> > wrote:
> >> On 08/13/2018 04:24 AM, Bin Meng wrote:
> >>> Hi Marek,
> >>>
> >>> On Fri, Aug 10, 2018 at 8:38 PM, Marek Vasut
> >>> wrote:
> On 08/10/2018 02:01 PM, Tom Rini wrote:
> > On Wed, Aug 08, 2018 at 09:37:25PM +0200, Marek Vasut wrote:
> >> On 08/08/2018 05:32 PM, Bin Meng wrote:
> >>> Hi Marek,
> >>>
> >>> On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut
> >>> wrote:
> On 08/08/2018 03:39 PM, Bin Meng wrote:
> > Hi Marek,
> >
> > On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut
> > wrote:
> >> On 08/08/2018 03:14 PM, Bin Meng wrote:
> >>> Hi Marek,
> >>>
> >>> On Wed, Aug 8, 2018 at 9:03 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
>
> Cc: Simon Glass
> ---
> drivers/pci/pci-uclass.c | 14 ++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/pci/pci-uclass.c
> b/drivers/pci/pci-uclass.c
> index 46e9c71bdf..306bea0dbf 100644
> --- a/drivers/pci/pci-uclass.c
> +++ b/drivers/pci/pci-uclass.c
> @@ -662,6 +662,8 @@ static int
> pci_find_and_bind_driver(struct udevice *parent,
> for (id = entry->match;
> id->vendor || id->subvendor ||
> id->class_mask;
> id++) {
> + ofnode node;
> +
> if (!pci_match_one_id(id,
> find_id))
> continue;
>
> @@ -691,6 +693,18 @@ static int
> pci_find_and_bind_driver(struct udevice *parent,
> goto error;
> debug("%s: Match found:
> %s\n", __func__, drv->name);
> dev->driver_data =
> find_id->driver_data;
> +
> + dev_for_each_subnode(node,
> parent) {
> + phys_addr_t df, size;
> + df =
> ofnode_get_addr_size(node, "reg", );
> +
> + if
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/21/2018 07:32 PM, Simon Glass wrote:
> Hi Bin,
>
> On 20 August 2018 at 21:46, Bin Meng wrote:
>> Hi Simon,
>>
>> On Tue, Aug 21, 2018 at 3:29 AM, Simon Glass wrote:
>>> Hi Marek,
>>>
>>> On 20 August 2018 at 12:42, Marek Vasut wrote:
On 08/20/2018 06:57 PM, Simon Glass wrote:
> Hi Bin,
>
> On 16 August 2018 at 19:51, Bin Meng wrote:
>> Hi Marek,
>>
>> On Thu, Aug 16, 2018 at 7:47 PM, Marek Vasut
>> wrote:
>>> On 08/15/2018 01:25 PM, Tom Rini wrote:
On Wed, Aug 15, 2018 at 06:19:25PM +0800, Bin Meng wrote:
> Hi Marek,
>
> On Wed, Aug 15, 2018 at 5:22 PM, Marek Vasut
> wrote:
>> On 08/14/2018 11:40 AM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Tue, Aug 14, 2018 at 4:55 PM, Marek Vasut
>>> wrote:
On 08/14/2018 03:46 AM, Bin Meng wrote:
> Hi Marek,
>
> On Mon, Aug 13, 2018 at 9:46 PM, Marek Vasut
> wrote:
>> On 08/13/2018 04:24 AM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Fri, Aug 10, 2018 at 8:38 PM, Marek Vasut
>>> wrote:
On 08/10/2018 02:01 PM, Tom Rini wrote:
> On Wed, Aug 08, 2018 at 09:37:25PM +0200, Marek Vasut wrote:
>> On 08/08/2018 05:32 PM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut
>>> wrote:
On 08/08/2018 03:39 PM, Bin Meng wrote:
> Hi Marek,
>
> On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut
> wrote:
>> On 08/08/2018 03:14 PM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Wed, Aug 8, 2018 at 9:03 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
Cc: Simon Glass
---
drivers/pci/pci-uclass.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/drivers/pci/pci-uclass.c
b/drivers/pci/pci-uclass.c
index 46e9c71bdf..306bea0dbf 100644
--- a/drivers/pci/pci-uclass.c
+++ b/drivers/pci/pci-uclass.c
@@ -662,6 +662,8 @@ static int
pci_find_and_bind_driver(struct udevice *parent,
for (id = entry->match;
id->vendor || id->subvendor ||
id->class_mask;
id++) {
+ ofnode node;
+
if (!pci_match_one_id(id,
find_id))
continue;
@@ -691,6 +693,18 @@ static int
pci_find_and_bind_driver(struct udevice *parent,
goto error;
debug("%s: Match found: %s\n",
__func__, drv->name);
dev->driver_data =
find_id->driver_data;
+
+ dev_for_each_subnode(node,
parent) {
+ phys_addr_t df, size;
+ df =
ofnode_get_addr_size(node, "reg", );
+
+ if (PCI_FUNC(df) ==
PCI_FUNC(bdf) &&
+ PCI_DEV(df) ==
PCI_DEV(bdf)) {
+ dev->node =
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Bin,
On 20 August 2018 at 21:46, Bin Meng wrote:
> Hi Simon,
>
> On Tue, Aug 21, 2018 at 3:29 AM, Simon Glass wrote:
>> Hi Marek,
>>
>> On 20 August 2018 at 12:42, Marek Vasut wrote:
>>>
>>> On 08/20/2018 06:57 PM, Simon Glass wrote:
>>> > Hi Bin,
>>> >
>>> > On 16 August 2018 at 19:51, Bin Meng wrote:
>>> >> Hi Marek,
>>> >>
>>> >> On Thu, Aug 16, 2018 at 7:47 PM, Marek Vasut
>>> >> wrote:
>>> >>> On 08/15/2018 01:25 PM, Tom Rini wrote:
>>> On Wed, Aug 15, 2018 at 06:19:25PM +0800, Bin Meng wrote:
>>> > Hi Marek,
>>> >
>>> > On Wed, Aug 15, 2018 at 5:22 PM, Marek Vasut
>>> > wrote:
>>> >> On 08/14/2018 11:40 AM, Bin Meng wrote:
>>> >>> Hi Marek,
>>> >>>
>>> >>> On Tue, Aug 14, 2018 at 4:55 PM, Marek Vasut
>>> >>> wrote:
>>> On 08/14/2018 03:46 AM, Bin Meng wrote:
>>> > Hi Marek,
>>> >
>>> > On Mon, Aug 13, 2018 at 9:46 PM, Marek Vasut
>>> > wrote:
>>> >> On 08/13/2018 04:24 AM, Bin Meng wrote:
>>> >>> Hi Marek,
>>> >>>
>>> >>> On Fri, Aug 10, 2018 at 8:38 PM, Marek Vasut
>>> >>> wrote:
>>> On 08/10/2018 02:01 PM, Tom Rini wrote:
>>> > On Wed, Aug 08, 2018 at 09:37:25PM +0200, Marek Vasut wrote:
>>> >> On 08/08/2018 05:32 PM, Bin Meng wrote:
>>> >>> Hi Marek,
>>> >>>
>>> >>> On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut
>>> >>> wrote:
>>> On 08/08/2018 03:39 PM, Bin Meng wrote:
>>> > Hi Marek,
>>> >
>>> > On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut
>>> > wrote:
>>> >> On 08/08/2018 03:14 PM, Bin Meng wrote:
>>> >>> Hi Marek,
>>> >>>
>>> >>> On Wed, Aug 8, 2018 at 9:03 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
>>>
>>> Cc: Simon Glass
>>> ---
>>> drivers/pci/pci-uclass.c | 14 ++
>>> 1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/pci/pci-uclass.c
>>> b/drivers/pci/pci-uclass.c
>>> index 46e9c71bdf..306bea0dbf 100644
>>> --- a/drivers/pci/pci-uclass.c
>>> +++ b/drivers/pci/pci-uclass.c
>>> @@ -662,6 +662,8 @@ static int
>>> pci_find_and_bind_driver(struct udevice *parent,
>>> for (id = entry->match;
>>> id->vendor || id->subvendor ||
>>> id->class_mask;
>>> id++) {
>>> + ofnode node;
>>> +
>>> if (!pci_match_one_id(id,
>>> find_id))
>>> continue;
>>>
>>> @@ -691,6 +693,18 @@ static int
>>> pci_find_and_bind_driver(struct udevice *parent,
>>> goto error;
>>> debug("%s: Match found: %s\n",
>>> __func__, drv->name);
>>> dev->driver_data =
>>> find_id->driver_data;
>>> +
>>> + dev_for_each_subnode(node,
>>> parent) {
>>> + phys_addr_t df, size;
>>> + df =
>>> ofnode_get_addr_size(node, "reg", );
>>> +
>>> + if (PCI_FUNC(df) ==
>>> PCI_FUNC(bdf) &&
>>> + PCI_DEV(df) ==
>>> PCI_DEV(bdf)) {
>>> + dev->node =
>>> node;
>>> +
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/21/2018 09:16 AM, Bin Meng wrote: > Hi Marek, > > On Tue, Aug 21, 2018 at 1:43 PM, Marek Vasut wrote: >> On 08/21/2018 06:56 AM, Bin Meng wrote: >> [...] >>> The proposal I made is: >>> >>> * Keep pci-uclass driver's post_bind() and child_post_bind() only for >>> Sandbox configuration >>> * Keep the call to pci_bus_find_devfn() in pci_bind_bus_devices() only >>> for Sandbox configuration >>> * Sandbox is special. We should limit the mechanism of matching PCI >>> emulation device via "compatible" to sandbox only >> >> The above three points can be done separately and I don't care about >> this too much. >> > > That's my concern. Only doing this created confusing and incomplete > design. Incomplete how ? >>> >>> I explained several times. I have to repeat here once again: >>> >>> Currently U-Boot supports two scenarios for PCI driver binding: >>> >>> - Declare a PCI device in the device tree. That requires specifying a >>> 'compatible' string as well as 'reg' property as defined by the 'PCI >>> Bus Binding' spec. DM uses the 'compatible' string to bind the driver >>> for the device. >>> - Don't declare a PCI device in the device tree. Instead, using >>> U_BOOT_PCI_DEVICE() to declare a device and driver mapping. >>> >>> You are creating a 3rd scenario which is a mix up of both scenario#1 >>> and #2 in this patch, yet without any documentation, plus any >>> additional changes to impacted drivers. >> >> I am extending both, it's not a "third scenario". >> >> I said in the previous discussion I am willing to update the >> documentation to match the implementation, but that's about it. >> >>> So far almost all PCI device drivers in U-Boot supports both >>> scenarios, except PCI UART which currently only supports scenario#1. >>> If you declare what you do in this patch is pureblood then you should >>> revoke the other 2 at the same time. Leaving such in the mainstream >>> only creates chaos in the future and we should avoid doing that, given >>> we already had lots of discussion here. >> >> No. The compatible is valid for some PCI subdevs, ie. bridges, so that >> has to stay. You also need compatible for sandbox, you said that >> yourself. And declaring a PCI device with BFD in DT is needed, ie. for >> the r8a7794-style bindings. >> > > OK, now new comments :) So you admit "compatible" can be valid for > some cases. _some_ select ones, and that is a _very_ important distinction. > I have to point out that your theory on PCI device probing > is self-contradictory.You refuse to add a "compatible" string to your > PCI device because you said the vendor/device id/class provides enough > information to bind the driver, then why do you want to specify your > devices in the device tree in the first place. Because of the USB PHY , which is attached to that device and can not be probed on the PCI bus, unlike the device itself ? > According to your > theory, "Each PCI device already has a PCI ID and class which is used > to identify it and based on which the drivers bind to it, so a DT > compatible is NOT needed and is actually redundant and harmful.", I > would argue why not just creating a dedicate PCI device driver using > PCI ID and class information to do the driver binding and start from > there. Because the same device with the same PCI ID can be used without that PHY . > As you mentioned PCI bus is probable bus like USB, everything > can be self-contained in the PCI device driver itself. There is no > need to create nothing in the device tree. If you want an example, > check 8250_pci.c in the Linux kernel. Everything that is needed to > configure the driver is included in the driver itself. It does not > read anything from the device tree. Well, see above for why this approach doesn't work. >> So again, how is the state of PCI more incomplete with this patch than >> without it ? >> > Yes, you probably don't care about this. But I care about this > as it impacts some other things like PCI uart driver which is used by > some x86 boards. Sandbox changes impact PCI UART driver and x86 boards how ? >>> >>> See above. It's not about sandbox! >> >> So what is it about ? And how does this core change impact anything ? >> Does it change behavior for some board or break some tests for you ? >> >>> * Create DM PCI test case against the DT node assignment >>> * Remove all compatible string in U-Boot's PCI device drivers: eg: >>> ehci_pci_ids[], xhci_pci_ids[], etc. IOW, all PCI device drivers >>> should only use U_BOOT_PCI_DEVICE(), aka the original U-Boot option 2 >>> * Fork a "pci-ns16550" driver to support U_BOOT_PCI_DEVICE(), as >>> currently PCI ns16550 device driver uses "compatible" string to do the >>> matching, and update crownbay.dts and galileo.dts (so far I only know >>> two boards are using PCI ns16550 serial port) >>> * Make sure all DM PCI test cases are not broken
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Marek, On Tue, Aug 21, 2018 at 1:43 PM, Marek Vasut wrote: > On 08/21/2018 06:56 AM, Bin Meng wrote: > [...] >> The proposal I made is: >> >> * Keep pci-uclass driver's post_bind() and child_post_bind() only for >> Sandbox configuration >> * Keep the call to pci_bus_find_devfn() in pci_bind_bus_devices() only >> for Sandbox configuration >> * Sandbox is special. We should limit the mechanism of matching PCI >> emulation device via "compatible" to sandbox only > > The above three points can be done separately and I don't care about > this too much. > That's my concern. Only doing this created confusing and incomplete design. >>> >>> Incomplete how ? >> >> I explained several times. I have to repeat here once again: >> >> Currently U-Boot supports two scenarios for PCI driver binding: >> >> - Declare a PCI device in the device tree. That requires specifying a >> 'compatible' string as well as 'reg' property as defined by the 'PCI >> Bus Binding' spec. DM uses the 'compatible' string to bind the driver >> for the device. >> - Don't declare a PCI device in the device tree. Instead, using >> U_BOOT_PCI_DEVICE() to declare a device and driver mapping. >> >> You are creating a 3rd scenario which is a mix up of both scenario#1 >> and #2 in this patch, yet without any documentation, plus any >> additional changes to impacted drivers. > > I am extending both, it's not a "third scenario". > > I said in the previous discussion I am willing to update the > documentation to match the implementation, but that's about it. > >> So far almost all PCI device drivers in U-Boot supports both >> scenarios, except PCI UART which currently only supports scenario#1. >> If you declare what you do in this patch is pureblood then you should >> revoke the other 2 at the same time. Leaving such in the mainstream >> only creates chaos in the future and we should avoid doing that, given >> we already had lots of discussion here. > > No. The compatible is valid for some PCI subdevs, ie. bridges, so that > has to stay. You also need compatible for sandbox, you said that > yourself. And declaring a PCI device with BFD in DT is needed, ie. for > the r8a7794-style bindings. > OK, now new comments :) So you admit "compatible" can be valid for some cases. I have to point out that your theory on PCI device probing is self-contradictory. You refuse to add a "compatible" string to your PCI device because you said the vendor/device id/class provides enough information to bind the driver, then why do you want to specify your devices in the device tree in the first place. According to your theory, "Each PCI device already has a PCI ID and class which is used to identify it and based on which the drivers bind to it, so a DT compatible is NOT needed and is actually redundant and harmful.", I would argue why not just creating a dedicate PCI device driver using PCI ID and class information to do the driver binding and start from there. As you mentioned PCI bus is probable bus like USB, everything can be self-contained in the PCI device driver itself. There is no need to create nothing in the device tree. If you want an example, check 8250_pci.c in the Linux kernel. Everything that is needed to configure the driver is included in the driver itself. It does not read anything from the device tree. > So again, how is the state of PCI more incomplete with this patch than > without it ? > Yes, you probably don't care about this. But I care about this as it impacts some other things like PCI uart driver which is used by some x86 boards. >>> >>> Sandbox changes impact PCI UART driver and x86 boards how ? >> >> See above. It's not about sandbox! > > So what is it about ? And how does this core change impact anything ? > Does it change behavior for some board or break some tests for you ? > >> * Create DM PCI test case against the DT node assignment >> * Remove all compatible string in U-Boot's PCI device drivers: eg: >> ehci_pci_ids[], xhci_pci_ids[], etc. IOW, all PCI device drivers >> should only use U_BOOT_PCI_DEVICE(), aka the original U-Boot option 2 >> * Fork a "pci-ns16550" driver to support U_BOOT_PCI_DEVICE(), as >> currently PCI ns16550 device driver uses "compatible" string to do the >> matching, and update crownbay.dts and galileo.dts (so far I only know >> two boards are using PCI ns16550 serial port) >> * Make sure all DM PCI test cases are not broken >> * Document all of the above changes in doc/driver-model/pci-info.txt > > I think you're just adding completely orthogonal stuff to this 5-liner > patch into the list and overly complicate the situation. Sure, if you > want to do all this, go ahead, but I don't see how this prevents this > particular patch from being applied , except maybe for the documentation > tweak. Please, think from the maintainer perspective. It's nothing related to
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/21/2018 06:56 AM, Bin Meng wrote: [...] > The proposal I made is: > > * Keep pci-uclass driver's post_bind() and child_post_bind() only for > Sandbox configuration > * Keep the call to pci_bus_find_devfn() in pci_bind_bus_devices() only > for Sandbox configuration > * Sandbox is special. We should limit the mechanism of matching PCI > emulation device via "compatible" to sandbox only The above three points can be done separately and I don't care about this too much. >>> >>> That's my concern. Only doing this created confusing and incomplete >>> design. >> >> Incomplete how ? > > I explained several times. I have to repeat here once again: > > Currently U-Boot supports two scenarios for PCI driver binding: > > - Declare a PCI device in the device tree. That requires specifying a > 'compatible' string as well as 'reg' property as defined by the 'PCI > Bus Binding' spec. DM uses the 'compatible' string to bind the driver > for the device. > - Don't declare a PCI device in the device tree. Instead, using > U_BOOT_PCI_DEVICE() to declare a device and driver mapping. > > You are creating a 3rd scenario which is a mix up of both scenario#1 > and #2 in this patch, yet without any documentation, plus any > additional changes to impacted drivers. I am extending both, it's not a "third scenario". I said in the previous discussion I am willing to update the documentation to match the implementation, but that's about it. > So far almost all PCI device drivers in U-Boot supports both > scenarios, except PCI UART which currently only supports scenario#1. > If you declare what you do in this patch is pureblood then you should > revoke the other 2 at the same time. Leaving such in the mainstream > only creates chaos in the future and we should avoid doing that, given > we already had lots of discussion here. No. The compatible is valid for some PCI subdevs, ie. bridges, so that has to stay. You also need compatible for sandbox, you said that yourself. And declaring a PCI device with BFD in DT is needed, ie. for the r8a7794-style bindings. So again, how is the state of PCI more incomplete with this patch than without it ? >>> Yes, you probably don't care about this. But I care about this >>> as it impacts some other things like PCI uart driver which is used by >>> some x86 boards. >> >> Sandbox changes impact PCI UART driver and x86 boards how ? > > See above. It's not about sandbox! So what is it about ? And how does this core change impact anything ? Does it change behavior for some board or break some tests for you ? > * Create DM PCI test case against the DT node assignment > * Remove all compatible string in U-Boot's PCI device drivers: eg: > ehci_pci_ids[], xhci_pci_ids[], etc. IOW, all PCI device drivers > should only use U_BOOT_PCI_DEVICE(), aka the original U-Boot option 2 > * Fork a "pci-ns16550" driver to support U_BOOT_PCI_DEVICE(), as > currently PCI ns16550 device driver uses "compatible" string to do the > matching, and update crownbay.dts and galileo.dts (so far I only know > two boards are using PCI ns16550 serial port) > * Make sure all DM PCI test cases are not broken > * Document all of the above changes in doc/driver-model/pci-info.txt I think you're just adding completely orthogonal stuff to this 5-liner patch into the list and overly complicate the situation. Sure, if you want to do all this, go ahead, but I don't see how this prevents this particular patch from being applied , except maybe for the documentation tweak. >>> >>> Please, think from the maintainer perspective. It's nothing related to >>> number of LOCs. It's that we need create a clean design. You probably >>> don't want someone just submits a single patch that changed current >>> USB design without a full consideration on all possible impacts. >> >> OK, after reading all this, what I gather from the discussion is that >> this patch somehow breaks the design or the other platforms, which is >> why you fight against it so much. Does it, yes or no? And if so, you >> still failed to explain how, so, how ? You did not answer this question. >> Improving sandbox and x86 PCI handling is orthogonal to this change, so >> are ns16550 driver improvements and if you want to work on them, nothing >> prevents you from doing so. > > First of all, it's not x86 PCI handling. It's PCI device driver clean > up to match the new design. It is not a new design, just an extension of the current one. Driver cleanups where you match on PCI IDs or classes instead of compatible strings are completely orthogonal to this change, you don't even need this patch to do such a cleanup. Same applies for the x86 DT cleanup, you don't need this patch for those either. So please stop lumping those together with this patch, they are unrelated. > Second, doing such is necessary to make > the design clean and complete. No, doing so
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Marek, On Tue, Aug 21, 2018 at 12:30 PM, Marek Vasut wrote: > On 08/21/2018 06:15 AM, Bin Meng wrote: > [...] > >>> From what I understand, Marek and Bin are discussing whether a >>> compatible string is needed to bind a driver. >>> >>> Generally it is. But with PCI and USB we have a search mechanism which >>> can be used instead. >>> >>> The patch Marek submitted does not seems at all desirable to me. >> >> Can you explain why ? > > We already have a compatible string as the standard way to attach > drivers to devices. > > For PCI, we already have PCI_DEVICE() and friends for when we can > attach a driver for a PCI device without using a compatible string. > > Both of these are defined in the DT specification. > > The patch seems to be a rework of PCI_DEVICE() and I cannot why it is > necessary. > >> >>> I would like to see what Bin proposes. >> >> Me too, so far I only see "not Marek's patch" and no real alternative. > > Bin, do you have a patch you can share? No, I don't have any patch series for now, although I offered to work on a series to implement my proposal. I haven't started it as I wanted to hear your thoughts. The proposal I made is to satisfy the requirement that Marek insisted on. Basically Marek thought current DM PCI implementation is wrong to ask for a "compatible" string of a PCI device in the device tree, because he thought adding "compatible" to DT is invalid and Linux does not do that either. While I disagree we have to 100% follow Linux's implementation, I am still open for any possible design changes, if that's the preferable practice in U-Boot (but we have to make it clear and document this officially somewhere). The proposal I made is: * Keep pci-uclass driver's post_bind() and child_post_bind() only for Sandbox configuration * Keep the call to pci_bus_find_devfn() in pci_bind_bus_devices() only for Sandbox configuration * Sandbox is special. We should limit the mechanism of matching PCI emulation device via "compatible" to sandbox only >>> >>> The above three points can be done separately and I don't care about >>> this too much. >>> >> >> That's my concern. Only doing this created confusing and incomplete >> design. > > Incomplete how ? I explained several times. I have to repeat here once again: Currently U-Boot supports two scenarios for PCI driver binding: - Declare a PCI device in the device tree. That requires specifying a 'compatible' string as well as 'reg' property as defined by the 'PCI Bus Binding' spec. DM uses the 'compatible' string to bind the driver for the device. - Don't declare a PCI device in the device tree. Instead, using U_BOOT_PCI_DEVICE() to declare a device and driver mapping. You are creating a 3rd scenario which is a mix up of both scenario#1 and #2 in this patch, yet without any documentation, plus any additional changes to impacted drivers. So far almost all PCI device drivers in U-Boot supports both scenarios, except PCI UART which currently only supports scenario#1. If you declare what you do in this patch is pureblood then you should revoke the other 2 at the same time. Leaving such in the mainstream only creates chaos in the future and we should avoid doing that, given we already had lots of discussion here. > >> Yes, you probably don't care about this. But I care about this >> as it impacts some other things like PCI uart driver which is used by >> some x86 boards. > > Sandbox changes impact PCI UART driver and x86 boards how ? See above. It's not about sandbox! > * Assign the DT node to the bound device in pci_find_and_bind_driver() if there is a valid PCI "reg" encoding for a specific PCI device in the device tree >>> >>> This is what this patch does. And in fact, I have real hardware which >>> needs this patch to be useful and on which I can test if this works. >>> >> >> You can do this in your own fork. Nothing prevents you from doing that. > > I am at loss for words, really. > * Create DM PCI test case against the DT node assignment * Remove all compatible string in U-Boot's PCI device drivers: eg: ehci_pci_ids[], xhci_pci_ids[], etc. IOW, all PCI device drivers should only use U_BOOT_PCI_DEVICE(), aka the original U-Boot option 2 * Fork a "pci-ns16550" driver to support U_BOOT_PCI_DEVICE(), as currently PCI ns16550 device driver uses "compatible" string to do the matching, and update crownbay.dts and galileo.dts (so far I only know two boards are using PCI ns16550 serial port) * Make sure all DM PCI test cases are not broken * Document all of the above changes in doc/driver-model/pci-info.txt >>> >>> I think you're just adding completely orthogonal stuff to this 5-liner >>> patch into the list and overly complicate the situation. Sure, if you >>> want to do
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/21/2018 06:15 AM, Bin Meng wrote: [...] >> From what I understand, Marek and Bin are discussing whether a >> compatible string is needed to bind a driver. >> >> Generally it is. But with PCI and USB we have a search mechanism which >> can be used instead. >> >> The patch Marek submitted does not seems at all desirable to me. > > Can you explain why ? We already have a compatible string as the standard way to attach drivers to devices. For PCI, we already have PCI_DEVICE() and friends for when we can attach a driver for a PCI device without using a compatible string. Both of these are defined in the DT specification. The patch seems to be a rework of PCI_DEVICE() and I cannot why it is necessary. > >> I would like to see what Bin proposes. > > Me too, so far I only see "not Marek's patch" and no real alternative. Bin, do you have a patch you can share? >>> >>> No, I don't have any patch series for now, although I offered to work >>> on a series to implement my proposal. I haven't started it as I wanted >>> to hear your thoughts. The proposal I made is to satisfy the >>> requirement that Marek insisted on. Basically Marek thought current DM >>> PCI implementation is wrong to ask for a "compatible" string of a PCI >>> device in the device tree, because he thought adding "compatible" to >>> DT is invalid and Linux does not do that either. While I disagree we >>> have to 100% follow Linux's implementation, I am still open for any >>> possible design changes, if that's the preferable practice in U-Boot >>> (but we have to make it clear and document this officially somewhere). >>> >>> The proposal I made is: >>> >>> * Keep pci-uclass driver's post_bind() and child_post_bind() only for >>> Sandbox configuration >>> * Keep the call to pci_bus_find_devfn() in pci_bind_bus_devices() only >>> for Sandbox configuration >>> * Sandbox is special. We should limit the mechanism of matching PCI >>> emulation device via "compatible" to sandbox only >> >> The above three points can be done separately and I don't care about >> this too much. >> > > That's my concern. Only doing this created confusing and incomplete > design. Incomplete how ? > Yes, you probably don't care about this. But I care about this > as it impacts some other things like PCI uart driver which is used by > some x86 boards. Sandbox changes impact PCI UART driver and x86 boards how ? >>> * Assign the DT node to the bound device in pci_find_and_bind_driver() >>> if there is a valid PCI "reg" encoding for a specific PCI device in >>> the device tree >> >> This is what this patch does. And in fact, I have real hardware which >> needs this patch to be useful and on which I can test if this works. >> > > You can do this in your own fork. Nothing prevents you from doing that. I am at loss for words, really. >>> * Create DM PCI test case against the DT node assignment >>> * Remove all compatible string in U-Boot's PCI device drivers: eg: >>> ehci_pci_ids[], xhci_pci_ids[], etc. IOW, all PCI device drivers >>> should only use U_BOOT_PCI_DEVICE(), aka the original U-Boot option 2 >>> * Fork a "pci-ns16550" driver to support U_BOOT_PCI_DEVICE(), as >>> currently PCI ns16550 device driver uses "compatible" string to do the >>> matching, and update crownbay.dts and galileo.dts (so far I only know >>> two boards are using PCI ns16550 serial port) >>> * Make sure all DM PCI test cases are not broken >>> * Document all of the above changes in doc/driver-model/pci-info.txt >> >> I think you're just adding completely orthogonal stuff to this 5-liner >> patch into the list and overly complicate the situation. Sure, if you >> want to do all this, go ahead, but I don't see how this prevents this >> particular patch from being applied , except maybe for the documentation >> tweak. > > Please, think from the maintainer perspective. It's nothing related to > number of LOCs. It's that we need create a clean design. You probably > don't want someone just submits a single patch that changed current > USB design without a full consideration on all possible impacts. OK, after reading all this, what I gather from the discussion is that this patch somehow breaks the design or the other platforms, which is why you fight against it so much. Does it, yes or no? And if so, you still failed to explain how, so, how ? Improving sandbox and x86 PCI handling is orthogonal to this change, so are ns16550 driver improvements and if you want to work on them, nothing prevents you from doing so. -- Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Marek,
On Tue, Aug 21, 2018 at 12:02 PM, Marek Vasut wrote:
> On 08/21/2018 05:46 AM, Bin Meng wrote:
>> Hi Simon,
>>
>> On Tue, Aug 21, 2018 at 3:29 AM, Simon Glass wrote:
>>> Hi Marek,
>>>
>>> On 20 August 2018 at 12:42, Marek Vasut wrote:
On 08/20/2018 06:57 PM, Simon Glass wrote:
> Hi Bin,
>
> On 16 August 2018 at 19:51, Bin Meng wrote:
>> Hi Marek,
>>
>> On Thu, Aug 16, 2018 at 7:47 PM, Marek Vasut
>> wrote:
>>> On 08/15/2018 01:25 PM, Tom Rini wrote:
On Wed, Aug 15, 2018 at 06:19:25PM +0800, Bin Meng wrote:
> Hi Marek,
>
> On Wed, Aug 15, 2018 at 5:22 PM, Marek Vasut
> wrote:
>> On 08/14/2018 11:40 AM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Tue, Aug 14, 2018 at 4:55 PM, Marek Vasut
>>> wrote:
On 08/14/2018 03:46 AM, Bin Meng wrote:
> Hi Marek,
>
> On Mon, Aug 13, 2018 at 9:46 PM, Marek Vasut
> wrote:
>> On 08/13/2018 04:24 AM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Fri, Aug 10, 2018 at 8:38 PM, Marek Vasut
>>> wrote:
On 08/10/2018 02:01 PM, Tom Rini wrote:
> On Wed, Aug 08, 2018 at 09:37:25PM +0200, Marek Vasut wrote:
>> On 08/08/2018 05:32 PM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut
>>> wrote:
On 08/08/2018 03:39 PM, Bin Meng wrote:
> Hi Marek,
>
> On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut
> wrote:
>> On 08/08/2018 03:14 PM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Wed, Aug 8, 2018 at 9:03 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
Cc: Simon Glass
---
drivers/pci/pci-uclass.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/drivers/pci/pci-uclass.c
b/drivers/pci/pci-uclass.c
index 46e9c71bdf..306bea0dbf 100644
--- a/drivers/pci/pci-uclass.c
+++ b/drivers/pci/pci-uclass.c
@@ -662,6 +662,8 @@ static int
pci_find_and_bind_driver(struct udevice *parent,
for (id = entry->match;
id->vendor || id->subvendor ||
id->class_mask;
id++) {
+ ofnode node;
+
if (!pci_match_one_id(id,
find_id))
continue;
@@ -691,6 +693,18 @@ static int
pci_find_and_bind_driver(struct udevice *parent,
goto error;
debug("%s: Match found: %s\n",
__func__, drv->name);
dev->driver_data =
find_id->driver_data;
+
+ dev_for_each_subnode(node,
parent) {
+ phys_addr_t df, size;
+ df =
ofnode_get_addr_size(node, "reg", );
+
+ if (PCI_FUNC(df) ==
PCI_FUNC(bdf) &&
+ PCI_DEV(df) ==
PCI_DEV(bdf)) {
+ dev->node =
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/21/2018 05:46 AM, Bin Meng wrote:
> Hi Simon,
>
> On Tue, Aug 21, 2018 at 3:29 AM, Simon Glass wrote:
>> Hi Marek,
>>
>> On 20 August 2018 at 12:42, Marek Vasut wrote:
>>>
>>> On 08/20/2018 06:57 PM, Simon Glass wrote:
Hi Bin,
On 16 August 2018 at 19:51, Bin Meng wrote:
> Hi Marek,
>
> On Thu, Aug 16, 2018 at 7:47 PM, Marek Vasut
> wrote:
>> On 08/15/2018 01:25 PM, Tom Rini wrote:
>>> On Wed, Aug 15, 2018 at 06:19:25PM +0800, Bin Meng wrote:
Hi Marek,
On Wed, Aug 15, 2018 at 5:22 PM, Marek Vasut
wrote:
> On 08/14/2018 11:40 AM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Tue, Aug 14, 2018 at 4:55 PM, Marek Vasut
>> wrote:
>>> On 08/14/2018 03:46 AM, Bin Meng wrote:
Hi Marek,
On Mon, Aug 13, 2018 at 9:46 PM, Marek Vasut
wrote:
> On 08/13/2018 04:24 AM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Fri, Aug 10, 2018 at 8:38 PM, Marek Vasut
>> wrote:
>>> On 08/10/2018 02:01 PM, Tom Rini wrote:
On Wed, Aug 08, 2018 at 09:37:25PM +0200, Marek Vasut wrote:
> On 08/08/2018 05:32 PM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut
>> wrote:
>>> On 08/08/2018 03:39 PM, Bin Meng wrote:
Hi Marek,
On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut
wrote:
> On 08/08/2018 03:14 PM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Wed, Aug 8, 2018 at 9:03 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
>>>
>>> Cc: Simon Glass
>>> ---
>>> drivers/pci/pci-uclass.c | 14 ++
>>> 1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/pci/pci-uclass.c
>>> b/drivers/pci/pci-uclass.c
>>> index 46e9c71bdf..306bea0dbf 100644
>>> --- a/drivers/pci/pci-uclass.c
>>> +++ b/drivers/pci/pci-uclass.c
>>> @@ -662,6 +662,8 @@ static int
>>> pci_find_and_bind_driver(struct udevice *parent,
>>> for (id = entry->match;
>>> id->vendor || id->subvendor ||
>>> id->class_mask;
>>> id++) {
>>> + ofnode node;
>>> +
>>> if (!pci_match_one_id(id,
>>> find_id))
>>> continue;
>>>
>>> @@ -691,6 +693,18 @@ static int
>>> pci_find_and_bind_driver(struct udevice *parent,
>>> goto error;
>>> debug("%s: Match found: %s\n",
>>> __func__, drv->name);
>>> dev->driver_data =
>>> find_id->driver_data;
>>> +
>>> + dev_for_each_subnode(node,
>>> parent) {
>>> + phys_addr_t df, size;
>>> + df =
>>> ofnode_get_addr_size(node, "reg", );
>>> +
>>> + if (PCI_FUNC(df) ==
>>> PCI_FUNC(bdf) &&
>>> + PCI_DEV(df) ==
>>> PCI_DEV(bdf)) {
>>> + dev->node =
>>> node;
>>> + break;
>>> + }
>>
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Simon,
On Tue, Aug 21, 2018 at 3:29 AM, Simon Glass wrote:
> Hi Marek,
>
> On 20 August 2018 at 12:42, Marek Vasut wrote:
>>
>> On 08/20/2018 06:57 PM, Simon Glass wrote:
>> > Hi Bin,
>> >
>> > On 16 August 2018 at 19:51, Bin Meng wrote:
>> >> Hi Marek,
>> >>
>> >> On Thu, Aug 16, 2018 at 7:47 PM, Marek Vasut
>> >> wrote:
>> >>> On 08/15/2018 01:25 PM, Tom Rini wrote:
>> On Wed, Aug 15, 2018 at 06:19:25PM +0800, Bin Meng wrote:
>> > Hi Marek,
>> >
>> > On Wed, Aug 15, 2018 at 5:22 PM, Marek Vasut
>> > wrote:
>> >> On 08/14/2018 11:40 AM, Bin Meng wrote:
>> >>> Hi Marek,
>> >>>
>> >>> On Tue, Aug 14, 2018 at 4:55 PM, Marek Vasut
>> >>> wrote:
>> On 08/14/2018 03:46 AM, Bin Meng wrote:
>> > Hi Marek,
>> >
>> > On Mon, Aug 13, 2018 at 9:46 PM, Marek Vasut
>> > wrote:
>> >> On 08/13/2018 04:24 AM, Bin Meng wrote:
>> >>> Hi Marek,
>> >>>
>> >>> On Fri, Aug 10, 2018 at 8:38 PM, Marek Vasut
>> >>> wrote:
>> On 08/10/2018 02:01 PM, Tom Rini wrote:
>> > On Wed, Aug 08, 2018 at 09:37:25PM +0200, Marek Vasut wrote:
>> >> On 08/08/2018 05:32 PM, Bin Meng wrote:
>> >>> Hi Marek,
>> >>>
>> >>> On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut
>> >>> wrote:
>> On 08/08/2018 03:39 PM, Bin Meng wrote:
>> > Hi Marek,
>> >
>> > On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut
>> > wrote:
>> >> On 08/08/2018 03:14 PM, Bin Meng wrote:
>> >>> Hi Marek,
>> >>>
>> >>> On Wed, Aug 8, 2018 at 9:03 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
>>
>> Cc: Simon Glass
>> ---
>> drivers/pci/pci-uclass.c | 14 ++
>> 1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/pci/pci-uclass.c
>> b/drivers/pci/pci-uclass.c
>> index 46e9c71bdf..306bea0dbf 100644
>> --- a/drivers/pci/pci-uclass.c
>> +++ b/drivers/pci/pci-uclass.c
>> @@ -662,6 +662,8 @@ static int
>> pci_find_and_bind_driver(struct udevice *parent,
>> for (id = entry->match;
>> id->vendor || id->subvendor ||
>> id->class_mask;
>> id++) {
>> + ofnode node;
>> +
>> if (!pci_match_one_id(id,
>> find_id))
>> continue;
>>
>> @@ -691,6 +693,18 @@ static int
>> pci_find_and_bind_driver(struct udevice *parent,
>> goto error;
>> debug("%s: Match found: %s\n",
>> __func__, drv->name);
>> dev->driver_data =
>> find_id->driver_data;
>> +
>> + dev_for_each_subnode(node,
>> parent) {
>> + phys_addr_t df, size;
>> + df =
>> ofnode_get_addr_size(node, "reg", );
>> +
>> + if (PCI_FUNC(df) ==
>> PCI_FUNC(bdf) &&
>> + PCI_DEV(df) ==
>> PCI_DEV(bdf)) {
>> + dev->node =
>> node;
>> + break;
>> + }
>> >>>
>> >>> The function
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/20/2018 09:29 PM, Simon Glass wrote: [...] > So what is the conclusion here ? Patch the design document and apply > this patch as is ? > I think we should see Simon's comments before we move forward. The proposal I made before should come in a series, not just documentation. >>> >>> This thread is too long :-) >>> >>> From what I understand, Marek and Bin are discussing whether a >>> compatible string is needed to bind a driver. >>> >>> Generally it is. But with PCI and USB we have a search mechanism which >>> can be used instead. >>> >>> The patch Marek submitted does not seems at all desirable to me. >> >> Can you explain why ? > > We already have a compatible string as the standard way to attach > drivers to devices. > > For PCI, we already have PCI_DEVICE() and friends for when we can > attach a driver for a PCI device without using a compatible string. > > Both of these are defined in the DT specification. > > The patch seems to be a rework of PCI_DEVICE() and I cannot why it is > necessary. This is explained in the patch description and the thread again. Please read the thread before replying. Take a look at the r8a7794.dtsi and its PCI bindings, there are PCI controller subnodes which add extra information for PCI devices on the bus. These nodes do not have a compatible string, only a BFD. This is perfectly valid, since you can match a driver on the PCI IDs or classes (PCI_DEVICE()), but the driver doesn't have a DT node associated with it. If there is a DT node with a matching BFD, it is associated with the driver instance by this patch. This allows ie. the EHCI PCI driver to access that DT node and extract information about PHYs from the DT (in case of the r8a7794). [...] -- Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Marek,
On 20 August 2018 at 12:42, Marek Vasut wrote:
>
> On 08/20/2018 06:57 PM, Simon Glass wrote:
> > Hi Bin,
> >
> > On 16 August 2018 at 19:51, Bin Meng wrote:
> >> Hi Marek,
> >>
> >> On Thu, Aug 16, 2018 at 7:47 PM, Marek Vasut wrote:
> >>> On 08/15/2018 01:25 PM, Tom Rini wrote:
> On Wed, Aug 15, 2018 at 06:19:25PM +0800, Bin Meng wrote:
> > Hi Marek,
> >
> > On Wed, Aug 15, 2018 at 5:22 PM, Marek Vasut
> > wrote:
> >> On 08/14/2018 11:40 AM, Bin Meng wrote:
> >>> Hi Marek,
> >>>
> >>> On Tue, Aug 14, 2018 at 4:55 PM, Marek Vasut
> >>> wrote:
> On 08/14/2018 03:46 AM, Bin Meng wrote:
> > Hi Marek,
> >
> > On Mon, Aug 13, 2018 at 9:46 PM, Marek Vasut
> > wrote:
> >> On 08/13/2018 04:24 AM, Bin Meng wrote:
> >>> Hi Marek,
> >>>
> >>> On Fri, Aug 10, 2018 at 8:38 PM, Marek Vasut
> >>> wrote:
> On 08/10/2018 02:01 PM, Tom Rini wrote:
> > On Wed, Aug 08, 2018 at 09:37:25PM +0200, Marek Vasut wrote:
> >> On 08/08/2018 05:32 PM, Bin Meng wrote:
> >>> Hi Marek,
> >>>
> >>> On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut
> >>> wrote:
> On 08/08/2018 03:39 PM, Bin Meng wrote:
> > Hi Marek,
> >
> > On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut
> > wrote:
> >> On 08/08/2018 03:14 PM, Bin Meng wrote:
> >>> Hi Marek,
> >>>
> >>> On Wed, Aug 8, 2018 at 9:03 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
>
> Cc: Simon Glass
> ---
> drivers/pci/pci-uclass.c | 14 ++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/pci/pci-uclass.c
> b/drivers/pci/pci-uclass.c
> index 46e9c71bdf..306bea0dbf 100644
> --- a/drivers/pci/pci-uclass.c
> +++ b/drivers/pci/pci-uclass.c
> @@ -662,6 +662,8 @@ static int
> pci_find_and_bind_driver(struct udevice *parent,
> for (id = entry->match;
> id->vendor || id->subvendor ||
> id->class_mask;
> id++) {
> + ofnode node;
> +
> if (!pci_match_one_id(id,
> find_id))
> continue;
>
> @@ -691,6 +693,18 @@ static int
> pci_find_and_bind_driver(struct udevice *parent,
> goto error;
> debug("%s: Match found: %s\n",
> __func__, drv->name);
> dev->driver_data =
> find_id->driver_data;
> +
> + dev_for_each_subnode(node,
> parent) {
> + phys_addr_t df, size;
> + df =
> ofnode_get_addr_size(node, "reg", );
> +
> + if (PCI_FUNC(df) ==
> PCI_FUNC(bdf) &&
> + PCI_DEV(df) ==
> PCI_DEV(bdf)) {
> + dev->node = node;
> + break;
> + }
> >>>
> >>> The function pci_find_and_bind_driver() is supposed to
> >>> bind devices
> >>> that are NOT in the device tree. Adding device tree
> >>> access in this
> >>> routine is quite
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/20/2018 06:57 PM, Simon Glass wrote:
> Hi Bin,
>
> On 16 August 2018 at 19:51, Bin Meng wrote:
>> Hi Marek,
>>
>> On Thu, Aug 16, 2018 at 7:47 PM, Marek Vasut wrote:
>>> On 08/15/2018 01:25 PM, Tom Rini wrote:
On Wed, Aug 15, 2018 at 06:19:25PM +0800, Bin Meng wrote:
> Hi Marek,
>
> On Wed, Aug 15, 2018 at 5:22 PM, Marek Vasut
> wrote:
>> On 08/14/2018 11:40 AM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Tue, Aug 14, 2018 at 4:55 PM, Marek Vasut
>>> wrote:
On 08/14/2018 03:46 AM, Bin Meng wrote:
> Hi Marek,
>
> On Mon, Aug 13, 2018 at 9:46 PM, Marek Vasut
> wrote:
>> On 08/13/2018 04:24 AM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Fri, Aug 10, 2018 at 8:38 PM, Marek Vasut
>>> wrote:
On 08/10/2018 02:01 PM, Tom Rini wrote:
> On Wed, Aug 08, 2018 at 09:37:25PM +0200, Marek Vasut wrote:
>> On 08/08/2018 05:32 PM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut
>>> wrote:
On 08/08/2018 03:39 PM, Bin Meng wrote:
> Hi Marek,
>
> On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut
> wrote:
>> On 08/08/2018 03:14 PM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Wed, Aug 8, 2018 at 9:03 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
Cc: Simon Glass
---
drivers/pci/pci-uclass.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/drivers/pci/pci-uclass.c
b/drivers/pci/pci-uclass.c
index 46e9c71bdf..306bea0dbf 100644
--- a/drivers/pci/pci-uclass.c
+++ b/drivers/pci/pci-uclass.c
@@ -662,6 +662,8 @@ static int
pci_find_and_bind_driver(struct udevice *parent,
for (id = entry->match;
id->vendor || id->subvendor ||
id->class_mask;
id++) {
+ ofnode node;
+
if (!pci_match_one_id(id, find_id))
continue;
@@ -691,6 +693,18 @@ static int
pci_find_and_bind_driver(struct udevice *parent,
goto error;
debug("%s: Match found: %s\n",
__func__, drv->name);
dev->driver_data =
find_id->driver_data;
+
+ dev_for_each_subnode(node, parent)
{
+ phys_addr_t df, size;
+ df =
ofnode_get_addr_size(node, "reg", );
+
+ if (PCI_FUNC(df) ==
PCI_FUNC(bdf) &&
+ PCI_DEV(df) ==
PCI_DEV(bdf)) {
+ dev->node = node;
+ break;
+ }
>>>
>>> The function pci_find_and_bind_driver() is supposed to bind
>>> devices
>>> that are NOT in the device tree. Adding device tree access
>>> in this
>>> routine is quite odd. You can add the EHCI controller that
>>> need such
>>> PHY subnodes in the device tree and there is no need to
>>> modify
>>> anything I believe. If you are looking for an example,
>>> please check
>>> pciuart0 in
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Bin,
On 16 August 2018 at 19:51, Bin Meng wrote:
> Hi Marek,
>
> On Thu, Aug 16, 2018 at 7:47 PM, Marek Vasut wrote:
>> On 08/15/2018 01:25 PM, Tom Rini wrote:
>>> On Wed, Aug 15, 2018 at 06:19:25PM +0800, Bin Meng wrote:
Hi Marek,
On Wed, Aug 15, 2018 at 5:22 PM, Marek Vasut wrote:
> On 08/14/2018 11:40 AM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Tue, Aug 14, 2018 at 4:55 PM, Marek Vasut
>> wrote:
>>> On 08/14/2018 03:46 AM, Bin Meng wrote:
Hi Marek,
On Mon, Aug 13, 2018 at 9:46 PM, Marek Vasut
wrote:
> On 08/13/2018 04:24 AM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Fri, Aug 10, 2018 at 8:38 PM, Marek Vasut
>> wrote:
>>> On 08/10/2018 02:01 PM, Tom Rini wrote:
On Wed, Aug 08, 2018 at 09:37:25PM +0200, Marek Vasut wrote:
> On 08/08/2018 05:32 PM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut
>> wrote:
>>> On 08/08/2018 03:39 PM, Bin Meng wrote:
Hi Marek,
On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut
wrote:
> On 08/08/2018 03:14 PM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Wed, Aug 8, 2018 at 9:03 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
>>> Cc: Simon Glass
>>> ---
>>> drivers/pci/pci-uclass.c | 14 ++
>>> 1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/pci/pci-uclass.c
>>> b/drivers/pci/pci-uclass.c
>>> index 46e9c71bdf..306bea0dbf 100644
>>> --- a/drivers/pci/pci-uclass.c
>>> +++ b/drivers/pci/pci-uclass.c
>>> @@ -662,6 +662,8 @@ static int
>>> pci_find_and_bind_driver(struct udevice *parent,
>>> for (id = entry->match;
>>> id->vendor || id->subvendor ||
>>> id->class_mask;
>>> id++) {
>>> + ofnode node;
>>> +
>>> if (!pci_match_one_id(id, find_id))
>>> continue;
>>>
>>> @@ -691,6 +693,18 @@ static int
>>> pci_find_and_bind_driver(struct udevice *parent,
>>> goto error;
>>> debug("%s: Match found: %s\n",
>>> __func__, drv->name);
>>> dev->driver_data =
>>> find_id->driver_data;
>>> +
>>> + dev_for_each_subnode(node, parent) {
>>> + phys_addr_t df, size;
>>> + df =
>>> ofnode_get_addr_size(node, "reg", );
>>> +
>>> + if (PCI_FUNC(df) ==
>>> PCI_FUNC(bdf) &&
>>> + PCI_DEV(df) ==
>>> PCI_DEV(bdf)) {
>>> + dev->node = node;
>>> + break;
>>> + }
>>
>> The function pci_find_and_bind_driver() is supposed to bind
>> devices
>> that are NOT in the device tree. Adding device tree access
>> in this
>> routine is quite odd. You can add the EHCI controller that
>> need such
>> PHY subnodes in the device tree and there is no need to
>> modify
>> anything I believe. If you are looking for an example,
>> please check
>> pciuart0 in arch/x86/dts/crownbay.dts.
>
> Well this does not work for me, the EHCI PCI doesn't get a DT
> node
> assigned, check
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/20/2018 09:18 AM, Bin Meng wrote:
> Hi Marek,
>
> On Fri, Aug 17, 2018 at 6:27 PM, Marek Vasut wrote:
>> On 08/17/2018 03:51 AM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Thu, Aug 16, 2018 at 7:47 PM, Marek Vasut wrote:
On 08/15/2018 01:25 PM, Tom Rini wrote:
> On Wed, Aug 15, 2018 at 06:19:25PM +0800, Bin Meng wrote:
>> Hi Marek,
>>
>> On Wed, Aug 15, 2018 at 5:22 PM, Marek Vasut
>> wrote:
>>> On 08/14/2018 11:40 AM, Bin Meng wrote:
Hi Marek,
On Tue, Aug 14, 2018 at 4:55 PM, Marek Vasut
wrote:
> On 08/14/2018 03:46 AM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Mon, Aug 13, 2018 at 9:46 PM, Marek Vasut
>> wrote:
>>> On 08/13/2018 04:24 AM, Bin Meng wrote:
Hi Marek,
On Fri, Aug 10, 2018 at 8:38 PM, Marek Vasut
wrote:
> On 08/10/2018 02:01 PM, Tom Rini wrote:
>> On Wed, Aug 08, 2018 at 09:37:25PM +0200, Marek Vasut wrote:
>>> On 08/08/2018 05:32 PM, Bin Meng wrote:
Hi Marek,
On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut
wrote:
> On 08/08/2018 03:39 PM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut
>> wrote:
>>> On 08/08/2018 03:14 PM, Bin Meng wrote:
Hi Marek,
On Wed, Aug 8, 2018 at 9:03 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
> Cc: Simon Glass
> ---
> drivers/pci/pci-uclass.c | 14 ++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/pci/pci-uclass.c
> b/drivers/pci/pci-uclass.c
> index 46e9c71bdf..306bea0dbf 100644
> --- a/drivers/pci/pci-uclass.c
> +++ b/drivers/pci/pci-uclass.c
> @@ -662,6 +662,8 @@ static int
> pci_find_and_bind_driver(struct udevice *parent,
> for (id = entry->match;
> id->vendor || id->subvendor ||
> id->class_mask;
> id++) {
> + ofnode node;
> +
> if (!pci_match_one_id(id,
> find_id))
> continue;
>
> @@ -691,6 +693,18 @@ static int
> pci_find_and_bind_driver(struct udevice *parent,
> goto error;
> debug("%s: Match found: %s\n",
> __func__, drv->name);
> dev->driver_data =
> find_id->driver_data;
> +
> + dev_for_each_subnode(node,
> parent) {
> + phys_addr_t df, size;
> + df =
> ofnode_get_addr_size(node, "reg", );
> +
> + if (PCI_FUNC(df) ==
> PCI_FUNC(bdf) &&
> + PCI_DEV(df) ==
> PCI_DEV(bdf)) {
> + dev->node = node;
> + break;
> + }
The function pci_find_and_bind_driver() is supposed to
bind devices
that are NOT in the device tree. Adding device tree access
in this
routine is quite odd. You can add the EHCI controller that
need such
PHY subnodes in the device tree and there is no
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Marek,
On Fri, Aug 17, 2018 at 6:27 PM, Marek Vasut wrote:
> On 08/17/2018 03:51 AM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Thu, Aug 16, 2018 at 7:47 PM, Marek Vasut wrote:
>>> On 08/15/2018 01:25 PM, Tom Rini wrote:
On Wed, Aug 15, 2018 at 06:19:25PM +0800, Bin Meng wrote:
> Hi Marek,
>
> On Wed, Aug 15, 2018 at 5:22 PM, Marek Vasut
> wrote:
>> On 08/14/2018 11:40 AM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Tue, Aug 14, 2018 at 4:55 PM, Marek Vasut
>>> wrote:
On 08/14/2018 03:46 AM, Bin Meng wrote:
> Hi Marek,
>
> On Mon, Aug 13, 2018 at 9:46 PM, Marek Vasut
> wrote:
>> On 08/13/2018 04:24 AM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Fri, Aug 10, 2018 at 8:38 PM, Marek Vasut
>>> wrote:
On 08/10/2018 02:01 PM, Tom Rini wrote:
> On Wed, Aug 08, 2018 at 09:37:25PM +0200, Marek Vasut wrote:
>> On 08/08/2018 05:32 PM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut
>>> wrote:
On 08/08/2018 03:39 PM, Bin Meng wrote:
> Hi Marek,
>
> On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut
> wrote:
>> On 08/08/2018 03:14 PM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Wed, Aug 8, 2018 at 9:03 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
Cc: Simon Glass
---
drivers/pci/pci-uclass.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/drivers/pci/pci-uclass.c
b/drivers/pci/pci-uclass.c
index 46e9c71bdf..306bea0dbf 100644
--- a/drivers/pci/pci-uclass.c
+++ b/drivers/pci/pci-uclass.c
@@ -662,6 +662,8 @@ static int
pci_find_and_bind_driver(struct udevice *parent,
for (id = entry->match;
id->vendor || id->subvendor ||
id->class_mask;
id++) {
+ ofnode node;
+
if (!pci_match_one_id(id, find_id))
continue;
@@ -691,6 +693,18 @@ static int
pci_find_and_bind_driver(struct udevice *parent,
goto error;
debug("%s: Match found: %s\n",
__func__, drv->name);
dev->driver_data =
find_id->driver_data;
+
+ dev_for_each_subnode(node, parent)
{
+ phys_addr_t df, size;
+ df =
ofnode_get_addr_size(node, "reg", );
+
+ if (PCI_FUNC(df) ==
PCI_FUNC(bdf) &&
+ PCI_DEV(df) ==
PCI_DEV(bdf)) {
+ dev->node = node;
+ break;
+ }
>>>
>>> The function pci_find_and_bind_driver() is supposed to bind
>>> devices
>>> that are NOT in the device tree. Adding device tree access
>>> in this
>>> routine is quite odd. You can add the EHCI controller that
>>> need such
>>> PHY subnodes in the device tree and there is no need to
>>> modify
>>> anything I believe. If you are looking for an example,
>>> please check
>>> pciuart0
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/17/2018 03:51 AM, Bin Meng wrote:
> Hi Marek,
>
> On Thu, Aug 16, 2018 at 7:47 PM, Marek Vasut wrote:
>> On 08/15/2018 01:25 PM, Tom Rini wrote:
>>> On Wed, Aug 15, 2018 at 06:19:25PM +0800, Bin Meng wrote:
Hi Marek,
On Wed, Aug 15, 2018 at 5:22 PM, Marek Vasut wrote:
> On 08/14/2018 11:40 AM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Tue, Aug 14, 2018 at 4:55 PM, Marek Vasut
>> wrote:
>>> On 08/14/2018 03:46 AM, Bin Meng wrote:
Hi Marek,
On Mon, Aug 13, 2018 at 9:46 PM, Marek Vasut
wrote:
> On 08/13/2018 04:24 AM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Fri, Aug 10, 2018 at 8:38 PM, Marek Vasut
>> wrote:
>>> On 08/10/2018 02:01 PM, Tom Rini wrote:
On Wed, Aug 08, 2018 at 09:37:25PM +0200, Marek Vasut wrote:
> On 08/08/2018 05:32 PM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut
>> wrote:
>>> On 08/08/2018 03:39 PM, Bin Meng wrote:
Hi Marek,
On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut
wrote:
> On 08/08/2018 03:14 PM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Wed, Aug 8, 2018 at 9:03 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
>>> Cc: Simon Glass
>>> ---
>>> drivers/pci/pci-uclass.c | 14 ++
>>> 1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/pci/pci-uclass.c
>>> b/drivers/pci/pci-uclass.c
>>> index 46e9c71bdf..306bea0dbf 100644
>>> --- a/drivers/pci/pci-uclass.c
>>> +++ b/drivers/pci/pci-uclass.c
>>> @@ -662,6 +662,8 @@ static int
>>> pci_find_and_bind_driver(struct udevice *parent,
>>> for (id = entry->match;
>>> id->vendor || id->subvendor ||
>>> id->class_mask;
>>> id++) {
>>> + ofnode node;
>>> +
>>> if (!pci_match_one_id(id, find_id))
>>> continue;
>>>
>>> @@ -691,6 +693,18 @@ static int
>>> pci_find_and_bind_driver(struct udevice *parent,
>>> goto error;
>>> debug("%s: Match found: %s\n",
>>> __func__, drv->name);
>>> dev->driver_data =
>>> find_id->driver_data;
>>> +
>>> + dev_for_each_subnode(node, parent) {
>>> + phys_addr_t df, size;
>>> + df =
>>> ofnode_get_addr_size(node, "reg", );
>>> +
>>> + if (PCI_FUNC(df) ==
>>> PCI_FUNC(bdf) &&
>>> + PCI_DEV(df) ==
>>> PCI_DEV(bdf)) {
>>> + dev->node = node;
>>> + break;
>>> + }
>>
>> The function pci_find_and_bind_driver() is supposed to bind
>> devices
>> that are NOT in the device tree. Adding device tree access
>> in this
>> routine is quite odd. You can add the EHCI controller that
>> need such
>> PHY subnodes in the device tree and there is no need to
>> modify
>> anything I believe. If you are looking for an example,
>> please check
>> pciuart0 in arch/x86/dts/crownbay.dts.
>
> Well this does not work for me, the EHCI PCI doesn't get a DT
> node
> assigned, check r8a7794.dtsi
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Marek,
On Thu, Aug 16, 2018 at 7:47 PM, Marek Vasut wrote:
> On 08/15/2018 01:25 PM, Tom Rini wrote:
>> On Wed, Aug 15, 2018 at 06:19:25PM +0800, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Wed, Aug 15, 2018 at 5:22 PM, Marek Vasut wrote:
On 08/14/2018 11:40 AM, Bin Meng wrote:
> Hi Marek,
>
> On Tue, Aug 14, 2018 at 4:55 PM, Marek Vasut
> wrote:
>> On 08/14/2018 03:46 AM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Mon, Aug 13, 2018 at 9:46 PM, Marek Vasut
>>> wrote:
On 08/13/2018 04:24 AM, Bin Meng wrote:
> Hi Marek,
>
> On Fri, Aug 10, 2018 at 8:38 PM, Marek Vasut
> wrote:
>> On 08/10/2018 02:01 PM, Tom Rini wrote:
>>> On Wed, Aug 08, 2018 at 09:37:25PM +0200, Marek Vasut wrote:
On 08/08/2018 05:32 PM, Bin Meng wrote:
> Hi Marek,
>
> On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut
> wrote:
>> On 08/08/2018 03:39 PM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut
>>> wrote:
On 08/08/2018 03:14 PM, Bin Meng wrote:
> Hi Marek,
>
> On Wed, Aug 8, 2018 at 9:03 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
>> Cc: Simon Glass
>> ---
>> drivers/pci/pci-uclass.c | 14 ++
>> 1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/pci/pci-uclass.c
>> b/drivers/pci/pci-uclass.c
>> index 46e9c71bdf..306bea0dbf 100644
>> --- a/drivers/pci/pci-uclass.c
>> +++ b/drivers/pci/pci-uclass.c
>> @@ -662,6 +662,8 @@ static int
>> pci_find_and_bind_driver(struct udevice *parent,
>> for (id = entry->match;
>> id->vendor || id->subvendor ||
>> id->class_mask;
>> id++) {
>> + ofnode node;
>> +
>> if (!pci_match_one_id(id, find_id))
>> continue;
>>
>> @@ -691,6 +693,18 @@ static int
>> pci_find_and_bind_driver(struct udevice *parent,
>> goto error;
>> debug("%s: Match found: %s\n",
>> __func__, drv->name);
>> dev->driver_data =
>> find_id->driver_data;
>> +
>> + dev_for_each_subnode(node, parent) {
>> + phys_addr_t df, size;
>> + df =
>> ofnode_get_addr_size(node, "reg", );
>> +
>> + if (PCI_FUNC(df) ==
>> PCI_FUNC(bdf) &&
>> + PCI_DEV(df) ==
>> PCI_DEV(bdf)) {
>> + dev->node = node;
>> + break;
>> + }
>
> The function pci_find_and_bind_driver() is supposed to bind
> devices
> that are NOT in the device tree. Adding device tree access in
> this
> routine is quite odd. You can add the EHCI controller that
> need such
> PHY subnodes in the device tree and there is no need to modify
> anything I believe. If you are looking for an example, please
> check
> pciuart0 in arch/x86/dts/crownbay.dts.
Well this does not work for me, the EHCI PCI doesn't get a DT
node
assigned, check r8a7794.dtsi for the PCI devices I use.
>>>
>>> I think that's because you don't specify a "compatible" string
>>> for
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/15/2018 01:25 PM, Tom Rini wrote:
> On Wed, Aug 15, 2018 at 06:19:25PM +0800, Bin Meng wrote:
>> Hi Marek,
>>
>> On Wed, Aug 15, 2018 at 5:22 PM, Marek Vasut wrote:
>>> On 08/14/2018 11:40 AM, Bin Meng wrote:
Hi Marek,
On Tue, Aug 14, 2018 at 4:55 PM, Marek Vasut wrote:
> On 08/14/2018 03:46 AM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Mon, Aug 13, 2018 at 9:46 PM, Marek Vasut
>> wrote:
>>> On 08/13/2018 04:24 AM, Bin Meng wrote:
Hi Marek,
On Fri, Aug 10, 2018 at 8:38 PM, Marek Vasut
wrote:
> On 08/10/2018 02:01 PM, Tom Rini wrote:
>> On Wed, Aug 08, 2018 at 09:37:25PM +0200, Marek Vasut wrote:
>>> On 08/08/2018 05:32 PM, Bin Meng wrote:
Hi Marek,
On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut
wrote:
> On 08/08/2018 03:39 PM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut
>> wrote:
>>> On 08/08/2018 03:14 PM, Bin Meng wrote:
Hi Marek,
On Wed, Aug 8, 2018 at 9:03 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
> Cc: Simon Glass
> ---
> drivers/pci/pci-uclass.c | 14 ++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/pci/pci-uclass.c
> b/drivers/pci/pci-uclass.c
> index 46e9c71bdf..306bea0dbf 100644
> --- a/drivers/pci/pci-uclass.c
> +++ b/drivers/pci/pci-uclass.c
> @@ -662,6 +662,8 @@ static int
> pci_find_and_bind_driver(struct udevice *parent,
> for (id = entry->match;
> id->vendor || id->subvendor ||
> id->class_mask;
> id++) {
> + ofnode node;
> +
> if (!pci_match_one_id(id, find_id))
> continue;
>
> @@ -691,6 +693,18 @@ static int
> pci_find_and_bind_driver(struct udevice *parent,
> goto error;
> debug("%s: Match found: %s\n",
> __func__, drv->name);
> dev->driver_data =
> find_id->driver_data;
> +
> + dev_for_each_subnode(node, parent) {
> + phys_addr_t df, size;
> + df =
> ofnode_get_addr_size(node, "reg", );
> +
> + if (PCI_FUNC(df) ==
> PCI_FUNC(bdf) &&
> + PCI_DEV(df) ==
> PCI_DEV(bdf)) {
> + dev->node = node;
> + break;
> + }
The function pci_find_and_bind_driver() is supposed to bind
devices
that are NOT in the device tree. Adding device tree access in
this
routine is quite odd. You can add the EHCI controller that
need such
PHY subnodes in the device tree and there is no need to modify
anything I believe. If you are looking for an example, please
check
pciuart0 in arch/x86/dts/crownbay.dts.
>>>
>>> Well this does not work for me, the EHCI PCI doesn't get a DT
>>> node
>>> assigned, check r8a7794.dtsi for the PCI devices I use.
>>>
>>
>> I think that's because you don't specify a "compatible" string
>> for
>> these two EHCI PCI nodes.
>
> That's perfectly fine, why should I specify it ? Linux has no
> problem
> with it either.
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/15/2018 12:19 PM, Bin Meng wrote:
> Hi Marek,
>
> On Wed, Aug 15, 2018 at 5:22 PM, Marek Vasut wrote:
>> On 08/14/2018 11:40 AM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Tue, Aug 14, 2018 at 4:55 PM, Marek Vasut wrote:
On 08/14/2018 03:46 AM, Bin Meng wrote:
> Hi Marek,
>
> On Mon, Aug 13, 2018 at 9:46 PM, Marek Vasut
> wrote:
>> On 08/13/2018 04:24 AM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Fri, Aug 10, 2018 at 8:38 PM, Marek Vasut
>>> wrote:
On 08/10/2018 02:01 PM, Tom Rini wrote:
> On Wed, Aug 08, 2018 at 09:37:25PM +0200, Marek Vasut wrote:
>> On 08/08/2018 05:32 PM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut
>>> wrote:
On 08/08/2018 03:39 PM, Bin Meng wrote:
> Hi Marek,
>
> On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut
> wrote:
>> On 08/08/2018 03:14 PM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Wed, Aug 8, 2018 at 9:03 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
Cc: Simon Glass
---
drivers/pci/pci-uclass.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/drivers/pci/pci-uclass.c
b/drivers/pci/pci-uclass.c
index 46e9c71bdf..306bea0dbf 100644
--- a/drivers/pci/pci-uclass.c
+++ b/drivers/pci/pci-uclass.c
@@ -662,6 +662,8 @@ static int pci_find_and_bind_driver(struct
udevice *parent,
for (id = entry->match;
id->vendor || id->subvendor ||
id->class_mask;
id++) {
+ ofnode node;
+
if (!pci_match_one_id(id, find_id))
continue;
@@ -691,6 +693,18 @@ static int
pci_find_and_bind_driver(struct udevice *parent,
goto error;
debug("%s: Match found: %s\n",
__func__, drv->name);
dev->driver_data =
find_id->driver_data;
+
+ dev_for_each_subnode(node, parent) {
+ phys_addr_t df, size;
+ df =
ofnode_get_addr_size(node, "reg", );
+
+ if (PCI_FUNC(df) ==
PCI_FUNC(bdf) &&
+ PCI_DEV(df) ==
PCI_DEV(bdf)) {
+ dev->node = node;
+ break;
+ }
>>>
>>> The function pci_find_and_bind_driver() is supposed to bind
>>> devices
>>> that are NOT in the device tree. Adding device tree access in
>>> this
>>> routine is quite odd. You can add the EHCI controller that need
>>> such
>>> PHY subnodes in the device tree and there is no need to modify
>>> anything I believe. If you are looking for an example, please
>>> check
>>> pciuart0 in arch/x86/dts/crownbay.dts.
>>
>> Well this does not work for me, the EHCI PCI doesn't get a DT
>> node
>> assigned, check r8a7794.dtsi for the PCI devices I use.
>>
>
> I think that's because you don't specify a "compatible" string for
> these two EHCI PCI nodes.
That's perfectly fine, why should I specify it ? Linux has no
problem
with it either.
>>>
>>> Without a "compatible" string, DM does not bind any device in the
>>> device tree to a driver, hence no device node created. This is not
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On Wed, Aug 15, 2018 at 06:19:25PM +0800, Bin Meng wrote:
> Hi Marek,
>
> On Wed, Aug 15, 2018 at 5:22 PM, Marek Vasut wrote:
> > On 08/14/2018 11:40 AM, Bin Meng wrote:
> >> Hi Marek,
> >>
> >> On Tue, Aug 14, 2018 at 4:55 PM, Marek Vasut wrote:
> >>> On 08/14/2018 03:46 AM, Bin Meng wrote:
> Hi Marek,
>
> On Mon, Aug 13, 2018 at 9:46 PM, Marek Vasut
> wrote:
> > On 08/13/2018 04:24 AM, Bin Meng wrote:
> >> Hi Marek,
> >>
> >> On Fri, Aug 10, 2018 at 8:38 PM, Marek Vasut
> >> wrote:
> >>> On 08/10/2018 02:01 PM, Tom Rini wrote:
> On Wed, Aug 08, 2018 at 09:37:25PM +0200, Marek Vasut wrote:
> > On 08/08/2018 05:32 PM, Bin Meng wrote:
> >> Hi Marek,
> >>
> >> On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut
> >> wrote:
> >>> On 08/08/2018 03:39 PM, Bin Meng wrote:
> Hi Marek,
>
> On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut
> wrote:
> > On 08/08/2018 03:14 PM, Bin Meng wrote:
> >> Hi Marek,
> >>
> >> On Wed, Aug 8, 2018 at 9:03 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
> >>> Cc: Simon Glass
> >>> ---
> >>> drivers/pci/pci-uclass.c | 14 ++
> >>> 1 file changed, 14 insertions(+)
> >>>
> >>> diff --git a/drivers/pci/pci-uclass.c
> >>> b/drivers/pci/pci-uclass.c
> >>> index 46e9c71bdf..306bea0dbf 100644
> >>> --- a/drivers/pci/pci-uclass.c
> >>> +++ b/drivers/pci/pci-uclass.c
> >>> @@ -662,6 +662,8 @@ static int
> >>> pci_find_and_bind_driver(struct udevice *parent,
> >>> for (id = entry->match;
> >>> id->vendor || id->subvendor ||
> >>> id->class_mask;
> >>> id++) {
> >>> + ofnode node;
> >>> +
> >>> if (!pci_match_one_id(id, find_id))
> >>> continue;
> >>>
> >>> @@ -691,6 +693,18 @@ static int
> >>> pci_find_and_bind_driver(struct udevice *parent,
> >>> goto error;
> >>> debug("%s: Match found: %s\n",
> >>> __func__, drv->name);
> >>> dev->driver_data =
> >>> find_id->driver_data;
> >>> +
> >>> + dev_for_each_subnode(node, parent) {
> >>> + phys_addr_t df, size;
> >>> + df =
> >>> ofnode_get_addr_size(node, "reg", );
> >>> +
> >>> + if (PCI_FUNC(df) ==
> >>> PCI_FUNC(bdf) &&
> >>> + PCI_DEV(df) ==
> >>> PCI_DEV(bdf)) {
> >>> + dev->node = node;
> >>> + break;
> >>> + }
> >>
> >> The function pci_find_and_bind_driver() is supposed to bind
> >> devices
> >> that are NOT in the device tree. Adding device tree access in
> >> this
> >> routine is quite odd. You can add the EHCI controller that
> >> need such
> >> PHY subnodes in the device tree and there is no need to modify
> >> anything I believe. If you are looking for an example, please
> >> check
> >> pciuart0 in arch/x86/dts/crownbay.dts.
> >
> > Well this does not work for me, the EHCI PCI doesn't get a DT
> > node
> > assigned, check r8a7794.dtsi for the PCI devices I use.
> >
>
> I think that's because you don't specify a "compatible" string
> for
> these two EHCI PCI nodes.
> >>>
> >>> That's perfectly fine, why should I specify it ? Linux has no
> >>> problem
> >>> with it either.
> >>>
> >>
> >> Without a
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Marek,
On Wed, Aug 15, 2018 at 5:22 PM, Marek Vasut wrote:
> On 08/14/2018 11:40 AM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Tue, Aug 14, 2018 at 4:55 PM, Marek Vasut wrote:
>>> On 08/14/2018 03:46 AM, Bin Meng wrote:
Hi Marek,
On Mon, Aug 13, 2018 at 9:46 PM, Marek Vasut wrote:
> On 08/13/2018 04:24 AM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Fri, Aug 10, 2018 at 8:38 PM, Marek Vasut
>> wrote:
>>> On 08/10/2018 02:01 PM, Tom Rini wrote:
On Wed, Aug 08, 2018 at 09:37:25PM +0200, Marek Vasut wrote:
> On 08/08/2018 05:32 PM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut
>> wrote:
>>> On 08/08/2018 03:39 PM, Bin Meng wrote:
Hi Marek,
On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut
wrote:
> On 08/08/2018 03:14 PM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Wed, Aug 8, 2018 at 9:03 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
>>> Cc: Simon Glass
>>> ---
>>> drivers/pci/pci-uclass.c | 14 ++
>>> 1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
>>> index 46e9c71bdf..306bea0dbf 100644
>>> --- a/drivers/pci/pci-uclass.c
>>> +++ b/drivers/pci/pci-uclass.c
>>> @@ -662,6 +662,8 @@ static int pci_find_and_bind_driver(struct
>>> udevice *parent,
>>> for (id = entry->match;
>>> id->vendor || id->subvendor ||
>>> id->class_mask;
>>> id++) {
>>> + ofnode node;
>>> +
>>> if (!pci_match_one_id(id, find_id))
>>> continue;
>>>
>>> @@ -691,6 +693,18 @@ static int pci_find_and_bind_driver(struct
>>> udevice *parent,
>>> goto error;
>>> debug("%s: Match found: %s\n",
>>> __func__, drv->name);
>>> dev->driver_data = find_id->driver_data;
>>> +
>>> + dev_for_each_subnode(node, parent) {
>>> + phys_addr_t df, size;
>>> + df = ofnode_get_addr_size(node,
>>> "reg", );
>>> +
>>> + if (PCI_FUNC(df) ==
>>> PCI_FUNC(bdf) &&
>>> + PCI_DEV(df) ==
>>> PCI_DEV(bdf)) {
>>> + dev->node = node;
>>> + break;
>>> + }
>>
>> The function pci_find_and_bind_driver() is supposed to bind
>> devices
>> that are NOT in the device tree. Adding device tree access in
>> this
>> routine is quite odd. You can add the EHCI controller that need
>> such
>> PHY subnodes in the device tree and there is no need to modify
>> anything I believe. If you are looking for an example, please
>> check
>> pciuart0 in arch/x86/dts/crownbay.dts.
>
> Well this does not work for me, the EHCI PCI doesn't get a DT node
> assigned, check r8a7794.dtsi for the PCI devices I use.
>
I think that's because you don't specify a "compatible" string for
these two EHCI PCI nodes.
>>>
>>> That's perfectly fine, why should I specify it ? Linux has no
>>> problem
>>> with it either.
>>>
>>
>> Without a "compatible" string, DM does not bind any device in the
>> device tree to a driver, hence no device node created. This is not
>> Linux.
>
> DT is NOT Linux specific, it is OS-agnostic, DT describes hardware and
> hardware only. If U-Boot cannot parse DT correctly, U-Boot is broken
> and
> must be
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/14/2018 11:40 AM, Bin Meng wrote:
> Hi Marek,
>
> On Tue, Aug 14, 2018 at 4:55 PM, Marek Vasut wrote:
>> On 08/14/2018 03:46 AM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Mon, Aug 13, 2018 at 9:46 PM, Marek Vasut wrote:
On 08/13/2018 04:24 AM, Bin Meng wrote:
> Hi Marek,
>
> On Fri, Aug 10, 2018 at 8:38 PM, Marek Vasut
> wrote:
>> On 08/10/2018 02:01 PM, Tom Rini wrote:
>>> On Wed, Aug 08, 2018 at 09:37:25PM +0200, Marek Vasut wrote:
On 08/08/2018 05:32 PM, Bin Meng wrote:
> Hi Marek,
>
> On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut
> wrote:
>> On 08/08/2018 03:39 PM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut
>>> wrote:
On 08/08/2018 03:14 PM, Bin Meng wrote:
> Hi Marek,
>
> On Wed, Aug 8, 2018 at 9:03 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
>> Cc: Simon Glass
>> ---
>> drivers/pci/pci-uclass.c | 14 ++
>> 1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
>> index 46e9c71bdf..306bea0dbf 100644
>> --- a/drivers/pci/pci-uclass.c
>> +++ b/drivers/pci/pci-uclass.c
>> @@ -662,6 +662,8 @@ static int pci_find_and_bind_driver(struct
>> udevice *parent,
>> for (id = entry->match;
>> id->vendor || id->subvendor ||
>> id->class_mask;
>> id++) {
>> + ofnode node;
>> +
>> if (!pci_match_one_id(id, find_id))
>> continue;
>>
>> @@ -691,6 +693,18 @@ static int pci_find_and_bind_driver(struct
>> udevice *parent,
>> goto error;
>> debug("%s: Match found: %s\n", __func__,
>> drv->name);
>> dev->driver_data = find_id->driver_data;
>> +
>> + dev_for_each_subnode(node, parent) {
>> + phys_addr_t df, size;
>> + df = ofnode_get_addr_size(node,
>> "reg", );
>> +
>> + if (PCI_FUNC(df) ==
>> PCI_FUNC(bdf) &&
>> + PCI_DEV(df) == PCI_DEV(bdf))
>> {
>> + dev->node = node;
>> + break;
>> + }
>
> The function pci_find_and_bind_driver() is supposed to bind
> devices
> that are NOT in the device tree. Adding device tree access in this
> routine is quite odd. You can add the EHCI controller that need
> such
> PHY subnodes in the device tree and there is no need to modify
> anything I believe. If you are looking for an example, please
> check
> pciuart0 in arch/x86/dts/crownbay.dts.
Well this does not work for me, the EHCI PCI doesn't get a DT node
assigned, check r8a7794.dtsi for the PCI devices I use.
>>>
>>> I think that's because you don't specify a "compatible" string for
>>> these two EHCI PCI nodes.
>>
>> That's perfectly fine, why should I specify it ? Linux has no problem
>> with it either.
>>
>
> Without a "compatible" string, DM does not bind any device in the
> device tree to a driver, hence no device node created. This is not
> Linux.
DT is NOT Linux specific, it is OS-agnostic, DT describes hardware and
hardware only. If U-Boot cannot parse DT correctly, U-Boot is broken
and
must be fixed.
This is a fix. If there is a better fix, I am open to it.
>>>
>>> DT should but isn't always OS agnostic. DTS files that reside in the
>>> Linux Kernel are in practice is
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/14/2018 11:35 AM, Bin Meng wrote: [...] >> I think knowing where Linux does this >> would be instructive to figure out where we need to have some additional >> logic added OR we can make some cost/benefit analysis to see if it makes >> more sense overall to add compatibles to some nodes rather than add to >> the binary size. > > Adding compatible does not make any sense, the PCI ID provides that > information. Adding compatible would only add redundancy which could > possibly be even harmful (ie. if the controller got replaced with > another one). To try and move things along rather than re-argue the same point, you're saying that our pci_find_and_bind_driver() is the rough equivalent of of_pci_find_child_device() or at least pci_set_of_node() (which calls of_pci_find_child_device()). So, Bin, if this isn't the right place to start down this path, where would be? Given that Linux can take a DTB and PCI bus with devices and get things right, what would it look like for U-Boot to replicate the same behavior? Instead of having to add explicit compatible nodes for each PCI device, as I understand (but correct me if I'm wrong) we're doing today. Thanks! >>> >>> So is this a requirement for all U-Boot driver subsystems to replicate >>> the same Linux behavior? If yes, can we have it officially documented >>> somewhere? >> >> No, because we are not replicating Linux behavior. >> > > But you kept mentioning you wanted U-Boot to use exactly the same DT > from Linux. And I pointed out that FreeBSD's DT files are not the same > as Linux. Here you are saying you don't want U-Boot to replicate Linux > behavior, if not the Linux behavior, what do you want to suggest then? Just parse the information out of the DT fully and be done with it. That is what I suggest. >>> Since Marek refused to take the original U-Boot option 1 to support >>> his case, and asked U-Boot to follow Linux's practice on PCI device >>> binding, if we go that way, here is what we can do: >> >> You are inserting words into my mouth and I dislike that. I never said >> anything about Linux. I said DT is OS agnostic and U-Boot should be able >> to parse DT as fully as possible. > > See above comment. I might have used some words that made you feel > uncomfortable, and for me I felt the same way. Part of the reason is > that I am not a native English speaker and I may mis-use/interpret > words. If that's the case I apologize. Anyway I won't quarrel against > this over and over again. This does not help to move things forward. This is a technical discussion, we should use arguments, not feelings. I am fine, but thanks for your concern. >>> * Keep pci-uclass driver's post_bind() and child_post_bind() only for >>> Sandbox configuration >>> * Keep the call to pci_bus_find_devfn() in pci_bind_bus_devices() only >>> for Sandbox configuration >>> * Sandbox is special. We should limit the mechanism of matching PCI >>> emulation device via "compatible" to sandbox only >> >> I don't think this is limited to sandbox, although I don't see a >> real-world usecase right now. >> > > You are welcome to send patches against sandbox PCI codes to remove > such limitation, to make it behave like a real-world bus device. > >>> * Assign the DT node to the bound device in pci_find_and_bind_driver() >>> if there is a valid PCI "reg" encoding for a specific PCI device in >>> the device tree >>> * Create DM PCI test case against the DT node assignment >>> * Remove all compatible string in U-Boot's PCI device drivers: eg: >>> ehci_pci_ids[], xhci_pci_ids[], etc. IOW, all PCI device drivers >>> should only use U_BOOT_PCI_DEVICE(), aka the original U-Boot option 2 >>> * Fork a "pci-ns16550" driver to support U_BOOT_PCI_DEVICE(), as >>> currently PCI ns16550 device driver uses "compatible" string to do the >>> matching, and update crownbay.dts and galileo.dts (so far I only know >>> two boards are using PCI ns16550 serial port) >> >> I cannot test such changes, but I believe there is >> PCI_CLASS_COMMUNICATION_SERIAL and matching on that would suffice ? >> > > Maybe, but I need check the datasheet to confirm. This can be a good > start though. Note I can test the changes, and I can do the whole > bunch of such proposed design changes if you don't mind, but let's > wait for Simon's comments. All those sandbox changes can be done in parallel to this change though, right ? >>> * Make sure all DM PCI test cases are not broken >>> * Document all of the above changes in doc/driver-model/pci-info.txt >>> >>> I am not sure if I missed anything. Simon, could you also comment on it? > > Regards, > Bin > -- Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On Tue, Aug 14, 2018 at 10:34:23AM +0800, Bin Meng wrote: > Hi Tom, > > On Tue, Aug 14, 2018 at 1:16 AM, Tom Rini wrote: > > On Mon, Aug 13, 2018 at 06:07:14PM +0200, Marek Vasut wrote: > >> On 08/13/2018 03:39 PM, Tom Rini wrote: > >> [...] > >> > >> Next step is to upstream the DT > >> changes to Linux kernel, then sync the changes to U-Boot to satisfy > >> this obsession - using exactly the same DT as Linux. > >> >>> > >> >>> This is not gonna happen. > >> >>> > >> >>> Sorry, you're really just wasting my time with this foolishness. If > >> >>> U-Boot cannot parse valid DT bindings while other OSes can, U-Boot is > >> >>> broken and must be fixed. So far I only see you attacking this patch > >> >>> and > >> >>> trying to pull in everything you can do avoid accepting this patch or > >> >>> providing a better alternative. This is not a constructive discussion, > >> >>> so I stop here. > >> >> > >> >> The fix in this patch is purely hack, period. > >> > > >> > Lets step back for a moment. > >> > > >> > First, U-Boot intends to be, in the case where a relevant DTS file > >> > exists, the Linux kernel one PLUS additions we require (u-boot,dm-spl, > >> > u-boot,dm-pre-reloc, but also sometimes stdout-path or properties that > >> > are omitted for various reasons). > >> > >> Right, which doesn't apply here. None of those u-boot,... props are > >> needed in this case. > > > > Which is why I also mentioned the non-u-boot specific props we also need > > sometimes. My point is two-fold: > > 1: We can and will _add_ information to the dts files that come from > > Linux. > > 2: Not all information that we add is U-Boot prefixed. > > It would be better if we document such DT expectation somewhere. Yes, it should be written down somewhere. But it's been a general expectation for a long time. It's why we even have the -u-boot.dtsi auto-include logic, so that the main dts/dtsi files can be re-synced without losing changes U-Boot needs. > >> > Second, I've asked before (both in this thread and on IRC), and not > >> > gotten an answer yet, as to how Linux goes "Oh, _this_ PCI device and > >> > _this_ DT node need to be matched and populate some data > >> > structures". > >> > >> You did get an answer to that on irc from George. Looks like > >> of_pci_find_child_device() in drivers/pci/of.c > > > > Yeah, George said he thought that might be it but didn't have time to > > confirm. > > > >> > Marek's patch seems to be, in short "here's where U-Boot > >> > needs to wire things up". Bin has said that no, the function in > >> > question is for other things. > >> > >> I disagree with this. It's a bind function and assigns other parameters > >> of the driver instance too. > >> > >> > I think knowing where Linux does this > >> > would be instructive to figure out where we need to have some additional > >> > logic added OR we can make some cost/benefit analysis to see if it makes > >> > more sense overall to add compatibles to some nodes rather than add to > >> > the binary size. > >> > >> Adding compatible does not make any sense, the PCI ID provides that > >> information. Adding compatible would only add redundancy which could > >> possibly be even harmful (ie. if the controller got replaced with > >> another one). > > > > To try and move things along rather than re-argue the same point, you're > > saying that our pci_find_and_bind_driver() is the rough equivalent of > > of_pci_find_child_device() or at least pci_set_of_node() (which calls > > of_pci_find_child_device()). > > > > So, Bin, if this isn't the right place to start down this path, where > > would be? Given that Linux can take a DTB and PCI bus with devices and > > get things right, what would it look like for U-Boot to replicate the > > same behavior? Instead of having to add explicit compatible nodes for > > each PCI device, as I understand (but correct me if I'm wrong) we're > > doing today. Thanks! > > So is this a requirement for all U-Boot driver subsystems to replicate > the same Linux behavior? If yes, can we have it officially documented > somewhere? Without very good and documented reasons, yes, it's an expectation that concepts / details we borrow from Linux should work like or close to Linux. Large parts of the MTD subsystem for example are re-synced. > Since Marek refused to take the original U-Boot option 1 to support > his case, and asked U-Boot to follow Linux's practice on PCI device > binding, if we go that way, here is what we can do: > > * Keep pci-uclass driver's post_bind() and child_post_bind() only for > Sandbox configuration > * Keep the call to pci_bus_find_devfn() in pci_bind_bus_devices() only > for Sandbox configuration > * Sandbox is special. We should limit the mechanism of matching PCI > emulation device via "compatible" to sandbox only Yes, Sandbox is special and if we #if that part as needed, that might be good too. > * Assign the DT node to the bound device in pci_find_and_bind_driver() > if there
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Marek,
On Tue, Aug 14, 2018 at 4:55 PM, Marek Vasut wrote:
> On 08/14/2018 03:46 AM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Mon, Aug 13, 2018 at 9:46 PM, Marek Vasut wrote:
>>> On 08/13/2018 04:24 AM, Bin Meng wrote:
Hi Marek,
On Fri, Aug 10, 2018 at 8:38 PM, Marek Vasut wrote:
> On 08/10/2018 02:01 PM, Tom Rini wrote:
>> On Wed, Aug 08, 2018 at 09:37:25PM +0200, Marek Vasut wrote:
>>> On 08/08/2018 05:32 PM, Bin Meng wrote:
Hi Marek,
On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut
wrote:
> On 08/08/2018 03:39 PM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut
>> wrote:
>>> On 08/08/2018 03:14 PM, Bin Meng wrote:
Hi Marek,
On Wed, Aug 8, 2018 at 9:03 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
> Cc: Simon Glass
> ---
> drivers/pci/pci-uclass.c | 14 ++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
> index 46e9c71bdf..306bea0dbf 100644
> --- a/drivers/pci/pci-uclass.c
> +++ b/drivers/pci/pci-uclass.c
> @@ -662,6 +662,8 @@ static int pci_find_and_bind_driver(struct
> udevice *parent,
> for (id = entry->match;
> id->vendor || id->subvendor ||
> id->class_mask;
> id++) {
> + ofnode node;
> +
> if (!pci_match_one_id(id, find_id))
> continue;
>
> @@ -691,6 +693,18 @@ static int pci_find_and_bind_driver(struct
> udevice *parent,
> goto error;
> debug("%s: Match found: %s\n", __func__,
> drv->name);
> dev->driver_data = find_id->driver_data;
> +
> + dev_for_each_subnode(node, parent) {
> + phys_addr_t df, size;
> + df = ofnode_get_addr_size(node,
> "reg", );
> +
> + if (PCI_FUNC(df) == PCI_FUNC(bdf)
> &&
> + PCI_DEV(df) == PCI_DEV(bdf)) {
> + dev->node = node;
> + break;
> + }
The function pci_find_and_bind_driver() is supposed to bind devices
that are NOT in the device tree. Adding device tree access in this
routine is quite odd. You can add the EHCI controller that need
such
PHY subnodes in the device tree and there is no need to modify
anything I believe. If you are looking for an example, please check
pciuart0 in arch/x86/dts/crownbay.dts.
>>>
>>> Well this does not work for me, the EHCI PCI doesn't get a DT node
>>> assigned, check r8a7794.dtsi for the PCI devices I use.
>>>
>>
>> I think that's because you don't specify a "compatible" string for
>> these two EHCI PCI nodes.
>
> That's perfectly fine, why should I specify it ? Linux has no problem
> with it either.
>
Without a "compatible" string, DM does not bind any device in the
device tree to a driver, hence no device node created. This is not
Linux.
>>>
>>> DT is NOT Linux specific, it is OS-agnostic, DT describes hardware and
>>> hardware only. If U-Boot cannot parse DT correctly, U-Boot is broken and
>>> must be fixed.
>>>
>>> This is a fix. If there is a better fix, I am open to it.
>>
>> DT should but isn't always OS agnostic. DTS files that reside in the
>> Linux Kernel are in practice is Linux-centric with the expectation that
>> even if you could solve a given problem with valid DTS changes you make
>> whatever is parsing it do additional logic instead. That,
>> approximately, is what your patch is doing. If
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Marek, On Tue, Aug 14, 2018 at 4:54 PM, Marek Vasut wrote: > On 08/14/2018 04:34 AM, Bin Meng wrote: >> Hi Tom, >> >> On Tue, Aug 14, 2018 at 1:16 AM, Tom Rini wrote: >>> On Mon, Aug 13, 2018 at 06:07:14PM +0200, Marek Vasut wrote: On 08/13/2018 03:39 PM, Tom Rini wrote: [...] Next step is to upstream the DT changes to Linux kernel, then sync the changes to U-Boot to satisfy this obsession - using exactly the same DT as Linux. >>> >>> This is not gonna happen. >>> >>> Sorry, you're really just wasting my time with this foolishness. If >>> U-Boot cannot parse valid DT bindings while other OSes can, U-Boot is >>> broken and must be fixed. So far I only see you attacking this patch and >>> trying to pull in everything you can do avoid accepting this patch or >>> providing a better alternative. This is not a constructive discussion, >>> so I stop here. >> >> The fix in this patch is purely hack, period. > > Lets step back for a moment. > > First, U-Boot intends to be, in the case where a relevant DTS file > exists, the Linux kernel one PLUS additions we require (u-boot,dm-spl, > u-boot,dm-pre-reloc, but also sometimes stdout-path or properties that > are omitted for various reasons). Right, which doesn't apply here. None of those u-boot,... props are needed in this case. >>> >>> Which is why I also mentioned the non-u-boot specific props we also need >>> sometimes. My point is two-fold: >>> 1: We can and will _add_ information to the dts files that come from >>> Linux. >>> 2: Not all information that we add is U-Boot prefixed. >>> >> >> It would be better if we document such DT expectation somewhere. >> > Second, I've asked before (both in this thread and on IRC), and not > gotten an answer yet, as to how Linux goes "Oh, _this_ PCI device and > _this_ DT node need to be matched and populate some data > structures". You did get an answer to that on irc from George. Looks like of_pci_find_child_device() in drivers/pci/of.c >>> >>> Yeah, George said he thought that might be it but didn't have time to >>> confirm. >>> > Marek's patch seems to be, in short "here's where U-Boot > needs to wire things up". Bin has said that no, the function in > question is for other things. I disagree with this. It's a bind function and assigns other parameters of the driver instance too. > I think knowing where Linux does this > would be instructive to figure out where we need to have some additional > logic added OR we can make some cost/benefit analysis to see if it makes > more sense overall to add compatibles to some nodes rather than add to > the binary size. Adding compatible does not make any sense, the PCI ID provides that information. Adding compatible would only add redundancy which could possibly be even harmful (ie. if the controller got replaced with another one). >>> >>> To try and move things along rather than re-argue the same point, you're >>> saying that our pci_find_and_bind_driver() is the rough equivalent of >>> of_pci_find_child_device() or at least pci_set_of_node() (which calls >>> of_pci_find_child_device()). >>> >>> So, Bin, if this isn't the right place to start down this path, where >>> would be? Given that Linux can take a DTB and PCI bus with devices and >>> get things right, what would it look like for U-Boot to replicate the >>> same behavior? Instead of having to add explicit compatible nodes for >>> each PCI device, as I understand (but correct me if I'm wrong) we're >>> doing today. Thanks! >> >> So is this a requirement for all U-Boot driver subsystems to replicate >> the same Linux behavior? If yes, can we have it officially documented >> somewhere? > > No, because we are not replicating Linux behavior. > But you kept mentioning you wanted U-Boot to use exactly the same DT from Linux. And I pointed out that FreeBSD's DT files are not the same as Linux. Here you are saying you don't want U-Boot to replicate Linux behavior, if not the Linux behavior, what do you want to suggest then? >> Since Marek refused to take the original U-Boot option 1 to support >> his case, and asked U-Boot to follow Linux's practice on PCI device >> binding, if we go that way, here is what we can do: > > You are inserting words into my mouth and I dislike that. I never said > anything about Linux. I said DT is OS agnostic and U-Boot should be able > to parse DT as fully as possible. See above comment. I might have used some words that made you feel uncomfortable, and for me I felt the same way. Part of the reason is that I am not a native English speaker and I may mis-use/interpret words. If that's the case I apologize. Anyway I won't quarrel against this over and over again. This does not help to move things forward. > >> * Keep pci-uclass driver's post_bind() and
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/14/2018 04:34 AM, Bin Meng wrote: > Hi Tom, > > On Tue, Aug 14, 2018 at 1:16 AM, Tom Rini wrote: >> On Mon, Aug 13, 2018 at 06:07:14PM +0200, Marek Vasut wrote: >>> On 08/13/2018 03:39 PM, Tom Rini wrote: >>> [...] >>> >>> Next step is to upstream the DT >>> changes to Linux kernel, then sync the changes to U-Boot to satisfy >>> this obsession - using exactly the same DT as Linux. >> >> This is not gonna happen. >> >> Sorry, you're really just wasting my time with this foolishness. If >> U-Boot cannot parse valid DT bindings while other OSes can, U-Boot is >> broken and must be fixed. So far I only see you attacking this patch and >> trying to pull in everything you can do avoid accepting this patch or >> providing a better alternative. This is not a constructive discussion, >> so I stop here. > > The fix in this patch is purely hack, period. Lets step back for a moment. First, U-Boot intends to be, in the case where a relevant DTS file exists, the Linux kernel one PLUS additions we require (u-boot,dm-spl, u-boot,dm-pre-reloc, but also sometimes stdout-path or properties that are omitted for various reasons). >>> >>> Right, which doesn't apply here. None of those u-boot,... props are >>> needed in this case. >> >> Which is why I also mentioned the non-u-boot specific props we also need >> sometimes. My point is two-fold: >> 1: We can and will _add_ information to the dts files that come from >> Linux. >> 2: Not all information that we add is U-Boot prefixed. >> > > It would be better if we document such DT expectation somewhere. > Second, I've asked before (both in this thread and on IRC), and not gotten an answer yet, as to how Linux goes "Oh, _this_ PCI device and _this_ DT node need to be matched and populate some data structures". >>> >>> You did get an answer to that on irc from George. Looks like >>> of_pci_find_child_device() in drivers/pci/of.c >> >> Yeah, George said he thought that might be it but didn't have time to >> confirm. >> Marek's patch seems to be, in short "here's where U-Boot needs to wire things up". Bin has said that no, the function in question is for other things. >>> >>> I disagree with this. It's a bind function and assigns other parameters >>> of the driver instance too. >>> I think knowing where Linux does this would be instructive to figure out where we need to have some additional logic added OR we can make some cost/benefit analysis to see if it makes more sense overall to add compatibles to some nodes rather than add to the binary size. >>> >>> Adding compatible does not make any sense, the PCI ID provides that >>> information. Adding compatible would only add redundancy which could >>> possibly be even harmful (ie. if the controller got replaced with >>> another one). >> >> To try and move things along rather than re-argue the same point, you're >> saying that our pci_find_and_bind_driver() is the rough equivalent of >> of_pci_find_child_device() or at least pci_set_of_node() (which calls >> of_pci_find_child_device()). >> >> So, Bin, if this isn't the right place to start down this path, where >> would be? Given that Linux can take a DTB and PCI bus with devices and >> get things right, what would it look like for U-Boot to replicate the >> same behavior? Instead of having to add explicit compatible nodes for >> each PCI device, as I understand (but correct me if I'm wrong) we're >> doing today. Thanks! > > So is this a requirement for all U-Boot driver subsystems to replicate > the same Linux behavior? If yes, can we have it officially documented > somewhere? No, because we are not replicating Linux behavior. > Since Marek refused to take the original U-Boot option 1 to support > his case, and asked U-Boot to follow Linux's practice on PCI device > binding, if we go that way, here is what we can do: You are inserting words into my mouth and I dislike that. I never said anything about Linux. I said DT is OS agnostic and U-Boot should be able to parse DT as fully as possible. > * Keep pci-uclass driver's post_bind() and child_post_bind() only for > Sandbox configuration > * Keep the call to pci_bus_find_devfn() in pci_bind_bus_devices() only > for Sandbox configuration > * Sandbox is special. We should limit the mechanism of matching PCI > emulation device via "compatible" to sandbox only I don't think this is limited to sandbox, although I don't see a real-world usecase right now. > * Assign the DT node to the bound device in pci_find_and_bind_driver() > if there is a valid PCI "reg" encoding for a specific PCI device in > the device tree > * Create DM PCI test case against the DT node assignment > * Remove all compatible string in U-Boot's PCI device drivers: eg: > ehci_pci_ids[], xhci_pci_ids[], etc. IOW, all PCI device drivers > should only use U_BOOT_PCI_DEVICE(), aka the original U-Boot option 2
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/14/2018 03:46 AM, Bin Meng wrote:
> Hi Marek,
>
> On Mon, Aug 13, 2018 at 9:46 PM, Marek Vasut wrote:
>> On 08/13/2018 04:24 AM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Fri, Aug 10, 2018 at 8:38 PM, Marek Vasut wrote:
On 08/10/2018 02:01 PM, Tom Rini wrote:
> On Wed, Aug 08, 2018 at 09:37:25PM +0200, Marek Vasut wrote:
>> On 08/08/2018 05:32 PM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut
>>> wrote:
On 08/08/2018 03:39 PM, Bin Meng wrote:
> Hi Marek,
>
> On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut
> wrote:
>> On 08/08/2018 03:14 PM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Wed, Aug 8, 2018 at 9:03 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
Cc: Simon Glass
---
drivers/pci/pci-uclass.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
index 46e9c71bdf..306bea0dbf 100644
--- a/drivers/pci/pci-uclass.c
+++ b/drivers/pci/pci-uclass.c
@@ -662,6 +662,8 @@ static int pci_find_and_bind_driver(struct
udevice *parent,
for (id = entry->match;
id->vendor || id->subvendor || id->class_mask;
id++) {
+ ofnode node;
+
if (!pci_match_one_id(id, find_id))
continue;
@@ -691,6 +693,18 @@ static int pci_find_and_bind_driver(struct
udevice *parent,
goto error;
debug("%s: Match found: %s\n", __func__,
drv->name);
dev->driver_data = find_id->driver_data;
+
+ dev_for_each_subnode(node, parent) {
+ phys_addr_t df, size;
+ df = ofnode_get_addr_size(node,
"reg", );
+
+ if (PCI_FUNC(df) == PCI_FUNC(bdf)
&&
+ PCI_DEV(df) == PCI_DEV(bdf)) {
+ dev->node = node;
+ break;
+ }
>>>
>>> The function pci_find_and_bind_driver() is supposed to bind devices
>>> that are NOT in the device tree. Adding device tree access in this
>>> routine is quite odd. You can add the EHCI controller that need such
>>> PHY subnodes in the device tree and there is no need to modify
>>> anything I believe. If you are looking for an example, please check
>>> pciuart0 in arch/x86/dts/crownbay.dts.
>>
>> Well this does not work for me, the EHCI PCI doesn't get a DT node
>> assigned, check r8a7794.dtsi for the PCI devices I use.
>>
>
> I think that's because you don't specify a "compatible" string for
> these two EHCI PCI nodes.
That's perfectly fine, why should I specify it ? Linux has no problem
with it either.
>>>
>>> Without a "compatible" string, DM does not bind any device in the
>>> device tree to a driver, hence no device node created. This is not
>>> Linux.
>>
>> DT is NOT Linux specific, it is OS-agnostic, DT describes hardware and
>> hardware only. If U-Boot cannot parse DT correctly, U-Boot is broken and
>> must be fixed.
>>
>> This is a fix. If there is a better fix, I am open to it.
>
> DT should but isn't always OS agnostic. DTS files that reside in the
> Linux Kernel are in practice is Linux-centric with the expectation that
> even if you could solve a given problem with valid DTS changes you make
> whatever is parsing it do additional logic instead. That,
> approximately, is what your patch is doing. If you added some HW
> description information to the dtsi file everything would work as
> expected as your DTS is describing the hardware and U-Boot is reading
> that description and figuring out what to do with it.
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Tom, On Tue, Aug 14, 2018 at 1:16 AM, Tom Rini wrote: > On Mon, Aug 13, 2018 at 06:07:14PM +0200, Marek Vasut wrote: >> On 08/13/2018 03:39 PM, Tom Rini wrote: >> [...] >> >> Next step is to upstream the DT >> changes to Linux kernel, then sync the changes to U-Boot to satisfy >> this obsession - using exactly the same DT as Linux. >> >>> >> >>> This is not gonna happen. >> >>> >> >>> Sorry, you're really just wasting my time with this foolishness. If >> >>> U-Boot cannot parse valid DT bindings while other OSes can, U-Boot is >> >>> broken and must be fixed. So far I only see you attacking this patch and >> >>> trying to pull in everything you can do avoid accepting this patch or >> >>> providing a better alternative. This is not a constructive discussion, >> >>> so I stop here. >> >> >> >> The fix in this patch is purely hack, period. >> > >> > Lets step back for a moment. >> > >> > First, U-Boot intends to be, in the case where a relevant DTS file >> > exists, the Linux kernel one PLUS additions we require (u-boot,dm-spl, >> > u-boot,dm-pre-reloc, but also sometimes stdout-path or properties that >> > are omitted for various reasons). >> >> Right, which doesn't apply here. None of those u-boot,... props are >> needed in this case. > > Which is why I also mentioned the non-u-boot specific props we also need > sometimes. My point is two-fold: > 1: We can and will _add_ information to the dts files that come from > Linux. > 2: Not all information that we add is U-Boot prefixed. > It would be better if we document such DT expectation somewhere. >> > Second, I've asked before (both in this thread and on IRC), and not >> > gotten an answer yet, as to how Linux goes "Oh, _this_ PCI device and >> > _this_ DT node need to be matched and populate some data >> > structures". >> >> You did get an answer to that on irc from George. Looks like >> of_pci_find_child_device() in drivers/pci/of.c > > Yeah, George said he thought that might be it but didn't have time to > confirm. > >> > Marek's patch seems to be, in short "here's where U-Boot >> > needs to wire things up". Bin has said that no, the function in >> > question is for other things. >> >> I disagree with this. It's a bind function and assigns other parameters >> of the driver instance too. >> >> > I think knowing where Linux does this >> > would be instructive to figure out where we need to have some additional >> > logic added OR we can make some cost/benefit analysis to see if it makes >> > more sense overall to add compatibles to some nodes rather than add to >> > the binary size. >> >> Adding compatible does not make any sense, the PCI ID provides that >> information. Adding compatible would only add redundancy which could >> possibly be even harmful (ie. if the controller got replaced with >> another one). > > To try and move things along rather than re-argue the same point, you're > saying that our pci_find_and_bind_driver() is the rough equivalent of > of_pci_find_child_device() or at least pci_set_of_node() (which calls > of_pci_find_child_device()). > > So, Bin, if this isn't the right place to start down this path, where > would be? Given that Linux can take a DTB and PCI bus with devices and > get things right, what would it look like for U-Boot to replicate the > same behavior? Instead of having to add explicit compatible nodes for > each PCI device, as I understand (but correct me if I'm wrong) we're > doing today. Thanks! So is this a requirement for all U-Boot driver subsystems to replicate the same Linux behavior? If yes, can we have it officially documented somewhere? Since Marek refused to take the original U-Boot option 1 to support his case, and asked U-Boot to follow Linux's practice on PCI device binding, if we go that way, here is what we can do: * Keep pci-uclass driver's post_bind() and child_post_bind() only for Sandbox configuration * Keep the call to pci_bus_find_devfn() in pci_bind_bus_devices() only for Sandbox configuration * Sandbox is special. We should limit the mechanism of matching PCI emulation device via "compatible" to sandbox only * Assign the DT node to the bound device in pci_find_and_bind_driver() if there is a valid PCI "reg" encoding for a specific PCI device in the device tree * Create DM PCI test case against the DT node assignment * Remove all compatible string in U-Boot's PCI device drivers: eg: ehci_pci_ids[], xhci_pci_ids[], etc. IOW, all PCI device drivers should only use U_BOOT_PCI_DEVICE(), aka the original U-Boot option 2 * Fork a "pci-ns16550" driver to support U_BOOT_PCI_DEVICE(), as currently PCI ns16550 device driver uses "compatible" string to do the matching, and update crownbay.dts and galileo.dts (so far I only know two boards are using PCI ns16550 serial port) * Make sure all DM PCI test cases are not broken * Document all of the above changes in doc/driver-model/pci-info.txt I am not sure if I missed anything. Simon, could you also comment on it? Regards, Bin
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Marek,
On Mon, Aug 13, 2018 at 9:46 PM, Marek Vasut wrote:
> On 08/13/2018 04:24 AM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Fri, Aug 10, 2018 at 8:38 PM, Marek Vasut wrote:
>>> On 08/10/2018 02:01 PM, Tom Rini wrote:
On Wed, Aug 08, 2018 at 09:37:25PM +0200, Marek Vasut wrote:
> On 08/08/2018 05:32 PM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut
>> wrote:
>>> On 08/08/2018 03:39 PM, Bin Meng wrote:
Hi Marek,
On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut
wrote:
> On 08/08/2018 03:14 PM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Wed, Aug 8, 2018 at 9:03 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
>>> Cc: Simon Glass
>>> ---
>>> drivers/pci/pci-uclass.c | 14 ++
>>> 1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
>>> index 46e9c71bdf..306bea0dbf 100644
>>> --- a/drivers/pci/pci-uclass.c
>>> +++ b/drivers/pci/pci-uclass.c
>>> @@ -662,6 +662,8 @@ static int pci_find_and_bind_driver(struct
>>> udevice *parent,
>>> for (id = entry->match;
>>> id->vendor || id->subvendor || id->class_mask;
>>> id++) {
>>> + ofnode node;
>>> +
>>> if (!pci_match_one_id(id, find_id))
>>> continue;
>>>
>>> @@ -691,6 +693,18 @@ static int pci_find_and_bind_driver(struct
>>> udevice *parent,
>>> goto error;
>>> debug("%s: Match found: %s\n", __func__,
>>> drv->name);
>>> dev->driver_data = find_id->driver_data;
>>> +
>>> + dev_for_each_subnode(node, parent) {
>>> + phys_addr_t df, size;
>>> + df = ofnode_get_addr_size(node,
>>> "reg", );
>>> +
>>> + if (PCI_FUNC(df) == PCI_FUNC(bdf) &&
>>> + PCI_DEV(df) == PCI_DEV(bdf)) {
>>> + dev->node = node;
>>> + break;
>>> + }
>>
>> The function pci_find_and_bind_driver() is supposed to bind devices
>> that are NOT in the device tree. Adding device tree access in this
>> routine is quite odd. You can add the EHCI controller that need such
>> PHY subnodes in the device tree and there is no need to modify
>> anything I believe. If you are looking for an example, please check
>> pciuart0 in arch/x86/dts/crownbay.dts.
>
> Well this does not work for me, the EHCI PCI doesn't get a DT node
> assigned, check r8a7794.dtsi for the PCI devices I use.
>
I think that's because you don't specify a "compatible" string for
these two EHCI PCI nodes.
>>>
>>> That's perfectly fine, why should I specify it ? Linux has no problem
>>> with it either.
>>>
>>
>> Without a "compatible" string, DM does not bind any device in the
>> device tree to a driver, hence no device node created. This is not
>> Linux.
>
> DT is NOT Linux specific, it is OS-agnostic, DT describes hardware and
> hardware only. If U-Boot cannot parse DT correctly, U-Boot is broken and
> must be fixed.
>
> This is a fix. If there is a better fix, I am open to it.
DT should but isn't always OS agnostic. DTS files that reside in the
Linux Kernel are in practice is Linux-centric with the expectation that
even if you could solve a given problem with valid DTS changes you make
whatever is parsing it do additional logic instead. That,
approximately, is what your patch is doing. If you added some HW
description information to the dtsi file everything would work as
expected as your DTS is describing the hardware and U-Boot is reading
that description and figuring out what to do with it.
>>>
>>> Yes, you need additional logic to match the PCI controller subnode in DT
>>> with PCI device BFD, that's expected. You do NOT need extra compatibles,
>>> the PCI bus
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On Mon, Aug 13, 2018 at 06:07:14PM +0200, Marek Vasut wrote: > On 08/13/2018 03:39 PM, Tom Rini wrote: > [...] > > Next step is to upstream the DT > changes to Linux kernel, then sync the changes to U-Boot to satisfy > this obsession - using exactly the same DT as Linux. > >>> > >>> This is not gonna happen. > >>> > >>> Sorry, you're really just wasting my time with this foolishness. If > >>> U-Boot cannot parse valid DT bindings while other OSes can, U-Boot is > >>> broken and must be fixed. So far I only see you attacking this patch and > >>> trying to pull in everything you can do avoid accepting this patch or > >>> providing a better alternative. This is not a constructive discussion, > >>> so I stop here. > >> > >> The fix in this patch is purely hack, period. > > > > Lets step back for a moment. > > > > First, U-Boot intends to be, in the case where a relevant DTS file > > exists, the Linux kernel one PLUS additions we require (u-boot,dm-spl, > > u-boot,dm-pre-reloc, but also sometimes stdout-path or properties that > > are omitted for various reasons). > > Right, which doesn't apply here. None of those u-boot,... props are > needed in this case. Which is why I also mentioned the non-u-boot specific props we also need sometimes. My point is two-fold: 1: We can and will _add_ information to the dts files that come from Linux. 2: Not all information that we add is U-Boot prefixed. > > Second, I've asked before (both in this thread and on IRC), and not > > gotten an answer yet, as to how Linux goes "Oh, _this_ PCI device and > > _this_ DT node need to be matched and populate some data > > structures". > > You did get an answer to that on irc from George. Looks like > of_pci_find_child_device() in drivers/pci/of.c Yeah, George said he thought that might be it but didn't have time to confirm. > > Marek's patch seems to be, in short "here's where U-Boot > > needs to wire things up". Bin has said that no, the function in > > question is for other things. > > I disagree with this. It's a bind function and assigns other parameters > of the driver instance too. > > > I think knowing where Linux does this > > would be instructive to figure out where we need to have some additional > > logic added OR we can make some cost/benefit analysis to see if it makes > > more sense overall to add compatibles to some nodes rather than add to > > the binary size. > > Adding compatible does not make any sense, the PCI ID provides that > information. Adding compatible would only add redundancy which could > possibly be even harmful (ie. if the controller got replaced with > another one). To try and move things along rather than re-argue the same point, you're saying that our pci_find_and_bind_driver() is the rough equivalent of of_pci_find_child_device() or at least pci_set_of_node() (which calls of_pci_find_child_device()). So, Bin, if this isn't the right place to start down this path, where would be? Given that Linux can take a DTB and PCI bus with devices and get things right, what would it look like for U-Boot to replicate the same behavior? Instead of having to add explicit compatible nodes for each PCI device, as I understand (but correct me if I'm wrong) we're doing today. Thanks! -- Tom signature.asc Description: PGP signature ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/13/2018 03:39 PM, Tom Rini wrote: [...] Next step is to upstream the DT changes to Linux kernel, then sync the changes to U-Boot to satisfy this obsession - using exactly the same DT as Linux. >>> >>> This is not gonna happen. >>> >>> Sorry, you're really just wasting my time with this foolishness. If >>> U-Boot cannot parse valid DT bindings while other OSes can, U-Boot is >>> broken and must be fixed. So far I only see you attacking this patch and >>> trying to pull in everything you can do avoid accepting this patch or >>> providing a better alternative. This is not a constructive discussion, >>> so I stop here. >> >> The fix in this patch is purely hack, period. > > Lets step back for a moment. > > First, U-Boot intends to be, in the case where a relevant DTS file > exists, the Linux kernel one PLUS additions we require (u-boot,dm-spl, > u-boot,dm-pre-reloc, but also sometimes stdout-path or properties that > are omitted for various reasons). Right, which doesn't apply here. None of those u-boot,... props are needed in this case. > Second, I've asked before (both in this thread and on IRC), and not > gotten an answer yet, as to how Linux goes "Oh, _this_ PCI device and > _this_ DT node need to be matched and populate some data > structures". You did get an answer to that on irc from George. Looks like of_pci_find_child_device() indrivers/pci/of.c > Marek's patch seems to be, in short "here's where U-Boot > needs to wire things up". Bin has said that no, the function in > question is for other things. I disagree with this. It's a bind function and assigns other parameters of the driver instance too. > I think knowing where Linux does this > would be instructive to figure out where we need to have some additional > logic added OR we can make some cost/benefit analysis to see if it makes > more sense overall to add compatibles to some nodes rather than add to > the binary size. Adding compatible does not make any sense, the PCI ID provides that information. Adding compatible would only add redundancy which could possibly be even harmful (ie. if the controller got replaced with another one). -- Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/13/2018 04:24 AM, Bin Meng wrote:
> Hi Marek,
>
> On Fri, Aug 10, 2018 at 8:38 PM, Marek Vasut wrote:
>> On 08/10/2018 02:01 PM, Tom Rini wrote:
>>> On Wed, Aug 08, 2018 at 09:37:25PM +0200, Marek Vasut wrote:
On 08/08/2018 05:32 PM, Bin Meng wrote:
> Hi Marek,
>
> On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut
> wrote:
>> On 08/08/2018 03:39 PM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut
>>> wrote:
On 08/08/2018 03:14 PM, Bin Meng wrote:
> Hi Marek,
>
> On Wed, Aug 8, 2018 at 9:03 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
>> Cc: Simon Glass
>> ---
>> drivers/pci/pci-uclass.c | 14 ++
>> 1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
>> index 46e9c71bdf..306bea0dbf 100644
>> --- a/drivers/pci/pci-uclass.c
>> +++ b/drivers/pci/pci-uclass.c
>> @@ -662,6 +662,8 @@ static int pci_find_and_bind_driver(struct
>> udevice *parent,
>> for (id = entry->match;
>> id->vendor || id->subvendor || id->class_mask;
>> id++) {
>> + ofnode node;
>> +
>> if (!pci_match_one_id(id, find_id))
>> continue;
>>
>> @@ -691,6 +693,18 @@ static int pci_find_and_bind_driver(struct
>> udevice *parent,
>> goto error;
>> debug("%s: Match found: %s\n", __func__,
>> drv->name);
>> dev->driver_data = find_id->driver_data;
>> +
>> + dev_for_each_subnode(node, parent) {
>> + phys_addr_t df, size;
>> + df = ofnode_get_addr_size(node,
>> "reg", );
>> +
>> + if (PCI_FUNC(df) == PCI_FUNC(bdf) &&
>> + PCI_DEV(df) == PCI_DEV(bdf)) {
>> + dev->node = node;
>> + break;
>> + }
>
> The function pci_find_and_bind_driver() is supposed to bind devices
> that are NOT in the device tree. Adding device tree access in this
> routine is quite odd. You can add the EHCI controller that need such
> PHY subnodes in the device tree and there is no need to modify
> anything I believe. If you are looking for an example, please check
> pciuart0 in arch/x86/dts/crownbay.dts.
Well this does not work for me, the EHCI PCI doesn't get a DT node
assigned, check r8a7794.dtsi for the PCI devices I use.
>>>
>>> I think that's because you don't specify a "compatible" string for
>>> these two EHCI PCI nodes.
>>
>> That's perfectly fine, why should I specify it ? Linux has no problem
>> with it either.
>>
>
> Without a "compatible" string, DM does not bind any device in the
> device tree to a driver, hence no device node created. This is not
> Linux.
DT is NOT Linux specific, it is OS-agnostic, DT describes hardware and
hardware only. If U-Boot cannot parse DT correctly, U-Boot is broken and
must be fixed.
This is a fix. If there is a better fix, I am open to it.
>>>
>>> DT should but isn't always OS agnostic. DTS files that reside in the
>>> Linux Kernel are in practice is Linux-centric with the expectation that
>>> even if you could solve a given problem with valid DTS changes you make
>>> whatever is parsing it do additional logic instead. That,
>>> approximately, is what your patch is doing. If you added some HW
>>> description information to the dtsi file everything would work as
>>> expected as your DTS is describing the hardware and U-Boot is reading
>>> that description and figuring out what to do with it.
>>
>> Yes, you need additional logic to match the PCI controller subnode in DT
>> with PCI device BFD, that's expected. You do NOT need extra compatibles,
>> the PCI bus gives you enough information to match a driver on them. In
>> fact, adding a compatible can interfere with this matching.
>>
>
> Please, read U-Boot's
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/13/2018 04:07 AM, Bin Meng wrote: [...] > Sorry this is terribly wrong. Imagine someone > writes another OS, and all he has is the device tree spec. He follows > the spec and writes some codes to parse a valid DT, and it's done. How > his OS makes use of the DT is his design decision and none of the > device tree specs has hard requiement on it. Of course, using exact > the same DT as Linux is nice-to-have feature but that's not the reason > to attack has OS has bugs to parse DT. There is supposed to be only one DT for all the OSes, which has nothing to do with Linux or U-Boot or any of those. If some OS cannot parse an valid DT completely, then it's lacking. >>> >>> I really don't understand such obsessiveness. >> >> I can say the same thing, I don't understand why you're trying so hard >> to protect the lacking PCI DT parsing code in U-Boot. That is the part >> which needs to be fixed. >> >>> Currently there is NO >>> ALL OSes. There is ONLY Linux. >> >> Er, no. You forgot all the BSDs, SunOS and possibly a few others I forgot. > > In fact, I didn't. I compared many dts files in BSD source codes > against the dts files in Linux tree (with the same name, for the same > board), and they _are_ unsurprisingly different. Some of nodes indeed > have "compatible" string while the other one does not, which reveals > the truth that what you kept saying all OSes should use exactly the > same DT is purely wrong. You are too Linux-centric. Contradiction (!truth) implies anything, that's logic 101. >>> What you kept talking about is the >>> reference implementation from Linux. You need convince me adding a >>> compatible string to the USB node makes the DT invalid. If you admit >>> adding "compatible" is perfectly OK, then per your theory Linux should >>> handle it without any problems. >> >> I think I lost you here. I won't pollute this thread with even more >> crap. USB is a probable bus, just like PCI, you don't need compatible >> strings there either. >> > > Didn't you see the "PCI Bus Binding" spec? It's a spec existing for years. > >>> Next step is to upstream the DT >>> changes to Linux kernel, then sync the changes to U-Boot to satisfy >>> this obsession - using exactly the same DT as Linux. >> >> This is not gonna happen. >> >> Sorry, you're really just wasting my time with this foolishness. If >> U-Boot cannot parse valid DT bindings while other OSes can, U-Boot is >> broken and must be fixed. So far I only see you attacking this patch and >> trying to pull in everything you can do avoid accepting this patch or >> providing a better alternative. This is not a constructive discussion, >> so I stop here. >> > > The fix in this patch is purely hack, period. So while you're constantly yelling "hack" "hack" here, I don't see any constructive feedback. Can you provide any ? -- Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On Mon, Aug 13, 2018 at 10:07:03AM +0800, Bin Meng wrote: > Hi Marek, > > On Fri, Aug 10, 2018 at 6:32 PM, Marek Vasut wrote: > > On 08/10/2018 05:42 AM, Bin Meng wrote: > >> Hi Marek, > >> > >> On Thu, Aug 9, 2018 at 6:25 PM, Marek Vasut wrote: > >>> On 08/09/2018 11:41 AM, Bin Meng wrote: > >>> > >>> [...] > >>> > Sorry this is a hack to current U-Boot implementation, not fix. > >>> > >>> I am waiting for a better solution or suggestion ... > >>> > The fix should be adding "ehci-pci" compatible string in the > r8a7794.dtsi. > >>> > >>> Wrong. The DT is perfectly valid as is. > >>> > >> > >> I did not say r8a7794.dtsi is invalid. Being valid does not mean it > >> works everywhere. Being valid only means its syntax follows the DTS > >> language and does not cause any build error. Adding a "compatible" > >> string to a DT node is also perfectly valid. See "Binding Guidelines" > >> in devicetree-specification-v0.2.pdf [1] > > > > Hacking up DT to work around bugs in an OS implementation makes it OS > > specific and this is incorrect. It is the OS that should be fixed, in > > this case U-Boot. > > > > It's not a bug of U-Boot implementation. U-Boot DM was designed this > way. > >>> > >>> Then the design is flawed. (and btw. the U-Boot DM was not designed that > >>> way, you can take my word for it :-) ) > >>> > >> > >> Then please propose something that that is not flawed. But this patch > >> is not a fix to the design, but a hack. > > > > You are mixing two things in this answer, so it makes no sense. Anyway, > > let me just stop you here, DM was not designed in such a way, period. > > > If you are in U-Boot, you must follow U-Boot's rule. And I don't > see current DM is a wrong design. > >>> > >>> Wrong, U-Boot is given some hardware to run on and a matching > >>> description, DT. If both are valid and U-Boot cannot operate on them, > >>> then U-Boot is broken and must be fixed. > >>> > >>> You cannot claim that just because U-Boot behaves in some way, a > >>> perfectly good hardware and/or it's description is wrong and needs to be > >>> adjusted. That makes no sense. > >>> > >> > >> Again, who said current dts is *invalid*? You keep saying "valid", > >> "valid"... Can you explain why adding a perfectly OK "compatible" > >> string suddenly makes the DT invalid? > > > > Adding a compatible to DT may or may not make it invalid. > > > > Yet, the DT at hand is valid already, so adding extra crap into it just > > to work around buggy or incomplete implementation of PCI DT parsing is > > not an option. > > > >>> Just assume the DT is in ROM, it's set in stone and it cannot be > >>> changed. Then the entire discussion below becomes pointless and we can > >>> focus on fixing U-Boot such that it'd be able to parse the DT properly. > >>> > >> > >> Sometimes we have to update DT as not every working DT is born from day 1. > > > > This DT at hand is working and valid. It cannot and will not be updated > > only because U-Boot PCI implementation is lacking. > > > > Keep in mind that the DT may even be stored in ROM and can not be > > modified. > > > >> 4.1 Binding Guidelines > >> 4.1.1 General Principles > >> When creating a new devicetree representation for a device, a binding > >> should be created that fully describes the required properties and > >> value of the device. This set of properties shall be sufficiently > >> descriptive to provide device drivers with needed attributes of the > >> device. Some recommended practices include: > >> 1. Define a *compatible* string using the conventions described in > >> section 2.3.1 > >> ... > > > > Yes, "recommended" . The compatible string is not a hard requirement. > > OK, since you mentioned "recommended", let me paste the bullet 2 from > the same spec: > > Some recommended practices include: > 1. Define a compatible string using the conventions described in section > 2.3.1. > 2. Use the standard properties (defined in sections 2.3 and 2.4) as > applicable for the new device. This usage typically includes the *reg* > and interrupts properties at a minimum. > 3. Use the conventions specified in section 4 (Device Bindings) if the > new device fits into one the DTSpec defined device classes > ... > > Bullet 2 says: This usage *typically* includes the *reg* and > interrupts properties at a minimum. That means "reg" is not mandatory. > > And now let's see this patch: > > df = ofnode_get_addr_size(node, "reg", ); > > Who says a node must have a "reg" property? Using this as a sign to > attach to DT is definitely wrong. > >>> > >>> If there is no "reg" property, then you won't get a node associated with > >>> the driver instance and that's perfectly fine. Without the "reg"
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Marek,
On Fri, Aug 10, 2018 at 8:38 PM, Marek Vasut wrote:
> On 08/10/2018 02:01 PM, Tom Rini wrote:
>> On Wed, Aug 08, 2018 at 09:37:25PM +0200, Marek Vasut wrote:
>>> On 08/08/2018 05:32 PM, Bin Meng wrote:
Hi Marek,
On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut wrote:
> On 08/08/2018 03:39 PM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut
>> wrote:
>>> On 08/08/2018 03:14 PM, Bin Meng wrote:
Hi Marek,
On Wed, Aug 8, 2018 at 9:03 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
> Cc: Simon Glass
> ---
> drivers/pci/pci-uclass.c | 14 ++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
> index 46e9c71bdf..306bea0dbf 100644
> --- a/drivers/pci/pci-uclass.c
> +++ b/drivers/pci/pci-uclass.c
> @@ -662,6 +662,8 @@ static int pci_find_and_bind_driver(struct
> udevice *parent,
> for (id = entry->match;
> id->vendor || id->subvendor || id->class_mask;
> id++) {
> + ofnode node;
> +
> if (!pci_match_one_id(id, find_id))
> continue;
>
> @@ -691,6 +693,18 @@ static int pci_find_and_bind_driver(struct
> udevice *parent,
> goto error;
> debug("%s: Match found: %s\n", __func__,
> drv->name);
> dev->driver_data = find_id->driver_data;
> +
> + dev_for_each_subnode(node, parent) {
> + phys_addr_t df, size;
> + df = ofnode_get_addr_size(node,
> "reg", );
> +
> + if (PCI_FUNC(df) == PCI_FUNC(bdf) &&
> + PCI_DEV(df) == PCI_DEV(bdf)) {
> + dev->node = node;
> + break;
> + }
The function pci_find_and_bind_driver() is supposed to bind devices
that are NOT in the device tree. Adding device tree access in this
routine is quite odd. You can add the EHCI controller that need such
PHY subnodes in the device tree and there is no need to modify
anything I believe. If you are looking for an example, please check
pciuart0 in arch/x86/dts/crownbay.dts.
>>>
>>> Well this does not work for me, the EHCI PCI doesn't get a DT node
>>> assigned, check r8a7794.dtsi for the PCI devices I use.
>>>
>>
>> I think that's because you don't specify a "compatible" string for
>> these two EHCI PCI nodes.
>
> That's perfectly fine, why should I specify it ? Linux has no problem
> with it either.
>
Without a "compatible" string, DM does not bind any device in the
device tree to a driver, hence no device node created. This is not
Linux.
>>>
>>> DT is NOT Linux specific, it is OS-agnostic, DT describes hardware and
>>> hardware only. If U-Boot cannot parse DT correctly, U-Boot is broken and
>>> must be fixed.
>>>
>>> This is a fix. If there is a better fix, I am open to it.
>>
>> DT should but isn't always OS agnostic. DTS files that reside in the
>> Linux Kernel are in practice is Linux-centric with the expectation that
>> even if you could solve a given problem with valid DTS changes you make
>> whatever is parsing it do additional logic instead. That,
>> approximately, is what your patch is doing. If you added some HW
>> description information to the dtsi file everything would work as
>> expected as your DTS is describing the hardware and U-Boot is reading
>> that description and figuring out what to do with it.
>
> Yes, you need additional logic to match the PCI controller subnode in DT
> with PCI device BFD, that's expected. You do NOT need extra compatibles,
> the PCI bus gives you enough information to match a driver on them. In
> fact, adding a compatible can interfere with this matching.
>
Please, read U-Boot's doc/driver-model/pci-info.txt. You really don't
understand current implementation in U-Boot. In short, U-Boot supports
two scenarios for PCI driver binding:
- Declare a PCI device
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Marek, On Fri, Aug 10, 2018 at 6:32 PM, Marek Vasut wrote: > On 08/10/2018 05:42 AM, Bin Meng wrote: >> Hi Marek, >> >> On Thu, Aug 9, 2018 at 6:25 PM, Marek Vasut wrote: >>> On 08/09/2018 11:41 AM, Bin Meng wrote: >>> >>> [...] >>> Sorry this is a hack to current U-Boot implementation, not fix. >>> >>> I am waiting for a better solution or suggestion ... >>> The fix should be adding "ehci-pci" compatible string in the r8a7794.dtsi. >>> >>> Wrong. The DT is perfectly valid as is. >>> >> >> I did not say r8a7794.dtsi is invalid. Being valid does not mean it >> works everywhere. Being valid only means its syntax follows the DTS >> language and does not cause any build error. Adding a "compatible" >> string to a DT node is also perfectly valid. See "Binding Guidelines" >> in devicetree-specification-v0.2.pdf [1] > > Hacking up DT to work around bugs in an OS implementation makes it OS > specific and this is incorrect. It is the OS that should be fixed, in > this case U-Boot. > It's not a bug of U-Boot implementation. U-Boot DM was designed this way. >>> >>> Then the design is flawed. (and btw. the U-Boot DM was not designed that >>> way, you can take my word for it :-) ) >>> >> >> Then please propose something that that is not flawed. But this patch >> is not a fix to the design, but a hack. > > You are mixing two things in this answer, so it makes no sense. Anyway, > let me just stop you here, DM was not designed in such a way, period. > If you are in U-Boot, you must follow U-Boot's rule. And I don't see current DM is a wrong design. >>> >>> Wrong, U-Boot is given some hardware to run on and a matching >>> description, DT. If both are valid and U-Boot cannot operate on them, >>> then U-Boot is broken and must be fixed. >>> >>> You cannot claim that just because U-Boot behaves in some way, a >>> perfectly good hardware and/or it's description is wrong and needs to be >>> adjusted. That makes no sense. >>> >> >> Again, who said current dts is *invalid*? You keep saying "valid", >> "valid"... Can you explain why adding a perfectly OK "compatible" >> string suddenly makes the DT invalid? > > Adding a compatible to DT may or may not make it invalid. > > Yet, the DT at hand is valid already, so adding extra crap into it just > to work around buggy or incomplete implementation of PCI DT parsing is > not an option. > >>> Just assume the DT is in ROM, it's set in stone and it cannot be >>> changed. Then the entire discussion below becomes pointless and we can >>> focus on fixing U-Boot such that it'd be able to parse the DT properly. >>> >> >> Sometimes we have to update DT as not every working DT is born from day 1. > > This DT at hand is working and valid. It cannot and will not be updated > only because U-Boot PCI implementation is lacking. > > Keep in mind that the DT may even be stored in ROM and can not be > modified. > >> 4.1 Binding Guidelines >> 4.1.1 General Principles >> When creating a new devicetree representation for a device, a binding >> should be created that fully describes the required properties and >> value of the device. This set of properties shall be sufficiently >> descriptive to provide device drivers with needed attributes of the >> device. Some recommended practices include: >> 1. Define a *compatible* string using the conventions described in >> section 2.3.1 >> ... > > Yes, "recommended" . The compatible string is not a hard requirement. OK, since you mentioned "recommended", let me paste the bullet 2 from the same spec: Some recommended practices include: 1. Define a compatible string using the conventions described in section 2.3.1. 2. Use the standard properties (defined in sections 2.3 and 2.4) as applicable for the new device. This usage typically includes the *reg* and interrupts properties at a minimum. 3. Use the conventions specified in section 4 (Device Bindings) if the new device fits into one the DTSpec defined device classes ... Bullet 2 says: This usage *typically* includes the *reg* and interrupts properties at a minimum. That means "reg" is not mandatory. And now let's see this patch: df = ofnode_get_addr_size(node, "reg", ); Who says a node must have a "reg" property? Using this as a sign to attach to DT is definitely wrong. >>> >>> If there is no "reg" property, then you won't get a node associated with >>> the driver instance and that's perfectly fine. Without the "reg" prop, >>> you cannot figure out which PCI device/function the node describes, so >>> how could you possibly associate the node with a driver instance ? You >>> cannot. >>> >> >> This is a hack. I said many times if you add a "compatible" string DM >> create the device node for you. > > I
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/10/2018 02:01 PM, Tom Rini wrote:
> On Wed, Aug 08, 2018 at 09:37:25PM +0200, Marek Vasut wrote:
>> On 08/08/2018 05:32 PM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut wrote:
On 08/08/2018 03:39 PM, Bin Meng wrote:
> Hi Marek,
>
> On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut wrote:
>> On 08/08/2018 03:14 PM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Wed, Aug 8, 2018 at 9:03 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
Cc: Simon Glass
---
drivers/pci/pci-uclass.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
index 46e9c71bdf..306bea0dbf 100644
--- a/drivers/pci/pci-uclass.c
+++ b/drivers/pci/pci-uclass.c
@@ -662,6 +662,8 @@ static int pci_find_and_bind_driver(struct udevice
*parent,
for (id = entry->match;
id->vendor || id->subvendor || id->class_mask;
id++) {
+ ofnode node;
+
if (!pci_match_one_id(id, find_id))
continue;
@@ -691,6 +693,18 @@ static int pci_find_and_bind_driver(struct
udevice *parent,
goto error;
debug("%s: Match found: %s\n", __func__,
drv->name);
dev->driver_data = find_id->driver_data;
+
+ dev_for_each_subnode(node, parent) {
+ phys_addr_t df, size;
+ df = ofnode_get_addr_size(node, "reg",
);
+
+ if (PCI_FUNC(df) == PCI_FUNC(bdf) &&
+ PCI_DEV(df) == PCI_DEV(bdf)) {
+ dev->node = node;
+ break;
+ }
>>>
>>> The function pci_find_and_bind_driver() is supposed to bind devices
>>> that are NOT in the device tree. Adding device tree access in this
>>> routine is quite odd. You can add the EHCI controller that need such
>>> PHY subnodes in the device tree and there is no need to modify
>>> anything I believe. If you are looking for an example, please check
>>> pciuart0 in arch/x86/dts/crownbay.dts.
>>
>> Well this does not work for me, the EHCI PCI doesn't get a DT node
>> assigned, check r8a7794.dtsi for the PCI devices I use.
>>
>
> I think that's because you don't specify a "compatible" string for
> these two EHCI PCI nodes.
That's perfectly fine, why should I specify it ? Linux has no problem
with it either.
>>>
>>> Without a "compatible" string, DM does not bind any device in the
>>> device tree to a driver, hence no device node created. This is not
>>> Linux.
>>
>> DT is NOT Linux specific, it is OS-agnostic, DT describes hardware and
>> hardware only. If U-Boot cannot parse DT correctly, U-Boot is broken and
>> must be fixed.
>>
>> This is a fix. If there is a better fix, I am open to it.
>
> DT should but isn't always OS agnostic. DTS files that reside in the
> Linux Kernel are in practice is Linux-centric with the expectation that
> even if you could solve a given problem with valid DTS changes you make
> whatever is parsing it do additional logic instead. That,
> approximately, is what your patch is doing. If you added some HW
> description information to the dtsi file everything would work as
> expected as your DTS is describing the hardware and U-Boot is reading
> that description and figuring out what to do with it.
Yes, you need additional logic to match the PCI controller subnode in DT
with PCI device BFD, that's expected. You do NOT need extra compatibles,
the PCI bus gives you enough information to match a driver on them. In
fact, adding a compatible can interfere with this matching.
> The problem here is that in Linux, something that sees the compatible
> renesas,pci-r8a7794 or renesas,pci-rcar-gen2 is doing whatever else
> needs to be done. Or something else? Please explain how what you want
> to have work here in U-Boot is working in the Linux kernel. Thanks!
This has nothing to do with a specific controller. iMX6 does the same
thing, see ie
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On Wed, Aug 08, 2018 at 09:37:25PM +0200, Marek Vasut wrote:
> On 08/08/2018 05:32 PM, Bin Meng wrote:
> > Hi Marek,
> >
> > On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut wrote:
> >> On 08/08/2018 03:39 PM, Bin Meng wrote:
> >>> Hi Marek,
> >>>
> >>> On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut wrote:
> On 08/08/2018 03:14 PM, Bin Meng wrote:
> > Hi Marek,
> >
> > On Wed, Aug 8, 2018 at 9:03 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
> >> Cc: Simon Glass
> >> ---
> >> drivers/pci/pci-uclass.c | 14 ++
> >> 1 file changed, 14 insertions(+)
> >>
> >> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
> >> index 46e9c71bdf..306bea0dbf 100644
> >> --- a/drivers/pci/pci-uclass.c
> >> +++ b/drivers/pci/pci-uclass.c
> >> @@ -662,6 +662,8 @@ static int pci_find_and_bind_driver(struct udevice
> >> *parent,
> >> for (id = entry->match;
> >> id->vendor || id->subvendor || id->class_mask;
> >> id++) {
> >> + ofnode node;
> >> +
> >> if (!pci_match_one_id(id, find_id))
> >> continue;
> >>
> >> @@ -691,6 +693,18 @@ static int pci_find_and_bind_driver(struct
> >> udevice *parent,
> >> goto error;
> >> debug("%s: Match found: %s\n", __func__,
> >> drv->name);
> >> dev->driver_data = find_id->driver_data;
> >> +
> >> + dev_for_each_subnode(node, parent) {
> >> + phys_addr_t df, size;
> >> + df = ofnode_get_addr_size(node, "reg",
> >> );
> >> +
> >> + if (PCI_FUNC(df) == PCI_FUNC(bdf) &&
> >> + PCI_DEV(df) == PCI_DEV(bdf)) {
> >> + dev->node = node;
> >> + break;
> >> + }
> >
> > The function pci_find_and_bind_driver() is supposed to bind devices
> > that are NOT in the device tree. Adding device tree access in this
> > routine is quite odd. You can add the EHCI controller that need such
> > PHY subnodes in the device tree and there is no need to modify
> > anything I believe. If you are looking for an example, please check
> > pciuart0 in arch/x86/dts/crownbay.dts.
>
> Well this does not work for me, the EHCI PCI doesn't get a DT node
> assigned, check r8a7794.dtsi for the PCI devices I use.
>
> >>>
> >>> I think that's because you don't specify a "compatible" string for
> >>> these two EHCI PCI nodes.
> >>
> >> That's perfectly fine, why should I specify it ? Linux has no problem
> >> with it either.
> >>
> >
> > Without a "compatible" string, DM does not bind any device in the
> > device tree to a driver, hence no device node created. This is not
> > Linux.
>
> DT is NOT Linux specific, it is OS-agnostic, DT describes hardware and
> hardware only. If U-Boot cannot parse DT correctly, U-Boot is broken and
> must be fixed.
>
> This is a fix. If there is a better fix, I am open to it.
DT should but isn't always OS agnostic. DTS files that reside in the
Linux Kernel are in practice is Linux-centric with the expectation that
even if you could solve a given problem with valid DTS changes you make
whatever is parsing it do additional logic instead. That,
approximately, is what your patch is doing. If you added some HW
description information to the dtsi file everything would work as
expected as your DTS is describing the hardware and U-Boot is reading
that description and figuring out what to do with it.
The problem here is that in Linux, something that sees the compatible
renesas,pci-r8a7794 or renesas,pci-rcar-gen2 is doing whatever else
needs to be done. Or something else? Please explain how what you want
to have work here in U-Boot is working in the Linux kernel. Thanks!
--
Tom
signature.asc
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/10/2018 05:42 AM, Bin Meng wrote: > Hi Marek, > > On Thu, Aug 9, 2018 at 6:25 PM, Marek Vasut wrote: >> On 08/09/2018 11:41 AM, Bin Meng wrote: >> >> [...] >> >>> Sorry this is a hack to current U-Boot implementation, not fix. >> >> I am waiting for a better solution or suggestion ... >> >>> The fix should be adding "ehci-pci" compatible string in the >>> r8a7794.dtsi. >> >> Wrong. The DT is perfectly valid as is. >> > > I did not say r8a7794.dtsi is invalid. Being valid does not mean it > works everywhere. Being valid only means its syntax follows the DTS > language and does not cause any build error. Adding a "compatible" > string to a DT node is also perfectly valid. See "Binding Guidelines" > in devicetree-specification-v0.2.pdf [1] Hacking up DT to work around bugs in an OS implementation makes it OS specific and this is incorrect. It is the OS that should be fixed, in this case U-Boot. >>> >>> It's not a bug of U-Boot implementation. U-Boot DM was designed this >>> way. >> >> Then the design is flawed. (and btw. the U-Boot DM was not designed that >> way, you can take my word for it :-) ) >> > > Then please propose something that that is not flawed. But this patch > is not a fix to the design, but a hack. You are mixing two things in this answer, so it makes no sense. Anyway, let me just stop you here, DM was not designed in such a way, period. >>> If you are in U-Boot, you must follow U-Boot's rule. And I don't >>> see current DM is a wrong design. >> >> Wrong, U-Boot is given some hardware to run on and a matching >> description, DT. If both are valid and U-Boot cannot operate on them, >> then U-Boot is broken and must be fixed. >> >> You cannot claim that just because U-Boot behaves in some way, a >> perfectly good hardware and/or it's description is wrong and needs to be >> adjusted. That makes no sense. >> > > Again, who said current dts is *invalid*? You keep saying "valid", > "valid"... Can you explain why adding a perfectly OK "compatible" > string suddenly makes the DT invalid? Adding a compatible to DT may or may not make it invalid. Yet, the DT at hand is valid already, so adding extra crap into it just to work around buggy or incomplete implementation of PCI DT parsing is not an option. >> Just assume the DT is in ROM, it's set in stone and it cannot be >> changed. Then the entire discussion below becomes pointless and we can >> focus on fixing U-Boot such that it'd be able to parse the DT properly. >> > > Sometimes we have to update DT as not every working DT is born from day 1. This DT at hand is working and valid. It cannot and will not be updated only because U-Boot PCI implementation is lacking. Keep in mind that the DT may even be stored in ROM and can not be modified. > 4.1 Binding Guidelines > 4.1.1 General Principles > When creating a new devicetree representation for a device, a binding > should be created that fully describes the required properties and > value of the device. This set of properties shall be sufficiently > descriptive to provide device drivers with needed attributes of the > device. Some recommended practices include: > 1. Define a *compatible* string using the conventions described in > section 2.3.1 > ... Yes, "recommended" . The compatible string is not a hard requirement. >>> >>> OK, since you mentioned "recommended", let me paste the bullet 2 from >>> the same spec: >>> >>> Some recommended practices include: >>> 1. Define a compatible string using the conventions described in section >>> 2.3.1. >>> 2. Use the standard properties (defined in sections 2.3 and 2.4) as >>> applicable for the new device. This usage typically includes the *reg* >>> and interrupts properties at a minimum. >>> 3. Use the conventions specified in section 4 (Device Bindings) if the >>> new device fits into one the DTSpec defined device classes >>> ... >>> >>> Bullet 2 says: This usage *typically* includes the *reg* and >>> interrupts properties at a minimum. That means "reg" is not mandatory. >>> >>> And now let's see this patch: >>> >>> df = ofnode_get_addr_size(node, "reg", ); >>> >>> Who says a node must have a "reg" property? Using this as a sign to >>> attach to DT is definitely wrong. >> >> If there is no "reg" property, then you won't get a node associated with >> the driver instance and that's perfectly fine. Without the "reg" prop, >> you cannot figure out which PCI device/function the node describes, so >> how could you possibly associate the node with a driver instance ? You >> cannot. >> > > This is a hack. I said many times if you add a "compatible" string DM > create the device node for you. I explained many times that the DT will not be updated to work around lacking implementation of PCI DT parsing in U-Boot. The DT is perfectly fine as-is. I also explained already that the PCI ID or class is
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Marek, On Thu, Aug 9, 2018 at 6:25 PM, Marek Vasut wrote: > On 08/09/2018 11:41 AM, Bin Meng wrote: > > [...] > >> Sorry this is a hack to current U-Boot implementation, not fix. > > I am waiting for a better solution or suggestion ... > >> The fix should be adding "ehci-pci" compatible string in the >> r8a7794.dtsi. > > Wrong. The DT is perfectly valid as is. > I did not say r8a7794.dtsi is invalid. Being valid does not mean it works everywhere. Being valid only means its syntax follows the DTS language and does not cause any build error. Adding a "compatible" string to a DT node is also perfectly valid. See "Binding Guidelines" in devicetree-specification-v0.2.pdf [1] >>> >>> Hacking up DT to work around bugs in an OS implementation makes it OS >>> specific and this is incorrect. It is the OS that should be fixed, in >>> this case U-Boot. >>> >> >> It's not a bug of U-Boot implementation. U-Boot DM was designed this >> way. > > Then the design is flawed. (and btw. the U-Boot DM was not designed that > way, you can take my word for it :-) ) > Then please propose something that that is not flawed. But this patch is not a fix to the design, but a hack. >> If you are in U-Boot, you must follow U-Boot's rule. And I don't >> see current DM is a wrong design. > > Wrong, U-Boot is given some hardware to run on and a matching > description, DT. If both are valid and U-Boot cannot operate on them, > then U-Boot is broken and must be fixed. > > You cannot claim that just because U-Boot behaves in some way, a > perfectly good hardware and/or it's description is wrong and needs to be > adjusted. That makes no sense. > Again, who said current dts is *invalid*? You keep saying "valid", "valid"... Can you explain why adding a perfectly OK "compatible" string suddenly makes the DT invalid? > Just assume the DT is in ROM, it's set in stone and it cannot be > changed. Then the entire discussion below becomes pointless and we can > focus on fixing U-Boot such that it'd be able to parse the DT properly. > Sometimes we have to update DT as not every working DT is born from day 1. >>> Keep in mind that the DT may even be stored in ROM and can not be modified. >>> 4.1 Binding Guidelines 4.1.1 General Principles When creating a new devicetree representation for a device, a binding should be created that fully describes the required properties and value of the device. This set of properties shall be sufficiently descriptive to provide device drivers with needed attributes of the device. Some recommended practices include: 1. Define a *compatible* string using the conventions described in section 2.3.1 ... >>> >>> Yes, "recommended" . The compatible string is not a hard requirement. >> >> OK, since you mentioned "recommended", let me paste the bullet 2 from >> the same spec: >> >> Some recommended practices include: >> 1. Define a compatible string using the conventions described in section >> 2.3.1. >> 2. Use the standard properties (defined in sections 2.3 and 2.4) as >> applicable for the new device. This usage typically includes the *reg* >> and interrupts properties at a minimum. >> 3. Use the conventions specified in section 4 (Device Bindings) if the >> new device fits into one the DTSpec defined device classes >> ... >> >> Bullet 2 says: This usage *typically* includes the *reg* and >> interrupts properties at a minimum. That means "reg" is not mandatory. >> >> And now let's see this patch: >> >> df = ofnode_get_addr_size(node, "reg", ); >> >> Who says a node must have a "reg" property? Using this as a sign to >> attach to DT is definitely wrong. > > If there is no "reg" property, then you won't get a node associated with > the driver instance and that's perfectly fine. Without the "reg" prop, > you cannot figure out which PCI device/function the node describes, so > how could you possibly associate the node with a driver instance ? You > cannot. > This is a hack. I said many times if you add a "compatible" string DM create the device node for you. > The device sitting at a particular slot/function can very well be ie. > xhci controller and the DT node would be valid for it too, unless you > enforce a compatible, which will mess things up. > > Each PCI device already has a PCI ID and class which is used to identify > it and based on which the drivers bind to it, so a DT compatible is NOT > needed and is actually redundant and harmful. > No, it's not redundant but complementary to existing PCI enumeration (vendor/device/class/subclass...) mechanism. Please check "PCI Bus Binding" specification [2] which defines how we should describe a PCI device using "compatible" string. "compatible" Construct a list of names in most-specific to least-specific order. The names shall be derived from values of the Vendor ID,
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/09/2018 11:41 AM, Bin Meng wrote: [...] > Sorry this is a hack to current U-Boot implementation, not fix. I am waiting for a better solution or suggestion ... > The fix should be adding "ehci-pci" compatible string in the r8a7794.dtsi. Wrong. The DT is perfectly valid as is. >>> >>> I did not say r8a7794.dtsi is invalid. Being valid does not mean it >>> works everywhere. Being valid only means its syntax follows the DTS >>> language and does not cause any build error. Adding a "compatible" >>> string to a DT node is also perfectly valid. See "Binding Guidelines" >>> in devicetree-specification-v0.2.pdf [1] >> >> Hacking up DT to work around bugs in an OS implementation makes it OS >> specific and this is incorrect. It is the OS that should be fixed, in >> this case U-Boot. >> > > It's not a bug of U-Boot implementation. U-Boot DM was designed this > way. Then the design is flawed. (and btw. the U-Boot DM was not designed that way, you can take my word for it :-) ) > If you are in U-Boot, you must follow U-Boot's rule. And I don't > see current DM is a wrong design. Wrong, U-Boot is given some hardware to run on and a matching description, DT. If both are valid and U-Boot cannot operate on them, then U-Boot is broken and must be fixed. You cannot claim that just because U-Boot behaves in some way, a perfectly good hardware and/or it's description is wrong and needs to be adjusted. That makes no sense. Just assume the DT is in ROM, it's set in stone and it cannot be changed. Then the entire discussion below becomes pointless and we can focus on fixing U-Boot such that it'd be able to parse the DT properly. >> Keep in mind that the DT may even be stored in ROM and can not be modified. >> >>> 4.1 Binding Guidelines >>> 4.1.1 General Principles >>> When creating a new devicetree representation for a device, a binding >>> should be created that fully describes the required properties and >>> value of the device. This set of properties shall be sufficiently >>> descriptive to provide device drivers with needed attributes of the >>> device. Some recommended practices include: >>> 1. Define a *compatible* string using the conventions described in section >>> 2.3.1 >>> ... >> >> Yes, "recommended" . The compatible string is not a hard requirement. > > OK, since you mentioned "recommended", let me paste the bullet 2 from > the same spec: > > Some recommended practices include: > 1. Define a compatible string using the conventions described in section > 2.3.1. > 2. Use the standard properties (defined in sections 2.3 and 2.4) as > applicable for the new device. This usage typically includes the *reg* > and interrupts properties at a minimum. > 3. Use the conventions specified in section 4 (Device Bindings) if the > new device fits into one the DTSpec defined device classes > ... > > Bullet 2 says: This usage *typically* includes the *reg* and > interrupts properties at a minimum. That means "reg" is not mandatory. > > And now let's see this patch: > > df = ofnode_get_addr_size(node, "reg", ); > > Who says a node must have a "reg" property? Using this as a sign to > attach to DT is definitely wrong. If there is no "reg" property, then you won't get a node associated with the driver instance and that's perfectly fine. Without the "reg" prop, you cannot figure out which PCI device/function the node describes, so how could you possibly associate the node with a driver instance ? You cannot. The device sitting at a particular slot/function can very well be ie. xhci controller and the DT node would be valid for it too, unless you enforce a compatible, which will mess things up. Each PCI device already has a PCI ID and class which is used to identify it and based on which the drivers bind to it, so a DT compatible is NOT needed and is actually redundant and harmful. >>> >>> No, it's not redundant but complementary to existing PCI enumeration >>> (vendor/device/class/subclass...) mechanism. Please check "PCI Bus >>> Binding" specification [2] which defines how we should describe a PCI >>> device using "compatible" string. >>> >>> "compatible" Construct a list of names in most-specific to >>> least-specific order. The names shall be derived from values of the >>> Vendor ID, Device ID, Subsystem Vendor ID, Subsystem ID, Revision ID >>> and Class Code bytes, and shall have the following form, and be placed >>> in the list in the following order: >>> pci,...RR >>> pci,.. >>> pci, >>> pci,.RR >>> pci, >>> pciclass,CCSSPP >>> pciclass,CCSS >>> ... >> >> Where does it say that the "compatible" string is mandatory ? I thought >> you yourself quoted a paragraph from that spec which says it's >> recommended, which means optional. >> > > You can't say adding an optional "compatible" is wrong either. Being > optional means an implementation can choose to support it
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Marek,
On Thu, Aug 9, 2018 at 3:54 PM, Marek Vasut wrote:
> On 08/09/2018 04:37 AM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Thu, Aug 9, 2018 at 8:36 AM, Marek Vasut wrote:
>>> On 08/09/2018 01:24 AM, Bin Meng wrote:
Hi Marek,
On Thu, Aug 9, 2018 at 3:37 AM, Marek Vasut wrote:
> On 08/08/2018 05:32 PM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut
>> wrote:
>>> On 08/08/2018 03:39 PM, Bin Meng wrote:
Hi Marek,
On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut
wrote:
> On 08/08/2018 03:14 PM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Wed, Aug 8, 2018 at 9:03 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
>>> Cc: Simon Glass
>>> ---
>>> drivers/pci/pci-uclass.c | 14 ++
>>> 1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
>>> index 46e9c71bdf..306bea0dbf 100644
>>> --- a/drivers/pci/pci-uclass.c
>>> +++ b/drivers/pci/pci-uclass.c
>>> @@ -662,6 +662,8 @@ static int pci_find_and_bind_driver(struct
>>> udevice *parent,
>>> for (id = entry->match;
>>> id->vendor || id->subvendor || id->class_mask;
>>> id++) {
>>> + ofnode node;
>>> +
>>> if (!pci_match_one_id(id, find_id))
>>> continue;
>>>
>>> @@ -691,6 +693,18 @@ static int pci_find_and_bind_driver(struct
>>> udevice *parent,
>>> goto error;
>>> debug("%s: Match found: %s\n", __func__,
>>> drv->name);
>>> dev->driver_data = find_id->driver_data;
>>> +
>>> + dev_for_each_subnode(node, parent) {
>>> + phys_addr_t df, size;
>>> + df = ofnode_get_addr_size(node,
>>> "reg", );
>>> +
>>> + if (PCI_FUNC(df) == PCI_FUNC(bdf) &&
>>> + PCI_DEV(df) == PCI_DEV(bdf)) {
>>> + dev->node = node;
>>> + break;
>>> + }
>>
>> The function pci_find_and_bind_driver() is supposed to bind devices
>> that are NOT in the device tree. Adding device tree access in this
>> routine is quite odd. You can add the EHCI controller that need such
>> PHY subnodes in the device tree and there is no need to modify
>> anything I believe. If you are looking for an example, please check
>> pciuart0 in arch/x86/dts/crownbay.dts.
>
> Well this does not work for me, the EHCI PCI doesn't get a DT node
> assigned, check r8a7794.dtsi for the PCI devices I use.
>
I think that's because you don't specify a "compatible" string for
these two EHCI PCI nodes.
>>>
>>> That's perfectly fine, why should I specify it ? Linux has no problem
>>> with it either.
>>>
>>
>> Without a "compatible" string, DM does not bind any device in the
>> device tree to a driver, hence no device node created. This is not
>> Linux.
>
> DT is NOT Linux specific, it is OS-agnostic, DT describes hardware and
> hardware only. If U-Boot cannot parse DT correctly, U-Boot is broken and
> must be fixed.
>
> This is a fix. If there is a better fix, I am open to it.
>
Sorry this is a hack to current U-Boot implementation, not fix.
>>>
>>> I am waiting for a better solution or suggestion ...
>>>
The fix should be adding "ehci-pci" compatible string in the r8a7794.dtsi.
>>>
>>> Wrong. The DT is perfectly valid as is.
>>>
>>
>> I did not say r8a7794.dtsi is invalid. Being valid does not mean it
>> works everywhere. Being valid only means its syntax follows the DTS
>> language and does not cause any build error. Adding a "compatible"
>> string to a DT node is also perfectly valid. See "Binding Guidelines"
>> in devicetree-specification-v0.2.pdf [1]
>
> Hacking up DT to work around bugs in an OS implementation makes it OS
> specific and this is incorrect. It is the OS that should
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/09/2018 04:37 AM, Bin Meng wrote:
> Hi Marek,
>
> On Thu, Aug 9, 2018 at 8:36 AM, Marek Vasut wrote:
>> On 08/09/2018 01:24 AM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Thu, Aug 9, 2018 at 3:37 AM, Marek Vasut wrote:
On 08/08/2018 05:32 PM, Bin Meng wrote:
> Hi Marek,
>
> On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut
> wrote:
>> On 08/08/2018 03:39 PM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut
>>> wrote:
On 08/08/2018 03:14 PM, Bin Meng wrote:
> Hi Marek,
>
> On Wed, Aug 8, 2018 at 9:03 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
>> Cc: Simon Glass
>> ---
>> drivers/pci/pci-uclass.c | 14 ++
>> 1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
>> index 46e9c71bdf..306bea0dbf 100644
>> --- a/drivers/pci/pci-uclass.c
>> +++ b/drivers/pci/pci-uclass.c
>> @@ -662,6 +662,8 @@ static int pci_find_and_bind_driver(struct
>> udevice *parent,
>> for (id = entry->match;
>> id->vendor || id->subvendor || id->class_mask;
>> id++) {
>> + ofnode node;
>> +
>> if (!pci_match_one_id(id, find_id))
>> continue;
>>
>> @@ -691,6 +693,18 @@ static int pci_find_and_bind_driver(struct
>> udevice *parent,
>> goto error;
>> debug("%s: Match found: %s\n", __func__,
>> drv->name);
>> dev->driver_data = find_id->driver_data;
>> +
>> + dev_for_each_subnode(node, parent) {
>> + phys_addr_t df, size;
>> + df = ofnode_get_addr_size(node,
>> "reg", );
>> +
>> + if (PCI_FUNC(df) == PCI_FUNC(bdf) &&
>> + PCI_DEV(df) == PCI_DEV(bdf)) {
>> + dev->node = node;
>> + break;
>> + }
>
> The function pci_find_and_bind_driver() is supposed to bind devices
> that are NOT in the device tree. Adding device tree access in this
> routine is quite odd. You can add the EHCI controller that need such
> PHY subnodes in the device tree and there is no need to modify
> anything I believe. If you are looking for an example, please check
> pciuart0 in arch/x86/dts/crownbay.dts.
Well this does not work for me, the EHCI PCI doesn't get a DT node
assigned, check r8a7794.dtsi for the PCI devices I use.
>>>
>>> I think that's because you don't specify a "compatible" string for
>>> these two EHCI PCI nodes.
>>
>> That's perfectly fine, why should I specify it ? Linux has no problem
>> with it either.
>>
>
> Without a "compatible" string, DM does not bind any device in the
> device tree to a driver, hence no device node created. This is not
> Linux.
DT is NOT Linux specific, it is OS-agnostic, DT describes hardware and
hardware only. If U-Boot cannot parse DT correctly, U-Boot is broken and
must be fixed.
This is a fix. If there is a better fix, I am open to it.
>>>
>>> Sorry this is a hack to current U-Boot implementation, not fix.
>>
>> I am waiting for a better solution or suggestion ...
>>
>>> The fix should be adding "ehci-pci" compatible string in the r8a7794.dtsi.
>>
>> Wrong. The DT is perfectly valid as is.
>>
>
> I did not say r8a7794.dtsi is invalid. Being valid does not mean it
> works everywhere. Being valid only means its syntax follows the DTS
> language and does not cause any build error. Adding a "compatible"
> string to a DT node is also perfectly valid. See "Binding Guidelines"
> in devicetree-specification-v0.2.pdf [1]
Hacking up DT to work around bugs in an OS implementation makes it OS
specific and this is incorrect. It is the OS that should be fixed, in
this case U-Boot.
Keep in mind that the DT may even be stored in ROM and can not be modified.
> 4.1 Binding Guidelines
> 4.1.1 General Principles
> When creating a
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Marek,
On Thu, Aug 9, 2018 at 8:36 AM, Marek Vasut wrote:
> On 08/09/2018 01:24 AM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Thu, Aug 9, 2018 at 3:37 AM, Marek Vasut wrote:
>>> On 08/08/2018 05:32 PM, Bin Meng wrote:
Hi Marek,
On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut wrote:
> On 08/08/2018 03:39 PM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut
>> wrote:
>>> On 08/08/2018 03:14 PM, Bin Meng wrote:
Hi Marek,
On Wed, Aug 8, 2018 at 9:03 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
> Cc: Simon Glass
> ---
> drivers/pci/pci-uclass.c | 14 ++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
> index 46e9c71bdf..306bea0dbf 100644
> --- a/drivers/pci/pci-uclass.c
> +++ b/drivers/pci/pci-uclass.c
> @@ -662,6 +662,8 @@ static int pci_find_and_bind_driver(struct
> udevice *parent,
> for (id = entry->match;
> id->vendor || id->subvendor || id->class_mask;
> id++) {
> + ofnode node;
> +
> if (!pci_match_one_id(id, find_id))
> continue;
>
> @@ -691,6 +693,18 @@ static int pci_find_and_bind_driver(struct
> udevice *parent,
> goto error;
> debug("%s: Match found: %s\n", __func__,
> drv->name);
> dev->driver_data = find_id->driver_data;
> +
> + dev_for_each_subnode(node, parent) {
> + phys_addr_t df, size;
> + df = ofnode_get_addr_size(node,
> "reg", );
> +
> + if (PCI_FUNC(df) == PCI_FUNC(bdf) &&
> + PCI_DEV(df) == PCI_DEV(bdf)) {
> + dev->node = node;
> + break;
> + }
The function pci_find_and_bind_driver() is supposed to bind devices
that are NOT in the device tree. Adding device tree access in this
routine is quite odd. You can add the EHCI controller that need such
PHY subnodes in the device tree and there is no need to modify
anything I believe. If you are looking for an example, please check
pciuart0 in arch/x86/dts/crownbay.dts.
>>>
>>> Well this does not work for me, the EHCI PCI doesn't get a DT node
>>> assigned, check r8a7794.dtsi for the PCI devices I use.
>>>
>>
>> I think that's because you don't specify a "compatible" string for
>> these two EHCI PCI nodes.
>
> That's perfectly fine, why should I specify it ? Linux has no problem
> with it either.
>
Without a "compatible" string, DM does not bind any device in the
device tree to a driver, hence no device node created. This is not
Linux.
>>>
>>> DT is NOT Linux specific, it is OS-agnostic, DT describes hardware and
>>> hardware only. If U-Boot cannot parse DT correctly, U-Boot is broken and
>>> must be fixed.
>>>
>>> This is a fix. If there is a better fix, I am open to it.
>>>
>>
>> Sorry this is a hack to current U-Boot implementation, not fix.
>
> I am waiting for a better solution or suggestion ...
>
>> The fix should be adding "ehci-pci" compatible string in the r8a7794.dtsi.
>
> Wrong. The DT is perfectly valid as is.
>
I did not say r8a7794.dtsi is invalid. Being valid does not mean it
works everywhere. Being valid only means its syntax follows the DTS
language and does not cause any build error. Adding a "compatible"
string to a DT node is also perfectly valid. See "Binding Guidelines"
in devicetree-specification-v0.2.pdf [1]
4.1 Binding Guidelines
4.1.1 General Principles
When creating a new devicetree representation for a device, a binding
should be created that fully describes the required properties and
value of the device. This set of properties shall be sufficiently
descriptive to provide device drivers with needed attributes of the
device. Some recommended practices include:
1. Define a *compatible* string using the conventions described in section 2.3.1
...
> The device sitting
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/09/2018 01:24 AM, Bin Meng wrote:
> Hi Marek,
>
> On Thu, Aug 9, 2018 at 3:37 AM, Marek Vasut wrote:
>> On 08/08/2018 05:32 PM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut wrote:
On 08/08/2018 03:39 PM, Bin Meng wrote:
> Hi Marek,
>
> On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut wrote:
>> On 08/08/2018 03:14 PM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Wed, Aug 8, 2018 at 9:03 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
Cc: Simon Glass
---
drivers/pci/pci-uclass.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
index 46e9c71bdf..306bea0dbf 100644
--- a/drivers/pci/pci-uclass.c
+++ b/drivers/pci/pci-uclass.c
@@ -662,6 +662,8 @@ static int pci_find_and_bind_driver(struct udevice
*parent,
for (id = entry->match;
id->vendor || id->subvendor || id->class_mask;
id++) {
+ ofnode node;
+
if (!pci_match_one_id(id, find_id))
continue;
@@ -691,6 +693,18 @@ static int pci_find_and_bind_driver(struct
udevice *parent,
goto error;
debug("%s: Match found: %s\n", __func__,
drv->name);
dev->driver_data = find_id->driver_data;
+
+ dev_for_each_subnode(node, parent) {
+ phys_addr_t df, size;
+ df = ofnode_get_addr_size(node, "reg",
);
+
+ if (PCI_FUNC(df) == PCI_FUNC(bdf) &&
+ PCI_DEV(df) == PCI_DEV(bdf)) {
+ dev->node = node;
+ break;
+ }
>>>
>>> The function pci_find_and_bind_driver() is supposed to bind devices
>>> that are NOT in the device tree. Adding device tree access in this
>>> routine is quite odd. You can add the EHCI controller that need such
>>> PHY subnodes in the device tree and there is no need to modify
>>> anything I believe. If you are looking for an example, please check
>>> pciuart0 in arch/x86/dts/crownbay.dts.
>>
>> Well this does not work for me, the EHCI PCI doesn't get a DT node
>> assigned, check r8a7794.dtsi for the PCI devices I use.
>>
>
> I think that's because you don't specify a "compatible" string for
> these two EHCI PCI nodes.
That's perfectly fine, why should I specify it ? Linux has no problem
with it either.
>>>
>>> Without a "compatible" string, DM does not bind any device in the
>>> device tree to a driver, hence no device node created. This is not
>>> Linux.
>>
>> DT is NOT Linux specific, it is OS-agnostic, DT describes hardware and
>> hardware only. If U-Boot cannot parse DT correctly, U-Boot is broken and
>> must be fixed.
>>
>> This is a fix. If there is a better fix, I am open to it.
>>
>
> Sorry this is a hack to current U-Boot implementation, not fix.
I am waiting for a better solution or suggestion ...
> The fix should be adding "ehci-pci" compatible string in the r8a7794.dtsi.
Wrong. The DT is perfectly valid as is.
The device sitting at a particular slot/function can very well be ie.
xhci controller and the DT node would be valid for it too, unless you
enforce a compatible, which will mess things up.
Each PCI device already has a PCI ID and class which is used to identify
it and based on which the drivers bind to it, so a DT compatible is NOT
needed and is actually redundant and harmful.
What is needed here is to assign a valid DT node to a driver instance of
a PCI device if such a matching node exists in DT and that is all this
patch does.
> I disagree DT is OS-agnostic. This are lots of stuff in DT that are
> OS-specific. eg: there are lots of bindings in DT that requires
> Linux's device driver framework to work with.
This logic is flawed. If there exists a binding which depends on some
behavior of specific OS then the binding is likely wrong. That
specifically does not imply DT is OS-specific. Again, it is not and that
is by design. The DT must
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Marek,
On Thu, Aug 9, 2018 at 3:37 AM, Marek Vasut wrote:
> On 08/08/2018 05:32 PM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut wrote:
>>> On 08/08/2018 03:39 PM, Bin Meng wrote:
Hi Marek,
On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut wrote:
> On 08/08/2018 03:14 PM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Wed, Aug 8, 2018 at 9:03 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
>>> Cc: Simon Glass
>>> ---
>>> drivers/pci/pci-uclass.c | 14 ++
>>> 1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
>>> index 46e9c71bdf..306bea0dbf 100644
>>> --- a/drivers/pci/pci-uclass.c
>>> +++ b/drivers/pci/pci-uclass.c
>>> @@ -662,6 +662,8 @@ static int pci_find_and_bind_driver(struct udevice
>>> *parent,
>>> for (id = entry->match;
>>> id->vendor || id->subvendor || id->class_mask;
>>> id++) {
>>> + ofnode node;
>>> +
>>> if (!pci_match_one_id(id, find_id))
>>> continue;
>>>
>>> @@ -691,6 +693,18 @@ static int pci_find_and_bind_driver(struct udevice
>>> *parent,
>>> goto error;
>>> debug("%s: Match found: %s\n", __func__,
>>> drv->name);
>>> dev->driver_data = find_id->driver_data;
>>> +
>>> + dev_for_each_subnode(node, parent) {
>>> + phys_addr_t df, size;
>>> + df = ofnode_get_addr_size(node, "reg",
>>> );
>>> +
>>> + if (PCI_FUNC(df) == PCI_FUNC(bdf) &&
>>> + PCI_DEV(df) == PCI_DEV(bdf)) {
>>> + dev->node = node;
>>> + break;
>>> + }
>>
>> The function pci_find_and_bind_driver() is supposed to bind devices
>> that are NOT in the device tree. Adding device tree access in this
>> routine is quite odd. You can add the EHCI controller that need such
>> PHY subnodes in the device tree and there is no need to modify
>> anything I believe. If you are looking for an example, please check
>> pciuart0 in arch/x86/dts/crownbay.dts.
>
> Well this does not work for me, the EHCI PCI doesn't get a DT node
> assigned, check r8a7794.dtsi for the PCI devices I use.
>
I think that's because you don't specify a "compatible" string for
these two EHCI PCI nodes.
>>>
>>> That's perfectly fine, why should I specify it ? Linux has no problem
>>> with it either.
>>>
>>
>> Without a "compatible" string, DM does not bind any device in the
>> device tree to a driver, hence no device node created. This is not
>> Linux.
>
> DT is NOT Linux specific, it is OS-agnostic, DT describes hardware and
> hardware only. If U-Boot cannot parse DT correctly, U-Boot is broken and
> must be fixed.
>
> This is a fix. If there is a better fix, I am open to it.
>
Sorry this is a hack to current U-Boot implementation, not fix. The
fix should be adding "ehci-pci" compatible string in the r8a7794.dtsi.
I disagree DT is OS-agnostic. This are lots of stuff in DT that are
OS-specific. eg: there are lots of bindings in DT that requires
Linux's device driver framework to work with. As you said, DT is just
a standard to describe hardware and hardware only. But there are
various methods to describe hardware in DT that's why we have a proper
defined bindings in Linux. How OS parses and utilizes these
information is completely on their own.
Regards,
Bin
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/08/2018 05:32 PM, Bin Meng wrote:
> Hi Marek,
>
> On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut wrote:
>> On 08/08/2018 03:39 PM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut wrote:
On 08/08/2018 03:14 PM, Bin Meng wrote:
> Hi Marek,
>
> On Wed, Aug 8, 2018 at 9:03 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
>> Cc: Simon Glass
>> ---
>> drivers/pci/pci-uclass.c | 14 ++
>> 1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
>> index 46e9c71bdf..306bea0dbf 100644
>> --- a/drivers/pci/pci-uclass.c
>> +++ b/drivers/pci/pci-uclass.c
>> @@ -662,6 +662,8 @@ static int pci_find_and_bind_driver(struct udevice
>> *parent,
>> for (id = entry->match;
>> id->vendor || id->subvendor || id->class_mask;
>> id++) {
>> + ofnode node;
>> +
>> if (!pci_match_one_id(id, find_id))
>> continue;
>>
>> @@ -691,6 +693,18 @@ static int pci_find_and_bind_driver(struct udevice
>> *parent,
>> goto error;
>> debug("%s: Match found: %s\n", __func__,
>> drv->name);
>> dev->driver_data = find_id->driver_data;
>> +
>> + dev_for_each_subnode(node, parent) {
>> + phys_addr_t df, size;
>> + df = ofnode_get_addr_size(node, "reg",
>> );
>> +
>> + if (PCI_FUNC(df) == PCI_FUNC(bdf) &&
>> + PCI_DEV(df) == PCI_DEV(bdf)) {
>> + dev->node = node;
>> + break;
>> + }
>
> The function pci_find_and_bind_driver() is supposed to bind devices
> that are NOT in the device tree. Adding device tree access in this
> routine is quite odd. You can add the EHCI controller that need such
> PHY subnodes in the device tree and there is no need to modify
> anything I believe. If you are looking for an example, please check
> pciuart0 in arch/x86/dts/crownbay.dts.
Well this does not work for me, the EHCI PCI doesn't get a DT node
assigned, check r8a7794.dtsi for the PCI devices I use.
>>>
>>> I think that's because you don't specify a "compatible" string for
>>> these two EHCI PCI nodes.
>>
>> That's perfectly fine, why should I specify it ? Linux has no problem
>> with it either.
>>
>
> Without a "compatible" string, DM does not bind any device in the
> device tree to a driver, hence no device node created. This is not
> Linux.
DT is NOT Linux specific, it is OS-agnostic, DT describes hardware and
hardware only. If U-Boot cannot parse DT correctly, U-Boot is broken and
must be fixed.
This is a fix. If there is a better fix, I am open to it.
--
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Marek,
On Wed, Aug 8, 2018 at 10:33 PM, Marek Vasut wrote:
> On 08/08/2018 03:39 PM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut wrote:
>>> On 08/08/2018 03:14 PM, Bin Meng wrote:
Hi Marek,
On Wed, Aug 8, 2018 at 9:03 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
> Cc: Simon Glass
> ---
> drivers/pci/pci-uclass.c | 14 ++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
> index 46e9c71bdf..306bea0dbf 100644
> --- a/drivers/pci/pci-uclass.c
> +++ b/drivers/pci/pci-uclass.c
> @@ -662,6 +662,8 @@ static int pci_find_and_bind_driver(struct udevice
> *parent,
> for (id = entry->match;
> id->vendor || id->subvendor || id->class_mask;
> id++) {
> + ofnode node;
> +
> if (!pci_match_one_id(id, find_id))
> continue;
>
> @@ -691,6 +693,18 @@ static int pci_find_and_bind_driver(struct udevice
> *parent,
> goto error;
> debug("%s: Match found: %s\n", __func__,
> drv->name);
> dev->driver_data = find_id->driver_data;
> +
> + dev_for_each_subnode(node, parent) {
> + phys_addr_t df, size;
> + df = ofnode_get_addr_size(node, "reg",
> );
> +
> + if (PCI_FUNC(df) == PCI_FUNC(bdf) &&
> + PCI_DEV(df) == PCI_DEV(bdf)) {
> + dev->node = node;
> + break;
> + }
The function pci_find_and_bind_driver() is supposed to bind devices
that are NOT in the device tree. Adding device tree access in this
routine is quite odd. You can add the EHCI controller that need such
PHY subnodes in the device tree and there is no need to modify
anything I believe. If you are looking for an example, please check
pciuart0 in arch/x86/dts/crownbay.dts.
>>>
>>> Well this does not work for me, the EHCI PCI doesn't get a DT node
>>> assigned, check r8a7794.dtsi for the PCI devices I use.
>>>
>>
>> I think that's because you don't specify a "compatible" string for
>> these two EHCI PCI nodes.
>
> That's perfectly fine, why should I specify it ? Linux has no problem
> with it either.
>
Without a "compatible" string, DM does not bind any device in the
device tree to a driver, hence no device node created. This is not
Linux.
Regards,
Bin
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/08/2018 03:39 PM, Bin Meng wrote:
> Hi Marek,
>
> On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut wrote:
>> On 08/08/2018 03:14 PM, Bin Meng wrote:
>>> Hi Marek,
>>>
>>> On Wed, Aug 8, 2018 at 9:03 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
Cc: Simon Glass
---
drivers/pci/pci-uclass.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
index 46e9c71bdf..306bea0dbf 100644
--- a/drivers/pci/pci-uclass.c
+++ b/drivers/pci/pci-uclass.c
@@ -662,6 +662,8 @@ static int pci_find_and_bind_driver(struct udevice
*parent,
for (id = entry->match;
id->vendor || id->subvendor || id->class_mask;
id++) {
+ ofnode node;
+
if (!pci_match_one_id(id, find_id))
continue;
@@ -691,6 +693,18 @@ static int pci_find_and_bind_driver(struct udevice
*parent,
goto error;
debug("%s: Match found: %s\n", __func__,
drv->name);
dev->driver_data = find_id->driver_data;
+
+ dev_for_each_subnode(node, parent) {
+ phys_addr_t df, size;
+ df = ofnode_get_addr_size(node, "reg",
);
+
+ if (PCI_FUNC(df) == PCI_FUNC(bdf) &&
+ PCI_DEV(df) == PCI_DEV(bdf)) {
+ dev->node = node;
+ break;
+ }
>>>
>>> The function pci_find_and_bind_driver() is supposed to bind devices
>>> that are NOT in the device tree. Adding device tree access in this
>>> routine is quite odd. You can add the EHCI controller that need such
>>> PHY subnodes in the device tree and there is no need to modify
>>> anything I believe. If you are looking for an example, please check
>>> pciuart0 in arch/x86/dts/crownbay.dts.
>>
>> Well this does not work for me, the EHCI PCI doesn't get a DT node
>> assigned, check r8a7794.dtsi for the PCI devices I use.
>>
>
> I think that's because you don't specify a "compatible" string for
> these two EHCI PCI nodes.
That's perfectly fine, why should I specify it ? Linux has no problem
with it either.
--
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Marek,
On Wed, Aug 8, 2018 at 9:24 PM, Marek Vasut wrote:
> On 08/08/2018 03:14 PM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Wed, Aug 8, 2018 at 9:03 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
>>> Cc: Simon Glass
>>> ---
>>> drivers/pci/pci-uclass.c | 14 ++
>>> 1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
>>> index 46e9c71bdf..306bea0dbf 100644
>>> --- a/drivers/pci/pci-uclass.c
>>> +++ b/drivers/pci/pci-uclass.c
>>> @@ -662,6 +662,8 @@ static int pci_find_and_bind_driver(struct udevice
>>> *parent,
>>> for (id = entry->match;
>>> id->vendor || id->subvendor || id->class_mask;
>>> id++) {
>>> + ofnode node;
>>> +
>>> if (!pci_match_one_id(id, find_id))
>>> continue;
>>>
>>> @@ -691,6 +693,18 @@ static int pci_find_and_bind_driver(struct udevice
>>> *parent,
>>> goto error;
>>> debug("%s: Match found: %s\n", __func__, drv->name);
>>> dev->driver_data = find_id->driver_data;
>>> +
>>> + dev_for_each_subnode(node, parent) {
>>> + phys_addr_t df, size;
>>> + df = ofnode_get_addr_size(node, "reg",
>>> );
>>> +
>>> + if (PCI_FUNC(df) == PCI_FUNC(bdf) &&
>>> + PCI_DEV(df) == PCI_DEV(bdf)) {
>>> + dev->node = node;
>>> + break;
>>> + }
>>
>> The function pci_find_and_bind_driver() is supposed to bind devices
>> that are NOT in the device tree. Adding device tree access in this
>> routine is quite odd. You can add the EHCI controller that need such
>> PHY subnodes in the device tree and there is no need to modify
>> anything I believe. If you are looking for an example, please check
>> pciuart0 in arch/x86/dts/crownbay.dts.
>
> Well this does not work for me, the EHCI PCI doesn't get a DT node
> assigned, check r8a7794.dtsi for the PCI devices I use.
>
I think that's because you don't specify a "compatible" string for
these two EHCI PCI nodes.
Regards,
Bin
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
On 08/08/2018 03:14 PM, Bin Meng wrote:
> Hi Marek,
>
> On Wed, Aug 8, 2018 at 9:03 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
>> Cc: Simon Glass
>> ---
>> drivers/pci/pci-uclass.c | 14 ++
>> 1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
>> index 46e9c71bdf..306bea0dbf 100644
>> --- a/drivers/pci/pci-uclass.c
>> +++ b/drivers/pci/pci-uclass.c
>> @@ -662,6 +662,8 @@ static int pci_find_and_bind_driver(struct udevice
>> *parent,
>> for (id = entry->match;
>> id->vendor || id->subvendor || id->class_mask;
>> id++) {
>> + ofnode node;
>> +
>> if (!pci_match_one_id(id, find_id))
>> continue;
>>
>> @@ -691,6 +693,18 @@ static int pci_find_and_bind_driver(struct udevice
>> *parent,
>> goto error;
>> debug("%s: Match found: %s\n", __func__, drv->name);
>> dev->driver_data = find_id->driver_data;
>> +
>> + dev_for_each_subnode(node, parent) {
>> + phys_addr_t df, size;
>> + df = ofnode_get_addr_size(node, "reg",
>> );
>> +
>> + if (PCI_FUNC(df) == PCI_FUNC(bdf) &&
>> + PCI_DEV(df) == PCI_DEV(bdf)) {
>> + dev->node = node;
>> + break;
>> + }
>
> The function pci_find_and_bind_driver() is supposed to bind devices
> that are NOT in the device tree. Adding device tree access in this
> routine is quite odd. You can add the EHCI controller that need such
> PHY subnodes in the device tree and there is no need to modify
> anything I believe. If you are looking for an example, please check
> pciuart0 in arch/x86/dts/crownbay.dts.
Well this does not work for me, the EHCI PCI doesn't get a DT node
assigned, check r8a7794.dtsi for the PCI devices I use.
--
Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] pci: Support parsing PCI controller DT subnodes
Hi Marek,
On Wed, Aug 8, 2018 at 9:03 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
> Cc: Simon Glass
> ---
> drivers/pci/pci-uclass.c | 14 ++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
> index 46e9c71bdf..306bea0dbf 100644
> --- a/drivers/pci/pci-uclass.c
> +++ b/drivers/pci/pci-uclass.c
> @@ -662,6 +662,8 @@ static int pci_find_and_bind_driver(struct udevice
> *parent,
> for (id = entry->match;
> id->vendor || id->subvendor || id->class_mask;
> id++) {
> + ofnode node;
> +
> if (!pci_match_one_id(id, find_id))
> continue;
>
> @@ -691,6 +693,18 @@ static int pci_find_and_bind_driver(struct udevice
> *parent,
> goto error;
> debug("%s: Match found: %s\n", __func__, drv->name);
> dev->driver_data = find_id->driver_data;
> +
> + dev_for_each_subnode(node, parent) {
> + phys_addr_t df, size;
> + df = ofnode_get_addr_size(node, "reg", );
> +
> + if (PCI_FUNC(df) == PCI_FUNC(bdf) &&
> + PCI_DEV(df) == PCI_DEV(bdf)) {
> + dev->node = node;
> + break;
> + }
The function pci_find_and_bind_driver() is supposed to bind devices
that are NOT in the device tree. Adding device tree access in this
routine is quite odd. You can add the EHCI controller that need such
PHY subnodes in the device tree and there is no need to modify
anything I believe. If you are looking for an example, please check
pciuart0 in arch/x86/dts/crownbay.dts.
> + }
> +
> *devp = dev;
> return 0;
> }
> --
Regards,
Bin
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
