mbox series

[v3,0/6] mmc: tmio: refactor TMIO core a bit and add UniPhier SD/eMMC controller support

Message ID 1534999460-15111-1-git-send-email-yamada.masahiro@socionext.com
Headers show
Series mmc: tmio: refactor TMIO core a bit and add UniPhier SD/eMMC controller support | expand

Message

Masahiro Yamada Aug. 23, 2018, 4:44 a.m. UTC
Add UniPhier SD/eMMC controller support.

As a preparation, I changed tmio_mmc_set_clock() to a platform hook.
The clock rate setting is platform-specific, and UniPhier variants
will add another way.  I thought it would be better to split this
to a hook to avoid a mess.

V3 is rebase on top of Linus' tree.

I dropped 6/7 because Renesas added more quirks
in the current MW.


Masahiro Yamada (6):
  mmc: tmio: replace tmio_mmc_clk_stop() calls with tmio_mmc_set_clock()
  mmc: tmio: move tmio_mmc_set_clock() to platform hook
  dt-bindings: mmc: add DT binding for UniPhier SD/eMMC controller
  mmc: uniphier-sd: add UniPhier SD/eMMC controller driver
  mmc: renesas_sdhi: merge clk_{start,stop} functions to set_clock
  mmc: tmio: refactor CLK_CTL bit calculation

 .../devicetree/bindings/mmc/uniphier-sd.txt        |  55 ++
 MAINTAINERS                                        |   1 +
 drivers/mmc/host/Kconfig                           |  10 +
 drivers/mmc/host/Makefile                          |   1 +
 drivers/mmc/host/renesas_sdhi_core.c               |  48 +-
 drivers/mmc/host/tmio_mmc.c                        |  56 ++
 drivers/mmc/host/tmio_mmc.h                        |   4 +-
 drivers/mmc/host/tmio_mmc_core.c                   |  92 +--
 drivers/mmc/host/uniphier-sd.c                     | 693 +++++++++++++++++++++
 9 files changed, 871 insertions(+), 89 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mmc/uniphier-sd.txt
 create mode 100644 drivers/mmc/host/uniphier-sd.c

-- 
2.7.4

Comments

Ulf Hansson Aug. 23, 2018, 10:43 a.m. UTC | #1
[...]

> +static void uniphier_sd_external_dma_request(struct tmio_mmc_host *host,

> +                                            struct tmio_mmc_data *pdata)

> +{

> +       struct uniphier_sd_priv *priv = uniphier_sd_priv(host);

> +       struct dma_chan *chan;

> +

> +       chan = dma_request_chan(mmc_dev(host->mmc), "rx-tx");

> +       if (IS_ERR(chan)) {

> +               dev_warn(mmc_dev(host->mmc),

> +                        "failed to request DMA channel. falling back to PIO\n");

> +               return; /* just use PIO even for -EPROBE_DEFER */

> +       }

> +

> +       /* this driver uses a single channel for both RX an TX */

> +       priv->chan = chan;

> +       host->chan_rx = chan;

> +       host->chan_tx = chan;

> +

> +       tasklet_init(&host->dma_issue, uniphier_sd_external_dma_issue,

> +                    (unsigned long)host);


Isn't it time to get rid of tasklets for tmio?

No worries, I don't require that to be changed prior $subject patch,
however using threaded IRQs is a justified modernization for the tmio
variants.

[...]

Kind regards
Uffe
Wolfram Sang Aug. 29, 2018, 8:27 a.m. UTC | #2
On Thu, Aug 23, 2018 at 12:43:31PM +0200, Ulf Hansson wrote:
> On 23 August 2018 at 06:44, Masahiro Yamada

> <yamada.masahiro@socionext.com> wrote:

> >

> > Add UniPhier SD/eMMC controller support.

> >

> > As a preparation, I changed tmio_mmc_set_clock() to a platform hook.

> > The clock rate setting is platform-specific, and UniPhier variants

> > will add another way.  I thought it would be better to split this

> > to a hook to avoid a mess.

> >

> > V3 is rebase on top of Linus' tree.

> 

> I have queued patch 1 -> 4, waiting for further reviews for patch 5 and 6.


I have re-tested patches 1-4 as applied to mmc/next and it all works
fine. For those

Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


Now, going to check patches 5+6...

> > I dropped 6/7 because Renesas added more quirks

> > in the current MW.


Totally fine. We need to cleanup HS400 support more, then we can try the
simplification you did for tmio_mmc.c, too. Thanks!
Ulf Hansson Aug. 30, 2018, 11:51 a.m. UTC | #3
On 29 August 2018 at 10:49, Wolfram Sang <wsa@the-dreams.de> wrote:
>

>> +     if (divisor <= 1) {

>> +             clk_sel = 1;

>> +             clk = 0;

>> +     } else {

>> +             clk_sel = 0;

>> +             /* bit7 set: 1/512, ... bit0 set:1/4, all bits clear: 1/2 */

>> +             clk = roundup_pow_of_two(divisor) >> 2;

>> +     }

>

> What about

>

>         clk_sel = (divisor <= 1);

>         clk = clk_sel ? 0 : (roundup_pow_of_two(divisor) >> 2)

>

> More concise, but I think still readable. I don't mind super much,

> though.


Right. So, may I apply Yamada-san's original patch (adding your
tested/reviewed-by tag) and the above as a code-cleanup for you to
address on top?

Kind regards
Uffe
Ulf Hansson Aug. 30, 2018, 12:30 p.m. UTC | #4
On 23 August 2018 at 06:44, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
>   for (clk = 0x80000080; new_clock >= (clock << 1); clk >>= 1)

>           clock <<= 1;

>

> ... is too tricky, hence I replaced with

>

>   roundup_pow_of_two(divisor) >> 2

>

> '(clk >> 22) & 0x1' is the bit test for the 1/1 divisor, but

> it is not clear.  'divisor <= 1' is easier to understand.

>

> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>


Applied for next, thanks!

Kind regards
Uffe

> ---

>

> Changes in v3: None

> Changes in v2: None

>

>  drivers/mmc/host/tmio_mmc.c | 18 +++++++++++++-----

>  1 file changed, 13 insertions(+), 5 deletions(-)

>

> diff --git a/drivers/mmc/host/tmio_mmc.c b/drivers/mmc/host/tmio_mmc.c

> index b031a77..e04c322 100644

> --- a/drivers/mmc/host/tmio_mmc.c

> +++ b/drivers/mmc/host/tmio_mmc.c

> @@ -48,19 +48,27 @@ static void tmio_mmc_clk_stop(struct tmio_mmc_host *host)

>  static void tmio_mmc_set_clock(struct tmio_mmc_host *host,

>                                unsigned int new_clock)

>  {

> -       u32 clk = 0, clock;

> +       unsigned int clock, divisor;

> +       u32 clk = 0;

> +       int clk_sel;

>

>         if (new_clock == 0) {

>                 tmio_mmc_clk_stop(host);

>                 return;

>         }

>

> -       clock = host->mmc->f_min;

> +       divisor = host->pdata->hclk / new_clock;

>

> -       for (clk = 0x80000080; new_clock >= (clock << 1); clk >>= 1)

> -               clock <<= 1;

> +       if (divisor <= 1) {

> +               clk_sel = 1;

> +               clk = 0;

> +       } else {

> +               clk_sel = 0;

> +               /* bit7 set: 1/512, ... bit0 set:1/4, all bits clear: 1/2 */

> +               clk = roundup_pow_of_two(divisor) >> 2;

> +       }

>

> -       host->pdata->set_clk_div(host->pdev, (clk >> 22) & 1);

> +       host->pdata->set_clk_div(host->pdev, clk_sel);

>

>         sd_ctrl_write16(host, CTL_SD_CARD_CLK_CTL, ~CLK_CTL_SCLKEN &

>                         sd_ctrl_read16(host, CTL_SD_CARD_CLK_CTL));

> --

> 2.7.4

>