| Message ID | ac37d27bda88fdf53f4c670f4457e9dca44447ea.1516094113.git-series.maxime.ripard@free-electrons.com |
|---|---|
| State | New |
| Headers | show |
| Series | env: Multiple env support and env transition for sunxi | expand |
Hi Maxime, On 16 January 2018 at 01:16, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > Allow boards and architectures to override the default environment lookup > code by overriding env_get_location. > > Reviewed-by: Andre Przywara <andre.przywara@arm.com> > Reviewed-by: Lukasz Majewski <lukma@denx.de> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > --- > env/env.c | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > I still don't really understand why this needs to be a weak function. If the board knows the priority order, can it not put it into global_data? We could have a little u8 array of 4 items with a terminator? > diff --git a/env/env.c b/env/env.c > index 75da2b921804..6d0ab8ebe1a4 100644 > --- a/env/env.c > +++ b/env/env.c > @@ -79,7 +79,25 @@ static void env_set_inited(enum env_location location) > > static enum env_location env_load_location; > > -static enum env_location env_get_location(enum env_operation op, int prio) > +/** > + * env_get_location() - Returns the best env location for a board > + * @op: operations performed on the environment > + * @prio: priority between the multiple environments, 0 being the > + * highest priority > + * > + * This will return the preferred environment for the given > + * priority. This is overridable by boards if they need to. > + * > + * All implementations are free to use the operation, the priority and > + * any other data relevant to their choice, but must take into account > + * the fact that the lowest prority (0) is the most important location > + * in the system. The following locations should be returned by order > + * of descending priorities, from the highest to the lowest priority. > + * > + * Returns: > + * an enum env_location value on success, a negative error code otherwise > + */ > +__weak enum env_location env_get_location(enum env_operation op, int prio) > { > switch (op) { > case ENVOP_GET_CHAR: > -- > git-series 0.9.1 Regards, Simon
Hi Simon, On Wed, Jan 17, 2018 at 03:07:58PM -0700, Simon Glass wrote: > On 16 January 2018 at 01:16, Maxime Ripard > <maxime.ripard@free-electrons.com> wrote: > > Allow boards and architectures to override the default environment lookup > > code by overriding env_get_location. > > > > Reviewed-by: Andre Przywara <andre.przywara@arm.com> > > Reviewed-by: Lukasz Majewski <lukma@denx.de> > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > --- > > env/env.c | 20 +++++++++++++++++++- > > 1 file changed, 19 insertions(+), 1 deletion(-) > > > > I still don't really understand why this needs to be a weak function. > If the board knows the priority order, can it not put it into > global_data? We could have a little u8 array of 4 items with a > terminator? Sure that would be simpler, but that would also prevent us from doing "smart" things based on data other than just whether the previous environment is usable. Things based for example on a GPIO state, or a custom algorithm to transition (or duplicate) the environment. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
Hi Maxime, On 18 January 2018 at 10:21, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > Hi Simon, > > On Wed, Jan 17, 2018 at 03:07:58PM -0700, Simon Glass wrote: >> On 16 January 2018 at 01:16, Maxime Ripard >> <maxime.ripard@free-electrons.com> wrote: >> > Allow boards and architectures to override the default environment lookup >> > code by overriding env_get_location. >> > >> > Reviewed-by: Andre Przywara <andre.przywara@arm.com> >> > Reviewed-by: Lukasz Majewski <lukma@denx.de> >> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> >> > --- >> > env/env.c | 20 +++++++++++++++++++- >> > 1 file changed, 19 insertions(+), 1 deletion(-) >> > >> >> I still don't really understand why this needs to be a weak function. >> If the board knows the priority order, can it not put it into >> global_data? We could have a little u8 array of 4 items with a >> terminator? > > Sure that would be simpler, but that would also prevent us from doing > "smart" things based on data other than just whether the previous > environment is usable. Things based for example on a GPIO state, or a > custom algorithm to transition (or duplicate) the environment. In that case the board could read the GPIO state, or the algorithm, and then set up the value. Basically I am saying it could set up the priority order in advance of it being needed, rather than having a callback. Regards, Simon
Hi, On Sun, Jan 21, 2018 at 05:29:56PM -0700, Simon Glass wrote: > > On Wed, Jan 17, 2018 at 03:07:58PM -0700, Simon Glass wrote: > >> On 16 January 2018 at 01:16, Maxime Ripard > >> <maxime.ripard@free-electrons.com> wrote: > >> > Allow boards and architectures to override the default environment lookup > >> > code by overriding env_get_location. > >> > > >> > Reviewed-by: Andre Przywara <andre.przywara@arm.com> > >> > Reviewed-by: Lukasz Majewski <lukma@denx.de> > >> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > >> > --- > >> > env/env.c | 20 +++++++++++++++++++- > >> > 1 file changed, 19 insertions(+), 1 deletion(-) > >> > > >> > >> I still don't really understand why this needs to be a weak function. > >> If the board knows the priority order, can it not put it into > >> global_data? We could have a little u8 array of 4 items with a > >> terminator? > > > > Sure that would be simpler, but that would also prevent us from doing > > "smart" things based on data other than just whether the previous > > environment is usable. Things based for example on a GPIO state, or a > > custom algorithm to transition (or duplicate) the environment. > > In that case the board could read the GPIO state, or the algorithm, > and then set up the value. > > Basically I am saying it could set up the priority order in advance of > it being needed, rather than having a callback. Aren't we kind of stuck here? On the previous iterations, we already discussed this and Tom eventually told he was in favour of __weak functions, and the discussion stopped there. I assumed you were ok with it. I'd really want to move forward on that. This is something that is really biting us *now* and I'd hate to miss yet another merge window because of debates like this. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
On Mon, Jan 22, 2018 at 01:46:46PM +0100, Maxime Ripard wrote: > Hi, > > On Sun, Jan 21, 2018 at 05:29:56PM -0700, Simon Glass wrote: > > > On Wed, Jan 17, 2018 at 03:07:58PM -0700, Simon Glass wrote: > > >> On 16 January 2018 at 01:16, Maxime Ripard > > >> <maxime.ripard@free-electrons.com> wrote: > > >> > Allow boards and architectures to override the default environment lookup > > >> > code by overriding env_get_location. > > >> > > > >> > Reviewed-by: Andre Przywara <andre.przywara@arm.com> > > >> > Reviewed-by: Lukasz Majewski <lukma@denx.de> > > >> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > >> > --- > > >> > env/env.c | 20 +++++++++++++++++++- > > >> > 1 file changed, 19 insertions(+), 1 deletion(-) > > >> > > > >> > > >> I still don't really understand why this needs to be a weak function. > > >> If the board knows the priority order, can it not put it into > > >> global_data? We could have a little u8 array of 4 items with a > > >> terminator? > > > > > > Sure that would be simpler, but that would also prevent us from doing > > > "smart" things based on data other than just whether the previous > > > environment is usable. Things based for example on a GPIO state, or a > > > custom algorithm to transition (or duplicate) the environment. > > > > In that case the board could read the GPIO state, or the algorithm, > > and then set up the value. > > > > Basically I am saying it could set up the priority order in advance of > > it being needed, rather than having a callback. > > Aren't we kind of stuck here? > > On the previous iterations, we already discussed this and Tom > eventually told he was in favour of __weak functions, and the > discussion stopped there. I assumed you were ok with it. > > I'd really want to move forward on that. This is something that is > really biting us *now* and I'd hate to miss yet another merge window > because of debates like this. Yes, I think this is where we want things to be weak, with a reasonable default. If we start to see that "everyone" does the same thing by and large we can re-evaluate. -- Tom
On Mon, Jan 22, 2018 at 07:49:41AM -0500, Tom Rini wrote: > On Mon, Jan 22, 2018 at 01:46:46PM +0100, Maxime Ripard wrote: > > Hi, > > > > On Sun, Jan 21, 2018 at 05:29:56PM -0700, Simon Glass wrote: > > > > On Wed, Jan 17, 2018 at 03:07:58PM -0700, Simon Glass wrote: > > > >> On 16 January 2018 at 01:16, Maxime Ripard > > > >> <maxime.ripard@free-electrons.com> wrote: > > > >> > Allow boards and architectures to override the default environment lookup > > > >> > code by overriding env_get_location. > > > >> > > > > >> > Reviewed-by: Andre Przywara <andre.przywara@arm.com> > > > >> > Reviewed-by: Lukasz Majewski <lukma@denx.de> > > > >> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > > >> > --- > > > >> > env/env.c | 20 +++++++++++++++++++- > > > >> > 1 file changed, 19 insertions(+), 1 deletion(-) > > > >> > > > > >> > > > >> I still don't really understand why this needs to be a weak function. > > > >> If the board knows the priority order, can it not put it into > > > >> global_data? We could have a little u8 array of 4 items with a > > > >> terminator? > > > > > > > > Sure that would be simpler, but that would also prevent us from doing > > > > "smart" things based on data other than just whether the previous > > > > environment is usable. Things based for example on a GPIO state, or a > > > > custom algorithm to transition (or duplicate) the environment. > > > > > > In that case the board could read the GPIO state, or the algorithm, > > > and then set up the value. > > > > > > Basically I am saying it could set up the priority order in advance of > > > it being needed, rather than having a callback. > > > > Aren't we kind of stuck here? > > > > On the previous iterations, we already discussed this and Tom > > eventually told he was in favour of __weak functions, and the > > discussion stopped there. I assumed you were ok with it. > > > > I'd really want to move forward on that. This is something that is > > really biting us *now* and I'd hate to miss yet another merge window > > because of debates like this. > > Yes, I think this is where we want things to be weak, with a reasonable > default. If we start to see that "everyone" does the same thing by and > large we can re-evaluate. Ok. I've fixed the bug I mentionned the other day on IRC, should I send a PR? Thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
On Mon, Jan 22, 2018 at 04:57:41PM +0100, Maxime Ripard wrote: > On Mon, Jan 22, 2018 at 07:49:41AM -0500, Tom Rini wrote: > > On Mon, Jan 22, 2018 at 01:46:46PM +0100, Maxime Ripard wrote: > > > Hi, > > > > > > On Sun, Jan 21, 2018 at 05:29:56PM -0700, Simon Glass wrote: > > > > > On Wed, Jan 17, 2018 at 03:07:58PM -0700, Simon Glass wrote: > > > > >> On 16 January 2018 at 01:16, Maxime Ripard > > > > >> <maxime.ripard@free-electrons.com> wrote: > > > > >> > Allow boards and architectures to override the default environment lookup > > > > >> > code by overriding env_get_location. > > > > >> > > > > > >> > Reviewed-by: Andre Przywara <andre.przywara@arm.com> > > > > >> > Reviewed-by: Lukasz Majewski <lukma@denx.de> > > > > >> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > > > >> > --- > > > > >> > env/env.c | 20 +++++++++++++++++++- > > > > >> > 1 file changed, 19 insertions(+), 1 deletion(-) > > > > >> > > > > > >> > > > > >> I still don't really understand why this needs to be a weak function. > > > > >> If the board knows the priority order, can it not put it into > > > > >> global_data? We could have a little u8 array of 4 items with a > > > > >> terminator? > > > > > > > > > > Sure that would be simpler, but that would also prevent us from doing > > > > > "smart" things based on data other than just whether the previous > > > > > environment is usable. Things based for example on a GPIO state, or a > > > > > custom algorithm to transition (or duplicate) the environment. > > > > > > > > In that case the board could read the GPIO state, or the algorithm, > > > > and then set up the value. > > > > > > > > Basically I am saying it could set up the priority order in advance of > > > > it being needed, rather than having a callback. > > > > > > Aren't we kind of stuck here? > > > > > > On the previous iterations, we already discussed this and Tom > > > eventually told he was in favour of __weak functions, and the > > > discussion stopped there. I assumed you were ok with it. > > > > > > I'd really want to move forward on that. This is something that is > > > really biting us *now* and I'd hate to miss yet another merge window > > > because of debates like this. > > > > Yes, I think this is where we want things to be weak, with a reasonable > > default. If we start to see that "everyone" does the same thing by and > > large we can re-evaluate. > > Ok. > > I've fixed the bug I mentionned the other day on IRC, should I send a PR? Lets give Simon a chance to provide any other feedback here, or another argument to convince me that no, we don't want to have this abstracted by a weak function but instead ..., thanks! -- Tom
Hi, On 22 January 2018 at 09:36, Tom Rini <trini@konsulko.com> wrote: > On Mon, Jan 22, 2018 at 04:57:41PM +0100, Maxime Ripard wrote: >> On Mon, Jan 22, 2018 at 07:49:41AM -0500, Tom Rini wrote: >> > On Mon, Jan 22, 2018 at 01:46:46PM +0100, Maxime Ripard wrote: >> > > Hi, >> > > >> > > On Sun, Jan 21, 2018 at 05:29:56PM -0700, Simon Glass wrote: >> > > > > On Wed, Jan 17, 2018 at 03:07:58PM -0700, Simon Glass wrote: >> > > > >> On 16 January 2018 at 01:16, Maxime Ripard >> > > > >> <maxime.ripard@free-electrons.com> wrote: >> > > > >> > Allow boards and architectures to override the default environment lookup >> > > > >> > code by overriding env_get_location. >> > > > >> > >> > > > >> > Reviewed-by: Andre Przywara <andre.przywara@arm.com> >> > > > >> > Reviewed-by: Lukasz Majewski <lukma@denx.de> >> > > > >> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> >> > > > >> > --- >> > > > >> > env/env.c | 20 +++++++++++++++++++- >> > > > >> > 1 file changed, 19 insertions(+), 1 deletion(-) >> > > > >> > >> > > > >> >> > > > >> I still don't really understand why this needs to be a weak function. >> > > > >> If the board knows the priority order, can it not put it into >> > > > >> global_data? We could have a little u8 array of 4 items with a >> > > > >> terminator? >> > > > > >> > > > > Sure that would be simpler, but that would also prevent us from doing >> > > > > "smart" things based on data other than just whether the previous >> > > > > environment is usable. Things based for example on a GPIO state, or a >> > > > > custom algorithm to transition (or duplicate) the environment. >> > > > >> > > > In that case the board could read the GPIO state, or the algorithm, >> > > > and then set up the value. >> > > > >> > > > Basically I am saying it could set up the priority order in advance of >> > > > it being needed, rather than having a callback. >> > > >> > > Aren't we kind of stuck here? >> > > >> > > On the previous iterations, we already discussed this and Tom >> > > eventually told he was in favour of __weak functions, and the >> > > discussion stopped there. I assumed you were ok with it. >> > > >> > > I'd really want to move forward on that. This is something that is >> > > really biting us *now* and I'd hate to miss yet another merge window >> > > because of debates like this. >> > >> > Yes, I think this is where we want things to be weak, with a reasonable >> > default. If we start to see that "everyone" does the same thing by and >> > large we can re-evaluate. >> >> Ok. >> >> I've fixed the bug I mentionned the other day on IRC, should I send a PR? > > Lets give Simon a chance to provide any other feedback here, or another > argument to convince me that no, we don't want to have this abstracted > by a weak function but instead ..., thanks! I suspect there is a reason why this is better than what I propose. Perhaps when I try it out it will become apparent. So let's go ahead and revisit later if we have new information. Reviewed-by: Simon Glass <sjg@chromium.org> Regards, Simon
On Mon, Jan 22, 2018 at 09:48:26AM -0700, Simon Glass wrote: > Hi, > > On 22 January 2018 at 09:36, Tom Rini <trini@konsulko.com> wrote: > > On Mon, Jan 22, 2018 at 04:57:41PM +0100, Maxime Ripard wrote: > >> On Mon, Jan 22, 2018 at 07:49:41AM -0500, Tom Rini wrote: > >> > On Mon, Jan 22, 2018 at 01:46:46PM +0100, Maxime Ripard wrote: > >> > > Hi, > >> > > > >> > > On Sun, Jan 21, 2018 at 05:29:56PM -0700, Simon Glass wrote: > >> > > > > On Wed, Jan 17, 2018 at 03:07:58PM -0700, Simon Glass wrote: > >> > > > >> On 16 January 2018 at 01:16, Maxime Ripard > >> > > > >> <maxime.ripard@free-electrons.com> wrote: > >> > > > >> > Allow boards and architectures to override the default environment lookup > >> > > > >> > code by overriding env_get_location. > >> > > > >> > > >> > > > >> > Reviewed-by: Andre Przywara <andre.przywara@arm.com> > >> > > > >> > Reviewed-by: Lukasz Majewski <lukma@denx.de> > >> > > > >> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > >> > > > >> > --- > >> > > > >> > env/env.c | 20 +++++++++++++++++++- > >> > > > >> > 1 file changed, 19 insertions(+), 1 deletion(-) > >> > > > >> > > >> > > > >> > >> > > > >> I still don't really understand why this needs to be a weak function. > >> > > > >> If the board knows the priority order, can it not put it into > >> > > > >> global_data? We could have a little u8 array of 4 items with a > >> > > > >> terminator? > >> > > > > > >> > > > > Sure that would be simpler, but that would also prevent us from doing > >> > > > > "smart" things based on data other than just whether the previous > >> > > > > environment is usable. Things based for example on a GPIO state, or a > >> > > > > custom algorithm to transition (or duplicate) the environment. > >> > > > > >> > > > In that case the board could read the GPIO state, or the algorithm, > >> > > > and then set up the value. > >> > > > > >> > > > Basically I am saying it could set up the priority order in advance of > >> > > > it being needed, rather than having a callback. > >> > > > >> > > Aren't we kind of stuck here? > >> > > > >> > > On the previous iterations, we already discussed this and Tom > >> > > eventually told he was in favour of __weak functions, and the > >> > > discussion stopped there. I assumed you were ok with it. > >> > > > >> > > I'd really want to move forward on that. This is something that is > >> > > really biting us *now* and I'd hate to miss yet another merge window > >> > > because of debates like this. > >> > > >> > Yes, I think this is where we want things to be weak, with a reasonable > >> > default. If we start to see that "everyone" does the same thing by and > >> > large we can re-evaluate. > >> > >> Ok. > >> > >> I've fixed the bug I mentionned the other day on IRC, should I send a PR? > > > > Lets give Simon a chance to provide any other feedback here, or another > > argument to convince me that no, we don't want to have this abstracted > > by a weak function but instead ..., thanks! > > I suspect there is a reason why this is better than what I propose. > Perhaps when I try it out it will become apparent. > > So let's go ahead and revisit later if we have new information. > > Reviewed-by: Simon Glass <sjg@chromium.org> Thanks! Maxime, please re-spin with the bugfix (or wait another day or two for other feedback), repost and I'll take it in Thurs/Fri or so. -- Tom
diff --git a/env/env.c b/env/env.c index 75da2b921804..6d0ab8ebe1a4 100644 --- a/env/env.c +++ b/env/env.c @@ -79,7 +79,25 @@ static void env_set_inited(enum env_location location) static enum env_location env_load_location; -static enum env_location env_get_location(enum env_operation op, int prio) +/** + * env_get_location() - Returns the best env location for a board + * @op: operations performed on the environment + * @prio: priority between the multiple environments, 0 being the + * highest priority + * + * This will return the preferred environment for the given + * priority. This is overridable by boards if they need to. + * + * All implementations are free to use the operation, the priority and + * any other data relevant to their choice, but must take into account + * the fact that the lowest prority (0) is the most important location + * in the system. The following locations should be returned by order + * of descending priorities, from the highest to the lowest priority. + * + * Returns: + * an enum env_location value on success, a negative error code otherwise + */ +__weak enum env_location env_get_location(enum env_operation op, int prio) { switch (op) { case ENVOP_GET_CHAR: