Re: [PATCH v2 4/4] mmc: host: tmio: fill in response from auto cmd12
On 15 February 2017 at 16:02, Wolfram Sang wrote: > >> > I see. Ulf, do you think it makes sense to extend the condition when to >> > call mmc_blk_cmd_recovery() with checking if stop.resp[0] has one of the >> > R1_* bits set which are marked with 'ex' (and probably 'erx', too)? I >> > agree with Shimoda-san, that the core is a good place to do it, since it >> > is about parsing the R1 and not the status bits of the host hardware. >> >> The method we use to indicate a stop command error to the mmc core, is >> to set ->stop.error in the host driver before completing the request. >> Perhaps set it to -EIO or -EILSEQ. >> >> In that way mmc_blk_err_check() sees the error and invokes the >> mmc_blk_cmd_recovery() to deal with it (response parsing etc). >> >> Does that work for you? > > It would work, yes. Since R1 response format is hardware independent, I > wondered if checking for ECC errors wouldn't be better suited in the > core. We roughly need something like this: > > if (stop.resp[0] & R1_CARD_ECC_FAILED) > stop.error = -EIO; > > We can copy this into every driver, of course. Yet, I wondered if we > couldn't have a helper function mapping the R1 error bits to an > apropriate error value and call that just before the check in > mmc_blk_err_check(). > > Do you get what I mean? I get it - and yes you have a point. By looking at the code in mmc_blk_err_check() and mmc_blk_cmd_recovery(), it deserves a clean-up. That said, I don't want to treat R1_CARD_ECC_FAILED as a special case. So if you decide to add this check in the core (which I am open to), we should also add checks the other potential R1 errors, to be consistent. Kind regards Uffe
RE: [PATCH v2 1/3] ARM: l2c: add pl310-no-sideband option
On Wednesday, February 15, 2017, Rob Herring wrote: > On Tue, Feb 07, 2017 at 12:09:27PM -0500, Chris Brandt wrote: > > If a PL310 is added to a system, but the sideband signals are not > > connected, some Cortex A9 optimizations cannot be used. In particular, > > enabling Full Line Zeros in the CA9 without sidebands connected will > > crash the system since the CA9 will expect the L2C to perform > > operations, yet the L2C never gets the commands. > > I assume you are talking about just the AxUSER signals, not the AxCACHE > signals too. That would be really broken. IIRC, the other AxUSER signals > are just hints and no connection would not be a problem. This is the only > one that requires coordination with enabling/disabling in the core. Yes, just the AxUSER, hence none of the "2.5.5 Cortex-A9 optimizations" work. Well, except "Store buffer device limitation" because that doesn't use sideband signals. Like you mentioned, having 'Early BRESP' enabled but not used doesn't hurt anything. I was just blocking that so it would print out that it was enabled and give users false hope (not that they are looking at the boot log that closely anyway.) FLZ is the killer. > I think this should follow existing feature properties and explicitly > disable the specific properties rather than have this indirection. I can change the code to: "arm,early-bresp-disable" "arm,fill-line-zero-disable" As I said, blocking BRESP is only for keeping the log message from coming out. If it's preferred to not block anything that doesn't break anything, I can just add "arm,fill-line-zero-disable" only. Just let me know. Chris
[PULL REQUEST] renesas/topic/sdhi-dma-sync for renesas drivers
Hi Geert, in case, we decide to include this patch despite not being tested for the original SDIO case, here is a formal pull request. Kind regards, Wolfram The following changes since commit 391a542fe891d6dac49fa7fc2422651e54af1d42: mmc: Adding AUTO_BKOPS_EN bit set for Auto BKOPS support (2017-02-13 13:21:08 +0100) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/topic/sdhi-dma-sync for you to fetch changes up to 2ea4d49db87684a27026922b73119d9b69c46074: mmc: host: tmio: ensure end of DMA and SD access are in sync (2017-02-15 23:17:55 +0100) Wolfram Sang (1): mmc: host: tmio: ensure end of DMA and SD access are in sync drivers/mmc/host/tmio_mmc.h | 2 +- drivers/mmc/host/tmio_mmc_dma.c | 70 +++-- drivers/mmc/host/tmio_mmc_pio.c | 4 +-- 3 files changed, 49 insertions(+), 27 deletions(-) signature.asc Description: PGP signature
Re: [PATCH v2 1/3] ARM: l2c: add pl310-no-sideband option
On Tue, Feb 07, 2017 at 12:09:27PM -0500, Chris Brandt wrote: > If a PL310 is added to a system, but the sideband signals are not > connected, some Cortex A9 optimizations cannot be used. In particular, > enabling Full Line Zeros in the CA9 without sidebands connected will > crash the system since the CA9 will expect the L2C to perform operations, > yet the L2C never gets the commands. I assume you are talking about just the AxUSER signals, not the AxCACHE signals too. That would be really broken. IIRC, the other AxUSER signals are just hints and no connection would not be a problem. This is the only one that requires coordination with enabling/disabling in the core. I think this should follow existing feature properties and explicitly disable the specific properties rather than have this indirection. > > Signed-off-by: Chris Brandt > --- > Documentation/devicetree/bindings/arm/l2c2x0.txt | 2 ++ > arch/arm/mm/cache-l2x0.c | 9 +++-- > 2 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/arm/l2c2x0.txt > b/Documentation/devicetree/bindings/arm/l2c2x0.txt > index 917199f..85046d2 100644 > --- a/Documentation/devicetree/bindings/arm/l2c2x0.txt > +++ b/Documentation/devicetree/bindings/arm/l2c2x0.txt > @@ -90,6 +90,8 @@ Optional properties: > - arm,standby-mode: L2 standby mode enable. Value <0> (forcibly disable), ><1> (forcibly enable), property absent (OS specific behavior, >preferably retain firmware settings) > +- arm,pl310-no-sideband : disable all features that require sideband signals > to > + be connected between the CPU and L2 (PL310 only). > > Example: >
Re: [PATCH/RFC] clk: renesas: r8a7795: Replace PLL3 multiplication setting
Hi Geert-san,
2017-02-15 18:28 GMT+09:00 Geert Uytterhoeven :
> Hi Kaneko-san,
>
> On Sun, Feb 12, 2017 at 6:35 PM, Yoshihiro Kaneko
> wrote:
>> From: Takeshi Kihara
>>
>> This patch replaces PLL3 multiplication setting for DDR clock frequency.
>>
>> - After changes, new PLL3 multiplication setting:
>> MD19 MD17 : DDR clock frequency
>> ---
>> 00 : DDR3200
>> 01 : DDR2800
>> 10 : DDR2400
>> 11 : DDR1600
>>
>> - Before changes, old PLL3 multiplication setting:
>> MD19 MD17 : DDR clock frequency
>> ---
>> 00 : DDR3200
>> 01 : DDR2133
>> 10 : Prohibited setting
>> 11 : DDR1600
>>
>> Signed-off-by: Takeshi Kihara
>> Signed-off-by: Yoshihiro Kaneko
>
> JFTR, this patch, and its counterpart for r8a7796, are too experimental
> for upstream.
Thanks for your review.
I withdraw these patches.
Thanks,
Kaneko
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@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
[RFC] mmc: host: tmio: ensure end of DMA and SD access are in sync
The current code assumes that DMA is finished before SD access end is
flagged. Thus, it schedules the 'dma_complete' tasklet in the SD card
interrupt routine when DATAEND is set. The assumption is not safe,
though. Even by mounting an SD card, it can be seen that sometimes DMA
complete is first, sometimes DATAEND. It seems they are usually close
enough timewise to not cause problems. However, a customer reported that
with CMD53 sometimes things really break apart. As a result, the BSP has
a patch which introduces flags for both events and makes sure both flags
are set before scheduling the tasklet. The customer accepted the patch,
yet it doesn't seem a proper upstream solution to me.
This patch refactors the code to replace the tasklet with already
existing and more lightweight mechanisms. First of all, we set the
callback in a DMA descriptor to automatically get notified when DMA is
done. In the callback, we then use a completion to make sure the SD
access has already ended. Then, we proceed as before.
Signed-off-by: Wolfram Sang
---
I tried various synchronization approaches and liked this one best.
There are a couple of reasons this patch is RFC:
The #ifdef's need to go but are handy for now.
More testing is needed. While it worked fine for me writing terrabytes to
different cards (still trying to break the wear-levelling), broader testing
with different workloads and use-cases is largely welcome. I specifically
couldn't test with CMD53 (SDIO) which originally caused the problem for the
customer because that seems not working with my Gen2 board. I tried to fix SDIO
on Gen2 as a side-task but didn't find anything obvious on a glimpse. SDIO
works with my Gen3 boards but for that we don't have DMA upstream yet.
Upstreaming Gen3 DMA will need some bigger DMA refactoring, so it might be
worth waiting until the refactoring is done. In a nutshell, there are
dependency issues currently.
So, calling for early review here. And opinions how to proceed. I am not sure
we want this in renesas-drivers until the SDIO functionality has been verified?
Because it is the reason this patch exists in the first place :)
The branch is here:
git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git
renesas/topic/sdhi-dma-sync
The test description is here:
http://elinux.org/Tests:SDHI-DMA-Sync
Cheers and thanks,
Wolfram
drivers/mmc/host/tmio_mmc.h | 2 +-
drivers/mmc/host/tmio_mmc_dma.c | 71 +++--
drivers/mmc/host/tmio_mmc_pio.c | 4 +--
3 files changed, 50 insertions(+), 27 deletions(-)
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h
index 2b349d48fb9a8a..891d400d2a7cf2 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -137,7 +137,7 @@ struct tmio_mmc_host {
boolforce_pio;
struct dma_chan *chan_rx;
struct dma_chan *chan_tx;
- struct tasklet_struct dma_complete;
+ struct completion dma_dataend;
struct tasklet_struct dma_issue;
struct scatterlist bounce_sg;
u8 *bounce_buf;
diff --git a/drivers/mmc/host/tmio_mmc_dma.c b/drivers/mmc/host/tmio_mmc_dma.c
index fa8a936a3d9ba1..d2b6aed644f361 100644
--- a/drivers/mmc/host/tmio_mmc_dma.c
+++ b/drivers/mmc/host/tmio_mmc_dma.c
@@ -43,6 +44,43 @@ void tmio_mmc_abort_dma(struct tmio_mmc_host *host)
tmio_mmc_enable_dma(host, true);
}
+static void tmio_mmc_dma_callback(void *arg)
+{
+ struct tmio_mmc_host *host = arg;
+
+#ifdef DEBUG
+ u32 status = sd_ctrl_read16_and_16_as_32(host, CTL_STATUS);
+
+ dev_dbg(&host->pdev->dev, "DMA complete (0x%08x)\n", status);
+#endif
+
+ wait_for_completion(&host->dma_dataend);
+
+#ifdef DEBUG
+ status = sd_ctrl_read16_and_16_as_32(host, CTL_STATUS);
+
+ dev_dbg(&host->pdev->dev, "DATAEND complete (0x%08x)\n", status);
+#endif
+
+ spin_lock_irq(&host->lock);
+
+ if (!host->data)
+ goto out;
+
+ if (host->data->flags & MMC_DATA_READ)
+ dma_unmap_sg(host->chan_rx->device->dev,
+host->sg_ptr, host->sg_len,
+DMA_FROM_DEVICE);
+ else
+ dma_unmap_sg(host->chan_tx->device->dev,
+host->sg_ptr, host->sg_len,
+DMA_TO_DEVICE);
+
+ tmio_mmc_do_data_irq(host);
+out:
+ spin_unlock_irq(&host->lock);
+}
+
static void tmio_mmc_start_dma_rx(struct tmio_mmc_host *host)
{
struct scatterlist *sg = host->sg_ptr, *sg_tmp;
@@ -88,6 +126,10 @@ static void tmio_mmc_start_dma_rx(struct tmio_mmc_host
*host)
DMA_DEV_TO_MEM, DMA_CTRL_ACK);
if (desc) {
+ reinit_completion(&host->dma_dataend);
+ desc->callback = tmio_mmc_dma_callback;
+ desc->callback_param = host;
+
cookie = dmaengine_submit(desc);
RE: [PATCH 2/3] dt-bindings: power: reset: add document for renesas-reset driver
Hi Geert, On Tuesday, February 14, 2017, Geert Uytterhoeven wrote: > > +Required properties: > > + - compatible: must be one or more of the following: > > +- "renesas,r7s72100-reset" for the r7s72100 SoC > > +- "renesas,wdt-reset" > > +This is a fallback for the above renesas,*-reset > > +entries > > Please use "renesas,r7s72100-wdt". DT describes the hardware (watchdog), > not the software policy (use it for reset only, as a max. timeout of 125 > ms is too short for a usable watchdog). I had a look at: Documentation/devicetree/bindings/watchdog/renesas-wdt.txt Which has: Required properties: - compatible : Should be "renesas,-wdt", and "renesas,rcar-gen3-wdt" as fallback. Examples with soctypes are: - "renesas,r8a7795-wdt" (R-Car H3) - "renesas,r8a7796-wdt" (R-Car M3-W) So in my 'renesas-reset.txt' should I do: A. "renesas,r7s72100-wdt", "renesas,rz-wdt" or just: B. "renesas,r7s72100-wdt" (no fallback, change the driver to add new SoCs) Thoughts Thank you, Chris
Re: [PATCH v3 6/7] dt-bindings: media: Add Renesas R-Car DRIF binding
On Tue, Feb 07, 2017 at 03:02:36PM +, Ramesh Shanmugasundaram wrote: > Add binding documentation for Renesas R-Car Digital Radio Interface > (DRIF) controller. > > Signed-off-by: Ramesh Shanmugasundaram > > --- > .../devicetree/bindings/media/renesas,drif.txt | 186 > + > 1 file changed, 186 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/renesas,drif.txt > > diff --git a/Documentation/devicetree/bindings/media/renesas,drif.txt > b/Documentation/devicetree/bindings/media/renesas,drif.txt > new file mode 100644 > index 000..6315609 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/renesas,drif.txt > @@ -0,0 +1,186 @@ > +Renesas R-Car Gen3 Digital Radio Interface controller (DRIF) > + > + > +R-Car Gen3 DRIF is a SPI like receive only slave device. A general > +representation of DRIF interfacing with a master device is shown below. > + > ++-++-+ > +| |-SCK--->|CLK | > +| Master|-SS>|SYNC DRIFn (slave) | > +| |-SD0--->|D0 | > +| |-SD1--->|D1 | > ++-++-+ > + > +As per the datasheet, each DRIF channel (drifn) is made up of two internal > +channels (drifn0 & drifn1). These two internal channels share the common > +CLK & SYNC. Each internal channel has its own dedicated resources like > +irq, dma channels, address space & clock. This internal split is not > +visible to the external master device. > + > +The device tree model represents each internal channel as a separate node. > +The internal channels sharing the CLK & SYNC are tied together by their > +phandles using a new property called "renesas,bonding". For the rest of > +the documentation, unless explicitly stated, the word channel implies an > +internal channel. > + > +When both internal channels are enabled they need to be managed together > +as one (i.e.) they cannot operate alone as independent devices. Out of the > +two, one of them needs to act as a primary device that accepts common > +properties of both the internal channels. This channel is identified by a > +new property called "renesas,primary-bond". > + > +To summarize, > + - When both the internal channels that are bonded together are enabled, > + the zeroth channel is selected as primary-bond. This channels accepts > + properties common to all the members of the bond. > + - When only one of the bonded channels need to be enabled, the property > + "renesas,bonding" or "renesas,primary-bond" will have no effect. That > + enabled channel can act alone as any other independent device. > + > +Required properties of an internal channel: > +--- > +- compatible: "renesas,r8a7795-drif" if DRIF controller is a part of R8A7795 > SoC. > + "renesas,rcar-gen3-drif" for a generic R-Car Gen3 compatible > device. > + When compatible with the generic version, nodes must list the > + SoC-specific version corresponding to the platform first > + followed by the generic version. > +- reg: offset and length of that channel. > +- interrupts: associated with that channel. > +- clocks: phandle and clock specifier of that channel. > +- clock-names: clock input name string: "fck". > +- dmas: phandles to the DMA channels. > +- dma-names: names of the DMA channel: "rx". > +- renesas,bonding: phandle to the other channel. > + > +Optional properties of an internal channel: > +--- > +- power-domains: phandle to the respective power domain. > + > +Required properties of an internal channel when: > + - It is the only enabled channel of the bond (or) > + - If it acts as primary among enabled bonds > + > +- pinctrl-0: pin control group to be used for this channel. > +- pinctrl-names: must be "default". > +- renesas,primary-bond: empty property indicating the channel acts as primary > + among the bonded channels. > +- port: child port node of a channel that defines the local and remote > + endpoints. The remote endpoint is assumed to be a third party tuner > + device endpoint. > + > +Optional endpoint property: > +--- > +- renesas,sync-active : Indicates sync signal polarity, 0/1 for low/high > + respectively. This property maps to SYNCAC bit in the > + hardware manual. The default is 1 (active high) Why does this belong in the endpoint? I'd prefer to not have vendor specific properties in endpoints. Is this a property of the tuner or DRIF? > + > +Example > + > + > +SoC common dtsi file > + > + drif00:
Re: [PATCH v3 2/7] dt-bindings: media: Add MAX2175 binding description
On Tue, Feb 07, 2017 at 03:02:32PM +, Ramesh Shanmugasundaram wrote: > Add device tree binding documentation for MAX2175 Rf to bits tuner > device. > > Signed-off-by: Ramesh Shanmugasundaram > > --- > .../devicetree/bindings/media/i2c/max2175.txt | 61 > ++ > .../devicetree/bindings/property-units.txt | 1 + > 2 files changed, 62 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/max2175.txt Acked-by: Rob Herring
Re: [PATCH v2 4/4] mmc: host: tmio: fill in response from auto cmd12
> > I see. Ulf, do you think it makes sense to extend the condition when to > > call mmc_blk_cmd_recovery() with checking if stop.resp[0] has one of the > > R1_* bits set which are marked with 'ex' (and probably 'erx', too)? I > > agree with Shimoda-san, that the core is a good place to do it, since it > > is about parsing the R1 and not the status bits of the host hardware. > > The method we use to indicate a stop command error to the mmc core, is > to set ->stop.error in the host driver before completing the request. > Perhaps set it to -EIO or -EILSEQ. > > In that way mmc_blk_err_check() sees the error and invokes the > mmc_blk_cmd_recovery() to deal with it (response parsing etc). > > Does that work for you? It would work, yes. Since R1 response format is hardware independent, I wondered if checking for ECC errors wouldn't be better suited in the core. We roughly need something like this: if (stop.resp[0] & R1_CARD_ECC_FAILED) stop.error = -EIO; We can copy this into every driver, of course. Yet, I wondered if we couldn't have a helper function mapping the R1 error bits to an apropriate error value and call that just before the check in mmc_blk_err_check(). Do you get what I mean? Thanks, Wolfram signature.asc Description: PGP signature
Re: [PATCH 1/2] spi: rspi: Fixes bogus received byte in qspi_transfer_in()
On 02/15/2017 01:50 PM, DongCV wrote:
In qspi_transfer_in(), when receiving the last n (or len) bytes of data,
one bogus byte was written in the receive buffer.
This code leads to a buffer overflow.
"jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found
at 0x03b4: 0x1900 instead
jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found
at 0x03b40004: 0x000c instead"
The error message above happens when trying to mount, unmount,
and remount a jffs2-formatted device.
This patch removed the bogus write to fixes: 3be09bec42a800d4
"spi: rspi: supports 32bytes buffer for DUAL and QUAD"
You were just asked to add the following tag to the patch (e.g. before
your signoff):
Fixes: 3be09bec42a8 ("spi: rspi: supports 32bytes buffer for DUAL and QUAD")
This simplifies the propagation of the patch to the -stable releases...
And here is Geert's comment:
"spi: rspi: Fix bogus received byte in qspi_transfer_in()
When there are less than QSPI_BUFFER_SIZE remaining bytes to be received,
qspi_transfer_in() writes one bogus byte in the receive buffer, possibly
leading to a buffer overflow.
This can be reproduced by mounting, unmounting, and remounting a
jffs2-formatted device, causing lots of warnings like:
"jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found
at 0x03b4: 0x1900 instead"
Remove the bogus write to fix this. "
I don't think effectively duplicating your patch description makes sense
here.
Signed-off-by: DongCV
Need full name here.
Reviewed-by: Geert Uytterhoeven
[...]
MBR, Sergei
Re: [PATCH 2/2] spi: rspi: Replaces "n" by "len" in qspi_transfer_*()
On Wed, Feb 15, 2017 at 11:50 AM, DongCV wrote:
> This patch replaced "n" by "len" bytes of data in qspi_transfer_in() and
> qspi_transfer_out() function. This will make improving readability.
>
> Signed-off-by: DongCV
Reviewed-by: Geert Uytterhoeven
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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
[PATCH 2/2] spi: rspi: Replaces "n" by "len" in qspi_transfer_*()
This patch replaced "n" by "len" bytes of data in qspi_transfer_in() and
qspi_transfer_out() function. This will make improving readability.
Signed-off-by: DongCV
---
drivers/spi/spi-rspi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/spi/spi-rspi.c b/drivers/spi/spi-rspi.c
index 2ee1301..bc3c868 100644
--- a/drivers/spi/spi-rspi.c
+++ b/drivers/spi/spi-rspi.c
@@ -808,7 +808,7 @@ static int qspi_transfer_out(struct rspi_data *rspi, struct
spi_transfer *xfer)
for (i = 0; i < len; i++)
rspi_write_data(rspi, *tx++);
} else {
- ret = rspi_pio_transfer(rspi, tx, NULL, n);
+ ret = rspi_pio_transfer(rspi, tx, NULL, len);
if (ret < 0)
return ret;
}
@@ -845,7 +845,7 @@ static int qspi_transfer_in(struct rspi_data *rspi, struct
spi_transfer *xfer)
for (i = 0; i < len; i++)
*rx++ = rspi_read_data(rspi);
} else {
- ret = rspi_pio_transfer(rspi, NULL, rx, n);
+ ret = rspi_pio_transfer(rspi, NULL, rx, len);
if (ret < 0)
return ret;
}
--
1.9.1
[PATCH 0/2 v4] spi: rspi: Fixes bogus received byte and replaces "n" by "len"
Hi Mark, Cc: Geert, Sergei Thanks for your comments and advices. I separated my patch into two patches as you've shown me. These patches fix: 3be09bec42a800d4 "spi: rspi: supports 32bytes buffer for DUAL and QUAD". One of them is to fix bogus received byte into qspi_transfer_in(), and other is to replace "n" by "len" bytes of data in qspi_transfer_in()and qspi_transfer_out(). These patches were tested at v4.10-rc8 on Renesas Gen2 Lager board. Please consider these patches to Renesas Rcar family. Thank you. Jinso/Dong. DongCV (2): spi: rspi: Fixes bogus received byte in qspi_transfer_in() spi: rspi: Replaces "n" by "len" in qspi_transfer_in() and qspi_transfer_out() drivers/spi/spi-rspi.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) -- 1.9.1
[PATCH 1/2] spi: rspi: Fixes bogus received byte in qspi_transfer_in()
In qspi_transfer_in(), when receiving the last n (or len) bytes of data, one bogus byte was written in the receive buffer. This code leads to a buffer overflow. "jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b4: 0x1900 instead jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40004: 0x000c instead" The error message above happens when trying to mount, unmount, and remount a jffs2-formatted device. This patch removed the bogus write to fixes: 3be09bec42a800d4 "spi: rspi: supports 32bytes buffer for DUAL and QUAD" And here is Geert's comment: "spi: rspi: Fix bogus received byte in qspi_transfer_in() When there are less than QSPI_BUFFER_SIZE remaining bytes to be received, qspi_transfer_in() writes one bogus byte in the receive buffer, possibly leading to a buffer overflow. This can be reproduced by mounting, unmounting, and remounting a jffs2-formatted device, causing lots of warnings like: "jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b4: 0x1900 instead" Remove the bogus write to fix this. " Signed-off-by: DongCV Reviewed-by: Geert Uytterhoeven --- drivers/spi/spi-rspi.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/spi/spi-rspi.c b/drivers/spi/spi-rspi.c index 9daf500..2ee1301 100644 --- a/drivers/spi/spi-rspi.c +++ b/drivers/spi/spi-rspi.c @@ -848,7 +848,6 @@ static int qspi_transfer_in(struct rspi_data *rspi, struct spi_transfer *xfer) ret = rspi_pio_transfer(rspi, NULL, rx, n); if (ret < 0) return ret; - *rx++ = ret; } n -= len; } -- 1.9.1
[PATCH v4] spi: rspi: Fixes bogus received byte in qspi_transfer_in()
In qspi_transfer_in(), when receiving the last n (or len) bytes of data, 1 bogus byte was written in the receive buffer. This code leads to a buffer overflow. "jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b4: 0x1900 instead jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40004: 0x000c instead" The error message above happens when trying to mount, unmount, and remount a jffs2-formatted device. This patch removed the bogus write to fixes: 3be09bec42a800d4 "spi: rspi: supports 32bytes buffer for DUAL and QUAD" And here is Geert's comment: "May I suggest the following: spi: rspi: Fix bogus received byte in qspi_transfer_in() When there are less than QSPI_BUFFER_SIZE remaining bytes to be received, qspi_transfer_in() writes one bogus byte in the receive buffer, possibly leading to a buffer overflow. This can be reproduced by mounting, unmounting, and remounting a jffs2-formatted device, causing lots of warnings like: jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b4: 0x1900 instead Remove the bogus write to fix this. " Signed-off-by: DongCV Reviewed-by: Geert Uytterhoeven --- drivers/spi/spi-rspi.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/spi/spi-rspi.c b/drivers/spi/spi-rspi.c index 9daf500..2ee1301 100644 --- a/drivers/spi/spi-rspi.c +++ b/drivers/spi/spi-rspi.c @@ -848,7 +848,6 @@ static int qspi_transfer_in(struct rspi_data *rspi, struct spi_transfer *xfer) ret = rspi_pio_transfer(rspi, NULL, rx, n); if (ret < 0) return ret; - *rx++ = ret; } n -= len; } -- 1.9.1
Re: [PATCH v2 4/4] mmc: host: tmio: fill in response from auto cmd12
On 14 February 2017 at 11:52, Wolfram Sang wrote: > Shimoda-san, Ulf, > > On Tue, Feb 14, 2017 at 10:06:47AM +, Yoshihiro Shimoda wrote: >> Hi, >> >> > From: Wolfram Sang [mailto:wsa+rene...@sang-engineering.com] >> > Sent: Tuesday, February 14, 2017 3:04 AM >> > >> > After we received the dataend interrupt, R1 response register carries >> > the value from the automatically generated stop command. Report that >> > info back to the MMC block layer, so we will be notified in case of e.g. >> > ECC errors which happened during the last transfer. >> > >> > Reviewed-by: Simon Horman >> > Signed-off-by: Wolfram Sang >> >> I tested this patch with a SD tester (SGDK320). >> As the commit log, this patch could pass the R1 response. So, >> >> Tested-by: Yoshihiro Shimoda > > Thank you very much for testing! > >> However, I think the MMC block layer should check the brq->stop.resp[0] >> because brq->stop.error should be zero in this case and >> mmc_blk_cmd_recovery() >> is not called in mmc_blk_err_check(). > > I see. Ulf, do you think it makes sense to extend the condition when to > call mmc_blk_cmd_recovery() with checking if stop.resp[0] has one of the > R1_* bits set which are marked with 'ex' (and probably 'erx', too)? I > agree with Shimoda-san, that the core is a good place to do it, since it > is about parsing the R1 and not the status bits of the host hardware. The method we use to indicate a stop command error to the mmc core, is to set ->stop.error in the host driver before completing the request. Perhaps set it to -EIO or -EILSEQ. In that way mmc_blk_err_check() sees the error and invokes the mmc_blk_cmd_recovery() to deal with it (response parsing etc). Does that work for you? Kind regards Uffe
Re: How to get related device pointer via DT?
On Wed, Feb 15, 2017 at 02:21:55AM +, Yoshihiro Shimoda wrote:
> Hi,
>
> I would like to get a related device pointer on usb EHCI drivers (or USB
> framework)
> because related device (e.g. OHCI or UHCI, called "companion controllers")
> has to
> finish resuming. I discussed this topic with Alan:
> http://marc.info/?t=14865351421&r=1&w=2
>
> In PCI bus, USB framework already has such a feature in
> drivers/usb/core/hcd-pci.c.
> However, in platform devices, we don't have it for now. So, I would like to
> add it.
>
> Then, I have 2 ideas to get the related device pointer:
>
> A) We add a new property "companion" as usb-generic.txt and EHCI node(s) have
>such a property to bind a companion controller.
> B) We assume EHCI controller binds a companion controller if some resources
>(irq or clock) are the same and it has a compatible strings as
> "generic-[uo]hci"
>for instance.
>
> My environment is R-Car H3, and it has 3 EHCI and 3 OHCI controllers.
> For example (I only wrote channel 0 of EHCI and OHCI):
> ehci0: usb@ee080100 {
> compatible = "generic-ehci";
> reg = <0 0xee080100 0 0x100>;
> interrupts = ;
> clocks = <&cpg CPG_MOD 703>;
> phys = <&usb2_phy0>;
> phy-names = "usb";
> power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
> status = "disabled";
> };
>
> ohci0: usb@ee08 {
> compatible = "generic-ohci";
> reg = <0 0xee08 0 0x100>;
> interrupts = ;
> clocks = <&cpg CPG_MOD 703>;
> phys = <&usb2_phy0>;
> phy-names = "usb";
> power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
> status = "disabled";
> };
>
> If my idea A), ehci0 will have companion = <&ohci>;
> If my idea B), no need to add any property.
>
> What do you think?
> Anyway, I will start to study DT programming :)
>
>From my point, idea A is better. It seems you have similar idea when you
tried Dual-role framework before. [1]
[1] https://patchwork.kernel.org/patch/9169745/
--
Best Regards,
Peter Chen
Re: [PATCH 1/1] arm64: dts: r8a7795: add IMR-LX4 support
On 2/15/2017 12:02 AM, Sergei Shtylyov wrote:
Describe the IMR-LX4 devices in the R8A7795 device tree.
Based on the original (and large) patch by Konstantin Kozhevnikov
.
Signed-off-by: Konstantin Kozhevnikov
Signed-off-by: Sergei Shtylyov
---
This patch is against the 'renesas-devel-20170213-v4.10-rc8' tag of Simon
Horman's 'renesas.git' repo.
The binding was documented in the IMR driver patch. I don't expect it to
change even if the driver would be rewritten for DRM.
[...]
Index: renesas/arch/arm64/boot/dts/renesas/r8a7795.dtsi
===
--- renesas.orig/arch/arm64/boot/dts/renesas/r8a7795.dtsi
+++ renesas/arch/arm64/boot/dts/renesas/r8a7795.dtsi
@@ -1419,6 +1419,38 @@
status = "disabled";
};
+ imr-lx4@fe86 {
+ compatible = "renesas,imr-lx4";
The manual seems to suggest that IMR-LX4 devicec are different between
7795 and 7796 SoCs, so we may need SoC specific compatibility. I'll look into
this...
MBR, Sergei
Re: [PATCH/RFC] clk: renesas: r8a7795: Replace PLL3 multiplication setting
Hi Kaneko-san,
On Sun, Feb 12, 2017 at 6:35 PM, Yoshihiro Kaneko wrote:
> From: Takeshi Kihara
>
> This patch replaces PLL3 multiplication setting for DDR clock frequency.
>
> - After changes, new PLL3 multiplication setting:
> MD19 MD17 : DDR clock frequency
> ---
> 00 : DDR3200
> 01 : DDR2800
> 10 : DDR2400
> 11 : DDR1600
>
> - Before changes, old PLL3 multiplication setting:
> MD19 MD17 : DDR clock frequency
> ---
> 00 : DDR3200
> 01 : DDR2133
> 10 : Prohibited setting
> 11 : DDR1600
>
> Signed-off-by: Takeshi Kihara
> Signed-off-by: Yoshihiro Kaneko
JFTR, this patch, and its counterpart for r8a7796, are too experimental
for upstream.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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
Re: [PATCH 3/3] ARM: dts: r7s72100: Add reset handler
On Mon, Feb 13, 2017 at 7:25 PM, Chris Brandt wrote:
> For the RZ/A1, the only way to do a reset is to overflow the WDT.
>
> Signed-off-by: Chris Brandt
> ---
> arch/arm/boot/dts/r7s72100.dtsi | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi
> index 614ba79..6fb4ef4 100644
> --- a/arch/arm/boot/dts/r7s72100.dtsi
> +++ b/arch/arm/boot/dts/r7s72100.dtsi
> @@ -371,6 +371,11 @@
> <0xe8202000 0x1000>;
> };
>
> + wdt: timer@fcfe {
> + compatible = "renesas,r7s72100-reset", "renesas,wdt-reset";
> + reg = <0xfcfe 0x6>;
No clocks property, pointing to p0_clk?
No interrupts property?
Yes, those should be documented in the bindings, too.
> + };
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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
Re: [PATCH] spi: rspi: Fixes bogus received byte and replaces "n" by "len" in commit 3be09be
Hello!
On 2/15/2017 12:12 PM, DongCV wrote:
In qspi_transfer_in(), when receiving the last n (or len) bytes of data,
1 bogus byte was written in the receive buffer.
This code leads to a buffer overflow.
"jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b4:
0x1900 instead
jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40004:
0x000c instead"
The error message above happens when trying to mount, unmount, and remount a
jffs2-formatted device.
This patch removed the bogus write and for clearer to use, here replaced "n" by
"len" in
qspi_transfer_in() and qspi_transfer_out() to fixes: 3be09bec42a800d4
"spi: rspi: supports 32bytes buffer for DUAL and QUAD"
I think this is a material for a separate patch.
Here is Geert's comments:
"May I suggest the following:
spi: rspi: Fix bogus received byte in qspi_transfer_in()
When there are less than QSPI_BUFFER_SIZE remaining bytes to be received,
qspi_transfer_in() writes one bogus byte in the receive buffer, possibly
leading to a buffer overflow.
This can be reproduced by mounting, unmounting, and remounting a
jffs2-formatted device, causing lots of warnings like:
jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found
at 0x03b4: 0x1900 instead
Remove the bogus write to fix this.
It's also a good idea to add a Fixes tag:
Fixes: 3be09bec42a800d4 ("spi: rspi: supports 32bytes buffer for
DUAL and QUAD")
I don't see you adding this tag.
(the code was moved afterwards, but both the origin and the move were
integrated in v4.10-rc1)."
Signed-off-by: DongCV
Reviewed-by: Geert Uytterhoeven
[...]
MBR, Sergei
Re: [PATCH 1/3] power: reset: Add Renesas reset driver
Hi Chris,
On Mon, Feb 13, 2017 at 7:25 PM, Chris Brandt wrote:
> Some Renesas SoCs do not have a reset register and the only way to do a SW
> controlled reset is to use the watchdog timer. Additionally, since all the
> WDT timeout options are so quick, a system reset is about the only thing
> it's good for.
>
> Signed-off-by: Chris Brandt
Reviewed-by: Geert Uytterhoeven
> --- /dev/null
> +++ b/drivers/power/reset/renesas-reset.c
> @@ -0,0 +1,103 @@
> +/* Watchdog Timer Registers */
> +#define WTCSR 0
> +#define WTCNT 2
> +#define WRCSR 4
> +
> +static void __iomem *base;
> +
> +static int wdt_reset_handler(struct notifier_block *this,
> +unsigned long mode, void *cmd)
> +{
> + pr_debug("%s %lu\n", __func__, mode);
> +
> + /* Dummy read (must read WRCSR:WOVF at least once before clearing) */
> + readw(base + WRCSR);
> +
> + writew(0xA500, base + WRCSR); /* Clear WOVF */
> + writew(0x5A5F, base + WRCSR); /* Reset Enable */
> + writew(0x5A00, base + WTCNT); /* Counter to 00 */
> + writew(0xA578, base + WTCSR); /* Start timer */
Hardcoded register values? Please use #defines.
Yes, the 0xa5 and 0x5a are magic values, but the other bits aren't.
> +
> + /* Wait for WDT overflow */
> + while (1)
> + ;
This burns CPU, and thus power, albeit for a very short time.
Perhaps add an msleep() to the loop body?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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
[PATCH] spi: rspi: Fixes bogus received byte and replaces "n" by "len" in commit 3be09be
In qspi_transfer_in(), when receiving the last n (or len) bytes of data,
1 bogus byte was written in the receive buffer.
This code leads to a buffer overflow.
"jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b4:
0x1900 instead
jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40004:
0x000c instead"
The error message above happens when trying to mount, unmount, and remount a
jffs2-formatted device.
This patch removed the bogus write and for clearer to use, here replaced "n" by
"len" in
qspi_transfer_in() and qspi_transfer_out() to fixes: 3be09bec42a800d4
"spi: rspi: supports 32bytes buffer for DUAL and QUAD"
Here is Geert's comments:
"May I suggest the following:
spi: rspi: Fix bogus received byte in qspi_transfer_in()
When there are less than QSPI_BUFFER_SIZE remaining bytes to be received,
qspi_transfer_in() writes one bogus byte in the receive buffer, possibly
leading to a buffer overflow.
This can be reproduced by mounting, unmounting, and remounting a
jffs2-formatted device, causing lots of warnings like:
jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found
at 0x03b4: 0x1900 instead
Remove the bogus write to fix this.
It's also a good idea to add a Fixes tag:
Fixes: 3be09bec42a800d4 ("spi: rspi: supports 32bytes buffer for
DUAL and QUAD")
(the code was moved afterwards, but both the origin and the move were
integrated in v4.10-rc1)."
Signed-off-by: DongCV
Reviewed-by: Geert Uytterhoeven
---
drivers/spi/spi-rspi.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/spi/spi-rspi.c b/drivers/spi/spi-rspi.c
index 9daf500..bc3c868 100644
--- a/drivers/spi/spi-rspi.c
+++ b/drivers/spi/spi-rspi.c
@@ -808,7 +808,7 @@ static int qspi_transfer_out(struct rspi_data *rspi, struct
spi_transfer *xfer)
for (i = 0; i < len; i++)
rspi_write_data(rspi, *tx++);
} else {
- ret = rspi_pio_transfer(rspi, tx, NULL, n);
+ ret = rspi_pio_transfer(rspi, tx, NULL, len);
if (ret < 0)
return ret;
}
@@ -845,10 +845,9 @@ static int qspi_transfer_in(struct rspi_data *rspi, struct
spi_transfer *xfer)
for (i = 0; i < len; i++)
*rx++ = rspi_read_data(rspi);
} else {
- ret = rspi_pio_transfer(rspi, NULL, rx, n);
+ ret = rspi_pio_transfer(rspi, NULL, rx, len);
if (ret < 0)
return ret;
- *rx++ = ret;
}
n -= len;
}
--
1.9.1
Re: [PATCH 1/1] clk: renesas: cpg-mssr: r8a7795: add IMR clocks
On Mon, Feb 13, 2017 at 10:06 PM, Sergei Shtylyov
wrote:
> Add the IMR[0-3] clocks to the R8A7795 CPG/MSSR driver.
>
> Based on the original (and large) patch by Konstantin Kozhevnikov
> .
>
> Signed-off-by: Konstantin Kozhevnikov
>
> Signed-off-by: Sergei Shtylyov
Reviewed-by: Geert Uytterhoeven
I cannot verify the parent clocks for H3 ES1.x, but they match the parent clocks
for the VSPs.
Will queue in clk-renesas-for-v4.12.
> ---
> This patch is against the 'clk-next' branch of CLK group's 'linux.git' repo.
>
> drivers/clk/renesas/r8a7795-cpg-mssr.c |4
> 1 file changed, 4 insertions(+)
>
> Index: linux/drivers/clk/renesas/r8a7795-cpg-mssr.c
> ===
> --- linux.orig/drivers/clk/renesas/r8a7795-cpg-mssr.c
> +++ linux/drivers/clk/renesas/r8a7795-cpg-mssr.c
> @@ -208,6 +208,10 @@ static const struct mssr_mod_clk r8a7795
> DEF_MOD("vin0", 811, R8A7795_CLK_S2D1),
> DEF_MOD("etheravb", 812, R8A7795_CLK_S3D2),
> DEF_MOD("sata0", 815, R8A7795_CLK_S3D2),
> + DEF_MOD("imr3", 820, R8A7795_CLK_S2D1),
> + DEF_MOD("imr2", 821, R8A7795_CLK_S2D1),
> + DEF_MOD("imr1", 822, R8A7795_CLK_S2D1),
> + DEF_MOD("imr0", 823, R8A7795_CLK_S2D1),
> DEF_MOD("gpio7", 905, R8A7795_CLK_CP),
> DEF_MOD("gpio6", 906, R8A7795_CLK_CP),
> DEF_MOD("gpio5", 907, R8A7795_CLK_CP),
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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
Re: [PATCH 1/1] arm64: dts: r8a7795: add IMR-LX4 support
On Tue, Feb 14, 2017 at 10:02 PM, Sergei Shtylyov
wrote:
> Describe the IMR-LX4 devices in the R8A7795 device tree.
>
> Based on the original (and large) patch by Konstantin Kozhevnikov
> .
>
> Signed-off-by: Konstantin Kozhevnikov
>
> Signed-off-by: Sergei Shtylyov
Reviewed-by: Geert Uytterhoeven
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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
Re: [PATCH v3] spi: rspi: Fix bogus received byte in qspi_transfer_in()
Hi Dong,
Thanks for the update!
On Wed, Feb 15, 2017 at 8:33 AM, DongCV wrote:
> In qspi_transfer_in(), when receiving the last n (or len) bytes of data,
> n(or len) bogus bytes were written in the receive buffer,
1 bogus byte was written
And I think you should mention this is a buffer overflow, as this has security
implications (see also "Fixes" below).
> This code leads to the following error that related to jffs2 format
> when trying to mount, unmount, and remount a jffs2-formatted device.
> Remove the bogus write to fix this.
You forgot to mention that you replaced "n" by "len", to improve readability.
> "jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at
> 0x03b4: 0x1900 instead
> jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at
> 0x03b40004: 0x000c instead"
>
> Signed-off-by: DongCV
Reviewed-by: Geert Uytterhoeven
Please add
Fixes: 3be09bec42a800d4 ("spi: rspi: supports 32bytes buffer for
DUAL and QUAD")
so the stable team knows which stable kernel version this needs backporting to.
> ---
> drivers/spi/spi-rspi.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/spi/spi-rspi.c b/drivers/spi/spi-rspi.c
> index 9daf500..bc3c868 100644
> --- a/drivers/spi/spi-rspi.c
> +++ b/drivers/spi/spi-rspi.c
> @@ -808,7 +808,7 @@ static int qspi_transfer_out(struct rspi_data *rspi,
> struct spi_transfer *xfer)
> for (i = 0; i < len; i++)
> rspi_write_data(rspi, *tx++);
> } else {
> - ret = rspi_pio_transfer(rspi, tx, NULL, n);
> + ret = rspi_pio_transfer(rspi, tx, NULL, len);
> if (ret < 0)
> return ret;
> }
> @@ -845,10 +845,9 @@ static int qspi_transfer_in(struct rspi_data *rspi,
> struct spi_transfer *xfer)
> for (i = 0; i < len; i++)
> *rx++ = rspi_read_data(rspi);
> } else {
> - ret = rspi_pio_transfer(rspi, NULL, rx, n);
> + ret = rspi_pio_transfer(rspi, NULL, rx, len);
> if (ret < 0)
> return ret;
> - *rx++ = ret;
> }
> n -= len;
> }
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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
