Re: [PATCH 3/3] arm64: dts: rockchip: fix RockPro64 sdmmc settings

[Shawn Lin]

On 2019/10/11 22:16, Soeren Moch wrote:

On 11.10.19 15:00, Robin Murphy wrote:

On 11/10/2019 12:40, Soeren Moch wrote:

On 11.10.19 10:22, Jonas Karlman wrote:

On 2019-10-04 19:24, Sören Moch wrote:

On 04.10.19 17:33, Shawn Lin wrote:

On 2019/10/4 22:20, Robin Murphy wrote:

On 04/10/2019 04:39, Soeren Moch wrote:

On 04.10.19 04:13, Shawn Lin wrote:

On 2019/10/4 8:53, Soeren Moch wrote:

On 04.10.19 02:01, Robin Murphy wrote:

On 2019-10-03 10:50 pm, Soeren Moch wrote:

According to the RockPro64 schematic [1] the rk3399 sdmmc
controller is
connected to a microSD (TF card) slot, which cannot be
switched to
1.8V.

Really? AFAICS the SDMMC0 wiring looks pretty much identical
to the
NanoPC-T4 schematic (it's the same reference design, after all),
and I
know that board can happily drive a UHS-I microSD card with 1.8v
I/Os,
because mine's doing so right now.

Robin.

OK, the RockPro64 does not allow a card reset (power cycle) since
VCC3V0_SD is directly connected to VCC3V3_SYS (via R89555), the
SDMMC0_PWH_H signal is not connected. So the card fails to
identify
itself after suspend or reboot when switched to 1.8V operation.

Ah, thanks for clarifying - I did overlook the subtlety that U12 and
friends have "NC" as alternative part numbers, even though they
aren't actually marked as DNP. So it's still not so much "cannot be
switched" as "switching can lead to other problems".


I believe we addressed this issue long time ago, please check:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6a11fc47f175c8d87018e89cb58e2d36c66534cb




Thanks for the pointer.
In this case I guess I should use following patch instead:

--- rk3399-rockpro64.dts.bak    2019-10-03 22:14:00.067745799 +0200
+++ rk3399-rockpro64.dts    2019-10-04 00:02:50.047892366 +0200
@@ -619,6 +619,8 @@
    max-frequency = <15000>;
    pinctrl-names = "default";
    pinctrl-0 = <_clk _cmd _bus4>;
+    sd-uhs-sdr104;
+    vqmmc-supply = <_sdio>;
    status = "okay";
    };
When I do so, the sd card is detected as SDR104, but a reboot
hangs:

Boot1: 2018-06-26, version: 1.14
CPUId = 0x0
ChipType = 0x10, 286
Spi_ChipId = c84018
no find rkpartition
SpiBootInit:
mmc: ERROR: SDHCI ERR:cmd:0x102,stat:0x18000
mmc: ERROR: Card did not respond to voltage select!
emmc reinit
mmc: ERROR: SDHCI ERR:cmd:0x102,stat:0x18000
mmc: ERROR: Card did not respond to voltage select!
emmc reinit
mmc: ERROR: SDHCI ERR:cmd:0x102,stat:0x18000
mmc: ERROR: Card did not respond to voltage select!
SdmmcInit=2 1
mmc0:cmd5,32
mmc0:cmd7,32
mmc0:cmd5,32
mmc0:cmd7,32
mmc0:cmd5,32
mmc0:cmd7,32
SdmmcInit=0 1

So I guess I should use a different miniloader for this reboot to
work!?
Or what else could be wrong here?

Hmm, I guess this is "the Tinkerboard problem" again - the patch
above would be OK if we could get as far as the kernel, but can't
help if the

I didn't realize that SD was used as boot medium for RockPro64, but I
did patch the vendor tree to solve the issue for Tinkerboard, see
https://github.com/rockchip-linux/kernel/commit/a4ccde21f5a9f04f996fb02479cb9f16d3dc8dc0



My initial plan was to patching upstream kernel by adding
->shutdown,but
never finish it.


offending card is itself the boot medium. There was a proposal here:

https://patchwork.kernel.org/patch/10817217/

This RFC also looks good to me, but seems it needs volunteers
to push it again.

Oh, I think this is a totally wrong way.

While this might work for some cards, setting the controller's i/o
voltage to 3.3V while leaving the card at 1.8V configuration is
totally
against the specification, can lead to all kinds of strange behaviour
and even cause hardware damage. It also would actively defend the
purpose of the above mentioned patch (6a11fc4) where the kernel
guesses
the i/o voltage from the card configuration and switches the
controller
accordingly. We would end up with a 1.8V card and controller
configuration and a regulator voltage of 3.3V. This would only work
with
good luck. Even if the kernel driver would switch the regulator
back to
1.8V in this case, the voltage mismatch remains in the bootloader when
this card contains the boot image.

The only sane way I see to handle this is implementing the same
workaround (mode guessing) also in the bootloader (rockchip miniloader
and u-boot SPL since both bootloader chains are supported for this
board).

Or maybe I miss something?

Thanks for your input, I have made a new series [1] with a similar
approach but is limited to dw_mmc-rockchip
and only changes the regulator at power_off after the regulator has
been disabled (the vqmmc regulator in affected devices is always-on).

Thanks for your work on this. Unfortunately I still think that this is
the wrong approach.
I see several problems in your patch series:
- You introduced GPIO0_PA1 as regulator enable for RockPro64.
Unfortunately Pine64 decided to disable this reg

Re: [PATCH 3/3] arm64: dts: rockchip: fix RockPro64 sdmmc settings【请注意，邮件由linux-rockchip-bounces+shawn.lin=rock-chips....@lists.infradead.org代发】

[Shawn Lin]

On 2019/10/4 22:20, Robin Murphy wrote:

On 04/10/2019 04:39, Soeren Moch wrote:



On 04.10.19 04:13, Shawn Lin wrote:

On 2019/10/4 8:53, Soeren Moch wrote:



On 04.10.19 02:01, Robin Murphy wrote:

On 2019-10-03 10:50 pm, Soeren Moch wrote:

According to the RockPro64 schematic [1] the rk3399 sdmmc
controller is
connected to a microSD (TF card) slot, which cannot be switched to
1.8V.


Really? AFAICS the SDMMC0 wiring looks pretty much identical to the
NanoPC-T4 schematic (it's the same reference design, after all), and I
know that board can happily drive a UHS-I microSD card with 1.8v I/Os,
because mine's doing so right now.

Robin.

OK, the RockPro64 does not allow a card reset (power cycle) since
VCC3V0_SD is directly connected to VCC3V3_SYS (via R89555), the
SDMMC0_PWH_H signal is not connected. So the card fails to identify
itself after suspend or reboot when switched to 1.8V operation.


Ah, thanks for clarifying - I did overlook the subtlety that U12 and 
friends have "NC" as alternative part numbers, even though they aren't 
actually marked as DNP. So it's still not so much "cannot be switched" 
as "switching can lead to other problems".




I believe we addressed this issue long time ago, please check:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6a11fc47f175c8d87018e89cb58e2d36c66534cb 




Thanks for the pointer.
In this case I guess I should use following patch instead:

--- rk3399-rockpro64.dts.bak    2019-10-03 22:14:00.067745799 +0200
+++ rk3399-rockpro64.dts    2019-10-04 00:02:50.047892366 +0200
@@ -619,6 +619,8 @@
  max-frequency = <15000>;
  pinctrl-names = "default";
  pinctrl-0 = <_clk _cmd _bus4>;
+    sd-uhs-sdr104;
+    vqmmc-supply = <_sdio>;
  status = "okay";
  };
When I do so, the sd card is detected as SDR104, but a reboot hangs:

Boot1: 2018-06-26, version: 1.14
CPUId = 0x0
ChipType = 0x10, 286
Spi_ChipId = c84018
no find rkpartition
SpiBootInit:
mmc: ERROR: SDHCI ERR:cmd:0x102,stat:0x18000
mmc: ERROR: Card did not respond to voltage select!
emmc reinit
mmc: ERROR: SDHCI ERR:cmd:0x102,stat:0x18000
mmc: ERROR: Card did not respond to voltage select!
emmc reinit
mmc: ERROR: SDHCI ERR:cmd:0x102,stat:0x18000
mmc: ERROR: Card did not respond to voltage select!
SdmmcInit=2 1
mmc0:cmd5,32
mmc0:cmd7,32
mmc0:cmd5,32
mmc0:cmd7,32
mmc0:cmd5,32
mmc0:cmd7,32
SdmmcInit=0 1

So I guess I should use a different miniloader for this reboot to work!?
Or what else could be wrong here?


Hmm, I guess this is "the Tinkerboard problem" again - the patch above 
would be OK if we could get as far as the kernel, but can't help if the 


I didn't realize that SD was used as boot medium for RockPro64, but I
did patch the vendor tree to solve the issue for Tinkerboard, see
https://github.com/rockchip-linux/kernel/commit/a4ccde21f5a9f04f996fb02479cb9f16d3dc8dc0

My initial plan was to patching upstream kernel by adding ->shutdown,but
never finish it.


offending card is itself the boot medium. There was a proposal here:

https://patchwork.kernel.org/patch/10817217/


This RFC also looks good to me, but seems it needs volunteers
to push it again.



although I'm not sure what if any progress has been made since then.

Robin.






--
Best Regards
Shawn Lin

Re: [PATCH 3/3] arm64: dts: rockchip: fix RockPro64 sdmmc settings【请注意，邮件由linux-rockchip-bounces+shawn.lin=rock-chips....@lists.infradead.org代发】

[Shawn Lin]

On 2019/10/4 8:53, Soeren Moch wrote:



On 04.10.19 02:01, Robin Murphy wrote:

On 2019-10-03 10:50 pm, Soeren Moch wrote:

According to the RockPro64 schematic [1] the rk3399 sdmmc controller is
connected to a microSD (TF card) slot, which cannot be switched to 1.8V.


Really? AFAICS the SDMMC0 wiring looks pretty much identical to the
NanoPC-T4 schematic (it's the same reference design, after all), and I
know that board can happily drive a UHS-I microSD card with 1.8v I/Os,
because mine's doing so right now.

Robin.

OK, the RockPro64 does not allow a card reset (power cycle) since
VCC3V0_SD is directly connected to VCC3V3_SYS (via R89555), the
SDMMC0_PWH_H signal is not connected. So the card fails to identify
itself after suspend or reboot when switched to 1.8V operation.



I believe we addressed this issue long time ago, please check:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6a11fc47f175c8d87018e89cb58e2d36c66534cb


Regards,
Soeren



So also configure the vcc_sdio regulator, which drives the i/o voltage
of the sdmmc controller, accordingly.

While at it, also remove the cap-mmc-highspeed property of the sdmmc
controller, since no mmc card can be connected here.

[1] http://files.pine64.org/doc/rockpro64/rockpro64_v21-SCH.pdf

Fixes: e4f3fb490967 ("arm64: dts: rockchip: add initial dts support
for Rockpro64")
Signed-off-by: Soeren Moch 
---
Cc: Heiko Stuebner 
Cc: linux-rockc...@lists.infradead.org
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
   arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dts | 3 +--
   1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dts
b/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dts
index 2e44dae4865a..084f1d994a50 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dts
@@ -353,7 +353,7 @@
   regulator-name = "vcc_sdio";
   regulator-always-on;
   regulator-boot-on;
-    regulator-min-microvolt = <180>;
+    regulator-min-microvolt = <300>;
   regulator-max-microvolt = <300>;
   regulator-state-mem {
   regulator-on-in-suspend;
@@ -624,7 +624,6 @@

    {
   bus-width = <4>;
-    cap-mmc-highspeed;
   cap-sd-highspeed;
   cd-gpios = < 7 GPIO_ACTIVE_LOW>;
   disable-wp;
--
2.17.1


___
Linux-rockchip mailing list
linux-rockc...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip



___
Linux-rockchip mailing list
linux-rockc...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip




--
Best Regards
Shawn Lin

Re: [PATCH v2] mmc: dw_mmc: Disable SDIO interrupts while suspended to fix suspend/resume【请注意，邮件由linux-rockchip-bounces+shawn.lin=rock-chips....@lists.infradead.org代发】

[Shawn Lin]

On 2019/4/30 4:40, Douglas Anderson wrote:

Processing SDIO interrupts while dw_mmc is suspended (or partly
suspended) seems like a bad idea.  We really don't want to be
processing them until we've gotten ourselves fully powered up.

You might be wondering how it's even possible to become suspended when
an SDIO interrupt is active.  As can be seen in
dw_mci_enable_sdio_irq(), we explicitly keep dw_mmc out of runtime
suspend when the SDIO interrupt is enabled.  ...but even though we
stop normal runtime suspend transitions when SDIO interrupts are
enabled, the dw_mci_runtime_suspend() can still get called for a full
system suspend.

Let's handle all this by explicitly masking SDIO interrupts in the
suspend call and unmasking them later in the resume call.  To do this
cleanly I'll keep track of whether the client requested that SDIO
interrupts be enabled so that we can reliably restore them regardless
of whether we're masking them for one reason or another.

It should be noted that if dw_mci_enable_sdio_irq() is never called
(for instance, if we don't have an SDIO card plugged in) that
"client_sdio_enb" will always be false.  In those cases this patch
adds a tiny bit of overhead to suspend/resume (a spinlock and a
read/write of INTMASK) but other than that is a no-op.  The
SDMMC_INT_SDIO bit should always be clear and clearing it again won't
hurt.

Without this fix it can be seen that rk3288-veyron Chromebooks with
Marvell WiFi would sometimes fail to resume WiFi even after picking my
recent mwifiex patch [1].  Specifically you'd see messages like this:
   mwifiex_sdio mmc1:0001:1: Firmware wakeup failed
   mwifiex_sdio mmc1:0001:1: PREP_CMD: FW in reset state

...and tracing through the resume code in the failing cases showed
that we were processing a SDIO interrupt really early in the resume
call.

NOTE: downstream in Chrome OS 3.14 and 3.18 kernels (both of which
support the Marvell SDIO WiFi card) we had a patch ("CHROMIUM: sdio:
Defer SDIO interrupt handling until after resume") [2].  Presumably
this is the same problem that was solved by that patch.

[1] https://lkml.kernel.org/r/20190404040106.40519-1-diand...@chromium.org
[2] https://crrev.com/c/230765



Sorry for late, but FWIW:

Reviewed-by: Shawn Lin 


Cc:  # 4.14.x
Signed-off-by: Douglas Anderson 
---
I didn't put any "Fixes" tag here, but presumably this could be
backported to whichever kernels folks found it useful for.  I have at
least confirmed that kernels v4.14 and v4.19 (as well as v5.1-rc2)
show the problem.  It is very easy to pick this to v4.19 and it
definitely fixes the problem there.

I haven't spent the time to pick this to 4.14 myself, but presumably
it wouldn't be too hard to backport this as far as v4.13 since that
contains commit 32dba73772f8 ("mmc: dw_mmc: Convert to use
MMC_CAP2_SDIO_IRQ_NOTHREAD for SDIO IRQs").  Prior to that it might
make sense for anyone experiencing this problem to just pick the old
CHROMIUM patch to fix them.

Changes in v2:
- Suggested 4.14+ in the stable tag (Sasha-bot)
- Extra note that this is a noop on non-SDIO (Shawn / Emil)
- Make boolean logic cleaner as per https://crrev.com/c/1586207/1
- Hopefully clear comments as per https://crrev.com/c/1586207/1

  drivers/mmc/host/dw_mmc.c | 27 +++
  drivers/mmc/host/dw_mmc.h |  3 +++
  2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 80dc2fd6576c..480067b87a94 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1664,7 +1664,8 @@ static void dw_mci_init_card(struct mmc_host *mmc, struct 
mmc_card *card)
}
  }
  
-static void __dw_mci_enable_sdio_irq(struct dw_mci_slot *slot, int enb)

+static void __dw_mci_enable_sdio_irq(struct dw_mci_slot *slot, bool enb,
+bool client_requested)
  {
struct dw_mci *host = slot->host;
unsigned long irqflags;
@@ -1672,6 +1673,20 @@ static void __dw_mci_enable_sdio_irq(struct dw_mci_slot 
*slot, int enb)
  
  	spin_lock_irqsave(>irq_lock, irqflags);
  
+	/*

+* If we're being called directly from dw_mci_enable_sdio_irq()
+* (which means that the client driver actually wants to enable or
+* disable interrupts) then save the request.  Otherwise this
+* wasn't directly requested by the client and we should logically
+* AND it with the client request since we want to disable if
+* _either_ the client disabled OR we have some other reason to
+* disable temporarily.
+*/
+   if (client_requested)
+   host->client_sdio_enb = enb;
+   else
+   enb &= host->client_sdio_enb;
+
/* Enable/disable Slot Specific SDIO interrupt */
int_mask = mci_readl(host, INTMASK);
if (enb)
@@ -1688,7 +1703,7 @@ static void dw_mci_enable_sdio_irq(struct mmc_host *mmc, 
int enb)
struct dw_mci

Re: [PATCH] PCI: rockchip: fix bitwise operations on status and ROCKCHIP_PCIE_EP_CMD_STATUS_IS

[Shawn Lin]

On 2019/4/12 17:51, Lorenzo Pieralisi wrote:

On Sat, Mar 30, 2019 at 03:09:10PM +, Colin King wrote:

From: Colin Ian King 

Currently the bitwise operations on the u16 variable 'status' with
the setting ROCKCHIP_PCIE_EP_CMD_STATUS_IS are incorrect because
ROCKCHIP_PCIE_EP_CMD_STATUS_IS is 1UL<<19 which is wider than the
u16 variable.  Fix this by making status a u32.  (Not tested).

Fixes: cf590b078391 ("PCI: rockchip: Add EP driver for Rockchip PCIe 
controller")
Signed-off-by: Colin Ian King 
---
  drivers/pci/controller/pcie-rockchip-ep.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


Shawn,

I need your ACK on this patch, thanks.


Acked-by: Shawn Lin 



Lorenzo


diff --git a/drivers/pci/controller/pcie-rockchip-ep.c 
b/drivers/pci/controller/pcie-rockchip-ep.c
index a5d799e2dff2..d743b0a48988 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -350,7 +350,7 @@ static void rockchip_pcie_ep_assert_intx(struct 
rockchip_pcie_ep *ep, u8 fn,
struct rockchip_pcie *rockchip = >rockchip;
u32 r = ep->max_regions - 1;
u32 offset;
-   u16 status;
+   u32 status;
u8 msg_code;
  
  	if (unlikely(ep->irq_pci_addr != ROCKCHIP_PCIE_EP_PCI_LEGACY_IRQ_ADDR ||

--
2.20.1

Re: [PATCH 2/2] mmc: dw_mmc-rockchip: fix transfer hangs on rk3188【请注意，邮件由linux-mmc-ow...@vger.kernel.org代发】

[Shawn Lin]

+ Caesar Wang

On 2019/3/21 1:48, Alexander Kochetkov wrote:

I've found that sometimes dw_mmc in my rk3188 based board stop transfer
any data with error:

kernel: dwmmc_rockchip 1021c000.dwmmc: Unexpected command timeout, state 3

Further digging into problem showed that sometimes one of EDMA-based
transfers hangs and abort with HTO error. I've made test, that 100%


I'm not sure what 100% means, but Caesar fired QA test for RK3036 with
EDMA-based dwmmc in vendor 4.4 kernel, and seems not big deal. The
vendor 4.4 kernel didn't patch anything else wrt EDMA code, but we did
enhance PL330 code and fix some bug there, so you may have a try.


reproduce the error. I found, that setting max_segs parameter to 1 fix
the problem.

I guess the problem is hardware related and relates to DMA controller
implementation for rk3188. Probably it can relates to missed FLUSHP,
see commit 271e1b86e691 ("dmaengine: pl330: add quirk for broken no
flushp"). It is possible that pl330 and dw_mmc become out of sync then
pl330 driver switch from one scatterlist to another. If we limit
scatterlist size to 1, we can avoid switching scatterlists and avoid
hardware problem. Setting max_segs to 1 tells mmc core to use maximum
one scatterlist for one transfer.

I guess that all other rk3xxx chips that lacks FLUSHP also affected by
the problem. So I made fix for all rk3xxx chips from rk2928 to rk3188.


Hard to find these acient platforms to test, expecially some was EOL



Signed-off-by: Alexander Kochetkov 
---
  drivers/mmc/host/dw_mmc-rockchip.c |   19 +++
  1 file changed, 19 insertions(+)

diff --git a/drivers/mmc/host/dw_mmc-rockchip.c 
b/drivers/mmc/host/dw_mmc-rockchip.c
index 8c86a80..2eed922 100644
--- a/drivers/mmc/host/dw_mmc-rockchip.c
+++ b/drivers/mmc/host/dw_mmc-rockchip.c
@@ -292,6 +292,24 @@ static int dw_mci_rk3288_parse_dt(struct dw_mci *host)
return 0;
  }
  
+static void dw_mci_rk2928_init_slot(struct dw_mci *host)

+{
+   struct mmc_host *mmc = host->slot->mmc;
+
+   if (host->use_dma == TRANS_MODE_EDMAC) {
+   /*
+* Using max_segs > 1 leads to rare EDMA transfer hangs
+* resulting in HTO errors.
+*/
+   mmc->max_segs = 1;
+   mmc->max_blk_size = 65535;
+   mmc->max_blk_count = 64 * 512;
+   mmc->max_req_size =
+   mmc->max_blk_size * mmc->max_blk_count;
+   mmc->max_seg_size = mmc->max_req_size;
+   }
+}
+
  static int dw_mci_rockchip_init(struct dw_mci *host)
  {
/* It is slot 8 on Rockchip SoCs */
@@ -314,6 +332,7 @@ static int dw_mci_rockchip_init(struct dw_mci *host)
  
  static const struct dw_mci_drv_data rk2928_drv_data = {

.init   = dw_mci_rockchip_init,
+   .init_slot  = dw_mci_rk2928_init_slot,
  };
  
  static const struct dw_mci_drv_data rk3288_drv_data = {

Re: [PATCH] mmc: dw_mmc: Fix to avoid potential NULL pointer dereference【请注意，邮件由linux-mmc-ow...@vger.kernel.org代发】

[Shawn Lin]

On 2019/3/19 8:19, Aditya Pakki wrote:

of_match_node can fail and return NULL in case no matching structure.
The patches checks for such a scenario and returns -ENXIO.

Signed-off-by: Aditya Pakki 
---
  drivers/mmc/host/dw_mmc-exynos.c | 2 ++
  drivers/mmc/host/dw_mmc-k3.c | 2 ++
  drivers/mmc/host/dw_mmc-pltfm.c  | 2 ++
  3 files changed, 6 insertions(+)



Could you check dw_mmc-zx.c and dw_mmc-rockchip.c as well?


diff --git a/drivers/mmc/host/dw_mmc-exynos.c b/drivers/mmc/host/dw_mmc-exynos.c
index d46c3439b508..8a75d7314606 100644
--- a/drivers/mmc/host/dw_mmc-exynos.c
+++ b/drivers/mmc/host/dw_mmc-exynos.c
@@ -554,6 +554,8 @@ static int dw_mci_exynos_probe(struct platform_device *pdev)
int ret;
  
  	match = of_match_node(dw_mci_exynos_match, pdev->dev.of_node);

+   if (!match)
+   return -ENXIO;
drv_data = match->data;
  
  	pm_runtime_get_noresume(>dev);

diff --git a/drivers/mmc/host/dw_mmc-k3.c b/drivers/mmc/host/dw_mmc-k3.c
index 89cdb3d533bb..cc50b7546a20 100644
--- a/drivers/mmc/host/dw_mmc-k3.c
+++ b/drivers/mmc/host/dw_mmc-k3.c
@@ -459,6 +459,8 @@ static int dw_mci_k3_probe(struct platform_device *pdev)
const struct of_device_id *match;
  
  	match = of_match_node(dw_mci_k3_match, pdev->dev.of_node);

+   if (!match)
+   return -ENXIO;
drv_data = match->data;
  
  	return dw_mci_pltfm_register(pdev, drv_data);

diff --git a/drivers/mmc/host/dw_mmc-pltfm.c b/drivers/mmc/host/dw_mmc-pltfm.c
index 58c13e21bd5a..b1948989f617 100644
--- a/drivers/mmc/host/dw_mmc-pltfm.c
+++ b/drivers/mmc/host/dw_mmc-pltfm.c
@@ -82,6 +82,8 @@ static int dw_mci_pltfm_probe(struct platform_device *pdev)
  
  	if (pdev->dev.of_node) {

match = of_match_node(dw_mci_pltfm_match, pdev->dev.of_node);
+   if (!match)
+   return -ENXIO;
drv_data = match->data;
}
  

Re: ulfh/next boot bisection: v5.1-rc1-22-gb3725d97ba75 on rk3399-gru-kevin【请注意，邮件由linux-mmc-ow...@vger.kernel.org代发】

[Shawn Lin]

On 2019/3/19 6:50, kernelci.org bot wrote:

* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
* This automated bisection report was sent to you on the basis  *
* that you may be involved with the breaking commit it has  *
* found.  No manual investigation has been done to verify it,   *
* and the root cause of the problem may be somewhere else.  *
* Hope this helps!  *
* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *

ulfh/next boot bisection: v5.1-rc1-22-gb3725d97ba75 on rk3399-gru-kevin

Summary:
   Start:  b3725d97ba75 Merge branch 'fixes' into next
   Details:https://kernelci.org/boot/id/5c8fa85d59b5148afcfe6052
   Plain log:  
https://storage.kernelci.org//ulfh/next/v5.1-rc1-22-gb3725d97ba75/arm64/defconfig/gcc-7/lab-collabora/boot-rk3399-gru-kevin.txt
   HTML log:   
https://storage.kernelci.org//ulfh/next/v5.1-rc1-22-gb3725d97ba75/arm64/defconfig/gcc-7/lab-collabora/boot-rk3399-gru-kevin.html
   Result: d6a6d722481f mmc: dw_mmc-rockchip: Enable hardware unbusy 
interrupt support


Thanks for report!

Ulf,

It's a known issue already but didn't got managed to post v3 due to
weekend. Should I post a increamental patch on top? or just resend a
v3 series?



Checks:
   revert: PASS
   verify: PASS

Parameters:
   Tree:   ulfh
   URL:https://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git
   Branch: next
   Target: rk3399-gru-kevin
   CPU arch:   arm64
   Lab:lab-collabora
   Compiler:   gcc-7
   Config: defconfig
   Test suite: boot

Breaking commit found:

---
commit d6a6d722481f357eafe7b798fe6fdadd2f5ac6bd
Author: Shawn Lin 
Date:   Tue Mar 12 15:35:09 2019 +0800

 mmc: dw_mmc-rockchip: Enable hardware unbusy interrupt support
 
 The new register for controlling hardware unbusy interrupt

 is in 0x120. It looks like:
 
 ||

 |Bit|  Attribute |  Reset Value  | Description   |
 ||
 |31:25  |RO  |  0x0  |  reserved |
 ||
 |24 |RO  |  0x0  |  rdyint_cnt_finish|
 |   ||   |When high, it indicates|
 |   ||   |that the rdyint counter|
 |   ||   |is finished.   |
 ||
 |23:16  |RO  |  0x0  |  rdyint_cnt_status|
 |   ||   |Couner status, reflect |
 |   ||   |internal counter value.|
 ||
 |15:9   |RO  |  0x0  |  reserved |
 ||
 |8  |RW  |  0x0  |  rdyint_gen_working   |
 |   ||   |When set, IP starts to |
 |   ||   |count and generate one |
 |   ||   |rdyint trigger. After  |
 |   ||   |the rdyint trigger is  |
 |   ||   |generated, it will be  |
 |   ||   |cleaned automatically. |
 |   ||   |Software should set it |
 |   ||   |again next time.   |
 ||
 |7:0|RW  |  0xff |   rdyint_gen_maxval   |
 |   ||   |Max counter value for  |
 |   ||   |the IP to count when   |
 |   ||   |rdyint_gen_working is  |
 |   ||   |set. This counter is   |
 |   ||   |based on biu_clk.  |
 ||
 
 Signed-off-by: Shawn Lin 

 Tested-by: Ziyuan Xu 
 Signed-off-by: Ulf Hansson 

diff --git a/drivers/mmc/host/dw_mmc-rockchip.c 
b/drivers/mmc/host/dw_mmc-rockchip.c
index 8c86a800a8fd..85b1e42782a0 100644
--- a/drivers/mmc/host/dw_mmc-rockchip.c
+++ b/drivers/mmc/host/dw_mmc-rockchip.c
@@ -20,6 +20,9 @@
  #include "dw_mmc-pltfm.h"
  
  #define RK3288_CLKGEN_DIV   2

+#define RKMMC_RDYINT_GEN   0x120
+#define RKMMC_RDYINT_GEN_WORKING BIT(8)
+#define RKMMC_RDYINT_GEN_MAXVAL GENMASK(7, 0)
  
  struct dw_mci_rockchip_priv_data {

struct clk  *drv_clk;
@@ -28,6 +31,23 @@ struct dw_mci_rockchip_priv_data {
int num_phases;
  };
  
+static int dw_mci_rockchip_prepare

Re: [PATCH 3/3] arm64: dts: rockchip: Disable DCMDs on RK3399's eMMC controller.【请注意，邮件由linux-mmc-ow...@vger.kernel.org代发】

[Shawn Lin]

On 2019/3/2 0:43, Christoph Muellner wrote:

When using direct commands (DCMDs) on an RK3399, we get spurious
CQE completion interrupts for the DCMD transaction slot (#31):


I didn't see it. Do you try any newer code, for instance, linux-next?



[  931.196520] [ cut here ]
[  931.201702] mmc1: cqhci: spurious TCN for tag 31
[  931.206906] WARNING: CPU: 0 PID: 1433 at
/usr/src/kernel/drivers/mmc/host/cqhci.c:725 cqhci_irq+0x2e4/0x490
[  931.206909] Modules linked in:
[  931.206918] CPU: 0 PID: 1433 Comm: irq/29-mmc1 Not tainted
4.19.8-rt6-funkadelic #1
[  931.206920] Hardware name: Theobroma Systems RK3399-Q7 SoM (DT)
[  931.206924] pstate: 4005 (nZcv daif -PAN -UAO)
[  931.206927] pc : cqhci_irq+0x2e4/0x490
[  931.206931] lr : cqhci_irq+0x2e4/0x490
[  931.206933] sp : 0e54bc80
[  931.206934] x29: 0e54bc80 x28: 
[  931.206939] x27: 0001 x26: 08f217e8
[  931.206944] x25: 8000f02ef030 x24: 091417b0
[  931.206948] x23: 090aa000 x22: 8000f008b000
[  931.206953] x21: 0002 x20: 001f
[  931.206957] x19: 8000f02ef018 x18: 
[  931.206961] x17:  x16: 
[  931.206966] x15: 090aa6c8 x14: 0720072007200720
[  931.206970] x13: 0720072007200720 x12: 0720072007200720
[  931.206975] x11: 0720072007200720 x10: 0720072007200720
[  931.206980] x9 : 0720072007200720 x8 : 0720072007200720
[  931.206984] x7 : 0720073107330720 x6 : 05a0
[  931.206988] x5 : 0860d4b0 x4 : 
[  931.206993] x3 : 0001 x2 : 0001
[  931.206997] x1 : 1bde3a91b0d4d900 x0 : 
[  931.207001] Call trace:
[  931.207005]  cqhci_irq+0x2e4/0x490
[  931.207009]  sdhci_arasan_cqhci_irq+0x5c/0x90
[  931.207013]  sdhci_irq+0x98/0x930
[  931.207019]  irq_forced_thread_fn+0x2c/0xa0
[  931.207023]  irq_thread+0x114/0x1c0
[  931.207027]  kthread+0x128/0x130
[  931.207032]  ret_from_fork+0x10/0x20
[  931.207035] ---[ end trace 0002 ]---

The driver shows this message only for the first spurious interrupt
by using WARN_ONCE(). Changing this to WARN() shows, that this is
happening quite frequently (up to once a second).

Since the eMMC 5.1 specification, where CQE and CQHCI are specified,
does not mention that spurious TCN interrupts for DCMDs can be simply
ignored, we must assume that using this feature is not working reliably.

The current implementation uses DCMD for REQ_OP_FLUSH only, and
I could not see any performance/power impact when disabling
this optional feature for RK3399.

Therefore this patch disables DCMDs for RK3399.


We need to sort out the problem, and see if it could be solved, or
we just simply remove MMC_CAP2_CQE_DCMD it from sdhci-of-arasan



Signed-off-by: Christoph Muellner 
Signed-off-by: Philipp Tomsich 
---
  arch/arm64/boot/dts/rockchip/rk3399.dtsi | 1 +
  1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi 
b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
index 6cc1c9fa4ea6..1bbf0da4e01d 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -333,6 +333,7 @@
phys = <_phy>;
phy-names = "phy_arasan";
power-domains = < RK3399_PD_EMMC>;
+   disable-cqe-dcmd;
status = "disabled";
};
  

Re: [PATCH] ARM: dts: rockchip: remove disable-wp from rv1108-elgin-r1 emmc node【请注意，邮件由linux-rockchip-bounces+shawn.lin=rock-chips....@lists.infradead.org代发】

[Shawn Lin]

On 2019/2/19 21:03, Johan Jonker wrote:

The mmc.txt didn't explicitly say disable-wp is for SD card slot only,
but that is what it was designed for in the first place.
Remove all disable-wp from emmc or sdio controllers.

Signed-off-by: Johan Jonker 
---
  arch/arm/boot/dts/rv1108-elgin-r1.dts | 1 -
  1 file changed, 1 deletion(-)

diff --git a/arch/arm/boot/dts/rv1108-elgin-r1.dts 
b/arch/arm/boot/dts/rv1108-elgin-r1.dts
index 1c4507b..b1db924 100644
--- a/arch/arm/boot/dts/rv1108-elgin-r1.dts
+++ b/arch/arm/boot/dts/rv1108-elgin-r1.dts
@@ -37,7 +37,6 @@
   {
bus-width = <8>;
cap-mmc-highspeed;
-   disable-wp;


Reviewed-by: Shawn Lin 


no-sd;
no-sdio;
non-removable;

Re: [PATCH v2 00/15] PCI: endpoint: Cleanup EPC features

[Shawn Lin]

On 2019/1/14 19:14, Kishon Vijay Abraham I wrote:

Hi Lorenzo,

The Endpoint controller driver uses features member in 'struct pci_epc'
to advertise the list of supported features to the endpoint function
driver.

There are a few shortcomings with this approach.
   *) Certain endpoint controllers support fixed size BAR (e.g. TI's
  AM654 uses Designware configuration with fixed size BAR). The
  size of each BARs cannot be passed to the endpoint function
  driver.
   *) Too many macros for handling EPC features.
  (EPC_FEATURE_NO_LINKUP_NOTIFIER, EPC_FEATURE_BAR_MASK,
   EPC_FEATURE_MSIX_AVAILABLE, EPC_FEATURE_SET_BAR,
   EPC_FEATURE_GET_BAR)
   *) Endpoint controllers are directly modifying struct pci_epc
  members. (I have plans to move struct pci_epc to
  drivers/pci/endpoint so that pci_epc members are referenced
  only by endpoint core).

To overcome the above shortcomings, introduced pci_epc_get_features()
API, pci_epc_features structure and a ->get_features() callback.

Also added a patch to set BAR flags in pci_epf_alloc_space and
remove it from pci-epf-test function driver.

Changes from v1:
*) Fixed helper function to return '0' (or BAR_0) for any incorrect
values in reserved BAR.
*) Do not set_bar or alloc space for BARs if the BARs are reserved
*) Fix incorrect check of epc_features in pci_epf_test_bind

Tested on TI's DRA7xx platform and AM654 platform. Support for PCIe
in AM654 platform will be posted shortly.




8<-


  drivers/pci/controller/pcie-rockchip-ep.c | 16 +++-


Acked-by: Shawn Lin 

Re: [PATCH v2 1/3] mmc: sdhci: add support for using external DMA devices

[Shawn Lin]

On 2018/11/29 17:59, Chunyan Zhang wrote:

Hi Adrian,

On Thu, 29 Nov 2018 at 15:36, Adrian Hunter  wrote:


On 29/11/18 8:22 AM, Chunyan Zhang wrote:

On Tue, 20 Nov 2018 at 21:41, Adrian Hunter  wrote:


On 12/11/18 9:26 AM, Chunyan Zhang wrote:

Some standard SD host controllers can support both external dma
controllers as well as ADMA/SDMA in which the SD host controller
acts as DMA master. TI's omap controller is the case as an example.

Currently the generic SDHCI code supports ADMA/SDMA integrated in
the host controller but does not have any support for external DMA
controllers implemented using dmaengine, meaning that custom code is
needed for any systems that use an external DMA controller with SDHCI.


I still think you probably need to reset the DMA if there are transfer
errors - perhaps you could comment on that.  Also there are some comments below.


With regard to "transfer error", do you mean if
sdhci_external_dma_setup() failed?


No, I mean any error interrupt that can leave the DMA uncompleted.  For
SDHCI, resetting the data circuit cleans that up, but presumably something
is needed for external DMA?


Yes, it should need a dmaengine_terminate_all().



No, dmaengine_terminate_all is deprecated for quite a long time.
Please use dmaengine_terminate_{async, sync}() instead.

See Documentation/dmaengine/client.txt



How about adding that at here (I will wrap it up of course):
https://elixir.bootlin.com/linux/v4.19.5/source/drivers/mmc/host/sdhci.c#L2553

Is there somewhere else I'm missing?

Thanks,
Chunyan

Re: [PATCH v2 1/3] mmc: sdhci: add support for using external DMA devices

[Shawn Lin]

On 2018/11/29 17:59, Chunyan Zhang wrote:

Hi Adrian,

On Thu, 29 Nov 2018 at 15:36, Adrian Hunter  wrote:


On 29/11/18 8:22 AM, Chunyan Zhang wrote:

On Tue, 20 Nov 2018 at 21:41, Adrian Hunter  wrote:


On 12/11/18 9:26 AM, Chunyan Zhang wrote:

Some standard SD host controllers can support both external dma
controllers as well as ADMA/SDMA in which the SD host controller
acts as DMA master. TI's omap controller is the case as an example.

Currently the generic SDHCI code supports ADMA/SDMA integrated in
the host controller but does not have any support for external DMA
controllers implemented using dmaengine, meaning that custom code is
needed for any systems that use an external DMA controller with SDHCI.


I still think you probably need to reset the DMA if there are transfer
errors - perhaps you could comment on that.  Also there are some comments below.


With regard to "transfer error", do you mean if
sdhci_external_dma_setup() failed?


No, I mean any error interrupt that can leave the DMA uncompleted.  For
SDHCI, resetting the data circuit cleans that up, but presumably something
is needed for external DMA?


Yes, it should need a dmaengine_terminate_all().



No, dmaengine_terminate_all is deprecated for quite a long time.
Please use dmaengine_terminate_{async, sync}() instead.

See Documentation/dmaengine/client.txt



How about adding that at here (I will wrap it up of course):
https://elixir.bootlin.com/linux/v4.19.5/source/drivers/mmc/host/sdhci.c#L2553

Is there somewhere else I'm missing?

Thanks,
Chunyan

Re: [PATCH v3] arm64: dts: rockchip: Add DT for nanopc-t4

[Shawn Lin]

On 2018/11/27 7:48, Heiko Stuebner wrote:

Hi Tomeu,

Am Montag, 26. November 2018, 15:47:49 CET schrieb Tomeu Vizoso:

This adds a device tree for the NanoPC-T4 SBC, which is based on the
Rockchip RK3399 SoC and marketed by FriendlyELEC.

Known working:

- Serial
- Ethernet
- HDMI
- USB 2.0

All of the interesting stuff is in a .dtsi because there are at least
two other boards that share most of it: NanoPi M4 and NanoPi NEO4.

Signed-off-by: Tomeu Vizoso 


looks pretty good overall, just some more small-scale things
below.


---

v2: - Rename compatible from friendlyelec to friendlyarm, to match
   existing bindings
 - Remove superfluous node spi1

v3: - Rewrite regulator tree to match the schematics (Heiko)
 - Sort top-level nodes alphabetically (Heiko)
 - Used defines for GPIO numbers (Heiko)
 - Enabled rga (Heiko)
 - Removed cdn_dp node (Heiko)
 - Removed dependencies to fusb0 as extcon (Heiko)
 - Removed superfluous properties (Heiko)




diff --git a/arch/arm64/boot/dts/rockchip/Makefile 
b/arch/arm64/boot/dts/rockchip/Makefile
index 49042c477870..4cbd2c461052 100644
--- a/arch/arm64/boot/dts/rockchip/Makefile
+++ b/arch/arm64/boot/dts/rockchip/Makefile
@@ -1,6 +1,7 @@
  # SPDX-License-Identifier: GPL-2.0
  dtb-$(CONFIG_ARCH_ROCKCHIP) += px30-evb.dtb
  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-evb.dtb
+dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopc-t4.dtb
  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-rock64.dtb
  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-roc-cc.dtb
  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3368-evb-act8846.dtb


These are definitly sorted in the Makefile, so this should move between
rk3399-gru-scarlet-kd.dtb and rk3399-puma-haikou.dtb :-)



diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi 
b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
new file mode 100644
index ..f102ff2317c3
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
@@ -0,0 +1,740 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * RK3399-based FriendlyElec boards device tree source
+ *
+ * Copyright (c) 2016 Fuzhou Rockchip Electronics Co., Ltd
+ *
+ * Copyright (c) 2018 FriendlyElec Computer Tech. Co., Ltd.
+ * (http://www.friendlyarm.com)
+ *
+ * Copyright (c) 2018 Collabora Ltd.
+ */
+
+/dts-v1/;
+#include 
+#include "rk3399.dtsi"
+#include "rk3399-opp.dtsi"
+
+/ {
+   chosen {
+   stdout-path = "serial2:150n8";
+   };
+
+   clkin_gmac: external-gmac-clock {
+   compatible = "fixed-clock";
+   clock-frequency = <12500>;
+   clock-output-names = "clkin_gmac";
+   #clock-cells = <0>;
+   };
+
+   vdd_5v: vdd_5v {
+   compatible = "regulator-fixed";
+   regulator-name = "vdd_5v";
+   regulator-always-on;
+   regulator-boot-on;
+   };
+
+   vcc5v0_core: vcc5v0_core {
+   compatible = "regulator-fixed";
+   regulator-name = "vcc5v0_core";
+   regulator-always-on;
+   regulator-boot-on;
+   vin-supply = <_5v>;
+   };
+
+   vcc3v3_sys: vcc3v3_sys {
+   compatible = "regulator-fixed";
+   regulator-name = "vcc3v3_sys";
+   regulator-always-on;
+   regulator-boot-on;
+   regulator-min-microvolt = <330>;
+   regulator-max-microvolt = <330>;
+   vin-supply = <_core>;
+   };
+
+   vcc5v0_sys: vcc5v0_sys {
+   compatible = "regulator-fixed";
+   regulator-name = "vcc5v0_sys";
+   regulator-always-on;
+   regulator-boot-on;
+   regulator-min-microvolt = <500>;
+   regulator-max-microvolt = <500>;
+   vin-supply = <_5v>;
+   };
+
+   vcc5v0_usb1: vcc5v0_usb1 {
+   compatible = "regulator-fixed";
+   regulator-name = "vcc5v0_usb1";
+   regulator-always-on;
+   regulator-boot-on;
+   vin-supply = <_sys>;
+   };
+
+   vcc5v0_usb2: vcc5v0_usb2 {
+   compatible = "regulator-fixed";
+   regulator-name = "vcc5v0_usb2";
+   regulator-always-on;
+   regulator-boot-on;
+   vin-supply = <_sys>;
+   };
+
+   /* switched by pmic_sleep */
+   vcc1v8_s3: vcca1v8_s3: vcc1v8-s3 {
+   compatible = "regulator-fixed";
+   regulator-name = "vcc1v8_s3";
+   regulator-always-on;
+   regulator-boot-on;
+   regulator-min-microvolt = <180>;
+   regulator-max-microvolt = <180>;
+   vin-supply = <_1v8>;
+   };
+
+   vcc3v0_sd: vcc3v0_sd {


dt-spec mandates node names with "-", so this should become
vcc3v0_sd: vcc3v0-sd {

Same for most regulators above.


+   rk808: pmic@1b {
+   compatible = "rockchip,rk808";
+   reg = 

Re: [PATCH v3] arm64: dts: rockchip: Add DT for nanopc-t4

[Shawn Lin]

On 2018/11/27 7:48, Heiko Stuebner wrote:

Hi Tomeu,

Am Montag, 26. November 2018, 15:47:49 CET schrieb Tomeu Vizoso:

This adds a device tree for the NanoPC-T4 SBC, which is based on the
Rockchip RK3399 SoC and marketed by FriendlyELEC.

Known working:

- Serial
- Ethernet
- HDMI
- USB 2.0

All of the interesting stuff is in a .dtsi because there are at least
two other boards that share most of it: NanoPi M4 and NanoPi NEO4.

Signed-off-by: Tomeu Vizoso 


looks pretty good overall, just some more small-scale things
below.


---

v2: - Rename compatible from friendlyelec to friendlyarm, to match
   existing bindings
 - Remove superfluous node spi1

v3: - Rewrite regulator tree to match the schematics (Heiko)
 - Sort top-level nodes alphabetically (Heiko)
 - Used defines for GPIO numbers (Heiko)
 - Enabled rga (Heiko)
 - Removed cdn_dp node (Heiko)
 - Removed dependencies to fusb0 as extcon (Heiko)
 - Removed superfluous properties (Heiko)




diff --git a/arch/arm64/boot/dts/rockchip/Makefile 
b/arch/arm64/boot/dts/rockchip/Makefile
index 49042c477870..4cbd2c461052 100644
--- a/arch/arm64/boot/dts/rockchip/Makefile
+++ b/arch/arm64/boot/dts/rockchip/Makefile
@@ -1,6 +1,7 @@
  # SPDX-License-Identifier: GPL-2.0
  dtb-$(CONFIG_ARCH_ROCKCHIP) += px30-evb.dtb
  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-evb.dtb
+dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3399-nanopc-t4.dtb
  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-rock64.dtb
  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3328-roc-cc.dtb
  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3368-evb-act8846.dtb


These are definitly sorted in the Makefile, so this should move between
rk3399-gru-scarlet-kd.dtb and rk3399-puma-haikou.dtb :-)



diff --git a/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi 
b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
new file mode 100644
index ..f102ff2317c3
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3399-nanopi4.dtsi
@@ -0,0 +1,740 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * RK3399-based FriendlyElec boards device tree source
+ *
+ * Copyright (c) 2016 Fuzhou Rockchip Electronics Co., Ltd
+ *
+ * Copyright (c) 2018 FriendlyElec Computer Tech. Co., Ltd.
+ * (http://www.friendlyarm.com)
+ *
+ * Copyright (c) 2018 Collabora Ltd.
+ */
+
+/dts-v1/;
+#include 
+#include "rk3399.dtsi"
+#include "rk3399-opp.dtsi"
+
+/ {
+   chosen {
+   stdout-path = "serial2:150n8";
+   };
+
+   clkin_gmac: external-gmac-clock {
+   compatible = "fixed-clock";
+   clock-frequency = <12500>;
+   clock-output-names = "clkin_gmac";
+   #clock-cells = <0>;
+   };
+
+   vdd_5v: vdd_5v {
+   compatible = "regulator-fixed";
+   regulator-name = "vdd_5v";
+   regulator-always-on;
+   regulator-boot-on;
+   };
+
+   vcc5v0_core: vcc5v0_core {
+   compatible = "regulator-fixed";
+   regulator-name = "vcc5v0_core";
+   regulator-always-on;
+   regulator-boot-on;
+   vin-supply = <_5v>;
+   };
+
+   vcc3v3_sys: vcc3v3_sys {
+   compatible = "regulator-fixed";
+   regulator-name = "vcc3v3_sys";
+   regulator-always-on;
+   regulator-boot-on;
+   regulator-min-microvolt = <330>;
+   regulator-max-microvolt = <330>;
+   vin-supply = <_core>;
+   };
+
+   vcc5v0_sys: vcc5v0_sys {
+   compatible = "regulator-fixed";
+   regulator-name = "vcc5v0_sys";
+   regulator-always-on;
+   regulator-boot-on;
+   regulator-min-microvolt = <500>;
+   regulator-max-microvolt = <500>;
+   vin-supply = <_5v>;
+   };
+
+   vcc5v0_usb1: vcc5v0_usb1 {
+   compatible = "regulator-fixed";
+   regulator-name = "vcc5v0_usb1";
+   regulator-always-on;
+   regulator-boot-on;
+   vin-supply = <_sys>;
+   };
+
+   vcc5v0_usb2: vcc5v0_usb2 {
+   compatible = "regulator-fixed";
+   regulator-name = "vcc5v0_usb2";
+   regulator-always-on;
+   regulator-boot-on;
+   vin-supply = <_sys>;
+   };
+
+   /* switched by pmic_sleep */
+   vcc1v8_s3: vcca1v8_s3: vcc1v8-s3 {
+   compatible = "regulator-fixed";
+   regulator-name = "vcc1v8_s3";
+   regulator-always-on;
+   regulator-boot-on;
+   regulator-min-microvolt = <180>;
+   regulator-max-microvolt = <180>;
+   vin-supply = <_1v8>;
+   };
+
+   vcc3v0_sd: vcc3v0_sd {


dt-spec mandates node names with "-", so this should become
vcc3v0_sd: vcc3v0-sd {

Same for most regulators above.


+   rk808: pmic@1b {
+   compatible = "rockchip,rk808";
+   reg = 

Re: dw_mmc: IDMAC Invalidate cache after read

[Shawn Lin]

On 2018/11/23 23:29, Robin Murphy wrote:

Hi Jan,

[repeating some of the discussion from your other thread for the benefit 
of the MMC audience]


On 21/11/2018 07:42, JABLONSKY Jan wrote:

CPU may not see most up-to-date and correct copy of DMA buffer, when
internal DMA controller is in use.
Problem appears on The Altera SoC FPGA (uses integrated DMA controller),
during higher CPU and system memory load

Signed-off-by: Jan Jablonsky 
---
  drivers/mmc/host/dw_mmc.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 80dc2fd..63873d9 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -499,8 +499,7 @@ static void dw_mci_dmac_complete_dma(void *arg)
  dev_vdbg(host->dev, "DMA complete\n");
-    if ((host->use_dma == TRANS_MODE_EDMAC) &&
-    data && (data->flags & MMC_DATA_READ))
+    if (data && (data->flags & MMC_DATA_READ))
  /* Invalidate cache after read */
  dma_sync_sg_for_cpu(mmc_dev(host->slot->mmc),
  data->sg,


It looks very dubious whether this is actually the right thing to do. 
Just considering this driver, edma has an complementary sync_sg call in 
its .start method, so if idma needed this one, logically shouldn't it 
also need the other one as well?


However, from a DMA API point of view, these syncs make no sense either 
way - the very next thing we do here is call host->dma_ops->cleanup(), 
which calls dma_unmap_sg(), which will perform the appropriate cache 
maintenance anyway. Thus I can't see why this code is even here to begin 
with. Similarly on the request path - the sg list really shouldn't have 
been touched since being mapped in dw_mci_pre_dma_transfer(), so that 
sync should also be an effective no-op unless it's papering over some 
race condition elsewhere.


Shawn - do you remember why these syncs were added in 3fc7eaef44dbc? 
Were you seeing actual coherency issues on RK31xx SoCs, or was it 
perhaps just some leftover or misunderstanding which missed getting 
cleaned up?


I  can't remember too much details but looking at the dma-mapping code 
again, it seems the complemetary sync-op here is useless.




Robin.

Re: dw_mmc: IDMAC Invalidate cache after read

[Shawn Lin]

On 2018/11/23 23:29, Robin Murphy wrote:

Hi Jan,

[repeating some of the discussion from your other thread for the benefit 
of the MMC audience]


On 21/11/2018 07:42, JABLONSKY Jan wrote:

CPU may not see most up-to-date and correct copy of DMA buffer, when
internal DMA controller is in use.
Problem appears on The Altera SoC FPGA (uses integrated DMA controller),
during higher CPU and system memory load

Signed-off-by: Jan Jablonsky 
---
  drivers/mmc/host/dw_mmc.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 80dc2fd..63873d9 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -499,8 +499,7 @@ static void dw_mci_dmac_complete_dma(void *arg)
  dev_vdbg(host->dev, "DMA complete\n");
-    if ((host->use_dma == TRANS_MODE_EDMAC) &&
-    data && (data->flags & MMC_DATA_READ))
+    if (data && (data->flags & MMC_DATA_READ))
  /* Invalidate cache after read */
  dma_sync_sg_for_cpu(mmc_dev(host->slot->mmc),
  data->sg,


It looks very dubious whether this is actually the right thing to do. 
Just considering this driver, edma has an complementary sync_sg call in 
its .start method, so if idma needed this one, logically shouldn't it 
also need the other one as well?


However, from a DMA API point of view, these syncs make no sense either 
way - the very next thing we do here is call host->dma_ops->cleanup(), 
which calls dma_unmap_sg(), which will perform the appropriate cache 
maintenance anyway. Thus I can't see why this code is even here to begin 
with. Similarly on the request path - the sg list really shouldn't have 
been touched since being mapped in dw_mci_pre_dma_transfer(), so that 
sync should also be an effective no-op unless it's papering over some 
race condition elsewhere.


Shawn - do you remember why these syncs were added in 3fc7eaef44dbc? 
Were you seeing actual coherency issues on RK31xx SoCs, or was it 
perhaps just some leftover or misunderstanding which missed getting 
cleaned up?


I  can't remember too much details but looking at the dma-mapping code 
again, it seems the complemetary sync-op here is useless.




Robin.

Re: [PATCH] ARM64: dts: rockchip: add some pins to rk3399

[Shawn Lin]

On 2018/6/13 2:21, klaus.go...@theobroma-systems.com wrote:

Hi Randy,


-8<-


pcie {
+   pcie_clkreqn: pci-clkreqn {
+   rockchip,pins =
+   <2 26 RK_FUNC_2 _pull_none>;
+   };
+
+   pcie_clkreqnb: pci-clkreqnb {
+   rockchip,pins =
+   <4 24 RK_FUNC_1 _pull_none>;
+   };
+


I’m not sure if pci-clkreqn is functional at all. If not I’m not sure if we 
should add it to the dtsi.
Shawn may know more about it.


Please refer to commit 461a00bb9d539e
("arm64: dts: rockchip: kill pcie_clkreqn and pcie_clkreqnb for rk3399")

CLKREQ# is used for PCI-PM L1.x, but it's not functional for rk3399, so
we have to support CPM(clock power management), thus I kill them last
year.




pcie_clkreqn_cpm: pci-clkreqn-cpm {
rockchip,pins =
-   <2 RK_PD2 RK_FUNC_GPIO _pull_none>;
+   <2 26 RK_FUNC_GPIO _pull_none>;
};

pcie_clkreqnb_cpm: pci-clkreqnb-cpm {
rockchip,pins =
-   <4 RK_PD0 RK_FUNC_GPIO _pull_none>;
+   <4 24 RK_FUNC_GPIO _pull_none>;
};
};

--
2.14.4



Could we actually use RK_Pxx for all new pin definitions? Would increase 
readability a lot.

Thanks,
Klaus





--
Best Regards
Shawn Lin

Re: [PATCH] ARM64: dts: rockchip: add some pins to rk3399

[Shawn Lin]

On 2018/6/13 2:21, klaus.go...@theobroma-systems.com wrote:

Hi Randy,


-8<-


pcie {
+   pcie_clkreqn: pci-clkreqn {
+   rockchip,pins =
+   <2 26 RK_FUNC_2 _pull_none>;
+   };
+
+   pcie_clkreqnb: pci-clkreqnb {
+   rockchip,pins =
+   <4 24 RK_FUNC_1 _pull_none>;
+   };
+


I’m not sure if pci-clkreqn is functional at all. If not I’m not sure if we 
should add it to the dtsi.
Shawn may know more about it.


Please refer to commit 461a00bb9d539e
("arm64: dts: rockchip: kill pcie_clkreqn and pcie_clkreqnb for rk3399")

CLKREQ# is used for PCI-PM L1.x, but it's not functional for rk3399, so
we have to support CPM(clock power management), thus I kill them last
year.




pcie_clkreqn_cpm: pci-clkreqn-cpm {
rockchip,pins =
-   <2 RK_PD2 RK_FUNC_GPIO _pull_none>;
+   <2 26 RK_FUNC_GPIO _pull_none>;
};

pcie_clkreqnb_cpm: pci-clkreqnb-cpm {
rockchip,pins =
-   <4 RK_PD0 RK_FUNC_GPIO _pull_none>;
+   <4 24 RK_FUNC_GPIO _pull_none>;
};
};

--
2.14.4



Could we actually use RK_Pxx for all new pin definitions? Would increase 
readability a lot.

Thanks,
Klaus





--
Best Regards
Shawn Lin

Re: [PATCH] mmc: Move the mmc driver init earlier

[Shawn Lin]

On 2018/6/12 18:29, Ulf Hansson wrote:

On 12 June 2018 at 10:42, Feng Tang  wrote:

Hi Ulf,

Thanks for the review.

On Tue, Jun 12, 2018 at 08:25:44AM +0200, Ulf Hansson wrote:

On 8 June 2018 at 11:51, Feng Tang  wrote:

When doing some boot time optimization for an eMMC rootfs NUCs,
we found the rootfs may spend around 100 microseconds waiting
for eMMC card to be initialized, then the rootfs could be
mounted.
 [1.216561] Waiting for root device /dev/mmcblk1p1...
 [1.289262] mmc1: new HS400 MMC card at address 0001
 [1.289667] mmcblk1: mmc1:0001 R1J56L 14.7 GiB
 [1.289772] mmcblk1boot0: mmc1:0001 R1J56L partition 1 8.00 MiB
 [1.289869] mmcblk1boot1: mmc1:0001 R1J56L partition 2 8.00 MiB
 [1.289967] mmcblk1rpmb: mmc1:0001 R1J56L partition 3 4.00 MiB
 [1.292798]  mmcblk1: p1 p2 p3
 [1.300576] EXT4-fs (mmcblk1p1): couldn't mount as ext3 due to 
feature incompatibilities
 [1.300912] EXT4-fs (mmcblk1p1): couldn't mount as ext2 due to 
feature incompatibilities

And this is a common problem for smartphones, tablets, embedded
devices and automotive products. This patch will make the eMMC/SD
card  start initializing earlier, by changing its order in drivers/Makefile.

On our platform, the waiting for eMMC card is almost eliminated with the patch,
which is critical to boot time.


I am wondering what kernel version you are running here. There have
been some changes to the mmc initialization path, which perhaps can
help.

These logs in commit msg are based on kernel 4.14, and the patch is generated
against kernel 4.17.


Right. So it's quite recent, even if lot's of changes have been made
to the mmc core since then.

A few things (old/new) that is important.
1) Check if your mmc host driver support MMC_CAP_WAIT_WHILE_BUSY. That
should have some effect, avoiding unnecessary polling.

2) Since 4.18 rc1, you will be able to configure an over estimated
"power on" delay (via DT as well). Look at commit
6d796c68cd15234a33a4bd2ef7231125fea2dc6c.


Sorry to chime in. We could also reduce the "power on" delay via hosts'
->set_ios() callback.



3) If you use a DT based platform, I think what people do is to
re-organize the order of device nodes, such that as many as possible
-EPROBE_DEFER is avoided to be returned by drivers. This is also not a
good solution, but the best we have at this moment.


Also DT based platform is allowed to add no-sd and no-sdio for the eMMC
node, which could slighly help reduce the boot time.
You could refer to Documentation/devicetree/bindings/mmc/mmc.txt for
details.









Signed-off-by: Feng Tang 
---
  drivers/Makefile | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/Makefile b/drivers/Makefile
index 24cd47014657..c473afd3c688 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -50,6 +50,9 @@ obj-$(CONFIG_REGULATOR)   += regulator/
  # reset controllers early, since gpu drivers might rely on them to initialize
  obj-$(CONFIG_RESET_CONTROLLER) += reset/

+# put mmc early as many morden devices use emm/sd card as rootfs storage
+obj-y  += mmc/
+


Your suggested approach isn't really a solution, as it may work for
your particular case but not for everybody else.


Do you mean the patch may break some platforms? Yes, I only tested on
some IA based NUCs, and I did think about other architectures, things
that may affect MMC are gpio/clk/pinctrl, and those are still earlier
than mmc after change.


I don't know if it breaks things, potentially it could, if drivers
don't implement support for -EPROBE_DEFER properly.

However, more importantly, it's not real fix to the problem, just
something that seems to work for you.

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html






--
Best Regards
Shawn Lin

Re: [PATCH] mmc: Move the mmc driver init earlier

[Shawn Lin]

On 2018/6/12 18:29, Ulf Hansson wrote:

On 12 June 2018 at 10:42, Feng Tang  wrote:

Hi Ulf,

Thanks for the review.

On Tue, Jun 12, 2018 at 08:25:44AM +0200, Ulf Hansson wrote:

On 8 June 2018 at 11:51, Feng Tang  wrote:

When doing some boot time optimization for an eMMC rootfs NUCs,
we found the rootfs may spend around 100 microseconds waiting
for eMMC card to be initialized, then the rootfs could be
mounted.
 [1.216561] Waiting for root device /dev/mmcblk1p1...
 [1.289262] mmc1: new HS400 MMC card at address 0001
 [1.289667] mmcblk1: mmc1:0001 R1J56L 14.7 GiB
 [1.289772] mmcblk1boot0: mmc1:0001 R1J56L partition 1 8.00 MiB
 [1.289869] mmcblk1boot1: mmc1:0001 R1J56L partition 2 8.00 MiB
 [1.289967] mmcblk1rpmb: mmc1:0001 R1J56L partition 3 4.00 MiB
 [1.292798]  mmcblk1: p1 p2 p3
 [1.300576] EXT4-fs (mmcblk1p1): couldn't mount as ext3 due to 
feature incompatibilities
 [1.300912] EXT4-fs (mmcblk1p1): couldn't mount as ext2 due to 
feature incompatibilities

And this is a common problem for smartphones, tablets, embedded
devices and automotive products. This patch will make the eMMC/SD
card  start initializing earlier, by changing its order in drivers/Makefile.

On our platform, the waiting for eMMC card is almost eliminated with the patch,
which is critical to boot time.


I am wondering what kernel version you are running here. There have
been some changes to the mmc initialization path, which perhaps can
help.

These logs in commit msg are based on kernel 4.14, and the patch is generated
against kernel 4.17.


Right. So it's quite recent, even if lot's of changes have been made
to the mmc core since then.

A few things (old/new) that is important.
1) Check if your mmc host driver support MMC_CAP_WAIT_WHILE_BUSY. That
should have some effect, avoiding unnecessary polling.

2) Since 4.18 rc1, you will be able to configure an over estimated
"power on" delay (via DT as well). Look at commit
6d796c68cd15234a33a4bd2ef7231125fea2dc6c.


Sorry to chime in. We could also reduce the "power on" delay via hosts'
->set_ios() callback.



3) If you use a DT based platform, I think what people do is to
re-organize the order of device nodes, such that as many as possible
-EPROBE_DEFER is avoided to be returned by drivers. This is also not a
good solution, but the best we have at this moment.


Also DT based platform is allowed to add no-sd and no-sdio for the eMMC
node, which could slighly help reduce the boot time.
You could refer to Documentation/devicetree/bindings/mmc/mmc.txt for
details.









Signed-off-by: Feng Tang 
---
  drivers/Makefile | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/Makefile b/drivers/Makefile
index 24cd47014657..c473afd3c688 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -50,6 +50,9 @@ obj-$(CONFIG_REGULATOR)   += regulator/
  # reset controllers early, since gpu drivers might rely on them to initialize
  obj-$(CONFIG_RESET_CONTROLLER) += reset/

+# put mmc early as many morden devices use emm/sd card as rootfs storage
+obj-y  += mmc/
+


Your suggested approach isn't really a solution, as it may work for
your particular case but not for everybody else.


Do you mean the patch may break some platforms? Yes, I only tested on
some IA based NUCs, and I did think about other architectures, things
that may affect MMC are gpio/clk/pinctrl, and those are still earlier
than mmc after change.


I don't know if it breaks things, potentially it could, if drivers
don't implement support for -EPROBE_DEFER properly.

However, more importantly, it's not real fix to the problem, just
something that seems to work for you.

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html






--
Best Regards
Shawn Lin

Re: [PATCH] mmc: dw_mmc: fix card threshold control configuration

[Shawn Lin]

On 2018/6/11 20:20, Ulf Hansson wrote:

+ Shawn Lin, Evgeniy Didin, Doug Andersson

On 29 May 2018 at 12:38, Qing Xia  wrote:

From: x00270170 

Card write threshold control is supposed to be set since controller
version 2.80a for data write in HS400 mode and data read in
HS200/HS400/SDR104 mode. However the current code returns without
configuring it in the case of data writing in HS400 mode.
Meanwhile the patch fixes that the current code goes to
'disable' when doing data reading in HS400 mode.



I'm more or less unable to review this, as I don't have 2.80a databook,
nor a such platform to verify it. :(


Signed-off-by: Qing Xia 


This looks good to me. However, it seems like Jaehoon has been busy,
no response yet.

I have looped in a few more people to see if they thinks this makes sense.

Kind regards
Uffe


---
  drivers/mmc/host/dw_mmc.c | 7 ---
  1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 29a1afa..3ee8f57 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1065,8 +1065,8 @@ static void dw_mci_ctrl_thld(struct dw_mci *host, struct 
mmc_data *data)
  * It's used when HS400 mode is enabled.
  */
 if (data->flags & MMC_DATA_WRITE &&
-   !(host->timing != MMC_TIMING_MMC_HS400))
-   return;
+   host->timing != MMC_TIMING_MMC_HS400)
+   goto disable;

 if (data->flags & MMC_DATA_WRITE)
 enable = SDMMC_CARD_WR_THR_EN;
@@ -1074,7 +1074,8 @@ static void dw_mci_ctrl_thld(struct dw_mci *host, struct 
mmc_data *data)
 enable = SDMMC_CARD_RD_THR_EN;

 if (host->timing != MMC_TIMING_MMC_HS200 &&
-   host->timing != MMC_TIMING_UHS_SDR104)
+   host->timing != MMC_TIMING_UHS_SDR104 &&
+   host->timing != MMC_TIMING_MMC_HS400)
 goto disable;

 blksz_depth = blksz / (1 << host->data_shift);
--
2.8.1








--
Best Regards
Shawn Lin

Re: [PATCH] mmc: dw_mmc: fix card threshold control configuration

[Shawn Lin]

On 2018/6/11 20:20, Ulf Hansson wrote:

+ Shawn Lin, Evgeniy Didin, Doug Andersson

On 29 May 2018 at 12:38, Qing Xia  wrote:

From: x00270170 

Card write threshold control is supposed to be set since controller
version 2.80a for data write in HS400 mode and data read in
HS200/HS400/SDR104 mode. However the current code returns without
configuring it in the case of data writing in HS400 mode.
Meanwhile the patch fixes that the current code goes to
'disable' when doing data reading in HS400 mode.



I'm more or less unable to review this, as I don't have 2.80a databook,
nor a such platform to verify it. :(


Signed-off-by: Qing Xia 


This looks good to me. However, it seems like Jaehoon has been busy,
no response yet.

I have looped in a few more people to see if they thinks this makes sense.

Kind regards
Uffe


---
  drivers/mmc/host/dw_mmc.c | 7 ---
  1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 29a1afa..3ee8f57 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1065,8 +1065,8 @@ static void dw_mci_ctrl_thld(struct dw_mci *host, struct 
mmc_data *data)
  * It's used when HS400 mode is enabled.
  */
 if (data->flags & MMC_DATA_WRITE &&
-   !(host->timing != MMC_TIMING_MMC_HS400))
-   return;
+   host->timing != MMC_TIMING_MMC_HS400)
+   goto disable;

 if (data->flags & MMC_DATA_WRITE)
 enable = SDMMC_CARD_WR_THR_EN;
@@ -1074,7 +1074,8 @@ static void dw_mci_ctrl_thld(struct dw_mci *host, struct 
mmc_data *data)
 enable = SDMMC_CARD_RD_THR_EN;

 if (host->timing != MMC_TIMING_MMC_HS200 &&
-   host->timing != MMC_TIMING_UHS_SDR104)
+   host->timing != MMC_TIMING_UHS_SDR104 &&
+   host->timing != MMC_TIMING_MMC_HS400)
 goto disable;

 blksz_depth = blksz / (1 << host->data_shift);
--
2.8.1








--
Best Regards
Shawn Lin

Re: [PATCH] mmc: block: propagate correct returned value in mmc_rpmb_ioctl

[Shawn Lin]

On 2018/5/17 14:16, Mathieu Malaterre wrote:

On Thu, May 17, 2018 at 4:45 AM, Shawn Lin <shawn@rock-chips.com> wrote:


On 2018/5/17 3:20, Mathieu Malaterre wrote:


In commit 97548575bef3 ("mmc: block: Convert RPMB to a character device")
a
new function `mmc_rpmb_ioctl` was added. The final return is simply
returning a value of `0` instead of propagating the correct return code.

Discovered during a compilation with W=1, silence the following gcc
warning

drivers/mmc/core/block.c:2470:6: warning: variable ‘ret’ set but not
used [-Wunused-but-set-variable]



Good catch! But hey, which gcc are you using now? Mine, gcc-linaro-
6.3.1-2017.05-x86_64_aarch64-linux-gnu, with W=1 doesn't warn about
this.


$ powerpc-linux-gnu-gcc --version
powerpc-linux-gnu-gcc (Debian 6.3.0-18) 6.3.0 20170516
Copyright (C) 2016 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.



Thanks for sharing.


This is the default powerpc cross compiler on Debian stable (no
change). Config is simply a ppc32 (with some minor customization).



And it's worth backporting to stable.


I am not familiar with the process, do I need to submit a v2 to add the line:

Cc: <sta...@vger.kernel.org> # v4.15+



In general Ulf could kindly help with it when applied. :)


?


Reviewed-by: Shawn Lin <shawn@rock-chips.com>



Signed-off-by: Mathieu Malaterre <ma...@debian.org>
---
   drivers/mmc/core/block.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 9e923cd1d80e..38a7586b00cc 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -2485,7 +2485,7 @@ static long mmc_rpmb_ioctl(struct file *filp,
unsigned int cmd,
 break;
 }
   - return 0;
+   return ret;
   }
 #ifdef CONFIG_COMPAT




--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mmc: block: propagate correct returned value in mmc_rpmb_ioctl

[Shawn Lin]

On 2018/5/17 14:16, Mathieu Malaterre wrote:

On Thu, May 17, 2018 at 4:45 AM, Shawn Lin  wrote:


On 2018/5/17 3:20, Mathieu Malaterre wrote:


In commit 97548575bef3 ("mmc: block: Convert RPMB to a character device")
a
new function `mmc_rpmb_ioctl` was added. The final return is simply
returning a value of `0` instead of propagating the correct return code.

Discovered during a compilation with W=1, silence the following gcc
warning

drivers/mmc/core/block.c:2470:6: warning: variable ‘ret’ set but not
used [-Wunused-but-set-variable]



Good catch! But hey, which gcc are you using now? Mine, gcc-linaro-
6.3.1-2017.05-x86_64_aarch64-linux-gnu, with W=1 doesn't warn about
this.


$ powerpc-linux-gnu-gcc --version
powerpc-linux-gnu-gcc (Debian 6.3.0-18) 6.3.0 20170516
Copyright (C) 2016 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.



Thanks for sharing.


This is the default powerpc cross compiler on Debian stable (no
change). Config is simply a ppc32 (with some minor customization).



And it's worth backporting to stable.


I am not familiar with the process, do I need to submit a v2 to add the line:

Cc:  # v4.15+



In general Ulf could kindly help with it when applied. :)


?


Reviewed-by: Shawn Lin 



Signed-off-by: Mathieu Malaterre 
---
   drivers/mmc/core/block.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 9e923cd1d80e..38a7586b00cc 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -2485,7 +2485,7 @@ static long mmc_rpmb_ioctl(struct file *filp,
unsigned int cmd,
 break;
 }
   - return 0;
+   return ret;
   }
 #ifdef CONFIG_COMPAT




--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mmc: block: propagate correct returned value in mmc_rpmb_ioctl

[Shawn Lin]

On 2018/5/17 3:20, Mathieu Malaterre wrote:

In commit 97548575bef3 ("mmc: block: Convert RPMB to a character device") a
new function `mmc_rpmb_ioctl` was added. The final return is simply
returning a value of `0` instead of propagating the correct return code.

Discovered during a compilation with W=1, silence the following gcc warning

   drivers/mmc/core/block.c:2470:6: warning: variable ‘ret’ set but not used 
[-Wunused-but-set-variable]



Good catch! But hey, which gcc are you using now? Mine, gcc-linaro-
6.3.1-2017.05-x86_64_aarch64-linux-gnu, with W=1 doesn't warn about
this. And it's worth backporting to stable.

Reviewed-by: Shawn Lin <shawn@rock-chips.com>


Signed-off-by: Mathieu Malaterre <ma...@debian.org>
---
  drivers/mmc/core/block.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 9e923cd1d80e..38a7586b00cc 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -2485,7 +2485,7 @@ static long mmc_rpmb_ioctl(struct file *filp, unsigned 
int cmd,
break;
}
  
-	return 0;

+   return ret;
  }
  
  #ifdef CONFIG_COMPAT

Re: [PATCH] mmc: block: propagate correct returned value in mmc_rpmb_ioctl

[Shawn Lin]

On 2018/5/17 3:20, Mathieu Malaterre wrote:

In commit 97548575bef3 ("mmc: block: Convert RPMB to a character device") a
new function `mmc_rpmb_ioctl` was added. The final return is simply
returning a value of `0` instead of propagating the correct return code.

Discovered during a compilation with W=1, silence the following gcc warning

   drivers/mmc/core/block.c:2470:6: warning: variable ‘ret’ set but not used 
[-Wunused-but-set-variable]



Good catch! But hey, which gcc are you using now? Mine, gcc-linaro-
6.3.1-2017.05-x86_64_aarch64-linux-gnu, with W=1 doesn't warn about
this. And it's worth backporting to stable.

Reviewed-by: Shawn Lin 


Signed-off-by: Mathieu Malaterre 
---
  drivers/mmc/core/block.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 9e923cd1d80e..38a7586b00cc 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -2485,7 +2485,7 @@ static long mmc_rpmb_ioctl(struct file *filp, unsigned 
int cmd,
break;
}
  
-	return 0;

+   return ret;
  }
  
  #ifdef CONFIG_COMPAT

Re: [PATCH] ethernet: stmmac: dwmac-rk: Add GMAC support for px30

[Shawn Lin]

Hi David,

On 2018/5/16 11:38, David Wu wrote:

Add constants and callback functions for the dwmac on px30 soc.


s/soc/SoC


The base structure is the same, but registers and the bits in
them moved slightly, and add the clk_mac_speed for the select


s/moved/are moved


of mac speed.


for selecting mas speed.



Signed-off-by: David Wu 


git log --oneline  drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c

shows very inconsistent format wrt. commit title, so please
follow the exsiting exsamples as possible.


---
  .../devicetree/bindings/net/rockchip-dwmac.txt |  1 +
  drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 64 ++
  2 files changed, 65 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt 
b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
index 9c16ee2..3b71da7 100644
--- a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
+++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt


It'd be better to split doc changes into a separate patch.


@@ -4,6 +4,7 @@ The device node has following properties.
  
  Required properties:

   - compatible: should be "rockchip,-gamc"
+   "rockchip,px30-gmac":   found on PX30 SoCs
 "rockchip,rk3128-gmac": found on RK312x SoCs
 "rockchip,rk3228-gmac": found on RK322x SoCs
 "rockchip,rk3288-gmac": found on RK3288 SoCs
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c 
b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
index 13133b3..4b2ab71 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
@@ -61,6 +61,7 @@ struct rk_priv_data {
struct clk *mac_clk_tx;
struct clk *clk_mac_ref;
struct clk *clk_mac_refout;
+   struct clk *clk_mac_speed;


No need to do anything now but it seems you could consider doing some
cleanup by using clk bulk APIs in the future.


struct clk *aclk_mac;
struct clk *pclk_mac;
struct clk *clk_phy;
@@ -83,6 +84,64 @@ struct rk_priv_data {
(((tx) ? soc##_GMAC_TXCLK_DLY_ENABLE : soc##_GMAC_TXCLK_DLY_DISABLE) | \
 ((rx) ? soc##_GMAC_RXCLK_DLY_ENABLE : soc##_GMAC_RXCLK_DLY_DISABLE))
  
+#define PX30_GRF_GMAC_CON1		0X0904


s/0X0904/0x0904 , since the other constants in this file follow the
same format.


+
+/* PX30_GRF_GMAC_CON1 */
+#define PX30_GMAC_PHY_INTF_SEL_RMII(GRF_CLR_BIT(4) | GRF_CLR_BIT(5) | \
+   GRF_BIT(6))
+#define PX30_GMAC_SPEED_10MGRF_CLR_BIT(2)
+#define PX30_GMAC_SPEED_100M   GRF_BIT(2)
+
+static void px30_set_to_rmii(struct rk_priv_data *bsp_priv)
+{
+   struct device *dev = _priv->pdev->dev;
+
+   if (IS_ERR(bsp_priv->grf)) {
+   dev_err(dev, "%s: Missing rockchip,grf property\n", __func__);
+   return;
+   }
+
+   regmap_write(bsp_priv->grf, PX30_GRF_GMAC_CON1,
+PX30_GMAC_PHY_INTF_SEL_RMII);
+}
+
+static void px30_set_rmii_speed(struct rk_priv_data *bsp_priv, int speed)
+{
+   struct device *dev = _priv->pdev->dev;
+   int ret;
+
+   if (IS_ERR(bsp_priv->clk_mac_speed)) {
+   dev_err(dev, "%s: Missing clk_mac_speed clock\n", __func__);
+   return;
+   }
+
+   if (speed == 10) {
+   regmap_write(bsp_priv->grf, PX30_GRF_GMAC_CON1,
+PX30_GMAC_SPEED_10M);
+
+   ret = clk_set_rate(bsp_priv->clk_mac_speed, 250);
+   if (ret)
+   dev_err(dev, "%s: set clk_mac_speed rate 250 failed: 
%d\n",
+   __func__, ret);
+   } else if (speed == 100) {
+   regmap_write(bsp_priv->grf, PX30_GRF_GMAC_CON1,
+PX30_GMAC_SPEED_100M);
+
+   ret = clk_set_rate(bsp_priv->clk_mac_speed, 2500);
+   if (ret)
+   dev_err(dev, "%s: set clk_mac_speed rate 2500 failed: 
%d\n",
+   __func__, ret);


I know it follows the existing examples, but IMHO it duplicates
unnecessary code as all the difference is PX30_GMAC_SPEED_*


+
+   } else {
+   dev_err(dev, "unknown speed value for RMII! speed=%d", speed);
+   }
+}
+
+static const struct rk_gmac_ops px30_ops = {
+   .set_to_rmii = px30_set_to_rmii,
+   .set_rmii_speed = px30_set_rmii_speed,
+};
+
  #define RK3128_GRF_MAC_CON0   0x0168
  #define RK3128_GRF_MAC_CON1   0x016c
  
@@ -1042,6 +1101,10 @@ static int rk_gmac_clk_init(struct plat_stmmacenet_data *plat)

}
}
  
+	bsp_priv->clk_mac_speed = devm_clk_get(dev, "clk_mac_speed");


Mightbe it'd be better to use "mac-speed" in DT bindings.


+   if (IS_ERR(bsp_priv->clk_mac_speed))
+   dev_err(dev, "cannot get clock %s\n", "clk_mac_speed");
+


Would you like to handle deferred probe?


if (bsp_priv->clock_input) {
dev_info(dev, 

Re: [PATCH] ethernet: stmmac: dwmac-rk: Add GMAC support for px30

[Shawn Lin]

Hi David,

On 2018/5/16 11:38, David Wu wrote:

Add constants and callback functions for the dwmac on px30 soc.


s/soc/SoC


The base structure is the same, but registers and the bits in
them moved slightly, and add the clk_mac_speed for the select


s/moved/are moved


of mac speed.


for selecting mas speed.



Signed-off-by: David Wu 


git log --oneline  drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c

shows very inconsistent format wrt. commit title, so please
follow the exsiting exsamples as possible.


---
  .../devicetree/bindings/net/rockchip-dwmac.txt |  1 +
  drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 64 ++
  2 files changed, 65 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt 
b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
index 9c16ee2..3b71da7 100644
--- a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt
+++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt


It'd be better to split doc changes into a separate patch.


@@ -4,6 +4,7 @@ The device node has following properties.
  
  Required properties:

   - compatible: should be "rockchip,-gamc"
+   "rockchip,px30-gmac":   found on PX30 SoCs
 "rockchip,rk3128-gmac": found on RK312x SoCs
 "rockchip,rk3228-gmac": found on RK322x SoCs
 "rockchip,rk3288-gmac": found on RK3288 SoCs
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c 
b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
index 13133b3..4b2ab71 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
@@ -61,6 +61,7 @@ struct rk_priv_data {
struct clk *mac_clk_tx;
struct clk *clk_mac_ref;
struct clk *clk_mac_refout;
+   struct clk *clk_mac_speed;


No need to do anything now but it seems you could consider doing some
cleanup by using clk bulk APIs in the future.


struct clk *aclk_mac;
struct clk *pclk_mac;
struct clk *clk_phy;
@@ -83,6 +84,64 @@ struct rk_priv_data {
(((tx) ? soc##_GMAC_TXCLK_DLY_ENABLE : soc##_GMAC_TXCLK_DLY_DISABLE) | \
 ((rx) ? soc##_GMAC_RXCLK_DLY_ENABLE : soc##_GMAC_RXCLK_DLY_DISABLE))
  
+#define PX30_GRF_GMAC_CON1		0X0904


s/0X0904/0x0904 , since the other constants in this file follow the
same format.


+
+/* PX30_GRF_GMAC_CON1 */
+#define PX30_GMAC_PHY_INTF_SEL_RMII(GRF_CLR_BIT(4) | GRF_CLR_BIT(5) | \
+   GRF_BIT(6))
+#define PX30_GMAC_SPEED_10MGRF_CLR_BIT(2)
+#define PX30_GMAC_SPEED_100M   GRF_BIT(2)
+
+static void px30_set_to_rmii(struct rk_priv_data *bsp_priv)
+{
+   struct device *dev = _priv->pdev->dev;
+
+   if (IS_ERR(bsp_priv->grf)) {
+   dev_err(dev, "%s: Missing rockchip,grf property\n", __func__);
+   return;
+   }
+
+   regmap_write(bsp_priv->grf, PX30_GRF_GMAC_CON1,
+PX30_GMAC_PHY_INTF_SEL_RMII);
+}
+
+static void px30_set_rmii_speed(struct rk_priv_data *bsp_priv, int speed)
+{
+   struct device *dev = _priv->pdev->dev;
+   int ret;
+
+   if (IS_ERR(bsp_priv->clk_mac_speed)) {
+   dev_err(dev, "%s: Missing clk_mac_speed clock\n", __func__);
+   return;
+   }
+
+   if (speed == 10) {
+   regmap_write(bsp_priv->grf, PX30_GRF_GMAC_CON1,
+PX30_GMAC_SPEED_10M);
+
+   ret = clk_set_rate(bsp_priv->clk_mac_speed, 250);
+   if (ret)
+   dev_err(dev, "%s: set clk_mac_speed rate 250 failed: 
%d\n",
+   __func__, ret);
+   } else if (speed == 100) {
+   regmap_write(bsp_priv->grf, PX30_GRF_GMAC_CON1,
+PX30_GMAC_SPEED_100M);
+
+   ret = clk_set_rate(bsp_priv->clk_mac_speed, 2500);
+   if (ret)
+   dev_err(dev, "%s: set clk_mac_speed rate 2500 failed: 
%d\n",
+   __func__, ret);


I know it follows the existing examples, but IMHO it duplicates
unnecessary code as all the difference is PX30_GMAC_SPEED_*


+
+   } else {
+   dev_err(dev, "unknown speed value for RMII! speed=%d", speed);
+   }
+}
+
+static const struct rk_gmac_ops px30_ops = {
+   .set_to_rmii = px30_set_to_rmii,
+   .set_rmii_speed = px30_set_rmii_speed,
+};
+
  #define RK3128_GRF_MAC_CON0   0x0168
  #define RK3128_GRF_MAC_CON1   0x016c
  
@@ -1042,6 +1101,10 @@ static int rk_gmac_clk_init(struct plat_stmmacenet_data *plat)

}
}
  
+	bsp_priv->clk_mac_speed = devm_clk_get(dev, "clk_mac_speed");


Mightbe it'd be better to use "mac-speed" in DT bindings.


+   if (IS_ERR(bsp_priv->clk_mac_speed))
+   dev_err(dev, "cannot get clock %s\n", "clk_mac_speed");
+


Would you like to handle deferred probe?


if (bsp_priv->clock_input) {
dev_info(dev, "clock input from 

Re: [PATCH v7 1/3] mmc: dw_mmc-bluefield: Add driver extension

[Shawn Lin]

On 2018/5/9 2:46, Liming Sun wrote:

This commit adds extension to the dw_mmc driver for Mellanox BlueField
SoC. It updates the UHS_REG_EXT register to bring up the eMMC card on
this SoC.



Reviewed-by: Shawn Lin <shawn@rock-chips.com>

Re: [PATCH v7 1/3] mmc: dw_mmc-bluefield: Add driver extension

[Shawn Lin]

On 2018/5/9 2:46, Liming Sun wrote:

This commit adds extension to the dw_mmc driver for Mellanox BlueField
SoC. It updates the UHS_REG_EXT register to bring up the eMMC card on
this SoC.



Reviewed-by: Shawn Lin 

Re: [PATCH v5 1/3] mmc: dw_mmc-bluefield: Add driver extension

[Shawn Lin]

On 2018/5/2 20:45, Liming Sun wrote:

Please see response inline.

Thanks,
Liming


-Original Message-
From: Shawn Lin [mailto:shawn@rock-chips.com]
Sent: Tuesday, May 1, 2018 9:02 PM
To: Liming Sun <l...@mellanox.com>; Mark Rutland
<mark.rutl...@arm.com>; Jaehoon Chung <jh80.ch...@samsung.com>;
Catalin Marinas <catalin.mari...@arm.com>; Will Deacon
<will.dea...@arm.com>
Cc: Ulf Hansson <ulf.hans...@linaro.org>; Rob Herring
<robh...@kernel.org>; shawn@rock-chips.com; linux-
m...@vger.kernel.org; devicet...@vger.kernel.org; linux-
ker...@vger.kernel.org; sta...@kernel.org
Subject: Re: [PATCH v5 1/3] mmc: dw_mmc-bluefield: Add driver extension

On 2018/5/2 2:19, Liming Sun wrote:

This commit adds extension to the dw_mmc driver for Mellanox BlueField
SoC. It updates the UHS_REG_EXT register to bring up the eMMC card on
this SoC.

Cc: sta...@kernel.org


Why?


[Liming] This is recommended value from HW team. Without this setting, the eMMC
controller and cards can be found in Linux, but not stable. Writing to it could 
caused
CRC error.


Well, I refered to the "Cc: sta...@kernel.org" in the commit msg.






Signed-off-by: Liming Sun <l...@mellanox.com>
Reviewed-by: David Woods <dwo...@mellanox.com>
---
   drivers/mmc/host/Kconfig|  9 +
   drivers/mmc/host/Makefile   |  1 +
   drivers/mmc/host/dw_mmc-bluefield.c | 72

+

   3 files changed, 82 insertions(+)
   create mode 100644 drivers/mmc/host/dw_mmc-bluefield.c

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 9589f9c..26ac6b5 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -717,6 +717,15 @@ config MMC_DW_K3
  Synopsys DesignWare Memory Card Interface driver. Select this

option

  for platforms based on Hisilicon K3 SoC's.

+config MMC_DW_BLUEFIELD


And did you have feedback of my comment in V2？
http://lists-archives.com/linux-kernel/29104830-mmc-dw_mmc-bluefield-
add-driver-extension.html


+   tristate "BlueField specific extensions for Synopsys DW Memory Card

Interface"

+   depends on MMC_DW
+   select MMC_DW_PLTFM
+   help
+ This selects support for Mellanox BlueField SoC specific extensions

to

+ the Synopsys DesignWare Memory Card Interface driver. Select this
+ option for platforms based on Mellanox BlueField SoC's.
+
   config MMC_DW_PCI
tristate "Synopsys Designware MCI support on PCI bus"
depends on MMC_DW && PCI
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 6aead24..267b3f1 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_MMC_DW_K3)   += dw_mmc-

k3.o

   obj-$(CONFIG_MMC_DW_PCI) += dw_mmc-pci.o
   obj-$(CONFIG_MMC_DW_ROCKCHIP)+= dw_mmc-rockchip.o
   obj-$(CONFIG_MMC_DW_ZX)  += dw_mmc-zx.o
+obj-$(CONFIG_MMC_DW_BLUEFIELD) += dw_mmc-bluefield.o
   obj-$(CONFIG_MMC_SH_MMCIF)   += sh_mmcif.o
   obj-$(CONFIG_MMC_JZ4740) += jz4740_mmc.o
   obj-$(CONFIG_MMC_VUB300) += vub300.o
diff --git a/drivers/mmc/host/dw_mmc-bluefield.c

b/drivers/mmc/host/dw_mmc-bluefield.c

new file mode 100644
index 000..12067b1
--- /dev/null
+++ b/drivers/mmc/host/dw_mmc-bluefield.c
@@ -0,0 +1,72 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Mellanox Technologies.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "dw_mmc.h"
+#include "dw_mmc-pltfm.h"
+
+static void dw_mci_bluefield_set_ios(struct dw_mci *host, struct

mmc_ios *ios)

+{
+   u32 regs;
+
+   /* Set drive=4 (bit 29:23) and sample=2 (bit 22:16) in UHS_REG_EXT.

*/

+   regs = mci_readl(host, UHS_REG_EXT);
+   regs = (regs & ~0x3F10 & ~0x7F) | (4 << 23) | (2 << 16);
+   mci_writel(host, UHS_REG_EXT, regs);
+}
+
+static const struct dw_mci_drv_data bluefield_drv_data = {
+   .set_ios= dw_mci_bluefield_set_ios
+};
+
+static const struct of_device_id dw_mci_bluefield_match[] = {
+   { .compatible = "mellanox,bluefield-dw-mshc",
+   .data = _drv_data },
+   {},
+};
+MODULE_DEVICE_TABLE(of, dw_mci_bluefield_match);
+
+static int dw_mci_bluefield_probe(struct platform_device *pdev)
+{
+   const struct dw_mci_drv_data *drv_data = NULL;
+   const struct of_device_id *match;
+
+   if (pdev->dev.of_node) {
+   match = of_match_node(dw_mci_bluefield_match,
+ pdev->dev.of_

Re: [PATCH v5 1/3] mmc: dw_mmc-bluefield: Add driver extension

[Shawn Lin]

On 2018/5/2 20:45, Liming Sun wrote:

Please see response inline.

Thanks,
Liming


-Original Message-
From: Shawn Lin [mailto:shawn@rock-chips.com]
Sent: Tuesday, May 1, 2018 9:02 PM
To: Liming Sun ; Mark Rutland
; Jaehoon Chung ;
Catalin Marinas ; Will Deacon

Cc: Ulf Hansson ; Rob Herring
; shawn@rock-chips.com; linux-
m...@vger.kernel.org; devicet...@vger.kernel.org; linux-
ker...@vger.kernel.org; sta...@kernel.org
Subject: Re: [PATCH v5 1/3] mmc: dw_mmc-bluefield: Add driver extension

On 2018/5/2 2:19, Liming Sun wrote:

This commit adds extension to the dw_mmc driver for Mellanox BlueField
SoC. It updates the UHS_REG_EXT register to bring up the eMMC card on
this SoC.

Cc: sta...@kernel.org


Why?


[Liming] This is recommended value from HW team. Without this setting, the eMMC
controller and cards can be found in Linux, but not stable. Writing to it could 
caused
CRC error.


Well, I refered to the "Cc: sta...@kernel.org" in the commit msg.






Signed-off-by: Liming Sun 
Reviewed-by: David Woods 
---
   drivers/mmc/host/Kconfig|  9 +
   drivers/mmc/host/Makefile   |  1 +
   drivers/mmc/host/dw_mmc-bluefield.c | 72

+

   3 files changed, 82 insertions(+)
   create mode 100644 drivers/mmc/host/dw_mmc-bluefield.c

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 9589f9c..26ac6b5 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -717,6 +717,15 @@ config MMC_DW_K3
  Synopsys DesignWare Memory Card Interface driver. Select this

option

  for platforms based on Hisilicon K3 SoC's.

+config MMC_DW_BLUEFIELD


And did you have feedback of my comment in V2？
http://lists-archives.com/linux-kernel/29104830-mmc-dw_mmc-bluefield-
add-driver-extension.html


+   tristate "BlueField specific extensions for Synopsys DW Memory Card

Interface"

+   depends on MMC_DW
+   select MMC_DW_PLTFM
+   help
+ This selects support for Mellanox BlueField SoC specific extensions

to

+ the Synopsys DesignWare Memory Card Interface driver. Select this
+ option for platforms based on Mellanox BlueField SoC's.
+
   config MMC_DW_PCI
tristate "Synopsys Designware MCI support on PCI bus"
depends on MMC_DW && PCI
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 6aead24..267b3f1 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_MMC_DW_K3)   += dw_mmc-

k3.o

   obj-$(CONFIG_MMC_DW_PCI) += dw_mmc-pci.o
   obj-$(CONFIG_MMC_DW_ROCKCHIP)+= dw_mmc-rockchip.o
   obj-$(CONFIG_MMC_DW_ZX)  += dw_mmc-zx.o
+obj-$(CONFIG_MMC_DW_BLUEFIELD) += dw_mmc-bluefield.o
   obj-$(CONFIG_MMC_SH_MMCIF)   += sh_mmcif.o
   obj-$(CONFIG_MMC_JZ4740) += jz4740_mmc.o
   obj-$(CONFIG_MMC_VUB300) += vub300.o
diff --git a/drivers/mmc/host/dw_mmc-bluefield.c

b/drivers/mmc/host/dw_mmc-bluefield.c

new file mode 100644
index 000..12067b1
--- /dev/null
+++ b/drivers/mmc/host/dw_mmc-bluefield.c
@@ -0,0 +1,72 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Mellanox Technologies.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "dw_mmc.h"
+#include "dw_mmc-pltfm.h"
+
+static void dw_mci_bluefield_set_ios(struct dw_mci *host, struct

mmc_ios *ios)

+{
+   u32 regs;
+
+   /* Set drive=4 (bit 29:23) and sample=2 (bit 22:16) in UHS_REG_EXT.

*/

+   regs = mci_readl(host, UHS_REG_EXT);
+   regs = (regs & ~0x3F10 & ~0x7F) | (4 << 23) | (2 << 16);
+   mci_writel(host, UHS_REG_EXT, regs);
+}
+
+static const struct dw_mci_drv_data bluefield_drv_data = {
+   .set_ios= dw_mci_bluefield_set_ios
+};
+
+static const struct of_device_id dw_mci_bluefield_match[] = {
+   { .compatible = "mellanox,bluefield-dw-mshc",
+   .data = _drv_data },
+   {},
+};
+MODULE_DEVICE_TABLE(of, dw_mci_bluefield_match);
+
+static int dw_mci_bluefield_probe(struct platform_device *pdev)
+{
+   const struct dw_mci_drv_data *drv_data = NULL;
+   const struct of_device_id *match;
+
+   if (pdev->dev.of_node) {
+   match = of_match_node(dw_mci_bluefield_match,
+ pdev->dev.of_node);
+   drv_data = match->data;
+   }
+
+   return dw_mci_pltfm_register(pdev, drv_data);
+}
+
+static struct platform_driver dw_mci_bluefield_pltfm_driver = {
+   .probe  = dw_mci_bluefield_probe,
+   .remove = dw

Re: [PATCH v5 1/3] mmc: dw_mmc-bluefield: Add driver extension

[Shawn Lin]

On 2018/5/2 2:19, Liming Sun wrote:

This commit adds extension to the dw_mmc driver for Mellanox BlueField
SoC. It updates the UHS_REG_EXT register to bring up the eMMC card on
this SoC.

Cc: sta...@kernel.org


Why?


Signed-off-by: Liming Sun 
Reviewed-by: David Woods 
---
  drivers/mmc/host/Kconfig|  9 +
  drivers/mmc/host/Makefile   |  1 +
  drivers/mmc/host/dw_mmc-bluefield.c | 72 +
  3 files changed, 82 insertions(+)
  create mode 100644 drivers/mmc/host/dw_mmc-bluefield.c

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 9589f9c..26ac6b5 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -717,6 +717,15 @@ config MMC_DW_K3
  Synopsys DesignWare Memory Card Interface driver. Select this option
  for platforms based on Hisilicon K3 SoC's.
  
+config MMC_DW_BLUEFIELD


And did you have feedback of my comment in V2？
http://lists-archives.com/linux-kernel/29104830-mmc-dw_mmc-bluefield-add-driver-extension.html


+   tristate "BlueField specific extensions for Synopsys DW Memory Card 
Interface"
+   depends on MMC_DW
+   select MMC_DW_PLTFM
+   help
+ This selects support for Mellanox BlueField SoC specific extensions to
+ the Synopsys DesignWare Memory Card Interface driver. Select this
+ option for platforms based on Mellanox BlueField SoC's.
+
  config MMC_DW_PCI
tristate "Synopsys Designware MCI support on PCI bus"
depends on MMC_DW && PCI
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 6aead24..267b3f1 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_MMC_DW_K3)   += dw_mmc-k3.o
  obj-$(CONFIG_MMC_DW_PCI)  += dw_mmc-pci.o
  obj-$(CONFIG_MMC_DW_ROCKCHIP) += dw_mmc-rockchip.o
  obj-$(CONFIG_MMC_DW_ZX)   += dw_mmc-zx.o
+obj-$(CONFIG_MMC_DW_BLUEFIELD) += dw_mmc-bluefield.o
  obj-$(CONFIG_MMC_SH_MMCIF)+= sh_mmcif.o
  obj-$(CONFIG_MMC_JZ4740)  += jz4740_mmc.o
  obj-$(CONFIG_MMC_VUB300)  += vub300.o
diff --git a/drivers/mmc/host/dw_mmc-bluefield.c 
b/drivers/mmc/host/dw_mmc-bluefield.c
new file mode 100644
index 000..12067b1
--- /dev/null
+++ b/drivers/mmc/host/dw_mmc-bluefield.c
@@ -0,0 +1,72 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Mellanox Technologies.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "dw_mmc.h"
+#include "dw_mmc-pltfm.h"
+
+static void dw_mci_bluefield_set_ios(struct dw_mci *host, struct mmc_ios *ios)
+{
+   u32 regs;
+
+   /* Set drive=4 (bit 29:23) and sample=2 (bit 22:16) in UHS_REG_EXT. */
+   regs = mci_readl(host, UHS_REG_EXT);
+   regs = (regs & ~0x3F10 & ~0x7F) | (4 << 23) | (2 << 16);
+   mci_writel(host, UHS_REG_EXT, regs);
+}
+
+static const struct dw_mci_drv_data bluefield_drv_data = {
+   .set_ios= dw_mci_bluefield_set_ios
+};
+
+static const struct of_device_id dw_mci_bluefield_match[] = {
+   { .compatible = "mellanox,bluefield-dw-mshc",
+   .data = _drv_data },
+   {},
+};
+MODULE_DEVICE_TABLE(of, dw_mci_bluefield_match);
+
+static int dw_mci_bluefield_probe(struct platform_device *pdev)
+{
+   const struct dw_mci_drv_data *drv_data = NULL;
+   const struct of_device_id *match;
+
+   if (pdev->dev.of_node) {
+   match = of_match_node(dw_mci_bluefield_match,
+ pdev->dev.of_node);
+   drv_data = match->data;
+   }
+
+   return dw_mci_pltfm_register(pdev, drv_data);
+}
+
+static struct platform_driver dw_mci_bluefield_pltfm_driver = {
+   .probe  = dw_mci_bluefield_probe,
+   .remove = dw_mci_pltfm_remove,
+   .driver = {
+   .name   = "dwmmc_bluefield",
+   .of_match_table = dw_mci_bluefield_match,
+   .pm = _mci_pltfm_pmops,
+   },
+};
+
+module_platform_driver(dw_mci_bluefield_pltfm_driver);
+
+MODULE_DESCRIPTION("BlueField DW Multimedia Card driver");
+MODULE_AUTHOR("Mellanox Technologies");
+MODULE_LICENSE("GPL v2");

Re: [PATCH v5 1/3] mmc: dw_mmc-bluefield: Add driver extension

[Shawn Lin]

On 2018/5/2 2:19, Liming Sun wrote:

This commit adds extension to the dw_mmc driver for Mellanox BlueField
SoC. It updates the UHS_REG_EXT register to bring up the eMMC card on
this SoC.

Cc: sta...@kernel.org


Why?


Signed-off-by: Liming Sun 
Reviewed-by: David Woods 
---
  drivers/mmc/host/Kconfig|  9 +
  drivers/mmc/host/Makefile   |  1 +
  drivers/mmc/host/dw_mmc-bluefield.c | 72 +
  3 files changed, 82 insertions(+)
  create mode 100644 drivers/mmc/host/dw_mmc-bluefield.c

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 9589f9c..26ac6b5 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -717,6 +717,15 @@ config MMC_DW_K3
  Synopsys DesignWare Memory Card Interface driver. Select this option
  for platforms based on Hisilicon K3 SoC's.
  
+config MMC_DW_BLUEFIELD


And did you have feedback of my comment in V2？
http://lists-archives.com/linux-kernel/29104830-mmc-dw_mmc-bluefield-add-driver-extension.html


+   tristate "BlueField specific extensions for Synopsys DW Memory Card 
Interface"
+   depends on MMC_DW
+   select MMC_DW_PLTFM
+   help
+ This selects support for Mellanox BlueField SoC specific extensions to
+ the Synopsys DesignWare Memory Card Interface driver. Select this
+ option for platforms based on Mellanox BlueField SoC's.
+
  config MMC_DW_PCI
tristate "Synopsys Designware MCI support on PCI bus"
depends on MMC_DW && PCI
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 6aead24..267b3f1 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_MMC_DW_K3)   += dw_mmc-k3.o
  obj-$(CONFIG_MMC_DW_PCI)  += dw_mmc-pci.o
  obj-$(CONFIG_MMC_DW_ROCKCHIP) += dw_mmc-rockchip.o
  obj-$(CONFIG_MMC_DW_ZX)   += dw_mmc-zx.o
+obj-$(CONFIG_MMC_DW_BLUEFIELD) += dw_mmc-bluefield.o
  obj-$(CONFIG_MMC_SH_MMCIF)+= sh_mmcif.o
  obj-$(CONFIG_MMC_JZ4740)  += jz4740_mmc.o
  obj-$(CONFIG_MMC_VUB300)  += vub300.o
diff --git a/drivers/mmc/host/dw_mmc-bluefield.c 
b/drivers/mmc/host/dw_mmc-bluefield.c
new file mode 100644
index 000..12067b1
--- /dev/null
+++ b/drivers/mmc/host/dw_mmc-bluefield.c
@@ -0,0 +1,72 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Mellanox Technologies.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "dw_mmc.h"
+#include "dw_mmc-pltfm.h"
+
+static void dw_mci_bluefield_set_ios(struct dw_mci *host, struct mmc_ios *ios)
+{
+   u32 regs;
+
+   /* Set drive=4 (bit 29:23) and sample=2 (bit 22:16) in UHS_REG_EXT. */
+   regs = mci_readl(host, UHS_REG_EXT);
+   regs = (regs & ~0x3F10 & ~0x7F) | (4 << 23) | (2 << 16);
+   mci_writel(host, UHS_REG_EXT, regs);
+}
+
+static const struct dw_mci_drv_data bluefield_drv_data = {
+   .set_ios= dw_mci_bluefield_set_ios
+};
+
+static const struct of_device_id dw_mci_bluefield_match[] = {
+   { .compatible = "mellanox,bluefield-dw-mshc",
+   .data = _drv_data },
+   {},
+};
+MODULE_DEVICE_TABLE(of, dw_mci_bluefield_match);
+
+static int dw_mci_bluefield_probe(struct platform_device *pdev)
+{
+   const struct dw_mci_drv_data *drv_data = NULL;
+   const struct of_device_id *match;
+
+   if (pdev->dev.of_node) {
+   match = of_match_node(dw_mci_bluefield_match,
+ pdev->dev.of_node);
+   drv_data = match->data;
+   }
+
+   return dw_mci_pltfm_register(pdev, drv_data);
+}
+
+static struct platform_driver dw_mci_bluefield_pltfm_driver = {
+   .probe  = dw_mci_bluefield_probe,
+   .remove = dw_mci_pltfm_remove,
+   .driver = {
+   .name   = "dwmmc_bluefield",
+   .of_match_table = dw_mci_bluefield_match,
+   .pm = _mci_pltfm_pmops,
+   },
+};
+
+module_platform_driver(dw_mci_bluefield_pltfm_driver);
+
+MODULE_DESCRIPTION("BlueField DW Multimedia Card driver");
+MODULE_AUTHOR("Mellanox Technologies");
+MODULE_LICENSE("GPL v2");

Re: [PATCH v2 1/3] mmc: dw_mmc-bluefield: Add driver extension

[Shawn Lin]

Hi Liming,

On 2018/4/23 23:32, Liming Sun wrote:

This commit adds extension to the dw_mmc driver for Mellanox BlueField
SoC. It updates the UHS_REG_EXT register to bring up the eMMC card on
this SoC.

Signed-off-by: Liming Sun 
---
  drivers/mmc/host/Kconfig|  9 +
  drivers/mmc/host/Makefile   |  1 +
  drivers/mmc/host/dw_mmc-bluefield.c | 72 +
  3 files changed, 82 insertions(+)
  create mode 100644 drivers/mmc/host/dw_mmc-bluefield.c

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 9589f9c..26ac6b5 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -717,6 +717,15 @@ config MMC_DW_K3
  Synopsys DesignWare Memory Card Interface driver. Select this option
  for platforms based on Hisilicon K3 SoC's.
  
+config MMC_DW_BLUEFIELD

+   tristate "BlueField specific extensions for Synopsys DW Memory Card 
Interface"
+   depends on MMC_DW
+   select MMC_DW_PLTFM
+   help
+ This selects support for Mellanox BlueField SoC specific extensions to
+ the Synopsys DesignWare Memory Card Interface driver. Select this
+ option for platforms based on Mellanox BlueField SoC's.
+


It'd better to keep the order, so you could place it before
MMC_DW_EXYNOS.


  config MMC_DW_PCI
tristate "Synopsys Designware MCI support on PCI bus"
depends on MMC_DW && PCI
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 6aead24..267b3f1 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_MMC_DW_K3)   += dw_mmc-k3.o
  obj-$(CONFIG_MMC_DW_PCI)  += dw_mmc-pci.o
  obj-$(CONFIG_MMC_DW_ROCKCHIP) += dw_mmc-rockchip.o
  obj-$(CONFIG_MMC_DW_ZX)   += dw_mmc-zx.o
+obj-$(CONFIG_MMC_DW_BLUEFIELD) += dw_mmc-bluefield.o


Ditto.


  obj-$(CONFIG_MMC_SH_MMCIF)+= sh_mmcif.o
  obj-$(CONFIG_MMC_JZ4740)  += jz4740_mmc.o
  obj-$(CONFIG_MMC_VUB300)  += vub300.o
diff --git a/drivers/mmc/host/dw_mmc-bluefield.c 
b/drivers/mmc/host/dw_mmc-bluefield.c
new file mode 100644
index 000..12067b1
--- /dev/null
+++ b/drivers/mmc/host/dw_mmc-bluefield.c
@@ -0,0 +1,72 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Mellanox Technologies.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+


Ditto.


+#include "dw_mmc.h"
+#include "dw_mmc-pltfm.h"
+
+static void dw_mci_bluefield_set_ios(struct dw_mci *host, struct mmc_ios *ios)
+{
+   u32 regs;
+
+   /* Set drive=4 (bit 29:23) and sample=2 (bit 22:16) in UHS_REG_EXT. */
+   regs = mci_readl(host, UHS_REG_EXT);
+   regs = (regs & ~0x3F10 & ~0x7F) | (4 << 23) | (2 << 16);


GENMASK woule be more readable IMHO.


+   mci_writel(host, UHS_REG_EXT, regs);
+}
+
+static const struct dw_mci_drv_data bluefield_drv_data = {
+   .set_ios= dw_mci_bluefield_set_ios
+};
+
+static const struct of_device_id dw_mci_bluefield_match[] = {
+   { .compatible = "mellanox,bluefield-dw-mshc",
+   .data = _drv_data },


Keep the indent.


+   {},
+};
+MODULE_DEVICE_TABLE(of, dw_mci_bluefield_match);
+
+static int dw_mci_bluefield_probe(struct platform_device *pdev)
+{
+   const struct dw_mci_drv_data *drv_data = NULL;
+   const struct of_device_id *match;
+
+   if (pdev->dev.of_node) {
+   match = of_match_node(dw_mci_bluefield_match,
+ pdev->dev.of_node);
+   drv_data = match->data;
+   }
+
+   return dw_mci_pltfm_register(pdev, drv_data);
+}
+
+static struct platform_driver dw_mci_bluefield_pltfm_driver = {
+   .probe  = dw_mci_bluefield_probe,
+   .remove = dw_mci_pltfm_remove,
+   .driver = {
+   .name   = "dwmmc_bluefield",
+   .of_match_table = dw_mci_bluefield_match,
+   .pm = _mci_pltfm_pmops,
+   },
+};
+
+module_platform_driver(dw_mci_bluefield_pltfm_driver);
+
+MODULE_DESCRIPTION("BlueField DW Multimedia Card driver");
+MODULE_AUTHOR("Mellanox Technologies");
+MODULE_LICENSE("GPL v2");

Re: [PATCH v2 1/3] mmc: dw_mmc-bluefield: Add driver extension

[Shawn Lin]

Hi Liming,

On 2018/4/23 23:32, Liming Sun wrote:

This commit adds extension to the dw_mmc driver for Mellanox BlueField
SoC. It updates the UHS_REG_EXT register to bring up the eMMC card on
this SoC.

Signed-off-by: Liming Sun 
---
  drivers/mmc/host/Kconfig|  9 +
  drivers/mmc/host/Makefile   |  1 +
  drivers/mmc/host/dw_mmc-bluefield.c | 72 +
  3 files changed, 82 insertions(+)
  create mode 100644 drivers/mmc/host/dw_mmc-bluefield.c

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 9589f9c..26ac6b5 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -717,6 +717,15 @@ config MMC_DW_K3
  Synopsys DesignWare Memory Card Interface driver. Select this option
  for platforms based on Hisilicon K3 SoC's.
  
+config MMC_DW_BLUEFIELD

+   tristate "BlueField specific extensions for Synopsys DW Memory Card 
Interface"
+   depends on MMC_DW
+   select MMC_DW_PLTFM
+   help
+ This selects support for Mellanox BlueField SoC specific extensions to
+ the Synopsys DesignWare Memory Card Interface driver. Select this
+ option for platforms based on Mellanox BlueField SoC's.
+


It'd better to keep the order, so you could place it before
MMC_DW_EXYNOS.


  config MMC_DW_PCI
tristate "Synopsys Designware MCI support on PCI bus"
depends on MMC_DW && PCI
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 6aead24..267b3f1 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -55,6 +55,7 @@ obj-$(CONFIG_MMC_DW_K3)   += dw_mmc-k3.o
  obj-$(CONFIG_MMC_DW_PCI)  += dw_mmc-pci.o
  obj-$(CONFIG_MMC_DW_ROCKCHIP) += dw_mmc-rockchip.o
  obj-$(CONFIG_MMC_DW_ZX)   += dw_mmc-zx.o
+obj-$(CONFIG_MMC_DW_BLUEFIELD) += dw_mmc-bluefield.o


Ditto.


  obj-$(CONFIG_MMC_SH_MMCIF)+= sh_mmcif.o
  obj-$(CONFIG_MMC_JZ4740)  += jz4740_mmc.o
  obj-$(CONFIG_MMC_VUB300)  += vub300.o
diff --git a/drivers/mmc/host/dw_mmc-bluefield.c 
b/drivers/mmc/host/dw_mmc-bluefield.c
new file mode 100644
index 000..12067b1
--- /dev/null
+++ b/drivers/mmc/host/dw_mmc-bluefield.c
@@ -0,0 +1,72 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2018 Mellanox Technologies.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+


Ditto.


+#include "dw_mmc.h"
+#include "dw_mmc-pltfm.h"
+
+static void dw_mci_bluefield_set_ios(struct dw_mci *host, struct mmc_ios *ios)
+{
+   u32 regs;
+
+   /* Set drive=4 (bit 29:23) and sample=2 (bit 22:16) in UHS_REG_EXT. */
+   regs = mci_readl(host, UHS_REG_EXT);
+   regs = (regs & ~0x3F10 & ~0x7F) | (4 << 23) | (2 << 16);


GENMASK woule be more readable IMHO.


+   mci_writel(host, UHS_REG_EXT, regs);
+}
+
+static const struct dw_mci_drv_data bluefield_drv_data = {
+   .set_ios= dw_mci_bluefield_set_ios
+};
+
+static const struct of_device_id dw_mci_bluefield_match[] = {
+   { .compatible = "mellanox,bluefield-dw-mshc",
+   .data = _drv_data },


Keep the indent.


+   {},
+};
+MODULE_DEVICE_TABLE(of, dw_mci_bluefield_match);
+
+static int dw_mci_bluefield_probe(struct platform_device *pdev)
+{
+   const struct dw_mci_drv_data *drv_data = NULL;
+   const struct of_device_id *match;
+
+   if (pdev->dev.of_node) {
+   match = of_match_node(dw_mci_bluefield_match,
+ pdev->dev.of_node);
+   drv_data = match->data;
+   }
+
+   return dw_mci_pltfm_register(pdev, drv_data);
+}
+
+static struct platform_driver dw_mci_bluefield_pltfm_driver = {
+   .probe  = dw_mci_bluefield_probe,
+   .remove = dw_mci_pltfm_remove,
+   .driver = {
+   .name   = "dwmmc_bluefield",
+   .of_match_table = dw_mci_bluefield_match,
+   .pm = _mci_pltfm_pmops,
+   },
+};
+
+module_platform_driver(dw_mci_bluefield_pltfm_driver);
+
+MODULE_DESCRIPTION("BlueField DW Multimedia Card driver");
+MODULE_AUTHOR("Mellanox Technologies");
+MODULE_LICENSE("GPL v2");

Re: clk: bulk: silently error out on EPROBE_DEFER

[Shawn Lin]

Hi Jerome,

On 2018/4/9 22:13, Jerome Brunet wrote:

In clk_bulk_get(), if we fail to get the clock due to probe deferal, we
shouldn't print an error message. Just be silent in this case.



I saw a confusing clk get failure log occasionally, but didn't pay
much attention to it as the driver finally probed fine. But probably it
came from clk_bulk_get,

Reviewed-by: Shawn Lin <shawn@rock-chips.com>


Signed-off-by: Jerome Brunet <jbru...@baylibre.com>
---
  drivers/clk/clk-bulk.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk-bulk.c b/drivers/clk/clk-bulk.c
index 4c10456f8a32..6904ed6da504 100644
--- a/drivers/clk/clk-bulk.c
+++ b/drivers/clk/clk-bulk.c
@@ -42,8 +42,9 @@ int __must_check clk_bulk_get(struct device *dev, int 
num_clks,
clks[i].clk = clk_get(dev, clks[i].id);
if (IS_ERR(clks[i].clk)) {
ret = PTR_ERR(clks[i].clk);
-   dev_err(dev, "Failed to get clk '%s': %d\n",
-   clks[i].id, ret);
+   if (ret != -EPROBE_DEFER)
+   dev_err(dev, "Failed to get clk '%s': %d\n",
+   clks[i].id, ret);
clks[i].clk = NULL;
goto err;
}

Re: clk: bulk: silently error out on EPROBE_DEFER

[Shawn Lin]

Hi Jerome,

On 2018/4/9 22:13, Jerome Brunet wrote:

In clk_bulk_get(), if we fail to get the clock due to probe deferal, we
shouldn't print an error message. Just be silent in this case.



I saw a confusing clk get failure log occasionally, but didn't pay
much attention to it as the driver finally probed fine. But probably it
came from clk_bulk_get,

Reviewed-by: Shawn Lin 


Signed-off-by: Jerome Brunet 
---
  drivers/clk/clk-bulk.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk-bulk.c b/drivers/clk/clk-bulk.c
index 4c10456f8a32..6904ed6da504 100644
--- a/drivers/clk/clk-bulk.c
+++ b/drivers/clk/clk-bulk.c
@@ -42,8 +42,9 @@ int __must_check clk_bulk_get(struct device *dev, int 
num_clks,
clks[i].clk = clk_get(dev, clks[i].id);
if (IS_ERR(clks[i].clk)) {
ret = PTR_ERR(clks[i].clk);
-   dev_err(dev, "Failed to get clk '%s': %d\n",
-   clks[i].id, ret);
+   if (ret != -EPROBE_DEFER)
+   dev_err(dev, "Failed to get clk '%s': %d\n",
+   clks[i].id, ret);
clks[i].clk = NULL;
goto err;
}

Re: [PATCH] mmc: dw_mmc-k3: Fix DDR52 mode by setting required clock divisor

[Shawn Lin]

On 2018/4/6 21:41, Ryan Grachek wrote:

On Wed, Apr 4, 2018 at 7:51 PM, Shawn Lin <shawn@rock-chips.com> wrote:

[+ Zhangfei Gao who added support for hi6220]

On 2018/4/4 23:31, Ryan Grachek wrote:


On Tue, Apr 3, 2018 at 6:31 AM, Shawn Lin <shawn@rock-chips.com
<mailto:shawn@rock-chips.com>> wrote:

 On 2018/3/30 2:24, oscardagrach wrote:

 Need at least one line commit body.

 Signed-off-by: oscardagrach <r...@edited.us
<mailto:r...@edited.us>>

 ---
drivers/mmc/host/dw_mmc-k3.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)

 diff --git a/drivers/mmc/host/dw_mmc-k3.c
 b/drivers/mmc/host/dw_mmc-k3.c
 index 89cdb3d533bb..efc546cb4db8 100644
 --- a/drivers/mmc/host/dw_mmc-k3.c
 +++ b/drivers/mmc/host/dw_mmc-k3.c
 @@ -194,8 +194,14 @@ static void dw_mci_hi6220_set_ios(struct
 dw_mci *host, struct mmc_ios *ios)
  int ret;
  unsigned int clock;
- clock = (ios->clock <= 2500) ? 2500 : ios->clock;
 -
 +   /* CLKDIV must be 1 for DDR52/8-bit mode */
 +   if (ios->bus_width == MMC_BUS_WIDTH_8 &&
 +   ios->timing == MMC_TIMING_MMC_DDR52) {
 +   mci_writel(host, CLKDIV, 0x1);
 +   clock = ios->clock;
 +   } else {
 +   clock = (ios->clock <= 2500) ? 2500 :
 ios->clock;
 +   }


 I undertand DDR52/8-bit need CLKDIV fixed 1, but shouldn't the
following
 change is more sensible?

 if (ios->bus_width == MMC_BUS_WIDTH_8 && ios->timing ==
 MMC_TIMING_MMC_DDR52)
  clock = ios->clock * 2;
 else
  clock = (ios->clock <= 2500) ? 2500 : ios->clock;


 The reason is ios->clock is 52MHz and you could claim 104MHz from the
 clock provider and let dw_mmc core take care of the divder to be 1.
 Otherwise, you just force it to be DDR52/8-bit with a clk rate of
26MHz.


  ret = clk_set_rate(host->biu_clk, clock);
  if (ret)
  dev_warn(host->dev, "failed to set rate
 %uHz\n", clock);





For future wise, please use plain mode mail, but not HTML format.


Your feedback is correct. I see the Rockchip dwmmc driver has a similar
implementation. After applying your suggested changes, however, my board
reports "dwmmc_k3 f723d000.dwmmc0: failed to set rate 10400Hz"
during intialization of eMMC. In addition, I do not see CLKDIV being
set to 1. clk_set_rate fails and I wonder if this is out-of-scope of
the driver.

If I set CLKDIV where I did prior, with your changes, the device fails
to set the clock and falls back to 52 MHz (26 MHz) and works fine, but
again, CLKDIV is reported as 0 (even though it is 1.) One thing of
interest to note is when I manually set the clock by doing:
(echo 10400 > /sys/kernel/debug/mmc0/clock) the device reports back
'mmc_host mmc0: Bus speed (slot 0) = 19840Hz (slot req 10400Hz,
   actual 9920HZ div = 1)' which works reliably and clk_set_rate does
not report any error.



When looking closely into the code, at least dw_mci_hi6220_set_ios
goes wrong with the bus_hz, since it should be ciu_clk but not biu_clk.
"b" stands for bus, and "c" stands for card IMHO, however bus_hz
describs the clock to the card, provided by controller. Does the
following patch help?


diff --git a/drivers/mmc/host/dw_mmc-k3.c b/drivers/mmc/host/dw_mmc-k3.c
index 89cdb3d..9e78cf2 100644
--- a/drivers/mmc/host/dw_mmc-k3.c
+++ b/drivers/mmc/host/dw_mmc-k3.c
@@ -194,13 +194,21 @@ static void dw_mci_hi6220_set_ios(struct dw_mci *host,
struct mmc_ios *ios)
 int ret;
 unsigned int clock;

-   clock = (ios->clock <= 2500) ? 2500 : ios->clock;
+   if (ios->bus_width == MMC_BUS_WIDTH_8 &&
+   ios->timing == MMC_TIMING_MMC_DDR52)
+   clock = ios->clock * 2;
+   else
+   clock = (ios->clock <= 2500) ? 2500 : ios->clock;

-   ret = clk_set_rate(host->biu_clk, clock);
+   ret = clk_set_rate(host->ciu_clk, clock);
 if (ret)
 dev_warn(host->dev, "failed to set rate %uHz\n", clock);

-   host->bus_hz = clk_get_rate(host->biu_clk);
+   clock = clk_get_rate(host->ciu_clk);
+   if (clock != host->bus_hz) {
+   host->bus_hz = clock;
+   host->current_speed = 0;
+   }

  }



I am not sure where to begin debugging these clock issues and welcome
any feedback.





The change results in the following:
'dwmmc_k3 f723e000.dwmmc1: failed to set rate 2500Hz'
'dwmmc_k3 f723d000.dwmmc0: failed to set rate 2500Hz'
'dwmmc

Re: [PATCH] mmc: dw_mmc-k3: Fix DDR52 mode by setting required clock divisor

[Shawn Lin]

On 2018/4/6 21:41, Ryan Grachek wrote:

On Wed, Apr 4, 2018 at 7:51 PM, Shawn Lin  wrote:

[+ Zhangfei Gao who added support for hi6220]

On 2018/4/4 23:31, Ryan Grachek wrote:


On Tue, Apr 3, 2018 at 6:31 AM, Shawn Lin mailto:shawn@rock-chips.com>> wrote:

 On 2018/3/30 2:24, oscardagrach wrote:

 Need at least one line commit body.

 Signed-off-by: oscardagrach mailto:r...@edited.us>>

 ---
drivers/mmc/host/dw_mmc-k3.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)

 diff --git a/drivers/mmc/host/dw_mmc-k3.c
 b/drivers/mmc/host/dw_mmc-k3.c
 index 89cdb3d533bb..efc546cb4db8 100644
 --- a/drivers/mmc/host/dw_mmc-k3.c
 +++ b/drivers/mmc/host/dw_mmc-k3.c
 @@ -194,8 +194,14 @@ static void dw_mci_hi6220_set_ios(struct
 dw_mci *host, struct mmc_ios *ios)
  int ret;
  unsigned int clock;
- clock = (ios->clock <= 2500) ? 2500 : ios->clock;
 -
 +   /* CLKDIV must be 1 for DDR52/8-bit mode */
 +   if (ios->bus_width == MMC_BUS_WIDTH_8 &&
 +   ios->timing == MMC_TIMING_MMC_DDR52) {
 +   mci_writel(host, CLKDIV, 0x1);
 +   clock = ios->clock;
 +   } else {
 +   clock = (ios->clock <= 2500) ? 2500 :
 ios->clock;
 +   }


 I undertand DDR52/8-bit need CLKDIV fixed 1, but shouldn't the
following
 change is more sensible?

 if (ios->bus_width == MMC_BUS_WIDTH_8 && ios->timing ==
 MMC_TIMING_MMC_DDR52)
  clock = ios->clock * 2;
 else
  clock = (ios->clock <= 2500) ? 2500 : ios->clock;


 The reason is ios->clock is 52MHz and you could claim 104MHz from the
 clock provider and let dw_mmc core take care of the divder to be 1.
 Otherwise, you just force it to be DDR52/8-bit with a clk rate of
26MHz.


  ret = clk_set_rate(host->biu_clk, clock);
  if (ret)
  dev_warn(host->dev, "failed to set rate
 %uHz\n", clock);





For future wise, please use plain mode mail, but not HTML format.


Your feedback is correct. I see the Rockchip dwmmc driver has a similar
implementation. After applying your suggested changes, however, my board
reports "dwmmc_k3 f723d000.dwmmc0: failed to set rate 10400Hz"
during intialization of eMMC. In addition, I do not see CLKDIV being
set to 1. clk_set_rate fails and I wonder if this is out-of-scope of
the driver.

If I set CLKDIV where I did prior, with your changes, the device fails
to set the clock and falls back to 52 MHz (26 MHz) and works fine, but
again, CLKDIV is reported as 0 (even though it is 1.) One thing of
interest to note is when I manually set the clock by doing:
(echo 10400 > /sys/kernel/debug/mmc0/clock) the device reports back
'mmc_host mmc0: Bus speed (slot 0) = 19840Hz (slot req 10400Hz,
   actual 9920HZ div = 1)' which works reliably and clk_set_rate does
not report any error.



When looking closely into the code, at least dw_mci_hi6220_set_ios
goes wrong with the bus_hz, since it should be ciu_clk but not biu_clk.
"b" stands for bus, and "c" stands for card IMHO, however bus_hz
describs the clock to the card, provided by controller. Does the
following patch help?


diff --git a/drivers/mmc/host/dw_mmc-k3.c b/drivers/mmc/host/dw_mmc-k3.c
index 89cdb3d..9e78cf2 100644
--- a/drivers/mmc/host/dw_mmc-k3.c
+++ b/drivers/mmc/host/dw_mmc-k3.c
@@ -194,13 +194,21 @@ static void dw_mci_hi6220_set_ios(struct dw_mci *host,
struct mmc_ios *ios)
 int ret;
 unsigned int clock;

-   clock = (ios->clock <= 2500) ? 2500 : ios->clock;
+   if (ios->bus_width == MMC_BUS_WIDTH_8 &&
+   ios->timing == MMC_TIMING_MMC_DDR52)
+   clock = ios->clock * 2;
+   else
+   clock = (ios->clock <= 2500) ? 2500 : ios->clock;

-   ret = clk_set_rate(host->biu_clk, clock);
+   ret = clk_set_rate(host->ciu_clk, clock);
 if (ret)
 dev_warn(host->dev, "failed to set rate %uHz\n", clock);

-   host->bus_hz = clk_get_rate(host->biu_clk);
+   clock = clk_get_rate(host->ciu_clk);
+   if (clock != host->bus_hz) {
+   host->bus_hz = clock;
+   host->current_speed = 0;
+   }

  }



I am not sure where to begin debugging these clock issues and welcome
any feedback.





The change results in the following:
'dwmmc_k3 f723e000.dwmmc1: failed to set rate 2500Hz'
'dwmmc_k3 f723d000.dwmmc0: failed to set rate 2500Hz'
'dwmmc_k3 f723f000.dwmmc2: failed to set rate 2500Hz'

and later on:
'dwmmc_k3 f723d000.dwm

Re: [PATCH] mmc: dw_mmc-k3: Fix DDR52 mode by setting required clock divisor

[Shawn Lin]

[+ Zhangfei Gao who added support for hi6220]

On 2018/4/4 23:31, Ryan Grachek wrote:
On Tue, Apr 3, 2018 at 6:31 AM, Shawn Lin <shawn@rock-chips.com 
<mailto:shawn@rock-chips.com>> wrote:


On 2018/3/30 2:24, oscardagrach wrote:

Need at least one line commit body.

Signed-off-by: oscardagrach <r...@edited.us <mailto:r...@edited.us>>
---
   drivers/mmc/host/dw_mmc-k3.c | 10 --
   1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc-k3.c
b/drivers/mmc/host/dw_mmc-k3.c
index 89cdb3d533bb..efc546cb4db8 100644
--- a/drivers/mmc/host/dw_mmc-k3.c
+++ b/drivers/mmc/host/dw_mmc-k3.c
@@ -194,8 +194,14 @@ static void dw_mci_hi6220_set_ios(struct
dw_mci *host, struct mmc_ios *ios)
         int ret;
         unsigned int clock;
   -     clock = (ios->clock <= 2500) ? 2500 : ios->clock;
-
+       /* CLKDIV must be 1 for DDR52/8-bit mode */
+       if (ios->bus_width == MMC_BUS_WIDTH_8 &&
+               ios->timing == MMC_TIMING_MMC_DDR52) {
+               mci_writel(host, CLKDIV, 0x1);
+               clock = ios->clock;
+       } else {
+               clock = (ios->clock <= 2500) ? 2500 :
ios->clock;
+       }


I undertand DDR52/8-bit need CLKDIV fixed 1, but shouldn't the following
change is more sensible?

if (ios->bus_width == MMC_BUS_WIDTH_8 && ios->timing ==
MMC_TIMING_MMC_DDR52)
         clock = ios->clock * 2;
else
         clock = (ios->clock <= 2500) ? 2500 : ios->clock;


The reason is ios->clock is 52MHz and you could claim 104MHz from the
clock provider and let dw_mmc core take care of the divder to be 1.
Otherwise, you just force it to be DDR52/8-bit with a clk rate of 26MHz.


         ret = clk_set_rate(host->biu_clk, clock);
         if (ret)
                 dev_warn(host->dev, "failed to set rate
%uHz\n", clock);





For future wise, please use plain mode mail, but not HTML format.


Your feedback is correct. I see the Rockchip dwmmc driver has a similar
implementation. After applying your suggested changes, however, my board
reports "dwmmc_k3 f723d000.dwmmc0: failed to set rate 10400Hz"
during intialization of eMMC. In addition, I do not see CLKDIV being
set to 1. clk_set_rate fails and I wonder if this is out-of-scope of
the driver.

If I set CLKDIV where I did prior, with your changes, the device fails
to set the clock and falls back to 52 MHz (26 MHz) and works fine, but
again, CLKDIV is reported as 0 (even though it is 1.) One thing of
interest to note is when I manually set the clock by doing:
(echo 10400 > /sys/kernel/debug/mmc0/clock) the device reports back
'mmc_host mmc0: Bus speed (slot 0) = 19840Hz (slot req 10400Hz,
  actual 9920HZ div = 1)' which works reliably and clk_set_rate does
not report any error.



When looking closely into the code, at least dw_mci_hi6220_set_ios
goes wrong with the bus_hz, since it should be ciu_clk but not biu_clk.
"b" stands for bus, and "c" stands for card IMHO, however bus_hz
describs the clock to the card, provided by controller. Does the
following patch help?


diff --git a/drivers/mmc/host/dw_mmc-k3.c b/drivers/mmc/host/dw_mmc-k3.c
index 89cdb3d..9e78cf2 100644
--- a/drivers/mmc/host/dw_mmc-k3.c
+++ b/drivers/mmc/host/dw_mmc-k3.c
@@ -194,13 +194,21 @@ static void dw_mci_hi6220_set_ios(struct dw_mci 
*host, struct mmc_ios *ios)

int ret;
unsigned int clock;

-   clock = (ios->clock <= 2500) ? 2500 : ios->clock;
+   if (ios->bus_width == MMC_BUS_WIDTH_8 &&
+   ios->timing == MMC_TIMING_MMC_DDR52)
+   clock = ios->clock * 2;
+   else
+   clock = (ios->clock <= 2500) ? 2500 : ios->clock;

-   ret = clk_set_rate(host->biu_clk, clock);
+   ret = clk_set_rate(host->ciu_clk, clock);
if (ret)
dev_warn(host->dev, "failed to set rate %uHz\n", clock);

-   host->bus_hz = clk_get_rate(host->biu_clk);
+   clock = clk_get_rate(host->ciu_clk);
+   if (clock != host->bus_hz) {
+   host->bus_hz = clock;
+   host->current_speed = 0;
+   }
 }



I am not sure where to begin debugging these clock issues and welcome
any feedback.

Re: [PATCH] mmc: dw_mmc-k3: Fix DDR52 mode by setting required clock divisor

[Shawn Lin]

[+ Zhangfei Gao who added support for hi6220]

On 2018/4/4 23:31, Ryan Grachek wrote:
On Tue, Apr 3, 2018 at 6:31 AM, Shawn Lin <mailto:shawn@rock-chips.com>> wrote:


On 2018/3/30 2:24, oscardagrach wrote:

Need at least one line commit body.

Signed-off-by: oscardagrach mailto:r...@edited.us>>
---
   drivers/mmc/host/dw_mmc-k3.c | 10 --
   1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc-k3.c
b/drivers/mmc/host/dw_mmc-k3.c
index 89cdb3d533bb..efc546cb4db8 100644
--- a/drivers/mmc/host/dw_mmc-k3.c
+++ b/drivers/mmc/host/dw_mmc-k3.c
@@ -194,8 +194,14 @@ static void dw_mci_hi6220_set_ios(struct
dw_mci *host, struct mmc_ios *ios)
         int ret;
         unsigned int clock;
   -     clock = (ios->clock <= 2500) ? 2500 : ios->clock;
-
+       /* CLKDIV must be 1 for DDR52/8-bit mode */
+       if (ios->bus_width == MMC_BUS_WIDTH_8 &&
+               ios->timing == MMC_TIMING_MMC_DDR52) {
+               mci_writel(host, CLKDIV, 0x1);
+               clock = ios->clock;
+       } else {
+               clock = (ios->clock <= 2500) ? 2500 :
ios->clock;
+       }


I undertand DDR52/8-bit need CLKDIV fixed 1, but shouldn't the following
change is more sensible?

if (ios->bus_width == MMC_BUS_WIDTH_8 && ios->timing ==
MMC_TIMING_MMC_DDR52)
         clock = ios->clock * 2;
else
         clock = (ios->clock <= 2500) ? 2500 : ios->clock;


The reason is ios->clock is 52MHz and you could claim 104MHz from the
clock provider and let dw_mmc core take care of the divder to be 1.
Otherwise, you just force it to be DDR52/8-bit with a clk rate of 26MHz.


         ret = clk_set_rate(host->biu_clk, clock);
         if (ret)
                 dev_warn(host->dev, "failed to set rate
%uHz\n", clock);





For future wise, please use plain mode mail, but not HTML format.


Your feedback is correct. I see the Rockchip dwmmc driver has a similar
implementation. After applying your suggested changes, however, my board
reports "dwmmc_k3 f723d000.dwmmc0: failed to set rate 10400Hz"
during intialization of eMMC. In addition, I do not see CLKDIV being
set to 1. clk_set_rate fails and I wonder if this is out-of-scope of
the driver.

If I set CLKDIV where I did prior, with your changes, the device fails
to set the clock and falls back to 52 MHz (26 MHz) and works fine, but
again, CLKDIV is reported as 0 (even though it is 1.) One thing of
interest to note is when I manually set the clock by doing:
(echo 10400 > /sys/kernel/debug/mmc0/clock) the device reports back
'mmc_host mmc0: Bus speed (slot 0) = 19840Hz (slot req 10400Hz,
  actual 9920HZ div = 1)' which works reliably and clk_set_rate does
not report any error.



When looking closely into the code, at least dw_mci_hi6220_set_ios
goes wrong with the bus_hz, since it should be ciu_clk but not biu_clk.
"b" stands for bus, and "c" stands for card IMHO, however bus_hz
describs the clock to the card, provided by controller. Does the
following patch help?


diff --git a/drivers/mmc/host/dw_mmc-k3.c b/drivers/mmc/host/dw_mmc-k3.c
index 89cdb3d..9e78cf2 100644
--- a/drivers/mmc/host/dw_mmc-k3.c
+++ b/drivers/mmc/host/dw_mmc-k3.c
@@ -194,13 +194,21 @@ static void dw_mci_hi6220_set_ios(struct dw_mci 
*host, struct mmc_ios *ios)

int ret;
unsigned int clock;

-   clock = (ios->clock <= 2500) ? 2500 : ios->clock;
+   if (ios->bus_width == MMC_BUS_WIDTH_8 &&
+   ios->timing == MMC_TIMING_MMC_DDR52)
+   clock = ios->clock * 2;
+   else
+   clock = (ios->clock <= 2500) ? 2500 : ios->clock;

-   ret = clk_set_rate(host->biu_clk, clock);
+   ret = clk_set_rate(host->ciu_clk, clock);
if (ret)
dev_warn(host->dev, "failed to set rate %uHz\n", clock);

-   host->bus_hz = clk_get_rate(host->biu_clk);
+   clock = clk_get_rate(host->ciu_clk);
+   if (clock != host->bus_hz) {
+   host->bus_hz = clock;
+   host->current_speed = 0;
+   }
 }



I am not sure where to begin debugging these clock issues and welcome
any feedback.

Re: [PATCH] mmc: dw_mmc-k3: Fix DDR52 mode by setting required clock divisor

[Shawn Lin]

On 2018/3/30 2:24, oscardagrach wrote:

Need at least one line commit body.


Signed-off-by: oscardagrach 
---
  drivers/mmc/host/dw_mmc-k3.c | 10 --
  1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc-k3.c b/drivers/mmc/host/dw_mmc-k3.c
index 89cdb3d533bb..efc546cb4db8 100644
--- a/drivers/mmc/host/dw_mmc-k3.c
+++ b/drivers/mmc/host/dw_mmc-k3.c
@@ -194,8 +194,14 @@ static void dw_mci_hi6220_set_ios(struct dw_mci *host, 
struct mmc_ios *ios)
int ret;
unsigned int clock;
  
-	clock = (ios->clock <= 2500) ? 2500 : ios->clock;

-
+   /* CLKDIV must be 1 for DDR52/8-bit mode */
+   if (ios->bus_width == MMC_BUS_WIDTH_8 &&
+   ios->timing == MMC_TIMING_MMC_DDR52) {
+   mci_writel(host, CLKDIV, 0x1);
+   clock = ios->clock;
+   } else {
+   clock = (ios->clock <= 2500) ? 2500 : ios->clock;
+   }


I undertand DDR52/8-bit need CLKDIV fixed 1, but shouldn't the following
change is more sensible?

if (ios->bus_width == MMC_BUS_WIDTH_8 && ios->timing ==
MMC_TIMING_MMC_DDR52)
clock = ios->clock * 2;
else
clock = (ios->clock <= 2500) ? 2500 : ios->clock;


The reason is ios->clock is 52MHz and you could claim 104MHz from the
clock provider and let dw_mmc core take care of the divder to be 1.
Otherwise, you just force it to be DDR52/8-bit with a clk rate of 26MHz.


ret = clk_set_rate(host->biu_clk, clock);
if (ret)
dev_warn(host->dev, "failed to set rate %uHz\n", clock);

Re: [PATCH] mmc: dw_mmc-k3: Fix DDR52 mode by setting required clock divisor

[Shawn Lin]

On 2018/3/30 2:24, oscardagrach wrote:

Need at least one line commit body.


Signed-off-by: oscardagrach 
---
  drivers/mmc/host/dw_mmc-k3.c | 10 --
  1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc-k3.c b/drivers/mmc/host/dw_mmc-k3.c
index 89cdb3d533bb..efc546cb4db8 100644
--- a/drivers/mmc/host/dw_mmc-k3.c
+++ b/drivers/mmc/host/dw_mmc-k3.c
@@ -194,8 +194,14 @@ static void dw_mci_hi6220_set_ios(struct dw_mci *host, 
struct mmc_ios *ios)
int ret;
unsigned int clock;
  
-	clock = (ios->clock <= 2500) ? 2500 : ios->clock;

-
+   /* CLKDIV must be 1 for DDR52/8-bit mode */
+   if (ios->bus_width == MMC_BUS_WIDTH_8 &&
+   ios->timing == MMC_TIMING_MMC_DDR52) {
+   mci_writel(host, CLKDIV, 0x1);
+   clock = ios->clock;
+   } else {
+   clock = (ios->clock <= 2500) ? 2500 : ios->clock;
+   }


I undertand DDR52/8-bit need CLKDIV fixed 1, but shouldn't the following
change is more sensible?

if (ios->bus_width == MMC_BUS_WIDTH_8 && ios->timing ==
MMC_TIMING_MMC_DDR52)
clock = ios->clock * 2;
else
clock = (ios->clock <= 2500) ? 2500 : ios->clock;


The reason is ios->clock is 52MHz and you could claim 104MHz from the
clock provider and let dw_mmc core take care of the divder to be 1.
Otherwise, you just force it to be DDR52/8-bit with a clk rate of 26MHz.


ret = clk_set_rate(host->biu_clk, clock);
if (ret)
dev_warn(host->dev, "failed to set rate %uHz\n", clock);

Re: [PATCH] [mmc_block] Prevent bus reference leak in mmc_blk_init

[Shawn Lin]

 patch deallocates the reference in mmc_blk_exit.



Fixes: 97548575bef3 ("mmc: block: Convert RPMB to a character device")
Cc: Stable <sta...@vger.kernel.org>


Signed-off-by: Alexander Kappner <a...@godking.net>
---
  drivers/mmc/core/block.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 2cfb963..9c6f639 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -3087,6 +3087,7 @@ static void __exit mmc_blk_exit(void)
mmc_unregister_driver(_driver);
unregister_blkdev(MMC_BLOCK_MAJOR, "mmc");
unregister_chrdev_region(mmc_rpmb_devt, MAX_DEVICES);
+   bus_unregister(_rpmb_bus_type);


Reviewed-by: Shawn Lin <shawn@rock-chips.com>


  }
  
  module_init(mmc_blk_init);

Re: [PATCH] [mmc_block] Prevent bus reference leak in mmc_blk_init

[Shawn Lin]

 patch deallocates the reference in mmc_blk_exit.



Fixes: 97548575bef3 ("mmc: block: Convert RPMB to a character device")
Cc: Stable 


Signed-off-by: Alexander Kappner 
---
  drivers/mmc/core/block.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 2cfb963..9c6f639 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -3087,6 +3087,7 @@ static void __exit mmc_blk_exit(void)
mmc_unregister_driver(_driver);
unregister_blkdev(MMC_BLOCK_MAJOR, "mmc");
unregister_chrdev_region(mmc_rpmb_devt, MAX_DEVICES);
+   bus_unregister(_rpmb_bus_type);


Reviewed-by: Shawn Lin 


  }
  
  module_init(mmc_blk_init);

Re: [RFC PATCH] sdhci: arasan: Add runtime PM support

[Shawn Lin]

On 2018/3/29 13:48, naraniman...@gmail.com wrote:

From: Manish Narani 

This patch adds runtime PM support in Arasan SD driver.

Signed-off-by: Manish Narani 
---
  drivers/mmc/host/sdhci-of-arasan.c | 83 +-
  1 file changed, 81 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c 
b/drivers/mmc/host/sdhci-of-arasan.c
index c33a5f7..47196b5 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -23,6 +23,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -349,6 +350,75 @@ static const struct sdhci_pltfm_data 
sdhci_arasan_cqe_pdata = {
SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
  };
  
+#ifdef CONFIG_PM

+/**
+ * sdhci_arasan_runtime_suspend - Suspend method for the driver
+ * @dev:   Address of the device structure
+ * Returns 0 on success and error value on error
+ *
+ * Put the device in a low power state.
+ */
+static int sdhci_arasan_runtime_suspend(struct device *dev)
+{


Would you help take care of cqhci_suspend?


+   struct platform_device *pdev = to_platform_device(dev);
+   struct sdhci_host *host = platform_get_drvdata(pdev);
+   struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+   struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
+   int ret;
+
+   ret = sdhci_runtime_suspend_host(host);
+   if (ret)
+   return ret;
+
+   if (host->tuning_mode != SDHCI_TUNING_MODE_3)
+   mmc_retune_needed(host->mmc);
+
+   clk_disable(pltfm_host->clk);
+   clk_disable(sdhci_arasan->clk_ahb);
+
+   return 0;
+}
+
+/**
+ * sdhci_arasan_runtime_resume - Resume method for the driver
+ * @dev:   Address of the device structure
+ * Returns 0 on success and error value on error
+ *
+ * Resume operation after suspend
+ */
+static int sdhci_arasan_runtime_resume(struct device *dev)
+{
+   struct platform_device *pdev = to_platform_device(dev);
+   struct sdhci_host *host = platform_get_drvdata(pdev);
+   struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+   struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
+   int ret;
+


Ditto, for cqhci_resume.


+   ret = clk_enable(sdhci_arasan->clk_ahb);
+   if (ret) {
+   dev_err(dev, "Cannot enable AHB clock.\n");
+   return ret;
+   }
+
+   ret = clk_enable(pltfm_host->clk);
+   if (ret) {
+   dev_err(dev, "Cannot enable SD clock.\n");
+   return ret;
+   }
+
+   ret = sdhci_runtime_resume_host(host);
+   if (ret)
+   goto out;
+
+   return 0;
+out:
+   clk_disable(pltfm_host->clk);
+   clk_disable(sdhci_arasan->clk_ahb);
+
+   return ret;
+}
+#endif /* ! CONFIG_PM */
+
  #ifdef CONFIG_PM_SLEEP
  /**
   * sdhci_arasan_suspend - Suspend method for the driver
@@ -443,8 +513,11 @@ static int sdhci_arasan_resume(struct device *dev)
  }
  #endif /* ! CONFIG_PM_SLEEP */
  
-static SIMPLE_DEV_PM_OPS(sdhci_arasan_dev_pm_ops, sdhci_arasan_suspend,

-sdhci_arasan_resume);
+static const struct dev_pm_ops sdhci_arasan_dev_pm_ops = {
+   SET_SYSTEM_SLEEP_PM_OPS(sdhci_arasan_suspend, sdhci_arasan_resume)
+   SET_RUNTIME_PM_OPS(sdhci_arasan_runtime_suspend,
+  sdhci_arasan_runtime_resume, NULL)
+};
  
  static const struct of_device_id sdhci_arasan_of_match[] = {

/* SoC-specific compatible strings w/ soc_ctl_map */
@@ -806,6 +879,12 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
if (ret)
goto err_add_host;
  
+	pm_runtime_set_active(>dev);

+   pm_runtime_enable(>dev);
+   pm_runtime_set_autosuspend_delay(>dev, 2000);
+   pm_runtime_mark_last_busy(>dev);
+   pm_runtime_use_autosuspend(>dev);
+
return 0;
  
  err_add_host:

Re: [RFC PATCH] sdhci: arasan: Add runtime PM support

[Shawn Lin]

On 2018/3/29 13:48, naraniman...@gmail.com wrote:

From: Manish Narani 

This patch adds runtime PM support in Arasan SD driver.

Signed-off-by: Manish Narani 
---
  drivers/mmc/host/sdhci-of-arasan.c | 83 +-
  1 file changed, 81 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c 
b/drivers/mmc/host/sdhci-of-arasan.c
index c33a5f7..47196b5 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -23,6 +23,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -349,6 +350,75 @@ static const struct sdhci_pltfm_data 
sdhci_arasan_cqe_pdata = {
SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
  };
  
+#ifdef CONFIG_PM

+/**
+ * sdhci_arasan_runtime_suspend - Suspend method for the driver
+ * @dev:   Address of the device structure
+ * Returns 0 on success and error value on error
+ *
+ * Put the device in a low power state.
+ */
+static int sdhci_arasan_runtime_suspend(struct device *dev)
+{


Would you help take care of cqhci_suspend?


+   struct platform_device *pdev = to_platform_device(dev);
+   struct sdhci_host *host = platform_get_drvdata(pdev);
+   struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+   struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
+   int ret;
+
+   ret = sdhci_runtime_suspend_host(host);
+   if (ret)
+   return ret;
+
+   if (host->tuning_mode != SDHCI_TUNING_MODE_3)
+   mmc_retune_needed(host->mmc);
+
+   clk_disable(pltfm_host->clk);
+   clk_disable(sdhci_arasan->clk_ahb);
+
+   return 0;
+}
+
+/**
+ * sdhci_arasan_runtime_resume - Resume method for the driver
+ * @dev:   Address of the device structure
+ * Returns 0 on success and error value on error
+ *
+ * Resume operation after suspend
+ */
+static int sdhci_arasan_runtime_resume(struct device *dev)
+{
+   struct platform_device *pdev = to_platform_device(dev);
+   struct sdhci_host *host = platform_get_drvdata(pdev);
+   struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+   struct sdhci_arasan_data *sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
+   int ret;
+


Ditto, for cqhci_resume.


+   ret = clk_enable(sdhci_arasan->clk_ahb);
+   if (ret) {
+   dev_err(dev, "Cannot enable AHB clock.\n");
+   return ret;
+   }
+
+   ret = clk_enable(pltfm_host->clk);
+   if (ret) {
+   dev_err(dev, "Cannot enable SD clock.\n");
+   return ret;
+   }
+
+   ret = sdhci_runtime_resume_host(host);
+   if (ret)
+   goto out;
+
+   return 0;
+out:
+   clk_disable(pltfm_host->clk);
+   clk_disable(sdhci_arasan->clk_ahb);
+
+   return ret;
+}
+#endif /* ! CONFIG_PM */
+
  #ifdef CONFIG_PM_SLEEP
  /**
   * sdhci_arasan_suspend - Suspend method for the driver
@@ -443,8 +513,11 @@ static int sdhci_arasan_resume(struct device *dev)
  }
  #endif /* ! CONFIG_PM_SLEEP */
  
-static SIMPLE_DEV_PM_OPS(sdhci_arasan_dev_pm_ops, sdhci_arasan_suspend,

-sdhci_arasan_resume);
+static const struct dev_pm_ops sdhci_arasan_dev_pm_ops = {
+   SET_SYSTEM_SLEEP_PM_OPS(sdhci_arasan_suspend, sdhci_arasan_resume)
+   SET_RUNTIME_PM_OPS(sdhci_arasan_runtime_suspend,
+  sdhci_arasan_runtime_resume, NULL)
+};
  
  static const struct of_device_id sdhci_arasan_of_match[] = {

/* SoC-specific compatible strings w/ soc_ctl_map */
@@ -806,6 +879,12 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
if (ret)
goto err_add_host;
  
+	pm_runtime_set_active(>dev);

+   pm_runtime_enable(>dev);
+   pm_runtime_set_autosuspend_delay(>dev, 2000);
+   pm_runtime_mark_last_busy(>dev);
+   pm_runtime_use_autosuspend(>dev);
+
return 0;
  
  err_add_host:

Re: [PATCH] mmc: Export card RCA register to sysfs.

[Shawn Lin]

On 2018/3/6 20:48, Harish Jenny K N wrote:



On Tuesday 27 February 2018 05:26 PM, Harish Jenny K N wrote:

This patch exports RCA register to sysfs which will help in
reading the disk identification information.

Signed-off-by: Harish Jenny K N <harish_kand...@mentor.com>
---
  drivers/mmc/core/mmc.c | 2 ++
  drivers/mmc/core/sd.c  | 2 ++
  2 files changed, 4 insertions(+)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 208a762..d45a244 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -792,6 +792,7 @@ static int mmc_compare_ext_csds(struct mmc_card *card, 
unsigned bus_width)
  MMC_DEV_ATTR(raw_rpmb_size_mult, "%#x\n", card->ext_csd.raw_rpmb_size_mult);
  MMC_DEV_ATTR(rel_sectors, "%#x\n", card->ext_csd.rel_sectors);
  MMC_DEV_ATTR(ocr, "0x%08x\n", card->ocr);
+MMC_DEV_ATTR(rca, "0x%x\n", card->rca);
  MMC_DEV_ATTR(cmdq_en, "%d\n", card->ext_csd.cmdq_en);


Just a nit that I tried to find some convention here, as RCA is a 16-bit 
register, so perhaps "0x%04x\n"? Otherwise,


Reviewed-by: Shawn Lin <shawn@rock-chips.com>



  static ssize_t mmc_fwrev_show(struct device *dev,
@@ -848,6 +849,7 @@ static ssize_t mmc_dsr_show(struct device *dev,
_attr_raw_rpmb_size_mult.attr,
_attr_rel_sectors.attr,
_attr_ocr.attr,
+   _attr_rca.attr,
_attr_dsr.attr,
_attr_cmdq_en.attr,
NULL,
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 62b84dd..17d9005 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -676,6 +676,7 @@ static int mmc_sd_init_uhs_card(struct mmc_card *card)
  MMC_DEV_ATTR(oemid, "0x%04x\n", card->cid.oemid);
  MMC_DEV_ATTR(serial, "0x%08x\n", card->cid.serial);
  MMC_DEV_ATTR(ocr, "0x%08x\n", card->ocr);
+MMC_DEV_ATTR(rca, "0x%x\n", card->rca);


Ditto.




  static ssize_t mmc_dsr_show(struct device *dev,
@@ -709,6 +710,7 @@ static ssize_t mmc_dsr_show(struct device *dev,
_attr_oemid.attr,
_attr_serial.attr,
_attr_ocr.attr,
+   _attr_rca.attr,
        _attr_dsr.attr,
NULL,
  };
--
1.9.1





Any comments/inputs on this ?


Thanks,
Harish Jenny K N






--
Best Regards
Shawn Lin

Re: [PATCH] mmc: Export card RCA register to sysfs.

[Shawn Lin]

On 2018/3/6 20:48, Harish Jenny K N wrote:



On Tuesday 27 February 2018 05:26 PM, Harish Jenny K N wrote:

This patch exports RCA register to sysfs which will help in
reading the disk identification information.

Signed-off-by: Harish Jenny K N 
---
  drivers/mmc/core/mmc.c | 2 ++
  drivers/mmc/core/sd.c  | 2 ++
  2 files changed, 4 insertions(+)

diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 208a762..d45a244 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -792,6 +792,7 @@ static int mmc_compare_ext_csds(struct mmc_card *card, 
unsigned bus_width)
  MMC_DEV_ATTR(raw_rpmb_size_mult, "%#x\n", card->ext_csd.raw_rpmb_size_mult);
  MMC_DEV_ATTR(rel_sectors, "%#x\n", card->ext_csd.rel_sectors);
  MMC_DEV_ATTR(ocr, "0x%08x\n", card->ocr);
+MMC_DEV_ATTR(rca, "0x%x\n", card->rca);
  MMC_DEV_ATTR(cmdq_en, "%d\n", card->ext_csd.cmdq_en);


Just a nit that I tried to find some convention here, as RCA is a 16-bit 
register, so perhaps "0x%04x\n"? Otherwise,


Reviewed-by: Shawn Lin 



  static ssize_t mmc_fwrev_show(struct device *dev,
@@ -848,6 +849,7 @@ static ssize_t mmc_dsr_show(struct device *dev,
_attr_raw_rpmb_size_mult.attr,
_attr_rel_sectors.attr,
_attr_ocr.attr,
+   _attr_rca.attr,
_attr_dsr.attr,
_attr_cmdq_en.attr,
NULL,
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index 62b84dd..17d9005 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -676,6 +676,7 @@ static int mmc_sd_init_uhs_card(struct mmc_card *card)
  MMC_DEV_ATTR(oemid, "0x%04x\n", card->cid.oemid);
  MMC_DEV_ATTR(serial, "0x%08x\n", card->cid.serial);
  MMC_DEV_ATTR(ocr, "0x%08x\n", card->ocr);
+MMC_DEV_ATTR(rca, "0x%x\n", card->rca);


Ditto.




  static ssize_t mmc_dsr_show(struct device *dev,
@@ -709,6 +710,7 @@ static ssize_t mmc_dsr_show(struct device *dev,
_attr_oemid.attr,
_attr_serial.attr,
_attr_ocr.attr,
+   _attr_rca.attr,
_attr_dsr.attr,
NULL,
  };
--
1.9.1





Any comments/inputs on this ?


Thanks,
Harish Jenny K N






--
Best Regards
Shawn Lin

Re: [PATCH v5] mmc: Export host capabilities to debugfs.

[Shawn Lin]

On 2018/3/6 21:53, Harish Jenny K N wrote:

This patch exports the host capabilities to debugfs

This idea of sharing host capabilities over debugfs
came up from Abbas Raza <abbas_r...@mentor.com>
Earlier discussions:
https://lkml.org/lkml/2018/3/5/357
https://www.spinics.net/lists/linux-mmc/msg48219.html

Signed-off-by: Harish Jenny K N <harish_kand...@mentor.com>
---

Changes in v5:
- Added parser logic in kernel by using debugfs_create_file
  for caps and caps2 instead of debugfs_create_x32


I'm fine with your method if Ulf likes it, but could you use
DEFINE_SHOW_ATTRIBUTE instead? :)



--
Best Regards
Shawn Lin

Re: [PATCH v5] mmc: Export host capabilities to debugfs.

[Shawn Lin]

On 2018/3/6 21:53, Harish Jenny K N wrote:

This patch exports the host capabilities to debugfs

This idea of sharing host capabilities over debugfs
came up from Abbas Raza 
Earlier discussions:
https://lkml.org/lkml/2018/3/5/357
https://www.spinics.net/lists/linux-mmc/msg48219.html

Signed-off-by: Harish Jenny K N 
---

Changes in v5:
- Added parser logic in kernel by using debugfs_create_file
  for caps and caps2 instead of debugfs_create_x32


I'm fine with your method if Ulf likes it, but could you use
DEFINE_SHOW_ATTRIBUTE instead? :)



--
Best Regards
Shawn Lin

Re: [PATCH] mmc: sdhci-of-arasan: Add quirk to avoid erroneous msg

[Shawn Lin]

On 2018/3/6 0:47, Phil Edworthy wrote:

Hi Shawn,

On 28 February 2018 01:53, Shawn Lin wrote:

On 2018/2/27 23:05, Phil Edworthy wrote:

On 27 February 2018 14:42, Shawn Lin wrote:

On 2018/2/27 22:31, Phil Edworthy wrote:

On 27 February 2018 14:28, Shawn Lin wrote:

在 2018/2/27 21:55, Phil Edworthy 写道:

Since the controller does not support the end-of-busy IRQ, don't use

it.

Otherwise, on older SD cards you will get lots of these messages:
"mmc0: Got data interrupt 0x0002 even though no data operation
was

in progress"




I'm afraid you have to explain which version of arasan's IP suffer
from this and what does the "older SD cards" mean?

Ok, I'll try to find out the IP version...
For "older SD cards", I can provide a list of a few cards that
exhibit this problem and others that don't, is that enough info?


What I meant is could you elaborate more about what kind of cards,
e.g, are them the  legacy SDSC cards or SDHC cards, or maybe they are
only running with defaut speed? or whatever, but not just with a
vague "older" cards. :)

Unfortunately, I have one SDHC card that works, one that doesn't. Both
cards are running with a 50MHz SD clock. All I know is this:


Thanks for sharing these, though it looks wired as I never remember I saw
this problem when extensively tested SD cards on one of arasan controllers
in 2014.

Not sure what you mean by 'wired'?

Note that this is on a relatively slow device, a dual core Cortex A7 @500MHz.
Maybe that has some effect.


That's why I hope you could add the IP version in your commit msg, and
that would be a hint for why it behaved different over platforms.



It's also interesting that someone posted the same fix for Xilinx a while
back, I linked to it in the commit msg.

Thanks
Phil



SD cards that report unexpected interrupts:
2GB Sandisk Extreme III (e624 SD02G 1.89 GiB)
8GB Sandisk (SDHC class 4)  ( SU08G 7.40 GiB)
8GB Sandisk Extreme III (SDHC class 6)(bb4e SD08G 7.61 GiB)

SD cards that work ok:
16GB Samsung (microSDHC U1 class 10)  (0001 0 14.6 GiB)
16GB Sandisk Ultra (microSDHC U1 class 10)( SL16G 14.8 GiB)
32GB Sandisk Ultra (microSDHC U1 class 10)( SL32G 29.7 GiB)

Thanks
Phil


This has been reported on Xilinx devices that also use the Arasan IP.
See https://patchwork.kernel.org/patch/8062871/

This has been tested on the Renesas RZ/ND-DB board with the RZ/N1

SoC.


Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
---
 drivers/mmc/host/sdhci-of-arasan.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c
b/drivers/mmc/host/sdhci-of-arasan.c
index c33a5f7..ab66e32 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -290,7 +290,8 @@ static const struct sdhci_pltfm_data

sdhci_arasan_pdata = {

.ops = _arasan_ops,
.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
-   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
+   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN

|

+   SDHCI_QUIRK2_STOP_WITH_TC,
 };

 static u32 sdhci_arasan_cqhci_irq(struct sdhci_host *host, u32
intmask)




--
Best Regards
Shawn Lin








--
Best Regards
Shawn Lin

Re: [PATCH] mmc: sdhci-of-arasan: Add quirk to avoid erroneous msg

[Shawn Lin]

On 2018/3/6 0:47, Phil Edworthy wrote:

Hi Shawn,

On 28 February 2018 01:53, Shawn Lin wrote:

On 2018/2/27 23:05, Phil Edworthy wrote:

On 27 February 2018 14:42, Shawn Lin wrote:

On 2018/2/27 22:31, Phil Edworthy wrote:

On 27 February 2018 14:28, Shawn Lin wrote:

在 2018/2/27 21:55, Phil Edworthy 写道:

Since the controller does not support the end-of-busy IRQ, don't use

it.

Otherwise, on older SD cards you will get lots of these messages:
"mmc0: Got data interrupt 0x0002 even though no data operation
was

in progress"




I'm afraid you have to explain which version of arasan's IP suffer
from this and what does the "older SD cards" mean?

Ok, I'll try to find out the IP version...
For "older SD cards", I can provide a list of a few cards that
exhibit this problem and others that don't, is that enough info?


What I meant is could you elaborate more about what kind of cards,
e.g, are them the  legacy SDSC cards or SDHC cards, or maybe they are
only running with defaut speed? or whatever, but not just with a
vague "older" cards. :)

Unfortunately, I have one SDHC card that works, one that doesn't. Both
cards are running with a 50MHz SD clock. All I know is this:


Thanks for sharing these, though it looks wired as I never remember I saw
this problem when extensively tested SD cards on one of arasan controllers
in 2014.

Not sure what you mean by 'wired'?

Note that this is on a relatively slow device, a dual core Cortex A7 @500MHz.
Maybe that has some effect.


That's why I hope you could add the IP version in your commit msg, and
that would be a hint for why it behaved different over platforms.



It's also interesting that someone posted the same fix for Xilinx a while
back, I linked to it in the commit msg.

Thanks
Phil



SD cards that report unexpected interrupts:
2GB Sandisk Extreme III (e624 SD02G 1.89 GiB)
8GB Sandisk (SDHC class 4)  ( SU08G 7.40 GiB)
8GB Sandisk Extreme III (SDHC class 6)(bb4e SD08G 7.61 GiB)

SD cards that work ok:
16GB Samsung (microSDHC U1 class 10)  (0001 0 14.6 GiB)
16GB Sandisk Ultra (microSDHC U1 class 10)( SL16G 14.8 GiB)
32GB Sandisk Ultra (microSDHC U1 class 10)( SL32G 29.7 GiB)

Thanks
Phil


This has been reported on Xilinx devices that also use the Arasan IP.
See https://patchwork.kernel.org/patch/8062871/

This has been tested on the Renesas RZ/ND-DB board with the RZ/N1

SoC.


Signed-off-by: Phil Edworthy 
---
 drivers/mmc/host/sdhci-of-arasan.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c
b/drivers/mmc/host/sdhci-of-arasan.c
index c33a5f7..ab66e32 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -290,7 +290,8 @@ static const struct sdhci_pltfm_data

sdhci_arasan_pdata = {

.ops = _arasan_ops,
.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
-   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
+   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN

|

+   SDHCI_QUIRK2_STOP_WITH_TC,
 };

 static u32 sdhci_arasan_cqhci_irq(struct sdhci_host *host, u32
intmask)




--
Best Regards
Shawn Lin








--
Best Regards
Shawn Lin

Re: [PATCH v3] mmc: Export host capabilities to debugfs.

[Shawn Lin]

Reviewed-by: Shawn Lin <shawn@rock-chips.com>

Re: [PATCH v3] mmc: Export host capabilities to debugfs.

[Shawn Lin]

Reviewed-by: Shawn Lin 

Re: [PATCH] soc: rockchip: power-domain: Add a sanity check on pd->num_clks

[Shawn Lin]

Hi Jeffy,

On 2018/3/5 17:17, Jeffy Chen wrote:

The of_count_phandle_with_args() can fail and return error(for example,
rk3399 pd_vio doesn't have clocks). That would break the pd probe.

Add a sanity check on pd->num_clks to avoid that.

Fixes: 65084121d59d ("soc: rockchip: power-domain: use clk_bulk APIs")
Reported-by: Shawn Lin <shawn@rock-chips.com>
Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>

Tested-by: Shawn Lin <shawn@rock-chips.com>

Re: [PATCH] soc: rockchip: power-domain: Add a sanity check on pd->num_clks

[Shawn Lin]

Hi Jeffy,

On 2018/3/5 17:17, Jeffy Chen wrote:

The of_count_phandle_with_args() can fail and return error(for example,
rk3399 pd_vio doesn't have clocks). That would break the pd probe.

Add a sanity check on pd->num_clks to avoid that.

Fixes: 65084121d59d ("soc: rockchip: power-domain: use clk_bulk APIs")
Reported-by: Shawn Lin 
Signed-off-by: Jeffy Chen 

Tested-by: Shawn Lin 

Re: [PATCH] soc: rockchip: power-domain: use clk_bulk APIs

[Shawn Lin]

Hi Heiko,

On 2018/3/2 23:43, Heiko Stuebner wrote:

Hi Jeffy,

Am Mittwoch, 28. Februar 2018, 13:41:43 CET schrieb Jeffy Chen:

Use clk_bulk APIs, and also add error handling for clk enable.

Signed-off-by: Jeffy Chen 


[...]


-   for (i = 0; i < clk_cnt; i++) {
-   clk = of_clk_get(node, i);
-   if (IS_ERR(clk)) {
-   error = PTR_ERR(clk);
+   pd->num_clks = of_count_phandle_with_args(node, "clocks",
+ "#clock-cells");
+
+   pd->clks = devm_kzalloc(pmu->dev, pd->num_clks * sizeof(pd->clks[0]),


This doesn't work for rk3399, as the pd_vio doesn't have any clocks
attached.

[0.713241] rockchip-pm-domain 
ff31.power-management:power-controller: failed to handle node 
pd_vio: -12
[0.714615] rockchip-pm-domain: probe of 
ff31.power-management:power-controller failed with error -12



I think Jeffy's v2 is coming, so I assume you will drop this version?




applied for 4.17, after changing to devm_kcalloc like below:

pd->clks = devm_kcalloc(pmu->dev, pd->num_clks, sizeof(*pd->clks),


Thanks
Heiko

___
Linux-rockchip mailing list
linux-rockc...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] soc: rockchip: power-domain: use clk_bulk APIs

[Shawn Lin]

Hi Heiko,

On 2018/3/2 23:43, Heiko Stuebner wrote:

Hi Jeffy,

Am Mittwoch, 28. Februar 2018, 13:41:43 CET schrieb Jeffy Chen:

Use clk_bulk APIs, and also add error handling for clk enable.

Signed-off-by: Jeffy Chen 


[...]


-   for (i = 0; i < clk_cnt; i++) {
-   clk = of_clk_get(node, i);
-   if (IS_ERR(clk)) {
-   error = PTR_ERR(clk);
+   pd->num_clks = of_count_phandle_with_args(node, "clocks",
+ "#clock-cells");
+
+   pd->clks = devm_kzalloc(pmu->dev, pd->num_clks * sizeof(pd->clks[0]),


This doesn't work for rk3399, as the pd_vio doesn't have any clocks
attached.

[0.713241] rockchip-pm-domain 
ff31.power-management:power-controller: failed to handle node 
pd_vio: -12
[0.714615] rockchip-pm-domain: probe of 
ff31.power-management:power-controller failed with error -12



I think Jeffy's v2 is coming, so I assume you will drop this version?




applied for 4.17, after changing to devm_kcalloc like below:

pd->clks = devm_kcalloc(pmu->dev, pd->num_clks, sizeof(*pd->clks),


Thanks
Heiko

___
Linux-rockchip mailing list
linux-rockc...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2] mmc: Export host capabilities to debugfs.

[Shawn Lin]

On 2018/3/5 12:24, Harish Jenny K N wrote:

From: Abbas Raza 

This patch exports the host capabilities to debugfs

Signed-off-by: Abbas Raza 
Signed-off-by: Andrew Gabbasov 
Signed-off-by: Harish Jenny K N 
---

Changes in v2:
- Changed Author

  drivers/mmc/core/debugfs.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
index c51e0c0..fa2df7f 100644
--- a/drivers/mmc/core/debugfs.c
+++ b/drivers/mmc/core/debugfs.c
@@ -289,6 +289,9 @@ void mmc_add_card_debugfs(struct mmc_card *card)
  
  	card->debugfs_root = root;
  
+	if (!debugfs_create_x32("host_caps", S_IRUSR, root, (u32 *)>caps))


Perhaps you don't need cast, and '>caps' should be fine?
And you don't need to export host->caps2?


+   goto err;
+
if (!debugfs_create_x32("state", S_IRUSR, root, >state))
goto err;
  

Re: [PATCH v2] mmc: Export host capabilities to debugfs.

[Shawn Lin]

On 2018/3/5 12:24, Harish Jenny K N wrote:

From: Abbas Raza 

This patch exports the host capabilities to debugfs

Signed-off-by: Abbas Raza 
Signed-off-by: Andrew Gabbasov 
Signed-off-by: Harish Jenny K N 
---

Changes in v2:
- Changed Author

  drivers/mmc/core/debugfs.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
index c51e0c0..fa2df7f 100644
--- a/drivers/mmc/core/debugfs.c
+++ b/drivers/mmc/core/debugfs.c
@@ -289,6 +289,9 @@ void mmc_add_card_debugfs(struct mmc_card *card)
  
  	card->debugfs_root = root;
  
+	if (!debugfs_create_x32("host_caps", S_IRUSR, root, (u32 *)>caps))


Perhaps you don't need cast, and '>caps' should be fine?
And you don't need to export host->caps2?


+   goto err;
+
if (!debugfs_create_x32("state", S_IRUSR, root, >state))
goto err;
  

Re: [PATCH] mmc: sdhci-of-arasan: Add quirk to avoid erroneous msg

[Shawn Lin]

On 2018/2/27 23:05, Phil Edworthy wrote:

Hi Shawn,

On 27 February 2018 14:42, Shawn Lin wrote:

On 2018/2/27 22:31, Phil Edworthy wrote:

Hi Shawn,

On 27 February 2018 14:28, Shawn Lin wrote:

在 2018/2/27 21:55, Phil Edworthy 写道:

Since the controller does not support the end-of-busy IRQ, don't use it.
Otherwise, on older SD cards you will get lots of these messages:
"mmc0: Got data interrupt 0x0002 even though no data operation
was

in progress"




I'm afraid you have to explain which version of arasan's IP suffer
from this and what does the "older SD cards" mean?

Ok, I'll try to find out the IP version...
For "older SD cards", I can provide a list of a few cards that exhibit
this problem and others that don't, is that enough info?


What I meant is could you elaborate more about what kind of cards, e.g, are
them the  legacy SDSC cards or SDHC cards, or maybe they are only running
with defaut speed? or whatever, but not just with a vague "older" cards. :)

Unfortunately, I have one SDHC card that works, one that doesn't. Both cards
are running with a 50MHz SD clock. All I know is this:


Thanks for sharing these, though it looks wired as I never remember I
saw this problem when extensively tested SD cards on one of arasan
controllers in 2014.



SD cards that report unexpected interrupts:
2GB Sandisk Extreme III (e624 SD02G 1.89 GiB)
8GB Sandisk (SDHC class 4)  ( SU08G 7.40 GiB)
8GB Sandisk Extreme III (SDHC class 6)(bb4e SD08G 7.61 GiB)

SD cards that work ok:
16GB Samsung (microSDHC U1 class 10)  (0001 0 14.6 GiB)
16GB Sandisk Ultra (microSDHC U1 class 10)( SL16G 14.8 GiB)
32GB Sandisk Ultra (microSDHC U1 class 10)( SL32G 29.7 GiB)

Thanks
Phil


This has been reported on Xilinx devices that also use the Arasan IP.
See https://patchwork.kernel.org/patch/8062871/

This has been tested on the Renesas RZ/ND-DB board with the RZ/N1

SoC.


Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
---
drivers/mmc/host/sdhci-of-arasan.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c
b/drivers/mmc/host/sdhci-of-arasan.c
index c33a5f7..ab66e32 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -290,7 +290,8 @@ static const struct sdhci_pltfm_data

sdhci_arasan_pdata = {

.ops = _arasan_ops,
.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
-   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
+   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN |
+   SDHCI_QUIRK2_STOP_WITH_TC,
};

static u32 sdhci_arasan_cqhci_irq(struct sdhci_host *host, u32
intmask)




--
Best Regards
Shawn Lin








--
Best Regards
Shawn Lin

Re: [PATCH] mmc: sdhci-of-arasan: Add quirk to avoid erroneous msg

[Shawn Lin]

On 2018/2/27 23:05, Phil Edworthy wrote:

Hi Shawn,

On 27 February 2018 14:42, Shawn Lin wrote:

On 2018/2/27 22:31, Phil Edworthy wrote:

Hi Shawn,

On 27 February 2018 14:28, Shawn Lin wrote:

在 2018/2/27 21:55, Phil Edworthy 写道:

Since the controller does not support the end-of-busy IRQ, don't use it.
Otherwise, on older SD cards you will get lots of these messages:
"mmc0: Got data interrupt 0x0002 even though no data operation
was

in progress"




I'm afraid you have to explain which version of arasan's IP suffer
from this and what does the "older SD cards" mean?

Ok, I'll try to find out the IP version...
For "older SD cards", I can provide a list of a few cards that exhibit
this problem and others that don't, is that enough info?


What I meant is could you elaborate more about what kind of cards, e.g, are
them the  legacy SDSC cards or SDHC cards, or maybe they are only running
with defaut speed? or whatever, but not just with a vague "older" cards. :)

Unfortunately, I have one SDHC card that works, one that doesn't. Both cards
are running with a 50MHz SD clock. All I know is this:


Thanks for sharing these, though it looks wired as I never remember I
saw this problem when extensively tested SD cards on one of arasan
controllers in 2014.



SD cards that report unexpected interrupts:
2GB Sandisk Extreme III (e624 SD02G 1.89 GiB)
8GB Sandisk (SDHC class 4)  ( SU08G 7.40 GiB)
8GB Sandisk Extreme III (SDHC class 6)(bb4e SD08G 7.61 GiB)

SD cards that work ok:
16GB Samsung (microSDHC U1 class 10)  (0001 0 14.6 GiB)
16GB Sandisk Ultra (microSDHC U1 class 10)( SL16G 14.8 GiB)
32GB Sandisk Ultra (microSDHC U1 class 10)( SL32G 29.7 GiB)

Thanks
Phil


This has been reported on Xilinx devices that also use the Arasan IP.
See https://patchwork.kernel.org/patch/8062871/

This has been tested on the Renesas RZ/ND-DB board with the RZ/N1

SoC.


Signed-off-by: Phil Edworthy 
---
drivers/mmc/host/sdhci-of-arasan.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c
b/drivers/mmc/host/sdhci-of-arasan.c
index c33a5f7..ab66e32 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -290,7 +290,8 @@ static const struct sdhci_pltfm_data

sdhci_arasan_pdata = {

.ops = _arasan_ops,
.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
-   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
+   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN |
+   SDHCI_QUIRK2_STOP_WITH_TC,
};

static u32 sdhci_arasan_cqhci_irq(struct sdhci_host *host, u32
intmask)




--
Best Regards
Shawn Lin








--
Best Regards
Shawn Lin

Re: [PATCH] mmc: sdhci-of-arasan: Add quirk to avoid erroneous msg

[Shawn Lin]

On 2018/2/27 22:31, Phil Edworthy wrote:

Hi Shawn,

On 27 February 2018 14:28, Shawn Lin wrote:

在 2018/2/27 21:55, Phil Edworthy 写道:

Since the controller does not support the end-of-busy IRQ, don't use it.
Otherwise, on older SD cards you will get lots of these messages:
"mmc0: Got data interrupt 0x0002 even though no data operation was

in progress"




I'm afraid you have to explain which version of arasan's IP suffer from this
and what does the "older SD cards" mean?

Ok, I'll try to find out the IP version...
For "older SD cards", I can provide a list of a few cards that exhibit this 
problem
and others that don't, is that enough info?


What I meant is could you elaborate more about what kind of cards, e.g,
are them the  legacy SDSC cards or SDHC cards, or maybe they are only
running with defaut speed? or whatever, but not just with a vague
"older" cards. :)



Thanks
Phil


This has been reported on Xilinx devices that also use the Arasan IP.
See https://patchwork.kernel.org/patch/8062871/

This has been tested on the Renesas RZ/ND-DB board with the RZ/N1 SoC.

Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
---
   drivers/mmc/host/sdhci-of-arasan.c | 3 ++-
   1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c
b/drivers/mmc/host/sdhci-of-arasan.c
index c33a5f7..ab66e32 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -290,7 +290,8 @@ static const struct sdhci_pltfm_data

sdhci_arasan_pdata = {

.ops = _arasan_ops,
.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
-   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
+   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN |
+   SDHCI_QUIRK2_STOP_WITH_TC,
   };

   static u32 sdhci_arasan_cqhci_irq(struct sdhci_host *host, u32
intmask)




--
Best Regards
Shawn Lin








--
Best Regards
Shawn Lin

Re: [PATCH] mmc: sdhci-of-arasan: Add quirk to avoid erroneous msg

[Shawn Lin]

On 2018/2/27 22:31, Phil Edworthy wrote:

Hi Shawn,

On 27 February 2018 14:28, Shawn Lin wrote:

在 2018/2/27 21:55, Phil Edworthy 写道:

Since the controller does not support the end-of-busy IRQ, don't use it.
Otherwise, on older SD cards you will get lots of these messages:
"mmc0: Got data interrupt 0x0002 even though no data operation was

in progress"




I'm afraid you have to explain which version of arasan's IP suffer from this
and what does the "older SD cards" mean?

Ok, I'll try to find out the IP version...
For "older SD cards", I can provide a list of a few cards that exhibit this 
problem
and others that don't, is that enough info?


What I meant is could you elaborate more about what kind of cards, e.g,
are them the  legacy SDSC cards or SDHC cards, or maybe they are only
running with defaut speed? or whatever, but not just with a vague
"older" cards. :)



Thanks
Phil


This has been reported on Xilinx devices that also use the Arasan IP.
See https://patchwork.kernel.org/patch/8062871/

This has been tested on the Renesas RZ/ND-DB board with the RZ/N1 SoC.

Signed-off-by: Phil Edworthy 
---
   drivers/mmc/host/sdhci-of-arasan.c | 3 ++-
   1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c
b/drivers/mmc/host/sdhci-of-arasan.c
index c33a5f7..ab66e32 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -290,7 +290,8 @@ static const struct sdhci_pltfm_data

sdhci_arasan_pdata = {

.ops = _arasan_ops,
.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
-   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
+   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN |
+   SDHCI_QUIRK2_STOP_WITH_TC,
   };

   static u32 sdhci_arasan_cqhci_irq(struct sdhci_host *host, u32
intmask)




--
Best Regards
Shawn Lin








--
Best Regards
Shawn Lin

Re: [PATCH] mmc: sdhci-of-arasan: Add quirk to avoid erroneous msg

[Shawn Lin]

在 2018/2/27 21:55, Phil Edworthy 写道:

Since the controller does not support the end-of-busy IRQ, don't use it.
Otherwise, on older SD cards you will get lots of these messages:
"mmc0: Got data interrupt 0x0002 even though no data operation was in 
progress"



I'm afraid you have to explain which version of arasan's IP suffer from
this and what does the "older SD cards" mean?


This has been reported on Xilinx devices that also use the Arasan IP.
See https://patchwork.kernel.org/patch/8062871/

This has been tested on the Renesas RZ/ND-DB board with the RZ/N1 SoC.

Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
---
  drivers/mmc/host/sdhci-of-arasan.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c 
b/drivers/mmc/host/sdhci-of-arasan.c
index c33a5f7..ab66e32 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -290,7 +290,8 @@ static const struct sdhci_pltfm_data sdhci_arasan_pdata = {
.ops = _arasan_ops,
.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
-   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
+   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN |
+   SDHCI_QUIRK2_STOP_WITH_TC,
  };
  
  static u32 sdhci_arasan_cqhci_irq(struct sdhci_host *host, u32 intmask)





--
Best Regards
Shawn Lin

Re: [PATCH] mmc: sdhci-of-arasan: Add quirk to avoid erroneous msg

[Shawn Lin]

在 2018/2/27 21:55, Phil Edworthy 写道:

Since the controller does not support the end-of-busy IRQ, don't use it.
Otherwise, on older SD cards you will get lots of these messages:
"mmc0: Got data interrupt 0x0002 even though no data operation was in 
progress"



I'm afraid you have to explain which version of arasan's IP suffer from
this and what does the "older SD cards" mean?


This has been reported on Xilinx devices that also use the Arasan IP.
See https://patchwork.kernel.org/patch/8062871/

This has been tested on the Renesas RZ/ND-DB board with the RZ/N1 SoC.

Signed-off-by: Phil Edworthy 
---
  drivers/mmc/host/sdhci-of-arasan.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c 
b/drivers/mmc/host/sdhci-of-arasan.c
index c33a5f7..ab66e32 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -290,7 +290,8 @@ static const struct sdhci_pltfm_data sdhci_arasan_pdata = {
.ops = _arasan_ops,
.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
-   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
+   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN |
+   SDHCI_QUIRK2_STOP_WITH_TC,
  };
  
  static u32 sdhci_arasan_cqhci_irq(struct sdhci_host *host, u32 intmask)





--
Best Regards
Shawn Lin

Re: [PATCH] mmc: card: Don't show eMMC RPMB and BOOT areas in /proc/partitions

[Shawn Lin]

在 2018/2/27 19:33, Harish Jenny K N 写道:

From: Andrew Gabbasov <andrew_gabba...@mentor.com>

Since RPMB area is accessible via special ioctl only and boot areas
are unlikely to contain any partitions, exclude them all from listing
in /proc/partitions. This will hide them from various user-level
software (e.g. fdisk), thus avoiding unnecessary access attempts.

Signed-off-by: Andrew Gabbasov <andrew_gabba...@mentor.com>
Signed-off-by: Harish Jenny K N <harish_kand...@mentor.com>
---
  drivers/mmc/core/block.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 20135a5..376e47e 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -2341,7 +2341,8 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct 
mmc_card *card,
set_disk_ro(md->disk, md->read_only || default_ro);
md->disk->flags = GENHD_FL_EXT_DEVT;
if (area_type & (MMC_BLK_DATA_AREA_RPMB | MMC_BLK_DATA_AREA_BOOT))
-   md->disk->flags |= GENHD_FL_NO_PART_SCAN;
+   md->disk->flags |= GENHD_FL_NO_PART_SCAN
+  | GENHD_FL_SUPPRESS_PARTITION_INFO;


I would prefer using GENHD_FL_HIDDEN instead of adding all these two
flags.

  
  	/*

 * As discussed on lkml, GENHD_FL_REMOVABLE should:




--
Best Regards
Shawn Lin

Re: [PATCH] mmc: card: Don't show eMMC RPMB and BOOT areas in /proc/partitions

[Shawn Lin]

在 2018/2/27 19:33, Harish Jenny K N 写道:

From: Andrew Gabbasov 

Since RPMB area is accessible via special ioctl only and boot areas
are unlikely to contain any partitions, exclude them all from listing
in /proc/partitions. This will hide them from various user-level
software (e.g. fdisk), thus avoiding unnecessary access attempts.

Signed-off-by: Andrew Gabbasov 
Signed-off-by: Harish Jenny K N 
---
  drivers/mmc/core/block.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 20135a5..376e47e 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -2341,7 +2341,8 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct 
mmc_card *card,
set_disk_ro(md->disk, md->read_only || default_ro);
md->disk->flags = GENHD_FL_EXT_DEVT;
if (area_type & (MMC_BLK_DATA_AREA_RPMB | MMC_BLK_DATA_AREA_BOOT))
-   md->disk->flags |= GENHD_FL_NO_PART_SCAN;
+   md->disk->flags |= GENHD_FL_NO_PART_SCAN
+  | GENHD_FL_SUPPRESS_PARTITION_INFO;


I would prefer using GENHD_FL_HIDDEN instead of adding all these two
flags.

  
  	/*

 * As discussed on lkml, GENHD_FL_REMOVABLE should:




--
Best Regards
Shawn Lin

Re: [PATCH 1/2 v3] mmc: dw_mmc: Fix the DTO timeout overflow calculation for 32-bit systems

[Shawn Lin]

Tested-by: Vineet Gupta 
Fixes: ARC STAR 9001306872 HSDK, sdio: board crashes when copying big files

Signed-off-by: Evgeniy Didin 

CC: Alexey Brodkin 
CC: Eugeniy Paltsev 
CC: Douglas Anderson 
CC: Ulf Hansson 
CC: linux-kernel@vger.kernel.org
CC: linux-snps-...@lists.infradead.org
Cc:  #  9d9491a7da2a mmc: dw_mmc: Fix the DTO timeout 
calculation


As I said, the correct tag may be:

Reported-by: Vineet Gupta  # ARC STAR 
9001306872 HSDK, sdio: board crashes when copying big files

Tested-by: Vineet Gupta 
Fixes: 9d9491a7da2a ("mmc: dw_mmc: Fix the DTO timeout calculation")
Cc: 
Signed-off-by: Evgeniy Didin 
...
...



---
Nothing changed since v2.
  drivers/mmc/host/dw_mmc.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)




-   drto_ms = DIV_ROUND_UP(MSEC_PER_SEC * drto_clks * drto_div,
+
+   drto_ms = DIV_ROUND_UP((u64)MSEC_PER_SEC * drto_clks * drto_div,
   host->bus_hz);
  


Hmm?

#define DIV_ROUND_DOWN_ULL(ll, d) \
({ unsigned long long _tmp = (ll); do_div(_tmp, d); _tmp; })

#define DIV_ROUND_UP_ULL(ll, d) DIV_ROUND_DOWN_ULL((ll) + (d) - 1, (d))


It uses intermediate unsigned long long _tmp for your "multiply", namely
MSEC_PER_SEC * drto_clks * drto_div, which could solves the problem.
So I don't see why DIV_ROUND_UP_ULL can't work for you?



/* add a bit spare time */

Re: [PATCH 1/2 v3] mmc: dw_mmc: Fix the DTO timeout overflow calculation for 32-bit systems

[Shawn Lin]

Tested-by: Vineet Gupta 
Fixes: ARC STAR 9001306872 HSDK, sdio: board crashes when copying big files

Signed-off-by: Evgeniy Didin 

CC: Alexey Brodkin 
CC: Eugeniy Paltsev 
CC: Douglas Anderson 
CC: Ulf Hansson 
CC: linux-kernel@vger.kernel.org
CC: linux-snps-...@lists.infradead.org
Cc:  #  9d9491a7da2a mmc: dw_mmc: Fix the DTO timeout 
calculation


As I said, the correct tag may be:

Reported-by: Vineet Gupta  # ARC STAR 
9001306872 HSDK, sdio: board crashes when copying big files

Tested-by: Vineet Gupta 
Fixes: 9d9491a7da2a ("mmc: dw_mmc: Fix the DTO timeout calculation")
Cc: 
Signed-off-by: Evgeniy Didin 
...
...



---
Nothing changed since v2.
  drivers/mmc/host/dw_mmc.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)




-   drto_ms = DIV_ROUND_UP(MSEC_PER_SEC * drto_clks * drto_div,
+
+   drto_ms = DIV_ROUND_UP((u64)MSEC_PER_SEC * drto_clks * drto_div,
   host->bus_hz);
  


Hmm?

#define DIV_ROUND_DOWN_ULL(ll, d) \
({ unsigned long long _tmp = (ll); do_div(_tmp, d); _tmp; })

#define DIV_ROUND_UP_ULL(ll, d) DIV_ROUND_DOWN_ULL((ll) + (d) - 1, (d))


It uses intermediate unsigned long long _tmp for your "multiply", namely
MSEC_PER_SEC * drto_clks * drto_div, which could solves the problem.
So I don't see why DIV_ROUND_UP_ULL can't work for you?



/* add a bit spare time */

Re: [PATCH 1/6] mmc: dw_mmc: remove the deprecated "clock-freq-min-max" property

[Shawn Lin]

Hi Andy,

On 2018/2/23 21:27, Andy Shevchenko wrote:

On Fri, Feb 23, 2018 at 8:41 AM, Jaehoon Chung <jh80.ch...@samsung.com> wrote:

'clock-freq-min-max' property had already deprecated.
Remove the 'clock-freq-min-max' property that is kept to maintain
the compatibility.


Removing a property without telling the user what to expect is a bad
idea and ABI breakage.



What's the general process to remove a property?

I guess we should do:
1) deprecate it in the first place and remove it from all upstream DT
2) wait some long enough days for expecting the stale of all old DTB
containing that property
3) remove the functionality of the deprecated property from the driver
but still leave some warning there
4) remove the left warning finally

And for the ABI breakage, we should add something in Documentation/ABI
/obsolete  or Documentation/ABI/removed ?

--
Best Regards
Shawn Lin

Re: [PATCH 1/6] mmc: dw_mmc: remove the deprecated "clock-freq-min-max" property

[Shawn Lin]

Hi Andy,

On 2018/2/23 21:27, Andy Shevchenko wrote:

On Fri, Feb 23, 2018 at 8:41 AM, Jaehoon Chung  wrote:

'clock-freq-min-max' property had already deprecated.
Remove the 'clock-freq-min-max' property that is kept to maintain
the compatibility.


Removing a property without telling the user what to expect is a bad
idea and ABI breakage.



What's the general process to remove a property?

I guess we should do:
1) deprecate it in the first place and remove it from all upstream DT
2) wait some long enough days for expecting the stale of all old DTB
containing that property
3) remove the functionality of the deprecated property from the driver
but still leave some warning there
4) remove the left warning finally

And for the ABI breakage, we should add something in Documentation/ABI
/obsolete  or Documentation/ABI/removed ?

--
Best Regards
Shawn Lin

Re: [PATCH v2] mmc: dw_mmc-k3: Fix out-of-bounds access through DT alias

[Shawn Lin]

On 2018/2/23 20:44, Geert Uytterhoeven wrote:

The hs_timing_cfg[] array is indexed using a value derived from the
"mshcN" alias in DT, which may lead to an out-of-bounds access.



Reviewed-by: Shawn Lin <shawn@rock-chips.com>

--
Best Regards
Shawn Lin

Re: [PATCH v2] mmc: dw_mmc-k3: Fix out-of-bounds access through DT alias

[Shawn Lin]

On 2018/2/23 20:44, Geert Uytterhoeven wrote:

The hs_timing_cfg[] array is indexed using a value derived from the
"mshcN" alias in DT, which may lead to an out-of-bounds access.



Reviewed-by: Shawn Lin 

--
Best Regards
Shawn Lin

Re: [PATCH] mmc: dw_mmc: update kernel-doc comments for dw_mci

[Shawn Lin]

Hi Alexey,

On 2018/2/23 3:45, Alexey Roslyakov wrote:

cur_slot and num_slots has been removed from struct dw_mci in 42f989c002f2.
Unfortunately, inline documentation was not updated so far.

Fix @lock field documentation in Locking section.
Move @mrq field of struct dw_mci_slot mention closer to it
description, so no one could miss this slightest detail.

Couple of code style fixes as a bonus.



Thanks for updating these.

Reviewed-by: Shawn Lin <shawn@rock-chips.com>


Signed-off-by: Alexey Roslyakov <alexey.roslya...@gmail.com>
---
  drivers/mmc/host/dw_mmc.h | 19 +--
  1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index e3124f06a47e..451bc0862493 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -65,8 +65,7 @@ struct dw_mci_dma_slave {
   * @fifo_reg: Pointer to MMIO registers for data FIFO
   * @sg: Scatterlist entry currently being processed by PIO code, if any.
   * @sg_miter: PIO mapping scatterlist iterator.
- * @cur_slot: The slot which is currently using the controller.
- * @mrq: The request currently being processed on @cur_slot,
+ * @mrq: The request currently being processed on @slot,
   *or NULL if the controller is idle.
   * @cmd: The command currently being sent to the card, or NULL.
   * @data: The data currently being transferred, or NULL if no data
@@ -102,7 +101,6 @@ struct dw_mci_dma_slave {
   * @bus_hz: The rate of @mck in Hz. This forms the basis for MMC bus
   *rate and timeout calculations.
   * @current_speed: Configured rate of the controller.
- * @num_slots: Number of slots available.
   * @fifoth_val: The value of FIFOTH register.
   * @verid: Denote Version ID.
   * @dev: Device associated with the MMC controller.
@@ -134,17 +132,17 @@ struct dw_mci_dma_slave {
   * ===
   *
   * @lock is a softirq-safe spinlock protecting @queue as well as
+ * @slot, @mrq and @state. These must always be updated
   * at the same time while holding @lock.
+ * The @mrq field of struct dw_mci_slot is also protected by @lock,
+ * and must always be written at the same time as the slot is added to
+ * @queue.
   *
   * @irq_lock is an irq-safe spinlock protecting the INTMASK register
   * to allow the interrupt handler to modify it directly.  Held for only long
   * enough to read-modify-write INTMASK and no other locks are grabbed when
   * holding this one.
   *
- * The @mrq field of struct dw_mci_slot is also protected by @lock,
- * and must always be written at the same time as the slot is added to
- * @queue.
- *
   * @pending_events and @completed_events are accessed using atomic bit
   * operations, so they don't need any locking.
   *
@@ -321,8 +319,8 @@ struct dw_mci_board {
  #define SDMMC_ENABLE_SHIFT0x110
  #define SDMMC_DATA(x) (x)
  /*
-* Registers to support idmac 64-bit address mode
-*/
+ * Registers to support idmac 64-bit address mode
+ */
  #define SDMMC_DBADDRL 0x088
  #define SDMMC_DBADDRU 0x08c
  #define SDMMC_IDSTS64 0x090
@@ -449,7 +447,8 @@ struct dw_mci_board {
(SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET | SDMMC_CTRL_DMA_RESET)
  
  /* FIFO register access macros. These should not change the data endian-ness

- * as they are written to memory to be dealt with by the upper layers */
+ * as they are written to memory to be dealt with by the upper layers
+ */
  #define mci_fifo_readw(__reg) __raw_readw(__reg)
  #define mci_fifo_readl(__reg) __raw_readl(__reg)
  #define mci_fifo_readq(__reg) __raw_readq(__reg)

Re: [PATCH] mmc: dw_mmc: update kernel-doc comments for dw_mci

[Shawn Lin]

Hi Alexey,

On 2018/2/23 3:45, Alexey Roslyakov wrote:

cur_slot and num_slots has been removed from struct dw_mci in 42f989c002f2.
Unfortunately, inline documentation was not updated so far.

Fix @lock field documentation in Locking section.
Move @mrq field of struct dw_mci_slot mention closer to it
description, so no one could miss this slightest detail.

Couple of code style fixes as a bonus.



Thanks for updating these.

Reviewed-by: Shawn Lin 


Signed-off-by: Alexey Roslyakov 
---
  drivers/mmc/host/dw_mmc.h | 19 +--
  1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index e3124f06a47e..451bc0862493 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -65,8 +65,7 @@ struct dw_mci_dma_slave {
   * @fifo_reg: Pointer to MMIO registers for data FIFO
   * @sg: Scatterlist entry currently being processed by PIO code, if any.
   * @sg_miter: PIO mapping scatterlist iterator.
- * @cur_slot: The slot which is currently using the controller.
- * @mrq: The request currently being processed on @cur_slot,
+ * @mrq: The request currently being processed on @slot,
   *or NULL if the controller is idle.
   * @cmd: The command currently being sent to the card, or NULL.
   * @data: The data currently being transferred, or NULL if no data
@@ -102,7 +101,6 @@ struct dw_mci_dma_slave {
   * @bus_hz: The rate of @mck in Hz. This forms the basis for MMC bus
   *rate and timeout calculations.
   * @current_speed: Configured rate of the controller.
- * @num_slots: Number of slots available.
   * @fifoth_val: The value of FIFOTH register.
   * @verid: Denote Version ID.
   * @dev: Device associated with the MMC controller.
@@ -134,17 +132,17 @@ struct dw_mci_dma_slave {
   * ===
   *
   * @lock is a softirq-safe spinlock protecting @queue as well as
+ * @slot, @mrq and @state. These must always be updated
   * at the same time while holding @lock.
+ * The @mrq field of struct dw_mci_slot is also protected by @lock,
+ * and must always be written at the same time as the slot is added to
+ * @queue.
   *
   * @irq_lock is an irq-safe spinlock protecting the INTMASK register
   * to allow the interrupt handler to modify it directly.  Held for only long
   * enough to read-modify-write INTMASK and no other locks are grabbed when
   * holding this one.
   *
- * The @mrq field of struct dw_mci_slot is also protected by @lock,
- * and must always be written at the same time as the slot is added to
- * @queue.
- *
   * @pending_events and @completed_events are accessed using atomic bit
   * operations, so they don't need any locking.
   *
@@ -321,8 +319,8 @@ struct dw_mci_board {
  #define SDMMC_ENABLE_SHIFT0x110
  #define SDMMC_DATA(x) (x)
  /*
-* Registers to support idmac 64-bit address mode
-*/
+ * Registers to support idmac 64-bit address mode
+ */
  #define SDMMC_DBADDRL 0x088
  #define SDMMC_DBADDRU 0x08c
  #define SDMMC_IDSTS64 0x090
@@ -449,7 +447,8 @@ struct dw_mci_board {
(SDMMC_CTRL_RESET | SDMMC_CTRL_FIFO_RESET | SDMMC_CTRL_DMA_RESET)
  
  /* FIFO register access macros. These should not change the data endian-ness

- * as they are written to memory to be dealt with by the upper layers */
+ * as they are written to memory to be dealt with by the upper layers
+ */
  #define mci_fifo_readw(__reg) __raw_readw(__reg)
  #define mci_fifo_readl(__reg) __raw_readl(__reg)
  #define mci_fifo_readq(__reg) __raw_readq(__reg)

Re: [PATCH 2/5] mmc: add stm32 sdmmc controller driver

[Shawn Lin]

MC_IDMACTRLR0x050
+#define IDMACTRLR_IDMAEN   BIT(0)
+#define IDMACTRLR_IDMABMODEBIT(1)
+#define IDMACTRLR_IDMABACT BIT(2)
+
+#define SDMMC_IDMABSIZER   0x054
+#define IDMABSIZER_IDMABNDT_MASK   GENMASK(12, 5)
+
+#define SDMMC_IDMABASE0R   0x058
+#define SDMMC_IDMABASE1R   0x05c
+
+#define SDMMC_IPVR 0x3fc
+#define IPVR_MINREV_MASK   GENMASK(3, 0)
+#define IPVR_MAJREV_MASK   GENMASK(7, 4)
+
+enum stm32_sdmmc_cookie {
+   COOKIE_UNMAPPED,
+   COOKIE_PRE_MAPPED,  /* mapped by pre_req() of stm32 */
+   COOKIE_MAPPED,  /* mapped by prepare_data() of stm32 */
+};
+
+struct sdmmc_stat {
+   unsigned long   n_req;
+       unsigned long   n_datareq;
+   unsigned long   n_ctimeout;
+   unsigned long   n_ccrcfail;
+   unsigned long   n_dtimeout;
+   unsigned long   n_dcrcfail;
+   unsigned long   n_txunderrun;
+   unsigned long   n_rxoverrun;
+   unsigned long   nb_dma_err;
+};
+
+struct sdmmc_host {
+   void __iomem*base;
+   struct mmc_host *mmc;
+   struct clk  *clk;
+   struct reset_control*rst;
+
+   u32 clk_reg_add;
+   u32 pwr_reg_add;
+
+   struct mmc_request  *mrq;
+   struct mmc_command  *cmd;
+   struct mmc_data *data;
+   struct mmc_command  stop_abort;
+   booldpsm_abort;
+
+   /* protect host registers access */
+   spinlock_t  lock;
+
+   unsigned intsdmmcclk;
+   unsigned intsdmmc_ck;
+
+   u32 size;
+
+   u32 ip_ver;
+   struct sdmmc_stat   stat;
+};




--
Best Regards
Shawn Lin

Re: [PATCH 2/5] mmc: add stm32 sdmmc controller driver

[Shawn Lin]

T(0)
+#define IDMACTRLR_IDMABMODEBIT(1)
+#define IDMACTRLR_IDMABACT BIT(2)
+
+#define SDMMC_IDMABSIZER   0x054
+#define IDMABSIZER_IDMABNDT_MASK   GENMASK(12, 5)
+
+#define SDMMC_IDMABASE0R   0x058
+#define SDMMC_IDMABASE1R   0x05c
+
+#define SDMMC_IPVR 0x3fc
+#define IPVR_MINREV_MASK   GENMASK(3, 0)
+#define IPVR_MAJREV_MASK   GENMASK(7, 4)
+
+enum stm32_sdmmc_cookie {
+   COOKIE_UNMAPPED,
+   COOKIE_PRE_MAPPED,  /* mapped by pre_req() of stm32 */
+   COOKIE_MAPPED,  /* mapped by prepare_data() of stm32 */
+};
+
+struct sdmmc_stat {
+   unsigned long   n_req;
+   unsigned long       n_datareq;
+   unsigned long   n_ctimeout;
+   unsigned long   n_ccrcfail;
+   unsigned long   n_dtimeout;
+   unsigned long   n_dcrcfail;
+   unsigned long   n_txunderrun;
+   unsigned long   n_rxoverrun;
+   unsigned long   nb_dma_err;
+};
+
+struct sdmmc_host {
+   void __iomem*base;
+   struct mmc_host *mmc;
+   struct clk  *clk;
+   struct reset_control*rst;
+
+   u32 clk_reg_add;
+   u32 pwr_reg_add;
+
+   struct mmc_request  *mrq;
+   struct mmc_command  *cmd;
+   struct mmc_data *data;
+   struct mmc_command  stop_abort;
+   booldpsm_abort;
+
+   /* protect host registers access */
+   spinlock_t  lock;
+
+   unsigned intsdmmcclk;
+   unsigned intsdmmc_ck;
+
+   u32 size;
+
+   u32 ip_ver;
+   struct sdmmc_stat   stat;
+};




--
Best Regards
Shawn Lin

[PATCH v2 2/2] phy: rockchip-emmc: use regmap_read_poll_timeout to poll dllrdy

[Shawn Lin]

Just use the API instead of open-coding it, no functional change
intended.

Signed-off-by: Shawn Lin <shawn@rock-chips.com>
Reviewed-by: Brian Norris <briannor...@chromium.org>
Tested-by: Caesar Wang <w...@rock-chips.com>
Tested-by: Ziyuan Xu <xzy...@rock-chips.com>
---

Changes in v2:
- propagate the error and print it
- avoid using busy wait

 drivers/phy/rockchip/phy-rockchip-emmc.c | 32 +---
 1 file changed, 13 insertions(+), 19 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c 
b/drivers/phy/rockchip/phy-rockchip-emmc.c
index 547b746..e54e78f 100644
--- a/drivers/phy/rockchip/phy-rockchip-emmc.c
+++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
@@ -79,6 +79,9 @@
 #define PHYCTRL_IS_CALDONE(x) \
x) >> PHYCTRL_CALDONE_SHIFT) & \
  PHYCTRL_CALDONE_MASK) == PHYCTRL_CALDONE_DONE)
+#define PHYCTRL_IS_DLLRDY(x) \
+   x) >> PHYCTRL_DLLRDY_SHIFT) & \
+ PHYCTRL_DLLRDY_MASK) == PHYCTRL_DLLRDY_DONE)
 
 struct rockchip_emmc_phy {
unsigned intreg_offset;
@@ -93,7 +96,6 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool 
on_off)
unsigned int dllrdy;
unsigned int freqsel = PHYCTRL_FREQSEL_200M;
unsigned long rate;
-   unsigned long timeout;
int ret;
 
/*
@@ -217,28 +219,20 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool 
on_off)
 * NOTE: There appear to be corner cases where the DLL seems to take
 * extra long to lock for reasons that aren't understood.  In some
 * extreme cases we've seen it take up to over 10ms (!).  We'll be
-* generous and give it 50ms.  We still busy wait here because:
+* generous and give it 50ms.  We still wait here because:
 * - In most cases it should be super fast.
 * - This is not called lots during normal operation so it shouldn't
-*   be a power or performance problem to busy wait.  We expect it
+*   be a power or performance problem to wait.  We expect it
 *   only at boot / resume.  In both cases, eMMC is probably on the
-*   critical path so busy waiting a little extra time should be OK.
+*   critical path so waiting a little extra time should be OK.
 */
-   timeout = jiffies + msecs_to_jiffies(50);
-   do {
-   udelay(1);
-
-   regmap_read(rk_phy->reg_base,
-   rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
-   );
-   dllrdy = (dllrdy >> PHYCTRL_DLLRDY_SHIFT) & PHYCTRL_DLLRDY_MASK;
-   if (dllrdy == PHYCTRL_DLLRDY_DONE)
-   break;
-   } while (!time_after(jiffies, timeout));
-
-   if (dllrdy != PHYCTRL_DLLRDY_DONE) {
-   pr_err("rockchip_emmc_phy_power: dllrdy timeout.\n");
-   return -ETIMEDOUT;
+   ret = regmap_read_poll_timeout(rk_phy->reg_base,
+  rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
+  dllrdy, PHYCTRL_IS_DLLRDY(dllrdy),
+  1, 50 * USEC_PER_MSEC);
+   if (ret) {
+   pr_err("%s: dllrdy failed %d.\n", __func__, ret);
+   return ret;
}
 
return 0;
-- 
1.9.1

[PATCH v2 2/2] phy: rockchip-emmc: use regmap_read_poll_timeout to poll dllrdy

[Shawn Lin]

Just use the API instead of open-coding it, no functional change
intended.

Signed-off-by: Shawn Lin 
Reviewed-by: Brian Norris 
Tested-by: Caesar Wang 
Tested-by: Ziyuan Xu 
---

Changes in v2:
- propagate the error and print it
- avoid using busy wait

 drivers/phy/rockchip/phy-rockchip-emmc.c | 32 +---
 1 file changed, 13 insertions(+), 19 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c 
b/drivers/phy/rockchip/phy-rockchip-emmc.c
index 547b746..e54e78f 100644
--- a/drivers/phy/rockchip/phy-rockchip-emmc.c
+++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
@@ -79,6 +79,9 @@
 #define PHYCTRL_IS_CALDONE(x) \
x) >> PHYCTRL_CALDONE_SHIFT) & \
  PHYCTRL_CALDONE_MASK) == PHYCTRL_CALDONE_DONE)
+#define PHYCTRL_IS_DLLRDY(x) \
+   x) >> PHYCTRL_DLLRDY_SHIFT) & \
+ PHYCTRL_DLLRDY_MASK) == PHYCTRL_DLLRDY_DONE)
 
 struct rockchip_emmc_phy {
unsigned intreg_offset;
@@ -93,7 +96,6 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool 
on_off)
unsigned int dllrdy;
unsigned int freqsel = PHYCTRL_FREQSEL_200M;
unsigned long rate;
-   unsigned long timeout;
int ret;
 
/*
@@ -217,28 +219,20 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool 
on_off)
 * NOTE: There appear to be corner cases where the DLL seems to take
 * extra long to lock for reasons that aren't understood.  In some
 * extreme cases we've seen it take up to over 10ms (!).  We'll be
-* generous and give it 50ms.  We still busy wait here because:
+* generous and give it 50ms.  We still wait here because:
 * - In most cases it should be super fast.
 * - This is not called lots during normal operation so it shouldn't
-*   be a power or performance problem to busy wait.  We expect it
+*   be a power or performance problem to wait.  We expect it
 *   only at boot / resume.  In both cases, eMMC is probably on the
-*   critical path so busy waiting a little extra time should be OK.
+*   critical path so waiting a little extra time should be OK.
 */
-   timeout = jiffies + msecs_to_jiffies(50);
-   do {
-   udelay(1);
-
-   regmap_read(rk_phy->reg_base,
-   rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
-   );
-   dllrdy = (dllrdy >> PHYCTRL_DLLRDY_SHIFT) & PHYCTRL_DLLRDY_MASK;
-   if (dllrdy == PHYCTRL_DLLRDY_DONE)
-   break;
-   } while (!time_after(jiffies, timeout));
-
-   if (dllrdy != PHYCTRL_DLLRDY_DONE) {
-   pr_err("rockchip_emmc_phy_power: dllrdy timeout.\n");
-   return -ETIMEDOUT;
+   ret = regmap_read_poll_timeout(rk_phy->reg_base,
+  rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
+  dllrdy, PHYCTRL_IS_DLLRDY(dllrdy),
+  1, 50 * USEC_PER_MSEC);
+   if (ret) {
+   pr_err("%s: dllrdy failed %d.\n", __func__, ret);
+   return ret;
}
 
return 0;
-- 
1.9.1

[PATCH v2 1/2] phy: rockchip-emmc: retry calpad busy trimming

[Shawn Lin]

It turns out that 5us isn't enough for all cases, so let's
retry some more times to wait for caldone.

Signed-off-by: Shawn Lin <shawn@rock-chips.com>
Reviewed-by: Brian Norris <briannor...@chromium.org>
Tested-by: Caesar Wang <w...@rock-chips.com>
Tested-by: Ziyuan Xu <xzy...@rock-chips.com>
---

Changes in v2:
- propagate the error and print it

 drivers/phy/rockchip/phy-rockchip-emmc.c | 27 +--
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c 
b/drivers/phy/rockchip/phy-rockchip-emmc.c
index f1b24f1..547b746 100644
--- a/drivers/phy/rockchip/phy-rockchip-emmc.c
+++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
@@ -76,6 +76,10 @@
 #define PHYCTRL_OTAPDLYSEL_MASK0xf
 #define PHYCTRL_OTAPDLYSEL_SHIFT   0x7
 
+#define PHYCTRL_IS_CALDONE(x) \
+   x) >> PHYCTRL_CALDONE_SHIFT) & \
+ PHYCTRL_CALDONE_MASK) == PHYCTRL_CALDONE_DONE)
+
 struct rockchip_emmc_phy {
unsigned intreg_offset;
struct regmap   *reg_base;
@@ -90,6 +94,7 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool 
on_off)
unsigned int freqsel = PHYCTRL_FREQSEL_200M;
unsigned long rate;
unsigned long timeout;
+   int ret;
 
/*
 * Keep phyctrl_pdb and phyctrl_endll low to allow
@@ -160,17 +165,19 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool 
on_off)
   PHYCTRL_PDB_SHIFT));
 
/*
-* According to the user manual, it asks driver to
-* wait 5us for calpad busy trimming
+* According to the user manual, it asks driver to wait 5us for
+* calpad busy trimming. However it is documented that this value is
+* PVT(A.K.A process,voltage and temperature) relevant, so some
+* failure cases are found which indicates we should be more tolerant
+* to calpad busy trimming.
 */
-   udelay(5);
-   regmap_read(rk_phy->reg_base,
-   rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
-   );
-   caldone = (caldone >> PHYCTRL_CALDONE_SHIFT) & PHYCTRL_CALDONE_MASK;
-   if (caldone != PHYCTRL_CALDONE_DONE) {
-   pr_err("rockchip_emmc_phy_power: caldone timeout.\n");
-   return -ETIMEDOUT;
+   ret = regmap_read_poll_timeout(rk_phy->reg_base,
+  rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
+  caldone, PHYCTRL_IS_CALDONE(caldone),
+  5, 50);
+   if (ret) {
+   pr_err("%s: caldone failed %d.\n", __func__, ret);
+   return ret;
}
 
/* Set the frequency of the DLL operation */
-- 
1.9.1

[PATCH v2 1/2] phy: rockchip-emmc: retry calpad busy trimming

[Shawn Lin]

It turns out that 5us isn't enough for all cases, so let's
retry some more times to wait for caldone.

Signed-off-by: Shawn Lin 
Reviewed-by: Brian Norris 
Tested-by: Caesar Wang 
Tested-by: Ziyuan Xu 
---

Changes in v2:
- propagate the error and print it

 drivers/phy/rockchip/phy-rockchip-emmc.c | 27 +--
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c 
b/drivers/phy/rockchip/phy-rockchip-emmc.c
index f1b24f1..547b746 100644
--- a/drivers/phy/rockchip/phy-rockchip-emmc.c
+++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
@@ -76,6 +76,10 @@
 #define PHYCTRL_OTAPDLYSEL_MASK0xf
 #define PHYCTRL_OTAPDLYSEL_SHIFT   0x7
 
+#define PHYCTRL_IS_CALDONE(x) \
+   x) >> PHYCTRL_CALDONE_SHIFT) & \
+ PHYCTRL_CALDONE_MASK) == PHYCTRL_CALDONE_DONE)
+
 struct rockchip_emmc_phy {
unsigned intreg_offset;
struct regmap   *reg_base;
@@ -90,6 +94,7 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool 
on_off)
unsigned int freqsel = PHYCTRL_FREQSEL_200M;
unsigned long rate;
unsigned long timeout;
+   int ret;
 
/*
 * Keep phyctrl_pdb and phyctrl_endll low to allow
@@ -160,17 +165,19 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool 
on_off)
   PHYCTRL_PDB_SHIFT));
 
/*
-* According to the user manual, it asks driver to
-* wait 5us for calpad busy trimming
+* According to the user manual, it asks driver to wait 5us for
+* calpad busy trimming. However it is documented that this value is
+* PVT(A.K.A process,voltage and temperature) relevant, so some
+* failure cases are found which indicates we should be more tolerant
+* to calpad busy trimming.
 */
-   udelay(5);
-   regmap_read(rk_phy->reg_base,
-   rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
-   );
-   caldone = (caldone >> PHYCTRL_CALDONE_SHIFT) & PHYCTRL_CALDONE_MASK;
-   if (caldone != PHYCTRL_CALDONE_DONE) {
-   pr_err("rockchip_emmc_phy_power: caldone timeout.\n");
-   return -ETIMEDOUT;
+   ret = regmap_read_poll_timeout(rk_phy->reg_base,
+  rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
+  caldone, PHYCTRL_IS_CALDONE(caldone),
+  5, 50);
+   if (ret) {
+   pr_err("%s: caldone failed %d.\n", __func__, ret);
+   return ret;
}
 
/* Set the frequency of the DLL operation */
-- 
1.9.1

[PATCH 2/2] phy: rockchip-emmc: use regmap_read_poll_timeout to poll dllrdy

[Shawn Lin]

Just use the API instead of open-coding it, no functional change
intended.

Signed-off-by: Shawn Lin <shawn@rock-chips.com>
---

 drivers/phy/rockchip/phy-rockchip-emmc.c | 21 +++--
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c 
b/drivers/phy/rockchip/phy-rockchip-emmc.c
index 512a6ef..c65979b 100644
--- a/drivers/phy/rockchip/phy-rockchip-emmc.c
+++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
@@ -79,6 +79,9 @@
 #define PHYCTRL_IS_CALDONE(x) \
x) >> PHYCTRL_CALDONE_SHIFT) & \
  PHYCTRL_CALDONE_MASK) == PHYCTRL_CALDONE_DONE)
+#define PHYCTRL_IS_DLLRDY(x) \
+   x) >> PHYCTRL_DLLRDY_SHIFT) & \
+ PHYCTRL_DLLRDY_MASK) == PHYCTRL_DLLRDY_DONE)
 
 struct rockchip_emmc_phy {
unsigned intreg_offset;
@@ -93,7 +96,6 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool 
on_off)
unsigned int dllrdy;
unsigned int freqsel = PHYCTRL_FREQSEL_200M;
unsigned long rate;
-   unsigned long timeout;
 
/*
 * Keep phyctrl_pdb and phyctrl_endll low to allow
@@ -222,19 +224,10 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool 
on_off)
 *   only at boot / resume.  In both cases, eMMC is probably on the
 *   critical path so busy waiting a little extra time should be OK.
 */
-   timeout = jiffies + msecs_to_jiffies(50);
-   do {
-   udelay(1);
-
-   regmap_read(rk_phy->reg_base,
-   rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
-   );
-   dllrdy = (dllrdy >> PHYCTRL_DLLRDY_SHIFT) & PHYCTRL_DLLRDY_MASK;
-   if (dllrdy == PHYCTRL_DLLRDY_DONE)
-   break;
-   } while (!time_after(jiffies, timeout));
-
-   if (dllrdy != PHYCTRL_DLLRDY_DONE) {
+   if (regmap_read_poll_timeout(rk_phy->reg_base,
+rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
+dllrdy, PHYCTRL_IS_DLLRDY(dllrdy),
+1, 50 * USEC_PER_MSEC)) {
pr_err("rockchip_emmc_phy_power: dllrdy timeout.\n");
return -ETIMEDOUT;
}
-- 
1.9.1

[PATCH 2/2] phy: rockchip-emmc: use regmap_read_poll_timeout to poll dllrdy

[Shawn Lin]

Just use the API instead of open-coding it, no functional change
intended.

Signed-off-by: Shawn Lin 
---

 drivers/phy/rockchip/phy-rockchip-emmc.c | 21 +++--
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c 
b/drivers/phy/rockchip/phy-rockchip-emmc.c
index 512a6ef..c65979b 100644
--- a/drivers/phy/rockchip/phy-rockchip-emmc.c
+++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
@@ -79,6 +79,9 @@
 #define PHYCTRL_IS_CALDONE(x) \
x) >> PHYCTRL_CALDONE_SHIFT) & \
  PHYCTRL_CALDONE_MASK) == PHYCTRL_CALDONE_DONE)
+#define PHYCTRL_IS_DLLRDY(x) \
+   x) >> PHYCTRL_DLLRDY_SHIFT) & \
+ PHYCTRL_DLLRDY_MASK) == PHYCTRL_DLLRDY_DONE)
 
 struct rockchip_emmc_phy {
unsigned intreg_offset;
@@ -93,7 +96,6 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool 
on_off)
unsigned int dllrdy;
unsigned int freqsel = PHYCTRL_FREQSEL_200M;
unsigned long rate;
-   unsigned long timeout;
 
/*
 * Keep phyctrl_pdb and phyctrl_endll low to allow
@@ -222,19 +224,10 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool 
on_off)
 *   only at boot / resume.  In both cases, eMMC is probably on the
 *   critical path so busy waiting a little extra time should be OK.
 */
-   timeout = jiffies + msecs_to_jiffies(50);
-   do {
-   udelay(1);
-
-   regmap_read(rk_phy->reg_base,
-   rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
-   );
-   dllrdy = (dllrdy >> PHYCTRL_DLLRDY_SHIFT) & PHYCTRL_DLLRDY_MASK;
-   if (dllrdy == PHYCTRL_DLLRDY_DONE)
-   break;
-   } while (!time_after(jiffies, timeout));
-
-   if (dllrdy != PHYCTRL_DLLRDY_DONE) {
+   if (regmap_read_poll_timeout(rk_phy->reg_base,
+rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
+dllrdy, PHYCTRL_IS_DLLRDY(dllrdy),
+1, 50 * USEC_PER_MSEC)) {
pr_err("rockchip_emmc_phy_power: dllrdy timeout.\n");
return -ETIMEDOUT;
}
-- 
1.9.1

[PATCH 1/2] phy: rockchip-emmc: retry calpad busy trimming

[Shawn Lin]

It turns out that 5us isn't enough for all cases, so let's
retry some more times to wait for caldone.

Signed-off-by: Shawn Lin <shawn@rock-chips.com>
---

 drivers/phy/rockchip/phy-rockchip-emmc.c | 21 +
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c 
b/drivers/phy/rockchip/phy-rockchip-emmc.c
index f1b24f1..512a6ef 100644
--- a/drivers/phy/rockchip/phy-rockchip-emmc.c
+++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
@@ -76,6 +76,10 @@
 #define PHYCTRL_OTAPDLYSEL_MASK0xf
 #define PHYCTRL_OTAPDLYSEL_SHIFT   0x7
 
+#define PHYCTRL_IS_CALDONE(x) \
+   x) >> PHYCTRL_CALDONE_SHIFT) & \
+ PHYCTRL_CALDONE_MASK) == PHYCTRL_CALDONE_DONE)
+
 struct rockchip_emmc_phy {
unsigned intreg_offset;
struct regmap   *reg_base;
@@ -160,15 +164,16 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool 
on_off)
   PHYCTRL_PDB_SHIFT));
 
/*
-* According to the user manual, it asks driver to
-* wait 5us for calpad busy trimming
+* According to the user manual, it asks driver to wait 5us for
+* calpad busy trimming. However it is documented that this value is
+* PVT(A.K.A process,voltage and temperature) relevant, so some
+* failure cases are found which indicates we should be more tolerant
+* to calpad busy trimming.
 */
-   udelay(5);
-   regmap_read(rk_phy->reg_base,
-   rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
-   );
-   caldone = (caldone >> PHYCTRL_CALDONE_SHIFT) & PHYCTRL_CALDONE_MASK;
-   if (caldone != PHYCTRL_CALDONE_DONE) {
+   if (regmap_read_poll_timeout(rk_phy->reg_base,
+rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
+caldone, PHYCTRL_IS_CALDONE(caldone),
+5, 50)) {
pr_err("rockchip_emmc_phy_power: caldone timeout.\n");
return -ETIMEDOUT;
}
-- 
1.9.1

[PATCH 1/2] phy: rockchip-emmc: retry calpad busy trimming

[Shawn Lin]

It turns out that 5us isn't enough for all cases, so let's
retry some more times to wait for caldone.

Signed-off-by: Shawn Lin 
---

 drivers/phy/rockchip/phy-rockchip-emmc.c | 21 +
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c 
b/drivers/phy/rockchip/phy-rockchip-emmc.c
index f1b24f1..512a6ef 100644
--- a/drivers/phy/rockchip/phy-rockchip-emmc.c
+++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
@@ -76,6 +76,10 @@
 #define PHYCTRL_OTAPDLYSEL_MASK0xf
 #define PHYCTRL_OTAPDLYSEL_SHIFT   0x7
 
+#define PHYCTRL_IS_CALDONE(x) \
+   x) >> PHYCTRL_CALDONE_SHIFT) & \
+ PHYCTRL_CALDONE_MASK) == PHYCTRL_CALDONE_DONE)
+
 struct rockchip_emmc_phy {
unsigned intreg_offset;
struct regmap   *reg_base;
@@ -160,15 +164,16 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool 
on_off)
   PHYCTRL_PDB_SHIFT));
 
/*
-* According to the user manual, it asks driver to
-* wait 5us for calpad busy trimming
+* According to the user manual, it asks driver to wait 5us for
+* calpad busy trimming. However it is documented that this value is
+* PVT(A.K.A process,voltage and temperature) relevant, so some
+* failure cases are found which indicates we should be more tolerant
+* to calpad busy trimming.
 */
-   udelay(5);
-   regmap_read(rk_phy->reg_base,
-   rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
-   );
-   caldone = (caldone >> PHYCTRL_CALDONE_SHIFT) & PHYCTRL_CALDONE_MASK;
-   if (caldone != PHYCTRL_CALDONE_DONE) {
+   if (regmap_read_poll_timeout(rk_phy->reg_base,
+rk_phy->reg_offset + GRF_EMMCPHY_STATUS,
+caldone, PHYCTRL_IS_CALDONE(caldone),
+5, 50)) {
pr_err("rockchip_emmc_phy_power: caldone timeout.\n");
return -ETIMEDOUT;
}
-- 
1.9.1

Re: [PATCH] MAINTAINERS: Add Lorenzo Pieralisi for PCI host bridge drivers

[Shawn Lin]

Hi Bjorn,

On 2017/11/9 23:05, Bjorn Helgaas wrote:

On Thu, Nov 09, 2017 at 11:28:36AM +0530, Kishon Vijay Abraham I wrote:

Hi Bjorn,

On Thursday 09 November 2017 01:56 AM, Bjorn Helgaas wrote:

On Wed, Nov 08, 2017 at 02:15:10PM -0600, Bjorn Helgaas wrote:

From: Bjorn Helgaas 

Add Lorenzo Pieralisi as maintainer for PCI native host bridge drivers and
the endpoint driver framework.

Signed-off-by: Bjorn Helgaas 


This is on my for-linus branch, and I intend to merge it for v4.14.


There is already an entry for PCI endpoint in MAINTAINERS file. Can Lorenzo be
added there?

PCI ENDPOINT SUBSYSTEM
M:  Kishon Vijay Abraham I 
L:  linux-...@vger.kernel.org
T:  git 
git://git.kernel.org/pub/scm/linux/kernel/git/kishon/pci-endpoint.git
S:  Supported
F:  drivers/pci/endpoint/
F:  drivers/misc/pci_endpoint_test.c
F:  tools/pci/


Right, thanks, I forgot all about this separate entry.  I added Lorenzo
there, resulting in the patch below.

My practice has been that all the PCI patches (everything in
drivers/pci plus some include and x86/pci stuff) have been merged via
my tree.

This includes things in drivers/pci/{host,dwc,endpoint,switch}, which
are non-core things and usually specific to a chipset.  I try to
ensure they have individual maintainers designated, and I ask for
their acks for non-trivial changes because I have no specs and no
hardware for testing them.  But I think it's still good to have one
person look over them all to try to keep some consistency across them
because they are all quite similar.

So my hope is that Lorenzo can take over that oversight role from me,
not that he would replace any of those designated maintainers.

Ideally, this will be transparent to patch submitters except that they
should add Lorenzo to the "To:" line (keeping linux-pci and other
interested parties).


commit 6b7be529634bbfbf6395f23217a66d731fbed0a0
Author: Bjorn Helgaas 
Date:   Wed Nov 8 08:49:49 2017 -0600

 MAINTAINERS: Add Lorenzo Pieralisi for PCI host bridge drivers
 
 Add Lorenzo Pieralisi as maintainer for PCI native host bridge drivers and

 the endpoint driver framework.
 
 Signed-off-by: Bjorn Helgaas 

 Acked-by: Lorenzo Pieralisi 

diff --git a/MAINTAINERS b/MAINTAINERS
index db412a627d96..6ce341e86fec 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10378,6 +10378,7 @@ F:  drivers/pci/dwc/*keystone*
  
  PCI ENDPOINT SUBSYSTEM

  M:Kishon Vijay Abraham I 
+M: Lorenzo Pieralisi 
  L:linux-...@vger.kernel.org
  T:git 
git://git.kernel.org/pub/scm/linux/kernel/git/kishon/pci-endpoint.git
  S:Supported
@@ -10429,6 +10430,15 @@ F: include/linux/pci*
  F:arch/x86/pci/
  F:arch/x86/kernel/quirks.c
  
+PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS

+M: Lorenzo Pieralisi 
+L: linux-...@vger.kernel.org
+Q: http://patchwork.ozlabs.org/project/linux-pci/list/
+T: git git://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/


So, does that mean the patch(es) for host drivers shoube be based on
this tree instead of yours? If yes, which tree should be preferred if
a patchset wanna touch both of pci core and host drivers?


+S: Supported
+F: drivers/pci/host/
+F: drivers/pci/dwc/
+
  PCIE DRIVER FOR AXIS ARTPEC
  M:Niklas Cassel 
  M:Jesper Nilsson 

Re: [PATCH] MAINTAINERS: Add Lorenzo Pieralisi for PCI host bridge drivers

[Shawn Lin]

Hi Bjorn,

On 2017/11/9 23:05, Bjorn Helgaas wrote:

On Thu, Nov 09, 2017 at 11:28:36AM +0530, Kishon Vijay Abraham I wrote:

Hi Bjorn,

On Thursday 09 November 2017 01:56 AM, Bjorn Helgaas wrote:

On Wed, Nov 08, 2017 at 02:15:10PM -0600, Bjorn Helgaas wrote:

From: Bjorn Helgaas 

Add Lorenzo Pieralisi as maintainer for PCI native host bridge drivers and
the endpoint driver framework.

Signed-off-by: Bjorn Helgaas 


This is on my for-linus branch, and I intend to merge it for v4.14.


There is already an entry for PCI endpoint in MAINTAINERS file. Can Lorenzo be
added there?

PCI ENDPOINT SUBSYSTEM
M:  Kishon Vijay Abraham I 
L:  linux-...@vger.kernel.org
T:  git 
git://git.kernel.org/pub/scm/linux/kernel/git/kishon/pci-endpoint.git
S:  Supported
F:  drivers/pci/endpoint/
F:  drivers/misc/pci_endpoint_test.c
F:  tools/pci/


Right, thanks, I forgot all about this separate entry.  I added Lorenzo
there, resulting in the patch below.

My practice has been that all the PCI patches (everything in
drivers/pci plus some include and x86/pci stuff) have been merged via
my tree.

This includes things in drivers/pci/{host,dwc,endpoint,switch}, which
are non-core things and usually specific to a chipset.  I try to
ensure they have individual maintainers designated, and I ask for
their acks for non-trivial changes because I have no specs and no
hardware for testing them.  But I think it's still good to have one
person look over them all to try to keep some consistency across them
because they are all quite similar.

So my hope is that Lorenzo can take over that oversight role from me,
not that he would replace any of those designated maintainers.

Ideally, this will be transparent to patch submitters except that they
should add Lorenzo to the "To:" line (keeping linux-pci and other
interested parties).


commit 6b7be529634bbfbf6395f23217a66d731fbed0a0
Author: Bjorn Helgaas 
Date:   Wed Nov 8 08:49:49 2017 -0600

 MAINTAINERS: Add Lorenzo Pieralisi for PCI host bridge drivers
 
 Add Lorenzo Pieralisi as maintainer for PCI native host bridge drivers and

 the endpoint driver framework.
 
 Signed-off-by: Bjorn Helgaas 

 Acked-by: Lorenzo Pieralisi 

diff --git a/MAINTAINERS b/MAINTAINERS
index db412a627d96..6ce341e86fec 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10378,6 +10378,7 @@ F:  drivers/pci/dwc/*keystone*
  
  PCI ENDPOINT SUBSYSTEM

  M:Kishon Vijay Abraham I 
+M: Lorenzo Pieralisi 
  L:linux-...@vger.kernel.org
  T:git 
git://git.kernel.org/pub/scm/linux/kernel/git/kishon/pci-endpoint.git
  S:Supported
@@ -10429,6 +10430,15 @@ F: include/linux/pci*
  F:arch/x86/pci/
  F:arch/x86/kernel/quirks.c
  
+PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS

+M: Lorenzo Pieralisi 
+L: linux-...@vger.kernel.org
+Q: http://patchwork.ozlabs.org/project/linux-pci/list/
+T: git git://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/


So, does that mean the patch(es) for host drivers shoube be based on
this tree instead of yours? If yes, which tree should be preferred if
a patchset wanna touch both of pci core and host drivers?


+S: Supported
+F: drivers/pci/host/
+F: drivers/pci/dwc/
+
  PCIE DRIVER FOR AXIS ARTPEC
  M:Niklas Cassel 
  M:Jesper Nilsson 

Re: [PATCH v2 0/5] mmc: dw_mmc: Fix the CTO timer patch, plus the DTO timer

[Shawn Lin]

Hi Ulf,

On 2017/10/30 19:40, Ulf Hansson wrote:

On 12 October 2017 at 22:11, Douglas Anderson  wrote:

Recently we landed 03de19212ea3 ("mmc: dw_mmc: introduce timer for
broken command transfer over scheme").  I found a bunch of problems
with that patch, so this series attempts to solve some of them.

This also fixes the DTO timer in some of the same ways even though I
haven't personally seen problems with the DTO timer.

NOTE: this series has only been lighly tested so far.  I can at least
reproduce the need for the CTO timer on one of my devices and so I can
confirm that part still works.  As mentioned in the 3rd patch I also
ran the mmc_test kernel module on this and did manage to see the 3rd
patch doing something useful.

Changes in v2:
- Removed extra "int i"
- Fix the DTO timeout calculation new for v2
- Cleanup the DTO timer new for v2

Douglas Anderson (5):
   mmc: dw_mmc: cancel the CTO timer after a voltage switch
   mmc: dw_mmc: Fix the CTO timeout calculation
   mmc: dw_mmc: Add locking to the CTO timer
   mmc: dw_mmc: Fix the DTO timeout calculation
   mmc: dw_mmc: Cleanup the DTO timer like the CTO one

  drivers/mmc/host/dw_mmc.c | 162 +-
  1 file changed, 146 insertions(+), 16 deletions(-)



Douglas, Jaehoon,

I decided to pick patch 1->4 for fixes and the patch 5 for next, that
should help us to get them more tested, while Jaehoon is still
catching up.

I can add ack/drop patches for yet a couple of days this week.


Patch 4 introduce a warning:

warning: unused variable ‘irqflags’ [-Wunused-variable]

irqflags should be introduced in patch 5 in the same place.
As it seems patch 5 will be candidate for 4.15, so could you please
help fix patch 4 and 5 manually? Or Doug need to resend patch 4 and 5?





Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/5] mmc: dw_mmc: Fix the CTO timer patch, plus the DTO timer

[Shawn Lin]

Hi Ulf,

On 2017/10/30 19:40, Ulf Hansson wrote:

On 12 October 2017 at 22:11, Douglas Anderson  wrote:

Recently we landed 03de19212ea3 ("mmc: dw_mmc: introduce timer for
broken command transfer over scheme").  I found a bunch of problems
with that patch, so this series attempts to solve some of them.

This also fixes the DTO timer in some of the same ways even though I
haven't personally seen problems with the DTO timer.

NOTE: this series has only been lighly tested so far.  I can at least
reproduce the need for the CTO timer on one of my devices and so I can
confirm that part still works.  As mentioned in the 3rd patch I also
ran the mmc_test kernel module on this and did manage to see the 3rd
patch doing something useful.

Changes in v2:
- Removed extra "int i"
- Fix the DTO timeout calculation new for v2
- Cleanup the DTO timer new for v2

Douglas Anderson (5):
   mmc: dw_mmc: cancel the CTO timer after a voltage switch
   mmc: dw_mmc: Fix the CTO timeout calculation
   mmc: dw_mmc: Add locking to the CTO timer
   mmc: dw_mmc: Fix the DTO timeout calculation
   mmc: dw_mmc: Cleanup the DTO timer like the CTO one

  drivers/mmc/host/dw_mmc.c | 162 +-
  1 file changed, 146 insertions(+), 16 deletions(-)



Douglas, Jaehoon,

I decided to pick patch 1->4 for fixes and the patch 5 for next, that
should help us to get them more tested, while Jaehoon is still
catching up.

I can add ack/drop patches for yet a couple of days this week.


Patch 4 introduce a warning:

warning: unused variable ‘irqflags’ [-Wunused-variable]

irqflags should be introduced in patch 5 in the same place.
As it seems patch 5 will be candidate for 4.15, so could you please
help fix patch 4 and 5 manually? Or Doug need to resend patch 4 and 5?





Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/5] mmc: dw_mmc: Cleanup the DTO timer like the CTO one

[Shawn Lin]

On 2017/10/17 13:05, Doug Anderson wrote:

Hi,

On Mon, Oct 16, 2017 at 6:17 PM, Shawn Lin <shawn@rock-chips.com> wrote:

Hi Doug


On 2017/10/13 4:11, Douglas Anderson wrote:


The recent CTO timer introduced in commit 03de19212ea3 ("mmc: dw_mmc:
introduce timer for broken command transfer over scheme") was causing
observable problems due to race conditions.  Previous patches have
fixed those race conditions.

It can be observed that these same race conditions ought to be
theoretically possible with the DTO timer too though they are
massively less likely to happen because the data timeout is always set
to 0xff right now.  That means even at a 200 MHz card clock we
were arming the DTO timer for 94 ms:
>>> (0xff * 1000. / 2) + 10
93.886075

We always also were setting the DTO timer _after_ starting the
transfer, unlike how the old code was seting the CTO timer.

In any case, even though the DTO timer is much less likely to have
races, it still makes sense to add code to handle it _just in case_.

Signed-off-by: Douglas Anderson <diand...@chromium.org>
---

Changes in v2:
- Cleanup the DTO timer new for v2

   drivers/mmc/host/dw_mmc.c | 54
---
   1 file changed, 51 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 6bc87b1385a9..bc0808615431 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1950,7 +1950,11 @@ static void dw_mci_set_drto(struct dw_mci *host)
 /* add a bit spare time */
 drto_ms += 10;
   - mod_timer(>dto_timer, jiffies + msecs_to_jiffies(drto_ms));
+   spin_lock_irqsave(>irq_lock, irqflags);
+   if (!test_bit(EVENT_DATA_COMPLETE, >pending_events))
+   mod_timer(>dto_timer,
+ jiffies + msecs_to_jiffies(drto_ms));
+   spin_unlock_irqrestore(>irq_lock, irqflags);
   }
 static bool dw_mci_clear_pending_cmd_complete(struct dw_mci *host)
