Date   

Re: [PATCH 4.4.y-cip 37/83] mmc: tmio: enhance illegal sequence handling

Biju Das <biju.das@...>
 

HI Pavel,

Subject: Re: [PATCH 4.4.y-cip 37/83] mmc: tmio: enhance illegal sequence
handling

Hi!

commit 96e0b2ba00ee5dacb12bed6585145ce784ec9153 upstream.

An illegal sequence command error may occur if there is a stopbit or
cmd_index error as well as a CRC error. The correct course of action
is to re-enable IRQs

An illegal sequence data error may occur if there is a CRC or stopbit
error, or underrun. In this case set data->error correctly.

This is in preparation for enabling tuning support which relies on
differentiating between illegal sequence and other errors.
@@ -609,8 +612,6 @@ static void tmio_mmc_cmd_irq(struct
tmio_mmc_host *host,
goto out;
}

- host->cmd = NULL;
-
/* This controller is sicker than the PXA one. Not only do we need to
* drop the top 8 bits of the first response word, we also need to
* modify the order of the response for short response command
Is this removal intentional? I'm not sure from the changelog.
Yes, it is not clear from the changelog.

Regards,
Biju


Re: [PATCH 4.4.y-cip 33/83] mmc: tmio: add eMMC support

Biju Das <biju.das@...>
 

Hi Pavel,

Subject: Re: [PATCH 4.4.y-cip 33/83] mmc: tmio: add eMMC support

On Thu 2019-11-07 08:32:02, Biju Das wrote:
From: Wolfram Sang <wsa+renesas@...>

commit 0bc0b6e86524526c92a9409faea79d53db8e7e6e upstream.

We need to add R1 without CRC support, refactor the bus width routine
a little and extend a quirk check. To support "non-removable;" we need
a workaround which will be hopefully removed when reworking PM soon.
index addbc71..eafd92d 100644
--- a/drivers/mmc/host/tmio_mmc.h
+++ b/drivers/mmc/host/tmio_mmc.h
@@ -79,6 +79,9 @@
#define CLK_CTL_DIV_MASK 0xff
#define CLK_CTL_SCLKEN BIT(8)

+#define CARD_OPT_WIDTH8 BIT(13)
+#define CARD_OPT_WIDTH BIT(15)
WIDTH_1 and WIDTH_8 would be better names, for consistency with rest of
code. "WIDTH" without number is quite confusing.
I agree, it is little bit confusing. May be we should fixing this Mainline first and then backport to cip kernel.

Cheers,
Biju

+ /* Some hardware cannot perform 2 byte requests in 4/8 bit mode
*/
+ if (host->mmc->ios.bus_width == MMC_BUS_WIDTH_4 ||
+ host->mmc->ios.bus_width == MMC_BUS_WIDTH_8) {
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany


Re: [isar-cip-core PATCH 0/4] Add rzg2m support

Quirin Gylstorff
 

On 11/6/19 5:19 PM, Chris Paterson wrote:
Hello,

From: Gylstorff Quirin <quirin.gylstorff@...>
Sent: 06 November 2019 14:37



On 11/6/19 3:12 PM, Jan Kiszka wrote:
Nice! Beyond building and actually deploying this, what else would be
missing to hook up the result with a board in the lab?
We would need to change the configuration of the LAVA jobs in [1] to
use the tarballs generated by the build.
Yes, at the moment they are painfully hardcoded. I plan to at least have them point to the 'latest' CIP Core.
However for your use case, you'll want to test the binaries you just created.
I guess we should decide on whether we want to re-use linux-cip-ci for CIP Core testing, or just end up creating a separate version for CIP Core.
Linux-cip-ci could easily be modified to support both CIP Linux & Core testing (we could just add another version of submit_tests.sh for CIP Core), and I think this would be easier to maintain down the line.
Is this something you'd like to have a go at?
I'm happy to do it, but I may struggle to find time in the next few weeks.
I have some other projects in the next weeks. Afterwards if time permits I will try to look into it. We can sync each other when one of use can
start with it.


Kind regards,
Quirn


Re: [PATCH 4.4.y-cip 31/83] mmc: host: sh_mobile_sdhi: move card_busy from tmio to sdhi

Biju Das <biju.das@...>
 

Hi Pavel,

Subject: Re: [PATCH 4.4.y-cip 31/83] mmc: host: sh_mobile_sdhi: move
card_busy from tmio to sdhi

On Thu 2019-11-07 08:32:00, Biju Das wrote:
From: Wolfram Sang <wsa+renesas@...>

commit 6a4679f312357ac7c74c0e1b996efdd1d0a612fa upstream.

card_busy is only used/tested on SDHI for R-Car Gen2 and later.
Move it to the SDHI driver, so we can then activate it conditionally
depending on the SDHI type.

@@ -217,6 +217,13 @@ static void sh_mobile_sdhi_clk_disable(struct
tmio_mmc_host *host)
clk_disable_unprepare(priv->clk);
}

+static int sh_mobile_sdhi_card_busy(struct mmc_host *mmc) {
+ struct tmio_mmc_host *host = mmc_priv(mmc);
+
+ return !(sd_ctrl_read16_and_16_as_32(host, CTL_STATUS) &
+TMIO_STAT_DAT0); }
+
Bool would make sense here, if the change is practical without changing too
many interfaces... (and unless interface is something like 0/1/-ERRNO...)
Yes I agree with you. Last statement is clearly returns a Boolean value.

Regards,
Biju


Re: [PATCH 4.4.y-cip 26/83] mmc: tmio/sdhi: introduce flag for RCar 2+ specific features

Biju Das <biju.das@...>
 

Hi Pavel,
[>]
Subject: Re: [PATCH 4.4.y-cip 26/83] mmc: tmio/sdhi: introduce flag for RCar
2+ specific features

Hi!

@@ -66,8 +66,8 @@
*/
#define TMIO_MMC_SDIO_IRQ (1 << 2)

-/* Some controllers don't need to wait 10ms for clock changes */
-#define TMIO_MMC_FAST_CLK_CHG (1 << 3)
+/* Some features are only available or tested on RCar Gen2 or later */
+#define TMIO_MMC_MIN_RCAR2 (1 << 3)
I'd add a comment here that gen2 allows fast clock changes ... and whatever
other features the driver is actually using.
Yes, I agree it should have done in the mainline first.

Regards,
Biju


Re: [PATCH 4.4.y-cip 18/83] mmc: tmio: remove now unneeded seperate irq handlers

Biju Das <biju.das@...>
 

Hi Pavel,

Subject: Re: [PATCH 4.4.y-cip 18/83] mmc: tmio: remove now unneeded
seperate irq handlers

Hi!

We removed installation of separate handlers previously, so we can
also remove the separate handlers.
-
-irqreturn_t tmio_mmc_sdio_irq(int irq, void *devid)
+static bool tmio_mmc_sdio_irq(int irq, void *devid)
{
struct tmio_mmc_host *host = devid;
struct mmc_host *mmc = host->mmc;
@@ -720,7 +696,7 @@ irqreturn_t tmio_mmc_sdio_irq(int irq, void *devid)
unsigned int sdio_status;

if (!(pdata->flags & TMIO_MMC_SDIO_IRQ))
- return IRQ_NONE;
+ return false;

status = sd_ctrl_read16(host, CTL_SDIO_STATUS);
ireg = status & TMIO_SDIO_MASK_ALL & ~host->sdcard_irq_mask;
@@
-734,9 +710,8 @@ irqreturn_t tmio_mmc_sdio_irq(int irq, void *devid)
if (mmc->caps & MMC_CAP_SDIO_IRQ && ireg &
TMIO_SDIO_STAT_IOIRQ)
mmc_signal_sdio_irq(mmc);

- return IRQ_RETVAL(ireg);
+ return ireg;
}
I'm not a great fan of function conversion to boolean here. With irqreturn_t it
is clear what the values area; it is more ambiguous with the bool.
@@ -751,7 +726,10 @@ irqreturn_t tmio_mmc_irq(int irq, void *devid)
if (__tmio_mmc_sdcard_irq(host, ireg, status))
return IRQ_HANDLED;

- return tmio_mmc_sdio_irq(irq, devid);
+ if (tmio_mmc_sdio_irq(irq, devid))
+ return IRQ_HANDLED;
+
+ return IRQ_NONE;
}
And it is converted right back into irqreturn_t here, anyway...
Yes, it looks ok now.

Regards,
Biju


Re: [PATCH 4.4.y-cip 15/83] mmc: tmio: stop clock when 0Hz is requested

Biju Das <biju.das@...>
 

Hi Pavel,


Subject: Re: [PATCH 4.4.y-cip 15/83] mmc: tmio: stop clock when 0Hz is
requested

Hi!

From: Wolfram Sang <wsa+renesas@...>

commit 148634d24d4a7dc82a49efcf1a215e1d0695f62c upstream.

Setting frequency to 0 is not enough, the clock explicitly has to be
disabled. Otherwise voltage switching (which needs SDCLK to be quiet)
fails for various cards.

Because we now do the 'new_clock == 0' check right at the beginning,
the indentation level of the rest of the code can be decreased a
little.
{
u32 clk = 0, clock;
clk = 0 initialization is never used.

+ for (clk = 0x80000080; new_clock >= (clock << 1); clk >>= 1)
+ clock <<= 1;
This is black magic. Where does 0x80000080 come from? Would it be possible
to get some comment/explanation?

+ /* 1/1 clock is option */
+ if ((host->pdata->flags & TMIO_MMC_CLK_ACTUAL) && ((clk >> 22)
& 0x1))
+ clk |= 0xff;

if (host->set_clk_div)
host->set_clk_div(host->pdev, (clk >> 22) & 1);
What does bit 22 in clk mean? Should it go to temporary variable with
explanation? (Also it is & 1 in one operation and & 0x1 in other operation).

It seems that low bits of clk variable are used as an actual clock divider, with
high bit doing something else. That is quite confusing...
Yes I agree this could have been done with Macro. Not sure, may be because of HALA [SD Host/Ancillary Product License Agreement],
it is defined as magic number at that time.

Regards,
Biju


[PATCH] gpiolib: Support 'gpio-reserved-ranges' property

johnsonch.chen@moxa.com
 

Hi Fabrizio Castro,

 

I find a kernel panic issue by gpio when I upgrade kernel to 4.4.182-cip34 with Freescale LS1021A (gpiochip is mpc8xxx series):

 

Kernel log shows “Unable to handle kernel NULL pointer dereference at virtual address 000000b4.”

 

Subsequently, I find the commit baff4777cdb80256cd24dede2a3d0af761356307 (gpiolib: Support 'gpio-reserved-ranges' property) could result in kernel panic because the code “*np= chip->dev->of_node” is executed when chip->dev is NULL:

 

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c

index 5fe34a9df3e6..ec642bf1d976 100644

--- a/drivers/gpio/gpiolib-of.c

+++ b/drivers/gpio/gpiolib-of.c

 

+static void of_gpiochip_init_valid_mask(struct gpio_chip *chip)

+{

+      int len, i;

+      u32 start, count;

+      struct device_node *np = chip->dev->of_node;

+

+      len = of_property_count_u32_elems(np,  "gpio-reserved-ranges");

+      if (len < 0 || len % 2 != 0)

+              return;

+

+      for (i = 0; i < len; i += 2) {

+              of_property_read_u32_index(np, "gpio-reserved-ranges",

+                                         i, &start);

+              of_property_read_u32_index(np, "gpio-reserved-ranges",

+                                         i + 1, &count);

+              if (start >= chip->ngpio || start + count >= chip->ngpio)

+                      continue;

+

+              bitmap_clear(chip->valid_mask, start, count);

+      }

+};

+

 

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c

index adb474089733..487a86495163 100644

--- a/drivers/gpio/gpiolib.c

+++ b/drivers/gpio/gpiolib.c

@@ -292,6 +292,43 @@ static unsigned long *gpiochip_allocate_mask(struct gpio_chip *chip)

      return p;

}

 

+static int gpiochip_init_valid_mask(struct gpio_chip *gpiochip)

+{

+#ifdef CONFIG_OF_GPIO

+      int size;

+      struct device_node *np = gpiochip->dev->of_node;

+

+      size = of_property_count_u32_elems(np,  "gpio-reserved-ranges");

+      if (size > 0 && size % 2 == 0)

+              gpiochip->need_valid_mask = true;

+#endif

+

+      if (!gpiochip->need_valid_mask)

+              return 0;

+

+      gpiochip->valid_mask = gpiochip_allocate_mask(gpiochip);

+      if (!gpiochip->valid_mask)

+              return -ENOMEM;

+

+      return 0;

+}

+

 

Could you help me understand why we need to replaced chip->of_node with chip->dev->of_node?

 

The following two items I try have no kernel panic:

 

1. For gpio drivers:

 

It’s fine and no kernel panic issue if probe(struct platform_device *pdev) in gpio drivers executes the following codes:

 

struct cpio_chip *gc;

gc->dev = &pdev->dev; //Put platform’s dev to gpiochip’s dev, and of_node info in pdev->dev->of_node also put in gpiochip structure.

 

But some gpio drivers doesn’t do this, such as gpio-mpc8xxx.c (mpc8xxx series), and result in kernel panic when use this commit.

 

2. For gpiolib:

 

//set the following code in gpiolib.c and gpiolib-of.c

struct device_node *np = gpiochip->of_node; 

 

Device node of platform will put in gpio chip when of_mm_gpiochip_add() is executed:

 

int of_mm_gpiochip_add(struct device_node *np,

                       struct of_mm_gpio_chip *mm_gc)

{

        int ret = -ENOMEM;

        struct gpio_chip *gc = &mm_gc->gc;

..

        mm_gc->gc.of_node = np;

}

 

It’s seems chip->of_node is good.

 

Thanks,

Johnson


Re: [PATCH 4.4.y-cip 02/83] mmc: tmio_dma: remove debug messages with little information

Pavel Machek
 

Hi!

From: Wolfram Sang <wsa+renesas@...>

commit 254d1456560fa42d4ca901c9b9308e87c951fee1 upstream.

When compiling the driver with CONFIG_MMC_DEBUG set, I got build
warnings. They have been 'fixed' meanwhile. However, because these
debug messages look random anyhow (some duplicate information printed
etc), let's just drop them and rather re-add something consistent if
that should ever be needed.
Its too late to change that now, but 1/ fixes messages and then 2/ just
deletes them. Single patch would do.
Are you ok to merge this patches while applying? or Do you want me to send another series fixing this?
Please let me know.
I believe this is minor detail, and we can use the patches as-is.

See my reply to Chris for thoughts about rest of the series.

Best regards,
Pavel
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany


Re: [PATCH 4.4.y-cip 00/83] Add RZ/G1C SD/eMMC support

Pavel Machek
 

Hi!

Thank you for your work on reviewing this series.
(For the record, I'm now done with the review... with exception of
renames&modifications in 66-69).

Looking at some of your comments, it looks like there are lots of issues to do with the original patches that were upstreamed, rather than changes that were made specifically for the CIP Kernel.

My understanding is that the CIP project follows an upstream first policy, i.e. code must all be upstreamed before being backported to the CIP kernel(s).

For the changes that you're pointing out, should we be upstreaming the relevant fixes, waiting for them to be released upstream, then re-submitting the whole patch series? Or should we be modifying the patches that we're submitting to the CIP kernel directly?
It really depends on case by case basis -- on severity of the problem.

I can take explanations, maybe I'm just misunderstanding the code.

Or perhaps code is really right, but unclear/needs additional comment,
etc... In such case I guess we can take patch as is, and just queue
cleanups/comments to the mainline.

But patch 34/ introduces data corrupting bug, which is only fixed at
the end of the series. I'd prefer having patches changed / reshuffled
/ something so that it does not corrupt someone's data during bisect.

Plus we need to figure out how to review the renames.

If the former, I'm not actually sure this will always be technically possible.
If the latter, do you want Biju to update each of the patches in his series? Or submit the new cip-specific changes in a new patch?
Not sure really.

One possible way forward would be to submit 1..33/ as separate series,
then figure out 34.. first rename, then figure out the rest.

Best regards,

Pavel
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany


Re: [PATCH 4.4.y-cip 74/83] mmc: tmio-mmc: fix bad pointer math

Pavel Machek
 

Hi!

From: Chris Brandt <chris.brandt@...>

commit b5dd7985e8d3357ff9537c0be231190ab1a131fe upstream.

commit 9c284c41c0886f09e75c323a16278b6d353b0b4a upstream.

The existing code gives an incorrect pointer value.
The buffer pointer 'buf' was of type unsigned short *, and 'count' was a
number in bytes. A cast of buf should have been used.

However, instead of casting, just change the code to use u32
pointers.
Yep, I see, nasty; silent data corruption. Could this be merged to the
patch that introduces the problem, or the original patch somehow
modified that it will not corrupt data ... for example during bisection?

+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -446,30 +446,29 @@ static void tmio_mmc_transfer_data(struct tmio_mmc_host *host,
* Transfer the data
*/
if (host->pdata->flags & TMIO_MMC_32BIT_DATA_PORT) {
- u8 data[4] = { };
+ u32 data = 0;
+ u32 *buf32 = (u32 *)buf;

if (is_read)
- sd_ctrl_read32_rep(host, CTL_SD_DATA_PORT, (u32 *)buf,
+ sd_ctrl_read32_rep(host, CTL_SD_DATA_PORT, buf32,
count >> 2);
else
- sd_ctrl_write32_rep(host, CTL_SD_DATA_PORT, (u32 *)buf,
+ sd_ctrl_write32_rep(host, CTL_SD_DATA_PORT, buf32,
count >> 2);

/* if count was multiple of 4 */
if (!(count & 0x3))
return;

- buf8 = (u8 *)(buf + (count >> 2));
+ buf32 += count >> 2;
count %= 4;
Can we do count &= 0x3, to keep style of if () above? Actually you can
do

count &= 0x3;
if (!count) return;

Plus, there's code handling 16 bit case just below this; it is
different in style, and by using

*buf8 = sd_ctrl_read16(host, CTL_SD_DATA_PORT) & 0xff;

instead of read16_rep(), it is buggy on big endian (as comment
says). Perhaps synchronizing to similar, non-buggy version would be
good?

Best regards,
Pavel
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany


Re: [PATCH 4.4.y-cip 00/83] Add RZ/G1C SD/eMMC support

Chris Paterson
 

Hello Pavel,

From: Pavel Machek <pavel@...>
Sent: 07 November 2019 21:02

Hi!

This patch series add SD/eMMC support support for RZ/G1C sbc.

RZ/G1C eMMC IP is different from other RZ/G1 SoC's. It is having an
internal DMA for data transfer which is similar to R-Car Gen3.

Support for internal DMAC is added in 4.14 kernel and support for
RZ/G1C added on 4.20 kernel.

Backported the relevent patches to linux-4.4.y-cip.

This patch series is based on linux-4.4.y-cip and all the patches
in this series are cherry-picked from linux rc tree.
Thanks for the series. I'm currently reviewing it, I have some
comments but nothing really bad so far. I'm now around the middle.
Thank you for your work on reviewing this series.

Looking at some of your comments, it looks like there are lots of issues to do with the original patches that were upstreamed, rather than changes that were made specifically for the CIP Kernel.

My understanding is that the CIP project follows an upstream first policy, i.e. code must all be upstreamed before being backported to the CIP kernel(s).

For the changes that you're pointing out, should we be upstreaming the relevant fixes, waiting for them to be released upstream, then re-submitting the whole patch series? Or should we be modifying the patches that we're submitting to the CIP kernel directly?

If the former, I'm not actually sure this will always be technically possible.
If the latter, do you want Biju to update each of the patches in his series? Or submit the new cip-specific changes in a new patch?

Kind regards, Chris


Best regards,
                                                              Pavel
--
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany


Re: [PATCH 4.4.y-cip 71/83] mmc: tmio, renesas-sdhi: add max_{segs, blk_count} to tmio_mmc_data

Pavel Machek
 

Hi!

commit 603aa14d3daaa7073bab4c472025c4963030e0cc upstream.

Allow TMIO and SDHI driver implementations to provide values for
max_segs and max_blk_count.

A follow-up patch will set these values for Renesas Gen3 SoCs
the using an SDHI driver.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...>
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -1224,10 +1224,10 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host,

mmc->caps |= MMC_CAP_4_BIT_DATA | pdata->capabilities;
mmc->caps2 |= pdata->capabilities2;
- mmc->max_segs = 32;
+ mmc->max_segs = pdata->max_segs ? : 32;
mmc->max_blk_size = 512;
- mmc->max_blk_count = (PAGE_CACHE_SIZE / mmc->max_blk_size) *
- mmc->max_segs;
+ mmc->max_blk_count = pdata->max_blk_count ? :
+ (PAGE_SIZE / mmc->max_blk_size) * mmc->max_segs;
mmc->max_req_size = mmc->max_blk_size * mmc->max_blk_count;
mmc->max_seg_size = mmc->max_req_size;
This also changes PAGE_CACHE_SIZE -> PAGE_SIZE. I guess that's
okay/bugfix, as PAGE_CACHE_SIZE probably is equal to PAGE_SIZE in all
relevant configurations, but...

Best regards,
Pavel
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany


Re: [PATCH 4.4.y-cip 69/83] mmc: renesas-sdhi: make renesas_sdhi_sys_dmac main module file

Pavel Machek
 

On Thu 2019-11-07 08:32:38, Biju Das wrote:
From: Simon Horman <horms+renesas@...>

commit 9d08428afb722fedaea699a32aaf603a8f1ebd5a upstream.

Make renesas_sdhi_sys_dmac.c a top-level module file that makes use of
library code supplied by renesas_sdhi_core.c

This is in order to facilitate adding other variants of SDHI;
in particular SDHI using different DMA controllers.
Patches 66 to 69 do a big rename while changing the code in
question. That makes them hard / tricky to review. I should review
that again, either now or in next version of the patch.

-static int renesas_sdhi_probe(struct platform_device *pdev)
+int renesas_sdhi_probe(struct platform_device *pdev,
+ const struct tmio_mmc_dma_ops *dma_ops)
{
- const struct renesas_sdhi_of_data *of_data = of_device_get_match_data(&pdev->dev);
+ const struct renesas_sdhi_of_data *of_data = of_device_get_match_data( &pdev->dev);
struct renesas_sdhi *priv;
Prototype changed here. The space was added in the wrong place.

@@ -46,8 +119,6 @@ static void renesas_sdhi_sys_dmac_dma_callback(void *arg)
{
struct tmio_mmc_host *host = arg;

- wait_for_completion(&host->dma_dataend);
-
spin_lock_irq(&host->lock);

if (!host->data)
@@ -62,6 +133,11 @@ static void renesas_sdhi_sys_dmac_dma_callback(void *arg)
host->sg_ptr, host->sg_len,
DMA_TO_DEVICE);

+ spin_unlock_irq(&host->lock);
+
+ wait_for_completion(&host->dma_dataend);
+
+ spin_lock_irq(&host->lock);
tmio_mmc_do_data_irq(host);
out:
spin_unlock_irq(&host->lock);
But this is worse: wait_for_completion is moving around, along with
locking changes. Change is not documented and is not present in
upstream version of the commit.

Best regards,
Pavel
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany


Re: [PATCH 4.4.y-cip 66/83] mmc: tmio: rename tmio_mmc_{pio => core}.c

Pavel Machek
 

Hi!

commit 426e95d1766bad20e59f219af64fdd50c39bcfee upstream.

Rename tmio_mmc_pio.c to tmio_mmc_core.c to more accurately reflect its
function: to provide core code for the tmio-mmc and sh-mobole-sdhi drivers.

Signed-off-by: Simon Horman <horms+renesas@...>
Acked-by: Arnd Bergmann <arnd@...>
Reviewed-by: Wolfram Sang <wsa+renesas@...>
Signed-off-by: Ulf Hansson <ulf.hansson@...>
Signed-off-by: Biju Das <biju.das@...>
---
drivers/mmc/host/Makefile | 1 -
drivers/mmc/host/tmio_mmc_core.c | 1394 ++++++++++++++++++++++++++++++++++++++
drivers/mmc/host/tmio_mmc_pio.c | 1394 --------------------------------------
This patch creates tmio_mmc_core.c ...

+++ b/drivers/mmc/host/Makefile
@@ -36,7 +36,6 @@ obj-$(CONFIG_MMC_S3C) += s3cmci.o
obj-$(CONFIG_MMC_SDRICOH_CS) += sdricoh_cs.o
obj-$(CONFIG_MMC_TMIO) += tmio_mmc.o
obj-$(CONFIG_MMC_TMIO_CORE) += tmio_mmc_core.o
-tmio_mmc_core-y := tmio_mmc_pio.o
obj-$(CONFIG_MMC_SDHI) += sh_mobile_sdhi.o tmio_mmc_dma.o
obj-$(CONFIG_MMC_CB710) += cb710-mmc.o
obj-$(CONFIG_MMC_VIA_SDMMC) += via-sdmmc.o
... which is already present in the Makefile before the patch. That
can't be right.

Best regards,
Pavel
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany


Re: [PATCH 4.4.y-cip 63/83] mmc: tmio: always get number of taps

Pavel Machek
 

Hi!

commit 43b0b361b0170030603cf76f70b099f3323edcf3 upstream.

Current code gets number of taps only once and keeps the value. This is
not correct, we need to obtain it every time before executing tuning,
so remove the outer if-block.
I'd somehow assume number of tuning parameters would be fixed... Does
the hardware really change it? Out of curiosity, why is it doing so?

Best regards,
Pavel
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany


Re: [PATCH 4.4.y-cip 56/83] mmc: host: tmio: refactor calls to sdio irq

Pavel Machek
 

On Thu 2019-11-07 08:32:25, Biju Das wrote:
From: Wolfram Sang <wsa+renesas@...>

commit e4f38eb18aedd098b3019e82df07f583a5cbcc58 upstream.

tmio_mmc_sdio_irq() is not used as a seperate irq handler anymore, so we
can make it similar to the other irq helper functions, namely:

* only give the host as argument function which is what it really needs
* prefix function name with __
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -709,7 +709,7 @@ static bool __tmio_mmc_sdcard_irq(struct tmio_mmc_host *host,
return false;
}

-static bool tmio_mmc_sdio_irq(int irq, void *devid)
+static bool __tmio_mmc_sdio_irq(int irq, void *devid)
{
struct tmio_mmc_host *host = devid;
struct mmc_host *mmc = host->mmc;

In mainline, the commit removes the argument, as it says in the
changelog. Not here in the backport. Why the difference?

Best regards,
Pavel
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany


Re: [PATCH 4.4.y-cip 41/83] mmc: tmio: Add tuning support

Pavel Machek
 

Hi!

commit 4f11997773b6b452b5a0d620c5ac5050e75c227e upstream.

Add tuning support for use with SDR104 mode
+ /* Tuning values: 1 for success, 0 for failure */
+ DECLARE_BITMAP(taps, BITS_PER_BYTE * sizeof(long));
I don't see why tuning should depend on register size (thus
sizeof(long)) of the host CPU. If 32 is enough, put 32 there... if
not, just use 64?

+ if (host->tap_num * 2 >= sizeof(host->taps) * BITS_PER_BYTE) {
+ dev_warn_once(&host->pdev->dev,
+ "Too many taps, skipping tuning. Please consider updating size of taps field of tmio_mmc_host\n");
+ goto out;
+ }
Or perhaps introduce a #define MAX_TAPS; it will be cleaner than
checking bit sizes like this.

Best regards,
Pavel

--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany


Re: [PATCH 4.4.y-cip 40/83] mmc: core: Add helper to see if a host can be retuned

Pavel Machek
 

On Thu 2019-11-07 08:32:09, Biju Das wrote:
From: Simon Horman <horms+renesas@...>

commit c820af5f18ec248b3cb61a9a9ce47ef0f2e9ec63 upstream.

This is in preparation for restoring saved tuning parameters
when resuming the TMIO driver.
@@ -515,4 +515,9 @@ static inline void mmc_retune_recheck(struct mmc_host *host)
host->retune_now = 1;
}

+static inline bool mmc_can_retune(struct mmc_host *host)
+{
+ return host->can_retune == 1;
+}
+
Probably not worth changing any more, but it would be easier to review
if it was merged with the first user.

Best regards,
Pavel

--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany


Re: [PATCH 4.4.y-cip 37/83] mmc: tmio: enhance illegal sequence handling

Pavel Machek
 

Hi!

commit 96e0b2ba00ee5dacb12bed6585145ce784ec9153 upstream.

An illegal sequence command error may occur if there is a stopbit or
cmd_index error as well as a CRC error. The correct course of action
is to re-enable IRQs

An illegal sequence data error may occur if there is a CRC or stopbit
error, or underrun. In this case set data->error correctly.

This is in preparation for enabling tuning support which relies on
differentiating between illegal sequence and other errors.
@@ -609,8 +612,6 @@ static void tmio_mmc_cmd_irq(struct tmio_mmc_host *host,
goto out;
}

- host->cmd = NULL;
-
/* This controller is sicker than the PXA one. Not only do we need to
* drop the top 8 bits of the first response word, we also need to
* modify the order of the response for short response command
Is this removal intentional? I'm not sure from the changelog.

Best regards,
Pavel
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

6421 - 6440 of 10124