date:20130808

Nishanth Menon  writes:

> On 08/08/2013 04:14 PM, Kevin Hilman wrote:
>> Dave Gerlach  writes:
>>
>>> On 08/08/2013 10:03 AM, Santosh Shilimkar wrote:
 $subject and patch don't match.

 On Thursday 08 August 2013 08:26 AM, Nishanth Menon wrote:
> On 08/08/2013 03:45 AM, Russ Dill wrote:
>> In reference to
>> the M3 handling it, the M3 wouldn't know which devices have a driver
>> bound and which don't.
> Does it need to? M3 firmware can pretty much define "I will force
> the device into low power state, and if the drivers dont handle
> things properly, fix the darned driver". M3 behavior should be
> considered as a "hardware" as far as Linux running on MPU is
> concerned, and firmware helps change the behavior by accounting for
> SoC quirks. *if* we have ability to handle this in the firmware,
> there is no need to carry this in Linux.
>
 I agree with Nishant. I don't like this patch and IIRC, I gave same
 comment in the last version. Linux need not know about all such firmware
 quirks. Also all these M3 specific stuff, should be done somewhere
 else. Probably having a small M3 driver won't be a bad idea.
>>>
>>> I am not opposed to doing it this way and letting the M3 firmware
>>> handle idling these modules, however the one concern raised in the
>>> last series is that an approach that does not acknowledge drivers will
>>> hide driver PM bugs. I suppose as long as I make sure to document that
>>> the devices are being idled by the M3 firmware this may not be an
>>> issue. I will look into implementing this.
>>
>> No, please don't start idling devices in firmware that are otherwise
>> managed by Linux.  Keep the firmware simple and dumb.  Linux is managing
>> these devices, it should manage their bugs too.
>
>>
>> This is not just about idling devices.  This is about handling broken IP
>> blocks whose power-on reset state does not allow the the powerdomain to
>> reach its target state.  That's just bad hardware design.
>
> Right, this is where M3 can help -> provide a consistent state for
> linux kernel to work with. by the fact that we want to keep majority
> of the power code inside master CPU, we are just letting M3 help us
> with nothing major at all.. 

heh, I would say HW design bugs like this are more than "nothing major
at all." :)  

> tiny stuff like these can help "fix" the hardware design quirks by
> hiding it behind the firmware and modifying the hardware behavior.

I disagree here.  I'm a firmware minimalist, and hiding bugs like this
in the firmware is wrong when Linux is otherwise managing these devices.
It also imposes criteria on the firmware of future SoCs that doesn't
belong there either.  IMO, the only stuff the firmware should do is what
Linux *cannot* do.

Remember, this only needs to happen when there isn't a driver for these
devices.  Should we communicate to the firmware that the OS has no
driver, so please enable the hack?  I think not.

> I know it breaks the purity of role, but as the
> next evolution, we might want to consider M3 something like an
> "accelerator" for power management activity.. (not saying it is that
> fast.. but conceptually).

Yes, it breaks the purity of role, and makes it hard to maintain and
extend to future SoCs.  As a maintainer, that's a red flag.  IMO, the
roles need to be kept clear.  The M3 manages some devices and the
interconnect that MPU/Linux cannot, the rest are managed by Linux.

>> That being said, IMO, the kernel (specifically omap_device) should
>> handle this, and it should be rather easy to do in the omap_device layer
>> and keep the SoC suspend/resume core code simple and ignorant of these
>> "quirks."
>>
>> AFAICT, there's no reason these quirks need to be dealt with immediatly
>> on suspend.  A slight delay should be fine, as long as it's before the
>> next suspend/idle attempt, right?
>>
>> Given that, what we need to do (and by we, I mean you) is to flag all
>> broken IP blocks, and let omap_device handle them in a suspend/resume
>> notifier (c.f. register_pm_notifier() and PM_POST_SUSPEND.)
>
> yes - that is the alternate that comes to mind.

In the earlier reviews of this series (many months ago now), I
complained about the presence of this device specific handling in the
core MPU PM code.  I'm somewhat troubled by the fact that nobody explored
alternatives that so easily come to mind.

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: v3.11-rc4: OMAP1/Amstrad Delta (E3) crash

2013-08-08 Thread Aaro Koskinen

Hi,

On Fri, Aug 09, 2013 at 12:11:15AM +0300, Aaro Koskinen wrote:
> On Thu, Aug 08, 2013 at 12:01:08PM +0100, Russell King - ARM Linux wrote:
> > On Wed, Aug 07, 2013 at 02:26:09AM +0300, Aaro Koskinen wrote:
> > > [0.258589] [1224] *pgd=, *pte=11fff0cb01f1, 
> > > *ppte=11fff00a
> > 
> > BTW, your oops dump is interesting for another reason - the above.
> > You seem to have 64-bit page table entries above.
> 
> Yes, this is worrying...
> 
> > Now, this is totally legal C:
> > 
> > unsigned int val = 0x12345678;
> > 
> > printk("%08llx\n", (long long)val);
> > 
> > and it should produce "12345678" but I'm willing to bet with your compiler
> > it produces "12345678" where  is just what happens to be
> > sitting in some register.  IOW, I think you have a compiler bug here.  Can
> > you investigate what's going on with this yourself please?
> 
> I made a quick check and added that to the fault handler, and yes,
> the result is crap:
> 
> +unsigned int test_bar = 0x12345678;
>  void show_pte(struct mm_struct *mm, unsigned long addr)
>  {
> pgd_t *pgd;
>  
> +   printk(KERN_INFO "%d\n", 0);
> +   printk(KERN_INFO "%llx\n", (long long)test_bar);
> +   printk(KERN_INFO "%d %llx\n", 1, (long long)test_bar);
> [...]
> [0.261788] 0
> [0.263849] 12345678
> [0.267201] 1 0

Still one more thing. Once I apply the FIQ patch and initiate a crash
from userspace, results are as expected:

# echo c > /proc/sysrq-trigger
[   75.547504] SysRq : Trigger a crash
[   75.562891] Unable to handle kernel NULL pointer dereference at virtual 
address 
[   75.594259] 0
[   75.607326] 12345678
[   75.620607] 1 12345678
[   75.633818] pgd = c10f
[   75.647114] [] *pgd=1195d831, *pte=, *ppte=

A.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] experimental: arm: dts: dra7xx: Add a DT node for VPE

2013-08-08 Thread Laurent Pinchart

Hi Archit,

Thank you for the patch.

On Friday 02 August 2013 19:33:43 Archit Taneja wrote:
> Add a DT node for VPE in dra7.dtsi. This is experimental because we might
> need to split the VPE address space a bit more, and also because the IRQ
> line described is accessible the IRQ crossbar driver is added for DRA7XX.
> 
> Cc: Rajendra Nayak 
> Cc: Sricharan R 
> Signed-off-by: Archit Taneja 
> ---
>  arch/arm/boot/dts/dra7.dtsi | 11 +++

Documentation is missing :-) As this is an experimental patch you can probably 
document the bindings later.

>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
> index ce9a0f0..3237972 100644
> --- a/arch/arm/boot/dts/dra7.dtsi
> +++ b/arch/arm/boot/dts/dra7.dtsi
> @@ -484,6 +484,17 @@
>   dmas = <&sdma 70>, <&sdma 71>;
>   dma-names = "tx0", "rx0";
>   };
> +
> + vpe {
> + compatible = "ti,vpe";
> + ti,hwmods = "vpe";
> + reg = <0x489d 0xd000>, <0x489dd000 0x400>;
> + reg-names = "vpe", "vpdma";
> + interrupts = <0 159 0x4>;
> + #address-cells = <1>;
> + #size-cells = <0>;

Are #address-cells and #size-cells really needed ?

> + };
> +
>   };
> 
>   clocks {
-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" 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/6] v4l: ti-vpe: Create a vpdma helper library

2013-08-08 Thread Laurent Pinchart

Hi Archit,

Thank you for the patch.

On Friday 02 August 2013 19:33:38 Archit Taneja wrote:
> The primary function of VPDMA is to move data between external memory and
> internal processing modules(in our case, VPE) that source or sink data.
> VPDMA is capable of buffering this data and then delivering the data as
> demanded to the modules as programmed. The modules that source or sink data
> are referred to as clients or ports. A channel is setup inside the VPDMA to
> connect a specific memory buffer to a specific client. The VPDMA
> centralizes the DMA control functions and buffering required to allow all
> the clients to minimize the effect of long latency times.
> 
> Add the following to the VPDMA helper:
> 
> - A data struct which describe VPDMA channels. For now, these channels are
> the ones used only by VPE, the list of channels will increase when
> VIP(Video Input Port) also uses the VPDMA library. This channel information
> will be used to populate fields required by data descriptors.
> 
> - Data structs which describe the different data types supported by VPDMA.
> This data type information will be used to populate fields required by data
> descriptors and used by the VPE driver to map a V4L2 format to the
> corresponding VPDMA data type.
> 
> - Provide VPDMA register offset definitions, functions to read, write and
> modify VPDMA registers.
> 
> - Functions to create and submit a VPDMA list. A list is a group of
> descriptors that makes up a set of DMA transfers that need to be completed.
> Each descriptor will either perform a DMA transaction to fetch input
> buffers and write to output buffers(data descriptors), or configure the
> MMRs of sub blocks of VPE(configuration descriptors), or provide control
> information to VPDMA (control descriptors).
> 
> - Functions to allocate, map and unmap buffers needed for the descriptor
> list, payloads containing MMR values and motion vector buffers. These use
> the DMA mapping APIs to ensure exclusive access to VPDMA.
> 
> - Functions to enable VPDMA interrupts. VPDMA can trigger an interrupt on
> the VPE interrupt line when a descriptor list is parsed completely and the
> DMA transactions are completed. This requires masking the events in VPDMA
> registers and configuring some top level VPE interrupt registers.
> 
> - Enable some VPDMA specific parameters: frame start event(when to start DMA
> for a client) and line mode(whether each line fetched should be mirrored or
> not).
> 
> - Function to load firmware required by VPDMA. VPDMA requires a firmware for
> it's internal list manager. We add the required request_firmware apis to
> fetch this firmware from user space.
> 
> - Function to dump VPDMA registers.
> 
> - A function to initialize VPDMA, this will be called by the VPE driver with
> it's platform device pointer, this function will take care of loading VPDMA
> firmware and returning a handle back to the VPE driver. The VIP driver will
> also call the same init function to initialize it's own VPDMA instance.
> 
> Signed-off-by: Archit Taneja 
> ---
>  drivers/media/platform/ti-vpe/vpdma.c  | 589 ++
>  drivers/media/platform/ti-vpe/vpdma.h  | 154 
>  drivers/media/platform/ti-vpe/vpdma_priv.h | 119 ++
>  3 files changed, 862 insertions(+)
>  create mode 100644 drivers/media/platform/ti-vpe/vpdma.c
>  create mode 100644 drivers/media/platform/ti-vpe/vpdma.h
>  create mode 100644 drivers/media/platform/ti-vpe/vpdma_priv.h
> 
> diff --git a/drivers/media/platform/ti-vpe/vpdma.c
> b/drivers/media/platform/ti-vpe/vpdma.c new file mode 100644
> index 000..b15b3dd
> --- /dev/null
> +++ b/drivers/media/platform/ti-vpe/vpdma.c
> @@ -0,0 +1,589 @@

[snip]

> +static int get_field(u32 value, u32 mask, int shift)
> +{
> + return (value & (mask << shift)) >> shift;
> +}
> +
> +static int get_field_reg(struct vpdma_data *vpdma, int offset,
> + u32 mask, int shift)

I would call this read_field_reg().

> +{
> + return get_field(read_reg(vpdma, offset), mask, shift);
> +}
> +
> +static void insert_field(u32 *valp, u32 field, u32 mask, int shift)
> +{
> + u32 val = *valp;
> +
> + val &= ~(mask << shift);
> + val |= (field & mask) << shift;
> + *valp = val;
> +}

get_field() and insert_field() are used in a single location, you can manually 
inline them.

> +static void insert_field_reg(struct vpdma_data *vpdma, int offset, u32
> field,
> + u32 mask, int shift)
> +{
> + u32 val = read_reg(vpdma, offset);
> +
> + insert_field(&val, field, mask, shift);
> +
> + write_reg(vpdma, offset, val);
> +}

[snip]

> +/*
> + * Allocate a DMA buffer
> + */
> +int vpdma_buf_alloc(struct vpdma_buf *buf, size_t size)
> +{
> + buf->size = size;
> + buf->mapped = 0;
> + buf->addr = kzalloc(size, GFP_KERNEL);

You should use the dma allocation API (depending on your needs, 
dma_alloc_coherent for instance) to allocate DMA-able buffers.

> + if (!buf->addr)
> +

Re: [PATCH 1/6] v4l: ti-vpe: Create a vpdma helper library

2013-08-08 Thread Laurent Pinchart

Hi Archit,

On Monday 05 August 2013 16:56:46 Archit Taneja wrote:
> On Monday 05 August 2013 01:43 PM, Tomi Valkeinen wrote:
> > On 02/08/13 17:03, Archit Taneja wrote:
> >> +struct vpdma_data_format vpdma_yuv_fmts[] = {
> >> +  [VPDMA_DATA_FMT_Y444] = {
> >> +  .data_type  = DATA_TYPE_Y444,
> >> +  .depth  = 8,
> >> +  },
> > 
> > This, and all the other tables, should probably be consts?
> 
> That's true, I'll fix those.
> 
> >> +static void insert_field(u32 *valp, u32 field, u32 mask, int shift)
> >> +{
> >> +  u32 val = *valp;
> >> +
> >> +  val &= ~(mask << shift);
> >> +  val |= (field & mask) << shift;
> >> +  *valp = val;
> >> +}
> > 
> > I think "insert" normally means, well, inserting a thing in between
> > something. What you do here is overwriting.
> > 
> > Why not just call it "write_field"?
> 
> sure, will change it.
> 
> >> + * Allocate a DMA buffer
> >> + */
> >> +int vpdma_buf_alloc(struct vpdma_buf *buf, size_t size)
> >> +{
> >> +  buf->size = size;
> >> +  buf->mapped = 0;
> > 
> > Maybe true/false is clearer here that 0/1.
> 
> okay.
> 
> >> +/*
> >> + * submit a list of DMA descriptors to the VPE VPDMA, do not wait for
> >> completion + */
> >> +int vpdma_submit_descs(struct vpdma_data *vpdma, struct vpdma_desc_list
> >> *list) +{
> >> +  /* we always use the first list */
> >> +  int list_num = 0;
> >> +  int list_size;
> >> +
> >> +  if (vpdma_list_busy(vpdma, list_num))
> >> +  return -EBUSY;
> >> +
> >> +  /* 16-byte granularity */
> >> +  list_size = (list->next - list->buf.addr) >> 4;
> >> +
> >> +  write_reg(vpdma, VPDMA_LIST_ADDR, (u32) list->buf.dma_addr);
> >> +  wmb();
> > 
> > What is the wmb() for?
> 
> VPDMA_LIST_ADDR needs to be written before VPDMA_LIST_ATTR, otherwise
> VPDMA doesn't work. wmb() ensures the ordering.

write_reg() calls iowrite32(), which already includes an __iowmb().

> >> +  write_reg(vpdma, VPDMA_LIST_ATTR,
> >> +  (list_num << VPDMA_LIST_NUM_SHFT) |
> >> +  (list->type << VPDMA_LIST_TYPE_SHFT) |
> >> +  list_size);
> >> +
> >> +  return 0;
> >> +}
> >> 
> >> +static void vpdma_firmware_cb(const struct firmware *f, void *context)
> >> +{
> >> +  struct vpdma_data *vpdma = context;
> >> +  struct vpdma_buf fw_dma_buf;
> >> +  int i, r;
> >> +
> >> +  dev_dbg(&vpdma->pdev->dev, "firmware callback\n");
> >> +
> >> +  if (!f || !f->data) {
> >> +  dev_err(&vpdma->pdev->dev, "couldn't get firmware\n");
> >> +  return;
> >> +  }
> >> +
> >> +  /* already initialized */
> >> +  if (get_field_reg(vpdma, VPDMA_LIST_ATTR, VPDMA_LIST_RDY_MASK,
> >> +  VPDMA_LIST_RDY_SHFT)) {
> >> +  vpdma->ready = true;
> >> +  return;
> >> +  }
> >> +
> >> +  r = vpdma_buf_alloc(&fw_dma_buf, f->size);
> >> +  if (r) {
> >> +  dev_err(&vpdma->pdev->dev,
> >> +  "failed to allocate dma buffer for firmware\n");
> >> +  goto rel_fw;
> >> +  }
> >> +
> >> +  memcpy(fw_dma_buf.addr, f->data, f->size);
> >> +
> >> +  vpdma_buf_map(vpdma, &fw_dma_buf);
> >> +
> >> +  write_reg(vpdma, VPDMA_LIST_ADDR, (u32) fw_dma_buf.dma_addr);
> >> +
> >> +  for (i = 0; i < 100; i++) { /* max 1 second */
> >> +  msleep_interruptible(10);
> > 
> > You call interruptible version here, but you don't handle the
> > interrupted case. I believe the loop will just continue looping, even if
> > the user interrupted.
> 
> Okay. I think I don't understand the interruptible version correctly. We
> don't need to msleep_interruptible here, we aren't waiting on any wake
> up event, we just want to wait till a bit gets set.
> 
> I am thinking of implementing something similar to wait_for_bit_change()
> in 'drivers/video/omap2/dss/dsi.c'

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 8/9] ARM: OMAP2+: AM33XX: Basic suspend resume support

2013-08-08 Thread Nishanth Menon


On 08/08/2013 04:14 PM, Kevin Hilman wrote:

Dave Gerlach  writes:


On 08/08/2013 10:03 AM, Santosh Shilimkar wrote:

$subject and patch don't match.

On Thursday 08 August 2013 08:26 AM, Nishanth Menon wrote:

On 08/08/2013 03:45 AM, Russ Dill wrote:

In reference to
the M3 handling it, the M3 wouldn't know which devices have a driver
bound and which don't.

Does it need to? M3 firmware can pretty much define "I will force
the device into low power state, and if the drivers dont handle
things properly, fix the darned driver". M3 behavior should be
considered as a "hardware" as far as Linux running on MPU is
concerned, and firmware helps change the behavior by accounting for
SoC quirks. *if* we have ability to handle this in the firmware,
there is no need to carry this in Linux.


I agree with Nishant. I don't like this patch and IIRC, I gave same
comment in the last version. Linux need not know about all such firmware
quirks. Also all these M3 specific stuff, should be done somewhere
else. Probably having a small M3 driver won't be a bad idea.

>>

I am not opposed to doing it this way and letting the M3 firmware
handle idling these modules, however the one concern raised in the
last series is that an approach that does not acknowledge drivers will
hide driver PM bugs. I suppose as long as I make sure to document that
the devices are being idled by the M3 firmware this may not be an
issue. I will look into implementing this.


No, please don't start idling devices in firmware that are otherwise
managed by Linux.  Keep the firmware simple and dumb.  Linux is managing
these devices, it should manage their bugs too.




This is not just about idling devices.  This is about handling broken IP
blocks whose power-on reset state does not allow the the powerdomain to
reach its target state.  That's just bad hardware design.


Right, this is where M3 can help -> provide a consistent state for linux 
kernel to work with. by the fact that we want to keep majority of the 
power code inside master CPU, we are just letting M3 help us with 
nothing major at all.. tiny stuff like these can help "fix" the hardware 
design quirks by hiding it behind the firmware and modifying the 
hardware behavior. I know it breaks the purity of role, but as the next 
evolution, we might want to consider M3 something like an "accelerator" 
for power management activity.. (not saying it is that fast.. but 
conceptually).




That being said, IMO, the kernel (specifically omap_device) should
handle this, and it should be rather easy to do in the omap_device layer
and keep the SoC suspend/resume core code simple and ignorant of these
"quirks."

AFAICT, there's no reason these quirks need to be dealt with immediatly
on suspend.  A slight delay should be fine, as long as it's before the
next suspend/idle attempt, right?

Given that, what we need to do (and by we, I mean you) is to flag all
broken IP blocks, and let omap_device handle them in a suspend/resume
notifier (c.f. register_pm_notifier() and PM_POST_SUSPEND.)


yes - that is the alternate that comes to mind.

--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ARM: omap2: Use a consistent AM33XX SoC option description

2013-08-08 Thread Ezequiel Garcia

Fix the option description to match the other TI SoCs.
This is just a cosmetic change.

Signed-off-by: Ezequiel Garcia 
---
 arch/arm/mach-omap2/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
index 76170dd..56021c6 100644
--- a/arch/arm/mach-omap2/Kconfig
+++ b/arch/arm/mach-omap2/Kconfig
@@ -64,7 +64,7 @@ config SOC_OMAP5
select ARM_ERRATA_798181 if SMP
 
 config SOC_AM33XX
-   bool "AM33XX support"
+   bool "TI AM33XX"
depends on ARCH_MULTI_V7
select ARCH_OMAP2PLUS
select ARM_CPU_SUSPEND if PM
-- 
1.8.1.5

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 8/9] ARM: OMAP2+: AM33XX: Basic suspend resume support

Dave Gerlach  writes:

> On 08/08/2013 10:03 AM, Santosh Shilimkar wrote:
>> $subject and patch don't match.
>>
>> On Thursday 08 August 2013 08:26 AM, Nishanth Menon wrote:
>>> On 08/08/2013 03:45 AM, Russ Dill wrote:
In reference to
 the M3 handling it, the M3 wouldn't know which devices have a driver
 bound and which don't.
>>> Does it need to? M3 firmware can pretty much define "I will force
>>> the device into low power state, and if the drivers dont handle
>>> things properly, fix the darned driver". M3 behavior should be
>>> considered as a "hardware" as far as Linux running on MPU is
>>> concerned, and firmware helps change the behavior by accounting for
>>> SoC quirks. *if* we have ability to handle this in the firmware,
>>> there is no need to carry this in Linux.
>>>
>> I agree with Nishant. I don't like this patch and IIRC, I gave same
>> comment in the last version. Linux need not know about all such firmware
>> quirks. Also all these M3 specific stuff, should be done somewhere
>> else. Probably having a small M3 driver won't be a bad idea.
>>
>> Regards,
>> Santosh
>>
>
> I am not opposed to doing it this way and letting the M3 firmware
> handle idling these modules, however the one concern raised in the
> last series is that an approach that does not acknowledge drivers will
> hide driver PM bugs. I suppose as long as I make sure to document that
> the devices are being idled by the M3 firmware this may not be an
> issue. I will look into implementing this.

No, please don't start idling devices in firmware that are otherwise
managed by Linux.  Keep the firmware simple and dumb.  Linux is managing
these devices, it should manage their bugs too.

This is not just about idling devices.  This is about handling broken IP
blocks whose power-on reset state does not allow the the powerdomain to
reach its target state.  That's just bad hardware design.  

That being said, IMO, the kernel (specifically omap_device) should
handle this, and it should be rather easy to do in the omap_device layer
and keep the SoC suspend/resume core code simple and ignorant of these
"quirks."

AFAICT, there's no reason these quirks need to be dealt with immediatly
on suspend.  A slight delay should be fine, as long as it's before the
next suspend/idle attempt, right?  

Given that, what we need to do (and by we, I mean you) is to flag all
broken IP blocks, and let omap_device handle them in a suspend/resume
notifier (c.f. register_pm_notifier() and PM_POST_SUSPEND.)

That will keep things contained to the omap_device/hwmod level and allow
flexiblity for future broken SoCs where the list of broken IP blocks is
different.  Though surely this broken hardware doesn't exist in AM4xxx
because someone noticed this early on and pointed out that it should be
fixed in hardware, right?  ;)

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: v3.11-rc4: OMAP1/Amstrad Delta (E3) crash

2013-08-08 Thread Aaro Koskinen

Hi,

On Thu, Aug 08, 2013 at 12:01:08PM +0100, Russell King - ARM Linux wrote:
> On Wed, Aug 07, 2013 at 02:26:09AM +0300, Aaro Koskinen wrote:
> > [0.258589] [1224] *pgd=, *pte=11fff0cb01f1, 
> > *ppte=11fff00a
> 
> BTW, your oops dump is interesting for another reason - the above.
> You seem to have 64-bit page table entries above.

Yes, this is worrying...

> Now, this is totally legal C:
> 
>   unsigned int val = 0x12345678;
> 
>   printk("%08llx\n", (long long)val);
> 
> and it should produce "12345678" but I'm willing to bet with your compiler
> it produces "12345678" where  is just what happens to be
> sitting in some register.  IOW, I think you have a compiler bug here.  Can
> you investigate what's going on with this yourself please?

I made a quick check and added that to the fault handler, and yes,
the result is crap:

+unsigned int test_bar = 0x12345678;
 void show_pte(struct mm_struct *mm, unsigned long addr)
 {
pgd_t *pgd;
 
+   printk(KERN_INFO "%d\n", 0);
+   printk(KERN_INFO "%llx\n", (long long)test_bar);
+   printk(KERN_INFO "%d %llx\n", 1, (long long)test_bar);
[...]
[0.261788] 0
[0.263849] 12345678
[0.267201] 1 0

However, addings similar prints to early boot produces correct results:

+unsigned int test_foo = 0x12345678;
 void __init smp_setup_processor_id(void)
[...]
printk(KERN_INFO "Booting Linux on physical CPU 0x%x\n", mpidr);
+   printk(KERN_INFO "%llx\n", (long long)test_foo);
+   printk(KERN_INFO "%d %llx\n", 1, (long long)test_foo);
[...]
[0.00] Booting Linux on physical CPU 0x0
[0.00] 12345678
[0.00] 1 12345678

So I'm pretty confused. Anyway, I'll continue investigating this next week...

A.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 4/9] ARM: OMAP2+: AM33XX: Reserve memory to comply with EMIF spec

On Thursday 08 August 2013 04:05 PM, Kevin Hilman wrote:
> Santosh Shilimkar  writes:
> 
>> On Thursday 08 August 2013 02:16 PM, Kevin Hilman wrote:
>>> Dave Gerlach  writes:
>>>
 From: Vaibhav Bedia 

 SDRAM controller on AM33XX requires that a modification of certain
 bit-fields in PWR_MGMT_CTRL register (ref. section 7.3.5.13 in
 AM335x-Rev H) is followed by a dummy read access to SDRAM. This
 scenario arises when entering a low power state like DeepSleep.
 To ensure that the read is not from a cached region we reserve
 some memory during bootup using the arm_memblock_steal() API.
>>>
>>> Hmm, sounds to me an awful lot like the existing omap_bus_sync() ?
>>>
>> All the credit of that awful omap_bus_sync() goes to me since 
>> I introduced it. And I keep beating the hardware guys
>> who have not left a choice but to introduce the ugly work
>> around in software. ;-)
> 
> Agreed, but what's even more awful than the current version is
> duplicating it in a slightly different way using yet another whole page
> mapping for a single read/write location.
> 
The real issue is limitation of the kernel memory steal(memblock) API which
won't let you still less than 1 MB. It would have been ok for page allocation
because that is any way what you will get minimum on standard non-cached
allocations.

Regards,
Santosh

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 4/9] ARM: OMAP2+: AM33XX: Reserve memory to comply with EMIF spec

Santosh Shilimkar  writes:

> On Thursday 08 August 2013 02:16 PM, Kevin Hilman wrote:
>> Dave Gerlach  writes:
>> 
>>> From: Vaibhav Bedia 
>>>
>>> SDRAM controller on AM33XX requires that a modification of certain
>>> bit-fields in PWR_MGMT_CTRL register (ref. section 7.3.5.13 in
>>> AM335x-Rev H) is followed by a dummy read access to SDRAM. This
>>> scenario arises when entering a low power state like DeepSleep.
>>> To ensure that the read is not from a cached region we reserve
>>> some memory during bootup using the arm_memblock_steal() API.
>> 
>> Hmm, sounds to me an awful lot like the existing omap_bus_sync() ?
>>
> All the credit of that awful omap_bus_sync() goes to me since 
> I introduced it. And I keep beating the hardware guys
> who have not left a choice but to introduce the ugly work
> around in software. ;-)

Agreed, but what's even more awful than the current version is
duplicating it in a slightly different way using yet another whole page
mapping for a single read/write location.

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 6/9] ARM: OMAP2+: timer: Add suspend-resume callbacks for clkevent device


On 08/08/2013 01:25 PM, Kevin Hilman wrote:

Dave Gerlach  writes:


From: Vaibhav Bedia 

OMAP timer code registers two timers - one as clocksource
and one as clockevent. Since AM33XX has only one usable timer
in the WKUP domain one of the timers needs suspend-resume
support to restore the configuration to pre-suspend state.

commit adc78e6 (timekeeping: Add suspend and resume
of clock event devices) introduced .suspend and .resume
callbacks for clock event devices. Leverages these
callbacks to have AM33XX clockevent timer which is
in not in WKUP domain to behave properly across system
suspend.

Signed-off-by: Vaibhav Bedia 
Signed-off-by: Dave Gerlach 
---
  arch/arm/mach-omap2/timer.c |   32 
  1 file changed, 32 insertions(+)

diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
index b37e1fc..cce5d39 100644
--- a/arch/arm/mach-omap2/timer.c
+++ b/arch/arm/mach-omap2/timer.c
@@ -118,11 +118,43 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode 
mode,
}
  }

+static void omap_clkevt_suspend(struct clock_event_device *unused)
+{
+   char name[10];
+   struct omap_hwmod *oh;
+
+   sprintf(name, "timer%d", clkev.id);
+   oh = omap_hwmod_lookup(name);


/me stops reviewing here.  This should be a one-time thing.

Seeing things *again* in patches that I've already reviewed (multiple
times) is very frustrating.  It also increases the likelihood of future
patches to be "filtered."  (in this case, you get a pass since you seem
to have inherited Vaibhav's code, but please take care to address all
reviewer comments, or at least explain why you didn'.)

Kevin



Sorry for missing this. Seems I missed this patch completely when taking 
over. I'll make sure it's addressed in the next version.


Regards,
Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 4/9] ARM: OMAP2+: AM33XX: Reserve memory to comply with EMIF spec

On Thursday 08 August 2013 02:16 PM, Kevin Hilman wrote:
> Dave Gerlach  writes:
> 
>> From: Vaibhav Bedia 
>>
>> SDRAM controller on AM33XX requires that a modification of certain
>> bit-fields in PWR_MGMT_CTRL register (ref. section 7.3.5.13 in
>> AM335x-Rev H) is followed by a dummy read access to SDRAM. This
>> scenario arises when entering a low power state like DeepSleep.
>> To ensure that the read is not from a cached region we reserve
>> some memory during bootup using the arm_memblock_steal() API.
> 
> Hmm, sounds to me an awful lot like the existing omap_bus_sync() ?
>
All the credit of that awful omap_bus_sync() goes to me since 
I introduced it. And I keep beating the hardware guys
who have not left a choice but to introduce the ugly work
around in software. ;-)

Regards,
Santosh
 

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 6/9] ARM: OMAP2+: timer: Add suspend-resume callbacks for clkevent device

Dave Gerlach  writes:

> From: Vaibhav Bedia 
>
> OMAP timer code registers two timers - one as clocksource
> and one as clockevent. Since AM33XX has only one usable timer
> in the WKUP domain one of the timers needs suspend-resume
> support to restore the configuration to pre-suspend state.
>
> commit adc78e6 (timekeeping: Add suspend and resume
> of clock event devices) introduced .suspend and .resume
> callbacks for clock event devices. Leverages these
> callbacks to have AM33XX clockevent timer which is
> in not in WKUP domain to behave properly across system
> suspend.
>
> Signed-off-by: Vaibhav Bedia 
> Signed-off-by: Dave Gerlach 
> ---
>  arch/arm/mach-omap2/timer.c |   32 
>  1 file changed, 32 insertions(+)
>
> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> index b37e1fc..cce5d39 100644
> --- a/arch/arm/mach-omap2/timer.c
> +++ b/arch/arm/mach-omap2/timer.c
> @@ -118,11 +118,43 @@ static void omap2_gp_timer_set_mode(enum 
> clock_event_mode mode,
>   }
>  }
>  
> +static void omap_clkevt_suspend(struct clock_event_device *unused)
> +{
> + char name[10];
> + struct omap_hwmod *oh;
> +
> + sprintf(name, "timer%d", clkev.id);
> + oh = omap_hwmod_lookup(name);

/me stops reviewing here.  This should be a one-time thing.

Seeing things *again* in patches that I've already reviewed (multiple
times) is very frustrating.  It also increases the likelihood of future
patches to be "filtered."  (in this case, you get a pass since you seem
to have inherited Vaibhav's code, but please take care to address all
reviewer comments, or at least explain why you didn'.)

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 4/9] ARM: OMAP2+: AM33XX: Reserve memory to comply with EMIF spec

Dave Gerlach  writes:

> From: Vaibhav Bedia 
>
> SDRAM controller on AM33XX requires that a modification of certain
> bit-fields in PWR_MGMT_CTRL register (ref. section 7.3.5.13 in
> AM335x-Rev H) is followed by a dummy read access to SDRAM. This
> scenario arises when entering a low power state like DeepSleep.
> To ensure that the read is not from a cached region we reserve
> some memory during bootup using the arm_memblock_steal() API.

Hmm, sounds to me an awful lot like the existing omap_bus_sync() ?

However, you remove that feature when you remove omap_reserve() here
becasue omap_reserve() ss not called from your new reserve function.
Are you *really* sure you don't need the functions called there?  This
is a pretty big change that's not mentioned anywhere in the changelog.

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 8/9] ARM: OMAP2+: AM33XX: Basic suspend resume support

2013-08-08 Thread Nishanth Menon


On 08/08/2013 11:06 AM, Dave Gerlach wrote:

On 08/08/2013 10:03 AM, Santosh Shilimkar wrote:

$subject and patch don't match.

On Thursday 08 August 2013 08:26 AM, Nishanth Menon wrote:

On 08/08/2013 03:45 AM, Russ Dill wrote:

   In reference to
the M3 handling it, the M3 wouldn't know which devices have a driver
bound and which don't.

Does it need to? M3 firmware can pretty much define "I will force the
device into low power state, and if the drivers dont handle things
properly, fix the darned driver". M3 behavior should be considered as
a "hardware" as far as Linux running on MPU is concerned, and
firmware helps change the behavior by accounting for SoC quirks. *if*
we have ability to handle this in the firmware, there is no need to
carry this in Linux.


I agree with Nishant. I don't like this patch and IIRC, I gave same
comment in the last version. Linux need not know about all such firmware
quirks. Also all these M3 specific stuff, should be done somewhere
else. Probably having a small M3 driver won't be a bad idea.

Regards,
Santosh



I am not opposed to doing it this way and letting the M3 firmware handle
idling these modules, however the one concern raised in the last series
is that an approach that does not acknowledge drivers will hide driver
PM bugs. I suppose as long as I make sure to document that the devices
are being idled by the M3 firmware this may not be an issue. I will look
into implementing this.


yes, but doing it in M3 will ensure it is a "hardware behavior" - from 
debug perspective, you could make M3 firmware "release mode" - which it 
is stringent in terms of forcing all down to target state, OR "debug" 
where it does nothing - thus helping pick up driver bugs. With M3 in the 
picture, we now have an awesome flexibility of "defining what the 
hardware behavior should be" - that allows us to have lesser code to 
carry in kernel- especially ones(like quirks) that does not really add 
value from power saving strategies.


--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 2/9] ARM: OMAP2+: AM33XX: control: Add some control module registers and APIs


On 08/08/2013 08:44 AM, Santosh Shilimkar wrote:

On Tuesday 06 August 2013 01:49 PM, Dave Gerlach wrote:

From: Vaibhav Bedia 

Interacting with WKUP-M3 requires some more control
module register writes. Add the register offsets and
APIs to write to these.

Signed-off-by: Vaibhav Bedia 
Signed-off-by: Dave Gerlach 
---
  arch/arm/mach-omap2/control.c |   57 +
  arch/arm/mach-omap2/control.h |   54 ++
  2 files changed, 111 insertions(+)

diff --git a/arch/arm/mach-omap2/control.c b/arch/arm/mach-omap2/control.c
index 31e0dfe..934041a 100644
--- a/arch/arm/mach-omap2/control.c
+++ b/arch/arm/mach-omap2/control.c
@@ -605,3 +605,60 @@ int omap3_ctrl_save_padconf(void)
  }

  #endif /* CONFIG_ARCH_OMAP3 && CONFIG_PM */
+
+#if defined(CONFIG_SOC_AM33XX) && defined(CONFIG_PM)
+void am33xx_txev_eoi(void)
+{
+   omap_ctrl_writel(AM33XX_M3_TXEV_ACK, AM33XX_CONTROL_M3_TXEV_EOI);
+}
+
+void am33xx_txev_enable(void)
+{
+   omap_ctrl_writel(AM33XX_M3_TXEV_ENABLE, AM33XX_CONTROL_M3_TXEV_EOI);
+}
+
+/*
+ * Invalidate M3 firmware version before hardreset.
+ * Write invalid version in lower 4 nibbles of parameter
+ * register (ipc_regs + 0x8).
+ */
+void am33xx_pm_version_clear(void)
+{
+   omap_ctrl_writel(0x, AM33XX_CONTROL_IPC_MSG_REG2);
+}
+
+int am33xx_pm_version_get(void)
+{
+   return omap_ctrl_readl(AM33XX_CONTROL_IPC_MSG_REG2) & M3_VERSION_MASK;
+}
+
+void am33xx_pm_ipc_cmd(struct am33xx_ipc_data *data)
+{
+   omap_ctrl_writel(data->resume_addr, AM33XX_CONTROL_IPC_MSG_REG0);
+   omap_ctrl_writel(data->sleep_mode, AM33XX_CONTROL_IPC_MSG_REG1);
+   omap_ctrl_writel(data->param1, AM33XX_CONTROL_IPC_MSG_REG2);
+   omap_ctrl_writel(data->param2, AM33XX_CONTROL_IPC_MSG_REG3);
+   omap_ctrl_writel(data->param3, AM33XX_CONTROL_IPC_MSG_REG4);
+}
+
+int am33xx_pm_status(void)
+{
+   int i;
+
+   i = omap_ctrl_readl(AM33XX_CONTROL_IPC_MSG_REG1);
+   i &= IPC_RESP_MASK;
+   i >>= __ffs(IPC_RESP_MASK);
+
+   return i;
+}
+
+int am33xx_pm_wake_src(void)
+{
+   int i;
+
+   i = omap_ctrl_readl(AM33XX_CONTROL_IPC_MSG_REG6);
+   i &= 0xff;
+
+   return i;
+}
+#endif /* CONFIG_SOC_AM33XX && CONFIG_PM */
diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-omap2/control.h
index f7d7c2e..9be587c 100644
--- a/arch/arm/mach-omap2/control.h
+++ b/arch/arm/mach-omap2/control.h
@@ -370,6 +370,22 @@
  #define AM33XX_DEV_FEATURE0x604
  #define AM33XX_SGX_MASK   BIT(29)

+/* AM33XX M3_TXEV_EOI register */
+#define AM33XX_CONTROL_M3_TXEV_EOI 0x1324
+
+#define AM33XX_M3_TXEV_ACK (0x1 << 0)
+#define AM33XX_M3_TXEV_ENABLE  (0x0 << 0)
+
+/* AM33XX IPC message registers */
+#define AM33XX_CONTROL_IPC_MSG_REG00x1328
+#define AM33XX_CONTROL_IPC_MSG_REG10x132C
+#define AM33XX_CONTROL_IPC_MSG_REG20x1330
+#define AM33XX_CONTROL_IPC_MSG_REG30x1334
+#define AM33XX_CONTROL_IPC_MSG_REG40x1338
+#define AM33XX_CONTROL_IPC_MSG_REG50x133C
+#define AM33XX_CONTROL_IPC_MSG_REG60x1340
+#define AM33XX_CONTROL_IPC_MSG_REG70x1344
+
  /* CONTROL OMAP STATUS register to identify OMAP3 features */
  #define OMAP3_CONTROL_OMAP_STATUS 0x044c

@@ -429,6 +445,44 @@ extern void omap3630_ctrl_disable_rta(void);
  extern int omap3_ctrl_save_padconf(void);
  extern void omap2_set_globals_control(void __iomem *ctrl,
  void __iomem *ctrl_pad);
+struct am33xx_ipc_data {
+   u32 resume_addr;
+   u32 sleep_mode;
+   u32 param1;
+   u32 param2;
+   u32 param3;
+   u32 param4;
+   u32 param5;
+   u32 param6;
+};
+
+#define IPC_RESP_SHIFT 16
+#define IPC_RESP_MASK  (0x << 16)
+
+#define M3_VERSION_SHIFT   0
+#define M3_VERSION_MASK(0x << 0)
+
+/*
+ * 9-4 = VTT GPIO PIN (6 Bits)
+ *   3 = VTT Status (1 Bit)
+ * 2-0 = Memory Type (2 Bits)
+*/
+#define MEM_TYPE_SHIFT (0x0)
+#define MEM_TYPE_MASK  (0x7 << 0)
+#define VTT_STAT_SHIFT (0x3)
+#define VTT_STAT_MASK  (0x1 << 3)
+#define VTT_GPIO_PIN_SHIFT (0x4)
+#define VTT_GPIO_PIN_MASK  (0x2f << 4)
+
+extern void am33xx_wkup_m3_ipc_cmd(struct am33xx_ipc_data *data);
+extern void am33xx_txev_eoi(void);
+extern void am33xx_txev_enable(void);
+extern void am33xx_pm_version_clear(void);
+extern int am33xx_pm_version_get(void);
+extern void am33xx_pm_ipc_cmd(struct am33xx_ipc_data *data);
+extern int am33xx_pm_status(void);
+extern int am33xx_pm_wake_src(void);
+

Lets address the above better. I don't see a need of 8 functions
exported doing one or 2 register writes.

Look M3 based handling is going to be there on future SOCs
as well and this kind of handling of IPC is very short cited.



The idea here was to move all control module register accesses into one 
file in planning of implementing a driver for the contr

Re: [PATCHv3 6/9] ARM: OMAP2+: timer: Add suspend-resume callbacks for clkevent device


On 08/08/2013 09:23 AM, Santosh Shilimkar wrote:

On Tuesday 06 August 2013 01:49 PM, Dave Gerlach wrote:

From: Vaibhav Bedia 

OMAP timer code registers two timers - one as clocksource
and one as clockevent. Since AM33XX has only one usable timer
in the WKUP domain one of the timers needs suspend-resume
support to restore the configuration to pre-suspend state.

commit adc78e6 (timekeeping: Add suspend and resume
of clock event devices) introduced .suspend and .resume
callbacks for clock event devices. Leverages these
callbacks to have AM33XX clockevent timer which is
in not in WKUP domain to behave properly across system
suspend.

Signed-off-by: Vaibhav Bedia 
Signed-off-by: Dave Gerlach 
---

NAK

This patch doesn't addressed previous comments.
- The issue is specific to AM33XX and hence you
need to take care of that. These callbacks will happen on
all OMAP machines where the problem doesn't exist.

- Don't use hwmod APIs directly. At least abstract it
at omap_device layer and use that one instead.



Ok I will fix this, seems I missed those.


  arch/arm/mach-omap2/timer.c |   32 
  1 file changed, 32 insertions(+)

diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
index b37e1fc..cce5d39 100644
--- a/arch/arm/mach-omap2/timer.c
+++ b/arch/arm/mach-omap2/timer.c
@@ -118,11 +118,43 @@ static void omap2_gp_timer_set_mode(enum clock_event_mode 
mode,
}
  }

+static void omap_clkevt_suspend(struct clock_event_device *unused)
+{
+   char name[10];
+   struct omap_hwmod *oh;
+
+   sprintf(name, "timer%d", clkev.id);
+   oh = omap_hwmod_lookup(name);
+   if (!oh)
+   return;
+
+   __omap_dm_timer_stop(&clkev, 1, clkev.rate);
+   omap_hwmod_idle(oh);
+}
+
+static void omap_clkevt_resume(struct clock_event_device *unused)
+{
+   char name[10];
+   struct omap_hwmod *oh;
+
+   sprintf(name, "timer%d", clkev.id);
+   oh = omap_hwmod_lookup(name);
+   if (!oh)
+   return;
+
+   omap_hwmod_enable(oh);
+   __omap_dm_timer_load_start(&clkev,
+   OMAP_TIMER_CTRL_ST | OMAP_TIMER_CTRL_AR, 0, 1);
+   __omap_dm_timer_int_enable(&clkev, OMAP_TIMER_INT_OVERFLOW);
+}
+
  static struct clock_event_device clockevent_gpt = {
.features   = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
.rating = 300,
.set_next_event = omap2_gp_timer_set_next_event,
.set_mode   = omap2_gp_timer_set_mode,
+   .suspend= omap_clkevt_suspend,
+   .resume = omap_clkevt_resume,
  };

  static struct property device_disabled = {





--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 8/9] ARM: OMAP2+: AM33XX: Basic suspend resume support

On 08/08/2013 10:03 AM, Santosh Shilimkar wrote:

$subject and patch don't match.

On Thursday 08 August 2013 08:26 AM, Nishanth Menon wrote:

On 08/08/2013 03:45 AM, Russ Dill wrote:

In reference to
the M3 handling it, the M3 wouldn't know which devices have a driver
bound and which don't.

Does it need to? M3 firmware can pretty much define "I will force the device into low power
state, and if the drivers dont handle things properly, fix the darned driver". M3 behavior
should be considered as a "hardware" as far as Linux running on MPU is concerned, and
firmware helps change the behavior by accounting for SoC quirks. *if* we have ability to handle
this in the firmware, there is no need to carry this in Linux.

I agree with Nishant. I don't like this patch and IIRC, I gave same
comment in the last version. Linux need not know about all such firmware
quirks. Also all these M3 specific stuff, should be done somewhere
else. Probably having a small M3 driver won't be a bad idea.

Regards,
Santosh

I am not opposed to doing it this way and letting the M3 firmware handle
idling these modules, however the one concern raised in the last series
is that an approach that does not acknowledge drivers will hide driver
PM bugs. I suppose as long as I make sure to document that the devices
are being idled by the M3 firmware this may not be an issue. I will look
into implementing this.

Regards,
Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 5/9] ARM: OMAP2+: AM33XX: Add assembly code for PM operations

On Thu, Aug 8, 2013 at 8:22 AM, Santosh Shilimkar
 wrote:
> On Thursday 08 August 2013 11:16 AM, Russ Dill wrote:
>> On Thu, Aug 8, 2013 at 7:50 AM, Santosh Shilimkar
>>  wrote:
>>> On Tuesday 06 August 2013 01:49 PM, Dave Gerlach wrote:
 From: Vaibhav Bedia 

 In preparation for suspend-resume support for AM33XX, add
 the assembly file with the code which is copied to internal
 memory (OCMC RAM) during bootup and runs from there.

 As part of the low power entry (DeepSleep0 mode in AM33XX TRM),
 the code running from OCMC RAM does the following
 1. Stores the EMIF configuration
 2. Puts external memory in self-refresh
 3. Disables EMIF clock
 4. Executes WFI after writing to MPU_CLKCTRL register.

 If no interrupts have come, WFI execution on MPU gets registered
 as an interrupt with the WKUP-M3. WKUP-M3 takes care of disabling
 some clocks which MPU should not (L3, L4, OCMC RAM etc) and takes
 care of clockdomain and powerdomain transitions as part of the
 DeepSleep0 mode entry.

 In case a late interrupt comes in, WFI ends up as a NOP and MPU
 continues execution from internal memory. The 'abort path' code
 undoes whatever was done as part of the low power entry and indicates
 a suspend failure by passing a non-zero value to the cpu_resume routine.

 The 'resume path' code is similar to the 'abort path' with the key
 difference of MMU being enabled in the 'abort path' but being
 disabled in the 'resume path' due to MPU getting powered off.

 Signed-off-by: Vaibhav Bedia 
 Signed-off-by: Dave Gerlach 
 Cc: Santosh Shilimkar 
 Cc: Kevin Hilman 
 ---
  arch/arm/mach-omap2/sleep33xx.S |  350 
 +++
  1 file changed, 350 insertions(+)
  create mode 100644 arch/arm/mach-omap2/sleep33xx.S

 diff --git a/arch/arm/mach-omap2/sleep33xx.S 
 b/arch/arm/mach-omap2/sleep33xx.S
 new file mode 100644
 index 000..834c7d4
 --- /dev/null
 +++ b/arch/arm/mach-omap2/sleep33xx.S
 @@ -0,0 +1,350 @@
 +/*
 + * Low level suspend code for AM33XX SoCs
 + *
 + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/
 + * Vaibhav Bedia 
 + *
 + * This program is free software; you can redistribute it and/or
 + * modify it under the terms of the GNU General Public License as
 + * published by the Free Software Foundation version 2.
 + *
 + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
 + * kind, whether express or implied; without even the implied warranty
 + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + * GNU General Public License for more details.
 + */
 +
 +#include 
 +#include 
 +#include 
 +#include 
 +
 +#include "cm33xx.h"
 +#include "pm33xx.h"
 +#include "prm33xx.h"
 +
 + .text
 + .align 3
 +
 +/*
 + * This routine is executed from internal RAM and expects some
 + * parameters to be passed in r0 _strictly_ in following order:
 + * 1) emif_addr_virt - ioremapped EMIF address
 + * 2) mem_type - 2 -> DDR2, 3-> DDR3
 + * 3) dram_sync_word - uncached word in SDRAM
 + *
 + * The code loads these values taking r0 value as reference to
 + * the array in registers starting from r0, i.e emif_addr_virt
 + * goes to r1, mem_type goes to r2 and and so on. These are
 + * then saved into memory locations before proceeding with the
 + * sleep sequence and hence registers r0, r1 etc can still be
 + * used in the rest of the sleep code.
 + */
 +
 +ENTRY(am33xx_do_wfi)
 + stmfd   sp!, {r4 - r11, lr} @ save registers on stack
 +
 + ldm r0, {r1-r3} @ gather values passed
 +
 + /* Save the values passed */
 + str r1, emif_addr_virt
 + str r2, mem_type
 + str r3, dram_sync_word
>>>
>>> None of this parameter are going to change for every suspend entry and
>>> exit so saving them once and accessing them should be fine. Just
>>> create a structure with above, save them in init from C code and
>>> then access that structure where you need to.
>>
>> It isn't possible to do so since the structure would be in SDRAM and
>> at resume time, we don't have access to SDRAM. Additionally, I'd like
>> to expand the mem_type parameter to a bit field in the future to allow
>> this code path to be shared with CPU idle.
>>
> You can copy the structure in SRAM. So how does memtype need
> changes for CPUIDLE ?

You'd have to reserve the address and have a symbol exported for it,
then after pushing everything to SRAM, you'd have to calculate the
pushed address of the reserved space, then do a memcpy. It'd be a bit
convoluted. Passing the address to the struct into tho wfi function is
really easy and a pretty common thing to do.

Re: [PATCHv3 2/8] mailbox/omap: add support for parsing dt devices

2013-08-08 Thread Suman Anna

On 08/08/2013 09:34 AM, Kumar Gala wrote:
> 
> On Aug 7, 2013, at 3:08 PM, Suman Anna wrote:
> 
>> On 08/07/2013 12:41 PM, Kumar Gala wrote:
>>>
>>> On Aug 7, 2013, at 11:59 AM, Suman Anna wrote:
>>>
 Kumar,

>> Logic has been added to the OMAP2+ mailbox code to
>> parse the mailbox dt nodes and construct the different
>> mailboxes associated with the instance. The design is
>> based on gathering the same information that was being
>> passed previously through the platform data, except for
>> the interrupt type configuration information.
>>
>> Signed-off-by: Suman Anna 
>> ---
>> .../devicetree/bindings/mailbox/omap-mailbox.txt   |  43 +++
>> drivers/mailbox/mailbox-omap2.c| 130 
>> ++---
>> 2 files changed, 158 insertions(+), 15 deletions(-)
>> create mode 100644 
>> Documentation/devicetree/bindings/mailbox/omap-mailbox.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mailbox/omap-mailbox.txt 
>> b/Documentation/devicetree/bindings/mailbox/omap-mailbox.txt
>> new file mode 100644
>> index 000..8ffb08a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mailbox/omap-mailbox.txt
>> @@ -0,0 +1,43 @@
>> +OMAP2+ Mailbox Driver
>> +
>> +Required properties:
>> +- compatible:   Should be one of the following,
>> +"ti,omap2-mailbox" for
>> +OMAP2420, OMAP2430, OMAP3430, OMAP3630 
>> SoCs
>> +"ti,omap4-mailbox" for
>> +OMAP44xx, OMAP54xx, AM33xx, AM43xx, 
>> DRA7xx SoCs
>> +- reg:  Contains the mailbox register address range 
>> (base address
>> +and length)
>> +- interrupts:   Contains the interrupt information for the 
>> mailbox
>> +device. The format is dependent on which 
>> interrupt
>> +controller the OMAP device uses
>> +- ti,hwmods:Name of the hwmod associated with the mailbox
>> +- ti,mbox-num-users:Number of targets (processor devices) that the 
>> mailbox device
>> +can interrupt
>> +- ti,mbox-num-fifos:Number of h/w fifos within the mailbox device
>
> Isn't "ti,mbox-num-users", "ti,mbox-num-fifos" this SoC specific, why do 
> we need to encode in the device tree.  Can we not have a more SoC 
> specific compatiable and encode such info in the driver as part of the 
> .data field in of_device_id ?

 They are IP design parameters for the number of h/w fifos and interrupts
 coming out of the IP block, with the functionality identical. This
 information could not be read from any registers. Until OMAP5, we always
 had a single IP in the SoC and so these could been encoded in the
 driver. But in DRA7xx, a new SoC, we have 13 mailboxes which have
 differing number of these properties even though the functional IP block
 is same.
>>>
>>> Ah, I see.  Since you've got examples of the same IP with different design 
>>> params in a given SoC than this makes sense.
>>>
>>> Is that true of ti,mbox-num-users?
>>
>> Yes, it is true of both "ti,mbox-num-users" and "ti,mbox-num-fifos".
> 
> So I think it would be good to update the binding to convey that SoCs might 
> have multiple mbox units w/different design pararms (maybe a short blurb as 
> part of the intro).

Sure will do. Will wait for Benoit also to come back on this series if I
need to address any further review comments.

regards
Suman

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: How to use gpio-omap as interrupt-controller

On Thursday 08 August 2013 11:23 AM, Lars Poeschel wrote:
> Hi!
> 
> I have a device-tree-booting omap board that uses gpio-omap as gpio driver. 
> Kernel version is 3.11.0-rc4. I have connected a device that signals 
> interrupts to a gpio pin of the omap. The driver for this device fails in 
> request_threaded_irq.
> The irq framework tries to setup the irq in __setup_irq which calls 
> gpio_irq_type in gpio-omap.c. This function checks if bank->mod_usage is 
> set and because it is not, the function fails. Looking at where bank-
>> mod_usage is set, I see it is only set in omap_gpio_request.
> This means I have to request at least one random gpio to be able to set the 
> type of the irq of another pin on this bank ?
> How do I correctly use the gpio-omap gpio driver in my case ?
> The board is booting using device tree and does not request a gpio prior to 
> requesting the irq on this gpio bank. I really do not want to request a 
> gpio. They should stay as they are.
> Or does this mean the driver of the connected device is wrong and instead 
> it has to request some random gpio before ?
> 
> An example of such a connected device is gpio-adnp by the way.
> The device tree part looks like this:
> 
>   gpioext: gpio-adnp@41 {
>   compatible = "ad,gpio-adnp";
>   reg = <0x41>;
> 
>   interrupt-parent = <&gpio>;
>   interrupts = <160 1>;
> 
>   gpio-controller;
>   #gpio-cells = <1>;
> 
>   interrupt-controller;
>   #interrupt-cells = <2>;
> 
>   nr-gpios = <64>;
>   };
> 
You hit the issue which we tried to address but ended
up reverting the patches. refer [1], [2] for more
information. For now, its broken unfortunately.
Regards,
Santosh


[1] http://www.mail-archive.com/linux-omap@vger.kernel.org/msg91405.html
[2] https://lkml.org/lkml/2013/7/29/280
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 5/9] ARM: OMAP2+: AM33XX: Add assembly code for PM operations

On Thursday 08 August 2013 11:16 AM, Russ Dill wrote:
> On Thu, Aug 8, 2013 at 7:50 AM, Santosh Shilimkar
>  wrote:
>> On Tuesday 06 August 2013 01:49 PM, Dave Gerlach wrote:
>>> From: Vaibhav Bedia 
>>>
>>> In preparation for suspend-resume support for AM33XX, add
>>> the assembly file with the code which is copied to internal
>>> memory (OCMC RAM) during bootup and runs from there.
>>>
>>> As part of the low power entry (DeepSleep0 mode in AM33XX TRM),
>>> the code running from OCMC RAM does the following
>>> 1. Stores the EMIF configuration
>>> 2. Puts external memory in self-refresh
>>> 3. Disables EMIF clock
>>> 4. Executes WFI after writing to MPU_CLKCTRL register.
>>>
>>> If no interrupts have come, WFI execution on MPU gets registered
>>> as an interrupt with the WKUP-M3. WKUP-M3 takes care of disabling
>>> some clocks which MPU should not (L3, L4, OCMC RAM etc) and takes
>>> care of clockdomain and powerdomain transitions as part of the
>>> DeepSleep0 mode entry.
>>>
>>> In case a late interrupt comes in, WFI ends up as a NOP and MPU
>>> continues execution from internal memory. The 'abort path' code
>>> undoes whatever was done as part of the low power entry and indicates
>>> a suspend failure by passing a non-zero value to the cpu_resume routine.
>>>
>>> The 'resume path' code is similar to the 'abort path' with the key
>>> difference of MMU being enabled in the 'abort path' but being
>>> disabled in the 'resume path' due to MPU getting powered off.
>>>
>>> Signed-off-by: Vaibhav Bedia 
>>> Signed-off-by: Dave Gerlach 
>>> Cc: Santosh Shilimkar 
>>> Cc: Kevin Hilman 
>>> ---
>>>  arch/arm/mach-omap2/sleep33xx.S |  350 
>>> +++
>>>  1 file changed, 350 insertions(+)
>>>  create mode 100644 arch/arm/mach-omap2/sleep33xx.S
>>>
>>> diff --git a/arch/arm/mach-omap2/sleep33xx.S 
>>> b/arch/arm/mach-omap2/sleep33xx.S
>>> new file mode 100644
>>> index 000..834c7d4
>>> --- /dev/null
>>> +++ b/arch/arm/mach-omap2/sleep33xx.S
>>> @@ -0,0 +1,350 @@
>>> +/*
>>> + * Low level suspend code for AM33XX SoCs
>>> + *
>>> + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/
>>> + * Vaibhav Bedia 
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU General Public License as
>>> + * published by the Free Software Foundation version 2.
>>> + *
>>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
>>> + * kind, whether express or implied; without even the implied warranty
>>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +#include "cm33xx.h"
>>> +#include "pm33xx.h"
>>> +#include "prm33xx.h"
>>> +
>>> + .text
>>> + .align 3
>>> +
>>> +/*
>>> + * This routine is executed from internal RAM and expects some
>>> + * parameters to be passed in r0 _strictly_ in following order:
>>> + * 1) emif_addr_virt - ioremapped EMIF address
>>> + * 2) mem_type - 2 -> DDR2, 3-> DDR3
>>> + * 3) dram_sync_word - uncached word in SDRAM
>>> + *
>>> + * The code loads these values taking r0 value as reference to
>>> + * the array in registers starting from r0, i.e emif_addr_virt
>>> + * goes to r1, mem_type goes to r2 and and so on. These are
>>> + * then saved into memory locations before proceeding with the
>>> + * sleep sequence and hence registers r0, r1 etc can still be
>>> + * used in the rest of the sleep code.
>>> + */
>>> +
>>> +ENTRY(am33xx_do_wfi)
>>> + stmfd   sp!, {r4 - r11, lr} @ save registers on stack
>>> +
>>> + ldm r0, {r1-r3} @ gather values passed
>>> +
>>> + /* Save the values passed */
>>> + str r1, emif_addr_virt
>>> + str r2, mem_type
>>> + str r3, dram_sync_word
>>
>> None of this parameter are going to change for every suspend entry and
>> exit so saving them once and accessing them should be fine. Just
>> create a structure with above, save them in init from C code and
>> then access that structure where you need to.
> 
> It isn't possible to do so since the structure would be in SDRAM and
> at resume time, we don't have access to SDRAM. Additionally, I'd like
> to expand the mem_type parameter to a bit field in the future to allow
> this code path to be shared with CPU idle.
>
You can copy the structure in SRAM. So how does memtype need
changes for CPUIDLE ?

I have several comments on this patch so I assume you are
going to address them then.

Regards,
Santosh


--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

How to use gpio-omap as interrupt-controller

2013-08-08 Thread Lars Poeschel

Hi!

I have a device-tree-booting omap board that uses gpio-omap as gpio driver. 
Kernel version is 3.11.0-rc4. I have connected a device that signals 
interrupts to a gpio pin of the omap. The driver for this device fails in 
request_threaded_irq.
The irq framework tries to setup the irq in __setup_irq which calls 
gpio_irq_type in gpio-omap.c. This function checks if bank->mod_usage is 
set and because it is not, the function fails. Looking at where bank-
>mod_usage is set, I see it is only set in omap_gpio_request.
This means I have to request at least one random gpio to be able to set the 
type of the irq of another pin on this bank ?
How do I correctly use the gpio-omap gpio driver in my case ?
The board is booting using device tree and does not request a gpio prior to 
requesting the irq on this gpio bank. I really do not want to request a 
gpio. They should stay as they are.
Or does this mean the driver of the connected device is wrong and instead 
it has to request some random gpio before ?

An example of such a connected device is gpio-adnp by the way.
The device tree part looks like this:

gpioext: gpio-adnp@41 {
compatible = "ad,gpio-adnp";
reg = <0x41>;

interrupt-parent = <&gpio>;
interrupts = <160 1>;

gpio-controller;
#gpio-cells = <1>;

interrupt-controller;
#interrupt-cells = <2>;

nr-gpios = <64>;
};

Regards,
Lars
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 5/9] ARM: OMAP2+: AM33XX: Add assembly code for PM operations

On Thu, Aug 8, 2013 at 7:50 AM, Santosh Shilimkar
 wrote:
> On Tuesday 06 August 2013 01:49 PM, Dave Gerlach wrote:
>> From: Vaibhav Bedia 
>>
>> In preparation for suspend-resume support for AM33XX, add
>> the assembly file with the code which is copied to internal
>> memory (OCMC RAM) during bootup and runs from there.
>>
>> As part of the low power entry (DeepSleep0 mode in AM33XX TRM),
>> the code running from OCMC RAM does the following
>> 1. Stores the EMIF configuration
>> 2. Puts external memory in self-refresh
>> 3. Disables EMIF clock
>> 4. Executes WFI after writing to MPU_CLKCTRL register.
>>
>> If no interrupts have come, WFI execution on MPU gets registered
>> as an interrupt with the WKUP-M3. WKUP-M3 takes care of disabling
>> some clocks which MPU should not (L3, L4, OCMC RAM etc) and takes
>> care of clockdomain and powerdomain transitions as part of the
>> DeepSleep0 mode entry.
>>
>> In case a late interrupt comes in, WFI ends up as a NOP and MPU
>> continues execution from internal memory. The 'abort path' code
>> undoes whatever was done as part of the low power entry and indicates
>> a suspend failure by passing a non-zero value to the cpu_resume routine.
>>
>> The 'resume path' code is similar to the 'abort path' with the key
>> difference of MMU being enabled in the 'abort path' but being
>> disabled in the 'resume path' due to MPU getting powered off.
>>
>> Signed-off-by: Vaibhav Bedia 
>> Signed-off-by: Dave Gerlach 
>> Cc: Santosh Shilimkar 
>> Cc: Kevin Hilman 
>> ---
>>  arch/arm/mach-omap2/sleep33xx.S |  350 
>> +++
>>  1 file changed, 350 insertions(+)
>>  create mode 100644 arch/arm/mach-omap2/sleep33xx.S
>>
>> diff --git a/arch/arm/mach-omap2/sleep33xx.S 
>> b/arch/arm/mach-omap2/sleep33xx.S
>> new file mode 100644
>> index 000..834c7d4
>> --- /dev/null
>> +++ b/arch/arm/mach-omap2/sleep33xx.S
>> @@ -0,0 +1,350 @@
>> +/*
>> + * Low level suspend code for AM33XX SoCs
>> + *
>> + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/
>> + * Vaibhav Bedia 
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation version 2.
>> + *
>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
>> + * kind, whether express or implied; without even the implied warranty
>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include "cm33xx.h"
>> +#include "pm33xx.h"
>> +#include "prm33xx.h"
>> +
>> + .text
>> + .align 3
>> +
>> +/*
>> + * This routine is executed from internal RAM and expects some
>> + * parameters to be passed in r0 _strictly_ in following order:
>> + * 1) emif_addr_virt - ioremapped EMIF address
>> + * 2) mem_type - 2 -> DDR2, 3-> DDR3
>> + * 3) dram_sync_word - uncached word in SDRAM
>> + *
>> + * The code loads these values taking r0 value as reference to
>> + * the array in registers starting from r0, i.e emif_addr_virt
>> + * goes to r1, mem_type goes to r2 and and so on. These are
>> + * then saved into memory locations before proceeding with the
>> + * sleep sequence and hence registers r0, r1 etc can still be
>> + * used in the rest of the sleep code.
>> + */
>> +
>> +ENTRY(am33xx_do_wfi)
>> + stmfd   sp!, {r4 - r11, lr} @ save registers on stack
>> +
>> + ldm r0, {r1-r3} @ gather values passed
>> +
>> + /* Save the values passed */
>> + str r1, emif_addr_virt
>> + str r2, mem_type
>> + str r3, dram_sync_word
>
> None of this parameter are going to change for every suspend entry and
> exit so saving them once and accessing them should be fine. Just
> create a structure with above, save them in init from C code and
> then access that structure where you need to.

It isn't possible to do so since the structure would be in SDRAM and
at resume time, we don't have access to SDRAM. Additionally, I'd like
to expand the mem_type parameter to a bit field in the future to allow
this code path to be shared with CPU idle.

>> +
>> + /*
>> +  * Flush all data from the L1 data cache before disabling
>> +  * SCTLR.C bit.
>> +  */
>> + ldr r1, kernel_flush
>> + blx r1
>> +
>> + /*
>> +  * Clear the SCTLR.C bit to prevent further data cache
>> +  * allocation. Clearing SCTLR.C would make all the data accesses
>> +  * strongly ordered and would not hit the cache.
>> +  */
>> + mrc p15, 0, r0, c1, c0, 0
>> + bic r0, r0, #(1 << 2)   @ Disable the C bit
>> + mcr p15, 0, r0, c1, c0, 0
>> + isb
>> +
>> + /*
>> +  * Invalidate L1 data cache. Even though only invalidate is
>> +  * necessary exported flush API is used here. Doing clean
>> +  * on already clean cache would be

Re: [PATCHv3 8/9] ARM: OMAP2+: AM33XX: Basic suspend resume support

$subject and patch don't match.

On Thursday 08 August 2013 08:26 AM, Nishanth Menon wrote:
> On 08/08/2013 03:45 AM, Russ Dill wrote:
>>   In reference to
>> the M3 handling it, the M3 wouldn't know which devices have a driver
>> bound and which don't.
> Does it need to? M3 firmware can pretty much define "I will force the device 
> into low power state, and if the drivers dont handle things properly, fix the 
> darned driver". M3 behavior should be considered as a "hardware" as far as 
> Linux running on MPU is concerned, and firmware helps change the behavior by 
> accounting for SoC quirks. *if* we have ability to handle this in the 
> firmware, there is no need to carry this in Linux.
> 
I agree with Nishant. I don't like this patch and IIRC, I gave same
comment in the last version. Linux need not know about all such firmware
quirks. Also all these M3 specific stuff, should be done somewhere
else. Probably having a small M3 driver won't be a bad idea.

Regards,
Santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 9/9] ARM: OMAP2+: AM33XX: Hookup AM33XX PM code into OMAP builds

On Tuesday 06 August 2013 01:49 PM, Dave Gerlach wrote:
> From: Vaibhav Bedia 
> 
> With all the requisite changes in place we can now
> enable the basic PM support for AM335x. This patch
> updates the various OMAP files to enable suspend-resume
> on AM335x.
> 
> Signed-off-by: Vaibhav Bedia 
> Signed-off-by: Dave Gerlach 
> ---
>  arch/arm/mach-omap2/Kconfig |7 +--
>  arch/arm/mach-omap2/Makefile|2 ++
>  arch/arm/mach-omap2/board-generic.c |1 +
>  arch/arm/mach-omap2/common.h|   10 ++
>  arch/arm/mach-omap2/io.c|5 +
>  arch/arm/mach-omap2/pm.c|3 ++-
>  arch/arm/mach-omap2/pm.h|5 +
>  arch/arm/mach-omap2/sram.c  |   10 +-
>  arch/arm/mach-omap2/sram.h  |2 ++
>  9 files changed, 41 insertions(+), 4 deletions(-)
> 
The patch looks fine but too many changes in one patch
than my liking. No strong preference though.

Acked-by: Santosh Shilikar

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 5/9] ARM: OMAP2+: AM33XX: Add assembly code for PM operations

On Tuesday 06 August 2013 01:49 PM, Dave Gerlach wrote:
> From: Vaibhav Bedia 
> 
> In preparation for suspend-resume support for AM33XX, add
> the assembly file with the code which is copied to internal
> memory (OCMC RAM) during bootup and runs from there.
> 
> As part of the low power entry (DeepSleep0 mode in AM33XX TRM),
> the code running from OCMC RAM does the following
> 1. Stores the EMIF configuration
> 2. Puts external memory in self-refresh
> 3. Disables EMIF clock
> 4. Executes WFI after writing to MPU_CLKCTRL register.
> 
> If no interrupts have come, WFI execution on MPU gets registered
> as an interrupt with the WKUP-M3. WKUP-M3 takes care of disabling
> some clocks which MPU should not (L3, L4, OCMC RAM etc) and takes
> care of clockdomain and powerdomain transitions as part of the
> DeepSleep0 mode entry.
> 
> In case a late interrupt comes in, WFI ends up as a NOP and MPU
> continues execution from internal memory. The 'abort path' code
> undoes whatever was done as part of the low power entry and indicates
> a suspend failure by passing a non-zero value to the cpu_resume routine.
> 
> The 'resume path' code is similar to the 'abort path' with the key
> difference of MMU being enabled in the 'abort path' but being
> disabled in the 'resume path' due to MPU getting powered off.
> 
> Signed-off-by: Vaibhav Bedia 
> Signed-off-by: Dave Gerlach 
> Cc: Santosh Shilimkar 
> Cc: Kevin Hilman 
> ---
>  arch/arm/mach-omap2/sleep33xx.S |  350 
> +++
>  1 file changed, 350 insertions(+)
>  create mode 100644 arch/arm/mach-omap2/sleep33xx.S
> 
> diff --git a/arch/arm/mach-omap2/sleep33xx.S b/arch/arm/mach-omap2/sleep33xx.S
> new file mode 100644
> index 000..834c7d4
> --- /dev/null
> +++ b/arch/arm/mach-omap2/sleep33xx.S
> @@ -0,0 +1,350 @@
> +/*
> + * Low level suspend code for AM33XX SoCs
> + *
> + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/
> + * Vaibhav Bedia 
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "cm33xx.h"
> +#include "pm33xx.h"
> +#include "prm33xx.h"
> +
> + .text
> + .align 3
> +
> +/*
> + * This routine is executed from internal RAM and expects some
> + * parameters to be passed in r0 _strictly_ in following order:
> + * 1) emif_addr_virt - ioremapped EMIF address
> + * 2) mem_type - 2 -> DDR2, 3-> DDR3
> + * 3) dram_sync_word - uncached word in SDRAM
> + *
> + * The code loads these values taking r0 value as reference to
> + * the array in registers starting from r0, i.e emif_addr_virt
> + * goes to r1, mem_type goes to r2 and and so on. These are
> + * then saved into memory locations before proceeding with the
> + * sleep sequence and hence registers r0, r1 etc can still be
> + * used in the rest of the sleep code.
> + */
> +
> +ENTRY(am33xx_do_wfi)
> + stmfd   sp!, {r4 - r11, lr} @ save registers on stack
> +
> + ldm r0, {r1-r3} @ gather values passed
> +
> + /* Save the values passed */
> + str r1, emif_addr_virt
> + str r2, mem_type
> + str r3, dram_sync_word

None of this parameter are going to change for every suspend entry and
exit so saving them once and accessing them should be fine. Just
create a structure with above, save them in init from C code and
then access that structure where you need to.

> +
> + /*
> +  * Flush all data from the L1 data cache before disabling
> +  * SCTLR.C bit.
> +  */
> + ldr r1, kernel_flush
> + blx r1
> +
> + /*
> +  * Clear the SCTLR.C bit to prevent further data cache
> +  * allocation. Clearing SCTLR.C would make all the data accesses
> +  * strongly ordered and would not hit the cache.
> +  */
> + mrc p15, 0, r0, c1, c0, 0
> + bic r0, r0, #(1 << 2)   @ Disable the C bit
> + mcr p15, 0, r0, c1, c0, 0
> + isb
> +
> + /*
> +  * Invalidate L1 data cache. Even though only invalidate is
> +  * necessary exported flush API is used here. Doing clean
> +  * on already clean cache would be almost NOP.
> +  */
Comment is stale for AM33XX since below flush will clean l1 and l2
together. We need to first flush and then invalidate. Please update it.
> + ldr r1, kernel_flush
> + blx r1
> +
> + ldr r0, emif_addr_virt
> + /* Save EMIF configuration */
> + ldr r1, [r0, #EMIF_SDRAM_CONFIG]
> + str r1, emif_sdcfg_val
> + ldr r1, [r0, #EMIF_SDRAM_REFRESH_CONTROL]
> + str

Re: [PATCHv3 2/8] mailbox/omap: add support for parsing dt devices

2013-08-08 Thread Kumar Gala


On Aug 7, 2013, at 3:08 PM, Suman Anna wrote:

> On 08/07/2013 12:41 PM, Kumar Gala wrote:
>> 
>> On Aug 7, 2013, at 11:59 AM, Suman Anna wrote:
>> 
>>> Kumar,
>>> 
> Logic has been added to the OMAP2+ mailbox code to
> parse the mailbox dt nodes and construct the different
> mailboxes associated with the instance. The design is
> based on gathering the same information that was being
> passed previously through the platform data, except for
> the interrupt type configuration information.
> 
> Signed-off-by: Suman Anna 
> ---
> .../devicetree/bindings/mailbox/omap-mailbox.txt   |  43 +++
> drivers/mailbox/mailbox-omap2.c| 130 
> ++---
> 2 files changed, 158 insertions(+), 15 deletions(-)
> create mode 100644 
> Documentation/devicetree/bindings/mailbox/omap-mailbox.txt
> 
> diff --git a/Documentation/devicetree/bindings/mailbox/omap-mailbox.txt 
> b/Documentation/devicetree/bindings/mailbox/omap-mailbox.txt
> new file mode 100644
> index 000..8ffb08a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/omap-mailbox.txt
> @@ -0,0 +1,43 @@
> +OMAP2+ Mailbox Driver
> +
> +Required properties:
> +- compatible:Should be one of the following,
> + "ti,omap2-mailbox" for
> + OMAP2420, OMAP2430, OMAP3430, OMAP3630 SoCs
> + "ti,omap4-mailbox" for
> + OMAP44xx, OMAP54xx, AM33xx, AM43xx, DRA7xx SoCs
> +- reg:   Contains the mailbox register address range 
> (base address
> + and length)
> +- interrupts:Contains the interrupt information for the 
> mailbox
> + device. The format is dependent on which interrupt
> + controller the OMAP device uses
> +- ti,hwmods: Name of the hwmod associated with the mailbox
> +- ti,mbox-num-users: Number of targets (processor devices) that the 
> mailbox device
> + can interrupt
> +- ti,mbox-num-fifos: Number of h/w fifos within the mailbox device
 
 Isn't "ti,mbox-num-users", "ti,mbox-num-fifos" this SoC specific, why do 
 we need to encode in the device tree.  Can we not have a more SoC specific 
 compatiable and encode such info in the driver as part of the .data field 
 in of_device_id ?
>>> 
>>> They are IP design parameters for the number of h/w fifos and interrupts
>>> coming out of the IP block, with the functionality identical. This
>>> information could not be read from any registers. Until OMAP5, we always
>>> had a single IP in the SoC and so these could been encoded in the
>>> driver. But in DRA7xx, a new SoC, we have 13 mailboxes which have
>>> differing number of these properties even though the functional IP block
>>> is same.
>> 
>> Ah, I see.  Since you've got examples of the same IP with different design 
>> params in a given SoC than this makes sense.
>> 
>> Is that true of ti,mbox-num-users?
> 
> Yes, it is true of both "ti,mbox-num-users" and "ti,mbox-num-fifos".

So I think it would be good to update the binding to convey that SoCs might 
have multiple mbox units w/different design pararms (maybe a short blurb as 
part of the intro).

- k

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by 
The Linux Foundation

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 7/9] ARM: OMAP: omap_device: Add APIs to enable and idle hwmods

On Tuesday 06 August 2013 01:49 PM, Dave Gerlach wrote:
> From: Vaibhav Bedia 
> 
> Needed to let the AM335x PM handle the IPs which need forced
> standby transition during every suspend-resume cycle when
> the corresponding driver is not compiled into the kernel.
> 
> Signed-off-by: Vaibhav Bedia 
> Signed-off-by: Dave Gerlach 
> ---
>  arch/arm/mach-omap2/omap_device.c |8 
>  arch/arm/mach-omap2/omap_device.h |2 ++
>  2 files changed, 10 insertions(+)
> 
The interface might be sane but the need of force standby
is just non-sense. Why can't we take care of this in
firmware itself. Linux doesn't need to know about it.
More comments on this in other patch where it is being used.

Regards,
Santosh

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 6/9] ARM: OMAP2+: timer: Add suspend-resume callbacks for clkevent device

On Tuesday 06 August 2013 01:49 PM, Dave Gerlach wrote:
> From: Vaibhav Bedia 
> 
> OMAP timer code registers two timers - one as clocksource
> and one as clockevent. Since AM33XX has only one usable timer
> in the WKUP domain one of the timers needs suspend-resume
> support to restore the configuration to pre-suspend state.
> 
> commit adc78e6 (timekeeping: Add suspend and resume
> of clock event devices) introduced .suspend and .resume
> callbacks for clock event devices. Leverages these
> callbacks to have AM33XX clockevent timer which is
> in not in WKUP domain to behave properly across system
> suspend.
> 
> Signed-off-by: Vaibhav Bedia 
> Signed-off-by: Dave Gerlach 
> ---
NAK

This patch doesn't addressed previous comments.
- The issue is specific to AM33XX and hence you
need to take care of that. These callbacks will happen on
all OMAP machines where the problem doesn't exist.

- Don't use hwmod APIs directly. At least abstract it
at omap_device layer and use that one instead.

>  arch/arm/mach-omap2/timer.c |   32 
>  1 file changed, 32 insertions(+)
> 
> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> index b37e1fc..cce5d39 100644
> --- a/arch/arm/mach-omap2/timer.c
> +++ b/arch/arm/mach-omap2/timer.c
> @@ -118,11 +118,43 @@ static void omap2_gp_timer_set_mode(enum 
> clock_event_mode mode,
>   }
>  }
>  
> +static void omap_clkevt_suspend(struct clock_event_device *unused)
> +{
> + char name[10];
> + struct omap_hwmod *oh;
> +
> + sprintf(name, "timer%d", clkev.id);
> + oh = omap_hwmod_lookup(name);
> + if (!oh)
> + return;
> +
> + __omap_dm_timer_stop(&clkev, 1, clkev.rate);
> + omap_hwmod_idle(oh);
> +}
> +
> +static void omap_clkevt_resume(struct clock_event_device *unused)
> +{
> + char name[10];
> + struct omap_hwmod *oh;
> +
> + sprintf(name, "timer%d", clkev.id);
> + oh = omap_hwmod_lookup(name);
> + if (!oh)
> + return;
> +
> + omap_hwmod_enable(oh);
> + __omap_dm_timer_load_start(&clkev,
> + OMAP_TIMER_CTRL_ST | OMAP_TIMER_CTRL_AR, 0, 1);
> + __omap_dm_timer_int_enable(&clkev, OMAP_TIMER_INT_OVERFLOW);
> +}
> +
>  static struct clock_event_device clockevent_gpt = {
>   .features   = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
>   .rating = 300,
>   .set_next_event = omap2_gp_timer_set_next_event,
>   .set_mode   = omap2_gp_timer_set_mode,
> + .suspend= omap_clkevt_suspend,
> + .resume = omap_clkevt_resume,
>  };
>  
>  static struct property device_disabled = {
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 4/9] ARM: OMAP2+: AM33XX: Reserve memory to comply with EMIF spec

On Tuesday 06 August 2013 01:49 PM, Dave Gerlach wrote:
> From: Vaibhav Bedia 
> 
> SDRAM controller on AM33XX requires that a modification of certain
> bit-fields in PWR_MGMT_CTRL register (ref. section 7.3.5.13 in
> AM335x-Rev H) is followed by a dummy read access to SDRAM. This
> scenario arises when entering a low power state like DeepSleep.
> To ensure that the read is not from a cached region we reserve
> some memory during bootup using the arm_memblock_steal() API.
> 
> A subsequent patch will pass along the location of the reserved
> memory location to the AM335x suspend handler which modifies the
> PWR_MGMT_CTRL register in the EMIF.
> 
> Signed-off-by: Vaibhav Bedia 
> Signed-off-by: Dave Gerlach 
> ---
Acked-by: Santosh Shilimkar 

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

ARM: AM335x: Reboot broken in 3.11

2013-08-08 Thread Mark Jackson

Rebooting appears to have broken in 3.11 (at some point before rc1).

Here is the console output:-

[0.00] Booting Linux on physical CPU 0x0
[0.00] Linux version 3.11.0-rc1-6-gf550793 (mpfj@mpfj-nanobone) 
(gcc version 4.6.3 (Buildroot 2013.02-dirty) ) #328 Thu Aug 8 14:36:16 BST 2013
...
Welcome to Buildroot
nanobone login: root
Password:
# reboot
#
[   23.867076] UBIFS: background thread "ubifs_bgt0_0" stops
The system is going down NOW!
Sent SIGTERM to all processes
Sent SIGKILL to all processes
Requesting system reboot
[   25.924496] reboot: Restarting system

... and at this point the CPU seems to just freeze.

In 3v10, the board would reboot correctly back into uboot, etc.

I've also noticed that some of the output LEDs light up dim when the kernel
is booting on, and they come on full brightness at the reboot "freeze" point.
There are 4 LEDs affected and they are all connected to UART transmit pins.

Before I start bisecting, does anyone have any ideas ?

Cheers
Mark J.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 3/9] ARM: OMAP: DTB: Update IRQ data for WKUP_M3

On Tuesday 06 August 2013 01:49 PM, Dave Gerlach wrote:
> From: Vaibhav Bedia 
> 
> Allow interrupt for wkup_m3 to be set from DT.
> 
> Signed-off-by: Vaibhav Bedia 
> Signed-off-by: Dave Gerlach 
> Cc: Benoit Cousson 
> ---
Acked-by: Santosh Shilimkar 

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 2/9] ARM: OMAP2+: AM33XX: control: Add some control module registers and APIs

On Tuesday 06 August 2013 01:49 PM, Dave Gerlach wrote:
> From: Vaibhav Bedia 
> 
> Interacting with WKUP-M3 requires some more control
> module register writes. Add the register offsets and
> APIs to write to these.
> 
> Signed-off-by: Vaibhav Bedia 
> Signed-off-by: Dave Gerlach 
> ---
>  arch/arm/mach-omap2/control.c |   57 
> +
>  arch/arm/mach-omap2/control.h |   54 ++
>  2 files changed, 111 insertions(+)
> 
> diff --git a/arch/arm/mach-omap2/control.c b/arch/arm/mach-omap2/control.c
> index 31e0dfe..934041a 100644
> --- a/arch/arm/mach-omap2/control.c
> +++ b/arch/arm/mach-omap2/control.c
> @@ -605,3 +605,60 @@ int omap3_ctrl_save_padconf(void)
>  }
>  
>  #endif /* CONFIG_ARCH_OMAP3 && CONFIG_PM */
> +
> +#if defined(CONFIG_SOC_AM33XX) && defined(CONFIG_PM)
> +void am33xx_txev_eoi(void)
> +{
> + omap_ctrl_writel(AM33XX_M3_TXEV_ACK, AM33XX_CONTROL_M3_TXEV_EOI);
> +}
> +
> +void am33xx_txev_enable(void)
> +{
> + omap_ctrl_writel(AM33XX_M3_TXEV_ENABLE, AM33XX_CONTROL_M3_TXEV_EOI);
> +}
> +
> +/*
> + * Invalidate M3 firmware version before hardreset.
> + * Write invalid version in lower 4 nibbles of parameter
> + * register (ipc_regs + 0x8).
> + */
> +void am33xx_pm_version_clear(void)
> +{
> + omap_ctrl_writel(0x, AM33XX_CONTROL_IPC_MSG_REG2);
> +}
> +
> +int am33xx_pm_version_get(void)
> +{
> + return omap_ctrl_readl(AM33XX_CONTROL_IPC_MSG_REG2) & M3_VERSION_MASK;
> +}
> +
> +void am33xx_pm_ipc_cmd(struct am33xx_ipc_data *data)
> +{
> + omap_ctrl_writel(data->resume_addr, AM33XX_CONTROL_IPC_MSG_REG0);
> + omap_ctrl_writel(data->sleep_mode, AM33XX_CONTROL_IPC_MSG_REG1);
> + omap_ctrl_writel(data->param1, AM33XX_CONTROL_IPC_MSG_REG2);
> + omap_ctrl_writel(data->param2, AM33XX_CONTROL_IPC_MSG_REG3);
> + omap_ctrl_writel(data->param3, AM33XX_CONTROL_IPC_MSG_REG4);
> +}
> +
> +int am33xx_pm_status(void)
> +{
> + int i;
> +
> + i = omap_ctrl_readl(AM33XX_CONTROL_IPC_MSG_REG1);
> + i &= IPC_RESP_MASK;
> + i >>= __ffs(IPC_RESP_MASK);
> +
> + return i;
> +}
> +
> +int am33xx_pm_wake_src(void)
> +{
> + int i;
> +
> + i = omap_ctrl_readl(AM33XX_CONTROL_IPC_MSG_REG6);
> + i &= 0xff;
> +
> + return i;
> +}
> +#endif /* CONFIG_SOC_AM33XX && CONFIG_PM */
> diff --git a/arch/arm/mach-omap2/control.h b/arch/arm/mach-omap2/control.h
> index f7d7c2e..9be587c 100644
> --- a/arch/arm/mach-omap2/control.h
> +++ b/arch/arm/mach-omap2/control.h
> @@ -370,6 +370,22 @@
>  #define AM33XX_DEV_FEATURE   0x604
>  #define AM33XX_SGX_MASK  BIT(29)
>  
> +/* AM33XX M3_TXEV_EOI register */
> +#define AM33XX_CONTROL_M3_TXEV_EOI   0x1324
> +
> +#define AM33XX_M3_TXEV_ACK   (0x1 << 0)
> +#define AM33XX_M3_TXEV_ENABLE(0x0 << 0)
> +
> +/* AM33XX IPC message registers */
> +#define AM33XX_CONTROL_IPC_MSG_REG0  0x1328
> +#define AM33XX_CONTROL_IPC_MSG_REG1  0x132C
> +#define AM33XX_CONTROL_IPC_MSG_REG2  0x1330
> +#define AM33XX_CONTROL_IPC_MSG_REG3  0x1334
> +#define AM33XX_CONTROL_IPC_MSG_REG4  0x1338
> +#define AM33XX_CONTROL_IPC_MSG_REG5  0x133C
> +#define AM33XX_CONTROL_IPC_MSG_REG6  0x1340
> +#define AM33XX_CONTROL_IPC_MSG_REG7  0x1344
> +
>  /* CONTROL OMAP STATUS register to identify OMAP3 features */
>  #define OMAP3_CONTROL_OMAP_STATUS0x044c
>  
> @@ -429,6 +445,44 @@ extern void omap3630_ctrl_disable_rta(void);
>  extern int omap3_ctrl_save_padconf(void);
>  extern void omap2_set_globals_control(void __iomem *ctrl,
> void __iomem *ctrl_pad);
> +struct am33xx_ipc_data {
> + u32 resume_addr;
> + u32 sleep_mode;
> + u32 param1;
> + u32 param2;
> + u32 param3;
> + u32 param4;
> + u32 param5;
> + u32 param6;
> +};
> +
> +#define IPC_RESP_SHIFT   16
> +#define IPC_RESP_MASK(0x << 16)
> +
> +#define M3_VERSION_SHIFT 0
> +#define M3_VERSION_MASK  (0x << 0)
> +
> +/*
> + * 9-4 = VTT GPIO PIN (6 Bits)
> + *   3 = VTT Status (1 Bit)
> + * 2-0 = Memory Type (2 Bits)
> +*/
> +#define MEM_TYPE_SHIFT   (0x0)
> +#define MEM_TYPE_MASK(0x7 << 0)
> +#define VTT_STAT_SHIFT   (0x3)
> +#define VTT_STAT_MASK(0x1 << 3)
> +#define VTT_GPIO_PIN_SHIFT   (0x4)
> +#define VTT_GPIO_PIN_MASK(0x2f << 4)
> +
> +extern void am33xx_wkup_m3_ipc_cmd(struct am33xx_ipc_data *data);
> +extern void am33xx_txev_eoi(void);
> +extern void am33xx_txev_enable(void);
> +extern void am33xx_pm_version_clear(void);
> +extern int am33xx_pm_version_get(void);
> +extern void am33xx_pm_ipc_cmd(struct am33xx_ipc_data *data);
> +extern int am33xx_pm_status(void);
> +extern int am33xx_pm_wake_src(void);
> +
Lets address the above better. I don't see a need of 8 functions
exported doing one or 2 register writes.

Look M3 based handling is going to be there on future SOCs

Re: [PATCHv3 1/9] memory: emif: Move EMIF register defines to include/linux/

(You have not CC'ed Greg, Looping him)

On Tuesday 06 August 2013 01:49 PM, Dave Gerlach wrote:
> OMAP4 and AM33XX share the same EMIF controller IP. Although there
> are significant differences in the IP integration due to which
> AM33XX can't reuse the EMIF driver DVFS similar to OMAP4,
> it can definitely benefit by reusing the EMIF related macros
> defined in drivers/memory/emif.h.
> 
> In the current OMAP PM framework the PM code resides under
> arch/arm/mach-omap2/. To enable reuse of the register defines move
> the register defines in the emif header file to include/linux so that
> both the EMIF driver and the AM33XX PM code can benefit.
> 
> Signed-off-by: Dave Gerlach 
> Cc: Santosh Shilimkar 
> Cc: Benoit Cousson 
> Cc: Aneesh V 
> ---
>  drivers/memory/emif.h   |  543 +
>  include/linux/ti_emif.h |  558 
> +++
>  2 files changed, 559 insertions(+), 542 deletions(-)
>  create mode 100644 include/linux/ti_emif.h
> 
For file movement or some part of file movement, while formating
patch, use "git format-patch -C" so that only delta change will
appear in the patch.

The patch as such is fine by me.
Acked-by: Santosh Shililmar 

Greg,
Your ack is needed on this patch so that it can go
along with the series. Subsequent patch from this series
use the register defines from this patch.

> diff --git a/drivers/memory/emif.h b/drivers/memory/emif.h
> index bfe08ba..8214f07 100644
> --- a/drivers/memory/emif.h
> +++ b/drivers/memory/emif.h
> @@ -12,548 +12,7 @@
>  #ifndef __EMIF_H
>  #define __EMIF_H
>  
> -/*
> - * Maximum number of different frequencies supported by EMIF driver
> - * Determines the number of entries in the pointer array for register
> - * cache
> - */
> -#define EMIF_MAX_NUM_FREQUENCIES 6
> -
> -/* State of the core voltage */
> -#define DDR_VOLTAGE_STABLE   0
> -#define DDR_VOLTAGE_RAMPING  1
> -
> -/* Defines for timing De-rating */
> -#define EMIF_NORMAL_TIMINGS  0
> -#define EMIF_DERATED_TIMINGS 1
> -
> -/* Length of the forced read idle period in terms of cycles */
> -#define EMIF_READ_IDLE_LEN_VAL   5
> -
> -/*
> - * forced read idle interval to be used when voltage
> - * is changed as part of DVFS/DPS - 1ms
> - */
> -#define READ_IDLE_INTERVAL_DVFS  (1*100)
> -
> -/*
> - * Forced read idle interval to be used when voltage is stable
> - * 50us - or maximum value will do
> - */
> -#define READ_IDLE_INTERVAL_NORMAL(50*100)
> -
> -/* DLL calibration interval when voltage is NOT stable - 1us */
> -#define DLL_CALIB_INTERVAL_DVFS  (1*100)
> -
> -#define DLL_CALIB_ACK_WAIT_VAL   5
> -
> -/* Interval between ZQCS commands - hw team recommended value */
> -#define EMIF_ZQCS_INTERVAL_US(50*1000)
> -/* Enable ZQ Calibration on exiting Self-refresh */
> -#define ZQ_SFEXITEN_ENABLE   1
> -/*
> - * ZQ Calibration simultaneously on both chip-selects:
> - * Needs one calibration resistor per CS
> - */
> -#define  ZQ_DUALCALEN_DISABLE0
> -#define  ZQ_DUALCALEN_ENABLE 1
> -
> -#define T_ZQCS_DEFAULT_NS90
> -#define T_ZQCL_DEFAULT_NS360
> -#define T_ZQINIT_DEFAULT_NS  1000
> -
> -/* DPD_EN */
> -#define DPD_DISABLE  0
> -#define DPD_ENABLE   1
> -
> -/*
> - * Default values for the low-power entry to be used if not provided by user.
> - * OMAP4/5 has a hw bug(i735) due to which this value can not be less than 
> 512
> - * Timeout values are in DDR clock 'cycles' and frequency threshold in Hz
> - */
> -#define EMIF_LP_MODE_TIMEOUT_PERFORMANCE 2048
> -#define EMIF_LP_MODE_TIMEOUT_POWER   512
> -#define EMIF_LP_MODE_FREQ_THRESHOLD  4
> -
> -/* DDR_PHY_CTRL_1 values for EMIF4D - ATTILA PHY combination */
> -#define EMIF_DDR_PHY_CTRL_1_BASE_VAL_ATTILAPHY   0x049FF000
> -#define EMIF_DLL_SLAVE_DLY_CTRL_400_MHZ_ATTILAPHY0x41
> -#define EMIF_DLL_SLAVE_DLY_CTRL_200_MHZ_ATTILAPHY0x80
> -#define EMIF_DLL_SLAVE_DLY_CTRL_100_MHZ_AND_LESS_ATTILAPHY 0xFF
> -
> -/* DDR_PHY_CTRL_1 values for EMIF4D5 INTELLIPHY combination */
> -#define EMIF_DDR_PHY_CTRL_1_BASE_VAL_INTELLIPHY  0x0E084200
> -#define EMIF_PHY_TOTAL_READ_LATENCY_INTELLIPHY_PS1
> -
> -/* TEMP_ALERT_CONFIG - corresponding to temp gradient 5 C/s */
> -#define TEMP_ALERT_POLL_INTERVAL_DEFAULT_MS  360
> -
> -#define EMIF_T_CSTA  3
> -#define EMIF_T_PDLL_UL   128
> -
> -/* External PHY control re

Re: [PATCHv3 0/9] ARM: OMAP2+: AM33XX: Add suspend-resume support

On Tuesday 06 August 2013 01:49 PM, Dave Gerlach wrote:
> Hi,
> 
> This is the third version of the patch series for adding basic suspend-resume
> support for AM33XX, previously submitted by Vaibhav Bedia. This patchset 
> is based on 3.11-rc4 and depends on a forthcoming patchset from Suman Anna
> that adds mailbox support for the wkup_m3. The patches at [1], [2], and [3] 
> are 
> required for the pm code to properly suspend and resume.
> 
> The PM code uses the firmware interface and expects the userspace to load 
> the WKUP_M3 binary before the suspend-resume functionality is made available.
> The binary file (and the source-code for WKUP_M3) can be obtained from the 
> 'next2' branch at [4]. The WKUP_M3 binary can either be loaded post bootup 
> via the sysfs entry './sys/devices/ocp.2/wkup_m3.4/firmware' or it can be 
> included in the kernel image as part of the build process.
> 
> Suspend to mem is tested on am335x-bone and am335x-evm.
> 
> More details on AM335x suspend-resume are provided within the commit logs
> for each patch.
> 
> Changes in v3:
> - Moved wkup_m3 code into separate driver
> - Split up ti_emif header move
> - Addressed clean-up comments
> - Removed mailbox patches
> - v2-v3 Discussion:
> http://marc.info/?l=linux-arm-kernel&m=135698501821090&w=4
> 
Please CC people who have at least reviewed the last version of
the series. Couple of patches I was CC'ed o.w I would missed this series.

Regards,
Santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 8/9] ARM: OMAP2+: AM33XX: Basic suspend resume support

2013-08-08 Thread Nishanth Menon


On 08/08/2013 03:45 AM, Russ Dill wrote:

  In reference to
the M3 handling it, the M3 wouldn't know which devices have a driver
bound and which don't.
Does it need to? M3 firmware can pretty much define "I will force the 
device into low power state, and if the drivers dont handle things 
properly, fix the darned driver". M3 behavior should be considered as a 
"hardware" as far as Linux running on MPU is concerned, and firmware 
helps change the behavior by accounting for SoC quirks. *if* we have 
ability to handle this in the firmware, there is no need to carry this 
in Linux.


--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" 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 02/12] ARM: edma: Don't clear EMR of channel in edma_stop

2013-08-08 Thread Sekhar Nori

On Monday 05 August 2013 09:44 PM, Joel Fernandes wrote:
> We certainly don't want error conditions to be cleared any other
> place but the EDMA error handler, as this will make us 'forget'
> about missed events we might need to know errors have occurred.
> 
> This fixes a race condition where the EMR was being cleared
> by the transfer completion interrupt handler.
> 
> Basically, what was happening was:
> 
> Missed event
>  |
>  |
>  V
> SG1-SG2-SG3-Null
>  \
>   \__TC Interrupt (Almost same time as ARM is executing
> TC interrupt handler, an event got missed and also forgotten
> by clearing the EMR).
> 
> This causes the following  problems:
> 
> 1.
> If error interrupt is also pending and TC interrupt clears the EMR
> by calling edma_stop as has been observed in the edma_callback function,
> the ARM will execute the error interrupt even though the EMR is clear.
> As a result, the  dma_ccerr_handler returns IRQ_NONE. If this happens
> enough number of times, IRQ subsystem disables the interrupt thinking
> its spurious which makes error handler never execute again.
> 
> 2.
> Also even if error handler doesn't return IRQ_NONE, the removing of EMR
> removes the knowledge about which channel had a missed event, and thus
> a manual trigger on such channels cannot be performed.
> 
> The EMR is ultimately being cleared by the Error interrupt handler
> once it is handled so we remove code that does it in edma_stop and
> allow it to happen there.
> 
> Signed-off-by: Joel Fernandes 

Queuing this for v3.11 fixes. While committing, I changed the headline
to remove capitalization and made it more readable by removing register
level details. The new headline is:

ARM: edma: don't clear missed events in edma_stop()

Thanks,
Sekhar

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: v3.11-rc4: OMAP1/Amstrad Delta (E3) crash

2013-08-08 Thread Russell King - ARM Linux

On Wed, Aug 07, 2013 at 02:26:09AM +0300, Aaro Koskinen wrote:
> [0.258589] [1224] *pgd=, *pte=11fff0cb01f1, 
> *ppte=11fff00a

BTW, your oops dump is interesting for another reason - the above.
You seem to have 64-bit page table entries above.  This is produced
by this:

pte_t *pte;
...
printk(", *pte=%08llx", (long long)pte_val(*pte));
#ifndef CONFIG_ARM_LPAE
printk(", *ppte=%08llx",
   (long long)pte_val(pte[PTE_HWTABLE_PTRS]));
#endif

with:

#define pte_val(x)  (x)
typedef pteval_t pte_t;
typedef u32 pteval_t;

Now, this is totally legal C:

unsigned int val = 0x12345678;

printk("%08llx\n", (long long)val);

and it should produce "12345678" but I'm willing to bet with your compiler
it produces "12345678" where  is just what happens to be
sitting in some register.  IOW, I think you have a compiler bug here.  Can
you investigate what's going on with this yourself please?
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: v3.11-rc4: OMAP1/Amstrad Delta (E3) crash

2013-08-08 Thread Russell King - ARM Linux

On Thu, Aug 08, 2013 at 03:05:03AM +0300, Aaro Koskinen wrote:
> That seems to work - the board boots fine to shell as before. Feel free
> to add Reported-by: or Tested-by: from me to the patch.

Thanks, done.  Should be in linux-next by tomorrow.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: OMAP2430 SDP boot broken after Linus' rmk merge

2013-08-08 Thread Rajendra Nayak

On Thursday 08 August 2013 11:07 AM, Rajendra Nayak wrote:
> On Wednesday 07 August 2013 11:39 PM, Paul Walmsley wrote:
>> Hi Rajendra,
>>
>> On Tue, 23 Jul 2013, Rajendra Nayak wrote:
>>
>>> So I tried commit 'fb2af00' on the 4430SDP and it did boot fine, though I 
>>> see
>>> the below errors. (I am using the mainline bootloaders which do not lock any
>>> additional DPLLs like USB)
>>
>> Could you please send patches to fix these problems, or ensure that 
>> someone else from TI fixes them?  Let's see if we can deal with the 
>> remaining bootloader dependencies here.
> 
> Sure Paul, I'll take a look at this.

+Mike,

Paul, I just posted a fix for this [1] though I am not sure if doing the
PLL locks in a certain sequence is the right thing to do, or perhaps there
is a need to relook at the need for a clk_set_rate() on all downstream clocks
as part of CCF.

regards,
Rajendra

[1] http://www.mail-archive.com/linux-omap@vger.kernel.org/msg93627.html

> 
>>
>> thanks
>>
>>
>> - Paul
>>
>>>
>>> [0.00] clock: dpll_usb_ck failed transition to 'locked'
>>> [0.00] Division by zero in kernel.
>>> [0.00] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 
>>> 3.10.0-03445-gfb2af00-dirty #7
>>> [0.00] [] (unwind_backtrace+0x0/0xf4) from [] 
>>> (show_stack+0x10/0x14)
>>> [0.00] [] (show_stack+0x10/0x14) from [] 
>>> (Ldiv0+0x8/0x10)
>>> [0.00] [] (Ldiv0+0x8/0x10) from [] 
>>> (clk_divider_set_rate+0x10/0x114)
>>> [0.00] [] (clk_divider_set_rate+0x10/0x114) from 
>>> [] (clk_change_rate+0x38/0xb8)
>>> [0.00] [] (clk_change_rate+0x38/0xb8) from [] 
>>> (clk_change_rate+0xa0/0xb8)
>>> [0.00] Division by zero in kernel.
>>> [0.00] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 
>>> 3.10.0-03445-gfb2af00-dirty #7
>>> [0.00] [] (unwind_backtrace+0x0/0xf4) from [] 
>>> (show_stack+0x10/0x14)
>>> [0.00] [] (show_stack+0x10/0x14) from [] 
>>> (Ldiv0+0x8/0x10)
>>> [0.00] [] (Ldiv0+0x8/0x10) from [] 
>>> (clk_divider_set_rate+0x10/0x114)
>>> [0.00] [] (clk_divider_set_rate+0x10/0x114) from 
>>> [] (clk_change_rate+0x38/0xb8)
>>> [0.00] [] (clk_change_rate+0x38/0xb8) from [] 
>>> (clk_change_rate+0xa0/0xb8)
>>> [0.00] Division by zero in kernel.
>>> [0.00] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 
>>> 3.10.0-03445-gfb2af00-dirty #7
>>> [0.00] [] (unwind_backtrace+0x0/0xf4) from [] 
>>> (show_stack+0x10/0x14)
>>> [0.00] [] (show_stack+0x10/0x14) from [] 
>>> (Ldiv0+0x8/0x10)
>>> [0.00] [] (Ldiv0+0x8/0x10) from [] 
>>> (clk_divider_set_rate+0x10/0x114)
>>> [0.00] [] (clk_divider_set_rate+0x10/0x114) from 
>>> [] (clk_change_rate+0x38/0xb8)
>>> [0.00] [] (clk_change_rate+0x38/0xb8) from [] 
>>> (clk_change_rate+0xa0/0xb8)
>>> [0.00] Division by zero in kernel.
>>> [0.00] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 
>>> 3.10.0-03445-gfb2af00-dirty #7
>>> [0.00] [] (unwind_backtrace+0x0/0xf4) from [] 
>>> (show_stack+0x10/0x14)
>>> [0.00] [] (show_stack+0x10/0x14) from [] 
>>> (Ldiv0+0x8/0x10)
>>> [0.00] [] (Ldiv0+0x8/0x10) from [] 
>>> (clk_divider_set_rate+0x10/0x114)
>>> [0.00] [] (clk_divider_set_rate+0x10/0x114) from 
>>> [] (clk_change_rate+0x38/0xb8)
>>> [0.00] [] (clk_change_rate+0x38/0xb8) from [] 
>>> (clk_change_rate+0xa0/0xb8)
>>> [0.00] Division by zero in kernel.
>>> [0.00] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 
>>> 3.10.0-03445-gfb2af00-dirty #7
>>> [0.00] [] (unwind_backtrace+0x0/0xf4) from [] 
>>> (show_stack+0x10/0x14)
>>> [0.00] [] (show_stack+0x10/0x14) from [] 
>>> (Ldiv0+0x8/0x10)
>>> [0.00] [] (Ldiv0+0x8/0x10) from [] 
>>> (clk_divider_set_rate+0x10/0x114)
>>> [0.00] [] (clk_divider_set_rate+0x10/0x114) from 
>>> [] (clk_change_rate+0x38/0xb8)
>>> [0.00] [] (clk_change_rate+0x38/0xb8) from [] 
>>> (clk_change_rate+0xa0/0xb8)
>>> [0.00] Division by zero in kernel.
>>> [0.00] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 
>>> 3.10.0-03445-gfb2af00-dirty #7
>>> [0.00] [] (unwind_backtrace+0x0/0xf4) from [] 
>>> (show_stack+0x10/0x14)
>>> [0.00] [] (show_stack+0x10/0x14) from [] 
>>> (Ldiv0+0x8/0x10)
>>> [0.00] [] (Ldiv0+0x8/0x10) from [] 
>>> (clk_divider_set_rate+0x10/0x114)
>>> [0.00] [] (clk_divider_set_rate+0x10/0x114) from 
>>> [] (clk_change_rate+0x38/0xb8)
>>> [0.00] [] (clk_change_rate+0x38/0xb8) from [] 
>>> (clk_change_rate+0xa0/0xb8)
>>> [0.00] clock: trace_clk_div_ck: could not find divisor for target 
>>> rate 0 for parent pmd_trace_clk_mux_ck
>>> [0.00] Division by zero in kernel.
>>> [0.00] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 
>>> 3.10.0-03445-gfb2af00-dirty #7
>>> [0.00] [] (unwind_backtrace+0x0/0xf4) from [] 
>>> (show_stack+0x10/0x14)
>>> [0.00] [] (show_stack+0x10/0x14) from [] 
>>> (Ldiv0+0x8/0x10)
>>> [0.00] [] (Ldiv

[PATCH] ARM: OMAP4: clock: Lock PLLs in the right sequence

2013-08-08 Thread Rajendra Nayak

On OMAP4 we have clk_set_rate()s being done for a few
DPLL clock nodes, as part of the clock init code, since
the bootloaders no longer locks these DPLLs.

So we have a clk_set_rate() done for a ABE DPLL node (which
inturn locks it) followed by a clk_set_rate() for the USB DPLL.

With USB DPLL being in bypass, we have this parent->child
relationship thats formed while the clocks get registered.

dpll_abe_ck
|
V
dpll_abe_x2_ck
|
V
dpll_abe_m3x2_ck
|
V
usb_hs_clk_div_ck
|
V
dpll_usb_ck

This is because usb_hs_clk_div_ck is bypass clock for dpll_usb_ck.

So with this parent->child relationship in place, a clk_set_rate()
on ABE DPLL results eventually in a clk_set_rate() call on USB DPLL,
because CCF does a clk_change_rate() (as part of clk_set_rate()) on
all downstream clocks resulting from a rate change on the top clock.

So its important that we lock USB DPLL before we lock ABE DPLL.
Without which we see these error logs at boot.
[These error logs will not be seen if using a bootloader that locks
USB DPLL]

[0.00] clock: dpll_usb_ck failed transition to 'locked'
[0.00] Division by zero in kernel.
[0.00] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 
3.10.0-03445-gfb2af00-dirty #7
[0.00] [] (unwind_backtrace+0x0/0xf4) from [] 
(show_stack+0x10/0x14)
[0.00] [] (show_stack+0x10/0x14) from [] 
(Ldiv0+0x8/0x10)
[0.00] [] (Ldiv0+0x8/0x10) from [] 
(clk_divider_set_rate+0x10/0x114)
[0.00] [] (clk_divider_set_rate+0x10/0x114) from [] 
(clk_change_rate+0x38/0xb8)
[0.00] [] (clk_change_rate+0x38/0xb8) from [] 
(clk_change_rate+0xa0/0xb8)
[0.00] Division by zero in kernel.
[0.00] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 
3.10.0-03445-gfb2af00-dirty #7
[0.00] [] (unwind_backtrace+0x0/0xf4) from [] 
(show_stack+0x10/0x14)
[0.00] [] (show_stack+0x10/0x14) from [] 
(Ldiv0+0x8/0x10)
[0.00] [] (Ldiv0+0x8/0x10) from [] 
(clk_divider_set_rate+0x10/0x114)
[0.00] [] (clk_divider_set_rate+0x10/0x114) from [] 
(clk_change_rate+0x38/0xb8)
[0.00] [] (clk_change_rate+0x38/0xb8) from [] 
(clk_change_rate+0xa0/0xb8)
[0.00] Division by zero in kernel.
[0.00] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 
3.10.0-03445-gfb2af00-dirty #7
[0.00] [] (unwind_backtrace+0x0/0xf4) from [] 
(show_stack+0x10/0x14)
[0.00] [] (show_stack+0x10/0x14) from [] 
(Ldiv0+0x8/0x10)
[0.00] [] (Ldiv0+0x8/0x10) from [] 
(clk_divider_set_rate+0x10/0x114)
[0.00] [] (clk_divider_set_rate+0x10/0x114) from [] 
(clk_change_rate+0x38/0xb8)
[0.00] [] (clk_change_rate+0x38/0xb8) from [] 
(clk_change_rate+0xa0/0xb8)
[0.00] Division by zero in kernel.
[0.00] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 
3.10.0-03445-gfb2af00-dirty #7
[0.00] [] (unwind_backtrace+0x0/0xf4) from [] 
(show_stack+0x10/0x14)
[0.00] [] (show_stack+0x10/0x14) from [] 
(Ldiv0+0x8/0x10)
[0.00] [] (Ldiv0+0x8/0x10) from [] 
(clk_divider_set_rate+0x10/0x114)
[0.00] [] (clk_divider_set_rate+0x10/0x114) from [] 
(clk_change_rate+0x38/0xb8)
[0.00] [] (clk_change_rate+0x38/0xb8) from [] 
(clk_change_rate+0xa0/0xb8)
[0.00] Division by zero in kernel.
[0.00] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 
3.10.0-03445-gfb2af00-dirty #7
[0.00] [] (unwind_backtrace+0x0/0xf4) from [] 
(show_stack+0x10/0x14)
[0.00] [] (show_stack+0x10/0x14) from [] 
(Ldiv0+0x8/0x10)
[0.00] [] (Ldiv0+0x8/0x10) from [] 
(clk_divider_set_rate+0x10/0x114)
[0.00] [] (clk_divider_set_rate+0x10/0x114) from [] 
(clk_change_rate+0x38/0xb8)
[0.00] [] (clk_change_rate+0x38/0xb8) from [] 
(clk_change_rate+0xa0/0xb8)
[0.00] Division by zero in kernel.
[0.00] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 
3.10.0-03445-gfb2af00-dirty #7
[0.00] [] (unwind_backtrace+0x0/0xf4) from [] 
(show_stack+0x10/0x14)
[0.00] [] (show_stack+0x10/0x14) from [] 
(Ldiv0+0x8/0x10)
[0.00] [] (Ldiv0+0x8/0x10) from [] 
(clk_divider_set_rate+0x10/0x114)
[0.00] [] (clk_divider_set_rate+0x10/0x114) from [] 
(clk_change_rate+0x38/0xb8)
[0.00] [] (clk_change_rate+0x38/0xb8) from [] 
(clk_change_rate+0xa0/0xb8)
[0.00] clock: trace_clk_div_ck: could not find divisor for target rate 
0 for parent pmd_trace_clk_mux_ck
[0.00] Division by zero in kernel.

Signed-off-by: Rajendra Nayak 
---
 arch/arm/mach-omap2/cclock44xx_data.c |   20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-omap2/cclock44xx_data.c 
b/arch/arm/mach-omap2/cclock44xx_data.c
index 88e37a4..1d5b529 100644
--- a/arch/arm/mach-omap2/cclock44xx_data.c
+++ b/arch/arm/mach-omap2/cclock44xx_data.c
@@ -1707,6 +1707,18 @@ int __init omap4xxx_clk_init(void)
omap2_clk_disable_autoidle_all();
 
/*
+* A set rate of ABE DPLL inturn triggers a set rate of USB DPLL
+* w

Re: [PATCH 4/5] omap: Avoid crashes in the case of hwmod misconfiguration

2013-08-08 Thread Pantelis Antoniou

Hi Kevin,

On Aug 8, 2013, at 12:15 AM, Kevin Hilman wrote:

> Pantelis Antoniou  writes:
> 
>> omap hwmod is really sensitive to hwmod misconfiguration.
>> Getting a minor clock wrong always ended up in a crash.
>> Attempt to be more resilient by not assigning variables with
>> error codes and then attempting to use them.
>> 
>> Without this patch, missing a clock ends up with something like this:
>> omap_hwmod: ehrpwm0: cannot clk_get opt_clk ehrpwm0_tbclk!
> 
> Definitely agree we should not be crashing when given bad data.
> 
> nit Re: "missing clock".  I don't think there will be any crash if a
> clock is missing.  This looks to me more like the clock name is wrong
> (tbclk instead of dbclk?), not missing.
> 

Yes, I'll rephrase.

> [...]
> 
>> index 7341eff..42cb7d4 100644
>> --- a/arch/arm/mach-omap2/omap_hwmod.c
>> +++ b/arch/arm/mach-omap2/omap_hwmod.c
>> @@ -784,7 +784,9 @@ static int _init_interface_clks(struct omap_hwmod *oh)
>>  if (IS_ERR(c)) {
>>  pr_warning("omap_hwmod: %s: cannot clk_get 
>> interface_clk %s\n",
>> oh->name, os->clk);
>> -ret = -EINVAL;
>> +if (ret == 0)
>> +ret = -EINVAL;
>> +continue;
> 
> the 'if (ret == 0)' adds confusion IMO.  If we don't care additional
> failures, errors, then just add a 'break' instead of these 3 lines.
> 
> [...]
> 


I tried to carry on as much as possible even on the presence of errors.
The remaining clocks won't be initialized, but that might be OK.

> Kevin

Regards

-- Pantelis

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] omap: Properly handle resources for omap_devices

2013-08-08 Thread Pantelis Antoniou

Hi Kevin,

On Aug 7, 2013, at 9:45 PM, Kevin Hilman wrote:

> [fixing address for Benoit]
> 
> Pantelis Antoniou  writes:
> 
>> omap_device relies on the platform notifier callbacks managing resources
>> behind the scenes. The resources were not properly linked causing crashes
>> when removing the device.
>> 
>> Rework the resource modification code so that linking is performed properly,
>> and make sure that no resources that have no parent (which can happen for DMA
>> & IRQ resources) are ever left for cleanup by the core resource layer.
>> 
>> Signed-off-by: Pantelis Antoniou 
> 
> This one failed my "took more than 15 minutes to understand" test.  The
> changelog is rather vague (especially about what "properly" means), and
> the combination of moving code and changing it makes the patch rather
> clunky to read, so I remain a bit confused about what the actual problem
> is.  Please elaborate.
> 
> Also, could you share a crash dump as well as details about how to
> reproduce this problem?
> 
> Thanks,
> 
> Kevin

It's the full patchset that fixes the problem:

Let me illustrate:

The kernel I use is located at:

g...@github.com:pantoniou/linux-beagle-track-mainline.git
branch: merge-20130806 (there are topic branches for other stuff too)

The DT overlay we're loading:

> /*
>  * Copyright (C) 2013 CircuitCo
>  *
>  * Virtual cape for UART1 on connector pins P9.24 P9.26
>  *
>  * This program is free software; you can redistribute it and/or modify
>  * it under the terms of the GNU General Public License version 2 as
>  * published by the Free Software Foundation.
>  */
> /dts-v1/;
> /plugin/;
> 
> / {
>   compatible = "ti,beaglebone", "ti,beaglebone-black";
> 
>   /* identification */
>   part-number = "BB-UART1";
>   version = "00A0";
> 
>   /* state the resources this cape uses */
>   exclusive-use =
>   /* the pin header uses */
>   "P9.24",/* uart1_txd */
>   "P9.26",/* uart1_rxd */
>   /* the hardware ip uses */
>   "uart1";
> 
>   fragment@0 {
>   target = <&am33xx_pinmux>;
>   __overlay__ {
>   bb_uart1_pins: pinmux_bb_uart1_pins {
>   pinctrl-single,pins = <
>   0x184 0x20 /* P9.24 uart1_txd.uart1_txd 
>  OUTPUT  */
>   0x180 0x20 /* P9.26 uart1_rxd.uart1_rxd 
>  INPUT  */
>   >;
>   };
>   };
>   };
> 
>   fragment@1 {
>   target = <&uart1>;  /* really uart1 */
>   __overlay__ {
>   status = "okay";
>   pinctrl-names = "default";
>   pinctrl-0 = <&bb_uart1_pins>;
>   };
>   };
> };
> 

Nothing complicated; just a serial device.

With the full patchset on a beaglebone and loading a simple case of a UART 
'cape'.

> root@beaglebone:/sys/devices/bone_capemgr.4# echo BB-UART1 >slots 
> [   58.546947] bone-capemgr bone_capemgr.4: part_number 'BB-UART1', version 
> 'N/A'
> [   58.578374] bone-capemgr bone_capemgr.4: slot #4: generic override
> [   58.584925] bone-capemgr bone_capemgr.4: bone: Using override eeprom data 
> at slot 4
> [   58.593086] bone-capemgr bone_capemgr.4: slot #4: 'Override Board 
> Name,00A0,Override Manuf,BB-UART1'
> [   58.618455] bone-capemgr bone_capemgr.4: slot #4: Requesting part 
> number/version based 'BB-UART1-00A0.dtbo
> [   58.638258] bone-capemgr bone_capemgr.4: slot #4: Requesting firmware 
> 'BB-UART1-00A0.dtbo' for board-name 'Override Board Name', version '00A0'
> [   58.681259] bone-capemgr bone_capemgr.4: slot #4: dtbo 
> 'BB-UART1-00A0.dtbo' loaded; converting to live tree
> [   58.705075] bone-capemgr bone_capemgr.4: slot #4: #2 overlays
> [   58.735058] 48022000.serial: ttyO1 at MMIO 0x48022000 (irq = 89) is a OMAP 
> UART1
> [   58.757834] bone-capemgr bone_capemgr.4: slot #4: Applied #2 overlays.
> root@beaglebone:/sys/devices/bone_capemgr.4# echo -4 >slots 
> [   62.362519] bone-capemgr bone_capemgr.4: Removed slot #4
> 

Revert "omap: Properly handle resources for omap_devices"
Revert "of: Link platform device resources properly."
Revert "pdev: Fix platform device resource linking"

> root@beaglebone:/sys/devices/bone_capemgr.4# echo BB-UART1 >slots 
> [   39.317978] bone-capemgr bone_capemgr.4: part_number 'BB-UART1', version 
> 'N/A'
> [   39.325630] bone-capemgr bone_capemgr.4: slot #4: generic override
> [   39.332248] bone-capemgr bone_capemgr.4: bone: Using override eeprom data 
> at slot 4
> [   39.340336] bone-capemgr bone_capemgr.4: slot #4: 'Override Board 
> Name,00A0,Override Manuf,BB-UART1'
> [   39.378854] bone-capemgr bone_capemgr.4: slot #4: Requesting part 
> number/version based 'BB-UART1-00A0.dtbo
> [   39.396013] bone-capemgr bone_capemgr.4: slot #4: Requesting firmware 
> 'BB-UART1-00A0.dtbo' for board-name 'Override Bo

Re: [RFC 1/2] usb: phy: Add Qualcomm SS-USB and HS-USB drivers for DWC3 core

2013-08-08 Thread Mark Rutland

On Tue, Aug 06, 2013 at 03:36:33PM +0100, Ivan T. Ivanov wrote:
> Hi,
> 
> On Tue, 2013-08-06 at 15:03 +0100, Mark Rutland wrote:
> > On Tue, Aug 06, 2013 at 12:53:10PM +0100, Ivan T. Ivanov wrote:
> > > From: "Ivan T. Ivanov" 
> > >
> > > Signed-off-by: Ivan T. Ivanov 
> > > ---
> > >  .../devicetree/bindings/usb/msm-ssusb.txt  |   49 +++
> > >  drivers/usb/phy/Kconfig|   11 +
> > >  drivers/usb/phy/Makefile   |2 +
> > >  drivers/usb/phy/phy-msm-dwc3-usb2.c|  342 
> > > +
> > >  drivers/usb/phy/phy-msm-dwc3-usb3.c|  389 
> > > 
> > >  5 files changed, 793 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/usb/msm-ssusb.txt
> > >  create mode 100644 drivers/usb/phy/phy-msm-dwc3-usb2.c
> > >  create mode 100644 drivers/usb/phy/phy-msm-dwc3-usb3.c
> > >
> > > diff --git a/Documentation/devicetree/bindings/usb/msm-ssusb.txt 
> > > b/Documentation/devicetree/bindings/usb/msm-ssusb.txt
> > > new file mode 100644
> > > index 000..550b496
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/usb/msm-ssusb.txt
> > > @@ -0,0 +1,49 @@
> > > +MSM SuperSpeed USB3.0 SoC controllers
> > > +
> > > +Required properities :
> > > +- compatible sould be "qcom,dwc3-usb2";
> > > +- reg : offset and length of the register set in the memory map
> > > +- clocks: <&cxo>, <&usb2a_phy_sleep_cxc>;
> > 
> > Huh? That doesn't describe what these are. These would be better
> > explained with a reference to clock-names and a basic description as to
> > what the input's called, what it drives, etc, as you've done done for
> > the *-supply properties.
> 
> Ok, I will fix this.
> 
> > 
> > > +- clock-names: "xo", "sleep_a_clk";
> > > +-supply: phandle to the regulator device tree node
> > > +Required "supply-name" examples are:
> > > +   "v1p8" : 1.8v supply for HSPHY
> > > +   "v3p3" : 3.3v supply for HSPHY
> > > +   "vbus" : vbus supply for host mode
> > > +   "vddcx" : vdd supply for HS-PHY digital circuit operation
> > > +
> > > +Required properities :
> > > +- compatible sould be "qcom,dwc3-usb3";
> > > +- reg : offset and length of the register set in the memory map
> > > +- clocks: <&cxo>, <&usb30_mock_utmi_cxc>;
> > 
> > Similarly, this doesn't describe what the clocks are.
> 
> Understood.
> 
> > 
> > > +- clock-names: "xo", "ref_clk";
> > > +-supply: phandle to the regulator device tree node
> > > +Required "supply-name" examples are:
> > > +   "v1p8" : 1.8v supply for SS-PHY
> > > +   "vddcx" : vdd supply for SS-PHY digital circuit operation
> > > +
> > > +Example device nodes:
> > > +
> > > +   dwc3_usb2: phy@f92f8800 {
> > > +   compatible = "qcom,dwc3-usb2";
> > > +   reg = <0xf92f8800 0x30>;
> > > +
> > > +   clocks = <&cxo>, <&usb2a_phy_sleep_cxc>;
> > > +   clock-names = "xo", "sleep_a_clk";
> > > +
> > > +   vbus-supply = <&supply>;
> > > +   vddcx-supply = <&supply>;
> > > +   v1p8-supply = <&supply>;
> > > +   v3p3-supply = <&supply>;
> > > +   };
> > > +
> > > +   dwc3_usb3: phy@f92f8830 {
> > > +   compatible = "qcom,dwc3-usb3";
> > > +   reg = <0xf92f8830 0x30>;
> > > +
> > > +   clocks = <&cxo>, <&usb30_mock_utmi_cxc>;
> > > +   clock-names = "xo", "ref_clk";
> > > +
> > > +   vddcx-supply = <&supply>;
> > > +   v1p8-supply = <&supply>;
> > > +   };
> > 
> > 
> > Those regster banks look suspiciously close. Are these the same IP
> > block? Can they ever appear separately?
> > 
> 
> They are part of the wrapper Qualcomm logic around Synopsys USB3 core.
> In this sense they are part of the one IP, I believe. Manage them from
> separate drivers simplify code.

Hmmm. I'm not entirely certain on this. On the one hand, they're
separate IP blocks, and have lgoically separate drivers, so describing
them as two devices makes sense. On the other hand, they've been fused
into one IP block with shared resources. Describing them as two devices
probably makes sense given you have the wrapper driver.

> 
> > Do the drivers not trample each other when messing with shared clocks
> > and regulators?
> > 
> 
> Regulators and clocks have reference counting, right?, so this should
> be safe. Even if they are part of the one driver, clocks and regulators
> could be switched off only if both PHY's do not use them.

Ok, I just wanted to be sure this had been considered :)

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 9/9] ARM: OMAP2+: AM33XX: Hookup AM33XX PM code into OMAP builds

On Tue, Aug 6, 2013 at 10:49 AM, Dave Gerlach  wrote:
> From: Vaibhav Bedia 
>
> With all the requisite changes in place we can now
> enable the basic PM support for AM335x. This patch
> updates the various OMAP files to enable suspend-resume
> on AM335x.
>
> Signed-off-by: Vaibhav Bedia 
> Signed-off-by: Dave Gerlach 

Reviewed-by: Russ Dill 

> ---
>  arch/arm/mach-omap2/Kconfig |7 +--
>  arch/arm/mach-omap2/Makefile|2 ++
>  arch/arm/mach-omap2/board-generic.c |1 +
>  arch/arm/mach-omap2/common.h|   10 ++
>  arch/arm/mach-omap2/io.c|5 +
>  arch/arm/mach-omap2/pm.c|3 ++-
>  arch/arm/mach-omap2/pm.h|5 +
>  arch/arm/mach-omap2/sram.c  |   10 +-
>  arch/arm/mach-omap2/sram.h  |2 ++
>  9 files changed, 41 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
> index 3eed000..ef3fe40 100644
> --- a/arch/arm/mach-omap2/Kconfig
> +++ b/arch/arm/mach-omap2/Kconfig
> @@ -67,11 +67,14 @@ config SOC_OMAP5
>  config SOC_AM33XX
> bool "AM33XX support"
> depends on ARCH_MULTI_V7
> -   select ARCH_OMAP2PLUS
> +   default y
> select ARM_CPU_SUSPEND if PM
> +   select COMMON_CLK
> select CPU_V7
> +   select MAILBOX if PM
> select MULTI_IRQ_HANDLER
> -   select COMMON_CLK
> +   select OMAP_MBOX_FWK if PM
> +   select OMAP2PLUS_MBOX if PM
>
>  config SOC_AM43XX
> bool "TI AM43x"
> diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
> index d4f6715..eb3a47a 100644
> --- a/arch/arm/mach-omap2/Makefile
> +++ b/arch/arm/mach-omap2/Makefile
> @@ -87,6 +87,7 @@ obj-$(CONFIG_ARCH_OMAP2)  += sleep24xx.o
>  obj-$(CONFIG_ARCH_OMAP3)   += pm34xx.o sleep34xx.o
>  obj-$(CONFIG_ARCH_OMAP4)   += pm44xx.o omap-mpuss-lowpower.o
>  obj-$(CONFIG_SOC_OMAP5)+= omap-mpuss-lowpower.o
> +obj-$(CONFIG_SOC_AM33XX)   += pm33xx.o sleep33xx.o wkup_m3.o
>  obj-$(CONFIG_PM_DEBUG) += pm-debug.o
>
>  obj-$(CONFIG_POWER_AVS_OMAP)   += sr_device.o
> @@ -94,6 +95,7 @@ obj-$(CONFIG_POWER_AVS_OMAP_CLASS3)+= 
> smartreflex-class3.o
>
>  AFLAGS_sleep24xx.o :=-Wa,-march=armv6
>  AFLAGS_sleep34xx.o :=-Wa,-march=armv7-a$(plus_sec)
> +AFLAGS_sleep33xx.o :=-Wa,-march=armv7-a$(plus_sec)
>
>  endif
>
> diff --git a/arch/arm/mach-omap2/board-generic.c 
> b/arch/arm/mach-omap2/board-generic.c
> index aed750c..3f2d6a7 100644
> --- a/arch/arm/mach-omap2/board-generic.c
> +++ b/arch/arm/mach-omap2/board-generic.c
> @@ -159,6 +159,7 @@ DT_MACHINE_START(AM33XX_DT, "Generic AM33XX (Flattened 
> Device Tree)")
> .reserve= am33xx_reserve,
> .map_io = am33xx_map_io,
> .init_early = am33xx_init_early,
> +   .init_late  = am33xx_init_late,
> .init_irq   = omap_intc_of_init,
> .handle_irq = omap3_intc_handle_irq,
> .init_machine   = omap_generic_init,
> diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h
> index 6b8ef74..80bf0da 100644
> --- a/arch/arm/mach-omap2/common.h
> +++ b/arch/arm/mach-omap2/common.h
> @@ -69,6 +69,15 @@ static inline int omap4_pm_init(void)
>  }
>  #endif
>
> +#if defined(CONFIG_PM) && defined(CONFIG_SOC_AM33XX)
> +int am33xx_pm_init(void);
> +#else
> +static inline int am33xx_pm_init(void)
> +{
> +   return 0;
> +}
> +#endif
> +
>  #ifdef CONFIG_OMAP_MUX
>  int omap_mux_late_init(void);
>  #else
> @@ -107,6 +116,7 @@ void omap2430_init_late(void);
>  void omap3430_init_late(void);
>  void omap35xx_init_late(void);
>  void omap3630_init_late(void);
> +void am33xx_init_late(void);
>  void am35xx_init_late(void);
>  void ti81xx_init_late(void);
>  int omap2_common_pm_late_init(void);
> diff --git a/arch/arm/mach-omap2/io.c b/arch/arm/mach-omap2/io.c
> index 3ad7543..63516e5 100644
> --- a/arch/arm/mach-omap2/io.c
> +++ b/arch/arm/mach-omap2/io.c
> @@ -584,6 +584,11 @@ void __init am33xx_init_early(void)
> omap_hwmod_init_postsetup();
> omap_clk_init = am33xx_clk_init;
>  }
> +
> +void __init am33xx_init_late(void)
> +{
> +   am33xx_pm_init();
> +}
>  #endif
>
>  #ifdef CONFIG_SOC_AM43XX
> diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
> index e742118..f8bd883 100644
> --- a/arch/arm/mach-omap2/pm.c
> +++ b/arch/arm/mach-omap2/pm.c
> @@ -305,7 +305,8 @@ int __init omap2_common_pm_late_init(void)
> }
>
>  #ifdef CONFIG_SUSPEND
> -   suspend_set_ops(&omap_pm_ops);
> +   if (!soc_is_am33xx())
> +   suspend_set_ops(&omap_pm_ops);
>  #endif
>
> return 0;
> diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
> index 7bdd22a..5122bb6 100644
> --- a/arch/arm/mach-omap2/pm.h
> +++ b/arch/arm/mach-omap2/pm.h
> @@ -82,6 +82,11 @@ extern unsigned in

Re: [PATCHv3 8/9] ARM: OMAP2+: AM33XX: Basic suspend resume support

On Tue, Aug 6, 2013 at 10:49 AM, Dave Gerlach  wrote:
> From: Vaibhav Bedia 
>
> AM335x supports various low power modes as documented
> in section 8.1.4.3 of the AM335x TRM which is available
> @ http://www.ti.com/litv/pdf/spruh73f
>
> DeepSleep0 mode offers the lowest power mode with limited
> wakeup sources without a system reboot and is mapped as
> the suspend state in the kernel. In this state, MPU and
> PER domains are turned off with the internal RAM held in
> retention to facilitate resume process. As part of the boot
> process, the assembly code is copied over to OCMCRAM using
> the OMAP SRAM code.
>
> AM335x has a Cortex-M3 (WKUP_M3) which assists the MPU
> in DeepSleep0 entry and exit. WKUP_M3 takes care of the
> clockdomain and powerdomain transitions based on the
> intended low power state. MPU needs to load the appropriate
> WKUP_M3 binary onto the WKUP_M3 memory space before it can
> leverage any of the PM features like DeepSleep.
>
> The IPC mechanism between MPU and WKUP_M3 uses a mailbox
> sub-module and 8 IPC registers in the Control module. MPU
> uses the assigned Mailbox for issuing an interrupt to
> WKUP_M3 which then goes and checks the IPC registers for
> the payload. WKUP_M3 has the ability to trigger on interrupt
> to MPU by executing the "sev" instruction.
>
> In the current implementation when the suspend process
> is initiated MPU interrupts the WKUP_M3 to let it know about
> the intent of entering DeepSleep0 and waits for an ACK. When
> the ACK is received MPU continues with its suspend process
> to suspend all the drivers and then jumps to assembly in
> OCMC RAM. The assembly code puts the PLLs in bypass, puts the
> external RAM in self-refresh mode and then finally execute the
> WFI instruction. Execution of the WFI instruction triggers another
> interrupt to the WKUP_M3 which then continues wiht the power down
> sequence wherein the clockdomain and powerdomain transition takes
> place. As part of the sleep sequence, WKUP_M3 unmasks the interrupt
> lines for the wakeup sources. WFI execution on WKUP_M3 causes the
> hardware to disable the main oscillator of the SoC.
>
> When a wakeup event occurs, WKUP_M3 starts the power-up
> sequence by switching on the power domains and finally
> enabling the clock to MPU. Since the MPU gets powered down
> as part of the sleep sequence in the resume path ROM code
> starts executing. The ROM code detects a wakeup from sleep
> and then jumps to the resume location in OCMC which was
> populated in one of the IPC registers as part of the suspend
> sequence.
>
> The low level code in OCMC relocks the PLLs, enables access
> to external RAM and then jumps to the cpu_resume code of
> the kernel to finish the resume process.
>
> Signed-off-by: Vaibhav Bedia 
> Signed-off-by: Dave Gerlach 
> Cc: Tony Lingren 
> Cc: Santosh Shilimkar 
> Cc: Benoit Cousson 
> Cc: Paul Walmsley 
> Cc: Kevin Hilman 

Reviewed-by: Russ Dill 

In response to Nishanth's comments about the list of omap modules to
idle, I saw that, but it looks like its only for devices that no
device driver ever binds do, eg, the support is not built into the
kernel or the module is currently not loaded. Hence I don't think
there is any suspend/resume function that gets called. In reference to
the M3 handling it, the M3 wouldn't know which devices have a driver
bound and which don't.

> ---
>  arch/arm/mach-omap2/pm33xx.c  |  474 
> +
>  arch/arm/mach-omap2/pm33xx.h  |   77 +++
>  arch/arm/mach-omap2/wkup_m3.c |  183 
>  3 files changed, 734 insertions(+)
>  create mode 100644 arch/arm/mach-omap2/pm33xx.c
>  create mode 100644 arch/arm/mach-omap2/pm33xx.h
>  create mode 100644 arch/arm/mach-omap2/wkup_m3.c
>
> diff --git a/arch/arm/mach-omap2/pm33xx.c b/arch/arm/mach-omap2/pm33xx.c
> new file mode 100644
> index 000..d291c76
> --- /dev/null
> +++ b/arch/arm/mach-omap2/pm33xx.c
> @@ -0,0 +1,474 @@
> +/*
> + * AM33XX Power Management Routines
> + *
> + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/
> + * Vaibhav Bedia 
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "pm.h"
> +#include "cm33xx.h"
> +#include "pm33xx.h"
> +#include "control.h"
> +#include "common.h"
> +#include "clockdomain.h"
> +#include "powerdom

Re: [PATCH 5/5] arm: omap: Proper cleanups for omap_device

2013-08-08 Thread Tony Lindgren

* Pantelis Antoniou  [130807 09:31]:
> Hi Tony,
> 
> On Aug 7, 2013, at 7:15 PM, Tony Lindgren wrote:
> 
> > * Pantelis Antoniou  [130806 02:44]:
> >> On Aug 6, 2013, at 12:33 PM, Greg Kroah-Hartman wrote:
> >>> On Tue, Aug 06, 2013 at 10:53:44AM +0300, Pantelis Antoniou wrote:
>  +
>  static int _omap_device_notifier_call(struct notifier_block *nb,
> unsigned long event, void *dev)
>  {
>  @@ -185,9 +211,13 @@ static int _omap_device_notifier_call(struct 
>  notifier_block *nb,
>   struct omap_device *od;
>  
>   switch (event) {
>  -case BUS_NOTIFY_DEL_DEVICE:
>  +case BUS_NOTIFY_UNBOUND_DRIVER:
>  +/* NOTIFY_DEL_DEVICE is not the right call...
>  + * we use a callback here, to make sure no-one is going 
>  to
>  + * try to use the omap_device data after they're deleted
>  + */
>   if (pdev->archdata.od)
>  -omap_device_delete(pdev->archdata.od);
>  +device_schedule_callback(dev, 
>  _omap_device_cleanup);
> >>> 
> >>> Really?  This is one sign that you are totally using the driver core
> >>> incorrectly.  You shouldn't have to rely on notifier callbacks to handle
> >>> device removals, your bus code should do that for you directly.
> >>> 
> >>> I don't like this at all, sorry.
> >>> 
> >> 
> >> Don't shoot the messenger please...
> > 
> > As you're inititalizing capebus with DT, let's figure out what if
> > anything you actually need from omap_device. I'd much rather remove
> > dependencies than add more.
> > 
> 
> There is no such thing as capebus anymore. This is just the path of
> removing a platform device, which happens to also be an omap_device.

OK, so let's figure out the minimal fixes needed.
 
> >> This is all about fixing a crash without messing too many things.
> > 
> > It seems this fix is only needed for supporting out-of-tree code?
> > These features with omap_device we may not even want to support in
> > the mainline tree as is being discussed..
> > 
> 
> What out of tree code? The only thing this patch does is make sure we
> don't crash when a perfectly valid call to platform_device_unregister() 
> happens.
> 
> Drivers that don't use omap_device work just fine.

So what's the minimal set of fixes then?

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 7/9] ARM: OMAP: omap_device: Add APIs to enable and idle hwmods

On Tue, Aug 6, 2013 at 10:49 AM, Dave Gerlach  wrote:
> From: Vaibhav Bedia 
>
> Needed to let the AM335x PM handle the IPs which need forced
> standby transition during every suspend-resume cycle when
> the corresponding driver is not compiled into the kernel.
>
> Signed-off-by: Vaibhav Bedia 
> Signed-off-by: Dave Gerlach 

Reviewed-by: Russ Dill 

> ---
>  arch/arm/mach-omap2/omap_device.c |8 
>  arch/arm/mach-omap2/omap_device.h |2 ++
>  2 files changed, 10 insertions(+)
>
> diff --git a/arch/arm/mach-omap2/omap_device.c 
> b/arch/arm/mach-omap2/omap_device.c
> index 5cc9287..8cf63f6 100644
> --- a/arch/arm/mach-omap2/omap_device.c
> +++ b/arch/arm/mach-omap2/omap_device.c
> @@ -219,6 +219,10 @@ static int _omap_device_enable_hwmods(struct omap_device 
> *od)
> return 0;
>  }
>
> +int omap_device_enable_hwmods(struct omap_device *od)
> +{
> +   return _omap_device_enable_hwmods(od);
> +}
>  /**
>   * _omap_device_idle_hwmods - call omap_hwmod_idle() on all hwmods
>   * @od: struct omap_device *od
> @@ -236,6 +240,10 @@ static int _omap_device_idle_hwmods(struct omap_device 
> *od)
> return 0;
>  }
>
> +int omap_device_idle_hwmods(struct omap_device *od)
> +{
> +   return _omap_device_idle_hwmods(od);
> +}
>  /* Public functions for use by core code */
>
>  /**
> diff --git a/arch/arm/mach-omap2/omap_device.h 
> b/arch/arm/mach-omap2/omap_device.h
> index 17ca1ae..655ec35 100644
> --- a/arch/arm/mach-omap2/omap_device.h
> +++ b/arch/arm/mach-omap2/omap_device.h
> @@ -87,6 +87,8 @@ struct device *omap_device_get_by_hwmod_name(const char 
> *oh_name);
>
>  /* OMAP PM interface */
>  int omap_device_get_context_loss_count(struct platform_device *pdev);
> +int omap_device_enable_hwmods(struct omap_device *od);
> +int omap_device_idle_hwmods(struct omap_device *od);
>
>  /* Other */
>
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 6/9] ARM: OMAP2+: timer: Add suspend-resume callbacks for clkevent device

On Tue, Aug 6, 2013 at 10:49 AM, Dave Gerlach  wrote:
> From: Vaibhav Bedia 
>
> OMAP timer code registers two timers - one as clocksource
> and one as clockevent. Since AM33XX has only one usable timer
> in the WKUP domain one of the timers needs suspend-resume
> support to restore the configuration to pre-suspend state.
>
> commit adc78e6 (timekeeping: Add suspend and resume
> of clock event devices) introduced .suspend and .resume
> callbacks for clock event devices. Leverages these
> callbacks to have AM33XX clockevent timer which is
> in not in WKUP domain to behave properly across system
> suspend.
>
> Signed-off-by: Vaibhav Bedia 
> Signed-off-by: Dave Gerlach 

Reviewed-by: Russ Dill 

> ---
>  arch/arm/mach-omap2/timer.c |   32 
>  1 file changed, 32 insertions(+)
>
> diff --git a/arch/arm/mach-omap2/timer.c b/arch/arm/mach-omap2/timer.c
> index b37e1fc..cce5d39 100644
> --- a/arch/arm/mach-omap2/timer.c
> +++ b/arch/arm/mach-omap2/timer.c
> @@ -118,11 +118,43 @@ static void omap2_gp_timer_set_mode(enum 
> clock_event_mode mode,
> }
>  }
>
> +static void omap_clkevt_suspend(struct clock_event_device *unused)
> +{
> +   char name[10];
> +   struct omap_hwmod *oh;
> +
> +   sprintf(name, "timer%d", clkev.id);
> +   oh = omap_hwmod_lookup(name);
> +   if (!oh)
> +   return;
> +
> +   __omap_dm_timer_stop(&clkev, 1, clkev.rate);
> +   omap_hwmod_idle(oh);
> +}
> +
> +static void omap_clkevt_resume(struct clock_event_device *unused)
> +{
> +   char name[10];
> +   struct omap_hwmod *oh;
> +
> +   sprintf(name, "timer%d", clkev.id);
> +   oh = omap_hwmod_lookup(name);
> +   if (!oh)
> +   return;
> +
> +   omap_hwmod_enable(oh);
> +   __omap_dm_timer_load_start(&clkev,
> +   OMAP_TIMER_CTRL_ST | OMAP_TIMER_CTRL_AR, 0, 1);
> +   __omap_dm_timer_int_enable(&clkev, OMAP_TIMER_INT_OVERFLOW);
> +}
> +
>  static struct clock_event_device clockevent_gpt = {
> .features   = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
> .rating = 300,
> .set_next_event = omap2_gp_timer_set_next_event,
> .set_mode   = omap2_gp_timer_set_mode,
> +   .suspend= omap_clkevt_suspend,
> +   .resume = omap_clkevt_resume,
>  };
>
>  static struct property device_disabled = {
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 5/9] ARM: OMAP2+: AM33XX: Add assembly code for PM operations