| Message ID | 20230602004824.20731-5-vikram.garhwal@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dynamic node programming using overlay dtbo | expand |
On 02/06/2023 02:48, Vikram Garhwal wrote: > Following changes are done to __unflatten_device_tree(): > 1. __unflatten_device_tree() is renamed to unflatten_device_tree(). > 2. Remove __init and static function type. > > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> > Reviewed-by: Henry Wang <Henry.Wang@arm.com> Reviewed-by: Michal Orzel <michal.orzel@amd.com> ~Michal
Hi, Title: 'type' is a bit confusing here. How about "Export __unflatten_device_tre()"? On 02/06/2023 01:48, Vikram Garhwal wrote: > Following changes are done to __unflatten_device_tree(): > 1. __unflatten_device_tree() is renamed to unflatten_device_tree(). > 2. Remove __init and static function type. As there is no external caller yet, please explain why you want to export the function. Cheers, > > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> > Reviewed-by: Henry Wang <Henry.Wang@arm.com> > --- > xen/common/device_tree.c | 9 ++++----- > xen/include/xen/device_tree.h | 5 +++++ > 2 files changed, 9 insertions(+), 5 deletions(-) > > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c > index bbdab07596..16b4b4e946 100644 > --- a/xen/common/device_tree.c > +++ b/xen/common/device_tree.c > @@ -2083,7 +2083,7 @@ static unsigned long unflatten_dt_node(const void *fdt, > } > > /** > - * __unflatten_device_tree - create tree of device_nodes from flat blob > + * unflatten_device_tree - create tree of device_nodes from flat blob > * > * unflattens a device-tree, creating the > * tree of struct device_node. It also fills the "name" and "type" > @@ -2092,8 +2092,7 @@ static unsigned long unflatten_dt_node(const void *fdt, > * @fdt: The fdt to expand > * @mynodes: The device_node tree created by the call > */ > -static int __init __unflatten_device_tree(const void *fdt, > - struct dt_device_node **mynodes) > +int unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes) > { > unsigned long start, mem, size; > struct dt_device_node **allnextp = mynodes; > @@ -2230,10 +2229,10 @@ dt_find_interrupt_controller(const struct dt_device_match *matches) > > void __init dt_unflatten_host_device_tree(void) > { > - int error = __unflatten_device_tree(device_tree_flattened, &dt_host); > + int error = unflatten_device_tree(device_tree_flattened, &dt_host); > > if ( error ) > - panic("__unflatten_device_tree failed with error %d\n", error); > + panic("unflatten_device_tree failed with error %d\n", error); > > dt_alias_scan(); This function doesn't seem to be called in the case of the overlay device-tree. Does this mean that it will never contain any alias? > } > diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h > index c2eada7489..2c35c0d391 100644 > --- a/xen/include/xen/device_tree.h > +++ b/xen/include/xen/device_tree.h > @@ -178,6 +178,11 @@ int device_tree_for_each_node(const void *fdt, int node, > */ > void dt_unflatten_host_device_tree(void); > > +/** > + * unflatten any device tree. Most of the exported function in device_tre.h have documentation. Can you do the same here? > + */ > +int unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes); NIT: From an external interface perspective, do we actually need to pass an extra pointer? Can't we instead, return the pointer? > + > /** > * IRQ translation callback > * TODO: For the moment we assume that we only have ONE Cheers,
Hi Julien, Will update the commit message regarding why we need to export this for dtbo programming. On 6/5/23 12:04 PM, Julien Grall wrote: > Hi, > > Title: > > 'type' is a bit confusing here. How about "Export > __unflatten_device_tre()"? > > On 02/06/2023 01:48, Vikram Garhwal wrote: >> Following changes are done to __unflatten_device_tree(): >> 1. __unflatten_device_tree() is renamed to unflatten_device_tree(). >> 2. Remove __init and static function type. > > As there is no external caller yet, please explain why you want to > export the function. > > Cheers, > >> >> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> >> Reviewed-by: Henry Wang <Henry.Wang@arm.com> >> --- >> xen/common/device_tree.c | 9 ++++----- >> xen/include/xen/device_tree.h | 5 +++++ >> 2 files changed, 9 insertions(+), 5 deletions(-) >> >> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c >> index bbdab07596..16b4b4e946 100644 >> --- a/xen/common/device_tree.c >> +++ b/xen/common/device_tree.c >> @@ -2083,7 +2083,7 @@ static unsigned long unflatten_dt_node(const >> void *fdt, >> } >> /** >> - * __unflatten_device_tree - create tree of device_nodes from flat blob >> + * unflatten_device_tree - create tree of device_nodes from flat blob >> * >> * unflattens a device-tree, creating the >> * tree of struct device_node. It also fills the "name" and "type" >> @@ -2092,8 +2092,7 @@ static unsigned long unflatten_dt_node(const >> void *fdt, >> * @fdt: The fdt to expand >> * @mynodes: The device_node tree created by the call >> */ >> -static int __init __unflatten_device_tree(const void *fdt, >> - struct dt_device_node >> **mynodes) >> +int unflatten_device_tree(const void *fdt, struct dt_device_node >> **mynodes) >> { >> unsigned long start, mem, size; >> struct dt_device_node **allnextp = mynodes; >> @@ -2230,10 +2229,10 @@ dt_find_interrupt_controller(const struct >> dt_device_match *matches) >> void __init dt_unflatten_host_device_tree(void) >> { >> - int error = __unflatten_device_tree(device_tree_flattened, >> &dt_host); >> + int error = unflatten_device_tree(device_tree_flattened, &dt_host); >> if ( error ) >> - panic("__unflatten_device_tree failed with error %d\n", error); >> + panic("unflatten_device_tree failed with error %d\n", error); >> dt_alias_scan(); > > This function doesn't seem to be called in the case of the overlay > device-tree. Does this mean that it will never contain any alias? > >> } >> diff --git a/xen/include/xen/device_tree.h >> b/xen/include/xen/device_tree.h >> index c2eada7489..2c35c0d391 100644 >> --- a/xen/include/xen/device_tree.h >> +++ b/xen/include/xen/device_tree.h >> @@ -178,6 +178,11 @@ int device_tree_for_each_node(const void *fdt, >> int node, >> */ >> void dt_unflatten_host_device_tree(void); >> +/** >> + * unflatten any device tree. > > Most of the exported function in device_tre.h have documentation. Can > you do the same here? > >> + */ >> +int unflatten_device_tree(const void *fdt, struct dt_device_node >> **mynodes); > > NIT: From an external interface perspective, do we actually need to > pass an extra pointer? Can't we instead, return the pointer? > >> + >> /** >> * IRQ translation callback >> * TODO: For the moment we assume that we only have ONE > > Cheers, >
On Tue, Jun 06, 2023 at 12:09:35PM -0700, Vikram Garhwal wrote: > Hi Julien, > Will update the commit message regarding why we need to export this for dtbo > programming. > > On 6/5/23 12:04 PM, Julien Grall wrote: > > Hi, > > > > Title: > > > > 'type' is a bit confusing here. How about "Export > > __unflatten_device_tre()"? > > > > On 02/06/2023 01:48, Vikram Garhwal wrote: > > > Following changes are done to __unflatten_device_tree(): > > > 1. __unflatten_device_tree() is renamed to unflatten_device_tree(). > > > 2. Remove __init and static function type. > > > > As there is no external caller yet, please explain why you want to > > export the function. Update the commit message in v8. > > > > Cheers, > > > > > > > > Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> > > > Reviewed-by: Henry Wang <Henry.Wang@arm.com> > > > --- > > > xen/common/device_tree.c | 9 ++++----- > > > xen/include/xen/device_tree.h | 5 +++++ > > > 2 files changed, 9 insertions(+), 5 deletions(-) > > > > > > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c > > > index bbdab07596..16b4b4e946 100644 > > > --- a/xen/common/device_tree.c > > > +++ b/xen/common/device_tree.c > > > @@ -2083,7 +2083,7 @@ static unsigned long unflatten_dt_node(const > > > void *fdt, > > > } > > > /** > > > - * __unflatten_device_tree - create tree of device_nodes from flat blob > > > + * unflatten_device_tree - create tree of device_nodes from flat blob > > > * > > > * unflattens a device-tree, creating the > > > * tree of struct device_node. It also fills the "name" and "type" > > > @@ -2092,8 +2092,7 @@ static unsigned long unflatten_dt_node(const > > > void *fdt, > > > * @fdt: The fdt to expand > > > * @mynodes: The device_node tree created by the call > > > */ > > > -static int __init __unflatten_device_tree(const void *fdt, > > > - struct dt_device_node > > > **mynodes) > > > +int unflatten_device_tree(const void *fdt, struct dt_device_node > > > **mynodes) > > > { > > > unsigned long start, mem, size; > > > struct dt_device_node **allnextp = mynodes; > > > @@ -2230,10 +2229,10 @@ dt_find_interrupt_controller(const struct > > > dt_device_match *matches) > > > void __init dt_unflatten_host_device_tree(void) > > > { > > > - int error = __unflatten_device_tree(device_tree_flattened, > > > &dt_host); > > > + int error = unflatten_device_tree(device_tree_flattened, &dt_host); > > > if ( error ) > > > - panic("__unflatten_device_tree failed with error %d\n", error); > > > + panic("unflatten_device_tree failed with error %d\n", error); > > > dt_alias_scan(); > > > > This function doesn't seem to be called in the case of the overlay > > device-tree. Does this mean that it will never contain any alias? > > I haven't seen any overlay example for FPGA use cases where alias are added. I have added a TODO in patch 16/19 where we are calling unflatten_device_tree(). > > > } > > > diff --git a/xen/include/xen/device_tree.h > > > b/xen/include/xen/device_tree.h > > > index c2eada7489..2c35c0d391 100644 > > > --- a/xen/include/xen/device_tree.h > > > +++ b/xen/include/xen/device_tree.h > > > @@ -178,6 +178,11 @@ int device_tree_for_each_node(const void *fdt, > > > int node, > > > */ > > > void dt_unflatten_host_device_tree(void); > > > +/** > > > + * unflatten any device tree. > > > > Most of the exported function in device_tre.h have documentation. Can > > you do the same here? Done! > > > > > + */ > > > +int unflatten_device_tree(const void *fdt, struct dt_device_node > > > **mynodes); > > > > NIT: From an external interface perspective, do we actually need to pass > > an extra pointer? Can't we instead, return the pointer? We will also need the error from the function. So, that's why i kept it as it is. Please review v8. I will send it in few hours. > > > > > + > > > /** > > > * IRQ translation callback > > > * TODO: For the moment we assume that we only have ONE > > > > Cheers, > > >
Hi, On 17/08/2023 00:49, Vikram Garhwal wrote: > On Tue, Jun 06, 2023 at 12:09:35PM -0700, Vikram Garhwal wrote: >> Hi Julien, >> Will update the commit message regarding why we need to export this for dtbo >> programming. >> >> On 6/5/23 12:04 PM, Julien Grall wrote: >>> Hi, >>> >>> Title: >>> >>> 'type' is a bit confusing here. How about "Export >>> __unflatten_device_tre()"? >>> >>> On 02/06/2023 01:48, Vikram Garhwal wrote: >>>> Following changes are done to __unflatten_device_tree(): >>>> 1. __unflatten_device_tree() is renamed to unflatten_device_tree(). >>>> 2. Remove __init and static function type. >>> >>> As there is no external caller yet, please explain why you want to >>> export the function. > Update the commit message in v8. >>> >>> Cheers, >>> >>>> >>>> Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> >>>> Reviewed-by: Henry Wang <Henry.Wang@arm.com> >>>> --- >>>> xen/common/device_tree.c | 9 ++++----- >>>> xen/include/xen/device_tree.h | 5 +++++ >>>> 2 files changed, 9 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c >>>> index bbdab07596..16b4b4e946 100644 >>>> --- a/xen/common/device_tree.c >>>> +++ b/xen/common/device_tree.c >>>> @@ -2083,7 +2083,7 @@ static unsigned long unflatten_dt_node(const >>>> void *fdt, >>>> } >>>> /** >>>> - * __unflatten_device_tree - create tree of device_nodes from flat blob >>>> + * unflatten_device_tree - create tree of device_nodes from flat blob >>>> * >>>> * unflattens a device-tree, creating the >>>> * tree of struct device_node. It also fills the "name" and "type" >>>> @@ -2092,8 +2092,7 @@ static unsigned long unflatten_dt_node(const >>>> void *fdt, >>>> * @fdt: The fdt to expand >>>> * @mynodes: The device_node tree created by the call >>>> */ >>>> -static int __init __unflatten_device_tree(const void *fdt, >>>> - struct dt_device_node >>>> **mynodes) >>>> +int unflatten_device_tree(const void *fdt, struct dt_device_node >>>> **mynodes) >>>> { >>>> unsigned long start, mem, size; >>>> struct dt_device_node **allnextp = mynodes; >>>> @@ -2230,10 +2229,10 @@ dt_find_interrupt_controller(const struct >>>> dt_device_match *matches) >>>> void __init dt_unflatten_host_device_tree(void) >>>> { >>>> - int error = __unflatten_device_tree(device_tree_flattened, >>>> &dt_host); >>>> + int error = unflatten_device_tree(device_tree_flattened, &dt_host); >>>> if ( error ) >>>> - panic("__unflatten_device_tree failed with error %d\n", error); >>>> + panic("unflatten_device_tree failed with error %d\n", error); >>>> dt_alias_scan(); >>> >>> This function doesn't seem to be called in the case of the overlay >>> device-tree. Does this mean that it will never contain any alias? >>> > I haven't seen any overlay example for FPGA use cases where alias are added. > I have added a TODO in patch 16/19 where we are calling unflatten_device_tree(). >>>> } >>>> diff --git a/xen/include/xen/device_tree.h >>>> b/xen/include/xen/device_tree.h >>>> index c2eada7489..2c35c0d391 100644 >>>> --- a/xen/include/xen/device_tree.h >>>> +++ b/xen/include/xen/device_tree.h >>>> @@ -178,6 +178,11 @@ int device_tree_for_each_node(const void *fdt, >>>> int node, >>>> */ >>>> void dt_unflatten_host_device_tree(void); >>>> +/** >>>> + * unflatten any device tree. >>> >>> Most of the exported function in device_tre.h have documentation. Can >>> you do the same here? > Done! >>> >>>> + */ >>>> +int unflatten_device_tree(const void *fdt, struct dt_device_node >>>> **mynodes); >>> >>> NIT: From an external interface perspective, do we actually need to pass >>> an extra pointer? Can't we instead, return the pointer? > We will also need the error from the function. So, that's why i kept it as it is. This can be achieved by using the ERR_PTR() infrastructure which I would rather prefer over passing an extra pointer here. Cheers,
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c index bbdab07596..16b4b4e946 100644 --- a/xen/common/device_tree.c +++ b/xen/common/device_tree.c @@ -2083,7 +2083,7 @@ static unsigned long unflatten_dt_node(const void *fdt, } /** - * __unflatten_device_tree - create tree of device_nodes from flat blob + * unflatten_device_tree - create tree of device_nodes from flat blob * * unflattens a device-tree, creating the * tree of struct device_node. It also fills the "name" and "type" @@ -2092,8 +2092,7 @@ static unsigned long unflatten_dt_node(const void *fdt, * @fdt: The fdt to expand * @mynodes: The device_node tree created by the call */ -static int __init __unflatten_device_tree(const void *fdt, - struct dt_device_node **mynodes) +int unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes) { unsigned long start, mem, size; struct dt_device_node **allnextp = mynodes; @@ -2230,10 +2229,10 @@ dt_find_interrupt_controller(const struct dt_device_match *matches) void __init dt_unflatten_host_device_tree(void) { - int error = __unflatten_device_tree(device_tree_flattened, &dt_host); + int error = unflatten_device_tree(device_tree_flattened, &dt_host); if ( error ) - panic("__unflatten_device_tree failed with error %d\n", error); + panic("unflatten_device_tree failed with error %d\n", error); dt_alias_scan(); } diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h index c2eada7489..2c35c0d391 100644 --- a/xen/include/xen/device_tree.h +++ b/xen/include/xen/device_tree.h @@ -178,6 +178,11 @@ int device_tree_for_each_node(const void *fdt, int node, */ void dt_unflatten_host_device_tree(void); +/** + * unflatten any device tree. + */ +int unflatten_device_tree(const void *fdt, struct dt_device_node **mynodes); + /** * IRQ translation callback * TODO: For the moment we assume that we only have ONE
Following changes are done to __unflatten_device_tree(): 1. __unflatten_device_tree() is renamed to unflatten_device_tree(). 2. Remove __init and static function type. Signed-off-by: Vikram Garhwal <vikram.garhwal@amd.com> Reviewed-by: Henry Wang <Henry.Wang@arm.com> --- xen/common/device_tree.c | 9 ++++----- xen/include/xen/device_tree.h | 5 +++++ 2 files changed, 9 insertions(+), 5 deletions(-)