diff mbox series

[2/8] usb: dwc3: core: Access XHCI address space temporarily to read port info

Message ID 20230310163420.7582-3-quic_kriskura@quicinc.com
State New
Headers show
Series Add multiport support for DWC3 controllers | expand

Commit Message

Krishna Kurapati March 10, 2023, 4:34 p.m. UTC
Currently host-only capable DWC3 controllers support Multiport. Temporarily
map XHCI address space for host-only controllers and parse XHCI Extended
Capabilities registers to read number of physical usb ports connected to the
multiport controller (presuming each port is at least HS capable) and extract
info on how many of these ports are Super Speed capable.

Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
---
 drivers/usb/dwc3/core.c | 75 +++++++++++++++++++++++++++++++++++++++++
 drivers/usb/dwc3/core.h |  9 +++++
 2 files changed, 84 insertions(+)

Comments

Thinh Nguyen March 10, 2023, 11:55 p.m. UTC | #1
On Fri, Mar 10, 2023, Krishna Kurapati wrote:
> Currently host-only capable DWC3 controllers support Multiport. Temporarily
> map XHCI address space for host-only controllers and parse XHCI Extended
> Capabilities registers to read number of physical usb ports connected to the
> multiport controller (presuming each port is at least HS capable) and extract
> info on how many of these ports are Super Speed capable.
> 
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> ---
>  drivers/usb/dwc3/core.c | 75 +++++++++++++++++++++++++++++++++++++++++
>  drivers/usb/dwc3/core.h |  9 +++++
>  2 files changed, 84 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 476b63618511..076c0f8a4441 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -37,6 +37,7 @@
>  #include "core.h"
>  #include "gadget.h"
>  #include "io.h"
> +#include "../host/xhci.h"

I think better to duplicate some of the logic in dwc3 driver and avoid
any direct dependency with the xhci driver.

>  
>  #include "debug.h"
>  
> @@ -1750,6 +1751,65 @@ static struct extcon_dev *dwc3_get_extcon(struct dwc3 *dwc)
>  	return edev;
>  }
>  
> +static int dwc3_read_port_info(struct dwc3 *dwc, struct resource *res)
> +{
> +	void __iomem		*regs;
> +	struct resource         dwc_res;
> +	u32			offset;
> +	u32			temp;
> +	u8			major_revision;
> +	int			ret = 0;
> +
> +	/*
> +	 * Remap xHCI address space to access XHCI ext cap regs,
> +	 * since it is needed to get port info.
> +	 */
> +	dwc_res = *res;
> +	dwc_res.start += 0;
> +	dwc_res.end = dwc->xhci_resources[0].start +
> +				DWC3_XHCI_REGS_END;

Isn't dwc->xhci_resources[0] already setup at this point? Can we use
dwc->xhci_resources[0] directly without copy the setting in dwc_res?

> +
> +	regs = ioremap(dwc_res.start, resource_size(&dwc_res));
> +	if (IS_ERR(regs))
> +		return PTR_ERR(regs);
> +
> +	offset = xhci_find_next_ext_cap(regs, 0,
> +					XHCI_EXT_CAPS_PROTOCOL);
> +	while (offset) {
> +		temp = readl(regs + offset);
> +		major_revision = XHCI_EXT_PORT_MAJOR(temp);
> +
> +		temp = readl(regs + offset + 0x08);
> +		if (major_revision == 0x03) {
> +			dwc->num_ss_ports += XHCI_EXT_PORT_COUNT(temp);
> +		} else if (major_revision <= 0x02) {
> +			dwc->num_ports += XHCI_EXT_PORT_COUNT(temp);
> +		} else {
> +			dev_err(dwc->dev, "port revision seems wrong\n");
> +			ret = -EINVAL;
> +			goto unmap_reg;
> +		}
> +
> +		offset = xhci_find_next_ext_cap(regs, offset,
> +						XHCI_EXT_CAPS_PROTOCOL);
> +	}
> +
> +	temp = readl(regs + DWC3_XHCI_HCSPARAMS1);
> +	if (HCS_MAX_PORTS(temp) != (dwc->num_ss_ports + dwc->num_ports)) {
> +		dev_err(dwc->dev, "inconsistency in port info\n");
> +		ret = -EINVAL;
> +		goto unmap_reg;
> +	}
> +
> +	dev_info(dwc->dev,
> +		"num-ports: %d ss-capable: %d\n", dwc->num_ports, dwc->num_ss_ports);

The end user doesn't need to know this info. This should be a debug
message. Perhaps it can be a tracepoint if needed?

> +
> +unmap_reg:
> +	iounmap(regs);
> +	return ret;
> +}
> +
>  static int dwc3_probe(struct platform_device *pdev)
>  {
>  	struct device		*dev = &pdev->dev;
> @@ -1757,6 +1817,7 @@ static int dwc3_probe(struct platform_device *pdev)
>  	struct dwc3		*dwc;
>  
>  	int			ret;
> +	unsigned int		hw_mode;
>  
>  	void __iomem		*regs;
>  
> @@ -1880,6 +1941,20 @@ static int dwc3_probe(struct platform_device *pdev)
>  			goto disable_clks;
>  	}
>  
> +	/*
> +	 * Currently DWC3 controllers that are host-only capable
> +	 * support Multiport.
> +	 */
> +	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
> +	if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) {
> +		ret = dwc3_read_port_info(dwc, res);
> +		if (ret)
> +			goto disable_clks;
> +	} else {
> +		dwc->num_ports = 1;
> +		dwc->num_ss_ports = 1;
> +	}
> +
>  	spin_lock_init(&dwc->lock);
>  	mutex_init(&dwc->mutex);
>  
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 582ebd9cf9c2..74386d6a0277 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -35,6 +35,9 @@
>  
>  #define DWC3_MSG_MAX	500
>  
> +/* XHCI Reg constants */
> +#define DWC3_XHCI_HCSPARAMS1	0x04
> +
>  /* Global constants */
>  #define DWC3_PULL_UP_TIMEOUT	500	/* ms */
>  #define DWC3_BOUNCE_SIZE	1024	/* size of a superspeed bulk */
> @@ -1023,6 +1026,10 @@ struct dwc3_scratchpad_array {
>   * @usb_psy: pointer to power supply interface.
>   * @usb2_phy: pointer to USB2 PHY
>   * @usb3_phy: pointer to USB3 PHY
> + * @num_ports: Indicates the number of physical USB ports present on HW
> + *		presuming each port is at least HS capable

This isn't the number of physical USB ports right? That's the number of
usb2 ports the controller is configured with right?. Perhaps we can use
num_usb2_ports and num_usb3_ports?

> + * @num_ss_ports: Indicates the number of USB ports present on HW that are
> + *		SS Capable
>   * @usb2_generic_phy: pointer to USB2 PHY
>   * @usb3_generic_phy: pointer to USB3 PHY
>   * @phys_ready: flag to indicate that PHYs are ready
> @@ -1158,6 +1165,8 @@ struct dwc3 {
>  	struct usb_phy		*usb2_phy;
>  	struct usb_phy		*usb3_phy;
>  
> +	u32			num_ports;
> +	u32			num_ss_ports;
>  	struct phy		*usb2_generic_phy;
>  	struct phy		*usb3_generic_phy;
>  
> -- 
> 2.39.0
> 

Thanks,
Thinh
Krishna Kurapati March 11, 2023, 2:54 a.m. UTC | #2
On 3/11/2023 5:25 AM, Thinh Nguyen wrote:
> On Fri, Mar 10, 2023, Krishna Kurapati wrote:
>> Currently host-only capable DWC3 controllers support Multiport. Temporarily
>> map XHCI address space for host-only controllers and parse XHCI Extended
>> Capabilities registers to read number of physical usb ports connected to the
>> multiport controller (presuming each port is at least HS capable) and extract
>> info on how many of these ports are Super Speed capable.
>>
>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>> ---
>>   drivers/usb/dwc3/core.c | 75 +++++++++++++++++++++++++++++++++++++++++
>>   drivers/usb/dwc3/core.h |  9 +++++
>>   2 files changed, 84 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 476b63618511..076c0f8a4441 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -37,6 +37,7 @@
>>   #include "core.h"
>>   #include "gadget.h"
>>   #include "io.h"
>> +#include "../host/xhci.h"
> 
> I think better to duplicate some of the logic in dwc3 driver and avoid
> any direct dependency with the xhci driver.
> 
>>   
>>   #include "debug.h"
>>   
>> @@ -1750,6 +1751,65 @@ static struct extcon_dev *dwc3_get_extcon(struct dwc3 *dwc)
>>   	return edev;
>>   }
>>   
>> +static int dwc3_read_port_info(struct dwc3 *dwc, struct resource *res)
>> +{
>> +	void __iomem		*regs;
>> +	struct resource         dwc_res;
>> +	u32			offset;
>> +	u32			temp;
>> +	u8			major_revision;
>> +	int			ret = 0;
>> +
>> +	/*
>> +	 * Remap xHCI address space to access XHCI ext cap regs,
>> +	 * since it is needed to get port info.
>> +	 */
>> +	dwc_res = *res;
>> +	dwc_res.start += 0;
>> +	dwc_res.end = dwc->xhci_resources[0].start +
>> +				DWC3_XHCI_REGS_END;
> 
> Isn't dwc->xhci_resources[0] already setup at this point? Can we use
> dwc->xhci_resources[0] directly without copy the setting in dwc_res?
> 
>> +
>> +	regs = ioremap(dwc_res.start, resource_size(&dwc_res));
>> +	if (IS_ERR(regs))
>> +		return PTR_ERR(regs);
>> +
>> +	offset = xhci_find_next_ext_cap(regs, 0,
>> +					XHCI_EXT_CAPS_PROTOCOL);
>> +	while (offset) {
>> +		temp = readl(regs + offset);
>> +		major_revision = XHCI_EXT_PORT_MAJOR(temp);
>> +
>> +		temp = readl(regs + offset + 0x08);
>> +		if (major_revision == 0x03) {
>> +			dwc->num_ss_ports += XHCI_EXT_PORT_COUNT(temp);
>> +		} else if (major_revision <= 0x02) {
>> +			dwc->num_ports += XHCI_EXT_PORT_COUNT(temp);
>> +		} else {
>> +			dev_err(dwc->dev, "port revision seems wrong\n");
>> +			ret = -EINVAL;
>> +			goto unmap_reg;
>> +		}
>> +
>> +		offset = xhci_find_next_ext_cap(regs, offset,
>> +						XHCI_EXT_CAPS_PROTOCOL);
>> +	}
>> +
>> +	temp = readl(regs + DWC3_XHCI_HCSPARAMS1);
>> +	if (HCS_MAX_PORTS(temp) != (dwc->num_ss_ports + dwc->num_ports)) {
>> +		dev_err(dwc->dev, "inconsistency in port info\n");
>> +		ret = -EINVAL;
>> +		goto unmap_reg;
>> +	}
>> +
>> +	dev_info(dwc->dev,
>> +		"num-ports: %d ss-capable: %d\n", dwc->num_ports, dwc->num_ss_ports);
> 
> The end user doesn't need to know this info. This should be a debug
> message. Perhaps it can be a tracepoint if needed?
> 
>> +
>> +unmap_reg:
>> +	iounmap(regs);
>> +	return ret;
>> +}
>> +
>>   static int dwc3_probe(struct platform_device *pdev)
>>   {
>>   	struct device		*dev = &pdev->dev;
>> @@ -1757,6 +1817,7 @@ static int dwc3_probe(struct platform_device *pdev)
>>   	struct dwc3		*dwc;
>>   
>>   	int			ret;
>> +	unsigned int		hw_mode;
>>   
>>   	void __iomem		*regs;
>>   
>> @@ -1880,6 +1941,20 @@ static int dwc3_probe(struct platform_device *pdev)
>>   			goto disable_clks;
>>   	}
>>   
>> +	/*
>> +	 * Currently DWC3 controllers that are host-only capable
>> +	 * support Multiport.
>> +	 */
>> +	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
>> +	if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) {
>> +		ret = dwc3_read_port_info(dwc, res);
>> +		if (ret)
>> +			goto disable_clks;
>> +	} else {
>> +		dwc->num_ports = 1;
>> +		dwc->num_ss_ports = 1;
>> +	}
>> +
>>   	spin_lock_init(&dwc->lock);
>>   	mutex_init(&dwc->mutex);
>>   
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 582ebd9cf9c2..74386d6a0277 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -35,6 +35,9 @@
>>   
>>   #define DWC3_MSG_MAX	500
>>   
>> +/* XHCI Reg constants */
>> +#define DWC3_XHCI_HCSPARAMS1	0x04
>> +
>>   /* Global constants */
>>   #define DWC3_PULL_UP_TIMEOUT	500	/* ms */
>>   #define DWC3_BOUNCE_SIZE	1024	/* size of a superspeed bulk */
>> @@ -1023,6 +1026,10 @@ struct dwc3_scratchpad_array {
>>    * @usb_psy: pointer to power supply interface.
>>    * @usb2_phy: pointer to USB2 PHY
>>    * @usb3_phy: pointer to USB3 PHY
>> + * @num_ports: Indicates the number of physical USB ports present on HW
>> + *		presuming each port is at least HS capable
> 
> This isn't the number of physical USB ports right? That's the number of
> usb2 ports the controller is configured with right?. Perhaps we can use
> num_usb2_ports and num_usb3_ports?
> 
Hi Thinh,

   Yes, naming this might have created a little confusion.
num_ports is supposed to indicate number of usb2 ports in the controller.

Incase of sa8295 (4 port controller with first two ports having ss 
capability), num_ports would be 4 and num_ss_ports would be 2. (and not 
6 as what num_ports usually sounds).
I can rename them accordingly in the next version and update the 
description as well.

Regards,
Krishna,

>> + * @num_ss_ports: Indicates the number of USB ports present on HW that are
>> + *		SS Capable
>>    * @usb2_generic_phy: pointer to USB2 PHY
>>    * @usb3_generic_phy: pointer to USB3 PHY
>>    * @phys_ready: flag to indicate that PHYs are ready
>> @@ -1158,6 +1165,8 @@ struct dwc3 {
>>   	struct usb_phy		*usb2_phy;
>>   	struct usb_phy		*usb3_phy;
>>   
>> +	u32			num_ports;
>> +	u32			num_ss_ports;
>>   	struct phy		*usb2_generic_phy;
>>   	struct phy		*usb3_generic_phy;
>>   
>> -- 
>> 2.39.0
>>
> 
> Thanks,
> Thinh
Krishna Kurapati March 11, 2023, 3:04 a.m. UTC | #3
On 3/11/2023 8:24 AM, Krishna Kurapati PSSNV wrote:
> 
> 
> On 3/11/2023 5:25 AM, Thinh Nguyen wrote:
>> On Fri, Mar 10, 2023, Krishna Kurapati wrote:
>>> Currently host-only capable DWC3 controllers support Multiport. 
>>> Temporarily
>>> map XHCI address space for host-only controllers and parse XHCI Extended
>>> Capabilities registers to read number of physical usb ports connected 
>>> to the
>>> multiport controller (presuming each port is at least HS capable) and 
>>> extract
>>> info on how many of these ports are Super Speed capable.
>>>
>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>> ---
>>>   drivers/usb/dwc3/core.c | 75 +++++++++++++++++++++++++++++++++++++++++
>>>   drivers/usb/dwc3/core.h |  9 +++++
>>>   2 files changed, 84 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index 476b63618511..076c0f8a4441 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -37,6 +37,7 @@
>>>   #include "core.h"
>>>   #include "gadget.h"
>>>   #include "io.h"
>>> +#include "../host/xhci.h"
>>
>> I think better to duplicate some of the logic in dwc3 driver and avoid
>> any direct dependency with the xhci driver.
>>
>>>   #include "debug.h"
>>> @@ -1750,6 +1751,65 @@ static struct extcon_dev 
>>> *dwc3_get_extcon(struct dwc3 *dwc)
>>>       return edev;
>>>   }
>>> +static int dwc3_read_port_info(struct dwc3 *dwc, struct resource *res)
>>> +{
>>> +    void __iomem        *regs;
>>> +    struct resource         dwc_res;
>>> +    u32            offset;
>>> +    u32            temp;
>>> +    u8            major_revision;
>>> +    int            ret = 0;
>>> +
>>> +    /*
>>> +     * Remap xHCI address space to access XHCI ext cap regs,
>>> +     * since it is needed to get port info.
>>> +     */
>>> +    dwc_res = *res;
>>> +    dwc_res.start += 0;
>>> +    dwc_res.end = dwc->xhci_resources[0].start +
>>> +                DWC3_XHCI_REGS_END;
>>
>> Isn't dwc->xhci_resources[0] already setup at this point? Can we use
>> dwc->xhci_resources[0] directly without copy the setting in dwc_res?
>>
>>> +
>>> +    regs = ioremap(dwc_res.start, resource_size(&dwc_res));
>>> +    if (IS_ERR(regs))
>>> +        return PTR_ERR(regs);
>>> +
>>> +    offset = xhci_find_next_ext_cap(regs, 0,
>>> +                    XHCI_EXT_CAPS_PROTOCOL);
>>> +    while (offset) {
>>> +        temp = readl(regs + offset);
>>> +        major_revision = XHCI_EXT_PORT_MAJOR(temp);
>>> +
>>> +        temp = readl(regs + offset + 0x08);
>>> +        if (major_revision == 0x03) {
>>> +            dwc->num_ss_ports += XHCI_EXT_PORT_COUNT(temp);
>>> +        } else if (major_revision <= 0x02) {
>>> +            dwc->num_ports += XHCI_EXT_PORT_COUNT(temp);
>>> +        } else {
>>> +            dev_err(dwc->dev, "port revision seems wrong\n");
>>> +            ret = -EINVAL;
>>> +            goto unmap_reg;
>>> +        }
>>> +
>>> +        offset = xhci_find_next_ext_cap(regs, offset,
>>> +                        XHCI_EXT_CAPS_PROTOCOL);
>>> +    }
>>> +
>>> +    temp = readl(regs + DWC3_XHCI_HCSPARAMS1);
>>> +    if (HCS_MAX_PORTS(temp) != (dwc->num_ss_ports + dwc->num_ports)) {
>>> +        dev_err(dwc->dev, "inconsistency in port info\n");
>>> +        ret = -EINVAL;
>>> +        goto unmap_reg;
>>> +    }
>>> +
>>> +    dev_info(dwc->dev,
>>> +        "num-ports: %d ss-capable: %d\n", dwc->num_ports, 
>>> dwc->num_ss_ports);
>>
>> The end user doesn't need to know this info. This should be a debug
>> message. Perhaps it can be a tracepoint if needed?
>>
>>> +
>>> +unmap_reg:
>>> +    iounmap(regs);
>>> +    return ret;
>>> +}
>>> +
>>>   static int dwc3_probe(struct platform_device *pdev)
>>>   {
>>>       struct device        *dev = &pdev->dev;
>>> @@ -1757,6 +1817,7 @@ static int dwc3_probe(struct platform_device 
>>> *pdev)
>>>       struct dwc3        *dwc;
>>>       int            ret;
>>> +    unsigned int        hw_mode;
>>>       void __iomem        *regs;
>>> @@ -1880,6 +1941,20 @@ static int dwc3_probe(struct platform_device 
>>> *pdev)
>>>               goto disable_clks;
>>>       }
>>> +    /*
>>> +     * Currently DWC3 controllers that are host-only capable
>>> +     * support Multiport.
>>> +     */
>>> +    hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
>>> +    if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) {
>>> +        ret = dwc3_read_port_info(dwc, res);
>>> +        if (ret)
>>> +            goto disable_clks;
>>> +    } else {
>>> +        dwc->num_ports = 1;
>>> +        dwc->num_ss_ports = 1;
>>> +    }
>>> +
>>>       spin_lock_init(&dwc->lock);
>>>       mutex_init(&dwc->mutex);
>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>> index 582ebd9cf9c2..74386d6a0277 100644
>>> --- a/drivers/usb/dwc3/core.h
>>> +++ b/drivers/usb/dwc3/core.h
>>> @@ -35,6 +35,9 @@
>>>   #define DWC3_MSG_MAX    500
>>> +/* XHCI Reg constants */
>>> +#define DWC3_XHCI_HCSPARAMS1    0x04
>>> +
>>>   /* Global constants */
>>>   #define DWC3_PULL_UP_TIMEOUT    500    /* ms */
>>>   #define DWC3_BOUNCE_SIZE    1024    /* size of a superspeed bulk */
>>> @@ -1023,6 +1026,10 @@ struct dwc3_scratchpad_array {
>>>    * @usb_psy: pointer to power supply interface.
>>>    * @usb2_phy: pointer to USB2 PHY
>>>    * @usb3_phy: pointer to USB3 PHY
>>> + * @num_ports: Indicates the number of physical USB ports present on HW
>>> + *        presuming each port is at least HS capable
>>
>> This isn't the number of physical USB ports right? That's the number of
>> usb2 ports the controller is configured with right?. Perhaps we can use
>> num_usb2_ports and num_usb3_ports?
>>
> Hi Thinh,
> 
>    Yes, naming this might have created a little confusion.
> num_ports is supposed to indicate number of usb2 ports in the controller.
> 
> Incase of sa8295 (4 port controller with first two ports having ss 
> capability), num_ports would be 4 and num_ss_ports would be 2. (and not 
> 6 as what num_ports usually sounds).
> I can rename them accordingly in the next version and update the 
> description as well.
> 
> Regards,
> Krishna,
> 
Hi Thinh,

One reason I didn't mention something like num_hs_ports and sticked to 
num_ports is because in core driver, wherever we need to do phy 
operations like:

for (i = 0; i < num_ports; i++)
{
	phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST);
	phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST);
}

The intention is as follows:
If number of usb2 ports is 4, the loop can go from 0-3 and its fine.
If number of usb3-ports is 2, we don't know for sure, if the first 2 
ports are SS capable or some other ports like (3 and 4) are SS capable.
So instead, I looped all phy operations around all usb2_generic_phy's 
and usb3_generic_phy's. If they are NULL, we just bail out inside phy 
operation.

While doing so, looping SS Phy operations around num_usb2_ports didn't 
sound good. From code view, it would be like we are looping usb3_phy ops 
around num_usb2_ports value (logically it is still correct as each port 
is atleast HS capable). So to avoid this, I named the variable as 
num_ports instead of num_usb2_ports

Regards,
Krishna,

>>> + * @num_ss_ports: Indicates the number of USB ports present on HW 
>>> that are
>>> + *        SS Capable
>>>    * @usb2_generic_phy: pointer to USB2 PHY
>>>    * @usb3_generic_phy: pointer to USB3 PHY
>>>    * @phys_ready: flag to indicate that PHYs are ready
>>> @@ -1158,6 +1165,8 @@ struct dwc3 {
>>>       struct usb_phy        *usb2_phy;
>>>       struct usb_phy        *usb3_phy;
>>> +    u32            num_ports;
>>> +    u32            num_ss_ports;
>>>       struct phy        *usb2_generic_phy;
>>>       struct phy        *usb3_generic_phy;
>>> -- 
>>> 2.39.0
>>>
>>
>> Thanks,
>> Thinh
Thinh Nguyen March 13, 2023, 11:53 p.m. UTC | #4
On Sat, Mar 11, 2023, Krishna Kurapati PSSNV wrote:
> 
> 
> On 3/11/2023 8:24 AM, Krishna Kurapati PSSNV wrote:
> > 
> > 
> > On 3/11/2023 5:25 AM, Thinh Nguyen wrote:
> > > On Fri, Mar 10, 2023, Krishna Kurapati wrote:
> > > > Currently host-only capable DWC3 controllers support Multiport.
> > > > Temporarily
> > > > map XHCI address space for host-only controllers and parse XHCI Extended
> > > > Capabilities registers to read number of physical usb ports
> > > > connected to the
> > > > multiport controller (presuming each port is at least HS
> > > > capable) and extract
> > > > info on how many of these ports are Super Speed capable.
> > > > 
> > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > > > ---
> > > >   drivers/usb/dwc3/core.c | 75 +++++++++++++++++++++++++++++++++++++++++
> > > >   drivers/usb/dwc3/core.h |  9 +++++
> > > >   2 files changed, 84 insertions(+)
> > > > 
> > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > index 476b63618511..076c0f8a4441 100644
> > > > --- a/drivers/usb/dwc3/core.c
> > > > +++ b/drivers/usb/dwc3/core.c
> > > > @@ -37,6 +37,7 @@
> > > >   #include "core.h"
> > > >   #include "gadget.h"
> > > >   #include "io.h"
> > > > +#include "../host/xhci.h"
> > > 
> > > I think better to duplicate some of the logic in dwc3 driver and avoid
> > > any direct dependency with the xhci driver.
> > > 
> > > >   #include "debug.h"
> > > > @@ -1750,6 +1751,65 @@ static struct extcon_dev
> > > > *dwc3_get_extcon(struct dwc3 *dwc)
> > > >       return edev;
> > > >   }
> > > > +static int dwc3_read_port_info(struct dwc3 *dwc, struct resource *res)
> > > > +{
> > > > +    void __iomem        *regs;
> > > > +    struct resource         dwc_res;
> > > > +    u32            offset;
> > > > +    u32            temp;
> > > > +    u8            major_revision;
> > > > +    int            ret = 0;
> > > > +
> > > > +    /*
> > > > +     * Remap xHCI address space to access XHCI ext cap regs,
> > > > +     * since it is needed to get port info.
> > > > +     */
> > > > +    dwc_res = *res;
> > > > +    dwc_res.start += 0;
> > > > +    dwc_res.end = dwc->xhci_resources[0].start +
> > > > +                DWC3_XHCI_REGS_END;
> > > 
> > > Isn't dwc->xhci_resources[0] already setup at this point? Can we use
> > > dwc->xhci_resources[0] directly without copy the setting in dwc_res?
> > > 
> > > > +
> > > > +    regs = ioremap(dwc_res.start, resource_size(&dwc_res));
> > > > +    if (IS_ERR(regs))
> > > > +        return PTR_ERR(regs);
> > > > +
> > > > +    offset = xhci_find_next_ext_cap(regs, 0,
> > > > +                    XHCI_EXT_CAPS_PROTOCOL);
> > > > +    while (offset) {
> > > > +        temp = readl(regs + offset);
> > > > +        major_revision = XHCI_EXT_PORT_MAJOR(temp);
> > > > +
> > > > +        temp = readl(regs + offset + 0x08);
> > > > +        if (major_revision == 0x03) {
> > > > +            dwc->num_ss_ports += XHCI_EXT_PORT_COUNT(temp);
> > > > +        } else if (major_revision <= 0x02) {
> > > > +            dwc->num_ports += XHCI_EXT_PORT_COUNT(temp);
> > > > +        } else {
> > > > +            dev_err(dwc->dev, "port revision seems wrong\n");
> > > > +            ret = -EINVAL;
> > > > +            goto unmap_reg;
> > > > +        }
> > > > +
> > > > +        offset = xhci_find_next_ext_cap(regs, offset,
> > > > +                        XHCI_EXT_CAPS_PROTOCOL);
> > > > +    }
> > > > +
> > > > +    temp = readl(regs + DWC3_XHCI_HCSPARAMS1);
> > > > +    if (HCS_MAX_PORTS(temp) != (dwc->num_ss_ports + dwc->num_ports)) {
> > > > +        dev_err(dwc->dev, "inconsistency in port info\n");
> > > > +        ret = -EINVAL;
> > > > +        goto unmap_reg;
> > > > +    }
> > > > +
> > > > +    dev_info(dwc->dev,
> > > > +        "num-ports: %d ss-capable: %d\n", dwc->num_ports,
> > > > dwc->num_ss_ports);
> > > 
> > > The end user doesn't need to know this info. This should be a debug
> > > message. Perhaps it can be a tracepoint if needed?
> > > 
> > > > +
> > > > +unmap_reg:
> > > > +    iounmap(regs);
> > > > +    return ret;
> > > > +}
> > > > +
> > > >   static int dwc3_probe(struct platform_device *pdev)
> > > >   {
> > > >       struct device        *dev = &pdev->dev;
> > > > @@ -1757,6 +1817,7 @@ static int dwc3_probe(struct
> > > > platform_device *pdev)
> > > >       struct dwc3        *dwc;
> > > >       int            ret;
> > > > +    unsigned int        hw_mode;
> > > >       void __iomem        *regs;
> > > > @@ -1880,6 +1941,20 @@ static int dwc3_probe(struct
> > > > platform_device *pdev)
> > > >               goto disable_clks;
> > > >       }
> > > > +    /*
> > > > +     * Currently DWC3 controllers that are host-only capable
> > > > +     * support Multiport.
> > > > +     */
> > > > +    hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
> > > > +    if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) {
> > > > +        ret = dwc3_read_port_info(dwc, res);
> > > > +        if (ret)
> > > > +            goto disable_clks;
> > > > +    } else {
> > > > +        dwc->num_ports = 1;
> > > > +        dwc->num_ss_ports = 1;
> > > > +    }
> > > > +
> > > >       spin_lock_init(&dwc->lock);
> > > >       mutex_init(&dwc->mutex);
> > > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> > > > index 582ebd9cf9c2..74386d6a0277 100644
> > > > --- a/drivers/usb/dwc3/core.h
> > > > +++ b/drivers/usb/dwc3/core.h
> > > > @@ -35,6 +35,9 @@
> > > >   #define DWC3_MSG_MAX    500
> > > > +/* XHCI Reg constants */
> > > > +#define DWC3_XHCI_HCSPARAMS1    0x04
> > > > +
> > > >   /* Global constants */
> > > >   #define DWC3_PULL_UP_TIMEOUT    500    /* ms */
> > > >   #define DWC3_BOUNCE_SIZE    1024    /* size of a superspeed bulk */
> > > > @@ -1023,6 +1026,10 @@ struct dwc3_scratchpad_array {
> > > >    * @usb_psy: pointer to power supply interface.
> > > >    * @usb2_phy: pointer to USB2 PHY
> > > >    * @usb3_phy: pointer to USB3 PHY
> > > > + * @num_ports: Indicates the number of physical USB ports present on HW
> > > > + *        presuming each port is at least HS capable
> > > 
> > > This isn't the number of physical USB ports right? That's the number of
> > > usb2 ports the controller is configured with right?. Perhaps we can use
> > > num_usb2_ports and num_usb3_ports?
> > > 
> > Hi Thinh,
> > 
> >    Yes, naming this might have created a little confusion.
> > num_ports is supposed to indicate number of usb2 ports in the controller.
> > 
> > Incase of sa8295 (4 port controller with first two ports having ss
> > capability), num_ports would be 4 and num_ss_ports would be 2. (and not
> > 6 as what num_ports usually sounds).
> > I can rename them accordingly in the next version and update the
> > description as well.
> > 
> > Regards,
> > Krishna,
> > 
> Hi Thinh,
> 
> One reason I didn't mention something like num_hs_ports and sticked to
> num_ports is because in core driver, wherever we need to do phy operations
> like:
> 
> for (i = 0; i < num_ports; i++)
> {
> 	phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST);
> 	phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST);
> }
> 
> The intention is as follows:
> If number of usb2 ports is 4, the loop can go from 0-3 and its fine.
> If number of usb3-ports is 2, we don't know for sure, if the first 2 ports
> are SS capable or some other ports like (3 and 4) are SS capable.
> So instead, I looped all phy operations around all usb2_generic_phy's and
> usb3_generic_phy's. If they are NULL, we just bail out inside phy operation.
> 
> While doing so, looping SS Phy operations around num_usb2_ports didn't sound
> good. From code view, it would be like we are looping usb3_phy ops around
> num_usb2_ports value (logically it is still correct as each port is atleast
> HS capable). So to avoid this, I named the variable as num_ports instead of
> num_usb2_ports
> 

