* Re: [PATCH 04/12] dt-bindings: irqchip: ti,sci-intr: Update bindings to drop the usage of gic as parent
From: Lokesh Vutla @ 2020-05-29 10:14 UTC (permalink / raw)
To: Rob Herring
Cc: Marc Zyngier, Thomas Gleixner, Nishanth Menon, Tero Kristo,
Santosh Shilimkar, Linux ARM Mailing List, Sekhar Nori,
Grygorii Strashko, Peter Ujfalusi, Device Tree Mailing List
In-Reply-To: <20200528221406.GA769073@bogus>
Hi Rob,
On 29/05/20 3:44 am, Rob Herring wrote:
> On Wed, May 20, 2020 at 06:14:46PM +0530, Lokesh Vutla wrote:
>> Drop the firmware related dt-bindings and use the hardware specified
>> interrupt numbers within Interrupt Router. This ensures interrupt router
>> DT node need not assume any interrupt parent type.
>
> I didn't like this binding to begin with, but now you're breaking
> compatibility.
Yes, I do agree that this change is breaking backward compatibility. But IMHO,
this does cleanup of firmware specific properties from DT. Since this is not
deployed out yet in the wild market, I took the leverage of breaking backward
compatibility. Before accepting these changes from firmware team, I did
discuss[0] with Marc on this topic.
>
>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>> ---
>> .../interrupt-controller/ti,sci-intr.txt | 31 ++++++++++---------
>> 1 file changed, 16 insertions(+), 15 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt b/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
>> index 1a8718f8855d..8b56b2de1c73 100644
>> --- a/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
>> @@ -44,15 +44,17 @@ Required Properties:
>> 4: If intr supports level triggered interrupts.
>> - interrupt-controller: Identifies the node as an interrupt controller
>> - #interrupt-cells: Specifies the number of cells needed to encode an
>> - interrupt source. The value should be 2.
>> - First cell should contain the TISCI device ID of source
>> - Second cell should contain the interrupt source offset
>> - within the device.
>> + interrupt source. The value should be 1.
>> + First cell should contain interrupt router input number
>> + as specified by hardware.
>> - ti,sci: Phandle to TI-SCI compatible System controller node.
>> -- ti,sci-dst-id: TISCI device ID of the destination IRQ controller.
>> -- ti,sci-rm-range-girq: Array of TISCI subtype ids representing the host irqs
>> - assigned to this interrupt router. Each subtype id
>> - corresponds to a range of host irqs.
>> +- ti,sci-dev-id: TISCI device id of interrupt controller.
>> +- ti,interrupt-ranges: Set of triplets containing ranges that convert
>> + the INTR output interrupt numbers to parent's
>> + interrupt number. Each triplet has following entries:
>> + - First entry specifies the base for intr output irq
>> + - Second entry specifies the base for parent irqs
>> + - Third entry specifies the limit
>
> Humm, sounds like what interrupt-map does.
Okay, Ill look at it.
[0]
https://lore.kernel.org/linux-arm-kernel/2331f04eacee3b06cc152fc609225233@www.loen.fr/
Thanks and regards,
Lokesh
^ permalink raw reply
* Re: [PATCH 04/12] dt-bindings: irqchip: ti,sci-intr: Update bindings to drop the usage of gic as parent
From: Marc Zyngier @ 2020-05-29 10:18 UTC (permalink / raw)
To: Lokesh Vutla
Cc: Rob Herring, Thomas Gleixner, Nishanth Menon, Tero Kristo,
Santosh Shilimkar, Linux ARM Mailing List, Sekhar Nori,
Grygorii Strashko, Peter Ujfalusi, Device Tree Mailing List
In-Reply-To: <f803f646-2a55-4f15-9682-1dc616d7c714@ti.com>
On 2020-05-29 11:14, Lokesh Vutla wrote:
> Hi Rob,
>
> On 29/05/20 3:44 am, Rob Herring wrote:
>> On Wed, May 20, 2020 at 06:14:46PM +0530, Lokesh Vutla wrote:
>>> Drop the firmware related dt-bindings and use the hardware specified
>>> interrupt numbers within Interrupt Router. This ensures interrupt
>>> router
>>> DT node need not assume any interrupt parent type.
>>
>> I didn't like this binding to begin with, but now you're breaking
>> compatibility.
>
> Yes, I do agree that this change is breaking backward compatibility.
> But IMHO,
> this does cleanup of firmware specific properties from DT. Since this
> is not
> deployed out yet in the wild market, I took the leverage of breaking
> backward
> compatibility. Before accepting these changes from firmware team, I did
> discuss[0] with Marc on this topic.
And I assume that should anyone complain about the kernel being broken
because they have an old firmware, you'll be OK with the patches being
reverted, right?
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* Re: [PATCH v5 05/16] spi: dw: Add SPI Rx-done wait method to DMA-based transfer
From: Andy Shevchenko @ 2020-05-29 10:20 UTC (permalink / raw)
To: Serge Semin
Cc: Serge Semin, Mark Brown, Grant Likely, Linus Walleij, Feng Tang,
Alan Cox, Vinod Koul, Georgy Vlasov, Ramil Zaripov,
Alexey Malahov, Thomas Bogendoerfer, Arnd Bergmann, Rob Herring,
linux-mips, devicetree, linux-spi, linux-kernel
In-Reply-To: <20200529101328.bfoyyvmwm5gfflxv@mobilestation>
On Fri, May 29, 2020 at 01:13:28PM +0300, Serge Semin wrote:
> On Fri, May 29, 2020 at 12:46:48PM +0300, Andy Shevchenko wrote:
> > On Fri, May 29, 2020 at 06:59:03AM +0300, Serge Semin wrote:
> > > Having any data left in the Rx FIFO after the DMA engine claimed it has
> > > finished all DMA transactions is an abnormal situation, since the DW SPI
> > > controller driver expects to have all the data being fetched and placed
> > > into the SPI Rx buffer at that moment. In case if this has happened we
> > > assume that DMA engine still may be doing the data fetching, thus we give
> > > it sometime to finish. If after a short period of time the data is still
> > > left in the Rx FIFO, the driver will give up waiting and return an error
> > > indicating that the SPI controller/DMA engine must have hung up or failed
> > > at some point of doing their duties.
> >
> > ...
> >
> > > +static int dw_spi_dma_wait_rx_done(struct dw_spi *dws)
> > > +{
> > > + int retry = WAIT_RETRIES;
> > > + struct spi_delay delay;
> > > + unsigned long ns, us;
> > > + u32 nents;
> > > +
> > > + /*
> > > + * It's unlikely that DMA engine is still doing the data fetching, but
> > > + * if it's let's give it some reasonable time. The timeout calculation
> > > + * is based on the synchronous APB/SSI reference clock rate, on a
> > > + * number of data entries left in the Rx FIFO, times a number of clock
> > > + * periods normally needed for a single APB read/write transaction
> > > + * without PREADY signal utilized (which is true for the DW APB SSI
> > > + * controller).
> > > + */
> > > + nents = dw_readl(dws, DW_SPI_RXFLR);
> >
>
> > > + ns = NSEC_PER_SEC / dws->max_freq * 4 * nents;
> >
> > I think we may slightly increase precision by writing this like
> >
> > ns = 4 * NSEC_PER_SEC / dws->max_freq * nents;
>
> Good point. Although both 4 and NSEC_PER_SEC are signed. The later is
> 1000000000L. Formally speaking on x32 systems (4 * 1000 000 000L) equals
> to a negative value. Though overflow still won't happen so the result will
> be correct. Anyway to be on a safe side it would be better to use an explicit
> unsigned literal:
>
> + ns = 4U * NSEC_PER_SEC / dws->max_freq * nents;
Yes, right.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 09/11] dmaengine: dw: Initialize min_burst capability
From: Andy Shevchenko @ 2020-05-29 10:25 UTC (permalink / raw)
To: Serge Semin
Cc: Vinod Koul, Viresh Kumar, Dan Williams, Serge Semin,
Alexey Malahov, Thomas Bogendoerfer, Arnd Bergmann, Rob Herring,
linux-mips, devicetree, dmaengine, linux-kernel
In-Reply-To: <20200528222401.26941-10-Sergey.Semin@baikalelectronics.ru>
On Fri, May 29, 2020 at 01:23:59AM +0300, Serge Semin wrote:
> According to the DW APB DMAC data book the minimum burst transaction
> length is 1 and it's true for any version of the controller since
> isn't parametrised in the coreAssembler so can't be changed at the
> IP-core synthesis stage. Let's initialise the min_burst member of the
> DMA controller descriptor so the DMA clients could use it to properly
> optimize the DMA requests.
> @@ -1229,6 +1229,7 @@ int do_dma_probe(struct dw_dma_chip *chip)
> dw->dma.device_issue_pending = dwc_issue_pending;
>
> /* DMA capabilities */
> + dw->dma.min_burst = 1;
Perhaps then relaxed maximum, like
dw->dma.max_burst = 256;
(channels will update this)
?
> dw->dma.src_addr_widths = DW_DMA_BUSWIDTHS;
> dw->dma.dst_addr_widths = DW_DMA_BUSWIDTHS;
> dw->dma.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV) |
> --
> 2.26.2
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 10/11] dmaengine: dw: Introduce max burst length hw config
From: Andy Shevchenko @ 2020-05-29 10:27 UTC (permalink / raw)
To: Serge Semin
Cc: Vinod Koul, Viresh Kumar, Dan Williams, Serge Semin,
Alexey Malahov, Thomas Bogendoerfer, Arnd Bergmann, Rob Herring,
linux-mips, devicetree, dmaengine, linux-kernel
In-Reply-To: <20200528222401.26941-11-Sergey.Semin@baikalelectronics.ru>
On Fri, May 29, 2020 at 01:24:00AM +0300, Serge Semin wrote:
> IP core of the DW DMA controller may be synthesized with different
> max burst length of the transfers per each channel. According to Synopsis
> having the fixed maximum burst transactions length may provide some
> performance gain. At the same time setting up the source and destination
> multi size exceeding the max burst length limitation may cause a serious
> problems. In our case the DMA transaction just hangs up. In order to fix
> this lets introduce the max burst length platform config of the DW DMA
> controller device and don't let the DMA channels configuration code
> exceed the burst length hardware limitation.
>
> Note the maximum burst length parameter can be detected either in runtime
> from the DWC parameter registers or from the dedicated DT property.
> Depending on the IP core configuration the maximum value can vary from
> channel to channel so by overriding the channel slave max_burst capability
> we make sure a DMA consumer will get the channel-specific max burst
> length.
LGTM, but consider comment to previous patch (in that case perhaps definition
of min and max should be moved there).
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: linux-mips@vger.kernel.org
> Cc: devicetree@vger.kernel.org
>
> ---
>
> Changelog v2:
> - Rearrange SoBs.
> - Discard dwc_get_maxburst() accessor. It's enough to have a clamping
> guard against exceeding the hardware max burst limitation.
>
> Changelog v3:
> - Override the slave channel max_burst capability instead of calculating
> the minimum value of max burst lengths and setting the DMA-device
> generic capability.
>
> Changelog v4:
> - Clamp the dst and src burst lengths in the generic dwc_config() method
> instead of doing that in the encode_maxburst() callback.
> - Define max_burst with u32 type in struct dw_dma_platform_data.
> - Perform of_property_read_u32_array() directly into the platform data
> max_burst member.
> ---
> drivers/dma/dw/core.c | 10 ++++++++++
> drivers/dma/dw/of.c | 5 +++++
> drivers/dma/dw/regs.h | 2 ++
> include/linux/platform_data/dma-dw.h | 4 ++++
> 4 files changed, 21 insertions(+)
>
> diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
> index a8cebb1dbb68..60ef779fc5e0 100644
> --- a/drivers/dma/dw/core.c
> +++ b/drivers/dma/dw/core.c
> @@ -791,6 +791,11 @@ static int dwc_config(struct dma_chan *chan, struct dma_slave_config *sconfig)
>
> memcpy(&dwc->dma_sconfig, sconfig, sizeof(*sconfig));
>
> + dwc->dma_sconfig.src_maxburst =
> + clamp(dwc->dma_sconfig.src_maxburst, 0U, dwc->max_burst);
> + dwc->dma_sconfig.dst_maxburst =
> + clamp(dwc->dma_sconfig.dst_maxburst, 0U, dwc->max_burst);
> +
> dw->encode_maxburst(dwc, &dwc->dma_sconfig.src_maxburst);
> dw->encode_maxburst(dwc, &dwc->dma_sconfig.dst_maxburst);
>
> @@ -1051,7 +1056,9 @@ static void dwc_free_chan_resources(struct dma_chan *chan)
>
> static void dwc_caps(struct dma_chan *chan, struct dma_slave_caps *caps)
> {
> + struct dw_dma_chan *dwc = to_dw_dma_chan(chan);
>
> + caps->max_burst = dwc->max_burst;
> }
>
> int do_dma_probe(struct dw_dma_chip *chip)
> @@ -1194,9 +1201,12 @@ int do_dma_probe(struct dw_dma_chip *chip)
> dwc->nollp =
> (dwc_params >> DWC_PARAMS_MBLK_EN & 0x1) == 0 ||
> (dwc_params >> DWC_PARAMS_HC_LLP & 0x1) == 1;
> + dwc->max_burst =
> + (0x4 << (dwc_params >> DWC_PARAMS_MSIZE & 0x7));
> } else {
> dwc->block_size = pdata->block_size;
> dwc->nollp = !pdata->multi_block[i];
> + dwc->max_burst = pdata->max_burst[i] ?: DW_DMA_MAX_BURST;
> }
> }
>
> diff --git a/drivers/dma/dw/of.c b/drivers/dma/dw/of.c
> index 9e27831dee32..1474b3817ef4 100644
> --- a/drivers/dma/dw/of.c
> +++ b/drivers/dma/dw/of.c
> @@ -98,6 +98,11 @@ struct dw_dma_platform_data *dw_dma_parse_dt(struct platform_device *pdev)
> pdata->multi_block[tmp] = 1;
> }
>
> + if (of_property_read_u32_array(np, "snps,max-burst-len", pdata->max_burst,
> + nr_channels)) {
> + memset32(pdata->max_burst, DW_DMA_MAX_BURST, nr_channels);
> + }
> +
> if (!of_property_read_u32(np, "snps,dma-protection-control", &tmp)) {
> if (tmp > CHAN_PROTCTL_MASK)
> return NULL;
> diff --git a/drivers/dma/dw/regs.h b/drivers/dma/dw/regs.h
> index 1ab840b06e79..76654bd13c1a 100644
> --- a/drivers/dma/dw/regs.h
> +++ b/drivers/dma/dw/regs.h
> @@ -126,6 +126,7 @@ struct dw_dma_regs {
> /* Bitfields in DWC_PARAMS */
> #define DWC_PARAMS_MBLK_EN 11 /* multi block transfer */
> #define DWC_PARAMS_HC_LLP 13 /* set LLP register to zero */
> +#define DWC_PARAMS_MSIZE 16 /* max group transaction size */
>
> /* bursts size */
> enum dw_dma_msize {
> @@ -284,6 +285,7 @@ struct dw_dma_chan {
> /* hardware configuration */
> unsigned int block_size;
> bool nollp;
> + u32 max_burst;
>
> /* custom slave configuration */
> struct dw_dma_slave dws;
> diff --git a/include/linux/platform_data/dma-dw.h b/include/linux/platform_data/dma-dw.h
> index f3eaf9ec00a1..29c484da2979 100644
> --- a/include/linux/platform_data/dma-dw.h
> +++ b/include/linux/platform_data/dma-dw.h
> @@ -12,6 +12,7 @@
>
> #define DW_DMA_MAX_NR_MASTERS 4
> #define DW_DMA_MAX_NR_CHANNELS 8
> +#define DW_DMA_MAX_BURST 256
>
> /**
> * struct dw_dma_slave - Controller-specific information about a slave
> @@ -42,6 +43,8 @@ struct dw_dma_slave {
> * @data_width: Maximum data width supported by hardware per AHB master
> * (in bytes, power of 2)
> * @multi_block: Multi block transfers supported by hardware per channel.
> + * @max_burst: Maximum value of burst transaction size supported by hardware
> + * per channel (in units of CTL.SRC_TR_WIDTH/CTL.DST_TR_WIDTH).
> * @protctl: Protection control signals setting per channel.
> */
> struct dw_dma_platform_data {
> @@ -56,6 +59,7 @@ struct dw_dma_platform_data {
> unsigned char nr_masters;
> unsigned char data_width[DW_DMA_MAX_NR_MASTERS];
> unsigned char multi_block[DW_DMA_MAX_NR_CHANNELS];
> + u32 max_burst[DW_DMA_MAX_NR_CHANNELS];
> #define CHAN_PROTCTL_PRIVILEGED BIT(0)
> #define CHAN_PROTCTL_BUFFERABLE BIT(1)
> #define CHAN_PROTCTL_CACHEABLE BIT(2)
> --
> 2.26.2
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 11/11] dmaengine: dw: Initialize max_sg_nents capability
From: Andy Shevchenko @ 2020-05-29 10:27 UTC (permalink / raw)
To: Serge Semin
Cc: Vinod Koul, Viresh Kumar, Dan Williams, Serge Semin,
Alexey Malahov, Thomas Bogendoerfer, Arnd Bergmann, Rob Herring,
linux-mips, devicetree, dmaengine, linux-kernel
In-Reply-To: <20200528222401.26941-12-Sergey.Semin@baikalelectronics.ru>
On Fri, May 29, 2020 at 01:24:01AM +0300, Serge Semin wrote:
> Multi-block support provides a way to map the kernel-specific SG-table so
> the DW DMA device would handle it as a whole instead of handling the
> SG-list items or so called LLP block items one by one. So if true LLP
> list isn't supported by the DW DMA engine, then soft-LLP mode will be
> utilized to load and execute each LLP-block one by one. The soft-LLP mode
> of the DMA transactions execution might not work well for some DMA
> consumers like SPI due to its Tx and Rx buffers inter-dependency. Let's
> initialize the max_sg_nents DMA channels capability based on the nollp
> flag state. If it's true, no hardware accelerated LLP is available and
> max_sg_nents should be set with 1, which means that the DMA engine
> can handle only a single SG list entry at a time. If noLLP is set to
> false, then hardware accelerated LLP is supported and the DMA engine
> can handle infinite number of SG entries in a single DMA transaction.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: linux-mips@vger.kernel.org
> Cc: devicetree@vger.kernel.org
>
> ---
>
> Changelog v3:
> - This is a new patch created as a result of the discussion with Vinud and
> Andy in the framework of DW DMA burst and LLP capabilities.
>
> Changelog v4:
> - Use explicit if-else statement when assigning the max_sg_nents field.
> ---
> drivers/dma/dw/core.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
> index 60ef779fc5e0..b76eee75fde8 100644
> --- a/drivers/dma/dw/core.c
> +++ b/drivers/dma/dw/core.c
> @@ -1059,6 +1059,18 @@ static void dwc_caps(struct dma_chan *chan, struct dma_slave_caps *caps)
> struct dw_dma_chan *dwc = to_dw_dma_chan(chan);
>
> caps->max_burst = dwc->max_burst;
> +
> + /*
> + * It might be crucial for some devices to have the hardware
> + * accelerated multi-block transfers supported, aka LLPs in DW DMAC
> + * notation. So if LLPs are supported then max_sg_nents is set to
> + * zero which means unlimited number of SG entries can be handled in a
> + * single DMA transaction, otherwise it's just one SG entry.
> + */
> + if (dwc->nollp)
> + caps->max_sg_nents = 1;
> + else
> + caps->max_sg_nents = 0;
> }
>
> int do_dma_probe(struct dw_dma_chip *chip)
> --
> 2.26.2
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 09/11] dmaengine: dw: Initialize min_burst capability
From: Andy Shevchenko @ 2020-05-29 10:29 UTC (permalink / raw)
To: Serge Semin
Cc: Vinod Koul, Viresh Kumar, Dan Williams, Serge Semin,
Alexey Malahov, Thomas Bogendoerfer, Arnd Bergmann, Rob Herring,
linux-mips, devicetree, dmaengine, linux-kernel
In-Reply-To: <20200529102515.GD1634618@smile.fi.intel.com>
On Fri, May 29, 2020 at 01:25:15PM +0300, Andy Shevchenko wrote:
> On Fri, May 29, 2020 at 01:23:59AM +0300, Serge Semin wrote:
> > According to the DW APB DMAC data book the minimum burst transaction
> > length is 1 and it's true for any version of the controller since
> > isn't parametrised in the coreAssembler so can't be changed at the
> > IP-core synthesis stage. Let's initialise the min_burst member of the
> > DMA controller descriptor so the DMA clients could use it to properly
> > optimize the DMA requests.
...
> > /* DMA capabilities */
>
> > + dw->dma.min_burst = 1;
>
> Perhaps then relaxed maximum, like
>
> dw->dma.max_burst = 256;
>
> (channels will update this)
>
> ?
And forgot to mention that perhaps we need a definitions for both.
> > dw->dma.src_addr_widths = DW_DMA_BUSWIDTHS;
> > dw->dma.dst_addr_widths = DW_DMA_BUSWIDTHS;
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 09/11] dmaengine: dw: Initialize min_burst capability
From: Serge Semin @ 2020-05-29 10:41 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Serge Semin, Vinod Koul, Viresh Kumar, Dan Williams,
Alexey Malahov, Thomas Bogendoerfer, Arnd Bergmann, Rob Herring,
linux-mips, devicetree, dmaengine, linux-kernel
In-Reply-To: <20200529102902.GG1634618@smile.fi.intel.com>
On Fri, May 29, 2020 at 01:29:02PM +0300, Andy Shevchenko wrote:
> On Fri, May 29, 2020 at 01:25:15PM +0300, Andy Shevchenko wrote:
> > On Fri, May 29, 2020 at 01:23:59AM +0300, Serge Semin wrote:
> > > According to the DW APB DMAC data book the minimum burst transaction
> > > length is 1 and it's true for any version of the controller since
> > > isn't parametrised in the coreAssembler so can't be changed at the
> > > IP-core synthesis stage. Let's initialise the min_burst member of the
> > > DMA controller descriptor so the DMA clients could use it to properly
> > > optimize the DMA requests.
>
> ...
>
> > > /* DMA capabilities */
> >
> > > + dw->dma.min_burst = 1;
> >
> > Perhaps then relaxed maximum, like
> >
> > dw->dma.max_burst = 256;
> >
> > (channels will update this)
> >
> > ?
>
> And forgot to mention that perhaps we need a definitions for both.
By "definitions for both" do you mean a macro with corresponding parameter
definition like it's done for the max burst length in the next patch?
Something like this:
--- include/linux/platform_data/dma-dw.h
+++ include/linux/platform_data/dma-dw.h
+#define DW_DMA_MIN_BURST 1
+#define DW_DMA_MAX_BURST 256
?
-Sergey
>
> > > dw->dma.src_addr_widths = DW_DMA_BUSWIDTHS;
> > > dw->dma.dst_addr_widths = DW_DMA_BUSWIDTHS;
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
^ permalink raw reply
* Re: [PATCH v4 09/11] dmaengine: dw: Initialize min_burst capability
From: Andy Shevchenko @ 2020-05-29 10:50 UTC (permalink / raw)
To: Serge Semin
Cc: Serge Semin, Vinod Koul, Viresh Kumar, Dan Williams,
Alexey Malahov, Thomas Bogendoerfer, Arnd Bergmann, Rob Herring,
linux-mips, devicetree, dmaengine, linux-kernel
In-Reply-To: <20200529104119.qrqoptp5iz5hs56r@mobilestation>
On Fri, May 29, 2020 at 01:41:19PM +0300, Serge Semin wrote:
> On Fri, May 29, 2020 at 01:29:02PM +0300, Andy Shevchenko wrote:
> > On Fri, May 29, 2020 at 01:25:15PM +0300, Andy Shevchenko wrote:
> > > On Fri, May 29, 2020 at 01:23:59AM +0300, Serge Semin wrote:
...
> > > > /* DMA capabilities */
> > > > + dw->dma.min_burst = 1;
> > >
> > > Perhaps then relaxed maximum, like
> > >
> > > dw->dma.max_burst = 256;
> > >
> > > (channels will update this)
> > >
> > > ?
>
> > And forgot to mention that perhaps we need a definitions for both.
>
> By "definitions for both" do you mean a macro with corresponding parameter
> definition like it's done for the max burst length in the next patch?
> Something like this:
> --- include/linux/platform_data/dma-dw.h
> +++ include/linux/platform_data/dma-dw.h
> +#define DW_DMA_MIN_BURST 1
> +#define DW_DMA_MAX_BURST 256
>
> ?
Yes!
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v3 1/5] regulator: Allow regulators to verify enabled during enable()
From: Mark Brown @ 2020-05-29 10:50 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Sumit Semwal, agross, lgirdwood, robh+dt, nishakumari,
linux-arm-msm, linux-kernel, devicetree, kgunda, rnayak
In-Reply-To: <20200529013743.GL279327@builder.lan>
[-- Attachment #1: Type: text/plain, Size: 965 bytes --]
On Thu, May 28, 2020 at 06:37:43PM -0700, Bjorn Andersson wrote:
> On Thu 28 May 08:46 PDT 2020, Sumit Semwal wrote:
> > Some regulators might need to verify that they have indeed been enabled
> > after the enable() call is made and enable_time delay has passed.
> > _regulator_enable_delay(delay);
> My interpretation of "enable_time" (i.e. the value of delay) is that it
> denotes the maximum time it will take for the regulator to turn on, and
Right.
> the purpose of this patch is to be able to handle cases where we can
> poll the hardware to see if it completed earlier.
Is that it? From the changelog it sounded like this was a workaround
for broken hardware not an attempt at optimization.
> So I think you should flip the meaning of your two variables around,
> making "delay" the total time to sleep and the newly introduced
> "poll_enabled_time" the interval at which you check if the hardware
> finished early.
Yes.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v4 09/11] dmaengine: dw: Initialize min_burst capability
From: Serge Semin @ 2020-05-29 10:51 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Serge Semin, Vinod Koul, Viresh Kumar, Dan Williams,
Alexey Malahov, Thomas Bogendoerfer, Arnd Bergmann, Rob Herring,
linux-mips, devicetree, dmaengine, linux-kernel
In-Reply-To: <20200529105009.GH1634618@smile.fi.intel.com>
On Fri, May 29, 2020 at 01:50:09PM +0300, Andy Shevchenko wrote:
> On Fri, May 29, 2020 at 01:41:19PM +0300, Serge Semin wrote:
> > On Fri, May 29, 2020 at 01:29:02PM +0300, Andy Shevchenko wrote:
> > > On Fri, May 29, 2020 at 01:25:15PM +0300, Andy Shevchenko wrote:
> > > > On Fri, May 29, 2020 at 01:23:59AM +0300, Serge Semin wrote:
>
> ...
>
> > > > > /* DMA capabilities */
> > > > > + dw->dma.min_burst = 1;
> > > >
> > > > Perhaps then relaxed maximum, like
> > > >
> > > > dw->dma.max_burst = 256;
> > > >
> > > > (channels will update this)
> > > >
> > > > ?
> >
> > > And forgot to mention that perhaps we need a definitions for both.
> >
> > By "definitions for both" do you mean a macro with corresponding parameter
> > definition like it's done for the max burst length in the next patch?
> > Something like this:
> > --- include/linux/platform_data/dma-dw.h
> > +++ include/linux/platform_data/dma-dw.h
> > +#define DW_DMA_MIN_BURST 1
> > +#define DW_DMA_MAX_BURST 256
> >
> > ?
>
> Yes!
Ok. Good idea. I'll do that. Thanks.
-Sergey
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
^ permalink raw reply
* Re: [PATCH] dt-bindings: sound: tlv320adcx140: Fix dt-binding-check issue
From: Mark Brown @ 2020-05-29 10:51 UTC (permalink / raw)
To: Dan Murphy
Cc: lgirdwood, perex, tiwai, robh, alsa-devel, linux-kernel,
devicetree
In-Reply-To: <20200528144711.18065-1-dmurphy@ti.com>
[-- Attachment #1: Type: text/plain, Size: 421 bytes --]
On Thu, May 28, 2020 at 09:47:11AM -0500, Dan Murphy wrote:
> Fix dt-binding-check issue
Please submit patches using subject lines reflecting the style for the
subsystem, this makes it easier for people to identify relevant patches.
Look at what existing commits in the area you're changing are doing and
make sure your subject lines visually resemble what they're doing.
There's no need to resubmit to fix this alone.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v3 1/5] regulator: Allow regulators to verify enabled during enable()
From: Mark Brown @ 2020-05-29 10:54 UTC (permalink / raw)
To: Sumit Semwal
Cc: agross, bjorn.andersson, lgirdwood, robh+dt, nishakumari,
linux-arm-msm, linux-kernel, devicetree, kgunda, rnayak
In-Reply-To: <20200528154625.17742-2-sumit.semwal@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 363 bytes --]
On Thu, May 28, 2020 at 09:16:21PM +0530, Sumit Semwal wrote:
> + while (time_remaining > 0) {
> + /* We've already waited for enable_time above;
> + * so we can start with immediate check of the
> + * status of the regulator.
> + */
> + if (rdev->desc->ops->is_enabled(rdev))
> + break;
I'd expect to prefer to get_status() if it's available.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/9] dt-bindings: mmc: Convert sdhci-pxa to json-schema
From: Ulf Hansson @ 2020-05-29 11:10 UTC (permalink / raw)
To: Rob Herring, Lubomir Rintel
Cc: Alessandro Zummo, Alexandre Belloni, Bartosz Golaszewski,
Daniel Lezcano, Jason Cooper, Linus Walleij, Marc Zyngier,
Thomas Gleixner, DTML, Linux Kernel Mailing List
In-Reply-To: <20200528225445.GB815881@bogus>
On Fri, 29 May 2020 at 00:54, Rob Herring <robh@kernel.org> wrote:
>
> On Wed, May 27, 2020 at 09:59:10AM +0200, Ulf Hansson wrote:
> > On Thu, 21 May 2020 at 11:14, Lubomir Rintel <lkundrak@v3.sk> wrote:
> > >
> > > Convert the sdhci-pxa binding to DT schema format using json-schema.
> > >
> > > At the same time, fix a couple of issues with the examples discovered by
> > > the validation tool -- a semicolon instead of a comma and wrong node names.
> > >
> > > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> >
> > Rob, are you fine with this v2? I am intending to queue it up via my
> > mmc tree, unless you want to pick it?
>
> You can take it if you drop my name from 'maintainers'. Ideally, it
> shouldn't be your name either (should have called it 'owners'
> instead...).
Patch amended!
>
> Reviewed-by: Rob Herring <robh@kernel.org>
Thanks, applied to my next branch!
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH 04/12] dt-bindings: irqchip: ti,sci-intr: Update bindings to drop the usage of gic as parent
From: Lokesh Vutla @ 2020-05-29 11:13 UTC (permalink / raw)
To: Rob Herring
Cc: Marc Zyngier, Thomas Gleixner, Nishanth Menon, Tero Kristo,
Santosh Shilimkar, Linux ARM Mailing List, Sekhar Nori,
Grygorii Strashko, Peter Ujfalusi, Device Tree Mailing List
In-Reply-To: <20200528221406.GA769073@bogus>
Hi Rob,
[..snip..]
>>
>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt b/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
>> index 1a8718f8855d..8b56b2de1c73 100644
>> --- a/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
>> @@ -44,15 +44,17 @@ Required Properties:
>> 4: If intr supports level triggered interrupts.
>> - interrupt-controller: Identifies the node as an interrupt controller
>> - #interrupt-cells: Specifies the number of cells needed to encode an
>> - interrupt source. The value should be 2.
>> - First cell should contain the TISCI device ID of source
>> - Second cell should contain the interrupt source offset
>> - within the device.
>> + interrupt source. The value should be 1.
>> + First cell should contain interrupt router input number
>> + as specified by hardware.
>> - ti,sci: Phandle to TI-SCI compatible System controller node.
>> -- ti,sci-dst-id: TISCI device ID of the destination IRQ controller.
>> -- ti,sci-rm-range-girq: Array of TISCI subtype ids representing the host irqs
>> - assigned to this interrupt router. Each subtype id
>> - corresponds to a range of host irqs.
>> +- ti,sci-dev-id: TISCI device id of interrupt controller.
>> +- ti,interrupt-ranges: Set of triplets containing ranges that convert
>> + the INTR output interrupt numbers to parent's
>> + interrupt number. Each triplet has following entries:
>> + - First entry specifies the base for intr output irq
>> + - Second entry specifies the base for parent irqs
>> + - Third entry specifies the limit
>
> Humm, sounds like what interrupt-map does.
IIUC, for every irq translation there should be an entry in interrupt-map
property. These Controllers has interrupts ranging from 32, 256 and more. It
might be odd to have 256 entries in the interrupt map no? Also there are
multiple interrupt controllers which need such translations.
Thanks and regards,
Lokesh
^ permalink raw reply
* Re: [PATCH v8 04/10] OPP: Add support for parsing interconnect bandwidth
From: Sibi Sankar @ 2020-05-29 11:39 UTC (permalink / raw)
To: Viresh Kumar
Cc: Georgi Djakov, vireshk, nm, sboyd, rjw, saravanak, mka, robh+dt,
rnayak, bjorn.andersson, vincent.guittot, jcrouse, evgreen,
linux-pm, devicetree, linux-kernel
In-Reply-To: <20200529044437.5wmbbews2vn66dia@vireshk-i7>
On 2020-05-29 10:14, Viresh Kumar wrote:
> On 12-05-20, 15:53, Georgi Djakov wrote:
>> struct dev_pm_opp *_opp_allocate(struct opp_table *table)
>> {
>> struct dev_pm_opp *opp;
>> - int count, supply_size;
>> + int supply_count, supply_size, icc_size;
>>
>> /* Allocate space for at least one supply */
>> - count = table->regulator_count > 0 ? table->regulator_count : 1;
>> - supply_size = sizeof(*opp->supplies) * count;
>> + supply_count = table->regulator_count > 0 ? table->regulator_count :
>> 1;
>> + supply_size = sizeof(*opp->supplies) * supply_count;
>> + icc_size = sizeof(*opp->bandwidth) * table->path_count;
>>
>> /* allocate new OPP node and supplies structures */
>> - opp = kzalloc(sizeof(*opp) + supply_size, GFP_KERNEL);
>> + opp = kzalloc(sizeof(*opp) + supply_size + icc_size, GFP_KERNEL);
>> +
>> if (!opp)
>> return NULL;
>>
>> /* Put the supplies at the end of the OPP structure as an empty
>> array */
>> opp->supplies = (struct dev_pm_opp_supply *)(opp + 1);
>> + opp->bandwidth = (struct dev_pm_opp_icc_bw *)(opp->supplies +
>> supply_count);
>> INIT_LIST_HEAD(&opp->node);
>
> Added this delta here.
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 7302f2631f8d..dfbd3d10410c 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1330,7 +1330,8 @@ struct dev_pm_opp *_opp_allocate(struct opp_table
> *table)
>
> /* Put the supplies at the end of the OPP structure as an empty
> array */
> opp->supplies = (struct dev_pm_opp_supply *)(opp + 1);
> - opp->bandwidth = (struct dev_pm_opp_icc_bw *)(opp->supplies +
> supply_count);
> + if (icc_size)
> + opp->bandwidth = (struct dev_pm_opp_icc_bw
> *)(opp->supplies + supply_count);
nice catch!
Reviewed-by: Sibi Sankar <sibis@codeaurora.org>
> INIT_LIST_HEAD(&opp->node);
>
> return opp;
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.
^ permalink raw reply
* Re: Fwd: [PATCH v3 2/2] media: cec: i2c: ch7322: Add ch7322 CEC controller driver
From: Hans Verkuil @ 2020-05-29 12:01 UTC (permalink / raw)
To: Jeff Chase, linux-media; +Cc: mchehab, robh+dt, devicetree
In-Reply-To: <CALTkaQ0NLgjS7H7De=7jy9jRG1xMFSbzdmxrFNerNU+o1rRzpg@mail.gmail.com>
On 29/05/2020 08:10, Jeff Chase wrote:
> (Resending as plain text, sorry)
>
>> +static int ch7322_cec_adap_enable(struct cec_adapter *adap, bool enable)
>> +{
>> + struct ch7322 *ch7322 = cec_get_drvdata(adap);
>> + int ret;
>> +
>> + if (enable)
>> + ret = ch7322_unmask_interrupt(ch7322);
>> + else
>> + ret = ch7322_mask_interrupt(ch7322);
>> +
>> + return ret;
>> +}
>> +
>
> I just realized that doing this here is broken -- the driver depends
> on the interrupt to detect when the physical address changes. I could
> mask only the tx/rx interrupt here instead but that is starting to
> feel a bit pointless.
Ah, OK. Just drop this then.
>
> I haven't looked into the cec notifier mechanism yet but would it be
> better to try to use that instead if possible and just ignore this
> device's physical address detection? Then I could do more of a proper
> reset in this enable op. But I'm not sure if I can properly associate
> the device with an HDMI port on my platform unless I make some changes
> to coreboot.
I don't think this is useful: it's nice to have the CEC adapter do the
physical address detection.
One question about that though: if there is no physical address, can this
driver still transmit CEC messages? Specifically Image View On. This is
important to wake up displays that pull down the HPD when in standby.
The easiest way to test this is with a Pulse-Eight CEC adapter.
See also the section "CEC Without HPD" here:
https://hverkuil.home.xs4all.nl/cec-status.txt
Regards,
Hans
^ permalink raw reply
* Re: [PATCH v4 03/11] dmaengine: Introduce min burst length capability
From: Andy Shevchenko @ 2020-05-29 12:07 UTC (permalink / raw)
To: Serge Semin
Cc: Vinod Koul, Viresh Kumar, Dan Williams, Serge Semin,
Alexey Malahov, Thomas Bogendoerfer, Arnd Bergmann, Rob Herring,
linux-mips, devicetree, dmaengine, linux-kernel
In-Reply-To: <20200528222401.26941-4-Sergey.Semin@baikalelectronics.ru>
On Fri, May 29, 2020 at 01:23:53AM +0300, Serge Semin wrote:
> Some hardware aside from default 0/1 may have greater minimum burst
> transactions length constraints. Here we introduce the DMA device
> and slave capability, which if required can be initialized by the DMA
> engine driver with the device-specific value.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: linux-mips@vger.kernel.org
> Cc: devicetree@vger.kernel.org
>
> ---
>
> Changelog v3:
> - This is a new patch created as a result of the discussion with Vinud and
> Andy in the framework of DW DMA burst and LLP capabilities.
> ---
> drivers/dma/dmaengine.c | 1 +
> include/linux/dmaengine.h | 4 ++++
> 2 files changed, 5 insertions(+)
>
> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index d31076d9ef25..b332ffe52780 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
> @@ -590,6 +590,7 @@ int dma_get_slave_caps(struct dma_chan *chan, struct dma_slave_caps *caps)
> caps->src_addr_widths = device->src_addr_widths;
> caps->dst_addr_widths = device->dst_addr_widths;
> caps->directions = device->directions;
> + caps->min_burst = device->min_burst;
> caps->max_burst = device->max_burst;
> caps->residue_granularity = device->residue_granularity;
> caps->descriptor_reuse = device->descriptor_reuse;
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index e1c03339918f..0c7403b27133 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -465,6 +465,7 @@ enum dma_residue_granularity {
> * Since the enum dma_transfer_direction is not defined as bit flag for
> * each type, the dma controller should set BIT(<TYPE>) and same
> * should be checked by controller as well
> + * @min_burst: min burst capability per-transfer
> * @max_burst: max burst capability per-transfer
> * @cmd_pause: true, if pause is supported (i.e. for reading residue or
> * for resume later)
> @@ -478,6 +479,7 @@ struct dma_slave_caps {
> u32 src_addr_widths;
> u32 dst_addr_widths;
> u32 directions;
> + u32 min_burst;
> u32 max_burst;
> bool cmd_pause;
> bool cmd_resume;
> @@ -769,6 +771,7 @@ struct dma_filter {
> * Since the enum dma_transfer_direction is not defined as bit flag for
> * each type, the dma controller should set BIT(<TYPE>) and same
> * should be checked by controller as well
> + * @min_burst: min burst capability per-transfer
> * @max_burst: max burst capability per-transfer
> * @residue_granularity: granularity of the transfer residue reported
> * by tx_status
> @@ -839,6 +842,7 @@ struct dma_device {
> u32 src_addr_widths;
> u32 dst_addr_widths;
> u32 directions;
> + u32 min_burst;
> u32 max_burst;
> bool descriptor_reuse;
> enum dma_residue_granularity residue_granularity;
> --
> 2.26.2
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 05/11] dmaengine: Introduce DMA-device device_caps callback
From: Andy Shevchenko @ 2020-05-29 12:12 UTC (permalink / raw)
To: Serge Semin
Cc: Vinod Koul, Viresh Kumar, Dan Williams, Serge Semin,
Alexey Malahov, Thomas Bogendoerfer, Arnd Bergmann, Rob Herring,
linux-mips, devicetree, dmaengine, linux-kernel
In-Reply-To: <20200528222401.26941-6-Sergey.Semin@baikalelectronics.ru>
On Fri, May 29, 2020 at 01:23:55AM +0300, Serge Semin wrote:
> There are DMA devices (like ours version of Synopsys DW DMAC) which have
> DMA capabilities non-uniformly redistributed amongst the device channels.
> In order to provide a way of exposing the channel-specific parameters to
> the DMA engine consumers, we introduce a new DMA-device callback. In case
> if provided it gets called from the dma_get_slave_caps() method and is
> able to override the generic DMA-device capabilities.
I thought there is a pattern to return something, but it seems none.
So, I have nothing against it to return void.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
But consider one comment below.
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: linux-mips@vger.kernel.org
> Cc: devicetree@vger.kernel.org
>
> ---
>
> Changelog v3:
> - This is a new patch created as a result of the discussion with Vinod and
> Andy in the framework of DW DMA burst and LLP capabilities.
> ---
> drivers/dma/dmaengine.c | 3 +++
> include/linux/dmaengine.h | 2 ++
> 2 files changed, 5 insertions(+)
>
> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index ad56ad58932c..edbb11d56cde 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
> @@ -599,6 +599,9 @@ int dma_get_slave_caps(struct dma_chan *chan, struct dma_slave_caps *caps)
> caps->cmd_resume = !!device->device_resume;
> caps->cmd_terminate = !!device->device_terminate_all;
>
Perhaps a comment to explain that this is channel specific correction /
override / you name it on top of device level capabilities?
> + if (device->device_caps)
> + device->device_caps(chan, caps);
> +
> return 0;
> }
> EXPORT_SYMBOL_GPL(dma_get_slave_caps);
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index a7e4d8dfdd19..b303e59929e5 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -899,6 +899,8 @@ struct dma_device {
> struct dma_chan *chan, dma_addr_t dst, u64 data,
> unsigned long flags);
>
> + void (*device_caps)(struct dma_chan *chan,
> + struct dma_slave_caps *caps);
> int (*device_config)(struct dma_chan *chan,
> struct dma_slave_config *config);
> int (*device_pause)(struct dma_chan *chan);
> --
> 2.26.2
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 07/11] dmaengine: dw: Set DMA device max segment size parameter
From: Andy Shevchenko @ 2020-05-29 12:18 UTC (permalink / raw)
To: Serge Semin
Cc: Vinod Koul, Viresh Kumar, Dan Williams, Serge Semin,
Alexey Malahov, Thomas Bogendoerfer, Arnd Bergmann, Rob Herring,
linux-mips, devicetree, dmaengine, linux-kernel
In-Reply-To: <20200528222401.26941-8-Sergey.Semin@baikalelectronics.ru>
On Fri, May 29, 2020 at 01:23:57AM +0300, Serge Semin wrote:
> Maximum block size DW DMAC configuration corresponds to the max segment
> size DMA parameter in the DMA core subsystem notation. Lets set it with a
> value specific to the probed DW DMA controller. It shall help the DMA
> clients to create size-optimized SG-list items for the controller. This in
> turn will cause less dw_desc allocations, less LLP reinitializations,
> better DMA device performance.
Yes, something like that for time being, thanks!
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: linux-mips@vger.kernel.org
> Cc: devicetree@vger.kernel.org
>
> ---
>
> Changelog v2:
> - This is a new patch created in place of the dropped one:
> "dmaengine: dw: Add LLP and block size config accessors".
>
> Changelog v3:
> - Use the block_size found for the very first channel instead of looking for
> the maximum of maximum block sizes.
> - Don't define device-specific device_dma_parameters object, since it has
> already been defined by the platform device core.
> ---
> drivers/dma/dw/core.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
> index 33e99d95b3d3..fb95920c429e 100644
> --- a/drivers/dma/dw/core.c
> +++ b/drivers/dma/dw/core.c
> @@ -1229,6 +1229,13 @@ int do_dma_probe(struct dw_dma_chip *chip)
> BIT(DMA_MEM_TO_MEM);
> dw->dma.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
>
> + /*
> + * For now there is no hardware with non uniform maximum block size
> + * across all of the device channels, so we set the maximum segment
> + * size as the block size found for the very first channel.
> + */
> + dma_set_max_seg_size(dw->dma.dev, dw->chan[0].block_size);
> +
> err = dma_async_device_register(&dw->dma);
> if (err)
> goto err_dma_register;
> --
> 2.26.2
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 08/11] dmaengine: dw: Add dummy device_caps callback
From: Andy Shevchenko @ 2020-05-29 12:19 UTC (permalink / raw)
To: Serge Semin
Cc: Vinod Koul, Viresh Kumar, Dan Williams, Serge Semin,
Alexey Malahov, Thomas Bogendoerfer, Arnd Bergmann, Rob Herring,
linux-mips, devicetree, dmaengine, linux-kernel
In-Reply-To: <20200528222401.26941-9-Sergey.Semin@baikalelectronics.ru>
On Fri, May 29, 2020 at 01:23:58AM +0300, Serge Semin wrote:
> Since some DW DMA controllers (like one installed on Baikal-T1 SoC) may
> have non-uniform DMA capabilities per device channels, let's add
> the DW DMA specific device_caps callback to expose that specifics up to
> the DMA consumer. It's a dummy function for now. We'll fill it in with
> capabilities overrides in the next commits.
This one I leave to Vinod to decide what to do.
It is not harmful per se, but I consider better if it has a user already.
Thus, no tag, sorry.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [RFC PATCH V4 4/4] platform: mtk-isp: Add Mediatek FD driver
From: Jerry-ch Chen @ 2020-05-29 12:26 UTC (permalink / raw)
To: Tomasz Figa
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 (林明俊), Linux Media Mailing List,
srv_heupstream, linux-devicetree, jerry-ch.chen
In-Reply-To: <CAAFQd5DodDfWsHkhQZP-M70k9_2sUwwb4zHtWfTx5EDyEKkwow@mail.gmail.com>
Hi Tomasz,
I Appreciate your review comments, here's the reply.
On Mon, 2020-05-25 at 14:24 +0200, Tomasz Figa wrote:
> 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.
>
Ok, I'll define them in macro with masks.
> >
> > 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.
>
Ok, I'll fix it.
> >
> > > > +
> > > > +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.
yes, the IDs, sorry for the wrong expression.
>
> [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.
>
Ok, got it.
> > 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.
>
Got it.
> > 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.
>
Got it, thanks for the comments.
> [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?
>
Yes, I'll replace sfmt with u32 pixfmt.
> > 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?
Yes, I'll add them back.
This function will be refined as :
static void mtk_fd_fill_pixfmt_mp(struct v4l2_pix_format_mplane *dfmt,
u32 pixfmt)
{
v4l2_fill_pixfmt_mp(dfmt, pixfmt, dfmt->width, dfmt->height);
dfmt->field = V4L2_FIELD_NONE;
dfmt->colorspace = V4L2_COLORSPACE_BT2020;
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);
}
>
> [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?
>
yes, it will be refined as following, I use the src_vb2_buf to reduce
the length for fitting 80 columns
src_vb2_buf = &src_buf->vb2_buf;
dst_vb2_buf = &dst_buf->vb2_buf;
for (i = 0; i < ctx->src_fmt.num_planes; i++)
fd_param.src_img[i].dma_addr =
vb2_dma_contig_plane_dma_addr(src_vb2_buf,i);
fd_param.user_result.dma_addr =
vb2_dma_contig_plane_dma_addr(dst_vb2_buf, 0);
> > 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
Thanks and Best regards,
Jerry
^ permalink raw reply
* Re: [PATCH 1/4] dt-bindings: thermal: rcar-thermal: Add device tree support for r8a7742
From: Geert Uytterhoeven @ 2020-05-29 12:48 UTC (permalink / raw)
To: Lad Prabhakar
Cc: Magnus Damm, Rob Herring, Linux-Renesas,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux Kernel Mailing List, Prabhakar
In-Reply-To: <1590614320-30160-2-git-send-email-prabhakar.mahadev-lad.rj@bp.renesas.com>
On Wed, May 27, 2020 at 11:19 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> Add thermal sensor support for r8a7742 SoC. The Renesas RZ/G1H
> (r8a7742) thermal sensor module is identical to the R-Car Gen2 family.
>
> No driver change is needed due to the fallback compatible value
> "renesas,rcar-gen2-thermal".
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Marian-Cristian Rotariu <marian-cristian.rotariu.rb@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH 3/4] dt-bindings: timer: renesas,cmt: Document r8a7742 CMT support
From: Geert Uytterhoeven @ 2020-05-29 12:53 UTC (permalink / raw)
To: Lad Prabhakar
Cc: Magnus Damm, Rob Herring, Linux-Renesas,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux Kernel Mailing List, Prabhakar
In-Reply-To: <1590614320-30160-4-git-send-email-prabhakar.mahadev-lad.rj@bp.renesas.com>
Hi Prabhakar,
On Wed, May 27, 2020 at 11:19 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> Document SoC specific compatible strings for r8a7742. No driver change
> is needed as the fallback strings will activate the right code.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Marian-Cristian Rotariu <marian-cristian.rotariu.rb@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Please note this DT binding is under yamlization, cfr.
"[PATCH v2] dt-bindings: timer: renesas: cmt: Convert to json-schema"
(20200505155127.4836-1-geert+renesas@glider.be).
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [RFC PATCH V4 4/4] platform: mtk-isp: Add Mediatek FD driver
From: Tomasz Figa @ 2020-05-29 12:59 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 (林明俊), Linux Media Mailing List,
srv_heupstream, linux-devicetree
In-Reply-To: <1590755163.23156.24.camel@mtksdccf07>
On Fri, May 29, 2020 at 2:26 PM Jerry-ch Chen
<Jerry-ch.Chen@mediatek.com> wrote:
>
> Hi Tomasz,
>
> I Appreciate your review comments, here's the reply.
>
> On Mon, 2020-05-25 at 14:24 +0200, Tomasz Figa wrote:
> > 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]
> > Isn't still a need to clamp() width and height to min/max, though?
> Yes, I'll add them back.
>
> This function will be refined as :
>
> static void mtk_fd_fill_pixfmt_mp(struct v4l2_pix_format_mplane *dfmt,
> u32 pixfmt)
> {
> v4l2_fill_pixfmt_mp(dfmt, pixfmt, dfmt->width, dfmt->height);
>
> dfmt->field = V4L2_FIELD_NONE;
> dfmt->colorspace = V4L2_COLORSPACE_BT2020;
> 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);
Note that this would cause the other fields of dfmt to be inconsistent
with width and height. The correct way to do this would be to first
clamp and then call v4l2_fill_pixfmt_mp().
Best regards,
Tomasz
^ 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