Message-ID: <9c451432-a2ce-fdc1-d831-b7a5c9c731ab@quicinc.com>
Date:   Thu, 4 May 2023 09:35:28 +0530
From:   Krishna Kurapati PSSNV <quic_kriskura@...cinc.com>
To:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>
CC:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Felipe Balbi <balbi@...nel.org>,
        "Wesley Cheng" <quic_wcheng@...cinc.com>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "quic_pkondeti@...cinc.com" <quic_pkondeti@...cinc.com>,
        "quic_ppratap@...cinc.com" <quic_ppratap@...cinc.com>,
        "quic_jackp@...cinc.com" <quic_jackp@...cinc.com>,
        "quic_harshq@...cinc.com" <quic_harshq@...cinc.com>,
        "ahalaney@...hat.com" <ahalaney@...hat.com>,
        "quic_shazhuss@...cinc.com" <quic_shazhuss@...cinc.com>
Subject: Re: [PATCH v7 3/9] usb: dwc3: core: Access XHCI address space
 temporarily to read port info



On 5/4/2023 3:19 AM, Thinh Nguyen wrote:
> On Wed, May 03, 2023, Krishna Kurapati PSSNV wrote:
>>
>>
>> On 5/3/2023 3:11 AM, Thinh Nguyen wrote:
>>> Hi,
>>>
>>> On Mon, May 01, 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 usb2 ports and
>>>> usb3 ports present on multiport controller. Each USB Port is at least HS
>>>> capable.
>>>>
>>>> The port info for usb2 and usb3 phy are identified as num_usb2_ports
>>>> and num_usb3_ports. The intention is as follows:
>>>>
>>>> Wherever we need to perform phy operations like:
>>>>
>>>> LOOP_OVER_NUMBER_OF_AVAILABLE_PORTS()
>>>> {
>>>> 	phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST);
>>>> 	phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST);
>>>> }
>>>>
>>>> If number of usb2 ports is 3, loop can go from index 0-2 for
>>>> usb2_generic_phy. 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 (2 and 3)
>>>> are SS capable. So instead, num_usb2_ports is used to loop around all
>>>> phy's (both hs and ss) for performing phy operations. If any
>>>> usb3_generic_phy turns out to be NULL, phy operation just bails out.
>>>>
>>>> num_usb3_ports is used to modify GUSB3PIPECTL registers while setting up
>>>> phy's as we need to know how many SS capable ports are there for this.
>>>>
>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@...cinc.com>
>>>> ---
>>>>    drivers/usb/dwc3/core.c | 68 +++++++++++++++++++++++++++++++++++++++++
>>>>    drivers/usb/dwc3/core.h | 58 +++++++++++++++++++++++++++++++++++
>>>>    2 files changed, 126 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index 0beaab932e7d..b8ac7bcee391 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>> @@ -1767,6 +1767,59 @@ static int dwc3_get_clocks(struct dwc3 *dwc)
>>>>    	return 0;
>>>>    }
>>>> +static int dwc3_read_port_info(struct dwc3 *dwc)
>>>> +{
>>>> +	void __iomem		*regs;
>>>> +	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.
>>>> +	 */
>>>> +	regs = ioremap(dwc->xhci_resources[0].start,
>>>> +				resource_size(&dwc->xhci_resources[0]));
>>>> +	if (IS_ERR(regs))
>>>> +		return PTR_ERR(regs);
>>>> +
>>>> +	offset = dwc3_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_usb3_ports += XHCI_EXT_PORT_COUNT(temp);
>>>> +		} else if (major_revision <= 0x02) {
>>>> +			dwc->num_usb2_ports += XHCI_EXT_PORT_COUNT(temp);
>>>> +		} else {
>>>> +			dev_err(dwc->dev, "port revision seems wrong\n");
>>>
>>> Can we print this instead:
>>> 	dev_err(dwc->dev, "Unrecognized port major revision %d\n", major_revision);
>>>
>>>> +			ret = -EINVAL;
>>>> +			goto unmap_reg;
>>>> +		}
>>>> +
>>>> +		offset = dwc3_xhci_find_next_ext_cap(regs, offset,
>>>> +						XHCI_EXT_CAPS_PROTOCOL);
>>>> +	}
>>>> +
>>>> +	temp = readl(regs + DWC3_XHCI_HCSPARAMS1);
>>>> +	if (HCS_MAX_PORTS(temp) != (dwc->num_usb3_ports + dwc->num_usb2_ports)) {
>>>> +		dev_err(dwc->dev, "inconsistency in port info\n");
>>>
>>> Can we print this instead:
>>> 	dev_err(dwc->dev, "Mismatched reported MAXPORTS (%d)\n", HCS_MAX_PORTS(temp));
>>>
>>>> +		ret = -EINVAL;
>>>> +		goto unmap_reg;
>>>> +	}
>>>> +
>>>> +	dev_dbg(dwc->dev,
>>>> +		"hs-ports: %d ss-ports: %d\n", dwc->num_usb2_ports, dwc->num_usb3_ports);
>>>> +
>>>> +unmap_reg:
>>>> +	iounmap(regs);
>>>> +	return ret;
>>>> +}
>>>> +
>>>>    static int dwc3_probe(struct platform_device *pdev)
>>>>    {
>>>>    	struct device		*dev = &pdev->dev;
>>>> @@ -1774,6 +1827,7 @@ static int dwc3_probe(struct platform_device *pdev)
>>>>    	void __iomem		*regs;
>>>>    	struct dwc3		*dwc;
>>>>    	int			ret;
>>>> +	unsigned int		hw_mode;
>>>>    	dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
>>>>    	if (!dwc)
>>>> @@ -1843,6 +1897,20 @@ static int dwc3_probe(struct platform_device *pdev)
>>>>    			goto err_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);
>>>> +		if (ret)
>>>> +			goto err_disable_clks;
>>>> +	} else {
>>>> +		dwc->num_usb2_ports = 1;
>>>> +		dwc->num_usb3_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 d56457c02996..21312703e053 100644
>>>> --- a/drivers/usb/dwc3/core.h
>>>> +++ b/drivers/usb/dwc3/core.h
>>>> @@ -35,6 +35,17 @@
>>>>    #define DWC3_MSG_MAX	500
>>>> +/* Define XHCI Extcap register offsets for getting multiport info */
>>>> +#define XHCI_HCC_PARAMS_OFFSET	0x10
>>>> +#define DWC3_XHCI_HCSPARAMS1	0x04
>>>> +#define XHCI_EXT_CAPS_PROTOCOL	2
>>>> +#define XHCI_HCC_EXT_CAPS(x)    (((x) >> 16) & 0xffff)
>>>> +#define XHCI_EXT_CAPS_ID(x)     (((x) >> 0) & 0xff)
>>>> +#define XHCI_EXT_CAPS_NEXT(x)   (((x) >> 8) & 0xff)
>>>> +#define XHCI_EXT_PORT_MAJOR(x)  (((x) >> 24) & 0xff)
>>>> +#define XHCI_EXT_PORT_COUNT(x)  (((x) >> 8) & 0xff)
>>>> +#define HCS_MAX_PORTS(x)        (((x) >> 24) & 0x7f)
>>>> +
>>>>    /* Global constants */
>>>>    #define DWC3_PULL_UP_TIMEOUT	500	/* ms */
>>>>    #define DWC3_BOUNCE_SIZE	1024	/* size of a superspeed bulk */
>>>> @@ -1025,6 +1036,8 @@ struct dwc3_scratchpad_array {
>>>>     * @usb_psy: pointer to power supply interface.
>>>>     * @usb2_phy: pointer to USB2 PHY
>>>>     * @usb3_phy: pointer to USB3 PHY
>>>> + * @num_usb2_ports: number of usb2 ports.
>>>> + * @num_usb3_ports: number of usb3 ports.
>>>>     * @usb2_generic_phy: pointer to USB2 PHY
>>>>     * @usb3_generic_phy: pointer to USB3 PHY
>>>>     * @phys_ready: flag to indicate that PHYs are ready
>>>> @@ -1162,6 +1175,9 @@ struct dwc3 {
>>>>    	struct usb_phy		*usb2_phy;
>>>>    	struct usb_phy		*usb3_phy;
>>>> +	u32			num_usb2_ports;
>>>> +	u32			num_usb3_ports;
>>>
>>> can we use u8?
>>>
>>>> +
>>>>    	struct phy		*usb2_generic_phy;
>>>>    	struct phy		*usb3_generic_phy;
>>>> @@ -1650,4 +1666,46 @@ static inline void dwc3_ulpi_exit(struct dwc3 *dwc)
>>>>    { }
>>>>    #endif
>>>> +/**
>>>> + * dwc3_xhci_find_next_ext_cap - Find the offset of the extended capabilities
>>>> + *					with capability ID id.
>>>> + *
>>>> + * @base	PCI MMIO registers base address.
>>>> + * @start	address at which to start looking, (0 or HCC_PARAMS to start at
>>>> + *		beginning of list)
>>>> + * @id		Extended capability ID to search for, or 0 for the next
>>>> + *		capability
>>>
>>> I know that this is a duplicate from the xhci driver, but can we fix the
>>> kerneldoc style as in other places if we're going to keep it?
>>>
>>
>> Hi Thinh,
>>
>>   Isn't this same as other functions ?
>>
>> /**
>>   * <function name> - description
>>   * @params
>>   *
>>   */
>>
>> I missed the function name in comments last time, but added it in this
>> version.
>>
>> Checkpatch too didn't give any errors/warnings other than alignment. Can you
>> help point out any other mistake in this function doc/comments so that I can
>> fix it in next version.
>>
> 
> It's missing ":" after @params.
> 
> You can use the kernel-doc script to check for documentation's
> errors/warnings.
> 
> Here's the output if you run the following command:
> # <KERNEL_SRC>/scripts/kernel-doc -none -Werror -function dwc3_xhci_find_next_ext_cap drivers/usb/dwc3/core.h
> 
> drivers/usb/dwc3/core.h:1684: warning: Function parameter or member 'base' not described in 'dwc3_xhci_find_next_ext_cap'
> drivers/usb/dwc3/core.h:1684: warning: Function parameter or member 'start' not described in 'dwc3_xhci_find_next_ext_cap'
> drivers/usb/dwc3/core.h:1684: warning: Function parameter or member 'id' not described in 'dwc3_xhci_find_next_ext_cap'
> 3 warnings as Errors
> 
> 
> This is a minor style issue. While I don't think it's strictly needed to
> follow the kerneldoc style for a non-api function. But if we do follow
> it, we should do it correctly.
> 
> Thanks,
> Thinh

Thanks Thinh for pointing it out. Will fix this in the next version.

Regards,
Krishna,

Powered by blists - more mailing lists