* [PATCH 2/3] Revert "usb: usb251xb: Add US port lanes inversion property"
2019-07-19 8:44 [PATCH 1/3] Revert "usb: usb251xb: Add US lanes inversion dts-bindings" Lucas Stach
@ 2019-07-19 8:44 ` Lucas Stach
2019-07-19 8:44 ` [PATCH 3/3] usb: usb251xb: Reallow swap-dx-lanes to apply to the upstream port Lucas Stach
2019-07-19 10:13 ` [PATCH 1/3] Revert "usb: usb251xb: Add US lanes inversion dts-bindings" Serge Semin
2 siblings, 0 replies; 5+ messages in thread
From: Lucas Stach @ 2019-07-19 8:44 UTC (permalink / raw)
To: Richard Leitner, Greg Kroah-Hartman, Rob Herring
Cc: Mark Rutland, Serge Semin, linux-usb, devicetree, kernel, patchwork-lst
This property isn't needed and not yet used anywhere. The swap-dx-lanes
property is perfectly fine for doing the swap on the upstream port
lanes.
CC: stable@vger.kernel.org #5.2
Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
drivers/usb/misc/usb251xb.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 4d6ae3795a88..119aeb658c81 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -574,8 +574,6 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
hub->port_swap = USB251XB_DEF_PORT_SWAP;
usb251xb_get_ports_field(hub, "swap-dx-lanes", data->port_cnt,
&hub->port_swap);
- if (of_get_property(np, "swap-us-lanes", NULL))
- hub->port_swap |= BIT(0);
/* The following parameters are currently not exposed to devicetree, but
* may be as soon as needed.
--
2.20.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 3/3] usb: usb251xb: Reallow swap-dx-lanes to apply to the upstream port
2019-07-19 8:44 [PATCH 1/3] Revert "usb: usb251xb: Add US lanes inversion dts-bindings" Lucas Stach
2019-07-19 8:44 ` [PATCH 2/3] Revert "usb: usb251xb: Add US port lanes inversion property" Lucas Stach
@ 2019-07-19 8:44 ` Lucas Stach
2019-07-19 10:13 ` [PATCH 1/3] Revert "usb: usb251xb: Add US lanes inversion dts-bindings" Serge Semin
2 siblings, 0 replies; 5+ messages in thread
From: Lucas Stach @ 2019-07-19 8:44 UTC (permalink / raw)
To: Richard Leitner, Greg Kroah-Hartman, Rob Herring
Cc: Mark Rutland, Serge Semin, linux-usb, devicetree, kernel, patchwork-lst
This is a partial revert of 73d31def1aab "usb: usb251xb: Create a ports
field collector method", which broke a existing devicetree
(arch/arm64/boot/dts/freescale/imx8mq.dtsi).
There is no reason why the swap-dx-lanes property should not apply to
the upstream port. The reason given in the breaking commit was that it's
inconsitent with respect to other port properties, but in fact it is not.
All other properties which only apply to the downstream ports explicitly
reject port 0, so there is pretty strong precedence that the driver
referred to the upstream port as port 0. So there is no inconsistency in
this property at all, other than the swapping being also applicable to
the upstream port.
CC: stable@vger.kernel.org #5.2
Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
drivers/usb/misc/usb251xb.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/misc/usb251xb.c b/drivers/usb/misc/usb251xb.c
index 119aeb658c81..6ca9111d150a 100644
--- a/drivers/usb/misc/usb251xb.c
+++ b/drivers/usb/misc/usb251xb.c
@@ -375,7 +375,8 @@ static int usb251xb_connect(struct usb251xb *hub)
#ifdef CONFIG_OF
static void usb251xb_get_ports_field(struct usb251xb *hub,
- const char *prop_name, u8 port_cnt, u8 *fld)
+ const char *prop_name, u8 port_cnt,
+ bool ds_only, u8 *fld)
{
struct device *dev = hub->dev;
struct property *prop;
@@ -383,7 +384,7 @@ static void usb251xb_get_ports_field(struct usb251xb *hub,
u32 port;
of_property_for_each_u32(dev->of_node, prop_name, prop, p, port) {
- if ((port >= 1) && (port <= port_cnt))
+ if ((port >= ds_only ? 1 : 0) && (port <= port_cnt))
*fld |= BIT(port);
else
dev_warn(dev, "port %u doesn't exist\n", port);
@@ -501,15 +502,15 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
hub->non_rem_dev = USB251XB_DEF_NON_REMOVABLE_DEVICES;
usb251xb_get_ports_field(hub, "non-removable-ports", data->port_cnt,
- &hub->non_rem_dev);
+ true, &hub->non_rem_dev);
hub->port_disable_sp = USB251XB_DEF_PORT_DISABLE_SELF;
usb251xb_get_ports_field(hub, "sp-disabled-ports", data->port_cnt,
- &hub->port_disable_sp);
+ true, &hub->port_disable_sp);
hub->port_disable_bp = USB251XB_DEF_PORT_DISABLE_BUS;
usb251xb_get_ports_field(hub, "bp-disabled-ports", data->port_cnt,
- &hub->port_disable_bp);
+ true, &hub->port_disable_bp);
hub->max_power_sp = USB251XB_DEF_MAX_POWER_SELF;
if (!of_property_read_u32(np, "sp-max-total-current-microamp",
@@ -573,7 +574,7 @@ static int usb251xb_get_ofdata(struct usb251xb *hub,
*/
hub->port_swap = USB251XB_DEF_PORT_SWAP;
usb251xb_get_ports_field(hub, "swap-dx-lanes", data->port_cnt,
- &hub->port_swap);
+ false, &hub->port_swap);
/* The following parameters are currently not exposed to devicetree, but
* may be as soon as needed.
--
2.20.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/3] Revert "usb: usb251xb: Add US lanes inversion dts-bindings"
2019-07-19 8:44 [PATCH 1/3] Revert "usb: usb251xb: Add US lanes inversion dts-bindings" Lucas Stach
2019-07-19 8:44 ` [PATCH 2/3] Revert "usb: usb251xb: Add US port lanes inversion property" Lucas Stach
2019-07-19 8:44 ` [PATCH 3/3] usb: usb251xb: Reallow swap-dx-lanes to apply to the upstream port Lucas Stach
@ 2019-07-19 10:13 ` Serge Semin
2019-07-22 7:46 ` Marco Felsch
2 siblings, 1 reply; 5+ messages in thread
From: Serge Semin @ 2019-07-19 10:13 UTC (permalink / raw)
To: Lucas Stach
Cc: Richard Leitner, Greg Kroah-Hartman, Rob Herring, Mark Rutland,
linux-usb, devicetree, kernel, patchwork-lst
Hello Lucas
On Fri, Jul 19, 2019 at 10:44:05AM +0200, Lucas Stach wrote:
> This reverts commit 3342ce35a1, as there is no need for this separate
> property and it breaks compatibility with existing devicetree files
> (arch/arm64/boot/dts/freescale/imx8mq.dtsi).
>
Hmm, didn't know there had been anything staged to merge and touching this
property before submitting the update. We must have done it nearly at the same
time, or your patch hasn't been merged at the time I prepared mine.
Anyway why would you prefer to change the interface again instead of
following the existing way? Firstly It is much easier to fix the dts-file
than to revert the interface back and break dts-files of possible other users.
Secondly the chip documentation doesn't have anything regarding port 0.
It states to swap the Ports from 1 to 4 (usb2514) corresponding to the bits
1 - 4 of the 'PORT SWAP' register, while bit 0 is connected with explicitly
named Upstream Port (without any numbering). Thirdly having a separate
property for US port makes the driver bindings interface a bit better
readable/logical, since in current implementation there is no implicit/unspoken/hidden
rule that port 0 corresponds to the Upstream Port, Port 0 just doesn't exists
(following the chip datasheet text), and the other port-related properties are
only applicable for downstream ports. So the driver code rejects them being
utilized for a port with 0 identifier. The only port-related setting being
exposed by the interface is the swap-port-one and it has a separately bound
property 'swap-us-lanes' for the Upstream port.
As for me, all of this makes more sense than having an implicit Port 0 - Upstream
port binding (as you suggest). Although the final decision of which solution is
better is after the subsystem maintainer after all.
Regards,
-Sergey
> CC: stable@vger.kernel.org #5.2
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
> Documentation/devicetree/bindings/usb/usb251xb.txt | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/usb251xb.txt b/Documentation/devicetree/bindings/usb/usb251xb.txt
> index bc7945e9dbfe..17915f64b8ee 100644
> --- a/Documentation/devicetree/bindings/usb/usb251xb.txt
> +++ b/Documentation/devicetree/bindings/usb/usb251xb.txt
> @@ -64,10 +64,8 @@ Optional properties :
> - power-on-time-ms : Specifies the time it takes from the time the host
> initiates the power-on sequence to a port until the port has adequate
> power. The value is given in ms in a 0 - 510 range (default is 100ms).
> - - swap-dx-lanes : Specifies the downstream ports which will swap the
> - differential-pair (D+/D-), default is not-swapped.
> - - swap-us-lanes : Selects the upstream port differential-pair (D+/D-)
> - swapping (boolean, default is not-swapped)
> + - swap-dx-lanes : Specifies the ports which will swap the differential-pair
> + (D+/D-), default is not-swapped.
>
> Examples:
> usb2512b@2c {
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread