* Re: [PATCH v3 2/2] mfd: ene-kb3930: Add driver for ENE KB3930 Embedded Controller
From: Lee Jones @ 2020-05-21 7:27 UTC (permalink / raw)
To: Lubomir Rintel; +Cc: Rob Herring, Mark Rutland, devicetree, linux-kernel
In-Reply-To: <20200520185955.2144252-3-lkundrak@v3.sk>
On Wed, 20 May 2020, Lubomir Rintel wrote:
> This driver provides access to the EC RAM of said embedded controller
> attached to the I2C bus as well as optionally supporting its slightly weird
> power-off/restart protocol.
>
> A particular implementation of the EC firmware can be identified by a
> model byte. If this driver identifies the Dell Ariel platform, it
> registers the appropriate cells.
>
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
>
> ---
> Changes since v2:
> - Sort the includes
> - s/EC_MODEL_ID/EC_MODEL/
> - Add a couple of clarifying comments
> - Use #defines for values used in poweroff routine
> - Remove priority from a restart notifier block
> - s/priv/ddata/
> - s/ec_ram/ram_regmap/ for the regmap name
> - Fix the error handling when getting off gpios was not successful
> - Remove a useless dev_info at the end of probe()
> - Use i2c probe_new() callback, drop i2c_device_id
> - Modify the logic in checking the model ID
>
> drivers/mfd/Kconfig | 10 ++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/ene-kb3930.c | 215 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 226 insertions(+)
> create mode 100644 drivers/mfd/ene-kb3930.c
Really starting to take shape.
Just a couple of nits, then we're good to go.
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 0a59249198d3..dae18a2beab5 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -398,6 +398,16 @@ config MFD_DLN2
> etc. must be enabled in order to use the functionality of
> the device.
>
> +config MFD_ENE_KB3930
> + tristate "ENE KB3930 Embedded Controller support"
> + depends on I2C
> + depends on MACH_MMP3_DT || COMPILE_TEST
> + select MFD_CORE
> + help
> + This adds support for accessing the registers on ENE KB3930, Embedded
> + Controller. Additional drivers such as LEDS_ARIEL must be enabled in
> + order to use the functionality of the device.
Can you mention/describe all of the sub-devices please?
[...]
> +struct kb3930 *global_kb3930;
Can we call this kb3930_power_off please.
[...]
> +static int kb3930_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct device_node *np = dev->of_node;
> + struct kb3930 *ddata;
> + unsigned int model;
> + int ret;
> +
> + if (global_kb3930)
> + return -EEXIST;
This should not happen. If .probe() is called twice, either
-EDEFER_PROBE was returned or a new device was registered.
[...]
> + /* These are the cells valid for model == 'J' only. */
> + ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
> + ariel_ec_cells,
> + ARRAY_SIZE(ariel_ec_cells),
> + NULL, 0, NULL);
> + if (ret < 0)
if (ret)
> + return ret;
[...]
> +static struct i2c_driver kb3930_driver = {
> + .probe_new = kb3930_probe,
> + .remove = kb3930_remove,
> + .driver = {
> + .name = "ene-kb3930",
> + .of_match_table = of_match_ptr(kb3930_dt_ids),
> + },
> +};
> +
Remove this line please.
> +module_i2c_driver(kb3930_driver);
> +
> +MODULE_AUTHOR("Lubomir Rintel <lkundrak@v3.sk>");
> +MODULE_DESCRIPTION("ENE KB3930 Embedded Controller Driver");
> +MODULE_LICENSE("Dual BSD/GPL");
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v3 00/14] mips: Prepare MIPS-arch code for Baikal-T1 SoC support
From: Thomas Bogendoerfer @ 2020-05-21 7:14 UTC (permalink / raw)
To: Serge Semin
Cc: Serge Semin, Alexey Malahov, Maxim Kaurkin, Pavel Parkhomenko,
Ramil Zaripov, Ekaterina Skachko, Vadim Vlasov, Alexey Kolotnikov,
Paul Burton, Ralf Baechle, Rob Herring, Frank Rowand,
Tony Lindgren, Arnd Bergmann, Olof Johansson, Greg Kroah-Hartman,
Jiri Slaby, linux-mips, devicetree, linux-kernel
In-Reply-To: <20200521004217.6gdcpboxaqizreky@mobilestation>
On Thu, May 21, 2020 at 03:42:17AM +0300, Serge Semin wrote:
> On Thu, May 21, 2020 at 03:34:29AM +0300, Serge Semin wrote:
> >
> > This patchset is rebased and tested on the mainline Linux kernel 5.7-rc4:
> > base-commit: 0e698dfa2822 ("Linux 5.7-rc4")
> > tag: v5.7-rc4
>
> Thomas,
> Please note that this patchset is based on the Linux 5.7-rc4 tree (it most likely
> will get cleanly applied on rc6 as well), while mips-next is still at rc1. Due
> to that the patchset fails to be applied on mips-next. I think it would be
> better first to merge the last Linux tree into the mips-next, then try to merge
> this patchset in. Should you have any problem after that, please let me know.
> I'll resend the patchset being rebased on top of the new mips-next tree.
no, that's not how it works. Please rebase your patches on top of
mips-next. Thank you.
Thomas.
--
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea. [ RFC1925, 2.3 ]
^ permalink raw reply
* Re: [PATCH v3 08/14] mips: MAAR: Use more precise address mask
From: Thomas Bogendoerfer @ 2020-05-21 7:11 UTC (permalink / raw)
To: Serge Semin
Cc: Serge Semin, Alexey Malahov, Paul Burton, Ralf Baechle,
Arnd Bergmann, Rob Herring, devicetree, Zhou Yanjie,
Paul Cercueil, Jiaxun Yang, linux-mips, linux-kernel
In-Reply-To: <20200521003443.11385-9-Sergey.Semin@baikalelectronics.ru>
On Thu, May 21, 2020 at 03:34:37AM +0300, Serge Semin wrote:
> Indeed according to the MIPS32 Privileged Resource Architecgture the MAAR
> pair register address field either takes [12:31] bits for non-XPA systems
> and [12:55] otherwise. In any case the current address mask is just
> wrong for 64-bit and 32-bits XPA chips. So lets extend it to 59-bits
> of physical address value. This shall cover the 64-bits architecture and
> systems with XPA enabled, and won't cause any problem for non-XPA 32-bit
> systems, since address values exceeding the architecture specific MAAR
> mask will be just truncated with setting zeros in the unsupported upper
> bits.
>
> Co-developed-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
>
> ---
>
> Changelog v3:
> - In accordance with MIPS32/64 Privileged Resource Architecture Extend
> the MAAR Addr mask to value [12:55] instead of P5600-specific [12:35].
> ---
> arch/mips/include/asm/mipsregs.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
applied to mips-next.
Thomas.
--
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea. [ RFC1925, 2.3 ]
^ permalink raw reply
* Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
From: Felipe Balbi @ 2020-05-21 6:22 UTC (permalink / raw)
To: Thinh Nguyen, Jun Li, Jun Li
Cc: John Stultz, lkml, Yu Chen, Greg Kroah-Hartman, Rob Herring,
Mark Rutland, ShuFan Lee, Heikki Krogerus, Suzuki K Poulose,
Chunfeng Yun, Hans de Goede, Andy Shevchenko, Valentin Schneider,
Jack Pham, Linux USB List,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Peter Chen
In-Reply-To: <87v9kpx0zh.fsf@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 1094 bytes --]
Hi Jun,
Felipe Balbi <balbi@kernel.org> writes:
>> In any case, increasing the timeout should be fine with me. It maybe
>> difficult to determine the max timeout base on the slowest clock rate
>> and number of cycles. Different controller and controller versions
>> behave differently and may have different number of clock cycles to
>> complete a command.
>>
>> The RTL engineer recommended timeout to be at least 1ms (which maybe
>> more than the polling rate of this patch). I'm fine with either the rate
>> provided by this tested patch or higher.
>
> A whole ms waiting for a command to complete? Wow, that's a lot of time
> blocking the CPU. It looks like, perhaps, we should move to command
> completion interrupts. The difficulty here is that we issue commands
> from within the interrupt handler and, as such, can't
> wait_for_completion().
>
> Meanwhile, we will take the timeout increase I guess, otherwise NXP
> won't have a working setup.
patch 1 in this series doesn't apply to testing/next. Care to rebase and
resend?
Thank you
--
balbi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
From: Felipe Balbi @ 2020-05-21 6:20 UTC (permalink / raw)
To: Thinh Nguyen, Jun Li, Jun Li
Cc: John Stultz, lkml, Yu Chen, Greg Kroah-Hartman, Rob Herring,
Mark Rutland, ShuFan Lee, Heikki Krogerus, Suzuki K Poulose,
Chunfeng Yun, Hans de Goede, Andy Shevchenko, Valentin Schneider,
Jack Pham, Linux USB List,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Peter Chen
In-Reply-To: <1c16bdb6-8d8d-1e1b-f08b-b3963f905eb0@synopsys.com>
[-- Attachment #1: Type: text/plain, Size: 3094 bytes --]
Hi,
Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>>>>>> "Power Down Scale (PwrDnScale)
>>>>>>>> The USB3 suspend_clk input replaces pipe3_rx_pclk as a clock source
>>>>>>>> to a small part of the USB3 controller that operates when the SS
>>>>>>>> PHY is in its lowest power (P3) state, and therefore does not provide a clock.
>>>>>>>> The Power Down Scale field specifies how many suspend_clk periods
>>>>>>>> fit into a 16 kHz clock period. When performing the division, round
>>>>>>>> up the remainder.
>>>>>>>> For example, when using an 8-bit/16-bit/32-bit PHY and 25-MHz
>>>>>>>> Suspend clock, Power Down Scale = 25000 kHz/16 kHz = 13'd1563
>>>>>>>> (rounder up)
>>>>>>>> Note:
>>>>>>>> - Minimum Suspend clock frequency is 32 kHz
>>>>>>>> - Maximum Suspend clock frequency is 125 MHz"
>>>>>>> Cool, now do we have an upper bound for how many clock cycles it
>>>>>>> takes to wake up the PHY?
>>>>>> My understanding is this ep command does not wake up the SS PHY, the
>>>>>> SS PHY still stays at P3 when execute this ep command. The time
>>>>>> required here is to wait controller complete something for this ep
>>>>>> command with 32K clock.
>>>>> Sorry I made a mistake. You're right. Just checked with one of the RTL
>>>>> engineers, and it doesn't need to wake up the phy. However, if it is
>>>>> eSS speed, it may take longer time as the command may be completing
>>>>> with the suspend clock.
>>>>>
>>>> What's the value for GCTL[7:6]?
>>> 2'b00
>>>
>>> Thanks
>>> Li Jun
>> (Sorry for the delay reply)
>>
>> If it's 0, then the ram clock should be the same as the bus_clk, which
>> is odd since you mentioned that the suspend_clk is used instead while in P3.
>
> Just checked with the RTL engineer, even if GCTL[7:6] is set to 0,
> internally it can still run with suspend clock during P3.
>
>> Anyway, I was looking for a way maybe to improve the speed during
>> issuing a command. One way is to set GUSB3PIPECTL[17]=0, and it should
>> wakeup the phy anytime. I think Felipe suggested it. It's odd that it
>> doesn't work for you. I don't have other ideas beside increasing the
>> command timeout.
>>
>
> In any case, increasing the timeout should be fine with me. It maybe
> difficult to determine the max timeout base on the slowest clock rate
> and number of cycles. Different controller and controller versions
> behave differently and may have different number of clock cycles to
> complete a command.
>
> The RTL engineer recommended timeout to be at least 1ms (which maybe
> more than the polling rate of this patch). I'm fine with either the rate
> provided by this tested patch or higher.
A whole ms waiting for a command to complete? Wow, that's a lot of time
blocking the CPU. It looks like, perhaps, we should move to command
completion interrupts. The difficulty here is that we issue commands
from within the interrupt handler and, as such, can't
wait_for_completion().
Meanwhile, we will take the timeout increase I guess, otherwise NXP
won't have a working setup.
--
balbi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* [PATCH v2 2/2] arm64: dts: imx8mn-ddr4-evk: correct ldo1/ldo2 voltage range
From: Robin Gong @ 2020-05-21 14:17 UTC (permalink / raw)
To: robh+dt, shawnguo, s.hauer, festevam, anson.huang, peng.fan
Cc: kernel, devicetree, linux-arm-kernel, linux-kernel, linux-imx
In-Reply-To: <1590070674-23027-1-git-send-email-yibin.gong@nxp.com>
Correct ldo1 voltage range from wrong high group(3.0v~3.3v) to low group
(1.6v~1.9v) because the ldo1 should be 1.8v. Actually, two voltage groups
have been supported at bd718x7-regulator driver, hence, just corrrect the
voltage range to 1.6v~3.3v. For ldo2@0.8v, correct voltage range too.
Otherwise, ldo1 would be kept @3.0v and ldo2@0.9v which violate i.mx8mn
datasheet as the below warning log in kernel:
[ 0.995524] LDO1: Bringing 1800000uV into 3000000-3000000uV
[ 0.999196] LDO2: Bringing 800000uV into 900000-900000uV
Signed-off-by: Robin Gong <yibin.gong@nxp.com>
---
arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts b/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
index d07e0e6..a1e5483 100644
--- a/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
@@ -113,7 +113,7 @@
ldo1_reg: LDO1 {
regulator-name = "LDO1";
- regulator-min-microvolt = <3000000>;
+ regulator-min-microvolt = <1600000>;
regulator-max-microvolt = <3300000>;
regulator-boot-on;
regulator-always-on;
@@ -121,7 +121,7 @@
ldo2_reg: LDO2 {
regulator-name = "LDO2";
- regulator-min-microvolt = <900000>;
+ regulator-min-microvolt = <800000>;
regulator-max-microvolt = <900000>;
regulator-boot-on;
regulator-always-on;
--
2.7.4
^ permalink raw reply related
* [PATCH v2 1/2] arm64: dts: imx8mm-evk: correct ldo1/ldo2 voltage range
From: Robin Gong @ 2020-05-21 14:17 UTC (permalink / raw)
To: robh+dt, shawnguo, s.hauer, festevam, anson.huang, peng.fan
Cc: kernel, devicetree, linux-arm-kernel, linux-kernel, linux-imx
Correct ldo1 voltage range from wrong high group(3.0v~3.3v) to low group
(1.6v~1.9v) because the ldo1 should be 1.8v. Actually, two voltage groups
have been supported at bd718x7-regulator driver, hence, just corrrect the
voltage range to 1.6v~3.3v. For ldo2@0.8v, correct voltage range too.
Otherwise, ldo1 would be kept @3.0v and ldo2@0.9v which violate i.mx8mm
datasheet as the below warning log in kernel:
[ 0.995524] LDO1: Bringing 1800000uV into 3000000-3000000uV
[ 0.999196] LDO2: Bringing 800000uV into 900000-900000uV
Signed-off-by: Robin Gong <yibin.gong@nxp.com>
---
arch/arm64/boot/dts/freescale/imx8mm-evk.dts | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/freescale/imx8mm-evk.dts b/arch/arm64/boot/dts/freescale/imx8mm-evk.dts
index e5ec832..0f1d7f8 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm-evk.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mm-evk.dts
@@ -208,7 +208,7 @@
ldo1_reg: LDO1 {
regulator-name = "LDO1";
- regulator-min-microvolt = <3000000>;
+ regulator-min-microvolt = <1600000>;
regulator-max-microvolt = <3300000>;
regulator-boot-on;
regulator-always-on;
@@ -216,7 +216,7 @@
ldo2_reg: LDO2 {
regulator-name = "LDO2";
- regulator-min-microvolt = <900000>;
+ regulator-min-microvolt = <800000>;
regulator-max-microvolt = <900000>;
regulator-boot-on;
regulator-always-on;
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v10 1/5] usb: xhci: Change the XHCI link order in the Makefile
From: Greg Kroah-Hartman @ 2020-05-21 6:09 UTC (permalink / raw)
To: Alan Cooper
Cc: Florian Fainelli, Alan Stern, : Linux Kernel Mailing List,
Andy Shevchenko, DTML, Krzysztof Kozlowski, USB list,
Mathias Nyman, Rob Herring, Yoshihiro Shimoda, Andy Shevchenko
In-Reply-To: <CAOGqxeV_9LpFudC3VuFPAOmYVYBQGJ+s01HCK2nP6XxpprK2Qw@mail.gmail.com>
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?
A: No.
Q: Should I include quotations after my reply?
http://daringfireball.net/2007/07/on_top
On Wed, May 20, 2020 at 01:29:45PM -0400, Alan Cooper wrote:
> Greg, Alan,
>
> The other 4 related patches were accepted into usb-next and I just
> realized that this one didn't make it. This patch will not fix the
> "insmod out of order" issue, but will help our controllers work with
> some poorly behaved USB devices when the drivers are builtin.
As it doesn't solve the real issue, I did not accept this so that you
all can continue to work on creating a real solution that works for both
situations (built in and as modules.)
I thought I said that already...
thanks,
greg k-h
^ permalink raw reply
* RE: [PATCH v1 2/2] arm64: dts: imx8mn-ddr4-evk: correct ldo1/ldo2 voltage range
From: Robin Gong @ 2020-05-21 6:05 UTC (permalink / raw)
To: Peng Fan, robh+dt@kernel.org, shawnguo@kernel.org,
s.hauer@pengutronix.de, festevam@gmail.com, Anson Huang
Cc: kernel@pengutronix.de, dl-linux-imx, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
In-Reply-To: <DB6PR0402MB2760907E5DE55EBA3BE38D9B88B70@DB6PR0402MB2760.eurprd04.prod.outlook.com>
2020/05/21 14:02 Peng Fan <peng.fan@nxp.com> wrote:
> > Subject: [PATCH v1 2/2] arm64: dts: imx8mn-ddr4-evk: correct ldo1/ldo2
> > voltage range
> >
> > Correct ldo1 voltage range from wrong high group(3.0v~3.3v) to low
> > group
> > (1.6v~1.9v) because the ldo1 should be 1.8v. Actually, two voltage
> > groups have been supported at bd718x7-regulator driver, hence, just
> > corrrect the voltage range to 1.6v~3.3v. For ldo2@0.8v, correct voltage range
> too.
> > Otherwise, ldo1 would be kept @3.0v and ldo2@0.9v which violate
> > i.mx8mm datasheet as the below warning log in kernel:
> >
> > [ 0.995524] LDO1: Bringing 1800000uV into 3000000-3000000uV
> > [ 0.999196] LDO2: Bringing 800000uV into 900000-900000uV
> >
> > Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> > ---
> > arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts | 4 ++--
> > arch/arm64/boot/dts/freescale/imx8mn-evk.dts | 9 +++++++++
> > 2 files changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
> > b/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
> > index d07e0e6..a1e5483 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
> > +++ b/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
> > @@ -113,7 +113,7 @@
> >
> > ldo1_reg: LDO1 {
> > regulator-name = "LDO1";
> > - regulator-min-microvolt = <3000000>;
> > + regulator-min-microvolt = <1600000>;
> > regulator-max-microvolt = <3300000>;
> > regulator-boot-on;
> > regulator-always-on;
> > @@ -121,7 +121,7 @@
> >
> > ldo2_reg: LDO2 {
> > regulator-name = "LDO2";
> > - regulator-min-microvolt = <900000>;
> > + regulator-min-microvolt = <800000>;
> > regulator-max-microvolt = <900000>;
> > regulator-boot-on;
> > regulator-always-on;
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mn-evk.dts
> > b/arch/arm64/boot/dts/freescale/imx8mn-evk.dts
> > index 61f3519..117ff4b 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mn-evk.dts
> > +++ b/arch/arm64/boot/dts/freescale/imx8mn-evk.dts
> > @@ -13,6 +13,15 @@
> > compatible = "fsl,imx8mn-evk", "fsl,imx8mn"; };
> >
> > +&ecspi1 {
> > + status = "okay";
> > +spidev0: spi@0 {
> > + compatible = "ge,achc";
> > + reg = <0>;
> > + spi-max-frequency = <1000000>;
> > + };
> > +};
> > +
>
> This was added by mistake?
Sorry, will send out v2.
^ permalink raw reply
* RE: [PATCH v1 2/2] arm64: dts: imx8mn-ddr4-evk: correct ldo1/ldo2 voltage range
From: Peng Fan @ 2020-05-21 6:02 UTC (permalink / raw)
To: Robin Gong, robh+dt@kernel.org, shawnguo@kernel.org,
s.hauer@pengutronix.de, festevam@gmail.com, Anson Huang
Cc: kernel@pengutronix.de, dl-linux-imx, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
In-Reply-To: <1590060368-2282-2-git-send-email-yibin.gong@nxp.com>
> Subject: [PATCH v1 2/2] arm64: dts: imx8mn-ddr4-evk: correct ldo1/ldo2
> voltage range
>
> Correct ldo1 voltage range from wrong high group(3.0v~3.3v) to low group
> (1.6v~1.9v) because the ldo1 should be 1.8v. Actually, two voltage groups
> have been supported at bd718x7-regulator driver, hence, just corrrect the
> voltage range to 1.6v~3.3v. For ldo2@0.8v, correct voltage range too.
> Otherwise, ldo1 would be kept @3.0v and ldo2@0.9v which violate i.mx8mm
> datasheet as the below warning log in kernel:
>
> [ 0.995524] LDO1: Bringing 1800000uV into 3000000-3000000uV
> [ 0.999196] LDO2: Bringing 800000uV into 900000-900000uV
>
> Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> ---
> arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts | 4 ++--
> arch/arm64/boot/dts/freescale/imx8mn-evk.dts | 9 +++++++++
> 2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
> b/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
> index d07e0e6..a1e5483 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
> @@ -113,7 +113,7 @@
>
> ldo1_reg: LDO1 {
> regulator-name = "LDO1";
> - regulator-min-microvolt = <3000000>;
> + regulator-min-microvolt = <1600000>;
> regulator-max-microvolt = <3300000>;
> regulator-boot-on;
> regulator-always-on;
> @@ -121,7 +121,7 @@
>
> ldo2_reg: LDO2 {
> regulator-name = "LDO2";
> - regulator-min-microvolt = <900000>;
> + regulator-min-microvolt = <800000>;
> regulator-max-microvolt = <900000>;
> regulator-boot-on;
> regulator-always-on;
> diff --git a/arch/arm64/boot/dts/freescale/imx8mn-evk.dts
> b/arch/arm64/boot/dts/freescale/imx8mn-evk.dts
> index 61f3519..117ff4b 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mn-evk.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mn-evk.dts
> @@ -13,6 +13,15 @@
> compatible = "fsl,imx8mn-evk", "fsl,imx8mn"; };
>
> +&ecspi1 {
> + status = "okay";
> +spidev0: spi@0 {
> + compatible = "ge,achc";
> + reg = <0>;
> + spi-max-frequency = <1000000>;
> + };
> +};
> +
This was added by mistake?
Regards,
Peng.
> &A53_0 {
> /delete-property/operating-points-v2;
> };
> --
> 2.7.4
^ permalink raw reply
* Re: [PATCH 2/6] soc: ti: omap-prm: Add basic power domain support
From: kbuild test robot @ 2020-05-21 4:53 UTC (permalink / raw)
To: Tony Lindgren, linux-omap
Cc: kbuild-all, Andrew F . Davis, Santosh Shilimkar, Suman Anna,
Tero Kristo, linux-kernel, linux-arm-kernel, Rob Herring,
devicetree
In-Reply-To: <20200520211334.61814-3-tony@atomide.com>
[-- Attachment #1: Type: text/plain, Size: 2779 bytes --]
Hi Tony,
I love your patch! Perhaps something to improve:
[auto build test WARNING on omap/for-next]
[also build test WARNING on robh/for-next keystone/next v5.7-rc6 next-20200519]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Tony-Lindgren/Add-initial-genpd-support-for-omap-PRM-driver/20200521-063328
base: https://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git for-next
config: arm-defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>, old ones prefixed by <<):
drivers/soc/ti/omap_prm.c: In function 'omap_prm_domain_detach_dev':
>> drivers/soc/ti/omap_prm.c:347:26: warning: variable 'prmd' set but not used [-Wunused-but-set-variable]
347 | struct omap_prm_domain *prmd;
| ^~~~
At top level:
drivers/soc/ti/omap_prm.c:123:21: warning: 'omap_prm_onoff_noauto' defined but not used [-Wunused-const-variable=]
123 | omap_prm_domain_map omap_prm_onoff_noauto = {
| ^~~~~~~~~~~~~~~~~~~~~
drivers/soc/ti/omap_prm.c:115:21: warning: 'omap_prm_nooff' defined but not used [-Wunused-const-variable=]
115 | omap_prm_domain_map omap_prm_nooff = {
| ^~~~~~~~~~~~~~
drivers/soc/ti/omap_prm.c:107:21: warning: 'omap_prm_noinact' defined but not used [-Wunused-const-variable=]
107 | omap_prm_domain_map omap_prm_noinact = {
| ^~~~~~~~~~~~~~~~
drivers/soc/ti/omap_prm.c:99:21: warning: 'omap_prm_all' defined but not used [-Wunused-const-variable=]
99 | omap_prm_domain_map omap_prm_all = {
| ^~~~~~~~~~~~
vim +/prmd +347 drivers/soc/ti/omap_prm.c
342
343 static void omap_prm_domain_detach_dev(struct generic_pm_domain *domain,
344 struct device *dev)
345 {
346 struct generic_pm_domain_data *genpd_data;
> 347 struct omap_prm_domain *prmd;
348
349 prmd = genpd_to_prm_domain(domain);
350
351 genpd_data = dev_gpd_data(dev);
352 genpd_data->data = NULL;
353 }
354
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 50472 bytes --]
^ permalink raw reply
* [PATCH] ASoC: dt-bindings: simple-card: care missing address #address-cells
From: Kuninori Morimoto @ 2020-05-21 3:54 UTC (permalink / raw)
To: Liam Girdwood, Mark Brown, Rob Herring, Mark Rutland
Cc: devicetree, alsa-devel, linux-kernel
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Current simple-card will get below error,
because it doesn't care about #address-cells at some part.
DTC Documentation/devicetree/bindings/sound/simple-card.example.dt.yaml
Documentation/devicetree/bindings/sound/simple-card.example.dts:171.46-173.15: \
Warning (unit_address_vs_reg): /example-4/sound/simple-audio-card,cpu@0: \
node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/sound/simple-card.example.dts:175.37-177.15: \
Warning (unit_address_vs_reg): /example-4/sound/simple-audio-card,cpu@1: \
node has a unit name, but no reg or ranges property
...
This patch fixup this issue.
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
.../bindings/sound/simple-card.yaml | 25 ++++++++++++++++++-
1 file changed, 24 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/sound/simple-card.yaml b/Documentation/devicetree/bindings/sound/simple-card.yaml
index cb2bb5fac0e1..6c4c2c6d6d3c 100644
--- a/Documentation/devicetree/bindings/sound/simple-card.yaml
+++ b/Documentation/devicetree/bindings/sound/simple-card.yaml
@@ -208,6 +208,11 @@ patternProperties:
reg:
maxItems: 1
+ "#address-cells":
+ const: 1
+ "#size-cells":
+ const: 0
+
# common properties
frame-master:
$ref: "#/definitions/frame-master"
@@ -288,7 +293,6 @@ examples:
#address-cells = <1>;
#size-cells = <0>;
-
simple-audio-card,dai-link@0 { /* I2S - HDMI */
reg = <0>;
format = "i2s";
@@ -392,11 +396,15 @@ examples:
simple-audio-card,routing = "ak4642 Playback", "DAI0 Playback",
"ak4642 Playback", "DAI1 Playback";
+ #address-cells = <1>;
+ #size-cells = <0>;
dpcmcpu: simple-audio-card,cpu@0 {
+ reg = <0>;
sound-dai = <&rcar_sound 0>;
};
simple-audio-card,cpu@1 {
+ reg = <1>;
sound-dai = <&rcar_sound 1>;
};
@@ -427,7 +435,12 @@ examples:
"pcm3168a Playback", "DAI3 Playback",
"pcm3168a Playback", "DAI4 Playback";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
simple-audio-card,dai-link@0 {
+ reg = <0>;
+
format = "left_j";
bitclock-master = <&sndcpu0>;
frame-master = <&sndcpu0>;
@@ -441,22 +454,30 @@ examples:
};
simple-audio-card,dai-link@1 {
+ reg = <1>;
+
format = "i2s";
bitclock-master = <&sndcpu1>;
frame-master = <&sndcpu1>;
convert-channels = <8>; /* TDM Split */
+ #address-cells = <1>;
+ #size-cells = <0>;
sndcpu1: cpu@0 {
+ reg = <0>;
sound-dai = <&rcar_sound 1>;
};
cpu@1 {
+ reg = <1>;
sound-dai = <&rcar_sound 2>;
};
cpu@2 {
+ reg = <2>;
sound-dai = <&rcar_sound 3>;
};
cpu@3 {
+ reg = <3>;
sound-dai = <&rcar_sound 4>;
};
codec {
@@ -468,6 +489,8 @@ examples:
};
simple-audio-card,dai-link@2 {
+ reg = <2>;
+
format = "i2s";
bitclock-master = <&sndcpu2>;
frame-master = <&sndcpu2>;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v3 4/4] PCI: cadence: Use "dma-ranges" instead of "cdns,no-bar-match-nbits" property
From: Kishon Vijay Abraham I @ 2020-05-21 3:30 UTC (permalink / raw)
To: Rob Herring
Cc: Bjorn Helgaas, Lorenzo Pieralisi, Robin Murphy, Tom Joseph, PCI,
devicetree, linux-kernel@vger.kernel.org,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <CAL_JsqJ1Om2CX5e1y32bzeiuv4fAdyFpZ88a346g4Q+jq_Ldcg@mail.gmail.com>
Hi Rob,
On 5/19/2020 10:41 PM, Rob Herring wrote:
> On Fri, May 8, 2020 at 7:07 AM Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>
>> Cadence PCIe core driver (host mode) uses "cdns,no-bar-match-nbits"
>> property to configure the number of bits passed through from PCIe
>> address to internal address in Inbound Address Translation register.
>> This only used the NO MATCH BAR.
>>
>> However standard PCI dt-binding already defines "dma-ranges" to
>> describe the address ranges accessible by PCIe controller. Add support
>> in Cadence PCIe host driver to parse dma-ranges and configure the
>> inbound regions for BAR0, BAR1 and NO MATCH BAR. Cadence IP specifies
>> maximum size for BAR0 as 256GB, maximum size for BAR1 as 2 GB, so if
>> the dma-ranges specifies a size larger than the maximum allowed, the
>> driver will split and configure the BARs.
>
> Would be useful to know what your dma-ranges contains now.
>
>
>> Legacy device tree binding compatibility is maintained by retaining
>> support for "cdns,no-bar-match-nbits".
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>> .../controller/cadence/pcie-cadence-host.c | 141 ++++++++++++++++--
>> drivers/pci/controller/cadence/pcie-cadence.h | 17 ++-
>> 2 files changed, 141 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
>> index 6ecebb79057a..2485ecd8434d 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
>> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
>> @@ -11,6 +11,12 @@
>>
>> #include "pcie-cadence.h"
>>
>> +static u64 cdns_rp_bar_max_size[] = {
>> + [RP_BAR0] = _ULL(128 * SZ_2G),
>> + [RP_BAR1] = SZ_2G,
>> + [RP_NO_BAR] = SZ_64T,
>> +};
>> +
>> void __iomem *cdns_pci_map_bus(struct pci_bus *bus, unsigned int devfn,
>> int where)
>> {
>> @@ -106,6 +112,117 @@ static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)
>> return 0;
>> }
>>
>> +static void cdns_pcie_host_bar_ib_config(struct cdns_pcie_rc *rc,
>> + enum cdns_pcie_rp_bar bar,
>> + u64 cpu_addr, u32 aperture)
>> +{
>> + struct cdns_pcie *pcie = &rc->pcie;
>> + u32 addr0, addr1;
>> +
>> + addr0 = CDNS_PCIE_AT_IB_RP_BAR_ADDR0_NBITS(aperture) |
>> + (lower_32_bits(cpu_addr) & GENMASK(31, 8));
>> + addr1 = upper_32_bits(cpu_addr);
>> + cdns_pcie_writel(pcie, CDNS_PCIE_AT_IB_RP_BAR_ADDR0(bar), addr0);
>> + cdns_pcie_writel(pcie, CDNS_PCIE_AT_IB_RP_BAR_ADDR1(bar), addr1);
>> +}
>> +
>> +static int cdns_pcie_host_bar_config(struct cdns_pcie_rc *rc,
>> + struct resource_entry *entry,
>> + enum cdns_pcie_rp_bar *index)
>> +{
>> + u64 cpu_addr, pci_addr, size, winsize;
>> + struct cdns_pcie *pcie = &rc->pcie;
>> + struct device *dev = pcie->dev;
>> + enum cdns_pcie_rp_bar bar;
>> + unsigned long flags;
>> + u32 aperture;
>> + u32 value;
>> +
>> + cpu_addr = entry->res->start;
>> + flags = entry->res->flags;
>> + pci_addr = entry->res->start - entry->offset;
>> + size = resource_size(entry->res);
>> + bar = *index;
>> +
>> + if (entry->offset) {
>> + dev_err(dev, "Cannot map PCI addr: %llx to CPU addr: %llx\n",
>> + pci_addr, cpu_addr);
>
> Would be a bit more clear to say PCI addr must equal CPU addr.
>
>> + return -EINVAL;
>> + }
>> +
>> + value = cdns_pcie_readl(pcie, CDNS_PCIE_LM_RC_BAR_CFG);
>> + while (size > 0) {
>> + if (bar > RP_NO_BAR) {
>> + dev_err(dev, "Failed to map inbound regions!\n");
>> + return -EINVAL;
>> + }
>> +
>> + winsize = size;
>> + if (size > cdns_rp_bar_max_size[bar])
>> + winsize = cdns_rp_bar_max_size[bar];
>> +
>> + aperture = ilog2(winsize);
>> +
>> + cdns_pcie_host_bar_ib_config(rc, bar, cpu_addr, aperture);
>> +
>> + if (bar == RP_NO_BAR)
>> + break;
>> +
>> + if (winsize + cpu_addr >= SZ_4G) {
>> + if (!(flags & IORESOURCE_PREFETCH))
>> + value |= LM_RC_BAR_CFG_CTRL_MEM_64BITS(bar);
>> + value |= LM_RC_BAR_CFG_CTRL_PREF_MEM_64BITS(bar);
>> + } else {
>> + if (!(flags & IORESOURCE_PREFETCH))
>> + value |= LM_RC_BAR_CFG_CTRL_MEM_32BITS(bar);
>> + value |= LM_RC_BAR_CFG_CTRL_PREF_MEM_32BITS(bar);
>> + }
>> +
>> + value |= LM_RC_BAR_CFG_APERTURE(bar, aperture);
>> +
>> + size -= winsize;
>> + cpu_addr += winsize;
>> + bar++;
>> + }
>> + cdns_pcie_writel(pcie, CDNS_PCIE_LM_RC_BAR_CFG, value);
>> + *index = bar;
>> +
>> + return 0;
>> +}
>> +
>> +static int cdns_pcie_host_map_dma_ranges(struct cdns_pcie_rc *rc)
>> +{
>> + enum cdns_pcie_rp_bar bar = RP_BAR0;
>> + struct cdns_pcie *pcie = &rc->pcie;
>> + struct device *dev = pcie->dev;
>> + struct device_node *np = dev->of_node;
>> + struct pci_host_bridge *bridge;
>> + struct resource_entry *entry;
>> + u32 no_bar_nbits = 32;
>> + int err;
>> +
>> + bridge = pci_host_bridge_from_priv(rc);
>> + if (!bridge)
>> + return -ENOMEM;
>> +
>> + if (list_empty(&bridge->dma_ranges)) {
>> + of_property_read_u32(np, "cdns,no-bar-match-nbits",
>> + &no_bar_nbits);
>> + cdns_pcie_host_bar_ib_config(rc, RP_NO_BAR, 0x0, no_bar_nbits);
>> + return 0;
>> + }
>> +
>> + resource_list_for_each_entry(entry, &bridge->dma_ranges) {
>> + err = cdns_pcie_host_bar_config(rc, entry, &bar);
>
> Seems like this should have some better logic to pick which BAR to
> use. Something like find the biggest region and then find the smallest
> BAR that it fits in. Then get the next biggest...
Okay, I'll change this something like for each region, find the smallest BAR
that it fits in and if there is no BAR big enough to hold the region, split the
region to see if can be fitted using multiple BARs. I don't see the purpose of
finding the biggest region first since at all times we'll only use the smallest
BAR to fit.
Thanks
Kishon
^ permalink raw reply
* [PATCH v1 2/2] arm64: dts: imx8mn-ddr4-evk: correct ldo1/ldo2 voltage range
From: Robin Gong @ 2020-05-21 11:26 UTC (permalink / raw)
To: robh+dt, shawnguo, s.hauer, festevam, Anson.Huang
Cc: kernel, linux-imx, devicetree, linux-arm-kernel, linux-kernel
In-Reply-To: <1590060368-2282-1-git-send-email-yibin.gong@nxp.com>
Correct ldo1 voltage range from wrong high group(3.0v~3.3v) to low group
(1.6v~1.9v) because the ldo1 should be 1.8v. Actually, two voltage groups
have been supported at bd718x7-regulator driver, hence, just corrrect the
voltage range to 1.6v~3.3v. For ldo2@0.8v, correct voltage range too.
Otherwise, ldo1 would be kept @3.0v and ldo2@0.9v which violate i.mx8mm
datasheet as the below warning log in kernel:
[ 0.995524] LDO1: Bringing 1800000uV into 3000000-3000000uV
[ 0.999196] LDO2: Bringing 800000uV into 900000-900000uV
Signed-off-by: Robin Gong <yibin.gong@nxp.com>
---
arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts | 4 ++--
arch/arm64/boot/dts/freescale/imx8mn-evk.dts | 9 +++++++++
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts b/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
index d07e0e6..a1e5483 100644
--- a/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mn-ddr4-evk.dts
@@ -113,7 +113,7 @@
ldo1_reg: LDO1 {
regulator-name = "LDO1";
- regulator-min-microvolt = <3000000>;
+ regulator-min-microvolt = <1600000>;
regulator-max-microvolt = <3300000>;
regulator-boot-on;
regulator-always-on;
@@ -121,7 +121,7 @@
ldo2_reg: LDO2 {
regulator-name = "LDO2";
- regulator-min-microvolt = <900000>;
+ regulator-min-microvolt = <800000>;
regulator-max-microvolt = <900000>;
regulator-boot-on;
regulator-always-on;
diff --git a/arch/arm64/boot/dts/freescale/imx8mn-evk.dts b/arch/arm64/boot/dts/freescale/imx8mn-evk.dts
index 61f3519..117ff4b 100644
--- a/arch/arm64/boot/dts/freescale/imx8mn-evk.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mn-evk.dts
@@ -13,6 +13,15 @@
compatible = "fsl,imx8mn-evk", "fsl,imx8mn";
};
+&ecspi1 {
+ status = "okay";
+spidev0: spi@0 {
+ compatible = "ge,achc";
+ reg = <0>;
+ spi-max-frequency = <1000000>;
+ };
+};
+
&A53_0 {
/delete-property/operating-points-v2;
};
--
2.7.4
^ permalink raw reply related
* [PATCH v1 1/2] arm64: dts: imx8mm-evk: correct ldo1/ldo2 voltage range
From: Robin Gong @ 2020-05-21 11:26 UTC (permalink / raw)
To: robh+dt, shawnguo, s.hauer, festevam, Anson.Huang
Cc: kernel, linux-imx, devicetree, linux-arm-kernel, linux-kernel
Correct ldo1 voltage range from wrong high group(3.0v~3.3v) to low group
(1.6v~1.9v) because the ldo1 should be 1.8v. Actually, two voltage groups
have been supported at bd718x7-regulator driver, hence, just corrrect the
voltage range to 1.6v~3.3v. For ldo2@0.8v, correct voltage range too.
Otherwise, ldo1 would be kept @3.0v and ldo2@0.9v which violate i.mx8mm
datasheet as the below warning log in kernel:
[ 0.995524] LDO1: Bringing 1800000uV into 3000000-3000000uV
[ 0.999196] LDO2: Bringing 800000uV into 900000-900000uV
Signed-off-by: Robin Gong <yibin.gong@nxp.com>
---
arch/arm64/boot/dts/freescale/imx8mm-evk.dts | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/freescale/imx8mm-evk.dts b/arch/arm64/boot/dts/freescale/imx8mm-evk.dts
index e5ec832..0f1d7f8 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm-evk.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mm-evk.dts
@@ -208,7 +208,7 @@
ldo1_reg: LDO1 {
regulator-name = "LDO1";
- regulator-min-microvolt = <3000000>;
+ regulator-min-microvolt = <1600000>;
regulator-max-microvolt = <3300000>;
regulator-boot-on;
regulator-always-on;
@@ -216,7 +216,7 @@
ldo2_reg: LDO2 {
regulator-name = "LDO2";
- regulator-min-microvolt = <900000>;
+ regulator-min-microvolt = <800000>;
regulator-max-microvolt = <900000>;
regulator-boot-on;
regulator-always-on;
--
2.7.4
^ permalink raw reply related
* RE: [EXT] Re: [PATCH net 3/4] ARM: dts: imx6: update fec gpr property to match new format
From: Andy Duan @ 2020-05-21 3:15 UTC (permalink / raw)
To: Andrew Lunn
Cc: davem@davemloft.net, netdev@vger.kernel.org,
martin.fuzzey@flowbird.group, robh+dt@kernel.org,
shawnguo@kernel.org, devicetree@vger.kernel.org
In-Reply-To: <20200520170322.GJ652285@lunn.ch>
From: Andrew Lunn <andrew@lunn.ch> Sent: Thursday, May 21, 2020 1:03 AM
> On Wed, May 20, 2020 at 04:31:55PM +0800, fugang.duan@nxp.com wrote:
> > From: Fugang Duan <fugang.duan@nxp.com>
> >
> > Update the gpr property to define gpr register offset and bit in DT.
> >
> > Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
> > ---
> > arch/arm/boot/dts/imx6qdl.dtsi | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/boot/dts/imx6qdl.dtsi
> > b/arch/arm/boot/dts/imx6qdl.dtsi index 98da446..a4a68b7 100644
> > --- a/arch/arm/boot/dts/imx6qdl.dtsi
> > +++ b/arch/arm/boot/dts/imx6qdl.dtsi
> > @@ -1045,7 +1045,7 @@
> > <&clks
> IMX6QDL_CLK_ENET>,
> > <&clks
> IMX6QDL_CLK_ENET_REF>;
> > clock-names = "ipg", "ahb", "ptp";
> > - gpr = <&gpr>;
> > + gpr = <&gpr 0x34 27>;
> > status = "disabled";
> > };
>
> Hi Andy
>
> This is the same values as hard coded, so no change here.
>
> The next patch does not use grp at all. So it is unclear to me if you actually
> make use of what you just added. I don't see anywhere
>
> gpr = <&gpr 0x42 24>;
>
> which is the whole point of this change, being able to specify different values.
>
> Andrew
Andrew, patch#1 in the series will parse the property to get register offset and bit.
Patch#2 describes the property format as below:
<&gpr req_gpr req_bit>.
gpr is the phandle to general purpose register node.
req_gpr is the gpr register offset for ENET stop request.
req_bit is the gpr bit offset for ENET stop request.
All i.MX support wake-on-lan, imx6q/dl/qp is the first platforms in upstream to support it.
As you know, most of i.MX chips has two ethernet instances, they have different gpr bit.
gpr is used to enter/exit stop mode for soc. So it can be defined in dtsi file.
"fsl,magic-packet;" property is define the board wakeup capability.
I am not sure whether above information is clear for you why to add the patch set.
Andy
^ permalink raw reply
* Re: [PATCH v3 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA
From: Feng Tang @ 2020-05-21 3:09 UTC (permalink / raw)
To: Serge Semin
Cc: Mark Brown, Grant Likely, Vinod Koul, Alan Cox, Linus Walleij,
Serge Semin, Georgy Vlasov, Ramil Zaripov, Alexey Malahov,
Thomas Bogendoerfer, Paul Burton, Ralf Baechle, Arnd Bergmann,
Andy Shevchenko, Rob Herring, linux-mips, devicetree,
Jarkko Nikula, Thomas Gleixner, Wan Ahmad Zainie, Linus Walleij,
Clement Leger, linux-spi, linux-kernel
In-Reply-To: <20200521012206.14472-2-Sergey.Semin@baikalelectronics.ru>
Hi Serge,
On Thu, May 21, 2020 at 04:21:51AM +0300, Serge Semin wrote:
> Since DMA transfers are performed asynchronously with actual SPI
> transaction, then even if DMA transfers are finished it doesn't mean
> all data is actually pushed to the SPI bus. Some data might still be
> in the controller FIFO. This is specifically true for Tx-only
> transfers. In this case if the next SPI transfer is recharged while
> a tail of the previous one is still in FIFO, we'll loose that tail
> data. In order to fix this lets add the wait procedure of the Tx/Rx
> SPI transfers completion after the corresponding DMA transactions
> are finished.
>
> Co-developed-by: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
> Signed-off-by: Georgy Vlasov <Georgy.Vlasov@baikalelectronics.ru>
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Fixes: 7063c0d942a1 ("spi/dw_spi: add DMA support")
> Cc: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: linux-mips@vger.kernel.org
> Cc: devicetree@vger.kernel.org
>
> ---
>
> Changelog v2:
> - Use conditional statement instead of the ternary operator in the ref
> clock getter.
> - Move the patch to the head of the series so one could be picked up to
> the stable kernels as a fix.
>
> Changelog v3:
> - Use spi_delay_exec() method to wait for the current operation completion.
> ---
> drivers/spi/spi-dw-mid.c | 69 ++++++++++++++++++++++++++++++++++++++++
> drivers/spi/spi-dw.h | 10 ++++++
> 2 files changed, 79 insertions(+)
>
> diff --git a/drivers/spi/spi-dw-mid.c b/drivers/spi/spi-dw-mid.c
> index f9757a370699..3526b196a7fc 100644
> --- a/drivers/spi/spi-dw-mid.c
> +++ b/drivers/spi/spi-dw-mid.c
> @@ -17,6 +17,7 @@
> #include <linux/pci.h>
> #include <linux/platform_data/dma-dw.h>
>
> +#define WAIT_RETRIES 5
> #define RX_BUSY 0
> #define TX_BUSY 1
>
> @@ -143,6 +144,47 @@ static enum dma_slave_buswidth convert_dma_width(u32 dma_width) {
> return DMA_SLAVE_BUSWIDTH_UNDEFINED;
> }
>
> +static void dw_spi_dma_calc_delay(struct dw_spi *dws, u32 nents,
> + struct spi_delay *delay)
> +{
> + unsigned long ns, us;
> +
> + ns = (NSEC_PER_SEC / spi_get_clk(dws)) * nents * dws->n_bytes *
> + BITS_PER_BYTE;
> +
> + if (ns <= NSEC_PER_USEC) {
> + delay->unit = SPI_DELAY_UNIT_NSECS;
> + delay->value = ns;
> + } else {
> + us = DIV_ROUND_UP(ns, NSEC_PER_USEC);
> + delay->unit = SPI_DELAY_UNIT_USECS;
> + delay->value = clamp_val(us, 0, USHRT_MAX);
> + }
> +}
> +
> +static inline bool dw_spi_dma_tx_busy(struct dw_spi *dws)
> +{
> + return !(dw_readl(dws, DW_SPI_SR) & SR_TF_EMPT);
> +}
> +
> +static void dw_spi_dma_wait_tx_done(struct dw_spi *dws)
> +{
> + int retry = WAIT_RETRIES;
> + struct spi_delay delay;
> + u32 nents;
> +
> + nents = dw_readl(dws, DW_SPI_TXFLR);
> + dw_spi_dma_calc_delay(dws, nents, &delay);
> +
> + while (dw_spi_dma_tx_busy(dws) && retry--)
> + spi_delay_exec(&delay, NULL);
> +
> + if (retry < 0) {
> + dev_err(&dws->master->dev, "Tx hanged up\n");
> + dws->master->cur_msg->status = -EIO;
> + }
> +}
> +
> /*
> * dws->dma_chan_busy is set before the dma transfer starts, callback for tx
> * channel will clear a corresponding bit.
> @@ -151,6 +193,8 @@ static void dw_spi_dma_tx_done(void *arg)
> {
> struct dw_spi *dws = arg;
>
> + dw_spi_dma_wait_tx_done(dws);
> +
> clear_bit(TX_BUSY, &dws->dma_chan_busy);
> if (test_bit(RX_BUSY, &dws->dma_chan_busy))
> return;
> @@ -192,6 +236,29 @@ static struct dma_async_tx_descriptor *dw_spi_dma_prepare_tx(struct dw_spi *dws,
> return txdesc;
> }
>
> +static inline bool dw_spi_dma_rx_busy(struct dw_spi *dws)
> +{
> + return !!(dw_readl(dws, DW_SPI_SR) & SR_RF_NOT_EMPT);
> +}
> +
> +static void dw_spi_dma_wait_rx_done(struct dw_spi *dws)
> +{
> + int retry = WAIT_RETRIES;
> + struct spi_delay delay;
> + u32 nents;
> +
> + nents = dw_readl(dws, DW_SPI_RXFLR);
> + dw_spi_dma_calc_delay(dws, nents, &delay);
> +
> + while (dw_spi_dma_rx_busy(dws) && retry--)
> + spi_delay_exec(&delay, NULL);
> +
> + if (retry < 0) {
> + dev_err(&dws->master->dev, "Rx hanged up\n");
> + dws->master->cur_msg->status = -EIO;
> + }
> +}
> +
> /*
> * dws->dma_chan_busy is set before the dma transfer starts, callback for rx
> * channel will clear a corresponding bit.
> @@ -200,6 +267,8 @@ static void dw_spi_dma_rx_done(void *arg)
> {
> struct dw_spi *dws = arg;
>
> + dw_spi_dma_wait_rx_done(dws);
I can understand the problem about TX, but I don't see how RX
will get hurt, can you elaborate more? thanks
- Feng
> +
> clear_bit(RX_BUSY, &dws->dma_chan_busy);
> if (test_bit(TX_BUSY, &dws->dma_chan_busy))
> return;
> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> index e92d43b9a9e6..81364f501b7e 100644
> --- a/drivers/spi/spi-dw.h
> +++ b/drivers/spi/spi-dw.h
> @@ -210,6 +210,16 @@ static inline void spi_set_clk(struct dw_spi *dws, u16 div)
> dw_writel(dws, DW_SPI_BAUDR, div);
> }
>
> +static inline u32 spi_get_clk(struct dw_spi *dws)
> +{
> + u32 div = dw_readl(dws, DW_SPI_BAUDR);
> +
> + if (!div)
> + return 0;
> +
> + return dws->max_freq / div;
> +}
> +
> /* Disable IRQ bits */
> static inline void spi_mask_intr(struct dw_spi *dws, u32 mask)
> {
> --
> 2.25.1
^ permalink raw reply
* RE: [EXT] Re: [PATCH net 1/4] net: ethernet: fec: move GPR register offset and bit into DT
From: Andy Duan @ 2020-05-21 2:40 UTC (permalink / raw)
To: Jakub Kicinski
Cc: davem@davemloft.net, netdev@vger.kernel.org,
martin.fuzzey@flowbird.group, robh+dt@kernel.org,
shawnguo@kernel.org, devicetree@vger.kernel.org
In-Reply-To: <20200520095430.05cffcee@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
From: Jakub Kicinski <kuba@kernel.org> Sent: Thursday, May 21, 2020 12:54 AM
> On Wed, 20 May 2020 16:31:53 +0800 fugang.duan@nxp.com wrote:
> > From: Fugang Duan <fugang.duan@nxp.com>
> >
> > The commit da722186f654(net: fec: set GPR bit on suspend by DT
> > configuration) set the GPR reigster offset and bit in driver for wake
> > on lan feature.
> >
> > But it introduces two issues here:
> > - one SOC has two instances, they have different bit
> > - different SOCs may have different offset and bit
> >
> > So to support wake-on-lan feature on other i.MX platforms, it should
> > configure the GPR reigster offset and bit from DT.
> >
> > Fixes: da722186f654(net: fec: set GPR bit on suspend by DT
> > configuration)
> > Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
>
> Fixes tag: Fixes: da722186f654(net: fec: set GPR bit on suspend by DT
> configuration) Has these problem(s):
> - missing space between the SHA1 and the subject
Thanks, I will update it v2.
^ permalink raw reply
* Re: [PATCH v2 08/12] i2c: designware: Introduce platform drivers glue layer interface
From: Serge Semin @ 2020-05-21 2:37 UTC (permalink / raw)
To: Jarkko Nikula
Cc: Serge Semin, Andy Shevchenko, Mika Westerberg, Alexey Malahov,
Thomas Bogendoerfer, Paul Burton, Ralf Baechle, Rob Herring,
Frank Rowand, linux-mips, devicetree, Wolfram Sang,
Rafael J. Wysocki, Hanjun Guo, Hans de Goede, linux-kernel,
linux-i2c
In-Reply-To: <4950bb1e-302f-947e-1924-452a8169b504@linux.intel.com>
On Wed, May 20, 2020 at 03:46:11PM +0300, Jarkko Nikula wrote:
> Hi
>
> On 5/10/20 12:50 PM, Serge Semin wrote:
> > Seeing the DW I2C platform driver is getting overcomplicated with a lot of
> > vendor-specific configs let's introduce a glue-layer interface so new
> > platforms which equipped with Synopsys Designware APB I2C IP-core would
> > be able to handle their peculiarities in the dedicated objects.
> >
> Comment to this patch and patches 9/12 and 12/12:
>
> Currently i2c-designware-platdrv.c is about 500 lines of code so I don't
> think it's too overcomplicated. But I feel we have already too many Kconfig
> options and source modules for i2c-designware and obviously would like to
> push back a little from adding more.
>
> I don't think i2c-designware-platdrv.c becomes yet too complicated if Baikal
> related code is added there, perhaps under #ifdef CONFIG_OF like MSCC Ocelot
> code is currently.
Well, it's up to you to decide, what solution is more suitable for you to
maintain. My idea of detaching the MSCC and Baikal-T1 code to the dedicated
source files was to eventually move the whole i2c-designware-* set of files
into a dedicated directory drivers/i2c/buses/dw as it's done for some others
Synopsys DesignWare controllers: drivers/pci/controller/dwc/, drivers/usb/dwc2,
drivers/usb/dwc3, drivers/net/ethernet/synopsys/ . If you think, that it's too
early for Dw I2C code to live in a dedicated directory, fine with me. I can
merge the MSCC and Baikal-T1 code back into the i2c-designware-platdrv.c .
So what's your final word in this matter?
-Sergey
>
> --
> Jarkko
^ permalink raw reply
* Re: [PATCH v2 07/12] i2c: designware: Move Baytrail sem config to the platform if-clause
From: Serge Semin @ 2020-05-21 2:22 UTC (permalink / raw)
To: Jarkko Nikula
Cc: Serge Semin, Alexey Malahov, Thomas Bogendoerfer, Paul Burton,
Ralf Baechle, Andy Shevchenko, Mika Westerberg, Wolfram Sang,
Rob Herring, Frank Rowand, linux-mips, devicetree, Wolfram Sang,
Jean Delvare, Krzysztof Kozlowski, Max Staudt, Stefan Roese,
linux-i2c, linux-kernel
In-Reply-To: <da6d8b18-b808-fd86-bbd2-13e7a90b2a5e@linux.intel.com>
On Wed, May 20, 2020 at 03:16:14PM +0300, Jarkko Nikula wrote:
> On 5/10/20 12:50 PM, Serge Semin wrote:
> > Currently Intel Baytrail I2C semaphore is a feature of the DW APB I2C
> > platform driver. It's a bit confusing to see it's config in the menu at
> > some separated place with no reference to the platform code. Lets move the
> > config definition under the if-I2C_DESIGNWARE_PLATFORM clause. By doing so
> > the config menu will display the feature right below the DW I2C platform
> > driver item and will indent it to the right so signifying its belonging.
> >
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> > Cc: Paul Burton <paulburton@kernel.org>
> > Cc: Ralf Baechle <ralf@linux-mips.org>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Cc: Wolfram Sang <wsa@the-dreams.de>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Frank Rowand <frowand.list@gmail.com>
> > Cc: linux-mips@vger.kernel.org
> > Cc: devicetree@vger.kernel.org
> > ---
> > drivers/i2c/busses/Kconfig | 30 +++++++++++++++++-------------
> > 1 file changed, 17 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > index 368aa64e9266..ed6927c4c540 100644
> > --- a/drivers/i2c/busses/Kconfig
> > +++ b/drivers/i2c/busses/Kconfig
> > @@ -530,8 +530,8 @@ config I2C_DESIGNWARE_CORE
> > config I2C_DESIGNWARE_PLATFORM
> > tristate "Synopsys DesignWare Platform"
> > - select I2C_DESIGNWARE_CORE
> > depends on (ACPI && COMMON_CLK) || !ACPI
> > + select I2C_DESIGNWARE_CORE
> > help
> > If you say yes to this option, support will be included for the
> > Synopsys DesignWare I2C adapter.
> > @@ -539,6 +539,22 @@ config I2C_DESIGNWARE_PLATFORM
> > This driver can also be built as a module. If so, the module
> > will be called i2c-designware-platform.
> > +if I2C_DESIGNWARE_PLATFORM
> > +
> > +config I2C_DESIGNWARE_BAYTRAIL
> > + bool "Intel Baytrail I2C semaphore support"
> > + depends on ACPI
> > + depends on (I2C_DESIGNWARE_PLATFORM=m && IOSF_MBI) || \
> > + (I2C_DESIGNWARE_PLATFORM=y && IOSF_MBI=y)
> > + help
> > + This driver enables managed host access to the PMIC I2C bus on select
> > + Intel BayTrail platforms using the X-Powers AXP288 PMIC. It allows
> > + the host to request uninterrupted access to the PMIC's I2C bus from
> > + the platform firmware controlling it. You should say Y if running on
> > + a BayTrail system using the AXP288.
> > +
> > +endif # I2C_DESIGNWARE_PLATFORM
> > +
>
> Is the added "if I2C_DESIGNWARE_PLATFORM" needed here? Should the "depends
> on" be enough?
The idea was to add if-endif clause here for features possibly added sometime
in future. But using normal "depends on I2C_DESIGNWARE_PLATFORM" shall make
the config depicted as an indented sub-config as well. Would you like me to
remove the if-clause and use the depends on operator instead?
-Sergey
>
> Jarkko
^ permalink raw reply
* Re: [PATCH v2 1/1] dt-bindings: spi: Add schema for Cadence QSPI Controller driver
From: Ramuthevar, Vadivel MuruganX @ 2020-05-21 2:18 UTC (permalink / raw)
To: Mark Brown
Cc: robh, devicetree, linux-kernel, linux-mtd, linux-spi, vigneshr,
cheol.yong.kim, qi-ming.wu
In-Reply-To: <20200520124329.GF4823@sirena.org.uk>
Hi Mark,
Thank you for the review comments...
On 20/5/2020 8:43 pm, Mark Brown wrote:
> On Wed, May 20, 2020 at 08:36:12PM +0800, Ramuthevar,Vadivel MuruganX wrote:
>> From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
>>
>> Add dt-bindings documentation for Cadence-QSPI controller to support
>> spi based flash memories.
>>
>> Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
>> ---
>> .../devicetree/bindings/mtd/cadence-quadspi.txt | 67 -----------
>> .../devicetree/bindings/spi/cdns,qspi-nor.yaml | 133 +++++++++++++++++++++
>
> The changelog says this is adding a new binding but the actual change is
> mostly a conversion to YAML. Please split the additions out into a
> separate change, ideally doing that before the conversion since there is
> a backlog on review of YAML conversions.
Initially was sending the only YAML file alone, then reviewers suggest
to me do this way so I did, next by split the patches like below...
1. remove the cadence-quadspi.txt (patch1)
2. convert txt to YAML (patch2)
Regards
Vadivel
>
^ permalink raw reply
* Re: [PATCH v2 04/12] i2c: designware: Convert driver to using regmap API
From: Serge Semin @ 2020-05-21 2:02 UTC (permalink / raw)
To: Jarkko Nikula
Cc: Serge Semin, Andy Shevchenko, Mika Westerberg, Alexey Malahov,
Thomas Bogendoerfer, Paul Burton, Ralf Baechle, Wolfram Sang,
Rob Herring, Frank Rowand, devicetree, linux-mips, Wolfram Sang,
Jean Delvare, Krzysztof Kozlowski, Max Staudt, Stefan Roese,
Uwe Kleine-König, Shaokun Zhang, linux-i2c, linux-kernel
In-Reply-To: <a0d4c6d8-c2c0-b7ea-d77b-e1fe610fd966@linux.intel.com>
On Wed, May 20, 2020 at 03:16:07PM +0300, Jarkko Nikula wrote:
> On 5/10/20 12:50 PM, Serge Semin wrote:
> > Seeing the DW I2C driver is using flags-based accessors with two
> > conditional clauses it would be better to replace them with the regmap
> > API IO methods and to initialize the regmap object with read/write
> > callbacks specific to the controller registers map implementation. This
> > will be also handy for the drivers with non-standard registers mapping
> > (like an embedded into the Baikal-T1 System Controller DW I2C block, which
> > glue-driver is a part of this series).
> >
> > As before the driver tries to detect the mapping setup at probe stage and
> > creates a regmap object accordingly, which will be used by the rest of the
> > code to correctly access the controller registers. In two places it was
> > appropriate to convert the hand-written read-modify-write and
> > read-poll-loop design patterns to the corresponding regmap API
> > ready-to-use methods.
> >
> > Note the regmap IO methods return value is checked only at the probe
> > stage. The rest of the code won't do this because basically we have
> > MMIO-based regmap so non of the read/write methods can fail (this also
> > won't be needed for the Baikal-T1-specific I2C controller).
> >
> > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> > Cc: Paul Burton <paulburton@kernel.org>
> > Cc: Ralf Baechle <ralf@linux-mips.org>
> > Cc: Wolfram Sang <wsa@the-dreams.de>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Frank Rowand <frowand.list@gmail.com>
> > Cc: devicetree@vger.kernel.org
> > Cc: linux-mips@vger.kernel.org
> > ---
> > drivers/i2c/busses/Kconfig | 1 +
> > drivers/i2c/busses/i2c-designware-common.c | 171 +++++++++++++++------
> > drivers/i2c/busses/i2c-designware-core.h | 18 +--
> > drivers/i2c/busses/i2c-designware-master.c | 125 ++++++++-------
> > drivers/i2c/busses/i2c-designware-slave.c | 77 +++++-----
> > 5 files changed, 239 insertions(+), 153 deletions(-)
> >
> Looking at patches 4/12-12/12 I think it would be good to move fixes and
> less invasive patches before this. Like
>
> i2c: designware: slave: Set DW I2C core module dependency
> i2c: designware: Use `-y` to build multi-object modules
> i2c: designware: Move Baytrail sem config to the platform if-clause
>
> That said, you may add:
>
> Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Ok. I'll move those three patches to be before this one in v3. Thanks.
-Sergey
^ permalink raw reply
* Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
From: Thinh Nguyen @ 2020-05-21 1:55 UTC (permalink / raw)
To: Jun Li, Felipe Balbi, Jun Li
Cc: John Stultz, lkml, Yu Chen, Greg Kroah-Hartman, Rob Herring,
Mark Rutland, ShuFan Lee, Heikki Krogerus, Suzuki K Poulose,
Chunfeng Yun, Hans de Goede, Andy Shevchenko, Valentin Schneider,
Jack Pham, Linux USB List,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Peter Chen
In-Reply-To: <bbfbd3f7-4908-5529-1a4e-29469e794b27@synopsys.com>
Thinh Nguyen wrote:
> Jun Li wrote:
>> Hi
>>
>>> -----Original Message-----
>>> From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>>> Sent: 2020年5月19日 14:46
>>> To: Jun Li <jun.li@nxp.com>; Felipe Balbi <balbi@kernel.org>; Jun Li
>>> <lijun.kernel@gmail.com>
>>> Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu
>>> Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob
>>> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee
>>> <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>;
>>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun
>>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko
>>> <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>;
>>> Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open
>>> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>;
>>> Peter Chen <peter.chen@nxp.com>
>>> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device
>>> controller
>>>
>>> Thinh Nguyen wrote:
>>>> Jun Li wrote:
>>>>>> -----Original Message-----
>>>>>> From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi
>>>>>> Sent: 2020年5月16日 19:57
>>>>>> To: Jun Li <jun.li@nxp.com>; Thinh Nguyen
>>>>>> <Thinh.Nguyen@synopsys.com>; Jun Li <lijun.kernel@gmail.com>
>>>>>> Cc: John Stultz <john.stultz@linaro.org>; lkml
>>>>>> <linux-kernel@vger.kernel.org>; Yu Chen <chenyu56@huawei.com>; Greg
>>>>>> Kroah-Hartman <gregkh@linuxfoundation.org>; Rob Herring
>>>>>> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan
>>>>>> Lee <shufan_lee@richtek.com>; Heikki Krogerus
>>>>>> <heikki.krogerus@linux.intel.com>;
>>>>>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun
>>>>>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>;
>>>>>> Andy Shevchenko <andy.shevchenko@gmail.com>; Valentin Schneider
>>>>>> <valentin.schneider@arm.com>; Jack Pham <jackp@codeaurora.org>;
>>>>>> Linux USB List <linux-usb@vger.kernel.org>; open list:OPEN FIRMWARE
>>>>>> AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>;
>>>>>> Peter Chen <peter.chen@nxp.com>; Thinh Nguyen
>>>>>> <Thinh.Nguyen@synopsys.com>
>>>>>> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct
>>>>>> cleared by device controller
>>>>>>
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Jun Li <jun.li@nxp.com> writes:
>>>>>>>>>> Hi Thinh, could you comment this?
>>>>>>>>> You only need to wake up the usb2 phy when issuing the command
>>>>>>>>> while running in highspeed or below. If you're running in SS or
>>>>>>>>> higher, internally the controller does it for you for usb3 phy.
>>>>>>>>> In Jun's case, it seems like it takes longer for his phy to wake up.
>>>>>>>>>
>>>>>>>>> IMO, in this case, I think it's fine to increase the command timeout.
>>>>>>>> Is there an upper limit to this? Is 32k clock the slowest that can
>>>>>>>> be fed to the PHY as a suspend clock?
>>>>>>> Yes, 32K clock is the slowest, Per DWC3 document on Power Down
>>>>>>> Scale (bits 31:19 of GCTL):
>>>>>>>
>>>>>>> "Power Down Scale (PwrDnScale)
>>>>>>> The USB3 suspend_clk input replaces pipe3_rx_pclk as a clock source
>>>>>>> to a small part of the USB3 controller that operates when the SS
>>>>>>> PHY is in its lowest power (P3) state, and therefore does not provide a clock.
>>>>>>> The Power Down Scale field specifies how many suspend_clk periods
>>>>>>> fit into a 16 kHz clock period. When performing the division, round
>>>>>>> up the remainder.
>>>>>>> For example, when using an 8-bit/16-bit/32-bit PHY and 25-MHz
>>>>>>> Suspend clock, Power Down Scale = 25000 kHz/16 kHz = 13'd1563
>>>>>>> (rounder up)
>>>>>>> Note:
>>>>>>> - Minimum Suspend clock frequency is 32 kHz
>>>>>>> - Maximum Suspend clock frequency is 125 MHz"
>>>>>> Cool, now do we have an upper bound for how many clock cycles it
>>>>>> takes to wake up the PHY?
>>>>> My understanding is this ep command does not wake up the SS PHY, the
>>>>> SS PHY still stays at P3 when execute this ep command. The time
>>>>> required here is to wait controller complete something for this ep
>>>>> command with 32K clock.
>>>> Sorry I made a mistake. You're right. Just checked with one of the RTL
>>>> engineers, and it doesn't need to wake up the phy. However, if it is
>>>> eSS speed, it may take longer time as the command may be completing
>>>> with the suspend clock.
>>>>
>>> What's the value for GCTL[7:6]?
>> 2'b00
>>
>> Thanks
>> Li Jun
> (Sorry for the delay reply)
>
> If it's 0, then the ram clock should be the same as the bus_clk, which
> is odd since you mentioned that the suspend_clk is used instead while in P3.
Just checked with the RTL engineer, even if GCTL[7:6] is set to 0,
internally it can still run with suspend clock during P3.
> Anyway, I was looking for a way maybe to improve the speed during
> issuing a command. One way is to set GUSB3PIPECTL[17]=0, and it should
> wakeup the phy anytime. I think Felipe suggested it. It's odd that it
> doesn't work for you. I don't have other ideas beside increasing the
> command timeout.
>
In any case, increasing the timeout should be fine with me. It maybe
difficult to determine the max timeout base on the slowest clock rate
and number of cycles. Different controller and controller versions
behave differently and may have different number of clock cycles to
complete a command.
The RTL engineer recommended timeout to be at least 1ms (which maybe
more than the polling rate of this patch). I'm fine with either the rate
provided by this tested patch or higher.
BR,
Thinh
^ permalink raw reply
* Re: [PATCH v2 5/6] dmaengine: dw: Introduce max burst length hw config
From: Serge Semin @ 2020-05-21 1:47 UTC (permalink / raw)
To: Vinod Koul
Cc: Serge Semin, Andy Shevchenko, Viresh Kumar, Dan Williams,
Alexey Malahov, Thomas Bogendoerfer, Paul Burton, Ralf Baechle,
Arnd Bergmann, Rob Herring, linux-mips, devicetree, dmaengine,
linux-kernel
In-Reply-To: <20200519170714.GT374218@vkoul-mobl.Dlink>
On Tue, May 19, 2020 at 10:37:14PM +0530, Vinod Koul wrote:
> On 17-05-20, 22:38, Serge Semin wrote:
> > On Fri, May 15, 2020 at 12:09:50PM +0530, Vinod Koul wrote:
> > > On 12-05-20, 22:12, Andy Shevchenko wrote:
> > > > On Tue, May 12, 2020 at 05:08:20PM +0300, Serge Semin wrote:
> > > > > On Fri, May 08, 2020 at 02:41:53PM +0300, Andy Shevchenko wrote:
> > > > > > On Fri, May 08, 2020 at 01:53:03PM +0300, Serge Semin wrote:
[nip]
> > > > > > But let's see what we can do better. Since maximum is defined on the slave side
> > > > > > device, it probably needs to define minimum as well, otherwise it's possible
> > > > > > that some hardware can't cope underrun bursts.
> > > > >
> > > > > There is no need to define minimum if such limit doesn't exists except a
> > > > > natural 1. Moreover it doesn't exist for all DMA controllers seeing noone has
> > > > > added such capability into the generic DMA subsystem so far.
> > > >
> > > > There is a contract between provider and consumer about DMA resource. That's
> > > > why both sides should participate in fulfilling it. Theoretically it may be a
> > > > hardware that doesn't support minimum burst available in DMA by a reason. For
> > > > such we would need minimum to be provided as well.
> > >
> > > Agreed and if required caps should be extended to tell consumer the
> > > minimum values supported.
> >
> > Sorry, it's not required by our hardware. Is there any, which actually has such
> > limitation? (minimum burst length)
>
> IIUC the idea is that you will tell maximum and minimum values supported
> and client can pick the best value. Esp in case of slave transfers
> things like burst, msize are governed by client capability and usage. So
> exposing the set to pick from would make sense
Agreed. I'll add min_burst capability.
-Sergey
>
> --
> ~Vinod
^ permalink raw reply
* Re: [PATCH v2 4/6] dmaengine: dw: Print warning if multi-block is unsupported
From: Serge Semin @ 2020-05-21 1:40 UTC (permalink / raw)
To: Vinod Koul
Cc: Serge Semin, Andy Shevchenko, Mark Brown, Viresh Kumar,
Dan Williams, Alexey Malahov, Thomas Bogendoerfer, Paul Burton,
Ralf Baechle, Arnd Bergmann, Rob Herring, linux-mips, devicetree,
dmaengine, Linux Kernel Mailing List
In-Reply-To: <20200519170246.GS374218@vkoul-mobl.Dlink>
On Tue, May 19, 2020 at 10:32:46PM +0530, Vinod Koul wrote:
> On 17-05-20, 22:23, Serge Semin wrote:
> > On Fri, May 15, 2020 at 12:00:39PM +0530, Vinod Koul wrote:
> > > Hi Serge,
> > >
> > > On 12-05-20, 15:42, Serge Semin wrote:
> > > > Vinod,
> > > >
> > > > Could you join the discussion for a little bit?
> > > >
> > > > In order to properly fix the problem discussed in this topic, we need to
> > > > introduce an additional capability exported by DMA channel handlers on per-channel
> > > > basis. It must be a number, which would indicate an upper limitation of the SG list
> > > > entries amount.
> > > > Something like this would do it:
> > > > struct dma_slave_caps {
> > > > ...
> > > > unsigned int max_sg_nents;
> > > > ...
> > >
> > > Looking at the discussion, I agree we should can this up in the
> > > interface. The max_dma_len suggests the length of a descriptor allowed,
> > > it does not convey the sg_nents supported which in the case of nollp is
> > > one.
> > >
> > > Btw is this is a real hardware issue, I have found that value of such
> > > hardware is very less and people did fix it up in subsequent revs to add
> > > llp support.
> >
> > Yes, it is. My DW DMAC doesn't support LLP and there isn't going to be new SoC
> > version produced.(
>
> Ouch
>
> > > Also, another question is why this cannot be handled in driver, I agree
> > > your hardware does not support llp but that does not stop you from
> > > breaking a multi_sg list into N hardware descriptors and keep submitting
> > > them (for this to work submission should be done in isr and not in bh,
> > > unfortunately very few driver take that route).
> >
> > Current DW DMA driver does that, but this isn't enough. The problem is that
> > in order to fix the issue in the DMA hardware driver we need to introduce
> > an inter-dependent channels abstraction and synchronously feed both Tx and
> > Rx DMA channels with hardware descriptors (LLP entries) one-by-one. This hardly
> > needed by any slave device driver rather than SPI, which Tx and Rx buffers are
> > inter-dependent. So Andy's idea was to move the fix to the SPI driver (feed
> > the DMA engine channels with Tx and Rx data buffers synchronously), but DMA
> > engine would provide an info whether such fix is required. This can be
> > determined by the maximum SG entries capability.
>
> Okay but having the sw limitation removed would also be a good idea, you
> can handle any user, I will leave it upto you, either way is okay
>
> >
> > (Note max_sg_ents isn't a limitation on the number of SG entries supported by
> > the DMA driver, but the number of SG entries handled by the DMA engine in a
> > single DMA transaction.)
> >
> > > TBH the max_sg_nents or
> > > max_dma_len are HW restrictions and SW *can* deal with then :-)
> >
> > Yes, it can, but it only works for the cases when individual DMA channels are
> > utilized. DMA hardware driver doesn't know that the target and source slave
> > device buffers (SPI Tx and Rx FIFOs) are inter-dependent, that writing to one
> > you will implicitly push data to another. So due to the interrupts handling
> > latency Tx DMA channel is restarted faster than Rx DMA channel is reinitialized.
> > This causes the SPI Rx FIFO overflow and data loss.
> >
> > >
> > > In an idea world, you should break the sw descriptor submitted into N hw
> > > descriptors and submit to hardware and let user know when the sw
> > > descriptor is completed. Of course we do not do that :(
> >
> > Well, the current Dw DMA driver does that. But due to the two slave device
> > buffers inter-dependency this isn't enough to perform safe DMA transactions.
> > Due to the interrupts handling latency Tx DMA channel pushes data to the slave
> > device buffer faster than Rx DMA channel starts to handle incoming data. This
> > causes the SPI Rx FIFO overflow.
> >
> > >
> > > > };
> > > > As Andy suggested it's value should be interpreted as:
> > > > 0 - unlimited number of entries,
> > > > 1:MAX_UINT - actual limit to the number of entries.
> > >
> >
> > > Hmm why 0, why not MAX_UINT for unlimited?
> >
> > 0 is much better for many reasons. First of all MAX_UINT is a lot, but it's
> > still a number. On x64 platform this might be actual limit if for instance
> > the block-size register is 32-bits wide. Secondly interpreting 0 as unlimited
> > number of entries would be more suitable since most of the drivers support
> > LLP functionality and we wouldn't need to update their code to set MAX_UINT.
> > Thirdly DMA engines, which don't support LLPs would need to set this parameter
> > as 1. So if we do as you say and interpret unlimited number of LLPs as MAX_UINT,
> > then 0 would left unused.
> >
> > To sum up I also think that using 0 as unlimited number SG entries supported is
> > much better.
>
> ok
>
> > > > In addition to that seeing the dma_get_slave_caps() method provide the caps only
> > > > by getting them from the DMA device descriptor, while we need to have an info on
> > > > per-channel basis, it would be good to introduce a new DMA-device callback like:
> > > > struct dma_device {
> > > > ...
> > > > int (*device_caps)(struct dma_chan *chan,
> > > > struct dma_slave_caps *caps);
> > >
> >
> > > Do you have a controller where channel caps are on per-channel basis?
> >
> > Yes, I do. Our DW DMA controller has got the maximum burst length non-uniformly
> > distributed per DMA channels. There are eight channels our controller supports,
> > among which first two channels can burst up to 32 transfer words, but the rest
> > of the channels support bursting up to 4 transfer words.
> >
> > So having such device_caps() callback to customize the device capabilities on
> > per-DMA-channel basis would be very useful! What do you think?
>
> Okay looks like per-ch basis is the way forward!
Great! Thanks. I'll send v3 with updates we've come up to in this discussion.
-Sergey
>
> --
> ~Vinod
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox