[PATCH 0/6] highbank: Update to driver model

The Calxeda Highbank and Midway systems have not been updated in a while, so were gathering more and more warning messages about not being DM compliant over time.
To avoid the support being removed, update the board to comply with modern U-Boot standards: - Enable OF_CONTROL, and use the DTB provided by previous firmware stages. - Convert the XGMAC network driver over to the driver model. - Read the actual DRAM size (as detect by the management processor) from the DTB and use that to populate the memory node, and U-Boot internal idea of DRAM.
Access to SATA drives broke with commit 66c54f1a3e46 ("scsi: Simplify scsi_read()/_write()") before the v2020.01 release, and remains non-functional with this series for now. Reverting that commit (and massaging it a bit) brought it back to life. I will send a proper patch once I wrapped my head around the SCSI/SATA/AHCI/DM-block connections.
To test this build, you can use chain-loading on an existing installation (firmware updates in flash on those boards are not trivial): $ mkimage -A arm -O u-boot -T standalone -C none -a 0x8000 -e 0x8000 \ -d u-boot.bin -n U-Boot /srv/tftp/u-boot-highbank.img => tftpboot 0x8000 u-boot-highbank.img => bootm 0x8000
Cheers, Andre
Andre Przywara (6): arm: highbank: Limit FDT and initrd load addresses arm: highbank: Enable OF_CONTROL net: calxedagmac: Convert to DM_ETH arm: highbank: Remove artificial SDRAM size arm: highbank: Do DRAM init from DT arm: highbank: Update maintainership
arch/arm/Kconfig | 11 +- board/highbank/MAINTAINERS | 2 +- board/highbank/highbank.c | 32 +++--- configs/highbank_defconfig | 4 +- drivers/net/Kconfig | 7 ++ drivers/net/calxedaxgmac.c | 192 +++++++++++++++++++++++------------ include/configs/highbank.h | 13 +-- include/netdev.h | 1 - scripts/config_whitelist.txt | 1 - 9 files changed, 170 insertions(+), 93 deletions(-)

So far on Highbank/Midway machines U-Boot only ever uses 512MB of DRAM, even though the machines have typically 4GB and 8GB, respectively. That means that so far we didn't need an extra limit for placing the DTB and initrd, as the 512MB are lower than the kernel's limit ("lowmem", typically 768MB).
With U-Boot now needing to learn about the actual memory size (to correctly populate the EFI memory map), it might relocate fdt and initrd to the end of DRAM, which is out of reach of the kernel.
So add limiting values to the fdt_high and initrd_high environment variables, to prevent U-Boot from using too high addresses.
Signed-off-by: Andre Przywara andre.przywara@arm.com --- include/configs/highbank.h | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/include/configs/highbank.h b/include/configs/highbank.h index bdbaa475d20..5e3cc3a1db6 100644 --- a/include/configs/highbank.h +++ b/include/configs/highbank.h @@ -57,4 +57,8 @@ #define CONFIG_SYS_INIT_SP_ADDR 0x01000000 #define CONFIG_SKIP_LOWLEVEL_INIT
+#define CONFIG_EXTRA_ENV_SETTINGS \ + "fdt_high=0x20000000\0" \ + "initrd_high=0x20000000\0" + #endif

On Mon, Apr 12, 2021 at 01:04:50AM +0100, Andre Przywara wrote:
So far on Highbank/Midway machines U-Boot only ever uses 512MB of DRAM, even though the machines have typically 4GB and 8GB, respectively. That means that so far we didn't need an extra limit for placing the DTB and initrd, as the 512MB are lower than the kernel's limit ("lowmem", typically 768MB).
With U-Boot now needing to learn about the actual memory size (to correctly populate the EFI memory map), it might relocate fdt and initrd to the end of DRAM, which is out of reach of the kernel.
So add limiting values to the fdt_high and initrd_high environment variables, to prevent U-Boot from using too high addresses.
Signed-off-by: Andre Przywara andre.przywara@arm.com
Reviewed-by: Tom Rini trini@konsulko.com

On Mon, Apr 12, 2021 at 01:04:50AM +0100, Andre Przywara wrote:
So far on Highbank/Midway machines U-Boot only ever uses 512MB of DRAM, even though the machines have typically 4GB and 8GB, respectively. That means that so far we didn't need an extra limit for placing the DTB and initrd, as the 512MB are lower than the kernel's limit ("lowmem", typically 768MB).
With U-Boot now needing to learn about the actual memory size (to correctly populate the EFI memory map), it might relocate fdt and initrd to the end of DRAM, which is out of reach of the kernel.
So add limiting values to the fdt_high and initrd_high environment variables, to prevent U-Boot from using too high addresses.
Signed-off-by: Andre Przywara andre.przywara@arm.com Reviewed-by: Tom Rini trini@konsulko.com
Applied to u-boot/master, thanks!

All Calxeda machines are actually a poster book example of device tree usage: the DT is loaded from flash by the management processor into DRAM, the memory node is populated with the detected DRAM size and this DT is then handed over to the kernel. So it's a shame that U-Boot didn't participate in this chain, but fortunately this is easy to fix:
Define CONFIG_OF_CONTROL and CONFIG_OF_BOARD, and provide a trivial function to tell U-Boot about the (fixed) location of the DTB in DRAM. Then enable DM_SERIAL, to let the PL011 driver pick up the UART platform data from the DT. Also define AHCI, to bring this driver into the driver model world as well.
Signed-off-by: Andre Przywara andre.przywara@arm.com --- arch/arm/Kconfig | 9 ++++++++- board/highbank/highbank.c | 10 ++++++++++ configs/highbank_defconfig | 1 + include/configs/highbank.h | 1 - 4 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 76adf7fdb24..bd6064923fe 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -748,7 +748,14 @@ config ARCH_S5PC1XX config ARCH_HIGHBANK bool "Calxeda Highbank" select CPU_V7A - select PL011_SERIAL + select PL01X_SERIAL + select DM + select DM_SERIAL + select OF_CONTROL + select OF_BOARD + select CLK + select CLK_CCF + select AHCI
config ARCH_INTEGRATOR bool "ARM Ltd. Integrator family" diff --git a/board/highbank/highbank.c b/board/highbank/highbank.c index 906bd9b6dda..2e2300a307f 100644 --- a/board/highbank/highbank.c +++ b/board/highbank/highbank.c @@ -119,6 +119,16 @@ int ft_board_setup(void *fdt, struct bd_info *bd) } #endif
+void *board_fdt_blob_setup(void) +{ + /* + * The ECME management processor loads the DTB from NOR flash + * into DRAM (at 4KB), where it gets patched to contain the + * detected memory size. + */ + return (void *)0x1000; +} + static int is_highbank(void) { uint32_t midr; diff --git a/configs/highbank_defconfig b/configs/highbank_defconfig index 369b65ceee8..773ed7a00bf 100644 --- a/configs/highbank_defconfig +++ b/configs/highbank_defconfig @@ -26,3 +26,4 @@ CONFIG_BOOTCOUNT_LIMIT=y CONFIG_SCSI=y CONFIG_CONS_INDEX=0 CONFIG_OF_LIBFDT=y +CONFIG_OF_BOARD=y diff --git a/include/configs/highbank.h b/include/configs/highbank.h index 5e3cc3a1db6..7f37c81fc9f 100644 --- a/include/configs/highbank.h +++ b/include/configs/highbank.h @@ -18,7 +18,6 @@ #define CONFIG_SYS_MALLOC_LEN (512 * 1024)
#define CONFIG_PL011_CLOCK 150000000 -#define CONFIG_PL01x_PORTS { (void *)(0xFFF36000) }
#define CONFIG_SYS_BOOTCOUNT_LE /* Use little-endian accessors */

On Mon, Apr 12, 2021 at 01:04:51AM +0100, Andre Przywara wrote:
All Calxeda machines are actually a poster book example of device tree usage: the DT is loaded from flash by the management processor into DRAM, the memory node is populated with the detected DRAM size and this DT is then handed over to the kernel. So it's a shame that U-Boot didn't participate in this chain, but fortunately this is easy to fix:
Define CONFIG_OF_CONTROL and CONFIG_OF_BOARD, and provide a trivial function to tell U-Boot about the (fixed) location of the DTB in DRAM. Then enable DM_SERIAL, to let the PL011 driver pick up the UART platform data from the DT. Also define AHCI, to bring this driver into the driver model world as well.
Signed-off-by: Andre Przywara andre.przywara@arm.com
Applied to u-boot/master, thanks!

To squash that nasty warning message and make better use of the newly gained OF_CONTROL feature, let's convert the calxedagmac driver to the "new" driver model. The conversion is pretty straight forward, mostly just adjusting the use of the involved data structures. The only actual change is the required split of the receive routine into a receive and free_pkt part. Also this allows us to get rid of the hardcoded platform information and explicit init calls.
This also uses the opportunity to wrap the code decoding the MMIO register base address, to make it safe for using PHYS_64BIT later.
Signed-off-by: Andre Przywara andre.przywara@arm.com --- arch/arm/Kconfig | 1 + board/highbank/highbank.c | 13 --- configs/highbank_defconfig | 1 + drivers/net/Kconfig | 7 ++ drivers/net/calxedaxgmac.c | 192 +++++++++++++++++++++++------------ include/configs/highbank.h | 2 - include/netdev.h | 1 - scripts/config_whitelist.txt | 1 - 8 files changed, 137 insertions(+), 81 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index bd6064923fe..0082d06182a 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -756,6 +756,7 @@ config ARCH_HIGHBANK select CLK select CLK_CCF select AHCI + select DM_ETH
config ARCH_INTEGRATOR bool "ARM Ltd. Integrator family" diff --git a/board/highbank/highbank.c b/board/highbank/highbank.c index 2e2300a307f..0667a48965c 100644 --- a/board/highbank/highbank.c +++ b/board/highbank/highbank.c @@ -10,7 +10,6 @@ #include <fdt_support.h> #include <init.h> #include <net.h> -#include <netdev.h> #include <scsi.h> #include <asm/global_data.h>
@@ -52,18 +51,6 @@ int board_init(void) return 0; }
-/* We know all the init functions have been run now */ -int board_eth_init(struct bd_info *bis) -{ - int rc = 0; - -#ifdef CONFIG_CALXEDA_XGMAC - rc += calxedaxgmac_initialize(0, 0xfff50000); - rc += calxedaxgmac_initialize(1, 0xfff51000); -#endif - return rc; -} - #ifdef CONFIG_SCSI_AHCI_PLAT void scsi_init(void) { diff --git a/configs/highbank_defconfig b/configs/highbank_defconfig index 773ed7a00bf..c3352b827d7 100644 --- a/configs/highbank_defconfig +++ b/configs/highbank_defconfig @@ -27,3 +27,4 @@ CONFIG_SCSI=y CONFIG_CONS_INDEX=0 CONFIG_OF_LIBFDT=y CONFIG_OF_BOARD=y +CONFIG_CALXEDA_XGMAC=y diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig index cf062fad4da..cebd84035c8 100644 --- a/drivers/net/Kconfig +++ b/drivers/net/Kconfig @@ -171,6 +171,13 @@ config CORTINA_NI_ENET This driver supports the Cortina-Access Ethernet MAC for all supported CAxxxx SoCs.
+config CALXEDA_XGMAC + bool "Calxeda XGMAC support" + depends on DM_ETH + help + This driver supports the XGMAC in Calxeda Highbank and Midway + machines. + config DWC_ETH_QOS bool "Synopsys DWC Ethernet QOS device support" depends on DM_ETH diff --git a/drivers/net/calxedaxgmac.c b/drivers/net/calxedaxgmac.c index 8b2ee49b441..b98d709117a 100644 --- a/drivers/net/calxedaxgmac.c +++ b/drivers/net/calxedaxgmac.c @@ -10,6 +10,8 @@ #include <linux/delay.h> #include <linux/err.h> #include <asm/io.h> +#include <dm.h> +#include <dm/device-internal.h> /* for dev_set_priv() */
#define TX_NUM_DESC 1 #define RX_NUM_DESC 32 @@ -212,6 +214,18 @@ struct xgmac_dma_desc { __le32 res[3]; };
+static struct xgmac_regs *xgmac_get_regs(struct eth_pdata *pdata) +{ + /* + * We use PHYS_64BIT on Highbank, so phys_addr_t is bigger than + * a pointer. U-Boot doesn't use LPAE (not even the MMU on highbank), + * so we can't access anything above 4GB. + * We have a check in the probe function below the ensure this, + * so casting to a 32-bit pointer type is fine here. + */ + return (struct xgmac_regs *)(uintptr_t)pdata->iobase; +} + /* XGMAC Descriptor Access Helpers */ static inline void desc_set_buf_len(struct xgmac_dma_desc *p, u32 buf_sz) { @@ -304,8 +318,6 @@ struct calxeda_eth_dev {
u32 tx_currdesc; u32 rx_currdesc; - - struct eth_device *dev; } __aligned(32);
/* @@ -313,10 +325,10 @@ struct calxeda_eth_dev { * advanced descriptors. */
-static void init_rx_desc(struct calxeda_eth_dev *priv) +static void init_rx_desc(struct eth_pdata *pdata, struct calxeda_eth_dev *priv) { struct xgmac_dma_desc *rxdesc = priv->rx_chain; - struct xgmac_regs *regs = (struct xgmac_regs *)priv->dev->iobase; + struct xgmac_regs *regs = xgmac_get_regs(pdata); void *rxbuffer = priv->rxbuffer; int i;
@@ -330,17 +342,16 @@ static void init_rx_desc(struct calxeda_eth_dev *priv) } }
-static void init_tx_desc(struct calxeda_eth_dev *priv) +static void init_tx_desc(struct eth_pdata *pdata, struct calxeda_eth_dev *priv) { - struct xgmac_regs *regs = (struct xgmac_regs *)priv->dev->iobase; + struct xgmac_regs *regs = xgmac_get_regs(pdata);
desc_init_tx_desc(priv->tx_chain, TX_NUM_DESC); writel((ulong)priv->tx_chain, ®s->txdesclist); }
-static int xgmac_reset(struct eth_device *dev) +static int xgmac_reset(struct xgmac_regs *regs) { - struct xgmac_regs *regs = (struct xgmac_regs *)dev->iobase; int timeout = MAC_TIMEOUT; u32 value;
@@ -356,27 +367,28 @@ static int xgmac_reset(struct eth_device *dev) return timeout; }
-static void xgmac_hwmacaddr(struct eth_device *dev) +static void xgmac_hwmacaddr(struct eth_pdata *pdata) { - struct xgmac_regs *regs = (struct xgmac_regs *)dev->iobase; + struct xgmac_regs *regs = xgmac_get_regs(pdata); u32 macaddr[2];
- memcpy(macaddr, dev->enetaddr, 6); + memcpy(macaddr, pdata->enetaddr, ARP_HLEN); writel(macaddr[1], ®s->macaddr[0].hi); writel(macaddr[0], ®s->macaddr[0].lo); }
-static int xgmac_init(struct eth_device *dev, struct bd_info * bis) +static int xgmac_eth_start(struct udevice *dev) { - struct xgmac_regs *regs = (struct xgmac_regs *)dev->iobase; - struct calxeda_eth_dev *priv = dev->priv; - int value; + struct eth_pdata *pdata = dev_get_plat(dev); + struct xgmac_regs *regs = xgmac_get_regs(pdata); + struct calxeda_eth_dev *priv = dev_get_priv(dev); + u32 value;
- if (xgmac_reset(dev) < 0) - return -1; + if (xgmac_reset(regs) < 0) + return -ETIMEDOUT;
/* set the hardware MAC address */ - xgmac_hwmacaddr(dev); + xgmac_hwmacaddr(pdata);
/* set the AXI bus modes */ value = XGMAC_DMA_BUSMODE_ATDS | @@ -401,8 +413,8 @@ static int xgmac_init(struct eth_device *dev, struct bd_info * bis) writel(value, ®s->flow_control);
/* Initialize the descriptor chains */ - init_rx_desc(priv); - init_tx_desc(priv); + init_rx_desc(pdata, priv); + init_tx_desc(pdata, priv);
/* must set to 0, or when started up will cause issues */ priv->tx_currdesc = 0; @@ -425,10 +437,11 @@ static int xgmac_init(struct eth_device *dev, struct bd_info * bis) return 0; }
-static int xgmac_tx(struct eth_device *dev, void *packet, int length) +static int xgmac_tx(struct udevice *dev, void *packet, int length) { - struct xgmac_regs *regs = (struct xgmac_regs *)dev->iobase; - struct calxeda_eth_dev *priv = dev->priv; + struct calxeda_eth_dev *priv = dev_get_priv(dev); + struct eth_pdata *pdata = dev_get_plat(dev); + struct xgmac_regs *regs = xgmac_get_regs(pdata); u32 currdesc = priv->tx_currdesc; struct xgmac_dma_desc *txdesc = &priv->tx_chain[currdesc]; int timeout; @@ -453,35 +466,45 @@ static int xgmac_tx(struct eth_device *dev, void *packet, int length) return 0; }
-static int xgmac_rx(struct eth_device *dev) +static int xgmac_rx(struct udevice *dev, int flags, uchar **packetp) { - struct xgmac_regs *regs = (struct xgmac_regs *)dev->iobase; - struct calxeda_eth_dev *priv = dev->priv; + struct calxeda_eth_dev *priv = dev_get_priv(dev); u32 currdesc = priv->rx_currdesc; struct xgmac_dma_desc *rxdesc = &priv->rx_chain[currdesc]; int length = 0;
/* check if the host has the desc */ if (desc_get_owner(rxdesc)) - return -1; /* something bad happened */ + return -EAGAIN; /* the MAC is still chewing on it */
length = desc_get_rx_frame_len(rxdesc); + *packetp = desc_get_buf_addr(rxdesc);
- net_process_received_packet(desc_get_buf_addr(rxdesc), length); + priv->rx_currdesc = (currdesc + 1) & (RX_NUM_DESC - 1); + + return length; +} + +static int xgmac_free_pkt(struct udevice *dev, uchar *packet, int length) +{ + struct eth_pdata *pdata = dev_get_plat(dev); + struct xgmac_regs *regs = xgmac_get_regs(pdata); + struct calxeda_eth_dev *priv = dev_get_priv(dev); + u32 rxdesc = ((char *)packet - priv->rxbuffer) / ETH_BUF_SZ; + struct xgmac_dma_desc *p = &priv->rx_chain[rxdesc];
/* set descriptor back to owned by XGMAC */ - desc_set_rx_owner(rxdesc); + desc_set_rx_owner(p); writel(1, ®s->rxpoll);
- priv->rx_currdesc = (currdesc + 1) & (RX_NUM_DESC - 1); - - return length; + return 0; }
-static void xgmac_halt(struct eth_device *dev) +static void xgmac_eth_stop(struct udevice *dev) { - struct xgmac_regs *regs = (struct xgmac_regs *)dev->iobase; - struct calxeda_eth_dev *priv = dev->priv; + struct calxeda_eth_dev *priv = dev_get_priv(dev); + struct eth_pdata *pdata = dev_get_plat(dev); + struct xgmac_regs *regs = xgmac_get_regs(pdata); int value;
/* Disable TX/RX */ @@ -499,47 +522,88 @@ static void xgmac_halt(struct eth_device *dev) priv->rx_currdesc = 0; }
-int calxedaxgmac_initialize(u32 id, ulong base_addr) +/* + * Changing the MAC address is not a good idea, as the fabric would + * need to know about this as well (it does not learn MAC addresses). + */ +static int xgmac_eth_write_hwaddr(struct udevice *dev) +{ + return -ENOSYS; +} + +static int xgmac_eth_read_rom_hwaddr(struct udevice *dev) { - struct eth_device *dev; - struct calxeda_eth_dev *priv; - struct xgmac_regs *regs; + struct eth_pdata *pdata = dev_get_plat(dev); + struct xgmac_regs *regs = xgmac_get_regs(pdata); u32 macaddr[2];
- regs = (struct xgmac_regs *)base_addr; + /* The MAC address is already configured, so read it from registers. */ + macaddr[1] = readl(®s->macaddr[0].hi); + macaddr[0] = readl(®s->macaddr[0].lo); + memcpy(pdata->enetaddr, macaddr, ARP_HLEN);
- /* check hardware version */ - if (readl(®s->version) != 0x1012) - return -1; + return 0; +}
- dev = malloc(sizeof(*dev)); - if (!dev) - return 0; - memset(dev, 0, sizeof(*dev)); +static int xgmac_ofdata_to_platdata(struct udevice *dev) +{ + struct eth_pdata *pdata = dev_get_plat(dev); + struct calxeda_eth_dev *priv;
/* Structure must be aligned, because it contains the descriptors */ priv = memalign(32, sizeof(*priv)); - if (!priv) { - free(dev); - return 0; + if (!priv) + return -ENOMEM; + dev_set_priv(dev, priv); + + pdata->iobase = devfdt_get_addr(dev); + if (pdata->iobase == FDT_ADDR_T_NONE) { + printf("%s: Cannot find XGMAC base address\n", __func__); + return -EINVAL; + } + if (pdata->iobase >= (1ULL << 32)) { + printf("%s: MMIO base address cannot be above 4GB\n", __func__); + return -EINVAL; }
- dev->iobase = (int)base_addr; - dev->priv = priv; - priv->dev = dev; - sprintf(dev->name, "xgmac%d", id); + return 0; +}
- /* The MAC address is already configured, so read it from registers. */ - macaddr[1] = readl(®s->macaddr[0].hi); - macaddr[0] = readl(®s->macaddr[0].lo); - memcpy(dev->enetaddr, macaddr, 6); +static int xgmac_eth_probe(struct udevice *dev) +{ + struct eth_pdata *pdata = dev_get_plat(dev); + struct xgmac_regs *regs = xgmac_get_regs(pdata);
- dev->init = xgmac_init; - dev->send = xgmac_tx; - dev->recv = xgmac_rx; - dev->halt = xgmac_halt; + /* check hardware version */ + if (readl(®s->version) != 0x1012) + return -ENODEV;
- eth_register(dev); + xgmac_eth_read_rom_hwaddr(dev);
- return 1; + return 0; } + +static const struct eth_ops xgmac_eth_ops = { + .start = xgmac_eth_start, + .send = xgmac_tx, + .recv = xgmac_rx, + .free_pkt = xgmac_free_pkt, + .stop = xgmac_eth_stop, + .write_hwaddr = xgmac_eth_write_hwaddr, + .read_rom_hwaddr = xgmac_eth_read_rom_hwaddr, +}; + +static const struct udevice_id xgmac_eth_ids[] = { + { .compatible = "calxeda,hb-xgmac" }, + { } +}; + +U_BOOT_DRIVER(eth_xgmac) = { + .name = "eth_xgmac", + .id = UCLASS_ETH, + .of_match = xgmac_eth_ids, + .of_to_plat = xgmac_ofdata_to_platdata, + .probe = xgmac_eth_probe, + .ops = &xgmac_eth_ops, + .plat_auto = sizeof(struct eth_pdata), +}; diff --git a/include/configs/highbank.h b/include/configs/highbank.h index 7f37c81fc9f..fbd26ddd0fc 100644 --- a/include/configs/highbank.h +++ b/include/configs/highbank.h @@ -27,8 +27,6 @@ #define CONFIG_SYS_SCSI_MAX_DEVICE (CONFIG_SYS_SCSI_MAX_SCSI_ID * \ CONFIG_SYS_SCSI_MAX_LUN)
-#define CONFIG_CALXEDA_XGMAC - #define CONFIG_BOOT_RETRY_TIME -1 #define CONFIG_RESET_TO_RETRY
diff --git a/include/netdev.h b/include/netdev.h index 0ad9f8dc3a5..b960c421063 100644 --- a/include/netdev.h +++ b/include/netdev.h @@ -33,7 +33,6 @@ int at91emac_register(struct bd_info *bis, unsigned long iobase); int ax88180_initialize(struct bd_info *bis); int bcm_sf2_eth_register(struct bd_info *bis, u8 dev_num); int bfin_EMAC_initialize(struct bd_info *bis); -int calxedaxgmac_initialize(u32 id, ulong base_addr); int cs8900_initialize(u8 dev_num, int base_addr); int dc21x4x_initialize(struct bd_info *bis); int designware_initialize(ulong base_addr, u32 interface); diff --git a/scripts/config_whitelist.txt b/scripts/config_whitelist.txt index 39e6416afc0..25d25dcc9e9 100644 --- a/scripts/config_whitelist.txt +++ b/scripts/config_whitelist.txt @@ -149,7 +149,6 @@ CONFIG_BTB CONFIG_BUFNO_AUTO_INCR_BIT CONFIG_BUILD_ENVCRC CONFIG_BUS_WIDTH -CONFIG_CALXEDA_XGMAC CONFIG_CDP_APPLIANCE_VLAN_TYPE CONFIG_CDP_CAPABILITIES CONFIG_CDP_DEVICE_ID

On Mon, Apr 12, 2021 at 3:05 AM Andre Przywara andre.przywara@arm.com wrote:
To squash that nasty warning message and make better use of the newly gained OF_CONTROL feature, let's convert the calxedagmac driver to the "new" driver model. The conversion is pretty straight forward, mostly just adjusting the use of the involved data structures. The only actual change is the required split of the receive routine into a receive and free_pkt part. Also this allows us to get rid of the hardcoded platform information and explicit init calls.
This also uses the opportunity to wrap the code decoding the MMIO register base address, to make it safe for using PHYS_64BIT later.
Signed-off-by: Andre Przywara andre.przywara@arm.com
arch/arm/Kconfig | 1 + board/highbank/highbank.c | 13 --- configs/highbank_defconfig | 1 + drivers/net/Kconfig | 7 ++ drivers/net/calxedaxgmac.c | 192 +++++++++++++++++++++++------------ include/configs/highbank.h | 2 - include/netdev.h | 1 - scripts/config_whitelist.txt | 1 - 8 files changed, 137 insertions(+), 81 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index bd6064923fe..0082d06182a 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -756,6 +756,7 @@ config ARCH_HIGHBANK select CLK select CLK_CCF select AHCI
select DM_ETH
config ARCH_INTEGRATOR bool "ARM Ltd. Integrator family" diff --git a/board/highbank/highbank.c b/board/highbank/highbank.c index 2e2300a307f..0667a48965c 100644 --- a/board/highbank/highbank.c +++ b/board/highbank/highbank.c @@ -10,7 +10,6 @@ #include <fdt_support.h> #include <init.h> #include <net.h> -#include <netdev.h> #include <scsi.h> #include <asm/global_data.h>
@@ -52,18 +51,6 @@ int board_init(void) return 0; }
-/* We know all the init functions have been run now */ -int board_eth_init(struct bd_info *bis) -{
int rc = 0;
-#ifdef CONFIG_CALXEDA_XGMAC
rc += calxedaxgmac_initialize(0, 0xfff50000);
rc += calxedaxgmac_initialize(1, 0xfff51000);
-#endif
return rc;
-}
#ifdef CONFIG_SCSI_AHCI_PLAT void scsi_init(void) { diff --git a/configs/highbank_defconfig b/configs/highbank_defconfig index 773ed7a00bf..c3352b827d7 100644 --- a/configs/highbank_defconfig +++ b/configs/highbank_defconfig @@ -27,3 +27,4 @@ CONFIG_SCSI=y CONFIG_CONS_INDEX=0 CONFIG_OF_LIBFDT=y CONFIG_OF_BOARD=y +CONFIG_CALXEDA_XGMAC=y diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig index cf062fad4da..cebd84035c8 100644 --- a/drivers/net/Kconfig +++ b/drivers/net/Kconfig @@ -171,6 +171,13 @@ config CORTINA_NI_ENET This driver supports the Cortina-Access Ethernet MAC for all supported CAxxxx SoCs.
+config CALXEDA_XGMAC
bool "Calxeda XGMAC support"
depends on DM_ETH
help
This driver supports the XGMAC in Calxeda Highbank and Midway
machines.
config DWC_ETH_QOS bool "Synopsys DWC Ethernet QOS device support" depends on DM_ETH diff --git a/drivers/net/calxedaxgmac.c b/drivers/net/calxedaxgmac.c index 8b2ee49b441..b98d709117a 100644 --- a/drivers/net/calxedaxgmac.c +++ b/drivers/net/calxedaxgmac.c @@ -10,6 +10,8 @@ #include <linux/delay.h> #include <linux/err.h> #include <asm/io.h> +#include <dm.h> +#include <dm/device-internal.h> /* for dev_set_priv() */
#define TX_NUM_DESC 1 #define RX_NUM_DESC 32 @@ -212,6 +214,18 @@ struct xgmac_dma_desc { __le32 res[3]; };
+static struct xgmac_regs *xgmac_get_regs(struct eth_pdata *pdata) +{
/*
* We use PHYS_64BIT on Highbank, so phys_addr_t is bigger than
* a pointer. U-Boot doesn't use LPAE (not even the MMU on highbank),
* so we can't access anything above 4GB.
* We have a check in the probe function below the ensure this,
* so casting to a 32-bit pointer type is fine here.
*/
return (struct xgmac_regs *)(uintptr_t)pdata->iobase;
+}
/* XGMAC Descriptor Access Helpers */ static inline void desc_set_buf_len(struct xgmac_dma_desc *p, u32 buf_sz) { @@ -304,8 +318,6 @@ struct calxeda_eth_dev {
u32 tx_currdesc; u32 rx_currdesc;
struct eth_device *dev;
} __aligned(32);
/* @@ -313,10 +325,10 @@ struct calxeda_eth_dev {
- advanced descriptors.
*/
-static void init_rx_desc(struct calxeda_eth_dev *priv) +static void init_rx_desc(struct eth_pdata *pdata, struct calxeda_eth_dev *priv) { struct xgmac_dma_desc *rxdesc = priv->rx_chain;
struct xgmac_regs *regs = (struct xgmac_regs *)priv->dev->iobase;
struct xgmac_regs *regs = xgmac_get_regs(pdata); void *rxbuffer = priv->rxbuffer; int i;
@@ -330,17 +342,16 @@ static void init_rx_desc(struct calxeda_eth_dev *priv) } }
-static void init_tx_desc(struct calxeda_eth_dev *priv) +static void init_tx_desc(struct eth_pdata *pdata, struct calxeda_eth_dev *priv) {
struct xgmac_regs *regs = (struct xgmac_regs *)priv->dev->iobase;
struct xgmac_regs *regs = xgmac_get_regs(pdata); desc_init_tx_desc(priv->tx_chain, TX_NUM_DESC); writel((ulong)priv->tx_chain, ®s->txdesclist);
}
-static int xgmac_reset(struct eth_device *dev) +static int xgmac_reset(struct xgmac_regs *regs) {
struct xgmac_regs *regs = (struct xgmac_regs *)dev->iobase; int timeout = MAC_TIMEOUT; u32 value;
@@ -356,27 +367,28 @@ static int xgmac_reset(struct eth_device *dev) return timeout; }
-static void xgmac_hwmacaddr(struct eth_device *dev) +static void xgmac_hwmacaddr(struct eth_pdata *pdata) {
struct xgmac_regs *regs = (struct xgmac_regs *)dev->iobase;
struct xgmac_regs *regs = xgmac_get_regs(pdata); u32 macaddr[2];
memcpy(macaddr, dev->enetaddr, 6);
memcpy(macaddr, pdata->enetaddr, ARP_HLEN); writel(macaddr[1], ®s->macaddr[0].hi); writel(macaddr[0], ®s->macaddr[0].lo);
}
-static int xgmac_init(struct eth_device *dev, struct bd_info * bis) +static int xgmac_eth_start(struct udevice *dev) {
struct xgmac_regs *regs = (struct xgmac_regs *)dev->iobase;
struct calxeda_eth_dev *priv = dev->priv;
int value;
struct eth_pdata *pdata = dev_get_plat(dev);
struct xgmac_regs *regs = xgmac_get_regs(pdata);
struct calxeda_eth_dev *priv = dev_get_priv(dev);
u32 value;
if (xgmac_reset(dev) < 0)
return -1;
if (xgmac_reset(regs) < 0)
return -ETIMEDOUT; /* set the hardware MAC address */
xgmac_hwmacaddr(dev);
xgmac_hwmacaddr(pdata); /* set the AXI bus modes */ value = XGMAC_DMA_BUSMODE_ATDS |
@@ -401,8 +413,8 @@ static int xgmac_init(struct eth_device *dev, struct bd_info * bis) writel(value, ®s->flow_control);
/* Initialize the descriptor chains */
init_rx_desc(priv);
init_tx_desc(priv);
init_rx_desc(pdata, priv);
init_tx_desc(pdata, priv); /* must set to 0, or when started up will cause issues */ priv->tx_currdesc = 0;
@@ -425,10 +437,11 @@ static int xgmac_init(struct eth_device *dev, struct bd_info * bis) return 0; }
-static int xgmac_tx(struct eth_device *dev, void *packet, int length) +static int xgmac_tx(struct udevice *dev, void *packet, int length) {
struct xgmac_regs *regs = (struct xgmac_regs *)dev->iobase;
struct calxeda_eth_dev *priv = dev->priv;
struct calxeda_eth_dev *priv = dev_get_priv(dev);
struct eth_pdata *pdata = dev_get_plat(dev);
struct xgmac_regs *regs = xgmac_get_regs(pdata); u32 currdesc = priv->tx_currdesc; struct xgmac_dma_desc *txdesc = &priv->tx_chain[currdesc]; int timeout;
@@ -453,35 +466,45 @@ static int xgmac_tx(struct eth_device *dev, void *packet, int length) return 0; }
-static int xgmac_rx(struct eth_device *dev) +static int xgmac_rx(struct udevice *dev, int flags, uchar **packetp) {
struct xgmac_regs *regs = (struct xgmac_regs *)dev->iobase;
struct calxeda_eth_dev *priv = dev->priv;
struct calxeda_eth_dev *priv = dev_get_priv(dev); u32 currdesc = priv->rx_currdesc; struct xgmac_dma_desc *rxdesc = &priv->rx_chain[currdesc]; int length = 0; /* check if the host has the desc */ if (desc_get_owner(rxdesc))
return -1; /* something bad happened */
return -EAGAIN; /* the MAC is still chewing on it */ length = desc_get_rx_frame_len(rxdesc);
*packetp = desc_get_buf_addr(rxdesc);
net_process_received_packet(desc_get_buf_addr(rxdesc), length);
priv->rx_currdesc = (currdesc + 1) & (RX_NUM_DESC - 1);
return length;
+}
+static int xgmac_free_pkt(struct udevice *dev, uchar *packet, int length) +{
struct eth_pdata *pdata = dev_get_plat(dev);
struct xgmac_regs *regs = xgmac_get_regs(pdata);
struct calxeda_eth_dev *priv = dev_get_priv(dev);
u32 rxdesc = ((char *)packet - priv->rxbuffer) / ETH_BUF_SZ;
struct xgmac_dma_desc *p = &priv->rx_chain[rxdesc]; /* set descriptor back to owned by XGMAC */
desc_set_rx_owner(rxdesc);
desc_set_rx_owner(p); writel(1, ®s->rxpoll);
priv->rx_currdesc = (currdesc + 1) & (RX_NUM_DESC - 1);
return length;
return 0;
}
-static void xgmac_halt(struct eth_device *dev) +static void xgmac_eth_stop(struct udevice *dev) {
struct xgmac_regs *regs = (struct xgmac_regs *)dev->iobase;
struct calxeda_eth_dev *priv = dev->priv;
struct calxeda_eth_dev *priv = dev_get_priv(dev);
struct eth_pdata *pdata = dev_get_plat(dev);
struct xgmac_regs *regs = xgmac_get_regs(pdata); int value; /* Disable TX/RX */
@@ -499,47 +522,88 @@ static void xgmac_halt(struct eth_device *dev) priv->rx_currdesc = 0; }
-int calxedaxgmac_initialize(u32 id, ulong base_addr) +/*
- Changing the MAC address is not a good idea, as the fabric would
- need to know about this as well (it does not learn MAC addresses).
- */
+static int xgmac_eth_write_hwaddr(struct udevice *dev) +{
return -ENOSYS;
+}
+static int xgmac_eth_read_rom_hwaddr(struct udevice *dev) {
struct eth_device *dev;
struct calxeda_eth_dev *priv;
struct xgmac_regs *regs;
struct eth_pdata *pdata = dev_get_plat(dev);
struct xgmac_regs *regs = xgmac_get_regs(pdata); u32 macaddr[2];
regs = (struct xgmac_regs *)base_addr;
/* The MAC address is already configured, so read it from registers. */
macaddr[1] = readl(®s->macaddr[0].hi);
macaddr[0] = readl(®s->macaddr[0].lo);
memcpy(pdata->enetaddr, macaddr, ARP_HLEN);
/* check hardware version */
if (readl(®s->version) != 0x1012)
return -1;
return 0;
+}
dev = malloc(sizeof(*dev));
if (!dev)
return 0;
memset(dev, 0, sizeof(*dev));
+static int xgmac_ofdata_to_platdata(struct udevice *dev) +{
struct eth_pdata *pdata = dev_get_plat(dev);
struct calxeda_eth_dev *priv; /* Structure must be aligned, because it contains the descriptors */ priv = memalign(32, sizeof(*priv));
if (!priv) {
free(dev);
return 0;
if (!priv)
return -ENOMEM;
dev_set_priv(dev, priv);
pdata->iobase = devfdt_get_addr(dev);
if (pdata->iobase == FDT_ADDR_T_NONE) {
printf("%s: Cannot find XGMAC base address\n", __func__);
return -EINVAL;
}
if (pdata->iobase >= (1ULL << 32)) {
printf("%s: MMIO base address cannot be above 4GB\n", __func__);
return -EINVAL; }
dev->iobase = (int)base_addr;
dev->priv = priv;
priv->dev = dev;
sprintf(dev->name, "xgmac%d", id);
return 0;
+}
/* The MAC address is already configured, so read it from registers. */
macaddr[1] = readl(®s->macaddr[0].hi);
macaddr[0] = readl(®s->macaddr[0].lo);
memcpy(dev->enetaddr, macaddr, 6);
+static int xgmac_eth_probe(struct udevice *dev) +{
struct eth_pdata *pdata = dev_get_plat(dev);
struct xgmac_regs *regs = xgmac_get_regs(pdata);
dev->init = xgmac_init;
dev->send = xgmac_tx;
dev->recv = xgmac_rx;
dev->halt = xgmac_halt;
/* check hardware version */
if (readl(®s->version) != 0x1012)
return -ENODEV;
eth_register(dev);
xgmac_eth_read_rom_hwaddr(dev);
return 1;
return 0;
}
+static const struct eth_ops xgmac_eth_ops = {
.start = xgmac_eth_start,
.send = xgmac_tx,
.recv = xgmac_rx,
.free_pkt = xgmac_free_pkt,
.stop = xgmac_eth_stop,
.write_hwaddr = xgmac_eth_write_hwaddr,
.read_rom_hwaddr = xgmac_eth_read_rom_hwaddr,
+};
+static const struct udevice_id xgmac_eth_ids[] = {
{ .compatible = "calxeda,hb-xgmac" },
{ }
+};
+U_BOOT_DRIVER(eth_xgmac) = {
.name = "eth_xgmac",
.id = UCLASS_ETH,
.of_match = xgmac_eth_ids,
.of_to_plat = xgmac_ofdata_to_platdata,
.probe = xgmac_eth_probe,
.ops = &xgmac_eth_ops,
.plat_auto = sizeof(struct eth_pdata),
+}; diff --git a/include/configs/highbank.h b/include/configs/highbank.h index 7f37c81fc9f..fbd26ddd0fc 100644 --- a/include/configs/highbank.h +++ b/include/configs/highbank.h @@ -27,8 +27,6 @@ #define CONFIG_SYS_SCSI_MAX_DEVICE (CONFIG_SYS_SCSI_MAX_SCSI_ID * \ CONFIG_SYS_SCSI_MAX_LUN)
-#define CONFIG_CALXEDA_XGMAC
#define CONFIG_BOOT_RETRY_TIME -1 #define CONFIG_RESET_TO_RETRY
diff --git a/include/netdev.h b/include/netdev.h index 0ad9f8dc3a5..b960c421063 100644 --- a/include/netdev.h +++ b/include/netdev.h @@ -33,7 +33,6 @@ int at91emac_register(struct bd_info *bis, unsigned long iobase); int ax88180_initialize(struct bd_info *bis); int bcm_sf2_eth_register(struct bd_info *bis, u8 dev_num); int bfin_EMAC_initialize(struct bd_info *bis); -int calxedaxgmac_initialize(u32 id, ulong base_addr); int cs8900_initialize(u8 dev_num, int base_addr); int dc21x4x_initialize(struct bd_info *bis); int designware_initialize(ulong base_addr, u32 interface); diff --git a/scripts/config_whitelist.txt b/scripts/config_whitelist.txt index 39e6416afc0..25d25dcc9e9 100644 --- a/scripts/config_whitelist.txt +++ b/scripts/config_whitelist.txt @@ -149,7 +149,6 @@ CONFIG_BTB CONFIG_BUFNO_AUTO_INCR_BIT CONFIG_BUILD_ENVCRC CONFIG_BUS_WIDTH -CONFIG_CALXEDA_XGMAC CONFIG_CDP_APPLIANCE_VLAN_TYPE CONFIG_CDP_CAPABILITIES CONFIG_CDP_DEVICE_ID -- 2.17.5
Nicely done. Thanks. Reviewed-by: Ramon Fried rfried.dev@gmail.com

On Mon, Apr 12, 2021 at 01:04:52AM +0100, Andre Przywara wrote:
To squash that nasty warning message and make better use of the newly gained OF_CONTROL feature, let's convert the calxedagmac driver to the "new" driver model. The conversion is pretty straight forward, mostly just adjusting the use of the involved data structures. The only actual change is the required split of the receive routine into a receive and free_pkt part. Also this allows us to get rid of the hardcoded platform information and explicit init calls.
This also uses the opportunity to wrap the code decoding the MMIO register base address, to make it safe for using PHYS_64BIT later.
Signed-off-by: Andre Przywara andre.przywara@arm.com Reviewed-by: Ramon Fried rfried.dev@gmail.com
Applied to u-boot/master, thanks!

So far we were defining a somewhat confusing PHYS_SDRAM_1_SIZE variable, which originally was only used for setting the memtest boundaries. This definition in highbank.h has been removed about a year ago (moved to Kconfig), so we also don't need the hard-coded size definition any longer.
Get rid of the misleading memory size definition, which was actually wrong anyway (it's 4088 MB for those machines with just 4GB of DRAM).
Signed-off-by: Andre Przywara andre.przywara@arm.com --- include/configs/highbank.h | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/include/configs/highbank.h b/include/configs/highbank.h index fbd26ddd0fc..ff92c4f5540 100644 --- a/include/configs/highbank.h +++ b/include/configs/highbank.h @@ -39,12 +39,6 @@ #define CONFIG_SYS_LOAD_ADDR 0x800000 #define CONFIG_SYS_64BIT_LBA
-/*----------------------------------------------------------------------- - * Physical Memory Map - * The DRAM is already setup, so do not touch the DT node later. - */ -#define PHYS_SDRAM_1_SIZE (4089 << 20) - /* Environment data setup */ #define CONFIG_SYS_NVRAM_BASE_ADDR 0xfff88000 /* NVRAM base address */

On Mon, Apr 12, 2021 at 01:04:53AM +0100, Andre Przywara wrote:
So far we were defining a somewhat confusing PHYS_SDRAM_1_SIZE variable, which originally was only used for setting the memtest boundaries. This definition in highbank.h has been removed about a year ago (moved to Kconfig), so we also don't need the hard-coded size definition any longer.
Get rid of the misleading memory size definition, which was actually wrong anyway (it's 4088 MB for those machines with just 4GB of DRAM).
Signed-off-by: Andre Przywara andre.przywara@arm.com
Applied to u-boot/master, thanks!

So far U-Boot was hard coding a (surely sufficient) memory size of 512 MB, even though all machines out there have at least 4GB of DRAM. Since U-Boot uses its memory knowledge to populate the EFI memory map, we are missing out here, at best losing everything beyond 4GB on Midway boxes (which typically come with 8GB of DRAM).
Since the management processor populated the DT memory node already with the detected DRAM size and configuration, we use that to populate U-Boot's memory bank information, which is the base for the UEFI memory map. This finally allows us to get rid of the NR_DRAM_BANKS=0 hack, that we had in place to avoid U-Boot messing up the DT memory node before loading the kernel.
Also, to cover the whole of memory, we need to enable PHYS_64BIT.
Signed-off-by: Andre Przywara andre.przywara@arm.com --- arch/arm/Kconfig | 1 + board/highbank/highbank.c | 9 +++++++-- configs/highbank_defconfig | 2 +- 3 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 0082d06182a..6b5479b60be 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -757,6 +757,7 @@ config ARCH_HIGHBANK select CLK_CCF select AHCI select DM_ETH + select PHYS_64BIT
config ARCH_INTEGRATOR bool "ARM Ltd. Integrator family" diff --git a/board/highbank/highbank.c b/board/highbank/highbank.c index 0667a48965c..ffb6fd922da 100644 --- a/board/highbank/highbank.c +++ b/board/highbank/highbank.c @@ -8,6 +8,7 @@ #include <cpu_func.h> #include <env.h> #include <fdt_support.h> +#include <fdtdec.h> #include <init.h> #include <net.h> #include <scsi.h> @@ -84,8 +85,12 @@ int misc_init_r(void)
int dram_init(void) { - gd->ram_size = SZ_512M; - return 0; + return fdtdec_setup_mem_size_base(); +} + +int dram_init_banksize(void) +{ + return fdtdec_setup_memory_banksize(); }
#if defined(CONFIG_OF_BOARD_SETUP) diff --git a/configs/highbank_defconfig b/configs/highbank_defconfig index c3352b827d7..5d65049c5c0 100644 --- a/configs/highbank_defconfig +++ b/configs/highbank_defconfig @@ -3,7 +3,7 @@ CONFIG_SYS_DCACHE_OFF=y CONFIG_SYS_THUMB_BUILD=y CONFIG_ARCH_HIGHBANK=y CONFIG_SYS_TEXT_BASE=0x00008000 -CONFIG_NR_DRAM_BANKS=0 +CONFIG_NR_DRAM_BANKS=2 CONFIG_ENV_SIZE=0x2000 CONFIG_SYS_BOOTCOUNT_ADDR=0xfff3cf0c CONFIG_SYS_BOOTCOUNT_SINGLEWORD=y

On Mon, Apr 12, 2021 at 01:04:54AM +0100, Andre Przywara wrote:
So far U-Boot was hard coding a (surely sufficient) memory size of 512 MB, even though all machines out there have at least 4GB of DRAM. Since U-Boot uses its memory knowledge to populate the EFI memory map, we are missing out here, at best losing everything beyond 4GB on Midway boxes (which typically come with 8GB of DRAM).
Since the management processor populated the DT memory node already with the detected DRAM size and configuration, we use that to populate U-Boot's memory bank information, which is the base for the UEFI memory map. This finally allows us to get rid of the NR_DRAM_BANKS=0 hack, that we had in place to avoid U-Boot messing up the DT memory node before loading the kernel.
Also, to cover the whole of memory, we need to enable PHYS_64BIT.
Signed-off-by: Andre Przywara andre.przywara@arm.com
Applied to u-boot/master, thanks!

Rob does not have access to any Calxeda systems anymore, also has expressed a lack of interest in those systems in the past.
I have multiple working Midway nodes under my desk in the office, so am happy to take over maintainership.
Signed-off-by: Andre Przywara andre.przywara@arm.com --- board/highbank/MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/board/highbank/MAINTAINERS b/board/highbank/MAINTAINERS index 69ddeddd600..a542bd1ee64 100644 --- a/board/highbank/MAINTAINERS +++ b/board/highbank/MAINTAINERS @@ -1,5 +1,5 @@ HIGHBANK BOARD -M: Rob Herring robh@kernel.org +M: Andre Przywara andre.przywara@arm.com S: Maintained F: board/highbank/ F: include/configs/highbank.h

On Mon, Apr 12, 2021 at 01:04:55AM +0100, Andre Przywara wrote:
Rob does not have access to any Calxeda systems anymore, also has expressed a lack of interest in those systems in the past.
I have multiple working Midway nodes under my desk in the office, so am happy to take over maintainership.
Signed-off-by: Andre Przywara andre.przywara@arm.com
Applied to u-boot/master, thanks!
participants (3)
-
Andre Przywara
-
Ramon Fried
-
Tom Rini