Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH net-next 1/2] net: add support for Cavium PTP coprocessor
To:     Aleksey Makarov <aleksey.makarov@cavium.com>,
        netdev@vger.kernel.org
Cc:     Robert Richter <rric@kernel.org>,
        "Goutham, Sunil" <Sunil.Goutham@cavium.com>,
        Radoslaw Biernacki <rad@semihalf.com>,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org
References: <20171107190704.15458-1-aleksey.makarov@cavium.com>
 <20171107190704.15458-2-aleksey.makarov@cavium.com>
From:   David Daney <ddaney@caviumnetworks.com>
Message-ID: <1d765627-edb1-cc0f-65bc-cf061687a8c6@caviumnetworks.com>
Date:   Tue, 7 Nov 2017 11:49:23 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.4.0
MIME-Version: 1.0
In-Reply-To: <20171107190704.15458-2-aleksey.makarov@cavium.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Received-SPF: None (protection.outlook.com: cavium.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Nov 2017 19:49:25.7831 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 3be765d1-6659-4e96-dc3c-08d52618a6b5
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 711e4ccf-2e9b-4bcf-a551-4094005b6194
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR07MB3496
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 11/07/2017 11:07 AM, Aleksey Makarov wrote:
> From: Radoslaw Biernacki <rad@semihalf.com>
> 
> This patch adds support for the Precision Time Protocol
> Clocks and Timestamping hardware found on Cavium ThunderX
> processors.
> 
> Signed-off-by: Radoslaw Biernacki <rad@semihalf.com>
> Signed-off-by: Aleksey Makarov <aleksey.makarov@cavium.com>
> ---
>   drivers/net/ethernet/cavium/Kconfig             |  13 +
>   drivers/net/ethernet/cavium/Makefile            |   1 +
>   drivers/net/ethernet/cavium/common/Makefile     |   1 +
>   drivers/net/ethernet/cavium/common/cavium_ptp.c | 353 ++++++++++++++++++++++++
>   drivers/net/ethernet/cavium/common/cavium_ptp.h |  78 ++++++
>   5 files changed, 446 insertions(+)
>   create mode 100644 drivers/net/ethernet/cavium/common/Makefile
>   create mode 100644 drivers/net/ethernet/cavium/common/cavium_ptp.c
>   create mode 100644 drivers/net/ethernet/cavium/common/cavium_ptp.h
> 
[...]
> +
> +/* The Cavium PTP can *only* be found in SoCs containing the ThunderX
> + * ARM64 CPU implementation.  All accesses to the device registers on this
> + * platform are implicitly strongly ordered with respect to memory
> + * accesses.

I believe that is not correct.  I/O register accesses are implicitly 
ordered with respect to other I/O register accesses.  However, with 
respect to memory accesses, no ordering is imposed.  Therefore, one must 
be very careful not to introduce subtile memory ordering bugs with these 
things when using the unordered versions.

> + * So writeq_relaxed() and readq_relaxed() are safe to use with
> + * no memory barriers in this driver.  The readq()/writeq() functions add
> + * explicit ordering operation which in this case are redundant, and only
> + * add overhead.


Also it should be noted that on production silicon, the performance 
difference between the "relaxed" variant and the normal variant of 
read*/write* is often negligible.


> + */
> +
> +static u64 cavium_ptp_reg_read(struct cavium_ptp *clock, u64 offset)
> +{
> +	return readq_relaxed(clock->reg_base + offset);
> +}
> +
> +static void cavium_ptp_reg_write(struct cavium_ptp *clock, u64 offset, u64 val)
> +{
> +	writeq_relaxed(val, clock->reg_base + offset);
> +}
> +

Are the PTP register access really so much in the hot path that using 
the relaxed variants can be measured here?  If not, would it make the 
driver look cleaner to remove these and just use readq/writeq calls 
directly  in the body of the driver?

David.

From 1583463173264924659@xxx Wed Nov 08 02:29:19 +0000 2017
X-GM-THRID: 1583463173264924659
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread