From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0DE7EEB64DB for ; Wed, 14 Jun 2023 10:59:17 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235907AbjFNK7P (ORCPT ); Wed, 14 Jun 2023 06:59:15 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:59278 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234356AbjFNK7N (ORCPT ); Wed, 14 Jun 2023 06:59:13 -0400 Received: from mail-wr1-x42a.google.com (mail-wr1-x42a.google.com [IPv6:2a00:1450:4864:20::42a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id AB2131BC3 for ; Wed, 14 Jun 2023 03:59:10 -0700 (PDT) Received: by mail-wr1-x42a.google.com with SMTP id ffacd0b85a97d-30e4eec95c8so6127799f8f.3 for ; Wed, 14 Jun 2023 03:59:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1686740349; x=1689332349; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=X76GkVQmm233Ef9CD45/5gbsHuJzJYm1L85f82G9R4o=; b=YC6D/9YorHI2DGemUAj/35LGIrIk1d7W7kFw5L0NFtynGJLSkwhdHBHTSkjoAWF6Hw pId1fTQQnxrw3IOIqwOPq4EYitLlCm4eclIXFwsCwdUT42yWTcdUPCNPSqVSLogvk1iU xduZ2AUrOfBKMsWgPwR20HTGWegtCwfw5LRXu0ASMVU6Gl/0ltjm7D9BezzW72ukdkUj vMiMK+9R2Qry+vuJ+0zUaYnKowG7HXx3jvRvpsXxISRg5C6jmrZ+zK0qBpdjVpM33kY0 fqFpfi058mntucNZ7KuAVG+aRmJWOKJxK2XdB2gecXB08r68rF+/Y/Rfxr6iVsBI6grQ zhXQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1686740349; x=1689332349; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=X76GkVQmm233Ef9CD45/5gbsHuJzJYm1L85f82G9R4o=; b=XcI6sQ2AzTxkM/OoeLAnuyX4ZERXSleQ+MXz76AnobufBZgis5BMnWyL0JKSpWW2JA uGdF5B1cRYotf5G+Jd76c4oUD8NmXM4TX0nms/UzbkxybByu5xz+QFf6PKPe5MFXQrDa J7QYERR+31sCUWprSqfKMygyadu/IuIuvZuWndgEFxa3iHn06ITe7f8AANqVYPADBmro JmYn09B7DVZraBUWx5Z9Ba/hAsu4PQ4eEV2fmZkZADqJ8it4h+lCnQIQz6jqQW/f8ihW m1jqMbXxhqU1jwxSyneNXrtt6HAY/uUSyfugCkMwYdRfFH15Ee9PjIUVIDi4ttT9sULA gfDA== X-Gm-Message-State: AC+VfDwDePqbB9pRa/+2nDq/TMpkprHCtiBTWlw69eKYjqHsLNXeNo7A xF7jcSiGh1Pcu4BldSp02B32NA== X-Google-Smtp-Source: ACHHUZ4osks2idP8nFHnF77UpFY2nhvaTzkrwZUVtXTupb7ex/nT+3I5JfmY4knyoi0MhzfpCxOZhg== X-Received: by 2002:adf:e4c8:0:b0:30f:c9bf:de69 with SMTP id v8-20020adfe4c8000000b0030fc9bfde69mr4851063wrm.46.1686740349081; Wed, 14 Jun 2023 03:59:09 -0700 (PDT) Received: from ?IPV6:2a05:6e02:1041:c10:d8c4:17ec:c49d:215e? ([2a05:6e02:1041:c10:d8c4:17ec:c49d:215e]) by smtp.googlemail.com with ESMTPSA id n7-20020adff087000000b0030ae5a0516csm17816540wro.17.2023.06.14.03.59.08 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 14 Jun 2023 03:59:08 -0700 (PDT) Message-ID: Date: Wed, 14 Jun 2023 12:59:07 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: [PATCH v14 1/2] thermal: loongson-2: add thermal management support Content-Language: en-US To: zhuyinbo , "Rafael J . Wysocki" , Amit Kucheria , Zhang Rui , Rob Herring , Krzysztof Kozlowski , linux-pm@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Cc: Jianmin Lv , wanghongliang@loongson.cn, Liu Peibao , loongson-kernel@lists.loongnix.cn, zhanghongchen References: <20230426062018.19755-1-zhuyinbo@loongson.cn> From: Daniel Lezcano In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org Hi Yinbo, On 14/06/2023 10:03, zhuyinbo wrote: > > Hi Daniel, > > Thank you very much for your feedback and suggestions. Below, I have > some comments, please review. [ ... ] >>> + >>> +    low += 100; >>> +    high += 100; Literals -> macros >>> +    reg_ctrl = low; >>> +    reg_ctrl |= enable ? 0x100 : 0; >>> +    writew(reg_ctrl, data->regs + LOONGSON2_TSENSOR_CTRL_LO + reg_off); >>> + >>> +    reg_ctrl = high; >>> +    reg_ctrl |= enable ? 0x100 : 0; >>> +    writew(reg_ctrl, data->regs + LOONGSON2_TSENSOR_CTRL_HI + reg_off); >> >> Is the 'enable' boolean really useful? > > > Yes, this 'enable' was to enable thermal irq. > >> >> Wouldn't be the sensor trip points disabled by default at reset time? >> > > > Only here will thermal irq be enabled throughout the entire driver, and > actual testing has shown that interrupts are valid, so this is > meaningful. Ok. >> If it is the case then we can get ride of this variable and make the >> routine simpler >> >>> +    return 0; >>> +} >>> + >>> +static int loongson2_thermal_get_temp(struct thermal_zone_device >>> *tz, int *temp) >>> +{ >>> +    u32 reg_val; >>> +    struct loongson2_thermal_data *data = tz->devdata; >>> + >>> +    reg_val = readl(data->regs + LOONGSON2_TSENSOR_OUT); >> >> Seems like there is no offset for the sensor id here ? > > > There is no need for a sensor ID here. > > There are some things that I didn't describe clearly, which made you > misunderstand. Actually, the temperature sensor of 2K1000 is like this: > > There are 4 sets of temperature interrupt controllers, only one set of > temperature sampling registers. a sets of temperature interrupt > controllers was considered a sensor, which sensor include 3 register as > follows, where "SEL" represents which sensor is referenced, In 2k1000 > datasheet, which "SEL" must be 0. I'm not sure to understand. Let me rephrase it and know what is wrong. 1. The thermal controller has 4 sensors. The interrupt can be set for these 4 sensors. 2. When reading a temperature, we have to select the sensor via the 'SEL' register. 3. The 2k1000 has one sensor with an id = 0. 4. In the future, more Loongson platform can be submitted with more than one sensor If this is correct, then my comments are about the inconsistency of the proposed changes. Guessing in the future Loongson board there will be more than one sensor, the existing code mixes support for one and multiple sensors as well as assuming id is 0. So if you add in the of_loongson2_thermal_match table a new platform with several sensors, the current code will be broken because: - the initialization loop does exit when the first thermal zone registration succeed - the interrupt handler does not figure out which sensor crossed the low/high limit - the get_temp is not selecting the right sensor That is my point: - write the code to support one sensor with id=0 only *or* - write the code to support multiple sensors If I'm not wrong the code is closer to support multiple sensors ;) Let me know if these deductions are correct -- Daniel ps : is there an English translation for the 2k1000 datasheet ? -- Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog