Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965125AbeAJLgI (ORCPT + 1 other); Wed, 10 Jan 2018 06:36:08 -0500 Received: from ispman.iskranet.ru ([62.213.33.10]:53870 "EHLO ispman.iskranet.ru" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964819AbeAJLgG (ORCPT ); Wed, 10 Jan 2018 06:36:06 -0500 Subject: Re: [RESEND, 4/4] i2c: mpc: always determine I2C clock prescaler at runtime To: Wolfram Sang , Arseny Solokha Cc: linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org, Valentin Longchamp References: <20171207102003.23496-5-asolokha@kb.kras.ru> <20171230181446.bxongiaogcqjrbos@ninjato> From: Arseny Solokha Message-ID: Date: Wed, 10 Jan 2018 18:36:00 +0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: <20171230181446.bxongiaogcqjrbos@ninjato> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: >> /* >> * Map and check POR Device Status Register 2 >> - * (PORDEVSR2) at 0xE0014 >> + * (PORDEVSR2) at 0xE0014. Note than while MPC8533 >> + * and MPC8544 indicate SEC frequency ratio >> + * configuration as bit 26 in PORDEVSR2, other MPC8xxx >> + * parts may store it differently or may not have it >> + * at all. > > So given this comment which you added... > >> */ >> reg = ioremap(get_immrbase() + *prop + 0x14, 0x4); >> if (!reg) >> printk(KERN_ERR >> "Error: couldn't map PORDEVSR2\n"); >> else >> - val = in_be32(reg) & 0x00000080; /* sec-cfg */ >> + val = in_be32(reg) & 0x00000020; /* sec-cfg */ > > ... are you really sure there is no ancient device which needs the > 0x00000080? Various MPC SoCs indeed have different bit layout of PORDEVSR2. Obviously not all of them indicate SEC frequency ratio configuration at all, either in PORDEVSR2 or in some other register, as not all SoCs contain SEC engine. In this regard, mpc_i2c_get_sec_cfg_8xxx() is poorly named as it in its current form is only applicable to a few SoCs from the whole MPC8xxx family. mpc_i2c_get_sec_cfg_8xxx() is only called from the following snippet: else if ((SVR_SOC_VER(svr) == SVR_8533) || (SVR_SOC_VER(svr) == SVR_8544)) /* the above 85xx SoCs have prescaler 3 or 2 */ prescaler = mpc_i2c_get_sec_cfg_8xxx() ? 3 : 2; Both MPC8533 and MPC8544 do in fact indicate SEC frequency ratio configuration as bit 26 in PORDEVSR2 according to [1,2], so I've added the comment as a precaution for future uses. I can also rename the function to something like mpc_i2c_get_sec_cfg_8533_8544(), which looks ugly and doesn't scale. AFAICS, mask 0x00000080 in the code clearly contradicts to what I read in the docs. [1] https://www.nxp.com/docs/en/reference-manual/MPC8533ERM.pdf [2] https://www.nxp.com/docs/en/reference-manual/MPC8544ERM.pdf Arsény