| Message ID | 20240831164639.2298097-1-caleb.connolly@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | cmd: fdt: use U-Boot's FDT by default | expand |
Hi Caleb, the problem here is hidden conditional behavior. On Sat, Aug 31, 2024 at 9:56 AM Caleb Connolly <caleb.connolly@linaro.org> wrote: > > When using the FDT command to inspect an arbitrary FDT in memory, it > will always be necessary to explicitly set the FDT address. However it > is also quite likely that the command is being used to inspect U-Boot's > own FDT. Simplify that common workflow of running "fdt addr -c" to get > the control address and set it by just making $fdtcontroladdr the > default FDT if there isn't one. > > Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> > --- > cmd/fdt.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/cmd/fdt.c b/cmd/fdt.c > index d16b141ce32d..8909706e2483 100644 > --- a/cmd/fdt.c > +++ b/cmd/fdt.c > @@ -276,8 +276,17 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) > > return CMD_RET_SUCCESS; > } > > + /* Try using U-Boot's FDT by default */ > + if (!working_fdt) { > + struct fdt_header *addr; > + > + addr = (void *)env_get_hex("fdtcontroladdr", 0); > + if (addr && fdt_check_header(&addr)) > + set_working_fdt_addr((phys_addr_t)addr); > + } > + > if (!working_fdt) { > puts("No FDT memory address configured. Please configure\n" > "the FDT address via \"fdt addr <address>\" command.\n" > "Aborting!\n"); > -- > 2.46.0 > The use of `fdt` command in the default action might be depended on by some userspace script as a success/fail result. I don't imagine what that might possibly be, just that the logic of scripts in u-boot depend on that pattern of use. Secondly there would need to be a warning to the user that some hidden conditional action is being applied? i.e. "No valid FDT address is configured to $fdt_addr_r or $fdt_addr so now configuring to use $fdtcontroladdr instead." or however you would phrase that. Otherwise I agree improvement to the fdt is welcome and my memory of first using this command is that it has not-sensible defaults but I then assumed it was designed this way because of possible use in U-Boot scripts. -E
Hi, On 31/08/2024 23:22, E Shattow wrote: > Hi Caleb, the problem here is hidden conditional behavior. > > On Sat, Aug 31, 2024 at 9:56 AM Caleb Connolly > <caleb.connolly@linaro.org> wrote: >> >> When using the FDT command to inspect an arbitrary FDT in memory, it >> will always be necessary to explicitly set the FDT address. However it >> is also quite likely that the command is being used to inspect U-Boot's >> own FDT. Simplify that common workflow of running "fdt addr -c" to get >> the control address and set it by just making $fdtcontroladdr the >> default FDT if there isn't one. >> >> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> >> --- >> cmd/fdt.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/cmd/fdt.c b/cmd/fdt.c >> index d16b141ce32d..8909706e2483 100644 >> --- a/cmd/fdt.c >> +++ b/cmd/fdt.c >> @@ -276,8 +276,17 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) >> >> return CMD_RET_SUCCESS; >> } >> >> + /* Try using U-Boot's FDT by default */ >> + if (!working_fdt) { >> + struct fdt_header *addr; >> + >> + addr = (void *)env_get_hex("fdtcontroladdr", 0); >> + if (addr && fdt_check_header(&addr)) >> + set_working_fdt_addr((phys_addr_t)addr); >> + } >> + >> if (!working_fdt) { >> puts("No FDT memory address configured. Please configure\n" >> "the FDT address via \"fdt addr <address>\" command.\n" >> "Aborting!\n"); >> -- >> 2.46.0 >> > > The use of `fdt` command in the default action might be depended on by > some userspace script as a success/fail result. I don't imagine what > that might possibly be, just that the logic of scripts in u-boot > depend on that pattern of use. I'm not sure what the rule is about breaking changes in the CLI, I do think this is not a realistic concern here though. Maybe Tom or Simon can weigh in? > > Secondly there would need to be a warning to the user that some hidden > conditional action is being applied? i.e. "No valid FDT address is > configured to $fdt_addr_r or $fdt_addr so now configuring to use > $fdtcontroladdr instead." or however you would phrase that. Since I call set_working_fdt_addr() it prints a message telling you that the fdt address was set on the first call. > > Otherwise I agree improvement to the fdt is welcome and my memory of > first using this command is that it has not-sensible defaults but I > then assumed it was designed this way because of possible use in > U-Boot scripts. Thanks. > > -E
Hi Caleb, On Sun, 1 Sept 2024 at 08:21, Caleb Connolly <caleb.connolly@linaro.org> wrote: > > Hi, > > On 31/08/2024 23:22, E Shattow wrote: > > Hi Caleb, the problem here is hidden conditional behavior. > > > > On Sat, Aug 31, 2024 at 9:56 AM Caleb Connolly > > <caleb.connolly@linaro.org> wrote: > >> > >> When using the FDT command to inspect an arbitrary FDT in memory, it > >> will always be necessary to explicitly set the FDT address. However it > >> is also quite likely that the command is being used to inspect U-Boot's > >> own FDT. Simplify that common workflow of running "fdt addr -c" to get > >> the control address and set it by just making $fdtcontroladdr the > >> default FDT if there isn't one. > >> > >> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> > >> --- > >> cmd/fdt.c | 9 +++++++++ > >> 1 file changed, 9 insertions(+) > >> > >> diff --git a/cmd/fdt.c b/cmd/fdt.c > >> index d16b141ce32d..8909706e2483 100644 > >> --- a/cmd/fdt.c > >> +++ b/cmd/fdt.c > >> @@ -276,8 +276,17 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) > >> > >> return CMD_RET_SUCCESS; > >> } > >> > >> + /* Try using U-Boot's FDT by default */ > >> + if (!working_fdt) { > >> + struct fdt_header *addr; > >> + > >> + addr = (void *)env_get_hex("fdtcontroladdr", 0); > >> + if (addr && fdt_check_header(&addr)) > >> + set_working_fdt_addr((phys_addr_t)addr); > >> + } > >> + > >> if (!working_fdt) { > >> puts("No FDT memory address configured. Please configure\n" > >> "the FDT address via \"fdt addr <address>\" command.\n" > >> "Aborting!\n"); > >> -- > >> 2.46.0 > >> > > > > The use of `fdt` command in the default action might be depended on by > > some userspace script as a success/fail result. I don't imagine what > > that might possibly be, just that the logic of scripts in u-boot > > depend on that pattern of use. > > I'm not sure what the rule is about breaking changes in the CLI, I do > think this is not a realistic concern here though. Maybe Tom or Simon > can weigh in? > > > > Secondly there would need to be a warning to the user that some hidden > > conditional action is being applied? i.e. "No valid FDT address is > > configured to $fdt_addr_r or $fdt_addr so now configuring to use > > $fdtcontroladdr instead." or however you would phrase that. > > Since I call set_working_fdt_addr() it prints a message telling you that > the fdt address was set on the first call. > > > > Otherwise I agree improvement to the fdt is welcome and my memory of > > first using this command is that it has not-sensible defaults but I > > then assumed it was designed this way because of possible use in > > U-Boot scripts. Definitely a useful feature, but how about adding a flag which sets the working fdt to the control one? That would make it less painful than using -c, and will keep existing cases running. Also note that test/cmd/fdt.c and doc/usage/cmd/fdt.rst need an update for this. Regards, Simon
Hi Simon, On 01/09/2024 21:10, Simon Glass wrote: > Hi Caleb, > > On Sun, 1 Sept 2024 at 08:21, Caleb Connolly <caleb.connolly@linaro.org> wrote: >> >> Hi, >> >> On 31/08/2024 23:22, E Shattow wrote: >>> Hi Caleb, the problem here is hidden conditional behavior. >>> >>> On Sat, Aug 31, 2024 at 9:56 AM Caleb Connolly >>> <caleb.connolly@linaro.org> wrote: >>>> >>>> When using the FDT command to inspect an arbitrary FDT in memory, it >>>> will always be necessary to explicitly set the FDT address. However it >>>> is also quite likely that the command is being used to inspect U-Boot's >>>> own FDT. Simplify that common workflow of running "fdt addr -c" to get >>>> the control address and set it by just making $fdtcontroladdr the >>>> default FDT if there isn't one. >>>> >>>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> >>>> --- >>>> cmd/fdt.c | 9 +++++++++ >>>> 1 file changed, 9 insertions(+) >>>> >>>> diff --git a/cmd/fdt.c b/cmd/fdt.c >>>> index d16b141ce32d..8909706e2483 100644 >>>> --- a/cmd/fdt.c >>>> +++ b/cmd/fdt.c >>>> @@ -276,8 +276,17 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) >>>> >>>> return CMD_RET_SUCCESS; >>>> } >>>> >>>> + /* Try using U-Boot's FDT by default */ >>>> + if (!working_fdt) { >>>> + struct fdt_header *addr; >>>> + >>>> + addr = (void *)env_get_hex("fdtcontroladdr", 0); >>>> + if (addr && fdt_check_header(&addr)) >>>> + set_working_fdt_addr((phys_addr_t)addr); >>>> + } >>>> + >>>> if (!working_fdt) { >>>> puts("No FDT memory address configured. Please configure\n" >>>> "the FDT address via \"fdt addr <address>\" command.\n" >>>> "Aborting!\n"); >>>> -- >>>> 2.46.0 >>>> >>> >>> The use of `fdt` command in the default action might be depended on by >>> some userspace script as a success/fail result. I don't imagine what >>> that might possibly be, just that the logic of scripts in u-boot >>> depend on that pattern of use. >> >> I'm not sure what the rule is about breaking changes in the CLI, I do >> think this is not a realistic concern here though. Maybe Tom or Simon >> can weigh in? >>> >>> Secondly there would need to be a warning to the user that some hidden >>> conditional action is being applied? i.e. "No valid FDT address is >>> configured to $fdt_addr_r or $fdt_addr so now configuring to use >>> $fdtcontroladdr instead." or however you would phrase that. >> >> Since I call set_working_fdt_addr() it prints a message telling you that >> the fdt address was set on the first call. >>> >>> Otherwise I agree improvement to the fdt is welcome and my memory of >>> first using this command is that it has not-sensible defaults but I >>> then assumed it was designed this way because of possible use in >>> U-Boot scripts. > > Definitely a useful feature, but how about adding a flag which sets > the working fdt to the control one? That would make it less painful > than using -c, and will keep existing cases running. Surely we have to /have/ some usecases to justify this?? > > Also note that test/cmd/fdt.c and doc/usage/cmd/fdt.rst need an update for this. For sure > > Regards, > Simon
On Sat, Aug 31, 2024 at 05:46:19PM +0100, Caleb Connolly wrote: > When using the FDT command to inspect an arbitrary FDT in memory, it > will always be necessary to explicitly set the FDT address. However it > is also quite likely that the command is being used to inspect U-Boot's > own FDT. Simplify that common workflow of running "fdt addr -c" to get > the control address and set it by just making $fdtcontroladdr the > default FDT if there isn't one. > > Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> > --- > cmd/fdt.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/cmd/fdt.c b/cmd/fdt.c > index d16b141ce32d..8909706e2483 100644 > --- a/cmd/fdt.c > +++ b/cmd/fdt.c > @@ -276,8 +276,17 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) > > return CMD_RET_SUCCESS; > } > > + /* Try using U-Boot's FDT by default */ > + if (!working_fdt) { > + struct fdt_header *addr; > + > + addr = (void *)env_get_hex("fdtcontroladdr", 0); > + if (addr && fdt_check_header(&addr)) > + set_working_fdt_addr((phys_addr_t)addr); > + } > + > if (!working_fdt) { > puts("No FDT memory address configured. Please configure\n" > "the FDT address via \"fdt addr <address>\" command.\n" > "Aborting!\n"); Setting aside the behavior change (which I am thinking about), this makes the next check of !working_fdt dead code which should be removed.
On Mon, Sep 02, 2024 at 03:07:44PM +0200, Caleb Connolly wrote: > Hi Simon, > > On 01/09/2024 21:10, Simon Glass wrote: > > Hi Caleb, > > > > On Sun, 1 Sept 2024 at 08:21, Caleb Connolly <caleb.connolly@linaro.org> wrote: > >> > >> Hi, > >> > >> On 31/08/2024 23:22, E Shattow wrote: > >>> Hi Caleb, the problem here is hidden conditional behavior. > >>> > >>> On Sat, Aug 31, 2024 at 9:56 AM Caleb Connolly > >>> <caleb.connolly@linaro.org> wrote: > >>>> > >>>> When using the FDT command to inspect an arbitrary FDT in memory, it > >>>> will always be necessary to explicitly set the FDT address. However it > >>>> is also quite likely that the command is being used to inspect U-Boot's > >>>> own FDT. Simplify that common workflow of running "fdt addr -c" to get > >>>> the control address and set it by just making $fdtcontroladdr the > >>>> default FDT if there isn't one. > >>>> > >>>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> > >>>> --- > >>>> cmd/fdt.c | 9 +++++++++ > >>>> 1 file changed, 9 insertions(+) > >>>> > >>>> diff --git a/cmd/fdt.c b/cmd/fdt.c > >>>> index d16b141ce32d..8909706e2483 100644 > >>>> --- a/cmd/fdt.c > >>>> +++ b/cmd/fdt.c > >>>> @@ -276,8 +276,17 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) > >>>> > >>>> return CMD_RET_SUCCESS; > >>>> } > >>>> > >>>> + /* Try using U-Boot's FDT by default */ > >>>> + if (!working_fdt) { > >>>> + struct fdt_header *addr; > >>>> + > >>>> + addr = (void *)env_get_hex("fdtcontroladdr", 0); > >>>> + if (addr && fdt_check_header(&addr)) > >>>> + set_working_fdt_addr((phys_addr_t)addr); > >>>> + } > >>>> + > >>>> if (!working_fdt) { > >>>> puts("No FDT memory address configured. Please configure\n" > >>>> "the FDT address via \"fdt addr <address>\" command.\n" > >>>> "Aborting!\n"); > >>>> -- > >>>> 2.46.0 > >>>> > >>> > >>> The use of `fdt` command in the default action might be depended on by > >>> some userspace script as a success/fail result. I don't imagine what > >>> that might possibly be, just that the logic of scripts in u-boot > >>> depend on that pattern of use. I don't think anything depends on "fdt addr" returning 1 if nothing is set. > >> I'm not sure what the rule is about breaking changes in the CLI, I do > >> think this is not a realistic concern here though. Maybe Tom or Simon > >> can weigh in? > >>> > >>> Secondly there would need to be a warning to the user that some hidden > >>> conditional action is being applied? i.e. "No valid FDT address is > >>> configured to $fdt_addr_r or $fdt_addr so now configuring to use > >>> $fdtcontroladdr instead." or however you would phrase that. > >> > >> Since I call set_working_fdt_addr() it prints a message telling you that > >> the fdt address was set on the first call. Yes, but it's not obvious as-is where that address came from / why, since today that happens because you're passing that to the command. > >>> Otherwise I agree improvement to the fdt is welcome and my memory of > >>> first using this command is that it has not-sensible defaults but I > >>> then assumed it was designed this way because of possible use in > >>> U-Boot scripts. > > > > Definitely a useful feature, but how about adding a flag which sets > > the working fdt to the control one? That would make it less painful > > than using -c, and will keep existing cases running. > > Surely we have to /have/ some usecases to justify this?? Well, "fdt" the command was introduced back when Linux started taking device trees on PowerPC platforms, so there wasn't some default to look at. U-Boot isn't standardized on "fdtaddr" or "fdt_addr_r" or "fdt_addr" as where the device tree for the OS is, only that "fdtcontroladdr" is ours. So yes, I would also prefer a new flag to automatically set the working address to fdtcontroladdr. But I assume there's a good reason to not just do like other platforms have historically and fdt addr ... as needed before other commands? Or is this really just for interactive development?
On 03/09/2024 20:29, Tom Rini wrote: > On Mon, Sep 02, 2024 at 03:07:44PM +0200, Caleb Connolly wrote: >> Hi Simon, >> >> On 01/09/2024 21:10, Simon Glass wrote: >>> Hi Caleb, >>> >>> On Sun, 1 Sept 2024 at 08:21, Caleb Connolly <caleb.connolly@linaro.org> wrote: >>>> >>>> Hi, >>>> >>>> On 31/08/2024 23:22, E Shattow wrote: >>>>> Hi Caleb, the problem here is hidden conditional behavior. >>>>> >>>>> On Sat, Aug 31, 2024 at 9:56 AM Caleb Connolly >>>>> <caleb.connolly@linaro.org> wrote: >>>>>> >>>>>> When using the FDT command to inspect an arbitrary FDT in memory, it >>>>>> will always be necessary to explicitly set the FDT address. However it >>>>>> is also quite likely that the command is being used to inspect U-Boot's >>>>>> own FDT. Simplify that common workflow of running "fdt addr -c" to get >>>>>> the control address and set it by just making $fdtcontroladdr the >>>>>> default FDT if there isn't one. >>>>>> >>>>>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> >>>>>> --- >>>>>> cmd/fdt.c | 9 +++++++++ >>>>>> 1 file changed, 9 insertions(+) >>>>>> >>>>>> diff --git a/cmd/fdt.c b/cmd/fdt.c >>>>>> index d16b141ce32d..8909706e2483 100644 >>>>>> --- a/cmd/fdt.c >>>>>> +++ b/cmd/fdt.c >>>>>> @@ -276,8 +276,17 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) >>>>>> >>>>>> return CMD_RET_SUCCESS; >>>>>> } >>>>>> >>>>>> + /* Try using U-Boot's FDT by default */ >>>>>> + if (!working_fdt) { >>>>>> + struct fdt_header *addr; >>>>>> + >>>>>> + addr = (void *)env_get_hex("fdtcontroladdr", 0); >>>>>> + if (addr && fdt_check_header(&addr)) >>>>>> + set_working_fdt_addr((phys_addr_t)addr); >>>>>> + } >>>>>> + >>>>>> if (!working_fdt) { >>>>>> puts("No FDT memory address configured. Please configure\n" >>>>>> "the FDT address via \"fdt addr <address>\" command.\n" >>>>>> "Aborting!\n"); >>>>>> -- >>>>>> 2.46.0 >>>>>> >>>>> >>>>> The use of `fdt` command in the default action might be depended on by >>>>> some userspace script as a success/fail result. I don't imagine what >>>>> that might possibly be, just that the logic of scripts in u-boot >>>>> depend on that pattern of use. > > I don't think anything depends on "fdt addr" returning 1 if nothing is > set. > >>>> I'm not sure what the rule is about breaking changes in the CLI, I do >>>> think this is not a realistic concern here though. Maybe Tom or Simon >>>> can weigh in? >>>>> >>>>> Secondly there would need to be a warning to the user that some hidden >>>>> conditional action is being applied? i.e. "No valid FDT address is >>>>> configured to $fdt_addr_r or $fdt_addr so now configuring to use >>>>> $fdtcontroladdr instead." or however you would phrase that. >>>> >>>> Since I call set_working_fdt_addr() it prints a message telling you that >>>> the fdt address was set on the first call. > > Yes, but it's not obvious as-is where that address came from / why, > since today that happens because you're passing that to the command. > >>>>> Otherwise I agree improvement to the fdt is welcome and my memory of >>>>> first using this command is that it has not-sensible defaults but I >>>>> then assumed it was designed this way because of possible use in >>>>> U-Boot scripts. >>> >>> Definitely a useful feature, but how about adding a flag which sets >>> the working fdt to the control one? That would make it less painful >>> than using -c, and will keep existing cases running. >> >> Surely we have to /have/ some usecases to justify this?? > > Well, "fdt" the command was introduced back when Linux started taking > device trees on PowerPC platforms, so there wasn't some default to look > at. U-Boot isn't standardized on "fdtaddr" or "fdt_addr_r" or "fdt_addr" > as where the device tree for the OS is, only that "fdtcontroladdr" is > ours. So yes, I would also prefer a new flag to automatically set the > working address to fdtcontroladdr. But I assume there's a good reason to > not just do like other platforms have historically and fdt addr ... as > needed before other commands? Or is this really just for interactive > development? My original justification was so we could add a boot menu option on phones that just did "fdt print /chosen bootargs" (to dump the cmdline args set by Qualcomm's bootloader). I originally had "fdt addr $fdtcontroladdr" in preboot but it felt a bit hacky, but maybe that was the right approach >
On 03/09/2024 20:14, Tom Rini wrote: > On Sat, Aug 31, 2024 at 05:46:19PM +0100, Caleb Connolly wrote: > >> When using the FDT command to inspect an arbitrary FDT in memory, it >> will always be necessary to explicitly set the FDT address. However it >> is also quite likely that the command is being used to inspect U-Boot's >> own FDT. Simplify that common workflow of running "fdt addr -c" to get >> the control address and set it by just making $fdtcontroladdr the >> default FDT if there isn't one. >> >> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> >> --- >> cmd/fdt.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/cmd/fdt.c b/cmd/fdt.c >> index d16b141ce32d..8909706e2483 100644 >> --- a/cmd/fdt.c >> +++ b/cmd/fdt.c >> @@ -276,8 +276,17 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) >> >> return CMD_RET_SUCCESS; >> } >> >> + /* Try using U-Boot's FDT by default */ >> + if (!working_fdt) { >> + struct fdt_header *addr; >> + >> + addr = (void *)env_get_hex("fdtcontroladdr", 0); >> + if (addr && fdt_check_header(&addr)) >> + set_working_fdt_addr((phys_addr_t)addr); >> + } >> + >> if (!working_fdt) { >> puts("No FDT memory address configured. Please configure\n" >> "the FDT address via \"fdt addr <address>\" command.\n" >> "Aborting!\n"); > > Setting aside the behavior change (which I am thinking about), this > makes the next check of !working_fdt dead code which should be removed. I wasn't sure if we could safely assume that fdtcontroladdr always points to a valid FDT, if that's true then yes this can be dropped. >
Hi Caleb, On Fri, 6 Sept 2024 at 03:31, Caleb Connolly <caleb.connolly@linaro.org> wrote: > > > > On 03/09/2024 20:14, Tom Rini wrote: > > On Sat, Aug 31, 2024 at 05:46:19PM +0100, Caleb Connolly wrote: > > > >> When using the FDT command to inspect an arbitrary FDT in memory, it > >> will always be necessary to explicitly set the FDT address. However it > >> is also quite likely that the command is being used to inspect U-Boot's > >> own FDT. Simplify that common workflow of running "fdt addr -c" to get > >> the control address and set it by just making $fdtcontroladdr the > >> default FDT if there isn't one. > >> > >> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> > >> --- > >> cmd/fdt.c | 9 +++++++++ > >> 1 file changed, 9 insertions(+) > >> > >> diff --git a/cmd/fdt.c b/cmd/fdt.c > >> index d16b141ce32d..8909706e2483 100644 > >> --- a/cmd/fdt.c > >> +++ b/cmd/fdt.c > >> @@ -276,8 +276,17 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) > >> > >> return CMD_RET_SUCCESS; > >> } > >> > >> + /* Try using U-Boot's FDT by default */ > >> + if (!working_fdt) { > >> + struct fdt_header *addr; > >> + > >> + addr = (void *)env_get_hex("fdtcontroladdr", 0); > >> + if (addr && fdt_check_header(&addr)) > >> + set_working_fdt_addr((phys_addr_t)addr); > >> + } > >> + > >> if (!working_fdt) { > >> puts("No FDT memory address configured. Please configure\n" > >> "the FDT address via \"fdt addr <address>\" command.\n" > >> "Aborting!\n"); > > > > Setting aside the behavior change (which I am thinking about), this > > makes the next check of !working_fdt dead code which should be removed. > > I wasn't sure if we could safely assume that fdtcontroladdr always > points to a valid FDT, if that's true then yes this can be dropped. $ ./tools/qconfig.py -f ~OF_CONTROL 11 matches integratorap_cm720t integratorap_cm920t integratorap_cm926ejs integratorap_cm946es integratorcp_cm1136 integratorcp_cm920t integratorcp_cm926ejs integratorcp_cm946es mx6memcal work_92105 xtfpga So yes, there are boards where it would not be set. Regards, Simon
On Fri, Sep 06, 2024 at 09:02:25AM -0600, Simon Glass wrote: > Hi Caleb, > > On Fri, 6 Sept 2024 at 03:31, Caleb Connolly <caleb.connolly@linaro.org> wrote: > > > > > > > > On 03/09/2024 20:14, Tom Rini wrote: > > > On Sat, Aug 31, 2024 at 05:46:19PM +0100, Caleb Connolly wrote: > > > > > >> When using the FDT command to inspect an arbitrary FDT in memory, it > > >> will always be necessary to explicitly set the FDT address. However it > > >> is also quite likely that the command is being used to inspect U-Boot's > > >> own FDT. Simplify that common workflow of running "fdt addr -c" to get > > >> the control address and set it by just making $fdtcontroladdr the > > >> default FDT if there isn't one. > > >> > > >> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> > > >> --- > > >> cmd/fdt.c | 9 +++++++++ > > >> 1 file changed, 9 insertions(+) > > >> > > >> diff --git a/cmd/fdt.c b/cmd/fdt.c > > >> index d16b141ce32d..8909706e2483 100644 > > >> --- a/cmd/fdt.c > > >> +++ b/cmd/fdt.c > > >> @@ -276,8 +276,17 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) > > >> > > >> return CMD_RET_SUCCESS; > > >> } > > >> > > >> + /* Try using U-Boot's FDT by default */ > > >> + if (!working_fdt) { > > >> + struct fdt_header *addr; > > >> + > > >> + addr = (void *)env_get_hex("fdtcontroladdr", 0); > > >> + if (addr && fdt_check_header(&addr)) > > >> + set_working_fdt_addr((phys_addr_t)addr); > > >> + } > > >> + > > >> if (!working_fdt) { > > >> puts("No FDT memory address configured. Please configure\n" > > >> "the FDT address via \"fdt addr <address>\" command.\n" > > >> "Aborting!\n"); > > > > > > Setting aside the behavior change (which I am thinking about), this > > > makes the next check of !working_fdt dead code which should be removed. > > > > I wasn't sure if we could safely assume that fdtcontroladdr always > > points to a valid FDT, if that's true then yes this can be dropped. > > $ ./tools/qconfig.py -f ~OF_CONTROL > 11 matches > integratorap_cm720t integratorap_cm920t integratorap_cm926ejs > integratorap_cm946es integratorcp_cm1136 integratorcp_cm920t > integratorcp_cm926ejs integratorcp_cm946es mx6memcal work_92105 xtfpga > > So yes, there are boards where it would not be set. Well, mx6memcal is special. Perhaps all of those integrator boards need dropping? And I'm not sure what's going on with xtfpga...
diff --git a/cmd/fdt.c b/cmd/fdt.c index d16b141ce32d..8909706e2483 100644 --- a/cmd/fdt.c +++ b/cmd/fdt.c @@ -276,8 +276,17 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) return CMD_RET_SUCCESS; } + /* Try using U-Boot's FDT by default */ + if (!working_fdt) { + struct fdt_header *addr; + + addr = (void *)env_get_hex("fdtcontroladdr", 0); + if (addr && fdt_check_header(&addr)) + set_working_fdt_addr((phys_addr_t)addr); + } + if (!working_fdt) { puts("No FDT memory address configured. Please configure\n" "the FDT address via \"fdt addr <address>\" command.\n" "Aborting!\n");
When using the FDT command to inspect an arbitrary FDT in memory, it will always be necessary to explicitly set the FDT address. However it is also quite likely that the command is being used to inspect U-Boot's own FDT. Simplify that common workflow of running "fdt addr -c" to get the control address and set it by just making $fdtcontroladdr the default FDT if there isn't one. Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> --- cmd/fdt.c | 9 +++++++++ 1 file changed, 9 insertions(+)