Message-ID: <966c1001-6d64-9163-0c07-96595156fc8c@quicinc.com>
Date:   Sat, 11 Mar 2023 08:34:04 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.12.0
Subject: Re: [PATCH 2/8] usb: dwc3: core: Access XHCI address space
 temporarily to read port info
Content-Language: en-US
From:   Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
To:     Thinh Nguyen <Thinh.Nguyen@synopsys.com>
CC:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Felipe Balbi <balbi@kernel.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-arm-msm@vger.kernel.org" <linux-arm-msm@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "quic_pkondeti@quicinc.com" <quic_pkondeti@quicinc.com>,
        "quic_ppratap@quicinc.com" <quic_ppratap@quicinc.com>,
        "quic_wcheng@quicinc.com" <quic_wcheng@quicinc.com>,
        "quic_jackp@quicinc.com" <quic_jackp@quicinc.com>,
        "quic_harshq@quicinc.com" <quic_harshq@quicinc.com>,
        "ahalaney@redhat.com" <ahalaney@redhat.com>,
        "quic_shazhuss@quicinc.com" <quic_shazhuss@quicinc.com>
References: <20230310163420.7582-1-quic_kriskura@quicinc.com>
 <20230310163420.7582-3-quic_kriskura@quicinc.com>
 <20230310235537.afl76rs3pcifbn46@synopsys.com>
 <c23453b3-4f91-aa0a-c45a-de99ac369048@quicinc.com>
In-Reply-To: <c23453b3-4f91-aa0a-c45a-de99ac369048@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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