Received-SPF: pass (google.com: domain of linux-kernel+bounces-205263-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Thu, 6 Jun 2024 22:14:27 -0500
From: Colin Foster <colin.foster@in-advantage.com>
To: Mark Brown <broonie@kernel.org>, linux-spi@vger.kernel.org,
	linux-kernel@vger.kernel.org, miquel.raynal@bootlin.com
Subject: Re: omap2-mcspi multi mode
Message-ID: <ZmJ7E305ow91ez2U@euler>
References: <Zl/V0dU6SjAMkpLG@colin-ia-desktop>
 <ZmFt7yfZFFJdsZuJ@localhost.localdomain>
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ZmFt7yfZFFJdsZuJ@localhost.localdomain>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?NUduZ0VJWkE4NVQ0RFkwbWl0bGJ5ZFdMaDRxVmNFZ1J6czYvVU5IWHVlN0Jy?=
 =?utf-8?B?d2E1TFBCZ1F4cHhEUmg4WXV4L2NqUU91Z2pvMGxGUmdBaENwMEFOeWVxUFFl?=
 =?utf-8?B?elBSSnVCcHozWWg3ZXFtVXMwcXZLYm9NdlJTNCs2V1gxejJvNmZFeGRYM3Zx?=
 =?utf-8?B?d2d1WENsR0pRZkZLVGdNeitPR0VUYXVlY0JtVngyRmxSeExlQnhvTE1RTnZm?=
 =?utf-8?B?Mk9nK0pJUWZrU0FaaytiMGVrZnVtV0doVjJJMXRzTXNoTURIV2lmblhLRHhV?=
 =?utf-8?B?Rno2TGVsczhUVFF6T080N0hVMjZlL2tZdG45cWhZeUFxcDVSVFplbSt0NkNV?=
 =?utf-8?B?RkpaNGJnb0E2aFlCSHVVT1IyKzJYdmxRbmFsNDRQYVl2Ty8rbXNPVGh5ZkJx?=
 =?utf-8?B?Rmlpd3FvT2drNjJ5R1FuWi9ObG9OLzBFak5rZEZ3YitNbnhFTFQ4RDN0Wlkz?=
 =?utf-8?B?SnF5eDNta21QeVdTaVRQWXdHbkJBaEJncVgraXJvYkl5VENzemhoei9GNjhY?=
 =?utf-8?B?SGFRSFFJZmI4YTcrNVpDWFBkRE5FRmtuTnVXTnNOcWtEYVZtaHl5eU1aaVZ6?=
 =?utf-8?B?MFhhYVg2Vk5qYjBobkZVMUJ0dVZtMXE3ZDJzRTRDNGszQWFaTldveW1HNHBu?=
 =?utf-8?B?NWpsQ2phbTBENzRrMzFIM2xYZW9raVdWRVhQK2VYaS9lZ1JUUXNoeGxFbGl0?=
 =?utf-8?B?ZUNwL051MFo1eXBaYlJqR0pHNnhXd3lhN08vcEU1MDE2OFhsZEdjOWlWcXd5?=
 =?utf-8?B?WElEb1VhbFpuWnVoQkRqazdNMlNnOVgvR1A0Y0VkNEZTNVVCcUlZS1QyQkI3?=
 =?utf-8?B?QktrbFkxLytCVkV0dXB4Q3o3bUI2UnVQQWNkTE9KbVdQaHNJa0ZacmFqTE52?=
 =?utf-8?B?aGVuRGdSZVZJQUdFWWtpSkE3cmszM1hsSjdWUmhjUTBValFXL2ptWXJJVUNS?=
 =?utf-8?B?bk1UZm5Sd2tucjJmYm1GQ2RFK200cTNZUmw4RXNTcndLNE9XQmlFeTUrN24y?=
 =?utf-8?B?Y3hkK0FwMTFHQWhXNFRVaDBZK3oyd1pJOUlFQTJWdzJpSUUxeFYvQzJWNUo3?=
 =?utf-8?B?SmlCSU5xbms2cGZxeHBJdGhzKzZuY0IrOFhpTnpLVE1BS0R5Q05OMm94emlZ?=
 =?utf-8?B?cHRROTZhU0hlWlV1ekYzMWIzb01hcVZNQzFEMmdXbUVWcTBBSzNhYmthdFRw?=
 =?utf-8?B?NEZ0YkZIK2xXUStkY3k0V0EvR0JVcnpka3JWaW5ISEZpTEl5dlRLMTBmK005?=
 =?utf-8?B?M3Z1ckdsbGZRd0xMNnhqblNoNktGa1lNZ0s0bThCUHRMT2VOMURaVGRMaWFE?=
 =?utf-8?B?NFJPR3dkZ0lMa2NVcFZiYnhCWFJCVTg0SjZsOE90UWwrVzNDZmFNaDIzNnpH?=
 =?utf-8?B?RzdjUkx1R3V0c1Y2eURuY2xuWUU1MUFnVVQ5eXZlN3BDc2xCclFoRXYrWnFa?=
 =?utf-8?B?TU5hM1RsZW13WmJ0ODUvaGR2Q1ZnL1lLUmVmRjhHcFVpUUlKQlRSVFU5Wml5?=
 =?utf-8?B?Sm83YXNFTW14Y3VNd2xWaUlXdTd2VXhSNWNEdUI5NTBWNnk4VEpQSTk3NXdQ?=
 =?utf-8?B?Yys0aXU3TGg1ZVlNSjVXZ1VqaksydkQ0L2NheHNWZEJSbys2WTRERzN4UldC?=
 =?utf-8?B?Rnc0emVHcEdQMWcrL2M5UzdDcWZzZXlNK1lxRC9jL2hPcnNWNDkya2VDa1JW?=
 =?utf-8?B?VGVUYUFoSkxNbWk1c3JKMlZDWDRsR0E3VTlmUXJjb1B2YnY1RmNsVy8zR0ZE?=
 =?utf-8?B?TjJyckp3V0gwNExsUWJLOTJiQ3IrS0lIaHkvRlZUTHpIR2ZXblhGb01VKzAr?=
 =?utf-8?B?bDJBNmxTSFk2aHlSZzRTRUl5MXhzVGhSUnNBU0VFZFBXSTUxdisxM1Jvc3Fh?=
 =?utf-8?B?MnJaSloxRWFKcDhaR0l2VTVZc25NZGdCc3l4Ui95RXVyemJzMlI1NFZNdllk?=
 =?utf-8?B?VHJRbmVPNzlpMjNNYkpkeDRUNnRkVUsrL0dSY21GNUdmS2piNUJoRmhZRHVM?=
 =?utf-8?B?ZVhvenFQUTNUSzRmQUlOdlpSYzNRZlRnRjY2SnFEMGEwM3Nrb1BUbG05di9L?=
 =?utf-8?B?Y1AwYWREeGtCSmQ1bTRicDRUMXZFVmkrR2lvVEIwYnp5ZHFoQldmRzZ5cy9V?=
 =?utf-8?B?cHgyTmc5R212dXVTQ1RkemgwZzFMR3lKL2d0TmRNdkJ4eEwrZk9CaDRFTFFR?=
 =?utf-8?Q?O/di7PDXYVa7eYNe9SXS5PA=3D?=
X-OriginatorOrg: in-advantage.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5c708c58-04e4-44e2-aff5-08dc869ff29b
X-MS-Exchange-CrossTenant-AuthSource: DS0PR10MB6974.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Jun 2024 03:14:30.9462
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 48e842ca-fbd8-4633-a79d-0c955a7d3aae
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Jr9XkjuBO9RQe309dGQc/PCPW03uWM+fQysfUU0mrfXWiaTZbhBms70kiprvauNCgt798nX/QUk0uTHlYNZaijeTRvo5c+eVG6QRzP05sVc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR10MB5769

Hi Louis,

On Thu, Jun 06, 2024 at 10:06:07AM +0200, Louis Chauvet wrote:
> Le 04/06/24 - 22:04, Colin Foster a écrit :
> > Hi Louis,
> 
> Hi,
>  
> > I found that commit e64d3b6fc9a3 ("spi: omap2-mcpsi: Enable MULTI-mode
> > in more situations") caused a regression in the ocelot_mfd driver. It
> > essentially causes the boot to hang during probe of the SPI device.
> 
> I don't know what can cause this. My patch can "compact" few words into 
> only a bigger one, so the signal on the cable can change, but it follows 
> the SPI specification and the device should have the same behavior.
> 
> Instead of two very distinct words (for example two 8 bits words):
> 
>   <-- first word -->             <-- second word -->
>    _   _   _   _   _              _   _   _   _   _
> __| |_| |_| ... |_| |____________| |_| |_| ... |_| |_
> 
> The signal on the wire will be merged into one bigger (one 16 bits word):
> 
>   <-- first word -->  <-- second word -->
>    _   _   _   _   _   _   _   _   _   _
> __| |_| |_| ... |_| |_| |_| |_| ... |_| |_
> 
> > The following patch restores functionality. I can hook up a logic
> > analyzer tomorrow to get some more info, but I wanted to see if you had
> > any ideas.
>  
> I don't understand the link between the solution and my patch, can you 
> share the logic analyzer results?
> 
> Maybe the issue is the same as [1]? Does it solves the issue?
> 
> [1]: https://lore.kernel.org/all/20240506-fix-omap2-mcspi-v2-1-d9c77ba8b9c7@bootlin.com/

I took three measurements:

1. My patch added
2. No patches
3. The 'fix' patch applied from [1]

2 and 3 appear to behave the same for me. But CS is certainly the issue
I'm seeing. Here's a quick description:

A write on this chip is seven bytes - three bytes address and four bytes
data. Every write in 1, 2, and 3 starts with a CS assertion, 7 bytes,
and a CS de-assertion. Writes work.

A read is 8 bytes - three for address, one padding, and four data.
Writes in 1 start and end with CS asserting and de-asserting. Reads in
2 and 3 assert CS and combine multiple writes, which fails. Reads no
longer work as a result.

I thought maybe the lack of cs_change might be the culprit, but this
didn't resolve the issue either:

@@ -172,8 +175,13 @@ static int ocelot_spi_regmap_bus_write(void *context, const void *data, size_t c
 {
        struct device *dev = context;
        struct spi_device *spi = to_spi_device(dev);
+       struct spi_transfer     t = {
+                       .tx_buf         = data,
+                       .len            = count,
+                       .cs_change      = 1,
+               };

-       return spi_write(spi, data, count);
+       return spi_sync_transfer(spi, &t, 1);
 }


The relevant documentation on cs_change:

 * (ii) When the transfer is the last one in the message, the chip may
 * stay selected until the next transfer.  On multi-device SPI busses
 * with nothing blocking messages going to other devices, this is just
 * a performance hint; starting a message to another device deselects
 * this one.  But in other cases, this can be used to ensure correctness.
 * Some devices need protocol transactions to be built from a series of
 * spi_message submissions, where the content of one message is determined
 * by the results of previous messages and where the whole transaction
 * ends when the chipselect goes inactive.

And relevant code around cs_change:
https://elixir.bootlin.com/linux/v6.10-rc2/source/drivers/spi/spi.c#L1715


So I think the question I have is:

Should the CS line be de-asserted at the end of "spi_write"?

If yes, the multi mode patch seems to break this on 8-byte transfers.

If no, then how can I ensure this?


Thanks

Colin Foster