Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <ac5651148d4ab2b243597af9d42fa0e374d810a8.camel@linux.intel.com>
Subject: Re: [PATCH] platform/x86/intel-uncore-freq: Uncore frequency
 control via TPMI
From:   srinivas pandruvada <srinivas.pandruvada@linux.intel.com>
To:     Hans de Goede <hdegoede@redhat.com>, markgross@kernel.org
Cc:     platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org
Date:   Thu, 06 Apr 2023 11:43:50 -0700
In-Reply-To: <9ea89b86-c027-5a73-7ccb-0c3388b31f5f@redhat.com>
References: <20230328215328.101990-1-srinivas.pandruvada@linux.intel.com>
         <9ea89b86-c027-5a73-7ccb-0c3388b31f5f@redhat.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.4 (3.42.4-2.fc35) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Hans,

> > 

[...]

> > The TPMI documentation can be downloaded from:
> > https://github.com/intel/tpmi_power_management
> > 
> > Signed-off-by: Srinivas Pandruvada
> > <srinivas.pandruvada@linux.intel.com>
> 
> Thanks 3 small remarks below (inline), if you can address those then
> v2 should be ready for merging.
Sure. Will send an update.

> 
> > ---
> > The fine grain control at cluster level patches are here:
> > https://github.com/spandruvada/linux-kernel/commit/0d66ea4ff76ea19127f2d207a7e17bb86846ca32
> > https://github.com/spandruvada/linux-kernel/commit/cb5c2349a58318c04955821d6528cc8015541e65
> > But not submitting to ease in review process as I posted too many
> > patches this cycle.
> 
> As mentioned before, it helps if you can get fellow Intel folks to
> review.
I will add to the next version with reviewed-by and tested-by.

> 
> >  .../x86/intel/uncore-frequency/Kconfig        |   4 +
> >  .../x86/intel/uncore-frequency/Makefile       |   2 +
> >  .../uncore-frequency/uncore-frequency-tpmi.c  | 346
> > ++++++++++++++++++
> > 

[...]

> > +/* Information for each power domain */
> > +struct tpmi_uncore_power_domain_info {
> > +       void __iomem *uncore_base;
> 
> Maybe make this an:
> 
>         u8 __iomem *uncore_base;
> 
> To avoid all the casts below ?
> 
> Although I wonder if the casts are necessary at all, AFAIK void *
> arithmics are the same as u8 * arithmics, so things should work
> without the casts regardless ?
> 
You are right. I already don't cast in SST, I missed this one.

> Still just turning this into a u8 * is probably better.
I will change to u8*

> > 

[...]


> > +               pd_info->uncore_base =
> > devm_ioremap_resource(&auxdev->dev, res);
> > +               if (IS_ERR(pd_info->uncore_base)) {
> > +                       ret = PTR_ERR(pd_info->uncore_base);
> > +                       pd_info->uncore_base = NULL;
> 
> pd_info is part of tpmi_uncore and on error the:
> 
>         auxiliary_set_drvdata(auxdev, tpmi_uncore);
> 
> call is skipped, so pd_info will never be reachable (and should get
> freed by devm).
> So AFAICT, there is no need to NULL uncore_base  ?

Correct. Let me think of case when firmware messed up one power domain
base address (very unlikely as this is goes through several validation
levels), may be continue to check next power domain instead of return.

> 
> > +                       goto err_rem_common;
> > +               }
> > +
> > +               /* Check for version and skip this resource if
> > there is mismatch */
> > +               header = readq(pd_info->uncore_base);
> > +               pd_info->ufs_header_ver = header &
> > UNCORE_GENMASK_VERSION;
> > +               if (pd_info->ufs_header_ver !=
> > UNCORE_HEADER_VERSION) {
> > +                       dev_info(&auxdev->dev, "Uncore: Unsupported
> > version:%d\n",
> > +                               pd_info->ufs_header_ver);
> > +                       continue;
> > +               }
> > +
> > +               /* Get Cluster ID Mask */
> > +               cluster_mask =
> > FIELD_GET(UNCORE_LOCAL_FABRIC_CLUSTER_ID_MASK, header);
> > +               if (!cluster_mask) {
> > +                       dev_info(&auxdev->dev, "Uncore: Invalid
> > cluster mask:%x\n", cluster_mask);
> > +                       continue;
> > +               }
> > +
> > +               /* Find out number of clusters in this resource */
> > +               mask = 0x01;
> > +               for (j = 0; j < UNCORE_MAX_CLUSTER_PER_DOMAIN; ++j)
> > {
> > +                       if (cluster_mask & mask)
> > +                               pd_info->cluster_count++;
> > +                       mask <<= 1;
> > +               }
> > +
> > +               pd_info->cluster_infos = devm_kcalloc(&auxdev->dev,
> > pd_info->cluster_count,
> > +                                                     sizeof(struct
> > tpmi_uncore_cluster_info),
> > +                                                     GFP_KERNEL);
> 
> This devm_kcalloc() call is missing error checking.
Thanks for pointing.

Thanks,
Srinivas

> 
> > +
> > +               /*
> > +                * Each byte in the register point to status and
> > control
> > +                * registers belonging to cluster id 0-8.
> > +                */
> > +               cluster_offset = readq((u8 __iomem *)pd_info-
> > >uncore_base +
> > +                                       UNCORE_FABRIC_CLUSTER_OFFSE
> > T);
> > +
> > +               for (j = 0; j < pd_info->cluster_count; ++j) {
> > +                       struct tpmi_uncore_cluster_info
> > *cluster_info;
> > +
> > +                       /* Get the offset for this cluster */
> > +                       mask = (cluster_offset &
> > UNCORE_CLUSTER_OFF_MASK);
> > +                       /* Offset in QWORD, so change to bytes */
> > +                       mask <<= 3;
> > +
> > +                       cluster_info = &pd_info->cluster_infos[j];
> > +
> > +                       cluster_info->cluster_base = (u8 __iomem
> > *)pd_info->uncore_base + mask;
> > +
> > +                       cluster_info->uncore_data.package_id = pkg;
> > +                       /* There are no dies like Cascade Lake */
> > +                       cluster_info->uncore_data.die_id = 0;
> > +
> > +                       /* Point to next cluster offset */
> > +                       cluster_offset >>=
> > UNCORE_MAX_CLUSTER_PER_DOMAIN;
> > +               }
> > +       }
> > +
> > +       auxiliary_set_drvdata(auxdev, tpmi_uncore);
> > +
> > +       tpmi_uncore->root_cluster.uncore_root = tpmi_uncore;
> > +       tpmi_uncore->root_cluster.uncore_data.package_id = pkg;
> > +       ret = uncore_freq_add_entry(&tpmi_uncore-
> > >root_cluster.uncore_data, 0);
> > +       if (ret)
> > +               goto err_rem_common;
> > +
> > +       return 0;
> > +
> > +err_rem_common:
> > +       uncore_freq_common_exit();
> > +
> > +       return ret;
> > +}
> > +
> > +static int tpmi_uncore_remove(struct auxiliary_device *auxdev)
> > +{
> > +       struct tpmi_uncore_struct *tpmi_uncore =
> > auxiliary_get_drvdata(auxdev);
> > +
> > +       uncore_freq_remove_die_entry(&tpmi_uncore-
> > >root_cluster.uncore_data);
> > +
> > +       uncore_freq_common_exit();
> > +
> > +       return 0;
> > +}
> > +
> > +static int intel_uncore_probe(struct auxiliary_device *auxdev,
> > const struct auxiliary_device_id *id)
> > +{
> > +       return tpmi_uncore_init(auxdev);
> > +}
> > +
> > +static void intel_uncore_remove(struct auxiliary_device *auxdev)
> > +{
> > +       tpmi_uncore_remove(auxdev);
> > +}
> > +
> > +static const struct auxiliary_device_id intel_uncore_id_table[] =
> > {
> > +       { .name = "intel_vsec.tpmi-uncore" },
> > +       {}
> > +};
> > +MODULE_DEVICE_TABLE(auxiliary, intel_uncore_id_table);
> > +
> > +static struct auxiliary_driver intel_uncore_aux_driver = {
> > +       .id_table       = intel_uncore_id_table,
> > +       .remove         = intel_uncore_remove,
> > +       .probe          = intel_uncore_probe,
> > +};
> > +
> > +module_auxiliary_driver(intel_uncore_aux_driver);
> > +
> > +MODULE_IMPORT_NS(INTEL_TPMI);
> > +MODULE_IMPORT_NS(INTEL_UNCORE_FREQUENCY);
> > +MODULE_DESCRIPTION("Intel TPMI UFS Driver");
> > +MODULE_LICENSE("GPL");
> 
> 
> Regards,
> 
> Hans
> 
>