| Message ID | 1448456289-31960-1-git-send-email-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Nov 25, 2015 at 01:58:09PM +0100, Marek Szyprowski wrote: > To read pid/cid registers, the probed device need to be properly turned on. > When it is inside a power domain, the bus code should ensure that the > given power domain is enabled before trying to access device's registers. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/amba/bus.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c > index f009936..25715cb 100644 > --- a/drivers/amba/bus.c > +++ b/drivers/amba/bus.c > @@ -373,6 +373,12 @@ int amba_device_add(struct amba_device *dev, struct resource *parent) > goto err_release; > } > > + ret = dev_pm_domain_attach(&dev->dev, true); > + if (ret) { > + iounmap(tmp); > + goto err_release; > + } > + NAK. If dev_pm_domain_attach() returns an error, even -EPROBE_DEFER, the result will be a missing device that has no way to be recovered. This is too fragile.
Hello, On 2015-11-25 14:24, Russell King - ARM Linux wrote: > On Wed, Nov 25, 2015 at 01:58:09PM +0100, Marek Szyprowski wrote: >> To read pid/cid registers, the probed device need to be properly turned on. >> When it is inside a power domain, the bus code should ensure that the >> given power domain is enabled before trying to access device's registers. >> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >> --- >> drivers/amba/bus.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c >> index f009936..25715cb 100644 >> --- a/drivers/amba/bus.c >> +++ b/drivers/amba/bus.c >> @@ -373,6 +373,12 @@ int amba_device_add(struct amba_device *dev, struct resource *parent) >> goto err_release; >> } >> >> + ret = dev_pm_domain_attach(&dev->dev, true); >> + if (ret) { >> + iounmap(tmp); >> + goto err_release; >> + } >> + > NAK. If dev_pm_domain_attach() returns an error, even -EPROBE_DEFER, > the result will be a missing device that has no way to be recovered. > This is too fragile. Then how the problem of accessing registers in turned-off device should be solved? Is ignoring dev_pm_domain_attach() return value a solution for you? Best regards
On 25 November 2015 at 14:34, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > Hello, > > On 2015-11-25 14:24, Russell King - ARM Linux wrote: >> >> On Wed, Nov 25, 2015 at 01:58:09PM +0100, Marek Szyprowski wrote: >>> >>> To read pid/cid registers, the probed device need to be properly turned >>> on. >>> When it is inside a power domain, the bus code should ensure that the >>> given power domain is enabled before trying to access device's registers. >>> >>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >>> --- >>> drivers/amba/bus.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c >>> index f009936..25715cb 100644 >>> --- a/drivers/amba/bus.c >>> +++ b/drivers/amba/bus.c >>> @@ -373,6 +373,12 @@ int amba_device_add(struct amba_device *dev, struct >>> resource *parent) >>> goto err_release; >>> } >>> + ret = dev_pm_domain_attach(&dev->dev, true); >>> + if (ret) { >>> + iounmap(tmp); >>> + goto err_release; >>> + } >>> + >> >> NAK. If dev_pm_domain_attach() returns an error, even -EPROBE_DEFER, >> the result will be a missing device that has no way to be recovered. >> This is too fragile. I agree with Russell here, this isn't going to work. > > > Then how the problem of accessing registers in turned-off device should be > solved? The long term solution has been discussed [1], please have a look and feel free to hack on it if you like. If not, I will sooner or later find time to pick up the work I have started, but can't give you any promises when ready. > > Is ignoring dev_pm_domain_attach() return value a solution for you? That's probably better than nothing, but I wonder if it in practice will have any effect? It requires the PM domain to be initialized (and having a DT provider for it) before you register the AMBA device, is that so in your case? Kind regards Uffe [1] http://comments.gmane.org/gmane.linux.kernel.mmc/32587 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On 2015-11-25 14:56, Ulf Hansson wrote: > On 25 November 2015 at 14:34, Marek Szyprowski <m.szyprowski@samsung.com> wrote: >> On 2015-11-25 14:24, Russell King - ARM Linux wrote: >>> On Wed, Nov 25, 2015 at 01:58:09PM +0100, Marek Szyprowski wrote: >>>> To read pid/cid registers, the probed device need to be properly turned >>>> on. >>>> When it is inside a power domain, the bus code should ensure that the >>>> given power domain is enabled before trying to access device's registers. >>>> >>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >>>> --- >>>> drivers/amba/bus.c | 7 +++++++ >>>> 1 file changed, 7 insertions(+) >>>> >>>> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c >>>> index f009936..25715cb 100644 >>>> --- a/drivers/amba/bus.c >>>> +++ b/drivers/amba/bus.c >>>> @@ -373,6 +373,12 @@ int amba_device_add(struct amba_device *dev, struct >>>> resource *parent) >>>> goto err_release; >>>> } >>>> + ret = dev_pm_domain_attach(&dev->dev, true); >>>> + if (ret) { >>>> + iounmap(tmp); >>>> + goto err_release; >>>> + } >>>> + >>> NAK. If dev_pm_domain_attach() returns an error, even -EPROBE_DEFER, >>> the result will be a missing device that has no way to be recovered. >>> This is too fragile. > I agree with Russell here, this isn't going to work. > >> >> Then how the problem of accessing registers in turned-off device should be >> solved? > The long term solution has been discussed [1], please have a look and > feel free to hack on it if you like. If not, I will sooner or later > find time to pick up the work I have started, but can't give you any > promises when ready. Frankly, I still don't get how do you want to solve the problem of incomplete/not-yet-probed pm domain during the amba_device_add() and reading pid/cid identifiers, so I will leave it for you. >> Is ignoring dev_pm_domain_attach() return value a solution for you? > That's probably better than nothing, but I wonder if it in practice > will have any effect? It requires the PM domain to be initialized (and > having a DT provider for it) before you register the AMBA device, is > that so in your case? Yes, this will work for me, as power domain driver is registered very early on Exynos platform (this was already needed to let it working with IOMMU driver). Best regards
On Wed, Nov 25, 2015 at 02:56:10PM +0100, Ulf Hansson wrote: > On 25 November 2015 at 14:34, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > Is ignoring dev_pm_domain_attach() return value a solution for you? > > That's probably better than nothing, but I wonder if it in practice > will have any effect? If the PM domain is down, then trying to tread the ID is likely to oops the kernel.
Hello, On 2015-11-25 19:09, Russell King - ARM Linux wrote: > On Wed, Nov 25, 2015 at 02:56:10PM +0100, Ulf Hansson wrote: >> On 25 November 2015 at 14:34, Marek Szyprowski <m.szyprowski@samsung.com> wrote: >>> Is ignoring dev_pm_domain_attach() return value a solution for you? >> That's probably better than nothing, but I wonder if it in practice >> will have any effect? > If the PM domain is down, then trying to tread the ID is likely to oops > the kernel. In my case kernel simply hangs (no single message, even when earlyprintk is enabled) instead of oopsing, that's why I've submitted this patch. Best regards
On 26 November 2015 at 09:39, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > Hello, > > On 2015-11-25 19:09, Russell King - ARM Linux wrote: >> >> On Wed, Nov 25, 2015 at 02:56:10PM +0100, Ulf Hansson wrote: >>> >>> On 25 November 2015 at 14:34, Marek Szyprowski <m.szyprowski@samsung.com> >>> wrote: >>>> >>>> Is ignoring dev_pm_domain_attach() return value a solution for you? >>> >>> That's probably better than nothing, but I wonder if it in practice >>> will have any effect? >> >> If the PM domain is down, then trying to tread the ID is likely to oops >> the kernel. > > > In my case kernel simply hangs (no single message, even when earlyprintk is > enabled) > instead of oopsing, that's why I've submitted this patch. > Okay. I suggest we go ahead and try that approach (ignoring the return value). It's "quick fix", but the easiest way forward to solve the problem. My only concern is if such change impacts the boot time. We should do some tests to see if the change is negligible, if not we should probably think of something else (like keeping the PM domain powered until late_init). Russell, what do you think? Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 26, 2015 at 11:24:50AM +0100, Ulf Hansson wrote: > On 26 November 2015 at 09:39, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > Hello, > > > > On 2015-11-25 19:09, Russell King - ARM Linux wrote: > >> > >> On Wed, Nov 25, 2015 at 02:56:10PM +0100, Ulf Hansson wrote: > >>> > >>> On 25 November 2015 at 14:34, Marek Szyprowski <m.szyprowski@samsung.com> > >>> wrote: > >>>> > >>>> Is ignoring dev_pm_domain_attach() return value a solution for you? > >>> > >>> That's probably better than nothing, but I wonder if it in practice > >>> will have any effect? > >> > >> If the PM domain is down, then trying to tread the ID is likely to oops > >> the kernel. > > > > > > In my case kernel simply hangs (no single message, even when earlyprintk is > > enabled) > > instead of oopsing, that's why I've submitted this patch. > > > > Okay. > > I suggest we go ahead and try that approach (ignoring the return > value). It's "quick fix", but the easiest way forward to solve the > problem. > > My only concern is if such change impacts the boot time. We should do > some tests to see if the change is negligible, if not we should > probably think of something else (like keeping the PM domain powered > until late_init). > > Russell, what do you think? I thought someone had a patch to read the ID during the match callback?
Hello, On 2015-11-26 11:59, Russell King - ARM Linux wrote: > On Thu, Nov 26, 2015 at 11:24:50AM +0100, Ulf Hansson wrote: >> On 26 November 2015 at 09:39, Marek Szyprowski <m.szyprowski@samsung.com> wrote: >>> Hello, >>> >>> On 2015-11-25 19:09, Russell King - ARM Linux wrote: >>>> On Wed, Nov 25, 2015 at 02:56:10PM +0100, Ulf Hansson wrote: >>>>> On 25 November 2015 at 14:34, Marek Szyprowski <m.szyprowski@samsung.com> >>>>> wrote: >>>>>> Is ignoring dev_pm_domain_attach() return value a solution for you? >>>>> That's probably better than nothing, but I wonder if it in practice >>>>> will have any effect? >>>> If the PM domain is down, then trying to tread the ID is likely to oops >>>> the kernel. >>> >>> In my case kernel simply hangs (no single message, even when earlyprintk is >>> enabled) >>> instead of oopsing, that's why I've submitted this patch. >>> >> Okay. >> >> I suggest we go ahead and try that approach (ignoring the return >> value). It's "quick fix", but the easiest way forward to solve the >> problem. >> >> My only concern is if such change impacts the boot time. We should do >> some tests to see if the change is negligible, if not we should >> probably think of something else (like keeping the PM domain powered >> until late_init). >> >> Russell, what do you think? > I thought someone had a patch to read the ID during the match callback? Okay, I've reused that patch and it seems to solve mthisproblem. I will send v2 in a few minutes. Best regards
Hello, On 2015-11-26 11:24, Ulf Hansson wrote: > On 26 November 2015 at 09:39, Marek Szyprowski <m.szyprowski@samsung.com> wrote: >> On 2015-11-25 19:09, Russell King - ARM Linux wrote: >>> On Wed, Nov 25, 2015 at 02:56:10PM +0100, Ulf Hansson wrote: >>>> On 25 November 2015 at 14:34, Marek Szyprowski <m.szyprowski@samsung.com> >>>> wrote: >>>>> Is ignoring dev_pm_domain_attach() return value a solution for you? >>>> That's probably better than nothing, but I wonder if it in practice >>>> will have any effect? >>> If the PM domain is down, then trying to tread the ID is likely to oops >>> the kernel. >> >> In my case kernel simply hangs (no single message, even when earlyprintk is >> enabled) >> instead of oopsing, that's why I've submitted this patch. >> > Okay. > > I suggest we go ahead and try that approach (ignoring the return > value). It's "quick fix", but the easiest way forward to solve the > problem. > > My only concern is if such change impacts the boot time. We should do > some tests to see if the change is negligible, if not we should > probably think of something else (like keeping the PM domain powered > until late_init). > > Russell, what do you think? Keeping PM domains powered until late_init would be a good idea regardless of the problem discussed here, because I often see that pm domains are turned on and off a dozen of times during typical boot of Exynos based systems. Best regards
On 26 November 2015 at 03:24, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 26 November 2015 at 09:39, Marek Szyprowski <m.szyprowski@samsung.com> wrote: >> Hello, >> >> On 2015-11-25 19:09, Russell King - ARM Linux wrote: >>> >>> On Wed, Nov 25, 2015 at 02:56:10PM +0100, Ulf Hansson wrote: >>>> >>>> On 25 November 2015 at 14:34, Marek Szyprowski <m.szyprowski@samsung.com> >>>> wrote: >>>>> >>>>> Is ignoring dev_pm_domain_attach() return value a solution for you? >>>> >>>> That's probably better than nothing, but I wonder if it in practice >>>> will have any effect? >>> >>> If the PM domain is down, then trying to tread the ID is likely to oops >>> the kernel. >> >> >> In my case kernel simply hangs (no single message, even when earlyprintk is >> enabled) >> instead of oopsing, that's why I've submitted this patch. >> > > Okay. > > I suggest we go ahead and try that approach (ignoring the return > value). It's "quick fix", but the easiest way forward to solve the > problem. > > My only concern is if such change impacts the boot time. We should do > some tests to see if the change is negligible, if not we should > probably think of something else (like keeping the PM domain powered > until late_init). > > Russell, what do you think? That's the approach I took for coresight on Juno - simply initialise the power domain very early on and keep it on for things like amba probing to be successful. When the boot process is done unused genPDs are switched off automatically. That way we don't need to add extra code to amba. > > Kind regards > Uffe > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On 2015-11-26 16:04, Mathieu Poirier wrote: > On 26 November 2015 at 03:24, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> On 26 November 2015 at 09:39, Marek Szyprowski <m.szyprowski@samsung.com> wrote: >>> On 2015-11-25 19:09, Russell King - ARM Linux wrote: >>>> On Wed, Nov 25, 2015 at 02:56:10PM +0100, Ulf Hansson wrote: >>>>> On 25 November 2015 at 14:34, Marek Szyprowski <m.szyprowski@samsung.com> >>>>> wrote: >>>>>> Is ignoring dev_pm_domain_attach() return value a solution for you? >>>>> That's probably better than nothing, but I wonder if it in practice >>>>> will have any effect? >>>> If the PM domain is down, then trying to tread the ID is likely to oops >>>> the kernel. >>> In my case kernel simply hangs (no single message, even when earlyprintk is >>> enabled) >>> instead of oopsing, that's why I've submitted this patch. >>> >> Okay. >> >> I suggest we go ahead and try that approach (ignoring the return >> value). It's "quick fix", but the easiest way forward to solve the >> problem. >> >> My only concern is if such change impacts the boot time. We should do >> some tests to see if the change is negligible, if not we should >> probably think of something else (like keeping the PM domain powered >> until late_init). >> >> Russell, what do you think? > That's the approach I took for coresight on Juno - simply initialise > the power domain very early on and keep it on for things like amba > probing to be successful. When the boot process is done unused genPDs > are switched off automatically. That way we don't need to add extra > code to amba. Frankly I don't get the point that 'you don't need to add extra code to amba'. Turning power domains on very early on boot is rationale, but then gen_pd core often turns them off and on, depending on the sequence of the registered devices and their runtime pm status. Amba bus should support proper registration of devices in both cases, when power domain for the registered device is turn on and off. In my opinion it should be a gen_pd core responsibility to keep power domain turned on until late init. Right now it turns it off when all devices, which have been registered so far, have been turned off. Usually a moment later another device get registered to the given domain and this requires the domain to get enabled again. This problem can be observed only when there are more than on device in a power domain. Best regards
diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c index f009936..25715cb 100644 --- a/drivers/amba/bus.c +++ b/drivers/amba/bus.c @@ -373,6 +373,12 @@ int amba_device_add(struct amba_device *dev, struct resource *parent) goto err_release; } + ret = dev_pm_domain_attach(&dev->dev, true); + if (ret) { + iounmap(tmp); + goto err_release; + } + ret = amba_get_enable_pclk(dev); if (ret == 0) { u32 pid, cid; @@ -398,6 +404,7 @@ int amba_device_add(struct amba_device *dev, struct resource *parent) } iounmap(tmp); + dev_pm_domain_detach(&dev->dev, true); if (ret) goto err_release;
To read pid/cid registers, the probed device need to be properly turned on. When it is inside a power domain, the bus code should ensure that the given power domain is enabled before trying to access device's registers. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/amba/bus.c | 7 +++++++ 1 file changed, 7 insertions(+)