On 2025/2/21 21:14, Sumit Gupta wrote: > > > On 19/02/25 00:53, Rafael J. Wysocki wrote: >> >> There seems to be some quite fundamental disagreement on how this >> should be done, so I'm afraid I cannot do much about it ATM. >> >> Please agree on a common approach and come back to me when you are ready. >> >> Sending two concurrent patchsets under confusingly similar names again >> and again isn't particularly helpful. >> >> Thanks! > > Hi Rafael, > > Thank you for looking into this. > > Hi Lifeng, > > As per the discussion, we can make the driver future extensible and > also can optimize the register read/write access. > > I gave some thought and below is my proposal. > > 1) Pick 'Patch 1-7' from your patch series [1] which optimize API's > to read/write a cpc register. > > 2) Pick my patches in [2]: > - Patch 1-4: Keep all cpc registers together under acpi_cppc sysfs. > Also, update existing API's to read/write regs in batch. > - Patch 5: Creates 'cppc_cpufreq_epp_driver' instance for booting > all CPU's in Auto mode and set registers with right values. > They can be updated after boot from sysfs to change hints to HW. > I can use the optimized API's from [1] where required in [2]. > > Let me know if you are okay with this proposal. > I can also send an updated patch series with all the patches combined? > > [1] https://lore.kernel.org/all/20250206131428.3261578-1-zhenglifeng1@xxxxxxxxxx/ > [2] https://lore.kernel.org/lkml/20250211103737.447704-1-sumitg@xxxxxxxxxx/ > > Regards, > Sumit Gupta > Hi Sumit, Over the past few days, I've been thinking about your proposal and scenario. I think we both agree that PATCH 1-7 in [1] doesn't conflicts with [2], so the rest of the discussion focuses on the differences between [2] and the PATCH 8 in [1]. We both tried to support autonomous selection mode in cppc_cpufreq but on different ways. I think the differences between these two approaches can be summarized into three questions: 1. Which sysfs files to expose? I think this is not a problem, we can keep all of them. 2. Where to expose these sysfs files? I understand your willing to keep all cpc registers together under acpi_cppc sysfs. But in my opinion, it is more suitable to expose them under cppc_cpufreq_attr, for these reasons: 1) It may probably introduce concurrency and data consistency issues, as I mentioned before. 2) The store functions call cpufreq_cpu_get() to get policy and update the driver_data which is a cppc_cpudata. Only the driver_data in cppc_cpufreq's policy is a cppc_cpudata! These operations are inappropriate in cppc_acpi. This file currently provides interfaces for cpufreq drivers to use. Reverse calls might mess up call relationships, break code structures, and cause problems that are hard to pinpoint the root cause! 3) Difficult to extend. Different cpufreq drivers may have different processing logic when reading from and writing to these CPC registers. Limiting all sysfs here makes it difficult for each cpufreq driver to extend. I think this is why there are only read-only interfaces under cppc_attrs before. Adding a 'ifdef' is not a good way to solve these problems. Defining this config does not necessarily mean that the cpufreq driver is cppc_cpufreq. 3. Is it necessary to add a new driver instance? [1] exposed the sysfs files to support users dynamically change the auto selection mode of each policy. Each policy can be operated seperately. It seems to me that if you want to boot all CPUs in auto mode, it should be sufficient to set all relevant registers to the correct values at boot time. I can't see why the new instance is necessary unless you explain it further. Could you explain more about why you add a new instance starting from answer these questions: For a specific CPU, what is the difference between using the two instances when auto_sel is 1? And what is the difference when auto_sel is 0? If it turns out that the new instance is necessary, I think we can reach a common approach by adding this new cpufreq driver instance and place the attributes in 'cppc_cpufreq_epp_attr', like amd-pstate did. What do you think? Regards, Lifeng
