From: vitor <Vitor.Soares@synopsys.com>
To: Boris Brezillon <boris.brezillon@bootlin.com>,
Vitor soares <Vitor.Soares@synopsys.com>
Cc: <wsa@the-dreams.de>, <linux-i2c@vger.kernel.org>,
<corbet@lwn.net>, <linux-doc@vger.kernel.org>,
<gregkh@linuxfoundation.org>, <arnd@arndb.de>,
<psroka@cadence.com>, <agolec@cadence.com>,
<adouglas@cadence.com>, <bfolta@cadence.com>, <dkos@cadence.com>,
<alicja@cadence.com>, <cwronka@cadence.com>,
<sureshp@cadence.com>, <rafalc@cadence.com>,
<thomas.petazzoni@bootlin.com>, <nm@ti.com>, <robh+dt@kernel.org>,
<pawel.moll@arm.com>, <mark.rutland@arm.com>,
<ijc+devicetree@hellion.org.uk>, <galak@codeaurora.org>,
<devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
<geert@linux-m68k.org>, <linus.walleij@linaro.org>,
<Xiang.Lin@synaptics.com>, <linux-gpio@vger.kernel.org>,
<nsekhar@ti.com>, <pgaj@cadence.com>, <peda@axentia.se>,
<Joao.Pinto@synopsys.com>
Subject: Re: [PATCH 1/3] i3c: master: Add driver for Synopsys DesignWare IP
Date: Wed, 8 Aug 2018 18:28:57 +0100 [thread overview]
Message-ID: <04ae034c-ec6c-ce43-6536-9da9d765d558@synopsys.com> (raw)
In-Reply-To: <20180727153850.5a8ca75c@bbrezillon>
Hi Boris,
On 27-07-2018 14:38, Boris Brezillon wrote:
> Hi Victor,
>
> On Fri, 20 Jul 2018 21:57:50 +0100
> Vitor soares <Vitor.Soares@synopsys.com> wrote:
>
>> This patch add driver for Synopsys DesignWare IP on top of
>> I3C subsystem patchset proposal V6
> The fact that you based your work on v6 of the I3C patchset should be
> placed ....
>
>> Signed-off-by: Vitor soares <soares@synopsys.com>
>> ---
> ... here, so that it does not appear in the final commit message.
>
>
> [...]
I will do that in the next submission.
>> +static int dw_i3c_master_get_free_pos(struct dw_i3c_master *master)
>> +{
>> + if (!(master->free_pos & GENMASK(master->maxdevs - 1, 0)))
>> + return -ENOSPC;
>> +
>> + return ffs(master->free_pos) - 1;
>> +}
> Okay, maybe we can have a generic infrastructure for that part (I have
> the same logic in the Cadence driver), but let's keep that for later.
I think everyone will need a table (SW or HW) to mask the slots
available for DAA.
>
>> +
>> +static void dw_i3c_master_wr_tx_fifo(struct dw_i3c_master *master,
>> + const u8 *bytes, int nbytes)
>> +{
>> + int i, j;
>> +
>> + for (i = 0; i < nbytes; i += 4) {
>> + u32 data = 0;
>> +
>> + for (j = 0; j < 4 && (i + j) < nbytes; j++)
>> + data |= (u32)bytes[i + j] << (j * 8);
>> +
>> + writel(data, master->regs + RX_TX_DATA_PORT);
> Maybe you can use writesl() as suggested by Arnd in his review of the
> Cadence driver.
Andy also suggest get_unaligned_le32() / put_unaligned_le32() to
read/write from FIFOS. I will try both solutions.
>
>> + }
>> +}
>> +
> [...]
>
>> +
>> +static void dw_i3c_master_detach_i2c_dev(struct i2c_dev_desc *dev)
>> +{
>> + struct dw_i3c_i2c_dev_data *data = i2c_dev_get_master_data(dev);
>> + struct i3c_master_controller *m = i2c_dev_get_master(dev);
>> + struct dw_i3c_master *master = to_dw_i3c_master(m);
>> +
>> + writel(NULL,
> ^ 0 not NULL
I will change it.
>
>> + master->regs +
>> + DEV_ADDR_TABLE_LOC(master->datstartaddr, data->index));
>> +
>> + i2c_dev_set_master_data(dev, NULL);
>> + master->addrs[data->index] = 0;
>> + master->free_pos |= BIT(data->index);
>> + kfree(data);
>> +}
>> +
>> +static int dw_i3c_probe(struct platform_device *pdev)
>> +{
>> + struct dw_i3c_master *master;
>> + struct resource *res;
>> + int ret, irq;
>> +
>> + master = devm_kzalloc(&pdev->dev, sizeof(*master), GFP_KERNEL);
>> + if (!master)
>> + return -ENOMEM;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + master->regs = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(master->regs))
>> + return PTR_ERR(master->regs);
>> +
>> + master->core_clk = devm_clk_get(&pdev->dev, "core_clk");
>> + if (IS_ERR(master->core_clk))
>> + return PTR_ERR(master->core_clk);
>> +
>> + master->core_rst = devm_reset_control_get_optional_exclusive(&pdev->dev,
>> + "core_rst");
>> + if (IS_ERR(master->core_rst))
>> + return PTR_ERR(master->core_rst);
>> +
>> + ret = clk_prepare_enable(master->core_clk);
>> + if (ret)
>> + goto err_disable_core_clk;
>> +
>> + reset_control_deassert(master->core_rst);
>> +
>> + spin_lock_init(&master->xferqueue.lock);
>> + INIT_LIST_HEAD(&master->xferqueue.list);
>> +
>> + writel(INTR_ALL, master->regs + INTR_STATUS);
>> + irq = platform_get_irq(pdev, 0);
>> + ret = devm_request_irq(&pdev->dev, irq,
>> + dw_i3c_master_irq_handler, 0,
>> + dev_name(&pdev->dev), master);
>> + if (ret)
>> + goto err_assert_rst;
>> +
>> + platform_set_drvdata(pdev, master);
>> +
>> + ret = readl(master->regs + QUEUE_STATUS_LEVEL);
>> + master->caps.cmdfifodepth = QUEUE_STATUS_LEVEL_CMD(ret);
>> +
>> + ret = readl(master->regs + DATA_BUFFER_STATUS_LEVEL);
>> + master->caps.datafifodepth = DATA_BUFFER_STATUS_LEVEL_TX(ret);
>> +
>> + ret = readl(master->regs + DEVICE_ADDR_TABLE_POINTER);
>> + master->datstartaddr = ret;
>> + master->maxdevs = ret >> 16;
>> + master->free_pos = GENMASK(master->maxdevs - 1, 0);
>> +
>> + /* read controller version */
>> + ret = readl(master->regs + I3C_VER_ID);
>> + master->version[0] = (char)(ret >> 24);
>> + master->version[1] = '.';
>> + master->version[2] = (char)(ret >> 16);
>> + master->version[3] = (char)(ret >> 8);
>> + master->version[4] = '\0';
>> +
>> + /* read controller type */
>> + ret = readl(master->regs + I3C_VER_TYPE);
>> + master->type[0] = (char)(ret >> 24);
>> + master->type[1] = (char)(ret >> 16);
>> + master->type[2] = (char)(ret >> 8);
>> + master->type[3] = (char) ret;
>> + master->type[4] = '\0';
> Hm, do you really intend to read these as strings? If you do, maybe you
> can use sprintf() here:
>
> sprintf(master->version, "%c.%c%c", ...)
> sprintf(master->type, "%c%c%c%c", ...)
Maybe this information is unnecessary. I will remove it.
>
>
>> +
>> + ret = i3c_master_register(&master->base, &pdev->dev,
>> + &dw_mipi_i3c_ops, false);
>> + if (ret)
>> + goto err_assert_rst;
>> +
>> + dev_info(&pdev->dev, "Synopsys DW MIPI I3C Master: version %s-%s\n",
>> + master->version, master->type);
>> +
>> + return 0;
>> +
>> +err_assert_rst:
>> + reset_control_assert(master->core_rst);
>> +
>> +err_disable_core_clk:
>> + clk_disable_unprepare(master->core_clk);
>> +
>> + return ret;
>> +}
> The driver looks pretty good already, and I'm pleasantly surprised to
> see that the Synopsys IP works pretty much the same way the Cadence one
> does. I could find some of the logic I implemented in the Cadence
> driver almost directly applied to this one, so I think there's room for
> code factorization. Anyway, let's see what the next controller looks
> like before doing that.
>
> Thanks for sharing your work early and for reviewing the previous
> versions of the I3C patchset.
>
> Regards,
>
> Boris
I tried to not reinvent the wheel. Right now the test with I3C devices
are running very well.
I still have one or another detail that can be optimize.
Thanks for your feedback.
Best regards,
Vitor Soares
next prev parent reply other threads:[~2018-08-08 17:29 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-07-20 20:57 [PATCH 0/3] Add driver for Synopsys DesignWare I3C master IP Vitor soares
2018-07-20 20:57 ` [PATCH 1/3] i3c: master: Add driver for Synopsys DesignWare IP Vitor soares
2018-07-21 15:35 ` Andy Shevchenko
[not found] ` <9fdfaf4e-5a20-af81-82ef-0d9e327f9133@synopsys.com>
2018-07-25 16:56 ` Andy Shevchenko
2018-08-08 17:01 ` vitor
2018-07-27 13:38 ` Boris Brezillon
2018-08-08 17:28 ` vitor [this message]
2018-07-20 20:57 ` [PATCH 2/3] dt-bindings: i3c: Document Synopsys DesignWare I3C master bindings Vitor soares
2018-07-25 19:57 ` Rob Herring
2018-08-08 16:59 ` vitor
2018-08-13 14:15 ` Rob Herring
2018-07-20 20:57 ` [PATCH 3/3] MAINTAINERS: Add myself as the dw-i3c-master module maintainer Vitor soares
2018-07-20 21:58 ` [PATCH 0/3] Add driver for Synopsys DesignWare I3C master IP Wolfram Sang
2018-07-21 17:15 ` Boris Brezillon
2018-07-21 22:59 ` Wolfram Sang
2018-10-29 10:06 Vitor soares
2018-10-29 10:06 ` [PATCH 1/3] i3c: master: Add driver for Synopsys DesignWare IP Vitor soares
2018-10-29 13:41 ` Matthew Wilcox
2018-11-07 15:15 ` vitor
2018-11-07 15:34 ` Geert Uytterhoeven
2018-11-07 17:16 ` vitor
2018-11-07 17:05 ` Matthew Wilcox
2018-11-07 17:30 ` vitor
2018-11-07 17:37 ` Matthew Wilcox
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=04ae034c-ec6c-ce43-6536-9da9d765d558@synopsys.com \
--to=vitor.soares@synopsys.com \
--cc=Joao.Pinto@synopsys.com \
--cc=Xiang.Lin@synaptics.com \
--cc=adouglas@cadence.com \
--cc=agolec@cadence.com \
--cc=alicja@cadence.com \
--cc=arnd@arndb.de \
--cc=bfolta@cadence.com \
--cc=boris.brezillon@bootlin.com \
--cc=corbet@lwn.net \
--cc=cwronka@cadence.com \
--cc=devicetree@vger.kernel.org \
--cc=dkos@cadence.com \
--cc=galak@codeaurora.org \
--cc=geert@linux-m68k.org \
--cc=gregkh@linuxfoundation.org \
--cc=ijc+devicetree@hellion.org.uk \
--cc=linus.walleij@linaro.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-i2c@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=nm@ti.com \
--cc=nsekhar@ti.com \
--cc=pawel.moll@arm.com \
--cc=peda@axentia.se \
--cc=pgaj@cadence.com \
--cc=psroka@cadence.com \
--cc=rafalc@cadence.com \
--cc=robh+dt@kernel.org \
--cc=sureshp@cadence.com \
--cc=thomas.petazzoni@bootlin.com \
--cc=wsa@the-dreams.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).