linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Anatolij Gustschin <agust@denx.de>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: linux-fpga@vger.kernel.org,
	Alan Tull <atull@opensource.altera.com>,
	Moritz Fischer <moritz.fischer@ettus.com>,
	matthew.gerlach@linux.intel.com, yi1.li@linux.intel.com,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v4] fpga manager: Add Altera CvP driver
Date: Wed, 3 May 2017 02:14:39 +0200	[thread overview]
Message-ID: <20170503021439.6d812223@crub> (raw)
In-Reply-To: <CAHp75VcQCa7pKSrTZDTH85G4ukY1QfmQG5epRjJDd8adufdK=w@mail.gmail.com>

On Wed, 3 May 2017 00:28:17 +0300
Andy Shevchenko andy.shevchenko@gmail.com wrote:
...
>>>Is 0xff a mask here? (Btw, you missed spaces around <<)  
>>
>> yes, it is. Will add spaces (checkpatch didn't warn here).  
>
>Then it makes sense to add _MASK and use GENMASK() instead of direct value.

ok, will do.

>>>Why do we need that?!
>>>In new drivers we try to avoid new module parameters. We have enough
>>>interfaces nowadays to let driver know some details (quirks).  
>>
>> Which interface is preffered here? Do you suggest sysfs? Won't be able
>> to pass the parameter on kernel command line, then.  
>
>Yes, my question here is to understand what so important that driver
>needs module parameter.
>Can you elaborate?

the driver doesn't need this parameter, but it could help testing
the loading of compressed or encrypted images.

...
>>>Why 2 is missed? Hardware limitation? Needs a comment about these magics.  
>>
>> it is not missed, other values are just not valid and filtered out here.  
>
>That's my suggestion.
>So, if reviewer asks such a question it's a flag like "Okay, here is
>an additional comment required".

ok

>>>> +       /* set number of CVP clock cycles for every CVP Data Register Write */
>>>> +       pci_read_config_dword(conf->pci_dev, VSEC_CVP_MODE_CTRL, &val32);
>>>> +       val32 &= ~VSEC_CVP_MODE_CTRL_NUMCLKS;  
>>>  
>>>> +       val32 |= 0x01 << 8;     /* 1 clock */  
>>>
>>>Yeah, needs more clear way to put clocks of choice.  
>>
>> what exactly is not clear here?  
>
>Magics. And extra prefixes where it's not needed.
>
>0x0 - makes reader to think "A-ha, this is probably address or some
>interesting data. Here is just 1.
>8 - why? Usually we do ..._SHIFT costant for a such.

ok, will change.

>>>> +       pci_write_config_dword(conf->pci_dev, VSEC_CVP_MODE_CTRL, val32);
>>>> +
>>>> +       for (i = 0; i < CVP_DUMMY_WR; i++)  
>>>  
>>>> +               conf->write_data(conf, 0xdeadbeef);  
>>>
>>>Why this dummy is chosen?  
>>
>> it is a dummy and can be anything. So why not? I re-used some code
>> where this value was chosen. Can change it to 0.  
>
>Not need to be changed, but, please add a comment.

ok, will do.

...
>Just think about it:
>
>while (1) {
> ...100500 lines of code...
>}
>
>Reader needs to go through the entire body to understand 2 things:
>- what is the exit condition if any? do we have only one exit condition?
>- how many iterrations are supposed to be
>
>It takes time even more that took for writing above lines.
>
>Good code would be read fast.

ok

>>>> +               pci_read_config_dword(pdev, VSEC_CVP_STATUS, &val32);
>>>> +               if ((val32 & VSEC_CVP_STATUS_CFG_RDY) == 0)
>>>> +                       break;
>>>> +  
>>>  
>>>> +               udelay(1);      /* wait 1us */  
>>>
>>>Why not 10? Needs a comment.  
>>
>> if this is not obvious,  
>
>No, it's not. Especially after what you wrote below.
>
>> we want to start the configuration early and want
>> to avoid unneeded delays when polling ready status. For 10 I would have
>> to use usleep_range() adding more delay.  
>
>usleep_range() has one big difference to udelay: it's not atomic. This
>makes me to ask even more questions instead of understanding what's
>going on here.
>
>So, what kind of this function is? Is it supposed to be run in atomic
>context, not atomic, or any?

not atomic, a callback always running in a process context.

>Depends on answer we need to choose best API to allow minimum delays
>_and_ CPU resource waste.
>
>>>> +               if (chkcfg && !(bytes % SZ_4K)) {  
>>>
>>>Is 4k comes from PCI spec, or is it page size?  
>>
>> no, it is more an arbitrary value. It was suggested to check for
>> error status after writing a data block and not after each data write
>> to speed-up the config process. The config images can be big (above
>> 36 MiB) and often checking will slow down the configuration.  
>
>Your comment didn't make it more clearer to me.
>So, you take bytes value and check that 12 LSBs are 0. Why?

when 12 LSBs are zero, the bytes value has been decremented by
4k, meaning that a new 4k data block has been written. Only
then the error checking is performed.

...
>>>> +       pci_read_config_word(pdev, VSEC_PCIE_EXT_CAP_ID, &val16);  
>>>
>>>Are you foing to do this without enabling device? Needs comment why if so.  
>>
>> pci config space access works without enabling the pci device,
>> writing commands to config space enables the device first. It is done
>> some lines below which you deleted when commenting (please see original
>> patch).  
>
>Your comment didn't clarify what's going on along these lines.
>
>I checked original patch, I didn't find any type of
>pci_enable_device() call.

I mean this part (instead of pci_enable_device()):

+	/* Enable memory BAR access */
+	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
+	if (!(cmd & PCI_COMMAND_MEMORY)) {
+		cmd |= PCI_COMMAND_MEMORY;
+		pci_write_config_word(pdev, PCI_COMMAND, cmd);
+	}

>So,
>- can you use it?
>- if no, elaborate why not
>
>Okay, looks like below you answer this somehow.
>
>>>So, you are using devm_ above, but avoid pcim_ here. Please clarify
>>>enabling device case and use if possible pcim_  
>>
>> I can't use pcim_enable_device(), it will make the driver unusable
>> on some platforms.  
>
>> The driver is only for re-configuring FPGAs and
>> there can be unlimited variations of different PCIe devices implemented
>> in FPGAs. Some of them have e.g. additional huge BARs (4GiB) for
>> which an address range cannot be assigned on embedded 32-bit
>> platforms. pcim_enable_device() will fail here complaining about
>> not claimed BAR, even if the concerned BAR is not needed for FPGA
>> configuration at all. This makes the driver unusable.  
>
>Please put something like above in the comment in ->probe() before
>first call to pci_...().

ok

Thanks,

Anatolij

  reply	other threads:[~2017-05-03  0:14 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-04-30 19:08 Anatolij Gustschin
2017-05-01 14:58 ` Alan Tull
2017-05-01 20:06 ` Andy Shevchenko
2017-05-02  9:53   ` Anatolij Gustschin
2017-05-02 21:28     ` Andy Shevchenko
2017-05-03  0:14       ` Anatolij Gustschin [this message]
2017-05-03 14:19         ` Alan Tull
2017-05-03 15:01         ` Andy Shevchenko
2017-05-14  8:38           ` Anatolij Gustschin
2017-05-02 23:36     ` Joe Perches
2017-05-03  0:22       ` Anatolij Gustschin

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=20170503021439.6d812223@crub \
    --to=agust@denx.de \
    --cc=andy.shevchenko@gmail.com \
    --cc=atull@opensource.altera.com \
    --cc=linux-fpga@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=matthew.gerlach@linux.intel.com \
    --cc=moritz.fischer@ettus.com \
    --cc=yi1.li@linux.intel.com \
    --subject='Re: [PATCH v4] fpga manager: Add Altera CvP driver' \
    /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

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).