Re: [PATCH v2 00/10] soundwire: intel: add multi-link support
On 01-09-20, 23:05, Bard Liao wrote: > This series enables multi-link support for Intel platforms. Applied all, thanks > > Changes in v2: > - Remove the "soundwire: intel: Only call sdw stream APIs for the first > cpu_dai" patch form this series. We will move the sounwdire stream > operations to machine driver in another series. > - Update the commit message and title of "soundwire: intel: add error log > for clock-stop invalid configs" > > Pierre-Louis Bossart (10): > soundwire: intel: disable shim wake on suspend > soundwire: intel: ignore software command retries > soundwire: intel: add multi-link support > soundwire: intel: add missing support for all clock stop modes > soundwire: bus: update multi-link definition with hw sync details > soundwire: intel: add multi-link hw_synchronization information > soundwire: stream: enable hw_sync as needed by hardware > soundwire: intel: add error log for clock-stop invalid configs > soundwire: intel: pass link_mask information to each master > soundwire: intel: don't manage link power individually > > drivers/soundwire/intel.c | 264 - > drivers/soundwire/intel.h | 2 + > drivers/soundwire/intel_init.c | 1 + > drivers/soundwire/stream.c | 15 +- > include/linux/soundwire/sdw.h | 6 + > 5 files changed, 243 insertions(+), 45 deletions(-) > > -- > 2.17.1 -- ~Vinod
Re: [PATCH 0/7] ASoC: soundwire: Move sdw stream operations to
On 01-09-20, 23:02, Bard Liao wrote: > sdw stream operation APIs can be called once per stream. dailink > callbacks are good places to call these APIs. Again, please mention here if this is to be merged thru sdw tree or ASoC tree > > Pierre-Louis Bossart (7): > ASoC: soc-dai: clarify return value for get_sdw_stream() > soundwire: stream: fix NULL/IS_ERR confusion > soundwire: intel: fix NULL/ERR_PTR confusion > ASOC: Intel: sof_sdw: add dailink .trigger callback > ASOC: Intel: sof_sdw: add dailink .prepare and .hw_free callback These should be ASoC > soundwire: intel: remove .trigger operation > soundwire: intel: remove stream handling from .prepare and .hw_free > > drivers/soundwire/intel.c| 60 --- > drivers/soundwire/stream.c | 2 +- > include/sound/soc-dai.h | 3 +- > sound/soc/intel/boards/sof_sdw.c | 81 > 4 files changed, 92 insertions(+), 54 deletions(-) > > -- > 2.17.1 -- ~Vinod
Re: [PATCH v4 0/3] ASoC: soundwire: fix port_ready[] dynamic allocation
On 31-08-20, 21:43, Bard Liao wrote: > The existing code allocates memory for the total number of ports. > This only works if the ports are contiguous, but will break if e.g. a > Devices uses port0, 1, and 14. The port_ready[] array would contain 3 > elements, which would lead to an out-of-bounds access. Conversely in > other cases, the wrong port index would be used leading to timeouts on > prepare. > > This can be fixed by allocating for the worst-case of 15 > ports (DP0..DP14). In addition since the number is now fixed, we can > use an array instead of a dynamic allocation. Applied all, thanks -- ~Vinod
Re: [PATCH v2] soundwire: fix double free of dangling pointer
On 02-09-20, 13:26, t...@redhat.com wrote:
> From: Tom Rix
>
> clang static analysis flags this problem
>
> stream.c:844:9: warning: Use of memory after
> it is freed
> kfree(bus->defer_msg.msg->buf);
> ^~~
>
> This happens in an error handler cleaning up memory
> allocated for elements in a list.
>
> list_for_each_entry(m_rt, >master_list, stream_node) {
> bus = m_rt->bus;
>
> kfree(bus->defer_msg.msg->buf);
> kfree(bus->defer_msg.msg);
> }
>
> And is triggered when the call to sdw_bank_switch() fails.
> There are a two problems.
>
> First, when sdw_bank_switch() fails, though it frees memory it
> does not clear bus's reference 'defer_msg.msg' to that memory.
>
> The second problem is the freeing msg->buf. In some cases
> msg will be NULL so this will dereference a null pointer.
> Need to check before freeing.
Applied, thanks
--
~Vinod
Re: [RESEND PATCH v2] dt-bindings: renesas,rcar-dmac: Document r8a7742 support
On 03-09-20, 08:38, Lad Prabhakar wrote: > Renesas RZ/G SoC also have the R-Car gen2/3 compatible DMA controllers. > Document RZ/G1H (also known as R8A7742) SoC bindings. Applied, thanks -- ~Vinod
Re: [PATCH] dmaengine: ti: k3-udma: Update rchan_oes_offset for am654 SYSFW ABI 3.0
On 31-08-20, 12:10, Peter Ujfalusi wrote: > SYSFW ABI 3.0 has changed the rchan_oes_offset value for am654 to support > SR2. > > Since the kernel now needs SYSFW API 3.0 to work because the merged irqchip > update, we need to also update the am654 rchan_oes_offset. Applied to fixes, thanks -- ~Vinod
Re: [PATCH v2 05/10] dt-bindings: renesas,rcar-dmac: Document r8a7742 support
On 27-08-20, 12:08, Lad, Prabhakar wrote: > Hi Vinod, > > On Sun, May 3, 2020 at 10:47 PM Lad Prabhakar > wrote: > > > > Renesas RZ/G SoC also have the R-Car gen2/3 compatible DMA controllers. > > Document RZ/G1H (also known as R8A7742) SoC bindings. > > > > Signed-off-by: Lad Prabhakar > > Reviewed-by: Marian-Cristian Rotariu > > > > Reviewed-by: Geert Uytterhoeven > > --- > > Documentation/devicetree/bindings/dma/renesas,rcar-dmac.yaml | 1 + > > 1 file changed, 1 insertion(+) > > > This patch is not present in linux-next yet, could you please take care of it. Can you please resend -- ~Vinod
Re: [PATCH 1/3] dmaengine: pl330: Simplify with dev_err_probe()
On 28-08-20, 17:26, Krzysztof Kozlowski wrote: > Common pattern of handling deferred probe can be simplified with > dev_err_probe(). Less code and the error value gets printed. Applied all, thanks -- ~Vinod
Re: [PATCH] dmaengine: Mark dma_request_slave_channel() deprecated
On 28-08-20, 14:05, Peter Ujfalusi wrote: > New drivers should use dma_request_chan() instead > dma_request_slave_channel() > > dma_request_slave_channel() is a simple wrapper for dma_request_chan() > eating up the error code for channel request failure and makes deferred > probing impossible. > > Move the dma_request_slave_channel() into the header as inline function, > mark it as deprecated. Applied, thanks -- ~Vinod
Re: [PATCH 09/11] soundwire: intel: add dynamic debug trace for clock-stop invalid configs
On 31-08-20, 10:15, Pierre-Louis Bossart wrote: > > > > > > > > Detect cases where the clock is assumed to be stopped but the IP > > > > > > > is > > > > > > > not in the relevant state, and add a dynamic debug trace. > > > > > > > > > > > > you meant a debug print..and it looks like error print below (also > > > > > > in title). > > > > > > > > > > I don't understand the comment. Is the 'trace' confusing and are you > > > > > asking > > > > > to e.g. change the commit message to 'add dynamic debug log'? > > > > > > > > Question is what is dynamic about this? > > > dev_dbg() is part of the kernel dynamic debug capability... > > > > > > https://www.kernel.org/doc/html/latest/admin-guide/dynamic-debug-howto.html > > > > > > Not sure what you are asking here? > > > > :-| where is dev_dbg() ? > > > > See [1] > > > > > [1] > > > > > + dev_err(dev, "%s invalid configuration, clock was not > > > stopped", __func__); > > > > ^^^ > > it's still a log using the "dynamic debug" framework. > > Again, what are you asking us to fix? Ah you are really testing my patience! The title says "dynamic debug" and then you use a dev_err which is *not* part of dynamic debug as it is printed always and cannot be dynamically enabled and disabled! See Documentation/admin-guide/dynamic-debug-howto.rst: "Dynamic debug is designed to allow you to dynamically enable/disable kernel code to obtain additional kernel information. Currently, if ``CONFIG_DYNAMIC_DEBUG`` is set, then all ``pr_debug()``/``dev_dbg()`` and ``print_hex_dump_debug()``/``print_hex_dump_bytes()`` calls can be dynamically enabled per-callsite." No dev_err here! -- ~Vinod
Re: [PATCH] regmap: soundwire: remove unsed header mod_devicetable.h
On 31-08-20, 09:12, Pierre-Louis Bossart wrote: > typo in commit message? Thanks for spotting, will send v2. > > On 8/29/20 5:39 AM, Vinod Koul wrote: > > mod_devicetable.h does not seem to be required for this file, so > > remove it. > > > > Signed-off-by: Vinod Koul > > --- > > drivers/base/regmap/regmap-sdw.c | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/drivers/base/regmap/regmap-sdw.c > > b/drivers/base/regmap/regmap-sdw.c > > index 50a66382d87d..c92d614b4943 100644 > > --- a/drivers/base/regmap/regmap-sdw.c > > +++ b/drivers/base/regmap/regmap-sdw.c > > @@ -2,7 +2,6 @@ > > // Copyright(c) 2015-17 Intel Corporation. > > #include > > -#include > > #include > > #include > > #include "internal.h" > > -- ~Vinod
[PATCH v2] regmap: soundwire: remove unused header mod_devicetable.h
mod_devicetable.h does not seem to be required for this file, so remove it. Signed-off-by: Vinod Koul --- changes in v2: - fix typo in patch subject drivers/base/regmap/regmap-sdw.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/base/regmap/regmap-sdw.c b/drivers/base/regmap/regmap-sdw.c index 50a66382d87d..c92d614b4943 100644 --- a/drivers/base/regmap/regmap-sdw.c +++ b/drivers/base/regmap/regmap-sdw.c @@ -2,7 +2,6 @@ // Copyright(c) 2015-17 Intel Corporation. #include -#include #include #include #include "internal.h" -- 2.26.2
Re: [PATCH] soundwire: fix error handling
Hello Tom,
On 29-08-20, 08:35, t...@redhat.com wrote:
> From: Tom Rix
>
> clang static analysis flags this problem
>
> stream.c:844:9: warning: Use of memory after
> it is freed
> kfree(bus->defer_msg.msg->buf);
> ^~~
>
> This happens in an error handler cleaning up memory
> allocated for elements in a list.
>
> list_for_each_entry(m_rt, >master_list, stream_node) {
> bus = m_rt->bus;
>
> kfree(bus->defer_msg.msg->buf);
> kfree(bus->defer_msg.msg);
> }
>
> And is triggered when the call to sdw_bank_switch() fails.
> There are a two problems.
>
> First, when sdw_bank_switch() fails, though it frees memory it
> does not clear bus's reference 'defer_msg.msg' to that memory.
>
> The second problem is the freeing msg->buf. In some cases
> msg will be NULL so this will dereference a null pointer.
> Need to check before freeing.
The change looks good to me, but the title of patch should be revised.
The patch subject should describe the patch, in this case is setting
pointer to null on cleanup, so an appropriate subject may be"
"[PATCH]: soundwire: set defer_msg to null".
Please revise subject line and update including the ack/reviews
received
Thanks
>
> Fixes: 99b8a5d608a6 ("soundwire: Add bank switch routine")
> Signed-off-by: Tom Rix
> ---
> drivers/soundwire/stream.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index 37290a799023..6e36deb505b1 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -717,6 +717,7 @@ static int sdw_bank_switch(struct sdw_bus *bus, int
> m_rt_count)
> kfree(wbuf);
> error_1:
> kfree(wr_msg);
> + bus->defer_msg.msg = NULL;
> return ret;
> }
>
> @@ -840,9 +841,10 @@ static int do_bank_switch(struct sdw_stream_runtime
> *stream)
> error:
> list_for_each_entry(m_rt, >master_list, stream_node) {
> bus = m_rt->bus;
> -
> - kfree(bus->defer_msg.msg->buf);
> - kfree(bus->defer_msg.msg);
> + if (bus->defer_msg.msg) {
> + kfree(bus->defer_msg.msg->buf);
> + kfree(bus->defer_msg.msg);
> + }
> }
>
> msg_unlock:
> --
> 2.18.1
--
~Vinod
Re: [PATCH v7 2/3] dt-bindings: phy: intel: Add Keem Bay eMMC PHY bindings
On 01-09-20, 04:58, Wan Mohamad, Wan Ahmad Zainie wrote: > > > @@ -0,0 +1,44 @@ > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2 > > > +--- > > > +$id: "http://devicetree.org/schemas/phy/intel,keembay-emmc- > > phy.yaml#" > > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#; > > > + > > > +title: Intel Keem Bay eMMC PHY bindings > > > > This seems same as > > Documentation/devicetree/bindings/phy/intel,lgm-emmc-phy.yaml, why > > not add a new compatible in lgm binding, or did I miss a difference? > > AFAIK, LGM make use of syscon node, whilst KMB does not. > And LGM and KMB belongs to different SoC family. So, I prefer them to > be in separate file. > > Having said that, with few changes in wordings in title and description, > I think we can make it generic and can be used across few products. The bindings seems quite similar. We can have two drivers loaded using two compatible but binding description can be made same -- ~Vinod
Re: [PATCH] phy: Move phy-rockchip-dphy-rx0 out of staging
On 25-08-20, 19:07, Ezequiel Garcia wrote: > There is no need for this driver to be in staging. > Let's promote it! Applied, thanks -- ~Vinod
Re: [RESEND PATCH v4 0/2] Add new UniPhier AHCI PHY driver
On 25-08-20, 19:41, Kunihiko Hayashi wrote: > This series adds support for AHCI PHY interface implemented in Socionext > UniPhier SoCs. This driver supports PXs2 and PXs3 SoCs. Applied both, thanks -- ~Vinod
Re: [PATCH v4 1/1] phy: tusb1210: use bitmasks to set VENDOR_SPECIFIC2
Hi Liam,
On 22-08-20, 16:53, Liam Beguin wrote:
> From: Liam Beguin
>
> Start by reading the content of the VENDOR_SPECIFIC2 register and update
> each bit field based on device properties when defined.
>
> The use of bit masks prevents fields from overriding each other and
> enables users to clear bits which are set by default, like datapolarity
> in this instance.
>
> Signed-off-by: Liam Beguin
> ---
> Changes since v1:
> - use set_mask_bits
>
> Changes since v2:
> - fix missing bit shift dropped in v2
> - rebase on 5.9-rc1
>
> Changes since v3:
> - switch to u8p_replace_bits() since there's little reason to protect
> against concurrent access
>
> drivers/phy/ti/phy-tusb1210.c | 27 ++-
> 1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/phy/ti/phy-tusb1210.c b/drivers/phy/ti/phy-tusb1210.c
> index d8d0cc11d187..a63213f5972a 100644
> --- a/drivers/phy/ti/phy-tusb1210.c
> +++ b/drivers/phy/ti/phy-tusb1210.c
> @@ -7,15 +7,16 @@
> * Author: Heikki Krogerus
> */
> #include
> +#include
> #include
> #include
> #include
> #include
>
> #define TUSB1210_VENDOR_SPECIFIC20x80
> -#define TUSB1210_VENDOR_SPECIFIC2_IHSTX_SHIFT0
> -#define TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_SHIFT 4
> -#define TUSB1210_VENDOR_SPECIFIC2_DP_SHIFT 6
> +#define TUSB1210_VENDOR_SPECIFIC2_IHSTX_MASK GENMASK(3, 0)
> +#define TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_MASKGENMASK(5, 4)
> +#define TUSB1210_VENDOR_SPECIFIC2_DP_MASKBIT(6)
>
> struct tusb1210 {
> struct ulpi *ulpi;
> @@ -118,22 +119,22 @@ static int tusb1210_probe(struct ulpi *ulpi)
>* diagram optimization and DP/DM swap.
>*/
>
> + reg = ulpi_read(ulpi, TUSB1210_VENDOR_SPECIFIC2);
> +
> /* High speed output drive strength configuration */
> - device_property_read_u8(>dev, "ihstx", );
> - reg = val << TUSB1210_VENDOR_SPECIFIC2_IHSTX_SHIFT;
> + if (!device_property_read_u8(>dev, "ihstx", ))
> + u8p_replace_bits(, val,
> (u8)TUSB1210_VENDOR_SPECIFIC2_IHSTX_MASK);
Any reason for using u8p_replace_bits and not u8_replace_bits?
Also please drop the u8 casts above, they seem unnecessary here
>
> /* High speed output impedance configuration */
> - device_property_read_u8(>dev, "zhsdrv", );
> - reg |= val << TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_SHIFT;
> + if (!device_property_read_u8(>dev, "zhsdrv", ))
> + u8p_replace_bits(, val,
> (u8)TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_MASK);
>
> /* DP/DM swap control */
> - device_property_read_u8(>dev, "datapolarity", );
> - reg |= val << TUSB1210_VENDOR_SPECIFIC2_DP_SHIFT;
> + if (!device_property_read_u8(>dev, "datapolarity", ))
> + u8p_replace_bits(, val,
> (u8)TUSB1210_VENDOR_SPECIFIC2_DP_MASK);
>
> - if (reg) {
> - ulpi_write(ulpi, TUSB1210_VENDOR_SPECIFIC2, reg);
> - tusb->vendor_specific2 = reg;
> - }
> + ulpi_write(ulpi, TUSB1210_VENDOR_SPECIFIC2, reg);
> + tusb->vendor_specific2 = reg;
>
> tusb->phy = ulpi_phy_create(ulpi, _ops);
> if (IS_ERR(tusb->phy))
>
> base-commit: 9123e3a74ec7b934a4a099e98af6a61c2f80bbf5
> --
> 2.27.0
--
~Vinod
Re: [PATCH v4] dt-binding: phy: convert ti,omap-usb2 to YAML
On 24-08-20, 10:47, Roger Quadros wrote: > Hi, > > On 21/08/2020 11:11, Roger Quadros wrote: > > Move ti,omap-usb2 to its own YAML schema. > > > > Signed-off-by: Roger Quadros > > Reviewed-by: Rob Herring > > --- > > > > v4 > > - fix example to fix dt_binding_check warnings > > - '#phy-cells' -> "#phy-cells" > > - Add 'oneOf' to compatible logic to allow just "ti,omap-usb2" as valid > > > > v3 > > - Removed quotes from compatibles > > - changed property to "ti,disable-charger-det" > > > > v2 > > - Address Rob's comments on YAML schema. > > > > .../devicetree/bindings/phy/ti,omap-usb2.yaml | 72 +++ > > .../devicetree/bindings/phy/ti-phy.txt| 37 -- > > 2 files changed, 72 insertions(+), 37 deletions(-) > > create mode 100644 Documentation/devicetree/bindings/phy/ti,omap-usb2.yaml > > > > diff --git a/Documentation/devicetree/bindings/phy/ti,omap-usb2.yaml > > b/Documentation/devicetree/bindings/phy/ti,omap-usb2.yaml > > new file mode 100644 > > index ..a05110351814 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/phy/ti,omap-usb2.yaml > > @@ -0,0 +1,72 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/phy/ti,omap-usb2.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: OMAP USB2 PHY > > + > > +maintainers: > > + - Kishon Vijay Abraham I > > + - Roger Quadros > > + > > +properties: > > + compatible: > > +oneOf: > > + - items: > > +- enum: > > + - ti,dra7x-usb2 > > + - ti,dra7x-usb2-phy2 > > + - ti,am654-usb2 > > I missed these two. > "ti,omap5-usb2" > "ti,am437x-usb2" > > While "ti,am437x-usb2" is being used in the device tree files > I don't see "ti,omap5-usb2" being used anywhere. > > omap5-l4.dtsi uses "ti,omap-usb2" > > Should we get rid of "ti,omap5-usb2"? Sure drop them ;-) we can always add back when we have a user -- ~Vinod
Re: [PATCH v5 1/2] dt-bindings: dma: Add bindings for intel LGM SOC
On 31-08-20, 16:06, Reddy, MallikarjunaX wrote: > Hi Vinod, > > Thanks for the review. Please see my comment inline. > > On 8/28/2020 6:45 PM, Vinod Koul wrote: > > On 27-08-20, 17:54, Reddy, MallikarjunaX wrote: > > > Hi Vinod, > > > Thanks for the review comments. > > > > > > On 8/25/2020 7:21 PM, Vinod Koul wrote: > > > > On 18-08-20, 15:00, Reddy, MallikarjunaX wrote: > > > > > > > > > > > + > > > > > > > +intel,chans: > > > > > > > + $ref: /schemas/types.yaml#/definitions/uint32-array > > > > > > > + description: > > > > > > > + The channels included on this port. Format is > > > > > > > channel start > > > > > > > + number and how many channels on this port. > > > > > > Why does this need to be in DT? This all seems like it can be in > > > > > > the dma > > > > > > cells for each client. > > > > > (*ABC) > > > > > Yes. We need this. > > > > > for dma0(lgm-cdma) old SOC supports 16 channels and the new SOC > > > > > supports 22 > > > > > channels. and the logical channel mapping for the peripherals also > > > > > differ > > > > > b/w old and new SOCs. > > > > > > > > > > Because of this hardware limitation we are trying to configure the > > > > > total > > > > > channels and port-channel mapping dynamically from device tree. > > > > > > > > > > based on port name we are trying to configure the default values for > > > > > different peripherals(ports). > > > > > Example: burst length is not same for all ports, so using port name > > > > > to do > > > > > default configurations. > > > > Sorry that does not make sense to me, why not specify the values to be > > > > used here instead of defining your own name scheme! > > > OK. Agreed. I will remove port name from DT and only use intel,chans > > what is intel,chans, why not use dma-channels? > The intel,chans says about the channels included on the correspondng port. What do you mean by a port here? > Format is channel start number and how many channels on this port. It is perfectly reasonable to have 16 channels but linux not use use all, lets say from 5th channel channel onwards So you need to use standard dma-channels also with dma-channel-mask to specify which channels linux can use > The reasong behind using this attribute instead of standrad dma-channels > is... > > > DMA_VER22 HW supports 22 channels. But there is a hole in HW, total it can > use only 16. > > Old soc supports 4ports and 16 channels. > New soc supports 6ports and 22 channels. > (old and new soc carry the same version VER22) > > port channel mapping for the old and new soc also not the same. > old soc: logical channels:(Rx, Tx) > 0, 1 - SPI0 > 2, 3 - SPI1 > 4, 5 - HSNAND > 12, 14, 13, 15 - Memcopy > > New soc: Logical channels(Rx, Tx) > 0, 1 - SPI0 > 2, 3 - SPI1 > 4, 5 - SPI2 > 6, 7 - SPI3 > 8, 9 - HSNAND > 10 to 21 - Mcopy Mapping is different, client can set that channel required in dmas property and use a specific required channel. > Because of these reasons we are trying to use "intel,chans" attribute, and > reading then number of channels from the dt. > Advantaage: > 1. we can map the channels correspondign to port > 2. Dynamically configure the channels (due to hw limitation) > > If this is not ok, please suggest us the better way to handle this. > > -- ~Vinod
Re: [PATCH v7 3/3] phy: intel: Add Keem Bay eMMC PHY support
On 21-08-20, 19:37, Wan Ahmad Zainie wrote: > +/* From ACS_eMMC51_16nFFC_RO1100_Userguide_v1p0.pdf p17 */ > +#define FREQSEL_200M_170M0x0 > +#define FREQSEL_170M_140M0x1 > +#define FREQSEL_140M_110M0x2 > +#define FREQSEL_110M_80M 0x3 > +#define FREQSEL_80M_50M 0x4 > + > +#define maskval(mask, val) (((val) << (ffs(mask) - 1)) & mask) Kernel has a macro do this for you, please use FIELD_PREP instead of your own macro -- ~Vinod
Re: [PATCH v7 2/3] dt-bindings: phy: intel: Add Keem Bay eMMC PHY bindings
On 21-08-20, 19:37, Wan Ahmad Zainie wrote:
> Binding description for Intel Keem Bay eMMC PHY.
>
> Signed-off-by: Wan Ahmad Zainie
> Reviewed-by: Rob Herring
> ---
> .../bindings/phy/intel,keembay-emmc-phy.yaml | 44 +++
> 1 file changed, 44 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/phy/intel,keembay-emmc-phy.yaml
>
> diff --git
> a/Documentation/devicetree/bindings/phy/intel,keembay-emmc-phy.yaml
> b/Documentation/devicetree/bindings/phy/intel,keembay-emmc-phy.yaml
> new file mode 100644
> index ..4cbbd3887c13
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/intel,keembay-emmc-phy.yaml
> @@ -0,0 +1,44 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/phy/intel,keembay-emmc-phy.yaml#;
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#;
> +
> +title: Intel Keem Bay eMMC PHY bindings
This seems same as
Documentation/devicetree/bindings/phy/intel,lgm-emmc-phy.yaml, why not
add a new compatible in lgm binding, or did I miss a difference?
> +
> +maintainers:
> + - Wan Ahmad Zainie
> +
> +properties:
> + compatible:
> +const: intel,keembay-emmc-phy
> +
> + reg:
> +maxItems: 1
> +
> + clocks:
> +maxItems: 1
> +
> + clock-names:
> +items:
> + - const: emmcclk
> +
> + "#phy-cells":
> +const: 0
> +
> +required:
> + - compatible
> + - reg
> + - "#phy-cells"
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> +phy@2029 {
> + compatible = "intel,keembay-emmc-phy";
> + reg = <0x2029 0x54>;
> + clocks = <>;
> + clock-names = "emmcclk";
> + #phy-cells = <0>;
> +};
> --
> 2.17.1
--
~Vinod
Re: [PATCH 0/8] drivers: phy: Constify static phy_ops structs
On 24-08-20, 00:00, Rikard Falkeborn wrote: > This series constifies all static phy_ops structs in drivers/phy. > Typically the only usage is to pass the address of it to devm_phy_create() > which takes a const pointer. The lone exception is in > drivers/phy/qualcomm/phy-qcom-ipq4019-usb.c where the address of the > structs is assigned to the data-field in of_device_id, which is a const > void pointer. > > Making the structs const allows the compiler to put them in read-only > memory. > > The patches are all independent of each other, and have been > compile-tested only. Applied all, thanks -- ~Vinod
Re: [PATCH v6 0/2] phy: omap-usb2-phy: Errata and coding style fix
On 24-08-20, 10:51, Roger Quadros wrote: > Hi, > > This series addresses silicon errata > i2075 - "USB2PHY: USB2PHY Charger Detect is Enabled by Default > Without VBUS Presence" > > It also fixes coding style issues. Applied, thanks -- ~Vinod
Re: [RESEND PATCH v3 3/3] phy: Add USB HSIC PHY driver for Marvell MMP3 SoC
On 18-08-20, 00:34, Lubomir Rintel wrote:
> Add PHY driver for the HSICs found on Marvell MMP3 SoC. The driver is
> rather straightforward -- the PHY essentially just needs to be enabled.
>
> Signed-off-by: Lubomir Rintel
>
> ---
> Changes since v1:
> - Explicitely cast drvdata pointer to make sparse happy
>
> drivers/phy/marvell/Kconfig | 12 +
> drivers/phy/marvell/Makefile| 1 +
> drivers/phy/marvell/phy-mmp3-hsic.c | 82 +
> 3 files changed, 95 insertions(+)
> create mode 100644 drivers/phy/marvell/phy-mmp3-hsic.c
>
> diff --git a/drivers/phy/marvell/Kconfig b/drivers/phy/marvell/Kconfig
> index 8f6273c837ec3..6c96f2bf52665 100644
> --- a/drivers/phy/marvell/Kconfig
> +++ b/drivers/phy/marvell/Kconfig
> @@ -116,3 +116,15 @@ config PHY_MMP3_USB
> The PHY driver will be used by Marvell udc/ehci/otg driver.
>
> To compile this driver as a module, choose M here.
> +
> +config PHY_MMP3_HSIC
> + tristate "Marvell MMP3 USB HSIC PHY Driver"
> + depends on MACH_MMP3_DT || COMPILE_TEST
> + select GENERIC_PHY
> + help
> + Enable this to support Marvell MMP3 USB HSIC PHY driver for
> + Marvell MMP3 SoC. This driver will be used my the Marvell EHCI
> + driver to initialize the interface to internal USB HSIC
> + components on MMP3-based boards.
> +
> + To compile this driver as a module, choose M here.
> diff --git a/drivers/phy/marvell/Makefile b/drivers/phy/marvell/Makefile
> index 5a106b1549f41..7f296ef028292 100644
> --- a/drivers/phy/marvell/Makefile
> +++ b/drivers/phy/marvell/Makefile
> @@ -3,6 +3,7 @@ obj-$(CONFIG_ARMADA375_USBCLUSTER_PHY)+=
> phy-armada375-usb2.o
> obj-$(CONFIG_PHY_BERLIN_SATA)+= phy-berlin-sata.o
> obj-$(CONFIG_PHY_BERLIN_USB) += phy-berlin-usb.o
> obj-$(CONFIG_PHY_MMP3_USB) += phy-mmp3-usb.o
> +obj-$(CONFIG_PHY_MMP3_HSIC) += phy-mmp3-hsic.o
> obj-$(CONFIG_PHY_MVEBU_A3700_COMPHY) += phy-mvebu-a3700-comphy.o
> obj-$(CONFIG_PHY_MVEBU_A3700_UTMI) += phy-mvebu-a3700-utmi.o
> obj-$(CONFIG_PHY_MVEBU_A38X_COMPHY) += phy-armada38x-comphy.o
> diff --git a/drivers/phy/marvell/phy-mmp3-hsic.c
> b/drivers/phy/marvell/phy-mmp3-hsic.c
> new file mode 100644
> index 0..47c1e8894939f
> --- /dev/null
> +++ b/drivers/phy/marvell/phy-mmp3-hsic.c
> @@ -0,0 +1,82 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2020 Lubomir Rintel
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define HSIC_CTRL0x08
> +#define HSIC_ENABLE BIT(7)
> +#define PLL_BYPASS BIT(4)
> +
> +static int mmp3_hsic_phy_init(struct phy *phy)
> +{
> + void __iomem *base = (void __iomem *)phy_get_drvdata(phy);
you are casting away from void * and casting to another void *,
something doesn't look correct!
> + u32 hsic_ctrl;
> +
> + hsic_ctrl = readl_relaxed(base + HSIC_CTRL);
> + hsic_ctrl |= HSIC_ENABLE;
> + hsic_ctrl |= PLL_BYPASS;
> + writel_relaxed(hsic_ctrl, base + HSIC_CTRL);
> +
> + return 0;
> +}
> +
> +static const struct phy_ops mmp3_hsic_phy_ops = {
> + .init = mmp3_hsic_phy_init,
> + .owner = THIS_MODULE,
> +};
> +
> +static const struct of_device_id mmp3_hsic_phy_of_match[] = {
> + { .compatible = "marvell,mmp3-hsic-phy", },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, mmp3_hsic_phy_of_match);
> +
> +static int mmp3_hsic_phy_probe(struct platform_device *pdev)
> +{
> + struct device *dev = >dev;
> + struct phy_provider *provider;
> + struct resource *resource;
> + void __iomem *base;
> + struct phy *phy;
> +
> + resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + base = devm_ioremap_resource(dev, resource);
> + if (IS_ERR(base)) {
> + dev_err(dev, "failed to remap PHY regs\n");
> + return PTR_ERR(base);
> + }
> +
> + phy = devm_phy_create(dev, NULL, _hsic_phy_ops);
> + if (IS_ERR(phy)) {
> + dev_err(dev, "failed to create PHY\n");
> + return PTR_ERR(phy);
> + }
> +
> + phy_set_drvdata(phy, (void *)base);
again skip the cast
> + provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> + if (IS_ERR(provider)) {
> + dev_err(dev, "failed to register PHY provider\n");
> + return PTR_ERR(provider);
> + }
> +
> + return 0;
> +}
> +
> +static struct platform_driver mmp3_hsic_phy_driver = {
> + .probe = mmp3_hsic_phy_probe,
> + .driver = {
> + .name = "mmp3-hsic-phy",
> + .of_match_table = mmp3_hsic_phy_of_match,
> + },
> +};
> +module_platform_driver(mmp3_hsic_phy_driver);
> +
> +MODULE_AUTHOR("Lubomir Rintel ");
> +MODULE_DESCRIPTION("Marvell MMP3 USB HSIC PHY Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.26.2
--
~Vinod
Re: [PATCH V5 5/7] phy: tegra: xusb: Add support for charger detect
On 20-07-20, 15:25, Nagarjuna Kristam wrote:
> +#define USB2_BATTERY_CHRG_OTGPADX_CTL0(x)(0x80 + (x) * 0x40)
> +#define PD_CHG BIT(0)
> +#define VDCD_DET_FILTER_EN BIT(4)
> +#define VDAT_DETBIT(5)
> +#define VDAT_DET_FILTER_EN BIT(8)
> +#define OP_SINK_EN BIT(9)
> +#define OP_SRC_EN BIT(10)
> +#define ON_SINK_EN BIT(11)
> +#define ON_SRC_EN BIT(12)
> +#define OP_I_SRC_EN BIT(13)
> +#define ZIP_FILTER_EN BIT(21)
> +#define ZIN_FILTER_EN BIT(25)
> +#define DCD_DETECTEDBIT(26)
> +
> +#define USB2_BATTERY_CHRG_OTGPADX_CTL1(x)(0x84 + (x) * 0x40)
> +#define PD_VREG BIT(6)
> +#define VREG_LEV(x) (((x) & 0x3) << 7)
> +#define VREG_DIR(x) (((x) & 0x3) << 11)
GENMASK and FIELD_PREP please
> +static void
> +tegra_xusb_padctl_set_debounce_time(struct tegra_xusb_padctl *padctl,
> + u32 debounce)
> +{
> + u32 value;
> +
> + value = padctl_readl(padctl,
> + XUSB_PADCTL_USB2_BATTERY_CHRG_TDCD_DBNC_TIMER_0);
Single line
> +static void
> +tegra_xusb_padctl_charger_detect_on(struct tegra_xusb_padctl *padctl, u32
> index)
> +{
> + u32 value;
> +
> + value = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
> + value &= ~USB2_OTG_PD_ZI;
> + padctl_writel(padctl, value, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
> +
> + value = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
> + value |= (USB2_OTG_PD2 | USB2_OTG_PD2_OVRD_EN);
> + padctl_writel(padctl, value, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
> +
> + value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> + value &= ~PD_CHG;
> + padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
maybe a padctl_updatel() helper to do read, modify and write
> +static void tegra_xusb_padctl_dcd(struct tegra_xusb_padctl *padctl, u32
> index)
> +{
> + u32 value;
> + unsigned int offset;
> + bool ret = false;
> +
> + /* Turn on IDP_SRC */
> + value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> + value |= OP_I_SRC_EN;
> + padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> +
> + /* Turn on D- pull-down resistor */
> + value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
> + value |= USBON_RPD_OVRD_VAL;
> + padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
> +
> + /* Wait for TDCD_DBNC (DCD debounce), refer to BC1.2 spec Table 5 */
> + usleep_range(1, 2);
> +
> + offset = USB2_BATTERY_CHRG_OTGPADX_CTL0(index);
> + ret = readl_poll_timeout(padctl->regs + offset, value,
> + value & DCD_DETECTED, 2,
> + TDCD_TIMEOUT_MS * 1000);
> + if (!ret)
> + dev_warn(padctl->dev, "%s: DCD timeout.", __func__);
Not dev_err() ? and continue after this?
> +static bool
> +tegra_xusb_padctl_primary_secondary(struct tegra_xusb_padctl *padctl, u32
> index,
> + bool is_primary)
> +{
> + u32 value;
> + u32 config = is_primary ? (OP_SRC_EN | ON_SINK_EN) :
> + (ON_SRC_EN | OP_SINK_EN);
> + bool ret = false;
> +
> + if (is_primary)
> + /* data contact detection */
> + tegra_xusb_padctl_dcd(padctl, index);
> +
> + /* Source D- to D+ */
> + value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> + value |= config;
> + padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> +
> + /*
> + * Wait for TVDPSRC_ON/TVDMSRC_ON(D+/- voltage source on time),
> + * refer to BC1.2 spec Table 5
> + */
> + msleep(40);
> +
> + value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL0(index));
> + if (is_primary)
> + ret = !!(value & VDAT_DET);
> + else
> + ret = !(value & VDAT_DET);
How about
ret = !(value & VDAT_DET);
if (is_primary)
ret = !ret;
> +#define VON_DIV2P0_DET BIT(0)
> +#define VON_DIV2P7_DET BIT(1)
> +#define VOP_DIV2P0_DET BIT(2)
> +#define VOP_DIV2P7_DET BIT(3)
> +
> +#define VREG_CUR_LEVEL_0500
> +#define VREG_CUR_LEVEL_1900
> +#define VREG_CUR_LEVEL_21500
> +#define VREG_CUR_LEVEL_32000
tabs or spaces, pick one please, not both
> +
> +#define IS_CUR_IN_RANGE(ma, low, high) \
> + ((ma >= VREG_CUR_LEVEL_##low) && (ma <= (VREG_CUR_LEVEL_##high - 1)))
> +#define VREG_LVL(ma, level) IS_CUR_IN_RANGE(ma, level, level + 1)
> +
> +static void
Re: [PATCH V5 4/7] phy: tegra: xusb: Add soc ops API to enable UTMI PAD protection
On 20-07-20, 15:25, Nagarjuna Kristam wrote:
> When USB charger is enabled, UTMI PAD needs to be protected according
> to the direction and current level. Add support for the same on Tegra210
> and Tegra186.
>
> Signed-off-by: Nagarjuna Kristam
> Acked-by: Thierry Reding
> ---
> V5:
> - No changes.
> ---
> V4:
> - Added Acked-by updates to commit message.
> ---
> V3:
> - Alligned function and its arguments.
> - Fixed other comments from Thierry.
> ---
> V2:
> - Commit message coorected.
> - Patch re-based.
> ---
> drivers/phy/tegra/xusb-tegra186.c | 40
> +++
> drivers/phy/tegra/xusb-tegra210.c | 32 +++
> drivers/phy/tegra/xusb.h | 13 +
> 3 files changed, 85 insertions(+)
>
> diff --git a/drivers/phy/tegra/xusb-tegra186.c
> b/drivers/phy/tegra/xusb-tegra186.c
> index f862254..59b78a7 100644
> --- a/drivers/phy/tegra/xusb-tegra186.c
> +++ b/drivers/phy/tegra/xusb-tegra186.c
> @@ -68,6 +68,13 @@
> #define PORTX_SPEED_SUPPORT_MASK (0x3)
> #define PORT_SPEED_SUPPORT_GEN1 (0x0)
>
> +#define USB2_BATTERY_CHRG_OTGPADX_CTL1(x) (0x84 + (x) * 0x40)
> +#define PD_VREG(1 << 6)
> +#define VREG_LEV(x)(((x) & 0x3) << 7)
> +#define VREG_DIR(x)(((x) & 0x3) << 11)
maybe FIELD_GET() would be better, avoids error with shifts
> +#define VREG_DIR_INVREG_DIR(1)
> +#define VREG_DIR_OUT VREG_DIR(2)
> +
> #define XUSB_PADCTL_USB2_OTG_PADX_CTL0(x)(0x88 + (x) * 0x40)
> #define HS_CURR_LEVEL(x)((x) & 0x3f)
> #define TERM_SELBIT(25)
> @@ -289,6 +296,37 @@ static void tegra_phy_xusb_utmi_pad_power_down(struct
> phy *phy)
> usb2->powered_on = false;
> }
>
> +static void
> +tegra186_xusb_padctl_utmi_pad_set_protection(struct tegra_xusb_port *port,
> + int level,
> + enum tegra_vbus_dir dir)
> +{
> + u32 value;
> + struct tegra_xusb_padctl *padctl = port->padctl;
> + unsigned int index = port->index;
> +
> + value = padctl_readl(padctl, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
> +
> + if (level < 0) {
> + /* disable pad protection */
> + value |= PD_VREG;
> + value &= ~VREG_LEV(~0);
> + value &= ~VREG_DIR(~0);
> + } else {
> + if (dir == TEGRA_VBUS_SOURCE)
> + value |= VREG_DIR_OUT;
> + else if (dir == TEGRA_VBUS_SINK)
> + value |= VREG_DIR_IN;
> +
> + value &= ~PD_VREG;
> + value &= ~VREG_DIR(~0);
> + value &= ~VREG_LEV(~0);
> + value |= VREG_LEV(level);
This would look much cleaner with FIELD_PREP() or u32_encoded_bits()
> + }
> +
> + padctl_writel(padctl, value, USB2_BATTERY_CHRG_OTGPADX_CTL1(index));
> +}
> +
> static int tegra186_xusb_padctl_vbus_override(struct tegra_xusb_padctl
> *padctl,
> bool status)
> {
> @@ -935,6 +973,8 @@ static const struct tegra_xusb_padctl_ops
> tegra186_xusb_padctl_ops = {
> .vbus_override = tegra186_xusb_padctl_vbus_override,
> .utmi_pad_power_on = tegra_phy_xusb_utmi_pad_power_on,
> .utmi_pad_power_down = tegra_phy_xusb_utmi_pad_power_down,
> + .utmi_pad_set_protection =
> + tegra186_xusb_padctl_utmi_pad_set_protection,
single line please
> };
>
> #if IS_ENABLED(CONFIG_ARCH_TEGRA_186_SOC)
> diff --git a/drivers/phy/tegra/xusb-tegra210.c
> b/drivers/phy/tegra/xusb-tegra210.c
> index 2e5f71c..3aff284 100644
> --- a/drivers/phy/tegra/xusb-tegra210.c
> +++ b/drivers/phy/tegra/xusb-tegra210.c
> @@ -74,6 +74,8 @@
> #define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV_MASK 0x3
> #define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV_VAL 0x1
> #define XUSB_PADCTL_USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_FIX18 (1 << 6)
> +#define USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_LEV(x) (((x) & 0x3) << 7)
> +#define USB2_BATTERY_CHRG_OTGPAD_CTL1_VREG_DIR(x) (((x) & 0x3) << 11)
>
> #define XUSB_PADCTL_USB2_OTG_PADX_CTL0(x) (0x088 + (x) * 0x40)
> #define XUSB_PADCTL_USB2_OTG_PAD_CTL0_PD_ZI (1 << 29)
> @@ -1116,6 +1118,34 @@ static void tegra210_usb2_pad_power_down(struct phy
> *phy)
> usb2->powered_on = false;
> }
>
> +static void
> +tegra210_xusb_padctl_utmi_pad_set_protection(struct tegra_xusb_port *port,
> + int level,
> + enum tegra_vbus_dir dir)
single line and aligned to preceeding parenthesis (hint checkpatch.pl
with --strict option tells you so)
> +{
> + u32 value;
> + struct tegra_xusb_padctl *padctl = port->padctl;
> + unsigned int index = port->index;
> +
> + value = padctl_readl(padctl,
Re: [PATCH] fsldma: fsl_ioread64*() do not need lower_32_bits()
Hi Linus,
On 29-08-20, 14:20, Linus Torvalds wrote:
> On Sat, Aug 29, 2020 at 1:40 PM Guenter Roeck wrote:
> >
> > Except for
> >
> > CHECK: spaces preferred around that '+' (ctx:VxV)
> > #29: FILE: drivers/dma/fsldma.h:223:
> > + u32 val_lo = in_be32((u32 __iomem *)addr+1);
>
> Added spaces.
>
> > I don't see anything wrong with it either, so
> >
> > Reviewed-by: Guenter Roeck
> >
> > Since I didn't see the real problem with the original code,
> > I'd take that with a grain of salt, though.
>
> Well, honestly, the old code was so confused that just making it build
> is clearly already an improvement even if everything else were to be
> wrong.
>
> So I committed my "fix". If it turns out there's more wrong in there
> and somebody tests it, we can fix it again. But now it hopefully
> compiles, at least.
>
> My bet is that if that driver ever worked on ppc32, it will continue
> to work whatever we do to that function.
>
> I _think_ the old code happened to - completely by mistake - get the
> value right for the case of "little endian access, with dma_addr_t
> being 32-bit". Because then it would still read the upper bits wrong,
> but the cast to dma_addr_t would then throw those bits away. And the
> lower bits would be right.
>
> But for big-endian accesses or for ARCH_DMA_ADDR_T_64BIT it really
> looks like it always returned a completely incorrect value.
>
> And again - the driver may have worked even with that completely
> incorrect value, since the use of it seems to be very incidental.
Thank you for the fix.
Acked-By: Vinod Koul
>
> In either case ("it didn't work before" or "it worked because the
> value doesn't really matter"), I don't think I could possibly have
> made things worse.
>
> Famous last words.
I guess no one tested this on 32bits seems to have caused this.
--
~Vinod
Re: [PATCH 09/11] soundwire: intel: add dynamic debug trace for clock-stop invalid configs
On 28-08-20, 09:54, Pierre-Louis Bossart wrote:
>
> > > > > Detect cases where the clock is assumed to be stopped but the IP is
> > > > > not in the relevant state, and add a dynamic debug trace.
> > > >
> > > > you meant a debug print..and it looks like error print below (also in
> > > > title).
> > >
> > > I don't understand the comment. Is the 'trace' confusing and are you
> > > asking
> > > to e.g. change the commit message to 'add dynamic debug log'?
> >
> > Question is what is dynamic about this?
> dev_dbg() is part of the kernel dynamic debug capability...
>
> https://www.kernel.org/doc/html/latest/admin-guide/dynamic-debug-howto.html
>
> Not sure what you are asking here?
:-| where is dev_dbg() ?
See [1]
On 18-08-20, 10:41, Bard Liao wrote:
> From: Pierre-Louis Bossart
>
> Detect cases where the clock is assumed to be stopped but the IP is
> not in the relevant state, and add a dynamic debug trace.
>
> Signed-off-by: Pierre-Louis Bossart
> Signed-off-by: Bard Liao
> ---
> drivers/soundwire/intel.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index 7c63581270fd..b82d02af3c4f 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -1964,6 +1964,11 @@ static int intel_resume_runtime(struct device *dev)
> }
> }
> } else if (!clock_stop_quirks) {
> +
> + clock_stop0 = sdw_cdns_is_clock_stop(>cdns);
> + if (!clock_stop0)
[1]
> + dev_err(dev, "%s invalid configuration, clock was not
> stopped", __func__);
^^^
> +
> ret = intel_init(sdw);
> if (ret) {
> dev_err(dev, "%s failed: %d", __func__, ret);
> --
> 2.17.1
--
~Vinod
[PATCH] regmap: soundwire: remove unsed header mod_devicetable.h
mod_devicetable.h does not seem to be required for this file, so remove it. Signed-off-by: Vinod Koul --- drivers/base/regmap/regmap-sdw.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/base/regmap/regmap-sdw.c b/drivers/base/regmap/regmap-sdw.c index 50a66382d87d..c92d614b4943 100644 --- a/drivers/base/regmap/regmap-sdw.c +++ b/drivers/base/regmap/regmap-sdw.c @@ -2,7 +2,6 @@ // Copyright(c) 2015-17 Intel Corporation. #include -#include #include #include #include "internal.h" -- 2.26.2
Re: [PATCH v5 1/2] dt-bindings: dma: Add bindings for intel LGM SOC
On 27-08-20, 17:54, Reddy, MallikarjunaX wrote: > Hi Vinod, > Thanks for the review comments. > > On 8/25/2020 7:21 PM, Vinod Koul wrote: > > On 18-08-20, 15:00, Reddy, MallikarjunaX wrote: > > > > > > > + > > > > > +intel,chans: > > > > > + $ref: /schemas/types.yaml#/definitions/uint32-array > > > > > + description: > > > > > + The channels included on this port. Format is > > > > > channel start > > > > > + number and how many channels on this port. > > > > Why does this need to be in DT? This all seems like it can be in the dma > > > > cells for each client. > > > (*ABC) > > > Yes. We need this. > > > for dma0(lgm-cdma) old SOC supports 16 channels and the new SOC supports > > > 22 > > > channels. and the logical channel mapping for the peripherals also differ > > > b/w old and new SOCs. > > > > > > Because of this hardware limitation we are trying to configure the total > > > channels and port-channel mapping dynamically from device tree. > > > > > > based on port name we are trying to configure the default values for > > > different peripherals(ports). > > > Example: burst length is not same for all ports, so using port name to do > > > default configurations. > > Sorry that does not make sense to me, why not specify the values to be > > used here instead of defining your own name scheme! > OK. Agreed. I will remove port name from DT and only use intel,chans what is intel,chans, why not use dma-channels? -- ~Vinod
Re: [PATCH] dmaengine: Remove unused define for dma_request_slave_channel_reason()
On 28-08-20, 11:41, Peter Ujfalusi wrote: > No users left in the kernel, it can be removed. Applied, thanks -- ~Vinod
Re: [PATCH] soundwire: cadence: fix race condition between suspend and Slave device alerts
On 21-08-20, 10:17, Pierre-Louis Bossart wrote: > > > > > cancel_work_sync() will either > > > a) wait until the current work completes, or > > > b) prevent a new one from starting. > > > > > > there's no way to really 'abort' a workqueue, 'cancel' means either > > > complete > > > or don't start. > > > > Quite right, as that is how everyone deals with it. Stop the irq from > > firing first and then wait until work is cancelled or completed, hence > > cancel_work_sync() > > No, this cannot work... The work queue in progress will initiate > transactions which would never complete because the interrupts are disabled. Okay that makes sense then > > > if you disable the interrupts then cancel the work, you have a risk of not > > > letting the work complete if it already started (case a). > > > > > > The race is > > > a) the interrupt thread (this function) starts > > > b) the work is scheduled and starts > > > c) the suspend handler starts and disables interrupts in [1] below. > > > d) the work initiates transactions which will never complete since Cadence > > > interrupts have been disabled. > > > > Would it not be better to let work handle the case of interrupts > > disabled and not initiates transactions which wont complete here? That > > sounds more reasonable to do rather than complete the work which anyone > > doesn't matter as you are suspending > > A in-progress workqueue has no notion that interrupts are disabled, nor that > the device is in the process of suspending. It writes into a fifo and blocks > with wait_for_completion(). the complete() is done in an interrupt thread, > triggered when the RX Fifo reaches a watermark. > > So if you disable interrupts, the complete() never happens. > > The safe way to do things with the current code is to let the workqueue > complete, then disable interrupts. > > We only disable interrupts on suspend, we don't need to test if interrupts > are enabled for every single byte that's transmitted on the bus. Not to > mention that this additional test would be racy as hell and require yet > another synchronization primitive making the code more complicated. > > So yes, the current transactions will complete and will be ignored, but it's > a lot better than trying to prevent these transactions from happening with > extra layers of complexity that will impact *every* transaction. > > BTW I looked at another alternative based on the msg lock, so that > interrupts cannot be disabled while a message is being sent. However because > a workqueue may send multiple messages, e.g. to read registers are > non-contiguous locations, there is no way to guarantee what happens how > messages and interrupt disabling are scheduled, so there'd still be a case > of a workqueue not completing and being stuck on a mutex_lock(), not so good > either. > > In short, this is the simplest way to fix the timeout on resume. Is this timeout for suspend or resume? Somehow I was under the assumption that it is former? Or is the result seen on resume? Rereading the race describe above in steps, I think this should be handled in step c above. Btw is that suspend or runtime suspend which causes this? Former would be bigger issue as we should not have work running when we return from suspend call. Latter should be dealt with anyway as device might be off after suspend. -- ~Vinod
Re: [PATCH 09/11] soundwire: intel: add dynamic debug trace for clock-stop invalid configs
On 26-08-20, 09:38, Pierre-Louis Bossart wrote: > > > On 8/26/20 4:48 AM, Vinod Koul wrote: > > On 18-08-20, 10:41, Bard Liao wrote: > > > From: Pierre-Louis Bossart > > > > > > Detect cases where the clock is assumed to be stopped but the IP is > > > not in the relevant state, and add a dynamic debug trace. > > > > you meant a debug print..and it looks like error print below (also in > > title). > > I don't understand the comment. Is the 'trace' confusing and are you asking > to e.g. change the commit message to 'add dynamic debug log'? Question is what is dynamic about this? -- ~Vinod
Re: [PATCH 07/11] soundwire: intel: Only call sdw stream APIs for the first cpu_dai
On 26-08-20, 09:35, Pierre-Louis Bossart wrote: > > > > - ret = sdw_prepare_stream(dma->stream); > > > + /* > > > + * All cpu dais belong to a stream. To ensure sdw_prepare_stream > > > + * is called once per stream, we should call it only when > > > + * dai = first_cpu_dai. > > > + */ > > > + if (first_cpu_dai == dai) > > > + ret = sdw_prepare_stream(dma->stream); > > > > Hmmm why not use the one place which is unique in the card to call this, > > hint machine dais are only called once. > > we are already calling directly sdw_startup_stream() and > sdw_shutdown_stream() from the machine driver. > > We could call sdw_stream_enable() in the dailink .trigger as well, since it > only calls the stream API. Correct :) > However for both .prepare() and .hw_free() there are a set of dai-level > configurations using static functions defined only in intel.c, and I don't > think we can move the code to the machine driver, or split the > prepare/hw_free in two (dailink and dai operations). Cant they be exported and continue to call those apis > I am not against your idea, I am not sure if it can be done. > > Would you be ok to merge this as a first step and let us work on an > optimization later (which would require ASoC/SoundWire synchronization)? The problem is that we add one flag then another and it does become an issue eventually, better to do the right thing now than later. -- ~Vinod
Re: [PATCH 07/11] soundwire: intel: Only call sdw stream APIs for the first cpu_dai
On 28-08-20, 01:47, Liao, Bard wrote: > > snd_pcm_substream *substream, > > > goto err; > > > } > > > > > > - ret = sdw_prepare_stream(dma->stream); > > > + /* > > > + * All cpu dais belong to a stream. To ensure sdw_prepare_stream > > > + * is called once per stream, we should call it only when > > > + * dai = first_cpu_dai. > > > + */ > > > + if (first_cpu_dai == dai) > > > + ret = sdw_prepare_stream(dma->stream); > > > > Hmmm why not use the one place which is unique in the card to call this, > > hint machine dais are only called once. > > Yes, we can call it in machine driver. But, shouldn't it belong to platform > level? The point is that if we move the stuff to machine driver, it will > force people to implement these stuff on their own Intel machine driver. nothing stops anyone from doing that right! machine driver is another component so it can be moved there as well -- ~Vinod
[PATCH] arm64: dts: qcom: sdm845-db845c: Fix hdmi nodes
As per binding documentation, we should have dsi as node 0 and hdmi
audio as node 1, so fix it
Reported-by: Dmitry Baryshkov
Fixes: aef9a119dfb9 ("arm64: dts: qcom: sdm845-db845c: Add hdmi bridge nodes")
Signed-off-by: Vinod Koul
---
arch/arm64/boot/dts/qcom/sdm845-db845c.dts | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
index a2a98680ccf5..99d33955270e 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
+++ b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
@@ -451,16 +451,16 @@ ports {
port@0 {
reg = <0>;
- lt9611_out: endpoint {
- remote-endpoint = <_con>;
+ lt9611_a: endpoint {
+ remote-endpoint = <_out>;
};
};
- port@1 {
- reg = <1>;
+ port@2 {
+ reg = <2>;
- lt9611_a: endpoint {
- remote-endpoint = <_out>;
+ lt9611_out: endpoint {
+ remote-endpoint = <_con>;
};
};
};
--
2.26.2
[PATCH] drm/bridge: Fix the dsi remote end-points
DSI end-points are supposed to be at node 0 and node 1 as per binding.
So fix this and use node 0 and node 1 for dsi.
Reported-by: Dmitry Baryshkov
Fixes: 23278bf54afe ("drm/bridge: Introduce LT9611 DSI to HDMI bridge")
Signed-off-by: Vinod Koul
---
drivers/gpu/drm/bridge/lontium-lt9611.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/lontium-lt9611.c
b/drivers/gpu/drm/bridge/lontium-lt9611.c
index 1009fc4ed4ed..d734d9402c35 100644
--- a/drivers/gpu/drm/bridge/lontium-lt9611.c
+++ b/drivers/gpu/drm/bridge/lontium-lt9611.c
@@ -960,13 +960,13 @@ static const struct drm_bridge_funcs lt9611_bridge_funcs
= {
static int lt9611_parse_dt(struct device *dev,
struct lt9611 *lt9611)
{
- lt9611->dsi0_node = of_graph_get_remote_node(dev->of_node, 1, -1);
+ lt9611->dsi0_node = of_graph_get_remote_node(dev->of_node, 0, -1);
if (!lt9611->dsi0_node) {
dev_err(lt9611->dev, "failed to get remote node for primary
dsi\n");
return -ENODEV;
}
- lt9611->dsi1_node = of_graph_get_remote_node(dev->of_node, 2, -1);
+ lt9611->dsi1_node = of_graph_get_remote_node(dev->of_node, 1, -1);
lt9611->ac_mode = of_property_read_bool(dev->of_node, "lt,ac-mode");
--
2.26.2
Re: [PATCH 05/11] soundwire: bus: update multi-link definition with hw sync details
On 26-08-20, 09:09, Pierre-Louis Bossart wrote: > > > > > + * @hw_sync_min_links: Number of links used by a stream above which > > > + * hardware-based synchronization is required. This value is only > > > + * meaningful if multi_link is set. If set to 1, hardware-based > > > + * synchronization will be used even if a stream only uses a single > > > + * SoundWire segment. > > > > Soundwire spec does not say anything about multi-link so this is left to > > implementer. Assuming that value of 1 would mean hw based sync will > > be used even for single stream does not make sense in generic terms. > > Maybe yes for Intel but may not be true for everyone? > > hw-based sync is required for Intel even for single stream. It's been part > of the recommended programming flows since the beginning but ignored so far. > > That said, this value is set by each master implementation, no one forces > non-Intel users to implement an Intel-specific requirement. > > > We already use m_rt_count in code for this, so the question is why is > > that not sufficient? > > Because as you rightly said above, Intel requires the hw_sync to be used > even for single stream, but we didn't want others to be forced to use the > hw-sync for single stream. the m_rt_count is not sufficient for Intel. > > I think we are in agreement on not forcing everyone to follow what is > required by Intel, and that's precisely why we added this setting. If you > set it to two you would only use hw_sync when two masters are used. Okay, it would be better if we move it to intel driver, but I see it may not be trivial, so lets go with this approach. -- ~Vinod
Re: [PATCH 4/4] regmap: sdw: add support for SoundWire 1.2 MBQ
On 26-08-20, 11:57, Pierre-Louis Bossart wrote:
>
> > > +#include
> > > +#include
> > > +#include
> >
> > Curious why do you need this header?
>
> I'll return the question back to you, since you added this header for
> regmap-sdw.c:
>
> 7c22ce6e21840 (Vinod Koul 2018-01-08 15:50:59 +0530 6) #include
>
Looks like it should be removed :)
I could compile it without any issues on few archs I do. let me push the
patch on my tree and see if bots are happy, will send the patch
>
> so I assumed it was needed here as well.
>
> > > +MODULE_DESCRIPTION("Regmap SoundWire Module");
> >
> > This is same of sdw module, pls make this one a bit different.
>
> yes, fixed.
--
~Vinod
Re: [PATCH 1/7] soundwire: bus: use property to set interrupt masks
Hi Mark,
On 18-08-20, 22:06, Bard Liao wrote:
> From: Pierre-Louis Bossart
>
> Add a slave-level property and program the SCP_INT1_MASK as desired by
> the codec driver. Since there is no DisCo property this has to be an
> implementation-specific firmware property or hard-coded in the driver.
>
> The only functionality change is that implementation-defined
> interrupts are no longer set for amplifiers - those interrupts are
> typically for jack detection or acoustic event detection/hotwording.
>
> Tested-by: Srinivas Kandagatla
> Signed-off-by: Pierre-Louis Bossart
> Reviewed-by: Kai Vehmanen
> Reviewed-by: Guennadi Liakhovetski
> Signed-off-by: Bard Liao
> ---
> drivers/soundwire/bus.c | 12 ++--
> include/linux/soundwire/sdw.h | 2 ++
> sound/soc/codecs/max98373-sdw.c | 3 +++
> sound/soc/codecs/rt1308-sdw.c | 2 ++
> sound/soc/codecs/rt5682-sdw.c | 4
> sound/soc/codecs/rt700-sdw.c| 4
> sound/soc/codecs/rt711-sdw.c| 4
> sound/soc/codecs/rt715-sdw.c| 4
> sound/soc/codecs/wsa881x.c | 1 +
This touches codecs, can you Ack it please
Ideally this should have been split up to header, the codec updates and
finally the bus change!
> 9 files changed, 30 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index e6e0fb9a81b4..3b6a87bc254e 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -1184,13 +1184,13 @@ static int sdw_initialize_slave(struct sdw_slave
> *slave)
> return ret;
>
> /*
> - * Set bus clash, parity and SCP implementation
> - * defined interrupt mask
> - * TODO: Read implementation defined interrupt mask
> - * from Slave property
> + * Set SCP_INT1_MASK register, typically bus clash and
> + * implementation-defined interrupt mask. The Parity detection
> + * may not always be correct on startup so its use is
> + * device-dependent, it might e.g. only be enabled in
> + * steady-state after a couple of frames.
>*/
> - val = SDW_SCP_INT1_IMPL_DEF | SDW_SCP_INT1_BUS_CLASH |
> - SDW_SCP_INT1_PARITY;
> + val = slave->prop.scp_int1_mask;
>
> /* Enable SCP interrupts */
> ret = sdw_update(slave, SDW_SCP_INTMASK1, val, val);
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index 76052f12c9f7..6d91f2ca20b2 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -355,6 +355,7 @@ struct sdw_dpn_prop {
> * @dp0_prop: Data Port 0 properties
> * @src_dpn_prop: Source Data Port N properties
> * @sink_dpn_prop: Sink Data Port N properties
> + * @scp_int1_mask: SCP_INT1_MASK desired settings
> */
> struct sdw_slave_prop {
> u32 mipi_revision;
> @@ -376,6 +377,7 @@ struct sdw_slave_prop {
> struct sdw_dp0_prop *dp0_prop;
> struct sdw_dpn_prop *src_dpn_prop;
> struct sdw_dpn_prop *sink_dpn_prop;
> + u8 scp_int1_mask;
> };
>
> /**
> diff --git a/sound/soc/codecs/max98373-sdw.c b/sound/soc/codecs/max98373-sdw.c
> index 5fe724728e84..17fd1989e873 100644
> --- a/sound/soc/codecs/max98373-sdw.c
> +++ b/sound/soc/codecs/max98373-sdw.c
> @@ -15,6 +15,7 @@
> #include
> #include
> #include
> +#include
> #include "max98373.h"
> #include "max98373-sdw.h"
>
> @@ -287,6 +288,8 @@ static int max98373_read_prop(struct sdw_slave *slave)
> unsigned long addr;
> struct sdw_dpn_prop *dpn;
>
> + prop->scp_int1_mask = SDW_SCP_INT1_BUS_CLASH | SDW_SCP_INT1_PARITY;
> +
> /* BITMAP: 1000 Dataport 3 is active */
> prop->source_ports = BIT(3);
> /* BITMAP: 0010 Dataport 1 is active */
> diff --git a/sound/soc/codecs/rt1308-sdw.c b/sound/soc/codecs/rt1308-sdw.c
> index b0ba0d2acbdd..5cf10fd447eb 100644
> --- a/sound/soc/codecs/rt1308-sdw.c
> +++ b/sound/soc/codecs/rt1308-sdw.c
> @@ -123,6 +123,8 @@ static int rt1308_read_prop(struct sdw_slave *slave)
> unsigned long addr;
> struct sdw_dpn_prop *dpn;
>
> + prop->scp_int1_mask = SDW_SCP_INT1_BUS_CLASH | SDW_SCP_INT1_PARITY;
> +
> prop->paging_support = true;
>
> /* first we need to allocate memory for set bits in port lists */
> diff --git a/sound/soc/codecs/rt5682-sdw.c b/sound/soc/codecs/rt5682-sdw.c
> index 94bf6bee78e6..544073975020 100644
> --- a/sound/soc/codecs/rt5682-sdw.c
> +++ b/sound/soc/codecs/rt5682-sdw.c
> @@ -19,6 +19,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -542,6 +543,9 @@ static int rt5682_read_prop(struct sdw_slave *slave)
> unsigned long addr;
> struct sdw_dpn_prop *dpn;
>
> + prop->scp_int1_mask = SDW_SCP_INT1_IMPL_DEF | SDW_SCP_INT1_BUS_CLASH |
> + SDW_SCP_INT1_PARITY;
> +
> prop->paging_support = false;
>
> /* first we need to allocate memory for set bits in port lists */
> diff --git
Re: [PATCH 1/3] dt-bindings: dmaengine: Document qcom,gpi dma binding
On 26-08-20, 08:35, Rob Herring wrote:
> On Wed, Aug 26, 2020 at 12:32 AM Vinod Koul wrote:
> >
> > On 25-08-20, 20:21, Vinod Koul wrote:
> > > Hey Rob,
> > >
> > > On 24-08-20, 11:40, Rob Herring wrote:
> > > > On Mon, 24 Aug 2020 14:17:10 +0530, Vinod Koul wrote:
> > > > > Add devicetree binding documentation for GPI DMA controller
> > > > > implemented on Qualcomm SoCs
> > > > >
> > > > > Signed-off-by: Vinod Koul
> > > > > ---
> > > > > .../devicetree/bindings/dma/qcom-gpi.yaml | 87
> > > > > +++
> > > > > 1 file changed, 87 insertions(+)
> > > > > create mode 100644
> > > > > Documentation/devicetree/bindings/dma/qcom-gpi.yaml
> > > > >
> > > >
> > > >
> > > > My bot found errors running 'make dt_binding_check' on your patch:
> > > >
> > > > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/dma/qcom-gpi.yaml:
> > > > properties:qcom,ev-factor: {'description': 'Event ring transfer size
> > > > compare to channel transfer ring. Event ring length = ev-factor *
> > > > transfer ring size', 'maxItems': 1} is not valid under any of the given
> > > > schemas (Possible causes of the failure):
> > > >
> > > > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/dma/qcom-gpi.yaml:
> > > > properties:qcom,ev-factor: 'not' is a required property
> > >
> > > Okay updating dt-schema I do see this, now the question is what is this
> > > and what does it mean ;-) I am not sure I comprehend the error message.
> > > I see this for all the new properties I added as required for this
> > > device node
> >
> > Okay I think I have figured it out, I need to provide ref to
> > /schemas/types.yaml#definitions/uint32 for this to work, which does
> > makes sense to me.
> >
> > qcom,max-num-gpii:
> > $ref: /schemas/types.yaml#definitions/uint32
> > maxItems: 1
>
> uint32 is always 1 item, so drop. Is there a max value you can define?
Sorry not sure I follow, to clarify you mean drop uint32, if so which
type to use u8? I can use u8 as max wont be beyond 255.
Yes I will define min as well max values too.
> Otherwise, up to 2^32 - 1 is valid.
I see one more warning given by your bot which I am able to reproduce as
well:
Documentation/devicetree/bindings/dma/qcom,gpi.example.dt.yaml: example-0:
dma-controller@80:reg:0: [0, 8388608, 0, 393216] is too long
So to fix this I added the #address-cells and #size-cells
#address-cells = <2>;
#size-cells = <2>;
reg = <0x0 0x0080 0x0 0x6>;
But I am getting the warning, what am I doing incorrect
--
~Vinod
Re: [PATCH 3/4] soundwire: SDCA: add helper macro to access controls
On 26-08-20, 10:00, Pierre-Louis Bossart wrote: > > > > > +/* v1.2 device - SDCA address mapping */ > > > > Can you please add description of bits used by each field here, > > something like we have done for DevId > > were you referring to something like this? > > * Spec definition > * Register Bit Contents > * DevId_0 [7:4]47:44 sdw_version > * DevId_0 [3:0]43:40 unique_id > * DevId_1 39:32 mfg_id [15:8] > * DevId_2 31:24 mfg_id [7:0] > * DevId_3 23:16 part_id [15:8] > * DevId_4 15:08 part_id [7:0] > * DevId_5 07:00 class_id Correct > > > > > +#define SDW_SDCA_CTL(fun, ent, ctl, ch) (BIT(30) | > > > \ > > > + (((fun) & 0x7) << 22) | > > > \ > > > + (((ent) & 0x40) << 15) | > > > \ > > > + (((ent) & 0x3f) << 7) | > > > \ > > > + (((ctl) & 0x30) << 15) | > > > \ > > > + (((ctl) & 0x0f) << 3) | > > > \ > > > + (((ch) & 0x38) << 12) | > > > \ > > > + ((ch) & 0x07)) > > > > GENMASK() for the bitmaps here please. Also it would look very neat by > > using FIELD_PREP() here, you can skip the bit shifts and they would be > > done by FIELD_PREP() for you. > > ok. FWIW I am testing changes to do the conversion for subsystem to use nice stuff in bitfield.h -- ~Vinod
[PATCH v2 1/5] ASoC: max98373: Fix return check for devm_regmap_init_sdw()
devm_regmap_init_sdw() returns a valid pointer on success or ERR_PTR on
failure which should be checked with IS_ERR. Also use PTR_ERR for
returning error codes.
Reported-by: Takashi Iwai
Fixes: 56a5b7910e96 ("ASoC: codecs: max98373: add SoundWire support")
Signed-off-by: Vinod Koul
---
sound/soc/codecs/max98373-sdw.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/max98373-sdw.c b/sound/soc/codecs/max98373-sdw.c
index 5fe724728e84..e4675cfff7b2 100644
--- a/sound/soc/codecs/max98373-sdw.c
+++ b/sound/soc/codecs/max98373-sdw.c
@@ -838,8 +838,8 @@ static int max98373_sdw_probe(struct sdw_slave *slave,
/* Regmap Initialization */
regmap = devm_regmap_init_sdw(slave, _sdw_regmap);
- if (!regmap)
- return -EINVAL;
+ if (IS_ERR(regmap))
+ return PTR_ERR(regmap);
return max98373_init(slave, regmap);
}
--
2.26.2
[PATCH v2 4/5] ASoC: rt715: Fix return check for devm_regmap_init_sdw()
devm_regmap_init_sdw() returns a valid pointer on success or ERR_PTR on
failure which should be checked with IS_ERR. Also use PTR_ERR for
returning error codes.
Reported-by: Takashi Iwai
Fixes: d1ede0641b05 ("ASoC: rt715: add RT715 codec driver")
Signed-off-by: Vinod Koul
---
sound/soc/codecs/rt715-sdw.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/rt715-sdw.c b/sound/soc/codecs/rt715-sdw.c
index d11b23d6b240..68a36739f1b0 100644
--- a/sound/soc/codecs/rt715-sdw.c
+++ b/sound/soc/codecs/rt715-sdw.c
@@ -527,8 +527,8 @@ static int rt715_sdw_probe(struct sdw_slave *slave,
/* Regmap Initialization */
sdw_regmap = devm_regmap_init_sdw(slave, _sdw_regmap);
- if (!sdw_regmap)
- return -EINVAL;
+ if (IS_ERR(sdw_regmap))
+ return PTR_ERR(sdw_regmap);
regmap = devm_regmap_init(>dev, NULL, >dev,
_regmap);
--
2.26.2
[PATCH v2 5/5] ASoC: rt700: Fix return check for devm_regmap_init_sdw()
devm_regmap_init_sdw() returns a valid pointer on success or ERR_PTR on
failure which should be checked with IS_ERR. Also use PTR_ERR for
returning error codes.
Reported-by: Takashi Iwai
Fixes: 7d2a5f9ae41e ("ASoC: rt700: add rt700 codec driver")
Signed-off-by: Vinod Koul
---
sound/soc/codecs/rt700-sdw.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/rt700-sdw.c b/sound/soc/codecs/rt700-sdw.c
index 4d14048d1197..1d24bf040718 100644
--- a/sound/soc/codecs/rt700-sdw.c
+++ b/sound/soc/codecs/rt700-sdw.c
@@ -452,8 +452,8 @@ static int rt700_sdw_probe(struct sdw_slave *slave,
/* Regmap Initialization */
sdw_regmap = devm_regmap_init_sdw(slave, _sdw_regmap);
- if (!sdw_regmap)
- return -EINVAL;
+ if (IS_ERR(sdw_regmap))
+ return PTR_ERR(sdw_regmap);
regmap = devm_regmap_init(>dev, NULL,
>dev, _regmap);
--
2.26.2
[PATCH v2 3/5] ASoC: rt711: Fix return check for devm_regmap_init_sdw()
devm_regmap_init_sdw() returns a valid pointer on success or ERR_PTR on
failure which should be checked with IS_ERR. Also use PTR_ERR for
returning error codes.
Reported-by: Takashi Iwai
Fixes: 320b8b0d13b8 ("ASoC: rt711: add rt711 codec driver")
Signed-off-by: Vinod Koul
---
sound/soc/codecs/rt711-sdw.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/rt711-sdw.c b/sound/soc/codecs/rt711-sdw.c
index 45b928954b58..7efff130a638 100644
--- a/sound/soc/codecs/rt711-sdw.c
+++ b/sound/soc/codecs/rt711-sdw.c
@@ -452,8 +452,8 @@ static int rt711_sdw_probe(struct sdw_slave *slave,
/* Regmap Initialization */
sdw_regmap = devm_regmap_init_sdw(slave, _sdw_regmap);
- if (!sdw_regmap)
- return -EINVAL;
+ if (IS_ERR(sdw_regmap))
+ return PTR_ERR(sdw_regmap);
regmap = devm_regmap_init(>dev, NULL,
>dev, _regmap);
--
2.26.2
[PATCH v2 2/5] ASoC: rt1308-sdw: Fix return check for devm_regmap_init_sdw()
devm_regmap_init_sdw() returns a valid pointer on success or ERR_PTR on
failure which should be checked with IS_ERR. Also use PTR_ERR for
returning error codes.
Reported-by: Takashi Iwai
Fixes: a87a6653a28c ("ASoC: rt1308-sdw: add rt1308 SdW amplifier driver")
Signed-off-by: Vinod Koul
---
sound/soc/codecs/rt1308-sdw.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/rt1308-sdw.c b/sound/soc/codecs/rt1308-sdw.c
index b0ba0d2acbdd..56e952a904a3 100644
--- a/sound/soc/codecs/rt1308-sdw.c
+++ b/sound/soc/codecs/rt1308-sdw.c
@@ -684,8 +684,8 @@ static int rt1308_sdw_probe(struct sdw_slave *slave,
/* Regmap Initialization */
regmap = devm_regmap_init_sdw(slave, _sdw_regmap);
- if (!regmap)
- return -EINVAL;
+ if (IS_ERR(regmap))
+ return PTR_ERR(regmap);
rt1308_sdw_init(>dev, regmap, slave);
--
2.26.2
[PATCH v2 0/5] ASoC: Fix return check for devm_regmap_init_sdw()
This series attempts to fix return check for devm_regmap_init_sdw() Changes in v2: - Add missing patch for rt711 - Add patch for rt700 Vinod Koul (5): ASoC: max98373: Fix return check for devm_regmap_init_sdw() ASoC: rt1308-sdw: Fix return check for devm_regmap_init_sdw() ASoC: rt711: Fix return check for devm_regmap_init_sdw() ASoC: rt715: Fix return check for devm_regmap_init_sdw() ASoC: rt700: Fix return check for devm_regmap_init_sdw() sound/soc/codecs/max98373-sdw.c | 4 ++-- sound/soc/codecs/rt1308-sdw.c | 4 ++-- sound/soc/codecs/rt700-sdw.c| 4 ++-- sound/soc/codecs/rt711-sdw.c| 4 ++-- sound/soc/codecs/rt715-sdw.c| 4 ++-- 5 files changed, 10 insertions(+), 10 deletions(-) -- 2.26.2
Re: [PATCH 4/4] ASoC: rt715: Fix return check for devm_regmap_init_sdw()
On 26-08-20, 10:09, Pierre-Louis Bossart wrote: > > > On 8/26/20 7:28 AM, Vinod Koul wrote: > > devm_regmap_init_sdw() returns a valid pointer on success or ERR_PTR on > > failure which should be checked with IS_ERR. Also use PTR_ERR for > > returning error codes. > > Do you mind changing these two additional codecs that were missed in this > series? Thanks! > > rt700-sdw.c: sdw_regmap = devm_regmap_init_sdw(slave, _sdw_regmap); > rt700-sdw.c- if (!sdw_regmap) > rt700-sdw.c- return -EINVAL; Yes missed this one > > -- > rt711-sdw.c: sdw_regmap = devm_regmap_init_sdw(slave, _sdw_regmap); > rt711-sdw.c- if (!sdw_regmap) > rt711-sdw.c- return -EINVAL; And somehow patch series is messed, I have two 2/4 but no 3/4 which patches rt711-sdw.c, will send v2 with these fixed -- ~Vinod
[PATCH 2/4] ASoC: rt1308-sdw: Fix return check for devm_regmap_init_sdw()
devm_regmap_init_sdw() returns a valid pointer on success or ERR_PTR on
failure which should be checked with IS_ERR. Also use PTR_ERR for
returning error codes.
Reported-by: Takashi Iwai
Fixes: a87a6653a28c ("ASoC: rt1308-sdw: add rt1308 SdW amplifier driver")
Signed-off-by: Vinod Koul
---
sound/soc/codecs/rt1308-sdw.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/rt1308-sdw.c b/sound/soc/codecs/rt1308-sdw.c
index b0ba0d2acbdd..56e952a904a3 100644
--- a/sound/soc/codecs/rt1308-sdw.c
+++ b/sound/soc/codecs/rt1308-sdw.c
@@ -684,8 +684,8 @@ static int rt1308_sdw_probe(struct sdw_slave *slave,
/* Regmap Initialization */
regmap = devm_regmap_init_sdw(slave, _sdw_regmap);
- if (!regmap)
- return -EINVAL;
+ if (IS_ERR(regmap))
+ return PTR_ERR(regmap);
rt1308_sdw_init(>dev, regmap, slave);
--
2.26.2
[PATCH 1/4] ASoC: max98373: Fix return check for devm_regmap_init_sdw()
devm_regmap_init_sdw() returns a valid pointer on success or ERR_PTR on
failure which should be checked with IS_ERR. Also use PTR_ERR for
returning error codes.
Reported-by: Takashi Iwai
Fixes: 56a5b7910e96 ("ASoC: codecs: max98373: add SoundWire support")
Signed-off-by: Vinod Koul
---
sound/soc/codecs/max98373-sdw.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/max98373-sdw.c b/sound/soc/codecs/max98373-sdw.c
index 5fe724728e84..e4675cfff7b2 100644
--- a/sound/soc/codecs/max98373-sdw.c
+++ b/sound/soc/codecs/max98373-sdw.c
@@ -838,8 +838,8 @@ static int max98373_sdw_probe(struct sdw_slave *slave,
/* Regmap Initialization */
regmap = devm_regmap_init_sdw(slave, _sdw_regmap);
- if (!regmap)
- return -EINVAL;
+ if (IS_ERR(regmap))
+ return PTR_ERR(regmap);
return max98373_init(slave, regmap);
}
--
2.26.2
[PATCH 4/4] ASoC: rt715: Fix return check for devm_regmap_init_sdw()
devm_regmap_init_sdw() returns a valid pointer on success or ERR_PTR on
failure which should be checked with IS_ERR. Also use PTR_ERR for
returning error codes.
Reported-by: Takashi Iwai
Fixes: d1ede0641b05 ("ASoC: rt715: add RT715 codec driver")
Signed-off-by: Vinod Koul
---
sound/soc/codecs/rt715-sdw.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/rt715-sdw.c b/sound/soc/codecs/rt715-sdw.c
index d11b23d6b240..68a36739f1b0 100644
--- a/sound/soc/codecs/rt715-sdw.c
+++ b/sound/soc/codecs/rt715-sdw.c
@@ -527,8 +527,8 @@ static int rt715_sdw_probe(struct sdw_slave *slave,
/* Regmap Initialization */
sdw_regmap = devm_regmap_init_sdw(slave, _sdw_regmap);
- if (!sdw_regmap)
- return -EINVAL;
+ if (IS_ERR(sdw_regmap))
+ return PTR_ERR(sdw_regmap);
regmap = devm_regmap_init(>dev, NULL, >dev,
_regmap);
--
2.26.2
[PATCH 2/4] ASoC: rt1308-sdw: Fix return check for devm_regmap_init_sdw()
devm_regmap_init_sdw() returns a valid pointer on success or ERR_PTR on
failure which should be checked with IS_ERR. Also use PTR_ERR for
Date: Wed, 26 Aug 2020 17:45:58 +0530
Subject: [PATCH 3/4] ASoC: rt711: Fix return check for devm_regmap_init_sdw()
devm_regmap_init_sdw() returns a valid pointer on success or ERR_PTR on
failure which should be checked with IS_ERR. Also use PTR_ERR for
returning error codes.
Reported-by: Takashi Iwai
Fixes: 320b8b0d13b8 ("ASoC: rt711: add rt711 codec driver")
Signed-off-by: Vinod Koul
---
sound/soc/codecs/rt711-sdw.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/rt711-sdw.c b/sound/soc/codecs/rt711-sdw.c
index 45b928954b58..7efff130a638 100644
--- a/sound/soc/codecs/rt711-sdw.c
+++ b/sound/soc/codecs/rt711-sdw.c
@@ -452,8 +452,8 @@ static int rt711_sdw_probe(struct sdw_slave *slave,
/* Regmap Initialization */
sdw_regmap = devm_regmap_init_sdw(slave, _sdw_regmap);
- if (!sdw_regmap)
- return -EINVAL;
+ if (IS_ERR(sdw_regmap))
+ return PTR_ERR(sdw_regmap);
regmap = devm_regmap_init(>dev, NULL,
>dev, _regmap);
--
2.26.2
Re: [PATCH 1/4] regmap: sdw: move to -EOPNOTSUPP
On 26-08-20, 12:22, Takashi Iwai wrote: > On Wed, 26 Aug 2020 12:13:01 +0200, > Mark Brown wrote: > > > > On Wed, Aug 26, 2020 at 12:09:28PM +0200, Takashi Iwai wrote: > > > Mark Brown wrote: > > > > > > checkpatch is broken. > > > > > Heh, I'm not objecting it :) > > > > > OTOH, it's also true that ENOTSUPP is no good error code if returned > > > to user-space. Unfortunately many codes (including what I wrote) use > > > this code mistakenly, and they can't be changed any longer... > > > > It's also used internally in various places without being returned to > > userspace, that's what's going on here - the regmap core has some > > specific checks for -ENOTSUPP. > > Sure, for such an internal usage any code can be used. > The question is a case like this -- where the return code might be > carried to outside. Though, looking through the grep output, all > callers simply return -EINVAL for any errors, so it doesn't matter > much for now. > > > BTW, there are a few callers of devm_regmap_init_sdw() checking the > return value with NULL. This will crash as the function returns the > error pointer, and they must be checked with IS_ERR() instead. Yes that is correct, all expect wsa codec do the incorrect thing. Will send patches for these shortly Thanks -- ~Vinod
Re: [PATCH 09/11] soundwire: intel: add dynamic debug trace for clock-stop invalid configs
On 18-08-20, 10:41, Bard Liao wrote:
> From: Pierre-Louis Bossart
>
> Detect cases where the clock is assumed to be stopped but the IP is
> not in the relevant state, and add a dynamic debug trace.
you meant a debug print..and it looks like error print below (also in title).
>
> Signed-off-by: Pierre-Louis Bossart
> Signed-off-by: Bard Liao
> ---
> drivers/soundwire/intel.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index 7c63581270fd..b82d02af3c4f 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -1964,6 +1964,11 @@ static int intel_resume_runtime(struct device *dev)
> }
> }
> } else if (!clock_stop_quirks) {
> +
> + clock_stop0 = sdw_cdns_is_clock_stop(>cdns);
> + if (!clock_stop0)
> + dev_err(dev, "%s invalid configuration, clock was not
> stopped", __func__);
> +
> ret = intel_init(sdw);
> if (ret) {
> dev_err(dev, "%s failed: %d", __func__, ret);
> --
> 2.17.1
--
~Vinod
Re: [PATCH 07/11] soundwire: intel: Only call sdw stream APIs for the first cpu_dai
On 18-08-20, 10:41, Bard Liao wrote:
> We should call these APIs once per stream. So we can only call it
> when the dai ops is invoked for the first cpu dai.
>
> Signed-off-by: Bard Liao
> Reviewed-by: Pierre-Louis Bossart
> Reviewed-by: Ranjani Sridharan
> ---
> drivers/soundwire/intel.c | 45 +--
> 1 file changed, 39 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index 89a8ad1f80e8..7c63581270fd 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -941,11 +941,13 @@ static int intel_hw_params(struct snd_pcm_substream
> *substream,
> static int intel_prepare(struct snd_pcm_substream *substream,
>struct snd_soc_dai *dai)
> {
> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> + struct snd_soc_dai *first_cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai);
> struct sdw_intel *sdw = cdns_to_intel(cdns);
> struct sdw_cdns_dma_data *dma;
> int ch, dir;
> - int ret;
> + int ret = 0;
>
> dma = snd_soc_dai_get_dma_data(dai, substream);
> if (!dma) {
> @@ -985,7 +987,13 @@ static int intel_prepare(struct snd_pcm_substream
> *substream,
> goto err;
> }
>
> - ret = sdw_prepare_stream(dma->stream);
> + /*
> + * All cpu dais belong to a stream. To ensure sdw_prepare_stream
> + * is called once per stream, we should call it only when
> + * dai = first_cpu_dai.
> + */
> + if (first_cpu_dai == dai)
> + ret = sdw_prepare_stream(dma->stream);
Hmmm why not use the one place which is unique in the card to call this,
hint machine dais are only called once.
>
> err:
> return ret;
> @@ -994,9 +1002,19 @@ static int intel_prepare(struct snd_pcm_substream
> *substream,
> static int intel_trigger(struct snd_pcm_substream *substream, int cmd,
>struct snd_soc_dai *dai)
> {
> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> + struct snd_soc_dai *first_cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> struct sdw_cdns_dma_data *dma;
> int ret;
>
> + /*
> + * All cpu dais belong to a stream. To ensure sdw_enable/disable_stream
> + * are called once per stream, we should call them only when
> + * dai = first_cpu_dai.
> + */
> + if (first_cpu_dai != dai)
> + return 0;
> +
> dma = snd_soc_dai_get_dma_data(dai, substream);
> if (!dma) {
> dev_err(dai->dev, "failed to get dma data in %s", __func__);
> @@ -1031,6 +1049,8 @@ static int intel_trigger(struct snd_pcm_substream
> *substream, int cmd,
> static int
> intel_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
> {
> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> + struct snd_soc_dai *first_cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai);
> struct sdw_intel *sdw = cdns_to_intel(cdns);
> struct sdw_cdns_dma_data *dma;
> @@ -1040,12 +1060,25 @@ intel_hw_free(struct snd_pcm_substream *substream,
> struct snd_soc_dai *dai)
> if (!dma)
> return -EIO;
>
> - ret = sdw_deprepare_stream(dma->stream);
> - if (ret) {
> - dev_err(dai->dev, "sdw_deprepare_stream: failed %d", ret);
> - return ret;
> + /*
> + * All cpu dais belong to a stream. To ensure sdw_deprepare_stream
> + * is called once per stream, we should call it only when
> + * dai = first_cpu_dai.
> + */
> + if (first_cpu_dai == dai) {
> + ret = sdw_deprepare_stream(dma->stream);
> + if (ret) {
> + dev_err(dai->dev, "sdw_deprepare_stream: failed %d",
> ret);
> + return ret;
> + }
> }
>
> + /*
> + * The sdw stream state will transition to RELEASED when stream->
> + * master_list is empty. So the stream state will transition to
> + * DEPREPARED for the first cpu-dai and to RELEASED for the last
> + * cpu-dai.
> + */
> ret = sdw_stream_remove_master(>bus, dma->stream);
> if (ret < 0) {
> dev_err(dai->dev, "remove master from stream %s failed: %d\n",
> --
> 2.17.1
--
~Vinod
Re: [PATCH 05/11] soundwire: bus: update multi-link definition with hw sync details
On 18-08-20, 10:41, Bard Liao wrote:
> From: Pierre-Louis Bossart
>
> Hardware-based synchronization is typically required when the
> bus->multi_link flag is set.
>
> On Intel platforms, when the Cadence IP is configured in 'Multi Master
> Mode', the hardware synchronization is required even when a stream
> only uses a single segment. The existing code only deal with hardware
> synchronization when a stream uses more than one segment so to remain
> backwards compatible we add a configuration threshold. For Intel cases
> this threshold will be set to one, other platforms may be able to use
> the SSP-based sync in those cases.
>
> Signed-off-by: Pierre-Louis Bossart
> Signed-off-by: Bard Liao
> ---
> include/linux/soundwire/sdw.h | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index 76052f12c9f7..9adbe4fd7980 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -827,6 +827,11 @@ struct sdw_master_ops {
> * @multi_link: Store bus property that indicates if multi links
> * are supported. This flag is populated by drivers after reading
> * appropriate firmware (ACPI/DT).
> + * @hw_sync_min_links: Number of links used by a stream above which
> + * hardware-based synchronization is required. This value is only
> + * meaningful if multi_link is set. If set to 1, hardware-based
> + * synchronization will be used even if a stream only uses a single
> + * SoundWire segment.
Soundwire spec does not say anything about multi-link so this is left to
implementer. Assuming that value of 1 would mean hw based sync will
be used even for single stream does not make sense in generic terms.
Maybe yes for Intel but may not be true for everyone?
We already use m_rt_count in code for this, so the question is why is
that not sufficient?
> */
> struct sdw_bus {
> struct device *dev;
> @@ -850,6 +855,7 @@ struct sdw_bus {
> unsigned int clk_stop_timeout;
> u32 bank_switch_timeout;
> bool multi_link;
> + int hw_sync_min_links;
> };
>
> int sdw_bus_master_add(struct sdw_bus *bus, struct device *parent,
> --
> 2.17.1
--
~Vinod
Re: [PATCH 4/4] regmap: sdw: add support for SoundWire 1.2 MBQ
On 25-08-20, 12:16, Pierre-Louis Bossart wrote:
> The SoundWire 1.1 specification only allowed for reads and writes of
> bytes. The SoundWire 1.2 specification adds a new capability to
> transfer "Multi-Byte Quantities" (MBQ) across the bus. The transfers
> still happens one-byte-at-a-time, but the update is atomic.
>
> For example when writing a 16-bit volume, the first byte transferred
> is only taken into account when the second byte is successfully
> transferred.
>
> The mechanism is symmetrical for read and writes:
> - On a read, the address of the last byte to be read is modified by
> setting the MBQ bit
> - On a write, the address of all but the last byte to be written are
> modified by setting the MBQ bit. The address for the last byte relies
> on the MBQ bit being cleared.
>
> The current definitions for MBQ-based controls in the SDCA draft
> standard are limited to 16 bits for volumes, so for now this is the
> only supported format. An update will be provided if and when support
> for 24-bit and 32-bit values is specified by the SDCA standard.
>
> One possible objection is that this code could have been handled with
> regmap-sdw.c. However this is a new spec addition not handled by every
> SoundWire 1.1 and non-SDCA device, so there's no reason to load code
> that will never be used.
>
> Also in practice it's extremely unlikely that CONFIG_REGMAP would not
> be selected with CONFIG_REGMAP_MBQ selected. However there's no
> functional dependency between the two modules so they can be selected
> separately.
Is there a reason for a new module for mbq writes, cant we do this as
part of sdw module? Driver can invoke either regmap_init_sdw() or
regmap_init_sdw_mbq()?
> +++ b/drivers/base/regmap/regmap-sdw-mbq.c
> @@ -0,0 +1,102 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright(c) 2020 Intel Corporation.
> +
> +#include
> +#include
> +#include
Curious why do you need this header?
> +#include
> +#include
> +#include
> +#include
> +#include "internal.h"
> +
> +static int regmap_sdw_mbq_write(void *context, unsigned int reg, unsigned
> int val)
> +{
> + struct device *dev = context;
> + struct sdw_slave *slave = dev_to_sdw_dev(dev);
> + int ret;
> +
> + ret = sdw_write(slave, SDW_SDCA_MBQ_CTL(reg), (val >> 8) & 0xff);
> + if (ret < 0)
> + return ret;
> +
> + return sdw_write(slave, reg, val & 0xff);
> +}
> +
> +static int regmap_sdw_mbq_read(void *context, unsigned int reg, unsigned int
> *val)
> +{
> + struct device *dev = context;
> + struct sdw_slave *slave = dev_to_sdw_dev(dev);
> + int read0;
> + int read1;
> +
> + read0 = sdw_read(slave, reg);
> + if (read0 < 0)
> + return read0;
> +
> + read1 = sdw_read(slave, SDW_SDCA_MBQ_CTL(reg));
> + if (read1 < 0)
> + return read1;
> +
> + *val = (read1 << 8) | read0;
> +
> + return 0;
> +}
> +
> +static struct regmap_bus regmap_sdw_mbq = {
> + .reg_read = regmap_sdw_mbq_read,
> + .reg_write = regmap_sdw_mbq_write,
> + .reg_format_endian_default = REGMAP_ENDIAN_LITTLE,
> + .val_format_endian_default = REGMAP_ENDIAN_LITTLE,
> +};
> +
> +static int regmap_sdw_mbq_config_check(const struct regmap_config *config)
> +{
> + /* MBQ-based controls are only 16-bits for now */
> + if (config->val_bits != 16)
> + return -EOPNOTSUPP;
> +
> + /* Registers are 32 bits wide */
> + if (config->reg_bits != 32)
> + return -EOPNOTSUPP;
> +
> + if (config->pad_bits != 0)
> + return -EOPNOTSUPP;
> +
> + return 0;
> +}
> +
> +struct regmap *__regmap_init_sdw_mbq(struct sdw_slave *sdw,
> + const struct regmap_config *config,
> + struct lock_class_key *lock_key,
> + const char *lock_name)
> +{
> + int ret;
> +
> + ret = regmap_sdw_mbq_config_check(config);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + return __regmap_init(>dev, _sdw_mbq,
> + >dev, config, lock_key, lock_name);
> +}
> +EXPORT_SYMBOL_GPL(__regmap_init_sdw_mbq);
> +
> +struct regmap *__devm_regmap_init_sdw_mbq(struct sdw_slave *sdw,
> + const struct regmap_config *config,
> + struct lock_class_key *lock_key,
> + const char *lock_name)
> +{
> + int ret;
> +
> + ret = regmap_sdw_mbq_config_check(config);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + return __devm_regmap_init(>dev, _sdw_mbq,
> + >dev, config, lock_key, lock_name);
> +}
> +EXPORT_SYMBOL_GPL(__devm_regmap_init_sdw_mbq);
> +
> +MODULE_DESCRIPTION("Regmap SoundWire Module");
This is same of sdw module, pls make this one a bit different.
--
~Vinod
Re: [PATCH 3/4] soundwire: SDCA: add helper macro to access controls
On 25-08-20, 12:16, Pierre-Louis Bossart wrote: > The upcoming SDCA (SoundWire Device Class Audio) specification defines > a hierarchical encoding to interface with Class-defined capabilities. > > The specification is not yet accessible to the general public but this > information is released with explicit permission from the MIPI Board > to avoid delays with SDCA support on Linux platforms. > > A block of 64 MBytes of register addresses are allocated to SDCA > controls, starting at address 0x4000. The 26 LSBs which identify > individual controls are set based on the following variables: > > - Function Number. An SCDA device can be split in up to 8 independent > Functions. Each of these Functions is described in the SDCA > specification, e.g. Smart Amplifier, Smart Microphone, Simple > Microphone, Jack codec, HID, etc. > > - Entity Number. Within each Function, an Entity is an identifiable > block. Up to 127 Entities are connected in a pre-defined > graph (similar to USB), with Entity0 reserved for Function-level > configurations. In contrast to USB, the SDCA spec pre-defines > Function Types, topologies, and allowed options, i.e. the degree of > freedom is not unlimited to limit the possibility of errors in > descriptors leading to software quirks. > > - Control Selector. Within each Entity, the SDCA specification defines > 48 controls such as Mute, Gain, AGC, etc, and 16 implementation > defined ones. Some Control Selectors might be used for low-level > platform setup, and other exposed to applications and users. Note > that the same Control Selector capability, e.g. Latency control, > might be located at different offsets in different entities, the > Control Selector mapping is Entity-specific. > > - Control Number. Some Control Selectors allow channel-specific values > to be set, with up to 64 channels allowed. This is mostly used for > volume control. > > - Current/Next values. Some Control Selectors are > 'Dual-Ranked'. Software may either update the Current value directly > for immediate effect. Alternatively, software may write into the > 'Next' values and update the SoundWire 1.2 'Commit Groups' register > to copy 'Next' values into 'Current' ones in a synchronized > manner. This is different from bank switching which is typically > used to change the bus configuration only. > > - MBQ. the Multi-Byte Quantity bit is used to provide atomic updates > when accessing more that one byte, for example a 16-bit volume > control would be updated consistently, the intermediate values > mixing old MSB with new LSB are not applied. > > These 6 parameters are used to build a 32-bit address to access the > desired Controls. Because of address range, paging is required, but > the most often used parameter values are placed in the lower 16 bits > of the address. This helps to keep the paging registers constant while > updating Controls for a specific Device/Function. > > Reviewed-by: Rander Wang > Reviewed-by: Guennadi Liakhovetski > Reviewed-by: Kai Vehmanen > Signed-off-by: Pierre-Louis Bossart > --- > include/linux/soundwire/sdw_registers.h | 13 + > 1 file changed, 13 insertions(+) > > diff --git a/include/linux/soundwire/sdw_registers.h > b/include/linux/soundwire/sdw_registers.h > index 5d3c271af7d1..906dadda7387 100644 > --- a/include/linux/soundwire/sdw_registers.h > +++ b/include/linux/soundwire/sdw_registers.h > @@ -305,4 +305,17 @@ > #define SDW_CASC_PORT_MASK_INTSTAT3 1 > #define SDW_CASC_PORT_REG_OFFSET_INTSTAT32 > > +/* v1.2 device - SDCA address mapping */ Can you please add description of bits used by each field here, something like we have done for DevId > +#define SDW_SDCA_CTL(fun, ent, ctl, ch) (BIT(30) | > \ > + (((fun) & 0x7) << 22) | > \ > + (((ent) & 0x40) << 15) | > \ > + (((ent) & 0x3f) << 7) | > \ > + (((ctl) & 0x30) << 15) | > \ > + (((ctl) & 0x0f) << 3) | > \ > + (((ch) & 0x38) << 12) | > \ > + ((ch) & 0x07)) GENMASK() for the bitmaps here please. Also it would look very neat by using FIELD_PREP() here, you can skip the bit shifts and they would be done by FIELD_PREP() for you. > -- > 2.25.1 -- ~Vinod
Re: [PATCH v2] soundwire: intel: fix intel_suspend/resume defined but not used warning
On 24-08-20, 21:32, Bard Liao wrote: > When CONFIG_PM_SLEEP is not defined, GCC throws compilation warnings: > > drivers/soundwire/intel.c:1799:12: warning: ‘intel_resume’ defined but not > used [-Wunused-function] > static int intel_resume(struct device *dev) > ^~~~ > drivers/soundwire/intel.c:1683:12: warning: ‘intel_suspend’ defined but not > used [-Wunused-function] > static int intel_suspend(struct device *dev) > ^ > > Fix by using __maybe_unused macro. Applied, thanks -- ~Vinod
Re: [PATCH 1/3] dt-bindings: dmaengine: Document qcom,gpi dma binding
On 25-08-20, 20:21, Vinod Koul wrote:
> Hey Rob,
>
> On 24-08-20, 11:40, Rob Herring wrote:
> > On Mon, 24 Aug 2020 14:17:10 +0530, Vinod Koul wrote:
> > > Add devicetree binding documentation for GPI DMA controller
> > > implemented on Qualcomm SoCs
> > >
> > > Signed-off-by: Vinod Koul
> > > ---
> > > .../devicetree/bindings/dma/qcom-gpi.yaml | 87 +++
> > > 1 file changed, 87 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/dma/qcom-gpi.yaml
> > >
> >
> >
> > My bot found errors running 'make dt_binding_check' on your patch:
> >
> > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/dma/qcom-gpi.yaml:
> > properties:qcom,ev-factor: {'description': 'Event ring transfer size
> > compare to channel transfer ring. Event ring length = ev-factor * transfer
> > ring size', 'maxItems': 1} is not valid under any of the given schemas
> > (Possible causes of the failure):
> >
> > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/dma/qcom-gpi.yaml:
> > properties:qcom,ev-factor: 'not' is a required property
>
> Okay updating dt-schema I do see this, now the question is what is this
> and what does it mean ;-) I am not sure I comprehend the error message.
> I see this for all the new properties I added as required for this
> device node
Okay I think I have figured it out, I need to provide ref to
/schemas/types.yaml#definitions/uint32 for this to work, which does
makes sense to me.
qcom,max-num-gpii:
$ref: /schemas/types.yaml#definitions/uint32
maxItems: 1
description:
Number of GPII instances
Looks good to schema tool
--
~Vinod
Re: [RESEND PATCH v2 0/6] dmaengine: axi-dmac: add support for reading bus attributes from register
On 25-08-20, 18:19, Alexandru Ardelean wrote: > The series adds support for reading the DMA bus attributes from the > INTERFACE_DESCRIPTION (0x10) register. > > The first 5 changes are a bit of rework prior to adding the actual > change in patch 6, as things need to be shifted around a bit, to enable > the clock to be enabled earlier, to be able to read the version > register. Applied, thanks -- ~Vinod
Re: [PATCH 1/3] dt-bindings: dmaengine: Document qcom,gpi dma binding
Hey Rob,
On 24-08-20, 11:40, Rob Herring wrote:
> On Mon, 24 Aug 2020 14:17:10 +0530, Vinod Koul wrote:
> > Add devicetree binding documentation for GPI DMA controller
> > implemented on Qualcomm SoCs
> >
> > Signed-off-by: Vinod Koul
> > ---
> > .../devicetree/bindings/dma/qcom-gpi.yaml | 87 +++
> > 1 file changed, 87 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/dma/qcom-gpi.yaml
> >
>
>
> My bot found errors running 'make dt_binding_check' on your patch:
>
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/dma/qcom-gpi.yaml:
> properties:qcom,ev-factor: {'description': 'Event ring transfer size compare
> to channel transfer ring. Event ring length = ev-factor * transfer ring
> size', 'maxItems': 1} is not valid under any of the given schemas (Possible
> causes of the failure):
>
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/dma/qcom-gpi.yaml:
> properties:qcom,ev-factor: 'not' is a required property
Okay updating dt-schema I do see this, now the question is what is this
and what does it mean ;-) I am not sure I comprehend the error message.
I see this for all the new properties I added as required for this
device node
>
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/dma/qcom-gpi.yaml:
> properties:qcom,gpii-mask: {'description': 'Bitmap of supported GPII
> instances for OS', 'maxItems': 1} is not valid under any of the given schemas
> (Possible causes of the failure):
>
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/dma/qcom-gpi.yaml:
> properties:qcom,gpii-mask: 'not' is a required property
>
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/dma/qcom-gpi.yaml:
> properties:qcom,max-num-gpii: {'description': 'Maximum number of GPII
> instances available', 'maxItems': 1} is not valid under any of the given
> schemas (Possible causes of the failure):
>
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/dma/qcom-gpi.yaml:
> properties:qcom,max-num-gpii: 'not' is a required property
>
> ./Documentation/devicetree/bindings/dma/qcom-gpi.yaml: $id: relative
> path/filename doesn't match actual path or filename
> expected: http://devicetree.org/schemas/dma/qcom-gpi.yaml#
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/dma/qcom-gpi.yaml:
> ignoring, error in schema: properties: qcom,max-num-gpii
> warning: no schema found in file:
> ./Documentation/devicetree/bindings/dma/qcom-gpi.yaml
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/dma/qcom-gpi.example.dt.yaml:
> example-0: dma@80:reg:0: [0, 8388608, 0, 393216] is too long
> From schema:
> /usr/local/lib/python3.8/dist-packages/dtschema/schemas/reg.yaml
>
>
> See https://patchwork.ozlabs.org/patch/1350170
>
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure dt-schema is up to date:
>
> pip3 install git+https://github.com/devicetree-org/dt-schema.git@master
> --upgrade
>
> Please check and re-submit.
--
~Vinod
Re: [PATCH v2 0/6] dmaengine: axi-dmac: add support for reading bus attributes from register
On 25-08-20, 15:48, Alexandru Ardelean wrote: > The series adds support for reading the DMA bus attributes from the > INTERFACE_DESCRIPTION (0x10) register. > > The first 5 changes are a bit of rework prior to adding the actual > change in patch 6, as things need to be shifted around a bit, to enable > the clock to be enabled earlier, to be able to read the version > register. Not sure what happened, I see two sets of patches in this series, maybe duplicate..? Better to resend a clean version.. Even PW shows 12 patches https://patchwork.kernel.org/project/linux-dmaengine/list/ Thanks > > Changelog v1 -> v2: > * fixed error-exit paths for the clock move patch > i.e. 'dmaengine: axi-dmac: move clock enable earlier' > * fixed error-exit path for patch > 'axi-dmac: wrap channel parameter adjust into function' > * added patch 'dmaengine: axi-dmac: move active_descs list init after > device-tree init' > the list of active_descs can be moved a bit lower in the init/probe > > Alexandru Ardelean (6): > dmaengine: axi-dmac: move version read in probe > dmaengine: axi-dmac: move active_descs list init after device-tree > init > dmaengine: axi-dmac: move clock enable earlier > dmaengine: axi-dmac: wrap entire dt parse in a function > dmaengine: axi-dmac: wrap channel parameter adjust into function > dmaengine: axi-dmac: add support for reading bus attributes from > registers > > drivers/dma/dma-axi-dmac.c | 138 - > 1 file changed, 107 insertions(+), 31 deletions(-) > > -- > 2.17.1 -- ~Vinod
Re: [PATCH 2/5] dmaengine: axi-dmac: move clock enable earlier
On 19-08-20, 10:16, Alexandru Ardelean wrote: > The clock may also be required to read registers from the IP core (if it is > provided and the driver needs to control it). > So, move it earlier in the probe. > > Signed-off-by: Alexandru Ardelean > --- > drivers/dma/dma-axi-dmac.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/dma/dma-axi-dmac.c b/drivers/dma/dma-axi-dmac.c > index 088c79137398..07665c60c21b 100644 > --- a/drivers/dma/dma-axi-dmac.c > +++ b/drivers/dma/dma-axi-dmac.c > @@ -850,6 +850,10 @@ static int axi_dmac_probe(struct platform_device *pdev) > if (IS_ERR(dmac->clk)) > return PTR_ERR(dmac->clk); > > + ret = clk_prepare_enable(dmac->clk); > + if (ret < 0) > + return ret; > + > INIT_LIST_HEAD(>chan.active_descs); Change is fine, but then you need to jump to err_clk_disable in few place below this and not return error > > of_channels = of_get_child_by_name(pdev->dev.of_node, "adi,channels"); > @@ -892,10 +896,6 @@ static int axi_dmac_probe(struct platform_device *pdev) > dmac->chan.vchan.desc_free = axi_dmac_desc_free; > vchan_init(>chan.vchan, dma_dev); > > - ret = clk_prepare_enable(dmac->clk); > - if (ret < 0) > - return ret; > - > version = axi_dmac_read(dmac, ADI_AXI_REG_VERSION); > > ret = axi_dmac_detect_caps(dmac, version); > -- > 2.17.1 -- ~Vinod
Re: [PATCH 3/5] dmaengine: axi-dmac: wrap entire dt parse in a function
On 19-08-20, 10:16, Alexandru Ardelean wrote:
> All these attributes will be read from registers in newer core versions, so
> just wrap the logic into a function.
>
> Signed-off-by: Alexandru Ardelean
> ---
> drivers/dma/dma-axi-dmac.c | 39 --
> 1 file changed, 25 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/dma/dma-axi-dmac.c b/drivers/dma/dma-axi-dmac.c
> index 07665c60c21b..473c4a159c89 100644
> --- a/drivers/dma/dma-axi-dmac.c
> +++ b/drivers/dma/dma-axi-dmac.c
> @@ -774,6 +774,28 @@ static int axi_dmac_parse_chan_dt(struct device_node
> *of_chan,
> return 0;
> }
>
> +static int axi_dmac_parse_dt(struct device *dev, struct axi_dmac *dmac)
> +{
> + struct device_node *of_channels, *of_chan;
> + int ret;
> +
> + of_channels = of_get_child_by_name(dev->of_node, "adi,channels");
> + if (of_channels == NULL)
> + return -ENODEV;
> +
> + for_each_child_of_node(of_channels, of_chan) {
> + ret = axi_dmac_parse_chan_dt(of_chan, >chan);
> + if (ret) {
> + of_node_put(of_chan);
> + of_node_put(of_channels);
> + return -EINVAL;
> + }
> + }
> + of_node_put(of_channels);
> +
> + return 0;
> +}
> +
> static int axi_dmac_detect_caps(struct axi_dmac *dmac, unsigned int version)
> {
> struct axi_dmac_chan *chan = >chan;
> @@ -823,7 +845,6 @@ static int axi_dmac_detect_caps(struct axi_dmac *dmac,
> unsigned int version)
>
> static int axi_dmac_probe(struct platform_device *pdev)
> {
> - struct device_node *of_channels, *of_chan;
> struct dma_device *dma_dev;
> struct axi_dmac *dmac;
> struct resource *res;
> @@ -856,19 +877,9 @@ static int axi_dmac_probe(struct platform_device *pdev)
>
> INIT_LIST_HEAD(>chan.active_descs);
>
> - of_channels = of_get_child_by_name(pdev->dev.of_node, "adi,channels");
> - if (of_channels == NULL)
> - return -ENODEV;
> -
> - for_each_child_of_node(of_channels, of_chan) {
> - ret = axi_dmac_parse_chan_dt(of_chan, >chan);
> - if (ret) {
> - of_node_put(of_chan);
> - of_node_put(of_channels);
> - return -EINVAL;
> - }
> - }
> - of_node_put(of_channels);
> + ret = axi_dmac_parse_dt(>dev, dmac);
> + if (ret < 0)
this need to jump to err_clk_disable
> + return ret;
>
> pdev->dev.dma_parms = >dma_parms;
> dma_set_max_seg_size(>dev, UINT_MAX);
> --
> 2.17.1
--
~Vinod
Re: [PATCH v5 1/2] dt-bindings: dma: Add bindings for intel LGM SOC
On 18-08-20, 15:00, Reddy, MallikarjunaX wrote: > > > + > > > +intel,chans: > > > + $ref: /schemas/types.yaml#/definitions/uint32-array > > > + description: > > > + The channels included on this port. Format is channel > > > start > > > + number and how many channels on this port. > > Why does this need to be in DT? This all seems like it can be in the dma > > cells for each client. > (*ABC) > Yes. We need this. > for dma0(lgm-cdma) old SOC supports 16 channels and the new SOC supports 22 > channels. and the logical channel mapping for the peripherals also differ > b/w old and new SOCs. > > Because of this hardware limitation we are trying to configure the total > channels and port-channel mapping dynamically from device tree. > > based on port name we are trying to configure the default values for > different peripherals(ports). > Example: burst length is not same for all ports, so using port name to do > default configurations. Sorry that does not make sense to me, why not specify the values to be used here instead of defining your own name scheme! Only older soc it should create 16 channels and new 22 (hint this is hw description so perfectly okay to specify in DT or in using driver_data and compatible for each version > > > > > + > > > + required: > > > +- reg > > > +- intel,name > > > +- intel,chans > > > + > > > + > > > + ldma-channels: > > > +type: object > > > +description: > > > + This sub-node must contain a sub-node for each DMA channel. > > > +properties: > > > + '#address-cells': > > > +const: 1 > > > + '#size-cells': > > > +const: 0 > > > + > > > +patternProperties: > > > + "^ldma-channels@[0-15]+$": > > > + type: object > > > + > > > + properties: > > > +reg: > > > + items: > > > +- enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, > > > 14, 15] > > > + description: > > > + Which channel this node refers to. > > > + > > > +intel,desc_num: > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + description: > > > + Per channel maximum descriptor number. The max value is > > > 255. > > > + > > > +intel,hdr-mode: > > > + $ref: /schemas/types.yaml#/definitions/uint32-array > > > + description: > > > + The first parameter is header mode size, the second > > > + parameter is checksum enable or disable. If enabled, > > > + header mode size is ignored. If disabled, header mode > > > + size must be provided. > > > + > > > +intel,hw-desc: > > > + $ref: /schemas/types.yaml#/definitions/uint32-array > > > + description: > > > + Per channel dma hardware descriptor configuration. > > > + The first parameter is descriptor physical address and > > > the > > > + second parameter hardware descriptor number. > > Again, this all seems like per client information for dma cells. > Ok, if we move all these attributes to 'dmas' then 'dma-channels' child > node is not needed in dtsi. > #dma-cells number i am already using 7. If we move all these attributes to > 'dmas' then integer cells will increase. > > Is there any limitation in using a number of integer cells & as determined > by the #dma-cells property? No I dont think there is but it needs to make sense :-) -- ~Vinod
Re: [PATCH v5 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA controller
On 24-08-20, 13:11, Sanjay R Mehta wrote:
> Apologies for my delayed response.
>
> On 7/3/2020 12:48 PM, Vinod Koul wrote:
> > [CAUTION: External Email]
> >
> > On 16-06-20, 20:11, Sanjay R Mehta wrote:
> >
> >> +static int pt_core_execute_cmd(struct ptdma_desc *desc,
> >> +struct pt_cmd_queue *cmd_q)
> >> +{
> >> + __le32 *mp;
> >> + u32 *dp;
> >> + u32 tail;
> >> + int i;
> >
> > no tabs, spaces pls
> Sure, will fix in the next version of patch.
Also, please make sure you run checkpatch.pl with --strict option, that
will help out reducing the churn here
Thanks
--
~Vinod
Re: [PATCH v2] drivers/dma/dma-jz4780: Fix race condition between probe and irq handler
On 21-08-20, 09:14, madhuparnabhowmi...@gmail.com wrote: > From: Madhuparna Bhowmik > > In probe, IRQ is requested before zchan->id is initialized which can be > read in the irq handler. Hence, shift request irq after other initializations > complete. Applied, thanks -- ~Vinod
Re: [PATCH] dmaengine: pl330: fix instruction dump formatting
On 13-08-20, 22:41, Łukasz Stelmach wrote:
> Instruction dump uses two printk() in a row to print one instruction. Use
> KERN_CONT to prevent breaking the output in the middle.
Applied, thanks
>
> Signed-off-by: Łukasz Stelmach
> ---
> drivers/dma/pl330.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 88b884cbb7c1..e1af6a470453 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -254,7 +254,7 @@ enum pl330_byteswap {
> static unsigned cmd_line;
> #define PL330_DBGCMD_DUMP(off, x...) do { \
> printk("%x:", cmd_line); \
> - printk(x); \
> + printk(KERN_CONT x); \
> cmd_line += off; \
> } while (0)
> #define PL330_DBGMC_START(addr) (cmd_line = addr)
> --
> 2.26.2
--
~Vinod
Re: [RFC PATCH 2/3] dmaengine: add peripheral configuration
Hi Peter,
On 25-08-20, 11:00, Peter Ujfalusi wrote:
> Hi Vinod,
>
> On 25/08/2020 10.10, Vinod Koul wrote:
> >>> /**
> >>> * struct dma_slave_config - dma slave channel runtime config
> >>> * @direction: whether the data shall go in or out on this slave
> >>> @@ -418,6 +485,10 @@ enum dma_slave_buswidth {
> >>> * @slave_id: Slave requester id. Only valid for slave channels. The dma
> >>> * slave peripheral will have unique id as dma requester which need to be
> >>> * pass as slave config.
> >>> + * @peripheral: type of peripheral to DMA to/from
> >>> + * @set_config: set peripheral config
> >>> + * @spi: peripheral config for spi
> >>> + * @:i2c peripheral config for i2c
> >>> *
> >>> * This struct is passed in as configuration data to a DMA engine
> >>> * in order to set up a certain channel for DMA transport at runtime.
> >>> @@ -443,6 +514,10 @@ struct dma_slave_config {
> >>> u32 dst_port_window_size;
> >>> bool device_fc;
> >>> unsigned int slave_id;
> >>> + enum dmaengine_peripheral peripheral;
> >>> + u8 set_config;
> >>> + struct dmaengine_spi_config spi;
> >>> + struct dmaengine_i2c_config i2c;
> >>
> >> Would it be possible to reuse one of the existing feature already
> >> supported by DMAengine?
> >> We have DMA_PREP_CMD to give a command instead of a real transfer:
> >> dmaengine_prep_slave_single(tx_chan, config_data, config_len,
> >>DMA_MEM_TO_DEV, DMA_PREP_CMD);
> >> dmaengine_prep_slave_single(tx_chan, tx_buff, tx_len, DMA_MEM_TO_DEV,
> >>DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> >> dma_async_issue_pending(tx_chan);
> >>
> >> or the metadata support:
> >> tx = dmaengine_prep_slave_single(tx_chan, tx_buff, tx_len,
> >> DMA_MEM_TO_DEV,
> >> DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> >> dmaengine_desc_attach_metadata(tx, config_data, config_len);
> >> dma_async_issue_pending(tx_chan);
> >>
> >> By reading the driver itself, it is not clear if you always need to send
> >> the config for TX, or only when the config is changing and what happens
> >> if the first transfer (for SPI, since that is the only implemented one)
> >> is RX, when you don't send config at all...
> >
> > So this config is sent to driver everytime before the prep call (can be
> > optimized to once if we have similar transfers in queue).
>
> I see that you queue the TREs in the prep callback.
Yes
> > This config is used to create the configuration passed to dmaengine
> > which is used to actually program both dmaengine as well as peripheral
> > registers (i2c/spi/etc), so we need a way to pass the spi/i2c config.
>
> But do you need to send it with each DMA_MEM_TO_DEV or only once?
> DMA_DEV_TO_MEM does not set the config, so I assume you must have one TX
> to initialize the peripheral as the first transfer.
Correct and we do transfers on same without sending configuration again.
> > I think prep cmd can be used to send this data, I do not see any issues
> > with that, it would work if we want to go that route.
>
> The only thing which might be an issue is that with the DMA_PREP_CMD the
> config_data is dma_addr_t (via dmaengine_prep_slave_single).
Yes I came to same conclusion
> > I did have a prototype with metadata but didnt work very well, the
> > problem is it assumes metadata for tx/rx but here i send the data
> > everytime from client data.
>
> Yes, the intended use case for metadata (per descriptor!) is for
> channels where each transfer might have different metadata needed for
> the given transfer (tx/rx).
>
> In your case you have semi static peripheral configuration data, which
> is not really changing between transfers.
>
> A compromise would be to add:
> void *peripheral_config;
> to the dma_slave_config, move the set_config inside of the device
> specific struct you are passing from a client to the core?
That sounds more saner to me and uses existing interfaces cleanly. I
think I like this option ;-)
> >> I'm concerned about the size increase of dma_slave_config (it grows by
> >>> 30 bytes) and for DMAs with hundreds of channels (UDMA) it will add up
> >> to a sizeable amount.
> >
> > I agree that is indeed a valid concern, that is the reason I tagged this
> > as a RFC patch ;-)
> >
> > I see the prep_cmd is a better approach for this, anyone else has better
> > suggestions?
> >
> > Thanks for looking in.
> >
>
> - Péter
>
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
--
~Vinod
Re: [PATCH] dmaengine: pl330: Fix burst length if burst size is smaller than bus width
On 25-08-20, 08:46, Marek Szyprowski wrote:
> Move the burst len fixup after setting the generic value for it. This
> finally enables the fixup introduced by commit 137bd11090d8 ("dmaengine:
> pl330: Align DMA memcpy operations to MFIFO width"), which otherwise was
> overwritten by the generic value.
Applied, thanks
--
~Vinod
Re: [RFC PATCH 2/3] dmaengine: add peripheral configuration
Hello Peter,
On 25-08-20, 09:52, Peter Ujfalusi wrote:
> Hi Vinod,
>
> On 24/08/2020 11.47, Vinod Koul wrote:
> > Some complex dmaengine controllers have capability to program the
> > peripheral device, so pass on the peripheral configuration as part of
> > dma_slave_config
> >
> > Signed-off-by: Vinod Koul
> > ---
> > include/linux/dmaengine.h | 75 +++
> > 1 file changed, 75 insertions(+)
> >
> > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> > index 6fbd5c99e30c..264643ca9209 100644
> > --- a/include/linux/dmaengine.h
> > +++ b/include/linux/dmaengine.h
> > @@ -380,6 +380,73 @@ enum dma_slave_buswidth {
> > DMA_SLAVE_BUSWIDTH_64_BYTES = 64,
> > };
> >
> > +/**
> > + * enum spi_transfer_cmd - spi transfer commands
> > + */
> > +enum spi_transfer_cmd {
> > + SPI_TX = 1,
> > + SPI_RX,
> > + SPI_DUPLEX,
> > +};
> > +
> > +/**
> > + * struct dmaengine_spi_config - spi config for peripheral
> > + *
> > + * @loopback_en: spi loopback enable when set
> > + * @clock_pol: clock polarity
> > + * @data_pol: data polarity
> > + * @pack_en: process tx/rx buffers as packed
> > + * @word_len: spi word length
> > + * @clk_div: source clock divider
> > + * @clk_src: serial clock
> > + * @cmd: spi cmd
> > + * @cs: chip select toggle
> > + * @rx_len: receive length for spi buffer
> > + */
> > +struct dmaengine_spi_config {
> > + u8 loopback_en;
> > + u8 clock_pol;
> > + u8 data_pol;
> > + u8 pack_en;
> > + u8 word_len;
> > + u32 clk_div;
> > + u32 clk_src;
> > + u8 fragmentation;
> > + enum spi_transfer_cmd cmd;
> > + u8 cs;
> > + u32 rx_len;
> > +};
> > +
> > +/**
> > + * struct dmaengine_i2c_config - i2c config for peripheral
> > + *
> > + * @pack_enable: process tx/rx buffers as packed
> > + * @cycle_count: clock cycles to be sent
> > + * @high_count: high period of clock
> > + * @low_count: low period of clock
> > + * @clk_div: source clock divider
> > + * @addr: i2c bus address
> > + * @strech: strech the clock at eot
> > + * @op: i2c cmd
> > + */
> > +struct dmaengine_i2c_config {
> > + u8 pack_enable;
> > + u8 cycle_count;
> > + u8 high_count;
> > + u8 low_count;
> > + u16 clk_div;
> > + u8 addr;
> > + u8 strech;
> > + u8 op;
> > +};
> > +
> > +enum dmaengine_peripheral {
> > + DMAENGINE_PERIPHERAL_SPI,
> > + DMAENGINE_PERIPHERAL_I2C,
> > + DMAENGINE_PERIPHERAL_UART,
> > + DMAENGINE_PERIPHERAL_LAST = DMAENGINE_PERIPHERAL_UART,
> > +};
> > +
> > /**
> > * struct dma_slave_config - dma slave channel runtime config
> > * @direction: whether the data shall go in or out on this slave
> > @@ -418,6 +485,10 @@ enum dma_slave_buswidth {
> > * @slave_id: Slave requester id. Only valid for slave channels. The dma
> > * slave peripheral will have unique id as dma requester which need to be
> > * pass as slave config.
> > + * @peripheral: type of peripheral to DMA to/from
> > + * @set_config: set peripheral config
> > + * @spi: peripheral config for spi
> > + * @:i2c peripheral config for i2c
> > *
> > * This struct is passed in as configuration data to a DMA engine
> > * in order to set up a certain channel for DMA transport at runtime.
> > @@ -443,6 +514,10 @@ struct dma_slave_config {
> > u32 dst_port_window_size;
> > bool device_fc;
> > unsigned int slave_id;
> > + enum dmaengine_peripheral peripheral;
> > + u8 set_config;
> > + struct dmaengine_spi_config spi;
> > + struct dmaengine_i2c_config i2c;
>
> Would it be possible to reuse one of the existing feature already
> supported by DMAengine?
> We have DMA_PREP_CMD to give a command instead of a real transfer:
> dmaengine_prep_slave_single(tx_chan, config_data, config_len,
> DMA_MEM_TO_DEV, DMA_PREP_CMD);
> dmaengine_prep_slave_single(tx_chan, tx_buff, tx_len, DMA_MEM_TO_DEV,
> DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> dma_async_issue_pending(tx_chan);
>
> or the metadata support:
> tx = dmaengine_prep_slave_single(tx_chan, tx_buff, tx_len,
>DMA_MEM_TO_DEV,
>DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> dmaengine_desc_attach_metadata(tx, config_data, config_len);
> dma_async_issue_pending(tx_chan);
>
> By reading the dr
Re: drivers/dma/pl330.c:2981:9: warning: Identical condition 'ret', second condition is always false
On 24-08-20, 15:50, Marek Szyprowski wrote: > Hi Vinod, > > On 08.08.2020 14:59, Vinod Koul wrote: > > On 08-08-20, 10:53, kernel test robot wrote: > >> tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git > >> master > >> head: 30185b69a2d533c4ba6ca926b8390ce7de495e29 > >> commit: a39cddc9e3775100100a4272feed64faac831be9 dmaengine: pl330: Drop > >> boilerplate code for suspend/resume > >> date: 8 months ago > >> compiler: aarch64-linux-gcc (GCC) 9.3.0 > >> > >> If you fix the issue, kindly add following tag as appropriate > >> Reported-by: kernel test robot > >> > >> > >> cppcheck warnings: (new ones prefixed by >>) > >> > >>>> drivers/dma/pl330.c:2981:9: warning: Identical condition 'ret', second > >>>> condition is always false [identicalConditionAfterEarlyExit] > >> return ret; > >> ^ > >> drivers/dma/pl330.c:2976:6: note: first condition > >> if (ret) > >> ^ > >> drivers/dma/pl330.c:2981:9: note: second condition > >> return ret; > > This one could be fixed by making this as return 0, but is harmless > > Right, with CONFIG_PM disabled, pm_runtime_force_resume() is noop, what > causes the above warning. > > >> ^ > >>>> drivers/dma/pl330.c:2798:23: warning: Variable 'desc->rqcfg.brst_len' is > >>>> reassigned a value before the old one has been used. > >>>> [redundantAssignment] > >> desc->rqcfg.brst_len = get_burst_len(desc, len); > >> ^ > >> drivers/dma/pl330.c:2796:24: note: Variable 'desc->rqcfg.brst_len' is > >> reassigned a value before the old one has been used. > >> desc->rqcfg.brst_len = 1; > >>^ > >> drivers/dma/pl330.c:2798:23: note: Variable 'desc->rqcfg.brst_len' is > >> reassigned a value before the old one has been used. > >> desc->rqcfg.brst_len = get_burst_len(desc, len); > > This one actually seems like a bug. Reading the code I think > > get_burst_len() should be called first before checking if burst size is > > smaller and setting to 1 in that case > > > > Sugar Zhang, Marek Szyprowski can you folks check this? > > Indeed. It look that the commit 137bd11090d89b added > desc->rqcfg.brst_len = 1 assignment before the desc->rqcfg.brst_len is > set. Maybe this was a result of the broken rebase or so. No idea. It > makes sense to switch the order and call desc->rqcfg.brst_len = > get_burst_len(desc, len) first. I can send a patch if you want. That would be great Marek Thanks -- ~Vinod
[PATCH 1/3] dt-bindings: dmaengine: Document qcom,gpi dma binding
Add devicetree binding documentation for GPI DMA controller
implemented on Qualcomm SoCs
Signed-off-by: Vinod Koul
---
.../devicetree/bindings/dma/qcom-gpi.yaml | 87 +++
1 file changed, 87 insertions(+)
create mode 100644 Documentation/devicetree/bindings/dma/qcom-gpi.yaml
diff --git a/Documentation/devicetree/bindings/dma/qcom-gpi.yaml
b/Documentation/devicetree/bindings/dma/qcom-gpi.yaml
new file mode 100644
index ..c56d601ad2d6
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/qcom-gpi.yaml
@@ -0,0 +1,87 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma/gpi-dma.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Technologies Inc GPI DMA controller
+
+description: |
+ QCOM GPI DMA controller provides DMA capabilities for
+ peripheral buses such as I2C, UART, and SPI.
+
+maintainers:
+ - Vinod Koul
+
+allOf:
+ - $ref: "dma-controller.yaml#"
+
+properties:
+ compatible:
+enum:
+ - qcom,gpi-dma
+
+ reg:
+maxItems: 1
+
+ interrupts:
+description:
+ Interrupt lines for each GPII instance
+maxItems: 14
+
+ qcom,max-num-gpii:
+description:
+ Maximum number of GPII instances available
+maxItems: 1
+
+ "#dma-cells":
+const: 1
+
+ qcom,gpii-mask:
+description:
+ Bitmap of supported GPII instances for OS
+maxItems: 1
+
+ qcom,ev-factor:
+description:
+ Event ring transfer size compare to channel transfer ring. Event
+ring length = ev-factor * transfer ring size
+maxItems: 1
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - qcom,max-num-gpii
+ - qcom,gpii-mask
+ - qcom,ev-factor
+ - "#dma-cells"
+
+unevaluatedProperties: false
+
+examples:
+ - |
+#include
+gpi_dma0: dma@80 {
+#dma-cells = <5>;
+compatible = "qcom,gpi-dma";
+reg = <0 0x0080 0 0x6>;
+qcom,max-num-gpii = <13>;
+qcom,gpii-mask = <0xfa>;
+qcom,ev-factor = <2>;
+interrupts = ,
+ ,
+ ,
+ ,
+ ,
+ ,
+ ,
+ ,
+ ,
+ ,
+ ,
+ ,
+ ;
+};
+
+...
--
2.26.2
[RFC PATCH 2/3] dmaengine: add peripheral configuration
Some complex dmaengine controllers have capability to program the
peripheral device, so pass on the peripheral configuration as part of
dma_slave_config
Signed-off-by: Vinod Koul
---
include/linux/dmaengine.h | 75 +++
1 file changed, 75 insertions(+)
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 6fbd5c99e30c..264643ca9209 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -380,6 +380,73 @@ enum dma_slave_buswidth {
DMA_SLAVE_BUSWIDTH_64_BYTES = 64,
};
+/**
+ * enum spi_transfer_cmd - spi transfer commands
+ */
+enum spi_transfer_cmd {
+ SPI_TX = 1,
+ SPI_RX,
+ SPI_DUPLEX,
+};
+
+/**
+ * struct dmaengine_spi_config - spi config for peripheral
+ *
+ * @loopback_en: spi loopback enable when set
+ * @clock_pol: clock polarity
+ * @data_pol: data polarity
+ * @pack_en: process tx/rx buffers as packed
+ * @word_len: spi word length
+ * @clk_div: source clock divider
+ * @clk_src: serial clock
+ * @cmd: spi cmd
+ * @cs: chip select toggle
+ * @rx_len: receive length for spi buffer
+ */
+struct dmaengine_spi_config {
+ u8 loopback_en;
+ u8 clock_pol;
+ u8 data_pol;
+ u8 pack_en;
+ u8 word_len;
+ u32 clk_div;
+ u32 clk_src;
+ u8 fragmentation;
+ enum spi_transfer_cmd cmd;
+ u8 cs;
+ u32 rx_len;
+};
+
+/**
+ * struct dmaengine_i2c_config - i2c config for peripheral
+ *
+ * @pack_enable: process tx/rx buffers as packed
+ * @cycle_count: clock cycles to be sent
+ * @high_count: high period of clock
+ * @low_count: low period of clock
+ * @clk_div: source clock divider
+ * @addr: i2c bus address
+ * @strech: strech the clock at eot
+ * @op: i2c cmd
+ */
+struct dmaengine_i2c_config {
+ u8 pack_enable;
+ u8 cycle_count;
+ u8 high_count;
+ u8 low_count;
+ u16 clk_div;
+ u8 addr;
+ u8 strech;
+ u8 op;
+};
+
+enum dmaengine_peripheral {
+ DMAENGINE_PERIPHERAL_SPI,
+ DMAENGINE_PERIPHERAL_I2C,
+ DMAENGINE_PERIPHERAL_UART,
+ DMAENGINE_PERIPHERAL_LAST = DMAENGINE_PERIPHERAL_UART,
+};
+
/**
* struct dma_slave_config - dma slave channel runtime config
* @direction: whether the data shall go in or out on this slave
@@ -418,6 +485,10 @@ enum dma_slave_buswidth {
* @slave_id: Slave requester id. Only valid for slave channels. The dma
* slave peripheral will have unique id as dma requester which need to be
* pass as slave config.
+ * @peripheral: type of peripheral to DMA to/from
+ * @set_config: set peripheral config
+ * @spi: peripheral config for spi
+ * @:i2c peripheral config for i2c
*
* This struct is passed in as configuration data to a DMA engine
* in order to set up a certain channel for DMA transport at runtime.
@@ -443,6 +514,10 @@ struct dma_slave_config {
u32 dst_port_window_size;
bool device_fc;
unsigned int slave_id;
+ enum dmaengine_peripheral peripheral;
+ u8 set_config;
+ struct dmaengine_spi_config spi;
+ struct dmaengine_i2c_config i2c;
};
/**
--
2.26.2
[PATCH 3/3] dmaengine: qcom: Add GPI dma driver
This controller provides DMAengine capabilities for a variety of peripheral buses such as I2C, UART, and SPI. By using GPI dmaengine driver, bus drivers can use a standardize interface that is protocol independent to transfer data between memory and peripheral. Signed-off-by: Vinod Koul --- drivers/dma/qcom/Kconfig | 12 + drivers/dma/qcom/Makefile |1 + drivers/dma/qcom/gpi.c| 2269 + 3 files changed, 2282 insertions(+) create mode 100644 drivers/dma/qcom/gpi.c diff --git a/drivers/dma/qcom/Kconfig b/drivers/dma/qcom/Kconfig index 3bcb689162c6..f925296b0c85 100644 --- a/drivers/dma/qcom/Kconfig +++ b/drivers/dma/qcom/Kconfig @@ -8,6 +8,18 @@ config QCOM_BAM_DMA Enable support for the QCOM BAM DMA controller. This controller provides DMA capabilities for a variety of on-chip devices. +config QCOM_GPI_DMA +tristate "Qualcomm Technologies GPI DMA support" +depends on ARCH_QCOM +select DMA_ENGINE +select DMA_VIRTUAL_CHANNELS +help + Enable support for the QCOM GPI DMA controller. This controller + provides DMA capabilities for a variety of peripheral buses such + as I2C, UART, and SPI. By using GPI dmaengine driver, bus drivers + can use a standardize interface that is protocol independent to + transfer data between DDR and peripheral. + config QCOM_HIDMA_MGMT tristate "Qualcomm Technologies HIDMA Management support" select DMA_ENGINE diff --git a/drivers/dma/qcom/Makefile b/drivers/dma/qcom/Makefile index 1ae92da88b0c..f33f027dd0fc 100644 --- a/drivers/dma/qcom/Makefile +++ b/drivers/dma/qcom/Makefile @@ -1,5 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 obj-$(CONFIG_QCOM_BAM_DMA) += bam_dma.o +obj-$(CONFIG_QCOM_GPI_DMA) += gpi.o obj-$(CONFIG_QCOM_HIDMA_MGMT) += hdma_mgmt.o hdma_mgmt-objs := hidma_mgmt.o hidma_mgmt_sys.o obj-$(CONFIG_QCOM_HIDMA) += hdma.o diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c new file mode 100644 index ..592fd39a7740 --- /dev/null +++ b/drivers/dma/qcom/gpi.c @@ -0,0 +1,2269 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2017-2020, The Linux Foundation. All rights reserved. + * Copyright (c) 2020, Linaro Limited + */ + +#define DEBUG + +#include +#include +#include +#include +#include +#include +#include +#include "../dmaengine.h" +#include "../virt-dma.h" + +#define TRE_TYPE_DMA 0x10 +#define TRE_TYPE_GO0x20 +#define TRE_TYPE_CONFIG0 0x22 + +/* TRE flags */ +#define TRE_FLAGS_CHAINBIT(0) +#define TRE_FLAGS_IEOB BIT(8) +#define TRE_FLAGS_IEOT BIT(9) +#define TRE_FLAGS_BEI BIT(10) +#define TRE_FLAGS_LINK BIT(11) +#define TRE_FLAGS_TYPE GENMASK(23, 16) + +/* SPI CONFIG0 WD0 */ +#define TRE_SPI_C0_WORD_SZ GENMASK(4, 0) +#define TRE_SPI_C0_LOOPBACKBIT(8) +#define TRE_SPI_C0_CS BIT(11) +#define TRE_SPI_C0_CPHABIT(12) +#define TRE_SPI_C0_CPOLBIT(13) +#define TRE_SPI_C0_TX_PACK BIT(24) +#define TRE_SPI_C0_RX_PACK BIT(25) + +/* SPI CONFIG0 WD2 */ +#define TRE_SPI_C0_CLK_DIV GENMASK(11, 0) +#define TRE_SPI_C0_CLK_SRC GENMASK(19, 16) + +/* SPI GO WD0 */ +#define TRE_SPI_GO_CMD GENMASK(4, 0) +#define TRE_SPI_GO_CS GENMASK(10, 8) +#define TRE_SPI_GO_FRAGBIT(26) +#define TRE_SPI_RX_LEN GENMASK(23, 0) + +/* DMA TRE */ +#define TRE_DMA_LENGENMASK(23, 0) + +/* Register offsets from gpi-top */ +#define GPII_n_CH_k_CNTXT_0_OFFS(n, k) (0x2 + (0x4000 * (n)) + (0x80 * (k))) +#define GPII_n_CH_k_CNTXT_0_EL_SIZEGENMASK(31, 24) +#define GPII_n_CH_k_CNTXT_0_CHSTATEGENMASK(23, 20) +#define GPII_n_CH_k_CNTXT_0_ERIDX GENMASK(18, 14) +#define GPII_n_CH_k_CNTXT_0_DIRBIT(3) +#define GPII_n_CH_k_CNTXT_0_PROTO GENMASK(2, 0) + +#define GPII_n_CH_k_CNTXT_0(el_size, erindex, dir, chtype_proto) \ + (FIELD_PREP(GPII_n_CH_k_CNTXT_0_EL_SIZE, el_size) | \ +FIELD_PREP(GPII_n_CH_k_CNTXT_0_ERIDX, erindex) | \ +FIELD_PREP(GPII_n_CH_k_CNTXT_0_DIR, dir) | \ +FIELD_PREP(GPII_n_CH_k_CNTXT_0_PROTO, chtype_proto)) + +#define GPI_CHTYPE_DIR_IN (0) +#define GPI_CHTYPE_DIR_OUT (1) + +#define GPI_CHTYPE_PROTO_GPI (0x2) + +#define GPII_n_CH_k_DOORBELL_0_OFFS(n, k) (0x22000 + (0x4000 * (n)) + (0x8 * (k))) +#define GPII_n_CH_CMD_OFFS(n) (0x23008 + (0x4000 * (n))) +#define GPII_n_CH_CMD_OPCODE GENMASK(31, 24) +#define GPII_n_CH_CMD_CHID GENMASK(7, 0) +#define GPII_n_CH_CMD(opcode, chid) \ +(FIELD_PREP(GPII_n_CH_CMD_OPCODE, opcode) | \ + FIELD_PREP(GPII_n_CH_CMD_CHID, chid)) + +#define GPII_n_CH_CMD_ALLOCATE (0) +#define GPI
Re: [PATCH v5] phy: omap-usb2-phy: disable PHY charger detect
On 21-08-20, 11:03, Roger Quadros wrote:
> AM654x PG1.0 has a silicon bug that D+ is pulled high after POR, which
> could cause enumeration failure with some USB hubs. Disabling the
> USB2_PHY Charger Detect function will put D+ into the normal state.
>
> This addresses Silicon Errata:
> i2075 - "USB2PHY: USB2PHY Charger Detect is Enabled by Default Without VBUS
> Presence"
>
> Signed-off-by: Roger Quadros
> Tested-by: Jan Kiszka
> ---
> Vinod/Kishon,
>
> As this is an errata fix, it should be targetted for 5.9-rc cycle.
> Thanks.
>
> cheers,
> -roger
>
> Changelog:
> v5
> - don't use dt property to enable workaround. Use soc_device_match() instead.
>
> v4
> - fix example to fix dt_binding_check warnings
> - '#phy-cells' -> "#phy-cells"
> - Add 'oneOf' to compatible logic to allow just "ti,omap-usb2" as valid
>
> v3
> - Removed quotes from compatibles
> - changed property to "ti,disable-charger-det"
>
> v2
> - Address Rob's comments on YAML schema.
>
> drivers/phy/ti/phy-omap-usb2.c | 70 +-
> 1 file changed, 51 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/phy/ti/phy-omap-usb2.c b/drivers/phy/ti/phy-omap-usb2.c
> index cb2dd3230fa7..65d73142d4ec 100644
> --- a/drivers/phy/ti/phy-omap-usb2.c
> +++ b/drivers/phy/ti/phy-omap-usb2.c
> @@ -6,26 +6,31 @@
> * Author: Kishon Vijay Abraham I
> */
>
> -#include
> -#include
> -#include
> -#include
> -#include
> -#include
> -#include
> #include
> -#include
> -#include
> #include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> #include
> +#include
> #include
> -#include
> +#include
> +#include
> #include
> -#include
> +#include
> +#include
> +#include
Can we do reorder in a separate patch and not in a fixes patch
> static int omap_usb2_probe(struct platform_device *pdev)
> {
> struct omap_usb *phy;
> @@ -366,14 +399,14 @@ static int omap_usb2_probe(struct platform_device *pdev)
> phy->mask = phy_data->mask;
> phy->power_on = phy_data->power_on;
> phy->power_off = phy_data->power_off;
> + phy->flags = phy_data->flags;
>
> - if (phy_data->flags & OMAP_USB2_CALIBRATE_FALSE_DISCONNECT) {
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - phy->phy_base = devm_ioremap_resource(>dev, res);
> - if (IS_ERR(phy->phy_base))
> - return PTR_ERR(phy->phy_base);
> - phy->flags |= OMAP_USB2_CALIBRATE_FALSE_DISCONNECT;
> - }
> + omap_usb2_init_errata(phy);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + phy->phy_base = devm_ioremap_resource(>dev, res);
> + if (IS_ERR(phy->phy_base))
> + return PTR_ERR(phy->phy_base);
>
> phy->syscon_phy_power = syscon_regmap_lookup_by_phandle(node,
> "syscon-phy-power");
> @@ -405,7 +438,6 @@ static int omap_usb2_probe(struct platform_device *pdev)
> }
> }
>
> -
this does not belong to this patch, pls feel free to send a separate one
for this
--
~Vinod
Re: [PATCH -next] phy: ti: j721e-wiz: Remove duplicate include
On 18-08-20, 19:47, YueHaibing wrote: > Remove duplicate include file Applied, thanks > > Signed-off-by: YueHaibing > --- > drivers/phy/ti/phy-j721e-wiz.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/phy/ti/phy-j721e-wiz.c b/drivers/phy/ti/phy-j721e-wiz.c > index 33c4cf0105a4..c9cfafe89cbf 100644 > --- a/drivers/phy/ti/phy-j721e-wiz.c > +++ b/drivers/phy/ti/phy-j721e-wiz.c > @@ -20,7 +20,6 @@ > #include > #include > #include > -#include > > #define WIZ_SERDES_CTRL 0x404 > #define WIZ_SERDES_TOP_CTRL 0x408 > -- > 2.17.1 > -- ~Vinod
Re: [RESEND PATCH v8 2/2] phy: Add USB3 PHY support for Intel LGM SoC
On 17-08-20, 15:05, Ramuthevar,Vadivel MuruganX wrote:
> From: Ramuthevar Vadivel Murugan
>
> Add support for USB PHY on Intel LGM SoC.
>
> Signed-off-by: Ramuthevar Vadivel Murugan
>
> Reviewed-by: Philipp Zabel
> ---
> drivers/phy/Kconfig | 11 ++
> drivers/phy/Makefile | 1 +
> drivers/phy/phy-lgm-usb.c | 278
> ++
> 3 files changed, 290 insertions(+)
> create mode 100644 drivers/phy/phy-lgm-usb.c
>
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index de9362c25c07..01b53f86004c 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -49,6 +49,17 @@ config PHY_XGENE
> help
> This option enables support for APM X-Gene SoC multi-purpose PHY.
>
> +config USB_LGM_PHY
> + tristate "INTEL Lightning Mountain USB PHY Driver"
> + depends on USB_SUPPORT
Why is the dependent on USB..? Should that not be other way around?
> +static int get_flipped(struct tca_apb *ta, bool *flipped)
> +{
> + union extcon_property_value property;
> + int ret;
> +
> + ret = extcon_get_property(ta->phy.edev, EXTCON_USB_HOST,
> + EXTCON_PROP_USB_TYPEC_POLARITY, );
> + if (ret) {
> + dev_err(ta->phy.dev, "no polarity property from extcon\n");
> + return ret;
> + }
> +
> + *flipped = property.intval;
> +
> + return ret;
return 0 here?
> +static int phy_init(struct usb_phy *phy)
> +{
> + struct tca_apb *ta = container_of(phy, struct tca_apb, phy);
> + void __iomem *ctrl1 = phy->io_priv + CTRL1_OFFSET;
> + int val, ret, i;
> +
> + if (ta->phy_initialized)
> + return 0;
> +
> + for (i = 0; i < ARRAY_SIZE(PHY_RESETS); i++)
> + reset_control_deassert(ta->resets[i]);
> +
> + ret = readl_poll_timeout(ctrl1, val, val & SRAM_INIT_DONE, 10, 10 *
> 1000);
> + if (ret) {
> + dev_err(ta->phy.dev, "SRAM init failed, 0x%x\n", val);
> + return ret;
> + }
> +
> + writel(readl(ctrl1) | SRAM_EXT_LD_DONE, ctrl1);
> +
> + ta->phy_initialized = true;
> + if (!ta->phy.edev) {
> + writel(TCPC_CONN, ta->phy.io_priv + TCPC_OFFSET);
> + return phy->set_vbus(phy, true);
> + }
> +
> + schedule_work(>wk);
why work for init?
> +static void tca_work(struct work_struct *work)
> +{
> + struct tca_apb *ta = container_of(work, struct tca_apb, wk);
> + bool connected;
> + bool flipped = false;
> + u32 val;
> + int ret;
> +
> + ret = get_flipped(ta, );
so every time this work is scheduled you are reading from firmware, why..
Typically we should read in probe and store it..
> + connected = extcon_get_state(ta->phy.edev, EXTCON_USB_HOST) && !ret;
lets handle ret and extcon_get_state separately please
> + if (connected == ta->connected)
> + return;
> +
> + ta->connected = connected;
> + if (connected) {
> + val = TCPC_CONN;
> + if (flipped)
> + val |= TCPC_FLIPPED;
> + dev_info(ta->phy.dev, "connected%s\n", flipped ? " flipped" :
> "");
debug perhaps
> + } else {
> + val = TCPC_DISCONN;
> + dev_info(ta->phy.dev, "disconnected\n");
here too
> +static int vbus_notifier(struct notifier_block *nb, unsigned long evnt, void
> *ptr)
> +{
> + return NOTIFY_DONE;
> +}
empty notifier, why bother?
> +static int phy_probe(struct platform_device *pdev)
> +{
> + struct reset_control *resets[ARRAY_SIZE(CTL_RESETS)];
> + struct device *dev = >dev;
> + struct usb_phy *phy;
> + struct tca_apb *ta;
> + int i;
> +
> + ta = devm_kzalloc(dev, sizeof(*ta), GFP_KERNEL);
> + if (!ta)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, ta);
> + INIT_WORK(>wk, tca_work);
> +
> + phy = >phy;
> + phy->dev = dev;
> + phy->label = dev_name(dev);
> + phy->type = USB_PHY_TYPE_USB3;
> + phy->init = phy_init;
> + phy->shutdown = phy_shutdown;
> + phy->set_vbus = phy_set_vbus;
> + phy->id_nb.notifier_call = id_notifier;
> + phy->vbus_nb.notifier_call = vbus_notifier;
> +
> + phy->io_priv = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(phy->io_priv))
> + return PTR_ERR(phy->io_priv);
> +
> + ta->vbus = devm_regulator_get(dev, "vbus");
> + if (IS_ERR(ta->vbus))
> + return PTR_ERR(ta->vbus);
> +
> + for (i = 0; i < ARRAY_SIZE(CTL_RESETS); i++) {
> + resets[i] = devm_reset_control_get_exclusive(dev,
> CTL_RESETS[i]);
> + if (IS_ERR(resets[i])) {
> + dev_err(dev, "%s reset not found\n", CTL_RESETS[i]);
> + return PTR_ERR(resets[i]);
> + }
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(PHY_RESETS); i++) {
> + ta->resets[i] = devm_reset_control_get_exclusive(dev,
> PHY_RESETS[i]);
> + if
Re: [PATCH V2 3/7] phy: qcom-qmp: Use correct values for ipq8074 PCIe Gen2 PHY init
On 29-07-20, 21:00, Sivaprakash Murugesan wrote:
> There were some problem in ipq8074 Gen2 PCIe phy init sequence.
>
> 1. Few register values were wrongly updated in the phy init sequence.
> 2. The register QSERDES_RX_SIGDET_CNTRL is a RX tuning parameter
>register which is added in serdes table causing the wrong register
>was getting updated.
> 3. Clocks and resets were not added in the phy init.
>
> Fix these to make Gen2 PCIe port on ipq8074 devices to work.
Applied to fixes, thanks
>
> Fixes: eef243d04b2b6 ("phy: qcom-qmp: Add support for IPQ8074")
>
no need of empty line here, have fixed it up while applying
--
~Vinod
Re: [PATCH V2 5/7] PCI: qcom: Do PHY power on before PCIe init
On 29-07-20, 21:00, Sivaprakash Murugesan wrote:
> Commit cc1e06f033af ("phy: qcom: qmp: Use power_on/off ops for PCIe")
> changed phy ops from init/deinit to power on/off, due to this phy enable
> is getting called after PCIe init.
>
> On some platforms like ipq8074 phy should be inited before accessing the
> PCIe register space, otherwise the system would hang.
Have you verified that this causes no regression on sdm845 and other
platforms?
> So move phy_power_on API before PCIe init.
>
> Fixes: commit cc1e06f033af ("phy: qcom: qmp: Use power_on/off ops for PCIe")
Is this a fix..? You are updating sequencing for a new platform
> Co-developed-by: Selvam Sathappan Periakaruppan
> Signed-off-by: Selvam Sathappan Periakaruppan
> Signed-off-by: Sivaprakash Murugesan
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c
> b/drivers/pci/controller/dwc/pcie-qcom.c
> index 3aac77a..e1b5651 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -1265,18 +1265,18 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
>
> qcom_ep_reset_assert(pcie);
>
> - ret = pcie->ops->init(pcie);
> + ret = phy_power_on(pcie->phy);
> if (ret)
> return ret;
>
> - ret = phy_power_on(pcie->phy);
> + ret = pcie->ops->init(pcie);
> if (ret)
> - goto err_deinit;
> + goto err_disable_phy;
>
> if (pcie->ops->post_init) {
> ret = pcie->ops->post_init(pcie);
> if (ret)
> - goto err_disable_phy;
> + goto err_deinit;
> }
>
> dw_pcie_setup_rc(pp);
> @@ -1295,10 +1295,10 @@ static int qcom_pcie_host_init(struct pcie_port *pp)
> qcom_ep_reset_assert(pcie);
> if (pcie->ops->post_deinit)
> pcie->ops->post_deinit(pcie);
> -err_disable_phy:
> - phy_power_off(pcie->phy);
> err_deinit:
> pcie->ops->deinit(pcie);
> +err_disable_phy:
> + phy_power_off(pcie->phy);
>
> return ret;
> }
> --
> 2.7.4
--
~Vinod
Re: [PATCH V2 4/7] phy: qcom-qmp: Add compatible for ipq8074 PCIe Gen3 qmp phy
Hi Sivaprakash,
On 29-07-20, 21:00, Sivaprakash Murugesan wrote:
> ipq8074 has two PCIe ports, One Gen2 and one Gen3 ports.
> Since support for Gen2 phy is already available, add support for
> PCIe Gen3 phy.
>
> Co-developed-by: Selvam Sathappan Periakaruppan
> Signed-off-by: Selvam Sathappan Periakaruppan
> Signed-off-by: Sivaprakash Murugesan
> ---
> [V2]
> * Addressed review comments from Vinod
> drivers/phy/qualcomm/phy-qcom-pcie3-qmp.h | 139
> drivers/phy/qualcomm/phy-qcom-qmp.c | 171
> +-
> 2 files changed, 308 insertions(+), 2 deletions(-)
> create mode 100644 drivers/phy/qualcomm/phy-qcom-pcie3-qmp.h
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-pcie3-qmp.h
> b/drivers/phy/qualcomm/phy-qcom-pcie3-qmp.h
> new file mode 100644
> index 000..812ee75
> --- /dev/null
> +++ b/drivers/phy/qualcomm/phy-qcom-pcie3-qmp.h
> @@ -0,0 +1,139 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + */
> +
> +/*
> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> + */
> +
> +#ifndef PHY_QCOM_PCIE_H
Kernel uses this convention..
#ifndef PHY_QCOM_PCIE_H
#define PHY_QCOM_PCIE_H
header contents
#endif
Please update
> +
> +/* QMP V2 PCIE PHY - Found in IPQ8074 gen3 port - QSERDES PLL registers */
> +#define QSERDES_PLL_BG_TIMER 0x00c
> +#define QSERDES_PLL_SSC_PER1 0x01c
> +#define QSERDES_PLL_SSC_PER2 0x020
> +#define QSERDES_PLL_SSC_STEP_SIZE1_MODE0 0x024
> +#define QSERDES_PLL_SSC_STEP_SIZE2_MODE0 0x028
> +#define QSERDES_PLL_SSC_STEP_SIZE1_MODE1 0x02c
> +#define QSERDES_PLL_SSC_STEP_SIZE2_MODE1 0x030
> +#define QSERDES_PLL_BIAS_EN_CLKBUFLR_EN 0x03c
> +#define QSERDES_PLL_CLK_ENABLE1 0x040
> +#define QSERDES_PLL_SYS_CLK_CTRL 0x044
> +#define QSERDES_PLL_SYSCLK_BUF_ENABLE0x048
> +#define QSERDES_PLL_PLL_IVCO 0x050
> +#define QSERDES_PLL_LOCK_CMP1_MODE0 0x054
> +#define QSERDES_PLL_LOCK_CMP2_MODE0 0x058
> +#define QSERDES_PLL_LOCK_CMP1_MODE1 0x060
> +#define QSERDES_PLL_LOCK_CMP2_MODE1 0x064
> +#define QSERDES_PLL_BG_TRIM 0x074
> +#define QSERDES_PLL_CLK_EP_DIV_MODE0 0x078
> +#define QSERDES_PLL_CLK_EP_DIV_MODE1 0x07c
> +#define QSERDES_PLL_CP_CTRL_MODE00x080
> +#define QSERDES_PLL_CP_CTRL_MODE10x084
> +#define QSERDES_PLL_PLL_RCTRL_MODE0 0x088
> +#define QSERDES_PLL_PLL_RCTRL_MODE1 0x08C
why tab here instead of single space in others?
> static const struct qmp_phy_cfg sdm845_qmp_pciephy_cfg = {
> .type = PHY_TYPE_PCIE,
> .nlanes = 1,
> @@ -3024,8 +3181,15 @@ static int phy_pipe_clk_register(struct qcom_qmp *qmp,
> struct device_node *np)
>
> init.ops = _fixed_rate_ops;
>
> - /* controllers using QMP phys use 125MHz pipe clock interface */
> - fixed->fixed_rate = 12500;
> + /*
> + * controllers using QMP phys use 125MHz pipe clock interface unless
> + * other frequency is specified in dts
> + */
> + ret = of_property_read_u32(np, "clock-output-rate",
> +(u32 *)>fixed_rate);
why this cast?
--
~Vinod
Re: [PATCH V2 1/7] dt-bindings: PCI: qcom: Add ipq8074 Gen3 PCIe compatible
On 29-07-20, 21:00, Sivaprakash Murugesan wrote: > ipq8074 has two PCIe ports while the support for Gen2 PCIe port is > already available add the support for Gen3 binding. > > Co-developed-by: Selvam Sathappan Periakaruppan > Signed-off-by: Selvam Sathappan Periakaruppan > Reviewed-by: Rob Herring > Signed-off-by: Sivaprakash Murugesan > --- > .../devicetree/bindings/pci/qcom,pcie.yaml | 47 > ++ The issue is the yaml file is not in linux-phy next.. did we get the conversion done? > 1 file changed, 47 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml > b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml > index 2eef6d5..e0559dd 100644 > --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml > @@ -23,6 +23,7 @@ properties: >- qcom,pcie-ipq8064 >- qcom,pcie-ipq8064-v2 >- qcom,pcie-ipq8074 > + - qcom,pcie-ipq8074-gen3 >- qcom,pcie-msm8996 >- qcom,pcie-qcs404 >- qcom,pcie-sdm845 > @@ -295,6 +296,52 @@ allOf: > compatible: > contains: > enum: > + - qcom,pcie-ipq8074-gen3 > + then: > + properties: > + clocks: > + items: > + - description: sys noc interface clock > + - description: AXI master clock > + - description: AXI secondary clock > + - description: AHB clock > + - description: Auxilary clock > + - description: AXI secondary bridge clock > + - description: PCIe rchng clock > + clock-names: > + items: > + - const: iface > + - const: axi_m > + - const: axi_s > + - const: ahb > + - const: aux > + - const: axi_bridge > + - const: rchng > + resets: > + items: > + - description: PIPE reset > + - description: PCIe sleep reset > + - description: PCIe sticky reset > + - description: AXI master reset > + - description: AXI secondary reset > + - description: AHB reset > + - description: AXI master sticky reset > + - description: AXI secondary sticky reset > + reset-names: > + items: > + - const: pipe > + - const: sleep > + - const: sticky > + - const: axi_m > + - const: axi_s > + - const: ahb > + - const: axi_m_sticky > + - const: axi_s_sticky > + - if: > + properties: > + compatible: > + contains: > + enum: > - qcom,pcie-msm8996 > then: > properties: > -- > 2.7.4 -- ~Vinod
Re: [PATCH v2 0/3] phy: ti: am654: improve PCIe enumeration performance
On 28-07-20, 01:16, Sekhar Nori wrote: > This patch series updates AM654x PCIe serdes settings to > latest recommended by hardware. This fixes Gen2 enumeration > issues seen previously. Applied after fixing typo 'threshold' in last patch, thanks -- ~Vinod
Re: [PATCH v5] phy: samsung: Use readl_poll_timeout function
On 20-07-20, 17:35, Anand Moon wrote: > Instead of a busy waiting while loop using udelay > use readl_poll_timeout function to check the condition > is met or timeout occurs in crport_handshake function. > readl_poll_timeout is called in non atomic context so > it safe to sleep until the condition is met. Applied, thanks -- ~Vinod
Re: [PATCH V5 3/7] phy: tegra: xusb: Add USB2 pad power control support for Tegra210
On 20-07-20, 15:25, Nagarjuna Kristam wrote:
> Add USB2 pad power on and off API's for TEgra210 and provide its control
> via soc ops. It can be used by operations like charger detect to power on
> and off USB2 pad if needed.
>
> Signed-off-by: Nagarjuna Kristam
> Acked-by: Thierry Reding
> ---
> V5:
> - Made tegra210_usb2_pad_power_on() and tegra210_usb2_pad_power_down()
> static.
> ---
> V4:
> - No changes
> ---
> V3:
> - Added Acked-by updates to commit message.
> ---
> V2:
> - Patch re-based.
> ---
> drivers/phy/tegra/xusb-tegra210.c | 190
> ++
> 1 file changed, 133 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/phy/tegra/xusb-tegra210.c
> b/drivers/phy/tegra/xusb-tegra210.c
> index 66bd461..2e5f71c 100644
> --- a/drivers/phy/tegra/xusb-tegra210.c
> +++ b/drivers/phy/tegra/xusb-tegra210.c
> @@ -994,6 +994,128 @@ static int tegra210_xusb_padctl_id_override(struct
> tegra_xusb_padctl *padctl,
> return 0;
> }
>
> +static void tegra210_usb2_bias_pad_power_on(struct tegra_xusb_usb2_pad *pad)
> +{
> + struct tegra_xusb_padctl *padctl = pad->base.padctl;
> + u32 value;
> +
> + if (pad->enable++ > 0)
> + return;
> +
> + dev_dbg(padctl->dev, "power on BIAS PAD & USB2 tracking\n");
> +
> + if (clk_prepare_enable(pad->clk))
> + dev_warn(padctl->dev, "failed to enable BIAS PAD & USB2
> tracking\n");
do you want to proceed ahead even if clock is not enabled..?
> +
> + value = padctl_readl(padctl, XUSB_PADCTL_USB2_BIAS_PAD_CTL1);
> + value &= ~((XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_START_TIMER_MASK <<
> + XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_START_TIMER_SHIFT) |
> +(XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_DONE_RESET_TIMER_MASK <<
> + XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_DONE_RESET_TIMER_SHIFT));
> + value |= (XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_START_TIMER_VAL <<
> + XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_START_TIMER_SHIFT) |
> + (XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_DONE_RESET_TIMER_VAL <<
> + XUSB_PADCTL_USB2_BIAS_PAD_CTL1_TRK_DONE_RESET_TIMER_SHIFT);
I recently found I think this would look lot neater
if we use FIELD_PREP or u32_encode_bits() and you can drop shift defines
Since this is not new code but moved here, I will leave it upto you to
change this, either ways is fine by me.
--
~Vinod
Re: [PATCH v4 0/2] Add new UniPhier AHCI PHY driver
Hello, On 21-08-20, 18:20, Kunihiko Hayashi wrote: > Gentle ping. > Are there any comments in this series? Sorry I dont have this in my inbox, can you please rebease and resend to me as well Thanks > > Thank you, > > On 2020/07/16 17:32, Kunihiko Hayashi wrote: > > This series adds support for AHCI PHY interface implemented in Socionext > > UniPhier SoCs. This driver supports PXs2 and PXs3 SoCs. > > > > Changes since v3: > > - Eliminate a meaningless blank line and a line break > > - Fix misspelling > > > > Changes since v2: > > - Adjust copyright year > > - Add helper for enabling the controller > > - Remove redundant .init in uniphier_pxs2_data > > - Add comments for dummy read accesses > > - Fix return value in uniphier_ahciphy_init > > - dt-bindings: Add Reviewed-by line > > > > Changes since v1: > > - dt-bindings: Fix items in reset-names > > > > Kunihiko Hayashi (2): > >dt-bindings: phy: Add UniPhier AHCI PHY description > >phy: socionext: Add UniPhier AHCI PHY driver support > > > > .../bindings/phy/socionext,uniphier-ahci-phy.yaml | 76 + > > drivers/phy/socionext/Kconfig | 10 + > > drivers/phy/socionext/Makefile | 1 + > > drivers/phy/socionext/phy-uniphier-ahci.c | 321 > > + > > 4 files changed, 408 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/phy/socionext,uniphier-ahci-phy.yaml > > create mode 100644 drivers/phy/socionext/phy-uniphier-ahci.c > > > > --- > Best Regards > Kunihiko Hayashi -- ~Vinod
Re: [PATCH v3 1/1] phy: tusb1210: use bitmasks to set VENDOR_SPECIFIC2
On 19-08-20, 15:57, Geert Uytterhoeven wrote:
> Hi Liam,
>
>
> On Mon, Aug 17, 2020 at 7:38 PM Liam Beguin wrote:
> > From: Liam Beguin
> >
> > Start by reading the content of the VENDOR_SPECIFIC2 register and update
> > each bit field based on device properties when defined.
> >
> > The use of bit masks prevents fields from overriding each other and
> > enables users to clear bits which are set by default, like datapolarity
> > in this instance.
> >
> > Signed-off-by: Liam Beguin
> > ---
> > Changes since v1:
> > - use set_mask_bits
> >
> > Changes since v2:
> > - fix missing bit shift dropped in v2
> > - rebase on 5.9-rc1
> >
> > drivers/phy/ti/phy-tusb1210.c | 27 +--
> > 1 file changed, 17 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/phy/ti/phy-tusb1210.c b/drivers/phy/ti/phy-tusb1210.c
> > index d8d0cc11d187..358842b5790f 100644
> > --- a/drivers/phy/ti/phy-tusb1210.c
> > +++ b/drivers/phy/ti/phy-tusb1210.c
> > @@ -14,8 +14,11 @@
> >
> > #define TUSB1210_VENDOR_SPECIFIC2 0x80
> > #define TUSB1210_VENDOR_SPECIFIC2_IHSTX_SHIFT 0
> > +#define TUSB1210_VENDOR_SPECIFIC2_IHSTX_MASK GENMASK(3, 0)
> > #define TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_SHIFT 4
> > +#define TUSB1210_VENDOR_SPECIFIC2_ZHSDRV_MASK GENMASK(5, 4)
> > #define TUSB1210_VENDOR_SPECIFIC2_DP_SHIFT 6
> > +#define TUSB1210_VENDOR_SPECIFIC2_DP_MASK BIT(6)
> >
> > struct tusb1210 {
> > struct ulpi *ulpi;
> > @@ -118,23 +121,27 @@ static int tusb1210_probe(struct ulpi *ulpi)
> > * diagram optimization and DP/DM swap.
> > */
> >
> > + reg = ulpi_read(ulpi, TUSB1210_VENDOR_SPECIFIC2);
> > +
> > /* High speed output drive strength configuration */
> > - device_property_read_u8(>dev, "ihstx", );
> > - reg = val << TUSB1210_VENDOR_SPECIFIC2_IHSTX_SHIFT;
> > + if (!device_property_read_u8(>dev, "ihstx", ))
> > + reg = set_mask_bits(,
> > TUSB1210_VENDOR_SPECIFIC2_IHSTX_MASK,
> > + val <<
> > TUSB1210_VENDOR_SPECIFIC2_IHSTX_SHIFT);
>
> Triggered by your patches to add support for cmpxchg on u8 pointers to
> various architectures (which is a valuable goal in itself ;-), I decided
> to have a look at the underlying problem you were facing.
>
> IMHO, using set_mask_bits() is overkill here. That helper is meant to
> update individual bits in a variable that may be accessed concurrently,
> hence its use of cmpxchg().
>
> In this driver, you just want to modify a local variable, by clearing a
> field, and setting some bits. No concurrency is involved.
> Perhaps using FIELD_PREP() from include/linux/bitfield.h is more
> appropriate?
Yeah i discovered the include/linux/bitfield.h and yes that looks more
apt here. u32_encode_bits() would make sense here and get rid of mask
and shift stuff too.
--
~Vinod
Re: [PATCH v5] phy: omap-usb2-phy: disable PHY charger detect
On 20-08-20, 20:27, Sekhar Nori wrote: > On 8/20/20 7:09 PM, Roger Quadros wrote: > > AM654x PG1.0 has a silicon bug that D+ is pulled high after POR, which > > could cause enumeration failure with some USB hubs. Disabling the > > USB2_PHY Charger Detect function will put D+ into the normal state. > > > > This addresses Silicon Errata: > > i2075 - "USB2PHY: USB2PHY Charger Detect is Enabled by Default Without VBUS > > Presence" > > > > Signed-off-by: Roger Quadros > > + Vinod as well Thanks, can you please rebase and resend these patches. I dont have them in my inbox Regards -- ~Vinod
Re: [PATCH 2/2] soundwire: fix port_ready[] dynamic allocation in mipi_disco and ASoC codecs
On 18-08-20, 07:09, Pierre-Louis Bossart wrote: > > > On 8/18/20 1:36 AM, Vinod Koul wrote: > > On 18-08-20, 01:47, Bard Liao wrote: > > > From: Pierre-Louis Bossart > > > > > > The existing code allocates memory for the total number of ports. > > > This only works if the ports are contiguous, but will break if e.g. a > > > Devices uses port0, 1, and 14. The port_ready[] array would contain 3 > > > elements, which would lead to an out-of-bounds access. Conversely in > > > other cases, the wrong port index would be used leading to timeouts on > > > prepare. > > > > > > This can be fixed by allocating for the worst-case of 15 > > > ports (DP0..DP14). In addition since the number is now fixed, we can > > > use an array instead of a dynamic allocation. > > > > > > Signed-off-by: Pierre-Louis Bossart > > > Reviewed-by: Rander Wang > > > Reviewed-by: Guennadi Liakhovetski > > > Signed-off-by: Bard Liao > > > --- > > > drivers/soundwire/mipi_disco.c | 18 +- > > > drivers/soundwire/slave.c | 4 > > > include/linux/soundwire/sdw.h | 2 +- > > > sound/soc/codecs/max98373-sdw.c | 15 +-- > > > sound/soc/codecs/rt1308-sdw.c | 14 +- > > > sound/soc/codecs/rt5682-sdw.c | 15 +-- > > > sound/soc/codecs/rt700-sdw.c| 15 +-- > > > sound/soc/codecs/rt711-sdw.c| 15 +-- > > > sound/soc/codecs/rt715-sdw.c| 33 + > > > > This looks fine, but the asoc changes are not dependent, so maybe we > > should split them up and then can go thru Mark. Or Mark acks, either way > > would work for me > > There are 3 dependencies that we tracked between SoundWire and ASoC > subsystems: > > a) addition of SDCA control macro (needed before SDCA codec drivers can be > shared) > b) this series - we could indeed submit the codec changes to Mark's tree > separately, but then the SoundWire tree would be broken: the codec drivers > would still try to allocate dynamically what is now a fixed-size array. > c) configuration of the interrupt masks in codec drivers instead of > hard-coded in bus driver + spurious parity error workaround (not posted yet > but ready). > > The changes in ASoC codecs are really only on the initialization part > (either removing a dynamic allocation or setting masks), there's no > functional change otherwise. > > I think the simplest to avoid multiple back-and-forth is to have these small > interface/initialization changes merged through the SoundWire subsystem, > then merged by Mark from a single immutable tag. Would this work for > everyone? That would work for me, but you need to split the asoc, regmap, sdw patches. I am sure looking at patch tag, other maintainers would have skipped these patches.. > In addition, there's a WIP change to regmap to add support for SoundWire 1.2 > MBQ-based register access, but this only affects regmap and ASoC trees, all > handled by Mark. > > I don't think we have any other cross-tree changes planned for now, the SDCA > infrastructure plumbing is still rather open. -- ~Vinod
Re: [PATCH v2] soundwire: SDCA: add helper macro to access controls
On 17-08-20, 10:14, Pierre-Louis Bossart wrote: > > > > The upcoming SDCA (SoundWire Device Class Audio) specification defines > > > a hiearchical encoding to interface with Class-defined capabilities, > > > > typo hiearchical > > ok > > > > based on which audio function, entity, control and channel being used. > > > > Can you please elaborate on what do these terms refer to? > > > > Also can we have some documentation for this and how Linux is going to > > use it.. > > These are concepts in the SDCA draft spec, and that should be the reference. > We worked with MIPI so that this spec will be available with a click-through > agreement when ratified, for now it's only available to contributors per > MIPI bylaws. > > If you do not have access to this specification, then that's a real problem. > Maybe you need to let Bard take care of this part as a co-maintainer? > > The goal with this macro is to enable a first set of codecs drivers using > these concepts to be released upstream. All you need to know at this point > is that controls are defined in a hierarchical way and accessed with a > read/write transaction from/to the address created with the following macro. Hmmm, if we cannot get some kind of Documentation of what it means and review the code, then I do not see a point in getting this into kernel. As kernel community we would like to see some form of Documentation associated with the patches on what this means. If that is not possible due to MIPI regulations, maybe deferring this would make sense Thanks > > > > +/* v1.2 device - SDCA address mapping */ > > > +#define SDW_SDCA_CTL(fun, ent, ctl, ch) (BIT(30) | > > > \ > > > + (((fun) & 0x7) << 22) | > > > \ > > > + (((ent) & 0x40) << 15) | > > > \ > > > + (((ent) & 0x3f) << 7) | > > > \ > > > + (((ctl) & 0x30) << 15) | > > > \ > > > + (((ctl) & 0x0f) << 3) | > > > \ > > > + (((ch) & 0x38) << 12) | > > > \ > > > + ((ch) & 0x07)) > > > + > > > > how about adding an underscore to the arguments here: > > > > #define SDW_SDCA_CTL(_fun, _ent, _ctl, _ch) > > and so on.. > > I checked the SoundWire defines and the vast majority of the macros don't > use underscores, and when they do there's no consistency between 1 or 2 > underscores. -- ~Vinod
Re: [PATCH] soundwire: cadence: fix race condition between suspend and Slave device alerts
On 19-08-20, 07:51, Pierre-Louis Bossart wrote: > > > On 8/19/20 4:06 AM, Vinod Koul wrote: > > On 18-08-20, 06:23, Bard Liao wrote: > > > From: Pierre-Louis Bossart > > > > > > In system suspend stress cases, the SOF CI reports timeouts. The root > > > cause is that an alert is generated while the system suspends. The > > > interrupt handling generates transactions on the bus that will never > > > be handled because the interrupts are disabled in parallel. > > > > > > As a result, the transaction never completes and times out on resume. > > > This error doesn't seem too problematic since it happens in a work > > > queue, and the system recovers without issues. > > > > > > Nevertheless, this race condition should not happen. When doing a > > > system suspend, or when disabling interrupts, we should make sure the > > > current transaction can complete, and prevent new work from being > > > queued. > > > > > > BugLink: https://github.com/thesofproject/linux/issues/2344 > > > Signed-off-by: Pierre-Louis Bossart > > > Reviewed-by: Ranjani Sridharan > > > Reviewed-by: Rander Wang > > > Signed-off-by: Bard Liao > > > --- > > > drivers/soundwire/cadence_master.c | 24 +++- > > > drivers/soundwire/cadence_master.h | 1 + > > > 2 files changed, 24 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/soundwire/cadence_master.c > > > b/drivers/soundwire/cadence_master.c > > > index 24eafe0aa1c3..1330ffc47596 100644 > > > --- a/drivers/soundwire/cadence_master.c > > > +++ b/drivers/soundwire/cadence_master.c > > > @@ -791,7 +791,16 @@ irqreturn_t sdw_cdns_irq(int irq, void *dev_id) > > >CDNS_MCP_INT_SLAVE_MASK, 0); > > > int_status &= ~CDNS_MCP_INT_SLAVE_MASK; > > > - schedule_work(>work); > > > + > > > + /* > > > + * Deal with possible race condition between interrupt > > > + * handling and disabling interrupts on suspend. > > > + * > > > + * If the master is in the process of disabling > > > + * interrupts, don't schedule a workqueue > > > + */ > > > + if (cdns->interrupt_enabled) > > > + schedule_work(>work); > > > > would it not make sense to mask the interrupts first and then cancel the > > work? that way you are guaranteed that after this call you dont have > > interrupts and work scheduled? > > cancel_work_sync() will either > a) wait until the current work completes, or > b) prevent a new one from starting. > > there's no way to really 'abort' a workqueue, 'cancel' means either complete > or don't start. Quite right, as that is how everyone deals with it. Stop the irq from firing first and then wait until work is cancelled or completed, hence cancel_work_sync() > if you disable the interrupts then cancel the work, you have a risk of not > letting the work complete if it already started (case a). > > The race is > a) the interrupt thread (this function) starts > b) the work is scheduled and starts > c) the suspend handler starts and disables interrupts in [1] below. > d) the work initiates transactions which will never complete since Cadence > interrupts have been disabled. Would it not be better to let work handle the case of interrupts disabled and not initiates transactions which wont complete here? That sounds more reasonable to do rather than complete the work which anyone doesn't matter as you are suspending > So the idea was that before disabling interrupts, the suspend handler > changes the status, and then calls cancel_work_sync(). the status is also > used to prevent a new work from being scheduled if you already know the > suspend is on-going. The test on the status above is not strictly necessary, > I believe the sequence is safe without it but it avoid starting a useless > work. > > If you want to follow the flow it's better to start with what the suspend > handler does below first, then look at how the interrupt thread might > interfere. The diff format does not help, might be also easier to apply the > patch and look at the rest of the code, e.g the 3 mask updates mentioned > below are not included in the diff. > > > > > > } > > > cdns_writel(cdns, CDNS_MCP_INTSTAT, int_status); > > > @@ -924,6 +933,19 @@ int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns, > > > bool state) > > >
Re: [PATCH] soundwire: intel: fix CONFIG_PM and CONFIG_PM_SLEEP confusion
On 20-08-20, 22:04, Bard Liao wrote:
> From: Pierre-Louis Bossart
>
> When CONFIG_PM_SLEEP is not defined, GCC throws compilation warnings:
>
> drivers/soundwire/intel.c:1816:12: warning: ‘intel_resume’ defined but
> not used [-Wunused-function]
> 1816 | static int intel_resume(struct device *dev)
> |^~~~
>
> drivers/soundwire/intel.c:1697:12: warning: ‘intel_suspend’ defined
> but not used [-Wunused-function]
> 1697 | static int intel_suspend(struct device *dev)
>
> Fix by adding the missing CONFIG_PM_SLEEP.
Can you rather use __maybe for for these rather than wrapping in another
ifdef, that is the recommended way to do this
Thanks
> Note that we could move code around and use only 2 ifdefs, but this
> will generate conflicts so let's do this when all the pm handling is
> merged.
>
> Signed-off-by: Pierre-Louis Bossart
> Signed-off-by: Bard Liao
> ---
> drivers/soundwire/intel.c | 12 ++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index dbcbe2708563..a2f0026cb2c1 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -1525,7 +1525,7 @@ int intel_master_process_wakeen_event(struct
> platform_device *pdev)
> * PM calls
> */
>
> -#ifdef CONFIG_PM
> +#ifdef CONFIG_PM_SLEEP
>
> static int intel_suspend(struct device *dev)
> {
> @@ -1562,6 +1562,9 @@ static int intel_suspend(struct device *dev)
>
> return 0;
> }
> +#endif
> +
> +#ifdef CONFIG_PM
>
> static int intel_suspend_runtime(struct device *dev)
> {
> @@ -1624,6 +1627,9 @@ static int intel_suspend_runtime(struct device *dev)
>
> return ret;
> }
> +#endif
> +
> +#ifdef CONFIG_PM_SLEEP
>
> static int intel_resume(struct device *dev)
> {
> @@ -1691,6 +1697,9 @@ static int intel_resume(struct device *dev)
>
> return ret;
> }
> +#endif
> +
> +#ifdef CONFIG_PM
>
> static int intel_resume_runtime(struct device *dev)
> {
> @@ -1797,7 +1806,6 @@ static int intel_resume_runtime(struct device *dev)
>
> return ret;
> }
> -
> #endif
>
> static const struct dev_pm_ops intel_pm = {
> --
> 2.17.1
--
~Vinod
Re: [PATCH] soundwire: bus: fix typo in comment on INTSTAT registers
On 18-08-20, 06:09, Bard Liao wrote: > From: Pierre-Louis Bossart > > s/Instat/Intstat/ Applied, thanks -- ~Vinod
Re: [PATCH] soundwire: cadence: fix race condition between suspend and Slave device alerts
On 18-08-20, 06:23, Bard Liao wrote:
> From: Pierre-Louis Bossart
>
> In system suspend stress cases, the SOF CI reports timeouts. The root
> cause is that an alert is generated while the system suspends. The
> interrupt handling generates transactions on the bus that will never
> be handled because the interrupts are disabled in parallel.
>
> As a result, the transaction never completes and times out on resume.
> This error doesn't seem too problematic since it happens in a work
> queue, and the system recovers without issues.
>
> Nevertheless, this race condition should not happen. When doing a
> system suspend, or when disabling interrupts, we should make sure the
> current transaction can complete, and prevent new work from being
> queued.
>
> BugLink: https://github.com/thesofproject/linux/issues/2344
> Signed-off-by: Pierre-Louis Bossart
> Reviewed-by: Ranjani Sridharan
> Reviewed-by: Rander Wang
> Signed-off-by: Bard Liao
> ---
> drivers/soundwire/cadence_master.c | 24 +++-
> drivers/soundwire/cadence_master.h | 1 +
> 2 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/soundwire/cadence_master.c
> b/drivers/soundwire/cadence_master.c
> index 24eafe0aa1c3..1330ffc47596 100644
> --- a/drivers/soundwire/cadence_master.c
> +++ b/drivers/soundwire/cadence_master.c
> @@ -791,7 +791,16 @@ irqreturn_t sdw_cdns_irq(int irq, void *dev_id)
>CDNS_MCP_INT_SLAVE_MASK, 0);
>
> int_status &= ~CDNS_MCP_INT_SLAVE_MASK;
> - schedule_work(>work);
> +
> + /*
> + * Deal with possible race condition between interrupt
> + * handling and disabling interrupts on suspend.
> + *
> + * If the master is in the process of disabling
> + * interrupts, don't schedule a workqueue
> + */
> + if (cdns->interrupt_enabled)
> + schedule_work(>work);
would it not make sense to mask the interrupts first and then cancel the
work? that way you are guaranteed that after this call you dont have
interrupts and work scheduled?
> }
>
> cdns_writel(cdns, CDNS_MCP_INTSTAT, int_status);
> @@ -924,6 +933,19 @@ int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns,
> bool state)
> slave_state = cdns_readl(cdns, CDNS_MCP_SLAVE_INTSTAT1);
> cdns_writel(cdns, CDNS_MCP_SLAVE_INTSTAT1, slave_state);
> }
> + cdns->interrupt_enabled = state;
> +
> + /*
> + * Complete any on-going status updates before updating masks,
> + * and cancel queued status updates.
> + *
> + * There could be a race with a new interrupt thrown before
> + * the 3 mask updates below are complete, so in the interrupt
> + * we use the 'interrupt_enabled' status to prevent new work
> + * from being queued.
> + */
> + if (!state)
> + cancel_work_sync(>work);
>
> cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK0, slave_intmask0);
> cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK1, slave_intmask1);
> diff --git a/drivers/soundwire/cadence_master.h
> b/drivers/soundwire/cadence_master.h
> index fdec62b912d3..4d1aab5b5ec2 100644
> --- a/drivers/soundwire/cadence_master.h
> +++ b/drivers/soundwire/cadence_master.h
> @@ -133,6 +133,7 @@ struct sdw_cdns {
>
> bool link_up;
> unsigned int msg_count;
> + bool interrupt_enabled;
>
> struct work_struct work;
>
> --
> 2.17.1
--
~Vinod
Re: [PATCH V2 0/3] do exception handling appropriately in at_dma_xlate()
On 17-08-20, 19:57, Yu Kuai wrote: > changes from V1: > -separate different changes to different patches, as suggested by Vinod. Please write proper cover letter explaining the patch series and also the changes from v1.. I have applied the patches. Thanks > Yu Kuai (3): > dmaengine: at_hdmac: check return value of of_find_device_by_node() in > at_dma_xlate() > dmaengine: at_hdmac: add missing put_device() call in at_dma_xlate() > dmaengine: at_hdmac: add missing kfree() call in at_dma_xlate() > > drivers/dma/at_hdmac.c | 11 +-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > -- > 2.25.4 -- ~Vinod
Re: [PATCH v3] usb: renesas-xhci: remove version check
On 18-08-20, 10:46, Greg Kroah-Hartman wrote:
> On Tue, Aug 18, 2020 at 02:09:19PM +0530, Vinod Koul wrote:
> > On 18-08-20, 09:31, Greg Kroah-Hartman wrote:
> > > On Tue, Aug 18, 2020 at 12:47:39PM +0530, Vinod Koul wrote:
> > > > Some devices in wild are reporting bunch of firmware versions, so remove
> > > > the check for versions in driver
> > > >
> > > > Reported by: Anastasios Vacharakis
> > > > Reported by: Glen Journeay
> > > > Fixes: 2478be82de44 ("usb: renesas-xhci: Add ROM loader for uPD720201")
> > > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=208911
> > > > Signed-off-by: Vinod Koul
> > > > ---
> > > > Changes in v3:
> > > > - drop additional firmware versions and remove the check
> > > >
> > > > Greg, this fixes regression for folks with preprogrammed controllers
> > > > please mark as stable material
> > >
> > > You could have done so by putting "Cc: stable..." in the s-o-b of the
> > > kernel, why force me to do that by hand?
> >
> > Oops, wasnt sure of your preference. Btw am sure you would have scripted
> > it :-)
> >
> > Would you like me to send an update with stable tagged or this is fine
> > for now?
>
> I can do it now, but in the future please be nice to maintainers, we are
> overworked as you well know :)
Thanks.
--
~Vinod
Re: [PATCH v3] usb: renesas-xhci: remove version check
On 18-08-20, 09:31, Greg Kroah-Hartman wrote:
> On Tue, Aug 18, 2020 at 12:47:39PM +0530, Vinod Koul wrote:
> > Some devices in wild are reporting bunch of firmware versions, so remove
> > the check for versions in driver
> >
> > Reported by: Anastasios Vacharakis
> > Reported by: Glen Journeay
> > Fixes: 2478be82de44 ("usb: renesas-xhci: Add ROM loader for uPD720201")
> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=208911
> > Signed-off-by: Vinod Koul
> > ---
> > Changes in v3:
> > - drop additional firmware versions and remove the check
> >
> > Greg, this fixes regression for folks with preprogrammed controllers
> > please mark as stable material
>
> You could have done so by putting "Cc: stable..." in the s-o-b of the
> kernel, why force me to do that by hand?
Oops, wasnt sure of your preference. Btw am sure you would have scripted
it :-)
Would you like me to send an update with stable tagged or this is fine
for now?
--
~Vinod
Re: [PATCH 1/2] soundwire: add definition for maximum number of ports
On 18-08-20, 06:53, Liao, Bard wrote: > > -Original Message- > > From: Vinod Koul > > Sent: Tuesday, August 18, 2020 2:36 PM > > To: Bard Liao > > Cc: alsa-de...@alsa-project.org; linux-kernel@vger.kernel.org; > > ti...@suse.de; > > broo...@kernel.org; gre...@linuxfoundation.org; j...@cadence.com; > > srinivas.kandaga...@linaro.org; rander.w...@linux.intel.com; > > ranjani.sridha...@linux.intel.com; hui.w...@canonical.com; pierre- > > louis.boss...@linux.intel.com; Kale, Sanyog R ; > > Lin, > > Mengdong ; Liao, Bard > > Subject: Re: [PATCH 1/2] soundwire: add definition for maximum number of > > ports > > > > On 18-08-20, 01:47, Bard Liao wrote: > > > From: Pierre-Louis Bossart > > > > > > A Device may have at most 15 physical ports (DP0, DP1..DP14). > > > > > > Signed-off-by: Pierre-Louis Bossart > > > > > > Reviewed-by: Rander Wang > > > Reviewed-by: Guennadi Liakhovetski > > > > > > Signed-off-by: Bard Liao > > > --- > > > include/linux/soundwire/sdw.h | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/include/linux/soundwire/sdw.h > > > b/include/linux/soundwire/sdw.h index 76052f12c9f7..0aa4c6af7554 > > > 100644 > > > --- a/include/linux/soundwire/sdw.h > > > +++ b/include/linux/soundwire/sdw.h > > > @@ -38,7 +38,8 @@ struct sdw_slave; > > > #define SDW_FRAME_CTRL_BITS 48 > > > #define SDW_MAX_DEVICES 11 > > > > > > -#define SDW_VALID_PORT_RANGE(n) ((n) <= 14 && (n) >= 1) > > > +#define SDW_MAX_PORTS15 > > > +#define SDW_VALID_PORT_RANGE(n) ((n) < > > SDW_MAX_PORTS && (n) >= 1) > > > > What is the use of this one if we are allocating all ports always, Also, I > > dont > > see it used in second patch? > > It is used in drivers/soundwire/stream.c and drivers/soundwire/debugfs.c. Ah overlooked that it is modified, pls ignore this comment -- ~Vinod
