| Message ID | 20231120193504.5922-1-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: phy: aquantia: make mailbox interface4 lsw addr mask more specific | expand |
On Mon, 20 Nov 2023 20:35:04 +0100 Christian Marangi wrote: > It seems some arch (s390) require a more specific mask for FIELD_PREP > and doesn't like using GENMASK(15, 2) for u16 values. > > Fix the compilation error by adding the additional mask for the BITS > that the PHY ignore and AND the passed addr with the real mask that the > PHY will parse for the mailbox interface 4 addr to make sure extra > values are correctly removed. Ah. Um. Pff. Erm. I'm not sure. Endianness is not my strong suit but this code: /* PHY expect addr in LE */ addr = (__force u32)cpu_to_le32(addr); /* ... use (u16)(addr) */ /* ... use (u16)(addr >> 16) */ does not make sense to me. You're operating on register values here, there is no endian. Endian only exists when you store or load from memory. IOW, this: addr = 0x12345678; print((u16)addr); print(addr >> 16); will print the same exact thing regardless of the CPU endian. Why did you put the byte swap in there?
On Tue, Nov 21, 2023 at 03:08:59PM -0800, Jakub Kicinski wrote: > On Mon, 20 Nov 2023 20:35:04 +0100 Christian Marangi wrote: > > It seems some arch (s390) require a more specific mask for FIELD_PREP > > and doesn't like using GENMASK(15, 2) for u16 values. > > > > Fix the compilation error by adding the additional mask for the BITS > > that the PHY ignore and AND the passed addr with the real mask that the > > PHY will parse for the mailbox interface 4 addr to make sure extra > > values are correctly removed. > > Ah. Um. Pff. Erm. I'm not sure. > > Endianness is not my strong suit but this code: > > /* PHY expect addr in LE */ > addr = (__force u32)cpu_to_le32(addr); > > /* ... use (u16)(addr) */ > /* ... use (u16)(addr >> 16) */ > > does not make sense to me. > > You're operating on register values here, there is no endian. > Endian only exists when you store or load from memory. IOW, this: > > addr = 0x12345678; > print((u16)addr); > print(addr >> 16); > > will print the same exact thing regardless of the CPU endian. > > Why did you put the byte swap in there? the 2 addr comes from a define #define DRAM_BASE_ADDR 0x3FFE0000 #define IRAM_BASE_ADDR 0x40000000 it wasn't clear to me if on BE these addrs gets saved differently or not. PHY wants the addr in LE. On testing by removing the cpu_to_le32 the error is correctly removed! I guess on BE the addr was actually swapped and FIELD_GET was correctly warning (and failing) as data was missing in applying the mask. If all of this makes sense, will send a followup patch that drop the cpu_to_le32 and also the other in the bottom that does cpu_to_be32 (to a __swab32 as FW is LE and mailbox calculate CRC in BE)
On Wed, 22 Nov 2023 00:32:56 +0100 Christian Marangi wrote: > the 2 addr comes from a define > > #define DRAM_BASE_ADDR 0x3FFE0000 > #define IRAM_BASE_ADDR 0x40000000 > > it wasn't clear to me if on BE these addrs gets saved differently or > not. PHY wants the addr in LE. > > On testing by removing the cpu_to_le32 the error is correctly removed! > > I guess on BE the addr was actually swapped and FIELD_GET was correctly > warning (and failing) as data was missing in applying the mask. I think so. It's the responsibility of whether underlies phy_write_mmd() to make sure the data is put on the bus in correct order (but that's still just within the u16 boundaries, splitting a constant into u16 halves is not endian dependent). > If all of this makes sense, will send a followup patch that drop the > cpu_to_le32 and also the other in the bottom that does cpu_to_be32 (to a > __swab32 as FW is LE and mailbox calculate CRC in BE) Not so sure about this one, it puts the u32 on the stack, and takes the address of it: u32 word; word = (__force u32)cpu_to_be32(word); crc = crc_ccitt_false(crc, (u8 *)&word, sizeof(word)); so the endian will matter here. My guess is that this part is correct.
On Tue, Nov 21, 2023 at 03:39:18PM -0800, Jakub Kicinski wrote: > On Wed, 22 Nov 2023 00:32:56 +0100 Christian Marangi wrote: > > the 2 addr comes from a define > > > > #define DRAM_BASE_ADDR 0x3FFE0000 > > #define IRAM_BASE_ADDR 0x40000000 > > > > it wasn't clear to me if on BE these addrs gets saved differently or > > not. PHY wants the addr in LE. > > > > On testing by removing the cpu_to_le32 the error is correctly removed! > > > > I guess on BE the addr was actually swapped and FIELD_GET was correctly > > warning (and failing) as data was missing in applying the mask. > > I think so. It's the responsibility of whether underlies > phy_write_mmd() to make sure the data is put on the bus in > correct order (but that's still just within the u16 boundaries, > splitting a constant into u16 halves is not endian dependent). > > > If all of this makes sense, will send a followup patch that drop the > > cpu_to_le32 and also the other in the bottom that does cpu_to_be32 (to a > > __swab32 as FW is LE and mailbox calculate CRC in BE) > > Not so sure about this one, it puts the u32 on the stack, and takes > the address of it: > > u32 word; > > word = (__force u32)cpu_to_be32(word); > crc = crc_ccitt_false(crc, (u8 *)&word, sizeof(word)); > > so the endian will matter here. My guess is that this part is correct. Ehhh this is problematic. Data comes from nvmem or filesystem, in theory they should not be touched/converted. nvmem_cell_read or request_firmware return pointer to u8 and it's the firmware (that is always in LE) If data is not converted and passed AS IS from what is read to the allocated data, then data should be always swapped. (this PHY is fun... it's probably BE internally but expect LE stuff in the mailbox, as it does emit BE CRC.) Any idea where I can verify if nvmem_cell_read or request_firmware makes any kind of endianess conversion on the data it does read?
On Wed, 22 Nov 2023 00:48:01 +0100 Christian Marangi wrote: > > Not so sure about this one, it puts the u32 on the stack, and takes > > the address of it: > > > > u32 word; > > > > word = (__force u32)cpu_to_be32(word); > > crc = crc_ccitt_false(crc, (u8 *)&word, sizeof(word)); > > > > so the endian will matter here. My guess is that this part is correct. Actually I'm wrong about this, you're reading and writing the data, so endian conversion happens twice. Canceling itself out. > Ehhh this is problematic. Data comes from nvmem or filesystem, in theory > they should not be touched/converted. > > nvmem_cell_read or request_firmware return pointer to u8 and it's the > firmware (that is always in LE) > > If data is not converted and passed AS IS from what is read to the > allocated data, then data should be always swapped. > (this PHY is fun... it's probably BE internally but expect LE stuff in > the mailbox, as it does emit BE CRC.) > > Any idea where I can verify if nvmem_cell_read or request_firmware makes > any kind of endianess conversion on the data it does read? The underlying storage should be byte-accessible, so neither interface should change anything about the endian. You should probably switch get_unaligned_le32() for reading it into the word variable, tho.
On Tue, Nov 21, 2023 at 03:58:12PM -0800, Jakub Kicinski wrote: > On Wed, 22 Nov 2023 00:48:01 +0100 Christian Marangi wrote: > > > Not so sure about this one, it puts the u32 on the stack, and takes > > > the address of it: > > > > > > u32 word; > > > > > > word = (__force u32)cpu_to_be32(word); > > > crc = crc_ccitt_false(crc, (u8 *)&word, sizeof(word)); > > > > > > so the endian will matter here. My guess is that this part is correct. > > Actually I'm wrong about this, you're reading and writing the data, > so endian conversion happens twice. Canceling itself out. > > > Ehhh this is problematic. Data comes from nvmem or filesystem, in theory > > they should not be touched/converted. > > > > nvmem_cell_read or request_firmware return pointer to u8 and it's the > > firmware (that is always in LE) > > > > If data is not converted and passed AS IS from what is read to the > > allocated data, then data should be always swapped. > > (this PHY is fun... it's probably BE internally but expect LE stuff in > > the mailbox, as it does emit BE CRC.) > > > > Any idea where I can verify if nvmem_cell_read or request_firmware makes > > any kind of endianess conversion on the data it does read? > > The underlying storage should be byte-accessible, so neither interface > should change anything about the endian. > > You should probably switch get_unaligned_le32() for reading it into > the word variable, tho. I don't need to read it, I need to pass the data directly from what is read to mailbox so using get_unaligned_le32 would actually make a conversion. Anyway thanks a lot for putting some extra words and make me check this further! Will send a v2 tomorrow!
On Tue, Nov 21, 2023 at 03:39:18PM -0800, Jakub Kicinski wrote: > On Wed, 22 Nov 2023 00:32:56 +0100 Christian Marangi wrote: > > the 2 addr comes from a define > > > > #define DRAM_BASE_ADDR 0x3FFE0000 > > #define IRAM_BASE_ADDR 0x40000000 > > > > it wasn't clear to me if on BE these addrs gets saved differently or > > not. PHY wants the addr in LE. > > > > On testing by removing the cpu_to_le32 the error is correctly removed! > > > > I guess on BE the addr was actually swapped and FIELD_GET was correctly > > warning (and failing) as data was missing in applying the mask. > > I think so. It's the responsibility of whether underlies > phy_write_mmd() to make sure the data is put on the bus in > correct order (but that's still just within the u16 boundaries, > splitting a constant into u16 halves is not endian dependent). MDIO bus accesses via the MDIO bus accessors are expected to produce the correct value in CPU order no matter what endian the host platform is. So if the BMCR contains the value 0x1234, then reading and then printing this register is expected to produce 0x1234 irrespective of the host CPU architecture. We do have 32-bit values split across two registers in clause 45 PHYs, namely the MMD present register pair. Another example is the PHY ID. In both cases we read the registers and apply the appropriate shift. See get_phy_c45_ids() and get_phy_c22_id(). Note that these, again, have to work independent of the CPU architecture.
diff --git a/drivers/net/phy/aquantia/aquantia.h b/drivers/net/phy/aquantia/aquantia.h index 9ed38972abdb..7685bfaf0b07 100644 --- a/drivers/net/phy/aquantia/aquantia.h +++ b/drivers/net/phy/aquantia/aquantia.h @@ -30,7 +30,10 @@ #define VEND1_GLOBAL_MAILBOX_INTERFACE3_MSW_ADDR(x) FIELD_PREP(VEND1_GLOBAL_MAILBOX_INTERFACE3_MSW_ADDR_MASK, (u16)((x) >> 16)) #define VEND1_GLOBAL_MAILBOX_INTERFACE4 0x0203 #define VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR_MASK GENMASK(15, 2) -#define VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR(x) FIELD_PREP(VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR_MASK, (u16)(x)) +#define VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR_DONT_CARE_MASK GENMASK(1, 0) +#define VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR(x) FIELD_PREP(VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR_MASK | \ + VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR_DONT_CARE_MASK, \ + (u16)((x) & VEND1_GLOBAL_MAILBOX_INTERFACE4_LSW_ADDR_MASK)) #define VEND1_GLOBAL_MAILBOX_INTERFACE5 0x0204 #define VEND1_GLOBAL_MAILBOX_INTERFACE5_MSW_DATA_MASK GENMASK(15, 0)
It seems some arch (s390) require a more specific mask for FIELD_PREP and doesn't like using GENMASK(15, 2) for u16 values. Fix the compilation error by adding the additional mask for the BITS that the PHY ignore and AND the passed addr with the real mask that the PHY will parse for the mailbox interface 4 addr to make sure extra values are correctly removed. Reported-by: kernel test robot <lkp@intel.com> Closes: https://lore.kernel.org/oe-kbuild-all/202311210414.sEJZjlcD-lkp@intel.com/ Fixes: e93984ebc1c8 ("net: phy: aquantia: add firmware load support") Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> --- drivers/net/phy/aquantia/aquantia.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)