@@ -1971,6 +1975,18 @@ static bool
dw_mci_clear_pending_cmd_complete(struct dw_mci *host)
 return true;
   }
   +static bool dw_mci_clear_pending_data_complete(struct dw_mci *host)
+{
+   if (!test_bit(EVENT_DATA_COMPLETE, >pending_events))
+   return false;
+
+   /* Extra paranoia just like dw_mci_clear_pending_cmd_complete() */
+   WARN_ON(del_timer_sync(>dto_timer));
+   clear_bit(EVENT_DATA_COMPLETE, >pending_events);
+
+   return true;
+}
+
   static void dw_mci_tasklet_func(unsigned long priv)
   {
 struct dw_mci *host = (struct dw_mci *)priv;
@@ -2112,8 +2128,7 @@ static void dw_mci_tasklet_func(unsigned long priv)
 /* fall through */
 case STATE_DATA_BUSY:
-   if (!test_and_clear_bit(EVENT_DATA_COMPLETE,
-   >pending_events)) {
+   if (!dw_mci_clear_pending_data_complete(host)) {
 /*
  * If data error interrupt comes but data
over
  * interrupt doesn't come within the given
time.
@@ -2683,6 +2698,8 @@ static irqreturn_t dw_mci_interrupt(int irq, void
*dev_id)
 }
 if (pending & SDMMC_INT_DATA_OVER) {
+   spin_lock_irqsave(>irq_lock, irqflags);
+
 del_timer(>dto_timer);
 mci_writel(host, RINTSTS, SDMMC_INT_DATA_OVER);
@@ -2695,6 +2712,8 @@ static irqreturn_t dw_mci_interrupt(int irq, void
*dev_id)
 }
 set_bit(EVENT_DATA_COMPLETE,
>pending_events);
 tasklet_schedule(>tasklet);
+
+   spin_unlock_irqrestore(>irq_lock, irqflags);
 }
 if (pending & SDMMC_INT_RXDR) {
@@ -3044,7 +3063,31 @@ static void dw_mci_cto_timer(unsigned long arg)
   static void dw_mci_dto_timer(unsigned long arg)
   {
 struct dw_mci *host = (struct dw_mci *)arg;
+   unsigned long irqflags;
+   u32 pending;
+
+   spin_lock_irqsave(>irq_lock, irqflags);
   + /*
+* The DTO timer is much longer than the CTO timer, so it's even
less
+* likely that we'll these cases, but it pays to be paranoid.
+*/
+   pending = mci_readl(host, MINTSTS); /* read-only mask reg */
+   if (pending & SDMMC_INT_DATA_OVER) {
+   /* The interrupt should fire; no need to act but we can
warn */
+   dev_warn(host->dev, "Unexpected data interrupt
latency\n");
+   goto exit;



I was checking a problem like this:

(1) Start a CTO timer
(2) Start a command
(3) Got CMD_DONE interrupt and cancel the CTO timer
(4) Start a DRTO timer
(5) Start external dma to get the data from fifo
(6) The system bus/DRAM port is idle for a very long t

Re: [PATCH v2 5/5] mmc: dw_mmc: Cleanup the DTO timer like the CTO one

[Shawn Lin]

On 2017/10/17 13:05, Doug Anderson wrote:

Hi,

On Mon, Oct 16, 2017 at 6:17 PM, Shawn Lin  wrote:

Hi Doug


On 2017/10/13 4:11, Douglas Anderson wrote:


The recent CTO timer introduced in commit 03de19212ea3 ("mmc: dw_mmc:
introduce timer for broken command transfer over scheme") was causing
observable problems due to race conditions.  Previous patches have
fixed those race conditions.

It can be observed that these same race conditions ought to be
theoretically possible with the DTO timer too though they are
massively less likely to happen because the data timeout is always set
to 0xff right now.  That means even at a 200 MHz card clock we
were arming the DTO timer for 94 ms:
>>> (0xff * 1000. / 2) + 10
93.886075

We always also were setting the DTO timer _after_ starting the
transfer, unlike how the old code was seting the CTO timer.

In any case, even though the DTO timer is much less likely to have
races, it still makes sense to add code to handle it _just in case_.

Signed-off-by: Douglas Anderson 
---

Changes in v2:
- Cleanup the DTO timer new for v2

   drivers/mmc/host/dw_mmc.c | 54
---
   1 file changed, 51 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 6bc87b1385a9..bc0808615431 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1950,7 +1950,11 @@ static void dw_mci_set_drto(struct dw_mci *host)
 /* add a bit spare time */
 drto_ms += 10;
   - mod_timer(>dto_timer, jiffies + msecs_to_jiffies(drto_ms));
+   spin_lock_irqsave(>irq_lock, irqflags);
+   if (!test_bit(EVENT_DATA_COMPLETE, >pending_events))
+   mod_timer(>dto_timer,
+ jiffies + msecs_to_jiffies(drto_ms));
+   spin_unlock_irqrestore(>irq_lock, irqflags);
   }
 static bool dw_mci_clear_pending_cmd_complete(struct dw_mci *host)
@@ -1971,6 +1975,18 @@ static bool
dw_mci_clear_pending_cmd_complete(struct dw_mci *host)
 return true;
   }
   +static bool dw_mci_clear_pending_data_complete(struct dw_mci *host)
+{
+   if (!test_bit(EVENT_DATA_COMPLETE, >pending_events))
+   return false;
+
+   /* Extra paranoia just like dw_mci_clear_pending_cmd_complete() */
+   WARN_ON(del_timer_sync(>dto_timer));
+   clear_bit(EVENT_DATA_COMPLETE, >pending_events);
+
+   return true;
+}
+
   static void dw_mci_tasklet_func(unsigned long priv)
   {
 struct dw_mci *host = (struct dw_mci *)priv;
@@ -2112,8 +2128,7 @@ static void dw_mci_tasklet_func(unsigned long priv)
 /* fall through */
 case STATE_DATA_BUSY:
-   if (!test_and_clear_bit(EVENT_DATA_COMPLETE,
-   >pending_events)) {
+   if (!dw_mci_clear_pending_data_complete(host)) {
 /*
  * If data error interrupt comes but data
over
  * interrupt doesn't come within the given
time.
@@ -2683,6 +2698,8 @@ static irqreturn_t dw_mci_interrupt(int irq, void
*dev_id)
 }
 if (pending & SDMMC_INT_DATA_OVER) {
+   spin_lock_irqsave(>irq_lock, irqflags);
+
 del_timer(>dto_timer);
 mci_writel(host, RINTSTS, SDMMC_INT_DATA_OVER);
@@ -2695,6 +2712,8 @@ static irqreturn_t dw_mci_interrupt(int irq, void
*dev_id)
 }
 set_bit(EVENT_DATA_COMPLETE,
>pending_events);
 tasklet_schedule(>tasklet);
+
+   spin_unlock_irqrestore(>irq_lock, irqflags);
 }
 if (pending & SDMMC_INT_RXDR) {
@@ -3044,7 +3063,31 @@ static void dw_mci_cto_timer(unsigned long arg)
   static void dw_mci_dto_timer(unsigned long arg)
   {
 struct dw_mci *host = (struct dw_mci *)arg;
+   unsigned long irqflags;
+   u32 pending;
+
+   spin_lock_irqsave(>irq_lock, irqflags);
   + /*
+* The DTO timer is much longer than the CTO timer, so it's even
less
+* likely that we'll these cases, but it pays to be paranoid.
+*/
+   pending = mci_readl(host, MINTSTS); /* read-only mask reg */
+   if (pending & SDMMC_INT_DATA_OVER) {
+   /* The interrupt should fire; no need to act but we can
warn */
+   dev_warn(host->dev, "Unexpected data interrupt
latency\n");
+   goto exit;



I was checking a problem like this:

(1) Start a CTO timer
(2) Start a command
(3) Got CMD_DONE interrupt and cancel the CTO timer
(4) Start a DRTO timer
(5) Start external dma to get the data from fifo
(6) The system bus/DRAM port is idle for a very long time for no
matter what happen.
(7) DRTO timer fires but DT

Re: [PATCH 0/9] Enable dw-mmc multi-card support

[Shawn Lin]

On 2017/10/7 3:21, Liming Sun wrote:

This series of commits enables the multi-card support for the dw-mmc
controller. It includes two parts as below.

The first part (patches 1-7) reverts the series of recent commits that
removed the multi-card support with comments saying there was no such
use case in the real world. Actually this feature is being used in
Mellanox Bluefield SoC and has been requested by customers.


Hrm it's so unlucky that your patchset comes a little late. As
your patch 8 and 9 said, you need them to fix problem for multi-card
support, so definitely there was no such use case, and even the code
was buggy to support it right? That makes the code hard to read and
maintain, so we decide to remove it.



The second part (patches 8-9) fixes the DesignWare multi-card support
according to the dw-mmc databook (synnopsys: DesignWare Cores Mobile
Storage Host Databook, 2.70a). It has changes to set the card number
into the CMD register to multiplex requests to different cards when
working in SD_MMC_CEATA mode, set the CTYPE / CLKENA / CDTHRCTL
registers properly according to the spec, and parse the per-card
configuration to match the Linux Documentation
(bindings/mmc/synopsys-dw-mshc.txt).


Havn'e check the databook for details yet, but I think it's ok
to re-introduce multi-slot support if a real user benefits from
it. But you need a new patch to silent the log "num-slots property not
found, assuming 1 slot is available" as we removed all the num-slots
from DT at that time.




Liming Sun (9):
   Revert "Documentation: dw-mshc: deprecate num-slots"
   Revert "mmc: dw_mmc: remove the unnecessary slot variable"
   Revert "mmc: dw_mmc: use the 'slot' instead of 'cur_slot'"
   Revert "mmc: dw_mmc: remove the 'id' arguments about functions
 relevant to slot"
   Revert "mmc: dw_mmc: change the array of slots"
   Revert "mmc: dw_mmc: remove the loop about finding slots"
   Revert "mmc: dw_mmc: deprecated the "num-slots" property"
   mmc: dw_mmc: Support two SD_MMC_CE-ATA cards
   mmc: dw_mmc: Parse slot-specific configuration

  .../devicetree/bindings/mmc/synopsys-dw-mshc.txt   |  16 +-
  drivers/mmc/host/dw_mmc-exynos.c   |   4 +-
  drivers/mmc/host/dw_mmc.c  | 277 -
  drivers/mmc/host/dw_mmc.h  |  17 +-
  4 files changed, 236 insertions(+), 78 deletions(-)

Re: [PATCH 0/9] Enable dw-mmc multi-card support

[Shawn Lin]

On 2017/10/7 3:21, Liming Sun wrote:

This series of commits enables the multi-card support for the dw-mmc
controller. It includes two parts as below.

The first part (patches 1-7) reverts the series of recent commits that
removed the multi-card support with comments saying there was no such
use case in the real world. Actually this feature is being used in
Mellanox Bluefield SoC and has been requested by customers.


Hrm it's so unlucky that your patchset comes a little late. As
your patch 8 and 9 said, you need them to fix problem for multi-card
support, so definitely there was no such use case, and even the code
was buggy to support it right? That makes the code hard to read and
maintain, so we decide to remove it.



The second part (patches 8-9) fixes the DesignWare multi-card support
according to the dw-mmc databook (synnopsys: DesignWare Cores Mobile
Storage Host Databook, 2.70a). It has changes to set the card number
into the CMD register to multiplex requests to different cards when
working in SD_MMC_CEATA mode, set the CTYPE / CLKENA / CDTHRCTL
registers properly according to the spec, and parse the per-card
configuration to match the Linux Documentation
(bindings/mmc/synopsys-dw-mshc.txt).


Havn'e check the databook for details yet, but I think it's ok
to re-introduce multi-slot support if a real user benefits from
it. But you need a new patch to silent the log "num-slots property not
found, assuming 1 slot is available" as we removed all the num-slots
from DT at that time.




Liming Sun (9):
   Revert "Documentation: dw-mshc: deprecate num-slots"
   Revert "mmc: dw_mmc: remove the unnecessary slot variable"
   Revert "mmc: dw_mmc: use the 'slot' instead of 'cur_slot'"
   Revert "mmc: dw_mmc: remove the 'id' arguments about functions
 relevant to slot"
   Revert "mmc: dw_mmc: change the array of slots"
   Revert "mmc: dw_mmc: remove the loop about finding slots"
   Revert "mmc: dw_mmc: deprecated the "num-slots" property"
   mmc: dw_mmc: Support two SD_MMC_CE-ATA cards
   mmc: dw_mmc: Parse slot-specific configuration

  .../devicetree/bindings/mmc/synopsys-dw-mshc.txt   |  16 +-
  drivers/mmc/host/dw_mmc-exynos.c   |   4 +-
  drivers/mmc/host/dw_mmc.c  | 277 -
  drivers/mmc/host/dw_mmc.h  |  17 +-
  4 files changed, 236 insertions(+), 78 deletions(-)

Re: [PATCH v2 5/5] mmc: dw_mmc: Cleanup the DTO timer like the CTO one

[Shawn Lin]

Hi Doug

On 2017/10/13 4:11, Douglas Anderson wrote:

The recent CTO timer introduced in commit 03de19212ea3 ("mmc: dw_mmc:
introduce timer for broken command transfer over scheme") was causing
observable problems due to race conditions.  Previous patches have
fixed those race conditions.

It can be observed that these same race conditions ought to be
theoretically possible with the DTO timer too though they are
massively less likely to happen because the data timeout is always set
to 0xff right now.  That means even at a 200 MHz card clock we
were arming the DTO timer for 94 ms:
   >>> (0xff * 1000. / 2) + 10
   93.886075

We always also were setting the DTO timer _after_ starting the
transfer, unlike how the old code was seting the CTO timer.

In any case, even though the DTO timer is much less likely to have
races, it still makes sense to add code to handle it _just in case_.

Signed-off-by: Douglas Anderson 
---

Changes in v2:
- Cleanup the DTO timer new for v2

  drivers/mmc/host/dw_mmc.c | 54 ---
  1 file changed, 51 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 6bc87b1385a9..bc0808615431 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1950,7 +1950,11 @@ static void dw_mci_set_drto(struct dw_mci *host)
/* add a bit spare time */
drto_ms += 10;
  
-	mod_timer(>dto_timer, jiffies + msecs_to_jiffies(drto_ms));

+   spin_lock_irqsave(>irq_lock, irqflags);
+   if (!test_bit(EVENT_DATA_COMPLETE, >pending_events))
+   mod_timer(>dto_timer,
+ jiffies + msecs_to_jiffies(drto_ms));
+   spin_unlock_irqrestore(>irq_lock, irqflags);
  }
  
  static bool dw_mci_clear_pending_cmd_complete(struct dw_mci *host)

@@ -1971,6 +1975,18 @@ static bool dw_mci_clear_pending_cmd_complete(struct 
dw_mci *host)
return true;
  }
  
+static bool dw_mci_clear_pending_data_complete(struct dw_mci *host)

+{
+   if (!test_bit(EVENT_DATA_COMPLETE, >pending_events))
+   return false;
+
+   /* Extra paranoia just like dw_mci_clear_pending_cmd_complete() */
+   WARN_ON(del_timer_sync(>dto_timer));
+   clear_bit(EVENT_DATA_COMPLETE, >pending_events);
+
+   return true;
+}
+
  static void dw_mci_tasklet_func(unsigned long priv)
  {
struct dw_mci *host = (struct dw_mci *)priv;
@@ -2112,8 +2128,7 @@ static void dw_mci_tasklet_func(unsigned long priv)
/* fall through */
  
  		case STATE_DATA_BUSY:

-   if (!test_and_clear_bit(EVENT_DATA_COMPLETE,
-   >pending_events)) {
+   if (!dw_mci_clear_pending_data_complete(host)) {
/*
 * If data error interrupt comes but data over
 * interrupt doesn't come within the given time.
@@ -2683,6 +2698,8 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
}
  
  		if (pending & SDMMC_INT_DATA_OVER) {

+   spin_lock_irqsave(>irq_lock, irqflags);
+
del_timer(>dto_timer);
  
  			mci_writel(host, RINTSTS, SDMMC_INT_DATA_OVER);

@@ -2695,6 +2712,8 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
}
set_bit(EVENT_DATA_COMPLETE, >pending_events);
tasklet_schedule(>tasklet);
+
+   spin_unlock_irqrestore(>irq_lock, irqflags);
}
  
  		if (pending & SDMMC_INT_RXDR) {

@@ -3044,7 +3063,31 @@ static void dw_mci_cto_timer(unsigned long arg)
  static void dw_mci_dto_timer(unsigned long arg)
  {
struct dw_mci *host = (struct dw_mci *)arg;
+   unsigned long irqflags;
+   u32 pending;
+
+   spin_lock_irqsave(>irq_lock, irqflags);
  
+	/*

+* The DTO timer is much longer than the CTO timer, so it's even less
+* likely that we'll these cases, but it pays to be paranoid.
+*/
+   pending = mci_readl(host, MINTSTS); /* read-only mask reg */
+   if (pending & SDMMC_INT_DATA_OVER) {
+   /* The interrupt should fire; no need to act but we can warn */
+   dev_warn(host->dev, "Unexpected data interrupt latency\n");
+   goto exit;


I was checking a problem like this:

(1) Start a CTO timer
(2) Start a command
(3) Got CMD_DONE interrupt and cancel the CTO timer
(4) Start a DRTO timer
(5) Start external dma to get the data from fifo
(6) The system bus/DRAM port is idle for a very long time for no
matter what happen.
(7) DRTO timer fires but DTO was set as the card have already
sent all data to the fifo.
(8) Now you patch bails out earlier  and notify the mmc core that this
data transfer was finished successfully.
(9) mmc core propgate the successful state to block layer and maybe
a 

Re: [PATCH v2 5/5] mmc: dw_mmc: Cleanup the DTO timer like the CTO one

[Shawn Lin]

Hi Doug

On 2017/10/13 4:11, Douglas Anderson wrote:

The recent CTO timer introduced in commit 03de19212ea3 ("mmc: dw_mmc:
introduce timer for broken command transfer over scheme") was causing
observable problems due to race conditions.  Previous patches have
fixed those race conditions.

It can be observed that these same race conditions ought to be
theoretically possible with the DTO timer too though they are
massively less likely to happen because the data timeout is always set
to 0xff right now.  That means even at a 200 MHz card clock we
were arming the DTO timer for 94 ms:
   >>> (0xff * 1000. / 2) + 10
   93.886075

We always also were setting the DTO timer _after_ starting the
transfer, unlike how the old code was seting the CTO timer.

In any case, even though the DTO timer is much less likely to have
races, it still makes sense to add code to handle it _just in case_.

Signed-off-by: Douglas Anderson 
---

Changes in v2:
- Cleanup the DTO timer new for v2

  drivers/mmc/host/dw_mmc.c | 54 ---
  1 file changed, 51 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 6bc87b1385a9..bc0808615431 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1950,7 +1950,11 @@ static void dw_mci_set_drto(struct dw_mci *host)
/* add a bit spare time */
drto_ms += 10;
  
-	mod_timer(>dto_timer, jiffies + msecs_to_jiffies(drto_ms));

+   spin_lock_irqsave(>irq_lock, irqflags);
+   if (!test_bit(EVENT_DATA_COMPLETE, >pending_events))
+   mod_timer(>dto_timer,
+ jiffies + msecs_to_jiffies(drto_ms));
+   spin_unlock_irqrestore(>irq_lock, irqflags);
  }
  
  static bool dw_mci_clear_pending_cmd_complete(struct dw_mci *host)

@@ -1971,6 +1975,18 @@ static bool dw_mci_clear_pending_cmd_complete(struct 
dw_mci *host)
return true;
  }
  
+static bool dw_mci_clear_pending_data_complete(struct dw_mci *host)

+{
+   if (!test_bit(EVENT_DATA_COMPLETE, >pending_events))
+   return false;
+
+   /* Extra paranoia just like dw_mci_clear_pending_cmd_complete() */
+   WARN_ON(del_timer_sync(>dto_timer));
+   clear_bit(EVENT_DATA_COMPLETE, >pending_events);
+
+   return true;
+}
+
  static void dw_mci_tasklet_func(unsigned long priv)
  {
struct dw_mci *host = (struct dw_mci *)priv;
@@ -2112,8 +2128,7 @@ static void dw_mci_tasklet_func(unsigned long priv)
/* fall through */
  
  		case STATE_DATA_BUSY:

-   if (!test_and_clear_bit(EVENT_DATA_COMPLETE,
-   >pending_events)) {
+   if (!dw_mci_clear_pending_data_complete(host)) {
/*
 * If data error interrupt comes but data over
 * interrupt doesn't come within the given time.
@@ -2683,6 +2698,8 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
}
  
  		if (pending & SDMMC_INT_DATA_OVER) {

+   spin_lock_irqsave(>irq_lock, irqflags);
+
del_timer(>dto_timer);
  
  			mci_writel(host, RINTSTS, SDMMC_INT_DATA_OVER);

@@ -2695,6 +2712,8 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
}
set_bit(EVENT_DATA_COMPLETE, >pending_events);
tasklet_schedule(>tasklet);
+
+   spin_unlock_irqrestore(>irq_lock, irqflags);
}
  
  		if (pending & SDMMC_INT_RXDR) {

@@ -3044,7 +3063,31 @@ static void dw_mci_cto_timer(unsigned long arg)
  static void dw_mci_dto_timer(unsigned long arg)
  {
struct dw_mci *host = (struct dw_mci *)arg;
+   unsigned long irqflags;
+   u32 pending;
+
+   spin_lock_irqsave(>irq_lock, irqflags);
  
+	/*

+* The DTO timer is much longer than the CTO timer, so it's even less
+* likely that we'll these cases, but it pays to be paranoid.
+*/
+   pending = mci_readl(host, MINTSTS); /* read-only mask reg */
+   if (pending & SDMMC_INT_DATA_OVER) {
+   /* The interrupt should fire; no need to act but we can warn */
+   dev_warn(host->dev, "Unexpected data interrupt latency\n");
+   goto exit;


I was checking a problem like this:

(1) Start a CTO timer
(2) Start a command
(3) Got CMD_DONE interrupt and cancel the CTO timer
(4) Start a DRTO timer
(5) Start external dma to get the data from fifo
(6) The system bus/DRAM port is idle for a very long time for no
matter what happen.
(7) DRTO timer fires but DTO was set as the card have already
sent all data to the fifo.
(8) Now you patch bails out earlier  and notify the mmc core that this
data transfer was finished successfully.
(9) mmc core propgate the successful state to block layer and maybe
a critical reader in file 

Re: [PATCH v2 3/5] mmc: dw_mmc: Add locking to the CTO timer

[Shawn Lin]

Hi Doug

On 2017/10/13 12:20, Doug Anderson wrote:

Shawn,

On Thu, Oct 12, 2017 at 6:32 PM, Shawn Lin <shawn@rock-chips.com> wrote:


On 2017/10/13 4:11, Douglas Anderson wrote:


This attempts to instill a bit of paranoia to the code dealing with
the CTO timer.  It's believed that this will make the CTO timer more
robust in the case that we're having very long interrupt latencies.



Ack. It could help fix some problems observed.



Note that I originally thought that perhaps this patch was being
overly paranoid and wasn't really needed, but then while I was running
mmc_test on an rk3399 board I saw one instance of the message:
dwmmc_rockchip fe32.dwmmc: Unexpected interrupt latency

I had debug prints in the CTO timer code and I found that it was
running CMD 13 at the time.

...so even though this patch seems like it might be overly paranoid,
maybe it really isn't?

Presumably the bad interrupt latency experienced was due to the fact
that I had serial console enabled as serial console is typically where
I place blame when I see absurdly large interrupt latencies.  In this
particular case there was an (unrelated) printout to the serial
console just before I saw the "Unexpected interrupt latency" printout.

...and actually, I managed to even reproduce the problems by running
"iw mlan0 scan > /dev/null" while mmc_test was running.  That not only
does a bunch of PCIe traffic but it also (on my system) outputs some
SELinux log spam.

Fixes: 03de19212ea3 ("mmc: dw_mmc: introduce timer for broken command


transfer over scheme")


Tested-by: Emil Renner Berthing <ker...@esmil.dk>
Signed-off-by: Douglas Anderson <diand...@chromium.org>
---

Changes in v2:
- Removed extra "int i"

   drivers/mmc/host/dw_mmc.c | 91
+--
   1 file changed, 81 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 16516c528a88..50148991f30e 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -403,6 +403,7 @@ static inline void dw_mci_set_cto(struct dw_mci *host)
 unsigned int cto_clks;
 unsigned int cto_div;
 unsigned int cto_ms;
+   unsigned long irqflags;
 cto_clks = mci_readl(host, TMOUT) & 0xff;
 cto_div = (mci_readl(host, CLKDIV) & 0xff) * 2;
@@ -413,8 +414,24 @@ static inline void dw_mci_set_cto(struct dw_mci
*host)
 /* add a bit spare time */
 cto_ms += 10;
   - mod_timer(>cto_timer,
- jiffies + msecs_to_jiffies(cto_ms) + 1);
+   /*
+* The durations we're working with are fairly short so we have to
be
+* extra careful about synchronization here.  Specifically in
hardware a
+* command timeout is _at most_ 5.1 ms, so that means we expect an
+* interrupt (either command done or timeout) to come rather
quickly
+* after the mci_writel.  ...but just in case we have a long
interrupt
+* latency let's add a bit of paranoia.
+*
+* In general we'll assume that at least an interrupt will be
asserted
+* in hardware by the time the cto_timer runs.  ...and if it
hasn't
+* been asserted in hardware by that time then we'll assume it'll
never
+* come.
+*/
+   spin_lock_irqsave(>irq_lock, irqflags);
+   if (!test_bit(EVENT_CMD_COMPLETE, >pending_events))
+   mod_timer(>cto_timer,
+   jiffies + msecs_to_jiffies(cto_ms) + 1);
+   spin_unlock_irqrestore(>irq_lock, irqflags);



IIUC, this change is beacuse you move
mci_writel(host, CMD, cmd_flags | SDMMC_CMD_START) before
setting up the timer, so there is a timing gap that the cmd_done
already comes and handled by dw_mci_interrupt->dw_mci_cmd_interrupt.
At this point, we don't need the cto timer at all.


As per below, if I don't move the mci_writel() before setting up the
timer then there's still a race.  ...and actually that race was harder
for me to write code for, but I invite you to try to see if it's
somehow cleaner.



   }
 static void dw_mci_start_command(struct dw_mci *host,
@@ -429,11 +446,11 @@ static void dw_mci_start_command(struct dw_mci
*host,
 wmb(); /* drain writebuffer */
 dw_mci_wait_while_busy(host, cmd_flags);
   + mci_writel(host, CMD, cmd_flags | SDMMC_CMD_START);
+
 /* response expected command only */
 if (cmd_flags & SDMMC_CMD_RESP_EXP)
 dw_mci_set_cto(host);
-
-   mci_writel(host, CMD, cmd_flags | SDMMC_CMD_START);




But why? If we still keep the original logic, it's always correct
that cmd_done comes after setting up the cto timer. So could you
eleborate a bit more to help me understand the real intention here?


No matter which order you put things, there's a race one way or the
other.  You need a lock.

Let's think about the old code you wrote.  You did thi