Messages in this thread | | | Date | Thu, 2 Nov 2023 09:50:38 +0800 | Subject | Re: [PATCH v2 2/8] coresight-tpda: Add support to configure CMB element | From | Tao Zhang <> |
| |
On 11/1/2023 7:36 PM, James Clark wrote: > > > On 01/11/2023 08:53, Tao Zhang wrote: >> >> On 10/30/2023 7:11 PM, James Clark wrote: >>> >>> On 25/10/2023 03:53, Tao Zhang wrote: >>>> Read the CMB element size from the device tree. Set the register >>>> bit that controls the CMB element size of the corresponding port. >>>> >>>> Signed-off-by: Tao Zhang<quic_taozha@quicinc.com> >>>> Signed-off-by: Mao Jinlong<quic_jinlmao@quicinc.com> >>>> --- >>>> drivers/hwtracing/coresight/coresight-tpda.c | 108 >>>> ++++++++++++++++----------- >>>> drivers/hwtracing/coresight/coresight-tpda.h | 6 ++ >>>> 2 files changed, 69 insertions(+), 45 deletions(-) >>>> >>>> diff --git a/drivers/hwtracing/coresight/coresight-tpda.c >>>> b/drivers/hwtracing/coresight/coresight-tpda.c >>>> index 5f82737..3101d2a 100644 >>>> --- a/drivers/hwtracing/coresight/coresight-tpda.c >>>> +++ b/drivers/hwtracing/coresight/coresight-tpda.c >>>> @@ -28,24 +28,54 @@ static bool coresight_device_is_tpdm(struct >>>> coresight_device *csdev) >>>> CORESIGHT_DEV_SUBTYPE_SOURCE_TPDM); >>>> } >>>> +static void tpdm_clear_element_size(struct coresight_device *csdev) >>>> +{ >>>> + struct tpda_drvdata *drvdata = >>>> dev_get_drvdata(csdev->dev.parent); >>>> + >>>> + if (drvdata->dsb_esize) >>>> + drvdata->dsb_esize = 0; >>>> + if (drvdata->cmb_esize) >>>> + drvdata->cmb_esize = 0; > > The ifs don't do anything here, it's just the same as always setting > to 0. I will update in the next patch series. > >>>> +} >>>> + >>>> +static void tpda_set_element_size(struct tpda_drvdata *drvdata, u32 >>>> *val) >>>> +{ >>>> + >>>> + if (drvdata->dsb_esize == 64) >>>> + *val |= TPDA_Pn_CR_DSBSIZE; >>>> + else if (drvdata->dsb_esize == 32) >>>> + *val &= ~TPDA_Pn_CR_DSBSIZE; >>>> + >>>> + if (drvdata->cmb_esize == 64) >>>> + *val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x2); >>>> + else if (drvdata->cmb_esize == 32) >>>> + *val |= FIELD_PREP(TPDA_Pn_CR_CMBSIZE, 0x1); >>>> + else if (drvdata->cmb_esize == 8) >>>> + *val &= ~TPDA_Pn_CR_CMBSIZE; >>>> +} >>>> + >>>> /* >>>> * Read the DSB element size from the TPDM device >>>> * Returns >>>> * The dsb element size read from the devicetree if available. >>> Hi Tao, >>> >>> I think the function description and the return value description above >>> need updating now. >> I will update this in the next patch series. >>> >>>> * 0 - Otherwise, with a warning once. >>>> */ >>>> -static int tpdm_read_dsb_element_size(struct coresight_device *csdev) >>>> +static int tpdm_read_element_size(struct tpda_drvdata *drvdata, >>>> + struct coresight_device *csdev) >>>> { >>>> - int rc = 0; >>>> - u8 size = 0; >>>> - >>>> - rc = fwnode_property_read_u8(dev_fwnode(csdev->dev.parent), >>>> - "qcom,dsb-element-size", &size); >>>> + int rc = -EEXIST; >>>> + >>>> + if (!fwnode_property_read_u8(dev_fwnode(csdev->dev.parent), >>>> + "qcom,dsb-element-size", &drvdata->dsb_esize)) >>>> + rc &= 0; >>> Is &= 0 significant? Why not = 0? >> I will update this in the next patch series. >>> >>>> + if (!fwnode_property_read_u8(dev_fwnode(csdev->dev.parent), >>>> + "qcom,cmb-element-size", &drvdata->cmb_esize)) >>>> + rc &= 0; >>>> if (rc) >>>> dev_warn_once(&csdev->dev, >>>> - "Failed to read TPDM DSB Element size: %d\n", rc); >>>> + "Failed to read TPDM Element size: %d\n", rc); >>>> - return size; >>>> + return rc; >>>> } >>>> /* >>>> @@ -56,13 +86,17 @@ static int tpdm_read_dsb_element_size(struct >>>> coresight_device *csdev) >>>> * Parameter "inport" is used to pass in the input port number >>>> * of TPDA, and it is set to -1 in the recursize call. >>>> */ >>>> -static int tpda_get_element_size(struct coresight_device *csdev, >>>> +static int tpda_get_element_size(struct tpda_drvdata *drvdata, >>>> + struct coresight_device *csdev, >>>> int inport) >>>> { >>>> - int dsb_size = -ENOENT; >>>> - int i, size; >>>> + int rc = -ENOENT; >>>> + int i; >>>> struct coresight_device *in; >>>> + if (inport > 0) >>>> + tpdm_clear_element_size(csdev); >>>> + >>>> for (i = 0; i < csdev->pdata->nr_inconns; i++) { >>>> in = csdev->pdata->in_conns[i]->src_dev; >>>> if (!in) >>>> @@ -74,25 +108,20 @@ static int tpda_get_element_size(struct >>>> coresight_device *csdev, >>>> continue; >>>> if (coresight_device_is_tpdm(in)) { >>>> - size = tpdm_read_dsb_element_size(in); >>>> + if (rc) >>>> + rc = tpdm_read_element_size(drvdata, in); >>>> + else >>>> + return -EINVAL; >>> This is quite hard to follow, is checking rc here before calling >>> tpdm_read_element_size() some kind of way of only calling it once? >> >> Yes, there can only be one TPDM on one input port of TPDA. If >> "tpdm_read_element_size" is called >> >> twice, it means that two TPDMs are found on one input port of TPDA. >> >>> rc isn't actually a return value at this point, it's just default >>> initialised to -ENOENT. >> >> In this loop, "tpdm_read_element_size" will be called for the first time >> TPDM found. If rc equals to >> >> 0, it means that at lease one TPDM has been found on the input port. >> Does it make sense? >> >>> Then it's not clear why the else condition returns an error? >> The same as the above. >>> >>>> } else { >>>> /* Recurse down the path */ >>>> - size = tpda_get_element_size(in, -1); >>>> - } >>>> - >>>> - if (size < 0) >>>> - return size; >>>> - >>>> - if (dsb_size < 0) { >>>> - /* Found a size, save it. */ >>>> - dsb_size = size; >>>> - } else { >>>> - /* Found duplicate TPDMs */ >>>> - return -EEXIST; >>>> + rc = tpda_get_element_size(drvdata, in, -1); >>> And then why it's assigned here but not checked for an error in this >>> case? >> >> |Remote ETM| |TPDM| >> >> | branch0 | branch1 >> >> -------------------------- >> >> funnel1 >> >> --------------------------- >> >> | input port 1 >> >> ----------------------------- >> >> TPDA1 >> >> ----------------------------- >> >> The branches on some TPDA's input ports may not have TPDM. For example, >> branch 0 doesn't has a >> >> TPDM connected, tpda_get_element_size will not return 0 here. This is a >> normal situation. We need to >> >> continue to find TPDM on branch1 through recursion. >> >>> >>> Maybe some comments explaining the flow would help. Or to me it seems >>> like a second variable to track the thing that's actually intended >>> could >>> be used as well. Like "bool found_element" or something, rather than >>> re-using rc to also track another thing. >> >> Do you prefer to use "static bool found_element" to mark if we already >> have read an element size in >> >> the recursion function? >> > > You could add a static, or you could use whether a set dsb or cmb size > exists to mark that one was found, which I think is already partially > done. > > My confusion comes from the fact that instead of just a recursive > search, the function doesn't stop at the first found one, it continues > to sanity check that there is only one connected across all ports. > > And instead of just checking the error condition at the very end, it > does it mid recursion, relying on the rc value from a previous > iteration. IMO the following is a simplification because it always > returns at the first error found, and checks the final condition outside > of the recursive function. But you could probably leave it as you have > but with some comments explaining why: > > > diff --git a/drivers/hwtracing/coresight/coresight-tpda.c > b/drivers/hwtracing/coresight/coresight-tpda.c > index 3101d2a0671d..00b7607d36be 100644 > --- a/drivers/hwtracing/coresight/coresight-tpda.c > +++ b/drivers/hwtracing/coresight/coresight-tpda.c > @@ -90,13 +90,10 @@ static int tpda_get_element_size(struct > tpda_drvdata *drvdata, > struct coresight_device *csdev, > int inport) > { > - int rc = -ENOENT; > + int rc = 0; > int i; > struct coresight_device *in; > > - if (inport > 0) > - tpdm_clear_element_size(csdev); > - > for (i = 0; i < csdev->pdata->nr_inconns; i++) { > in = csdev->pdata->in_conns[i]->src_dev; > if (!in) > @@ -108,19 +105,23 @@ static int tpda_get_element_size(struct > tpda_drvdata *drvdata, > continue; > > if (coresight_device_is_tpdm(in)) { > - if (rc) > - rc = tpdm_read_element_size(drvdata, in); > - else > + if ((drvdata->dsb_esize) || > (drvdata->cmb_esize)) { > + /* Error if already previously found > and initialised one */ > + dev_warn_once(&drvdata->csdev->dev, > + "Detected multiple TPDMs on > port %d", -EEXIST); > return -EINVAL; > + } > + rc = tpdm_read_element_size(drvdata, in); > + if (rc) > + return rc; > } else { > /* Recurse down the path */ > rc = tpda_get_element_size(drvdata, in, -1); > + if (rc) > + return rc; > } > } > > - if ((drvdata->dsb_esize) || (drvdata->cmb_esize)) > - rc = 0; > - > return rc; > } > > @@ -146,15 +147,17 @@ static int tpda_enable_port(struct tpda_drvdata > *drvdata, int port) > * Set the bit to 0 if the size is 32 > * Set the bit to 1 if the size is 64 > */ > + tpdm_clear_element_size(drvdata->csdev); > rc = tpda_get_element_size(drvdata, drvdata->csdev, port); > - if (!rc) { > + if (!rc && ((drvdata->dsb_esize) || (drvdata->cmb_esize))) > + { > tpda_set_element_size(drvdata, &val); > /* Enable the port */ > val |= TPDA_Pn_CR_ENA; > writel_relaxed(val, drvdata->base + TPDA_Pn_CR(port)); > - } else if (rc == -EINVAL) { > - dev_warn_once(&drvdata->csdev->dev, > - "Detected multiple TPDMs on port %d", -EEXIST); > + } else { > + dev_warn_once(&drvdata->csdev->dev, "Didn't find TPDM > elem size"); > + rc = -EINVAL; > } > > return rc;
Make sense. I will refine and update the code in the next patch series.
Best,
Tao
> >
| |