* [PATCH v6 1/3] dt-bindings: Add vendor prefix for Caninos Loucos
From: Matheus Castello @ 2020-05-25 13:22 UTC (permalink / raw)
To: afaerber, manivannan.sadhasivam, mark.rutland, robh+dt
Cc: edgar.righi, igor.lima, linux-arm-kernel, devicetree,
linux-kernel, linux-actions, Matheus Castello, Rob Herring
In-Reply-To: <20200525132218.190841-1-matheus@castello.eng.br>
The Caninos Loucos Program develops Single Board Computers with an open
structure. The Program wants to form a community of developers to use
IoT technologies and disseminate the learning of embedded systems in
Brazil.
It is an initiative of the Technological Integrated Systems Laboratory
(LSI-TEC) with the support of Polytechnic School of the University of
São Paulo (Poli-USP) and Jon "Maddog" Hall.
Signed-off-by: Matheus Castello <matheus@castello.eng.br>
Acked-by: Rob Herring <robh@kernel.org>
Reviewed-by: Andreas Färber <afaerber@suse.de>
---
Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index d3891386d671..a20a05e3deaa 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -167,6 +167,8 @@ patternProperties:
description: CALAO Systems SAS
"^calxeda,.*":
description: Calxeda
+ "^caninos,.*":
+ description: Caninos Loucos Program
"^capella,.*":
description: Capella Microsystems, Inc
"^cascoda,.*":
--
2.26.2
^ permalink raw reply related
* [PATCH v6 0/3] Add Caninos Loucos Labrador CoM and Base Board Device Tree
From: Matheus Castello @ 2020-05-25 13:22 UTC (permalink / raw)
To: afaerber, manivannan.sadhasivam, mark.rutland, robh+dt
Cc: edgar.righi, igor.lima, linux-arm-kernel, devicetree,
linux-kernel, linux-actions, Matheus Castello
Thanks Andreas, Mani and Rob for your time reviewing it.
Changes since v5:
(Suggested by Andreas Färber)
- Put caninos,labrador-v2 as const one level down
Changes since v4:
(Suggested by Rob Herring)
- Fix issues with yaml indentation
Matheus Castello (3):
dt-bindings: Add vendor prefix for Caninos Loucos
dt-bindings: arm: actions: Document Caninos Loucos Labrador
ARM: dts: Add Caninos Loucos Labrador
.../devicetree/bindings/arm/actions.yaml | 5 +++
.../devicetree/bindings/vendor-prefixes.yaml | 2 ++
arch/arm/boot/dts/Makefile | 1 +
.../arm/boot/dts/owl-s500-labrador-base-m.dts | 34 +++++++++++++++++++
arch/arm/boot/dts/owl-s500-labrador-v2.dtsi | 22 ++++++++++++
5 files changed, 64 insertions(+)
create mode 100644 arch/arm/boot/dts/owl-s500-labrador-base-m.dts
create mode 100644 arch/arm/boot/dts/owl-s500-labrador-v2.dtsi
--
2.26.2
^ permalink raw reply
* Re: [PATCH v2 08/12] i2c: designware: Introduce platform drivers glue layer interface
From: Jarkko Nikula @ 2020-05-25 13:16 UTC (permalink / raw)
To: Serge Semin
Cc: Serge Semin, Andy Shevchenko, Mika Westerberg, Alexey Malahov,
Thomas Bogendoerfer, Paul Burton, Ralf Baechle, Rob Herring,
Frank Rowand, linux-mips, devicetree, Wolfram Sang,
Rafael J. Wysocki, Hanjun Guo, Hans de Goede, linux-kernel,
linux-i2c
In-Reply-To: <20200521023735.mja62ujmxebgwyef@mobilestation>
Hi
On 5/21/20 5:37 AM, Serge Semin wrote:
> On Wed, May 20, 2020 at 03:46:11PM +0300, Jarkko Nikula wrote:
>> Hi
>>
>> On 5/10/20 12:50 PM, Serge Semin wrote:
>>> Seeing the DW I2C platform driver is getting overcomplicated with a lot of
>>> vendor-specific configs let's introduce a glue-layer interface so new
>>> platforms which equipped with Synopsys Designware APB I2C IP-core would
>>> be able to handle their peculiarities in the dedicated objects.
>>>
>> Comment to this patch and patches 9/12 and 12/12:
>>
>> Currently i2c-designware-platdrv.c is about 500 lines of code so I don't
>> think it's too overcomplicated. But I feel we have already too many Kconfig
>> options and source modules for i2c-designware and obviously would like to
>> push back a little from adding more.
>>
>> I don't think i2c-designware-platdrv.c becomes yet too complicated if Baikal
>> related code is added there, perhaps under #ifdef CONFIG_OF like MSCC Ocelot
>> code is currently.
>
> Well, it's up to you to decide, what solution is more suitable for you to
> maintain. My idea of detaching the MSCC and Baikal-T1 code to the dedicated
> source files was to eventually move the whole i2c-designware-* set of files
> into a dedicated directory drivers/i2c/buses/dw as it's done for some others
> Synopsys DesignWare controllers: drivers/pci/controller/dwc/, drivers/usb/dwc2,
> drivers/usb/dwc3, drivers/net/ethernet/synopsys/ . If you think, that it's too
> early for Dw I2C code to live in a dedicated directory, fine with me. I can
> merge the MSCC and Baikal-T1 code back into the i2c-designware-platdrv.c .
> So what's your final word in this matter?
>
I think sub directory decision under each subsystem is more subsystem
rather than vendor/driver specific. Good point anyway.
For this patchset I'd like more if changes are done to
i2c-designware-platdrv.c since it's not too complicated yet :-)
If it starts to look too messy in the future then it's time split I think.
Jarkko
^ permalink raw reply
* Re: [PATCH v13 2/2] media: v4l: xilinx: Add Xilinx MIPI CSI-2 Rx Subsystem driver
From: Luca Ceresoli @ 2020-05-25 13:14 UTC (permalink / raw)
To: Vishal Sagar, Hyun Kwon, laurent.pinchart, mchehab, robh+dt,
mark.rutland, Michal Simek, linux-media, devicetree, hans.verkuil,
linux-arm-kernel, linux-kernel, Dinesh Kumar, Sandip Kothari,
Jacopo Mondi
Cc: Hyun Kwon
In-Reply-To: <20200512151947.120348-3-vishal.sagar@xilinx.com>
Hi Vishal,
thanks. I have only a few minor nitpicking comments.
On 12/05/20 17:19, Vishal Sagar wrote:
> The Xilinx MIPI CSI-2 Rx Subsystem soft IP is used to capture images
> from MIPI CSI-2 camera sensors and output AXI4-Stream video data ready
> for image processing. Please refer to PG232 for details.
>
> The CSI2 Rx controller filters out all packets except for the packets
> with data type fixed in hardware. RAW8 packets are always allowed to
> pass through.
>
> It is also used to setup and handle interrupts and enable the core. It
> logs all the events in respective counters between streaming on and off.
>
> The driver supports only the video format bridge enabled configuration.
> Some data types like YUV 422 10bpc, RAW16, RAW20 are supported when the
> CSI v2.0 feature is enabled in design. When the VCX feature is enabled,
> the maximum number of virtual channels becomes 16 from 4.
>
> Signed-off-by: Vishal Sagar <vishal.sagar@xilinx.com>
> Reviewed-by: Hyun Kwon <hyun.kwon@xilinx.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
[...]
> +static int xcsi2rxss_start_stream(struct xcsi2rxss_state *state)
> +{
> + int ret = 0;
> +
> + /* enable core */
> + xcsi2rxss_set(state, XCSI_CCR_OFFSET, XCSI_CCR_ENABLE);
> +
> + ret = xcsi2rxss_soft_reset(state);
> + if (ret < 0) {
'if (ret)' is enough, it's a classic nonzero-on-error return value.
> +/**
> + * xcsi2rxss_irq_handler - Interrupt handler for CSI-2
> + * @irq: IRQ number
> + * @data: Pointer to device state
> + *
> + * In the interrupt handler, a list of event counters are updated for
> + * corresponding interrupts. This is useful to get status / debug.
> + *
> + * Return: IRQ_HANDLED after handling interrupts
> + */
> +static irqreturn_t xcsi2rxss_irq_handler(int irq, void *data)
> +{
> + struct xcsi2rxss_state *state = (struct xcsi2rxss_state *)data;
> + struct device *dev = state->dev;
> + u32 status;
> +
> + status = xcsi2rxss_read(state, XCSI_ISR_OFFSET) & XCSI_ISR_ALLINTR_MASK;
> + xcsi2rxss_write(state, XCSI_ISR_OFFSET, status);
> +
> + /* Received a short packet */
> + if (status & XCSI_ISR_SPFIFONE) {
> + u32 count = 0;
> +
> + /*
> + * Drain generic short packet FIFO by reading max 31
> + * (fifo depth) short packets from fifo or till fifo is empty.
> + */
> + for (count = 0; count < XCSI_SPKT_FIFO_DEPTH; ++count) {
> + u32 spfifostat, spkt;
> +
> + spkt = xcsi2rxss_read(state, XCSI_SPKTR_OFFSET);
> + dev_dbg(dev, "Short packet = 0x%08x\n", spkt);
> + spfifostat = xcsi2rxss_read(state, XCSI_ISR_OFFSET);
> + spfifostat &= XCSI_ISR_SPFIFONE;
> + if (!spfifostat)
> + break;
> + xcsi2rxss_write(state, XCSI_ISR_OFFSET, spfifostat);
> + }
> + }
> +
> + /* Short packet FIFO overflow */
> + if (status & XCSI_ISR_SPFIFOF)
> + dev_dbg_ratelimited(dev, "Short packet FIFO overflowed\n");
> +
> + /*
> + * Stream line buffer full
> + * This means there is a backpressure from downstream IP
> + */
> + if (status & XCSI_ISR_SLBF) {
> + dev_alert_ratelimited(dev, "Stream Line Buffer Full!\n");
> +
> + /* disable interrupts */
> + xcsi2rxss_clr(state, XCSI_IER_OFFSET, XCSI_IER_INTR_MASK);
> + xcsi2rxss_clr(state, XCSI_GIER_OFFSET, XCSI_GIER_GIE);
> +
> + /* disable core */
> + xcsi2rxss_clr(state, XCSI_CCR_OFFSET, XCSI_CCR_ENABLE);
> + state->streaming = false;
> +
> + /*
> + * The IP needs to be hard reset before it can be used now.
> + * This will be done in streamoff.
> + */
> +
> + /*
> + * TODO: Notify the whole pipeline with v4l2_subdev_notify() to
> + * inform userspace.
> + */
> + }
> +
> + /* Increment event counters */
> + if (status & XCSI_ISR_ALLINTR_MASK) {
> + unsigned int i;
> +
> + for (i = 0; i < XCSI_NUM_EVENTS; i++) {
> + if (!(status & xcsi2rxss_events[i].mask))
> + continue;
> + state->events[i]++;
> + dev_dbg_ratelimited(dev, "%s: %u\n",
> + xcsi2rxss_events[i].name,
> + state->events[i]);
> + }
> +
> + if (status & XCSI_ISR_VCXFE && state->en_vcx) {
> + u32 vcxstatus;
> +
> + vcxstatus = xcsi2rxss_read(state, XCSI_VCXR_OFFSET);
> + vcxstatus &= XCSI_VCXR_VCERR;
> + for (i = 0; i < XCSI_VCX_NUM_EVENTS; i++) {
> + if (!(vcxstatus & (1 << i)))
You can use BIT(i) instead of (1 << i).
> +/**
> + * xcsi2rxss_set_format - This is used to set the pad format
> + * @sd: Pointer to V4L2 Sub device structure
> + * @cfg: Pointer to sub device pad information structure
> + * @fmt: Pointer to pad level media bus format
> + *
> + * This function is used to set the pad format. Since the pad format is fixed
> + * in hardware, it can't be modified on run time. So when a format set is
> + * requested by application, all parameters except the format type is saved
> + * for the pad and the original pad format is sent back to the application.
> + *
> + * Return: 0 on success
> + */
> +static int xcsi2rxss_set_format(struct v4l2_subdev *sd,
> + struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_format *fmt)
> +{
> + struct xcsi2rxss_state *xcsi2rxss = to_xcsi2rxssstate(sd);
> + struct v4l2_mbus_framefmt *__format;
> + u32 dt;
> +
> + /* only sink pad format can be updated */
This comment should be placed...
> + mutex_lock(&xcsi2rxss->lock);
> +
> + /*
> + * Only the format->code parameter matters for CSI as the
> + * CSI format cannot be changed at runtime.
> + * Ensure that format to set is copied to over to CSI pad format
> + */
> + __format = __xcsi2rxss_get_pad_format(xcsi2rxss, cfg,
> + fmt->pad, fmt->which);
> +
...here.
> + if (fmt->pad == XVIP_PAD_SOURCE) {
> + fmt->format = *__format;
> + mutex_unlock(&xcsi2rxss->lock);
> + return 0;
> + }
> +
> + /*
> + * RAW8 is supported in all datatypes. So if requested media bus format
> + * is of RAW8 type, then allow to be set. In case core is configured to
> + * other RAW, YUV422 8/10 or RGB888, set appropriate media bus format.
> + */
> + dt = xcsi2rxss_get_dt(fmt->format.code);
> + if (dt != xcsi2rxss->datatype && dt != XCSI_DT_RAW8) {
> + dev_dbg(xcsi2rxss->dev, "Unsupported media bus format");
> + /* set the default format for the data type */
> + fmt->format.code = xcsi2rxss_get_nth_mbus(xcsi2rxss->datatype,
> + 0);
> + }
> +
> + *__format = fmt->format;
> + mutex_unlock(&xcsi2rxss->lock);
> +
> + return 0;
> +}
> +
> +/*
> + * xcsi2rxss_enum_mbus_code - Handle pixel format enumeration
> + * @sd : pointer to v4l2 subdev structure
> + * @cfg: V4L2 subdev pad configuration
> + * @code : pointer to v4l2_subdev_mbus_code_enum structure
Remove space before colon here.
Looks good otherwise, and my comments are minor details so:
Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
I tried to runtime test this driver as well replacing the v10 driver
that I'm using at the moment, but ran into many problems, apparently in
the media entity navigation. The diff between v10 and v13 does not
justify these problems, so I'm assuming v13 needs a more recent kernel
than the 4.19 I'm currentl stuck on.
--
Luca Ceresoli
^ permalink raw reply
* Re: [PATCH] arm: dts: am33xx-bone-common: add gpio-line-names
From: Linus Walleij @ 2020-05-25 13:03 UTC (permalink / raw)
To: Drew Fustini
Cc: Grygorii Strashko, Benoît Cousson, Tony Lindgren,
Rob Herring, Linux-OMAP,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel@vger.kernel.org, Jason Kridner, Robert Nelson
In-Reply-To: <20200525120710.GA898135@x1>
On Mon, May 25, 2020 at 2:07 PM Drew Fustini <drew@beagleboard.org> wrote:
> On Mon, May 25, 2020 at 11:23:17AM +0200, Linus Walleij wrote:
> > On Thu, May 21, 2020 at 12:02 AM Drew Fustini <drew@beagleboard.org> wrote:
> >
> > > I've posted a v2 which I hope improves the intent of the line names. [0]
> > >
> > > I'm happy to integrate any feedback and create a v3 - especially if it
> > > is prefered for me to list the specific peripherial signals instead of
> > > an abstract term like "[ethernet]" or "[emmc]". This is for lines that
> > > can not be used because they are not routed to the expansion headers.
> > >
> > > [0] https://lore.kernel.org/linux-omap/20200520214757.GA362547@x1/T/#u
> >
> > This looks good to me. FWIW
> > Acked-by: Linus Walleij <linus.walleij@linaro.org>
> >
> > Yours,
> > Linus Walleij
>
> Linus -
>
> I have posted a newer patch that targets am335x-beagleblack.dts [0]
> instead of am335x-bone-common.dtsi as Grygorii Strashko pointed out
> that these line names are not applicable to all BeagleBone models.
>
> The gpio line naming scheme is the same, is it ok to add your Ack?
Yes FWIW
Acked-by.
Linus Walleij
^ permalink raw reply
* Re: [PATCH v5 2/3] dt-bindings: arm: actions: Document Caninos Loucos Labrador
From: Matheus Castello @ 2020-05-25 12:38 UTC (permalink / raw)
To: Andreas Färber, manivannan.sadhasivam, robh+dt
Cc: mark.rutland, edgar.righi, igor.lima, linux-arm-kernel,
devicetree, linux-kernel, linux-actions, Rob Herring
In-Reply-To: <8fc7b0d0-6516-ecd4-ce9c-a63a3cba7e9a@suse.de>
Hi Andreas,
Em 5/25/20 7:41 AM, Andreas Färber escreveu:
> Hi,
>
> Am 25.05.20 um 03:30 schrieb Matheus Castello:
>> Update the documentation to add the Caninos Loucos Labrador. Labrador
>> project consists of a computer on module based on the Actions Semi S500
>> processor and the Labrador base board.
>>
>> Signed-off-by: Matheus Castello <matheus@castello.eng.br>
>> Acked-by: Rob Herring <robh@kernel.org>
>> ---
>> Documentation/devicetree/bindings/arm/actions.yaml | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/actions.yaml
>> b/Documentation/devicetree/bindings/arm/actions.yaml
>> index ace3fdaa8396..2187e1c5bc73 100644
>> --- a/Documentation/devicetree/bindings/arm/actions.yaml
>> +++ b/Documentation/devicetree/bindings/arm/actions.yaml
>> @@ -19,6 +19,11 @@ properties:
>> - allo,sparky # Allo.com Sparky
>> - cubietech,cubieboard6 # Cubietech CubieBoard6
>> - const: actions,s500
>> + - items:
>> + - enum:
>> + - caninos,labrador-v2 # Labrador Core v2
>> + - caninos,labrador-base-m # Labrador Base Board M v1
>
> This enum still strikes me as wrong, it means either-or. (Was planning
> to look into it myself, but no time yet...) caninos,labrador-v2 should
> be a const one level down: board, SoM, SoC from most specific to most
> generic. Compare Guitar below.
I got it, I agree make sense, I will send the v6.
>
>> + - const: actions,s500
>> - items:
>> - enum:
>> - lemaker,guitar-bb-rev-b # LeMaker Guitar Base Board
>> rev. B
>
> Regards,
> Andreas
>
Best Regards,
Matheus Castello
^ permalink raw reply
* Re: [PATCH v2 07/12] i2c: designware: Move Baytrail sem config to the platform if-clause
From: Jarkko Nikula @ 2020-05-25 13:01 UTC (permalink / raw)
To: Serge Semin
Cc: Serge Semin, Alexey Malahov, Thomas Bogendoerfer, Paul Burton,
Ralf Baechle, Andy Shevchenko, Mika Westerberg, Wolfram Sang,
Rob Herring, Frank Rowand, linux-mips, devicetree, Wolfram Sang,
Jean Delvare, Krzysztof Kozlowski, Max Staudt, Stefan Roese,
linux-i2c, linux-kernel
In-Reply-To: <20200521022215.ubvhuop47aflqkkb@mobilestation>
On 5/21/20 5:22 AM, Serge Semin wrote:
> On Wed, May 20, 2020 at 03:16:14PM +0300, Jarkko Nikula wrote:
>> On 5/10/20 12:50 PM, Serge Semin wrote:
>>> Currently Intel Baytrail I2C semaphore is a feature of the DW APB I2C
>>> platform driver. It's a bit confusing to see it's config in the menu at
>>> some separated place with no reference to the platform code. Lets move the
>>> config definition under the if-I2C_DESIGNWARE_PLATFORM clause. By doing so
>>> the config menu will display the feature right below the DW I2C platform
>>> driver item and will indent it to the right so signifying its belonging.
>>>
>>> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
>>> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
>>> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
>>> Cc: Paul Burton <paulburton@kernel.org>
>>> Cc: Ralf Baechle <ralf@linux-mips.org>
>>> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
>>> Cc: Wolfram Sang <wsa@the-dreams.de>
>>> Cc: Rob Herring <robh+dt@kernel.org>
>>> Cc: Frank Rowand <frowand.list@gmail.com>
>>> Cc: linux-mips@vger.kernel.org
>>> Cc: devicetree@vger.kernel.org
>>> ---
>>> drivers/i2c/busses/Kconfig | 30 +++++++++++++++++-------------
>>> 1 file changed, 17 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>>> index 368aa64e9266..ed6927c4c540 100644
>>> --- a/drivers/i2c/busses/Kconfig
>>> +++ b/drivers/i2c/busses/Kconfig
>>> @@ -530,8 +530,8 @@ config I2C_DESIGNWARE_CORE
>>> config I2C_DESIGNWARE_PLATFORM
>>> tristate "Synopsys DesignWare Platform"
>>> - select I2C_DESIGNWARE_CORE
>>> depends on (ACPI && COMMON_CLK) || !ACPI
>>> + select I2C_DESIGNWARE_CORE
>>> help
>>> If you say yes to this option, support will be included for the
>>> Synopsys DesignWare I2C adapter.
>>> @@ -539,6 +539,22 @@ config I2C_DESIGNWARE_PLATFORM
>>> This driver can also be built as a module. If so, the module
>>> will be called i2c-designware-platform.
>>> +if I2C_DESIGNWARE_PLATFORM
>>> +
>>> +config I2C_DESIGNWARE_BAYTRAIL
>>> + bool "Intel Baytrail I2C semaphore support"
>>> + depends on ACPI
>>> + depends on (I2C_DESIGNWARE_PLATFORM=m && IOSF_MBI) || \
>>> + (I2C_DESIGNWARE_PLATFORM=y && IOSF_MBI=y)
>>> + help
>>> + This driver enables managed host access to the PMIC I2C bus on select
>>> + Intel BayTrail platforms using the X-Powers AXP288 PMIC. It allows
>>> + the host to request uninterrupted access to the PMIC's I2C bus from
>>> + the platform firmware controlling it. You should say Y if running on
>>> + a BayTrail system using the AXP288.
>>> +
>>> +endif # I2C_DESIGNWARE_PLATFORM
>>> +
>>
>> Is the added "if I2C_DESIGNWARE_PLATFORM" needed here? Should the "depends
>> on" be enough?
>
> The idea was to add if-endif clause here for features possibly added sometime
> in future. But using normal "depends on I2C_DESIGNWARE_PLATFORM" shall make
> the config depicted as an indented sub-config as well. Would you like me to
> remove the if-clause and use the depends on operator instead?
>
Yes, please remove it from this patch. Keeps this patch simpler and if
some future feature needs it then that patch(set) is the right place to
add it.
Jarkko
^ permalink raw reply
* Re: [PATCH v3 10/16] gpio: add a reusable generic gpio_chip using regmap
From: Linus Walleij @ 2020-05-25 12:59 UTC (permalink / raw)
To: Michael Walle
Cc: Bartosz Golaszewski, Andy Shevchenko, linux-gpio,
linux-devicetree, LKML, linux-hwmon, linux-pwm, LINUXWATCHDOG,
arm-soc, Rob Herring, Jean Delvare, Guenter Roeck, Lee Jones,
Thierry Reding, Uwe Kleine-König, Wim Van Sebroeck,
Shawn Guo, Li Yang, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Mark Brown, Greg Kroah-Hartman
In-Reply-To: <951244aab2ff553a463f7431ba09bf27@walle.cc>
On Mon, May 25, 2020 at 12:20 PM Michael Walle <michael@walle.cc> wrote:
> If you like I could submit this patch on its own. But then there
> wouldn't be a user for it.
I'm pretty much fine with that, we do merge code that has no
users if we anticipate they will be around the corner.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v11 02/13] dt-bindings: video-interface: Replace 'rotation' description
From: Jacopo Mondi @ 2020-05-25 12:46 UTC (permalink / raw)
To: Pavel Machek
Cc: open list:MEDIA INPUT INFRASTRUCTURE (V4L/DVB), libcamera-devel,
Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus,
Laurent Pinchart, Rob Herring, tfiga, devicetree
In-Reply-To: <20200525111621.GE27989@amd>
[-- Attachment #1: Type: text/plain, Size: 3193 bytes --]
Pavel,
On Mon, May 25, 2020 at 01:16:21PM +0200, Pavel Machek wrote:
> On Sat 2020-05-09 11:04:45, Jacopo Mondi wrote:
> > Replace the 'rotation' property description by providing a definition
> > relative to the camera sensor pixel array coordinate system and the
> > captured scene.
> >
> > Acked-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> > .../bindings/media/video-interfaces.txt | 359 +++++++++++++++++-
> > 1 file changed, 356 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > index b1ff492c7da7a..3920f25a91235 100644
> > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > @@ -85,9 +85,362 @@ Optional properties
> >
> > - lens-focus: A phandle to the node of the focus lens controller.
> >
> > -- rotation: The device, typically an image sensor, is not mounted upright,
> > - but a number of degrees counter clockwise. Typical values are 0 and 180
> > - (upside down).
> > +- rotation: The camera rotation is expressed as the angular difference in
> > + degrees between two reference systems, one relative to the camera module, and
> > + one defined on the external world scene to be captured when projected on the
> > + image sensor pixel array.
>
> So.. how is this supposed to work. There's a phone, with its main
> camera. It is designed to be used in both portrait and landscape
> modes, internal accelerometr can tell between the two.
>
> Plus you have phone with a keyboard. Common usage is portrait with
> keyboard closed, and landscape with keyboard open...
>
While I agree for 'orientation' we're far from perfection (and that's
why I've been asked s/location/orientation/ to reserve 'location' for
the future), 'rotation' is only defined by the camera sensor mounting
rotation and the external scene. The way you move your phone should
not impact the pixel read-out sequence.
All phones can be used in portarait or landscape mode, I don't see why
having a mechanical keyboard is different. You can type in landscape
mode on a touchscreen keyboard, if you like to.
The property gives you the rotation correction (actually, the control
gives the correction) for display in the 'intended usage orientation'.
If your concern is displaying the image, as you sense the
accellerometers to rotate the UI, the application that display the
image will be rotated too, adding to the corrected image (capture image +
rotation here specified) the required UI rotation.
On the 'intended usage orientation', as there is one for the UI, there
is one for all other components in the phone, so if your system
establishes that 'portrait' is the (rotation = 0) point for the UI,
this applies to the rest of the system.
> And yes, there's linux with v4l2 working on Nokia N900.
>
How is this and orientation handled there ?
> Best regards,
> Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [RFC PATCH V4 4/4] platform: mtk-isp: Add Mediatek FD driver
From: Tomasz Figa @ 2020-05-25 12:24 UTC (permalink / raw)
To: Jerry-ch Chen
Cc: Hans Verkuil, Hans Verkuil, Laurent Pinchart, Matthias Brugger,
Mauro Carvalho Chehab, Pi-Hsun Shih, yuzhao, zwisler,
moderated list:ARM/Mediatek SoC support,
list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>,,
Sean Cheng (鄭昇弘), Sj Huang,
Christie Yu (游雅惠),
Frederic Chen (陳俊元),
Jungo Lin (林明俊),
Rynn Wu (吳育恩), Linux Media Mailing List,
srv_heupstream, linux-devicetree
In-Reply-To: <1590156658.27807.84.camel@mtksdccf07>
r
On Fri, May 22, 2020 at 4:11 PM Jerry-ch Chen
<Jerry-ch.Chen@mediatek.com> wrote:
>
> Hi Tomasz,
>
> On Thu, 2020-05-21 at 18:28 +0000, Tomasz Figa wrote:
> > Hi Jerry,
> >
> > On Wed, Dec 04, 2019 at 08:47:32PM +0800, Jerry-ch Chen wrote:
[snip]
> > > +
> > > +enum face_angle {
> > > + MTK_FD_FACE_FRONT,
> > > + MTK_FD_FACE_RIGHT_50,
> > > + MTK_FD_FACE_LEFT_50,
> > > + MTK_FD_FACE_RIGHT_90,
> > > + MTK_FD_FACE_LEFT_90,
> > > + MTK_FD_FACE_ANGLE_NUM,
> > > +};
> >
> > This enum seems to define values for the V4L2_CID_MTK_FD_DETECT_POSE
> > control. Considering that this is an enumeration and the values are
> > actually integers (-90, -50, 0, 50, 90), perhaps this should be an
> > INTEGER_MENU control instead?
> >
>
> this ioctl let user select multiple face positions(combination of angles
> and directions) to be detected. so I thought I am not able to use the
> INTEGER_MENU for this purpose.
Ah, okay, I thought there is only one selection possible.
>
> A bit-field as following should be used by user.
> I consider adding it to uapi.
>
> struct face_direction_def {
> __u16 MTK_FD_FACE_DIR_0 : 1,
> MTK_FD_FACE_DIR_30 : 1,
> MTK_FD_FACE_DIR_60 : 1,
> MTK_FD_FACE_DIR_90 : 1,
> MTK_FD_FACE_DIR_120 : 1,
> MTK_FD_FACE_DIR_150 : 1,
> MTK_FD_FACE_DIR_180 : 1,
> MTK_FD_FACE_DIR_210 : 1,
> MTK_FD_FACE_DIR_240 : 1,
> MTK_FD_FACE_DIR_270 : 1,
> MTK_FD_FACE_DIR_300 : 1,
> MTK_FD_FACE_DIR_330 : 1,
> : 4;
> };
Note that bit fields are not recommended in UAPI because of not being
well specified by the language. Instead bits should be defined using
macros with explicit masks or sometimes enums.
>
> User can also select some face directions of each face angle in one
> ioctl, for example:
>
> /*
> * u16 face_directions[MTK_FD_FACE_ANGLE_NUM] = {0};
> *
> * face_directions[MTK_FD_FACE_FRONT] = 0x7; //angle:0, dir:0,30,60
> * face_directions[MTK_FACE_RIGHT_50] = 0x2; //angle:50, dir:30
> *
> */
Makes sense, thanks.
>
> > > +
> > > +struct fd_buffer {
> > > + __u32 scp_addr; /* used by SCP */
> > > + __u32 dma_addr; /* used by DMA HW */
> > > +} __packed;
> fd buffer is used for scp ipi
>
> > > +
> > > +struct fd_face_result {
> > > + char data[16];
> > > +};
> fd_face_result is used for user, so it should be moved to
> include/uapi/linux.
> In fact, it has bit-field definition for user, so I would like to define
> it in include/uapi/linux as following:
>
> struct fd_face_result {
> __u64 face_idx : 12,
> type : 1,
> x0 : 10,
> y0 : 10,
> x1 : 10,
> y1 : 10,
> fcv1 : 11;
> __u64 fcv2 : 7,
> rip_dir : 4,
> rop_dir : 3,
> det_size : 5;
> };
>
Indeed this should be defined, but as per my comment above, please
avoid using the bitfield construct and define shifts and masks
instead.
>
> > > +
> > > +struct fd_user_output {
> > > + struct fd_face_result results[MTK_FD_MAX_RESULT_NUM];
> > > + __u16 number;
> >
> > Is this perhaps the number of results? If so, would num_results be a better
> > name?
> >
> yes, fixed.
> > > +};
> >
> > Since this struct is the meta buffer format, it is a part of the userspace
> > interface and should be defined in a header under include/uapi/linux/.
> >
> Ok, I will create include/uapi/linux/mtk_fd_40.h
> which suppose to include structures that userspace will use.
> should the private IOCTLs be placed in it together?
>
Sorry, what private IOCTLs are you referring to? If you mean private
control IDs, then yes, they should go to that header.
[snip]
> > > +static int mtk_fd_vb2_queue_setup(struct vb2_queue *vq,
> > > + unsigned int *num_buffers,
> > > + unsigned int *num_planes,
> > > + unsigned int sizes[],
> > > + struct device *alloc_devs[])
> > > +{
> > > + struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> > > + unsigned int size[2];
> > > + unsigned int plane;
> > > +
> > > + switch (vq->type) {
> > > + case V4L2_BUF_TYPE_META_CAPTURE:
> > > + size[0] = ctx->dst_fmt.buffersize;
> > > + break;
> > > + case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> > > + size[0] = ctx->src_fmt.plane_fmt[0].sizeimage;
> > > + if (*num_planes == 2)
> > > + size[1] = ctx->src_fmt.plane_fmt[1].sizeimage;
> > > + break;
> > > + }
> >
> > Is this code above needed? The code below sets sizes[] and it uses a for loop,
> > without opencoded assignment for the second plane.
> >
>
> Looks like not really useful here,
> it should check sizes and num_planes if num_plane not zero,
> and for V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE, it will at most have 2
> planes, maybe no need for loop as well.
Loops generally make the code cleaner and there might be some desire
to add support for more formats in the future, e.g. in case a next
generation of the hardware shows up.
> I will refine this function as following:
> mtk_fd_vb2_queue_setup(...)
> {
> struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
>
> if (*num_planes == 0) {
> if (vq->type == V4L2_BUF_TYPE_META_CAPTURE) {
> sizes[0] = ctx->dst_fmt.buffersize;
> *num_planes = 1;
> return 0;
> } else if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> *num_planes = ctx->src_fmt.num_planes;
> sizes[0] = ctx->src_fmt.plane_fmt[0].sizeimage;
> if (*num_planes == 2)
> sizes[1] = ctx->src_fmt.plane_fmt[1].sizeimage;
> return 0;
> }
> return -EINVAL;
> }
>
> /* If num_plane not zero, check the num_plane and sizes*/
> if (vq->type == V4L2_BUF_TYPE_META_CAPTURE) {
> if ((*num_planes == 1) &&
> (sizes[0] <= ctx->dst_fmt.buffersize))
> return 0;
nit: The typical convention is to check for problems and return the
error code earlier, with the success handled at the end of the block.
> else
> return -EINVAL;
> }
> if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> if ((*num_planes == 1) &&
> (sizes[0] <= ctx->src_fmt.plane_fmt[0].sizeimage))
> return 0;
> else if ((*num_planes == 2) &&
> (sizes[0] <= ctx->src_fmt.plane_fmt[0].sizeimage) &&
> (sizes[1] <= ctx->src_fmt.plane_fmt[1].sizeimage))
> return 0;
Wouldn't a loop eliminate the need to if/else if through the various
supported cases and duplicate the size checks?
> else
> return -EINVAL;
>
> }
> return 0;
> }
How about the following?
mtk_fd_vb2_queue_setup(...)
{
struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
if (vq->type == V4L2_BUF_TYPE_META_CAPTURE) {
if (*num_planes == 0) {
*num_planes = 1;
sizes[0] = ctx->dst_fmt.buffersize;
return 0;
}
if (*num_planes != 1 || sizes[0] < ctx->dst_fmt.buffersize)
return -EINVAL;
return 0;
}
/* V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE */
if (*num_planes == 0) {
*num_planes = ctx->src_fmt.num_planes;
for (i = 0; i < ctx->src_fmt.num_planes; ++i)
sizes[i] = ctx->src_fmt.plane_fmt[i].sizeimage;
return 0;
}
if (*num_planes < ctx->src_fmt.num_planes)
return -EINVAL;
for (i = 0; i < ctx->src_fmt.num_planes; ++i)
if (sizes[i] < ctx->src_fmt.plane_fmt[i].sizeimage)
return -EINVAL;
return 0;
}
Note that it fully separates the code dealing with each queue and thus
improves the readability.
In this case, it could actually be beneficial to split the vb2_ops
implementation into one that deals only with video_output_mplane and
one only with meta_capture. This would allow eliminating the special
casing based on vq->type and thus further simplify the code. Not sure
if it applies to the other vb2 callbacks, though.
[snip]
> > > +static void mtk_fd_fill_pixfmt_mp(struct v4l2_pix_format_mplane *dfmt,
> > > + const struct v4l2_pix_format_mplane *sfmt)
> > > +{
> > > + dfmt->field = V4L2_FIELD_NONE;
> > > + dfmt->colorspace = V4L2_COLORSPACE_BT2020;
> > > + dfmt->num_planes = sfmt->num_planes;
> > > + dfmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> > > + dfmt->quantization = V4L2_QUANTIZATION_DEFAULT;
> > > + dfmt->xfer_func =
> > > + V4L2_MAP_XFER_FUNC_DEFAULT(dfmt->colorspace);
> > > +
> > > + /* Keep user setting as possible */
> > > + dfmt->width = clamp(dfmt->width,
> > > + MTK_FD_OUTPUT_MIN_WIDTH,
> > > + MTK_FD_OUTPUT_MAX_WIDTH);
> > > + dfmt->height = clamp(dfmt->height,
> > > + MTK_FD_OUTPUT_MIN_HEIGHT,
> > > + MTK_FD_OUTPUT_MAX_HEIGHT);
> > > +
> > > + if (sfmt->num_planes == 2) {
> > > + /* NV16M and NV61M has 1 byte per pixel */
> > > + dfmt->plane_fmt[0].bytesperline = dfmt->width;
> > > + dfmt->plane_fmt[1].bytesperline = dfmt->width;
> > > + } else {
> > > + /* 2 bytes per pixel */
> > > + dfmt->plane_fmt[0].bytesperline = dfmt->width * 2;
> > > + }
> > > +
> > > + dfmt->plane_fmt[0].sizeimage =
> > > + dfmt->height * dfmt->plane_fmt[0].bytesperline;
> >
> > Could some of the code above be replaced with v4l2_fill_pixfmt_mp()?
> >
> I would like to refine as following
>
> mtk_fd_fill_pixfmt_mp(...){
> v4l2_fill_pixfmt_mp(dfmt, sfmt->pixelformat, dfmt->width,
> dfmt->height);
>
> dfmt->field = V4L2_FIELD_NONE;
> dfmt->colorspace = V4L2_COLORSPACE_BT2020;
> dfmt->num_planes = sfmt->num_planes;
num_planes should be already filled in by the helper. That actually
raises a question on whether there is any need to have sfmt passed to
this function at all. Perhaps all we need is the value of pixelformat?
> dfmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> dfmt->quantization = V4L2_QUANTIZATION_DEFAULT;
> dfmt->xfer_func =
> V4L2_MAP_XFER_FUNC_DEFAULT(dfmt->colorspace);
> }
>
Isn't still a need to clamp() width and height to min/max, though?
[snip]
> > > + fd_param.user_param.src_img_fmt =
> > > + get_fd_img_fmt(ctx->src_fmt.pixelformat);
> > > + if (ctx->src_fmt.num_planes == 2)
> > > + fd_param.src_img[1].dma_addr =
> > > + vb2_dma_contig_plane_dma_addr(&src_buf->vb2_buf, 1);
> >
> > nit: Could this be moved above, to be just below src_img[0] initialization,
> > for readability reasons?
> >
> Ok, this function will be refined as
>
> static void mtk_fd_device_run(void *priv)
> {
> struct mtk_fd_ctx *ctx = priv;
> struct mtk_fd_dev *fd = ctx->fd_dev;
> struct vb2_v4l2_buffer *src_buf, *dst_buf;
> struct fd_enq_param fd_param;
>
> src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
>
> fd_param.src_img[0].dma_addr =
> vb2_dma_contig_plane_dma_addr(&src_buf->vb2_buf, 0);
> if (ctx->src_fmt.num_planes == 2)
> fd_param.src_img[1].dma_addr =
> vb2_dma_contig_plane_dma_addr(&src_buf->vb2_buf, 1);
How about making this a loop in terms of ctx->src_fmt.num_planes?
> fd_param.user_result.dma_addr =
> vb2_dma_contig_plane_dma_addr(&dst_buf->vb2_buf, 0);
> fd_param.user_param.src_img_fmt =
> get_fd_img_fmt(fd->dev, ctx->src_fmt.pixelformat);
>
> mtk_fd_fill_user_param(&fd_param.user_param, &ctx->hdl);
>
> /* Complete request controls if any */
> v4l2_ctrl_request_complete(src_buf->vb2_buf.req_obj.req, &ctx->hdl);
>
> fd->output = vb2_plane_vaddr(&dst_buf->vb2_buf, 0);
> mtk_fd_hw_job_exec(fd, &fd_param);
> }
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH v5 6/8] drm/panel: Add ilitek ili9341 panel driver
From: Linus Walleij @ 2020-05-25 12:23 UTC (permalink / raw)
To: dillon min
Cc: Mark Brown,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, linux-kernel@vger.kernel.org, linux-spi, linux-stm32,
open list:DRM PANEL DRIVERS, linux-clk
In-Reply-To: <1590378348-8115-7-git-send-email-dillon.minfei@gmail.com>
On Mon, May 25, 2020 at 5:46 AM <dillon.minfei@gmail.com> wrote:
> From: dillon min <dillon.minfei@gmail.com>
>
> This driver combine tiny/ili9341.c mipi_dbi_interface driver
> with mipi_dpi_interface driver, can support ili9341 with serial
> mode or parallel rgb interface mode by register configuration.
>
> Changes since V3:
>
> accoding to Linus Walleij's suggestion.
> 1 add more comments to driver.
> 2 reduce magic number usage in the driver.
> 3 move panel configuration from common place to system configuration.
> 4 reuse MIPI_DCS_* as more as possible.
>
> Signed-off-by: dillon min <dillon.minfei@gmail.com>
This looks good to me!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v5 4/8] dt-bindings: display: panel: Add ilitek ili9341 panel bindings
From: Linus Walleij @ 2020-05-25 12:20 UTC (permalink / raw)
To: dillon min
Cc: Mark Brown,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, linux-kernel@vger.kernel.org, linux-spi, linux-stm32,
open list:DRM PANEL DRIVERS, linux-clk
In-Reply-To: <1590378348-8115-5-git-send-email-dillon.minfei@gmail.com>
On Mon, May 25, 2020 at 5:46 AM <dillon.minfei@gmail.com> wrote:
> From: dillon min <dillon.minfei@gmail.com>
>
> Add documentation for "ilitek,ili9341" panel.
>
> Signed-off-by: dillon min <dillon.minfei@gmail.com>
This looks good to me!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH] arm: dts: am33xx-bone-common: add gpio-line-names
From: Drew Fustini @ 2020-05-25 12:07 UTC (permalink / raw)
To: Linus Walleij
Cc: Grygorii Strashko, Benoît Cousson, Tony Lindgren,
Rob Herring, Linux-OMAP,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel@vger.kernel.org, Jason Kridner, Robert Nelson
In-Reply-To: <CACRpkdYV4Pd2rL=Kha6HxL8J5+vFy_M0hV7+qMghS4AVxd9D2w@mail.gmail.com>
On Mon, May 25, 2020 at 11:23:17AM +0200, Linus Walleij wrote:
> On Thu, May 21, 2020 at 12:02 AM Drew Fustini <drew@beagleboard.org> wrote:
>
> > I've posted a v2 which I hope improves the intent of the line names. [0]
> >
> > I'm happy to integrate any feedback and create a v3 - especially if it
> > is prefered for me to list the specific peripherial signals instead of
> > an abstract term like "[ethernet]" or "[emmc]". This is for lines that
> > can not be used because they are not routed to the expansion headers.
> >
> > [0] https://lore.kernel.org/linux-omap/20200520214757.GA362547@x1/T/#u
>
> This looks good to me. FWIW
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>
> Yours,
> Linus Walleij
Linus -
I have posted a newer patch that targets am335x-beagleblack.dts [0]
instead of am335x-bone-common.dtsi as Grygorii Strashko pointed out
that these line names are not applicable to all BeagleBone models.
The gpio line naming scheme is the same, is it ok to add your Ack?
thanks,
drew
[0] https://lore.kernel.org/linux-omap/20200521200926.GC429020@x1/
^ permalink raw reply
* Re: [PATCH v15 2/2] i2c: core: support bus regulator controlling in adapter
From: Wolfram Sang @ 2020-05-25 12:00 UTC (permalink / raw)
To: Tomasz Figa
Cc: Bibby Hsieh, Bartosz Golaszewski, linux-i2c, Nicolas Boichat,
srv_heupstream, Rob Herring, Mark Rutland, linux-devicetree,
Rafael J . Wysocki
In-Reply-To: <CAAFQd5CdmcYpSLsgOPJqo+=kOYundF5NZua4QpdOTTWYSYAopQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 270 bytes --]
> > Reverted because of regression reports in linux-next. I am all open for
> > the change in general, but it looks to me that we should wait another
> > cycle.
> >
>
> Thanks Wolfram and sorry for the trouble.
No worries. Glad you guys are working on it.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/6] arm64: dts: qcom: sm8250: add apps_smmu node
From: Sai Prakash Ranjan @ 2020-05-25 11:58 UTC (permalink / raw)
To: Jonathan Marek
Cc: linux-arm-msm, Andy Gross, Bjorn Andersson, Rob Herring,
devicetree, linux-kernel, linux-arm-msm-owner
In-Reply-To: <8f9a5750-7909-4be7-6780-198d8c242af3@marek.ca>
On 2020-05-25 17:23, Jonathan Marek wrote:
> On 5/25/20 7:40 AM, Sai Prakash Ranjan wrote:
>> On 2020-05-25 16:57, Jonathan Marek wrote:
>>> On 5/25/20 7:17 AM, Sai Prakash Ranjan wrote:
>>>> Hi,
>>>>
>>>> On 2020-05-25 16:38, Jonathan Marek wrote:
>>>>> On 5/25/20 6:54 AM, Sai Prakash Ranjan wrote:
>>>>>> On 2020-05-25 15:39, Jonathan Marek wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 5/25/20 5:42 AM, Sai Prakash Ranjan wrote:
>>>>>>>> Hi Jonathan,
>>>>>>>>
>>>>>>>> On 2020-05-24 08:08, Jonathan Marek wrote:
>>>>>>>>> Add the apps_smmu node for sm8250. Note that adding the iommus
>>>>>>>>> field for
>>>>>>>>> UFS is required because initializing the iommu removes the
>>>>>>>>> bypass mapping
>>>>>>>>> that created by the bootloader.
>>>>>>>>>
>>>>>>>>
>>>>>>>> This statement doesn't seem right, you can just say since the
>>>>>>>> bypass is disabled
>>>>>>>> by default now, we need to add this property to enable
>>>>>>>> translation and avoid global faults.
>>>>>>>>
>>>>>>>
>>>>>>> If I use this patch [1] then the UFS iommu property isn't needed.
>>>>>>> In
>>>>>>> downstream, the identity (bypass?) stream mapping is inherited
>>>>>>> from
>>>>>>> the bootloader, and UFS is used without any iommu property.
>>>>>>> Setting
>>>>>>> ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n doesn't make it work on its
>>>>>>> own
>>>>>>> (without the UFS iommu property), so there's more to it than just
>>>>>>> "bypass is disabled by default now".
>>>>>>>
>>>>>>> https://patchwork.kernel.org/patch/11310757/
>>>>>>>
>>>>>>
>>>>>> "iommus" property is not about inheriting stream mapping from
>>>>>> bootloader,
>>>>>> it is used to enable SMMU address translation for the
>>>>>> corresponding
>>>>>> master when specified. So when you have disabled bypass, i.e.,
>>>>>> ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=y or via cmdline
>>>>>> "arm-smmu.disable_bypass=1"
>>>>>> and iommus property with SID and mask is not specified, then it
>>>>>> will result
>>>>>> in SMMU global faults.
>>>>>>
>>>>>> Downstream has bypass
>>>>>> enabled(ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n),so you
>>>>>> won't see any global faults if you do not have iommus property.
>>>>>>
>>>>>> Patch in your link is for display because of the usecase for
>>>>>> splash screen
>>>>>> on android and some other devices where the bootloader will
>>>>>> configure SMMU,
>>>>>> it has not yet merged and not likely to get merged in the current
>>>>>> state.
>>>>>>
>>>>>> So yes "there is *not* much more to it than bypass is disabled by
>>>>>> default now"
>>>>>> and you have to specify "iommus" for the master devices or you
>>>>>> should enable bypass,
>>>>>> i.e., ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n or
>>>>>> arm-smmu.disable_bypass=n
>>>>>>
>>>>>> Try without the patch in the link and without iommus for UFS and
>>>>>> ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=y and you will see.
>>>>>>
>>>>>> -Sai
>>>>>
>>>>> I know that the "iommus" property is not about inheriting stream
>>>>> mapping. Probing the iommu removes the stream mapping created by
>>>>> the
>>>>> bootloader, the iommus property is added so that new mappings are
>>>>> created to replace what was removed.
>>>>>
>>>>> You seem to be under the impression that the SM8150/SM8250
>>>>> bootloader
>>>>> does not configure SMMU. It does, for both UFS and SDHC, just like
>>>>> it
>>>>> does for display/splash screen on some devices.
>>>>>
>>>>
>>>> It could be that bootloader does configure SMMU for UFS and SDHC,
>>>> but the
>>>> upstream SMMU driver doesnt allow to inherit stream mapping from the
>>>> bootloader
>>>> yet, so adding iommus property based on the assumption that it is
>>>> inherited seems
>>>> wrong.
>>>>
>>>
>>> I never said adding the iommus property is for inheriting stream
>>> mapping. I mentioned inheriting to say UFS works without the iommus
>>> property on downstream (it inherits a identity/bypass mapping).
>>>
>>
>> Your commit description says "adding the iommus field for UFS is
>> required
>> because initializing the iommu removes the bypass mapping that created
>> by the
>> bootloader". So here it would mean like iommus property for UFS is not
>> for
>> enabling address translation by SMMU for UFS but to avoid removing
>> mappings
>> created by the bootloader which is not exactly what iommus property is
>> for.
>>
>
> I guess the commit message is ambiguous, that's not what I meant. Is
> "Now that the kernel initializes the iommu, the bypass mappings set by
> the bootloader are cleared. Adding the iommus property is required so
> that new mappings are created for UFS." better?
>
Yes looks good.
>>>>> With either value of ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT, it will
>>>>> not
>>>>> work without the iommus property.
>>>>
>>>> I'm pretty sure that if you have
>>>> ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n and
>>>> without iommus, it should work.
>>>>
>>>
>>> It doesn't work, with either ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n or
>>> ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=y.
>>>
>>
>> Ok since you are very sure about this, I will try with your patches on
>> SM8150 MTP tomorrow since I do not have access to one now.
>> Also just to make sure, please remove all the extra SMMU patches you
>> have
>> in your tree which are not yet merged or from downstream kernel.
>>
>
> FYI, I have a branch [1] integrating patches for upstream. All the
> patches up to 34fff8a519cc075933 ("arm64: dts: qcom: add sm8250 GPU
> nodes") have been submitted (and the patches after that are
> unnecessary for testing on sm8150).
>
> [1] https://github.com/flto/linux/commits/sm8x50-upstream
>
Thanks, I will test this out.
-Sai
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member
of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply
* Re: [PATCH 2/6] arm64: dts: qcom: sm8250: add apps_smmu node
From: Jonathan Marek @ 2020-05-25 11:53 UTC (permalink / raw)
To: Sai Prakash Ranjan
Cc: linux-arm-msm, Andy Gross, Bjorn Andersson, Rob Herring,
devicetree, linux-kernel, linux-arm-msm-owner
In-Reply-To: <1adedb96a999a08809afe62416e80075@codeaurora.org>
On 5/25/20 7:40 AM, Sai Prakash Ranjan wrote:
> On 2020-05-25 16:57, Jonathan Marek wrote:
>> On 5/25/20 7:17 AM, Sai Prakash Ranjan wrote:
>>> Hi,
>>>
>>> On 2020-05-25 16:38, Jonathan Marek wrote:
>>>> On 5/25/20 6:54 AM, Sai Prakash Ranjan wrote:
>>>>> On 2020-05-25 15:39, Jonathan Marek wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 5/25/20 5:42 AM, Sai Prakash Ranjan wrote:
>>>>>>> Hi Jonathan,
>>>>>>>
>>>>>>> On 2020-05-24 08:08, Jonathan Marek wrote:
>>>>>>>> Add the apps_smmu node for sm8250. Note that adding the iommus
>>>>>>>> field for
>>>>>>>> UFS is required because initializing the iommu removes the
>>>>>>>> bypass mapping
>>>>>>>> that created by the bootloader.
>>>>>>>>
>>>>>>>
>>>>>>> This statement doesn't seem right, you can just say since the
>>>>>>> bypass is disabled
>>>>>>> by default now, we need to add this property to enable
>>>>>>> translation and avoid global faults.
>>>>>>>
>>>>>>
>>>>>> If I use this patch [1] then the UFS iommu property isn't needed. In
>>>>>> downstream, the identity (bypass?) stream mapping is inherited from
>>>>>> the bootloader, and UFS is used without any iommu property. Setting
>>>>>> ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n doesn't make it work on its own
>>>>>> (without the UFS iommu property), so there's more to it than just
>>>>>> "bypass is disabled by default now".
>>>>>>
>>>>>> https://patchwork.kernel.org/patch/11310757/
>>>>>>
>>>>>
>>>>> "iommus" property is not about inheriting stream mapping from
>>>>> bootloader,
>>>>> it is used to enable SMMU address translation for the corresponding
>>>>> master when specified. So when you have disabled bypass, i.e.,
>>>>> ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=y or via cmdline
>>>>> "arm-smmu.disable_bypass=1"
>>>>> and iommus property with SID and mask is not specified, then it
>>>>> will result
>>>>> in SMMU global faults.
>>>>>
>>>>> Downstream has bypass
>>>>> enabled(ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n),so you
>>>>> won't see any global faults if you do not have iommus property.
>>>>>
>>>>> Patch in your link is for display because of the usecase for splash
>>>>> screen
>>>>> on android and some other devices where the bootloader will
>>>>> configure SMMU,
>>>>> it has not yet merged and not likely to get merged in the current
>>>>> state.
>>>>>
>>>>> So yes "there is *not* much more to it than bypass is disabled by
>>>>> default now"
>>>>> and you have to specify "iommus" for the master devices or you
>>>>> should enable bypass,
>>>>> i.e., ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n or
>>>>> arm-smmu.disable_bypass=n
>>>>>
>>>>> Try without the patch in the link and without iommus for UFS and
>>>>> ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=y and you will see.
>>>>>
>>>>> -Sai
>>>>
>>>> I know that the "iommus" property is not about inheriting stream
>>>> mapping. Probing the iommu removes the stream mapping created by the
>>>> bootloader, the iommus property is added so that new mappings are
>>>> created to replace what was removed.
>>>>
>>>> You seem to be under the impression that the SM8150/SM8250 bootloader
>>>> does not configure SMMU. It does, for both UFS and SDHC, just like it
>>>> does for display/splash screen on some devices.
>>>>
>>>
>>> It could be that bootloader does configure SMMU for UFS and SDHC, but
>>> the
>>> upstream SMMU driver doesnt allow to inherit stream mapping from the
>>> bootloader
>>> yet, so adding iommus property based on the assumption that it is
>>> inherited seems
>>> wrong.
>>>
>>
>> I never said adding the iommus property is for inheriting stream
>> mapping. I mentioned inheriting to say UFS works without the iommus
>> property on downstream (it inherits a identity/bypass mapping).
>>
>
> Your commit description says "adding the iommus field for UFS is required
> because initializing the iommu removes the bypass mapping that created
> by the
> bootloader". So here it would mean like iommus property for UFS is not for
> enabling address translation by SMMU for UFS but to avoid removing mappings
> created by the bootloader which is not exactly what iommus property is for.
>
I guess the commit message is ambiguous, that's not what I meant. Is
"Now that the kernel initializes the iommu, the bypass mappings set by
the bootloader are cleared. Adding the iommus property is required so
that new mappings are created for UFS." better?
>>>> With either value of ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT, it will not
>>>> work without the iommus property.
>>>
>>> I'm pretty sure that if you have ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n
>>> and
>>> without iommus, it should work.
>>>
>>
>> It doesn't work, with either ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n or
>> ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=y.
>>
>
> Ok since you are very sure about this, I will try with your patches on
> SM8150 MTP tomorrow since I do not have access to one now.
> Also just to make sure, please remove all the extra SMMU patches you have
> in your tree which are not yet merged or from downstream kernel.
>
FYI, I have a branch [1] integrating patches for upstream. All the
patches up to 34fff8a519cc075933 ("arm64: dts: qcom: add sm8250 GPU
nodes") have been submitted (and the patches after that are unnecessary
for testing on sm8150).
[1] https://github.com/flto/linux/commits/sm8x50-upstream
> -Sai
>
^ permalink raw reply
* Re: [V6, 2/2] media: i2c: dw9768: Add DW9768 VCM driver
From: Tomasz Figa @ 2020-05-25 11:45 UTC (permalink / raw)
To: Dongchun Zhu
Cc: Linus Walleij, Bartosz Golaszewski, Mauro Carvalho Chehab,
Andy Shevchenko, Rob Herring, Mark Rutland, Sakari Ailus,
Nicolas Boichat, Matthias Brugger, Cao Bing Bu, srv_heupstream,
moderated list:ARM/Mediatek SoC support,
list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>,,
Sj Huang, Linux Media Mailing List, linux-devicetree, Louis Kuo,
Shengnan Wang (王圣男)
In-Reply-To: <1590139561.8804.390.camel@mhfsdcap03>
On Fri, May 22, 2020 at 11:27 AM Dongchun Zhu <dongchun.zhu@mediatek.com> wrote:
>
> Hi Tomasz,
>
> Thanks for the review. My replies are as below.
>
> On Thu, 2020-05-21 at 19:51 +0000, Tomasz Figa wrote:
> > Hi Dongchun, Sakari,
> >
> > On Mon, May 18, 2020 at 09:27:31PM +0800, Dongchun Zhu wrote:
[snip]
> > > + pm_runtime_enable(dev);
> > > + if (!pm_runtime_enabled(dev)) {
> > > + ret = dw9768_runtime_resume(dev);
> > > + if (ret < 0) {
> > > + dev_err(dev, "failed to power on: %d\n", ret);
> > > + goto entity_cleanup;
> > > + }
> > > + }
> > > +
> > > + ret = v4l2_async_register_subdev(&dw9768->sd);
> > > + if (ret < 0)
> > > + goto entity_cleanup;
> > > +
> > > + return 0;
> > > +
> > > +entity_cleanup:
> >
> > Need to power off if the code above powered on.
> >
>
> Thanks for the reminder.
> If there is something wrong with runtime PM, actuator is to be powered
> on via dw9768_runtime_resume() API.
> When actuator sub-device is powered on completely and async registered
> successfully, we shall power off it afterwards.
>
The code above calls dw9768_runtime_resume() if
!pm_runtime_enabled(dev), but the clean-up code below the
entity_cleanup label doesn't have the corresponding
dw9768_runtime_suspend() call.
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH v4 3/8] spi: stm32: Add 'SPI_SIMPLEX_RX', 'SPI_3WIRE_RX' support for stm32f4
From: Mark Brown @ 2020-05-25 11:43 UTC (permalink / raw)
To: dillon min
Cc: Rob Herring, p.zabel, Maxime Coquelin,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Alexandre Torgue, thierry.reding, Sam Ravnborg, Dave Airlie,
Daniel Vetter, Michael Turquette, Stephen Boyd, Linux ARM,
linux-kernel, linux-spi, linux-stm32, open list:DRM PANEL DRIVERS,
linux-clk
In-Reply-To: <CAL9mu0+E5R0mDUW3f+aKpfE_457VimS-ow2z_xVOmCfCAMnKuA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 573 bytes --]
On Sat, May 23, 2020 at 09:35:06AM +0800, dillon min wrote:
> - if (ctlr->flags & (SPI_CONTROLLER_MUST_RX | SPI_CONTROLLER_MUST_TX)) {
> + if ((ctlr->flags & (SPI_CONTROLLER_MUST_RX | SPI_CONTROLLER_MUST_TX)) &&
> + !(msg->spi->mode & SPI_3WIRE)) {
> max_tx = 0;
> max_rx = 0;
> for my board, lcd panel ilitek ill9341 use 3wire mode, gyro l3gd20 use
> simplex rx mode.
> it's has benefits to l3gd20, no impact to ili9341.
> if it's fine to spi-core, i will include it to my next submits.
Yes, looks reasonable.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v4 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA
From: Mark Brown @ 2020-05-25 11:41 UTC (permalink / raw)
To: Serge Semin
Cc: Serge Semin, Andy Shevchenko, Vinod Koul, Feng Tang, Grant Likely,
Georgy Vlasov, Ramil Zaripov, Alexey Malahov, Thomas Bogendoerfer,
Paul Burton, Ralf Baechle, Arnd Bergmann, Rob Herring, linux-mips,
devicetree, Wan Ahmad Zainie, Thomas Gleixner, Jarkko Nikula,
wuxu.wu, Clement Leger, Linus Walleij, linux-spi, linux-kernel
In-Reply-To: <20200523083410.3qarkfkwsmodvxwk@mobilestation>
[-- Attachment #1: Type: text/plain, Size: 1784 bytes --]
On Sat, May 23, 2020 at 11:34:10AM +0300, Serge Semin wrote:
> On Fri, May 22, 2020 at 04:22:41PM +0100, Mark Brown wrote:
> > Right, that definitely needs to be fixed then - 8MHz is indeed a totally
> > normal clock rate for SPI so people will hit it. I guess if there's a
> > noticable performance hit to defer to thread then we could implement
> > both and look at how long the delay is going to be to decide which to
> > use, that's annoyingly complicated though so if the overhead is small
> > enough we could just not bother.
> As I suggested before we can implement a solution without performance drop.
> Just wait for the DMA completion locally in the dw_spi_dma_transfer() method and
> return 0 instead of 1 from the transfer_one() callback. In that function we'll
> wait while DMA finishes its business, after that we can check the Tx/Rx FIFO
> emptiness and wait for the data to be completely transferred with delays or
> sleeps or whatever.
No extra context switches there at least, that's the main issue.
> NOTE Currently the DW APB SSI driver doesn't set xfer->effective_speed_hz, though as
> far as I can see that field exists there to be initialized by the SPI controller
> driver, right? If so, strange it isn't done in any SPI drivers...
Yes. Not that many people are concerned about the exact timing it turns
out, the work that was being used for never fully made it upstream.
> What do think about this?
Sure.
> patchset "spi: dw: Add generic DW DMA controller support" (it's being under
> review in this email thread) ? Anyway, if the fixup is getting to be that
> complicated, will it have to be backported to another stable kernels?
No, if it's too invasive it shouldn't be (though the stable people might
decide they want it anyway these days :/ ).
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v15 2/2] i2c: core: support bus regulator controlling in adapter
From: Tomasz Figa @ 2020-05-25 11:40 UTC (permalink / raw)
To: Wolfram Sang, Bibby Hsieh
Cc: Bartosz Golaszewski, linux-i2c, Nicolas Boichat, srv_heupstream,
Rob Herring, Mark Rutland, linux-devicetree, Rafael J . Wysocki
In-Reply-To: <20200522150037.GF5670@ninjato>
On Fri, May 22, 2020 at 5:00 PM Wolfram Sang <wsa@the-dreams.de> wrote:
>
> On Tue, May 19, 2020 at 10:48:33AM +0200, Wolfram Sang wrote:
> > On Tue, May 19, 2020 at 03:27:29PM +0800, Bibby Hsieh wrote:
> > > Although in the most platforms, the bus power of i2c
> > > are alway on, some platforms disable the i2c bus power
> > > in order to meet low power request.
> > >
> > > We get and enable bulk regulator in i2c adapter device.
> > >
> > > Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> >
> > Applied to for-next, thanks! I added Tomasz Rev-by: for you.
>
> Reverted because of regression reports in linux-next. I am all open for
> the change in general, but it looks to me that we should wait another
> cycle.
>
Thanks Wolfram and sorry for the trouble.
Bibby, I think we need a bit more thorough testing. We can discuss the
details offline.
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH 2/6] arm64: dts: qcom: sm8250: add apps_smmu node
From: Sai Prakash Ranjan @ 2020-05-25 11:40 UTC (permalink / raw)
To: Jonathan Marek
Cc: linux-arm-msm, Andy Gross, Bjorn Andersson, Rob Herring,
devicetree, linux-kernel, linux-arm-msm-owner
In-Reply-To: <33b34a58-46d2-80ec-1d79-8e02aa5ae026@marek.ca>
On 2020-05-25 16:57, Jonathan Marek wrote:
> On 5/25/20 7:17 AM, Sai Prakash Ranjan wrote:
>> Hi,
>>
>> On 2020-05-25 16:38, Jonathan Marek wrote:
>>> On 5/25/20 6:54 AM, Sai Prakash Ranjan wrote:
>>>> On 2020-05-25 15:39, Jonathan Marek wrote:
>>>>> Hi,
>>>>>
>>>>> On 5/25/20 5:42 AM, Sai Prakash Ranjan wrote:
>>>>>> Hi Jonathan,
>>>>>>
>>>>>> On 2020-05-24 08:08, Jonathan Marek wrote:
>>>>>>> Add the apps_smmu node for sm8250. Note that adding the iommus
>>>>>>> field for
>>>>>>> UFS is required because initializing the iommu removes the bypass
>>>>>>> mapping
>>>>>>> that created by the bootloader.
>>>>>>>
>>>>>>
>>>>>> This statement doesn't seem right, you can just say since the
>>>>>> bypass is disabled
>>>>>> by default now, we need to add this property to enable translation
>>>>>> and avoid global faults.
>>>>>>
>>>>>
>>>>> If I use this patch [1] then the UFS iommu property isn't needed.
>>>>> In
>>>>> downstream, the identity (bypass?) stream mapping is inherited from
>>>>> the bootloader, and UFS is used without any iommu property. Setting
>>>>> ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n doesn't make it work on its
>>>>> own
>>>>> (without the UFS iommu property), so there's more to it than just
>>>>> "bypass is disabled by default now".
>>>>>
>>>>> https://patchwork.kernel.org/patch/11310757/
>>>>>
>>>>
>>>> "iommus" property is not about inheriting stream mapping from
>>>> bootloader,
>>>> it is used to enable SMMU address translation for the corresponding
>>>> master when specified. So when you have disabled bypass, i.e.,
>>>> ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=y or via cmdline
>>>> "arm-smmu.disable_bypass=1"
>>>> and iommus property with SID and mask is not specified, then it will
>>>> result
>>>> in SMMU global faults.
>>>>
>>>> Downstream has bypass
>>>> enabled(ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n),so you
>>>> won't see any global faults if you do not have iommus property.
>>>>
>>>> Patch in your link is for display because of the usecase for splash
>>>> screen
>>>> on android and some other devices where the bootloader will
>>>> configure SMMU,
>>>> it has not yet merged and not likely to get merged in the current
>>>> state.
>>>>
>>>> So yes "there is *not* much more to it than bypass is disabled by
>>>> default now"
>>>> and you have to specify "iommus" for the master devices or you
>>>> should enable bypass,
>>>> i.e., ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n or
>>>> arm-smmu.disable_bypass=n
>>>>
>>>> Try without the patch in the link and without iommus for UFS and
>>>> ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=y and you will see.
>>>>
>>>> -Sai
>>>
>>> I know that the "iommus" property is not about inheriting stream
>>> mapping. Probing the iommu removes the stream mapping created by the
>>> bootloader, the iommus property is added so that new mappings are
>>> created to replace what was removed.
>>>
>>> You seem to be under the impression that the SM8150/SM8250 bootloader
>>> does not configure SMMU. It does, for both UFS and SDHC, just like it
>>> does for display/splash screen on some devices.
>>>
>>
>> It could be that bootloader does configure SMMU for UFS and SDHC, but
>> the
>> upstream SMMU driver doesnt allow to inherit stream mapping from the
>> bootloader
>> yet, so adding iommus property based on the assumption that it is
>> inherited seems
>> wrong.
>>
>
> I never said adding the iommus property is for inheriting stream
> mapping. I mentioned inheriting to say UFS works without the iommus
> property on downstream (it inherits a identity/bypass mapping).
>
Your commit description says "adding the iommus field for UFS is
required
because initializing the iommu removes the bypass mapping that created
by the
bootloader". So here it would mean like iommus property for UFS is not
for
enabling address translation by SMMU for UFS but to avoid removing
mappings
created by the bootloader which is not exactly what iommus property is
for.
>>> With either value of ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT, it will not
>>> work without the iommus property.
>>
>> I'm pretty sure that if you have ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n
>> and
>> without iommus, it should work.
>>
>
> It doesn't work, with either ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n or
> ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=y.
>
Ok since you are very sure about this, I will try with your patches on
SM8150 MTP tomorrow since I do not have access to one now.
Also just to make sure, please remove all the extra SMMU patches you
have
in your tree which are not yet merged or from downstream kernel.
-Sai
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member
of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply
* Re: [PATCH v15 2/2] i2c: core: support bus regulator controlling in adapter
From: Tomasz Figa @ 2020-05-25 11:34 UTC (permalink / raw)
To: Grygorii Strashko
Cc: Bibby Hsieh, Wolfram Sang, Bartosz Golaszewski, linux-i2c,
Nicolas Boichat, srv_heupstream, Rob Herring, Mark Rutland,
linux-devicetree, Rafael J . Wysocki
In-Reply-To: <952995ec-0865-d8ff-e285-522705fa9709@ti.com>
Hi Grygorii,
On Fri, May 22, 2020 at 7:59 PM Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
>
>
>
> On 19/05/2020 10:27, Bibby Hsieh wrote:
> > Although in the most platforms, the bus power of i2c
> > are alway on, some platforms disable the i2c bus power
> > in order to meet low power request.
> >
> > We get and enable bulk regulator in i2c adapter device.
> >
> > Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> > ---
> > drivers/i2c/i2c-core-base.c | 84 +++++++++++++++++++++++++++++++++++++
> > include/linux/i2c.h | 2 +
> > 2 files changed, 86 insertions(+)
> >
> > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> > index 5cc0b0ec5570..e1cc8d46bc51 100644
> > --- a/drivers/i2c/i2c-core-base.c
> > +++ b/drivers/i2c/i2c-core-base.c
> > @@ -313,12 +313,14 @@ static int i2c_smbus_host_notify_to_irq(const struct i2c_client *client)
> > static int i2c_device_probe(struct device *dev)
> > {
> > struct i2c_client *client = i2c_verify_client(dev);
> > + struct i2c_adapter *adap;
> > struct i2c_driver *driver;
> > int status;
> >
> > if (!client)
> > return 0;
> >
> > + adap = client->adapter;
> > driver = to_i2c_driver(dev->driver);
> >
> > client->irq = client->init_irq;
> > @@ -378,6 +380,12 @@ static int i2c_device_probe(struct device *dev)
> >
> > dev_dbg(dev, "probe\n");
> >
> > + status = regulator_enable(adap->bus_regulator);
> > + if (status < 0) {
> > + dev_err(&adap->dev, "Failed to enable power regulator\n");
> > + goto err_clear_wakeup_irq;
> > + }
> > +
> > status = of_clk_set_defaults(dev->of_node, false);
> > if (status < 0)
> > goto err_clear_wakeup_irq;
> > @@ -414,12 +422,14 @@ static int i2c_device_probe(struct device *dev)
> > static int i2c_device_remove(struct device *dev)
> > {
> > struct i2c_client *client = i2c_verify_client(dev);
> > + struct i2c_adapter *adap;
> > struct i2c_driver *driver;
> > int status = 0;
> >
> > if (!client || !dev->driver)
> > return 0;
> >
> > + adap = client->adapter;
> > driver = to_i2c_driver(dev->driver);
> > if (driver->remove) {
> > dev_dbg(dev, "remove\n");
> > @@ -427,6 +437,8 @@ static int i2c_device_remove(struct device *dev)
> > }
> >
> > dev_pm_domain_detach(&client->dev, true);
> > + if (!pm_runtime_status_suspended(&client->dev))
> > + regulator_disable(adap->bus_regulator);
>
> Not sure this check is correct.
>
> i2c_device_probe()
> - regulator_enable - 1
>
> pm_runtime_get()
> - regulator_enable - 2
>
I believe regulator_enable() wouldn't be called again, because the
device was already active in probe. However, I've been having
difficulties keeping track of runtime PM semantics under various
circumstances (e.g. ACPI vs DT), so can't tell for sure anymore.
> i2c_device_remove()
> - pm_runtime_status_suspended() flase
> - regulator_disable() - 1 --> still active?
>
> Sorry, I probably missing smth.
>
> >
> > dev_pm_clear_wake_irq(&client->dev);
> > device_init_wakeup(&client->dev, false);
> > @@ -438,6 +450,72 @@ static int i2c_device_remove(struct device *dev)
> > return status;
> > }
> >
>
> [...]
>
> --
> Best regards,
> grygorii
^ permalink raw reply
* Re: [PATCH 2/6] arm64: dts: qcom: sm8250: add apps_smmu node
From: Jonathan Marek @ 2020-05-25 11:27 UTC (permalink / raw)
To: Sai Prakash Ranjan
Cc: linux-arm-msm, Andy Gross, Bjorn Andersson, Rob Herring,
devicetree, linux-kernel, linux-arm-msm-owner
In-Reply-To: <72d771390af9a68759d3f81cb79e46a6@codeaurora.org>
On 5/25/20 7:17 AM, Sai Prakash Ranjan wrote:
> Hi,
>
> On 2020-05-25 16:38, Jonathan Marek wrote:
>> On 5/25/20 6:54 AM, Sai Prakash Ranjan wrote:
>>> On 2020-05-25 15:39, Jonathan Marek wrote:
>>>> Hi,
>>>>
>>>> On 5/25/20 5:42 AM, Sai Prakash Ranjan wrote:
>>>>> Hi Jonathan,
>>>>>
>>>>> On 2020-05-24 08:08, Jonathan Marek wrote:
>>>>>> Add the apps_smmu node for sm8250. Note that adding the iommus
>>>>>> field for
>>>>>> UFS is required because initializing the iommu removes the bypass
>>>>>> mapping
>>>>>> that created by the bootloader.
>>>>>>
>>>>>
>>>>> This statement doesn't seem right, you can just say since the
>>>>> bypass is disabled
>>>>> by default now, we need to add this property to enable translation
>>>>> and avoid global faults.
>>>>>
>>>>
>>>> If I use this patch [1] then the UFS iommu property isn't needed. In
>>>> downstream, the identity (bypass?) stream mapping is inherited from
>>>> the bootloader, and UFS is used without any iommu property. Setting
>>>> ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n doesn't make it work on its own
>>>> (without the UFS iommu property), so there's more to it than just
>>>> "bypass is disabled by default now".
>>>>
>>>> https://patchwork.kernel.org/patch/11310757/
>>>>
>>>
>>> "iommus" property is not about inheriting stream mapping from
>>> bootloader,
>>> it is used to enable SMMU address translation for the corresponding
>>> master when specified. So when you have disabled bypass, i.e.,
>>> ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=y or via cmdline
>>> "arm-smmu.disable_bypass=1"
>>> and iommus property with SID and mask is not specified, then it will
>>> result
>>> in SMMU global faults.
>>>
>>> Downstream has bypass
>>> enabled(ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n),so you
>>> won't see any global faults if you do not have iommus property.
>>>
>>> Patch in your link is for display because of the usecase for splash
>>> screen
>>> on android and some other devices where the bootloader will configure
>>> SMMU,
>>> it has not yet merged and not likely to get merged in the current state.
>>>
>>> So yes "there is *not* much more to it than bypass is disabled by
>>> default now"
>>> and you have to specify "iommus" for the master devices or you should
>>> enable bypass,
>>> i.e., ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n or arm-smmu.disable_bypass=n
>>>
>>> Try without the patch in the link and without iommus for UFS and
>>> ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=y and you will see.
>>>
>>> -Sai
>>
>> I know that the "iommus" property is not about inheriting stream
>> mapping. Probing the iommu removes the stream mapping created by the
>> bootloader, the iommus property is added so that new mappings are
>> created to replace what was removed.
>>
>> You seem to be under the impression that the SM8150/SM8250 bootloader
>> does not configure SMMU. It does, for both UFS and SDHC, just like it
>> does for display/splash screen on some devices.
>>
>
> It could be that bootloader does configure SMMU for UFS and SDHC, but the
> upstream SMMU driver doesnt allow to inherit stream mapping from the
> bootloader
> yet, so adding iommus property based on the assumption that it is
> inherited seems
> wrong.
>
I never said adding the iommus property is for inheriting stream
mapping. I mentioned inheriting to say UFS works without the iommus
property on downstream (it inherits a identity/bypass mapping).
>> With either value of ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT, it will not
>> work without the iommus property.
>
> I'm pretty sure that if you have ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n and
> without iommus, it should work.
>
It doesn't work, with either ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n or
ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=y.
> -Sai
>
^ permalink raw reply
* Re: [PATCH 2/6] arm64: dts: qcom: sm8250: add apps_smmu node
From: Sai Prakash Ranjan @ 2020-05-25 11:17 UTC (permalink / raw)
To: Jonathan Marek
Cc: linux-arm-msm, Andy Gross, Bjorn Andersson, Rob Herring,
devicetree, linux-kernel, linux-arm-msm-owner
In-Reply-To: <c9c21e4c-fc89-5a74-fa78-203e5fb64e27@marek.ca>
Hi,
On 2020-05-25 16:38, Jonathan Marek wrote:
> On 5/25/20 6:54 AM, Sai Prakash Ranjan wrote:
>> On 2020-05-25 15:39, Jonathan Marek wrote:
>>> Hi,
>>>
>>> On 5/25/20 5:42 AM, Sai Prakash Ranjan wrote:
>>>> Hi Jonathan,
>>>>
>>>> On 2020-05-24 08:08, Jonathan Marek wrote:
>>>>> Add the apps_smmu node for sm8250. Note that adding the iommus
>>>>> field for
>>>>> UFS is required because initializing the iommu removes the bypass
>>>>> mapping
>>>>> that created by the bootloader.
>>>>>
>>>>
>>>> This statement doesn't seem right, you can just say since the bypass
>>>> is disabled
>>>> by default now, we need to add this property to enable translation
>>>> and avoid global faults.
>>>>
>>>
>>> If I use this patch [1] then the UFS iommu property isn't needed. In
>>> downstream, the identity (bypass?) stream mapping is inherited from
>>> the bootloader, and UFS is used without any iommu property. Setting
>>> ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n doesn't make it work on its own
>>> (without the UFS iommu property), so there's more to it than just
>>> "bypass is disabled by default now".
>>>
>>> https://patchwork.kernel.org/patch/11310757/
>>>
>>
>> "iommus" property is not about inheriting stream mapping from
>> bootloader,
>> it is used to enable SMMU address translation for the corresponding
>> master when specified. So when you have disabled bypass, i.e.,
>> ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=y or via cmdline
>> "arm-smmu.disable_bypass=1"
>> and iommus property with SID and mask is not specified, then it will
>> result
>> in SMMU global faults.
>>
>> Downstream has bypass enabled(ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n),so
>> you
>> won't see any global faults if you do not have iommus property.
>>
>> Patch in your link is for display because of the usecase for splash
>> screen
>> on android and some other devices where the bootloader will configure
>> SMMU,
>> it has not yet merged and not likely to get merged in the current
>> state.
>>
>> So yes "there is *not* much more to it than bypass is disabled by
>> default now"
>> and you have to specify "iommus" for the master devices or you should
>> enable bypass,
>> i.e., ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n or
>> arm-smmu.disable_bypass=n
>>
>> Try without the patch in the link and without iommus for UFS and
>> ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=y and you will see.
>>
>> -Sai
>
> I know that the "iommus" property is not about inheriting stream
> mapping. Probing the iommu removes the stream mapping created by the
> bootloader, the iommus property is added so that new mappings are
> created to replace what was removed.
>
> You seem to be under the impression that the SM8150/SM8250 bootloader
> does not configure SMMU. It does, for both UFS and SDHC, just like it
> does for display/splash screen on some devices.
>
It could be that bootloader does configure SMMU for UFS and SDHC, but
the
upstream SMMU driver doesnt allow to inherit stream mapping from the
bootloader
yet, so adding iommus property based on the assumption that it is
inherited seems
wrong.
> With either value of ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT, it will not
> work without the iommus property.
I'm pretty sure that if you have ARM_SMMU_DISABLE_BYPASS_BY_DEFAULT=n
and
without iommus, it should work.
-Sai
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member
of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply
* Re: [PATCH v11 02/13] dt-bindings: video-interface: Replace 'rotation' description
From: Pavel Machek @ 2020-05-25 11:16 UTC (permalink / raw)
To: Jacopo Mondi
Cc: open list:MEDIA INPUT INFRASTRUCTURE (V4L/DVB), libcamera-devel,
Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus,
Laurent Pinchart, Rob Herring, tfiga, devicetree
In-Reply-To: <20200509090456.3496481-3-jacopo@jmondi.org>
[-- Attachment #1: Type: text/plain, Size: 1896 bytes --]
On Sat 2020-05-09 11:04:45, Jacopo Mondi wrote:
> Replace the 'rotation' property description by providing a definition
> relative to the camera sensor pixel array coordinate system and the
> captured scene.
>
> Acked-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
> .../bindings/media/video-interfaces.txt | 359 +++++++++++++++++-
> 1 file changed, 356 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index b1ff492c7da7a..3920f25a91235 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -85,9 +85,362 @@ Optional properties
>
> - lens-focus: A phandle to the node of the focus lens controller.
>
> -- rotation: The device, typically an image sensor, is not mounted upright,
> - but a number of degrees counter clockwise. Typical values are 0 and 180
> - (upside down).
> +- rotation: The camera rotation is expressed as the angular difference in
> + degrees between two reference systems, one relative to the camera module, and
> + one defined on the external world scene to be captured when projected on the
> + image sensor pixel array.
So.. how is this supposed to work. There's a phone, with its main
camera. It is designed to be used in both portrait and landscape
modes, internal accelerometr can tell between the two.
Plus you have phone with a keyboard. Common usage is portrait with
keyboard closed, and landscape with keyboard open...
And yes, there's linux with v4l2 working on Nokia N900.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox