| Message ID | 20180830190523.31474-2-robh@kernel.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | of: root #{size,address}-cells clean-ups | expand |
On 08/30/18 12:05, Rob Herring wrote: > Scan the root node properties (#{size,address}-cells) earlier, ^^^^^^^ before mdesc->dt_fixup() is called > so that > the dt_root_addr_cells and dt_root_size_cells variables are initialized > and can be used. by mdesc->dt_fixup() > > Cc: Frank Rowand <frowand.list@gmail.com> > Signed-off-by: Rob Herring <robh@kernel.org> > --- > drivers/of/fdt.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > Moving early_init_dt_scan_root() to inside early_init_dt_verify() puts something that has nothing to do with verifying the fdt into a function whose purpose is the verify. It hides the side effect of initializing the dt_root_addr_cells and dt_root_size_cells variables. I suggest creating a new function early_init_dt_scan_init_pre_dt_fixup(), move the chunk of code there instead of to early_init_dt_scan_nodes(), and call the new function from setup_machine_fdt(), just before calling mdesc->dt_fixup(). This would be a little bit more code, but more clearly showing the intent. -Frank > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c > index 800ad252cf9c..49abe18f1bde 100644 > --- a/drivers/of/fdt.c > +++ b/drivers/of/fdt.c > @@ -1215,6 +1215,10 @@ bool __init early_init_dt_verify(void *params) > initial_boot_params = params; > of_fdt_crc32 = crc32_be(~0, initial_boot_params, > fdt_totalsize(initial_boot_params)); > + > + /* Initialize {size,address}-cells info */ > + of_scan_flat_dt(early_init_dt_scan_root, NULL); > + > return true; > } > > @@ -1224,9 +1228,6 @@ void __init early_init_dt_scan_nodes(void) > /* Retrieve various information from the /chosen node */ > of_scan_flat_dt(early_init_dt_scan_chosen, boot_command_line); > > - /* Initialize {size,address}-cells info */ > - of_scan_flat_dt(early_init_dt_scan_root, NULL); > - > /* Setup memory, calling early_init_dt_add_memory_arch */ > of_scan_flat_dt(early_init_dt_scan_memory, NULL); > } >
On Tue, Sep 4, 2018 at 8:49 PM Frank Rowand <frowand.list@gmail.com> wrote: > > On 08/30/18 12:05, Rob Herring wrote: > > Scan the root node properties (#{size,address}-cells) earlier, > > ^^^^^^^ > before mdesc->dt_fixup() is called > > > so that > > the dt_root_addr_cells and dt_root_size_cells variables are initialized > > and can be used. > by mdesc->dt_fixup() That's an ARM specific detail. Granted, ARM is the only caller. > > > > Cc: Frank Rowand <frowand.list@gmail.com> > > Signed-off-by: Rob Herring <robh@kernel.org> > > --- > > drivers/of/fdt.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > Moving early_init_dt_scan_root() to inside early_init_dt_verify() > puts something that has nothing to do with verifying the fdt > into a function whose purpose is the verify. It hides the side > effect of initializing the dt_root_addr_cells and dt_root_size_cells > variables. It already has the side effect of setting initial_boot_params which every subsequent function needs. > I suggest creating a new function early_init_dt_scan_init_pre_dt_fixup(), > move the chunk of code there instead of to early_init_dt_scan_nodes(), > and call the new function from setup_machine_fdt(), just before > calling mdesc->dt_fixup(). This would be a little bit more code, > but more clearly showing the intent. I'm trying to reduce the number of functions arches call and renaming would need a bunch of arch changes. This change will also let me make early_init_dt_scan_root private as powerpc is the only user. I need to dust off a patch for that. I'd be more inclined to push exynos to remove this altogether. After all, if they claim their bindings are unstable, they can't really claim their bootloader is stable/fixed. Rob
On 09/05/18 04:51, Rob Herring wrote: > On Tue, Sep 4, 2018 at 8:49 PM Frank Rowand <frowand.list@gmail.com> wrote: >> >> On 08/30/18 12:05, Rob Herring wrote: >>> Scan the root node properties (#{size,address}-cells) earlier, >> >> ^^^^^^^ >> before mdesc->dt_fixup() is called >> >>> so that >>> the dt_root_addr_cells and dt_root_size_cells variables are initialized >>> and can be used. >> by mdesc->dt_fixup() > > That's an ARM specific detail. Granted, ARM is the only caller. The dt_root_addr_cells and dt_root_size_cells variables are being initialized earlier in this patch series so that of_fdt_limit_memory() can use them. The only caller of of_fdt_limit_memory() is exynos_dt_fixup(), which is an mdesc->dt_fixup() function. > >>> >>> Cc: Frank Rowand <frowand.list@gmail.com> >>> Signed-off-by: Rob Herring <robh@kernel.org> >>> --- >>> drivers/of/fdt.c | 7 ++++--- >>> 1 file changed, 4 insertions(+), 3 deletions(-) >>> >> Moving early_init_dt_scan_root() to inside early_init_dt_verify() >> puts something that has nothing to do with verifying the fdt >> into a function whose purpose is the verify. It hides the side >> effect of initializing the dt_root_addr_cells and dt_root_size_cells >> variables. > > It already has the side effect of setting initial_boot_params which > every subsequent function needs. And that side effect should probably also be moved. >> I suggest creating a new function early_init_dt_scan_init_pre_dt_fixup(), >> move the chunk of code there instead of to early_init_dt_scan_nodes(), >> and call the new function from setup_machine_fdt(), just before >> calling mdesc->dt_fixup(). This would be a little bit more code, >> but more clearly showing the intent. > > I'm trying to reduce the number of functions arches call I like that goal. > and renaming > would need a bunch of arch changes. This change will also let me make > early_init_dt_scan_root private as powerpc is the only user. I need to > dust off a patch for that. > > I'd be more inclined to push exynos to remove this altogether. After Not a bad idea. > all, if they claim their bindings are unstable, they can't really > claim their bootloader is stable/fixed. It seems that this series is showing us that maybe the three architecture specific (arc, arm, arm64) versions of setup_machine_fdt() should be consolidated so that we have consistent behavior for FDT. If we had a single setup_machine_fdt() then some of he hidden side effects of functions called by setup_machine_fdt() could instead be hoisted into setup_machine_fdt(). > > Rob >
On Wed, Sep 5, 2018 at 1:19 PM Frank Rowand <frowand.list@gmail.com> wrote: > > On 09/05/18 04:51, Rob Herring wrote: > > On Tue, Sep 4, 2018 at 8:49 PM Frank Rowand <frowand.list@gmail.com> wrote: > >> > >> On 08/30/18 12:05, Rob Herring wrote: > >>> Scan the root node properties (#{size,address}-cells) earlier, > >> > >> ^^^^^^^ > >> before mdesc->dt_fixup() is called > >> > >>> so that > >>> the dt_root_addr_cells and dt_root_size_cells variables are initialized > >>> and can be used. > >> by mdesc->dt_fixup() > > > > That's an ARM specific detail. Granted, ARM is the only caller. > > The dt_root_addr_cells and dt_root_size_cells variables are being > initialized earlier in this patch series so that of_fdt_limit_memory() > can use them. The only caller of of_fdt_limit_memory() is > exynos_dt_fixup(), which is an mdesc->dt_fixup() function. > > > > > >>> > >>> Cc: Frank Rowand <frowand.list@gmail.com> > >>> Signed-off-by: Rob Herring <robh@kernel.org> > >>> --- > >>> drivers/of/fdt.c | 7 ++++--- > >>> 1 file changed, 4 insertions(+), 3 deletions(-) > >>> > >> Moving early_init_dt_scan_root() to inside early_init_dt_verify() > >> puts something that has nothing to do with verifying the fdt > >> into a function whose purpose is the verify. It hides the side > >> effect of initializing the dt_root_addr_cells and dt_root_size_cells > >> variables. > > > > It already has the side effect of setting initial_boot_params which > > every subsequent function needs. > > And that side effect should probably also be moved. So 2 functions? One to set the blob and one to verify it. Then we can just let arches decide if they want to do any verification or not. Perhaps it should be called fdt_init(blob) and then it is vague enough I can do whatever I want. > >> I suggest creating a new function early_init_dt_scan_init_pre_dt_fixup(), > >> move the chunk of code there instead of to early_init_dt_scan_nodes(), > >> and call the new function from setup_machine_fdt(), just before > >> calling mdesc->dt_fixup(). This would be a little bit more code, > >> but more clearly showing the intent. > > > > I'm trying to reduce the number of functions arches call > > I like that goal. > > > > and renaming > > would need a bunch of arch changes. This change will also let me make > > early_init_dt_scan_root private as powerpc is the only user. I need to > > dust off a patch for that. > > > > I'd be more inclined to push exynos to remove this altogether. After > > Not a bad idea. > > > all, if they claim their bindings are unstable, they can't really > > claim their bootloader is stable/fixed. > > It seems that this series is showing us that maybe the three architecture > specific (arc, arm, arm64) versions of setup_machine_fdt() should be > consolidated so that we have consistent behavior for FDT. > > If we had a single setup_machine_fdt() then some of he hidden side > effects of functions called by setup_machine_fdt() could instead > be hoisted into setup_machine_fdt(). Those functions are all quite a bit different. ARM matches the machine desc while arm64 doesn't have any such thing. How the DTB gets mapped into virtual space also varies. Rob
On 09/05/18 13:06, Rob Herring wrote: > On Wed, Sep 5, 2018 at 1:19 PM Frank Rowand <frowand.list@gmail.com> wrote: >> >> On 09/05/18 04:51, Rob Herring wrote: >>> On Tue, Sep 4, 2018 at 8:49 PM Frank Rowand <frowand.list@gmail.com> wrote: >>>> >>>> On 08/30/18 12:05, Rob Herring wrote: >>>>> Scan the root node properties (#{size,address}-cells) earlier, >>>> >>>> ^^^^^^^ >>>> before mdesc->dt_fixup() is called >>>> >>>>> so that >>>>> the dt_root_addr_cells and dt_root_size_cells variables are initialized >>>>> and can be used. >>>> by mdesc->dt_fixup() >>> >>> That's an ARM specific detail. Granted, ARM is the only caller. >> >> The dt_root_addr_cells and dt_root_size_cells variables are being >> initialized earlier in this patch series so that of_fdt_limit_memory() >> can use them. The only caller of of_fdt_limit_memory() is >> exynos_dt_fixup(), which is an mdesc->dt_fixup() function. >> >> >>> >>>>> >>>>> Cc: Frank Rowand <frowand.list@gmail.com> >>>>> Signed-off-by: Rob Herring <robh@kernel.org> >>>>> --- >>>>> drivers/of/fdt.c | 7 ++++--- >>>>> 1 file changed, 4 insertions(+), 3 deletions(-) >>>>> >>>> Moving early_init_dt_scan_root() to inside early_init_dt_verify() >>>> puts something that has nothing to do with verifying the fdt >>>> into a function whose purpose is the verify. It hides the side >>>> effect of initializing the dt_root_addr_cells and dt_root_size_cells >>>> variables. >>> >>> It already has the side effect of setting initial_boot_params which >>> every subsequent function needs. >> >> And that side effect should probably also be moved. > > So 2 functions? One to set the blob and one to verify it. Then we can No, I would not add yet another function. All of these side effects are an argument in favor of a single setup_machine_fdt(), as I suggested below. Then all of these side effects could be in setup_machine_fdt() instead of hiding them in sub-functions that are called by all of the different architectures. > just let arches decide if they want to do any verification or not. > > Perhaps it should be called fdt_init(blob) and then it is vague enough > I can do whatever I want. > >>>> I suggest creating a new function early_init_dt_scan_init_pre_dt_fixup(), >>>> move the chunk of code there instead of to early_init_dt_scan_nodes(), >>>> and call the new function from setup_machine_fdt(), just before >>>> calling mdesc->dt_fixup(). This would be a little bit more code, >>>> but more clearly showing the intent. >>> >>> I'm trying to reduce the number of functions arches call >> >> I like that goal. >> >> >>> and renaming >>> would need a bunch of arch changes. This change will also let me make >>> early_init_dt_scan_root private as powerpc is the only user. I need to >>> dust off a patch for that. >>> >>> I'd be more inclined to push exynos to remove this altogether. After >> >> Not a bad idea. >> >>> all, if they claim their bindings are unstable, they can't really >>> claim their bootloader is stable/fixed. >> >> It seems that this series is showing us that maybe the three architecture >> specific (arc, arm, arm64) versions of setup_machine_fdt() should be >> consolidated so that we have consistent behavior for FDT. >> >> If we had a single setup_machine_fdt() then some of he hidden side >> effects of functions called by setup_machine_fdt() could instead >> be hoisted into setup_machine_fdt(). > > Those functions are all quite a bit different. ARM matches the machine > desc while arm64 doesn't have any such thing. How the DTB gets mapped > into virtual space also varies. I argue that they _should be_ made to be more alike than different. You have only pointed out two differences. Of those, the mapping could be cleanly handled by an mdesc-> callback. (I would have to look at the match to see if that could be handled easily, but I would expect so.) On the other hand, in a previous reply you considered removing of_fdt_limit_memory(), which is only used for an exynos fixup. If you do that, then patch 1 disappears, and we can continue to sweep under the rug the side effects that you reminded me of with patch 1. > > Rob >
On Wed, Sep 5, 2018 at 4:10 PM Frank Rowand <frowand.list@gmail.com> wrote: > > On 09/05/18 13:06, Rob Herring wrote: > > On Wed, Sep 5, 2018 at 1:19 PM Frank Rowand <frowand.list@gmail.com> wrote: > >> > >> On 09/05/18 04:51, Rob Herring wrote: > >>> On Tue, Sep 4, 2018 at 8:49 PM Frank Rowand <frowand.list@gmail.com> wrote: > >>>> > >>>> On 08/30/18 12:05, Rob Herring wrote: > >>>>> Scan the root node properties (#{size,address}-cells) earlier, > >>>> > >>>> ^^^^^^^ > >>>> before mdesc->dt_fixup() is called > >>>> > >>>>> so that > >>>>> the dt_root_addr_cells and dt_root_size_cells variables are initialized > >>>>> and can be used. > >>>> by mdesc->dt_fixup() > >>> > >>> That's an ARM specific detail. Granted, ARM is the only caller. > >> > >> The dt_root_addr_cells and dt_root_size_cells variables are being > >> initialized earlier in this patch series so that of_fdt_limit_memory() > >> can use them. The only caller of of_fdt_limit_memory() is > >> exynos_dt_fixup(), which is an mdesc->dt_fixup() function. > >> > >> > >>> > >>>>> > >>>>> Cc: Frank Rowand <frowand.list@gmail.com> > >>>>> Signed-off-by: Rob Herring <robh@kernel.org> > >>>>> --- > >>>>> drivers/of/fdt.c | 7 ++++--- > >>>>> 1 file changed, 4 insertions(+), 3 deletions(-) > >>>>> > >>>> Moving early_init_dt_scan_root() to inside early_init_dt_verify() > >>>> puts something that has nothing to do with verifying the fdt > >>>> into a function whose purpose is the verify. It hides the side > >>>> effect of initializing the dt_root_addr_cells and dt_root_size_cells > >>>> variables. > >>> > >>> It already has the side effect of setting initial_boot_params which > >>> every subsequent function needs. > >> > >> And that side effect should probably also be moved. > > > > So 2 functions? One to set the blob and one to verify it. Then we can > > No, I would not add yet another function. All of these side effects are > an argument in favor of a single setup_machine_fdt(), as I suggested below. > Then all of these side effects could be in setup_machine_fdt() instead > of hiding them in sub-functions that are called by all of the different > architectures. > > > > just let arches decide if they want to do any verification or not. > > > > Perhaps it should be called fdt_init(blob) and then it is vague enough > > I can do whatever I want. > > > >>>> I suggest creating a new function early_init_dt_scan_init_pre_dt_fixup(), > >>>> move the chunk of code there instead of to early_init_dt_scan_nodes(), > >>>> and call the new function from setup_machine_fdt(), just before > >>>> calling mdesc->dt_fixup(). This would be a little bit more code, > >>>> but more clearly showing the intent. > >>> > >>> I'm trying to reduce the number of functions arches call > >> > >> I like that goal. > >> > >> > >>> and renaming > >>> would need a bunch of arch changes. This change will also let me make > >>> early_init_dt_scan_root private as powerpc is the only user. I need to > >>> dust off a patch for that. > >>> > >>> I'd be more inclined to push exynos to remove this altogether. After > >> > >> Not a bad idea. > >> > >>> all, if they claim their bindings are unstable, they can't really > >>> claim their bootloader is stable/fixed. > >> > >> It seems that this series is showing us that maybe the three architecture > >> specific (arc, arm, arm64) versions of setup_machine_fdt() should be > >> consolidated so that we have consistent behavior for FDT. > >> > >> If we had a single setup_machine_fdt() then some of he hidden side > >> effects of functions called by setup_machine_fdt() could instead > >> be hoisted into setup_machine_fdt(). > > > > Those functions are all quite a bit different. ARM matches the machine > > desc while arm64 doesn't have any such thing. How the DTB gets mapped > > into virtual space also varies. > > I argue that they _should be_ made to be more alike than different. You > have only pointed out two differences. Of those, the mapping could be > cleanly handled by an mdesc-> callback. (I would have to look at the > match to see if that could be handled easily, but I would expect so.) The machine desc is in no way common and only used on a few arches (and not even common across those arches). So there's no way the core DT code can just call a mdesc callback without addressing making that common first. And callbacks are just another way to call arch specific functions which are another thing I'm trying to remove. > On the other hand, in a previous reply you considered removing > of_fdt_limit_memory(), which is only used for an exynos fixup. If > you do that, then patch 1 disappears, and we can continue to > sweep under the rug the side effects that you reminded me of > with patch 1. I'm inclined to just drop the patch. Seemed like a simple clean-up and I'm not interested in doing more right now (did you look at the stack of stuff in dt/testing branch). Maybe someone else will care (spoiler: they won't). Rob
Hi Rob, On 09/05/18 14:31, Rob Herring wrote: > On Wed, Sep 5, 2018 at 4:10 PM Frank Rowand <frowand.list@gmail.com> wrote: >> >> On 09/05/18 13:06, Rob Herring wrote: >>> On Wed, Sep 5, 2018 at 1:19 PM Frank Rowand <frowand.list@gmail.com> wrote: >>>> >>>> On 09/05/18 04:51, Rob Herring wrote: >>>>> On Tue, Sep 4, 2018 at 8:49 PM Frank Rowand <frowand.list@gmail.com> wrote: >>>>>> >>>>>> On 08/30/18 12:05, Rob Herring wrote: >>>>>>> Scan the root node properties (#{size,address}-cells) earlier, >>>>>> >>>>>> ^^^^^^^ >>>>>> before mdesc->dt_fixup() is called >>>>>> >>>>>>> so that >>>>>>> the dt_root_addr_cells and dt_root_size_cells variables are initialized >>>>>>> and can be used. >>>>>> by mdesc->dt_fixup() >>>>> >>>>> That's an ARM specific detail. Granted, ARM is the only caller. >>>> >>>> The dt_root_addr_cells and dt_root_size_cells variables are being >>>> initialized earlier in this patch series so that of_fdt_limit_memory() >>>> can use them. The only caller of of_fdt_limit_memory() is >>>> exynos_dt_fixup(), which is an mdesc->dt_fixup() function. >>>> >>>> >>>>> >>>>>>> >>>>>>> Cc: Frank Rowand <frowand.list@gmail.com> >>>>>>> Signed-off-by: Rob Herring <robh@kernel.org> >>>>>>> --- >>>>>>> drivers/of/fdt.c | 7 ++++--- >>>>>>> 1 file changed, 4 insertions(+), 3 deletions(-) >>>>>>> >>>>>> Moving early_init_dt_scan_root() to inside early_init_dt_verify() >>>>>> puts something that has nothing to do with verifying the fdt >>>>>> into a function whose purpose is the verify. It hides the side >>>>>> effect of initializing the dt_root_addr_cells and dt_root_size_cells >>>>>> variables. >>>>> >>>>> It already has the side effect of setting initial_boot_params which >>>>> every subsequent function needs. >>>> >>>> And that side effect should probably also be moved. >>> >>> So 2 functions? One to set the blob and one to verify it. Then we can >> >> No, I would not add yet another function. All of these side effects are >> an argument in favor of a single setup_machine_fdt(), as I suggested below. >> Then all of these side effects could be in setup_machine_fdt() instead >> of hiding them in sub-functions that are called by all of the different >> architectures. >> >> >>> just let arches decide if they want to do any verification or not. >>> >>> Perhaps it should be called fdt_init(blob) and then it is vague enough >>> I can do whatever I want. >>> >>>>>> I suggest creating a new function early_init_dt_scan_init_pre_dt_fixup(), >>>>>> move the chunk of code there instead of to early_init_dt_scan_nodes(), >>>>>> and call the new function from setup_machine_fdt(), just before >>>>>> calling mdesc->dt_fixup(). This would be a little bit more code, >>>>>> but more clearly showing the intent. >>>>> >>>>> I'm trying to reduce the number of functions arches call >>>> >>>> I like that goal. >>>> >>>> >>>>> and renaming >>>>> would need a bunch of arch changes. This change will also let me make >>>>> early_init_dt_scan_root private as powerpc is the only user. I need to >>>>> dust off a patch for that. >>>>> >>>>> I'd be more inclined to push exynos to remove this altogether. After >>>> >>>> Not a bad idea. >>>> >>>>> all, if they claim their bindings are unstable, they can't really >>>>> claim their bootloader is stable/fixed. >>>> >>>> It seems that this series is showing us that maybe the three architecture >>>> specific (arc, arm, arm64) versions of setup_machine_fdt() should be >>>> consolidated so that we have consistent behavior for FDT. >>>> >>>> If we had a single setup_machine_fdt() then some of he hidden side >>>> effects of functions called by setup_machine_fdt() could instead >>>> be hoisted into setup_machine_fdt(). >>> >>> Those functions are all quite a bit different. ARM matches the machine >>> desc while arm64 doesn't have any such thing. How the DTB gets mapped >>> into virtual space also varies. >> >> I argue that they _should be_ made to be more alike than different. You >> have only pointed out two differences. Of those, the mapping could be >> cleanly handled by an mdesc-> callback. (I would have to look at the >> match to see if that could be handled easily, but I would expect so.) > > The machine desc is in no way common and only used on a few arches > (and not even common across those arches). So there's no way the core > DT code can just call a mdesc callback without addressing making that > common first. And callbacks are just another way to call arch specific > functions which are another thing I'm trying to remove. > >> On the other hand, in a previous reply you considered removing >> of_fdt_limit_memory(), which is only used for an exynos fixup. If >> you do that, then patch 1 disappears, and we can continue to >> sweep under the rug the side effects that you reminded me of >> with patch 1. > > I'm inclined to just drop the patch. Seemed like a simple clean-up and > I'm not interested in doing more right now (did you look at the stack > of stuff in dt/testing branch). Maybe someone else will care (spoiler: > they won't). I would agree with just dropping patch 1 and 2. Patch 3 is still fine. > > Rob >
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c index 800ad252cf9c..49abe18f1bde 100644 --- a/drivers/of/fdt.c +++ b/drivers/of/fdt.c @@ -1215,6 +1215,10 @@ bool __init early_init_dt_verify(void *params) initial_boot_params = params; of_fdt_crc32 = crc32_be(~0, initial_boot_params, fdt_totalsize(initial_boot_params)); + + /* Initialize {size,address}-cells info */ + of_scan_flat_dt(early_init_dt_scan_root, NULL); + return true; } @@ -1224,9 +1228,6 @@ void __init early_init_dt_scan_nodes(void) /* Retrieve various information from the /chosen node */ of_scan_flat_dt(early_init_dt_scan_chosen, boot_command_line); - /* Initialize {size,address}-cells info */ - of_scan_flat_dt(early_init_dt_scan_root, NULL); - /* Setup memory, calling early_init_dt_add_memory_arch */ of_scan_flat_dt(early_init_dt_scan_memory, NULL); }
Scan the root node properties (#{size,address}-cells) earlier, so that the dt_root_addr_cells and dt_root_size_cells variables are initialized and can be used. Cc: Frank Rowand <frowand.list@gmail.com> Signed-off-by: Rob Herring <robh@kernel.org> --- drivers/of/fdt.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)