Re: [PATCH tty v1 72/74] serial: ucc_uart: Use port lock wrappers

[Timur Tabi]

On Thu, Sep 14, 2023 at 1:39 PM John Ogness  wrote:

> Converted with coccinelle. No functional change.
>
> Signed-off-by: Thomas Gleixner 
> ---
>  drivers/tty/serial/ucc_uart.c | 4 ++--

Acked-by: Timur Tabi 

Re: [PATCH] Documentation: devices.txt: reconcile serial/ucc_uart minor numers

[Timur Tabi]

On Mon, Jul 24, 2023 at 1:33 AM Randy Dunlap  wrote:
>
> Reconcile devices.txt with serial/ucc_uart.c regarding device number
> assignments. ucc_uart.c supports 4 ports and uses minor devnums
> 46-49, so update devices.txt with that info.
> Then update ucc_uart.c's reference to the location of the devices.txt
> list in the kernel source tree.
>
> Fixes: d7584ed2b994 ("[POWERPC] qe-uart: add support for Freescale 
> QUICCEngine UART")
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Randy Dunlap 
> Cc: Timur Tabi 
> Cc: Kumar Gala 
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: Greg Kroah-Hartman 
> Cc: Jiri Slaby 
> Cc: linux-ser...@vger.kernel.org
> Cc: Jonathan Corbet 
> Cc: linux-...@vger.kernel.org

Acked-by: Timur Tabi 

One thing does concern me.  The UCC UART driver piggy-backs on the CPM
driver's layout (see cpm_uart.h), but apparently CPM UART supports 6
devices, not four:

#define UART_NRfs_uart_nr

where fs_uart_nr is defined in enum fs_uart_id.

Unfortunately, it's been so long since I've touched this code, I'm not
sure whether this means anything.

Re: [PATCH] serial: Use of_property_read_bool() for boolean properties

[Timur Tabi]

On Fri, Mar 10, 2023 at 8:48 AM Rob Herring  wrote:
>
> It is preferred to use typed property access functions (i.e.
> of_property_read_ functions) rather than low-level
> of_get_property/of_find_property functions for reading properties.
> Convert reading boolean properties to to of_property_read_bool().
>
> Signed-off-by: Rob Herring 
> ---
>  drivers/tty/serial/imx.c   | 14 +-
>  drivers/tty/serial/mxs-auart.c |  4 ++--
>  drivers/tty/serial/ucc_uart.c  |  2 +-

ucc_uart.c portion:

Acked-by: Timur Tabi 

Re: [PATCH v2 1/5] serial: ucc_uart: Remove custom frame size calculation

[Timur Tabi]

On Tue, Aug 30, 2022 at 3:49 AM Ilpo Järvinen
 wrote:
>
> The number of bits can be calculated using tty_get_frame_size(), no
> need for the driver to do it on its own.
>
> Also remove a comment on number of bits that doesn't match the code nor
> the comment on ucc_uart_pram's rx_length ("minus 1" part differs). That
> comment seems a verbatim copy of that in cpm_uart/cpm_uart_core.c
> anyway so perhaps it was just copied over w/o much thinking.
>
> Reviewed-by: Andy Shevchenko 
> Signed-off-by: Ilpo Järvinen 

Acked-by: Timur Tabi 

Re: [PATCH] tty: serial: Fix refcount leak bug in ucc_uart.c

[Timur Tabi]

On Sat, Jun 18, 2022 at 1:09 AM Liang He  wrote:
>
> In soc_info(), of_find_node_by_type() will return a node pointer
> with refcount incremented. We should use of_node_put() when it is
> not used anymore.
>
> Signed-off-by: Liang He 

Acked-by: Timur Tabi 

Re: [PATCH -next 9/9] ASoC: fsl_xcvr: check return value after calling platform_get_resource_byname()

[Timur Tabi]

On Fri, Jun 11, 2021 at 4:32 AM Yang Yingliang  wrote:

> rx_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "rxfifo");
> tx_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "txfifo");
> +   if (!rx_res || !tx_res) {
> +   dev_err(dev, "Invalid resource\n");
> +   return -EINVAL;
> +   }

If platform_get_resource_byname() returns an error, it's probably
because the name cannot be found.  So I think this error message is
more accurate:

"could not find rxfifo or txfifo resource"

Re: [PATCH 1/2] ASoC: fsl_xcvr: Add XCVR ASoC CPU DAI driver

[Timur Tabi]

On 9/18/20 9:21 AM, Viorel Suman (OSS) wrote:

+static const u32 fsl_xcvr_earc_channels[] = { 1, 2, 8, 16, 32, }; /*
+one bit 6, 12 ? */

What's the meaning of the comments?

Just a thought noted as comment. HDMI2.1 spec defines 6- and 12-channels layout 
when
one bit audio stream is transmitted - I was wandering how can this be enforced. 
Is a @todo like of comment.


Please don't add comments that other developers could never understand.

The text that you just wrote here would be a great starting point for a 
better comment.

Re: [PATCH v2 net-next] net: convert suitable drivers to use phy_do_ioctl_running

[Timur Tabi]

On 1/21/20 3:09 PM, Heiner Kallweit wrote:

  drivers/net/ethernet/qualcomm/emac/emac.c  | 14 +-


Acked-by: Timur Tabi 

Re: [PATCH] evh_bytechan: fix out of bounds accesses

[Timur Tabi]

On 1/15/20 2:01 PM, Scott Wood wrote:

FWIW I'd rather see the original patch,
that keeps the raw asm hcall stuff as simple wrappers in one place.


I can live with that.

Re: [PATCH] evh_bytechan: fix out of bounds accesses

[Timur Tabi]

On 1/14/20 12:31 AM, Stephen Rothwell wrote:

+/**
+ * ev_byte_channel_send - send characters to a byte stream
+ * @handle: byte stream handle
+ * @count: (input) num of chars to send, (output) num chars sent
+ * @bp: pointer to chars to send
+ *
+ * Returns 0 for success, or an error code.
+ */
+static unsigned int ev_byte_channel_send(unsigned int handle,
+   unsigned int *count, const char *bp)


Well, now you've moved this into the .c file and it is no longer 
available to other callers.  Anything wrong with keeping it in the .h file?

Re: [PATCH] evh_bytechan: fix out of bounds accesses

[Timur Tabi]

On 1/14/20 2:29 AM, Segher Boessenkool wrote:

You have no working lswx I suppose?:-)


I don't know if the P4080 supports lswx, but it does, than that would be 
an elegant way to fix this bug.

Re: [PATCH] evh_bytechan: fix out of bounds accesses

[Timur Tabi]

On 1/14/20 3:18 AM, Laurentiu Tudor wrote:

I can offer myself. I'll send a MAINTAINERS patch if nobody is against it.


Yes, please do.  I'm always available if you have any questions on the code.

Re: [PATCH] MAINTAINERS: Add myself as maintainer of ehv_bytechan tty driver

[Timur Tabi]

On 1/14/20 5:00 AM, Laurentiu Tudor wrote:

Michael Ellerman made a call for volunteers from NXP to maintain
this driver and I offered myself.

Signed-off-by: Laurentiu Tudor


Acked-by: Timur Tabi 

Re: [PATCH] evh_bytechan: fix out of bounds accesses

[Timur Tabi]

On 1/13/20 7:10 PM, Timur Tabi wrote:


I would prefer that ev_byte_channel_send() is updated to access only 
'count' bytes.  If that means adding a memcpy to the 
ev_byte_channel_send() itself, then so be it.  Trying to figure out how 
to stuff n bytes into 4 32-bit registers is probably not worth the effort.


Looks like ev_byte_channel_receive() has the same bug, but in reverse.

Re: [PATCH] evh_bytechan: fix out of bounds accesses

[Timur Tabi]

On 1/13/20 2:25 PM, Stephen Rothwell wrote:

The problem is not really the declaration, the problem is that
ev_byte_channel_send always accesses 16 bytes from the buffer and it is
not always passed a buffer that long (in one case it is passed a
pointer to a single byte).  So the alternative to the memcpy approach I
have take is to complicate ev_byte_channel_send so that only accesses
count bytes from the buffer.


Ah, I see now.  This is all coming back to me.

I would prefer that ev_byte_channel_send() is updated to access only 
'count' bytes.  If that means adding a memcpy to the 
ev_byte_channel_send() itself, then so be it.  Trying to figure out how 
to stuff n bytes into 4 32-bit registers is probably not worth the effort.

Re: [PATCH] evh_bytechan: fix out of bounds accesses

[Timur Tabi]

On Thu, Jan 9, 2020 at 1:41 AM Stephen Rothwell  wrote:
>
> ev_byte_channel_send() assumes that its third argument is a 16 byte array.
> Some places where it is called it may not be (or we can't easily tell
> if it is).  Newer compilers have started producing warnings about this,
> so make sure we actually pass a 16 byte array.

...

> +static unsigned int local_ev_byte_channel_send(unsigned int handle,
> +unsigned int *count, const char *p)
> +{
> +   char buffer[EV_BYTE_CHANNEL_MAX_BYTES];
> +   unsigned int c = *count;
> +
> +   if (c < sizeof(buffer)) {
> +   memcpy(buffer, p, c);
> +   memset([c], 0, sizeof(buffer) - c);
> +   p = buffer;
> +   }
> +   return ev_byte_channel_send(handle, count, p);
> +}

Why not simply correct the parameters of ev_byte_channel_send?

static inline unsigned int ev_byte_channel_send(unsigned int handle,
-unsigned int *count, const char buffer[EV_BYTE_CHANNEL_MAX_BYTES])
+unsigned int *count, const char *buffer)

Back then, I probably thought I was just being clever with this code,
but I realize now that it doesn't make sense to do the way I did.

Re: [PATCH] evh_bytechan: fix out of bounds accesses

[Timur Tabi]

On 1/13/20 8:34 AM, Laurentiu Tudor wrote:

There are a few users that I know of, but I can't tell if that's enough
to justify keeping the driver.

[1]https://source.codeaurora.org/external/qoriq/qoriq-yocto-sdk/hypervisor/


IIRC, the driver is the only reasonable way to get a serial console from 
a guest.  So if there are users of the hypervisor, then I think there's 
a good chance at least one is using the byte channel driver.

Re: [PATCH] evh_bytechan: fix out of bounds accesses

[Timur Tabi]

On 1/13/20 6:26 AM, Michael Ellerman wrote:

I've never heard of it, and I have no idea how to test it.

It's not used by qemu, I guess there is/was a Freescale hypervisor that
used it.


Yes, there is/was a Freescale hypervisor that I and a few others worked 
on.  I've added a couple people on CC that might be able to tell the 
current disposition of it.



But maybe it's time to remove it if it's not being maintained/used by
anyone?


I wouldn't be completely opposed to that if there really are no more 
users.  There really weren't any users even when I wrote the driver.


I haven't had a chance to study the patch, but my first instinct is that 
there's got to be a better way to fix this than introducing a memcpy.

Re: [PATCH] serial: ucc_uart: remove redundant assignment to pointer bdp

[Timur Tabi]

On 12/19/19 6:10 PM, Colin King wrote:

From: Colin Ian King

The variable bdp is being initialized with a value that is never
read and it is being updated later with a new value. The initialization
is redundant and can be removed.

Addresses-Coverity: ("Unused value")
Signed-off-by: Colin Ian King


Acked-by: Timur Tabi 

Looks like this bug has been there since day 1.

Re: [PATCH v6 00/49] QUICC Engine support on ARM, ARM64, PPC64

[Timur Tabi]

On 11/28/19 8:55 AM, Rasmus Villemoes wrote:

There have been several attempts in the past few years to allow
building the QUICC engine drivers for platforms other than PPC32. This
is yet another attempt.

v5 can be found 
here:https://lore.kernel.org/lkml/20191118112324.22725-1-li...@rasmusvillemoes.dk/


If it helps:

Entire series:
Acked-by: Timur Tabi 

I've worked on all code covered by this patchset except for the hdlc 
driver.  I don't know if my ACKs are acceptable to everyone, but you 
have them regardless.

Re: [PATCH v5 00/48] QUICC Engine support on ARM, ARM64, PPC64

[Timur Tabi]

On 11/18/19 5:22 AM, Rasmus Villemoes wrote:

There have been several attempts in the past few years to allow
building the QUICC engine drivers for platforms other than PPC32. This
is yet another attempt.

v4 can be found here: 
https://lore.kernel.org/lkml/20191108130123.6839-1-li...@rasmusvillemoes.dk/


This is excellent work, thank you.

All patches:

Reviewed-by: Timur Tabi 

Serial patches:

Acked-by: Timur Tabi 

Re: [PATCH v4 32/47] serial: ucc_uart: use of_property_read_u32() in ucc_uart_probe()

[Timur Tabi]

On 11/15/19 2:01 AM, Rasmus Villemoes wrote:

That would be a separate patch, this patch is only concerned with
eliminating the implicit assumption of the host being big-endian. And
there's already been some pushback to adding arch-specific ifdefs (which
I agree with, but as I responded there see as the lesser evil), so
unless there's a very good reason to add that complexity, I'd rather not.


We don't want to encourage people to introduce device trees that don't 
have the brg-frequency property in them.

Re: [PATCH v4 45/47] net/wan/fsl_ucc_hdlc: reject muram offsets above 64K

[Timur Tabi]

On 11/15/19 1:44 AM, Rasmus Villemoes wrote:

I can change it, sure, but it's a matter of taste. To me the above asks
"does the value change when it is truncated to a u16" which makes
perfect sense when the value is next used with iowrite16be(). Using a
comparison to U16_MAX takes more brain cycles for me, because I have to
think whether it should be > or >=, and are there some
signedness/integer promotion business interfering with that test.


Ok.

Re: [PATCH v4 46/47] net: ethernet: freescale: make UCC_GETH explicitly depend on PPC32

[Timur Tabi]

On 11/15/19 1:54 AM, Rasmus Villemoes wrote:

"Also, the QE Ethernet has never been integrated on any non-PowerPC SoC
and most likely will not be in the future."


That works for me, thanks.

Re: [PATCH v4 46/47] net: ethernet: freescale: make UCC_GETH explicitly depend on PPC32

[Timur Tabi]

On 11/14/19 11:44 PM, Li Yang wrote:

Can you add an explanation why we don't want ucc_geth on non-PowerPC platforms?

I think it is because the QE Ethernet was never integrated in any
non-PowerPC SoC and most likely will not be in the future.  We
probably can make it compile for other architectures for general code
quality but it is not a priority.


This explanation belongs in the commit message.

Re: [PATCH v4 07/47] soc: fsl: qe: qe.c: guard use of pvr_version_is() with CONFIG_PPC32

[Timur Tabi]

On Fri, Nov 8, 2019 at 7:04 AM Rasmus Villemoes
 wrote:
>
> +static bool qe_general4_errata(void)
> +{
> +#ifdef CONFIG_PPC32
> +   return pvr_version_is(PVR_VER_836x) || pvr_version_is(PVR_VER_832x);
> +#endif
> +   return false;
> +}
> +
>  /* Program the BRG to the given sampling rate and multiplier
>   *
>   * @brg: the BRG, QE_BRG1 - QE_BRG16
> @@ -223,7 +231,7 @@ int qe_setbrg(enum qe_clock brg, unsigned int rate, 
> unsigned int multiplier)
> /* Errata QE_General4, which affects some MPC832x and MPC836x SOCs, 
> says
>that the BRG divisor must be even if you're not using divide-by-16
>mode. */

Can you also move this comment (and fix the comment formatting so that
it's a proper function comment) to qe_general4_errata()?

Re: [PATCH v4 45/47] net/wan/fsl_ucc_hdlc: reject muram offsets above 64K

[Timur Tabi]

On Fri, Nov 8, 2019 at 7:04 AM Rasmus Villemoes
 wrote:

> diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
> index 8d13586bb774..f029eaa7cfc0 100644
> --- a/drivers/net/wan/fsl_ucc_hdlc.c
> +++ b/drivers/net/wan/fsl_ucc_hdlc.c
> @@ -245,6 +245,11 @@ static int uhdlc_init(struct ucc_hdlc_private *priv)
> ret = -ENOMEM;
> goto free_riptr;
> }
> +   if (riptr != (u16)riptr || tiptr != (u16)tiptr) {

"riptr/tiptr > U16_MAX" is clearer.

Re: [PATCH v4 46/47] net: ethernet: freescale: make UCC_GETH explicitly depend on PPC32

[Timur Tabi]

On Fri, Nov 8, 2019 at 7:04 AM Rasmus Villemoes
 wrote:
>
> Currently, QUICC_ENGINE depends on PPC32, so this in itself does not
> change anything. In order to allow removing the PPC32 dependency from
> QUICC_ENGINE and avoid allmodconfig build failures, add this explicit
> dependency.

Can you add an explanation why we don't want ucc_geth on non-PowerPC platforms?

Re: [PATCH v4 32/47] serial: ucc_uart: use of_property_read_u32() in ucc_uart_probe()

[Timur Tabi]

On Fri, Nov 8, 2019 at 7:03 AM Rasmus Villemoes
 wrote:
>
> +   if (of_property_read_u32(np, "cell-index", ) &&
> +   of_property_read_u32(np, "device-id", )) {

I know that this is technically correct, but it's obfuscated IMHO.
'val' is set correctly only when of_property_read_u32(...) is "false",
which is doubly-weird because of_property_read_u32(...) doesn't
actually return a boolean.

I would rather you break this into two if-statements like the original code.

Re: [PATCH v4 32/47] serial: ucc_uart: use of_property_read_u32() in ucc_uart_probe()

[Timur Tabi]

On Fri, Nov 8, 2019 at 7:03 AM Rasmus Villemoes
 wrote:
>
> -   if (*iprop)
> -   qe_port->port.uartclk = *iprop;
> +   if (val)
> +   qe_port->port.uartclk = val;
> else {
> /*
>  * Older versions of U-Boot do not initialize the 
> brg-frequency
>  * property, so in this case we assume the BRG frequency is
>  * half the QE bus frequency.
>  */

This bug in older U-Boots is definitely PowerPC-specific, so could you
change this so that it reports an error on ARM if brg-frequency is
zero, and displays a warning on PowerPC?

Re: [PATCH v4 30/47] serial: ucc_uart: factor out soft_uart initialization

[Timur Tabi]

On Fri, Nov 8, 2019 at 7:03 AM Rasmus Villemoes
 wrote:
>
> -   /*
> -* Determine if we need Soft-UART mode
> -*/
> if (of_find_property(np, "soft-uart", NULL)) {
> dev_dbg(>dev, "using Soft-UART mode\n");
> soft_uart = 1;
> +   } else {
> +   return 0;
> }

How about:

if (!of_find_property(np, "soft-uart", NULL))
return 0;

And I think you should be able to get rid of the "soft_uart" variable.

Re: [PATCH v4 04/47] soc: fsl: qe: introduce qe_io{read,write}* wrappers

[Timur Tabi]

On 11/12/19 1:14 AM, Rasmus Villemoes wrote:

but that's because readl and writel by definition work on little-endian
registers. I.e., on a BE platform, the readl and writel implementation
must themselves contain a swab, so the above would end up doing two
swabs on a BE platform.


Do you know whether the compiler optimizes-out the double swab?


(On PPC, there's a separate definition of mmio_read32be, namely
writel_be, which in turn does a out_be32, so on PPC that doesn't
actually end up doing two swabs).

So ioread32be etc. have well-defined semantics: access a big-endian
register and return the result in native endianness.


It seems weird that there aren't any cross-arch lightweight 
endian-specific I/O accessors.

Re: [PATCH v4 04/47] soc: fsl: qe: introduce qe_io{read, write}* wrappers

[Timur Tabi]

On Fri, Nov 8, 2019 at 7:03 AM Rasmus Villemoes
 wrote:
>
> The QUICC engine drivers use the powerpc-specific out_be32() etc. In
> order to allow those drivers to build for other architectures, those
> must be replaced by iowrite32be(). However, on powerpc, out_be32() is
> a simple inline function while iowrite32be() is out-of-line. So in
> order not to introduce a performance regression on powerpc when making
> the drivers work on other architectures, introduce qe_io* helpers.

Isn't it also true that iowrite32be() assumes a little-endian platform
and always does a byte swap?

Re: [alsa-devel] [PATCH] ASoC: cs42xx8: Remove S32_LE in format list

[Timur Tabi]

On 3/1/19 12:32 AM, S.j. Wang wrote:

This case is covered by S24_LE I think.  The S32_LE means the data is 32bit and 
slot width
Is 32bit, this is not in data sheet.


The problem is that if you have 32-bit samples in your audio file, and 
you want to play them, then software (e.g. alsalib) will need to convert 
the audio to 24-bit before sending it to hardware.  This is unnecessary 
because the hardware can "convert" the sample to 24-bit automatically by 
ignoring the lower 8 bits.


I think a lot of codecs do this already.

Re: [PATCH] ASoC: cs42xx8: Remove S32_LE in format list

[Timur Tabi]

On 2/27/19 11:56 PM, S.j. Wang wrote:

cs42xx8 is a 24-bit A/D and 24-bit D/A device, so the S32_LE
should not be in the supported format list.

Signed-off-by: Shengjiu Wang


Is the device capable of accepting 32-bit samples, even if it downgrades 
it to 24-bit internally?  If so, then maybe SNDRV_PCM_FMTBIT_S32_LE 
should stay.

Re: [PATCH v3 18/18] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs

[Timur Tabi]

On 3/16/18 6:04 PM, Steve Wise wrote:

Anybody understand why the PPC implementation of writeX_relaxed() isn't
relaxed?


You probably should ask that on the linuxppc-dev@lists.ozlabs.org 
mailing list.


I've always wondered why PowerPC has non-standard I/O accessors.

--
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v4 00/11] ASoC: fsl_ssi: Clean up - coding style level

[Timur Tabi]

On 12/17/17 8:51 PM, Nicolin Chen wrote:

Nicolin Chen (11):
   ASoC: fsl_ssi: Rename fsl_ssi_private to fsl_ssi
   ASoC: fsl_ssi: Cache pdev->dev pointer
   ASoC: fsl_ssi: Refine all comments
   ASoC: fsl_ssi: Rename registers and fields macros
   ASoC: fsl_ssi: Refine indentations and wrappings
   ASoC: fsl_ssi: Refine printk outputs
   ASoC: fsl_ssi: Rename cpu_dai parameter to dai
   ASoC: fsl_ssi: Rename scr_val to scr
   ASoC: fsl_ssi: Replace fsl_ssi_rxtx_reg_val with fsl_ssi_regvals
   ASoC: fsl_ssi: Rename i2smode to i2s_net
   ASoC: fsl_ssi: Define ternary macros to simplify code


Acked-by: Timur Tabi <ti...@tabi.org>

Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments

[Timur Tabi]

On 12/16/17 11:49 AM, Nicolin Chen wrote:

I never said that I don't agree with Timur. Every change here is
to simplify things. As long as Timur or any reviewer feels one of
new comments is harder to understand, I am totally fine to rework.
I respect everyone's opinion, but I hope everyone can respect my
effort too by telling me which one needs to rework and why?


I do respect it.  I apologize if I came across as unduly harsh.

Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments

[Timur Tabi]

On 12/16/17 11:30 AM, Caleb Crome wrote:

Having come to work on this driver with very little knowledge about
kernel programming, and i.MX, I have to agree with Timur. It's an
amazingly complex driver (with support of so many variants).  By
eliminating verbose commentary, it's also wiping away a lot of
knowledge.  The more sparse commentary makes things harder to
understand for newcomers, or really anybody who isn't already steeped
in knowledge about the SSI port and linux, and it's interaction with
DMA.


This is exactly why I wrote the comments the way I did.  As I was 
learning about the hardware and ASoC drivers, whenever I was confused 
about something and then figured it out, I would add a comment about 
that.  I figured if it wasn't obvious to me, it wouldn't be obvious to 
anyone else.  And since this was one of the first ASoC drivers, and the 
first to use device tree, it would become a reference driver for years 
to come.  That made it even more important that I document everything I 
learned.


I am pleased that other developers kept up that commenting style.  This 
patch destroys all of that.

Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments

[Timur Tabi]

On 12/13/17 5:18 PM, Nicolin Chen wrote:


-   /* Used when using fsl-ssi as sound-card. This is only used by ppc and
-* should be replaced with simple-sound-card. */
struct platform_device *pdev;


Is this comment no longer true?


+ * 1) SSI in earlier SoCS has crtical bits in control registers that


critical


-/**
- * fsl_ssi_isr: SSI interrupt handler
- *
- * Although it's possible to use the interrupt handler to send and receive
- * data to/from the SSI, we use the DMA instead.  Programming is more
- * complicated, but the performance is much better.
- *
- * This interrupt handler is used only to gather statistics.
- *
- * @irq: IRQ of the SSI device
- * @dev_id: pointer to the fsl_ssi structure for this SSI device
- */


What's wrong with this comment?


-/*
- * Clear RX or TX FIFO to remove samples from the previous
- * stream session which may be still present in the FIFO and
- * may introduce bad samples and/or channel slipping.
- *
- * Note: The SOR is not documented in recent IMX datasheet, but
- * is described in IMX51 reference manual at section 56.3.3.15.
+/**
+ * Clear remaining data in the FIFO to avoid dirty data or channel slipping


I think the original is better, unless there's something untrue about it.


-* We are running on a SoC which does not support online SSI
-* reconfiguration, so we have to enable all necessary flags at once
-* even if we do not use them later (capture and playback configuration)
+* Online configuration is not supported
+* Enable or Disable all necessary bits at once


Ditto


-   /*
-* Configure single direction units while the SSI unit is running
-* (online configuration)
-*/
+   /* Online configure single direction while SSI is running */


Ditto


-   /*
-* Disabling the necessary flags for one of rx/tx while the
-* other stream is active is a little bit more difficult. We
-* have to disable only those flags that differ between both
-* streams (rx XOR tx) and that are set in the stream that is
-* disabled now. Otherwise we could alter flags of the other
-* stream
-*/
-
-   /* These assignments are simply vals without bits set in avals*/
+   /* Exclude necessary bits for the opposite stream */


Ditto


-   /*
-* Be sure the Tx FIFO is filled when TE is set.
-* Otherwise, there are some chances to start the
-* playback with some void samples inserted first,
-* generating a channel slip.
-*
-* First, SSIEN must be set, to let the FIFO be filled.
-*
-* Notes:
-* - Limit this fix to the DMA case until FIQ cases can
-*   be tested.
-* - Limit the length of the busy loop to not lock the
-*   system too long, even if 1-2 loops are sufficient
-*   in general.
-*/


What's wrong with this comment?


-   /*
-* Note that these below aren't just normal registers.
-* They are a way to disable or enable bits in SACCST
-* register:
-* - writing a '1' bit at some position in SACCEN sets the
-* relevant bit in SACCST,
-* - writing a '1' bit at some position in SACCDIS unsets
-* the relevant bit in SACCST register.
-*
-* The two writes below first disable all channels slots,
-* then enable just slots 3 & 4 ("PCM Playback Left Channel"
-* and "PCM Playback Right Channel").
-*/
+   /* Disable all channel slots */


Ditto.



-* Why are we setting up SACCST everytime we are starting a
-* playback?
-* Some CODECs (like VT1613 CODEC on UDOO board) like to
-* (sometimes) set extra bits in their SLOTREQ requests.
-* When a bit is set in a SLOTREQ request then SSI sets the
-* relevant bit in SACCST automatically (it is enough if a bit was
-* set in a SLOTREQ just once, bits in SACCST are 'sticky').
-* If an extra slot gets enabled that's a disaster for playback
-* because some of normal left or right channel samples are
-* redirected instead to this extra slot.
+* SACCST might be modified via AC Link by a CODEC if it sends
+* extra bits in their SLOTREQ requests, which'll accidentally
+* send valid data to slots other than normal playback slots.
 *
-* A workaround implemented in fsl-asoc-card of setting an
-* appropriate CODEC register so that slots 3 & 4 

Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments

[Timur Tabi]

On 12/16/17 12:10 AM, Nicolin Chen wrote:

Hi,

I am outside so can't use mutt. Sorry for that.

This comment is going to be replaced in the 2nd set anyway because the 
whole function will be replaced.


So you're asking me to review comment changes that will soon be deleted? 
 Can you send out a new patch without changes to comments, so that I 
can focus on the ones that matter?


And please point out all comments that you think I need to rework. I am 
totally fine to do that. I don't think every single one is bad. And this 
patch has to go in as it also adds a lot of new comments.


Ok.

Re: [PATCH v3 00/11] ASoC: fsl_ssi: Clean up - coding style level

[Timur Tabi]

On 12/13/17 5:18 PM, Nicolin Chen wrote:

Additionally, in order to fix/work-around hardware bugs and design
flaws, the driver made a lot of compromise so now its program flow
looks very complicated and it's getting hard to maintain or update.

So I am going to clean up the driver on both coding style level and
program flow level.


I'm okay with everything except patch #3.

Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments

[Timur Tabi]

On 12/13/17 5:18 PM, Nicolin Chen wrote:

-* We are running on a SoC which does not support online SSI
-* reconfiguration, so we have to enable all necessary flags at once
-* even if we do not use them later (capture and playback configuration)
+* Online configuration is not supported
+* Enable or Disable all necessary bits at once


This is an example of a bad change, IMHO.  The original was written in 
elegant prose.  The new version is just two short sentences.

Re: [PATCH 0/3] tty/serial/ucc_uart: Adjustments for two functions

[Timur Tabi]

On 12/06/2017 03:10 PM, SF Markus Elfring wrote:


Three update suggestions were taken into account
from static source code analysis.

Markus Elfring (3):
   Delete an error message for a failed memory allocation in ucc_uart_probe()
   Improve a size determination in ucc_uart_probe()
   Fix a typo in a comment line


All three:

Acked-by: Timur Tabi <ti...@tabi.org>

Re: [PATCH 04/10] ASoC: fsl_ssi: Refine all comments

[Timur Tabi]

On 12/4/17 2:46 PM, Nicolin Chen wrote:

This patch refines the comments by:
1) Removing all out-of-date comments
2) Removing all not-so-useful comments
3) Unifying the styles of all comments
4) Simplifying over-descriptive comments
5) Adding comments to improve code readablity
6) Moving all register related comments to fsl_ssi.h
7) Adding comments to all register and field defines

Even after adding dozens of lines in fsl_ssi.h, this patch reduces
100 lines totally.


I'll review the other patches later, but I'm not keen on your removal of 
some of the comments in this patch.  I don't see why line count is so 
important, and you're removing some informative text.  I can see 
removing trivial comments and outdated ones, but "no-so-useful" and 
"over-descriptive" are subjective.

Re: [PATCH 05/11] serial: uuc_uart: constify uart_ops structures

[Timur Tabi]

On 8/13/17 1:21 AM, Julia Lawall wrote:

These uart_ops structures are only stored in the ops field of a
uart_port structure and this fields is const, so the uart_ops
structures can also be const.

Done with the help of Coccinelle.

Signed-off-by: Julia Lawall<julia.law...@lip6.fr>


Acked-by: Timur Tabi <ti...@tabi.org>

Re: [PATCH] sound: fsl_dma: remove dma_object path member

[Timur Tabi]

On 7/18/17 4:43 PM, Rob Herring wrote:

-   dev_err(>dev, "could not determine resources for %s\n",
-   ssi_np->full_name);
+   dev_err(>dev, "could not determine resources for %pOF\n",
+   ssi_np);


I think you meant to include this in the other patch.

Re: [PATCH] ASoC : fsl_ssi : Correct the condition to check AC97 mode

[Timur Tabi]

Andreas Schwab wrote:

> +  return !!(ssi_private->dai_fmt & SND_SOC_DAIFMT_AC97) ==
> +  SND_SOC_DAIFMT_AC97;

This is never true.


I think the right parenthesis should be at the end of the expression.

Re: [PATCH 1/1] serial/uuc_uart: Set shutdown timeout to CONFIG_HZ independent 2ms

[Timur Tabi]

Alexander Stein wrote:

Okay, I was just wondering why the timeout is dependant on the timer tick.
That didn't seem obvious to me.
Rethinking about this, I would rather replace those lines with msleep instead.


What's wrong with leaving it as-is?  The code is five years old, and 
Freescale/NXP barely uses the QE any more.  I don't have access to any 
hardware to test any changes you would propose.

Re: [PATCH 1/1] serial/uuc_uart: Set shutdown timeout to CONFIG_HZ independent 2ms

[Timur Tabi]

Alexander Stein wrote:

-   schedule_timeout(2);
+   schedule_timeout(msecs_to_jiffies(2));


NACK.

So I don't remember why I wrote this code, but I don't think I was 
expecting it to be 2ms.  Instead, I think I just wanted it to be some 
delay, but I believed that schedule_timeout(1) was too short or would be 
"optimized" out somehow.


Note that right below this, I do:

if (qe_port->wait_closing) {
/* Wait a bit longer */
set_current_state(TASK_UNINTERRUPTIBLE);
schedule_timeout(qe_port->wait_closing);
}

And wait_closing is a number of jiffies, so I knew that 
schedule_timeout() took jiffies as a parameter.


So I think I'm going to NACK this patch, since I believe I knew what I 
was doing when I wrote it five years ago.

Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults

[Timur Tabi]

Maciej S. Szmigiero wrote:

On 17.01.2016 15:16, Maciej S. Szmigiero wrote:

On 17.01.2016 06:16, Timur Tabi wrote:

Maciej S. Szmigiero wrote:

This is because (at least according to the datasheet) imx21-class SSI
registers end at CCSR_SSI_SRMSK (no SACC{ST,EN,DIS} regs), so
reading them for cache initialization may not be safe.

Also, a "MXC 91221 only" comment before these regs in FSL tree
(drivers/mxc/ssi/registers.h) seems to confirm that these registers
aren't present at least on some SSI (or SoC) models.


Can't we just mark them as precious or something, so that we don't have to have 
two structures?


Looks like it can be done with just one static regmap config struct
used then as template - I will post updated patch.


Updated patch:
diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 40dfd8a36484..105de76dd2fc 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -112,20 +112,6 @@ struct fsl_ssi_rxtx_reg_val {
struct fsl_ssi_reg_val tx;
  };

-static const struct reg_default fsl_ssi_reg_defaults[] = {
-   {CCSR_SSI_SCR, 0x},
-   {CCSR_SSI_SIER,0x3003},
-   {CCSR_SSI_STCR,0x0200},
-   {CCSR_SSI_SRCR,0x0200},
-   {CCSR_SSI_STCCR,   0x0004},
-   {CCSR_SSI_SRCCR,   0x0004},
-   {CCSR_SSI_SACNT,   0x},
-   {CCSR_SSI_STMSK,   0x},
-   {CCSR_SSI_SRMSK,   0x},
-   {CCSR_SSI_SACCEN,  0x},
-   {CCSR_SSI_SACCDIS, 0x},
-};
-
  static bool fsl_ssi_readable_reg(struct device *dev, unsigned int reg)
  {
switch (reg) {
@@ -190,8 +176,7 @@ static const struct regmap_config fsl_ssi_regconfig = {
.val_bits = 32,
.reg_stride = 4,
.val_format_endian = REGMAP_ENDIAN_NATIVE,
-   .reg_defaults = fsl_ssi_reg_defaults,
-   .num_reg_defaults = ARRAY_SIZE(fsl_ssi_reg_defaults),
+   .num_reg_defaults_raw = CCSR_SSI_SACCDIS / 4 + 1,


Replace "4" with "sizeof(uint32_t).


.readable_reg = fsl_ssi_readable_reg,
.volatile_reg = fsl_ssi_volatile_reg,
.precious_reg = fsl_ssi_precious_reg,
@@ -201,6 +186,7 @@ static const struct regmap_config fsl_ssi_regconfig = {

  struct fsl_ssi_soc_data {
bool imx;
+   bool imx21regs;


Please add a comment explaining why this is needed.


bool offline_config;
u32 sisr_write_mask;
  };
@@ -295,6 +281,7 @@ struct fsl_ssi_private {

  static struct fsl_ssi_soc_data fsl_ssi_mpc8610 = {
.imx = false,
+   .imx21regs = false,


This is unnecessary.  The default is already 0 (false).


.offline_config = true,
.sisr_write_mask = CCSR_SSI_SISR_RFRC | CCSR_SSI_SISR_TFRC |
CCSR_SSI_SISR_ROE0 | CCSR_SSI_SISR_ROE1 |
@@ -303,12 +290,14 @@ static struct fsl_ssi_soc_data fsl_ssi_mpc8610 = {

  static struct fsl_ssi_soc_data fsl_ssi_imx21 = {
.imx = true,
+   .imx21regs = true,
.offline_config = true,
.sisr_write_mask = 0,
  };

  static struct fsl_ssi_soc_data fsl_ssi_imx35 = {
.imx = true,
+   .imx21regs = false,


Same here.


.offline_config = true,
.sisr_write_mask = CCSR_SSI_SISR_RFRC | CCSR_SSI_SISR_TFRC |
CCSR_SSI_SISR_ROE0 | CCSR_SSI_SISR_ROE1 |
@@ -317,6 +306,7 @@ static struct fsl_ssi_soc_data fsl_ssi_imx35 = {

  static struct fsl_ssi_soc_data fsl_ssi_imx51 = {
.imx = true,
+   .imx21regs = false,
.offline_config = false,
.sisr_write_mask = CCSR_SSI_SISR_ROE0 | CCSR_SSI_SISR_ROE1 |
CCSR_SSI_SISR_TUE0 | CCSR_SSI_SISR_TUE1,
@@ -586,8 +576,11 @@ static void fsl_ssi_setup_ac97(struct fsl_ssi_private 
*ssi_private)
 */
regmap_write(regs, CCSR_SSI_SACNT,
CCSR_SSI_SACNT_AC97EN | CCSR_SSI_SACNT_FV);
-   regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
-   regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
+
+   if (!ssi_private->soc->imx21regs) {
+   regmap_write(regs, CCSR_SSI_SACCDIS, 0xff);
+   regmap_write(regs, CCSR_SSI_SACCEN, 0x300);
+   }


This needs a comment.



/*
 * Enable SSI, Transmit and Receive. AC97 has to communicate with the
@@ -1397,6 +1390,7 @@ static int fsl_ssi_probe(struct platform_device *pdev)
struct resource *res;
void __iomem *iomem;
char name[64];
+   struct regmap_config regconfig = fsl_ssi_regconfig;

of_id = of_match_device(fsl_ssi_ids, >dev);
if (!of_id || !of_id->data)
@@ -1444,15 +1438,22 @@ static int fsl_ssi_probe(struct platform_device *pdev)
return PTR_ERR(iomem);
ssi_private->ssi_phys = res->start;

+   if (ssi_private->soc->imx21regs) {
+   /* According to datasheet imx21-class SSI have less regs */


First of all, it would be "fewer regs", but even better would be to say 
that certain regs don't e

Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults

[Timur Tabi]

Maciej S. Szmigiero wrote:

+static const struct regmap_config fsl_ssi_regconfig_imx21 = {
+   .max_register = CCSR_SSI_SRMSK,
+   .reg_bits = 32,
+   .val_bits = 32,
+   .reg_stride = 4,
+   .val_format_endian = REGMAP_ENDIAN_NATIVE,
+   .num_reg_defaults_raw = CCSR_SSI_SRMSK / 4 + 1,
+   .readable_reg = fsl_ssi_readable_reg,
+   .volatile_reg = fsl_ssi_volatile_reg,
+   .precious_reg = fsl_ssi_precious_reg,
+   .writeable_reg = fsl_ssi_writeable_reg,
+   .cache_type = REGCACHE_RBTREE,
+};
+
  static const struct regmap_config fsl_ssi_regconfig = {
.max_register = CCSR_SSI_SACCDIS,
.reg_bits = 32,
.val_bits = 32,
.reg_stride = 4,
.val_format_endian = REGMAP_ENDIAN_NATIVE,
-   .reg_defaults = fsl_ssi_reg_defaults,
-   .num_reg_defaults = ARRAY_SIZE(fsl_ssi_reg_defaults),
+   .num_reg_defaults_raw = CCSR_SSI_SACCDIS / 4 + 1,
.readable_reg = fsl_ssi_readable_reg,
.volatile_reg = fsl_ssi_volatile_reg,
.precious_reg = fsl_ssi_precious_reg,


Is this really necessary?  Why do we need separate register configs for 
one specific SOC?  There are already too many "if 
(some_stupid_imx_variant)" blocks in this driver.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults

[Timur Tabi]

Maciej S. Szmigiero wrote:

This is because (at least according to the datasheet) imx21-class SSI
registers end at CCSR_SSI_SRMSK (no SACC{ST,EN,DIS} regs), so
reading them for cache initialization may not be safe.

Also, a "MXC 91221 only" comment before these regs in FSL tree
(drivers/mxc/ssi/registers.h) seems to confirm that these registers
aren't present at least on some SSI (or SoC) models.


Can't we just mark them as precious or something, so that we don't have 
to have two structures?

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 2/2] ASoC: fsl: select SND_SOC_FSL_SAI or SND_SOC_FSL_SSI depending on SoC type

[Timur Tabi]

Lothar Waßmann wrote:

Why? If more than one of the IMX6 SoCs are selected, both interfaces
may be selected at the same time without any harm.


Oh, ok.  I thought the point behind the patch was that you *souldn't* 
enable the the SSI driver on an i.MX6UL.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 2/2] ASoC: fsl: select SND_SOC_FSL_SAI or SND_SOC_FSL_SSI depending on SoC type

[Timur Tabi]

Lothar Waßmann wrote:

-   select SND_SOC_FSL_SSI
+   select SND_SOC_FSL_SAI if SOC_IMX6UL
+   select SND_SOC_FSL_SSI if SOC_IMX6Q || SOC_IMX6SL || SOC_IMX6SX


I don't think this is compatible with a multiarch kernel.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults

[Timur Tabi]

Mark Brown wrote:

regcache handles this fine, it's perfectly happy to just go and allocate
the cache as registers get used (this is why the code that's doing the
allocation exists...).  What is causing problems here is that the first
access to the register is happening in interrupt context so we can't do
a GFP_KERNEL allocation for it.


Considering how small and not-sparse the SSI register space is, would 
using REGCACHE_FLAT be appropriate?

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults

[Timur Tabi]

Mark Brown wrote:

Quite possibly (it'll be more efficient and it's intended for such use
cases) but as I said in my other reply that then has the issue that it
implicitly gives default values to all the registers so I'd expect we
still need to handle the cache initialisation explicitly (or
alternatively the hardware sync with the cache on startup).


Why does REGCACHE_FLAT assume that all registers have a default value of 
0?  Shouldn't it have the same behavior w.r.t. cache values as 
REGCACHE_RBTREE?

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults

[Timur Tabi]

Mark Brown wrote:

That's possibly problematic because the flat cache will of necessity end
up with defaults (of 0 from the kzalloc()) for all the registers.
You'll still have default values in the cache, though some of the
behaviour around optimising syncs does change without them explicitly
given.  It does deal with the allocation issue but given that the issue
was incorrect defaults I'd be a bit concerned.


Ok, I'm confused.  Granted, all of this regcache stuff was added after I 
stopped working on this driver, so I'm out of the loop.  But it appears 
that the regcache cannot properly handle an uninitialized cache.  I 
would expect it to know to perform hard reads of any registers that are 
uninitialized.


If the regcache wants to have an initialized cache, then it should 
automatically perform reads an all non-volatile, non-precious registers 
at initialization.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 1/3] ASoC: fsl_ssi: mark SACNT register volatile

[Timur Tabi]

Maciej S. Szmigiero wrote:

+   regmap_write(regs, CCSR_SSI_SACNT,
+   ssi_private->regcache_sacnt);


So I'm not familiar with all of the regcache features, but I understand 
this patch.  I was wondering if it makes sense to write the same exact 
value that was read previously.  Isn't it possible for the WR or RD bits 
to change between fsl_ssi_suspend() and fsl_ssi_resume()?  That is, 
should we be doing this instead?


u32 temp;
regmap_read(regs, CCSR_SSI_SACNT, );
temp &= 0x18; // preserve WR and RD
regmap_write(regs, CCSR_SSI_SACNT, (ssi_private->regcache_sacnt & ~0x18) 
| temp);


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 2/3] ASoC: fsl_ssi: mark some registers precious

[Timur Tabi]

Maciej S. Szmigiero wrote:

Mark some registers precious since their
reads have side effects (like clearing flags).

Signed-off-by: Maciej S. Szmigiero<m...@maciej.szmigiero.name>


Acked-by: Timur Tabi <ti...@tabi.org>
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 3/3] ASoC: fsl_ssi: remove register defaults

[Timur Tabi]

Maciej S. Szmigiero wrote:

There is no guarantee that on fsl_ssi module load
SSI registers will have their power-on-reset values.

In fact, if the driver is reloaded the values in
registers will be whatever they were set to previously.

This fixes hard lockup on fsl_ssi module reload,
at least in AC'97 mode.

Fixes: 05cf237972fe ("ASoC: fsl_ssi: Add driver suspend and resume to support MEGA 
Fast")

Signed-off-by: Maciej S. Szmigiero<m...@maciej.szmigiero.name>


Acked-by: Timur Tabi <ti...@tabi.org>

I'm surprised that we're actually encouraging drivers to contain 
hard-coded register values.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [alsa-devel] [PATCH 1/3] ASoC: fsl_ssi: mark SACNT register volatile

[Timur Tabi]

On Sun, Dec 20, 2015 at 2:30 PM, Maciej S. Szmigiero
 wrote:
> SACNT register should be marked volatile since
> its WR and RD bits are cleared by SSI after
> completing the relevant operation.
> This unbreaks AC'97 register access.
>
> Fixes: 05cf237972fe ("ASoC: fsl_ssi: Add driver suspend and resume to support 
> MEGA Fast")
>
> Signed-off-by: Maciej S. Szmigiero 

These patches seem okay, but can we hold off merging them until I get
back from vacation and have a chance to review them?
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [net-next v5 0/8] dpaa_eth: Add the Freescale DPAA Ethernet driver

[Timur Tabi]

On Thu, Dec 3, 2015 at 6:08 AM,  <> wrote:
> From: Madalin Bucur 
>
> This patch series adds the Ethernet driver for the Freescale
> QorIQ Data Path Acceleration Architecture (DPAA).

Please fix your git-send-email configuration, so that your emails are
formatted properly.  This is the From: header:

From: <>

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 1/2] net/fsl_pq_mdio: check TBI address for consistency with mapped range

[Timur Tabi]

Gerlando Falauto wrote:


Change-Id: If1e7d8931f440ea9259726c36d3df797dda016fb


You need to remove these from patches that are emailed, and fix the 
pointer type comparison.


Otherwise,

Acked-by: Timur Tabi <ti...@tabi.org>
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v3 1/3] powerpc/512x: add LocalPlus Bus FIFO device driver

[Timur Tabi]

On 09/30/2015 04:24 PM, Alexander Popov wrote:


Can you test for "!cs" here instead?


+e = -EFAULT;
+goto err_param;
+}


Unfortunately no: 0 is a valid value for Chip Select.
Is it OK to leave it like that?


Yes.


+lpbfifo.ram_bus_addr = sg_dma_address(); /* For freeing later */
+sg_dma_len() = lpbfifo.req->size;


I don't think sg_dma_len() is meant to be used as an lvalue.


I've double-checked and found many cases of such usage of this macro.
It seems that I can't avoid it too.


Ok.


Driver code that has to parse #address-cells or #size-cells
is usually wrong.


I would not call it "parsing", I just check whether the dts-file is good.
Anyway, could you give me a clue how to do better?


You should use of_n_size_cells() and of_n_addr_cells().

--
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v3 2/3] powerpc/512x: add a device tree binding for LocalPlus Bus FIFO

[Timur Tabi]

Alexander Popov wrote:

I've just followed devicetree/bindings/dma/dma.txt...
This "rx-tx" doesn't mean much but it could show that LocalPlus Bus FIFO
uses a single DMA read-write channel. Should I really drop it?


Hmmm, I'm not sure.  Is there anything else (besides your driver) that 
parses this device tree node?


dma.txt says this:

	"The specific strings that can be used are defined in the binding of 
the DMA client device."


So this looks like it's driver-specific, but it is a required property.
I guess you should keep it, but I think you should get a second opinion.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v3 1/3] powerpc/512x: add LocalPlus Bus FIFO device driver

[Timur Tabi]

Alexander Popov wrote:

The only question I have: why calling dma_unmap_single() from within
a spinlock is a bad practice?


I don't know, but usually functions that allocate or free memory cannot 
be called from within a spinlock.  You need to check that.  Since the 
MPC5121 is a single-core CPU, you might not notice if you're doing 
something wrong.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v3 2/3] powerpc/512x: add a device tree binding for LocalPlus Bus FIFO

[Timur Tabi]

Alexander Popov wrote:

+- dma-names: should be "rx-tx";


Why bother, if it can only be one value?
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v3 3/3] dmaengine: mpc512x: initialize with subsys_initcall()

[Timur Tabi]

Alexander Popov wrote:

Initialize Freescale MPC512x DMA driver with subsys_initcall()
to allow the depending drivers to call dma_request_slave_channel()
during their probe.

Signed-off-by: Alexander Popov


Is there any way we can use -EPROBEDEFER instead?
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v3 1/3] powerpc/512x: add LocalPlus Bus FIFO device driver

[Timur Tabi]

Alexander Popov wrote:

+struct mpc512x_lpbfifo_request {
+   phys_addr_t bus_phys;   /* physical address of some device on LPB */


Is this a phys_addr_t or a dma_addr_t?  It can't be both a physical 
address and a bus address.



+   void *ram_virt; /* virtual address of some region in RAM */
+   u32 size;
+   enum lpb_dev_portsize portsize;
+   enum mpc512x_lpbfifo_req_dir dir;
+   void (*callback)(struct mpc512x_lpbfifo_request *);
+};
+
+int mpc512x_lpbfifo_submit(struct mpc512x_lpbfifo_request *req);
+
  #endif /* __ASM_POWERPC_MPC5121_H__ */
diff --git a/arch/powerpc/platforms/512x/Kconfig 
b/arch/powerpc/platforms/512x/Kconfig
index 48bf38d..f09016f 100644
--- a/arch/powerpc/platforms/512x/Kconfig
+++ b/arch/powerpc/platforms/512x/Kconfig
@@ -10,6 +10,12 @@ config PPC_MPC512x
select USB_EHCI_BIG_ENDIAN_MMIO if USB_EHCI_HCD
select USB_EHCI_BIG_ENDIAN_DESC if USB_EHCI_HCD

+config MPC512x_LPBFIFO
+   tristate "MPC512x LocalPlus Bus FIFO driver"
+   depends on PPC_MPC512x && MPC512X_DMA
+   help
+ Enable support for Freescale MPC512x LocalPlus Bus FIFO (SCLPC).
+
  config MPC5121_ADS
bool "Freescale MPC5121E ADS"
depends on PPC_MPC512x
diff --git a/arch/powerpc/platforms/512x/Makefile 
b/arch/powerpc/platforms/512x/Makefile
index 0169312..f47d422 100644
--- a/arch/powerpc/platforms/512x/Makefile
+++ b/arch/powerpc/platforms/512x/Makefile
@@ -5,4 +5,5 @@ obj-$(CONFIG_COMMON_CLK)+= clock-commonclk.o
  obj-y += mpc512x_shared.o
  obj-$(CONFIG_MPC5121_ADS) += mpc5121_ads.o mpc5121_ads_cpld.o
  obj-$(CONFIG_MPC512x_GENERIC) += mpc512x_generic.o
+obj-$(CONFIG_MPC512x_LPBFIFO)  += mpc512x_lpbfifo.o
  obj-$(CONFIG_PDM360NG)+= pdm360ng.o
diff --git a/arch/powerpc/platforms/512x/mpc512x_lpbfifo.c 
b/arch/powerpc/platforms/512x/mpc512x_lpbfifo.c
new file mode 100644
index 000..e6f2c6b
--- /dev/null
+++ b/arch/powerpc/platforms/512x/mpc512x_lpbfifo.c
@@ -0,0 +1,560 @@
+/*
+ * The driver for Freescale MPC512x LocalPlus Bus FIFO
+ * (called SCLPC in the Reference Manual).
+ *
+ * Copyright (C) 2013-2015 Alexander Popov.
+ *
+ * This file is released under the GPLv2.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define DRV_NAME "mpc512x_lpbfifo"
+
+struct cs_range {
+   u32 csnum;
+   u32 base; /* must be zero */
+   u32 addr;
+   u32 size;
+};
+
+static struct lpbfifo_data {
+   spinlock_t lock; /* for protecting lpbfifo_data */
+   phys_addr_t regs_phys;
+   resource_size_t regs_size;
+   struct mpc512x_lpbfifo __iomem *regs;
+   int irq;
+   struct cs_range *cs_ranges;
+   size_t cs_n;
+   struct dma_chan *chan;
+   struct mpc512x_lpbfifo_request *req;
+   dma_addr_t ram_bus_addr;
+   bool wait_lpbfifo_irq;
+   bool wait_lpbfifo_callback;
+} lpbfifo;
+
+/*
+ * A data transfer from RAM to some device on LPB is finished
+ * when both mpc512x_lpbfifo_irq() and mpc512x_lpbfifo_callback()
+ * have been called. We execute the callback registered in
+ * mpc512x_lpbfifo_request just after that.
+ * But for a data transfer from some device on LPB to RAM we don't enable
+ * LPBFIFO interrupt because clearing MPC512X_SCLPC_SUCCESS interrupt flag
+ * automatically disables LPBFIFO reading request to the DMA controller
+ * and the data transfer hangs. So the callback registered in
+ * mpc512x_lpbfifo_request is executed at the end of 
mpc512x_lpbfifo_callback().
+ */
+
+/*
+ * mpc512x_lpbfifo_irq - IRQ handler for LPB FIFO
+ */
+static irqreturn_t mpc512x_lpbfifo_irq(int irq, void *param)
+{
+   struct device *d = (struct device *)param;


Please use the variable name 'dev' instead of 'd', for consistency.


+   struct mpc512x_lpbfifo_request *req = lpbfifo.req;
+   unsigned long flags;
+   u32 status;
+
+   spin_lock_irqsave(, flags);
+
+   if (!lpbfifo.regs)
+   goto end0;
+
+   if (!req || req->dir == MPC512X_LPBFIFO_REQ_DIR_READ) {
+   dev_err(d, "bogus LPBFIFO IRQ\n");
+   goto end0;
+   }


So this might be an obscure bug.  The code says that "req = lpbfifo.req" 
above.  However, it doesn't know that this block is in a critical 
section, so it will initialize 'req' not on the top line of the 
function, but rather right here.  That means that although you think 
that you're initializing 'req' before the spin_lock_irqsave(), the code 
might actually initialize it after the spin_lock_irqsave().


My suggestion is that you explicitly initialize 'req' after 
spin_lock_irqsave().  Or better yet, don't use 'req' and explicitly 
reference lpbfifo.req every time.



+
+   status = in_be32(>status);
+   if (status != MPC512X_SCLPC_SUCCESS) {
+   dev_err(d, "DMA transfer between RAM and peripheral 

Re: [PATCH v2] video: fbdev: fsl: Fix the sleep function for FSL DIU module

[Timur Tabi]

Wang Dongsheng wrote:

Thanks Timur.

@Scott,
Could you apply this patch?


You need to ask the fbdev maintainer to apply it, because it has to go 
through his tree.


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2] video: fbdev: fsl: Fix the sleep function for FSL DIU module

[Timur Tabi]

Dongsheng Wang wrote:

For deep sleep, the diu module will power off, when wake up
from the deep sleep, the registers need to be reinitialized.

Signed-off-by: Jason Jinjason@freescale.com
Signed-off-by: Wang Dongshengdongsheng.w...@freescale.com


Acked-by: Timur Tabi ti...@tabi.org

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [alsa-devel][PATCH 2/2] ASoC: fsl_ssi: sound is wrong after suspend/resume

[Timur Tabi]

On Jul 6, 2015, at 4:49 AM, Zidan Wang wrote:

 The register SFCSR is volatile, but some bits in it need to be
 recovered after suspend/resume.
 
 Signed-off-by: Zidan Wang zidan.w...@freescale.com

Shouldn't this be part of patch #1?
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [alsa-devel][PATCH 1/2] ASoC: fsl_ssi: Add driver suspend and resume to support MEGA Fast

[Timur Tabi]

On Jul 6, 2015, at 4:49 AM, Zidan Wang wrote:

 +static bool fsl_ssi_readable_reg(struct device *dev, unsigned int reg)
 +{
 + switch (reg) {
 + case CCSR_SSI_STX0:
 + case CCSR_SSI_STX1:
 + case CCSR_SSI_SRX0:
 + case CCSR_SSI_SRX1:
 + case CCSR_SSI_SCR:
 + case CCSR_SSI_SISR:
 + case CCSR_SSI_SIER:
 + case CCSR_SSI_STCR:
 + case CCSR_SSI_SRCR:
 + case CCSR_SSI_STCCR:
 + case CCSR_SSI_SRCCR:
 + case CCSR_SSI_SFCSR:
 + case CCSR_SSI_STR:
 + case CCSR_SSI_SOR:
 + case CCSR_SSI_SACNT:
 + case CCSR_SSI_SACADD:
 + case CCSR_SSI_SACDAT:
 + case CCSR_SSI_SATAG:
 + case CCSR_SSI_STMSK:
 + case CCSR_SSI_SRMSK:
 + case CCSR_SSI_SACCST:
 + case CCSR_SSI_SACCEN:
 + case CCSR_SSI_SACCDIS:
 + return true;
 + default:
 + return false;
 + }
 +}

This should be the other way around: return true by default, and false it is 
one of the few registers that is not readable.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] ASoC: fsl_ssi: fix AC'97 mode

[Timur Tabi]

Maciej S. Szmigiero wrote:

If there isn't going to be new platforms added with old bindings then this 
won't be needed - I'll remove it.


I would love it if someone would port those original device trees to the 
new binding, so that we can get rid of the old one.  But we should 
definitely not allow new device trees with the old binding.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 3/6] ASoC: fsl_ssi: enable AC'97 asymmetric rates

[Timur Tabi]

Maciej S. Szmigiero wrote:

/* Are the RX and the TX clocks locked? */
if (!of_find_property(np, fsl,ssi-asynchronous, NULL)) {
-   ssi_private-cpu_dai_drv.symmetric_rates = 1;
+   if (!fsl_ssi_is_ac97(ssi_private))
+   ssi_private-cpu_dai_drv.symmetric_rates = 1;
+


Is this necessary?  Why not just add fsl,ssi-asynchronous to the AC97 
device tree node?

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] ASoC: fsl_ssi: fix AC'97 mode

[Timur Tabi]

Maciej S. Szmigiero wrote:

+   if (newbinding  fsl_ssi_is_ac97(ssi_private)) {


Is the newbinding necessary?  I thought only the original PowerPC 
device trees were the only one that have the old binding, and they never 
supported AC97.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] ASoC: fsl: Add dedicated DMA buffer size for each cpu dai

[Timur Tabi]

Nicolin Chen wrote:

As the ssi is not the only cpu dai, there are esai, spdif, sai.
and imx_pcm_dma can be used by all of them. Especially ESAI need
a larger DMA buffer size. So Add dedicated DMA buffer for each cpu
dai.

Signed-off-by: Shengjiu Wangshengjiu.w...@freescale.com

Acked-by: Nicolin Chennicoleots...@gmail.com


Acked-by: Timur Tabi ti...@tabi.org

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: new way of writing defconfigs for freescale's powerpc platforms

[Timur Tabi]

On 04/21/2015 12:55 PM, Scott Wood wrote:


Ok, then define a new Kconfig option that merge_config.sh will look for.
   So in p1_defconfig, there will be this line:

CONFIG_OTHER_DEFCONFIGS=fsl_basic_config

If you want to do that go ahead.  In the meantime we'll use the
mechanism that already exists.


Dude, you are not making any sense.  If there is a mechanism that 
already exists, then what are we talking about?


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: new way of writing defconfigs for freescale's powerpc platforms

[Timur Tabi]

Scott Wood wrote:

We want single-name config targets to still work from the user's
perspective, but we want to reduce the (often imperfect) duplication
under the hood.


Ok, then define a new Kconfig option that merge_config.sh will look for. 
 So in p1_defconfig, there will be this line:


CONFIG_OTHER_DEFCONFIGS=fsl_basic_config

--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: new way of writing defconfigs for freescale's powerpc platforms

[Timur Tabi]

Scott Wood wrote:


Why do you need a powerpc-specific way to use merge_config.sh?  Other
architectures have the same problem with defconfigs.



What are you perceiving as powerpc-specific about what we're
proposing?


Well, there's the subject of this thread, which is new way of writing 
defconfigs for freescale's powerpc platforms.


 Are you complaining about the actual content of which

fragments to use to produce which defconfigs going in arch/powerpc?


No, I'm just trying to figure out what's powerpc-specific about Lijun's 
proposal.



Besides, wouldn't it make more sense to define a new defconfig type,
like p1_defconfig.merge, and if you do make p1_defconfig.merge it
knows to call merge_config.sh?



That's already there.  make foo.config.


Ok, so I'm definitely confused now.  I have no idea what's actually 
being proposed, since apparently the ability to have merge configs 
already exists.


Wouldn't it just be simpler to pass multiple defconfigs to 'make', and 
then 'make' will know to call merge_config.sh on them?  So instead of


make ./scripts/kconfig/merge_config.sh 
arch/powerpc/configs/fsl_basic_config p1_defconfig

make

we can just do

make fsl_basic_config p1_defconfig
make


--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: new way of writing defconfigs for freescale's powerpc platforms

[Timur Tabi]

On Mon, Apr 20, 2015 at 3:31 PM, Scott Wood scottw...@freescale.com wrote:


 The ability to merge configs is already there.  We're just talking about
 using that functionality.

Why do you need a powerpc-specific way to use merge_config.sh?  Other
architectures have the same problem with defconfigs.

Besides, wouldn't it make more sense to define a new defconfig type,
like p1_defconfig.merge, and if you do make p1_defconfig.merge it
knows to call merge_config.sh?

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: new way of writing defconfigs for freescale's powerpc platforms

[Timur Tabi]

On Fri, Apr 17, 2015 at 11:53 PM, Lijun Pan lijun@freescale.com wrote:

 Have just sent out a patch considering the previous discussion.
 http://patchwork.ozlabs.org/patch/462249/
 [PATCH] powerpc/defconfig: new way of writing defconfig

The ability to merge defconfigs is a feature that's been requested by
many people for a long time.  Any solution should definitely NOT be
PowerPC-only.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: free irq before irq_dispose_mapping()

[Timur Tabi]

On 12/01/2014 10:49 AM, Lars-Peter Clausen wrote:


The driver creates the mapping by calling irq_of_parse_and_map(), so it
also has to dispose the mapping. But the easy way out is to simply use
platform_get_irq() instead of irq_of_parse_map(). In this case the
mapping is not managed by the device but by the of core, so the device
has not to dispose the mapping.


Is this a problem unique to the SSI driver?  Maybe devm_free_irq() 
should also dispose of the mapping?


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: free irq before irq_dispose_mapping()

[Timur Tabi]

On 12/01/2014 10:49 AM, Lars-Peter Clausen wrote:

The driver creates the mapping by calling irq_of_parse_and_map(), so it
also has to dispose the mapping.


I agree with Markus, this does seem weird.  It sounds like you're saying 
that irq_of_parse_and_map() and devm_request_irq() are incompatible.  A 
quick grep shows the following drivers that call both functions:


ata/pata_mpc52xx.c
built-in.o
cpufreq/exynos5440-cpufreq.c
crypto/omap-sham.c
dma/moxart-dma.c
edac/mpc85xx_edac.c
hsi/clients/nokia-modem.c
i2c/busses/i2c-wmt.c
input/serio/apbps2.c
mmc/host/omap_hsmmc.c
mmc/host/moxart-mmc.c
mtd/nand/mpc5121_nfc.c
net/ethernet/arc/emac_main.c
net/ethernet/moxa/moxart_ether.c
pci/host/pcie-rcar.c
pinctrl/samsung/pinctrl-exynos5440.c
pinctrl/samsung/pinctrl-exynos.c
pinctrl/pinctrl-bcm2835.c
spi/spi-bcm2835.c
spi/spi-mpc512x-psc.c
staging/xillybus/xillybus_of.c
thermal/samsung/exynos_tmu.c

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: free irq before irq_dispose_mapping()

[Timur Tabi]

On 12/01/2014 01:56 PM, Arnd Bergmann wrote:

All other drivers that call irq_of_parse_and_map and pass that into
devm_request_irq just never unmap, and their interrupts are already
mapped by the platform code, so I think it's not even a leak.


Does this mean that fsl_ssi.c should not be calling 
irq_of_parse_and_map?  How else should it get the IRQ?

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: free irq before irq_dispose_mapping()

[Timur Tabi]

On 12/01/2014 02:01 PM, Arnd Bergmann wrote:

Does this mean that fsl_ssi.c should not be calling
irq_of_parse_and_map?  How else should it get the IRQ?

platform_get_irq()


Ok, but that function also calls irq_create_of_mapping().  So it still 
appears that the only way to get the IRQ is to map it, but then we can't 
use devm_request_irq().


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: free irq before irq_dispose_mapping()

[Timur Tabi]

On 12/01/2014 02:40 PM, Fabio Estevam wrote:

Hm... that's new. But it's not really a driver issue anymore if it is done
in the core. So I guess for now just use platform_get_irq() and ignore the
other issue.



With the suggested changes below, the removal of the driver works fine on a mx6:


Would the mapping continue to exist after the driver is unloaded?  Can 
you try multiple loads/unloads and see if interrupts still work?

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 1/3] qe-uart: modify qe-uart to adapt both powerpc and arm

[Timur Tabi]

On Fri, Oct 10, 2014 at 1:05 PM, Scott Wood scottw...@freescale.com wrote:
 There are many changes in here that ought to be separate patches with
 separate justification.

 Also, some of the QE changes seem to be reasonable cleanup, but not
 related to making the code work on ARM.

I agree with Scott.  This patch already makes significant code
changes, so you should have one patch that just makes the
out_be32-iowrite32be changes.  Changes to the QE library should NOT
be in the same patch as changes to ucc_uart.c.

In addition, changes like this:

-   iprop = of_get_property(np, port-number, NULL);
-   if (!iprop) {
+   ret = of_property_read_u32_index(np, port-number, 0, val);
+   if (ret) {

should be changed to remove the OF dependency.  If you're going to
replace of_get_property, replace it with device_property_read_u32(),
to remove the OF dependency.

 diff --git a/arch/arm/include/asm/delay.h b/arch/arm/include/asm/delay.h
 index dff714d..a932f99 100644
 --- a/arch/arm/include/asm/delay.h
 +++ b/arch/arm/include/asm/delay.h
 @@ -57,6 +57,22 @@ extern void __bad_udelay(void);
   __const_udelay((n) * UDELAY_MULT)) :\
 __udelay(n))

 +#define spin_event_timeout(condition, timeout, delay)   
\
 +({  
\
 + typeof(condition) __ret;   
 \
 + int i = 0; 
 \
 + while (!(__ret = (condition))  (i++  timeout)) {
 \
 + if (delay) 
 \
 + udelay(delay); 
 \
 + else   
 \
 + cpu_relax();   
 \
 + udelay(1); 
 \
 + }  
 \

 This will delay too long if delay is used.

Shouldn't ARM have a version of tb_ticks_since() by now?

 + if (!__ret)
 \
 + __ret = (condition);   
 \
 + __ret; 
 \

 Timur, do you remember why that final if (!__ret) __ret = (condition);
 is needed?

powerpc: Fix spin_event_timeout() to be robust over context switches

Current implementation of spin_event_timeout can be interrupted by an
IRQ or context switch after testing the condition, but before checking
the timeout.  This can cause the loop to report a timeout when the
condition actually became true in the middle.

This patch adds one final check of the condition upon exit of the loop
if the last test of the condition was still false.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH V2] ASoC: fsl_ssi: refine ipg clock usage in this module

[Timur Tabi]

Shengjiu Wang wrote:

+   ret = clk_prepare_enable(ssi_private-clk);
+   if (ret)
+   return ret;


Will this work on PowerPC, where ssi_private-clk is always NULL?
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH V1] ASoC: fsl_ssi: refine ipg clock usage in this module

[Timur Tabi]

Shengjiu Wang wrote:

+   if (ssi_private-soc-imx)
+   clk_prepare_enable(ssi_private-clk);


How about this instead?

if (ssi_private-clk)
clk_prepare_enable(ssi_private-clk);

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH V1] ASoC: fsl_ssi: refine ipg clock usage in this module

[Timur Tabi]

On 09/09/2014 10:21 AM, Mark Brown wrote:

if (ssi_private-clk)
clk_prepare_enable(ssi_private-clk);



Should be a !IS_ERR() - NULL is a valid clock.


In that case, ssi_private-clk needs to be initialized to -EINVAL or 
something, so that the check works on systems that don't have any clocks.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH V1] ASoC: fsl_ssi: refine ipg clock usage in this module

[Timur Tabi]

On 09/09/2014 01:38 PM, Nicolin Chen wrote:

make sure to have the call for imx only because it seems that
the other platforms do not depend on the clock.


Although I doubt anyone will every add support for clocks to PowerPC 
side of this driver, I would prefer to avoid IMX-specific changes. 
Instead, the code should check if a clock is available.  That's why I 
suggested this change:


-   if (ssi_private-soc-imx)
+   if (!IS_ERR(ssi_private-clk))
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH V1] ASoC: fsl_ssi: refine ipg clock usage in this module

[Timur Tabi]

On 09/09/2014 02:59 PM, Nicolin Chen wrote:

+   /*
+* Initially mark the clock to NULL for all platforms so that later
+* clk_prepare_enable() will ignore and return 0 for non-clock cases.
+*/
+   ssi_private-clk = NULL;


According to Mark, NULL is a valid clock, so this should be instead:

ssi_private-clk = PTR_ERR(-EINVAL);

although that doesn't sit well with me.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH V1] ASoC: fsl_ssi: refine ipg clock usage in this module

[Timur Tabi]

On 09/09/2014 03:27 PM, Nicolin Chen wrote:

I guess Mark's comment is merely against the check for clk validation
because if talking about clk validation, we should check IS_ERR(clk)
rather than check !=NULL directly.


Ah, that makes sense now.


However, my approach doesn't need any check. The open() or pm_resume()
can just call clk_prepare_enable() directly. The __clk_enable() will
then handle the 'clk == NULL' case:


Yes, I was thinking the same thing.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: qe: move qe from arch/powerpc to drivers

[Timur Tabi]

On Wed, Aug 6, 2014 at 3:53 AM, qiang.z...@freescale.com
qiang.z...@freescale.com wrote:

 Actually qe is a kind of IP block, so in my opinion, it is proper to put it 
 under driver/(just in my opinion).

The QE library is not a driver, however.  It doesn't register as a
driver with the kernel.

Scott suggests drivers/soc.  I'm not crazy about that, but I don't
maintain that code any more.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: qe: move qe from arch/powerpc to drivers

[Timur Tabi]

On 08/07/2014 03:08 PM, Scott Wood wrote:

Scott suggests drivers/soc.  I'm not crazy about that, but I don't
maintain that code any more.



Do you have a better suggestion?


Leave it where it is?
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: qe: move qe from arch/powerpc to drivers

[Timur Tabi]

On 08/07/2014 03:11 PM, Scott Wood wrote:

 Do you have a better suggestion?


Leave it where it is?



We need it on ARM as well.


In that case, drivers/soc is the least-bad option.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 4/4] ASoC: fsl_ssi: Add stream names for DPCM usage

[Timur Tabi]

Nicolin Chen wrote:

DPCM needs extra dapm routes in the machine driver to route audio
between Front-End and Back-End. In order to differ the stream names
in the route map from CODECs, we here add specific stream names to
SSI driver so that we can implement ASRC via DPCM to it.

Signed-off-by: Nicolin Chennicoleots...@gmail.com


Acked-by: Timur Tabi ti...@tabi.org
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [alsa-devel] [RFC][PATCH v1] ASoC: fsl_ssi: Add DAI master mode support for SSI on i.MX series

[Timur Tabi]

On Thu, Dec 12, 2013 at 4:44 AM, Nicolin Chen
guangyu.c...@freescale.com wrote:

 +static int fsl_ssi_set_dai_sysclk(struct snd_soc_dai *cpu_dai,
 + int clk_id, unsigned int freq, int dir)
 +{
 +   struct fsl_ssi_private *ssi_private = 
 snd_soc_dai_get_drvdata(cpu_dai);
 +   struct ccsr_ssi __iomem *ssi = ssi_private-ssi;
 +   int synchronous = ssi_private-cpu_dai_drv.symmetric_rates, ret;
 +   u32 pm = 999, div2, psr, stccr, mask, afreq, factor, i;
 +   unsigned long flags, clkrate, baudrate, tmprate;
 +   u64 sub, savesub = 10;
 +
 +   /* Don't apply it to any non-baudclk circumstance */
 +   if (IS_ERR(ssi_private-baudclk))
 +   return -EINVAL;
 +
 +   /* It should be already enough to divide clock by setting pm alone */
 +   psr = 0;
 +   div2 = 0;
 +
 +   factor = (div2 + 1) * (7 * psr + 1) * 2;
 +
 +   for (i = 0; i  255; i++) {
 +   /* The bclk rate must be smaller than 1/5 sysclk rate */
 +   if (factor * (i + 1)  5)
 +   continue;
 +
 +   tmprate = freq * factor * (i + 2);
 +   clkrate = clk_round_rate(ssi_private-baudclk, tmprate);
 +
 +   do_div(clkrate, factor);

This do_div() call causes this warning on PowerPC:

  CC  sound/soc/fsl/fsl_ssi.o
sound/soc/fsl/fsl_ssi.c: In function 'fsl_ssi_set_bclk':
sound/soc/fsl/fsl_ssi.c:593:3: warning: comparison of distinct pointer
types lacks a cast
sound/soc/fsl/fsl_ssi.c:593:3: warning: right shift count = width of type
sound/soc/fsl/fsl_ssi.c:593:3: warning: passing argument 1 of
'__div64_32' from incompatible pointer type
include/asm-generic/div64.h:35:17: note: expected 'uint64_t *' but
argument is of type 'long unsigned int *'

The comments in do_div() say that clkrate should be 64-bits.  Changing
clkrate to a u64 does fix this problem, but I'm wondering if anyone
else has seen this.  This code has been around for months.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev