Re: [PATCH v2 2/3] phy: realtek: usb: Add driver for the Realtek SoC USB 2.0/3.0 PHY

Re: [PATCH v2 2/3] phy: realtek: usb: Add driver for the Realtek SoC USB 2.0/3.0 PHY

[Greg Kroah-Hartman]

On Thu, May 25, 2023 at 10:26:03AM +0800, Stanley Chang wrote:
> Realtek DHC (digital home center) RTD SoCs support DWC3 XHCI USB 2.0/3.0
> controller. Added two drivers to drive the  USB 2.0/3.0 PHY transceivers.
> For USB 3.0 transceivers, a driver phy-rtk-usb3 is provided.
> The driver phy-rtk-usb2 is used to support USB 2.0 transceivers.
> 
> Signed-off-by: Stanley Chang <stanley_chang@realtek.com>
> Reported-by: kernel test robot <lkp@intel.com>

The kernel test robot did not report that a new driver was needed :(


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 2/3] phy: realtek: usb: Add driver for the Realtek SoC USB 2.0/3.0 PHY

[Greg Kroah-Hartman]

On Tue, May 30, 2023 at 01:50:51AM +0000, Stanley Chang[昌育德] wrote:
> Hi Greg,
> 
> > On Thu, May 25, 2023 at 10:26:03AM +0800, Stanley Chang wrote:
> > > Realtek DHC (digital home center) RTD SoCs support DWC3 XHCI USB
> > > 2.0/3.0 controller. Added two drivers to drive the  USB 2.0/3.0 PHY
> > transceivers.
> > > For USB 3.0 transceivers, a driver phy-rtk-usb3 is provided.
> > > The driver phy-rtk-usb2 is used to support USB 2.0 transceivers.
> > >
> > > Signed-off-by: Stanley Chang <stanley_chang@realtek.com>
> > > Reported-by: kernel test robot <lkp@intel.com>
> > 
> > The kernel test robot did not report that a new driver was needed :(
> > 
> 
> This report is based on v1 patch.
> https://patchwork.kernel.org/project/linux-usb/patch/20230519045825.28369-2-stanley_chang@realtek.com/
> 
> The original driver is at drivers/usb/phy/
> If this report is not appropriate, I will remove it.

Does it make sense to include it?

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 1/3] usb: phy: add usb phy notify port status API

[Greg Kroah-Hartman]

On Thu, May 25, 2023 at 10:26:02AM +0800, Stanley Chang wrote:
> In Realtek SoC, the parameter of usb phy is designed to can dynamic
> tuning base on port status. Therefore, add a notify callback of phy
> driver when usb port status change.
> 
> Signed-off-by: Stanley Chang <stanley_chang@realtek.com>
> ---
> v1 to v2 change:
>     No change
> ---
>  drivers/usb/core/hub.c  | 13 +++++++++++++
>  include/linux/usb/phy.h | 14 ++++++++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 97a0f8faea6e..b4fbbeae1927 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -614,6 +614,19 @@ static int hub_ext_port_status(struct usb_hub *hub, int port1, int type,
>  		ret = 0;
>  	}
>  	mutex_unlock(&hub->status_mutex);
> +
> +	if (!ret) {
> +		struct usb_device *hdev = hub->hdev;
> +
> +		if (hdev && !hdev->parent) {
> +			struct usb_hcd *hcd = bus_to_hcd(hdev->bus);
> +
> +			if (hcd->usb_phy)
> +				usb_phy_notify_port_status(hcd->usb_phy,
> +					    port1 - 1, *status, *change);
> +		}
> +	}
> +
>  	return ret;
>  }
>  
> diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h
> index e4de6bc1f69b..53bf3540098f 100644
> --- a/include/linux/usb/phy.h
> +++ b/include/linux/usb/phy.h
> @@ -144,6 +144,10 @@ struct usb_phy {
>  	 */
>  	int	(*set_wakeup)(struct usb_phy *x, bool enabled);
>  
> +	/* notify phy port status change */
> +	int	(*notify_port_status)(struct usb_phy *x,
> +		int port, u16 portstatus, u16 portchange);
> +
>  	/* notify phy connect status change */
>  	int	(*notify_connect)(struct usb_phy *x,
>  			enum usb_device_speed speed);

Why can't this be part of the same notify_connect() callback?

What makes it different somehow?  Please document this much better.


> @@ -316,6 +320,16 @@ usb_phy_set_wakeup(struct usb_phy *x, bool enabled)
>  		return 0;
>  }
>  
> +static inline int
> +usb_phy_notify_port_status(struct usb_phy *x, int port, u16 portstatus,
> +	    u16 portchange)
> +{
> +	if (x && x->notify_port_status)

How can x ever be NULL?

thanks,

greg k-h

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 1/3] usb: phy: add usb phy notify port status API

[Greg Kroah-Hartman]

On Tue, May 30, 2023 at 02:19:45AM +0000, Stanley Chang[昌育德] wrote:
> Hi Greg,
> 
> > > --- a/include/linux/usb/phy.h
> > > +++ b/include/linux/usb/phy.h
> > > @@ -144,6 +144,10 @@ struct usb_phy {
> > >        */
> > >       int     (*set_wakeup)(struct usb_phy *x, bool enabled);
> > >
> > > +     /* notify phy port status change */
> > > +     int     (*notify_port_status)(struct usb_phy *x,
> > > +             int port, u16 portstatus, u16 portchange);
> > > +
> > >       /* notify phy connect status change */
> > >       int     (*notify_connect)(struct usb_phy *x,
> > >                       enum usb_device_speed speed);
> > 
> > Why can't this be part of the same notify_connect() callback?
> 
> The notify connect is at device ready. But I want notify port status change before port reset.
> 
> > What makes it different somehow?  Please document this much better.
> 
> In Realtek phy driver, we have designed to dynamically adjust disconnection level and calibrate phy parameters.
> So we do this when the device connected bit changes and when the disconnected bit changes.
> Port status change notification:
> 1. Check if portstatus is USB_PORT_STAT_CONNECTION and portchange is USB_PORT_STAT_C_CONNECTION.
>   The device is connected, the driver lowers the disconnection level and calibrates the phy parameters.
> 2. The device disconnects, the driver increases the disconnect level and calibrates the phy parameters.
> 
> If we adjust the disconnection level in notify_connect , the disconnect may have been triggered at this stage. 
> So we need to change that as early as possible.

Please put this type of information in the changelog and in the comments
for the callback when you resubmit it.

thanks,

greg k-h

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 3/3] dt-bindings: phy: realtek: Add the doc about the Realtek SoC USB 2.0/3.0 PHY

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 1451 bytes --]

Hey,

On Thu, May 25, 2023 at 10:26:04AM +0800, Stanley Chang wrote:

> +properties:
> +  compatible:
> +    enum:
> +      - realtek,usb2phy
> +      - realtek,rtd-usb2phy
> +      - realtek,rtd1295-usb2phy
> +      - realtek,rtd1395-usb2phy
> +      - realtek,rtd1619-usb2phy
> +      - realtek,rtd1319-usb2phy
> +      - realtek,rtd1619b-usb2phy
> +      - realtek,rtd1312c-usb2phy
> +      - realtek,rtd1319d-usb2phy
> +      - realtek,rtd1315e-usb2phy

> +properties:
> +  compatible:
> +    enum:
> +      - realtek,usb3phy
> +      - realtek,rtd-usb3phy
> +      - realtek,rtd1295-usb3phy
> +      - realtek,rtd1619-usb3phy
> +      - realtek,rtd1319-usb3phy
> +      - realtek,rtd1619b-usb3phy
> +      - realtek,rtd1319d-usb3phy

Ignoring everything else, because I really want Krzysztof or Rob to
review this rather than me, but what's going on here with the
compatibles?
What hardware do "usbNphy" and "rtd-usbNphy" represent?

You have device-specific compatibles, which is great, but you also allow
only those two generic ones. I had a _brief_ look at the driver, and it
seems like there is no decision making done based on the compatibles,
only on the properties. Is that correct?
If it is, I would understand having "realtek,usb3phy" as a fallback
compatible for "realtek,rtd1619-usb3phy", but I do not get the current
setup.

Also, I really think this should be broken down into two patches, one
for each of USB 2 & 3.

Cheers,
Conor.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 3/3] dt-bindings: phy: realtek: Add the doc about the Realtek SoC USB 2.0/3.0 PHY

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 2488 bytes --]

On Tue, May 30, 2023 at 03:08:29AM +0000, Stanley Chang[昌育德] wrote:
> Hi Conor,
> 
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - realtek,usb2phy
> > > +      - realtek,rtd-usb2phy
> > > +      - realtek,rtd1295-usb2phy
> > > +      - realtek,rtd1395-usb2phy
> > > +      - realtek,rtd1619-usb2phy
> > > +      - realtek,rtd1319-usb2phy
> > > +      - realtek,rtd1619b-usb2phy
> > > +      - realtek,rtd1312c-usb2phy
> > > +      - realtek,rtd1319d-usb2phy
> > > +      - realtek,rtd1315e-usb2phy
> 
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - realtek,usb3phy
> > > +      - realtek,rtd-usb3phy
> > > +      - realtek,rtd1295-usb3phy
> > > +      - realtek,rtd1619-usb3phy
> > > +      - realtek,rtd1319-usb3phy
> > > +      - realtek,rtd1619b-usb3phy
> > > +      - realtek,rtd1319d-usb3phy
> 
> > Ignoring everything else, because I really want Krzysztof or Rob to
> > review this rather than me, but what's going on here with the
> > compatibles?
> > What hardware do "usbNphy" and "rtd-usbNphy" represent?
> > 
> > You have device-specific compatibles, which is great, but you also allow
> > only those two generic ones. I had a _brief_ look at the driver, and it
> > seems like there is no decision making done based on the compatibles,
> > only on the properties. Is that correct?
> > If it is, I would understand having "realtek,usb3phy" as a fallback
> > compatible for "realtek,rtd1619-usb3phy", but I do not get the current
> > setup.
> 
> This driver is compatible with all Realtek RTD SoCs without specifying different settings.
> So use "realtek,usb3phy" as fallback compatible for all SoCs.
> This is the compatible name we use.
> Other compatible names simply indicate that the driver supports the SoCs.

Then you should write the binding such that having fallback compatibles
is permitted. Try plugging
compatible = "realtek,rtd1295-usb2phy", "realtek,rtd-usb2phy", "realtek,usb2phy";
into your example below and see what happens.

> The name "usbNphy" and "rtd-usbNphy" seem to be more generic for all RTD SoCs,
> but they are not device-specific compatible.
> Do you have a better suggestion?

Write the binding so that having fallback compatibles in the DT actually
works, don't add the SoC-specific ones merely as indicators that those
SoCs are supported and don't permit "realtek,usbNphy" or
"realtek,rtd-usbNphy" in isolation ;)

Cheers,
Conor.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 3/3] dt-bindings: phy: realtek: Add the doc about the Realtek SoC USB 2.0/3.0 PHY

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 2589 bytes --]

On Thu, Jun 01, 2023 at 02:24:28AM +0000, Stanley Chang[昌育德] wrote:
> Hi Conor,
> 
> > > > You have device-specific compatibles, which is great, but you also allow
> > > > only those two generic ones. I had a _brief_ look at the driver, and it
> > > > seems like there is no decision making done based on the compatibles,
> > > > only on the properties. Is that correct?
> > > > If it is, I would understand having "realtek,usb3phy" as a fallback
> > > > compatible for "realtek,rtd1619-usb3phy", but I do not get the current
> > > > setup.
> > >
> > > This driver is compatible with all Realtek RTD SoCs without specifying different settings.
> > > So use "realtek,usb3phy" as fallback compatible for all SoCs.
> > > This is the compatible name we use.
> > > Other compatible names simply indicate that the driver supports the SoCs.
> > 
> > Then you should write the binding such that having fallback compatibles
> > is permitted. Try plugging
> > compatible = "realtek,rtd1295-usb2phy", "realtek,rtd-usb2phy", "realtek,usb2phy";
> > into your example below and see what happens.
> > 
> > > The name "usbNphy" and "rtd-usbNphy" seem to be more generic for all RTD SoCs,
> > > but they are not device-specific compatible.
> > > Do you have a better suggestion?
> > 
> > Write the binding so that having fallback compatibles in the DT actually
> > works, don't add the SoC-specific ones merely as indicators that those
> > SoCs are supported and don't permit "realtek,usbNphy" or
> > "realtek,rtd-usbNphy" in isolation ;)
> >
> 
> As far as I understand what you mean.
> I should follow other docs to define compatible.
> Reference:
>   Documentation/devicetree/bindings/phy/mediatek,xsphy.yaml
>   drivers/phy/mediatek/phy-mtk-xsphy.c
> 
> For example:
> 
>   compatible:
>     items:
>       - enum:
>           - realtek,rtd1295-usb2phy
>           - realtek,rtd1395-usb2phy
>           - realtek,rtd1619-usb2phy
>           - realtek,rtd1319-usb2phy
>           - realtek,rtd1619b-usb2phy
>           - realtek,rtd1312c-usb2phy
>           - realtek,rtd1319d-usb2phy
>           - realtek,rtd1315e-usb2phy
>       - const: realtek,usb2phy
> 
> examples:
>   -
>     dwc3_u3drd_usb2phy: dwc3_u3drd_usb2phy@98013e14 {
>         compatible = "realtek,rtd1319-usb2phy", "realtek,usb2phy";
>   
> And use only "Realtek, usb2phy" in the driver.
> static const struct of_device_id usbphy_rtk_dt_match[] = {
>         { .compatible = "realtek,usb2phy", },
>         {},
> };

Yes, this would be a vast improvement, thanks.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 3/3] dt-bindings: phy: realtek: Add the doc about the Realtek SoC USB 2.0/3.0 PHY

[Krzysztof Kozlowski]

On 01/06/2023 12:49, Stanley Chang[昌育德] wrote:
> Hi Krzysztof,
> 
>> Thank you for your patch. There is something to discuss/improve.
>>
>> Actually a lot... The bindings are not suitable for review.
> 
> Thanks for your patience in reviewing my patches.
> 
> Most of the properties are about the phy parameters.
> Is the phy parameter data suitable to be placed in DTS?
> I referenced other phy drivers. 
> These parameters should not be defined in dts.
> I would move the parameters to the driver.

If these can be in the driver, why would ever they be in DTS in the
first place?

> 
>>> +  realtek,usb:
>>> +    description: The phandler of realtek dwc3 node
>>
>> "phandler"? Except obvious typo, drop "The phandler of" and describe what is
>> it for.
> 
> realtek,usb is a phandle of syscon used to control realtek dwc3 register.

Then no, phy should not control dwc3.

> 
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>
>> Anyway, it shouldn't be here. No, no.
> 
> Can I use it for phandle of syscon?

PHY getting phandle to block using this PHY? Looks wrong. Why would PHY
need to poke IP block register?


Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v2 3/3] dt-bindings: phy: realtek: Add the doc about the Realtek SoC USB 2.0/3.0 PHY

[Krzysztof Kozlowski]

On 02/06/2023 05:20, Stanley Chang[昌育德] wrote:
> Hi Krzysztof,
> 
>>>
>>> Most of the properties are about the phy parameters.
>>> Is the phy parameter data suitable to be placed in DTS?
>>> I referenced other phy drivers.
>>> These parameters should not be defined in dts.
>>> I would move the parameters to the driver.
>>
>> If these can be in the driver, why would ever they be in DTS in the first place?
>>
> Our platforms have 3 xhci controllers which map to 3 different phy ports.

You mean on the same SoC?

> And the three phy ports use the same driver, but the parameters are different.
> So I put the parameter settings in DTS, we have 3 usb-phy nodes representing 3 phy ports.
> Also, some parameters have to be adjusted for different boards.
> Therefore, it is more applicable in DTS than in driver.

Then it looks justified in DT, so please write proper descriptions for
proper properties. Underscores are not allowed in node names. No fake
nodes. Properties should usually describe physical/hardware effect not
the register value.

qcom,usb-snps-femto-v2.yaml is nice example. Few Mediatek bindings also
would work.
'

Best regards,
Krzysztof


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy