Re: [PATCH 4/7] omap3isp: Return -EPROBE_DEFER if the required regulators can't be obtained

[Pavel Machek]

Hi!

> > > > No idea really. I only have N900 working with linux at the moment. I'm
> > > > trying to get N9 and N950 working, but no luck so far.
> > > 
> > > Still no? :-(
> > > 
> > > Do you know if you get the kernel booting? Do you have access to the 
> > > serial
> > > console? I might have seen the e-mail chain but I lost the track. What
> > > happens after the flasher has pushed the kernel to RAM and the boot 
> > > starts?
> > > It's wonderful for debugging if something's wrong...
> > 
> > Still no. No serial cable, unfortunately. Flasher seems to run the
> > kernel, but I see no evidence new kernel started successfully. I was
> > told display is not expected to work, and on USB I see bootloader
> > disconnecting and that's it.
> > 
> > If you had a kernel binary that works for you, and does something I
> > can observe, that would be welcome :-).
> 
> I put my .config I use for N9 here:
> 
> 
> 
> The root filesystem is over NFS root with usbnet. You should see something
> like this in dmesg:
> 
> [35792.056138] usb 2-2: new high-speed USB device number 58 using ehci-pci
> [35792.206238] usb 2-2: New USB device found, idVendor=0525, idProduct=a4a1
> [35792.206247] usb 2-2: New USB device strings: Mfr=1, Product=2, 
> SerialNumber=0
> [35792.206252] usb 2-2: Product: Ethernet Gadget
> [35792.206257] usb 2-2: Manufacturer: Linux 4.13.0-rc1-00089-g4c341695f3b6 
> with musb-hdrc

Could not get it to work, same result as usual: no response on the
device, disconnect on USB and then quiet.

Could I get actual zImage-dtb from you? What development options are
enabled in your case? I was mostly using none -- sudo
../maemo/0x/src/0x -F "" -R 0 .

Thanks,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH 4/7] omap3isp: Return -EPROBE_DEFER if the required regulators can't be obtained

[Sakari Ailus]

On Tue, Jul 18, 2017 at 11:27:12PM +0200, Pavel Machek wrote:
> Hi!

EHLO

> 
> > > No idea really. I only have N900 working with linux at the moment. I'm
> > > trying to get N9 and N950 working, but no luck so far.
> > 
> > Still no? :-(
> > 
> > Do you know if you get the kernel booting? Do you have access to the serial
> > console? I might have seen the e-mail chain but I lost the track. What
> > happens after the flasher has pushed the kernel to RAM and the boot starts?
> > It's wonderful for debugging if something's wrong...
> 
> Still no. No serial cable, unfortunately. Flasher seems to run the
> kernel, but I see no evidence new kernel started successfully. I was
> told display is not expected to work, and on USB I see bootloader
> disconnecting and that's it.
> 
> If you had a kernel binary that works for you, and does something I
> can observe, that would be welcome :-).

I put my .config I use for N9 here:



The root filesystem is over NFS root with usbnet. You should see something
like this in dmesg:

[35792.056138] usb 2-2: new high-speed USB device number 58 using ehci-pci
[35792.206238] usb 2-2: New USB device found, idVendor=0525, idProduct=a4a1
[35792.206247] usb 2-2: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[35792.206252] usb 2-2: Product: Ethernet Gadget
[35792.206257] usb 2-2: Manufacturer: Linux 4.13.0-rc1-00089-g4c341695f3b6 with 
musb-hdrc

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

Re: [PATCH 4/7] omap3isp: Return -EPROBE_DEFER if the required regulators can't be obtained

[Pavel Machek]

Hi!

> > No idea really. I only have N900 working with linux at the moment. I'm
> > trying to get N9 and N950 working, but no luck so far.
> 
> Still no? :-(
> 
> Do you know if you get the kernel booting? Do you have access to the serial
> console? I might have seen the e-mail chain but I lost the track. What
> happens after the flasher has pushed the kernel to RAM and the boot starts?
> It's wonderful for debugging if something's wrong...

Still no. No serial cable, unfortunately. Flasher seems to run the
kernel, but I see no evidence new kernel started successfully. I was
told display is not expected to work, and on USB I see bootloader
disconnecting and that's it.

If you had a kernel binary that works for you, and does something I
can observe, that would be welcome :-).

Best regards,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH 4/7] omap3isp: Return -EPROBE_DEFER if the required regulators can't be obtained

[Sakari Ailus]

On Tue, Jul 18, 2017 at 11:02:28PM +0200, Pavel Machek wrote:
> Hi!
> 
> > > > > diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> > > > > b/drivers/media/platform/omap3isp/ispccp2.c index
> > > > > 4f8fd0c00748..47210b102bcb 100644
> > > > > --- a/drivers/media/platform/omap3isp/ispccp2.c
> > > > > +++ b/drivers/media/platform/omap3isp/ispccp2.c
> > > > > @@ -1140,6 +1140,11 @@ int omap3isp_ccp2_init(struct isp_device *isp)
> > > > >   if (isp->revision == ISP_REVISION_2_0) {
> > > > >   ccp2->vdds_csib = devm_regulator_get(isp->dev, 
> > > > > "vdds_csib");
> > > > >   if (IS_ERR(ccp2->vdds_csib)) {
> > > > > + if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER) {
> > > > > + dev_dbg(isp->dev,
> > > > > + "Can't get regulator vdds_csib, 
> > > > deferring probing\n");
> > > > > + return -EPROBE_DEFER;
> > > > > + }
> > > > >   dev_dbg(isp->dev,
> > > > >   "Could not get regulator vdds_csib\n");
> > > > 
> > > > I would just move this message above the -EPROBE_DEFER check and remove 
> > > > the 
> > > > one inside the check. Probe deferral debug information can be obtained 
> > > > by 
> > > > enabling the debug messages in the driver core.
> > > 
> > > Actually, in such case perhaps the message in -EPROBE_DEFER could be
> > > removed. Deferred probing happens all the time. OTOH "Could not get
> > > regulator" probably should be dev_err(), as it will make device
> > > unusable?
> > 
> > Isn't this only if you need ccp2?
> 
> No idea really. I only have N900 working with linux at the moment. I'm
> trying to get N9 and N950 working, but no luck so far.

Still no? :-(

Do you know if you get the kernel booting? Do you have access to the serial
console? I might have seen the e-mail chain but I lost the track. What
happens after the flasher has pushed the kernel to RAM and the boot starts?
It's wonderful for debugging if something's wrong...

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

Re: [PATCH 4/7] omap3isp: Return -EPROBE_DEFER if the required regulators can't be obtained

[Pavel Machek]

Hi!

> > > > diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> > > > b/drivers/media/platform/omap3isp/ispccp2.c index
> > > > 4f8fd0c00748..47210b102bcb 100644
> > > > --- a/drivers/media/platform/omap3isp/ispccp2.c
> > > > +++ b/drivers/media/platform/omap3isp/ispccp2.c
> > > > @@ -1140,6 +1140,11 @@ int omap3isp_ccp2_init(struct isp_device *isp)
> > > > if (isp->revision == ISP_REVISION_2_0) {
> > > > ccp2->vdds_csib = devm_regulator_get(isp->dev, 
> > > > "vdds_csib");
> > > > if (IS_ERR(ccp2->vdds_csib)) {
> > > > +   if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER) {
> > > > +   dev_dbg(isp->dev,
> > > > +   "Can't get regulator vdds_csib, 
> > > deferring probing\n");
> > > > +   return -EPROBE_DEFER;
> > > > +   }
> > > > dev_dbg(isp->dev,
> > > > "Could not get regulator vdds_csib\n");
> > > 
> > > I would just move this message above the -EPROBE_DEFER check and remove 
> > > the 
> > > one inside the check. Probe deferral debug information can be obtained by 
> > > enabling the debug messages in the driver core.
> > 
> > Actually, in such case perhaps the message in -EPROBE_DEFER could be
> > removed. Deferred probing happens all the time. OTOH "Could not get
> > regulator" probably should be dev_err(), as it will make device
> > unusable?
> 
> Isn't this only if you need ccp2?

No idea really. I only have N900 working with linux at the moment. I'm
trying to get N9 and N950 working, but no luck so far.

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH 4/7] omap3isp: Return -EPROBE_DEFER if the required regulators can't be obtained

[Sakari Ailus]

Hi Pavel,

On Tue, Jul 18, 2017 at 12:03:52PM +0200, Pavel Machek wrote:
> Hi!
> 
> > > diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> > > b/drivers/media/platform/omap3isp/ispccp2.c index
> > > 4f8fd0c00748..47210b102bcb 100644
> > > --- a/drivers/media/platform/omap3isp/ispccp2.c
> > > +++ b/drivers/media/platform/omap3isp/ispccp2.c
> > > @@ -1140,6 +1140,11 @@ int omap3isp_ccp2_init(struct isp_device *isp)
> > >   if (isp->revision == ISP_REVISION_2_0) {
> > >   ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
> > >   if (IS_ERR(ccp2->vdds_csib)) {
> > > + if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER) {
> > > + dev_dbg(isp->dev,
> > > + "Can't get regulator vdds_csib, 
> > deferring probing\n");
> > > + return -EPROBE_DEFER;
> > > + }
> > >   dev_dbg(isp->dev,
> > >   "Could not get regulator vdds_csib\n");
> > 
> > I would just move this message above the -EPROBE_DEFER check and remove the 
> > one inside the check. Probe deferral debug information can be obtained by 
> > enabling the debug messages in the driver core.
> 
> Actually, in such case perhaps the message in -EPROBE_DEFER could be
> removed. Deferred probing happens all the time. OTOH "Could not get
> regulator" probably should be dev_err(), as it will make device
> unusable?

Isn't this only if you need ccp2?

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

Re: [PATCH 4/7] omap3isp: Return -EPROBE_DEFER if the required regulators can't be obtained

[Laurent Pinchart]

Hi Pavel,

On Tuesday 18 Jul 2017 12:03:52 Pavel Machek wrote:
> Hi!
> 
> >> diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> >> b/drivers/media/platform/omap3isp/ispccp2.c index
> >> 4f8fd0c00748..47210b102bcb 100644
> >> --- a/drivers/media/platform/omap3isp/ispccp2.c
> >> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> >> @@ -1140,6 +1140,11 @@ int omap3isp_ccp2_init(struct isp_device *isp)
> >>if (isp->revision == ISP_REVISION_2_0) {
> >>ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
> >>if (IS_ERR(ccp2->vdds_csib)) {
> >> +  if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER) {
> >> +  dev_dbg(isp->dev,
> >> +  "Can't get regulator vdds_csib,
> >> deferring probing\n");
> >> +  return -EPROBE_DEFER;
> >> +  }
> >> 
> >>dev_dbg(isp->dev,
> >>"Could not get regulator vdds_csib\n");
> > 
> > I would just move this message above the -EPROBE_DEFER check and remove
> > the one inside the check. Probe deferral debug information can be obtained
> > by enabling the debug messages in the driver core.
> 
> Actually, in such case perhaps the message in -EPROBE_DEFER could be
> removed. Deferred probing happens all the time. OTOH "Could not get
> regulator" probably should be dev_err(), as it will make device
> unusable?

Messages along the lines of "I'm deferring probe" are in my opinion not 
valuable, as we can get that from the driver core. Debug messages that tell 
why probe is being deferred can however be useful for debugging. That's why I 
proposed to move the regulator get error debug message above the probe 
deferral check, as it would then always be printed. Turning it into an error 
makes sense, but only when not deferring probe then.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/7] omap3isp: Return -EPROBE_DEFER if the required regulators can't be obtained

[Pavel Machek]

Hi!

> > diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> > b/drivers/media/platform/omap3isp/ispccp2.c index
> > 4f8fd0c00748..47210b102bcb 100644
> > --- a/drivers/media/platform/omap3isp/ispccp2.c
> > +++ b/drivers/media/platform/omap3isp/ispccp2.c
> > @@ -1140,6 +1140,11 @@ int omap3isp_ccp2_init(struct isp_device *isp)
> > if (isp->revision == ISP_REVISION_2_0) {
> > ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
> > if (IS_ERR(ccp2->vdds_csib)) {
> > +   if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER) {
> > +   dev_dbg(isp->dev,
> > +   "Can't get regulator vdds_csib, 
> deferring probing\n");
> > +   return -EPROBE_DEFER;
> > +   }
> > dev_dbg(isp->dev,
> > "Could not get regulator vdds_csib\n");
> 
> I would just move this message above the -EPROBE_DEFER check and remove the 
> one inside the check. Probe deferral debug information can be obtained by 
> enabling the debug messages in the driver core.

Actually, in such case perhaps the message in -EPROBE_DEFER could be
removed. Deferred probing happens all the time. OTOH "Could not get
regulator" probably should be dev_err(), as it will make device
unusable?

Thanks,

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH 4/7] omap3isp: Return -EPROBE_DEFER if the required regulators can't be obtained

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Tuesday 18 Jul 2017 01:01:13 Sakari Ailus wrote:
> From: Pavel Machek 
> 
> If regulator returns -EPROBE_DEFER, we need to return it too, so that
> omap3isp will be re-probed when regulator is ready.
> 
> Signed-off-by: Pavel Machek 
> Reviewed-by: Laurent Pinchart 
> Signed-off-by: Sakari Ailus 
> ---
>  drivers/media/platform/omap3isp/isp.c | 3 ++-
>  drivers/media/platform/omap3isp/ispccp2.c | 5 +
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index 80ed5a5f862a..4e6ba7f90e35
> 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -1880,7 +1880,8 @@ static int isp_initialize_modules(struct isp_device
> *isp)
> 
>   ret = omap3isp_ccp2_init(isp);
>   if (ret < 0) {
> - dev_err(isp->dev, "CCP2 initialization failed\n");
> + if (ret != -EPROBE_DEFER)
> + dev_err(isp->dev, "CCP2 initialization failed\n");
>   goto error_ccp2;
>   }
> 
> diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> b/drivers/media/platform/omap3isp/ispccp2.c index
> 4f8fd0c00748..47210b102bcb 100644
> --- a/drivers/media/platform/omap3isp/ispccp2.c
> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> @@ -1140,6 +1140,11 @@ int omap3isp_ccp2_init(struct isp_device *isp)
>   if (isp->revision == ISP_REVISION_2_0) {
>   ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
>   if (IS_ERR(ccp2->vdds_csib)) {
> + if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER) {
> + dev_dbg(isp->dev,
> + "Can't get regulator vdds_csib, 
deferring probing\n");
> + return -EPROBE_DEFER;
> + }
>   dev_dbg(isp->dev,
>   "Could not get regulator vdds_csib\n");

I would just move this message above the -EPROBE_DEFER check and remove the 
one inside the check. Probe deferral debug information can be obtained by 
enabling the debug messages in the driver core.

>   ccp2->vdds_csib = NULL;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/7] omap3isp: Return -EPROBE_DEFER if the required regulators can't be obtained

[Sebastian Reichel]

Hi,

On Tue, Jul 18, 2017 at 01:01:13AM +0300, Sakari Ailus wrote:
> From: Pavel Machek 
> 
> If regulator returns -EPROBE_DEFER, we need to return it too, so that
> omap3isp will be re-probed when regulator is ready.
> 
> Signed-off-by: Pavel Machek 
> Reviewed-by: Laurent Pinchart 
> Signed-off-by: Sakari Ailus 
> ---
>  drivers/media/platform/omap3isp/isp.c | 3 ++-
>  drivers/media/platform/omap3isp/ispccp2.c | 5 +
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c 
> b/drivers/media/platform/omap3isp/isp.c
> index 80ed5a5f862a..4e6ba7f90e35 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -1880,7 +1880,8 @@ static int isp_initialize_modules(struct isp_device 
> *isp)
>  
>   ret = omap3isp_ccp2_init(isp);
>   if (ret < 0) {
> - dev_err(isp->dev, "CCP2 initialization failed\n");
> + if (ret != -EPROBE_DEFER)
> + dev_err(isp->dev, "CCP2 initialization failed\n");
>   goto error_ccp2;
>   }
>  
> diff --git a/drivers/media/platform/omap3isp/ispccp2.c 
> b/drivers/media/platform/omap3isp/ispccp2.c
> index 4f8fd0c00748..47210b102bcb 100644
> --- a/drivers/media/platform/omap3isp/ispccp2.c
> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> @@ -1140,6 +1140,11 @@ int omap3isp_ccp2_init(struct isp_device *isp)
>   if (isp->revision == ISP_REVISION_2_0) {
>   ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
>   if (IS_ERR(ccp2->vdds_csib)) {
> + if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER) {
> + dev_dbg(isp->dev,
> + "Can't get regulator vdds_csib, 
> deferring probing\n");
> + return -EPROBE_DEFER;
> + }

I wonder if the right approach wouldn't be to always bail out for
errors. devm_regulator_get should provide a dummy regulator if
none is specified. If we get an error here it means something is
configured incorrectly or we have serious problems.

>   dev_dbg(isp->dev,
>   "Could not get regulator vdds_csib\n");
>   ccp2->vdds_csib = NULL;

-- Sebastian


signature.asc
Description: PGP signature