Re: [U-Boot] [PATCH V2 1/2] pci: Support parsing PCI controller DT subnodes
Hi Marek, On 1 September 2018 at 16:45, Marek Vasut wrote: > > On 09/01/2018 11:50 PM, Simon Glass wrote: > > Hi Marek, > > > > On 30 August 2018 at 07:42, Marek Vasut wrote: > >> On 08/30/2018 03:32 PM, Bin Meng wrote: > >>> Hi Marek, > >>> > >>> On Thu, Aug 30, 2018 at 1:07 AM Marek Vasut wrote: > > On 08/29/2018 05:15 PM, Bin Meng wrote: > > +Simon > > > > Hi Marek, > > > > On Wed, Aug 29, 2018 at 10:22 PM Marek Vasut > > wrote: > >> > >> On 08/24/2018 08:27 PM, Marek Vasut wrote: > >>> The PCI controller can have DT subnodes describing extra properties > >>> of particular PCI devices, ie. a PHY attached to an EHCI controller > >>> on a PCI bus. This patch parses those DT subnodes and assigns a node > >>> to the PCI device instance, so that the driver can extract details > >>> from that node and ie. configure the PHY using the PHY subsystem. > >>> > >>> Signed-off-by: Marek Vasut > >>> Cc: Simon Glass > >>> Cc: Tom Rini > >> > >> Well, bump ? > >> > >> This is the only missing patch to get my hardware working properly. > > > > I don't think we ever had an agreement on the v1 patch. Simon had a > > long email that pointed out what Linux does seems like a 'fallback' to > > find a node with no compatible string. > > > > Back to this, if we have to go with this way, please create a test > > case to cover this scenario. > > The fact that it works on a particular board is not tested enough? > Do we need a custom, special, synthetic test ? > > >>> > >>> I believe that's always been the requirement against the DM code > >>> changes. I was requested in the past when I changed something in the > >>> DM and I see other people were asked to do so. Like Alex said, it does > >>> not mean this patch was not tested enough, but to ensure future > >>> commits won't break this. > >> > >> So, do you have any suggestion how to implement this test ? It seems > >> Alex posed the same question. It doesn't seem to be trivial in the > >> context of sandbox. > > > > I suppose you need a PCI_DEVICE() declaration for sandbox, with an > > associated DT node and no compatible string. Then check that you can > > locate the device and that it read a DT property correctly. > > Is there any example of this stuff already ? See the bottom of swap_case.c. You might be able to add a new one of those, If you look at pci-controller2 in test.dts it has a device with a compatible string. You could try adding a second device with no compatible string. > > Anyway, any feedback on the patch ? Did you test it ? I again only see > "do this random stuff and that random stuff" , but zero actual feedback. > > >>> > >>> If "this and that random stuff" means test case I asked for, please > >>> check my proposal on the v1 patch thread which indicated that a proper > >>> test case should be created. You seems to have missed that. > >> > >> So, any feedback on this actual patch ? > > > > What is 'potention'? > > potential typo . > > > Is there any check needed that it does not attach the same DT node to > > two different devices? Or perhaps that cannot happen, since we > > shouldn't expect two nodes to share a BDF? > > I guess it could happen and I didn't find a good solution to this even > in Linux. The current take on this possibility seems to be "let's live > with it". OK. > > > I think it looks OK, assuming this is the way we want to go. > > > > Regards, > > Simon > > > > > -- > Best regards, > Marek Vasut Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH V2 2/2] pci: Update documentation to make 'compatible' string optional
Hi, On 1 September 2018 at 16:41, Marek Vasut wrote: > On 09/01/2018 11:45 PM, Simon Glass wrote: >> Hi Marek, >> >> On 30 August 2018 at 04:20, Marek Vasut wrote: >>> On 08/30/2018 02:29 AM, Simon Glass wrote: Hi Marek, >>> >>> Hi, >>> On 24 August 2018 at 12:27, Marek Vasut wrote: > Reword the documentation to make it clear the compatible string is now > optional, yet still matching on it takes precedence over PCI IDs and > PCI classes. > > Signed-off-by: Marek Vasut > Cc: Simon Glass > Cc: Tom Rini > --- > V2: New patch > --- > doc/driver-model/pci-info.txt | 14 +- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/doc/driver-model/pci-info.txt b/doc/driver-model/pci-info.txt > index e1701d1fbc..14364c5c75 100644 > --- a/doc/driver-model/pci-info.txt > +++ b/doc/driver-model/pci-info.txt > @@ -34,11 +34,15 @@ under that bus. > Note that this is all done on a lazy basis, as needed, so until > something is > touched on PCI (eg: a call to pci_find_devices()) it will not be probed. > > -PCI devices can appear in the flattened device tree. If they do this > serves to > -specify the driver to use for the device. In this case they will be > bound at > -first. Each PCI device node must have a compatible string list as well > as a > - property, as defined by the IEEE Std 1275-1994 PCI bus binding > document > -v2.1. Note we must describe PCI devices with the same bus hierarchy as > the > +PCI devices can appear in the flattened device tree. If they do, their > node > +often contains extra information which cannot be derived from the PCI > IDs or > +PCI class of the device. Each PCI device node must have a > property, as > +defined by the IEEE Std 1275-1994 PCI bus binding document v2.1. > Compatible > +string list is optional and generally not needed, since PCI is > discoverable > +bus, albeit there are justified exceptions. If the compatible string is > +present, matching on it takes precedence over PCI IDs and PCI classes. > + > +Note we must describe PCI devices with the same bus hierarchy as the > hardware, otherwise driver model cannot detect the correct > parent/children > relationship during PCI bus enumeration thus PCI devices won't be bound > to > their drivers accordingly. A working example like below: > -- > 2.16.2 > Are we really saying that compatible strings are 'generally not needed'? >>> >>> Yes, PCI is a discoverable bus. >>> device tree pci supplement 2.1 talks about some new formats for the compatible string, but doesn't say it is not needed. I much prefer a compatible string since it is easy to find the driver in the source code. >>> >>> But it duplicates (badly) what the PCI IDs and classes are used for >>> since PCI's inception. >>> Can way say that a compatible string is preferred, but in extremis you can avoid it by... >>> >>> No, see above, PCI is discoverable by design. >> >> I feel that these two things are orthogonal. >> >> You can probe the bus and find a device. That is the 'discoverable' part. >> >> But it is not automatically configurable. If it it were, there would >> be no DT node and no settings in the DT for the device. But from your >> patch, in some cases we need more information, and the DT node >> provides that. > > Pretty much, you can have stuff on the PCI card which needs extra info. > >> So to get the settings to pass to the driver, you have to find the >> device-tree node to use for the device. The only way U-Boot supports >> is to use the 'reg' property, which specifies the PCI address. (We >> don't support a compatible string starting with "pci...". We could >> support that, but it is more code for essentially the same purpose.) > > Yes > >> So we are not talking about the discoverability, which is already >> supported by U-Boot. We are talking about the configuration of the >> device, via settings passed to the driver. > > Yes > >> In fact the only issue here is whether to require a compatible string >> for PCI nodes or allow matching solely based on the 'reg' property. Is >> the latter widely used in Linux? Presumably not on x86, which doesn't >> even use DT. > > I only see the compatible string used for bridges, the rest of the > subdevices match on reg property. Where are you looking? Regards, Simon ___ U-Boot 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 V2 1/2] pci: Support parsing PCI controller DT subnodes
On 09/01/2018 11:50 PM, Simon Glass wrote: > Hi Marek, > > On 30 August 2018 at 07:42, Marek Vasut wrote: >> On 08/30/2018 03:32 PM, Bin Meng wrote: >>> Hi Marek, >>> >>> On Thu, Aug 30, 2018 at 1:07 AM Marek Vasut wrote: On 08/29/2018 05:15 PM, Bin Meng wrote: > +Simon > > Hi Marek, > > On Wed, Aug 29, 2018 at 10:22 PM Marek Vasut > wrote: >> >> On 08/24/2018 08:27 PM, Marek Vasut wrote: >>> The PCI controller can have DT subnodes describing extra properties >>> of particular PCI devices, ie. a PHY attached to an EHCI controller >>> on a PCI bus. This patch parses those DT subnodes and assigns a node >>> to the PCI device instance, so that the driver can extract details >>> from that node and ie. configure the PHY using the PHY subsystem. >>> >>> Signed-off-by: Marek Vasut >>> Cc: Simon Glass >>> Cc: Tom Rini >> >> Well, bump ? >> >> This is the only missing patch to get my hardware working properly. > > I don't think we ever had an agreement on the v1 patch. Simon had a > long email that pointed out what Linux does seems like a 'fallback' to > find a node with no compatible string. > > Back to this, if we have to go with this way, please create a test > case to cover this scenario. The fact that it works on a particular board is not tested enough? Do we need a custom, special, synthetic test ? >>> >>> I believe that's always been the requirement against the DM code >>> changes. I was requested in the past when I changed something in the >>> DM and I see other people were asked to do so. Like Alex said, it does >>> not mean this patch was not tested enough, but to ensure future >>> commits won't break this. >> >> So, do you have any suggestion how to implement this test ? It seems >> Alex posed the same question. It doesn't seem to be trivial in the >> context of sandbox. > > I suppose you need a PCI_DEVICE() declaration for sandbox, with an > associated DT node and no compatible string. Then check that you can > locate the device and that it read a DT property correctly. Is there any example of this stuff already ? Anyway, any feedback on the patch ? Did you test it ? I again only see "do this random stuff and that random stuff" , but zero actual feedback. >>> >>> If "this and that random stuff" means test case I asked for, please >>> check my proposal on the v1 patch thread which indicated that a proper >>> test case should be created. You seems to have missed that. >> >> So, any feedback on this actual patch ? > > What is 'potention'? potential typo . > Is there any check needed that it does not attach the same DT node to > two different devices? Or perhaps that cannot happen, since we > shouldn't expect two nodes to share a BDF? I guess it could happen and I didn't find a good solution to this even in Linux. The current take on this possibility seems to be "let's live with it". > I think it looks OK, assuming this is the way we want to go. > > Regards, > Simon > -- Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH V2 2/2] pci: Update documentation to make 'compatible' string optional
On 09/01/2018 11:45 PM, Simon Glass wrote: > Hi Marek, > > On 30 August 2018 at 04:20, Marek Vasut wrote: >> On 08/30/2018 02:29 AM, Simon Glass wrote: >>> Hi Marek, >> >> Hi, >> >>> On 24 August 2018 at 12:27, Marek Vasut wrote: Reword the documentation to make it clear the compatible string is now optional, yet still matching on it takes precedence over PCI IDs and PCI classes. Signed-off-by: Marek Vasut Cc: Simon Glass Cc: Tom Rini --- V2: New patch --- doc/driver-model/pci-info.txt | 14 +- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/doc/driver-model/pci-info.txt b/doc/driver-model/pci-info.txt index e1701d1fbc..14364c5c75 100644 --- a/doc/driver-model/pci-info.txt +++ b/doc/driver-model/pci-info.txt @@ -34,11 +34,15 @@ under that bus. Note that this is all done on a lazy basis, as needed, so until something is touched on PCI (eg: a call to pci_find_devices()) it will not be probed. -PCI devices can appear in the flattened device tree. If they do this serves to -specify the driver to use for the device. In this case they will be bound at -first. Each PCI device node must have a compatible string list as well as a - property, as defined by the IEEE Std 1275-1994 PCI bus binding document -v2.1. Note we must describe PCI devices with the same bus hierarchy as the +PCI devices can appear in the flattened device tree. If they do, their node +often contains extra information which cannot be derived from the PCI IDs or +PCI class of the device. Each PCI device node must have a property, as +defined by the IEEE Std 1275-1994 PCI bus binding document v2.1. Compatible +string list is optional and generally not needed, since PCI is discoverable +bus, albeit there are justified exceptions. If the compatible string is +present, matching on it takes precedence over PCI IDs and PCI classes. + +Note we must describe PCI devices with the same bus hierarchy as the hardware, otherwise driver model cannot detect the correct parent/children relationship during PCI bus enumeration thus PCI devices won't be bound to their drivers accordingly. A working example like below: -- 2.16.2 >>> >>> Are we really saying that compatible strings are 'generally not needed'? >> >> Yes, PCI is a discoverable bus. >> >>> device tree pci supplement 2.1 talks about some new formats for the >>> compatible string, but doesn't say it is not needed. I much prefer a >>> compatible string since it is easy to find the driver in the source >>> code. >> >> But it duplicates (badly) what the PCI IDs and classes are used for >> since PCI's inception. >> >>> Can way say that a compatible string is preferred, but in extremis you >>> can avoid it by... >> >> No, see above, PCI is discoverable by design. > > I feel that these two things are orthogonal. > > You can probe the bus and find a device. That is the 'discoverable' part. > > But it is not automatically configurable. If it it were, there would > be no DT node and no settings in the DT for the device. But from your > patch, in some cases we need more information, and the DT node > provides that. Pretty much, you can have stuff on the PCI card which needs extra info. > So to get the settings to pass to the driver, you have to find the > device-tree node to use for the device. The only way U-Boot supports > is to use the 'reg' property, which specifies the PCI address. (We > don't support a compatible string starting with "pci...". We could > support that, but it is more code for essentially the same purpose.) Yes > So we are not talking about the discoverability, which is already > supported by U-Boot. We are talking about the configuration of the > device, via settings passed to the driver. Yes > In fact the only issue here is whether to require a compatible string > for PCI nodes or allow matching solely based on the 'reg' property. Is > the latter widely used in Linux? Presumably not on x86, which doesn't > even use DT. I only see the compatible string used for bridges, the rest of the subdevices match on reg property. -- Best regards, Marek Vasut ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH V2 1/2] pci: Support parsing PCI controller DT subnodes
Hi Marek, On 30 August 2018 at 07:42, Marek Vasut wrote: > On 08/30/2018 03:32 PM, Bin Meng wrote: >> Hi Marek, >> >> On Thu, Aug 30, 2018 at 1:07 AM Marek Vasut wrote: >>> >>> On 08/29/2018 05:15 PM, Bin Meng wrote: +Simon Hi Marek, On Wed, Aug 29, 2018 at 10:22 PM Marek Vasut wrote: > > On 08/24/2018 08:27 PM, Marek Vasut wrote: >> The PCI controller can have DT subnodes describing extra properties >> of particular PCI devices, ie. a PHY attached to an EHCI controller >> on a PCI bus. This patch parses those DT subnodes and assigns a node >> to the PCI device instance, so that the driver can extract details >> from that node and ie. configure the PHY using the PHY subsystem. >> >> Signed-off-by: Marek Vasut >> Cc: Simon Glass >> Cc: Tom Rini > > Well, bump ? > > This is the only missing patch to get my hardware working properly. I don't think we ever had an agreement on the v1 patch. Simon had a long email that pointed out what Linux does seems like a 'fallback' to find a node with no compatible string. Back to this, if we have to go with this way, please create a test case to cover this scenario. >>> >>> The fact that it works on a particular board is not tested enough? >>> Do we need a custom, special, synthetic test ? >>> >> >> I believe that's always been the requirement against the DM code >> changes. I was requested in the past when I changed something in the >> DM and I see other people were asked to do so. Like Alex said, it does >> not mean this patch was not tested enough, but to ensure future >> commits won't break this. > > So, do you have any suggestion how to implement this test ? It seems > Alex posed the same question. It doesn't seem to be trivial in the > context of sandbox. I suppose you need a PCI_DEVICE() declaration for sandbox, with an associated DT node and no compatible string. Then check that you can locate the device and that it read a DT property correctly. > >>> Anyway, any feedback on the patch ? Did you test it ? I again only see >>> "do this random stuff and that random stuff" , but zero actual feedback. >>> >> >> If "this and that random stuff" means test case I asked for, please >> check my proposal on the v1 patch thread which indicated that a proper >> test case should be created. You seems to have missed that. > > So, any feedback on this actual patch ? What is 'potention'? Is there any check needed that it does not attach the same DT node to two different devices? Or perhaps that cannot happen, since we shouldn't expect two nodes to share a BDF? I think it looks OK, assuming this is the way we want to go. Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 3/8] binman: Add a new "skip-at-start" property in Section class
Hi,
On 30 August 2018 at 23:22, Jagdish Gediya wrote:
> Hi,
>
>> -Original Message-
>> From: s...@google.com On Behalf Of Simon Glass
>> Sent: Thursday, August 30, 2018 8:21 AM
>> To: Jagdish Gediya
>> Cc: U-Boot Mailing List ; Prabhakar Kushwaha
>> ; York Sun ; Poonam
>> Aggrwal ; Bin Meng ;
>> Tom Rini
>> Subject: Re: [PATCH v2 3/8] binman: Add a new "skip-at-start" property in
>> Section class
>>
>> Hi,
>>
>> On 28 August 2018 at 08:49, Jagdish Gediya
>> wrote:
>> >
>> > Currently binman calculates '_skip_at_start' based on 'end-at-4gb'
>> > property and it is used for x86 images.
>> >
>> > For Powerpc mpc85xx based CPU, CONFIG_SYS_TEXT_BASE is the first entry
>> > offset which can be 0xeff4 or 0xfff4 for nor flash boot,
>> > 0x201000 for sd boot etc, so "_skip_at_start" should be set to
>> > CONFIG_SYS_TEXT_BASE.
>> >
>> > 'end-at-4gb' property is not applicable where CONFIG_SYS_TEXT_BASE +
>> > Image size != 4gb.
>> >
>> > Add new property "skip-at-start" in Section class so that
>> > '_skip_at_start' can be calculated either based on "end-at-4gb"
>> > or based on "skip-at-start".
>> >
>> > Signed-off-by: Jagdish Gediya
>> > ---
>> > Changes for v2:
>> > - Renamed 'start-pos' property to 'skip-at-start'
>> > - Updated README
>> >
>> > tools/binman/README | 9 +
>> > tools/binman/bsection.py | 1 +
>> > 2 files changed, 10 insertions(+)
>> >
>>
>> Please add a test for this feature. You will need to add a new numbered dts
>> in tools/binman/tests and test in ftest.py
>>
>> > diff --git a/tools/binman/README b/tools/binman/README index
>> > cb34171..7b4bf2e 100644
>> > --- a/tools/binman/README
>> > +++ b/tools/binman/README
>> > @@ -397,6 +397,15 @@ end-at-4gb:
>> > 8MB ROM, the offset of the first entry would be 0xfff8 with
>> > this option, instead of 0 without this option.
>> >
>> > +skip-at-start:
>> > + This property specifies the first entry offset if not 0.
>> > +
>> > + For Powerpc Book-E architecture, CONFIG_SYS_TEXT_BASE is the first
>> > + entry offset which can be 0xeff4 or 0xfff4 for nor flash
>> > boot,
>> > + 0x201000 for sd boot etc.
>>
>> Can you say 'entry offset of the first entry. It can be ...'. I think it is
>> clearer.
>>
>> > +
>> > + 'end-at-4gb' property is not applicable where CONFIG_SYS_TEXT_BASE
>> +
>> > + Image size != 4gb.
>> >
>> > Examples of the above options can be found in the tests. See the
>> > tools/binman/test directory.
>> > diff --git a/tools/binman/bsection.py b/tools/binman/bsection.py index
>> > a0bd1b6..68997b7 100644
>> > --- a/tools/binman/bsection.py
>> > +++ b/tools/binman/bsection.py
>> > @@ -79,6 +79,7 @@ class Section(object):
>> > self._pad_byte = fdt_util.GetInt(self._node, 'pad-byte', 0)
>> > self._sort = fdt_util.GetBool(self._node, 'sort-by-offset')
>> > self._end_4gb = fdt_util.GetBool(self._node, 'end-at-4gb')
>> > +self._skip_at_start = fdt_util.GetInt(self._node,
>> > + 'skip-at-start', 0)
>>
>> This is a bit pedantic, but...
>>
>> I think this needs to drop the '0' default value. Also in the __init__
>> constructor, set _skip_at_start to None
>>
>> > if self._end_4gb and not self._size:
>> > self._Raise("Section size must be provided when using
>> > end-at-4gb")
>> > if self._end_4gb:
>>
>> ...here you need to check that self._skip_at_start is None, so people don't
>> set
>> both properties. Then set it to 0 if not set and not end_4gb. Something like:
>>
>> if self._end_4gb:
>>if if self._skip_at_start is not None:
>> self.Raise...
>>self._skip_at_start = 0x1 - self._size
>> else:
>>self._skip_at_start = 0
>>
>> Does that make sense? This needs a test too...
> I think it should be checked that self._skip_at_start is None before setting
> it to 0 in else part.
> What's your opinion on below implementation?
>
> self._end_4gb = fdt_util.GetBool(self._node, 'end-at-4gb')
> self._skip_at_start = fdt_util.GetInt(self._node, 'skip-at-start')
> if self._end_4gb:
> if not self._size:
> self._Raise("Section size must be provided when using
> end-at-4gb")
> if self._skip_at_start is not None:
> self._Raise("Provide either 'end-at-4gb' or 'skip-at-start'")
> else:
> self._skip_at_start = 0x1 - self._size
> else:
> if self._skip_at_start is None:
> self._skip_at_start = 0
That looks OK to me. Make sure you change __init__() to set it to None
instead of 0.
Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH V2 2/2] pci: Update documentation to make 'compatible' string optional
Hi Marek, On 30 August 2018 at 04:20, Marek Vasut wrote: > On 08/30/2018 02:29 AM, Simon Glass wrote: >> Hi Marek, > > Hi, > >> On 24 August 2018 at 12:27, Marek Vasut wrote: >>> Reword the documentation to make it clear the compatible string is now >>> optional, yet still matching on it takes precedence over PCI IDs and >>> PCI classes. >>> >>> Signed-off-by: Marek Vasut >>> Cc: Simon Glass >>> Cc: Tom Rini >>> --- >>> V2: New patch >>> --- >>> doc/driver-model/pci-info.txt | 14 +- >>> 1 file changed, 9 insertions(+), 5 deletions(-) >>> >>> diff --git a/doc/driver-model/pci-info.txt b/doc/driver-model/pci-info.txt >>> index e1701d1fbc..14364c5c75 100644 >>> --- a/doc/driver-model/pci-info.txt >>> +++ b/doc/driver-model/pci-info.txt >>> @@ -34,11 +34,15 @@ under that bus. >>> Note that this is all done on a lazy basis, as needed, so until something >>> is >>> touched on PCI (eg: a call to pci_find_devices()) it will not be probed. >>> >>> -PCI devices can appear in the flattened device tree. If they do this >>> serves to >>> -specify the driver to use for the device. In this case they will be bound >>> at >>> -first. Each PCI device node must have a compatible string list as well as a >>> - property, as defined by the IEEE Std 1275-1994 PCI bus binding >>> document >>> -v2.1. Note we must describe PCI devices with the same bus hierarchy as the >>> +PCI devices can appear in the flattened device tree. If they do, their node >>> +often contains extra information which cannot be derived from the PCI IDs >>> or >>> +PCI class of the device. Each PCI device node must have a property, >>> as >>> +defined by the IEEE Std 1275-1994 PCI bus binding document v2.1. Compatible >>> +string list is optional and generally not needed, since PCI is discoverable >>> +bus, albeit there are justified exceptions. If the compatible string is >>> +present, matching on it takes precedence over PCI IDs and PCI classes. >>> + >>> +Note we must describe PCI devices with the same bus hierarchy as the >>> hardware, otherwise driver model cannot detect the correct parent/children >>> relationship during PCI bus enumeration thus PCI devices won't be bound to >>> their drivers accordingly. A working example like below: >>> -- >>> 2.16.2 >>> >> >> Are we really saying that compatible strings are 'generally not needed'? > > Yes, PCI is a discoverable bus. > >> device tree pci supplement 2.1 talks about some new formats for the >> compatible string, but doesn't say it is not needed. I much prefer a >> compatible string since it is easy to find the driver in the source >> code. > > But it duplicates (badly) what the PCI IDs and classes are used for > since PCI's inception. > >> Can way say that a compatible string is preferred, but in extremis you >> can avoid it by... > > No, see above, PCI is discoverable by design. I feel that these two things are orthogonal. You can probe the bus and find a device. That is the 'discoverable' part. But it is not automatically configurable. If it it were, there would be no DT node and no settings in the DT for the device. But from your patch, in some cases we need more information, and the DT node provides that. So to get the settings to pass to the driver, you have to find the device-tree node to use for the device. The only way U-Boot supports is to use the 'reg' property, which specifies the PCI address. (We don't support a compatible string starting with "pci...". We could support that, but it is more code for essentially the same purpose.) So we are not talking about the discoverability, which is already supported by U-Boot. We are talking about the configuration of the device, via settings passed to the driver. In fact the only issue here is whether to require a compatible string for PCI nodes or allow matching solely based on the 'reg' property. Is the latter widely used in Linux? Presumably not on x86, which doesn't even use DT. Regards, Simon ___ U-Boot 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
[U-Boot] [PATCH] mx7dsabresd: Add the qspi target to the list of supported defconfigs
From: Fabio Estevam Add an entry for mx7dsabresd_qspi_defconfig to avoid the following warnings: WARNING: no status info for 'mx7dsabresd_qspi' WARNING: no maintainers for 'mx7dsabresd_qspi' Reported-by: Tom Rini Signed-off-by: Fabio Estevam --- board/freescale/mx7dsabresd/MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/board/freescale/mx7dsabresd/MAINTAINERS b/board/freescale/mx7dsabresd/MAINTAINERS index b96642a..5f805af 100644 --- a/board/freescale/mx7dsabresd/MAINTAINERS +++ b/board/freescale/mx7dsabresd/MAINTAINERS @@ -4,3 +4,4 @@ S: Maintained F: board/freescale/mx7dsabresd F: include/configs/mx7dsabresd.h F: configs/mx7dsabresd_defconfig +F: configs/mx7dsabresd_qspi_defconfig -- 2.7.4 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PULL] Please pull u-boot-imx
On Fri, Aug 31, 2018 at 03:04:31PM +0200, Stefano Babic wrote: > Hi Tom, > > please pull from u-boot-imx, thanks ! > > The following changes since commit 11ed312896c5f5814064c5d45dcb2f53dc121437: > > configs: am57xx: change default board name to beagle_x15 (2018-08-26 > 12:26:16 -0400) > > are available in the Git repository at: > > git://www.denx.de/git/u-boot-imx.git master > > for you to fetch changes up to 2846e663fd62200a189bba357135e284a379a38b: > > imx: missing CONFIG_MII in mx7dsabresd_qspi_defconfig (2018-08-31 > 12:08:43 +0200) > OK, NAK for the following problems: - Fail to build: https://travis-ci.org/trini/u-boot/jobs/423197300 The failing board is ids8313 and it's failing to configure as CONFIG_SYS_BOOTCOUNT_I2C_BUS needs to be set, and isn't - In looking into the above I see configs/ge_bx50v3_defconfig and configs/mx53ppd_defconfig and a few more have a bunch of comments added to it. These will be blown away on the next re-sync. Comments need to go into the board README or something not an auto-generated file. - So I start reading the whole diff and I see: commit 8e00d802e402d2a6734a88ebeb77a70efcfc354c Author: Sébastien Szymanski Date: Wed Jul 25 14:47:53 2018 +0200 ARM: opos6ul: make the board boot again Which is wrong. The -u-boot.dtsi file is automatically included and should have all of the U-Boot specific DTS changes. - And since the above big real problems exist I'm going to point out for you to fix: WARNING: no status info for 'mx7dsabresd_qspi' WARNING: no maintainers for 'mx7dsabresd_qspi' Rather than fix it myself like I was going to before I found the other issues. -- 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 v2 07/13] x86: Fix signed shift overflow in MSR_IA32_APICBASE_BASE
Hi there,
On Tue, Aug 28, 2018 at 08:42:01AM +0200, Eugeniu Rosca wrote:
> Hi Bin,
>
> cc: Masahiro, Andrey
>
> On Tue, Aug 28, 2018 at 10:05:51AM +0800, Bin Meng wrote:
> > Hi Eugeniu,
> >
> > On Mon, Aug 27, 2018 at 7:19 AM Eugeniu Rosca
> > wrote:
> > >
> > > Fix the following UBSAN report:
> > > ==
> > > UBSAN: Undefined behaviour in arch/x86/cpu/lapic.c:73:14
> > > left shift of 1048575 by 12 places cannot be represented in type 'int'
> > > ==
> > >
> > > Steps to reproduce the above:
> > > * echo CONFIG_UBSAN=y >> configs/qemu-x86_defconfig
> > > * make ARCH=x86 qemu-x86_defconfig all
> > > * qemu-system-i386 --version
> > > QEMU emulator version 2.5.0 (Debian 1:2.5+dfsg-5ubuntu10.31)
> > > * qemu-system-i386 --nographic -bios u-boot.rom
> > >
> > > Fixes: 98568f0fa96b ("x86: Import MSR/MTRR code from Linux")
> > > Signed-off-by: Eugeniu Rosca
> > > ---
> > >
> > > Changes in v2:
> > > - None. Newly pushed.
> > > ---
> > > arch/x86/include/asm/msr-index.h | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/arch/x86/include/asm/msr-index.h
> > > b/arch/x86/include/asm/msr-index.h
> > > index 9c1dbe61d596..d8b7b8013c74 100644
> > > --- a/arch/x86/include/asm/msr-index.h
> > > +++ b/arch/x86/include/asm/msr-index.h
> > > @@ -370,7 +370,7 @@
> > > #define MSR_IA32_APICBASE 0x001b
> > > #define MSR_IA32_APICBASE_BSP (1<<8)
> > > #define MSR_IA32_APICBASE_ENABLE (1<<11)
> > > -#define MSR_IA32_APICBASE_BASE (0xf<<12)
> > > +#define MSR_IA32_APICBASE_BASE (0xfUL << 12)
> >
> > I don't understand why such warnings is emitted: "left shift of
> > 1048575 by 12 places cannot be represented in type 'int'"
> >
> > Compilers don't complain this code and Linux kernel has the same
> > definition here.
>
> I wrote a basic kernel module printing the result of "(0xf << 12)"
> and kernel UBSAN doesn't complain indeed.
>
> I started to compare the compiler flags between Linux and U-Boot and
> nailed down empirically that Linux UBSAN warning is inhibited by the
> -fno-strict-overflow gcc option, introduced in Linux commit [1]. The
> latter actually replaces another gcc option -fwrapv, introduced in [2].
>
> Any of the two flags makes the UBSAN error vanish in the kernel.
> Neither of the two flags is used in U-Boot.
>
> I am in the process of browsing some documentation related to -fwrapv
> and -fno-strict-overflow (e.g. [3]). Please, feel free to share any
> thoughts and/or cc anybody who might have dealt with these topics
> in the past. I will come back with more feedback later.
>
> [1] v2.6.31 commit a137802ee839 ("Don't use '-fwrapv' compiler option: it's
> buggy in gcc-4.1.x")
> [2] v2.6.29 commit 68df3755e383 ("Add '-fwrapv' to gcc CFLAGS")
> [3] https://www.airs.com/blog/archives/120
>
> > Regards,
> > Bin
Just wanted to let you know that coreboot folks are going through
similar discussions in [1]. Also, experimenting with various gcc
versions and flags in my spare time, I collected some evidence [2]
showing that the behavior of GCC UBSAN (-fsanitize=undefined &
friends) may differ a lot depending on the gcc version and below
flags (none used by U-Boot, but some used in Linux kernel):
-fwrapv
-fstrict-overflow
-fno-strict-overflow
Checking how -fno-strict-overflow and -fwrapv compare to each other
(since they seem to accomplish similar goals according to many sources),
I've used the sample app from [3] to see how gcc handles signed integer
wraparound depending on gcc version, flags, optimization level and
on whether UBSAN is enabled or not. The variance/inconsistency of the
results [4] is very high in my opinion.
One clear conclusion of [4] is that questions like why gcc UBSAN
complains in U-Boot but not in the Kernel require knowing at least the
parameters tracked in [4] (and maybe more).
[1] https://mail.coreboot.org/pipermail/coreboot/2018-February/086146.html
[2] UBSAN behavior (printing 1 << 31) is highly dependent on gcc version and
flags
+--+-+-+
| gcc flags | gcc version | UB? |
|--|-|-|
| | gcc-4.9.4 | - |
| -fsanitize=undefined | gcc-5.5.0 | y |
| | gcc-7.3.0 | y |
| | gcc-8.1.0 | y |
+--+
| | gcc-4.9.4 | - |
| -fsanitize=undefined | gcc-5.5.0 | y |
| -fstrict-overflow| gcc-7.3.0 | y |
| | gcc-8.1.0 | y |
+--+
| | gcc-4.9.4 | - |
| -fsanitize=undefined | gcc-5.5.0 | y |
| -fno-strict-overflow | gcc-7.3.0 | y |
| | gcc-8.1.0 | - |
+
Re: [U-Boot] [PATCH v3 2/3] efi_loader: ARM: run EFI payloads non-secure
On 31.08.18 20:45, Mark Kettenis wrote: >> From: Heinrich Schuchardt >> Date: Fri, 31 Aug 2018 19:37:25 +0200 >> >> On 06/14/2018 12:41 AM, Mark Kettenis wrote: >>> If desired (and possible) switch into HYP mode or non-secure SVC mode >>> before calling the entry point of an EFI application. This allows >>> U-Boot to provide a usable PSCI implementation and makes it possible >>> to boot kernels into hypervisor mode using an EFI bootloader. >>> >>> Based on diffs from Heinrich Schuchardt and Alexander Graf. >>> >>> Signed-off-by: Mark Kettenis >> >> bootefi hello fails on vexpress_ca15_tc2_defconfig when run on qemu with >> >> QEMU_AUDIO_DRV=none qemu-system-arm \ >> -M vexpress-a15 -cpu cortex-a15 -kernel u-boot \ >> -net user -net nic,model=lan9118 \ >> -m 1024M --nographic \ >> -drive if=sd,file=img.vexpress,media=disk,format=raw > > Works for me with: > > $ qemu-system-arm --version > QEMU emulator version 3.0.0 > Copyright (c) 2003-2017 Fabrice Bellard and the QEMU Project developers > > => bootefi hello > Scanning disks on mmc... > MMC Device 1 not found > MMC Device 2 not found > MMC Device 3 not found > Found 3 disks > WARNING: booting without device tree > ## Starting EFI application at a0008000 ... > WARNING: using memory device/image path, this may confuse some payloads! > Hello, world! > Running on UEFI 2.7 > Have SMBIOS table > Load options: > ## Application terminated, r = 0 > > That is with CONFIG_CMD_BOOTEFI_HELLO=y added to the > vexpress_ca15_tc2_defconfig of course. > >> Bisection points to >> efi_loader: ARM: run EFI payloads non-secure >> commit dc500c369486fbe04000fd325c46bb309e4a1827 > > That suggests an issue with emulation if the mode switching > instructions or HYP support in qemu. Or a toolchain issue of course. Or maybe Heinrich's QEMU version starts up in a different EL mode? Alex ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 13/17] efi_loader: capitalization table
On 01.09.18 11:43, Alexander Graf wrote:
>
>
> On 31.08.18 21:31, Heinrich Schuchardt wrote:
>> This patch provides a define to initialize a table that maps lower to
>> capital letters for Unicode code point 0x - 0x.
>>
>> Signed-off-by: Heinrich Schuchardt
>> ---
>> v2
>> add shorter tables for code pages 437 and 1250
>> ---
>> MAINTAINERS |1 +
>> include/capitalization.h | 2028 ++
>> 2 files changed, 2029 insertions(+)
>> create mode 100644 include/capitalization.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 46f826a0fe..8c9cd83347 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -381,6 +381,7 @@ T: git git://github.com/agraf/u-boot.git
>> F: doc/README.uefi
>> F: doc/README.iscsi
>> F: Documentation/efi.rst
>> +F: include/capitalization.h
>> F: include/efi*
>> F: include/pe.h
>> F: include/asm-generic/pe.h
>> diff --git a/include/capitalization.h b/include/capitalization.h
>> new file mode 100644
>> index 00..2c24e1bf47
>> --- /dev/null
>> +++ b/include/capitalization.h
>> @@ -0,0 +1,2028 @@
>> +/* SPDX-License-Identifier: Unicode-DFS-2016 */
>> +/*
>> + * Capitalization tables
>> + */
>> +
>> +struct capitalization_table {
>> +u16 upper;
>> +u16 lower;
>> +};
>> +
>> +/*
>> + * Correspondence table for small and capital Unicode letters in the range
>> of
>> + * 0x - 0x based on
>> http://www.unicode.org/Public/UCA/11.0.0/allkeys.txt
>> + */
>> +#define UNICODE_CAPITALIZATION_TABLE { \
>> +{ 0x0531, /* ARMENIAN CAPITAL LETTER AYB */ \
>> + 0x0561, /* ARMENIAN SMALL LETTER AYB */ }, \
>
>
> [...]
>
>> +{ 0x2C7F, /* LATIN CAPITAL LETTER Z WITH SWASH TAIL */ \
>> + 0x0240, /* LATIN SMALL LETTER Z WITH SWASH TAIL */ }, \
>> +{ 0x, /* END OF LIST CAPITAL LETTERS */ \
>> + 0x, /* END OF LIST SMALL LETTERS */ }, \
>> +}
>> +
>> +/*
>> + * Correspondence table for small and capital letters of codepage 437.
>> + */
>> +#define CP437_CAPITALIZATION_TABLE { \
>> +{ 0x03a6, 0x03c6, }, \
>
> I like how you added comments to each entry on what exactly the
> character is. Please keep that habit in the trimmed down tables too.
>
>> +{ 0x03a3, 0x03c3, }, \
>> +{ 0x0041, 0x0061, }, \
>> +{ 0x00c4, 0x00e4, }, \
>> +{ 0x00c5, 0x00e5, }, \
>> +{ 0x00c6, 0x00e6, }, \
>> +{ 0x0042, 0x0062, }, \
>> +{ 0x0043, 0x0063, }, \
>> +{ 0x00c7, 0x00e7, }, \
>> +{ 0x0044, 0x0064, }, \
>> +{ 0x0045, 0x0065, }, \
>> +{ 0x00c9, 0x00e9, }, \
>> +{ 0x0046, 0x0066, }, \
>> +{ 0x0047, 0x0067, }, \
>> +{ 0x0048, 0x0068, }, \
>> +{ 0x0049, 0x0069, }, \
>> +{ 0x004a, 0x006a, }, \
>> +{ 0x004b, 0x006b, }, \
>> +{ 0x004c, 0x006c, }, \
>> +{ 0x004d, 0x006d, }, \
>> +{ 0x004e, 0x006e, }, \
>> +{ 0x00d1, 0x00f1, }, \
>> +{ 0x004f, 0x006f, }, \
>> +{ 0x00d6, 0x00f6, }, \
>> +{ 0x0050, 0x0070, }, \
>> +{ 0x0051, 0x0071, }, \
>
> Most of these are just latin A to Z. These are already covered in your
> conversion by code, no? So you can just omit them.
>
>> +{ 0x0052, 0x0072, }, \
>> +{ 0x0053, 0x0073, }, \
>> +{ 0x0054, 0x0074, }, \
>> +{ 0x0055, 0x0075, }, \
>> +{ 0x00dc, 0x00fc, }, \
>> +{ 0x0056, 0x0076, }, \
>> +{ 0x0057, 0x0077, }, \
>> +{ 0x0058, 0x0078, }, \
>> +{ 0x0059, 0x0079, }, \
>> +{ 0x005a, 0x007a, }, \
>> +{ 0x, 0x, }, \
>
> ... that would leave 11 entries for cp437 ...
>
>> +}
>> +
>> +/*
>> + * Correspondence table for small and capital letters of codepage 1250.
>> + */
>> +#define CP1250_CAPITALIZATION_TABLE { \
>> +{ 0x0041, 0x0061, }, \
>
> Please sort the list by code point - or any other recognizable sorting
> order ;).
>
>> +{ 0x00c1, 0x00e1, }, \
>> +{ 0x0102, 0x0103, }, \
>> +{ 0x00c2, 0x00e2, }, \
>> +{ 0x00c4, 0x00e4, }, \
>> +{ 0x0104, 0x0105, }, \
>> +{ 0x0042, 0x0062, }, \
>> +{ 0x0043, 0x0063, }, \
>> +{ 0x0106, 0x0107, }, \
>> +{ 0x010c, 0x010d, }, \
>> +{ 0x00c7, 0x00e7, }, \
>> +{ 0x0044, 0x0064, }, \
>> +{ 0x010e, 0x010f, }, \
>> +{ 0x0110, 0x0111, }, \
>> +{ 0x0045, 0x0065, }, \
>> +{ 0x00c9, 0x00e9, }, \
>> +{ 0x011a, 0x011b, }, \
>> +{ 0x00cb, 0x00eb, }, \
>> +{ 0x0118, 0x0119, }, \
>> +{ 0x0046, 0x0066, }, \
>> +{ 0x0047, 0x0067, }, \
>> +{ 0x0048, 0x0068, }, \
>> +{ 0x0049, 0x0069, }, \
>> +{ 0x00cd, 0x00ed, }, \
>> +{ 0x00ce, 0x00ee, }, \
>> +{ 0x004a, 0x006a, }, \
>> +{ 0x004b, 0x006b, }, \
>> +{ 0x004c, 0x006c, }, \
>> +{ 0x0139, 0x013a, }, \
>> +{ 0x013d, 0x013e, }, \
>> +{ 0x0141, 0x0142, }, \
>> +{ 0x004d, 0x006d, }, \
>> +{ 0x004e, 0x006e, }, \
>> +{ 0x0143, 0x0144, }, \
>> +{ 0x0147, 0x0148, }, \
>> +{ 0x004f, 0x006f, }, \
>> +{ 0x00d3, 0x00f3, }, \
>> +{ 0x00d4, 0x00f4, }, \
>> +{ 0x00d6, 0x00f6, }, \
>> +{ 0x0150, 0x
Re: [U-Boot] [PATCH v2 15/17] test: tests for utf_to_lower() utf_to_upper().
On 31.08.18 21:31, Heinrich Schuchardt wrote:
> Provide unit tests for utf_to_lower() utf_to_upper().
>
> Signed-off-by: Heinrich Schuchardt
> ---
> v2:
> use ut_assert*() for testing
> ---
> test/unicode_ut.c | 30 ++
> 1 file changed, 30 insertions(+)
>
> diff --git a/test/unicode_ut.c b/test/unicode_ut.c
> index d0dbd7985a..3e4231b241 100644
> --- a/test/unicode_ut.c
> +++ b/test/unicode_ut.c
> @@ -500,6 +500,36 @@ int ut_utf16_utf8_strncpy(struct unit_test_state *uts)
> }
> UNICODE_TEST(ut_utf16_utf8_strncpy);
>
> +int ut_utf_to_lower(struct unit_test_state *uts)
> +{
> + ut_asserteq('@', utf_to_lower('@'));
> + ut_asserteq('a', utf_to_lower('A'));
> + ut_asserteq('z', utf_to_lower('Z'));
> + ut_asserteq('[', utf_to_lower('['));
> + ut_asserteq('m', utf_to_lower('m'));
Please make sure you test one of the characters from the shortened list
here too.
Alex
> +#ifdef CONFIG_EFI_UNICODE_CAPITALIZATION
> + /* Cyrillic letter I*/
> + ut_asserteq(0x0438, utf_to_lower(0x0418));
> +#endif
> + return 0;
> +}
> +UNICODE_TEST(ut_utf_to_lower);
> +
> +int ut_utf_to_upper(struct unit_test_state *uts)
> +{
> + ut_asserteq('`', utf_to_upper('`'));
> + ut_asserteq('A', utf_to_upper('a'));
> + ut_asserteq('Z', utf_to_upper('z'));
> + ut_asserteq('{', utf_to_upper('{'));
> + ut_asserteq('M', utf_to_upper('M'));
> +#ifdef CONFIG_EFI_UNICODE_CAPITALIZATION
> + /* Cyrillic letter I */
> + ut_asserteq(0x0418, utf_to_upper(0x0438));
> +#endif
> + return 0;
> +}
> +UNICODE_TEST(ut_utf_to_upper);
> +
> int do_ut_unicode(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> {
> struct unit_test *tests = ll_entry_start(struct unit_test,
> unicode_test);
>
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 13/17] efi_loader: capitalization table
On 31.08.18 21:31, Heinrich Schuchardt wrote:
> This patch provides a define to initialize a table that maps lower to
> capital letters for Unicode code point 0x - 0x.
>
> Signed-off-by: Heinrich Schuchardt
> ---
> v2
> add shorter tables for code pages 437 and 1250
> ---
> MAINTAINERS |1 +
> include/capitalization.h | 2028 ++
> 2 files changed, 2029 insertions(+)
> create mode 100644 include/capitalization.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 46f826a0fe..8c9cd83347 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -381,6 +381,7 @@ T:git git://github.com/agraf/u-boot.git
> F: doc/README.uefi
> F: doc/README.iscsi
> F: Documentation/efi.rst
> +F: include/capitalization.h
> F: include/efi*
> F: include/pe.h
> F: include/asm-generic/pe.h
> diff --git a/include/capitalization.h b/include/capitalization.h
> new file mode 100644
> index 00..2c24e1bf47
> --- /dev/null
> +++ b/include/capitalization.h
> @@ -0,0 +1,2028 @@
> +/* SPDX-License-Identifier: Unicode-DFS-2016 */
> +/*
> + * Capitalization tables
> + */
> +
> +struct capitalization_table {
> + u16 upper;
> + u16 lower;
> +};
> +
> +/*
> + * Correspondence table for small and capital Unicode letters in the range of
> + * 0x - 0x based on
> http://www.unicode.org/Public/UCA/11.0.0/allkeys.txt
> + */
> +#define UNICODE_CAPITALIZATION_TABLE { \
> + { 0x0531, /* ARMENIAN CAPITAL LETTER AYB */ \
> + 0x0561, /* ARMENIAN SMALL LETTER AYB */ }, \
[...]
> + { 0x2C7F, /* LATIN CAPITAL LETTER Z WITH SWASH TAIL */ \
> + 0x0240, /* LATIN SMALL LETTER Z WITH SWASH TAIL */ }, \
> + { 0x, /* END OF LIST CAPITAL LETTERS */ \
> + 0x, /* END OF LIST SMALL LETTERS */ }, \
> +}
> +
> +/*
> + * Correspondence table for small and capital letters of codepage 437.
> + */
> +#define CP437_CAPITALIZATION_TABLE { \
> + { 0x03a6, 0x03c6, }, \
I like how you added comments to each entry on what exactly the
character is. Please keep that habit in the trimmed down tables too.
> + { 0x03a3, 0x03c3, }, \
> + { 0x0041, 0x0061, }, \
> + { 0x00c4, 0x00e4, }, \
> + { 0x00c5, 0x00e5, }, \
> + { 0x00c6, 0x00e6, }, \
> + { 0x0042, 0x0062, }, \
> + { 0x0043, 0x0063, }, \
> + { 0x00c7, 0x00e7, }, \
> + { 0x0044, 0x0064, }, \
> + { 0x0045, 0x0065, }, \
> + { 0x00c9, 0x00e9, }, \
> + { 0x0046, 0x0066, }, \
> + { 0x0047, 0x0067, }, \
> + { 0x0048, 0x0068, }, \
> + { 0x0049, 0x0069, }, \
> + { 0x004a, 0x006a, }, \
> + { 0x004b, 0x006b, }, \
> + { 0x004c, 0x006c, }, \
> + { 0x004d, 0x006d, }, \
> + { 0x004e, 0x006e, }, \
> + { 0x00d1, 0x00f1, }, \
> + { 0x004f, 0x006f, }, \
> + { 0x00d6, 0x00f6, }, \
> + { 0x0050, 0x0070, }, \
> + { 0x0051, 0x0071, }, \
Most of these are just latin A to Z. These are already covered in your
conversion by code, no? So you can just omit them.
> + { 0x0052, 0x0072, }, \
> + { 0x0053, 0x0073, }, \
> + { 0x0054, 0x0074, }, \
> + { 0x0055, 0x0075, }, \
> + { 0x00dc, 0x00fc, }, \
> + { 0x0056, 0x0076, }, \
> + { 0x0057, 0x0077, }, \
> + { 0x0058, 0x0078, }, \
> + { 0x0059, 0x0079, }, \
> + { 0x005a, 0x007a, }, \
> + { 0x, 0x, }, \
... that would leave 11 entries for cp437 ...
> +}
> +
> +/*
> + * Correspondence table for small and capital letters of codepage 1250.
> + */
> +#define CP1250_CAPITALIZATION_TABLE { \
> + { 0x0041, 0x0061, }, \
Please sort the list by code point - or any other recognizable sorting
order ;).
> + { 0x00c1, 0x00e1, }, \
> + { 0x0102, 0x0103, }, \
> + { 0x00c2, 0x00e2, }, \
> + { 0x00c4, 0x00e4, }, \
> + { 0x0104, 0x0105, }, \
> + { 0x0042, 0x0062, }, \
> + { 0x0043, 0x0063, }, \
> + { 0x0106, 0x0107, }, \
> + { 0x010c, 0x010d, }, \
> + { 0x00c7, 0x00e7, }, \
> + { 0x0044, 0x0064, }, \
> + { 0x010e, 0x010f, }, \
> + { 0x0110, 0x0111, }, \
> + { 0x0045, 0x0065, }, \
> + { 0x00c9, 0x00e9, }, \
> + { 0x011a, 0x011b, }, \
> + { 0x00cb, 0x00eb, }, \
> + { 0x0118, 0x0119, }, \
> + { 0x0046, 0x0066, }, \
> + { 0x0047, 0x0067, }, \
> + { 0x0048, 0x0068, }, \
> + { 0x0049, 0x0069, }, \
> + { 0x00cd, 0x00ed, }, \
> + { 0x00ce, 0x00ee, }, \
> + { 0x004a, 0x006a, }, \
> + { 0x004b, 0x006b, }, \
> + { 0x004c, 0x006c, }, \
> + { 0x0139, 0x013a, }, \
> + { 0x013d, 0x013e, }, \
> + { 0x0141, 0x0142, }, \
> + { 0x004d, 0x006d, }, \
> + { 0x004e, 0x006e, }, \
> + { 0x0143, 0x0144, }, \
> + { 0x0147, 0x0148, }, \
> + { 0x004f, 0x006f, }, \
> + { 0x00d3, 0x00f3, }, \
> + { 0x00d4, 0x00f4, }, \
> + { 0x00d6, 0x00f6, }, \
> + { 0x0150, 0x0151, }, \
> + { 0x0050, 0x0070, }, \
> + { 0x0051, 0x0071, }, \
> + { 0x0052, 0x0072, }, \
> + { 0x0154, 0x0155, }, \
>
Re: [U-Boot] [PATCH v2 07/17] lib: vsprintf: correct printing of Unicode strings
On 31.08.18 21:31, Heinrich Schuchardt wrote:
> The width and precision of the printf() function refer to the number of
> characters not to the number of bytes printed.
>
> Signed-off-by: Heinrich Schuchardt
> ---
> v2:
> use library function utf16_utf8_strncpy() for string16()
> ---
> lib/vsprintf.c | 13 -
> 1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 5abf734750..4213441fbf 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -280,18 +280,13 @@ static char *string16(char *buf, char *end, u16 *s, int
> field_width,
> int precision, int flags)
> {
> u16 *str = s ? s : L"";
> - int utf16_len = u16_strnlen(str, precision);
> - u8 utf8[utf16_len * MAX_UTF8_PER_UTF16];
> - int utf8_len, i;
> -
> - utf8_len = utf16_to_utf8(utf8, str, utf16_len) - utf8;
> + ssize_t len = utf16_strnlen(str, precision);
>
> if (!(flags & LEFT))
> - while (utf8_len < field_width--)
> + for (; len < field_width; --field_width)
> ADDCH(buf, ' ');
> - for (i = 0; i < utf8_len; ++i)
> - ADDCH(buf, utf8[i]);
> - while (utf8_len < field_width--)
> + utf16_utf8_strncpy(&buf, str, len);
> + for (; len < field_width; --field_width)
> ADDCH(buf, ' ');
> return buf;
Both the string() and string16() functions seem to completely ignore the
"end" variable. Doesn't that mean we're potentially running over the end
of a buffer with snprintf()?
I think the patch as is is fine, because string() has the same problem
and the previous version didn't bother either. But in the long run, we
should probably fix the end checks.
Alex
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v7 13/13] cmd: mtdparts: describe as legacy
On 31.08.2018 16:57, Miquel Raynal wrote: The 'mtdparts' command is not needed anymore. While the environment variable is still valid (and useful), the command has been replaced by 'mtd' which is much more close to the MTD stack and do not add its own specific glue. The 'mtdids' variable, only used by the 'mtdparts' command is also useless if the right MTD device name is used in the 'mtdparts' variable. Signed-off-by: Miquel Raynal --- cmd/Kconfig | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/cmd/Kconfig b/cmd/Kconfig index 4deec0b238..0786663f4a 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -1673,7 +1673,11 @@ config CMD_MTDPARTS bool "MTD partition support" select MTD_DEVICE if (CMD_NAND || NAND) help - MTD partition support + MTD partitioning tool support. + It is strongly encouraged to avoid using this command + anymore. One can still declare the partitions in the + mtdparts environment variable but better use the MTD stack + and the mtd command instead than this one. config MTDIDS_DEFAULT string "Default MTD IDs" @@ -1681,6 +1685,10 @@ config MTDIDS_DEFAULT help Defines a default MTD IDs list for use with MTD partitions in the Linux MTD command line partitions format. + Declaration of this environment variable is not useful + anymore when using the right MTD names in mtdparts along + with the use of the 'mtd' command instead of the legacy + 'mtdparts'. config MTDPARTS_DEFAULT string "Default MTD partition scheme" Very nice work overall. :) Reviewed-by: Stefan Roese Thanks, Stefan ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v7 12/13] cmd: ubi: clean the partition handling
On 31.08.2018 16:57, Miquel Raynal wrote:
UBI should not mess with MTD partitions, now that the partitions are
handled in a clean way, clean the ubi command and avoid using this
uneeded extra-glue to reference the devices.
Signed-off-by: Miquel Raynal
---
cmd/Kconfig | 2 ++
cmd/ubi.c | 100 +++-
2 files changed, 31 insertions(+), 71 deletions(-)
Nice diffstat. ;)
diff --git a/cmd/Kconfig b/cmd/Kconfig
index 0778d2ecff..4deec0b238 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1815,6 +1815,8 @@ config CMD_UBI
capabilities. Please, consult the MTD web site for more details
(www.linux-mtd.infradead.org). Activate this option if you want
to use U-Boot UBI commands.
+ It is also strongly encouraged to also enable CONFIG_MTD to get full
+ partition support.
config CMD_UBIFS
tristate "Enable UBIFS - Unsorted block images filesystem commands"
diff --git a/cmd/ubi.c b/cmd/ubi.c
index 0a3405a3b1..af0cea2ca5 100644
--- a/cmd/ubi.c
+++ b/cmd/ubi.c
@@ -15,6 +15,9 @@
#include
#include
#include
+#if defined(CONFIG_MTD)
+#include
+#endif
#include
#include
#include
@@ -29,17 +32,6 @@
/* Private own data */
static struct ubi_device *ubi;
-static char buffer[80];
-static int ubi_initialized;
-
-struct selected_dev {
- char part_name[80];
- int selected;
- int nr;
- struct mtd_info *mtd_info;
-};
-
-static struct selected_dev ubi_dev;
#ifdef CONFIG_CMD_UBIFS
int ubifs_is_mounted(void);
@@ -404,43 +396,24 @@ int ubi_volume_read(char *volume, char *buf, size_t size)
return err;
}
-static int ubi_dev_scan(struct mtd_info *info, char *ubidev,
- const char *vid_header_offset)
+static int ubi_dev_scan(struct mtd_info *info, const char *vid_header_offset)
{
- struct mtd_device *dev;
- struct part_info *part;
- struct mtd_partition mtd_part;
char ubi_mtd_param_buffer[80];
- u8 pnum;
int err;
- if (find_dev_and_part(ubidev, &dev, &pnum, &part) != 0)
- return 1;
+ if (!vid_header_offset)
+ sprintf(ubi_mtd_param_buffer, "%s", info->name);
+ else
+ sprintf(ubi_mtd_param_buffer, "%s,%s", info->name,
+ vid_header_offset);
- sprintf(buffer, "mtd=%d", pnum);
- memset(&mtd_part, 0, sizeof(mtd_part));
- mtd_part.name = buffer;
- mtd_part.size = part->size;
- mtd_part.offset = part->offset;
- add_mtd_partitions(info, &mtd_part, 1);
-
- strcpy(ubi_mtd_param_buffer, buffer);
- if (vid_header_offset)
- sprintf(ubi_mtd_param_buffer, "mtd=%d,%s", pnum,
- vid_header_offset);
err = ubi_mtd_param_parse(ubi_mtd_param_buffer, NULL);
- if (err) {
- del_mtd_partitions(info);
+ if (err)
return -err;
- }
err = ubi_init();
- if (err) {
- del_mtd_partitions(info);
+ if (err)
return -err;
- }
-
- ubi_initialized = 1;
return 0;
}
@@ -465,50 +438,35 @@ int ubi_detach(void)
/*
* Call ubi_exit() before re-initializing the UBI subsystem
*/
- if (ubi_initialized) {
+ if (ubi)
ubi_exit();
- del_mtd_partitions(ubi_dev.mtd_info);
- ubi_initialized = 0;
- }
- ubi_dev.selected = 0;
+ ubi = NULL;
+
return 0;
}
int ubi_part(char *part_name, const char *vid_header_offset)
{
+ struct mtd_info *mtd;
int err = 0;
- char mtd_dev[16];
- struct mtd_device *dev;
- struct part_info *part;
- u8 pnum;
ubi_detach();
- /*
-* Search the mtd device number where this partition
-* is located
-*/
- if (find_dev_and_part(part_name, &dev, &pnum, &part)) {
+
+#ifdef CONFIG_MTD
+ mtd_probe_devices();
+#endif
+ mtd = get_mtd_device_nm(part_name);
+ if (IS_ERR(mtd)) {
printf("Partition %s not found!\n", part_name);
return 1;
}
- sprintf(mtd_dev, "%s%d", MTD_DEV_TYPE(dev->id->type), dev->id->num);
- ubi_dev.mtd_info = get_mtd_device_nm(mtd_dev);
- if (IS_ERR(ubi_dev.mtd_info)) {
- printf("Partition %s not found on device %s!\n", part_name,
- mtd_dev);
- return 1;
- }
+ put_mtd_device(mtd);
- ubi_dev.selected = 1;
-
- strcpy(ubi_dev.part_name, part_name);
- err = ubi_dev_scan(ubi_dev.mtd_info, ubi_dev.part_name,
- vid_header_offset);
+ err = ubi_dev_scan(mtd, vid_header_offset);
if (err) {
printf("UBI init error %d\n", err);
printf("Please check, if the correct MTD partition is used (size big
enough?)\n");
- ubi_dev.selected = 0;
Re: [U-Boot] [PATCH v7 11/13] cmd: mtdparts: try to probe the MTD devices as a fallback
On 31.08.2018 16:57, Miquel Raynal wrote:
Current implementation of mtdparts command errors out if the desired MTD
device is not found. Fallback to the new probe function in this case
before erroring out.
This will the save the user the need to call something like 'mtd list'
before mtdparts.
Signed-off-by: Miquel Raynal
Acked-by: Jagan Teki
---
cmd/mtdparts.c | 16 +---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/cmd/mtdparts.c b/cmd/mtdparts.c
index f7ed1a0779..a1102c3fc5 100644
--- a/cmd/mtdparts.c
+++ b/cmd/mtdparts.c
@@ -79,6 +79,10 @@
#include
#include
+#if defined(CONFIG_MTD)
+#include
+#endif
+
#if defined(CONFIG_CMD_NAND)
#include
#include
@@ -307,9 +311,15 @@ static int get_mtd_info(u8 type, u8 num, struct mtd_info
**mtd)
sprintf(mtd_dev, "%s%d", MTD_DEV_TYPE(type), num);
*mtd = get_mtd_device_nm(mtd_dev);
- if (IS_ERR(*mtd)) {
- printf("Device %s not found!\n", mtd_dev);
- return 1;
+ if (IS_ERR_OR_NULL(*mtd)) {
+#ifdef CONFIG_MTD
+ mtd_probe_devices();
+ *mtd = get_mtd_device_nm(mtd_dev);
+#endif
+ if (IS_ERR_OR_NULL(*mtd)) {
+ printf("Device %s not found!\n", mtd_dev);
+ return 1;
+ }
}
put_mtd_device(*mtd);
This is most likely the use case for CMD_MTD without MTD, correct?
Reviewed-by: Stefan Roese
Thanks,
Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v7 10/13] cmd: mtd: add 'mtd' command
On 31.08.2018 16:57, Miquel Raynal wrote:
There should not be a 'nand' command, a 'sf' command and certainly not
a new 'spi-nand' command. Write a 'mtd' command instead to manage all
MTD devices/partitions at once. This should be the preferred way to
access any MTD device.
Signed-off-by: Miquel Raynal
Acked-by: Jagan Teki
---
cmd/Kconfig | 10 +-
cmd/Makefile | 1 +
cmd/mtd.c| 517 +++
drivers/mtd/Makefile | 2 +-
include/mtd.h| 1 +
5 files changed, 528 insertions(+), 3 deletions(-)
create mode 100644 cmd/mtd.c
diff --git a/cmd/Kconfig b/cmd/Kconfig
index ef43ed8dda..0778d2ecff 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -847,6 +847,12 @@ config CMD_MMC_SWRITE
Enable support for the "mmc swrite" command to write Android sparse
images to eMMC.
+config CMD_MTD
+ bool "mtd"
+ select MTD_PARTITIONS
+ help
+ MTD commands support.
+
config CMD_NAND
bool "nand"
default y if NAND_SUNXI
@@ -1671,14 +1677,14 @@ config CMD_MTDPARTS
config MTDIDS_DEFAULT
string "Default MTD IDs"
- depends on CMD_MTDPARTS || CMD_NAND || CMD_FLASH
+ depends on CMD_MTD || CMD_MTDPARTS || CMD_NAND || CMD_FLASH
help
Defines a default MTD IDs list for use with MTD partitions in the
Linux MTD command line partitions format.
config MTDPARTS_DEFAULT
string "Default MTD partition scheme"
- depends on CMD_MTDPARTS || CMD_NAND || CMD_FLASH
+ depends on CMD_MTD || CMD_MTDPARTS || CMD_NAND || CMD_FLASH
help
Defines a default MTD partitioning scheme in the Linux MTD command
line partitions format
diff --git a/cmd/Makefile b/cmd/Makefile
index 323f1fd2c7..32fd102189 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -90,6 +90,7 @@ obj-$(CONFIG_CMD_MISC) += misc.o
obj-$(CONFIG_CMD_MMC) += mmc.o
obj-$(CONFIG_CMD_MMC_SPI) += mmc_spi.o
obj-$(CONFIG_MP) += mp.o
+obj-$(CONFIG_CMD_MTD) += mtd.o
obj-$(CONFIG_CMD_MTDPARTS) += mtdparts.o
obj-$(CONFIG_CMD_NAND) += nand.o
obj-$(CONFIG_CMD_NET) += net.o
diff --git a/cmd/mtd.c b/cmd/mtd.c
new file mode 100644
index 00..32295fe86c
--- /dev/null
+++ b/cmd/mtd.c
@@ -0,0 +1,517 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * mtd.c
+ *
+ * Generic command to handle basic operations on any memory device.
+ *
+ * Copyright: Bootlin, 2018
+ * Author: Miquèl Raynal
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define MTD_NAME_MAX_LEN 20
+
+static char *old_mtdparts;
+
+static void mtd_dump_buf(u8 *buf, uint len, uint offset)
+{
+ int i, j;
+
+ for (i = 0; i < len; ) {
+ printf("0x%08x:\t", offset + i);
+ for (j = 0; j < 8; j++)
+ printf("%02x ", buf[i + j]);
+ printf(" ");
+ i += 8;
+ for (j = 0; j < 8; j++)
+ printf("%02x ", buf[i + j]);
+ printf("\n");
+ i += 8;
+ }
+}
+
+static void mtd_show_parts(struct mtd_info *mtd, int level)
+{
+ struct mtd_info *part;
+ int i;
+
+ if (list_empty(&mtd->partitions))
+ return;
+
+ list_for_each_entry(part, &mtd->partitions, node) {
+ for (i = 0; i < level; i++)
+ printf("\t");
+ printf("* %s\n", part->name);
+ for (i = 0; i < level; i++)
+ printf("\t");
+ printf(" > Offset: 0x%llx bytes\n", part->offset);
+ for (i = 0; i < level; i++)
+ printf("\t");
+ printf(" > Size: 0x%llx bytes\n", part->size);
+
+ mtd_show_parts(part, level + 1);
+ }
+}
+
+static void mtd_show_device(struct mtd_info *mtd)
+{
+ /* Device */
+ printf("* %s", mtd->name);
+ if (mtd->dev)
+ printf(" [device: %s] [parent: %s] [driver: %s]",
+ mtd->dev->name, mtd->dev->parent->name,
+ mtd->dev->driver->name);
+ printf("\n");
+
+ /* MTD device information */
+ printf(" > type: ");
+ switch (mtd->type) {
+ case MTD_RAM:
+ printf("RAM\n");
+ break;
+ case MTD_ROM:
+ printf("ROM\n");
+ break;
+ case MTD_NORFLASH:
+ printf("NOR flash\n");
+ break;
+ case MTD_NANDFLASH:
+ printf("NAND flash\n");
+ break;
+ case MTD_DATAFLASH:
+ printf("Data flash\n");
+ break;
+ case MTD_UBIVOLUME:
+ printf("UBI volume\n");
+ break;
+ case MTD_MLCNANDFLASH:
+ printf("MLC NAND flash\n");
+ break;
+ case MTD_ABSENT:
+ default:
+ printf("Unknown\n");
+
Re: [U-Boot] [PATCH v7 09/13] mtd: mtdpart: implement proper partition handling
On 31.08.2018 16:57, Miquel Raynal wrote:
Instead of collecting partitions in a flat list, create a hierarchy
within the mtd_info structure: use a partitions list to keep track of
the partitions of an MTD device (which might be itself a partition of
another MTD device), a pointer to the parent device (NULL when the MTD
device is the root one, not a partition).
By also saving directly in mtd_info the offset of the partition, we
can get rid of the mtd_part structure.
Signed-off-by: Miquel Raynal
---
drivers/mtd/mtdcore.c | 2 +
drivers/mtd/mtdpart.c | 412 ++---
include/linux/mtd/mtd.h| 31 +++
include/linux/mtd/partitions.h | 1 -
4 files changed, 208 insertions(+), 238 deletions(-)
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index fb1d68d5e2..fb6c779abb 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -426,6 +426,8 @@ int add_mtd_device(struct mtd_info *mtd)
mtd->index = i;
mtd->usecount = 0;
+ INIT_LIST_HEAD(&mtd->partitions);
+
/* default value if not set by driver */
if (mtd->bitflip_threshold == 0)
mtd->bitflip_threshold = mtd->ecc_strength;
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 096351e48b..afc02dcb24 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -29,29 +29,12 @@
#include "mtdcore.h"
-/* Our partition linked list */
-static LIST_HEAD(mtd_partitions);
#ifndef __UBOOT__
static DEFINE_MUTEX(mtd_partitions_mutex);
#else
DEFINE_MUTEX(mtd_partitions_mutex);
#endif
-/* Our partition node structure */
-struct mtd_part {
- struct mtd_info mtd;
- struct mtd_info *master;
- uint64_t offset;
- struct list_head list;
-};
-
-/*
- * Given a pointer to the MTD object in the mtd_part structure, we can retrieve
- * the pointer to that structure with this macro.
- */
-#define PART(x) ((struct mtd_part *)(x))
-
-
#ifdef __UBOOT__
/* from mm/util.c */
@@ -333,19 +316,18 @@ int mtd_search_alternate_name(const char *mtdname, char *altname,
static int part_read(struct mtd_info *mtd, loff_t from, size_t len,
size_t *retlen, u_char *buf)
{
- struct mtd_part *part = PART(mtd);
struct mtd_ecc_stats stats;
int res;
- stats = part->master->ecc_stats;
- res = part->master->_read(part->master, from + part->offset, len,
- retlen, buf);
+ stats = mtd->parent->ecc_stats;
+ res = mtd->parent->_read(mtd->parent, from + mtd->offset, len,
+retlen, buf);
if (unlikely(mtd_is_eccerr(res)))
mtd->ecc_stats.failed +=
- part->master->ecc_stats.failed - stats.failed;
+ mtd->parent->ecc_stats.failed - stats.failed;
else
mtd->ecc_stats.corrected +=
- part->master->ecc_stats.corrected - stats.corrected;
+ mtd->parent->ecc_stats.corrected - stats.corrected;
return res;
}
@@ -353,17 +335,13 @@ static int part_read(struct mtd_info *mtd, loff_t from, size_t len,
static int part_point(struct mtd_info *mtd, loff_t from, size_t len,
size_t *retlen, void **virt, resource_size_t *phys)
{
- struct mtd_part *part = PART(mtd);
-
- return part->master->_point(part->master, from + part->offset, len,
- retlen, virt, phys);
+ return mtd->parent->_point(mtd->parent, from + mtd->offset, len,
+ retlen, virt, phys);
}
static int part_unpoint(struct mtd_info *mtd, loff_t from, size_t len)
{
- struct mtd_part *part = PART(mtd);
-
- return part->master->_unpoint(part->master, from + part->offset, len);
+ return mtd->parent->_unpoint(mtd->parent, from + mtd->offset, len);
}
#endif
@@ -372,17 +350,13 @@ static unsigned long part_get_unmapped_area(struct mtd_info *mtd,
unsigned long offset,
unsigned long flags)
{
- struct mtd_part *part = PART(mtd);
-
- offset += part->offset;
- return part->master->_get_unmapped_area(part->master, len, offset,
- flags);
+ offset += mtd->offset;
+ return mtd->parent->_get_unmapped_area(mtd->parent, len, offset, flags);
}
static int part_read_oob(struct mtd_info *mtd, loff_t from,
struct mtd_oob_ops *ops)
{
- struct mtd_part *part = PART(mtd);
int res;
if (from >= mtd->size)
@@ -407,7 +381,7 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from,
return -EINVAL;
}
- res = part->master->_read_oob(part->master, from + part->offset, ops);
+ res = mtd->parent->_read_oob(mtd->parent, from + mtd->offset, ops);
if (unlikel
Re: [U-Boot] [PATCH v7 08/13] mtd: uclass: search for an equivalent MTD name with the mtdids
On 31.08.2018 16:57, Miquel Raynal wrote:
Using an MTD device (resp. partition) name in mtdparts is simple and
straightforward. However, for a long time already, another name was
given in mtdparts to indicate a device (resp. partition) so the
"mtdids" environment variable was created to do the match.
Let's create a function that, from an MTD device (resp. partition)
name, search for the equivalent name in the "mtdparts" environment
variable thanks to the "mtdids" string.
Signed-off-by: Miquel Raynal
---
drivers/mtd/mtdpart.c | 62 ++
include/linux/mtd/partitions.h | 2 ++
2 files changed, 64 insertions(+)
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index d9708da2ae..096351e48b 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -263,6 +263,68 @@ int mtd_parse_partitions(struct mtd_info *parent, const
char **_mtdparts,
return 0;
}
+/**
+ * mtd_search_alternate_name - Search an alternate name for @mtdname thanks to
+ * the mtdids legacy environment variable.
+ *
+ * The mtdids string is a list of comma-separated 'dev_id=mtd_id' tupples.
+ * Check if one of the mtd_id matches mtdname, in this case save dev_id in
+ * altname.
+ *
+ * @mtdname: Current MTD device name
+ * @altname: Alternate name to return
+ * @max_len: Length of the alternate name buffer
+ *
+ * @return 0 on success, an error otherwise.
+ */
+int mtd_search_alternate_name(const char *mtdname, char *altname,
+ unsigned int max_len)
+{
+ const char *mtdids, *equal, *comma, *dev_id, *mtd_id;
+ int dev_id_len, mtd_id_len;
+
+ mtdids = env_get("mtdids");
+ if (!mtdids)
+ return -EINVAL;
+
+ do {
+ /* Find the '=' sign */
+ dev_id = mtdids;
+ equal = strchr(dev_id, '=');
+ if (!equal)
+ break;
+
+ dev_id_len = equal - mtdids;
+ mtd_id = equal + 1;
+
+ /* Find the end of the tupple */
+ comma = strchr(mtdids, ',');
+ if (comma)
+ mtd_id_len = comma - equal;
+ else
+ mtd_id_len = &mtdids[strlen(mtdids)] - equal;
+
+ if (!dev_id_len || !mtd_id_len)
+ return -EINVAL;
+
+ if (dev_id_len + 1 > max_len)
+ continue;
+
+ /* Compare the name we search with the current mtd_id */
+ if (!strncmp(mtdname, mtd_id, mtd_id_len)) {
+ strncpy(altname, dev_id, dev_id_len);
+ altname[dev_id_len] = 0;
+
+ return 0;
+ }
+
+ /* Go to the next tupple */
+ mtdids = comma + 1;
+ } while (comma);
+
+ return -EINVAL;
+}
+
/*
* MTD methods which simply translate the effective address and pass through
* to the _real_ device.
diff --git a/include/linux/mtd/partitions.h b/include/linux/mtd/partitions.h
index ed4ece5e13..082a4966ea 100644
--- a/include/linux/mtd/partitions.h
+++ b/include/linux/mtd/partitions.h
@@ -88,5 +88,7 @@ int mtd_del_partition(struct mtd_info *master, int partno);
uint64_t mtd_get_device_size(const struct mtd_info *mtd);
int mtd_parse_partitions(struct mtd_info *parent, const char **_mtdparts,
struct mtd_partition **_parts, int *_nb_parts);
+int mtd_search_alternate_name(const char *mtdname, char *altname,
+ unsigned int max_len);
#endif
Reviewed-by: Stefan Roese
Thanks,
Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v7 07/13] mtd: uclass: add a generic 'mtdparts' parser
On 31.08.2018 16:57, Miquel Raynal wrote:
The current parser is very specific to U-Boot mtdparts implementation.
It does not use MTD structures like mtd_info and mtd_partition. Copy
and adapt the current parser in drivers/mtd/mtd-uclass.c (to not break
the current use of mtdparts.c itself) and write some kind of a wrapper
around the current implementation to allow other commands to benefit
from this parsing in a user-friendly way.
This new function will allocate an mtd_partition array for each
successful call. This array must be freed after use by the caller.
The given 'mtdparts' buffer pointer will be moved forward to the next
MTD device (if any, it will point towards a '\0' character otherwise).
Signed-off-by: Miquel Raynal
Acked-by: Jagan Teki
---
drivers/mtd/mtdpart.c | 187 +
include/linux/mtd/partitions.h | 2 +
2 files changed, 189 insertions(+)
diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
index 9ccb1b3361..d9708da2ae 100644
--- a/drivers/mtd/mtdpart.c
+++ b/drivers/mtd/mtdpart.c
@@ -76,6 +76,193 @@ char *kstrdup(const char *s, gfp_t gfp)
}
#endif
+#define MTD_SIZE_REMAINING (~0LLU)
+#define MTD_OFFSET_NOT_SPECIFIED (~0LLU)
+
+/**
+ * mtd_parse_partition - Parse @mtdparts partition definition, fill @partition
+ * with it and update the @mtdparts string pointer.
+ *
+ * The partition name is allocated and must be freed by the caller.
+ *
+ * This function is widely inspired from part_parse (mtdparts.c).
+ *
+ * @mtdparts: String describing the partition with mtdparts command syntax
+ * @partition: MTD partition structure to fill
+ *
+ * @return 0 on success, an error otherwise.
+ */
+static int mtd_parse_partition(const char **_mtdparts,
+ struct mtd_partition *partition)
+{
+ const char *mtdparts = *_mtdparts;
+ const char *name = NULL;
+ int name_len;
+ char *buf;
+
+ /* Ensure the partition structure is empty */
+ memset(partition, 0, sizeof(struct mtd_partition));
+
+ /* Fetch the partition size */
+ if (*mtdparts == '-') {
+ /* Assign all remaining space to this partition */
+ partition->size = MTD_SIZE_REMAINING;
+ mtdparts++;
+ } else {
+ partition->size = ustrtoull(mtdparts, (char **)&mtdparts, 0);
+ if (partition->size < SZ_4K) {
+ printf("Minimum allowed partition size is 4kiB\n");
+ return -EINVAL;
+ }
+ }
+
+ /* Check for the offset */
+ partition->offset = MTD_OFFSET_NOT_SPECIFIED;
+ if (*mtdparts == '@') {
+ mtdparts++;
+ partition->offset = ustrtoull(mtdparts, (char **)&mtdparts, 0);
+ }
+
+ /* Now look for the name */
+ if (*mtdparts == '(') {
+ name = ++mtdparts;
+ if ((mtdparts = strchr(name, ')')) == NULL) {
+ printf("No closing ')' found in partition name\n");
+ return -EINVAL;
+ }
+ name_len = mtdparts - name + 1;
+ if ((name_len - 1) == 0) {
+ printf("Empty partition name\n");
+ return -EINVAL;
+ }
+ mtdparts++;
+ } else {
+ /* Name will be of the form size@offset */
+ name_len = 22;
+ }
+
+ /* Check if the partition is read-only */
+ if (strncmp(mtdparts, "ro", 2) == 0) {
+ partition->mask_flags |= MTD_WRITEABLE;
+ mtdparts += 2;
+ }
+
+ /* Check for a potential next partition definition */
+ if (*mtdparts == ',') {
+ if (partition->size == MTD_SIZE_REMAINING) {
+ printf("No partitions allowed after a fill-up
partition\n");
+ return -EINVAL;
+ }
+ ++mtdparts;
+ } else if (*mtdparts == ';') {
+ ++mtdparts;
+ } else if (*mtdparts == '\0') {
+ /* NOP */
+ } else {
+ printf("Unexpected character '%c' in mtdparts\n", *mtdparts);
+ return -EINVAL;
+ }
+
+ /*
+* Allocate a buffer for the name and either copy the provided name or
+* auto-generate it with the form 'size@offset'.
+*/
+ buf = malloc(name_len);
+ if (!buf)
+ return -ENOMEM;
+
+ if (name)
+ strncpy(buf, name, name_len - 1);
+ else
+ snprintf(buf, name_len, "0x%08llx@0x%08llx",
+partition->size, partition->offset);
+
+ buf[name_len - 1] = '\0';
+ partition->name = buf;
+
+ *_mtdparts = mtdparts;
+
+ return 0;
+}
+
+/**
+ * mtdparts_parse_part - Create a partition array from an mtdparts definition
+ *
+ * Stateless function that takes a @parent MTD device, a strin
Re: [U-Boot] [PATCH v7 06/13] mtd: uclass: add probe function
On 31.08.2018 16:57, Miquel Raynal wrote:
The user might want to trigger the probe of any MTD device, export these
functions so they can be called from a command source file.
Signed-off-by: Miquel Raynal
Acked-by: Jagan Teki
---
drivers/mtd/mtd-uclass.c | 16
include/mtd.h| 2 ++
2 files changed, 18 insertions(+)
diff --git a/drivers/mtd/mtd-uclass.c b/drivers/mtd/mtd-uclass.c
index 9ca049c437..5418217431 100644
--- a/drivers/mtd/mtd-uclass.c
+++ b/drivers/mtd/mtd-uclass.c
@@ -5,9 +5,25 @@
#include
#include
+#include
#include
#include
+/**
+ * mtd_probe - Probe the device @dev if not already done
+ *
+ * @dev: U-Boot device to probe
+ *
+ * @return 0 on success, an error otherwise.
+ */
+int mtd_probe(struct udevice *dev)
+{
+ if (device_active(dev))
+ return 0;
+
+ return device_probe(dev);
+}
+
/*
* Implement a MTD uclass which should include most flash drivers.
* The uclass private is pointed to mtd_info.
diff --git a/include/mtd.h b/include/mtd.h
index 548e7f191b..6e6da3002f 100644
--- a/include/mtd.h
+++ b/include/mtd.h
@@ -19,4 +19,6 @@ static inline struct mtd_info *mtd_get_info(struct udevice
*dev)
return dev_get_uclass_priv(dev);
}
+int mtd_probe(struct udevice *dev);
+
#endif/* _MTD_H_ */
Reviewed-by: Stefan Roese
Thanks,
Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v7 05/13] cmd: mtdparts: remove mandatory 'mtdparts=' prefix
On 31.08.2018 16:57, Miquel Raynal wrote:
All U-Boot users must define the mtdparts environment variable with:
setenv mtdparts mtdparts=...
While this may ease the partition declaration job to be passed to
Linux, this is a pure software limitation and forcing this prefix is a
complete non-sense. Let the user to declare manually the mtdparts
variable without the prefix.
Signed-off-by: Miquel Raynal
Acked-by: Jagan Teki
---
cmd/mtdparts.c | 17 ++---
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/cmd/mtdparts.c b/cmd/mtdparts.c
index 2e547894c6..f7ed1a0779 100644
--- a/cmd/mtdparts.c
+++ b/cmd/mtdparts.c
@@ -44,7 +44,7 @@
*
* 'mtdparts' - partition list
*
- * mtdparts=mtdparts=[;...]
+ * mtdparts=[mtdparts=][;...]
*
* := :[,...]
*:= unique device tag used by linux kernel to find mtd device
(mtd->name)
@@ -62,11 +62,11 @@
*
* 1 NOR Flash, with 1 single writable partition:
* mtdids=nor0=edb7312-nor
- * mtdparts=mtdparts=edb7312-nor:-
+ * mtdparts=[mtdparts=]edb7312-nor:-
*
* 1 NOR Flash with 2 partitions, 1 NAND with one
* mtdids=nor0=edb7312-nor,nand0=edb7312-nand
- * mtdparts=mtdparts=edb7312-nor:256k(ARMboot)ro,-(root);edb7312-nand:-(home)
+ * mtdparts=[mtdparts=]edb7312-nor:256k(ARMboot)ro,-(root);edb7312-nand:-(home)
*
*/
@@ -1099,9 +1099,6 @@ static int generate_mtdparts(char *buf, u32 buflen)
return 0;
}
- strcpy(p, "mtdparts=");
- p += 9;
-
list_for_each(dentry, &devices) {
dev = list_entry(dentry, struct mtd_device, link);
@@ -1572,11 +1569,9 @@ static int parse_mtdparts(const char *const mtdparts)
if (!p)
p = mtdparts;
- if (strncmp(p, "mtdparts=", 9) != 0) {
- printf("mtdparts variable doesn't start with 'mtdparts='\n");
- return err;
- }
- p += 9;
+ /* Skip the useless prefix, if any */
+ if (strncmp(p, "mtdparts=", 9) == 0)
+ p += 9;
while (*p != '\0') {
err = 1;
I always wondered about this double mtdparts thing but was never brave
enough to touch it. ;)
Reviewed-by: Stefan Roese
Thanks,
Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v7 03/13] mtd: Kconfig: remove MTD_PARTITIONS duplicated symbol
On 31.08.2018 16:57, Miquel Raynal wrote: MTD_PARTITIONS is declared twice. Remove the redundant entry with no help associated. Signed-off-by: Miquel Raynal --- drivers/mtd/Kconfig | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig index d98457e223..9341d518f3 100644 --- a/drivers/mtd/Kconfig +++ b/drivers/mtd/Kconfig @@ -1,8 +1,5 @@ menu "MTD Support" -config MTD_PARTITIONS - bool - config MTD bool "Enable Driver Model for MTD drivers" depends on DM Reviewed-by: Stefan Roese Thanks, Stefan ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v7 04/13] cmd: mtdparts: accept spi-nand devices
On 31.08.2018 16:57, Miquel Raynal wrote:
Let spi-nand devices be recognized by mtdparts. This is superfluous
but a full mtdparts rework would be very time-consuming.
Signed-off-by: Miquel Raynal
Acked-by: Jagan Teki
Reviewed-by: Boris Brezillon
---
cmd/mtdparts.c | 13 -
include/jffs2/load_kernel.h | 7 +--
2 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/cmd/mtdparts.c b/cmd/mtdparts.c
index 756fc6018f..2e547894c6 100644
--- a/cmd/mtdparts.c
+++ b/cmd/mtdparts.c
@@ -37,7 +37,7 @@
* mtdids=[,,...]
*
* := =
- *:= 'nand'|'nor'|'onenand'
+ *:= 'nand'|'nor'|'onenand'|'spi-nand'
* := mtd device number, 0...
*:= unique device tag used by linux kernel to find mtd device
(mtd->name)
*
@@ -339,7 +339,7 @@ static int part_validate_eraseblock(struct mtdids *id,
struct part_info *part)
if (!mtd->numeraseregions) {
/*
-* Only one eraseregion (NAND, OneNAND or uniform NOR),
+* Only one eraseregion (NAND, SPI-NAND, OneNAND or uniform
NOR),
* checking for alignment is easy here
*/
offset = part->offset;
@@ -1030,7 +1030,7 @@ static struct mtdids* id_find_by_mtd_id(const char
*mtd_id, unsigned int mtd_id_
}
/**
- * Parse device id string := 'nand'|'nor'|'onenand',
+ * Parse device id string :=
'nand'|'nor'|'onenand'|'spi-nand',
* return device type and number.
*
* @param id string describing device id
@@ -1054,6 +1054,9 @@ int mtd_id_parse(const char *id, const char **ret_id, u8
*dev_type,
} else if (strncmp(p, "onenand", 7) == 0) {
*dev_type = MTD_DEV_TYPE_ONENAND;
p += 7;
+ } else if (strncmp(p, "spi-nand", 8) == 0) {
+ *dev_type = MTD_DEV_TYPE_SPINAND;
+ p += 8;
} else {
printf("incorrect device type in %s\n", id);
return 1;
@@ -1636,7 +1639,7 @@ static int parse_mtdids(const char *const ids)
while(p && (*p != '\0')) {
ret = 1;
- /* parse 'nor'|'nand'|'onenand' */
+ /* parse 'nor'|'nand'|'onenand'|'spi-nand' */
if (mtd_id_parse(p, &p, &type, &num) != 0)
break;
@@ -2112,7 +2115,7 @@ static char mtdparts_help_text[] =
"'mtdids' - linux kernel mtd device id <-> u-boot device id mapping\n\n"
"mtdids=[,,...]\n\n"
":= =\n"
- " := 'nand'|'nor'|'onenand'\n"
+ " := 'nand'|'nor'|'onenand'|'spi-nand'\n"
" := mtd device number, 0...\n"
" := unique device tag used by linux kernel to find mtd device
(mtd->name)\n\n"
"'mtdparts' - partition list\n\n"
diff --git a/include/jffs2/load_kernel.h b/include/jffs2/load_kernel.h
index 1ddff062ad..9346d7ee9f 100644
--- a/include/jffs2/load_kernel.h
+++ b/include/jffs2/load_kernel.h
@@ -15,9 +15,12 @@
#define MTD_DEV_TYPE_NOR 0x0001
#define MTD_DEV_TYPE_NAND 0x0002
#define MTD_DEV_TYPE_ONENAND 0x0004
+#define MTD_DEV_TYPE_SPINAND 0x0008
-#define MTD_DEV_TYPE(type) ((type == MTD_DEV_TYPE_NAND) ? "nand" : \
- (type == MTD_DEV_TYPE_ONENAND) ? "onenand" : "nor")
+#define MTD_DEV_TYPE(type) (type == MTD_DEV_TYPE_NAND ? "nand" : \
+ (type == MTD_DEV_TYPE_NOR ? "nor" : \
+(type == MTD_DEV_TYPE_ONENAND ? "onenand" : \
+ "spi-nand"))) \
struct mtd_device {
struct list_head link;
Reviewed-by: Stefan Roese
Thanks,
Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v7 02/13] lib: strto: fix metric suffix parsing in strtoul[l]
On 31.08.2018 16:57, Miquel Raynal wrote:
While 1kB or 1kiB will be parsed correctly, 1k will return the right
amount, but the metric suffix will not be escaped once the char
pointer updated. Fix this situation by simplifying the move of the
endp pointer.
Signed-off-by: Miquel Raynal
---
lib/strto.c | 22 ++
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/lib/strto.c b/lib/strto.c
index 84f8d92d57..502a0153e7 100644
--- a/lib/strto.c
+++ b/lib/strto.c
@@ -97,12 +97,11 @@ unsigned long ustrtoul(const char *cp, char **endp,
unsigned int base)
case 'K':
case 'k':
result *= 1024;
- if ((*endp)[1] == 'i') {
- if ((*endp)[2] == 'B')
- (*endp) += 3;
- else
- (*endp) += 2;
- }
+ (*endp)++;
+ if (**endp == 'i')
+ (*endp)++;
+ if (**endp == 'B')
+ (*endp)++;
}
return result;
}
@@ -122,12 +121,11 @@ unsigned long long ustrtoull(const char *cp, char **endp,
unsigned int base)
case 'K':
case 'k':
result *= 1024;
- if ((*endp)[1] == 'i') {
- if ((*endp)[2] == 'B')
- (*endp) += 3;
- else
- (*endp) += 2;
- }
+ (*endp)++;
+ if (**endp == 'i')
+ (*endp)++;
+ if (**endp == 'B')
+ (*endp)++;
}
return result;
}
Even though KiB is not equal to KB in general (at least in Linux
userspace AFAIK), lets not change this in U-Boot and always use
KiB and KB as a representation for 1024 (instead of 1000). So:
Reviewed-by: Stefan Roese
Thanks,
Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v7 01/13] lib: strto: parse all lowercase metric prefixes in ustrtoul[l]
On 31.08.2018 16:57, Miquel Raynal wrote:
Both ustrtoul and ustrtoull interpret 1k but not 1m or 1g. Even if the
SI symbols for Mega and Giga are 'M' and 'G', certain entries of
eg. mtdparts also use (wrongly) the metric prefix 'm' and 'g'.
I do not see how parsing lowercase prefixes could break anything, so
parse them like their uppercase counterpart.
Signed-off-by: Miquel Raynal
---
lib/strto.c | 4
1 file changed, 4 insertions(+)
diff --git a/lib/strto.c b/lib/strto.c
index 7f6076909a..84f8d92d57 100644
--- a/lib/strto.c
+++ b/lib/strto.c
@@ -87,9 +87,11 @@ unsigned long ustrtoul(const char *cp, char **endp, unsigned
int base)
unsigned long result = simple_strtoul(cp, endp, base);
switch (**endp) {
case 'G':
+ case 'g':
result *= 1024;
/* fall through */
case 'M':
+ case 'm':
result *= 1024;
/* fall through */
case 'K':
@@ -110,9 +112,11 @@ unsigned long long ustrtoull(const char *cp, char **endp,
unsigned int base)
unsigned long long result = simple_strtoull(cp, endp, base);
switch (**endp) {
case 'G':
+ case 'g':
result *= 1024;
/* fall through */
case 'M':
+ case 'm':
Wouldn't it be better, to use tolower() on the char and drop all the
upper case checks completely - also for the 'K' case?
Thanks,
Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 3/3] arm: spear: fix enabling of SSP2 clock
On 31.08.2018 16:28, Quentin Schulz wrote: The SSP2 clock is at bit 6 in the register, so the value is 0x40 unlike the current 0x70 which enables the clock of UART2, SSP1 and SSP2. Signed-off-by: Quentin Schulz --- added in v2 @Stefan: I think you'd want to test the FPGA on x600 again as it's using this constant. Having worked on a system close to the x600, I'm guessing that it'll work as fine as on my platform with this patch but better be sure than sorry. I don't have easily access to the x600 board right now. Please go ahead with this patch. I'm now informed about this change and will make some tests, if I get this board on my desk again. Acked-by: Stefan Roese Thanks, Stefan ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
