Re: [PATCH v3 10/11] dt-bindings: clock: stm32mp1 new compatible for secure rcc
On 4/19/21 11:38 AM, gabriel.fernan...@foss.st.com wrote: From: Gabriel Fernandez Introduce new compatible string "st,stm32mp1-rcc-secure" for stm32mp1 clock driver when the device is configured with RCC security support hardened. Signed-off-by: Etienne Carriere Signed-off-by: Gabriel Fernandez --- .../devicetree/bindings/clock/st,stm32mp1-rcc.yaml | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml b/Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml index 4e385508f516..8b1ecb2ecdd5 100644 --- a/Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml +++ b/Documentation/devicetree/bindings/clock/st,stm32mp1-rcc.yaml @@ -54,7 +54,9 @@ properties: compatible: items: - - const: st,stm32mp1-rcc + - enum: + - st,stm32mp1-rcc-secure + - st,stm32mp1-rcc It is still the same IP and same SoC silicon, so shouldn't the configuration be discerned via DT property instead of compatible string ?
Re: [PATCH 11/13] ARM: dts: stm32: fix LTDC port node on STM32 MCU ad MPU
On 4/15/21 4:35 PM, Alexandre TORGUE wrote:
On 4/15/21 4:30 PM, Marek Vasut wrote:
On 4/15/21 3:34 PM, Alexandre TORGUE wrote:
Hi Marek
Hello Alexandre,
diff --git a/arch/arm/boot/dts/stm32mp157c-dk2.dts
b/arch/arm/boot/dts/stm32mp157c-dk2.dts
index 2bc92ef3aeb9..19ef475a48fc 100644
--- a/arch/arm/boot/dts/stm32mp157c-dk2.dts
+++ b/arch/arm/boot/dts/stm32mp157c-dk2.dts
@@ -82,9 +82,15 @@
};
{
- status = "okay";
-
port {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ltdc_ep0_out: endpoint@0 {
+ reg = <0>;
+ remote-endpoint = <_in>;
+ };
+
ltdc_ep1_out: endpoint@1 {
reg = <1>;
remote-endpoint = <_in>;
[...]
diff --git a/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi
b/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi
index 64dca5b7f748..e7f10975cacf 100644
--- a/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi
+++ b/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi
@@ -277,11 +277,7 @@
status = "okay";
port {
- #address-cells = <1>;
- #size-cells = <0>;
-
- ltdc_ep0_out: endpoint@0 {
- reg = <0>;
+ ltdc_ep0_out: endpoint {
remote-endpoint = <_in>;
};
};
I think this is wrong, the AV96 can have two displays connected to
two ports of the LTDC, just like DK2 for example.
As for dk2 address/size cells are added only if there are 2
endpoints. It is for this reason I moved endpoint0 definition from
stm32mp15xx-dkx to stm32mp151a-dk1.dts (dk1 has only one endpoint).
Here it's the same, if you have second endpoint then adress/size will
have to be added.
That's a bit problematic. Consider either the use case of DTO which
adds the other display, or even a custom board DTS. Without your
patch, this works:
arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi
{
...
ports {
ltdc_ep0_out: endpoint@0 {
remote-endpoint = <_in>;
};
};
};
board-with-display.dts or board-overlay.dts
{
ports {
endpoint@1 { // just add another endpoint@1, no problem
remote-endpoint = <>;
};
};
};
With your patch, the DTS would have to modify the "endpoint" node to
be "endpoint@0" probably with a whole lot of /detele-node/ etc. magic
(DTO cannot do that, so that's a problem, and I do use DTOs on AV96
extensively for the various expansion cards) and then add the
endpoint@1. That becomes real complicated in custom board DT, and
impossible with DTO.
Yes I agree that it'll be problematic. So maybe so solution would be to
not detect a warning for the initial case (only one endpoint with a reg)
That looks OK. Or even better, if the checker warned only on IPs which
cannot have more than one endpoint, but have endpoint@N in DT (where N
in 0..+inf) . On IPs which can have one or more endpoints, the warning
should not be emitted.
Re: [PATCH 11/13] ARM: dts: stm32: fix LTDC port node on STM32 MCU ad MPU
On 4/15/21 3:34 PM, Alexandre TORGUE wrote:
Hi Marek
Hello Alexandre,
diff --git a/arch/arm/boot/dts/stm32mp157c-dk2.dts
b/arch/arm/boot/dts/stm32mp157c-dk2.dts
index 2bc92ef3aeb9..19ef475a48fc 100644
--- a/arch/arm/boot/dts/stm32mp157c-dk2.dts
+++ b/arch/arm/boot/dts/stm32mp157c-dk2.dts
@@ -82,9 +82,15 @@
};
{
- status = "okay";
-
port {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ltdc_ep0_out: endpoint@0 {
+ reg = <0>;
+ remote-endpoint = <_in>;
+ };
+
ltdc_ep1_out: endpoint@1 {
reg = <1>;
remote-endpoint = <_in>;
[...]
diff --git a/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi
b/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi
index 64dca5b7f748..e7f10975cacf 100644
--- a/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi
+++ b/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi
@@ -277,11 +277,7 @@
status = "okay";
port {
- #address-cells = <1>;
- #size-cells = <0>;
-
- ltdc_ep0_out: endpoint@0 {
- reg = <0>;
+ ltdc_ep0_out: endpoint {
remote-endpoint = <_in>;
};
};
I think this is wrong, the AV96 can have two displays connected to two
ports of the LTDC, just like DK2 for example.
As for dk2 address/size cells are added only if there are 2 endpoints.
It is for this reason I moved endpoint0 definition from stm32mp15xx-dkx
to stm32mp151a-dk1.dts (dk1 has only one endpoint).
Here it's the same, if you have second endpoint then adress/size will
have to be added.
That's a bit problematic. Consider either the use case of DTO which adds
the other display, or even a custom board DTS. Without your patch, this
works:
arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi
{
...
ports {
ltdc_ep0_out: endpoint@0 {
remote-endpoint = <_in>;
};
};
};
board-with-display.dts or board-overlay.dts
{
ports {
endpoint@1 { // just add another endpoint@1, no problem
remote-endpoint = <>;
};
};
};
With your patch, the DTS would have to modify the "endpoint" node to be
"endpoint@0" probably with a whole lot of /detele-node/ etc. magic (DTO
cannot do that, so that's a problem, and I do use DTOs on AV96
extensively for the various expansion cards) and then add the
endpoint@1. That becomes real complicated in custom board DT, and
impossible with DTO.
Re: [PATCH 11/13] ARM: dts: stm32: fix LTDC port node on STM32 MCU ad MPU
On 4/15/21 12:10 PM, Alexandre Torgue wrote:
Running "make dtbs_check W=1", some warnings are reported concerning
LTDC port subnode:
/soc/display-controller@5a001000/port:
unnecessary #address-cells/#size-cells without "ranges" or child "reg"
property
/soc/display-controller@5a001000/port: graph node has single child node
'endpoint', #address-cells/#size-cells are not necessary
btw could you retain diffstat on your patches ? It's useful to see which
files changed right away.
[...]
diff --git a/arch/arm/boot/dts/stm32mp157c-dk2.dts
b/arch/arm/boot/dts/stm32mp157c-dk2.dts
index 2bc92ef3aeb9..19ef475a48fc 100644
--- a/arch/arm/boot/dts/stm32mp157c-dk2.dts
+++ b/arch/arm/boot/dts/stm32mp157c-dk2.dts
@@ -82,9 +82,15 @@
};
{
- status = "okay";
-
port {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ltdc_ep0_out: endpoint@0 {
+ reg = <0>;
+ remote-endpoint = <_in>;
+ };
+
ltdc_ep1_out: endpoint@1 {
reg = <1>;
remote-endpoint = <_in>;
[...]
diff --git a/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi
b/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi
index 64dca5b7f748..e7f10975cacf 100644
--- a/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi
+++ b/arch/arm/boot/dts/stm32mp15xx-dhcor-avenger96.dtsi
@@ -277,11 +277,7 @@
status = "okay";
port {
- #address-cells = <1>;
- #size-cells = <0>;
-
- ltdc_ep0_out: endpoint@0 {
- reg = <0>;
+ ltdc_ep0_out: endpoint {
remote-endpoint = <_in>;
};
};
I think this is wrong, the AV96 can have two displays connected to two
ports of the LTDC, just like DK2 for example.
Re: [PATCH stable] gpiolib: Read "gpio-line-names" from a firmware node
On 4/10/21 2:07 PM, Greg Kroah-Hartman wrote:
On Sat, Apr 10, 2021 at 11:09:19AM +0200, Bartosz Golaszewski wrote:
From: Andy Shevchenko
On STM32MP1, the GPIO banks are subnodes of pin-controller@50002000,
see arch/arm/boot/dts/stm32mp151.dtsi. The driver for
pin-controller@50002000 is in drivers/pinctrl/stm32/pinctrl-stm32.c
and iterates over all of its DT subnodes when registering each GPIO
bank gpiochip. Each gpiochip has:
- gpio_chip.parent = dev,
where dev is the device node of the pin controller
- gpio_chip.of_node = np,
which is the OF node of the GPIO bank
Therefore, dev_fwnode(chip->parent) != of_fwnode_handle(chip.of_node),
i.e. pin-controller@50002000 != pin-controller@50002000/gpio@5000*000.
The original code behaved correctly, as it extracted the "gpio-line-names"
from of_fwnode_handle(chip.of_node) = pin-controller@50002000/gpio@5000*000.
To achieve the same behaviour, read property from the firmware node.
Fixes: 7cba1a4d5e162 ("gpiolib: generalize devprop_gpiochip_set_names() for device
properties")
Cc: sta...@vger.kernel.org
Reported-by: Marek Vasut
Reported-by: Roman Guskov
Signed-off-by: Andy Shevchenko
Tested-by: Marek Vasut
Reviewed-by: Marek Vasut
Signed-off-by: Bartosz Golaszewski
---
Hi Greg,
This patch somehow got lost and never made its way into stable. Could you
please apply it?
This has been added and removed more times than I can remember already.
Are you all _SURE_ this is safe for a stable kernel release? Look in
the archives for complaints when we added this in the past.
I now tested this on stm32mp1, which was the original platform that got
affected by the problem this is supposed to fix, and I can confirm this
patch fixes the problem there.
So for what it's worth
Tested-by: Marek Vasut # on stm32mp1
Re: [PATCH v1 1/1] gpiolib: Read "gpio-line-names" from a firmware node
On 4/10/21 11:06 AM, Bartosz Golaszewski wrote:
On Sat, Apr 10, 2021 at 2:46 AM Marek Vasut wrote:
On 3/15/21 6:04 PM, Andy Shevchenko wrote:
On Mon, Mar 15, 2021 at 6:49 PM Bartosz Golaszewski
wrote:
On Mon, Mar 15, 2021 at 3:34 PM Andy Shevchenko
wrote:
On Mon, Mar 15, 2021 at 03:04:37PM +0100, Bartosz Golaszewski wrote:
On Mon, Mar 15, 2021 at 1:50 PM Andy Shevchenko
wrote:
On Mon, Mar 15, 2021 at 12:16:26PM +0200, Andy Shevchenko wrote:
On Mon, Mar 15, 2021 at 10:01:47AM +0100, Bartosz Golaszewski wrote:
On Fri, Mar 5, 2021 at 1:03 PM Andy Shevchenko
wrote:
Unfortunately while this may fix the particular use-case on STM32, it
breaks all other users as the 'gpio-line-names' property doesn't live
on dev_fwnode(>dev) but on dev_fwnode(chip->parent).
How about we first look for this property on the latter and only if
it's not present descend down to the former fwnode?
Oops, I have tested on x86 and it worked the same way.
Lemme check this, but I think the issue rather in ordering when we apply fwnode
to the newly created device and when we actually retrieve gpio-line-names
property.
Hmm... I can't see how it's possible can be. Can you provide a platform name
and pointers to the DTS that has been broken by the change?
I noticed it with gpio-mockup (libgpiod tests failed on v5.12-rc3) and
the WiP gpio-sim - but it's the same on most DT platforms. The node
that contains the `gpio-line-names` is the one associated with the
platform device passed to the GPIO driver. The gpiolib then creates
another struct device that becomes the child of that node but it
doesn't copy the parent's properties to it (nor should it).
Every driver that reads device properties does it from the parent
device, not the one in gdev - whether it uses of_, fwnode_ or generic
device_ properties.
What you are telling contradicts with the idea of copying parent's fwnode
(or OF node) in the current code.
Ha! While the OF node of the parent device is indeed assigned to the
gdev's dev, the same isn't done in the core code for fwnodes and
simulated chips don't have an associated OF node, so this is the
culprit I suppose.
Close, but not fully correct.
First of all it depends on the OF / ACPI / platform enumeration.
Second, we are talking about secondary fwnode in the case where it happens.
I'm in the middle of debugging this, I'll come up with something soon I believe.
Was there ever any follow up on this ?
I would like to point out that on STM32MP1 in Linux 5.10.y, the
gpio-line-names are still broken, and a revert of "gpiolib: generalize
devprop_gpiochip_set_names() for device properties" is still necessary.
Yes, Andy has fixed that in commit b41ba2ec54a7 ("gpiolib: Read
"gpio-line-names" from a firmware node") but for some reason this has
never made its way into stable. I'll resend it.
Yes, that's the missing one, thanks. With that picked, the mp1 is fine.
Re: [PATCH v1 1/1] gpiolib: Read "gpio-line-names" from a firmware node
On 3/15/21 6:04 PM, Andy Shevchenko wrote: On Mon, Mar 15, 2021 at 6:49 PM Bartosz Golaszewski wrote: On Mon, Mar 15, 2021 at 3:34 PM Andy Shevchenko wrote: On Mon, Mar 15, 2021 at 03:04:37PM +0100, Bartosz Golaszewski wrote: On Mon, Mar 15, 2021 at 1:50 PM Andy Shevchenko wrote: On Mon, Mar 15, 2021 at 12:16:26PM +0200, Andy Shevchenko wrote: On Mon, Mar 15, 2021 at 10:01:47AM +0100, Bartosz Golaszewski wrote: On Fri, Mar 5, 2021 at 1:03 PM Andy Shevchenko wrote: Unfortunately while this may fix the particular use-case on STM32, it breaks all other users as the 'gpio-line-names' property doesn't live on dev_fwnode(>dev) but on dev_fwnode(chip->parent). How about we first look for this property on the latter and only if it's not present descend down to the former fwnode? Oops, I have tested on x86 and it worked the same way. Lemme check this, but I think the issue rather in ordering when we apply fwnode to the newly created device and when we actually retrieve gpio-line-names property. Hmm... I can't see how it's possible can be. Can you provide a platform name and pointers to the DTS that has been broken by the change? I noticed it with gpio-mockup (libgpiod tests failed on v5.12-rc3) and the WiP gpio-sim - but it's the same on most DT platforms. The node that contains the `gpio-line-names` is the one associated with the platform device passed to the GPIO driver. The gpiolib then creates another struct device that becomes the child of that node but it doesn't copy the parent's properties to it (nor should it). Every driver that reads device properties does it from the parent device, not the one in gdev - whether it uses of_, fwnode_ or generic device_ properties. What you are telling contradicts with the idea of copying parent's fwnode (or OF node) in the current code. Ha! While the OF node of the parent device is indeed assigned to the gdev's dev, the same isn't done in the core code for fwnodes and simulated chips don't have an associated OF node, so this is the culprit I suppose. Close, but not fully correct. First of all it depends on the OF / ACPI / platform enumeration. Second, we are talking about secondary fwnode in the case where it happens. I'm in the middle of debugging this, I'll come up with something soon I believe. Was there ever any follow up on this ? I would like to point out that on STM32MP1 in Linux 5.10.y, the gpio-line-names are still broken, and a revert of "gpiolib: generalize devprop_gpiochip_set_names() for device properties" is still necessary.
Re: [PATCH 5.11 12/31] gpiolib: Read "gpio-line-names" from a firmware node
On 3/19/21 1:36 PM, Greg Kroah-Hartman wrote: On Fri, Mar 19, 2021 at 01:27:23PM +0100, Marek Vasut wrote: On 3/19/21 1:19 PM, Greg Kroah-Hartman wrote: From: Andy Shevchenko [ Upstream commit b41ba2ec54a70908067034f139aa23d0dd2985ce ] On STM32MP1, the GPIO banks are subnodes of pin-controller@50002000, see arch/arm/boot/dts/stm32mp151.dtsi. The driver for pin-controller@50002000 is in drivers/pinctrl/stm32/pinctrl-stm32.c and iterates over all of its DT subnodes when registering each GPIO bank gpiochip. Each gpiochip has: - gpio_chip.parent = dev, where dev is the device node of the pin controller - gpio_chip.of_node = np, which is the OF node of the GPIO bank Therefore, dev_fwnode(chip->parent) != of_fwnode_handle(chip.of_node), i.e. pin-controller@50002000 != pin-controller@50002000/gpio@5000*000. The original code behaved correctly, as it extracted the "gpio-line-names" from of_fwnode_handle(chip.of_node) = pin-controller@50002000/gpio@5000*000. To achieve the same behaviour, read property from the firmware node. I think we agreed to drop this one for now before, see [PATCH 5.10 081/290] gpiolib: Read "gpio-line-names" from a firmware node Message-ID: Sorry, now dropped. Again. No worries, good thing we have the review process in place :)
Re: [PATCH 5.11 12/31] gpiolib: Read "gpio-line-names" from a firmware node
On 3/19/21 1:19 PM, Greg Kroah-Hartman wrote: From: Andy Shevchenko [ Upstream commit b41ba2ec54a70908067034f139aa23d0dd2985ce ] On STM32MP1, the GPIO banks are subnodes of pin-controller@50002000, see arch/arm/boot/dts/stm32mp151.dtsi. The driver for pin-controller@50002000 is in drivers/pinctrl/stm32/pinctrl-stm32.c and iterates over all of its DT subnodes when registering each GPIO bank gpiochip. Each gpiochip has: - gpio_chip.parent = dev, where dev is the device node of the pin controller - gpio_chip.of_node = np, which is the OF node of the GPIO bank Therefore, dev_fwnode(chip->parent) != of_fwnode_handle(chip.of_node), i.e. pin-controller@50002000 != pin-controller@50002000/gpio@5000*000. The original code behaved correctly, as it extracted the "gpio-line-names" from of_fwnode_handle(chip.of_node) = pin-controller@50002000/gpio@5000*000. To achieve the same behaviour, read property from the firmware node. I think we agreed to drop this one for now before, see [PATCH 5.10 081/290] gpiolib: Read "gpio-line-names" from a firmware node Message-ID:
Re: [PATCH 5.10 081/290] gpiolib: Read "gpio-line-names" from a firmware node
On 3/15/21 2:52 PM, gre...@linuxfoundation.org wrote: From: Greg Kroah-Hartman From: Andy Shevchenko commit b41ba2ec54a70908067034f139aa23d0dd2985ce upstream. On STM32MP1, the GPIO banks are subnodes of pin-controller@50002000, see arch/arm/boot/dts/stm32mp151.dtsi. The driver for pin-controller@50002000 is in drivers/pinctrl/stm32/pinctrl-stm32.c and iterates over all of its DT subnodes when registering each GPIO bank gpiochip. Each gpiochip has: - gpio_chip.parent = dev, where dev is the device node of the pin controller - gpio_chip.of_node = np, which is the OF node of the GPIO bank Therefore, dev_fwnode(chip->parent) != of_fwnode_handle(chip.of_node), i.e. pin-controller@50002000 != pin-controller@50002000/gpio@5000*000. The original code behaved correctly, as it extracted the "gpio-line-names" from of_fwnode_handle(chip.of_node) = pin-controller@50002000/gpio@5000*000. To achieve the same behaviour, read property from the firmware node. There seem to be some discussion going on around this patch, so please postpone backporting until that is settled. Same for v5.11 backport. I hope Andy/Bartosz agrees ?
Re: [Linux-stm32] [PATCH v2 00/14] Introduce STM32MP1 RCC in secured mode
On 3/11/21 7:10 PM, Alexandre TORGUE wrote: Hi Guys On 3/11/21 5:11 PM, Marek Vasut wrote: On 3/11/21 3:41 PM, Ahmad Fatoum wrote: Hello, Hi, On 11.03.21 15:02, Alexandre TORGUE wrote: On 3/11/21 12:43 PM, Marek Vasut wrote: On 3/11/21 9:08 AM, Alexandre TORGUE wrote: 1- Break the current ABI: as soon as those patches are merged, stm32mp157c-dk2.dtb will impose to use A tf-a for scmi clocks. For people using u-boot spl, the will have to create their own "no-secure" devicetree. NAK, this breaks existing boards and existing setups, e.g. DK2 that does not use ATF. 2-As you suggest, create a new "secure" dtb per boards (Not my wish for maintenance perspectives). I agree with Alex (G) that the "secure" option should be opt-in. That way existing setups remain working and no extra requirements are imposed on MP1 users. Esp. since as far as I understand this, the "secure" part isn't really about security, but rather about moving clock configuration from Linux to some firmware blob. 3- Keep kernel device tree as they are and applied this secure layer (scmi clocks phandle) thanks to dtbo in U-boot. Is this really better than #include "stm32mp15xx-enable-secure-stuff.dtsi" in a board DT ? Because that is how I imagine the opt-in "secure" option could work. Discussing with Patrick about u-boot, we could use dtbo application thanks to extlinux.conf. BUT it it will not prevent other case (i.e. TF-A which jump directly in kernel@). So the "least worst" solution is to create a new "stm32mp1257c-scmi-dk2 board which will overload clock entries with a scmi phandle (as proposed by Alex). I raised this issue before with your colleagues. I still believe the correct way would be for the TF-A to pass down either a device tree or an overlay with the actual settings in use, e.g.: - Clocks/Resets done via SCMI - Reserved memory regions If TF-A directly boots Linux, it can apply the overlay itself, otherwise it's passed down to SSBL that applies it before booting Linux. That sounds good and it is something e.g. R-Car already does, it merges DT fragment from prior stages at U-Boot level and then passes the result to Linux. So on ST hardware, the same could very well happen and it would work for both non-ATF / ATF / ATF+TEE options. Even this solution sounds good but we are currently not able to do it in our TF-A/u-boot so not feasible for the moment. Why not ? U-Boot is perfectly capable of merging prior stage DT and DT loaded from elsewhere. See R-Car3 for example. So we have to find a solution for now. Create a new dtb can be this solution. Our internal strategy is to use scmi on our official ST board. It will be a really drawback to include a "no-scmi.dtsi" in DH boards (for example) and to create a stm32mp157c-noscmi-dk2.dts ? I would highly prefer the SCMI to be opt-in, not opt-out. But still, isn't it possible to auto-detect the board configuration in Linux and pick the clock enumeration based on that automatically ?
Re: [Linux-stm32] [PATCH v2 00/14] Introduce STM32MP1 RCC in secured mode
On 3/11/21 3:41 PM, Ahmad Fatoum wrote: Hello, Hi, On 11.03.21 15:02, Alexandre TORGUE wrote: On 3/11/21 12:43 PM, Marek Vasut wrote: On 3/11/21 9:08 AM, Alexandre TORGUE wrote: 1- Break the current ABI: as soon as those patches are merged, stm32mp157c-dk2.dtb will impose to use A tf-a for scmi clocks. For people using u-boot spl, the will have to create their own "no-secure" devicetree. NAK, this breaks existing boards and existing setups, e.g. DK2 that does not use ATF. 2-As you suggest, create a new "secure" dtb per boards (Not my wish for maintenance perspectives). I agree with Alex (G) that the "secure" option should be opt-in. That way existing setups remain working and no extra requirements are imposed on MP1 users. Esp. since as far as I understand this, the "secure" part isn't really about security, but rather about moving clock configuration from Linux to some firmware blob. 3- Keep kernel device tree as they are and applied this secure layer (scmi clocks phandle) thanks to dtbo in U-boot. Is this really better than #include "stm32mp15xx-enable-secure-stuff.dtsi" in a board DT ? Because that is how I imagine the opt-in "secure" option could work. Discussing with Patrick about u-boot, we could use dtbo application thanks to extlinux.conf. BUT it it will not prevent other case (i.e. TF-A which jump directly in kernel@). So the "least worst" solution is to create a new "stm32mp1257c-scmi-dk2 board which will overload clock entries with a scmi phandle (as proposed by Alex). I raised this issue before with your colleagues. I still believe the correct way would be for the TF-A to pass down either a device tree or an overlay with the actual settings in use, e.g.: - Clocks/Resets done via SCMI - Reserved memory regions If TF-A directly boots Linux, it can apply the overlay itself, otherwise it's passed down to SSBL that applies it before booting Linux. That sounds good and it is something e.g. R-Car already does, it merges DT fragment from prior stages at U-Boot level and then passes the result to Linux. So on ST hardware, the same could very well happen and it would work for both non-ATF / ATF / ATF+TEE options.
Re: [PATCH v2 00/14] Introduce STM32MP1 RCC in secured mode
On 3/11/21 2:15 PM, Alexandre TORGUE wrote: Hi Marek Hello Alexandre, On 3/11/21 12:43 PM, Marek Vasut wrote: On 3/11/21 9:08 AM, Alexandre TORGUE wrote: Hi ALex Hello everyone, [...] Subject: Re: [PATCH v2 00/14] Introduce STM32MP1 RCC in secured mode On 1/26/21 3:01 AM, gabriel.fernan...@foss.st.com wrote: From: Gabriel Fernandez Platform STM32MP1 can be used in configuration where some clocks and IP resets can relate as secure resources. These resources are moved from a RCC clock/reset handle to a SCMI clock/reset_domain handle. The RCC clock driver is now dependent of the SCMI driver, then we have to manage now the probe defering. v1 -> v2: - fix yamllint warnings. Hi Gabriel, I don't have much clout with the maintainers, but I have to NAK this series after finding major breakage. The problem with series is that it breaks pretty much every board it touches. I have a DK2 here that I'm using for development, which no longer boots with this series applied. The crux of the matter is that this series assumes all boards will boot with an FSBL that implements a very specific SCMI clock tree. This is major ABI breakage for anyone not using TF-A as the first stage bootloader. Anyone using u-boot SPL is screwed. This series imposes a SOC-wide change via the dtsi files. So even boards that you don't intend to convert to SCMI will get broken this way. Adding a -no-scmi file that isn't used anywhere doesn't help things. You are right. We mainly take care about NO ST (DH/...) boards, but not really about current usage Of our stm32 boards. Several options exist: Since a lot of people benefit from the good upstream support for the MP1 _and_ keep updating their machines to get the latest fixes, it is very important to keep the current usage working. 1- Break the current ABI: as soon as those patches are merged, stm32mp157c-dk2.dtb will impose to use A tf-a for scmi clocks. For people using u-boot spl, the will have to create their own "no-secure" devicetree. NAK, this breaks existing boards and existing setups, e.g. DK2 that does not use ATF. > 2-As you suggest, create a new "secure" dtb per boards (Not my wish for maintenance perspectives). I agree with Alex (G) that the "secure" option should be opt-in. That way existing setups remain working and no extra requirements are imposed on MP1 users. Esp. since as far as I understand this, the "secure" part isn't really about security, but rather about moving clock configuration from Linux to some firmware blob. 3- Keep kernel device tree as they are and applied this secure layer (scmi clocks phandle) thanks to dtbo in U-boot. Is this really better than #include "stm32mp15xx-enable-secure-stuff.dtsi" in a board DT ? Because that is how I imagine the opt-in "secure" option could work. The dtbo usage could avoid to add another st board (actually a secure config) in arch/arm/boot/dts. It isn't even a board, it is a configuration. Could you detect this secure/non-secure state at runtime, have both clock options in the DT, and handle it accordingly ? That might be even better option.
Re: [PATCH v2 00/14] Introduce STM32MP1 RCC in secured mode
On 3/11/21 9:08 AM, Alexandre TORGUE wrote: Hi ALex Hello everyone, [...] Subject: Re: [PATCH v2 00/14] Introduce STM32MP1 RCC in secured mode On 1/26/21 3:01 AM, gabriel.fernan...@foss.st.com wrote: From: Gabriel Fernandez Platform STM32MP1 can be used in configuration where some clocks and IP resets can relate as secure resources. These resources are moved from a RCC clock/reset handle to a SCMI clock/reset_domain handle. The RCC clock driver is now dependent of the SCMI driver, then we have to manage now the probe defering. v1 -> v2: - fix yamllint warnings. Hi Gabriel, I don't have much clout with the maintainers, but I have to NAK this series after finding major breakage. The problem with series is that it breaks pretty much every board it touches. I have a DK2 here that I'm using for development, which no longer boots with this series applied. The crux of the matter is that this series assumes all boards will boot with an FSBL that implements a very specific SCMI clock tree. This is major ABI breakage for anyone not using TF-A as the first stage bootloader. Anyone using u-boot SPL is screwed. This series imposes a SOC-wide change via the dtsi files. So even boards that you don't intend to convert to SCMI will get broken this way. Adding a -no-scmi file that isn't used anywhere doesn't help things. You are right. We mainly take care about NO ST (DH/...) boards, but not really about current usage Of our stm32 boards. Several options exist: Since a lot of people benefit from the good upstream support for the MP1 _and_ keep updating their machines to get the latest fixes, it is very important to keep the current usage working. 1- Break the current ABI: as soon as those patches are merged, stm32mp157c-dk2.dtb will impose to use A tf-a for scmi clocks. For people using u-boot spl, the will have to create their own "no-secure" devicetree. NAK, this breaks existing boards and existing setups, e.g. DK2 that does not use ATF. 2-As you suggest, create a new "secure" dtb per boards (Not my wish for maintenance perspectives). I agree with Alex (G) that the "secure" option should be opt-in. That way existing setups remain working and no extra requirements are imposed on MP1 users. Esp. since as far as I understand this, the "secure" part isn't really about security, but rather about moving clock configuration from Linux to some firmware blob. 3- Keep kernel device tree as they are and applied this secure layer (scmi clocks phandle) thanks to dtbo in U-boot. Is this really better than #include "stm32mp15xx-enable-secure-stuff.dtsi" in a board DT ? Because that is how I imagine the opt-in "secure" option could work. The third could be the less costly. [...]
Re: [PATCH v3 2/2] drm: bridge: Add TI SN65DSI83/84/85 DSI to LVDS bridge
On 2/14/21 6:44 PM, Jagan Teki wrote:
[...]
+static const struct regmap_config sn65dsi_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = SN65DSI_CHA_ERR,
+ .name = "sn65dsi",
+ .cache_type = REGCACHE_RBTREE,
+};
You might want to look at the driver I posted one more time, it defines
the regmap precisely and limits each register access, see:
[PATCH 2/2] drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 driver
that way it can be dumped via debugfs and the regmap does not cache
registers which do not exist, like it does here.
[...]
+static int sn65dsi_get_clk_range(int min, int max, unsigned long clock,
+unsigned long start, unsigned long diff)
+{
+ unsigned long next;
+ int i;
+
+ for (i = min; i <= max; i++) {
+ next = start + diff;
+ if (start <= clock && clock < next)
+ return i;
+
+ start += diff;
+ }
+
+ return -EINVAL;
+}
The clock rates can be calculated in linear time, see the driver above,
it is implemented there.
+static void sn65dsi_enable(struct drm_bridge *bridge)
+{
+ struct sn65dsi *sn = bridge_to_sn65dsi(bridge);
+ struct drm_display_mode *mode = bridge_to_mode(bridge);
+ int bpp = mipi_dsi_pixel_format_to_bpp(sn->dsi->format);
+ unsigned int lanes = sn->dsi->lanes;
+ unsigned int pixel_clk = mode->clock * 1000;
+ unsigned int dsi_clk = pixel_clk * bpp / (lanes * 2);
+ unsigned int val;
+
+ /* reset SOFT_RESET bit */
+ regmap_write(sn->regmap, SN65DSI_SOFT_RESET, 0x0);
+
+ msleep(10);
Why is there msleep(10) all over the place ?
I don't see such a requirement listed anywhere in the DSI83 datasheet.
+ /* reset PLL_EN bit */
+ regmap_write(sn->regmap, SN65DSI_CLK_PLL, 0x0);
+
+ msleep(10);
Here too.
[...]
You also want to check the feedback on the driver I posted, it deals
with polling for the PLL to be ready, which seems to be missing here.
That should remove most of the msleep() calls.
Re: [PATCH v3 1/2] dt-bindings: display: bridge: Add bindings for SN65DSI83/84/85
On 2/14/21 6:44 PM, Jagan Teki wrote: SN65DSI83/84/85 devices are MIPI DSI to LVDS based bridge controller IC's from Texas Instruments. SN65DSI83 - Single Channel DSI to Single-link LVDS bridge SN65DSI84 - Single Channel DSI to Dual-link LVDS bridge SN65DSI85 - Dual Channel DSI to Dual-link LVDS bridge [...] +description: | + SN65DSI83/84/85 devices are MIPI DSI to LVDS based bridge controller + IC's from Texas Instruments. + + SN65DSI83 - Single Channel DSI to Single-link LVDS bridge + SN65DSI84 - Single Channel DSI to Dual-link LVDS bridge + SN65DSI85 - Dual Channel DSI to Dual-link LVDS bridge [...] +properties: + compatible: +enum: + - ti,sn65dsi83 + - ti,sn65dsi84 DSI85 seems missing ? + reg: +const: 0x2c I have the DSI83 device at 0x2d, so this cannot be const 0x2c ? + enable-gpios: +maxItems: 1 +description: GPIO specifier for bridge enable pin (active high). The bridge can work without this GPIO, so its optional. [...] Also, Doug reported that vcc and vcore regulators should likely be listed, see feedback on: [PATCH 1/2] dt-bindings: drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 bindings
Re: [PATCH v1 1/1] gpiolib: Read "gpio-line-names" from a firmware node
On 3/5/21 1:24 PM, Andy Shevchenko wrote:
On Fri, Mar 05, 2021 at 01:11:39PM +0100, Marek Vasut wrote:
On 3/5/21 1:02 PM, Andy Shevchenko wrote:
On STM32MP1, the GPIO banks are subnodes of pin-controller@50002000,
see arch/arm/boot/dts/stm32mp151.dtsi. The driver for
pin-controller@50002000 is in drivers/pinctrl/stm32/pinctrl-stm32.c
and iterates over all of its DT subnodes when registering each GPIO
bank gpiochip. Each gpiochip has:
- gpio_chip.parent = dev,
where dev is the device node of the pin controller
- gpio_chip.of_node = np,
which is the OF node of the GPIO bank
Therefore, dev_fwnode(chip->parent) != of_fwnode_handle(chip.of_node),
i.e. pin-controller@50002000 != pin-controller@50002000/gpio@5000*000.
The original code behaved correctly, as it extracted the "gpio-line-names"
from of_fwnode_handle(chip.of_node) = pin-controller@50002000/gpio@5000*000.
To achieve the same behaviour, read property from the firmware node.
Fixes: 7cba1a4d5e162 ("gpiolib: generalize devprop_gpiochip_set_names() for device
properties")
Reported-by: Marek Vasut
Reported-by: Roman Guskov
Signed-off-by: Andy Shevchenko
Tested-by: Marek Vasut
Reviewed-by: Marek Vasut
Thanks!
Thanks
static int devprop_gpiochip_set_names(struct gpio_chip *chip)
{
struct gpio_device *gdev = chip->gpiodev;
- struct device *dev = chip->parent;
+ struct fwnode_handle *fwnode = dev_fwnode(>dev);
You could make the order here a reverse xmas tree, but that's a nitpick.
They are dependent, can't be reordered.
Doh, you're right.
Re: [PATCH v1 1/1] gpiolib: Read "gpio-line-names" from a firmware node
On 3/5/21 1:02 PM, Andy Shevchenko wrote:
On STM32MP1, the GPIO banks are subnodes of pin-controller@50002000,
see arch/arm/boot/dts/stm32mp151.dtsi. The driver for
pin-controller@50002000 is in drivers/pinctrl/stm32/pinctrl-stm32.c
and iterates over all of its DT subnodes when registering each GPIO
bank gpiochip. Each gpiochip has:
- gpio_chip.parent = dev,
where dev is the device node of the pin controller
- gpio_chip.of_node = np,
which is the OF node of the GPIO bank
Therefore, dev_fwnode(chip->parent) != of_fwnode_handle(chip.of_node),
i.e. pin-controller@50002000 != pin-controller@50002000/gpio@5000*000.
The original code behaved correctly, as it extracted the "gpio-line-names"
from of_fwnode_handle(chip.of_node) = pin-controller@50002000/gpio@5000*000.
To achieve the same behaviour, read property from the firmware node.
Fixes: 7cba1a4d5e162 ("gpiolib: generalize devprop_gpiochip_set_names() for device
properties")
Reported-by: Marek Vasut
Reported-by: Roman Guskov
Signed-off-by: Andy Shevchenko
Tested-by: Marek Vasut
Reviewed-by: Marek Vasut
Thanks
static int devprop_gpiochip_set_names(struct gpio_chip *chip)
{
struct gpio_device *gdev = chip->gpiodev;
- struct device *dev = chip->parent;
+ struct fwnode_handle *fwnode = dev_fwnode(>dev);
You could make the order here a reverse xmas tree, but that's a nitpick.
[...]
Re: [PATCH AUTOSEL 5.10 050/217] rsi: Fix TX EAPOL packet handling against iwlwifi AP
On 3/4/21 9:47 PM, Sasha Levin wrote:
On Tue, Mar 02, 2021 at 08:25:49PM +0100, Marek Vasut wrote:
On 12/23/20 3:13 AM, Sasha Levin wrote:
Hello Sasha,
From: Marek Vasut
[ Upstream commit 65277100caa2f2c62b6f3c4648b90d6f0435f3bc ]
In case RSI9116 SDIO WiFi operates in STA mode against Intel 9260 in
AP mode,
the association fails. The former is using wpa_supplicant during
association,
the later is set up using hostapd:
[...]
Was this patch possibly missed from 5.10.y ?
I'm not sure what happened there, but I can queue it up.
Thank you
Also, while at it, I think it might make sense to pick the following
two patches as well, they dramatically reduce interrupt rate of the
RSI WiFi device, so it stops overloading lower-end devices:
287431463e786 ("rsi: Move card interrupt handling to RX thread")
And this one too.
Thanks
abd131a19f6b8 ("rsi: Clean up loop in the interrupt handler")
But not this one, it looks like just a cleanup. Why is it needed?
Now I got confused, yes, please skip abd131a19f6b8, thanks for spotting
it. (I still have one more patch for the RSI wifi which I need to send
out, but that's for later)
Re: [PATCH v5 00/14] Add BLK_CTL support for i.MX8MP
On 3/3/21 11:47 AM, Abel Vesa wrote:
On 20-11-03 13:18:12, Abel Vesa wrote:
The BLK_CTL according to HW design is basically the wrapper of the entire
function specific group of IPs and holds GPRs that usually cannot be placed
into one specific IP from that group. Some of these GPRs are used to control
some clocks, other some resets, others some very specific function that does
not fit into clocks or resets. Since the clocks are registered using the i.MX
clock subsystem API, the driver is placed into the clock subsystem, but it
also registers the resets. For the other functionalities that other GPRs might
have, the syscon is used.
This approach seems to be introducing a possible ABBA deadlock due to
the core clock and genpd locking. Here is a backtrace I got from Pete
Zhang (he reported the issue on the internal mailing list):
[ 11.667711][ T108] -> #1 (>mlock){+.+.}-{3:3}:
[ 11.675041][ T108]__lock_acquire+0xae4/0xef8
[ 11.680093][ T108]lock_acquire+0xfc/0x2f8
[ 11.684888][ T108]__mutex_lock+0x90/0x870
[ 11.689685][ T108]mutex_lock_nested+0x44/0x50
[ 11.694826][ T108]genpd_lock_mtx+0x18/0x24
[ 11.699706][ T108]genpd_runtime_resume+0x90/0x214 (hold
genpd->mlock)
[ 11.705194][ T108]__rpm_callback+0x80/0x2c0
[ 11.710160][ T108]rpm_resume+0x468/0x650
[ 11.714866][ T108]__pm_runtime_resume+0x60/0x88
[ 11.720180][ T108]clk_pm_runtime_get+0x28/0x9c
[ 11.725410][ T108]clk_disable_unused_subtree+0x8c/0x144
[ 11.731420][ T108]clk_disable_unused_subtree+0x124/0x144
[ 11.737518][ T108]clk_disable_unused+0xa4/0x11c (hold prepare_lock)
[ 11.742833][ T108]do_one_initcall+0x98/0x178
[ 11.747888][ T108]do_initcall_level+0x9c/0xb8
[ 11.753028][ T108]do_initcalls+0x54/0x94
[ 11.757736][ T108]do_basic_setup+0x24/0x30
[ 11.762614][ T108]kernel_init_freeable+0x70/0xa4
[ 11.768014][ T108]kernel_init+0x14/0x18c
[ 11.772722][ T108]ret_from_fork+0x10/0x18
[ 11.777512][ T108] -> #0 (prepare_lock){+.+.}-{3:3}:
[ 11.784749][ T108]check_noncircular+0x134/0x13c
[ 11.790064][ T108]validate_chain+0x590/0x2a04
[ 11.795204][ T108]__lock_acquire+0xae4/0xef8
[ 11.800258][ T108]lock_acquire+0xfc/0x2f8
[ 11.805050][ T108]__mutex_lock+0x90/0x870
[ 11.809841][ T108]mutex_lock_nested+0x44/0x50
[ 11.814983][ T108]clk_unprepare+0x5c/0x100 ((hold prepare_lock))
[ 11.819864][ T108]imx8m_pd_power_off+0xac/0x110
[ 11.825179][ T108]genpd_power_off+0x1b4/0x2dc
[ 11.830318][ T108]genpd_power_off_work_fn+0x38/0x58 (hold
genpd->mlock)
[ 11.835981][ T108]process_one_work+0x270/0x444
[ 11.841208][ T108]worker_thread+0x280/0x4e4
[ 11.846176][ T108]kthread+0x13c/0x14
[ 11.850621][ T108]ret_from_fork+0x10/0x18
Now, this has been reproduced only on the NXP internal tree, but I think
it is pretty obvious this could happen in upstream too, with this
patchset applied.
First, my thought was to change the prepare_lock/enable_lock in clock
core, from a global approach to a per clock basis. But that doesn't
actually fix the issue.
The usecase seen above is due to clk_disable_unused, but the same could
happen when a clock consumer calls prepare/unprepare on a clock.
I guess the conclusion is that the current state of the clock core and
genpd implementation does not support a usecase where a clock controller
has a PD which in turn uses another clock (from another clock controller).
Jacky, Pete, did I miss anything here ?
Just for completeness, I have a feeling I already managed to trigger
this and discussed this with Lucas before, so this concern is certainly
valid.
Re: [PATCH AUTOSEL 5.10 050/217] rsi: Fix TX EAPOL packet handling against iwlwifi AP
On 12/23/20 3:13 AM, Sasha Levin wrote:
Hello Sasha,
From: Marek Vasut
[ Upstream commit 65277100caa2f2c62b6f3c4648b90d6f0435f3bc ]
In case RSI9116 SDIO WiFi operates in STA mode against Intel 9260 in AP mode,
the association fails. The former is using wpa_supplicant during association,
the later is set up using hostapd:
[...]
Was this patch possibly missed from 5.10.y ?
Also, while at it, I think it might make sense to pick the following two
patches as well, they dramatically reduce interrupt rate of the RSI WiFi
device, so it stops overloading lower-end devices:
287431463e786 ("rsi: Move card interrupt handling to RX thread")
abd131a19f6b8 ("rsi: Clean up loop in the interrupt handler")
Re: [PATCH 02/13] PCI: rcar: Convert to MSI domains
On 2/25/21 4:10 PM, Marc Zyngier wrote: In anticipation of the removal of the msi_controller structure, convert the Rcar host controller driver to MSI domains. We end-up with the usual two domain structure, the top one being a generic PCI/MSI domain, the bottom one being Rcar-specific and handling the actual HW interrupt allocation. Also take the opportunity to get rid of the cargo-culted memory allocation for the MSI capture address. *ANY* sufficiently aligned address should be good enough, so use the physical address of the msi structure instead. On R8A7795 with Intel 600p NVMe SSD and IWLwifi 6235 card, Tested-by: Marek Vasut Thanks.
Re: (.text.ks8851_probe_common+0x370): undefined reference to `__this_module'
On 2/24/21 9:38 AM, Arnd Bergmann wrote: On Wed, Feb 24, 2021 at 3:38 AM Randy Dunlap wrote: On 2/21/21 10:12 PM, kernel test robot wrote: Hi Marek, FYI, the error/warning still remains. tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master head: 31caf8b2a847214be856f843e251fc2ed2cd1075 commit: ef3631220d2b3d8d14cf64464760505baa60d6ac net: ks8851: Register MDIO bus and the internal PHY date: 7 weeks ago config: parisc-randconfig-r034-20210222 (attached as .config) compiler: hppa-linux-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 # https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ef3631220d2b3d8d14cf64464760505baa60d6ac git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git git fetch --no-tags linus master git checkout ef3631220d2b3d8d14cf64464760505baa60d6ac # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=parisc If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All errors (new ones prefixed by >>): hppa-linux-ld: drivers/net/ethernet/micrel/ks8851_common.o: in function `ks8851_probe_common': (.text.ks8851_probe_common+0x370): undefined reference to `__this_module' hppa-linux-ld: (.text.ks8851_probe_common+0x374): undefined reference to `__this_module' Hey Arnd- I wanted to see if you had any ideas about this problem. CONFIG_KS8851=y CONFIG_KS8851_MLL=m The problem is that 2 drivers share some common code, but in one case the shared code is builtin and for the other driver it is a loadable module. The common code is first built as builtin, so it does not have the "__this_module" symbol. This is the patch I sent for it: https://lore.kernel.org/lkml/20210125121937.3900988-1-a...@kernel.org/T/#u I was under the impression that the patch was already applied, wasn't it?
Re: [PATCH] pinctrl: imx: imx8mm: fix pad offset of SD1_DATA0 pin
On 2/11/21 11:17 AM, Frieder Schrempf wrote:
On 11.02.21 10:54, Claudius Heine wrote:
There is a 0 missing in the pad register offset. This patch adds it.
Signed-off-by: Claudius Heine
I think this should rather be prefixed by "arm64: dts: imx8mm:" as this
is no change in the pinctrl driver, but only in the devicetree.
And I guess this deserves a "Fixes" and "Cc: stable" tag, so:
Fixes: c1c9d41319c3 ("dt-bindings: imx: Add pinctrl binding doc for
imx8mm")
Cc: sta...@vger.kernel.org
Reviewed-by: Frieder Schrempf
Indeed.
But since this isn't the first such fix, I wonder whether it wouldn't be
a good idea to regenerate those pinctrl tables and see whether there are
any other such issues in them. I wonder, is there some sort of register
and bit list in machine-parseable form for the MX8M ?
Re: [PATCH v2 2/2] drm: bridge: Add SN65DSI84 DSI to LVDS bridge
On 2/4/21 11:29 PM, Laurent Pinchart wrote: Hi Jagan, Thank you for the patch. On Wed, Feb 03, 2021 at 12:42:56PM +0530, Jagan Teki wrote: SN65DSI84 is a Single Channel DSI to Dual-link LVDS bridge from Texas Instruments. SN65DSI83, SN65DSI85 are variants of the same family of bridge controllers. Right now the bridge driver is supporting a single link, dual-link support requires to initiate I2C Channel B registers. MArek Vasut (on CC) has very recently posted a driver for the SN65DSI86. Should the two drivers be merged together ? Since Jagan's V1 was out first, I will let Jagan pick whatever might be useful from the driver I posted, probably the O(1) clock rate calculation and some of the regmap stuff, and once there is some merged result, I am happy to test it on my hardware. The DSI83 is I think the same as DSI84, except with half of the channels. [...]
Re: [PATCH 1/2] mmc: mmci: enable MMC_CAP_NEED_RSP_BUSY
On 2/4/21 1:54 PM, Yann GAUTIER wrote: On 2/4/21 1:26 PM, Marek Vasut wrote: On 2/4/21 1:05 PM, yann.gaut...@foss.st.com wrote: From: Yann Gautier To properly manage commands awaiting R1B responses, the capability MMC_CAP_NEED_RSP_BUSY is enabled in mmci driver, for variants that manage busy detection. This R1B management needs both the flags MMC_CAP_NEED_RSP_BUSY and MMC_CAP_WAIT_WHILE_BUSY to be enabled together. Shouldn't this have Fixes: tag ? Hi Marek, There is no unique patch that brought the issue, but a combination of several things: - The series that brought the MMC_CAP_NEED_RSP_BUSY flag [1] - The series that enabled MMC_ERASE for all hosts [2] (removal of MMC_CAP_ERASE) But you're right, this patch may go on v5.8.x kernel and newer versions. I think there will be quite some interest in 5.10.y LTS on the MP1 from the various industrial/embedded users, so it would be nice to have that 5.10.y well maintained with necessary backports / fixes :)
Re: [PATCH 1/2] mmc: mmci: enable MMC_CAP_NEED_RSP_BUSY
On 2/4/21 1:05 PM, yann.gaut...@foss.st.com wrote: From: Yann Gautier To properly manage commands awaiting R1B responses, the capability MMC_CAP_NEED_RSP_BUSY is enabled in mmci driver, for variants that manage busy detection. This R1B management needs both the flags MMC_CAP_NEED_RSP_BUSY and MMC_CAP_WAIT_WHILE_BUSY to be enabled together. Shouldn't this have Fixes: tag ?
Re: [RFC 3/3] clk: imx: Add blk-ctl driver for i.MX8MN
On 10/24/20 6:20 PM, Adam Ford wrote: This driver is intended to work with the multimedia block which contains display and camera subsystems: LCDIF ISI MIPI CSI MIPI DSI Signed-off-by: Adam Ford --- drivers/clk/imx/clk-blk-ctl-imx8mn.c | 80 You seem to be missing the Makefile entries.
Re: [PATCH] [5.8 regression] net: ks8851: fix link error
On 1/25/21 1:19 PM, Arnd Bergmann wrote:
From: Arnd Bergmann
An object file cannot be built for both loadable module and built-in
use at the same time:
arm-linux-gnueabi-ld: drivers/net/ethernet/micrel/ks8851_common.o: in function
`ks8851_probe_common':
ks8851_common.c:(.text+0xf80): undefined reference to `__this_module'
Change the ks8851_common code to be a standalone module instead,
and use Makefile logic to ensure this is built-in if at least one
of its two users is.
Fixes: 797047f875b5 ("net: ks8851: Implement Parallel bus operations")
Signed-off-by: Arnd Bergmann
---
Marek sent two other patches to address the problem:
https://lore.kernel.org/netdev/20210116164828.40545-1-ma...@denx.de/
https://lore.kernel.org/netdev/20210115134239.126152-1-ma...@denx.de/
My version is what I applied locally to my randconfig tree, and
I think this is the cleanest solution.
If this version works for all the configuration combinations, then
that's perfect, thanks.
Re: [PATCH] net: ks8851: remove definition of DEBUG
On 1/15/21 4:31 PM, t...@redhat.com wrote: From: Tom Rix Defining DEBUG should only be done in development. So remove DEBUG. Signed-off-by: Tom Rix Reviewed-by: Marek Vasut Thanks
Re: [RFC 0/3] clk: imx: Implement blk-ctl driver for i.MX8MN
On 10/25/20 1:05 PM, Abel Vesa wrote:
[...]
>> Together, both the GPC and the clk-blk driver should be able to pull
>> the multimedia block out of reset. Currently, the GPC can handle the
>> USB OTG and the GPU, but the LCDIF and MIPI DSI appear to be gated by
>> the clock block
>>
>> My original patch RFC didn't include the imx8mn node, because it
>> hangs, but the node I added looks like:
>>
>> media_blk_ctl: clock-controller@32e28000 {
>> compatible = "fsl,imx8mn-media-blk-ctl", "syscon";
>> reg = <0x32e28000 0x1000>;
>> #clock-cells = <1>;
>> #reset-cells = <1>;
>> };
>>
>> I was hoping you might have some feedback on the 8mn clk-blk driver
>> since you did the 8mp clk-blk drive and they appear to be very
>> similar.
>>
>
> I'll do you one better still. I'll apply the patch in my tree and give it
> a test tomorrow morning.
You can also apply the one for 8MM:
https://lore.kernel.org/linux-arm-kernel/20201003224555.163780-5-ma...@denx.de/
Re: [PATCH] arm64: dts: imx8mm: Add GPU node
On 10/22/20 7:16 PM, Adam Ford wrote: > According to the documentation from NXP, the i.MX8M Nano has a > Vivante GC7000 Ultra Lite as its GPU core. > > With this patch, the Etnaviv driver presents the GPU as: >etnaviv-gpu 3800.gpu: model: GC7000, revision: 6203 > > It uses the GPCV2 controller to enable the power domain for the GPU. Subject should say 8mn , not 8mm . Are the assigned-clock-rates correct ?
Re: PHY reset question
On 10/12/20 7:48 AM, Oleksij Rempel wrote:
> Hi all,
>
> thank you for the feedback!
>
> On Fri, Oct 09, 2020 at 04:25:49PM +0200, Bruno Thomsen wrote:
>> Hi Fabio and Oleksij
>>
>> Den ons. 7. okt. 2020 kl. 11.50 skrev Fabio Estevam :
>>>
>>> Hi Oleksij,
>>>
>>> On Tue, Oct 6, 2020 at 5:05 AM Oleksij Rempel
>>> wrote:
Hello PHY experts,
Short version:
what is the proper way to handle the PHY reset before identifying PHY?
Long version:
I stumbled over following issue:
If PHY reset is registered within PHY node. Then, sometimes, we will not
be
able to identify it (read PHY ID), because PHY is under reset.
mdio {
compatible = "virtual,mdio-gpio";
[...]
/* Microchip KSZ8081 */
usbeth_phy: ethernet-phy@3 {
reg = <0x3>;
interrupts-extended = < 12 IRQ_TYPE_LEVEL_LOW>;
reset-gpios = < 11 GPIO_ACTIVE_LOW>;
reset-assert-us = <500>;
reset-deassert-us = <1000>;
};
[...]
};
On simple boards with one PHY per MDIO bus, it is easy to workaround by
using
phy-reset-gpios withing MAC node (illustrated in below DT example),
instead of
using reset-gpios within PHY node (see above DT example).
{
[...]
phy-mode = "rmii";
phy-reset-gpios = < 12 GPIO_ACTIVE_LOW>;
[...]
>>>
>>> I thought this has been fixed by Bruno's series:
>>> https://www.spinics.net/lists/netdev/msg673611.html
>>
>> Yes, that has fixed the Microchip/Micrel PHY ID auto detection
>> issue. I have send a DTS patch v3 that makes use of the newly
>> added device tree parameter:
>> https://lkml.org/lkml/2020/9/23/595
>
> This way is suitable only for boards with single PHY and single reset
> line. But it is not scale on boards with multiple PHY and multiple reset
> lines.
>
> So far, it looks like using compatible like "ethernet-phy-id."
> is the only way to go.
I did further digging in this, and I agree it is either this or reset in
boot loader, sigh.
Re: PHY reset question
On 10/7/20 11:06 AM, Marco Felsch wrote: > On 20-10-07 10:23, Marek Vasut wrote: >> On 10/7/20 10:14 AM, Marco Felsch wrote: >>> Hi Marek, >> >> Hi, >> >> [...] >> >>> On 20-10-06 14:11, Florian Fainelli wrote: >>>> On 10/6/2020 1:24 PM, Marek Vasut wrote: >>> >>> ... >>> >>>>> If this happens on MX6 with FEC, can you please try these two patches? >>>>> >>>>> https://patchwork.ozlabs.org/project/netdev/patch/20201006135253.97395-1-ma...@denx.de/ >>>>> >>>>> https://patchwork.ozlabs.org/project/netdev/patch/20201006202029.254212-1-ma...@denx.de/ >>>> >>>> Your patches are not scaling across multiple Ethernet MAC drivers >>>> unfortunately, so I am not sure this should be even remotely considered a >>>> viable solution. >>> >>> Recently I added clk support for the smcs driver [1] and dropped the >>> PHY_RST_AFTER_CLK_EN flag for LAN8710/20 devices because I had the same >>> issues. Hope this will help you too. >>> >>> [1] https://www.spinics.net/lists/netdev/msg682080.html >> >> I feel this might be starting to go a bit off-topic here, > > You're right, just wanted to provide you a link :) Can you CC me on the next version of those patches ? I seems the LAN8710 is causing grief to many. >> but isn't the >> last patch 5/5 breaking existing setups ? > > IMHO the solution proposed using the PHY_RST_AFTER_CLK_EN was wrong so > we needed to fix that. Yes we need to take care of DT backward > compatibility but we still must be able to fix wrong behaviours within > the driver. I could also argue that PHY_RST_AFTER_CLK_EN solution was > breaking exisitng setups too. > >> The LAN8710 surely does need >> clock enabled before the reset line is toggled. > > Yep and therefore you can specify it yet within the DT. So the idea is that the PHY enables the clock for itself . And if the MAC doesn't export these clock as clk to which you can refer to in DT, then you still need the PHY_RST_AFTER_CLK_EN flag, so the MAC can deal with enabling the clock ? Or is the idea to fix the MAC drivers too ?
Re: PHY reset question
On 10/7/20 10:14 AM, Marco Felsch wrote: > Hi Marek, Hi, [...] > On 20-10-06 14:11, Florian Fainelli wrote: >> On 10/6/2020 1:24 PM, Marek Vasut wrote: > > ... > >>> If this happens on MX6 with FEC, can you please try these two patches? >>> >>> https://patchwork.ozlabs.org/project/netdev/patch/20201006135253.97395-1-ma...@denx.de/ >>> >>> https://patchwork.ozlabs.org/project/netdev/patch/20201006202029.254212-1-ma...@denx.de/ >> >> Your patches are not scaling across multiple Ethernet MAC drivers >> unfortunately, so I am not sure this should be even remotely considered a >> viable solution. > > Recently I added clk support for the smcs driver [1] and dropped the > PHY_RST_AFTER_CLK_EN flag for LAN8710/20 devices because I had the same > issues. Hope this will help you too. > > [1] https://www.spinics.net/lists/netdev/msg682080.html I feel this might be starting to go a bit off-topic here, but isn't the last patch 5/5 breaking existing setups ? The LAN8710 surely does need clock enabled before the reset line is toggled.
Re: PHY reset question
On 10/6/20 11:11 PM, Florian Fainelli wrote:
>
>
> On 10/6/2020 1:24 PM, Marek Vasut wrote:
>> On 10/6/20 9:36 PM, Florian Fainelli wrote:
>> [...]
>>>> - Use compatible ("compatible = "ethernet-phy-id0022.1560") in the
>>>> devicetree,
>>>> so that reading the PHYID is not needed
>>>> - easy to solve.
>>>> Disadvantage:
>>>> - losing PHY auto-detection capability
>>>> - need a new devicetree if different PHY is used (for example in
>>>> different
>>>> board revision)
>>>
>>> Or you can punt that to the boot loader to be able to tell the
>>> difference and populate different compatible, or even manage the PHY
>>> reset to be able to read the actual PHY OUI. To me that is still the
>>> best solution around.
>>
>> Wasn't there some requirement for Linux to be bootloader-independent ?
>
> What kind of dependency does this create here? The fact that Linux is
> capable of parsing a compatible string of the form
> "ethernet-phy." is not something that is exclusively applicable
> to Linux. Linux just so happens to support that, but so could FreeBSD or
> any OS for that matter.
>
> This is exactly the way firmware should be going, that is to describe
> accurately the hardware, while making the life of the OS much easier
> when it can. If we supported ACPI that is exactly what would have to
> happen IMHO.
I should have been more specific, I meant the part where bootloader
should handle the PHY reset. If the kernel code depends on the fact that
the bootloader did PHY reset, then it depends on the bootloader
behavior, and I think that used to be frowned upon.
>> Some systems cannot replace their bootloaders, e.g. if the bootloader is
>> in ROM, so this might not be a solution.
>
> It is always possible to chain load a field updateable boot loader
Not always, but that's another discussion.
>, and
> even when that is not desirable you could devise a solution that allows
> to utilize say a slightly different DTB that you could append to the
> kernel. Again, if you want to use strictly the same DTB, then you have
> to do what I just suggested and have the boot loader absorb some of this
> complexit
That sounds like moving the problem one level down without really
solving it, the bootloader will have this exact same problem -- how does
it determine that the PHY needs reset if it cannot reads its ID ?
>>>> - modify PHY framework to deassert reset before identifying the PHY.
>>>> Disadvantages?
>>
>> If this happens on MX6 with FEC, can you please try these two patches?
>>
>> https://patchwork.ozlabs.org/project/netdev/patch/20201006135253.97395-1-ma...@denx.de/
>>
>>
>> https://patchwork.ozlabs.org/project/netdev/patch/20201006202029.254212-1-ma...@denx.de/
>>
>
> Your patches are not scaling across multiple Ethernet MAC drivers
> unfortunately, so I am not sure this should be even remotely considered
> a viable solution.
Sorry for that . Since Oleksij was running into this problem on MX6 and
I had similar issue on MX6 with LAN8710 PHY, I thought this might be
helpful.
Re: PHY reset question
On 10/6/20 9:36 PM, Florian Fainelli wrote:
[...]
>> - Use compatible ("compatible = "ethernet-phy-id0022.1560") in the
>> devicetree,
>> so that reading the PHYID is not needed
>> - easy to solve.
>> Disadvantage:
>> - losing PHY auto-detection capability
>> - need a new devicetree if different PHY is used (for example in
>> different
>> board revision)
>
> Or you can punt that to the boot loader to be able to tell the
> difference and populate different compatible, or even manage the PHY
> reset to be able to read the actual PHY OUI. To me that is still the
> best solution around.
Wasn't there some requirement for Linux to be bootloader-independent ?
Some systems cannot replace their bootloaders, e.g. if the bootloader is
in ROM, so this might not be a solution.
>> - modify PHY framework to deassert reset before identifying the PHY.
>> Disadvantages?
If this happens on MX6 with FEC, can you please try these two patches?
https://patchwork.ozlabs.org/project/netdev/patch/20201006135253.97395-1-ma...@denx.de/
https://patchwork.ozlabs.org/project/netdev/patch/20201006202029.254212-1-ma...@denx.de/
Thanks!
Re: [PATCH v4 5/5] usb: xhci-pci: Add reset controller support
On 6/12/20 6:46 PM, Nicolas Saenz Julienne wrote: > Some atypical users of xhci-pci might need to manually reset their xHCI > controller before starting the HCD setup. Check if a reset controller > device is available to the PCI bus and trigger a reset. > > Signed-off-by: Nicolas Saenz Julienne > --- > drivers/usb/host/xhci-pci.c | 38 +++-- > 1 file changed, 36 insertions(+), 2 deletions(-) Can the XHCI core code do the reset management instead ? Then everyone benefits from it. The rest of the series is nice, thanks.
Re: [PATCH] PCI: rcar: handle the failure case of pm_runtime_get_sync
On 6/5/20 5:23 AM, Navid Emamdoost wrote: > Calling pm_runtime_get_sync increments the counter even in case of > failure, causing incorrect ref count. Call pm_runtime_put if > pm_runtime_get_sync fails. > > Signed-off-by: Navid Emamdoost This looks like a V2 of [PATCH] PCI: rcar: fix runtime pm imbalance on error This looks good to me, but I'm no runtime-pm expert.
Re: [PATCH] PCI: rcar: fix runtime pm imbalance on error
On 5/20/20 10:22 AM, Dinghao Liu wrote: > pm_runtime_get_sync() increments the runtime PM usage counter even > it returns an error code. Thus a pairing decrement is needed on > the error handling path to keep the counter balanced. Sorry for the late reply. > Signed-off-by: Dinghao Liu > --- > drivers/pci/controller/pcie-rcar.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/pcie-rcar.c > b/drivers/pci/controller/pcie-rcar.c > index 759c6542c5c8..a9de65438051 100644 > --- a/drivers/pci/controller/pcie-rcar.c > +++ b/drivers/pci/controller/pcie-rcar.c > @@ -1207,9 +1207,8 @@ static int rcar_pcie_probe(struct platform_device *pdev) > irq_dispose_mapping(pcie->msi.irq1); > > err_pm_put: You might want to remove this label too. I'm not runtime-pm expert to comment on the validity of this patch though. > - pm_runtime_put(dev); > - > err_pm_disable: > + pm_runtime_put(dev); > pm_runtime_disable(dev); > pci_free_resource_list(>resources); > >
Re: [PATCH v3 0/2] usb: xhci: Load Raspberry Pi 4 VL805's firmware
On 6/4/20 1:18 PM, Nicolas Saenz Julienne wrote: > On Mon, 2020-06-01 at 17:27 +0200, Marek Vasut wrote: >> On 6/1/20 4:41 PM, Nicolas Saenz Julienne wrote: >>> On Mon, 2020-06-01 at 13:12 +0200, Marek Vasut wrote: >>>> On 6/1/20 1:09 PM, Nicolas Saenz Julienne wrote: >>>>> On Mon, 2020-06-01 at 12:53 +0200, Marek Vasut wrote: >>>>>> On 6/1/20 12:47 PM, Nicolas Saenz Julienne wrote: >>>>>>> On Tue, 2020-05-05 at 18:26 +0200, Nicolas Saenz Julienne wrote: >>>>>>>> Newer revisions of the RPi4 need their xHCI chip, VL805, firmware >>>>>>>> to >>>>>>>> be >>>>>>>> loaded explicitly. Earlier versions didn't need that as they where >>>>>>>> using >>>>>>>> an EEPROM for that purpose. This series takes care of setting up >>>>>>>> the >>>>>>>> relevant infrastructure and run the firmware loading routine at >>>>>>>> the >>>>>>>> right moment. >>>>>>>> >>>>>>>> Note that this builds on top of Sylwester Nawrocki's "USB host >>>>>>>> support >>>>>>>> for Raspberry Pi 4 board" series. >>>>>>>> >>>>>>>> --- >>>>>>> >>>>>>> Please don't forget about this series. The new 8GB RPi4 contains >>>>>>> this HW >>>>>>> design >>>>>>> change and USB will not work without it. See this discussion on the >>>>>>> downstream >>>>>>> kernel github, where other OS/bootloaders are hitting the issue: >>>>>>> >>>>>>> https://github.com/raspberrypi/firmware/issues/1402 >>>>>>> >>>>>>> Otherwise, the Linux version of this is already in linux-next: >>>>>>> >>>>>>> >>> > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/usb/host/pci-quirks.c?h=next-20200529=c65822fef4adc0ba40c37a47337376ce75f7a7bc >>>>>> We're already at 2020.07-rc3 , so unless this is a bugfix (does not >>>>>> look >>>>>> that way), this will have to wait for next release cycle. >>>>> >>>>> Of course. As long as it eventually gets in I'm happy (not implying this >>>>> specific series is flawless, but the overall mechanism). I'm just >>>>> worried >>>>> this >>>>> gets lost. >>>>> >>>>>> Also, it seems >>>>>> there was a lengthy ongoing discussion, is that already sorted out ? >>>>> >>>>> Well, there was some discussion on how to incorporate the platform >>>>> specific >>>>> callback into XCHI's code. Which this revision of the series addresses. >>>>> But, >>>>> IIRC, that's pretty much it as far as discussion is concerned. >>>> >>>> Oh, right, since the firmware loading hook looks like a reset hook, why >>>> isn't that implemented via reset controller API instead ? >>> >>> That could be pretty clean, I hadn't though about it that way. Some >>> questions: >>> >>> - Being a PCIe device the XHCI controller doesn't show up in the device- >>> tree. I >>> guess it could be added as a child node of pcie-brcmstb, but is that even >>> acceptable? >> >> Yes, there are other such DTs . >> >>> - Same goes for xhci-pci being a consumer of the reset controller. Given the >>> reset scheme is board specific (the chip can be found all over the place, >>> but >>> the firmware loading scheme is 100% RPi specific), to what extent we can >>> introduce that as a binding? >> >> I'm not sure what you're asking me here, you'll just have some reset >> controller in a DT and a phandle from the xhci-controller to this reset >> controller. > > Sorry I wasn't clear, overall my concern here is that xhic-pci maintainers, > both in u-boot y linux (as I'd like to have the same solution on both sides, > since it involves changes in dt), might see it as too platform specific to add > it into an otherwise generic xhci-pci implmentation. > > But nevermind, I'll just post the series and see what happens :). I think it should be OK, thanks.
Re: [PATCH v3 0/2] usb: xhci: Load Raspberry Pi 4 VL805's firmware
On 6/1/20 4:41 PM, Nicolas Saenz Julienne wrote: > On Mon, 2020-06-01 at 13:12 +0200, Marek Vasut wrote: >> On 6/1/20 1:09 PM, Nicolas Saenz Julienne wrote: >>> On Mon, 2020-06-01 at 12:53 +0200, Marek Vasut wrote: >>>> On 6/1/20 12:47 PM, Nicolas Saenz Julienne wrote: >>>>> On Tue, 2020-05-05 at 18:26 +0200, Nicolas Saenz Julienne wrote: >>>>>> Newer revisions of the RPi4 need their xHCI chip, VL805, firmware to >>>>>> be >>>>>> loaded explicitly. Earlier versions didn't need that as they where >>>>>> using >>>>>> an EEPROM for that purpose. This series takes care of setting up the >>>>>> relevant infrastructure and run the firmware loading routine at the >>>>>> right moment. >>>>>> >>>>>> Note that this builds on top of Sylwester Nawrocki's "USB host support >>>>>> for Raspberry Pi 4 board" series. >>>>>> >>>>>> --- >>>>> >>>>> Please don't forget about this series. The new 8GB RPi4 contains this HW >>>>> design >>>>> change and USB will not work without it. See this discussion on the >>>>> downstream >>>>> kernel github, where other OS/bootloaders are hitting the issue: >>>>> >>>>> https://github.com/raspberrypi/firmware/issues/1402 >>>>> >>>>> Otherwise, the Linux version of this is already in linux-next: >>>>> >>>>> >>> > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/usb/host/pci-quirks.c?h=next-20200529=c65822fef4adc0ba40c37a47337376ce75f7a7bc >>>> We're already at 2020.07-rc3 , so unless this is a bugfix (does not look >>>> that way), this will have to wait for next release cycle. >>> >>> Of course. As long as it eventually gets in I'm happy (not implying this >>> specific series is flawless, but the overall mechanism). I'm just worried >>> this >>> gets lost. >>> >>>> Also, it seems >>>> there was a lengthy ongoing discussion, is that already sorted out ? >>> >>> Well, there was some discussion on how to incorporate the platform specific >>> callback into XCHI's code. Which this revision of the series addresses. But, >>> IIRC, that's pretty much it as far as discussion is concerned. >> >> Oh, right, since the firmware loading hook looks like a reset hook, why >> isn't that implemented via reset controller API instead ? > > That could be pretty clean, I hadn't though about it that way. Some questions: > > - Being a PCIe device the XHCI controller doesn't show up in the device-tree. > I > guess it could be added as a child node of pcie-brcmstb, but is that even > acceptable? Yes, there are other such DTs . > - Same goes for xhci-pci being a consumer of the reset controller. Given the > reset scheme is board specific (the chip can be found all over the place, > but > the firmware loading scheme is 100% RPi specific), to what extent we can > introduce that as a binding? I'm not sure what you're asking me here, you'll just have some reset controller in a DT and a phandle from the xhci-controller to this reset controller.
Re: [PATCH v3 0/2] usb: xhci: Load Raspberry Pi 4 VL805's firmware
On 6/1/20 1:09 PM, Nicolas Saenz Julienne wrote: > On Mon, 2020-06-01 at 12:53 +0200, Marek Vasut wrote: >> On 6/1/20 12:47 PM, Nicolas Saenz Julienne wrote: >>> On Tue, 2020-05-05 at 18:26 +0200, Nicolas Saenz Julienne wrote: >>>> Newer revisions of the RPi4 need their xHCI chip, VL805, firmware to be >>>> loaded explicitly. Earlier versions didn't need that as they where using >>>> an EEPROM for that purpose. This series takes care of setting up the >>>> relevant infrastructure and run the firmware loading routine at the >>>> right moment. >>>> >>>> Note that this builds on top of Sylwester Nawrocki's "USB host support >>>> for Raspberry Pi 4 board" series. >>>> >>>> --- >>> >>> Please don't forget about this series. The new 8GB RPi4 contains this HW >>> design >>> change and USB will not work without it. See this discussion on the >>> downstream >>> kernel github, where other OS/bootloaders are hitting the issue: >>> >>> https://github.com/raspberrypi/firmware/issues/1402 >>> >>> Otherwise, the Linux version of this is already in linux-next: >>> >>> > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/usb/host/pci-quirks.c?h=next-20200529=c65822fef4adc0ba40c37a47337376ce75f7a7bc >> >> We're already at 2020.07-rc3 , so unless this is a bugfix (does not look >> that way), this will have to wait for next release cycle. > > Of course. As long as it eventually gets in I'm happy (not implying this > specific series is flawless, but the overall mechanism). I'm just worried this > gets lost. > >> Also, it seems >> there was a lengthy ongoing discussion, is that already sorted out ? > > Well, there was some discussion on how to incorporate the platform specific > callback into XCHI's code. Which this revision of the series addresses. But, > IIRC, that's pretty much it as far as discussion is concerned. Oh, right, since the firmware loading hook looks like a reset hook, why isn't that implemented via reset controller API instead ?
Re: [PATCH v3 0/2] usb: xhci: Load Raspberry Pi 4 VL805's firmware
On 6/1/20 12:47 PM, Nicolas Saenz Julienne wrote: > On Tue, 2020-05-05 at 18:26 +0200, Nicolas Saenz Julienne wrote: >> Newer revisions of the RPi4 need their xHCI chip, VL805, firmware to be >> loaded explicitly. Earlier versions didn't need that as they where using >> an EEPROM for that purpose. This series takes care of setting up the >> relevant infrastructure and run the firmware loading routine at the >> right moment. >> >> Note that this builds on top of Sylwester Nawrocki's "USB host support >> for Raspberry Pi 4 board" series. >> >> --- > > Please don't forget about this series. The new 8GB RPi4 contains this HW > design > change and USB will not work without it. See this discussion on the downstream > kernel github, where other OS/bootloaders are hitting the issue: > > https://github.com/raspberrypi/firmware/issues/1402 > > Otherwise, the Linux version of this is already in linux-next: > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/usb/host/pci-quirks.c?h=next-20200529=c65822fef4adc0ba40c37a47337376ce75f7a7bc We're already at 2020.07-rc3 , so unless this is a bugfix (does not look that way), this will have to wait for next release cycle. Also, it seems there was a lengthy ongoing discussion, is that already sorted out ?
Re: [PATCH] perf: Make perf able to build with latest libbfd
Hi,
since commit
0ada120c883d ("perf: Make perf able to build with latest libbfd")
is in master, can it be backported to stable as well? I keep hitting
this with too new binutils on Linux 5.4.y and I have to keep
cherry-picking this commit to fix it.
Thanks
[tip: irq/core] genirq: Check irq_data_get_irq_chip() return value before use
The following commit has been merged into the irq/core branch of tip:
Commit-ID: 1d0326f352bb094771df17f045bdbadff89a43e6
Gitweb:
https://git.kernel.org/tip/1d0326f352bb094771df17f045bdbadff89a43e6
Author:Marek Vasut
AuthorDate:Thu, 14 May 2020 02:25:55 +02:00
Committer: Thomas Gleixner
CommitterDate: Thu, 28 May 2020 15:58:04 +02:00
genirq: Check irq_data_get_irq_chip() return value before use
irq_data_get_irq_chip() can return NULL, however it is expected that this
never happens. If a buggy driver leads to NULL being returned from
irq_data_get_irq_chip(), warn about it instead of crashing the machine.
Signed-off-by: Marek Vasut
Signed-off-by: Thomas Gleixner
To: linux-arm-ker...@lists.infradead.org
---
kernel/irq/manage.c | 4
1 file changed, 4 insertions(+)
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 453a8a0..7619111 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -2619,6 +2619,8 @@ int __irq_get_irqchip_state(struct irq_data *data, enum
irqchip_irq_state which,
do {
chip = irq_data_get_irq_chip(data);
+ if (WARN_ON_ONCE(!chip))
+ return -ENODEV;
if (chip->irq_get_irqchip_state)
break;
#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
@@ -2696,6 +2698,8 @@ int irq_set_irqchip_state(unsigned int irq, enum
irqchip_irq_state which,
do {
chip = irq_data_get_irq_chip(data);
+ if (WARN_ON_ONCE(!chip))
+ return -ENODEV;
if (chip->irq_set_irqchip_state)
break;
#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
Re: [PATCH] MAINTAINERS: Add Marek and Shimoda-san as R-Car PCIE co-maintainers
On 10/16/19 2:02 PM, Simon Horman wrote: > At the end of the v5.3 upstream development cycle I stepped down > from my role at Renesas. > > Pass maintainership of the R-Car PCIE to Marek and Shimoda-san. > > Signed-off-by: Simon Horman Co-maintainer model is great: Acked-by: Marek Vasut -- Best regards, Marek Vasut
Re: [PATCH 00/11] of: dma-ranges fixes and improvements
On 9/30/19 2:52 PM, Robin Murphy wrote: > On 30/09/2019 13:40, Marek Vasut wrote: >> On 9/27/19 2:24 AM, Rob Herring wrote: >>> This series fixes several issues related to 'dma-ranges'. Primarily, >>> 'dma-ranges' in a PCI bridge node does correctly set dma masks for PCI >>> devices not described in the DT. A common case needing dma-ranges is a >>> 32-bit PCIe bridge on a 64-bit system. This affects several platforms >>> including Broadcom, NXP, Renesas, and Arm Juno. There's been several >>> attempts to fix these issues, most recently earlier this week[1]. >>> >>> In the process, I found several bugs in the address translation. It >>> appears that things have happened to work as various DTs happen to use >>> 1:1 addresses. >>> >>> First 3 patches are just some clean-up. The 4th patch adds a unittest >>> exhibiting the issues. Patches 5-9 rework how of_dma_configure() works >>> making it work on either a struct device child node or a struct >>> device_node parent node so that it works on bus leaf nodes like PCI >>> bridges. Patches 10 and 11 fix 2 issues with address translation for >>> dma-ranges. >>> >>> My testing on this has been with QEMU virt machine hacked up to set PCI >>> dma-ranges and the unittest. Nicolas reports this series resolves the >>> issues on Rpi4 and NXP Layerscape platforms. >> >> With the following patches applied: >> https://patchwork.ozlabs.org/patch/1144870/ >> https://patchwork.ozlabs.org/patch/1144871/ > > Can you try it without those additional patches? This series aims to > make the parsing work properly generically, such that we shouldn't need > to add an additional PCI-specific version of almost the same code. Seems to work even without those. -- Best regards, Marek Vasut
Re: [PATCH 00/11] of: dma-ranges fixes and improvements
On 9/27/19 2:24 AM, Rob Herring wrote: > This series fixes several issues related to 'dma-ranges'. Primarily, > 'dma-ranges' in a PCI bridge node does correctly set dma masks for PCI > devices not described in the DT. A common case needing dma-ranges is a > 32-bit PCIe bridge on a 64-bit system. This affects several platforms > including Broadcom, NXP, Renesas, and Arm Juno. There's been several > attempts to fix these issues, most recently earlier this week[1]. > > In the process, I found several bugs in the address translation. It > appears that things have happened to work as various DTs happen to use > 1:1 addresses. > > First 3 patches are just some clean-up. The 4th patch adds a unittest > exhibiting the issues. Patches 5-9 rework how of_dma_configure() works > making it work on either a struct device child node or a struct > device_node parent node so that it works on bus leaf nodes like PCI > bridges. Patches 10 and 11 fix 2 issues with address translation for > dma-ranges. > > My testing on this has been with QEMU virt machine hacked up to set PCI > dma-ranges and the unittest. Nicolas reports this series resolves the > issues on Rpi4 and NXP Layerscape platforms. With the following patches applied: https://patchwork.ozlabs.org/patch/1144870/ https://patchwork.ozlabs.org/patch/1144871/ on R8A7795 Salvator-XS Tested-by: Marek Vasut -- Best regards, Marek Vasut
Re: [PATCH net-next 3/3] net: dsa: microchip: remove NET_DSA_TAG_KSZ_COMMON
On 9/6/19 11:30 PM, George McCollister wrote: > Remove the superfluous NET_DSA_TAG_KSZ_COMMON and just use the existing > NET_DSA_TAG_KSZ. Update the description to mention the three switch > families it supports. No functional change. > > Signed-off-by: George McCollister Reviewed-by: Marek Vasut -- Best regards, Marek Vasut
Re: [PATCH net-next 2/3] net: dsa: microchip: add ksz9567 to ksz9477 driver
On 9/6/19 11:30 PM, George McCollister wrote:
> Add support for the KSZ9567 7-Port Gigabit Ethernet Switch to the
> ksz9477 driver. The KSZ9567 supports both SPI and I2C. Oddly the
> ksz9567 is already in the device tree binding documentation.
>
> Signed-off-by: George McCollister
> ---
> drivers/net/dsa/microchip/ksz9477.c | 9 +
> drivers/net/dsa/microchip/ksz9477_i2c.c | 1 +
> drivers/net/dsa/microchip/ksz9477_spi.c | 1 +
> 3 files changed, 11 insertions(+)
>
> diff --git a/drivers/net/dsa/microchip/ksz9477.c
> b/drivers/net/dsa/microchip/ksz9477.c
> index 187be42de5f1..50ffc63d6231 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -1529,6 +1529,15 @@ static const struct ksz_chip_data
> ksz9477_switch_chips[] = {
> .cpu_ports = 0x07, /* can be configured as cpu port */
> .port_cnt = 3, /* total port count */
> },
> + {
> + .chip_id = 0x00956700,
> + .dev_name = "KSZ9567",
> + .num_vlans = 4096,
> + .num_alus = 4096,
> + .num_statics = 16,
> + .cpu_ports = 0x7F, /* can be configured as cpu port */
> + .port_cnt = 7, /* total physical port count */
I might be wrong, and this is just an idea for future improvement, but
is .cpu_ports = GEN_MASK(.port_cnt, 0) always ?
> + },
> };
>
> static int ksz9477_switch_init(struct ksz_device *dev)
> diff --git a/drivers/net/dsa/microchip/ksz9477_i2c.c
> b/drivers/net/dsa/microchip/ksz9477_i2c.c
> index 85fd0fb43941..c1548a43b60d 100644
> --- a/drivers/net/dsa/microchip/ksz9477_i2c.c
> +++ b/drivers/net/dsa/microchip/ksz9477_i2c.c
> @@ -77,6 +77,7 @@ MODULE_DEVICE_TABLE(i2c, ksz9477_i2c_id);
> static const struct of_device_id ksz9477_dt_ids[] = {
> { .compatible = "microchip,ksz9477" },
> { .compatible = "microchip,ksz9897" },
> + { .compatible = "microchip,ksz9567" },
> {},
> };
> MODULE_DEVICE_TABLE(of, ksz9477_dt_ids);
> diff --git a/drivers/net/dsa/microchip/ksz9477_spi.c
> b/drivers/net/dsa/microchip/ksz9477_spi.c
> index 2e402e4d866f..f4198d6f72be 100644
> --- a/drivers/net/dsa/microchip/ksz9477_spi.c
> +++ b/drivers/net/dsa/microchip/ksz9477_spi.c
> @@ -81,6 +81,7 @@ static const struct of_device_id ksz9477_dt_ids[] = {
> { .compatible = "microchip,ksz9893" },
> { .compatible = "microchip,ksz9563" },
> { .compatible = "microchip,ksz8563" },
> + { .compatible = "microchip,ksz9567" },
> {},
> };
> MODULE_DEVICE_TABLE(of, ksz9477_dt_ids);
>
Reviewed-by: Marek Vasut
--
Best regards,
Marek Vasut
Re: [PATCH net-next 1/3] net: dsa: microchip: add KSZ9477 I2C driver
On 9/6/19 11:30 PM, George McCollister wrote: [...] > --- /dev/null > +++ b/drivers/net/dsa/microchip/ksz9477_i2c.c > @@ -0,0 +1,100 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Microchip KSZ9477 series register access through I2C > + * > + * Copyright (C) 2018-2019 Microchip Technology Inc. Doesn't the copyright need update ? > + */ > + > +#include > +#include > +#include > +#include Please keep the headers sorted. > +#include "ksz_common.h" > + > +KSZ_REGMAP_TABLE(ksz9477, not_used, 16, 0, 0); > + The rest looks good. [...] -- Best regards, Marek Vasut
Re: [PATCH] net: dsa: microchip: fill regmap_config name
On 8/29/19 4:14 PM, George McCollister wrote:
> Use the register value width as the regmap_config name to prevent the
> following error when the second and third regmap_configs are
> initialized.
> "debugfs: Directory '${bus-id}' with parent 'regmap' already present!"
>
> Signed-off-by: George McCollister
Reviewed-by: Marek Vasut
Re: [PATCH] Input: ili210x - switch to using threaded IRQ
On 8/11/19 6:11 PM, Dmitry Torokhov wrote: > Let's switch the driver to using threaded IRQ so that we do not need to > manage the interrupt and work separately, and we do not acknowledge > interrupt until we finished handling it completely. > > Signed-off-by: Dmitry Torokhov On ILI2117 Tested-by: Marek Vasut
Re: [PATCH] iio: adc: gyroadc: fix uninitialized return code
On 7/4/19 2:10 PM, Geert Uytterhoeven wrote:
> Hi Wolfram,
>
> On Thu, Jul 4, 2019 at 2:08 PM Wolfram Sang wrote:
>> On Thu, Jul 04, 2019 at 01:37:47PM +0200, Arnd Bergmann wrote:
>>> gcc-9 complains about a blatant uninitialized variable use that
>>> all earlier compiler versions missed:
>>>
>>> drivers/iio/adc/rcar-gyroadc.c:510:5: warning: 'ret' may be used
>>> uninitialized in this function [-Wmaybe-uninitialized]
>>>
>>> Return -EINVAL instead here.
>>>
>>> Cc: sta...@vger.kernel.org
>>> Fixes: 059c53b32329 ("iio: adc: Add Renesas GyroADC driver")
>>> Signed-off-by: Arnd Bergmann
>>
>> This is correct but missing that the above 'return ret' is broken, too.
>> ret is initialized but 0 in that case.
>
> Nice catch! Oh well, given enough eyeballs, ...
I don't think ret is initialized, reg is, not ret .
>> And maybe we can use something else than -EINVAL for this case? I am on
>> the go right now, I will look for a suggestion later.
>
> -EINVAL is correct here (and in the above case, too), IMHO.
Yep, -EINVAL is fine.
--
Best regards,
Marek Vasut
Re: [RFC][PATCH] regmap: Drop CONFIG_64BIT checks from core
On 6/26/19 1:15 PM, Mark Brown wrote: > On Wed, Jun 26, 2019 at 01:31:16AM +0200, Marek Vasut wrote: >> Drop the CONFIG_64BIT checks from core regmap code, as it is well >> possible to access e.g. an SPI device with 64bit registers from a >> 32bit CPU. The CONFIG_64BIT checks are still left in place in the >> regmap mmio code however. > > The issue with 8 bit byte > registers was that we use unsigned long for > addresses and values and a 64 bit value won't fit in those on a 32 bit > system. Some of the bulk APIs will work but things like individual > register writes and the caches will have problems. Good thing I sent this as RFC, I realized that shortly after too. So, what would be the suggestion here ? -- Best regards, Marek Vasut
[RFC][PATCH] regmap: Drop CONFIG_64BIT checks from core
Drop the CONFIG_64BIT checks from core regmap code, as it is well
possible to access e.g. an SPI device with 64bit registers from a
32bit CPU. The CONFIG_64BIT checks are still left in place in the
regmap mmio code however.
Signed-off-by: Marek Vasut
Cc: Rafael J. Wysocki
Cc: Mark Brown
---
drivers/base/regmap/regcache.c | 4
drivers/base/regmap/regmap.c | 14 --
2 files changed, 18 deletions(-)
diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index a93cafd7be4f..e443d9de3f7e 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -577,14 +577,12 @@ bool regcache_set_val(struct regmap *map, void *base,
unsigned int idx,
cache[idx] = val;
break;
}
-#ifdef CONFIG_64BIT
case 8: {
u64 *cache = base;
cache[idx] = val;
break;
}
-#endif
default:
BUG();
}
@@ -618,13 +616,11 @@ unsigned int regcache_get_val(struct regmap *map, const
void *base,
return cache[idx];
}
-#ifdef CONFIG_64BIT
case 8: {
const u64 *cache = base;
return cache[idx];
}
-#endif
default:
BUG();
}
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 19f57ccfbe1d..7da9dbb98d8a 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -298,7 +298,6 @@ static void regmap_format_32_native(void *buf, unsigned int
val,
*(u32 *)buf = val << shift;
}
-#ifdef CONFIG_64BIT
static void regmap_format_64_be(void *buf, unsigned int val, unsigned int
shift)
{
__be64 *b = buf;
@@ -318,7 +317,6 @@ static void regmap_format_64_native(void *buf, unsigned int
val,
{
*(u64 *)buf = (u64)val << shift;
}
-#endif
static void regmap_parse_inplace_noop(void *buf)
{
@@ -407,7 +405,6 @@ static unsigned int regmap_parse_32_native(const void *buf)
return *(u32 *)buf;
}
-#ifdef CONFIG_64BIT
static unsigned int regmap_parse_64_be(const void *buf)
{
const __be64 *b = buf;
@@ -440,7 +437,6 @@ static unsigned int regmap_parse_64_native(const void *buf)
{
return *(u64 *)buf;
}
-#endif
static void regmap_lock_hwlock(void *__map)
{
@@ -921,7 +917,6 @@ struct regmap *__regmap_init(struct device *dev,
}
break;
-#ifdef CONFIG_64BIT
case 64:
switch (reg_endian) {
case REGMAP_ENDIAN_BIG:
@@ -937,7 +932,6 @@ struct regmap *__regmap_init(struct device *dev,
goto err_hwlock;
}
break;
-#endif
default:
goto err_hwlock;
@@ -998,7 +992,6 @@ struct regmap *__regmap_init(struct device *dev,
goto err_hwlock;
}
break;
-#ifdef CONFIG_64BIT
case 64:
switch (val_endian) {
case REGMAP_ENDIAN_BIG:
@@ -1019,7 +1012,6 @@ struct regmap *__regmap_init(struct device *dev,
goto err_hwlock;
}
break;
-#endif
}
if (map->format.format_write) {
@@ -2081,11 +2073,9 @@ int regmap_bulk_write(struct regmap *map, unsigned int
reg, const void *val,
case 4:
ival = *(u32 *)(val + (i * val_bytes));
break;
-#ifdef CONFIG_64BIT
case 8:
ival = *(u64 *)(val + (i * val_bytes));
break;
-#endif
default:
ret = -EINVAL;
goto out;
@@ -2809,9 +2799,7 @@ int regmap_bulk_read(struct regmap *map, unsigned int
reg, void *val,
for (i = 0; i < val_count * val_bytes; i += val_bytes)
map->format.parse_inplace(val + i);
} else {
-#ifdef CONFIG_64BIT
u64 *u64 = val;
-#endif
u32 *u32 = val;
u16 *u16 = val;
u8 *u8 = val;
@@ -2827,11 +2815,9 @@ int regmap_bulk_read(struct regmap *map, unsigned int
reg, void *val,
goto out;
switch (map->format.val_bytes) {
-#ifdef CONFIG_64BIT
case 8:
u64[i] = ival;
break;
-#endif
case 4:
u32[i] = ival;
break;
--
2.20.1
Re: Kernel touch Kconfig consult
On 6/23/19 9:02 AM, Dmitry Torokhov wrote: > Hi, > > On Fri, Jun 14, 2019 at 06:47:19AM -0400, luhua.xu wrote: >> Hi Dmitry,Rob,Marek, Nick,Richard,Martin, >> >> In our customer support experience, many smartphone have two or three >> touch vendor mixture , and customer use one load to support all touches. >> For easy to config touch driver we use kernel config like this down >> below, >> >> We change the config type from 'bool' to 'string'. >> >> config TOUCHSCREEN_MTK_TOUCH >> string "Touch IC name for Mediatek package" >> help >> Set touch IC name if you have touch panel. >> To compile this dirver for used touch IC. >> >> >> And we config touch driver like this： >> CONFIG_TOUCHSCREEN_MTK_TOUCH="GT9886 GT1151 TD4320" >> >> I only use one config to support 3 touches, while we have to use 3 >> config to support 3 touch drivers if we set the config as 'bool'. >> >> So can I use Kconfig like this? >> I do look forward to receiving your reply at your convenience . >> > > I really do not see why having a sting is easier to have than 3 bools, > especially if they pertain to different touch controllers. You must also > have some custom processing of the config above as I am pretty sure our > standard build tools would not work for it. I might be missing something obvious, but isn't DT something you want to use on your ARM device to describe the hardware , instead of hard-coding it into the kernel configuration ? I recently worked with MT6797 (the Gemini PDA SoC), and the vendorkernel does exactly this, it's a spectacular display of ifdeffery and Kconfig chaos, so I suspect this is where the idea of putting stuff into Kconfig comes from. -- Best regards, Marek Vasut
Re: [PATCH v13 3/3] dt-bindings: mfd: Document Renesas R-Car Gen3 RPC-IF controller bindings
On 6/6/19 9:40 AM, masonccy...@mxic.com.tw wrote: [...] > RPC-IF works either in SPI or HyperFlash is decided by external hardware > pins > configuration and it can NOT switch it's operation mode in the run time. > This is not like my understanding of MFD. Which external hardware pins decide the RPC configuration ? It seems to me like PHYCNT register, PHYMEM bitfield, selects what device is connected, and then a couple of other bits control the communication, but I see nothing which would be tied to any external configuration pins. [...] -- Best regards, Marek Vasut
Re: [PATCH v2 1/3] mtd: spinand: Add #define-s for page-read ops with three-byte addresses
On 5/14/19 11:53 PM, Jeff Kletsky wrote: > From: Jeff Kletsky That #define in $subject is called a macro. Seems this patch adds a lot of almost duplicate code, can it be somehow de-duplicated ? > The GigaDevice GD5F1GQ4UFxxG SPI NAND utilizes three-byte addresses > for its page-read ops. > > http://www.gigadevice.com/datasheet/gd5f1gq4xfxxg/ > > Signed-off-by: Jeff Kletsky > --- > include/linux/mtd/spinand.h | 30 ++ > 1 file changed, 30 insertions(+) > > diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h > index b92e2aa955b6..05fe98eebe27 100644 > --- a/include/linux/mtd/spinand.h > +++ b/include/linux/mtd/spinand.h > @@ -68,30 +68,60 @@ > SPI_MEM_OP_DUMMY(ndummy, 1), \ > SPI_MEM_OP_DATA_IN(len, buf, 1)) > > +#define SPINAND_PAGE_READ_FROM_CACHE_OP_3A(fast, addr, ndummy, buf, len) \ > + SPI_MEM_OP(SPI_MEM_OP_CMD(fast ? 0x0b : 0x03, 1), \ > +SPI_MEM_OP_ADDR(3, addr, 1), \ > +SPI_MEM_OP_DUMMY(ndummy, 1), \ > +SPI_MEM_OP_DATA_IN(len, buf, 1)) > + > #define SPINAND_PAGE_READ_FROM_CACHE_X2_OP(addr, ndummy, buf, len) \ > SPI_MEM_OP(SPI_MEM_OP_CMD(0x3b, 1), \ > SPI_MEM_OP_ADDR(2, addr, 1), \ > SPI_MEM_OP_DUMMY(ndummy, 1), \ > SPI_MEM_OP_DATA_IN(len, buf, 2)) > > +#define SPINAND_PAGE_READ_FROM_CACHE_X2_OP_3A(addr, ndummy, buf, len) > \ > + SPI_MEM_OP(SPI_MEM_OP_CMD(0x3b, 1), \ > +SPI_MEM_OP_ADDR(3, addr, 1), \ > +SPI_MEM_OP_DUMMY(ndummy, 1), \ > +SPI_MEM_OP_DATA_IN(len, buf, 2)) > + > #define SPINAND_PAGE_READ_FROM_CACHE_X4_OP(addr, ndummy, buf, len) \ > SPI_MEM_OP(SPI_MEM_OP_CMD(0x6b, 1), \ > SPI_MEM_OP_ADDR(2, addr, 1), \ > SPI_MEM_OP_DUMMY(ndummy, 1), \ > SPI_MEM_OP_DATA_IN(len, buf, 4)) > > +#define SPINAND_PAGE_READ_FROM_CACHE_X4_OP_3A(addr, ndummy, buf, len) > \ > + SPI_MEM_OP(SPI_MEM_OP_CMD(0x6b, 1), \ > +SPI_MEM_OP_ADDR(3, addr, 1), \ > +SPI_MEM_OP_DUMMY(ndummy, 1), \ > +SPI_MEM_OP_DATA_IN(len, buf, 4)) > + > #define SPINAND_PAGE_READ_FROM_CACHE_DUALIO_OP(addr, ndummy, buf, len) > \ > SPI_MEM_OP(SPI_MEM_OP_CMD(0xbb, 1), \ > SPI_MEM_OP_ADDR(2, addr, 2), \ > SPI_MEM_OP_DUMMY(ndummy, 2), \ > SPI_MEM_OP_DATA_IN(len, buf, 2)) > > +#define SPINAND_PAGE_READ_FROM_CACHE_DUALIO_OP_3A(addr, ndummy, buf, len) \ > + SPI_MEM_OP(SPI_MEM_OP_CMD(0xbb, 1), \ > +SPI_MEM_OP_ADDR(3, addr, 2), \ > +SPI_MEM_OP_DUMMY(ndummy, 2), \ > +SPI_MEM_OP_DATA_IN(len, buf, 2)) > + > #define SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(addr, ndummy, buf, len) > \ > SPI_MEM_OP(SPI_MEM_OP_CMD(0xeb, 1), \ > SPI_MEM_OP_ADDR(2, addr, 4), \ > SPI_MEM_OP_DUMMY(ndummy, 4), \ > SPI_MEM_OP_DATA_IN(len, buf, 4)) > > +#define SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP_3A(addr, ndummy, buf, len) \ > + SPI_MEM_OP(SPI_MEM_OP_CMD(0xeb, 1), \ > +SPI_MEM_OP_ADDR(3, addr, 4), \ > +SPI_MEM_OP_DUMMY(ndummy, 4), \ > +SPI_MEM_OP_DATA_IN(len, buf, 4)) > + > #define SPINAND_PROG_EXEC_OP(addr) \ > SPI_MEM_OP(SPI_MEM_OP_CMD(0x10, 1), \ > SPI_MEM_OP_ADDR(3, addr, 1), \ > -- Best regards, Marek Vasut
Re: [PATCH] mtd: spi-nor: enable 4B opcodes for n25q256a
On 5/3/19 12:37 PM, Simon Goldschmidt wrote: > On Fri, May 3, 2019 at 12:00 PM Marek Vasut wrote: >> >> On 5/3/19 10:53 AM, Simon Goldschmidt wrote: >>> Tested on socfpga cyclone5 where this is required to ensure that the >>> boot rom can access this flash after warm reboot. >> >> Are you sure _all_ variants of the N25Q256 support 4NB opcodes ? >> I think there were some which didn't, but I might be wrong. > > Oh, damn, you're right. The documentation [1] statest that 4-byte erase and > program opcodes are only supported for part numbers N25Q256A83ESF40x, > N25Q256A83E1240x and N25QA83ESFA0F. ;-) > Any idea of how I can still enable 4-byte opcodes for my chip? Maybe SFDP tables contains some information whether the chip supports the 4B opcodes ? -- Best regards, Marek Vasut
Re: [PATCH] mtd: spi-nor: enable 4B opcodes for n25q256a
On 5/3/19 10:53 AM, Simon Goldschmidt wrote:
> Tested on socfpga cyclone5 where this is required to ensure that the
> boot rom can access this flash after warm reboot.
Are you sure _all_ variants of the N25Q256 support 4NB opcodes ?
I think there were some which didn't, but I might be wrong.
> Signed-off-by: Simon Goldschmidt
> ---
>
> drivers/mtd/spi-nor/spi-nor.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index fae147452..4cdec2cc2 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1874,7 +1874,7 @@ static const struct flash_info spi_nor_ids[] = {
> { "n25q064a",INFO(0x20bb17, 0, 64 * 1024, 128, SECT_4K |
> SPI_NOR_QUAD_READ) },
> { "n25q128a11", INFO(0x20bb18, 0, 64 * 1024, 256, SECT_4K |
> SPI_NOR_QUAD_READ) },
> { "n25q128a13", INFO(0x20ba18, 0, 64 * 1024, 256, SECT_4K |
> SPI_NOR_QUAD_READ) },
> - { "n25q256a",INFO(0x20ba19, 0, 64 * 1024, 512, SECT_4K |
> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> + { "n25q256a",INFO(0x20ba19, 0, 64 * 1024, 512, SECT_4K |
> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
> { "n25q256ax1", INFO(0x20bb19, 0, 64 * 1024, 512, SECT_4K |
> SPI_NOR_QUAD_READ) },
> { "n25q512a",INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR |
> SPI_NOR_QUAD_READ) },
> { "n25q512ax3", INFO(0x20ba20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR |
> SPI_NOR_QUAD_READ) },
>
--
Best regards,
Marek Vasut
Re: [PATCH v2 0/4] Patches to allow consistent mmc / mmcblk numbering w/ device tree
On 3/17/19 4:43 PM, Russell King - ARM Linux admin wrote:
> On Sun, Mar 17, 2019 at 04:05:14PM +0100, Stefan Agner wrote:
>> On 16.03.2019 16:39, Russell King - ARM Linux admin wrote:
>>> On Sat, Mar 16, 2019 at 01:33:58PM +0100, Marek Vasut wrote:
>>>> If you have a FS or partition table there, it does.
>>>> If you don't, I agree ... that's a problem.
>>>
>>> eMMC boot partitions are called mmcblkXbootY, and unless you have more
>>> than one eMMC device on the system, they can be found either by looking
>>> for /dev/mmcblk*boot* or by querying udev. The advantage of using udev
>>> is you can discover the physical device behind it by looking at DEVPATH,
>>> ID_PATH, etc, but you may not have that installed on an embedded device.
>>>
>>> However, as I say, just looking for /dev/mmcblk*boot* is sufficient to
>>> find the eMMC boot partitions where there is just one eMMC device
>>> present (which seems to be the standard setup.)
>>>
>>>>> I don't care the slightest what the numbering is, as long as it is
>>>>> stable. On some hardware, with an unpatched kernel, the mmc device
>>>>> numbering changes depending on whether or not an SD card is inserted on
>>>>> boot. Getting rid of that behaviour is really all I want.
>>>>
>>>> Agreed, that would be an improvement.
>>>
>>> The mmc device numbering was tied to the mmc host numbering a while back
>>> and the order that the hosts are probed should be completely independent
>>> of whether a card is inserted or not:
>>>
>>> snprintf(md->disk->disk_name, sizeof(md->disk->disk_name),
>>> "mmcblk%u%s", card->host->index, subname ? subname : "");
>>>
>>> snprintf(rpmb_name, sizeof(rpmb_name),
>>> "mmcblk%u%s", card->host->index, subname ? subname : "");
>>>
>>> I suspect that Mans is quoting something from the dim and distant past
>>> to confuse the issue - as shown above, it is now dependent on the host
>>> numbering order not the order in which cards are inserted.
>>
>> Commit 9aaf3437aa72 ("mmc: block: Use the mmc host device index as the
>> mmcblk device index") which came in with v4.6 enables constant mmc block
>> device numbering. I can confirm that it works nicely, and it improved
>> the situation a lot.
>>
>> That being said, we still use a patch downstream which allows
>> renumbering using an alias. We deal with a bunch of different boards
>> with different SoC's. I have a couple of SD cards with various rootfs
>> and use internal eMMC boot quite often as well. Remembering which board
>> uses which numbering is a pain. Maintaining a patch is just easier...
>> Furthermore, U-Boot allows reordering and all boards I deal with use mmc
>> 0 for the internal eMMC. The aliases allow consistency.
>
> Maybe eMMC should've been given a different block device name?
I presume that's because they have hardware and software partitioning
and that's why you want to discern those two.
--
Best regards,
Marek Vasut
Re: [PATCH v2 0/4] Patches to allow consistent mmc / mmcblk numbering w/ device tree
On 3/16/19 1:22 PM, Måns Rullgård wrote: > Marek Vasut writes: > >> On 3/15/19 10:52 PM, Tim Harvey wrote: >>> Tim Harvey - Principal Software EngineerGateworks Corporation - >>> http://www.gateworks.com/3026 S. Higuera St. San Luis Obispo CA >>> 93401805-781-2000 >>> On Tue, Mar 5, 2019 at 4:39 AM Måns Rullgård wrote: >>>> >>>> Douglas Anderson writes: >>>> >>>>> This series picks patches from various different places to produce what >>>>> I consider the best solution to getting consistent mmc and mmcblk >>>>> ordering. >>>>> >>>>> Why consistent ordering and why not just use UUIDs? IMHO consistent >>>>> ordering solves a few different problems: >>>>> >>>>> 1. For poor, feeble-minded humans like me, have sane numbering for >>>>>devices helps a lot. When grepping through dmesg it's terribly handy >>>>>if a given SDMMC device has a consistent number. I know that I can >>>>>do "dmesg | grep mmc0" or "dmesg | grep mmcblk0" to find info about >>>>>the eMMC. I know that I can do "dmesg | grep mmc1" to find info >>>>>about the SD card slot. I don't want it to matter which one probed >>>>>first, I don't want it to matter if I'm working on a variant of the >>>>>hardware that has the SD card slot disabled, and I don't want to care >>>>>what my boot device was. Worrying about what device number I got >>>>>increases my cognitive load. >>>>> >>>>> 2. There are cases where it's not trivially easy during development to >>>>>use the UUID. Specifically I work a lot with coreboot / depthcharge >>>>>as a BIOS. When configured properly, that BIOS has a nice feature to >>>>>allow you to fetch the kernel and kernel command line from TFTP by >>>>>pressing Ctrl-N. In this particular case the BIOS doesn't actually >>>>>know which disk I'd like for my root filesystem, so it's not so easy >>>>>for it to put the right UUID into the command line. For this >>>>>purpose, knowing that "mmcblk0" will always refer to eMMC is handy. >>>>> >>>>> Changes in v2: >>>>> - Rebased atop mmc-next >>>>> - Stat dynamic allocation after fixed allocation; thanks Wolfram! >>>>> - rk3288 patch new for v2 >>>>> >>>>> Douglas Anderson (1): >>>>> ARM: dts: rockchip: Add mmc aliases for rk3288 platform >>>>> >>>>> Jaehoon Chung (1): >>>>> Documentation: mmc: Document mmc aliases >>>>> >>>>> Stefan Agner (2): >>>>> mmc: read mmc alias from device tree >>>>> mmc: use SD/MMC host ID for block device name ID >>>>> >>>>> Documentation/devicetree/bindings/mmc/mmc.txt | 11 +++ >>>>> arch/arm/boot/dts/rk3288.dtsi | 4 >>>>> drivers/mmc/card/block.c | 2 +- >>>>> drivers/mmc/core/host.c | 17 - >>>>> 4 files changed, 32 insertions(+), 2 deletions(-) >>>> >>>> Did anyone ever come up with an acceptable solution for this? After >>>> three years, I'm getting tired of rebasing these patches onto every new >>>> kernel. >>>> >>>> UUIDs or similar are NOT an option for multiple reasons: >>>> >>>> - We have two rootfs partitions for ping-pong updates, so simply >>>> referring to "the thing with ID foo" doesn't work. >>>> >>>> - Installing said updates needs direct access the device/partition, >>>> which may not even have a filesystem. >>>> >>>> - The u-boot environment is stored in an eMMC "boot" partition, and >>>> userspace needs to know where to find it. >>>> >>>> I'm sure I'm not the only one in a similar situation. >>>> >>>> Russel, feel free to shout abuse at me. I don't care, but it makes you >>>> look stupid. >>>> >>> >>> Completely agree here - we need a dt solution that allows us to >>> specify ordering. >> >> Nope, ordering would be a policy and does not describe hardware, thus it >> shouldn't be in the DT. Use UUID or PARTUUID, they apply both to raw FS >> (fsuuid) and to partitions (part uuid). Linux kernel can mount FS using >> PARTUUID, to support UUID you need initramfs. > > That doesn't address how to find eMMC boot partitions. If you have a FS or partition table there, it does. If you don't, I agree ... that's a problem. > I don't care the slightest what the numbering is, as long as it is > stable. On some hardware, with an unpatched kernel, the mmc device > numbering changes depending on whether or not an SD card is inserted on > boot. Getting rid of that behaviour is really all I want. Agreed, that would be an improvement. -- Best regards, Marek Vasut
Re: [PATCH v2 0/4] Patches to allow consistent mmc / mmcblk numbering w/ device tree
On 3/15/19 10:52 PM, Tim Harvey wrote:
> Tim Harvey - Principal Software EngineerGateworks Corporation -
> http://www.gateworks.com/3026 S. Higuera St. San Luis Obispo CA
> 93401805-781-2000
> On Tue, Mar 5, 2019 at 4:39 AM Måns Rullgård wrote:
>>
>> Douglas Anderson writes:
>>
>>> This series picks patches from various different places to produce what
>>> I consider the best solution to getting consistent mmc and mmcblk
>>> ordering.
>>>
>>> Why consistent ordering and why not just use UUIDs? IMHO consistent
>>> ordering solves a few different problems:
>>>
>>> 1. For poor, feeble-minded humans like me, have sane numbering for
>>>devices helps a lot. When grepping through dmesg it's terribly handy
>>>if a given SDMMC device has a consistent number. I know that I can
>>>do "dmesg | grep mmc0" or "dmesg | grep mmcblk0" to find info about
>>>the eMMC. I know that I can do "dmesg | grep mmc1" to find info
>>>about the SD card slot. I don't want it to matter which one probed
>>>first, I don't want it to matter if I'm working on a variant of the
>>>hardware that has the SD card slot disabled, and I don't want to care
>>>what my boot device was. Worrying about what device number I got
>>>increases my cognitive load.
>>>
>>> 2. There are cases where it's not trivially easy during development to
>>>use the UUID. Specifically I work a lot with coreboot / depthcharge
>>>as a BIOS. When configured properly, that BIOS has a nice feature to
>>>allow you to fetch the kernel and kernel command line from TFTP by
>>>pressing Ctrl-N. In this particular case the BIOS doesn't actually
>>>know which disk I'd like for my root filesystem, so it's not so easy
>>>for it to put the right UUID into the command line. For this
>>>purpose, knowing that "mmcblk0" will always refer to eMMC is handy.
>>>
>>> Changes in v2:
>>> - Rebased atop mmc-next
>>> - Stat dynamic allocation after fixed allocation; thanks Wolfram!
>>> - rk3288 patch new for v2
>>>
>>> Douglas Anderson (1):
>>> ARM: dts: rockchip: Add mmc aliases for rk3288 platform
>>>
>>> Jaehoon Chung (1):
>>> Documentation: mmc: Document mmc aliases
>>>
>>> Stefan Agner (2):
>>> mmc: read mmc alias from device tree
>>> mmc: use SD/MMC host ID for block device name ID
>>>
>>> Documentation/devicetree/bindings/mmc/mmc.txt | 11 +++
>>> arch/arm/boot/dts/rk3288.dtsi | 4
>>> drivers/mmc/card/block.c | 2 +-
>>> drivers/mmc/core/host.c | 17 -
>>> 4 files changed, 32 insertions(+), 2 deletions(-)
>>
>> Did anyone ever come up with an acceptable solution for this? After
>> three years, I'm getting tired of rebasing these patches onto every new
>> kernel.
>>
>> UUIDs or similar are NOT an option for multiple reasons:
>>
>> - We have two rootfs partitions for ping-pong updates, so simply
>> referring to "the thing with ID foo" doesn't work.
>>
>> - Installing said updates needs direct access the device/partition,
>> which may not even have a filesystem.
>>
>> - The u-boot environment is stored in an eMMC "boot" partition, and
>> userspace needs to know where to find it.
>>
>> I'm sure I'm not the only one in a similar situation.
>>
>> Russel, feel free to shout abuse at me. I don't care, but it makes you
>> look stupid.
>>
>
> Completely agree here - we need a dt solution that allows us to
> specify ordering.
Nope, ordering would be a policy and does not describe hardware, thus it
shouldn't be in the DT. Use UUID or PARTUUID, they apply both to raw FS
(fsuuid) and to partitions (part uuid). Linux kernel can mount FS using
PARTUUID, to support UUID you need initramfs.
> I support a variety of IMX6 boards where for PCB routing reasons the
> bootable MMC device is not always the first sdhc (sometimes the first
> one is an SDIO radio for example). It seems ridiculous that I can't
> handle this with:
>
> aliases {
> mmc0 = /* MMC boot device */
> mmc1 = /* SDIO radio */
> };
>
> I see the imx6q-dhcom-som added in
> 52c7a088badd665a09ca9307ffa91e88d5686a7d re-defines the default
> imx6qdl.dtsi mmc0-mmc3 aiases but I don't see any handling of this in
> code anywhere - am I missing something?
>
> Marek, why did you change the alias ordering for imx6q-dhcom-som.dtsi?
> (maybe your carrying around a patch to make this useful?)
Nope, likely a cleanup remnant which can be dropped.
> + aliases {
> + mmc0 =
> + mmc1 =
> + mmc2 =
> + mmc3 =
> + };
>
> Regards,
>
> Tim
>
--
Best regards,
Marek Vasut
Re: [PATCH] gpio: of: Restrict enable-gpio quirk to regulator-gpio
On 2/20/19 11:52 AM, Thierry Reding wrote:
> From: Thierry Reding
>
> Commit 0e7d6f940164 ("gpio: of: Apply regulator-gpio quirk only to
> enable-gpios") breaks the device tree ABI specified in the device tree
> bindings for fixed regulators (compatible "regulator-fixed"). According
> to these bindings the polarity of the GPIO is exclusively controlled by
> the presence or absence of the enable-active-high property. As such the
> polarity quirk implemented in of_gpio_flags_quirks() must be applied to
> the GPIO specified for fixed regulators.
>
> However, commit 0e7d6f940164 ("gpio: of: Apply regulator-gpio quirk only
> to enable-gpios") restricted the quirk to the enable-gpios property for
> fixed regulators as well, whereas according to the commit message itself
> it should only apply to "regulator-gpio" compatible device tree nodes.
>
> Fix this by actually implementing what the offending commit intended,
> which is to ensure that the quirk is applied to the GPIO specified by
> the "enable-gpio" property for the "regulator-gpio" bindings only.
>
> This fixes a regression on Jetson TX1 where the fixed regulator for the
> HDMI +5V pin relies on the flags quirk for the proper polarity.
>
> Fixes: 0e7d6f940164 ("gpio: of: Apply regulator-gpio quirk only to
> enable-gpios")
> Signed-off-by: Thierry Reding
> ---
> drivers/gpio/gpiolib-of.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 1b4c741e0635..bddfc6102a50 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -84,10 +84,10 @@ static void of_gpio_flags_quirks(struct device_node *np,
>* Note that active low is the default.
>*/
> if (IS_ENABLED(CONFIG_REGULATOR) &&
> - !strcmp(propname, "enable-gpio") &&
> (of_device_is_compatible(np, "regulator-fixed") ||
>of_device_is_compatible(np, "reg-fixed-voltage") ||
> - of_device_is_compatible(np, "regulator-gpio"))) {
> + (of_device_is_compatible(np, "regulator-gpio") &&
> + strcmp(propname, "enable-gpio") == 0))) {
> /*
>* The regulator GPIO handles are specified such that the
>* presence or absence of "enable-active-high" solely controls
On R8A7795 Salvator-XS
Tested-by: Marek Vasut
--
Best regards,
Marek Vasut
Re: Applied "spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver" to the spi tree
On 2/14/19 10:12 AM, masonccy...@mxic.com.tw wrote: > Hi, Hi, >> "Marek Vasut" >> 2019/02/13 下午 08:37 >> >> On 2/13/19 1:16 PM, Mark Brown wrote: >> > On Wed, Feb 13, 2019 at 04:25:32PM +0800, masonccy...@mxic.com.tw wrote: >> > >> >> From current mainline branch, MFD seems support the device which is on >> >> the same hardware bus(i.e, I2C, SPI, MMIO and SPMI)for multi-function >> >> by Read/Write the common same registers. >> > >> > That's most MFDs but there are some that do some level of enumeration >> > (even if it's just looking at the device ID that got registered) to >> > decide what subdevices get registered, that's what people are suggesting >> > here I think. >> >> Right. Although I think some of the code could be shared between the SPI >> and HF modes. > > If it is right that MFD is based on the same hardware bus for multi > function > device,i.e,. based on SPI/I2C/PCI bus to register device by > mfd_add_devices(). > > For a multi function device works on different hardware bus, it has nothing > to do with mfd-core.c(MFD framework) even though their driver are in > drivers/mfd directory, i.e,. mcp-core.x, sm501.c. > > Since RPC-IF works on different hardware bus for SPI bus or CFI > HyperFlash bus, > is it a good idea to implement MFD framework for RPC-IF ? > Or we just separate RPC-IF driver by spi mode and cfi mode ? > > any comments/opinions on RPC-IF/MFD-framework is welcome! Mark mentioned it before, you can use the MFD for MMIO too. The goal here is to have some common code which is shared by the SPI and HF part of the driver, and then a separate SPI handling code and HF handling code. The common code should determine which part to activate based on the DT. -- Best regards, Marek Vasut
Re: Applied "spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver" to the spi tree
On 2/13/19 1:16 PM, Mark Brown wrote: > On Wed, Feb 13, 2019 at 04:25:32PM +0800, masonccy...@mxic.com.tw wrote: > >> From current mainline branch, MFD seems support the device which is on >> the same hardware bus(i.e, I2C, SPI, MMIO and SPMI)for multi-function >> by Read/Write the common same registers. > > That's most MFDs but there are some that do some level of enumeration > (even if it's just looking at the device ID that got registered) to > decide what subdevices get registered, that's what people are suggesting > here I think. Right. Although I think some of the code could be shared between the SPI and HF modes. >> I am checking and not sure if MMIO of MFD could support RPC-IF for >> different hardware bus on SPI and CFI. >> I also doubt if this method is a correct solution for RPC-IF works >> either in SPI mode or CFI mode. > > For MMIO devices MFD just passes through the parent resources. > -- Best regards, Marek Vasut
Re: Applied "spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver" to the spi tree
On 2/12/19 3:22 PM, Mark Brown wrote: > The patch > >spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver > > has been applied to the spi tree at > >https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git > > All being well this means that it will be integrated into the linux-next > tree (usually sometime in the next 24 hours) and sent to Linus during > the next merge window (or sooner if it is a bug fix), however if > problems are discovered then the patch may be dropped or reverted. > > You may get further e-mails resulting from automated or manual testing > and review of the tree, please engage with people reporting problems and > send followup patches addressing any issues that are reported if needed. > > If any updates are required or you are submitting further changes they > should be sent as incremental updates against current git, existing > patches will not be replaced. > > Please add any relevant lists and maintainers to the CCs when replying > to this mail. How did that happen when there were still comments and open topics ? -- Best regards, Marek Vasut
Re: [PATCH] Input: ili210x - switch to using devm_device_add_group()
On 2/7/19 7:27 AM, Dmitry Torokhov wrote:
> By switching to devm_device_add_group() we can complete driver conversion
> to using managed resources and get rid of ili210x_i2c_remove().
>
> Signed-off-by: Dmitry Torokhov
> ---
> drivers/input/touchscreen/ili210x.c | 16 ++--
> 1 file changed, 2 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/input/touchscreen/ili210x.c
> b/drivers/input/touchscreen/ili210x.c
> index 6cfe463ac118..af1dd9cff12a 100644
> --- a/drivers/input/touchscreen/ili210x.c
> +++ b/drivers/input/touchscreen/ili210x.c
> @@ -376,7 +376,7 @@ static int ili210x_i2c_probe(struct i2c_client *client,
> return error;
> }
>
> - error = sysfs_create_group(>kobj, _attr_group);
> + error = devm_device_add_group(dev, _attr_group);
> if (error) {
> dev_err(dev, "Unable to create sysfs attributes, err: %d\n",
> error);
> @@ -386,7 +386,7 @@ static int ili210x_i2c_probe(struct i2c_client *client,
> error = input_register_device(priv->input);
> if (error) {
> dev_err(dev, "Cannot register input device, err: %d\n", error);
> - goto err_remove_sysfs;
> + return error;
> }
>
> device_init_wakeup(dev, 1);
> @@ -396,17 +396,6 @@ static int ili210x_i2c_probe(struct i2c_client *client,
> client->irq, firmware.id, firmware.major, firmware.minor);
>
> return 0;
> -
> -err_remove_sysfs:
> - sysfs_remove_group(>kobj, _attr_group);
> - return error;
> -}
> -
> -static int ili210x_i2c_remove(struct i2c_client *client)
> -{
> - sysfs_remove_group(>dev.kobj, _attr_group);
> -
> - return 0;
> }
>
> static int __maybe_unused ili210x_i2c_suspend(struct device *dev)
> @@ -454,7 +443,6 @@ static struct i2c_driver ili210x_ts_driver = {
> },
> .id_table = ili210x_i2c_id,
> .probe = ili210x_i2c_probe,
> - .remove = ili210x_i2c_remove,
> };
>
> module_i2c_driver(ili210x_ts_driver);
>
Reviewed-by: Marek Vasut
On ILI251x
Tested-by: Marek Vasut
--
Best regards,
Marek Vasut
Re: [EXT] Re: [PATCH v2] mtd: spi-nor: Fix wrong abbreviation HWCPAS
On 2/8/19 7:43 PM, Bean Huo (beanhuo) wrote: > Hi,Boris > I sent three times, seems last time is successful. would you check that is > correct? > > git send-email 0001-mtd-spi-nor-Fix-wrong-abbreviation-HWCPAS.patch > 0001-mtd-spi-nor-Fix-wrong-abbreviation-HWCPAS.patch git send-email --annotate --to=... --cc=... --cc=... 000*patch This will likely make your life easier, rather than having to paste various email addresses to git send-email queries. [...] -- Best regards, Marek Vasut
Re: [PATCH] rtc: rv3028: new driver
t; +
> +#define RV3028_CTRL1_EERDBIT(3)
> +#define RV3028_CTRL1_WADABIT(5)
> +
> +#define RV3028_CTRL2_RESET BIT(0)
> +#define RV3028_CTRL2_12_24 BIT(1)
> +#define RV3028_CTRL2_EIE BIT(2)
> +#define RV3028_CTRL2_AIE BIT(3)
> +#define RV3028_CTRL2_TIE BIT(4)
> +#define RV3028_CTRL2_UIE BIT(5)
> +#define RV3028_CTRL2_TSE BIT(7)
> +
> +#define RV3028_EVT_CTRL_TSR BIT(2)
> +
> +#define RV3028_EEPROM_CMD_WRITE 0x21
> +#define RV3028_EEPROM_CMD_READ 0x22
> +
> +#define RV3028_EEBUSY_POLL 1
> +#define RV3028_EEBUSY_TIMEOUT10
> +
> +#define RV3028_BACKUP_TCEBIT(5)
> +#define RV3028_BACKUP_TCR_MASK GENMASK(1,0)
> +
> +#define OFFSET_STEP_PPT 953674
> +
> +enum rv3028_type {
> + rv_3028,
> +};
> +
> +struct rv3028_data {
> + struct regmap *regmap;
> + struct rtc_device *rtc;
> + enum rv3028_type type;
> +};
> +
> +static u32 rv3028_trickle_resistors[] = {1000, 3000, 6000, 11000};
u16 ?
The rest looks good to me.
--
Best regards,
Marek Vasut
Re: [PATCH v7 1/2] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver
On 1/30/19 3:22 AM, masonccy...@mxic.com.tw wrote: > Hi Marek, Hi, >> "Marek Vasut" >> 2019/01/29 下午 12:45 >> >> To >> >> masonccy...@mxic.com.tw, >> >> cc >> >> bbrezil...@kernel.org, broo...@kernel.org, "Geert Uytterhoeven" >> , "Simon Horman" , >> julie...@mxic.com.tw, linux-kernel@vger.kernel.org, linux-renesas- >> s...@vger.kernel.org, linux-...@vger.kernel.org, >> sergei.shtyl...@cogentembedded.com, zhengxu...@mxic.com.tw >> >> Subject >> >> Re: [PATCH v7 1/2] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller > driver >> >> On 1/29/19 3:26 AM, masonccy...@mxic.com.tw wrote: >> > Hi Marek, >> >> Hi, >> >> >> >> "Marek Vasut" >> >> >> >> >> > +module_platform_driver(rpc_spi_driver); >> >> >> >> >> >> >> >> >> >> RPC is not a SPI controller, it's a SPI and HF controller. >> >> >> >> >> >> >> >> >> >> Also, how difficult will it be to add the HF support ? >> >> >> >> > >> >> >> >> > One of my customers needs RPC SPI driver for our company's >> >> >> >> > Octal-Flash,MX25UW51245G. >> >> >> >> > We don't have HF product and hope you could understanding. >> >> >> >> >> >> >> >> I am worried that when we need to add RPC HF support (which is >> > what all >> >> >> >> boards but the D3 Draak use), we will have to rewrite the entire >> > driver >> >> >> >> and/or convert it to MFD and that would be a tremendous >> >> > undertaking. I'd >> >> >> >> prefer to have the driver ready for the HF addition before it's >> >> > accepted >> >> >> >> upstream. >> >> >> >> >> >> >> > >> >> >> > I think maybe your concerned would be happened only if HF driver >> >> > goes with >> >> >> > spi-mem layer. >> >> >> > >> >> >> > A comment for HF from Daniel Fishman. FYR. >> >> >> > >> >> >> > https://www.quora.com/What-is-a-hyper-flash-memory-and-how-is-it- >> >> >> different-from-normal-flash-memory >> >> >> >> >> >> I have a decent idea what HF and SPI NOR are, since I wrote the RPC >> >> >> driver for both HF and SPI mode for U-Boot (as I mentioned earlier). >> >> >> >> >> >> The HF in Linux would use the CFI NOR part of MTD framework. My > concern >> >> >> is that when we need to add HF support into this driver, this driver >> >> >> will have to be basically rewritten, since the architecture > won't allow >> >> >> for that. I'd like to avoid that, since the majority of Gen3 boards, >> >> >> expect for the D3 Draak, use RPC in HF mode. >> >> > >> >> > FYI~ >> >> > >> >> > MX25UW51245g(64MByte Octa) S26KL512S(64MByte HF) >> >> > 8 IO 8 IO >> >> > 200MHz DDR@1.8v 166MHz DDR@1.8v >> >> > >> >> > support Read-while-write Not support >> >> > good for OTA,etc >> >> > powerful application >> >> >> >> What does that mean ? >> > >> > I have no idea why would you say "since the majority of Gen3 boards use >> > RPC in HF mode" ? >> >> Well, the H3/M3W/M3N S-X(S) and the H3/M3 ULCB and E3 Ebisu all boot >> from HF. Only the D3 Draak uses QSPI NOR. > > It's understandable because mx25uw51245g is a new product and it has been > adopted by Renesas’ Automotive Instrument Cluster RH850/D1M1A MCU. The aforementioned boards are not going away however. There's too many users to ignore those. > We also have patched R-Car's BL for booting from Octa-Flash as bellow log: > > NOTICE: BL2: R-Car D3 Initial Program Loader(CA53) Rev.0.5.1 > NOTICE: BL2: PRR is R-Car D3 Ver1.0 > NOTICE: BL2: Boot device is MXIC_OctaFlash > NOTICE: BL2: LCM state is CM > NOTICE: BL2: DDR3L-1866(rev.0.02) > NOTICE: BL2: QoS is default setting(rev.0.07) > NOTICE: BL2: v1.3(release): > NOTICE: BL2: Built : 09:56:31, Sep 26 2018 > NOTICE: BL2: Normal
Re: [PATCH v7 1/2] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver
On 1/29/19 3:26 AM, masonccy...@mxic.com.tw wrote: > Hi Marek, Hi, >> >> "Marek Vasut" >> >> >> >> > +module_platform_driver(rpc_spi_driver); >> >> >> >> >> >> >> >> RPC is not a SPI controller, it's a SPI and HF controller. >> >> >> >> >> >> >> >> Also, how difficult will it be to add the HF support ? >> >> >> > >> >> >> > One of my customers needs RPC SPI driver for our company's >> >> >> > Octal-Flash,MX25UW51245G. >> >> >> > We don't have HF product and hope you could understanding. >> >> >> >> >> >> I am worried that when we need to add RPC HF support (which is > what all >> >> >> boards but the D3 Draak use), we will have to rewrite the entire > driver >> >> >> and/or convert it to MFD and that would be a tremendous >> > undertaking. I'd >> >> >> prefer to have the driver ready for the HF addition before it's >> > accepted >> >> >> upstream. >> >> >> >> >> > >> >> > I think maybe your concerned would be happened only if HF driver >> > goes with >> >> > spi-mem layer. >> >> > >> >> > A comment for HF from Daniel Fishman. FYR. >> >> > >> >> > https://www.quora.com/What-is-a-hyper-flash-memory-and-how-is-it- >> >> different-from-normal-flash-memory >> >> >> >> I have a decent idea what HF and SPI NOR are, since I wrote the RPC >> >> driver for both HF and SPI mode for U-Boot (as I mentioned earlier). >> >> >> >> The HF in Linux would use the CFI NOR part of MTD framework. My concern >> >> is that when we need to add HF support into this driver, this driver >> >> will have to be basically rewritten, since the architecture won't allow >> >> for that. I'd like to avoid that, since the majority of Gen3 boards, >> >> expect for the D3 Draak, use RPC in HF mode. >> > >> > FYI~ >> > >> > MX25UW51245g(64MByte Octa) S26KL512S(64MByte HF) >> > 8 IO 8 IO >> > 200MHz DDR@1.8v 166MHz DDR@1.8v >> > >> > support Read-while-write Not support >> > good for OTA,etc >> > powerful application >> >> What does that mean ? > > I have no idea why would you say "since the majority of Gen3 boards use > RPC in HF mode" ? Well, the H3/M3W/M3N S-X(S) and the H3/M3 ULCB and E3 Ebisu all boot from HF. Only the D3 Draak uses QSPI NOR. > So far as I know that HF is provided by Cypress only and > any mass production product use the component which is provided by only > one provider > will be a big risk. > > Compare to HF, there are more provider of SPI/Octa could support the > mass production product > as their second provider. > > In addition, from the technical points of view, mx25uw51245g is more > powerful than HF and > good for complicate user application, i.e., OTA and so on. Did you consider protocol overhead too ? I don't think you can compare them just by raw numbers of pins and bus frequency. Note that over-the-air update (if that's what you mean by OTA) is completely separate from the underlying storage device. > I think customer have more choice for their flash memory component. Note that none of this is really relevant to my concerns above regarding HF support. This driver should be implemented as MFD driver and the SPI part should use the MFD core part , just like the future HF part. -- Best regards, Marek Vasut
Re: [PATCH v7 1/2] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver
On 1/28/19 2:38 AM, masonccy...@mxic.com.tw wrote: > Hi Marek, Hi, >> "Marek Vasut" >> >> >> > +module_platform_driver(rpc_spi_driver); >> >> >> >> >> >> RPC is not a SPI controller, it's a SPI and HF controller. >> >> >> >> >> >> Also, how difficult will it be to add the HF support ? >> >> > >> >> > One of my customers needs RPC SPI driver for our company's >> >> > Octal-Flash,MX25UW51245G. >> >> > We don't have HF product and hope you could understanding. >> >> >> >> I am worried that when we need to add RPC HF support (which is what all >> >> boards but the D3 Draak use), we will have to rewrite the entire driver >> >> and/or convert it to MFD and that would be a tremendous > undertaking. I'd >> >> prefer to have the driver ready for the HF addition before it's > accepted >> >> upstream. >> >> >> > >> > I think maybe your concerned would be happened only if HF driver > goes with >> > spi-mem layer. >> > >> > A comment for HF from Daniel Fishman. FYR. >> > >> > https://www.quora.com/What-is-a-hyper-flash-memory-and-how-is-it- >> different-from-normal-flash-memory >> >> I have a decent idea what HF and SPI NOR are, since I wrote the RPC >> driver for both HF and SPI mode for U-Boot (as I mentioned earlier). >> >> The HF in Linux would use the CFI NOR part of MTD framework. My concern >> is that when we need to add HF support into this driver, this driver >> will have to be basically rewritten, since the architecture won't allow >> for that. I'd like to avoid that, since the majority of Gen3 boards, >> expect for the D3 Draak, use RPC in HF mode. > > FYI~ > > MX25UW51245g(64MByte Octa) S26KL512S(64MByte HF) > 8 IO 8 IO > 200MHz DDR@1.8v 166MHz DDR@1.8v > > support Read-while-write Not support > good for OTA,etc > powerful application What does that mean ? -- Best regards, Marek Vasut
Re: [PATCH v7 1/2] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver
On 1/24/19 7:28 AM, masonccy...@mxic.com.tw wrote: > Hi Marek, Hi, >> "Marek Vasut" >> 2019/01/24 上午 11:14 >> >> >> >> > +module_platform_driver(rpc_spi_driver); >> >> >> >> RPC is not a SPI controller, it's a SPI and HF controller. >> >> >> >> Also, how difficult will it be to add the HF support ? >> > >> > One of my customers needs RPC SPI driver for our company's >> > Octal-Flash,MX25UW51245G. >> > We don't have HF product and hope you could understanding. >> >> I am worried that when we need to add RPC HF support (which is what all >> boards but the D3 Draak use), we will have to rewrite the entire driver >> and/or convert it to MFD and that would be a tremendous undertaking. I'd >> prefer to have the driver ready for the HF addition before it's accepted >> upstream. >> > > I think maybe your concerned would be happened only if HF driver goes with > spi-mem layer. > > A comment for HF from Daniel Fishman. FYR. > > https://www.quora.com/What-is-a-hyper-flash-memory-and-how-is-it-different-from-normal-flash-memory I have a decent idea what HF and SPI NOR are, since I wrote the RPC driver for both HF and SPI mode for U-Boot (as I mentioned earlier). The HF in Linux would use the CFI NOR part of MTD framework. My concern is that when we need to add HF support into this driver, this driver will have to be basically rewritten, since the architecture won't allow for that. I'd like to avoid that, since the majority of Gen3 boards, expect for the D3 Draak, use RPC in HF mode. -- Best regards, Marek Vasut
Re: [PATCH v7 1/2] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver
On 1/24/19 3:23 AM, masonccy...@mxic.com.tw wrote: > Hi Marek, Hi, >> "Marek Vasut" >> 2019/01/24 上午 09:54 >> >> >> > +#define RPC_CMNCR 0x // R/W >> >> Is there any reason for using those horrible C++ comments ? > > By Mark's comments for the SPDX header needs to be C++ style and > I patch the whole RPC driver comments using C++ style otherwise it looks > messy. I think the C++ comments should only be applied to the SPDX identifier, maybe the header, but not the entire file. >> [...] >> >> > +module_platform_driver(rpc_spi_driver); >> >> RPC is not a SPI controller, it's a SPI and HF controller. >> >> Also, how difficult will it be to add the HF support ? > > One of my customers needs RPC SPI driver for our company's > Octal-Flash,MX25UW51245G. > We don't have HF product and hope you could understanding. I am worried that when we need to add RPC HF support (which is what all boards but the D3 Draak use), we will have to rewrite the entire driver and/or convert it to MFD and that would be a tremendous undertaking. I'd prefer to have the driver ready for the HF addition before it's accepted upstream. -- Best regards, Marek Vasut
Re: [PATCH v7 2/2] dt-bindings: spi: Document Renesas R-Car Gen3 RPC-IF controller bindings
On 1/23/19 8:09 AM, Mason Yang wrote:
> Document the bindings used by the Renesas R-Car Gen3 RPC-IF controller.
>
> Signed-off-by: Mason Yang
> ---
> .../devicetree/bindings/spi/spi-renesas-rpc.txt| 46
> ++
> 1 file changed, 46 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
>
> diff --git a/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> new file mode 100644
> index 000..305bd10
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> @@ -0,0 +1,46 @@
> +Renesas R-Car Gen3 RPC-IF controller Device Tree Bindings
> +
> +
> +Required properties:
> +- compatible: should be an SoC-specific compatible value, followed by
> + "renesas,rcar-gen3-rpc" as a fallback, i.e.,
> + "renesas,r8a77995-rpc", "renesas,rcar-gen3-rpc".
> + "renesas,r8a7795-rpc" (R-Car H3)
> + "renesas,r8a7796-rpc" (R-Car M3-W)
> + "renesas,r8a77965-rpc" (R-Car M3-N)
> + "renesas,r8a77970-rpc" (R-Car V3M)
> + "renesas,r8a77980-rpc" (R-Car V3H)
> + "renesas,r8a77990-rpc" (R-Car E3)
> + "renesas,r8a77995-rpc" (R-Car D3)
Was it tested on all of those SoCs and do we already handle all the
quirks of those ?
> +- reg: should contain three register areas:
> + first for the base address of rpc-if registers,
> + second for the direct mapping read mode and
> + third for the write buffer area.
> +- reg-names: should contain "regs", "dirmap" and "wbuf"
> +- clocks: should contain 1 entries for the module's clock
> +- clock-names: should contain "rpc"
> +- #address-cells: should be 1
> +- #size-cells: should be 0
> +
> +Example:
> +
> + rpc: rpc@ee20 {
> + compatible = "renesas,r8a77995-rpc", "renesas,rcar-gen3-rpc";
> + reg = <0 0xee20 0 0x200>, <0 0x0800 0 0x400>,
> + <0 0xee208000 0 0x100>;
> + reg-names = "regs", "dirmap", "wbuf";
> + clocks = < CPG_MOD 917>;
> + clock-names = "rpc";
> + power-domains = < R8A77995_PD_ALWAYS_ON>;
> + resets = < 917>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + flash@0 {
> + compatible = "jedec,spi-nor";
> + reg = <0>;
> + spi-max-frequency = <4000>;
> + spi-tx-bus-width = <1>;
> + spi-rx-bus-width = <1>;
Is the bus width really 1 or is it 4 on the D3 ?
> + };
> + };
>
--
Best regards,
Marek Vasut
Re: [PATCH v7 1/2] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver
On 1/23/19 8:09 AM, Mason Yang wrote:
> Add a driver for Renesas R-Car Gen3 RPC-IF SPI controller.
>
> Signed-off-by: Mason Yang
> Signed-off-by: Sergei Shtylyov
[...]
> +#define RPC_CMNCR0x // R/W
Is there any reason for using those horrible C++ comments ?
[...]
> +module_platform_driver(rpc_spi_driver);
RPC is not a SPI controller, it's a SPI and HF controller.
Also, how difficult will it be to add the HF support ?
> +MODULE_AUTHOR("Mason Yang ");
> +MODULE_DESCRIPTION("Renesas R-Car Gen3 RPC-IF SPI controller driver");
> +MODULE_LICENSE("GPL v2");
>
--
Best regards,
Marek Vasut
Re: [PATCH v6 2/2] dt-bindings: spi: Document Renesas R-Car Gen3 RPC-IF controller bindings
On 1/18/19 9:10 AM, Geert Uytterhoeven wrote:
> Hi Marek,
>
> On Fri, Jan 18, 2019 at 9:08 AM Marek Vasut wrote:
>> On 1/18/19 9:03 AM, Geert Uytterhoeven wrote:
>>> On Fri, Jan 18, 2019 at 6:55 AM Mason Yang wrote:
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
>
>>>> +Example:
>>>> +
>>>> + rpc: rpc@ee20 {
>>>> + compatible = "renesas,rcar-gen3-rpc";
>>>
>>> compatible = "renesas,r8a7795-rpc," renesas,rcar-gen3-rpc";
>>
>> Without the extra comma after r8a7795-rpc, of course ;-)
>
> Let's move it after the closing double quotes. Would that be OK for you? ^-)
Ack
--
Best regards,
Marek Vasut
Re: [PATCH v6 2/2] dt-bindings: spi: Document Renesas R-Car Gen3 RPC-IF controller bindings
On 1/18/19 9:03 AM, Geert Uytterhoeven wrote:
> Hi Mason,
>
> On Fri, Jan 18, 2019 at 6:55 AM Mason Yang wrote:
>> Document the bindings used by the Renesas R-Car Gen3 RPC-IF controller.
>>
>> Signed-off-by: Mason Yang
>
> Thanks for the update!
>
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
>> @@ -0,0 +1,37 @@
>> +Renesas R-Car Gen3 RPC-IF controller Device Tree Bindings
>> +--
>> +
>> +Required properties:
>> +- compatible: should be
>
> compatible: should be an SoC-specific compatible value, followed by
> "renesas,rcar-gen3-rpc" as a fallback.
>
> Examples of the latter are:
> - "renesas,r8a7795-rpc" (R-Car H3).
>
> This makes it future-proof, in case the RPC on a specific SoC version needs
> to be handled specially. We already know that is the case for R-Car V3M.
>
>> +- #address-cells: should be 1
>> +- #size-cells: should be 0
>> +- reg: should contain three register areas:
>> + first for the base address of rpc-if registers,
>> + second for the direct mapping read mode and
>> + third for the write buffer area.
>> +- reg-names: should contain "regs", "dirmap" and "wbuf"
>> +- clock-names: should contain "rpc"
>> +- clocks: should contain 1 entries for the module's clock
>> +
>> +Example:
>> +
>> + rpc: rpc@ee20 {
>> + compatible = "renesas,rcar-gen3-rpc";
>
> compatible = "renesas,r8a7795-rpc," renesas,rcar-gen3-rpc";
Without the extra comma after r8a7795-rpc, of course ;-)
--
Best regards,
Marek Vasut
Re: [PATCH v6 2/2] dt-bindings: spi: Document Renesas R-Car Gen3 RPC-IF controller bindings
On 1/18/19 8:41 AM, masonccy...@mxic.com.tw wrote:
> Hi Marek,
Hi,
>> "Marek Vasut"
>> 2019/01/18 下午 03:10
>>
>> > +Renesas R-Car Gen3 RPC-IF controller Device Tree Bindings
>> > +--
>> > +
>> > +Required properties:
>> > +- compatible: should be "renesas,rcar-gen3-rpc"
>> > +- #address-cells: should be 1
>> > +- #size-cells: should be 0
>> > +- reg: should contain three register areas:
>> > + first for the base address of rpc-if registers,
>> > + second for the direct mapping read mode and
>> > + third for the write buffer area.
>> > +- reg-names: should contain "regs", "dirmap" and "wbuf"
>> > +- clock-names: should contain "rpc"
>> > +- clocks: should contain 1 entries for the module's clock
>> > +
>> > +Example:
>> > +
>> > + rpc: rpc@ee20 {
>> > + compatible = "renesas,rcar-gen3-rpc";
>> > + reg = <0 0xee20 0 0x7fff>, <0 0x0800 0 0x400>,
>>
>> 0x7fff should be 0x8000 , right ?
>
> RPC write buffer starts at 0xee208000 w/ 256 bytes size.
The size of the RPC-IF block is 0x8000 though, is it not ?
>> > + <0 0xee208000 0 0x100>;
>>
>> Isn't the write buffer part of the RPC-IF register set ?
>
> yep, but by Sergei and Geert's comments, we use a separate reg entry
> for this write buffer.
> copy the replied email from Geert -
>> Maybe Geert or Marek could comment on it, thanks.
>
> Given the writer buffer is separate from the other registers (it's in
> a different
> 4 KiB page, and thus may be at a different offset on future SoCs), and not
> present on RZ/A1, I think it's a good idea to use a separate reg entry
> for it.
> ---
+CC Chris, the RZ/A1 has no write buffer for the RPC ?
--
Best regards,
Marek Vasut
Re: [PATCH v6 2/2] dt-bindings: spi: Document Renesas R-Car Gen3 RPC-IF controller bindings
On 1/18/19 6:54 AM, Mason Yang wrote:
> Document the bindings used by the Renesas R-Car Gen3 RPC-IF controller.
>
> Signed-off-by: Mason Yang
> ---
> .../devicetree/bindings/spi/spi-renesas-rpc.txt| 37
> ++
> 1 file changed, 37 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
>
> diff --git a/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> new file mode 100644
> index 000..9b5001e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt
> @@ -0,0 +1,37 @@
> +Renesas R-Car Gen3 RPC-IF controller Device Tree Bindings
> +--
> +
> +Required properties:
> +- compatible: should be "renesas,rcar-gen3-rpc"
> +- #address-cells: should be 1
> +- #size-cells: should be 0
> +- reg: should contain three register areas:
> + first for the base address of rpc-if registers,
> + second for the direct mapping read mode and
> + third for the write buffer area.
> +- reg-names: should contain "regs", "dirmap" and "wbuf"
> +- clock-names: should contain "rpc"
> +- clocks: should contain 1 entries for the module's clock
> +
> +Example:
> +
> + rpc: rpc@ee20 {
> + compatible = "renesas,rcar-gen3-rpc";
> + reg = <0 0xee20 0 0x7fff>, <0 0x0800 0 0x400>,
0x7fff should be 0x8000 , right ?
> + <0 0xee208000 0 0x100>;
Isn't the write buffer part of the RPC-IF register set ?
> + reg-names = "regs", "dirmap", ";
> + clocks = < CPG_MOD 917>;
> + power-domains = < R8A77995_PD_ALWAYS_ON>;
> + resets = < 917>;
> + clock-names = "rpc";
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + flash@0 {
> + compatible = "jedec,spi-nor";
> + reg = <0>;
> + spi-max-frequency = <4000>;
> + spi-tx-bus-width = <1>;
> + spi-rx-bus-width = <1>;
> + };
> + };
>
--
Best regards,
Marek Vasut
Re: [PATCH] MAINTAINERS: add myself as SPI NOR co-maintainer
On 1/15/19 5:57 PM, tudor.amba...@microchip.com wrote: > From: Tudor Ambarus > > I have been reviewing and contributing to the SPI NOR subsystem > for the last few months and I'm willing to continue doing so. > Volunteer as a maintainer for the SPI NOR part of the MTD subsystem. > > Signed-off-by: Tudor Ambarus Acked-by: Marek Vasut > --- > MAINTAINERS | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 6682420421c1..7deb0e91e3ed 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -14081,6 +14081,7 @@ F:arch/arm/mach-spear/ > > SPI NOR SUBSYSTEM > M: Marek Vasut > +M: Tudor Ambarus > L: linux-...@lists.infradead.org > W: http://www.linux-mtd.infradead.org/ > Q: http://patchwork.ozlabs.org/project/linux-mtd/list/ > -- Best regards, Marek Vasut
Re: [PATCH v5 2/2] dt-bindings: spi: Document Renesas R-Car RPC-IF controller bindings
On 1/10/19 10:31 AM, masonccy...@mxic.com.tw wrote: > Hi Marek, Hi, >> "Marek Vasut" >> 2019/01/08 下午 08:06 >> > diff --git a/Documentation/devicetree/bindings/spi/spi-renesas- >> rpc.txt b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt >> > new file mode 100644 >> > index 000..5f96532 >> > --- /dev/null >> > +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt >> > @@ -0,0 +1,37 @@ >> > +Renesas R-Car Gen3 RPC-IF controller Device Tree Bindings >> > +-- >> > + >> > +Required properties: >> > +- compatible: should be "renesas,r8a77995-rpc" >> > +- #address-cells: should be 1 >> > +- #size-cells: should be 0 >> > +- reg: should contain 2 entries, one for the registers and one >> for the direct >> > + mapping area >> > +- reg-names: should contain "regs" and "dirmap" >> > +- clock-names: should contain "rpc" >> > +- clocks: should contain 1 entries for the module's clock >> > +- renesas,rpc-mode: should contain "spi" for rpc spi mode or >> > + "hyperflash" for rpc hyperflash mode. >> >> Why do we need this property ? I believe it is possible to detect the >> configuration based on the type of child of the RPC node. If the driver >> was properly designed, it could well behave as either CFI NOR driver or >> SPI flash driver and all would be good, but it seems this is written >> with it being SPI flash driver only and once the HF mode would need to >> be added, it'd mean a tremendous undertaking to rework the entire driver. >> > > Except to check if there are any SPI NOR child nodes by "jedec,spi-nor" > compatible, is any other way better ? > > any suggestion is welcome. That's the one. A MFD RPC driver can sanitize it's child nodes, verify that the config is valid and configure the RPC accordingly. All of the devices that can be connected to the RPC are either valid jedec-nor or CFI NOR (HF), so this should work fine. -- Best regards, Marek Vasut
Re: [PATCH v5 1/2] spi: Add Renesas R-Car Gen3 RPC-IF SPI controller driver
On 1/9/19 8:04 PM, Sergei Shtylyov wrote: > On 01/09/2019 09:49 PM, Geert Uytterhoeven wrote: > >>>> Add a driver for Renesas R-Car Gen3 RPC-IF SPI controller. >>>> >>>> Signed-off-by: Mason Yang >>> >>>You now need to add: >>> >>> Signed-off-by: Sergei Shtylyov >> >>>> --- a/drivers/spi/Kconfig >>>> +++ b/drivers/spi/Kconfig >>>> @@ -544,6 +544,12 @@ config SPI_RSPI >>>> help >>>> SPI driver for Renesas RSPI and QSPI blocks. >>>> >>>> +config SPI_RENESAS_RPC_IF >>> >>>Since the driver is called without -IF suffix, I wouldn't use it in the >>> Kconfig name either, the following is enough: >>> >>>> + tristate "Renesas R-Car Gen3 RPC-IF SPI controller" >>>> + depends on ARCH_RENESAS || COMPILE_TEST >>> >>>Judging on the compilation error reported by the 0-day bot about readq(), >>> we now need to depend on 64BIT or something... >> >> IIRC, this hardware block is also used on RZ/A, which is 32-bit. > > I'm not seeing it in the "RZ/A1H Group, RZ/A1M Group User’s Manual: > Hardware" > Rev 3.00. What SoC did you have in mind? At least the GR Peach boots from this block, so that one :) -- Best regards, Marek Vasut
Re: [PATCH] clk: Fix a missing check on regmap_bulk_read
On 12/24/18 8:00 PM, Aditya Pakki wrote: > Currently, vc5_pll_recalc_rate() may produce incorrect output when > regmap_bulk_read() fails. The fix checks the return value of the > latter function and returns 0 in case of failure. > > Signed-off-by: Aditya Pakki > --- > drivers/clk/clk-versaclock5.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c > index decffb3826ec..cd76a893c594 100644 > --- a/drivers/clk/clk-versaclock5.c > +++ b/drivers/clk/clk-versaclock5.c > @@ -413,7 +413,8 @@ static unsigned long vc5_pll_recalc_rate(struct clk_hw > *hw, > u32 div_int, div_frc; > u8 fb[5]; > > - regmap_bulk_read(vc5->regmap, VC5_FEEDBACK_INT_DIV, fb, 5); > + if (regmap_bulk_read(vc5->regmap, VC5_FEEDBACK_INT_DIV, fb, 5)) > + return 0; Shouldn't this return ret on failure ? > div_int = (fb[0] << 4) | (fb[1] >> 4); > div_frc = (fb[2] << 16) | (fb[3] << 8) | fb[4]; > -- Best regards, Marek Vasut
Re: [PATCH v5 2/2] dt-bindings: spi: Document Renesas R-Car RPC-IF controller bindings
On 1/8/19 5:17 AM, Mason Yang wrote: > Document the bindings used by the Renesas R-Car Gen3 RPC-IF controller. > > Signed-off-by: Mason Yang > --- > .../devicetree/bindings/spi/spi-renesas-rpc.txt| 37 > ++ > 1 file changed, 37 insertions(+) > create mode 100644 Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt > > diff --git a/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt > b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt > new file mode 100644 > index 000..5f96532 > --- /dev/null > +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt > @@ -0,0 +1,37 @@ > +Renesas R-Car Gen3 RPC-IF controller Device Tree Bindings > +-- > + > +Required properties: > +- compatible: should be "renesas,r8a77995-rpc" > +- #address-cells: should be 1 > +- #size-cells: should be 0 > +- reg: should contain 2 entries, one for the registers and one for the direct > + mapping area > +- reg-names: should contain "regs" and "dirmap" > +- clock-names: should contain "rpc" > +- clocks: should contain 1 entries for the module's clock > +- renesas,rpc-mode: should contain "spi" for rpc spi mode or > +"hyperflash" for rpc hyperflash mode. Why do we need this property ? I believe it is possible to detect the configuration based on the type of child of the RPC node. If the driver was properly designed, it could well behave as either CFI NOR driver or SPI flash driver and all would be good, but it seems this is written with it being SPI flash driver only and once the HF mode would need to be added, it'd mean a tremendous undertaking to rework the entire driver. -- Best regards, Marek Vasut
Re: [PATCH v4 2/2] dt-bindings: spi: Document Renesas R-Car Gen3 RPC controller bindings
On 12/24/18 7:52 AM, Mason Yang wrote: > Document the bindings used by the Renesas R-Car Gen3 RPC controller. > > Signed-off-by: Mason Yang > --- > .../devicetree/bindings/spi/spi-renesas-rpc.txt| 37 > ++ > 1 file changed, 37 insertions(+) > create mode 100644 Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt > > diff --git a/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt > b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt > new file mode 100644 > index 000..ba863d7 > --- /dev/null > +++ b/Documentation/devicetree/bindings/spi/spi-renesas-rpc.txt > @@ -0,0 +1,37 @@ > +Renesas R-Car Gen3 RPC controller Device Tree Bindings > +-- > + > +Required properties: > +- compatible: should be "renesas,r8a77995-rpc" > +- #address-cells: should be 1 > +- #size-cells: should be 0 > +- reg: should contain 2 entries, one for the registers and one for the direct > + mapping area > +- reg-names: should contain "regs" and "dirmap" > +- clock-names: should contain "rpc" > +- clocks: should contain 1 entries for the module's clock > +- renesas,rpc-mode: should contain "spi" for rpc spi mode or > +"hyperflash" for rpc hyerflash mode. We do not need special property to identify that the controller is in HF mode, just attach CFI NOR node or JEDEC SPI NOR underneath it. -- Best regards, Marek Vasut
Re: [PATCH v4 0/2] spi: Add Renesas R-Car Gen3 RPC SPI driver
On 12/24/18 7:52 AM, Mason Yang wrote: > Hi Mark, > > This Renesas R-Car Gen3 RPC SPI driver is based on Boris's new > spi-mem direct mapping read/write mode [1][2]. Again, the RPC is NOT a SPI controller, it is dual SPI/HF controller. [...] -- Best regards, Marek Vasut
Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again"
On 12/21/2018 10:08 PM, Paul Burton wrote:
> Hi Marek,
>
> On Fri, Dec 21, 2018 at 09:08:28PM +0100, Marek Vasut wrote:
>> On 12/16/2018 11:28 PM, Paul Burton wrote:
>>> On Sun, Dec 16, 2018 at 11:01:18PM +0100, Marek Vasut wrote:
>>>>>>> I did suggest an alternative approach which would rename
>>>>>>> serial8250_clear_fifos() and split it into 2 variants - one that
>>>>>>> disables FIFOs & one that does not, then use the latter in
>>>>>>> __do_stop_tx_rs485():
>>>>>>>
>>>>>>> https://lore.kernel.org/lkml/20181213014805.77u5dzydo23cm6fq@pburton-laptop/
>>>>>>>
>>>>>>> However I have no access to the OMAP3 hardware that Marek's patch was
>>>>>>> attempting to fix & have heard nothing back with regards to him testing
>>>>>>> that approach, so here's a simple revert that fixes the Ingenic JZ4780.
>>>>>>>
>>>>>>> I've marked for stable back to v4.10 presuming that this is how far the
>>>>>>> broken patch may be backported, given that this is where commit
>>>>>>> 2bed8a8e7072 ("Clearing FIFOs in RS485 emulation mode causes subsequent
>>>>>>> transmits to break") that it tried to fix was introduced.
>>>>>>
>>>>>> OK, I tested this on AM335x / OMAP3 and the system is again broken, so
>>>>>> that's a NAK.
>>>>>
>>>>> To be clear - what did you test? This revert or the patch linked to
>>>>> above?
>>>>>
>>>>> This revert would of course reintroduce your RS485 issue because it just
>>>>> undoes your change.
>>>>
>>>> The revert. Which of the two patches do you need me to test.
>>>
>>> The one in the email I sent on Thursday 13th at 01:48:06 UTC, linked to
>>> at lore.kernel.org in the quote right above:
>>>
>>> https://lore.kernel.org/lkml/20181213014805.77u5dzydo23cm6fq@pburton-laptop/
>>>
>>> You replied with comments on the patch, you just never tested it or
>>> never told me if you did. The lack of response means I don't know
>>> whether that potential patch even still works for your system, hence the
>>> revert.
>>
>> I shared the entire testcase, which now fails on AM335x due to this
>> revert. Is there any progress on a proper fix from your side which does
>> not break the AM335x ?
>
> No.
>
> Let's be clear - I didn't break your AM335x system, your broken patch
> says that Daniel did with a commit applied back in v4.10. As such I
> don't consider it my responsibility to fix your problem at all, I don't
> have any access to the hardware anyway & I won't be buying hardware to
> fix a bug for you.
>
> Despite all that I did write a patch which I expect would have solved
> the problem for both of us, which is linked *twice* in the quoted emails
> above & which as far as I can tell you *still* have not tested. I can
> only surmise that you're trying deliberately to be annoying at this
> point.
>
> If you want to try the patch I already wrote, and confirm whether it
> actually works for you, then let's talk. Otherwise we're done here.
Understood. However I did test your patch, but it was generating
spurious IRQs and overruns (ttyS ttyS2: 91 input overrun(s)) on the
AM335x, so that's not a proper solution.
I even brought the CI20 V2 I have back to life (yes, I bought one). The
patch Ezequiel posted did fix the problem on the CI20, and did not break
the AM335x, so I'd prefer if it was revisited instead of a heavy-handed
revert.
And I'd prefer to keep this thread alive, since I am still convinced
that the FIFO handling code is wrong. Moreover, considering the UME bit
on JZ4780 in FCR, the original code should confuse the UART on the
JZ4780 too, although this might be hidden by some other surrounding code
specific to the 8250 core on the JZ4780.
I am also on mipslinux IRC channel, in case you want to discuss this.
--
Best regards,
Marek Vasut
Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again"
On 12/16/2018 11:28 PM, Paul Burton wrote:
> Hi Marek,
>
> On Sun, Dec 16, 2018 at 11:01:18PM +0100, Marek Vasut wrote:
>>>>> I did suggest an alternative approach which would rename
>>>>> serial8250_clear_fifos() and split it into 2 variants - one that
>>>>> disables FIFOs & one that does not, then use the latter in
>>>>> __do_stop_tx_rs485():
>>>>>
>>>>> https://lore.kernel.org/lkml/20181213014805.77u5dzydo23cm6fq@pburton-laptop/
>>>>>
>>>>> However I have no access to the OMAP3 hardware that Marek's patch was
>>>>> attempting to fix & have heard nothing back with regards to him testing
>>>>> that approach, so here's a simple revert that fixes the Ingenic JZ4780.
>>>>>
>>>>> I've marked for stable back to v4.10 presuming that this is how far the
>>>>> broken patch may be backported, given that this is where commit
>>>>> 2bed8a8e7072 ("Clearing FIFOs in RS485 emulation mode causes subsequent
>>>>> transmits to break") that it tried to fix was introduced.
>>>>
>>>> OK, I tested this on AM335x / OMAP3 and the system is again broken, so
>>>> that's a NAK.
>>>
>>> To be clear - what did you test? This revert or the patch linked to
>>> above?
>>>
>>> This revert would of course reintroduce your RS485 issue because it just
>>> undoes your change.
>>
>> The revert. Which of the two patches do you need me to test.
>
> The one in the email I sent on Thursday 13th at 01:48:06 UTC, linked to
> at lore.kernel.org in the quote right above:
>
> https://lore.kernel.org/lkml/20181213014805.77u5dzydo23cm6fq@pburton-laptop/
>
> You replied with comments on the patch, you just never tested it or
> never told me if you did. The lack of response means I don't know
> whether that potential patch even still works for your system, hence the
> revert.
I shared the entire testcase, which now fails on AM335x due to this
revert. Is there any progress on a proper fix from your side which does
not break the AM335x ?
--
Best regards,
Marek Vasut
Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again"
On 12/17/2018 06:20 PM, Marek Vasut wrote:
> On 12/17/2018 05:30 PM, Ezequiel Garcia wrote:
>> On Sun, 16 Dec 2018 at 19:35, Paul Burton wrote:
>>>
>>> Hi Ezequiel,
>>>
>>> On Sun, Dec 16, 2018 at 07:28:22PM -0300, Ezequiel Garcia wrote:
>>>> On Sun, 16 Dec 2018 at 19:24, Paul Burton wrote:
>>>>> This helps, but it only addresses one part of one of the 4 reasons I
>>>>> listed as motivation for my revert. For example serial8250_do_shutdown()
>>>>> also clearly intends to disable the FIFOs.
>>>>>
>>>>
>>>> OK. So, let's fix that :-)
>>>
>>> I already did, or at least tried to, on Thursday [1].
>>>
>>>> By all means, it would be really nice to push forward and fix the garbage
>>>> issue on JZ4780, as well as the transmission issue on AM335x.
>>>>
>>>> AM335x is a wildly popular platform, and it's not funny to break it.
>>>
>>> Well, clearly not if it was broken in v4.10 & only just fixed..? And
>>> from Marek's commit message the patch in v4.10 doesn't break the whole
>>> system just RS485.
>>>
>>
>> Careful here. It's naive to consider v4.10 as old in this context.
>>
>> AM335x is used in hundreds of thousands of products, probably
>> "industrial" products.
>> Manufacturers don't always follow the kernel, and it's entirely
>> likely that they notice a regression only when developing a new product,
>> or when rebasing on top of a newer longterm kernel.
>>
>> Then again, I don't have any details about what is Marek's original fix
>> actually fixing.
>>
>> Marek: could you please post the test case that you used to validate your
>> fix?
>> I can't find that anywhere.
>
> I can't share the testcase itself because it has no license and I didn't
> write it, but I can tell you what it's doing. (I'll check if I can share
> the testcase verbatim too, I just sent an email to the author)
>
> The test spawns two threads, one sending , one receiving. The sending
> thread sends 8 bytes of data from /dev/ttyS4 , the receiving thread
> receives the data on /dev/ttyS2 and compares the pattern. The port
> settings is B100 8N1 with rs485conf.flags set to SER_RS485_ENABLED
> and SER_RS485_RTS_AFTER_SEND. Sometimes the received data do not match
> the sent data, but rather look as if one character was left over from
> the previous transmission in the FIFO.
>
> Those two UARTs are connected together by two wires, without any real
> RS485 hardware to minimize the hardware complexity (it's easy to
> implement that on the pocketbeagle, which is the cheap option here).
Test code is below . On the pocketbeagle, connect SPI0:CLK with U4:TX
and SPI0:MISO with U4:RX , apply the DT patch below and run the example.
It'll fail after a few iterations.
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
std::atomic running{true};
int set_interface_attribs (int fd, int speed, int parity)
{
struct termios tty;
memset (, 0, sizeof tty);
if (tcgetattr (fd, ) != 0)
{
std::cerr << "Error from tcgetattr" << std::endl;
return -1;
}
cfsetospeed (, speed);
cfsetispeed (, speed);
tty.c_cflag = (tty.c_cflag & ~CSIZE) | CS8;
tty.c_iflag &= ~IGNBRK;
tty.c_lflag = 0;
tty.c_oflag = 0;
tty.c_cc[VMIN] = 8;
tty.c_cc[VTIME] = 0;
tty.c_iflag &= ~(IXON | IXOFF | IXANY);
tty.c_cflag |= (CLOCAL | CREAD);
tty.c_cflag &= ~(PARENB | PARODD);
tty.c_cflag |= parity;
tty.c_cflag &= ~CSTOPB;
tty.c_cflag &= ~CRTSCTS;
if (tcsetattr (fd, TCSANOW, ) != 0)
{
std::cerr << "Error from tcsetattr" << std::endl;
return -1;
}
return 0;
}
void enable_rts(int fd, int rts)
{
struct serial_rs485 rs485conf;
if (rts) {
rs485conf.flags = SER_RS485_ENABLED ;
rs485conf.flags |= SER_RS485_RTS_AFTER_SEND;
rs485conf.flags &= ~(SER_RS485_RTS_ON_SEND);
rs485conf.delay_rts_before_send = 0;
rs485conf.delay_rts_after_send = 0;
} else {
rs485conf.flags = 0 ;
}
if (ioctl( fd, TIOCSRS485, ) < 0){
std::cerr << "Cannot set device in RS485 mode" << std::endl;
}
}
void output_function(int fd)
{
while(running) {
write (fd, "asdfghjk", 8);
usleep (2);
}
}
void input_function(int fd)
{
char buf [100];
size_t count=0;
st
Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again"
On 12/17/2018 05:30 PM, Ezequiel Garcia wrote: > On Sun, 16 Dec 2018 at 19:35, Paul Burton wrote: >> >> Hi Ezequiel, >> >> On Sun, Dec 16, 2018 at 07:28:22PM -0300, Ezequiel Garcia wrote: >>> On Sun, 16 Dec 2018 at 19:24, Paul Burton wrote: >>>> This helps, but it only addresses one part of one of the 4 reasons I >>>> listed as motivation for my revert. For example serial8250_do_shutdown() >>>> also clearly intends to disable the FIFOs. >>>> >>> >>> OK. So, let's fix that :-) >> >> I already did, or at least tried to, on Thursday [1]. >> >>> By all means, it would be really nice to push forward and fix the garbage >>> issue on JZ4780, as well as the transmission issue on AM335x. >>> >>> AM335x is a wildly popular platform, and it's not funny to break it. >> >> Well, clearly not if it was broken in v4.10 & only just fixed..? And >> from Marek's commit message the patch in v4.10 doesn't break the whole >> system just RS485. >> > > Careful here. It's naive to consider v4.10 as old in this context. > > AM335x is used in hundreds of thousands of products, probably > "industrial" products. > Manufacturers don't always follow the kernel, and it's entirely > likely that they notice a regression only when developing a new product, > or when rebasing on top of a newer longterm kernel. > > Then again, I don't have any details about what is Marek's original fix > actually fixing. > > Marek: could you please post the test case that you used to validate your fix? > I can't find that anywhere. I can't share the testcase itself because it has no license and I didn't write it, but I can tell you what it's doing. (I'll check if I can share the testcase verbatim too, I just sent an email to the author) The test spawns two threads, one sending , one receiving. The sending thread sends 8 bytes of data from /dev/ttyS4 , the receiving thread receives the data on /dev/ttyS2 and compares the pattern. The port settings is B100 8N1 with rs485conf.flags set to SER_RS485_ENABLED and SER_RS485_RTS_AFTER_SEND. Sometimes the received data do not match the sent data, but rather look as if one character was left over from the previous transmission in the FIFO. Those two UARTs are connected together by two wires, without any real RS485 hardware to minimize the hardware complexity (it's easy to implement that on the pocketbeagle, which is the cheap option here). -- Best regards, Marek Vasut
Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again"
On 12/17/2018 04:18 PM, Greg Kroah-Hartman wrote: > On Sun, Dec 16, 2018 at 10:35:12PM +, Paul Burton wrote: >> Hi Ezequiel, >> >> On Sun, Dec 16, 2018 at 07:28:22PM -0300, Ezequiel Garcia wrote: >>> On Sun, 16 Dec 2018 at 19:24, Paul Burton wrote: >>>> This helps, but it only addresses one part of one of the 4 reasons I >>>> listed as motivation for my revert. For example serial8250_do_shutdown() >>>> also clearly intends to disable the FIFOs. >>>> >>> >>> OK. So, let's fix that :-) >> >> I already did, or at least tried to, on Thursday [1]. >> >>> By all means, it would be really nice to push forward and fix the garbage >>> issue on JZ4780, as well as the transmission issue on AM335x. >>> >>> AM335x is a wildly popular platform, and it's not funny to break it. >> >> Well, clearly not if it was broken in v4.10 & only just fixed..? And >> from Marek's commit message the patch in v4.10 doesn't break the whole >> system just RS485. >> >>> So, let's please stop discussing which board we'll break and just fix both. >> >> I completely agree that would be ideal and I wrote a patch hoping to do >> that on Thursday, but didn't get any response on testing. It's late in >> the cycle hence a revert made sense. Simple as that. > > A revert makes sense now, I'll go queue this up, thanks. I don't like this for multiple reasons. 1) There is a better patch posted which doesn't break the AM335x and clearly identifies and fixes the problem on the JZ4780 / CI20 2) The JZ4780 8250 core is not a standard 8250 core, since it has extra bits in the FCR register (like the UME bit, which disables the whole UART block), so the revert IMO would break that core too, it just hides the breakage. I'm still trying to understand the implications of that in detail, but the discussion wasn't quite constructive. I'd much rather see Ezequiel's patch applied, since that's far less destructive approach to fixing the problem than the revert. -- Best regards, Marek Vasut
Re: [PATCH v3 1/2] spi: Add Renesas R-Car Gen3 RPC SPI controller driver
On 12/17/2018 08:42 AM, masonccy...@mxic.com.tw wrote:
> Hi Sergei,
>
>
>> > +static int rpc_spi_io_xfer(struct rpc_spi *rpc,
>> > + const void *tx_buf, void *rx_buf)
>> > +{
>> > + u32 smenr, smcr, data, pos = 0;
>> > + int ret = 0;
>> > +
>> > + regmap_write(rpc->regmap, RPC_CMNCR, RPC_CMNCR_MD | RPC_CMNCR_SFDE |
>> > + RPC_CMNCR_MOIIO_HIZ | RPC_CMNCR_IOFV_HIZ |
>> > + RPC_CMNCR_BSZ(0));
>> > + regmap_write(rpc->regmap, RPC_SMDRENR, 0x0);
>> > + regmap_write(rpc->regmap, RPC_SMCMR, rpc->cmd);
>> > + regmap_write(rpc->regmap, RPC_SMDMCR, rpc->dummy);
>> > + regmap_write(rpc->regmap, RPC_SMADR, rpc->addr);
>> > +
>> > + if (tx_buf) {
>> > + smenr = rpc->smenr;
>> > +
>> > + while (pos < rpc->xferlen) {
>> > + u32 nbytes = rpc->xferlen - pos;
>> > +
>> > + regmap_write(rpc->regmap, RPC_SMWDR0,
>> > + get_unaligned((u32 *)(tx_buf + pos)));
>> > +
>> > + if (nbytes > 4) {
>> > + nbytes = 4;
>> > + smcr = rpc->smcr |
>> > + RPC_SMCR_SPIE | RPC_SMCR_SSLKP;
>> > + } else {
>> > + smcr = rpc->smcr | RPC_SMCR_SPIE;
>> > + }
>> > +
>> > + regmap_write(rpc->regmap, RPC_SMENR, smenr);
>> > + regmap_write(rpc->regmap, RPC_SMCR, smcr);
>> > + ret = wait_msg_xfer_end(rpc);
>> > + if (ret)
>> > + goto out;
>> > +
>> > + pos += nbytes;
>> > + smenr = rpc->smenr & ~RPC_SMENR_CDE &
>> > + ~RPC_SMENR_ADE(0xf);
>> > + }
>> > + } else if (rx_buf) {
>> > + while (pos < rpc->xferlen) {
>> > + u32 nbytes = rpc->xferlen - pos;
>> > +
>> > + if (nbytes > 4)
>> > + nbytes = 4;
>> > +
>> > + regmap_write(rpc->regmap, RPC_SMENR, rpc->smenr);
>> > + regmap_write(rpc->regmap, RPC_SMCR,
>> > + rpc->smcr | RPC_SMCR_SPIE);
>>
>> Hm... our flash chip (Spansion S25FS512S) doesn't get detected; it
> sends
>> JEDEC ID bytes 0..3 repeatedly, unless I copy the SSLKP logic from the
> writing
>> branch above...
>
> Do you switch the SW1, SW2, SW3, SW13, SW31 and SW10 to on-board QSPI
> mode ?
> Because R-Car D3 Draak board default is booting from HyperFlsah.
So this puts us back to the original discussion -- the driver should
support HF mode as well IMO.
> what follows is my booting log, FYI.
> --
> [ 1.625053] m25p80 spi5.0: s25fl129p1 (16384 Kbytes)
> [ 1.634391] 12 fixed-partitions partitions found on MTD device spi5.0
> [ 1.642198] Creating 12 MTD partitions on "spi5.0":
> [ 1.647598] 0x-0x0004 : "Bank 1 - Boot parameter"
> [ 1.660893] 0x0004-0x0018 : "Bank 1 - Loader-BL2"
> [ 1.671287] 0x0018-0x001c : "Bank 1 - Certification"
> ---
>
>>
>> > + ret = wait_msg_xfer_end(rpc);
>> > + if (ret)
>> > + goto out;
>> > +
>> > + regmap_read(rpc->regmap, RPC_SMRDR0, );
>> > + memcpy_fromio(rx_buf + pos, (void *), nbytes);
>> > + pos += nbytes;
>>
>> ... and it skips byte 4 unless I copy the code from the end of the
> writing
>> branch, clearing CDE/ADE. But even then the byte 4 reads as 0x03
> instead of 0.
>
> yup, I think this is some kind of RPC HW limitation,
> in RPC manual I/O mode, it only could read 4 bytes data w/ one command.
>
> That is, one command + read 4 bytes data + read 4 bytes data + read 4
> bytes data + ...
> will get the incorrect data.
>
> That's why RPC in manual I/O mode, driver only could do,
> one command + read 4 bytes data; one command + read 4 bytes data and so on.
>
> But RPC in external address space read mode(here we call it direct
> mapping read mode)
> is ok for one command + read 4 bytes data + read 4 bytes data +
I think the U-Boot driver solves those problems, since it works in both
RPC and HF mode on all of Gen3 boards , not just D3 in non-standard SPI
boot configuration. Please take a look.
--
Best regards,
Marek Vasut
Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again"
On 12/16/2018 10:52 PM, Ezequiel Garcia wrote: > On Sun, 16 Dec 2018 at 18:45, Marek Vasut wrote: > [skips discussion] >> >>> Ultimately it's Greg's decision but it sounds like you're asking me to >>> say it's OK to break the JZ4780 in a stable kernel with a patch that I >>> think would be risky anyway, and I won't do that. >> >> I am saying this revert breaks AM335x, so this is a stalemate. I had a >> discussion with Ezequiel (on CC) and he seems to have a different >> smaller patch coming for this problem. >> > > Can you guys test this? Note that serial8250_do_startup has a comment > stating clearly that it has the intention of disabling the FIFOs, > so it seems this is the right thing to do. > > Paul, this removes the garbage on my CI20 (rev.1) > > diff --git a/drivers/tty/serial/8250/8250_port.c > b/drivers/tty/serial/8250/8250_port.c > index c39482b96111..fac19cbc51d1 100644 > --- a/drivers/tty/serial/8250/8250_port.c > +++ b/drivers/tty/serial/8250/8250_port.c > @@ -2209,10 +2209,11 @@ int serial8250_do_startup(struct uart_port *port) > /* > * Clear the FIFO buffers and disable them. > * (they will be reenabled in set_termios()) > */ > serial8250_clear_fifos(up); > + serial_out(up, UART_FCR, 0); > > /* > * Clear the interrupt registers. > */ > serial_port_in(port, UART_LSR); > > On AM335x pocketbeagle Tested-by: Marek Vasut -- Best regards, Marek Vasut
Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again"
On 12/16/2018 10:39 PM, Paul Burton wrote:
> Hi Marek,
Hi,
> On Sun, Dec 16, 2018 at 10:08:48PM +0100, Marek Vasut wrote:
>>> I did suggest an alternative approach which would rename
>>> serial8250_clear_fifos() and split it into 2 variants - one that
>>> disables FIFOs & one that does not, then use the latter in
>>> __do_stop_tx_rs485():
>>>
>>> https://lore.kernel.org/lkml/20181213014805.77u5dzydo23cm6fq@pburton-laptop/
>>>
>>> However I have no access to the OMAP3 hardware that Marek's patch was
>>> attempting to fix & have heard nothing back with regards to him testing
>>> that approach, so here's a simple revert that fixes the Ingenic JZ4780.
>>>
>>> I've marked for stable back to v4.10 presuming that this is how far the
>>> broken patch may be backported, given that this is where commit
>>> 2bed8a8e7072 ("Clearing FIFOs in RS485 emulation mode causes subsequent
>>> transmits to break") that it tried to fix was introduced.
>>
>> OK, I tested this on AM335x / OMAP3 and the system is again broken, so
>> that's a NAK.
>
> To be clear - what did you test? This revert or the patch linked to
> above?
>
> This revert would of course reintroduce your RS485 issue because it just
> undoes your change.
The revert. Which of the two patches do you need me to test.
> Either way, commit f6aa5beb45be ("serial: 8250: Fix clearing FIFOs in
> RS485 mode again") breaks systems that worked before it so at this late
> stage in the 4.20 cycle a revert would make sense to me. If that breaks
> RS85 on OMAP3 then my question would be how much can anyone really care
> if nobody noticed since v4.10? And why should that lead to you breaking
> the JZ4780 which has been discovered before a stable kernel release
> includes the breakage?
There's always a .y release where this can be properly investigated and
solved, instead of breaking one platform or the other.
Then again, see the patch from Ezequiel that was just posted, I think it
might be a far better solution.
--
Best regards,
Marek Vasut
Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again"
On 12/16/2018 10:31 PM, Paul Burton wrote: > Hi Marek, Hi, > On Sun, Dec 16, 2018 at 09:32:19PM +0100, Marek Vasut wrote: >> I am unable to test it on such a short notice as I'm currently ill, so I >> cannot tell if your change breaks the OMAP3/AM335x boards or not. Given >> that there are very few CI20 boards in use, I'd like to ask you for some >> extra time to investigate this on the OMAP3 too. > > I'm sorry to hear that you're ill, but your patch is getting awfully > close to becoming part of a stable kernel release & it causes > regressions. Even if it didn't break a board I use, I think the patch > would be broken & risky for the reasons I outlined in my revert's commit > message. That's what the incremental releases are for, so that minor problems can get fixed there. Sure, it's great to have things perfect in the first release, but if that breaks other systems, too bad. > Ultimately it's Greg's decision but it sounds like you're asking me to > say it's OK to break the JZ4780 in a stable kernel with a patch that I > think would be risky anyway, and I won't do that. I am saying this revert breaks AM335x, so this is a stalemate. I had a discussion with Ezequiel (on CC) and he seems to have a different smaller patch coming for this problem. [...] -- Best regards, Marek Vasut
Re: [PATCH] Revert "serial: 8250: Fix clearing FIFOs in RS485 mode again"
On 12/16/2018 09:10 PM, Paul Burton wrote:
> Commit f6aa5beb45be ("serial: 8250: Fix clearing FIFOs in RS485 mode
> again") makes a change to FIFO clearing code which its commit message
> suggests was intended to be specific to use with RS485 mode, however:
>
> 1) The change made does not just affect __do_stop_tx_rs485(), it also
> affects other uses of serial8250_clear_fifos() including paths for
> starting up, shutting down or auto-configuring a port regardless of
> whether it's an RS485 port or not.
>
> 2) It makes the assumption that resetting the FIFOs is a no-op when
> FIFOs are disabled, and as such it checks for this case & explicitly
> avoids setting the FIFO reset bits when the FIFO enable bit is
> clear. A reading of the PC16550D manual would suggest that this is
> OK since the FIFO should automatically be reset if it is later
> enabled, but we support many 16550-compatible devices and have never
> required this auto-reset behaviour for at least the whole git era.
> Starting to rely on it now seems risky, offers no benefit, and
> indeed breaks at least the Ingenic JZ4780's UARTs which reads
> garbage when the RX FIFO is enabled if we don't explicitly reset it.
>
> 3) By only resetting the FIFOs if they're enabled, the behaviour of
> serial8250_do_startup() during boot now depends on what the value of
> FCR is before the 8250 driver is probed. This in itself seems
> questionable and leaves us with FCR=0 & no FIFO reset if the UART
> was used by 8250_early, otherwise it depends upon what the
> bootloader left behind.
>
> 4) Although the naming of serial8250_clear_fifos() may be unclear, it
> is clear that callers of it expect that it will disable FIFOs. Both
> serial8250_do_startup() & serial8250_do_shutdown() contain comments
> to that effect, and other callers explicitly re-enable the FIFOs
> after calling serial8250_clear_fifos(). The premise of that patch
> that disabling the FIFOs is incorrect therefore seems wrong.
>
> For these reasons, this reverts commit f6aa5beb45be ("serial: 8250: Fix
> clearing FIFOs in RS485 mode again").
>
> Signed-off-by: Paul Burton
> Fixes: f6aa5beb45be ("serial: 8250: Fix clearing FIFOs in RS485 mode again").
> Cc: Greg Kroah-Hartman
> Cc: Daniel Jedrychowski
> Cc: Marek Vasut
> Cc: linux-m...@vger.kernel.org
> Cc: linux-ser...@vger.kernel.org
> Cc: stable # 4.10+
> ---
> I did suggest an alternative approach which would rename
> serial8250_clear_fifos() and split it into 2 variants - one that
> disables FIFOs & one that does not, then use the latter in
> __do_stop_tx_rs485():
>
> https://lore.kernel.org/lkml/20181213014805.77u5dzydo23cm6fq@pburton-laptop/
>
> However I have no access to the OMAP3 hardware that Marek's patch was
> attempting to fix & have heard nothing back with regards to him testing
> that approach, so here's a simple revert that fixes the Ingenic JZ4780.
>
> I've marked for stable back to v4.10 presuming that this is how far the
> broken patch may be backported, given that this is where commit
> 2bed8a8e7072 ("Clearing FIFOs in RS485 emulation mode causes subsequent
> transmits to break") that it tried to fix was introduced.
OK, I tested this on AM335x / OMAP3 and the system is again broken, so
that's a NAK.
> ---
> drivers/tty/serial/8250/8250_port.c | 29 +
> 1 file changed, 5 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_port.c
> b/drivers/tty/serial/8250/8250_port.c
> index f776b3eafb96..3f779d25ec0c 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -552,30 +552,11 @@ static unsigned int serial_icr_read(struct
> uart_8250_port *up, int offset)
> */
> static void serial8250_clear_fifos(struct uart_8250_port *p)
> {
> - unsigned char fcr;
> - unsigned char clr_mask = UART_FCR_CLEAR_RCVR | UART_FCR_CLEAR_XMIT;
> -
> if (p->capabilities & UART_CAP_FIFO) {
> - /*
> - * Make sure to avoid changing FCR[7:3] and ENABLE_FIFO bits.
> - * In case ENABLE_FIFO is not set, there is nothing to flush
> - * so just return. Furthermore, on certain implementations of
> - * the 8250 core, the FCR[7:3] bits may only be changed under
> - * specific conditions and changing them if those conditions
> - * are not met can have nasty side effects. One such core is
> - * the 8250-omap present in TI AM335x.
> - */
> - fcr = serial_in(p, UART_FCR);
> -
> - /
