From: Serge Semin <Sergey.Semin@baikalelectronics.ru> To: Lars Povlsen <lars.povlsen@microchip.com> Cc: Serge Semin <fancer.lancer@gmail.com>, Mark Brown <broonie@kernel.org>, <devicetree@vger.kernel.org>, Alexandre Belloni <alexandre.belloni@bootlin.com>, <linux-kernel@vger.kernel.org>, <linux-spi@vger.kernel.org>, SoC Team <soc@kernel.org>, Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>, <linux-arm-kernel@lists.infradead.org> Subject: Re: [PATCH 05/10] spi: spi-dw-mmio: Spin off MSCC platforms into spi-dw-mchp Date: Wed, 3 Jun 2020 00:12:04 +0300 [thread overview] Message-ID: <20200602211203.3lad22zvt5yagane@mobilestation> (raw) In-Reply-To: <20200519120519.GE24801@soft-dev15.microsemi.net> On Tue, May 19, 2020 at 02:05:19PM +0200, Lars Povlsen wrote: > On 13/05/20 16:18, Mark Brown wrote: > > Date: Wed, 13 May 2020 16:18:11 +0100 > > From: Mark Brown <broonie@kernel.org> > > To: Lars Povlsen <lars.povlsen@microchip.com> > > Cc: SoC Team <soc@kernel.org>, Microchip Linux Driver Support > > <UNGLinuxDriver@microchip.com>, linux-spi@vger.kernel.org, > > devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, > > linux-arm-kernel@lists.infradead.org, Alexandre Belloni > > <alexandre.belloni@bootlin.com> > > Subject: Re: [PATCH 05/10] spi: spi-dw-mmio: Spin off MSCC platforms into > > spi-dw-mchp > > User-Agent: Mutt/1.10.1 (2018-07-13) > > > > On Wed, May 13, 2020 at 04:00:26PM +0200, Lars Povlsen wrote: > > > > > +config SPI_DW_MCHP > > > + tristate "Memory-mapped io interface driver using DW SPI core of MSCC SoCs" > > > + default y if ARCH_SPARX5 > > > + default y if SOC_VCOREIII > > > > Why the default ys? > > The SoC will typically boot from SPI... But its not a requirement per > se. I will remove it. > > > > > > +++ b/drivers/spi/Makefile > > > @@ -37,6 +37,7 @@ obj-$(CONFIG_SPI_DAVINCI) += spi-davinci.o > > > obj-$(CONFIG_SPI_DLN2) += spi-dln2.o > > > obj-$(CONFIG_SPI_DESIGNWARE) += spi-dw.o > > > obj-$(CONFIG_SPI_DW_MMIO) += spi-dw-mmio.o > > > +obj-$(CONFIG_SPI_DW_MCHP) += spi-dw-mchp.o > > > obj-$(CONFIG_SPI_DW_PCI) += spi-dw-midpci.o > > > spi-dw-midpci-objs := spi-dw-pci.o spi-dw-mid.o > > > obj-$(CONFIG_SPI_EFM32) += spi-efm32.o > > > > Please keep the file alphabetically sorted. > > > > Noted. > > > > +++ b/drivers/spi/spi-dw-mchp.c > > > @@ -0,0 +1,232 @@ > > > +// SPDX-License-Identifier: GPL-2.0-only > > > +/* > > > + * Memory-mapped interface driver for MSCC SoCs > > > + * > > > > Please make the entire comment a C++ one so things look more > > intentional. > > Sure, I can do that. The presented form matches that of the other > spi-dw-* drivers, but I can see other using // blocks. Ack. > > > > > > +#define MAX_CS 4 > > > > This should be namespaced. > > Ack. > > > > > > + rx_sample_dly = 0; > > > + device_property_read_u32(&pdev->dev, "spi-rx-delay-us", &rx_sample_dly); > > > + dws->rx_sample_dly = DIV_ROUND_UP(rx_sample_dly, > > > + (dws->max_freq / 1000000)); Perhaps 100000 is better to be replace with macro USEC_PER_SEC... Moreover are you sure the formulae is correct? dws->rx_sample_dly - a number of ssi_clk periods/cycles to delay the Rx-data sample, dws->max_freq - ssi_clk frequency (not period). In real math the formulae would look like: S = d * P [s], where d - number of delay cycles, P - ssi_clk period in seconds, S - requested delay in seconds. In the driver notation: d = dws->rx_sample_dly, P = 1 / dws->max_freq, S = rx_sample_dly ("spi-rx-delay-us" property value). dws->rx_sample_dly * (1 / dws->max_freq) = rx_sample_dly <=> dws->rx_sample_dly = rx_sample_dly * dws->max_freq. Though that's represented in seconds, so if rx_sample_dly is specified in usec, then you'd need to scale it down dividing by USEC_PER_SEC. For example, imagine we need a delay of 1 usec with ssi_clk of 50MHz. By your formulae we'd have: 1 / (50000000 / 1000000) = 0 cycles (actually 1 due to DIV_ROUND_UP, but incorrect anyway), By mine: 1 * (500000000 / 1000000) = 50 cycles. Seems closer to reality. Am I missing something? > > > > If this is a standard feature of the DesignWare IP why parse it here and > > not in the generic code? > > This is a standard feature of the DesignWare IP, so good suggestion. I > will arrange with Serge. Regarding "spi-rx-delay-us" and the sampling delay the IP supports. Here is what documentation says regarding the register, which is then initialized with this parameter "This register controls the number of ssi_clk cycles that are delayed from the default sample time before the actual sample of the rxd input signal occurs." While the "spi-rx-delay-us" property is described as: "Delay, in microseconds, after a read transfer." I may misunderstand something, but IMO these descriptions don't refer to the same values. The only real use of the "spi-rx-delay-us" property I've found in "./drivers/input/rmi4/rmi_spi.c". That driver gets the value of the property and just sets the delay_usecs of some transfers, which isn't even close to the functionality the RX_SAMPLE_DLY register provides. To be clear the RX_SAMPLE_DLY register can be used to delay the RX-bits sample with respect to the normal Rx sampling timing. The delay is measured in the numbers of the ssi_clk periods. (Note also that the maximum delay is limited with a constant parameter pre-initialized at the IP-core synthesis stage. It can be defined within a range [4, 255]. In our IP it's limited with just 4 periods.) As I see it, a better way would be to either define a new vendor-specific property like "snps,rx-sample-delay-ns" (note NS here, since normally the ssi_clk is much higher than 1MHz), or define a new generic SPI property. Mark, Andy? -Sergey > > Thank you for your comments! > > ---Lars > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
WARNING: multiple messages have this Message-ID (diff)
From: Serge Semin <Sergey.Semin@baikalelectronics.ru> To: Lars Povlsen <lars.povlsen@microchip.com> Cc: devicetree@vger.kernel.org, Alexandre Belloni <alexandre.belloni@bootlin.com>, linux-kernel@vger.kernel.org, Serge Semin <fancer.lancer@gmail.com>, linux-spi@vger.kernel.org, SoC Team <soc@kernel.org>, Mark Brown <broonie@kernel.org>, Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 05/10] spi: spi-dw-mmio: Spin off MSCC platforms into spi-dw-mchp Date: Wed, 3 Jun 2020 00:12:04 +0300 [thread overview] Message-ID: <20200602211203.3lad22zvt5yagane@mobilestation> (raw) In-Reply-To: <20200519120519.GE24801@soft-dev15.microsemi.net> On Tue, May 19, 2020 at 02:05:19PM +0200, Lars Povlsen wrote: > On 13/05/20 16:18, Mark Brown wrote: > > Date: Wed, 13 May 2020 16:18:11 +0100 > > From: Mark Brown <broonie@kernel.org> > > To: Lars Povlsen <lars.povlsen@microchip.com> > > Cc: SoC Team <soc@kernel.org>, Microchip Linux Driver Support > > <UNGLinuxDriver@microchip.com>, linux-spi@vger.kernel.org, > > devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, > > linux-arm-kernel@lists.infradead.org, Alexandre Belloni > > <alexandre.belloni@bootlin.com> > > Subject: Re: [PATCH 05/10] spi: spi-dw-mmio: Spin off MSCC platforms into > > spi-dw-mchp > > User-Agent: Mutt/1.10.1 (2018-07-13) > > > > On Wed, May 13, 2020 at 04:00:26PM +0200, Lars Povlsen wrote: > > > > > +config SPI_DW_MCHP > > > + tristate "Memory-mapped io interface driver using DW SPI core of MSCC SoCs" > > > + default y if ARCH_SPARX5 > > > + default y if SOC_VCOREIII > > > > Why the default ys? > > The SoC will typically boot from SPI... But its not a requirement per > se. I will remove it. > > > > > > +++ b/drivers/spi/Makefile > > > @@ -37,6 +37,7 @@ obj-$(CONFIG_SPI_DAVINCI) += spi-davinci.o > > > obj-$(CONFIG_SPI_DLN2) += spi-dln2.o > > > obj-$(CONFIG_SPI_DESIGNWARE) += spi-dw.o > > > obj-$(CONFIG_SPI_DW_MMIO) += spi-dw-mmio.o > > > +obj-$(CONFIG_SPI_DW_MCHP) += spi-dw-mchp.o > > > obj-$(CONFIG_SPI_DW_PCI) += spi-dw-midpci.o > > > spi-dw-midpci-objs := spi-dw-pci.o spi-dw-mid.o > > > obj-$(CONFIG_SPI_EFM32) += spi-efm32.o > > > > Please keep the file alphabetically sorted. > > > > Noted. > > > > +++ b/drivers/spi/spi-dw-mchp.c > > > @@ -0,0 +1,232 @@ > > > +// SPDX-License-Identifier: GPL-2.0-only > > > +/* > > > + * Memory-mapped interface driver for MSCC SoCs > > > + * > > > > Please make the entire comment a C++ one so things look more > > intentional. > > Sure, I can do that. The presented form matches that of the other > spi-dw-* drivers, but I can see other using // blocks. Ack. > > > > > > +#define MAX_CS 4 > > > > This should be namespaced. > > Ack. > > > > > > + rx_sample_dly = 0; > > > + device_property_read_u32(&pdev->dev, "spi-rx-delay-us", &rx_sample_dly); > > > + dws->rx_sample_dly = DIV_ROUND_UP(rx_sample_dly, > > > + (dws->max_freq / 1000000)); Perhaps 100000 is better to be replace with macro USEC_PER_SEC... Moreover are you sure the formulae is correct? dws->rx_sample_dly - a number of ssi_clk periods/cycles to delay the Rx-data sample, dws->max_freq - ssi_clk frequency (not period). In real math the formulae would look like: S = d * P [s], where d - number of delay cycles, P - ssi_clk period in seconds, S - requested delay in seconds. In the driver notation: d = dws->rx_sample_dly, P = 1 / dws->max_freq, S = rx_sample_dly ("spi-rx-delay-us" property value). dws->rx_sample_dly * (1 / dws->max_freq) = rx_sample_dly <=> dws->rx_sample_dly = rx_sample_dly * dws->max_freq. Though that's represented in seconds, so if rx_sample_dly is specified in usec, then you'd need to scale it down dividing by USEC_PER_SEC. For example, imagine we need a delay of 1 usec with ssi_clk of 50MHz. By your formulae we'd have: 1 / (50000000 / 1000000) = 0 cycles (actually 1 due to DIV_ROUND_UP, but incorrect anyway), By mine: 1 * (500000000 / 1000000) = 50 cycles. Seems closer to reality. Am I missing something? > > > > If this is a standard feature of the DesignWare IP why parse it here and > > not in the generic code? > > This is a standard feature of the DesignWare IP, so good suggestion. I > will arrange with Serge. Regarding "spi-rx-delay-us" and the sampling delay the IP supports. Here is what documentation says regarding the register, which is then initialized with this parameter "This register controls the number of ssi_clk cycles that are delayed from the default sample time before the actual sample of the rxd input signal occurs." While the "spi-rx-delay-us" property is described as: "Delay, in microseconds, after a read transfer." I may misunderstand something, but IMO these descriptions don't refer to the same values. The only real use of the "spi-rx-delay-us" property I've found in "./drivers/input/rmi4/rmi_spi.c". That driver gets the value of the property and just sets the delay_usecs of some transfers, which isn't even close to the functionality the RX_SAMPLE_DLY register provides. To be clear the RX_SAMPLE_DLY register can be used to delay the RX-bits sample with respect to the normal Rx sampling timing. The delay is measured in the numbers of the ssi_clk periods. (Note also that the maximum delay is limited with a constant parameter pre-initialized at the IP-core synthesis stage. It can be defined within a range [4, 255]. In our IP it's limited with just 4 periods.) As I see it, a better way would be to either define a new vendor-specific property like "snps,rx-sample-delay-ns" (note NS here, since normally the ssi_clk is much higher than 1MHz), or define a new generic SPI property. Mark, Andy? -Sergey > > Thank you for your comments! > > ---Lars > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-06-02 21:12 UTC|newest] Thread overview: 78+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-05-13 14:00 [PATCH 00/10] spi: Adding support for Microchip Sparx5 SoC Lars Povlsen 2020-05-13 14:00 ` Lars Povlsen 2020-05-13 14:00 ` [PATCH 01/10] spi: dw: Add support for polled operation via no IRQ specified in DT Lars Povlsen 2020-05-13 14:00 ` Lars Povlsen 2020-05-13 14:20 ` Mark Brown 2020-05-14 13:04 ` Serge Semin 2020-05-14 13:04 ` Serge Semin 2020-05-15 9:11 ` Lars Povlsen 2020-05-15 9:11 ` Lars Povlsen 2020-05-13 14:37 ` Mark Brown 2020-05-19 10:21 ` Lars Povlsen 2020-05-19 10:21 ` Lars Povlsen 2020-05-13 14:55 ` Andy Shevchenko 2020-05-13 14:55 ` Andy Shevchenko 2020-05-19 10:25 ` Lars Povlsen 2020-05-19 10:25 ` Lars Povlsen 2020-06-02 19:10 ` Serge Semin 2020-06-02 19:10 ` Serge Semin 2020-06-09 9:13 ` Lars Povlsen 2020-06-09 9:13 ` Lars Povlsen 2020-05-13 14:00 ` [PATCH 02/10] spi: dw: Add support for RX sample delay register Lars Povlsen 2020-05-13 14:00 ` Lars Povlsen 2020-06-02 19:39 ` Serge Semin 2020-06-02 19:39 ` Serge Semin 2020-06-09 10:04 ` Lars Povlsen 2020-06-09 10:04 ` Lars Povlsen 2020-05-13 14:00 ` [PATCH 03/10] spi: dw: Add support for client driver memory operations Lars Povlsen 2020-05-13 14:00 ` Lars Povlsen 2020-05-13 14:00 ` [PATCH 04/10] dt-bindings: spi: Add bindings for spi-dw-mchp Lars Povlsen 2020-05-13 14:00 ` Lars Povlsen 2020-05-13 14:52 ` Mark Brown 2020-05-19 11:47 ` Lars Povlsen 2020-05-19 11:47 ` Lars Povlsen 2020-05-19 11:58 ` Mark Brown 2020-05-19 12:10 ` Lars Povlsen 2020-05-19 12:10 ` Lars Povlsen 2020-06-02 19:49 ` Serge Semin 2020-06-02 19:49 ` Serge Semin 2020-06-09 10:27 ` Lars Povlsen 2020-06-09 10:27 ` Lars Povlsen 2020-05-13 14:00 ` [PATCH 05/10] spi: spi-dw-mmio: Spin off MSCC platforms into spi-dw-mchp Lars Povlsen 2020-05-13 14:00 ` Lars Povlsen 2020-05-13 15:18 ` Mark Brown 2020-05-19 12:05 ` Lars Povlsen 2020-05-19 12:05 ` Lars Povlsen 2020-06-02 21:12 ` Serge Semin [this message] 2020-06-02 21:12 ` Serge Semin 2020-06-10 14:28 ` Lars Povlsen 2020-06-10 14:28 ` Lars Povlsen 2020-05-13 14:00 ` [PATCH 06/10] dt-bindings: spi: spi-dw-mchp: Add Sparx5 support Lars Povlsen 2020-05-13 14:00 ` Lars Povlsen 2020-05-13 15:25 ` Mark Brown 2020-06-02 23:07 ` Serge Semin 2020-06-02 23:07 ` Serge Semin 2020-06-10 12:27 ` Lars Povlsen 2020-06-10 12:27 ` Lars Povlsen 2020-05-13 14:00 ` [PATCH 07/10] " Lars Povlsen 2020-05-13 14:00 ` Lars Povlsen 2020-05-14 10:25 ` Mark Brown 2020-05-19 9:29 ` Lars Povlsen 2020-05-19 9:29 ` Lars Povlsen 2020-06-02 23:22 ` Serge Semin 2020-06-02 23:22 ` Serge Semin 2020-05-13 14:00 ` [PATCH 08/10] arm64: dts: sparx5: Add SPI controller Lars Povlsen 2020-05-13 14:00 ` Lars Povlsen 2020-05-13 14:00 ` [PATCH 09/10] arm64: dts: sparx5: Add spi-nor support Lars Povlsen 2020-05-13 14:00 ` Lars Povlsen 2020-05-13 14:00 ` [PATCH 10/10] arm64: dts: sparx5: Add spi-nand devices Lars Povlsen 2020-05-13 14:00 ` Lars Povlsen 2020-05-29 16:21 ` [PATCH 00/10] spi: Adding support for Microchip Sparx5 SoC Serge Semin 2020-05-29 16:21 ` Serge Semin 2020-06-02 8:18 ` Lars Povlsen 2020-06-02 8:18 ` Lars Povlsen 2020-06-02 8:21 ` Serge Semin 2020-06-02 8:21 ` Serge Semin 2020-06-02 9:56 ` Mark Brown 2020-06-02 23:44 ` Serge Semin 2020-06-02 23:44 ` Serge Semin
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=20200602211203.3lad22zvt5yagane@mobilestation \ --to=sergey.semin@baikalelectronics.ru \ --cc=UNGLinuxDriver@microchip.com \ --cc=alexandre.belloni@bootlin.com \ --cc=broonie@kernel.org \ --cc=devicetree@vger.kernel.org \ --cc=fancer.lancer@gmail.com \ --cc=lars.povlsen@microchip.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-spi@vger.kernel.org \ --cc=soc@kernel.org \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.