lkml.org 
[lkml]   [2023]   [May]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v7 3/9] usb: dwc3: core: Access XHCI address space temporarily to read port info
From


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@quicinc.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,

\
 
 \ /
  Last update: 2023-05-04 06:06    [W:0.122 / U:0.156 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site