Hi Krishna,

I think it's clearer if add this note along with using num_usb2_ports.
We just need to note this once and I think it's sufficient.

Thanks,
Thinh
diff mbox series

Patch

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 476b63618511..076c0f8a4441 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -37,6 +37,7 @@ 
 #include "core.h"
 #include "gadget.h"
 #include "io.h"
+#include "../host/xhci.h"
 
 #include "debug.h"
 
@@ -1750,6 +1751,65 @@  static struct extcon_dev *dwc3_get_extcon(struct dwc3 *dwc)
 	return edev;
 }
 
+static int dwc3_read_port_info(struct dwc3 *dwc, struct resource *res)
+{
+	void __iomem		*regs;
+	struct resource         dwc_res;
+	u32			offset;
+	u32			temp;
+	u8			major_revision;
+	int			ret = 0;
+
+	/*
+	 * Remap xHCI address space to access XHCI ext cap regs,
+	 * since it is needed to get port info.
+	 */
+	dwc_res = *res;
+	dwc_res.start += 0;
+	dwc_res.end = dwc->xhci_resources[0].start +
+				DWC3_XHCI_REGS_END;
+
+	regs = ioremap(dwc_res.start, resource_size(&dwc_res));
+	if (IS_ERR(regs))
+		return PTR_ERR(regs);
+
+	offset = xhci_find_next_ext_cap(regs, 0,
+					XHCI_EXT_CAPS_PROTOCOL);
+	while (offset) {
+		temp = readl(regs + offset);
+		major_revision = XHCI_EXT_PORT_MAJOR(temp);
+
+		temp = readl(regs + offset + 0x08);
+		if (major_revision == 0x03) {
+			dwc->num_ss_ports += XHCI_EXT_PORT_COUNT(temp);
+		} else if (major_revision <= 0x02) {
+			dwc->num_ports += XHCI_EXT_PORT_COUNT(temp);
+		} else {
+			dev_err(dwc->dev, "port revision seems wrong\n");
+			ret = -EINVAL;
+			goto unmap_reg;
+		}
+
+		offset = xhci_find_next_ext_cap(regs, offset,
+						XHCI_EXT_CAPS_PROTOCOL);
+	}
+
+	temp = readl(regs + DWC3_XHCI_HCSPARAMS1);
+	if (HCS_MAX_PORTS(temp) != (dwc->num_ss_ports + dwc->num_ports)) {
+		dev_err(dwc->dev, "inconsistency in port info\n");
+		ret = -EINVAL;
+		goto unmap_reg;
+	}
+
+	dev_info(dwc->dev,
+		"num-ports: %d ss-capable: %d\n", dwc->num_ports, dwc->num_ss_ports);
+
+unmap_reg:
+	iounmap(regs);
+	return ret;
+}
+
 static int dwc3_probe(struct platform_device *pdev)
 {
 	struct device		*dev = &pdev->dev;
@@ -1757,6 +1817,7 @@  static int dwc3_probe(struct platform_device *pdev)
 	struct dwc3		*dwc;
 
 	int			ret;
+	unsigned int		hw_mode;
 
 	void __iomem		*regs;
 
@@ -1880,6 +1941,20 @@  static int dwc3_probe(struct platform_device *pdev)
 			goto disable_clks;
 	}
 
+	/*
+	 * Currently DWC3 controllers that are host-only capable
+	 * support Multiport.
+	 */
+	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
+	if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) {
+		ret = dwc3_read_port_info(dwc, res);
+		if (ret)
+			goto disable_clks;
+	} else {
+		dwc->num_ports = 1;
+		dwc->num_ss_ports = 1;
+	}
+
 	spin_lock_init(&dwc->lock);
 	mutex_init(&dwc->mutex);
 
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 582ebd9cf9c2..74386d6a0277 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -35,6 +35,9 @@ 
 
 #define DWC3_MSG_MAX	500
 
+/* XHCI Reg constants */
+#define DWC3_XHCI_HCSPARAMS1	0x04
+
 /* Global constants */
 #define DWC3_PULL_UP_TIMEOUT	500	/* ms */
 #define DWC3_BOUNCE_SIZE	1024	/* size of a superspeed bulk */
@@ -1023,6 +1026,10 @@  struct dwc3_scratchpad_array {
  * @usb_psy: pointer to power supply interface.
  * @usb2_phy: pointer to USB2 PHY
  * @usb3_phy: pointer to USB3 PHY
+ * @num_ports: Indicates the number of physical USB ports present on HW
+ *		presuming each port is at least HS capable
+ * @num_ss_ports: Indicates the number of USB ports present on HW that are
+ *		SS Capable
  * @usb2_generic_phy: pointer to USB2 PHY
  * @usb3_generic_phy: pointer to USB3 PHY
  * @phys_ready: flag to indicate that PHYs are ready
@@ -1158,6 +1165,8 @@  struct dwc3 {
 	struct usb_phy		*usb2_phy;
 	struct usb_phy		*usb3_phy;
 
+	u32			num_ports;
+	u32			num_ss_ports;
 	struct phy		*usb2_generic_phy;
 	struct phy		*usb3_generic_phy;