Re: [PATCH v5 3/5] driver core: handle -EPROBE_DEFER from bus_type.match()

[Russell King - ARM Linux]

On Wed, Dec 23, 2015 at 11:59:26AM +0100, Marek Szyprowski wrote:
> From: Tomeu Vizoso 
> 
> Allow implementations of the match() callback in struct bus_type to
> return errors and if it's -EPROBE_DEFER then queue the device for
> deferred probing.
> 
> This is useful to buses such as AMBA in which devices are registered
> before their matching information can be retrieved from the HW
> (typically because a clock driver hasn't probed yet).
> 
> Signed-off-by: Tomeu Vizoso 
> [changed if-else code structure, adjusted documentation to match the code,
> extended comments]
> Signed-off-by: Marek Szyprowski 
> Reviewed-by: Ulf Hansson 

This patch _really_ needs an ack from Greg before I can merge it.

DRIVER CORE, KOBJECTS, DEBUGFS, KERNFS AND SYSFS
M:  Greg Kroah-Hartman 
T:  git 
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.gitS:  
Supported
F:  Documentation/kobject.txt
F:  drivers/base/

alternatively, the whole series should be taken by Greg, with my ack
for the amba and ARM bits.

Given that Greg is unlikely to respond this close to the merge window
(he's not responded to my messages about the amba-pl011 driver having
been messed up by the wrong patch set being taken...) I think this has
basically missed the 4.5 merge window.  Sorry.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 2/5] ARM: sa1111: ensure no negative value gets returned on positive match

[Russell King - ARM Linux]

On Wed, Dec 23, 2015 at 11:59:25AM +0100, Marek Szyprowski wrote:
> This patch ensures that existing bus match callbacks don't return
> negative values (which might be interpreted as potential errors in the
> future) in case of positive match.

This actually can't return a negative number - only valid devid bits
are 0 to 9 inclusive, and this isn't going to ever change.  So the
patch isn't strictly necessary, but is good from the point of view
of ensuring consistency.  Hence:

> Signed-off-by: Marek Szyprowski 

Acked-by: Russell King 

> ---
>  arch/arm/common/sa.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/common/sa.c b/arch/arm/common/sa.c
> index 3d22494..fb0a0a4 100644
> --- a/arch/arm/common/sa.c
> +++ b/arch/arm/common/sa.c
> @@ -1290,7 +1290,7 @@ static int sa_match(struct device *_dev, struct 
> device_driver *_drv)
>   struct sa_dev *dev = SA_DEV(_dev);
>   struct sa_driver *drv = SA_DRV(_drv);
>  
> - return dev->devid & drv->devid;
> + return !!(dev->devid & drv->devid);
>  }
>  
>  static int sa_bus_suspend(struct device *dev, pm_message_t state)
> -- 
> 1.9.2
> 

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/3] ARM: amba: Move reading of periphid to amba_match()

[Russell King - ARM Linux]

On Tue, Dec 01, 2015 at 02:34:25PM +0100, Marek Szyprowski wrote:
> From: Tomeu Vizoso 
> 
> Reading the periphid when the Primecell device is registered means that
> the apb pclk must be available by then or the device won't be registered
> at all.
> 
> By reading the periphid in amba_match() we can return -EPROBE_DEFER if
> the apb pclk isn't there yet and the device will be retried later.
> 
> Signed-off-by: Tomeu Vizoso 
> [minor code adjustments, added missing comment]
> Signed-off-by: Marek Szyprowski 
> ---
>  drivers/amba/bus.c | 77 
> +-
>  1 file changed, 41 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index f009936..879a421 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -24,6 +24,8 @@
>  
>  #define to_amba_driver(d)container_of(d, struct amba_driver, drv)
>  
> +static int amba_read_periphid(struct amba_device *dev);
> +

Please avoid forward declarations where possible.

>  static const struct amba_id *
>  amba_lookup(const struct amba_id *table, struct amba_device *dev)
>  {
> @@ -43,11 +45,23 @@ static int amba_match(struct device *dev, struct 
> device_driver *drv)
>  {
>   struct amba_device *pcdev = to_amba_device(dev);
>   struct amba_driver *pcdrv = to_amba_driver(drv);
> + int ret;
>  
>   /* When driver_override is set, only bind to the matching driver */
>   if (pcdev->driver_override)
>   return !strcmp(pcdev->driver_override, drv->name);
>  
> + /* Do plug-n-play if no hard-coded primecell ID has been provided */
> + if (!pcdev->periphid) {
> + ret = amba_read_periphid(pcdev);
> + if (ret) {
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "Failed to read periphid: %d",
> + ret);
> + return ret;
> + }
> + }
> +
>   return amba_lookup(pcdrv->id_table, pcdev) != NULL;
>  }
>  
> @@ -336,31 +350,11 @@ static void amba_device_release(struct device *dev)
>   kfree(d);
>  }
>  
> -/**
> - *   amba_device_add - add a previously allocated AMBA device structure
> - *   @dev: AMBA device allocated by amba_device_alloc
> - *   @parent: resource parent for this devices resources
> - *
> - *   Claim the resource, and read the device cell ID if not already
> - *   initialized.  Register the AMBA device with the Linux device
> - *   manager.
> - */
> -int amba_device_add(struct amba_device *dev, struct resource *parent)
> +static int amba_read_periphid(struct amba_device *dev)
>  {
>   u32 size;
>   void __iomem *tmp;
> - int i, ret;
> -
> - WARN_ON(dev->irq[0] == (unsigned int)-1);
> - WARN_ON(dev->irq[1] == (unsigned int)-1);
> -
> - ret = request_resource(parent, >res);
> - if (ret)
> - goto err_out;
> -
> - /* Hard-coded primecell ID instead of plug-n-play */
> - if (dev->periphid != 0)
> - goto skip_probe;
> + int i, ret = 0;
>  
>   /*
>* Dynamically calculate the size of the resource
> @@ -368,10 +362,8 @@ int amba_device_add(struct amba_device *dev, struct 
> resource *parent)
>*/
>   size = resource_size(>res);
>   tmp = ioremap(dev->res.start, size);
> - if (!tmp) {
> - ret = -ENOMEM;
> - goto err_release;
> - }
> + if (!tmp)
> + return -ENOMEM;
>  
>   ret = amba_get_enable_pclk(dev);
>   if (ret == 0) {
> @@ -399,26 +391,39 @@ int amba_device_add(struct amba_device *dev, struct 
> resource *parent)
>  
>   iounmap(tmp);
>  
> + return ret;
> +}
> +
> +/**
> + *   amba_device_add - add a previously allocated AMBA device structure
> + *   @dev: AMBA device allocated by amba_device_alloc
> + *   @parent: resource parent for this devices resources
> + *
> + *   Claim the resource, and register the AMBA device with the Linux device
> + *   manager.
> + */
> +int amba_device_add(struct amba_device *dev, struct resource *parent)
> +{
> + int ret;
> +
> + WARN_ON(dev->irq[0] == (unsigned int)-1);
> + WARN_ON(dev->irq[1] == (unsigned int)-1);
> +
> + ret = request_resource(parent, >res);
>   if (ret)
> - goto err_release;
> + return ret;
>  
> - skip_probe:
>   ret = device_add(>dev);
>   if (ret)
> - goto err_release;
> + return ret;

NAK.

The original code handled error conditions correctly.  This new code
completely breaks the error handling here by removing the release of
the above requested resource.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  

Re: [PATCH 1/2] drivers: amba: properly handle devices with power domains

[Russell King - ARM Linux]

On Thu, Nov 26, 2015 at 11:24:50AM +0100, Ulf Hansson wrote:
> On 26 November 2015 at 09:39, Marek Szyprowski <m.szyprow...@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.szyprow...@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?

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] drivers: amba: properly handle devices with power domains

[Russell King - ARM Linux]

On Wed, Nov 25, 2015 at 02:56:10PM +0100, Ulf Hansson wrote:
> On 25 November 2015 at 14:34, Marek Szyprowski  
> 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.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] drivers: amba: properly handle devices with power domains

[Russell King - ARM Linux]

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 
> ---
>  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, 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.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: use "depends on" for SoC configs instead of "if" after prompt

[Russell King - ARM Linux]

On Mon, Nov 16, 2015 at 10:32:51AM +, yamada.masah...@socionext.com wrote:
> Hi Arnd,
> 
>  
> > On Monday 16 November 2015 12:06:10 Masahiro Yamada wrote:
> > > Many ARM sub-architectures use prompts followed by "if" conditional,
> > > but it is wrong.
> > >
> > > Please notice the difference between
> > >
> > > config ARCH_FOO
> > > bool "Foo SoCs" if ARCH_MULTI_V7
> > >
> > > and
> > >
> > > config ARCH_FOO
> > > bool "Foo SoCs"
> > > depends on ARCH_MULTI_V7
> > >
> > > These two are *not* equivalent!
> > >
> > > In the former statement, it is not ARCH_FOO, but its prompt that
> > > depends on ARCH_MULTI_V7.  So, it is completely valid that ARCH_FOO is
> > > selected by another, but ARCH_MULTI_V7 is still disabled. As it is not
> > > unmet dependency, Kconfig never warns.  This is probably not what you
> > > want.
> > 
> > Did you encounter a case where someone actually did a 'select' on one of
> > those symbols? I probably introduced a lot of them and did not expect that
> > to happen.
> 
> No, for ARM sub-architectures.
> But, yes for the ARM core part.
> 
> 
> For example, the following entry in arch/arm/Kconfig is suspicous.
> 
> config PCI
> bool "PCI support" if MIGHT_HAVE_PCI
> help
>   Find out whether you have a PCI motherboard. PCI is the name of a
>   bus system, i.e. the way the CPU talks to the other stuff inside
>   your box. Other bus systems are ISA, EISA, MicroChannel (MCA) or
>   VESA. If you have PCI, say Y, otherwise N.
> 
> 
> 
> 
> Try "make ARCH=arm footbridge_defconfig" and check the .config file.
> 
> It defines CONFIG_PCI=y, but not CONFIG_MIGHT_HAVE_PCI.
> I am not sure this is a sane .config or not.

It's correct.  "MIGHT_HAVE_PCI" is used by platforms which _might_ _have_
_PCI_, not by platforms which _do_ _have_ _PCI_.  Platforms which _do_
_have_ _PCI_ select PCI directly, and because "MIGHT_HAVE_PCI" is not
set, users are not offered an option that they can never disable.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] ARM: exynos_defconfig: Increase CONFIG_BLK_DEV_RAM_SIZE to 64K

[Russell King - ARM Linux]

On Mon, Oct 19, 2015 at 01:48:13PM +0200, Javier Martinez Canillas wrote:
> Hello Alim,
> 
> On 10/19/2015 12:48 PM, Alim Akhtar wrote:
> > CONFIG_BLK_DEV_RAM_SIZE is currently set to 8K, which is a bit on the
> > smaller side, lets bump it up to 64K so that a bigger RAM_DISK can
> > be used with defconfig.
> > 
> > Signed-off-by: Alim Akhtar 
> > ---
> 
> I agree that 8KiB is too small and that should be bumped, I'm also
> not sure what the best size would be but 64KiB sounds reasonable
> to me. So for this patch:

It's _not_ 8KiB or 64KiB.

config BLK_DEV_RAM_SIZE
int "Default RAM disk size (kbytes)"

It's 8MiB or 64MiB - the value you place here is in units of 1024 bytes.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv9 06/15] rc: Add HDMI CEC protocol handling

[Russell King - ARM Linux]

On Mon, Oct 12, 2015 at 01:50:47PM +0200, Hans Verkuil wrote:
> > Yet the rc-cec is a module in the filesystem, but it doesn't seem to
> > be loaded automatically - even after the system has booted, the module
> > hasn't been loaded.
> > 
> > It looks like it _should_ be loaded, but this plainly isn't working:
> > 
> > map = seek_rc_map(name);
> > #ifdef MODULE

This is the problem.  MODULE is only set when _this_ file is built as a
module.  If this is built in, but your rc maps are modules, then they
won't get loaded because this symbol will not be defined.

It needs to be CONFIG_MODULES - and when it is, the rc-cec module is
automatically loaded.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv9 07/15] cec: add HDMI CEC framework

[Russell King - ARM Linux]

On Mon, Oct 12, 2015 at 01:35:54PM +0200, Hans Verkuil wrote:
> On 10/06/2015 07:06 PM, Russell King - ARM Linux wrote:
> > Surely you aren't proposing that drivers should write directly to
> > adap->phys_addr without calling some notification function that the
> > physical address has changed?
> 
> Userspace is informed through CEC_EVENT_STATE_CHANGE when the adapter is
> enabled/disabled. When the adapter is enabled and CEC_CAP_PHYS_ADDR is
> not set (i.e. the kernel takes care of this), then calling 
> CEC_ADAP_G_PHYS_ADDR
> returns the new physical address.

Okay, so when I see the EDID arrive, I should be doing:

phys = parse_hdmi_addr(block->edid);
cec->adap->phys_addr = phys;
cec_enable(cec->adap, true);

IOW, you _are_ expecting adap->phys_addr to be written, but only while
the adapter is disabled?

Thanks.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv9 06/15] rc: Add HDMI CEC protocol handling

[Russell King - ARM Linux]

On Mon, Oct 12, 2015 at 01:50:47PM +0200, Hans Verkuil wrote:
> On 10/06/2015 08:05 PM, Russell King - ARM Linux wrote:
> > On Mon, Sep 07, 2015 at 03:44:35PM +0200, Hans Verkuil wrote:
> >> From: Kamil Debski <ka...@wypas.org>
> >>
> >> Add handling of remote control events coming from the HDMI CEC bus.
> >> This patch includes a new keymap that maps values found in the CEC
> >> messages to the keys pressed and released. Also, a new protocol has
> >> been added to the core.
> >>
> >> Signed-off-by: Kamil Debski <ka...@wypas.org>
> >> Signed-off-by: Hans Verkuil <hans.verk...@cisco.com>
> > 
> > (Added Mauro)
> > 
> > Hmm, how is rc-cec supposed to be loaded?
> 
> Is CONFIG_RC_MAP enabled in your config? Ran 'depmod -a'? (Sorry, I'm sure 
> you've done
> that, just checking...)

CONFIG_RC_MAP=m

and yes, if depmod hadn't have been run, modprobing rc-cec would not
have worked - modprobe always looks up in the depmod information to
find out where the module is located, and also to determine any
dependencies.

> It's optional as I understand it, since you could configure the keytable from
> userspace instead of using this module.
> 
> For the record (just tried it), it does load fine on my setup.

Immediately after boot, I have:

# lsmod
Module  Size  Used by
...
coda   54685  0
v4l2_mem2mem   14517  1 coda
videobuf2_dma_contig 9478  1 coda
videobuf2_vmalloc   5529  1 coda
videobuf2_memops1888  2 videobuf2_dma_contig,videobuf2_vmalloc
cecd_dw_hdmi3129  0
# modprobe rc-cec
# lsmod
Module  Size  Used by
rc_cec  1785  0
...
coda   54685  0
v4l2_mem2mem   14517  1 coda
videobuf2_dma_contig 9478  1 coda
videobuf2_vmalloc   5529  1 coda
videobuf2_memops1888  2 videobuf2_dma_contig,videobuf2_vmalloc
cecd_dw_hdmi3129  0

So, rc-cec is perfectly loadable, it just doesn't get loaded at boot.
Manually loading it like this is useless though - I have to unload
cecd_dw_hdmi and then re-load it after rc-cec is loaded for rc-cec to
be seen.  At that point, (and with the help of a userspace program)
things start working as expected.

> BTW, I am still on the fence whether using the kernel RC subsystem is
> the right thing to do. There are a number of CEC RC commands that use
> extra parameters that cannot be mapped to the RC API, so you still
> need to handle those manually.

Even though it is a remote control which is being forwarded for the
most part, but there are operation codes which aren't related to
key presses specified by the standard.  I don't think there's anything
wrong with having a RC interface present, but allowing other interfaces
as a possibility is a good thing too - it allows a certain amount of
flexibility.

For example, with rc-cec loaded and properly bound, I can control at
least rhythmbox within gnome using the TVs remote control with no
modifications - and that happens because the X server passes on the
events it receives via the event device.

Given the range of media applications, I think that's key - it needs
to at least have the capability to plug into the existing ways of doing
things, even if those ways are not perfect.

> Perhaps I should split it off into a separate patch and keep it out
> from the initial pull request once we're ready for that.

I'm biased because it is an enablement feature - it allows CEC to work
out of the box with at least some existing media apps. :)

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv9 14/15] cec: s5p-cec: Add s5p-cec driver

[Russell King - ARM Linux]

On Mon, Oct 12, 2015 at 02:39:42PM +0200, Hans Verkuil wrote:
> On 10/12/2015 02:33 PM, Kamil Debski wrote:
> > The possible status values that are implemented in the CEC framework
> > are following:
> > 
> > +/* cec status field */
> > +#define CEC_TX_STATUS_OK   (0)
> > +#define CEC_TX_STATUS_ARB_LOST (1 << 0)
> > +#define CEC_TX_STATUS_RETRY_TIMEOUT(1 << 1)
> > +#define CEC_TX_STATUS_FEATURE_ABORT(1 << 2)
> > +#define CEC_TX_STATUS_REPLY_TIMEOUT(1 << 3)
> > +#define CEC_RX_STATUS_READY(0)
> > 
> > The only two ones I could match with the Exynos CEC module status bits are
> > CEC_TX_STATUS_OK and  CEC_TX_STATUS_RETRY_TIMEOUT.
> > 
> > The status bits in Exynos HW are:
> > - Tx_Error
> > - Tx_Done
> > - Tx_Transferring
> > - Tx_Running
> > - Tx_Bytes_Transferred
> > 
> > - Tx_Wait
> > - Tx_Sending_Status_Bit
> > - Tx_Sending_Hdr_Blk
> > - Tx_Sending_Data_Blk
> > - Tx_Latest_Initiator
> > 
> > - Tx_Wait_SFT_Succ
> > - Tx_Wait_SFT_New
> > - Tx_Wait_SFT_Retran
> > - Tx_Retrans_Cnt
> > - Tx_ACK_Failed
> 
> So are these all intermediate states? And every transfer always ends with 
> Tx_Done or
> Tx_Error state?
> 
> It does look that way...

For the Synopsis DW CEC, I have:

Bit Field   Description
4   ERROR_INIT  An error is detected on cec line (for initiator only).
3   ARB_LOSTThe initiator losses the CEC line arbitration to a second
initiator. (specification CEC 9).
2   NACKA frame is not acknowledged in a directly addressed message.
Or a frame is negatively acknowledged in a broadcast message
(for initiator only).
0   DONEThe current transmission is successful (for initiator only).

That's about as much of a description that there is for this hardware.
Quite what comprises an "ERROR_INIT", I don't know.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] sched_clock: add data pointer argument to read callback

[Russell King - ARM Linux]

On Sat, Oct 10, 2015 at 01:42:47AM +0100, Måns Rullgård wrote:
> Russell King - ARM Linux <li...@arm.linux.org.uk> writes:
> 
> > On Sat, Oct 10, 2015 at 12:48:22AM +0100, Måns Rullgård wrote:
> >> Russell King - ARM Linux <li...@arm.linux.org.uk> writes:
> >> 
> >> > On Fri, Oct 09, 2015 at 10:57:35PM +0100, Mans Rullgard wrote:
> >> >> This passes a data pointer specified in the sched_clock_register()
> >> >> call to the read callback allowing simpler implementations thereof.
> >> >> 
> >> >> In this patch, existing uses of this interface are simply updated
> >> >> with a null pointer.
> >> >
> >> > This is a bad description.  It tells us what the patch is doing,
> >> > (which we can see by reading the patch) but not _why_.  Please include
> >> > information on why the change is necessary - describe what you are
> >> > trying to achieve.
> >> 
> >> Currently most of the callbacks use a global variable to store the
> >> address of a counter register.  This has several downsides:
> >> 
> >> - Loading the address of a global variable can be more expensive than
> >>   keeping a pointer next to the function pointer.
> >> 
> >> - It makes it impossible to have multiple instances of a driver call
> >>   sched_clock_register() since the caller can't know which clock will
> >>   win in the end.
> >> 
> >> - Many of the existing callbacks are practically identical and could be
> >>   replaced with a common generic function if it had a pointer argument.
> >> 
> >> If I've missed something that makes this a stupid idea, please tell.
> >
> > So my next question is whether you intend to pass an iomem pointer
> > through this, or a some kind of structure, or both.  It matters,
> > because iomem pointers have a __iomem attribute to keep sparse
> > happy.  Having to force that attribute on and off pointers is frowned
> > upon, as it defeats the purpose of the sparse static checker.
> 
> So this is an instance where tools like sparse get in the way of doing
> the simplest, most efficient, and obviously correct thing.  Who wins in
> such cases?

In that case, NAK on the patch.  I don't have time for your stupid games.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] sched_clock: add data pointer argument to read callback

[Russell King - ARM Linux]

On Fri, Oct 09, 2015 at 10:57:35PM +0100, Mans Rullgard wrote:
> This passes a data pointer specified in the sched_clock_register()
> call to the read callback allowing simpler implementations thereof.
> 
> In this patch, existing uses of this interface are simply updated
> with a null pointer.

This is a bad description.  It tells us what the patch is doing,
(which we can see by reading the patch) but not _why_.  Please include
information on why the change is necessary - describe what you are
trying to achieve.

I generally don't accept patches what add new stuff to the kernel with
no users of that new stuff - that's called experience, experience of
people who submit stuff like that, and then vanish leaving their junk
in the kernel without any users.  Please ensure that this gets a user
very quickly, or better still, submit this patch as part of a series
which makes use of it.

Also, copying soo many people is guaranteed to be silently dropped by
mailing lists.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] sched_clock: add data pointer argument to read callback

[Russell King - ARM Linux]

On Sat, Oct 10, 2015 at 12:48:22AM +0100, Måns Rullgård wrote:
> Russell King - ARM Linux <li...@arm.linux.org.uk> writes:
> 
> > On Fri, Oct 09, 2015 at 10:57:35PM +0100, Mans Rullgard wrote:
> >> This passes a data pointer specified in the sched_clock_register()
> >> call to the read callback allowing simpler implementations thereof.
> >> 
> >> In this patch, existing uses of this interface are simply updated
> >> with a null pointer.
> >
> > This is a bad description.  It tells us what the patch is doing,
> > (which we can see by reading the patch) but not _why_.  Please include
> > information on why the change is necessary - describe what you are
> > trying to achieve.
> 
> Currently most of the callbacks use a global variable to store the
> address of a counter register.  This has several downsides:
> 
> - Loading the address of a global variable can be more expensive than
>   keeping a pointer next to the function pointer.
> 
> - It makes it impossible to have multiple instances of a driver call
>   sched_clock_register() since the caller can't know which clock will
>   win in the end.
> 
> - Many of the existing callbacks are practically identical and could be
>   replaced with a common generic function if it had a pointer argument.
> 
> If I've missed something that makes this a stupid idea, please tell.

So my next question is whether you intend to pass an iomem pointer
through this, or a some kind of structure, or both.  It matters,
because iomem pointers have a __iomem attribute to keep sparse
happy.  Having to force that attribute on and off pointers is frowned
upon, as it defeats the purpose of the sparse static checker.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv9 06/15] rc: Add HDMI CEC protocol handling

[Russell King - ARM Linux]

On Mon, Sep 07, 2015 at 03:44:35PM +0200, Hans Verkuil wrote:
> From: Kamil Debski 
> 
> Add handling of remote control events coming from the HDMI CEC bus.
> This patch includes a new keymap that maps values found in the CEC
> messages to the keys pressed and released. Also, a new protocol has
> been added to the core.
> 
> Signed-off-by: Kamil Debski 
> Signed-off-by: Hans Verkuil 

(Added Mauro)

Hmm, how is rc-cec supposed to be loaded?

At boot, I see:

[   16.577704] IR keymap rc-cec not found
[   16.586675] Registered IR keymap rc-empty
[   16.591668] input: RC for dw_hdmi as 
/devices/soc0/soc/12.hdmi/rc/rc1/input3
[   16.597769] rc1: RC for dw_hdmi as /devices/soc0/soc/12.hdmi/rc/rc1

Yet the rc-cec is a module in the filesystem, but it doesn't seem to
be loaded automatically - even after the system has booted, the module
hasn't been loaded.

It looks like it _should_ be loaded, but this plainly isn't working:

map = seek_rc_map(name);
#ifdef MODULE
if (!map) {
int rc = request_module("%s", name);
if (rc < 0) {
printk(KERN_ERR "Couldn't load IR keymap %s\n", name);
return NULL;
}
msleep(20); /* Give some time for IR to register */

map = seek_rc_map(name);
}
#endif
if (!map) {
printk(KERN_ERR "IR keymap %s not found\n", name);
return NULL;
}

Any ideas?

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv9 07/15] cec: add HDMI CEC framework

[Russell King - ARM Linux]

On Mon, Sep 07, 2015 at 03:44:36PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil 
> 
> The added HDMI CEC framework provides a generic kernel interface for
> HDMI CEC devices.
> 
> Signed-off-by: Hans Verkuil 
> [k.deb...@samsung.com: Merged CEC Updates commit by Hans Verkuil]
> [k.deb...@samsung.com: Merged Update author commit by Hans Verkuil]
> [k.deb...@samsung.com: change kthread handling when setting logical
> address]
> [k.deb...@samsung.com: code cleanup and fixes]
> [k.deb...@samsung.com: add missing CEC commands to match spec]
> [k.deb...@samsung.com: add RC framework support]
> [k.deb...@samsung.com: move and edit documentation]
> [k.deb...@samsung.com: add vendor id reporting]
> [k.deb...@samsung.com: add possibility to clear assigned logical
> addresses]
> [k.deb...@samsung.com: documentation fixes, clenaup and expansion]
> [k.deb...@samsung.com: reorder of API structs and add reserved fields]
> [k.deb...@samsung.com: fix handling of events and fix 32/64bit timespec
> problem]
> [k.deb...@samsung.com: add cec.h to include/uapi/linux/Kbuild]
> [k.deb...@samsung.com: add sequence number handling]
> [k.deb...@samsung.com: add passthrough mode]
> [k.deb...@samsung.com: fix CEC defines, add missing CEC 2.0 commands]
> minor additions]
> Signed-off-by: Kamil Debski 
> Signed-off-by: Hans Verkuil 

I don't see much in the way of support for source devices in this:
how do we handle hotplug of the sink, and how to do we configure the
physical address?

Surely you aren't proposing that drivers should write directly to
adap->phys_addr without calling some notification function that the
physical address has changed?

Please can you give some guidance on how a HDMI source bridge driver
should deal with these issues.  Thanks.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv9 14/15] cec: s5p-cec: Add s5p-cec driver

[Russell King - ARM Linux]

On Mon, Sep 07, 2015 at 03:44:43PM +0200, Hans Verkuil wrote:
> + if (status & CEC_STATUS_TX_DONE) {
> + if (status & CEC_STATUS_TX_ERROR) {
> + dev_dbg(cec->dev, "CEC_STATUS_TX_ERROR set\n");
> + cec->tx = STATE_ERROR;
> + } else {
> + dev_dbg(cec->dev, "CEC_STATUS_TX_DONE\n");
> + cec->tx = STATE_DONE;
> + }
> + s5p_clr_pending_tx(cec);
> + }

Your CEC implementation seems to be written around the idea that there
are only two possible outcomes from a CEC message - "done" and "error",
which get translated to:

> + case STATE_DONE:
> + cec_transmit_done(cec->adap, CEC_TX_STATUS_OK);
> + cec->tx = STATE_IDLE;
> + break;
> + case STATE_ERROR:
> + cec_transmit_done(cec->adap, CEC_TX_STATUS_RETRY_TIMEOUT);
> + cec->tx = STATE_IDLE;

"okay" and "retry_timeout".  So, if we have an adapter which can report
(eg) a NACK, we have to report it as the obscure "retry timeout" status?
Why this obscure naming - why can't we have something that uses the
terminology in the spec?

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv9 14/15] cec: s5p-cec: Add s5p-cec driver

[Russell King - ARM Linux]

On Mon, Sep 07, 2015 at 03:44:43PM +0200, Hans Verkuil wrote:
> + cec->adap = cec_create_adapter(_cec_adap_ops, cec,
> + CEC_NAME, CEC_CAP_STATE |
> + CEC_CAP_PHYS_ADDR | CEC_CAP_LOG_ADDRS | CEC_CAP_IO |
> + CEC_CAP_IS_SOURCE,
> + 0, THIS_MODULE, >dev);
> + ret = PTR_ERR_OR_ZERO(cec->adap);
> + if (ret)
> + return ret;
> + cec->adap->available_log_addrs = 1;
> +
> + platform_set_drvdata(pdev, cec);
> + pm_runtime_enable(dev);

This is really not a good interface.

"cec_create_adapter" creates and registers the adapter, at which point it
becomes available to userspace.  However, you haven't finished setting it
up, so it's possible to nip in here and start using it before the setup
has completed.  This needs fixing.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFT] mach-s3c64xx:Fix error handling for certain calls to s3c_gpio_cfgpin_range in the file dev-audio.c

[Russell King - ARM Linux]

On Wed, Sep 16, 2015 at 11:00:35PM -0400, Nicholas Krause wrote:
> diff --git a/arch/arm/mach-s3c64xx/dev-audio.c 
> b/arch/arm/mach-s3c64xx/dev-audio.c
> index ff780a8..81fabdb 100644
> --- a/arch/arm/mach-s3c64xx/dev-audio.c
> +++ b/arch/arm/mach-s3c64xx/dev-audio.c
> @@ -27,6 +27,7 @@
>  static int s3c64xx_i2s_cfg_gpio(struct platform_device *pdev)
>  {
>   unsigned int base;
> + int ret;
>  
>   switch (pdev->id) {
>   case 0:
> @@ -47,9 +48,9 @@ static int s3c64xx_i2s_cfg_gpio(struct platform_device 
> *pdev)
>   return -EINVAL;
>   }
>  
> - s3c_gpio_cfgpin_range(base, 5, S3C_GPIO_SFN(3));
> -
> - return 0;
> + ret = s3c_gpio_cfgpin_range(base, 5, S3C_GPIO_SFN(3));
> +
> + return ret;

Let's look at the code:

case 2:
s3c_gpio_cfgpin(S3C64XX_GPC(4), S3C_GPIO_SFN(5));
s3c_gpio_cfgpin(S3C64XX_GPC(5), S3C_GPIO_SFN(5));
s3c_gpio_cfgpin(S3C64XX_GPC(7), S3C_GPIO_SFN(5));
s3c_gpio_cfgpin_range(S3C64XX_GPH(6), 4, S3C_GPIO_SFN(5));
return 0;
default:
printk(KERN_DEBUG "Invalid I2S Controller number: %d\n",
pdev->id);
return -EINVAL;
}

s3c_gpio_cfgpin_range(base, 5, S3C_GPIO_SFN(3));

return 0;

and you're changing it to:

case 2:
s3c_gpio_cfgpin(S3C64XX_GPC(4), S3C_GPIO_SFN(5));
s3c_gpio_cfgpin(S3C64XX_GPC(5), S3C_GPIO_SFN(5));
s3c_gpio_cfgpin(S3C64XX_GPC(7), S3C_GPIO_SFN(5));
s3c_gpio_cfgpin_range(S3C64XX_GPH(6), 4, S3C_GPIO_SFN(5));
return 0;
default:
printk(KERN_DEBUG "Invalid I2S Controller number: %d\n",
pdev->id);
return -EINVAL;
}

ret = s3c_gpio_cfgpin_range(base, 5, S3C_GPIO_SFN(3));

return ret;

What about all the previous calls to s3c_gpio_cfgpin_range() and
s3c_gpio_cfgpin() ?  Can't they fail as well?

However, _maybe_ the original authors idea was "I don't care if these
calls fail, it's safer to continue if they do" and your changes actually
result in breakage.

Maybe the better solution would be to add WARN_ON() around each of these.

There's a lot of questions here, none of them are a trivial case of "lets
generate a patch to just do some random change to the code and hope it's
right."

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 14/16] drm: bridge: analogix/dp: try force hpd after plug in lookup failed

[Russell King - ARM Linux]

On Thu, Sep 03, 2015 at 11:04:40AM +0200, Thierry Reding wrote:
> Conversely, if the panel isn't capable of generating an HPD signal, then
> I don't think it would be appropriate to make it a DT property. It would
> be better to hard-code it in the driver, lest someone forget to set the
> property in DT and get stuck with a device that isn't operational.

There is another way to deal with this: DRM supports the idea of connector
forcing - where you can force the connector to think that it's connected
or disconnected.

One of the problems is that not many ARM DRM drivers implement it - maybe
it should be a requirement for code to be accepted? :)

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp

[Russell King - ARM Linux]

On Tue, Aug 25, 2015 at 04:21:51PM +0200, Thierry Reding wrote:
 You cited code from dw_hdmi.c earlier, it looks like it might be correct
 even though it doesn't cite a reference for why this was done. Perhaps
 someone on this thread, or someone involved with dw_hdmi can answer
 where that code came from.

dw_hdmi doesn't do any format conversion - it's hard coded to RGB, 8
bits per colour component.  That's a requirement for all HDMI sinks.

The reason it's hard-coded in dw_hdmi is that (a) no one has yet decided
its worth the effort to get the dw_hdmi hardware to do the colourspace
conversion to the YUV spaces and verify that it works, and (b) I really
don't see the point when we're talking about computer like devices which
work primerily with RGB and RGB is always supported by the sink.

As far as greater colour depths go, the driver came from the Freescale
iMX6 code base, and the hardware which feeds dw_hdmi can't do more than
8 bits per component - so going to 10, 12 or 16 bits per component is
beyond what iMX6 can cope with.  Hence, no one on the iMX6 side has a
need for the deep colour stuff.

In any case, I view this as a very low priority issue - it would be nice
to have audio support on HDMI for iMX6 at some point in the next 20 years,
preferably before the hardware becomes obsolete.  I've been maintaining
patches for this for 1.5 years now... how much longer is it going to take?
My pull request to David from 15th July was ignored.  My re-send of that
after he returned was ignored.  My reminder of it has been ignored.  What's
going on in DRM land?  It would be nice to get _some_ kind of feedback so
I know why they're not being taken, so I can fix whatever the issue is.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp

[Russell King - ARM Linux]

On Tue, Aug 25, 2015 at 11:12:48AM +0200, Thierry Reding wrote:
 On Mon, Aug 24, 2015 at 09:48:27AM -0500, Rob Herring wrote:
  It goes beyond bindings IMO. The use of the component framework or not
  has been at the whim of driver writers as well. It is either used or
  private APIs are created. I'm using components and my need for it
  boils down to passing the struct drm_device pointer to the encoder.
  Other components like panels and bridges have different ways to attach
  to the DRM driver.
 
 I certainly support unification, but it needs to be reasonable. There
 are cases where a different structure for the binding work better than
 another and I think this always needs to be evaluated on a case by case
 basis.

It can't be a case-by-case basis.

The TDA998x encoder/connector is going to be component only.  This is
a generic chip, which can be attached to the output of any parallel
RGB+sync+clock bus.  In other words, it could appear anywhere.

Are you really saying that we need to support multiple schemes of
attaching the driver to DRM?  That's totally insane IMHO.

The problem with the drm_encoder_slave stuff is that you can't sanely
attach of-nodes to the drm-created i2c device.  Yes, you can parse
them from the DT file as a sub-node of the upper device, but that
then goes against the principle of the I2C bindings, which is to
list the I2C devices as a child below the I2C adapter node.  If you
try and put the DT node there, then the OF code will create the I2C
device for you, and the drm_encoder_slave stuff won't have the
control it needs to communicate through the wrapped i2c_driver
stuff.

So, tda998x is going component-only, as that's the _only_ sane solution
for it.

Now, what happens when some other DRM driver wants to use the tda998x
driver, and its bindings are not compatible with the component helpers?
They're pretty much stuck up the creek without a paddle.

Case by case doesn't work unless you're talking about truely isolated
hardware where no one shares anything.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 06/14] Documentation: drm/bridge: add document for analogix_dp

[Russell King - ARM Linux]

On Tue, Aug 25, 2015 at 12:40:01PM +0200, Thierry Reding wrote:
 On Tue, Aug 25, 2015 at 10:29:39AM +0100, Russell King - ARM Linux wrote:
  Now, what happens when some other DRM driver wants to use the tda998x
  driver, and its bindings are not compatible with the component helpers?
  They're pretty much stuck up the creek without a paddle.
 
 I'm sure that will be very helpful response for whoever's going to end
 up having to deal with that situation.

Thank you for that comment, it's very constructive and much appreciated.
I can see it's well worth me continuing to spend time on this thread.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/18] ARM: use const and __initconst for smp_operations

[Russell King - ARM Linux]

On Mon, Aug 24, 2015 at 02:12:06PM -0700, Olof Johansson wrote:
 Easiest of all would probably be to get the sub-arch patches into one
 release, then switch the prototypes and function definitions in the
 next. If you switch prototypes first you'll get a bunch of warnings,
 right?

Wrong way around. :)

If you change the sub-arches to declare the smp operations as const,
and try and pass them into a function which doesn't take a const-pointer,
you'll get a warning.  The core bits need to go in first before the
sub-arch patches.

I think the series has limited value - it allows us to (a) check that a
small quantity of code doesn't write to these things, and (b) allows us
to move the SMP operations structure from __initdata to __initconstdata.
It's still going to end up in the init region which is read/write in any
case, and still gets thrown away.

Given where we are, I don't think we need to rush this in during the
last week before the merge window opens, even though it's trivial.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv8 07/15] cec: add HDMI CEC framework

[Russell King - ARM Linux]

On Tue, Aug 18, 2015 at 10:26:32AM +0200, Hans Verkuil wrote:
 + /* Part 2: Initialize and register the character device */
 + cdev_init(cecdev-cdev, cec_devnode_fops);
 + cecdev-cdev.owner = owner;
 +
 + ret = cdev_add(cecdev-cdev, MKDEV(MAJOR(cec_dev_t), cecdev-minor),
 + 1);
 + if (ret  0) {
 + pr_err(%s: cdev_add failed\n, __func__);
 + goto error;
 + }
 +
 + /* Part 3: Register the cec device */
 + cecdev-dev.bus = cec_bus_type;
 + cecdev-dev.devt = MKDEV(MAJOR(cec_dev_t), cecdev-minor);
 + cecdev-dev.release = cec_devnode_release;
 + if (cecdev-parent)
 + cecdev-dev.parent = cecdev-parent;
 + dev_set_name(cecdev-dev, cec%d, cecdev-minor);
 + ret = device_register(cecdev-dev);

It's worth pointing out that you can greatly simplify the lifetime
handling (you don't need to get and put cecdev-dev) if you make
the cdev a child of the cecdev-dev.

If you grep for kobj.parent in drivers/ you'll see many drivers are
doing this.

cecdev-cdev.kobj.parent = cecdev-dev.kobj;

but you will need to call device_initialize() on cecdev-dev first,
and use device_add() here.

 + if (ret  0) {
 + pr_err(%s: device_register failed\n, __func__);
 + goto error;
 + }
 +
 + /* Part 4: Activate this minor. The char device can now be used. */
 + set_bit(CEC_FLAG_REGISTERED, cecdev-flags);

Having flags to indicate whether userspace can open something is racy.
I don't see any other uses of cecdev-flags.  I think you should kill
this, and replace it with a cecdev-dead flag which indicates when the
cecdev is going away, and causes any pre-existing users to fail.


 +
 + return 0;
 +
 +error:
 + cdev_del(cecdev-cdev);
 + clear_bit(cecdev-minor, cec_devnode_nums);
 + return ret;
 +}
 +
 +/**
 + * cec_devnode_unregister - unregister a cec device node
 + * @cecdev: the device node to unregister
 + *
 + * This unregisters the passed device. Future open calls will be met with
 + * errors.
 + *
 + * This function can safely be called if the device node has never been
 + * registered or has already been unregistered.
 + */
 +static void cec_devnode_unregister(struct cec_devnode *cecdev)
 +{
 + /* Check if cecdev was ever registered at all */
 + if (!cec_devnode_is_registered(cecdev))
 + return;

Just make it a programming error if someone unregisters something that
they haven't registered... that's pretty standard kernel programming.

 +
 + mutex_lock(cec_devnode_lock);
 + clear_bit(CEC_FLAG_REGISTERED, cecdev-flags);

This should wake up the poll waitqueue so that users get to hear about
the device going away in a timely manner.

 + mutex_unlock(cec_devnode_lock);
 + device_unregister(cecdev-dev);
 +}
 +
 +int cec_create_adapter(struct cec_adapter *adap, const char *name, u32 caps,
 +u8 ninputs, struct module *owner, struct device *parent)
 +{
 + int res = 0;
 +
 + adap-owner = owner;
 + if (WARN_ON(!owner))
 + return -ENXIO;
 + adap-devnode.parent = parent;
 + if (WARN_ON(!parent))
 + return -ENXIO;
 + adap-name = name;
 + adap-phys_addr = CEC_PHYS_ADDR_INVALID;
 + adap-capabilities = caps;
 + adap-ninputs = ninputs;
 + adap-is_source = caps  CEC_CAP_IS_SOURCE;
 + if (WARN_ON(!adap-ninputs  !adap-is_source))
 + return -ENXIO;
 + adap-cec_version = CEC_OP_CEC_VERSION_2_0;
 + adap-vendor_id = CEC_VENDOR_ID_NONE;
 + adap-available_log_addrs = 1;
 + adap-sequence = 0;
 + memset(adap-phys_addrs, 0xff, sizeof(adap-phys_addrs));
 + mutex_init(adap-lock);
 + INIT_LIST_HEAD(adap-transmit_queue);
 + INIT_LIST_HEAD(adap-wait_queue);
 + adap-kthread = kthread_run(cec_thread_func, adap, cec-%s, name);
 + init_waitqueue_head(adap-kthread_waitq);
 + if (IS_ERR(adap-kthread)) {
 + pr_err(cec-%s: kernel_thread() failed\n, name);
 + return PTR_ERR(adap-kthread);
 + }
 + if (caps) {
 + res = cec_devnode_register(adap-devnode, adap-owner);

Okay, so adap-devnode contains a struct device.  That struct device
controls the lifetime of adap-devnode, and because adap-devnode is
part of adap, this also defines the lifetime of adap as well.  adap
must _never_ be freed until cec_devnode_release() has been called.

Looking at patch 15, the adapter structure is part of the cobalt
streams.  This makes that structure also have a lifetime controlled
by this struct device.  There is no release method implemented in
there, and indeed cec_devnode_release() shows that the release node is
optional, which suggests a misunderstanding in this area.

Far too many nested data structures are involved here.  This needs fixing
- with the code in its present form, it contains serious data structure
lifetime issues, and therefore is not ready for 

Re: [PATCH v2 06/13] irqchip: kill off set_irq_flags usage

[Russell King - ARM Linux]

On Sun, Jul 12, 2015 at 06:43:56PM +0200, Thomas Gleixner wrote:
 The probe function was added in the initial implementation of the
 driver (2006), so it predates device tree.
 
 drivers/net/appletalk/ltpc.c
 drivers/net/arcnet/com20020-isa.c
 drivers/net/arcnet/com90io.c
 drivers/net/arcnet/com90xx.c
 
 Surely not stuff you find on todays ARM systems
 
 drivers/net/ethernet/8390/ne.c
 drivers/net/ethernet/8390/wd.c
 drivers/net/ethernet/amd/lance.c
 drivers/net/ethernet/amd/ni65.c
 drivers/net/ethernet/amd/pcnet32.c

pcnet32 is used on the Netwinder, which we still have supported in the ARM
tree.  Even worse, the Netwinder has the Cyberpro capture IRQ missing a
resistor, so it defaults to asserted and can trigger a stuck-IRQ, so
it's best not to allow probing of that known bad IRQ.

 Ditto
 
 drivers/net/ethernet/smsc/smc911x.c
 drivers/net/ethernet/smsc/smc9194.c
 drivers/net/ethernet/smsc/smc91x.c
 
 Those might still be, but on the DT based boards the probing should be
 completely irrelevant

SA11x0 stuff uses smc91x.c

 drivers/pcmcia/yenta_socket.c
 
 Russell might still use that.

Some EBSA285 systems use that, Compaq Personal Server (which is my wireless
AP using hostap) does.

ucb1x00.c definitely uses IRQ probing on SA11x0 platforms.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 06/13] irqchip: kill off set_irq_flags usage

[Russell King - ARM Linux]

On Thu, Jul 16, 2015 at 09:32:20PM +0200, Robert Jarzmik wrote:
 For PXA I must admit I don't yet know. I know lubbock has an UCB1400, but I
 don't have it working yet, so I can't foresee the problems yet.

The UCB1400 is an AC'97 device which is quite different from its
predecessors, and is not supported by the UCB1x00 driver.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/98] exynos_drm.h: use __u64 from linux/types.h

[Russell King - ARM Linux]

On Mon, Jun 01, 2015 at 11:08:21AM +0200, Christian König wrote:
 Yeah, completely agree with Linus on the visibility problem and that's
 exactly the reason why we don't include stdint.h in the kernel header and
 expect userspace to define the ISO types somewhere.
 
 But using the types from include/linux/types.h and especially including it
 into the uapi headers doesn't make the situation better, but rather worse.
 
 With this step we not only make the headers depend on another header that
 isn't part of the uapi, but also pollute the user space namespace with __sXX
 and __uXX types which aren't defined anywhere else.

1) Header files are permitted to pollute userspace with __-defined stuff.

2) __[su]XX types are defined as part of the kernel's uapi.
   include/uapi/linux/types.h includes asm/types.h, which in userspace
   picks up include/uapi/asm-generic/types.h.  That picks up
   asm-generic/int-ll64.h, which defines these types.

Moreover, this is done throughout the kernel headers _already_ (as you
would expect for a policy set 10+ years ago).

Please, I'm not interested in this discussion, so please don't argue
with me - I may agree with your position, but what you think and what
I think are really not relevant here.  If you have a problem, take it
up with Linus - I made that clear in my email.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/98] exynos_drm.h: use __u64 from linux/types.h

[Russell King - ARM Linux]

On Mon, Jun 01, 2015 at 10:20:10AM +0200, Christian König wrote:
 Using types that differs on 32-bit and 64-bit machines for a kernel
 interface is indeed a rather bad idea. This not only includes longs, but
 pointers as well.

[cut standard stdint.h types argument which we've heard before]

You need to read Linus' rant on this subject:

 From: Linus Torvalds torva...@osdl.org
 Subject: Re: [RFC] Splitting kernel headers and deprecating __KERNEL__
 Date: Mon, 29 Nov 2004 01:30:46 GMT

 Ok, this discussion has gone on for too long anyway, but let's make it
 easier for everybody. The kernel uses u8/u16/u32 because:

 - the kernel should not depend on, or pollute user-space naming.
   YOU MUST NOT USE uint32_t when that may not be defined, and
   user-space rules for when it is defined are arcane and totally
   arbitrary.

 - since the kernel cannot use those types for anything that is
   visible to user space anyway, there has to be alternate names.
   The tradition is to prepend two underscores, so the kernel would
   have to use __uint32_t etc for its header files.

 - at that point, there's no longer any valid argument that it's a
   standard type (it ain't), and I personally find it a lot more
   readable to just use the types that the kernel has always used:
   __u8/__u16/__u32. For stuff that is only used for the kernel,
   the shorter u8/u16/u32 versions may be used.

 In short: having the kernel use the same names as user space is ACTIVELY
 BAD, exactly because those names have standards-defined visibility, which
 means that the kernel _cannot_ use them in all places anyway. So don't
 even _try_.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/98] exynos_drm.h: use __u64 from linux/types.h

[Russell King - ARM Linux]

On Sat, May 30, 2015 at 05:37:57PM +0200, Mikko Rapeli wrote:
 Fixes userspace compilation error:
 
 drm/exynos_drm.h:30:2: error: unknown type name ‘uint64_t’
 
 Signed-off-by: Mikko Rapeli mikko.rap...@iki.fi

This is another thing which we need to address.  We should not be using
unsigned int/unsigned long/uintXX_t/etc in here, but the __uXX and __sXX
types.

The lesson learned from other DRM developers is that using these types
simplifies the API especially when it comes to the differences between
32-bit and 64-bit machines, and compat applications.

Note that drm/drm.h is all that should need to be included - drm/drm.h
takes care of including linux/types.h when building on Linux platforms.
(note: if your compiler doesn't set __linux__ then you're probably not
using the proper compiler...)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: fix exynos randconfig build error

[Russell King - ARM Linux]

On Tue, Mar 03, 2015 at 01:44:34PM +0100, Bartlomiej Zolnierkiewicz wrote:
 
 Hi,
 
 On Tuesday, March 03, 2015 04:40:02 AM Kukjin Kim wrote:
  On 02/27/15 06:30, Kukjin Kim wrote:
   On 02/25/15 20:46, Krzysztof Kozlowski wrote:
   2015-02-25 12:26 GMT+01:00 Russell King rmk+ker...@arm.linux.org.uk:
   The following error was observed with SMP=n in v4.0-rc1:
  
   arch/arm/mach-exynos/pm.c: In function 'exynos_cpu0_enter_aftr':
   arch/arm/mach-exynos/pm.c:246:4: error: implicit declaration of 
   function 'arch_send_wakeup_ipi_mask' 
   [-Werror=implicit-function-declaration]
  
   As the code unconditionally calls a function only available with SMP=y,
   make the Exynos PM support depend on SMP.
  
   Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
  
   Hi,
  
   Thanks for the patch but this already waits for Kukjin top be picked
   up. The first patch was similar to yours (adds dependency on SMP),
   sent on 4th of February:
   https://patchwork.ozlabs.org/patch/436231/
  
   But later Bartlomiej fixed this in other way (allowing to use cpuidle
   on non-SMP):
   https://patchwork.ozlabs.org/patch/436445/
  
   Unfortunately none of them were picked up.
  
   I've missed the fix, sorry.
   
   BTW, as you know, all of exynos SoCs are based on SMP so generally (in
   normal case) there is no reason to use non-SMP on exynos
   platforms...even though I understand the build error should be fixed...
   
   Anyway, I'll have a look Bart's patch and Russell's fix in this weekend.
   
  
  Firstly, let me take rmk's patch for the randconfig build error...BTW
 
 What is wrong with picking my patch instead?  It is non-invasive and fixes
 cpuidle support on UP (which is a regression from previous kernels)?
 
 https://lkml.org/lkml/2015/2/4/521
 
  I'm still wondering exynos stuff needs to support non-SMP and need to
  think more about its usefulness?...
 
 Currently UP is supported and at least I find it useful for testing/debug
 purposes.  If you want to to make Exynos SMP only thats OK but it should
 be done globally for Exynos arch support not just for cpuidle support.

Has either of these happened yet?  I don't see either in arm-soc.

(lkml.org seems to be - as seems typical - having problems.  The above
URL now returns a blank message.)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] video: treat signal like timeout as failure

[Russell King - ARM Linux]

On Tue, Mar 10, 2015 at 01:51:16PM +0100, Nicholas Mc Guire wrote:
 On Tue, 10 Mar 2015, Tomi Valkeinen wrote:
 
  On 20/01/15 07:23, Nicholas Mc Guire wrote:
   if(!wait_for_completion_interruptible_timeout(...))
   only handles the timeout case - this patch adds handling the
   signal case the same as timeout and cleans up.
   
   Signed-off-by: Nicholas Mc Guire der.h...@hofr.at
   ---
   
   Only the timeout case was being handled, return of 0 in 
   wait_for_completion_interruptible_timeout, the signal case (-ERESTARTSYS)
   was treated just like the case of successful completion, which is most 
   likely not reasonable.
   
   Note that exynos_mipi_dsi_wr_data/exynos_mipi_dsi_rd_data return values
   are not checked at the call sites in s6e8ax0.c (cmd_read/cmd_write)!
   
   This patch simply treats the signal case the same way as the timeout case,
   by releasing locks and returning 0 - which might not be the right thing to
   do - this needs a review by someone knowing the details of this driver.
  
  While I agree that this patch is a bit better than the current state,
  the code still looks wrong as Russell said.
  
  I can merge this, but I'd rather have someone from Samsung look at the
  code and change it to use wait_for_completion_killable_timeout() if
  that's what this code is really supposed to use.
 
 If someone that knows the details takes care of it
 that is of course the best solution. If someone Samsung is 
 going to look into it then it is probably best to completly
 drop this speculative patch so that this does not lead
 to more confusion than it does good.

IMHO, just change it to wait_for_completion_killable_timeout() - that's
a much better change than the change you're proposing.

If we think about it...  The current code uses this:

if (!wait_for_completion_interruptible_timeout(dsim_wr_comp,
MIPI_FIFO_TIMEOUT)) {
dev_warn(dsim-dev, command write timeout.\n);
mutex_unlock(dsim-lock);
return -EAGAIN;
}

which has the effect of treating a signal as success, and doesn't return
an error.  So, if the calling application receives (eg) a SIGPIPE or a
SIGALRM, we proceed as if we received the FIFO empty interrupt and doesn't
cause an error.

Your change results in:

timeout = wait_for_completion_interruptible_timeout(
dsim_wr_comp, MIPI_FIFO_TIMEOUT);
if (timeout = 0) {
dev_warn(dsim-dev,
command write timed-out/interrupted.\n);
mutex_unlock(dsim-lock);
return -EAGAIN;
}

which now means that this call returns -EAGAIN when a signal is raised.

Now, further auditing of this exynos crap (and I really do mean crap)
shows that this function is assigned to a method called cmd_write.
Grepping for that shows that *no caller ever checks the return value*!

So, really, there's a bug here in that we should _never_ complete on a
signal, and we most *definitely can not* error out on a signal either.
The *only* sane change to this code without author/maintainer input is
to change this to wait_for_completion_killable_timeout() - so that
signals do not cause either premature completion nor premature failure
of the wait.

The proper fix is absolutely huge: all call paths need to be augmented
with code to detect this function failing, and back out whatever changes
they've made, and restoring the previous state (if they can) and
propagate the error all the way back to userland, so that syscall
restarting can work correctly.  _Only then_ is it safe to use a call
which causes an interruptible sleep.

Personally, I'd be happier seeing this moved into drivers/staging and
eventually deleted from the kernel unless someone is willing to review
the driver and fix some of these glaring problems.  I wouldn't be
surprised if there was _loads_ of this kind of crap there.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] video: treat signal like timeout as failure

[Russell King - ARM Linux]

On Tue, Mar 10, 2015 at 03:39:28PM +0100, Nicholas Mc Guire wrote:
 On Tue, 10 Mar 2015, Russell King - ARM Linux wrote:
  On Tue, Mar 10, 2015 at 01:51:16PM +0100, Nicholas Mc Guire wrote:
   On Tue, 10 Mar 2015, Tomi Valkeinen wrote:
   
On 20/01/15 07:23, Nicholas Mc Guire wrote:
 if(!wait_for_completion_interruptible_timeout(...))
 only handles the timeout case - this patch adds handling the
 signal case the same as timeout and cleans up.
 
 Signed-off-by: Nicholas Mc Guire der.h...@hofr.at
 ---
 
 Only the timeout case was being handled, return of 0 in 
 wait_for_completion_interruptible_timeout, the signal case 
 (-ERESTARTSYS)
 was treated just like the case of successful completion, which is 
 most 
 likely not reasonable.
 
 Note that exynos_mipi_dsi_wr_data/exynos_mipi_dsi_rd_data return 
 values
 are not checked at the call sites in s6e8ax0.c (cmd_read/cmd_write)!
 
 This patch simply treats the signal case the same way as the timeout 
 case,
 by releasing locks and returning 0 - which might not be the right 
 thing to
 do - this needs a review by someone knowing the details of this 
 driver.

While I agree that this patch is a bit better than the current state,
the code still looks wrong as Russell said.

I can merge this, but I'd rather have someone from Samsung look at the
code and change it to use wait_for_completion_killable_timeout() if
that's what this code is really supposed to use.
   
   If someone that knows the details takes care of it
   that is of course the best solution. If someone Samsung is 
   going to look into it then it is probably best to completly
   drop this speculative patch so that this does not lead
   to more confusion than it does good.
  
  IMHO, just change it to wait_for_completion_killable_timeout() - that's
  a much better change than the change you're proposing.
  
  If we think about it...  The current code uses this:
  
  if 
  (!wait_for_completion_interruptible_timeout(dsim_wr_comp,
  MIPI_FIFO_TIMEOUT)) 
  {
  dev_warn(dsim-dev, command write timeout.\n);
  mutex_unlock(dsim-lock);
  return -EAGAIN;
  }
  
  which has the effect of treating a signal as success, and doesn't return
  an error.  So, if the calling application receives (eg) a SIGPIPE or a
  SIGALRM, we proceed as if we received the FIFO empty interrupt and doesn't
  cause an error.
  
  Your change results in:
  
  timeout = wait_for_completion_interruptible_timeout(
  dsim_wr_comp, MIPI_FIFO_TIMEOUT);
  if (timeout = 0) {
  dev_warn(dsim-dev,
  command write timed-out/interrupted.\n);
  mutex_unlock(dsim-lock);
  return -EAGAIN;
  }
  
  which now means that this call returns -EAGAIN when a signal is raised.
 
 but in case of wait_for_completion_killable_timeout it also would return
 -ERESTARTSYS (unless I'm missreading do_wait_for_common - 
 signal_pending_state(state, current)) so I still think it would be better to 
 have the
 dev_warn() in the path and then when the task is killed it atleast leaves
 some trace of the of what was going on ?
 
  
  Now, further auditing of this exynos crap (and I really do mean crap)
  shows that this function is assigned to a method called cmd_write.
  Grepping for that shows that *no caller ever checks the return value*!
 
 
 yup - as was noted in the patch - and this is also why it was
 not really possible to figure out what should really be done
 as it runs into a dead end in all cases - the only point of the patch was
 to atleast generate a debug message and return some signal
 indicating error ... which is then unhandled...
  
  So, really, there's a bug here in that we should _never_ complete on a
  signal, and we most *definitely can not* error out on a signal either.
  The *only* sane change to this code without author/maintainer input is
  to change this to wait_for_completion_killable_timeout() - so that
  signals do not cause either premature completion nor premature failure
  of the wait.
  
  The proper fix is absolutely huge: all call paths need to be augmented
  with code to detect this function failing, and back out whatever changes
  they've made, and restoring the previous state (if they can) and
  propagate the error all the way back to userland, so that syscall
  restarting can work correctly.  _Only then_ is it safe to use a call
  which causes an interruptible sleep.
  
  Personally, I'd be happier seeing this moved into drivers/staging and
  eventually deleted from the kernel unless someone is willing to review
  the driver and fix some of these glaring

Re: [PATCH] video: treat signal like timeout as failure

[Russell King - ARM Linux]

On Tue, Mar 10, 2015 at 04:55:56PM +0200, Tomi Valkeinen wrote:
 On 10/03/15 16:46, Russell King - ARM Linux wrote:
 
  In which case, let me propose that the exynos fbdev driver needs to be
  moved to drivers/staging, and stay there until this stuff gets fixed.
  drivers/staging is supposed to be for stuff which isn't up to the mark,
  and which is potentially unstable.  And that's what this driver exactly
  is.
 
 There is drivers/gpu/drm/exynos/ which is getting a lot of updates. So...
 
 I'd propose removing the exynos fbdev driver if the exynos drm driver
 offers the same functionality. I don't know if that's the case. Does the
 drm driver support all the devices the fbdev supports?
 
 Also, I'm not sure if and how we can remove drivers. If exynos fbdev
 driver is dropped, that would perhaps break boards that have exynos
 fbdev in their .dts file. And if the drm driver doesn't offer the exact
 same /dev/fbX interface, it would break the userspace.
 
 So I don't know if that's possible. But that's what I'd like to do,
 eventually, for all the fbdev drivers. Implement drm driver, remove the
 fbdev one.

That's why I suggested moving it to drivers/staging - it's a hint that
the driver needs a serious amount of work, and when built as a module,
it also provides users with the hint that the module they're loading is
of questionable quality (which is definitely the case here.)

Others have done that kind of thing before - we've had drivers which
have fallen by the way side, and at some point the decision has been
made to move them to drivers/staging, and if nothing happens to fix
them up (showing that no one cares about them), they've eventually
been dropped.

Of course, us talking about this might be enough to spur some effort
to get the thing properly fixed. :)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] drm/exynos: fix DMA_ATTR_NO_KERNEL_MAPPING usage

[Russell King - ARM Linux]

On Wed, Feb 04, 2015 at 11:20:19AM +0100, Marek Szyprowski wrote:
 diff --git a/drivers/gpu/drm/exynos/exynos_drm_buf.c 
 b/drivers/gpu/drm/exynos/exynos_drm_buf.c
 index 9c80884..24994ba 100644
 --- a/drivers/gpu/drm/exynos/exynos_drm_buf.c
 +++ b/drivers/gpu/drm/exynos/exynos_drm_buf.c
 @@ -63,11 +63,11 @@ static int lowlevel_buffer_allocate(struct drm_device 
 *dev,
  return -ENOMEM;
  }
 -buf-kvaddr = (void __iomem *)dma_alloc_attrs(dev-dev,
 +buf-cookie = dma_alloc_attrs(dev-dev,
  buf-size,
  buf-dma_addr, GFP_KERNEL,
  buf-dma_attrs);
 -if (!buf-kvaddr) {
 +if (!buf-cookie) {
  DRM_ERROR(failed to allocate buffer.\n);
  ret = -ENOMEM;
  goto err_free;

I wonder whether anyone has looked at what exynos is doing with this.

start_addr = buf-dma_addr;
while (i  nr_pages) {
buf-pages[i] = phys_to_page(start_addr);
start_addr += PAGE_SIZE;
i++;
}

There is no guarantee that DMA addresses are the same as physical addresses
in the kernel, so this is a layering violation.  If you want to do this,
then a better way to do this on ARM would be:

buf-pages[i] = pfn_to_page(dma_to_pfn(dev, 
start_addr));

The difference here is that dma_to_pfn() knows how to convert a dma_addr_t
to a PFN which can then be converted to a struct page (provided it is
backed by kernel managed system memory.)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 03/14] drm/bridge: make bridge registration independent of drm flow

[Russell King - ARM Linux]

On Fri, Jan 30, 2015 at 10:37:19AM -0500, Rob Clark wrote:
 On Tue, Jan 20, 2015 at 11:38 AM, Ajay Kumar ajaykumar...@samsung.com wrote:
 I'll also need to update the new bridge in the msm edp code..
 although that isn't such a big deal if I knew how this was *supposed*
 to work.. since what is there now at least doesn't look right..

I wonder whether the new dw-hdmi bridge code get fixed up too...

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 0/7] Enable L2 cache support on Exynos4210/4x12 SoCs

[Russell King - ARM Linux]

On Wed, Jan 14, 2015 at 04:46:03PM +0100, Alexandre Belloni wrote:
 Hi,
 
 This patch set hasn't moved since while. We actually need patch 4 to
 properly configure prefetch on sama5d4. What would be needed to come to
 an agreement ?

What do you mean hasn't moved since a while - there has been movement.
It was discovered that it breaks OMAP4 platforms.

Since then, work has been done to resolve that breakage, and I've merged
the recent patch set into my tree for further regression testing, and
I'm going to push it out to linux-next later this week.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: regression: OMAP4 (next-20141204) (bisect to: ARM: 8208/1: l2c: Refactor the driver to use commit-like)

[Russell King - ARM Linux]

On Tue, Dec 23, 2014 at 12:00:00PM +0100, Marek Szyprowski wrote:
 I hope I did it right: https://lkml.org/lkml/2014/12/23/158
 Please test, because I have no access to Omap hardware.

Patch 1/8 looks like I'd expect it to.  Nishanth, please test with your
failing scenario, thanks.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: regression: OMAP4 (next-20141204) (bisect to: ARM: 8208/1: l2c: Refactor the driver to use commit-like)

[Russell King - ARM Linux]

On Thu, Dec 11, 2014 at 11:42:48AM +0100, Marek Szyprowski wrote:
 On 2014-12-11 10:29, Russell King - ARM Linux wrote:
 On Wed, Dec 10, 2014 at 10:42:33AM +0100, Marek Szyprowski wrote:
 I assume that now it won't be possible to get l2c patches back to -next,
 so I will resend them (again...) with the omap related fix.
 What, you mean you don't know the fundamental rules of kernel development?
 
 No one should ever dump any new code into linux-next during a merge
 window which is not a fix for a regression or a bug fix, period.
 
 Linus has in the past taken a snapshot of linux-next at the beginning
 of a merge window, and then threatened to refuse to merge anything that
 wasn't in his local snapshot, or which doesn't qualify as the above.
 
 So no, it won't be possible, because I play by the community rules when
 it comes to what gets merged and at what time in the cycle.
 
 I know the rules. It was just my whining, that it is yet another release
 cycle
 that got missed. It is really disappointing, that those patches have been
 floating for months and noone found issues related to different order of
 initialization. It took way to long to get them scheduled for testing in
 -next.

Right, so - we're now at -rc1, and we should see about queuing this up
sooner rather than later - in its fixed form.  From what I can see,
there's been little progress on the OMAP problem.

Nishanth - can we push OMAP over to using the generic DT L2C
initialisation (the one from init_IRQ in arch/arm/kernel/irq.c) and
kill the SoC specific stuff in arch/arm/mach-omap2/omap4-common.c ?

From what I can see, in the DT case, the only thing which is used
there is the ioremap() to provide omap4_get_l2cache_base() with
something to return.  Everything else - the initialisation of the
l2c_write_sec pointer, and the aux mask and values - can be specified
via the machine_desc struct.

That only leaves the non-DT stuff to worry about this, and from what I
understand, that's going to be removed soon.  If we're going to keep
the non-DT stuff, we should implement a new machine_desc hook for it
instead of hijacking one of the existing callbacks.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: regression: OMAP4 (next-20141204) (bisect to: ARM: 8208/1: l2c: Refactor the driver to use commit-like)

[Russell King - ARM Linux]

On Mon, Dec 22, 2014 at 11:12:42AM -0600, Nishanth Menon wrote:
 On Mon, Dec 22, 2014 at 11:04 AM, Russell King - ARM Linux
 li...@arm.linux.org.uk wrote:
  That only leaves the non-DT stuff to worry about this, and from what I
  understand, that's going to be removed soon.  If we're going to keep
  the non-DT stuff, we should implement a new machine_desc hook for it
  instead of hijacking one of the existing callbacks.
 
 none of the PL310 support requires non-DT. PL310 is needed for OMAP4
 and AM437x both of which are DT only.

Right, so the simple answer for the time being is to kill most of
omap_l2_cache_init(), leaving just the ioremap() behind.  Everything
else can go into the machine_desc structures, and OMAP4 and AM437x
can both benefit from initialising the L2 cache at exactly the same
point as most other platforms.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: regression: OMAP4 (next-20141204) (bisect to: ARM: 8208/1: l2c: Refactor the driver to use commit-like)

[Russell King - ARM Linux]

On Wed, Dec 10, 2014 at 10:42:33AM +0100, Marek Szyprowski wrote:
 I assume that now it won't be possible to get l2c patches back to -next,
 so I will resend them (again...) with the omap related fix.

What, you mean you don't know the fundamental rules of kernel development?

No one should ever dump any new code into linux-next during a merge
window which is not a fix for a regression or a bug fix, period.

Linus has in the past taken a snapshot of linux-next at the beginning
of a merge window, and then threatened to refuse to merge anything that
wasn't in his local snapshot, or which doesn't qualify as the above.

So no, it won't be possible, because I play by the community rules when
it comes to what gets merged and at what time in the cycle.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 04/15] regulator: add restrack support

[Russell King - ARM Linux]

On Thu, Dec 11, 2014 at 12:58:37PM +, Mark Brown wrote:
 I'd expect someone reading the change in the regulator API to have at
 least some idea how this fits in with the rest of the API and how to use
 it, and probably more importantly I'd expect to be able to understand
 why this is DT only.

Yep.

This is a repetitive problem, and I fully agree with your concern about
stuff which is supposed to be arch-independent being designed with only
DT in mind.

New core kernel features should *not* be designed with only DT in mind -
DT is not the only firmware description language which the kernel
supports.  Folk need to understand that if they design a new arch
independent kernel feature where the sole use case is with DT, that new
feature is probably going to get rejected, especially when it's
something as generic as resource tracking.

The world is not DT only.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 00/15] Resource tracking/allocation framework

[Russell King - ARM Linux]

On Wed, Dec 10, 2014 at 04:48:18PM +0100, Andrzej Hajda wrote:
 3. There are drivers which can work without specific resource, but if
   the resource becomes available/unavailable it can do some additional stuff.
   An example of such driver is DRM driver (more precisely drm_connector) -
   it can start without attached drm_panel, but if the panel becomes available 
 it
   can react by generating HPD event and start using it.

Bad example, and actually incorrect.  DRM connectors are referenced in
userspace by an IDR number, which can be re-used in the case of a
connector appearing, disappearing, and then a different connector
re-appearing.

DRM really is *not* safe to hotplug like this: DRM is more a card-level
thing, which is why we have the component helpers - which allow us to
merge several devices into one logical card-like device in a generic
manner.

DRM needs a stable picture of the CRTCs, encoders and connectors, which
should _never_ change during the lifetime of the DRM device.  Devices
attached to connectors can be hotplugged, but that's about the limit of
hot-plugging in DRM.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 00/15] Resource tracking/allocation framework

[Russell King - ARM Linux]

On Wed, Dec 10, 2014 at 06:23:49PM +0100, A H wrote:
 10 gru 2014 17:16 Russell King - ARM Linux li...@arm.linux.org.uk
 napisał(a):
 
  On Wed, Dec 10, 2014 at 04:48:18PM +0100, Andrzej Hajda wrote:
   3. There are drivers which can work without specific resource, but if
 the resource becomes available/unavailable it can do some additional
 stuff.
 An example of such driver is DRM driver (more precisely
 drm_connector) -
 it can start without attached drm_panel, but if the panel becomes
 available it
 can react by generating HPD event and start using it.
 
  Bad example, and actually incorrect.  DRM connectors are referenced in
  userspace by an IDR number, which can be re-used in the case of a
  connector appearing, disappearing, and then a different connector
  re-appearing.
 
 But it is not about reappearing of drm_connector, it is about reappearing
 of drm_panel which is fortunately not a part of drm driver. Connector here
 is only consumer of drm_panel which should have possibility to receive
 notifications on panel (dis-)appearance.

Okay, so that's exactly like a panel being hotplugged to the connector,
which should be fine.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: regression: OMAP4 (next-20141204) (bisect to: ARM: 8208/1: l2c: Refactor the driver to use commit-like)

[Russell King - ARM Linux]

On Mon, Dec 08, 2014 at 08:54:18PM +0900, Tomasz Figa wrote:
 2014-12-06 1:23 GMT+09:00 Russell King - ARM Linux li...@arm.linux.org.uk:
  Given where we are in the cycle (-final likely this weekend) the only
  thing we can do right now is to drop the patch set; exynos (and mvebu)
  will have to wait another cycle until this patch set (hopefully in a
  revised form) can be merged.
 
 Or a fix could be queued on top of this. Since (I believe) this series
 has been queued for 3.19, we have 6 or 7 RC releases ahead, which
 could be used for the purpose of fixing things (as they are supposed
 to?).

They were merged on 27th November, so they would've been in linux-next
from about last Monday.  Nishanth reported a failure on Friday, the
last week day before the merge window opens.  There's no time to try
and fix this before the merge window, which is why I decided to drop
it.  They have already been dropped.  They were dropped immediately
after my message on Friday was sent.  So they've not been in linux-next
over this past weekend.

We don't push known broken code in during the merge window.  The merge
window is the exact time when subtle bugs are likely to be introduced,
and is the exact time that you want bisect to work.  If we push known
broken patches into the kernel during that period, we break the ability
to use bisect to find problems, especially where it causes a platform
to become unbootable.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dmaengine: pl330: Set residue in tx_status callback

[Russell King - ARM Linux]

On Mon, Dec 08, 2014 at 06:37:27PM +0530, Vinod Koul wrote:
 I actually like the split model, you can also prepare txn ahead of time and
 submit them when needed.

Actually, you can't - that's not permitted.  I have email(s) from Dan
explicitly stating that it is permitted for a driver to take a spinlock
in their prepare callback, and release it when the descriptor is
submitted.  Several DMA engine drivers (particularly those in for
async_tx) do exactly that.

The reason that submit is separate from prepare is to allow DMA engine
users to set a callback - if it weren't for that, there wouldn't be a
submit step, prepare would have done everything.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: regression: OMAP4 (next-20141204) (bisect to: ARM: 8208/1: l2c: Refactor the driver to use commit-like)

[Russell King - ARM Linux]

On Mon, Dec 08, 2014 at 07:54:08AM -0600, Nishanth Menon wrote:
 On 12/08/2014 06:22 AM, Russell King - ARM Linux wrote:
  On Mon, Dec 08, 2014 at 08:54:18PM +0900, Tomasz Figa wrote:
  2014-12-06 1:23 GMT+09:00 Russell King - ARM Linux 
  li...@arm.linux.org.uk:
  Given where we are in the cycle (-final likely this weekend) the only
  thing we can do right now is to drop the patch set; exynos (and mvebu)
  will have to wait another cycle until this patch set (hopefully in a
  revised form) can be merged.
 
  Or a fix could be queued on top of this. Since (I believe) this series
  has been queued for 3.19, we have 6 or 7 RC releases ahead, which
  could be used for the purpose of fixing things (as they are supposed
  to?).
  
  They were merged on 27th November, so they would've been in linux-next
  from about last Monday.  Nishanth reported a failure on Friday, the
 
 For what ever it is worth, the l2c changes actually appeared on
 Thursday CST (next-20141204). Found the regression against
 next-20141203 tag and it took me a day to track it down (after looking
 at a few other regressions as well)..

I wonder if that's because I didn't push my tree out until Tuesday-ish.
It takes around 48 hours between when I push stuff out to it appearing
in linux-next, which is not particularly desirable, but unavoidable due
to timezone differences.

I did decide that I wouldn't push it out the same day I merged it because
I wanted it to run through my build/boot system, which it did successfully,
but then my OMAP4 build doesn't have CPU idle enabled (and as you've
identified, when CPU idle is disabled, it works.)

The problem there is that if I decide not to push some merged changes out,
it takes several days before I get around to touching the kernel tree
again.  Hence why it took from Thursday until Tuesday for it to eventually
get pushed out.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dmaengine: pl330: Set residue in tx_status callback

[Russell King - ARM Linux]

On Wed, Dec 03, 2014 at 01:21:37PM +0530, Jassi Brar wrote:
 because the reasoning above seems incorrect considering the following
 documentation...
 
 Documentation/crypto/async-tx-api.txt says
   

async-tx-api is not the DMA slave API.

 Once a driver-specific threshold is met the driver
 automatically issues pending operations.  An application can force this
 event by calling async_tx_issue_pending_all(). 
   ^^

DMA slave users do not call this function.  This documentation is not
relevant for DMA slave.

 include/linux/dmaengine.h says
   dma_async_tx_descriptor.tx_submit(): set the prepared descriptor(s)
 to be executed by the engine

It doesn't say when.

 so theoretically a driver, not starting transfer until
 issue_pending(), is broken.

It isn't.  DMA slave engine implementations have been needing the
issue_pending() call since their dawn, so it's something that they've
always needed.

 At best the patch@04abf5daf7d makes the driver slightly more
 complicated and the reason behind confusion such as in this thread.

That may be, and yes, it _might_ be worth discussing whether this should
be relaxed or not, but that should be done as a proposal, not trying to
hide it as a bug fix.  It isn't.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: regression: OMAP4 (next-20141204) (bisect to: ARM: 8208/1: l2c: Refactor the driver to use commit-like)

[Russell King - ARM Linux]

On Fri, Dec 05, 2014 at 10:13:51AM -0600, Nishanth Menon wrote:
 On 12/05/2014 10:10 AM, Nishanth Menon wrote:
  Case #2: Reverting the following allows boot.
  
  From next-20141204
  10df7d5 ARM: 8211/1: l2c: Add support for overriding prefetch settings
  revert this  - boot still fails
  
  d42ced0 ARM: 8210/1: l2c: Get outer cache .write_sec callback from
  mach_desc only if not NULL
  revert this  - boot still fails
  
  46b9af8 ARM: 8209/1: l2c: Add interface to ask hypervisor to configure L2C
  revert this  - boot still fails
  
  c94e325 ARM: 8208/1: l2c: Refactor the driver to use commit-like
  revert this  - boot passed (first bad commit).
  
  
 
 + linux-samsung soc and updated Thomaz's mail ID (gmail now).

Given where we are in the cycle (-final likely this weekend) the only
thing we can do right now is to drop the patch set; exynos (and mvebu)
will have to wait another cycle until this patch set (hopefully in a
revised form) can be merged.

I think we need 8208/1 split up into smaller changes so that the cause
of this regression can be found.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 0/7] Enable L2 cache support on Exynos4210/4x12 SoCs

[Russell King - ARM Linux]

On Fri, Nov 28, 2014 at 12:11:38PM +0100, Arnd Bergmann wrote:
 I'm fine with it either way. Russell, if you like you can merge
 http://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung 
 v3.19-next/pm-samsung-2

It'd be nicer to have a git URL for it.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFT 2/2] arm: dts: disable CCI on exynos420 based arndale-octa

[Russell King - ARM Linux]

On Mon, Dec 01, 2014 at 10:03:28AM +0100, Krzysztof Kozlowski wrote:
 On pią, 2014-11-28 at 21:09 +0530, Abhilash Kesavan wrote:
  Hello Krzysztof,
  
  On Fri, Nov 28, 2014 at 8:49 PM, Krzysztof Kozlowski
  k.kozlow...@samsung.com wrote:
   On pią, 2014-11-28 at 20:20 +0530, Abhilash Kesavan wrote:
   The arndale-octa board was giving imprecise external aborts during
   boot-up with MCPM enabled. CCI enablement of the boot cluster was found
   to be the cause of these aborts (possibly because the secure f/w was not
   allowing it). Hence, disable CCI for the arndale-octa board.
  
   Signed-off-by: Abhilash Kesavan a.kesa...@samsung.com
   ---
arch/arm/boot/dts/exynos5420-arndale-octa.dts |4 
arch/arm/boot/dts/exynos5420.dtsi |2 +-
2 files changed, 5 insertions(+), 1 deletion(-)
  
   I tested these 2 patches on Arndale Octa but there are no improvements.
   I still got imprecise aborts (some not fatal and sometimes killing init
   with full backtrace).
  
  Thanks for testing. Are you testing this with exynos_defconfig with no
  other changes ? Can you please confirm from the bootlog that MCPM and
  CCI are not being initialized.
  
 That was exynos_defconfig with disabled DRM and enabled some debug,
 next-20141128.
 
 When I tried only exynos_defconfig (with disabled DRM) it worked fine...
 So the imprecise aborts were caused by one of following debug options:
 
 DEBUG_SECTION_MISMATCH
 DYNAMIC_DEBUG
 DEBUG_ATOMIC_SLEEP
 DEBUG_PREEMPT
 PROVE_LOCKING
 LOCKUP_DETECTOR
 DEBUG_LOCK_ALLOC
 PROVE_RCU
 DEBUG_RT_MUTEXES
 DEBUG_MUTEXES
 DEBUG_SPINLOCK
 DEBUG_LIST
 DEBUG_PAGEALLOC
 SPARSE_RCU_POINTER
 DEBUG_FS
 PM_DEBUG
 PM_ADVANCED_DEBUG
 GPIO_SYSFS
 
  Can you remove these 2 patches and on linux-next check if you are
  getting aborts even with 5420_MCPM disabled.
 
 I tried this already and imprecise aborts shown, however with my
 debugging options above.
 
 Overall the patches seems to work properly (although the debugging issue
 needs to be resolved still), so:
 
 On Arndale Octa (Exynos 5420):
 Tested-by: Krzysztof Kozlowski k.kozlow...@samsung.com

Reading this message, it seems that this should *not* be given a tested-by,
because it seems from what you've reported above, they don't work correctly.

If you have to turn debugging options off in order to get the kernel to
apparently run correctly after applying some patches, it means those
patches themselves are probably buggy, rather than the debug itself
being buggy.

I'd suggest that you have some further work to do (a manual bisect of the
config options you've disabled) to discover which is the cause of the
problem.

It could be that the code introduces something like a use-after-free bug.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/1] thermal: cpu_cooling: check for the readiness of cpufreq layer

[Russell King - ARM Linux]

On Fri, Nov 28, 2014 at 10:53:30AM -0400, Eduardo Valentin wrote:
 diff --git a/drivers/thermal/samsung/exynos_thermal_common.c 
 b/drivers/thermal/samsung/exynos_thermal_common.c
 index 3f5ad25..d4eaa1b 100644
 --- a/drivers/thermal/samsung/exynos_thermal_common.c
 +++ b/drivers/thermal/samsung/exynos_thermal_common.c
 @@ -371,9 +371,10 @@ int exynos_register_thermal(struct thermal_sensor_conf 
 *sensor_conf)
   th_zone-cool_dev[th_zone-cool_dev_size] =
   cpufreq_cooling_register(mask_val);
   if (IS_ERR(th_zone-cool_dev[th_zone-cool_dev_size])) {
 - dev_err(sensor_conf-dev,
 - Failed to register cpufreq cooling device\n);
 - ret = -EINVAL;
 + ret = 
 PTR_ERR(th_zone-cool_dev[th_zone-cool_dev_size]);
 + if (ret != -EPROBE_DEFER)
 + dev_err(sensor_conf-dev,
 + Failed to register cpufreq cooling 
 device\n);

Something which bugs me quite a lot is when there is an error code (which
tells you why something didn't work) and you have an error message, and
the error message doesn't bother printing the error code.

You might as well just print Failed\n and leave it at that, or md5sum
the error message and print the sum instead. :)

Knowing why something failed allows you to read the source, and find
possible reasons for the failure (which could come down to one reason)
and allows faster resolution of the problem.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 0/7] Enable L2 cache support on Exynos4210/4x12 SoCs

[Russell King - ARM Linux]

On Mon, Nov 17, 2014 at 12:48:22PM +0100, Marek Szyprowski wrote:
 This is an updated patchset, which intends to add support for L2 cache
 on Exynos4 SoCs on boards running under secure firmware, which requires
 certain initialization steps to be done with help of firmware, as
 selected registers are writable only from secure mode.
 
 First four patches extend existing support for secure write in L2C driver
 to account for design of secure firmware running on Exynos. Namely:
  1) direct read access to certain registers is needed on Exynos, because
 secure firmware calls set several registers at once,
  2) not all boards are running secure firmware, so .write_sec callback
 needs to be installed in Exynos firmware ops initialization code,
  3) write access to {DATA,TAG}_LATENCY_CTRL registers fron non-secure world
 is not allowed and so must use l2c_write_sec as well,
  4) on certain boards, default value of prefetch register is incorrect
 and must be overridden at L2C initialization.
 For boards running with firmware that provides access to individual
 L2C registers this series should introduce no functional changes. However
 since the driver is widely used on other platforms I'd like to kindly ask
 any interested people for testing.
 
 Further three patches add implementation of .write_sec and .configure
 callbacks for Exynos secure firmware and necessary DT nodes to enable
 L2 cache.
 
 Changes in this version tested on Exynos4412-based TRATS2 and OdroidU3+
 boards (both with secure firmware). There should be no functional change
 for Exynos boards running without secure firmware. I do not have access
 to affected non-Exynos boards, so I could not test on them.

So, I applied this series, and now I get a conflicts between my tree and
arm-soc for:

arch/arm/mach-exynos/firmware.c
arch/arm/mach-exynos/sleep.S

So, I'm going to un-stage the exynos bits, and we'll have to work out
some way to handle those.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/5] drivers: soc: Add support for Exynos PMU driver

[Russell King - ARM Linux]

On Thu, Nov 20, 2014 at 11:09:25AM +0530, Amit Daniel Kachhap wrote:
 diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
 index 063113d..44d220d 100644
 --- a/drivers/soc/Makefile
 +++ b/drivers/soc/Makefile
 @@ -6,3 +6,4 @@ obj-$(CONFIG_ARCH_QCOM)   += qcom/
  obj-$(CONFIG_ARCH_TEGRA) += tegra/
  obj-$(CONFIG_SOC_TI) += ti/
  obj-$(CONFIG_PLAT_VERSATILE) += versatile/
 +obj-$(CONFIG_ARCH_EXYNOS)+= samsung/

Is ARCH_EXYNOS appropriate here, or is your new SOC_SAMSUNG better?

 diff --git a/drivers/soc/samsung/Kconfig b/drivers/soc/samsung/Kconfig
 new file mode 100644
 index 000..a424ebc
 --- /dev/null
 +++ b/drivers/soc/samsung/Kconfig
 @@ -0,0 +1,20 @@
 +#
 +# SAMSUNG SOC drivers
 +#
 +menuconfig SOC_SAMSUNG
 + bool Samsung SOC drivers support

If you intend to select SOC_SAMSUNG, is there any point in making this
a user-visible symbol?

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] PM / Domains: Power on the PM domain right after attach completes

[Russell King - ARM Linux]

On Mon, Nov 17, 2014 at 04:19:10PM +0100, Ulf Hansson wrote:
 The amba bus, amba drivers and a vast amount of platform drivers which
 enables runtime PM, don't invoke a pm_runtime_get_sync() while probing
 their devices.
 
 Instead, once they have turned on their PM resourses during -probe()
 and are ready to handle I/O, these invokes pm_runtime_set_active() to
 synchronize its state towards the runtime PM core.

The above is misleading for amba.  The code sequence is:

pm_runtime_get_noresume(dev);
pm_runtime_set_active(dev);
pm_runtime_enable(dev);

ret = pcdrv-probe(pcdev, id);

AMBA drivers should never call pm_runtime_set_active(), as the runtime PM
state has already been initialised by the bus code.  Platform drivers are
different; the platform code provides zero help for runtime PM.

The sequence used by AMBA bus code is the sequence which was used by PCI
(as per commit f3ec4f87d607) at the time the runtime PM support was
written for AMBA.  PCI assumes that unbound devices are already powered
up, and as far as I'm aware, that's also true of AMBA devices as well.
I have yet to have access to a platform where this isn't true, neither
has anyone reported that such a platform exists.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] PM / Domains: Power on the PM domain right after attach completes

[Russell King - ARM Linux]

On Mon, Nov 17, 2014 at 06:54:44PM +0200, Grygorii Strashko wrote:
 I'd be very appreciated if you would be able to clarify one point to me as
 I'm not familiar with amba hw?
 
 I've found at least 2 AMBA drivers where secondary clock is used 
 to enable/disable device in addition to apb_pclk:
  - drivers/mmc/host/mmci.c 
  - drivers/spi/spi-pl022.c
 So, form the code point of view, the assumption that unbound AMBA
 devices are already powered up is not always true. And apb_pclk
 is just an interface clock in such cases. 

Right, what we're talking about here is that the device is accessible
to the CPU - that it is powered up and the bus clock to it is running,
so that accesses to the device will not cause a bus fault.

It does not mean that the external interface to the outside world is
functional - for example, there's no suggestion that the clock to the
SD/MMC card attached to MMCI would be running, or that the SD/MMC card
itself would be powered up, and the same goes for the clocks supplied
as functional clocks.  The driver is expected to manage these clocks
(see below.)

How this all works in the AMBA context is:

- we ensure that the apb_pclk is running by getting it, preparing and
  enabling it.

- the bus code calls pm_runtime_get_noresume() which increments the
  usage counter without calling the (as yet not bound) driver.

- if a PM domain is attached to the device, it should already be active
  and powered up.

- the device is marked active (as it should now be powered and bus-clocked.)

- runtime PM is enabled.

At this point, the device must be accessible to the CPU.

- the driver probe function is called, and it can go around getting any
  additional clocks it needs, and enabling or disabling them as it
  requires.  (For example, in the case of MMCI, a card may not be inserted
  in the slot, so the driver may decide to keep the functional MMCICLK
  disabled.)

- if the driver wishes to take part in runtime PM, it calls pm_runtime_put()
  or an equivalent function.  This balances the pm_runtime_get_noresume()
  above, and allows the runtime PM infrastructure to consider the device
  idle, and a candidate for runtime suspend.

Should the device runtime suspend, it's up to the driver to deal with the
functional clocks it is managing, because only it knows whether it kept
those clocks running, and disabling an already disabled clock is not
permitted.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Russell King - ARM Linux]

On Thu, Oct 30, 2014 at 11:01:02AM +0100, Andrzej Hajda wrote:
 On 10/29/2014 10:14 AM, Thierry Reding wrote:
  On Wed, Oct 29, 2014 at 09:57:02AM +0100, Daniel Vetter wrote:
  I think we nee try_get_module for the code and kref on the actual data
  structures.
  
  Agreed, that should do the trick. We'd probably need some sort of logic
  to also make operations return something like -ENODEV when the
  underlying device has disappeared. I think David had introduced
  something similar for DRM device not so long ago?
 
 If the underlying device disappears it would be good to receive
 notification anyway to trigger DRM HPD event. And if we have the
 notification, we can release references to the device smoothly. We do
 not need to play tricky games with krefs, zombie data and module
 refcounting.

Any solution which thinks it needs to lock modules into the core is
fundamentally broken.  It totally misses the point.

While you can lock a module into the core using try_get_module(), that
doesn't stop the device itself being unbound from a driver.  Soo many
people have fallen into that trap.  They write their device driver,
along with some kind of framework which they make use try_get_module().
They think its safe.  When you then echo the device name into the
driver's unbind sysfs file, all hell breaks loose and the kernel oopses.

try_get_module is /totally/ useless for ensuring that things stick around.

The reality is that you can't make devices stick around.  Once that
remove callback from the driver layer is called, that's it, the device
_is_ going away whether you like it or not.  You can't stop it.  It's
no good returning -EBUSY, because the remove return code is ignored.

What's more scarey is when you consider that in a real hotplug
situation, when the remove callback is called, the device has
/already/ gone.

So please, stop thinking that try_get_module() has some magic solution.
Any solution to device lifetimes using try_get_module() totally misses
the problem, and is just mere obfuscation and actually a bug in itself.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 4/7] ARM: l2c: Add support for overriding prefetch settings

[Russell King - ARM Linux]

On Mon, Oct 27, 2014 at 12:19:34PM +0100, Marek Szyprowski wrote:
 Hello,

 On 2014-10-27 12:14, Russell King - ARM Linux wrote:
 On Mon, Oct 27, 2014 at 12:05:47PM +0100, Marek Szyprowski wrote:
 From: Tomasz Figa t.f...@samsung.com

 Firmware on certain boards (e.g. ODROID-U3) can leave incorrect L2C prefetch
 settings configured in registers leading to crashes if L2C is enabled
 without overriding them. This patch introduces bindings to enable
 prefetch settings to be specified from DT and necessary support in the
 driver.

 Signed-off-by: Tomasz Figa t.f...@samsung.com
 [mszyprow: rebased onto v3.18-rc1, added error messages when property value
   is missing]
 Why?  What if the boot loader has already set these up appropriately?  Why
 should we force people to list these in the DT?

 The error message is displayed only when user provided prefetch related
 properties without any value (empty properties). Something that Mark Rutland
 requested here: https://lkml.org/lkml/2014/9/24/426 I'm sorry if I didn't
 describe it clearly enough.

Ok.

I'd ask for one change.  Please make all these messages start with
L2C-310 OF not PL310 OF:.  The device is described in ARM
documentation as a L2C-310 not PL310.  (Also note the : is dropped
too - most of the other messages don't have the : either.)

The:

PL310 OF: cache setting yield illegal associativity
PL310 OF: -1073346556 calculated, only 8 and 16 legal

message could also be changed to something like:

L2C-310 OF cache associativity %d invalid, only 8 or 16 permitted\n

Thanks.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 4/7] ARM: l2c: Add support for overriding prefetch settings

[Russell King - ARM Linux]

On Mon, Oct 27, 2014 at 12:05:47PM +0100, Marek Szyprowski wrote:
 From: Tomasz Figa t.f...@samsung.com
 
 Firmware on certain boards (e.g. ODROID-U3) can leave incorrect L2C prefetch
 settings configured in registers leading to crashes if L2C is enabled
 without overriding them. This patch introduces bindings to enable
 prefetch settings to be specified from DT and necessary support in the
 driver.
 
 Signed-off-by: Tomasz Figa t.f...@samsung.com
 [mszyprow: rebased onto v3.18-rc1, added error messages when property value
  is missing]

Why?  What if the boot loader has already set these up appropriately?  Why
should we force people to list these in the DT?

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V7 03/12] drm/bridge: Add helper functions for drm_bridge

[Russell King - ARM Linux]

On Mon, Oct 27, 2014 at 11:26:30PM +0100, Daniel Vetter wrote:
 Looking at the of_drm_find_panel function I actually wonder how that
 works - the drm_panel doesn't really need to stick around afaics.
 After all panel_list is global so some other driver can unload.
 Russell's of support for possible_crtcs code works differently since
 it only looks at per-drm_device lists.
 
 This bridge code here though suffers from the same. So to me this
 looks rather fishy.
 
 It doesn't help that we have drm_of.[hc] around but not all the of
 code is in there. Adding Russell too.

I rather dropped the ball with imx-drm when things became difficult with
asking Greg to pull my git tree - and as a result, I decided that I would
no longer be helping with patch submission for imx-drm, nor trying to get
it out of the staging tree anymore.

I do still have the patch (dated from July) in my git tree which adds it
to imx-drm - see below.  Rebased to 3.18-rc2 today.

I also have a patch (from April) which creates a generic OF DDC connector
helper, but that remains unfinished, in the sense that it lives beside
imx-drm, pulling the DDC specific code out of imx-hdmi and imx-tve, even
though there's nothing imx-drm specific about it.

8
From: Russell King rmk+ker...@arm.linux.org.uk
Subject: [PATCH] imx-drm: convert imx-drm to use the generic DRM OF helper

Use the generic DRM OF helper to locate the possible CRTCs for the
encoder, thereby shrinking the imx-drm driver some more.

Acked-by: Philipp Zabel p.za...@pengutronix.de
Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk
---
 drivers/staging/imx-drm/imx-drm-core.c | 72 ++
 1 file changed, 13 insertions(+), 59 deletions(-)

diff --git a/drivers/staging/imx-drm/imx-drm-core.c 
b/drivers/staging/imx-drm/imx-drm-core.c
index 9cb222e2996f..5e2c1f4b9165 100644
--- a/drivers/staging/imx-drm/imx-drm-core.c
+++ b/drivers/staging/imx-drm/imx-drm-core.c
@@ -24,6 +24,7 @@
 #include drm/drm_crtc_helper.h
 #include drm/drm_gem_cma_helper.h
 #include drm/drm_fb_cma_helper.h
+#include drm/drm_of.h
 
 #include imx-drm.h
 
@@ -47,7 +48,6 @@ struct imx_drm_crtc {
struct drm_crtc *crtc;
int pipe;
struct imx_drm_crtc_helper_funcsimx_drm_helper_funcs;
-   struct device_node  *port;
 };
 
 static int legacyfb_depth = 16;
@@ -366,9 +366,10 @@ int imx_drm_add_crtc(struct drm_device *drm, struct 
drm_crtc *crtc,
 
imx_drm_crtc-imx_drm_helper_funcs = *imx_drm_helper_funcs;
imx_drm_crtc-pipe = imxdrm-pipes++;
-   imx_drm_crtc-port = port;
imx_drm_crtc-crtc = crtc;
 
+   crtc-port = port;
+
imxdrm-crtc[imx_drm_crtc-pipe] = imx_drm_crtc;
 
*new_crtc = imx_drm_crtc;
@@ -409,34 +410,6 @@ int imx_drm_remove_crtc(struct imx_drm_crtc *imx_drm_crtc)
 }
 EXPORT_SYMBOL_GPL(imx_drm_remove_crtc);
 
-/*
- * Find the DRM CRTC possible mask for the connected endpoint.
- *
- * The encoder possible masks are defined by their position in the
- * mode_config crtc_list.  This means that CRTCs must not be added
- * or removed once the DRM device has been fully initialised.
- */
-static uint32_t imx_drm_find_crtc_mask(struct imx_drm_device *imxdrm,
-   struct device_node *endpoint)
-{
-   struct device_node *port;
-   unsigned i;
-
-   port = of_graph_get_remote_port(endpoint);
-   if (!port)
-   return 0;
-   of_node_put(port);
-
-   for (i = 0; i  MAX_CRTC; i++) {
-   struct imx_drm_crtc *imx_drm_crtc = imxdrm-crtc[i];
-
-   if (imx_drm_crtc  imx_drm_crtc-port == port)
-   return drm_crtc_mask(imx_drm_crtc-crtc);
-   }
-
-   return 0;
-}
-
 static struct device_node *imx_drm_of_get_next_endpoint(
const struct device_node *parent, struct device_node *prev)
 {
@@ -449,35 +422,16 @@ static struct device_node *imx_drm_of_get_next_endpoint(
 int imx_drm_encoder_parse_of(struct drm_device *drm,
struct drm_encoder *encoder, struct device_node *np)
 {
-   struct imx_drm_device *imxdrm = drm-dev_private;
-   struct device_node *ep = NULL;
-   uint32_t crtc_mask = 0;
-   int i;
+   uint32_t crtc_mask = drm_of_find_possible_crtcs(drm, np);
 
-   for (i = 0; ; i++) {
-   u32 mask;
-
-   ep = imx_drm_of_get_next_endpoint(np, ep);
-   if (!ep)
-   break;
-
-   mask = imx_drm_find_crtc_mask(imxdrm, ep);
-
-   /*
-* If we failed to find the CRTC(s) which this encoder is
-* supposed to be connected to, it's because the CRTC has
-* not been registered yet.  Defer probing, and hope that
-* the required CRTC is added later.
-*/
-   if (mask == 0)
-   return -EPROBE_DEFER;
-
-   crtc_mask |= mask;
-   

Re: PROBLEM: BUG appearing when trying to allocate interrupt on Exynos MCT after CPU hotplug

[Russell King - ARM Linux]

On Thu, Oct 23, 2014 at 03:51:16PM +0200, Marcin Jabrzyk wrote:
 [1.] One line summary of the problem: BUG: sleeping function called from
 invalid context at mm/slub.c:1250 after CPU hotplug

I'm really not surprised.

 When SoC have MCT_INT_SPI interrupt it is being allocated after hotplugging
 of the CPU, secondary_start_kernel() is sending CPU boot notifications which
 are send when preemption and interrupts are disabled. Exynos_mct
 notification handler tries to set up and allocate IRQ for SPI type interrupt
 for started CPU and then BUG appears.
 There might be similar problem on qcom-timer I think just after looking on
 the code.

The CPU notifier is called via notify_cpu_starting(), which is called
with interrupts disabled, and a reason code of CPU_STARTING.  Interrupts
at this point /must/ remain disabled.

The Exynos code then goes on to call exynos4_local_timer_setup() which
tries to reverse the free_irq() in exynos4_local_timer_stop() by calling
request_irq().  Calling request_irq() with interrupts off has never been
permissible.

So, this code is wrong today, and it was also wrong when it was written.
It /couldn't/ have been tested.  It looks like this commit added this
buggy code:

commit ee98d27df6827b5ba4bd99cb7d5cb1239b6a1a31
Author: Stephen Boyd sb...@codeaurora.org
Date:   Fri Feb 15 16:40:51 2013 -0800

ARM: EXYNOS4: Divorce mct from local timer API

Separate the mct local timers from the local timer API. This will
allow us to remove ARM local timer support in the near future and
gets us closer to moving this driver to drivers/clocksource.

Acked-by: Kukjin Kim kgene@samsung.com
Acked-by: Marc Zyngier marc.zyng...@arm.com
Cc: Thomas Abraham thomas.abra...@linaro.org
Signed-off-by: Stephen Boyd sb...@codeaurora.org

A good question would be: why doesn't this happen at boot time when CPU1
is first brought up?  The conditions here are no different from hotplugging
CPU1 back in.  Do you see a similar warning on boot too?

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drm/exynos: fix vblank handling during dpms off

[Russell King - ARM Linux]

On Thu, Oct 09, 2014 at 02:43:02PM +0900, Alexandre Courbot wrote:
 But there might be another issue, which is that calls to  
 drm_vblank_get() will return -EINVAL if invoked between drm_blank_off  
 and drm_blank_on. Is this really the desired behavior? Can it at least  
 happen? If so, how are drivers supposed to react to this situation?

I've not yet seen the commit which causes this problem, but I hope
that drm_wait_vblank() isn't affected by this.  In current mainline,
drm_vblank_get() is used inside drm_wait_vblank(), which is called as
a result of userspace calling DRM_IOCTL_WAIT_VBLANK.

So, what is the effect of this change on user applications making use
of the vblank wait ioctl - and is that change intended?

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC 0/4] drm/core: restore suspend/resume calbacks in KMS drm drivers

[Russell King - ARM Linux]

On Fri, Oct 03, 2014 at 01:39:21PM +0200, Daniel Vetter wrote:
 On Fri, Oct 3, 2014 at 11:42 AM, Andrzej Hajda a.ha...@samsung.com wrote:
  But this is an issue closely connected with component framework.
  Component framework separates master component probe and drm device
  initialization. As a result PM ops which are synchronized with probe
  (via device_lock)
  are no more synchronized with drm initialization which is usually called
  from
  .bind callback.
 
 Now I'm confused. component-bind and drm initialization is about
 driver load, but all this code here is about system suspend/resume
 really. So I'm rather confused what problem you actually want to fix
 ..

The component *helper* will disconnect the bind of the master device
from its probe if the components aren't already known.  If all the
components are known at the point the master device probes, the
master device lock will be held (since we'll be operating within the
master device's probe function.)

The issue here is that while the master device is being probed, the
per-device lock is held.  Beneath that lock there are locks in the
component helper which will be taken, and thus will end up depending
upon the per-device lock.

This means that we pretty much can not guarantee synchronisation between
PM on the master device and the component helper calling the bind
callback - if we tried to take the per-device lock here, we would either
deadlock, or we would end up with AB-BA lock dependencies.

What we /could/ do is expose lock/unlock functions from the component
helper so that PM implementations can synchronise with binds - or I could
look at whether we could integrate the component helper into the device
model.  I suspect the latter would not be met with enthusiasm as it would
mean adding bloat to the driver core structures, which would not be needed
in 99% of cases.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 4/7] ARM: l2c: Add support for overriding prefetch settings

[Russell King - ARM Linux]

On Wed, Sep 24, 2014 at 01:10:51PM +0100, Mark Rutland wrote:
 On Wed, Sep 24, 2014 at 12:19:45PM +0100, Tomasz Figa wrote:
  On 24.09.2014 13:14, Mark Rutland wrote:
   I'm not too keen on tristate properties. Is this level of flexibility
   really required?

I would say that we need a way to enable _and_ disable these features,
because:

1. When we converted the L2 code to DT, no one thought about creating
   a full and proper DT specification for the L2 code.  So, we have the
   situation today where platform code (and firmware code) enables or
   disables these features depending on platform knowledge.

2. If we are going to specify these options as a boolean-enable value,
   this implies that the lack of the boolean disables the option.
   We have pre-existing DT files which do not contain this option, but
   the platform may rely on the feature being enabled.

This is precisely the kind of mess that happens when incomplete DT
bindings are accepted.  DT bindings for any device should be _full_
bindings from the start, and not a half-hearted attempt at the job.

As much as we don't like tristate properties, we only have ourselves to
blame for them becoming a necessity in cases like this.

 With the lack of warnings for present but empty properties, I can forsee
 people placing empty properties (as the names make them sound like
 booleans which enable features).
 
 Surely for enabling/disabling options we should only need to override
 one-way, disabling a feature that causes breakage for some reason?
 Otherwise we can keep the reset value (which might be sub-optimal).

What if enabling prefetching on an existing platform causes a failure?
That's a regression on a previously working DT file, and needing a DT
update to resolve.

The obvious alternative is we have two properties per feature - one
boolean to enable, and one boolean to disable the feature.  We already
have a number of negative properties because of exactly the same
problem I mention above (where the driver has assumed defaults for one
platform which are not appropriate for all platforms.)

  As for why they cause crashes, I'm not an expert if it is about L2C
  internals, so I can't really tell, but apparently the cache can work
  correctly only on certain settings.
 
 Likewise, I'm no expert on the l2x0 family. It would be nice to know if
 this justs masks an issue elsewhere in Linux, is required for some
 reason by the interconnect, etc. I guess we don't have enough
 information to figure that out.

No, it would be nice to have some errata information...

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 4/7] ARM: l2c: Add support for overriding prefetch settings

[Russell King - ARM Linux]

On Fri, Sep 19, 2014 at 08:30:07PM +0200, Alexandre Belloni wrote:
 On 19/09/2014 at 17:39:32 +0100, Russell King - ARM Linux wrote :
  On Fri, Sep 19, 2014 at 11:50:01AM +0200, Alexandre Belloni wrote:
   On 26/08/2014 at 16:17:57 +0200, Tomasz Figa wrote :
Firmware on certain boards (e.g. ODROID-U3) can leave incorrect L2C 
prefetch
settings configured in registers leading to crashes if L2C is enabled
without overriding them. This patch introduces bindings to enable
prefetch settings to be specified from DT and necessary support in the
driver.

Signed-off-by: Tomasz Figa t.f...@samsung.com
   
   Tested-by: Alexandre Belloni alexandre.bell...@free-electrons.com
   
   It is working and useful on Atmel's sama5d4 were the bootloader is not
   configuring the L2C prefetch. However, I'm wondering whether we should
   add support for setting L310_PREFETCH_CTRL_DATA_PREFETCH and
   L310_PREFETCH_CTRL_INSTR_PREFETCH. I'm currently doing it by using
   .l2c_aux_val= L310_AUX_CTRL_DATA_PREFETCH |
   L310_AUX_CTRL_INSTR_PREFETCH (those are the same bits) but this has the
   disadvantage of displaying the L2C: platform modifies aux control
   register: twice.
  
  The L2C documentation, freely available from the ARM infocentre website,
  has the answer to this for you.
  
  The two bits in the prefetch control register which control the data
  and instruction prefetching are aliases of the aux control register.
  If you set them to a value in one register, they are reflected in the
  other.
  
  The reason for that is that once the L2 cache is enabled, writes to
  the aux control register are no longer permitted, but it's safe to
  enable and disable the prefetching with the cache already enabled.
  This reason is even stated in the documentation.
  
 
 Yeah, so my question still holds, should we have an other way to
 enable/disable I/D prefetch by adding two other DT bindings ?

Your question doesn't hold, because the above answers it conclusively.
No.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 4/7] ARM: l2c: Add support for overriding prefetch settings

[Russell King - ARM Linux]

On Fri, Sep 19, 2014 at 11:50:01AM +0200, Alexandre Belloni wrote:
 On 26/08/2014 at 16:17:57 +0200, Tomasz Figa wrote :
  Firmware on certain boards (e.g. ODROID-U3) can leave incorrect L2C prefetch
  settings configured in registers leading to crashes if L2C is enabled
  without overriding them. This patch introduces bindings to enable
  prefetch settings to be specified from DT and necessary support in the
  driver.
  
  Signed-off-by: Tomasz Figa t.f...@samsung.com
 
 Tested-by: Alexandre Belloni alexandre.bell...@free-electrons.com
 
 It is working and useful on Atmel's sama5d4 were the bootloader is not
 configuring the L2C prefetch. However, I'm wondering whether we should
 add support for setting L310_PREFETCH_CTRL_DATA_PREFETCH and
 L310_PREFETCH_CTRL_INSTR_PREFETCH. I'm currently doing it by using
 .l2c_aux_val= L310_AUX_CTRL_DATA_PREFETCH |
 L310_AUX_CTRL_INSTR_PREFETCH (those are the same bits) but this has the
 disadvantage of displaying the L2C: platform modifies aux control
 register: twice.

The L2C documentation, freely available from the ARM infocentre website,
has the answer to this for you.

The two bits in the prefetch control register which control the data
and instruction prefetching are aliases of the aux control register.
If you set them to a value in one register, they are reflected in the
other.

The reason for that is that once the L2 cache is enabled, writes to
the aux control register are no longer permitted, but it's safe to
enable and disable the prefetching with the cache already enabled.
This reason is even stated in the documentation.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Exynos build failure in -next allmodconfig

[Russell King - ARM Linux]

On Tue, Sep 16, 2014 at 01:44:44PM +0200, Krzysztof Kozłowski wrote:
 On 15.09.2014 19:57, Russell King - ARM Linux wrote:
 On Mon, Sep 15, 2014 at 09:34:58AM -0700, Mark Brown wrote:
 On Mon, Sep 15, 2014 at 11:57:09AM +0100, Build bot for Mark Brown wrote:

 Today's -next got a build failure in ARM allmodconfig due to platsmp.c:

 | arch/arm/mach-exynos/platsmp.c:198:31: warning: incorrect type in return 
 expression (different address spaces)
 | arch/arm/mach-exynos/platsmp.c:198:31:expected void [noderef] asn:2*
 | arch/arm/mach-exynos/platsmp.c:198:31:got void *
 | arch/arm/mach-exynos/platsmp.c:198:31: warning: incorrect type in return 
 expression (different address spaces)
 | arch/arm/mach-exynos/platsmp.c:198:31:expected void [noderef] asn:2*
 | arch/arm/mach-exynos/platsmp.c:198:31:got void *
 |   CC  arch/arm/mach-exynos/platsmp.o
 | /tmp/ccC9fkwF.s: Assembler messages:
 | /tmp/ccC9fkwF.s:423: Error: selected processor does not support ARM mode 
 `isb '
 | /tmp/ccC9fkwF.s:428: Error: selected processor does not support ARM mode 
 `isb '
 | /tmp/ccC9fkwF.s:429: Error: selected processor does not support ARM mode 
 `dsb '
 | scripts/Makefile.build:257: recipe for target 
 'arch/arm/mach-exynos/platsmp.o' failed

 Looks like we need a compiler flags override for that file.

 Or.. the question is why a .c file is not using the proper macros.

 Actually I am the one to blame for build failure (commit: ARM: EXYNOS:  
 Move code from hotplug.c to platsmp.c). The problem is  
 v7_exit_coherency_flush() which I think does not make sense on ARMv6.

 I'll replace the ISB and DSB commands with macros but the real question  
 is whether the mach-exynos/platsmp.c file and mach-exynos directory  
 should be compiled when CONFIG_ARCH_EXYNOS is not defined?

It's entirely possible that a kernel will be configured to support ARMv6
and ARMv7, which can also include exynos support.  In this case, it will
be built using compiler flags for ARMv6, since ARMv7 is compatible with
the ARMv6 ISA (even though a few instructions are deprecated.)

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 5/7] ARM: EXYNOS: Add .write_sec outer cache callback for L2C-310

[Russell King - ARM Linux]

On Tue, Aug 26, 2014 at 04:17:58PM +0200, Tomasz Figa wrote:
 Exynos4 SoCs equipped with an L2C-310 cache controller and running under
 secure firmware require certain registers of aforementioned IP to be
 accessed only from secure mode. This means that SMC calls are required
 for certain register writes. To handle this, an implementation of
 .write_sec and .configure callbacks is provided by this patch.
 
 Signed-off-by: Tomasz Figa t.f...@samsung.com
 ---
  arch/arm/mach-exynos/firmware.c | 38 ++
  1 file changed, 38 insertions(+)
 
 diff --git a/arch/arm/mach-exynos/firmware.c b/arch/arm/mach-exynos/firmware.c
 index f5e626d..554b350 100644
 --- a/arch/arm/mach-exynos/firmware.c
 +++ b/arch/arm/mach-exynos/firmware.c
 @@ -17,6 +17,7 @@
  #include asm/cacheflush.h
  #include asm/cputype.h
  #include asm/firmware.h
 +#include asm/hardware/cache-l2x0.h
  #include asm/suspend.h
  
  #include mach/map.h
 @@ -120,6 +121,31 @@ static const struct firmware_ops exynos_firmware_ops = {
   .resume = exynos_resume,
  };
  
 +static void exynos_l2_write_sec(unsigned long val, unsigned reg)
 +{
 + switch (reg) {
 + case L2X0_CTRL:
 + if (val  L2X0_CTRL_EN)
 + exynos_smc(SMC_CMD_L2X0INVALL, 0, 0, 0);

If we're calling this with the cache already enabled, presumably you're
doing this to cover the case where we're disabling the cache.

1. Do you really want to *invalidate* the L2 cache, discarding its
   contents?
2. Don't you think that... if you needed something like this here, then
   it could be a defficiency in the common code?

If (2) doesn't apply, then should be a comment here why this is needed.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 6/7] ARM: EXYNOS: Add support for non-secure L2X0 resume

[Russell King - ARM Linux]

On Tue, Aug 26, 2014 at 04:17:59PM +0200, Tomasz Figa wrote:
 On Exynos SoCs it is necessary to resume operation of L2C early in
 assembly code, because otherwise certain systems will crash. This patch
 adds necessary code to non-secure resume handler.
 
 Signed-off-by: Tomasz Figa t.f...@samsung.com
 ---
  arch/arm/mach-exynos/common.h   |  1 +
  arch/arm/mach-exynos/firmware.c |  4 +++-
  arch/arm/mach-exynos/sleep.S| 41 
 +
  3 files changed, 45 insertions(+), 1 deletion(-)
 
 diff --git a/arch/arm/mach-exynos/common.h b/arch/arm/mach-exynos/common.h
 index c218200..e88c0f9 100644
 --- a/arch/arm/mach-exynos/common.h
 +++ b/arch/arm/mach-exynos/common.h
 @@ -113,6 +113,7 @@ IS_SAMSUNG_CPU(exynos5800, EXYNOS5800_SOC_ID, 
 EXYNOS5_SOC_MASK)
  
  extern u32 cp15_save_diag;
  extern u32 cp15_save_power;
 +extern unsigned long l2x0_regs_phys;
  
  extern void __iomem *sysram_ns_base_addr;
  extern void __iomem *sysram_base_addr;
 diff --git a/arch/arm/mach-exynos/firmware.c b/arch/arm/mach-exynos/firmware.c
 index 554b350..71bcfbd 100644
 --- a/arch/arm/mach-exynos/firmware.c
 +++ b/arch/arm/mach-exynos/firmware.c
 @@ -102,7 +102,9 @@ static int exynos_suspend(void)
   writel(EXYNOS_SLEEP_MAGIC, sysram_ns_base_addr + EXYNOS_BOOT_FLAG);
   writel(virt_to_phys(exynos_cpu_resume_ns),
   sysram_ns_base_addr + EXYNOS_BOOT_ADDR);
 -
 +#ifdef CONFIG_CACHE_L2X0
 + l2x0_regs_phys = virt_to_phys(l2x0_saved_regs);
 +#endif

NAK.  Please look at how arch/arm/mm/l2c-l2x0-resume.S gets the address
of this structure in assembly code.  The name of this variable is crap
in any case.  It's not the registers, it's the saved registers.  So even
more reason to kill this abomination, which incidentally, I've already
killed off once before in the exynos code.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Exynos build failure in -next allmodconfig

[Russell King - ARM Linux]

On Mon, Sep 15, 2014 at 09:34:58AM -0700, Mark Brown wrote:
 On Mon, Sep 15, 2014 at 11:57:09AM +0100, Build bot for Mark Brown wrote:
 
 Today's -next got a build failure in ARM allmodconfig due to platsmp.c:
 
 | arch/arm/mach-exynos/platsmp.c:198:31: warning: incorrect type in return 
 expression (different address spaces)
 | arch/arm/mach-exynos/platsmp.c:198:31:expected void [noderef] asn:2*
 | arch/arm/mach-exynos/platsmp.c:198:31:got void *
 | arch/arm/mach-exynos/platsmp.c:198:31: warning: incorrect type in return 
 expression (different address spaces)
 | arch/arm/mach-exynos/platsmp.c:198:31:expected void [noderef] asn:2*
 | arch/arm/mach-exynos/platsmp.c:198:31:got void *
 |   CC  arch/arm/mach-exynos/platsmp.o
 | /tmp/ccC9fkwF.s: Assembler messages:
 | /tmp/ccC9fkwF.s:423: Error: selected processor does not support ARM mode 
 `isb '
 | /tmp/ccC9fkwF.s:428: Error: selected processor does not support ARM mode 
 `isb '
 | /tmp/ccC9fkwF.s:429: Error: selected processor does not support ARM mode 
 `dsb '
 | scripts/Makefile.build:257: recipe for target 
 'arch/arm/mach-exynos/platsmp.o' failed
 
 Looks like we need a compiler flags override for that file.

Or.. the question is why a .c file is not using the proper macros.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v11 11/19] irqchip: vic: Add support for FIQ management

[Russell King - ARM Linux]

On Tue, Sep 02, 2014 at 02:00:45PM +0100, Daniel Thompson wrote:
 diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
 index bda5a91..8821160 100644
 --- a/drivers/irqchip/irq-gic.c
 +++ b/drivers/irqchip/irq-gic.c
 @@ -502,13 +502,17 @@ static void __init gic_init_fiq(struct gic_chip_data 
 *gic,
  /*
   * Fully acknowledge (both ack and eoi) a FIQ-based IPI
   */
 -static int gic_handle_fiq_ipi(struct notifier_block *nb, unsigned long regs,
 -void *data)
 +void gic_handle_fiq_ipi(void)
  {
   struct gic_chip_data *gic = gic_data[0];
 - void __iomem *cpu_base = gic_data_cpu_base(gic);
 + void __iomem *cpu_base;
   unsigned long irqstat, irqnr;
  
 + if (!gic || !gic-fiq_enable)
 + return;
 +
 + cpu_base = gic_data_cpu_base(gic);
 +
   if (WARN_ON(!in_nmi()))
   return NOTIFY_BAD;
  
 @@ -525,13 +529,6 @@ static int gic_handle_fiq_ipi(struct notifier_block *nb, 
 unsigned long regs,
  
   return NOTIFY_OK;
  }
 -
 -/*
 - * Notifier to ensure IPI FIQ is acknowledged correctly.
 - */
 -static struct notifier_block gic_fiq_ipi_notifier = {
 - .notifier_call = gic_handle_fiq_ipi,
 -};
  #else /* CONFIG_FIQ */
  static inline void gic_set_group_irq(void __iomem *base, unsigned int hwirq,
int group) {}
 @@ -1250,10 +1247,6 @@ void __init gic_init_bases(unsigned int gic_nr, int 
 irq_start,
  #ifdef CONFIG_SMP
   set_smp_cross_call(gic_raise_softirq);
   register_cpu_notifier(gic_cpu_notifier);
 -#ifdef CONFIG_FIQ
 - if (gic_data_fiq_enable(gic))
 - register_fiq_nmi_notifier(gic_fiq_ipi_notifier);
 -#endif
  #endif
   set_handle_irq(gic_handle_irq);
   }

Shouldn't this be merged into some other patch?

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ARM: EXYNOS: Fix build with PM_SLEEP=n part #2

[Russell King - ARM Linux]

On Mon, Jul 14, 2014 at 04:43:59PM +0200, Bartlomiej Zolnierkiewicz wrote:
 Fix building of exynos_defconfig with disabled CONFIG_PM_SLEEP by
 adding checking whether Exynos cpuidle support is enabled before
 accessing exynos_enter_aftr.
 
 The build error message:
 arch/arm/mach-exynos/built-in.o:(.data+0x74): undefined reference to 
 `exynos_enter_aftr'
 make: *** [vmlinux] Error 1
 
 Signed-off-by: Bartlomiej Zolnierkiewicz b.zolnier...@samsung.com
 Acked-by: Kyungmin Park kyungmin.p...@samsung.com

What's happening with this patch?  I'm still seeing failures in my builds
due to the above error from Tuesday's arm-soc.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/7] ARM: l2c: Refactor the driver to use commit-like interface

[Russell King - ARM Linux]

On Thu, Jul 17, 2014 at 06:38:56PM +0200, Tomasz Figa wrote:
 Certain implementations of secure hypervisors (namely the one found on
 Samsung Exynos-based boards) do not provide access to individual L2C
 registers. This makes the .write_sec()-based interface insufficient and
 provoking ugly hacks.
 
 This patch is first step to make the driver not rely on availability of
 writes to individual registers. This is achieved by refactoring the
 driver to use a commit-like operation scheme: all register values are
 prepared first and stored in an instance of l2x0_regs struct and then a
 single callback is responsible to flush those values to the hardware.

This isn't going to work very well...

 +static const struct l2c_init_data *l2x0_data;

So you keep a pointer to the init data...

 +static void l2c_resume(void)
 +{
 + l2x0_data-enable(l2x0_base, l2x0_saved_regs.aux_ctrl,
 + l2x0_data-num_lock);

which you dereference at resume time...

  static const struct l2c_init_data l2c210_data __initconst = {

but the structures which get assigned to the pointer are marked __initconst.

That's not going to work very well.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: config: update multi_v7_defconfig

[Russell King - ARM Linux]

On Fri, Jul 18, 2014 at 12:31:48AM +0200, Javier Martinez Canillas wrote:
 In the case of the MAX77686, the mfd, regulator and clock drivers use
 subsys_initcall() instead of module_init() but I wonder which of these
 really need to be initialized at subsys init call time and which one
 can just be built as a module.

I think you're making a frequently made mistake concerning module
initialisation.

Having code initialised by subsys_initcall() (or indeed any other level)
does not preclude it being a module:

#ifndef MODULE
...
#define subsys_initcall(fn) __define_initcall(fn, 4)
...
#else /* MODULE */
...
#define subsys_initcall(fn) module_init(fn)
...
#endif

So, subsys_initcall() automagically becomes module_init() when built
as a module.  There's no need to change anything here.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] cpuidle fixes and cleanup

[Russell King - ARM Linux]

On Tue, Jul 08, 2014 at 03:56:48PM +0200, Tomasz Figa wrote:
 On 02.07.2014 05:11, Chander Kashyap wrote:
  On Tue, Jul 1, 2014 at 8:22 PM, Russell King - ARM Linux
  li...@arm.linux.org.uk wrote:
  On Tue, Jul 01, 2014 at 08:02:36PM +0530, Chander Kashyap wrote:
  This patch series fixes the cpuidle for different states. Also removes arm
  diagnostic and power register save/restore code as it is made generic.
 
  Based on:
  ARM: save/restore diagnostic register on Cortex-A9 suspend/resume
   http://www.spinics.net/lists/arm-kernel/msg340506.html
 
 [Ccing people who participated in discussion in that thread]
 
 
  As there is an outstanding issue with this patch, we can't proceed with
  this set of changes until we know what's going on there.
  
  Sure, I will wait for the conclusion.
 
 So I believe the conclusion was that this can't be handled in generic
 way, because on systems running in non-secure mode writing to those
 registers faults.

That is, unfortunately, correct.

There is a way around this, but people aren't going to like it.  That
is - we move it to the suspend and resume paths anyway.

In the resume path, we read the register, compare it with the value
which we would update it to, and if it's identical, we avoid the write.

This gets secure-mode platforms working.

For non-secure mode platforms, we have to require them to insert some
assembly code into the early resume path which calls their secure
monitor to restore these registers before continuing on to cpu_resume,
thereby ensuring that the CPU specific code sees that the value in the
register is identical with the saved value, and omitting the write.

This isn't really a new principle - we already have this requirement
for non-secure mode platforms when they're booting a kernel with
various errata enabled.

I can't see any other alternative which satisfies everyone (by
everyone, I'm including the requirements from the hardware folk who
expect these registers to be static once the MMU is enabled.)

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ABBA deadlock in Common Clock Framework

[Russell King - ARM Linux]

On Wed, Jul 02, 2014 at 12:59:04PM +0200, Tomasz Figa wrote:
 Hi All,
 
 While testing linux-next (next-20140625) on Exynos4412-based TRATS2
 board, from time to time I hit a deadlock between clk_disable_unused()
 of Common Clock Framework and parallel clk_prepare() from s3c24xx-i2c
 driver.

This is pretty sad.  The Linux kernel has quite a range of truely excellent
debugging tools which can be built in, but it seems many developers don't
enable them.  The important one here is lockdep (which I notice isn't on
in your kernel.)

Lockdep is a static lock checker - it tracks the dependencies and contexts
between various locks and can report whether deadlock is possible without
having to run into the deadlock.  It is /highly/ recommended that all
developers should run their changes through a kernel with this feature
on before submitting them upstream - see Documentation/SubmitChecklist
point 15.

It can really catch these things before the patch is even submitted...
The recommendation is that if you're doing kernel development, always
have lockdep enabled.  If you want to do performance checking, then
obviously it has an impact on that, so turn it off to do that, but
remember to turn it back on before you do further development.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] cpuidle fixes and cleanup

[Russell King - ARM Linux]

On Tue, Jul 01, 2014 at 08:02:36PM +0530, Chander Kashyap wrote:
 This patch series fixes the cpuidle for different states. Also removes arm
 diagnostic and power register save/restore code as it is made generic.
 
 Based on:
 ARM: save/restore diagnostic register on Cortex-A9 suspend/resume
  http://www.spinics.net/lists/arm-kernel/msg340506.html

As there is an outstanding issue with this patch, we can't proceed with
this set of changes until we know what's going on there.

Also, bear in mind that I have changes which conflict with this in my
tree (which I've just updated.)

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: convert all mov.* pc, reg to bx reg for ARMv6+ (part1)

[Russell King - ARM Linux]

On Tue, Jul 01, 2014 at 05:42:42PM +0100, Måns Rullgård wrote:
 Russell King rmk+ker...@arm.linux.org.uk writes:
 
  ARMv6 and greater introduced a new instruction (bx) which can be used
  to return from function calls.  Recent CPUs perform better when the
  bx lr instruction is used rather than the mov pc, lr instruction,
  and this sequence is strongly recommended to be used by the ARM
  architecture manual (section A.4.1.1).
 
  We provide a new macro ret with all its variants for the condition
  code which will resolve to the appropriate instruction.
 
 When the source register is not lr the name ret is a misnomer since
 only the bx lr instruction is predicted as a function return.  The
 bx instruction with other source registers uses the normal prediction
 mechanisms, leaving the return stack alone, and should not be used for
 function returns.  Any code currently using another register to return
 from a function should probably be modified to use lr instead, unless
 there are special reasons for doing otherwise.  If code jumping to an
 address in a non-lr register is not a return, using the ret name will
 make for some rather confusing reading.
 
 I would suggest either using a more neutral name than ret or adding an
 alias to be used for non-return jumps so as to make the intent clearer.

If you read the patch, the branches which are changed are those which
do indeed return in some way.  There are those which do this having
moved lr to a different register.

As you point out, bx lr /may/ be treated specially (I've actually been
discussing this with Will Deacon over the last couple of days, who has
also been talking to the hardware people in ARM, and Will is happy with
this patch as in its current form.)  This is why I've changed all
mov pc, reg instructions which return in some way to use this macro,
and left others (those which are used to call some function and return
back to the same point) alone.

I have thought about introducing a call macro for those other sites,
but as there are soo few of them, I've left them as-is for the time
being (this patch is already large enough.)

If there are any in the patch which you have specific concerns about,
please specifically point them out those which give you a concern.

Thanks.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 05/14] clk: Add generic driver for Maxim PMIC clocks

[Russell King - ARM Linux]

On Mon, Jun 30, 2014 at 12:58:57PM +0200, Javier Martinez Canillas wrote:
  +   if (!max_gen-lookup)
  +   return ERR_PTR(-ENOMEM);
  +
  +   max_gen-lookup-con_id = hw-init-name;
  
  Also IMO,  init-name should be over-written if name is provided in DT,
  otherwise generic clock-output-names property will go futile,
  perhaps it should be done before clk_register.
  
 
 Even though Documentation/devicetree/bindings/clock/clock-bindings.txt says 
 that
 the clock-output-names property is optional I agree with you that will be
 better to support it. So I'll add it on the next version as well.

However, remember that con_id is the _DEVICE_ specific connection name,
not the _CLOCK_ name.  You will get a NAK from me if you violate this
rule.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mainline boot: 64 boots: 62 pass, 2 fail (v3.16-rc1-2-gebe0618)

[Russell King - ARM Linux]

On Fri, Jun 27, 2014 at 02:09:58AM -0700, Laura Abbott wrote:
 On 6/26/2014 8:06 PM, Tushar Behera wrote:
 arm_add_memory is getting called before this is being set, resulting in
 none of the memory banks getting added[1].

 setup_machine_fdt - early_init_dt_scan - early_init_dt_scan_memory

 Would it make sense to re-introduce the config option ARM_NR_BANKS and
 replace max_cnt with NR_BANKS?

 [1] http://pastebin.com/MawYD7kb


 I was hoping to avoid re-introducing the config option but that may be
 the case if we can't make the machine_info work. I'll take a better
 look tomorrow.

The problem with the config option is that it's not single zImage
friendly.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] drm: Constify struct drm_fb_helper_funcs

[Russell King - ARM Linux]

On Fri, Jun 27, 2014 at 05:19:23PM +0200, Thierry Reding wrote:
 From: Thierry Reding tred...@nvidia.com
 
 There's no need for this to be modifiable. Make it const so that it can
 be put into the .rodata section.
 
 Reviewed-by: Daniel Vetter daniel.vet...@ffwll.ch
 Signed-off-by: Thierry Reding tred...@nvidia.com

Definitely a good thing.  For Armada:

Acked-by: Russell King rmk+ker...@arm.linux.org.uk

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] Deterministic UART numbering on Samsung SoCs

[Russell King - ARM Linux]

On Thu, Jun 26, 2014 at 01:24:32PM +0200, Tomasz Figa wrote:
 Current Samsung UART driver relies on probe order of particular
 samsung-uart instances, which makes it impossible to get proper
 initialization of ports when not all ports are available on board,
 not even saying of deterministic device naming.
 
 This series intends to fix this situation by adding support to parse
 aliases from device tree and use them to assign instance IDs to
 particular port instances.

How about instead exporting the path/id information so that userspace
can create /dev/serial/by-{path,id}/... for internal devices instead?

The problem you're raising is very much the same problem you have when
there are multiple USB serial devices connected to the machine - you
just get a bunch of /dev/ttyUSB* devices which are unordered (they can
change on each boot, or change order if you disconnect and reconnect
them.)

/dev/serial/by-{path,id}/ allows for a much more stable path.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: mainline boot: 64 boots: 62 pass, 2 fail (v3.16-rc1-2-gebe0618)

[Russell King - ARM Linux]

On Thu, Jun 26, 2014 at 07:59:19AM -0700, Kevin Hilman wrote:
 I agree that the u-boot bug needs to be fixed, and FWIW, I updated my
 u-boot and haven't seen the boot failure yet after several boots with
 next-20140625.
 
 That being said, since it's not always feasible/practical to update
 u-boot, and when it comes down to it, this is still a kernel
 regression, we should also fix the kernel to sanity check the values
 coming from u-boot, like it was doing before.

It wasn't sanity checking the values (there is some sanity checking,
but the sanity checking doesn't catch this).

What caught it was that the kernel was configured to only look at the
first 8 of the 12 meminfo entries with ATAGs.  Since we no longer have
that limit, all meminfo entries are now looked at (since the kernel
doesn't need the limit.)

We could add back a soft-limit on the number of meminfo entries, but
this has to be platform specific.  Another entry to go into the
mach_info structures?

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] arm: exynos: Modify pm code to check for cortex A9 rather than the SoC

[Russell King - ARM Linux]

On Wed, Jun 25, 2014 at 10:30:46AM +0530, Abhilash Kesavan wrote:
 I see that you have sent a patch out that ensures both part and
 implementor number are checked. Currently, my patch has been applied
 to the fixes branch of the arm-soc tree and I wanted to know how to
 proceed (without it there is a crash on the 5420):
 Should I request Arnd to drop it (if possible) and send out a new
 patch using your updated function ?

I'll hold my patch off - as yours is in the fixes branch, it should end
up in a -rc release.  Mine isn't -rc material.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] ARM: Kconfig.debug: Update Samsung UART config options

[Russell King - ARM Linux]

On Wed, Jun 25, 2014 at 08:41:13PM +0900, Kukjin Kim wrote:
 Sachin Kamat wrote:
  
 + Russell
 
  In a multiplatform config, the low level debug option shows several
  UART port entries. Improve the user visible string so that it becomes
  clear to the user about Samsung UART ports.
  While at it also remove some lines from the help text that are no
  longer applicable across all Samsung platforms.
  
 Looks good to me and will apply into cleanup.

That's fine.  I don't have anything which clashes with this.  Thanks for
asking.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/6] Enable L2 cache support on Exynos4210/4x12 SoCs

[Russell King - ARM Linux]

On Wed, Jun 25, 2014 at 03:37:25PM +0200, Tomasz Figa wrote:
 This series intends to add support for L2 cache on Exynos4 SoCs on boards
 running under secure firmware, which requires certain initialization steps
 to be done with help of firmware, as selected registers are writable only
 from secure mode.

What I said in my message on June 12th applies to this series.  I'm
not having the virtual address exposed via the write_sec call.

Yes, you need to read other registers in order to use your secure
firmware implementation.  Let's fix that by providing a better write_sec
interface so you don't have to read back these registers, rather than
working around this short-coming.

That's exactly what I meant when I talked on June 12th about turning
cache-l2x0.c back into a pile of crap.  You're working around problems
rather than fixing the underlying issue, as seems to be standard
platform maintainer behaviour when things like core ARM code is
concerned.  This is why things devolve over time into piles of crap,
because platforms just hack around problems rather than fixing the
root cause of the problem.

So... I'm NAKing the entire series.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/6] Enable L2 cache support on Exynos4210/4x12 SoCs

[Russell King - ARM Linux]

On Wed, Jun 25, 2014 at 04:13:16PM +0200, Tomasz Figa wrote:
 On 25.06.2014 15:50, Russell King - ARM Linux wrote:
  On Wed, Jun 25, 2014 at 03:37:25PM +0200, Tomasz Figa wrote:
  This series intends to add support for L2 cache on Exynos4 SoCs on boards
  running under secure firmware, which requires certain initialization steps
  to be done with help of firmware, as selected registers are writable only
  from secure mode.
  
  What I said in my message on June 12th applies to this series.  I'm
  not having the virtual address exposed via the write_sec call.
  
  Yes, you need to read other registers in order to use your secure
  firmware implementation.  Let's fix that by providing a better write_sec
  interface so you don't have to read back these registers, rather than
  working around this short-coming.
 
 Do you have anything in particular in mind? I would be glad to implement
 it and send patches.

As I've already said, you are not the only ones who need fuller information
to make your secure monitor calls.  So, what that implies is that rather
than the interface being please write register X with value V, and then
having platforms work-around that by reading various registers, we need
a more flexible interface which passes the desired state.

  That's exactly what I meant when I talked on June 12th about turning
  cache-l2x0.c back into a pile of crap.  You're working around problems
  rather than fixing the underlying issue, as seems to be standard
  platform maintainer behaviour when things like core ARM code is
  concerned.  This is why things devolve over time into piles of crap,
  because platforms just hack around problems rather than fixing the
  root cause of the problem.
 
 I'm not sure what part of my patches exactly is turning cache-l2x0.c
 into a pile of crap. On the contrary, I believe that working around the
 firmware brokenness on platform level, while keeping the core code
 simple does the opposite.

What you're doing is making the future maintanence of cache-l2x0.c
harder by breaking the modularity of it.  By exposing the virtual
address of the registers, reading the registers and getting your
secure monitor to write them back, you're making assumptions about
the behaviours inside cache-l2x0.c - while this may seem to be a no-op
think about what happens a few years down the line when someone needs
to change how this stuff operates, and you've long since moved on to
new pastures and aren't around to answer questions on the exynos
implementation.

Compare that to modifying the implementation to give the secure
monitors what they want - the desired state of the secure registers
when enabling and resuming the L2 cache.  The API becomes much
clearer, and maintainable, because - from the core persective,
there isn't a load of platforms doing weird crap with an API which
doesn't really fit what they're trying to do.

So, if we accept your patches as is and let that approach continue,
then years down the line, cleaning up the crap that you've deposited
becomes much harder, and we end up either having to break Exynos
declaring it as a SoC we just don't give a damn about it, or keep the
old interface.  That is bad news which ever way you look at it.

Quite simply, if a job is worth doing, then it's worth doing well and
properly.

The reason we have l2c_write_sec() right now is that when I sorted out
the existing shitpile that platform maintainers had turned that code
into over the years, it was the interface which suited the current
implementations.  As the secure APIs are typically confidential, there
is no way to consider what other alternatives are out there, so this
is something that is going to have to remain flexible - in other words,
it needs to remain long term maintainable, so that it can change.  So,
working around it's short-comings in platform code is totally the wrong
thing to do.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] ARM: EXYNOS: Add support for firmware-assisted suspend/resume

[Russell King - ARM Linux]

On Wed, Jun 25, 2014 at 06:18:34PM +0200, Tomasz Figa wrote:
 +static int exynos_suspend(void)
 +{
 + /* Save Power control and Diagnostic registers */
 + asm (mrc p15, 0, %0, c15, c0, 0\n
 + mrc p15, 0, %1, c15, c0, 1\n
 + : =r (cp15_power), =r (cp15_diag) : : cc);
 +
 + writel(EXYNOS_SLEEP_MAGIC, sysram_ns_base_addr + EXYNOS_BOOT_FLAG);
 + writel(virt_to_phys(cpu_resume),
 + sysram_ns_base_addr + EXYNOS_BOOT_ADDR);
 +
 + return cpu_suspend(0, exynos_cpu_suspend);
 +}
 +
 +static int exynos_resume(void)
 +{
 + exynos_smc(SMC_CMD_C15RESUME, cp15_power, cp15_diag, 0);

I am told that these two registers are not expected to change value
once the MMU is on.  This presents something of a problem where the
secure monitor is involved, because what that means is that this
really needs to be done before we get to C code.

OMAP has similar issues where it needs to restore the L2 cache
setup via SMC calls before the MMU is enabled, and they deal with
this via some hand-crafted assembly code which runs prior to calling
into cpu_resume.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] arm: exynos: Modify pm code to check for cortex A9 rather than the SoC

[Russell King - ARM Linux]

On Mon, Jun 16, 2014 at 09:37:14AM +0530, Abhilash Kesavan wrote:
 Hi Kukjin,
 
 On Fri, May 23, 2014 at 8:31 AM, Abhilash Kesavan a.kesa...@samsung.com 
 wrote:
  Signed-off-by: Abhilash Kesavan a.kesa...@samsung.com
 
 Do you have any comments on this patch ?

I do.

  diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
  index d10c351..6dd4a11 100644
  --- a/arch/arm/mach-exynos/pm.c
  +++ b/arch/arm/mach-exynos/pm.c
  @@ -300,7 +300,7 @@ static int exynos_pm_suspend(void)
  tmp = (S5P_USE_STANDBY_WFI0 | S5P_USE_STANDBY_WFE0);
  __raw_writel(tmp, S5P_CENTRAL_SEQ_OPTION);
 
  -   if (!soc_is_exynos5250())
  +   if (read_cpuid_part_number() == ARM_CPU_PART_CORTEX_A9)
  exynos_cpu_save_register();
...
  @@ -334,7 +334,7 @@ static void exynos_pm_resume(void)
  if (exynos_pm_central_resume())
  goto early_wakeup;
 
  -   if (!soc_is_exynos5250())
  +   if (read_cpuid_part_number() == ARM_CPU_PART_CORTEX_A9)
  exynos_cpu_restore_register();

It is invalid to check just the part number.  The part number on its
own is meaningless without taking account of the implementor.  Both
the implementor and the part number should be checked at each of these
sites.

Another point: exynos have taken it upon themselves to add code which
saves various ARM core registers.  This is a bad idea, it brings us
back to the days where every platform did their own suspend implementations.

CPU level registers should be handled by CPU level code, not by platform
code.  Is there a reason why this can't be added to the Cortex-A9
support code in proc-v7.S ?

  @@ -353,7 +353,7 @@ static void exynos_pm_resume(void)
 
  s3c_pm_do_restore_core(exynos_core_save, 
  ARRAY_SIZE(exynos_core_save));
 
  -   if (!soc_is_exynos5250())
  +   if (read_cpuid_part_number() == ARM_CPU_PART_CORTEX_A9)
  scu_enable(S5P_VA_SCU);
 
   early_wakeup:
  @@ -440,15 +440,18 @@ static int exynos_cpu_pm_notifier(struct 
  notifier_block *self,
  case CPU_PM_ENTER:
  if (cpu == 0) {
  exynos_pm_central_suspend();
  -   exynos_cpu_save_register();
  +   if (read_cpuid_part_number() == 
  ARM_CPU_PART_CORTEX_A9)
  +   exynos_cpu_save_register();
  }
  break;
 
  case CPU_PM_EXIT:
  if (cpu == 0) {
  -   if (!soc_is_exynos5250())
  +   if (read_cpuid_part_number() ==
  +   ARM_CPU_PART_CORTEX_A9) {
  scu_enable(S5P_VA_SCU);
  -   exynos_cpu_restore_register();
  +   exynos_cpu_restore_register();
  +   }
  exynos_pm_central_resume();
  }
  break;

And presumably with the CPU level code dealing with those registers,
you don't need the calls to save and restore these registers in this
notifier.

Which, by the way, is probably illegal to run as it runs in a read-
lock code path and with the SCU disabled.  As you're calling
scu_enable() that means you're non-coherent with the other CPUs,
and therefore locks don't work.

I think this code is very broken and wrongly architected, and shows
that we're continuing to make the same mistakes that we made all
through the 2000s with platforms doing their own crap rather than
properly thinking about this stuff.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] arm: exynos: Modify pm code to check for cortex A9 rather than the SoC

[Russell King - ARM Linux]

On Tue, Jun 24, 2014 at 05:11:14PM +0100, Russell King - ARM Linux wrote:
 Another point: exynos have taken it upon themselves to add code which
 saves various ARM core registers.  This is a bad idea, it brings us
 back to the days where every platform did their own suspend implementations.
 
 CPU level registers should be handled by CPU level code, not by platform
 code.  Is there a reason why this can't be added to the Cortex-A9
 support code in proc-v7.S ?

BTW, Shawn Guo recently posted a patch to add the diagnostic register
save/restore to proc-v7.S.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] arm: exynos: Modify pm code to check for cortex A9 rather than the SoC

[Russell King - ARM Linux]

On Tue, Jun 24, 2014 at 06:20:56PM +0200, Tomasz Figa wrote:
 Hi Russell,
 
 On 24.06.2014 18:11, Russell King - ARM Linux wrote:
  On Mon, Jun 16, 2014 at 09:37:14AM +0530, Abhilash Kesavan wrote:
  Hi Kukjin,
 
  On Fri, May 23, 2014 at 8:31 AM, Abhilash Kesavan a.kesa...@samsung.com 
  wrote:
  Signed-off-by: Abhilash Kesavan a.kesa...@samsung.com
 
  Do you have any comments on this patch ?
  
  I do.
  
  diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
  index d10c351..6dd4a11 100644
  --- a/arch/arm/mach-exynos/pm.c
  +++ b/arch/arm/mach-exynos/pm.c
  @@ -300,7 +300,7 @@ static int exynos_pm_suspend(void)
  tmp = (S5P_USE_STANDBY_WFI0 | S5P_USE_STANDBY_WFE0);
  __raw_writel(tmp, S5P_CENTRAL_SEQ_OPTION);
 
  -   if (!soc_is_exynos5250())
  +   if (read_cpuid_part_number() == ARM_CPU_PART_CORTEX_A9)
  exynos_cpu_save_register();
  ...
  @@ -334,7 +334,7 @@ static void exynos_pm_resume(void)
  if (exynos_pm_central_resume())
  goto early_wakeup;
 
  -   if (!soc_is_exynos5250())
  +   if (read_cpuid_part_number() == ARM_CPU_PART_CORTEX_A9)
  exynos_cpu_restore_register();
  
  It is invalid to check just the part number.  The part number on its
  own is meaningless without taking account of the implementor.  Both
  the implementor and the part number should be checked at each of these
  sites.
 
 Just out of curiosity, are you aware of more than one implementor of
 Cortex A9 on Exynos SoCs that would differ in having the need for save
 and restore of those registers?

That doesn't stop stuff changing in the future.  We've been here before.
Let's do the job properly if we're going to do the job at all.

If people still whine about this, I will force a change to make it
harder to do the wrong thing - I will get rid of the _part_number
interface replacing it with one which always returns the implementor
as well as the part number so both have to be checked.

  Another point: exynos have taken it upon themselves to add code which
  saves various ARM core registers.  This is a bad idea, it brings us
  back to the days where every platform did their own suspend implementations.
  
  CPU level registers should be handled by CPU level code, not by platform
  code.  Is there a reason why this can't be added to the Cortex-A9
  support code in proc-v7.S ?
 
 I agree that there is nothing platform specific in saving and restoring
 those registers and that this should be probably handled by generic code.
 
 However, when running in non-secure world, the only way to restore this
 is to call a firmware operation, which is platform specific. Is there a
 way to do something like this from proc-v7.S?

We never call firmware operations from assembly code.  However, in exynos'
case, it's not running in non-secure mode because it's happily reading
and writing these registers with no issue.

Platforms running in non-secure mode already have to ensure that various
work-arounds are implemented in their firmware/boot loader, and this
really is no different (we wouldn't need this code in the kernel in the
first place if the firmware/boot loader would get its act together.)

  And presumably with the CPU level code dealing with those registers,
  you don't need the calls to save and restore these registers in this
  notifier.
  
  Which, by the way, is probably illegal to run as it runs in a read-
  lock code path and with the SCU disabled.  As you're calling
  scu_enable() that means you're non-coherent with the other CPUs,
  and therefore locks don't work.
 
 I don't see the read lock code path you mention. Could you elaborate on
 this? By the way, other CPUs are still offline at this point.

We get there from kernel/cpu_pm.c, when the notifier chain is called.
The notifier chain is called while taking a read lock on
cpu_pm_notifier_lock.

If this is about the last CPU going down, then why the notifier?  Why
not put the code in exynos_suspend_enter() ?  Why add this needless
complexity?

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] arm: exynos: Modify pm code to check for cortex A9 rather than the SoC

[Russell King - ARM Linux]

On Tue, Jun 24, 2014 at 07:16:47PM +0200, Tomasz Figa wrote:
 I tend to disagree. The chance of a new Cortex A9 based SoC with
 different implementor code showing up is so close to zero that I don't
 see increasing of code complexity by adding yet another check justified.

That's your opinion which I strongly disagree with.

  If people still whine about this, I will force a change to make it
  harder to do the wrong thing - I will get rid of the _part_number
  interface replacing it with one which always returns the implementor
  as well as the part number so both have to be checked.
 
 That might be actually a better option. Something like
 
   if (read_cpuid_impl_and_part() == ARM_CPU(impl, part))
 
 or even
 
   if (ARM_CPU_IS(impl, part))
 
 would keep the checks simple, while still checking both values.

Indeed, but... Cortex is an ARM Ltd trademark, so I really doubt that
we'll see a Cortex processor implemented by another party other than
ARM.  So, there's no need for ARM_CPU(impl, part).

  We never call firmware operations from assembly code.  However, in exynos'
  case, it's not running in non-secure mode because it's happily reading
  and writing these registers with no issue.
 
 Current version of code, yes. However it handles only Exynos-based
 boards running in secure mode. For those running in non-secure mode,
 mainline does not support sleep yet.
 
 I already have patches to change this, which I was planning to post
 after eliminating last issue. The change set includes making this
 save/restore being executed only in secure mode.

As Will has already pointed out, writing to the diagnostic register
becomes a no-op in non-secure mode - it doesn't fault.  So moving
the saving and restoring of this register into generic code, where
other platforms already require it, makes total sense.

Of course, when you have to deal with it in non-secure mode, that's
something that you have to deal with, but in secure mode, platform
code should not have to worry about this level of detail.

 In ideal world (which I would be glad to be living in)...
 
 We both know that we can't fully rely on firmware. Firmware bugs are
 common and in many cases we can't do anything about it, because it
 already comes with the device and in many cases it is undesirable to
 change it or it can't be done at all.

Yes, I'm aware of the crap situation there.  That doesn't stop us
talking about these aspects though and setting what we expect in the
future - and changing the code to a better structure.

  We get there from kernel/cpu_pm.c, when the notifier chain is called.
  The notifier chain is called while taking a read lock on
  cpu_pm_notifier_lock.
 
 Your point. Thanks for explaining this. However this will be still
 running with just one CPU online.
 
  
  If this is about the last CPU going down, then why the notifier?  Why
  not put the code in exynos_suspend_enter() ?  Why add this needless
  complexity?
  
 
 This code is used by both system-wide suspend/resume and cpuidle paths.
 Daniel has moved this code to CPU PM notifier as a part of his Exynos
 cpuidle consolidation series to avoid code duplication, as this is the
 common point of both paths.
 
 To clarify, on suspend/resume path, the notifier is being called from
 syscore_ops registered in kernel/cpu_pm.c, while on cpuidle path, this
 is invoked from exynos_enter_core0_aftr() in
 drivers/cpuidle/cpuidle-exynos.c, which calls cpu_pm_enter().

... which then goes on to call cpu_suspend().  So moving this stuff
into the CPU level makes total sense rather than having every platform
running in secure mode duplicating this functionality.  Thanks for
pointing that out and adding further justification to my assertion.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with 1 CPU

[Russell King - ARM Linux]

On Sun, Jun 08, 2014 at 01:45:30PM +0100, Lorenzo Pieralisi wrote:
 Olof, it is not puritanism, it is all about upstreaming code. If we
 keep accepting these hacks and we end up with mach code full of them
 we have a problem, do you agree ?

To see the kind of problem that accepting hacked up code can cause, you
only have to look at Olof's build logs to see the warnings from the L2x0
cache code, which I've been totally unable to complete the cleanup of
/because/ we've historically accepted hacks into it.

I think that the legacy code is just going to have to stay (and I don't
want the warnings papered over, because I /want/ that crap to stick out
like a sore thumb), until someone can get sufficient motivation to work
out how to finally unuse the old functions.

Had I been on top of the L2 cache code earlier, and prevented these hacks
from going in (insisting that it was done properly) we would not be in
this position today where no one seems to know to fix this stuff.

This is the whole point - and nicely illustrates how easy it is for code
to become unmaintainable by accepting hacks to it.  A bit of push-back at
code acceptance time helps to save us from these problems in the future.

So, what I would want to see is not only what Lorenzo is saying (the
disclaimer in the comments) but also a technical description it, so if
the code needs to be modified in the future, we don't end up in this
kind of situation wondering what the code is doing/why the code exists.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with 1 CPU

[Russell King - ARM Linux]

On Sun, Jun 08, 2014 at 02:26:43PM -0400, Nicolas Pitre wrote:
 On Sun, 8 Jun 2014, Russell King - ARM Linux wrote:
 
  On Sat, Jun 07, 2014 at 04:53:34PM -0700, Olof Johansson wrote:
   You do realize that you have absolutely zero leverage over us on this,
   right? Our product is already shipped with kernel code that fixes
   this.
  
  That is never a justification for forcing /any/ code into the kernel.
  
  We've been here before with the iPAQs, where there were all sorts of
  horrid hacks that were in the code for that device, and we said no to
  it, and we kept it out of the mainline kernel, and stopped those hacks
  polluting elsewhere (because people got to know on the whole that if
  they used those hacks, it would bar them from mainline participation.)
 
 That's different.  The iPaq had very little in terms of firmware, and 
 the one it had was field upgradable with almost no risk to brick it 
 (unless you wanted to hack the firmware code but that's another story).  
 The reason iPaq had never been well supported in mainline can be 
 attributed to laziness for not wanting to make the code into a shape 
 acceptable for mainline inclusion.  But nothing fundamentally prevented 
 that from happening if someone had wanted to do it.

No, I was specifically thinking about the various iPAQ specific things
like the additional platform specific ATAGs that they invented with
zero reference to mainline, and then expected them to be accepted as-is.

I gave them a very clear message over that (which was no way) and that
was essentially the last of their mainline submissions.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with 1 CPU

[Russell King - ARM Linux]

On Sun, Jun 08, 2014 at 02:55:03PM -0400, Nicolas Pitre wrote:
 On Sun, 8 Jun 2014, Russell King - ARM Linux wrote:
  No, I was specifically thinking about the various iPAQ specific things
  like the additional platform specific ATAGs that they invented with
  zero reference to mainline, and then expected them to be accepted as-is.
  
  I gave them a very clear message over that (which was no way) and that
  was essentially the last of their mainline submissions.
 
 That's basically what I'm saying.  They were too lazy to fix their code.  
 But nothing prevented that otherwise. They certainly couldn't use the 
 firmware is broken and too hard to update excuse.

No, they just continued to use those ATAGs that they invented.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with 1 CPU

[Russell King - ARM Linux]

On Sat, Jun 07, 2014 at 04:53:34PM -0700, Olof Johansson wrote:
 You do realize that you have absolutely zero leverage over us on this,
 right? Our product is already shipped with kernel code that fixes
 this.

That is never a justification for forcing /any/ code into the kernel.

We've been here before with the iPAQs, where there were all sorts of
horrid hacks that were in the code for that device, and we said no to
it, and we kept it out of the mainline kernel, and stopped those hacks
polluting elsewhere (because people got to know on the whole that if
they used those hacks, it would bar them from mainline participation.)

There's many other instances.  Think about it - if we allowed this as
an acceptance criteria, then all that vendors have to do to get their
code into the kernel is change their development cycle: develop
product, ship product, force code into mainline as a done deal not
accepting any review comments back.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems booting exynos5420 with 1 CPU

[Russell King - ARM Linux]

On Fri, Jun 06, 2014 at 01:49:11PM -0700, Doug Anderson wrote:
 This works and IMHO is much cleaner because it totally removes the
 U-Boot dependency.  I'll cleanup to not be so insane and post:
 
 diff --git a/arch/arm/mach-exynos/mcpm-exynos.c
 b/arch/arm/mach-exynos/mcpm-exynos.c
 index 0498d0b..9c5df7b 100644
 --- a/arch/arm/mach-exynos/mcpm-exynos.c
 +++ b/arch/arm/mach-exynos/mcpm-exynos.c
 @@ -290,6 +290,14 @@ static void __naked
 exynos_pm_power_up_setup(unsigned int affinity_level)
 b  cci_enable_port_for_self);
  }
 
 +static void __naked exynos_mcpm_secondary_cpu_start(void)
 +{
 +   asm volatile (\n
 +   ldrr0, [pc, #0]\n
 +   bx r0\n
 +   .word  0 );
 +}
 +

So does it matter whether the above code gets assembled as thumb or
ARM?  How does your caller know which ISA mode to enter this fragment
in?

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Exynos induced build error with recent arm-soc

[Russell King - ARM Linux]

Spotted with a randconfig today:

arch/arm/mach-exynos/built-in.o: In function `exynos_set_cpu_boot_addr':
pm_domains.c:(.text+0x19c): undefined reference to `sysram_ns_base_addr'
arch/arm/mach-exynos/built-in.o:(.data+0x94): undefined reference to 
`exynos_enter_aftr'

This should be fixed before the merge window...

The failing .config is:

http://www.arm.linux.org.uk/developer/build/file.php?lid=8598

and the resulting build log with errors:

http://www.arm.linux.org.uk/developer/build/result.php?type=logcid=3001lid=8600format=build

Thanks.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 0/4] drivers/base: Generic framework for tracking internal interfaces

[Russell King - ARM Linux]

On Thu, May 01, 2014 at 09:04:19AM +0200, Andrzej Hajda wrote:
 2. You replace calls of component_add and component_del with calls
 to interface_tracker_ifup(dev, INTERFACE_TRACKER_TYPE_COMPONENT,  
 specific_component_ops),
 or interface_tracker_ifdown.
 Thats all for components.

How does the master get to decide which components are for it?  As
I see it, all masters see all components of a particular type.
What if you have a system with two masters each of which are bound
to a set of unique components but some of the components are of a
the same type?

How does the master know what type to use?

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html