| Message ID | 1360853581-9756-1-git-send-email-pdeschrijver@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/14/2013 07:52 AM, Peter De Schrijver wrote: > tegra_boot_secondary() relies on some of the car ops. This means having an > uninitialized tegra_cpu_car_ops will lead to an early boot panic. > Providing a dummy struct avoids this and makes adding Tegra114 clock support > in a bisectable way a lot easier. > > -- > > Stephen, > > Should this be a separate patch or should I make this part of new release of > the Tegra114 clock series? I will try and remember to apply this before the Tegra114 clock series, although I think you're reposting that anyway, so feel free to include this in that series to make things simpler.
> Subject: [PATCH] clk: tegra: provide dummy cpu car ops > > tegra_boot_secondary() relies on some of the car ops. This means having an > uninitialized tegra_cpu_car_ops will lead to an early boot panic. > Providing a dummy struct avoids this and makes adding Tegra114 clock > support in a bisectable way a lot easier. > > -- > > Stephen, > > Should this be a separate patch or should I make this part of new release of > the Tegra114 clock series? > > Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com> > --- > drivers/clk/tegra/clk.c | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c index > a603b9a..f6c141f 100644 > --- a/drivers/clk/tegra/clk.c > +++ b/drivers/clk/tegra/clk.c > @@ -22,7 +22,8 @@ > #include "clk.h" > > /* Global data of Tegra CPU CAR ops */ > -struct tegra_cpu_car_ops *tegra_cpu_car_ops; Sorry for bringing this up so late... Shouldn't the above be "struct tegra_cpu_car_ops tegra_cpu_car_ops;"? > +static struct tegra_cpu_car_ops *dummy_car_ops; struct > +tegra_cpu_car_ops *tegra_cpu_car_ops = &dummy_car_ops; > > void __init tegra_init_dup_clks(struct tegra_clk_duplicate *dup_list, > struct clk *clks[], int clk_max) > -- > 1.7.7.rc0.72.g4b5ea.dirty > > -- > To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the > body of a message to majordomo@vger.kernel.org More majordomo info at > http://vger.kernel.org/majordomo-info.html
On 03/06/2013 04:20 PM, Andrew Chew wrote: >> Subject: [PATCH] clk: tegra: provide dummy cpu car ops >> >> tegra_boot_secondary() relies on some of the car ops. This means having an >> uninitialized tegra_cpu_car_ops will lead to an early boot panic. >> Providing a dummy struct avoids this and makes adding Tegra114 clock >> support in a bisectable way a lot easier. >> >> -- >> >> Stephen, >> >> Should this be a separate patch or should I make this part of new release of >> the Tegra114 clock series? I'm not sure if I answered this. Peter, I intend to apply this patch to a branch right before the CCF, so there's no explicit need to include it in the series, although if you do, that's fine. >> diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c index >> /* Global data of Tegra CPU CAR ops */ >> -struct tegra_cpu_car_ops *tegra_cpu_car_ops; > > Sorry for bringing this up so late... > Shouldn't the above be "struct tegra_cpu_car_ops tegra_cpu_car_ops;"? > >> +static struct tegra_cpu_car_ops *dummy_car_ops; struct >> +tegra_cpu_car_ops *tegra_cpu_car_ops = &dummy_car_ops; No, the value is used as a pointer in include/linux/clk/tegra.h, e.g.: tegra_cpu_car_ops->wait_for_reset(cpu);
> From: linux-tegra-owner@vger.kernel.org [mailto:linux-tegra- > owner@vger.kernel.org] On Behalf Of Stephen Warren > Sent: Wednesday, March 06, 2013 3:43 PM > To: Andrew Chew > Cc: Peter De Schrijver; linux-tegra@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; Stephen Warren; Prashant Gaikwad; Mike > Turquette; linux-kernel@vger.kernel.org > Subject: Re: [PATCH] clk: tegra: provide dummy cpu car ops > > On 03/06/2013 04:20 PM, Andrew Chew wrote: > >> Subject: [PATCH] clk: tegra: provide dummy cpu car ops > >> > >> tegra_boot_secondary() relies on some of the car ops. This means > >> having an uninitialized tegra_cpu_car_ops will lead to an early boot panic. > >> Providing a dummy struct avoids this and makes adding Tegra114 clock > >> support in a bisectable way a lot easier. > >> > >> -- > >> > >> Stephen, > >> > >> Should this be a separate patch or should I make this part of new > >> release of the Tegra114 clock series? > > I'm not sure if I answered this. Peter, I intend to apply this patch to a branch > right before the CCF, so there's no explicit need to include it in the series, > although if you do, that's fine. > > >> diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c index > > >> /* Global data of Tegra CPU CAR ops */ -struct tegra_cpu_car_ops > >> *tegra_cpu_car_ops; > > > > Sorry for bringing this up so late... > > Shouldn't the above be "struct tegra_cpu_car_ops tegra_cpu_car_ops;"? > > > >> +static struct tegra_cpu_car_ops *dummy_car_ops; struct > >> +tegra_cpu_car_ops *tegra_cpu_car_ops = &dummy_car_ops; > > No, the value is used as a pointer in include/linux/clk/tegra.h, e.g.: > > tegra_cpu_car_ops->wait_for_reset(cpu); Yeah, I get that tegra_cpu_car_ops is a pointer to an ops table. It seems to me that what's happening above is that tegra_cpu_car_ops is getting assigned a pointer to a pointer that's supposed to point to an instance of struct tegra_cpu_car_ops (but it really points to NULL as far as I can tell). In any case, dummy_car_ops never actually gets instantiated. I assume the intention is for dummy_car_ops to be an instance of struct tegra_cpu_car_ops, but with all of its members zero'd.
On 03/06/2013 04:59 PM, Andrew Chew wrote: >> From: linux-tegra-owner@vger.kernel.org [mailto:linux-tegra- >> owner@vger.kernel.org] On Behalf Of Stephen Warren >> Sent: Wednesday, March 06, 2013 3:43 PM >> To: Andrew Chew >> Cc: Peter De Schrijver; linux-tegra@vger.kernel.org; linux-arm- >> kernel@lists.infradead.org; Stephen Warren; Prashant Gaikwad; Mike >> Turquette; linux-kernel@vger.kernel.org >> Subject: Re: [PATCH] clk: tegra: provide dummy cpu car ops >> >> On 03/06/2013 04:20 PM, Andrew Chew wrote: >>>> Subject: [PATCH] clk: tegra: provide dummy cpu car ops >>>> >>>> tegra_boot_secondary() relies on some of the car ops. This means >>>> having an uninitialized tegra_cpu_car_ops will lead to an early boot panic. >>>> Providing a dummy struct avoids this and makes adding Tegra114 clock >>>> support in a bisectable way a lot easier. >>>> >>>> -- >>>> >>>> Stephen, >>>> >>>> Should this be a separate patch or should I make this part of new >>>> release of the Tegra114 clock series? >> >> I'm not sure if I answered this. Peter, I intend to apply this patch to a branch >> right before the CCF, so there's no explicit need to include it in the series, >> although if you do, that's fine. >> >>>> diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c index >> >>>> /* Global data of Tegra CPU CAR ops */ -struct tegra_cpu_car_ops >>>> *tegra_cpu_car_ops; >>> >>> Sorry for bringing this up so late... >>> Shouldn't the above be "struct tegra_cpu_car_ops tegra_cpu_car_ops;"? >>> >>>> +static struct tegra_cpu_car_ops *dummy_car_ops; struct >>>> +tegra_cpu_car_ops *tegra_cpu_car_ops = &dummy_car_ops; >> >> No, the value is used as a pointer in include/linux/clk/tegra.h, e.g.: >> >> tegra_cpu_car_ops->wait_for_reset(cpu); > > Yeah, I get that tegra_cpu_car_ops is a pointer to an ops table. It seems > to me that what's happening above is that tegra_cpu_car_ops is getting > assigned a pointer to a pointer that's supposed to point to an instance of > struct tegra_cpu_car_ops (but it really points to NULL as far as I can tell). > In any case, dummy_car_ops never actually gets instantiated. > > I assume the intention is for dummy_car_ops to be an instance of > struct tegra_cpu_car_ops, but with all of its members zero'd. Oh right, I guess your comment was about the line after where you wrote it rather than the line before. So, you mean: static struct tegra_cpu_car_ops *dummy_car_ops; struct tegra_cpu_car_ops *tegra_cpu_car_ops = &dummy_car_ops; should be instead: static struct tegra_cpu_car_ops dummy_car_ops; struct tegra_cpu_car_ops *tegra_cpu_car_ops = &dummy_car_ops; Yes, you're right.
diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c index a603b9a..f6c141f 100644 --- a/drivers/clk/tegra/clk.c +++ b/drivers/clk/tegra/clk.c @@ -22,7 +22,8 @@ #include "clk.h" /* Global data of Tegra CPU CAR ops */ -struct tegra_cpu_car_ops *tegra_cpu_car_ops; +static struct tegra_cpu_car_ops *dummy_car_ops; +struct tegra_cpu_car_ops *tegra_cpu_car_ops = &dummy_car_ops; void __init tegra_init_dup_clks(struct tegra_clk_duplicate *dup_list, struct clk *clks[], int clk_max)
tegra_boot_secondary() relies on some of the car ops. This means having an uninitialized tegra_cpu_car_ops will lead to an early boot panic. Providing a dummy struct avoids this and makes adding Tegra114 clock support in a bisectable way a lot easier. -- Stephen, Should this be a separate patch or should I make this part of new release of the Tegra114 clock series? Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com> --- drivers/clk/tegra/clk.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)