From: Moritz Fischer <mdf@kernel.org>
To: Thor Thayer <thor.thayer@linux.intel.com>
Cc: Moritz Fischer <mdf@kernel.org>,
richard.gong@linux.intel.com, agust@denx.de,
linux-fpga@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCHv2 2/3] fpga: altera-cvp: Preparation for V2 parts.
Date: Wed, 24 Jul 2019 07:57:04 -0700 [thread overview]
Message-ID: <20190724145704.GB24455@archbox> (raw)
In-Reply-To: <6e54c0ee-b8ec-4f95-cf81-70aacc82c72e@linux.intel.com>
On Tue, Jul 23, 2019 at 09:40:51AM -0500, Thor Thayer wrote:
> Hi Moritz,
>
> On 7/21/19 7:59 PM, Moritz Fischer wrote:
> > Thor,
> >
> > On Tue, Jul 16, 2019 at 05:48:06PM -0500, thor.thayer@linux.intel.com wrote:
> > > From: Thor Thayer <thor.thayer@linux.intel.com>
> > >
> > > In preparation for adding newer V2 parts that use a FIFO,
> > > reorganize altera_cvp_chk_error() and change the write
> > > function to block based.
> > > V2 parts have a block size matching the FIFO while older
> > > V1 parts write a 32 bit word at a time.
> > >
> > > Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
> > > ---
> > > v2 Remove inline function declaration
> > > Reverse Christmas Tree format for local variables
> > > ---
> > > drivers/fpga/altera-cvp.c | 72 ++++++++++++++++++++++++++++++-----------------
> > > 1 file changed, 46 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
> > > index b78c90580071..37419d6b9915 100644
> > > --- a/drivers/fpga/altera-cvp.c
> > > +++ b/drivers/fpga/altera-cvp.c
> > > @@ -140,6 +140,41 @@ static int altera_cvp_wait_status(struct altera_cvp_conf *conf, u32 status_mask,
> > > return -ETIMEDOUT;
> > > }
> > > +static int altera_cvp_chk_error(struct fpga_manager *mgr, size_t bytes)
> > > +{
> > > + struct altera_cvp_conf *conf = mgr->priv;
> > > + u32 val;
> > > +
> > > + /* STEP 10 (optional) - check CVP_CONFIG_ERROR flag */
> > > + altera_read_config_dword(conf, VSE_CVP_STATUS, &val);
> > Same as in the other email, why can we ignore return values here. I
> > think the original code probably did that already.
>
> Yes, I actually didn't make any changes to this function. You can see I
> moved it from below since it is used in the following function.
>
> I'm not checking the return code from any of the read/write functions since
> the original driver didn't. Would you prefer I check and issue a warning?
Not sure a warning would change much here. We should probably look at
why it was ok in the first place.
>
> Thanks for reviewing!
>
> > > + if (val & VSE_CVP_STATUS_CFG_ERR) {
> > > + dev_err(&mgr->dev, "CVP_CONFIG_ERROR after %zu bytes!\n",
> > > + bytes);
> > > + return -EPROTO;
> > > + }
> > > + return 0;
> > > +}
> > > +
> > > +static int altera_cvp_send_block(struct altera_cvp_conf *conf,
> > > + const u32 *data, size_t len)
> > > +{
> > > + u32 mask, words = len / sizeof(u32);
> > > + int i, remainder;
> > > +
> > > + for (i = 0; i < words; i++)
> > > + conf->write_data(conf, *data++);
> > > +
> > > + /* write up to 3 trailing bytes, if any */
> > > + remainder = len % sizeof(u32);
> > > + if (remainder) {
> > > + mask = BIT(remainder * 8) - 1;
> > > + if (mask)
> > > + conf->write_data(conf, *data & mask);
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > static int altera_cvp_teardown(struct fpga_manager *mgr,
> > > struct fpga_image_info *info)
> > > {
> > > @@ -262,39 +297,29 @@ static int altera_cvp_write_init(struct fpga_manager *mgr,
> > > return 0;
> > > }
> > > -static inline int altera_cvp_chk_error(struct fpga_manager *mgr, size_t bytes)
> > > -{
> > > - struct altera_cvp_conf *conf = mgr->priv;
> > > - u32 val;
> > > -
> > > - /* STEP 10 (optional) - check CVP_CONFIG_ERROR flag */
> > > - altera_read_config_dword(conf, VSE_CVP_STATUS, &val);
> > > - if (val & VSE_CVP_STATUS_CFG_ERR) {
> > > - dev_err(&mgr->dev, "CVP_CONFIG_ERROR after %zu bytes!\n",
> > > - bytes);
> > > - return -EPROTO;
> > > - }
> > > - return 0;
> > > -}
> > > -
> > > static int altera_cvp_write(struct fpga_manager *mgr, const char *buf,
> > > size_t count)
> > > {
> > > struct altera_cvp_conf *conf = mgr->priv;
> > > + size_t done, remaining, len;
> > > const u32 *data;
> > > - size_t done, remaining;
> > > int status = 0;
> > > - u32 mask;
> > > /* STEP 9 - write 32-bit data from RBF file to CVP data register */
> > > data = (u32 *)buf;
> > > remaining = count;
> > > done = 0;
> > > - while (remaining >= 4) {
> > > - conf->write_data(conf, *data++);
> > > - done += 4;
> > > - remaining -= 4;
> > > + while (remaining) {
> > > + if (remaining >= sizeof(u32))
> > > + len = sizeof(u32);
> > > + else
> > > + len = remaining;
> > > +
> > > + altera_cvp_send_block(conf, data, len);
> > > + data++;
> > > + done += len;
> > > + remaining -= len;
> > > /*
> > > * STEP 10 (optional) and STEP 11
> > > @@ -312,11 +337,6 @@ static int altera_cvp_write(struct fpga_manager *mgr, const char *buf,
> > > }
> > > }
> > > - /* write up to 3 trailing bytes, if any */
> > > - mask = BIT(remaining * 8) - 1;
> > > - if (mask)
> > > - conf->write_data(conf, *data & mask);
> > > -
> > > if (altera_cvp_chkcfg)
> > > status = altera_cvp_chk_error(mgr, count);
> > > --
> > > 2.7.4
> > >
> > Cheers,
> > Moritz
> >
>
Moritz
next prev parent reply other threads:[~2019-07-24 14:57 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-16 22:48 [PATCHv2 0/3] fpga: altera-cvp: Add Stratix10 Support thor.thayer
2019-07-16 22:48 ` [PATCHv2 1/3] fpga: altera-cvp: Discover Vendor Specific offset thor.thayer
2019-07-16 22:48 ` [PATCHv2 2/3] fpga: altera-cvp: Preparation for V2 parts thor.thayer
2019-07-22 0:59 ` Moritz Fischer
2019-07-23 14:40 ` Thor Thayer
2019-07-24 14:57 ` Moritz Fischer [this message]
2019-07-24 16:59 ` Thor Thayer
2019-07-24 17:05 ` Moritz Fischer
2019-07-16 22:48 ` [PATCHv2 3/3] fpga: altera-cvp: Add Stratix10 (V2) Support thor.thayer
2019-07-22 0:56 ` Moritz Fischer
2019-07-23 14:38 ` Thor Thayer
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=20190724145704.GB24455@archbox \
--to=mdf@kernel.org \
--cc=agust@denx.de \
--cc=linux-fpga@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=richard.gong@linux.intel.com \
--cc=thor.thayer@linux.intel.com \
/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).