* Re: [PATCH v6 3/5] Documentation: synopsys-dw-mshc: add binding for fifo quirks
From: Jun Nie @ 2016-11-24 2:19 UTC (permalink / raw)
To: Shawn Guo, xie.baoyou, Rob Herring, mark.rutland
Cc: Ulf Hansson, Jaehoon Chung, Jason Liu, chen.chaokai, lai.binz,
linux-mmc, Jun Nie, devicetree
In-Reply-To: <1479450555-19047-4-git-send-email-jun.nie@linaro.org>
2016-11-18 14:29 GMT+08:00 Jun Nie <jun.nie@linaro.org>:
> Add fifo-addr property and fifo-watermark-quirk property to
> synopsys-dw-mshc bindings. It is intended to provide more
> dt interface to support SoCs specific configuration.
>
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
> Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> index 4e00e85..8bf2e41 100644
> --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt
> @@ -76,6 +76,17 @@ Optional properties:
>
> * broken-cd: as documented in mmc core bindings.
>
> +* data-addr: Override fifo address with value provided by DT. The default FIFO reg
> + offset is assumed as 0x100 (version < 0x240A) and 0x200(version >= 0x240A) by
> + driver. If the controller does not follow this rule, please use this property
> + to set fifo address in device tree.
> +
> +* fifo-watermark-aligned: Data done irq is expected if data length is less than
> + watermark in PIO mode. But fifo watermark is requested to be aligned with data
> + length in some SoC so that TX/RX irq can be generated with data done irq. Add this
> + watermark quirk to mark this requirement and force fifo watermark setting
> + accordingly.
> +
> * vmmc-supply: The phandle to the regulator to use for vmmc. If this is
> specified we'll defer probe until we can find this regulator.
>
> @@ -103,6 +114,8 @@ board specific portions as listed below.
> interrupts = <0 75 0>;
> #address-cells = <1>;
> #size-cells = <0>;
> + data-addr = <0x200>;
> + fifo-watermark-aligned;
> };
>
> [board specific internal DMA resources]
> --
> 1.9.1
>
Hi Rob & Mark,
Could you help review and act this patch if you think it is OK? Thank you!
Jun
^ permalink raw reply
* Re: [PATCH v2] mmc: block: delete packed command support
From: Jaehoon Chung @ 2016-11-24 5:24 UTC (permalink / raw)
To: Linus Walleij, linux-mmc, Ulf Hansson
Cc: Chunyan Zhang, Baolin Wang, Namjae Jeon, Maya Erez
In-Reply-To: <1479916086-26641-1-git-send-email-linus.walleij@linaro.org>
Hi Linus,
On 11/24/2016 12:48 AM, Linus Walleij wrote:
> I've had it with this code now.
>
> The packed command support is a complex hurdle in the MMC/SD block
> layer, around 500+ lines of code which was introduced in 2013 in
> commits
>
> ce39f9d17c14 ("mmc: support packed write command for eMMC4.5 devices")
> abd9ac144947 ("mmc: add packed command feature of eMMC4.5")
>
> ...and since then it has been rotting. The original author of the
> code has disappeared from the community and the mail address is
> bouncing.
>
> For the code to be exercised the host must flag that it supports
> packed commands, so in mmc_blk_prep_packed_list() which is called for
> every single request, the following construction appears:
>
> u8 max_packed_rw = 0;
>
> if ((rq_data_dir(cur) == WRITE) &&
> mmc_host_packed_wr(card->host))
> max_packed_rw = card->ext_csd.max_packed_writes;
>
> if (max_packed_rw == 0)
> goto no_packed;
>
> This has the following logical deductions:
>
> - Only WRITE commands can really be packed, so the solution is
> only half-done: we support packed WRITE but not packed READ.
> The packed command support has not been finalized by supporting
> reads in three years!
>
> - mmc_host_packed_wr() is just a static inline that checks
> host->caps2 & MMC_CAP2_PACKED_WR. The problem with this is
> that NO upstream host sets this capability flag! No driver
> in the kernel is using it, and we can't test it. Packed
> command may be supported in out-of-tree code, but I doubt
> it. I doubt that the code is even working anymore due to
> other refactorings in the MMC block layer, who would
> notice if patches affecting it broke packed commands?
> No one.
>
> - There is no Device Tree binding or code to mark a host as
> supporting packed read or write commands, just this flag
> in caps2, so for sure there are not any DT systems using
> it either.
>
> It has other problems as well: mmc_blk_prep_packed_list() is
> speculatively picking requests out of the request queue with
> blk_fetch_request() making the MMC/SD stack harder to convert
> to the multiqueue block layer. By this we get rid of an
> obstacle.
>
> The way I see it this is just cruft littering the MMC/SD
> stack.
>
> Cc: Namjae Jeon <namjae.jeon@samsung.com>
> Cc: Maya Erez <qca_merez@qca.qualcomm.com>
> Acked-by: Jaehoon Chung <jh80.chung@samsung.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Did i send the acked-by tag? :)
I just agreed some this patch's points.
When i become assured, i will send the acked-by tag. is this way right?
Best Regards,
Jaehoon Chung
> ---
> ChangeLog v1->v2:
> - Rebased on Ulf's next branch
> - Added Jaehoon's ACK
> ---
> drivers/mmc/card/block.c | 482 ++---------------------------------------------
> drivers/mmc/card/queue.c | 53 +-----
> drivers/mmc/card/queue.h | 19 --
> include/linux/mmc/host.h | 5 -
> 4 files changed, 20 insertions(+), 539 deletions(-)
>
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index 6618126fcb9f..3d0f1e05a425 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -66,9 +66,6 @@ MODULE_ALIAS("mmc:block");
>
> #define mmc_req_rel_wr(req) ((req->cmd_flags & REQ_FUA) && \
> (rq_data_dir(req) == WRITE))
> -#define PACKED_CMD_VER 0x01
> -#define PACKED_CMD_WR 0x02
> -
> static DEFINE_MUTEX(block_mutex);
>
> /*
> @@ -102,7 +99,6 @@ struct mmc_blk_data {
> unsigned int flags;
> #define MMC_BLK_CMD23 (1 << 0) /* Can do SET_BLOCK_COUNT for multiblock */
> #define MMC_BLK_REL_WR (1 << 1) /* MMC Reliable write support */
> -#define MMC_BLK_PACKED_CMD (1 << 2) /* MMC packed command support */
>
> unsigned int usage;
> unsigned int read_only;
> @@ -126,12 +122,6 @@ struct mmc_blk_data {
>
> static DEFINE_MUTEX(open_lock);
>
> -enum {
> - MMC_PACKED_NR_IDX = -1,
> - MMC_PACKED_NR_ZERO,
> - MMC_PACKED_NR_SINGLE,
> -};
> -
> module_param(perdev_minors, int, 0444);
> MODULE_PARM_DESC(perdev_minors, "Minors numbers to allocate per device");
>
> @@ -139,17 +129,6 @@ static inline int mmc_blk_part_switch(struct mmc_card *card,
> struct mmc_blk_data *md);
> static int get_card_status(struct mmc_card *card, u32 *status, int retries);
>
> -static inline void mmc_blk_clear_packed(struct mmc_queue_req *mqrq)
> -{
> - struct mmc_packed *packed = mqrq->packed;
> -
> - mqrq->cmd_type = MMC_PACKED_NONE;
> - packed->nr_entries = MMC_PACKED_NR_ZERO;
> - packed->idx_failure = MMC_PACKED_NR_IDX;
> - packed->retries = 0;
> - packed->blocks = 0;
> -}
> -
> static struct mmc_blk_data *mmc_blk_get(struct gendisk *disk)
> {
> struct mmc_blk_data *md;
> @@ -1420,111 +1399,12 @@ static enum mmc_blk_status mmc_blk_err_check(struct mmc_card *card,
> if (!brq->data.bytes_xfered)
> return MMC_BLK_RETRY;
>
> - if (mmc_packed_cmd(mq_mrq->cmd_type)) {
> - if (unlikely(brq->data.blocks << 9 != brq->data.bytes_xfered))
> - return MMC_BLK_PARTIAL;
> - else
> - return MMC_BLK_SUCCESS;
> - }
> -
> if (blk_rq_bytes(req) != brq->data.bytes_xfered)
> return MMC_BLK_PARTIAL;
>
> return MMC_BLK_SUCCESS;
> }
>
> -static int mmc_packed_init(struct mmc_queue *mq, struct mmc_card *card)
> -{
> - struct mmc_queue_req *mqrq_cur = &mq->mqrq[0];
> - struct mmc_queue_req *mqrq_prev = &mq->mqrq[1];
> - int ret = 0;
> -
> -
> - mqrq_cur->packed = kzalloc(sizeof(struct mmc_packed), GFP_KERNEL);
> - if (!mqrq_cur->packed) {
> - pr_warn("%s: unable to allocate packed cmd for mqrq_cur\n",
> - mmc_card_name(card));
> - ret = -ENOMEM;
> - goto out;
> - }
> -
> - mqrq_prev->packed = kzalloc(sizeof(struct mmc_packed), GFP_KERNEL);
> - if (!mqrq_prev->packed) {
> - pr_warn("%s: unable to allocate packed cmd for mqrq_prev\n",
> - mmc_card_name(card));
> - kfree(mqrq_cur->packed);
> - mqrq_cur->packed = NULL;
> - ret = -ENOMEM;
> - goto out;
> - }
> -
> - INIT_LIST_HEAD(&mqrq_cur->packed->list);
> - INIT_LIST_HEAD(&mqrq_prev->packed->list);
> -
> -out:
> - return ret;
> -}
> -
> -static void mmc_packed_clean(struct mmc_queue *mq)
> -{
> - struct mmc_queue_req *mqrq_cur = &mq->mqrq[0];
> - struct mmc_queue_req *mqrq_prev = &mq->mqrq[1];
> -
> - kfree(mqrq_cur->packed);
> - mqrq_cur->packed = NULL;
> - kfree(mqrq_prev->packed);
> - mqrq_prev->packed = NULL;
> -}
> -
> -static enum mmc_blk_status mmc_blk_packed_err_check(struct mmc_card *card,
> - struct mmc_async_req *areq)
> -{
> - struct mmc_queue_req *mq_rq = container_of(areq, struct mmc_queue_req,
> - mmc_active);
> - struct request *req = mq_rq->req;
> - struct mmc_packed *packed = mq_rq->packed;
> - enum mmc_blk_status status, check;
> - int err;
> - u8 *ext_csd;
> -
> - packed->retries--;
> - check = mmc_blk_err_check(card, areq);
> - err = get_card_status(card, &status, 0);
> - if (err) {
> - pr_err("%s: error %d sending status command\n",
> - req->rq_disk->disk_name, err);
> - return MMC_BLK_ABORT;
> - }
> -
> - if (status & R1_EXCEPTION_EVENT) {
> - err = mmc_get_ext_csd(card, &ext_csd);
> - if (err) {
> - pr_err("%s: error %d sending ext_csd\n",
> - req->rq_disk->disk_name, err);
> - return MMC_BLK_ABORT;
> - }
> -
> - if ((ext_csd[EXT_CSD_EXP_EVENTS_STATUS] &
> - EXT_CSD_PACKED_FAILURE) &&
> - (ext_csd[EXT_CSD_PACKED_CMD_STATUS] &
> - EXT_CSD_PACKED_GENERIC_ERROR)) {
> - if (ext_csd[EXT_CSD_PACKED_CMD_STATUS] &
> - EXT_CSD_PACKED_INDEXED_ERROR) {
> - packed->idx_failure =
> - ext_csd[EXT_CSD_PACKED_FAILURE_INDEX] - 1;
> - check = MMC_BLK_PARTIAL;
> - }
> - pr_err("%s: packed cmd failed, nr %u, sectors %u, "
> - "failure index: %d\n",
> - req->rq_disk->disk_name, packed->nr_entries,
> - packed->blocks, packed->idx_failure);
> - }
> - kfree(ext_csd);
> - }
> -
> - return check;
> -}
> -
> static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
> struct mmc_card *card,
> int disable_multi,
> @@ -1685,222 +1565,6 @@ static void mmc_blk_rw_rq_prep(struct mmc_queue_req *mqrq,
> mmc_queue_bounce_pre(mqrq);
> }
>
> -static inline u8 mmc_calc_packed_hdr_segs(struct request_queue *q,
> - struct mmc_card *card)
> -{
> - unsigned int hdr_sz = mmc_large_sector(card) ? 4096 : 512;
> - unsigned int max_seg_sz = queue_max_segment_size(q);
> - unsigned int len, nr_segs = 0;
> -
> - do {
> - len = min(hdr_sz, max_seg_sz);
> - hdr_sz -= len;
> - nr_segs++;
> - } while (hdr_sz);
> -
> - return nr_segs;
> -}
> -
> -static u8 mmc_blk_prep_packed_list(struct mmc_queue *mq, struct request *req)
> -{
> - struct request_queue *q = mq->queue;
> - struct mmc_card *card = mq->card;
> - struct request *cur = req, *next = NULL;
> - struct mmc_blk_data *md = mq->blkdata;
> - struct mmc_queue_req *mqrq = mq->mqrq_cur;
> - bool en_rel_wr = card->ext_csd.rel_param & EXT_CSD_WR_REL_PARAM_EN;
> - unsigned int req_sectors = 0, phys_segments = 0;
> - unsigned int max_blk_count, max_phys_segs;
> - bool put_back = true;
> - u8 max_packed_rw = 0;
> - u8 reqs = 0;
> -
> - /*
> - * We don't need to check packed for any further
> - * operation of packed stuff as we set MMC_PACKED_NONE
> - * and return zero for reqs if geting null packed. Also
> - * we clean the flag of MMC_BLK_PACKED_CMD to avoid doing
> - * it again when removing blk req.
> - */
> - if (!mqrq->packed) {
> - md->flags &= (~MMC_BLK_PACKED_CMD);
> - goto no_packed;
> - }
> -
> - if (!(md->flags & MMC_BLK_PACKED_CMD))
> - goto no_packed;
> -
> - if ((rq_data_dir(cur) == WRITE) &&
> - mmc_host_packed_wr(card->host))
> - max_packed_rw = card->ext_csd.max_packed_writes;
> -
> - if (max_packed_rw == 0)
> - goto no_packed;
> -
> - if (mmc_req_rel_wr(cur) &&
> - (md->flags & MMC_BLK_REL_WR) && !en_rel_wr)
> - goto no_packed;
> -
> - if (mmc_large_sector(card) &&
> - !IS_ALIGNED(blk_rq_sectors(cur), 8))
> - goto no_packed;
> -
> - mmc_blk_clear_packed(mqrq);
> -
> - max_blk_count = min(card->host->max_blk_count,
> - card->host->max_req_size >> 9);
> - if (unlikely(max_blk_count > 0xffff))
> - max_blk_count = 0xffff;
> -
> - max_phys_segs = queue_max_segments(q);
> - req_sectors += blk_rq_sectors(cur);
> - phys_segments += cur->nr_phys_segments;
> -
> - if (rq_data_dir(cur) == WRITE) {
> - req_sectors += mmc_large_sector(card) ? 8 : 1;
> - phys_segments += mmc_calc_packed_hdr_segs(q, card);
> - }
> -
> - do {
> - if (reqs >= max_packed_rw - 1) {
> - put_back = false;
> - break;
> - }
> -
> - spin_lock_irq(q->queue_lock);
> - next = blk_fetch_request(q);
> - spin_unlock_irq(q->queue_lock);
> - if (!next) {
> - put_back = false;
> - break;
> - }
> -
> - if (mmc_large_sector(card) &&
> - !IS_ALIGNED(blk_rq_sectors(next), 8))
> - break;
> -
> - if (mmc_req_is_special(next))
> - break;
> -
> - if (rq_data_dir(cur) != rq_data_dir(next))
> - break;
> -
> - if (mmc_req_rel_wr(next) &&
> - (md->flags & MMC_BLK_REL_WR) && !en_rel_wr)
> - break;
> -
> - req_sectors += blk_rq_sectors(next);
> - if (req_sectors > max_blk_count)
> - break;
> -
> - phys_segments += next->nr_phys_segments;
> - if (phys_segments > max_phys_segs)
> - break;
> -
> - list_add_tail(&next->queuelist, &mqrq->packed->list);
> - cur = next;
> - reqs++;
> - } while (1);
> -
> - if (put_back) {
> - spin_lock_irq(q->queue_lock);
> - blk_requeue_request(q, next);
> - spin_unlock_irq(q->queue_lock);
> - }
> -
> - if (reqs > 0) {
> - list_add(&req->queuelist, &mqrq->packed->list);
> - mqrq->packed->nr_entries = ++reqs;
> - mqrq->packed->retries = reqs;
> - return reqs;
> - }
> -
> -no_packed:
> - mqrq->cmd_type = MMC_PACKED_NONE;
> - return 0;
> -}
> -
> -static void mmc_blk_packed_hdr_wrq_prep(struct mmc_queue_req *mqrq,
> - struct mmc_card *card,
> - struct mmc_queue *mq)
> -{
> - struct mmc_blk_request *brq = &mqrq->brq;
> - struct request *req = mqrq->req;
> - struct request *prq;
> - struct mmc_blk_data *md = mq->blkdata;
> - struct mmc_packed *packed = mqrq->packed;
> - bool do_rel_wr, do_data_tag;
> - __le32 *packed_cmd_hdr;
> - u8 hdr_blocks;
> - u8 i = 1;
> -
> - mqrq->cmd_type = MMC_PACKED_WRITE;
> - packed->blocks = 0;
> - packed->idx_failure = MMC_PACKED_NR_IDX;
> -
> - packed_cmd_hdr = packed->cmd_hdr;
> - memset(packed_cmd_hdr, 0, sizeof(packed->cmd_hdr));
> - packed_cmd_hdr[0] = cpu_to_le32((packed->nr_entries << 16) |
> - (PACKED_CMD_WR << 8) | PACKED_CMD_VER);
> - hdr_blocks = mmc_large_sector(card) ? 8 : 1;
> -
> - /*
> - * Argument for each entry of packed group
> - */
> - list_for_each_entry(prq, &packed->list, queuelist) {
> - do_rel_wr = mmc_req_rel_wr(prq) && (md->flags & MMC_BLK_REL_WR);
> - do_data_tag = (card->ext_csd.data_tag_unit_size) &&
> - (prq->cmd_flags & REQ_META) &&
> - (rq_data_dir(prq) == WRITE) &&
> - blk_rq_bytes(prq) >= card->ext_csd.data_tag_unit_size;
> - /* Argument of CMD23 */
> - packed_cmd_hdr[(i * 2)] = cpu_to_le32(
> - (do_rel_wr ? MMC_CMD23_ARG_REL_WR : 0) |
> - (do_data_tag ? MMC_CMD23_ARG_TAG_REQ : 0) |
> - blk_rq_sectors(prq));
> - /* Argument of CMD18 or CMD25 */
> - packed_cmd_hdr[((i * 2)) + 1] = cpu_to_le32(
> - mmc_card_blockaddr(card) ?
> - blk_rq_pos(prq) : blk_rq_pos(prq) << 9);
> - packed->blocks += blk_rq_sectors(prq);
> - i++;
> - }
> -
> - memset(brq, 0, sizeof(struct mmc_blk_request));
> - brq->mrq.cmd = &brq->cmd;
> - brq->mrq.data = &brq->data;
> - brq->mrq.sbc = &brq->sbc;
> - brq->mrq.stop = &brq->stop;
> -
> - brq->sbc.opcode = MMC_SET_BLOCK_COUNT;
> - brq->sbc.arg = MMC_CMD23_ARG_PACKED | (packed->blocks + hdr_blocks);
> - brq->sbc.flags = MMC_RSP_R1 | MMC_CMD_AC;
> -
> - brq->cmd.opcode = MMC_WRITE_MULTIPLE_BLOCK;
> - brq->cmd.arg = blk_rq_pos(req);
> - if (!mmc_card_blockaddr(card))
> - brq->cmd.arg <<= 9;
> - brq->cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
> -
> - brq->data.blksz = 512;
> - brq->data.blocks = packed->blocks + hdr_blocks;
> - brq->data.flags = MMC_DATA_WRITE;
> -
> - brq->stop.opcode = MMC_STOP_TRANSMISSION;
> - brq->stop.arg = 0;
> - brq->stop.flags = MMC_RSP_SPI_R1B | MMC_RSP_R1B | MMC_CMD_AC;
> -
> - mmc_set_data_timeout(&brq->data, card);
> -
> - brq->data.sg = mqrq->sg;
> - brq->data.sg_len = mmc_queue_map_sg(mq, mqrq);
> -
> - mqrq->mmc_active.mrq = &brq->mrq;
> - mqrq->mmc_active.err_check = mmc_blk_packed_err_check;
> -
> - mmc_queue_bounce_pre(mqrq);
> -}
> -
> static int mmc_blk_cmd_err(struct mmc_blk_data *md, struct mmc_card *card,
> struct mmc_blk_request *brq, struct request *req,
> int ret)
> @@ -1923,79 +1587,10 @@ static int mmc_blk_cmd_err(struct mmc_blk_data *md, struct mmc_card *card,
> if (blocks != (u32)-1) {
> ret = blk_end_request(req, 0, blocks << 9);
> }
> - } else {
> - if (!mmc_packed_cmd(mq_rq->cmd_type))
> - ret = blk_end_request(req, 0, brq->data.bytes_xfered);
> - }
> - return ret;
> -}
> -
> -static int mmc_blk_end_packed_req(struct mmc_queue_req *mq_rq)
> -{
> - struct request *prq;
> - struct mmc_packed *packed = mq_rq->packed;
> - int idx = packed->idx_failure, i = 0;
> - int ret = 0;
> -
> - while (!list_empty(&packed->list)) {
> - prq = list_entry_rq(packed->list.next);
> - if (idx == i) {
> - /* retry from error index */
> - packed->nr_entries -= idx;
> - mq_rq->req = prq;
> - ret = 1;
> -
> - if (packed->nr_entries == MMC_PACKED_NR_SINGLE) {
> - list_del_init(&prq->queuelist);
> - mmc_blk_clear_packed(mq_rq);
> - }
> - return ret;
> - }
> - list_del_init(&prq->queuelist);
> - blk_end_request(prq, 0, blk_rq_bytes(prq));
> - i++;
> }
> -
> - mmc_blk_clear_packed(mq_rq);
> return ret;
> }
>
> -static void mmc_blk_abort_packed_req(struct mmc_queue_req *mq_rq)
> -{
> - struct request *prq;
> - struct mmc_packed *packed = mq_rq->packed;
> -
> - while (!list_empty(&packed->list)) {
> - prq = list_entry_rq(packed->list.next);
> - list_del_init(&prq->queuelist);
> - blk_end_request(prq, -EIO, blk_rq_bytes(prq));
> - }
> -
> - mmc_blk_clear_packed(mq_rq);
> -}
> -
> -static void mmc_blk_revert_packed_req(struct mmc_queue *mq,
> - struct mmc_queue_req *mq_rq)
> -{
> - struct request *prq;
> - struct request_queue *q = mq->queue;
> - struct mmc_packed *packed = mq_rq->packed;
> -
> - while (!list_empty(&packed->list)) {
> - prq = list_entry_rq(packed->list.prev);
> - if (prq->queuelist.prev != &packed->list) {
> - list_del_init(&prq->queuelist);
> - spin_lock_irq(q->queue_lock);
> - blk_requeue_request(mq->queue, prq);
> - spin_unlock_irq(q->queue_lock);
> - } else {
> - list_del_init(&prq->queuelist);
> - }
> - }
> -
> - mmc_blk_clear_packed(mq_rq);
> -}
> -
> static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
> {
> struct mmc_blk_data *md = mq->blkdata;
> @@ -2006,15 +1601,11 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
> struct mmc_queue_req *mq_rq;
> struct request *req = rqc;
> struct mmc_async_req *areq;
> - const u8 packed_nr = 2;
> u8 reqs = 0;
>
> if (!rqc && !mq->mqrq_prev->req)
> return 0;
>
> - if (rqc)
> - reqs = mmc_blk_prep_packed_list(mq, rqc);
> -
> do {
> if (rqc) {
> /*
> @@ -2029,11 +1620,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
> goto cmd_abort;
> }
>
> - if (reqs >= packed_nr)
> - mmc_blk_packed_hdr_wrq_prep(mq->mqrq_cur,
> - card, mq);
> - else
> - mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
> + mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
> areq = &mq->mqrq_cur->mmc_active;
> } else
> areq = NULL;
> @@ -2058,13 +1645,8 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
> */
> mmc_blk_reset_success(md, type);
>
> - if (mmc_packed_cmd(mq_rq->cmd_type)) {
> - ret = mmc_blk_end_packed_req(mq_rq);
> - break;
> - } else {
> - ret = blk_end_request(req, 0,
> - brq->data.bytes_xfered);
> - }
> + ret = blk_end_request(req, 0,
> + brq->data.bytes_xfered);
>
> /*
> * If the blk_end_request function returns non-zero even
> @@ -2101,8 +1683,7 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
> err = mmc_blk_reset(md, card->host, type);
> if (!err)
> break;
> - if (err == -ENODEV ||
> - mmc_packed_cmd(mq_rq->cmd_type))
> + if (err == -ENODEV)
> goto cmd_abort;
> /* Fall through */
> }
> @@ -2133,23 +1714,14 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
> }
>
> if (ret) {
> - if (mmc_packed_cmd(mq_rq->cmd_type)) {
> - if (!mq_rq->packed->retries)
> - goto cmd_abort;
> - mmc_blk_packed_hdr_wrq_prep(mq_rq, card, mq);
> - mmc_start_req(card->host,
> - &mq_rq->mmc_active, NULL);
> - } else {
> -
> - /*
> - * In case of a incomplete request
> - * prepare it again and resend.
> - */
> - mmc_blk_rw_rq_prep(mq_rq, card,
> - disable_multi, mq);
> - mmc_start_req(card->host,
> - &mq_rq->mmc_active, NULL);
> - }
> + /*
> + * In case of a incomplete request
> + * prepare it again and resend.
> + */
> + mmc_blk_rw_rq_prep(mq_rq, card,
> + disable_multi, mq);
> + mmc_start_req(card->host,
> + &mq_rq->mmc_active, NULL);
> mq_rq->brq.retune_retry_done = retune_retry_done;
> }
> } while (ret);
> @@ -2157,15 +1729,11 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
> return 1;
>
> cmd_abort:
> - if (mmc_packed_cmd(mq_rq->cmd_type)) {
> - mmc_blk_abort_packed_req(mq_rq);
> - } else {
> - if (mmc_card_removed(card))
> - req->cmd_flags |= REQ_QUIET;
> - while (ret)
> - ret = blk_end_request(req, -EIO,
> - blk_rq_cur_bytes(req));
> - }
> + if (mmc_card_removed(card))
> + req->cmd_flags |= REQ_QUIET;
> + while (ret)
> + ret = blk_end_request(req, -EIO,
> + blk_rq_cur_bytes(req));
>
> start_new_req:
> if (rqc) {
> @@ -2173,12 +1741,6 @@ static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
> rqc->cmd_flags |= REQ_QUIET;
> blk_end_request_all(rqc, -EIO);
> } else {
> - /*
> - * If current request is packed, it needs to put back.
> - */
> - if (mmc_packed_cmd(mq->mqrq_cur->cmd_type))
> - mmc_blk_revert_packed_req(mq, mq->mqrq_cur);
> -
> mmc_blk_rw_rq_prep(mq->mqrq_cur, card, 0, mq);
> mmc_start_req(card->host,
> &mq->mqrq_cur->mmc_active, NULL);
> @@ -2361,14 +1923,6 @@ static struct mmc_blk_data *mmc_blk_alloc_req(struct mmc_card *card,
> blk_queue_write_cache(md->queue.queue, true, true);
> }
>
> - if (mmc_card_mmc(card) &&
> - (area_type == MMC_BLK_DATA_AREA_MAIN) &&
> - (md->flags & MMC_BLK_CMD23) &&
> - card->ext_csd.packed_event_en) {
> - if (!mmc_packed_init(&md->queue, card))
> - md->flags |= MMC_BLK_PACKED_CMD;
> - }
> -
> return md;
>
> err_putdisk:
> @@ -2472,8 +2026,6 @@ static void mmc_blk_remove_req(struct mmc_blk_data *md)
> */
> card = md->queue.card;
> mmc_cleanup_queue(&md->queue);
> - if (md->flags & MMC_BLK_PACKED_CMD)
> - mmc_packed_clean(&md->queue);
> if (md->disk->flags & GENHD_FL_UP) {
> device_remove_file(disk_to_dev(md->disk), &md->force_ro);
> if ((md->area_type & MMC_BLK_DATA_AREA_BOOT) &&
> diff --git a/drivers/mmc/card/queue.c b/drivers/mmc/card/queue.c
> index 3f6a2463ab30..7dacf2744fbd 100644
> --- a/drivers/mmc/card/queue.c
> +++ b/drivers/mmc/card/queue.c
> @@ -406,41 +406,6 @@ void mmc_queue_resume(struct mmc_queue *mq)
> }
> }
>
> -static unsigned int mmc_queue_packed_map_sg(struct mmc_queue *mq,
> - struct mmc_packed *packed,
> - struct scatterlist *sg,
> - enum mmc_packed_type cmd_type)
> -{
> - struct scatterlist *__sg = sg;
> - unsigned int sg_len = 0;
> - struct request *req;
> -
> - if (mmc_packed_wr(cmd_type)) {
> - unsigned int hdr_sz = mmc_large_sector(mq->card) ? 4096 : 512;
> - unsigned int max_seg_sz = queue_max_segment_size(mq->queue);
> - unsigned int len, remain, offset = 0;
> - u8 *buf = (u8 *)packed->cmd_hdr;
> -
> - remain = hdr_sz;
> - do {
> - len = min(remain, max_seg_sz);
> - sg_set_buf(__sg, buf + offset, len);
> - offset += len;
> - remain -= len;
> - sg_unmark_end(__sg++);
> - sg_len++;
> - } while (remain);
> - }
> -
> - list_for_each_entry(req, &packed->list, queuelist) {
> - sg_len += blk_rq_map_sg(mq->queue, req, __sg);
> - __sg = sg + (sg_len - 1);
> - sg_unmark_end(__sg++);
> - }
> - sg_mark_end(sg + (sg_len - 1));
> - return sg_len;
> -}
> -
> /*
> * Prepare the sg list(s) to be handed of to the host driver
> */
> @@ -449,26 +414,14 @@ unsigned int mmc_queue_map_sg(struct mmc_queue *mq, struct mmc_queue_req *mqrq)
> unsigned int sg_len;
> size_t buflen;
> struct scatterlist *sg;
> - enum mmc_packed_type cmd_type;
> int i;
>
> - cmd_type = mqrq->cmd_type;
> -
> - if (!mqrq->bounce_buf) {
> - if (mmc_packed_cmd(cmd_type))
> - return mmc_queue_packed_map_sg(mq, mqrq->packed,
> - mqrq->sg, cmd_type);
> - else
> - return blk_rq_map_sg(mq->queue, mqrq->req, mqrq->sg);
> - }
> + if (!mqrq->bounce_buf)
> + return blk_rq_map_sg(mq->queue, mqrq->req, mqrq->sg);
>
> BUG_ON(!mqrq->bounce_sg);
>
> - if (mmc_packed_cmd(cmd_type))
> - sg_len = mmc_queue_packed_map_sg(mq, mqrq->packed,
> - mqrq->bounce_sg, cmd_type);
> - else
> - sg_len = blk_rq_map_sg(mq->queue, mqrq->req, mqrq->bounce_sg);
> + sg_len = blk_rq_map_sg(mq->queue, mqrq->req, mqrq->bounce_sg);
>
> mqrq->bounce_sg_len = sg_len;
>
> diff --git a/drivers/mmc/card/queue.h b/drivers/mmc/card/queue.h
> index 334c9306070f..47f5532b5776 100644
> --- a/drivers/mmc/card/queue.h
> +++ b/drivers/mmc/card/queue.h
> @@ -22,23 +22,6 @@ struct mmc_blk_request {
> int retune_retry_done;
> };
>
> -enum mmc_packed_type {
> - MMC_PACKED_NONE = 0,
> - MMC_PACKED_WRITE,
> -};
> -
> -#define mmc_packed_cmd(type) ((type) != MMC_PACKED_NONE)
> -#define mmc_packed_wr(type) ((type) == MMC_PACKED_WRITE)
> -
> -struct mmc_packed {
> - struct list_head list;
> - __le32 cmd_hdr[1024];
> - unsigned int blocks;
> - u8 nr_entries;
> - u8 retries;
> - s16 idx_failure;
> -};
> -
> struct mmc_queue_req {
> struct request *req;
> struct mmc_blk_request brq;
> @@ -47,8 +30,6 @@ struct mmc_queue_req {
> struct scatterlist *bounce_sg;
> unsigned int bounce_sg_len;
> struct mmc_async_req mmc_active;
> - enum mmc_packed_type cmd_type;
> - struct mmc_packed *packed;
> };
>
> struct mmc_queue {
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 2a6418d0c343..2ce32fefb41c 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -494,11 +494,6 @@ static inline int mmc_host_uhs(struct mmc_host *host)
> MMC_CAP_UHS_DDR50);
> }
>
> -static inline int mmc_host_packed_wr(struct mmc_host *host)
> -{
> - return host->caps2 & MMC_CAP2_PACKED_WR;
> -}
> -
> static inline int mmc_card_hs(struct mmc_card *card)
> {
> return card->host->ios.timing == MMC_TIMING_SD_HS ||
>
^ permalink raw reply
* Re: [PATCH 2/5] smd: Make packet size a constant
From: Bjorn Andersson @ 2016-11-24 6:14 UTC (permalink / raw)
To: Jeremy McNicoll
Cc: linux-arm-msm, linux-soc, devicetree, linux-mmc, andy.gross,
sboyd, robh, arnd, riteshh
In-Reply-To: <1479863388-23678-3-git-send-email-jeremymc@redhat.com>
On Tue 22 Nov 17:09 PST 2016, Jeremy McNicoll wrote:
> Use a macro to define the maximum size of a RPM message.
>
No thanks.
> Signed-off-by: Jeremy McNicoll <jeremymc@redhat.com>
> ---
> drivers/soc/qcom/smd-rpm.c | 2 +-
> include/linux/soc/qcom/smd.h | 7 +++++++
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/soc/qcom/smd-rpm.c b/drivers/soc/qcom/smd-rpm.c
> index 6609d7e..b5a2836 100644
> --- a/drivers/soc/qcom/smd-rpm.c
> +++ b/drivers/soc/qcom/smd-rpm.c
> @@ -114,7 +114,7 @@ int qcom_rpm_smd_write(struct qcom_smd_rpm *rpm,
> size_t size = sizeof(*pkt) + count;
>
> /* SMD packets to the RPM may not exceed 256 bytes */
> - if (WARN_ON(size >= 256))
> + if (WARN_ON(size >= SMD_RPM_MAX_SIZE))
> return -EINVAL;
The only thing you do is to change "oh, the max packet size is 256
bytes" to "hmm, i wonder what SMD_RPM_MAX_SIZE is and if the comment is
still valid".
>
> pkt = kmalloc(size, GFP_KERNEL);
> diff --git a/include/linux/soc/qcom/smd.h b/include/linux/soc/qcom/smd.h
> index f148e0f..8039015 100644
> --- a/include/linux/soc/qcom/smd.h
> +++ b/include/linux/soc/qcom/smd.h
> @@ -4,6 +4,13 @@
> #include <linux/device.h>
> #include <linux/mod_devicetable.h>
>
> +
> +/*
> + * SMD packets to the RPM may not exceed 256 bytes
> + */
> +#define SMD_RPM_MAX_SIZE 256
> +
And this has nothing to do with SMD, it's a limitation of RPM.
Regards,
Bjorn
^ permalink raw reply
* Re: [PATCH v2] mmc: block: delete packed command support
From: kbuild test robot @ 2016-11-24 6:35 UTC (permalink / raw)
Cc: kbuild-all, linux-mmc, Ulf Hansson, Chunyan Zhang, Baolin Wang,
Linus Walleij, Namjae Jeon, Maya Erez
In-Reply-To: <1479916086-26641-1-git-send-email-linus.walleij@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 2975 bytes --]
Hi Linus,
[auto build test WARNING on ulf.hansson-mmc/next]
[cannot apply to linus/master linux/master v4.9-rc6 next-20161123]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Linus-Walleij/mmc-block-delete-packed-command-support/20161124-095329
base: git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git next
config: x86_64-randconfig-s0-11241041 (attached as .config)
compiler: gcc-4.4 (Debian 4.4.7-8) 4.4.7
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All warnings (new ones prefixed by >>):
drivers/mmc/card/block.c: In function 'mmc_blk_issue_rw_rq':
>> drivers/mmc/card/block.c:1604: warning: unused variable 'reqs'
vim +/reqs +1604 drivers/mmc/card/block.c
ecf8b5d0a Subhash Jadavani 2012-06-07 1588 ret = blk_end_request(req, 0, blocks << 9);
67716327e Adrian Hunter 2011-08-29 1589 }
67716327e Adrian Hunter 2011-08-29 1590 }
67716327e Adrian Hunter 2011-08-29 1591 return ret;
67716327e Adrian Hunter 2011-08-29 1592 }
67716327e Adrian Hunter 2011-08-29 1593
ee8a43a51 Per Forlin 2011-07-01 1594 static int mmc_blk_issue_rw_rq(struct mmc_queue *mq, struct request *rqc)
54d49d776 Per Forlin 2011-07-01 1595 {
3e1319bfe Linus Walleij 2016-11-18 1596 struct mmc_blk_data *md = mq->blkdata;
54d49d776 Per Forlin 2011-07-01 1597 struct mmc_card *card = md->queue.card;
54d49d776 Per Forlin 2011-07-01 1598 struct mmc_blk_request *brq = &mq->mqrq_cur->brq;
b8360a494 Adrian Hunter 2015-05-07 1599 int ret = 1, disable_multi = 0, retry = 0, type, retune_retry_done = 0;
d78d4a8ad Per Forlin 2011-07-01 1600 enum mmc_blk_status status;
ee8a43a51 Per Forlin 2011-07-01 1601 struct mmc_queue_req *mq_rq;
a5075eb94 Saugata Das 2012-05-17 1602 struct request *req = rqc;
ee8a43a51 Per Forlin 2011-07-01 1603 struct mmc_async_req *areq;
ce39f9d17 Seungwon Jeon 2013-02-06 @1604 u8 reqs = 0;
ee8a43a51 Per Forlin 2011-07-01 1605
ee8a43a51 Per Forlin 2011-07-01 1606 if (!rqc && !mq->mqrq_prev->req)
ee8a43a51 Per Forlin 2011-07-01 1607 return 0;
54d49d776 Per Forlin 2011-07-01 1608
54d49d776 Per Forlin 2011-07-01 1609 do {
ee8a43a51 Per Forlin 2011-07-01 1610 if (rqc) {
a5075eb94 Saugata Das 2012-05-17 1611 /*
a5075eb94 Saugata Das 2012-05-17 1612 * When 4KB native sector is enabled, only 8 blocks
:::::: The code at line 1604 was first introduced by commit
:::::: ce39f9d17c14e56ea6772aa84393e6e0cc8499c4 mmc: support packed write command for eMMC4.5 devices
:::::: TO: Seungwon Jeon <tgih.jun@samsung.com>
:::::: CC: Chris Ball <cjb@laptop.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31381 bytes --]
^ permalink raw reply
* Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller
From: Ulf Hansson @ 2016-11-24 9:05 UTC (permalink / raw)
To: Gregory CLEMENT, Rob Herring
Cc: Ziji Hu, Adrian Hunter, linux-mmc@vger.kernel.org, Jason Cooper,
Andrew Lunn, Sebastian Hesselbarth, devicetree@vger.kernel.org,
Thomas Petazzoni, linux-arm-kernel@lists.infradead.org,
Jack(SH) Zhu, Jimmy Xu, Jisheng Zhang, Nadav Haklai, Ryan Gao,
Doug Jones, Shiwu Zhang, Victor Gu, Wei(SOCP) Liu
In-Reply-To: <87d1hno2d7.fsf@free-electrons.com>
On 22 November 2016 at 18:23, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:
> Hi Rob,
>
> On jeu., nov. 10 2016, Ziji Hu <huziji@marvell.com> wrote:
>
> [...]
>
>>>> +
>>>> +- reg:
>>>> + * For "marvell,xenon-sdhci", one register area for Xenon IP.
>>>> +
>>>> + * For "marvell,armada-3700-sdhci", two register areas.
>>>> + The first one for Xenon IP register. The second one for the Armada 3700 SOC
>>>> + PHY PAD Voltage Control register.
>>>> + Please follow the examples with compatible "marvell,armada-3700-sdhci"
>>>> + in below.
>>>> + Please also check property marvell,pad-type in below.
>>>> +
>>>> +Optional Properties:
>>>> +- marvell,xenon-slotno:
>>>
>>> Multiple slots should be represented as child nodes IMO. I think some
>>> other bindings already do this.
>>>
>>
>> All the slots are entirely independent.
>> I prefer to consider it as multiple independent SDHCs placed in
>> a single IP, instead of that a IP contains multiple child slots.
>
> It was indeed what I tried to show in my answer for the 1st version:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-October/461860.html
>
> Maybe you missed it.
>
> You also mentioned other bindings using child nodes, but for this one
> we have one controller with only one set of register with multiple slots
> (Atmel is an example). Here each slot have it own set of register.
>
> Actually giving the fact that each slot is controlled by a different set
> of register I wonder why the hardware can't also deduce the slot number
> from the address register. For me it looks like an hardware bug but we
> have to deal with it.
>
> Do you still think we needchild node here?
Using child-nodes for slots like what's done in the atmel case, is
currently broken. I would recommend to avoid using child-nodes for
slots, if possible.
To give you some more background, currently the mmc core treats child
nodes as embedded non-removable cards or SDIO funcs. However, we can
change to make child-nodes also allowed to describe slots, but it
requires a specific compatible for "slots" and of course then we also
need to update the DT parsing of the child-nodes in the mmc core.
Documentation/devicetree/bindings/mmc/mmc.txt
Documentation/devicetree/bindings/mmc/mmc-card.txt
>
>>
>> It is unlike the implementation which put multiple slots behind PCIe EP interface. sdhci-pci.c will handle each slot init one by one.
>> If Xenon SDHC slots are represented as child nodes, there should also be a main entry in Xenon driver to init each child node one by one.
>> In my very own opinion, it is inconvenient and unnecessary.
>
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller
From: Arnd Bergmann @ 2016-11-24 9:11 UTC (permalink / raw)
To: Ulf Hansson
Cc: Gregory CLEMENT, Rob Herring, Ziji Hu, Adrian Hunter,
linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Jason Cooper,
Andrew Lunn, Sebastian Hesselbarth,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Thomas Petazzoni,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Jack(SH) Zhu, Jimmy Xu, Jisheng Zhang, Nadav Haklai, Ryan Gao,
Doug Jones, Shiwu Zhang, Vi
In-Reply-To: <CAPDyKFpoifsKkse7Fc-bbZAoa=QGT=9QOQ-4D=f60ptx0hzZsA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Thursday, November 24, 2016 10:05:45 AM CET Ulf Hansson wrote:
> > You also mentioned other bindings using child nodes, but for this one
> > we have one controller with only one set of register with multiple slots
> > (Atmel is an example). Here each slot have it own set of register.
> >
> > Actually giving the fact that each slot is controlled by a different set
> > of register I wonder why the hardware can't also deduce the slot number
> > from the address register. For me it looks like an hardware bug but we
> > have to deal with it.
> >
> > Do you still think we needchild node here?
>
> Using child-nodes for slots like what's done in the atmel case, is
> currently broken. I would recommend to avoid using child-nodes for
> slots, if possible.
>
> To give you some more background, currently the mmc core treats child
> nodes as embedded non-removable cards or SDIO funcs. However, we can
> change to make child-nodes also allowed to describe slots, but it
> requires a specific compatible for "slots" and of course then we also
> need to update the DT parsing of the child-nodes in the mmc core.
>
> Documentation/devicetree/bindings/mmc/mmc.txt
> Documentation/devicetree/bindings/mmc/mmc-card.txt
I don't see anything wrong with having child nodes for the slots
even with the current binding, under one condition:
The mmc.txt binding above must refer only to the child node, while
the parent node conceptually becomes a plain bus or MFD that
happens to encapsulate multiple MMC host controllers, and possibly
provides some shared registers to them.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller
From: Gregory CLEMENT @ 2016-11-24 9:22 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Ulf Hansson, Rob Herring, Ziji Hu, Adrian Hunter,
linux-mmc@vger.kernel.org, Jason Cooper, Andrew Lunn,
Sebastian Hesselbarth, devicetree@vger.kernel.org,
Thomas Petazzoni, linux-arm-kernel@lists.infradead.org,
Jack(SH) Zhu, Jimmy Xu, Jisheng Zhang, Nadav Haklai, Ryan Gao,
Doug Jones, Shiwu Zhang, Victor Gu <xi>
In-Reply-To: <4031579.CBE32NHUoW@wuerfel>
Hi Arnd,
On jeu., nov. 24 2016, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Thursday, November 24, 2016 10:05:45 AM CET Ulf Hansson wrote:
>> > You also mentioned other bindings using child nodes, but for this one
>> > we have one controller with only one set of register with multiple slots
>> > (Atmel is an example). Here each slot have it own set of register.
>> >
>> > Actually giving the fact that each slot is controlled by a different set
>> > of register I wonder why the hardware can't also deduce the slot number
>> > from the address register. For me it looks like an hardware bug but we
>> > have to deal with it.
>> >
>> > Do you still think we needchild node here?
>>
>> Using child-nodes for slots like what's done in the atmel case, is
>> currently broken. I would recommend to avoid using child-nodes for
>> slots, if possible.
>>
>> To give you some more background, currently the mmc core treats child
>> nodes as embedded non-removable cards or SDIO funcs. However, we can
>> change to make child-nodes also allowed to describe slots, but it
>> requires a specific compatible for "slots" and of course then we also
>> need to update the DT parsing of the child-nodes in the mmc core.
>>
>> Documentation/devicetree/bindings/mmc/mmc.txt
>> Documentation/devicetree/bindings/mmc/mmc-card.txt
>
> I don't see anything wrong with having child nodes for the slots
> even with the current binding, under one condition:
>
> The mmc.txt binding above must refer only to the child node, while
> the parent node conceptually becomes a plain bus or MFD that
> happens to encapsulate multiple MMC host controllers, and possibly
> provides some shared registers to them.
I don't have an option for mmc in general, but using child node do not
fit at all the xenon controller.
For this controller each slot has its own set of register, so there is
no common ressource to share so no advantage to use it. Using child node
in our case will just make the code more complex for no benefit.
Gregory
>
> Arnd
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller
From: Arnd Bergmann @ 2016-11-24 9:34 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Gregory CLEMENT, Jimmy Xu, Andrew Lunn, Ulf Hansson,
Romain Perier, Liuliu Zhao, Peng Zhu,
linux-kernel@vger.kernel.org, Nadav Haklai, Ziji Hu, Victor Gu,
Doug Jones, Jisheng Zhang, Yehuda Yitschak, Marcin Wojtas,
Xueping Liu, Hilbert Zhang, Shiwu Zhang, Yu Cao,
Sebastian Hesselbarth
In-Reply-To: <877f7tmduw.fsf@free-electrons.com>
On Thursday, November 24, 2016 10:22:31 AM CET Gregory CLEMENT wrote:
>
> I don't have an option for mmc in general, but using child node do not
> fit at all the xenon controller.
>
> For this controller each slot has its own set of register, so there is
> no common ressource to share so no advantage to use it. Using child node
> in our case will just make the code more complex for no benefit.
If every slot has its own registers, what is it that makes up the
'controller'? It sounds to me that you just have to adjust the terminology
and talk about multiple controllers then, with one slot per controller.
Arnd
^ permalink raw reply
* Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller
From: Thomas Petazzoni @ 2016-11-24 9:48 UTC (permalink / raw)
To: Gregory CLEMENT
Cc: Arnd Bergmann, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
Jimmy Xu, Andrew Lunn, Ulf Hansson, Romain Perier, Liuliu Zhao,
Peng Zhu, linux-kernel@vger.kernel.org, Nadav Haklai, Ziji Hu,
Victor Gu, Doug Jones, Jisheng Zhang, Yehuda Yitschak,
Marcin Wojtas, Xueping Liu, Hilbert Zhang, Shiwu Zhang, Yu Cao,
Sebastian Hesselbarth <sebastian.hesselbart>
In-Reply-To: <8737ihmctr.fsf-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Hello,
On Thu, 24 Nov 2016 10:44:48 +0100, Gregory CLEMENT wrote:
> "A single Xenon IP can support multiple slots.
> Each slot acts as an independent SDHC. It owns independent resources, such
> as register sets clock and PHY.
> Each slot should have an independent device tree node."
I think this wording is still very confusing, and continues to cause
confusion.
We should just state that each Xenon controller supports a single slot,
and that's it.
The text still says "a single Xenon IP can support multiple slots",
which continues to cause confusion.
Best regards,
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller
From: Marcin Wojtas @ 2016-11-24 9:49 UTC (permalink / raw)
To: Gregory CLEMENT
Cc: Arnd Bergmann, linux-arm-kernel@lists.infradead.org, Jimmy Xu,
Andrew Lunn, Ulf Hansson, Romain Perier, Liuliu Zhao, Peng Zhu,
linux-kernel@vger.kernel.org, Nadav Haklai, Ziji Hu, Victor Gu,
Doug Jones, Jisheng Zhang, Yehuda Yitschak, Xueping Liu,
Hilbert Zhang, Shiwu Zhang, Yu Cao,
Sebastian Hesselbarth <sebastian.he>
In-Reply-To: <8737ihmctr.fsf@free-electrons.com>
Hi Gregory,
2016-11-24 10:44 GMT+01:00 Gregory CLEMENT <gregory.clement@free-electrons.com>:
> Hi Arnd,
>
> On jeu., nov. 24 2016, Arnd Bergmann <arnd@arndb.de> wrote:
>
>> On Thursday, November 24, 2016 10:22:31 AM CET Gregory CLEMENT wrote:
>>>
>>> I don't have an option for mmc in general, but using child node do not
>>> fit at all the xenon controller.
>>>
>>> For this controller each slot has its own set of register, so there is
>>> no common ressource to share so no advantage to use it. Using child node
>>> in our case will just make the code more complex for no benefit.
>>
>> If every slot has its own registers, what is it that makes up the
>> 'controller'? It sounds to me that you just have to adjust the terminology
>> and talk about multiple controllers then, with one slot per controller.
>>
>
> I agree and actually there were some words about in at the begining of
> the binding:
>
> "A single Xenon IP can support multiple slots.
> Each slot acts as an independent SDHC. It owns independent resources, such
> as register sets clock and PHY.
> Each slot should have an independent device tree node."
>
> All the confusion came from the fact that we still need to identify a
> slot ID. For an obscure reason the hardware can't guess the slot ID from
> the address register."
>
How about to avoid confusion, by simply renaming this number to
port-id/xenon-id or anything else but slot? I guess this may allow to
avoid some misunderstandings.
Best regards,
Marcin
^ permalink raw reply
* Re: [PATCH v2] mmc: block: delete packed command support
From: Linus Walleij @ 2016-11-24 9:50 UTC (permalink / raw)
To: Jaehoon Chung
Cc: linux-mmc@vger.kernel.org, Ulf Hansson, Chunyan Zhang,
Baolin Wang, Namjae Jeon, Maya Erez
In-Reply-To: <0e12314b-e1ce-d2ce-1136-02465fdbead4@samsung.com>
On Thu, Nov 24, 2016 at 6:24 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>> Cc: Namjae Jeon <namjae.jeon@samsung.com>
>> Cc: Maya Erez <qca_merez@qca.qualcomm.com>
>> Acked-by: Jaehoon Chung <jh80.chung@samsung.com>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>
> Did i send the acked-by tag? :)
> I just agreed some this patch's points.
> When i become assured, i will send the acked-by tag. is this way right?
As per Documentation/SubmittingPatches, section 12:
--------------------------
Acked-by: is not as formal as Signed-off-by:. It is a record that the acker
has at least reviewed the patch and has indicated acceptance. Hence patch
mergers will sometimes manually convert an acker's "yep, looks good to me"
into an Acked-by: (but note that it is usually better to ask for an
explicit ack).
---------------------------
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v2] mmc: block: delete packed command support
From: Linus Walleij @ 2016-11-24 9:53 UTC (permalink / raw)
To: kbuild test robot
Cc: kbuild-all@01.org, linux-mmc@vger.kernel.org, Ulf Hansson,
Chunyan Zhang, Baolin Wang, Namjae Jeon, Maya Erez
In-Reply-To: <201611241455.f15P3Xee%fengguang.wu@intel.com>
On Thu, Nov 24, 2016 at 7:35 AM, kbuild test robot <lkp@intel.com> wrote:
> Hi Linus,
>
> [auto build test WARNING on ulf.hansson-mmc/next]
> [cannot apply to linus/master linux/master v4.9-rc6 next-20161123]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
This is indeed a false build error.
Patches sent to linux-mmc@vger.kernel.org is probably better tested agains
the "next" branch on Ulf's MMC tree at:
https://git.kernel.org/cgit/linux/kernel/git/ulfh/mmc.git/
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC
From: Arnd Bergmann @ 2016-11-24 9:56 UTC (permalink / raw)
To: Gregory CLEMENT
Cc: Ulf Hansson, Adrian Hunter, linux-mmc-u79uwXL29TY76Z2rM5mHXA,
Jason Cooper, Andrew Lunn, Sebastian Hesselbarth, Rob Herring,
devicetree-u79uwXL29TY76Z2rM5mHXA, Thomas Petazzoni,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Ziji Hu,
Jack(SH) Zhu, Jimmy Xu, Jisheng Zhang, Nadav Haklai, Ryan Gao,
Doug Jones, Shiwu Zhang, Victor Gu, Wei(SOCP) Liu, Wilson Ding,
Xueping Liu <xpli>
In-Reply-To: <a05ffd140f4edc02fc3128db8445b2264cf38723.1477911954.git-series.gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
On Monday, October 31, 2016 12:09:56 PM CET Gregory CLEMENT wrote:
> From: Ziji Hu <huziji-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
>
> Marvell Xenon eMMC/SD/SDIO Host Controller contains PHY.
> Three types of PHYs are supported.
>
> Add support to multiple types of PHYs init and configuration.
> Add register definitions of PHYs.
>
> Signed-off-by: Hu Ziji <huziji-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>
Please explain in the changelog why this is not a generic
phy driver (or three of them).
Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller
From: Arnd Bergmann @ 2016-11-24 10:04 UTC (permalink / raw)
To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
Cc: Thomas Petazzoni, Gregory CLEMENT, Jimmy Xu, Andrew Lunn,
Ulf Hansson, Romain Perier, Liuliu Zhao, Peng Zhu, Adrian Hunter,
Nadav Haklai, Ziji Hu, Victor Gu, Doug Jones, Jisheng Zhang,
Yehuda Yitschak, Wei(SOCP) Liu, Xueping Liu, Hilbert Zhang,
Shiwu Zhang, Yu Cao, Sebastian
In-Reply-To: <20161124104858.3604c11d-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
On Thursday, November 24, 2016 10:48:58 AM CET Thomas Petazzoni wrote:
> Hello,
>
> On Thu, 24 Nov 2016 10:44:48 +0100, Gregory CLEMENT wrote:
>
> > "A single Xenon IP can support multiple slots.
> > Each slot acts as an independent SDHC. It owns independent resources, such
> > as register sets clock and PHY.
> > Each slot should have an independent device tree node."
>
> I think this wording is still very confusing, and continues to cause
> confusion.
>
> We should just state that each Xenon controller supports a single slot,
> and that's it.
>
> The text still says "a single Xenon IP can support multiple slots",
> which continues to cause confusion.
Agreed. Ideally we'd find out why exactly the slot number must
be used for accessing some of the registers to have a better
explanation to put in there, aside from stating that only one
slot is supported but the number must be set.
Could it be that this is some form of pinmuxing, i.e. that each
controller could in theory be used for any of the slots but you
have to pick one of them?
Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller
From: Thomas Petazzoni @ 2016-11-24 10:10 UTC (permalink / raw)
To: Marcin Wojtas
Cc: Gregory CLEMENT, Arnd Bergmann,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Jimmy Xu, Andrew Lunn, Ulf Hansson, Romain Perier, Liuliu Zhao,
Peng Zhu, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Nadav Haklai, Ziji Hu, Victor Gu, Doug Jones, Jisheng Zhang,
Yehuda Yitschak, Xueping Liu, Hilbert Zhang, Shiwu Zhang
In-Reply-To: <CAPv3WKddPHgpRU2_tVoDF=5Z-nqfFPxjgJ-+z9o-1tR2=fFvAA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Hello,
On Thu, 24 Nov 2016 10:49:23 +0100, Marcin Wojtas wrote:
> How about to avoid confusion, by simply renaming this number to
> port-id/xenon-id or anything else but slot? I guess this may allow to
> avoid some misunderstandings.
Agreed.
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2] mmc: block: delete packed command support
From: Jaehoon Chung @ 2016-11-24 10:23 UTC (permalink / raw)
To: Linus Walleij
Cc: linux-mmc@vger.kernel.org, Ulf Hansson, Chunyan Zhang,
Baolin Wang, Namjae Jeon, Maya Erez
In-Reply-To: <CACRpkdZqL0b9i0r+TwRVYD-uw5PuoNbOsxKoghtHam+Wawp1Rg@mail.gmail.com>
On 11/24/2016 06:50 PM, Linus Walleij wrote:
> On Thu, Nov 24, 2016 at 6:24 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>
>>> Cc: Namjae Jeon <namjae.jeon@samsung.com>
>>> Cc: Maya Erez <qca_merez@qca.qualcomm.com>
>>> Acked-by: Jaehoon Chung <jh80.chung@samsung.com>
>>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>>
>> Did i send the acked-by tag? :)
>> I just agreed some this patch's points.
>> When i become assured, i will send the acked-by tag. is this way right?
>
> As per Documentation/SubmittingPatches, section 12:
>
> --------------------------
> Acked-by: is not as formal as Signed-off-by:. It is a record that the acker
> has at least reviewed the patch and has indicated acceptance. Hence patch
> mergers will sometimes manually convert an acker's "yep, looks good to me"
> into an Acked-by: (but note that it is usually better to ask for an
> explicit ack).
> ---------------------------
Thanks for sharing this information..i didn't read this documentation.
Anyway, until today, i have checked the I/O performance with Exynos3/4/5 boards which i have.
It seems that there is no I/O performance benefit about packed command at this time.
I don't know how other guys are thinking.
Explicit..
Acked-by: Jaehoon Chung <jh80.chung@samsung.com>
Best Regards,
Jaehoon Chung
>
> Yours,
> Linus Walleij
>
>
>
^ permalink raw reply
* Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller
From: Ziji Hu @ 2016-11-24 10:38 UTC (permalink / raw)
To: Thomas Petazzoni, Marcin Wojtas
Cc: Gregory CLEMENT, Arnd Bergmann,
linux-arm-kernel@lists.infradead.org, Jimmy Xu, Andrew Lunn,
Ulf Hansson, Romain Perier, Liuliu Zhao, Peng Zhu,
linux-kernel@vger.kernel.org, Nadav Haklai, Victor Gu, Doug Jones,
Jisheng Zhang, Yehuda Yitschak, Xueping Liu, Hilbert Zhang,
Shiwu Zhang, Yu Cao, Sebastian
In-Reply-To: <20161124111029.035553ce@free-electrons.com>
Hi all,
On 2016/11/24 18:10, Thomas Petazzoni wrote:
> Hello,
>
> On Thu, 24 Nov 2016 10:49:23 +0100, Marcin Wojtas wrote:
>
>> How about to avoid confusion, by simply renaming this number to
>> port-id/xenon-id or anything else but slot? I guess this may allow to
>> avoid some misunderstandings.
>
We borrow the term "slot" from PCIe interface from SD spec.
According to Appendix C in SD spec 3.0, slot means an independent set of register from the view of SW.
I can avoid using "slot" and replace "slot index" with "sdhc-id".
Thanks for the suggestions.
Thank you.
Best regards,
Hu Ziji
> Agreed.
>
> Thomas
>
^ permalink raw reply
* Re: [PATCH 6/10] mmc: sdhci-xenon: Add Marvell Xenon SDHC core functionality
From: Ulf Hansson @ 2016-11-24 10:43 UTC (permalink / raw)
To: Gregory CLEMENT
Cc: Adrian Hunter, linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Jason Cooper, Andrew Lunn, Sebastian Hesselbarth, Rob Herring,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Thomas Petazzoni,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Ziji Hu, Jack(SH) Zhu, Jimmy Xu, Jisheng Zhang, Nadav Haklai,
Ryan Gao, Doug Jones, Shiwu Zhang, Victor Gu, Wei(SOCP)
In-Reply-To: <0390e7a05b6163deabb545f93729ea615eeaaee2.1477911954.git-series.gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
On 31 October 2016 at 12:09, Gregory CLEMENT
<gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> From: Ziji Hu <huziji-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
>
> Add Xenon eMMC/SD/SDIO host controller core functionality.
> Add Xenon specific intialization process.
> Add Xenon specific mmc_host_ops APIs.
> Add Xenon specific register definitions.
>
> Add CONFIG_MMC_SDHCI_XENON support in drivers/mmc/host/Kconfig.
>
> Marvell Xenon SDHC conforms to SD Physical Layer Specification
> Version 3.01 and is designed according to the guidelines provided
> in the SD Host Controller Standard Specification Version 3.00.
>
> Signed-off-by: Hu Ziji <huziji-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
> MAINTAINERS | 1 +-
> drivers/mmc/host/Kconfig | 9 +-
> drivers/mmc/host/Makefile | 3 +-
> drivers/mmc/host/sdhci-xenon.c | 594 ++++++++++++++++++++++++++++++++++-
> drivers/mmc/host/sdhci-xenon.h | 142 ++++++++-
> 5 files changed, 749 insertions(+), 0 deletions(-)
> create mode 100644 drivers/mmc/host/sdhci-xenon.c
> create mode 100644 drivers/mmc/host/sdhci-xenon.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 850a0afb0c8d..d92f4175574b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7608,6 +7608,7 @@ MARVELL XENON MMC/SD/SDIO HOST CONTROLLER DRIVER
> M: Ziji Hu <huziji-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> L: linux-mmc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> S: Supported
> +F: drivers/mmc/host/sdhci-xenon.*
> F: Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.txt
>
> MATROX FRAMEBUFFER DRIVER
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 5274f503a39a..85a53623526a 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -798,3 +798,12 @@ config MMC_SDHCI_BRCMSTB
> Broadcom STB SoCs.
>
> If unsure, say Y.
> +
> +config MMC_SDHCI_XENON
> + tristate "Marvell Xenon eMMC/SD/SDIO SDHCI driver"
> + depends on MMC_SDHCI && MMC_SDHCI_PLTFM
> + help
> + This selects Marvell Xenon eMMC/SD/SDIO SDHCI.
> + If you have a machine with integrated Marvell Xenon SDHC IP,
> + say Y or M here.
> + If unsure, say N.
> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> index e2bdaaf43184..75eaf743486c 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -80,3 +80,6 @@ obj-$(CONFIG_MMC_SDHCI_BRCMSTB) += sdhci-brcmstb.o
> ifeq ($(CONFIG_CB710_DEBUG),y)
> CFLAGS-cb710-mmc += -DDEBUG
> endif
> +
> +obj-$(CONFIG_MMC_SDHCI_XENON) += sdhci-xenon-driver.o
> +sdhci-xenon-driver-y += sdhci-xenon.o
> diff --git a/drivers/mmc/host/sdhci-xenon.c b/drivers/mmc/host/sdhci-xenon.c
> new file mode 100644
> index 000000000000..3ea059f2aaab
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-xenon.c
> @@ -0,0 +1,594 @@
> +/*
> + * Driver for Marvell SOC Platform Group Xenon SDHC as a platform device
> + *
> + * Copyright (C) 2016 Marvell, All Rights Reserved.
> + *
> + * Author: Hu Ziji <huziji-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> + * Date: 2016-8-24
> + *
> + * 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 version 2.
> + *
> + * Inspired by Jisheng Zhang <jszhang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> + * Special thanks to Video BG4 project team.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/mmc/host.h>
> +#include <linux/mmc/mmc.h>
> +#include <linux/mmc/sdio.h>
> +#include <linux/mmc/card.h>
> +#include <linux/mmc/host.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +
> +#include "sdhci-pltfm.h"
> +#include "sdhci.h"
> +#include "sdhci-xenon.h"
> +
> +/* Set SDCLK-off-while-idle */
> +static void xenon_set_sdclk_off_idle(struct sdhci_host *host,
> + unsigned char slot_idx, bool enable)
> +{
> + u32 reg;
> + u32 mask;
> +
> + reg = sdhci_readl(host, SDHC_SYS_OP_CTRL);
> + /* Get the bit shift basing on the slot index */
> + mask = (0x1 << (SDCLK_IDLEOFF_ENABLE_SHIFT + slot_idx));
> + if (enable)
> + reg |= mask;
> + else
> + reg &= ~mask;
> +
> + sdhci_writel(host, reg, SDHC_SYS_OP_CTRL);
> +}
> +
> +/* Enable/Disable the Auto Clock Gating function */
> +static void xenon_set_acg(struct sdhci_host *host, bool enable)
> +{
> + u32 reg;
> +
> + reg = sdhci_readl(host, SDHC_SYS_OP_CTRL);
> + if (enable)
> + reg &= ~AUTO_CLKGATE_DISABLE_MASK;
> + else
> + reg |= AUTO_CLKGATE_DISABLE_MASK;
> + sdhci_writel(host, reg, SDHC_SYS_OP_CTRL);
> +}
> +
> +/* Enable this slot */
> +static void xenon_enable_slot(struct sdhci_host *host,
> + unsigned char slot_idx)
> +{
> + u32 reg;
> +
> + reg = sdhci_readl(host, SDHC_SYS_OP_CTRL);
> + reg |= (BIT(slot_idx) << SLOT_ENABLE_SHIFT);
> + sdhci_writel(host, reg, SDHC_SYS_OP_CTRL);
> +
> + /*
> + * Manually set the flag which all the slots require,
> + * including SD, eMMC, SDIO
> + */
> + host->mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY;
> +}
> +
> +/* Disable this slot */
> +static void xenon_disable_slot(struct sdhci_host *host,
> + unsigned char slot_idx)
> +{
> + u32 reg;
> +
> + reg = sdhci_readl(host, SDHC_SYS_OP_CTRL);
> + reg &= ~(BIT(slot_idx) << SLOT_ENABLE_SHIFT);
> + sdhci_writel(host, reg, SDHC_SYS_OP_CTRL);
> +}
> +
> +/* Enable Parallel Transfer Mode */
> +static void xenon_enable_slot_parallel_tran(struct sdhci_host *host,
> + unsigned char slot_idx)
> +{
> + u32 reg;
> +
> + reg = sdhci_readl(host, SDHC_SYS_EXT_OP_CTRL);
> + reg |= BIT(slot_idx);
> + sdhci_writel(host, reg, SDHC_SYS_EXT_OP_CTRL);
> +}
> +
> +static void xenon_slot_tuning_setup(struct sdhci_host *host)
> +{
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
> + u32 reg;
> +
> + /* Disable the Re-Tuning Request functionality */
> + reg = sdhci_readl(host, SDHC_SLOT_RETUNING_REQ_CTRL);
> + reg &= ~RETUNING_COMPATIBLE;
> + sdhci_writel(host, reg, SDHC_SLOT_RETUNING_REQ_CTRL);
> +
> + /* Disable the Re-tuning Event Signal Enable */
> + reg = sdhci_readl(host, SDHCI_SIGNAL_ENABLE);
> + reg &= ~SDHCI_INT_RETUNE;
> + sdhci_writel(host, reg, SDHCI_SIGNAL_ENABLE);
> +
> + /* Force to use Tuning Mode 1 */
> + host->tuning_mode = SDHCI_TUNING_MODE_1;
> + /* Set re-tuning period */
> + host->tuning_count = 1 << (priv->tuning_count - 1);
> +}
> +
> +/*
> + * Operations inside struct sdhci_ops
> + */
> +/* Recover the Register Setting cleared during SOFTWARE_RESET_ALL */
> +static void sdhci_xenon_reset_exit(struct sdhci_host *host,
> + unsigned char slot_idx, u8 mask)
> +{
> + /* Only SOFTWARE RESET ALL will clear the register setting */
> + if (!(mask & SDHCI_RESET_ALL))
> + return;
> +
> + /* Disable tuning request and auto-retuning again */
> + xenon_slot_tuning_setup(host);
> +
> + xenon_set_acg(host, true);
> +
> + xenon_set_sdclk_off_idle(host, slot_idx, false);
> +}
> +
> +static void sdhci_xenon_reset(struct sdhci_host *host, u8 mask)
> +{
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +
> + sdhci_reset(host, mask);
> + sdhci_xenon_reset_exit(host, priv->slot_idx, mask);
> +}
> +
> +/*
> + * Xenon defines different values for HS200 and SDR104
> + * in Host_Control_2
> + */
> +static void xenon_set_uhs_signaling(struct sdhci_host *host,
> + unsigned int timing)
> +{
> + u16 ctrl_2;
> +
> + ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> + /* Select Bus Speed Mode for host */
> + ctrl_2 &= ~SDHCI_CTRL_UHS_MASK;
> + if (timing == MMC_TIMING_MMC_HS200)
> + ctrl_2 |= XENON_SDHCI_CTRL_HS200;
> + else if (timing == MMC_TIMING_UHS_SDR104)
> + ctrl_2 |= SDHCI_CTRL_UHS_SDR104;
> + else if (timing == MMC_TIMING_UHS_SDR12)
> + ctrl_2 |= SDHCI_CTRL_UHS_SDR12;
> + else if (timing == MMC_TIMING_UHS_SDR25)
> + ctrl_2 |= SDHCI_CTRL_UHS_SDR25;
> + else if (timing == MMC_TIMING_UHS_SDR50)
> + ctrl_2 |= SDHCI_CTRL_UHS_SDR50;
> + else if ((timing == MMC_TIMING_UHS_DDR50) ||
> + (timing == MMC_TIMING_MMC_DDR52))
> + ctrl_2 |= SDHCI_CTRL_UHS_DDR50;
> + else if (timing == MMC_TIMING_MMC_HS400)
> + ctrl_2 |= XENON_SDHCI_CTRL_HS400;
> + sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
> +}
> +
> +static const struct sdhci_ops sdhci_xenon_ops = {
> + .set_clock = sdhci_set_clock,
> + .set_bus_width = sdhci_set_bus_width,
> + .reset = sdhci_xenon_reset,
> + .set_uhs_signaling = xenon_set_uhs_signaling,
> + .get_max_clock = sdhci_pltfm_clk_get_max_clock,
> +};
> +
> +static const struct sdhci_pltfm_data sdhci_xenon_pdata = {
> + .ops = &sdhci_xenon_ops,
> + .quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC |
> + SDHCI_QUIRK_NO_SIMULT_VDD_AND_POWER |
> + SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
> +};
> +
> +/*
> + * Xenon Specific Operations in mmc_host_ops
> + */
> +static void xenon_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> +{
> + struct sdhci_host *host = mmc_priv(mmc);
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
> + unsigned long flags;
> + u32 reg;
> +
> + /*
> + * HS400/HS200/eMMC HS doesn't have Preset Value register.
> + * However, sdhci_set_ios will read HS400/HS200 Preset register.
> + * Disable Preset Value register for HS400/HS200.
> + * eMMC HS with preset_enabled set will trigger a bug in
> + * get_preset_value().
> + */
> + spin_lock_irqsave(&host->lock, flags);
> + if ((ios->timing == MMC_TIMING_MMC_HS400) ||
> + (ios->timing == MMC_TIMING_MMC_HS200) ||
> + (ios->timing == MMC_TIMING_MMC_HS)) {
> + host->preset_enabled = false;
> + host->quirks2 |= SDHCI_QUIRK2_PRESET_VALUE_BROKEN;
> +
> + reg = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> + reg &= ~SDHCI_CTRL_PRESET_VAL_ENABLE;
> + sdhci_writew(host, reg, SDHCI_HOST_CONTROL2);
> + } else {
> + host->quirks2 &= ~SDHCI_QUIRK2_PRESET_VALUE_BROKEN;
> + }
> + spin_unlock_irqrestore(&host->lock, flags);
> +
> + sdhci_set_ios(mmc, ios);
> +
> + if (host->clock > DEFAULT_SDCLK_FREQ) {
> + spin_lock_irqsave(&host->lock, flags);
> + xenon_set_sdclk_off_idle(host, priv->slot_idx, true);
> + spin_unlock_irqrestore(&host->lock, flags);
> + }
> +}
> +
> +static int __emmc_signal_voltage_switch(struct mmc_host *mmc,
> + const unsigned char signal_voltage)
> +{
> + u32 ctrl;
> + unsigned char voltage_code;
> + struct sdhci_host *host = mmc_priv(mmc);
> +
> + if (signal_voltage == MMC_SIGNAL_VOLTAGE_330)
> + voltage_code = EMMC_VCCQ_3_3V;
> + else if (signal_voltage == MMC_SIGNAL_VOLTAGE_180)
> + voltage_code = EMMC_VCCQ_1_8V;
> + else
> + return -EINVAL;
> +
> + /*
> + * This host is for eMMC, XENON self-defined
> + * eMMC slot control register should be accessed
> + * instead of Host Control 2
> + */
> + ctrl = sdhci_readl(host, SDHC_SLOT_EMMC_CTRL);
> + ctrl &= ~EMMC_VCCQ_MASK;
> + ctrl |= voltage_code;
> + sdhci_writel(host, ctrl, SDHC_SLOT_EMMC_CTRL);
> +
> + /* There is no standard to determine this waiting period */
> + usleep_range(1000, 2000);
> +
> + /* Check whether io voltage switch is done */
> + ctrl = sdhci_readl(host, SDHC_SLOT_EMMC_CTRL);
> + ctrl &= EMMC_VCCQ_MASK;
> + /*
> + * This bit is set only when regulator feeds back the voltage switch
> + * results to Xenon SDHC.
> + * However, in actaul implementation, regulator might not provide
> + * this feedback.
> + * Thus we shall not rely on this bit to determine if switch failed.
> + * If the bit is not set, just throw a message.
> + * Besides, error code should not be returned.
> + */
> + if (ctrl != voltage_code)
> + dev_info(mmc_dev(mmc), "fail to detect eMMC signal voltage stable\n");
> + return 0;
> +}
> +
> +static int xenon_emmc_signal_voltage_switch(struct mmc_host *mmc,
> + struct mmc_ios *ios)
> +{
> + unsigned char voltage = ios->signal_voltage;
> +
> + if ((voltage == MMC_SIGNAL_VOLTAGE_330) ||
> + (voltage == MMC_SIGNAL_VOLTAGE_180))
> + return __emmc_signal_voltage_switch(mmc, voltage);
> +
> + dev_err(mmc_dev(mmc), "Unsupported signal voltage: %d\n",
> + voltage);
> + return -EINVAL;
This wrapper function seems unnessarry. It only adds a dev_err(), so
then might as well do that in __emmc_signal_voltage_switch().
> +}
> +
> +static int xenon_start_signal_voltage_switch(struct mmc_host *mmc,
> + struct mmc_ios *ios)
> +{
> + struct sdhci_host *host = mmc_priv(mmc);
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +
> + /*
> + * Before SD/SDIO set signal voltage, SD bus clock should be
> + * disabled. However, sdhci_set_clock will also disable the Internal
> + * clock in mmc_set_signal_voltage().
If that's the case then that is wrong in the generic sdhci code.
What's the reason why it can't be fixed there instead of having this
workaround?
> + * If Internal clock is disabled, the 3.3V/1.8V bit can not be updated.
> + * Thus here manually enable internal clock.
> + *
> + * After switch completes, it is unnecessary to disable internal clock,
> + * since keeping internal clock active obeys SD spec.
> + */
> + enable_xenon_internal_clk(host);
> +
> + if (priv->emmc_slot)
> + return xenon_emmc_signal_voltage_switch(mmc, ios);
> +
> + return sdhci_start_signal_voltage_switch(mmc, ios);
> +}
> +
> +/*
> + * After determining which slot is used for SDIO,
> + * some additional task is required.
> + */
> +static void xenon_init_card(struct mmc_host *mmc, struct mmc_card *card)
> +{
> + struct sdhci_host *host = mmc_priv(mmc);
> + u32 reg;
> + u8 slot_idx;
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +
> + /* Link the card for delay adjustment */
> + priv->card_candidate = card;
> + /* Set tuning functionality of this slot */
> + xenon_slot_tuning_setup(host);
This looks weird. I assume this can be done as a part of the regular
tuning seqeunce!?
> +
> + slot_idx = priv->slot_idx;
> + if (!mmc_card_sdio(card)) {
> + /* Clear SDIO Card Inserted indication */
Why do you need this?
If you need to reset this, I think it's better to do it from
->set_ios() at MMC_POWER_OFF.
> + reg = sdhci_readl(host, SDHC_SYS_CFG_INFO);
> + reg &= ~(1 << (slot_idx + SLOT_TYPE_SDIO_SHIFT));
> + sdhci_writel(host, reg, SDHC_SYS_CFG_INFO);
> +
> + if (mmc_card_mmc(card)) {
> + mmc->caps |= MMC_CAP_NONREMOVABLE;
> + if (!(host->quirks2 & SDHCI_QUIRK2_NO_1_8_V))
> + mmc->caps |= MMC_CAP_1_8V_DDR;
> + /*
> + * Force to clear BUS_TEST to
> + * skip bus_test_pre and bus_test_post
> + */
> + mmc->caps &= ~MMC_CAP_BUS_WIDTH_TEST;
> + mmc->caps2 |= MMC_CAP2_HC_ERASE_SZ |
> + MMC_CAP2_PACKED_CMD;
> + if (mmc->caps & MMC_CAP_8_BIT_DATA)
> + mmc->caps2 |= MMC_CAP2_HS400_1_8V;
Most of this can be specified as DT configurations. Please use that instead.
More importantly, please don't use the ->init_card() ops to assign
host caps. If not DT, please do it from ->probe().
> + }
> + } else {
> + /*
> + * Set SDIO Card Inserted indication
> + * to inform that the current slot is for SDIO
> + */
> + reg = sdhci_readl(host, SDHC_SYS_CFG_INFO);
> + reg |= (1 << (slot_idx + SLOT_TYPE_SDIO_SHIFT));
> + sdhci_writel(host, reg, SDHC_SYS_CFG_INFO);
So this makes sence to have in the ->init_card() ops. The rest above, not.
> + }
> +}
> +
> +static int xenon_execute_tuning(struct mmc_host *mmc, u32 opcode)
> +{
> + struct sdhci_host *host = mmc_priv(mmc);
> +
> + if (host->timing == MMC_TIMING_UHS_DDR50)
> + return 0;
> +
> + return sdhci_execute_tuning(mmc, opcode);
> +}
> +
> +static void xenon_replace_mmc_host_ops(struct sdhci_host *host)
> +{
> + host->mmc_host_ops.set_ios = xenon_set_ios;
> + host->mmc_host_ops.start_signal_voltage_switch =
> + xenon_start_signal_voltage_switch;
> + host->mmc_host_ops.init_card = xenon_init_card;
> + host->mmc_host_ops.execute_tuning = xenon_execute_tuning;
> +}
> +
> +static int xenon_probe_dt(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct sdhci_host *host = platform_get_drvdata(pdev);
> + struct mmc_host *mmc = host->mmc;
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
> + int err;
> + u32 slot_idx, nr_slot;
> + u32 tuning_count;
> + u32 reg;
> +
> + /* Standard MMC property */
> + err = mmc_of_parse(mmc);
> + if (err)
> + return err;
> +
> + /* Standard SDHCI property */
> + sdhci_get_of_property(pdev);
> +
> + /*
> + * Xenon Specific property:
> + * emmc: explicitly indicate whether this slot is for eMMC
> + * slotno: the index of slot. Refer to SDHC_SYS_CFG_INFO register
> + * tun-count: the interval between re-tuning
> + * PHY type: "sdhc phy", "emmc phy 5.0" or "emmc phy 5.1"
> + */
> + if (of_property_read_bool(np, "marvell,xenon-emmc"))
> + priv->emmc_slot = true;
So, you need this because of the eMMC voltage switch behaviour, right?
Then I would rather like to describe this a generic DT bindings for
the eMMC voltage level support. There have acutally been some earlier
discussions for this, but we haven't yet made some changes.
I think what is missing is a mmc-ddr-3_3v DT binding, which when set,
allows the host driver to accept I/O voltage switches to 3.3V. If not
supported the ->start_signal_voltage_switch() ops may return -EINVAL.
This would inform the mmc core to move on to the next supported
voltage level. There might be some minor additional changes to the mmc
card initialization sequence, but those should be simple.
I can help out to look into this, unless you want to do it yourself of course!?
> + else
> + priv->emmc_slot = false;
> +
> + if (!of_property_read_u32(np, "marvell,xenon-slotno", &slot_idx)) {
> + nr_slot = sdhci_readl(host, SDHC_SYS_CFG_INFO);
> + nr_slot &= NR_SUPPORTED_SLOT_MASK;
> + if (unlikely(slot_idx > nr_slot)) {
> + dev_err(mmc_dev(mmc), "Slot Index %d exceeds Number of slots %d\n",
> + slot_idx, nr_slot);
> + return -EINVAL;
> + }
> + } else {
> + priv->slot_idx = 0x0;
> + }
> +
> + if (!of_property_read_u32(np, "marvell,xenon-tun-count",
> + &tuning_count)) {
> + if (unlikely(tuning_count >= TMR_RETUN_NO_PRESENT)) {
> + dev_err(mmc_dev(mmc), "Wrong Re-tuning Count. Set default value %d\n",
> + DEF_TUNING_COUNT);
> + tuning_count = DEF_TUNING_COUNT;
> + }
> + } else {
> + priv->tuning_count = DEF_TUNING_COUNT;
> + }
To make the code a bit easier...
Maybe set "priv->tuning_count = DEF_TUNING_COUNT" before the "if", and
instead have the of_property_read_u32() to update the value when set.
> +
> + if (of_property_read_bool(np, "marvell,xenon-mask-conflict-err")) {
> + reg = sdhci_readl(host, SDHC_SYS_EXT_OP_CTRL);
> + reg |= MASK_CMD_CONFLICT_ERROR;
> + sdhci_writel(host, reg, SDHC_SYS_EXT_OP_CTRL);
> + }
> +
> + return err;
> +}
> +
> +static int xenon_slot_probe(struct sdhci_host *host)
> +{
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
> + u8 slot_idx = priv->slot_idx;
> +
> + /* Enable slot */
> + xenon_enable_slot(host, slot_idx);
> +
> + /* Enable ACG */
> + xenon_set_acg(host, true);
> +
> + /* Enable Parallel Transfer Mode */
> + xenon_enable_slot_parallel_tran(host, slot_idx);
> +
> + priv->timing = MMC_TIMING_FAKE;
> + priv->clock = 0;
What are these used for?
> +
> + return 0;
> +}
> +
> +static void xenon_slot_remove(struct sdhci_host *host)
> +{
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
> + u8 slot_idx = priv->slot_idx;
> +
> + /* disable slot */
> + xenon_disable_slot(host, slot_idx);
> +}
> +
> +static int sdhci_xenon_probe(struct platform_device *pdev)
> +{
> + struct sdhci_pltfm_host *pltfm_host;
> + struct sdhci_host *host;
> + struct clk *clk, *axi_clk;
> + struct sdhci_xenon_priv *priv;
> + int err;
> +
> + host = sdhci_pltfm_init(pdev, &sdhci_xenon_pdata,
> + sizeof(struct sdhci_xenon_priv));
> + if (IS_ERR(host))
> + return PTR_ERR(host);
> +
> + pltfm_host = sdhci_priv(host);
> + priv = sdhci_pltfm_priv(pltfm_host);
> +
> + xenon_set_acg(host, false);
> +
> + /*
> + * Link Xenon specific mmc_host_ops function,
> + * to replace standard ones in sdhci_ops.
> + */
> + xenon_replace_mmc_host_ops(host);
> +
> + clk = devm_clk_get(&pdev->dev, "core");
> + if (IS_ERR(clk)) {
> + dev_err(&pdev->dev, "Failed to setup input clk.\n");
> + err = PTR_ERR(clk);
> + goto free_pltfm;
> + }
> + clk_prepare_enable(clk);
Check error code.
> + pltfm_host->clk = clk;
Why not assign pltfm_host->clk immedately when doing devm_clk_get(),
that would make this a bit cleaner, right?
> +
> + /*
> + * Some SOCs require additional clock to
> + * manage AXI bus clock.
> + * It is optional.
> + */
> + axi_clk = devm_clk_get(&pdev->dev, "axi");
> + if (!IS_ERR(axi_clk)) {
> + clk_prepare_enable(axi_clk);
> + priv->axi_clk = axi_clk;
> + }
Same comments as for the above core clock.
> +
> + err = xenon_probe_dt(pdev);
> + if (err)
> + goto err_clk;
> +
> + err = xenon_slot_probe(host);
> + if (err)
> + goto err_clk;
> +
> + err = sdhci_add_host(host);
> + if (err)
> + goto remove_slot;
> +
> + return 0;
> +
> +remove_slot:
> + xenon_slot_remove(host);
> +err_clk:
> + clk_disable_unprepare(pltfm_host->clk);
> + if (!IS_ERR(axi_clk))
> + clk_disable_unprepare(axi_clk);
> +free_pltfm:
> + sdhci_pltfm_free(pdev);
> + return err;
> +}
> +
> +static int sdhci_xenon_remove(struct platform_device *pdev)
> +{
> + struct sdhci_host *host = platform_get_drvdata(pdev);
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
> + int dead = (readl(host->ioaddr + SDHCI_INT_STATUS) == 0xFFFFFFFF);
> +
> + xenon_slot_remove(host);
> +
> + sdhci_remove_host(host, dead);
> +
> + clk_disable_unprepare(pltfm_host->clk);
> + clk_disable_unprepare(priv->axi_clk);
> +
> + sdhci_pltfm_free(pdev);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id sdhci_xenon_dt_ids[] = {
> + { .compatible = "marvell,xenon-sdhci",},
> + { .compatible = "marvell,armada-3700-sdhci",},
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, sdhci_xenon_dt_ids);
> +
> +static struct platform_driver sdhci_xenon_driver = {
> + .driver = {
> + .name = "xenon-sdhci",
> + .of_match_table = sdhci_xenon_dt_ids,
> + .pm = &sdhci_pltfm_pmops,
> + },
> + .probe = sdhci_xenon_probe,
> + .remove = sdhci_xenon_remove,
> +};
> +
> +module_platform_driver(sdhci_xenon_driver);
> +
> +MODULE_DESCRIPTION("SDHCI platform driver for Marvell Xenon SDHC");
> +MODULE_AUTHOR("Hu Ziji <huziji-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/mmc/host/sdhci-xenon.h b/drivers/mmc/host/sdhci-xenon.h
> new file mode 100644
> index 000000000000..4601d0a4b22f
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-xenon.h
I don't think you need a specific header for this, let's instead just
put everthing in the c-file.
> @@ -0,0 +1,142 @@
> +/*
> + * Copyright (C) 2016 Marvell, All Rights Reserved.
> + *
> + * Author: Hu Ziji <huziji-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> + * Date: 2016-8-24
> + *
> + * 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 version 2.
> + */
> +#ifndef SDHCI_XENON_H_
> +#define SDHCI_XENON_H_
> +
> +#include <linux/clk.h>
> +#include <linux/mmc/card.h>
> +#include <linux/of.h>
> +#include "sdhci.h"
> +
> +/* Register Offset of SD Host Controller SOCP self-defined register */
> +#define SDHC_SYS_CFG_INFO 0x0104
> +#define SLOT_TYPE_SDIO_SHIFT 24
> +#define SLOT_TYPE_EMMC_MASK 0xFF
> +#define SLOT_TYPE_EMMC_SHIFT 16
> +#define SLOT_TYPE_SD_SDIO_MMC_MASK 0xFF
> +#define SLOT_TYPE_SD_SDIO_MMC_SHIFT 8
> +#define NR_SUPPORTED_SLOT_MASK 0x7
> +
> +#define SDHC_SYS_OP_CTRL 0x0108
> +#define AUTO_CLKGATE_DISABLE_MASK BIT(20)
> +#define SDCLK_IDLEOFF_ENABLE_SHIFT 8
> +#define SLOT_ENABLE_SHIFT 0
> +
> +#define SDHC_SYS_EXT_OP_CTRL 0x010C
> +#define MASK_CMD_CONFLICT_ERROR BIT(8)
> +
> +#define SDHC_SLOT_OP_STATUS_CTRL 0x0128
> +#define DELAY_90_DEGREE_MASK_EMMC5 BIT(7)
> +#define DELAY_90_DEGREE_SHIFT_EMMC5 7
> +#define EMMC_5_0_PHY_FIXED_DELAY_MASK 0x7F
> +#define EMMC_PHY_FIXED_DELAY_MASK 0xFF
> +#define EMMC_PHY_FIXED_DELAY_WINDOW_MIN (EMMC_PHY_FIXED_DELAY_MASK >> 3)
> +#define SDH_PHY_FIXED_DELAY_MASK 0x1FF
> +#define SDH_PHY_FIXED_DELAY_WINDOW_MIN (SDH_PHY_FIXED_DELAY_MASK >> 4)
> +
> +#define TUN_CONSECUTIVE_TIMES_SHIFT 16
> +#define TUN_CONSECUTIVE_TIMES_MASK 0x7
> +#define TUN_CONSECUTIVE_TIMES 0x4
> +#define TUNING_STEP_SHIFT 12
> +#define TUNING_STEP_MASK 0xF
> +#define TUNING_STEP_DIVIDER BIT(6)
> +
> +#define FORCE_SEL_INVERSE_CLK_SHIFT 11
> +
> +#define SDHC_SLOT_EMMC_CTRL 0x0130
> +#define ENABLE_DATA_STROBE BIT(24)
> +#define SET_EMMC_RSTN BIT(16)
> +#define DISABLE_RD_DATA_CRC BIT(14)
> +#define DISABLE_CRC_STAT_TOKEN BIT(13)
> +#define EMMC_VCCQ_MASK 0x3
> +#define EMMC_VCCQ_1_8V 0x1
> +#define EMMC_VCCQ_3_3V 0x3
> +
> +#define SDHC_SLOT_RETUNING_REQ_CTRL 0x0144
> +/* retuning compatible */
> +#define RETUNING_COMPATIBLE 0x1
> +
> +#define SDHC_SLOT_EXT_PRESENT_STATE 0x014C
> +#define LOCK_STATE 0x1
> +
> +#define SDHC_SLOT_DLL_CUR_DLY_VAL 0x0150
> +
> +/* Tuning Parameter */
> +#define TMR_RETUN_NO_PRESENT 0xF
> +#define DEF_TUNING_COUNT 0x9
> +
> +#define MMC_TIMING_FAKE 0xFF
> +
> +#define DEFAULT_SDCLK_FREQ (400000)
> +
> +/* Xenon specific Mode Select value */
> +#define XENON_SDHCI_CTRL_HS200 0x5
> +#define XENON_SDHCI_CTRL_HS400 0x6
For all defines above:
All these defines needs some *SDHCI* prefix. Can you please update that.
> +
> +struct sdhci_xenon_priv {
> + /*
> + * The bus_width, timing, and clock fields in below
> + * record the current setting of Xenon SDHC.
> + * Driver will call a Sampling Fixed Delay Adjustment
> + * if any setting is changed.
> + */
> + unsigned char bus_width;
> + unsigned char timing;
These two are not used. Please remove.
> + unsigned char tuning_count;
> + unsigned int clock;
"clock" isn't used, please remove.
> + struct clk *axi_clk;
> +
> + /* Slot idx */
> + u8 slot_idx;
> + /* Whether this slot is for eMMC */
> + bool emmc_slot;
> +
> + /*
> + * When initializing card, Xenon has to determine card type and
> + * adjust Sampling Fixed delay for the speed mode in which
> + * DLL tuning is not support.
> + * However, at that time, card structure is not linked to mmc_host.
> + * Thus a card pointer is added here to provide
> + * the delay adjustment function with the card structure
> + * of the card during initialization.
> + *
> + * It is only valid during initialization after it is updated in
> + * xenon_init_card().
> + * Do not access this variable in normal transfers after
> + * initialization completes.
> + */
> + struct mmc_card *card_candidate;
Not activley used in this change, please remove and let's discuss it
in the next step.
> +};
> +
> +static inline int enable_xenon_internal_clk(struct sdhci_host *host)
> +{
> + u32 reg;
> + u8 timeout;
> +
> + reg = sdhci_readl(host, SDHCI_CLOCK_CONTROL);
> + reg |= SDHCI_CLOCK_INT_EN;
> + sdhci_writel(host, reg, SDHCI_CLOCK_CONTROL);
> + /* Wait max 20 ms */
> + timeout = 20;
> + while (!((reg = sdhci_readw(host, SDHCI_CLOCK_CONTROL))
> + & SDHCI_CLOCK_INT_STABLE)) {
> + if (timeout == 0) {
> + pr_err("%s: Internal clock never stabilised.\n",
> + mmc_hostname(host->mmc));
> + return -ETIMEDOUT;
> + }
> + timeout--;
> + mdelay(1);
> + }
> +
> + return 0;
> +}
> +#endif
> --
> git-series 0.8.10
Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH mmc/next] mmc: sh_mobile_sdhi: remove support for sh7372
From: Simon Horman @ 2016-11-24 10:48 UTC (permalink / raw)
To: Wolfram Sang, Ulf Hansson
Cc: Magnus Damm, linux-mmc, linux-renesas-soc, Simon Horman
Remove documentation of support for the SH7372 (SH-Mobile AP4) from the MMC
driver. The driver itself appears to have no SH7372 specific code.
Commit edf4100906044225 ("ARM: shmobile: sh7372 dtsi: Remove Legacy file")
removes this SoC from the kernel in v4.1.
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
Documentation/devicetree/bindings/mmc/tmio_mmc.txt | 1 -
1 file changed, 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
index 13df9c2399c3..1db9e74bb9c1 100644
--- a/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
+++ b/Documentation/devicetree/bindings/mmc/tmio_mmc.txt
@@ -11,7 +11,6 @@ optional bindings can be used.
Required properties:
- compatible: "renesas,sdhi-shmobile" - a generic sh-mobile SDHI unit
- "renesas,sdhi-sh7372" - SDHI IP on SH7372 SoC
"renesas,sdhi-sh73a0" - SDHI IP on SH73A0 SoC
"renesas,sdhi-r8a73a4" - SDHI IP on R8A73A4 SoC
"renesas,sdhi-r8a7740" - SDHI IP on R8A7740 SoC
--
2.7.0.rc3.207.g0ac5344
^ permalink raw reply related
* Re: [PATCH mmc/next] mmc: sh_mobile_sdhi: remove support for sh7372
From: Geert Uytterhoeven @ 2016-11-24 10:53 UTC (permalink / raw)
To: Simon Horman
Cc: Wolfram Sang, Ulf Hansson, Magnus Damm, Linux MMC List,
Linux-Renesas
In-Reply-To: <1479984534-25468-1-git-send-email-horms+renesas@verge.net.au>
On Thu, Nov 24, 2016 at 11:48 AM, Simon Horman
<horms+renesas@verge.net.au> wrote:
> Remove documentation of support for the SH7372 (SH-Mobile AP4) from the MMC
> driver. The driver itself appears to have no SH7372 specific code.
>
> Commit edf4100906044225 ("ARM: shmobile: sh7372 dtsi: Remove Legacy file")
> removes this SoC from the kernel in v4.1.
>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC
From: Ziji Hu @ 2016-11-24 10:57 UTC (permalink / raw)
To: Arnd Bergmann, Gregory CLEMENT
Cc: Ulf Hansson, Adrian Hunter, linux-mmc, Jason Cooper, Andrew Lunn,
Sebastian Hesselbarth, Rob Herring, devicetree, Thomas Petazzoni,
linux-arm-kernel, Jack(SH) Zhu, Jimmy Xu, Jisheng Zhang,
Nadav Haklai, Ryan Gao, Doug Jones, Shiwu Zhang, Victor Gu,
Wei(SOCP) Liu, Wilson Ding, Xueping Liu
In-Reply-To: <2204525.IWIYQVjIXl@wuerfel>
Hi Arnd,
On 2016/11/24 17:56, Arnd Bergmann wrote:
> On Monday, October 31, 2016 12:09:56 PM CET Gregory CLEMENT wrote:
>> From: Ziji Hu <huziji@marvell.com>
>>
>> Marvell Xenon eMMC/SD/SDIO Host Controller contains PHY.
>> Three types of PHYs are supported.
>>
>> Add support to multiple types of PHYs init and configuration.
>> Add register definitions of PHYs.
>>
>> Signed-off-by: Hu Ziji <huziji@marvell.com>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>>
>
> Please explain in the changelog why this is not a generic
> phy driver (or three of them).
>
Actually we tried to put the PHY code into Linux PHY framework.
But it cannot fit in Linux common PHY framework.
Our Xenon SDHC PHY register is a part of Xenon SDHC register set.
Besides, during MMC initialization, MMC sequence has to call several PHY functions to complete timing setting.
In those PHY setting functions, they have to access SDHC register and know current MMC setting, such as bus width, clock frequency and speed mode.
As a result, we have to implement PHY under MMC directory.
Thank you.
Best regards,
Hu Ziji
> Arnd
>
^ permalink raw reply
* [PATCH 2/3] mmc: dw_mmc: add the debug message for polling and non-removable
From: Jaehoon Chung @ 2016-11-24 11:04 UTC (permalink / raw)
To: linux-mmc; +Cc: ulf.hansson, shawn.lin, Jaehoon Chung
In-Reply-To: <20161124110442.22058-1-jh80.chung@samsung.com>
If card is polling or non-removable, display the more exact message.
It's helpful to debug which detecting scheme is using.
Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
---
drivers/mmc/host/dw_mmc.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index c6cc618..d508225 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1525,9 +1525,23 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
int gpio_cd = mmc_gpio_get_cd(mmc);
/* Use platform get_cd function, else try onboard card detect */
- if ((mmc->caps & MMC_CAP_NEEDS_POLL) || !mmc_card_is_removable(mmc))
+ if (((mmc->caps & MMC_CAP_NEEDS_POLL)
+ || !mmc_card_is_removable(mmc))) {
present = 1;
- else if (gpio_cd >= 0)
+
+ if (!test_bit(DW_MMC_CARD_PRESENT, &slot->flags)) {
+ if (mmc->caps & MMC_CAP_NEEDS_POLL) {
+ dev_info(&mmc->class_dev,
+ "card is polling.\n");
+ } else {
+ dev_info(&mmc->class_dev,
+ "card is non-removable.\n");
+ }
+ set_bit(DW_MMC_CARD_PRESENT, &slot->flags);
+ }
+
+ return present;
+ } else if (gpio_cd >= 0)
present = gpio_cd;
else
present = (mci_readl(slot->host, CDETECT) & (1 << slot->id))
--
2.10.1
^ permalink raw reply related
* [PATCH 3/3] mmc: dw_mmc: display the clock message only one time when card is polling
From: Jaehoon Chung @ 2016-11-24 11:04 UTC (permalink / raw)
To: linux-mmc; +Cc: ulf.hansson, shawn.lin, Jaehoon Chung
In-Reply-To: <20161124110442.22058-1-jh80.chung@samsung.com>
When card is polling (broken-cd), there is a spamming messge related to
clock.
After applied this patch, display the message only one time at boot
time. It's enough to check which clock values is used.
Also prevent to display the spamming message.
Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
---
drivers/mmc/host/dw_mmc.c | 13 ++++++++++++-
drivers/mmc/host/dw_mmc.h | 1 +
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index d508225..dd7e95d 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1176,13 +1176,24 @@ static void dw_mci_setup_bus(struct dw_mci_slot *slot, bool force_clkinit)
div = (host->bus_hz != clock) ? DIV_ROUND_UP(div, 2) : 0;
- if (clock != slot->__clk_old || force_clkinit)
+ if ((clock != slot->__clk_old &&
+ !test_bit(DW_MMC_CARD_NEEDS_POLL, &slot->flags)) ||
+ force_clkinit) {
dev_info(&slot->mmc->class_dev,
"Bus speed (slot %d) = %dHz (slot req %dHz, actual %dHZ div = %d)\n",
slot->id, host->bus_hz, clock,
div ? ((host->bus_hz / div) >> 1) :
host->bus_hz, div);
+ /*
+ * If card is polling, display the message only
+ * one time at boot time.
+ */
+ if (slot->mmc->caps & MMC_CAP_NEEDS_POLL &&
+ slot->mmc->f_min == clock)
+ set_bit(DW_MMC_CARD_NEEDS_POLL, &slot->flags);
+ }
+
/* disable clock */
mci_writel(host, CLKENA, 0);
mci_writel(host, CLKSRC, 0);
diff --git a/drivers/mmc/host/dw_mmc.h b/drivers/mmc/host/dw_mmc.h
index 4a6ae750..c594658 100644
--- a/drivers/mmc/host/dw_mmc.h
+++ b/drivers/mmc/host/dw_mmc.h
@@ -272,6 +272,7 @@ struct dw_mci_slot {
#define DW_MMC_CARD_NEED_INIT 1
#define DW_MMC_CARD_NO_LOW_PWR 2
#define DW_MMC_CARD_NO_USE_HOLD 3
+#define DW_MMC_CARD_NEEDS_POLL 4
int id;
int sdio_id;
};
--
2.10.1
^ permalink raw reply related
* Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC
From: Arnd Bergmann @ 2016-11-24 11:09 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Hilbert Zhang, Andrew Lunn, Ulf Hansson, Romain Perier,
Liuliu Zhao, Peng Zhu, Adrian Hunter, Nadav Haklai, Jack(SH) Zhu,
Victor Gu, Doug Jones, Jisheng Zhang, Yehuda Yitschak,
Wei(SOCP) Liu, Xueping Liu, Shiwu Zhang, Yu Cao,
Sebastian Hesselbarth, devicetree, Jason Cooper, Keji Zhang,
Kostya Porotchkin, Rob Herring, Ryan Gao
In-Reply-To: <f7334e41-39dd-f868-ff10-199afaebe926@marvell.com>
On Thursday, November 24, 2016 6:57:18 PM CET Ziji Hu wrote:
> >
> > Please explain in the changelog why this is not a generic
> > phy driver (or three of them).
> >
> Actually we tried to put the PHY code into Linux PHY framework.
> But it cannot fit in Linux common PHY framework.
>
> Our Xenon SDHC PHY register is a part of Xenon SDHC register set.
> Besides, during MMC initialization, MMC sequence has to call several PHY functions to complete timing setting.
> In those PHY setting functions, they have to access SDHC register and know current MMC setting, such as bus width, clock frequency and speed mode.
> As a result, we have to implement PHY under MMC directory.
>
Ok, that makes sense, just put the same text in the changelog comment.
Arnd
^ permalink raw reply
* [PATCH 1/3] mmc: dw_mmc: check the "present" variable before checking flags
From: Jaehoon Chung @ 2016-11-24 11:04 UTC (permalink / raw)
To: linux-mmc; +Cc: ulf.hansson, shawn.lin, Jaehoon Chung
Before checking flags, it has to check "present" variable.
Otherwise, flags should be cleared everytime.
Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com>
---
drivers/mmc/host/dw_mmc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index d400afc..c6cc618 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1536,7 +1536,8 @@ static int dw_mci_get_cd(struct mmc_host *mmc)
spin_lock_bh(&host->lock);
if (present && !test_and_set_bit(DW_MMC_CARD_PRESENT, &slot->flags))
dev_dbg(&mmc->class_dev, "card is present\n");
- else if (!test_and_clear_bit(DW_MMC_CARD_PRESENT, &slot->flags))
+ else if (!present &&
+ !test_and_clear_bit(DW_MMC_CARD_PRESENT, &slot->flags))
dev_dbg(&mmc->class_dev, "card is not present\n");
spin_unlock_bh(&host->lock);
--
2.10.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox